[codex] Fix active item selection after removal#3767
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds utilities to compute the focus target after removal and to immutably remove workspaces; updates close/delete workspace hooks to use these utilities for optimistic navigation; refactors pane store to use a unified active-after-removal algorithm; and adds tests covering removal and focus-selection behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Hook as Mutation Hook
participant Cache as React-Query Cache
participant Nav as Router/Navigation
participant Utils as Removal Utils
UI->>Hook: trigger close/delete workspace(id)
Hook->>Cache: cancel queries & snapshot previousAll/previousGrouped
Hook->>Utils: getWorkspaceFocusTargetAfterRemoval(previousGrouped, id)
Utils-->>Hook: nextFocusId or null
Hook->>Cache: optimistic update -> removeWorkspaceFromGroups(previousGrouped, id)
Hook->>Nav: navigate to nextFocusId or fallback (/workspace)
alt mutation succeeds
Hook->>Cache: finalize (no extra navigation)
else mutation fails
Hook->>Cache: rollback previousAll/previousGrouped
Hook->>Nav: navigate back to removed id if wasViewingClosed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR introduces a unified Confidence Score: 5/5Safe to merge — all changes are well-tested refactors with no regressions identified. All findings are P2 or lower. The new shared utility is thoroughly unit-tested, error rollback paths correctly restore both cache and navigation state, and the intentional next-then-previous policy change is consistent and documented. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/panes/src/core/store/utils/utils.ts | Adds getPaneIdsInLayout (DFS traversal returning ordered pane IDs) and getActiveIdAfterRemoval (next-then-previous selection logic); both are well-tested and handle null/missing activeId gracefully. |
| packages/panes/src/core/store/store.ts | Replaces findSiblingPaneId-based and first-tab fallback selection with getActiveIdAfterRemoval across removeTab, closePane, and three pane-move paths; consistently uses the pre-removal layout for ordered IDs and keeps the findFirstPaneId fallback for the null-activePaneId case. |
| apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts | New shared utilities: getFlattenedWorkspaceIdsFromGroups (sidebar visual order), getWorkspaceFocusTargetAfterRemoval, and the refactored removeWorkspaceFromGroups; logic is clean and mirrors the group-filtering behavior previously inlined in both hooks. |
| apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts | Navigation now happens in onMutate using pre-removal data instead of post-success; adds error rollback navigation and lazy getAllGrouped.fetch() when cache is cold and user is viewing the closed workspace. |
| apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts | Moves cancel + snapshot before navigation (fixing the old ordering where navigation happened before the cancel/snapshot), and delegates focus-target selection to getWorkspaceFocusTargetAfterRemoval. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts | Replaced custom prev-then-next index logic with getActiveIdAfterRemoval, switching to the unified next-then-previous policy. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.ts | Replaces ids.find(id => id !== workspaceId) (always picked the first non-matching ID regardless of position) with getDeleteFocusTargetWorkspaceId for correct position-relative sibling selection. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User removes workspace/tab/pane] --> B{Is active item being removed?}
B -- No --> C[Keep current activeId unchanged]
B -- Yes --> D[getActiveIdAfterRemoval\norderedIds, activeId, removedId]
D --> E{removedId in orderedIds?}
E -- No --> F[Return orderedIds[0] ?? null]
E -- Yes --> G{Has next item?}
G -- Yes --> H[Return next item]
G -- No --> I{Has previous item?}
I -- Yes --> J[Return previous item]
I -- No --> K[Return null]
Reviews (1): Last reviewed commit: "Fix active item selection after removal" | Re-trigger Greptile
Kitenite
force-pushed
the
fix-tab-close-selection
branch
from
07f8b4a to
37515eb
Compare
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts">
<violation number="1" location="apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts:45">
P2: Avoid silently swallowing `getAllGrouped.fetch()` failures; handle/log the error so this failure path is observable.
(Based on your team's feedback about handling async failures explicitly and avoiding silent error swallowing.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (1)
35-83:⚠️ Potential issue | 🟡 MinorRemove
navigatedTofrom the returned context or wire it up to a handler.
navigatedTois assigned at lines 55 and 58 and returned in the context, but it's never consumed. The hook's own handlers (onSuccess,onError,onSettled) only readpreviousGrouped,previousAll, andwasViewingDeletedfrom the context. External callers invoke the mutation without passing custom options handlers. This makesnavigatedTodead code that can drift out of sync with actual navigation on future edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts` around lines 35 - 83, The returned DeleteContext includes a navigatedTo value that is set by code using getWorkspaceFocusTargetAfterRemoval, navigateToWorkspace, and navigate but is never consumed by the hook's handlers (onSuccess/onError/onSettled) or by external callers, so remove navigatedTo from the returned context (and from the local variable assignment) OR wire it into the mutation lifecycle by returning it in the context and using it in the hook's onSuccess/onSettled handler (or exposing it via the mutation result) so it is actually read; update the DeleteContext type and references in useDeleteWorkspace (remove usages of navigatedTo or add usage in onSuccess/onSettled) and ensure getWorkspaceFocusTargetAfterRemoval, navigateToWorkspace, and navigate calls remain unchanged.
🧹 Nitpick comments (8)
packages/panes/src/core/store/utils/utils.test.ts (1)
96-146: Solid coverage; consider one extra case.The five cases capture the main branches well. One nit: there's no explicit test for
activeId === null | undefined(the?? nullbranch on Line 46 ofutils.ts). Adding one assertion (e.g.activeId: null, removedId: "b"→null) would lock in that behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/utils/utils.test.ts` around lines 96 - 146, Add a test covering the branch where activeId is null/undefined: in the getActiveIdAfterRemoval suite add an "it" case that calls getActiveIdAfterRemoval with orderedIds: ["a","b"], activeId: null (or undefined), removedId: "b" and assert the result is null; reference the getActiveIdAfterRemoval function to locate where to add the new test and mirror the style of the existing test cases.packages/panes/src/core/store/utils/utils.ts (1)
36-55: Note: "missing id" fallback returns the first ordered id, not null.When
activeId === removedIdbutremovedIdis not present inorderedIds(Line 50–52), the function returnsorderedIds[0] ?? nullrather thannull. This is a deliberate fallback (covered by the "falls back to the first id when the removed id is missing" test), but it's a subtle semantic that callers should be aware of — especiallygetDeleteFocusTargetWorkspaceId, which previously returnednullin this case. Consider a brief JSDoc on this function documenting the missing-id behavior so future consumers don't have to read the test to understand it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/utils/utils.ts` around lines 36 - 55, Add a concise JSDoc to the getActiveIdAfterRemoval function explaining its missing-id fallback: when activeId === removedId but removedId is not found in orderedIds, the function intentionally returns the first id in orderedIds (orderedIds[0]) rather than null, otherwise it returns activeId or the next/previous id; mention the return type (string|null) and the exact semantic so callers (e.g., getDeleteFocusTargetWorkspaceId) know this behavior without inspecting tests or implementation.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.test.ts (1)
4-20: Consider a test for the "deleted id not in list" case.The three covered scenarios match the wrapper's current behavior, but given the semantic shift introduced by delegating to
getActiveIdAfterRemoval(see comment ongetDeleteFocusTargetWorkspaceId.ts), an explicit test forgetDeleteFocusTargetWorkspaceId(["w1","w2"], "w3")would lock down the chosen behavior (currently"w1").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.test.ts` around lines 4 - 20, Add a unit test to cover the case where the deleted id is not present in the list by updating the getDeleteFocusTargetWorkspaceId tests: in getDeleteFocusTargetWorkspaceId.test.ts add a new it block that calls getDeleteFocusTargetWorkspaceId(["w1","w2"], "w3") and asserts the current chosen behavior (expect(...).toBe("w1")). Use a descriptive test name like "returns fallback when deleted id is not in list" so the wrapper behavior delegating to getActiveIdAfterRemoval is locked down.apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts (2)
36-45: Minor:hasVisibleWorkspacescan short-circuit.
reducealways walks every section even after the count exceeds zero.Array.prototype.somereads more clearly and exits early.♻️ Suggested rewrite
function hasVisibleWorkspaces(group: WorkspaceGroupLike): boolean { - return ( - group.workspaces.length + - group.sections.reduce( - (sum, section) => sum + section.workspaces.length, - 0, - ) > - 0 - ); + return ( + group.workspaces.length > 0 || + group.sections.some((section) => section.workspaces.length > 0) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts` around lines 36 - 45, The current hasVisibleWorkspaces function sums all section workspace counts with reduce which prevents early exit; change it to short-circuit by returning true if group.workspaces.length > 0 or if group.sections.some(section => section.workspaces.length > 0). Update the function (referencing hasVisibleWorkspaces, group.workspaces, group.sections, and section.workspaces) to use Array.prototype.some so it stops as soon as a visible workspace is found.
47-79: Flattening anchors ontopLevelItems— make sure that's the data invariant.The function ignores any workspace that's in
group.workspacesbut absent fromtopLevelItems, and silently skips section-typed top-level items whoseidisn't insectionsById. Both are correct under the invariant thattopLevelItemsis the source of truth for visible top-level entries and is kept consistent withworkspaces/sections. If that ever drifts (e.g., a backend shape change), the flattened list will silently lose entries andgetActiveIdAfterRemovalwill pick a non-adjacent target. Worth a brief comment documenting that invariant on the function or type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts` around lines 47 - 79, Add a brief comment above getFlattenedWorkspaceIdsFromGroups documenting the data invariant that group.topLevelItems is the authoritative list of visible top-level entries and must stay consistent with group.workspaces and group.sections; explain that the function intentionally ignores workspaces/sections not referenced in topLevelItems (and that this behavior affects getActiveIdAfterRemoval by choosing adjacency based only on topLevelItems), and note that if the backend shape drifts a runtime validation or fallback should be added to avoid silently dropping entries.packages/panes/src/core/store/store.test.ts (1)
42-66: LGTM — middle/last active-tab removal policy is well covered.The two new tests cleanly validate the next-then-previous fallback for the active tab. Worth considering a small additional case for completeness: removing a non-active tab and asserting
activeTabIdis unchanged. This guards against future regressions ingetActiveIdAfterRemoval's no-op branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.test.ts` around lines 42 - 66, Add a test that verifies removing a non-active tab does not change the active tab: create a store with multiple tabs via addTab, set one tab active using setActiveTab, then remove a different (non-active) tab with removeTab and assert tabs list updated while activeTabId remains the same; this guards the no-op branch in getActiveIdAfterRemoval and should reference removeTab and activeTabId for clarity.packages/panes/src/core/store/store.ts (1)
281-298: Consistent helper usage; consider extracting the repeated fallback.The pattern
getActiveIdAfterRemoval({ orderedIds: getPaneIdsInLayout(prevLayout), activeId, removedId: paneId }) ?? findFirstPaneId(nextLayout)recurs inclosePane,movePaneToSplit,movePaneToTab, andmovePaneToNewTab. A small helper (e.g.,nextActivePaneIdAfterRemoval(prevLayout, nextLayout, activeId, removedId)) would centralize the contract — particularly the "fall back to first pane in new layout when ordering can't determine a target" behavior — and make any future tweak land in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 281 - 298, The code repeats the fallback pattern for computing the new active pane ID; implement a small helper function (e.g., nextActivePaneIdAfterRemoval(prevLayout, nextLayout, activeId, removedId)) that calls getActiveIdAfterRemoval({ orderedIds: getPaneIdsInLayout(prevLayout), activeId, removedId }) and returns that result or findFirstPaneId(nextLayout) as the fallback, then replace the repeated expressions in closePane, movePaneToSplit, movePaneToTab, and movePaneToNewTab to call this helper (use the same argument names to preserve intent).apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts (1)
88-124: Duplicate rollback path betweenonSuccess(success:false) andonError.The cache rollback + re-navigation block is repeated verbatim in both handlers. Extracting a small
rollbackOptimisticDelete(context, variables)closure inside the hook would remove ~15 lines and keep the two failure paths in lockstep when the rollback contract evolves.♻️ Sketch
+ const rollback = (context: DeleteContext | undefined, id: string) => { + if (context?.previousGrouped !== undefined) { + utils.workspaces.getAllGrouped.setData(undefined, context.previousGrouped); + } + if (context?.previousAll !== undefined) { + utils.workspaces.getAll.setData(undefined, context.previousAll); + } + if (context?.wasViewingDeleted) { + navigateToWorkspace(id, navigate); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts` around lines 88 - 124, Extract the duplicated rollback + navigation logic from the onSuccess and onError handlers into a small helper function (e.g., rollbackOptimisticDelete) inside useDeleteWorkspace; the helper should accept (context, variables) and perform the current operations: restore utils.workspaces.getAllGrouped.setData(undefined, context.previousGrouped) and utils.workspaces.getAll.setData(undefined, context.previousAll) when those context fields are defined, and call navigateToWorkspace(variables.id, navigate) when context.wasViewingDeleted is true; then call this helper from both onSuccess (when data.success is false) and onError to keep rollback behavior centralized and in sync, preserving the await options?.onSuccess/onError calls afterwards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts`:
- Around line 7-11: The current implementation of
getDeleteFocusTargetWorkspaceId delegates to getActiveIdAfterRemoval even when
deletedWorkspaceId is not present in flattenedWorkspaceIds, causing it to fall
back to the first workspace; add an explicit guard in
getDeleteFocusTargetWorkspaceId to check whether flattenedWorkspaceIds includes
deletedWorkspaceId and return null if it does not, otherwise call
getActiveIdAfterRemoval with orderedIds: flattenedWorkspaceIds, activeId:
deletedWorkspaceId, removedId: deletedWorkspaceId; reference
getDeleteFocusTargetWorkspaceId, getActiveIdAfterRemoval, flattenedWorkspaceIds,
and deletedWorkspaceId so the change is easy to locate.
---
Outside diff comments:
In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts`:
- Around line 35-83: The returned DeleteContext includes a navigatedTo value
that is set by code using getWorkspaceFocusTargetAfterRemoval,
navigateToWorkspace, and navigate but is never consumed by the hook's handlers
(onSuccess/onError/onSettled) or by external callers, so remove navigatedTo from
the returned context (and from the local variable assignment) OR wire it into
the mutation lifecycle by returning it in the context and using it in the hook's
onSuccess/onSettled handler (or exposing it via the mutation result) so it is
actually read; update the DeleteContext type and references in
useDeleteWorkspace (remove usages of navigatedTo or add usage in
onSuccess/onSettled) and ensure getWorkspaceFocusTargetAfterRemoval,
navigateToWorkspace, and navigate calls remain unchanged.
---
Nitpick comments:
In `@apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts`:
- Around line 88-124: Extract the duplicated rollback + navigation logic from
the onSuccess and onError handlers into a small helper function (e.g.,
rollbackOptimisticDelete) inside useDeleteWorkspace; the helper should accept
(context, variables) and perform the current operations: restore
utils.workspaces.getAllGrouped.setData(undefined, context.previousGrouped) and
utils.workspaces.getAll.setData(undefined, context.previousAll) when those
context fields are defined, and call navigateToWorkspace(variables.id, navigate)
when context.wasViewingDeleted is true; then call this helper from both
onSuccess (when data.success is false) and onError to keep rollback behavior
centralized and in sync, preserving the await options?.onSuccess/onError calls
afterwards.
In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts`:
- Around line 36-45: The current hasVisibleWorkspaces function sums all section
workspace counts with reduce which prevents early exit; change it to
short-circuit by returning true if group.workspaces.length > 0 or if
group.sections.some(section => section.workspaces.length > 0). Update the
function (referencing hasVisibleWorkspaces, group.workspaces, group.sections,
and section.workspaces) to use Array.prototype.some so it stops as soon as a
visible workspace is found.
- Around line 47-79: Add a brief comment above
getFlattenedWorkspaceIdsFromGroups documenting the data invariant that
group.topLevelItems is the authoritative list of visible top-level entries and
must stay consistent with group.workspaces and group.sections; explain that the
function intentionally ignores workspaces/sections not referenced in
topLevelItems (and that this behavior affects getActiveIdAfterRemoval by
choosing adjacency based only on topLevelItems), and note that if the backend
shape drifts a runtime validation or fallback should be added to avoid silently
dropping entries.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.test.ts`:
- Around line 4-20: Add a unit test to cover the case where the deleted id is
not present in the list by updating the getDeleteFocusTargetWorkspaceId tests:
in getDeleteFocusTargetWorkspaceId.test.ts add a new it block that calls
getDeleteFocusTargetWorkspaceId(["w1","w2"], "w3") and asserts the current
chosen behavior (expect(...).toBe("w1")). Use a descriptive test name like
"returns fallback when deleted id is not in list" so the wrapper behavior
delegating to getActiveIdAfterRemoval is locked down.
In `@packages/panes/src/core/store/store.test.ts`:
- Around line 42-66: Add a test that verifies removing a non-active tab does not
change the active tab: create a store with multiple tabs via addTab, set one tab
active using setActiveTab, then remove a different (non-active) tab with
removeTab and assert tabs list updated while activeTabId remains the same; this
guards the no-op branch in getActiveIdAfterRemoval and should reference
removeTab and activeTabId for clarity.
In `@packages/panes/src/core/store/store.ts`:
- Around line 281-298: The code repeats the fallback pattern for computing the
new active pane ID; implement a small helper function (e.g.,
nextActivePaneIdAfterRemoval(prevLayout, nextLayout, activeId, removedId)) that
calls getActiveIdAfterRemoval({ orderedIds: getPaneIdsInLayout(prevLayout),
activeId, removedId }) and returns that result or findFirstPaneId(nextLayout) as
the fallback, then replace the repeated expressions in closePane,
movePaneToSplit, movePaneToTab, and movePaneToNewTab to call this helper (use
the same argument names to preserve intent).
In `@packages/panes/src/core/store/utils/utils.test.ts`:
- Around line 96-146: Add a test covering the branch where activeId is
null/undefined: in the getActiveIdAfterRemoval suite add an "it" case that calls
getActiveIdAfterRemoval with orderedIds: ["a","b"], activeId: null (or
undefined), removedId: "b" and assert the result is null; reference the
getActiveIdAfterRemoval function to locate where to add the new test and mirror
the style of the existing test cases.
In `@packages/panes/src/core/store/utils/utils.ts`:
- Around line 36-55: Add a concise JSDoc to the getActiveIdAfterRemoval function
explaining its missing-id fallback: when activeId === removedId but removedId is
not found in orderedIds, the function intentionally returns the first id in
orderedIds (orderedIds[0]) rather than null, otherwise it returns activeId or
the next/previous id; mention the return type (string|null) and the exact
semantic so callers (e.g., getDeleteFocusTargetWorkspaceId) know this behavior
without inspecting tests or implementation.
🪄 Autofix (Beta)
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
Run ID: 7e518be1-ab5a-4d47-af58-1c9d30c6a087
📒 Files selected for processing (13)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.tspackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/panes/src/core/store/store.ts (1)
288-294: Optional: extract thegetActiveIdAfterRemoval(...) ?? findFirstPaneId(...)pattern.The same 6-line invocation appears in
closePane,movePaneToSplit,movePaneToTab, andmovePaneToNewTab. A tiny local helper would reduce duplication and make the intent (recompute active pane after removingremovedIdfromoldLayout, with a first-pane safety net) explicit at each call site.♻️ Suggested helper
function nextActivePaneId( oldLayout: LayoutNode, nextLayout: LayoutNode, activeId: string | null | undefined, removedId: string, ): string | null { return ( getActiveIdAfterRemoval({ orderedIds: getPaneIdsInLayout(oldLayout), activeId, removedId, }) ?? findFirstPaneId(nextLayout) ); }Each call site then becomes:
- activePaneId: - getActiveIdAfterRemoval({ - orderedIds: getPaneIdsInLayout(sourceTab.layout), - activeId: t.activePaneId, - removedId: args.paneId, - }) ?? findFirstPaneId(nextSourceLayout), + activePaneId: nextActivePaneId( + sourceTab.layout, + nextSourceLayout, + t.activePaneId, + args.paneId, + ),Also applies to: 692-697, 755-760, 811-816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 288 - 294, Extract a small helper (e.g., nextActivePaneId) that wraps the repeated pattern getActiveIdAfterRemoval({ orderedIds: getPaneIdsInLayout(oldLayout), activeId, removedId }) ?? findFirstPaneId(nextLayout), then replace the six-line duplicates in closePane, movePaneToSplit, movePaneToTab, and movePaneToNewTab with calls to that helper; use the same parameter names (oldLayout/tab.layout, nextLayout, activeId/tab.activePaneId, removedId/args.paneId) so callers set activePaneId using nextActivePaneId(oldLayout, nextLayout, tab.activePaneId, args.paneId).apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts (1)
92-121: Empty sections (and theirtopLevelItemsentries) persist after their last workspace is removed.When removing the only workspace in a section, the section remains in
group.sectionswithworkspaces: []and the corresponding section entry stays ingroup.topLevelItems.hasVisibleWorkspacesonly filters whole groups, so the orphan section row will still render until the next refetch. If this is intentional (sections-as-containers), feel free to ignore; otherwise consider pruning empty sections here.♻️ Optional cleanup of empty sections
- const sections = group.sections.map((section) => ({ + const sections = group.sections + .map((section) => ({ ...section, workspaces: section.workspaces.filter( (workspace) => workspace.id !== workspaceId, ), - })); + })) + .filter((section) => section.workspaces.length > 0); + const removedSectionIds = new Set( + group.sections + .filter( + (section) => + section.workspaces.some((w) => w.id === workspaceId) && + section.workspaces.length === 1, + ) + .map((section) => section.id), + ); return { ...group, workspaces, sections, - topLevelItems: isTopLevelWorkspace - ? group.topLevelItems.filter((item) => item.id !== workspaceId) - : group.topLevelItems, + topLevelItems: group.topLevelItems.filter( + (item) => + !(isTopLevelWorkspace && item.id === workspaceId) && + !removedSectionIds.has(item.id), + ), } as TGroup;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts` around lines 92 - 121, The removeWorkspaceFromGroups function currently leaves section objects with workspaces: [] and their topLevelItems entries when the last workspace in a section is removed; update removeWorkspaceFromGroups to prune sections that become empty and also remove their corresponding topLevelItems: inside the map for each group (the block that builds sections and topLevelItems), after filtering section.workspaces remove any section whose workspaces length is 0 and, when doing so, also filter out the matching topLevelItems entry (matching by section.id) so orphaned section rows are not left behind; keep the overall return shape and still use hasVisibleWorkspaces to filter groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts`:
- Around line 92-121: The removeWorkspaceFromGroups function currently leaves
section objects with workspaces: [] and their topLevelItems entries when the
last workspace in a section is removed; update removeWorkspaceFromGroups to
prune sections that become empty and also remove their corresponding
topLevelItems: inside the map for each group (the block that builds sections and
topLevelItems), after filtering section.workspaces remove any section whose
workspaces length is 0 and, when doing so, also filter out the matching
topLevelItems entry (matching by section.id) so orphaned section rows are not
left behind; keep the overall return shape and still use hasVisibleWorkspaces to
filter groups.
In `@packages/panes/src/core/store/store.ts`:
- Around line 288-294: Extract a small helper (e.g., nextActivePaneId) that
wraps the repeated pattern getActiveIdAfterRemoval({ orderedIds:
getPaneIdsInLayout(oldLayout), activeId, removedId }) ??
findFirstPaneId(nextLayout), then replace the six-line duplicates in closePane,
movePaneToSplit, movePaneToTab, and movePaneToNewTab with calls to that helper;
use the same parameter names (oldLayout/tab.layout, nextLayout,
activeId/tab.activePaneId, removedId/args.paneId) so callers set activePaneId
using nextActivePaneId(oldLayout, nextLayout, tab.activePaneId, args.paneId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 591a935b-42ae-48a0-a543-fa04015e299d
📒 Files selected for processing (13)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.tspackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/panes/src/core/store/utils/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.ts
- apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.ts
- packages/panes/src/core/store/store.test.ts
Kitenite
force-pushed
the
fix-tab-close-selection
branch
from
37515eb to
773456e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/panes/src/core/store/store.ts (1)
686-698: Optional: extract thegetActiveIdAfterRemoval(...) ?? findFirstPaneId(nextLayout)pattern into a helper.The same 5-line
activePaneIdcomputation appears inclosePane,movePaneToSplit,movePaneToTab, andmovePaneToNewTab. A small helper (e.g.pickActivePaneAfterRemoval(prevLayout, nextLayout, activeId, removedId)) would deduplicate it and make the intent at each call site one line.♻️ Sketch
function pickActivePaneAfterRemoval( prevLayout: LayoutNode, nextLayout: LayoutNode, activeId: string | null | undefined, removedId: string, ): string | null { return ( getActiveIdAfterRemoval(getPaneIdsInLayout(prevLayout), activeId, removedId) ?? findFirstPaneId(nextLayout) ); }Also applies to: 749-762, 805-817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 686 - 698, Extract the repeated activePaneId selection into a small helper (e.g. pickActivePaneAfterRemoval(prevLayout, nextLayout, activeId, removedId)) that returns getActiveIdAfterRemoval(getPaneIdsInLayout(prevLayout), activeId, removedId) ?? findFirstPaneId(nextLayout); then replace the 5-line expressions in closePane, movePaneToSplit, movePaneToTab, and movePaneToNewTab (and the other occurrences around the shown diff) with a single call to that helper using sourceTab.layout / nextSourceLayout (or the appropriate prev/next layout variables) and the current activePaneId and removed pane id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts`:
- Around line 74-103: The current removeWorkspaceFromGroups function removes the
workspace from group.sections[*].workspaces but leaves section objects (and
their corresponding topLevelItems entries of kind:"section") even when they
become empty; update removeWorkspaceFromGroups to prune sections whose
workspaces array becomes empty and also remove any topLevelItems entries that
reference those emptied sections (in addition to the existing topLevelItems
removal for top-level workspaces), ensuring you still return TGroup and respect
hasVisibleWorkspaces filtering; locate logic around group.sections and
group.topLevelItems in removeWorkspaceFromGroups to implement the
drop-empty-sections and topLevelItems cleanup.
---
Nitpick comments:
In `@packages/panes/src/core/store/store.ts`:
- Around line 686-698: Extract the repeated activePaneId selection into a small
helper (e.g. pickActivePaneAfterRemoval(prevLayout, nextLayout, activeId,
removedId)) that returns getActiveIdAfterRemoval(getPaneIdsInLayout(prevLayout),
activeId, removedId) ?? findFirstPaneId(nextLayout); then replace the 5-line
expressions in closePane, movePaneToSplit, movePaneToTab, and movePaneToNewTab
(and the other occurrences around the shown diff) with a single call to that
helper using sourceTab.layout / nextSourceLayout (or the appropriate prev/next
layout variables) and the current activePaneId and removed pane id.
🪄 Autofix (Beta)
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
Run ID: fd39f9d3-dbae-475e-884e-273435ac7091
📒 Files selected for processing (13)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.tspackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts
✅ Files skipped from review due to trivial changes (1)
- packages/panes/src/core/store/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.ts
- packages/panes/src/core/store/store.test.ts
Comment on lines
+74
to
+103
| export function removeWorkspaceFromGroups<TGroup extends WorkspaceGroupLike>( | ||
| groups: readonly TGroup[], | ||
| workspaceId: string, | ||
| ): TGroup[] { | ||
| return groups | ||
| .map((group) => { | ||
| const isTopLevelWorkspace = group.workspaces.some( | ||
| (workspace) => workspace.id === workspaceId, | ||
| ); | ||
| const workspaces = group.workspaces.filter( | ||
| (workspace) => workspace.id !== workspaceId, | ||
| ); | ||
| const sections = group.sections.map((section) => ({ | ||
| ...section, | ||
| workspaces: section.workspaces.filter( | ||
| (workspace) => workspace.id !== workspaceId, | ||
| ), | ||
| })); | ||
|
|
||
| return { | ||
| ...group, | ||
| workspaces, | ||
| sections, | ||
| topLevelItems: isTopLevelWorkspace | ||
| ? group.topLevelItems.filter((item) => item.id !== workspaceId) | ||
| : group.topLevelItems, | ||
| } as TGroup; | ||
| }) | ||
| .filter(hasVisibleWorkspaces); | ||
| } |
Contributor
There was a problem hiding this comment.
Empty sections are preserved in sections and topLevelItems — confirm this is intended.
removeWorkspaceFromGroups removes the workspace from section.workspaces but never prunes a section that becomes empty, nor does it remove the corresponding { kind: "section" } entry from topLevelItems. getWorkspaceIdsFromGroups correctly skips empty sections when computing focus order, so navigation works — but the sidebar will continue to render the empty section header until the next refetch.
If empty sections are intended to remain visible (so the user can still drop workspaces back into them), this is fine; otherwise consider also dropping empty sections here for visual consistency with the optimistic update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts`
around lines 74 - 103, The current removeWorkspaceFromGroups function removes
the workspace from group.sections[*].workspaces but leaves section objects (and
their corresponding topLevelItems entries of kind:"section") even when they
become empty; update removeWorkspaceFromGroups to prune sections whose
workspaces array becomes empty and also remove any topLevelItems entries that
reference those emptied sections (in addition to the existing topLevelItems
removal for top-level workspaces), ensuring you still return TGroup and respect
hasVisibleWorkspaces filtering; locate logic around group.sections and
group.topLevelItems in removeWorkspaceFromGroups to implement the
drop-empty-sections and topLevelItems cleanup.
773456e to
3b2c4b8
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (1)
88-102:⚠️ Potential issue | 🟠 Major
onErrordoes not invoke the consumer-suppliedoptions?.onError.
onSuccessat line 110 properly composesoptions?.onSuccess, and the parallel hookuseDeleteWorkspace(line 118) composesoptions?.onError. Here, however, callers passing anonErrortouseCloseWorkspacewill silently never receive errors —...optionsis spread first but the explicitonErrordefined on the same object overrides it. Worth fixing while this block is being touched.🛠️ Proposed fix
- onError: (_err, variables, context) => { + onError: async (_err, variables, context, ...rest) => { // Rollback to previous state on error if (context?.previousGrouped !== undefined) { utils.workspaces.getAllGrouped.setData( undefined, context.previousGrouped, ); } if (context?.previousAll !== undefined) { utils.workspaces.getAll.setData(undefined, context.previousAll); } if (context?.wasViewingClosed) { navigateToWorkspace(variables.id, navigate); } + await options?.onError?.(_err, variables, context, ...rest); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts` around lines 88 - 102, The onError in useCloseWorkspace currently performs rollback logic but does not call the consumer-supplied options?.onError; update the onError handler in useCloseWorkspace (the handler that rolls back utils.workspaces.getAllGrouped/setData, utils.workspaces.getAll.setData and navigateToWorkspace) to invoke options?.onError(err, variables, context) (or await it if it returns a promise) after performing rollback (or before, if you prefer ordering consistent with useDeleteWorkspace) so callers receive their error callback; ensure you reference the same variables (err, variables, context) and preserve existing rollback/navigation behavior and the surrounding mutation options composition.
🧹 Nitpick comments (1)
packages/panes/src/core/store/store.ts (1)
686-699: Optional: extract the repeatedactivePaneId-after-removal computation.The same 5-line snippet appears in
closePane,movePaneToSplit,movePaneToTab, andmovePaneToNewTab:getActiveIdAfterRemoval( getPaneIdsInLayout(sourceTab.layout), t.activePaneId, args.sourcePaneId, // or args.paneId ) ?? findFirstPaneId(nextSourceLayout)A small file-local helper would centralize the "next active pane after pane removal" rule (originalLayout, activePaneId, removedPaneId, nextLayout) and make future changes to the policy a single-edit fix.
♻️ Sketch
function nextActivePaneIdAfterRemoval( originalLayout: LayoutNode, activePaneId: string | null | undefined, removedPaneId: string, nextLayout: LayoutNode | null, ): string | null { return ( getActiveIdAfterRemoval( getPaneIdsInLayout(originalLayout), activePaneId, removedPaneId, ) ?? (nextLayout ? findFirstPaneId(nextLayout) : null) ); }Then each call site becomes:
activePaneId: nextActivePaneIdAfterRemoval( sourceTab.layout, t.activePaneId, args.sourcePaneId, nextSourceLayout, ),Also applies to: 749-762, 803-818
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 686 - 699, Introduce a small file-local helper (e.g., nextActivePaneIdAfterRemoval) that wraps getActiveIdAfterRemoval(getPaneIdsInLayout(originalLayout), activePaneId, removedPaneId) ?? (nextLayout ? findFirstPaneId(nextLayout) : null) and replace the duplicated 5-line expression in the closePane, movePaneToSplit, movePaneToTab, and movePaneToNewTab call sites (where you currently compute activePaneId for t using sourceTab/layout, t.activePaneId and args.sourcePaneId or args.paneId and nextSourceLayout) with a single call to nextActivePaneIdAfterRemoval(originalLayout, t.activePaneId, removedPaneId, nextLayout) to centralize the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts`:
- Around line 88-102: The onError in useCloseWorkspace currently performs
rollback logic but does not call the consumer-supplied options?.onError; update
the onError handler in useCloseWorkspace (the handler that rolls back
utils.workspaces.getAllGrouped/setData, utils.workspaces.getAll.setData and
navigateToWorkspace) to invoke options?.onError(err, variables, context) (or
await it if it returns a promise) after performing rollback (or before, if you
prefer ordering consistent with useDeleteWorkspace) so callers receive their
error callback; ensure you reference the same variables (err, variables,
context) and preserve existing rollback/navigation behavior and the surrounding
mutation options composition.
---
Nitpick comments:
In `@packages/panes/src/core/store/store.ts`:
- Around line 686-699: Introduce a small file-local helper (e.g.,
nextActivePaneIdAfterRemoval) that wraps
getActiveIdAfterRemoval(getPaneIdsInLayout(originalLayout), activePaneId,
removedPaneId) ?? (nextLayout ? findFirstPaneId(nextLayout) : null) and replace
the duplicated 5-line expression in the closePane, movePaneToSplit,
movePaneToTab, and movePaneToNewTab call sites (where you currently compute
activePaneId for t using sourceTab/layout, t.activePaneId and args.sourcePaneId
or args.paneId and nextSourceLayout) with a single call to
nextActivePaneIdAfterRemoval(originalLayout, t.activePaneId, removedPaneId,
nextLayout) to centralize the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3af8d6a5-2d2e-461f-8dda-0d7e70d44fd7
📒 Files selected for processing (13)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.tspackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts
✅ Files skipped from review due to trivial changes (1)
- packages/panes/src/core/store/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.ts
- packages/panes/src/index.ts
- packages/panes/src/core/store/utils/utils.test.ts
- packages/panes/src/core/store/store.test.ts
- packages/panes/src/core/store/utils/utils.ts
Kitenite
force-pushed
the
fix-tab-close-selection
branch
from
3b2c4b8 to
12d2143
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx (1)
120-122:onSelectnow fires twice on left-click of the title.The parent
<div>'sonMouseDowncallsonSelect()on left button, and the inner title<button>still hasonClick={onSelect}(line 150), so a normal left-click on the title triggersonSelecton bothmousedownandclick. This is harmless ifonSelectis purely idempotent state-setting, but will double-fire any side effects (telemetry, focus moves, etc.). Consider dropping the inneronClick={onSelect}since the parent'smousedownalready covers the entire row (including icon/accessory areas) and fires earlier.♻️ Suggested cleanup
<TooltipTrigger asChild> <button className="flex h-full min-w-0 flex-1 items-center gap-1.5 pl-3 pr-1 text-left text-xs transition-colors" onAuxClick={(event) => { if (event.button === 1) { event.preventDefault(); onClose(); } }} - onClick={onSelect} onDoubleClick={startEditing} type="button" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx` around lines 120 - 122, The TabItem fires onSelect twice because the parent div's onMouseDown handler calls onSelect and the inner title button still has onClick={onSelect}; remove the inner button's onClick handler (or conditionally prevent propagation) so only the parent onMouseDown triggers selection; locate the title button element in TabItem (the element with onClick={onSelect}) and delete that prop (or replace it with onClick={(e) => e.stopPropagation()} if you need a no-op) so clicks only invoke TabItem's onMouseDown onSelect once.packages/panes/src/core/store/store.ts (1)
281-299: Consider extracting the repeatedgetActiveIdAfterRemoval(...) ?? findFirstPaneId(nextLayout)pattern.The same active-pane-after-removal computation is duplicated across
closePane,movePaneToSplit,movePaneToTab, andmovePaneToNewTab. A small helper would centralize the policy and make future tweaks (e.g., spatial fallback) one-line changes.♻️ Suggested helper
// in ./utils export function getNextActivePaneId( prevLayout: LayoutNode, activePaneId: string | null | undefined, removedPaneId: string, nextLayout: LayoutNode, ): string | null { return ( getActiveIdAfterRemoval( getPaneIdsInLayout(prevLayout), activePaneId, removedPaneId, ) ?? findFirstPaneId(nextLayout) ); }Then each callsite becomes:
- activePaneId: - getActiveIdAfterRemoval( - getPaneIdsInLayout(tab.layout), - tab.activePaneId, - args.paneId, - ) ?? findFirstPaneId(nextLayout), + activePaneId: getNextActivePaneId( + tab.layout, + tab.activePaneId, + args.paneId, + nextLayout, + ),Also applies to: 693-697, 755-760, 811-816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 281 - 299, Extract the repeated pattern getActiveIdAfterRemoval(getPaneIdsInLayout(prevLayout), tab.activePaneId, removedId) ?? findFirstPaneId(nextLayout) into a single helper (e.g., getNextActivePaneId(prevLayout, activePaneId, removedPaneId, nextLayout)) placed in a utils module, and update all callsites (closePane, movePaneToSplit, movePaneToTab, movePaneToNewTab — and the other occurrences around the shown indices) to call this helper instead of duplicating the expression so the active-pane-after-removal policy is centralized and easy to change.apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts (1)
88-102: Forward to user-providedoptions.onErrorfor consistency withonSuccess.
onSuccessalready chains the consumer's callback viaoptions?.onSuccess?.(...)(line 110), but the modifiedonErrorhere silently dropsoptions?.onError. Consumers passing anonErrortouseCloseWorkspace(...)will not see it fire, which is surprising given the spread-and-override pattern at line 37.♻️ Proposed fix
- onError: (_err, variables, context) => { + onError: async (err, variables, context, ...rest) => { // Rollback to previous state on error if (context?.previousGrouped !== undefined) { utils.workspaces.getAllGrouped.setData( undefined, context.previousGrouped, ); } if (context?.previousAll !== undefined) { utils.workspaces.getAll.setData(undefined, context.previousAll); } if (context?.wasViewingClosed) { navigateToWorkspace(variables.id, navigate); } + await options?.onError?.(err, variables, context, ...rest); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts` around lines 88 - 102, The onError handler in useCloseWorkspace performs rollback and navigation but does not forward to the consumer's options.onError like onSuccess does; after performing the existing rollback steps (restoring utils.workspaces.getAllGrouped and getAll and calling navigateToWorkspace when context.wasViewingClosed) invoke options?.onError with the same signature (pass the error, variables, and context) so user-provided error callbacks are called; locate the onError block inside useCloseWorkspace and add the call to options?.onError(err, variables, context) at the end of that block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.ts`:
- Around line 88-102: The onError handler in useCloseWorkspace performs rollback
and navigation but does not forward to the consumer's options.onError like
onSuccess does; after performing the existing rollback steps (restoring
utils.workspaces.getAllGrouped and getAll and calling navigateToWorkspace when
context.wasViewingClosed) invoke options?.onError with the same signature (pass
the error, variables, and context) so user-provided error callbacks are called;
locate the onError block inside useCloseWorkspace and add the call to
options?.onError(err, variables, context) at the end of that block.
In `@packages/panes/src/core/store/store.ts`:
- Around line 281-299: Extract the repeated pattern
getActiveIdAfterRemoval(getPaneIdsInLayout(prevLayout), tab.activePaneId,
removedId) ?? findFirstPaneId(nextLayout) into a single helper (e.g.,
getNextActivePaneId(prevLayout, activePaneId, removedPaneId, nextLayout)) placed
in a utils module, and update all callsites (closePane, movePaneToSplit,
movePaneToTab, movePaneToNewTab — and the other occurrences around the shown
indices) to call this helper instead of duplicating the expression so the
active-pane-after-removal policy is centralized and easy to change.
In
`@packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx`:
- Around line 120-122: The TabItem fires onSelect twice because the parent div's
onMouseDown handler calls onSelect and the inner title button still has
onClick={onSelect}; remove the inner button's onClick handler (or conditionally
prevent propagation) so only the parent onMouseDown triggers selection; locate
the title button element in TabItem (the element with onClick={onSelect}) and
delete that prop (or replace it with onClick={(e) => e.stopPropagation()} if you
need a no-op) so clicks only invoke TabItem's onMouseDown onSelect once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66234512-8757-4b4c-9f83-d647962819eb
📒 Files selected for processing (14)
apps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.tsapps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.tspackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.tspackages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId/getDeleteFocusTargetWorkspaceId.ts
✅ Files skipped from review due to trivial changes (3)
- packages/panes/src/core/store/utils/index.ts
- apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.ts
- packages/panes/src/core/store/store.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.ts
- packages/panes/src/core/store/utils/utils.test.ts
- apps/desktop/src/renderer/react-query/workspaces/utils/workspace-removal.test.ts
- apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
- packages/panes/src/core/store/utils/utils.ts
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useNavigateAwayFromWorkspace/useNavigateAwayFromWorkspace.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests