Let a read say what it wants instead of spelling out SQL - #1982
Conversation
…ot yet run Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
We had three good pieces that did not fit together. A table knew its columns and how to read them. A set of filters knew which rows to keep and what values to fill them with. But every read still wrote the join between them by hand: the SELECT, the FROM, the WHERE, the ORDER BY, the LIMIT. That handwriting is where the last of the raw SQL lived. A column list now knows how to fetch itself. Ask it for the rows you want and it writes the query, because it already knows its own table and columns. Thirteen reads across images, listings, the catalog, API keys, site pages, groups and notes now declare what they want. Two of them had taken a piece of SQL as text from their callers; those doors are closed. Notes had grown a fourth way of describing which rows to fetch, days old. It now uses the same one as everything else, which also means asking for notes about no records costs nothing rather than being refused. Every reader skips the database when what it asked for cannot match any row, through one shared piece rather than three copies of the same check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 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 (4)
📝 WalkthroughWalkthroughThe PR replaces table projections with chosen-column readers, adds shared typed query helpers, migrates database read paths, updates listing and image row types, and renames ledger projection helpers. ChangesDatabase read migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
A pass over the new code against AGENTS.md, before anyone else reads it: The column-list code moved out of the table file into its own. That file had grown to 955 lines, forty short of the size we fail a build at, and reading a chosen set of columns is its own idea with its own name. Its tests moved with it. The helper that skips a pointless read is now named for what it checks — that no row can match — rather than sounding like it checks whether the rows are empty. The check behind it is no longer offered to callers, since nothing outside used it; two tests that reached for it now watch what actually happens instead. The ledger's newest-first list still built its own ending — the filter, the order and the cap — as did the activity log and the column lists. All three now share one piece, and say what they return by its proper name rather than spelling the shape out again. Three long comments cut back to the one thing each was there to say. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
![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: 312cf445cd
ℹ️ 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".
| * joins or carries a subquery writes its own SQL and runs it through | ||
| * {@link TableProjection.queryAll}. | ||
| */ | ||
| export type ProjectionSelect = { |
There was a problem hiding this comment.
Rename the newly exported ProjectionSelect surface, and the associated new module, around the plain concept already stated here—reading chosen table columns. projection is explicitly listed as CS jargon to avoid when a plain phrase works, and exposing it in the public type and import path spreads that vocabulary to every future database reader.
AGENTS.md reference: AGENTS.md:L69-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and this is a fair hit — I introduced projection into a module name and an import path during a pass whose whole purpose was conforming to AGENTS.md, where the word is listed by name.
Fixed in 2faaed9, and I went wider than the new surface. Renaming only ProjectionSelect would have left a plainly-named file exporting jargon, so the whole vocabulary moved:
| Was | Now |
|---|---|
db/projection.ts |
db/chosen-columns.ts |
defineTableProjection |
chooseColumns |
TableProjection |
ChosenColumns |
ProjectionSelect |
ReadRequest |
StoredTableProjectionRow |
StoredRowOf |
ItemImageProjection |
ItemImageColumns |
xxxProjection locals |
xxxColumns |
The call site reads as the plain sentence now: chooseColumns(rawListingsTable, ["id", "name", "active"]), then .select({ where, order }).
The word was in comments and two error messages as well, so those changed too — "Cannot select projected column X" became "Cannot select X from Y: it is not one of its columns", and the two tests pinning those messages were updated.
66 references across 12 files; full precommit green.
Generated by Claude Code
| * Tests for a projection's declared read (`select` / `selectOne` in | ||
| * `src/shared/db/table.ts`), which composes the projection's columns and table | ||
| * with the shared filter clauses so an ordinary single-table read needs no SQL. |
There was a problem hiding this comment.
Update this comment to reference src/shared/db/projection.ts: this commit moved select and selectOne out of table.ts, so the current pointer now sends maintainers to a file that no longer contains the code under test. The repository requires comments to describe the current code rather than preserve stale information.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2faaed9 — the comment pointed at table.ts, which no longer holds the code under test.
The rename in the sibling thread moved the module again, so the comment now names src/shared/db/chosen-columns.ts, and the test directory moved to test/shared/db/chosen-columns/ to mirror it. That also keeps the mutation gate's rule that a source file has a direct test at the matching path.
Generated by Claude Code
Our own guide lists "projection" as a word to avoid when a plain phrase says the same thing, and this branch had just put it in a file name and in the import line of every reader that picks its columns. The idea is choosing a set of a table's columns, so that is what it is called now: you choose columns, and what you get back is the chosen columns. The error messages and comments say it in plain words too, and the tests sit beside the file they cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
…rn-collections-pfvtod
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2faaed9bc5
ℹ️ 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".
|
|
||
| /** Positive cost total for one listing. Cost legs source `cost:<listingId>`. */ | ||
| export const costProjection = | ||
| export const costColumns = |
There was a problem hiding this comment.
The new costColumns export does not read or return columns: it returns a curried calculation of one listing's numeric cost from transfers, and profitColumns repeats the same mismatch. These public names now suggest database column selection instead of ledger calculations, so use plain domain names such as costOfListing and profitOfListing.
AGENTS.md reference: AGENTS.md:L69-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it exposed a bigger problem than the two names you flagged.
I did the projection → chosen columns rename with a tree-wide regex (\b(\w+)Projection\b), which caught every identifier ending in that word regardless of what it meant. Auditing all 22 renames, three were wrong:
costProjection/profitProjection— ledger sums, exactly as you say. NowcostOfListing/profitOfListing, which reads better than the old name too.CatalogProjection,StoredValueProjection,fieldSupportsProjectionincatalog-fields/definition.ts— these describe how a catalog field is stored, not columns. My rename producedfieldSupportsColumns, which means nothing. Reverted to their original names; renaming those properly isn't this change's business.
The other 19 are genuine column selections and stay renamed. I also made listingDayPriceColumns singular, since it builds one column.
Fixed in ef36044. Current main is merged in as well — the branch had gone behind. Full precommit green.
Generated by Claude Code
Renaming "projection" to "chosen columns" was done with a search and replace across the whole tree, and it caught names that had nothing to do with reading columns. Two of them work out money from the ledger — one listing's cost and its profit — so they are now named for the values they return. Three others describe how a catalog field is stored, and are back to the names they had, since renaming those is not this change's business. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/shared/db/chosen-columns.ts`:
- Around line 129-135: Update statementFor to include table.primaryKey in the
internal SELECT whenever the requested columns omit it, so row-ID-dependent
transforms receive the stored key while the public read result still returns
only the requested columns. Preserve the existing selection when the primary key
is already requested, and add a regression test covering select() without id
using the local tag transform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f73573a4-62af-4add-a3cf-9e9d18b4e033
📒 Files selected for processing (33)
src/features/public/types.tssrc/shared/accounting/queries.tssrc/shared/db/activityLog.tssrc/shared/db/api-keys.tssrc/shared/db/attributes.tssrc/shared/db/chosen-columns.tssrc/shared/db/groups.tssrc/shared/db/groups/candidates.tssrc/shared/db/images.tssrc/shared/db/listings/attendees.tssrc/shared/db/listings/catalog.tssrc/shared/db/listings/records.tssrc/shared/db/listings/select.tssrc/shared/db/listings/table.tssrc/shared/db/news-posts.tssrc/shared/db/notes/queries.tssrc/shared/db/notes/target.tssrc/shared/db/site-pages.tssrc/shared/db/table.tssrc/shared/db/where-clauses.tssrc/shared/ledger/project.tssrc/shared/types.tssrc/ui/templates/public/reservations/form.tsxsrc/ui/templates/public/reservations/og-tags.tssrc/ui/templates/public/reservations/ticket-page.tsxsrc/ui/templates/public/reservations/types.tssrc/ui/templates/public/shared.tsxtest/integration/api-keys.test.tstest/shared/db/chosen-columns/columns.test.tstest/shared/db/chosen-columns/select.test.tstest/shared/db/notes/target.test.tstest/shared/db/where-clauses.test.tstest/shared/ledger/project.test.ts
💤 Files with no reviewable changes (1)
- src/shared/db/table.ts
Some columns are stored sealed, and if one will not open the reader records which row it came from so an operator can find it. It works that out from the row's own key — but a read that had not asked for the key never received it, so the record would have said "image undefined" and named nothing. Reads now fetch the key whether or not it was asked for, and drop it again before handing the row back, so the caller still gets exactly the columns it chose. No read does this today: the two image reads happen to ask for the key already. It was waiting for the next one that did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab550d6087
ℹ️ 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".
| import { describeWithEnv } from "#test-utils/db.ts"; | ||
| import { createTestListing } from "#test-utils/db-helpers/listings.ts"; | ||
|
|
||
| describeWithEnv("db > table > declared projection reads", { db: true }, () => { |
There was a problem hiding this comment.
Fresh evidence after the earlier API rename is that this new chosen-columns test still labels the suite declared projection reads; test/shared/db/chosen-columns/columns.test.ts likewise retains ProjectionRow, projectionTable, and projection_rows. This leaves the explicitly forbidden term throughout the tests for the renamed mechanism, so rename these fixtures and descriptions consistently around “chosen columns.”
AGENTS.md reference: AGENTS.md:L69-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — the rename stopped at the production surface and left the word standing in the tests for the very mechanism it renamed. Fixed in ccff42b:
| Was | Now |
|---|---|
ProjectionRow |
SampleRow |
projectionTable |
sampleTable |
"projection_rows" |
"sample_rows" |
"db > table > declared projection reads" |
"db > chosen columns > declared reads" |
The suite title also said db > table >, which was stale twice over — the code moved out of table.ts two commits ago — so it now names the module it actually tests.
One word stayed on purpose: "runs projected column transforms" names col.projected, a table.ts column builder this branch doesn't touch. Renaming a production builder is a separate change, and the test title should keep matching the thing it calls.
Full precommit green.
Generated by Claude Code
The tests for the renamed mechanism still called their fixture table a projection, which is the word the rename was meant to remove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
…rn-collections-pfvtod
2 participants
What changed
We had three good pieces that did not fit together.
A table already knew its columns and how to read them. A set of filters already
knew which rows to keep and which values fill them. But every read still wrote
the join between those two by hand — the SELECT, the FROM, the WHERE, the
ORDER BY, the LIMIT. That handwriting is where the last of the raw SQL lived,
and where a filter could drift away from its values.
A column list now knows how to fetch itself. You tell it which rows you want,
in what order, and how many; it writes the query, because it already knows its
own table and its own columns.
What that tidied up
Thirteen reads across images, listings, the catalog, API keys, site pages,
groups and notes now say what they want instead of spelling out a query.
Two doors closed. Site pages and notes each took a piece of SQL as text
from their callers — the same door this work has been shutting elsewhere. Both
now take described filters.
A fourth way of asking, days old. Notes had grown its own way of describing
which rows to fetch. It now uses the same one as everything else. A pleasant
side effect: asking for the notes of no records used to be refused outright,
and now simply costs nothing.
One check instead of three. Every reader skips the database when what it
was asked for cannot match any row. That check had been written out three
separate times; it is now one shared piece that all of them go through.
What is deliberately left alone
Reads that join a second table or carry a subquery still write their own SQL —
images by use, attributes by listing, news posts, the group candidate list, and
the two big listing and attendee readers. They keep using the same column lists
and the same filters underneath, so nothing is duplicated; they simply say
more than a single-table read can.
The aim is one way to say which rows, one way to say which columns, and no
handwriting in between — not a layer that hides what a complicated query does.
Tests
single-row form, and that a read which cannot match anything asks the
database nothing.
for a single row. The columns it selects — what that test is about — are
unchanged.
Full checks pass, including complete test coverage.
Generated by Claude Code
Summary by CodeRabbit