marcel/familienarchiv
1
0
Fork 0
Code Issues 119 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Delete dead conversations/ route (old Korrespondenz page) #193

New Issue
Closed
opened 2026-04-07 10:47:31 +02:00 by marcel · 7 comments
marcel commented 2026-04-07 10:47:31 +02:00
Owner
Copy Link

Context

The conversations/ route is leftover from before briefwechsel/ replaced it. No navigation link points to /conversations anymore — AppNav and CoCorrespondentsList both link to /briefwechsel. The only internal reference to /conversations is from within the route itself.

Scope

Delete the following files:

  • frontend/src/routes/conversations/+page.svelte
  • frontend/src/routes/conversations/+page.server.ts
  • frontend/src/routes/conversations/ConversationFilterBar.svelte
  • frontend/src/routes/conversations/ConversationTimeline.svelte

Also:

  • Audit i18n keys in messages/de.json, en.json, es.json — remove any keys only used by the conversations route
  • Verify no other file imports from conversations/

Acceptance Criteria

  • conversations/ directory fully removed
  • No orphaned i18n keys remain
  • npm run check passes
  • npm run build passes
## Context The `conversations/` route is leftover from before `briefwechsel/` replaced it. No navigation link points to `/conversations` anymore — AppNav and CoCorrespondentsList both link to `/briefwechsel`. The only internal reference to `/conversations` is from within the route itself. ## Scope Delete the following files: - `frontend/src/routes/conversations/+page.svelte` - `frontend/src/routes/conversations/+page.server.ts` - `frontend/src/routes/conversations/ConversationFilterBar.svelte` - `frontend/src/routes/conversations/ConversationTimeline.svelte` Also: - Audit i18n keys in `messages/de.json`, `en.json`, `es.json` — remove any keys only used by the conversations route - Verify no other file imports from `conversations/` ## Acceptance Criteria - [ ] `conversations/` directory fully removed - [ ] No orphaned i18n keys remain - [ ] `npm run check` passes - [ ] `npm run build` passes
marcel added the refactor label 2026-04-07 10:48:53 +02:00
marcel commented 2026-04-07 10:49:46 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Questions & Observations

  • The issue lists 4 files to delete but the conversations/ directory may also contain layout files (+layout.svelte, +layout.server.ts). Verify the full file list before deleting -- ls -R the directory first.
  • Are there any Vitest unit or integration tests that import from conversations/? These would break silently if not removed.
  • The ConversationTimeline.svelte and ConversationFilterBar.svelte in conversations/ may share naming with components in briefwechsel/. Confirm no import path accidentally resolves to the wrong directory.

Suggestions

  • Run grep -r "conversations" frontend/src/ before deleting to catch any stray references beyond what was already identified (AppNav, CoCorrespondentsList). Include test files and i18n message files in the search.
  • After deletion, run npm run check AND npm run test -- the acceptance criteria list check and build but not tests. Add npm run test to the checklist.
  • One atomic commit: delete the directory + remove orphaned i18n keys in the same commit. This is a single logical change.
## 👨‍💻 Felix Brandt -- Senior Fullstack Developer ### Questions & Observations - The issue lists 4 files to delete but the `conversations/` directory may also contain layout files (`+layout.svelte`, `+layout.server.ts`). Verify the full file list before deleting -- `ls -R` the directory first. - Are there any Vitest unit or integration tests that import from `conversations/`? These would break silently if not removed. - The `ConversationTimeline.svelte` and `ConversationFilterBar.svelte` in `conversations/` may share naming with components in `briefwechsel/`. Confirm no import path accidentally resolves to the wrong directory. ### Suggestions - Run `grep -r "conversations" frontend/src/` before deleting to catch any stray references beyond what was already identified (AppNav, CoCorrespondentsList). Include test files and i18n message files in the search. - After deletion, run `npm run check` AND `npm run test` -- the acceptance criteria list check and build but not tests. Add `npm run test` to the checklist. - One atomic commit: delete the directory + remove orphaned i18n keys in the same commit. This is a single logical change.
marcel commented 2026-04-07 10:49:53 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller -- Application Architect

