fix(core): stop app on unhandled q and ctrl+c#238
Merged
RtlZeroMemory merged 6 commits intomainfrom Mar 4, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughUnhandled top-level Changes
Sequence DiagramsequenceDiagram
participant User
participant EventLoop as Event Loop
participant Router as Widget Router
participant QuitDetector as Quit Detector
participant App as App Instance
User->>EventLoop: Input (text 'q'/'Q' or key Ctrl+C)
EventLoop->>Router: Route event to widgets/keybindings
Router->>QuitDetector: Was event handled / needsRender?
alt Event consumed by widget/keybinding
QuitDetector-->>Router: consumed (no quit)
Router->>App: Execute routed handler
else Unhandled quit condition detected
QuitDetector-->>App: stopFromUnhandledQuitEvent()
App->>App: stop()/dispose() (graceful shutdown)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 442-458: The isUnmodifiedTextQuitEvent function's multi-line
return should be consolidated into a single expression to satisfy the formatter:
replace the current multi-line return with one-line boolean expression that
checks ev.codepoint against KEY_Q, KEY_LOWER_Q and CTRL_C_CODEPOINT (e.g. return
ev.codepoint === KEY_Q || ev.codepoint === KEY_LOWER_Q || ev.codepoint ===
CTRL_C_CODEPOINT;), leaving the existing early kind check intact.
- Around line 1343-1351: Remove the unnecessary "continue;" at the end of the
for-loop branch inside createApp (the block that checks routed.action,
routed.needsRender, isUnmodifiedTextQuitEvent(ev) ||
isUnhandledCtrlCKeyEvent(ev)); specifically delete the trailing continue after
noteBreadcrumbConsumptionPath("widgetRouting") and stopFromUnhandledQuitEvent()
since control will naturally proceed to the next iteration—no other control-flow
changes are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 205cacad-a7d6-46a2-bb7c-f206df6ab708
📒 Files selected for processing (3)
CHANGELOG.mdpackages/core/src/app/__tests__/resilience.test.tspackages/core/src/app/createApp.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d821e5eb01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 963-969: The stopFromUnhandledQuitEvent function currently calls
void app.stop() which drops any promise rejection; change it to capture the
Promise from app.stop() (e.g., const p = app.stop()), wrap that call in
try/catch to handle synchronous throws, and attach .catch(...) to handle async
rejections and .finally(...) if needed for post-stop cleanup—use the same
pattern as quitFromTopLevelViewError to ensure any rejection from app.stop() is
observed and handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15a5fc9d-fe3b-4620-b9fc-640387aabb80
📒 Files selected for processing (1)
packages/core/src/app/createApp.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/app/__tests__/resilience.test.ts (1)
182-207: Split Ctrl+C key and text assertions into separate tests.This test currently depends on stop/start timing in the middle of the same case. Using separate app instances (or two tests) will reduce flake risk and make failures more local.
Proposed refactor
-test("unhandled ctrl+c key/text events stop app by default", async () => { +test("unhandled ctrl+c key event stops app by default", async () => { const backend = new StubBackend(); const app = createApp({ backend, initialState: {} }); app.view(() => ui.text("hello")); await app.start(); await emitResize(backend, 1); await settleNextFrame(backend); await pushEvents(backend, [ { kind: "key", timeMs: 3, key: 67, mods: ZR_MOD_CTRL, action: "down" }, ]); await flushMicrotasks(30); assert.equal(backend.stopCalls >= 1, true); +}); - await app.start(); +test("unhandled ctrl+c text event stops app by default", async () => { + const backend = new StubBackend(); + const app = createApp({ backend, initialState: {} }); + + app.view(() => ui.text("hello")); + + await app.start(); await emitResize(backend, 5); await settleNextFrame(backend); await pushEvents(backend, [{ kind: "text", timeMs: 6, codepoint: 3 }]); await flushMicrotasks(30); - assert.equal(backend.stopCalls >= 2, true); + assert.equal(backend.stopCalls >= 1, true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/resilience.test.ts` around lines 182 - 207, The test "unhandled ctrl+c key/text events stop app by default" mixes two separate behaviors and restarts the same app mid-test, causing timing flakiness; split into two separate tests (e.g., "unhandled ctrl+c key event stops app" and "unhandled ctrl+c text event stops app") each creating its own StubBackend and app via createApp, registering the view, starting the app, emitting resize/settle, pushing the appropriate single event with pushEvents (the key event with key=67 and mods=ZR_MOD_CTRL for the key test, and the text event with codepoint=3 for the text test), flushing microtasks, and asserting backend.stopCalls >= 1; remove the second app.start() reuse and ensure each test fully isolates lifecycle and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 963-970: The stopFromUnhandledQuitEvent function is currently
swallowing synchronous errors from app.stop(); change the catch block to capture
the error (e.g., catch (err)) and do not return silently: log the error (using
the existing logger or processLogger) and rethrow or assign stopPromise =
Promise.reject(err) so the failure surfaces as a fatal condition instead of
dropping the quit intent; ensure stopPromise is always set (either to the
returned promise or a rejected promise) and remove the early return so upstream
handlers can observe the failure.
---
Nitpick comments:
In `@packages/core/src/app/__tests__/resilience.test.ts`:
- Around line 182-207: The test "unhandled ctrl+c key/text events stop app by
default" mixes two separate behaviors and restarts the same app mid-test,
causing timing flakiness; split into two separate tests (e.g., "unhandled ctrl+c
key event stops app" and "unhandled ctrl+c text event stops app") each creating
its own StubBackend and app via createApp, registering the view, starting the
app, emitting resize/settle, pushing the appropriate single event with
pushEvents (the key event with key=67 and mods=ZR_MOD_CTRL for the key test, and
the text event with codepoint=3 for the text test), flushing microtasks, and
asserting backend.stopCalls >= 1; remove the second app.start() reuse and ensure
each test fully isolates lifecycle and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bb3d66d-9e37-49b4-a7e5-afa4ad746a73
📒 Files selected for processing (4)
packages/core/src/app/__tests__/resilience.test.tspackages/core/src/app/createApp.tspackages/core/src/app/widgetRenderer.tspackages/core/src/app/widgetRenderer/inputEditing.ts
Closed
This was referenced Mar 6, 2026
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
q/Qtext, rawCtrl+Ctext, and unhandledCtrl+Ckey events)Qhandling remains unchangedValidation
npm -w @rezi-ui/core run buildnode --test --test-concurrency=1 packages/core/dist/app/__tests__/resilience.test.jsnode scripts/run-tests.mjs --scope packages --filter "packages/core/dist/app/__tests__/(resilience|shortcutEnforcement|keybindings.api)\.test\.js$"Fixes #233
Summary by CodeRabbit
Bug Fixes
Tests
Documentation