feat: Add chart-owned runtime data limits and issue reporting - #656

packages/mosaic/__tests__/chart-runtime.test.ts (1)

21-35: ⚡ Quick win

Add a regression test for maxRows: 0.

Given policy semantics, zero should be a valid hard limit. Adding this test will lock in boundary behavior.

Suggested test case

   it('skips disabled policies and supports custom size estimators', () => {
@@
   });
+
+  it('enforces zero-row limits when maxRows is 0', () => {
+    expect(() => assertChartDataPolicy({maxRows: 0}, [{x: 1}])).toThrow(
+      DataPointLimitError,
+    );
+    expect(() => assertChartDataPolicy({maxRows: 0}, [])).not.toThrow();
+  });

🤖 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 `@packages/mosaic/__tests__/chart-runtime.test.ts` around lines 21 - 35, Add a
regression test that verifies maxRows: 0 is treated as a valid hard limit: using
assertChartDataPolicy with {maxRows: 0} and an empty data array should not
throw, while using {maxRows: 0} with any non-empty array (e.g., [{x:1}]) should
throw DataPointLimitError; place this assertion alongside the existing tests
that call assertChartDataPolicy and reference the same DataPointLimitError to
lock in the boundary behavior.

packages/mosaic/src/useVgPlotChartRender.ts (1)

154-173: ⚡ Quick win

Subtle: catch-path delegates to mark.queryError, which by then is the wrapped version.

In the queryResult catch branch you call mark.queryError(normalizedError) (line 160). Because the same forEach iteration installs the queryError wrapper a few lines later (175–197), the runtime call dispatches to the wrapper — so the issue is still reported via that path. This is correct but fragile: if the two if blocks are ever reordered or the wrapping is split, the runtimeIssueContext branch at lines 162–170 will be silently skipped whenever mark.queryError exists. A short comment explaining the dependency, or moving the explicit reportIssue call out of the else so it always fires on failure, would harden this.

🤖 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 `@packages/mosaic/src/useVgPlotChartRender.ts` around lines 154 - 173, In the
queryResult catch branch, ensure the runtime issue is always reported regardless
of whether mark.queryError is present: move or duplicate the
runtimeIssueReporter?.reportIssue(createChartRuntimeIssueFromError(normalizedError,
runtimeIssueContext, dataPolicy)) call so it executes before delegating to
mark.queryError(normalizedError) (or call it unconditionally then call
mark.queryError if present), and/or add a short comment clarifying the ordering
dependency; update the catch block around mark.queryError, runtimeIssueContext,
runtimeIssueReporter, and createChartRuntimeIssueFromError to guarantee
reporting always happens prior to or independent of the wrapped mark.queryError
invocation.

packages/mosaic/src/useMosaicClient.ts (1)

110-121: ⚡ Quick win

onQueryError forwards raw unknown; consider normalizing for consistency.

useVgPlotChartRender normalizes thrown values to Error before reporting/forwarding, but here the raw error: unknown is passed both to createChartRuntimeIssueFromError and to queryErrorRef.current. If a non-Error value (string, DOMException, etc.) bubbles up from Mosaic, downstream consumers expecting Error (e.g. .message) may break. Normalize once and reuse:

♻️ Suggested change

       onQueryError: (error) => {
+        const normalizedError =
+          error instanceof Error ? error : new Error(String(error));
         if (runtimeIssueContextRef.current) {
           runtimeIssueReporterRef.current?.reportIssue(
             createChartRuntimeIssueFromError(
-              error,
+              normalizedError,
               runtimeIssueContextRef.current,
               dataPolicyRef.current,
             ),
           );
         }
-        queryErrorRef.current?.(error);
+        queryErrorRef.current?.(normalizedError);
       },

🤖 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 `@packages/mosaic/src/useMosaicClient.ts` around lines 110 - 121, Normalize the
incoming unknown `error` in `onQueryError` to an actual `Error` instance before
using it: create a normalized Error (e.g., via a small helper or `error
instanceof Error ? error : new Error(String(error))`) and pass that same
normalized Error into `createChartRuntimeIssueFromError(...)` and to
`queryErrorRef.current(...)` so downstream consumers always receive an Error
object; update the `onQueryError` handler in `useMosaicClient` to perform this
single normalization and reuse the normalized value.

Fix all unresolved CodeRabbit comments on this PR:

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