Skip to content

refactor: give duplicated helpers one home - #1089

Merged
lcottercertinia merged 5 commits into
certinia:mainfrom
lukecotter:refactor-share-duplicated-code
Sep 29, 2026
Merged

lcottercertinia merged 5 commits into
certinia:mainfrom
lukecotter:refactor-share-duplicated-code

Conversation

@lukecotter

Copy link
Copy Markdown
Collaborator

📝 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
  • ♻️ 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?

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

SOQLView, DMLView and SOSLView each re-registered the Tabulator modules
and re-declared the clipboard and grouping options that TableShared
already owns. Route all three through registerTableModules, which gains
a `grouping` flag, and the new groupingOptions const.

BottomUpTable drops its own bare registerModule call for the same three
grouping modules, and adopts clipboardCopyOptions and groupingOptions.

groupToggleElement stays per table. A grid with a groupClick handler
sets false; BottomUpTable has none and needs 'header'.

GroupCalcs and GroupSort declare their table options through module
augmentation, matching AnchoringPolicy. That retires two internal
@ts-expect-error comments and the one in BottomUpTable.

The three views now also get AnchoringPolicy. That is inert without
`anchoringPolicy: true`, which none of them sets, so behaviour is
unchanged and no CHANGELOG entry is due.
Ten hand-rolled ns-to-ms conversions and percent-of-whole calculations
now call core/utility/Duration.ts and core/utility/Util.ts. That retires
six unseparated 1000000 literals and four spellings of the
divide-by-zero guard.

formatMs becomes formatNsAsMs. It takes nanoseconds, so at a call site
the old name read as a conversion that was not happening.

No behaviour change. ProgressComponent's guard widens from
`totalValue !== 0` to `whole > 0`; both its callers feed durations and
row counts, so a negative never reaches it. StackedTimeBar's readout
gains a zero guard it cannot use, since render() already returns early
on `denominator <= 0`.
Three copies of the same pop-and-push depth-first loop become one
walkEvents generator 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.

Line count barely moves: the helper costs what the three loops saved.
The point is one named walk with tests, instead of three hand-rolled
stacks with none.

Two sites the plan names are not in this commit:

- optimised/apex-limit-series.ts:138 walks every event in the log to
  build the metric strip, with 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:1048 pushes children back to front on
  purpose, so popping yields them in log order. walkEvents would have to
  offer that ordering first.
Ten sites wrote `x instanceof Error ? x.message : String(x)` by hand.
tryCatch.ts already had that normalisation, private; it is now exported
alongside errorMessage, which is the `.message` all ten wanted.

No behaviour change: errorMessage gives the same string either way,
since new Error(String(x)).message is String(x).

log-viewer's two sites stay as they are. It must not import from lana.
…icated-code

# Conflicts:
#	log-viewer/src/features/timeline/utils/category-self-time.ts
@lcottercertinia
lcottercertinia merged commit 56b5ca7 into certinia:main Sep 29, 2026
8 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