feat(desktop): inline keypad loader on v2 workspace hydration#3821
Closed
saddlepaddle wants to merge 2 commits into
Closed
feat(desktop): inline keypad loader on v2 workspace hydration#3821saddlepaddle wants to merge 2 commits into
saddlepaddle wants to merge 2 commits into
Conversation
)" This reverts commit 95cff6b.
Replaces #3788's separate /v2-workspace-loading route with an inline render in v2-workspace/layout.tsx. The route + duplicate component tree were the source of the navigation oscillation crash: layout queried v2Workspaces + v2Hosts (joined isReady), the loading route queried only v2Workspaces, so the two effects ping-ponged when v2Hosts hydrated slower. Reuses the existing v1 KeypadLoader + StepProgress (presentational only; no v1 store coupling) instead of duplicating assets.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR removes the keypad-based workspace loading UI (KeypadLoader and StepProgress components) and the intermediate v2-workspace-loading route. The pending page now renders progress steps inline from the host progress object. The v2-workspace layout conditionally renders a simplified loading view instead of navigating away. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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 removes the dedicated Confidence Score: 5/5Safe to merge — a straightforward simplification with no correctness issues. All findings are P2 (style/efficiency): the setInterval running past max step is harmless (React bails on same-value state updates), and the workspaceName being unreachable during loading is a no-op since the component handles undefined gracefully. No data-integrity, routing, or runtime defects introduced. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Adds inline !isReady guard that renders V2WorkspaceLoadingView before the live query resolves; adds name to the select projection (unused during loading). Core logic is clean. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx | New lightweight loader component that reuses v1 KeypadLoader + StepProgress with a fake timer-driven step progression (400 ms/step). Timer runs past max step unnecessarily but causes no visible defect. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx | Removes stale V2WorkspaceLoadingView import and adds HiCheck for the "Workspace ready — opening..." success state. No logical changes to the creation/retry flow. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace-loading/$workspaceId/page.tsx | Entire route deleted (38 lines); replaced by the inline conditional in the v2-workspace layout. No remaining references to this route in the codebase. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User navigates to /v2-workspace/$workspaceId] --> B{workspaceId resolved?}
B -- No --> C[return null]
B -- Yes --> D{useLiveQuery isReady?}
D -- No --> E[V2WorkspaceLoadingView\nfake timer-driven KeypadLoader\nworkspaceName always undefined here]
D -- Yes --> F{workspace and hostUrl found?}
F -- No --> G[WorkspaceNotFoundState]
F -- Yes --> H[WorkspaceTrpcProvider + Outlet]
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/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx
Line: 24-28
Comment:
**Interval keeps firing after reaching max step**
Once `stepIdx` reaches `VISIBLE_STEPS.length - 1`, `Math.min` returns the same value on every tick, so the callback fires every 400 ms indefinitely but never produces a state change (React bails out). This is harmless in practice, but clearing the interval early would be cleaner.
```suggestion
useEffect(() => {
const id = window.setInterval(() => {
setStepIdx((prev) => {
const next = Math.min(prev + 1, VISIBLE_STEPS.length - 1);
if (next === VISIBLE_STEPS.length - 1) window.clearInterval(id);
return next;
});
}, STEP_INTERVAL_MS);
return () => window.clearInterval(id);
}, []);
```
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: 76-78
Comment:
**`workspaceName` will always be `undefined` during loading**
When `!isReady`, `useLiveQuery` hasn't resolved its first result, so `workspacesWithHost` is the default `[]`, making `workspace` null and `workspace?.name` always `undefined` at the point this branch is reached. The `name` field added to the `select` projection is effectively unreachable from this render path. The loader handles `undefined` gracefully, so there's no bug — but the workspace subtitle will never appear here unless `useLiveQuery` begins returning cached data before `isReady` becomes true.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(desktop): inline keypad loader on v..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
24
to
28
| useEffect(() => { | ||
| if (currentStepProp !== undefined) return; | ||
| const id = window.setInterval(() => { | ||
| setStepIdx((prev) => Math.min(prev + 1, VISIBLE_STEPS.length - 1)); | ||
| }, STEP_INTERVAL_MS); | ||
| return () => window.clearInterval(id); |
There was a problem hiding this comment.
Once stepIdx reaches VISIBLE_STEPS.length - 1, Math.min returns the same value on every tick, so the callback fires every 400 ms indefinitely but never produces a state change (React bails out). This is harmless in practice, but clearing the interval early would be cleaner.
Suggested change
| useEffect(() => { | |
| if (currentStepProp !== undefined) return; | |
| const id = window.setInterval(() => { | |
| setStepIdx((prev) => Math.min(prev + 1, VISIBLE_STEPS.length - 1)); | |
| }, STEP_INTERVAL_MS); | |
| return () => window.clearInterval(id); | |
| useEffect(() => { | |
| const id = window.setInterval(() => { | |
| setStepIdx((prev) => { | |
| const next = Math.min(prev + 1, VISIBLE_STEPS.length - 1); | |
| if (next === VISIBLE_STEPS.length - 1) window.clearInterval(id); | |
| return next; | |
| }); | |
| }, STEP_INTERVAL_MS); | |
| return () => window.clearInterval(id); | |
| }, []); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/V2WorkspaceLoadingView/V2WorkspaceLoadingView.tsx
Line: 24-28
Comment:
**Interval keeps firing after reaching max step**
Once `stepIdx` reaches `VISIBLE_STEPS.length - 1`, `Math.min` returns the same value on every tick, so the callback fires every 400 ms indefinitely but never produces a state change (React bails out). This is harmless in practice, but clearing the interval early would be cleaner.
```suggestion
useEffect(() => {
const id = window.setInterval(() => {
setStepIdx((prev) => {
const next = Math.min(prev + 1, VISIBLE_STEPS.length - 1);
if (next === VISIBLE_STEPS.length - 1) window.clearInterval(id);
return next;
});
}, STEP_INTERVAL_MS);
return () => window.clearInterval(id);
}, []);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+76
to
+78
| if (!isReady) { | ||
| return <V2WorkspaceLoadingView workspaceName={workspace?.name} />; | ||
| } |
There was a problem hiding this comment.
workspaceName will always be undefined during loading
When !isReady, useLiveQuery hasn't resolved its first result, so workspacesWithHost is the default [], making workspace null and workspace?.name always undefined at the point this branch is reached. The name field added to the select projection is effectively unreachable from this render path. The loader handles undefined gracefully, so there's no bug — but the workspace subtitle will never appear here unless useLiveQuery begins returning cached data before isReady becomes true.
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: 76-78
Comment:
**`workspaceName` will always be `undefined` during loading**
When `!isReady`, `useLiveQuery` hasn't resolved its first result, so `workspacesWithHost` is the default `[]`, making `workspace` null and `workspace?.name` always `undefined` at the point this branch is reached. The `name` field added to the `select` projection is effectively unreachable from this render path. The loader handles `undefined` gracefully, so there's no bug — but the workspace subtitle will never appear here unless `useLiveQuery` begins returning cached data before `isReady` becomes true.
How can I resolve this? If you propose a fix, please make it concise.
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.
Follow-up to #3820 (the revert).
Restores the keypad animation #3788 was after, but as a 5-line conditional render in
v2-workspace/layout.tsxinstead of a separate route + duplicate component tree.useLiveQuery/isReadyeverywhere — no asymmetric query, no cross-route navigation, no oscillation.KeypadLoader+StepProgress(purely presentational — no v1 store/trpc coupling).nameso the loader can show the workspace name.Test plan
WorkspaceNotFoundStateonceisReadySummary by cubic
Inlines the keypad loading screen in
v2-workspace/layout.tsxso v2 workspaces show the loader during hydration without navigating to a separate route. This removes the redirect loop caused by asymmetric queries and reuses the v1KeypadLoaderandStepProgress.Bug Fixes
useLiveQuery/isReadypath in the layout (no cross-route redirects).v2Hostshydrate on slow networks; no flicker or redirect loops.Refactors
/v2-workspace-loadingroute and all duplicated loader assets/components.nameto the layout query to show it in the inline loader.KeypadLoaderandStepProgressfromrenderer/screens/main/components/WorkspaceView/WorkspaceInitializingView/*.Written for commit 331fc12. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Refactor