Add optimistic Electric collection updates#3722
Conversation
|
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:
📝 WalkthroughWalkthroughCentralizes optimistic Electric-backed mutations via a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant Hook as useOptimisticCollectionActions
participant TanStack as TanStack DB
participant API as API (tRPC)
participant PG as PostgreSQL
participant Electric as Electric Sync
UI->>Hook: call action (e.g. updateStatus(id, status))
activate Hook
Hook->>TanStack: collection.update() (optimistic overlay)
TanStack-->>Hook: transaction (includes isPersisted.promise)
Hook->>API: mutate(...) (send change)
activate API
API->>PG: perform mutation inside tx
API->>PG: SELECT pg_current_xact_id()
PG-->>API: txid
API-->>Hook: { result..., txid }
deactivate API
Electric->>TanStack: stream txid (confirm/rollback)
TanStack-->>Hook: persistence outcome
Hook-->>UI: resolve transaction / surface error toast if needed
deactivate Hook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR centralises all task and workspace optimistic mutations behind two domain action hooks ( Confidence Score: 5/5Safe to merge — no blocking issues; the one P2 finding is a defensive-coding suggestion on field spreading. All issues found are P2 style suggestions. The core pattern (shared mutation hook → collection update → onUpdate handler → tRPC with txid → Electric confirmation/rollback) is correctly implemented. The tRPC schema validates with Zod which strips unknown keys, so the changes spread concern is not a runtime bug today. apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts — the onUpdate handler spreads changes without explicit field filtering.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.ts | New shared hook that wraps collection mutations with synchronous-throw and async-rejection error handling, surfaces user-visible toasts, and returns the transaction (or null on sync failure). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/useTaskOptimisticActions.ts | New hook centralising all task mutations (title, description, status, priority, assignee, delete) through useOptimisticCollectionMutation; correctly memoised and typed. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/useV2WorkspaceOptimisticActions.ts | New hook providing renameWorkspace and updateWorkspace actions through the shared mutation helper; correctly guarded with undefined-checks before setting draft fields. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Adds onUpdate Electric handler for v2Workspaces; accesses mutations[0] (safe for single-mutation call sites) and spreads changes directly into the API call without explicit field filtering. |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | update procedure now wraps the UPDATE statement and getCurrentTxid in a single explicit transaction and returns txid alongside the updated row for Electric confirmation. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts | Replaces direct apiTrpcClient.v2Workspace.update call with workspaceActions.renameWorkspace; submitRename is now synchronous, which is correct since the optimistic mutation is fire-and-handle. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts | Test updated to assert use of useTaskOptimisticActions and taskActions.deleteTask instead of the old collections API; assertions match the new implementation. |
Sequence Diagram
sequenceDiagram
participant UI as UI Component
participant Hook as useTaskOptimisticActions /<br/>useV2WorkspaceOptimisticActions
participant Runner as useOptimisticCollectionMutation
participant Col as TanStack DB Collection
participant API as tRPC API
participant Elec as Electric (Postgres txid stream)
UI->>Hook: updateStatus / renameWorkspace / …
Hook->>Runner: runMutation(failureTitle, mutationFn)
Runner->>Col: collection.update(id, draft)
Col-->>Runner: PersistableTransaction (optimistic state applied)
Runner->>Runner: attach .catch() to isPersisted.promise
Runner-->>Hook: transaction
Hook-->>UI: transaction (non-null = optimistic applied)
UI->>UI: setOpen(false) — immediate feedback
Col->>API: onUpdate handler → v2Workspace.update / task handler
API->>API: BEGIN tx · UPDATE row · pg_current_xact_id()
API-->>Col: { txid }
Elec-->>Col: streams txid commit
Col->>Col: drops optimistic overlay (confirmed)
alt Persistence fails
API-->>Col: error
Col->>Runner: isPersisted.promise rejects
Runner->>UI: toast.error(failureTitle, description)
Col->>Col: rolls back optimistic state
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
Line: 339-343
Comment:
**Spreading `changes` blindly into the API call**
`changes` is spread directly into `v2Workspace.update.mutate` without explicitly selecting only the fields the endpoint expects (`name`, `branch`, `hostId`). While Zod strips unknown keys by default, any future field added to the workspace schema that also appears in `changes` would silently be passed to the API, potentially triggering unexpected validation paths (e.g., `hostId` triggers `getScopedHost`). Prefer an explicit pick over the spread.
```suggestion
const { name, branch, hostId } = changes as Partial<typeof original>;
const result = await apiClient.v2Workspace.update.mutate({
id: original.id,
...(name !== undefined && { name }),
...(branch !== undefined && { branch }),
...(hostId !== undefined && { hostId }),
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add optimistic Electric collection updat..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/trpc/src/router/v2-workspace/v2-workspace.ts (1)
174-192:NOT_FOUNDthrow runs after the transaction has already committed.The
return { updated, txid }is inside thedbWs.transaction(...)callback, so the transaction commits before theif (!updated)check on line 186. If a concurrent delete races betweengetWorkspaceAccessand theUPDATE, you'll have:
- Committed a no-op transaction (and consumed a real
pg_current_xact_id()).- Returned a
NOT_FOUNDto the caller.- Streamed an empty txid through Electric that no client
isPersisted.promiseis waiting on.In practice the upfront access check makes this a rare race, but moving the check inside the transaction (so it rolls back) avoids burning xids on no-op writes:
♻️ Optional: throw inside the transaction so it rolls back
- const result = await dbWs.transaction(async (tx) => { - const [updated] = await tx - .update(v2Workspaces) - .set(data) - .where(eq(v2Workspaces.id, workspace.id)) - .returning(); - - const txid = await getCurrentTxid(tx); - - return { updated, txid }; - }); - const { updated, txid } = result; - if (!updated) { - throw new TRPCError({ - code: "NOT_FOUND", - message: "Workspace not found", - }); - } + const { updated, txid } = await dbWs.transaction(async (tx) => { + const [row] = await tx + .update(v2Workspaces) + .set(data) + .where(eq(v2Workspaces.id, workspace.id)) + .returning(); + if (!row) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "Workspace not found", + }); + } + return { updated: row, txid: await getCurrentTxid(tx) }; + }); return { ...updated, txid };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts` around lines 174 - 192, The NOT_FOUND check occurs after dbWs.transaction(...) returns so the transaction commits even when no row was updated; move the if (!updated) throw new TRPCError({...}) inside the dbWs.transaction callback (before returning { updated, txid }) so the transaction will roll back on NOT_FOUND; ensure getCurrentTxid(tx) is only called after confirming updated and return { ...updated, txid } from inside the transaction so callers never receive a committed empty txid.apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.ts (1)
19-19: Fallback message conflates sync-throw and async-rollback cases.
getErrorMessagefalls back to"The local change was rolled back.", which fits theisPersisted.promiserejection branch but is misleading for the synchronouscatchon line 47 — whencollections.x.update(...)throws (e.g., key not in collection, schema validation), no optimistic state was ever applied so nothing was rolled back. Consider either a neutral fallback ("The change could not be applied.") or distinct messages per branch.Also applies to: 38-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.ts` at line 19, getErrorMessage currently returns a single fallback ("The local change was rolled back.") which wrongly describes both the sync throw from collections.x.update (no optimistic state applied) and the async isPersisted.promise rejection (rollback happened). Modify getErrorMessage to accept a boolean (e.g., wasRolledBack) or an enum and return distinct messages: a neutral failure message for sync failures ("The change could not be applied.") and the rollback message for async persistence failure; then update the synchronous catch around collections.x.update to call getErrorMessage(false) and the isPersisted.promise rejection handler to call getErrorMessage(true). Ensure callers of getErrorMessage (and any tests) are adjusted accordingly.apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts (1)
14-22: Consider loosening the substring assertions.The exact substring
"taskActions.deleteTask(task.id)"will break under benign formatting changes (e.g., a Biome wrap to multiple lines, or renamingtask→row.originalat the call site) despite identical runtime behavior. Consider matching with a regex like/taskActions\.deleteTask\s*\(/or, better, replacing source-string verification with a proper unit test ofTaskContextMenuthat mocksuseTaskOptimisticActions.♻️ Looser regex-based assertion
- expect(source).toContain("useTaskOptimisticActions"); - expect(source).toContain("taskActions.deleteTask(task.id)"); + expect(source).toContain("useTaskOptimisticActions"); + expect(source).toMatch(/taskActions\.deleteTask\s*\(/); expect(source).toContain("onSelect={handleDelete}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts` around lines 14 - 22, The test is brittle because it asserts an exact substring; update TasksTableView.test.ts to loosen the assertion by either replacing expect(source).toContain("taskActions.deleteTask(task.id)") with a regex-based check such as matching /taskActions\.deleteTask\s*\(/ or, preferably, convert this source-string check into a proper unit test of TaskContextMenu that mocks useTaskOptimisticActions and asserts the delete action is invoked (referencing TaskContextMenu, useTaskOptimisticActions, and taskActions.deleteTask) so formatting or local variable renames won't break the test.
🤖 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/docs/OPTIMISTIC_ELECTRIC_UPDATES.md`:
- Line 99: The phrase "multi-step server side effect" should use hyphens for
compound modifiers: change it to "multi-step server-side effect" (and verify
other compounds around "Use `{ optimistic: false }` only for exceptional flows
... such as a workflow that depends on a server-generated identifier or a
multi-step server-side effect" are consistently hyphenated).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts`:
- Around line 14-22: The test is brittle because it asserts an exact substring;
update TasksTableView.test.ts to loosen the assertion by either replacing
expect(source).toContain("taskActions.deleteTask(task.id)") with a regex-based
check such as matching /taskActions\.deleteTask\s*\(/ or, preferably, convert
this source-string check into a proper unit test of TaskContextMenu that mocks
useTaskOptimisticActions and asserts the delete action is invoked (referencing
TaskContextMenu, useTaskOptimisticActions, and taskActions.deleteTask) so
formatting or local variable renames won't break the test.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.ts`:
- Line 19: getErrorMessage currently returns a single fallback ("The local
change was rolled back.") which wrongly describes both the sync throw from
collections.x.update (no optimistic state applied) and the async
isPersisted.promise rejection (rollback happened). Modify getErrorMessage to
accept a boolean (e.g., wasRolledBack) or an enum and return distinct messages:
a neutral failure message for sync failures ("The change could not be applied.")
and the rollback message for async persistence failure; then update the
synchronous catch around collections.x.update to call getErrorMessage(false) and
the isPersisted.promise rejection handler to call getErrorMessage(true). Ensure
callers of getErrorMessage (and any tests) are adjusted accordingly.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 174-192: The NOT_FOUND check occurs after dbWs.transaction(...)
returns so the transaction commits even when no row was updated; move the if
(!updated) throw new TRPCError({...}) inside the dbWs.transaction callback
(before returning { updated, txid }) so the transaction will roll back on
NOT_FOUND; ensure getCurrentTxid(tx) is only called after confirming updated and
return { ...updated, txid } from inside the transaction so callers never receive
a committed empty txid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae8f5739-b9b2-40c3-ac89-b32059edd9d1
📒 Files selected for processing (21)
apps/desktop/docs/OPTIMISTIC_ELECTRIC_UPDATES.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/AssigneeProperty/AssigneeProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/PriorityProperty/PriorityProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/StatusProperty/StatusProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskActionMenu/TaskActionMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksBoardView/TasksBoardView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/components/TaskContextMenu/TaskContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/AssigneeCell/AssigneeCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/PriorityCell/PriorityCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/StatusCell/StatusCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/useTaskOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/useV2WorkspaceOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
1946d59 to
c44c6cc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/useV2ProjectOptimisticActions.ts (1)
43-49: OptimisticgithubRepositoryId = nullmay flicker for non-null URL updates.
updateRepositoryaccepts a non-nullrepoCloneUrlbut always setsdraft.githubRepositoryId = nulloptimistically. The server (v2ProjectRouter.update) re-resolves the repo link from the new URL whengithubRepositoryIdis not provided, so Electric will eventually stream back the correct id — but in the interim the UI shows the project as unlinked. If any UI reacts togithubRepositoryId(e.g., "unlinked" badge, repo metadata pane), this can produce a brief flicker. Consider only nulling whenrepoCloneUrl === null.♻️ Suggested adjustment
updateRepository: (projectId: string, repoCloneUrl: string | null) => runMutation("Failed to update project repository", () => collections.v2Projects.update(projectId, (draft) => { draft.repoCloneUrl = repoCloneUrl; - draft.githubRepositoryId = null; + if (repoCloneUrl === null) { + draft.githubRepositoryId = null; + } }), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/useV2ProjectOptimisticActions.ts` around lines 43 - 49, The optimistic update in updateRepository currently always sets draft.githubRepositoryId = null, causing a flicker when a non-null repoCloneUrl is provided; change the mutation in updateRepository (the collections.v2Projects.update callback) to only set draft.githubRepositoryId = null when repoCloneUrl === null, otherwise leave draft.githubRepositoryId unchanged so the UI doesn't show an intermediate "unlinked" state while the server resolves the repo.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.ts (1)
41-46: Consider gating the rename-close on the returned transaction for consistency.Other call sites in this PR (e.g.,
RepositorySection.save,PriorityProperty.handleSelectPriority,TaskContextMenu.handleDelete) only close the edit/dropdown UI whenoptimisticAction(...)returns a truthytransaction. HeresetIsRenaming(false)runs unconditionally beforeprojectActions.renameProject(...), and the returned transaction is discarded. If the optimistic action ever fails to create a transaction synchronously, the input closes and the user loses the in-progress value with only a toast to explain it.♻️ Proposed alignment with the transaction-gated pattern
const submitRename = () => { - setIsRenaming(false); const trimmed = renameValue.trim(); - if (!trimmed || trimmed === project.name) return; - projectActions.renameProject(project.id, trimmed); + if (!trimmed || trimmed === project.name) { + setIsRenaming(false); + return; + } + const transaction = projectActions.renameProject(project.id, trimmed); + if (transaction) { + setIsRenaming(false); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.ts` around lines 41 - 46, The submitRename handler closes the rename input unconditionally before calling projectActions.renameProject, discarding any returned transaction and diverging from the optimisticAction-gated pattern; change submitRename to trim and early-return as now, then call projectActions.renameProject(project.id, trimmed), capture its return value (transaction), and only call setIsRenaming(false) if that captured transaction is truthy so the UI only closes when the optimistic action successfully creates a transaction.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts (1)
62-67: Same rename-close inconsistency as the project section hook.
setIsRenaming(false)runs beforeworkspaceActions.renameWorkspace(...)and the returned transaction is ignored. Other optimistic call sites in this PR gate UI closing on the returned transaction; consider aligning for consistency and to avoid closing the input when no transaction is created.♻️ Proposed alignment
const submitRename = () => { - setIsRenaming(false); const trimmed = renameValue.trim(); - if (!trimmed || trimmed === workspaceName) return; - workspaceActions.renameWorkspace(workspaceId, trimmed); + if (!trimmed || trimmed === workspaceName) { + setIsRenaming(false); + return; + } + const transaction = workspaceActions.renameWorkspace(workspaceId, trimmed); + if (transaction) { + setIsRenaming(false); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts` around lines 62 - 67, submitRename currently calls setIsRenaming(false) before invoking workspaceActions.renameWorkspace, which ignores the returned transaction and can close the UI when no transaction was created; update submitRename to call workspaceActions.renameWorkspace(workspaceId, trimmed) first, capture its return (e.g., tx), and only call setIsRenaming(false) after confirming tx exists (and/or after awaiting it if other hooks await completion) so the rename input only closes when a transaction is created—refer to submitRename, setIsRenaming, workspaceActions.renameWorkspace, workspaceName, and renameValue to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.ts`:
- Around line 41-46: The submitRename handler closes the rename input
unconditionally before calling projectActions.renameProject, discarding any
returned transaction and diverging from the optimisticAction-gated pattern;
change submitRename to trim and early-return as now, then call
projectActions.renameProject(project.id, trimmed), capture its return value
(transaction), and only call setIsRenaming(false) if that captured transaction
is truthy so the UI only closes when the optimistic action successfully creates
a transaction.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts`:
- Around line 62-67: submitRename currently calls setIsRenaming(false) before
invoking workspaceActions.renameWorkspace, which ignores the returned
transaction and can close the UI when no transaction was created; update
submitRename to call workspaceActions.renameWorkspace(workspaceId, trimmed)
first, capture its return (e.g., tx), and only call setIsRenaming(false) after
confirming tx exists (and/or after awaiting it if other hooks await completion)
so the rename input only closes when a transaction is created—refer to
submitRename, setIsRenaming, workspaceActions.renameWorkspace, workspaceName,
and renameValue to locate and change the logic.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/useV2ProjectOptimisticActions.ts`:
- Around line 43-49: The optimistic update in updateRepository currently always
sets draft.githubRepositoryId = null, causing a flicker when a non-null
repoCloneUrl is provided; change the mutation in updateRepository (the
collections.v2Projects.update callback) to only set draft.githubRepositoryId =
null when repoCloneUrl === null, otherwise leave draft.githubRepositoryId
unchanged so the UI doesn't show an intermediate "unlinked" state while the
server resolves the repo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb2f66be-ed82-4cfe-a2b9-567621c0b62c
📒 Files selected for processing (30)
apps/desktop/docs/OPTIMISTIC_ELECTRIC_UPDATES.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/AssigneeProperty/AssigneeProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/PriorityProperty/PriorityProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/StatusProperty/StatusProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskActionMenu/TaskActionMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksBoardView/TasksBoardView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/components/TaskContextMenu/TaskContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/AssigneeCell/AssigneeCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/PriorityCell/PriorityCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/StatusCell/StatusCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/useTaskOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useChatSessionOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useChatSessionOptimisticActions/useChatSessionOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/useV2ProjectOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/useV2WorkspaceOptimisticActions.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsxpackages/trpc/src/router/chat/chat.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
✅ Files skipped from review due to trivial changes (4)
- apps/desktop/src/renderer/routes/_authenticated/hooks/useV2ProjectOptimisticActions/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/index.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useV2WorkspaceOptimisticActions/index.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useChatSessionOptimisticActions/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/StatusProperty/StatusProperty.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/PriorityCell/PriorityCell.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskActionMenu/TaskActionMenu.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksBoardView/TasksBoardView.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/StatusCell/StatusCell.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsx
- apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/index.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/hooks/useTaskOptimisticActions/useTaskOptimisticActions.ts
- apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionMutation/useOptimisticCollectionMutation.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts (1)
99-111:⚠️ Potential issue | 🟡 MinorAnalytics event fires before persistence is confirmed.
posthog.capture("chat_session_deleted", ...)runs as soon as the optimistic transaction is enqueued. Iftransaction.isPersisted.promiselater rejects (rollback path inuseOptimisticCollectionActions), the session reappears locally but achat_session_deletedevent has already been emitted, skewing analytics. Consider gating the capture on persistence success, e.g.:Proposed change
- const transaction = chatSessionActions.deleteSession(sessionIdToDelete); - if (!transaction && !isDesktopChatDevMode()) { - throw new Error("Failed to delete chat session"); - } - - posthog.capture("chat_session_deleted", { - workspace_id: workspaceId, - session_id: sessionIdToDelete, - organization_id: organizationId, - }); + const transaction = chatSessionActions.deleteSession(sessionIdToDelete); + if (!transaction && !isDesktopChatDevMode()) { + throw new Error("Failed to delete chat session"); + } + + const capture = () => + posthog.capture("chat_session_deleted", { + workspace_id: workspaceId, + session_id: sessionIdToDelete, + organization_id: organizationId, + }); + if (transaction) { + void transaction.isPersisted.promise.then(capture, () => {}); + } else { + // dev mode: no real persistence, capture immediately + capture(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts around lines 99 - 111, The analytics event is emitted before the optimistic delete is confirmed; change the flow in the delete path around chatSessionActions.deleteSession so you only call posthog.capture("chat_session_deleted", ...) and invoke onSessionIdChange(null) after the persisted outcome succeeds: check the returned transaction from chatSessionActions.deleteSession and await transaction?.isPersisted?.promise (or attach a .then) to run the capture and session-id change on success; if there is no transaction (legacy/dev path) preserve the current behavior but avoid emitting the event on an eventual rollback (i.e., do not capture on rejection).
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/useOptimisticCollectionActions.ts (1)
191-199:deleteSessionconflates dev-mode skip with failure.Returning
nullhere means callers cannot distinguish "deletion intentionally skipped in dev mode" from "mutation threw synchronously and was reported via toast". The current consumer (useWorkspaceChatController.handleDeleteSession) works around this by re-checkingisDesktopChatDevMode(), which leaks the internal contract. Consider returning a distinct sentinel for the dev-mode branch (e.g. a stubPersistableTransactionwhoseisPersisted.promiseresolves), or have the consumer own the dev-mode short-circuit so this hook's return value uniformly means "mutation enqueued vs failed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/useOptimisticCollectionActions.ts` around lines 191 - 199, The deleteSession implementation in useOptimisticCollectionActions conflates a dev-mode short-circuit with a failure by returning null; change deleteSession so its return value consistently represents "mutation enqueued" vs "failed": either move the isDesktopChatDevMode() short-circuit to the caller (useWorkspaceChatController.handleDeleteSession) or, preferably, return a sentinel stub PersistableTransaction from deleteSession when dev mode is active (a stub whose isPersisted.promise resolves immediately) so callers can uniformly await/inspect the transaction; update references to runChatSessionMutation, collections.chatSessions.delete, and deleteSession accordingly.apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx (1)
34-47: Minor UX note on rollback after edit-mode close.When
updateRepositoryreturns a truthy transaction we close edit mode immediately, but if persistence later fails the row reverts to the prior URL while the user is no longer in the edit field — they only see the toast. That matches the optimistic-online policy elsewhere in the PR, so this is fine, but worth confirming this is the intended behavior for repository edits (where users may want to re-attempt the input rather than re-open the editor from scratch).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx around lines 34 - 47, The save handler currently calls projectActions.updateRepository(...) and immediately calls setIsEditing(false) when a transaction is returned, which closes the editor even if persistence later fails; change save so it does not unconditionally close the editor — instead, call projectActions.updateRepository(projectId, trimmed === "" ? null : trimmed) and if it returns a promise/transaction attach success/failure handlers so you only call setIsEditing(false) when the transaction completes successfully, and on failure keep the editor open (and show the existing toast/error) so the user can retry without re-opening the field; update the save function and the transaction handling logic to subscribe to the returned transaction result rather than closing edit-mode immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.ts:
- Around line 99-111: The analytics event is emitted before the optimistic
delete is confirmed; change the flow in the delete path around
chatSessionActions.deleteSession so you only call
posthog.capture("chat_session_deleted", ...) and invoke onSessionIdChange(null)
after the persisted outcome succeeds: check the returned transaction from
chatSessionActions.deleteSession and await transaction?.isPersisted?.promise (or
attach a .then) to run the capture and session-id change on success; if there is
no transaction (legacy/dev path) preserve the current behavior but avoid
emitting the event on an eventual rollback (i.e., do not capture on rejection).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/useOptimisticCollectionActions.ts`:
- Around line 191-199: The deleteSession implementation in
useOptimisticCollectionActions conflates a dev-mode short-circuit with a failure
by returning null; change deleteSession so its return value consistently
represents "mutation enqueued" vs "failed": either move the
isDesktopChatDevMode() short-circuit to the caller
(useWorkspaceChatController.handleDeleteSession) or, preferably, return a
sentinel stub PersistableTransaction from deleteSession when dev mode is active
(a stub whose isPersisted.promise resolves immediately) so callers can uniformly
await/inspect the transaction; update references to runChatSessionMutation,
collections.chatSessions.delete, and deleteSession accordingly.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx:
- Around line 34-47: The save handler currently calls
projectActions.updateRepository(...) and immediately calls setIsEditing(false)
when a transaction is returned, which closes the editor even if persistence
later fails; change save so it does not unconditionally close the editor —
instead, call projectActions.updateRepository(projectId, trimmed === "" ? null :
trimmed) and if it returns a promise/transaction attach success/failure handlers
so you only call setIsEditing(false) when the transaction completes
successfully, and on failure keep the editor open (and show the existing
toast/error) so the user can retry without re-opening the field; update the save
function and the transaction handling logic to subscribe to the returned
transaction result rather than closing edit-mode immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c5e9c1-0461-4cba-8bb1-87b2372a5dee
📒 Files selected for processing (17)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/AssigneeProperty/AssigneeProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/PriorityProperty/PriorityProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/StatusProperty/StatusProperty.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskActionMenu/TaskActionMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksBoardView/TasksBoardView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/components/TaskContextMenu/TaskContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/AssigneeCell/AssigneeCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/PriorityCell/PriorityCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/StatusCell/StatusCell.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/ChatPane/hooks/useWorkspaceChatController/useWorkspaceChatController.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/useOptimisticCollectionActions.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTableView/TasksTableView.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/StatusProperty/StatusProperty.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksBoardView/TasksBoardView.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskActionMenu/TaskActionMenu.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/PriorityProperty/PriorityProperty.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/components/PriorityCell/PriorityCell.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/AssigneeProperty/AssigneeProperty.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/docs/OPTIMISTIC_ELECTRIC_UPDATES.md (1)
99-99:⚠️ Potential issue | 🟡 MinorGrammar: hyphenate compound modifier.
The compound modifier "server side" should be hyphenated when used as an adjective.
📝 Proposed fix
-Use `{ optimistic: false }` only for exceptional flows where the UI must wait for server confirmation before revealing the result, such as a workflow that depends on a server-generated identifier or a multi-step server side effect. +Use `{ optimistic: false }` only for exceptional flows where the UI must wait for server confirmation before revealing the result, such as a workflow that depends on a server-generated identifier or a multi-step server-side effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/docs/OPTIMISTIC_ELECTRIC_UPDATES.md` at line 99, The phrase "server side" in the sentence "a workflow that depends on a server-generated identifier or a multi-step server side effect." should be hyphenated as "server-side" because it's a compound modifier; update that occurrence (in the sentence referencing a workflow that depends on a server-generated identifier or a multi-step server side effect) to read "multi-step server-side effect."
🤖 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/docs/OPTIMISTIC_ELECTRIC_UPDATES.md`:
- Line 99: The phrase "server side" in the sentence "a workflow that depends on
a server-generated identifier or a multi-step server side effect." should be
hyphenated as "server-side" because it's a compound modifier; update that
occurrence (in the sentence referencing a workflow that depends on a
server-generated identifier or a multi-step server side effect) to read
"multi-step server-side effect."
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a9884a5-4940-48b8-80e0-d3967d087190
📒 Files selected for processing (1)
apps/desktop/docs/OPTIMISTIC_ELECTRIC_UPDATES.md
PR1〜PR5 (#435 #436 #437 #438 #440) で 13 commits 全件 cherry-pick + 手動 conflict 解消で取り込み済み。 本コミットは git 履歴上 behind=0 とするための ours マージ記録。 取り込み済み 13 commits: - 1f55c62 Fix host service restart adoption (superset-sh#3732) - 0fe65d2 test(desktop): remove host-service-coordinator test (superset-sh#3734) - 3012b5a Add optimistic Electric collection updates (superset-sh#3722) - c272a51 fix(desktop): drop branch row from v2 sidebar workspace item (superset-sh#3733) - c2f3fdc feat(desktop): add fade-edge mask utilities (superset-sh#3735) - 682d07c fix v2 terminal osc links (superset-sh#3736) - 7c0d22b feat(ports): surface remote host-service ports in the sidebar (superset-sh#3676) - 6a3be2d [codex] Stabilize v2 terminal resize (superset-sh#3739) - 8928ac6 [codex] Improve v2 pane header responsiveness (superset-sh#3737) - 5fe3d22 refactor(desktop): tidy v2 terminal session dropdown (superset-sh#3743) - 66c23d6 Fix automation timezone scheduling (superset-sh#3738) - 16e270c [codex] Add terminal session titles (superset-sh#3740) - 583fa5d fix(desktop): refit v2 terminal after font settle (superset-sh#3742)
Summary
useOptimisticCollectionActions, grouped by tasks, v2 projects, v2 workspaces, and chat sessionstxidsTests
Manual QA
Deleting "<name>"...while cleanup runs, hides the row during deletion, and is not affected by optimistic collection actions.