SCAL-322108: Enable chat history support for MCP server#172
SCAL-322108: Enable chat history support for MCP server#172mouryabalabhadra wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the showSpotterPastConversations tenant flag and refactors how data source discovery (auto mode) is handled when creating agent conversations. Specifically, it updates the MCP server, ThoughtSpot client, and ThoughtSpot service to pass these flags downstream, validates that auto mode is enabled when no explicit data source is provided, and adds comprehensive integration and unit tests. Feedback suggests simplifying the redundant string conversion of showSpotterPastConversations to a direct boolean cast.
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.
| ) { | ||
| (client as any).createAgentConversationWithAutoMode = async ({ | ||
| dataSourceId, | ||
| showSpotterPastConversations, |
There was a problem hiding this comment.
Prefer naming this flag saveChatEnabled in all function headers instead of showSpotterPastConversations
There was a problem hiding this comment.
Done in commit Rename showSpotterPastConversations to saveChatEnabled in function headers. Renamed showSpotterPastConversations → saveChatEnabled across all function headers in thoughtspot-client.ts, thoughtspot-service.ts, types.ts, and mcp-server.ts. The raw ThoughtSpot API response field (info.configInfo?.showSpotterPastConversations) is preserved when mapping to the internal saveChatEnabled property.
| const allowlist = rawAllowlist | ||
| .split(",") | ||
| .map((id) => id.trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
Why do we need to filter? Shouldn't all clusters in the allow list be "true"?
There was a problem hiding this comment.
Just eliminates any accidental blank strings but we can remove it
There was a problem hiding this comment.
Removed .filter(Boolean) in the same commit. Note that the if (!clusterId) guard ensures the cluster ID is always a non-empty truthy value before the allowlist check, so empty strings in the allowlist can never produce a false positive match.
|
@copilot review the pr |
…aders; remove filter(Boolean) from allowlist
Reviewed. The changes look good overall — the gating logic for |
No description provided.