fix(core): use monotonic render metrics#292
Merged
RtlZeroMemory merged 2 commits intomainfrom Mar 18, 2026
Merged
Conversation
📝 WalkthroughWalkthroughRender timing instrumentation was changed to use a monotonic clock for render start/elapsed measurements; a test helper to mock Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ink-compat/src/__tests__/perf/bottleneck-profile.test.ts (2)
325-329: Add an explicit reuse-path correctness assertion.Current check proves fresh allocation equivalence, but it doesn’t validate the second-call reset/reuse behavior that the benchmark is timing. A quick mutate-then-reuse assertion would lock this down.
Suggested patch
const expectedGrid = allocateGrid_CURRENT(cols, rows); reusableCols = 0; const actualGrid = allocateGrid_REUSE(cols, rows); assert.deepEqual(actualGrid, expectedGrid); + actualGrid[0]![0]!.char = "X"; + actualGrid[0]![0]!.style = { bold: true }; + const reusedGrid = allocateGrid_REUSE(cols, rows); + assert.equal(reusedGrid, actualGrid); + assert.deepEqual(reusedGrid, expectedGrid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/__tests__/perf/bottleneck-profile.test.ts` around lines 325 - 329, The test currently only compares a fresh allocateGrid_REUSE(cols, rows) to allocateGrid_CURRENT(cols, rows) but doesn't assert that the reuse path actually resets and reinitializes state on a second call; update the test to mutate the grid returned by the first allocateGrid_REUSE call (or set reusableCols to a nonzero sentinel), then call allocateGrid_REUSE(cols, rows) again and assert the result equals allocateGrid_CURRENT(cols, rows) (use allocateGrid_CURRENT(cols, rows) as the expected value) to validate the reset/reuse behavior of allocateGrid_REUSE; reference allocateGrid_REUSE, allocateGrid_CURRENT and reusableCols when making the change.
227-238: Harden median helpers for edge inputs.
median()/benchMedian()currently assume non-empty samples. Ifsamplesis ever set to0, Line 229 returnsundefined(asserted non-null) and downstream math becomes invalid.Suggested patch
function median(values: readonly number[]): number { + if (values.length === 0) { + throw new Error("median() requires at least one value"); + } const sorted = [...values].sort((left, right) => left - right); - return sorted[Math.floor(sorted.length / 2)]!; + const middle = Math.floor(sorted.length / 2); + return sorted.length % 2 === 0 + ? (sorted[middle - 1]! + sorted[middle]!) / 2 + : sorted[middle]!; } function benchMedian(fn: () => void, iterations: number, samples: number): number { + if (samples <= 0) { + throw new Error("benchMedian() requires samples > 0"); + } const runs: number[] = []; for (let index = 0; index < samples; index += 1) { runs.push(bench(`sample-${String(index)}`, fn, iterations)); } return median(runs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/__tests__/perf/bottleneck-profile.test.ts` around lines 227 - 238, The median() and benchMedian() helpers assume non-empty inputs and will return undefined when given empty arrays or samples=0; update median(values) to handle values.length===0 (e.g., return 0 or NaN) instead of indexing into sorted[], and update benchMedian(fn, iterations, samples) to early-return the same neutral numeric value when samples<=0 (no runs) to avoid downstream invalid math; reference the median and benchMedian functions (and the use of bench within benchMedian) and ensure return types remain number.
🤖 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/ink-compat/src/__tests__/perf/bottleneck-profile.test.ts`:
- Around line 325-329: The test currently only compares a fresh
allocateGrid_REUSE(cols, rows) to allocateGrid_CURRENT(cols, rows) but doesn't
assert that the reuse path actually resets and reinitializes state on a second
call; update the test to mutate the grid returned by the first
allocateGrid_REUSE call (or set reusableCols to a nonzero sentinel), then call
allocateGrid_REUSE(cols, rows) again and assert the result equals
allocateGrid_CURRENT(cols, rows) (use allocateGrid_CURRENT(cols, rows) as the
expected value) to validate the reset/reuse behavior of allocateGrid_REUSE;
reference allocateGrid_REUSE, allocateGrid_CURRENT and reusableCols when making
the change.
- Around line 227-238: The median() and benchMedian() helpers assume non-empty
inputs and will return undefined when given empty arrays or samples=0; update
median(values) to handle values.length===0 (e.g., return 0 or NaN) instead of
indexing into sorted[], and update benchMedian(fn, iterations, samples) to
early-return the same neutral numeric value when samples<=0 (no runs) to avoid
downstream invalid math; reference the median and benchMedian functions (and the
use of bench within benchMedian) and ensure return types remain number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 793ca435-ca9e-43b4-8f96-8c7e25540004
📒 Files selected for processing (1)
packages/ink-compat/src/__tests__/perf/bottleneck-profile.test.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
internal_onRendermetrics with the always-on monotonic clock instead of perf-only timingframe.renderTimeMsaligned with the same monotonic render measurementTesting
PTY Evidence
300x68backend_submitted=735,worker_payload=735,worker_accepted=735,worker_completed=735hash_mismatch_backend_vs_worker=0bridge=377,engineering=293,crew=53,settings=12,invalidCmdStreams=0bridge=0xcec610ce,engineering=0xbff64150,crew=0xff318b6f,settings=0x17d371fdSummary by CodeRabbit
Bug Fixes
Tests