Split server-attendees.test.ts into themed test files - #1681
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR splits the server-attendees tests into focused suites, adds shared helpers for attendee and merge flows, updates biome config for the new layout, and expands coverage across add, detail, edit, check-in, delete, merge, payment, questions, and resend-notification endpoints. ChangesAdmin attendees test suite
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/lib/server-attendees/checkin.test.ts`:
- Around line 76-81: The test body in checkin.test.ts uses an unnecessary inline
dynamic import for getListingActivityLog even though `#test-utils` is already
statically imported; add getListingActivityLog to the existing top-level import
list and remove the await import("`#test-utils`") inside the check-in assertion
block.
In `@test/lib/server-attendees/delete.test.ts`:
- Around line 154-169: The mismatched attendee name test only checks the flash
message and does not verify that return_url is preserved in the redirect. Update
the deleteAction test case in delete.test.ts for the "preserves return_url on
mismatched attendee name" scenario to assert the response Location header points
to the expected redirect target, using the existing deleteAction and response
assertions so a missing return_url cannot slip through.
In `@test/lib/server-attendees/helpers.ts`:
- Around line 164-204: The return type of mergePairWithQuestion currently claims
a2 is always an Answer, but the function can be called with only one answer text
and then answers[1] is undefined. Update the mergePairWithQuestion signature and
returned object so a2 is optional (or Answer | undefined), and keep the rest of
the helper behavior unchanged; this makes the contract accurate for callers like
mergeNonConffictingAnswer and any future destructuring use.
- Around line 37-49: The getMergeVersion helper is using a non-null assertion on
the extracted merge_version input, which can hide a missing field and defer
failure to submitMerge or other tests. Update getMergeVersion to validate the
result of extractInputValue(html, "merge_version") explicitly and throw a clear
error if the hidden input is absent, so failures are reported at the source
instead of passing undefined through.
- Around line 97-106: The `firstAttendee` helper is throwing a generic error and
dropping the `reason` from the `{ success: false; reason: string }` result.
Update `firstAttendee` to include that failure reason in the thrown error
message so setup failures surface the real cause, and review the new unit test
around `firstAttendee` so it does not assert on an exact fixed literal message.
In `@test/lib/server-attendees/merge-conflicts.test.ts`:
- Around line 118-129: Hoist the ad hoc imports used in the merge-conflicts test
into the existing top-level imports so the test body doesn’t dynamically import
modules that are already available. Update the test around createTestGroup to
use the existing `#test-utils` import, and move queryAll from `#shared/db/client.ts`
into the file’s static imports near the other dependencies. Keep the test logic
the same, but reference the already imported createTestGroup and queryAll
symbols directly.
In `@test/lib/server-attendees/merge-post.test.ts`:
- Line 121: The tests in merge-post.test.ts are redundantly re-importing modules
that are already available or used multiple times, including
getListingActivityLog, getAttendeeRaw, and queryAll, with inconsistent local
naming. Hoist these repeated dynamic imports to the top-level static imports in
the test file, then update the affected test cases to use the shared imported
symbols consistently so the import style is uniform and duplication is removed.
In `@test/lib/server-attendees/resend-notification.test.ts`:
- Line 30: Update the stale describe titles in resend-notification.test.ts so
they match the actual endpoint being exercised. The two describe blocks
currently reference the old
"/admin/listing/:listingId/attendee/:attendeeId/resend-notification" shape, but
the tests and the handler in attendees.ts use
"/admin/attendees/:attendeeId/resend-notification"; rename the describe strings
accordingly so they accurately describe the route under test.
- Around line 81-97: The mismatch-name test is in the wrong describe group and
duplicates existing POST coverage. Move the resend-notification POST case in the
test file into the POST-focused block around the other resendNotification tests,
or remove it if it adds no new assertion beyond the existing "rejects mismatched
attendee name" case. Keep the test logic tied to handleRequest,
followRedirectWithFlash, and setupAdminTest so the placement matches the HTTP
method being exercised.
🪄 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: 0f51b437-87b2-47f4-8aac-c4e5e4d8a0da
📒 Files selected for processing (15)
biome.jsontest/lib/server-attendees.test.tstest/lib/server-attendees/add-attendee.test.tstest/lib/server-attendees/attendee-detail.test.tstest/lib/server-attendees/attendee-edit.test.tstest/lib/server-attendees/checkin.test.tstest/lib/server-attendees/delete-incomplete.test.tstest/lib/server-attendees/delete.test.tstest/lib/server-attendees/helpers.tstest/lib/server-attendees/merge-conflicts.test.tstest/lib/server-attendees/merge-panel.test.tstest/lib/server-attendees/merge-post.test.tstest/lib/server-attendees/payment.test.tstest/lib/server-attendees/questions.test.tstest/lib/server-attendees/resend-notification.test.ts
💤 Files with no reviewable changes (1)
- biome.json
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 973f34a4de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** Submit the admin add-attendee form for `listing` with the given fields, | ||
| * injecting the session's CSRF token. Returns the response. */ | ||
| export const submitAddAttendee = async ( |
There was a problem hiding this comment.
AGENTS.md says refactored files, including test files, should stay under roughly 400 lines; this new shared helper is 542 lines, so the attendee split still leaves a large grab-bag test module that exceeds the repo's documented maintainability limit. Please split these helpers by area before treating the monolith as fully broken up.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in b1737bd. Split the merge helpers (mergePair, mergePairWithQuestion, assignMergeAnswers, submitMerge, getMergeVersion, mergeWithAnswerConflict, mergeNonConflictingAnswer) into a new merge.ts (240 lines). helpers.ts is now 313 lines — both under the ~400 target from AGENTS.md.
| } from "#test-utils"; | ||
|
|
||
| /** A slot for one of the optional contact fields on a direct attendee booking. */ | ||
| export type DirectBooking = { |
There was a problem hiding this comment.
A repo-wide search shows no test imports this type; it is only used inside helpers.ts, and the same new helper module also exports expectMergeAnswer even though only a local helper calls it. AGENTS.md's dead-code rule explicitly calls out unused exports, so these exports create a needless public helper API after the split; keep them file-local instead.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in b1737bd. Removed the export from DirectBooking type and expectMergeAnswer — both are only used inside their own module (merge.ts / helpers.ts), so they are now file-local. Per AGENTS.md dead-code rule, unused exports are a signal to remove them, not allow-list them.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/server-attendees/merge.ts`:
- Around line 144-193: Hoist the late `await import(...)` calls in
`assignMergeAnswers` and `expectMergeAnswer` into the existing top-level import
block, since `saveAttendeeAnswers`, `getAttendeesByTokens`, and
`getAttendeeAnswersByQuestion` are already available as static imports in this
test helper file. Update the `#shared/db/questions.ts` and
`#shared/db/attendees.ts` imports at the top of the module, then replace the
function-local dynamic imports in `assignMergeAnswers` and `expectMergeAnswer`
with those imported symbols.
🪄 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: b34e551a-21c5-4bd2-b7ff-379a54d6141d
📒 Files selected for processing (5)
test/lib/server-attendees/helpers.tstest/lib/server-attendees/merge-conflicts.test.tstest/lib/server-attendees/merge-panel.test.tstest/lib/server-attendees/merge-post.test.tstest/lib/server-attendees/merge.ts
…d has 49 clones (0.3%) Split the 3674-line monolith (grandfathered in biome.json's noExcessiveLinesPerFile override) into 12 themed files under test/lib/server-attendees/, each under the 1000-line Biome hard limit: delete, delete-incomplete, checkin, add-attendee, attendee-detail, attendee-edit, resend-notification, payment, questions, merge-panel, merge-post, merge-conflicts (+ helpers.ts with getMergeVersion). Pure relocation via verbatim line-range extraction; no assertions changed. Removed the biome.json exclusion entry. Biome trimmed each file's imports to actual usage. All 186 tests pass; lint clean. Remaining: cpd surfaces 49 clones (0.3%) the monolith previously hid (jscpd skips very large files). Next commit extracts shared helpers to dedup.
…, cpd still 37 clones
Extracted shared helpers into test/lib/server-attendees/helpers.ts:
setupListingAndAttendee, setupListingAndDirectAttendee, mergePair,
mergePairWithQuestion, assignMergeAnswers, submitMerge, firstAttendee,
expectFlashPage, submitDeleteIncomplete, getMergeVersion.
Applied to merge-{panel,post,conflicts}, delete, checkin, attendee-detail.
All 186 tests still pass. Remaining: dedup payment, delete-incomplete,
attendee-edit, resend-notification, add-attendee, questions.
Extracted 15 shared helpers into helpers.ts and applied across all 12 test files. cpd now reports 0 clones (was 49 / 0.3% when the split surfaced the monolith's hidden duplication). All 186 tests pass, lint clean. Running full precommit next.
- getMergeVersion: replace if-throw guard with non-null assertion (the merge page always contains merge_version; the throw was defending against an impossible state) - refreshPaymentAsStripe: replace ?? [] with ! (isPaymentRefunded is always called during refresh; the fallback was impossible) - firstAttendee: add in-process unit test exercising the throw branch on a failed booking result
…rrors, fix describe titles - Hoist repeated dynamic imports (getListingActivityLog, getAttendeeRaw, queryAll, createTestGroup) to static top-level imports in checkin, merge-conflicts, and merge-post test files. - getMergeVersion: restore explicit throw with descriptive message when merge_version input is missing (was using a silent non-null assertion). - firstAttendee: include result.reason in the thrown error message. - mergePairWithQuestion: type a2 as Answer | undefined (the type was lying — a2 is undefined when only one answer is passed; callers that need it now assert with a2!). - resend-notification: update stale describe titles to match the actual route (/admin/attendees/:attendeeId/resend-notification, not the old listing-scoped shape). - resend-notification: move the mismatch-name POST test from the GET describe block into the POST describe block where it belongs.
The throw created an untestable branch (the merge preview template always renders merge_version, so the branch can never fire in normal operation). AGENTS.md: 'Don't defend against the impossible.' Reverted to the non-null assertion with a comment citing the invariant.
…ernal types
Address CodeRabbit P2 findings:
- helpers.ts was 542 lines; split the merge helpers (mergePair,
mergePairWithQuestion, assignMergeAnswers, submitMerge, getMergeVersion,
mergeWithAnswerConflict, mergeNonConflictingAnswer) into a new merge.ts
(240 lines). helpers.ts is now 313 lines — both under the ~400 target.
- Made DirectBooking type and expectMergeAnswer function file-local (removed
export) — they are only used inside their own module, so per AGENTS.md's
dead-code rule they should not be part of the public helper API.
- Updated merge-{panel,post,conflicts}.test.ts imports accordingly.
Main (commit 706ab9d, PR #1660) added 'a merge that adopts the source's email re-homes Previous bookings' to the deleted server-attendees.test.ts. Re-inserted it into merge-post.test.ts, in the POST /merge describe block between 'rejects self-merge' and 'merges source listings into target' — matching main's placement.
stefan-burke
force-pushed
the
split-server-attendees
branch
from
July 9, 2026 21:11
b1737bd to
ad8a176
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/server-attendees/add-attendee.test.ts`:
- Around line 220-221: The negative-path assertion in add-attendee.test.ts is
too weak because expect.stringContaining("") matches any flash message. Update
the test around the response/status check to assert the specific validation
message or error contract returned by the attendee validation path, using the
existing expectFlash helper so the test verifies the actual error text instead
of an empty substring.
In `@test/lib/server-attendees/helpers.ts`:
- Around line 205-224: Hoist the `#shared/db/questions.ts` imports out of
`attendeeAnswerIds` and `saveAttendeeAnswer` so they use the existing static
import block instead of function-local `await import(...)`. Update the helpers
to reference `getAttendeeAnswersBatch` and `saveAttendeeAnswers` from the
top-level import alongside the other symbols already imported from that module,
matching the refactor pattern used elsewhere in this PR.
In `@test/lib/server-attendees/payment.test.ts`:
- Around line 251-269: The Stripe refund test currently only checks the error
flash, but it should also verify that the payment still appears unrefunded after
the ledger failure. Update the test around refreshPaymentAsStripe in
payment.test.ts to add a state/UI assertion after the POST, using the existing
attendee/payment setup and response handling, so the refund flow cannot regress
by showing an error while still marking the payment refunded.
- Around line 68-71: The Stripe test fixture is using a live-style secret key
prefix, which can trigger secret scanners. Update the test setup in the payment
test to use a Stripe test-key value instead, keeping the fixture synthetic but
prefixed like a test credential. Use the existing settings.setForTest call in
the payment test to locate and replace the stripe_secret_key value.
In `@test/lib/server-attendees/resend-notification.test.ts`:
- Around line 264-271: The resend-notification test is relying on a zero-delay
timer to wait for the fire-and-forget webhook, which is race-prone and can make
the assertion flaky. Update the test around adminFormPost and webhookFetch to
wait on a deterministic completion signal instead of setTimeout(..., 0), such as
exposing an awaitable promise from the test helper or using a deferred webhook
stub that resolves when the request is actually dispatched, then assert the
webhook call after that await.
🪄 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: dcbae62b-ada5-4612-bf0a-02e92daaa212
📒 Files selected for processing (16)
biome.jsontest/lib/server-attendees.test.tstest/lib/server-attendees/add-attendee.test.tstest/lib/server-attendees/attendee-detail.test.tstest/lib/server-attendees/attendee-edit.test.tstest/lib/server-attendees/checkin.test.tstest/lib/server-attendees/delete-incomplete.test.tstest/lib/server-attendees/delete.test.tstest/lib/server-attendees/helpers.tstest/lib/server-attendees/merge-conflicts.test.tstest/lib/server-attendees/merge-panel.test.tstest/lib/server-attendees/merge-post.test.tstest/lib/server-attendees/merge.tstest/lib/server-attendees/payment.test.tstest/lib/server-attendees/questions.test.tstest/lib/server-attendees/resend-notification.test.ts
💤 Files with no reviewable changes (1)
- biome.json
stefan-burke
enabled auto-merge
July 9, 2026 21:23
stefan-burke
disabled auto-merge
July 9, 2026 21:23
… elsewhere CodeRabbit flagged that merge.ts and helpers.ts still use dynamic await import() for #shared/db/questions.ts symbols that are already statically imported at the top of each file. Hoisted saveAttendeeAnswers, getAttendeeAnswersByQuestion (merge.ts), and getAttendeeAnswersBatch, saveAttendeeAnswers (helpers.ts) into the static import blocks, plus getAttendeesByTokens from #shared/db/attendees.ts in merge.ts.
Five pre-existing test weaknesses CodeRabbit flagged on PR #1681 were relocated verbatim from the original monolith; the PR's scope (pure relocation + cpd dedup) forbade changing what the tests assert. Recorded here for follow-up: vacuous empty-substring assertion, live Stripe key fixture, missing unrefunded-state assertion, race-prone setTimeout(0) webhook sync, and missing return_url Location-header check.
stefan-burke
enabled auto-merge
July 9, 2026 22:00
stefan-burke
added a commit
that referenced
this pull request
Jul 10, 2026
Resolved conflicts: - server-attendees.test.ts (+ server-parents-gate.test.ts): main split these monoliths into themed files (#1681, #1684); accepted main's split (deleted the monoliths) and repointed the new themed files' imports of the deleted #shared/db/questions.ts onto the split sub-modules (question-types, attendee-answers/{save,reads}, queries, tables). - server-booking-preserve.test.ts: main refactored to use the assignQuestion test helper, so dropped the now-unused direct setListingQuestions/ answersTable/questionsTable imports.
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splits the largest excluded test file (
test/lib/server-attendees.test.ts, 3674 lines) into 12 themed test files undertest/lib/server-attendees/, each under the 1000-line Biome hard limit, and fixes every cpd duplicate the split surfaced.What changed
test/lib/server-attendees.test.tsfrom thenoExcessiveLinesPerFileexclusion inbiome.json(the last override entry the file was grandfathered under) and deleted the original 3674-line monolith.describeWithEnvblock each — pure relocation, no changed assertions:delete.test.ts— GET/POST/DELETE attendee delete + parseAttendeeIdsdelete-incomplete.test.ts— POST delete-incompletecheckin.test.ts— checkin/checkout + no-quantity row action guardsadd-attendee.test.ts— POST add attendee (email/phone/both/daily/validation/flash)attendee-detail.test.ts— GET attendee detail + edit form renderingattendee-edit.test.ts— POST edit attendee (validation/update/PII/quantity/dual-path)resend-notification.test.ts— GET/POST resend-notification + package rehydrationpayment.test.ts— payment details + refresh-payment (Stripe/refunded/ledger)questions.test.ts— edit attendee questions (show/save/update/clear/invalid)merge-panel.test.ts— merge panel GET + preview branchesmerge-post.test.ts— POST merge (PII/booking conflict/skip/take_source)merge-conflicts.test.ts— merge answer conflicts + take_source on packageDeduplication
A file this large had been invisible to jscpd (it skips very large files), so the split surfaced 49 clones (0.3%) that had been hiding inside the monolith. All 49 are fixed by extracting shared helpers into two files:
helpers.ts(313 lines) — setup, attendee creation, form submission, flash/assertion helpersmerge.ts(240 lines) — all merge-related helpers (mergePair, submitMerge, answer conflict resolution)Final cpd: 0 clones at the 0% threshold.
Verification
deno task lint— cleandeno task test:files test/lib/server-attendees/— 187 tests passdeno task cpd— 0 clonesdeno task precommit— all steps green (lint, typecheck, cpd, build:edge, test:coverage at 100%, mutation)Summary by CodeRabbit