Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/core/webview/webviewMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1249,7 +1249,7 @@ export const webviewMessageHandler = async (
const exists = await fileExistsAtPath(mcpPath)

if (!exists) {
await safeWriteJson(mcpPath, { mcpServers: {} })
await safeWriteJson(mcpPath, { mcpServers: {} }, { prettyPrint: true })
}

await openFile(mcpPath)
Expand Down
1 change: 1 addition & 0 deletions src/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@
"i18next": "^25.0.0",
"ignore": "^7.0.3",
"isbinaryfile": "^5.0.2",
"json-stream-stringify": "^3.1.6",
"jwt-decode": "^4.0.0",
"lodash.debounce": "^4.0.8",
"mammoth": "^1.9.1",
Expand Down
6 changes: 3 additions & 3 deletions src/services/mcp/McpHub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1531,7 +1531,7 @@ export class McpHub {
}
this.isProgrammaticUpdate = true
try {
await safeWriteJson(configPath, updatedConfig)
await safeWriteJson(configPath, updatedConfig, { prettyPrint: true })
} finally {
// Reset flag after watcher debounce period (non-blocking)
this.flagResetTimer = setTimeout(() => {
Expand Down Expand Up @@ -1616,7 +1616,7 @@ export class McpHub {
mcpServers: config.mcpServers,
}

await safeWriteJson(configPath, updatedConfig)
await safeWriteJson(configPath, updatedConfig, { prettyPrint: true })

// Update server connections with the correct source
await this.updateServerConnections(config.mcpServers, serverSource)
Expand Down Expand Up @@ -1767,7 +1767,7 @@ export class McpHub {
}
this.isProgrammaticUpdate = true
try {
await safeWriteJson(normalizedPath, config)
await safeWriteJson(normalizedPath, config, { prettyPrint: true })
} finally {
// Reset flag after watcher debounce period (non-blocking)
this.flagResetTimer = setTimeout(() => {
Expand Down
77 changes: 32 additions & 45 deletions src/utils/safeWriteJson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,20 @@ import * as fs from "fs/promises"
import * as fsSync from "fs"
import * as path from "path"
import * as lockfile from "proper-lockfile"
import Disassembler from "stream-json/Disassembler"
import Stringer from "stream-json/Stringer"
import { JsonStreamStringify } from "json-stream-stringify"

/**
* Options for safeWriteJson function
*/
export interface SafeWriteJsonOptions {
/**
* Whether to pretty-print the JSON output with indentation.
* When true, uses tab characters for indentation.
* When false or undefined, outputs compact JSON.
* @default false
*/
prettyPrint?: boolean
}

/**
* Safely writes JSON data to a file.
Expand All @@ -12,13 +24,15 @@ import Stringer from "stream-json/Stringer"
* - Writes to a temporary file first.
* - If the target file exists, it's backed up before being replaced.
* - Attempts to roll back and clean up in case of errors.
* - Supports pretty-printing with indentation while maintaining streaming efficiency.
*
* @param {string} filePath - The absolute path to the target file.
* @param {any} data - The data to serialize to JSON and write.
* @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting.
* @returns {Promise<void>}
*/

async function safeWriteJson(filePath: string, data: any): Promise<void> {
async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise<void> {
const absoluteFilePath = path.resolve(filePath)
let releaseLock = async () => {} // Initialized to a no-op

Expand Down Expand Up @@ -75,7 +89,7 @@ async function safeWriteJson(filePath: string, data: any): Promise<void> {
`.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
)

await _streamDataToFile(actualTempNewFilePath, data)
await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint)

// Step 2: Check if the target file exists. If so, rename it to a backup path.
try {
Expand Down Expand Up @@ -182,53 +196,26 @@ async function safeWriteJson(filePath: string, data: any): Promise<void> {
* Helper function to stream JSON data to a file.
* @param targetPath The path to write the stream to.
* @param data The data to stream.
* @param prettyPrint Whether to format the JSON with indentation.
* @returns Promise<void>
*/
async function _streamDataToFile(targetPath: string, data: any): Promise<void> {
async function _streamDataToFile(targetPath: string, data: any, prettyPrint = false): Promise<void> {
// Stream data to avoid high memory usage for large JSON objects.
const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" })
const disassembler = Disassembler.disassembler()
// Output will be compact JSON as standard Stringer is used.
const stringer = Stringer.stringer()

return new Promise<void>((resolve, reject) => {
let errorOccurred = false
const handleError = (_streamName: string) => (err: Error) => {
if (!errorOccurred) {
errorOccurred = true
if (!fileWriteStream.destroyed) {
fileWriteStream.destroy(err)
}
reject(err)
}
}

disassembler.on("error", handleError("Disassembler"))
stringer.on("error", handleError("Stringer"))
fileWriteStream.on("error", (err: Error) => {
if (!errorOccurred) {
errorOccurred = true
reject(err)
}
})

fileWriteStream.on("finish", () => {
if (!errorOccurred) {
resolve()
}
})

disassembler.pipe(stringer).pipe(fileWriteStream)
// JsonStreamStringify traverses the object and streams tokens directly
// The 'spaces' parameter adds indentation during streaming, not via a separate pass
const stringifyStream = new JsonStreamStringify(
data,
undefined, // replacer
prettyPrint ? "\t" : undefined, // spaces for indentation
)
Comment on lines +208 to +212
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original implementation had special handling for undefined values, converting them to null before serialization. This new implementation passes undefined directly to JsonStreamStringify, which will serialize it as the string "undefined" rather than null. This breaks the existing contract where undefined root values were written as valid JSON (null). Consider adding back the undefined check: const dataToWrite = data === undefined ? null : data before passing to JsonStreamStringify.

Fix it with Roo Code or mention @roomote and request a fix.


// stream-json's Disassembler might error if `data` is undefined.
// JSON.stringify(undefined) would produce the string "undefined" if it's the root value.
// Writing 'null' is a safer JSON representation for a root undefined value.
if (data === undefined) {
disassembler.write(null)
} else {
disassembler.write(data)
}
disassembler.end()
return new Promise<void>((resolve, reject) => {
stringifyStream.on("error", reject)
fileWriteStream.on("error", reject)
fileWriteStream.on("finish", resolve)
stringifyStream.pipe(fileWriteStream)
})
}

Expand Down
Loading