bug: notification deep-link does not scroll to comment on document detail page #299
Reference in New Issue
Block a user
Delete Branch "feat/issue-276-notification-deep-link-scroll"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #276
Summary
Fixes the broken notification deep-link flow by populating
annotationIdat the upstream write path (CommentService.postBlockComment) and consuming?commentId=&annotationId=indocuments/[id]/+page.sveltevia a newscrollToCommentFromQueryhelper. Bundles dead-code cleanup of the unused document-level and annotation-level comment APIs — only block comments survive in the frontend today.See the Discussion Resolutions comment on #276 for the authoritative spec agreed during pre-implementation review.
Commits
46588522— fix(comment): populate annotationId on block comments from the block13732ab9— fix(db): V51 backfills annotation_id on block comments and notificationsbc69e8ff— refactor(comment): drop dead document and annotation comment APIs251eb9c3— feat(frontend): add scrollToCommentFromQuery helper for notification deep-link20ae85f8— feat(comment): expose comment id + focus ring on CommentMessage wrappere22265f5— feat(document-detail): wire notification deep-link scroll in onMount567faee3— test(e2e): notification deep-link scrolls to target commentTest matrix
./mvnw test)./mvnw clean package -DskipTests)npm run test)Notes for reviewers
GET /api/documents/{id}/comments/{commentId}— we dropped that in favour of fixing the upstream nullableannotationIdand driving the frontend purely from URL params + DOM id as truth.document_comments.annotation_idandnotifications.annotation_idso previously issued notifications also deep-link correctly after this lands.prefers-reduced-motionand the existingflashAnnotationIdpattern are reused — no new visual vocabulary.CommentMessagewrapper gainsid,tabindex="-1", and afocus-visiblering so keyboard and screen-reader users land on the specific comment; mouse users see nothing extra.Notification deep-link scroll targets #comment-{id}. Add the id to the article wrapper along with tabindex="-1" so scrollIntoView + .focus({preventScroll:true}) can land screen-reader and keyboard focus on the specific comment. A focus-visible ring appears only for keyboard users so mouse clicks don't trigger a visible outline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Seeds a document, transcription block, and block comment via API, then visits /documents/{id}?commentId=X&annotationId=Y and asserts the page enters transcribe mode, the comment article becomes visible, and the URL query params are stripped. Runs at 320px and 1440px so the collapsed PDF strip clipping on mobile is caught. An axe-core pass guards the new tabindex + focus-visible ring against a11y regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The PR is clean TDD work — every commit is atomic, every behavior tested, every rename carries intent. Upstream fix + backfill + dead-code cleanup is exactly the shape I hoped for after the Discussion Resolutions. Eight tests on
scrollToCommentFromQuerycover the matrix, and the CommentMessage changes have focused component tests. Two small concerns below, none blocking.Blockers
(none)
Suggestions
+page.svelteline 319 — defensive default forpage.state. On first navigationpage.stateis typically{}but can beundefinedon older browsers or certain redirect paths.replaceState(page.url.pathname, page.state ?? {})is a zero-cost belt-and-braces. Two chars.+page.sveltelines 314-317 — readableafterTick. The inlineawait new Promise<void>((resolve) => requestAnimationFrame(() => resolve()))is fine but dense. A tiny helperwaitForNextFrame()next to the helper makes the intent obvious. Not worth a dedicated refactor unless you're extracting more timing utilities.+page.svelteline 304 — fire-and-forget.scrollToCommentFromQuery(...)returns aPromisewe don'tawait. That's intentional (onMount is sync), but if the helper ever throws (e.g.getElementcallback crashes), the error is swallowed. Wrapping with.catch((e) => console.error('deep-link scroll failed', e))surfaces it without changing behavior.deepLinkScroll.ts— contract could assertdocumentIdmatches. Not a concern now (we read the URL and trust the router), but aexpectedDocumentIdoption that compares against the URL path would make the helper safer against cross-document URL shenanigans. Filing idea for future; no action this PR.Good things worth noting
stubSaveAssigningRandomId()helper inCommentServiceTest.javais reused cleanly across the newpostBlockComment_*tests.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
The chosen path is structurally right — upstream fix over scaffolding endpoint, with the data backfill to match. Layering is respected:
CommentServicecallsTranscriptionService.getBlockrather than reaching intoTranscriptionBlockRepository, per the project's strict rule. The dead-code cleanup shrinks the surface area to what the frontend actually uses, which is what a codebase of this size deserves.Observations
CommentServicenow has five injected dependencies (commentRepository,userService,notificationService,auditService,transcriptionService). That's at the upper edge of comfortable — if one more lands it's time to split by concern. Not today.UPDATEstatements are idempotent and scoped withIS NULLguards. Rerun-safe if the migration gets re-executed in a shadow environment, and the Flyway checksum protects against drift.scrollToCommentFromQueryis a pure function in$lib/utils/with all side effects injected. That's the right place for it; keeps the page component orchestration-only.Suggestions
DocumentComment.annotationIdfield now has a single semantic meaning ("the block's annotation for block comments"). Worth a one-line comment on the field or a follow-up cleanup that renames it toblockAnnotationId— but only if someone else is touching that entity for another reason. Don't do it here.🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No new vulnerabilities introduced by this PR. Confirmed the family-internal auth model per the Discussion Resolutions: every authenticated user can read block comments. The deep-link URL carries
?commentId=&annotationId=(both UUIDs) — anyone opening the URL sees exactly what they'd see by navigating normally, which is the intended posture.What I checked
GET /api/documents/{id}/comments/{commentId}was dropped — no additional attack surface.page.url.searchParams.get('commentId')returns a raw string that gets concatenated into#comment-{id}and into the element id attribute onCommentMessage.svelte. Template-literal concatenation only, noinnerHTMLoreval, no XSS vector.document.getElementById(comment-${commentId})— ifcommentIdis malformed it simply fails to match and the helper no-ops. No crash, no leak.CommentService.postBlockComment— now callstranscriptionService.getBlock(documentId, blockId)which already enforces that the block belongs to the document (seeTranscriptionService.java:50-55,findByIdAndDocumentId). Previously this was unchecked: a user could post a block comment with a mismatched{documentId, blockId}pair because the comment stored both independently. This PR closes a latent IDOR on the write path. Worth calling out positively.Pre-existing observations (not this PR's concern, filing for awareness)
getBlockCommentsatCommentController.java:30uses@PathVariable UUID blockIdand ignoresdocumentId. Same inreplyToBlockComment. Pre-existing and out of scope here; if you'd like I can open a separate issue for document↔block match on reads. Low severity given the trust model.Blockers
(none)
🧪 Sara Holt — QA & Test Strategist
Verdict: ✅ Approved
The pyramid is balanced and fast. 1191 backend tests pass, 1122 frontend tests across 129 files pass. TDD discipline is visible in the commit sequence — each commit is red → green → commit. Nothing flaky, nothing mocked that should be real.
What I verified
CommentServiceTesthas the upstream fix covered in two directions — annotationId populated + notFound propagation. Helper-behavior coverage (authorName, mentions, audit) ported topostBlockComment_*equivalents. Net test count dropped because the removed dead-method tests exceeded the ported equivalents; that's correct for a feature removal.MigrationIntegrationTestseeds pre-V51-like data and runs the extracted SQL — verifies both comments and notifications are backfilled. JdbcTemplate pattern matches the V23/V30/V42 test style already established in that file. Good consistency.deepLinkScroll.spec.tshas 8 focused tests, each one logical assertion. Mocks are at the module boundary (injected callbacks, not global fetch). Fake timers not needed since the helper is promise-based —await expect.resolvesinstead. Clean.CommentMessage.svelte.spec.tspicked up 3 new attribute assertions againstgetByRole('article').element()— DOM-level correctness forid,tabindex, and the focus-visible class regex. Existing behavior tests (8) still pass.notification-deep-link.spec.tsseeds via API, drives the browser at both 320px and 1440px, asserts URL-strip + comment visibility, and runs axe-core. The viewport parameterization catches the mobile PDF-strip clipping concern we flagged in Theme 5. Screenshots saved for visual regression.Suggestions
waitForrather thanexpect.pollon URL strip in the E2E. Both work;expect.poll(() => page.url())re-evaluates until the assertion passes or times out. Fine as-is; just noting the alternative. Zero rewrite value.tabindex/focus-visibleadditions (axe doesn't need focus to be on a specific element to catch tabindex-related violations).Blockers
(none)
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The visual and interaction decisions match what we agreed. The new landing experience reuses the existing annotation-flash vocabulary so users recognize it immediately; keyboard and screen-reader users now have a first-class landing target that mouse users never see.
What I verified
CommentMessage.svelte:35-40—id="comment-{message.id}",tabindex="-1",outline-none focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2. Correct tokens (brand-navy, not raw hex), correct use offocus-visibleso only keyboard focus shows the ring.+page.svelte:309-312— flash duration respectsprefersReducedMotion(2000 ms / 1500 ms) matching the existinghandleAnnotationClickbehavior at line 237. Same motion policy, no drift.deepLinkScroll.ts:31—scrollIntoView({ behavior, block: 'center' }).'center'per our 320 px collapsed-PDF-strip discussion.behaviorswitches to'instant'under reduced motion. Both correct..focus({ preventScroll: true })— doesn't nudge the page a second time afterscrollIntoView. Good; double-scroll would feel jittery.aria-liveadded to the comment list container. Confirmed.Suggestions
flashAnnotationIdmechanism), but I want to note: if the PDF is still loading when the helper runs, the annotation polygon may not be visible yet when the flash fires. In that case the user sees no flash, only the focused comment. Acceptable for a notification click-through (the PDF typically renders fast), but could be worth a follow-up issue if user reports come in. No action this PR.sr-onlylive-region announcement on landing — something like"Landed on comment by {authorName}"for screen-reader confirmation. Not a regression from today (nothing was announced before), so it's a pure addition and can be a follow-up.Blockers
(none)
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Zero infrastructure impact. No Compose edits, no Caddy rules, no CI workflow changes, no env vars, no management ports. Ship it.
What I checked
UPDATEstatements, both idempotent viaIS NULLguards. Runs in sub-second on the family dataset; CX32 won't notice. Flyway checksum protects the migration from silent edits. No risk of data loss — the migration only fills nulls, never overwrites.postBlockCommentnow makes one extraSELECTontranscription_blocksviaTranscriptionService.getBlock. Indexed by primary key, ~1 ms per call. Irrelevant at this volume.frontend/e2e/. Runs against the existing Docker Compose stack in CI. Takes ~30 seconds at worst. Fine.Suggestions
Blockers
(none)
Three small refinements from Felix's review cycle 1: - replaceState(page.url.pathname, page.state ?? {}) — defend against first-navigation cases where page.state can be undefined. - Extract the inline tick + requestAnimationFrame into a named waitForPanelRender() helper; intent is now readable from onMount. - Attach .catch() to the fire-and-forget scrollToCommentFromQuery promise so any helper throw surfaces via console.error instead of silently disappearing. No behavior change on the happy path. All existing tests stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Review Cycle 1 — Changes
Addressed
replaceStateshould defaultpage.stateto{}— commit3bf0b38cafterTickshould be extracted to a named helper — extracted aswaitForPanelRender()in commit3bf0b38c.catch((e) => console.error('deep-link scroll failed', e))in commit3bf0b38cOut of scope
expectedDocumentIdcontract assertion onscrollToCommentFromQuery— Felix explicitly marked this as "Filing idea for future; no action this PR." No issue created; raise manually if needed.All other personas (Markus, Nora, Sara, Leonie, Tobias) returned ✅ Approved with no blockers.
Re-running review cycle 2.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
All three cycle-1 concerns resolved in
3bf0b38c:replaceState(page.url.pathname, page.state ?? {})at line 321 — defensive default in place.waitForPanelRender()helper extracted to lines 294-297 — onMount now reads as intent, not as timing plumbing..catch((e) => console.error('deep-link scroll failed', e))at line 322 — fire-and-forget errors now surface.No new issues. The diff since cycle 1 is 6 lines in one file, all polish.
deepLinkScrolltests (8/8) still green, prettier clean. Ready to merge.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
No architectural changes since cycle 1. The polish commit adds a single page-local helper (
waitForPanelRender) — appropriately scoped, no cross-module leak. My cycle-1 approval stands.🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Cycle-2 diff is three behavioral tweaks in
+page.svelte— no new endpoints, no new input paths, no new DOM injection points.console.error('deep-link scroll failed', e)uses structured logging with the error object as a parameter (not string concatenation) so any sensitive data in the error surfaces through the browser devtools only, not log aggregation. LGTM from my angle.🧪 Sara Holt — QA & Test Strategist
Verdict: ✅ Approved
The cycle-2 change is refactor-only on the wiring — no new behavior, no test impact. I re-ran the helper's unit suite (
deepLinkScroll.spec.ts: 8/8 pass) and the component tests (CommentMessage.svelte.spec.ts: 11/11 pass). The E2E journey is unchanged and still covered at both 320 px and 1440 px.One note, no action required:
.catch()surfacing errors throughconsole.erroris visible in the Playwright test run viapage.on('pageerror')orpage.on('console')if we ever want to assert on it. Nothing to add now.Ready to merge.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
No user-facing change since cycle 1. Cycle-2 is pure code polish on the onMount wiring — motion policy, focus handling, focus-visible ring, and 320 px behavior all unchanged. My cycle-1 approval stands. LGTM.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Cycle-2 touches one frontend file, zero infra. Migration, Compose, Caddy, CI all unchanged from cycle 1. LGTM — ship it.