refactor: remove dead code and share test setup by lukecotter · Pull Request #1066 · certinia/debug-log-analyzer

lukecotter · 2026-09-17T18:13:51Z

📝 PR Overview

Removes code the extension no longer runs, and gives the test suites shared setup.
Nothing a user can reach changes.

This is the first of two planned reduction PRs. This one is deletions and test-helper
extraction only, so it cannot alter behaviour. A second PR will share duplicated
production code — the three database grid views, the find and column-view controllers —
and is deliberately kept separate because those diffs touch live paths.

Draft: in progress. More commits are coming on this branch. Each one is a single
theme and lands with pnpm lint and pnpm test green.

🛠️ Changes made

Landed so far:

Drop the sprite timeline render path. The mesh renderers replaced it: FlameChart
uses MeshRectangleRenderer, MeshMarkerRenderer and MeshAxisRenderer, and
SearchOrchestrator uses MeshSearchStyleRenderer. No production file imported the
sprite path. Deletes AxisRenderer (287), SearchStyleRenderer (209),
TimelineMarkerRenderer (192), SpritePool (154), EventBatchRenderer (144) — whose
only importers were each other — plus markers.test.ts (699) and batching.test.ts
(564), which constructed only those classes.

Still to come on this branch:

Drop TimeGridCalculator.ts, which has no importer.
Drop dead exports, two dead shims and three dead CSS selectors.
Drop the unused @rollup/plugin-alias devDependency.
Map #vscode-elements in the jest config, removing 35 copies of the same
jest.mock(…) line.
Shared test helpers: a lit mount/settle pair (28 local copies today), log event
builders (4 near-identical createEvent clones), apex-log fixtures (the same
EXECUTION_STARTED line pasted across 22 files), tabulator and pixi doubles, and the
lana asContext cast (67 occurrences).

🧩 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]

No UI change. The timeline renders through the mesh path before and after.

🔗 Related Issues

None.

✅ Tests added?

👍 yes

sortMarkersByTimeAndSeverity had no coverage outside the deleted markers.test.ts, so
it gains a case in MarkerProcessor.test.ts. The later commits add shared helpers rather
than new cases.

Coverage this PR gives up. markers.test.ts also covered marker viewport culling,
end-time resolution and per-type colour. Those cases asserted sprite tint, alpha, x
and width — internals of the deleted class — so they could not move across as written.
MeshMarkerRenderer keeps visibleIndicators private and had no suite of its own before
this PR either, so the gap is pre-existing rather than new, but it is not closed here.
The fix worth making is extracting MeshMarkerRenderer.render()'s pure per-marker block
into MarkerProcessor, where those three behaviours become directly testable. Filed as
follow-up, not done here.

batching.test.ts needs no replacement: TemporalSegmentTree.test.ts and
RectangleCache.bucket.test.ts already cover the culling, size classification and
bucketing it exercised, against the live path.

📚 Docs updated?

🙅 not needed

No user-visible change, so no CHANGELOG entry. README and help site untouched.

Anything else we need to know? [optional]

Verified per commit with pnpm test, pnpm lint and pnpm build. The build is the
load-bearing check for the deletions — rollup has to resolve every import for the bundles
to be produced.

QuickPickWorkspace.ts has no caller and is kept on purpose: it holds the
multi-root workspace prompt we want to reinstate. It now carries a comment saying so, and
naming RetrieveLogFile.ts:68 and LogView.ts:213 as the two sites that take
workspaceFolders[0] today.

Three things found while reviewing, all left alone as out of scope:

MetricStripRenderer.ts:471 re-sorts the marker array every frame
([...markers].sort) inside the 16.6ms budget, and exception markers are unbounded.
Memoising on array identity is a three-line fix.
SearchHighlightRenderer.ts:150 hardcodes const EVENT_HEIGHT = 15 where its twin
uses TIMELINE_CONSTANTS.EVENT_HEIGHT, which that file already imports.
The frame inset derivation (halfGap, gappedHeight) is repeated across five sites,
and the mesh ones clamp with Math.max(0, …) where HighlightRenderer does not. The
"must match" comments describe that coupling instead of enforcing it.

Out of scope by agreement: apex-log-parser/ is untouched, and the legacy timeline
behind lana.timeline.legacy is a separate piece of work.

