cleanup(legibility): rewrite vague comments; remove "ask Marcel" markers #413
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Part of Epic #411 — Cleanup. This is CLEANUP-2: scrub the codebase of comments that signal AI-generation or unresolved questions, and convert remaining what-comments into why-comments where they're load-bearing.
Per the Legibility Rubric, this addresses C7.2, C7.3 (Major).
Search patterns to find offenders
Run these greps against the post-refactor codebase:
Method
For each offender:
// TODO(legibility, #N): clarifyreferencing it// TODO(#N): <one-line>CODESTYLE.mdif present).Specific files known to contain markers (from requirements phase)
CLAUDE.mdfiles are full of LLM-targeted instructions — those are appropriate AS LLM-only and don't need scrubbing if they aren't surfaced to humans// TODOintroduced during AI generation that was never resolvedAcceptance criteria
Dependency
Soft dependency on Epic 4 refactor (some markers may move with files; easier to scrub once stable).
Definition of Done
PR merged; closing comment includes the final grep output (all zero) for the four critical patterns.
👨💻 Felix Brandt — Senior Fullstack Developer
Observations
.venv,CLAUDE.md, and.claude/). All four return zero matches today — no"ask Marcel","Claude generated","Claude wrote", or"non-obvious"in production code. The codebase is clean on the four critical patterns right now.frontend/src/routes/admin/+layout.server.ts:29—TODO: replace with a dedicated /api/admin/stats endpoint that returns counts only— a well-reasoned architectural TODO (explains the why, not the what), but it doesn't reference a Gitea issue number.frontend/src/lib/components/chronik/ChronikRow.svelte:163— a multi-line TODO referencing a missing backend field onActivityFeedItemDTO, plus a SECURITY reminder about not using{@html}. This is the most interesting one: it names Leonie and Nora by name in the comment, references PR #288 and issue #285 — it's almost correctly formatted but lacks the// TODO(#N): one-lineformat the issue prescribes.CODESTYLE.mdalready documents the comment standard clearly (why-only, no what-comments). This issue is really about enforcement, not policy.Recommendations
commentPreviewonActivityFeedItemDTO; if not, open one; then replace the block comment with// TODO(#N): add commentPreview to ActivityFeedItemDTO (then switch to {text}, never {@html})..venvin the acceptance-criteria greps — it generates hundreds of false positives from pip/pytest internals. Scope the grep tofrontend/src,backend/src, andocr-service/*.pyto keep the signal clean.non-obviouswill match CODESTYLE.md itself (the word appears in examples). Either add--exclude="CODESTYLE.md"or accept that one hit as a known false positive.Open Decisions
// TODO(#N):, decide whether to keep the SECURITY line as a separate comment or fold it into the tracking issue. Keeping it in the code is actually good (Nora's pattern of threat-model comments) — don't delete it just because it's a cleanup pass.🏗️ Markus Keller — Application Architect
Observations
.claude/) and human-readable source code. That boundary is the right call architecturally — instructions for the LLM are a different audience than future developers.TODO: replace with a dedicated /api/admin/stats endpointcomment in+layout.server.ts:29is not just a comment smell — it flags a real architectural concern: the admin layout currently loads full entity lists (/api/users,/api/groups,/api/tags) solely to display counts on a dashboard. This is a layering violation: the frontend is fetching data it doesn't render, and it's creating unnecessary load on the backend. This TODO describes a genuine architectural improvement, not a vague placeholder.ChronikRowTODO references a missing backend DTO field (commentPreviewonActivityFeedItemDTO). That is a feature gap, not a comment smell. The comment correctly documents why the placeholder exists — it is an exemplary why-comment, not an AI-smell.Recommendations
Enhancementticket that belongs in the backlog. The cleanup task then becomes: replace the comment with// TODO(#<new-issue>): dedicated stats endpoint to avoid full-list fetch.frontend/src,backend/src, andocr-service/*.pyonly. Running against the full repo will hit.venv(hundreds of false positives),CLAUDE.mdfiles (intentional LLM-targeted text), anddocs/(issue descriptions referencing those exact phrases).Open Decisions
🔒 Nora "NullX" Steiner — Application Security Engineer
Observations
ChronikRow.sveltecomment at line 163 contains an active, well-placed security reminder:SECURITY: once item.commentPreview lands, render via {text}, never {@html}. The backend must truncate and strip tags server-side.This is exactly the kind of threat-model comment I advocate for. It tells the next developer exactly what attack vector exists (XSS via raw HTML rendering) and what the mitigation must be.Recommendations
// SECURITY: render via {text} not {@html} when commentPreview arrives — XSS risk (#285). Do not fold it into the tracking issue only — it must stay visible in the code where the future developer will be working.commentPreview, so that when the field is implemented, the developer is reminded to look for this comment and address it.frontend/src,backend/src,ocr-service(excluding.venv) that fails the build on any match would prevent drift. Onegrep -rn "ask Marcel" ...per pipeline run is negligible overhead.Open Decisions
commentPreviewis implemented, then delete it alongside the feature PR — this keeps the security constraint visible to whoever touches that code. (B) put it only in the tracking issue and remove the code comment now. Option A is clearly safer: the security note is co-located with the risk, and the developer implementingcommentPreviewcannot miss it. I recommend A, but worth confirming intent.🧪 Sara Holt — QA Engineer & Test Strategist
Observations
CLAUDE.mdfiles from the four critical grep checks. This distinction is important for avoiding false positives in the grep gate."ask Marcel"or similar.Recommendations
.from the repo root will hit.venv(50+ false positives from pip internals) and.claude/(intentional LLM instructions). Scope explicitly to source directories.non-obviousgrep excludesCODESTYLE.md— the word appears twice in that file as a usage example, generating false positives. Either add--exclude="CODESTYLE.md"to that specific grep or accept it as a known false positive and document it in the closing comment.No open decisions from the QA angle — the scope is clear and the verification criteria are testable.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Observations
ChronikRow.sveltecomment that this issue must address. The comment references "Leonie, PR #288 review" as the rationale for the placeholder rendering. I want to note: that context is valuable and should be preserved in some form. The comment explains a deliberate UI decision — rendering an ellipsis instead of the document title — that was made to prevent confusing UI feedback.Recommendations
commentPreviewknows the intent behind the placeholder behavior.commentPreviewonActivityFeedItemDTO.No open decisions from a UX perspective. The cleanup scope is correctly bounded to comment quality, not behavior.
🖥️ Tobias Wendt — DevOps & Platform Engineer
Observations
.): the.venvdirectory underocr-service/contains pip and pytest internals with dozens ofFIXME,XXX, andHACKmatches. These are third-party vendored dependencies, not project code.--exclude="CLAUDE.md"flags or scoped directory targets.Recommendations
.gitea/workflows/) as a new step or job, scoped tofrontend/src backend/src ocr-service/*.py. This turns the one-time cleanup into a permanent guardrail:.venvdirectory is gitignored but present locally. Running unscoped greps will produce false positives that obscure real matches and waste developer time..venvshould be gitignored and excluded from all tooling. Verify it is in.gitignore— if it isn't, that's a separate cleanup item.No open decisions. The CI gate recommendation is straightforward to implement as part of this PR.
📋 Elicit — Requirements Engineer
Observations
Recommendations
TODO(#N):format referencing a Gitea issue, or (b) was resolved in this PR. Anything else is unjustified. This makes the acceptance criterion testable without judgment calls.// TODO(#N): one-line summary."No open decisions — the scope is clear once "justified" is given a testable definition.
🗳️ Decision Queue — Action Required
1 decision needs your input before implementation starts.
Security
Fate of the SECURITY comment in ChronikRow.svelte — The comment at line 163 currently contains both a TODO (
commentPreviewfield is missing onActivityFeedItemDTO) and a security reminder (SECURITY: render via {text} not {@html} when commentPreview arrives — XSS risk). When the TODO is converted to// TODO(#N): one-line, what happens to the security line?Option A (recommended by Nora): Keep the SECURITY line as a separate, standalone comment in the code until the
commentPreviewfeature is implemented. Delete it in the same PR that adds the field. This keeps the XSS reminder co-located with the code that will eventually be changed.Option B: Remove the in-code SECURITY comment now, and capture the security constraint only in the tracking issue for
commentPreview. Cleaner code, but the constraint is one click away rather than inline.Option A is the lower-risk choice — the security note is co-located with the risk and cannot be missed by whoever implements
commentPreview. (Raised by: Felix, Nora)CLEANUP-2 — Closing summary
All critical grep patterns return zero matches in production source code:
TODOs converted to issue-referenced stubs:
frontend/src/routes/admin/+layout.server.ts:29// TODO(#453): replace with dedicated /api/admin/stats endpoint returning counts onlyfrontend/src/lib/activity/ChronikRow.svelte:163// TODO(#454): add commentPreview to ActivityFeedItemDTO, then render here+// SECURITY: render via {text} not {@html} when commentPreview arrives — XSS risk (#285)(kept as standalone)Decision on SECURITY comment (Option A): Kept as a standalone comment co-located with the risk. The XSS reminder must remain visible to whoever implements
commentPreview— putting it only in the tracking issue would hide it at implementation time.Implemented in commit
0fa90d58on branchfeat/issue-411-legibility-cleanup.