Move servicing event journeys to Cucumber - #1921
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a servicing hold BDD specification and Cucumber steps covering visibility, duplication, deletion, capacity restoration, and cost recording. It also expands mutation-route assertions, updates story catalog expectations, and removes obsolete browser-based servicing helpers. ChangesServicing hold acceptance coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
🤖 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 `@specs/servicing/hold-and-cost.feature`:
- Line 2: Change the feature tag in specs/servicing/hold-and-cost.feature at
lines 2-2 from `@owner`:attendees to `@owner`:servicing. Update
test/scripts/specs/catalog.test.ts at lines 12-23 to assert this story’s
servicing owner or verify that `@owner`:servicing selection includes it,
preventing future tag mismatches.
In `@test/integration/servicing/route-guards.test.ts`:
- Around line 93-100: Remove the unrelated createTestServicingEvent assertion
from the delete test after the deletion assertion, including the "unused" event
creation and positive-ID check. Keep the test focused solely on verifying
deletion; cover any creation-after-deletion invariant separately if required.
- Around line 113-119: Strengthen the assertions in the redirect-copy test
around the Location header and copied servicing event: assert that the header
exists, the servicing ID match and capture are present, the copied event exists,
and its first booking exists before reading fields. Then use the established
values directly with justified non-null narrowing, removing ?? "", optional
chaining, and fallback-driven NaN/undefined failures while preserving the
existing value checks.
In `@test/specs/steps/servicing-hold.ts`:
- Around line 152-158: Strengthen the assertions in
test/specs/steps/servicing-hold.ts at lines 152-158 and 181-187: identify the
rendered servicing entries and verify that exactly two are Annual Inspection
holds, rather than counting arbitrary servicing links and one label; for the
Boiler part cost record, assert the formatted amount is exactly 90.00, avoiding
substring matches such as 190.00.
- Line 107: Update the override object passed by the scenario in
servicing-hold.ts to use the helper’s camelCase key maximumDaysAfter instead of
maximum_days_after, so createHoldViaProduction applies the intended 1000-day
booking horizon.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca9bd074-471b-48ac-98cd-e7f8206e5dab
📒 Files selected for processing (8)
specs/owners.jsonspecs/servicing/hold-and-cost.featuretest/e2e/servicing/e2e-narrative.test.tstest/integration/servicing/route-guards.test.tstest/scripts/specs/catalog.test.tstest/specs/steps/servicing-hold.tstest/specs/support/world.tstest/test-utils/servicing.ts
💤 Files with no reviewable changes (2)
- test/e2e/servicing/e2e-narrative.test.ts
- test/test-utils/servicing.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edc7e2a79f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
edc7e2a to
bc91f4c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc91f4c28a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/specs/steps/servicing-hold.ts (1)
153-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert exactly two duplicated holds.
Line 158 accepts an erroneous duplicate operation that creates three or more holds. Require the expected count of two.
Proposed fix
- expect(count).toBeGreaterThanOrEqual(2); + expect(count).toBe(2);As per coding guidelines, tests must use strong non-tautological assertions.
🤖 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 `@test/specs/steps/servicing-hold.ts` around lines 153 - 159, Update the assertion in the “the admin dashboard shows two Annual Inspection holds” step to require the match count to equal exactly 2 rather than accepting any count of 2 or greater. Keep the existing page rendering and match-count calculation unchanged.Source: Coding guidelines
🤖 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 `@test/integration/servicing/route-guards.test.ts`:
- Around line 87-95: Update the delete redirect test and the adjacent duplicate
test to validate required response fields before using them: assert that the
delete response contains a location header before constructing the URL, and
assert the duplicate test’s capture and first-booking values before using them.
Replace unchecked non-null assertions with contextual assertions followed by
justified narrowing, while preserving the existing redirect and booking
expectations.
---
Duplicate comments:
In `@test/specs/steps/servicing-hold.ts`:
- Around line 153-159: Update the assertion in the “the admin dashboard shows
two Annual Inspection holds” step to require the match count to equal exactly 2
rather than accepting any count of 2 or greater. Keep the existing page
rendering and match-count calculation unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dd8c406-9daa-4066-a31a-bf386918cfd0
📒 Files selected for processing (8)
specs/owners.jsonspecs/servicing/hold-and-cost.featuretest/e2e/servicing/e2e-narrative.test.tstest/integration/servicing/route-guards.test.tstest/scripts/specs/catalog.test.tstest/specs/steps/servicing-hold.tstest/specs/support/world.tstest/test-utils/servicing.ts
💤 Files with no reviewable changes (2)
- test/e2e/servicing/e2e-narrative.test.ts
- test/test-utils/servicing.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51f02f2db5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954621a04d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const assertRedirectPathname = ( | ||
| response: Response, | ||
| expected: string, | ||
| ): void => { | ||
| expect(response.status).toBe(302); |
There was a problem hiding this comment.
This adds assertRedirectPathname beside assertRedirectTo at lines 301–304, leaving the validation tests on the weaker substring check while new tests use exact pathname semantics. Keeping two helpers for the same redirect contract lets future tests choose the assertion that accepts broken nested destinations; replace the existing helper and migrate its callers instead of maintaining parallel implementations.
AGENTS.md reference: AGENTS.md:L72-L72
Useful? React with 👍 / 👎.
| const findForms = (html: string): Array<{ action: string; body: string }> => { | ||
| const re = /<form\s[^>]*action="([^"]*)"[^>]*>([\s\S]*?)<\/form>/gi; | ||
| const forms: Array<{ action: string; body: string }> = []; | ||
| for (const match of html.matchAll(re)) { | ||
| forms.push({ action: match[1]!, body: match[2]! }); |
There was a problem hiding this comment.
This introduces a second HTML form finder and submission pipeline beside the one in test/test-utils/test-browser.ts: the existing implementation already parses forms, decodes actions, identifies the clicked button, preserves repeated controls, and applies overrides, while this copy separately reimplements those responsibilities through line 69. The two paths have already diverged—for example, this one neither decodes the action nor submits a clicked button's name/value and collapses repeated fields through Object.fromEntries—so future servicing-form changes can make Cucumber send a request different from a browser; expose and reuse the underlying form mechanism instead.
AGENTS.md reference: AGENTS.md:L72-L72
Useful? React with 👍 / 👎.
1 participant
Summary
assertRedirectPathnamehelper for exact redirect-path assertionsWhy
The six servicing e2e tests were narrative scenarios driven through a TestBrowser. Four described observable operator journeys; two were weak (a custom-question test that never created a question, and a URL-poke test already covered by direct route-guard tests). The four strong scenarios now live in one plain-language Feature.
Rules covered
Validation
nix develop -c deno task specs:files specs/servicing/hold-and-cost.feature: 4 scenarios and 21 steps passednix develop -c deno task precommitgit diff --check origin/main...HEAD