fix(log-viewer): mark a caller row by the caller, not the calls it made - #980
Merged
lcottercertinia merged 3 commits intoSep 1, 2026
Merged
Conversation
Four views each answered three source-keyed events, so the same source test sat in nine places and the same three unsubscribe fields in four. The bus gains onSource, and wireInspectorTab names the set once: a view supplies only what it does differently and gets one unsubscribe.
The sweep reaches only the rows a table has attached, so a row lit while on screen kept the class when the mark moved away and it was scrolled out. The formatter does not run again for a row the renderer only re-attaches, so the old mark came back with it. litByHost records what either half lit.
A grid row under the pointer told the inspector every call it counts, so a Bottom Up caller row marked rows for the leaf calls rather than for itself. rowFrames climbs to the row's own depth, as the forward direction has since certinia#973, reading the direction the table shows so a top-down row does not.
lcottercertinia
approved these changes
Sep 1, 2026
15 tasks
lcottercertinia
pushed a commit
that referenced
this pull request
Sep 1, 2026
…g it (#981) # 📝 PR Overview The inspector's row mark reached a row two ways: a sweep of the rows attached when the mark moved, and the row formatter as a row is built. Neither reaches a row that was built earlier, detached when it scrolled out of view, and only then named by a mark. The renderer re-attaches such a row without re-running the formatter, so it comes back with no mark. Scroll to it, or sort with it just off screen, and the highlight is simply missing. The fix watches the arrival: a `MutationObserver` for `childList` on the element the renderer attaches rows to. A row entering the table is a child mutation, so a scroll and a structural render are one case, and a table destroyed and rebuilt into the same container is watched again rather than going quiet. ## 🛠️ Changes made - **`watchRenders(host)`** — one observer per marked table, established on the first non-empty mark. The callback re-sweeps only while a mark is set, and touches nothing but a class, so it cannot report itself back. - **Keyed on the row-holding element, not the container.** Several views destroy the table and build another in the same element; `CallStackDetail` does it on every event change. Keying on the container would leave the watch pointing at a destroyed table, and the old observer is disconnected when a new one takes over. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [ ] ♻️ Refactor - internal changes with no user impact - [ ] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 Screenshots / gifs / video [optional] N/A. What changes is whether a row carries its highlight when the grid hands it back. ## 🔗 Related Issues Completes the row mark from #975 and #980. ## ✅ Tests added? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help Two cases: a row named while detached, and a table rebuilt into the same container. Each guard was proven by reverting the code it covers — no watch fails both; watching the spacer's `style` rather than the arrival fails both; keying so a rebuild is skipped fails the second. ## 📚 Docs updated? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed No entry: this corrects the unreleased Inspector, so it belongs to its existing entry. Ticked to record that it was considered. ## Anything else we need to know? [optional] **Why not the signals that look more natural.** Two were tried and rejected with evidence: - **A scroll listener.** A structural render — sort, filter, column show/hide, a tree re-expand of children already built — re-attaches an initialised row with no formatter run and fires no scroll event at all. The renderer also documents \`scrollend\` as unreliable: "The RAF stability check deliberately replaces \`scrollend\`, which never fires while the scrollbar thumb is held still" (\`VirtualVerticalRenderer.ts:430\`). - **The virtual spacer paddings.** Reading their values is unsound, because \`ScrollAnchor\` writes them too, so a value can return to one already swept while the window has moved. Observing the write instead is no better: a CSSOM write of an unchanged value queues no mutation record at all, so a sort at the top of a table reports nothing. **Cost.** The observer fires when rows enter or leave, which is exactly when work is due, and only while a mark is set. A sweep reads what the table has attached — the viewport plus at most \`OVERSCAN_MAX\` rows each side — never the row count, and the renderer's idle prewarm builds cells with \`inFragment: true\` so it does not widen that. The callback runs in a microtask after the render, and touches only \`classList\`, so it forces no layout. **Test plan.** - \`pnpm lint\` and \`pnpm test\`. - Call Tree, Bottom Up, Inspector open. Click a caller row so grid rows mark, then **sort a column while at the top of the table**: the marks survive. That is the case a value-based signal missed. - Filter and clear it; collapse and re-expand a marked row's parent. - Scroll past rows never rendered and back. Then click a row, scroll a lit row out of view, press \`Escape\`, scroll back: no stale highlight. - Large log with a mark set: scroll hard and watch for jank. - Both themes, Inspector docked at the side and at the bottom. **Known unrelated failure locally.** \`lana/src/services/__tests__/servicesRuntime.test.ts\` cannot resolve \`effect\` in a worktree not installed since #951. It passes in CI. **Follow-up, not in this PR.** The renderer already knows which rows it attached: \`allAttached\` is built in \`_attachRanges\`. Dispatching that would make relighting O(rows attached) rather than O(rows rendered), and passing the \`Tabulator\` to \`mark()\` instead of its element — every one of the eight owners already holds it — would add disposal on \`tableDestroyed\` and let \`Find\` drop its own scroll listener onto the same event.
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
2 participants
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.
📝 PR Overview
Two bugs in the inspector's row mark, both about which rows light up.
A grid row told the inspector the wrong thing. A row under the pointer emitted every call it counts, so a Bottom Up caller row marked the inspector rows for the leaf calls underneath it rather than for the caller itself. The forward direction stopped doing that in #973; the reverse direction was still on the old rule, so the two disagreed depending on which side you pointed at.
A mark could come back after being dropped. The sweep reaches only the rows a table has attached. A row lit while on screen, then scrolled out, kept the class when the mark moved away, and the renderer re-attaches such a row without running the row formatter again, so the stale highlight returned with it.
🛠️ Changes made
rowFrames(row, root, direction)— a bottom-up caller row climbs to its own depth,depthOf(_pathId) - 1hops above each call it counts. A top-down row already sits at its frames' depth, so it climbs nothing.LogStore.framesAbove— the climb, next tostackByEventIndex, which already owned this parent-pointer walk. The inspector'sframeEventIndexesnow reads it too, so the two sides cannot drift.directionOf(this.viewMode), which the sibling_emitDetailSelectionalready uses. No new parameter and no second source of truth.litByHost— what each table's mark has lit, whichever half lit it, so a new mark can un-light an element the renderer has since detached.detail:locatedoc described the old rule; it now says what the protocol carries.🧩 Type of change (check all applicable)
📷 Screenshots / gifs / video [optional]
N/A. What changes is which rows carry the highlight.
🔗 Related Issues
Follows #973, which changed the forward direction, and #975, which shipped the mark mechanism the second fix corrects.
✅ Tests added?
Three
rowFramescases and one for the detached row. Each guard was proven by reverting the code it covers: dropping the direction check makes a top-down row climb; removing the shared climb fails onerowFramestest and twoframeEventIndexestests, which also shows the inspector test runs the real method rather than a copy of it; restoring the old clearing fails the detached-row test.📚 Docs updated?
No entry: both fixes correct the unreleased Inspector, so they belong to its existing entry. Ticked to record that it was considered.
Anything else we need to know? [optional]
Where to start.
LogStore.framesAboveis the mechanism;rowFramesis the grid's use of it andframeEventIndexesthe inspector's.Why the mark has to remember what it lit. Tabulator builds a row's element once (
Row.create()is guarded by `this.created`) and `Row.initialize()` re-uses it, and both `deinitialize()` calls in Tabulator are inside `reinitializeRows()`, a column-layout path. So an ordinary scroll neither rebuilds the element nor re-runs the formatter: the class persists, and clearing it has to reach elements the query cannot see.On the dedupe. A merged row aggregates distinct caller frames that share a signature, so the climb is many-to-many and the answer can be as long as what was asked about. It is not safe to climb from one call and assume the rest agree.
Test plan.
Known unrelated failure locally. `lana/src/services/tests/servicesRuntime.test.ts` cannot resolve `effect` in a worktree not installed since #951. It passes in CI.
Follow-up, not in this PR.
deriveCallsalready walks each call's parents insidechainReachesand stops exactly at the frame the row is, then keeps the index and throws the frame away, soframesAbovere-walks the same edges. Fusing the two means havingchainReachesreturn the node it stopped at.