refactor(core): split createApp orchestration controllers#256
Merged
RtlZeroMemory merged 2 commits intomainfrom Mar 6, 2026
Merged
refactor(core): split createApp orchestration controllers#256RtlZeroMemory merged 2 commits intomainfrom
RtlZeroMemory merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRefactors createApp internals by extracting error handling, focus dispatch, dirty-flag management, and run-signal wiring into new modules ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
packages/core/src/app/createApp/topLevelViewError.ts (1)
62-62: Callout text omits theCtrl+Cquit path.The behavior supports quitting via Ctrl+C, but the prompt only advertises
R/Q. Consider making the instruction match actual behavior.🧩 Tiny text parity fix
- ui.callout("Press R to retry, Q to quit", { variant: "warning" }), + ui.callout("Press R to retry, Q or Ctrl+C to quit", { variant: "warning" }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/createApp/topLevelViewError.ts` at line 62, Update the callout text in topLevelViewError.ts where ui.callout is invoked (the line with ui.callout("Press R to retry, Q to quit", { variant: "warning" }) ) to reflect the actual quit options by including the Ctrl+C path (e.g., "Press R to retry, Q to quit or Ctrl+C to quit"); keep the same ui.callout invocation and variant but change only the displayed string so the prompt matches runtime behavior.packages/core/src/app/__tests__/runSignals.controller.test.ts (1)
4-61: Add a regression test for sync-throwingonSignal.Current coverage misses the case where
onSignalthrows synchronously. Add one test to assertcontroller.promisestill resolves and handlers are detached in that path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/runSignals.controller.test.ts` around lines 4 - 61, Add a test that simulates a sync-throwing onSignal to ensure createRunSignalController still resolves and detaches handlers: create a fake processLike with on/off and listeners Map (as in the first test), pass an onSignal that throws synchronously and an onDetached that records calls, verify controller.canRegisterSignals is true, trigger the registered signal handlers (e.g., call each handler for "SIGINT"), await controller.promise, then assert events include "detached" and that listeners for "SIGINT"/"SIGTERM" were removed; reference createRunSignalController, controller.promise, onSignal, onDetached, and the listeners Map to locate and adapt the existing tests.
🤖 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 1753-1764: app.start() may throw synchronously which currently
bypasses the promise rejection handler and leaves signal handlers registered;
wrap the call to app.start() in a try/catch so that synchronous throws also call
runController.detach() before rethrowing, preserving the existing behavior in
the async rejection branch (i.e., still rethrow the error after detaching), and
keep the existing logic that calls runController.settle() when
runController.canRegisterSignals is false in the successful path.
In `@packages/core/src/app/createApp/runSignals.ts`:
- Around line 89-97: The signal handler can let a synchronous throw from
options.onSignal escape before .catch/.finally attach; inside the handler (the
function named handler) replace Promise.resolve(options.onSignal()) with an
asynchronous initiation such as Promise.resolve().then(() => options.onSignal())
or explicitly capture synchronous throws via try/catch and convert to a rejected
promise, then chain .catch(() => undefined).finally(() => { resolvePromise();
}); ensure you keep the settled flag, detach(), and resolvePromise() behavior
intact so controller.promise is always resolved even if onSignal throws
synchronously.
In `@packages/core/src/app/createApp/topLevelViewError.ts`:
- Around line 6-11: Replace the exported alias TopLevelViewError (currently
declared as type Readonly<{ ... }>) with an exported interface TopLevelViewError
that defines the same properties as readonly (readonly code:
"ZRUI_USER_CODE_THROW"; readonly detail: string; readonly message: string;
readonly stack?: string), keeping the same export name and types so consumers
are unchanged; update any imports/usages if necessary to use the interface but
otherwise preserve semantics.
- Around line 95-102: The isUnhandledCtrlCKeyEvent predicate currently treats
Ctrl+Shift+C as the same as Ctrl+C; update the modifier check in
isUnhandledCtrlCKeyEvent to also exclude ZR_MOD_SHIFT so only plain Ctrl+C (no
Alt/Meta/Shift) returns true. Concretely, in the final return that currently
tests (ev.mods & (ZR_MOD_ALT | ZR_MOD_META)) === 0, include ZR_MOD_SHIFT in that
bitmask (e.g., (ZR_MOD_ALT | ZR_MOD_META | ZR_MOD_SHIFT)) and keep the earlier
CTRL presence check intact.
---
Nitpick comments:
In `@packages/core/src/app/__tests__/runSignals.controller.test.ts`:
- Around line 4-61: Add a test that simulates a sync-throwing onSignal to ensure
createRunSignalController still resolves and detaches handlers: create a fake
processLike with on/off and listeners Map (as in the first test), pass an
onSignal that throws synchronously and an onDetached that records calls, verify
controller.canRegisterSignals is true, trigger the registered signal handlers
(e.g., call each handler for "SIGINT"), await controller.promise, then assert
events include "detached" and that listeners for "SIGINT"/"SIGTERM" were
removed; reference createRunSignalController, controller.promise, onSignal,
onDetached, and the listeners Map to locate and adapt the existing tests.
In `@packages/core/src/app/createApp/topLevelViewError.ts`:
- Line 62: Update the callout text in topLevelViewError.ts where ui.callout is
invoked (the line with ui.callout("Press R to retry, Q to quit", { variant:
"warning" }) ) to reflect the actual quit options by including the Ctrl+C path
(e.g., "Press R to retry, Q to quit or Ctrl+C to quit"); keep the same
ui.callout invocation and variant but change only the displayed string so the
prompt matches runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ea91a6-7b94-42ff-8e3a-eee154dd3331
📒 Files selected for processing (8)
packages/core/src/app/__tests__/dirtyPlan.controller.test.tspackages/core/src/app/__tests__/focusDispatcher.controller.test.tspackages/core/src/app/__tests__/runSignals.controller.test.tspackages/core/src/app/createApp.tspackages/core/src/app/createApp/dirtyPlan.tspackages/core/src/app/createApp/focusDispatcher.tspackages/core/src/app/createApp/runSignals.tspackages/core/src/app/createApp/topLevelViewError.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/app/__tests__/topLevelViewError.test.ts (1)
2-3: Prefer shared keycode constant over raw67in this test.Using
KEY_Ckeeps this aligned with keycode definitions if mappings are ever centralized/adjusted.♻️ Proposed tweak
-import { ZR_MOD_CTRL, ZR_MOD_SHIFT } from "../../keybindings/keyCodes.js"; +import { KEY_C, ZR_MOD_CTRL, ZR_MOD_SHIFT } from "../../keybindings/keyCodes.js"; @@ - key: 67, + key: KEY_C, @@ - key: 67, + key: KEY_C,Also applies to: 10-11, 21-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/topLevelViewError.test.ts` around lines 2 - 3, Replace the hardcoded keycode 67 in the top-level view error test with the shared KEY_C constant: add an import for KEY_C from the keyCodes module alongside ZR_MOD_CTRL and ZR_MOD_SHIFT, then replace the raw 67 values in the isUnhandledCtrlCKeyEvent test event objects (the occurrences around the uses of ZR_MOD_CTRL/ZR_MOD_SHIFT and the Ctrl+C checks) with KEY_C so the test uses the centralized keycode constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/app/__tests__/topLevelViewError.test.ts`:
- Around line 2-3: Replace the hardcoded keycode 67 in the top-level view error
test with the shared KEY_C constant: add an import for KEY_C from the keyCodes
module alongside ZR_MOD_CTRL and ZR_MOD_SHIFT, then replace the raw 67 values in
the isUnhandledCtrlCKeyEvent test event objects (the occurrences around the uses
of ZR_MOD_CTRL/ZR_MOD_SHIFT and the Ctrl+C checks) with KEY_C so the test uses
the centralized keycode constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2f1758f-dca2-4497-85ab-03ea01009174
📒 Files selected for processing (6)
packages/core/src/app/__tests__/resilience.test.tspackages/core/src/app/__tests__/runSignals.controller.test.tspackages/core/src/app/__tests__/topLevelViewError.test.tspackages/core/src/app/createApp.tspackages/core/src/app/createApp/runSignals.tspackages/core/src/app/createApp/topLevelViewError.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/app/createApp/topLevelViewError.ts
- packages/core/src/app/createApp/runSignals.ts
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
createApp.tsinto its own controller moduleValidation
npm run buildnpm run lintnode scripts/run-tests.mjsSummary by CodeRabbit
Tests
Refactor
Bug Fixes / Behavior