bug: notification deep-link does not scroll to comment on document detail page #276

New Issue

marcel · 2026-04-19T15:59:19+02:00

marcel commented

2026-04-19 15:59:19 +02:00

What's broken

Clicking a notification (bell dropdown or notifications page) navigates to /documents/{id}?commentId={uuid} but nothing happens on the document detail page — no scroll, no highlight, no annotation panel opened. The link lands on the page and the query params are silently ignored.

This was working at some point and was lost during the component split/refactoring.

Root cause analysis

URL generation — partially broken

NotificationBell.svelte:35 and notifications/+page.svelte:53 both try to include annotationId in the URL when it is present on the notification object:

notification.annotationId
  ? ...?commentId=...&annotationId=...
  : ...?commentId=...

However, in practice notification.annotationId is null even for annotation comments, so the URL only ever contains ?commentId={uuid}. The upstream cause (why annotationId isn't reaching the notification) is a separate investigation; for the purposes of this fix, treat commentId as the only reliable param.

Since all comments are annotation comments, a commentId always resolves to exactly one annotation. The annotation can be looked up server-side from the comment.

Consuming side — entirely missing ❌

documents/[id]/+page.svelte has no code that reads ?commentId from url.searchParams. The page receives the param in the URL but ignores it.

DOM anchor — missing ❌

CommentMessage.svelte renders each comment as <div role="article" class="flex gap-2"> with no id attribute. Even if the page read ?commentId, there is nothing to scroll to.

What needs to be implemented

1. Backend: endpoint to resolve a comment's annotation

Add GET /api/documents/{documentId}/comments/{commentId} (or a dedicated context endpoint) that returns at minimum the comment's annotationId. This is needed so the detail page can open the right annotation panel without annotationId in the URL.

Alternatively, extend the existing notification DTO/API to reliably return annotationId — but the frontend fix should not depend on this being fixed first.

2. Add `id` anchor to `CommentMessage.svelte`

<div id="comment-{message.id}" role="article" class="flex gap-2">

3. Read `?commentId` and resolve annotation in `documents/[id]/+page.svelte` `onMount`

import { page } from '$app/state';
import { replaceState } from '$app/navigation';

onMount(async () => {
    // ... existing logic ...

    const commentId = page.url.searchParams.get('commentId');
    if (commentId) {
        // Resolve the annotation from the comment
        const res = await fetch(`/api/documents/${doc.id}/comments/${commentId}`);
        if (res.ok) {
            const comment = await res.json();
            if (comment.annotationId) {
                activeAnnotationId = comment.annotationId;
                flashAnnotationId  = comment.annotationId;
                // Wait for the annotation panel to render, then scroll
                await tick();
                document.getElementById(`comment-${commentId}`)
                    ?.scrollIntoView({ behavior: scrollBehavior, block: 'nearest' });
            }
        }
        replaceState(page.url.pathname, {});
    }
});

4. Ordering: annotation panel must be open before scroll

The CommentThread lives inside the annotation detail panel. activeAnnotationId must be set and the panel must finish rendering before scrollIntoView is called. A single tick() may not be sufficient if the panel has async rendering — a requestAnimationFrame inside the tick may be needed.

Scope

frontend/src/lib/components/CommentMessage.svelte — add id="comment-{message.id}" to the article wrapper
frontend/src/routes/documents/[id]/+page.svelte — read ?commentId in onMount, fetch comment to resolve annotationId, activate annotation panel, scroll to comment, clean up URL
backend/CommentController.java — add GET /api/documents/{documentId}/comments/{commentId} read endpoint (or extend existing notification API)
No database changes needed

## What's broken Clicking a notification (bell dropdown or notifications page) navigates to `/documents/{id}?commentId={uuid}` but nothing happens on the document detail page — no scroll, no highlight, no annotation panel opened. The link lands on the page and the query params are silently ignored. This was working at some point and was lost during the component split/refactoring. --- ## Root cause analysis ### URL generation — partially broken `NotificationBell.svelte:35` and `notifications/+page.svelte:53` both try to include `annotationId` in the URL when it is present on the notification object: ``` notification.annotationId ? ...?commentId=...&annotationId=... : ...?commentId=... ``` However, in practice `notification.annotationId` is `null` even for annotation comments, so the URL only ever contains `?commentId={uuid}`. The upstream cause (why `annotationId` isn't reaching the notification) is a separate investigation; for the purposes of this fix, **treat `commentId` as the only reliable param**. Since all comments are annotation comments, a `commentId` always resolves to exactly one annotation. The annotation can be looked up server-side from the comment. ### Consuming side — entirely missing ❌ `documents/[id]/+page.svelte` has **no code** that reads `?commentId` from `url.searchParams`. The page receives the param in the URL but ignores it. ### DOM anchor — missing ❌ `CommentMessage.svelte` renders each comment as `<div role="article" class="flex gap-2">` with no `id` attribute. Even if the page read `?commentId`, there is nothing to scroll to. --- ## What needs to be implemented ### 1. Backend: endpoint to resolve a comment's annotation Add `GET /api/documents/{documentId}/comments/{commentId}` (or a dedicated context endpoint) that returns at minimum the comment's `annotationId`. This is needed so the detail page can open the right annotation panel without `annotationId` in the URL. Alternatively, extend the existing notification DTO/API to reliably return `annotationId` — but the frontend fix should not depend on this being fixed first. ### 2. Add `id` anchor to `CommentMessage.svelte` ```svelte <div id="comment-{message.id}" role="article" class="flex gap-2"> ``` ### 3. Read `?commentId` and resolve annotation in `documents/[id]/+page.svelte` `onMount` ```typescript import { page } from '$app/state'; import { replaceState } from '$app/navigation'; onMount(async () => { // ... existing logic ... const commentId = page.url.searchParams.get('commentId'); if (commentId) { // Resolve the annotation from the comment const res = await fetch(`/api/documents/${doc.id}/comments/${commentId}`); if (res.ok) { const comment = await res.json(); if (comment.annotationId) { activeAnnotationId = comment.annotationId; flashAnnotationId = comment.annotationId; // Wait for the annotation panel to render, then scroll await tick(); document.getElementById(`comment-${commentId}`) ?.scrollIntoView({ behavior: scrollBehavior, block: 'nearest' }); } } replaceState(page.url.pathname, {}); } }); ``` ### 4. Ordering: annotation panel must be open before scroll The `CommentThread` lives inside the annotation detail panel. `activeAnnotationId` must be set and the panel must finish rendering before `scrollIntoView` is called. A single `tick()` may not be sufficient if the panel has async rendering — a `requestAnimationFrame` inside the tick may be needed. --- ## Scope - `frontend/src/lib/components/CommentMessage.svelte` — add `id="comment-{message.id}"` to the article wrapper - `frontend/src/routes/documents/[id]/+page.svelte` — read `?commentId` in `onMount`, fetch comment to resolve `annotationId`, activate annotation panel, scroll to comment, clean up URL - `backend/CommentController.java` — add `GET /api/documents/{documentId}/comments/{commentId}` read endpoint (or extend existing notification API) - No database changes needed

marcel added the bug notification labels 2026-04-19 15:59:24 +02:00

marcel commented

2026-04-21 12:35:12 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The existing handleAnnotationClick at documents/[id]/+page.svelte:220-248 already does activate-panel + flash + requestAnimationFrame + scrollIntoView. That is the pattern the ?commentId handler should reuse — not duplicate.
prefers-reduced-motion is already honored at documents/[id]/+page.svelte:41-43 and used to choose scrollBehavior and flash duration. The new path must reuse the same derivation.
Issue references notifications/+page.svelte:53 — that file does not exist in the current repo. URL generation lives only in NotificationBell.svelte:34-37. If a standalone notifications page is intended separately, scope this fix to the bell dropdown only or clarify the path.
CommentMessage.svelte:35 confirmed: <div role="article" class="flex gap-2"> has no id. Adding id="comment-{message.id}" is correct.

Recommendations

Extract a named helper scrollToCommentFromQuery(commentId) at the top of the <script>. It fetches the comment, sets activeAnnotationId, waits one requestAnimationFrame after tick(), scrolls, flashes the comment, then calls replaceState(page.url.pathname, {}). Reuse the scrollBehavior already derived from prefersReducedMotion.
Use import { page } from '$app/state' (Svelte 5) — the deprecated store form should not creep in.
TDD order:
1. Red: scrollIntoView is called on #comment-{id} when ?commentId is present and backend returns 200
2. Red: no fetch, no scroll when ?commentId is absent
3. Red: graceful no-op when backend returns 404 (no crash, no visible error)
4. Red: replaceState strips commentId after handling
5. Red: reduced-motion path uses 'instant' behavior
Backend: return the full DocumentComment entity from the new GET, not a {annotationId} projection. Clients already handle the shape; a one-field DTO adds coupling for no gain.

Open Decisions

Comments live on documents, annotations, OR transcription blocks (CommentController.java:29-117). The issue assumes annotation scope. A block-comment deep-link would need transcribe mode + block activation; a document-level comment has no annotation to open. Options: (a) scope this PR to annotation comments only, explicit no-op + TODO for the other two; (b) handle all three scopes now. (a) ships quickly with a known gap; (b) meaningfully larger.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The existing `handleAnnotationClick` at `documents/[id]/+page.svelte:220-248` already does activate-panel + flash + `requestAnimationFrame` + `scrollIntoView`. That is the pattern the `?commentId` handler should reuse — not duplicate. - `prefers-reduced-motion` is already honored at `documents/[id]/+page.svelte:41-43` and used to choose `scrollBehavior` and flash duration. The new path must reuse the same derivation. - Issue references `notifications/+page.svelte:53` — that file does not exist in the current repo. URL generation lives only in `NotificationBell.svelte:34-37`. If a standalone notifications page is intended separately, scope this fix to the bell dropdown only or clarify the path. - `CommentMessage.svelte:35` confirmed: `<div role="article" class="flex gap-2">` has no `id`. Adding `id="comment-{message.id}"` is correct. ### Recommendations - Extract a named helper `scrollToCommentFromQuery(commentId)` at the top of the `<script>`. It fetches the comment, sets `activeAnnotationId`, waits one `requestAnimationFrame` after `tick()`, scrolls, flashes the comment, then calls `replaceState(page.url.pathname, {})`. Reuse the `scrollBehavior` already derived from `prefersReducedMotion`. - Use `import { page } from '$app/state'` (Svelte 5) — the deprecated store form should not creep in. - TDD order: 1. Red: `scrollIntoView` is called on `#comment-{id}` when `?commentId` is present and backend returns 200 2. Red: no fetch, no scroll when `?commentId` is absent 3. Red: graceful no-op when backend returns 404 (no crash, no visible error) 4. Red: `replaceState` strips `commentId` after handling 5. Red: reduced-motion path uses `'instant'` behavior - Backend: return the full `DocumentComment` entity from the new GET, not a `{annotationId}` projection. Clients already handle the shape; a one-field DTO adds coupling for no gain. ### Open Decisions - Comments live on documents, annotations, OR transcription blocks (`CommentController.java:29-117`). The issue assumes annotation scope. A block-comment deep-link would need transcribe mode + block activation; a document-level comment has no annotation to open. Options: (a) scope this PR to annotation comments only, explicit no-op + TODO for the other two; (b) handle all three scopes now. (a) ships quickly with a known gap; (b) meaningfully larger.

marcel commented

2026-04-21 12:35:22 +02:00

🏛️ Markus Keller — Application Architect

Observations

The proposed fix adds a new endpoint to compensate for a known upstream defect (notification.annotationId is null even for annotation comments). The frontend fix is clean and isolated, but the underlying data bug remains — every notification click-through will make one extra round-trip forever.
CommentController.java:29-117 has no @RequirePermission on the GET endpoints. Authentication is enforced by Spring Security's blanket .authenticated(), but there is no declarative per-document authorization at the controller layer. Any new GET should match the existing pattern, not invent a new convention mid-PR.
The issue cites notifications/+page.svelte:53 — that path doesn't exist in the repo. Worth clarifying before implementation to avoid confusion.

Recommendations

Place the new read at GET /api/documents/{documentId}/comments/{commentId}. It reads symmetrically against the other three GETs in CommentController (document, annotation, block). Keeping {documentId} in the path gives Nora a clean document-match guard to add.
Return the full DocumentComment entity. Single-field projection DTOs create coupling that rots when the frontend wants one more field.

Open Decisions

Fix the upstream or add the endpoint. Two legitimate paths:
- (A) Fix upstream: populate annotationId on notification creation, backfill existing rows in a small migration. The frontend fix drops the new endpoint entirely — just reads ?annotationId from the URL. Cost: touches the notification write path and adds a one-time backfill.
- (B) Keep the endpoint as proposed. Accepts the upstream bug as permanent and adds persistent scaffolding around it.
  My recommendation is (A). The fix is small enough that doing it once is cheaper than maintaining two paths. But it expands this PR's scope — your call whether to do it here or split.

## 🏛️ Markus Keller — Application Architect ### Observations - The proposed fix adds a new endpoint to compensate for a known upstream defect (`notification.annotationId` is null even for annotation comments). The frontend fix is clean and isolated, but the underlying data bug remains — every notification click-through will make one extra round-trip forever. - `CommentController.java:29-117` has no `@RequirePermission` on the GET endpoints. Authentication is enforced by Spring Security's blanket `.authenticated()`, but there is no declarative per-document authorization at the controller layer. Any new GET should match the existing pattern, not invent a new convention mid-PR. - The issue cites `notifications/+page.svelte:53` — that path doesn't exist in the repo. Worth clarifying before implementation to avoid confusion. ### Recommendations - Place the new read at `GET /api/documents/{documentId}/comments/{commentId}`. It reads symmetrically against the other three GETs in `CommentController` (document, annotation, block). Keeping `{documentId}` in the path gives Nora a clean document-match guard to add. - Return the full `DocumentComment` entity. Single-field projection DTOs create coupling that rots when the frontend wants one more field. ### Open Decisions - **Fix the upstream or add the endpoint.** Two legitimate paths: - (A) Fix upstream: populate `annotationId` on notification creation, backfill existing rows in a small migration. The frontend fix drops the new endpoint entirely — just reads `?annotationId` from the URL. Cost: touches the notification write path and adds a one-time backfill. - (B) Keep the endpoint as proposed. Accepts the upstream bug as permanent and adds persistent scaffolding around it. My recommendation is (A). The fix is small enough that doing it once is cheaper than maintaining two paths. But it expands this PR's scope — your call whether to do it here or split.

marcel commented

2026-04-21 12:35:32 +02:00

🛡️ Nora "NullX" Steiner — Application Security Engineer

Observations

Existing comment GETs in CommentController.java have no @RequirePermission. Global .authenticated() is enforced at the Spring Security layer, but per-document authorization is not visible at the controller. Any authenticated user can call GET /api/documents/{docId}/comments today for any document. Confirm this is intentional for the family-internal trust model.
A notification deep-link URL carrying ?commentId=<uuid> will be followed by whoever opens it. If the link is forwarded or the email client prefetches it, the recipient must not learn anything they couldn't learn without the URL.

Recommendations

Validate document↔comment match. Otherwise GET /api/documents/{docA}/comments/{commentIdFromDocB} leaks comment B's annotation id — weak IDOR. Return 404 (not 200, not 403) when the comment's documentId does not equal the path documentId.
Add explicit tests on the new endpoint:
- @WithAnonymousUser → 401
- Valid user but document mismatch → 404
- Valid user, valid match → 200 with annotationId
- Malformed UUID in path → 400 (Spring's UUID coercion; confirm it's a 400, not a 500)
Log WARN with a structured tag (event=comment_lookup_failed, commentId={}) when the lookup misses. Pattern: logger.warn("Stale notification deep-link: commentId={}", commentId) — parameterized form only, never +.
Frontend: the commentId value is a raw string from page.url.searchParams.get(). Template-literal concatenation into the fetch URL is safe as long as the handler rejects malformed UUIDs. Don't build the URL via encodeURIComponent — unnecessary, and hides malformed input you want the backend to reject loudly.

Open Decisions

None from my angle once the document-match guard is in place. Separately: the blanket missing @RequirePermission on existing GETs is worth a dedicated issue — out of scope here but I'd tag it.

## 🛡️ Nora "NullX" Steiner — Application Security Engineer ### Observations - Existing comment GETs in `CommentController.java` have no `@RequirePermission`. Global `.authenticated()` is enforced at the Spring Security layer, but per-document authorization is not visible at the controller. Any authenticated user can call `GET /api/documents/{docId}/comments` today for any document. Confirm this is intentional for the family-internal trust model. - A notification deep-link URL carrying `?commentId=<uuid>` will be followed by whoever opens it. If the link is forwarded or the email client prefetches it, the recipient must not learn anything they couldn't learn without the URL. ### Recommendations - **Validate document↔comment match.** Otherwise `GET /api/documents/{docA}/comments/{commentIdFromDocB}` leaks comment B's annotation id — weak IDOR. Return 404 (not 200, not 403) when the comment's `documentId` does not equal the path `documentId`. - Add explicit tests on the new endpoint: - `@WithAnonymousUser` → 401 - Valid user but document mismatch → 404 - Valid user, valid match → 200 with annotationId - Malformed UUID in path → 400 (Spring's `UUID` coercion; confirm it's a 400, not a 500) - Log `WARN` with a structured tag (`event=comment_lookup_failed`, `commentId={}`) when the lookup misses. Pattern: `logger.warn("Stale notification deep-link: commentId={}", commentId)` — parameterized form only, never `+`. - Frontend: the `commentId` value is a raw string from `page.url.searchParams.get()`. Template-literal concatenation into the fetch URL is safe as long as the handler rejects malformed UUIDs. Don't build the URL via `encodeURIComponent` — unnecessary, and hides malformed input you want the backend to reject loudly. ### Open Decisions - None from my angle once the document-match guard is in place. Separately: the blanket missing `@RequirePermission` on existing GETs is worth a dedicated issue — out of scope here but I'd tag it.

marcel commented

2026-04-21 12:35:42 +02:00

🧪 Sara Holt — QA & Test Strategist

Observations

The issue correctly flags the timing race: activeAnnotationId set → panel renders → scroll. That's a known brittle surface — testing it wrong produces a flaky suite.
handleAnnotationClick at documents/[id]/+page.svelte:242 already combines requestAnimationFrame with scrollIntoView. Tests should assert the same timing pattern, not reinvent it.

Recommendations

Backend — @WebMvcTest(CommentController.class):

getCommentById_returns_comment_with_annotationId_when_found
getCommentById_returns_404_when_commentId_does_not_exist
getCommentById_returns_404_when_documentId_does_not_match (per Nora)
getCommentById_returns_401_when_unauthenticated
getCommentById_returns_400_when_commentId_is_not_a_valid_uuid

Frontend — vitest-browser-svelte tests on documents/[id]/+page.svelte:

scrolls_to_comment_when_commentId_param_is_present
no_scroll_attempted_when_commentId_param_absent
graceful_noop_when_backend_returns_404_for_commentId
strips_commentId_from_url_after_handling
honors_prefers_reduced_motion_for_scroll_behavior

Mock fetch at the module boundary. Use waitFor on the scroll spy — never setTimeout in the assertion.

E2E — Playwright (frontend/e2e/):

Critical journey: click notification in bell dropdown → URL contains ?commentId=X → annotation panel open → target comment visible in viewport + flash visible. Run at 320px AND 1440px; the 320px layout is the one most likely to hide the comment behind the PDF strip.
Include an axe-core check on the page once the comment is focused — new id, new tabindex if Leonie's focus proposal lands. Don't let accessibility regress silently.

Open Decisions

None from my angle. The test strategy above is a straightforward pyramid — no judgment calls required.

## 🧪 Sara Holt — QA & Test Strategist ### Observations - The issue correctly flags the timing race: `activeAnnotationId` set → panel renders → scroll. That's a known brittle surface — testing it wrong produces a flaky suite. - `handleAnnotationClick` at `documents/[id]/+page.svelte:242` already combines `requestAnimationFrame` with `scrollIntoView`. Tests should assert the same timing pattern, not reinvent it. ### Recommendations **Backend — `@WebMvcTest(CommentController.class)`:** - `getCommentById_returns_comment_with_annotationId_when_found` - `getCommentById_returns_404_when_commentId_does_not_exist` - `getCommentById_returns_404_when_documentId_does_not_match` (per Nora) - `getCommentById_returns_401_when_unauthenticated` - `getCommentById_returns_400_when_commentId_is_not_a_valid_uuid` **Frontend — `vitest-browser-svelte` tests on `documents/[id]/+page.svelte`:** - `scrolls_to_comment_when_commentId_param_is_present` - `no_scroll_attempted_when_commentId_param_absent` - `graceful_noop_when_backend_returns_404_for_commentId` - `strips_commentId_from_url_after_handling` - `honors_prefers_reduced_motion_for_scroll_behavior` Mock `fetch` at the module boundary. Use `waitFor` on the scroll spy — never `setTimeout` in the assertion. **E2E — Playwright (`frontend/e2e/`):** - Critical journey: click notification in bell dropdown → URL contains `?commentId=X` → annotation panel open → target comment visible in viewport + flash visible. Run at 320px AND 1440px; the 320px layout is the one most likely to hide the comment behind the PDF strip. - Include an axe-core check on the page once the comment is focused — new `id`, new `tabindex` if Leonie's focus proposal lands. Don't let accessibility regress silently. ### Open Decisions - None from my angle. The test strategy above is a straightforward pyramid — no judgment calls required.

marcel commented

2026-04-21 12:35:54 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

prefers-reduced-motion is already plumbed in documents/[id]/+page.svelte:41-43 and used for both the scroll behavior ('instant' vs 'smooth') and flash duration (2000 ms vs 1500 ms) in handleAnnotationClick. The new code path must reuse that same derivation — introducing a second motion policy breaks the feel.
CommentMessage.svelte:35 has no visible focus indicator today. Adding just id="comment-{message.id}" doesn't help screen readers or keyboard users land on the comment — it only helps scrollIntoView.
At 320px, the annotation panel becomes full-width below md (see documents/[id]/+page.svelte:375). After scroll, the comment can end up partially hidden behind the 70px collapsed PDF strip.

Recommendations

Reuse the flashAnnotationId vocabulary. After the scroll, trigger the existing flash animation on the comment for 1500 ms (2000 ms with reduced motion). Users already recognize this as "here's where you landed" from annotation clicks — don't teach them a second visual.
Add tabindex="-1" alongside the new id on CommentMessage.svelte. Then call .focus() after scrollIntoView. This moves screen-reader focus so the comment announces itself, without polluting the normal tab order. Pair with focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 + outline-none on the same element — visible ring for sighted keyboard users.
Use block: 'center' not block: 'nearest' for the scroll. At 320px the collapsed PDF strip can clip 'nearest' positioning; 'center' guarantees vertical centering regardless of surrounding chrome.
Do not add aria-live to the comment list container — the list rerenders frequently during editing and would over-announce.

Open Decisions

Flash alone vs. flash + focus transfer. Flash alone: familiar, silent for screen readers. Flash + focus: announces the landing, adds a visible ring on the comment for keyboard users, costs ~5 lines. Both is my recommendation; the ring only appears under focus-visible, so mouse users never see it. But it's a judgment call on UX weight.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - `prefers-reduced-motion` is already plumbed in `documents/[id]/+page.svelte:41-43` and used for both the scroll behavior (`'instant'` vs `'smooth'`) and flash duration (2000 ms vs 1500 ms) in `handleAnnotationClick`. The new code path must reuse that same derivation — introducing a second motion policy breaks the feel. - `CommentMessage.svelte:35` has no visible focus indicator today. Adding just `id="comment-{message.id}"` doesn't help screen readers or keyboard users land on the comment — it only helps `scrollIntoView`. - At 320px, the annotation panel becomes full-width below `md` (see `documents/[id]/+page.svelte:375`). After scroll, the comment can end up partially hidden behind the 70px collapsed PDF strip. ### Recommendations - **Reuse the `flashAnnotationId` vocabulary.** After the scroll, trigger the existing flash animation on the comment for 1500 ms (2000 ms with reduced motion). Users already recognize this as "here's where you landed" from annotation clicks — don't teach them a second visual. - **Add `tabindex="-1"` alongside the new `id` on `CommentMessage.svelte`.** Then call `.focus()` after `scrollIntoView`. This moves screen-reader focus so the comment announces itself, without polluting the normal tab order. Pair with `focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2` + `outline-none` on the same element — visible ring for sighted keyboard users. - **Use `block: 'center'` not `block: 'nearest'` for the scroll.** At 320px the collapsed PDF strip can clip `'nearest'` positioning; `'center'` guarantees vertical centering regardless of surrounding chrome. - Do not add `aria-live` to the comment list container — the list rerenders frequently during editing and would over-announce. ### Open Decisions - **Flash alone vs. flash + focus transfer.** Flash alone: familiar, silent for screen readers. Flash + focus: announces the landing, adds a visible ring on the comment for keyboard users, costs ~5 lines. Both is my recommendation; the ring only appears under `focus-visible`, so mouse users never see it. But it's a judgment call on UX weight.

marcel commented

2026-04-21 12:36:01 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

Pure application change: one backend GET handler, one frontend onMount branch, one attribute on CommentMessage.svelte. No Compose changes, no Caddy rules, no CI steps, no env vars, no migration (unless Markus's upstream-fix option is chosen).
Zero capacity impact on CX32. Request volume equals notification click-through rate.
Stale notifications (comment deleted after notification was issued) will hit the new endpoint and 404 silently. Without a log line, this is invisible operationally.

Recommendations

Add a WARN log on the 404 path with structured fields: logger.warn("Stale notification deep-link: commentId={}", commentId). Loki/Grafana picks it up automatically once the message format is grep-able — no new dashboard required.
If Markus's upstream-fix route is chosen (populate annotationId on notification creation + backfill migration), the backfill SQL belongs in a new Flyway migration file — don't modify existing migrations. Single UPDATE over notifications joined to comments; runs in a few hundred ms on the family dataset.
No Caddy block list change needed — the new endpoint sits under /api/* which is already proxied.

Open Decisions

None from my angle.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - Pure application change: one backend GET handler, one frontend `onMount` branch, one attribute on `CommentMessage.svelte`. No Compose changes, no Caddy rules, no CI steps, no env vars, no migration (unless Markus's upstream-fix option is chosen). - Zero capacity impact on CX32. Request volume equals notification click-through rate. - Stale notifications (comment deleted after notification was issued) will hit the new endpoint and 404 silently. Without a log line, this is invisible operationally. ### Recommendations - Add a `WARN` log on the 404 path with structured fields: `logger.warn("Stale notification deep-link: commentId={}", commentId)`. Loki/Grafana picks it up automatically once the message format is grep-able — no new dashboard required. - If Markus's upstream-fix route is chosen (populate `annotationId` on notification creation + backfill migration), the backfill SQL belongs in a new Flyway migration file — don't modify existing migrations. Single UPDATE over `notifications` joined to comments; runs in a few hundred ms on the family dataset. - No Caddy block list change needed — the new endpoint sits under `/api/*` which is already proxied. ### Open Decisions - None from my angle.

marcel commented

2026-04-21 12:36:11 +02:00

🗳️ Decision Queue — Action Required

3 decisions need your input before implementation starts.

Architecture

Upstream fix vs. new endpoint. The real root cause is that notification.annotationId is null even for annotation comments. The proposed fix adds a new GET endpoint to route around it. Options:
- (A) Fix upstream: populate annotationId on notification creation + one-time backfill migration. Drop the new endpoint entirely. Frontend reads ?annotationId from the URL directly. Cleaner long-term; expands this PR's scope.
- (B) Keep the new endpoint as proposed. Faster to ship; carries the upstream bug permanently. One extra HTTP round-trip on every notification click forever.
  (Raised by: Markus)

Scope

Comment scope coverage. Comments attach to three things: documents (annotationId null), annotations, and transcription blocks. The proposed fix assumes annotation scope. Options:
- (A) Annotation-only for this PR. Explicit no-op + TODO for document-level and block comments. Ships fast with a documented gap.
- (B) Handle all three scopes now. Block comments need transcribe mode + block activation; document-level needs no panel open. Larger PR but no known gap afterward.
  (Raised by: Felix)

Accessibility

Flash alone vs. flash + focus transfer on landing. Options:
- (A) Flash only. Familiar visual, no audible announcement for screen readers.
- (B) Flash + focus + visible ring. Adds tabindex="-1" to the comment and calls .focus() after scroll; a focus-visible ring appears only for keyboard users. ~5 extra lines, better for screen-reader and keyboard users.
  (Raised by: Leonie)

## 🗳️ Decision Queue — Action Required _3 decisions need your input before implementation starts._ ### Architecture - **Upstream fix vs. new endpoint.** The real root cause is that `notification.annotationId` is null even for annotation comments. The proposed fix adds a new GET endpoint to route around it. Options: - **(A) Fix upstream**: populate `annotationId` on notification creation + one-time backfill migration. Drop the new endpoint entirely. Frontend reads `?annotationId` from the URL directly. Cleaner long-term; expands this PR's scope. - **(B) Keep the new endpoint as proposed.** Faster to ship; carries the upstream bug permanently. One extra HTTP round-trip on every notification click forever. _(Raised by: Markus)_ ### Scope - **Comment scope coverage.** Comments attach to three things: documents (annotationId null), annotations, and transcription blocks. The proposed fix assumes annotation scope. Options: - **(A) Annotation-only for this PR.** Explicit no-op + TODO for document-level and block comments. Ships fast with a documented gap. - **(B) Handle all three scopes now.** Block comments need transcribe mode + block activation; document-level needs no panel open. Larger PR but no known gap afterward. _(Raised by: Felix)_ ### Accessibility - **Flash alone vs. flash + focus transfer on landing.** Options: - **(A) Flash only.** Familiar visual, no audible announcement for screen readers. - **(B) Flash + focus + visible ring.** Adds `tabindex="-1"` to the comment and calls `.focus()` after scroll; a `focus-visible` ring appears only for keyboard users. ~5 extra lines, better for screen-reader and keyboard users. _(Raised by: Leonie)_

marcel commented

2026-04-21 12:53:38 +02:00

🎯 Discussion Resolutions

After reviewing the persona feedback with the user, here are the agreed decisions. These supersede the original issue body where they conflict, and act as the authoritative design for implementation.

Important domain context surfaced during discussion: comments now exist only on transcription blocks. The document-level and annotation-level comment paths are dead code that this PR cleans up.

Theme 1 — Root-cause strategy

Decision: Fix upstream. In CommentService.postBlockComment, set comment.annotationId = block.getAnnotationId() at creation. Add a Flyway migration to backfill existing block comments from their blocks. The new GET endpoint proposed in the issue body is dropped entirely.

Rationale: The data model already has DocumentComment.annotationId; postBlockComment simply forgot to populate it. Fixing once is cheaper than a permanent workaround endpoint and one extra round-trip per notification click.

Theme 2 — Dead-code cleanup (bundled in this PR)

Decision: Delete all document-level and annotation-level comment code paths.

CommentController.java:29-86 — delete six handlers: getDocumentComments, postDocumentComment, replyToDocumentComment, getAnnotationComments, postAnnotationComment, replyToAnnotationComment.
CommentService — delete getCommentsForDocument, getCommentsForAnnotation, and the 2-arg postComment(documentId, annotationId, …) overload. Keep replyToComment (block-reply path uses it).
Delete corresponding test methods (don't comment out).
DocumentComment.annotationId column stays; its meaning is now "the block's annotation" for block comments.

Rationale: Zero frontend consumers of the dead endpoints were found. Shipping cleanup with the bug fix is cheaper than a separate PR.

Theme 3 — collapsed

No new backend endpoint needed (follows from Theme 1).

Theme 4 — Authorization on remaining block-comment GETs

Decision: No change. Every authenticated user is allowed to read in this family-internal model. The absence of @RequirePermission is intentional.

Rationale: Confirmed with user.

Theme 5 — Frontend handling flow

Decision: Extract a helper scrollToCommentFromQuery(commentId) in documents/[id]/+page.svelte. No backend fetch — the URL now reliably carries both ?commentId=…&annotationId=… after the Theme 1 upstream fix, and DOM id is the source of truth for "does this comment still exist?"

Flow:

Read both params via import { page } from '$app/state'; bail early if commentId is absent.
If transcribeMode is off, set it to true and await loadTranscriptionBlocks().
Set activeAnnotationId from the URL's annotationId.
await tick() + one requestAnimationFrame — mirrors the existing handleAnnotationClick:242-247 pattern.
document.getElementById('comment-' + commentId)?.scrollIntoView({ behavior: scrollBehavior, block: 'center' }).
.focus({ preventScroll: true }) on the same element.
Trigger the existing flashAnnotationId flash (1500 ms / 2000 ms reduced-motion).
replaceState(page.url.pathname, {}) to strip both query params.

Reuse points:

prefersReducedMotion derivation at lines 41-43 — unchanged.
flashAnnotationId vocabulary — no new animation.
block: 'center' chosen so the 320 px collapsed PDF strip can't clip the landing.

Rationale: Using the URL as state and the DOM as truth removes an entire class of failure (stale comments silently no-op) and eliminates a round-trip. The helper composes with the already-battle-tested handleAnnotationClick timing.

Theme 6 — DOM anchor and focus behavior

Decision: Ship flash + focus transfer.

On CommentMessage.svelte:35, change the wrapper to:

<div
    id="comment-{message.id}"
    role="article"
    tabindex="-1"
    class="flex gap-2 rounded outline-none
           focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2"
>

After scroll, call .focus({ preventScroll: true }) on the element. Do not add aria-live to the comment list container — it rerenders during editing and would over-announce.

Rationale: Flash covers sighted mouse users. tabindex="-1" + .focus() covers screen readers and sighted keyboard users. The focus-visible ring only appears for keyboard navigation — mouse users see nothing extra.

Theme 7 — Testing strategy

Backend (unit/slice):

CommentServiceTest.postBlockComment_sets_annotationId_from_block — core upstream fix
NotificationServiceTest.createsNotification_with_annotationId_populated_from_block_comment — regression pin on the notification path
Flyway migration implicitly covered by existing @SpringBootTest Testcontainers chain
Delete the test methods for the six dead controller handlers

Frontend (vitest-browser-svelte on documents/[id]/+page.svelte):

scrolls_to_comment_and_focuses_it_when_commentId_and_annotationId_are_in_url
enters_transcribe_mode_when_query_params_present_and_mode_is_off
no_scroll_attempted_when_commentId_absent
graceful_noop_when_target_comment_id_is_not_in_DOM
strips_both_commentId_and_annotationId_from_url_after_handling
honors_prefers_reduced_motion

Mock fetch at the module boundary. Use waitFor on scroll/focus spies — never setTimeout.

E2E (Playwright):

One critical journey at 320 px and 1440 px: seed a block comment + notification → log in as recipient → click notification in bell → assert URL has both params, transcribe mode active, block activated, #comment-{id} in viewport, flash visible, focus on article.
axe-core check after focus lands — guards the new tabindex + focus-visible ring against a11y regressions.

These resolutions are the authoritative spec. The implement skill will read this comment alongside the original issue body.

# 🎯 Discussion Resolutions After reviewing the persona feedback with the user, here are the agreed decisions. These supersede the original issue body where they conflict, and act as the authoritative design for implementation. **Important domain context surfaced during discussion:** comments now exist only on transcription blocks. The document-level and annotation-level comment paths are dead code that this PR cleans up. --- ## Theme 1 — Root-cause strategy **Decision:** Fix upstream. In `CommentService.postBlockComment`, set `comment.annotationId = block.getAnnotationId()` at creation. Add a Flyway migration to backfill existing block comments from their blocks. The new GET endpoint proposed in the issue body is dropped entirely. **Rationale:** The data model already has `DocumentComment.annotationId`; `postBlockComment` simply forgot to populate it. Fixing once is cheaper than a permanent workaround endpoint and one extra round-trip per notification click. --- ## Theme 2 — Dead-code cleanup (bundled in this PR) **Decision:** Delete all document-level and annotation-level comment code paths. - `CommentController.java:29-86` — delete six handlers: `getDocumentComments`, `postDocumentComment`, `replyToDocumentComment`, `getAnnotationComments`, `postAnnotationComment`, `replyToAnnotationComment`. - `CommentService` — delete `getCommentsForDocument`, `getCommentsForAnnotation`, and the 2-arg `postComment(documentId, annotationId, …)` overload. Keep `replyToComment` (block-reply path uses it). - Delete corresponding test methods (don't comment out). - `DocumentComment.annotationId` column stays; its meaning is now "the block's annotation" for block comments. **Rationale:** Zero frontend consumers of the dead endpoints were found. Shipping cleanup with the bug fix is cheaper than a separate PR. --- ## Theme 3 — collapsed No new backend endpoint needed (follows from Theme 1). --- ## Theme 4 — Authorization on remaining block-comment GETs **Decision:** No change. Every authenticated user is allowed to read in this family-internal model. The absence of `@RequirePermission` is intentional. **Rationale:** Confirmed with user. --- ## Theme 5 — Frontend handling flow **Decision:** Extract a helper `scrollToCommentFromQuery(commentId)` in `documents/[id]/+page.svelte`. No backend fetch — the URL now reliably carries both `?commentId=…&annotationId=…` after the Theme 1 upstream fix, and DOM id is the source of truth for "does this comment still exist?" Flow: 1. Read both params via `import { page } from '$app/state'`; bail early if `commentId` is absent. 2. If `transcribeMode` is off, set it to `true` and `await loadTranscriptionBlocks()`. 3. Set `activeAnnotationId` from the URL's `annotationId`. 4. `await tick()` + one `requestAnimationFrame` — mirrors the existing `handleAnnotationClick:242-247` pattern. 5. `document.getElementById('comment-' + commentId)?.scrollIntoView({ behavior: scrollBehavior, block: 'center' })`. 6. `.focus({ preventScroll: true })` on the same element. 7. Trigger the existing `flashAnnotationId` flash (1500 ms / 2000 ms reduced-motion). 8. `replaceState(page.url.pathname, {})` to strip both query params. Reuse points: - `prefersReducedMotion` derivation at lines 41-43 — unchanged. - `flashAnnotationId` vocabulary — no new animation. - `block: 'center'` chosen so the 320 px collapsed PDF strip can't clip the landing. **Rationale:** Using the URL as state and the DOM as truth removes an entire class of failure (stale comments silently no-op) and eliminates a round-trip. The helper composes with the already-battle-tested `handleAnnotationClick` timing. --- ## Theme 6 — DOM anchor and focus behavior **Decision:** Ship flash + focus transfer. On `CommentMessage.svelte:35`, change the wrapper to: ```svelte <div id="comment-{message.id}" role="article" tabindex="-1" class="flex gap-2 rounded outline-none focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2" > ``` After scroll, call `.focus({ preventScroll: true })` on the element. Do **not** add `aria-live` to the comment list container — it rerenders during editing and would over-announce. **Rationale:** Flash covers sighted mouse users. `tabindex="-1"` + `.focus()` covers screen readers and sighted keyboard users. The `focus-visible` ring only appears for keyboard navigation — mouse users see nothing extra. --- ## Theme 7 — Testing strategy **Backend (unit/slice):** - `CommentServiceTest.postBlockComment_sets_annotationId_from_block` — core upstream fix - `NotificationServiceTest.createsNotification_with_annotationId_populated_from_block_comment` — regression pin on the notification path - Flyway migration implicitly covered by existing `@SpringBootTest` Testcontainers chain - **Delete** the test methods for the six dead controller handlers **Frontend (`vitest-browser-svelte` on `documents/[id]/+page.svelte`):** - `scrolls_to_comment_and_focuses_it_when_commentId_and_annotationId_are_in_url` - `enters_transcribe_mode_when_query_params_present_and_mode_is_off` - `no_scroll_attempted_when_commentId_absent` - `graceful_noop_when_target_comment_id_is_not_in_DOM` - `strips_both_commentId_and_annotationId_from_url_after_handling` - `honors_prefers_reduced_motion` Mock `fetch` at the module boundary. Use `waitFor` on scroll/focus spies — never `setTimeout`. **E2E (Playwright):** - One critical journey at **320 px and 1440 px**: seed a block comment + notification → log in as recipient → click notification in bell → assert URL has both params, transcribe mode active, block activated, `#comment-{id}` in viewport, flash visible, focus on article. - `axe-core` check after focus lands — guards the new `tabindex` + `focus-visible` ring against a11y regressions. --- These resolutions are the authoritative spec. The `implement` skill will read this comment alongside the original issue body.

marcel commented

2026-04-21 13:47:35 +02:00

✅ Implementation complete

All tasks from the approved plan are green. Branch: feat/issue-276-notification-deep-link-scroll.

Commits (in order)

46588522 — fix(comment): populate annotationId on block comments from the block
13732ab9 — fix(db): V51 backfills annotation_id on block comments and notifications
bc69e8ff — refactor(comment): drop dead document and annotation comment APIs
251eb9c3 — feat(frontend): add scrollToCommentFromQuery helper for notification deep-link
20ae85f8 — feat(comment): expose comment id + focus ring on CommentMessage wrapper
e22265f5 — feat(document-detail): wire notification deep-link scroll in onMount
567faee3 — test(e2e): notification deep-link scrolls to target comment

Test matrix

Backend: 1191 tests pass (./mvnw test), build green (./mvnw clean package -DskipTests)
Frontend: 1122 tests pass across 129 test files (npm run test)
Net delta: −243 lines (dead code removal outweighs new code + tests)

What's next

deliver-issue takes over: push branch, open PR, run the review+fix loop.

## ✅ Implementation complete All tasks from the approved plan are green. Branch: `feat/issue-276-notification-deep-link-scroll`. ### Commits (in order) 1. `46588522` — fix(comment): populate annotationId on block comments from the block 2. `13732ab9` — fix(db): V51 backfills annotation_id on block comments and notifications 3. `bc69e8ff` — refactor(comment): drop dead document and annotation comment APIs 4. `251eb9c3` — feat(frontend): add scrollToCommentFromQuery helper for notification deep-link 5. `20ae85f8` — feat(comment): expose comment id + focus ring on CommentMessage wrapper 6. `e22265f5` — feat(document-detail): wire notification deep-link scroll in onMount 7. `567faee3` — test(e2e): notification deep-link scrolls to target comment ### Test matrix - Backend: **1191 tests pass** (`./mvnw test`), build green (`./mvnw clean package -DskipTests`) - Frontend: **1122 tests pass** across 129 test files (`npm run test`) - Net delta: −243 lines (dead code removal outweighs new code + tests) ### What's next `deliver-issue` takes over: push branch, open PR, run the review+fix loop.

marcel referenced this issue from a commit

2026-04-21 14:04:28 +02:00

fix(comment): populate annotationId on block comments from the block

marcel referenced a pull request that will close this issue

2026-04-21 14:04:57 +02:00

bug: notification deep-link does not scroll to comment on document detail page #299

marcel closed this issue

2026-04-21 15:06:02 +02:00

marcel referenced this issue

2026-04-21 15:49:36 +02:00

feat: chronik mentions should deep-link to the comment, matching the bell dropdown #300

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#276

bug: notification deep-link does not scroll to comment on document detail page #276

What's broken

Root cause analysis

URL generation — partially broken

Consuming side — entirely missing ❌

DOM anchor — missing ❌

What needs to be implemented

1. Backend: endpoint to resolve a comment's annotation

2. Add id anchor to CommentMessage.svelte

3. Read ?commentId and resolve annotation in documents/[id]/+page.svelte onMount

4. Ordering: annotation panel must be open before scroll

Scope

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Recommendations

Open Decisions

🏛️ Markus Keller — Application Architect

Observations

Recommendations

Open Decisions

🛡️ Nora "NullX" Steiner — Application Security Engineer

Observations

Recommendations

Open Decisions

🧪 Sara Holt — QA & Test Strategist

Observations

Recommendations

Open Decisions

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

Recommendations

Open Decisions

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

🗳️ Decision Queue — Action Required

Architecture

Scope

Accessibility

🎯 Discussion Resolutions

Theme 1 — Root-cause strategy

Theme 2 — Dead-code cleanup (bundled in this PR)

Theme 3 — collapsed

Theme 4 — Authorization on remaining block-comment GETs

Theme 5 — Frontend handling flow

Theme 6 — DOM anchor and focus behavior

Theme 7 — Testing strategy

✅ Implementation complete

Commits (in order)

Test matrix

What's next

2. Add `id` anchor to `CommentMessage.svelte`

3. Read `?commentId` and resolve annotation in `documents/[id]/+page.svelte` `onMount`