Skip to content

[profiler] Follow the application theme - #6953

Open
Karakatiza666 wants to merge 1 commit into
redesign-profiler-diagram-12from
redesign-profiler-diagram-13
Open

Karakatiza666 wants to merge 1 commit into
redesign-profiler-diagram-12from
redesign-profiler-diagram-13

Conversation

@Karakatiza666

@Karakatiza666 Karakatiza666 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Part of #6895, split one commit per PR.

The diagram was drawn from one palette in a console that has two, so it
stayed light while everything around it went dark. Visualizer.setTheme
switches the palette on a diagram already on screen: the stylesheet is
rebuilt, the observers are told, and the chip images - the one piece of
per-node data with the palette baked into it - are rewritten in a batch.
Nothing is laid out again, so nothing moves.

The prop is threaded down through SupportBundleViewerLayout and
ProfilerLayout to ProfilerDiagram, where it is applied from an effect
of its own: the effect that builds the visualizer disposes and rebuilds
it, so tracking the theme there would reparse the profile and re-run the
ELK layout on every toggle. The profile viewer feeds it from
useDarkMode.

Describe Manual Test Plan

Toggle the console between light and dark with a profile open. The diagram follows, and nothing moves: no relayout runs.

Verified at this commit, not just at the tip of the stack: checked out detached with js-packages/profiler-lib/dist deleted and rebuilt from this commit's source, then profiler-lib bun run check and bun run test, and profiler-layout bun run check and bun run test (all three vitest projects, browser suites included). All four green.

Checklist

  • Unit tests added/updated
  • Integration tests added/updated
  • Documentation updated
  • Changelog updated

Breaking Changes?

Mark if you think the answer is yes for any of these components:

  • OpenAPI / REST HTTP API / feldera-types / manager
  • Feldera SQL (Syntax, Semantics)
  • feldera-sqllib (incl. dependencies fxp, etc.)
  • Python SDK
  • fda (CLI arguments)
  • Adapters (including configuration)
  • Storage Format / Checkpoints
  • Others (specify)

Describe Incompatible Changes

None. The change is confined to js-packages/.

// cannot ride along on the next `updateGraph`.
this.cy.startBatch();
for (const node of this.cy.nodes().toArray()) {
refreshChips(node, theme);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshChips(node, theme) defaults hovered to false, so a theme toggle made while the pointer rests on a composite replaces the expand/collapse control with the leaf count — and it stays wrong until the pointer leaves the node and comes back, since installChipButtons only rewrites on mouseover/mouseout/layoutstop.

Verified locally with a throwaway vitest on a headless graph: refreshChips(p, 'light', true) then the setTheme loop gives exactly nodeChips(true, 3, 'dark')[1] — the count chip.

Cheapest fix is to let the chip buttons own it: give installChipButtons a themeChanged observer hook that re-applies the hovered node with hovered = true, and skip that node here.


/* The minimap sits on top of the graph, so it needs its own ground in either palette. */
.visualizer-wrapper[data-diagram-theme='dark'] .visualizer-navigator {
background-color: rgba(34, 38, 43, 0.95);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgba(34, 38, 43, 0.95) is DIAGRAM_PALETTES.dark.heatLow (#22262b) hand-copied into CSS, and the light rule above is the same copy of #ffffff. The palette is now exported from profiler-lib's index, so nothing pins these two together — change the dark ground in diagramTheme.ts and the minimap's panel silently drifts off the diagram it floats over. Consider setting the background from DIAGRAM_PALETTES[theme] (a style: binding or a CSS custom property on the wrapper) instead of a second literal.

@mythical-fred-oss

Copy link
Copy Markdown

Read the diff plus the surrounding theme plumbing (diagramTheme.ts, navigator.ts, chipButtons.ts), and ran, on this merge commit:

What Result
profiler-lib: bun run check, bun run test ✅ 251 tests, 13 files
profiler-layout: bun run check, bun run test (browser projects, playwright installed) ✅ 46 tests, 10 files
web-console: bun run test-unit 603/604; the one failure is MetricsTables.svelte.spec.ts timing out under load, passes in isolation and is untouched here
pre-commit run --files <changed>

The mechanics hold up: cy.style() is the only stylesheet path (no element bypasses to be wiped), nodeText and installChipButtons read the theme through getters so they need nothing, navigator.setTheme repaints the existing picture rather than waiting for a layout, refreshChips over every node is idempotent for leaf_count === 0, and setTheme after dispose() is a no-op because Visualizer.dispose nulls rendering. The $effect.root in the init effect does keep theme untracked there, so the no-relayout claim is real.

Gate this misses: unit tests — the checklist says "Unit tests added/updated" but no test file changed. The live switch is only exercised two PRs later (#6955's await setTheme('dark')), whereas setTheme lands here and is unit-testable today: I pinned both the chip rewrite and the hover regression in ~20 lines against a headless cytoscape instance, in the shape diagramTheme.test.ts already uses. Uncovered beyond that: theme toggled while a node is hovered (see inline — a real, if small, regression), and toggling with no visualizer yet (instance === null, then built from config.theme). Minor: profiler-app mounts SupportBundleViewerLayout without the prop and so is pinned to 'light', which is fine while it has no theme switch, but the ?: DiagramTheme = 'light' default means any future consumer stays light silently rather than failing to compile.

@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 604c812 to 12568e6 Compare August 26, 2026 07:38
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 12568e6 to 149d010 Compare September 1, 2026 14:06
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch 2 times, most recently from d586d71 to 718f612 Compare September 1, 2026 17:55
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 718f612 to 712e7a4 Compare September 15, 2026 09:33
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 712e7a4 to 51ba8db Compare September 15, 2026 12:08
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 51ba8db to 613befc Compare September 15, 2026 16:59
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 613befc to 8e2c073 Compare September 15, 2026 18:44
The diagram was drawn from one palette in a console that has two, so it
stayed light while everything around it went dark. `Visualizer.setTheme`
switches the palette on a diagram already on screen: the stylesheet is
rebuilt, the observers are told, and the chip images - the one piece of
per-node data with the palette baked into it - are rewritten in a batch.
Nothing is laid out again, so nothing moves.

The prop is threaded down through `SupportBundleViewerLayout` to
`ProfilerDiagram`, where it is applied from an effect of its own: the
effect that builds the visualizer disposes and rebuilds it, so tracking
the theme there would reparse the profile and re-run the ELK layout on
every toggle. The profile viewer feeds it from `useDarkMode`.

Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
@Karakatiza666
Karakatiza666 force-pushed the redesign-profiler-diagram-13 branch from 8e2c073 to dbe4e55 Compare September 22, 2026 20:47

This branch has not been deployed

No deployments
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.

1 participant