Repository navigation
feat: error handling in charts and UI\UX imporovements - #702
Merged
Merged
Conversation
Add field existence and aggregation type validation to all create*ChartSpec functions: - Created shared validation module with three utilities: - validateFieldExists: checks field exists in data table - getValidatedColumn: validates and returns column metadata - validateAggregation: validates aggregation can be applied to field type (numeric only) - Updated all chart specs to use validation: - histogram: field existence validation - line-chart: x field, y fields, and aggregation validation - bubble-chart: x and y field validation - heatmap: x and y field validation - count-plot: field existence validation - Added comprehensive test suite for validation utilities Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors the Mosaic chart system's error handling, spec generation, and UI composition. Chart validation now throws typed errors (RequiredFieldsError, MissingColumnsError, InvalidColumnTypeError) that MosaicChartError renders with type-specific layouts. The CreateSpecOptions contract switches from tableName to dataTable, enabling all chart type specs to validate and resolve columns with stronger type checking. VgPlotChart suppresses error rendering when a runtime issue reporter is present, deferring to the runtime issue panel instead. New reusable components (BlockCaptionEditor, MosaicChartSettingsButton) and mark-level error aggregation in useVgPlotChartRender defer runtime issue reporting until all marks complete. ChangesChart system refactoring and dashboard composition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mosaic/src/useVgPlotChartRender.ts (1)
256-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't clear retained mark state before the restore pass.
markStatesandmarkErrorsare wiped here, thencheckAndReportAggregateStateruns against empty maps and returns immediately. If a cached chart is reattached without immediately re-querying, its previous runtime error disappears instead of being restored.🤖 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 256 - 285, The code currently clears cachedChart.markStates and cachedChart.markErrors before calling checkAndReportAggregateState, which causes previous runtime errors to be lost; change the sequence so checkAndReportAggregateState runs while cachedChart.markStates/markErrors are still intact (i.e., remove or move the clear() calls for markStates and markErrors until after checkAndReportAggregateState has executed), then continue with attachPlotElement/asPlotDomElement, resizeChartElement, wrapMarkHandlers, and only clear or reset retained state afterward if/when you intentionally reinitialize marks.
🧹 Nitpick comments (5)
packages/mosaic/src/charts/chart-types/bubble-chart/spec.ts (1)
71-79: ⚡ Quick winEliminate redundant type checks.
Each column's type is validated twice: once in the
ifcondition (line 71) and again when building the error array (lines 74-75). This duplication is unnecessary.♻️ Refactor to check once
- // Validate X and Y field are numeric - if (!isNumericType(xColumn.type) || !isNumericType(yColumn.type)) { - throw new InvalidColumnTypeError( - [ - ...(!isNumericType(xColumn.type) ? [xColumn.name] : []), - ...(!isNumericType(yColumn.type) ? [yColumn.name] : []), - ], - 'numeric', - ); - } + // Validate X and Y field are numeric + const invalidColumns = [ + ...(isNumericType(xColumn.type) ? [] : [xColumn.name]), + ...(isNumericType(yColumn.type) ? [] : [yColumn.name]), + ]; + if (invalidColumns.length > 0) { + throw new InvalidColumnTypeError(invalidColumns, 'numeric'); + }🤖 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/charts/chart-types/bubble-chart/spec.ts` around lines 71 - 79, The code currently calls isNumericType(xColumn.type) and isNumericType(yColumn.type) twice; refactor the check by computing booleans (e.g., xIsNumeric, yIsNumeric) or building a failingColumns array once, then use those values both for the if-condition and to populate the InvalidColumnTypeError; update the block surrounding isNumericType, InvalidColumnTypeError, xColumn and yColumn so each column type is validated a single time and the error array is constructed from that single result.packages/mosaic/src/charts/chart-types/line-chart/spec.ts (1)
171-176: 💤 Low valueRemove unnecessary type assertion.
The return value on line 175 already matches the
ValidatedLineChartSettingstype; theas ValidatedLineChartSettingscast is unnecessary.♻️ Remove the cast
return { xColumn, yColumns, xInterval, - } as ValidatedLineChartSettings; + }; }🤖 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/charts/chart-types/line-chart/spec.ts` around lines 171 - 176, The return statement currently casts the object to ValidatedLineChartSettings unnecessarily; remove the "as ValidatedLineChartSettings" type assertion so the function returns { xColumn, yColumns, xInterval } directly (the object already conforms to the ValidatedLineChartSettings type), ensuring no runtime change and cleaner code.packages/mosaic/src/charts/chart-types/heatmap/spec.ts (1)
71-79: ⚡ Quick winEliminate redundant type checks.
Each column's type is validated twice: once in the
ifcondition (line 71) and again when building the error array (lines 74-75). This is identical to the redundant pattern inbubble-chart/spec.ts.♻️ Refactor to check once
- // Validate X and Y field are numeric - if (!isNumericType(xColumn.type) || !isNumericType(yColumn.type)) { - throw new InvalidColumnTypeError( - [ - ...(!isNumericType(xColumn.type) ? [xColumn.name] : []), - ...(!isNumericType(yColumn.type) ? [yColumn.name] : []), - ], - 'numeric', - ); - } + // Validate X and Y field are numeric + const invalidColumns = [ + ...(isNumericType(xColumn.type) ? [] : [xColumn.name]), + ...(isNumericType(yColumn.type) ? [] : [yColumn.name]), + ]; + if (invalidColumns.length > 0) { + throw new InvalidColumnTypeError(invalidColumns, 'numeric'); + }🤖 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/charts/chart-types/heatmap/spec.ts` around lines 71 - 79, Compute the numeric-check results once and reuse them instead of calling isNumericType twice; e.g., evaluate const xNumeric = isNumericType(xColumn.type) and const yNumeric = isNumericType(yColumn.type), then use if (!xNumeric || !yNumeric) { throw new InvalidColumnTypeError([...(!xNumeric ? [xColumn.name] : []), ...(!yNumeric ? [yColumn.name] : [])], 'numeric'); } so you only call isNumericType for xColumn.type and yColumn.type a single time (affecting the block that currently references xColumn, yColumn, isNumericType, and InvalidColumnTypeError in heatmap spec).packages/mosaic/src/charts/useMosaicChartRenderContext.ts (1)
87-97: ⚡ Quick winRemove the temporary spec/settings logging before merge.
These
console.logcalls run during spec generation and dump chart settings/specs into the browser console on every recomputation. Please gate them behind a dev-only logger or drop them.🤖 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/charts/useMosaicChartRenderContext.ts` around lines 87 - 97, Remove the two temporary console.log calls in createMosaicSpecChartRenderContext that print settings and spec during chartTypeDefinition.createSpec; either delete these console.log statements or wrap them with a development-only guard (e.g., a check like process.env.NODE_ENV === 'development' or the project's devLogger) so they do not run in production, ensuring no sensitive chart settings/specs are dumped to the browser console on every recomputation.packages/mosaic/src/components/BlockCaptionEditor.tsx (1)
12-28: 💤 Low valueConsider adding JSDoc for the public component.
As per coding guidelines, public exports in
packages/should be documented. A brief JSDoc block describing the component's purpose and prop expectations would improve discoverability.📝 Example documentation
+/** + * Editable text input for block captions (chart/dashboard titles). + * Wraps EditableText with standardized styling for caption editing. + */ export const BlockCaptionEditor: FC<BlockCaptionEditorProps> = ({🤖 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/components/BlockCaptionEditor.tsx` around lines 12 - 28, Add a JSDoc block above the exported const BlockCaptionEditor that briefly describes the component's purpose (a small caption editor for blocks) and documents its props via BlockCaptionEditorProps: value, className, placeholder, isReadOnly, and onChange (include types/expected behavior for each prop and note that it renders EditableText). Keep it short (one-line summary + param tags) and place it immediately above the BlockCaptionEditor declaration.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/mosaic/__tests__/LineChartSettings.test.tsx`:
- Around line 25-74: The tests only assert the boolean expression and don't
verify that LineChartSettings.tsx actually runs the useEffect/useRef logic and
calls onChangeConfig('xInterval', undefined); update tests to be
integration-style: render LineChartSettings, simulate changing the X field from
a temporal to a non-temporal field, and assert that the component's
onChangeConfig prop was invoked with ('xInterval', undefined); target the
component and its effects (useEffect/useRef behavior) by interacting with the
rendered DOM rather than asserting the standalone boolean.
In `@packages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsx`:
- Around line 61-77: Multiple Mosaic chart settings components are
inconsistently wrapping tooltips: MosaicChartSettings.ViewSpecButton and
MosaicChartSettings.CloseButton include their own TooltipProvider while
MosaicChartSettingsButton uses Tooltip without a provider; standardize by either
moving a single TooltipProvider to a higher-level component (e.g., RoomShell)
and documenting that Mosaic components expect it, or wrap
MosaicChartSettingsButton with TooltipProvider to match ViewSpecButton and
CloseButton so all three (MosaicChartSettings.ViewSpecButton,
MosaicChartSettings.CloseButton, MosaicChartSettingsButton) use the same
provider behavior and consistent timing defaults.
In `@packages/mosaic/src/charts/MosaicChartSettingsButton.tsx`:
- Around line 11-15: The exported type MosaicChartSettingsButtonProps is missing
JSDoc; add a concise JSDoc block above the export documenting the prop interface
(describe the component usage and each prop: className, isSettingsOpen,
onToggleSettings) following the suggested comments style so consumers of
MosaicChartSettingsButton know intent and types; ensure the comment is generic
and composable (no app-specific examples) and placed immediately above the
MosaicChartSettingsButtonProps declaration.
In `@packages/mosaic/src/useVgPlotChartRender.ts`:
- Around line 141-157: Pre-populate tracking for every queryable mark so
checkAndReportAggregateState sees them: ensure before you wrap mark.queryPending
you add each QueryableMark from plot.marks into chart.markStates (e.g., set to
an initial state like 'idle' or similar) and ensure chart.markErrors exists;
continue to wrap mark.queryPending as done but do not rely on queryPending being
present to register the mark. This involves touching the loop over plot.marks
and calling chart.markStates.set(mark, 'idle') (or equivalent initial state) for
every mark that can be queried so checkAndReportAggregateState will
await/monitor all marks, then keep the existing override of mark.queryPending to
update the state to 'pending' when it runs.
- Around line 149-215: wrapMarkHandlers is not idempotent and re-wrapping the
same mark doubles side effects; make it detect already-wrapped marks and skip
wrapping. Add a unique marker (Symbol or WeakMap) keyed by the mark to store
whether handlers were wrapped and/or to store originals (e.g.,
originalQueryPending/originalQueryResult/originalQueryError); before replacing
mark.queryPending/queryResult/queryError check that marker and only create
wrappers if not already wrapped, otherwise leave existing handlers intact.
Ensure the marker is used consistently when wrapping logic around QueryableMark
methods so repeated calls to wrapMarkHandlers do not nest wrappers or repeat
checkAndReportAggregateState/onError/reportIssue/clearIssue effects.
In `@packages/mosaic/src/VgPlotChart.tsx`:
- Around line 186-190: The current code hides VgPlotChartError whenever a
runtimeIssueReporter is present; update VgPlotChart so its fallback error UI is
independent from runtimeIssueReporter by removing the implicit suppression
(remove the hasRuntimeIssueReporter check) and instead gate the fallback only by
an explicit opt-out prop (e.g., introduce and check isSpecProps(props) &&
props.disableFallbackErrorUI or similar). Locate the logic in VgPlotChart around
the error handling (the isSpecProps(props) check, runtimeIssueReporter
reference, and the VgPlotChartError rendering) and replace the
runtimeIssueReporter-based condition with the new explicit opt-out prop so
callers can opt out intentionally while public default remains self-contained.
---
Outside diff comments:
In `@packages/mosaic/src/useVgPlotChartRender.ts`:
- Around line 256-285: The code currently clears cachedChart.markStates and
cachedChart.markErrors before calling checkAndReportAggregateState, which causes
previous runtime errors to be lost; change the sequence so
checkAndReportAggregateState runs while cachedChart.markStates/markErrors are
still intact (i.e., remove or move the clear() calls for markStates and
markErrors until after checkAndReportAggregateState has executed), then continue
with attachPlotElement/asPlotDomElement, resizeChartElement, wrapMarkHandlers,
and only clear or reset retained state afterward if/when you intentionally
reinitialize marks.
---
Nitpick comments:
In `@packages/mosaic/src/charts/chart-types/bubble-chart/spec.ts`:
- Around line 71-79: The code currently calls isNumericType(xColumn.type) and
isNumericType(yColumn.type) twice; refactor the check by computing booleans
(e.g., xIsNumeric, yIsNumeric) or building a failingColumns array once, then use
those values both for the if-condition and to populate the
InvalidColumnTypeError; update the block surrounding isNumericType,
InvalidColumnTypeError, xColumn and yColumn so each column type is validated a
single time and the error array is constructed from that single result.
In `@packages/mosaic/src/charts/chart-types/heatmap/spec.ts`:
- Around line 71-79: Compute the numeric-check results once and reuse them
instead of calling isNumericType twice; e.g., evaluate const xNumeric =
isNumericType(xColumn.type) and const yNumeric = isNumericType(yColumn.type),
then use if (!xNumeric || !yNumeric) { throw new
InvalidColumnTypeError([...(!xNumeric ? [xColumn.name] : []), ...(!yNumeric ?
[yColumn.name] : [])], 'numeric'); } so you only call isNumericType for
xColumn.type and yColumn.type a single time (affecting the block that currently
references xColumn, yColumn, isNumericType, and InvalidColumnTypeError in
heatmap spec).
In `@packages/mosaic/src/charts/chart-types/line-chart/spec.ts`:
- Around line 171-176: The return statement currently casts the object to
ValidatedLineChartSettings unnecessarily; remove the "as
ValidatedLineChartSettings" type assertion so the function returns { xColumn,
yColumns, xInterval } directly (the object already conforms to the
ValidatedLineChartSettings type), ensuring no runtime change and cleaner code.
In `@packages/mosaic/src/charts/useMosaicChartRenderContext.ts`:
- Around line 87-97: Remove the two temporary console.log calls in
createMosaicSpecChartRenderContext that print settings and spec during
chartTypeDefinition.createSpec; either delete these console.log statements or
wrap them with a development-only guard (e.g., a check like process.env.NODE_ENV
=== 'development' or the project's devLogger) so they do not run in production,
ensuring no sensitive chart settings/specs are dumped to the browser console on
every recomputation.
In `@packages/mosaic/src/components/BlockCaptionEditor.tsx`:
- Around line 12-28: Add a JSDoc block above the exported const
BlockCaptionEditor that briefly describes the component's purpose (a small
caption editor for blocks) and documents its props via BlockCaptionEditorProps:
value, className, placeholder, isReadOnly, and onChange (include types/expected
behavior for each prop and note that it renders EditableText). Keep it short
(one-line summary + param tags) and place it immediately above the
BlockCaptionEditor declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55cf37fd-fa34-4298-b8ba-881077436f6f
📒 Files selected for processing (27)
packages/mosaic/__tests__/LineChartSettings.test.tsxpackages/mosaic/src/VgPlotChart.tsxpackages/mosaic/src/charts/MosaicChartError.tsxpackages/mosaic/src/charts/MosaicChartSettingsButton.tsxpackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsxpackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/bubble-chart/spec.tspackages/mosaic/src/charts/chart-types/count-plot/spec.tspackages/mosaic/src/charts/chart-types/custom-spec/spec.tspackages/mosaic/src/charts/chart-types/errors.tspackages/mosaic/src/charts/chart-types/heatmap/spec.tspackages/mosaic/src/charts/chart-types/histogram/spec.tspackages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/charts/chart-types/line-chart/spec.tspackages/mosaic/src/charts/dashboard/MosaicDashboardChartHeaderActions.tsxpackages/mosaic/src/charts/useMosaicChartRenderContext.tspackages/mosaic/src/charts/worksheet/ChartBlockHeader.tsxpackages/mosaic/src/components/BlockCaptionEditor.tsxpackages/mosaic/src/components/FieldSelectorInput.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardAddPanelDropdown.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardDataTableSelector.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardResetFiltersButton.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardToolbar.tsxpackages/mosaic/src/data-table-explorer/DataTableExplorer.tsxpackages/mosaic/src/useVgPlotChartRender.tspackages/mosaic/src/useVgPlotChartRetention.ts
💤 Files with no reviewable changes (1)
- packages/mosaic/src/dashboard/toolbar/MosaicDashboardResetFiltersButton.tsx
Collaborator
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4452d74ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mosaic/src/useVgPlotChartRender.ts (2)
230-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall the stored
queryErrorhandler, not the wrapped one.On the policy-failure path, Line 242 invokes
mark.queryError, but that function has already been replaced by the wrapper below. The same failure therefore re-runs aggregate reporting, and Line 270 also drops the original handler's return value.Suggested fix
} catch (error) { const normalizedError = error instanceof Error ? error : new Error(String(error)); chart.markStates!.set(mark, 'error'); chart.markErrors!.set(mark, normalizedError); checkAndReportAggregateState( chart, onError, runtimeIssueReporter, runtimeIssueContext, dataPolicy, ); - if (mark.queryError) { - return mark.queryError(normalizedError); + const originalQueryError = markWithOriginals[ORIGINAL_QUERY_ERROR]; + if (originalQueryError) { + return originalQueryError.call(this, normalizedError); } return undefined; } @@ mark.queryError = function (this: QueryableMark, error: unknown) { const normalizedError = error instanceof Error ? error : new Error(String(error)); chart.markStates?.set(mark, 'error'); chart.markErrors?.set(mark, normalizedError); checkAndReportAggregateState( chart, onError, runtimeIssueReporter, runtimeIssueContext, dataPolicy, ); - originalQueryError.call(this, error); + return originalQueryError.call(this, error); };Also applies to: 256-270
🤖 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 230 - 244, The catch block is calling mark.queryError which at this point has been replaced by a wrapper, causing duplicate aggregate reporting and dropping the original handler's return; update the logic to call and return the original stored handler (e.g., the pre-wrapped reference you saved when wrapping, such as originalQueryError or _queryErrorHandler) instead of mark.queryError, and ensure you invoke it with normalizedError; also mirror this change in the similar policy-failure path around the 256-270 region and avoid re-running checkAndReportAggregateState by only calling that once before invoking the original handler.
117-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
idlemarks as unfinished when computing aggregate completion.Because every queryable mark is registered as
'idle'on Lines 195-197, the currentstate !== 'pending'check lets the first finished mark resolve the aggregate while other marks are still idle. That brings back earlyclearIssue/onErrortransitions before the whole plot has settled.Suggested fix
- const allDone = Array.from(chart.markStates.values()).every( - (state) => state !== 'pending', - ); + const allDone = Array.from(chart.markStates.values()).every( + (state) => state === 'success' || state === 'error', + );Also applies to: 194-197
🤖 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 117 - 124, The aggregate-completion check uses chart.markStates and the allDone predicate but treats 'idle' as finished; update the check used when computing allDone (the Array.from(chart.markStates.values()).every(...) that returns early) so that it only considers marks finished when their state is explicitly 'done' (or equivalently exclude both 'pending' and 'idle')—this prevents early resolution while marks registered as 'idle' (see where marks are set to 'idle') are still waiting; change the predicate to require state === 'done' (or state !== 'pending' && state !== 'idle') so clearIssue/onError wait for all marks.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@packages/mosaic/src/useVgPlotChartRender.ts`:
- Around line 230-244: The catch block is calling mark.queryError which at this
point has been replaced by a wrapper, causing duplicate aggregate reporting and
dropping the original handler's return; update the logic to call and return the
original stored handler (e.g., the pre-wrapped reference you saved when
wrapping, such as originalQueryError or _queryErrorHandler) instead of
mark.queryError, and ensure you invoke it with normalizedError; also mirror this
change in the similar policy-failure path around the 256-270 region and avoid
re-running checkAndReportAggregateState by only calling that once before
invoking the original handler.
- Around line 117-124: The aggregate-completion check uses chart.markStates and
the allDone predicate but treats 'idle' as finished; update the check used when
computing allDone (the Array.from(chart.markStates.values()).every(...) that
returns early) so that it only considers marks finished when their state is
explicitly 'done' (or equivalently exclude both 'pending' and 'idle')—this
prevents early resolution while marks registered as 'idle' (see where marks are
set to 'idle') are still waiting; change the predicate to require state ===
'done' (or state !== 'pending' && state !== 'idle') so clearIssue/onError wait
for all marks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1421653d-0cd4-4c27-ab1a-2d49dbe04aa8
📒 Files selected for processing (11)
packages/mosaic/src/charts/MosaicChartError.tsxpackages/mosaic/src/charts/MosaicChartSettingsButton.tsxpackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/chart-types/bubble-chart/spec.tspackages/mosaic/src/charts/chart-types/errors.tspackages/mosaic/src/charts/chart-types/heatmap/spec.tspackages/mosaic/src/charts/useMosaicChartRenderContext.tspackages/mosaic/src/column-types-utils.tspackages/mosaic/src/components/BlockCaptionEditor.tsxpackages/mosaic/src/useVgPlotChartRender.tspackages/mosaic/src/useVgPlotChartRetention.ts
💤 Files with no reviewable changes (2)
- packages/mosaic/src/charts/useMosaicChartRenderContext.ts
- packages/mosaic/src/charts/MosaicChartView.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mosaic/src/charts/MosaicChartError.tsx
- packages/mosaic/src/components/BlockCaptionEditor.tsx
- packages/mosaic/src/charts/MosaicChartSettingsButton.tsx
- packages/mosaic/src/charts/chart-types/bubble-chart/spec.ts
ilyabo
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements