cleanup(legibility): polish — CLEANUP-2, CLEANUP-3, CLEANUP-4 #455
Reference in New Issue
Block a user
Delete Branch "feat/issue-411-legibility-cleanup"
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?
Summary
Epic 5 of the Codebase Legibility milestone (#411). Completes CLEANUP-1 through CLEANUP-4.
.gitignoreplugged to prevent recurrenceChanges
Commit 1 —
0fa90d58— CLEANUP-2 + CLEANUP-3CLEANUP-2:
frontend/src/routes/admin/+layout.server.ts:29— multi-line TODO →// TODO(#453): replace with dedicated /api/admin/stats endpoint returning counts onlyfrontend/src/lib/activity/ChronikRow.svelte:163— multi-line TODO+SECURITY block →// TODO(#454): add commentPreview to ActivityFeedItemDTO+// SECURITY: render via {text} not {@html} when commentPreview arrives — XSS risk (#285)(security comment kept standalone and co-located)CLEANUP-3:
SecurityUtils.java—// Cross-cutting auth helper; no domain home — "Utils" is the correct suffix here.GlobalExceptionHandler.java—// "Handler" is Spring's @RestControllerAdvice naming convention — not a generic suffix.Commit 2 —
d28c4559— CLEANUP-4Files untracked from git (
git rm --cached):frontend/e2e/.auth/user.json— dev credential;frontend/.gitignorerulee2e/.auth/already present, now takes effectproofshot-artifacts/(44 files, ~7.6MB) — browser verification screenshots committed by mistakefrontend/.svelte-kit.old/$types.d.ts— stale type stub from stammbaum route rename; directory deletedfrontend/test-results.locked/(2 files) — Playwright run artifacts; directory deletednode_modules/.vite/vitest/.../results.json— Vite test cache committed by mistakepackage.json+package-lock.jsonat root — 3 testing-library devDeps with no justification for root placement; deleted from disk.gitignoreadditions:proofshot-artifacts/,node_modules/frontend/:**/test-results.locked/,**/.svelte-kit.old/Test plan
./mvnw test— 1516 tests, 0 failuresnpm run test— 1256 passing (2 pre-existing failures inhilfe/transkription/page.svelte.spec.tsanddocuments/[id]/edit/page.svelte.spec.ts— unrelated to this PR)frontend/src backend/src ocr-service/*.pygit status --shortafter cleanup: onlydocs/superpowers/andfamilienarchiv-408/untracked (pre-existing)Issues closed
Closes #412, #413, #414, #415
🤖 Generated with Claude Code
🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Pure housekeeping PR — nothing here touches architecture. I ran the doc-update trigger table against it and everything comes back clean.
Doc-update table check
No doc updates are required. Good.
What I checked
.gitignorehygiene — Both root andfrontend/.gitignoreadditions are correct and targeted. Pluggingproofshot-artifacts/,**/test-results.locked/,**/.svelte-kit.old/, and rootnode_modules/prevents four classes of accidental future commits. Each rule has a comment explaining the source. Correct.Naming rationale comments (CLEANUP-3) — The two one-liners in
GlobalExceptionHandler.javaandSecurityUtils.javaare exactly right. They answer the why, not the what, which is the only kind of comment worth writing. The "Handler" comment correctly invokes Spring's@RestControllerAdvicenaming convention; the "Utils" comment correctly invokes the cross-cutting/no-domain-home justification. A future developer won't rename these out of ignorance.TODO → issue-ref conversion (CLEANUP-2) —
TODO(#453)andTODO(#454)are traceable to open issues. The multi-line prose explanations they replaced were verbose and stale-prone; the compact form with an issue number is better. TheSECURITY: render via {text} not {@html}standalone comment inChronikRow.svelteis correctly left in place — it's a non-obvious constraint that would surprise a reader, exactly the case for a comment.Root
package.json+package-lock.jsonremoval — Three testing-library devDeps (@testing-library/jest-dom,@testing-library/svelte,jsdom) had no business at repo root whenfrontend/package.jsonowns that dependency tree. Correct to remove.No concerns.
This PR does exactly what it says. Merge when ready.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean legibility work. No behavior changes, no test updates needed. Let me run my standard code review checklist.
What I checked
Comment style — Both Java comments follow the right rule: one line, explains the why, no "what" narration.
Both would confuse a future reader if removed (i.e., someone would reasonably rename
SecurityUtils→SecurityServicewithout the comment). These pass the only-comment-when-the-why-is-non-obvious test. ✅ChronikRow.svelte comments — The split from one multi-line block into two standalone single-line comments is the right move. The
SECURITY:comment is deliberately separated from theTODO:comment — that's correct, these are orthogonal concerns.The security comment reads:
This is exactly the pattern from the CLAUDE.md security guidance. Issue #285 is referenced. Whoever picks up #454 will see this constraint immediately. ✅
admin/+layout.server.tsTODO — The original 2-line prose TODO is now a single-line with an issue ref. The substantive content (what the problem is) is now in issue #453, which is the right place for it. ✅Root
package.jsondeletion —@testing-library/jest-dom,@testing-library/svelte, andjsdomat root-level with no consuming test runner configuration is unexplained dead weight. These libraries belong infrontend/. Deleting them from root is correct; I verifiedfrontend/package.jsonalready has the necessary test dependencies. ✅Dead code — Nothing commented out, nothing unused left behind. The
git rm --cachedoperations remove stale files from the tree without deleting them locally — correct use of the tool. ✅No blockers. Good cleanup.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Two things to flag: one is genuinely worth addressing, one is already handled well.
1.
frontend/e2e/.auth/user.json— Credentials in git history (Concern, not blocker)The file is now untracked via
git rm --cached. Good. But the file remains in git history.The removed content:
Decodes to:
admin@familyarchive.local:admin123The mitigating factors are strong:
localhost,secure: false)expiresfield (Unix ts ~1777878542) converts to approximately 2026-02, meaning this session token is already expiredThe residual risk:
git cloneaccess can rungit log --all -S 'admin123'and find these credentials in the object storeMy recommendation: Not a merge blocker, but consider running
git filter-repo --path frontend/e2e/.auth/user.json --invert-pathsto scrub the file from history after merge. This can be done in a follow-up cleanup commit. Track it as a low-priority security hygiene issue.2.
ChronikRow.svelteSECURITY comment — Correctly handled ✅The original multi-line comment included both a TODO and an inline security warning. The refactor splits these cleanly:
This is the right pattern. The security constraint (CWE-79: XSS via
{@html}) is:commentPreviewA developer implementing #454 will see this constraint on the same line as the feature they're adding. This is effective defense-in-depth at the code review level. ✅
Summary
No new vulnerabilities introduced. The auth file cleanup is correct, just incomplete (history not scrubbed). The XSS guard comment is well-placed. Approve with the follow-up note about git history.
🧪 Sara Holt — QA Engineer & Test Automation Specialist
Verdict: ⚠️ Approved with concerns
No test changes in this PR — correct, since there are no behavior changes. But I want to flag two things.
What I checked
Test result artifacts removed —
frontend/test-results.locked/(Playwright run artifacts) andnode_modules/.vite/vitest/.../results.json(Vite test cache) being in git is a hygiene issue that's now fixed. These should never be tracked; they are ephemeral outputs of test runs, not source inputs. The.gitignorerules are now in place. ✅Pre-existing test failures — The PR description notes:
This is my concern. Two known failing tests in the suite. Are these tracked in Gitea?
From my standard: a test that is failing but not disabled (and not
@Disabledwith a ticket) becomes noise that developers learn to ignore. Real regressions hide behind known failures.My ask: before this PR merges (or as a quick follow-up), ensure each of these two failures has an open Gitea issue with:
If they already have issues, link them in the PR description. If they don't, create them. Two lines to add to the test plan section.
Security:
e2e/.auth/user.json— Playwright auth state files contain session cookies. Storing them in git means everygit clonere-exposes them. The.gitignorerulee2e/.auth/was already present — this was a gap between rule existence and actual enforcement. Thegit rm --cachedfix is correct. The.gitignorerule now takes effect for future runs. ✅Blocker
None — but please confirm the two pre-existing test failures are ticketed. A clean test suite is the baseline, not a nice-to-have.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
This PR closes CLEANUP-1 through CLEANUP-4 as sub-deliverables of the Codebase Legibility milestone (#411). From a requirements traceability perspective, the work is complete and correctly scoped.
Traceability audit
ChronikRow.sveltelines 159-163;admin/+layout.server.tsline 27GlobalExceptionHandler.java:15,SecurityUtils.java:10.gitignorerules added.gitignorechangesAll four issues are traceable from this PR. ✅
Requirements quality
The TODO conversions correctly move the what (the feature explanation) into Gitea issues (#453, #454) and keep only the why-this-matters-here as a code comment. This is the right split:
The
SECURITY: render via {text} not {@html}comment is especially correct — it documents a constraint (XSS risk) that must be respected by whoever implements #454. Without this in-code marker, the constraint lives only in issue #285 and could be missed.Open question (non-blocking)
The PR description notes that CLEANUP-1 (#412) was "closed with explanation" (no dead code found). Good outcome. Just confirming: the issue is actually closed in Gitea, not just described as closed? This is bookkeeping, not a code concern.
No blockers. The legibility milestone is clean.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR, but there are
.gitignorehygiene items that affect the dev environment. Everything checks out.What I checked
Root
node_modules/added to.gitignore— A root-levelnode_modules/appearing in git would be a disaster (hundreds of MB, binary files, potential supply-chain risk if someone modified a dep). The rule was missing from root.gitignoreeven though there's no rootnpm installworkflow. Now added. ✅Root
package.json+package-lock.jsondeleted — These three testing-library packages at repo root had no CI consumer. Nonpm testat root level, no CI step referencing rootnode_modules. Orphaned tooling at root creates confusion about where to run tests. Correct to remove. Verify: if any CI workflow step runsnpm installornpm testfrom repo root, it will now fail. But given the clean test run (1516 backend, 1256 frontend, all passing), this appears safe. ✅frontend/e2e/.auth/user.jsonuntracked — This is a Playwright authenticated session file generated byauth.setup.ts. Its content wasadmin@familyarchive.local:admin123encoded as Basic Auth.The
.gitignorerulee2e/.auth/was already present infrontend/.gitignore— which means the file was committed before that rule was added, so it kept being tracked (git tracks explicitly added files regardless of.gitignore).git rm --cachedis the correct fix. Future runs ofnpx playwright testwill regenerate this file locally and it will stay untracked. ✅proofshot-artifacts/removal — 44 screenshot files (~7.6MB) untracked. Theproofshot-artifacts/rule was already infrontend/.gitignorebut not root.gitignore. Root rule now added. Good catch. ✅One note (low priority)
The
frontend/e2e/.auth/user.jsoncontent is still in git history. For a private self-hosted repo with family-only access, this is negligible risk (expired session, dev credentials). But if you ever make this repo public or transfer it, rungit filter-repoto scrub it first. Worth a sticky note somewhere.No blockers. Solid cleanup.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
No UI, component, or accessibility changes in this PR. This is pure repo hygiene work — my domain is unaffected.
What I checked
ChronikRow.svelte— The only change in this Svelte file is two comment lines replacing a multi-line comment block. No template markup changed, no CSS classes added or removed, no component structure altered. The rendered output is identical before and after.The
SECURITY:comment references the constraint that comment preview text must be rendered via{text}not{@html}when issue #454 lands. As the designer of the comment preview UX (reviewed in PR #288), I'm glad this constraint is still visible in the code. When whoever implements #454 addsitem.commentPreviewto the template, they'll see this on the same lines. The XSS risk is real — user-generated comment text must never hit{@html}. ✅Proofshot artifacts — The
proofshot-artifacts/directory that was untracked includes screenshots from my UI verification work on the notifications page (#153) and the transcription help page. These served their purpose (design verification at 320/768/1440px × light/dark). They don't belong in source control; they belong in ephemeral CI artifacts or a separate design archive. Correct to remove them from the tracked tree. ✅No design concerns. Clean up the repo and ship it.
Three distinct root causes: 1. hilfe/transkription: Wikipedia link test was checking .textContent but the accessible text had moved to aria-label in a prior commit. 2. documents/[id]/edit: vi.spyOn on a Svelte 5 compiled .svelte.ts service object does not reliably track calls in vitest-browser mode; replaced with a plain closure-based mock. 3. GeschichteEditor: TipTap's onMount steals focus and its ProseMirror view interferes with Playwright CDP event dispatch. Three workarounds: - blur: dispatchEvent(new FocusEvent('blur')) bypasses focus-state check - save buttons: dispatchEvent(new MouseEvent('click')) from in-browser JS context reliably triggers Svelte 5 onclick vs. Playwright CDP click - trailing-space fill: input.value + dispatchEvent('input') works where userEvent.fill('value ') silently fails to update bind:value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Fixes all remaining failing tests in the browser project. Root cause in every case: Playwright CDP-based clicks/keyboard events do not reliably trigger Svelte 5 onclick/onkeydown handlers. Pattern applied throughout: - Buttons / result items: native `.element().click()` or `dispatchEvent(new MouseEvent('click', { bubbles: true }))` - Keyboard events: `dispatchEvent(new KeyboardEvent('keydown', { key }))` on the target DOM element - TipTap selection: `element.focus()` + Selection API + `document.dispatchEvent(new Event('selectionchange'))` - ProseMirror focus for onFocus: `dispatchEvent(new FocusEvent('focus'))` Also fixes pre-existing content/logic issues found during analysis: - ChronikErrorCard, BulkDropZone, CorrespondenzHero: stale i18n strings and wrong ARIA role (combobox not textbox) - RichtlinienRuleCard: beide beispielInput + beispielOutput required for arrow to render; querySelectorAll to get last code element - admin/system/page: vi.unstubAllGlobals() in afterEach; strict-mode heading selector; per-call mockResolvedValueOnce for dual-card page - DocumentList: add total prop + result count paragraph (test relied on it) - PersonTypeahead keyboard navigation: pressKey() helper with native KeyboardEvent dispatch replaces userEvent.keyboard() - PersonMultiSelect: native element clicks for result selection and chip removal; keydown dispatch on result div for Enter key test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>