Repository navigation
[CI] Reduce import, frame-capture, teardown, and shard costs - #28879
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 4.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 42 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly summarizes the main changes: reducing CI costs across imports, frame capture, teardown, and shard execution. Description check The description is complete and detailed. It covers the required explanation, changes, rationale, issue context, visual proof, testing evidence, review notes, compatibility considerations, and checkli…
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- 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.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments: 1
config/scripts/ci-unit-module-cache.mjs: 1
216-216: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake the
prepareandsealphases tolerate a missing identity report.The
identityphase catches its errors and only logs a warning. If it fails, the workflow skips the later steps, because thekeyoutput is empty. However, a failedbunlookup or a hashing error can still happen afteroutput()writes the key. In that case, Line 216 throws an unhandled error while it readsIDENTITY_REPORT, and the shard fails. The risk is small, becausewriteAssignmentruns beforeoutput. Still, theprepareandsealphases are optional cache work. An unhandled read error in either phase must not fail the test job.Proposed fix
- const identity = JSON.parse(readFileSync(IDENTITY_REPORT, 'utf8')) + let identity + try { + identity = JSON.parse(readFileSync(IDENTITY_REPORT, 'utf8')) + } catch (error) { + console.warn(`Unit module cache identity unavailable: ${String(error)}`) + process.exit(0) + }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5692b33e-2718-49ea-8da0-db2b75ebcecd
⛔ Files ignored due to path filters: 2
cloud/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/renderer/src/components/settings/__snapshots__/terminal-preview-content.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing: 54
.github/workflows/cloud-verify.yml.github/workflows/e2e.yml.github/workflows/unit-module-cache-benchmark.yml.github/workflows/unit-tests.ymlcloud/apps/relay/package.jsoncloud/apps/relay/src/postgres-lost-reply-postgres.test.tscloud/apps/relay/vitest.config.tscloud/dev/scripts/run-relay-tests.mjscloud/dev/scripts/run-relay-tests.test.mjscloud/package.jsonconfig/scripts/ci-e2e-job-selection.test.mjsconfig/scripts/ci-native-cache.test.mjsconfig/scripts/ci-shard-timings.jsonconfig/scripts/ci-shard-timings.mdconfig/scripts/ci-unit-module-cache-contents.mjsconfig/scripts/ci-unit-module-cache-contents.test.mjsconfig/scripts/ci-unit-module-cache-trailer.mjsconfig/scripts/ci-unit-module-cache.mjsconfig/scripts/ci-unit-module-cache.test.mjsconfig/scripts/ci-unit-timing-reporter.mjsconfig/scripts/ci-unit-timing-reporter.test.mjsconfig/scripts/orcad-e2e-routing.test.mjsconfig/scripts/release-cut-token-permissions.test.mjsconfig/vitest.config.tssrc/main/browser/browser-popup-compatibility-cdp-fixture.tssrc/main/browser/browser-popup-compatibility-electron-fixture.tssrc/main/browser/browser-popup-compatibility-page-fixture.tssrc/main/browser/hidden-renderer-frame-capture-fixture.tssrc/main/runtime/mobile-subscribe-idle-pane-hydrate.test.tssrc/main/runtime/opencode-wsl-folder-launch-host.test.tssrc/main/runtime/rpc/dispatcher-transport-parity.test.tssrc/main/runtime/rpc/methods/agent-launch-restart-replay.test.tssrc/main/runtime/rpc/methods/managed-server.test.tssrc/main/runtime/rpc/methods/mobile-web-bundle-read-concurrency.test.tssrc/main/runtime/rpc/methods/mobile-web-bundle-window-refusals.test.tssrc/main/runtime/rpc/methods/mobile-web-bundle.test.tssrc/main/runtime/rpc/methods/structured-agent-session-attachments.test.tssrc/main/runtime/rpc/orchestration-legacy-compatibility-dispatcher.test.tssrc/main/startup/codex-hook-startup-wiring.test.tssrc/main/startup/main-process-runtime-launch-activation.test.tssrc/renderer/src/components/editor/MonacoEditor.font-family.test.tsxsrc/renderer/src/components/editor/MonacoEditor.system-theme.test.tsxsrc/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsxsrc/renderer/src/components/settings/ShortcutCommandBlock.test.tsxsrc/renderer/src/components/settings/appearance-interface-summary.test.tssrc/renderer/src/components/settings/terminal-preview-content.test.tstests/e2e/helpers/electron-process-shutdown.tstests/e2e/helpers/electron-process-shutdown.unit.test.tstests/e2e/helpers/electron-shutdown-timing.tstests/e2e/helpers/hidden-renderer-frame-capture.tstests/e2e/helpers/orca-app.tstests/e2e/remote-terminal-input-delivery-model.unit.test.tstests/e2e/terminal-windows-shell-paste-ownership.spec.tstests/playwright.config.ts
💤 Files with no reviewable changes: 5
- src/renderer/src/components/settings/ShortcutCommandBlock.test.tsx
- src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx
- src/renderer/src/components/settings/terminal-preview-content.test.ts
- config/scripts/release-cut-token-permissions.test.mjs
- src/renderer/src/components/settings/appearance-interface-summary.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments: 1
config/scripts/pr-auxiliary-preflight-admission.test.mjs: 1
251-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover
node-next-compat.ymlin the relay-default assertion.
node-next-compat.ymlis an ordinary unit-test caller. It invokesunit-tests.ymlwithoutrun_relay_integration, so a futurefalseoverride could skip its relay tests while this admission test still passes because it checks onlypr.yml.Suggested fix
const workflow = parse(readFileSync('.github/workflows/pr.yml', 'utf8')) const unit = parse(readFileSync('.github/workflows/unit-tests.yml', 'utf8')) +const nodeNextCompat = parse(readFileSync('.github/workflows/node-next-compat.yml', 'utf8')) @@ expect( evaluate(unit.jobs.relay_integration.if, { inputs: { run_relay_integration: false } }) ).toBe(false) expect(workflow.jobs.test.with.run_relay_integration).toBeUndefined() + expect(nodeNextCompat.jobs.test.with.run_relay_integration).toBeUndefined() expect(unit.jobs.relay_integration.needs).toBeUndefined()
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
171caa41-facc-432e-8349-a04681f11452
📒 Files selected for processing: 1
config/scripts/pr-auxiliary-preflight-admission.test.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
nwparker
changed the title
ELI5
CI spends less time loading unrelated code, waiting for hidden browser frames, and holding processes open after a test. The runner types, shard counts, and capacity stay the same. Hosted checks will determine the net runner-time improvement.
What Changed
Why
Measurements identify fixture waits, unrelated imports, and duplicate setup as substantial costs. Reusing existing capture, cleanup, registry, and process-host implementations keeps the changes narrow and preserves behavior checks. Caching needs a successful same-source full-suite comparison before it can become a normal CI policy.
Linked Issue
N/A — internal CI maintenance.
Visual Proof
N/A — no product layout or interaction changes. Hidden-renderer screenshots and presentation-call records were inspected during fixture validation.
Testing
All figures below are local evidence, not hosted runner savings:
Popup validation also passed 39 cases twice on macOS, plus an independent serial 39-case run matching packaging's file policy (46.55s). Linux trace/video outputs validate and decode; final cleanup leaves no owned processes, sockets, Playwright scratch directories, or profiles. Warm-restart scrollback passes on Linux and macOS. Cloud adds 16 runner contracts; the separate lost-reply fix reduces its four-case suite from 17.52s to 10.97s and is not included in the full-command result above. Counterbalanced import comparisons retain byte-identical case bodies; Node comparisons also pass.
A serial local delivery-model comparison retained 228 MiB less heap after GC with the same 10 cases, 2,000 schedules, 120,000 steps, and 75,191,101 PTY bytes. The timing difference was only about 75 ms; no material runtime or hosted saving is claimed.
Full root typecheck, changed-code quality and focused lint/format pass. Cloud typecheck/build stages and independent reviews pass. The scheduling importer/sequencer/planner contracts pass 32 cases; all 375 cases across the 12 script suites referencing the affected workflows pass. Upload always starts before foreground native preparation, followed by a bare wait; older-ref fallback is preserved. Normal PRs retain default-enabled relay integration, with explicit opt-out confined to manual comparisons. Full E2E typecheck has existing errors outside the changed files.
Hosted qualification for exact head
d5f647565852ab49fc789e79e96bb48b86813f02: PR run 38655504365, source2f446371e395b8c6525b92dfe25663a676848bf9, five successful head workflows and all 50 checks complete (29 success, 21 expected skips). The full PR aggregate passes. Original ZIP hashes, all 20 unit reports, exact merge parents, independent source inventory and recomputed assignments are verified: 12,171 passed modules, 151 skipped, zero errors, full 12,322-file coverage. All 12,312 common modules retain their prior status; ten passed modules were added and four documented cosmetic modules removed.The combined observation is 7.90 runner-minutes lower (11.39%). It includes setup, native preparation, caches, uploads and post steps, at unchanged capacity. Package commands, all 78 test-file blobs, case counts and skips match; Linux retains all nine moved Electron suites. Historical source/workload, unit images and cache warmth differ, so this is not an isolated causal measurement. The original 48.57-minute unit baseline is only 0.13 minute below this result. The selected auxiliary workflows and E2E specs differ; their unadjusted totals are not counted as comparable savings. Ordinary transform caching remains off pending the manual same-source comparison.
Review
Independent reviews checked assertions, hidden-window policy, cleanup/persistence, native ABI ordering, cache validation, workflow routing, and cross-platform/remote assumptions. Full unit selection remains enabled.
Deliberately removed coverage: Pull/Sync/Publish icon choices, conflict-warning wrapping, default-font summary wording, and exact preview demo text/ANSI/final cursor placement. Commit actions, warning generation/localization, selected-language summaries, preview lifecycle, and real Monaco setup/ownership retain coverage. The two mocked-editor suites no longer incidentally load real Monaco setup.
Agent skill upstream boundary
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)