feat: Remove the Briefwechsel view; retarget its links to document search (#716) #721
Reference in New Issue
Block a user
Delete Branch "feat/issue-716-remove-briefwechsel"
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?
Closes #716.
Removes the standalone Briefwechsel view (
/briefwechsel) entirely — frontend and backend — and retargets its one inbound link (the "Häufige Korrespondenten" card) into the existing document search. Grep-driven removal per the spec; the "no dangling references" AC holds.Behaviour change (deliberate, see ADR-030)
The card link now deep-links to
/documents?senderId=A&receiverId=B, which filters sender AND receiver → shows only the A→B direction. The old bidirectional view's reverse direction (replies) is intentionally dropped; a bidirectional search filter is a separate future enhancement. The ×N badge stays bilateral (shared-letters signal) with a clarifyingtitle.Commits (11, atomic)
feat(persons)retarget card → document search (newpersonNameprop, both-names search title, magnifier icon, badge title, newperson_correspondents_search_*i18n keys, test updated)feat(briefwechsel)delete the route in full + e2e specs/fixture + the stalekorrespondenz.spec.ts(grep-caught, targeted the route's former/korrespondenzpath)testactivebriefwechsel-removed.spec.ts(404 guard) + cleanedstammbaum.spec.ts/auth.spec.tsfeat(i18n)remove 22 orphaned keys/locale (keptconv_sort_newest/oldest)refactor(document)moveConversationThumbnail→lib/document/, delete emptylib/conversation/feat(document)remove endpoint, service methods (+ dead 2-arggetConversation), repo queries, with testschore(api)trim the generated TS clientdocsroute tables,ARCHITECTURE.md, C4 diagrams,GLOSSARY.md, superseded design specsdocs(adr)ADR-030 records the bilateral→unidirectional regressionVerification
clean package→ BUILD SUCCESS;DocumentServiceTest(169) +DocumentRepositoryTest(29) green.frontend/src,frontend/e2e,backend/srcclean — only intentional matches: the new 404 guard and a comment in the immutable Flyway migrationV62__index_fk_columns.sql(its FK indexes still serve document search).Notes
briefwechsel-*.pngbaselines existed → that cleanup was a no-op.docs/specs/*.htmlmockups that only show Briefwechsel in static nav chrome are left as frozen point-in-time artifacts; only the two Briefwechsel-specific specs were superseded.npm run generate:apiwas not run against a live backend (needs the full stack); the generated client was surgically trimmed to exactly the removed endpoint — worth a regen on CI to confirm.🤖 Generated with Claude Code
🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
I reviewed this as a clean, grep-driven feature removal: does the dead code go away completely, do the layer boundaries stay intact, and — most important for me — do the docs match the code after the change?
What's done well
Sortin the controller,LocalDatein the repository) cleaned up. No dangling service method, no dormant repository query. This is exactly how a removal should look.hasSender/hasReceiverspecifications are correctly preserved — they still back document search (DocumentService.java:557-558). The ADR explicitly records this. Good discipline; an over-eager removal would have broken search.CLAUDE.md+frontend/CLAUDE.md),ARCHITECTURE.md(derived-domain prose), both C4 component descriptions (l3-backend-3b,l3-frontend-3c— including the in-prose endpoint mentions, not just nodes), thec4-diagrams.mdmirror, both GLOSSARY touchpoints, and superseded the two design specs. That is the full grep set from the issue.Concerns (non-blocking)
frontend/e2e/CLAUDE.md:28was missed. It still listskorrespondenz.spec.ts # Correspondence timeline— but that spec file was deleted in this PR. This is a stale documentation reference that my doc-currency rule treats as a real omission. It's a one-line fix in a doc, so I'm filing it as a concern rather than a hard blocker, but it should be cleaned up: the "no dangling references" AC is the spec's headline criterion and this is a (doc-level) dangling reference.api.tswas hand-trimmed, not regenerated. The PR notes this and asks for a CI regen to confirm. Architecturally I'd prefer the generated artifact never be edited by hand — but the surgical removal here is exactly the diff a regen would produce (one path + one operation), so the risk is contained. Confirm on CI.No accidental complexity introduced, no boundary violations, no missing DB-layer concern (tables untouched). Once the
e2e/CLAUDE.mdline is fixed, this is a clean approve from the architecture side.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
This is a clean deletion-heavy PR (5433 lines out, 137 in). I focused on the code that survives — the retargeted card — and on whether the tests were updated honestly rather than just made to pass.
What I like
CoCorrespondentsListchange is textbook props discipline. A newpersonName: stringprop threads person A's name in from the parent (+page.svelte:71passesperson.displayName), instead of reaching for a store or refetching. Specific domain-named props, not adatablob. 👍CoCorrespondentsList.svelte.test.tsupdates all three stale spots the issue called out: the href assertion (/documents?...), the hint-text assertion, and thepersonNameprop on everyrender(). The new testlabels the link as a search action naming both personsasserts thetitlecontains both names via regex — and it importsmfrom paraglide instead of retyping the German copy, which is exactly right (the copy lives in one place).person_correspondents_search_title/_hint/_badge_title) added to all three locales with the{A}/{B}placeholders matched to usage; 22 orphanedconv_*/nav_conversations/doc_conversation_title/person_correspondents_hintkeys removed — andconv_sort_newest/conv_sort_oldestcorrectly kept (still consumed byPersonDocumentList). Usages were rewritten before keys were deleted, so no key-first build break.lib/conversation/→lib/document/move is correct. Single surviving component, single consumer (ThumbnailRow.svelte:58), import updated, empty folder deleted, and thefrontend/CLAUDE.mdlib description line removed. No leftover empty package.DocumentServiceTestblocks and theDocumentRepositoryTestsection removed (grep confirms zero residual references to the removed method names). The PR reportsclean packagegreen.Minor (not blocking)
×Nbadge tooltip is atitleattribute on a<span>. Functionally fine and matches the spec's "don't recompute, just clarify" decision. No change requested.TDD-wise I can't see the red phase from the diff, but the test deltas line up precisely with the behavior changes and nothing looks reverse-engineered to pass. Solid, focused work. Approve.
🛡️ Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
A feature removal is mostly attack-surface reduction, so my job here is to confirm nothing was opened up and that the removed endpoint didn't leave a security hole behind. I checked the removed endpoint, the surviving search path, and the retargeted link.
Attack-surface assessment
GET /api/documents/conversationis gone from the controller, the OpenAPI path (api.ts), and the C4 docs. It sat behind the sameanyRequest().authenticated()rule as the rest of/api/documents, and the destination it's replaced by (/documentssearch) sits behind the identical rule, so no authorization boundary changed. No IDOR delta: both old and new paths take person UUIDs as filter params, both require an authenticated session.:person1,:from, etc.) — and the surviving search still composes parameterizedhasSender/hasReceiverspecifications. The retargeted link builds/documents?senderId={personId}&receiverId={c.id}from server-provided UUIDs interpolated into anhref; these are trusted IDs from the person's own correspondent list, not user free-text, and they flow back through the typedsenderId/receiverIdquery params that the search load function already validates. No string-concatenated query anywhere.auth.spec.tschange is safe. Dropping/briefwechselfrom the protected-route loop is correct — the route no longer exists./documents/newand/personsremain as authenticated-route guards, and/documents(the new destination) is covered by the sameauthenticated()rule, so protected-route coverage is not meaningfully reduced.title/hint strings are i18n keys mapped through Paraglide — no raw backend errors, no implementation details leaked. The icon SVG isaria-hiddenand inert.Notes (not findings)
briefwechsel-removed.spec.tsasserts a genuine 404 in the authenticated project — good: it confirms the old URL doesn't silently redirect (which could be an open-redirect smell). A 404 on the styled error page is the secure outcome.Nothing to fix. No new CWE introduced, one endpoint of attack surface retired. Approve.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
I care about one thing above all on a removal: the acceptance criteria each have a test that actually executes, and the "no dangling references" guard is real and not skipped. The issue itself flagged a trap here (a 404 guard hidden inside a
test.skip()block), so I looked closely.What's solid
test.skip()describe instammbaum.spec.tswas deleted, andbriefwechsel-removed.spec.tswas added as a standalone spec. I confirmed the Playwright config has a singlechromiumproject that runs with stored auth (storageState) — so this spec genuinely executes authenticated and the route 404s on the styled error page rather than bouncing to/login. The trap the issue warned about was avoided. 👍stammbaum.spec.tsandauth.spec.tsboth cleaned as the AC required — the contradicting "still renders without 404" assertion is gone, and/briefwechselis removed from the protected-route loop.personNameadded to everyrender()(otherwise the title formatter throws on undefined — they got that right). Assertions use the paraglide message, not retyped copy.getConversationFilteredblocks and thefindSinglePersonCorrespondencerepo section removed; zero residual references; PR reportsDocumentServiceTest(169) andDocumentRepositoryTest(29) green.Concerns
briefwechsel-removed.spec.ts404 assertion is slightly brittle.expect(page.getByText('404')).toBeVisible()—getByText('404')is a substring match and404could appear elsewhere, and it relies on the error page rendering that literal. The HTTP-status assertionexpect(response?.status()).toBe(404)is the load-bearing one and is solid; the text assertion is belt-and-suspenders but a touch fragile. ConsidergetByRole('heading', { name: '404' })or scoping it. Minor — not a merge blocker.titlewas added but there's no test assertingperson_correspondents_badge_titleis present on the badge. The card test would be the natural home for a one-linetoHaveAttribute('title', ...)check. Nice-to-have.briefwechsel-*.pngbaselines existed (no-op), and the icon swap (chat → magnifier) has no visual-regression snapshot. Acceptable given the project runs browser/visual tests on CI, but worth a CI confirmation that the icon change didn't shift any existing person-detail snapshot.The headline AC (404 guard executes for real) is correctly handled, which was the risky part. Tidy up the brittle text assertion and ideally add the badge-title test. Approve with those concerns.
🔧 Tobias Lange — DevOps Engineer
Verdict: ✅ Approved (LGTM)
I checked this for infrastructure impact: Compose services, CI workflow, reverse-proxy/Caddy config, secrets, migrations, and generated-artifact handling.
What I checked
docker-compose*.yml, no Caddyfile, no CI workflow, no env vars. (Theinfra/caddy/Caddyfile,vite.config.ts, andrenovate.jsonchanges you may see in amain...HEADdiff belong to other merged commits — they are not in this PR's diff against its base sha. Confirmed.)V62__index_fk_columns.sql(it names/briefwechselas one of several flows the FK indexes serve). Correct call to leave it: Flyway migrations are immutable once shipped, and thoseidx_documents_sender_idindexes still serve document search. No schema change, no migration risk, no restore-test impact.api.ts) was hand-edited, not regenerated. This is the one thing I'd normally grumble about — the OpenAPI client should come out ofnpm run generate:apiagainst a live backend, not a manual trim. The PR is transparent about it and the diff is exactly the one removed path + operation, so it'll reconcile on the next CI regen. Flagging as a note, not a blocker: ensure CI runsgenerate:apiand the diff comes back empty.Pure code/test/docs removal with zero ops surface. Nothing to fix from my side. Approve.
📋 "Elicit" — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
I assessed this against the issue #716 acceptance criteria (the stated single source of truth) for traceability and completeness, and probed the one deliberate behaviour change for whether it was properly surfaced rather than silently swallowed.
AC traceability — every Gherkin scenario maps to an implementation
/briefwechselreturns 404 (no redirect) on styled error pagebriefwechsel-removed.spec.ts, active project/documents?senderId=A&receiverId=BCoCorrespondentsList.sveltehref + testtitleadded, but see concern 1conv_sort_*retained/conversationendpoint gone (404)The deliberate bilateral → unidirectional regression was correctly handled the way the issue demanded: accepted, not "fixed," and recorded in ADR-030 with Context/Decision/Consequence. The "future bidirectional filter" remains explicitly out of scope. Good requirements hygiene — the regression is documented, not hidden.
Concerns
{c.name}); both names live in thetitleattribute, which is a tooltip, not the computed accessible name. The test asserts againsttitle, which matches the issue's own structural hint ("assert againsttitle"), so this is internally consistent with the spec — but a strict reading of "accessible name" vs "title" is a latent ambiguity. Acceptable as specified; flagging it so it isn't mistaken for full accessible-name coverage.frontend/e2e/CLAUDE.md:28still lists the now-deletedkorrespondenz.spec.ts. The AC's grep set isbriefwechsel/getConversation/findConversation/etc., notkorrespondenz, so this technically slips through the letter of the AC — but the intent ("only intentional matches remain") is violated by a stale doc pointer to a deleted file. Low-effort fix; worth closing for completeness.No scope creep, no contradictions, no un-surfaced TBDs. The two items above are completeness gaps against a generally well-traced PR. Approve once concern 2 is tidied.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The whole UI surface of this PR is one card — the "Häufige Korrespondenten" chip — so I went deep on it: the icon swap, the copy, the badge tooltip, the accessible name, and brand/contrast compliance.
What's right
d="M21 21l-6-6m2-5a7 7 0 11-14 0 7 7 0 0114 0z") honestly signals a search action. It keepsaria-hidden="true", so it stays inert to screen readers — correct, because the action's meaning must come from the link's text/label, not a decorative glyph. 👍<a>with visible text ({c.name}) and the existinghover:border-primary/hover:bg-surfacestates. The focus-visible ring is unchanged. Keyboard users keep a discernible target.border-line,bg-muted,text-ink,text-ink-3. No hardcoded hex, no off-palette colors introduced.Concerns
text-[10px]on the hint and the badge. Both the search hint (person_correspondents_search_hint) and the×Nbadge render attext-[10px]. My hard floor is 12px for any visible text, and this is a senior-facing (60+) audience. To be fair, this is pre-existing — the old hint was also 10px and this PR didn't introduce it — so I'm not blocking this PR on it. But since the file is being touched and the copy is being rewritten anyway, it's the natural moment to bump the hint to at leasttext-xs(12px). Flagging as a concern, not a blocker, because it's inherited.title-only, which seniors and touch users won't discover. The bilateral-count clarification ("Gemeinsame Briefe in beide Richtungen") lives entirely in atitleattribute on a<span>.titletooltips don't appear on touch, are often missed by older users, and aren't reliably announced by screen readers on a non-interactive<span>. The spec explicitly accepted "a small caption/title… is enough," so this satisfies the letter of the requirement — but from a pure UX standpoint the disambiguation is effectively invisible to a chunk of the audience. If you want the clarification to actually land, a<span class="sr-only">companion or a visible micro-caption would do it. Non-blocking given the spec's allowance.toHaveScreenshotcovering the person-detail card at 320/768/1440. Not introduced by this PR's scope, but worth a CI glance that the swap didn't shift any existing person-detail baseline.Touch target on the chip looks fine (it's a padded
px-3 py-1.5pill with text, comfortably tappable). No contrast regressions — the colors are unchanged. The two type/tooltip items are inherited or spec-sanctioned, so: approve with concerns, and please consider thetext-xsbump while you're in the file.