fix: correct the comparison relation sentence and remove NUL bytes from source - #5
Conversation
The accessible summary read:
99e7880 is ahead of 49b7926: 2 commits ahead, 0 behind, 12 files
changed, 714 lines added and 170 removed.
which is backwards. In `base...head` semantics `aheadBy` is how far **head**
is ahead of base, so the sentence named the wrong subject: 49b7926 is the
commit that is two ahead, not 99e7880.
The template was also ungrammatical for two of the four relations, because it
dropped the relation word into a fixed "X is <word> of Y" frame:
49b7926 is identical of 49b7926: ...
49b7926 is behind of 99e7880: ...
Each relation now gets its own complete sentence, so the grammar cannot change
meaning between statuses:
49b7926 is 2 commits ahead of 99e7880. 12 files changed, 714 lines added
and 170 removed.
99e7880 is 2 commits behind 49b7926. 12 files changed, 170 lines added
and 714 removed.
49b7926 and 49b7926 identify the same commit. No file differs between them.
bbb2222 is 3 commits ahead of and 5 commits behind aaa1111. 20 files
changed, 300 lines added and 200 removed.
Sighted readers get the direction from the arrow between the two cards; that
arrow was the only thing saying which way the numbers ran.
The wording is a pure function in `src/lib/compare/describe.ts`, tested against
real comparisons of a known history so the endpoint order is checked too, not
just the presence of the words "ahead" and "behind".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ComparePicker.tsx used a raw NUL byte as a sentinel for "nothing selected", as
the fallback of `selected ?? ...` inside a startsWith call.
It behaved correctly -- no sha starts with a NUL -- but it put two raw NUL bytes
into a TSX file, so `file` reported it as `data` and Git classified it as binary
and stopped producing readable diffs for it. A change nobody can read in a diff
is a change nobody can review.
The intent is now stated outright:
const isSelected = (sha: string): boolean =>
selected !== null && sha.startsWith(selected);
A source-integrity test was added to catch this class of mistake, and it
immediately found two more files carrying raw NUL bytes from earlier work:
- e2e/api.spec.ts used one as hostile test input for path validation;
- src/lib/stats/growth.ts used three as a composite-key separator.
Both intents are sound, so both keep their NUL -- written as a unicode escape
sequence rather than embedded as a byte. The runtime strings are identical, so
behaviour is unchanged; the files are now text.
All three now report as text to `file` and to `git grep -I`, and the guard fails
the build if any tracked source file gains a NUL byte, fails to decode as UTF-8,
or becomes binary to Git.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The replacement-character check contained a literal U+FFFD, so the guard reported its own file. It is written as an escape now, which is the same correction it exists to enforce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28d3eb2d4
ℹ️ 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".
| return output | ||
| .split('\0') | ||
| .filter(Boolean) | ||
| .filter((file) => /\.(ts|tsx|js|mjs|cjs|json|css|md|yml|yaml|svg|txt)$/.test(file)) |
There was a problem hiding this comment.
Include tracked .mts sources in the integrity guard
The extension allowlist omits .mts, so the tracked TypeScript source vitest.config.mts is never checked. If that configuration file acquires a raw NUL or invalid UTF-8, this new guard still passes even though Git may classify the file as binary—the exact regression the test is intended to prevent. Include .mts in the filter.
Useful? React with 👍 / 👎.
Two correctness fixes to the comparison feature. No UI redesign, no summaries.
Problem 1: the accessible sentence named the wrong subject
The visually hidden summary dropped the relation word into a fixed
X is <word> of Yframe. That frame was backwards, and ungrammatical for two ofthe four relations.
Before
In
base...headsemanticsaheadByis how far head is ahead of base. Thefirst line therefore named the wrong commit:
49b7926is the one that is twocommits ahead, not
99e7880. The trailing clause repeated the same numbers withthe same ambiguity, and
is behind of/is identical of/is diverged ofarenot English.
After
Each relation is now a complete sentence of its own, so the grammar cannot change
meaning between statuses. Singular and plural are handled (
1 commit ahead), andthousands are separated to match the numbers on screen.
Sighted readers get the direction from the arrow between the two endpoint cards.
That arrow was the only thing saying which way the numbers ran, which is exactly
why the sentence has to say it in words.
The wording is a pure function in
src/lib/compare/describe.ts.Tests
Written against real comparisons of a known history, so the endpoint order is
checked rather than assumed:
the presence of the words;
ahead, with the exact sentence;behindafter swapping the ends, with the exact sentence, and asserting theold
behind ofshape is gone;identical, including the changes clause;diverged, with both distances;E2E reads both shas off the endpoint cards and asserts the sentence names them in
the right order, before and after pressing swap.
Problem 2: raw NUL bytes made source files binary
ComparePicker.tsxused a raw NUL as a sentinel for "nothing selected", as thefallback of
selected ?? ...inside astartsWithcall.It behaved correctly, since no sha starts with a NUL. But it put two raw NUL
bytes into a TSX file:
Git classified the file as binary and stopped producing readable diffs for it. A
change nobody can read in a diff is a change nobody can review.
The intent is now stated outright:
After
The guard found two more
A source-integrity test was added, and it immediately found two files carrying
raw NUL bytes from earlier work that I did not know about:
e2e/api.spec.tssrc/lib/stats/growth.tsarea + bucketmap keyBoth intents are sound, so both keep their NUL, written as a unicode escape
sequence instead of embedded as a byte. The runtime strings are identical, so
behaviour is unchanged; the files are now text. All three report as text to
fileand togit grep -I.The guard fails if any tracked source file gains a NUL byte, fails to decode as
UTF-8, or becomes binary to Git. It was verified in both directions: injecting a
NUL makes it fail, removing it makes it pass. It also caught itself once, because
the replacement-character check contained a literal U+FFFD, which is the third
commit.
Picker tests
23 component tests covering the acceptance criteria and the surrounding
behaviour that had to stay intact: a null selection marking nothing, full and
abbreviated sha selection, tag selection, tags outside the loaded range not being
offered, filtering by subject, sha and tag name, Escape, outside click, toggle,
disabled, focus moving to the filter field on open, and the side labelling.
E2E keeps its 360px picker-open overflow check.
Verification
npm run typechecknpm run lintnpm testnpm run buildnpm run test:e2ePreserved
Built-in demo unchanged:
dataSource: builtin,htmlUrl: null, exactly 16commits,
rateLimit: nulland zero GitHub requests. Unknowndemo/*stillrefused locally with
404and null quota fields. Badge still reads0 GitHub requests.src/lib/github/client.tsuntouched, so the server-onlytoken architecture is byte-identical. Caching and rate-limit behaviour unchanged.
No generated summaries anywhere.
No old PR description was modified, no history rewritten, no force-push.
Generated with Claude Code