Questions & Observations

  • Clean removal of dead code -- no architectural concerns here. This is the simplest possible change: delete files, verify nothing references them, done.
  • The backend /api/conversations endpoints (if any exist) are not mentioned. If the conversations route had its own server-side API calls in +page.server.ts, verify whether those backend endpoints are also unused or still serve briefwechsel/.
  • The i18n key audit is the right call. Orphaned keys in message bundles are technical debt that accumulates quietly.

Suggestions

  • Check +page.server.ts in conversations to see which API endpoints it calls. If those endpoints are shared with briefwechsel, leave them alone. If they are conversations-only, note them for potential future backend cleanup (separate issue, not this one).
  • No ADR needed for removing dead code.
## 🏗️ Markus Keller -- Application Architect ### Questions & Observations - Clean removal of dead code -- no architectural concerns here. This is the simplest possible change: delete files, verify nothing references them, done. - The backend `/api/conversations` endpoints (if any exist) are not mentioned. If the conversations route had its own server-side API calls in `+page.server.ts`, verify whether those backend endpoints are also unused or still serve `briefwechsel/`. - The i18n key audit is the right call. Orphaned keys in message bundles are technical debt that accumulates quietly. ### Suggestions - Check `+page.server.ts` in conversations to see which API endpoints it calls. If those endpoints are shared with briefwechsel, leave them alone. If they are conversations-only, note them for potential future backend cleanup (separate issue, not this one). - No ADR needed for removing dead code.
marcel commented 2026-04-07 10:49:58 +02:00
Author
Owner
Copy Link

🧪 Sara Holt -- Senior QA Engineer

Questions & Observations

  • Are there any Playwright E2E tests that navigate to /conversations? If so, those tests need to be removed or redirected to /briefwechsel in the same commit. A broken E2E test would block CI.
  • The acceptance criteria cover npm run check and npm run build but not npm run test (Vitest) or E2E tests. Both should pass.

Suggestions

  • Add to acceptance criteria:
    • npm run test passes (no broken imports)
    • E2E tests pass (no navigation to /conversations)
  • After deletion, a quick grep -r "conversations" frontend/e2e/ will catch any E2E references.
  • No new tests needed -- this is pure deletion. But verify the existing suite still passes end-to-end.
## 🧪 Sara Holt -- Senior QA Engineer ### Questions & Observations - Are there any Playwright E2E tests that navigate to `/conversations`? If so, those tests need to be removed or redirected to `/briefwechsel` in the same commit. A broken E2E test would block CI. - The acceptance criteria cover `npm run check` and `npm run build` but not `npm run test` (Vitest) or E2E tests. Both should pass. ### Suggestions - Add to acceptance criteria: - [ ] `npm run test` passes (no broken imports) - [ ] E2E tests pass (no navigation to `/conversations`) - After deletion, a quick `grep -r "conversations" frontend/e2e/` will catch any E2E references. - No new tests needed -- this is pure deletion. But verify the existing suite still passes end-to-end.
marcel commented 2026-04-07 10:50:04 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner -- Application Security Engineer

Questions & Observations

  • No security concerns from my angle. Removing dead code reduces attack surface -- one fewer route that could be accessed directly via URL, even if not linked in navigation.
  • Verify that the /conversations route did not have any unique authorization checks in its +page.server.ts that might hint at a different security model. If it was a straight copy of briefwechsel's auth, no concern.

Suggestions

  • Consider whether /conversations should get a redirect to /briefwechsel (via +page.server.ts with redirect(301, '/briefwechsel')) rather than a 404, in case any external bookmarks or shared links exist. This is a UX decision, not a security one -- but worth a 5-second discussion.
