Migrate v1 keypad loading screen to v2 as a separate page#3788
Merged
AviPeltz merged 6 commits intoApr 27, 2026
Conversation
Adds a separate /v2-workspace-loading/$workspaceId route that mirrors v1's keypad-loader/step-progress visuals while the v2Workspaces collection is hydrating. Layout redirects there when isReady is false and the loading page navigates back to the workspace once data lands.
…n-from-v1-to-v2-with-separate
The pending workspace page now drives V2WorkspaceLoadingView off the live workspaceCreation.getProgress feed (fork + checkout) so keys press in response to real backend steps instead of a slow fixed timer. Mapper collapses the host's 3 steps onto the keypad's 5-key vocabulary; fallback timer for adopt + cold-load is sped up from 1500ms to 400ms.
Contributor
📝 WalkthroughWalkthroughPending/workspace flow refactor: pending page and workspace layout now route to and render a new V2WorkspaceLoadingView that maps host progress into init steps and displays a keypad loader + step progress until navigation to the workspace occurs. Changes
Sequence DiagramsequenceDiagram
participant User
participant Pending as Pending Page
participant Layout as V2WorkspaceLayout
participant V2Load as V2WorkspaceLoadingView
participant Keypad as KeypadLoader
participant Steps as StepProgress
participant Host
participant DB
User->>Pending: open pending workspace
Pending->>Layout: mounts workspace layout
Layout->>V2Load: redirect to /v2-workspace-loading if workspace not ready
Host->>DB: emit progress updates
DB-->>V2Load: mapped currentStep updates
V2Load->>Keypad: render with currentStep
V2Load->>Steps: render with currentStep
Keypad-->>User: visual keypad states (pressed/active)
Steps-->>User: animated step list (hold/completion)
DB-->>V2Load: final step = ready
V2Load->>Pending: delay then navigate to workspace route
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Greptile SummaryThis PR ports the v1 keypad loading animation to v2 as a standalone
Confidence Score: 4/5Safe to merge after the two P1 navigation issues are addressed; the visual components themselves are solid. Two P1 logic issues exist: (1) the loading page navigates unconditionally when
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/page.tsx | New cold-start loading route: navigates unconditionally when isReady even if the workspace row is null, and participates in a potential redirect loop with the v2-workspace layout. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Adds redirect to loading route when !isReady; could create a flicker/redirect loop with the loading page if useLiveQuery always initialises isReady to false on re-mount. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx | Replaces the inline creating-state UI with V2WorkspaceLoadingView; mapHostProgressToInitStep has a gap-state that returns "pending" between host steps, causing a brief visual regression. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx | New container component; fallback timer and prop-driven step wiring look correct; no issues found. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.tsx | New v2 copy of KeypadLoader without audio dependency; pressed/active derivation via getStepIndex is correct. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.tsx | New step-progress ticker; one-at-a-time advancement with DONE_HOLD_MS hold and backward-jump guard looks correct; "failed" step returns index -1 but is never passed in practice. |
Sequence Diagram
sequenceDiagram
participant U as User
participant L as V2WorkspaceLayout
participant LP as V2WorkspaceLoadingPage
participant DB as useLiveQuery (v2Workspaces)
U->>L: Navigate to /v2-workspace/$id (cold start)
L->>DB: subscribe(v2Workspaces)
DB-->>L: isReady=false
L->>LP: navigate (replace) /v2-workspace-loading/$id
LP->>DB: subscribe(v2Workspaces)
DB-->>LP: isReady=false then isReady=true
LP->>L: navigate (replace) /v2-workspace/$id
L->>DB: subscribe(v2Workspaces) re-mount
alt useLiveQuery returns isReady=true synchronously
DB-->>L: isReady=true
L->>U: Render workspace
else useLiveQuery initialises isReady=false on every mount
DB-->>L: isReady=false
L->>LP: navigate (replace) loop
LP->>LP: isReady=true immediately navigate back
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/page.tsx
Line: 28-35
Comment:
**Unconditional navigation when `isReady` — workspace may not exist**
`useEffect` navigates to `/v2-workspace/$workspaceId` whenever `isReady` flips to `true`, even when `workspace` is `null` (the collection loaded but the row wasn't found). If a user somehow reaches this URL with a non-existent or already-deleted `workspaceId`, they'll be silently redirected to the workspace route, which renders `WorkspaceNotFoundState`. Adding a `workspace !== null` guard ensures the loading page only advances when the record actually hydrated — it can otherwise stay and keep showing the animation while the row hasn't synced yet.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
Line: 74-82
Comment:
**Potential redirect loop on cold-start re-mount**
When the loading page's `isReady` fires and it navigates back to `/v2-workspace/$workspaceId` (line 30–34 of `page.tsx`), this layout re-mounts and immediately runs its own `useLiveQuery`. If `useLiveQuery` initialises `isReady` to `false` synchronously on every fresh mount (before the first subscription tick delivers the already-hydrated collection state), this effect fires again with `workspaceId && !isReady === true` and sends the user back to the loading page. The loading page sees `isReady=true` almost immediately and bounces back, creating a rapidly alternating flicker loop until the hook initialises to `true`. The guard `if (!workspaceId || !isReady) return null` on line 84 means the `Outlet` never renders during that window, which compounds the visible jank.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Line: 517-527
Comment:
**Gap state between host steps maps to `"pending"`, causing keypad to visually reset**
When `ensuring_repo` transitions from `"active"` → `"done"` but `creating_worktree` has not yet become `"active"` (a window that can span the 500 ms poll interval), none of the four conditions match and the function falls through to `return "pending"`. This takes `StepProgress` back to index 0 — visible as the progress ticker snapping backwards — before jumping forward again when the next poll arrives. Adding a `byId.get("ensuring_repo") === "done"` case mapped to something like `"fetching"` (or `"creating_worktree"`) would eliminate the regression.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "wire keypad loader to host-service progr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+28
to
+35
| useEffect(() => { | ||
| if (!isReady) return; | ||
| void navigate({ | ||
| to: "/v2-workspace/$workspaceId", | ||
| params: { workspaceId }, | ||
| replace: true, | ||
| }); | ||
| }, [isReady, navigate, workspaceId]); |
There was a problem hiding this comment.
isReady — workspace may not exist
useEffect navigates to /v2-workspace/$workspaceId whenever isReady flips to true, even when workspace is null (the collection loaded but the row wasn't found). If a user somehow reaches this URL with a non-existent or already-deleted workspaceId, they'll be silently redirected to the workspace route, which renders WorkspaceNotFoundState. Adding a workspace !== null guard ensures the loading page only advances when the record actually hydrated — it can otherwise stay and keep showing the animation while the row hasn't synced yet.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/page.tsx
Line: 28-35
Comment:
**Unconditional navigation when `isReady` — workspace may not exist**
`useEffect` navigates to `/v2-workspace/$workspaceId` whenever `isReady` flips to `true`, even when `workspace` is `null` (the collection loaded but the row wasn't found). If a user somehow reaches this URL with a non-existent or already-deleted `workspaceId`, they'll be silently redirected to the workspace route, which renders `WorkspaceNotFoundState`. Adding a `workspace !== null` guard ensures the loading page only advances when the record actually hydrated — it can otherwise stay and keep showing the animation while the row hasn't synced yet.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+74
to
+82
| useEffect(() => { | ||
| if (workspaceId && !isReady) { | ||
| void navigate({ | ||
| to: "/v2-workspace-loading/$workspaceId", | ||
| params: { workspaceId }, | ||
| replace: true, | ||
| }); | ||
| } | ||
| }, [workspaceId, isReady, navigate]); |
There was a problem hiding this comment.
When the loading page's isReady fires and it navigates back to /v2-workspace/$workspaceId (line 30–34 of page.tsx), this layout re-mounts and immediately runs its own useLiveQuery. If useLiveQuery initialises isReady to false synchronously on every fresh mount (before the first subscription tick delivers the already-hydrated collection state), this effect fires again with workspaceId && !isReady === true and sends the user back to the loading page. The loading page sees isReady=true almost immediately and bounces back, creating a rapidly alternating flicker loop until the hook initialises to true. The guard if (!workspaceId || !isReady) return null on line 84 means the Outlet never renders during that window, which compounds the visible jank.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
Line: 74-82
Comment:
**Potential redirect loop on cold-start re-mount**
When the loading page's `isReady` fires and it navigates back to `/v2-workspace/$workspaceId` (line 30–34 of `page.tsx`), this layout re-mounts and immediately runs its own `useLiveQuery`. If `useLiveQuery` initialises `isReady` to `false` synchronously on every fresh mount (before the first subscription tick delivers the already-hydrated collection state), this effect fires again with `workspaceId && !isReady === true` and sends the user back to the loading page. The loading page sees `isReady=true` almost immediately and bounces back, creating a rapidly alternating flicker loop until the hook initialises to `true`. The guard `if (!workspaceId || !isReady) return null` on line 84 means the `Outlet` never renders during that window, which compounds the visible jank.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+517
to
+527
| function mapHostProgressToInitStep( | ||
| steps: HostProgressStep[] | null | undefined, | ||
| ): WorkspaceInitStep | undefined { | ||
| if (!steps || steps.length === 0) return undefined; | ||
| const byId = new Map(steps.map((s) => [s.id, s.status])); | ||
| if (byId.get("registering") === "done") return "ready"; | ||
| if (byId.get("registering") === "active") return "finalizing"; | ||
| if (byId.get("creating_worktree") === "active") return "creating_worktree"; | ||
| if (byId.get("ensuring_repo") === "active") return "syncing"; | ||
| return "pending"; | ||
| } |
There was a problem hiding this comment.
"pending", causing keypad to visually reset
When ensuring_repo transitions from "active" → "done" but creating_worktree has not yet become "active" (a window that can span the 500 ms poll interval), none of the four conditions match and the function falls through to return "pending". This takes StepProgress back to index 0 — visible as the progress ticker snapping backwards — before jumping forward again when the next poll arrives. Adding a byId.get("ensuring_repo") === "done" case mapped to something like "fetching" (or "creating_worktree") would eliminate the regression.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Line: 517-527
Comment:
**Gap state between host steps maps to `"pending"`, causing keypad to visually reset**
When `ensuring_repo` transitions from `"active"` → `"done"` but `creating_worktree` has not yet become `"active"` (a window that can span the 500 ms poll interval), none of the four conditions match and the function falls through to `return "pending"`. This takes `StepProgress` back to index 0 — visible as the progress ticker snapping backwards — before jumping forward again when the next poll arrives. Adding a `byId.get("ensuring_repo") === "done"` case mapped to something like `"fetching"` (or `"creating_worktree"`) would eliminate the regression.
How can I resolve this? If you propose a fix, please make it concise.![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: 2
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.tsx (1)
28-64: Pre-computethresholdIdxonce per key.
getStepIndex(pressedAfter)is recomputed on every render for all five keys. SincepressedAfteris static, you can resolve thresholds when definingKEYSand skip the per-render lookup.♻️ Proposed refactor
-const KEYS: readonly KeyDef[] = [ +const KEYS_RAW: readonly KeyDef[] = [ { id: "one", pressedAfter: "verifying", activeSteps: ["pending", "syncing", "verifying"], Icon: LuRefreshCw, label: "Syncing", }, // ... rest unchanged ]; + +const KEYS = KEYS_RAW.map((k) => ({ + ...k, + thresholdIdx: getStepIndex(k.pressedAfter), +}));And in the render:
-{KEYS.map(({ id, pressedAfter, activeSteps, Icon }) => { - const thresholdIdx = getStepIndex(pressedAfter); +{KEYS.map(({ id, thresholdIdx, activeSteps, Icon }) => {🤖 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-loading/`$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.tsx around lines 28 - 64, The KEYS array currently stores pressedAfter strings and calls getStepIndex(pressedAfter) on every render; compute each key's threshold index once when defining KEYS to avoid repeated lookups. Modify the KEYS definition (KeyDef entries) to include a precomputed numeric field (e.g., thresholdIdx or thresholdIndex) computed from getStepIndex(pressedAfter) and then update rendering logic to read that field instead of calling getStepIndex for each key; update any references to pressedAfter in the render to use the new threshold field so per-render work is eliminated.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx (1)
38-44: Optionally stop the interval oncestepIdxreaches the end.Once
stepIdxis clamped toVISIBLE_STEPS.length - 1, the interval keeps firing every 400ms with a no-opsetStepIdx. Not a leak (cleared on unmount), but easy to short-circuit.♻️ Proposed refactor
useEffect(() => { if (currentStepProp !== undefined) return; const id = window.setInterval(() => { - setStepIdx((prev) => Math.min(prev + 1, VISIBLE_STEPS.length - 1)); + setStepIdx((prev) => { + const next = prev + 1; + if (next >= VISIBLE_STEPS.length - 1) { + window.clearInterval(id); + return VISIBLE_STEPS.length - 1; + } + return next; + }); }, STEP_INTERVAL_MS); return () => window.clearInterval(id); }, [currentStepProp]);🤖 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-loading/`$workspaceId/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx around lines 38 - 44, The interval keeps firing after stepIdx reaches VISIBLE_STEPS.length - 1; update the useEffect (the hook that depends on currentStepProp) so the interval clears itself once the last step is reached: in the window.setInterval callback (used with STEP_INTERVAL_MS) compute the next index from setStepIdx (using current prev), and if next === VISIBLE_STEPS.length - 1 (or next === prev) call window.clearInterval(id) before returning, otherwise call setStepIdx as today; keep the early return when currentStepProp !== undefined. Reference: useEffect, currentStepProp, setStepIdx, VISIBLE_STEPS, STEP_INTERVAL_MS.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.tsx (1)
97-184: Consider extracting icon sub-components.
CheckCircle,EmptyCircle,HalfCircle, andEllipsisare exported-shape React components living alongside the main one. Per the repo convention (one component per file), you could move these into a co-locatedicons/(orparts/) subfolder. Not blocking — they're private helpers — but worth tracking if these grow.As per coding guidelines: "Do not create multi-component files; use one component per file."
🤖 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-loading/`$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.tsx around lines 97 - 184, The file defines multiple React subcomponents (CheckCircle, EmptyCircle, HalfCircle, Ellipsis) alongside the main StepIcon/StepProgress component which violates the "one component per file" convention; extract each of those four into its own file under a co-located icons/ (or parts/) folder (e.g., icons/CheckCircle.tsx, icons/EmptyCircle.tsx, icons/HalfCircle.tsx, icons/Ellipsis.tsx) as default-exported React components, update the StepProgress/StepIcon file to import those components and remove their local definitions, and ensure any relative imports/exports or tests referencing these symbols are updated accordingly so behavior remains unchanged.apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (1)
517-527: Mapper can momentarily reverse progress between step transitions.Between when one host step flips to
doneand the next step flips toactive, all three mapped checks miss and the function returns"pending". That can briefly walk the keypad backward (e.g.creating_worktree→pending→finalizing) at each step boundary. Includingdonefallthroughs anchors the loader to the highest reached step:♻️ Proposed monotonic mapping
function mapHostProgressToInitStep( steps: HostProgressStep[] | null | undefined, ): WorkspaceInitStep | undefined { if (!steps || steps.length === 0) return undefined; const byId = new Map(steps.map((s) => [s.id, s.status])); if (byId.get("registering") === "done") return "ready"; if (byId.get("registering") === "active") return "finalizing"; + if (byId.get("creating_worktree") === "done") return "finalizing"; if (byId.get("creating_worktree") === "active") return "creating_worktree"; + if (byId.get("ensuring_repo") === "done") return "creating_worktree"; if (byId.get("ensuring_repo") === "active") return "syncing"; return "pending"; }🤖 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/pending/`$pendingId/page.tsx around lines 517 - 527, The mapper mapHostProgressToInitStep currently returns "pending" during the brief window between one step becoming "done" and the next becoming "active", which causes UI regressions; update mapHostProgressToInitStep (and its use of HostProgressStep/WorkspaceInitStep) to treat "done" as having reached that step by adding fallthrough checks for done statuses (e.g., if byId.get("creating_worktree") === "done' return "creating_worktree"; if byId.get("ensuring_repo") === "done" return "syncing"; similarly ensure registering "done" still returns "ready"), and order the checks so the highest-reached step (active or done) is returned to keep progress monotonic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Line 17: V2WorkspaceLoadingView (and its dependencies KeypadLoader and
StepProgress) is being imported across a route segment ($workspaceId), which
couples component ownership to routing; move V2WorkspaceLoadingView plus
KeypadLoader and StepProgress into a shared, route-independent location (for
example _authenticated/_dashboard/components/V2WorkspaceLoadingView/) and update
both consumers (v2-workspace-loading/$workspaceId/page.tsx and
pending/$pendingId/page.tsx) to import from that new stable path so the
component is owned by a shared parent instead of a specific route segment.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/`$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.css:
- Around line 1-10: In the .keypad-loader rule update the formatting so there's
a blank line between the custom property --travel and the first regular
declaration; specifically insert an empty line before the position: relative;
declaration (in the .keypad-loader block) to satisfy the
declaration-empty-line-before Stylelint rule.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 517-527: The mapper mapHostProgressToInitStep currently returns
"pending" during the brief window between one step becoming "done" and the next
becoming "active", which causes UI regressions; update mapHostProgressToInitStep
(and its use of HostProgressStep/WorkspaceInitStep) to treat "done" as having
reached that step by adding fallthrough checks for done statuses (e.g., if
byId.get("creating_worktree") === "done' return "creating_worktree"; if
byId.get("ensuring_repo") === "done" return "syncing"; similarly ensure
registering "done" still returns "ready"), and order the checks so the
highest-reached step (active or done) is returned to keep progress monotonic.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/`$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.tsx:
- Around line 28-64: The KEYS array currently stores pressedAfter strings and
calls getStepIndex(pressedAfter) on every render; compute each key's threshold
index once when defining KEYS to avoid repeated lookups. Modify the KEYS
definition (KeyDef entries) to include a precomputed numeric field (e.g.,
thresholdIdx or thresholdIndex) computed from getStepIndex(pressedAfter) and
then update rendering logic to read that field instead of calling getStepIndex
for each key; update any references to pressedAfter in the render to use the new
threshold field so per-render work is eliminated.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/`$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.tsx:
- Around line 97-184: The file defines multiple React subcomponents
(CheckCircle, EmptyCircle, HalfCircle, Ellipsis) alongside the main
StepIcon/StepProgress component which violates the "one component per file"
convention; extract each of those four into its own file under a co-located
icons/ (or parts/) folder (e.g., icons/CheckCircle.tsx, icons/EmptyCircle.tsx,
icons/HalfCircle.tsx, icons/Ellipsis.tsx) as default-exported React components,
update the StepProgress/StepIcon file to import those components and remove
their local definitions, and ensure any relative imports/exports or tests
referencing these symbols are updated accordingly so behavior remains unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/`$workspaceId/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx:
- Around line 38-44: The interval keeps firing after stepIdx reaches
VISIBLE_STEPS.length - 1; update the useEffect (the hook that depends on
currentStepProp) so the interval clears itself once the last step is reached: in
the window.setInterval callback (used with STEP_INTERVAL_MS) compute the next
index from setStepIdx (using current prev), and if next === VISIBLE_STEPS.length
- 1 (or next === prev) call window.clearInterval(id) before returning, otherwise
call setStepIdx as today; keep the early return when currentStepProp !==
undefined. Reference: useEffect, currentStepProp, setStepIdx, VISIBLE_STEPS,
STEP_INTERVAL_MS.
🪄 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: 5c67c4fa-656d-4f1f-a26a-1080f572976a
⛔ Files ignored due to path filters (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/assets/key-single.pngis excluded by!**/*.pngapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/assets/keypad-base.pngis excluded by!**/*.png
📒 Files selected for processing (11)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/StepProgress/StepProgress.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/StepProgress/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
| clearAttachments, | ||
| loadAttachments, | ||
| } from "renderer/lib/pending-attachment-store"; | ||
| import { V2WorkspaceLoadingView } from "renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/components/V2WorkspaceLoadingView"; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Promote V2WorkspaceLoadingView to a shared location.
V2WorkspaceLoadingView is now consumed by both v2-workspace-loading/$workspaceId/page.tsx (where it currently lives) and this pending page in a sibling route. The cross-route import path crossing a $workspaceId segment is also a smell — that segment exists for routing, not for component ownership. Consider moving the component (and its KeypadLoader/StepProgress dependencies) up to the highest shared parent, e.g. _authenticated/_dashboard/components/V2WorkspaceLoadingView/, so both consumers import from a stable, route-independent location.
As per coding guidelines: "If a utility is used once, nest it under the parent's directory. If used 2+ times, promote it to the highest shared parent's directory or top-level components/."
🤖 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/pending/`$pendingId/page.tsx
at line 17, V2WorkspaceLoadingView (and its dependencies KeypadLoader and
StepProgress) is being imported across a route segment ($workspaceId), which
couples component ownership to routing; move V2WorkspaceLoadingView plus
KeypadLoader and StepProgress into a shared, route-independent location (for
example _authenticated/_dashboard/components/V2WorkspaceLoadingView/) and update
both consumers (v2-workspace-loading/$workspaceId/page.tsx and
pending/$pendingId/page.tsx) to import from that new stable path so the
component is owned by a shared parent instead of a specific route segment.
Comment on lines
+1
to
+10
| .keypad-loader { | ||
| --travel: 26; | ||
| position: relative; | ||
| aspect-ratio: 400 / 310; | ||
| display: flex; | ||
| place-items: center; | ||
| width: clamp(260px, 34vw, 420px); | ||
| transform-style: preserve-3d; | ||
| user-select: none; | ||
| } |
Contributor
There was a problem hiding this comment.
Stylelint: missing empty line before position declaration.
Static analysis flagged declaration-empty-line-before on line 3 — the rule wants a blank line between a custom property and a regular declaration in the same block.
🎨 Proposed fix
.keypad-loader {
--travel: 26;
+
position: relative;🧰 Tools
🪛 Stylelint (17.9.0)
[error] 3-3: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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-loading/`$workspaceId/components/V2WorkspaceLoadingView/KeypadLoader/KeypadLoader.css
around lines 1 - 10, In the .keypad-loader rule update the formatting so there's
a blank line between the custom property --travel and the first regular
declaration; specifically insert an empty line before the position: relative;
declaration (in the .keypad-loader block) to satisfy the
declaration-empty-line-before Stylelint rule.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
The host clears workspaceCreation progress without ever reporting "registering: done", so the 5th keypad key never got the pressed state from real progress alone — and the moment the pending row flipped to "succeeded" we swapped to the old text-based "Workspace ready" UI. Now V2WorkspaceLoadingView stays mounted through "succeeded" with currentStep="ready" (all keys pressed), and doNavigate is held 700ms so the press animation actually plays before the route transition. The sync-timeout recovery UI still applies when the workspace row hasn't synced; warnings render below the keypad in the success path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (2)
526-536: Mapper can transiently regress to"pending"between host steps.The mapper only checks
"active"forensuring_repo/creating_worktree. If a step has finished but the next one hasn't flipped toactiveyet (e.g.creating_worktree === "done"whileregistering === "pending"), the function falls through to"pending"and the keypad visually rewinds to the first key. Same hazard betweenensuring_repo === "done"andcreating_worktree === "pending".♻️ Make the mapping monotonic by checking `done` for prior steps
function mapHostProgressToInitStep( steps: HostProgressStep[] | null | undefined, ): WorkspaceInitStep | undefined { if (!steps || steps.length === 0) return undefined; const byId = new Map(steps.map((s) => [s.id, s.status])); if (byId.get("registering") === "done") return "ready"; if (byId.get("registering") === "active") return "finalizing"; + if (byId.get("creating_worktree") === "done") return "finalizing"; if (byId.get("creating_worktree") === "active") return "creating_worktree"; + if (byId.get("ensuring_repo") === "done") return "creating_worktree"; if (byId.get("ensuring_repo") === "active") return "syncing"; return "pending"; }🤖 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/pending/`$pendingId/page.tsx around lines 526 - 536, mapHostProgressToInitStep can regress to "pending" when a later step has finished but the prior hasn't flipped to "active"; fix by making the mapping monotonic: in function mapHostProgressToInitStep (and using HostProgressStep ids "registering", "creating_worktree", "ensuring_repo"), treat "done" on later steps as evidence of progress — e.g. if byId.get("creating_worktree") === "done" return "creating_worktree"; if byId.get("ensuring_repo") === "done" return "syncing"; keep existing checks for "active" and the registering/done => "ready" case so the UI never visually rewinds to "pending" when a subsequent step is already done.
517-521: DeriveHostProgressStepfrom the tRPC procedure return type to prevent type drift.The local type duplicates the host-service
ProgressStepinterface. If the server adds a new status value (e.g.,"error"), the client type won't update automatically, and the mapper's fallthrough to"pending"(line 535) would silently mask the new state. Use type inference instead:Awaited<ReturnType<typeof client.workspaceCreation.getProgress.query>>['steps'][number]or equivalent to keep the type in sync with the server.🤖 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/pending/`$pendingId/page.tsx around lines 517 - 521, The local HostProgressStep type duplicates the server ProgressStep and can drift; replace it by deriving the client-side step type from the tRPC procedure return type (use Awaited<ReturnType<typeof client.workspaceCreation.getProgress.query>>['steps'][number] or equivalent) so the client stays in sync with the server, and remove the hardcoded union ("pending"|"active"|"done"); update any mappers that inspect step.status (the mapper that currently falls back to "pending") to handle the derived type instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 415-424: The current UI only renders pending.warnings when
isFinalizing is true inside V2WorkspaceLoadingView, causing warnings to be lost
when syncTimedOut && !workspaceSynced (where showKeypad is false) and the
recovery fallthrough is shown; update the rendering so pending.warnings is
visible in both branches — either hoist the warnings block out of the
isFinalizing conditional (render it above the
V2WorkspaceLoadingView/sync-timeout branching) or duplicate the same warnings
list into the sync-timeout/recovery branch that checks syncTimedOut and
workspaceSynced, using the same JSX structure and keys so warnings appear
whether finalizing succeeded or the sync-timeout recovery UI is shown.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 526-536: mapHostProgressToInitStep can regress to "pending" when a
later step has finished but the prior hasn't flipped to "active"; fix by making
the mapping monotonic: in function mapHostProgressToInitStep (and using
HostProgressStep ids "registering", "creating_worktree", "ensuring_repo"), treat
"done" on later steps as evidence of progress — e.g. if
byId.get("creating_worktree") === "done" return "creating_worktree"; if
byId.get("ensuring_repo") === "done" return "syncing"; keep existing checks for
"active" and the registering/done => "ready" case so the UI never visually
rewinds to "pending" when a subsequent step is already done.
- Around line 517-521: The local HostProgressStep type duplicates the server
ProgressStep and can drift; replace it by deriving the client-side step type
from the tRPC procedure return type (use Awaited<ReturnType<typeof
client.workspaceCreation.getProgress.query>>['steps'][number] or equivalent) so
the client stays in sync with the server, and remove the hardcoded union
("pending"|"active"|"done"); update any mappers that inspect step.status (the
mapper that currently falls back to "pending") to handle the derived type
instead.
🪄 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: 4850fa35-9da2-4fa8-abbd-4b4e71dfb00d
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Comment on lines
+415
to
+424
| {isFinalizing && pending.warnings.length > 0 && ( | ||
| <ul className="mt-2 space-y-1 text-xs text-amber-500 text-left"> | ||
| {pending.warnings.map((w) => ( | ||
| <li key={w} className="flex items-start gap-1.5"> | ||
| <HiExclamationTriangle className="size-3.5 mt-0.5 shrink-0" /> | ||
| <span>{w}</span> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| )} |
Contributor
There was a problem hiding this comment.
Warnings are dropped when sync times out.
pending.warnings only renders inside V2WorkspaceLoadingView when isFinalizing is true. Once syncTimedOut && !workspaceSynced becomes true, showKeypad is false and the fallthrough recovery UI at lines 440-477 doesn't surface warnings — so any host warnings collected on success are silently lost in the very state where the user is stalling on a recoverable issue (likely the most useful place to see them).
Consider also rendering pending.warnings inside the sync-timeout block, or hoisting them above the conditional render so they appear in both success/recovery branches.
🤖 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/pending/`$pendingId/page.tsx
around lines 415 - 424, The current UI only renders pending.warnings when
isFinalizing is true inside V2WorkspaceLoadingView, causing warnings to be lost
when syncTimedOut && !workspaceSynced (where showKeypad is false) and the
recovery fallthrough is shown; update the rendering so pending.warnings is
visible in both branches — either hoist the warnings block out of the
isFinalizing conditional (render it above the
V2WorkspaceLoadingView/sync-timeout branching) or duplicate the same warnings
list into the sync-timeout/recovery branch that checks syncTimedOut and
workspaceSynced, using the same JSX structure and keys so warnings appear
whether finalizing succeeded or the sync-timeout recovery UI is shown.
The 700ms hold meant to let the keypad's last key animation finish was buggy — doNavigate's useCallback dep on `pending` (a fresh object each live-query tick) churned identity, so the effect cleared and re-armed the timer every render and never actually navigated. Workspace creation got stuck on the loading screen. Drop the hold; navigate immediately.
…n-from-v1-to-v2-with-separate # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (2)
18-18: 🛠️ Refactor suggestion | 🟠 MajorCo-locate
V2WorkspaceLoadingViewat a shared parent.
V2WorkspaceLoadingView(and itsKeypadLoader/StepProgressdeps) is now consumed by both this pending page andv2-workspace-loading/$workspaceId/page.tsx. Importing across a$workspaceIdroute segment couples component ownership to routing. Move it to e.g._authenticated/_dashboard/components/V2WorkspaceLoadingView/so both routes import from a stable location.As per coding guidelines: "If a utility is used once, nest it under the parent's directory. If used 2+ times, promote it to the highest shared parent's directory or top-level
components/."🤖 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/pending/`$pendingId/page.tsx at line 18, V2WorkspaceLoadingView and its dependencies (KeypadLoader, StepProgress) are being imported from the $workspaceId route which couples component ownership to routing; move the V2WorkspaceLoadingView component and its related components into a shared parent directory (e.g., _authenticated/_dashboard/components/V2WorkspaceLoadingView/) and update imports in both the pending page (where V2WorkspaceLoadingView is currently imported) and the v2-workspace-loading page so both import from the new stable shared location, ensuring export names (V2WorkspaceLoadingView, KeypadLoader, StepProgress) remain unchanged.
420-429:⚠️ Potential issue | 🟡 MinorWarnings are silently dropped on sync timeout.
pending.warningsonly renders inside theisFinalizingbranch ofV2WorkspaceLoadingView. OncesyncTimedOut && !workspaceSyncedflips true,showKeypadbecomes false and the recovery block at lines 445–482 has no warnings UI — so any host warnings collected at success are lost in the exact branch where they're most actionable. Either hoist the warnings list above the conditional render or duplicate the list into the sync-timeout branch.Also applies to: 445-453
🤖 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/pending/`$pendingId/page.tsx around lines 420 - 429, The warnings UI (pending.warnings) is only rendered inside the isFinalizing branch of V2WorkspaceLoadingView so warnings are lost when syncTimedOut && !workspaceSynced flips and showKeypad becomes false; fix by hoisting the warnings list out of the isFinalizing conditional (render it alongside the main loading view) or by duplicating the same warnings list into the sync-timeout/recovery branch that is shown when syncTimedOut && !workspaceSynced, referencing the existing pending.warnings map and the same list markup used in the current isFinalizing block to ensure parity.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (1)
522-541: Optional: deriveHostProgressStepfrom the tRPC query return type.
HostProgressStepis hand-rolled here while the real shape originates fromclient.workspaceCreation.getProgress.query(line 293). If the host-service ever adds a step status (e.g."failed") or renamesidvalues, this local type silently drifts and the mapper will fall through to"pending". Consider deriving it from the query result, e.g.NonNullable<Awaited<ReturnType<typeof client.workspaceCreation.getProgress.query>>>["steps"][number], or importing the shared host-service type if one is exported.🤖 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/pending/`$pendingId/page.tsx around lines 522 - 541, The locally-declared HostProgressStep type can drift from the actual tRPC query shape used by client.workspaceCreation.getProgress.query; update the code so HostProgressStep is derived from the query return type instead of being hand-rolled (or import the shared host-service type if available). Specifically, replace the manual HostProgressStep declaration and use a type like NonNullable<Awaited<ReturnType<typeof client.workspaceCreation.getProgress.query>>>["steps"][number] (or the exported host-service step type) and keep mapHostProgressToInitStep unchanged so it uses the derived type for its steps parameter.
🤖 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/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Line 18: V2WorkspaceLoadingView and its dependencies (KeypadLoader,
StepProgress) are being imported from the $workspaceId route which couples
component ownership to routing; move the V2WorkspaceLoadingView component and
its related components into a shared parent directory (e.g.,
_authenticated/_dashboard/components/V2WorkspaceLoadingView/) and update imports
in both the pending page (where V2WorkspaceLoadingView is currently imported)
and the v2-workspace-loading page so both import from the new stable shared
location, ensuring export names (V2WorkspaceLoadingView, KeypadLoader,
StepProgress) remain unchanged.
- Around line 420-429: The warnings UI (pending.warnings) is only rendered
inside the isFinalizing branch of V2WorkspaceLoadingView so warnings are lost
when syncTimedOut && !workspaceSynced flips and showKeypad becomes false; fix by
hoisting the warnings list out of the isFinalizing conditional (render it
alongside the main loading view) or by duplicating the same warnings list into
the sync-timeout/recovery branch that is shown when syncTimedOut &&
!workspaceSynced, referencing the existing pending.warnings map and the same
list markup used in the current isFinalizing block to ensure parity.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 522-541: The locally-declared HostProgressStep type can drift from
the actual tRPC query shape used by client.workspaceCreation.getProgress.query;
update the code so HostProgressStep is derived from the query return type
instead of being hand-rolled (or import the shared host-service type if
available). Specifically, replace the manual HostProgressStep declaration and
use a type like NonNullable<Awaited<ReturnType<typeof
client.workspaceCreation.getProgress.query>>>["steps"][number] (or the exported
host-service step type) and keep mapHostProgressToInitStep unchanged so it uses
the derived type for its steps parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8298e9e2-c0ef-4799-8a6b-ebc5a7679b3e
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
7 tasks
4 tasks
4 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
V2WorkspaceLoadingView) with copies ofKeypadLoaderandStepProgressthat drop the audio dep and accept a genericcurrentStepprop./v2-workspace-loading/$workspaceIdthat the v2 workspace layout redirects to while the v2Workspaces collection is hydrating; the loading page navigates back onceisReadyflips true.V2WorkspaceLoadingViewinto the pending workspace page (/pending/$pendingId) for bothcreatingand the briefsucceededwindow. Title varies by intent (Setting up workspace / Checking out branch / Adopting worktree); Dismiss button passes through aschildrenduring creation; warnings render below the keypad in the success frame.workspaceCreation.getProgresspolling for fork + checkout (mapper collapses the host's 3 steps onto the keypad's 5-key vocabulary). Onsucceededthe loader switches tocurrentStep="ready"so the last key reaches the pressed state — the host clears progress before flaggingregistering: done, so this transition is the only signal we have for that frame. Navigation fires immediately after success.Test plan
/v2-workspace-loading/$workspaceId, keypad shows briefly, page transitions back to the workspace once collections hydrate.