fix: distinguish malformed JSON from partial arrays#19
Conversation
|
|
|
@rupayon123 is attempting to deploy a commit to the promplate Team on Vercel. A member of the Team first needs to authorize it. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves malformed and partial JSON parsing by introducing the isPartialJSON helper and ensuring only PartialJSON errors are caught and handled during partial parsing. It also addresses trailing whitespace issues in arrays by adding skipBlank() calls. However, a similar trailing whitespace issue remains in object parsing (e.g., {"a": 1, }), where blanks are not skipped after a comma, potentially leading to unexpected errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } catch (e) { | ||
| if (Allow.OBJ & allow) return obj; | ||
| else markPartialJSON("Expected '}' at end of object"); | ||
| if (isPartialJSON(e)) { |
There was a problem hiding this comment.
While the trailing whitespace issue has been successfully resolved for arrays (by adding skipBlank() after skipping the comma), the same issue still exists for objects.
If an object has trailing whitespace after a comma (e.g., {"a": 1, }), the spaces after the comma are not skipped before the loop condition jsonString[index] !== "}" is checked. This causes the parser to enter the loop and attempt to parse } as a key, leading to an unexpected error.
To fix this, we should also skip blanks after skipping a comma in parseObj:
skipBlank();
if (jsonString[index] === ",") {
index++; // skip comma
skipBlank();
}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughFix for Malformed JSON Handling (Arrays vs. Partial JSON)Changes in
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main parser fix: distinguishing malformed JSON from partial arrays. |
| Description check | ✅ Passed | The description matches the parser bug fix and added Vitest coverage described in the changes. |
| Linked Issues check | ✅ Passed | The parser now reports malformed numbers instead of swallowing them and preserves partial parsing semantics, satisfying #12. |
| Out of Scope Changes check | ✅ Passed | The changes stay within parser behavior and focused tests, with no clearly unrelated or out-of-scope edits. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.test.ts (1)
4-16: ⚡ Quick winAdd one object-path regression test.
parseObjnow has the same non-partial rethrow behavior as arrays, but this suite only exercises arrays. A small case like{"a": .05, "b": 2}would lock down that changed catch path too.Test addition
describe("malformed JSON handling", () => { it("throws for malformed numbers inside arrays", () => { expect(() => parse("[1, .05, 2]")).toThrow(MalformedJSON); }); + + it("throws for malformed numbers inside objects", () => { + expect(() => parse('{"a": .05, "b": 2}')).toThrow(MalformedJSON); + }); it("keeps parsing after an empty array with spaces", () => {🤖 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/index.test.ts` around lines 4 - 16, Add a new test case to the "malformed JSON handling" describe block that validates parseObj's non-partial rethrow behavior for malformed JSON inside objects. Create a test similar to the existing "throws for malformed numbers inside arrays" test, but use an object with a malformed number instead (such as {"a": .05, "b": 2}). The test should verify that parse() throws a MalformedJSON error when encountering malformed numbers inside objects, ensuring the same error handling path is exercised for both array and object parsing scenarios.
🤖 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.
Nitpick comments:
In `@src/index.test.ts`:
- Around line 4-16: Add a new test case to the "malformed JSON handling"
describe block that validates parseObj's non-partial rethrow behavior for
malformed JSON inside objects. Create a test similar to the existing "throws for
malformed numbers inside arrays" test, but use an object with a malformed number
instead (such as {"a": .05, "b": 2}). The test should verify that parse() throws
a MalformedJSON error when encountering malformed numbers inside objects,
ensuring the same error handling path is exercised for both array and object
parsing scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b238bc7-ea49-498d-8c5a-8120f38822e7
📒 Files selected for processing (2)
src/index.test.tssrc/index.ts
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new error-handling branches in
parseObjandparseArrboth callmarkPartialJSON(...)and then fall through tothrow e, but sincemarkPartialJSONalready throws, thethrow eis unreachable in theisPartialJSON(e)branch; consider restructuring these catch blocks to avoid redundant/unreachable throws and make the control flow clearer. - The
isPartialJSONhelper centralizes theinstanceofcheck nicely; you might want to use the same pattern anywhere else that currently distinguishesPartialJSONfrom other errors to keep error handling consistent across the parser.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new error-handling branches in `parseObj` and `parseArr` both call `markPartialJSON(...)` and then fall through to `throw e`, but since `markPartialJSON` already throws, the `throw e` is unreachable in the `isPartialJSON(e)` branch; consider restructuring these catch blocks to avoid redundant/unreachable throws and make the control flow clearer.
- The `isPartialJSON` helper centralizes the `instanceof` check nicely; you might want to use the same pattern anywhere else that currently distinguishes `PartialJSON` from other errors to keep error handling consistent across the parser.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I pushed the follow-up changes from the review: object malformed-number coverage, blank skipping after object commas, and a small cleanup to the partial-error catch flow. I couldn’t run the suite locally here, so CI is still the check. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.test.ts (1)
21-23: ⚡ Quick winConsider adding a parallel array test for consistency.
This test validates that trailing commas with whitespace are handled correctly in objects. For symmetry with the object/array malformed-number tests (lines 5–11), consider adding an equivalent array test:
it("keeps parsing after an array comma with spaces", () => { expect(parse('[1, ]')).toEqual([1]); });This would ensure consistent trailing-comma handling across both structures.
🤖 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/index.test.ts` around lines 21 - 23, Add a new test case to match the existing object trailing-comma test for consistency. Create a new it() test called "keeps parsing after an array comma with spaces" that invokes the parse() function with an array containing a trailing comma and whitespace (similar to the object test pattern), and verify it returns the expected array with one element. Place this new test case after the existing object test to maintain parallel structure across both data types.
🤖 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.
Nitpick comments:
In `@src/index.test.ts`:
- Around line 21-23: Add a new test case to match the existing object
trailing-comma test for consistency. Create a new it() test called "keeps
parsing after an array comma with spaces" that invokes the parse() function with
an array containing a trailing comma and whitespace (similar to the object test
pattern), and verify it returns the expected array with one element. Place this
new test case after the existing object test to maintain parallel structure
across both data types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d09b29f9-a039-40f2-91db-b150e95795f6
📒 Files selected for processing (2)
src/index.test.tssrc/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
|
Added the parallel array trailing-comma-with-spaces test from the review note: I couldn’t run the suite from here, so CI is still the check. |
|
Follow-up pushed in Local validation:
Note: |
1 participant
This fixes the two malformed-array cases from #12:
MalformedJSONinstead of being treated as partial arrays]now skip blanks before checking the closing bracketI also added focused Vitest coverage for both cases.
I could not run the test suite from this environment, so CI should be the check here.
Closes #12