Write embedded GeoJSON compactly when saving a project (#1829) - #1832
Conversation
`serializeProject` pretty-printed the entire `.geolibre.json`, so every coordinate value got its own indented line. Coordinate arrays are never hand-edited, and the whitespace cost roughly three bytes for every byte of data: a project embedding 367k features weighed 158 MB and 5.9M lines pretty-printed, but 61 MB and 668 lines compact. Files that large made reopening unreliable, with layers rendering briefly and then vanishing as the tab ran out of memory. Serialization now indents the project structure as before but hands each GeoJSON feature, geometry and collection to `JSON.stringify` with no spacing, so files stay readable and diffable while dropping the bloat. This applies everywhere projects are serialized: save, share, autosave, collaboration sync and the embed bridge. Two related fixes on the save path: - `buildCurrentProject` no longer serializes the project it returns. Every caller re-serialized after redacting credentials, so the unused string doubled the peak memory of a save. - Serialization throws `RangeError: Invalid string length` past V8's ~536 MB string cap. That escaped `void handleSave()` as an unhandled rejection, so Save silently did nothing; it now reports a visible error suggesting PMTiles or FlatGeobuf. The embed prompt also warns above 50 MB of embedded data, pointing at PMTiles/FlatGeobuf rather than embedding features.
📝 WalkthroughWalkthroughThe project serializer now compacts embedded GeoJSON while preserving project formatting. Save operations defer serialization until credential handling completes, report serialization failures, and warn about large embedded data. Localized messages cover these warnings and save errors. ChangesProject save-size handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectFileDialogs
participant ProjectFileActions
participant serializeProject
User->>ProjectFileDialogs: embed vector data
ProjectFileDialogs->>ProjectFileActions: create project snapshot
ProjectFileActions->>serializeProject: serialize finalized project
serializeProject-->>ProjectFileActions: serialized content or error
ProjectFileActions-->>ProjectFileDialogs: warning or save error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/layout/toolbar/ProjectFileDialogs.tsx (1)
245-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the repeated
formatByteSizecall.
formatByteSize(projectFiles.embedVectorDataPrompt?.bytes ?? 0)runs twice: once for the description text and once for the warning text. Compute it once and reuse the value.♻️ Proposed refactor
+ {(() => { + const embedBytes = projectFiles.embedVectorDataPrompt?.bytes ?? 0; + const formattedSize = formatByteSize(embedBytes); + return ( + <> <DialogHeader> <DialogTitle>{t("toolbar.item.embedVectorTitle")}</DialogTitle> <DialogDescription> {t( projectFiles.embedVectorDataPrompt?.desktop ? "toolbar.item.embedVectorDescDesktop" : "toolbar.item.embedVectorDesc", { count: projectFiles.embedVectorDataPrompt?.count ?? 0, - size: formatByteSize(projectFiles.embedVectorDataPrompt?.bytes ?? 0), + size: formattedSize, }, )} </DialogDescription> </DialogHeader> - {(projectFiles.embedVectorDataPrompt?.bytes ?? 0) >= LARGE_EMBED_WARNING_BYTES ? ( + {embedBytes >= LARGE_EMBED_WARNING_BYTES ? ( <p role="alert" className="rounded-md border border-amber-500/40 bg-amber-500/10 p-3 text-sm text-amber-700 dark:text-amber-300" > - {t("toolbar.item.embedVectorLargeWarning", { - size: formatByteSize(projectFiles.embedVectorDataPrompt?.bytes ?? 0), - })} + {t("toolbar.item.embedVectorLargeWarning", { size: formattedSize })} </p> ) : null} + </> + ); + })()}🤖 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 `@apps/geolibre-desktop/src/components/layout/toolbar/ProjectFileDialogs.tsx` around lines 245 - 265, In the component rendering the embed vector data dialog, compute formatByteSize(projectFiles.embedVectorDataPrompt?.bytes ?? 0) once before the DialogDescription and warning markup, store the result in a local value, and reuse it for both translation calls. Preserve the existing fallback and warning behavior.
🤖 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 `@apps/geolibre-desktop/src/hooks/useProjectFileActions.ts`:
- Around line 682-700: Update serializeForSave so the projectTooLargeToSave
message is selected only when the RangeError matches the known V8 string-length
failure, such as by checking its message. Route RangeErrors from custom toJSON
methods, getters, recursion, or any other cause to couldNotSaveProject, while
preserving the existing error logging and null return behavior.
In `@packages/core/src/project.ts`:
- Around line 167-171: Update the array serialization branch in
serializeProjectValue to visit every numeric index, including sparse holes, and
emit "null" for missing slots. Preserve the existing formatting and conversion
behavior for populated entries and empty arrays.
- Around line 154-159: Update serializeProjectValue and every recursive
serializeProjectValue(entry, depth + 1) call to accept and propagate the
containing property key or array index, then pass that key to the value’s toJSON
method instead of an empty string. Preserve the existing top-level behavior with
an empty key where no containing key exists.
In `@tests/core-project.test.ts`:
- Around line 636-649: Extend the test in “matches JSON.stringify for values it
drops, empty containers, and toJSON” with key-sensitive toJSON props in both an
object and an array, asserting serializeProject matches JSON.stringify. Also add
a sparse array such as [1, , 2] and include the same comparison, preserving the
existing edge-case coverage.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/toolbar/ProjectFileDialogs.tsx`:
- Around line 245-265: In the component rendering the embed vector data dialog,
compute formatByteSize(projectFiles.embedVectorDataPrompt?.bytes ?? 0) once
before the DialogDescription and warning markup, store the result in a local
value, and reuse it for both translation calls. Preserve the existing fallback
and warning behavior.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b6e08ec-bbff-48dc-baa4-b1ab6f2f768c
📒 Files selected for processing (22)
apps/geolibre-desktop/src/components/layout/toolbar/ProjectFileDialogs.tsxapps/geolibre-desktop/src/hooks/useProjectFileActions.tsapps/geolibre-desktop/src/i18n/locales/ar.jsonapps/geolibre-desktop/src/i18n/locales/de.jsonapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/i18n/locales/es.jsonapps/geolibre-desktop/src/i18n/locales/fa.jsonapps/geolibre-desktop/src/i18n/locales/fr.jsonapps/geolibre-desktop/src/i18n/locales/hi.jsonapps/geolibre-desktop/src/i18n/locales/id.jsonapps/geolibre-desktop/src/i18n/locales/it.jsonapps/geolibre-desktop/src/i18n/locales/ja.jsonapps/geolibre-desktop/src/i18n/locales/ka.jsonapps/geolibre-desktop/src/i18n/locales/ko.jsonapps/geolibre-desktop/src/i18n/locales/nl.jsonapps/geolibre-desktop/src/i18n/locales/pt.jsonapps/geolibre-desktop/src/i18n/locales/ru.jsonapps/geolibre-desktop/src/i18n/locales/th.jsonapps/geolibre-desktop/src/i18n/locales/tr.jsonapps/geolibre-desktop/src/i18n/locales/zh.jsonpackages/core/src/project.tstests/core-project.test.ts
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Quality
Security / Performance / CLAUDE.md
|
- Detect circular references in the project serializer and throw the TypeError JSON.stringify raises, instead of recursing until the stack overflows. The RangeError an overflow raises is indistinguishable from the string-length cap the save path reads as "project too large". - Visit sparse array holes by index rather than mapping, so a hole is written as null the way JSON.stringify writes it (mapping skipped the hole and emitted invalid JSON). - Pass the property name (or stringified array index) to a value's toJSON hook, matching the key JSON.stringify supplies. - Narrow the "project too large to save" message to the string-length RangeError, so other RangeErrors report a generic save failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/src/project.ts`:
- Line 171: Update the serializer’s primitive/object boundary in project.ts to
detect boxed Number, String, and Boolean values and serialize their unboxed
primitives before recursing through Object.entries(value). Preserve existing
handling for null, non-objects, and ordinary objects, matching native
JSON.stringify output for boxed primitives.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2223481-986b-4ce8-949b-aef0227fc21b
📒 Files selected for processing (3)
apps/geolibre-desktop/src/hooks/useProjectFileActions.tspackages/core/src/project.tstests/core-project.test.ts
Code reviewBugs: None found. I traced the new Security: None found. Performance: None found — the change is a net performance/memory win as described (removes a duplicate full serialization on every save, and the compact GeoJSON formatting is the intended fix for the underlying OOM/slow-reopen issue). Low confidence aside: a hand-written recursive stringifier will hit V8's call-stack limit at a shallower depth than the native routine for extremely deep (non-cyclic) nesting, but this is already anticipated by the code (the Quality: Two low-confidence nits posted inline on One additional observation (not inline, since it touches files outside this diff): the new large-embed warning ( CLAUDE.md adherence: The i18n changes follow the documented convention ( |
- Unwrap boxed Number/String/Boolean objects to their primitives, the way
JSON.stringify does, instead of recursing over their (empty) own
properties and writing {}.
- Document that isGeoJsonValue discriminates on the `type` string alone,
so a future schema field must not reuse one of those nine names for a
non-GeoJSON value.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
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 (1)
packages/core/src/project.ts (1)
210-212: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRead each object property when it is serialized.
Object.entries(value)evaluates every property now, beforeserializeProjectValueprocesses the first entry. An earliertoJSONhook can mutate a later sibling and cause this serializer to serialize stale values. UseObject.keys(value)and readvalue[entryKey]in the loop; cover this with a regression test against nativeJSON.stringify.Proposed fix
- for (const [entryKey, entry] of Object.entries(value)) { + for (const entryKey of Object.keys(value)) { + const entry = (value as Record<string, unknown>)[entryKey]; const serialized = serializeProjectValue(entry, depth + 1, entryKey, ancestors);🤖 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/core/src/project.ts` around lines 210 - 212, Update the object-property loop in serializeProjectValue to iterate with Object.keys(value) and read value[entryKey] inside each iteration, ensuring each sibling is fetched only when serialized. Add a regression test comparing the serializer’s output with native JSON.stringify when an earlier toJSON hook mutates a later property.
🤖 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/core/src/project.ts`:
- Around line 210-212: Update the object-property loop in serializeProjectValue
to iterate with Object.keys(value) and read value[entryKey] inside each
iteration, ensuring each sibling is fetched only when serialized. Add a
regression test comparing the serializer’s output with native JSON.stringify
when an earlier toJSON hook mutates a later property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10086b47-84ac-45fe-9455-c793419d6a98
📒 Files selected for processing (2)
packages/core/src/project.tstests/core-project.test.ts
Detect the "too large to serialize" failure across webview engines, not just V8. GeoLibre's Tauri webview is JavaScriptCore on macOS and Linux, which reports an out-of-memory error rather than V8's "Invalid string length", so matching only V8's wording left desktop users on those platforms with the generic save failure instead of the PMTiles/FlatGeobuf guidance. The check now tests the message against the known engine phrasings, still narrow enough that a cycle or other real serialization bug reports generically.
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 (1)
apps/geolibre-desktop/src/hooks/useProjectFileActions.ts (1)
686-696: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply the same visibility and credential-stripping contract to template saves.
SaveTemplateDialogstill receives the raw project frombuildCurrentProjectand passes it directly tocreateProjectTemplate, so templates can keep hidden GeoJSON fields and plain-text credentials. Strip those from the project before creating the template, and add coverage for template egress.🤖 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 `@apps/geolibre-desktop/src/hooks/useProjectFileActions.ts` around lines 686 - 696, Update the template-save flow in SaveTemplateDialog to sanitize the project returned by buildCurrentProject before passing it to createProjectTemplate, applying the same visibility filtering and credential redaction used for project saves. Ensure template output excludes hidden GeoJSON fields and plain-text credentials, and add coverage verifying this sanitized egress.
🤖 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 `@apps/geolibre-desktop/src/hooks/useProjectFileActions.ts`:
- Around line 686-696: Update the template-save flow in SaveTemplateDialog to
sanitize the project returned by buildCurrentProject before passing it to
createProjectTemplate, applying the same visibility filtering and credential
redaction used for project saves. Ensure template output excludes hidden GeoJSON
fields and plain-text credentials, and add coverage verifying this sanitized
egress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 371321c7-c9bb-433e-99ed-5662f35d3c2f
📒 Files selected for processing (1)
apps/geolibre-desktop/src/hooks/useProjectFileActions.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Fixes #1829
The problem
serializeProjectpretty-printed the whole.geolibre.jsonat two-space indent, so every single coordinate value got its own indented line. Coordinate arrays are never hand-edited, and the whitespace cost roughly three bytes for every byte of data.The fix
Serialization keeps the project structure indented (so the file is still readable and still diffs sensibly) but hands every GeoJSON feature, geometry and collection to
JSON.stringifywith no spacing. Since embedded feature data is essentially all of a large project's bytes, the result comes out within a few percent of a fully minified file without turning the whole thing into one unreadable line.This applies everywhere a project is serialized, not just Save: Share, autosave snapshots, collaboration sync and the embed bridge all go through the same function.
Measured on the real app with
dem_points.geojson(366,411 features) plusOTEX_p.geojson(877 features), saved with data embedded:A smaller single-layer case (
OTEX_p.geojsonalone) went from 23.0 MiB / 759,096 lines to 7.2 MiB / 385 lines, the same ~3.2x the reporter measured.Two related fixes on the save path
buildCurrentProjectno longer serializes the project it returns. Every caller re-serialized afterwards anyway (credentials are redacted first), so that unused string doubled the peak memory of a save.RangeError: Invalid string lengthonce the text passes V8's ~536 MB string cap. That throw escapedvoid handleSave()as an unhandled rejection, so Save silently did nothing. It is now caught and surfaced as a visible error suggesting PMTiles or FlatGeobuf, matching the guidance in the issue thread. (Autosave already guarded this; the explicit save path did not.)Size warning
The embed prompt now shows an inline warning above 50 MB of embedded data, recommending PMTiles or FlatGeobuf instead of embedding features. The prompt's size estimate was already computed compactly, so with this change it finally matches the file that actually lands: the dialog said 61.3 MB and the file was 61.3 MiB.
New string added to
en.jsonand all 17 other locale catalogs.Verification
Real app at
localhost:5173, Chromium via Playwright:OTEX_p.geojson-> Save with data embedded -> 7.2 MiB / 385 lines, structure still indented, feature collection on one line.dem_points.geojsonon top (61.3 MB embedded) -> the large-embed warning appears; checked in both dark and light themes.New tests in
tests/core-project.test.tscover the compact output for bothlayer.geojsonandmetadata.embeddedGeoJSON, the size ratio against a fully minified file, byte-identical output toJSON.stringify(project, null, 2)for a project with no GeoJSON, matchingJSON.stringifysemantics for dropped values / empty containers /toJSON, and aparseProjectround trip.npm run test:frontend(5735 pass),npm run typecheck, andpre-commitall green.Summary by CodeRabbit
New Features
Bug Fixes
Localization