Cut the comments outside the review-required paths - #131
Merged
Conversation
Most of what went was narration: a header restating what the file below it does, or a paragraph on why a design was picked over one nobody proposed. 4569 comment lines down to 3089, across src/ minus auth, middleware, db and storage, which get their own PR. What stayed is the part that stops somebody making a change that breaks something. The three SVG conditions in uploads.ts and svgSanitize.ts, which say what has to remain true for an inline SVG to be safe. The memberStateHash note, since a field added to buildMemberList and not to it reaches nobody with nothing erroring. addressIsOwn, where a wrong answer is a LAN join that admits whoever can reach the proxy. The invite role rules being applied twice. The metrics port not being publishable. checkSessionAllowed being called before grytUserId is assigned. Tests and scripts are left alone. A comment in a test saying why a case exists is close to a test name, and 400-odd more lines would make this diff harder to read for no gain. 694 tests pass. The emitted JavaScript is unchanged: I built HEAD and this branch with tsc --removeComments and the output directories are identical. 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
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.
4569 comment lines down to 3089 across
src/, minusauth,middleware,dbandstorage. Those four get their own PR so this one stays readable.Most of what went was narration: a header restating what the file below it does, or a paragraph on why a design was picked over one nobody proposed.
constants/permissions.tswas 57% comment and is now 37%; every permission in the list carried a paragraph and most of them now carry a line.What stayed
The parts that stop somebody making a change that breaks something.
routes/uploads.tsandutils/svgSanitize.ts— what has to remain true for an inline SVG to be safe, including that the sandbox CSP is not optionalmemberStateHash, where a field added tobuildMemberListand not to it reaches nobody with nothing erroring (GRYT-65)addressIsOwn, where a wrong answer is a LAN open join that admits whoever can reach the proxycheckSessionAllowedbeing called beforegrytUserIdis assignedlowS: false's equivalent here —parseVersion's closed set of stages, where full semver reads agit describestring as a prereleaseTwo stale comments
Nothing found in this repo, unlike crypto. Everything that named a mechanism still names one that exists.
Verification
694 tests pass. The emitted JavaScript is unchanged — I built HEAD and this branch with
tsc --removeCommentsand the output directories are identical.*.test.tsis left alone. A comment in a test saying why a case exists is close to a test name, and 400-odd more lines would make this diff harder to read for no gain.What to look at
Whether anything I cut was load-bearing. The headers compressed hardest are
moderation/evict.ts,socket/utils/clientNotices.tsandroutes/uploadStorage.ts— all three are security reasoning, and all three kept their conclusion and lost the history that led to it. If any of those reads thinner than it should, say so and I will put the sentence back rather than the paragraph.Part of the comment cleanup tracked under GRYT-899, which stays open. The branch and title are free of the id on purpose:
vikunja-task-done.ymlreads the branch name and the title, so naming it in either closes the task on merge — which it has done three times already.🤖 Generated with Claude Code