Skip to content

fix: improve deep cloning for graph and resampling to handle non-cloneable properties #1365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SheepFromHeaven wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: improve deep cloning for graph and resampling to handle non-cloneable properties #1365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion public/modules/dynamic/heightmap-selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,14 @@ function getName(id) {
}

function getGraph(currentGraph) {
const newGraph = shouldRegenerateGrid(currentGraph, seed) ? generateGrid() : structuredClone(currentGraph);
if (shouldRegenerateGrid(currentGraph, seed)) return generateGrid();

// Deep clone avoiding non-cloneable properties (like D3 quadtrees with functions)
const newGraph = JSON.parse(JSON.stringify(currentGraph));
// Recreate voronoi cells since JSON doesn't preserve typed arrays or circular refs
const {cells, vertices} = calculateVoronoi(newGraph.points, newGraph.boundary);
newGraph.cells = cells;
newGraph.vertices = vertices;
delete newGraph.cells.h;
return newGraph;
Comment on lines +265 to 272
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using JSON.parse(JSON.stringify(currentGraph)) as a deep clone is fragile and can be very expensive for large graphs. It also drops/rewrites non-JSON-safe values (typed arrays, undefined, Infinity/NaN, etc.), and will still throw if any circular references exist. Consider cloning only the specific fields needed for preview generation (e.g., points/boundary/dimensions), or using a dedicated clone that explicitly omits the non-cloneable quadtree rather than serializing the entire graph.

Copilot uses AI. Check for mistakes.
}
Expand Down
48 changes: 45 additions & 3 deletions src/modules/resample.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,48 @@
import type { River } from "./river-generator";
import type { Point } from "./voronoi";

/**
* Deeply clones an object, omitting properties that cannot be cloned with structuredClone.
* D3 quadtrees store accessor functions that fail structuredClone, so they're excluded.
*/
const safeDeepClone = <T>(obj: T): T => {
if (obj === null || typeof obj !== "object") {
return obj;
}

// Handle typed arrays - slice() creates a copy with a new underlying buffer
if (ArrayBuffer.isView(obj) && !(obj instanceof DataView)) {
return (obj as any).slice() as T;
}

// Handle arrays
if (Array.isArray(obj)) {
return obj.map((item) => safeDeepClone(item)) as T;
}

// Handle objects - skip quadtree (has functions) and other non-clonable properties
const cloned: Record<string, unknown> = {};
for (const key in obj) {
if (!Object.prototype.hasOwnProperty.call(obj, key)) continue;

Check warning on line 38 in src/modules/resample.ts

View workflow job for this annotation

GitHub Actions / quality

lint/suspicious/noPrototypeBuiltins 

Do not access Object.prototype method 'hasOwnProperty' from target object.

const value = (obj as Record<string, unknown>)[key];

// Skip quadtree properties (D3 quadtrees have _x and _y functions)
if (value && typeof value === "object" && "_x" in (value as object)) {
Comment on lines +42 to +43
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quadtree-detection heuristic ("_x" in value) is fairly broad and can skip cloning for any object that happens to have an _x property, even if it is not a D3 quadtree. If you only need to skip the known cells.q quadtree field, prefer an explicit key/path check (e.g., when key === "q" under cells) to avoid unintentionally dropping unrelated data.

Suggested change
// Skip quadtree properties (D3 quadtrees have _x and _y functions)
if (value && typeof value === "object" && "_x" in (value as object)) {
// Skip known quadtree properties (D3 quadtrees have _x and _y functions)
if (key === "q" && value && typeof value === "object" && "_x" in (value as object)) {

Copilot uses AI. Check for mistakes.
continue;
}

// Skip functions
if (typeof value === "function") {
continue;
}

cloned[key] = safeDeepClone(value);
}

return cloned as T;
};
Comment on lines +20 to +56
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safeDeepClone creates plain objects via {} and recursively copies enumerable own props, which does not preserve prototypes (e.g., Quadtree instances, classes) and may change behavior if any cloned values rely on their prototype methods. If the intent is only to omit pack.cells.q, consider explicitly stripping that field (or using structuredClone with a targeted fallback) so other object types keep their identity/shape as much as possible.

Copilot uses AI. Check for mistakes.

declare global {
var Resample: Resampler;
}
Expand Down Expand Up @@ -495,9 +537,9 @@
process(options: ResamplerProcessOptions): void {
const { projection, inverse, scale } = options;
const parentMap = {
grid: structuredClone(grid),
pack: structuredClone(pack),
notes: structuredClone(notes),
grid: safeDeepClone(grid),
pack: safeDeepClone(pack),
notes: safeDeepClone(notes),
};
const riversData = this.saveRiversData(pack.rivers);

Expand Down
2 changes: 1 addition & 1 deletion src/types/PackedGraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export interface PackedGraph {
p: [number, number][]; // cell polygon points
b: boolean[]; // cell is on border
h: TypedArray; // cell heights
q: Quadtree<[number, number, number]>; // cell quadtree index
q?: Quadtree<[number, number, number]>; // cell quadtree index (optional, not cloned)
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making PackedGraph.cells.q optional is a type-level breaking change for any code that expects cells.q to always exist. If q is only non-cloneable transient state, consider modeling it outside of PackedGraph (or as a separate runtime-only augmentation type) so consumers of PackedGraph don't need to handle undefined everywhere.

Copilot uses AI. Check for mistakes.
/** Terrain type */
t: TypedArray; // cell terrain types
r: TypedArray; // river id passing through cell
Expand Down
Loading