Skip to content

changes - #679

Open
ruchI9897 wants to merge 4 commits into
mainfrom
embedX
Open

ruchI9897 wants to merge 4 commits into
mainfrom
embedX

Conversation

@ruchI9897

Copy link
Copy Markdown
Contributor

No description provided.

@ruchI9897
ruchI9897 requested a review from a team as a code owner September 20, 2026 12:27

@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 a local debug-agent widget (local/agent-widget.ts) and integrates it into the SDK initialization flow via a new enableDebugAgent configuration option. When enabled, the SDK dispatches a ts-debug-agent-enabled event, allowing the host page to dynamically mount the widget. Feedback on these changes highlights several critical improvements: registering the event listener before calling init() to prevent missing the synchronous event, renaming SESSION_STORAGE_KEY to LOCAL_STORAGE_KEY to match its actual usage with localStorage, wrapping SSE JSON parsing in a try-catch block to avoid stream crashes, and adding robust error handling and concurrency guards to the message submission flow.

Comment thread local/index.ts
Comment on lines 3 to 16
init({
thoughtSpotHost: 'https://embed-1-do-not-delete.thoughtspotstaging.cloud',
authType: AuthType.None,
enableDebugAgent: true,
});

// The SDK only signals the flag via this event — the host page decides what
// to mount. Here, embed-ai's local test widget (see agent-widget.ts).
// NOTE: this only fires once dist/tsembed.es.js is rebuilt with the
// enableDebugAgent change (src/embed/base.ts) — `npm run build` currently
// fails in this repo for unrelated reasons (see prior conversation).
window.addEventListener('ts-debug-agent-enabled', () => {
import('./agent-widget').then(({ mountAgentWidget }) => mountAgentWidget());
});

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.

high

The ts-debug-agent-enabled event is dispatched synchronously inside the init() function. Because the event listener is registered after init() is called, the event will have already been dispatched and missed by the listener. Register the event listener before calling init() to ensure it is caught correctly.

Suggested change
init({
thoughtSpotHost: 'https://embed-1-do-not-delete.thoughtspotstaging.cloud',
authType: AuthType.None,
enableDebugAgent: true,
});
// The SDK only signals the flag via this event — the host page decides what
// to mount. Here, embed-ai's local test widget (see agent-widget.ts).
// NOTE: this only fires once dist/tsembed.es.js is rebuilt with the
// enableDebugAgent change (src/embed/base.ts) — `npm run build` currently
// fails in this repo for unrelated reasons (see prior conversation).
window.addEventListener('ts-debug-agent-enabled', () => {
import('./agent-widget').then(({ mountAgentWidget }) => mountAgentWidget());
});
// The SDK only signals the flag via this event — the host page decides what
// to mount. Here, embed-ai's local test widget (see agent-widget.ts).
// NOTE: this only fires once dist/tsembed.es.js is rebuilt with the
// enableDebugAgent change (src/embed/base.ts) — `npm run build` currently
// fails in this repo for unrelated reasons (see prior conversation).
window.addEventListener('ts-debug-agent-enabled', () => {
import('./agent-widget').then(({ mountAgentWidget }) => mountAgentWidget());
});
init({
thoughtSpotHost: 'https://embed-1-do-not-delete.thoughtspotstaging.cloud',
authType: AuthType.None,
enableDebugAgent: true,
});

Comment thread local/agent-widget.ts Outdated
Comment on lines +15 to +31
const SESSION_STORAGE_KEY = 'spottercode-extension-session-id';

function getStoredSessionId(): string | null {
try {
return localStorage.getItem(SESSION_STORAGE_KEY);
} catch {
return null;
}
}

function setStoredSessionId(id: string) {
try {
localStorage.setItem(SESSION_STORAGE_KEY, id);
} catch {
// ignore — worst case the user re-pastes it next reload
}
}

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

The constant is named SESSION_STORAGE_KEY but it is used with localStorage. This naming mismatch is confusing and can lead to maintainability issues. Rename the constant to LOCAL_STORAGE_KEY to accurately reflect its usage.

