fix(dashboard): settle multi-select Other before input#2302
Conversation
Full Suite shard 3 failed Other-only multi-select when checkbox click did not commit isOtherSelected before findByTestId under CI load. Wait for checked state and apply findByTestId across remaining Other planning-flow paths.
📝 WalkthroughWalkthroughUpdated PlanningModeModal planning-flow tests to await asynchronously rendered Other inputs, verify checkbox state before assertions, and validate Continue-button behavior after text changes. ChangesPlanning mode test synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
Greptile SummaryThis PR changes how planning-flow tests wait for the conditional Other input.
Confidence Score: 5/5The test behavior remains covered, but the documented flaky case must follow the repository's quarantine process.
packages/dashboard/app/components/tests/PlanningModeModal.planning-flow.test.tsx Important Files Changed
Reviews (1): Last reviewed commit: "fix(dashboard): settle multi-select Othe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| const otherCheckbox = within(screen.getByTestId("planning-option-other")).getByRole("checkbox"); | ||
| fireEvent.click(otherCheckbox); | ||
| await waitFor(() => expect(otherCheckbox).toBeChecked()); |
There was a problem hiding this comment.
The linked CI timeout is handled by adding polling through waitFor and findByTestId. AGENTS.md lines 144–151 prohibit retries or stabilization waits for observed flaky tests and require quarantine with the failing run instead, so this leaves the known flake active outside the repository's required quarantine process.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🤖 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.
Inline comments:
In
`@packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx`:
- Around line 706-714: Quarantine the flaky planning-flow test by adding the
required ledger entry and matching Vitest exclusion, rather than using polling
retries. In
packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx
lines 706-714, remove waitFor and restore synchronous checkbox validation before
applying the test exclusion. At lines 359, 607, 646-649, and 906-907 of the same
file, replace findByTestId with getByTestId and remove await; these sites are
part of the same deterministic test cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3038fbc9-bcf9-4469-8d77-1a023449685c
📒 Files selected for processing (1)
packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx
| FNXC:PlanningModeOptions 2026-07-18-14:00: | ||
| Full Suite shard 3 (29646721723) timed out finding planning-other-input after a bare | ||
| checkbox click under CI load. Wait for the multi-select Other checkbox to be checked, | ||
| then findByTestId the input (extends FN-8245 settle discipline). | ||
| */ | ||
| const otherCheckbox = within(screen.getByTestId("planning-option-other")).getByRole("checkbox"); | ||
| fireEvent.click(otherCheckbox); | ||
| await waitFor(() => expect(otherCheckbox).toBeChecked()); | ||
| expect(continueButton).toBeDisabled(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Quarantine flaky tests instead of appeasing them with polling loops.
The FNXC comment at this anchor explicitly states these changes address a CI timeout by introducing waitFor and findByTestId. However, findByTestId and waitFor rely on polling loops and retries under the hood. As per coding guidelines, you must "Quarantine flaky tests immediately by adding the required ledger entry and matching Vitest exclusion in the same commit. Never appease flakes with larger timeouts, retries... do not add... polling loops".
Because fireEvent flushes plain React state updates synchronously via act(), getByTestId is the correct deterministic approach; the CI timeout reveals a true underlying flake (such as test bleed or environment resource starvation) that must be quarantined rather than masked.
packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L706-L714: Remove thewaitForretry, revert to synchronous state validation, and quarantine the test (e.g., via a.skipexclusion and a ledger entry).packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L359-L359: RevertfindByTestIdback to the synchronousgetByTestIdand removeawait.packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L607-L607: RevertfindByTestIdback to the synchronousgetByTestIdand removeawait.packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L646-L649: RevertfindByTestIdback to the synchronousgetByTestIdand removeawait.packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L906-L907: RevertfindByTestIdback to the synchronousgetByTestIdand removeawait.
📍 Affects 1 file
packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L706-L714(this comment)packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L359-L359packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L607-L607packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L646-L649packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx#L906-L907
🤖 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
`@packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx`
around lines 706 - 714, Quarantine the flaky planning-flow test by adding the
required ledger entry and matching Vitest exclusion, rather than using polling
retries. In
packages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsx
lines 706-714, remove waitFor and restore synchronous checkbox validation before
applying the test exclusion. At lines 359, 607, 646-649, and 906-907 of the same
file, replace findByTestId with getByTestId and remove await; these sites are
part of the same deterministic test cleanup.
Source: Coding guidelines
1 participant
Summary
planning-other-input.toBeChecked(), thenfindByTestId; apply same settle to remaining Other paths.Test plan
Summary by CodeRabbit