Skip to content

Integrate SpotterViz in TS MCP Server#159

Open
RohitEdathilTSP wants to merge 3 commits into
mainfrom
spotterviz-integration
Open

Integrate SpotterViz in TS MCP Server#159
RohitEdathilTSP wants to merge 3 commits into
mainfrom
spotterviz-integration

Conversation

@RohitEdathilTSP

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +268 to +270
existing_liveboard_id: z
.uuid()
.optional()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

In Zod, z.uuid() is not a valid function. To validate a UUID string, you should use z.string().uuid() instead. Using z.uuid() will cause a runtime crash on startup.

		existing_liveboard_id: z
			.string()
			.uuid()
			.optional()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment on lines +170 to +179
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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;
	}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Returning primitives is not a valid scenario

Comment on lines +71 to +86
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}`);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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}`);
}

Comment on lines +307 to +335
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}`,
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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}`,
			);
		}

Comment on lines +386 to +410
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}`,
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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}`,
			);
		}

Comment on lines +325 to +329
private async readPollCount(spotterVizSessionId: string): Promise<number> {
const metadata =
await this.storage.getMetadata<AuroraSessionContext>(spotterVizSessionId);
return typeof metadata.pollCount === "number" ? metadata.pollCount : 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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;
		}
	}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This can cause undefined behavior; this should fail the call as the metadata is needed for the flow to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant