ui/e2e/load-error-retry.spec.ts (1)

1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the documented describe/it convention for all Playwright cases.

• ui/e2e/load-error-retry.spec.ts#L1-L4: declare the retry case with it.
• ui/e2e/settings-url.spec.ts#L1-L4: declare the settings case with it.
• ui/e2e/stats-csv.spec.ts#L1-L4: declare the CSV case with it.

As per coding guidelines, ui/e2e/**/*.spec.ts cases use it within describe.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/e2e/load-error-retry.spec.ts` around lines 1 - 4, Use the documented
describe/it convention by changing the test declaration to it in
ui/e2e/load-error-retry.spec.ts lines 1-4, ui/e2e/settings-url.spec.ts lines
1-4, and ui/e2e/stats-csv.spec.ts lines 1-4; leave the surrounding describe
blocks and test behavior unchanged.

Source: Coding guidelines

ui/e2e/deep-link.spec.ts (1)

3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use it for Playwright cases consistently.

• ui/e2e/deep-link.spec.ts#L3-L4: alias test as it and use it(...) for the case.
• ui/e2e/embed-load.spec.ts#L3-L4: alias test as it and use it(...) for the case.

As per coding guidelines, ui/e2e/**/*.spec.ts must group behavior with describe, using it for cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/e2e/deep-link.spec.ts` around lines 3 - 4, Use the aliased `it` identifier
consistently for Playwright cases: in ui/e2e/deep-link.spec.ts lines 3-4, alias
the imported `test` as `it` and replace the case declaration with `it(...)`;
apply the same alias and case update in ui/e2e/embed-load.spec.ts lines 3-4.

Source: Coding guidelines

ui/src/components/ChartCard.integration.test.ts (3)

173-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated ref resets in beforeEach.

chartTypeRef.value = 'bar' and threeDRef.value = false are each assigned twice.

♻️ Cleanup

     holder.chartType = 'bar'
     chartTypeRef.value = 'bar'
-    chartTypeRef.value = 'bar'
     holder.threeD = false
     threeDRef.value = false
-    threeDRef.value = false

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/ChartCard.integration.test.ts` around lines 173 - 177,
Remove the duplicate assignments in the beforeEach setup, keeping one
chartTypeRef.value reset to 'bar' and one threeDRef.value reset to false while
preserving the existing holder.threeD initialization.

---

333-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test depends on state captured by earlier mounts.

holder.loadingComp/errorComp are only populated when defineAsyncComponent runs, so this case silently relies on module import/earlier tests rather than its own arrangement. Also, the if (typeof ...) guards mean the assertions can be skipped entirely without failing. Consider mounting within the test and asserting unconditionally.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/ChartCard.integration.test.ts` around lines 333 - 342,
Update the “invokes async loading and error placeholders” test to mount the
component or otherwise trigger its async setup within the test before reading
holder.loadingComp and holder.errorComp. Remove the typeof guards and assert
both placeholders unconditionally, including invoking them when appropriate so
the test cannot pass without exercising its own arrangement.

---

285-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

These two tests only assert the stub exists, so they can't fail for the right reason.

pass-through option updates and freezes buffer while loading are named around option propagation/freezing, but neither inspects the option actually bound to the chart. Assert the stub's option prop (e.g. w.findComponent({ name: /ChartStub/ })...props('option')) so the buffered-vs-live behavior is verified.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/ChartCard.integration.test.ts` around lines 285 - 298, The
tests “pass-through option updates when not loading” and “freezes buffer while
loading true” only verify rendering, not option propagation. Update both tests
to inspect the chart stub’s bound option prop via the ChartStub component:
assert the new option is exposed when not loading, and assert the previously
buffered option remains exposed while loading.

ui/src/components/charts.integration.test.ts (1)

80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

expect(name).toBeTruthy() is a no-op.

name is the literal from it.each; the assertion exists only to consume the parameter. Drop it (or use name in the assertion message instead).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/charts.integration.test.ts` around lines 80 - 82, Remove
the redundant expect(name).toBeTruthy() assertion from the it.each test, since
name is always a literal parameter; leave the meaningful useMock assertion
unchanged.

ui/src/composables/charts/shared/chartConfig.test.ts (3)

329-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent import style within the same file.

Some new suites resolve helpers via await import('./chartConfig') while adjacent ones (e.g. createValueAxisConfig, createTooltipConfig, renderDonutSvg) use the existing top-level static import. Unless the lazy import is needed for mock ordering, prefer the static imports already at the top of the file.

Also applies to: 344-349

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/chartConfig.test.ts` around lines 329 - 341,
Update the createHorizontalDataZoomConfig tests to use the file’s existing
top-level static imports instead of await import('./chartConfig'); apply the
same change to the related tests at lines 344-349, preserving their current
assertions and behavior.

