fix(core): harden runtime lifecycle and layer routing#265
Merged
RtlZeroMemory merged 6 commits intomainfrom Mar 6, 2026
Merged
fix(core): harden runtime lifecycle and layer routing#265RtlZeroMemory merged 6 commits intomainfrom
RtlZeroMemory merged 6 commits intomainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens app lifecycle and render coalescing, centralizes thrown-value formatting, enforces unique focus-container IDs, changes top-layer-only Escape routing semantics, hardens widget-renderer focus/scroll/layout behavior, makes local-state snapshots immutable, and adds extensive tests across lifecycle, routing, perf, and renderer paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as App API
participant Scheduler as Lifecycle Scheduler
participant Backend as Backend
participant Renderer as Renderer
participant FatalSink as Fatal Queue
Client->>Scheduler: start()/emit(event)
Scheduler->>Backend: start() (guarded by lifecycleGeneration)
Backend-->>Scheduler: started / start-failed
alt start aborted or failed
Scheduler->>Backend: cleanupStartedBackendAfterAbort()
Scheduler->>FatalSink: fatalNowOrEnqueue(fatal)
else started
Scheduler->>Renderer: scheduleRender() (coalesced / deferred)
Renderer->>Scheduler: commit/render (may schedule follow-up)
alt user-code throws in event/commit/render
Renderer->>FatalSink: fatalNowOrEnqueue(fatal)
Scheduler->>Backend: stop()/dispose() (teardown)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: fef7139e0c
ℹ️ 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".
| rectById: this.rectById, | ||
| scrollOverrides: this.scrollOverrides, | ||
| findNearestScrollableAncestor: (targetId) => this.findNearestScrollableAncestor(targetId), | ||
| findScrollableAncestors: (targetId) => this.findScrollableAncestors(targetId), |
There was a problem hiding this comment.
routeMouseWheel still reads ctx.findNearestScrollableAncestor (see packages/core/src/app/widgetRenderer/mouseRouting.ts), but this call site now passes the callback as findScrollableAncestors, so the wheel fallback path will call undefined and throw on mouse-wheel events that reach generic scroll-container routing. In production this turns a normal wheel input into a fatal widget routing threw error instead of scrolling.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/core/src/app/widgetRenderer.ts (2)
2389-2402:⚠️ Potential issue | 🟠 MajorGuard focus-zone error reporting against sink failures.
At Line 2390 and Line 2401,
reportUserCodeError(...)is called inside catch blocks without a guard. If the sink throws, routing can fail while handling an already-caught widget callback error.🛡️ Suggested hardening
- } catch (error: unknown) { - this.reportUserCodeError(`focusZone onExit threw: ${describeThrown(error)}`); + } catch (error: unknown) { + this.reportFocusZoneCallbackError("onExit", error); } @@ - } catch (error: unknown) { - this.reportUserCodeError(`focusZone onEnter threw: ${describeThrown(error)}`); + } catch (error: unknown) { + this.reportFocusZoneCallbackError("onEnter", error); }private reportFocusZoneCallbackError(phase: "onEnter" | "onExit", error: unknown): void { const detail = `focusZone ${phase} threw: ${describeThrown(error)}`; try { this.reportUserCodeError(detail); } catch (sinkError: unknown) { const c = (globalThis as { console?: { error?: (message: string) => void } }).console; c?.error?.( `[rezi][runtime] onUserCodeError sink threw while reporting focusZone ${phase}: ${describeThrown( sinkError, )}; original=${detail}`, ); } }As per coding guidelines:
**/*.ts: All code changes must comply with docs/dev/code-standards.md including TypeScript strictness, runtime/layout/reconciliation invariants, and callback safety with async cancellation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2389 - 2402, The catch blocks that call reportUserCodeError directly (around the focus-zone onExit and onEnter handlers) must be hardened so a failing sink cannot bubble while handling an already-caught widget error; add a helper method (e.g., reportFocusZoneCallbackError(phase: "onEnter" | "onExit", error: unknown)) that builds the detail string using describeThrown(error), calls this.reportUserCodeError(detail) inside its own try/catch, and on sink failure logs a safe fallback to globalThis.console.error including the sink error (via describeThrown) and the original detail; replace the two direct reportUserCodeError(...) calls with calls to reportFocusZoneCallbackError("onExit", error) and reportFocusZoneCallbackError("onEnter", error).
2125-2141:⚠️ Potential issue | 🔴 CriticalBuild-blocking wheel-routing context mismatch at line 2140: property name and return type incompatible.
The call passes
findScrollableAncestors(plural, returns array) butRouteMouseWheelContextexpectsfindNearestScrollableAncestor(singular, returns single object or null). This is a TypeScript compile error (TS2561 type incompatibility).Fix
- findScrollableAncestors: (targetId) => this.findScrollableAncestors(targetId), + findNearestScrollableAncestor: (targetId: string | null) => + this.findScrollableAncestors(targetId)[0] ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2125 - 2141, The routeMouseWheel call is passing this.findScrollableAncestors (plural, returns an array) but RouteMouseWheelContext expects findNearestScrollableAncestor (singular, returns one element or null); fix by passing a wrapper named findNearestScrollableAncestor that calls this.findScrollableAncestors(targetId) and returns the first result or null, and supply that wrapper in the routeMouseWheel context (ensure the property key is findNearestScrollableAncestor), so update the object passed to routeMouseWheel accordingly (referencing routeMouseWheel, RouteMouseWheelContext, findScrollableAncestors, and findNearestScrollableAncestor).packages/core/src/runtime/commit.ts (1)
1972-1980:⚠️ Potential issue | 🟡 MinorApply formatter output in the commit path block
CI reports formatting drift in this segment; please apply formatter output to unblock checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/commit.ts` around lines 1972 - 1980, The code in the commit path around ensureInteractiveId, ensureFocusContainerId and the lifecycle lists (ctx.collectLifecycleInstanceIds with ctx.lists.reused and ctx.lists.mounted) has formatting drift; run and apply the project's formatter (prettier/clang-format/ruff as used by the repo) to this block so spacing/indentation/braces match CI expectations, ensuring the if/else and return lines for idFatal, focusContainerFatal and the prev branch are formatted consistently with the rest of the file.packages/core/src/app/createApp.ts (2)
25-59:⚠️ Potential issue | 🟡 MinorResolve formatter/import-order CI failures in this file
CI is still failing on import ordering and formatting for this segment; please apply formatter output so checks pass.
Also applies to: 691-696
🤖 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 25 - 59, The imports in createApp.ts are mis-ordered/formatted and causing CI failures; reorder and reformat this import block to match the project's import-style rules (group and sort imports consistently and keep type-only imports together), then run the project formatter/linter (e.g. prettier/eslint --fix or the repo's format script) to apply exact formatting. Focus on the listed symbols to locate the block: ZrUiError/ZrUiErrorCode, BACKEND_* markers and BackendEventBatch/RuntimeBackend, UiEvent/ZrevEvent, BindingMap/KeyContext/KeybindingManagerState/ModeBindingMap, the keybinding helpers (createManagerState/getBindings/getMode/getPendingChord/registerBindings/registerModes/resetChordState/routeKeyEvent/setMode), ZR_MOD_CTRL, ResponsiveBreakpointThresholds/normalizeBreakpointThresholds, Rect, describeThrown, and PERF_ENABLED/perfMarkStart/perfMarkEnd/perfNow/perfRecord; also fix the similar ordering/formatting issue referenced around lines 691-696 by applying the same reformatting. Ensure no manual stylistic deviations so CI formatting checks pass.
1491-1501:⚠️ Potential issue | 🟠 MajorPrevent duplicate
batch.release()calls when a turn aborts mid-iterationAt Line 1514 and Line 1535,
drainIgnored(items)drains the full turn payload, including already-processedeventBatchentries that were already released insideprocessEventBatch(). With inline fatal transitions, this can double-release backend resources.🔧 Proposed fix
-function drainIgnored(items: readonly WorkItem[]): void { - for (const it of items) { +function drainIgnored(items: readonly WorkItem[], fromIndex = 0): void { + for (let i = fromIndex; i < items.length; i++) { + const it = items[i]; if (it.kind === "eventBatch") { try { it.batch.release(); } catch { // ignore @@ function processTurn(items: readonly WorkItem[]): void { renderRequestQueuedForCurrentTurn = false; const st = sm.state; if (st === "Disposed" || st === "Faulted") { drainIgnored(items); return; } let sawKick = false; - for (const item of items) { + for (let index = 0; index < items.length; index++) { + const item = items[index]; if (sm.state === "Faulted" || sm.state === "Disposed") { - drainIgnored(items); + drainIgnored(items, index); return; } switch (item.kind) { case "fatal": { doFatal(item.code, item.detail); - drainIgnored(items); + drainIgnored(items, index + 1); return; } @@ processEventBatch(item.batch); if (sm.state !== "Running") { - drainIgnored(items); + drainIgnored(items, index + 1); return; }Based on learnings: Applies to packages/core/src/runtime/{commit.ts,reconcile.ts,router/wheel.ts}|packages/core/src/app/createApp.ts|packages/core/src/layout/|packages/core/src/renderer/|packages/core/src/drawlist/|packages/core/src/binary/ : Extra care required when modifying danger zones: runtime commit/reconcile, wheel router, app creation, layout engine, renderer, drawlist, and binary modules; must verify runtime invariants and reconciliation behavior.
Also applies to: 1512-1515, 1520-1522, 1534-1536
🤖 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 1491 - 1501, drainIgnored currently unconditionally calls batch.release() for every WorkItem of kind "eventBatch", which can double-release resources that were already released inside processEventBatch; change drainIgnored to skip releasing batches that have already been released by tracking identity: maintain a local Set (e.g., releasedBatches) or check a processed flag on the WorkItem before calling batch.release(), and ensure processEventBatch marks the batch (or WorkItem) as released/processed when it releases it so drainIgnored can detect and avoid calling batch.release() again; update references to the WorkItem/eventBatch handling accordingly to prevent duplicate releases.
🧹 Nitpick comments (2)
packages/core/src/runtime/__tests__/localStateStore.test.ts (1)
35-66: Add a regression case for mutating the returned snapshot objects.Consider also mutating
state.measuredHeights/state.loadingKeys/state.expandedSetafterset()and assertingstore.get(...)remains unchanged. That would guard the full “stable snapshot” contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/__tests__/localStateStore.test.ts` around lines 35 - 66, Add assertions that mutating the snapshot objects returned from set() does not affect the store by mutating the state returned from set() and then verifying store.get(...) remains unchanged; specifically, after calling createVirtualListStateStore().set("list", { measuredHeights }) mutate state.measuredHeights (e.g., set a new key) and assert store.get("list").measuredHeights does not contain that new key, and likewise after createTreeStateStore().set("tree", { loadingKeys, expandedSet, expandedSetRef: Object.freeze([...]) }) mutate state.loadingKeys and state.expandedSet and assert store.get("tree").loadingKeys and store.get("tree").expandedSet do not reflect those mutations to ensure snapshots are stable.packages/core/src/widgets/__tests__/layers.golden.test.ts (1)
816-818: Addconsumedassertion for thecloseOnEscape=falsecaseThis test now validates “not closed,” but it no longer locks in Escape consumption/ownership. Adding that assertion would prevent silent routing regressions.
✅ Suggested test hardening
assert.equal(result.closedLayerId, undefined); assert.equal(closedLayer, null); + assert.equal(result.consumed, true);Based on learnings: Applies to **/*.{ts,tsx,jsx} : Modal overlays must support close with Escape key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/layers.golden.test.ts` around lines 816 - 818, The test checks that the layer was not closed (assert.equal(result.closedLayerId, undefined); assert.equal(closedLayer, null)) but doesn’t assert that the Escape key was not consumed; add an assertion that verifies Escape was not consumed/owned in the closeOnEscape=false case (e.g., assert.equal(result.consumed['Escape'], false) or the appropriate shape used in this test harness) so the test locks in that Escape is not captured when closeOnEscape is false; update the test near the existing asserts for closedLayer and closedLayerId, referencing result and the Escape key consumption field.
🤖 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/topLevelViewError.ts`:
- Around line 1-4: The import statements in topLevelViewError.ts are not in the
project's expected order; reorder them to satisfy the linter/formatter (group
and sort imports per project rules) so that imports are consistently ordered
(e.g., external packages, then local modules, then types, each alphabetized) —
specifically reorder the imports for ZrevEvent, describeThrown,
ZR_MOD_ALT|CTRL|META|SHIFT, and VNode into the correct sorted/grouped order and
re-run the formatter/linter to confirm the import-order error is resolved.
In `@packages/core/src/runtime/__tests__/widgetMeta.test.ts`:
- Around line 381-387: The block containing ui.input({ id: "first-input", ...
}), ui.focusZone({ id: "zone-a" }, ...), and ui.focusTrap({ id: "trap-a",
active: true }, ...) is misformatted and failing CI; re-run the repository
formatter (or manually reformat) so the widget list and their nested arrays
match the project's formatting rules (ensure commas, indentation, and line
breaks around the array elements and trailing commas match formatter output) for
the test file's widgetMeta test where ui.input, ui.focusZone, and ui.focusTrap
are defined.
In `@packages/core/src/runtime/commit.ts`:
- Around line 544-556: The code references a non-existent VNode kind "fragment"
causing TS errors; remove fragment-specific handling and utility(s) from
commit.ts: delete or stop calling fragmentPropsEqual and remove the "fragment"
case in canFastReuseContainerSelf plus any other switch/case branches and
comparisons that check prev.kind === "fragment" (and mirror usages elsewhere in
this file such as the other case blocks around the other reported locations),
ensuring remaining branches only handle the canonical VNode kinds defined in
widgets/types.ts; update any calls that relied on fragmentPropsEqual to use the
existing props comparison logic for the actual VNode kinds or inline the
appropriate equality check.
In `@packages/core/src/runtime/localState.ts`:
- Around line 91-97: The cloneReadonlyMap and cloneReadonlySet functions return
mutable Map/Set instances that leak through frozen state objects; change them to
freeze the cloned collections before returning (e.g., create the clone with new
Map(value) / new Set(value), call Object.freeze on that instance, and return it
as a ReadonlyMap/ReadonlySet) so fields like measuredHeights, loadingKeys, and
expandedSet embedded in Object.freeze(state) cannot be mutated; update the
return to the appropriate Readonly type (or cast) while keeping function names
cloneReadonlyMap and cloneReadonlySet.
In `@packages/core/src/runtime/router/layer.ts`:
- Around line 49-56: Update the LayerRoutingResult type to include the optional
callbackError property so the object returned from routeLayerEscape can carry
the caught error; locate the LayerRoutingResult definition (types.ts) and add
callbackError?: unknown (or Error) to its shape, then ensure any callers or
tests (e.g., routeLayerEscape in layer.ts which returns { consumed: true,
callbackError: error }) compile with the updated type.
In `@packages/core/src/runtime/widgetMeta.ts`:
- Around line 860-869: The loop that attributes focusables to the nearest zone
must stop when an enclosing trap/modal is encountered so trap-contained
focusables are not added to outer zones; update the scan in the method using
_containerStack and _zoneFocusables (the loop shown) to first check if
container.kind is "trap" (or the modal kind used in your code) and break without
attributing, otherwise if container.kind === "zone" push the focusableId into
_zoneFocusables.get(container.id) and break — this mirrors collectFocusZones()
behavior and prevents adding trap-contained focusables to outer zones.
---
Outside diff comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 25-59: The imports in createApp.ts are mis-ordered/formatted and
causing CI failures; reorder and reformat this import block to match the
project's import-style rules (group and sort imports consistently and keep
type-only imports together), then run the project formatter/linter (e.g.
prettier/eslint --fix or the repo's format script) to apply exact formatting.
Focus on the listed symbols to locate the block: ZrUiError/ZrUiErrorCode,
BACKEND_* markers and BackendEventBatch/RuntimeBackend, UiEvent/ZrevEvent,
BindingMap/KeyContext/KeybindingManagerState/ModeBindingMap, the keybinding
helpers
(createManagerState/getBindings/getMode/getPendingChord/registerBindings/registerModes/resetChordState/routeKeyEvent/setMode),
ZR_MOD_CTRL, ResponsiveBreakpointThresholds/normalizeBreakpointThresholds, Rect,
describeThrown, and PERF_ENABLED/perfMarkStart/perfMarkEnd/perfNow/perfRecord;
also fix the similar ordering/formatting issue referenced around lines 691-696
by applying the same reformatting. Ensure no manual stylistic deviations so CI
formatting checks pass.
- Around line 1491-1501: drainIgnored currently unconditionally calls
batch.release() for every WorkItem of kind "eventBatch", which can
double-release resources that were already released inside processEventBatch;
change drainIgnored to skip releasing batches that have already been released by
tracking identity: maintain a local Set (e.g., releasedBatches) or check a
processed flag on the WorkItem before calling batch.release(), and ensure
processEventBatch marks the batch (or WorkItem) as released/processed when it
releases it so drainIgnored can detect and avoid calling batch.release() again;
update references to the WorkItem/eventBatch handling accordingly to prevent
duplicate releases.
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 2389-2402: The catch blocks that call reportUserCodeError directly
(around the focus-zone onExit and onEnter handlers) must be hardened so a
failing sink cannot bubble while handling an already-caught widget error; add a
helper method (e.g., reportFocusZoneCallbackError(phase: "onEnter" | "onExit",
error: unknown)) that builds the detail string using describeThrown(error),
calls this.reportUserCodeError(detail) inside its own try/catch, and on sink
failure logs a safe fallback to globalThis.console.error including the sink
error (via describeThrown) and the original detail; replace the two direct
reportUserCodeError(...) calls with calls to
reportFocusZoneCallbackError("onExit", error) and
reportFocusZoneCallbackError("onEnter", error).
- Around line 2125-2141: The routeMouseWheel call is passing
this.findScrollableAncestors (plural, returns an array) but
RouteMouseWheelContext expects findNearestScrollableAncestor (singular, returns
one element or null); fix by passing a wrapper named
findNearestScrollableAncestor that calls this.findScrollableAncestors(targetId)
and returns the first result or null, and supply that wrapper in the
routeMouseWheel context (ensure the property key is
findNearestScrollableAncestor), so update the object passed to routeMouseWheel
accordingly (referencing routeMouseWheel, RouteMouseWheelContext,
findScrollableAncestors, and findNearestScrollableAncestor).
In `@packages/core/src/runtime/commit.ts`:
- Around line 1972-1980: The code in the commit path around ensureInteractiveId,
ensureFocusContainerId and the lifecycle lists (ctx.collectLifecycleInstanceIds
with ctx.lists.reused and ctx.lists.mounted) has formatting drift; run and apply
the project's formatter (prettier/clang-format/ruff as used by the repo) to this
block so spacing/indentation/braces match CI expectations, ensuring the if/else
and return lines for idFatal, focusContainerFatal and the prev branch are
formatted consistently with the rest of the file.
---
Nitpick comments:
In `@packages/core/src/runtime/__tests__/localStateStore.test.ts`:
- Around line 35-66: Add assertions that mutating the snapshot objects returned
from set() does not affect the store by mutating the state returned from set()
and then verifying store.get(...) remains unchanged; specifically, after calling
createVirtualListStateStore().set("list", { measuredHeights }) mutate
state.measuredHeights (e.g., set a new key) and assert
store.get("list").measuredHeights does not contain that new key, and likewise
after createTreeStateStore().set("tree", { loadingKeys, expandedSet,
expandedSetRef: Object.freeze([...]) }) mutate state.loadingKeys and
state.expandedSet and assert store.get("tree").loadingKeys and
store.get("tree").expandedSet do not reflect those mutations to ensure snapshots
are stable.
In `@packages/core/src/widgets/__tests__/layers.golden.test.ts`:
- Around line 816-818: The test checks that the layer was not closed
(assert.equal(result.closedLayerId, undefined); assert.equal(closedLayer, null))
but doesn’t assert that the Escape key was not consumed; add an assertion that
verifies Escape was not consumed/owned in the closeOnEscape=false case (e.g.,
assert.equal(result.consumed['Escape'], false) or the appropriate shape used in
this test harness) so the test locks in that Escape is not captured when
closeOnEscape is false; update the test near the existing asserts for
closedLayer and closedLayerId, referencing result and the Escape key consumption
field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dddb7618-89a5-4258-b9ef-8d165e7317ef
📒 Files selected for processing (25)
CHANGELOG.mddocs/guide/lifecycle-and-updates.mdpackages/core/src/app/__tests__/onEventHandlers.test.tspackages/core/src/app/__tests__/onFocusChange.test.tspackages/core/src/app/__tests__/syncFrameAckFastPath.test.tspackages/core/src/app/__tests__/terminalProfile.test.tspackages/core/src/app/createApp.tspackages/core/src/app/createApp/topLevelViewError.tspackages/core/src/app/rawRenderer.tspackages/core/src/app/widgetRenderer.tspackages/core/src/debug/describeThrown.tspackages/core/src/keybindings/__tests__/keybinding.conflicts.test.tspackages/core/src/perf/__tests__/perf.test.tspackages/core/src/perf/perf.tspackages/core/src/runtime/__tests__/duplicateIdFatal.test.tspackages/core/src/runtime/__tests__/focus.layers.test.tspackages/core/src/runtime/__tests__/localStateStore.test.tspackages/core/src/runtime/__tests__/widgetMeta.test.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/localState.tspackages/core/src/runtime/router/layer.tspackages/core/src/runtime/widgetMeta.tspackages/core/src/widgets/__tests__/layers.golden.test.tspackages/core/src/widgets/__tests__/modal.focus.test.tspackages/core/src/widgets/__tests__/overlay.edge.test.ts
Comment on lines
1
to
4
| import type { ZrevEvent } from "../../events.js"; | ||
| import { describeThrown } from "../../debug/describeThrown.js"; | ||
| import { ZR_MOD_ALT, ZR_MOD_CTRL, ZR_MOD_META, ZR_MOD_SHIFT } from "../../keybindings/keyCodes.js"; | ||
| import type { VNode } from "../../widgets/types.js"; |
There was a problem hiding this comment.
Sort imports to satisfy CI
This file still has an import-order failure in CI; please apply formatter/linter ordering.
🧰 Tools
🪛 GitHub Actions: ci
[error] 1-4: Import statements could be sorted.
🤖 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` around lines 1 - 4, The
import statements in topLevelViewError.ts are not in the project's expected
order; reorder them to satisfy the linter/formatter (group and sort imports per
project rules) so that imports are consistently ordered (e.g., external
packages, then local modules, then types, each alphabetized) — specifically
reorder the imports for ZrevEvent, describeThrown, ZR_MOD_ALT|CTRL|META|SHIFT,
and VNode into the correct sorted/grouped order and re-run the formatter/linter
to confirm the import-order error is resolved.
Comment on lines
+860
to
+869
| // Attribute to the nearest zone and nearest enclosing trap. Focusables | ||
| // inside zones still participate in an enclosing trap's focus scope. | ||
| for (let i = this._containerStack.length - 1; i >= 0; i--) { | ||
| const container = this._containerStack[i]; | ||
| if (!container) continue; | ||
| if (container.kind === "zone") { | ||
| this._zoneFocusables.get(container.id)?.push(focusableId); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not attribute trap-contained focusables to outer zones
The zone-attribution scan currently walks past enclosing traps/modals, so focusables inside a trap can be added to an outer zone. That breaks zone scoping parity with collectFocusZones() behavior and can produce incorrect zone focus sets.
🔧 Proposed fix
- for (let i = this._containerStack.length - 1; i >= 0; i--) {
+ for (let i = this._containerStack.length - 1; i >= 0; i--) {
const container = this._containerStack[i];
if (!container) continue;
+ if (container.kind === "trap") {
+ // Trap/modal boundaries own local focus scope.
+ break;
+ }
if (container.kind === "zone") {
this._zoneFocusables.get(container.id)?.push(focusableId);
break;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/runtime/widgetMeta.ts` around lines 860 - 869, The loop
that attributes focusables to the nearest zone must stop when an enclosing
trap/modal is encountered so trap-contained focusables are not added to outer
zones; update the scan in the method using _containerStack and _zoneFocusables
(the loop shown) to first check if container.kind is "trap" (or the modal kind
used in your code) and break without attributing, otherwise if container.kind
=== "zone" push the focusableId into _zoneFocusables.get(container.id) and break
— this mirrors collectFocusZones() behavior and prevents adding trap-contained
focusables to outer zones.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/app/widgetRenderer.ts (1)
2389-2401:⚠️ Potential issue | 🟠 MajorGuard
reportUserCodeErrorhere to avoid secondary sink failures escaping routing.
onExit/onEntercallback throws are now reported, but ifonUserCodeErroritself throws, this path can still bubble unexpectedly. Please mirror the sink-guard pattern already used inreportInputCallbackError.Suggested patch
if (prevZoneId !== null) { const prev = prevZones.get(prevZoneId); if (prev?.onExit) { try { prev.onExit(); } catch (error: unknown) { - this.reportUserCodeError(`focusZone onExit threw: ${describeThrown(error)}`); + const detail = `focusZone onExit threw: ${describeThrown(error)}`; + try { + this.reportUserCodeError(detail); + } catch (sinkError: unknown) { + warnDev( + `[rezi][runtime] onUserCodeError sink threw while reporting focusZone onExit: ${describeThrown(sinkError)}; original=${detail}`, + ); + } } } } @@ if (nextZoneId !== null) { const next = nextZones.get(nextZoneId); if (next?.onEnter) { try { next.onEnter(); } catch (error: unknown) { - this.reportUserCodeError(`focusZone onEnter threw: ${describeThrown(error)}`); + const detail = `focusZone onEnter threw: ${describeThrown(error)}`; + try { + this.reportUserCodeError(detail); + } catch (sinkError: unknown) { + warnDev( + `[rezi][runtime] onUserCodeError sink threw while reporting focusZone onEnter: ${describeThrown(sinkError)}; original=${detail}`, + ); + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2389 - 2401, The calls to this.reportUserCodeError after catching exceptions from focus zone callbacks (the reportUserCodeError invocations in the current.onExit and next.onEnter handling) must be wrapped with the same sink-guard used by reportInputCallbackError so a failure inside reportUserCodeError itself cannot escape; update both spots to catch any error thrown by this.reportUserCodeError and forward that secondary error into this.reportInputCallbackError (or otherwise swallow it safely) so routing cannot be broken by a reporting failure.packages/core/src/runtime/commit.ts (1)
1678-1687:⚠️ Potential issue | 🟠 MajorUse
describeThrownfor this remaining user-throw path.Line 1683 still manually formats thrown values while other paths now use
describeThrown, leaving error handling behavior inconsistent in the same lifecycle flow.💡 Proposed fix
if (evalRes.threw !== null) { return { ok: false, fatal: { code: "ZRUI_USER_CODE_THROW", - detail: - evalRes.threw instanceof Error - ? `${evalRes.threw.name}: ${evalRes.threw.message}` - : String(evalRes.threw), + detail: describeThrown(evalRes.threw), }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/commit.ts` around lines 1678 - 1687, The user-thrown handling branch in commitEvaluation (the evalRes.threw !== null branch) manually formats thrown values; replace that manual formatting with a call to describeThrown so it matches other paths: in the branch that returns fatal { code: "ZRUI_USER_CODE_THROW", detail: ... } use describeThrown(evalRes.threw) (or the equivalent helper used elsewhere) to produce the detail string and keep the surrounding return structure unchanged, ensuring you import or reference describeThrown the same way other handlers do.
🧹 Nitpick comments (1)
packages/core/src/app/widgetRenderer.ts (1)
4005-4008: Consider adding an upper hard cap to settle passes to bound worst-case frame time.
nodes.length + 1improves deep convergence, but for very large graphs it can produce long synchronous frames. A bounded ceiling (with audit count) would keep behavior predictable under pathological trees.Based on learnings: Extra care required when modifying danger zones: runtime commit/reconcile, wheel router, app creation, layout engine, renderer, drawlist, and binary modules; must verify runtime invariants and reconciliation behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 4005 - 4008, The current settle-pass calculation uses Math.max(3, constraintGraph.nodes.length + 1) which can produce unbounded synchronous frames for very large graphs; add an upper hard cap (e.g. const HARD_CAP = <reasonable-number>) and compute maxSettlePasses = Math.min(HARD_CAP, Math.max(3, constraintGraph.nodes.length + 1)); also increment or emit an audit/count metric when the cap is hit so we can detect pathological trees; update the code around maxSettlePasses and add a brief comment explaining the cap and the audit to preserve runtime/reconciliation invariants (verify behavior in the layout/renderer reconciliation paths after change).
🤖 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/runtime/commit.ts`:
- Around line 1959-1960: The small formatting drift around the
ensureFocusContainerId call is causing CI failures; open the block containing
ensureFocusContainerId(ctx.seenFocusContainerIds, instanceId, vnode) and the
subsequent early return using focusContainerFatal and reformat it to match the
project's code style (run the repo formatter / Prettier or apply the same
spacing/semicolon conventions used elsewhere), ensuring the single-line const
assignment and the if return remain correctly indented and spaced: const
focusContainerFatal = ensureFocusContainerId(...); if (focusContainerFatal)
return { ok: false, fatal: focusContainerFatal };.
---
Outside diff comments:
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 2389-2401: The calls to this.reportUserCodeError after catching
exceptions from focus zone callbacks (the reportUserCodeError invocations in the
current.onExit and next.onEnter handling) must be wrapped with the same
sink-guard used by reportInputCallbackError so a failure inside
reportUserCodeError itself cannot escape; update both spots to catch any error
thrown by this.reportUserCodeError and forward that secondary error into
this.reportInputCallbackError (or otherwise swallow it safely) so routing cannot
be broken by a reporting failure.
In `@packages/core/src/runtime/commit.ts`:
- Around line 1678-1687: The user-thrown handling branch in commitEvaluation
(the evalRes.threw !== null branch) manually formats thrown values; replace that
manual formatting with a call to describeThrown so it matches other paths: in
the branch that returns fatal { code: "ZRUI_USER_CODE_THROW", detail: ... } use
describeThrown(evalRes.threw) (or the equivalent helper used elsewhere) to
produce the detail string and keep the surrounding return structure unchanged,
ensuring you import or reference describeThrown the same way other handlers do.
---
Nitpick comments:
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 4005-4008: The current settle-pass calculation uses Math.max(3,
constraintGraph.nodes.length + 1) which can produce unbounded synchronous frames
for very large graphs; add an upper hard cap (e.g. const HARD_CAP =
<reasonable-number>) and compute maxSettlePasses = Math.min(HARD_CAP,
Math.max(3, constraintGraph.nodes.length + 1)); also increment or emit an
audit/count metric when the cap is hit so we can detect pathological trees;
update the code around maxSettlePasses and add a brief comment explaining the
cap and the audit to preserve runtime/reconciliation invariants (verify behavior
in the layout/renderer reconciliation paths after change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2b6e90d-3b25-4bb2-9db3-06fa680bcc15
📒 Files selected for processing (6)
packages/core/src/app/__tests__/commandPaletteRouting.test.tspackages/core/src/app/widgetRenderer.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/router/types.tspackages/core/src/widgets/__tests__/layers.golden.test.tspackages/core/src/widgets/__tests__/modal.focus.test.ts
packages/core/src/runtime/commit.ts
Outdated
Comment on lines
+1959
to
+1960
| const focusContainerFatal = ensureFocusContainerId(ctx.seenFocusContainerIds, instanceId, vnode); | ||
| if (focusContainerFatal) return { ok: false, fatal: focusContainerFatal }; |
There was a problem hiding this comment.
Formatter gate is failing on this block.
CI reports formatting drift around Line 1957-1963; this should be formatted to unblock merge.
🎨 Expected formatting
- const focusContainerFatal = ensureFocusContainerId(ctx.seenFocusContainerIds, instanceId, vnode);
+ const focusContainerFatal = ensureFocusContainerId(
+ ctx.seenFocusContainerIds,
+ instanceId,
+ vnode,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const focusContainerFatal = ensureFocusContainerId(ctx.seenFocusContainerIds, instanceId, vnode); | |
| if (focusContainerFatal) return { ok: false, fatal: focusContainerFatal }; | |
| const focusContainerFatal = ensureFocusContainerId( | |
| ctx.seenFocusContainerIds, | |
| instanceId, | |
| vnode, | |
| ); | |
| if (focusContainerFatal) return { ok: false, fatal: focusContainerFatal }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/runtime/commit.ts` around lines 1959 - 1960, The small
formatting drift around the ensureFocusContainerId call is causing CI failures;
open the block containing ensureFocusContainerId(ctx.seenFocusContainerIds,
instanceId, vnode) and the subsequent early return using focusContainerFatal and
reformat it to match the project's code style (run the repo formatter / Prettier
or apply the same spacing/semicolon conventions used elsewhere), ensuring the
single-line const assignment and the if return remain correctly indented and
spaced: const focusContainerFatal = ensureFocusContainerId(...); if
(focusContainerFatal) return { ok: false, fatal: focusContainerFatal };.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/app/createApp.ts (1)
34-35:⚠️ Potential issue | 🟡 MinorFix import ordering to unblock CI.
Line 34-35 and Line 58-59 are failing Biome
organizeImportsin the pipeline.Also applies to: 58-59
🤖 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 34 - 35, The import statements in createApp.ts are out of the expected ordering (specifically the type imports for UiEvent and ZrevEvent and the later import block around the other types), which causes Biome's organizeImports failure; reorder and group imports according to the project's import style (external packages first, then internal modules, and alphabetically within groups), combine and alphabetize the type imports (e.g., the import that declares UiEvent and ZrevEvent) and the later type import block so they are consistently ordered, remove any unused imports, and ensure the final import ordering satisfies Biome's organizeImports checks.
🧹 Nitpick comments (2)
packages/core/src/app/widgetRenderer.ts (1)
2421-2482: Consider trimming allocations in the scrollable-ancestor traversal hot path.On Line 2460, freezing a newly cloned array during traversal is extra work for wheel-heavy flows. You can defer freezing until return time and keep intermediate path arrays mutable.
Low-risk micro-optimization
- let scrollables = frame.scrollables; + let scrollables = frame.scrollables; @@ - if (hasScrollableAxis) { - scrollables = Object.freeze([...scrollables, { nodeId, meta }]); - } + if (hasScrollableAxis) { + scrollables = [...scrollables, { nodeId, meta }]; + } @@ - if (nodeId === targetId) { - return Object.freeze([...scrollables].reverse()); - } + if (nodeId === targetId) { + return Object.freeze(scrollables.slice().reverse()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2421 - 2482, The traversal creates and freezes arrays on every pushed scrollable match (see findScrollableAncestors and the local scrollables variable), which allocates and freezes repeatedly; instead, initialize Cursor.scrollables with a plain mutable [] (not Object.freeze([])), when discovering a scrollable create a new array using [...scrollables, { nodeId, meta }] without freezing, pass that array into child frames, and only call Object.freeze (and reverse as currently done) at the two return sites (the found-path return and the final empty return) so freezing is deferred to return time.packages/core/src/app/createApp.ts (1)
1495-1498: Avoid double-queuingrenderRequestin theme-transition turns.Line 1495 can enqueue a second
renderRequestin the same turn afterscheduleThemeTransitionContinuation()already queued one, which adds avoidable no-op queue work.♻️ Proposed tweak
- if (dirtyTracker.getFlags() !== 0 && !renderRequestQueuedForCurrentTurn) { + if ( + dirtyTracker.getFlags() !== 0 && + !renderRequestQueuedForCurrentTurn && + themeTransition === null + ) { renderRequestQueuedForCurrentTurn = true; scheduler.enqueue({ kind: "renderRequest" }); }🤖 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 1495 - 1498, The code can enqueue a duplicate render request because scheduleThemeTransitionContinuation() may already queue a renderRequest; to fix, ensure we don't enqueue twice by using the shared renderRequestQueuedForCurrentTurn flag consistently: either have scheduleThemeTransitionContinuation() set renderRequestQueuedForCurrentTurn = true when it enqueues a renderRequest, or change this block (which checks dirtyTracker.getFlags()) to also check whatever internal indicator scheduleThemeTransitionContinuation uses before calling scheduler.enqueue({ kind: "renderRequest" }); reference symbols: dirtyTracker.getFlags, renderRequestQueuedForCurrentTurn, scheduleThemeTransitionContinuation, and scheduler.enqueue({ kind: "renderRequest" }).
🤖 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/__tests__/widgetRenderer.integration.test.ts`:
- Around line 828-862: The test "focusZone error reporting swallows
onUserCodeError sink failures" currently only asserts no throw and final focus,
so add counters to prove both the focus handler and the sink were invoked:
create numeric counters and increment them inside the focusZone onEnter handler
and the WidgetRenderer onUserCodeError sink (pass a function that increments a
counter and throws), then after calling renderer.routeEngineEvent(keyEvent(3 /*
TAB */)) assert that the onEnter counter is 1 and the onUserCodeError counter is
1 in addition to the existing doesNotThrow and getFocusedId assertions so the
error-routing path is deterministically exercised (refer to symbols
WidgetRenderer, onUserCodeError, onEnter, renderer.routeEngineEvent, and
renderer.getFocusedId).
In `@packages/core/src/runtime/__tests__/widgetMeta.test.ts`:
- Around line 420-433: The test currently calls
collectAllWidgetMetadata(committed) which creates a fresh collector per call and
doesn't exercise the pooled (reusable) collector path; change the test to obtain
and use the pooled collector API instead (e.g., call
createPooledWidgetMetadataCollector() or acquirePooledWidgetMetadataCollector()
from the widget meta module), use the pooledCollector.collect(committed) (or
equivalent) to collect metadata, then release/return the pooled collector after
use so the test exercises reuse/state retention in the pooled collector rather
than the one-shot collectAllWidgetMetadata call.
In `@packages/core/src/runtime/localState.ts`:
- Around line 110-112: Replace the usage of Map.prototype.forEach in the wrapper
methods with explicit for...of loops: in the forEach method of the LocalState
wrapper (the method currently calling this.map.forEach((value, key) =>
callbackfn.call(thisArg, value, key, this))), iterate with for (const [key,
value] of this.map) and invoke callbackfn.call(thisArg, value, key, this) inside
the loop; do the same for the other wrapper implementation mentioned at line
~163 (the similar wrapper that currently uses .forEach) so both follow the
for...of pattern required by the linter.
---
Outside diff comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 34-35: The import statements in createApp.ts are out of the
expected ordering (specifically the type imports for UiEvent and ZrevEvent and
the later import block around the other types), which causes Biome's
organizeImports failure; reorder and group imports according to the project's
import style (external packages first, then internal modules, and alphabetically
within groups), combine and alphabetize the type imports (e.g., the import that
declares UiEvent and ZrevEvent) and the later type import block so they are
consistently ordered, remove any unused imports, and ensure the final import
ordering satisfies Biome's organizeImports checks.
---
Nitpick comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 1495-1498: The code can enqueue a duplicate render request because
scheduleThemeTransitionContinuation() may already queue a renderRequest; to fix,
ensure we don't enqueue twice by using the shared
renderRequestQueuedForCurrentTurn flag consistently: either have
scheduleThemeTransitionContinuation() set renderRequestQueuedForCurrentTurn =
true when it enqueues a renderRequest, or change this block (which checks
dirtyTracker.getFlags()) to also check whatever internal indicator
scheduleThemeTransitionContinuation uses before calling scheduler.enqueue({
kind: "renderRequest" }); reference symbols: dirtyTracker.getFlags,
renderRequestQueuedForCurrentTurn, scheduleThemeTransitionContinuation, and
scheduler.enqueue({ kind: "renderRequest" }).
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 2421-2482: The traversal creates and freezes arrays on every
pushed scrollable match (see findScrollableAncestors and the local scrollables
variable), which allocates and freezes repeatedly; instead, initialize
Cursor.scrollables with a plain mutable [] (not Object.freeze([])), when
discovering a scrollable create a new array using [...scrollables, { nodeId,
meta }] without freezing, pass that array into child frames, and only call
Object.freeze (and reverse as currently done) at the two return sites (the
found-path return and the final empty return) so freezing is deferred to return
time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0127f9f3-97e7-4094-8b26-8c2fb749fe0a
📒 Files selected for processing (10)
packages/core/src/app/__tests__/eventPump.test.tspackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/createApp.tspackages/core/src/app/widgetRenderer.tspackages/core/src/runtime/__tests__/localStateStore.test.tspackages/core/src/runtime/__tests__/widgetMeta.test.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/localState.tspackages/core/src/runtime/widgetMeta.tspackages/core/src/widgets/__tests__/layers.golden.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/widgets/tests/layers.golden.test.ts
Comment on lines
+420
to
+433
| test("widgetMeta: pooled collector does not attribute trap focusables to outer zones", () => { | ||
| const committed = commitTree( | ||
| ui.focusZone({ id: "outer-zone" }, [ | ||
| ui.button({ id: "outer-btn", label: "Outer" }), | ||
| ui.focusTrap({ id: "inner-trap", active: true }, [ | ||
| ui.button({ id: "inner-btn", label: "Inner" }), | ||
| ]), | ||
| ]), | ||
| ); | ||
|
|
||
| const metadata = collectAllWidgetMetadata(committed); | ||
|
|
||
| assert.deepEqual(metadata.zones.get("outer-zone")?.focusableIds, ["outer-btn"]); | ||
| assert.deepEqual(metadata.traps.get("inner-trap")?.focusableIds, ["inner-btn"]); |
There was a problem hiding this comment.
pooled collector test is currently exercising the one-shot collector path
Line 430 calls collectAllWidgetMetadata(committed), but packages/core/src/runtime/widgetMeta.ts (Lines 1109-1113) creates a fresh collector per call. This won’t catch regressions specific to reusable collector state reuse.
Proposed fix
test("widgetMeta: pooled collector does not attribute trap focusables to outer zones", () => {
+ const collector = createWidgetMetadataCollector();
+
+ // Seed a prior collect to exercise reusable-collector state reset behavior.
+ collector.collect(
+ commitTree(
+ ui.focusTrap({ id: "seed-trap", active: true }, [
+ ui.button({ id: "seed-btn", label: "Seed" }),
+ ]),
+ ),
+ );
+
const committed = commitTree(
ui.focusZone({ id: "outer-zone" }, [
ui.button({ id: "outer-btn", label: "Outer" }),
ui.focusTrap({ id: "inner-trap", active: true }, [
ui.button({ id: "inner-btn", label: "Inner" }),
]),
]),
);
- const metadata = collectAllWidgetMetadata(committed);
+ const metadata = collector.collect(committed);
assert.deepEqual(metadata.zones.get("outer-zone")?.focusableIds, ["outer-btn"]);
assert.deepEqual(metadata.traps.get("inner-trap")?.focusableIds, ["inner-btn"]);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/runtime/__tests__/widgetMeta.test.ts` around lines 420 -
433, The test currently calls collectAllWidgetMetadata(committed) which creates
a fresh collector per call and doesn't exercise the pooled (reusable) collector
path; change the test to obtain and use the pooled collector API instead (e.g.,
call createPooledWidgetMetadataCollector() or
acquirePooledWidgetMetadataCollector() from the widget meta module), use the
pooledCollector.collect(committed) (or equivalent) to collect metadata, then
release/return the pooled collector after use so the test exercises reuse/state
retention in the pooled collector rather than the one-shot
collectAllWidgetMetadata call.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/app/createApp.ts (1)
884-890:⚠️ Potential issue | 🟠 MajorDefer inline fatal dispatch while still inside user handler depth.
At Line 885,
fatalNowOrEnqueuecan calldoFatal()duringemit()beforeinEventHandlerDepthis decremented. That makes fatal callbacks run in a re-entrant context, so app API calls from fatal handlers (e.g.,dispose()at Line 1887) throw and get swallowed indoFatal’s handler loop. This silently drops user cleanup/reporting logic in this failure path.💡 Proposed fix
function fatalNowOrEnqueue(code: ZrUiErrorCode, detail: string): void { - if (scheduler.isExecuting) { + const canFailFastInline = + scheduler.isExecuting && !inRender && !inCommit && inEventHandlerDepth === 0; + if (canFailFastInline) { doFatal(code, detail); return; } enqueueFatal(code, detail); }Also applies to: 919-939, 945-963, 1886-1890
🧹 Nitpick comments (1)
packages/core/src/app/widgetRenderer.ts (1)
2426-2431: Preferinterfacefor these object-shape declarations.Lines 2426–2427 define
ScrollableMatchandCursorusingtypewithReadonly<{...}>wrapper. Per the repo's TypeScript guidelines, these should be declared asinterfacewithreadonlyfields instead.Suggested conversion
- type ScrollableMatch = Readonly<{ nodeId: string; meta: LayoutOverflowMetadata }>; - type Cursor = Readonly<{ - runtimeNode: RuntimeInstance; - layoutNode: LayoutTree; - scrollables: readonly ScrollableMatch[]; - }>; + interface ScrollableMatch { + readonly nodeId: string; + readonly meta: LayoutOverflowMetadata; + } + interface Cursor { + readonly runtimeNode: RuntimeInstance; + readonly layoutNode: LayoutTree; + readonly scrollables: readonly ScrollableMatch[]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2426 - 2431, Convert the two type aliases into interfaces and use readonly properties instead of Readonly<...>; replace the type ScrollableMatch = Readonly<{ nodeId: string; meta: LayoutOverflowMetadata }>; with an interface ScrollableMatch { readonly nodeId: string; readonly meta: LayoutOverflowMetadata } and replace the type Cursor = Readonly<{ runtimeNode: RuntimeInstance; layoutNode: LayoutTree; scrollables: readonly ScrollableMatch[] }>; with an interface Cursor { readonly runtimeNode: RuntimeInstance; readonly layoutNode: LayoutTree; readonly scrollables: readonly ScrollableMatch[] } so the shapes are declared as interfaces and each field is marked readonly.
🤖 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/widgetRenderer.ts`:
- Around line 2426-2431: Convert the two type aliases into interfaces and use
readonly properties instead of Readonly<...>; replace the type ScrollableMatch =
Readonly<{ nodeId: string; meta: LayoutOverflowMetadata }>; with an interface
ScrollableMatch { readonly nodeId: string; readonly meta: LayoutOverflowMetadata
} and replace the type Cursor = Readonly<{ runtimeNode: RuntimeInstance;
layoutNode: LayoutTree; scrollables: readonly ScrollableMatch[] }>; with an
interface Cursor { readonly runtimeNode: RuntimeInstance; readonly layoutNode:
LayoutTree; readonly scrollables: readonly ScrollableMatch[] } so the shapes are
declared as interfaces and each field is marked readonly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25bf7942-1979-4d3d-8354-fe25281324b4
📒 Files selected for processing (6)
packages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/createApp.tspackages/core/src/app/createApp/topLevelViewError.tspackages/core/src/app/widgetRenderer.tspackages/core/src/runtime/__tests__/widgetMeta.test.tspackages/core/src/runtime/localState.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/app/createApp/topLevelViewError.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
Verification
Summary by CodeRabbit
Documentation
Bug Fixes
API Changes
Tests