fix resizing issue#112
Open
groninge01 wants to merge 4 commits into
Open
Conversation
📝 WalkthroughUI/Chat
WalkthroughTerminal resize detection is extracted from inline CLI startup code into a dedicated ChangesTerminal resize handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.tsx`:
- Line 417: The call to setupTerminalResize(r) returns a cleanup function that
is currently discarded; capture that return (e.g., const terminalResizeCleanup =
setupTerminalResize(r)) and then invoke terminalResizeCleanup() inside
runCleanup() before calling restoreTerminal(); this ensures event listeners are
removed, the polling interval cleared, and DEC mode 2048 disabled without
leaking or duplicating cleanup logic from restoreTerminal().
In `@tests/terminal-resize.test.ts`:
- Around line 59-65: The test's process mock doesn't replace module-level
references because resize-handler.ts captures process.stdout on import; update
the code so setupTerminalResize (in resize-handler.ts) accepts a stdout/process
argument (e.g., add an optional parameter like stdout or process) and use that
value instead of referencing process.stdout directly, then change the tests to
pass the mockProcess/stdout into setupTerminalResize; alternatively, if you
prefer not to change production API, change the tests to dynamically import the
module after installing the mock (use await import('./resize-handler') in
beforeEach) so the module captures the mocked process—refer to
setupTerminalResize and resize-handler.ts when making the change.
- Around line 30-40: Replace the multiple `: any` test stubs with properly typed
mocks: define explicit mock interfaces (e.g., MockProcess and MockStdout) that
declare only the properties/methods your tests use, then change the declarations
for mockProcess, mockStdout, origProcessStdout, origProcessOn, and
origProcessRemoveListener to those types (or to `unknown` plus narrow with small
type-guard functions before use); remove the `: any` annotations and the
biome-ignore comments, and update any usages to use the mock interfaces or the
type-guarded values so the test compiles under strict mode and Bun's test
runtime.
- Line 1: Replace Vitest imports and APIs with Bun's test runner and mocks:
change the top-level import to use "bun:test" (describe, test, expect,
beforeEach, afterEach) and remove any Vitest-specific import of vi; replace all
vi.fn() usages with Bun.mock() or simple manual stubs, and replace
vi.useFakeTimers()/vi.clearAllTimers()/vi.advanceTimersByTime() with Bun's timer
controls or manual timer stubbing; also replace
vi.restoreAllMocks()/vi.resetAllMocks() with equivalent manual cleanup in
beforeEach/afterEach. Locate and update occurrences of vi.* in this file (e.g.,
any calls to vi.fn, vi.useFakeTimers, vi.restoreAllMocks, vi.clearAllTimers,
vi.advanceTimersByTime) and ensure tests run under Bun by using Bun-compatible
mocks and timer management.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1c804fe-2a1d-4f7b-9601-1582f90fd22e
📒 Files selected for processing (3)
src/core/terminal/resize-handler.tssrc/index.tsxtests/terminal-resize.test.ts
| } catch {} | ||
| }, 1000); | ||
| resizePoll.unref?.(); | ||
| setupTerminalResize(r); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Store and invoke the cleanup function.
src/index.tsx:417 — setupTerminalResize(r) returns a cleanup function that removes event listeners, clears the polling interval, and disables DEC mode 2048. Return value is discarded, so listeners accumulate and the interval continues until process exit. Violates the function contract and duplicates the DEC mode disable logic already in restoreTerminal() line 46.
Store the cleanup function and call it in runCleanup() before restoreTerminal().
Proposed fix
+let cleanupTerminalResize: (() => void) | null = null;
+
export async function start(opts: StartOptions): Promise<void> {
const r = await opts.createCliRenderer({
// ... config ...
});
if (process.stdout.isTTY) {
- setupTerminalResize(r);
+ cleanupTerminalResize = setupTerminalResize(r);
}
// ... rest of start function ...
}Then in runCleanup() after line 54:
function runCleanup(): void {
if (cleanedUp) return;
cleanedUp = true;
+ try {
+ cleanupTerminalResize?.();
+ } catch {}
restoreTerminal();
// ... rest of cleanup ...
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.tsx` at line 417, The call to setupTerminalResize(r) returns a
cleanup function that is currently discarded; capture that return (e.g., const
terminalResizeCleanup = setupTerminalResize(r)) and then invoke
terminalResizeCleanup() inside runCleanup() before calling restoreTerminal();
this ensures event listeners are removed, the polling interval cleared, and DEC
mode 2048 disabled without leaking or duplicating cleanup logic from
restoreTerminal().
| @@ -0,0 +1,192 @@ | |||
| import { describe, test, expect, vi, beforeEach, afterEach } from "vitest"; | |||
There was a problem hiding this comment.
Reject: tests must use Bun's native runner, not vitest.
tests/** requires import { describe, expect, test } from "bun:test" per CONTRIBUTING.md. Vitest imports will fail CI.
Bun provides mock() for spies. Rework needed to replace vi.fn(), vi.useFakeTimers(), etc. with Bun equivalents or manual stubs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/terminal-resize.test.ts` at line 1, Replace Vitest imports and APIs
with Bun's test runner and mocks: change the top-level import to use "bun:test"
(describe, test, expect, beforeEach, afterEach) and remove any Vitest-specific
import of vi; replace all vi.fn() usages with Bun.mock() or simple manual stubs,
and replace vi.useFakeTimers()/vi.clearAllTimers()/vi.advanceTimersByTime() with
Bun's timer controls or manual timer stubbing; also replace
vi.restoreAllMocks()/vi.resetAllMocks() with equivalent manual cleanup in
beforeEach/afterEach. Locate and update occurrences of vi.* in this file (e.g.,
any calls to vi.fn, vi.useFakeTimers, vi.restoreAllMocks, vi.clearAllTimers,
vi.advanceTimersByTime) and ensure tests run under Bun by using Bun-compatible
mocks and timer management.
Source: Coding guidelines
Comment on lines
+30
to
+40
| // biome-ignore lint/suspicious/noExplicitAny: test-only process stub | ||
| let mockProcess: any; | ||
| // biome-ignore lint/suspicious/noExplicitAny: test-only stdout stub | ||
| let mockStdout: any; | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: test-only global replacement | ||
| let origProcessStdout: any; | ||
| // biome-ignore lint/suspicious/noExplicitAny: test-only global replacement | ||
| let origProcessOn: any; | ||
| // biome-ignore lint/suspicious/noExplicitAny: test-only global replacement | ||
| let origProcessRemoveListener: any; |
There was a problem hiding this comment.
Multiple : any annotations violate strict mode.
CONTRIBUTING.md: "reject : any and as any (strict mode — use Zod inference or proper types)". biome-ignore comments don't override repo policy. Use unknown with type guards or define explicit mock types.
Since this file needs rework for bun:test anyway, address both together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/terminal-resize.test.ts` around lines 30 - 40, Replace the multiple `:
any` test stubs with properly typed mocks: define explicit mock interfaces
(e.g., MockProcess and MockStdout) that declare only the properties/methods your
tests use, then change the declarations for mockProcess, mockStdout,
origProcessStdout, origProcessOn, and origProcessRemoveListener to those types
(or to `unknown` plus narrow with small type-guard functions before use); remove
the `: any` annotations and the biome-ignore comments, and update any usages to
use the mock interfaces or the type-guarded values so the test compiles under
strict mode and Bun's test runtime.
Source: Coding guidelines
Comment on lines
+59
to
+65
| origProcessStdout = process.stdout; | ||
| origProcessOn = process.on; | ||
| origProcessRemoveListener = process.removeListener; | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: test-only process stub | ||
| (globalThis as any).process = mockProcess; | ||
| }); |
There was a problem hiding this comment.
Process mocking won't intercept module-level references.
resize-handler.ts accesses process.stdout directly. By the time beforeEach replaces globalThis.process, the module has already imported and cached the real process reference. The mock won't be used.
Either:
- Use dependency injection (pass
stdout/processtosetupTerminalResize) - Use Bun's module mocking if available
- Re-import the module after mocking with dynamic
import()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/terminal-resize.test.ts` around lines 59 - 65, The test's process mock
doesn't replace module-level references because resize-handler.ts captures
process.stdout on import; update the code so setupTerminalResize (in
resize-handler.ts) accepts a stdout/process argument (e.g., add an optional
parameter like stdout or process) and use that value instead of referencing
process.stdout directly, then change the tests to pass the mockProcess/stdout
into setupTerminalResize; alternatively, if you prefer not to change production
API, change the tests to dynamically import the module after installing the mock
(use await import('./resize-handler') in beforeEach) so the module captures the
mocked process—refer to setupTerminalResize and resize-handler.ts when making
the change.
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.
What
fixes #91
Why
see #91
Scope check
CONTRIBUTING.mdand this PR is in the In scope table.bun run lint:fix && bun run format && bun run typecheckpasses.Verification