---

472-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

typeof html === 'string' asserts nothing.

tooltipSpreadRows always returns a string, and the non-finite path is properly covered by the dedicated suite at Lines 548-566. Assert the concrete output for [0, 0, 0] or drop this case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/chartConfig.test.ts` around lines 472 - 480,
Update the non-finite stats test for tooltipSpreadRows to assert the concrete
expected output for [0, 0, 0], matching the behavior covered by the dedicated
non-finite suite, or remove this redundant case; do not retain the vacuous
typeof html assertion.

---

429-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Comments reference an absolute source line and describe unreachable branches.

"dead branch at 746" will rot on the next edit to chartConfig.ts, and the renderDonutSvg total guard title claims a non-positive-total branch while the assertion really covers the <2 positives guard. Rename the case (e.g. "returns empty when fewer than two positive slices") and refer to the symbol rather than a line number.

Also applies to: 496-500

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/chartConfig.test.ts` around lines 429 - 439,
Update the affected renderDonutSvg tests to remove absolute source-line
references and rename the case to describe the behavior actually asserted:
returning an empty string when fewer than two positive slices remain. Reference
the renderDonutSvg symbol and its positive-slice guard rather than calling the
branch unreachable or naming a non-positive-total condition.

ui/src/components/StatsPanel.integration.test.ts (2)

248-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Stubbing the whole URL global drops new URL(...) for the duration of these tests.

Only the two static methods are needed; replacing the constructor makes any incidental new URL() in the component or its dependencies throw. Prefer spying on the real object.

♻️ Narrower stub

-    vi.stubGlobal('URL', {
-      createObjectURL,
-      revokeObjectURL,
-    })
+    vi.spyOn(URL, 'createObjectURL').mockImplementation(createObjectURL)
+    vi.spyOn(URL, 'revokeObjectURL').mockImplementation(revokeObjectURL)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/StatsPanel.integration.test.ts` around lines 248 - 251,
Update the URL setup in the StatsPanel integration tests to preserve the real
URL constructor while mocking only createObjectURL and revokeObjectURL. Replace
the whole-global vi.stubGlobal call with spies on the existing URL static
methods, and retain the current mock implementations and cleanup behavior.

---

750-831: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Coverage-driven cases with no (or conditional) assertions.

setupState branch hammer patches Array.prototype.sort globally and mutates $.setupState directly without asserting any outcome; the axis-selector cases guard assertions behind if (selectors[1]) and toBeGreaterThanOrEqual(before), which pass regardless of behavior. These lift coverage numbers without protecting behavior and will break on any internal rename. Consider driving these branches through props/DOM and asserting concrete rendered output.

Also applies to: 372-376, 471-484

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/StatsPanel.integration.test.ts` around lines 750 - 831, The
tests around setupState branch coverage, including the `setupState branch
hammer` case and the referenced axis-selector cases, need behavioral assertions
instead of direct internal-state access. Drive sorting, correlation-axis
selection, and related branches through mounted component props and DOM
interactions, then assert concrete rendered output or user-visible state; remove
the global `Array.prototype.sort` patch and avoid assertions that are
unconditional or pass regardless of behavior.

ui/src/composables/charts/shared/visualMap.test.ts (1)

34-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

One it covers three unrelated assertions.

The title only describes the <= 0 fallback, yet the body also checks the positive max and resolve2DScatterVisualMap's default dimension. Split into separate it cases so a failure names the broken contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/visualMap.test.ts` around lines 34 - 44,
Split the combined test in the maxFromScatterValues describe block into separate
it cases: keep the non-positive fallback assertions together, move the positive
maximum assertion into its own clearly titled case, and move the
resolve2DScatterVisualMap default-dimension assertion into another case so
failures identify the affected contract.

ui/src/components/simple.integration.test.ts (2)

157-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the shared history fixture into ui/src/test-utils.

This HistoryEntry[] fixture is reused by HistoryPopover, the meta badges, and DatasetHeader cases here and is the kind of data other suites will want. As per coding guidelines, "share fixtures from ui/src/test-utils".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/simple.integration.test.ts` around lines 157 - 168, Move
the shared HistoryEntry[] fixture currently declared as history into
ui/src/test-utils, then update the HistoryPopover, meta badge, and DatasetHeader
test cases to import and reuse that fixture instead of defining it locally.

Source: Coding guidelines

---

369-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer asserting on w.html() directly.

Concatenating classes().join(' ') with html() obscures which surface carries min-h-[32rem]; expect(w.html()).toContain(...) (or a class assertion on the root) is clearer.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/simple.integration.test.ts` around lines 369 - 372, Update
the LoadError test using w to assert directly against w.html() for
min-h-[32rem], removing the classes().join(' ') concatenation so the asserted
markup surface is clear.

