fix(web-defaults): safely parse JSON with strings containing slashes … - #247
Open
shobhitagnihotri69 wants to merge 1 commit into
Conversation
…and comments (fixes firecrawl#219)
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- In
src/utils/web-defaults.ts, lone\rline endings prevent//comments from terminating, so the scanner can discard the rest of a settings file and skip Claude defaults setup; treat\ras a line-comment terminator and add coverage for legacy line endings.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils/web-defaults.ts">
<violation number="1" location="src/utils/web-defaults.ts:52">
P2: When a settings file uses lone `\r` line endings, this scanner never terminates `//` comments and drops the rest of the JSON, causing Claude defaults setup to skip the file. Treat `\r` as a line-comment terminator as well as `\n`.</violation>
</file>
Heads up: you’re close to your flex budget. Increase your flex budget so reviews don’t pause.
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| const nextChar = i + 1 < content.length ? content[i + 1] : ''; | ||
|
|
||
| if (inLineComment) { | ||
| if (char === '\n') { |
Contributor
There was a problem hiding this comment.
P2: When a settings file uses lone \r line endings, this scanner never terminates // comments and drops the rest of the JSON, causing Claude defaults setup to skip the file. Treat \r as a line-comment terminator as well as \n.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/web-defaults.ts, line 52:
<comment>When a settings file uses lone `\r` line endings, this scanner never terminates `//` comments and drops the rest of the JSON, causing Claude defaults setup to skip the file. Treat `\r` as a line-comment terminator as well as `\n`.</comment>
<file context>
@@ -37,10 +37,60 @@ async function writeText(filePath: string, content: string): Promise<void> {
+ const nextChar = i + 1 < content.length ? content[i + 1] : '';
+
+ if (inLineComment) {
+ if (char === '\n') {
+ inLineComment = false;
+ result += char;
</file context>
Suggested change
| if (char === '\n') { | |
| if (char === '\n' || char === '\r') { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #219.
When
firecrawl setup defaultsreads existing configuration files (such as Claude desktop config or VS Code settings), regex-based comment stripping (\/\/.*$) was aggressively removing characters inside string literals (e.g.Read(//c/Users/...)). This produced malformed JSON syntax errors (SyntaxError: Expected double-quoted property name in JSON...) and silently skipped saving web default configurations.Changes
removeJsonCommentsinsrc/utils/web-defaults.tswith a character-by-character scanner that respects string literal boundaries and escape sequences (\",\\), ensuring//within strings is never removed.JSON.parse(existing)first before applying comment stripping.src/__tests__/utils/web-defaults.test.tsverifying that://paths are preserved and parse cleanly.//or/* */comments are parsed without corruption.Testing
pnpm testpassed 506/506 tests across 28 test suites (100% green).pnpm run buildsucceeded with zero TypeScript errors.Summary by cubic
Fixes
firecrawl setup defaultsfailing to save web defaults when existing config files contain//inside string literals (e.g.Read(//c/Users/...)). The old regex-based comment stripper removed slashes inside strings, causing JSON syntax errors and silently skipping the save. The new string-aware scanner respects string boundaries and escape sequences, and direct JSON parse is attempted first.Bug Fixes
JSON.parseon the raw content before applying comment stripping.//paths and files mixing comments with such strings.Written for commit 3de37f2. Summary will update on new commits.