Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughAdded a dedicated Merge Risk: ⚪ Minimal · up to This change only reorganizes existing dispatch-liveness tests without changing production behavior or assertions; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is complete and relevant. It explains the change, reason, linked issue, testing, platform limitations, review considerations, and checklist status. The missing Agent skill upstream boundary section is non-critical for this test-only change.
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dade61aa-9bd9-41f8-82e9-d53424b88d39
📥 Commits
Reviewing files that changed from the base of the PR and between e4e54a1 and ffd2794b1063d52605e6f537df3430593573e29e.
📒 Files selected for processing (2)
src/main/runtime/orchestration/db-dispatch-liveness.test.tssrc/main/runtime/orchestration/db.test.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/orchestration/db.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.
|
Cross-linking for context, @Tauri-EPO: I landed your #15165 assertion fix on an internal branch (#15869) so it could run full CI, which means That makes this split more useful, not less. It's still on my list. Nothing needed from you here; just flagging that |
db.test.ts landed at exactly 800 effective lines — the max-lines ceiling for **/*.test.ts — which closed the file to any further addition until #14889 splits it, and forced the failDispatch sibling of this same invariant into dispatch-failure-idempotency.test.ts where its name does not fit. Move both straggler cases into db-heartbeat-straggler-guard.test.ts. The assertions and the seeded-heartbeat fixture are unchanged; db.test.ts drops to 790 effective lines (10 of headroom) and this PR no longer has a merge-order dependency on #14889. Co-authored-by: Tauri-EPO <enrico.pin@gmail.com>
ffd2794 to
70b0f8b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
70b0f8b to
29383c8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
db.test.ts sits just under the 800-line max-lines budget, leaving no room for new coverage. Move the recordHeartbeat and getStaleDispatches tests into a new db-dispatch-liveness.test.ts — a coherent unit around detecting whether a dispatched task is still alive — and leave the dispatch-context CRUD coverage behind, which is a different concern. No assertions or fixture behaviour changed; only file location.
29383c8 to
e613a60
Compare
|
Rebased onto current
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Greptile SummaryThe PR moves dispatch-liveness tests from the near-limit orchestration database test file into a dedicated test module without changing production behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/main/runtime/orchestration/db-dispatch-liveness.test.ts | Adds the extracted dispatch-liveness test suite with its fixture helper and regression rationale intact. |
| src/main/runtime/orchestration/db.test.ts | Removes the relocated dispatch-liveness tests while retaining the unrelated orchestration database coverage. |
Reviews (2): Last reviewed commit: "test(orchestration): move the #8452 rati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…heir tests Review follow-up (Greptile): the regression comments stayed behind in db.test.ts, orphaned above an unrelated thread-message test; each block now precedes the exact relocated test it documents.
|
Closing: the premise no longer holds. #24043 retired redundant cases and brought |
ELI5
db.test.tsis nearly at the 800-line lint ceiling, so there is no room to add coverage to it. This moves one coherent group of tests into their own file. Nothing else changes.What Changed
Moved the
recordHeartbeatandgetStaleDispatchestests out ofsrc/main/runtime/orchestration/db.test.tsinto a newsrc/main/runtime/orchestration/db-dispatch-liveness.test.ts.The dispatch-context CRUD coverage stays where it is — that is a different concern from "is this dispatched task still alive", which is what the moved tests are about.
No assertions, test names, or fixture behaviour changed. This is a file move.
Why
db.test.tssits a handful of lines under themax-linesbudget of 800.AGENTS.mdforbids both adding aneslint-disable max-linesand bumping a per-file budget, so the only way to add coverage near it is to split first. Recent commits onmain(refactor(daemon,runtime): split daemon, pty and rpc modules under the max-lines budget, and its sibling) do exactly this, so this follows the direction the repo is already moving.Splitting by domain rather than by line count keeps the result meaningful: the new file is everything about detecting whether a dispatched task is still alive.
Linked Issue
Prerequisite for #14907 — it unblocks adding dispatch-liveness coverage without tripping the lint budget. Not
Fixes, because this PR is only the split; it implements none of that issue.Visual Proof
N/A— test file reorganisation, no runtime code and no user-visible behaviour.Testing
pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orchestration/db.test.ts src/main/runtime/orchestration/db-dispatch-liveness.test.ts→ 61 passed, which is the same count and the same tests as before the move.pnpm exec oxlint src/main/runtime/orchestration/→ clean.pnpm run lint,pnpm typecheckandpnpm build→ all pass locally.Tested on Windows 11.
No tests added or changed — every assertion is byte-identical to its previous form, including the
#8452regression comments.AI Disclosure
Written with Claude Opus 5 via Claude Code. The decision to split and the choice of grouping were made in collaboration with the author; the mechanical move and the verification runs were agent-assisted.
Review
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Note on the last box:
pnpm lintandpnpm typecheckpass locally. The fullpnpm testsuite cannot start on Windows from a clean install of this repo — see #14918. Briefly:windows-native-registry@3.2.2installs with an emptybuild/, so the Windows branch ofensure-native-runtimerefuses to run the suite. Not specific to this branch, and not a Node version problem (reproduced identically on Node 22 and 24).Running vitest directly, bypassing that wrapper, the suite has pre-existing failures on this Windows machine that are unrelated to this change (missing POSIX shell,
spawnSync … EINVAL, assertions expecting/and receiving\). I verified attribution by running the whole suite on the merge base and on this branch: both fail the same set, and this branch adds none.Author