[Fix] Subtask approval E2E times out after task restoration - #1434
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
934ef0d to
993ebd9
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (9)Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases. Check cleanup and deterministic async behavior and prefer shared typed test helpe...⚙️ CodeRabbit configuration file Files:
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths. Verify promises and errors are handled, existing helpers are reused, and new code introduces no `any`, unjustified dou...⚙️ CodeRabbit configuration file Files:
Reserve end-to-end coverage for behavior that requires the real VS Code host, workspace APIs, extension activation, webview messaging, file watchers, or a full workflow. Keep detailed protocol, parsing, storage, retry, and edge cases at low...⚙️ CodeRabbit configuration file Files:
Act as an adversarial second-opinion reviewer. Verify PR claims against implementation, contracts, and tests. Trace changed inputs through normal, boundary, error, cancellation, retry, and default paths and their consumers. Seek plausible c...⚙️ CodeRabbit configuration file Files:
Add focused tests for UI binding and save behavior, persistence or normalization, and the value returned by `getStateToPostToWebview()`, including true and false/unset cases when defaults could hide omissions.📄 CodeRabbit inference engine (AGENTS.md) Files:
Fix lint violations in new TypeScript code instead of suppressing them.📄 CodeRabbit inference engine (AGENTS.md) Files:
Keep e2e tests focused on high-value cross-boundary smoke coverage; do not place detailed protocol, parsing, storage, retry, or edge-case assertions there when lower-level tests can cover them.📄 CodeRabbit inference engine (AGENTS.md) Files:
Prefer package-local unit or integration tests over E2E tests; use E2E tests for real extension-host boundaries and full-workflow smoke checks rather than detailed service, protocol, or UI assertions.📄 CodeRabbit inference engine (apps/vscode-e2e/AGENTS.md) Files:
Use `mock.addFixture()` with a `RegExp` match for requests that cannot be matched by a stable substring; programmatic fixtures are replay-only and are not recorded.📄 CodeRabbit inference engine (apps/vscode-e2e/AGENTS.md) Files:
🔇 Additional comments (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe VS Code E2E changes enforce floating-promise checks, handle asynchronous failures, rewrite polling cleanup, and await task resumption before approval assertions. ChangesVS Code E2E promise handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change synchronizes asynchronous E2E task restoration and strengthens promise-handling checks without introducing a known production-impacting issue; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the root cause, implementation, promise-handling changes, timer cleanup, testing steps, and scope. It references related issues and completes the required sections. The issue-link checklist remains unchecked, but the description is otherwise substantially complete. Full details: Regression EvidenceExplanation The pending-subtask E2E test provides focused coverage for the awaited parent and child restorations. However, Resolution Add lowest-layer tests for Full details: Trust And Persistence InvariantsExplanation PASS. The changed paths do not introduce a secret/PII leak, input trust bypass, approval bypass, or persisted-state loss.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
apps/vscode-e2e/eslint.config.mjsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/vscode-e2e/src/runTest.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). apps/vscode-e2e/src/suite/index.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
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}
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-actions
Bot
added
the
awaiting-review
2 participants
Related GitHub Issue
Follow-up to #1223 and #1320.
Description
Await both asynchronous task restorations in the pending-subtask-approval E2E test before observing or approving the restored action. Enable type-aware
@typescript-eslint/no-floating-promisesacross the E2E module so this class of race fails lint, and make the existing async entry points, event handlers, test registrations, and polling helper explicit about promise ownership. The polling helper now also clears its timers deterministically on success, rejection, and timeout.Test Procedure
pnpm --filter @roo-code/vscode-e2e lint.pnpm --filter @roo-code/vscode-e2e check-types.pnpm --filter @roo-code/vscode-e2e themes:test.pnpm test.Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md-> "When a UI change needs a snapshot". Not applicable; this is test synchronization only.Visual Snapshots
Not applicable. There is no UI change.
Videos (interaction / animation only)
Not applicable.
Documentation Updates
Additional Notes
The original failure timed out after restoring the parent but before creating the child. The test passed 10/10 in isolation before the fix, confirming the flake depended on ordered lifecycle timing rather than approval-recovery behavior. Enabling the new lint rule surfaced eight existing call sites; all were resolved rather than baselined or suppressed.
Get in Touch
Follow up in the linked Discord thread or mention
@roomoteon this PR.