Let notes be about any record, not only an attendee - #1978
Merged
Conversation
…ull suite not yet run Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
The single read had a fallback that no input could reach, only there to satisfy the compiler. Opening one note is now the shared step a batch is built from, so there is no unreachable branch to test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
…moved test not stop a mutation run The mutation gate demands a test at the mirrored path; the note routes' suite was under test/integration, so the gate had nothing direct to mutate against. It drives real pages through the session helpers, which is what test/features holds. The walk that finds test files also died on a path that had moved: the guard for a missing directory was around creating the reader, but the disk is only reached when the entries are read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
…e none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR generalizes encrypted system notes from attendee-only storage to typed entity targets, adds the schema migration and deletion path, updates application consumers and tests, and extracts mutation glob expansion into reusable tooling. ChangesEntity-based notes migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AdminPage
participant NotesQueries
participant SystemNotes
participant NoteSealing
AdminPage->>NotesQueries: request notes for attendee target
NotesQueries->>SystemNotes: select entity_type and entity_id rows
NotesQueries->>NoteSealing: open sealed note rows
NoteSealing-->>AdminPage: return decrypted notes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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: fc912b906b
ℹ️ 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".
The probe that decides which steps are still to do now reads the primary: a replica can lag behind columns an earlier attempt added, and adding one twice is an error. Asking for the notes of no records threw away its own SQL and was reached only by its test. It now says so loudly, and the one live empty path stays where it saves a round trip. The importer plan told a future reader to repoint notes by a column this change removes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8063d91be7
ℹ️ 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".
…s many The glob expansion moved into its own module so the missing-directory fix has a direct test; it fails against the old lazy-readDir shape with the same NotFound the gate hit. One record's notes are now asked for as a list of one, so filtering and ordering cannot drift from the list reads. Comments describe the design as it is rather than what it replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
The catch that classified the error had arms no test could take. Asking statOrNull first says the same thing in one step, and the base a glob walks from is always absolute, so its empty-path arm went too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
The copy that makes a mutation run's snapshot collected directory entries the same way the glob walk did. Both now ask the shared walk helper, which also gained the missing-folder answer, and the glob walk stops carrying its own copy of a recursive walk that already existed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb7beb2b1a
ℹ️ 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".
![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: 4
🤖 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 `@scripts/mutation/isolation-state.ts`:
- Line 143: Update copyDirectory at the directoryEntries call to use a strict
directory-reading helper that throws when fromDir is missing or is not a
directory, rather than returning an empty list. Preserve normal recursive
copying and add a regression test covering a missing source child that verifies
the mutation snapshot operation fails.
In `@scripts/walk-files.ts`:
- Around line 10-18: Update directoryEntries to wrap the statOrNull check and
Deno.readDir iteration in error handling, returning an empty array for expected
missing-path or ENOTDIR failures, including directories removed between calls,
while rethrowing unexpected errors. Add a regression test covering a file path
treated as a directory.
In `@src/features/admin/attendees-edit.ts`:
- Around line 171-174: Update the stale-note cleanup around getNotesFor to
collect IDs for matching system refund-failure notes, then delete them through
the available batch operation or concurrently as independent deletions instead
of awaiting each deletion sequentially. Keep the existing note filtering and
attendeeNotes target, and ensure the refresh path stays within the 40-call limit
when provider calls are included.
In `@src/shared/db/migrations/2026-07-28_note_entities.ts`:
- Line 27: Update src/shared/db/migrations/2026-07-28_note_entities.ts at lines
27-27 so the attendee_id-absent branch calls syncIndexes() before returning. Add
a regression test in test/shared/db/migrations/2026-07-28_note_entities.test.ts
at lines 99-120 that simulates a rebuilt table with unsynchronized indexes,
reruns the migration, and asserts idx_system_notes_entity is restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 303d24e1-35bc-404a-bd15-cc16ce4850ce
📒 Files selected for processing (54)
TODO.mddocs/importer-plan.mdscripts/mutation.tsscripts/mutation/expand.tsscripts/mutation/isolation-state.tsscripts/walk-files.tssrc/features/admin/attendee-notes.tssrc/features/admin/attendee-page.tssrc/features/admin/attendees-edit.tssrc/features/admin/attendees-list.tssrc/features/admin/listing-page-data.tssrc/features/api/payment-processing/store-refund.tssrc/shared/db/attendees/delete.tssrc/shared/db/migrations/2026-06-23_system_notes.tssrc/shared/db/migrations/2026-07-28_note_entities.tssrc/shared/db/migrations/registry.tssrc/shared/db/migrations/schema/tables-questions.tssrc/shared/db/notes/queries.tssrc/shared/db/notes/sealing.tssrc/shared/db/notes/target.tssrc/shared/db/notes/types.tssrc/shared/db/system-notes.tssrc/ui/templates/admin/attendee-notes.tsxsrc/ui/templates/admin/attendees-list.tsxsrc/ui/templates/admin/listings/types.tstest/features/admin/attendee-notes.test.tstest/integration/migration-restore-verify.test.tstest/integration/migration-round-trip-budget.test.tstest/integration/server/attendee-form-status.test.tstest/integration/server/attendees-list.test.tstest/integration/server/attendees/placeholder-refresh.test.tstest/integration/server/listings/show-actions-and-activity.test.tstest/integration/server/payments-success-refunds.test.tstest/integration/server/payments/replay.test.tstest/integration/server/webhooks/multi-ticket-refunds.test.tstest/integration/server/webhooks/price-signature-stored-refund-and-ignore.test.tstest/integration/servicing/purge-edge-cases.test.tstest/integration/webhook-price-signature/helpers.tstest/scripts/mutation/expand.test.tstest/shared/db/attendees/delete.test.tstest/shared/db/migrations/2026-07-28_note_entities.test.tstest/shared/db/migrations/registry.test.tstest/shared/db/migrations/schema/tables-questions.test.tstest/shared/db/migrations/schema/version/guard.test.tstest/shared/db/notes/queries.test.tstest/shared/db/notes/sealing.test.tstest/shared/db/notes/target.test.tstest/shared/db/orphan-attendees.test.tstest/specs/steps/payment-capacity.tstest/test-utils/reservation/helpers.tstest/test-utils/webhooks.tstest/ui/templates/admin/attendee-notes.test.tsxtest/ui/templates/admin/attendee-page.test.tstest/ui/templates/admin/attendees-list.test.ts
💤 Files with no reviewable changes (1)
- src/shared/db/system-notes.ts
… deletes The shared walk is strict again — a caller naming a root that is not there is asking about something it believes in, and a snapshot copy that answered nothing would report success on an empty copy. Only the glob expansion tolerates a path that has moved, which is the case it is for. A run that died right after the table rebuild left no index behind, and the next run returned early without making one. It now asks for the indexes on that path too. Adding a column is taken as done when the column is already there: a primary read can still briefly answer with the schema as it was. Stale notes go in one batch, and the version label names this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
stefan-burke
enabled auto-merge
July 28, 2026 21:14
stefan-burke
disabled auto-merge
July 28, 2026 21:14
A column already there is taken as done; anything else means the database could not do what was asked. Only the tolerated case had a test, so the arm that lets a real failure through was never run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f459d62b9
ℹ️ 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".
I argued on review that a stale read could only send the migration backwards into work every step tolerated. That was wrong about one step: if the rebuild had finished, moving the notes across failed on an attendee_id that had already gone. Each step now asks the database and believes its answer — a column already there, an attendee_id already gone — through one shared helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
… nothing Mutation testing showed both: blanking either "already so" message still passed, because one test failed every statement and the other message threw regardless; and the index sync after the rebuild could be removed with nothing noticing, because the rebuild takes the table's indexes from SCHEMA as it goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdDZCWRXwjqsidfRJFarXC
…grations-status-yl8jsv
Lines moved in test-browser.ts and attendee-page.ts, so the recorded positions no longer named the same mutants.
stefan-burke
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e7844584
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/shared/db/migrations/2026-07-28_note_entities.test.ts`:
- Around line 180-203: Update the finishedDatabase mock used by
noteEntities().up() so its execute method returns a stale schema-read result for
the initial pragma_table_info query instead of rejecting it. Keep the table
initialized in the migrated state, and restrict the simulated “already done”
errors to the ALTER and UPDATE statements so the migration reaches and tolerates
those mutation errors.
In `@test/shared/db/notes/queries.test.ts`:
- Around line 227-235: Update the empty-list test around deleteNotes to
instrument the database executor before calling
deleteNotes(attendeeNotes(owner), []), then assert the executor call count is
unchanged afterward. Keep the existing row-preservation assertion, and verify
the zero-query contract directly rather than inferring it from note survival.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 024c186a-64e6-4500-8fbe-4f0f2a7172c8
📒 Files selected for processing (15)
TODO.mddocs/importer-plan.mdscripts/mutation/equivalent-mutants.txtscripts/mutation/expand.tsscripts/walk-files.tssrc/features/admin/attendee-notes.tssrc/features/admin/attendees-edit.tssrc/shared/db/migrations/2026-07-28_note_entities.tssrc/shared/db/migrations/schema/version.tssrc/shared/db/notes/queries.tssrc/shared/db/notes/sealing.tstest/scripts/mutation/expand.test.tstest/shared/db/migrations/2026-07-28_note_entities.test.tstest/shared/db/migrations/schema/version/guard.test.tstest/shared/db/notes/queries.test.ts
The test's name promised a zero round-trip contract but only checked the note survived, so an empty batch would have passed it.
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
2 participants
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.
What changed
Notes used to belong to an attendee and nothing else. Each note now says what
kind of record it is about, and which one. Nothing has changed for the people
using the site: every note that exists today is a note about an attendee, and
it still shows up in the same places. What changed is that the store is now
ready for notes on other kinds of record — a listing, a booking — without
another change to the table.
Why
The importer and the entity pages both want notes on records that are not
attendees. Doing it one record type at a time would mean a new column each
time. Naming the record instead means the store is written once.
How
attendee_idcolumn for a pair: the kind of recordand its id.
src/shared/db/notes/, holds the notes store: the kindsof record a note can be about, how a note's text is sealed and opened, and the
reads and writes. The part that decides which rows to touch is plain
data-in, data-out code with no database in it, so it is easy to test on its
own.
notes and asking for many take the same path.
attendee_id, andrebuilds the table without it. It can be run again safely: each step takes the
database's own word for whether it is already done, so a run that stopped
halfway is picked up by the next one.
Testing
deno task precommitpasses.rate.
rebuilt index, and for a real failure at each statement being raised rather
than swallowed.
Summary by CodeRabbit
New Features
Documentation
Refactor