feat(sidebar): add workspace sections with drag & drop reordering#2067
Merged
Kitenite merged 17 commits intosuperset-sh:mainfrom Mar 7, 2026
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 workspace sections: DB migration and schema, TRPC section procedures and visual-order/reorder utils, React Query hooks and invalidation, sidebar components and DnD (sections + per-section reorder/move), a small Zustand drag-store, unit tests, and UI wiring for create/rename/delete/reorder/collapse/move operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ProjectSection
participant RQ as React Query Hook
participant TRPC as TRPC Client
participant Backend as TRPC Backend
participant DB as Database
User->>UI: Click "New Section"
UI->>RQ: createSection({ projectId, name })
RQ->>TRPC: electronTrpc.workspaces.createSection(...)
TRPC->>Backend: validate & compute tabOrder
Backend->>DB: INSERT workspace_sections
DB-->>Backend: created section
Backend-->>TRPC: return section
TRPC->>RQ: onSuccess -> invalidate getAllGrouped
RQ->>TRPC: refetch getAllGrouped
TRPC->>Backend: fetch workspaces & sections
Backend->>DB: SELECT workspaces, sections
DB-->>Backend: rows
Backend-->>TRPC: grouped payload
TRPC->>UI: updated grouped data
sequenceDiagram
participant User
participant UI as WorkspaceListItem
participant Store as ActiveDragStore
participant RQ as Reorder Hook
participant TRPC as TRPC Client
participant Backend as TRPC Backend
participant DB as Database
User->>UI: Start drag
UI->>Store: setActiveDragItem({id, sectionId, index})
User->>UI: Drop on target
UI->>RQ: call reorder (project or section)
RQ->>TRPC: electronTrpc.workspaces.reorderWorkspacesInSection(...) or reorderWorkspaces(...)
TRPC->>Backend: validate indices, compute new tabOrders
Backend->>DB: UPDATE workspaces.tab_order
DB-->>Backend: success
Backend-->>TRPC: success
TRPC->>RQ: onSuccess -> invalidate workspace queries
UI->>Store: clearActiveDragItem()
UI-->>User: render new order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 9
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/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx (1)
67-75:⚠️ Potential issue | 🟠 MajorGuard against undefined Electric URL when env validation is skipped.
With the new condition at Line 67, the code can reach Line 71 with unresolved/undefined env URLs and still call
setElectricUrl, which risks invalid runtime configuration.Suggested guard
if (useElectricCloud === undefined && !env.SKIP_ENV_VALIDATION) { return null; } + const electricUrl = useElectricCloud + ? env.NEXT_PUBLIC_ELECTRIC_URL + : env.NEXT_PUBLIC_ELECTRIC_PROXY_URL; + if (!electricUrl) return null; + - setElectricUrl( - useElectricCloud - ? env.NEXT_PUBLIC_ELECTRIC_URL - : env.NEXT_PUBLIC_ELECTRIC_PROXY_URL, - ); + setElectricUrl(electricUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx` around lines 67 - 75, The code may call setElectricUrl with undefined when env validation is skipped; update the logic in CollectionsProvider (where useElectricCloud is checked) to verify the chosen URL (env.NEXT_PUBLIC_ELECTRIC_URL or env.NEXT_PUBLIC_ELECTRIC_PROXY_URL) is defined before calling setElectricUrl—if the selected env value is undefined, either return null or use a safe fallback and log or surface an error; ensure this guard references useElectricCloud, setElectricUrl, and env.SKIP_ENV_VALIDATION so the component never sets an undefined electric URL.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx (1)
87-87: Avoid silent catch during workspace lookup.Line 87 swallows all errors, which makes close-flow navigation regressions hard to diagnose. A debug log keeps this best-effort behavior while preserving traceability.
Suggested tweak
- } catch {} + } catch (error) { + console.debug( + "[project-header] Failed to resolve current workspace before close", + error, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx` at line 87, The empty catch in the ProjectHeader component swallows errors during the workspace lookup; update the catch block in the workspace lookup logic inside ProjectHeader (the try/catch that currently ends with "catch {}") to log the caught error (e.g., using the existing debug/logging utility or processLogger) while preserving the current best-effort behavior (do not rethrow), so failures are traceable but the UI flow remains unchanged.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx (1)
179-199: Consider extracting shared workspace rendering to reduce branch drift.Both branches duplicate
WorkspaceList+sections.map(...)rendering with near-identical props.Also applies to: 255-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx` around lines 179 - 199, The WorkspaceList and sections.map(...) rendering is duplicated across branches; extract the shared JSX into a single reusable constant or small component and use it in both places. Move the block that renders <WorkspaceList ...> (passing workspaces, shortcutBaseIndex, sectionId=null, sections, isCollapsed=isSidebarCollapsed) and the sections.map(...) that returns <WorkspaceSection ...> (using section.id, projectId, sectionIndex, section.name, section.isCollapsed, section.workspaces, shortcutBaseIndex=sectionBaseIndices.get(section.id) ?? 0, isSidebarCollapsed, allSections=sections) into a helper (e.g., renderWorkspaces or WorkspaceSectionsBlock) and replace the duplicated branches with a single invocation to that helper so props like sectionBaseIndices and isSidebarCollapsed are consistently reused.apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts (1)
12-17: Parallelize independent invalidations inonSuccess.These cache invalidations are independent; running them serially adds unnecessary delay.
♻️ Suggested change
onSuccess: async (...args) => { - await utils.workspaces.getAll.invalidate(); - await utils.workspaces.getAllGrouped.invalidate(); - await utils.workspaces.getPreviousWorkspace.invalidate(); - await utils.workspaces.getNextWorkspace.invalidate(); + await Promise.all([ + utils.workspaces.getAll.invalidate(), + utils.workspaces.getAllGrouped.invalidate(), + utils.workspaces.getPreviousWorkspace.invalidate(), + utils.workspaces.getNextWorkspace.invalidate(), + ]); await options?.onSuccess?.(...args); },🤖 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/useReorderWorkspacesInSection.ts` around lines 12 - 17, The onSuccess handler in useReorderWorkspacesInSection is running multiple independent cache invalidations serially (utils.workspaces.getAll.invalidate, getAllGrouped.invalidate, getPreviousWorkspace.invalidate, getNextWorkspace.invalidate), which slows completion; change it to run those invalidate() calls in parallel using Promise.all and await that Promise, then await options?.onSuccess(...args) afterwards so the external callback still runs after invalidations complete. Ensure you reference the onSuccess handler in useReorderWorkspacesInSection and the four invalidate methods when making this change.packages/local-db/drizzle/meta/0035_snapshot.json (1)
1172-1178: Add an index onworkspaces.section_idfor section-scale operations.
section_idis now a foreign key but lacks an index; section move/reorder/grouped queries can degrade into scans as workspace counts increase.Also applies to: 1180-1202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/drizzle/meta/0035_snapshot.json` around lines 1172 - 1178, The workspaces table is missing an index on section_id which will cause scans for section-scoped operations; add a non-unique index on the workspaces.section_id column (e.g., createIndex on table "workspaces" for ["section_id"]) and update the migration/schema snapshot (0035 snapshot entry for workspaces) so the new index appears in the JSON metadata and a corresponding CREATE INDEX or migration step is added to the SQL/migration code path that manages the schema.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
561-583: Avoid no-op “Move to Section” mutations.Current menu always allows selecting the current destination (current section or “No Section”), which triggers unnecessary mutations/invalidation.
Suggested refactor
<ContextMenuItem + disabled={sectionId === null} onSelect={() => moveToSection.mutate({ workspaceId: id, sectionId: null, }) } > No Section </ContextMenuItem> @@ {sections.map((section) => ( <ContextMenuItem key={section.id} + disabled={section.id === sectionId} onSelect={() => moveToSection.mutate({ workspaceId: id, sectionId: section.id, }) } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 561 - 583, Prevent no-op mutations by checking the current destination before calling moveToSection.mutate: inside the ContextMenuItem handlers for "No Section" and for each section (the map over sections), compare the target sectionId (null for No Section or section.id) to the current section identifier (e.g., currentSectionId or whatever variable holds the item's current section) and either disable the menu item or short-circuit the onSelect to do nothing when they match; update the ContextMenuItem props for the "No Section" entry and for each section entry (the items created in the sections.map) to avoid calling moveToSection.mutate if the selected destination equals the current one.
🤖 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/lib/trpc/routers/workspaces/procedures/sections.ts`:
- Around line 58-67: deleteSection currently deletes workspaceSections but
leaves workspaces.sectionId pointing to the removed id; update workspaces first
to clear/null the foreign key then delete the section. In the deleteSection
mutation (function deleteSection) run an update on workspaces where
eq(workspaces.sectionId, input.id) to set sectionId to null (or remove the
relation), await that operation (make the mutation async if needed), then
proceed to delete from workspaceSections using the existing localDb.delete(...)
call and return success; ensure you reference workspaceSections,
workspaces.sectionId, localDb and deleteSection when making the change.
- Around line 173-185: The mutation moveWorkspaceToSection currently sets
sectionId blindly; change it to first validate the requested section (lookup in
the sections table) and ensure it either is null (allowed) or exists and belongs
to the same project as the target workspace: fetch the workspace by workspaceId
from workspaces, fetch the section by sectionId from sections (if non-null),
verify section.projectId === workspace.projectId, and only then call
localDb.update(workspaces).set({ sectionId: input.sectionId
}).where(eq(workspaces.id, input.workspaceId)).run(); if validation fails, throw
or return a descriptive error (e.g., "Section not found" or "Section belongs to
a different project") instead of performing the update.
In `@apps/desktop/src/renderer/react-query/workspaces/useReorderSections.ts`:
- Around line 12-16: The onSuccess handler in useReorderSections.ts currently
invalidates getAllGrouped, getPreviousWorkspace, and getNextWorkspace but omits
invalidating the flat cache utils.workspaces.getAll; update the onSuccess
implementation to also await utils.workspaces.getAll.invalidate() before calling
options?.onSuccess so flat consumers are refreshed after a section reorder
(refer to the onSuccess block and utils.workspaces.getAll/getAllGrouped
identifiers).
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.ts`:
- Around line 38-51: The handleDrop path currently finalizes drop but doesn't
clear a pending auto-expand timer; inside the handleDrop callback (alongside
existing calls to getActiveDragItem, canAccept, moveToSection, setting
item.handled, dragEnterCount.current = 0 and setIsDragOver(false)) call
clearTimeout on the auto-expand timer ref (e.g. pendingAutoExpand.current or
autoExpandTimer) and set that ref to null to prevent it firing after drop;
ensure you reference the exact timer ref name used in this module and clear/null
it within handleDrop.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx`:
- Around line 179-199: The collapsed branch is missing the ungrouped drop
handlers, so add ungroupedDropZone.handlers to the container rendering the
ungrouped workspaces when isSidebarCollapsed is true (the same place the
expanded branch mounts them). Specifically, ensure the element that renders
<WorkspaceList ... /> in the collapsed branch (and/or the parent wrapper around
the mapped <WorkspaceSection /> list) spreads ungroupedDropZone.handlers so
drag-to-ungroup works; reference ungroupedDropZone.handlers, WorkspaceList and
WorkspaceSection to find and update the collapsed branch rendering logic.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 260-265: The reorder mutation is being triggered twice because
handleReorder is invoked in both the drop handler and the end handler; move the
mutation out of the drop handler and into the end handler so it runs only once.
Specifically, remove the handleReorder call from the drop handler and in the end
handler (where useActiveDragItemStore.getState().clearActiveDragItem() is
already called) call handleReorder(item) only when item is present,
!item.handled, and monitor.didDrop() is true (use monitor.didDrop() to ensure a
valid drop occurred); keep the cross-section logic in useSectionDropZone that
sets item.handled as-is.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx`:
- Around line 80-82: The section drop-zone currently allows drops across
projects because useSectionDropZone's canAccept only checks item.sectionId !==
sectionId; update the canAccept predicate used when calling useSectionDropZone
to also verify item.projectId === projectId so items from other projects are
rejected (look for useSectionDropZone call and the canAccept arrow that
references sectionId and projectId). Ensure the same project check is enforced
before invoking moveToSection if there is any later validation path.
- Around line 114-117: The drop flow currently calls commitSectionReorder twice:
once in the drop handler and again in the end handler; remove the call to
commitSectionReorder from the drop handler (the function body for the DnD drop
callback) and keep the single call in the end callback (the end: (item) => { ...
} handler) so the mutation is dispatched only once per drag operation; update
tests or comments if any reference both handlers to reflect that
commitSectionReorder is invoked solely from end.
In `@packages/local-db/drizzle/0035_add_workspace_sections.sql`:
- Line 12: The migration adds workspaces.section_id referencing
workspace_sections(id) but omits the ON DELETE SET NULL clause used in the
TypeScript schema; update the ALTER TABLE statement that creates the foreign key
for workspaces.section_id (reference to workspace_sections.id) so the FK is
created with ON DELETE SET NULL to match the TypeScript relation and ensure
deletes of workspace_sections set workspaces.section_id to NULL.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx`:
- Around line 67-75: The code may call setElectricUrl with undefined when env
validation is skipped; update the logic in CollectionsProvider (where
useElectricCloud is checked) to verify the chosen URL
(env.NEXT_PUBLIC_ELECTRIC_URL or env.NEXT_PUBLIC_ELECTRIC_PROXY_URL) is defined
before calling setElectricUrl—if the selected env value is undefined, either
return null or use a safe fallback and log or surface an error; ensure this
guard references useElectricCloud, setElectricUrl, and env.SKIP_ENV_VALIDATION
so the component never sets an undefined electric URL.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts`:
- Around line 12-17: The onSuccess handler in useReorderWorkspacesInSection is
running multiple independent cache invalidations serially
(utils.workspaces.getAll.invalidate, getAllGrouped.invalidate,
getPreviousWorkspace.invalidate, getNextWorkspace.invalidate), which slows
completion; change it to run those invalidate() calls in parallel using
Promise.all and await that Promise, then await options?.onSuccess(...args)
afterwards so the external callback still runs after invalidations complete.
Ensure you reference the onSuccess handler in useReorderWorkspacesInSection and
the four invalidate methods when making this change.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx`:
- Line 87: The empty catch in the ProjectHeader component swallows errors during
the workspace lookup; update the catch block in the workspace lookup logic
inside ProjectHeader (the try/catch that currently ends with "catch {}") to log
the caught error (e.g., using the existing debug/logging utility or
processLogger) while preserving the current best-effort behavior (do not
rethrow), so failures are traceable but the UI flow remains unchanged.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx`:
- Around line 179-199: The WorkspaceList and sections.map(...) rendering is
duplicated across branches; extract the shared JSX into a single reusable
constant or small component and use it in both places. Move the block that
renders <WorkspaceList ...> (passing workspaces, shortcutBaseIndex,
sectionId=null, sections, isCollapsed=isSidebarCollapsed) and the
sections.map(...) that returns <WorkspaceSection ...> (using section.id,
projectId, sectionIndex, section.name, section.isCollapsed, section.workspaces,
shortcutBaseIndex=sectionBaseIndices.get(section.id) ?? 0, isSidebarCollapsed,
allSections=sections) into a helper (e.g., renderWorkspaces or
WorkspaceSectionsBlock) and replace the duplicated branches with a single
invocation to that helper so props like sectionBaseIndices and
isSidebarCollapsed are consistently reused.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 561-583: Prevent no-op mutations by checking the current
destination before calling moveToSection.mutate: inside the ContextMenuItem
handlers for "No Section" and for each section (the map over sections), compare
the target sectionId (null for No Section or section.id) to the current section
identifier (e.g., currentSectionId or whatever variable holds the item's current
section) and either disable the menu item or short-circuit the onSelect to do
nothing when they match; update the ContextMenuItem props for the "No Section"
entry and for each section entry (the items created in the sections.map) to
avoid calling moveToSection.mutate if the selected destination equals the
current one.
In `@packages/local-db/drizzle/meta/0035_snapshot.json`:
- Around line 1172-1178: The workspaces table is missing an index on section_id
which will cause scans for section-scoped operations; add a non-unique index on
the workspaces.section_id column (e.g., createIndex on table "workspaces" for
["section_id"]) and update the migration/schema snapshot (0035 snapshot entry
for workspaces) so the new index appears in the JSON metadata and a
corresponding CREATE INDEX or migration step is added to the SQL/migration code
path that manages the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 714650a6-45ca-46de-8f98-6df0ff3a13c8
📒 Files selected for processing (27)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/renderer/react-query/workspaces/index.tsapps/desktop/src/renderer/react-query/workspaces/useMoveWorkspaceToSection.tsapps/desktop/src/renderer/react-query/workspaces/useReorderSections.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceList/WorkspaceList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceList/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/types.tsapps/desktop/src/renderer/stores/active-drag-item.tspackages/local-db/drizzle/0035_add_workspace_sections.sqlpackages/local-db/drizzle/meta/0035_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/relations.tspackages/local-db/src/schema/schema.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 27 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/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx:87">
P2: Avoid swallowing errors with an empty `catch {}`; log a warning with context so navigation issues can be diagnosed.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:260">
P1: Same-section drag/drop can trigger duplicate reorder mutations because `end` calls `handleReorder` even after a successful drop.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts:40">
P2: Workspaces with stale/unknown `sectionId` are excluded from sidebar visual order, which can break previous/next workspace navigation for those items.</violation>
</file>
<file name="apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts">
<violation number="1" location="apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts:3">
P2: This new hook duplicates existing workspace cache-invalidation business logic; extract the shared invalidation flow into a common helper/hook to prevent behavior drift across reorder mutations.
(Based on your team's feedback about avoiding duplicate business logic.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:81">
P1: Section drop acceptance is missing a same-project guard, so workspaces can be dropped into sections from other projects and persisted with inconsistent project/section linkage.</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:114">
P1: Section reorder is executed twice (in `drop` and again in drag `end`), which can apply `fromIndex`/`toIndex` twice and produce incorrect final ordering.</violation>
</file>
<file name="packages/local-db/src/schema/schema.ts">
<violation number="1" location="packages/local-db/src/schema/schema.ts:124">
P2: Add an index for `workspaces.sectionId` to avoid unnecessary full-table scans during section FK maintenance and section-based lookups.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx:30">
P2: Shortcut base indices now include sectioned workspaces, but keyboard navigation still flattens only ungrouped workspaces, so Cmd+N labels and actual targets can diverge.</violation>
</file>
<file name="packages/local-db/drizzle/0035_add_workspace_sections.sql">
<violation number="1" location="packages/local-db/drizzle/0035_add_workspace_sections.sql:12">
P1: Add `ON DELETE SET NULL` to the `workspaces.section_id` foreign key. Without it, deleting a section can fail due to FK `NO ACTION` instead of ungrouping attached workspaces.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.ts:49">
P2: Clear the pending auto-expand timeout in `handleDrop`; otherwise a collapsed section can auto-expand after the drop has already completed.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts (1)
153-168:⚠️ Potential issue | 🟠 MajorValidate that target section belongs to the same project as the workspace.
The mutation sets
sectionIdwithout verifying that the section exists or belongs to the workspace's project. This could create cross-project workspace-section relationships.🛡️ Proposed fix with validation
moveWorkspaceToSection: publicProcedure .input( z.object({ workspaceId: z.string(), sectionId: z.string().nullable(), }), ) .mutation(({ input }) => { + const workspace = localDb + .select() + .from(workspaces) + .where(eq(workspaces.id, input.workspaceId)) + .get(); + if (!workspace) { + throw new Error(`Workspace ${input.workspaceId} not found`); + } + + if (input.sectionId) { + const section = localDb + .select() + .from(workspaceSections) + .where(eq(workspaceSections.id, input.sectionId)) + .get(); + if (!section || section.projectId !== workspace.projectId) { + throw new Error("Section not found or belongs to a different project"); + } + } + localDb .update(workspaces) .set({ sectionId: input.sectionId }) .where(eq(workspaces.id, input.workspaceId)) .run(); return { success: true }; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts` around lines 153 - 168, In moveWorkspaceToSection validate the target section belongs to the same project as the workspace before updating: fetch the workspace by workspaces.id and if input.sectionId is non-null fetch the section by sections.id, confirm section.projectId === workspace.projectId, and if not found or mismatched throw an appropriate error; only then call localDb.update(workspaces).set({ sectionId: input.sectionId }).where(eq(workspaces.id, input.workspaceId)).run(); allow input.sectionId === null to clear the section without validation.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts (1)
36-58: Consider pre-grouping workspaces by sectionId for efficiency.The current implementation filters
projectWorkspacesmultiple times: once for ungrouped (Line 41-45) and once per section (Lines 52-56). For projects with many sections, this becomes O(workspaces × sections).♻️ Optional optimization using a Map
for (const project of activeProjects) { const projectWorkspaces = workspaces .filter((w) => w.projectId === project.id) .sort((a, b) => a.tabOrder - b.tabOrder); + // Group workspaces by sectionId once + const workspacesBySectionId = new Map<string | null, typeof projectWorkspaces>(); + for (const ws of projectWorkspaces) { + const key = ws.sectionId; + if (!workspacesBySectionId.has(key)) { + workspacesBySectionId.set(key, []); + } + workspacesBySectionId.get(key)!.push(ws); + } + - for (const ws of projectWorkspaces.filter( - (w) => w.sectionId === null, - )) { + for (const ws of workspacesBySectionId.get(null) ?? []) { orderedIds.push(ws.id); } const projectSections = sections .filter((s) => s.projectId === project.id) .sort((a, b) => a.tabOrder - b.tabOrder); for (const section of projectSections) { - for (const ws of projectWorkspaces.filter( - (w) => w.sectionId === section.id, - )) { + for (const ws of workspacesBySectionId.get(section.id) ?? []) { orderedIds.push(ws.id); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts` around lines 36 - 58, The loop repeatedly filters projectWorkspaces causing O(workspaces × sections) cost; fix visual ordering by pre-grouping workspaces by sectionId (including null) once per project into a Map keyed by sectionId, then push ids for map.get(null) first and iterate projectSections (from sections sorted by tabOrder) to push map.get(section.id) entries; update references to projectWorkspaces/projectSections in the function that builds orderedIds so filtering is removed and lookups use the Map for O(1) access.apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts (1)
69-98: Loop-based updates are acceptable for now, but consider batching for scale.Both
reorderSectionsandreorderWorkspacesInSectionupdate each item individually in a loop. This is noted as a known limitation in the PR description. For small counts this is fine, but a single batch update (via SQL CASE WHEN or transaction batching) would be more efficient at scale.Also applies to: 122-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts` around lines 69 - 98, The current implementations of reorderSections and reorderWorkspacesInSection perform per-item updates inside a loop (iterating over sections/workspaces and calling localDb.update(...) for each), which is inefficient at scale; replace the looped single-row updates with a batched update—either construct a single UPDATE using a CASE WHEN mapping each id to its new tabOrder for workspaceSections and the corresponding table (e.g., workspaceWorkspaces), or execute the updates inside a single transaction/batch call provided by localDb so all tabOrder changes are applied in one round-trip; update the logic inside the reorderSections and reorderWorkspacesInSection procedures to build and run that batch/CASE update instead of calling localDb.update per item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts`:
- Around line 153-168: In moveWorkspaceToSection validate the target section
belongs to the same project as the workspace before updating: fetch the
workspace by workspaces.id and if input.sectionId is non-null fetch the section
by sections.id, confirm section.projectId === workspace.projectId, and if not
found or mismatched throw an appropriate error; only then call
localDb.update(workspaces).set({ sectionId: input.sectionId
}).where(eq(workspaces.id, input.workspaceId)).run(); allow input.sectionId ===
null to clear the section without validation.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts`:
- Around line 69-98: The current implementations of reorderSections and
reorderWorkspacesInSection perform per-item updates inside a loop (iterating
over sections/workspaces and calling localDb.update(...) for each), which is
inefficient at scale; replace the looped single-row updates with a batched
update—either construct a single UPDATE using a CASE WHEN mapping each id to its
new tabOrder for workspaceSections and the corresponding table (e.g.,
workspaceWorkspaces), or execute the updates inside a single transaction/batch
call provided by localDb so all tabOrder changes are applied in one round-trip;
update the logic inside the reorderSections and reorderWorkspacesInSection
procedures to build and run that batch/CASE update instead of calling
localDb.update per item.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts`:
- Around line 36-58: The loop repeatedly filters projectWorkspaces causing
O(workspaces × sections) cost; fix visual ordering by pre-grouping workspaces by
sectionId (including null) once per project into a Map keyed by sectionId, then
push ids for map.get(null) first and iterate projectSections (from sections
sorted by tabOrder) to push map.get(section.id) entries; update references to
projectWorkspaces/projectSections in the function that builds orderedIds so
filtering is removed and lookups use the Map for O(1) access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f09d9a2c-808a-4674-bf8a-665e1c889971
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/reorder.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/reorder.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.tsapps/desktop/src/renderer/stores/active-drag-item.test.tsapps/desktop/test-setup.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/stores/active-drag-item.test.ts
Contributor
There was a problem hiding this comment.
7 issues found across 33 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/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsx:87">
P2: Avoid using an empty `catch {}` here; it silently hides failures in a navigation-critical async path. Log a warning (with context) at minimum so operational issues are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts">
<violation number="1" location="apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts:12">
P3: This onSuccess invalidation block duplicates the same workspace cache invalidations already used in useReorderWorkspaces. Consider extracting a shared helper/hook to avoid drift and keep cache invalidation logic consistent.
(Based on your team's feedback about avoiding duplicated business logic.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx:30">
P2: Shortcut indices now count sectioned workspaces, but the hotkey navigation list still excludes sectioned items. This makes the displayed ⌘1–9 labels inconsistent with what the shortcuts actually open when sections are present. Update the hotkey workspace list to include section workspaces in the same order as the UI.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:221">
P2: Optimistic reorder cache updates are never reverted on mutation failure, so a failed reorder can leave the sidebar in the wrong order. Add an onSettled invalidation (as before) to restore server state on errors.</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:260">
P1: `handleReorder` is now invoked in both `drop` and `end`, so a normal drop triggers two reorder mutations. Since the mutation uses from/to indices, the second call can reorder a different workspace after the first update. Guard the end handler with `monitor.didDrop()` (as before) to avoid the duplicate mutation.</violation>
</file>
<file name="packages/local-db/drizzle/0035_add_workspace_sections.sql">
<violation number="1" location="packages/local-db/drizzle/0035_add_workspace_sections.sql:12">
P1: `section_id` should use `ON DELETE SET NULL` so deleting a section doesn’t fail with a FK constraint and instead returns workspaces to the ungrouped list.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:114">
P2: The reorder mutation fires twice on a normal section drop because both `drop` and `end` call `commitSectionReorder`. Guard `end` with `monitor.didDrop()` (or remove the drop commit) to avoid duplicate reorder requests.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
JimmyDore
added a commit
to JimmyDore/superset
that referenced
this pull request
Mar 5, 2026
- Prevent duplicate reorder mutations via monitor.didDrop() guards - Add projectId guard on cross-section workspace drops - Null workspace sectionIds before deleting section (FK safety) - Log warning instead of empty catch in ProjectHeader - Add sectionId index on workspaces table - Include section workspaces in keyboard shortcut flattening - Clear auto-expand timer on section drop - Extract shared invalidateWorkspaceQueries helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts (1)
158-170:⚠️ Potential issue | 🟠 MajorValidate workspace/section integrity before updating
sectionId.
moveWorkspaceToSectioncurrently writes blindly. It should verify: workspace exists, section exists (if non-null), and section.projectId matches workspace.projectId before update.🛡️ Suggested fix
moveWorkspaceToSection: publicProcedure .input( z.object({ workspaceId: z.string(), sectionId: z.string().nullable(), }), ) .mutation(({ input }) => { + const workspace = localDb + .select() + .from(workspaces) + .where(eq(workspaces.id, input.workspaceId)) + .get(); + + if (!workspace) { + throw new Error(`Workspace ${input.workspaceId} not found`); + } + + if (input.sectionId) { + const section = localDb + .select() + .from(workspaceSections) + .where(eq(workspaceSections.id, input.sectionId)) + .get(); + + if (!section) { + throw new Error(`Section ${input.sectionId} not found`); + } + + if (section.projectId !== workspace.projectId) { + throw new Error("Section belongs to a different project"); + } + } + localDb .update(workspaces) .set({ sectionId: input.sectionId }) .where(eq(workspaces.id, input.workspaceId)) .run();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts` around lines 158 - 170, In moveWorkspaceToSection, don’t write blindly: first query localDb for the target workspace (use workspaces and the workspaceId input) and throw/return an error if missing; if input.sectionId is non-null, query sections for that id and error if missing; then verify the found section.projectId equals the workspace.projectId and error if it doesn’t match; only after these checks perform the localDb.update(workspaces).set({ sectionId: input.sectionId }).where(eq(workspaces.id, input.workspaceId)).run(); ensure you reference the same localDb, workspaces, and sections symbols so the checks and update operate against the same datastore.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/stores/active-drag-item.test.ts (1)
4-10: Consider typing the shared fixture to prevent contract drift.
testItemis currently inferred. Making it explicitly follow the store getter contract will catch schema drift at compile time.♻️ Suggested refactor
import { beforeEach, describe, expect, test } from "bun:test"; import { getActiveDragItem, useActiveDragItemStore } from "./active-drag-item"; +type ActiveDragItem = NonNullable<ReturnType<typeof getActiveDragItem>>; + -const testItem = { +const testItem: ActiveDragItem = { id: "ws-1", projectId: "p-1", sectionId: null, index: 0, originalIndex: 0, };As per coding guidelines, "Avoid
anytype unless necessary - prioritize type safety".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/active-drag-item.test.ts` around lines 4 - 10, The test fixture `testItem` should be explicitly typed to match the store's expected item shape to prevent contract drift; import the appropriate type (e.g., ActiveDragItem, DragItem, or the store getter's return type) from the store/module and declare the fixture as `const testItem: <ImportedType> = { id: "ws-1", projectId: "p-1", sectionId: null, index: 0, originalIndex: 0 };` so the compiler enforces the schema rather than relying on inference.apps/desktop/src/renderer/react-query/workspaces/invalidateWorkspaceQueries.ts (1)
6-9: Batch invalidations concurrently to reduce post-mutation latency.Current behavior is correct, but these invalidations can run in parallel.
♻️ Suggested refactor
export async function invalidateWorkspaceQueries(utils: Utils) { - await utils.workspaces.getAll.invalidate(); - await utils.workspaces.getAllGrouped.invalidate(); - await utils.workspaces.getPreviousWorkspace.invalidate(); - await utils.workspaces.getNextWorkspace.invalidate(); + await Promise.all([ + utils.workspaces.getAll.invalidate(), + utils.workspaces.getAllGrouped.invalidate(), + utils.workspaces.getPreviousWorkspace.invalidate(), + utils.workspaces.getNextWorkspace.invalidate(), + ]); }🤖 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/invalidateWorkspaceQueries.ts` around lines 6 - 9, The four invalidate calls (utils.workspaces.getAll.invalidate, utils.workspaces.getAllGrouped.invalidate, utils.workspaces.getPreviousWorkspace.invalidate, utils.workspaces.getNextWorkspace.invalidate) are awaited serially which increases post-mutation latency; run them concurrently by replacing the sequential awaits with a single await Promise.all([...]) (or return Promise.all([...]) if appropriate) of those invalidate promises so all invalidations execute in parallel.apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts (1)
94-100: Wrap reorder persistence in a single DB transaction.Both reorder mutations perform multi-row updates in loops; partial failures can leave inconsistent
tabOrderstates.♻️ Suggested pattern
- for (const section of sections) { - localDb - .update(workspaceSections) - .set({ tabOrder: section.tabOrder }) - .where(eq(workspaceSections.id, section.id)) - .run(); - } + localDb.transaction(() => { + for (const section of sections) { + localDb + .update(workspaceSections) + .set({ tabOrder: section.tabOrder }) + .where(eq(workspaceSections.id, section.id)) + .run(); + } + })();Apply the same pattern to
reorderWorkspacesInSection.Also applies to: 147-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts` around lines 94 - 100, The multi-row updates in the loops that set tabOrder (currently using localDb.update(workspaceSections).set(...).where(...).run() per section) must be executed inside a single DB transaction to avoid partial failures; update the code in the procedure(s) that perform these loops (the reorder function that iterates over sections and the reorderWorkspacesInSection function referenced in the comment) to start a transaction (e.g., localDb.transaction or the project’s transaction API), perform all update calls against the transactional object (not the top-level localDb), and commit/rollback together so either all tabOrder updates succeed or none do. Ensure you replace the per-iteration localDb.update(...) calls with transactional updates and await the transaction completion.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
549-588: Avoid no-op “Move to Section” mutations for current section.Selecting the current section (or “No Section” when already unsectioned) still fires a mutation. Consider disabling or skipping these options to reduce unnecessary writes/invalidation.
♻️ Optional refactor
<ContextMenuItem + disabled={sectionId === null} onSelect={() => + sectionId === null + ? undefined + : moveToSection.mutate({ workspaceId: id, sectionId: null, }) } > No Section </ContextMenuItem> @@ {sections.map((section) => ( <ContextMenuItem key={section.id} + disabled={section.id === sectionId} onSelect={() => + section.id === sectionId + ? undefined + : moveToSection.mutate({ workspaceId: id, sectionId: section.id, }) } > {section.name} </ContextMenuItem> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 549 - 588, The context menu always calls moveToSection.mutate even when selecting the workspace's current section (or "No Section"), causing unnecessary mutations; update the onSelect handlers for the "No Section" ContextMenuItem and the mapped ContextMenuItem (inside sections.map) to first check current section equality (compare workspace id/section id via id and section.id or null) and only call moveToSection.mutate when the target section differs, and optionally disable the menu item by setting a disabled prop when the target equals the current section to prevent no-op actions.
🤖 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/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx`:
- Around line 100-103: The mutation currently only toasts on error and must
instead rollback the optimistic reorder: in the mutation defined in
WorkspaceSection.tsx (the one with onError: toast and the optimistic update done
in hover/onMutate), capture and return the previous sections list from onMutate
(e.g., return { previousSections }) and then in onError use that context to
restore the cache via queryClient.setQueryData (or equivalent) to
previousSections, then show the toast; also optionally call
queryClient.invalidateQueries for the sections query to ensure server state
sync.
- Around line 115-119: The end() handler currently calls
commitSectionReorder(item) when monitor.didDrop() is false, which persists an
optimistic reorder that should be reverted; change end() so that if
!monitor.didDrop() you do NOT call commitSectionReorder but instead invalidate
the sections cache to roll back the optimistic change (use the same
react-query/query client and query key used by your sections fetcher — e.g.,
call queryClient.invalidateQueries(<the sections query key> or the hook's query
key like useSectionsQuery key) ); keep hover() for optimistic updates and leave
drop() to call commitSectionReorder when the drop is accepted.
In `@packages/local-db/src/schema/schema.ts`:
- Around line 154-159: Add a unique constraint/index to protect the per-project
ordering by making (project_id, tab_order) unique in the table definition where
projectId and tabOrder are declared (the section schema that references
projects.id and defines tabOrder and projectId); update the table schema to add
a unique index/constraint on these two columns (and apply the same change to the
other identical section definition mentioned) so concurrent inserts cannot
create duplicate tab_order values for the same project.
- Around line 124-126: Add a DB-level guard ensuring workspaces.section_id
(non-null) belongs to the same project as workspaces.project_id by creating a
trigger function and trigger on the workspaces table: implement a BEFORE INSERT
OR UPDATE trigger (e.g., enforce_workspace_section_same_project) that, when
NEW.section_id is not null, SELECTs project_id FROM workspace_sections WHERE id
= NEW.section_id and compares it to NEW.project_id and raises an exception if
they differ; attach this trigger to the workspaces table (update the
schema/migration where sectionId is defined) so any insert/update violating the
same-project invariant is rejected at the DB level.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts`:
- Around line 158-170: In moveWorkspaceToSection, don’t write blindly: first
query localDb for the target workspace (use workspaces and the workspaceId
input) and throw/return an error if missing; if input.sectionId is non-null,
query sections for that id and error if missing; then verify the found
section.projectId equals the workspace.projectId and error if it doesn’t match;
only after these checks perform the localDb.update(workspaces).set({ sectionId:
input.sectionId }).where(eq(workspaces.id, input.workspaceId)).run(); ensure you
reference the same localDb, workspaces, and sections symbols so the checks and
update operate against the same datastore.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts`:
- Around line 94-100: The multi-row updates in the loops that set tabOrder
(currently using localDb.update(workspaceSections).set(...).where(...).run() per
section) must be executed inside a single DB transaction to avoid partial
failures; update the code in the procedure(s) that perform these loops (the
reorder function that iterates over sections and the reorderWorkspacesInSection
function referenced in the comment) to start a transaction (e.g.,
localDb.transaction or the project’s transaction API), perform all update calls
against the transactional object (not the top-level localDb), and
commit/rollback together so either all tabOrder updates succeed or none do.
Ensure you replace the per-iteration localDb.update(...) calls with
transactional updates and await the transaction completion.
In
`@apps/desktop/src/renderer/react-query/workspaces/invalidateWorkspaceQueries.ts`:
- Around line 6-9: The four invalidate calls
(utils.workspaces.getAll.invalidate, utils.workspaces.getAllGrouped.invalidate,
utils.workspaces.getPreviousWorkspace.invalidate,
utils.workspaces.getNextWorkspace.invalidate) are awaited serially which
increases post-mutation latency; run them concurrently by replacing the
sequential awaits with a single await Promise.all([...]) (or return
Promise.all([...]) if appropriate) of those invalidate promises so all
invalidations execute in parallel.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 549-588: The context menu always calls moveToSection.mutate even
when selecting the workspace's current section (or "No Section"), causing
unnecessary mutations; update the onSelect handlers for the "No Section"
ContextMenuItem and the mapped ContextMenuItem (inside sections.map) to first
check current section equality (compare workspace id/section id via id and
section.id or null) and only call moveToSection.mutate when the target section
differs, and optionally disable the menu item by setting a disabled prop when
the target equals the current section to prevent no-op actions.
In `@apps/desktop/src/renderer/stores/active-drag-item.test.ts`:
- Around line 4-10: The test fixture `testItem` should be explicitly typed to
match the store's expected item shape to prevent contract drift; import the
appropriate type (e.g., ActiveDragItem, DragItem, or the store getter's return
type) from the store/module and declare the fixture as `const testItem:
<ImportedType> = { id: "ws-1", projectId: "p-1", sectionId: null, index: 0,
originalIndex: 0 };` so the compiler enforces the schema rather than relying on
inference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d381f589-ab16-4373-86eb-93892d1d65a1
📒 Files selected for processing (18)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.tsapps/desktop/src/renderer/hooks/useWorkspaceShortcuts.tsapps/desktop/src/renderer/react-query/workspaces/invalidateWorkspaceQueries.tsapps/desktop/src/renderer/react-query/workspaces/useMoveWorkspaceToSection.tsapps/desktop/src/renderer/react-query/workspaces/useReorderSections.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.tsapps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectHeader.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.tsapps/desktop/src/renderer/stores/active-drag-item.test.tspackages/local-db/drizzle/0036_add_workspaces_section_id_idx.sqlpackages/local-db/drizzle/meta/0036_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
✅ Files skipped from review due to trivial changes (1)
- packages/local-db/drizzle/meta/0036_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/local-db/drizzle/meta/_journal.json
- apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspacesInSection.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/hooks/useSectionDropZone.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.test.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts
Contributor
There was a problem hiding this comment.
1 issue found across 37 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="packages/local-db/drizzle/0035_add_workspace_sections.sql">
<violation number="1" location="packages/local-db/drizzle/0035_add_workspace_sections.sql:12">
P2: The new workspaces.section_id FK is missing `ON DELETE SET NULL`, so deleting a section can fail or leave orphaned references. Add the delete action so workspaces revert to ungrouped as intended.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx (1)
117-121:⚠️ Potential issue | 🟠 MajorDo not persist reorder on canceled drag.
At Line 119-Line 120,
end()commits reorder whenmonitor.didDrop()is false, which can persist a hover-only optimistic order after a canceled drop. Revert cache instead; keep persistence indrop()only.Suggested fix
end: (item, monitor) => { if (!item) return; - if (monitor.didDrop()) return; - commitSectionReorder(item); + if (!monitor.didDrop()) { + void utils.workspaces.getAllGrouped.invalidate(); + } },react-dnd 16 useDrag end callback monitor.didDrop behavior canceled drop versus accepted drop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx` around lines 117 - 121, The end() drag handler currently calls commitSectionReorder(item) when monitor.didDrop() is false, which persists an optimistic hover reorder after a canceled drop; change end() to only call commitSectionReorder when monitor.didDrop() is true and otherwise revert the optimistic cache/state instead of persisting (i.e., undo the hover reorder via the existing cache reset or revert function used elsewhere), and ensure the actual persistence remains in the drop() handler so commits happen only on accepted drops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx`:
- Around line 117-121: The end() drag handler currently calls
commitSectionReorder(item) when monitor.didDrop() is false, which persists an
optimistic hover reorder after a canceled drop; change end() to only call
commitSectionReorder when monitor.didDrop() is true and otherwise revert the
optimistic cache/state instead of persisting (i.e., undo the hover reorder via
the existing cache reset or revert function used elsewhere), and ensure the
actual persistence remains in the drop() handler so commits happen only on
accepted drops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99991222-c8a4-4015-b969-1d96369f23eb
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsxpackages/local-db/drizzle/0035_add_workspace_sections.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.ts
Contributor
There was a problem hiding this comment.
2 issues found across 37 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/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:219">
P2: Double-click-to-rename is coupled with click-to-toggle on the same button, so rename attempts also send collapse toggle mutations.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts:51">
P2: `computeVisualOrder` can silently omit workspaces when `sectionId` is non-null but unresolved within the project, causing previous/next workspace navigation to skip those items. Add the same orphan fallback used in `getAllGrouped` (treat unresolved sections as ungrouped).</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx (1)
117-121:⚠️ Potential issue | 🟠 MajorDo not persist reorder when drag ends without a valid drop.
Line 120 commits reorder when
monitor.didDrop()is false, which represents a canceled/unhandled drop in react-dnd. That can persist hover-only optimistic state. Revert cache on canceled drag and keep commit only in accepted drop flow.Proposed fix
end: (item, monitor) => { if (!item) return; - if (monitor.didDrop()) return; - commitSectionReorder(item); + if (!monitor.didDrop()) { + void utils.workspaces.getAllGrouped.invalidate(); + } }, @@ drop: (item: SectionDragItem) => { + if (item.projectId !== projectId) return; commitSectionReorder(item); if (item.originalIndex !== item.index) return { reordered: true }; },In react-dnd v16, does source end() always fire, and does monitor.didDrop() return false when no compatible drop target handled the drop?Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx` around lines 117 - 121, The end handler currently calls commitSectionReorder even when monitor.didDrop() is false, which persists optimistic hover-only state; update the end: (item, monitor) => { ... } handler to only call commitSectionReorder when monitor.didDrop() === true and, when monitor.didDrop() === false, revert the optimistic reorder (e.g., restore the previous sections order via the same state-reset/revert helper used for optimistic updates or add a new revertSectionOrder function) so canceled/unhandled drops do not persist; apply the same change to the other drag end handler that mirrors this logic (the similar end handler at the later block).
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts (1)
54-57: Consider replacing repeated per-section filtering with a pre-grouped map.This loop rescans
projectWorkspacesfor every section. Pre-grouping once keeps behavior identical and reduces repeated allocations/scans.♻️ Suggested refactor
const sectionIds = new Set(projectSections.map((s) => s.id)); + const workspacesBySection = new Map<string | null, WorkspaceLike[]>(); - // Ungrouped workspaces: null sectionId OR orphaned (sectionId not in project) - for (const ws of projectWorkspaces.filter( - (w) => w.sectionId === null || !sectionIds.has(w.sectionId), - )) { - orderedIds.push(ws.id); - } + for (const ws of projectWorkspaces) { + const key = + ws.sectionId !== null && sectionIds.has(ws.sectionId) + ? ws.sectionId + : null; + const bucket = workspacesBySection.get(key); + if (bucket) { + bucket.push(ws); + } else { + workspacesBySection.set(key, [ws]); + } + } + + // Ungrouped workspaces: null sectionId OR orphaned (sectionId not in project) + for (const ws of workspacesBySection.get(null) ?? []) { + orderedIds.push(ws.id); + } for (const section of projectSections) { - for (const ws of projectWorkspaces.filter( - (w) => w.sectionId === section.id, - )) { + for (const ws of workspacesBySection.get(section.id) ?? []) { orderedIds.push(ws.id); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts` around lines 54 - 57, The loop over projectSections repeatedly filters projectWorkspaces for each section causing redundant scans; pre-group projectWorkspaces once into a map keyed by sectionId (e.g., build groupedWorkspacesBySectionId from projectWorkspaces) and then iterate projectSections and lookup workspaces via groupedWorkspacesBySectionId.get(section.id) (falling back to an empty array) to preserve behavior while avoiding repeated allocations and filtering in the nested for-loops that reference projectSections and projectWorkspaces.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx (1)
11-11: Clear the click timer on unmount.The pending timeout can still fire after unmount and dispatch
toggleCollapsed.mutate. Add cleanup to clearclickTimer.Proposed fix
-import { useCallback, useRef, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; @@ const clickTimer = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (clickTimer.current) { + clearTimeout(clickTimer.current); + clickTimer.current = null; + } + }; + }, []);Also applies to: 149-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx` at line 11, The component registers a pending timeout via clickTimer that can fire after unmount and call toggleCollapsed.mutate; add cleanup to clear that timer on unmount by using a useEffect cleanup that calls clearTimeout on the ref (e.g., clearTimeout(clickTimer.current)); also ensure you clear any existing timer before setting a new one in the click handler to avoid duplicates. Apply the same cleanup/fix for the other instance mentioned around lines 149-157 so both clickTimer usages are cleared on unmount and before resetting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx`:
- Around line 117-121: The end handler currently calls commitSectionReorder even
when monitor.didDrop() is false, which persists optimistic hover-only state;
update the end: (item, monitor) => { ... } handler to only call
commitSectionReorder when monitor.didDrop() === true and, when monitor.didDrop()
=== false, revert the optimistic reorder (e.g., restore the previous sections
order via the same state-reset/revert helper used for optimistic updates or add
a new revertSectionOrder function) so canceled/unhandled drops do not persist;
apply the same change to the other drag end handler that mirrors this logic (the
similar end handler at the later block).
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.ts`:
- Around line 54-57: The loop over projectSections repeatedly filters
projectWorkspaces for each section causing redundant scans; pre-group
projectWorkspaces once into a map keyed by sectionId (e.g., build
groupedWorkspacesBySectionId from projectWorkspaces) and then iterate
projectSections and lookup workspaces via
groupedWorkspacesBySectionId.get(section.id) (falling back to an empty array) to
preserve behavior while avoiding repeated allocations and filtering in the
nested for-loops that reference projectSections and projectWorkspaces.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx`:
- Line 11: The component registers a pending timeout via clickTimer that can
fire after unmount and call toggleCollapsed.mutate; add cleanup to clear that
timer on unmount by using a useEffect cleanup that calls clearTimeout on the ref
(e.g., clearTimeout(clickTimer.current)); also ensure you clear any existing
timer before setting a new one in the click handler to avoid duplicates. Apply
the same cleanup/fix for the other instance mentioned around lines 149-157 so
both clickTimer usages are cleared on unmount and before resetting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d7b2dc8-4cdf-4739-95ef-55897163fe46
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/visual-order.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx
Add the ability to organize workspaces into custom sections within the sidebar. Users can create, rename, and delete sections, drag workspaces between sections, reorder workspaces within a section, and reorder sections themselves. Changes: - Add workspace_sections table and sectionId column to local DB schema - Add tRPC procedures for workspace sections CRUD operations - Include sections in getAllGrouped query response - Add WorkspaceSection, WorkspaceList UI components - Add 'Move to Section' context menu on workspace items - Add drag & drop support for workspaces between sections - Add intra-section workspace reordering and section DnD - Extract useSectionDropZone hook and active-drag-item store
Extract pure utility functions (reorderItems, computeNextTabOrder, computeVisualOrder) from tRPC procedures and test them directly. Add Zustand store tests for active-drag-item. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Prevent duplicate reorder mutations via monitor.didDrop() guards - Add projectId guard on cross-section workspace drops - Null workspace sectionIds before deleting section (FK safety) - Log warning instead of empty catch in ProjectHeader - Add sectionId index on workspaces table - Include section workspaces in keyboard shortcut flattening - Clear auto-expand timer on section drop - Extract shared invalidateWorkspaceQueries helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…order, fix migration FK - Add workspace/section existence and cross-project validation to moveWorkspaceToSection - Invalidate getAllGrouped on section reorder error to rollback stale optimistic UI - Add ON DELETE SET NULL to section_id FK in migration SQL to match Drizzle schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ectionIds - Debounce single click on section header so double-click-to-rename doesn't also fire the collapse toggle - Treat workspaces with orphaned sectionId (pointing to non-existent section) as ungrouped in computeVisualOrder to prevent them from being silently omitted Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions Cmd+click toggles individual workspace selection, Shift+click selects a contiguous range. Dragging a selected group onto a section moves all selected workspaces. Right-click "Move to Section" also bulk-moves when multiple are selected. - Zustand selection store (workspace-selection.ts) - Bulk moveWorkspacesToSection tRPC procedure + React Query hook - MultiDragPreview floating badge during multi-drag - Selection visual highlight and opacity on drag - Escape / click-empty-space clears selection - Snapshot selection on context menu open for reliable bulk moves Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…op handler The native HTML5 drop zone wasn't reliably firing when dropping onto a workspace item in a different section. Now the react-dnd useDrop handler directly handles cross-section moves (both single and multi-select), with an item.handled guard to prevent double-processing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Batch DB writes with inArray instead of N+1 loop, optimize MultiDragPreview collector to skip work when not dragging, remove dead projectId prop from WorkspaceList, use store actions instead of raw setState, guard Escape key handler against input elements, and remove unused getSelectedWorkspaceIds export. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
JimmyDore
force-pushed
the
jimmy-dore/featuresidebar-ordering
branch
from
d994b82 to
9defc7c
Compare
Contributor
Author
|
@Kitenite PR ready to review
|
Contributor
There was a problem hiding this comment.
2 issues found across 14 files (changes from recent commits).
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/useCreateSectionFromWorkspaces.ts">
<violation number="1" location="apps/desktop/src/renderer/react-query/workspaces/useCreateSectionFromWorkspaces.ts:29">
P2: If `moveWorkspaces` fails after the section is successfully created, `invalidateWorkspaceQueries` is skipped, leaving the UI out of sync with the database. Move the invalidation call to a `finally` block so it runs regardless of whether the second mutation succeeds.</violation>
</file>
<file name="packages/local-db/drizzle/0035_add_workspace_sections.sql">
<violation number="1" location="packages/local-db/drizzle/0035_add_workspace_sections.sql:14">
P1: Missing `ON DELETE SET NULL` on the `section_id` foreign key. The Drizzle schema specifies `onDelete: "set null"` so that deleting a section returns its workspaces to the ungrouped state, but this migration drops that clause. With the default `NO ACTION`, deleting a section will either fail (if `PRAGMA foreign_keys` is on) or leave dangling `section_id` references (if it's off).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| --> statement-breakpoint | ||
| CREATE INDEX `workspace_sections_project_id_idx` ON `workspace_sections` (`project_id`);--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `section_id` text REFERENCES workspace_sections(id);--> statement-breakpoint | ||
| CREATE INDEX `workspaces_section_id_idx` ON `workspaces` (`section_id`); No newline at end of file |
Contributor
There was a problem hiding this comment.
P1: Missing ON DELETE SET NULL on the section_id foreign key. The Drizzle schema specifies onDelete: "set null" so that deleting a section returns its workspaces to the ungrouped state, but this migration drops that clause. With the default NO ACTION, deleting a section will either fail (if PRAGMA foreign_keys is on) or leave dangling section_id references (if it's off).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/local-db/drizzle/0035_add_workspace_sections.sql, line 14:
<comment>Missing `ON DELETE SET NULL` on the `section_id` foreign key. The Drizzle schema specifies `onDelete: "set null"` so that deleting a section returns its workspaces to the ungrouped state, but this migration drops that clause. With the default `NO ACTION`, deleting a section will either fail (if `PRAGMA foreign_keys` is on) or leave dangling `section_id` references (if it's off).</comment>
<file context>
@@ -4,9 +4,11 @@ CREATE TABLE `workspace_sections` (
-ALTER TABLE `workspaces` ADD `section_id` text REFERENCES workspace_sections(id) ON DELETE SET NULL;
\ No newline at end of file
+ALTER TABLE `workspaces` ADD `section_id` text REFERENCES workspace_sections(id);--> statement-breakpoint
+CREATE INDEX `workspaces_section_id_idx` ON `workspaces` (`section_id`);
\ No newline at end of file
</file context>
| workspaceIds, | ||
| sectionId: section.id, | ||
| }); | ||
| await invalidateWorkspaceQueries(utils); |
Contributor
There was a problem hiding this comment.
P2: If moveWorkspaces fails after the section is successfully created, invalidateWorkspaceQueries is skipped, leaving the UI out of sync with the database. Move the invalidation call to a finally block so it runs regardless of whether the second mutation succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/react-query/workspaces/useCreateSectionFromWorkspaces.ts, line 29:
<comment>If `moveWorkspaces` fails after the section is successfully created, `invalidateWorkspaceQueries` is skipped, leaving the UI out of sync with the database. Move the invalidation call to a `finally` block so it runs regardless of whether the second mutation succeeds.</comment>
<file context>
@@ -0,0 +1,38 @@
+ workspaceIds,
+ sectionId: section.id,
+ });
+ await invalidateWorkspaceQueries(utils);
+ } catch (error) {
+ toast.error(
</file context>
…on, typecheck fix - Add color column to workspace sections with random color on creation - Add colored dot and left border to section UI - Add color picker context menu for sections - Add "Create Section from Selection" on multi-select right-click - Fix pre-existing typecheck error in ChangesView OrderedSection union type - Consolidate migrations into single 0035_add_workspace_sections
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx:262">
P1: The `min-h-8` class is gated on `isDragOver`, but `isDragOver` requires the user to hover over the element — which is effectively impossible when the div has zero height (no workspaces). The original code always applied `minHeight: "2rem"` on empty, ensuring the drop target was always reachable. This change makes it impossible to drag a workspace back to the ungrouped area when it's empty.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
… show drop targets during drag - Extract useWorkspaceDnD hook from WorkspaceListItem (DnD setup, reorder, move-to-section on drop) - Extract WorkspaceContextMenu component (context menu items, hover card wrapping) - Extract useSectionMutations hook from WorkspaceSection (toggle/rename/delete/setColor) - Add onSettled invalidation to reorder callbacks to restore server state on failure - Move WORKSPACE_DND_TYPE to constants.ts - Show drop zone indicators when any compatible item is dragging (not just on hover) - useSectionDropZone now returns isDropTarget (any valid drag active) alongside isDragOver
Contributor
There was a problem hiding this comment.
4 issues found across 9 files (changes from recent commits).
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/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:142">
P2: `useCallback` with `[mutations]` dependency never memoizes because `useSectionMutations` returns a new object on every render (no `useMemo`). Either memoize the return value in `useSectionMutations` with `useMemo`, or depend on a stable reference like `mutations.toggle` directly.</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx:203">
P2: Switching from `ring-1` to `border` introduces layout shift — the 1px border appears/disappears during drag, pushing content inward. Use `ring-1` (box-shadow based, no layout effect) instead, or add a permanent `border border-transparent` to the base classes so only the color/style changes on drag.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/useWorkspaceDnD.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/useWorkspaceDnD.ts:106">
P2: `useDrop` should use the factory function + deps pattern (`useDrop(() => spec, deps)`) to match `useDrag` and avoid tearing down/re-registering the drop target on every render. This matters in a list where each item creates its own drop target.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx:267">
P2: Missing `border` (border-width) class when `isDragOver` is true. `border-solid` and `border-primary/30` have no visible effect without `border` because Tailwind defaults border-width to 0. The drag-over border indicator won't render.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| clickTimer.current = null; | ||
| mutations.toggle(); | ||
| }, 250); | ||
| }, [mutations]); |
Contributor
There was a problem hiding this comment.
P2: useCallback with [mutations] dependency never memoizes because useSectionMutations returns a new object on every render (no useMemo). Either memoize the return value in useSectionMutations with useMemo, or depend on a stable reference like mutations.toggle directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx, line 142:
<comment>`useCallback` with `[mutations]` dependency never memoizes because `useSectionMutations` returns a new object on every render (no `useMemo`). Either memoize the return value in `useSectionMutations` with `useMemo`, or depend on a stable reference like `mutations.toggle` directly.</comment>
<file context>
@@ -181,9 +137,9 @@ export function WorkspaceSection({
+ mutations.toggle();
}, 250);
- }, [sectionId, toggleCollapsed]);
+ }, [mutations]);
const handleDoubleClick = useCallback(() => {
</file context>
| className={cn( | ||
| "flex items-center w-full pl-2 pr-2 py-2 text-[11px] font-medium uppercase tracking-wider", | ||
| "text-muted-foreground hover:bg-muted/50 transition-colors", | ||
| dropZone.isDropTarget && |
Contributor
There was a problem hiding this comment.
P2: Switching from ring-1 to border introduces layout shift — the 1px border appears/disappears during drag, pushing content inward. Use ring-1 (box-shadow based, no layout effect) instead, or add a permanent border border-transparent to the base classes so only the color/style changes on drag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSection/WorkspaceSection.tsx, line 203:
<comment>Switching from `ring-1` to `border` introduces layout shift — the 1px border appears/disappears during drag, pushing content inward. Use `ring-1` (box-shadow based, no layout effect) instead, or add a permanent `border border-transparent` to the base classes so only the color/style changes on drag.</comment>
<file context>
@@ -248,7 +200,11 @@ export function WorkspaceSection({
"flex items-center w-full pl-2 pr-2 py-2 text-[11px] font-medium uppercase tracking-wider",
"text-muted-foreground hover:bg-muted/50 transition-colors",
- dropZone.isDragOver && "bg-primary/10 ring-1 ring-primary/40",
+ dropZone.isDropTarget &&
+ !dropZone.isDragOver &&
+ "border border-dashed border-primary/20 rounded-sm",
</file context>
| [id, projectId, sectionId, index, handleReorder], | ||
| ); | ||
|
|
||
| const [, drop] = useDrop({ |
Contributor
There was a problem hiding this comment.
P2: useDrop should use the factory function + deps pattern (useDrop(() => spec, deps)) to match useDrag and avoid tearing down/re-registering the drop target on every render. This matters in a list where each item creates its own drop target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/useWorkspaceDnD.ts, line 106:
<comment>`useDrop` should use the factory function + deps pattern (`useDrop(() => spec, deps)`) to match `useDrag` and avoid tearing down/re-registering the drop target on every render. This matters in a list where each item creates its own drop target.</comment>
<file context>
@@ -0,0 +1,162 @@
+ [id, projectId, sectionId, index, handleReorder],
+ );
+
+ const [, drop] = useDrop({
+ accept: WORKSPACE_DND_TYPE,
+ hover: (item: DragItem) => {
</file context>
Comment on lines
+267
to
+268
| ungroupedDropZone.isDragOver && | ||
| "bg-primary/5 border-solid border-primary/30", |
Contributor
There was a problem hiding this comment.
P2: Missing border (border-width) class when isDragOver is true. border-solid and border-primary/30 have no visible effect without border because Tailwind defaults border-width to 0. The drag-over border indicator won't render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/ProjectSection/ProjectSection.tsx, line 267:
<comment>Missing `border` (border-width) class when `isDragOver` is true. `border-solid` and `border-primary/30` have no visible effect without `border` because Tailwind defaults border-width to 0. The drag-over border indicator won't render.</comment>
<file context>
@@ -256,12 +256,16 @@ export function ProjectSection({
+ ungroupedDropZone.isDragOver) &&
workspaces.length === 0 &&
"min-h-8",
+ ungroupedDropZone.isDragOver &&
+ "bg-primary/5 border-solid border-primary/30",
)}
</file context>
Suggested change
| ungroupedDropZone.isDragOver && | |
| "bg-primary/5 border-solid border-primary/30", | |
| ungroupedDropZone.isDragOver && | |
| "bg-primary/5 border border-solid border-primary/30", |
This was referenced Mar 16, 2026
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
2 participants
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.
Demo
Sections
https://www.youtube.com/watch?v=qzNTkqexMSw
Multi-Select & Batch Move
https://www.youtube.com/shorts/HZz8ifI8mJc
Summary
workspace_sectionstable in local SQLite DB with Drizzle migrationWhy / Context
As the number of workspaces per project grows, the flat list becomes hard to manage. Sections let users group related workspaces (e.g., by feature, sprint, or status) with collapsible headers and drag-and-drop reordering.
How It Works
Data layer:
workspace_sectionstable (id,projectId,name,tabOrder,isCollapsed)workspaces.sectionIdFK (nullable,onDelete: set null) — ungrouped workspaces remain at the topmoveWorkspacesToSectionbatch mutation for moving multiple workspaces at once (singleinArrayUPDATE)UI layer:
WorkspaceSectioncomponent: collapsible header with context menu (rename, delete), section-level DnD viareact-dndWorkspaceListextracted as shared list renderer for both ungrouped and sectioned workspacesuseSectionDropZonehook: native drag event handling for cross-section workspace moves with 600ms auto-expand on collapsed sectionsactive-drag-itemZustand store: bridgesreact-dnddrag state with native drop zone handlersworkspace-selectionZustand store: tracks multi-select state (Cmd+click to toggle, Shift+click to range-select)MultiDragPreviewcustom drag layer showing "{N} workspaces" badge during multi-draggetAllGroupedquery now returns sections nested under each projectManual QA Checklist
Section CRUD
Multi-Select
Drag & Drop — Workspaces
Drag & Drop — Sections
Context Menu
Edge Cases
Testing
bun run typecheck— passes (pre-existing@ai-sdk/openaierror on upstream, unrelated)bun run lint:fix— passesKnown Limitations
tabOrdervalues in a loop (fine for typical section/workspace counts, not optimized for hundreds)Follow-ups