Skip to content

fix(mcp): report a JSON syntax error when saving the MCP config editor - #470

Open
pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/466-mcp-config-invalid-json
Open

pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/466-mcp-config-invalid-json

Conversation

@pjdoland

Copy link
Copy Markdown
Collaborator

Summary

Saving malformed JSON in the MCP config editor dropped the edit with no feedback anywhere in the UI. The only trace was a SyntaxError in the devtools console, and because the save handler runs when the document goes clean, the editor tab looked saved while the server still held the previous config.

Solution

MCPConfigEditor._onSave read the document with this._docWidget.context.model.toJSON() one line above its try. That call is JSON.parse(this.sharedModel.getSource() || 'null') in @jupyterlab/docregistry, so a syntax error in the user's edit threw before the error handling was in scope. The existing catch was written for server-side rejections and never saw a client-side parse failure.

The save now lives in saveMCPConfig, which takes the document read as a thunk. That is the load-bearing decision: the parse cannot end up outside the guarded path again, because the function that guards it is the one that calls it. It returns a description rather than throwing, so nothing rejects out of a signal handler that has nowhere to put it.

describeMCPConfigSaveError chooses the wording. A parse failure says the file is not valid JSON and keeps the parser's position; a server rejection passes through with its own message. The message is deliberately terse because JupyterLab truncates a notification at 140 characters, and the part that would be cut is the trailing (line 64 column 10) that locates the typo. Measured on a realistic eight-server config, the old phrasing produced 139 characters against that limit; the current one produces 127.

Two smaller corrections came out of review:

  • The capabilities refresh after a successful save was a floating promise that rejected unobserved if the server was restarting after the MCP reconcile. It is caught now, and deliberately not reported as a save failure, because the save did succeed.
  • String(reason) on a plain object rendered Failed to save MCP config: [object Object] at the user. That case now says the save failed for an unknown reason.

Testing

  • tests/ts/mcp-config-save.test.ts, 10 tests. Four pin saveMCPConfig itself: a parse failure must not reach the server or refresh capabilities, a server rejection must not refresh capabilities, a success must post the parsed config and refresh, and a failed refresh must not turn a successful save into a failure. The rest cover the wording, including that a SyntaxError and an Error carrying identical text are described differently.
  • The regression is pinned, and I checked that by mutation rather than assuming: moving the read back outside the try fails reports a parse failure without reaching the server, and restoring it passes. An earlier version of this branch tested only the message formatter, and that version survived the same mutation with all tests green.
  • pytest 2181 passed, jlpm tsc --noEmit clean, jlpm lint:check clean, jlpm jest 543 passed across 46 suites.
  • Verified in a running JupyterLab against this branch's build. Deleting a closing brace and saving raises Failed to save MCP config: not valid JSON. Expected ',' or '}' after property value in JSON at position 294 (line 10 column 1) (126 characters, inside the truncation limit). Restoring the brace and saving succeeds with no notification.

Risks / follow-ups

The document still goes clean on a failed save, which is JupyterLab's save lifecycle rather than something this handler controls; the notification is what tells the user the config did not persist. Holding the document dirty on failure would give the tab an unsaved marker and a close confirmation, which is worth considering separately.

MCPConfigEditor.open() is also called without observing its promise, so a failure in getMCPConfigFile or claimMCPTempFileName opens nothing and says nothing. Same class of problem, different trigger, left alone here to keep this change to one issue.

Closes #466

The save handler read the document with `model.toJSON()` before entering its
try block. That call is `JSON.parse` on the editor's text, so a typo in the
user's edit threw straight past the error handling: no notification, nothing
in the UI at all, only a message in the devtools console. The handler runs
when the document goes clean, so the tab looked saved while the server still
held the previous config.

Move the save into `saveMCPConfig`, which takes the read as a thunk so it
cannot end up outside the guarded path again, and returns a description
instead of throwing out of a signal handler with nowhere to put it. A parse
failure names itself as invalid JSON and keeps the parser's position so the
user can find the typo; a server rejection passes through with its own
message. The capabilities refresh that follows a successful save no longer
rejects unobserved, and is not reported as a save failure, because the save
did succeed.

Closes plmbr#466
@pjdoland pjdoland added the bug Something isn't working label Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP config editor silently discards edits when the JSON is malformed

1 participant