Integrate SpotterViz in TS MCP Server#159
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the SpotterViz (Aurora liveboard agent) integration, adding four new tools (spotterviz_create_session, spotterviz_submit_query, spotterviz_get_updates, and spotterviz_save_liveboard) to the MCP server, along with corresponding client, service, and SSE stream processing implementations. It also extends the conversation storage server to support metadata GET and PATCH endpoints, and updates the ThoughtSpot client and service to handle BACH pinboard sessions. The feedback highlights a critical bug in the tool definitions where z.uuid() is incorrectly used instead of z.string().uuid(), which would cause a runtime crash. Additionally, several defensive programming improvements are recommended to prevent potential runtime TypeError crashes when parsing JSON payloads or handling unexpected server responses, as well as wrapping metadata retrieval in a try-catch block to prevent poll count failures from crashing the updates polling.
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.
| existing_liveboard_id: z | ||
| .uuid() | ||
| .optional() |
There was a problem hiding this comment.
| let parsed: Record<string, unknown>; | ||
| try { | ||
| parsed = JSON.parse(dataJson); | ||
| } catch (err) { | ||
| console.warn( | ||
| `Aurora SSE frame had unparseable data (event=${eventType ?? "unknown"}):`, | ||
| err, | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
If dataJson is parsed as a primitive (like null or a number), JSON.parse will succeed but parsed will not be an object. Accessing properties like parsed.event_type will then throw a runtime TypeError. Add a defensive check to ensure the parsed value is a non-null object.
let parsed: Record<string, unknown>;
try {
const val = JSON.parse(dataJson);
if (typeof val !== "object" || val === null) {
console.warn(
`Aurora SSE frame had invalid JSON payload (event=${eventType ?? "unknown"})`
);
return null;
}
parsed = val;
} catch (err) {
console.warn(
`Aurora SSE frame had unparseable data (event=${eventType ?? "unknown"}):`,
err,
);
return null;
}There was a problem hiding this comment.
Returning primitives is not a valid scenario
| const data = (await response.json()) as { | ||
| success?: boolean; | ||
| message?: string; | ||
| errors?: string[]; | ||
| jwt_token?: string; | ||
| session_id?: string; | ||
| liveboard_name?: string; | ||
| }; | ||
|
|
||
| if (!data.success || !data.session_id || !data.jwt_token) { | ||
| const errMsg = | ||
| data.errors?.join("; ") || | ||
| data.message || | ||
| "Aurora /init returned an unsuccessful response"; | ||
| throw new Error(`createAuroraSession failed: ${errMsg}`); | ||
| } |
There was a problem hiding this comment.
If the server returns an empty or unexpected JSON response (e.g., null), accessing data.success or data.errors will throw a runtime TypeError. Adding a defensive check to ensure data is a non-null object before accessing its properties makes this much more robust.
| const data = (await response.json()) as { | |
| success?: boolean; | |
| message?: string; | |
| errors?: string[]; | |
| jwt_token?: string; | |
| session_id?: string; | |
| liveboard_name?: string; | |
| }; | |
| if (!data.success || !data.session_id || !data.jwt_token) { | |
| const errMsg = | |
| data.errors?.join("; ") || | |
| data.message || | |
| "Aurora /init returned an unsuccessful response"; | |
| throw new Error(`createAuroraSession failed: ${errMsg}`); | |
| } | |
| const data = (await response.json()) as { | |
| success?: boolean; | |
| message?: string; | |
| errors?: string[]; | |
| jwt_token?: string; | |
| session_id?: string; | |
| liveboard_name?: string; | |
| } | null; | |
| if (!data || typeof data !== "object" || !data.success || !data.session_id || !data.jwt_token) { | |
| const errMsg = | |
| data?.errors?.join("; ") || | |
| data?.message || | |
| "Aurora /init returned an unsuccessful response"; | |
| throw new Error(`createAuroraSession failed: ${errMsg}`); | |
| } |
| const data = (await response.json()) as { | ||
| status?: { | ||
| statusCode?: string; | ||
| errorMessage?: string; | ||
| errorCode?: string; | ||
| }; | ||
| pinboardSession?: { | ||
| transactionId?: string; | ||
| generationNumber?: string | number; | ||
| }; | ||
| pinboardResponses?: Array<{ | ||
| loadPinboard?: { | ||
| status?: { statusCode?: string; errorMessage?: string }; | ||
| }; | ||
| status?: { statusCode?: string; errorMessage?: string }; | ||
| }>; | ||
| }; | ||
|
|
||
| const statusCode = data?.status?.statusCode; | ||
| if (statusCode && statusCode !== "OK") { | ||
| const detail = | ||
| data.status?.errorMessage || | ||
| data.pinboardResponses?.[0]?.loadPinboard?.status?.errorMessage || | ||
| data.pinboardResponses?.[0]?.status?.errorMessage || | ||
| JSON.stringify(data.status); | ||
| throw new Error( | ||
| `createBachPinboardSession returned non-OK status ${statusCode}: ${detail}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
If the server returns null or an unexpected JSON response, accessing data.status or other properties will throw a runtime TypeError. Adding a defensive check to ensure data is a non-null object before accessing its properties makes this much more robust.
const data = (await response.json()) as {
status?: {
statusCode?: string;
errorMessage?: string;
errorCode?: string;
};
pinboardSession?: {
transactionId?: string;
generationNumber?: string | number;
};
pinboardResponses?: Array<{
loadPinboard?: {
status?: { statusCode?: string; errorMessage?: string };
};
status?: { statusCode?: string; errorMessage?: string };
}>;
} | null;
if (!data || typeof data !== "object") {
throw new Error("createBachPinboardSession failed: invalid JSON response");
}
const statusCode = data.status?.statusCode;
if (statusCode && statusCode !== "OK") {
const detail =
data.status?.errorMessage ||
data.pinboardResponses?.[0]?.loadPinboard?.status?.errorMessage ||
data.pinboardResponses?.[0]?.status?.errorMessage ||
JSON.stringify(data.status);
throw new Error(
`createBachPinboardSession returned non-OK status ${statusCode}: ${detail}`,
);
}| const data = (await response.json()) as { | ||
| status?: { | ||
| statusCode?: string; | ||
| errorMessage?: string; | ||
| errorCode?: string; | ||
| }; | ||
| pinboardResponses?: Array<{ | ||
| savePinboard?: { | ||
| status?: { statusCode?: string; errorMessage?: string }; | ||
| }; | ||
| status?: { statusCode?: string; errorMessage?: string }; | ||
| }>; | ||
| }; | ||
|
|
||
| const statusCode = data?.status?.statusCode; | ||
| if (statusCode && statusCode !== "OK") { | ||
| const detail = | ||
| data.status?.errorMessage || | ||
| data.pinboardResponses?.[0]?.savePinboard?.status?.errorMessage || | ||
| data.pinboardResponses?.[0]?.status?.errorMessage || | ||
| JSON.stringify(data.status); | ||
| throw new Error( | ||
| `saveBachPinboard returned non-OK status ${statusCode}: ${detail}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
If the server returns null or an unexpected JSON response, accessing data.status or other properties will throw a runtime TypeError. Adding a defensive check to ensure data is a non-null object before accessing its properties makes this much more robust.
const data = (await response.json()) as {
status?: {
statusCode?: string;
errorMessage?: string;
errorCode?: string;
};
pinboardResponses?: Array<{
savePinboard?: {
status?: { statusCode?: string; errorMessage?: string };
};
status?: { statusCode?: string; errorMessage?: string };
}>;
} | null;
if (!data || typeof data !== "object") {
throw new Error("saveBachPinboard failed: invalid JSON response");
}
const statusCode = data.status?.statusCode;
if (statusCode && statusCode !== "OK") {
const detail =
data.status?.errorMessage ||
data.pinboardResponses?.[0]?.savePinboard?.status?.errorMessage ||
data.pinboardResponses?.[0]?.status?.errorMessage ||
JSON.stringify(data.status);
throw new Error(
`saveBachPinboard returned non-OK status ${statusCode}: ${detail}`,
);
}| private async readPollCount(spotterVizSessionId: string): Promise<number> { | ||
| const metadata = | ||
| await this.storage.getMetadata<AuroraSessionContext>(spotterVizSessionId); | ||
| return typeof metadata.pollCount === "number" ? metadata.pollCount : 0; | ||
| } |
There was a problem hiding this comment.
If getMetadata throws an error (e.g., because the metadata is not found or has expired), readPollCount will propagate the error and crash the entire getUpdates call. Wrapping this in a try-catch block and returning 0 as a fallback is much more robust and matches the error-handling pattern used in resetPollCount.
private async readPollCount(spotterVizSessionId: string): Promise<number> {
try {
const metadata =
await this.storage.getMetadata<AuroraSessionContext>(spotterVizSessionId);
return typeof metadata?.pollCount === "number" ? metadata.pollCount : 0;
} catch (err) {
console.warn(`Failed to read pollCount for ${spotterVizSessionId}:`, err);
return 0;
}
}There was a problem hiding this comment.
This can cause undefined behavior; this should fail the call as the metadata is needed for the flow to work.
Design document: https://docs.google.com/document/d/19pliZtODaMldv7ceYeQWxMyllZS-d5-gwvvbd7bcxps/edit?usp=sharing