Repository navigation
fix(example): prepare project access before read activation - #487
Merged
Merged
Conversation
`projects.list` asked "owner OR member" of `projects`, which no index spans, so
it walked the table from the newest end and post-filtered in JS. A viewer with
nothing to see drained the whole table, and every scanned row joined the
subscription read set. Measured 40 scanned / 0 returned at 40 rows.
Two more defects were hiding behind it:
- `.filter()` hands the callback the raw Convex row, whose id field is `_id`, so
`memberProjectIds.has(project.id)` was `has(undefined)` and always false.
Projects visible only through a membership never appeared in the list at all.
- `hasAnyProject` was unbounded too: `{ ownerId, archived }` ties `index('archived')`
and `index('ownerId')` at the same planner score, declaration order wins, and the
loser becomes a post-filter over the whole partition. 40 and 43 reads against
bounds of 2 and 3.
Add `projectAccess`, one row per (user, project), indexed
`(userId, archived, projectCreatedAt)`. `_helpers/project_access.ts` owns every
grant/revoke and is the single owner of "which projects can this user see", used
by `list`, `listForDropdown` and `hasAny`. 4 reads at 10 noise rows and 4 at 400.
Maintained by explicit mutation helpers, not schema triggers: async cascade
continuation workers build their ORM from the raw `ctx.db` and drop triggers past
the first batch, so a trigger-maintained copy would drift. Deletion needs neither,
both FKs cascade and cascades still run in those batches. `revokeProjectAccess`
re-derives entitlement rather than deleting blindly, and `grantProjectAccess`
derives `isOwner` from the project so a caller cannot mislabel a row.
Also repair the read-bound harness, which was measuring nothing:
- `countDocumentReads` patched `db.get` as a one-arg forwarder while the lifecycle
writer only calls the two-arg overload, so `oldDoc`/`newDoc` came back null and
every trigger was silently skipped.
- The example read-bound tests installed the counter after the ORM was built, so
they measured a constant 0. Fixing placement exposed the `hasAnyProject` scan and
a tag-merge bound that was really 16, not 12 (same at 40 and 120 target rows, so
the property it pins still holds).
Bounds are asserted as equalities so a dead counter fails loudly.
Add compound `isPublic_archived` for the unauthenticated branch, which had the
same planner tie and read every archived public project.
`projectAccess` started empty on any deployment that already held projects, so switching `list`, `listForDropdown` and `hasAny` onto it made all three return nothing. `backfillProjectAccess` existed but was referenced only by its test and `migrations/manifest.ts` was empty, so there was no callable path to populate it. Add `20260906_022807_backfill_project_access`, scaffolded with `kitcn migrate create` and registered in the manifest. It walks `projects` in batches and re-derives each project's rows; `grantProjectAccess` updates rather than inserts, so re-running is safe. `down` clears them. The runner pages `ctx.db.query(table)`, so `migrateOne` receives the RAW Convex document -- `_id`, not `id`, and no hydrated `createdAt`. Same trap that made the old `.filter()` membership check dead. Re-read the row through the ORM instead of handing that shape to a helper expecting the public one. Extract `syncProjectAccessForProject` as the single owner of "what should this project's access rows look like", shared by the migration and `backfillProjectAccess`, so a deployment repaired by the migration and one rebuilt in a test cannot drift. This also fixes a latent bug in the old backfill, which capped memberships globally at 1000 rather than per project. Test drives the migration's own `migrateOne` over raw docs: reads are empty before, correct after, re-running does not duplicate, `down` empties. Verified armed by neutering the migration and watching it fail.
The backfill read each project's memberships in one `limit: 1000` call. Nothing caps a project's member count -- `addMember` only rejects the owner, a duplicate, and a non-owner caller -- and `findMany` truncates silently rather than throwing (probe: 5 rows, `limit: 3`, returned 3, no error). Members past the bound would get no access row, and `list`, `hasAny` and `listForDropdown` would hide a project they belong to, permanently and with the migration reporting success. Split the backfill into one migration per source table: owners over `projects`, memberships over `projectMembers`. The runner pages each table itself, so neither is bounded by a limit this code picks. That shape is also what keeps it correct at runtime. A cursor loop inside `migrateOne` would issue a second `.paginate()` inside the runner's own paginated read, which real Convex rejects with `MultiplePaginatedDatabaseQueries` while convex-test does not model it -- it would have passed tests and failed in prod. `backfillProjectAccess` is now explicitly test-only and reads `limit + 1` so it throws on truncation instead of quietly backfilling a prefix. Tests drive both migrations over raw `ctx.db` docs and cover every member row; both were verified armed by neutering each migration in turn.
… bounds docs Four review findings on #452, all verified against source. Member-migration rollback deleted owner-derived rows. An owner can also hold a `projectMembers` row -- `aggregateDemo` seeds exactly that -- so `migrate down --steps 1` removed the access row the still-applied owner migration created, and the owner lost their own project from every read. The member `down` now leaves the row alone when the user is the project's current owner. Dropdown lost capacity. `listProjectsForDropdown` bounds the union with one limit, where the read it replaced bounded each side separately: up to 1000 owned plus up to 1000 member. A user with 750 of each silently lost 500 from an unpaginated dropdown. The caller now passes the combined figure. `syncProjectArchived`'s comment was wrong. It claimed the update was "bounded by the project's member count, which addMember already gates"; `addMember` caps nothing. The real ceiling is the ORM's `mutationMaxRows` (10,000), above which the update throws. Comment corrected to state that; a member cap or a batched sync is a product decision, not one to make here. Rollout window documented. `kitcn deploy` pushes the backend and only then runs migrations, so a deployment holding existing projects shows empty lists until both backfills finish. Acceptable for a re-seedable example app; the owner migration now says so and points at expand/backfill/contract for apps where it is not. Both new tests verified armed by reverting the fix under them.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Member
Author
|
Autoclosure receipt for Preparation source is verified. Canonical public readers remain active; dependent activation #452 requires per-target preparation deployment and both completed backfill receipts. No remote deployment or merge performed. Full bun check passed, including fixture/verify/runtime lanes. Clean committed-branch autoreview and three independent source lenses. After the final identity push, all24 focused tests passed, including real authenticated write/access state, existing-data preparation reads, >1000 membership backfill, fractional ordering and1500-project dropdown. CLI-generated static artifacts retained. Plan checker passed. Live raw inventory has 3 top-level comments, 0 reviews and 0 threads. Zero actionable P1-or-higher findings. Required CI status is tracked separately; no merge authorization inferred.
Explicit user deferral on2026-10-14: archive above10000 access rows remains P2 in #452 (comment). No cap or async archive contract introduced. |
This was referenced Oct 14, 2026
This branch had an error being deployed
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
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.
🐛 Fixes ➖ N/A. Preparation for #443; dependent activation is #452.
🧭 Task plan: docs/plans/443-project-access-preparation.md
🟢 95-100% confidence in preparation source and proof. Activation remains gated.
✅ Outcome
Prepare projectAccess schema, maintained entitlement rows and independently paged owner/member backfills. Public list, dropdown and hasAny retain their canonical implementations during preparation. Real authenticated mutation tests inspect access state through grants, revokes, archive/restore, transfer and deletion cascades.
Deploy this revision and complete both backfills on every target before activating #452. No real remote deployment or backfill has been executed here. The archive limit above10000 access rows is explicitly deferred by the maintainer; its original review thread remains open. The unchanged main member-list filter defect is replaced by the activation PR.
🏗️ Design
Separate deployable preparation and activation revisions preserve the migration boundary. No readiness flag or compatibility reader. Entitlement writes live in one example helper; foreign keys own deletion cascades. Raw creation times retain fractional precision. Dropdown hydration uses bounded ID batches.
🧪 Verified
Three independent cleanup lenses completed. Full bun check passed, including all eight generated fixture checks and runtime scenarios. Convex and kitcn CLI regenerated static API/data model/procedure locations against an isolated anonymous backend. Frozen committed-branch autoreview passed with no accepted/actionable findings. Package source is unchanged; no changeset required.
Need help on this PR? Tag
@codesmith-botwith what you need.