Organize tests and strengthen covered boundaries#1861
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR adds broad unit, integration, API, database, migration, payment, email, i18n, form, routing, and UI coverage. It also tightens validation behavior, introduces nullable lookup names and ChangesValidation and shared contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 23
🤖 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/features/admin/attendees-csv/questions.test.ts`:
- Around line 7-23: Promote the local question fixture to the shared `#test-utils`
module, preserving the existing QuestionWithAnswers construction and distinct
answer-id behavior. Remove the duplicate question helper from questions.test.ts
and update both questions.test.ts and attendee-answers.test.ts to import and
reuse the shared helper.
In `@test/features/tickets/token-utils/parsing.test.ts`:
- Around line 53-60: Replace the extractTokenSegment test that relies on an
unescaped regex prefix and an empty capture group with a case using a literal
route prefix. Assert only the observable token-extraction behavior for valid
production-style prefixes, and remove comments or expectations tied to match[1],
regex grouping, or the ?? versus || implementation detail.
In `@test/integration/db/listings/crud.test.ts`:
- Around line 99-102: Remove the duplicate “getListingWithCount returns null for
missing listing” test near the referenced block, retaining the existing
equivalent test elsewhere in the test suite and leaving unique coverage
unchanged.
In `@test/integration/server/admin-api/listings/create.test.ts`:
- Around line 121-142: Update the customisable-days listing validation used by
the admin listings POST flow and the test “creates a customisable-days listing
with day_prices” so any invalid day_prices key or value causes a 400 response
instead of silently dropping malformed entries. Preserve successful creation
only for payloads where every day_prices entry is valid, and assert the rejected
response rather than a partial listing.
In `@test/integration/server/admin-api/listings/lifecycle.test.ts`:
- Around line 134-155: Strengthen the lifecycle tests around the reactivation
and media-deletion cases by reloading the affected listing from persistent
storage after each API call, then assert the observable state: active must be
true after reactivation and the deleted media must be null. Keep the existing
response assertions, and add mutation-resistant persistence checks in the tests
covering the reactivation case and the media deletion case.
In `@test/integration/server/attributes/detail.test.ts`:
- Around line 38-64: Update the test "links each option to its edit page with a
listings count" to assert each option link and its expected listings count
within the same table row, ensuring Easy is paired with 1 and Hard with 0.
Replace the independent cell-content checks with mutation-resistant row-scoped
assertions while preserving the existing delete-link assertion.
In `@test/integration/server/listings/create-validation.test.ts`:
- Around line 162-168: Update the test “accepts max_price equal to unit_price +
100” to create the listing with can_pay_more enabled, ensuring it exercises the
conditional equality boundary. Keep the existing max_price and unit_price
values, and retain the assertion that max_price is 1100.
- Around line 17-26: Strengthen the “creates listing with unit_price when
authenticated” test by loading the created listing after the redirect and
asserting its persisted unit_price equals the submitted 10.00 value. Reuse the
established listing lookup or database assertion helper and retain the existing
redirect-status assertion.
In `@test/integration/server/listings/date-and-location.test.ts`:
- Around line 92-109: Update the test “CSV export includes Listing Date and
Listing Location columns” to request and inspect the generated CSV instead of
the HTML export page. Reuse the CSV assertion helper used by the negative case,
and assert that the CSV contains “Listing Date,” “Listing Location,” and
“Village Hall.”
In `@test/integration/server/listings/index-and-new.test.ts`:
- Around line 43-60: Strengthen the assertions in the authenticated type-picker
test by explicitly asserting that the “Custom / advanced” card exists before
comparing its position with “Import from file.” Update the ordering check around
the existing html.indexOf calls so a missing custom card causes the test to fail
rather than allowing a -1 index to pass.
In `@test/integration/server/news/admin.test.ts`:
- Around line 237-255: Update both rejected-update tests around the “missing
name” and “invalid slug” cases to assert that the persisted post’s slug remains
equal to the original post.slug after the request. Keep the existing name
assertions, and ensure the invalid-slug test specifically verifies the submitted
invalid value was not stored.
In `@test/integration/server/parents-gate/qty-matching.test.ts`:
- Around line 121-122: Update the rejected-booking assertions in the relevant
test cases to always verify that every child has no booking, and also verify
stranger has no booking when that entity is present; remove the
extraChildIdsZero option and its conditional assertion path so atomicity is
asserted uniformly for every rejection.
In `@test/integration/server/settings/calendar-feeds.test.ts`:
- Around line 16-19: Update the calendar-feeds settings test around
settings.calendarFeedsGroupBy so the unsupported persisted value "unknown" is
expected to trigger a boundary validation error instead of returning
"attendees". Preserve the cache invalidation and key-loading setup, and assert
the rejection/failure from loading or accessing the setting.
In `@test/integration/server/settings/payment-provider.test.ts`:
- Around line 2-12: Remove the unnecessary afterEach global cleanup hook and the
setDemoModeForTest import from the test module. Keep describeWithEnv and the
contained tests unchanged, relying on isolated test environments without
shared-state cleanup.
- Around line 15-23: Extend the payment-provider integration tests around
adminFormPost to verify the settings store persists the selected provider after
each successful request, including “square,” “stripe,” and the disabled state.
Assert the store’s observable value with mutation-resistant checks while
retaining the existing status and flash assertions.
In `@test/integration/server/users/invite.test.ts`:
- Around line 84-96: Strengthen the user-invite integration tests by locating
the persisted user by username and asserting newmanager has the manager role,
rather than checking only the total count. In the missing-username test, assert
the expected validation flash and redirect target, then verify the persisted
user count remains unchanged; retain status assertions but do not rely on 302
alone.
In `@test/integration/server/users/manage.test.ts`:
- Around line 42-61: Strengthen the user-deletion tests around adminFormPost and
getAllUsers by asserting usernames rather than only row counts: after successful
deletion, verify deleteme is absent and the current owner remains; in the
missing-confirmation and incorrect-confirmation cases, verify the targeted user
remains present and the owner is intact.
In `@test/integration/server/webhooks/acknowledge-edge-cases.test.ts`:
- Around line 20-22: Remove the global afterEach hooks that call
resetStripeClient in
test/integration/server/webhooks/acknowledge-edge-cases.test.ts:20-22 and
test/integration/server/webhooks/can-pay-more-single-ticket.test.ts:22-24.
Introduce and reuse a shared wrapper around each Stripe-backed test body that
invokes resetStripeClient in a try/finally block, ensuring cleanup occurs per
test without BDD hooks.
In `@test/integration/server/webhooks/can-pay-more-single-ticket.test.ts`:
- Around line 92-117: Update the createTestListing call in the “single-ticket
can_pay_more keeps and refunds amount above maximum price” test to explicitly
configure a maxPrice below 20000, using the fixture’s established maximum-price
symbol or default rather than hard-coding an implicit production constant. Keep
the existing overpayment scenario and assertions unchanged.
In `@test/integration/test-utils/describe-with-env-storage.test.ts`:
- Around line 68-70: Rewrite the comments in
test/integration/test-utils/describe-with-env-storage.test.ts at lines 68-70 and
91-97 to describe current behavior: the first must state that explicit local
storage configuration takes precedence over ambient CDN credentials, and the
second must state that storage configuration is scoped and must not mutate
environment variables. Remove implementation-history references without changing
the test behavior.
- Around line 35-65: Remove the shared observedDir variable and the local
backend afterAll hook. Update each test within describeWithEnv to assert its own
temporary directory is removed and getTestStoragePath() is null after test
cleanup, using an isolated test or helper that does not depend on execution
order or shared mutable state.
- Around line 8-13: Update dirExists to suppress errors only when Deno.statSync
reports a missing path; rethrow permission and all other filesystem errors so
teardown assertions cannot silently pass. Preserve returning isDirectory for
existing paths and false for not-found paths.
In `@test/shared/payment-helpers/limits.test.ts`:
- Around line 205-209: Rewrite the comment in the “leaves a too-long
thank_you_url untouched” test to describe only current behavior:
buildItemsMetadata bounds thank_you_url before signing, while
enforceMetadataLimits intentionally leaves an over-cap value unchanged. Remove
references to moving or no longer performing the operation.
🪄 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: 1789426b-90bb-43ff-a482-af56119b63c7
📒 Files selected for processing (66)
test/doc/database.test.tstest/features/admin/attendees-csv/questions.test.tstest/features/admin/ledger/names.test.tstest/features/admin/refunds/provider/batch.test.tstest/features/api/helpers/json-body.test.tstest/features/tickets/token-utils/parsing.test.tstest/integration/db/listings/crud.test.tstest/integration/merge/attendee-merge/checkout-stage-cleanup.test.tstest/integration/server/admin-api/listings/create.test.tstest/integration/server/admin-api/listings/groups.test.tstest/integration/server/admin-api/listings/lifecycle.test.tstest/integration/server/admin/attendees/create-atomic.test.tstest/integration/server/attendees/merge-conflicts.test.tstest/integration/server/attributes/detail.test.tstest/integration/server/bulk-actions/reactivate.test.tstest/integration/server/bulk-email/preview.test.tstest/integration/server/bulk-email/previous-bookings.test.tstest/integration/server/holidays/entity-page.test.tstest/integration/server/listings/assign-built-site.test.tstest/integration/server/listings/create-validation.test.tstest/integration/server/listings/daily-type.test.tstest/integration/server/listings/date-and-location.test.tstest/integration/server/listings/deactivate.test.tstest/integration/server/listings/edit-form-and-income.test.tstest/integration/server/listings/index-and-new.test.tstest/integration/server/listings/reactivate.test.tstest/integration/server/listings/show-basics.test.tstest/integration/server/news/admin.test.tstest/integration/server/parents-booking/api-availability.test.tstest/integration/server/parents-gate/auto-fill.test.tstest/integration/server/parents-gate/customisable-fold.test.tstest/integration/server/parents-gate/daily-fold.test.tstest/integration/server/parents-gate/qty-matching.test.tstest/integration/server/public/ticket-terms-checkbox.test.tstest/integration/server/servicing/ledger-validation.test.tstest/integration/server/settings/calendar-feeds.test.tstest/integration/server/settings/column-order.test.tstest/integration/server/settings/payment-provider.test.tstest/integration/server/users/invite.test.tstest/integration/server/users/manage.test.tstest/integration/server/webhooks/acknowledge-edge-cases.test.tstest/integration/server/webhooks/can-pay-more-single-ticket.test.tstest/integration/test-utils/compat.test.tstest/integration/test-utils/describe-with-env-storage.test.tstest/integration/test-utils/record-queries.test.tstest/integration/tooling/cli.test.tstest/integration/ui/templates/admin/ledger/account-label.test.tstest/shared/booking/model/capacity.test.tstest/shared/booking/model/day-counts.test.tstest/shared/booking/model/eligibility.test.tstest/shared/columns/attendee-columns/attendee-answers.test.tstest/shared/db/migrations/schema/admin-feature-triggers.test.tstest/shared/db/settings/theme.test.tstest/shared/db/table/projection.test.tstest/shared/email/send-bulk-email.test.tstest/shared/email/send-email.test.tstest/shared/i18n/loading.test.tstest/shared/interval-gate.test.tstest/shared/lazy-logger.test.tstest/shared/payment-helpers/limits.test.tstest/shared/square-provider/provider.test.tstest/shared/square/retrieve-refund.test.tstest/shared/stripe/webhook-cleanup.test.tstest/shared/stripe/webhook.test.tstest/shared/types/admin-roles.test.tstest/ui/templates/admin/recalculate-rows.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 23
🤖 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/features/admin/attendees-csv/questions.test.ts`:
- Around line 7-23: Promote the local question fixture to the shared `#test-utils`
module, preserving the existing QuestionWithAnswers construction and distinct
answer-id behavior. Remove the duplicate question helper from questions.test.ts
and update both questions.test.ts and attendee-answers.test.ts to import and
reuse the shared helper.
In `@test/features/tickets/token-utils/parsing.test.ts`:
- Around line 53-60: Replace the extractTokenSegment test that relies on an
unescaped regex prefix and an empty capture group with a case using a literal
route prefix. Assert only the observable token-extraction behavior for valid
production-style prefixes, and remove comments or expectations tied to match[1],
regex grouping, or the ?? versus || implementation detail.
In `@test/integration/db/listings/crud.test.ts`:
- Around line 99-102: Remove the duplicate “getListingWithCount returns null for
missing listing” test near the referenced block, retaining the existing
equivalent test elsewhere in the test suite and leaving unique coverage
unchanged.
In `@test/integration/server/admin-api/listings/create.test.ts`:
- Around line 121-142: Update the customisable-days listing validation used by
the admin listings POST flow and the test “creates a customisable-days listing
with day_prices” so any invalid day_prices key or value causes a 400 response
instead of silently dropping malformed entries. Preserve successful creation
only for payloads where every day_prices entry is valid, and assert the rejected
response rather than a partial listing.
In `@test/integration/server/admin-api/listings/lifecycle.test.ts`:
- Around line 134-155: Strengthen the lifecycle tests around the reactivation
and media-deletion cases by reloading the affected listing from persistent
storage after each API call, then assert the observable state: active must be
true after reactivation and the deleted media must be null. Keep the existing
response assertions, and add mutation-resistant persistence checks in the tests
covering the reactivation case and the media deletion case.
In `@test/integration/server/attributes/detail.test.ts`:
- Around line 38-64: Update the test "links each option to its edit page with a
listings count" to assert each option link and its expected listings count
within the same table row, ensuring Easy is paired with 1 and Hard with 0.
Replace the independent cell-content checks with mutation-resistant row-scoped
assertions while preserving the existing delete-link assertion.
In `@test/integration/server/listings/create-validation.test.ts`:
- Around line 162-168: Update the test “accepts max_price equal to unit_price +
100” to create the listing with can_pay_more enabled, ensuring it exercises the
conditional equality boundary. Keep the existing max_price and unit_price
values, and retain the assertion that max_price is 1100.
- Around line 17-26: Strengthen the “creates listing with unit_price when
authenticated” test by loading the created listing after the redirect and
asserting its persisted unit_price equals the submitted 10.00 value. Reuse the
established listing lookup or database assertion helper and retain the existing
redirect-status assertion.
In `@test/integration/server/listings/date-and-location.test.ts`:
- Around line 92-109: Update the test “CSV export includes Listing Date and
Listing Location columns” to request and inspect the generated CSV instead of
the HTML export page. Reuse the CSV assertion helper used by the negative case,
and assert that the CSV contains “Listing Date,” “Listing Location,” and
“Village Hall.”
In `@test/integration/server/listings/index-and-new.test.ts`:
- Around line 43-60: Strengthen the assertions in the authenticated type-picker
test by explicitly asserting that the “Custom / advanced” card exists before
comparing its position with “Import from file.” Update the ordering check around
the existing html.indexOf calls so a missing custom card causes the test to fail
rather than allowing a -1 index to pass.
In `@test/integration/server/news/admin.test.ts`:
- Around line 237-255: Update both rejected-update tests around the “missing
name” and “invalid slug” cases to assert that the persisted post’s slug remains
equal to the original post.slug after the request. Keep the existing name
assertions, and ensure the invalid-slug test specifically verifies the submitted
invalid value was not stored.
In `@test/integration/server/parents-gate/qty-matching.test.ts`:
- Around line 121-122: Update the rejected-booking assertions in the relevant
test cases to always verify that every child has no booking, and also verify
stranger has no booking when that entity is present; remove the
extraChildIdsZero option and its conditional assertion path so atomicity is
asserted uniformly for every rejection.
In `@test/integration/server/settings/calendar-feeds.test.ts`:
- Around line 16-19: Update the calendar-feeds settings test around
settings.calendarFeedsGroupBy so the unsupported persisted value "unknown" is
expected to trigger a boundary validation error instead of returning
"attendees". Preserve the cache invalidation and key-loading setup, and assert
the rejection/failure from loading or accessing the setting.
In `@test/integration/server/settings/payment-provider.test.ts`:
- Around line 2-12: Remove the unnecessary afterEach global cleanup hook and the
setDemoModeForTest import from the test module. Keep describeWithEnv and the
contained tests unchanged, relying on isolated test environments without
shared-state cleanup.
- Around line 15-23: Extend the payment-provider integration tests around
adminFormPost to verify the settings store persists the selected provider after
each successful request, including “square,” “stripe,” and the disabled state.
Assert the store’s observable value with mutation-resistant checks while
retaining the existing status and flash assertions.
In `@test/integration/server/users/invite.test.ts`:
- Around line 84-96: Strengthen the user-invite integration tests by locating
the persisted user by username and asserting newmanager has the manager role,
rather than checking only the total count. In the missing-username test, assert
the expected validation flash and redirect target, then verify the persisted
user count remains unchanged; retain status assertions but do not rely on 302
alone.
In `@test/integration/server/users/manage.test.ts`:
- Around line 42-61: Strengthen the user-deletion tests around adminFormPost and
getAllUsers by asserting usernames rather than only row counts: after successful
deletion, verify deleteme is absent and the current owner remains; in the
missing-confirmation and incorrect-confirmation cases, verify the targeted user
remains present and the owner is intact.
In `@test/integration/server/webhooks/acknowledge-edge-cases.test.ts`:
- Around line 20-22: Remove the global afterEach hooks that call
resetStripeClient in
test/integration/server/webhooks/acknowledge-edge-cases.test.ts:20-22 and
test/integration/server/webhooks/can-pay-more-single-ticket.test.ts:22-24.
Introduce and reuse a shared wrapper around each Stripe-backed test body that
invokes resetStripeClient in a try/finally block, ensuring cleanup occurs per
test without BDD hooks.
In `@test/integration/server/webhooks/can-pay-more-single-ticket.test.ts`:
- Around line 92-117: Update the createTestListing call in the “single-ticket
can_pay_more keeps and refunds amount above maximum price” test to explicitly
configure a maxPrice below 20000, using the fixture’s established maximum-price
symbol or default rather than hard-coding an implicit production constant. Keep
the existing overpayment scenario and assertions unchanged.
In `@test/integration/test-utils/describe-with-env-storage.test.ts`:
- Around line 68-70: Rewrite the comments in
test/integration/test-utils/describe-with-env-storage.test.ts at lines 68-70 and
91-97 to describe current behavior: the first must state that explicit local
storage configuration takes precedence over ambient CDN credentials, and the
second must state that storage configuration is scoped and must not mutate
environment variables. Remove implementation-history references without changing
the test behavior.
- Around line 35-65: Remove the shared observedDir variable and the local
backend afterAll hook. Update each test within describeWithEnv to assert its own
temporary directory is removed and getTestStoragePath() is null after test
cleanup, using an isolated test or helper that does not depend on execution
order or shared mutable state.
- Around line 8-13: Update dirExists to suppress errors only when Deno.statSync
reports a missing path; rethrow permission and all other filesystem errors so
teardown assertions cannot silently pass. Preserve returning isDirectory for
existing paths and false for not-found paths.
In `@test/shared/payment-helpers/limits.test.ts`:
- Around line 205-209: Rewrite the comment in the “leaves a too-long
thank_you_url untouched” test to describe only current behavior:
buildItemsMetadata bounds thank_you_url before signing, while
enforceMetadataLimits intentionally leaves an over-cap value unchanged. Remove
references to moving or no longer performing the operation.
🪄 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: 1789426b-90bb-43ff-a482-af56119b63c7
📒 Files selected for processing (66)
test/doc/database.test.tstest/features/admin/attendees-csv/questions.test.tstest/features/admin/ledger/names.test.tstest/features/admin/refunds/provider/batch.test.tstest/features/api/helpers/json-body.test.tstest/features/tickets/token-utils/parsing.test.tstest/integration/db/listings/crud.test.tstest/integration/merge/attendee-merge/checkout-stage-cleanup.test.tstest/integration/server/admin-api/listings/create.test.tstest/integration/server/admin-api/listings/groups.test.tstest/integration/server/admin-api/listings/lifecycle.test.tstest/integration/server/admin/attendees/create-atomic.test.tstest/integration/server/attendees/merge-conflicts.test.tstest/integration/server/attributes/detail.test.tstest/integration/server/bulk-actions/reactivate.test.tstest/integration/server/bulk-email/preview.test.tstest/integration/server/bulk-email/previous-bookings.test.tstest/integration/server/holidays/entity-page.test.tstest/integration/server/listings/assign-built-site.test.tstest/integration/server/listings/create-validation.test.tstest/integration/server/listings/daily-type.test.tstest/integration/server/listings/date-and-location.test.tstest/integration/server/listings/deactivate.test.tstest/integration/server/listings/edit-form-and-income.test.tstest/integration/server/listings/index-and-new.test.tstest/integration/server/listings/reactivate.test.tstest/integration/server/listings/show-basics.test.tstest/integration/server/news/admin.test.tstest/integration/server/parents-booking/api-availability.test.tstest/integration/server/parents-gate/auto-fill.test.tstest/integration/server/parents-gate/customisable-fold.test.tstest/integration/server/parents-gate/daily-fold.test.tstest/integration/server/parents-gate/qty-matching.test.tstest/integration/server/public/ticket-terms-checkbox.test.tstest/integration/server/servicing/ledger-validation.test.tstest/integration/server/settings/calendar-feeds.test.tstest/integration/server/settings/column-order.test.tstest/integration/server/settings/payment-provider.test.tstest/integration/server/users/invite.test.tstest/integration/server/users/manage.test.tstest/integration/server/webhooks/acknowledge-edge-cases.test.tstest/integration/server/webhooks/can-pay-more-single-ticket.test.tstest/integration/test-utils/compat.test.tstest/integration/test-utils/describe-with-env-storage.test.tstest/integration/test-utils/record-queries.test.tstest/integration/tooling/cli.test.tstest/integration/ui/templates/admin/ledger/account-label.test.tstest/shared/booking/model/capacity.test.tstest/shared/booking/model/day-counts.test.tstest/shared/booking/model/eligibility.test.tstest/shared/columns/attendee-columns/attendee-answers.test.tstest/shared/db/migrations/schema/admin-feature-triggers.test.tstest/shared/db/settings/theme.test.tstest/shared/db/table/projection.test.tstest/shared/email/send-bulk-email.test.tstest/shared/email/send-email.test.tstest/shared/i18n/loading.test.tstest/shared/interval-gate.test.tstest/shared/lazy-logger.test.tstest/shared/payment-helpers/limits.test.tstest/shared/square-provider/provider.test.tstest/shared/square/retrieve-refund.test.tstest/shared/stripe/webhook-cleanup.test.tstest/shared/stripe/webhook.test.tstest/shared/types/admin-roles.test.tstest/ui/templates/admin/recalculate-rows.test.ts
🛑 Comments failed to post (23)
test/features/admin/attendees-csv/questions.test.ts (1)
7-23: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the repeated question fixture.
This duplicates
test/shared/columns/attendee-columns/attendee-answers.test.tslines 8-23. Promote it to#test-utilsand import it from both suites. As per coding guidelines, “Use#test-utilshelpers instead of defining duplicate local test utilities” and “Eliminate code duplication with shared helpers or currying.”🤖 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/features/admin/attendees-csv/questions.test.ts` around lines 7 - 23, Promote the local question fixture to the shared `#test-utils` module, preserving the existing QuestionWithAnswers construction and distinct answer-id behavior. Remove the duplicate question helper from questions.test.ts and update both questions.test.ts and attendee-answers.test.ts to import and reuse the shared helper.Source: Coding guidelines
test/features/tickets/token-utils/parsing.test.ts (1)
53-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not pin regex-capture internals as route behavior.
This case only succeeds because the unescaped prefix adds its own capture group and changes
match[1]. Production prefixes are literal route segments, so this makes escaping the prefix appear to be a regression. Remove it or replace it with a literal-prefix contract case. As per coding guidelines, “Tests must … test observable behavior rather than implementation details.”🤖 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/features/tickets/token-utils/parsing.test.ts` around lines 53 - 60, Replace the extractTokenSegment test that relies on an unescaped regex prefix and an empty capture group with a case using a literal route prefix. Assert only the observable token-extraction behavior for valid production-style prefixes, and remove comments or expectations tied to match[1], regex grouping, or the ?? versus || implementation detail.Source: Coding guidelines
test/integration/db/listings/crud.test.ts (1)
99-102: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the duplicate missing-listing test.
Lines 99-102 exactly repeat Lines 82-85, adding no coverage and potentially triggering duplication checks.
As per coding guidelines, eliminate code duplication rather than retaining repeated tests.
🤖 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/integration/db/listings/crud.test.ts` around lines 99 - 102, Remove the duplicate “getListingWithCount returns null for missing listing” test near the referenced block, retaining the existing equivalent test elsewhere in the test suite and leaving unique coverage unchanged.Source: Coding guidelines
test/integration/server/admin-api/listings/create.test.ts (1)
121-142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject malformed
day_pricesinstead of saving a partial price map.This request contains invalid keys and values, but expects
201after silently dropping them. That can persist unintended pricing; the boundary should return400rather than partially accepting the payload.As per coding guidelines, expected external data must be explicitly validated at the boundary and failures must be loud.
🤖 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/integration/server/admin-api/listings/create.test.ts` around lines 121 - 142, Update the customisable-days listing validation used by the admin listings POST flow and the test “creates a customisable-days listing with day_prices” so any invalid day_prices key or value causes a 400 response instead of silently dropping malformed entries. Preserve successful creation only for payloads where every day_prices entry is valid, and assert the rejected response rather than a partial listing.Source: Coding guidelines
test/integration/server/admin-api/listings/lifecycle.test.ts (1)
134-155: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the lifecycle changes were persisted.
These tests only validate response bodies, so implementations that return success without updating or deleting the row would pass. Reload the listing and assert
active === trueafter reactivation andnullafter media deletion.As per coding guidelines, tests must use strong mutation-resistant assertions against observable behavior.
Also applies to: 184-202
🤖 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/integration/server/admin-api/listings/lifecycle.test.ts` around lines 134 - 155, Strengthen the lifecycle tests around the reactivation and media-deletion cases by reloading the affected listing from persistent storage after each API call, then assert the observable state: active must be true after reactivation and the deleted media must be null. Keep the existing response assertions, and add mutation-resistant persistence checks in the tests covering the reactivation case and the media deletion case.Source: Coding guidelines
test/integration/server/attributes/detail.test.ts (1)
38-64: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bind each listing count to its option row.
The independent
1and0cell checks still pass if Easy and Hard receive swapped counts. Assert each option link and expected count within the same table row.As per coding guidelines, tests must use strong mutation-resistant 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/integration/server/attributes/detail.test.ts` around lines 38 - 64, Update the test "links each option to its edit page with a listings count" to assert each option link and its expected listings count within the same table row, ensuring Easy is paired with 1 and Hard with 0. Replace the independent cell-content checks with mutation-resistant row-scoped assertions while preserving the existing delete-link assertion.Source: Coding guidelines
test/integration/server/listings/create-validation.test.ts (2)
17-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the submitted
unit_pricewas persisted.A redirect alone passes even if the handler discards the price.
Proposed regression assertion
expect(response.status).toBe(302); + expect((await getListingWithCount(1))?.unit_price).toBe(1000);As per coding guidelines, tests must use strong mutation-resistant assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("creates listing with unit_price when authenticated", async () => { const { response } = await adminFormPost("/admin/listing", { max_attendees: "50", max_quantity: "1", name: "Paid Listing", thank_you_url: "https://example.com/thanks", unit_price: "10.00", }); expect(response.status).toBe(302); expect((await getListingWithCount(1))?.unit_price).toBe(1000); });🤖 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/integration/server/listings/create-validation.test.ts` around lines 17 - 26, Strengthen the “creates listing with unit_price when authenticated” test by loading the created listing after the redirect and asserting its persisted unit_price equals the submitted 10.00 value. Reuse the established listing lookup or database assertion helper and retain the existing redirect-status assertion.Source: Coding guidelines
162-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enable
can_pay_morebefore testing its equality boundary.The
unit_price + 100constraint only applies whencan_pay_moreis enabled. This test leaves it disabled, so it cannot detect rejection of the valid boundary case.Proposed fix
const listing = await createTestListing({ + canPayMore: true, maxPrice: 1100, unitPrice: 1000, });As per coding guidelines, tests must use strong mutation-resistant assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("accepts max_price equal to unit_price + 100", async () => { const listing = await createTestListing({ canPayMore: true, maxPrice: 1100, unitPrice: 1000, }); expect(listing.max_price).toBe(1100); });🤖 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/integration/server/listings/create-validation.test.ts` around lines 162 - 168, Update the test “accepts max_price equal to unit_price + 100” to create the listing with can_pay_more enabled, ensuring it exercises the conditional equality boundary. Keep the existing max_price and unit_price values, and retain the assertion that max_price is 1100.Source: Coding guidelines
test/integration/server/listings/date-and-location.test.ts (1)
92-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the generated CSV rather than the HTML export page.
This test uses
assertAdminHtml, so the CSV can omit these columns while the test passes. Use the same CSV helper as the negative case.Proposed fix
- const { listing } = await setupListingAndLogin({ + const { listing, cookie } = await setupListingAndLogin({ date: "2026-06-15T14:00", location: "Village Hall", }); await createTestAttendee( listing.id, listing.slug, "Alice", "alice@test.com", ); - await assertAdminHtml( - `/admin/listing/${listing.id}/export`, - "Listing Date", - "Listing Location", - "Village Hall", - ); + const csv = await fetchListingExportCsv(listing.id, cookie); + expect(csv).toContain("Listing Date"); + expect(csv).toContain("Listing Location"); + expect(csv).toContain("Village Hall");As per coding guidelines, tests must call production code and assert observable behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("CSV export includes Listing Date and Listing Location columns", async () => { const { listing, cookie } = await setupListingAndLogin({ date: "2026-06-15T14:00", location: "Village Hall", }); await createTestAttendee( listing.id, listing.slug, "Alice", "alice@test.com", ); const csv = await fetchListingExportCsv(listing.id, cookie); expect(csv).toContain("Listing Date"); expect(csv).toContain("Listing Location"); expect(csv).toContain("Village Hall"); });🤖 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/integration/server/listings/date-and-location.test.ts` around lines 92 - 109, Update the test “CSV export includes Listing Date and Listing Location columns” to request and inspect the generated CSV instead of the HTML export page. Reuse the CSV assertion helper used by the negative case, and assert that the CSV contains “Listing Date,” “Listing Location,” and “Village Hall.”Source: Coding guidelines
test/integration/server/listings/index-and-new.test.ts (1)
43-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the custom card exists before comparing positions.
If
"Custom / advanced"disappears,indexOfreturns-1and the ordering assertion still passes.Proposed fix
expect(html).toContain('href="/admin/catalog/import"'); + expect(html).toContain("Custom / advanced"); expect(html.indexOf("Custom / advanced")).toBeLessThan(As per coding guidelines, tests must use strong mutation-resistant assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("renders type picker when authenticated (no template param)", async () => { const response = await adminGet("/admin/listing/new"); const html = await expectHtmlResponse( response, 200, "Add Listing", "Choose a listing type", ); // "Import from file" is a card in the picker grid, and the final one — // after every listing-type card (including "Custom / advanced"). expect(html).toContain('href="/admin/catalog/import"'); expect(html).toContain("Custom / advanced"); expect(html.indexOf("Custom / advanced")).toBeLessThan( html.indexOf("Import from file"), ); // No other picker card follows it. expect(html.lastIndexOf('class="listing-type-card"')).toBe( html.indexOf('class="listing-type-card" href="/admin/catalog/import"'), );🤖 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/integration/server/listings/index-and-new.test.ts` around lines 43 - 60, Strengthen the assertions in the authenticated type-picker test by explicitly asserting that the “Custom / advanced” card exists before comparing its position with “Import from file.” Update the ordering check around the existing html.indexOf calls so a missing custom card causes the test to fail rather than allowing a -1 index to pass.Source: Coding guidelines
test/integration/server/news/admin.test.ts (1)
237-255: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the slug remains unchanged on rejected updates.
These tests only inspect
name, so partial slug persistence would pass unnoticed. The invalid-slug test’s name assertion is also unchanged by the submitted form. Assert the stored slug equalspost.slugafter both requests.As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${BASE}/${post.id}/edit\\?)
Note: [CWE-1333] Inefficient Regular Expression Complexity(regexp-from-variable)
[warning] 252-252: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${BASE}/${post.id}/edit\\?)
Note: [CWE-1333] Inefficient Regular Expression Complexity(regexp-from-variable)
🤖 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/integration/server/news/admin.test.ts` around lines 237 - 255, Update both rejected-update tests around the “missing name” and “invalid slug” cases to assert that the persisted post’s slug remains equal to the original post.slug after the request. Keep the existing name assertions, and ensure the invalid-slug test specifically verifies the submitted invalid value was not stored.Source: Coding guidelines
test/integration/server/parents-gate/qty-matching.test.ts (1)
121-122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert atomicity for every rejected booking case.
The conditional check lets most cases pass if the parent is rejected but one of its children is still persisted. After every rejection, verify all
children—andstrangerwhen present—have no booking, then removeextraChildIdsZero.As per coding guidelines, tests must use strong mutation-resistant assertions and test observable behavior.
Also applies to: 192-210
🤖 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/integration/server/parents-gate/qty-matching.test.ts` around lines 121 - 122, Update the rejected-booking assertions in the relevant test cases to always verify that every child has no booking, and also verify stranger has no booking when that entity is present; remove the extraChildIdsZero option and its conditional assertion path so atomicity is asserted uniformly for every rejection.Source: Coding guidelines
test/integration/server/settings/calendar-feeds.test.ts (1)
16-19: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not lock in a silent fallback for an unknown persisted setting.
Converting
"unknown"to"attendees"hides malformed configuration and can silently change calendar grouping. The settings boundary should reject the unsupported value, and this test should assert that failure.As per coding guidelines, missing expected external data must cause a boundary validation error rather than being defaulted.
🤖 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/integration/server/settings/calendar-feeds.test.ts` around lines 16 - 19, Update the calendar-feeds settings test around settings.calendarFeedsGroupBy so the unsupported persisted value "unknown" is expected to trigger a boundary validation error instead of returning "attendees". Preserve the cache invalidation and key-loading setup, and assert the rejection/failure from loading or accessing the setting.Source: Coding guidelines
test/integration/server/settings/payment-provider.test.ts (2)
2-12: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the unnecessary global cleanup hook.
No test here enables demo mode, so this hook only adds hidden shared-state coupling. Remove
afterEachandsetDemoModeForTest; tests should rely on their isolated environment.As per coding guidelines, do not use global BDD hooks and rely on virgin isolates.
🤖 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/integration/server/settings/payment-provider.test.ts` around lines 2 - 12, Remove the unnecessary afterEach global cleanup hook and the setDemoModeForTest import from the test module. Keep describeWithEnv and the contained tests unchanged, relying on isolated test environments without shared-state cleanup.Source: Coding guidelines
15-23: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert that each successful request persists the selected provider.
Status and flash text can remain correct even if the handler’s save operation is removed. Verify the settings store contains
"square","stripe", or the disabled state after each request.As per coding guidelines, tests must use strong mutation-resistant assertions against observable behavior.
Also applies to: 34-50
🤖 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/integration/server/settings/payment-provider.test.ts` around lines 15 - 23, Extend the payment-provider integration tests around adminFormPost to verify the settings store persists the selected provider after each successful request, including “square,” “stripe,” and the disabled state. Assert the store’s observable value with mutation-resistant checks while retaining the existing status and flash assertions.Source: Coding guidelines
test/integration/server/users/invite.test.ts (1)
84-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact persisted user and rejected-request side effects.
The success case should verify
newmanagerwas stored with themanagerrole. The missing-username case should verify the expected validation flash/target and that the user count remains unchanged; status302alone can pass for unrelated redirects.As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
Also applies to: 125-132
🤖 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/integration/server/users/invite.test.ts` around lines 84 - 96, Strengthen the user-invite integration tests by locating the persisted user by username and asserting newmanager has the manager role, rather than checking only the total count. In the missing-username test, assert the expected validation flash and redirect target, then verify the persisted user count remains unchanged; retain status assertions but do not rely on 302 alone.Source: Coding guidelines
test/integration/server/users/manage.test.ts (1)
42-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert which user was deleted or retained.
Row counts cannot detect deletion of the wrong account, and the missing-confirmation test never checks persistence. Assert the target username is absent after success and remains present after rejection, while the current owner remains intact.
As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
Also applies to: 82-94, 103-122
🤖 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/integration/server/users/manage.test.ts` around lines 42 - 61, Strengthen the user-deletion tests around adminFormPost and getAllUsers by asserting usernames rather than only row counts: after successful deletion, verify deleteme is absent and the current owner remains; in the missing-confirmation and incorrect-confirmation cases, verify the targeted user remains present and the owner is intact.Source: Coding guidelines
test/integration/server/webhooks/acknowledge-edge-cases.test.ts (1)
20-22: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the Stripe
afterEachhooks with per-test cleanup.Both suites rely on BDD hooks for shared singleton restoration. Use a shared wrapper that runs each Stripe-backed test body inside
try/finally.
test/integration/server/webhooks/acknowledge-edge-cases.test.ts#L20-L22: removeafterEachand use the wrapper for each test.test/integration/server/webhooks/can-pay-more-single-ticket.test.ts#L22-L24: remove the duplicate hook and use the same wrapper.As per coding guidelines, tests must not use global BDD hooks. <coding_guidelines>
📍 Affects 2 files
test/integration/server/webhooks/acknowledge-edge-cases.test.ts#L20-L22(this comment)test/integration/server/webhooks/can-pay-more-single-ticket.test.ts#L22-L24🤖 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/integration/server/webhooks/acknowledge-edge-cases.test.ts` around lines 20 - 22, Remove the global afterEach hooks that call resetStripeClient in test/integration/server/webhooks/acknowledge-edge-cases.test.ts:20-22 and test/integration/server/webhooks/can-pay-more-single-ticket.test.ts:22-24. Introduce and reuse a shared wrapper around each Stripe-backed test body that invokes resetStripeClient in a try/finally block, ensuring cleanup occurs per test without BDD hooks.Source: Coding guidelines
test/integration/server/webhooks/can-pay-more-single-ticket.test.ts (1)
92-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Configure the maximum price explicitly.
This “above maximum” case relies on
createTestListing’s implicitmaxPrice. A fixture-default change could make20000valid and stop exercising the intended branch.Proposed fix
const listing = await createTestListing({ canPayMore: true, maxAttendees: 50, + maxPrice: 15000, unitPrice: 1000, });As per coding guidelines, tests must not hard-code or reimplement implicit production constants. <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("single-ticket can_pay_more keeps and refunds amount above maximum price", async () => { await setupStripe(); const listing = await createTestListing({ canPayMore: true, maxAttendees: 50, maxPrice: 15000, unitPrice: 1000, }); const { mockRefund } = await expectWebhookKeptAndRefunded( checkoutSessionEvent({ amountTotal: 20000, eventId: "evt_pay_too_much", metadata: signedMeta( { email: "overpay@example.com", items: singleItem(listing.id, 1, 20000), name: "Overpay User", }, 20000, ), paymentIntent: "pi_pay_too_much", sessionId: "cs_pay_too_much", }), "re_pay_too_much", );🤖 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/integration/server/webhooks/can-pay-more-single-ticket.test.ts` around lines 92 - 117, Update the createTestListing call in the “single-ticket can_pay_more keeps and refunds amount above maximum price” test to explicitly configure a maxPrice below 20000, using the fixture’s established maximum-price symbol or default rather than hard-coding an implicit production constant. Keep the existing overpayment scenario and assertions unchanged.Source: Coding guidelines
test/integration/test-utils/describe-with-env-storage.test.ts (3)
8-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file='test/integration/test-utils/describe-with-env-storage.test.ts' echo '--- file outline ---' ast-grep outline "$file" --view expanded || true echo echo '--- relevant lines ---' nl -ba "$file" | sed -n '1,180p'Repository: chobbledotcom/tickets
Length of output: 393
🏁 Script executed:
#!/bin/bash set -euo pipefail file='test/integration/test-utils/describe-with-env-storage.test.ts' echo '--- relevant lines with numbers ---' awk '{printf "%4d %s\n", NR, $0}' "$file" | sed -n '1,220p'Repository: chobbledotcom/tickets
Length of output: 5380
Only swallow not-found here.
Deno.statSyncpermission and other filesystem errors should surface instead of being turned intofalse, or the teardown assertion can pass when it shouldn’t.🤖 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/integration/test-utils/describe-with-env-storage.test.ts` around lines 8 - 13, Update dirExists to suppress errors only when Deno.statSync reports a missing path; rethrow permission and all other filesystem errors so teardown assertions cannot silently pass. Preserve returning isDirectory for existing paths and false for not-found paths.Source: Coding guidelines
35-65: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove the shared
observedDir/afterAlllifecycle assertion.These tests communicate through mutable suite state, so their result depends on which cases execute and their ordering. Assert each directory’s cleanup within an isolated test or a dedicated helper instead.
As per coding guidelines, tests must not use global BDD hooks or share mutable state without resetting and pinning it. <coding_guidelines>
🤖 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/integration/test-utils/describe-with-env-storage.test.ts` around lines 35 - 65, Remove the shared observedDir variable and the local backend afterAll hook. Update each test within describeWithEnv to assert its own temporary directory is removed and getTestStoragePath() is null after test cleanup, using an isolated test or helper that does not depend on execution order or shared mutable state.Source: Coding guidelines
68-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rewrite the regression comments around the current contract.
Both blocks preserve implementation history rather than explaining current behavior.
test/integration/test-utils/describe-with-env-storage.test.ts#L68-L70: state that explicit local storage configuration takes precedence over ambient CDN credentials.test/integration/test-utils/describe-with-env-storage.test.ts#L91-L97: state that storage configuration is scoped and must not mutate environment variables.As per coding guidelines, “Do not leave historical comments describing replaced implementations; comments must explain the current code.” <coding_guidelines>
📍 Affects 1 file
test/integration/test-utils/describe-with-env-storage.test.ts#L68-L70(this comment)test/integration/test-utils/describe-with-env-storage.test.ts#L91-L97🤖 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/integration/test-utils/describe-with-env-storage.test.ts` around lines 68 - 70, Rewrite the comments in test/integration/test-utils/describe-with-env-storage.test.ts at lines 68-70 and 91-97 to describe current behavior: the first must state that explicit local storage configuration takes precedence over ambient CDN credentials, and the second must state that storage configuration is scoped and must not mutate environment variables. Remove implementation-history references without changing the test behavior.Source: Coding guidelines
test/shared/payment-helpers/limits.test.ts (1)
205-209: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Describe the current ownership without migration history.
“Moved” and “no longer” document the replaced implementation. State that
buildItemsMetadatabounds this value before signing, so this function intentionally leaves it unchanged.Proposed wording
- test("leaves a too-long thank_you_url untouched (capping moved pre-sign)", () => { - // enforceMetadataLimits no longer strips thank_you_url: that is done in - // buildItemsMetadata before signing so the proof and metadata stay - // consistent. An over-cap URL passing through here is - // unchanged (it never reaches here over-cap in production). + test("leaves thank_you_url unchanged because it is bounded before signing", () => { + // buildItemsMetadata bounds thank_you_url before signing, keeping the + // proof and emitted metadata consistent. Production inputs therefore + // reach this function within the provider limit.As per coding guidelines, “Do not leave historical comments describing replaced implementations; comments must explain the current code.” <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("leaves thank_you_url unchanged because it is bounded before signing", () => { // buildItemsMetadata bounds thank_you_url before signing, keeping the // proof and emitted metadata consistent. Production inputs therefore // reach this function within the provider limit.🤖 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/shared/payment-helpers/limits.test.ts` around lines 205 - 209, Rewrite the comment in the “leaves a too-long thank_you_url untouched” test to describe only current behavior: buildItemsMetadata bounds thank_you_url before signing, while enforceMetadataLimits intentionally leaves an over-cap value unchanged. Remove references to moving or no longer performing the operation.Source: Coding guidelines
stefan-burke
changed the title
There was a problem hiding this comment.
Actionable comments posted: 30
🤖 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/features/admin/catalog-transfer/import/fields.test.ts`:
- Around line 40-55: Update the test “accepts datetimes with and without a
timezone suffix” to assert the persisted event date field derived from the
timezone-less listing.date, using its exact UTC-normalized value. Keep the
existing closes_at assertion, and verify the stored date through the imported
listing returned by getListingWithCount.
In `@test/features/admin/settings-listing-defaults.test.ts`:
- Around line 9-12: Move the local ok helper from the test into the shared
`#test-utils` module, reusing the existing result-unwrapping behavior and error
message. Update both settings-listing-defaults.test.ts and
customisable-fold.test.ts to import and use the centralized helper, removing
their duplicate definitions.
- Around line 79-86: Update the “keeps the valid ticked days and drops invalid
ones” test to assert that parse rejects the entire input when
default_bookable_days contains both Funday and Monday. Replace the successful
ok/value.bookableDays expectation with the established boundary-validation error
assertion, preserving coverage that malformed external day values are not
silently discarded.
In `@test/shared/accounting/queries/operator-ledger.test.ts`:
- Around line 198-224: Add transfers occurring exactly at range.startMs and
range.endMs in the “a date range bounds the totals and the list to [start, end)”
test. Update the expected income and visible reference assertions to prove the
start-boundary row is included while the end-boundary row is excluded, using
exact totals and ordered references that distinguish both boundary conditions.
In `@test/shared/apple-wallet/cms.test.ts`:
- Around line 152-163: Strengthen the test around signManifest by parsing the
returned CMS signature’s certificate set and asserting that it contains the
certificate from nonRsaCertificatePem(), preferably by comparing its certificate
bytes or fingerprint. Replace the signature.length threshold assertion while
preserving the existing non-RSA WWDR signing setup.
In `@test/shared/db/activityLog.test.ts`:
- Around line 216-218: Update the getListingWithActivityLog contract and this
test so the API follows the lookup naming guideline: either rename the nullable
lookup to getListingWithActivityLogOrNull and retain the null assertion, or make
getListingWithActivityLog throw for a missing listing and assert the rejected
call instead. Keep the non-existent-listing behavior consistent between
implementation and test.
In `@test/shared/db/address-cache.test.ts`:
- Around line 55-59: Strengthen the test named “the search index is an HMAC, not
the search text” by asserting that repeated calls to computeAddressSearchIndex
with identical inputs return the same index. Keep the existing assertions that
the index does not expose the search text or namespace, and use a direct
equality assertion between independently computed results.
In `@test/shared/db/attendees/balance.test.ts`:
- Around line 232-265: Update the test around getAttendeeOrderSummary to verify
the N+1 guarantee through query-count comparisons rather than SQL substring
matching. Measure queries for one booking and for multiple bookings, then assert
the count does not grow per listing while preserving the existing summary result
assertions; remove assertions tied to aliases or JOIN syntax.
In `@test/shared/db/attendees/capacity/checks/availability.test.ts`:
- Around line 55-58: Update the tests for hasAvailableSpots in
test/shared/db/attendees/capacity/checks/availability.test.ts lines 55-58 and
batch availability in test/shared/db/attendees/capacity/checks/batch.test.ts
lines 23-27 to expect rejection when a referenced listing does not exist, rather
than asserting false. Ensure both tests verify the lookup failure throws for
unknown listing IDs.
- Around line 156-158: Update the comments above the getQueryLog().length
assertion to describe the current query-count invariant directly, without
mentioning the old implementation or contrasting historical query behavior. Keep
the assertion and its limit unchanged.
In `@test/shared/db/attendees/capacity/groups.test.ts`:
- Around line 43-48: Strengthen the assertion for the standard group in the
getDatelessGroupRemaining test: replace the membership-only check for
standardGroup.id with an assertion that remaining.get(standardGroup.id) equals
5. Keep the existing assertion that dailyGroup.id is absent.
In `@test/shared/db/attendees/delete.test.ts`:
- Around line 41-43: Rename the nullable attendee lookup API from getAttendee to
getAttendeeOrNull, or retain getAttendee as the throwing variant while adding
and using getAttendeeOrNull for absence-expected paths. Update the referenced
tests and all call sites to use the explicit nullable name, preserving the
existing null result behavior.
In `@test/shared/db/attendees/stats.test.ts`:
- Around line 98-114: Update the “treats non-numeric price_paid as zero” test to
persist or inject an actually non-numeric price_paid value instead of passing
numeric 0 through createPaidTestAttendee. Preserve the existing stats
assertions, or rename the test if it is intended only to cover zero-priced
attendees.
In `@test/shared/db/listings/table.test.ts`:
- Around line 41-53: Update the date, numeric, and JSON boundary-transform tests
around the listings table to expect validation failures for malformed or missing
external data instead of asserting fallback values such as "", 1, or []. Change
the corresponding production transforms to reject these inputs, while preserving
normal valid-data behavior and updating the affected assertions in the
invalid-date and referenced test cases.
In `@test/shared/db/migrations/2026-06-22_transfers_time_int.test.ts`:
- Around line 7-10: Replace the non-null assertion on the MIGRATIONS.find lookup
with an explicit missing-entry check, throwing a targeted error immediately when
no migration with id "2026-06-22_transfers_time_int" is found; preserve the
existing migration usage when the lookup succeeds.
In `@test/shared/db/questions/parsing.test.ts`:
- Around line 80-85: Update the optional free-text parsing and its test around
expectOptionalFreeTextEmpty so optional: true accepts only absent or blank
answers; supplied text exceeding MAX_TEXTAREA_LENGTH must produce the boundary
validation error instead of being skipped or defaulted. Adjust the test to
assert rejection and preserve the existing success behavior for valid empty
optional answers.
In `@test/shared/forms/definition/field-schema.test.ts`:
- Around line 212-220: Update the test around rejectUnknownSection to invoke the
callback and assert that it throws the expected error, replacing the typeof
assertion. Preserve the existing section-content assertions and keep the
TypeScript suppression for the intentionally invalid section ID.
In `@test/shared/validation/email.test.ts`:
- Around line 69-85: Strengthen the tests around the settings cache by proving
the first read performs database/decryption work while the second read reuses
cached data, and by verifying invalidateCache causes subsequent reads to observe
updated underlying storage or repeat decryption. Update the tests named “uses
settings cache for reads” and “invalidateSettingsCache forces decrypt from
database” with mutation-resistant assertions that fail if caching is removed or
invalidation is a no-op.
- Around line 9-11: Update the test around settings.businessEmail to assert the
property directly instead of applying the nullish fallback. Remove the ?? ""
default and verify the expected empty-string value from businessEmail itself, so
undefined cannot pass silently.
In `@test/ui/client/order.test.ts`:
- Around line 61-265: Extract the browser harness and DOM helper
symbols—harness, Harness, listing, setBody, addLink, shadow, cartButton,
dialogEl, clickAnchor, clickIn, openCart, logHas, buttonType, textOf,
storedCart, mountOpenListing, openCartWithOne, stepperButtons, and
stubStorage—into a focused test-support module. Import them into order.test.ts
and leave only behavioral test cases and their test-specific setup in the
original file.
- Around line 544-552: Update the test “the close button and the dialog close
event return focus” to verify each focus-restoration trigger independently:
capture the focus-call count before and after clicking .close, then assert that
delta is positive; reset or use a new baseline before dispatching the dialog
close event and assert its delta is also positive. Keep the existing dialog
open-state assertions.
In `@test/ui/templates/admin/activityLog.test.ts`:
- Around line 48-51: Wrap the activity-log test suites in a parent describe
block and move the existing beforeAll setup hook inside that parent suite. Keep
setupTestEncryptionKey and signCsrfToken unchanged, ensuring the hook is scoped
to these tests rather than remaining module-level.
- Around line 22-32: Update the documentation for the logEntry factory to
accurately state that all properties in its overrides object, including created
and id, can vary; remove the claim that only message, attendee_id, and
listing_id are override knobs.
In `@test/ui/templates/admin/footer.test.ts`:
- Around line 18-23: Remove the shared top-level crypto/CSRF hooks and isolate
setup and reset of mutable process-global state in the affected suites:
test/ui/templates/admin/footer.test.ts (lines 18-23),
test/ui/templates/admin/groups/overview/groups.test.ts (lines 15-18), and
test/ui/templates/attendee-table.test.ts (lines 18-21). Use per-test isolation
or mark each file to run alone, ensuring parallel execution is not
order-dependent and preserving the required CSRF-token setup.
In `@test/ui/templates/admin/groups/overview/groups.test.ts`:
- Around line 322-340: Strengthen the assertions in the “renders saved overrides
and falls back to defaults for members without a row” test by inspecting the
exact inputs for listing 2. Bind the quantity assertion to name="package_qty_2"
and verify its value is "1", and bind the price assertion to
name="package_price_2" and verify it is blank, using complete input matching or
parsed named elements.
In `@test/ui/templates/admin/update.test.ts`:
- Around line 78-82: Strengthen the “includes release notes link” test around
adminUpdatePage so it verifies that GITHUB_RELEASES_URL and the “release notes”
label occur within the same anchor element. Parse the rendered HTML or assert
the complete anchor relationship, rather than relying on separate independent
containment checks.
In `@test/ui/templates/attendee-table.test.ts`:
- Around line 810-825: The test covering the default registered-date rendering
currently asserts only the year, allowing formatting regressions. Update the
expectation in “renders default cell format when no filter applied” to assert
the complete formatted cell value, including date, ordering, and time, while
retaining the existing check that the Liquid strftime format is not used.
- Around line 752-761: Update the invalid-template test around AttendeeTable and
makeOpts so it expects the validation error for "{{invalid_column}}" instead of
asserting default columns are rendered. Ensure template validation occurs at the
AttendeeTable boundary and removes the silent fallback to the default column
configuration.
- Around line 61-827: Split the oversized AttendeeTable test module into focused
colocated suites, keeping the existing assertions and shared fixtures reusable:
isolate table rendering/column behavior, sortAttendeeRows and
formatAddressInline helpers, questionData rendering, and columnTemplate
contracts into separate test files. Keep each file roughly under 400 lines and
preserve the current describe/test coverage without changing production
behavior.
In `@test/ui/templates/public/reservations/questions.test.ts`:
- Around line 135-159: Update the saved-form-state tests around “restores a
saved free-text answer from saved form data” and “restores selected select
answers from saved form data” so cleanup always runs in a finally block,
including when setup, rendering, or assertions throw. Keep each test’s existing
assertions and saved data while ensuring clearSavedFormData is reliably invoked
after every test.
🪄 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: 23733c6c-55bd-4d98-868b-0af087e512e8
📒 Files selected for processing (75)
deno.jsontest/features/admin/catalog-transfer/import/fields.test.tstest/features/admin/catalog-transfer/import/packages.test.tstest/features/admin/catalog-transfer/import/reachability.test.tstest/features/admin/settings-listing-defaults.test.tstest/integration/merge/attendee-merge/checkout-stage-cleanup.test.tstest/integration/server/attendees/merge-conflicts.test.tstest/integration/server/bulk-email/preview.test.tstest/integration/server/bulk-email/previous-bookings.test.tstest/integration/server/listings/deactivate.test.tstest/integration/server/listings/reactivate.test.tstest/integration/server/news/admin.test.tstest/integration/server/parents-booking/api-availability.test.tstest/integration/server/parents-gate/customisable-fold.test.tstest/integration/server/parents-gate/daily-fold.test.tstest/integration/server/public/ticket-terms-checkbox.test.tstest/integration/tooling/cli.test.tstest/integration/ui/templates/admin/ledger/account-label.test.tstest/shared/accounting/queries/balances.test.tstest/shared/accounting/queries/operator-ledger.test.tstest/shared/accounting/queries/projections.test.tstest/shared/accounting/queries/reads.test.tstest/shared/admin-surface.test.tstest/shared/apple-wallet/certificate.test.tstest/shared/apple-wallet/cms.test.tstest/shared/booking/model/capacity.test.tstest/shared/booking/model/day-counts.test.tstest/shared/booking/model/eligibility.test.tstest/shared/booking/tree.test.tstest/shared/columns/attendee-columns/attendee-answers.test.tstest/shared/db/activityLog.test.tstest/shared/db/address-cache.test.tstest/shared/db/attendees/balance.test.tstest/shared/db/attendees/capacity/checks/availability.test.tstest/shared/db/attendees/capacity/checks/batch.test.tstest/shared/db/attendees/capacity/groups.test.tstest/shared/db/attendees/capacity/remaining.test.tstest/shared/db/attendees/delete.test.tstest/shared/db/attendees/order-parents.test.tstest/shared/db/attendees/select.test.tstest/shared/db/attendees/stats.test.tstest/shared/db/attendees/update.test.tstest/shared/db/listings/table.test.tstest/shared/db/migrations/2026-06-22_drop_listing_attendee_refunded.test.tstest/shared/db/migrations/2026-06-22_transfers_time_int.test.tstest/shared/db/migrations/2026-07-15_checkout_stages.test.tstest/shared/db/migrations/schema-sync/free-text.test.tstest/shared/db/migrations/schema-sync/recreate-table-atomic.test.tstest/shared/db/migrations/schema/triggers/listing-aggregates.test.tstest/shared/db/questions/parsing.test.tstest/shared/db/questions/queries.test.tstest/shared/db/settings/json-field.test.tstest/shared/db/users.test.tstest/shared/forms/definition/define-form.test.tstest/shared/forms/definition/field-schema.test.tstest/shared/forms/validation.test.tstest/shared/order/evaluate.test.tstest/shared/rest/resource.test.tstest/shared/square-provider/provider.test.tstest/shared/square/retrieve-refund.test.tstest/shared/stripe-provider.test.tstest/shared/stripe/webhook-cleanup.test.tstest/shared/stripe/webhook.test.tstest/shared/validation/email.test.tstest/ui/client/order.test.tstest/ui/templates/admin/activityLog.test.tstest/ui/templates/admin/footer.test.tstest/ui/templates/admin/groups/overview/groups.test.tstest/ui/templates/admin/update.test.tstest/ui/templates/attendee-table.test.tstest/ui/templates/fields/admin/contracts.test.tstest/ui/templates/public/reservations/day-config.test.tstest/ui/templates/public/reservations/form.test.tstest/ui/templates/public/reservations/questions.test.tstest/ui/templates/public/shared.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (30)
test/features/admin/catalog-transfer/import/fields.test.ts (1)
40-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the timezone-less
datevalue is persisted as UTC.The test only checks
closesAt, so a regression that misinterprets or drops the naivedatestill passes. Assert the stored event date’s exact normalized value.As per coding guidelines, tests must use strong mutation-resistant assertions and test observable behavior.
🤖 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/features/admin/catalog-transfer/import/fields.test.ts` around lines 40 - 55, Update the test “accepts datetimes with and without a timezone suffix” to assert the persisted event date field derived from the timezone-less listing.date, using its exact UTC-normalized value. Keep the existing closes_at assertion, and verify the stored date through the imported listing returned by getListingWithCount.Source: Coding guidelines
test/features/admin/settings-listing-defaults.test.ts (2)
9-12: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the duplicated result unwrapping into
#test-utils.This exact
okhelper is also defined incustomisable-fold.test.ts; centralize it so result handling cannot diverge.As per coding guidelines, “Use
#test-utilshelpers instead of defining duplicate local test utilities.”🤖 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/features/admin/settings-listing-defaults.test.ts` around lines 9 - 12, Move the local ok helper from the test into the shared `#test-utils` module, reusing the existing result-unwrapping behavior and error message. Update both settings-listing-defaults.test.ts and customisable-fold.test.ts to import and use the centralized helper, removing their duplicate definitions.Source: Coding guidelines
79-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not codify silently dropping malformed day values.
This permits invalid external input whenever one valid day accompanies it. The parser should reject the entire field at the boundary, and this test should expect an error.
Proposed test correction
-test("keeps the valid ticked days and drops invalid ones", () => { - const value = ok( - parse( - "default_bookable_days_enabled=1&default_bookable_days=Funday&default_bookable_days=Monday", - ), - ); - expect(value.bookableDays).toEqual(["Monday"]); +test("rejects invalid ticked days", () => { + expect( + parse( + "default_bookable_days_enabled=1&default_bookable_days=Funday&default_bookable_days=Monday", + ).error, + ).not.toBeNull(); });As per coding guidelines, invalid expected external data must cause a boundary validation error rather than being silently defaulted or discarded.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("rejects invalid ticked days", () => { expect( parse( "default_bookable_days_enabled=1&default_bookable_days=Funday&default_bookable_days=Monday", ).error, ).not.toBeNull(); });🤖 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/features/admin/settings-listing-defaults.test.ts` around lines 79 - 86, Update the “keeps the valid ticked days and drops invalid ones” test to assert that parse rejects the entire input when default_bookable_days contains both Funday and Monday. Replace the successful ok/value.bookableDays expectation with the established boundary-validation error assertion, preserving coverage that malformed external day values are not silently discarded.Source: Coding guidelines
test/shared/accounting/queries/operator-ledger.test.ts (1)
198-224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise both exact date-range boundaries.
The current data cannot distinguish
[start, end)from(start, end]. Add rows exactly atstartMsandendMs, asserting the start row is included and the end row excluded.As per coding guidelines, tests must use strong mutation-resistant 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/shared/accounting/queries/operator-ledger.test.ts` around lines 198 - 224, Add transfers occurring exactly at range.startMs and range.endMs in the “a date range bounds the totals and the list to [start, end)” test. Update the expected income and visible reference assertions to prove the start-boundary row is included while the end-boundary row is excluded, using exact totals and ordered references that distinguish both boundary conditions.Source: Coding guidelines
test/shared/apple-wallet/cms.test.ts (1)
152-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the intermediate certificate is actually embedded.
signature.length > 500can still pass if the WWDR certificate is accidentally omitted. Parse the CMS certificate set and compare the embedded certificate or fingerprint againstnonRsaCertificatePem().As per coding guidelines, tests must use strong mutation-resistant assertions and observable behavior.
🤖 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/shared/apple-wallet/cms.test.ts` around lines 152 - 163, Strengthen the test around signManifest by parsing the returned CMS signature’s certificate set and asserting that it contains the certificate from nonRsaCertificatePem(), preferably by comparing its certificate bytes or fingerprint. Replace the signature.length threshold assertion while preserving the existing non-RSA WWDR signing setup.Source: Coding guidelines
test/shared/db/activityLog.test.ts (1)
216-218: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not codify a nullable result behind a non-nullable lookup name.
If absence is expected, expose
getListingWithActivityLogOrNull; otherwise make the lookup throw and test the rejection.As per coding guidelines, intentional nullable results require
*OrNullnaming, while lookups must fail when expected data is missing.🤖 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/shared/db/activityLog.test.ts` around lines 216 - 218, Update the getListingWithActivityLog contract and this test so the API follows the lookup naming guideline: either rename the nullable lookup to getListingWithActivityLogOrNull and retain the null assertion, or make getListingWithActivityLog throw for a missing listing and assert the rejected call instead. Keep the non-existent-listing behavior consistent between implementation and test.Source: Coding guidelines
test/shared/db/address-cache.test.ts (1)
55-59: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert that identical input produces a stable search index.
A randomized index passes these assertions but breaks cache hits when the store and read paths compute the key separately.
Proposed regression assertion
const index = await computeAddressSearchIndex("easypostcodes", "SW1A 2AA"); +const repeated = await computeAddressSearchIndex( + "easypostcodes", + "SW1A 2AA", +); +expect(repeated).toBe(index); expect(index).not.toContain("SW1A");As per coding guidelines, tests must use strong mutation-resistant assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("the search index is an HMAC, not the search text", async () => { const index = await computeAddressSearchIndex("easypostcodes", "SW1A 2AA"); const repeated = await computeAddressSearchIndex( "easypostcodes", "SW1A 2AA", ); expect(repeated).toBe(index); expect(index).not.toContain("SW1A"); expect(index).not.toContain("easypostcodes"); });🤖 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/shared/db/address-cache.test.ts` around lines 55 - 59, Strengthen the test named “the search index is an HMAC, not the search text” by asserting that repeated calls to computeAddressSearchIndex with identical inputs return the same index. Keep the existing assertions that the index does not expose the search text or namespace, and use a direct equality assertion between independently computed results.Source: Coding guidelines
test/shared/db/attendees/balance.test.ts (1)
232-265: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test the N+1 guarantee, not exact SQL text.
These substring assertions couple the test to aliases and join syntax. Compare query counts for one versus multiple bookings instead, so equivalent query rewrites remain valid.
As per coding guidelines, tests must verify observable behavior rather than implementation details.
🤖 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/shared/db/attendees/balance.test.ts` around lines 232 - 265, Update the test around getAttendeeOrderSummary to verify the N+1 guarantee through query-count comparisons rather than SQL substring matching. Measure queries for one booking and for multiple bookings, then assert the count does not grow per listing while preserving the existing summary result assertions; remove assertions tied to aliases or JOIN syntax.Source: Coding guidelines
test/shared/db/attendees/capacity/checks/availability.test.ts (2)
55-58: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make missing listing references fail at the capacity boundary.
Both tests currently normalize a broken listing reference into
false, making invalid data indistinguishable from genuine insufficient capacity.
test/shared/db/attendees/capacity/checks/availability.test.ts#L55-L58: assert that single-listing availability rejects an unknown listing ID.test/shared/db/attendees/capacity/checks/batch.test.ts#L23-L27: assert that batch availability rejects when any referenced listing is missing.As per coding guidelines, expected lookup failures must throw instead of returning sentinel values. <coding_guidelines>
📍 Affects 2 files
test/shared/db/attendees/capacity/checks/availability.test.ts#L55-L58(this comment)test/shared/db/attendees/capacity/checks/batch.test.ts#L23-L27🤖 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/shared/db/attendees/capacity/checks/availability.test.ts` around lines 55 - 58, Update the tests for hasAvailableSpots in test/shared/db/attendees/capacity/checks/availability.test.ts lines 55-58 and batch availability in test/shared/db/attendees/capacity/checks/batch.test.ts lines 23-27 to expect rejection when a referenced listing does not exist, rather than asserting false. Ensure both tests verify the lookup failure throws for unknown listing IDs.Source: Coding guidelines
156-158: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Describe the current query invariant, not the replaced implementation.
Proposed update
- // One capacity query (listing + group caps, all days) plus the listing - // lookup — not the overlap + two group reads the old code ran. + // The listing lookup and batched capacity check must stay within + // a two-query budget.As per coding guidelines, comments must explain current code rather than historical implementations. <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// The listing lookup and batched capacity check must stay within // a two-query budget. expect(getQueryLog().length).toBeLessThanOrEqual(2);🤖 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/shared/db/attendees/capacity/checks/availability.test.ts` around lines 156 - 158, Update the comments above the getQueryLog().length assertion to describe the current query-count invariant directly, without mentioning the old implementation or contrasting historical query behavior. Keep the assertion and its limit unchanged.Source: Coding guidelines
test/shared/db/attendees/capacity/groups.test.ts (1)
43-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the computed group capacity, not only membership.
remaining.has(standardGroup.id)permits an incorrect capacity value to pass. Assertremaining.get(standardGroup.id)equals5.As per coding guidelines, tests must use strong mutation-resistant 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/shared/db/attendees/capacity/groups.test.ts` around lines 43 - 48, Strengthen the assertion for the standard group in the getDatelessGroupRemaining test: replace the membership-only check for standardGroup.id with an assertion that remaining.get(standardGroup.id) equals 5. Keep the existing assertion that dailyGroup.id is absent.Source: Coding guidelines
test/shared/db/attendees/delete.test.ts (1)
41-43: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use an explicitly nullable attendee lookup.
These tests expect absence, but
getAttendeedoes not advertise a nullable result. Rename the production API togetAttendeeOrNull, or provide a throwinggetAttendeeplus an explicit nullable variant.As per coding guidelines, intentional nullable results require
*OrNullnaming.Also applies to: 127-130, 249-251
🤖 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/shared/db/attendees/delete.test.ts` around lines 41 - 43, Rename the nullable attendee lookup API from getAttendee to getAttendeeOrNull, or retain getAttendee as the throwing variant while adding and using getAttendeeOrNull for absence-expected paths. Update the referenced tests and all call sites to use the explicit nullable name, preserving the existing null result behavior.Source: Coding guidelines
test/shared/db/attendees/stats.test.ts (1)
98-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reproduce the claimed non-numeric input.
The helper receives numeric
0, so this only tests a zero-priced attendee and cannot catch regressions involving non-numeric persisted data. Either inject the malformed database value or rename the test to describe zero-income handling.As per coding guidelines, tests must test observable behavior and every regression test must reproduce the original failure.
🤖 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/shared/db/attendees/stats.test.ts` around lines 98 - 114, Update the “treats non-numeric price_paid as zero” test to persist or inject an actually non-numeric price_paid value instead of passing numeric 0 through createPaidTestAttendee. Preserve the existing stats assertions, or rename the test if it is intended only to cover zero-priced attendees.Source: Coding guidelines
test/shared/db/listings/table.test.ts (1)
41-53: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not lock silent corruption fallbacks into the contract.
These cases accept malformed date, numeric, and JSON data by converting it to
"",1, or[]. That hides corrupt persisted/input data. Production boundary transforms should reject these values, and the tests should assert those validation failures instead.As per coding guidelines, missing expected external data must cause a boundary validation error rather than being defaulted.
Also applies to: 119-120, 138-152
🤖 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/shared/db/listings/table.test.ts` around lines 41 - 53, Update the date, numeric, and JSON boundary-transform tests around the listings table to expect validation failures for malformed or missing external data instead of asserting fallback values such as "", 1, or []. Change the corresponding production transforms to reject these inputs, while preserving normal valid-data behavior and updating the affected assertions in the invalid-date and referenced test cases.Source: Coding guidelines
test/shared/db/migrations/2026-06-22_transfers_time_int.test.ts (1)
7-10: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the migration registry lookup explicitly.
A missing registry entry currently fails later at
migration.up()with an unhelpful undefined-property error. Throw a targeted error immediately whenfind()returns no migration.Proposed fix
const migration = MIGRATIONS.find( (m) => m.id === "2026-06-22_transfers_time_int", -)!; +); +if (!migration) { + throw new Error( + "Migration 2026-06-22_transfers_time_int is missing from the registry", + ); +}Based on learnings, dynamically loaded migration registry entries require explicit boundary validation rather than non-null assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const MIGRATIONS = await loadMigrations(); const migration = MIGRATIONS.find( (m) => m.id === "2026-06-22_transfers_time_int", ); if (!migration) { throw new Error( "Migration 2026-06-22_transfers_time_int is missing from the registry", ); }🤖 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/shared/db/migrations/2026-06-22_transfers_time_int.test.ts` around lines 7 - 10, Replace the non-null assertion on the MIGRATIONS.find lookup with an explicit missing-entry check, throwing a targeted error immediately when no migration with id "2026-06-22_transfers_time_int" is found; preserve the existing migration usage when the lookup succeeds.Source: Learnings
test/shared/db/questions/parsing.test.ts (1)
80-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject supplied over-length text even when the field is optional.
optional: trueshould permit an absent or blank answer, but a supplied over-limit value is malformed input. Returning success silently discards the user’s submission and prevents the caller from showing a validation error.As per coding guidelines, malformed external data must cause a boundary validation error rather than being defaulted or discarded.
🤖 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/shared/db/questions/parsing.test.ts` around lines 80 - 85, Update the optional free-text parsing and its test around expectOptionalFreeTextEmpty so optional: true accepts only absent or blank answers; supplied text exceeding MAX_TEXTAREA_LENGTH must produce the boundary validation error instead of being skipped or defaulted. Adjust the test to assert rejection and preserve the existing success behavior for valid empty optional answers.Source: Coding guidelines
test/shared/forms/definition/field-schema.test.ts (1)
212-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Actually invoke the unknown-section lookup.
The
typeofassertion never exercisesform.section("missing"), so removing its runtime guard would not fail this test. Assertexpect(rejectUnknownSection).toThrow(...).As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
🤖 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/shared/forms/definition/field-schema.test.ts` around lines 212 - 220, Update the test around rejectUnknownSection to invoke the callback and assert that it throws the expected error, replacing the typeof assertion. Preserve the existing section-content assertions and keep the TypeScript suppression for the intentionally invalid section ID.Source: Coding guidelines
test/shared/validation/email.test.ts (2)
9-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not mask an undefined setting value.
?? ""makes this pass ifbusinessEmailincorrectly returnsundefined. Assert the property directly.Proposed fix
- const result = settings.businessEmail ?? ""; - expect(result).toBe(""); + expect(settings.businessEmail).toBe("");As per coding guidelines, expected data must not be silently defaulted. <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("returns empty string when no business email is set", () => { expect(settings.businessEmail).toBe("");🤖 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/shared/validation/email.test.ts` around lines 9 - 11, Update the test around settings.businessEmail to assert the property directly instead of applying the nullish fallback. Remove the ?? "" default and verify the expected empty-string value from businessEmail itself, so undefined cannot pass silently.Source: Coding guidelines
69-85: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Measure the cache behavior being claimed.
Both tests pass if caching is removed or
invalidateCache()becomes a no-op. Record database/decryption calls, or mutate storage behind the cache and verify stale-before/fresh-after invalidation.As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
🤖 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/shared/validation/email.test.ts` around lines 69 - 85, Strengthen the tests around the settings cache by proving the first read performs database/decryption work while the second read reuses cached data, and by verifying invalidateCache causes subsequent reads to observe updated underlying storage or repeat decryption. Update the tests named “uses settings cache for reads” and “invalidateSettingsCache forces decrypt from database” with mutation-resistant assertions that fail if caching is removed or invalidation is a no-op.Source: Coding guidelines
test/ui/client/order.test.ts (2)
61-265: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract the browser harness and DOM helpers from this 763-line test file.
This block is a separate concern and pushes the suite far beyond the repository’s roughly 400-line limit. Move the harness/helper layer into a focused module and leave behavioral cases here.
As per coding guidelines, overloaded files must be split into focused modules. <coding_guidelines>
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 150-150: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: h.document.body.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation(unsafe-html-content-assignment)
[warning] 150-150: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: h.document.body.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation(dom-content-modification)
🪛 OpenGrep (1.25.0)
[ERROR] 116-116: new Function() with dynamic input can execute arbitrary code. Avoid dynamic code evaluation entirely, or use a safe alternative.
(coderabbit.code-injection.new-function-js)
[WARNING] 151-151: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🤖 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/ui/client/order.test.ts` around lines 61 - 265, Extract the browser harness and DOM helper symbols—harness, Harness, listing, setBody, addLink, shadow, cartButton, dialogEl, clickAnchor, clickIn, openCart, logHas, buttonType, textOf, storedCart, mountOpenListing, openCartWithOne, stepperButtons, and stubStorage—into a focused test-support module. Import them into order.test.ts and leave only behavioral test cases and their test-specific setup in the original file.Source: Coding guidelines
544-552: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the two focus-restoration paths independently.
The final
length > 0passes when either the close-button path or the dialogclosehandler is broken. Split the test or assert the call-count delta after each trigger.As per coding guidelines, tests must cover one behavior with mutation-resistant assertions. <coding_guidelines>
🤖 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/ui/client/order.test.ts` around lines 544 - 552, Update the test “the close button and the dialog close event return focus” to verify each focus-restoration trigger independently: capture the focus-call count before and after clicking .close, then assert that delta is positive; reset or use a new baseline before dispatching the dialog close event and assert its delta is also positive. Keep the existing dialog open-state assertions.Source: Coding guidelines
test/ui/templates/admin/activityLog.test.ts (2)
22-32: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the stale factory documentation.
The comment says only
message,attendee_id, andlisting_idvary, butcreatedandidare also override knobs andidis overridden below.As per coding guidelines, comments must explain the current code. <coding_guidelines>
🤖 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/ui/templates/admin/activityLog.test.ts` around lines 22 - 32, Update the documentation for the logEntry factory to accurately state that all properties in its overrides object, including created and id, can vary; remove the claim that only message, attendee_id, and listing_id are override knobs.Source: Coding guidelines
48-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Scope this
beforeAllinside a parent suite.This is a module-level BDD hook. Wrap the activity-log suites in a parent
describeand place the setup hook there to preserve test isolation.As per coding guidelines, global BDD hooks are prohibited. <coding_guidelines>
🤖 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/ui/templates/admin/activityLog.test.ts` around lines 48 - 51, Wrap the activity-log test suites in a parent describe block and move the existing beforeAll setup hook inside that parent suite. Keep setupTestEncryptionKey and signCsrfToken unchanged, ensuring the hook is scoped to these tests rather than remaining module-level.Source: Coding guidelines
test/ui/templates/admin/footer.test.ts (1)
18-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the shared top-level crypto/CSRF hooks.
All three suites mutate process-global state without resetting or isolating it, making parallel execution order-dependent.
test/ui/templates/admin/footer.test.ts#L18-L23: use isolated setup/reset or mark the file run-alone.test/ui/templates/admin/groups/overview/groups.test.ts#L15-L18: apply the same isolation strategy.test/ui/templates/attendee-table.test.ts#L18-L21: apply the same isolation strategy.As per coding guidelines, tests must not use global BDD hooks or share mutable state without isolation. <coding_guidelines>
📍 Affects 3 files
test/ui/templates/admin/footer.test.ts#L18-L23(this comment)test/ui/templates/admin/groups/overview/groups.test.ts#L15-L18test/ui/templates/attendee-table.test.ts#L18-L21🤖 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/ui/templates/admin/footer.test.ts` around lines 18 - 23, Remove the shared top-level crypto/CSRF hooks and isolate setup and reset of mutable process-global state in the affected suites: test/ui/templates/admin/footer.test.ts (lines 18-23), test/ui/templates/admin/groups/overview/groups.test.ts (lines 15-18), and test/ui/templates/attendee-table.test.ts (lines 18-21). Use per-test isolation or mark each file to run alone, ensuring parallel execution is not order-dependent and preserving the required CSRF-token setup.Source: Coding guidelines
test/ui/templates/admin/groups/overview/groups.test.ts (1)
322-340: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bind the defaults to listing 2's exact inputs.
Separate checks for
name="package_qty_2"and anyvalue="1"can pass when listing 2 has the wrong value. Assert the complete input or parse and inspect that named element; do the same for its blank price.As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
🤖 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/ui/templates/admin/groups/overview/groups.test.ts` around lines 322 - 340, Strengthen the assertions in the “renders saved overrides and falls back to defaults for members without a row” test by inspecting the exact inputs for listing 2. Bind the quantity assertion to name="package_qty_2" and verify its value is "1", and bind the price assertion to name="package_price_2" and verify it is blank, using complete input matching or parsed named elements.Source: Coding guidelines
test/ui/templates/admin/update.test.ts (1)
78-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the release-notes URL and label belong to the same anchor.
The two independent checks can pass for a broken link. Parse the HTML or assert the complete anchor relationship.
As per coding guidelines, tests must use strong mutation-resistant 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/ui/templates/admin/update.test.ts` around lines 78 - 82, Strengthen the “includes release notes link” test around adminUpdatePage so it verifies that GITHUB_RELEASES_URL and the “release notes” label occur within the same anchor element. Parse the rendered HTML or assert the complete anchor relationship, rather than relying on separate independent containment checks.Source: Coding guidelines
test/ui/templates/attendee-table.test.ts (3)
61-827: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this 827-line test module by responsibility.
Separate table rendering, sorting/address helpers, question data, and column-template contracts into focused colocated suites.
As per coding guidelines, files should remain roughly under 400 lines and separate overloaded concepts. <coding_guidelines>
🤖 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/ui/templates/attendee-table.test.ts` around lines 61 - 827, Split the oversized AttendeeTable test module into focused colocated suites, keeping the existing assertions and shared fixtures reusable: isolate table rendering/column behavior, sortAttendeeRows and formatAddressInline helpers, questionData rendering, and columnTemplate contracts into separate test files. Keep each file roughly under 400 lines and preserve the current describe/test coverage without changing production behavior.Source: Coding guidelines
752-761: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not codify silent fallback for an invalid column template.
Invalid configuration currently becomes a default table silently. Validate the template at its boundary and make this test assert the resulting validation error instead.
As per coding guidelines, invalid expected external data must fail loudly rather than use a fallback. <coding_guidelines>
🤖 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/ui/templates/attendee-table.test.ts` around lines 752 - 761, Update the invalid-template test around AttendeeTable and makeOpts so it expects the validation error for "{{invalid_column}}" instead of asserting default columns are rendered. Ensure template validation occurs at the AttendeeTable boundary and removes the silent fallback to the default column configuration.Source: Coding guidelines
810-825: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete default registered-date format.
Checking only
"2026"allows date, month, time, and ordering regressions to pass. Assert the exact expected formatted cell value.As per coding guidelines, tests must use strong mutation-resistant assertions. <coding_guidelines>
🤖 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/ui/templates/attendee-table.test.ts` around lines 810 - 825, The test covering the default registered-date rendering currently asserts only the year, allowing formatting regressions. Update the expectation in “renders default cell format when no filter applied” to assert the complete formatted cell value, including date, ordering, and time, while retaining the existing check that the Liquid strftime format is not used.Source: Coding guidelines
test/ui/templates/public/reservations/questions.test.ts (1)
135-159: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore saved form state in
finally.If rendering throws,
clearSavedFormData()is skipped and later tests inherit stale global state. Wrap each set/render/assert sequence intry/finally, or introduce a shared scoped helper that always clears the state.As per coding guidelines, tests must be isolated and must not share mutable state without reliably resetting it.
🤖 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/ui/templates/public/reservations/questions.test.ts` around lines 135 - 159, Update the saved-form-state tests around “restores a saved free-text answer from saved form data” and “restores selected select answers from saved form data” so cleanup always runs in a finally block, including when setup, rendering, or assertions throw. Keep each test’s existing assertions and saved data while ensuring clearSavedFormData is reliably invoked after every test.Source: Coding guidelines
|
Addressed the 30 findings from the review whose inline comments failed to post in
Verification: |
stefan-burke
changed the title
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@src/features/admin/listing-page-data.ts`:
- Around line 310-313: Document both intentional non-null assertions with
concise invariant comments: in src/features/admin/listing-page-data.ts lines
310-313, explain that the previously loaded LoadedListing guarantees the
activity-log lookup exists; in test/lib/server-attendee-merge-pin.test.ts lines
54-87, explain that a successful merge redirect guarantees the target attendee
still exists. Keep the existing ! assertions and behavior unchanged.
In `@src/features/admin/settings-listing-defaults.ts`:
- Around line 84-94: Update the submitted-day validation in the settings-listing
defaults handler to retain the first invalid token and return the invalid-day
error containing that token, while preserving the missing-days error when no
valid days are submitted. In
test/features/admin/settings-listing-defaults.test.ts lines 79-84, strengthen
the regression assertion to verify both the specific malformed value and the
returned invalid-day message.
In `@src/shared/db/listings/table.ts`:
- Around line 147-150: Update the parsed-value validation in the bookable_days
handling to require both an array and string type for every element before
returning it as string[]. Reject mixed-type arrays such as ["Monday", 1] with
the existing validation error path, and add a regression test covering that
stored input.
In `@src/shared/result.ts`:
- Around line 17-23: Update the generic requireSuccess overload so T is
constrained to either { ok: true } or { ok: false; error: string }, preventing
failed results without a string message. Change its assertion return type to use
Extract<T, { ok: true }>, while preserving the implementation’s existing error
throw behavior.
In `@test/shared/validation/email.test.ts`:
- Around line 31-56: Update the email persistence tests around
updateBusinessEmail to invalidate and reload CONFIG_KEYS.BUSINESS_EMAIL before
asserting settings.businessEmail, especially after clearing values. Ensure
assertions read the persisted encrypted database-backed value rather than only
the in-memory cache, while preserving the existing expected email values.
In `@test/ui/client/order/support.ts`:
- Around line 151-155: Rename the nullable host lookup helper from hostEl to
hostElOrNull and update all references, including callers such as
storage.test.ts; keep shadow using the renamed helper, while leaving any
mounted-widget invariant assertions to its callers.
- Line 38: Define a named Harness interface before the exported harness helper,
then add explicit return-type annotations to both harness and hostEl. Preserve
hostEl’s Queryable | null return contract and use the appropriate Harness return
type for harness.
In `@test/ui/templates/admin/activityLog.test.ts`:
- Around line 46-50: Replace the suite-scoped beforeAll setup in the “activity
log templates” describe block with a per-test helper that initializes the
encryption key and CSRF token and resets/restores the related mutable state
after each test. Update the tests in this suite to use that helper, preserving
the existing setup behavior without relying on a global BDD lifecycle hook.
In `@test/ui/templates/admin/footer.test.ts`:
- Around line 19-24: Remove the obsolete logout-form comment and CSRF
initialization from beforeAll in test/ui/templates/admin/footer.test.ts (lines
19-24). In test/ui/templates/admin/groups/overview/groups.test.ts (lines
364-371), rewrite the Actions-panel description to state its current ownership
directly, without “now” or “no longer.”
In `@test/ui/templates/attendee-table/actions.test.ts`:
- Around line 30-33: Strengthen the test “includes the current CSRF token” in
AttendeeTable by asserting that the expected csrf_token field name and current
token value appear together in the rendered input markup, rather than checking
the token value alone.
In `@test/ui/templates/attendee-table/questions.test.ts`:
- Around line 77-81: Update the test “counts Answers in an empty table layout”
to assert that the empty-state row uses a six-column colspan, reflecting five
default columns plus Answers. Keep the existing header and empty-table
assertions, and use a specific mutation-resistant assertion against the rendered
HTML.
🪄 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: e31ee6ad-b34f-44a2-b030-8f08a37bef45
📒 Files selected for processing (63)
src/features/admin/attendee-form-routes.tssrc/features/admin/attendee-notes.tssrc/features/admin/attendees-route-helpers.tssrc/features/admin/listing-page-data.tssrc/features/admin/settings-listing-defaults.tssrc/shared/column-order.tssrc/shared/db/activityLog.tssrc/shared/db/attendees/capacity/checks.tssrc/shared/db/attendees/queries.tssrc/shared/db/attendees/select.tssrc/shared/db/listings/table.tssrc/shared/db/questions/parsing.tssrc/shared/forms/definition.tssrc/shared/result.tssrc/shared/types.tssrc/shared/update.tstest/features/admin/catalog-transfer/import/fields.test.tstest/features/admin/settings-listing-defaults.test.tstest/integration/servicing/route-guards.test.tstest/lib/catalog-transfer.test.tstest/lib/code-quality.test.tstest/lib/db/attendees/apply-attendee-atomic-edit.test.tstest/lib/db/auth.test.tstest/lib/server-attendee-form/questions.test.tstest/lib/server-attendee-logistics-others.test.tstest/lib/server-attendee-logistics-tab.test.tstest/lib/server-attendee-merge-pin.test.tstest/lib/test-utils/db-contracts.test.tstest/shared/accounting/queries/operator-ledger.test.tstest/shared/apple-wallet/cms.test.tstest/shared/booking-date-fields.test.tstest/shared/column-order.test.tstest/shared/db/activityLog.test.tstest/shared/db/address-cache.test.tstest/shared/db/attendee-statuses.test.tstest/shared/db/attendees/balance.test.tstest/shared/db/attendees/capacity/checks/availability.test.tstest/shared/db/attendees/capacity/checks/batch.test.tstest/shared/db/attendees/capacity/groups.test.tstest/shared/db/attendees/delete.test.tstest/shared/db/attendees/queries/get-attendee.test.tstest/shared/db/attendees/stats.test.tstest/shared/db/listings/table.test.tstest/shared/db/migrations/2026-06-22_transfers_time_int.test.tstest/shared/db/questions/parsing.test.tstest/shared/forms/definition/field-schema.test.tstest/shared/result.test.tstest/shared/validation/email.test.tstest/test-utils/activity-log.tstest/ui/client/order.test.tstest/ui/client/order/storage.test.tstest/ui/client/order/support.tstest/ui/templates/admin/activityLog.test.tstest/ui/templates/admin/footer.test.tstest/ui/templates/admin/groups/overview/groups.test.tstest/ui/templates/admin/update.test.tstest/ui/templates/attendee-table/actions.test.tstest/ui/templates/attendee-table/columns.test.tstest/ui/templates/attendee-table/questions.test.tstest/ui/templates/attendee-table/shared.tstest/ui/templates/attendee-table/template.test.tstest/ui/templates/attendee-table/values.test.tstest/ui/templates/public/reservations/questions.test.ts
1 participant
What changed
#test/aliases for shared test helpers and add#cli/for the top-level CLI package.OrNull.mainchanges and clear the PR conflict.Report improvement
The first report captured 385 mirrored source files. The updated report has 440 mirrored source files, despite
mainchanging the source-file total during this work.Current report:
Checks
deno task unit-tests-reportdeno task precommitdeno task precommit:mutationcannot build its changed-test map for this rename-heavy PR because it rejects moved tests whose source files are unchanged. It exits before running mutants; it does not report surviving mutants.Summary by CodeRabbit
Bug Fixes
Tests