fix(terminal): use buffered write drain scheduler with visibility fallback#1547
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/renderer/writeDrainScheduler.test.ts (1)
42-94: Good test coverage for core scenarios.The tests correctly verify the three main paths: RAF when visible, timeout when hidden, and timeout fallback when RAF stalls. The third test also validates that
cancelAnimationFrameis called with the correct handle during cleanup.Consider adding a test for the cancellation contract (calling the returned function before execution should prevent
run()from firing). This would validate the disposal path used byTerminalSessionManager.💡 Optional: Add cancellation test
+ it('cancellation prevents run from executing', () => { + let frameCallback: ((time: number) => void) | null = null; + const run = vi.fn(); + + globals.document = { visibilityState: 'visible' } as Document; + globals.requestAnimationFrame = vi.fn((callback: (time: number) => void) => { + frameCallback = callback; + return 1; + }); + globals.cancelAnimationFrame = vi.fn(); + + const cancel = scheduleTerminalWriteDrain(run); + + cancel(); + + expect(globals.cancelAnimationFrame).toHaveBeenCalledWith(1); + + // Simulate RAF firing after cancellation + frameCallback?.(16); + vi.runAllTimers(); + + expect(run).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/renderer/writeDrainScheduler.test.ts` around lines 42 - 94, Add a new test for scheduleTerminalWriteDrain that verifies the cancellation contract: call scheduleTerminalWriteDrain(run) and immediately invoke the returned cancel function, then advance timers and/or trigger the stored RAF callback (depending on visibility) and assert that run was not called and that cancelAnimationFrame was invoked with the RAF handle when applicable; reference scheduleTerminalWriteDrain, globals.requestAnimationFrame, globals.cancelAnimationFrame and the returned cancel function to locate the code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/renderer/writeDrainScheduler.test.ts`:
- Around line 42-94: Add a new test for scheduleTerminalWriteDrain that verifies
the cancellation contract: call scheduleTerminalWriteDrain(run) and immediately
invoke the returned cancel function, then advance timers and/or trigger the
stored RAF callback (depending on visibility) and assert that run was not called
and that cancelAnimationFrame was invoked with the RAF handle when applicable;
reference scheduleTerminalWriteDrain, globals.requestAnimationFrame,
globals.cancelAnimationFrame and the returned cancel function to locate the code
to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1213744e-4e7c-4801-aaf6-f2d7f7a53c0c
📒 Files selected for processing (3)
src/renderer/terminal/TerminalSessionManager.tssrc/renderer/terminal/writeDrainScheduler.tssrc/test/renderer/writeDrainScheduler.test.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
fixes #1531 |
Summary
Replaces the raw
requestAnimationFramecall inTerminalSessionManager.scheduleWriteDrain()with a newscheduleTerminalWriteDrainutility that handles background-tab scenarios.Problem
When the app tab is hidden,
requestAnimationFramecallbacks are throttled or paused by the browser, causing terminal write drains to stall indefinitely.Changes
writeDrainScheduler.ts— schedules drains viarequestAnimationFramewhen visible, with asetTimeoutfallback (48ms) to guarantee delivery even when throttled. Falls back tosetTimeout(0)when the document is hidden.TerminalSessionManagerto use the new scheduler and track acancelPendingWriteDrainhandle, ensuring pending drains are properly cancelled on dispose/reset.Fixes #1510
Summary by CodeRabbit
Refactor
Tests