Public: anyone can read this; the crew can change it.

/apps and the dashboard query once per row again: two N+1 guards are red #503

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

The dashboard no longer runs extra queries per row: with 30 apps and 30 assigned issues, `/` runs the same number of SQL statements as with 5. All 13 tests in `test/surfaces-adversarial.test.ts` pass, and `bun run check` is green.

shipped production is serving it deploy 2fa0c24

opened this

The dashboard no longer runs extra queries per row: with 30 apps and 30 assigned issues, / runs the same number of SQL statements as with 5. All 13 tests in test/surfaces-adversarial.test.ts pass, and bun run check is green.

The extra query. I logged every statement and the call stack. canSeeOwnerFacts, which just calls canWrite, was being called once per repository. Each call cost one owner lookup (SELECT * FROM user WHERE id = ?1) and one team-permission read in orgAccess. On the dashboard it came in through needsYouCard → askCountFor → askCandidatesFor in src/web/studio/asks.ts, which checked every visible repo (up to 200) this way before reading pulls. That took the page from 38 to 88 statements, and caused both red dashboard guards (including the needsYouCard one). shelfRows (src/web/studio/shelf.ts) and the board's scope (src/web/pages/board.ts) did the same per-row check, so I changed them too. The /apps guard named in the issue already passed on this main.

The fix.

  • orgAccessByOrg (src/core/orgs.ts): returns the viewer's highest permission in each org they belong to, from one query.
  • ownerFactRepoIds (src/core/repos.ts): the list version of canSeeOwnerFacts. It applies canWrite's rules exactly (owner, site admin, or a write/admin team in the owning org) using that one read.
  • I didn't reuse writableRepoIds: its SQL (REPO_WRITABLE) also lets crew members on the repo through, which canWrite doesn't, so it would have changed who sees owner facts.
  • A new test in test/orgs.test.ts checks the batched answer against canSeeOwnerFacts repo by repo for an admin, a writer, a reader, an outsider and a signed-out viewer, and that the batch runs at most one query.

The gate. scripts/suite.ts and its EXCLUDED list no longer exist; the gate is now the explicit file list in forge.json's check.run. I added test/surfaces-adversarial.test.ts and test/orgs.test.ts to that list. No existing test was changed.

On the first full gate run, test/fork-flow.test.ts › "a mirror whose git is not ready yet is fetched again, and lands" failed once. This change doesn't touch that code; the file passes 15/15 on its own with the gate's timeout, and the next full run was green.

Not done: other single-repo pages (now, app, previews) still call canSeeOwnerFacts once per page; that is one call per page, not per row.

To try it: bun test test/surfaces-adversarial.test.ts test/orgs.test.ts, then bun run check.

🤖 Generated with Claude Code

Closes #292

commented

Fixes the dashboard N+1 from #292: with 30 apps and 30 assigned issues, / now runs the same number of SQL statements as with 5. All 13 tests in test/surfaces-adversarial.test.ts pass, and that file now runs in the gate.

The extra query. canSeeOwnerFacts (which is canWrite) ran once per repository. Each call cost an owner lookup (SELECT * FROM user WHERE id = ?1) plus the team-permission read in orgAccess. On the dashboard it came in through needsYouCard → askCountFor → askCandidatesFor in src/web/studio/asks.ts, which checked every visible repo (up to 200) this way before reading pulls. With 30 org apps the page went from 38 to 88 statements, which turned both dashboard guards red. shelfRows (src/web/studio/shelf.ts) and the board's scope (src/web/pages/board.ts) did the same per-row check. The /apps guard named in the issue already passed on main.

The fix.

  • orgAccessByOrg (src/core/orgs.ts): returns the viewer's highest permission in each org they belong to, from one query.
  • ownerFactRepoIds (src/core/repos.ts): answers canSeeOwnerFacts for a whole list from that one read, using canWrite's rules exactly (owner, site admin, or a write/admin team in the owning org).
  • Not writableRepoIds: its SQL (REPO_WRITABLE) also admits crew members on the repo, which would have changed who sees owner facts.
  • New test in test/orgs.test.ts: checks the batched answer against canSeeOwnerFacts repo by repo for an admin, a writer, a reader, an outsider and a signed-out viewer, and that it runs at most one query.

