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

As a user I want to open a document directly at a specific comment so I can jump into a discussion from an email or notification #73

New Issue
Closed
opened 2026-03-25 20:23:57 +01:00 by marcel · 3 comments
marcel commented 2026-03-25 20:23:57 +01:00
Owner
Copy Link

User Journey

A user receives an email notification — either because they were @mentioned in a comment or because someone replied in a thread they're part of. The email contains a direct link. They click it, the document page opens, the comment panel (side drawer or bottom drawer, depending on viewport) slides open automatically, and the referenced comment is scrolled into view with a brief visual highlight so it's immediately obvious which comment prompted the notification. The same behavior applies when they click a notification entry in the in-app bell dropdown.

E2E Scenarios

Scenario: Open document at a specific comment via deep link
  Given I am logged in
  When I navigate to /documents/{documentId}?commentId={commentId}
  Then the comment panel opens automatically
  And the referenced comment is scrolled into view
  And the comment is visually highlighted for ~2 seconds

Scenario: Unauthenticated deep link redirects to login then back
  Given I am not logged in
  When I navigate to /documents/{documentId}?commentId={commentId}
  Then I am redirected to /login
  And after login I am returned to /documents/{documentId}?commentId={commentId}
  And the comment panel opens and the comment is highlighted

Scenario: Deep link with non-existent commentId shows no highlight
  Given I am logged in
  When I navigate to /documents/{documentId}?commentId=nonexistent-uuid
  Then the document page loads normally
  And the comment panel does not auto-open

Implementation Notes

URL scheme

/documents/{documentId}?commentId={commentId}

No backend change needed — commentId is a frontend-only query param read on +page.svelte.

Frontend logic (documents/[id]/+page.svelte)

  • On mount: read $page.url.searchParams.get('commentId').
  • If present: set activeAnnotationId = commentId (existing binding) OR set a new openToCommentId prop on CommentThread.
  • Force the comment panel open (set showComments = true or equivalent state).
  • After the panel renders: scrollIntoView({ behavior: 'smooth', block: 'center' }) on the matching comment element.
  • Add a CSS class comment-highlight for ~2 s (remove via setTimeout or a Svelte transition).

