Repository navigation
fix(desktop): retry terminal repaint after focus restore - #3322
surlymochan wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a "recovery burst" to terminal reattachment: on visibility/focus restore it runs recovery immediately and schedules two follow-up recovery attempts (~120ms and ~260ms), and it tracks/clears those timeout IDs during cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Window
participant TerminalHook
participant Scheduler
participant RecoveryCallback
Window->>TerminalHook: visibilitychange / focus
TerminalHook->>Scheduler: scheduleRecoveryBurst(forceResize)
Scheduler->>RecoveryCallback: invoke immediate reattachRecovery
note right of Scheduler: schedule timeouts (120ms, 260ms)
Scheduler->>Scheduler: store timeout IDs in burstTimeouts
alt not unmounted after 120ms
Scheduler->>RecoveryCallback: invoke reattachRecovery (120ms)
end
alt not unmounted after 260ms
Scheduler->>RecoveryCallback: invoke reattachRecovery (260ms)
end
Window->>TerminalHook: unmount/cleanup
TerminalHook->>Scheduler: clear stored burstTimeouts (clearTimeout) and reset array
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 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 addresses a lingering blank-terminal regression after focus/visibility restore by adding a short "recovery burst": after the immediate
Confidence Score: 4/5Safe to merge after fixing the TypeScript return-type annotation in the test file; the production implementation change is small, well-isolated, and properly cleaned up. The implementation change is minimal and self-contained — a burst of two extra setTimeout calls with proper cleanup. The only concrete issue is a TypeScript type error in the test helper (scheduleBurst not declared in the return type), which would fail typecheck CI. Once that one-liner annotation fix is in, the PR is ready. useTerminalLifecycle.test.ts — return-type annotation for Important Files Changed
Sequence DiagramsequenceDiagram
participant W as window / document
participant H as handleWindowFocus / handleVisibilityChange
participant B as scheduleRecoveryBurst
participant S as scheduleReattachRecovery
participant R as runReattachRecovery (xterm)
W->>H: focus / visibilitychange
H->>B: scheduleRecoveryBurst(forceResize)
B->>S: immediate call
S->>S: requestAnimationFrame
S->>R: runReattachRecovery() [T=0]
Note over R: clearTextureAtlas + fit + refresh
B->>B: setTimeout 120ms
B->>B: setTimeout 260ms
Note over S,R: T = 120ms
B-->>S: scheduleReattachRecovery()
S->>S: requestAnimationFrame
S->>R: runReattachRecovery() [T≈120ms]
Note over S,R: T = 260ms
B-->>S: scheduleReattachRecovery()
S->>S: requestAnimationFrame
S->>R: runReattachRecovery() [T≈260ms]
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts (1)
40-44:⚠️ Potential issue | 🟠 MajorExpose
scheduleBurstinmakeScheduler's return type.The function returns an object with
scheduleBurstat line 113, but the declared return type at lines 40-44 does not include it. This causes a TypeScript error when the test at line 191 destructuresscheduleBurst.🛠️ Proposed fix
function makeScheduler(runRecovery: (forceResize: boolean) => void): { schedule: (forceResize: boolean) => void; + scheduleBurst: (forceResize: boolean) => void; flush: () => void; state: SchedulerState; } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts` around lines 40 - 44, The declared return type of makeScheduler is missing the scheduleBurst method, causing a TypeScript error when the test destructures scheduleBurst; update the return type of makeScheduler to include scheduleBurst: (forceResize: boolean, burstMs?: number) => void (matching the implementation) so callers like the test at line ~191 can destructure scheduleBurst without type errors, and ensure any SchedulerState reference remains unchanged.
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.ts`:
- Around line 40-44: The declared return type of makeScheduler is missing the
scheduleBurst method, causing a TypeScript error when the test destructures
scheduleBurst; update the return type of makeScheduler to include scheduleBurst:
(forceResize: boolean, burstMs?: number) => void (matching the implementation)
so callers like the test at line ~191 can destructure scheduleBurst without type
errors, and ensure any SchedulerState reference remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46ebaf8f-6d1d-43f7-b902-8a4056ea9e7d
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
|
Follow-up update pushed to this PR. I addressed the concrete review items in the regression test:
Validation run locally:
I kept the PR scope limited to the repaint/focus-restore bug in #3321. The other downstream fixes I found while validating the patched build are being split into separate upstream candidates so this PR stays reviewable. |
|
i have another repro in the same family here: #3504 on my mac, i can also get terminal content ghosting into the chat view, not only blank space inside the terminal. when i toggle fullscreen, it fixes the rendering right away. that makes me think the repaint recovery path in this PR is very relevant for my case too. i am closing my draft PR #3506 to avoid overlap. if this PR does not fully cover the chat ghosting and fullscreen case, i can send a smaller follow up later. |
Summary
Why
The existing terminal restore path already does the right kinds of work on refocus:
The remaining problem is timing. In practice, one refocus recovery can still happen slightly before the terminal container has fully settled after the app/window/workspace becomes visible again. That leaves stale blank space or stale terminal/TUI styling until some later repaint happens.
Running a short recovery burst fixed the repro locally:
This keeps the change small and local to the focus/visibility recovery path, while giving xterm another chance to repaint once layout has actually stabilized.
Test plan
bun test ./src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.tsRelated
Summary by CodeRabbit
Bug Fixes
Tests