Skip to content

test: share native chat render cost setup - #29316

Merged
OrcaWin merged 18 commits into
mainfrom
OrcaWin/test-audit-chat-render-startup
Oct 16, 2026
Merged

OrcaWin merged 18 commits into
mainfrom
OrcaWin/test-audit-chat-render-startup

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

ELI5

Keep the three chat rendering performance checks together so each PR loads their shared component graph once.

What Changed

Combine the streaming-render and work-run memo tests in one render-cost suite. Preserve settled-prose work per frame, lazy diff parsing and rebuilding only the live tool/reasoning run. Every probe calls the real implementation; counters reset before each case. The separate tool-stream pacing suite stays with its own environment. Production code is unchanged; test code drops31lines.

Why

The two suites loaded the same rendering graph separately. Sharing that setup reduces worker work while retaining three independent performance contracts. Keeping the real probes preserves protection against expensive chat updates.

Linked Issue

Maintainer-requested audit of tests run on every PR.

Visual Proof

N/A — test organization only.

Testing

  • I manually tested these changes locally
  • Automated tests updated

Linux, background launch, Node24.21.0/Bun1.4.3. Six owner/marker suites pass59cases with shuffle104729. Three alternating one-worker pairs retain the exact three performance names: median wall6.889→6.061s and summed module5.187→4.450s. These are selected local measurements, without a whole-PR timing claim.

Three deliberate production regressions fail their intended retained assertions: defeating row reuse, eagerly parsing diff details and rebuilding settled work runs. All production sources were restored byte-for-byte and all three performance cases pass afterward. Renderer typecheck, changed-code quality, lint, formatting and whitespace pass.

Historical hosted measurements are retained as historical evidence; final main-based exact-head CI and source-qualified hosted costs are required before merge.

Review

Independent preservation review approved all original assertions and real call-through probes. Initial120-row positive control, exact diff summary/detail counts and rendered text, and four mounted work runs with exactly five tail rebuilds remain. The refreshed delta preserves the original two logical paths, +116/-147 test lines, with no production change.

Agent skill upstream boundary

  • Not applicable; no upstream skill source copied.

Checklist

  • Before/after, mechanism and approach explained
  • Independent preservation review and local measurement
  • Platform/SSH/folder behavior unchanged by test-only organization
  • Final main-based full PR workflow and separate checks
  • Final hosted measurement qualified against actual executed sources

Final hosted qualification: exact393063c7a3 full .github/workflows/pr.yml run38878737147 passes verify116903217505 and all five unit shards; current separate Headless38878736888 and LoC38878737080 checks pass or intentionally skip. Earlier same-head event runs are superseded. Actual checkouta8500a87 matches all26 inspected relevant blobs. All five inventories reconcile12333unique module paths with zero missing/duplicate/omitted failures or unhandled errors. Baseline actual549ea6c2 equals main2e97 tree. The three unchanged performance cases measure3504→2065ms selected module time (-41.07%); displayed execution731→855ms. Historical/unpaired runner observations establish neither a causal execution change nor whole-PR elapsed savings.

@OrcaWin
OrcaWin changed the base branch from OrcaWin/test-audit-runtime-title-startup to main October 16, 2026 08:39
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The render-cost test now counts calls to nativeChatWorkRunEntries and verifies that five streaming-tail rerenders cause five calls. It also checks that the initial transcript renders four tool-run states. The separate work-run memo test file was removed.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 7e4c1

This change only affects tests, so there is no runtime impact. The streaming render check could be tightened to confirm that the live run is the one rebuilt. This is a minor follow-up and not a merge blocker.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the main change: sharing native chat render-cost test setup.
Description check Passed The description is detailed and covers the change, rationale, testing, review, scope, and performance evidence. It includes visual proof as not applicable and confirms that production code is unchange…
Full details: Description check

Explanation

The description is detailed and covers the change, rationale, testing, review, scope, and performance evidence. It includes visual proof as not applicable and confirms that production code is unchanged. The description does not provide an actual linked issue reference, but the remaining content is substantially complete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f9efa099-1646-454b-a467-5d1ac2b81cfa
📥 Commits

Reviewing files that changed from the base of the PR and between b89373c and 7e4c169.

📒 Files selected for processing: 2
  • src/renderer/src/components/native-chat/NativeChatMessageList.render-cost.test.tsx
  • src/renderer/src/components/native-chat/NativeChatMessageList.work-run-memo.test.tsx
💤 Files with no reviewable changes: 1
  • src/renderer/src/components/native-chat/NativeChatMessageList.work-run-memo.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

for (let frame = 1; frame <= 5; frame += 1) {
rerender(list(tail(frame)))
}
expect(entriesCalls.count).toBe(5)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Verify that each call rebuilds the live run.

The total of five calls does not identify the run passed to nativeChatWorkRunEntries. If one settled run rebuilds on each update while the live run stays stale, this assertion still passes. Record the wrapper’s members arguments and assert that every post-reset call contains the updated live run, not a settled run. This directly checks the stated purpose of the case.

@OrcaWin
OrcaWin merged commit 9ee7ee7 into main Oct 16, 2026
38 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant