feat(ui): add workspace-root navigation to directory browser - #724
Conversation
Add a "Workspace Root" button (Home icon) to the directory browser dialog that navigates the view back to the configured workspace root (FileSystemListingMetadata.rootPath). Implementation: - The button is its own grid cell placed to the right of the "Select folder or enter path" label, fully separated from the New Folder and Open actions. It uses a new transparent `selector-button-ghost` variant so it reads as a navigation tool rather than a filled action button. - Clicking it calls navigateTo(rootPath()); it is disabled when there is no root path, when the user is already at the root, or while a folder is being created. - A `canGoToWorkspaceRoot` memo drives the disabled state. i18n: - Add `directoryBrowser.goToWorkspaceRoot` in en plus de, es, fr, he, ja, ne, ru, tr and zh-Hans so the tooltip/aria-label are localized. Layout details: - `.directory-browser-current` now uses a four-column grid; the flexible middle column pushes New Folder to the far right and keeps the root button adjacent to the label. Responsive rules stack the controls at <=640px and <=380px. Edge cases: - In files mode the New Folder action is hidden; the Workspace Root button remains available so users can still jump back to the root. For the Windows drives pseudo-view rootPath is empty, so the button is disabled. Validation: - No typecheck run (npx disallowed in this environment); manual browser verification of spacing/divider and locale labels is recommended.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/35502137445 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
The convenience is understandable, but the destination needs an explicit product contract before this shared control is added.
FileSystemListingMetadata.rootPath is the server browsing/authorization origin, not necessarily the dialog's starting folder or the active workspace. Electron and Tauri launch the server with --unrestricted-root and no explicit --workspace-root, so this value defaults to the server process working directory. The shared dialog is used for workspace selection, clone destinations, OpenCode binaries, and file attachments, each with different initialPath semantics. A Home icon labelled “Workspace Root” therefore has no stable meaning across callers.
Please first choose the intended destination and model it explicitly: capture the canonical initial location for “back to start”, pass an active workspace target from callers, use homePath for filesystem home, or clearly expose the configured browsing root. The editable path field does not provide history. CodeNomad currently persists recent opened project folders, but has no generic directory history or favourites; those should not be conflated. A per-dialog start shortcut or back/forward history can stay local, while persistent favourites deserve a separate scoped design.
There are also two confirmed implementation regressions in the inline comments. Please add a real browser regression covering directory and file modes at desktop and narrow widths, plus the edited-path navigation case.
| type="button" | ||
| class="selector-button selector-button-ghost directory-browser-go-root" | ||
| disabled={!canGoToWorkspaceRoot()} | ||
| onClick={() => void navigateTo(rootPath())} |
There was a problem hiding this comment.
This bypasses handleNavigateTo(), which deliberately clears pathInputDirty before changing directories. Reproduction: navigate below the root, edit the path field without submitting it, then click this button. The listing moves to the root but the field keeps the stale edited path; the next Open or New Folder action operates on that stale target. Route this action through the shared navigation handler (or otherwise clear/synchronize the edit state) and cover the sequence with a regression test.
| flex-shrink: 0; | ||
| } | ||
|
|
||
| .directory-browser-open-path { |
There was a problem hiding this comment.
The grid templates still reserve an open area, but this rule no longer assigns grid-area: open. Chromium therefore auto-places Open into the desktop spacer on the label row; below 640px it occupies only half the intended row, and in files mode it lands beside the root button. Restore the explicit area assignment. Please cover both dialog modes and responsive layouts so this cannot regress silently.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/35512118379 Artifacts expire in 7 days.
|
Address the directory-browser review on PR NeuralNomadsAI#724. Replace the single "Workspace Root" button with conditional, de-duplicated navigation shortcuts driven by FileSystemListingMetadata: - workspace-root shortcut only when scope is "restricted" (rootPath is the configured browsing root), - user-home shortcut only when scope is "unrestricted" (uses homePath), - initial-path shortcut when initialPath is provided. Identical absolute targets collapse into a single button. Fix Regression A: shortcut clicks clear the edited-path state (setPathInputDirty(false)) before navigating, so a stale manually-edited path field no longer leaks into the next Open / New Folder action. Fix Regression B: restore grid-area: open on .directory-browser-open-path so the Open button is explicitly placed instead of being auto-placed into the label-row spacer at desktop widths and below 640px (and beside the root button in files mode). Add a browser regression test (directory-browser.test.ts + fixture) covering restricted/unrestricted scopes, dedupe, files mode, narrow widths, and the edited-path navigation case. Add i18n keys directoryBrowser.goToHome and directoryBrowser.goToInitial across all 10 locales.
Rename the workspace-root shortcut to a "Default Start Directory" shortcut (i18n key directoryBrowser.goToRoot) so the label reflects the resolved root semantics (CLI_WORKSPACE_ROOT -> --root -> cwd) instead of a parameter name. Show the start-directory shortcut whenever rootPath is present in BOTH restricted and unrestricted modes. This matches issue NeuralNomadsAI#370, where CLI_WORKSPACE_ROOT is the intended default path even under --unrestricted-root, so the server's resolved root is a valid jump target regardless of scope. Keep the home shortcut limited to unrestricted scope (its target is outside the authorized root in restricted mode) and dedupe it against rootPath. The initial-path shortcut is unchanged. All three shortcuts are still de-duplicated by their canonical absolute path. Update i18n keys/labels across all 10 locales and adjust the browser regression test: unrestricted scenarios now yield root + home + initial (3 shortcuts), and the home shortcut is selected by index.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
The destination contract is now much clearer, and the two production regressions from the previous review are corrected: shortcut navigation clears the edited-path state and grid-area: open is restored.
This HEAD is still not merge-ready. The new browser suite never mounts the dialog and all five tests fail in required CI; after fixing the fixture, two assertions are themselves guaranteed to fail. There is also a user-visible mismatch in how a relative initial path is resolved in unrestricted mode, plus a broken shortcut after initial-path fallback. Please address the inline findings and rerun the browser suite. No merge performed.
| let navigations: string[] = [] | ||
| const [open, setOpen] = createSignal(true) | ||
|
|
||
| render( |
There was a problem hiding this comment.
DirectoryBrowserDialog calls useI18n() immediately, so rendering it without I18nProvider throws before the dialog mounts; I18nProvider in turn requires ConfigProvider. This is why all five new tests time out waiting for .directory-browser-current-path in required CI instead of exercising any assertions. Wrap this fixture in the same ConfigProvider + I18nProvider stack used by the other real-component fixtures (and preferably surface pageerror so fixture startup failures fail fast).
| } | ||
| const initial = props.initialPath?.trim() | ||
| if (initial) { | ||
| const target = isAbsolutePathLike(initial) ? initial : resolveAbsolutePath(meta.rootPath, initial) |
There was a problem hiding this comment.
This does not preserve the server's initialPath semantics. initialize() sends a relative initial path unchanged, and unrestricted FileSystemBrowser.resolveUnrestrictedPath() resolves it against homePath; this line instead rewrites it under rootPath. With root /srv/start, home /home/user, and initial projects, the dialog opens /home/user/projects but Initial Path jumps to /srv/start/projects. The button also remains after an invalid/missing initial path made initialize() fall back to the default root, leaving a permanently failing shortcut. Capture the canonical absolute path only from a successful initial navigation and use that for the target/deduplication; omit the shortcut after fallback.
| // initialPath equals rootPath, so the workspace + initial shortcuts collapse into one. | ||
| await openFixture(page, scenario, "initialPath=/ws&mode=directories") | ||
| assert.equal(await shortcutCount(page), 1) | ||
| await page.locator(".directory-browser-shortcut").first().click() |
There was a problem hiding this comment.
Once the fixture mounts, this click still cannot succeed: the initial path is /ws, the sole deduplicated shortcut also targets /ws, and atTarget() therefore disables the button. Playwright will wait for it to become enabled and time out. Assert that the deduplicated button is disabled here, or initialize below the root before testing navigation.
| assert.equal(await page.locator(".directory-browser-open-path").count(), 1) | ||
| // Open button should span the full row at this width, not half. | ||
| const box = await page.locator(".directory-browser-open-path").boundingBox() | ||
| assert.ok(box !== null && box.width >= 300, `open button width ${box?.width} should fill the row`) |
There was a problem hiding this comment.
At a 360 px viewport the modal is at most 90vw and the body has 48 px horizontal padding, so the Open button cannot be 300 px wide; this assertion will fail after the fixture is repaired. Compare its width/edges with the grid content box instead of a hard-coded viewport threshold. Also, 360 px exercises the separate <=380px template, not the 381–640px two-column breakpoint where the original half-row/file-mode regression occurred; please cover that breakpoint in both directory and file modes as requested.
…tics Address the latest PR NeuralNomadsAI#724 review. Fixture: - Wrap DirectoryBrowserDialog in ConfigProvider + I18nProvider + ThemeProvider (the dialog calls useI18n() at mount, so without the providers it threw and every test timed out). Surface pageerror so fixture startup failures fail fast. Component: - Capture the Initial Path shortcut target from a *successful* initial navigation via getAbsolutePathFromMetadata, instead of re-resolving props.initialPath under rootPath. The server resolves relative initial paths against homePath in unrestricted mode, so the old code opened one location but the button jumped to another. Now the target equals exactly where the dialog opened. - Omit the Initial Path shortcut when the initial path is rejected and initialize() falls back to the default root, so no broken/permanently-disabled shortcut remains. Tests: - Fix the dedupe test: assert the collapsed shortcut is disabled (it targets the current path) instead of clicking a disabled button. - Replace the hard-coded 300px width assertion with a comparison against the grid content box, and cover the 381-640px two-column breakpoint in both directory and file modes. - Assert no pageerror in every test.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
The fixture now mounts and capturing the canonical result of the initial navigation is the right direction. This HEAD is still not merge-ready: the required browser job fails both new responsive assertions, the captured initial shortcut can survive a later fallback when the mounted dialog is reopened, and the relative-path contract being fixed is not covered by the fixture. Please address the inline findings and rerun the focused browser suite.
| const [loadingPaths, setLoadingPaths] = createSignal<Set<string>>(new Set()) | ||
| const [currentPathKey, setCurrentPathKey] = createSignal<string | null>(null) | ||
| const [currentMetadata, setCurrentMetadata] = createSignal<FileSystemListingMetadata | null>(null) | ||
| const [initialAbsolutePath, setInitialAbsolutePath] = createSignal("") |
There was a problem hiding this comment.
This new per-opening value is not cleared by resetState(). The dialog component remains mounted while open is false, so a successful first opening can leak its captured target into a later opening whose initialPath is absent or rejected. After fallback to the default root, the old Initial Path shortcut is shown and points back to the stale/failing location, contrary to the comment in initialize(). Clear the signal before each initialization and cover successful open → close → fallback/reopen.
| const openBox = await page.locator(".directory-browser-open-path").boundingBox() | ||
| assert.ok(bodyBox && openBox, "layout boxes must be measurable") | ||
| // Open spans the full content row at the 381-640px two-column breakpoint. | ||
| assert.ok(openBox.width >= bodyBox.width - 16, `open width ${openBox.width} should fill content width ${bodyBox.width}`) |
There was a problem hiding this comment.
This still compares against the body border box rather than the grid content box. .directory-browser-body includes horizontal padding, so required CI on this exact HEAD measures the button at 512px and the body at 536px; this assertion and its file-mode copy at line 203 both fail. Compare the button with .directory-browser-current (preferably matching left/right edges with a small subpixel tolerance) instead.
| parentPath: scenario.parentPath, | ||
| rootPath: scenario.rootPath, | ||
| homePath: scenario.homePath, | ||
| displayPath: currentPath, |
There was a problem hiding this comment.
The fixture forces every response to pathKind: "absolute" and echoes the requested path, while every test supplies an absolute initialPath. Consequently the regression motivating the production change—an unrestricted relative initial path being canonicalized under homePath rather than rootPath—would not be caught here. Add a scenario such as initialPath=projects whose fake server returns /home/projects, navigate away, then assert that the Initial Path shortcut returns to that canonical target.
…h contract Address the third PR NeuralNomadsAI#724 review round. Component: - Clear the initialAbsolutePath signal in resetState() so a successful first opening no longer leaks its captured target into a later reopen whose initialPath is absent or rejected (the mounted dialog stays alive while open is false). Fixture: - Make initialPath a signal and expose setInitialPath/open/close so tests can drive close -> change initialPath -> reopen on the same mounted component. - Mirror server canonicalization in the fake server: a relative initial path resolves under homePath in unrestricted mode and under rootPath in restricted mode, instead of always echoing the absolute value. Tests: - Add a relative-initial-path test (scope=unrestricted, initialPath=projects) asserting the dialog opens at /home/user/projects and the Initial Path shortcut returns there after navigating away, covering the regression that motivated the production change. - Add a close/reopen leak test: open with /ws/start (2 shortcuts), close, drop initialPath, reopen -> only the start-directory shortcut remains (1), proving resetState clears the captured target. - Fix both two-column (600px) responsive assertions to compare the Open button's left/right edges against the .directory-browser-current grid box (within 2px) instead of the padded .directory-browser-body, in both directory and file modes.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Re-reviewed the updated HEAD 77c3d2a6296c7ad66448ed8916b9c99ff82ab8ea. The outstanding findings from my previous review are addressed; no remaining blocking finding in the directory-browser changes.
Confirmed:
resetState()clears the captured canonical initial target before each opening, preventing the old shortcut from surviving a subsequent opening without an initial path.- Both responsive tests now compare the Open button's left/right edges against
.directory-browser-current, at the actual two-column breakpoint and in both dialog modes. - The relative-path fixture now models unrestricted resolution under
homePath, and the regression navigates away and back to the canonical initial location. - The prior fixes remain in place: provider-backed fixture, edited-path synchronization, explicit Open grid area, and distinct/deduplicated shortcut destinations.
Validation on this exact HEAD in an isolated worktree:
- UI typecheck: passed.
node --import tsx --test tests/browser/directory-browser.test.ts: 8 passed, 0 failed.
CI/integration follow-up before merge: run 35870907908 is still red and did not reach the browser suite. Its current workflow invokes files absent from this older branch (view-menu.test.ts, runtime-support.ts, test-session-aside-native.mjs, and test-opencode-install-native.mjs). Tauri Windows additionally reports two cross-host client-state election failures (178 passed, 2 failed), outside this PR's diff. Please synchronize with current dev and rerun required CI; this code approval is not a claim that the global checks pass. No merge performed.
Replace the three persistent root/home/initial icons with an address field that reveals a named return destination, Home when unrestricted, and the parent when useful. Filter already loaded child directories only after typing, preserving direct path entry while keeping Open and New Folder visible in narrow layouts. Share the address presentation across every caller of the directory browser and localize its labels. Fence late filesystem responses across dialog reopenings so an older visit cannot replace the new location or populate its cache. Cover canonical relative starts, failed starts, keyboard and Escape behavior, stale responses, and file/directory layouts at both responsive breakpoints with real browser fixtures. Validation: 11 directory-browser Playwright scenarios, UI typecheck, UI build, and staged diff check passed. Reviewed desktop and narrow rendered captures.
Keep Parent folder in the address destinations and remove the redundant Up one level row from the file and directory lists. This keeps return navigation available while avoiding two controls for the same destination in the same dialog. Add browser coverage for both dialog modes so the parent remains reachable from the address and cannot reappear as a duplicate folder row. Include real English browser captures of contextual destinations and typed child-folder suggestions to illustrate the PR interaction to its author. Validation: all 13 focused directory-browser browser scenarios and UI typecheck passed; reviewed both screenshot captures and the staged diff.
|
@bluelovers I pushed a follow-up to this PR ( Here are actual browser captures of the updated shared directory browser (English locale, deterministic example paths). These show the interaction I described above: Focus after moving away from the starting folder — “Back to start” and Home appear under the path field; the normal folder list is below it.
Typing
I noticed the old “Up one level” list row repeated the Parent folder destination in the address. That duplication is fixed in |
2 participants
Summary
Adds a "Workspace Root" button to the directory browser dialog that navigates the view back to the configured workspace root (
FileSystemListingMetadata.rootPath).What changed
navigateTo(rootPath())to jump back to the workspace root. It is disabled when there is no root path, when the user is already at the root, or while a folder is being created (driven by a newcanGoToWorkspaceRootmemo).New FolderandOpenactions. It uses a new transparentselector-button-ghostvariant so it reads as a navigation tool rather than a filled action button.directoryBrowser.goToWorkspaceRootforenplusde,es,fr,he,ja,ne,ru,tr, andzh-Hans(used for tooltip/aria-label)..directory-browser-currentnow uses a four-column grid; the flexible middle column pushesNew Folderto the far right and keeps the root button next to the label. Responsive rules stack the controls at<=640pxand<=380px.Edge cases
New Folderaction is hidden, but the Workspace Root button stays available so users can still jump back to the root.rootPathis empty, so the button is disabled.Validation
npxis disallowed here); manual browser verification of spacing and the localized label is recommended.🤖 Generated with CodeNomad