refactor(log-viewer): give a tab one wiring call to the inspector - #977
Merged
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.
15 tasks
lcottercertinia
approved these changes
Sep 1, 2026
lcottercertinia
pushed a commit
that referenced
this pull request
Sep 1, 2026
…de (#980) > **Stacked on #977.** Its commit shows in this diff until that merges. Review from `be6412ea` onward. # 📝 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) - 1` hops above each call it counts. A top-down row already sits at its frames' depth, so it climbs nothing. - **`LogStore.framesAbove`** — the climb, next to `stackByEventIndex`, which already owned this parent-pointer walk. The inspector's `frameEventIndexes` now reads it too, so the two sides cannot drift. - **The direction is read at hover time**, from `directionOf(this.viewMode)`, which the sibling `_emitDetailSelection` already 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. - **The `detail:locate` doc** described the old rule; it now says what the protocol carries. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [x] ♻️ Refactor - internal changes with no user impact - [ ] ✨ New feature – adds new functionality - [ ] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 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? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help Three `rowFrames` cases 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 one `rowFrames` test **and** two `frameEventIndexes` tests, 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? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed 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.framesAbove` is the mechanism; `rowFrames` is the grid's use of it and `frameEventIndexes` the 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.** - \`pnpm lint\` and \`pnpm test\`. - Call Tree, **Bottom Up**, Inspector open. Hover a bucket row, then a caller row one level down: the inspector mark moves up the stack with you rather than staying on the leaf calls. - Step two and three levels up: one frame per level. - Analysis: same, hovering a row under a method bucket. - **Aggregated** and **Time Order**: unchanged, since a row there already sits at its own frames' depth. - Hover an inspector row so a grid row lights, scroll that row out of view, move the pointer off the inspector row, then scroll back: no highlight. That is the second fix. - 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.** `deriveCalls` already walks each call's parents inside `chainReaches` and stops exactly at the frame the row is, then keeps the index and throws the frame away, so `framesAbove` re-walks the same edges. Fusing the two means having `chainReaches` return the node it stopped at.
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
Three events reach every tab's view and each view answered only its own, so the same
detail.source === '<tab>'test sat in nine places. Behind that was a bigger repeat: all four views subscribe to the same three events, hold three nullable unsubscribe fields, and undo them one at a time in teardown.ApexLogTimelinespent twelve lines on it.The bus gains
onSource(event, source, callback), which delivers only what names a tab. On top of that,wireInspectorTabnames the set of three once and returns one unsubscribe, so a view supplies only what it does differently. Net 90 insertions against 291 deletions.🛠️ Changes made
eventBus.onSource— aSourcedEventtype admits only the payloads that name a tab, so the source test lives on the bus. No cast is needed:EventMap[K]already resolves to the union of sourced payloads.wireInspectorTab(source, emphasis, sync)— one call per view, one unsubscribe. A view givesmark,reveal,clear, andmovesToMergedPickwhere a picked row that merges occurrences should also move.revealfor both kinds of pick. The Call Tree and Analysis views were passing the same method twice, once for a single frame and once for the first of several.movesToMergedPicknow says when it moves, and the helper catches a rejection, so no view needsvoidon a promise.inspectorLocate.tsis gone, absorbed intoinspectorTab.ts. Its doc no longer has to ask the caller to subscribe it a particular way, since the module does the subscribing.🧩 Type of change (check all applicable)
📷 Screenshots / gifs / video [optional]
N/A. Nothing changes on screen.
🔗 Related Issues
Follows #974, #975 and #976, which built the inspector mark this wiring carries.
✅ Tests added?
Nine
wireInspectorTabcases driven through the real bus, and three foronSource. Three guards were proven by reverting the code they cover: returning only the first unsubscribe, dropping themovesToMergedPickgate, and swapping mark with move each fail a test. The last needed an ordered log, since two separate lists cannot show order; the ordering had no test before this.📚 Docs updated?
Nothing user-visible changes.
Anything else we need to know? [optional]
Where to start.
log-viewer/src/components/inspectorTab.tsis the whole mechanism, then any one view to see what a caller now looks like.What was deliberately left.
SourcedEventalso admitsdetail:select,detail:viewanddetail:locate. Those must not be filtered by source: the inspector records every tab's selection, so filtering would lose the tab it is not showing. The type's doc says so, since the constraint belongs to the caller rather than the type.Test plan.
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.