test(meshcore): fix renderer-ui setup-abort reconnect contract - #822
Conversation
The `attemptMeshcoreReconnect treats setup AbortError as superseded reconnect` source contract sliced a fixed 900-char window after the `isMeshcoreSetupAbortError(err)` marker. The PR-review fix that adds `lateTransport.cleanup(openedDriverIdentityId)` (plus its comment) before `return 'defer'` pushed the defer past that window, failing renderer-ui CI. Delimit the abort branch at the non-abort `return 'retry'` path instead of a char count, and additionally assert the branch performs the late-transport cleanup, so the contract tracks intent without being length-brittle.
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe reconnect regression test now identifies the setup-abort branch by the subsequent retry return. It also verifies cleanup of the transport opened by the superseded attempt before deferred retry. ChangesReconnect regression coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts-531-533 (1)
531-533: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert cleanup ordering, not only branch membership.
toContain()confirms that both expressions exist inabortBlock. It does not confirm thatlateTransport.cleanup(openedDriverIdentityId)appears beforereturn 'defer'. A regression that returns before cleanup would still pass this test. Compare both indexes and assert that the cleanup index is smaller.As per path instructions, this source-contract test must verify late-transport cleanup before deferred retry.
Suggested assertion
- expect(abortBlock).toContain("return 'defer'"); - // Setup abort must clean up any transport this doomed attempt opened before deferring. - expect(abortBlock).toContain('lateTransport.cleanup(openedDriverIdentityId)'); + const deferIdx = abortBlock.indexOf("return 'defer'"); + const cleanupIdx = abortBlock.indexOf('lateTransport.cleanup(openedDriverIdentityId)'); + expect(deferIdx).toBeGreaterThanOrEqual(0); + expect(cleanupIdx).toBeGreaterThanOrEqual(0); + expect(cleanupIdx).toBeLessThan(deferIdx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts` around lines 531 - 533, Update the source-contract assertions for abortBlock in the reconnect test to compare the positions of lateTransport.cleanup(openedDriverIdentityId) and return 'defer'. Assert that the cleanup expression’s index is smaller, while retaining the existing presence checks as appropriate.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts`:
- Around line 531-533: Update the source-contract assertions for abortBlock in
the reconnect test to compare the positions of
lateTransport.cleanup(openedDriverIdentityId) and return 'defer'. Assert that
the cleanup expression’s index is smaller, while retaining the existing presence
checks as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 9cf9d153-c165-4d3e-9855-ffe5d99bea10
📒 Files selected for processing (1)
src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts
Strengthen the setup-abort reconnect source contract to check ordering, not just presence: compare the indices of lateTransport.cleanup(openedDriverIdentityId) and return 'defer' and assert cleanup runs first, so a future edit cannot move the defer ahead of the late-transport cleanup.
Summary
Fixes the
Coverage (renderer-ui)CI failure from run 31261707882 (PR #821). One source-contract test failed:useMeshcoreRuntime.reconnect.test.ts › attemptMeshcoreReconnect treats setup AbortError as superseded reconnect.isMeshcoreSetupAbortError(err)marker and asserted it containedreturn 'defer'.await lateTransport.cleanup(openedDriverIdentityId)(plus its explanatory comment) inside the setup-abort branch before the defer — correct behavior (a doomed attempt's late-opened transport must be cleaned up before deferring), but it pushedreturn 'defer'past the 900-char window, so the assertion failed.return 'retry'path instead of a char count, and additionally assert the branch performs the late-transport cleanup. This tracks intent without being length-brittle.No production code changed — test-only.
Test plan
pnpm exec vitest run src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts(64/64 pass)renderer-uiproject locally: 3126/3126 pass after the fix (was 1 failed)check:i18n:branch, staged Vitest incl.sourcePolicy) greenCoverage (renderer-ui)greenSummary by CodeRabbit