Copilot commented on code in PR #37244:
URL: https://github.com/apache/superset/pull/37244#discussion_r2707573032
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -159,11 +159,44 @@ export const getLayer: GetLayerType<ContourLayer> =
function ({
return baseTooltipContent(o);
};
+ const MIN_CELL_SIZE = 10;
+ const MAX_CELL_SIZE = 5000;
+ const MAX_GRID_CELLS = 1_000_000;
+
+ let parsedCellSize = Number(cellSize || 200);
+ if (!Number.isFinite(parsedCellSize)) {
+ parsedCellSize = 200;
+ }
+
+ let safeCellSize = Math.min(
+ MAX_CELL_SIZE,
+ Math.max(MIN_CELL_SIZE, parsedCellSize),
+ );
+
+ const viewport = fd.viewport;
+ if (viewport?.width && viewport?.height) {
+ const estimatedCells =
+ (viewport.width / safeCellSize) * (viewport.height / safeCellSize);
+
+ if (estimatedCells > MAX_GRID_CELLS) {
+ const scaleFactor = Math.sqrt(estimatedCells / MAX_GRID_CELLS);
+ const adjusted = Math.ceil(safeCellSize * scaleFactor);
+
+ console.warn(
+ `[DeckGL Contour] cellSize=${safeCellSize} would create ~${Math.round(
+ estimatedCells,
+ )} cells. Auto-adjusted to ${adjusted} to prevent WebGL crash.`,
+ );
+
+ safeCellSize = adjusted;
Review Comment:
After auto-adjustment, the safeCellSize could exceed MAX_CELL_SIZE. For
example, if safeCellSize is 5000 and the scaleFactor is greater than 1, the
adjusted value will be larger than 5000. Consider clamping the adjusted value
to ensure it doesn't exceed MAX_CELL_SIZE after line 191.
```suggestion
safeCellSize = Math.min(MAX_CELL_SIZE, adjusted);
```
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -159,11 +159,44 @@ export const getLayer: GetLayerType<ContourLayer> =
function ({
return baseTooltipContent(o);
};
+ const MIN_CELL_SIZE = 10;
+ const MAX_CELL_SIZE = 5000;
+ const MAX_GRID_CELLS = 1_000_000;
+
+ let parsedCellSize = Number(cellSize || 200);
+ if (!Number.isFinite(parsedCellSize)) {
+ parsedCellSize = 200;
+ }
+
+ let safeCellSize = Math.min(
+ MAX_CELL_SIZE,
+ Math.max(MIN_CELL_SIZE, parsedCellSize),
+ );
+
+ const viewport = fd.viewport;
+ if (viewport?.width && viewport?.height) {
+ const estimatedCells =
+ (viewport.width / safeCellSize) * (viewport.height / safeCellSize);
+
+ if (estimatedCells > MAX_GRID_CELLS) {
+ const scaleFactor = Math.sqrt(estimatedCells / MAX_GRID_CELLS);
+ const adjusted = Math.ceil(safeCellSize * scaleFactor);
+
+ console.warn(
+ `[DeckGL Contour] cellSize=${safeCellSize} would create ~${Math.round(
+ estimatedCells,
+ )} cells. Auto-adjusted to ${adjusted} to prevent WebGL crash.`,
+ );
+
+ safeCellSize = adjusted;
+ }
+ }
Review Comment:
The cellSize validation and auto-scaling logic introduced here is
safety-critical and would benefit from unit tests. Consider adding tests to
verify: 1) non-finite values default to 200, 2) values below MIN_CELL_SIZE are
clamped to 10, 3) values above MAX_CELL_SIZE are clamped to 5000, 4)
auto-scaling triggers when estimated cells exceed MAX_GRID_CELLS, and 5) the
warning message is logged when auto-adjustment occurs.
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -159,11 +159,44 @@ export const getLayer: GetLayerType<ContourLayer> =
function ({
return baseTooltipContent(o);
};
+ const MIN_CELL_SIZE = 10;
+ const MAX_CELL_SIZE = 5000;
+ const MAX_GRID_CELLS = 1_000_000;
+
+ let parsedCellSize = Number(cellSize || 200);
+ if (!Number.isFinite(parsedCellSize)) {
+ parsedCellSize = 200;
+ }
+
+ let safeCellSize = Math.min(
+ MAX_CELL_SIZE,
+ Math.max(MIN_CELL_SIZE, parsedCellSize),
+ );
+
+ const viewport = fd.viewport;
+ if (viewport?.width && viewport?.height) {
+ const estimatedCells =
+ (viewport.width / safeCellSize) * (viewport.height / safeCellSize);
+
+ if (estimatedCells > MAX_GRID_CELLS) {
+ const scaleFactor = Math.sqrt(estimatedCells / MAX_GRID_CELLS);
+ const adjusted = Math.ceil(safeCellSize * scaleFactor);
+
+ console.warn(
+ `[DeckGL Contour] cellSize=${safeCellSize} would create ~${Math.round(
+ estimatedCells,
+ )} cells. Auto-adjusted to ${adjusted} to prevent WebGL crash.`,
+ );
+
+ safeCellSize = adjusted;
Review Comment:
The variable name 'adjusted' could be more descriptive. Consider renaming it
to 'adjustedCellSize' to make it clearer what is being adjusted.
```suggestion
const adjustedCellSize = Math.ceil(safeCellSize * scaleFactor);
console.warn(
`[DeckGL Contour] cellSize=${safeCellSize} would create
~${Math.round(
estimatedCells,
)} cells. Auto-adjusted to ${adjustedCellSize} to prevent WebGL
crash.`,
);
safeCellSize = adjustedCellSize;
```
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -159,11 +159,44 @@ export const getLayer: GetLayerType<ContourLayer> =
function ({
return baseTooltipContent(o);
};
+ const MIN_CELL_SIZE = 10;
+ const MAX_CELL_SIZE = 5000;
+ const MAX_GRID_CELLS = 1_000_000;
+
+ let parsedCellSize = Number(cellSize || 200);
+ if (!Number.isFinite(parsedCellSize)) {
+ parsedCellSize = 200;
+ }
+
+ let safeCellSize = Math.min(
+ MAX_CELL_SIZE,
+ Math.max(MIN_CELL_SIZE, parsedCellSize),
+ );
+
+ const viewport = fd.viewport;
+ if (viewport?.width && viewport?.height) {
Review Comment:
If viewport width or height is zero, the estimatedCells calculation will
result in Infinity, which will always trigger auto-scaling. While this may
work, it would be clearer to add an explicit check for zero or negative
dimensions to avoid division by zero and handle this edge case more explicitly.
```suggestion
if (viewport && viewport.width > 0 && viewport.height > 0) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
