fix: preserve null for optional booleans and strip empty JSON objects in refineContentFields#3783
fix: preserve null for optional booleans and strip empty JSON objects in refineContentFields#3783
Conversation
… in refineContentFields
Two issues with field refinement from D1/SQLite results:
1. Boolean fields: `Boolean(item[key])` is called unconditionally.
SQLite NULL → JS null → Boolean(null) === false. This injects
`false` for truly-absent optional boolean fields. Fix: delete the
key when the value is null/undefined instead of coercing.
2. JSON fields: optional JSON columns (e.g. `seo`) default to '{}'
in D1. Parsing produces an empty `{}` that pollutes the document
when the YAML/markdown source has no such key. Fix: after parsing,
if the result is an empty object (and the key is not `meta` which
is used as a catch-all bucket), delete it instead of assigning.
The `meta` field is exempted because `applyCollectionSchema` uses it
as a fallback bucket for unrecognized keys.
|
Someone is attempting to deploy a commit to the Nuxt Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe refineContentFields function now normalizes D1/SQLite query results into JS types more robustly: JSON-typed fields are parsed and, if the parsed value is an empty object and the key is not Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/runtime/internal/collection.ts`:
- Around line 10-12: The dynamic delete usage (delete item[key]) in the
normalization block that checks key !== 'meta' and parsed being an empty object
should be replaced by assigning item[key] = undefined (or 'NULL') and then
filtering those undefined/'NULL' entries out when returning the normalized
object; update the logic in the same function handling item, key, parsed (and
the similar occurrence around lines 18-20) so you mark fields to omit by setting
them to undefined rather than deleting, then at the end produce a new object
that excludes keys whose value is undefined or the sentinel 'NULL' to preserve
the “field omitted” behavior without using dynamic delete.
- Around line 17-23: The boolean coercion branch in the collection normalization
treats the sentinel string 'NULL' as truthy (using Boolean(item[key])), causing
optional boolean fields to be kept instead of omitted; update the logic in the
boolean handling for item[key] (the branch that checks fields[key] ===
'boolean') to first treat the exact string 'NULL' as empty (delete the key or
treat as null) before any Boolean(...) conversion, and apply the same sentinel
check to the later normalization block that handles null/undefined/'NULL' to
ensure 'NULL' never gets coerced to true; look for uses of fields, item and key
in the boolean branch and the subsequent normalization block and add the
sentinel check there.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccba6021-110e-401e-9fe1-e5d6b023a964
📒 Files selected for processing (1)
src/runtime/internal/collection.ts
| if (fields[key as string] === 'boolean' && item[key] !== 'undefined') { | ||
| item[key] = Boolean(item[key]) as never | ||
| if (item[key] == null) { | ||
| delete item[key] | ||
| } | ||
| else { | ||
| item[key] = Boolean(item[key]) as never | ||
| } |
There was a problem hiding this comment.
Handle 'NULL' sentinel before boolean coercion
At Line 22, Boolean(item[key]) will coerce 'NULL' to true. Since 'NULL' normalization happens later (Line 27+), this can leak wrong boolean values instead of omitting the optional field.
Treat 'NULL' as empty in the boolean branch before coercion.
Also applies to: 27-30
🧰 Tools
🪛 GitHub Check: ubuntu
[failure] 19-19:
Do not delete dynamically computed property keys
🤖 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 `@src/runtime/internal/collection.ts` around lines 17 - 23, The boolean
coercion branch in the collection normalization treats the sentinel string
'NULL' as truthy (using Boolean(item[key])), causing optional boolean fields to
be kept instead of omitted; update the logic in the boolean handling for
item[key] (the branch that checks fields[key] === 'boolean') to first treat the
exact string 'NULL' as empty (delete the key or treat as null) before any
Boolean(...) conversion, and apply the same sentinel check to the later
normalization block that handles null/undefined/'NULL' to ensure 'NULL' never
gets coerced to true; look for uses of fields, item and key in the boolean
branch and the subsequent normalization block and add the sentinel check there.
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)
src/runtime/internal/collection.ts (1)
4-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
T extends objectconstraint required to unblock CI —Reflect.deletePropertyrejects unconstrainedT
Reflect.deletePropertyis typed as(target: object, propertyKey: PropertyKey): boolean. BecauseTis unconstrained, TypeScript rejectsitem(of typeT) as the first argument, producing theTS2345errors at Lines 11 and 19 that are currently failing CI.The fix is a one-character change at the function signature — the constraint is also semantically accurate since
docis immediately spread into an object literal.🔧 Proposed fix
-export function refineContentFields<T>(sql: string, doc: T) { +export function refineContentFields<T extends object>(sql: string, doc: T) {🤖 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 `@src/runtime/internal/collection.ts` at line 4, The TypeScript error is caused by using Reflect.deleteProperty with an unconstrained generic T; update the function signature of refineContentFields to constrain T to object (i.e., change to refineContentFields<T extends object>(...)) so that doc/item are accepted by Reflect.deleteProperty; modify only the generic constraint on the refineContentFields function to resolve the TS2345 failures while leaving the function body unchanged.
♻️ Duplicate comments (1)
src/runtime/internal/collection.ts (1)
17-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
'NULL'sentinel is coerced totruein the boolean branch — still unresolved from prior review
'NULL'(the SQL NULL sentinel string) is truthy, soitem[key] == nullisfalse, andBoolean('NULL')evaluates totrue. The sentinel normalization loop (Lines 27–31) only runs after this branch, so optional boolean fields backed by a SQL NULL can be injected astrueinstead of being removed.Extend the null-guard at Line 18 to also cover the
'NULL'sentinel:🔧 Proposed fix
- if (item[key] == null) { + if (item[key] == null || item[key] === 'NULL') { Reflect.deleteProperty(item, key) }🤖 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 `@src/runtime/internal/collection.ts` around lines 17 - 24, In the boolean normalization branch (where fields[key as string] === 'boolean') extend the null-guard so SQL NULL sentinels are treated like null: instead of only checking item[key] == null, also check for the string 'NULL' and delete the property in that case; otherwise continue to coerce with Boolean(item[key]). Update the check around item[key] in that branch (use item[key] == null || item[key] === 'NULL') so optional boolean fields backed by the SQL 'NULL' sentinel are removed rather than coerced to true.
🤖 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 `@src/runtime/internal/collection.ts`:
- Line 4: The TypeScript error is caused by using Reflect.deleteProperty with an
unconstrained generic T; update the function signature of refineContentFields to
constrain T to object (i.e., change to refineContentFields<T extends
object>(...)) so that doc/item are accepted by Reflect.deleteProperty; modify
only the generic constraint on the refineContentFields function to resolve the
TS2345 failures while leaving the function body unchanged.
---
Duplicate comments:
In `@src/runtime/internal/collection.ts`:
- Around line 17-24: In the boolean normalization branch (where fields[key as
string] === 'boolean') extend the null-guard so SQL NULL sentinels are treated
like null: instead of only checking item[key] == null, also check for the string
'NULL' and delete the property in that case; otherwise continue to coerce with
Boolean(item[key]). Update the check around item[key] in that branch (use
item[key] == null || item[key] === 'NULL') so optional boolean fields backed by
the SQL 'NULL' sentinel are removed rather than coerced to true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1547fc42-3829-46ec-8168-f751e73edd36
📒 Files selected for processing (1)
src/runtime/internal/collection.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/internal/collection.ts (1)
17-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
'NULL'sentinel still coerced totruein the boolean branch (unresolved from prior review)The new null guard at line 18 correctly handles JS
null, but the string'NULL'(returned by D1/SQLite for absent columns) is neither== nullnor=== 'undefined', so it falls through toBoolean('NULL') === true. By the time the'NULL'-normalization loop (lines 27–31) runs, the key already holdstrueand is never cleaned up.🐛 Proposed fix
- if (item[key] == null) { + if (item[key] == null || item[key] === 'NULL') { Reflect.deleteProperty(item as object, key) }🤖 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 `@src/runtime/internal/collection.ts` around lines 17 - 24, The boolean branch in collection.ts (the block using fields, item, key and Reflect.deleteProperty) currently coerces the string sentinel 'NULL' to true; change the guard so that values equal to null, the string 'NULL', or the string 'undefined' are treated as absent and removed (use Reflect.deleteProperty(item as object, key)), otherwise coerce to Boolean as before (item[key] = Boolean(item[key]) as never). Ensure you update only that branch so the subsequent 'NULL'-normalization loop no longer has to handle already-coerced booleans.
🤖 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 `@src/runtime/internal/collection.ts`:
- Around line 8-15: The loop that parses JSON fields (references: fields, item,
key and the JSON.parse call) only guards against the literal string 'undefined'
so values equal to 'NULL' get passed to JSON.parse and throw; update the guard
to also exclude the string 'NULL' (i.e., treat item[key] === 'NULL' like
'undefined') and additionally wrap the JSON.parse call in a try/catch so
malformed JSON doesn't propagate a SyntaxError—on parse failure either log the
error and leave the original value or set the field to undefined/omit it
similarly to how 'undefined' is handled.
---
Duplicate comments:
In `@src/runtime/internal/collection.ts`:
- Around line 17-24: The boolean branch in collection.ts (the block using
fields, item, key and Reflect.deleteProperty) currently coerces the string
sentinel 'NULL' to true; change the guard so that values equal to null, the
string 'NULL', or the string 'undefined' are treated as absent and removed (use
Reflect.deleteProperty(item as object, key)), otherwise coerce to Boolean as
before (item[key] = Boolean(item[key]) as never). Ensure you update only that
branch so the subsequent 'NULL'-normalization loop no longer has to handle
already-coerced booleans.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94011a21-9022-4691-bbc3-58705fab08a1
📒 Files selected for processing (1)
src/runtime/internal/collection.ts
| if (fields[key as string] === 'json' && item[key] && item[key] !== 'undefined') { | ||
| item[key] = JSON.parse(item[key] as string) | ||
| const parsed = JSON.parse(item[key] as string) | ||
| if (key !== 'meta' && parsed && typeof parsed === 'object' && !Array.isArray(parsed) && Object.keys(parsed).length === 0) { | ||
| Reflect.deleteProperty(item as object, key) | ||
| } | ||
| else { | ||
| item[key] = parsed | ||
| } |
There was a problem hiding this comment.
'NULL' sentinel bypasses the JSON guard and throws SyntaxError
The guard at line 8 only excludes the string 'undefined'. The string 'NULL' is truthy and passes the check, so JSON.parse('NULL') is called — which throws a SyntaxError because uppercase NULL is not valid JSON. The 'NULL'→undefined normalization at lines 27–31 runs after this loop and cannot rescue the field.
Fix: extend the guard to exclude 'NULL', mirroring how 'undefined' is handled.
🐛 Proposed fix
- if (fields[key as string] === 'json' && item[key] && item[key] !== 'undefined') {
+ if (fields[key as string] === 'json' && item[key] && item[key] !== 'undefined' && item[key] !== 'NULL') {As a secondary concern, JSON.parse at line 9 is still unchecked — any malformed JSON row will throw an unhandled SyntaxError. Consider wrapping in a try/catch or at least logging the error.
🤖 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 `@src/runtime/internal/collection.ts` around lines 8 - 15, The loop that parses
JSON fields (references: fields, item, key and the JSON.parse call) only guards
against the literal string 'undefined' so values equal to 'NULL' get passed to
JSON.parse and throw; update the guard to also exclude the string 'NULL' (i.e.,
treat item[key] === 'NULL' like 'undefined') and additionally wrap the
JSON.parse call in a try/catch so malformed JSON doesn't propagate a
SyntaxError—on parse failure either log the error and leave the original value
or set the field to undefined/omit it similarly to how 'undefined' is handled.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/runtime/internal/collection.ts (2)
22-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
'NULL'sentinel is coerced totruein the boolean branch.At Line 22 the guard only filters the string
'undefined'. Foritem[key] === 'NULL',item[key] == nullis false (Line 23), so execution falls through toBoolean('NULL')→trueat Line 27. The optional boolean field then leaks astrueinstead of being omitted. The'NULL'normalization later (lines 32–36) cannot recover because the value is already a real boolean by then.🐛 Proposed fix
- if (fields[key as string] === 'boolean' && item[key] !== 'undefined') { - if (item[key] == null) { + if (fields[key as string] === 'boolean' && item[key] !== 'undefined' && item[key] !== 'NULL') { + if (item[key] == null) { Reflect.deleteProperty(item as object, key) } else { item[key] = Boolean(item[key]) as never } } + else if (fields[key as string] === 'boolean' && item[key] === 'NULL') { + Reflect.deleteProperty(item as object, key) + }Or, more simply, normalize
'NULL'(and'undefined') sentinels intonull/undefinedbefore the type-specific branches, eliminating the duplicated guards entirely.🤖 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 `@src/runtime/internal/collection.ts` around lines 22 - 29, The boolean branch mistakenly coerces the string sentinel 'NULL' to true; update the logic in collection handling (around the fields/item/key processing) to normalize sentinel strings ('NULL' and 'undefined') to null/undefined before any type-specific branches (or, alternatively, extend the boolean guard to treat 'NULL' as null), then delete the property via Reflect.deleteProperty when sentinel indicates omission so optional booleans are omitted instead of becoming true; adjust the code that currently uses Boolean(item[key]) and the later NULL normalization so sentinels are handled only once up-front.
13-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
'NULL'sentinel still bypasses the JSON guard and crashesJSON.parse.The guard at Line 13 only excludes
'undefined'. The string'NULL'is truthy and passes through, soJSON.parse('NULL')at Line 14 throwsSyntaxError(uppercaseNULLis not valid JSON). The'NULL'→undefinednormalization at lines 32–36 runs after this loop, so it cannot rescue the field. Additionally,JSON.parseis unguarded — any malformed row throws an unhandled exception.🐛 Proposed fix
- if (fields[key as string] === 'json' && item[key] && item[key] !== 'undefined') { - const parsed = JSON.parse(item[key] as string) - if (key !== 'meta' && parsed && typeof parsed === 'object' && !Array.isArray(parsed) && Object.keys(parsed).length === 0) { - Reflect.deleteProperty(item as object, key) - } - else { - item[key] = parsed - } - } + if (fields[key as string] === 'json' && item[key] && item[key] !== 'undefined' && item[key] !== 'NULL') { + let parsed: unknown + try { + parsed = JSON.parse(item[key] as string) + } + catch { + // Leave the original value in place if parsing fails. + continue + } + if (key !== 'meta' && parsed && typeof parsed === 'object' && !Array.isArray(parsed) && Object.keys(parsed as object).length === 0) { + Reflect.deleteProperty(item as object, key) + } + else { + item[key] = parsed as never + } + }🤖 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 `@src/runtime/internal/collection.ts` around lines 13 - 21, The loop currently calls JSON.parse on any truthy item[key] (e.g., "NULL") and can throw; update the block that handles fields[key] === 'json' to (1) explicitly treat sentinel strings like 'NULL' (and optionally other non-JSON sentinels) as absent so they don't reach JSON.parse, and (2) wrap JSON.parse(item[key] as string) in a try/catch; on parse failure or sentinel hit, normalize the field to undefined (or delete the property using the existing empty-object rule for non-'meta' keys), otherwise assign the parsed value and keep the existing logic that deletes empty objects for non-meta keys (use the same identifiers: fields, key, item, parsed, meta).
🤖 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.
Duplicate comments:
In `@src/runtime/internal/collection.ts`:
- Around line 22-29: The boolean branch mistakenly coerces the string sentinel
'NULL' to true; update the logic in collection handling (around the
fields/item/key processing) to normalize sentinel strings ('NULL' and
'undefined') to null/undefined before any type-specific branches (or,
alternatively, extend the boolean guard to treat 'NULL' as null), then delete
the property via Reflect.deleteProperty when sentinel indicates omission so
optional booleans are omitted instead of becoming true; adjust the code that
currently uses Boolean(item[key]) and the later NULL normalization so sentinels
are handled only once up-front.
- Around line 13-21: The loop currently calls JSON.parse on any truthy item[key]
(e.g., "NULL") and can throw; update the block that handles fields[key] ===
'json' to (1) explicitly treat sentinel strings like 'NULL' (and optionally
other non-JSON sentinels) as absent so they don't reach JSON.parse, and (2) wrap
JSON.parse(item[key] as string) in a try/catch; on parse failure or sentinel
hit, normalize the field to undefined (or delete the property using the existing
empty-object rule for non-'meta' keys), otherwise assign the parsed value and
keep the existing logic that deletes empty objects for non-meta keys (use the
same identifiers: fields, key, item, parsed, meta).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8214a9bf-89e1-452e-bbce-65beb408a0c7
📒 Files selected for processing (1)
src/runtime/internal/collection.ts
1 participant
Description
Two issues with
refineContentFieldswhen processing D1/SQLite query results:1. Optional boolean fields get incorrectly set to
falseBoolean(item[key])is called unconditionally for boolean-typed fields. When a field is absent in the source document, SQLite stores NULL. After query: NULL → JSnull→Boolean(null) === false.This means truly-absent optional boolean fields get injected as
falsein the returned document, causing:2. Empty JSON objects pollute the document
Optional JSON columns (e.g.
seo) default to'{}'in D1. After parsing, this produces an empty{}on the document. The YAML source has no such key — this causes false change detection in Studio and inflates the document.Changes
src/runtime/internal/collection.ts:delete item[key]instead of coercing to falsemeta), delete itThe
metafield is exempted becauseapplyCollectionSchemauses it as a catch-all bucket for unrecognized fields.Test Plan
field: falseseo: {}or similar empty objectsmetafield continues to work as a catch-all (not stripped even when empty)