fix(hierarchy): hold the tree's place when a deep sub-event is clicked - #181
Merged
Conversation
Curators, re-testing #137: "no jumping in the search and the analysis table and none in the upper section of event hierarchy, but pathways near the bottom of the hierarchy (e.g. subevents) still jump." They were right, and the revealing was not the cause. Rebuilding the tree starts by emptying it -- a workaround for an Angular Material bug where nested children otherwise do not render -- and emptying it destroys every row, so the container collapses and the browser resets its scroll to the top. Clicking a sub-event that was already on screen measured 274px, then 0, then 6: a jump to the top and a scroll back. Expansion state was already carried across that rebuild. The scroll position now is too, restored again on the next frame because the height is not final until the restored branches have rendered and the browser clamps a scrollTop set against a container that is still short. Clicking a row you can already see now moves the tree 0px, from 274. A row you cannot see is still brought into view -- 274 to 130 for one above the fold -- because that is the half worth keeping, and both are tested. I tried removing the workaround instead, since the bug it cites is from 2018 and it carries a "check performance issue" note. It is still needed: without it the tree renders 29 rows instead of 41 and no branch opens. Left in place, with what it costs written down next to it. Verified to fail without the fix: the already-visible case goes red, the reveal case stays green. Fixes #137 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamjohnwright
enabled auto-merge (squash)
September 9, 2026 16:49
adamjohnwright
added a commit
that referenced
this pull request
Sep 9, 2026
#181 restores the reader's scroll position on frame one; RevealDirective brings a selected row into view on frame two. That order is what makes `block: 'nearest'` compare against where the reader actually was, so a row already on screen is left alone. Reverse it and clicking a visible row scrolls twice again -- the bug #181 fixed. Neither side said so, and both comments read as if their frame count were a local choice. Noted in both, because either one being "simplified" breaks the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamjohnwright
added a commit
that referenced
this pull request
Sep 9, 2026
…185) * fix(routing): decide "replace or add" from the URL, not from a flag #184 put a `settling` boolean on the service: settle() raised it, the writer effect read it and lowered it. That only works if the writer always runs, once, right after. It does not: - the writer returns early on the content and search routes, before it ever lowers the flag; - a default that already matches the current value changes no signal, so the writer never runs at all. Either way the flag outlives its turn, and the reader's next write -- a real choice -- silently replaces their history instead of adding to it. So the writer now decides from the values. settle() records the params the URL should say once its change has been applied; the writer replaces only if it is about to write exactly those, and clears the record either way. Both sides go through one currentQueryParams(), so they agree by construction rather than by a comment. The record is taken untracked. settle() is called from inside the details defaulting effect, so reading every param made that effect depend on every param: choosing the Molecule tab re-ran the default, which set the tab straight back to details. The URL never changed and the click did nothing -- caught by e2e/back-button.spec.ts, which now passes both ways again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(hierarchy): write down why the scroll restore takes one frame #181 restores the reader's scroll position on frame one; RevealDirective brings a selected row into view on frame two. That order is what makes `block: 'nearest'` compare against where the reader actually was, so a row already on screen is left alone. Reverse it and clicking a visible row scrolls twice again -- the bug #181 fixed. Neither side said so, and both comments read as if their frame count were a local choice. Noted in both, because either one being "simplified" breaks the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 9, 2026
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
1 participant
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.
Curators, re-testing #137: "no jumping in the search and the analysis table and none in the upper section of event hierarchy, but pathways near the bottom of the hierarchy (e.g. subevents) still jump."
They were right — and the revealing was not the cause.
What was actually happening
Rebuilding the tree starts by emptying it:
Emptying it destroys every row, so the container collapses and the browser resets its scroll to the top. Clicking a sub-event that was already on screen, measured:
A jump to the top and a scroll back. Expansion state was already carried across that rebuild; the scroll position was not.
The fix
Carry the scroll position the same way expansion already is, and restore it again on the next frame — the height is not final until the restored branches have rendered, and the browser clamps a
scrollTopset against a container that is still short.Both halves are tested, because keeping the second one is the whole point of the directive.
What I tried first
Removing the workaround, since the Angular bug it cites is from 2018 and it carries a
//todo: check performance issue. It is still needed — without it the tree renders 29 rows instead of 41 and no branch opens. Left in place, with what it costs now written next to it.Verified to fail without the fix: the already-visible case goes red, the reveal case stays green.
Fixes #137
🤖 Generated with Claude Code