Skip to content

fix(log-viewer): mark every row an inspector pick names, not just the rendered ones - #975

Merged
lcottercertinia merged 5 commits into
certinia:mainfrom
lukecotter:feat-declarative-row-mark
Aug 28, 2026
Merged

fix(log-viewer): mark every row an inspector pick names, not just the rendered ones#975
lcottercertinia merged 5 commits into
certinia:mainfrom
lukecotter:feat-declarative-row-mark

Conversation

@lukecotter

@lukecotter lukecotter commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

📝 PR Overview

The mark that shows where an inspector row's frames sit was a one-shot DOM sweep over the rows a table had rendered. Tabulator builds a row's element on its first render, so the sweep could not reach a row that had never been on screen. Pick a row, then scroll down to a marked row below the viewport or expand a tree row whose children are built afterwards, and those rows arrive unmarked.

The mark now belongs to the table rather than to a list of elements: the marker records the ids it wants, and the row formatter lights a row as it stamps it. That also removes the reason a pick's mark had to wait for the view to finish moving, so the handler #974 added loses its ordering, its awaits and three of its tests.

🛠️ Changes made

  • The mark is declarativeLocatedRowMarker records the wanted ids per table host, and rowIndexStamper / stampRowPath light a row as they stamp it. Every rowFormatter in the app already routes through one of those two, so no table factory needed a marker plumbed into it.
  • The sweep un-lights as well as lights — it toggles rather than adds. Tabulator re-uses a row's element rather than rebuilding it, so a row can come back carrying a mark that has since moved, and a view that switches tables no longer leaves the one it left marked.
  • Mark before movinginspectorLocateHandler marks, then asks the view to move, and waits for nothing. A row the move renders lights itself. Gone with the ordering: the async body, the emphasis read-back, and the three tests that only covered which report won a race.
  • A failed move is still answered, so a view that cannot reach a frame leaves no unhandled rejection.

🧩 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 that was never on screen is marked when you reach it.

🔗 Related Issues

Follows #974, which added the shared handler this simplifies.

✅ Tests added?

  • 👍 yes
  • 🙅 no, not needed
  • 🙋 no, I need help

Four new LocatedRowMarker cases: a row arriving after the mark lights itself, a re-used element loses a stale mark, a table nothing has marked is left alone, and a table the mark has left stops lighting rows.

📚 Docs updated?

  • 🔖 README.md
  • 🔖 CHANGELOG.md
  • 📖 help site
  • 🧪 Marked any pre-release-only features
  • 🙅 not needed

No entry: the Inspector is unreleased, so this belongs to its existing entry. Ticked to record that it was considered.

Anything else we need to know? [optional]

Where to start. log-viewer/src/components/locatedRow.tswantedByHost, stamp and sweep are the whole mechanism. inspectorLocate.ts is what falls out of it, now 16 lines with no async.

What does not happen, since it reads as though it should. A class on a row element survives ordinary scrolling: Row.create() is guarded by this.created, Row.initialize() deletes cells but re-uses the element, RowManager.styleRow adds and removes parity classes rather than assigning className, and our renderer only detaches and re-attaches the element. The gap is the first render, not a later one.

Why a walk up the DOM. stamp finds its table by walking parents until it meets a marked host. The alternative was threading the view's marker through four table factories to reach the formatters. The walk is a handful of nodes per row per render, against cell rendering that is orders of magnitude more, and it keeps the change inside one file.

Test plan.

  • `pnpm lint` and `pnpm test`.
  • Call Tree tab, inspector Call Tree → Bottom Up. Pick a caller row, then scroll the grid down past what was on screen: rows the pick names are marked when you reach them. That is the fix.
  • Expand a tree row under a marked row: the children it builds arrive marked where they should be.
  • Pick a row and press `Escape` quickly: the mark stays gone, including on rows you scroll to afterwards.
  • Pick a row, then hover a different one: the hovered row's frames end up lit.
  • Timeline dims without panning; the Database grids mark without scrolling; Call Tree and Analysis move and mark.
  • 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. No `lana` file is touched here, and it passes in CI.

Follow-ups, not in this PR.

  • The source filter is in nine places. Every `DetailSource`-carrying event is filtered the same way in each view; an `eventBus.onSource` would fold all nine, and with it the four near-identical `inspector:reveal` handlers, so one gesture stops needing two subscriptions per view.
  • A stale move is not abandoned. Clicking two inspector rows quickly lets the earlier move settle last and scroll away from the newer row. The mark is right either way; the scroll is not, and a Tabulator expand and scroll is not cancellable.
  • Whether the Database grids and the flame chart should move on a merged pick. Both can. The shared handler makes that an explicit argument rather than an accident, but changing it is a product decision.