## 🔒 Nora "NullX" Steiner -- Application Security Engineer ### Questions & Observations - No security concerns from my angle. Removing dead code reduces attack surface -- one fewer route that could be accessed directly via URL, even if not linked in navigation. - Verify that the `/conversations` route did not have any unique authorization checks in its `+page.server.ts` that might hint at a different security model. If it was a straight copy of briefwechsel's auth, no concern. ### Suggestions - Consider whether `/conversations` should get a redirect to `/briefwechsel` (via `+page.server.ts` with `redirect(301, '/briefwechsel')`) rather than a 404, in case any external bookmarks or shared links exist. This is a UX decision, not a security one -- but worth a 5-second discussion.
marcel commented 2026-04-07 10:50:09 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss -- UI/UX Design Lead

Questions & Observations

  • No UI/UX concerns. The conversations route was already replaced by briefwechsel in the navigation. Users cannot reach it through normal interaction.
  • The only UX consideration is whether any users might have bookmarked /conversations directly. If this is an internal-only family app with a small user base, this is negligible.

Suggestions

  • If any family members have shared /conversations?senderId=...&receiverId=... links (e.g. in emails or chat), those links would break. A temporary redirect (as NullX suggested) would be graceful but optional for a small user group.
## 🎨 Leonie Voss -- UI/UX Design Lead ### Questions & Observations - No UI/UX concerns. The conversations route was already replaced by briefwechsel in the navigation. Users cannot reach it through normal interaction. - The only UX consideration is whether any users might have bookmarked `/conversations` directly. If this is an internal-only family app with a small user base, this is negligible. ### Suggestions - If any family members have shared `/conversations?senderId=...&receiverId=...` links (e.g. in emails or chat), those links would break. A temporary redirect (as NullX suggested) would be graceful but optional for a small user group.
marcel commented 2026-04-07 10:50:13 +02:00
Author
Owner
Copy Link

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Questions & Observations

  • No infrastructure impact. This is a pure frontend file deletion -- no Docker, no CI workflow, no environment variable changes.
  • The build output will be slightly smaller (fewer routes bundled), which is a minor win.

Suggestions

  • No concerns from my angle. Verify npm run build succeeds (already in acceptance criteria) and the CI pipeline stays green. That is the full extent of the deployment check needed here.
## ⚙️ Tobias Wendt -- DevOps & Platform Engineer ### Questions & Observations - No infrastructure impact. This is a pure frontend file deletion -- no Docker, no CI workflow, no environment variable changes. - The build output will be slightly smaller (fewer routes bundled), which is a minor win. ### Suggestions - No concerns from my angle. Verify `npm run build` succeeds (already in acceptance criteria) and the CI pipeline stays green. That is the full extent of the deployment check needed here.
marcel commented 2026-04-07 11:04:38 +02:00
Author
Owner
Copy Link

Addressing Review Feedback

Re: Felix Brandt

  • Layout files in conversations/: Verified via ls -R. The directory contains exactly 5 files: +page.svelte, +page.server.ts, ConversationFilterBar.svelte, ConversationTimeline.svelte, page.svelte.spec.ts. No +layout.svelte or +layout.server.ts. The issue's 4-file list missed the spec file -- page.svelte.spec.ts must also be deleted.
  • Vitest tests importing from conversations/: Yes. frontend/src/routes/conversations/page.svelte.spec.ts (165 lines) is a full Vitest browser test suite for the old page. It must be deleted along with the route files.
  • Component naming collision with briefwechsel/: Both directories have ConversationFilterBar.svelte and ConversationTimeline.svelte. No cross-directory imports exist -- each route imports its own copy via relative ./ paths (conversations +page.svelte:6-7). No collision risk, but confirms these are independent duplicates.
  • grep before deleting: Done. The only source-code references to /conversations outside the conversations/ directory itself are in E2E tests (see Sara Holt section below). AppNav.svelte links to /briefwechsel, not /conversations (lines 63, 164).
  • Add npm run test to acceptance criteria: Agreed. Adding it.
  • One atomic commit: Agreed. Directory deletion + orphaned i18n keys + E2E test cleanup = one logical change.