ui/src/components/settings/controls.integration.test.ts (2)

245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing await before asserting the ignored index.

Every other emission in this file is awaited; here the assertion runs without flushing, so it would pass even if an emission were queued. Await the emit for consistency.

♻️ Proposed change

     const vm = w.findComponent({ name: 'Selector' })
-    vm.vm.$emit('select', 99)
+    await vm.vm.$emit('select', 99)
     expect(vals).toEqual(['y/x'])

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/settings/controls.integration.test.ts` around lines 245 -
247, Await the Selector component’s select emission in the test before asserting
vals, updating the vm.vm.$emit call for the ignored index while preserving the
existing expected result.

---

93-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

async factory with no await.

The SettingsToggle.vue mock factory is async but never awaits (unlike the importOriginal mocks). Drop async to match the other plain stubs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/settings/controls.integration.test.ts` around lines 93 -
112, Remove the unnecessary async modifier from the SettingsToggle.vue vi.mock
factory. Keep the existing defineComponent stub, props, emits, and setup
behavior unchanged.

ui/src/components/DescriptiveColumnSelect.integration.test.ts (2)

175-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale comment contradicts the ghost mock.

The comment says the unknown-key path is "hard" to hit, but lines 91-97 mock isDescriptiveColumnKey precisely to make 'ghost' pass the guard while lookup fails — which line 178 exercises. Update the comment so the intent of the mock is clear.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/DescriptiveColumnSelect.integration.test.ts` around lines
175 - 178, Update the comment above the filter assertions to explain that the
mocked isDescriptiveColumnKey allows the 'ghost' key past the guard while lookup
fails, and that the assertion exercises this unknown-key path; remove the stale
claim that the path is hard to hit.

---

29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused expose API in the Combobox stub.

setModel/setSearch/runFilter are never used; the tests drive the component via cb.vm.$emit(...) and cb.props('filterFunction'). Dropping the expose block keeps the stub minimal.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/DescriptiveColumnSelect.integration.test.ts` around lines
29 - 40, Remove the unused expose API from the Combobox stub’s setup function,
including setModel, setSearch, and runFilter; retain the existing props, emit,
and slots handling used by the tests.

ui/src/components/Selector.integration.test.ts (2)

38-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused stub buttons.

emit-value, emit-open-false, and emit-search are never clicked; every test emits through cb.vm.$emit(...). Consider trimming the stub.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/Selector.integration.test.ts` around lines 38 - 65, Remove
the unused stub buttons with test IDs emit-value, emit-open-false, and
emit-search from the rendered selector stub; the tests already trigger these
events through cb.vm.$emit(...), so preserve the existing test behavior while
trimming only these unreachable controls.

---

184-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Several new cases assert nothing falsifiable. These sites appear written to reach uncovered branches for the new 100% thresholds, but their assertions pass regardless of behavior, so the coverage they add is not protective.

• ui/src/components/Selector.integration.test.ts#L184-L187: replace the monotonic count >= before check with the exact expected select emission count after close.
• ui/src/components/SettingsPanel.integration.test.ts#L279-L285: replace expect(w.exists()).toBe(true) with an assertion on the swap/3D control state produced by the empty arrangement map.
• ui/src/components/simple.integration.test.ts#L443-L453: assert pop.exists() and emit update:open unconditionally instead of guarding with if.
• ui/src/components/Selector.integration.test.ts#L335-L353: assert the rendered trigger label and absence of select/selectValue emissions for the missing-activeId/activeValue branch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/Selector.integration.test.ts` around lines 184 - 187,
Strengthen the listed tests with behavior-specific assertions: in
ui/src/components/Selector.integration.test.ts:184-187, assert the exact select
emission count after closing; in
ui/src/components/SettingsPanel.integration.test.ts:279-285, assert the swap/3D
control state for an empty arrangement map; in
ui/src/components/simple.integration.test.ts:443-453, assert pop exists and emit
update:open unconditionally; and in
ui/src/components/Selector.integration.test.ts:335-353, assert the trigger label
plus no select or selectValue emissions for missing activeId/activeValue.

ui/src/composables/charts/shared/3d.test.ts (1)

743-760: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test name doesn't match what is set up or asserted.