Suggested change
const SESSION_STORAGE_KEY = 'spottercode-extension-session-id';
function getStoredSessionId(): string | null {
try {
return localStorage.getItem(SESSION_STORAGE_KEY);
} catch {
return null;
}
}
function setStoredSessionId(id: string) {
try {
localStorage.setItem(SESSION_STORAGE_KEY, id);
} catch {
// ignore — worst case the user re-pastes it next reload
}
}
const LOCAL_STORAGE_KEY = 'spottercode-extension-session-id';
function getStoredSessionId(): string | null {
try {
return localStorage.getItem(LOCAL_STORAGE_KEY);
} catch {
return null;
}
}
function setStoredSessionId(id: string) {
try {
localStorage.setItem(LOCAL_STORAGE_KEY, id);
} catch {
// ignore — worst case the user re-pastes it next reload
}
}

Comment thread local/agent-widget.ts Outdated
Comment on lines +33 to +40
function toSseEvents(buffer: string): { events: unknown[]; rest: string } {
const parts = buffer.split('\n\n');
const rest = parts.pop() ?? '';
const events = parts
.filter((p) => p.startsWith('data: '))
.map((p) => JSON.parse(p.slice(6)));
return { events, rest };
}

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 SSE event data is not valid JSON, JSON.parse will throw an error and crash the entire stream reading loop. Wrap JSON.parse in a try-catch block to handle parsing errors gracefully and ensure robustness.

function toSseEvents(buffer: string): { events: unknown[]; rest: string } {
	const parts = buffer.split('\n\n');
	const rest = parts.pop() ?? '';
	const events: unknown[] = [];
	for (const p of parts) {
		if (p.startsWith('data: ')) {
			try {
				events.push(JSON.parse(p.slice(6)));
			} catch (err) {
				console.error('Failed to parse SSE event:', err);
			}
		}
	}
	return { events, rest };
}

Comment thread local/agent-widget.ts Outdated
Comment on lines +143 to +208
const history: { role: 'user' | 'assistant'; content: string }[] = [];

function addBubble(role: 'user' | 'assistant', text: string) {
const el = document.createElement('div');
el.className = `msg ${role}`;
el.textContent = text;
messagesEl.appendChild(el);
messagesEl.scrollTop = messagesEl.scrollHeight;
return el;
}

function addToolLine(toolName: string) {
const el = document.createElement('div');
el.className = 'tool';
el.textContent = `🔧 ${toolName}`;
messagesEl.appendChild(el);
messagesEl.scrollTop = messagesEl.scrollHeight;
}

form.addEventListener('submit', async (e) => {
e.preventDefault();
const text = input.value.trim();
if (!text) return;
input.value = '';
addBubble('user', text);
history.push({ role: 'user', content: text });

let assistantEl: HTMLDivElement | null = null;
let assistantText = '';

try {
const response = await fetch(`${AGENT_API_ORIGIN}/agent/embed-assistant`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
agentType: 'ask-docs',
messages: history,
extensionSessionId: sessionInput.value.trim() || undefined,
}),
});
const reader = response.body!.getReader();
const decoder = new TextDecoder();
let buffer = '';

while (true) {
const { done, value } = await reader.read();
if (done) break;
buffer += decoder.decode(value, { stream: true });
const { events, rest } = toSseEvents(buffer);
buffer = rest;
for (const event of events as Array<Record<string, unknown>>) {
if (event.type === 'text') {
if (!assistantEl) assistantEl = addBubble('assistant', '');
assistantText += event.content as string;
assistantEl.textContent = assistantText;
messagesEl.scrollTop = messagesEl.scrollHeight;
} else if (event.type === 'tool-start') {
addToolLine(event.toolName as string);
}
}
}
if (assistantText) history.push({ role: 'assistant', content: assistantText });
} catch (err) {
addBubble('assistant', `Request failed: ${(err as Error).message}`);
}
});

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

