feat(sidebar): add workspace management - #2138
Conversation
📝 WalkthroughWalkthroughThe PR adds sidebar workspace registration and lifecycle handling. It merges managed environments with session groups, supports empty and archived workspaces, adds versioned project mutations, updates directory ordering, and expands picker, grouping, localization, and regression coverage. ChangesWorkspace registration and sidebar lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WindowSideBar
participant projectStore
participant ProjectService
participant ProjectSnapshot
User->>WindowSideBar: choose Add Workspace
WindowSideBar->>projectStore: openFolderPicker({ select: false })
projectStore->>ProjectService: selectDirectory()
ProjectService->>ProjectSnapshot: activate directory and commit version
ProjectSnapshot-->>ProjectService: return path and version
ProjectService-->>projectStore: return path and version
projectStore-->>WindowSideBar: apply committed snapshot
WindowSideBar-->>User: reveal workspace
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/stores/ui/session.ts (1)
1311-1314: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the last persisted mode after a failed queued write.
Two failed queued requests can leave
groupModeat a mode that was never persisted. If aprojectwrite fails and a latertimewrite also fails, the later rollback restorespreviousModeasproject, although the durable setting remainstime.Track the last successfully persisted mode. Use that value for rollback. Add a regression test for two queued failed writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/ui/session.ts` around lines 1311 - 1314, Update the group-mode persistence flow around the persistError catch to track the last successfully persisted mode and restore that value on any failed queued write, rather than using the request-local previousMode. Ensure successful writes update the tracked persisted mode, and add a regression test covering two queued failures where rollback must return to the durable mode.
🧹 Nitpick comments (7)
test/renderer/components/WindowSideBar.test.ts (3)
1073-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe concurrency test calls an internal method instead of the rendered control.
Lines 1084-1085 invoke
(wrapper.vm as any).handleAddWorkspace()twice. The user-visible guard is the disabled add-workspace button. Trigger the button twice so the test covers the observable contract and does not break when the handler is renamed.♻️ Proposed refactor
- const firstRequest = (wrapper.vm as any).handleAddWorkspace() - const secondRequest = (wrapper.vm as any).handleAddWorkspace() - await nextTick() + const addButton = wrapper.get('[data-testid="window-sidebar-add-workspace-button"]') + await addButton.trigger('click') + await nextTick() + await addButton.trigger('click') + await nextTick()As per coding guidelines: "Keep committed tests lean and focused on project reliability, stability, and observable contracts; remove temporary checks that only test implementation internals before handoff."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/WindowSideBar.test.ts` around lines 1073 - 1103, Update the concurrency test to trigger the rendered add-workspace button twice instead of calling the internal handleAddWorkspace method directly. Use the existing button selector and preserve the assertions covering a single picker invocation, disabled state, cancellation side effects, and concurrent-request behavior.Source: Coding guidelines
60-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
createEnvironmentproduces archived and removed fixtures without timestamps.
fallbackStatussetsstatus, butarchivedAtandremovedAtstill default tonull. An archived fixture therefore hasstatus: 'archived'andarchivedAt: null, which the main process never produces. If the component ever readsarchivedAtorremovedAtinstead ofstatus, the test passes for the wrong reason. The equivalent helper intest/renderer/stores/projectStore.test.tsderives the timestamps from the status.♻️ Proposed refactor
status: environment.status ?? fallbackStatus, sortOrder: environment.sortOrder ?? 0, - archivedAt: environment.archivedAt ?? null, - removedAt: environment.removedAt ?? null + archivedAt: + environment.archivedAt ?? ((environment.status ?? fallbackStatus) === 'archived' ? 100 : null), + removedAt: + environment.removedAt ?? ((environment.status ?? fallbackStatus) === 'removed' ? 100 : null) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/WindowSideBar.test.ts` around lines 60 - 74, Update createEnvironment so fallbackStatus-derived fixtures also receive the corresponding archivedAt or removedAt timestamp, matching the projectStore.test.ts helper; preserve explicitly supplied timestamps and leave unrelated status handling unchanged.
1045-1048: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the focus target, not that some element received focus.
expect(focusSpy).toHaveBeenCalled()passes when any element callsfocus(), including the search input or the add button. The test claims the registered row receives focus. Assert the receiver instead.♻️ Proposed refactor
- expect(focusSpy).toHaveBeenCalled() + expect(focusSpy.mock.instances).toContain( + wrapper.get('[data-group-id="/work/new"]').element + )As per coding guidelines: "Add the smallest regression test for user-visible behavior or a documented contract."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/WindowSideBar.test.ts` around lines 1045 - 1048, Update the focus assertion in the test around the group ordering expectations to verify that the registered row element is the receiver of focus, rather than only checking that focusSpy was called. Preserve the existing ordering assertions and use the row’s focus target or registered-row selector already established in the test.Source: Coding guidelines
src/renderer/src/stores/ui/project.ts (1)
288-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe catch block re-wraps errors it produced itself.
When
refreshProjectSnapshotfails,error.valuebecomesFailed to load project snapshot: .... Line 282 throws that string, and this catch overwriteserror.valuewithFailed to open folder picker: Error: Failed to load project snapshot: .... The stored message nests twice. Preserve the original snapshot error instead.♻️ Proposed refactor
- await refreshProjectSnapshot() - if (error.value) { - throw new Error(error.value) - } + await refreshProjectSnapshot() + if (error.value) { + // Keep the snapshot failure message; do not re-wrap it below. + throw new SnapshotRefreshError(error.value) + } if (!environments.value.some((environment) => environment.path === selectedPath)) { throw new Error('Selected workspace is missing from the project snapshot') } return selectedPath } catch (cause) { - error.value = `Failed to open folder picker: ${cause}` + if (!(cause instanceof SnapshotRefreshError)) { + error.value = `Failed to open folder picker: ${cause}` + } throw cause }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/ui/project.ts` around lines 288 - 291, Update the folder-picker error handling around refreshProjectSnapshot so errors already produced by that snapshot flow are not rewrapped as “Failed to open folder picker.” Preserve the original snapshot error in error.value while retaining the existing behavior for genuine folder-picker failures and rethrowing the cause.src/renderer/src/components/WindowSideBar.vue (2)
477-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant inner
v-if.
DropdownMenunow renders only whenisActiveProjectDirectoryGroup(group)is true. That predicate already impliesisProjectDirectoryGroup(group). Thev-ifon the trigger button is always true here. An emptyDropdownMenuTrigger as-childwould also break Reka UI if the condition were ever false.♻️ Proposed change
<DropdownMenuTrigger as-child> <DcButton - v-if="isProjectDirectoryGroup(group)" type="button"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/WindowSideBar.vue` around lines 477 - 480, Remove the redundant v-if="isProjectDirectoryGroup(group)" from the DcButton inside DropdownMenuTrigger in the isActiveProjectDirectoryGroup(group) branch, leaving the trigger and button rendered directly whenever the active directory group condition passes.
438-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
titleattribute on an SVG does not create a native tooltip.
Iconrenders an<svg>element. Browsers show tooltips for SVG only when a<title>child element exists, not from atitleattribute. Thesr-onlyspan at Lines 450-456 keeps the text available to screen readers, so this is visual only. Wrap the icon in a<span :title="...">to restore the hover tooltip.💡 Proposed change
- <Icon - v-if="isWorkspaceUnavailable(group)" - icon="lucide:circle-alert" - data-testid="window-sidebar-workspace-unavailable" - aria-hidden="true" - class="ml-auto size-3.5 shrink-0 text-amber-500" - :title=" - t('chat.input.workspaceUnavailableTooltip', { - path: getGroupIdentifier(group) - }) - " - /> + <span + v-if="isWorkspaceUnavailable(group)" + class="ml-auto flex shrink-0 items-center" + :title=" + t('chat.input.workspaceUnavailableTooltip', { + path: getGroupIdentifier(group) + }) + " + > + <Icon + icon="lucide:circle-alert" + data-testid="window-sidebar-workspace-unavailable" + aria-hidden="true" + class="size-3.5 text-amber-500" + /> + </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/WindowSideBar.vue` around lines 438 - 449, Move the workspace-unavailable tooltip binding from the Icon component to a wrapping span around it, keeping the existing translated message and preserving the Icon’s accessibility and styling attributes. Use the existing isWorkspaceUnavailable, getGroupIdentifier, and translation expression unchanged.src/renderer/src/i18n/fa-IR/chat.json (1)
353-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the RTL layout of the new sidebar indicators.
The translations are correct.
fa-IRandhe-ILare RTL locales.WindowSideBar.vuepositions theEmptylabel (Line 434) and the unavailable icon (Line 443) withml-auto. That class does not flip underdir="rtl". Use the logicalms-autoif the app renders these locales withdir="rtl".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/i18n/fa-IR/chat.json` around lines 353 - 355, Update the sidebar indicator positioning in WindowSideBar.vue by replacing the physical ml-auto utility on the Empty label and unavailable icon with the logical ms-auto utility, preserving correct alignment for both LTR and RTL locales.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/project/index.ts`:
- Around line 253-265: Serialize the post-picker active-environment read and
update sequence around getEnvironments(), markActive(), and reorderActive() so
concurrent selections cannot compute ordering from the same stale list. Ensure
each selection re-reads and reorders the complete active list atomically,
preserving selection order and preventing duplicate sort_order values. Add a
regression test covering concurrent selections.
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 1588-1615: Update handleAddWorkspace to await the project
environment refresh after openFolderPicker resolves, and assign
projectEnvironmentMetadataReady from the refresh result instead of setting it
unconditionally; then ensure revealWorkspaceGroup performs its [data-group-id]
lookup after the refresh or retries across several animation frames before
giving up. Apply these changes at src/renderer/src/components/WindowSideBar.vue
lines 1588-1615 and 1559-1575.
- Around line 930-943: Normalize project paths with the same separator-free path
normalization used by groupByProject before creating
activeProjectEnvironmentByPath, historicalProjectEnvironmentByPath,
selectableProjectPathSet, and projectOrderIndex. Apply that normalization to
every key and lookup in sessionGroupByPath as well, preserving lookups for paths
with trailing separators.
In `@src/renderer/src/stores/ui/project.ts`:
- Around line 280-286: Update openFolderPicker and the selectDirectory flow to
return the bumped snapshot version together with the selected path, then pass
that version as minVersion to refreshProjectSnapshot so validation waits for the
correct refresh. If post-selectProject validation fails, restore the previous
workspace selection before propagating the error.
---
Outside diff comments:
In `@src/renderer/src/stores/ui/session.ts`:
- Around line 1311-1314: Update the group-mode persistence flow around the
persistError catch to track the last successfully persisted mode and restore
that value on any failed queued write, rather than using the request-local
previousMode. Ensure successful writes update the tracked persisted mode, and
add a regression test covering two queued failures where rollback must return to
the durable mode.
---
Nitpick comments:
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 477-480: Remove the redundant
v-if="isProjectDirectoryGroup(group)" from the DcButton inside
DropdownMenuTrigger in the isActiveProjectDirectoryGroup(group) branch, leaving
the trigger and button rendered directly whenever the active directory group
condition passes.
- Around line 438-449: Move the workspace-unavailable tooltip binding from the
Icon component to a wrapping span around it, keeping the existing translated
message and preserving the Icon’s accessibility and styling attributes. Use the
existing isWorkspaceUnavailable, getGroupIdentifier, and translation expression
unchanged.
In `@src/renderer/src/i18n/fa-IR/chat.json`:
- Around line 353-355: Update the sidebar indicator positioning in
WindowSideBar.vue by replacing the physical ml-auto utility on the Empty label
and unavailable icon with the logical ms-auto utility, preserving correct
alignment for both LTR and RTL locales.
In `@src/renderer/src/stores/ui/project.ts`:
- Around line 288-291: Update the folder-picker error handling around
refreshProjectSnapshot so errors already produced by that snapshot flow are not
rewrapped as “Failed to open folder picker.” Preserve the original snapshot
error in error.value while retaining the existing behavior for genuine
folder-picker failures and rethrowing the cause.
In `@test/renderer/components/WindowSideBar.test.ts`:
- Around line 1073-1103: Update the concurrency test to trigger the rendered
add-workspace button twice instead of calling the internal handleAddWorkspace
method directly. Use the existing button selector and preserve the assertions
covering a single picker invocation, disabled state, cancellation side effects,
and concurrent-request behavior.
- Around line 60-74: Update createEnvironment so fallbackStatus-derived fixtures
also receive the corresponding archivedAt or removedAt timestamp, matching the
projectStore.test.ts helper; preserve explicitly supplied timestamps and leave
unrelated status handling unchanged.
- Around line 1045-1048: Update the focus assertion in the test around the group
ordering expectations to verify that the registered row element is the receiver
of focus, rather than only checking that focusSpy was called. Preserve the
existing ordering assertions and use the row’s focus target or registered-row
selector already established in the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 455d099f-dfab-43d2-921f-48de66bbff58
📒 Files selected for processing (34)
docs/features/complete-directory-management/spec.mddocs/features/sidebar-workspace-registration/plan.mddocs/features/sidebar-workspace-registration/spec.mddocs/features/sidebar-workspace-registration/tasks.mdsrc/main/project/index.tssrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/project.tssrc/renderer/src/stores/ui/session.tstest/main/project/data/tables/newEnvironmentPreferencesTable.test.tstest/main/project/projectService.test.tstest/renderer/components/WindowSideBar.test.tstest/renderer/stores/projectStore.test.tstest/renderer/stores/sessionStore.test.ts
| const handleAddWorkspace = async () => { | ||
| if (isAddingWorkspace.value) { | ||
| return | ||
| } | ||
|
|
||
| isAddingWorkspace.value = true | ||
| try { | ||
| const selectedPath = await projectStore.openFolderPicker({ select: false }) | ||
| if (!selectedPath) { | ||
| return | ||
| } | ||
|
|
||
| projectEnvironmentMetadataReady.value = true | ||
| sessionSearchQuery.value = '' | ||
| await sessionStore.setGroupMode('project') | ||
| await revealWorkspaceGroup(selectedPath) | ||
| } catch (error) { | ||
| console.warn('[WindowSideBar] Failed to add workspace:', error) | ||
| notifyRenderer({ | ||
| kind: 'error', | ||
| code: 'chat.workspace.registrationFailed', | ||
| title: t('common.error.operationFailed'), | ||
| description: t('chat.sidebar.addWorkspaceFailed') | ||
| }) | ||
| } finally { | ||
| isAddingWorkspace.value = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
handleAddWorkspace does not await an environment refresh after the folder pick. The flow marks the environment metadata ready and then reveals the group, but nothing guarantees that projectStore.environments already contains the new path. Both sites fail in the same way when the store snapshot is still in flight: the merged projection reads stale environments, and the reveal query finds no [data-group-id] element.
src/renderer/src/components/WindowSideBar.vue#L1588-L1615: await an environment refresh afteropenFolderPickerresolves, and setprojectEnvironmentMetadataReadyfrom that result instead of setting it unconditionally at Line 1600.src/renderer/src/components/WindowSideBar.vue#L1559-L1575: run the[data-group-id]lookup after the refresh resolves, or retry the lookup across a few animation frames before you give up.
📍 Affects 1 file
src/renderer/src/components/WindowSideBar.vue#L1588-L1615(this comment)src/renderer/src/components/WindowSideBar.vue#L1559-L1575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/components/WindowSideBar.vue` around lines 1588 - 1615,
Update handleAddWorkspace to await the project environment refresh after
openFolderPicker resolves, and assign projectEnvironmentMetadataReady from the
refresh result instead of setting it unconditionally; then ensure
revealWorkspaceGroup performs its [data-group-id] lookup after the refresh or
retries across several animation frames before giving up. Apply these changes at
src/renderer/src/components/WindowSideBar.vue lines 1588-1615 and 1559-1575.
zhangmo8
left a comment
zhangmo8
left a comment
There was a problem hiding this comment.
LGTM 👍
Reviewed the projection change and verified locally on macOS:
- Sidebar now projects
projectStore.environmentsas the source of truth and merges Session groups as children — exactly the direction #2115 called for. Empty workspaces render with theEmptylabel and clicking them starts a correctly scoped draft. reorderActive's conditional upsert (WHERE status = 'active') cleanly prevents a reorder from resurrecting archived environments, and thewasActiveguard inselectDirectorykeeps duplicate selections order-stable as described.setGroupModepersisting the capturedmode(instead of re-readinggroupMode.value) plus the rollback-and-rethrow chain is a nice concurrency fix along the way.- Both callers of the now-throwing
openFolderPickerhandle the rejection with user-facing feedback. - All 20 locale files contain the three new
chat.sidebar.*keys. - Ran locally: WindowSideBar 62/62, projectStore 13/13, projectService 31/31 (the 4 native SQLite table tests skip as documented), and
pnpm run typecheckpasses.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/main/routes/dispatcher.test.ts (1)
5617-5626: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the published mutation version.
These assertions validate only the route response. The
publishEnvironmentsChangedfixture discards itsversionargument and publishesDate.now(). A regression in archive or selection event-version propagation would pass.Capture the callback arguments. Assert that both mutations publish version
1.As per coding guidelines, “Add the smallest regression test for user-visible behavior or a documented contract.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/routes/dispatcher.test.ts` around lines 5617 - 5626, Update the test covering archive and directory selection mutations to capture the arguments passed to the publishEnvironmentsChanged fixture instead of discarding its version value. Assert that both mutation events publish version 1, while preserving the existing route-response assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/stores/ui/project.ts`:
- Around line 228-231: Update setDefaultProject, reorderEnvironments,
restoreEnvironment, and removeEnvironment to capture the mutation result
returned by their configClient calls, with each route exposing its committed
version. Pass result.version to requireProjectSnapshot instead of 0 so the store
waits for the exact mutation snapshot, and add regression tests covering an
older in-flight refresh for each mutation.
In `@test/renderer/api/clients.test.ts`:
- Line 2925: Update the `bridge.invoke` assertion for `tools.listDefinitions` to
expect invocation 13 instead of 12, while leaving the asserted event and
arguments unchanged.
---
Outside diff comments:
In `@test/main/routes/dispatcher.test.ts`:
- Around line 5617-5626: Update the test covering archive and directory
selection mutations to capture the arguments passed to the
publishEnvironmentsChanged fixture instead of discarding its version value.
Assert that both mutation events publish version 1, while preserving the
existing route-response assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c064c610-1808-4b20-bd22-c5153542bd70
📒 Files selected for processing (23)
docs/features/complete-directory-management/spec.mddocs/features/sidebar-workspace-registration/plan.mddocs/features/sidebar-workspace-registration/spec.mddocs/features/sidebar-workspace-registration/tasks.mdsrc/main/project/data/tables/newEnvironmentPreferences.tssrc/main/project/index.tssrc/main/project/routes.tssrc/renderer/api/ProjectClient.tssrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/project.tssrc/renderer/src/stores/ui/session.tssrc/shared/contracts/routes/project.routes.tssrc/shared/utils/filesystem.tstest/main/project/data/tables/newEnvironmentPreferencesTable.test.tstest/main/project/projectService.test.tstest/main/routes/contracts.test.tstest/main/routes/dispatcher.test.tstest/main/shared/filesystem.test.tstest/renderer/api/clients.test.tstest/renderer/components/WindowSideBar.test.tstest/renderer/stores/projectStore.test.tstest/renderer/stores/sessionStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/features/complete-directory-management/spec.md
- src/renderer/src/components/WindowSideBar.vue
| async function setDefaultProject(path: string | null): Promise<void> { | ||
| try { | ||
| await configClient.setDefaultProjectPath(normalizePath(path)) | ||
| await refreshProjectSnapshot() | ||
| await requireProjectSnapshot() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Return and require the exact mutation snapshot version.
These mutations call requireProjectSnapshot() with version 0. Their routes return no version, although they publish versioned environment or default-project events. If an older refresh is in flight, this code can accept that stale snapshot and report mutation success before the committed projection includes the change.
Return version from config.setDefaultProjectPath, project.reorderEnvironments, project.restoreEnvironment, and project.removeEnvironment. Then pass each returned version to requireProjectSnapshot(result.version). Add stale-refresh regression tests for these mutations.
Proposed store-side change
- await projectClient.reorderEnvironments(orderedPaths)
- await requireProjectSnapshot()
+ const result = await projectClient.reorderEnvironments(orderedPaths)
+ await requireProjectSnapshot(result.version)
- await projectClient.restoreEnvironment(path)
- await requireProjectSnapshot()
+ const result = await projectClient.restoreEnvironment(path)
+ await requireProjectSnapshot(result.version)
const result = await projectClient.removeEnvironment(path)
- await requireProjectSnapshot()
+ await requireProjectSnapshot(result.version)Also applies to: 247-248, 279-280, 289-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/stores/ui/project.ts` around lines 228 - 231, Update
setDefaultProject, reorderEnvironments, restoreEnvironment, and
removeEnvironment to capture the mutation result returned by their configClient
calls, with each route exposing its committed version. Pass result.version to
requireProjectSnapshot instead of 0 so the store waits for the exact mutation
snapshot, and add regression tests covering an older in-flight refresh for each
mutation.
| expect(bridge.on).toHaveBeenCalledWith('project:environments-changed', expect.any(Function)) | ||
| expect(unsubscribe).toEqual(expect.any(Function)) | ||
| expect(bridge.invoke).toHaveBeenNthCalledWith(11, 'tools.listDefinitions', { | ||
| expect(bridge.invoke).toHaveBeenNthCalledWith(12, 'tools.listDefinitions', { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the invocation ordinal.
Line 2921 verifies invocation 11. Line 2887 performs another project.selectDirectory invocation. tools.listDefinitions is invocation 13, not invocation 12. This assertion fails.
Proposed fix
- expect(bridge.invoke).toHaveBeenNthCalledWith(12, 'tools.listDefinitions', {
+ expect(bridge.invoke).toHaveBeenNthCalledWith(13, 'tools.listDefinitions', {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(bridge.invoke).toHaveBeenNthCalledWith(12, 'tools.listDefinitions', { | |
| expect(bridge.invoke).toHaveBeenNthCalledWith(13, 'tools.listDefinitions', { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/renderer/api/clients.test.ts` at line 2925, Update the `bridge.invoke`
assertion for `tools.listDefinitions` to expect invocation 13 instead of 12,
while leaving the asserted event and arguments unchanged.
2 participants
Problem
The sidebar treated Session groups as the source of workspace existence. Selecting a directory could persist a valid active environment, but a zero-Session workspace was filtered out and appeared not to have been added.
The same projection caused several related UX problems:
UX
Before
A selected directory with no Session did not appear.
After
Archivereuses the existing reversible environment-hide flow. It does not introduce a new lifecycle, delete Sessions, delete messages, or touch the real folder.Solution
Emptyand start the first correctly scoped draft through the existing one-shot project intent.projectStore.archiveEnvironment(path)action behind confirmation and failure feedback.Business behavior
projectDir.Validation
pnpm run formatpnpm run i18npnpm run lintpnpm run typecheckValidation notes
/mock/...versus resolvedC:\mock\...); the focused changed contracts pass.Closes #2115
Summary by CodeRabbit
New Features
Bug Fixes
Localization