chadtmiller.com/knotless

Switch repository

Loading…

What changes

A commit's page no longer waits for its whole diff. It asks getCommit with firstRows, and the first files come with their rows, up to 10 files or 1500 lines, as a pull's files page has them. The rest are listed with their line counts and fetched from getCommit with paths, ten at a time, as each nears the screen; a file changing more than 400 lines waits to be asked for.

getCommit takes firstRows and paths, both optional: asked without them it answers as before, so the CLI, MCP and the iOS app are unchanged.

The fetching is shared. What the pull files page did inline is now web/src/lib/later.svelte.ts, used by PullDiff and by Diff when given more. The new-pull page still draws its whole diff.

Why

On prod, getCommit for a 292-file commit (e996a5e6 on chadtmiller.com/knotless) is a 2.2 MB answer, 450 KB compressed, and takes over a second on the server each time it is viewed, every file read and highlighted, then all of it drawn before any shows. The pull files page solved this already; the commit page now does the same.

How it was checked

a_merge_commit_shows_what_it_brought_in now covers firstRows (12 files: the first 10 with rows, the rest with only their counts), paths and the 50-path limit. The test helper asks for a commit page's data as the page does. npm run check passes. The shots' recorded commit answers are re-keyed with &firstRows=true: their commits are small enough that the answer is the same. Builds and the rest are left to CI.

Shortcomings

A file still to come shows the pull page's sized placeholder until it arrives; it is fetched 800px before it reaches the screen, so most readers will not see it. The answer is not cached by commit: it depends on who asks for a private repository, and with firstRows the work left is the patch alone.

This moves the pull files page's lazy row fetching into LaterRows (later.svelte.ts). The commit page now asks getCommit with firstRows and fetches the remaining files with paths. getCommit takes the new parameters as repeated pairs, limits paths to 50, and answers as before when they are absent.

I found nothing that must change. The PullDiff refactor keeps its behaviour. DiffFile already has the waiting, ask and near props that Diff now passes. The test covers firstRows, paths and the 50-path limit, and the docs, lexicon and API reference are updated with the code.

One point to check in CI: getCommit no longer reads sha through RepoParams, so it now rejects a request missing repo or sha with the combined message "repo and sha are required". If any recorded answer or test expects the old message, the test check will show it.

This is a re-review at a new head (9c66e91), which adds a merge of main and one commit, "The commit page draws its files as before, highlighted at once". I found nothing new that must change. The code I approved last round is unchanged. getCommit takes firstRows and paths and limits paths to 50. LaterRows is shared by PullDiff and Diff, and the commit page passes more.

Last round I asked you to check whether the combined "repo and sha are required" message broke any test or recorded answer. The earlier test run passed. The checks for this head are still queued, so I haven't seen them.

The one thing to watch is shots. The run on the previous head failed because commit and commit-merged moved by a few pixels. It said "moved, but not named", so a Shots-moved: line in the body would be needed if they still move. The new commit looks aimed at that. If shots still reports movement at this head, look at the wrapper <div use:seen> that Diff now puts around each file when it is given more. If the movement is intended, add Shots-moved: commit, commit-merged to the body. The shots check enforces this, so I'm not holding approval on it.

Carried forward: the change this pull makes at c4fd7ef0016c75b583974222e38715ef440dee70 is the one reviewed at 9c66e9161456e917dc58522e8526e9f98ec2bafe, so that review stands. What is new comes from its base.

This is a re-review at head 8b29e6c. It adds a merge of main and one commit, which turns the ok_or_else in follow_once (moderation.rs) into .context(format!(..)). That commit has nothing to do with the commit-page work. It compiles, since anyhow::Context is already imported and works on Option. The error text is unchanged, so I have no objection to it. It would have been tidier as its own pull.

The commit-page change I approved earlier is unchanged: getCommit takes firstRows and paths (limited to 50), LaterRows is shared by PullDiff and Diff, and the commit page passes more.

The checks for this head are queued or running and I haven't seen them. At the previous head, c4fd7ef, the test check failed after about 995 s, and I don't know why. shots passed at that head, so the Shots-moved question is settled. Look at the test result at this head. If it fails again, read the log to see whether it is a timeout or a real failure in a_merge_commit_shows_what_it_brought_in. These checks enforce this, so I'm not holding approval on them.