The mesh renderers replaced the sprite ones: FlameChart uses MeshRectangleRenderer, MeshMarkerRenderer and MeshAxisRenderer, and SearchOrchestrator uses MeshSearchStyleRenderer. No production file imported the sprite path. Deletes AxisRenderer, SearchStyleRenderer, TimelineMarkerRenderer, SpritePool and EventBatchRenderer, plus markers.test.ts and batching.test.ts, which constructed only those classes. TemporalSegmentTree.test.ts and RectangleCache.bucket.test.ts already cover the culling, size classification and bucketing that batching.test.ts exercised. sortMarkersByTimeAndSeverity had no coverage outside markers.test.ts, so its case moves to MarkerProcessor.test.ts. Marker viewport culling, end-time resolution and per-type colour lose their tests. They asserted sprite tint, alpha, x and width — internals of the deleted class. MeshMarkerRenderer keeps visibleIndicators private and had no suite of its own before this change either.

It holds the multi-root workspace prompt. RetrieveLogFile and LogView take workspaceFolders[0], so a multi-root workspace never gets a choice; this is the logic that gives it one.

TimeGridCalculator has no importer. Its two name hits are its own header and a comment banner in timeAxisConstants.ts. MeshAxisRenderer calls selectInterval directly. Its four comments naming the deleted AxisRenderer go with it.

Each symbol was verified as a single rg hit, its own declaration. Dead exports: APEX_METRIC_COLORS and getMetricColor (already deprecated), METRIC_STRIP_TOGGLE_COLORS, MinimapKeyboardCallbacks, validateMarker, MINIMAP_HEIGHT, LABEL_OFFSET_X/Y, EventDetail, createMockEventTree, and the SearchMatch, HeatStripMetricSnapshot, HeatStripTimeSeriesMetric, SegmentTreeQueryResult and Swimlane* types. MetricStripTimeSeries and MetricStripColors' soql/dml/cpu/heap lose their last readers with the symbols above, so they go too. MinimapViewport.setHeatStripReservation only ever set heatStripReservation to its initial 0, so getChartBottom subtracted a constant zero. The shim, the field and the subtraction go together, which leaves getChartBottom a copy of getHeight; its eight callers now call getHeight directly. TimelineFlameChart's isInitialized @State was written, never read. AppConfig declared a Flow timeline colour that lana/package.json does not contribute and sets additionalProperties false against, so it could never be populated. Flow events map to Workflow through LEGACY_CATEGORY_MAP. datagrid-range-filter declared its own FilterRange, byte-identical to the one in tabulator/filters/MinMax.ts that every other consumer imports. APEX_GOVERNOR_LIMITS_DOC had no consumer; the URL moves into the file header so the provenance survives. Dead selectors: .loading-message, .vs-checkbox-label, .header-bar. Every --lana-* token is live, so none go.

Nothing imports it. rollup.config.mjs takes commonjs, json and node-resolve only; scripts/measure/rolldown.config.ts uses rolldown's own resolve.alias. The lockfile entries are removed by hand. Regenerating drops the supports-color peer annotations throughout, which is 2,176 lines of churn unrelated to this change.