Email links (#71)

NotificationService already builds {baseUrl}/documents/{documentId} links. Extend to append ?commentId={referenceId} for both REPLY and MENTION notifications.

In-app bell (#71)

NotificationBell already navigates to goto('/documents/{documentId}') on item click. Extend to goto('/documents/{documentId}?commentId={referenceId}').

Highlight style

Brief ring or left-border flash using a Tailwind animation (e.g. animate-pulse for one cycle, or a custom @keyframes flash). Should respect the active color theme.

Dependencies

  • Refs #71 (notifications — email links + bell navigation)
  • Refs #72 (@mentions — mention notification links)

Both #71 and #72 should be implemented before or alongside this issue, since they generate the links that need to deep-link here.

## User Journey A user receives an email notification — either because they were @mentioned in a comment or because someone replied in a thread they're part of. The email contains a direct link. They click it, the document page opens, the comment panel (side drawer or bottom drawer, depending on viewport) slides open automatically, and the referenced comment is scrolled into view with a brief visual highlight so it's immediately obvious which comment prompted the notification. The same behavior applies when they click a notification entry in the in-app bell dropdown. ## E2E Scenarios ``` Scenario: Open document at a specific comment via deep link Given I am logged in When I navigate to /documents/{documentId}?commentId={commentId} Then the comment panel opens automatically And the referenced comment is scrolled into view And the comment is visually highlighted for ~2 seconds Scenario: Unauthenticated deep link redirects to login then back Given I am not logged in When I navigate to /documents/{documentId}?commentId={commentId} Then I am redirected to /login And after login I am returned to /documents/{documentId}?commentId={commentId} And the comment panel opens and the comment is highlighted Scenario: Deep link with non-existent commentId shows no highlight Given I am logged in When I navigate to /documents/{documentId}?commentId=nonexistent-uuid Then the document page loads normally And the comment panel does not auto-open ``` ## Implementation Notes ### URL scheme `/documents/{documentId}?commentId={commentId}` No backend change needed — `commentId` is a frontend-only query param read on `+page.svelte`. ### Frontend logic (`documents/[id]/+page.svelte`) - On mount: read `$page.url.searchParams.get('commentId')`. - If present: set `activeAnnotationId = commentId` (existing binding) OR set a new `openToCommentId` prop on `CommentThread`. - Force the comment panel open (set `showComments = true` or equivalent state). - After the panel renders: `scrollIntoView({ behavior: 'smooth', block: 'center' })` on the matching comment element. - Add a CSS class `comment-highlight` for ~2 s (remove via `setTimeout` or a Svelte transition). ### Email links (#71) `NotificationService` already builds `{baseUrl}/documents/{documentId}` links. Extend to append `?commentId={referenceId}` for both REPLY and MENTION notifications. ### In-app bell (#71) `NotificationBell` already navigates to `goto('/documents/{documentId}')` on item click. Extend to `goto('/documents/{documentId}?commentId={referenceId}')`. ### Highlight style Brief ring or left-border flash using a Tailwind animation (e.g. `animate-pulse` for one cycle, or a custom `@keyframes` flash). Should respect the active color theme. ## Dependencies - Refs #71 (notifications — email links + bell navigation) - Refs #72 (@mentions — mention notification links) Both #71 and #72 should be implemented before or alongside this issue, since they generate the links that need to deep-link here.
marcel commented 2026-03-25 20:27:10 +01:00
Author
Owner
Copy Link

Decision: smart routing via annotationId query param

DocumentComment already has a nullable annotationId field — this is the discriminator:

  • annotationId present → annotation comment → side drawer (PDF annotation panel)
  • annotationId absent → thread comment → bottom drawer (general comment thread)

Updated URL scheme

Comment type Link
Annotation comment /documents/{id}?commentId={commentId}&annotationId={annotationId}
Thread comment /documents/{id}?commentId={commentId}

NotificationService change

When building the link for REPLY and MENTION notifications, load the comment and check comment.getAnnotationId():

String commentPath = comment.getAnnotationId() != null
    ? "?commentId=" + comment.getId() + "&annotationId=" + comment.getAnnotationId()
    : "?commentId=" + comment.getId();

String link = baseUrl + "/documents/" + comment.getDocumentId() + commentPath;

Frontend routing logic (documents/[id]/+page.svelte)

const commentId = $page.url.searchParams.get('commentId');
const annotationId = $page.url.searchParams.get('annotationId');

if (commentId && annotationId) {
    // annotation comment — open side drawer at this annotation
    activeAnnotationId = annotationId;
    showAnnotationPanel = true;
} else if (commentId) {
    // thread comment — open bottom drawer
    showComments = true;
}

Pass openToCommentId={commentId} into whichever panel component is opened so it can scroll + highlight the specific comment once rendered.

## Decision: smart routing via `annotationId` query param `DocumentComment` already has a nullable `annotationId` field — this is the discriminator: - `annotationId` present → annotation comment → **side drawer** (PDF annotation panel) - `annotationId` absent → thread comment → **bottom drawer** (general comment thread) ### Updated URL scheme | Comment type | Link | |---|---| | Annotation comment | `/documents/{id}?commentId={commentId}&annotationId={annotationId}` | | Thread comment | `/documents/{id}?commentId={commentId}` | ### `NotificationService` change When building the link for REPLY and MENTION notifications, load the comment and check `comment.getAnnotationId()`: ```java String commentPath = comment.getAnnotationId() != null ? "?commentId=" + comment.getId() + "&annotationId=" + comment.getAnnotationId() : "?commentId=" + comment.getId(); String link = baseUrl + "/documents/" + comment.getDocumentId() + commentPath; ``` ### Frontend routing logic (`documents/[id]/+page.svelte`) ```typescript const commentId = $page.url.searchParams.get('commentId'); const annotationId = $page.url.searchParams.get('annotationId'); if (commentId && annotationId) { // annotation comment — open side drawer at this annotation activeAnnotationId = annotationId; showAnnotationPanel = true; } else if (commentId) { // thread comment — open bottom drawer showComments = true; } ``` Pass `openToCommentId={commentId}` into whichever panel component is opened so it can scroll + highlight the specific comment once rendered.
marcel added the notification label 2026-03-25 20:30:07 +01:00
marcel commented 2026-03-27 19:04:12 +01:00
Author
Owner
Copy Link

UX Review — @leonievoss

The URL scheme is clean, the annotationId discriminator from the architect's comment is the right call. Two UX gaps to address.

🔴 High — 2-second highlight timeout fails for seniors

A 2-second flash is sufficient for a millennial scanning quickly. A senior reading slowly will miss it entirely — they'll see the comment panel open but have no idea which comment triggered the notification.

Don't auto-remove the highlight on a timeout. Remove it on the user's first interaction after it appears:

el.classList.add('comment-highlight');
const removeHighlight = () => {
    el.classList.remove('comment-highlight');
    el.removeEventListener('scroll', removeHighlight);
    document.removeEventListener('click', removeHighlight);
    document.removeEventListener('keydown', removeHighlight);
};
// Remove on first scroll, click, or keypress — not on a timer
el.addEventListener('scroll', removeHighlight, { once: true });
document.addEventListener('click', removeHighlight, { once: true });
document.addEventListener('keydown', removeHighlight, { once: true });

This costs nothing and serves both audiences. The highlight disappears naturally the moment the user acts — which is exactly when they no longer need it.

🟡 Medium — animate-pulse is the wrong highlight pattern

animate-pulse signals "loading" in most design systems — it will read as a spinner or skeleton state, not as "this is the comment you're looking for". Use a left-border flash instead:

.comment-highlight {
  box-shadow: inset 4px 0 0 #002850; /* brand-navy */
  background-color: color-mix(in srgb, #002850 8%, transparent);
  transition: box-shadow 0.6s ease, background-color 0.6s ease;
}

Clear, directional, non-alarming. Fades out smoothly when the class is removed.

🟡 Medium — Scroll timing must wait for panel render

The $effect that scrolls to the target comment must fire after the panel has rendered its comment list — comments are fetched async, so the element may not exist in the DOM when the query param is first read.

$effect(() => {
    if (openToCommentId && comments.length > 0) {
        // Only runs once comments have loaded
        const el = document.querySelector(`[data-comment-id="${openToCommentId}"]`);
        el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
        // ... add highlight class and interaction listeners
    }
});

The comments.length > 0 dependency guard is the key — it ensures the effect re-runs after the async fetch resolves. This is the most common failure point for deep-link scroll behavior and should be called out explicitly in implementation notes.

🟡 Medium — Mobile panel behavior is unspecified

On desktop the comment panel is a side drawer; on mobile it's presumably a bottom sheet or full-screen. Auto-opening a bottom sheet can obscure the document entirely on a small viewport. Specify:

On mobile: the panel opens, the target comment is scrolled into view within the panel. The document partial view above the panel remains visible. Do not auto-scroll the document itself.

✅ What's good

  • annotationId discriminator to route between side drawer and bottom drawer — clean, uses existing DocumentComment.annotationId field, no new infrastructure.
  • Unauthenticated redirect flow (redirect to /login, then back to full deep link after auth) is correctly specified.
  • Non-existent commentId falls back to normal page load without error — correct graceful degradation.
  • Frontend-only query param — no backend change needed, correct.
## UX Review — @leonievoss The URL scheme is clean, the `annotationId` discriminator from the architect's comment is the right call. Two UX gaps to address. ### 🔴 High — 2-second highlight timeout fails for seniors A 2-second flash is sufficient for a millennial scanning quickly. A senior reading slowly will miss it entirely — they'll see the comment panel open but have no idea which comment triggered the notification. **Don't auto-remove the highlight on a timeout.** Remove it on the user's first interaction after it appears: ```typescript el.classList.add('comment-highlight'); const removeHighlight = () => { el.classList.remove('comment-highlight'); el.removeEventListener('scroll', removeHighlight); document.removeEventListener('click', removeHighlight); document.removeEventListener('keydown', removeHighlight); }; // Remove on first scroll, click, or keypress — not on a timer el.addEventListener('scroll', removeHighlight, { once: true }); document.addEventListener('click', removeHighlight, { once: true }); document.addEventListener('keydown', removeHighlight, { once: true }); ``` This costs nothing and serves both audiences. The highlight disappears naturally the moment the user acts — which is exactly when they no longer need it. ### 🟡 Medium — `animate-pulse` is the wrong highlight pattern `animate-pulse` signals "loading" in most design systems — it will read as a spinner or skeleton state, not as "this is the comment you're looking for". Use a left-border flash instead: ```css .comment-highlight { box-shadow: inset 4px 0 0 #002850; /* brand-navy */ background-color: color-mix(in srgb, #002850 8%, transparent); transition: box-shadow 0.6s ease, background-color 0.6s ease; } ``` Clear, directional, non-alarming. Fades out smoothly when the class is removed. ### 🟡 Medium — Scroll timing must wait for panel render The `$effect` that scrolls to the target comment must fire **after** the panel has rendered its comment list — comments are fetched async, so the element may not exist in the DOM when the query param is first read. ```typescript $effect(() => { if (openToCommentId && comments.length > 0) { // Only runs once comments have loaded const el = document.querySelector(`[data-comment-id="${openToCommentId}"]`); el?.scrollIntoView({ behavior: 'smooth', block: 'center' }); // ... add highlight class and interaction listeners } }); ``` The `comments.length > 0` dependency guard is the key — it ensures the effect re-runs after the async fetch resolves. This is the most common failure point for deep-link scroll behavior and should be called out explicitly in implementation notes. ### 🟡 Medium — Mobile panel behavior is unspecified On desktop the comment panel is a side drawer; on mobile it's presumably a bottom sheet or full-screen. Auto-opening a bottom sheet can obscure the document entirely on a small viewport. Specify: > On mobile: the panel opens, the target comment is scrolled into view **within the panel**. The document partial view above the panel remains visible. Do not auto-scroll the document itself. ### ✅ What's good - `annotationId` discriminator to route between side drawer and bottom drawer — clean, uses existing `DocumentComment.annotationId` field, no new infrastructure. - Unauthenticated redirect flow (redirect to `/login`, then back to full deep link after auth) is correctly specified. - Non-existent `commentId` falls back to normal page load without error — correct graceful degradation. - Frontend-only query param — no backend change needed, correct.
marcel commented 2026-03-27 19:13:24 +01:00
Author
Owner
Copy Link

QA Review — @saraholt

The spec is clean and Leonie's UX comments cover the critical interaction gaps. Two things need to be encoded as tests, not just implementation notes.

🔴 High — the interaction-based highlight must be verified in E2E, not just specified

Leonie's requirement (highlight persists until user interaction, not a 2-second timer) is easy to implement incorrectly and easy to verify if we have a test:

test('comment highlight persists until user interaction, not on a timer', async ({ page }) => {
  await page.goto(`/documents/${docId}?commentId=${commentId}`);
  const comment = page.locator(`[data-comment-id="${commentId}"]`);

  await expect(comment).toHaveClass(/comment-highlight/);

  // Still highlighted after 3 seconds — timer-based implementation would fail here
  await page.waitForTimeout(3000);
  await expect(comment).toHaveClass(/comment-highlight/);

  // Removed after first user interaction
  await page.click('body');
  await expect(comment).not.toHaveClass(/comment-highlight/);
});

Without this test, a future refactor can swap in a setTimeout removal and the requirement silently regresses.

🔴 High — scroll timing must be tested with async comment loading

Leonie's note about comments.length > 0 as the $effect guard is the most common failure point for deep-link scroll behavior. Verify it:

test('scrolls to target comment after async fetch resolves', async ({ page }) => {
  // Intercept and delay the comments API response to simulate async loading
  await page.route('**/api/documents/*/comments', async route => {
    await new Promise(r => setTimeout(r, 500)); // simulate slow load
    await route.continue();
  });

  await page.goto(`/documents/${docId}?commentId=${commentId}`);

  // Comment must be visible after the delayed load, not just after mount
  const comment = page.locator(`[data-comment-id="${commentId}"]`);
  await expect(comment).toBeVisible();
  await expect(comment).toHaveClass(/comment-highlight/);
});

🟡 Complete E2E coverage for the three scenarios

The issue spec lists three E2E scenarios. Add these to the Playwright suite as written — they map directly to the implementation:

  • annotationId present → side drawer opens (not bottom drawer)
  • annotationId absent → bottom drawer opens
  • Unauthenticated deep link → redirects to /login → after login, returns to full URL with ?commentId= intact
  • Non-existent commentId → page loads normally, no panel auto-opens, no error

The login-redirect scenario is the one most likely to regress silently — SvelteKit's redirect handling needs to preserve query params through the auth flow.

✅ What's solid

  • annotationId discriminator to route between side drawer and bottom drawer — uses existing DocumentComment.annotationId field, no new infrastructure, clean
  • Frontend-only query param — no backend change needed
  • Graceful degradation for non-existent commentId is correctly specified
  • NotificationService deep-link construction (with annotationId conditional) is covered in the #71 unit tests
## QA Review — @saraholt The spec is clean and Leonie's UX comments cover the critical interaction gaps. Two things need to be encoded as tests, not just implementation notes. --- ### 🔴 High — the interaction-based highlight must be verified in E2E, not just specified Leonie's requirement (highlight persists until user interaction, not a 2-second timer) is easy to implement incorrectly and easy to verify if we have a test: ```typescript test('comment highlight persists until user interaction, not on a timer', async ({ page }) => { await page.goto(`/documents/${docId}?commentId=${commentId}`); const comment = page.locator(`[data-comment-id="${commentId}"]`); await expect(comment).toHaveClass(/comment-highlight/); // Still highlighted after 3 seconds — timer-based implementation would fail here await page.waitForTimeout(3000); await expect(comment).toHaveClass(/comment-highlight/); // Removed after first user interaction await page.click('body'); await expect(comment).not.toHaveClass(/comment-highlight/); }); ``` Without this test, a future refactor can swap in a `setTimeout` removal and the requirement silently regresses. --- ### 🔴 High — scroll timing must be tested with async comment loading Leonie's note about `comments.length > 0` as the `$effect` guard is the most common failure point for deep-link scroll behavior. Verify it: ```typescript test('scrolls to target comment after async fetch resolves', async ({ page }) => { // Intercept and delay the comments API response to simulate async loading await page.route('**/api/documents/*/comments', async route => { await new Promise(r => setTimeout(r, 500)); // simulate slow load await route.continue(); }); await page.goto(`/documents/${docId}?commentId=${commentId}`); // Comment must be visible after the delayed load, not just after mount const comment = page.locator(`[data-comment-id="${commentId}"]`); await expect(comment).toBeVisible(); await expect(comment).toHaveClass(/comment-highlight/); }); ``` --- ### 🟡 Complete E2E coverage for the three scenarios The issue spec lists three E2E scenarios. Add these to the Playwright suite as written — they map directly to the implementation: - `annotationId` present → side drawer opens (not bottom drawer) - `annotationId` absent → bottom drawer opens - Unauthenticated deep link → redirects to `/login` → after login, returns to full URL with `?commentId=` intact - Non-existent `commentId` → page loads normally, no panel auto-opens, no error The login-redirect scenario is the one most likely to regress silently — SvelteKit's `redirect` handling needs to preserve query params through the auth flow. --- ### ✅ What's solid - `annotationId` discriminator to route between side drawer and bottom drawer — uses existing `DocumentComment.annotationId` field, no new infrastructure, clean - Frontend-only query param — no backend change needed - Graceful degradation for non-existent `commentId` is correctly specified - `NotificationService` deep-link construction (with `annotationId` conditional) is covered in the #71 unit tests
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label notification
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#73
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?