This block has two issues: First, response.body can be null, and using the non-null assertion operator response.body! is unsafe and can cause a runtime TypeError. Additionally, there is no check on response.ok before reading the stream. Second, there is no guard against concurrent submissions, which can lead to race conditions and garbled chat history if the user submits multiple messages while a request is active. We should introduce an isSending flag to disable/ignore submissions during an active request, check response.ok, and safely check response.body.

	const history: { role: 'user' | 'assistant'; content: string }[] = [];
	let isSending = false;

	function addBubble(role: 'user' | 'assistant', text: string) {
		const el = document.createElement('div');
		el.className = 'msg ' + role;
		el.textContent = text;
		messagesEl.appendChild(el);
		messagesEl.scrollTop = messagesEl.scrollHeight;
		return el;
	}

	function addToolLine(toolName: string) {
		const el = document.createElement('div');
		el.className = 'tool';
		el.textContent = '🔧 ' + toolName;
		messagesEl.appendChild(el);
		messagesEl.scrollTop = messagesEl.scrollHeight;
	}

	form.addEventListener('submit', async (e) => {
		e.preventDefault();
		if (isSending) return;
		const text = input.value.trim();
		if (!text) return;
		isSending = true;
		input.value = '';
		addBubble('user', text);
		history.push({ role: 'user', content: text });

		let assistantEl: HTMLDivElement | null = null;
		let assistantText = '';

		try {
			const response = await fetch(AGENT_API_ORIGIN + '/agent/embed-assistant', {
				method: 'POST',
				headers: { 'Content-Type': 'application/json' },
				body: JSON.stringify({
					agentType: 'ask-docs',
					messages: history,
					extensionSessionId: sessionInput.value.trim() || undefined,
				}),
			});
			if (!response.ok) {
				throw new Error('HTTP error! status: ' + response.status);
			}
			if (!response.body) {
				throw new Error('Response body is null');
			}
			const reader = response.body.getReader();
			const decoder = new TextDecoder();
			let buffer = '';

			while (true) {
				const { done, value } = await reader.read();
				if (done) break;
				buffer += decoder.decode(value, { stream: true });
				const { events, rest } = toSseEvents(buffer);
				buffer = rest;
				for (const event of events as Array<Record<string, unknown>>) {
					if (event.type === 'text') {
						if (!assistantEl) assistantEl = addBubble('assistant', '');
						assistantText += event.content as string;
						assistantEl.textContent = assistantText;
						messagesEl.scrollTop = messagesEl.scrollHeight;
					} else if (event.type === 'tool-start') {
						addToolLine(event.toolName as string);
					}
				}
			}
			if (assistantText) history.push({ role: 'assistant', content: assistantText });
		} catch (err) {
			addBubble('assistant', 'Request failed: ' + (err as Error).message);
		} finally {
			isSending = false;
		}
	});

@pkg-pr-new

pkg-pr-new Bot commented Sep 20, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@thoughtspot/visual-embed-sdk@679

commit: f01a56b

Ships the on-screen debug agent as an SDK-exported React component
(<DebugAgent />) instead of host-app-side scaffolding, so it renders
automatically when init() sets enableDebugAgent: true.
DebugAgent was exported from src/react/index.tsx but the package's
"./react" export map points at all-types-export.ts, which re-exports
a curated list from index.tsx — DebugAgent was missing from that list,
so `import { DebugAgent } from '@thoughtspot/visual-embed-sdk/react'`
failed with "does not provide an export named 'DebugAgent'".
DebugAgent: the messages container was a flex child with
overflow-y:auto but no min-height:0, so it grew past its flex-basis
instead of scrolling — content spilled under the header and past the
panel's bottom edge. Also adds markdown-ish rendering (bold/code/
bullets), a close/reopen control, and a larger default size.

package.json: @types/mixpanel-browser's ^2.35.6 range resolved to
2.66.0, a deprecated stub release with zero type declarations,
breaking every local build (tsc/rollup) with "Cannot find type
definition file for 'mixpanel-browser'". Pinned to 2.60.0, the last
real release before the stub.
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