# 📝 PR Overview The timeline built three Pixi apps and four rectangle meshes from copies of the same setup. Each now has one home in `optimised/rendering/`. No behaviour change. Draft until the dev host check below is done. A missing renderer here fails silently — the canvas simply comes up empty — and neither jest nor `pnpm measure` can see it. ## 🛠️ Changes made - **`share the pixi app setup`** — `FlameChart`, `MetricStripOrchestrator` and `MinimapOrchestrator` each held the same ten lines, differing only in `height` and `antialias`. They now call `createTimelineApp`, beside the `destroyTimelineApp` they already shared. `antialias` defaults off; only the metric strip draws lines that need it. - **`share the pixi mesh construction`** — `MeshRectangleRenderer`, `MeshMarkerRenderer`, `MeshSearchStyleRenderer` and `MeshAxisRenderer` each built the same geometry, shader, mesh, label and `addChild` sequence. They now call `createRectangleMesh`. ### Two differences worth stating - **`this.app` is assigned after `init()` resolves**, where before `this.app = new PIXI.Application()` set a not-yet-initialised app ahead of the `await`. Nothing reads it in that window: `FlameChart` wires its `ResizeObserver` only after the first render, and `TimelineFlameChart` guards teardown with an init epoch rather than racing it. Where it would matter it is safer — `resize()` guards on `!this.app`, and the old code could reach an app that had not been initialised. - **The four `private shader` fields are gone**, since the helper owns the shader. Each was assigned once, never read again, and no `destroy()` released it. Holding it was a small retention: `Mesh.destroy()` nulls its own reference, so the field kept the `Shader` alive after the mesh was already gone. `pixiApp.ts` imports pixi as a value now rather than as a type. Its only three importers already did, and no bundler config makes pixi a lazy chunk. ### Left out on purpose - `minimap/MinimapRenderer.ts` builds a fifth mesh and is untouched. It uses `MinimapBarGeometry`, takes no label, and adds itself to a container later. - The `BUCKET_BLOCK` derivation is still written twice. It is five lines of straight-line arithmetic over two exported constants, with nothing that can drift between the sites, and two of its four original sites are deleted by #1066. Better revisited once that lands and the surviving count is settled. ## 🧩 Type of change (check all applicable) - [ ] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [x] ♻️ 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] None yet. The dev host pass below is what would produce them. ## 🔗 Related Issues None. Touches none of the files #1066 deletes, so the two are independent. ## ✅ Tests added? - [ ] 👍 yes - [x] 🙅 no, not needed - [ ] 🙋 no, I need help Both changes are construction-time Pixi code with no testable behaviour of their own, and the existing suites cover the renderers that use them. `pnpm test` is green at 2,625 tests across 191 suites, with `pnpm exec tsc -b --force` and `pnpm exec eslint` clean. There is no automated check that can cover this. `scripts/measure/measure.ts:8` says its harness is "free of the DOM and of PixiJS, so they run under Node", and jest runs under jsdom with no WebGL. Hence the manual pass. ## 📚 Docs updated? - [ ] 🔖 README.md - [ ] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [x] 🙅 not needed Nothing reachable by a user changes. ## Anything else we need to know? [optional] **Dev host check, required before this leaves draft.** Open a `sample-app/` log and confirm on the timeline tab that all six draw: - [ ] frames - [ ] markers - [ ] the time axis - [ ] find highlights - [ ] the minimap - [ ] the metric strip, expanded — the one app built with `antialias: true` --------- Co-authored-by: Luke Cotter <81575432+lcottercertinia@users.noreply.github.com>

