Read through the table wherever the table can answer - #2016
Merged
Conversation
Follow-up to #2015: fixes two things review found in that reader, and moves the reads that were waiting on it. Two reads a table could not answer honestly are refused again. A filter could name a value worked out from another table, which reached the database as a column it has never had; the filter now checks the column against the table. And a whole-row read promised the whole row while selecting only stored columns, so a listing came back without its image values — that refusal is back, dropped in #2015 on the mistaken grounds that the shared CRUD path needed whole rows. It only asks whether a row is there, so it asks read.exists now, which fetches one column instead of decrypting all of them. A read can also be handed out as a statement, for callers this reader could not reach: inside an open transaction, or as one leg of a batch. Those had kept hand-written SQL, with column lists nothing checked against the table. Nine such reads now go through the table: the site-page and question existence checks inside their write transactions, the activity log (three reads folded onto one declared order, one of them a leg of a batch), email templates, the groups cache, and the status defaults a write reads back — which now arrive as the booleans they are declared to be rather than 1 and 0. Every mutant of the reader and of attendee-statuses is killed, including four gaps in the status default rules that predate this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0145jX1UQn15ZvXtkoqNz1pe
Contributor
📝 WalkthroughWalkthroughThe change extends the table-reader API with ChangesTable reader API
Typed database query migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Resource
participant TableReader
participant Database
Resource->>TableReader: table.read.exists(filter)
TableReader->>Database: select primary key
Database-->>TableReader: matching key or no row
TableReader-->>Resource: boolean result
Resource-->>Resource: continue or return not found
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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 `@test/integration/server/listings-create.test.ts`:
- Around line 111-112: Remove the duplicate comment near the create-path
read-back assertion, keeping only one concise plain-language comment describing
the non-obvious replica-read behavior.
In `@test/integration/server/listings-error-pages.test.ts`:
- Around line 217-221: Update the comments above the readStub2 exists stub to
state that it returns false, simulating the listing being deleted between the
initial check and update; remove the outdated nullable-row wording and keep the
explanation concise.
In `@test/shared/db/attendee-statuses.test.ts`:
- Around line 102-117: Strengthen both save tests in
test/shared/db/attendee-statuses.test.ts: lines 102-117 should re-read seed
after saving and assert is_paid_default and is_public_default are exactly true;
lines 119-135 should assert the created status has both flags exactly true while
retaining the existing assertions that seed lost both flags.
- Around line 79-83: Update the test “the seeded status is the word an operator
sees” to load the seeded attendee-status row through the production database
access path, then assert that the persisted status name is “Confirmed.” Replace
the constant-only assertion while preserving the test’s observable behavior
focus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f92b7b3c-6edb-4fc1-9fd2-f90add54d1f0
📒 Files selected for processing (18)
src/shared/db/activityLog.tssrc/shared/db/attendee-statuses.tssrc/shared/db/built-sites.tssrc/shared/db/email-templates.tssrc/shared/db/groups.tssrc/shared/db/questions/attendee-answers/save.tssrc/shared/db/site-page-items.tssrc/shared/db/site-pages.tssrc/shared/db/table-reader.tssrc/shared/rest/resource.tstest/integration/server/bookable-alone.test.tstest/integration/server/groups/edit-delete.test.tstest/integration/server/listings-create.test.tstest/integration/server/listings-error-pages.test.tstest/shared/db/attendee-statuses.test.tstest/shared/db/built-sites/crud.test.tstest/shared/db/table-reader/filters.test.tstest/shared/db/table-reader/reads.test.ts
Comment on lines
+111
to
+112
| // The optional replica read misses the row; the primary read-back the | ||
| // create path actually uses must still find it. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the duplicate test comment.
Lines 109-112 state the same replica-read behavior twice. Keep one concise comment.
As per coding guidelines, “Keep comments short, current, plain-language, and limited to non-obvious reasons.”
🤖 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.test.ts` around lines 111 - 112,
Remove the duplicate comment near the create-path read-back assertion, keeping
only one concise plain-language comment describing the non-obvious replica-read
behavior.
Source: Coding guidelines
Comment on lines
+217
to
+221
| // updateResource.update which checks whether the row is still there. | ||
| // Return null to simulate the listing being deleted | ||
| // between the initial check and the update. | ||
| const readStub2 = stub(listingsTable.read, "one", () => | ||
| Promise.resolve(null), | ||
| const readStub2 = stub(listingsTable.read, "exists", () => | ||
| Promise.resolve(false), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale comment to match the exists-based stub.
The comment says "Return null to simulate the listing being deleted", but the stub now returns Promise.resolve(false) because listingsTable.read.exists returns a boolean, not a nullable row. Update the wording to describe the boolean result.
📝 Proposed comment fix
- // updateResource.update which checks whether the row is still there.
- // Return null to simulate the listing being deleted
- // between the initial check and the update.
+ // updateResource.update which checks whether the row is still there.
+ // Return false to simulate the listing being deleted
+ // between the initial check and the update.
const readStub2 = stub(listingsTable.read, "exists", () =>
Promise.resolve(false),As per coding guidelines, "Keep comments short, current, plain-language, and limited to non-obvious reasons; do not narrate code or describe historical implementations."
📝 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.
Suggested change
| // updateResource.update which checks whether the row is still there. | |
| // Return null to simulate the listing being deleted | |
| // between the initial check and the update. | |
| const readStub2 = stub(listingsTable.read, "one", () => | |
| Promise.resolve(null), | |
| const readStub2 = stub(listingsTable.read, "exists", () => | |
| Promise.resolve(false), | |
| // updateResource.update which checks whether the row is still there. | |
| // Return false to simulate the listing being deleted | |
| // between the initial check and the update. | |
| const readStub2 = stub(listingsTable.read, "exists", () => | |
| Promise.resolve(false), |
🤖 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-error-pages.test.ts` around lines 217 - 221,
Update the comments above the readStub2 exists stub to state that it returns
false, simulating the listing being deleted between the initial check and
update; remove the outdated nullable-row wording and keep the explanation
concise.
Source: Coding guidelines
Comment on lines
+79
to
+83
| test("the seeded status is the word an operator sees", async () => { | ||
| // The name is what an attendee's status reads as until anyone renames it, | ||
| // so it has to be a real word rather than blank. | ||
| expect(DEFAULT_ATTENDEE_STATUS_NAME).toBe("Confirmed"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the seeded database row.
This assertion only checks DEFAULT_ATTENDEE_STATUS_NAME. It does not verify the seeded status that an operator reads. Load the seeded status and assert its persisted name.
Proposed fix
test("the seeded status is the word an operator sees", async () => {
- expect(DEFAULT_ATTENDEE_STATUS_NAME).toBe("Confirmed");
+ expect((await attendeeStatuses.getAll())[0]?.name).toBe("Confirmed");
});As per coding guidelines, tests must call production code and test 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.
Suggested change
| test("the seeded status is the word an operator sees", async () => { | |
| // The name is what an attendee's status reads as until anyone renames it, | |
| // so it has to be a real word rather than blank. | |
| expect(DEFAULT_ATTENDEE_STATUS_NAME).toBe("Confirmed"); | |
| }); | |
| test("the seeded status is the word an operator sees", async () => { | |
| // The name is what an attendee's status reads as until anyone renames it, | |
| // so it has to be a real word rather than blank. | |
| expect((await attendeeStatuses.getAll())[0]?.name).toBe("Confirmed"); | |
| }); |
🤖 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/attendee-statuses.test.ts` around lines 79 - 83, Update the
test “the seeded status is the word an operator sees” to load the seeded
attendee-status row through the production database access path, then assert
that the persisted status name is “Confirmed.” Replace the constant-only
assertion while preserving the test’s observable behavior focus.
Source: Coding guidelines
Comment on lines
+102
to
+117
| test("a default status may be saved while keeping its defaults", async () => { | ||
| const seed = (await attendeeStatuses.getAll())[0]!; | ||
|
|
||
| // Keeping a default is not giving it up, so this must save rather than be | ||
| // refused for leaving the site without one. | ||
| const saved = await attendeeStatusWrites.save( | ||
| seed.id, | ||
| statusInput("Still confirmed", { | ||
| isPaidDefault: true, | ||
| isPublicDefault: true, | ||
| }), | ||
| ); | ||
|
|
||
| expect(saved.ok).toBe(true); | ||
| expect((await getAttendeeStatus(seed.id))?.name).toBe("Still confirmed"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the default flags after each save.
These tests can pass if the write clears the old flags but does not persist the requested flags.
test/shared/db/attendee-statuses.test.ts#L102-L117: Readseedafter the save and assertis_paid_defaultandis_public_defaultare bothtrue.test/shared/db/attendee-statuses.test.ts#L119-L135: Read the created status and assertis_paid_defaultandis_public_defaultare bothtrue, in addition to asserting thatseedlost both flags.
As per coding guidelines, tests must test observable behavior and use strong exact assertions.
📍 Affects 1 file
test/shared/db/attendee-statuses.test.ts#L102-L117(this comment)test/shared/db/attendee-statuses.test.ts#L119-L135
🤖 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/attendee-statuses.test.ts` around lines 102 - 117, Strengthen
both save tests in test/shared/db/attendee-statuses.test.ts: lines 102-117
should re-read seed after saving and assert is_paid_default and
is_public_default are exactly true; lines 119-135 should assert the created
status has both flags exactly true while retaining the existing assertions that
seed lost both flags.
Source: Coding guidelines
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.
Follow-up to #2015. It fixes two things review found in that change, and then moves across the reads that were waiting on the fix.
Two reads a table could not answer honestly
Both were caught by Codex on #2015, and both are right.
A filter could name something the table does not store. A listing's image values are worked out from another table — they are part of a listing, but not columns of
listings. The filter accepted them anyway, and the read reached the database asking for a column it has never had. The filter now checks each column against the table, the same check that choosing columns already made, so this is caught as the call is written.A whole-row read promised more than it selected. It said it returned a whole listing while selecting only stored columns, so a listing came back with its image values missing even though its type says they are there. The refusal for those tables is back.
I dropped that refusal in #2015 believing it could not survive, because the shared admin CRUD path reads any table by key and would have hit it on a normal listing edit. That was wrong about what the CRUD path needs: it only asks whether the row is there and throws the row away. It now asks a new
read.exists, which fetches just the key — so a table with worked-out values answers it fine, and every other table stops decrypting a full row to answer a yes-or-no.Reads that can now go through the table
A read can also be handed out as a statement, for the callers this reader could not reach: ones that must run inside an already-open write transaction, or as one leg of a batch. Those had all kept hand-written SQL, with column lists nothing checked against the table.
Nine reads moved across:
Testing
deno task precommitpasses: lint, typecheck, duplicate check, copy check, edge build, full suite at 100% coverage.Both fixes have a regression test that reproduces the reported case, each confirmed to fail with its fix removed. Mutation testing kills every mutant of the reader (19/19) and of attendee-statuses (71/71) — the latter including four gaps in the status default rules that predate this change: keeping a default while renaming, saving a status that holds no default, and a new default status taking the flags off the old one. Mutation testing also found genuinely dead code in my own new
exists, since removed.Generated by Claude Code
Summary by CodeRabbit
Performance
Reliability
Bug Fixes