… its mark

Picking an inspector row moves the grid, and the move re-renders the rows the
mark sits on, so the mark has to go on after it. The Call Tree applied the mark
it had read before the move, so dropping the pick while the move ran cleared the
mark and the move then put it back with nothing picked. Hovering another row
landed the same way, with the hover's own mark stripped and never re-applied.

The mark now reads what the inspector is pointing at when the move settles,
which is the emphasis the view already keeps, so there is no second copy of that
truth to fall out of step.

Analysis had the same defect and its own copy of the handler; the Database grids
and the flame chart held a third and fourth copy of the shape, without the move.
All four now share `inspectorLocateHandler`, so whether a pick moves a view is an
argument rather than a property of which handler was copied last. A move that
fails no longer leaves an unhandled rejection.
The mark was a one-shot DOM sweep, adding a class to the rows rendered at that
instant and remembering those elements. The row formatter stamps the row id but
never re-applied the class, and the renderer de-initialises a row scrolled out
of view and builds it again on the way back, so scrolling a grid while a picked
inspector row was lit lost the mark.

The mark now belongs to the table rather than to those elements:
`LocatedRowMarker` records the ids it wants, and the stamp lights a row as it
enters the DOM. The sweep stays, for the rows already rendered when the mark is
set, and it now un-lights as well as lights so a re-used element cannot arrive
carrying an old mark.
The mark had to wait for a pick's move to settle, because the move re-rendered
the rows it sat on and stripped the class. Nothing has to wait now: the table
holds what it wants lit, and a row the move renders lights itself.

That drops the ordering the handler was built around, and with it the read-back
of the emphasis, the awaits, and the three tests that only covered which report
won a race. A move that fails is still answered, so a view that cannot reach a
frame leaves no unhandled rejection.
…w-mark

# Conflicts:
#	log-viewer/src/components/__tests__/inspectorLocate.test.ts
#	log-viewer/src/components/inspectorLocate.ts
@lukecotter lukecotter changed the title fix(log-viewer): keep the inspector's row mark through a scroll fix(log-viewer): mark every row an inspector pick names, not just the rendered ones Aug 28, 2026
The comment claimed the renderer rebuilds a row scrolled out of view and drops
its class. It does not: Row.create() is guarded, Row.initialize() re-uses the
element, and our renderer only detaches it. The gap is a row that has never
been rendered, so the sweep finds no element to mark.
@lcottercertinia
lcottercertinia merged commit 104b9e5 into certinia:main Aug 28, 2026
lcottercertinia pushed a commit that referenced this pull request Aug 28, 2026
# 📝 PR Overview

#975 explained the row mark with a mechanism that does not exist: that
the renderer de-initialises a row scrolled out of view and rebuilds it,
dropping the class. A reader who trusts that comment would expect an
ordinary scroll to lose a mark, and would look in the wrong place when
the mark misbehaves.

What actually happens: `Row.create()` is guarded by `this.created`,
`Row.initialize()` deletes cells but re-uses the element,
`RowManager.styleRow` adds and removes parity classes rather than
assigning `className`, and our renderer only detaches and re-attaches
the element. A class on a row element survives scrolling. The gap the
declarative mark closes is the **first** render: a row that has never
been on screen has no element, so a sweep of what is rendered cannot
reach it.

## 🛠️ Changes made

- Correct the `wantedByHost` comment to name the real gap: a row below
the viewport, or a tree child built after the mark was set.
- Rename one test from "as a scroll back brings one" to "as scrolling to
a new one does", and correct its helper comment, so the test says which
case it guards.

## 🧩 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
- [x] 📝 Documentation - README or documentation site changes
- [ ] 🔧 Chore - dev tooling, CI, config
- [ ] 💥 Breaking change

## 📷 Screenshots / gifs / video [optional]

N/A.

## 🔗 Related Issues

Corrects comments added in #975.

## ✅ Tests added?

- [ ] 👍 yes
- [x] 🙅 no, not needed
- [ ] 🙋 no, I need help

Comments and one test name. The nine `LocatedRowMarker` tests still pass
unchanged.

## 📚 Docs updated?

- [ ] 🔖 README.md
- [ ] 🔖 CHANGELOG.md
- [ ] 📖 help site
- [ ] 🧪 Marked any pre-release-only features
- [x] 🙅 not needed

Nothing user-visible changes.

## Anything else we need to know? [optional]

No code changes: 6 insertions and 6 deletions, all inside comments and
one `it` title.
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