A notification click lands on its comment from any page, not only the same one - #531
Open
yayashuxue wants to merge 1 commit into
Open
A notification click lands on its comment from any page, not only the same one#531yayashuxue wants to merge 1 commit into
yayashuxue wants to merge 1 commit into
Conversation
… same one Clicking a notification from another page opened the right document with `?comment=` in the URL and then went nowhere: the page sat at the top and the comment was below the fold. On the same document the same click scrolled to the comment and opened it. (JUL-38) The deep-link effect in document-shell runs when the comments arrive. On a fresh arrival that is before the frame's first `tdoc:pins`, so the target has no cluster yet. The effect read that as "anchor lost", opened the card as a floating one and cleared the target -- and with the target cleared it never ran again, so the document never scrolled. On the same document the pins were already there, which is why that path worked. The fallback was written (#328) for a comment whose anchor no longer resolves. That case has since been given a seat: the frame reports a pin for every comment the margin shows, a real one or a seat at the end of the document, and the deep target is always shown. So a target with no cluster is one the frame has not laid out yet, never one it has lost, and the effect now waits for the pins instead. It runs again when they come, and from there the existing path scrolls to the pin -- or to the seat -- and opens the card. Reproduced and verified in a browser against server.js: a document made long enough that the comment starts below the fold, a seeded inbox, one click. Before: card open, pin never on screen, no scroll. After: the same scroll-and-open as the same-document case. The orphaned comment #328 cared about still opens, now scrolled to its seat rather than floating at the top. Tests: a gated browser suite covers all three cases -- same document, another page, anchor gone -- and the second fails on the old code. The static overlay-pure test that pinned the old fallback's shape is rewritten to state the new contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PreviewOpen this: https://pr-531-tdoc-preview.jyshi1107.workers.dev/ — the homepage, this PR's code This link is unique to this PR. New commits update the same URL. It is not tdoc.dev. Preview has no Durable Object — concurrent comments use the KV fallback. Data expires in 14 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes JUL-38 — Notifications don't redirect to the target doc/comment.
What was happening
Clicking a notification from another page opened the right document with
?comment=<id>in the URL, then went nowhere: the page sat at the top, the comment stayed below the fold. Clicking the same notification on the same document scrolled to the comment and opened it. That asymmetry is the whole bug.Why
The deep-link effect in
document-shell.jsxruns when the comments arrive. On a fresh arrival that is before the frame's firsttdoc:pins, so the target has no cluster yet. The effect read "no cluster" as "anchor lost", opened the card as a floating one and cleared the target — and once cleared it never ran again, so the document never scrolled. On the same document the pins were already there, which is why that path worked.The fix
That fallback was written in #328 for a comment whose anchor no longer resolves. Since then that case has been given a seat: the frame reports a pin for every comment the margin shows — a real one, or a seat at the end of the document when the anchor is gone — and the deep target is always shown. So a target with no cluster is one the frame hasn't laid out yet, never one it has lost. The effect now waits (returns without clearing the target); it re-runs when the pins arrive, and the existing path scrolls to the pin — or the seat — and opens the card.
One branch changed; nothing added.
Verified in a browser
Booted
server.jswith the e2e inbox, lengthened the fixture so the comment starts far below the fold, seeded one notification, clicked it.Tests
test/notification-deep-link.test.jscovers all three rows above. The second one fails onmain(pin never becomes visible) and passes here — checked by stashing the fix and re-running.test/overlay-pure.test.jshad a source-text assertion pinning the old fallback's exact shape; rewritten to state the new contract and point at the browser test for the orphan case.npm test: 80/80 green.artifact-shell.test.js(43 browser tests over the same comment layer): 43/43.🤖 Generated with Claude Code