# 📝 PR Overview Four small pieces of duplicated logic get one home each. Nothing a user can reach changes. Every commit was checked against the code it replaced rather than assumed equivalent — three of the plan's own predictions turned out to be wrong, and are recorded below. ## 🛠️ Changes made - **`use TableShared in the database grids`** — SOQL, DML and SOSL each re-registered the Tabulator modules and re-declared the clipboard and grouping options that `TableShared` already owns. `registerTableModules` gains a `grouping` flag, and `groupingOptions` is new. `GroupCalcs` and `GroupSort` now declare their table options by module augmentation, matching `AnchoringPolicy`; that retires three `@ts-expect-error` comments. - **`route duration and share maths through the helpers`** — ten hand-rolled ns-to-ms conversions and percent-of-whole calculations now call `core/utility/Duration.ts` and `core/utility/Util.ts`. Six unseparated `1000000` literals and four spellings of the divide-by-zero guard go with them. `formatMs` becomes `formatNsAsMs`: it takes nanoseconds, so at a call site the old name read as a conversion that was not happening. - **`walk event trees through one generator`** — three copies of the same pop-and-push depth-first loop become `walkEvents` in `core/utility/EventTree.ts`, beside `outermostEvents`. A generator, so each caller keeps its own per-event work in its loop body: `namespaceSelfTimes` still checks its frame budget and still returns `null` when a walk is abandoned. - **`export toError and use it`** — ten sites in `lana/` wrote `x instanceof Error ? x.message : String(x)` by hand. `tryCatch.ts` already had that normalisation, private. It is now exported alongside `errorMessage`. ### Three things that look like behaviour changes and are not - The three database grids now also register `AnchoringPolicy`. It is inert without `anchoringPolicy: true`, which none of them sets: `AnchoringPolicy.ts:67` registers the option defaulting to `false`, and `:70-78` gates every listener on it. - `ProgressComponent`'s guard widens from `totalValue !== 0` to `whole > 0`. Both callers feed durations and row counts, so a negative never reaches it. - `errorMessage` returns the same string either way, since `new Error(String(x)).message` is `String(x)`. `log-viewer`'s own two error-normalisation sites stay hand-rolled. It must not import from `lana/`. ### Left out on purpose - `optimised/apex-limit-series.ts` walks every event in the log through a hand-written indexed loop. Swapping in a generator there needs a measured before and after, and `pnpm measure` has no stage that calls `apexLimitTimeSeries`. - `core/log/frameVariables.ts` pushes children back to front on purpose, so popping yields them in log order. `walkEvents` would have to offer that ordering first. - The two `categorySelfTimes` functions are **not** duplicates, despite looking it. One starts at `root.children` and the other at `root`; one buckets uncategorised events into `OTHER_CATEGORY` and the other drops them; one keys by `string` and is memoised, the other keys by `LogCategory` and is not. Deleting either changes what the UI shows. ## 🧩 Type of change (check all applicable) - [ ] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [x] ♻️ 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] None — no UI change. ## 🔗 Related Issues None. ## ✅ Tests added? - [x] 👍 yes `log-viewer/src/core/utility/__tests__/EventTree.test.ts` is new: 7 cases for `walkEvents`, including a 100,000-deep chain and early termination by the caller. The full gate ran on every commit — `pnpm exec tsc -b --force`, `pnpm exec eslint`, `pnpm test`. 2,632 tests across 192 suites, green. ## 📚 Docs updated? - [ ] 🔖 README.md - [ ] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [x] 🙅 not needed No CHANGELOG entry: nothing here is reachable by a user. The plan that produced this work predicted one commit would close an `AnchoringPolicy` gap in the database grids and so need an entry. It does not — see above. ## Anything else we need to know? [optional] **Not checked in the dev host.** Worth one pass on the SOQL, DML and SOSL tabs before merge: grouping still groups, group headers still show calcs when a group is closed, `Cmd/Ctrl+C` copies the whole table, CSV download works. Tabulator's module registry is barely exercised under jsdom. The percentage and duration readings in those grids, in `StackedTimeBar`, in the `GovernorSummary` gauges and in the database overview are the other thing to glance at. Independent of #1066 and #1069 — no file overlap, so these can merge in any order.

# 📝 PR Overview Gives the test suites shared setup. Second of two reduction PRs, and **stacked on #1066** — its base is `refactor-drop-dead-code`, so review that one first and this diff stays readable. The suites are 43,933 lines against 77,031 of source. Most of the excess is the same setup written again in each file: 28 local `mount` helpers, four near-identical `createEvent` builders, the same `EXECUTION_STARTED` fixture line pasted across 22 files. This PR gives each of those one home. Nothing here changes shipped code. The one production-file edit in the first commit is a comment. **Draft: in progress.** More commits are coming on this branch; each lands with `pnpm lint` and `pnpm test` green, and I check the passing-test count is unchanged so a shared helper cannot quietly stop a case running. ## 🛠️ Changes made Landed so far: - **Mock `#vscode-elements` through the jest config.** 21 suites carried 35 copies of `jest.mock('#vscode-elements/…', () => ({}))`. jsdom's `ElementInternals` has no `setFormValue`, so a form-associated element fails on its first update, and `vscode-icon` warns on every connect about the missing codicon stylesheet — a library-wide fact, not a per-suite decision. `TimelineKey` and `DockLayout` each stubbed a whole component to dodge the same problem and now exercise the real `OverflowList` and `DetailDock`. Still to come on this branch: - A shared lit `mount`/`settle`/`cleanupDom` (28 local copies today). - Shared log event builders (four near-identical `createEvent` clones, ~250 lines). - Shared apex-log fixtures (the same header lines pasted across 22 files). - Tabulator, pixi and viewport doubles, plus 55 lines of an unreachable `__mocks__` file. - The lana `asContext` cast (67 occurrences) and `lastRegisteredCommand`. - `it.each` collapses, and one governor-tier policy currently asserted twice. ## 🧩 Type of change (check all applicable) - [ ] 🐛 Bug fix - something not working as expected - [ ] ✨ New feature – adds new functionality - [x] ♻️ 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] No UI change. ## 🔗 Related Issues Stacked on #1066. ## ✅ Tests added? - [x] 🙅 no, not needed This PR changes how tests are set up, not what they cover. The bar for every commit is that the passing-test count does not move: **2,567** before and after. Two behaviour widenings are worth a reviewer's attention rather than being buried: - The `#vscode-elements` rule went from opt-in to blanket. 38 suites reach such an import transitively and 21 already mocked, so 17 now get the stub where they previously loaded the real module. A control run with the mapper removed fails 18 suites and 260 tests, which is what proves the stub is load-bearing and that no passing assertion was weakened. - `TimelineKey` and `DockLayout` no longer stub out `OverflowList` and `DetailDock`, so those two suites now cover more than they did. `vscode-single-select` is exempt from the mapper: `VsSelect` extends `VscodeSingleSelect` and spreads its static styles, so a stub cannot be extended. That exemption currently holds only because no suite mounts a `<vs-select>` — the real element is form-associated and would hit the missing `setFormValue`. `VsSelect` carries a comment saying so. The clean fix is an `ElementInternals` no-op polyfill beside the existing `ResizeObserver` one in `setup.ts`; filed as a follow-up rather than smuggled in here. ## 📚 Docs updated? - [x] 🙅 not needed Test-only change, so no CHANGELOG entry. ## Anything else we need to know? [optional] Verified per commit with `pnpm lint` and `pnpm test`, and by comparing the passing-test count before and after. Out of scope by agreement: `apex-log-parser`, and the legacy timeline behind `lana.timeline.legacy`.

Three conflicts: - lana/src/workspace/AppConfig.ts — main removed the whole timeline `colors` block; this branch had removed only its `Flow` field. Took main's. - log-viewer/src/features/timeline/types/flamechart.types.ts — main removed SearchOptions, FindEventDetail and FindResultsEventDetail; this branch removed SearchMatch. Took both, which empties the SEARCH & HIGHLIGHT section, so its banner goes too. - log-viewer/src/features/timeline/__tests__/batching.test.ts — deleted here, and main's only change to it was the parser package rename. Kept the deletion, since EventBatchRenderer goes with it. Re-verified the deletions against main, which has gained MinimapAxisRenderer, ClockTimeAxisRenderer and ElapsedTimeAxisRenderer since this branch was vetted. None of them reference the five sprite renderers or TimeGridCalculator.

lukecotter added 5 commits September 17, 2026 18:45

docs(lana): say why QuickPickWorkspace has no caller

8564175

It holds the multi-root workspace prompt. RetrieveLogFile and LogView take workspaceFolders[0], so a multi-root workspace never gets a choice; this is the logic that gives it one.

lukecotter mentioned this pull request Sep 18, 2026

refactor(test): share test setup across the suites #1069

Merged

3 of 9 tasks

lcottercertinia marked this pull request as ready for review September 18, 2026 18:55

lcottercertinia previously approved these changes Sep 18, 2026

View reviewed changes

lukecotter mentioned this pull request Sep 20, 2026

🔧 chore: restore test coverage for the Mesh timeline renderers #1075

Closed

This was referenced Sep 29, 2026

refactor: give duplicated helpers one home #1089

Merged

refactor(log-viewer): share the pixi app and mesh setup #1090

Merged

lukecotter dismissed lcottercertinia’s stale review via fd35014 September 29, 2026 13:28

lcottercertinia approved these changes Sep 29, 2026

View reviewed changes

lcottercertinia merged commit f650fbe into certinia:main Sep 29, 2026
8 checks passed

Navigation Menu

refactor: remove dead code and share test setup - #1066

refactor: remove dead code and share test setup#1066
lcottercertinia merged 6 commits into
certinia:mainfrom
lukecotter:refactor-drop-dead-code

lukecotter commented Sep 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lukecotter commented Sep 17, 2026

📝 PR Overview

🛠️ Changes made

🧩 Type of change (check all applicable)

📷 Screenshots / gifs / video [optional]

🔗 Related Issues

✅ Tests added?

📚 Docs updated?

Anything else we need to know? [optional]

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants