Skip to content

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

Open
lukecotter wants to merge 5 commits into
certinia:mainfrom
lukecotter:refactor-drop-dead-code
Open

lukecotter wants to merge 5 commits into
certinia:mainfrom
lukecotter:refactor-drop-dead-code

Conversation

@lukecotter

Copy link
Copy Markdown
Collaborator

📝 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.
@lcottercertinia
lcottercertinia marked this pull request as ready for review September 18, 2026 18:55
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