fix(notifications): jump to the notified comment

[16th-admin]

Closes #110

• Carry from, take, and skip through notification navigation.
• Keep the anchored comment in the returned context instead of dropping it as a pagination duplicate.
• Scroll the notified comment into view and use the routed page size for subsequent pagination.
• Remount the comment list and refresh the title when a kept-alive Comments route targets another notification.

Validation: changed-file ESLint (two existing complexity warnings); filtered type-check; git diff --check.

[github-actions]

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

Now let me read the full files for context.

PR Review: fix(notifications): jump to the notified comment

Scope: 3 files, +25/−5 lines · MessageList.vue, NotificationItem.vue, Comments.vue

The PR wires a comment notification click through a URL query (?from={CommentID}&skip=0) to the Comments view, then scrolls the comment list to the notified comment. The plumbing is clean and typed, but the core jump mechanism has a correctness bug that prevents it from working.

---

🔴 Blocking

1. The target comment is removed by messages.shift() before the scroll check

src/components/messages/MessageList.vue:155

if (from) messages.shift()

from is initialized to the target comment id (initialFrom), so on the first load the API returns the target comment as the first item (this is why the existing cursor-pagination shifts it on continuation loads — the server echoes the cursor). The shift() therefore removes the exact comment the user is trying to jump to.

Result: the notified comment is never rendered, and the check at MessageList.vue:166 —

if (targetCommentId && items.value.some((item) => item.ID === targetCommentId)) {

— can never succeed on the initial page, so scrollIntoView never fires. The feature silently degrades to "load the list starting after the target comment."

Fix suggestion: only dedupe the cursor on continuation loads, not on the initial jump load — e.g. keep the target comment when it equals initialFrom, or shift only when items.value is non-empty. Also verify the actual server semantics of GetComments(CommentID, Skip) to confirm the cursor is returned first.

🟡 Important

2. Scroll check re-runs on every infinite-scroll load (no one-shot guard)

src/components/messages/MessageList.vue:166

Once the target is in items (after fixing #1), every subsequent handleLoad() (i.e. every time the user scrolls to load more) will re-center the view on the target comment, fighting the user's scroll direction. Add a one-shot flag (e.g. let scrolledToTarget = false) and scroll only once, or only on the first successful page.

3. Props are read only at component setup — in-place navigation won't re-jump

src/components/messages/MessageList.vue:42-43

let skip = ref(initialSkip)
let from: CommentResult['ID'] | null = initialFrom || null

Comments.vue is a route-level component that Vue Router reuses when navigating between different /c/:category/:id/:name?from=... URLs (same route record). If a second notification jump happens while the comments page is already mounted, commentFrom/commentSkip update but from/skip never re-initialize and no fetch/scroll occurs. Consider watch-ing the props or adding a :key to force remount.

🟢 Nits / Suggestions

• MessageList.vue:4 — :id="comment-${item.ID}" + document.getElementById couples global DOM ids to list internals and can collide if two lists render on one page. A ref map keyed by item.ID would be more robust.
• NotificationItem.vue:99 — fields.CommentID isn't guarded like targetId/targetName; an empty value yields ?from=&skip=0 (harmless fallback to page 1, but a if (!fields.CommentID) return guard would make intent explicit).
• NotificationItem.vue:99 — the hardcoded &skip=0 is redundant; commentSkip already defaults to 0 when absent.

🎉 What works well

• skip parsing in Comments.vue:63-66 is properly validated (Number.isSafeInteger + >= 0) — good handling of untrusted URL input.
• Props are fully typed with defaults, keeping the child contract explicit.
• The URL-query contract (from/skip) is a clean, shareable mechanism for deep-linking into a comment thread.
• The nextTick() before scrolling is correct timing-wise (waits for the newly rendered nodes).

Decision

🔄 Request changes — #1 must be addressed for the feature to work at all; #2 should be fixed in the same pass to avoid a scroll-fighting UX bug. #3 is a worthwhile follow-up.

[github-actions]

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

PR Review: fix(notifications): jump to the notified comment

Reviewed via git diff origin/main...HEAD across MessageList.vue, NotificationItem.vue, Comments.vue. The feature: clicking a notification opens the comments page and scrolls to / highlights the specific comment.

Summary

The approach (pass the anchored CommentID + skip via query string, seed the list from that anchor, scroll to it after mount) is reasonable and works for the happy path. The key correctness fix — keeping the anchored comment (from !== initialFrom shift guard) — is correct. A few usability and architecture issues below, ordered by severity.

Findings

1. (Medium) scrollIntoView re-fires on every subsequent page-load, yanking the user back

src/components/messages/MessageList.vue:166-171

targetCommentId remains set for the component lifetime, and handleLoad runs on every InfiniteScroll @load. After the user has scrolled past the target, the next pagination load re-triggers scrollIntoView and drags the viewport back to the comment. Add a scrolledOnce guard so the scroll happens at most once (or only until the target is first found).

if (!jumped && targetCommentId && items.value.some((i) => i.ID === targetCommentId)) {
  jumped = true
  document.getElementById(`comment-${targetCommentId}`)?.scrollIntoView({ ... })
}

2. (Low) Direct DOM query instead of a template ref

src/components/messages/MessageList.vue:4,167

Using :id="comment-${item.ID}" + document.getElementById bypasses Vue's render layer and couples to raw id strings (global namespace, potential collisions if the list is ever rendered twice). Prefer a scoped template ref (e.g. :ref on the loop item or an anchor ref matching the found ID) and ref.value?.scrollIntoView(...).

3. (Low) Side effects in template / prop-init timing is fine, but worth a comment

src/components/messages/MessageList.vue:42-43 capture initialSkip/initialFrom once at setup. Since these come from route.query, a same-component navigation (URL change without remount) would not re-init — acceptable here because the route is recreated, but the one-time init + site-effect handleLoad() in setup could be a deliberate onBeforeMount/watch for clarity. Minor.

4. (Info) _length captured before shift/from mutation

src/components/messages/MessageList.vue:154-160 — _length is measured pre-shift, so the _length < 20 → noMore heuristic runs against the pre-guard count. With the anchor kept it's fine; just be aware the "remaining < 20" signal is computed on the anchored batch, so a page that happens to return exactly 20 including the anchor may drop the paging signal. Consider basing it on messages.length after the guard.

Nitpicks

• NotificationItem.vue sends skip=0 literally; fine, since commentSkip defaults to 0.
• target-comment-id and initial-from are always identical — the two props are coupled; could be documented or, better, the scroll target derived from initialFrom internally.

Verify

• Manual: click notification → verify the comment shows in top context and page center-scrolls; then scroll down (load more) and confirm the view does not get pulled back up.
• Check a non-notification normal visit (no ?from=) still paginates with no unexpected shift.

Overall: ship-blocking item is only #1; the rest are cleanup suggestions.

[github-actions]

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

Reviewed the diff origin/main...HEAD (PR head = detached pull/190/merge). Focus: notification → comment deep-link jump across MessageList.vue, NotificationItem.vue, and Comments.vue.

Code Review Report — Notification comment-jump feature

Summary

The change lets a notification open a comment route with from/take/skip query params and auto-scroll to the anchored comment. The approach is sound and mostly clean. Below are issues in order of importance.

---

1. (Bug / UX) scrollIntoView re-fires on every page load

MessageList.vue:176-181

if (targetCommentId && items.value.some((item) => item.ID === targetCommentId)) {
  document.getElementById(`comment-${targetCommentId}`)?.scrollIntoView({ block: 'center', behavior: 'smooth' })
}

Once the anchor exists it will always be present after the first page, and this runs at the end of every handleLoad (i.e. each time the user scrolls/paginates). Every scroll-to-load-more will yank the viewport back to the comment. Add a once-guard:

let hasScrolled = false
...
if (targetCommentId && !hasScrolled && items.value.some((item) => item.ID === targetCommentId)) {
  document.getElementById(`comment-${targetCommentId}`)?.scrollIntoView({ block: 'center', behavior: 'smooth' })
  hasScrolled = true
}

---

2. (Maintainability) anchor/shift now depends on a prop, easy to break

MessageList.vue:165

if (from && from !== initialFrom) messages.shift()

Previously this was a self-consistent dedup rule (from was always produced by a prior load). Now it hard-codes special-casing of a client-supplied prop. The first page keeps the anchor item while every later page drops its first (duplicate) item — an intentional asymmetrical behavior that is not self-evident. At minimum add a comment explaining why the first page must retain the anchor (the API returns the from comment itself). Also note messages.shift() on an empty array is silently a no-op; it may be worth a length check.

3. (Robustness) Component state is frozen at mount; won't reset if the route reuses the instance

MessageList.vue:47-53

pageSize, skip, and from are computed/initialized once from initial* props. If MessagesList is ever re-fetched with a different query while the instance is reused (same component, changed route params/query), from/skip/skip retain stale values and from === initialFrom dedup logic breaks. This is fine today (notification opens a new window via _self), but it's fragile. Consider watch() on the props (or a v-if/:key on the parent) so state rebuilds on a real param change. Confirm via the router that route.params.id changes always remount or key this component:

<MessagesList :key="route.params.id + ':' + commentFrom" ... />

4. (Minor) Duplicated pagination clamping

Comments.vue:68-71 clamps take to 1..50, and MessageList.vue:47 clamps again with Math.min(50, Math.max(1, ...)). Keeping the server-side clamp (pageSize) is good defense; the UI clamp is redundant but harmless. Leave one authoritative source if you want to reduce confusion.

5. (Minor) Redundant param in the opened URL

NotificationItem.vue:99 always appends &skip=0 and &take=20. take=20 duplicates the component default and skip=0 the default. Fine to keep explicit for future flexibility, but it means a bare notification link and a deep-jump link look similar yet behave differently (only due to from). Worth a one-line comment that from is the actual signal and take/skip are intended to make it explicit.

---

What's good

• Proper use of computed() for query param parsing with Number.isSafeInteger guards (Comments.vue:60-71) — avoids NaN/negative injection into the API.
• pageSize clamp prevents a malicious ?take=9999 from over-fetching.
• Element ids are namespaced (comment-${ID}) and keyed, avoiding duplicate-id collisions.
• Dedup logic (existingIds set in the upDate watch) stays correct with the added anchor handling.

Non-blocking style nits

• MessageList.vue:49,52: let items = ref(...) / let noMore = ref(...) should be const (refs are reassigned via .value, not rebinding).
• Comments.vue:63,66,70: the @route.query.from recomputes could fall back to boolean undefined handling if router normalizes query; current typeof === 'string' check is fine.

---

Verdict: Approve with a follow-up — fix #1 (repeated scroll) before merge, and apply either the :key remount or a documented watcher in #3 to make the state-reset contract explicit. Items #4/#5 are optional cleanups.

[github-actions]

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

I'll start by inspecting the PR's commit history
The PR head is a merge commit for pull/190. Let me diff against main
The PR is focused on 3 files. Let me review the full diffs
Now let me read the full files for context
Let me review the remaining files and the infinite scroll interaction
Let me review the underlying commits and run lint

PR Review Report

PR #190 — Notification deep-link → comment jump (4 commits: 78e3a4f, 3e2bd13, 84befd3, 9c3ea8c)
Scope: src/components/messages/MessageList.vue, src/components/messages/NotificationItem.vue, src/views/Comments.vue

The change routes ?from&take&skip from notification items into the comment page, remounts the list, and scrolls to the anchored comment. The intent is clear and the direction is right (the onMounted → watch + titleTicket refactor in Comments.vue is a solid improvement). Findings below.

High priority

1. Scroll-to-target re-fires on every pagination load — MessageList.vue:175-181
scrollIntoView lives inside handleLoad, which runs on mount and on every infinite-scroll page load. Once the target is in the list, every subsequent load yanks the viewport back to the centered anchor, fighting the user's downward scroll. Since has-more is usually true when a deep-linked comment is on page 1, this will almost always trigger.

let anchored = false
// ...
if (!anchored && targetCommentId && items.value.some((i) => i.ID === targetCommentId)) {
  anchored = true
  document.getElementById(`comment-${targetCommentId}`)?.scrollIntoView(...)
}

The :key remount resets the flag naturally, so no cleanup is needed.

Medium priority

2. Pagination mixes cursor (CommentID) and offset (Skip) — MessageList.vue:58-66,162-170
With initialSkip now routable, both from (cursor) and skip (offset) advance together. The server's semantics for simultaneous CommentID + Skip are unclear; if the cursor wins and Skip is ignored, the initial anchor may never appear in the returned page (→ no scroll and the user sees post-anchor comments), while if both are honored you can duplicate/skip rows as skip increments on every page. This was latent before, but the routed skip/take now expose it to real URLs. Verify against /Messages/GetComments contract — ideally pick one pagination strategy.

3. Anchor retention depends on undocumented server behavior — MessageList.vue:165
if (from && from !== initialFrom) messages.shift() works only if the server returns the from comment as the first row. If the API returns Take rows after from (which the old shift() logic implies), the deep-linked comment is never rendered and nothing scrolls. Worth a test assertion against the live API.

4. Fragile implicit coupling via :key remount — Comments.vue:20-30
Destructured props (initialFrom, initialTake, targetCommentId, …) are read once at setup and are non-reactive. Correctness relies entirely on the parent force-remounting via commentListKey. If any future parent renders <MessageList> without a changing key, prop updates will be silently ignored. Either watch the props explicitly or derive everything from the route inside MessageList; at minimum, document the remount dependency.

Low priority / minor

• Duplicated clamping: commentTake is validated to 1–50 in Comments.vue:69-72 and re-clamped in MessageList.vue:47. These can drift — keep one source of truth.
• encodeURIComponent(fields.CommentID) — NotificationItem.vue:99 emits from=undefined if CommentID is missing; guard it.
• Same-target re-click doesn't re-scroll — navigating to an identical from/id/take/skip produces an identical key, so the "refresh reused comment targets" intent (commit 9c3ea8c) won't fire for a repeated click on the same comment. Minor, but the ticket suggests it was intended.
• Unhandled rejection in parse() — Comments.vue:90; the titleTicket guard is good, but a rejecting parse still leaves an unhandled promise.
• scrollIntoView({ block: 'center' }) may tuck the target behind the sticky header/composer; block: 'start' + a small offset may be safer.

Verdict

Good, focused fix with a clear feature intent and a nice router-watch refactor. The repeated-scroll bug (#1) should be fixed before merge; #2/#3 are worth confirming against the API contract. No blocking type or lint regressions detected (couldn't run local lint/typecheck — node_modules absent).