mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: simplify safeWriteJson to always pretty-print
- Remove optional indent parameter from safeWriteJson - always use 2-space indent - Remove streaming approach for compact output (no longer needed) - Update McpHub.ts to remove explicit indent=2 parameters - Update tests to reflect the simplified behavior - Add test for un-compacting previously compacted JSON files
This commit is contained in:
Roo Code
parent
1b465be350
commit
b3615dc111
4 changed files with 55 additions and 120 deletions
|
|
@ -479,7 +479,7 @@ export class McpHub {
|
|||
)
|
||||
const fileExists = await fileExistsAtPath(mcpSettingsFilePath)
|
||||
if (!fileExists) {
|
||||
await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} }, 2)
|
||||
await safeWriteJson(mcpSettingsFilePath, { mcpServers: {} })
|
||||
}
|
||||
return mcpSettingsFilePath
|
||||
}
|
||||
|
|
@ -1568,7 +1568,7 @@ export class McpHub {
|
|||
}
|
||||
this.isProgrammaticUpdate = true
|
||||
try {
|
||||
await safeWriteJson(configPath, updatedConfig, 2)
|
||||
await safeWriteJson(configPath, updatedConfig)
|
||||
} finally {
|
||||
// Reset flag after watcher debounce period (non-blocking)
|
||||
this.flagResetTimer = setTimeout(() => {
|
||||
|
|
@ -1653,7 +1653,7 @@ export class McpHub {
|
|||
mcpServers: config.mcpServers,
|
||||
}
|
||||
|
||||
await safeWriteJson(configPath, updatedConfig, 2)
|
||||
await safeWriteJson(configPath, updatedConfig)
|
||||
|
||||
// Update server connections with the correct source
|
||||
await this.updateServerConnections(config.mcpServers, serverSource)
|
||||
|
|
@ -1804,7 +1804,7 @@ export class McpHub {
|
|||
}
|
||||
this.isProgrammaticUpdate = true
|
||||
try {
|
||||
await safeWriteJson(normalizedPath, config, 2)
|
||||
await safeWriteJson(normalizedPath, config)
|
||||
} finally {
|
||||
// Reset flag after watcher debounce period (non-blocking)
|
||||
this.flagResetTimer = setTimeout(() => {
|
||||
|
|
|
|||
|
|
@ -41,12 +41,12 @@ import { safeWriteJson } from "../../../utils/safeWriteJson"
|
|||
|
||||
// Mock safeWriteJson
|
||||
vi.mock("../../../utils/safeWriteJson", () => ({
|
||||
safeWriteJson: vi.fn(async (filePath, data, indent) => {
|
||||
safeWriteJson: vi.fn(async (filePath, data) => {
|
||||
// Instead of trying to write to the file system, just call fs.writeFile mock
|
||||
// This avoids the complex file locking and temp file operations
|
||||
const fs = await import("fs/promises")
|
||||
// Support indent parameter for formatting
|
||||
const jsonString = indent !== undefined ? JSON.stringify(data, null, indent) : JSON.stringify(data)
|
||||
// Always pretty-print with 2-space indentation (matching the real implementation)
|
||||
const jsonString = JSON.stringify(data, null, 2)
|
||||
return fs.writeFile(filePath, jsonString, "utf8")
|
||||
}),
|
||||
}))
|
||||
|
|
@ -914,7 +914,7 @@ describe("McpHub", () => {
|
|||
expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool")
|
||||
})
|
||||
|
||||
it("should preserve JSON pretty-print formatting when toggling tool always allow", async () => {
|
||||
it("should always write pretty-printed JSON when toggling tool always allow", async () => {
|
||||
const mockConfig = {
|
||||
mcpServers: {
|
||||
"test-server": {
|
||||
|
|
@ -947,17 +947,11 @@ describe("McpHub", () => {
|
|||
|
||||
await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true)
|
||||
|
||||
// Verify safeWriteJson was called with indent parameter
|
||||
// Verify safeWriteJson was called (it always pretty-prints now)
|
||||
const safeWriteJsonMock = vi.mocked(safeWriteJson)
|
||||
expect(safeWriteJsonMock).toHaveBeenCalled()
|
||||
|
||||
// Get the last call to safeWriteJson (most recent write)
|
||||
const lastCall = safeWriteJsonMock.mock.calls[safeWriteJsonMock.mock.calls.length - 1]
|
||||
|
||||
// Verify indent=2 was passed (third parameter)
|
||||
expect(lastCall[2]).toBe(2)
|
||||
|
||||
// Verify the written content would be pretty-printed
|
||||
// Verify the written content is pretty-printed
|
||||
const writeCalls = vi.mocked(fs.writeFile).mock.calls
|
||||
const lastWriteCall = writeCalls[writeCalls.length - 1]
|
||||
if (lastWriteCall) {
|
||||
|
|
|
|||
|
|
@ -478,10 +478,10 @@ describe("safeWriteJson", () => {
|
|||
consoleErrorSpy.mockRestore()
|
||||
})
|
||||
|
||||
// Tests for pretty-printing functionality
|
||||
test("should write pretty-printed JSON with numeric indent parameter", async () => {
|
||||
// Tests for pretty-printing functionality (always enabled)
|
||||
test("should always write pretty-printed JSON with 2-space indentation", async () => {
|
||||
const data = { mcpServers: { test: { command: "node", args: ["test.js"] } } }
|
||||
await safeWriteJson(currentTestFilePath, data, 2)
|
||||
await safeWriteJson(currentTestFilePath, data)
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
|
@ -495,38 +495,7 @@ describe("safeWriteJson", () => {
|
|||
expect(content).toMatch(/\s+"test":\s+\{/)
|
||||
})
|
||||
|
||||
test("should write pretty-printed JSON with string indent parameter", async () => {
|
||||
const data = { test: "value", nested: { key: "data" } }
|
||||
await safeWriteJson(currentTestFilePath, data, "\t")
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
||||
// Verify data is correct
|
||||
expect(parsed).toEqual(data)
|
||||
|
||||
// Verify it uses tab indentation
|
||||
expect(content).toContain("\t")
|
||||
expect(content).toContain("\n")
|
||||
})
|
||||
|
||||
test("should maintain compact format when no indent parameter is specified", async () => {
|
||||
const data = { test: "value", nested: { key: "data" } }
|
||||
await safeWriteJson(currentTestFilePath, data)
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
||||
// Verify data is correct
|
||||
expect(parsed).toEqual(data)
|
||||
|
||||
// Verify it's compact (no newlines except possibly at the very end)
|
||||
// Compact JSON from stream-json should be all on one line
|
||||
const lines = content.split("\n").filter((line) => line.trim().length > 0)
|
||||
expect(lines.length).toBe(1)
|
||||
})
|
||||
|
||||
test("should preserve formatting for complex nested structures with indent", async () => {
|
||||
test("should preserve formatting for complex nested structures", async () => {
|
||||
const data = {
|
||||
mcpServers: {
|
||||
server1: {
|
||||
|
|
@ -542,7 +511,7 @@ describe("safeWriteJson", () => {
|
|||
},
|
||||
},
|
||||
}
|
||||
await safeWriteJson(currentTestFilePath, data, 2)
|
||||
await safeWriteJson(currentTestFilePath, data)
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
|
@ -557,8 +526,8 @@ describe("safeWriteJson", () => {
|
|||
expect(content).toMatch(/"alwaysAllow":\s*\[/)
|
||||
})
|
||||
|
||||
test("should handle undefined data with indent parameter", async () => {
|
||||
await safeWriteJson(currentTestFilePath, undefined, 2)
|
||||
test("should handle undefined data by converting to null", async () => {
|
||||
await safeWriteJson(currentTestFilePath, undefined)
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
|
@ -567,4 +536,27 @@ describe("safeWriteJson", () => {
|
|||
expect(parsed).toBeNull()
|
||||
expect(content.trim()).toBe("null")
|
||||
})
|
||||
|
||||
test("should un-compact previously compacted JSON files on save", async () => {
|
||||
// Simulate a previously compacted JSON file
|
||||
const compactJson = '{"mcpServers":{"test":{"command":"node","args":["test.js"],"alwaysAllow":["tool1"]}}}'
|
||||
await fs.writeFile(currentTestFilePath, compactJson)
|
||||
|
||||
// Read the data and write it back using safeWriteJson
|
||||
const data = JSON.parse(compactJson)
|
||||
await safeWriteJson(currentTestFilePath, data)
|
||||
|
||||
const content = await fs.readFile(currentTestFilePath, "utf-8")
|
||||
const parsed = JSON.parse(content)
|
||||
|
||||
// Verify data is correct
|
||||
expect(parsed).toEqual(data)
|
||||
|
||||
// Verify the output is now pretty-printed (un-compacted)
|
||||
expect(content).toContain("\n")
|
||||
expect(content).toMatch(/\{\s+"mcpServers"/)
|
||||
// Should have multiple lines now
|
||||
const lines = content.split("\n").filter((line) => line.trim().length > 0)
|
||||
expect(lines.length).toBeGreaterThan(1)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -2,24 +2,22 @@ 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"
|
||||
|
||||
/**
|
||||
* Safely writes JSON data to a file.
|
||||
* Safely writes JSON data to a file with pretty-printing (2-space indentation).
|
||||
* - Creates parent directories if they don't exist
|
||||
* - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path.
|
||||
* - 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.
|
||||
* - Always outputs pretty-printed JSON for readability and manual editing.
|
||||
*
|
||||
* @param {string} filePath - The absolute path to the target file.
|
||||
* @param {any} data - The data to serialize to JSON and write.
|
||||
* @param {number | string} indent - Optional indentation for pretty-printing. Use a number for spaces or a string (e.g., '\t') for custom indentation.
|
||||
* @returns {Promise<void>}
|
||||
*/
|
||||
|
||||
async function safeWriteJson(filePath: string, data: any, indent?: number | string): Promise<void> {
|
||||
async function safeWriteJson(filePath: string, data: any): Promise<void> {
|
||||
const absoluteFilePath = path.resolve(filePath)
|
||||
let releaseLock = async () => {} // Initialized to a no-op
|
||||
|
||||
|
|
@ -76,7 +74,7 @@ async function safeWriteJson(filePath: string, data: any, indent?: number | stri
|
|||
`.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
|
||||
)
|
||||
|
||||
await _streamDataToFile(actualTempNewFilePath, data, indent)
|
||||
await _streamDataToFile(actualTempNewFilePath, data)
|
||||
|
||||
// Step 2: Check if the target file exists. If so, rename it to a backup path.
|
||||
try {
|
||||
|
|
@ -180,71 +178,22 @@ async function safeWriteJson(filePath: string, data: any, indent?: number | stri
|
|||
}
|
||||
|
||||
/**
|
||||
* 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 indent Optional indentation for pretty-printing.
|
||||
* Helper function to write JSON data to a file with pretty-printing.
|
||||
* @param targetPath The path to write to.
|
||||
* @param data The data to serialize as JSON.
|
||||
* @returns Promise<void>
|
||||
*/
|
||||
async function _streamDataToFile(targetPath: string, data: any, indent?: number | string): Promise<void> {
|
||||
// If indent is specified, use JSON.stringify for pretty-printing
|
||||
// This is suitable for small config files where readability is more important than memory efficiency
|
||||
if (indent !== undefined) {
|
||||
const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" })
|
||||
return new Promise<void>((resolve, reject) => {
|
||||
fileWriteStream.on("error", reject)
|
||||
fileWriteStream.on("finish", resolve)
|
||||
|
||||
// Handle undefined data by converting to null for valid JSON
|
||||
const jsonString = JSON.stringify(data === undefined ? null : data, null, indent)
|
||||
fileWriteStream.write(jsonString)
|
||||
fileWriteStream.end()
|
||||
})
|
||||
}
|
||||
|
||||
// For compact output, use streaming to avoid high memory usage for large JSON objects
|
||||
async function _streamDataToFile(targetPath: string, data: any): Promise<void> {
|
||||
const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" })
|
||||
const disassembler = Disassembler.disassembler()
|
||||
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)
|
||||
}
|
||||
}
|
||||
fileWriteStream.on("error", reject)
|
||||
fileWriteStream.on("finish", resolve)
|
||||
|
||||
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)
|
||||
|
||||
// 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()
|
||||
// Handle undefined data by converting to null for valid JSON
|
||||
// Always use 2-space indentation for readability
|
||||
const jsonString = JSON.stringify(data === undefined ? null : data, null, 2)
|
||||
fileWriteStream.write(jsonString)
|
||||
fileWriteStream.end()
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue