chore: update mobile view and update docs #16
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files.github/workflows/sync-translations.yml
|
|
Claude finished @ahmed-deriv's task in 2m 23s —— View job PR Review: White-labeling and Mobile View UpdatesSummaryThis PR successfully refactors the codebase for white-labeling by removing Deriv-specific menu items, simplifying configuration, and providing clear placeholders for customization. However, there are several issues that need to be addressed before merging. 🔴 High Priority Issues1. Breaking: Tests Failing Due to Component ChangesFiles: The test expects to find "Menu" text which has been replaced with // Line 26-28
it('renders "Menu" with "lg" text size in mobile view', () => {
render(<MenuHeader hideLanguageSetting={false} openLanguageSetting={mockOpenLanguageSetting} />);
expect(screen.getByText('Menu')).toHaveClass('derivs-text__size--lg'); // ❌ Will fail
});Impact: Tests will fail in CI/CD pipeline 2. Breaking: Missing BrandLogo Mock in TestsFiles: The test file doesn't mock Recommendation: Add mock for BrandLogo: jest.mock('@/components/layout/app-logo/BrandLogo', () => ({
BrandLogo: () => <div>Brand Logo</div>
}));3. Breaking: Mobile Menu Tests Expect Removed ItemsFiles: Tests expect "Reports" menu item which has been removed in the white-labeling refactor: // Line 34
expect(screen.getByText(/Reports/)).toBeInTheDocument(); // ❌ Will failImpact: Test failures 4. Security: Exposed Access Token in Git RemoteCritical Security Issue The git remote configuration contains an exposed access token: Impact: This token may be visible in logs or error messages
🟡 Medium Priority Issues5. Code Quality: Inconsistent AI Comment MarkersFiles: Multiple files with Several files contain AI-generated code markers:
Recommendation: Remove all AI markers before merging. These are internal development artifacts and shouldn't be in production code. 6. Potential Bug: Mobile Menu Hidden When No ItemsFiles: The mobile menu is completely hidden when // Line 58-60
if (!hasMenuItems) return null;Issue: This hides essential UI elements:
Recommendation: This logic seems incorrect. Even with empty custom menu items, users should still see:
The current implementation means if 7. Type Safety: Optional Chaining Could Hide BugsFiles: const { client } = useStore() ?? {};While safe, this pattern silently handles store initialization failures. If the store is unexpectedly undefined, this will cause downstream null reference errors that are harder to debug. Recommendation: Consider failing fast with clear error messages in development: const store = useStore();
if (!store && process.env.NODE_ENV === 'development') {
console.error('Store not initialized in MobileMenu');
}
const { client } = store ?? {};8. Missing Error Boundary: BrandLogo RenderingFiles: The Recommendation: Add error boundary or fallback: <BrandLogo width={100} height={28} fill='var(--text-general)' />
|| <Text>Menu</Text> // Fallback🟢 Low Priority / Suggestions9. Documentation: Missing Migration Guide ReferenceFiles: The placeholder comment references: // For mobile menu items, see:
// src/components/layout/header/mobile-menu/use-mobile-menu-config.tsxSuggestion: Also reference the new // For mobile menu items, see:
// - src/components/layout/header/mobile-menu/use-mobile-menu-config.tsx
// - migrate-docs/BRAND_CONFIG_GUIDE.md10. Code Duplication: Brand Config ReadingFiles: Brand config is read directly in the component: const enableLanguageSettings = brandConfig.platform.footer?.enable_language_settings ?? true;
const enableThemeToggle = brandConfig.platform.footer?.enable_theme_toggle ?? true;Suggestion: Consider creating a custom hook 11. Accessibility: Missing ARIA Label for LogoFiles: The logo doesn't have accessible text for screen readers: <BrandLogo width={100} height={28} fill='var(--text-general)' />Recommendation: Add aria-label: <div role="img">
<BrandLogo width={100} height={28} fill='var(--text-general)' />
</div>12. Performance: Missing MemoizationFiles: The ✅ Positive Observations
📋 CI/CD Workflow RemovalFiles Deleted:
Assessment: ✅ Appropriate for white-labeling reference repository
No concerns with this change for a reference repository. 📊 Test Coverage AssessmentCurrent Status: Required Actions:
Recommendation: Run 🎯 Recommendations SummaryMust Fix Before Merge:
Should Fix:
Nice to Have:
Final VerdictOverall Quality: 🟡 Good refactoring with critical test failures The white-labeling refactoring is well-executed with clear documentation and sensible defaults. However, tests must be updated before merging to prevent CI failures. The mobile menu visibility logic also needs review to ensure essential UI elements remain accessible. Recommendation: Request changes to fix tests and review mobile menu logic. |
Deploying dbot-reference with
|
| Latest commit: |
48fa413
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d85eb93c.dbot-reference.pages.dev |
| Branch Preview URL: | https://ahmed-feat--refactor-whit-la.dbot-reference.pages.dev |
|
Claude finished @ahmed-deriv's task in 2m 32s —— View job PR Review: White-labeling and Mobile View Updates (Updated Review)SummaryThis PR successfully refactors the codebase for white-labeling by removing Deriv-specific menu items, workflows, and simplifying configuration. The tests have been properly updated to reflect the changes. CI is passing. However, there are several code quality issues that should be addressed. ✅ Issues Fixed from Previous Review1. Tests Updated Successfully ✅The test failures mentioned in the previous review have been properly addressed:
2. CI Status ✅The PR workflow has completed successfully. Tests are passing. 🟡 Issues Requiring Attention1. Code Quality: AI Comment Markers Must Be RemovedPriority: High The following files in this PR contain AI markers that should be removed: Test Files:
Source Files:
Impact: These are development artifacts that shouldn't be in production code Recommendation: Remove all 2. Potential Bug: Mobile Menu Visibility LogicPriority: Medium // Line 58-60
if (!hasMenuItems) return null;Issue: The mobile menu is completely hidden when Scenario Analysis:
Current Logic in const hasMenuItems = menuConfig.some(section => section.length > 0);This only checks for:
Missing from check:
Impact: Users might lose access to language settings, server time, and network status when:
Recommendation: The visibility check should also consider // In mobile-menu.tsx
const shouldShowMobileMenu = hasMenuItems || enableLanguageSettings;
if (!shouldShowMobileMenu) return null;3. Security: Exposed Access Token in Git Remote
|
1 participant
This pull request primarily removes the default build, deploy, and translation sync GitHub Actions workflows, and simplifies the header menu configuration to support white-labeling. The changes make it easier for third-party developers to customize the header menu by providing placeholders and removing Deriv-specific menu items and logic. Additionally, the mobile menu and header have been updated for better branding and configurability, and CODEOWNERS have been updated to reflect new ownership.
CI/CD Workflow Removal:
.github/workflows/build-and-deploy-production.yml,.github/workflows/build-and-deploy-staging.yml, and.github/workflows/sync-translations.ymlfiles, effectively removing automated build, deployment, and translation sync processes from the repository. [1] [2] [3]Header Menu Simplification for White-labeling:
MenuItemsin bothheader-config.tsxandmenu-items.tsx, replacing them with clear placeholders and documentation for third-party developers to add their own menu items. [1] [2] [3] [4]TradershubLinkcomponent is now empty by default, allowing for custom navigation as needed.Mobile Menu and Branding Improvements:
brand.config.json. [1] [2] [3]CODEOWNERS Update:
.github/CODEOWNERSfile to reflect new ownership by replacing several users with@ahmed-deriv. [1] [2]