Re: Markus Keller

  • Backend endpoints: +page.server.ts:22 calls GET /api/documents/conversation and GET /api/persons/{id}. Both are also used by briefwechsel/+page.server.ts:29. These are shared endpoints -- no backend cleanup needed.

  • Orphaned i18n keys: 14 keys in messages/{de,en,es}.json are used ONLY by the conversations route (not by briefwechsel or any other source file):

    • conv_heading, conv_subtitle, conv_empty_text, conv_summary
    • conv_label_correspondent_optional, conv_hint_single_person, conv_hint_single_person_filtered
    • conv_strip_period, conv_strip_from_placeholder, conv_strip_to_placeholder, conv_strip_all_correspondents
    • conv_empty_search_placeholder, conv_asym_sent, conv_asym_received

    These should be removed from all three message files. The remaining conv_* keys are actively used by briefwechsel/ and must stay.

Re: Sara Holt

  • E2E tests navigating to /conversations: Yes, significant impact found:

    • frontend/e2e/auth.spec.ts:27 -- includes /conversations in the protected-routes redirect test array
    • frontend/e2e/persons.spec.ts:185-311 -- entire test.describe('Person detail -- conversations link') block (lines 185-203), test.describe('Conversations') block (lines 205-228), and test.describe('Conversations -- enhancements') block (lines 230-312) all navigate to /conversations

    These E2E tests must be removed or rewritten to target /briefwechsel. The auth spec line should replace /conversations with /briefwechsel. The persons.spec.ts conversation tests (lines 185-312) should be deleted entirely if equivalent tests exist for briefwechsel, or rewritten to use /briefwechsel if not.

  • Add npm run test and E2E pass to acceptance criteria: Agreed on both.

Re: Nora "NullX" Steiner

  • Authorization model: conversations/+page.server.ts has no unique auth checks beyond the global layout auth (+layout.server.ts loads the user session). The canWrite flag passed to the template (line 101 of +page.svelte) comes from the layout data, same as briefwechsel. No security model difference.
  • 301 redirect: This is a small family app. No external bookmarks are expected. A redirect would add a file where we're trying to delete files. Skip it -- a 404 is fine. If someone reports a broken bookmark, a redirect can be added then.

Re: Leonie Voss

  • Bookmarked URLs: Same answer as above. Small user base, no external link sharing. Not worth adding a redirect preemptively.

Re: Tobias Wendt

  • No concerns: Confirmed. Pure frontend deletion, no infra impact.

Updated Acceptance Criteria

Based on the feedback, the full checklist should be:

  • conversations/ directory fully removed (5 files, not 4 -- includes page.svelte.spec.ts)
  • 14 orphaned i18n keys removed from messages/de.json, en.json, es.json
  • E2E tests updated: auth.spec.ts line 27 changed from /conversations to /briefwechsel; conversation test blocks in persons.spec.ts (lines 185-312) removed or rewritten for /briefwechsel
  • npm run check passes
  • npm run build passes
  • npm run test passes
  • E2E tests pass (no references to /conversations remain)