aggPoints is non-empty here, so legend totals are present, and no assertion checks that any Σ tag is omitted — only 'Z1' and '<svg'. Either drop the "omits Σ tags" claim (the empty-aggPoints case at lines 666-677 already covers it) or assert the color-fallback effect explicitly, e.g. that the rendered dot/donut uses the empty-string color.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/3d.test.ts` around lines 743 - 760, The test
name for the case around create3DTooltipFormatter does not match its setup or
assertions. Rename it to describe the non-empty aggPoints color-fallback
scenario, or change the fixture to empty aggPoints and assert Σ omission;
preferably keep this case focused on the mocked getNextColorFor fallback by
asserting the rendered dot/donut uses an empty-string color.

ui/src/components/SettingsPanel.integration.test.ts (1)

312-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reaching into $.setupState couples the test to Vue internals.

valueFor/onUpdate are exercised here through private component internals. Prefer driving them via the stubbed control components (as the other cases do) so a Vue internal rename doesn't break the suite.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/SettingsPanel.integration.test.ts` around lines 312 - 329,
Update the test case around valueFor and onUpdate to exercise these behaviors
through the stubbed control components instead of accessing w.vm.$.setupState.
Preserve the empty barConfigRef scenario, verify valueFor’s undefined behavior
through the relevant control, and trigger the onUpdate path using the control
interaction used by the neighboring tests.

ui/src/composables/charts/useBarChartOptions.test.ts (2)

258-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse the shared makeValueChartData fixture. @/test-utils already exports a makeValueChartData with the same valueTuples (see ui/src/composables/charts/shared/valueMode.test.ts line 62), so this local copy duplicates and can drift from it.

As per coding guidelines, "share fixtures from ui/src/test-utils".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useBarChartOptions.test.ts` around lines 258 - 270,
Remove the local makeValueChartData fixture and import/reuse the shared
makeValueChartData export from "`@/test-utils`" in the affected tests, preserving
the existing test data and behavior.

Source: Coding guidelines

---

534-549: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Weak sort assertion. With both slots empty the comparator is a no-op, so ['A', 'B'] also passes when sorting is skipped entirely. Mix one populated and one empty slot (e.g. values: [undefined as unknown as number, 5]) so ordering actually proves the ?? 0 fallback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useBarChartOptions.test.ts` around lines 534 - 549,
Strengthen the test around useBarChartOptions by giving the two y-axis slots
different effective values: one undefined and one populated value such as 5.
Keep descending sorting enabled and assert the resulting series order reflects
the undefined slot being compared as 0, so the test verifies sorting and the
nullish fallback rather than only the default order.

ui/src/composables/charts/shared/mixedMode.test.ts (1)

64-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assertion doesn't verify what the title claims. Both branches only check that axisPointer exists, so a regression swapping line/shadow pointers would pass. Assert the pointer type instead.

♻️ Suggested tightening

-    expect(line && typeof line === 'object' && 'axisPointer' in line).toBe(true)
-    expect(bar && typeof bar === 'object' && 'axisPointer' in bar).toBe(true)
+    const pointerType = (t: typeof line) =>
+      (t as { axisPointer?: { type?: string } }).axisPointer?.type
+    expect(pointerType(line)).toBe('line')
+    expect(pointerType(bar)).toBe('shadow')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/mixedMode.test.ts` around lines 64 - 69,
Strengthen the assertions in the “uses line pointer for scatter/line and shadow
for bar” test by checking the axisPointer.type value returned by
createMixedModeTooltip: expect the line case to use “line” and the bar case to
use “shadow”, rather than only verifying axisPointer exists.

ui/src/composables/charts/useHeatmapChartOptions.test.ts (3)

404-412: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Assertion can't fail. Both the 3D and 2D heatmap paths emit series[0].type === 'heatmap', so mocking is3D and asserting only the type verifies nothing about the "undefined points" branch. Assert the resulting series[0].data shape instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useHeatmapChartOptions.test.ts` around lines 404 -
412, Update the test around useHeatmapChartOptions so it verifies the
undefined-points fallback through series[0].data rather than the shared series
type. Keep the is3D mock and chartData setup, and assert the resulting data
shape that distinguishes the undefined-points branch.

---

