feat(design-files): group nested paths as folders#472
feat(design-files): group nested paths as folders#472irixzafra wants to merge 3 commits intonexu-io:mainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Hi @irixzafra! 🎉 Thanks for making open-design better! |
lefarcen
left a comment
There was a problem hiding this comment.
Hi @irixzafra — nice work on the folder grouping! The core logic is solid and the test coverage is a good start. I found a few areas where the interaction between grouping/sections/pagination could be tightened up:
P2 findings:
- Folder rows appear duplicated across sections when a real folder contains mixed file types
- Section pagination can hide visible folders after expansion
P3 suggestions:
- Performance: row building runs on every render
- Test coverage: only static SSR, no interactive expansion tests
See inline comments for details and concrete fixes. Overall this is ready to iterate — the scoped "view-only" boundary is clear and respected.
| @@ -162,87 +186,133 @@ export function DesignFilesPanel({ | |||
| ) : ( | |||
| SECTION_ORDER.filter((s) => grouped[s].length > 0).map((section) => { | |||
There was a problem hiding this comment.
P2 Grouping happens after files are split into kind sections, so one real top-level folder with mixed contents renders as separate identical folder rows under Pages/Scripts/Images/Other.
Example: site/index.html, site/app.js, and site/logo.png become three site folders, each with a partial count, which breaks the mental model of "folder" rows.
Fix: Build folder groups before sectioning, or make the UI explicitly section-scoped (e.g., show section prefix in folder label like "Pages: site") so duplicate folder labels/counts are not misleading.
| expandedFolders, | ||
| preview, | ||
| ); | ||
| const visibleLimit = sectionLimits[section] ?? INITIAL_SECTION_FILE_LIMIT; |
There was a problem hiding this comment.
P2 The section "show more" limit is applied to the flattened row list after expanded children are inserted. Expanding a visible folder near the top of a section can push previously visible sibling folders/files past the slice boundary and make them disappear until "show more" is clicked.
Fix: Apply the limit only to top-level entries, then render children for expanded visible folders outside that top-level pagination.
| ) : ( | ||
| SECTION_ORDER.filter((s) => grouped[s].length > 0).map((section) => { | ||
| const sectionFiles = grouped[section]; | ||
| const sectionRows = buildSectionRows( |
There was a problem hiding this comment.
P3 buildSectionRows sorts and rebuilds folder rows during every render, including hover/menu/drag state updates. Large imported directories can make simple hover interactions more expensive than before.
Fix: Memoize rows per section from grouped, expandedFolders, and preview, or separate stable row structure from the active preview decoration.
| onNewSketch: vi.fn(), | ||
| }; | ||
|
|
||
| it('groups nested project paths under a collapsed folder row', () => { |
There was a problem hiding this comment.
P3 The new regression test only checks static collapsed SSR markup and does not exercise the core interactive behavior: clicking a folder, rendering nested children, active folder state when previewing a nested file, mixed-kind folders, or pagination interaction.
Fix: Add at least one interaction-level test for expansion and one edge-case test for section/limit behavior; alternatively extract and directly test the row-building logic.
|
Updated this PR with the final folder-browser shape:
Verification completed:
The broader file-management surface is tracked separately in #474. |
lefarcen
left a comment
There was a problem hiding this comment.
Hi @irixzafra — great progress! The dedicated Folders section is exactly the right move:
Resolved:
✅ P2 #1 (folder duplication) — mixed file types now render once in the Folders section
✅ P3 (performance) — folderRows memoization eliminates re-sorting on hover
Still needs attention:
- P2 #2 (pagination) — not fully resolved yet; see inline
- P3 (test coverage) — still only static SSR; see inline
One more iteration should wrap this up. The core architecture is solid.
| startSectionExpansion: TransitionStartFunction; | ||
| t: TranslateFn; | ||
| }) { | ||
| const visibleLimit = sectionLimits[section] ?? INITIAL_SECTION_FILE_LIMIT; |
There was a problem hiding this comment.
P2 P2 #2 from prior review is not fully resolved. FileSection still paginates rows after buildFolderRows has flattened expanded children into the folder row list (line 595). Expanding one visible folder can still consume the first 30 visible slots with child files and push previously visible sibling folders below the slice.
Fix: Paginate only top-level folder rows, then render children for expanded visible folders outside the folder pagination count.
| onNewSketch: vi.fn(), | ||
| }; | ||
|
|
||
| it('groups nested project paths under a single collapsed folder section', () => { |
There was a problem hiding this comment.
P3 The updated test still only validates static SSR markup for a collapsed folder; it does not exercise clicking/expansion, mixed-kind single-folder behavior, or the pagination edge case.
Fix: Add an interaction test using the project's preferred React test setup, or extract the row-building/pagination function and directly test expanded folders with more than INITIAL_SECTION_FILE_LIMIT siblings.
…ders # Conflicts: # apps/web/src/components/DesignFilesPanel.tsx # apps/web/src/index.css
|
Synced this branch with the current Conflict resolution kept both feature sets:
Verification after the merge:
|
mrcfps
left a comment
There was a problem hiding this comment.
Thanks @irixzafra — I reviewed the folder-section rendering, locale/type additions, CSS updates, and the regression coverage. I found one remaining pagination edge case in the folder section; it looks merge-safe but worth tightening so expanded folders do not hide sibling folders unexpectedly.
Generated by Looper 0.5.4 · runner=reviewer · agent=opencode| t: TranslateFn; | ||
| }) { | ||
| const visibleLimit = sectionLimits[section] ?? INITIAL_SECTION_FILE_LIMIT; | ||
| const visibleRows = rows.slice(0, visibleLimit); |
There was a problem hiding this comment.
The folder section still paginates the already-flattened rows list (visibleRows = rows.slice(...)). Because buildFolderRows appends expanded child files into that same list after each folder, expanding a folder near the top can consume the first 30 visible slots with children and push previously visible sibling folders below the “show more” boundary. That makes folders appear to disappear even though the user only expanded one folder.
A safer fix is to paginate only the top-level folder entries first, then render the expanded children for those visible folders outside the pagination count. Please also add a regression that creates more than INITIAL_SECTION_FILE_LIMIT top-level folders, expands one with many children, and verifies the sibling folders that were visible before expansion remain visible. 🙂
lefarcen
left a comment
There was a problem hiding this comment.
Hi @irixzafra — good merge! The conflict resolution looks clean and both feature sets play nicely together.
From prior reviews:
- P2 #1 (folder duplication) — still resolved ✓
- P3 (performance) — still resolved ✓
- P2 #2 (pagination) — still present; see inline
- P3 (test coverage) — still incomplete; see inline
New from this merge:
- P3 (interactive nesting) — multi-select checkbox inside button; see inline
The Folders architecture is solid. One more pass on pagination and you'll be there.
| startSectionExpansion: TransitionStartFunction; | ||
| t: TranslateFn; | ||
| }) { | ||
| const visibleLimit = sectionLimits[section] ?? INITIAL_SECTION_FILE_LIMIT; |
There was a problem hiding this comment.
P2 (carryover) P2 #2 is still present after the merge. FileSection still slices rows after buildFolderRows has flattened expanded folder children (line 748), so expanding a folder can still consume the first 30 visible slots and push sibling folder rows below the pagination boundary.
Fix: Paginate only top-level folder rows, then render expanded children for those visible folders outside the folder-section slice.
| onNewSketch: vi.fn(), | ||
| }; | ||
|
|
||
| it('groups nested project paths under a single collapsed folder section', () => { |
There was a problem hiding this comment.
P3 (carryover) Test coverage is still only static SSR for the collapsed folder state. It does not click/expand a folder, validate nested rows, cover multi-select inside expanded folders, or catch the pagination regression.
Fix: Add an interaction test or extract row-building/pagination logic for direct tests.
| onClick={() => setPreview(f.name)} | ||
| onDoubleClick={() => onOpenFile(f.name)} | ||
| > | ||
| <span |
There was a problem hiding this comment.
P3 (new from merge) The merge added multi-select with a focusable checkbox-like span inside a row button, which is invalid interactive nesting and can produce inconsistent keyboard/screen-reader behavior.
Fix: Make the row a non-button container with separate checkbox and open/preview controls, or move selection into a real checkbox outside the row button.
Summary
Folderssection at the top of Design Files for top-level nested project paths.Pages,Scripts,Images, etc.) below the folder section.Why
Projects already support nested file paths, archive tree export, and nested raw file serving. Design Files previously rendered nested paths inside each kind section, which made imported/exported projects hard to scan and could repeat the same folder across
Pages,Scripts, andImages.This PR keeps the change view-only and aligns the browser with the established design-tool pattern: folders are first-class rows, expand inline, and reveal their contents in place.
Related adjacent work: #211, #401, #341, #455.
Test plan
pnpm --filter @open-design/web typecheckpnpm --filter @open-design/web exec vitest run -c vitest.config.ts src/components/DesignFilesPanel.test.tsxFolderssection renders onceNotes
This PR intentionally does not add rename, move, or create-folder operations. Those should be handled in a separate file-management PR because they affect open tabs, preview URLs, artifact manifests, comments,
@filereferences, exports/deploy output, empty-folder persistence, and path-safety validation.