Phase 6.3 - UI restructure#53
Conversation
Phase 06.3: UI Structure Refactor - Implementation decisions documented - Phase boundary established - Design mockups created in Pencil Key structural decisions: - Sidebar becomes app navigation (Files, Settings), not folder tree - Folder navigation via click-to-enter and [..] PARENT_DIR - Fixed footer with copyright, links, and status indicator - Toolbar with breadcrumbs left, actions right - File list with bracketed sortable column headers Also: Add .planning/ to markdownlint ignore (uses custom heading structure with HTML-like section tags) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This reverts commit 0e90fb9.
Phase 06.3: UI Structure Refactor - Implementation decisions documented - Phase boundary established - Design mockups created in Pencil Key structural decisions: - Sidebar becomes app navigation (Files, Settings), not folder tree - Folder navigation via click-to-enter and [..] PARENT_DIR - Fixed footer with copyright, links, and status indicator - Toolbar with breadcrumbs left, actions right - File list with bracketed sortable column headers Also fixes 06.2-CONTEXT.md to pass markdownlint (MD001 heading increment) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 6.3: UI Structure Refactor - Standard stack identified (no new dependencies) - Architecture patterns documented (AppShell, URL-based navigation) - Component inventory created (9 new, 7 modify, 3 remove) - Pitfalls catalogued (5 common issues) - Design specs extracted from Pencil file Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 06.3: UI Structure Refactor - 5 plans in 3 waves - Wave 1: AppShell layout components (Plan 01) - Wave 2: Routing + File list changes (Plans 02, 03) - parallel - Wave 3: FileBrowser integration + Verification (Plans 04, 05) Key changes: - Fixed AppShell layout (header, sidebar, footer) - URL-based folder navigation (/files/:folderId) - In-place navigation with [..] PARENT_DIR row - 3-column file list (NAME, SIZE, MODIFIED) - FolderTree deprecated, replaced by app-level nav sidebar Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add CSS Grid layout system in layout.css with header/sidebar/main/footer areas - Sidebar fixed at 180px, main area scrollable, 768px mobile breakpoint - Create AppShell wrapper component composing header, sidebar, main, footer - Add stub header, sidebar, footer components (fully implemented in later tasks) - Include --color-nav-active token for active nav item background
- Create UserMenu with hover-triggered dropdown for settings/logout - Create StatusIndicator with connection status using health check hook - Update AppHeader with logo and UserMenu component - Update AppFooter with copyright, links (help/privacy/terms/github), status - Terminal style: bracket-wrapped labels [settings] [logout] [CONNECTED]
- Create NavItem with terminal-style icons [DIR] and [CFG] - Create StorageQuota with progress bar using quota store - Update AppSidebar with Files/Settings navigation and active states - Add index.ts barrel file exporting all 8 layout components - Active nav detection uses useLocation from react-router-dom
Tasks completed: 3/3 - Create Layout CSS and AppShell Component - Create Header and Footer Components - Create Sidebar Components SUMMARY: .planning/phases/06.3-ui-structure-refactor/06.3-01-SUMMARY.md
- Add ParentDirRow component for [..] PARENT_DIR navigation - Update FileList headers to bracket format [NAME] [SIZE] [MODIFIED] - Remove TYPE column from header and grid layout - Add showParentRow/onNavigateUp props to FileList - Update CSS grid from 4 columns to 3 columns
- Create FilesPage.tsx wrapping FileBrowser in AppShell - Create SettingsPage.tsx wrapping LinkedMethods in AppShell - Update routes: /files/:folderId? pattern, /dashboard redirect - Use ParentDirRow in FileList for [..] PARENT_DIR navigation - Adjust file list grid to 3 columns (removed TYPE column) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove TYPE column display from FileListItem - Remove getFileExtension helper (no longer needed) - Update Breadcrumbs to path format ~/root/path - Remove back button (parent nav via [..] row now) - Update breadcrumbs.css for path format display
- Replace useState(currentFolderId) with useParams from react-router - navigateTo uses navigate() for URL-based history - Browser back/forward now works for folder navigation - Root folder maps to /files, subfolders to /files/:folderId Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add terminal-style ASCII folder art to empty state - Update message to '// EMPTY DIRECTORY' format - Add lowercase hint 'drag files here or use upload' - Add CSS styles for ASCII art display
- Add import for layout.css in App.css - Mark dashboard-container and dashboard-header as deprecated - Mark api-status styles as deprecated (replaced by StatusIndicator) - Add deprecation comment to Dashboard.tsx component - Add settings page styles Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tasks completed: 3/3 - Create ParentDirRow and update FileList structure - Update FileListItem and Breadcrumbs - Update EmptyState with ASCII art SUMMARY: .planning/phases/06.3-ui-structure-refactor/06.3-03-SUMMARY.md
Tasks completed: 3/3 - Update routes with /files/:folderId? pattern - Update useFolderNavigation for URL-based state - Update App.css and deprecate old dashboard styles SUMMARY: .planning/phases/06.3-ui-structure-refactor/06.3-02-SUMMARY.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove FolderTree import and sidebar component - Remove mobile sidebar state (sidebarOpen, isMobile, toggleSidebar, closeSidebar) - Remove window resize listener for mobile detection - Remove handleDrop callback (was for sidebar drag-drop) - Remove moveItem from useFolder hook (unused) - Add showParentRow and onNavigateUp props to FileList - Simplify layout from file-browser to file-browser-content - Update component docstring for new navigation pattern
- Add @deprecated JSDoc to FolderTree.tsx (in-place navigation replaces it) - Add @deprecated JSDoc to FolderTreeNode.tsx - Add @deprecated JSDoc to ApiStatusIndicator.tsx (replaced by StatusIndicator) - Update responsive.css for AppShell mobile layout - Remove old file-browser sidebar mobile styles (now deprecated) - Add app-shell single column layout for mobile (768px breakpoint) - Add app-sidebar fixed overlay with translateX animation - Simplify file list to 2-column on mobile (date column hidden)
- Deprecate old file-browser sidebar layout styles - Deprecate folder-tree styles (replaced by in-place navigation) - Add file-browser-content container styles - Export ParentDirRow from index.ts barrel - Export CreateFolderDialog from index.ts - Move FolderTree/FolderTreeNode exports to deprecated section
Tasks completed: 3/3 - Update FileBrowser to remove FolderTree - Mark deprecated components and update responsive styles - Update file-browser.css and final integration SUMMARY: .planning/phases/06.3-ui-structure-refactor/06.3-04-SUMMARY.md
Tasks completed: 3/3 - Task 1: Run application and test core functionality (verification) - Task 2: Visual verification checkpoint (Playwright MCP - all passed) - Task 3: Apply style fixes if needed (skipped - no issues) Phase 6.3 UI Structure Refactor complete: - AppShell layout matches Pencil design - URL-based folder navigation with browser history - In-place navigation via [..] PARENT_DIR row - Mobile responsive layout verified - All file operations working SUMMARY: .planning/phases/06.3-ui-structure-refactor/06.3-05-SUMMARY.md
Phase 6.3: UI Structure Refactor - Design Research - Extracted design tokens from Pencil frame nRzxj - Documented color palette including missing #001108 toolbar bg - Identified toolbar, breadcrumb, button, nav styling issues - Created CSS fix specifications with frame ID references - Listed missing designs (empty state, hover states) - Fixed markdown lint issues in verification doc Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Implement MoveDialogPage to handle file and folder movement interactions. - Update BreadcrumbsPage to improve navigation and interaction. - Enhance ContextMenuPage with Move option functionality. - Introduce ParentDirPage for upward navigation in file browser. - Refactor full workflow tests to utilize new move functionality via context menu and drag-drop. - Deprecate FolderTreePage as navigation is now handled through breadcrumbs and parent directory row. - Update Playwright config to support external mock IPNS service.
WalkthroughAdds Phase 6.3 UI Structure Refactor: a CSS‑Grid AppShell and eight layout components, URL-driven /files/:folderId routing, in-place parent-directory navigation, drag/drop + MoveDialog move flow, file‑browser 3‑column refactor, many planning/design/verification docs, and E2E/page‑object updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Router as React Router
participant FilesPage
participant AppShell
participant FileBrowser
participant FileList
participant MoveDialog
participant Store
User->>Router: Navigate to /files/:folderId
Router->>FilesPage: render with folderId
FilesPage->>AppShell: wrap content
AppShell->>FileBrowser: render
FileBrowser->>FileList: render items + ParentDirRow
User->>FileList: open context menu on item
FileList->>FileBrowser: request MoveDialog
FileBrowser->>MoveDialog: open
MoveDialog->>Store: fetch folders
Store-->>MoveDialog: folder list
User->>MoveDialog: select dest & confirm
MoveDialog->>FileBrowser: onConfirm(dest)
FileBrowser->>Store: move item
Store-->>FileBrowser: success
FileBrowser->>Router: update/navigate if needed
FileBrowser-->>FileList: refresh
sequenceDiagram
actor User
participant FileList
participant Router as React Router
participant useFolderNav as useFolderNavigation
participant Breadcrumbs
User->>FileList: double-click folder
FileList->>Router: navigate to /files/newFolderId
Router->>useFolderNav: useParams -> currentFolderId
useFolderNav-->>FileList: new folder content
FileList->>Breadcrumbs: update path
User->>Breadcrumbs: drag file onto breadcrumb segment
Breadcrumbs->>FileList: onDrop(source,dest)
FileList->>Router: navigate to /files/destFolderId (if needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/styles/responsive.css (1)
30-37:⚠️ Potential issue | 🟡 MinorRemove unused
fadeInkeyframes.The
fadeInkeyframes defined at lines 30-37 are not referenced anywhere in the codebase and should be removed.
🤖 Fix all issues with AI agents
In @.planning/phases/06.3-ui-structure-refactor/06.3-VERIFICATION.md:
- Line 38: Update the verification table entry for the AppFooter component to
use the correct capitalization "GitHub" in the links description: find the row
referencing AppFooter.tsx and change "github links" to "GitHub links" (the
component name AppFooter and the StatusIndicator mention can help locate the
correct table row).
In `@apps/web/src/App.css`:
- Around line 188-203: The .settings-section CSS uses
var(--color-background-dark) which isn't defined; update the property to include
a fallback so the background won't become invalid—replace the value in the
.settings-section rule (background-color: var(--color-background-dark)) with a
fallback expression like var(--color-background-dark, var(--color-background))
so it falls back to --color-background when --color-background-dark is
undefined.
In `@apps/web/src/components/file-browser/Breadcrumbs.tsx`:
- Around line 9-10: BreadcrumbsProps currently declares a required onNavigateUp
prop but the Breadcrumbs component signature (export function Breadcrumbs({
breadcrumbs, onNavigate, onDrop }: BreadcrumbsProps)) doesn't destructure or use
it; either remove onNavigateUp from BreadcrumbsProps if it's truly unused, or
make it optional (onNavigateUp?: () => void) and add it to the component
destructure (or use it where needed) so callers aren't forced to pass an unused
prop—update the BreadcrumbsProps interface and the Breadcrumbs component
signature (and any callers) accordingly.
In `@apps/web/src/components/layout/UserMenu.tsx`:
- Around line 21-43: The dropdown is only opened via onMouseEnter/onMouseLeave
which blocks keyboard users; update the UserMenu component to toggle visibility
via the button and support keyboard interaction by wiring the trigger button
(className "user-menu-trigger") to call setIsOpen(!isOpen) onClick and handle
key events (Enter/Space to toggle, Escape to close) and ensure the menu
container reflects isOpen state; also add appropriate ARIA attributes
(aria-haspopup, aria-expanded on the trigger and role="menu"/role="menuitem" on
the dropdown and items) and keep existing onMouseEnter/onMouseLeave behavior so
mouse still works; maintain handleLogout for the logout button.
In `@apps/web/src/styles/layout.css`:
- Around line 317-319: The mobile layout hides .app-sidebar making Files
inaccessible and UserMenu uses hover-only handlers; implement a touch-friendly
navigation for screens <=768px by adding a responsive hamburger/drawer component
(e.g., MobileNav or AppDrawer) and toggle it with a button rendered when
.app-sidebar is hidden; move or duplicate the Files and Settings links into this
MobileNav and ensure the UserMenu component stops relying solely on
onMouseEnter/onMouseLeave and supports click/touch toggling (e.g., onClick and
aria-expanded state) so the drawer and user menu both open/close via tap and
keyboard.
🧹 Nitpick comments (23)
apps/web/src/components/layout/StatusIndicator.tsx (1)
33-38: Consider adding accessibility attributes for screen readers.The status indicator conveys important connection state but lacks accessibility support. Screen reader users won't be notified of status changes.
♿ Suggested accessibility improvement
return ( - <div className="status-indicator" data-testid="status-indicator"> + <div className="status-indicator" data-testid="status-indicator" role="status" aria-live="polite"> <span className={dotClass} /> - <span className="status-indicator-text">{statusText}</span> + <span className="status-indicator-text" aria-label={`API status: ${statusText.replace(/[[\]]/g, '')}`}> + {statusText} + </span> </div> );apps/web/src/components/layout/StorageQuota.tsx (1)
6-16: DuplicateformatBytesfunction already exists in utils.This helper duplicates
formatBytesfromapps/web/src/utils/format.ts. The existing utility also handles trailing zeros more cleanly. Reuse it to avoid duplication.♻️ Proposed fix to reuse existing utility
import { useQuotaStore } from '../../stores/quota.store'; +import { formatBytes } from '../../utils/format'; -/** - * Format bytes to human-readable string. - */ -function formatBytes(bytes: number): string { - if (bytes === 0) return '0 B'; - - const units = ['B', 'KB', 'MB', 'GB', 'TB']; - const k = 1024; - const i = Math.floor(Math.log(bytes) / Math.log(k)); - const value = bytes / Math.pow(k, i); - - // Use 1 decimal place for larger units - return `${value.toFixed(i > 1 ? 1 : 0)} ${units[i]}`; -} - /** * Storage quota indicator component.apps/web/src/styles/layout.css (1)
22-23: Consider using100dvhfor better mobile browser compatibility.The research document (06.3-RESEARCH.md, line 234) explicitly warns about the
100vhpitfall on mobile browsers where the address bar causes layout issues. Consider usingheight: 100dvhwith a fallback:height: 100vh; /* fallback */ height: 100dvh;apps/web/src/styles/dialogs.css (1)
218-221: Add a visible focus indicator for keyboard users.Right now focus removes the outline and only sets a hover-like background. Consider a :focus-visible outline to make keyboard focus clearer.
♿ Suggested focus-visible styling
.move-dialog-folder-item:focus { outline: none; background-color: var(--color-green-darker); } + +.move-dialog-folder-item:focus-visible { + outline: 2px solid var(--color-green-primary); + outline-offset: 2px; +}As per coding guidelines, focus on accessibility (a11y) concerns.
apps/web/src/components/layout/AppFooter.tsx (1)
14-22: Placeholder links withhref="#"are an accessibility anti-pattern.Using
href="#"causes the page to scroll to the top when clicked, which is confusing for users. If these links are not yet implemented, consider:
- Using
<button>elements styled as links (if they'll trigger actions)- Disabling them visually and for assistive tech until implemented
- Using
hrefwith actual routes even if pages don't exist yet♻️ Suggested approach using buttons for unimplemented actions
<div className="footer-center"> - <a href="#" className="footer-link"> + <button type="button" className="footer-link" disabled> [help] - </a> - <a href="#" className="footer-link"> + </button> + <button type="button" className="footer-link" disabled> [privacy] - </a> - <a href="#" className="footer-link"> + </button> + <button type="button" className="footer-link" disabled> [terms] - </a> + </button>tests/e2e/page-objects/file-browser/context-menu.page.ts (1)
19-31: JSDoc comments are mismatched with their methods.The comments appear to be incorrectly paired:
- Line 20: Says "Download menu option" but the method is
renameOption()- Line 27: Says "Rename menu option" but the method is
deleteOption()This appears to be a pre-existing issue, but since the file is being modified, consider fixing these misleading comments.
📝 Fix JSDoc comments
/** - * Get the Download menu option (files only). + * Get the Rename menu option. */ renameOption(): Locator { return this.menu().locator('button[role="menuitem"]', { hasText: 'Rename' }); } /** - * Get the Rename menu option. + * Get the Delete menu option. */ deleteOption(): Locator { return this.menu().locator('button[role="menuitem"]', { hasText: 'Delete' }); }apps/web/src/routes/SettingsPage.tsx (1)
23-29: Consider adding ARIA attributes to the loading state for accessibility.The loading state works correctly, but adding
role="status"andaria-live="polite"would improve screen reader support.♿ Proposed accessibility improvement
if (isLoading) { return ( <AppShell> - <div className="loading">Loading...</div> + <div className="loading" role="status" aria-live="polite">Loading...</div> </AppShell> ); }.planning/phases/06.3-ui-structure-refactor/DESIGN-FIXES.md (1)
6-7: Consider using a relative path for the design source reference.The absolute path
/Users/michael/Code/cipher-box/designs/cipher-box-design.penis developer-specific. Using a relative path likedesigns/cipher-box-design.penwould be more portable for other contributors.📝 Proposed fix
-**Design Source:** `/Users/michael/Code/cipher-box/designs/cipher-box-design.pen` +**Design Source:** `designs/cipher-box-design.pen`apps/web/src/components/file-browser/ParentDirRow.tsx (1)
24-32: Consider adding Space key support for keyboard accessibility.The component correctly handles Enter key for activation, but interactive elements typically also respond to Space key per WCAG guidelines.
♻️ Suggested improvement
onKeyDown={(e) => e.key === 'Enter' && onClick()} + onKeyDown={(e) => (e.key === 'Enter' || e.key === ' ') && onClick()}If adding Space, also call
e.preventDefault()to prevent page scrolling:- onKeyDown={(e) => e.key === 'Enter' && onClick()} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } + }}apps/web/src/components/file-browser/MoveDialog.tsx (1)
64-72: Consider caching depth calculations for performance.
getDepthtraverses the parent chain and is called twice per folder during the sort operation (once in comparator, once in the loop at Line 72). For large folder trees, pre-computing depths could improve performance.♻️ Optional optimization
+ // Pre-compute depths to avoid redundant traversals + const depthCache = new Map<string, number>(); + for (const folder of folderEntries) { + depthCache.set(folder.id, getDepth(folder.id, folders)); + } + // Sort by depth then name for predictable display folderEntries.sort((a, b) => { - const depthA = getDepth(a.id, folders); - const depthB = getDepth(b.id, folders); + const depthA = depthCache.get(a.id)!; + const depthB = depthCache.get(b.id)!; if (depthA !== depthB) return depthA - depthB; return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' }); }); for (const folder of folderEntries) { - const depth = getDepth(folder.id, folders); + const depth = depthCache.get(folder.id)!;apps/web/src/styles/responsive.css (1)
170-172: Consider removing deprecated folder-tree-item styles.The
.folder-tree-item:activestyle targets the deprecated FolderTree component. Per the PR changes, FolderTree is marked deprecated and planned for removal. This CSS may become dead code.apps/web/src/hooks/useFolderNavigation.ts (2)
119-130: Memoization ineffective due togetState()snapshot.
useVaultStore.getState()is called at render time (Line 120), returning a new object reference each render. This breaks memoization forbreadcrumbs(Line 128-130) sincevaultStateis always a new reference.Consider subscribing to specific vault state values instead:
♻️ Proposed fix
- // Get vault state snapshot for helper functions - const vaultState = useVaultStore.getState(); + // Get vault state snapshot for helper functions (stable reference via useMemo) + const vaultState = useMemo( + () => ({ rootFolderKey, rootIpnsKeypair, rootIpnsName }), + [rootFolderKey, rootIpnsKeypair, rootIpnsName] + );This creates a stable reference that only changes when the subscribed values change.
169-174: Stale closure in setTimeout placeholder.The
targetFolderreference is captured whennavigateTois called, but the setTimeout callback executes 100ms later. If the folder state changes during that window,setFolderwrites stale data.Since this is noted as a placeholder for Phase 7, consider adding a TODO comment to address this when implementing actual folder loading.
.planning/phases/06.3-ui-structure-refactor/06.3-CONTEXT.md (1)
24-24: Minor: Consider capitalizing "GitHub" or note the intentional lowercase.Static analysis flagged
[github]should be[GitHub]. However, given the terminal aesthetic with lowercase links ([help] [privacy] [terms]), the lowercase appears intentional for consistency. Consider adding a brief note that lowercase is intentional, or capitalize it if brand consistency is preferred.apps/web/src/components/file-browser/FileListItem.tsx (2)
233-249: Consider validating parsed drag data shape at runtime.The
JSON.parseresult is type-asserted without runtime validation. If malformed data is dropped from external sources, accessingparsed.id,parsed.type, orparsed.parentIdcould lead to unexpected behavior.🛡️ Proposed validation
try { const data = e.dataTransfer.getData('application/json'); if (!data) return; const parsed = JSON.parse(data) as { id: string; type: 'file' | 'folder'; parentId: string; }; + // Validate expected shape + if ( + typeof parsed.id !== 'string' || + (parsed.type !== 'file' && parsed.type !== 'folder') || + typeof parsed.parentId !== 'string' + ) { + return; + } + // Don't allow dropping onto self if (parsed.id === item.id) return;
290-296: Consider adding Space key support for keyboard accessibility.The component handles
Enterfor keyboard navigation but notSpace. For full keyboard accessibility on interactive elements withtabIndex={0}, both keys should typically trigger activation.♿ Proposed fix
onKeyDown={(e) => { - if (e.key === 'Enter') { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); // Prevent page scroll on Space if (isFolder(item)) { onNavigate(item.id); } } }}apps/web/src/styles/file-browser.css (1)
150-152: Consider using a CSS variable for the secondary button hover glow.Line 151 uses a hardcoded
rgba(0, 208, 132, 0.2)while other glows usevar(--glow-green). For consistency and easier theme maintenance, consider defining a secondary glow variable.🎨 Proposed fix
If a
--glow-green-subtlevariable exists or can be added to your design tokens:.toolbar-btn--secondary:hover:not(:disabled) { - box-shadow: 0 0 8px rgba(0, 208, 132, 0.2); + box-shadow: var(--glow-green-subtle, 0 0 8px rgba(0, 208, 132, 0.2)); }tests/e2e/page-objects/dialogs/move-dialog.page.ts (1)
128-130: The?? falsemay be redundant.Playwright's
isDisabled()returnsPromise<boolean>, so the null coalescing operator?? falseis likely unnecessary. However, keeping it as defensive code is acceptable if you prefer consistency with other state-check methods.♻️ Optional simplification
async isMoveDisabled(): Promise<boolean> { - return (await this.moveButton().isDisabled()) ?? false; + return await this.moveButton().isDisabled(); }apps/web/src/components/file-browser/FileBrowser.tsx (1)
112-127: Consider structured error handling instead of logging raw error objects.The
console.errorstatements log the full error object which could potentially include sensitive information in stack traces or error messages (e.g., folder IDs, user context). While this is likely acceptable for development, consider using a more structured error logging approach that sanitizes or limits what's logged.} catch (err) { - console.error('Move failed:', err); + console.error('Move failed:', err instanceof Error ? err.message : 'Unknown error'); }tests/e2e/tests/full-workflow.spec.ts (2)
158-164: Fixed timeout could cause flakiness in CI.Using
page.waitForTimeout(500)is a test smell that can cause flaky tests. Consider waiting for a specific condition instead.async function navigateBack(): Promise<void> { await parentDir.click(); - await page.waitForTimeout(500); // Wait for navigation + // Wait for navigation to complete by checking URL or breadcrumb change + await page.waitForURL(/.*files/); if (navigationStack.length > 1) { navigationStack.pop(); } }
171-197: Multiple fixed timeouts in navigation loop.The
navigateToRootfunction uses fixed timeouts which can be unreliable across different CI environments. Consider using explicit waits for state changes.♻️ Suggested improvement
async function navigateToRoot(): Promise<void> { const maxIterations = 10; for (let i = 0; i < maxIterations; i++) { const parentDirRow = page.locator('[data-testid="parent-dir-row"]'); const isVisible = await parentDirRow.isVisible().catch(() => false); if (!isVisible) { break; } await parentDirRow.click(); - await page.waitForTimeout(300); // Wait for navigation + // Wait for the file list to update after navigation + await page.locator('.file-list').waitFor({ state: 'visible' }); } // ... rest of function }tests/e2e/page-objects/file-browser/breadcrumbs.page.ts (2)
24-28: Low risk: RegExp from variable input in test context.Static analysis flagged potential ReDoS vulnerability. While folder names in E2E tests are controlled data (from test constants), escaping special regex characters would be safer and prevent issues if test data ever includes characters like
(,),[,], etc.♻️ Suggested fix to escape regex special characters
+ /** + * Escape special regex characters in a string. + */ + private escapeRegex(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + } + /** * Get a breadcrumb item by folder name. */ getBreadcrumbItem(name: string): Locator { return this.container().locator('.breadcrumb-item', { - hasText: new RegExp(`^${name}$`, 'i'), + hasText: new RegExp(`^${this.escapeRegex(name)}$`, 'i'), }); }
107-111: Same RegExp concern applies here.The
waitForPathToContainmethod also constructs a RegExp from thefolderNameparameter. Consider using the same escape utility if you implement it.async waitForPathToContain(folderName: string, options?: { timeout?: number }): Promise<void> { await this.container() - .locator('.breadcrumb-item', { hasText: new RegExp(folderName, 'i') }) + .locator('.breadcrumb-item', { hasText: new RegExp(this.escapeRegex(folderName), 'i') }) .waitFor({ state: 'visible', ...options }); }
| .app-sidebar { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's search for mobile navigation patterns and component files
rg -n -i '(hamburger|mobile.*nav|drawer|slide.*menu|mobile-nav|MobileNav|Drawer|mobile.*menu)' --type=tsx --type=css -g '!node_modules' | head -50Repository: FSM1/cipher-box
Length of output: 86
🏁 Script executed:
# Also check the layout.css file for context around lines 317-319
sed -n '300,330p' apps/web/src/styles/layout.cssRepository: FSM1/cipher-box
Length of output: 666
🏁 Script executed:
# Look for layout/sidebar/nav related React components
fd -e tsx -e jsx -g '*nav*' -g '*drawer*' -g '*sidebar*' -g '*layout*' apps/web/src | head -30Repository: FSM1/cipher-box
Length of output: 284
🏁 Script executed:
# Search for mobile navigation patterns with correct rg syntax
rg -i '(hamburger|drawer|mobile.*nav|slide.*menu)' apps/web/src --type-add 'web:*.tsx' -t web -t css | head -40Repository: FSM1/cipher-box
Length of output: 41
🏁 Script executed:
# Look at the layout component structure
find apps/web/src/components/layout -type f -name "*.tsx" -o -name "*.ts" -o -name "*.css" | sortRepository: FSM1/cipher-box
Length of output: 464
🏁 Script executed:
# Check the main app or layout file for navigation handling
rg -n 'sidebar|Sidebar|nav|Nav|drawer|Drawer' apps/web/src/components/layout --type-add 'web:*.tsx' -t web -A 2 | head -60Repository: FSM1/cipher-box
Length of output: 3810
🏁 Script executed:
# Look at the overall app structure
find apps/web/src -maxdepth 2 -type f \( -name "*.tsx" -o -name "*.ts" \) | grep -i 'app\|layout\|nav' | sortRepository: FSM1/cipher-box
Length of output: 1317
🏁 Script executed:
# Check the AppHeader component
cat -n apps/web/src/components/layout/AppHeader.tsxRepository: FSM1/cipher-box
Length of output: 655
🏁 Script executed:
# Check the AppShell component
cat -n apps/web/src/components/layout/AppShell.tsxRepository: FSM1/cipher-box
Length of output: 880
🏁 Script executed:
# Check for any mobile nav or drawer patterns in the entire layout.css
grep -n -i 'mobile\|responsive\|drawer\|hamburger\|menu' apps/web/src/styles/layout.css | head -30Repository: FSM1/cipher-box
Length of output: 336
🏁 Script executed:
# Look at the App.tsx to understand overall structure
cat -n apps/web/src/App.tsx | head -100Repository: FSM1/cipher-box
Length of output: 237
🏁 Script executed:
# Get the full mobile responsive section of layout.css
sed -n '305,370p' apps/web/src/styles/layout.cssRepository: FSM1/cipher-box
Length of output: 544
🏁 Script executed:
# Check if there are any aria-labels or accessibility features suggesting mobile nav
rg -n 'aria-\|role=' apps/web/src/components/layout/*.tsx -A 1Repository: FSM1/cipher-box
Length of output: 41
🏁 Script executed:
# Look for any menu toggle or state management related to mobile nav
rg -n 'useState\|useRef' apps/web/src/components/layout/*.tsxRepository: FSM1/cipher-box
Length of output: 41
🏁 Script executed:
# Check if the UserMenu provides access to Files/Settings
cat -n apps/web/src/components/layout/UserMenu.tsxRepository: FSM1/cipher-box
Length of output: 1601
🏁 Script executed:
# Look at the full layout.css to understand the grid structure and see if mobile nav is elsewhere
cat -n apps/web/src/styles/layout.css | head -100Repository: FSM1/cipher-box
Length of output: 3068
🏁 Script executed:
# Check full AppSidebar to confirm navigation structure
cat -n apps/web/src/components/layout/AppSidebar.tsxRepository: FSM1/cipher-box
Length of output: 1114
🏁 Script executed:
# Verify FilesPage and Settings routes are the main app sections
rg -n 'Files\|Settings' apps/web/src/routes/index.tsx -B 2 -A 2Repository: FSM1/cipher-box
Length of output: 41
🏁 Script executed:
# Check the routes structure
cat -n apps/web/src/routes/index.tsxRepository: FSM1/cipher-box
Length of output: 749
🏁 Script executed:
# Look for any mobile-specific components or pages
fd -type f -name "*.tsx" apps/web/src | xargs rg -l 'mobile\|Mobile' 2>/dev/nullRepository: FSM1/cipher-box
Length of output: 231
🏁 Script executed:
# Search more carefully for any mobile nav implementation or alternative access to Files
rg -n '/files' apps/web/src --type-add 'web:*.tsx' -t web -B 2 -A 2Repository: FSM1/cipher-box
Length of output: 1603
🏁 Script executed:
# Check if UserMenu handles mobile or if there's any touch event handling
rg -n 'onClick\|onTouchStart\|onTouchEnd\|mobile\|Mobile' apps/web/src/components/layout/UserMenu.tsxRepository: FSM1/cipher-box
Length of output: 41
🏁 Script executed:
# Look for any mobile-specific menu or nav in the header
cat -n apps/web/src/styles/layout.css | tail -50Repository: FSM1/cipher-box
Length of output: 1420
Mobile navigation is completely inaccessible on small screens.
The sidebar containing the Files link is hidden on mobile devices (≤768px) with no alternative navigation pattern. Files—the primary app feature—is only accessible via the sidebar and becomes unreachable on mobile. Additionally, the UserMenu relies on hover events (onMouseEnter/onMouseLeave), which do not work on touch devices.
Implement a mobile-friendly navigation pattern (hamburger menu, bottom navigation, or drawer) that provides access to both Files and Settings on screens below 768px. Ensure the menu works with both click and touch interactions.
🤖 Prompt for AI Agents
In `@apps/web/src/styles/layout.css` around lines 317 - 319, The mobile layout
hides .app-sidebar making Files inaccessible and UserMenu uses hover-only
handlers; implement a touch-friendly navigation for screens <=768px by adding a
responsive hamburger/drawer component (e.g., MobileNav or AppDrawer) and toggle
it with a button rendered when .app-sidebar is hidden; move or duplicate the
Files and Settings links into this MobileNav and ensure the UserMenu component
stops relying solely on onMouseEnter/onMouseLeave and supports click/touch
toggling (e.g., onClick and aria-expanded state) so the drawer and user menu
both open/close via tap and keyboard.
There was a problem hiding this comment.
Pull request overview
Restructures the Phase 6.3 UI to use an AppShell layout (header/sidebar/footer), URL-based folder routing, and updated file-browser navigation patterns (breadcrumbs + [..] PARENT_DIR), with corresponding E2E updates.
Changes:
- Introduces AppShell layout components and CSS, and wraps authenticated pages in the new shell.
- Updates file browser navigation (inline folder navigation + interactive breadcrumbs) and adds a Move dialog flow.
- Updates Playwright E2E tests + page objects to match the new UI structure and selectors.
Reviewed changes
Copilot reviewed 62 out of 63 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/tests/full-workflow.spec.ts | Updates full workflow E2E to use new navigation patterns (breadcrumbs + parent dir row + user menu logout). |
| tests/e2e/playwright.config.ts | Allows configurable mock IPNS server URL and always reuses existing server if available. |
| tests/e2e/page-objects/file-browser/parent-dir.page.ts | Adds page object for the new [..] PARENT_DIR row. |
| tests/e2e/page-objects/file-browser/index.ts | Updates barrel exports to include ParentDirPage and deprecate FolderTreePage usage. |
| tests/e2e/page-objects/file-browser/folder-tree.page.ts | Marks FolderTreePage as deprecated (tree removed in Phase 6.3). |
| tests/e2e/page-objects/file-browser/file-list.page.ts | Adds helper for drag-to-folder interactions in file list. |
| tests/e2e/page-objects/file-browser/context-menu.page.ts | Adds Move option support to context menu page object. |
| tests/e2e/page-objects/file-browser/breadcrumbs.page.ts | Updates breadcrumb selectors + adds helpers for new interactive breadcrumb behavior. |
| tests/e2e/page-objects/dialogs/move-dialog.page.ts | Adds page object for MoveDialog interactions. |
| tests/e2e/page-objects/dialogs/index.ts | Exports MoveDialogPage from dialogs barrel. |
| apps/web/src/styles/responsive.css | Updates responsive rules for AppShell + new file browser layout; removes legacy sidebar toggle styles. |
| apps/web/src/styles/layout.css | Adds AppShell grid layout styles and UI shell component styling. |
| apps/web/src/styles/file-browser.css | Removes legacy sidebar layout, updates toolbar styles, 3-column list, parent row, and drag-over styling. |
| apps/web/src/styles/dialogs.css | Adds Move dialog folder list styling. |
| apps/web/src/styles/breadcrumbs.css | Reworks breadcrumb styles for interactive, clickable segments with drag-drop support. |
| apps/web/src/routes/index.tsx | Switches routing to /files/:folderId?, introduces /dashboard redirect to /files. |
| apps/web/src/routes/SettingsPage.tsx | Adds AppShell-wrapped settings route with auth guard. |
| apps/web/src/routes/FilesPage.tsx | Adds AppShell-wrapped files route with auth guard. |
| apps/web/src/routes/Dashboard.tsx | Deprecates Dashboard route/component (now redirected to FilesPage). |
| apps/web/src/index.css | Adds new CSS tokens used by toolbar/nav. |
| apps/web/src/hooks/useFolderNavigation.ts | Moves folder navigation source-of-truth into the URL via react-router params/navigation. |
| apps/web/src/hooks/useAuth.ts | Updates session restore logic to better handle social vs external wallet key availability. |
| apps/web/src/components/layout/index.ts | Adds layout barrel exports for new AppShell components. |
| apps/web/src/components/layout/UserMenu.tsx | Adds header user menu dropdown (settings/logout). |
| apps/web/src/components/layout/StorageQuota.tsx | Adds storage quota indicator component for sidebar. |
| apps/web/src/components/layout/StatusIndicator.tsx | Adds footer status indicator using health check hook. |
| apps/web/src/components/layout/NavItem.tsx | Adds sidebar nav item component with active styling. |
| apps/web/src/components/layout/AppSidebar.tsx | Adds app-level sidebar navigation + storage quota. |
| apps/web/src/components/layout/AppShell.tsx | Adds AppShell composition (header/sidebar/main/footer). |
| apps/web/src/components/layout/AppHeader.tsx | Adds header with logo and UserMenu. |
| apps/web/src/components/layout/AppFooter.tsx | Adds footer with links and StatusIndicator. |
| apps/web/src/components/file-browser/index.ts | Updates file-browser barrel exports; adds new components + deprecates folder tree exports. |
| apps/web/src/components/file-browser/ParentDirRow.tsx | Adds [..] PARENT_DIR navigation row component. |
| apps/web/src/components/file-browser/MoveDialog.tsx | Adds MoveDialog UI for selecting destination folder. |
| apps/web/src/components/file-browser/FolderTreeNode.tsx | Marks folder tree node as deprecated. |
| apps/web/src/components/file-browser/FolderTree.tsx | Marks folder tree as deprecated. |
| apps/web/src/components/file-browser/FileListItem.tsx | Updates file list item layout; adds drag-over/drop target behavior; removes TYPE column. |
| apps/web/src/components/file-browser/FileList.tsx | Adds parent-dir row support and wires drop-on-folder callback; updates headers to 3-column format. |
| apps/web/src/components/file-browser/FileBrowser.tsx | Removes FolderTree layout; adds toolbar/actions; wires Move dialog + drop-to-move behavior. |
| apps/web/src/components/file-browser/EmptyState.tsx | Updates empty state to terminal-style ASCII art + upload zone. |
| apps/web/src/components/file-browser/ContextMenu.tsx | Adds “Move to...” action support. |
| apps/web/src/components/file-browser/Breadcrumbs.tsx | Replaces breadcrumb UI with interactive segments + drag-drop drop targets. |
| apps/web/src/components/ApiStatusIndicator.tsx | Deprecates old API status indicator in favor of footer component. |
| apps/web/src/App.css | Imports layout styles and adds settings-page styles; deprecates old dashboard-related styles. |
| .planning/phases/06.3-ui-structure-refactor/DESIGN-FIXES.md | Adds design discrepancy notes and intended fixes for Phase 6.3. |
| .planning/phases/06.3-ui-structure-refactor/06.3-VERIFICATION.md | Adds Phase 6.3 verification report. |
| .planning/phases/06.3-ui-structure-refactor/06.3-RESEARCH.md | Adds Phase 6.3 research notes and design specs. |
| .planning/phases/06.3-ui-structure-refactor/06.3-CONTEXT.md | Adds Phase 6.3 context and decisions. |
| .planning/phases/06.3-ui-structure-refactor/06.3-05-SUMMARY.md | Adds Phase 6.3 plan summary (visual verification). |
| .planning/phases/06.3-ui-structure-refactor/06.3-05-PLAN.md | Adds Phase 6.3 plan document. |
| .planning/phases/06.3-ui-structure-refactor/06.3-04-SUMMARY.md | Adds Phase 6.3 plan summary (integration). |
| .planning/phases/06.3-ui-structure-refactor/06.3-04-PLAN.md | Adds Phase 6.3 plan document. |
| .planning/phases/06.3-ui-structure-refactor/06.3-03-SUMMARY.md | Adds Phase 6.3 plan summary (file list updates). |
| .planning/phases/06.3-ui-structure-refactor/06.3-03-PLAN.md | Adds Phase 6.3 plan document. |
| .planning/phases/06.3-ui-structure-refactor/06.3-02-SUMMARY.md | Adds Phase 6.3 plan summary (routing + URL navigation). |
| .planning/phases/06.3-ui-structure-refactor/06.3-02-PLAN.md | Adds Phase 6.3 plan document. |
| .planning/phases/06.3-ui-structure-refactor/06.3-01-SUMMARY.md | Adds Phase 6.3 plan summary (AppShell components). |
| .planning/phases/06.3-ui-structure-refactor/06.3-01-PLAN.md | Adds Phase 6.3 plan document. |
| .planning/phases/06.2-restyle-app-with-pencil-design/06.2-CONTEXT.md | Minor formatting/condensing of Phase 6.2 context doc. |
| .planning/STATE.md | Updates project state to reflect Phase 6.3 completion. |
| .planning/ROADMAP.md | Updates roadmap to include Phase 6.3 plan list and metrics. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const handleKeyDown = useCallback( | ||
| (event: React.KeyboardEvent, folderId: string) => { | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| onNavigate(folderId); |
There was a problem hiding this comment.
handleKeyDown uses the React.KeyboardEvent type but React is not imported in this module. With the repo TS config (noUnusedLocals, no allowUmdGlobalAccess), this will fail to compile ("Cannot find name 'React'"). Use import type { KeyboardEvent } from 'react' (or import type React from 'react') and update the annotation accordingly.
| const handleKeyDown = useCallback( | ||
| (event: React.KeyboardEvent, folderId: string) => { | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| handleSelectFolder(folderId); | ||
| } |
There was a problem hiding this comment.
handleKeyDown uses the React.KeyboardEvent type but React isn't imported. This will cause a TypeScript compile error in this repo. Switch to import type { KeyboardEvent } from 'react' (or import React as a type) and update the annotation.
| /* Additional color token for active nav item */ | ||
| :root { | ||
| --color-nav-active: #001a11; | ||
| } |
There was a problem hiding this comment.
--color-nav-active is defined here and also added to apps/web/src/index.css. Duplicating the same design token in multiple places makes it easy for values to drift and complicates theming/debugging. Prefer defining it in a single place (typically index.css) and remove the duplicate :root token block here.
| .app-sidebar { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Mobile sidebar behavior is inconsistent: layout.css sets .app-sidebar { display: none; } at <=768px, but responsive.css defines an overlay sidebar pattern (.app-sidebar with translateX + .app-sidebar--open). Since display is never overridden, the overlay sidebar styles can’t take effect. Either remove the mobile display: none (and implement/toggle .app-sidebar--open), or delete the overlay sidebar rules to avoid dead/conflicting CSS.
| /* File list: 2-column layout on mobile (date hidden) */ | ||
| .file-list-header { | ||
| display: none; | ||
| grid-template-columns: 1fr 80px; | ||
| } | ||
|
|
||
| /* File list items: 2-row stacked layout per Pencil design */ | ||
| .file-list-item { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 4px; | ||
| padding: 10px 12px; | ||
| min-height: auto; | ||
| } | ||
|
|
||
| /* Row 1: Icon + Name (left aligned) */ | ||
| .file-list-item-row-top { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 6px; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .file-list-item-icon { | ||
| font-size: 10px; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .file-list-item-name { | ||
| font-size: 11px; | ||
| flex: 1; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| /* Row 2: Modified (left) | Size (right) - space-between */ | ||
| .file-list-item-row-bottom { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| gap: 12px; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .file-list-item-date, | ||
| .file-list-item-size { | ||
| font-size: 9px; | ||
| color: var(--color-text-secondary); | ||
| grid-template-columns: 1fr 80px; | ||
| } | ||
|
|
||
| /* Hide TYPE column on mobile (shown in row layout via icon) */ | ||
| .file-list-item-type { | ||
| .file-list-header-date, | ||
| .file-list-item-date { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
On mobile, .file-list-item is switched to grid-template-columns: 1fr 80px but the base grid-template-areas: "name size date" still applies. That leaves an implicit 3rd column (even though the date cell is hidden) and can cause header/row misalignment. Consider also overriding grid-template-areas on mobile (e.g., "name size") or explicitly resetting the date column sizing to avoid an extra track.
| <div | ||
| className="user-menu" | ||
| data-testid="user-menu" | ||
| onMouseEnter={() => setIsOpen(true)} | ||
| onMouseLeave={() => setIsOpen(false)} | ||
| > | ||
| <button className="user-menu-trigger" type="button"> | ||
| <span className="user-menu-email">{email}</span> | ||
| <span className="user-menu-caret">▼</span> | ||
| </button> |
There was a problem hiding this comment.
The dropdown is only controlled via onMouseEnter/onMouseLeave, which makes it inaccessible to keyboard and touch users (the trigger button doesn’t toggle the menu). Add keyboard/touch support (e.g., toggle on click + open on focus/close on blur), and wire appropriate ARIA (aria-expanded, aria-haspopup, aria-controls).
- Add file-browser-new-folder-button class to fix E2E test selector - Capitalize GitHub in VERIFICATION.md - Add CSS fallback for undefined --color-background-dark - Make onNavigateUp optional in Breadcrumbs component - Add keyboard accessibility to UserMenu (click toggle, aria attrs) - Remove unused fadeIn keyframes from responsive.css Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.planning/phases/06.3-ui-structure-refactor/06.3-VERIFICATION.md:
- Around line 1-7: The verification frontmatter's "verified" timestamp is stale
relative to commit 0b739cf; re-run the verification against the final PR state
(commit 0b739cf) and update the frontmatter "verified" value to the new
verification timestamp (or to 2026-02-02T02:22:46Z if you want to reflect that
commit time), ensuring the "status", "score", and "gaps" remain accurate; also
update any other verification entries in this file that mirror the same
timestamp (the other occurrences referenced as 12-12 and 124-124) so all
verification timestamps are consistent with the re-run.
In `@apps/web/src/styles/responsive.css`:
- Around line 44-58: The off-canvas sidebar (.app-sidebar) is still focusable
when translated off-screen; update CSS and the component toggle so the closed
state also sets visibility: hidden and pointer-events: none (e.g., on
.app-sidebar when closed) and the open state (.app-sidebar--open) restores
visibility/pointer-events, and ensure the component toggles aria-hidden and/or
inert on the DOM element when closed to remove it from the accessibility/tab
order; reference the .app-sidebar and .app-sidebar--open class names and update
the component that adds/removes those classes to also set aria-hidden/inert
accordingly.
🧹 Nitpick comments (4)
apps/web/src/components/file-browser/Breadcrumbs.tsx (2)
110-128: Consider validating the parsed drag data structure more defensively.The JSON parsing has a try-catch, but the type assertion
as { id, type, parentId }doesn't validate the actual shape. If malformed data is dropped, accessingparsed.idorparsed.typecould result inundefinedvalues being passed toonDrop.🛡️ Optional: Add runtime validation
const parsed = JSON.parse(data) as { id: string; type: 'file' | 'folder'; parentId: string; }; + // Validate required fields exist + if (!parsed.id || !parsed.type || !parsed.parentId) return; + // Don't allow dropping onto self if (parsed.id === destFolderId) return;
155-167: Button already hasonClick, makingonKeyDownredundant for Enter/Space.Native
<button>elements automatically triggeronClickwhen pressing Enter or Space. TheonKeyDownhandler (Line 159) duplicates this behavior. While not harmful, it's unnecessary code.♻️ Optional: Remove redundant keyboard handler
<button type="button" className={`breadcrumb-item ${isDragOver ? 'breadcrumb-item--drag-over' : ''} ${isLast ? 'breadcrumb-item--current' : ''}`} onClick={() => handleClick(crumb.id)} - onKeyDown={(e) => handleKeyDown(e, crumb.id)} onDragOver={onDrop ? (e) => handleDragOver(e, crumb.id) : undefined}apps/web/src/components/file-browser/FileBrowser.tsx (1)
261-266: Passing deprecatedonNavigateUpprop to Breadcrumbs.The
Breadcrumbscomponent receivesonNavigateUp={navigateUp}(Line 264), but this prop is marked as@deprecatedand is not used insideBreadcrumbs. This is dead code that should be cleaned up.♻️ Remove unused deprecated prop
<Breadcrumbs breadcrumbs={breadcrumbs} onNavigate={handleNavigate} - onNavigateUp={navigateUp} onDrop={handleDropOnFolder} />.planning/phases/06.3-ui-structure-refactor/06.3-VERIFICATION.md (1)
90-97: Consider automating manual verification checks with Playwright.Several items listed under "Human Verification Required" could be automated using Playwright E2E tests:
- Mobile responsive at <768px: Set viewport size and verify sidebar visibility, layout structure
- User menu opens on hover: Trigger hover events and assert dropdown visibility
- Browser back/forward works: Use
page.goBack()/page.goForward()and verify URL/state changes- Visual layout verification: Capture screenshots, verify computed styles (header color, sidebar width), check element positions
Only subjective visual design comparison truly requires human review. Automating the behavioral and structural checks would:
- Provide regression protection for future changes
- Make verification repeatable and consistent
- Reduce manual testing burden
Based on learnings: "Always attempt to verify application changes using Playwright MCP when available by navigating to the app URL, waiting for elements, capturing screenshots, verifying element existence, testing interactive features, and checking computed styles against design specifications."
- Fix React.KeyboardEvent import in Breadcrumbs.tsx and MoveDialog.tsx - Remove duplicate --color-nav-active token from layout.css (already in index.css) - Add Space key support to ParentDirRow for keyboard accessibility - Remove unused initial value in StatusIndicator.tsx - Remove dead overlay sidebar CSS from responsive.css - Fix grid-template-areas for mobile file list to match 2-column layout Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/file-browser/MoveDialog.tsx`:
- Around line 237-245: The key handler only supports Enter/Space; implement
Up/Down arrow navigation using a roving-tabindex pattern: add state (e.g.,
focusedIndex) and a ref array for option elements, update handleKeyDown (or
create a new handleListKeyDown) to handle 'ArrowUp'/'ArrowDown' by computing the
next index, calling focus() on the corresponding ref, updating focusedIndex, and
preventing default; keep the existing Enter/Space branch to call
handleSelectFolder(folderId). Attach the new handler as onKeyDown on the listbox
container and ensure each option element uses tabIndex={focusedIndex === i ? 0 :
-1} so keyboard focus follows the arrows.
🧹 Nitpick comments (6)
apps/web/src/components/layout/StatusIndicator.tsx (1)
33-36: Add status semantics for screen readers.Expose the changing status as a live region and hide the decorative dot to avoid noise.
♿️ Suggested a11y tweak
- return ( - <div className="status-indicator" data-testid="status-indicator"> - <span className={dotClass} /> - <span className="status-indicator-text">{statusText}</span> - </div> - ); + return ( + <div + className="status-indicator" + data-testid="status-indicator" + role="status" + aria-live="polite" + > + <span className={dotClass} aria-hidden="true" /> + <span className="status-indicator-text">{statusText}</span> + </div> + );As per coding guidelines: “apps/web/**: … Accessibility (a11y) concerns.”
apps/web/src/styles/responsive.css (2)
26-28: Empty section header without associated styles.The "AppShell Mobile Overlay" section header at lines 26-28 contains no CSS rules. If the overlay styles were moved elsewhere (e.g.,
layout.css), consider removing this placeholder to reduce noise.
20-24: Consider removing deprecated commented-out code.The deprecated block lists removed class names but doesn't provide value. Since the deprecation is documented in the section header above, the empty comment block can be removed for cleaner CSS.
♻️ Suggested cleanup
-/* -.file-browser-toggle { ... } -.file-browser-backdrop { ... } -.file-browser-sidebar-close { ... } -*/apps/web/src/components/file-browser/Breadcrumbs.tsx (1)
110-126: Add runtime validation for parsed drag data.The type assertion on line 114 doesn't guarantee the parsed object has the expected shape. If
parsed.id,parsed.type, orparsed.parentIdareundefined, the comparisons on lines 121 and 124 will silently pass (e.g.,undefined === destFolderIdis false), butonDropwould receive invalid data.🛡️ Proposed fix with runtime validation
try { const data = e.dataTransfer.getData('application/json'); if (!data) return; - const parsed = JSON.parse(data) as { - id: string; - type: 'file' | 'folder'; - parentId: string; - }; + const parsed = JSON.parse(data); + + // Validate expected structure + if ( + typeof parsed?.id !== 'string' || + (parsed.type !== 'file' && parsed.type !== 'folder') || + typeof parsed?.parentId !== 'string' + ) { + return; + } // Don't allow dropping onto self if (parsed.id === destFolderId) return;apps/web/src/components/file-browser/MoveDialog.tsx (2)
64-72: Consider caching folder depths to avoid redundant traversals.
getDepth()traverses the parent chain on each call. Currently it's called twice per comparison during sort and again in the loop body, resulting in redundant work. For deeply nested structures, this could be optimized.♻️ Proposed optimization to compute depths once
// Build sorted list of all folders (excluding root) const folderEntries = Object.values(folders).filter((f) => f.id !== 'root'); + // Cache depths to avoid repeated parent-chain traversals + const depthCache = new Map<string, number>(); + for (const folder of folderEntries) { + depthCache.set(folder.id, getDepth(folder.id, folders)); + } + // Sort by depth then name for predictable display folderEntries.sort((a, b) => { - const depthA = getDepth(a.id, folders); - const depthB = getDepth(b.id, folders); + const depthA = depthCache.get(a.id) ?? 0; + const depthB = depthCache.get(b.id) ?? 0; if (depthA !== depthB) return depthA - depthB; return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' }); }); for (const folder of folderEntries) { - const depth = getDepth(folder.id, folders); + const depth = depthCache.get(folder.id) ?? 0; let isDisabled = false;
248-248: Consider memoizingisValidto avoid redundant validation on each render.
validate()is executed on every render to computeisValid. Whilevalidateis wrapped inuseCallback, the function body still runs each time. Since the result depends on stable dependencies, consider usinguseMemo.♻️ Proposed fix
- const isValid = !validate(); + const validationError = useMemo(() => validate(), [validate]); + const isValid = !validationError;This also allows reusing
validationErrorinhandleSubmitif desired.
| const handleKeyDown = useCallback( | ||
| (event: KeyboardEvent, folderId: string) => { | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| handleSelectFolder(folderId); | ||
| } | ||
| }, | ||
| [handleSelectFolder] | ||
| ); |
There was a problem hiding this comment.
Add arrow key navigation for listbox accessibility.
The listbox widget is missing Up/Down arrow key navigation, which is expected per WAI-ARIA authoring practices. Currently only Enter/Space are handled for selection, but users (especially screen reader users) expect arrow keys to navigate between options.
♿ Proposed fix to add arrow navigation
Consider implementing roving tabindex with arrow key support. Here's a simplified approach using a ref to manage focus:
+ const listRef = useRef<HTMLDivElement>(null);
+
+ const handleListKeyDown = useCallback(
+ (event: KeyboardEvent<HTMLDivElement>) => {
+ const enabledItems = folderList.filter((f) => !f.isDisabled);
+ if (enabledItems.length === 0) return;
+
+ const currentIndex = enabledItems.findIndex((f) => f.id === selectedFolderId);
+
+ if (event.key === 'ArrowDown') {
+ event.preventDefault();
+ const nextIndex = currentIndex < enabledItems.length - 1 ? currentIndex + 1 : 0;
+ setSelectedFolderId(enabledItems[nextIndex].id);
+ } else if (event.key === 'ArrowUp') {
+ event.preventDefault();
+ const prevIndex = currentIndex > 0 ? currentIndex - 1 : enabledItems.length - 1;
+ setSelectedFolderId(enabledItems[prevIndex].id);
+ }
+ },
+ [folderList, selectedFolderId]
+ );Then add onKeyDown={handleListKeyDown} to the listbox container div.
🤖 Prompt for AI Agents
In `@apps/web/src/components/file-browser/MoveDialog.tsx` around lines 237 - 245,
The key handler only supports Enter/Space; implement Up/Down arrow navigation
using a roving-tabindex pattern: add state (e.g., focusedIndex) and a ref array
for option elements, update handleKeyDown (or create a new handleListKeyDown) to
handle 'ArrowUp'/'ArrowDown' by computing the next index, calling focus() on the
corresponding ref, updating focusedIndex, and preventing default; keep the
existing Enter/Space branch to call handleSelectFolder(folderId). Attach the new
handler as onKeyDown on the listbox container and ensure each option element
uses tabIndex={focusedIndex === i ? 0 : -1} so keyboard focus follows the
arrows.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.