The gate. scripts/suite.ts and its EXCLUDED list no longer exist; the gate is the file list in forge.json's check.run. This pull adds test/surfaces-adversarial.test.ts and test/orgs.test.ts to it. The merge from main conflicted only on that line: main had added test/web-apps.test.ts, and the merged list keeps all three. No existing test was changed.

bun run check is green after the merge (637 tests in 43 files). In an earlier full run before the merge, test/fork-flow.test.ts › "a mirror whose git is not ready yet is fetched again, and lands" failed once. This change doesn't touch that code; the file passed 15/15 twice on its own with the gate's timeout, and every full run since has been green.

Not done: the single-repo pages (now, app, previews) still call canSeeOwnerFacts once per page; that is one call per page, not per row.

To try it: bun test test/surfaces-adversarial.test.ts test/orgs.test.ts, then bun run check.

🤖 Generated with Claude Code

commented

The forge now accepts check.run. On main it was already 998 characters. Merging main into this branch added test/fleet-ops.test.ts, which pushed the list past the forge's 1000-character limit together with the two files this branch gates (test/surfaces-adversarial.test.ts, test/orgs.test.ts). I fixed it by moving the list of test files out of forge.json and into a file, without dropping any of them.

  • scripts/gate.txt is a new file that lists the 44 test files the forge's check runs, one per line. Blank lines and # comments are skipped. It is the merge of main's list and this branch's two extra files.
  • forge.json: check.run is now bun x tsc --noEmit && bun scripts/test-shards.ts --timeout 60000 --gate scripts/gate.txt.
  • scripts/shards.ts: a new parseGate reads the list. One bad line, a file listed twice, or an empty list rejects the whole file, so a typo can't quietly make the gate run fewer tests.
  • scripts/test-shards.ts: a new --gate <file> option adds the listed files to any named on the command line. After that it works as before: it checks every file exists and weighs files by time from scripts/test-times.json. bun run check:record still works, because it still finds scripts/test-shards.ts once in check.run.
  • test/shards.test.ts has new tests for parseGate (comments, ./, bad lines, repeated files, empty list). Two more guard the real repo: every file in scripts/gate.txt exists, and check.run stays under 1000 characters and points at the list.
  • README.md and CLAUDE.md now say where the gated test files are listed and why.

bun x tsc --noEmit is clean and test/shards.test.ts passes (28 tests). A full bun run check ran 670 tests in 44 files with one failure: "a newer commit supersedes a check still waiting for the same environment" in test/checks.test.ts expected the middle check to be cancelled but got ok. That file doesn't touch anything this change edits, and all 23 of its tests passed when I re-ran it alone. It looks like a timing-sensitive test failing under parallel load, so I left it unchanged and did not re-run the full gate. The merge commit is pushed.

To try it: run bun run check. The first line should read test-shards: 44 files in N shards.

🤖 Generated with Claude Code

commented

Operator: main moved under this branch (forge.json's check command was shortened, among others). Merge main into the branch, resolve the conflicts keeping this change's intent, run the check locally, and push.

commented

/apps, the dashboard, the shelf and the board now check owner facts with one query per page instead of one per row, so both N+1 guards pass again. The fix is the earlier commit 1e8cfed: src/core/repos.ts and src/core/orgs.ts load the facts for all rows at once, and board.ts, shelf.ts and asks.ts call that instead of asking once per row. test/orgs.test.ts now covers it.

The guards live in test/surfaces-adversarial.test.ts and test/orgs.test.ts, and the check never ran either file, which is how both went red unnoticed. Both are now on the test list in forge.json's check.run.

This push merges main into the branch. Main shortened check.run to -- name … short names (splitArgs in scripts/shards.ts). That fixes the same 1000-character limit this branch's scripts/gate.txt and --gate flag were for. In the conflicts I took main's version and removed scripts/gate.txt, parseGate and their wording in README.md and CLAUDE.md, so the branch keeps only its fix and the two added test names (check.run is about 540 characters). bun run check passes locally: 710 tests across 50 files.

To try it, run bun run check, or open /apps and the dashboard on the preview with several apps and repos.

🤖 Generated with Claude Code

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs