docs: write comments for the next reader, not the reviewer - #347
Merged
Merged
Conversation
The comments added over the last few PRs had drifted into arguing decisions — text that belongs in a commit message, read once, rather than in a file read every time. `cli-io.ts` was the clearest case: six lines of doc comment on a two-method interface, ending in a sentence explaining why the refactor was worth doing, which the commit already said. Applied one rule throughout: a comment earns its place only when it says something the code cannot — a constraint, a rejected alternative and why, a non-local dependency. Test names state the behaviour, not the reasoning. 87 lines net removed. Comment ratio: cli-io.ts 50% -> 11%, docs/cli.ts 16% -> 4%, docs-embed.mjs 35% -> 15%. Bundled docs 2251 -> 2072 words with no topic dropped. No behaviour change — every check still passes. Recorded in AGENTS.md so it holds for future sessions too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe PR updates CLI documentation, generated documentation, source comments, and test descriptions. It clarifies documented behavior and adds checks for runnable documentation commands. Runtime logic and public declarations remain unchanged. ChangesCLI documentation and comment cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
This was referenced Aug 2, 2026
oekazuma
added a commit
that referenced
this pull request
Aug 2, 2026
The candidate list appended extensions unconditionally, so an import written `from '$lib/server/store.js'` — how a NodeNext/ESM TypeScript project spells an import of its own `.ts` source — produced `store.js.ts` and `store.js.js`, matched nothing, and left the write unarbitrated. Verified as a real miss against a fixture before fixing. A path already carrying an extension is now checked as written, and a `.js` one is then remapped to `.ts`. Extensionless paths are unchanged. A client imported the same way still resolves and stays exempt. Also from review: a test was named after why it exists rather than what it verifies, which is the AGENTS.md rule added in #347, and the changeset had an ungrammatical sentence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oekazuma
added a commit
that referenced
this pull request
Aug 2, 2026
* fix(core): report a hand-rolled in-memory store under $lib/server Closes #354. `security/handler-state-write` exempts `.set()`/`.update()` on imports resolving under the `$lib` server root — that is where database and KV clients live, and `db.set(…)` on one is persistence, not shared state. The check was purely path-based, so a plain `new Map()` in the same directory was exempt too, which is exactly the shape that serves one user's data to the next. The call shape cannot separate the two, and the pure parse cannot read another file, so arbitration moves to the collector, which has the Runtime: `parseKitModuleFacts` records the deferred write with the resolved path and the exported name, and `collectKitModuleFacts` reads the target module and promotes only the writes whose export is an in-memory container. Precision-first, like the rest of this default-on rule. An export initialized to `new Map`/`Set`/`WeakMap`/`WeakSet` or to an object/array literal is reported; anything else — a client built from a package import, a re-export, a module that cannot be found or read — stays exempt, so what the read cannot positively identify is silence rather than a false positive. Only the modules a handler actually writes to are read, asserted by a test, so a project whose handlers never touch `$lib/server` does no extra I/O. Aliased imports resolve through the exported name. Property writes were already reported everywhere and are untouched. Both the CLI and the Vite plugin go through the same collector, so both gain this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): resolve a NodeNext `.js` specifier to its TypeScript source The candidate list appended extensions unconditionally, so an import written `from '$lib/server/store.js'` — how a NodeNext/ESM TypeScript project spells an import of its own `.ts` source — produced `store.js.ts` and `store.js.js`, matched nothing, and left the write unarbitrated. Verified as a real miss against a fixture before fixing. A path already carrying an extension is now checked as written, and a `.js` one is then remapped to `.ts`. Extensionless paths are unchanged. A client imported the same way still resolves and stays exempt. Also from review: a test was named after why it exists rather than what it verifies, which is the AGENTS.md rule added in #347, and the changeset had an ungrammatical sentence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Comments added over the last few PRs had drifted into arguing decisions — text aimed at whoever was reviewing at the time, rather than at whoever opens the file next.
packages/cli/src/cli-io.tswas the clearest specimen: six lines of doc comment on a two-method interface, ending withwhich explains why the refactor was worth doing — and which the commit message already said. Every future reader pays for that sentence; the commit is read once.
The rule applied
A comment earns its place only when it says something the code cannot: a constraint, a rejected alternative and why, or a non-local dependency. Rationale for a change goes in the commit message and the PR. Test names state the behaviour, not the reasoning.
Before / after on the same interface:
Test names got the same treatment —
it('says the topics ship with the CLI, which is the reason to prefer them over a web search')becameit('says the topics match the running version').Result
src/cli-io.tscomment ratiosrc/docs/cli.tscomment ratioscripts/docs-embed.mjscomment ratiopackages/cli/docs/*.md)87 lines net removed across 15 files. No topic dropped from the bundled docs, no assertion dropped from a test, no behaviour change.
Comments that survived are the ones carrying real information — why the staleness test compares content rather than rendered text (oxfmt reformats the generated module), why
string-mapcannot say "never replaces" (it is a spread, unlikestring-list), why the frontmatter reader rejects quotes (the sibling reader inscripts/rules-index.mjsunquotes, and the two must not disagree).Also
Recorded the rule in
AGENTS.mdunder Conventions, so it applies to future sessions rather than living only in this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests
svelte-vitals explaininstead of the removed MCP tool.