361-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead delete immediately followed by reassignment. Line 361 has no effect since points is re-set on the next line with the same value already given to grouped3DHeatmap. Drop both lines.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useHeatmapChartOptions.test.ts` around lines 361 -
362, Remove the redundant points mutation in the affected heatmap test: delete
both the delete operation and the immediately following reassignment on
chartData.points. Keep the existing grouped3DHeatmap setup unchanged, since it
already provides the required points value.

---

390-412: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Module spies on @/lib/utils are restored inline instead of in a hook. In both files spy.mockRestore() sits at the end of the test body, so a failing assertion leaves getNextColorFor/is3D mocked for the rest of the file and turns one failure into several.

• ui/src/composables/charts/useHeatmapChartOptions.test.ts#L390-L412: add afterEach(() => vi.restoreAllMocks()) to the mocked color fallbacks describe and drop the two inline spy.mockRestore() calls.
• ui/src/composables/charts/useScatter3DChartOptions.test.ts#L285-L307: add the same afterEach to the more optional branches describe and drop the inline restore.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useHeatmapChartOptions.test.ts` around lines 390 -
412, Replace inline spy restoration with describe-scoped cleanup: in
ui/src/composables/charts/useHeatmapChartOptions.test.ts lines 390-412, add
afterEach(() => vi.restoreAllMocks()) to the “mocked color fallbacks” describe
and remove both spy.mockRestore() calls; apply the same change in
ui/src/composables/charts/useScatter3DChartOptions.test.ts lines 285-307 within
the “more optional branches” describe, removing its inline restore.

ui/src/composables/charts/shared/valueMode.test.ts (1)

240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test name overpromises. Nothing here asserts the cross axis-pointer or the visualMap dimension; only tooltip truthiness and itemStyle. Either assert tooltip.axisPointer.type / visualMap.dimension === 1, or rename to match what's checked.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/shared/valueMode.test.ts` around lines 240 - 248,
Update the test named “uses cross pointer for scatter/line and color dim 1
without third slot” so its assertions cover the promised cross axis pointer and
visualMap dimension, including tooltip.axisPointer.type and visualMap.dimension
where applicable; otherwise rename the test to describe only the tooltip and
itemStyle behavior it currently verifies.

ui/src/composables/charts/useBar3DChartOptions.test.ts (1)

252-260: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Dead is3D spy + assertions that don't verify the claimed nullish-points/defaults behavior. Both tests spy on is3D from @/lib/utils, but the reviewed composable code never calls is3D, so the mock has no bearing on the outcome. Both also assert only generic series presence/length, which is already guaranteed by the fixed grouped render fixture regardless of whether points is undefined — so the actual "nullish points treated as empty" / "without points" behavior (i.e., the points ?? [] fallback feeding aggPoints/tooltip) is never exercised.

• ui/src/composables/charts/useBar3DChartOptions.test.ts#L252-L260: drop the unused is3D spy and add an assertion that actually depends on points being nullish (e.g. invoke the tooltip formatter and assert it doesn't throw / returns expected text) rather than only checking series.length.
• ui/src/composables/charts/useLine3DChartOptions.test.ts#L229-L239: same fix — remove the irrelevant is3D spy and strengthen the assertion to cover the rotate/scale/points defaults actually being exercised.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/charts/useBar3DChartOptions.test.ts` around lines 252 -
260, In ui/src/composables/charts/useBar3DChartOptions.test.ts:252-260, remove
the unused is3D spy and assert behavior that depends on nullish points, such as
invoking the tooltip formatter and verifying it returns expected text without
throwing. In ui/src/composables/charts/useLine3DChartOptions.test.ts:229-239,
likewise remove the is3D spy and strengthen the test to verify the rotate,
scale, and points defaults are exercised rather than only checking generic
series output.

ui/src/composables/useChartPipeline.test.ts (1)

479-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assertion doesn't cover the documented drain branch. The comments describe pumpQueue's draining early-return, but the only assertion is that an unknown signature never becomes a slot. Consider asserting the posted compute calls (e.g. exactly one in-flight compute while draining) so the test fails for the right reason if the guard is removed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/useChartPipeline.test.ts` around lines 479 - 490, The test
case “ignores pump while already draining and drops unknown chart slots” does
not verify the documented draining guard. Add an assertion on worker.postMessage
calls after the recompute is queued, confirming exactly one compute request
remains in flight while draining, while retaining the existing unknown-slot
assertion.

ui/src/workers/transform.worker.test.ts (1)

419-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Second half of this test asserts nothing and never runs. With data: [] there are no signatures, so sig is always undefined and the if (sig) block is dead — the unknown-group fallback in ui/src/workers/transform.worker.ts (Lines 168-172) is not covered here. Exercise it with non-empty data and a bogus groupName, asserting a chart is still produced.

♻️ Suggested rework

-    // compute with empty grouped map path: re-init with empty data
-    send(buildInit({ data: [] }))
-    const sig = ready()?.signatures[0]?.signature
-    postSpy.mockClear()
-    if (sig) {
-      send(buildCompute({ signature: sig, groupName: 'ghost' }))
-      // no signatures from empty data → may be undefined
-    }
+    // unknown groupName falls back to the first group
+    send(buildInit())
+    const sig = ready()!.signatures[0]!.signature
+    postSpy.mockClear()
+    send(buildCompute({ signature: sig, groupName: 'ghost' }))
+    expect(charts()).toHaveLength(1)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/workers/transform.worker.test.ts` around lines 419 - 438, Update the
test case around buildInit and buildCompute so the unknown-group fallback uses
non-empty data that produces a signature. Remove the empty-data guard, send
buildCompute with a bogus groupName such as “ghost,” and assert that the worker
produces a chart/result, thereby covering the fallback in the transform worker.