## Addressing Review Feedback ### Re: Felix Brandt - **Layout files in conversations/**: Verified via `ls -R`. The directory contains exactly 5 files: `+page.svelte`, `+page.server.ts`, `ConversationFilterBar.svelte`, `ConversationTimeline.svelte`, `page.svelte.spec.ts`. No `+layout.svelte` or `+layout.server.ts`. The issue's 4-file list missed the spec file -- `page.svelte.spec.ts` must also be deleted. - **Vitest tests importing from conversations/**: Yes. `frontend/src/routes/conversations/page.svelte.spec.ts` (165 lines) is a full Vitest browser test suite for the old page. It must be deleted along with the route files. - **Component naming collision with briefwechsel/**: Both directories have `ConversationFilterBar.svelte` and `ConversationTimeline.svelte`. No cross-directory imports exist -- each route imports its own copy via relative `./` paths (conversations `+page.svelte:6-7`). No collision risk, but confirms these are independent duplicates. - **grep before deleting**: Done. The only source-code references to `/conversations` outside the conversations/ directory itself are in E2E tests (see Sara Holt section below). `AppNav.svelte` links to `/briefwechsel`, not `/conversations` (lines 63, 164). - **Add `npm run test` to acceptance criteria**: Agreed. Adding it. - **One atomic commit**: Agreed. Directory deletion + orphaned i18n keys + E2E test cleanup = one logical change. ### Re: Markus Keller - **Backend endpoints**: `+page.server.ts:22` calls `GET /api/documents/conversation` and `GET /api/persons/{id}`. Both are also used by `briefwechsel/+page.server.ts:29`. These are shared endpoints -- no backend cleanup needed. - **Orphaned i18n keys**: 14 keys in `messages/{de,en,es}.json` are used ONLY by the conversations route (not by briefwechsel or any other source file): - `conv_heading`, `conv_subtitle`, `conv_empty_text`, `conv_summary` - `conv_label_correspondent_optional`, `conv_hint_single_person`, `conv_hint_single_person_filtered` - `conv_strip_period`, `conv_strip_from_placeholder`, `conv_strip_to_placeholder`, `conv_strip_all_correspondents` - `conv_empty_search_placeholder`, `conv_asym_sent`, `conv_asym_received` These should be removed from all three message files. The remaining `conv_*` keys are actively used by briefwechsel/ and must stay. ### Re: Sara Holt - **E2E tests navigating to /conversations**: Yes, significant impact found: - `frontend/e2e/auth.spec.ts:27` -- includes `/conversations` in the protected-routes redirect test array - `frontend/e2e/persons.spec.ts:185-311` -- entire `test.describe('Person detail -- conversations link')` block (lines 185-203), `test.describe('Conversations')` block (lines 205-228), and `test.describe('Conversations -- enhancements')` block (lines 230-312) all navigate to `/conversations` These E2E tests must be removed or rewritten to target `/briefwechsel`. The auth spec line should replace `/conversations` with `/briefwechsel`. The persons.spec.ts conversation tests (lines 185-312) should be deleted entirely if equivalent tests exist for briefwechsel, or rewritten to use `/briefwechsel` if not. - **Add `npm run test` and E2E pass to acceptance criteria**: Agreed on both. ### Re: Nora "NullX" Steiner - **Authorization model**: `conversations/+page.server.ts` has no unique auth checks beyond the global layout auth (`+layout.server.ts` loads the user session). The `canWrite` flag passed to the template (line 101 of `+page.svelte`) comes from the layout data, same as briefwechsel. No security model difference. - **301 redirect**: This is a small family app. No external bookmarks are expected. A redirect would add a file where we're trying to delete files. Skip it -- a 404 is fine. If someone reports a broken bookmark, a redirect can be added then. ### Re: Leonie Voss - **Bookmarked URLs**: Same answer as above. Small user base, no external link sharing. Not worth adding a redirect preemptively. ### Re: Tobias Wendt - **No concerns**: Confirmed. Pure frontend deletion, no infra impact. ### Updated Acceptance Criteria Based on the feedback, the full checklist should be: - [ ] `conversations/` directory fully removed (5 files, not 4 -- includes `page.svelte.spec.ts`) - [ ] 14 orphaned i18n keys removed from `messages/de.json`, `en.json`, `es.json` - [ ] E2E tests updated: `auth.spec.ts` line 27 changed from `/conversations` to `/briefwechsel`; conversation test blocks in `persons.spec.ts` (lines 185-312) removed or rewritten for `/briefwechsel` - [ ] `npm run check` passes - [ ] `npm run build` passes - [ ] `npm run test` passes - [ ] E2E tests pass (no references to `/conversations` remain)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label refactor
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#193
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?