Feature/ai guided onboarding 64 - #73
Open
KolaSailaja wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
accessibility
bug
🛠️ PR Needs UpdatesHey @KolaSailaja! 👋 A few things need fixing before a mentor can review this PR. Warning
How to fix:
Once fixed, the workflow re-runs automatically and pings the right mentor. 🤖 TabTwin Automation · Updates automatically on edits |
📝 WalkthroughWalkthroughAdds an optional AI onboarding flow for guests, including page analysis, Anthropic-generated guidance with fallbacks, WebSocket delivery, temporary highlights, session state integration, onboarding UI, and validation tests. ChangesAI-Guided Onboarding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostExtension
participant ActiveTab
participant AnthropicAPI
participant SignalingServer
participant GuestWebApp
HostExtension->>ActiveTab: Request onboarding:analyze
ActiveTab-->>HostExtension: Return page structure summary
HostExtension->>AnthropicAPI: Generate guidance from summary
AnthropicAPI-->>HostExtension: Return onboarding guidance
HostExtension->>SignalingServer: Send onboarding:guidance
SignalingServer->>GuestWebApp: Forward guidance
HostExtension->>ActiveTab: Send onboarding:highlight
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
19 issues found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="extension/onboarding/onboardingService.js">
<violation number="1" location="extension/onboarding/onboardingService.js:9">
P2: The `fetch` call to the Anthropic API has no request timeout, so a stalled HTTPS connection (e.g., flaky network, slow API response) can hang for an extended period before throwing. Because `triggerOnboardingForGuest` in `serviceWorker.js` simply awaits `generateOnboardingGuidance` with no external timeout, this can leave a guest stuck waiting for onboarding guidance that never arrives. The function already falls back to `createFallbackOnboarding` on failure, so adding an `AbortController` with a reasonable timeout (e.g., 10 seconds) would keep the flow resilient without adding new error-handling logic.</violation>
</file>
<file name="extension/content/annotationOverlay.js">
<violation number="1" location="extension/content/annotationOverlay.js:79">
P2: `highlightRegions` applies a full-page dimming shadow (`boxShadow: '0 0 0 9999px rgba(15, 23, 42, 0.12)'`) to every matched region individually. When multiple regions are highlighted, the shadows stack and make the page progressively darker than intended while forcing repeated expensive full-page repaints. Consider using one shared backdrop overlay for the page dim and placing only the per-region outline highlights on top of it.</violation>
<violation number="2" location="extension/content/annotationOverlay.js:83">
P1: The annotation notes and onboarding highlight boxes are both appended to the same `layer` container. `addAnnotation()` uses `layer.children.length` to calculate the vertical stacking position, assuming every child is an annotation note. Since `highlightRegions()` now appends temporary highlight boxes to that same container, the child count is inflated while those boxes exist, and any annotation added during the highlight TTL gets positioned too low (or off-screen) because the child count includes highlight overlays.
To fix this, either keep onboarding highlights in a separate sub-container inside `layer`, or maintain annotation-specific positioning logic (e.g. count only annotation notes rather than all children).</violation>
</file>
<file name="extension/manifest.json">
<violation number="1" location="extension/manifest.json:29">
P2: The new `onboarding/pageAnalyzer.js` script is statically injected via `content_scripts` with `matches: ["<all_urls>"]`, so it loads in every tab even when AI onboarding is disabled by default. Because this is an opt-in host-controlled feature, consider injecting it dynamically with `chrome.scripting.executeScript` or `chrome.scripting.registerContentScripts` only when `enableAiOnboarding` is active, rather than unconditionally listing it in the manifest. This avoids unnecessary script load and listener registration in tabs where the feature will never be used.</violation>
</file>
<file name="webapp/src/components/PlaybackControls.jsx">
<violation number="1" location="webapp/src/components/PlaybackControls.jsx:28">
P2: The playback position slider (`type="range"`) and the playback speed `<select>` are missing accessible labels. Screen reader users won't be able to determine the purpose of these controls because neither has a `<label>`, `aria-label`, or `aria-labelledby`. Since this PR explicitly highlights accessibility improvements, newly introduced interactive controls should include accessible names.</violation>
</file>
<file name="webapp/src/recording/SessionRecorder.js">
<violation number="1" location="webapp/src/recording/SessionRecorder.js:115">
P2: Event IDs generated by `buildEvent` can collide when multiple events of the same type arrive from the same participant within a single millisecond. The current pattern (`${eventType}-${timestamp}-${participantId}`) uses only millisecond precision, which is insufficient for high-frequency streams like cursor moves, scrolls, or rapid clicks. This will silently break any downstream consumer that relies on unique IDs for keyed rendering, deduplication, or mapping.
Consider appending a monotonic counter (e.g., `this.eventCounter++`) or a random suffix to guarantee uniqueness.</violation>
</file>
<file name="webapp/src/onboarding/useOnboarding.js">
<violation number="1" location="webapp/src/onboarding/useOnboarding.js:24">
P1: `importantRegions` elements are passed directly to `body` without `String()` coercion, unlike the surrounding fields. Because `body` is rendered as a React child in `OnboardingTooltip` and `OnboardingPanel`, a non-string element (e.g., an object from an AI payload) will cause a runtime React render error.</violation>
<violation number="2" location="webapp/src/onboarding/useOnboarding.js:34">
P1: The `useOnboarding` hook computes and returns onboarding steps (`steps`, `currentStep`) even when `enabled` is `false`. The `enabled` flag only resets `activeStep` and `dismissed` inside an effect, but it does not suppress returned data. Because `Session.jsx` guards rendering with `!dismissed && currentStep` without independently checking `enabled`, the onboarding UI and tooltip will render even when onboarding is disabled — breaking the "disabled by default" guarantee. The hook should enforce the `enabled` contract in its returned state (e.g., by returning `currentStep: null` when `!enabled`), so consumers don't need to duplicate that guard.</violation>
</file>
<file name="webapp/src/hooks/useSession.js">
<violation number="1" location="webapp/src/hooks/useSession.js:63">
P1: The WebSocket effect now feeds events into `SessionRecorder`, but its cleanup only closes the socket. If the component unmounts without calling `leave()` — for example via browser navigation or an error boundary — the recorder keeps `isRecording = true` and the captured timeline is never exported, resulting in data loss. The effect cleanup should also stop and export the recorder so unmount paths don't silently discard recorded data. Consider updating the cleanup to something like:
```js
return () => {
recorderRef.current?.stop();
const exported = recorderRef.current?.exportTimeline() ?? [];
setRecording(exported.length ? { sessionId, events: exported } : null);
socket.close();
};
```</violation>
</file>
<file name="server/signalingHandler.js">
<violation number="1" location="server/signalingHandler.js:79">
P2: The `onboarding:guidance` event is relayed unconditionally by the server whenever a session exists, despite the PR describing onboarding as host-controlled and disabled by default. If a client emits this event while the host has disabled AI onboarding, the server will still forward it to the target guest or host. Consider adding a server-side guard—such as checking a session-level `onboardingEnabled` flag—before relaying this event, so the signaling layer enforces the host toggle rather than trusting every client to respect it.</violation>
</file>
<file name="webapp/src/pages/Session.jsx">
<violation number="1" location="webapp/src/pages/Session.jsx:99">
P2: The recording toggle is controlled through both a React prop (`recordingEnabled` passed to `useSession`) and an imperative setter (`session.setRecordingEnabled`) in the same event handler. Inside `useSession`, the actual `SessionRecorder` start/stop behavior is driven exclusively by the `recordingEnabled` prop change via a `useEffect`. The hook's `setRecordingEnabled` method only updates an internal `recordingOn` state variable and does not touch the recorder. Because the prop path already handles recorder behavior, the imperative call is redundant here. More importantly, keeping both paths creates a misleading API: calling `session.setRecordingEnabled` without also updating the parent `recordingEnabled` state would silently fail to toggle recording. Prefer a single control path—either fully prop-controlled or fully imperative—and remove the redundant setter call in this handler.</violation>
</file>
<file name="webapp/src/components/RecordingBadge.jsx">
<violation number="1" location="webapp/src/components/RecordingBadge.jsx:7">
P2: The `RecordingBadge` component displays dynamic session status ('Recording active' / 'Playback active'), but it is rendered as a plain `<div>` without a live region or status role. Screen-reader users may miss when recording or playback starts or stops because assistive technologies won't announce the appearance or text change of this badge automatically. Consider adding `role="status"` (and `aria-atomic="true"` for complete announcement) so the status is conveyed to assistive technologies as it changes.</violation>
</file>
<file name="webapp/src/onboarding/OnboardingTooltip.jsx">
<violation number="1" location="webapp/src/onboarding/OnboardingTooltip.jsx:12">
P1: The tooltip uses `position: fixed`, but the positioning code adds `window.scrollY`/`window.scrollX` to viewport-relative `getBoundingClientRect()` coordinates. Since `fixed` positioning interprets `top`/`left` as viewport-relative, this produces an inconsistent coordinate system: on scrolled pages the tooltip will be offset by the scroll distance and appear far from its target. Consider removing the scroll offsets so the tooltip is placed using viewport-relative coordinates, which is consistent with `position: fixed`.</violation>
<violation number="2" location="webapp/src/onboarding/OnboardingTooltip.jsx:25">
P2: The dialog role needs an accessible name and initial focus management. The visible title isn’t connected via `aria-labelledby`, so screen readers don’t announce the dialog’s purpose, and focus remains on the page behind the tooltip. Since this PR highlights accessibility improvements, connect the title element with `aria-labelledby` and move focus to the Dismiss button (or the dialog itself) when the tooltip mounts.</violation>
</file>
<file name="server/ai/onboardingService.ts">
<violation number="1" location="server/ai/onboardingService.ts:38">
P2: The AI response is parsed with `JSON.parse(text)` directly on the first text block. Anthropic models frequently wrap JSON in markdown code fences (e.g., ```json ... ```). When this happens, parsing throws and the code silently falls back to generic onboarding guidance — even though the API returned a valid structured response — making the AI onboarding feature unreliable. Consider stripping markdown fences before parsing, or using Anthropic's structured output/tool-use API for guaranteed JSON. Also, items inside `importantRegions` and `recommendedActions` arrays are not validated for type before being rendered in the UI.</violation>
</file>
<file name="extension/onboarding/pageAnalyzer.js">
<violation number="1" location="extension/onboarding/pageAnalyzer.js:28">
P1: The page analyzer extracts raw `textContent` from broad structural containers (`main`, `section`, `dialog`, `aside`) and user input elements (`textarea`, `[contenteditable="true"]`), then transmits that data to Anthropic's API and broadcasts it over WebSocket to guests. This contradicts the stated design goal of avoiding sensitive information.
`getTextContent` returns the full `element.textContent` first, which for large containers can include entire dashboards, documents, or messages. `collectInputFields` reads `textContent` from `textarea` and `contenteditable` fields, capturing live user-entered text. There are no length caps or redaction steps downstream. I recommend adding per-element text-length limits (e.g., 120–200 characters), avoiding `textContent` for `textarea` and `contenteditable` fields, and preferring structural attributes (`aria-label`, `placeholder`, `name`) over bulk text extraction.</violation>
<violation number="2" location="extension/onboarding/pageAnalyzer.js:31">
P2: Input buttons and submits are silently dropped from the onboarding summary because `getTextContent` doesn't read the `value` attribute. For elements like `<input type="submit" value="Save">`, the visible label lives in `value`, not `textContent`, and with no `aria-label`/`title`/`placeholder`, the text comes back empty and the element is skipped. Consider including `element.value` as a fallback after `textContent` so these primary actions are captured.</violation>
<violation number="3" location="extension/onboarding/pageAnalyzer.js:46">
P2: The analyzer discards element identity by reducing matched DOM nodes to plain text and then deduplicating via `Array.from(new Set(items))`. Downstream, `highlightRegions` (in `extension/content/annotationOverlay.js`) must map those text strings back to DOM elements using a simple `.find()` + `.includes()` text search. This breaks reliable walkthrough targeting when multiple UI controls share the same label (e.g., two "Edit" buttons), because only the first match is ever highlighted and duplicate labels are removed before the AI even sees them. Consider preserving selector, index, or bounding-rect metadata alongside text so that the highlight overlay can target the correct element deterministically.</violation>
<violation number="4" location="extension/onboarding/pageAnalyzer.js:56">
P2: Search inputs (`<input type="search">`) are explicitly excluded from the page scan even though the PR goal is to identify important UI regions including search bars. This means pages that use semantic `type="search"` will omit search functionality from the onboarding summary sent to the AI or the fallback generator, which can lead to incomplete walkthrough guidance.
Consider either removing the `:not([type="search"])` exclusion so search fields are surfaced in `inputs`, or adding a dedicated `searchBars` field (e.g., `searchBars: collectVisibleMatches(root, 'input[type="search"]')`) to `analyzePageStructure` so onboarding explicitly captures them.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| background: 'rgba(20, 184, 166, 0.12)', | ||
| pointerEvents: 'none' | ||
| }); | ||
| layer.append(box); |
Contributor
There was a problem hiding this comment.
P1: The annotation notes and onboarding highlight boxes are both appended to the same layer container. addAnnotation() uses layer.children.length to calculate the vertical stacking position, assuming every child is an annotation note. Since highlightRegions() now appends temporary highlight boxes to that same container, the child count is inflated while those boxes exist, and any annotation added during the highlight TTL gets positioned too low (or off-screen) because the child count includes highlight overlays.
To fix this, either keep onboarding highlights in a separate sub-container inside layer, or maintain annotation-specific positioning logic (e.g. count only annotation notes rather than all children).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extension/content/annotationOverlay.js, line 83:
<comment>The annotation notes and onboarding highlight boxes are both appended to the same `layer` container. `addAnnotation()` uses `layer.children.length` to calculate the vertical stacking position, assuming every child is an annotation note. Since `highlightRegions()` now appends temporary highlight boxes to that same container, the child count is inflated while those boxes exist, and any annotation added during the highlight TTL gets positioned too low (or off-screen) because the child count includes highlight overlays.
To fix this, either keep onboarding highlights in a separate sub-container inside `layer`, or maintain annotation-specific positioning logic (e.g. count only annotation notes rather than all children).</comment>
<file context>
@@ -51,9 +52,43 @@
+ background: 'rgba(20, 184, 166, 0.12)',
+ pointerEvents: 'none'
+ });
+ layer.append(box);
+ window.setTimeout(() => box.remove(), HIGHLIGHT_TTL_MS);
+ });
</file context>
| ]; | ||
| } | ||
|
|
||
| export function useOnboarding({ enabled = false, guidance = null, onFinish = () => {} }) { |
Contributor
There was a problem hiding this comment.
P1: The useOnboarding hook computes and returns onboarding steps (steps, currentStep) even when enabled is false. The enabled flag only resets activeStep and dismissed inside an effect, but it does not suppress returned data. Because Session.jsx guards rendering with !dismissed && currentStep without independently checking enabled, the onboarding UI and tooltip will render even when onboarding is disabled — breaking the "disabled by default" guarantee. The hook should enforce the enabled contract in its returned state (e.g., by returning currentStep: null when !enabled), so consumers don't need to duplicate that guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/src/onboarding/useOnboarding.js, line 34:
<comment>The `useOnboarding` hook computes and returns onboarding steps (`steps`, `currentStep`) even when `enabled` is `false`. The `enabled` flag only resets `activeStep` and `dismissed` inside an effect, but it does not suppress returned data. Because `Session.jsx` guards rendering with `!dismissed && currentStep` without independently checking `enabled`, the onboarding UI and tooltip will render even when onboarding is disabled — breaking the "disabled by default" guarantee. The hook should enforce the `enabled` contract in its returned state (e.g., by returning `currentStep: null` when `!enabled`), so consumers don't need to duplicate that guard.</comment>
<file context>
@@ -0,0 +1,80 @@
+ ];
+}
+
+export function useOnboarding({ enabled = false, guidance = null, onFinish = () => {} }) {
+ const [activeStep, setActiveStep] = useState(0);
+ const [dismissed, setDismissed] = useState(false);
</file context>
| ...importantRegions.slice(0, 3).map((region, index) => ({ | ||
| id: `region-${index}`, | ||
| title: 'Highlight', | ||
| body: region |
Contributor
There was a problem hiding this comment.
P1: importantRegions elements are passed directly to body without String() coercion, unlike the surrounding fields. Because body is rendered as a React child in OnboardingTooltip and OnboardingPanel, a non-string element (e.g., an object from an AI payload) will cause a runtime React render error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/src/onboarding/useOnboarding.js, line 24:
<comment>`importantRegions` elements are passed directly to `body` without `String()` coercion, unlike the surrounding fields. Because `body` is rendered as a React child in `OnboardingTooltip` and `OnboardingPanel`, a non-string element (e.g., an object from an AI payload) will cause a runtime React render error.</comment>
<file context>
@@ -0,0 +1,80 @@
+ ...importantRegions.slice(0, 3).map((region, index) => ({
+ id: `region-${index}`,
+ title: 'Highlight',
+ body: region
+ })),
+ {
</file context>
| socket.addEventListener('message', (event) => { | ||
| const message = JSON.parse(event.data); | ||
| if (message.event === 'session:joined') { | ||
| recorderRef.current?.capture({ type: 'participant:joined', payload: { guest: message.payload.guest }, participantId: guestName, timestamp: Date.now() }); |
Contributor
There was a problem hiding this comment.
P1: The WebSocket effect now feeds events into SessionRecorder, but its cleanup only closes the socket. If the component unmounts without calling leave() — for example via browser navigation or an error boundary — the recorder keeps isRecording = true and the captured timeline is never exported, resulting in data loss. The effect cleanup should also stop and export the recorder so unmount paths don't silently discard recorded data. Consider updating the cleanup to something like:
return () => {
recorderRef.current?.stop();
const exported = recorderRef.current?.exportTimeline() ?? [];
setRecording(exported.length ? { sessionId, events: exported } : null);
socket.close();
};Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/src/hooks/useSession.js, line 63:
<comment>The WebSocket effect now feeds events into `SessionRecorder`, but its cleanup only closes the socket. If the component unmounts without calling `leave()` — for example via browser navigation or an error boundary — the recorder keeps `isRecording = true` and the captured timeline is never exported, resulting in data loss. The effect cleanup should also stop and export the recorder so unmount paths don't silently discard recorded data. Consider updating the cleanup to something like:
```js
return () => {
recorderRef.current?.stop();
const exported = recorderRef.current?.exportTimeline() ?? [];
setRecording(exported.length ? { sessionId, events: exported } : null);
socket.close();
};
```</comment>
<file context>
@@ -35,6 +60,7 @@ export function useSession({ sessionId, guestName }) {
socket.addEventListener('message', (event) => {
const message = JSON.parse(event.data);
if (message.event === 'session:joined') {
+ recorderRef.current?.capture({ type: 'participant:joined', payload: { guest: message.payload.guest }, participantId: guestName, timestamp: Date.now() });
setGuest(message.payload.guest);
setPermissions(message.payload.permissions || DEFAULT_PERMISSIONS);
</file context>
|
|
||
| const rect = target.getBoundingClientRect(); | ||
| const element = ref.current; | ||
| element.style.top = `${Math.max(16, rect.top + window.scrollY + 12)}px`; |
Contributor
There was a problem hiding this comment.
P1: The tooltip uses position: fixed, but the positioning code adds window.scrollY/window.scrollX to viewport-relative getBoundingClientRect() coordinates. Since fixed positioning interprets top/left as viewport-relative, this produces an inconsistent coordinate system: on scrolled pages the tooltip will be offset by the scroll distance and appear far from its target. Consider removing the scroll offsets so the tooltip is placed using viewport-relative coordinates, which is consistent with position: fixed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/src/onboarding/OnboardingTooltip.jsx, line 12:
<comment>The tooltip uses `position: fixed`, but the positioning code adds `window.scrollY`/`window.scrollX` to viewport-relative `getBoundingClientRect()` coordinates. Since `fixed` positioning interprets `top`/`left` as viewport-relative, this produces an inconsistent coordinate system: on scrolled pages the tooltip will be offset by the scroll distance and appear far from its target. Consider removing the scroll offsets so the tooltip is placed using viewport-relative coordinates, which is consistent with `position: fixed`.</comment>
<file context>
@@ -0,0 +1,33 @@
+
+ const rect = target.getBoundingClientRect();
+ const element = ref.current;
+ element.style.top = `${Math.max(16, rect.top + window.scrollY + 12)}px`;
+ element.style.left = `${Math.max(16, rect.left + window.scrollX)}px`;
+ element.style.maxWidth = '280px';
</file context>
| }, [body, onClose, targetSelector]); | ||
|
|
||
| return ( | ||
| <div ref={ref} className="fixed z-[2147483647] rounded-lg border border-slate-200 bg-white p-3 shadow-lg" role="dialog" aria-live="polite"> |
Contributor
There was a problem hiding this comment.
P2: The dialog role needs an accessible name and initial focus management. The visible title isn’t connected via aria-labelledby, so screen readers don’t announce the dialog’s purpose, and focus remains on the page behind the tooltip. Since this PR highlights accessibility improvements, connect the title element with aria-labelledby and move focus to the Dismiss button (or the dialog itself) when the tooltip mounts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/src/onboarding/OnboardingTooltip.jsx, line 25:
<comment>The dialog role needs an accessible name and initial focus management. The visible title isn’t connected via `aria-labelledby`, so screen readers don’t announce the dialog’s purpose, and focus remains on the page behind the tooltip. Since this PR highlights accessibility improvements, connect the title element with `aria-labelledby` and move focus to the Dismiss button (or the dialog itself) when the tooltip mounts.</comment>
<file context>
@@ -0,0 +1,33 @@
+ }, [body, onClose, targetSelector]);
+
+ return (
+ <div ref={ref} className="fixed z-[2147483647] rounded-lg border border-slate-200 bg-white p-3 shadow-lg" role="dialog" aria-live="polite">
+ <div className="text-sm font-semibold text-slate-950">{title}</div>
+ <p className="mt-1 text-sm text-slate-600">{body}</p>
</file context>
| } | ||
|
|
||
| const body = await response.json(); | ||
| const text = body.content?.find((part) => part.type === 'text')?.text || '{}'; |
Contributor
There was a problem hiding this comment.
P2: The AI response is parsed with JSON.parse(text) directly on the first text block. Anthropic models frequently wrap JSON in markdown code fences (e.g., json ... ). When this happens, parsing throws and the code silently falls back to generic onboarding guidance — even though the API returned a valid structured response — making the AI onboarding feature unreliable. Consider stripping markdown fences before parsing, or using Anthropic's structured output/tool-use API for guaranteed JSON. Also, items inside importantRegions and recommendedActions arrays are not validated for type before being rendered in the UI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ai/onboardingService.ts, line 38:
<comment>The AI response is parsed with `JSON.parse(text)` directly on the first text block. Anthropic models frequently wrap JSON in markdown code fences (e.g., ```json ... ```). When this happens, parsing throws and the code silently falls back to generic onboarding guidance — even though the API returned a valid structured response — making the AI onboarding feature unreliable. Consider stripping markdown fences before parsing, or using Anthropic's structured output/tool-use API for guaranteed JSON. Also, items inside `importantRegions` and `recommendedActions` arrays are not validated for type before being rendered in the UI.</comment>
<file context>
@@ -0,0 +1,50 @@
+ }
+
+ const body = await response.json();
+ const text = body.content?.find((part) => part.type === 'text')?.text || '{}';
+ const parsed = JSON.parse(text);
+ return {
</file context>
| const inputs = []; | ||
| if (!root || typeof root.querySelectorAll !== 'function') return inputs; | ||
|
|
||
| const matches = root.querySelectorAll('input:not([type="hidden"]):not([type="password"]):not([type="email"]):not([type="tel"]):not([type="number"]):not([type="search"]), textarea, [contenteditable="true"]'); |
Contributor
There was a problem hiding this comment.
P2: Search inputs (<input type="search">) are explicitly excluded from the page scan even though the PR goal is to identify important UI regions including search bars. This means pages that use semantic type="search" will omit search functionality from the onboarding summary sent to the AI or the fallback generator, which can lead to incomplete walkthrough guidance.
Consider either removing the :not([type="search"]) exclusion so search fields are surfaced in inputs, or adding a dedicated searchBars field (e.g., searchBars: collectVisibleMatches(root, 'input[type="search"]')) to analyzePageStructure so onboarding explicitly captures them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extension/onboarding/pageAnalyzer.js, line 56:
<comment>Search inputs (`<input type="search">`) are explicitly excluded from the page scan even though the PR goal is to identify important UI regions including search bars. This means pages that use semantic `type="search"` will omit search functionality from the onboarding summary sent to the AI or the fallback generator, which can lead to incomplete walkthrough guidance.
Consider either removing the `:not([type="search"])` exclusion so search fields are surfaced in `inputs`, or adding a dedicated `searchBars` field (e.g., `searchBars: collectVisibleMatches(root, 'input[type="search"]')`) to `analyzePageStructure` so onboarding explicitly captures them.</comment>
<file context>
@@ -0,0 +1,104 @@
+ const inputs = [];
+ if (!root || typeof root.querySelectorAll !== 'function') return inputs;
+
+ const matches = root.querySelectorAll('input:not([type="hidden"]):not([type="password"]):not([type="email"]):not([type="tel"]):not([type="number"]):not([type="search"]), textarea, [contenteditable="true"]');
+ for (const element of matches) {
+ if (!isVisible(element)) continue;
</file context>
Suggested change
| const matches = root.querySelectorAll('input:not([type="hidden"]):not([type="password"]):not([type="email"]):not([type="tel"]):not([type="number"]):not([type="search"]), textarea, [contenteditable="true"]'); | |
| const matches = root.querySelectorAll('input:not([type="hidden"]):not([type="password"]):not([type="email"]):not([type="tel"]):not([type="number"]), textarea, [contenteditable="true"]'); |
| if (!isVisible(element)) continue; | ||
| const text = getTextContent(element); | ||
| if (!text) continue; | ||
| items.push(text); |
Contributor
There was a problem hiding this comment.
P2: The analyzer discards element identity by reducing matched DOM nodes to plain text and then deduplicating via Array.from(new Set(items)). Downstream, highlightRegions (in extension/content/annotationOverlay.js) must map those text strings back to DOM elements using a simple .find() + .includes() text search. This breaks reliable walkthrough targeting when multiple UI controls share the same label (e.g., two "Edit" buttons), because only the first match is ever highlighted and duplicate labels are removed before the AI even sees them. Consider preserving selector, index, or bounding-rect metadata alongside text so that the highlight overlay can target the correct element deterministically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extension/onboarding/pageAnalyzer.js, line 46:
<comment>The analyzer discards element identity by reducing matched DOM nodes to plain text and then deduplicating via `Array.from(new Set(items))`. Downstream, `highlightRegions` (in `extension/content/annotationOverlay.js`) must map those text strings back to DOM elements using a simple `.find()` + `.includes()` text search. This breaks reliable walkthrough targeting when multiple UI controls share the same label (e.g., two "Edit" buttons), because only the first match is ever highlighted and duplicate labels are removed before the AI even sees them. Consider preserving selector, index, or bounding-rect metadata alongside text so that the highlight overlay can target the correct element deterministically.</comment>
<file context>
@@ -0,0 +1,104 @@
+ if (!isVisible(element)) continue;
+ const text = getTextContent(element);
+ if (!text) continue;
+ items.push(text);
+ }
+
</file context>
| function getTextContent(element) { | ||
| const directText = normalizeText(element?.textContent || ''); | ||
| if (directText) return directText; | ||
| const ariaLabel = normalizeText(element?.getAttribute?.('aria-label') || ''); |
Contributor
There was a problem hiding this comment.
P2: Input buttons and submits are silently dropped from the onboarding summary because getTextContent doesn't read the value attribute. For elements like <input type="submit" value="Save">, the visible label lives in value, not textContent, and with no aria-label/title/placeholder, the text comes back empty and the element is skipped. Consider including element.value as a fallback after textContent so these primary actions are captured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extension/onboarding/pageAnalyzer.js, line 31:
<comment>Input buttons and submits are silently dropped from the onboarding summary because `getTextContent` doesn't read the `value` attribute. For elements like `<input type="submit" value="Save">`, the visible label lives in `value`, not `textContent`, and with no `aria-label`/`title`/`placeholder`, the text comes back empty and the element is skipped. Consider including `element.value` as a fallback after `textContent` so these primary actions are captured.</comment>
<file context>
@@ -0,0 +1,104 @@
+function getTextContent(element) {
+ const directText = normalizeText(element?.textContent || '');
+ if (directText) return directText;
+ const ariaLabel = normalizeText(element?.getAttribute?.('aria-label') || '');
+ const title = normalizeText(element?.getAttribute?.('title') || '');
+ const placeholder = normalizeText(element?.getAttribute?.('placeholder') || '');
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
extension/content/annotationOverlay.js (1)
55-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo cleanup of previously drawn boxes before adding new ones.
Each call to
highlightRegionsappends new boxes without clearing any still-visible boxes from a prior call. If onboarding triggers highlighting more than once in quick succession (e.g., re-triggered guidance), boxes can stack up until their individual timeouts expire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/content/annotationOverlay.js` around lines 55 - 86, The highlightRegions function in annotationOverlay.js appends new overlay boxes on every call without removing any existing ones first, so repeated onboarding triggers can stack highlights. Update highlightRegions to clear any previously added boxes from the overlay layer before creating new ones, using the existing layer management helpers such as ensureLayer and the shared layer element, while keeping the current timeout-based removal for newly created boxes.webapp/src/onboarding/OnboardingPanel.jsx (1)
20-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winButton block duplicates
WalkthroughControls.This markup (Previous/Skip/Next-or-Finish) is nearly identical to
webapp/src/onboarding/WalkthroughControls.jsx. Consider havingOnboardingPanelrender<WalkthroughControls>instead of duplicating the buttons, to avoid divergence between the two over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/onboarding/OnboardingPanel.jsx` around lines 20 - 36, OnboardingPanel currently duplicates the Previous/Skip/Next-or-Finish button block already implemented in WalkthroughControls, which risks the two UIs drifting apart. Update OnboardingPanel to render the shared WalkthroughControls component instead of inlining the buttons, passing through the same handlers and state props it needs (such as canGoBack, canGoNext, onPrevious, onSkip, onNext, and onFinish). Keep the button behavior and styling sourced from WalkthroughControls so there is only one place to maintain this control set.webapp/src/onboarding/OnboardingTooltip.jsx (1)
6-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTooltip doesn't reposition on scroll/resize, and lacks initial focus for the dialog role.
Position is only computed once per
[body, onClose, targetSelector]change; scrolling or resizing after mount leaves the tooltip misaligned withtarget. Also, the element hasrole="dialog"but nothing moves focus into it on mount, which weakens the keyboard-accessibility goal called out in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/onboarding/OnboardingTooltip.jsx` around lines 6 - 22, The OnboardingTooltip positioning and accessibility need to be updated: in OnboardingTooltip.jsx, the current effect only computes placement once and never reacts to scroll/resize, so the tooltip can drift from targetSelector after mount. Refactor the useEffect that sets ref.current styles to recompute position on window scroll and resize (with cleanup), and make sure it still uses target.getBoundingClientRect() from the target element. Also, when the tooltip mounts with role="dialog", move initial focus into the tooltip element via ref.current so keyboard users land inside it immediately.extension/onboarding/onboardingService.js (1)
3-22: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftRaw scraped page content is sent to a third-party AI vendor with no technical redaction.
summary(built fromanalyzePageStructureinpageAnalyzer.js) includes raw visible text — headings, button labels, form/section text, input field labels — and is forwarded verbatim asJSON.stringify(summary)to Anthropic. The only safeguard against leaking sensitive on-page content is a soft system-prompt instruction ("avoid sensitive details"), which is not a technical control and doesn't prevent the raw payload itself from being transmitted and processed by the third-party API.Given this runs automatically whenever a host enables AI onboarding (no per-guest/per-page consent), consider scrubbing or limiting the transmitted summary (e.g., truncating field values, excluding sections likely to contain sensitive dashboards/forms) before sending it externally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/onboarding/onboardingService.js` around lines 3 - 22, The onboarding request currently forwards raw page summary data to Anthropic without technical redaction. Update generateOnboardingGuidance in onboardingService.js so it sanitizes or reduces the summary before building the messages payload, ideally by filtering sensitive fields, truncating visible text, or excluding likely private sections generated by analyzePageStructure in pageAnalyzer.js. Keep the external request focused on a minimal, redacted summary instead of passing JSON.stringify(summary) verbatim.extension/background/serviceWorker.js (1)
187-201: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRaw page
summaryrelayed to the guest (and through the signaling server) unfiltered.
sendSocket('onboarding:guidance', { guestId, guidance, summary, enabled: true })forwards the entire scraped page structure (headings, form/section text, button labels, etc.) through the signaling server to the guest. This expands what passes through your own relay infrastructure for every guest join when the feature is enabled — worth confirming this is intentional and doesn't need trimming/redaction before broadcast, separate from the AI-vendor exposure already noted inonboardingService.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/background/serviceWorker.js` around lines 187 - 201, The `triggerOnboardingForGuest` flow is relaying the full scraped `summary` object to guests via `sendSocket('onboarding:guidance', ...)`, which may expose more page content than intended. Update this path to trim/redact the `summary` before broadcast, or replace it with a minimal payload containing only the fields needed by the guest and signaling flow, while keeping `generateOnboardingGuidance` and `sendToActiveTab` behavior intact.server/ai/onboardingService.ts (1)
1-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing type annotations in a
.tsfile.None of the functions, parameters, or return values are typed (
summary,apiKey, the parsed AI response shape). Given this parses untrusted JSON text from an external API response, an explicit interface for the onboarding payload would catch shape mismatches at compile time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/ai/onboardingService.ts` around lines 1 - 50, Add explicit TypeScript types to the onboarding flow in generateOnboardingGuidance and createFallbackOnboarding, since summary, apiKey, and the parsed response are currently untyped in a .ts file. Define a clear onboarding payload interface for the returned object and a type for the parsed Anthropic JSON, then use those types in the function signatures and when reading body.content and JSON.parse output. Keep the fallback and response-normalization logic the same, but make the shape of welcomeMessage, pageOverview, importantRegions, recommendedActions, and walkthrough explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extension/content/annotationOverlay.js`:
- Around line 60-68: The matching in annotationOverlay.js is too broad because
candidates.find() on the querySelectorAll results will pick container elements
like main, section, nav, and form before more specific descendants. Update the
candidate selection logic near the phrases loop to prefer the smallest or most
specific matching element in annotationOverlay.js, using the existing candidates
array and the target search step, so buttons/inputs/links are chosen over their
wrapping containers. Consider filtering out large structural containers or
ranking matches by specificity before assigning target.
In `@extension/onboarding/onboardingService.js`:
- Around line 9-22: The Anthropic request in generateOnboardingGuidance lacks a
timeout, so a hung fetch can leave triggerOnboardingForGuest waiting forever.
Add an AbortController-based timeout around the fetch call in
onboardingService.js, and make sure triggerOnboardingForGuest in
serviceWorker.js can fall back cleanly when the request is aborted or fails so
the guest still gets onboarding handling.
In `@extension/onboarding/pageAnalyzer.js`:
- Around line 68-104: The file uses ES6 export statements but is loaded as a
classic content script from manifest.json, which doesn't support ES6 modules and
will throw an error before the chrome.runtime.onMessage listener and
globalThis.TabTwinOnboarding assignments execute. Remove the export keywords
from the function declarations analyzePageStructure and createFallbackOnboarding
since these functions are already being registered globally via
globalThis.TabTwinOnboarding and the chrome.runtime.onMessage listener, making
the export statements unnecessary and breaking the content script.
In `@server/ai/onboardingService.ts`:
- Around line 12-50: The Anthropic request in generateOnboardingGuidance can
hang indefinitely because the fetch call has no timeout. Update this function to
use an AbortController (or equivalent timeout wrapper) around the fetch to
https://api.anthropic.com/v1/messages, abort the request after a reasonable
duration, and ensure the existing fallback path is still returned on timeout or
abort. Keep the fix localized to generateOnboardingGuidance and its fetch
handling.
- Around line 1-50: The server-side onboarding implementation is a duplicate of
the extension onboarding service and is no longer referenced anywhere, so remove
the unused `generateOnboardingGuidance`/`createFallbackOnboarding` module
entirely. If any future server-side caller needs this behavior, point it to the
existing `extension/onboarding/onboardingService.js` logic or extract a shared
helper instead of keeping two copies in sync.
In `@tests/onboarding.test.js`:
- Around line 25-27: The mock in the onboarding test is using a selector string
that no longer matches the real collectInputFields implementation, so the
sensitive input branch never runs and the assertion is trivial. Update the
querySelectorAll stub in the onboarding test to match the full selector used by
pageAnalyzer.js, and keep the secret input fixture in the branch that should be
exercised so summary.inputs.length validates the intended filtering behavior.
In `@tests/recording.test.js`:
- Around line 30-42: The test expectation is missing the guaranteed-kept cursor
event from SessionRecorder.exportTimeline. Since the first captured cursor:move
is not skipped by shouldSkipCursorEvent in SessionRecorder, update the
assertions in SessionRecorder exports timeline after recording is disabled to
expect a length of 3 and verify the preserved event order as session:start,
cursor:move, session:end.
- Around line 6-28: The SessionRecorder timeline expectation in this test does
not match actual ordering and compression behavior because
session:start/session:end use Date.now() while the captured events use fixed
timestamps, and the cursor:move entries at (5,8) and (9,8) are not compressible.
Update the assertions in recording.test.js to reflect the real exportTimeline()
output from SessionRecorder.start, capture, stop, and exportTimeline, or freeze
the clock so the session markers sort consistently and the expected event
count/order is correct.
In `@webapp/src/components/Timeline.jsx`:
- Around line 15-20: The Timeline event list is using key={event.id} without a
fallback, so update the events.map rendering in Timeline to use a stable
composite key when id is missing, matching the safer approach already used in
Session.jsx (for example combining event.id or the loop index with the index).
Keep the key on the mapped div wrapper and do not move it to the inner span
elements.
In `@webapp/src/hooks/useSession.js`:
- Around line 131-137: The leave() flow in useSession is discarding the exported
timeline because it sets recording state and then immediately hard-navigates
away, unmounting the app before Session.jsx can use it. Update leave() so the
timeline exported from recorderRef.current.exportTimeline() is persisted
somewhere that survives navigation, and only then close the socket and redirect;
keep the recording accessible through the session end/playback path rather than
relying on transient React state alone.
- Around line 18-51: The recording state in useSession is split between the
recordingEnabled prop and the internal recordingOn state, so setRecordingEnabled
only updates UI state without driving SessionRecorder. Make recordingOn the
single source of truth in useSession by having the recorder effect respond to
recordingOn (or by wiring setRecordingEnabled to the prop-driven path), and
update the SessionRecorder start/stop/export logic in the useSession effect and
setRecordingEnabled so they stay synchronized for any caller.
In `@webapp/src/onboarding/useOnboarding.js`:
- Around line 40-49: The reset logic in useOnboarding is redundant and
incomplete: both branches of the enabled check do the same thing, and the effect
in useEffect only reacts to enabled changes. Simplify the branch to a single
reset, and update the effect dependencies so that when guidance changes in
useMemo-derived steps while enabled remains true, activeStep and dismissed are
reset too. Use the existing useOnboarding, useEffect, and steps identifiers to
locate the reset behavior.
In `@webapp/src/pages/Session.jsx`:
- Line 78: The RecordingBadge state logic in Session.jsx drops out of playback
when playbackState becomes paused, causing the badge to disappear during paused
playback. Update the state computation for RecordingBadge so it treats both
'playing' and 'paused' as playback-related states (while keeping the existing
idle/recording behavior), using the playbackState and recordingEnabled values
already in Session.jsx and the RecordingBadge prop.
- Around line 28-32: Gate the onboarding UI in Session.jsx on
session.onboarding?.enabled, because useOnboarding() can still produce a truthy
currentStep even when onboarding is disabled. Update the render logic around the
useOnboarding result so the onboarding elements only appear when onboarding is
explicitly enabled, and keep using the existing useOnboarding hook and
currentStep/activeStep checks as the location to apply the guard.
In `@webapp/src/recording/SessionRecorder.js`:
- Around line 13-24: `SessionRecorder.start()` is reusing old timeline data
after a previous `stop()`, so a new recording can start with stale `session:end`
events still present. Update the `start()` flow to reset or clear the recorder
state before beginning a fresh session, and make sure the
`session:start`/`session:end` sequence in `capture()` remains valid across
repeated enable/disable cycles triggered by `useSession.js`.
---
Nitpick comments:
In `@extension/background/serviceWorker.js`:
- Around line 187-201: The `triggerOnboardingForGuest` flow is relaying the full
scraped `summary` object to guests via `sendSocket('onboarding:guidance', ...)`,
which may expose more page content than intended. Update this path to
trim/redact the `summary` before broadcast, or replace it with a minimal payload
containing only the fields needed by the guest and signaling flow, while keeping
`generateOnboardingGuidance` and `sendToActiveTab` behavior intact.
In `@extension/content/annotationOverlay.js`:
- Around line 55-86: The highlightRegions function in annotationOverlay.js
appends new overlay boxes on every call without removing any existing ones
first, so repeated onboarding triggers can stack highlights. Update
highlightRegions to clear any previously added boxes from the overlay layer
before creating new ones, using the existing layer management helpers such as
ensureLayer and the shared layer element, while keeping the current
timeout-based removal for newly created boxes.
In `@extension/onboarding/onboardingService.js`:
- Around line 3-22: The onboarding request currently forwards raw page summary
data to Anthropic without technical redaction. Update generateOnboardingGuidance
in onboardingService.js so it sanitizes or reduces the summary before building
the messages payload, ideally by filtering sensitive fields, truncating visible
text, or excluding likely private sections generated by analyzePageStructure in
pageAnalyzer.js. Keep the external request focused on a minimal, redacted
summary instead of passing JSON.stringify(summary) verbatim.
In `@server/ai/onboardingService.ts`:
- Around line 1-50: Add explicit TypeScript types to the onboarding flow in
generateOnboardingGuidance and createFallbackOnboarding, since summary, apiKey,
and the parsed response are currently untyped in a .ts file. Define a clear
onboarding payload interface for the returned object and a type for the parsed
Anthropic JSON, then use those types in the function signatures and when reading
body.content and JSON.parse output. Keep the fallback and response-normalization
logic the same, but make the shape of welcomeMessage, pageOverview,
importantRegions, recommendedActions, and walkthrough explicit.
In `@webapp/src/onboarding/OnboardingPanel.jsx`:
- Around line 20-36: OnboardingPanel currently duplicates the
Previous/Skip/Next-or-Finish button block already implemented in
WalkthroughControls, which risks the two UIs drifting apart. Update
OnboardingPanel to render the shared WalkthroughControls component instead of
inlining the buttons, passing through the same handlers and state props it needs
(such as canGoBack, canGoNext, onPrevious, onSkip, onNext, and onFinish). Keep
the button behavior and styling sourced from WalkthroughControls so there is
only one place to maintain this control set.
In `@webapp/src/onboarding/OnboardingTooltip.jsx`:
- Around line 6-22: The OnboardingTooltip positioning and accessibility need to
be updated: in OnboardingTooltip.jsx, the current effect only computes placement
once and never reacts to scroll/resize, so the tooltip can drift from
targetSelector after mount. Refactor the useEffect that sets ref.current styles
to recompute position on window scroll and resize (with cleanup), and make sure
it still uses target.getBoundingClientRect() from the target element. Also, when
the tooltip mounts with role="dialog", move initial focus into the tooltip
element via ref.current so keyboard users land inside it immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cb5f57b-f490-4bf2-8415-fbcb009710d0
📒 Files selected for processing (26)
README.mdextension/background/serviceWorker.jsextension/content/annotationOverlay.jsextension/manifest.jsonextension/onboarding/onboardingService.jsextension/onboarding/pageAnalyzer.jsextension/popup/Popup.jsxpackage.jsonserver/ai/onboardingService.tsserver/signalingHandler.jstests/onboarding.test.jstests/recording.test.jswebapp/src/components/PlaybackControls.jsxwebapp/src/components/RecordingBadge.jsxwebapp/src/components/Timeline.jsxwebapp/src/hooks/useSession.jswebapp/src/onboarding/OnboardingPanel.jsxwebapp/src/onboarding/OnboardingTooltip.jsxwebapp/src/onboarding/WalkthroughControls.jsxwebapp/src/onboarding/useOnboarding.jswebapp/src/pages/Session.jsxwebapp/src/recording/PlaybackEngine.jswebapp/src/recording/README.mdwebapp/src/recording/SessionRecorder.jswebapp/src/recording/recordingDemo.jswebapp/src/recording/types.js
Comment on lines
+60
to
+68
| const candidates = Array.from(document.querySelectorAll('button, a, nav, form, main, section, header, footer, input, textarea, [role="button"], [role="navigation"], [role="main"], [role="region"], [role="dialog"]')); | ||
| phrases.forEach((phrase) => { | ||
| const target = candidates.find((candidate) => { | ||
| const text = candidate.textContent || ''; | ||
| return text.toLowerCase().includes(phrase.toLowerCase()); | ||
| }); | ||
|
|
||
| if (!target) return; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Container elements will shadow specific targets in matching.
querySelectorAll returns nodes in document order, so wrapping containers (main, section, nav, form, header) appear before their own descendants in candidates. Since textContent on a container includes all descendant text, candidates.find(...) will typically match the broad container first for almost any phrase, rather than the specific button/nav/input the guidance intended to highlight. This defeats the purpose of precise region highlighting — most highlight boxes will end up covering large containers instead of the actual UI element.
💡 Suggested fix: prefer smallest/most specific match
- const target = candidates.find((candidate) => {
- const text = candidate.textContent || '';
- return text.toLowerCase().includes(phrase.toLowerCase());
- });
+ const matches = candidates.filter((candidate) => {
+ const text = candidate.textContent || '';
+ return text.toLowerCase().includes(phrase.toLowerCase());
+ });
+ // Prefer the most specific (smallest) matching element to avoid
+ // highlighting large containers like <main> or <section>.
+ const target = matches.sort((a, b) => a.textContent.length - b.textContent.length)[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const candidates = Array.from(document.querySelectorAll('button, a, nav, form, main, section, header, footer, input, textarea, [role="button"], [role="navigation"], [role="main"], [role="region"], [role="dialog"]')); | |
| phrases.forEach((phrase) => { | |
| const target = candidates.find((candidate) => { | |
| const text = candidate.textContent || ''; | |
| return text.toLowerCase().includes(phrase.toLowerCase()); | |
| }); | |
| if (!target) return; | |
| const candidates = Array.from(document.querySelectorAll('button, a, nav, form, main, section, header, footer, input, textarea, [role="button"], [role="navigation"], [role="main"], [role="region"], [role="dialog"]')); | |
| phrases.forEach((phrase) => { | |
| const matches = candidates.filter((candidate) => { | |
| const text = candidate.textContent || ''; | |
| return text.toLowerCase().includes(phrase.toLowerCase()); | |
| }); | |
| // Prefer the most specific (smallest) matching element to avoid | |
| // highlighting large containers like <main> or <section>. | |
| const target = matches.sort((a, b) => a.textContent.length - b.textContent.length)[0]; | |
| if (!target) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extension/content/annotationOverlay.js` around lines 60 - 68, The matching in
annotationOverlay.js is too broad because candidates.find() on the
querySelectorAll results will pick container elements like main, section, nav,
and form before more specific descendants. Update the candidate selection logic
near the phrases loop to prefer the smallest or most specific matching element
in annotationOverlay.js, using the existing candidates array and the target
search step, so buttons/inputs/links are chosen over their wrapping containers.
Consider filtering out large structural containers or ranking matches by
specificity before assigning target.
Comment on lines
+9
to
+22
| const response = await fetch('https://api.anthropic.com/v1/messages', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'x-api-key': apiKey, | ||
| 'anthropic-version': '2023-06-01' | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: 'claude-sonnet-4-20250514', | ||
| max_tokens: 700, | ||
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | ||
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | ||
| }) | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout guard on the Anthropic API call.
This is the implementation actually wired into triggerOnboardingForGuest in extension/background/serviceWorker.js. Without a timeout, a stalled network request will leave triggerOnboardingForGuest's await generateOnboardingGuidance(...) pending indefinitely, so the guest never receives onboarding guidance (or a fallback) for that join event.
⏱️ Add a timeout via AbortController
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 8000);
const response = await fetch('https://api.anthropic.com/v1/messages', {
method: 'POST',
+ signal: controller.signal,
headers: {
'Content-Type': 'application/json',
'x-api-key': apiKey,
'anthropic-version': '2023-06-01'
},
body: JSON.stringify({
model: 'claude-sonnet-4-20250514',
max_tokens: 700,
system: '...',
messages: [{ role: 'user', content: JSON.stringify(summary) }]
})
});
+ clearTimeout(timeoutId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const response = await fetch('https://api.anthropic.com/v1/messages', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'x-api-key': apiKey, | |
| 'anthropic-version': '2023-06-01' | |
| }, | |
| body: JSON.stringify({ | |
| model: 'claude-sonnet-4-20250514', | |
| max_tokens: 700, | |
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | |
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | |
| }) | |
| }); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 8000); | |
| const response = await fetch('https://api.anthropic.com/v1/messages', { | |
| method: 'POST', | |
| signal: controller.signal, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'x-api-key': apiKey, | |
| 'anthropic-version': '2023-06-01' | |
| }, | |
| body: JSON.stringify({ | |
| model: 'claude-sonnet-4-20250514', | |
| max_tokens: 700, | |
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | |
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | |
| }) | |
| }); | |
| clearTimeout(timeoutId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extension/onboarding/onboardingService.js` around lines 9 - 22, The Anthropic
request in generateOnboardingGuidance lacks a timeout, so a hung fetch can leave
triggerOnboardingForGuest waiting forever. Add an AbortController-based timeout
around the fetch call in onboardingService.js, and make sure
triggerOnboardingForGuest in serviceWorker.js can fall back cleanly when the
request is aborted or fails so the guest still gets onboarding handling.
Comment on lines
+68
to
+104
| export function analyzePageStructure(root = globalThis.document) { | ||
| const title = normalizeText(root?.title || root?.querySelector?.('title')?.textContent || ''); | ||
| return { | ||
| title, | ||
| navigation: collectVisibleMatches(root, 'nav, [role="navigation"]'), | ||
| buttons: collectVisibleMatches(root, 'button, [role="button"], input[type="button"], input[type="submit"]'), | ||
| forms: collectVisibleMatches(root, 'form'), | ||
| headings: collectVisibleMatches(root, 'h1, h2, h3'), | ||
| inputs: collectInputFields(root), | ||
| sections: collectVisibleMatches(root, 'main, [role="main"], section, [role="region"], aside, [role="complementary"], dialog, [role="dialog"]') | ||
| }; | ||
| } | ||
|
|
||
| export function createFallbackOnboarding(summary = {}) { | ||
| const title = normalizeText(summary.title || 'this page'); | ||
| return { | ||
| welcomeMessage: `Welcome! ${title ? `You are viewing ${title}.` : 'You are viewing a new page.'}`, | ||
| pageOverview: 'The page is ready for a quick walkthrough. Focus on the main sections and the primary actions first.', | ||
| importantRegions: [], | ||
| recommendedActions: ['Review the main navigation.', 'Check the primary call to action.', 'Take the next step with confidence.'], | ||
| walkthrough: 'Begin by reviewing the visible navigation and the most important action on the page.' | ||
| }; | ||
| } | ||
|
|
||
| if (typeof globalThis !== 'undefined') { | ||
| globalThis.TabTwinOnboarding = { analyzePageStructure, createFallbackOnboarding }; | ||
| } | ||
|
|
||
| if (typeof chrome !== 'undefined' && chrome.runtime?.onMessage) { | ||
| chrome.runtime.onMessage.addListener((message, _sender, sendResponse) => { | ||
| if (message?.type === 'onboarding:analyze') { | ||
| sendResponse(analyzePageStructure(globalThis.document)); | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'webpack.config' -e vite.config -e rollup.config
cat package.json 2>/dev/null | jq '.scripts'
fd -a 'manifest.json' --exec cat {}Repository: itzzavdhesh/TabTwin
Length of output: 1150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## extension/package.json\n'
cat extension/package.json
printf '\n## extension build-related files\n'
fd -a 'vite.config.*|rollup.config.*|webpack.config.*|tsconfig*.json|package.json' extension
printf '\n## pageAnalyzer.js outline\n'
ast-grep outline extension/onboarding/pageAnalyzer.js --view expanded || trueRepository: itzzavdhesh/TabTwin
Length of output: 1267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## pageAnalyzer.js first 220 lines\n'
cat -n extension/onboarding/pageAnalyzer.js | sed -n '1,220p'
printf '\n## onboardingService.js references\n'
rg -n "createFallbackOnboarding|TabTwinOnboarding|pageAnalyzer" extension -SRepository: itzzavdhesh/TabTwin
Length of output: 5660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## extension/package.json\n'
cat extension/package.json
printf '\n## extension build-related files\n'
fd -a 'vite.config.*|rollup.config.*|webpack.config.*|tsconfig*.json|package.json' extension
printf '\n## pageAnalyzer.js outline\n'
ast-grep outline extension/onboarding/pageAnalyzer.js --view expanded || true
printf '\n## pageAnalyzer.js source\n'
cat -n extension/onboarding/pageAnalyzer.js | sed -n '1,220p'
printf '\n## onboardingService.js references\n'
rg -n "createFallbackOnboarding|TabTwinOnboarding|pageAnalyzer" extension -SRepository: itzzavdhesh/TabTwin
Length of output: 6761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## extension/vite.config.js\n'
cat -n extension/vite.config.js
printf '\n## files under extension/onboarding\n'
fd -a . extension/onboarding
printf '\n## references to extension/onboarding/pageAnalyzer.js outside manifest/service\n'
rg -n "onboarding/pageAnalyzer\.js|onboarding/pageAnalyzer|TabTwinOnboarding" -S .Repository: itzzavdhesh/TabTwin
Length of output: 1192
export breaks this content script
extension/onboarding/pageAnalyzer.js is loaded directly from manifest.json, and the extension Vite config only builds the popup. Chrome content scripts run as classic scripts, so the top-level export statements will throw and prevent both chrome.runtime.onMessage and globalThis.TabTwinOnboarding from ever registering. Split this into a content-script-safe file or bundle it before listing it in the manifest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extension/onboarding/pageAnalyzer.js` around lines 68 - 104, The file uses
ES6 export statements but is loaded as a classic content script from
manifest.json, which doesn't support ES6 modules and will throw an error before
the chrome.runtime.onMessage listener and globalThis.TabTwinOnboarding
assignments execute. Remove the export keywords from the function declarations
analyzePageStructure and createFallbackOnboarding since these functions are
already being registered globally via globalThis.TabTwinOnboarding and the
chrome.runtime.onMessage listener, making the export statements unnecessary and
breaking the content script.
Comment on lines
+1
to
+50
| function createFallbackOnboarding(summary = {}) { | ||
| const title = String(summary.title || 'this page').trim(); | ||
| return { | ||
| welcomeMessage: `Welcome! ${title ? `You are viewing ${title}.` : 'You are viewing a new page.'}`, | ||
| pageOverview: 'The page is ready for a quick walkthrough. Focus on the main sections and the primary actions first.', | ||
| importantRegions: [], | ||
| recommendedActions: ['Review the main navigation.', 'Check the primary call to action.', 'Take the next step with confidence.'], | ||
| walkthrough: 'Begin by reviewing the visible navigation and the most important action on the page.' | ||
| }; | ||
| } | ||
|
|
||
| export async function generateOnboardingGuidance({ summary, apiKey }) { | ||
| if (!apiKey) { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch('https://api.anthropic.com/v1/messages', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'x-api-key': apiKey, | ||
| 'anthropic-version': '2023-06-01' | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: 'claude-sonnet-4-20250514', | ||
| max_tokens: 700, | ||
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | ||
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | ||
| }) | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
|
|
||
| const body = await response.json(); | ||
| const text = body.content?.find((part) => part.type === 'text')?.text || '{}'; | ||
| const parsed = JSON.parse(text); | ||
| return { | ||
| welcomeMessage: String(parsed.welcomeMessage || 'Welcome!'), | ||
| pageOverview: String(parsed.pageOverview || 'This page contains a few key areas to review.'), | ||
| importantRegions: Array.isArray(parsed.importantRegions) ? parsed.importantRegions : [], | ||
| recommendedActions: Array.isArray(parsed.recommendedActions) ? parsed.recommendedActions : [], | ||
| walkthrough: String(parsed.walkthrough || 'Start by scanning the main navigation and primary action.') | ||
| }; | ||
| } catch { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "onboardingService" --type=ts --type=js -C2
rg -n "from ['\"].*ai/onboardingService" -C2Repository: itzzavdhesh/TabTwin
Length of output: 204
Remove unused duplicate onboarding logic. server/ai/onboardingService.ts duplicates extension/onboarding/onboardingService.js, with the fallback text already diverged. No code imports or calls this server module—codebase searches found zero references. Delete this file and have any future server-side needs import the extension version or extract a shared module, eliminating maintenance burden of keeping two copies in sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/ai/onboardingService.ts` around lines 1 - 50, The server-side
onboarding implementation is a duplicate of the extension onboarding service and
is no longer referenced anywhere, so remove the unused
`generateOnboardingGuidance`/`createFallbackOnboarding` module entirely. If any
future server-side caller needs this behavior, point it to the existing
`extension/onboarding/onboardingService.js` logic or extract a shared helper
instead of keeping two copies in sync.
Comment on lines
+12
to
+50
| export async function generateOnboardingGuidance({ summary, apiKey }) { | ||
| if (!apiKey) { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch('https://api.anthropic.com/v1/messages', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'x-api-key': apiKey, | ||
| 'anthropic-version': '2023-06-01' | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: 'claude-sonnet-4-20250514', | ||
| max_tokens: 700, | ||
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | ||
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | ||
| }) | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
|
|
||
| const body = await response.json(); | ||
| const text = body.content?.find((part) => part.type === 'text')?.text || '{}'; | ||
| const parsed = JSON.parse(text); | ||
| return { | ||
| welcomeMessage: String(parsed.welcomeMessage || 'Welcome!'), | ||
| pageOverview: String(parsed.pageOverview || 'This page contains a few key areas to review.'), | ||
| importantRegions: Array.isArray(parsed.importantRegions) ? parsed.importantRegions : [], | ||
| recommendedActions: Array.isArray(parsed.recommendedActions) ? parsed.recommendedActions : [], | ||
| walkthrough: String(parsed.walkthrough || 'Start by scanning the main navigation and primary action.') | ||
| }; | ||
| } catch { | ||
| return createFallbackOnboarding(summary); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on the Anthropic fetch call.
If the request hangs (network stall, slow endpoint), this await fetch(...) can block indefinitely since there's no AbortController/timeout, delaying whatever calls this function with no bound.
⏱️ Add a timeout via AbortController
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 8000);
const response = await fetch('https://api.anthropic.com/v1/messages', {
method: 'POST',
+ signal: controller.signal,
headers: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function generateOnboardingGuidance({ summary, apiKey }) { | |
| if (!apiKey) { | |
| return createFallbackOnboarding(summary); | |
| } | |
| try { | |
| const response = await fetch('https://api.anthropic.com/v1/messages', { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'x-api-key': apiKey, | |
| 'anthropic-version': '2023-06-01' | |
| }, | |
| body: JSON.stringify({ | |
| model: 'claude-sonnet-4-20250514', | |
| max_tokens: 700, | |
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | |
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | |
| }) | |
| }); | |
| if (!response.ok) { | |
| return createFallbackOnboarding(summary); | |
| } | |
| const body = await response.json(); | |
| const text = body.content?.find((part) => part.type === 'text')?.text || '{}'; | |
| const parsed = JSON.parse(text); | |
| return { | |
| welcomeMessage: String(parsed.welcomeMessage || 'Welcome!'), | |
| pageOverview: String(parsed.pageOverview || 'This page contains a few key areas to review.'), | |
| importantRegions: Array.isArray(parsed.importantRegions) ? parsed.importantRegions : [], | |
| recommendedActions: Array.isArray(parsed.recommendedActions) ? parsed.recommendedActions : [], | |
| walkthrough: String(parsed.walkthrough || 'Start by scanning the main navigation and primary action.') | |
| }; | |
| } catch { | |
| return createFallbackOnboarding(summary); | |
| } | |
| } | |
| export async function generateOnboardingGuidance({ summary, apiKey }) { | |
| if (!apiKey) { | |
| return createFallbackOnboarding(summary); | |
| } | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 8000); | |
| const response = await fetch('https://api.anthropic.com/v1/messages', { | |
| method: 'POST', | |
| signal: controller.signal, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'x-api-key': apiKey, | |
| 'anthropic-version': '2023-06-01' | |
| }, | |
| body: JSON.stringify({ | |
| model: 'claude-sonnet-4-20250514', | |
| max_tokens: 700, | |
| system: 'You are TabTwin onboarding assistant. Return compact JSON with welcomeMessage, pageOverview, importantRegions, recommendedActions, walkthrough. Limit the whole response to about 200 words and avoid sensitive details.', | |
| messages: [{ role: 'user', content: JSON.stringify(summary) }] | |
| }) | |
| }); | |
| if (!response.ok) { | |
| return createFallbackOnboarding(summary); | |
| } | |
| const body = await response.json(); | |
| const text = body.content?.find((part) => part.type === 'text')?.text || '{}'; | |
| const parsed = JSON.parse(text); | |
| return { | |
| welcomeMessage: String(parsed.welcomeMessage || 'Welcome!'), | |
| pageOverview: String(parsed.pageOverview || 'This page contains a few key areas to review.'), | |
| importantRegions: Array.isArray(parsed.importantRegions) ? parsed.importantRegions : [], | |
| recommendedActions: Array.isArray(parsed.recommendedActions) ? parsed.recommendedActions : [], | |
| walkthrough: String(parsed.walkthrough || 'Start by scanning the main navigation and primary action.') | |
| }; | |
| } catch { | |
| return createFallbackOnboarding(summary); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/ai/onboardingService.ts` around lines 12 - 50, The Anthropic request
in generateOnboardingGuidance can hang indefinitely because the fetch call has
no timeout. Update this function to use an AbortController (or equivalent
timeout wrapper) around the fetch to https://api.anthropic.com/v1/messages,
abort the request after a reasonable duration, and ensure the existing fallback
path is still returned on timeout or abort. Keep the fix localized to
generateOnboardingGuidance and its fetch handling.
Comment on lines
131
to
137
| function leave() { | ||
| recorderRef.current?.stop(); | ||
| const exported = recorderRef.current?.exportTimeline() ?? []; | ||
| setRecording(exported.length ? { sessionId, events: exported } : null); | ||
| socketRef.current?.close(); | ||
| window.location.href = '/'; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exported recording is discarded before it can ever be used.
leave() computes exported/calls setRecording(...) and then immediately does a hard navigation (window.location.href = '/'). That unmounts/destroys the whole React app before the updated recording state can be rendered anywhere, so a guest who ends their session via "leave" while recording was on loses the entire timeline — despite the UI copy in Session.jsx promising it stays "available for playback after the session ends."
🛠️ Suggested stopgap: persist before navigating away
function leave() {
recorderRef.current?.stop();
const exported = recorderRef.current?.exportTimeline() ?? [];
- setRecording(exported.length ? { sessionId, events: exported } : null);
+ if (exported.length) {
+ sessionStorage.setItem(`recording:${sessionId}`, JSON.stringify({ sessionId, events: exported }));
+ }
socketRef.current?.close();
window.location.href = '/';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function leave() { | |
| recorderRef.current?.stop(); | |
| const exported = recorderRef.current?.exportTimeline() ?? []; | |
| setRecording(exported.length ? { sessionId, events: exported } : null); | |
| socketRef.current?.close(); | |
| window.location.href = '/'; | |
| } | |
| function leave() { | |
| recorderRef.current?.stop(); | |
| const exported = recorderRef.current?.exportTimeline() ?? []; | |
| if (exported.length) { | |
| sessionStorage.setItem(`recording:${sessionId}`, JSON.stringify({ sessionId, events: exported })); | |
| } | |
| socketRef.current?.close(); | |
| window.location.href = '/'; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 133-133: React's useState should not be directly called
Context: setRecording(exported.length ? { sessionId, events: exported } : null)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/hooks/useSession.js` around lines 131 - 137, The leave() flow in
useSession is discarding the exported timeline because it sets recording state
and then immediately hard-navigates away, unmounting the app before Session.jsx
can use it. Update leave() so the timeline exported from
recorderRef.current.exportTimeline() is persisted somewhere that survives
navigation, and only then close the socket and redirect; keep the recording
accessible through the session end/playback path rather than relying on
transient React state alone.
Comment on lines
+40
to
+49
| useEffect(() => { | ||
| if (!enabled) { | ||
| setActiveStep(0); | ||
| setDismissed(false); | ||
| return; | ||
| } | ||
|
|
||
| setActiveStep(0); | ||
| setDismissed(false); | ||
| }, [enabled]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Redundant branching; guidance changes while enabled don't reset step.
Both branches of the if (!enabled) check perform the identical reset (setActiveStep(0); setDismissed(false);), making the conditional dead weight — it can be simplified to an unconditional reset. More importantly, the effect only depends on [enabled], so if a new guidance payload arrives while enabled stays true (e.g., host retriggers onboarding for the same still-connected guest), activeStep/dismissed won't reset even though steps (derived from guidance via useMemo) changes underneath it — potentially leaving activeStep pointing at a stale/mismatched step.
💡 Suggested fix
useEffect(() => {
- if (!enabled) {
- setActiveStep(0);
- setDismissed(false);
- return;
- }
-
setActiveStep(0);
setDismissed(false);
- }, [enabled]);
+ }, [enabled, guidance]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!enabled) { | |
| setActiveStep(0); | |
| setDismissed(false); | |
| return; | |
| } | |
| setActiveStep(0); | |
| setDismissed(false); | |
| }, [enabled]); | |
| useEffect(() => { | |
| setActiveStep(0); | |
| setDismissed(false); | |
| }, [enabled, guidance]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/onboarding/useOnboarding.js` around lines 40 - 49, The reset logic
in useOnboarding is redundant and incomplete: both branches of the enabled check
do the same thing, and the effect in useEffect only reacts to enabled changes.
Simplify the branch to a single reset, and update the effect dependencies so
that when guidance changes in useMemo-derived steps while enabled remains true,
activeStep and dismissed are reset too. Use the existing useOnboarding,
useEffect, and steps identifiers to locate the reset behavior.
Comment on lines
+28
to
+32
| const { steps, activeStep, currentStep, dismissed, next, previous, skip, finish } = useOnboarding({ | ||
| enabled: Boolean(session.onboarding?.enabled && session.onboarding?.guidance), | ||
| guidance: session.onboarding?.guidance, | ||
| onFinish: session.clearOnboarding | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect createOnboardingSteps to see if it returns non-empty steps for empty guidance.
fd -i onboarding.js webapp/src/onboarding | xargs -I{} ast-grep run --pattern 'function createOnboardingSteps($_) { $$$ }' --lang javascript {}
rg -n -A 20 'function createOnboardingSteps' webapp/src/onboardingRepository: itzzavdhesh/TabTwin
Length of output: 4522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' webapp/src/pages/Session.jsx
printf '\n---\n'
sed -n '1,220p' webapp/src/onboarding/useOnboarding.jsRepository: itzzavdhesh/TabTwin
Length of output: 10197
Gate onboarding UI on session.onboarding?.enabled
useOnboarding() always builds default steps, so currentStep can stay truthy even when onboarding is disabled. Add the enabled check to the render path in Session.jsx so guests don’t see onboarding when the host has it off.
Proposed fix
- {!dismissed && currentStep ? (
+ {session.onboarding?.enabled && !dismissed && currentStep ? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/pages/Session.jsx` around lines 28 - 32, Gate the onboarding UI in
Session.jsx on session.onboarding?.enabled, because useOnboarding() can still
produce a truthy currentStep even when onboarding is disabled. Update the render
logic around the useOnboarding result so the onboarding elements only appear
when onboarding is explicitly enabled, and keep using the existing useOnboarding
hook and currentStep/activeStep checks as the location to apply the guard.
| </div> | ||
| <SessionStatus status={session.status} label={session.statusLabel} /> | ||
| <div className="flex items-center gap-3"> | ||
| <RecordingBadge enabled={recordingEnabled} state={playbackState === 'playing' ? 'playback' : recordingEnabled ? 'recording' : 'idle'} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Recording badge disappears while playback is paused.
RecordingBadge's state prop is only 'playback' when playbackState === 'playing'; when the user pauses (PlaybackEngine.pause() sets state = 'paused' and calls notifyProgress()), playbackState becomes 'paused', so the computed prop falls through to recordingEnabled ? 'recording' : 'idle'. Since recordingEnabled is false whenever session.recording is populated (playback available), the badge silently disappears mid-pause, which is confusing since playback is still loaded/active.
🔧 Proposed fix
- <RecordingBadge enabled={recordingEnabled} state={playbackState === 'playing' ? 'playback' : recordingEnabled ? 'recording' : 'idle'} />
+ <RecordingBadge enabled={recordingEnabled} state={playbackState === 'playing' || playbackState === 'paused' ? 'playback' : recordingEnabled ? 'recording' : 'idle'} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <RecordingBadge enabled={recordingEnabled} state={playbackState === 'playing' ? 'playback' : recordingEnabled ? 'recording' : 'idle'} /> | |
| <RecordingBadge enabled={recordingEnabled} state={playbackState === 'playing' || playbackState === 'paused' ? 'playback' : recordingEnabled ? 'recording' : 'idle'} /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/pages/Session.jsx` at line 78, The RecordingBadge state logic in
Session.jsx drops out of playback when playbackState becomes paused, causing the
badge to disappear during paused playback. Update the state computation for
RecordingBadge so it treats both 'playing' and 'paused' as playback-related
states (while keeping the existing idle/recording behavior), using the
playbackState and recordingEnabled values already in Session.jsx and the
RecordingBadge prop.
Comment on lines
+13
to
+24
| start() { | ||
| if (!this.enabled) return; | ||
| if (this.isRecording) return; | ||
|
|
||
| this.isRecording = true; | ||
| this.sessionStartedAt = this.sessionStartedAt ?? Date.now(); | ||
| this.lastCursorEvent = null; | ||
| const hasStartEvent = this.timeline.some((event) => event.eventType === 'session:start'); | ||
| if (!hasStartEvent) { | ||
| this.capture({ type: 'session:start', payload: { startedAt: this.sessionStartedAt }, participantId: this.participantId, timestamp: this.sessionStartedAt }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
start() doesn't reset the timeline after a prior stop(), corrupting re-recorded sessions.
hasStartEvent only checks that some session:start exists anywhere in this.timeline, not whether the previous recording was closed with session:end. Since useSession.js's recording-toggle effect calls stop()/start() on every enable/disable cycle without ever calling clear(), toggling recording off then on again produces a timeline shaped like [session:start, …, session:end, …more events, (no new session:start)]. That stray mid-timeline session:end corrupts the exported recording's semantics for playback (e.g., PlaybackEngine.getDuration() just reads the last event, silently ignoring the earlier "end").
🛠️ Proposed fix: clear stale timeline before starting a new recording session
start() {
if (!this.enabled) return;
if (this.isRecording) return;
this.isRecording = true;
this.sessionStartedAt = this.sessionStartedAt ?? Date.now();
this.lastCursorEvent = null;
- const hasStartEvent = this.timeline.some((event) => event.eventType === 'session:start');
- if (!hasStartEvent) {
+ const hasOpenSession = this.timeline.some((event) => event.eventType === 'session:start')
+ && !this.timeline.some((event) => event.eventType === 'session:end');
+ if (!hasOpenSession) {
+ this.timeline = [];
+ this.sessionStartedAt = Date.now();
this.capture({ type: 'session:start', payload: { startedAt: this.sessionStartedAt }, participantId: this.participantId, timestamp: this.sessionStartedAt });
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| start() { | |
| if (!this.enabled) return; | |
| if (this.isRecording) return; | |
| this.isRecording = true; | |
| this.sessionStartedAt = this.sessionStartedAt ?? Date.now(); | |
| this.lastCursorEvent = null; | |
| const hasStartEvent = this.timeline.some((event) => event.eventType === 'session:start'); | |
| if (!hasStartEvent) { | |
| this.capture({ type: 'session:start', payload: { startedAt: this.sessionStartedAt }, participantId: this.participantId, timestamp: this.sessionStartedAt }); | |
| } | |
| } | |
| start() { | |
| if (!this.enabled) return; | |
| if (this.isRecording) return; | |
| this.isRecording = true; | |
| this.sessionStartedAt = this.sessionStartedAt ?? Date.now(); | |
| this.lastCursorEvent = null; | |
| const hasOpenSession = this.timeline.some((event) => event.eventType === 'session:start') | |
| && !this.timeline.some((event) => event.eventType === 'session:end'); | |
| if (!hasOpenSession) { | |
| this.timeline = []; | |
| this.sessionStartedAt = Date.now(); | |
| this.capture({ type: 'session:start', payload: { startedAt: this.sessionStartedAt }, participantId: this.participantId, timestamp: this.sessionStartedAt }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/recording/SessionRecorder.js` around lines 13 - 24,
`SessionRecorder.start()` is reusing old timeline data after a previous
`stop()`, so a new recording can start with stale `session:end` events still
present. Update the `start()` flow to reset or clear the recorder state before
beginning a fresh session, and make sure the `session:start`/`session:end`
sequence in `capture()` remains valid across repeated enable/disable cycles
triggered by `useSession.js`.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
webapp/src/pages/Session.jsx (1)
64-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFocus the accessible onboarding panel, not an unlabeled wrapper.
panelRef.current.focus()targets a generic<div>, while the labeled<section>is its child. Give the focus target an accessible region/dialog name or forward the ref toOnboardingPanelso assistive technologies announce the newly displayed step.Also applies to: 138-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/pages/Session.jsx` around lines 64 - 68, Update the focus logic in the useEffect for the onboarding panel so panelRef targets the labeled focusable section or OnboardingPanel rather than the generic wrapper div. Ensure the focused element has an accessible region/dialog name, and preserve focusing on dismissed or activeStep changes.extension/popup/Popup.jsx (2)
39-52: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake
send()reject Chrome runtime errors.The new
catchonly handles synchronous throws; asynchronouschrome.runtime.lastErroris currently resolved as an undefined response bysend(). InspectlastErrorin the message callback and reject so the intended error path is reliable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/popup/Popup.jsx` around lines 39 - 52, Update the send function’s Chrome message callback to inspect chrome.runtime.lastError and reject with that error when present, rather than resolving an undefined response. Preserve normal response resolution so Popup’s session-start catch block reliably handles runtime failures.
100-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAnnounce startup errors to assistive technology.
Add
role="alert"oraria-live="polite"so screen-reader users are notified when session startup fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/popup/Popup.jsx` around lines 100 - 102, Update the error message paragraph in the Popup component’s startup-error rendering to include an assistive-technology announcement mechanism, using either role="alert" or aria-live="polite" while preserving the existing conditional rendering and styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@extension/popup/Popup.jsx`:
- Around line 39-52: Update the send function’s Chrome message callback to
inspect chrome.runtime.lastError and reject with that error when present, rather
than resolving an undefined response. Preserve normal response resolution so
Popup’s session-start catch block reliably handles runtime failures.
- Around line 100-102: Update the error message paragraph in the Popup
component’s startup-error rendering to include an assistive-technology
announcement mechanism, using either role="alert" or aria-live="polite" while
preserving the existing conditional rendering and styling.
In `@webapp/src/pages/Session.jsx`:
- Around line 64-68: Update the focus logic in the useEffect for the onboarding
panel so panelRef targets the labeled focusable section or OnboardingPanel
rather than the generic wrapper div. Ensure the focused element has an
accessible region/dialog name, and preserve focusing on dismissed or activeStep
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37afe084-b38e-4578-8bcf-2c9410e34dfb
📒 Files selected for processing (5)
extension/background/serviceWorker.jsextension/popup/Popup.jsxserver/signalingHandler.jswebapp/src/hooks/useSession.jswebapp/src/pages/Session.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- extension/background/serviceWorker.js
- server/signalingHandler.js
- webapp/src/hooks/useSession.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 Program
SSOC
📝 Description
This PR implements AI-Guided Onboarding for New Guests to provide contextual assistance when a guest joins a collaboration session.
The feature helps new participants quickly understand the current webpage by analyzing the visible page structure and generating an interactive onboarding experience. It is completely optional, enabled by the host, and designed to integrate seamlessly with TabTwin's existing collaboration workflow without affecting current functionality.
✨ Features Added
Added an AI-powered onboarding system for newly joined guests.
Host-controlled Enable AI Onboarding toggle (disabled by default).
Analyzes the visible webpage structure while avoiding sensitive information.
Generates contextual onboarding guidance with AI and falls back to default guidance if AI is unavailable.
Automatically identifies important UI regions such as:
Reuses the existing annotation overlay to highlight important page elements.
Added interactive onboarding components:
OnboardingPanelOnboardingTooltipWalkthroughControlsWalkthrough supports:
Added keyboard accessibility and automatic highlight cleanup.
Included unit tests for onboarding functionality.
Updated documentation.
🔗 Related Issue
Closes #64
🔄 Type of Change
🧪 How to Test
Clone the repository and install dependencies.
Configure the required environment variables (including the AI provider API key if testing AI-generated guidance).
Start the signaling server.
Start the guest web application.
Load the Chrome extension.
Enable AI Onboarding from the extension settings.
Start a collaboration session and share the session link.
Join the session as a guest.
Verify that:
Disable AI Onboarding and verify that guests join without the onboarding flow.
✅ Checklist
feat: add annotation tools)Summary by cubic
Adds host-enabled onboarding for new guests and optional session recording. Implements issue #64 with both features opt-in and isolated from live collaboration.
New Features
onboarding:guidancethrough signaling, and briefly highlightsimportantRegions.useOnboarding,OnboardingPanel,OnboardingTooltip, andWalkthroughControlsintoSession.jsxwith Next/Previous/Skip/Finish and keyboard support; clears state on finish.Migration
Written for commit 93d03ca. Summary will update on new commits.
Summary by CodeRabbit