Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an on-page debugging panel, , which allows developers to inspect and troubleshoot embedded ThoughtSpot instances by interacting with an AI assistant and picking host page elements for style context. The panel is dynamically mounted when enableDebugAgent is enabled in the configuration. Feedback on these changes highlights several areas for improvement: correcting JSDoc tag ordering and versioning in src/types.ts per the style guide, making the SSE parser in toSseEvents more robust, clearing the hovered element state when the cursor leaves the viewport or enters the debug panel, using a temporary style block to prevent cursor flickering during element picking, and resolving a TypeScript type mismatch in the sendMessage event handler.
| * @default false | ||
| * @version SDK: 1.53.0 |
There was a problem hiding this comment.
The JSDoc tags for enableDebugAgent violate the Repository Style Guide rules on Tag Ordering (Rule 1). @version must be ordered before @default. Additionally, if this feature is backported or MCP'd to an earlier ThoughtSpot Cloud version, the @version JSDoc tag should reflect the actual release version where the feature is available, overriding the canonical SDK-to-ThoughtSpot version mapping style guide.
| * @default false | |
| * @version SDK: 1.53.0 | |
| * @version SDK: 1.53.0 | ThoughtSpot Cloud: 26.10.0.cl | |
| * @default false |
References
- Enforce tag ordering: @Version (3) must come before @default (9). (link)
- When a feature is backported or MCP'd to an earlier ThoughtSpot Cloud version, the @Version JSDoc tag should reflect the actual release version where the feature is available, overriding the canonical SDK-to-ThoughtSpot version mapping style guide.
| function toSseEvents(buffer: string): { events: Array<Record<string, 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)) as Record<string, unknown>); | ||
| return { events, rest }; | ||
| } |
There was a problem hiding this comment.
The current toSseEvents implementation splits the buffer by \n\n and assumes each block starts with data: . This can fail if there are leading newlines, keep-alive comments, or other SSE fields (like event: ) preceding the data: line in the same block. Additionally, if JSON.parse throws an error on a malformed line, the entire stream processing is aborted.
Splitting by \n and processing lines starting with data: is much more robust and standard for SSE parsing.
function toSseEvents(buffer: string): { events: Array<Record<string, unknown>>; rest: string } {
const lines = buffer.split('\n');
const rest = lines.pop() ?? '';
const events: Array<Record<string, unknown>> = [];
for (const line of lines) {
const trimmed = line.trim();
if (trimmed.startsWith('data: ')) {
try {
const data = JSON.parse(trimmed.slice(6));
events.push(data);
} catch (e) {
// Ignore malformed JSON lines gracefully
}
}
}
return { events, rest };
}
| const onMove = (e: MouseEvent) => { | ||
| const target = document.elementFromPoint(e.clientX, e.clientY); | ||
| if (!target || panelRef.current?.contains(target)) return; | ||
| setHovered(target); | ||
| }; |
There was a problem hiding this comment.
When the user moves the mouse over the debug panel itself or out of the viewport, the hovered state is not cleared, leaving the hover overlay stuck on the last hovered element. Clearing hovered when the target is null or inside the panel improves the user experience.
const onMove = (e: MouseEvent) => {
const target = document.elementFromPoint(e.clientX, e.clientY);
if (!target || panelRef.current?.contains(target)) {
setHovered(null);
return;
}
setHovered(target);
};
| const prevCursor = document.body.style.cursor; | ||
| document.body.style.cursor = 'crosshair'; | ||
| return () => { | ||
| document.removeEventListener('mousemove', onMove, true); | ||
| document.removeEventListener('click', onClick, true); | ||
| document.body.style.cursor = prevCursor; | ||
| }; |
There was a problem hiding this comment.
Setting document.body.style.cursor = 'crosshair' does not override the cursor style of elements that have their own cursor defined in CSS (such as links, buttons, or inputs). This causes the cursor to flicker between a crosshair and other pointers during element picking. Appending a temporary <style> block ensures a consistent crosshair cursor across all elements.
const style = document.createElement('style');
style.textContent = '* { cursor: crosshair !important; }';
document.head.appendChild(style);
return () => {
document.removeEventListener('mousemove', onMove, true);
document.removeEventListener('click', onClick, true);
style.remove();
};
| async function sendMessage(e: React.FormEvent) { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
The sendMessage function is typed to expect a React.FormEvent, but it is also called from onKeyDown with a React.KeyboardEvent. Under strict TypeScript compiler options, this will cause a type mismatch compilation error. Changing the parameter type to React.SyntheticEvent (and making it optional) resolves this cleanly.
| async function sendMessage(e: React.FormEvent) { | |
| e.preventDefault(); | |
| async function sendMessage(e?: React.SyntheticEvent) { | |
| if (e) e.preventDefault(); |
commit: |
…ser pin The prior commit added @types/react-dom and repinned @types/mixpanel-browser in package.json via pnpm, which never touches package-lock.json, so CI's lockfile-sync check failed. Regenerated with npm install --legacy-peer-deps. Co-Authored-By: Claude Sonnet 5 <[email protected]>
react-dom's top-level export never had createRoot (it moved to the react-dom/client subpath in React 18) and dropped render entirely in React 19, so mountDebugAgent's typeof-check fallback always failed with "ReactDOM.render is not a function" on any React 18+ peer. Now tries react-dom/client first, falling back to legacy react-dom render only for React < 18 peers where the client subpath doesn't exist. Added custom-typings/react-dom-client.d.ts since @types/react-dom@17 (this repo's devDependency, matching its react-dom@16 dep) ships no react-dom/client declarations. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Previously "Pick element" only ever saw the host page's own DOM — document.elementFromPoint cannot cross into the cross-origin ThoughtSpot iframe, so clicking on it just returned the <iframe> element itself. When a browser-extension debugging session (extensionSessionId) is connected, clicking inside the iframe now sends a chat message asking the agent to inspect that point inside the iframe's own frame via the extension relay's evaluate_script tool (which already has chrome.debugger access into cross-origin iframes through CDP's Target.setAutoAttach). Without a session, iframe clicks still fall back to picking the <iframe> element itself as before. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Picking an element used to snapshot its computed styles up front and paste them into the prompt, and the iframe case shipped the model a hand-written instruction to compose list_frames + evaluate_script itself. With an extension session connected, a pick now just sends a reference to the picked point — coordinates plus which frame — and lets the agent call the extension's new inspect_element_in_frame tool when it wants the element's styles. That tool goes through chrome.debugger, so the same path covers the host page and the cross-origin ThoughtSpot iframe. Without an extension session the previous host-only snapshot behaviour is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Clicking the embed while picking now asks the agent to run start_element_picker inside the iframe's own frame, so the user gets the same hover-highlight-then-click experience in there as on the host page. Coordinate-based picking could not do that: hover has to be driven from inside the frame, and a click mapped by coordinates resolves whatever happens to be at that point when the tool finally runs, not what was under the cursor. Host page picking is unchanged — it reads the DOM directly and needs no extension. Adds a "Connect extension" field so the session id can be pasted from the extension popup and kept in localStorage. Without it the component had no way to receive one at all: init() auto-mounts DebugAgent with no props, so the extensionSessionId prop was always undefined. The id also rotates whenever Chrome evicts the extension's service worker, which config alone would handle poorly. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The picker prompt told the model to find the frame by the iframe's src, but an embedded ThoughtSpot frame reports an empty url to list_frames. With nothing to match, the model omitted frameSessionId and the picker ran in the host page — picking ts-embed-container instead of anything inside the embed. Now it selects the frame by type and is told frameSessionId is required. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Picking inside the embed previously meant clicking "Pick element", then clicking the iframe, which sent a chat message asking the agent to locate the frame and start the picker. That was slow and unreliable — the model could target the host page, omit frameSessionId, or not re-run the picker after one completed, which is why picking appeared to die after the first use. There is now a separate "Pick in embed" button, shown once an extension session is connected. It calls list_frames and start_element_picker itself through POST /extension/tool-call, so the highlight arms as soon as the button is pressed, and hands the picked element to the agent for styling advice. "Pick element" keeps covering the host page. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Picking inside the embed immediately fired off "what is it and how would I style it?", which forced one canned question and ended the turn. Host-page picks never did that — they just attach the element and wait. Embed picks now behave the same: the element becomes a context chip and the developer asks whatever they want about it, across as many turns as they like. The preamble notes which side of the iframe boundary each picked element is on, since that decides whether ordinary CSS applies or the change has to go through customCSS. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
One extension session covers every tab, so the panel has to say which tab it is running in. It matched on origin alone and took the first hit, meaning two tabs of the same app were indistinguishable and the picker could arm in the wrong window. Now prefers an exact URL match, then a tab the developer has actually granted debugger access to, and says so plainly when this tab is not attached rather than failing further down inside list_frames. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
| nodes.push( | ||
| <a | ||
| key={`${keyPrefix}-${i}`} | ||
| href={link[2]} |
This file was a throwaway entry point used only to bundle the panel for browser screenshots while developing; it got picked up by an earlier commit. It is not referenced by the build, the tests or the published package, so it should not ship. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
No description provided.