Skip to content

fix(log-viewer): mark a row the renderer hands back without rebuilding it - #981

Merged
lcottercertinia merged 1 commit into
certinia:mainfrom
lukecotter:feat-mark-on-render
Sep 1, 2026
Merged

fix(log-viewer): mark a row the renderer hands back without rebuilding it#981
lcottercertinia merged 1 commit into
certinia:mainfrom
lukecotter:feat-mark-on-render

Conversation

@lukecotter

Copy link
Copy Markdown
Collaborator

📝 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)

  • 🐛 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?

  • 👍 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
  • 🔖 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.

…g it

The mark reached a row two ways: a sweep of what was attached when it moved,
and the row formatter as a row is built. Neither reaches a row built earlier,
detached when scrolled out, and only then named: the renderer re-attaches
such a row without running the formatter. A MutationObserver on the
row-holding element sweeps again as rows arrive, so a scroll and a sort are
one case, and a table rebuilt in place is watched again rather than going
quiet.
@lcottercertinia
lcottercertinia merged commit 2fb6c43 into certinia:main Sep 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants