refactor(core): split app runtime orchestration by concern#281
Merged
RtlZeroMemory merged 1 commit intomainfrom Mar 17, 2026
Merged
refactor(core): split app runtime orchestration by concern#281RtlZeroMemory merged 1 commit intomainfrom
RtlZeroMemory merged 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR refactors the app creation system by extracting configuration resolution, event loop, render loop, guard logic, keybinding utilities, and breadcrumb helpers into separate modules. The main Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
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 (1)
packages/core/src/app/createApp.ts (1)
375-394: KeepResolvedAppConfigas the only source of truth fordrawlistValidateParams.The new config module already resolves this flag, but Lines 378-392 still re-read
opts.config. Passingconfig.drawlistValidateParamsthrough here keeps the renderer wiring aligned withresolveAppConfig()and avoids silent drift if a renderer default changes later.♻️ Proposed cleanup
const rawRenderer = new RawRenderer({ backend, maxDrawlistBytes: config.maxDrawlistBytes, - ...(opts.config?.drawlistValidateParams === undefined - ? {} - : { drawlistValidateParams: opts.config.drawlistValidateParams }), + drawlistValidateParams: config.drawlistValidateParams, drawlistReuseOutputBuffer: config.drawlistReuseOutputBuffer, drawlistEncodedStringCacheCap: config.drawlistEncodedStringCacheCap, }); const widgetRenderer = new WidgetRenderer<S>({ backend, @@ rootPadding: config.rootPadding, breakpointThresholds: config.breakpointThresholds, terminalProfile, - ...(opts.config?.drawlistValidateParams === undefined - ? {} - : { drawlistValidateParams: opts.config.drawlistValidateParams }), + drawlistValidateParams: config.drawlistValidateParams, drawlistReuseOutputBuffer: config.drawlistReuseOutputBuffer, drawlistEncodedStringCacheCap: config.drawlistEncodedStringCacheCap,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/createApp.ts` around lines 375 - 394, The renderer constructors are reading drawlistValidateParams from opts.config instead of the resolved config, causing divergence from ResolvedAppConfig; update the RawRenderer and WidgetRenderer initializers (the new RawRenderer(...) and WidgetRenderer<S>(...) calls) to pass drawlistValidateParams: config.drawlistValidateParams rather than using the conditional ...(opts.config?.drawlistValidateParams === undefined ? {} : { drawlistValidateParams: opts.config.drawlistValidateParams }), so the renderers always receive the resolved value from resolveAppConfig().
🤖 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/config.ts`:
- Around line 82-85: The config currently accepts any positive integer for
maxDrawlistBytes; update the resolution so after calling requirePositiveInt (or
using DEFAULT_CONFIG.maxDrawlistBytes) you clamp the result to the drawlist
layer's supported max (2147483647) to avoid later render-time failures—i.e., in
resolveAppConfig/update in createApp/config.ts compute value =
config.maxDrawlistBytes === undefined ? DEFAULT_CONFIG.maxDrawlistBytes :
requirePositiveInt("maxDrawlistBytes", config.maxDrawlistBytes) and then set
maxDrawlistBytes = Math.min(value, 2147483647).
In `@packages/core/src/app/createApp/eventLoop.ts`:
- Around line 97-125: The catch in commitUpdates currently enqueues a fatal but
lets execution continue; change commitUpdates (the catch block for state
updaters) to immediately abort the turn after calling options.fatalNowOrEnqueue
by rethrowing the caught error (or throwing a new sentinel) so callers like
eventBatch/userCommit and processTurn observe the failure and stop further
rendering; ensure the finally block still runs (so keep
options.setInCommit(false) and perfMarkEnd in finally) and only add the throw
inside the catch in commitUpdates to propagate the failure outwards.
In `@packages/core/src/app/createApp/renderLoop.ts`:
- Around line 327-341: The post-submit callbacks (focus/onRender/onLayout) may
throw or return early leaving res.inFlight unobserved; move the call to
scheduleFrameSettlement(res.inFlight, submitStartMs, buildEndMs) so it runs
immediately after updating frames-in-flight and interactive budget (i.e., right
after options.setFramesInFlight(...) and options.setInteractiveBudget(...)) and
before invoking emitFocusChangeIfNeeded(), emitInternalRenderMetrics(), and
emitInternalLayoutSnapshot() in the submitFrame handling path (and apply the
same reordering in the widget branch handled around the other submitFrame branch
noted in the review).
- Around line 325-333: The render timing uses perfNow() (which is 0 when
PERF_ENABLED is off) to compute renderTime, so update the render time
measurement to use monotonicNowMs() for public metrics while keeping
perfNow()/perfRecord()/perfMarkStart()/perfMarkEnd() around submit_frame for
optional profiling; specifically, replace the renderStart = perfNow() used
before calling options.rawRenderer.submitFrame(...) with renderStart =
monotonicNowMs(), and pass the monotonic delta (monotonicNowMs() - renderStart)
into emitInternalRenderMetrics so renderTime/renderTimeMs are always populated,
leaving perfMarkStart("submit_frame")/perfMarkEnd("submit_frame") and any
perfRecord usage intact for profiling; apply the same change in the other block
referenced (around emitInternalRenderMetrics later in the file).
---
Nitpick comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 375-394: The renderer constructors are reading
drawlistValidateParams from opts.config instead of the resolved config, causing
divergence from ResolvedAppConfig; update the RawRenderer and WidgetRenderer
initializers (the new RawRenderer(...) and WidgetRenderer<S>(...) calls) to pass
drawlistValidateParams: config.drawlistValidateParams rather than using the
conditional ...(opts.config?.drawlistValidateParams === undefined ? {} : {
drawlistValidateParams: opts.config.drawlistValidateParams }), so the renderers
always receive the resolved value from resolveAppConfig().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a8b0bd5-7017-43f5-81d3-cb8c5e573ff4
📒 Files selected for processing (7)
packages/core/src/app/createApp.tspackages/core/src/app/createApp/breadcrumbs.tspackages/core/src/app/createApp/config.tspackages/core/src/app/createApp/eventLoop.tspackages/core/src/app/createApp/guards.tspackages/core/src/app/createApp/keybindings.tspackages/core/src/app/createApp/renderLoop.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
packages/core/src/app/createApp.tsinto internal helper modules.Why
Validation
npm run lintnpm run typechecknpm run buildnode scripts/run-tests.mjs --filter "packages/core/dist/app/__tests__/"node scripts/run-tests.mjs --filter "packages/core/dist/__tests__/integration/"node scripts/run-tests.mjsPTY / frame-audit evidence
npm run build:nativeso the PTY app could start in this environment.68x300, then resized to40x120during the run.t23) while rendering was active.node scripts/frame-audit-report.mjs /tmp/rezi-frame-audit.ndjson --latest-pid22585:records=4969,backend_submitted=236,worker_payload=236,worker_accepted=236,worker_completed=236,hash_mismatch_backend_vs_worker=0.bridge submitted=196/completed=196,engineering 26/26,crew 11/11,settings 3/3.120x40, theme cycle, and quit handling in order.Summary by CodeRabbit
Release Notes
resolveAppConfigutility for explicit application configuration handling.