[profiler] Add the diagram palettes, stylesheet and corner chips - #6944
[profiler] Add the diagram palettes, stylesheet and corner chips#6944Karakatiza666 wants to merge 1 commit into
Conversation
| export function formatLeafCount(count: number): string { | ||
| if (count < 1000) { | ||
| return String(count); | ||
| } | ||
| const [divisor, suffix] = count < 1_000_000 ? [1000, 'K'] : [1_000_000, 'M']; | ||
| const scaled = count / divisor!; | ||
| return `${scaled < 10 ? Math.round(scaled * 10) / 10 : Math.round(scaled)}${suffix}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
formatLeafCount never rolls over to the next unit, so the label can exceed the 5 glyphs BADGE_CANVAS_WIDTH is sized for. Verified by running it:
| count | label | glyphs |
|---|---|---|
| 999_499 | 999K |
4 |
| 999_500 | 1000K |
5 (should be 1M) |
| 1_000_000_000 | 1000M |
5 |
| 12_000_000_000 | 12000M |
6 |
badgePillWidth clamps the pill with Math.min, but glyphRun is still laid out at label.length * CHIP_GLYPH_WIDTH, so past 6 glyphs the count runs off the pill and is clipped by the canvas. Rounding first and then re-checking the divisor fixes both (999_500 → 1M, 1e9 → 1000M capped or a B suffix). Neither the 1000K case nor a >= 1e9 count is in the tests.
| it('keeps the badge pill inside its canvas for the widest label', () => { | ||
| // The badge canvas is a fixed size in the stylesheet; a pill wider than the canvas would | ||
| // be clipped, and a taller one would distort. | ||
| const svg = decode(nodeChips(false, 999_999_999, 'light')[1]!) | ||
| expect(attr(svg, 'rect', 'width')).toBeLessThanOrEqual(attr(svg, 'svg', 'width')) | ||
| }) |
There was a problem hiding this comment.
This assertion cannot fail: badgePillWidth is Math.min(BADGE_CANVAS_WIDTH, …), so the rect is ≤ the canvas for any label, widest or not. The invariant that actually holds the chip together is that the glyph run stays inside the pill — text x + textLength <= pill.x + pill.width. Asserted that way it would catch the overflow described on formatLeafCount.
| // Measured by `labelWidth` rather than by cytoscape, so the room a counter chip needs | ||
| // can be added to it. An expanded region ignores this and its height both, and sizes | ||
| // itself to its children. | ||
| 'width': 'data(text_width)', |
There was a problem hiding this comment.
Both data() mappings in this sheet — width: data(text_width) here and min-width: data(min_width) at L238 — are untested, and cytoscape fails them silently. Every node in diagramTheme.test.ts is built without those two fields, and headless cytoscape resolves the node to its default width of 30 rather than erroring:
no text_width -> width = 30
with text_width: 120 -> width = 120
So a rename or a typo in either key ships as "every node is 30px wide" with the suite green — the same class of silent failure the file's header comment sets out to pin. One node carrying text_width/min_width and an assertion on the resolved width would close it.
| export function labelWidth(text: string): number { | ||
| if (labelContext === undefined) { | ||
| labelContext = typeof document === 'undefined' | ||
| ? null | ||
| : document.createElement('canvas').getContext('2d'); | ||
| if (labelContext !== null) { | ||
| labelContext.font = labelFont(); | ||
| } | ||
| } | ||
| if (labelContext === null) { | ||
| return Math.ceil(text.length * LABEL_GLYPH_FALLBACK); | ||
| } | ||
| return Math.ceil(labelContext.measureText(text).width); |
There was a problem hiding this comment.
labelWidth is the one piece of real logic in this module — two branches, a memoized context, a fallback constant — and it has no test. It is even imported into diagramTheme.test.ts (L23, along with REGION_PADDING at L29) and never used; tsconfig.json excludes *.test.ts, so noUnusedLocals does not catch the dead imports.
Worth pinning at least: the no-DOM fallback (text.length * LABEL_GLYPH_FALLBACK, ceiled), monotonicity in the length of the text, and the empty string. The measureText branch needs the browser project.
| const palette = DIAGRAM_PALETTES.light | ||
| const [border, edge] = [palette.border, palette.edge] | ||
| try { | ||
| palette.edge = '#123456' | ||
| const cy = graph('light') | ||
| expect(cy.$id('e').style('line-color')).toBe(hexToRgb('#123456')) | ||
| expect(cy.$id('plain').style('border-color')).toBe(hexToRgb(border)) | ||
|
|
||
| palette.border = '#654321' | ||
| const repainted = graph('light') | ||
| expect(repainted.$id('plain').style('border-color')).toBe(hexToRgb('#654321')) | ||
| expect(repainted.$id('e').style('line-color')).toBe(hexToRgb('#123456')) | ||
| } finally { | ||
| palette.border = border | ||
| palette.edge = edge |
There was a problem hiding this comment.
Mutating the exported DIAGRAM_PALETTES singleton to prove the two entries are distinct. The finally restores it and vitest isolates files, so it is not flaky today, but it is a trap for whoever adds the next test: chips.ts caches chip URIs keyed on the theme name, so a palette repainted mid-suite yields chips built from the old colors, and ARCHITECTURE.md lists "no global state or singletons" for this package.
Making DiagramPalette's fields readonly and having buildGraphStyle accept a DiagramPalette (with buildGraphStyle(DIAGRAM_PALETTES[theme]) at the call site) buys the same test with a throwaway palette and no shared mutation.
| export function nodeChips( | ||
| hasSource: boolean, | ||
| leafCount: number, | ||
| theme: DiagramTheme, | ||
| counter: CounterGlyph = 'count' | ||
| ): Array<string> { | ||
| const count = formatLeafCount(leafCount); | ||
| return [ | ||
| hasSource ? cached(`code:${theme}`, () => codeChip(theme)) : CHIP_NONE, | ||
| leafCount === 0 | ||
| ? CHIP_NONE | ||
| : counter === 'count' | ||
| ? cached(`count:${theme}:${count}`, () => counterChip(count, 'count', theme)) | ||
| : cached(`${counter}:${theme}:${count.length}`, | ||
| () => counterChip(count, counter, theme)), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Two smaller things here:
- The return type is
Array<string>, but every consumer — the stylesheet's nine per-slot lists,chipButtons.ts, and the tests — depends on it being exactly two entries.[string, string]makes that contract compile-checked and drops the!assertions scattered through both test files. cacheis a module-levelMapthat is never cleared. Bounded by construction (≈1000 count labels × 2 themes × 3 glyphs), so not a leak, but it is process-global state in a library documented as having none — a field on the renderer, or an explicitly-documented memo, would be easier to reason about.
|
Reviewed against Ran, from
Nothing timing-based, ordering-dependent or external in the new suites, so no flakiness risk; the one shared-state mutation is restored in a |
39d44fc to
7ca1adc
Compare
The look of the diagram was a stylesheet literal inside
`CytographRendering`, with every color, size and radius spelled out at its
only point of use and no second palette possible. Two new modules replace
it, wired up in a later commit:
diagramTheme.ts geometry constants, the light and dark palettes, and
`buildGraphStyle(theme)`, which builds a cytoscape
stylesheet from one of them
chips.ts the corner chips drawn on a node: an SVG for the "this
node has SQL behind it" mark and one for the count of
operators a region hides, plus the metrics both the
stylesheet and the hit testing size them by
The two import each other: a chip is drawn from the palette, and the
stylesheet needs the background-image slots the chips are placed in.
Both suites drive a headless cytoscape instance, which resolves styles
without a renderer. That is what pins the mechanisms that fail silently:
the per-node chip image list, the taxi edge routing, and the draw order
that keeps an edge from crossing a region's chips.
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
7ca1adc to
90d19db
Compare
Part 4 of 15 of #6895, split one commit per PR. Based on
redesign-profiler-diagram-3; merge in order.The stack (this is 4 of 15)
The look of the diagram was a stylesheet literal inside
CytographRendering, with every color, size and radius spelled out at itsonly point of use and no second palette possible. Two new modules replace
it, wired up in a later commit:
diagramTheme.ts geometry constants, the light and dark palettes, and
buildGraphStyle(theme), which builds a cytoscapestylesheet from one of them
chips.ts the corner chips drawn on a node: an SVG for the "this
node has SQL behind it" mark and one for the count of
operators a region hides, plus the metrics both the
stylesheet and the hit testing size them by
The two import each other: a chip is drawn from the palette, and the
stylesheet needs the background-image slots the chips are placed in.
Both suites drive a headless cytoscape instance, which resolves styles
without a renderer. That is what pins the mechanisms that fail silently:
the per-node chip image list, the taxi edge routing, and the draw order
that keeps an edge from crossing a region's chips.
Describe Manual Test Plan
Nothing to look at: the two modules have no caller until part 6.
Verified at this commit, not just at the tip of the stack: checked out detached with
js-packages/profiler-lib/distdeleted and rebuilt from this commit's source, thenprofiler-libbun run checkandbun run test, andprofiler-layoutbun run checkandbun run test(all three vitest projects, browser suites included). All four green.Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes
None. The change is confined to
js-packages/.