chore: Enhance schema compatibility checks and error handling#576
chore: Enhance schema compatibility checks and error handling#576softmarshmallow merged 2 commits intomainfrom
Conversation
- Updated the CanvasPlayground component to improve error handling for document data, ensuring that incompatible files are quarantined to preserve user data. - Introduced a new schema compatibility check in the grida-canvas-io package, throwing an error if the file version is not compatible with the current schema version. - Added detailed documentation for the schema compatibility logic in the grida-canvas-schema package, clarifying versioning policy and compatibility rules. These changes enhance data integrity and user experience by safeguarding against data corruption and ensuring compatibility with schema updates.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
WalkthroughAdd a schema compatibility API and enforce schema version checks during file decoding; broaden OPFS load error handling to quarantine existing data for any non-"not found" failures and update the related log message. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant OPFS as OPFS (storage)
participant Decoder as Format Decoder
participant Schema as Schema Checker
participant Quarantine as Quarantine Store
Client->>OPFS: openHandle(key)
OPFS-->>Client: handle / not found / error
alt handle returned
Client->>OPFS: read(handle)
OPFS-->>Client: data
Client->>Decoder: decode(data)
Decoder->>Schema: extract schemaVersion
Schema->>Schema: isSchemaCompatible(fileVersion)
alt compatible
Decoder-->>Client: document
else incompatible or Schema throws
Client->>Quarantine: quarantine(handle, reason)
Client-->>Client: surface error
end
else not found
Client-->>Client: proceed with empty/new doc
else error (other than not found)
Client->>Quarantine: quarantine(handle, reason)
Client-->>Client: surface error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: 59aed69a6b
ℹ️ 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".
| } else { | ||
| // Any decode, structural validation, or state-init failure |
There was a problem hiding this comment.
This new else branch quarantines on every non-not found exception from the OPFS load path, so transient failures (for example Failed to read OPFS document.grida: ... from storage/runtime issues) and editor runtime errors from reset()/loadImages() now trigger opfs.quarantine(). Because quarantine() moves files and removes the originals (removeEntry in packages/grida-canvas-io/index.ts), a temporary failure can effectively hide otherwise valid user data and force fallback to an empty/default document. Quarantine should remain limited to decode/schema incompatibility signals, not all load-time exceptions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/grida-canvas-schema/grida.ts (1)
550-561: Consider failing fast if the current SCHEMA_VERSION is malformed.If
parseVersion(SCHEMA_VERSION)returnsnulldue to a malformed constant (e.g., merge error or typo), all compatibility checks silently returnfalse, causing every file to be rejected without clear indication of the root cause.Since
SCHEMA_VERSIONis controlled internally, a parsing failure there indicates a bug in this codebase rather than invalid user input. Throwing an error would surface this issue immediately during development/testing.Proposed fix
export function isSchemaCompatible(fileVersion: string): boolean { const current = parseVersion(SCHEMA_VERSION); + if (!current) { + throw new Error( + `Invalid SCHEMA_VERSION constant: "${SCHEMA_VERSION}". Expected semver format.` + ); + } const file = parseVersion(fileVersion); - if (!current || !file) return false; + if (!file) return false; if (current.major === 0) {Based on learnings: "When breaking compatibility... readers should... reject with a clear error—do not silently misinterpret old data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-schema/grida.ts` around lines 550 - 561, The function isSchemaCompatible currently treats a malformed internal SCHEMA_VERSION the same as any bad input; change it so that after calling parseVersion(SCHEMA_VERSION) you fail fast if current is null by throwing a clear error (e.g., include SCHEMA_VERSION value) so developers see the broken constant; keep the existing logic for when file parseVersion(fileVersion) returns null (return false) and preserve the branch logic tied to current.major === 0 and post-1.0 behavior in isSchemaCompatible.editor/grida-canvas-hosted/playground/playground.tsx (1)
502-513: Scope quarantine to compatibility/decode errors only.With Line 502 now treating all non-
"not found"failures as quarantine-worthy, transient OPFS/read/runtime failures can quarantine otherwise valid data. Consider gating quarantine to known compatibility/decode failures and leaving other errors as recoverable/log-only.Based on learnings: “Readers should use
schema_versionto decide whether to accept and decode normally, accept with best-effort degradation, migrate, or reject with a clear error.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-hosted/playground/playground.tsx` around lines 502 - 513, The current catch treats any non-"not found" error as grounds to call opfs?.quarantine(), which can quarantine valid data on transient I/O/runtime failures; change the handler so only known compatibility/decoding/schema-mismatch errors trigger opfs?.quarantine(). Concretely: in the catch that currently logs "OPFS document unusable ..." and calls opfs?.quarantine(), detect specific error types or markers (e.g., error.name === "DecodeError" || error.code === "INCOMPATIBLE_SCHEMA" || a schema_version mismatch after reading metadata) and call opfs?.quarantine() only for those; for all other errors (I/O, transient, runtime) emit a warn/error log with error details and fall back to the default document without quarantining. Ensure you reference the same error variable and call opfs?.quarantine() only under the compatibility/validation checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@editor/grida-canvas-hosted/playground/playground.tsx`:
- Around line 502-513: The current catch treats any non-"not found" error as
grounds to call opfs?.quarantine(), which can quarantine valid data on transient
I/O/runtime failures; change the handler so only known
compatibility/decoding/schema-mismatch errors trigger opfs?.quarantine().
Concretely: in the catch that currently logs "OPFS document unusable ..." and
calls opfs?.quarantine(), detect specific error types or markers (e.g.,
error.name === "DecodeError" || error.code === "INCOMPATIBLE_SCHEMA" || a
schema_version mismatch after reading metadata) and call opfs?.quarantine() only
for those; for all other errors (I/O, transient, runtime) emit a warn/error log
with error details and fall back to the default document without quarantining.
Ensure you reference the same error variable and call opfs?.quarantine() only
under the compatibility/validation checks.
In `@packages/grida-canvas-schema/grida.ts`:
- Around line 550-561: The function isSchemaCompatible currently treats a
malformed internal SCHEMA_VERSION the same as any bad input; change it so that
after calling parseVersion(SCHEMA_VERSION) you fail fast if current is null by
throwing a clear error (e.g., include SCHEMA_VERSION value) so developers see
the broken constant; keep the existing logic for when file
parseVersion(fileVersion) returns null (return false) and preserve the branch
logic tied to current.major === 0 and post-1.0 behavior in isSchemaCompatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78a31937-c1ab-4e8f-ad5e-ed1d4d50571d
📒 Files selected for processing (3)
editor/grida-canvas-hosted/playground/playground.tsxpackages/grida-canvas-io/format.tspackages/grida-canvas-schema/grida.ts
- Replaced hardcoded schema version string with dynamic reference to `grida.program.document.SCHEMA_VERSION` in the archive test file. This change enhances maintainability by ensuring the test reflects the current schema version automatically.
softmarshmallow
changed the title
1 participant
These changes enhance data integrity and user experience by safeguarding against data corruption and ensuring compatibility with schema updates.
Summary by CodeRabbit
Bug Fixes
New Features
Tests