ui/src/composables/useStatsWorker.test.ts (1)

149-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the id lookup robust and drop the unused async. Line 150's test never awaits, and Line 156 assumes postMessage.mock.calls[0] is this test's request — any earlier post on the shared singleton worker (warmup or a prior test in this describe) would yield the wrong id and hang the assertion. Prefer the most recent call.

♻️ Suggested tightening

-  it('ignores replies for unknown ids without throwing', async () => {
+  it('ignores replies for unknown ids without throwing', () => {
     expect(() => worker.__emit({ type: 'result', id: 999_999, seriesProfiles: [] })).not.toThrow()
   })
 
   it('defaults missing seriesProfiles to an empty array', async () => {
     const chart = makeChart('no-profiles')
+    worker.postMessage.mockClear()
     const p = computeDescriptive(chart)
-    const id = worker.postMessage.mock.calls[0]![0].id as number
+    const id = worker.postMessage.mock.calls.at(-1)![0].id as number
     worker.__emit({ type: 'result', id })
     await expect(p).resolves.toEqual([])
   })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/useStatsWorker.test.ts` around lines 149 - 158, Update the
tests around the unknown-id and missing-seriesProfiles cases: remove the
unnecessary async modifier from the test that only checks __emit does not throw,
and in the missing-seriesProfiles test obtain the request id from the most
recent worker.postMessage mock call rather than index 0 so it matches
computeDescriptive’s request.

ui/src/workers/transform.worker.ts (1)

168-172: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Non-null assertion on the first-group fallback is load-bearing. The invariant (a signature entry exists ⇒ at least one group) holds today, but if it ever breaks, rows is undefined and buildChartForSignature throws inside the worker with no reply posted — the pipeline slot stays pending forever. A ?? [] fallback would degrade to an empty chart instead.

🛡️ Optional hardening

     let rows = s.grouped.get(msg.groupName)
     if (rows === undefined) {
       // After init/arrangement the map always has at least one group.
-      rows = s.grouped.values().next().value!
+      rows = s.grouped.values().next().value ?? []
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/workers/transform.worker.ts` around lines 168 - 172, Preserve the
non-null assertion on the first-group fallback in the grouped-row lookup near
buildChartForSignature. Do not replace it with a [] fallback; the existing
invariant requires a group whenever a signature entry exists, and violations
should remain explicit rather than producing an empty chart.

ui/src/views/Dashboard.integration.test.ts (1)

34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

holder.charts / holder.groupNames / holder.datasetInitializing are inert duplicates of the refs.

Lines 65-67 snapshot those holder values once, and the useChartPipeline mock reads only the refs — so Line 260 (and any future holder.charts = … / holder.groupNames = … write) is a silent no-op, which is an easy trap for the next test author. Drop the holder fields and keep the refs as the single source.

♻️ Proposed cleanup

-  charts: [] as { key: string; data?: ChartData; pending: boolean }[],
-  groupNames: ['g0', 'g1'] as string[],
-  datasetInitializing: false,
   dashInit: vi.fn(),

-const groupNamesRef = ref(holder.groupNames)
-const chartsRef = ref(holder.charts)
-const datasetInitializingRef = ref(holder.datasetInitializing)
+const groupNamesRef = ref<string[]>(['g0', 'g1'])
+const chartsRef = ref<{ key: string; data?: ChartData; pending: boolean }[]>([])
+const datasetInitializingRef = ref(false)

-    holder.datasetInitializing = false
     datasetInitializingRef.value = false

Also applies to: 260-261

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/views/Dashboard.integration.test.ts` around lines 34 - 36, Remove the
holder.charts, holder.groupNames, and holder.datasetInitializing fields and any
reads or writes of them, including the assignments around the affected test
lines. Use the existing refs directly as the single source for the mocked
useChartPipeline state and related assertions.

ui/src/composables/useDataPoint.test.ts (2)

508-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test title claims behavior it does not assert.

normalizes null settings/data on prepare and rejects catalog shells without id only asserts the normalization half; the id rejection is covered by the separate test at Lines 531-550. Trim the title to match.

♻️ Suggested rename

-  it('normalizes null settings/data on prepare and rejects catalog shells without id', async () => {
+  it('normalizes null settings/data on prepare', async () => {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/useDataPoint.test.ts` around lines 508 - 529, Rename the
test case beginning with “normalizes null settings/data on prepare and rejects
catalog shells without id” to describe only its asserted null settings/data
normalization behavior; leave the separate catalog-shell ID rejection test
unchanged.

---

291-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Consider splitting this multi-concern test.

One it covers arrangement, group selection, three chart-mode transitions, invalid-index selection, and the non-array guard. A failure anywhere gives one opaque red test. it.each over the dataset/mode table plus separate cases for arrangement, groups, and the guard would localize failures.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/useDataPoint.test.ts` around lines 291 - 383, Split the
combined test “covers arrangement, groups, chart modes, and selection edge
cases” into focused tests: separate arrangement behavior, group selection,
invalid dataset selection, and the non-array datasets guard, and use an it.each
table for the dataset/chart-mode transitions. Preserve all existing assertions
while making each failure identify its specific behavior.

ui/src/composables/useSettingsStore.test.ts (1)

84-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The it.each rows carry duplicated import/store boilerplate.

Each row's apply closure re-imports the module and constructs the store, so the table encodes procedure rather than data. Passing the setter key and argument would collapse both rows to data and keep the arrange step in the test body.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/composables/useSettingsStore.test.ts` around lines 84 - 123, Refactor
the parameterized test around the it.each table so each row supplies the setter
key and argument instead of an apply closure that imports useSettingsStore and
creates the store. Move the module import, store construction, and setter
invocation into the test body, dynamically calling the provided setter while
preserving the existing config setup and activeConfig assertion.

ui/src/lib/transform.ts (1)

297-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated swapAxisLabels(...)! non-null assertion. Both sites spread a nullable AxisLabels | undefined and silence the type with !; spreading undefined already yields {}, so the assertion is pure noise.

• ui/src/lib/transform.ts#L297-L297: drop the ! from { ...swapAxisLabels(identity, target, baseLabels)! }.
• ui/src/lib/builders/value.ts#L23-L23: drop the ! from the identical expression in ValueBuilder.build.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/transform.ts` at line 297, Remove the redundant non-null assertion
from the spread of swapAxisLabels in ui/src/lib/transform.ts lines 297-297 and
in ValueBuilder.build at ui/src/lib/builders/value.ts lines 23-23; leave the
surrounding label construction unchanged.

ui/src/lib/transform.test.ts (4)

841-845: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

fieldValueName is a tautology and adds no coverage.

It re-runs the same projectValueCoords({name: undefined}, 'n', 'n') call already asserted on Line 813 and returns '' by construction, so expect(fieldValueName()).toBe('') can't fail. Either assert the projection result directly or drop the helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/transform.test.ts` around lines 841 - 845, Remove the tautological
fieldValueName helper and its associated assertion, or replace it with a direct
assertion on projectValueCoords for the undefined name case. Ensure the test
independently verifies the actual projection result rather than returning a
value constructed to always be ''.

---

883-888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test doesn't exercise what its name claims.

skips incomplete 3D coords when a dimension maps to name only repeats the projectValueCoords(..., 'xy', 'xyz') null assertion from Line 812 and never calls buildValueModeChart. Either call the builder with an arrangement that yields incomplete coords, or remove this duplicate.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/transform.test.ts` around lines 883 - 888, The test named “skips
incomplete 3D coords when a dimension maps to name” does not exercise
buildValueModeChart and duplicates an existing projectValueCoords assertion.
Update this test to call buildValueModeChart with an arrangement producing
incomplete 3D coordinates and assert the intended skip behavior, or remove the
duplicate test if that scenario cannot be constructed.

---

945-951: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assertion contradicts the test name.

The case is titled "skips points whose x/y are outside the index maps", but the only assertion is that xValues contains MISSING — nothing is skipped, since both x values come from the same points array. Rename to describe the actual behavior, or construct a canonical order that genuinely excludes an axis value (as done at Lines 1216-1232).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/transform.test.ts` around lines 945 - 951, Fix the test around the
`skips points whose x/y are outside the index maps` case so its name and
assertion describe the same behavior. Prefer constructing canonical index maps
that exclude the stray axis value, following the pattern used around the
referenced canonical-order test; otherwise rename the test to reflect that
`MISSING` is included in `render.xValues`.

---

1131-1150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Strip the leftover exploratory comments.

Lines 1132-1133 and 1148-1149 read as in-progress reasoning ("actually Ghost is included in sets", "wait applyCanonicalOrder filters…"). Replace with a one-line statement of the invariant each case pins.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/transform.test.ts` around lines 1131 - 1150, Remove the
exploratory comments around the tests for Ghost preservation and canonical-order
filtering. Replace each comment block with a single concise statement describing
the invariant the test asserts, without mentioning the investigation or
uncertainty; update the comments near build3DRender and cellsFor respectively.

ui/src/lib/builders/builders.test.ts (2)

20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Same emptyChart factory copied into two test files. ui/src/test-utils/chartFixtures.ts already exports emptyChartData(overrides); both files re-declare an equivalent local factory.

• ui/src/lib/builders/builders.test.ts#L20-L28: replace the local emptyChart with emptyChartData imported from @/test-utils.
• ui/src/lib/utils.test.ts#L446-L454: replace the local emptyChart with the same shared import.

As per coding guidelines: "share fixtures from ui/src/test-utils".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/builders/builders.test.ts` around lines 20 - 28, Replace the
duplicated local emptyChart factories in
ui/src/lib/builders/builders.test.ts#L20-L28 and
ui/src/lib/utils.test.ts#L446-L454 with the shared emptyChartData import from
`@/test-utils`, updating usages as needed while preserving each test’s existing
override behavior.

Source: Coding guidelines

---

500-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Line 517 assertion can't fail.

After the canonical remap the tuple indices are either remapped or fall back to the original xi, so every possible outcome satisfies xi === 0 || xi === 1. Assert the exact expected tuples so the branch is actually pinned.

💡 Suggested tightening

-    expect(canon.mixedTuples?.some(([xi]) => xi === 0 || xi === 1)).toBe(true)
+    expect(canon.mixedTuples).toEqual([
+      [0, 1],
+      [1, 2],
+    ])

(adjust to the remap's real output)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/builders/builders.test.ts` around lines 500 - 518, Replace the
broad some assertion in the finalizeChart test with an exact deep-equality
assertion for canon.mixedTuples. Pin the remapped output produced by canonical x
values A and C, including the fallback index for the missing B category, while
retaining the existing xCategories assertion.

ui/src/lib/utils.test.ts (1)

468-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

applyTheme mutates shared theme state with no cleanup.

Any later test in this file that reads the palette now sees #111,#222``, making results order-dependent. Wrap in a narrowly scoped hook that restores the previous theme after the case.

As per coding guidelines: "Keep UI tests deterministic and beside the tested code".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/lib/utils.test.ts` around lines 468 - 472, Update the “reads active
palette helpers” test around applyTheme and add a narrowly scoped cleanup hook
that captures the existing theme before the test and restores it afterward.
Ensure later tests observe the original palette regardless of execution order,
while keeping the test beside the existing palette-helper coverage.

Source: Coding guidelines

ui/src/test-utils/chartFixtures.ts (1)

163-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use isRef here instead of checking 'value' in value.

A plain object override like visibleZ: { value: true } matches the duck-typed branch and gets treated as a ref, which breaks the config.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/test-utils/chartFixtures.ts` around lines 163 - 168, Update asRef to
use Vue’s isRef predicate instead of the duck-typed `'value' in value` check, so
plain object overrides are wrapped with ref while actual refs are returned
unchanged.

ui/src/components/ui/ui-primitives.integration.test.ts (1)

210-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a scoped afterEach for DOM-attached mounts.

Both Popover and Combobox family blocks mount with attachTo: document.body and rely on manual w.unmount() calls at the end of each it. If an assertion throws before reaching unmount(), the attached nodes remain in document.body for subsequent tests. A narrowly-scoped afterEach per describe block would guarantee cleanup regardless of assertion outcome.

As per coding guidelines, ui/src/**/*.integration.test.ts should use "hooks at the narrowest useful scope."

♻️ Example scoped cleanup

 describe('Popover', () => {
+  afterEach(() => {
+    document.body.innerHTML = ''
+  })
+
   it('mounts trigger and force-mounted content with default align', async () => {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/components/ui/ui-primitives.integration.test.ts` around lines 210 -
333, Add scoped afterEach cleanup to the Popover and Combobox family describe
blocks so DOM-attached mounts are unmounted even when assertions fail. Track
each mount or otherwise clean up all attached wrappers within the corresponding
describe scope, and remove the reliance on per-test manual w.unmount() calls
while preserving the existing test behavior.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes