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

New Issue

Closed

opened 2026-04-21 15:49:36 +02:00 by marcel · 8 comments

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

Owner

Copy Link

What's missing

Clicking a mention or comment row on /chronik should land the user on the specific comment — same UX as clicking a notification in the bell dropdown (wired up in #276). Today the chronik rows land on /documents/{id} with no query params, so the deep-link scroll helper no-ops.

Current behaviour by entry point

Entry point	Link generated	Result
Bell dropdown (NotificationBell.svelte:34-37)	/documents/{id}?commentId=X&annotationId=Y (both params after #276)	Scrolls to comment, opens edit panel, flashes annotation, focuses article
Chronik "Für dich" sidebar (ChronikFuerDichBox.svelte:20-22)	/documents/{id}?commentId=X (only commentId)	No-op — scrollToCommentFromQuery bails when annotationId is missing
Chronik main feed row (ChronikRow.svelte:102)	/documents/{id} (no params at all)	No-op — user has to manually find the comment

Why they should match

Users don't distinguish between these surfaces in their mental model. A mention is a mention, whether it's surfaced in the bell or the timeline. Three separate UX paths for the same action is inconsistent and makes the chronik feel less useful than it could be.

Root cause

Backend — ActivityFeedItemDTO doesn't expose the comment/annotation

ActivityFeedItemDTO.java has no commentId or annotationId fields:

public record ActivityFeedItemDTO(
        AuditKind kind, ActivityActorDTO actor,
        UUID documentId, String documentTitle,
        OffsetDateTime happenedAt,
        boolean youMentioned, boolean youParticipated,
        int count, OffsetDateTime happenedAtUntil
) {}

The audit log payload for COMMENT_ADDED / MENTION_CREATED already carries commentId (populated by CommentService.logCommentPosted), and AuditLogQueryRepository already pulls MIN(s.payload::text)::jsonb AS payload in the aggregated CTE. So commentId is reachable — it's just not surfaced to the DTO.

annotationId is not in the audit payload today. The cheapest fix: extract commentId from the payload in SQL, LEFT JOIN document_comments dc ON dc.id = (payload->>'commentId')::uuid, select dc.annotation_id alongside. After #276's V51 backfill, every block-comment row has annotation_id populated.

Frontend — two places currently build links without these params

• ChronikRow.svelte:102 — always links to bare /documents/{id} regardless of kind.
• ChronikFuerDichBox.svelte:20-22 — includes commentId but omits annotationId.

Proposed change

Backend

1. Add nullable fields to ActivityFeedItemDTO:

   @Nullable UUID commentId,
   @Nullable UUID annotationId,
   

2. Extend ActivityFeedRow projection with getCommentId() / getAnnotationId().
3. Update the SQL in AuditLogQueryRepository.findActivityFeed to extract commentId from the aggregated payload and LEFT JOIN document_comments for the annotation.
4. Update DashboardService.getActivity to pass them through.
5. Regenerate OpenAPI types (npm run generate:api).

Frontend

1. ChronikRow.svelte — when commentId and annotationId are set on the row, build the href with both query params. Keep the bare URL for non-comment kinds (TEXT_SAVED, BLOCK_REVIEWED, etc.).
2. ChronikFuerDichBox.svelte — extend NotificationItem.href() to include &annotationId=... when present (same conditional pattern as NotificationBell.svelte:34-37; can be extracted to a shared helper in $lib/utils/notificationHref.ts so the three call sites stop diverging).

Test coverage

• Backend: extend AuditLogQueryRepositoryIntegrationTest (or a dashboard-service test) to assert that a seeded COMMENT_ADDED audit row produces a DTO with both commentId and annotationId populated.
• Frontend: component tests for ChronikRow.svelte and ChronikFuerDichBox.svelte asserting the href shape for a comment/mention row (with and without annotation id, to cover block comments vs. future cases).
• E2E: extend frontend/e2e/notification-deep-link.spec.ts or add a chronik-specific spec that clicks a mention row and asserts the scroll + flash + focus contract matches the bell flow.

Out of scope

• Backfilling the commentId into audit payloads older than commit edb4e54d — that was already handled by V50__backfill_comment_audit_events.sql.
• Unifying the three link-builders into a single helper is a nice-to-have but not required for this issue.
• Adding a comment-body preview to the chronik row (see the existing TODO at ChronikRow.svelte:154-163 tracked separately).

Acceptance criteria

• Clicking a COMMENT_ADDED or MENTION_CREATED row on /chronik opens the document with transcribe mode + edit panel on, scrolls to the comment, flashes the annotation, and focuses the comment article.
• Clicking a mention in the "Für dich" sidebar does the same.
• Visual / motion policy is identical to the bell dropdown (reduced-motion honoured, block: 'center' scroll, existing flashAnnotationId vocabulary).
• Behaviour at 320 px and 1440 px is verified by E2E.

## What's missing Clicking a mention or comment row on `/chronik` should land the user on the specific comment — same UX as clicking a notification in the bell dropdown (wired up in #276). Today the chronik rows land on `/documents/{id}` with no query params, so the deep-link scroll helper no-ops. ## Current behaviour by entry point | Entry point | Link generated | Result | |---|---|---| | Bell dropdown (`NotificationBell.svelte:34-37`) | `/documents/{id}?commentId=X&annotationId=Y` (both params after #276) | Scrolls to comment, opens edit panel, flashes annotation, focuses article | | Chronik "Für dich" sidebar (`ChronikFuerDichBox.svelte:20-22`) | `/documents/{id}?commentId=X` (only commentId) | No-op — `scrollToCommentFromQuery` bails when `annotationId` is missing | | Chronik main feed row (`ChronikRow.svelte:102`) | `/documents/{id}` (no params at all) | No-op — user has to manually find the comment | ## Why they should match Users don't distinguish between these surfaces in their mental model. A mention is a mention, whether it's surfaced in the bell or the timeline. Three separate UX paths for the same action is inconsistent and makes the chronik feel less useful than it could be. ## Root cause ### Backend — `ActivityFeedItemDTO` doesn't expose the comment/annotation `ActivityFeedItemDTO.java` has no `commentId` or `annotationId` fields: ```java public record ActivityFeedItemDTO( AuditKind kind, ActivityActorDTO actor, UUID documentId, String documentTitle, OffsetDateTime happenedAt, boolean youMentioned, boolean youParticipated, int count, OffsetDateTime happenedAtUntil ) {} ``` The audit log payload for `COMMENT_ADDED` / `MENTION_CREATED` already carries `commentId` (populated by `CommentService.logCommentPosted`), and `AuditLogQueryRepository` already pulls `MIN(s.payload::text)::jsonb AS payload` in the aggregated CTE. So `commentId` is reachable — it's just not surfaced to the DTO. `annotationId` is not in the audit payload today. The cheapest fix: extract `commentId` from the payload in SQL, LEFT JOIN `document_comments dc ON dc.id = (payload->>'commentId')::uuid`, select `dc.annotation_id` alongside. After #276's V51 backfill, every block-comment row has annotation_id populated. ### Frontend — two places currently build links without these params - `ChronikRow.svelte:102` — always links to bare `/documents/{id}` regardless of kind. - `ChronikFuerDichBox.svelte:20-22` — includes `commentId` but omits `annotationId`. ## Proposed change ### Backend 1. Add nullable fields to `ActivityFeedItemDTO`: ```java @Nullable UUID commentId, @Nullable UUID annotationId, ``` 2. Extend `ActivityFeedRow` projection with `getCommentId()` / `getAnnotationId()`. 3. Update the SQL in `AuditLogQueryRepository.findActivityFeed` to extract `commentId` from the aggregated payload and LEFT JOIN `document_comments` for the annotation. 4. Update `DashboardService.getActivity` to pass them through. 5. Regenerate OpenAPI types (`npm run generate:api`). ### Frontend 1. `ChronikRow.svelte` — when `commentId` and `annotationId` are set on the row, build the href with both query params. Keep the bare URL for non-comment kinds (TEXT_SAVED, BLOCK_REVIEWED, etc.). 2. `ChronikFuerDichBox.svelte` — extend `NotificationItem.href()` to include `&annotationId=...` when present (same conditional pattern as `NotificationBell.svelte:34-37`; can be extracted to a shared helper in `$lib/utils/notificationHref.ts` so the three call sites stop diverging). ## Test coverage - Backend: extend `AuditLogQueryRepositoryIntegrationTest` (or a dashboard-service test) to assert that a seeded `COMMENT_ADDED` audit row produces a DTO with both `commentId` and `annotationId` populated. - Frontend: component tests for `ChronikRow.svelte` and `ChronikFuerDichBox.svelte` asserting the href shape for a comment/mention row (with and without annotation id, to cover block comments vs. future cases). - E2E: extend `frontend/e2e/notification-deep-link.spec.ts` or add a chronik-specific spec that clicks a mention row and asserts the scroll + flash + focus contract matches the bell flow. ## Out of scope - Backfilling the `commentId` into audit payloads older than commit `edb4e54d` — that was already handled by `V50__backfill_comment_audit_events.sql`. - Unifying the three link-builders into a single helper is a nice-to-have but not required for this issue. - Adding a comment-body preview to the chronik row (see the existing TODO at `ChronikRow.svelte:154-163` tracked separately). ## Acceptance criteria - Clicking a `COMMENT_ADDED` or `MENTION_CREATED` row on `/chronik` opens the document with transcribe mode + edit panel on, scrolls to the comment, flashes the annotation, and focuses the comment article. - Clicking a mention in the "Für dich" sidebar does the same. - Visual / motion policy is identical to the bell dropdown (reduced-motion honoured, `block: 'center'` scroll, existing `flashAnnotationId` vocabulary). - Behaviour at 320 px and 1440 px is verified by E2E.

marcel added the featurenotificationui labels 2026-04-21 15:49:43 +02:00

marcel commented 2026-04-21 15:54:28 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

• The SQL already extracts ag.payload in the aggregated CTE and uses ag.payload->>'commentId' in the youParticipated subquery (AuditLogQueryRepository.java:99). Exposing it to the projection is a three-line change.
• scrollToCommentFromQuery bails when either commentId or annotationId is missing (deepLinkScroll.ts:19-23). Our fix must populate both — which works for block comments after V51, but DocumentComment.annotationId is null for document-level comments (no block_id). Those mentions will silently no-op even with this fix.
• Three near-identical href builders exist: NotificationBell.svelte:34-37, ChronikFuerDichBox.svelte:20-22, and the future ChronikRow.svelte:102. The issue calls the shared helper "nice-to-have" — I disagree.

Recommendations

• Extract the helper now, not later. Create src/lib/utils/notificationHref.ts:

  export function buildCommentHref(documentId: string, commentId: string, annotationId: string | null): string {
      const base = `/documents/${documentId}?commentId=${commentId}`;
      return annotationId ? `${base}&annotationId=${annotationId}` : base;
  }
  

  Three call sites already diverged — fixing two of them without unifying invites the fourth divergence (mobile bell, SSE push, future email deep-links).
• TDD order:
  1. AuditLogQueryRepositoryIntegrationTest — seed doc + block + document_comments + COMMENT_ADDED audit row, assert DTO has both IDs (extend the pattern at AuditLogQueryRepositoryIntegrationTest.java:64-80).
  2. ChronikRow.svelte.spec.ts — three cases: COMMENT_ADDED with both IDs, COMMENT_ADDED with commentId only, non-comment kind.
  3. ChronikFuerDichBox.svelte.spec.ts — href shape for block-comment vs document-comment mention.
  4. E2E — new chronik-deep-link.spec.ts (do not cram into notification-deep-link.spec.ts — the two surfaces will evolve independently).
• SQL: add LEFT JOIN document_comments dc ON dc.id = (ag.payload->>'commentId')::uuid after the aggregated CTE, in the outer SELECT. Keep the projection minimal: just dc.annotation_id AS annotationId and (ag.payload->>'commentId')::uuid AS commentId.
• DTO: the two new fields are only populated for COMMENT_ADDED / MENTION_CREATED. Mark them @Nullable in both ActivityFeedItemDTO and ActivityFeedRow, and ensure @Schema(requiredMode = NOT_REQUIRED) so the generated TypeScript type is commentId?: string | null.
• Guard against document-level comments: the helper should strip commentId/annotationId params on navigation regardless of whether scroll succeeded. Current helper already does this via opts.onStripUrl() — good, keep it.

Open Decisions

• Rollup behavior: when count > 1 on a COMMENT_ADDED session, which comment do we deep-link to? The MIN(payload::text)::jsonb trick picks the earliest. For rollups, earliest may not be what the user expects (most recent thread activity is usually the interesting one). Options: (a) pick earliest — current behavior, simplest; (b) switch to latest — requires MAX aggregation with timestamp tiebreaker; (c) don't deep-link rollups, bare URL only. Leonie's call.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The SQL already extracts `ag.payload` in the aggregated CTE and uses `ag.payload->>'commentId'` in the `youParticipated` subquery (`AuditLogQueryRepository.java:99`). Exposing it to the projection is a three-line change. - `scrollToCommentFromQuery` bails when **either** `commentId` or `annotationId` is missing (`deepLinkScroll.ts:19-23`). Our fix must populate both — which works for block comments after V51, but `DocumentComment.annotationId` is `null` for **document-level comments** (no `block_id`). Those mentions will silently no-op even with this fix. - Three near-identical href builders exist: `NotificationBell.svelte:34-37`, `ChronikFuerDichBox.svelte:20-22`, and the future `ChronikRow.svelte:102`. The issue calls the shared helper "nice-to-have" — I disagree. ### Recommendations - **Extract the helper now**, not later. Create `src/lib/utils/notificationHref.ts`: ```typescript export function buildCommentHref(documentId: string, commentId: string, annotationId: string | null): string { const base = `/documents/${documentId}?commentId=${commentId}`; return annotationId ? `${base}&annotationId=${annotationId}` : base; } ``` Three call sites already diverged — fixing two of them without unifying invites the fourth divergence (mobile bell, SSE push, future email deep-links). - **TDD order**: 1. `AuditLogQueryRepositoryIntegrationTest` — seed doc + block + `document_comments` + `COMMENT_ADDED` audit row, assert DTO has both IDs (extend the pattern at `AuditLogQueryRepositoryIntegrationTest.java:64-80`). 2. `ChronikRow.svelte.spec.ts` — three cases: COMMENT_ADDED with both IDs, COMMENT_ADDED with commentId only, non-comment kind. 3. `ChronikFuerDichBox.svelte.spec.ts` — href shape for block-comment vs document-comment mention. 4. E2E — new `chronik-deep-link.spec.ts` (do **not** cram into notification-deep-link.spec.ts — the two surfaces will evolve independently). - **SQL**: add `LEFT JOIN document_comments dc ON dc.id = (ag.payload->>'commentId')::uuid` **after** the `aggregated` CTE, in the outer SELECT. Keep the projection minimal: just `dc.annotation_id AS annotationId` and `(ag.payload->>'commentId')::uuid AS commentId`. - **DTO**: the two new fields are only populated for `COMMENT_ADDED` / `MENTION_CREATED`. Mark them `@Nullable` in both `ActivityFeedItemDTO` and `ActivityFeedRow`, and ensure `@Schema(requiredMode = NOT_REQUIRED)` so the generated TypeScript type is `commentId?: string | null`. - **Guard against document-level comments**: the helper should strip `commentId`/`annotationId` params on navigation regardless of whether scroll succeeded. Current helper already does this via `opts.onStripUrl()` — good, keep it. ### Open Decisions - Rollup behavior: when `count > 1` on a COMMENT_ADDED session, which comment do we deep-link to? The `MIN(payload::text)::jsonb` trick picks the earliest. For rollups, earliest may not be what the user expects (most recent thread activity is usually the interesting one). Options: (a) pick earliest — current behavior, simplest; (b) switch to latest — requires `MAX` aggregation with timestamp tiebreaker; (c) don't deep-link rollups, bare URL only. Leonie's call.

marcel commented 2026-04-21 15:54:45 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

Observations

• AuditLogQueryRepository.findRolledUpActivityFeed already crosses a domain boundary: it reads from notifications (line 96) to compute youParticipated. The proposed fix adds a second cross-domain table: document_comments. The query is becoming a cross-cutting projection, not a pure audit query.
• DashboardService.getActivity already post-processes rows — it calls DocumentService.getDocumentsByIds to enrich document titles (DashboardService.java:122). There is a precedent for enriching in Java rather than SQL.
• The feed limit is 40-50 rows. Batch lookups against document_comments PK are ~1ms.

Recommendations

• Enrich in the service, not the SQL. Extend ActivityFeedRow with getCommentId() (from the existing ag.payload) but not getAnnotationId(). Then in DashboardService.getActivity:

  List<UUID> commentIds = rows.stream()
      .map(r -> extractCommentId(r))
      .filter(Objects::nonNull).toList();
  Map<UUID, UUID> annotationByComment = commentService.findAnnotationIdsByIds(commentIds);
  

  This respects the boundary — CommentService owns document_comments, not the audit query. It matches the pattern already in use for document titles.
• Rationale: every time we add a cross-domain JOIN to the audit CTE, we make it harder to extract the audit module later, and we make the query harder to read. The SQL is already 70+ lines of CTE stacking. Push enrichment up to the service layer where domain boundaries are explicit.
• DTO design: the two new fields are semantically coupled to kind ∈ {COMMENT_ADDED, MENTION_CREATED}. Java records don't express this invariant well, but at minimum: add a @Schema.description noting "present only for comment/mention events" so OpenAPI consumers know. Optional sealed-interface refactor is overkill for this project size.
• Migration posture: no schema change needed — V51 already backfilled annotation_id on document_comments. New code reads existing columns. Good.

Open Decisions

• SQL JOIN vs. service-layer enrichment: I'm recommending service-layer. If you prefer the SQL JOIN (fewer round trips, one-query simplicity), the cost is ~20 more lines of stacked CTE that future-you will regret reading. State the preference; I'll defer to project velocity either way.

## 🏛️ Markus Keller — Senior Application Architect ### Observations - `AuditLogQueryRepository.findRolledUpActivityFeed` already crosses a domain boundary: it reads from `notifications` (line 96) to compute `youParticipated`. The proposed fix adds a second cross-domain table: `document_comments`. The query is becoming a cross-cutting projection, not a pure audit query. - `DashboardService.getActivity` already post-processes rows — it calls `DocumentService.getDocumentsByIds` to enrich document titles (`DashboardService.java:122`). There is a precedent for enriching in Java rather than SQL. - The feed limit is 40-50 rows. Batch lookups against `document_comments` PK are ~1ms. ### Recommendations - **Enrich in the service, not the SQL.** Extend `ActivityFeedRow` with `getCommentId()` (from the existing `ag.payload`) but **not** `getAnnotationId()`. Then in `DashboardService.getActivity`: ```java List<UUID> commentIds = rows.stream() .map(r -> extractCommentId(r)) .filter(Objects::nonNull).toList(); Map<UUID, UUID> annotationByComment = commentService.findAnnotationIdsByIds(commentIds); ``` This respects the boundary — `CommentService` owns `document_comments`, not the audit query. It matches the pattern already in use for document titles. - **Rationale**: every time we add a cross-domain JOIN to the audit CTE, we make it harder to extract the audit module later, and we make the query harder to read. The SQL is already 70+ lines of CTE stacking. Push enrichment up to the service layer where domain boundaries are explicit. - **DTO design**: the two new fields are semantically coupled to `kind ∈ {COMMENT_ADDED, MENTION_CREATED}`. Java records don't express this invariant well, but at minimum: add a `@Schema.description` noting "present only for comment/mention events" so OpenAPI consumers know. Optional sealed-interface refactor is overkill for this project size. - **Migration posture**: no schema change needed — V51 already backfilled `annotation_id` on `document_comments`. New code reads existing columns. Good. ### Open Decisions - **SQL JOIN vs. service-layer enrichment**: I'm recommending service-layer. If you prefer the SQL JOIN (fewer round trips, one-query simplicity), the cost is ~20 more lines of stacked CTE that future-you will regret reading. State the preference; I'll defer to project velocity either way.

marcel commented 2026-04-21 15:55:00 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

• No new data exposure. commentId and annotationId are UUIDs; the document title is already in the feed, so the sensitive field (what the comment says) isn't surfaced.
• Pre-existing concern unrelated to this PR: AuditLogQueryRepository.findRolledUpActivityFeed does not filter by the user's document permissions. Any authenticated user sees every document's activity in their feed (title, document ID, now also commentId/annotationId). If permission-scoped documents exist, this is already a broader IDOR exposure — but it's not introduced by this issue, so flagging and leaving scope.
• Query parameterization: the proposed LEFT JOIN uses (ag.payload->>'commentId')::uuid — Postgres does the cast, input is not user-controlled at this point (it's stored audit data). Injection-safe.
• Frontend: url.searchParams.get('commentId') → opts.setActiveAnnotationId(annotationId) → DOM id comment-${commentId} in deepLinkScroll.ts:37. A user-crafted URL with a malicious value lands in a document.getElementById call, which cannot execute code. Safe.

Recommendations

• Add a permission check on the feed endpoint (separate issue, not this one): DashboardService.getActivity should filter out audit rows for documents the current user cannot read. This issue widens the information surface incrementally — if the principle "user can see what they have access to" is supposed to hold, now is a cheap time to file the spinoff. I can draft the issue if useful.
• Validate query parameter format in scrollToCommentFromQuery: even though it's safe today, add a UUID regex check before getElementById — defense in depth in case a future use of the param does reach a sink that matters (e.g. a log line, an API call, HTML attribute construction):

  const UUID_RE = /^[0-9a-f-]{36}$/i;
  if (commentId && !UUID_RE.test(commentId)) return;
  if (annotationId && !UUID_RE.test(annotationId)) return;
  

  Tiny cost, eliminates a class of future footguns.
• Regression test for deep-link with non-existent IDs: add a Playwright case where a user visits /documents/{id}?commentId=<random-uuid>&annotationId=<random-uuid> for a comment that isn't theirs — assert no scroll, no flash, no error surfaced to console. Confirms fail-safe behavior.

Open Decisions (none)

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - No new data exposure. `commentId` and `annotationId` are UUIDs; the document title is already in the feed, so the sensitive field (what the comment *says*) isn't surfaced. - **Pre-existing concern unrelated to this PR**: `AuditLogQueryRepository.findRolledUpActivityFeed` does **not filter by the user's document permissions**. Any authenticated user sees every document's activity in their feed (title, document ID, now also commentId/annotationId). If permission-scoped documents exist, this is already a broader IDOR exposure — but it's not introduced by this issue, so flagging and leaving scope. - Query parameterization: the proposed LEFT JOIN uses `(ag.payload->>'commentId')::uuid` — Postgres does the cast, input is not user-controlled at this point (it's stored audit data). Injection-safe. - Frontend: `url.searchParams.get('commentId')` → `opts.setActiveAnnotationId(annotationId)` → DOM id `comment-${commentId}` in `deepLinkScroll.ts:37`. A user-crafted URL with a malicious value lands in a `document.getElementById` call, which cannot execute code. Safe. ### Recommendations - **Add a permission check on the feed endpoint** (separate issue, not this one): `DashboardService.getActivity` should filter out audit rows for documents the current user cannot read. This issue widens the information surface incrementally — if the principle "user can see what they have access to" is supposed to hold, now is a cheap time to file the spinoff. I can draft the issue if useful. - **Validate query parameter format** in `scrollToCommentFromQuery`: even though it's safe today, add a UUID regex check before `getElementById` — defense in depth in case a future use of the param does reach a sink that matters (e.g. a log line, an API call, HTML attribute construction): ```typescript const UUID_RE = /^[0-9a-f-]{36}$/i; if (commentId && !UUID_RE.test(commentId)) return; if (annotationId && !UUID_RE.test(annotationId)) return; ``` Tiny cost, eliminates a class of future footguns. - **Regression test for deep-link with non-existent IDs**: add a Playwright case where a user visits `/documents/{id}?commentId=<random-uuid>&annotationId=<random-uuid>` for a comment that isn't theirs — assert no scroll, no flash, no error surfaced to console. Confirms fail-safe behavior. ### Open Decisions _(none)_

marcel commented 2026-04-21 15:55:20 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Observations

• Existing test coverage for the feed query is shallow on comment events — AuditLogQueryRepositoryIntegrationTest.findRolledUpActivityFeed_returnsAnnotationEntry only covers ANNOTATION_CREATED, not COMMENT_ADDED. This issue's regression risk is in the SQL, so this is the gap to close first.
• notification-deep-link.spec.ts already proves the bell flow at 320 and 1440 with an axe check. The chronik flow is a parallel surface — its tests should mirror this spec's structure so the two contracts are visible together.
• ChronikRow.svelte.spec.ts and ChronikFuerDichBox.svelte.spec.ts exist — extend them, don't duplicate.

Recommendations

Test strategy, layer by layer:

1. Integration (AuditLogQueryRepositoryIntegrationTest) — one new test per case:

   • findRolledUpActivityFeed_returns_commentId_and_annotationId_for_block_comment: seed users + doc + transcription_blocks + document_comments with annotation_id set + audit_log COMMENT_ADDED with commentId payload → assert projection fields populated.
   • findRolledUpActivityFeed_returns_null_annotationId_for_document_level_comment: same but document_comments.annotation_id = NULL → assert commentId populated, annotationId null.
   • findRolledUpActivityFeed_returns_null_both_for_non_comment_kind: existing ANNOTATION_CREATED test extended to assert the two new fields are null.

2. Component (ChronikRow.svelte.spec.ts):

   • COMMENT_ADDED + both IDs → href includes both params.
   • COMMENT_ADDED + commentId only → href has only commentId.
   • TEXT_SAVED → bare /documents/{id}.
   • Rollup with count=3 → whatever Leonie decides (see my open decision).

3. Component (ChronikFuerDichBox.svelte.spec.ts):

   • Mention with annotationId → both params.
   • Mention without annotationId → commentId only.

4. E2E (frontend/e2e/chronik-deep-link.spec.ts) — new file, structure mirroring notification-deep-link.spec.ts:

   • beforeAll: create doc + block + post block comment → chronik row exists.
   • Test at 320px and 1440px: visit /chronik, click the row, assert transcribe mode entered, comment visible, URL stripped, flash visible (or instant with reduced-motion).
   • Separate axe test on /chronik and on the deep-linked document view.

5. Do NOT extend notification-deep-link.spec.ts with chronik cases — the two surfaces have separate failure modes (bell uses notificationStore.markRead, chronik uses plain <a href>). Shared fixtures, separate specs.

• Flaky-test prevention: the chronik row is SSR'd — the test must wait for [data-hydrated] on the document page after navigation, same as the existing spec. Don't use Thread.sleep-equivalent await page.waitForTimeout.
• Permission boundary test (Nora's point): a Playwright test as a user without access to the target document — visiting the deep-link URL should not leak comment presence via DOM/scroll behavior.

Open Decisions

• Rollup click behavior: if ChronikRow shows comment added × 3, a single click can only go to one comment. Which one dictates what the backend's rollup aggregation emits. Sara's test depends on Leonie's UX decision (Felix also flagged this).

## 🧪 Sara Holt — Senior QA Engineer ### Observations - Existing test coverage for the feed query is shallow on comment events — `AuditLogQueryRepositoryIntegrationTest.findRolledUpActivityFeed_returnsAnnotationEntry` only covers `ANNOTATION_CREATED`, not `COMMENT_ADDED`. This issue's regression risk is in the SQL, so this is the gap to close first. - `notification-deep-link.spec.ts` already proves the bell flow at 320 and 1440 with an axe check. The chronik flow is a parallel surface — its tests should mirror this spec's structure so the two contracts are visible together. - `ChronikRow.svelte.spec.ts` and `ChronikFuerDichBox.svelte.spec.ts` exist — extend them, don't duplicate. ### Recommendations **Test strategy, layer by layer:** 1. **Integration (`AuditLogQueryRepositoryIntegrationTest`)** — one new test per case: - `findRolledUpActivityFeed_returns_commentId_and_annotationId_for_block_comment`: seed users + doc + `transcription_blocks` + `document_comments` with `annotation_id` set + `audit_log COMMENT_ADDED` with `commentId` payload → assert projection fields populated. - `findRolledUpActivityFeed_returns_null_annotationId_for_document_level_comment`: same but `document_comments.annotation_id = NULL` → assert `commentId` populated, `annotationId` null. - `findRolledUpActivityFeed_returns_null_both_for_non_comment_kind`: existing `ANNOTATION_CREATED` test extended to assert the two new fields are null. 2. **Component (`ChronikRow.svelte.spec.ts`)**: - COMMENT_ADDED + both IDs → href includes both params. - COMMENT_ADDED + commentId only → href has only `commentId`. - TEXT_SAVED → bare `/documents/{id}`. - Rollup with count=3 → whatever Leonie decides (see my open decision). 3. **Component (`ChronikFuerDichBox.svelte.spec.ts`)**: - Mention with annotationId → both params. - Mention without annotationId → commentId only. 4. **E2E (`frontend/e2e/chronik-deep-link.spec.ts`)** — new file, structure mirroring `notification-deep-link.spec.ts`: - `beforeAll`: create doc + block + post block comment → chronik row exists. - Test at 320px and 1440px: visit `/chronik`, click the row, assert transcribe mode entered, comment visible, URL stripped, flash visible (or instant with reduced-motion). - Separate axe test on `/chronik` and on the deep-linked document view. 5. **Do NOT extend `notification-deep-link.spec.ts`** with chronik cases — the two surfaces have separate failure modes (bell uses `notificationStore.markRead`, chronik uses plain `<a href>`). Shared fixtures, separate specs. - **Flaky-test prevention**: the chronik row is SSR'd — the test must wait for `[data-hydrated]` on the document page after navigation, same as the existing spec. Don't use `Thread.sleep`-equivalent `await page.waitForTimeout`. - **Permission boundary test (Nora's point)**: a Playwright test as a user *without* access to the target document — visiting the deep-link URL should not leak comment presence via DOM/scroll behavior. ### Open Decisions - **Rollup click behavior**: if ChronikRow shows `comment added × 3`, a single click can only go to one comment. Which one dictates what the backend's rollup aggregation emits. Sara's test depends on Leonie's UX decision (Felix also flagged this).

marcel commented 2026-04-21 15:55:43 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

• The issue correctly names the user-mental-model problem: a mention is a mention, surface-agnostic. Three different outcomes for the same conceptual action is friction, not feature.
• The existing bell flow's contract is strong: scroll with block: 'center', focus({preventScroll: true}), flash the annotation, strip URL. The issue explicitly requires parity — good.
• ChronikRow.svelte currently wraps the entire row in an <a> — keyboard Enter will trigger navigation the same as click, which is correct for deep-links. ✅
• Blind spot: block comments with annotation_id = NULL (document-level comments) will arrive with commentId only. The helper bails at deepLinkScroll.ts:21-23. From the user's perspective: they click the row, land on the document page, nothing happens. That is a worse outcome than landing on the bare document.

Recommendations

• Fix scrollToCommentFromQuery to degrade gracefully for commentId-only (no annotationId):
  • Enter transcribe mode + edit panel (still needed — comments render in edit view).
  • Skip setActiveAnnotationId (no annotation to activate).
  • Scroll to comment-{id}, focus, no flash (no annotation to flash).
  • Without this, the feature silently fails for any document-level mention — inconsistent with the premise of the issue.
• Rollup click behavior (answering the open question): for a rollup with count > 1, link to the most recent comment in the session. Rationale: if someone added 3 comments over 30 minutes, the user's mental model after seeing the chronik row is "what's new here" — latest is newest. Technically this means MAX(happened_at) in the aggregation, with the corresponding payload lookup. If that's too complex, fall back to option (c): no deep-link on rollups, bare URL only. Do not pick earliest — it's the stalest possible choice.
• Focus management: on arrival, focus the comment article as the bell flow does. For screen reader users, the <article> should have an accessible name (e.g. aria-label="Kommentar von Max · 14:23") so arriving via focus makes sense aurally. Verify comment-{id} element has this.
• Reduced motion: confirmed the bell flow uses 'instant' scroll + skips flash animation. Chronik flow must do the same. The helper already reads prefersReducedMotion — just wire it through on the chronik entry path.
• 44px target at 320px: the chronik row is ~60px tall with a 40×40 avatar + 8px padding; hit area is the whole <a> so this is fine. ✅
• Visual cue consistency: when the deep-linked comment flashes, the same flashAnnotationId mechanism must trigger on the annotation in the PDF and highlight the comment itself via focus-visible ring. The bell currently flashes the annotation; verify the comment article also gets a brief visual cue on arrival (focus ring is likely sufficient).
• Do NOT introduce a new animation style. Reuse the existing flash (1500ms / 2000ms reduced-motion). Three surfaces with three animation styles would be worse than zero deep-link.

Open Decisions

• Rollup link target: see above — my recommendation is "most recent comment", fallback "no deep-link on rollup". Felix and Sara both flagged this as needing a call.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - The issue correctly names the user-mental-model problem: a mention is a mention, surface-agnostic. Three different outcomes for the same conceptual action is friction, not feature. - The existing bell flow's contract is strong: scroll with `block: 'center'`, `focus({preventScroll: true})`, flash the annotation, strip URL. The issue explicitly requires parity — good. - `ChronikRow.svelte` currently wraps the entire row in an `<a>` — keyboard Enter will trigger navigation the same as click, which is correct for deep-links. ✅ - **Blind spot**: block comments with `annotation_id = NULL` (document-level comments) will arrive with commentId only. The helper bails at `deepLinkScroll.ts:21-23`. From the user's perspective: they click the row, land on the document page, **nothing happens**. That is a worse outcome than landing on the bare document. ### Recommendations - **Fix `scrollToCommentFromQuery` to degrade gracefully for commentId-only** (no annotationId): - Enter transcribe mode + edit panel (still needed — comments render in edit view). - Skip `setActiveAnnotationId` (no annotation to activate). - Scroll to `comment-{id}`, focus, **no flash** (no annotation to flash). - Without this, the feature silently fails for any document-level mention — inconsistent with the premise of the issue. - **Rollup click behavior (answering the open question)**: for a rollup with `count > 1`, link to the **most recent** comment in the session. Rationale: if someone added 3 comments over 30 minutes, the user's mental model after seeing the chronik row is "what's new here" — latest is newest. Technically this means `MAX(happened_at)` in the aggregation, with the corresponding payload lookup. If that's too complex, fall back to option (c): no deep-link on rollups, bare URL only. **Do not pick earliest** — it's the stalest possible choice. - **Focus management**: on arrival, focus the comment article as the bell flow does. For screen reader users, the `<article>` should have an accessible name (e.g. `aria-label="Kommentar von Max · 14:23"`) so arriving via focus makes sense aurally. Verify `comment-{id}` element has this. - **Reduced motion**: confirmed the bell flow uses `'instant'` scroll + skips flash animation. Chronik flow must do the same. The helper already reads `prefersReducedMotion` — just wire it through on the chronik entry path. - **44px target at 320px**: the chronik row is ~60px tall with a 40×40 avatar + 8px padding; hit area is the whole `<a>` so this is fine. ✅ - **Visual cue consistency**: when the deep-linked comment flashes, the same `flashAnnotationId` mechanism must trigger on the annotation in the PDF *and* highlight the comment itself via `focus-visible` ring. The bell currently flashes the annotation; verify the comment article also gets a brief visual cue on arrival (focus ring is likely sufficient). - **Do NOT** introduce a new animation style. Reuse the existing flash (1500ms / 2000ms reduced-motion). Three surfaces with three animation styles would be worse than zero deep-link. ### Open Decisions - **Rollup link target**: see above — my recommendation is "most recent comment", fallback "no deep-link on rollup". Felix and Sara both flagged this as needing a call.

marcel commented 2026-04-21 15:55:57 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

• No infrastructure changes. No new services, no new containers, no migrations (V51 already did the backfill).
• The feed endpoint runs the existing CTE-heavy query; adding a LEFT JOIN on document_comments adds ~40 indexed PK lookups for a 40-row feed. Negligible on the current VPS (CX32, 4 vCPU).
• Frontend E2E (frontend/e2e/) is currently not in CI per frontend/e2e/CLAUDE.md. The new chronik-deep-link spec will only run locally unless CI is extended.

Recommendations

• No CI changes required for this PR, but flag: every new E2E spec widens the gap between "what a developer can verify locally" and "what merges through without human checks". If we keep adding E2E without running them in CI, they're decorative.
  • Recommendation: file a separate issue to add a Playwright job to infra/gitea/workflows/ci.yml (uses Docker Compose, runs on merge). Scope this issue to just the two specs.
• OpenAPI regeneration is the one workflow gotcha: npm run generate:api requires backend running with --spring.profiles.active=dev. Mention this explicitly in the PR checklist so the TypeScript types in frontend/src/lib/generated/api.ts are committed — easy to miss when working in the backend-first part of the plan.
• Observability: the activity feed query doesn't currently emit p95 latency metrics. Worth adding @Timed on DashboardController.getActivity if not already present — once the LEFT JOIN lands, we have a baseline. Same separate-issue applies if wanted.
• Frontend bundle impact: one extracted helper (notificationHref.ts) is ~15 lines gzipped. Zero concern.
• Rollback posture: purely additive change — new DTO fields are @Nullable, frontend falls back to the bare URL when they're null. If the backend rolls back while the frontend is updated, old JSON doesn't include the fields, undefined flows through, bare URL is built. Forward-compatible. ✅

Open Decisions (none)

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - No infrastructure changes. No new services, no new containers, no migrations (V51 already did the backfill). - The feed endpoint runs the existing CTE-heavy query; adding a LEFT JOIN on `document_comments` adds ~40 indexed PK lookups for a 40-row feed. Negligible on the current VPS (CX32, 4 vCPU). - Frontend E2E (`frontend/e2e/`) is currently **not in CI** per `frontend/e2e/CLAUDE.md`. The new chronik-deep-link spec will only run locally unless CI is extended. ### Recommendations - **No CI changes required for this PR**, but flag: every new E2E spec widens the gap between "what a developer can verify locally" and "what merges through without human checks". If we keep adding E2E without running them in CI, they're decorative. - Recommendation: file a separate issue to add a Playwright job to `infra/gitea/workflows/ci.yml` (uses Docker Compose, runs on merge). Scope this issue to just the two specs. - **OpenAPI regeneration** is the one workflow gotcha: `npm run generate:api` requires backend running with `--spring.profiles.active=dev`. Mention this explicitly in the PR checklist so the TypeScript types in `frontend/src/lib/generated/api.ts` are committed — easy to miss when working in the backend-first part of the plan. - **Observability**: the activity feed query doesn't currently emit p95 latency metrics. Worth adding `@Timed` on `DashboardController.getActivity` if not already present — once the LEFT JOIN lands, we have a baseline. Same separate-issue applies if wanted. - **Frontend bundle impact**: one extracted helper (`notificationHref.ts`) is ~15 lines gzipped. Zero concern. - **Rollback posture**: purely additive change — new DTO fields are `@Nullable`, frontend falls back to the bare URL when they're null. If the backend rolls back while the frontend is updated, old JSON doesn't include the fields, `undefined` flows through, bare URL is built. Forward-compatible. ✅ ### Open Decisions _(none)_

marcel commented 2026-04-21 15:56:21 +02:00

Author

Owner

Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Architecture

• Where to enrich annotationId: SQL JOIN vs. service-layer lookup — Option A: add LEFT JOIN document_comments dc ON dc.id = (ag.payload->>'commentId')::uuid inside AuditLogQueryRepository.findRolledUpActivityFeed and select dc.annotation_id. Cheap (one query), but stacks another cross-domain join onto a CTE that already references notifications and users. Option B: return just commentId from SQL, then in DashboardService.getActivity batch-fetch annotation IDs via a new CommentService.findAnnotationIdsByIds(commentIds) method. Respects the module boundary (CommentService owns document_comments), matches the existing documentService.getDocumentsByIds pattern. Adds one round-trip (~1ms for PK lookups). (Raised by: Markus — recommends B)

UX

• Rollup deep-link target — when ChronikRow shows comment added × 3, a single click can only link to one comment. Options: (a) earliest in session — what the current MIN(payload::text)::jsonb aggregation happens to produce, zero extra work, but shows the stalest choice; (b) latest in session — requires changing the aggregation to pick the MAX-timestamped row's payload, matches "what's new here" user intent; (c) no deep-link on rollup — bare /documents/{id}, user navigates manually. Leonie recommends (b), fallback (c); Felix and Sara are blocked on this for their TDD plan. (Raised by: Felix, Sara, Leonie)

## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Architecture - **Where to enrich `annotationId`: SQL JOIN vs. service-layer lookup** — Option A: add `LEFT JOIN document_comments dc ON dc.id = (ag.payload->>'commentId')::uuid` inside `AuditLogQueryRepository.findRolledUpActivityFeed` and select `dc.annotation_id`. Cheap (one query), but stacks another cross-domain join onto a CTE that already references `notifications` and `users`. Option B: return just `commentId` from SQL, then in `DashboardService.getActivity` batch-fetch annotation IDs via a new `CommentService.findAnnotationIdsByIds(commentIds)` method. Respects the module boundary (`CommentService` owns `document_comments`), matches the existing `documentService.getDocumentsByIds` pattern. Adds one round-trip (~1ms for PK lookups). _(Raised by: Markus — recommends B)_ ### UX - **Rollup deep-link target** — when ChronikRow shows `comment added × 3`, a single click can only link to one comment. Options: (a) earliest in session — what the current `MIN(payload::text)::jsonb` aggregation happens to produce, zero extra work, but shows the stalest choice; (b) latest in session — requires changing the aggregation to pick the MAX-timestamped row's payload, matches "what's new here" user intent; (c) no deep-link on rollup — bare `/documents/{id}`, user navigates manually. Leonie recommends (b), fallback (c); Felix and Sara are blocked on this for their TDD plan. _(Raised by: Felix, Sara, Leonie)_

marcel commented 2026-04-21 16:46:53 +02:00

Author

Owner

Copy Link

🎯 Discussion Resolutions

After reviewing the persona feedback with the user, these are the authoritative decisions. The implement skill should treat this comment as the design source alongside the original issue.

Theme 1 — Enrichment approach

• Decision: Service-layer enrichment. ActivityFeedRow gains only getCommentId() (extracted from the existing ag.payload). A new CommentService.findAnnotationIdsByIds(List<UUID>) batch-loads annotation IDs. DashboardService.getActivity applies the lookup to each row.
• Rationale: Keeps AuditLogQueryRepository free of another cross-domain JOIN. Matches the existing DocumentService.getDocumentsByIds pattern already used for titles. ~1ms overhead (PK lookups) on a 40-row feed.

Theme 2 — Rollup deep-link target

• Decision: Latest comment in session. SQL aggregation changes from MIN(payload::text)::jsonb to FIRST_VALUE(payload) OVER (PARTITION BY ... ORDER BY happened_at DESC) so the surfaced commentId is the newest event in the rollup.
• Rationale: User's mental model when they see a rollup is "what's new here" — newest is the closest fit. Earliest would be the stalest possible choice.

Theme 3 — Document-level comments (annotation_id = NULL)

• Decision: Not handled. scrollToCommentFromQuery continues to require both commentId and annotationId.
• Rationale: CommentController only exposes block-comment and reply endpoints; both service methods always set annotationId; V51 backfilled historical rows. There are no code paths that produce annotation_id = NULL. Tightening DocumentComment.annotationId to NOT NULL is a separate concern, not this PR.

Theme 4 — Shared href helper

• Decision: Extract src/lib/utils/commentDeepLink.ts:

  export function buildCommentHref(documentId: string, commentId: string, annotationId: string | null): string {
      const base = `/documents/${documentId}?commentId=${commentId}`;
      return annotationId ? `${base}&annotationId=${annotationId}` : base;
  }
  

  Used by NotificationBell.svelte, ChronikFuerDichBox.svelte, and ChronikRow.svelte. Gets its own unit test.
• Rationale: Three divergent builders caused this bug. Unifying two of three leaves the third divergence waiting to happen.

Theme 5 — DTO shape

• Decision: ActivityFeedItemDTO gains:

  @Nullable
  @Schema(
      requiredMode = Schema.RequiredMode.NOT_REQUIRED,
      description = "Deep-link target comment; populated only for COMMENT_ADDED and MENTION_CREATED kinds."
  )
  UUID commentId,
  // same for annotationId
  

  ActivityFeedRow gains only getCommentId(); annotationId is resolved in Java. PR checklist includes npm run generate:api (backend must run with --spring.profiles.active=dev).

Theme 6 — Test strategy

• Integration tests (in AuditLogQueryRepositoryIntegrationTest):
  1. findRolledUpActivityFeed_returns_commentId_for_COMMENT_ADDED — seed doc + block + document_comments + COMMENT_ADDED audit row → assert commentId populated.
  2. findRolledUpActivityFeed_returns_null_commentId_for_non_comment_kind — extend the existing ANNOTATION_CREATED case to assert commentId null.
  3. findRolledUpActivityFeed_rollup_commentId_points_to_latest_event_in_session — seed three COMMENT_ADDED rows in one session → assert the rolled-up row's commentId matches the most recent event.
• Service test: new CommentService.findAnnotationIdsByIds unit test (happy path + empty input + unknown IDs).
• Component tests:
  • commentDeepLink.spec.ts (new) — helper branching.
  • ChronikRow.svelte.spec.ts — three cases: COMMENT_ADDED (both params), TEXT_SAVED (bare), rollup with count>1 (points to latest-event IDs).
  • ChronikFuerDichBox.svelte.spec.ts — one case: mention with both IDs → both params.
• E2E (new frontend/e2e/chronik-deep-link.spec.ts):
  • beforeAll: seed doc + block + block comment.
  • At 320px and 1440px: click the chronik row, assert transcribe mode entered, #comment-{id} visible, URL params stripped.
  • Fail-safe case: random UUIDs → no scroll, no console error.
  • Axe check at both viewports.
  • Wait for [data-hydrated]. No waitForTimeout.
  • Do not extend notification-deep-link.spec.ts.
• Permission-boundary test: not needed — all authenticated users can read all documents in this app.

Theme 7 — Accessibility & animation parity

• Decision: Parity with bell flow only. Reuse the existing 1500ms / 2000ms flash, use prefersReducedMotion for instant scroll, no new animation styles, no aria-label additions. 44px touch target already met by the full-row <a> on ChronikRow.

Theme 8 — Security hardening

• Decision: No UUID regex guard in scrollToCommentFromQuery. getElementById is a safe sink today; defensive validation at the source drifts out of sync with actual threats. Validate at new sinks when/if they're introduced.

Theme 9 — Spinoff issues

• Decision: None filed. Nora's feed-IDOR concern is moot (all users read all documents by design). Tobi's items (E2E in CI, @Timed on feed endpoint) are logged in the review thread for future reference but not urgent enough to file.

---

These resolutions are the authoritative design for implementation. The implement skill will read this alongside the original issue.

# 🎯 Discussion Resolutions After reviewing the persona feedback with the user, these are the authoritative decisions. The `implement` skill should treat this comment as the design source alongside the original issue. ## Theme 1 — Enrichment approach - **Decision**: Service-layer enrichment. `ActivityFeedRow` gains only `getCommentId()` (extracted from the existing `ag.payload`). A new `CommentService.findAnnotationIdsByIds(List<UUID>)` batch-loads annotation IDs. `DashboardService.getActivity` applies the lookup to each row. - **Rationale**: Keeps `AuditLogQueryRepository` free of another cross-domain JOIN. Matches the existing `DocumentService.getDocumentsByIds` pattern already used for titles. ~1ms overhead (PK lookups) on a 40-row feed. ## Theme 2 — Rollup deep-link target - **Decision**: Latest comment in session. SQL aggregation changes from `MIN(payload::text)::jsonb` to `FIRST_VALUE(payload) OVER (PARTITION BY ... ORDER BY happened_at DESC)` so the surfaced `commentId` is the newest event in the rollup. - **Rationale**: User's mental model when they see a rollup is "what's new here" — newest is the closest fit. Earliest would be the stalest possible choice. ## Theme 3 — Document-level comments (annotation_id = NULL) - **Decision**: Not handled. `scrollToCommentFromQuery` continues to require both `commentId` and `annotationId`. - **Rationale**: `CommentController` only exposes block-comment and reply endpoints; both service methods always set `annotationId`; V51 backfilled historical rows. There are no code paths that produce `annotation_id = NULL`. Tightening `DocumentComment.annotationId` to NOT NULL is a separate concern, not this PR. ## Theme 4 — Shared href helper - **Decision**: Extract `src/lib/utils/commentDeepLink.ts`: ```typescript export function buildCommentHref(documentId: string, commentId: string, annotationId: string | null): string { const base = `/documents/${documentId}?commentId=${commentId}`; return annotationId ? `${base}&annotationId=${annotationId}` : base; } ``` Used by `NotificationBell.svelte`, `ChronikFuerDichBox.svelte`, and `ChronikRow.svelte`. Gets its own unit test. - **Rationale**: Three divergent builders caused this bug. Unifying two of three leaves the third divergence waiting to happen. ## Theme 5 — DTO shape - **Decision**: `ActivityFeedItemDTO` gains: ```java @Nullable @Schema( requiredMode = Schema.RequiredMode.NOT_REQUIRED, description = "Deep-link target comment; populated only for COMMENT_ADDED and MENTION_CREATED kinds." ) UUID commentId, // same for annotationId ``` `ActivityFeedRow` gains only `getCommentId()`; `annotationId` is resolved in Java. PR checklist includes `npm run generate:api` (backend must run with `--spring.profiles.active=dev`). ## Theme 6 — Test strategy - **Integration tests** (in `AuditLogQueryRepositoryIntegrationTest`): 1. `findRolledUpActivityFeed_returns_commentId_for_COMMENT_ADDED` — seed doc + block + `document_comments` + COMMENT_ADDED audit row → assert `commentId` populated. 2. `findRolledUpActivityFeed_returns_null_commentId_for_non_comment_kind` — extend the existing ANNOTATION_CREATED case to assert `commentId` null. 3. `findRolledUpActivityFeed_rollup_commentId_points_to_latest_event_in_session` — seed three COMMENT_ADDED rows in one session → assert the rolled-up row's `commentId` matches the most recent event. - **Service test**: new `CommentService.findAnnotationIdsByIds` unit test (happy path + empty input + unknown IDs). - **Component tests**: - `commentDeepLink.spec.ts` (new) — helper branching. - `ChronikRow.svelte.spec.ts` — three cases: COMMENT_ADDED (both params), TEXT_SAVED (bare), rollup with count>1 (points to latest-event IDs). - `ChronikFuerDichBox.svelte.spec.ts` — one case: mention with both IDs → both params. - **E2E** (new `frontend/e2e/chronik-deep-link.spec.ts`): - `beforeAll`: seed doc + block + block comment. - At 320px and 1440px: click the chronik row, assert transcribe mode entered, `#comment-{id}` visible, URL params stripped. - Fail-safe case: random UUIDs → no scroll, no console error. - Axe check at both viewports. - Wait for `[data-hydrated]`. No `waitForTimeout`. - Do **not** extend `notification-deep-link.spec.ts`. - **Permission-boundary test**: not needed — all authenticated users can read all documents in this app. ## Theme 7 — Accessibility & animation parity - **Decision**: Parity with bell flow only. Reuse the existing 1500ms / 2000ms flash, use `prefersReducedMotion` for instant scroll, no new animation styles, no aria-label additions. 44px touch target already met by the full-row `<a>` on ChronikRow. ## Theme 8 — Security hardening - **Decision**: No UUID regex guard in `scrollToCommentFromQuery`. `getElementById` is a safe sink today; defensive validation at the source drifts out of sync with actual threats. Validate at new sinks when/if they're introduced. ## Theme 9 — Spinoff issues - **Decision**: None filed. Nora's feed-IDOR concern is moot (all users read all documents by design). Tobi's items (E2E in CI, `@Timed` on feed endpoint) are logged in the review thread for future reference but not urgent enough to file. --- These resolutions are the authoritative design for implementation. The `implement` skill will read this alongside the original issue.

marcel referenced this issue from a commit 2026-04-21 17:46:23 +02:00

feat(audit): expose commentId on rolled-up activity feed projection

marcel referenced this issue from a commit 2026-04-21 17:46:24 +02:00

feat(comment): add findAnnotationIdsByIds batch lookup

marcel referenced this issue from a commit 2026-04-21 17:46:24 +02:00

feat(dashboard): enrich activity feed DTO with commentId + annotationId

marcel referenced this issue from a commit 2026-04-21 17:46:24 +02:00

chore(api): hand-edit generated types for commentId + annotationId

marcel referenced this issue from a commit 2026-04-21 17:46:24 +02:00

feat(utils): add buildCommentHref helper for comment deep-links

marcel referenced this issue from a commit 2026-04-21 17:46:24 +02:00

refactor(notification-bell): use buildCommentHref helper

marcel referenced this issue from a commit 2026-04-21 17:46:25 +02:00

feat(chronik-fuer-dich): include annotationId in mention deep-link

marcel referenced a pull request that will close this issue 2026-04-21 17:46:46 +02:00

feat(chronik): deep-link mentions and comments to the specific comment #301

marcel closed this issue 2026-04-21 19:06:20 +02:00

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feat/issue-533-mass-import-ux

feat/issue-527-unsaved-warning-new-pages

worktree-feat+issue-557-upload-artifact-v3-pin

worktree-chore+issue-556-drop-client-branches-coverage-gate

fix/issue-514-prerender-crawl-bakes-redirects

fix/issue-472-prerender-entries

feat/issue-395-readme

feat/issue-345-bulk-mark-reviewed

feat/issue-344-bell-tooltip

feat/issue-341-annotieren-contrast

feat/issue-225-bulk-metadata-edit

feat/issue-317-bulk-upload

feat/issue-271-dashboard-redesign

docs/issue-240-mission-control-spec

refactor/issues-193-200

feat/issue-162-korrespondenz-redesign

feature/68-new-document-file-first

feat/81-discussion-discoverability

feat/62-document-bottom-panel

feature/56-backfill-file-hashes

feat/38-document-edit-history

fix/svelte5-test-delegation-and-login-auth

No results found.

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 feature notification ui

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#300