Skip to content

feat: faster screenshot output preview data #659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 29, 2025
Merged

feat: faster screenshot output preview data #659

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
09064e9
feat: add update screenshot step data
RohitR311 Jun 25, 2025
1a6dc32
feat: emit direct ss capture socket
RohitR311 Jun 25, 2025
6768dda
feat: rm socket logic, add ss data
RohitR311 Jun 25, 2025
d5f5440
feat: add on preview click
RohitR311 Jun 25, 2025
9512433
feat: add on preview complete
RohitR311 Jun 25, 2025
22c043c
feat: register capture direct ss socket event
RohitR311 Jun 25, 2025
a4897d0
feat: enforce pointer cursor
RohitR311 Jun 25, 2025
390c5f2
Merge branch 'develop' into out-ss
amhsirak Jun 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions server/src/browser-management/classes/RemoteBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,63 @@ export class RemoteBrowser {
) as Array<Record<string, string>>;
}

/**
* Captures a screenshot directly without running the workflow interpreter
* @param settings Screenshot settings containing fullPage, type, etc.
* @returns Promise<void>
*/
public captureDirectScreenshot = async (settings: {
fullPage: boolean;
type: 'png' | 'jpeg';
timeout?: number;
animations?: 'disabled' | 'allow';
caret?: 'hide' | 'initial';
scale?: 'css' | 'device';
}): Promise<void> => {
Comment on lines +1874 to +1881
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation for screenshot settings.

The method accepts settings from the client without validation. Malicious or invalid values could cause issues.

 public captureDirectScreenshot = async (settings: {
   fullPage: boolean;
   type: 'png' | 'jpeg';
   timeout?: number;
   animations?: 'disabled' | 'allow';
   caret?: 'hide' | 'initial';
   scale?: 'css' | 'device';
 }): Promise<void> => {
+  // Validate settings
+  if (settings.timeout && (settings.timeout < 0 || settings.timeout > 60000)) {
+    logger.error("Invalid timeout value");
+    this.socket.emit('screenshotError', {
+      userId: this.userId,
+      error: 'Invalid timeout value (must be 0-60000ms)'
+    });
+    return;
+  }
+
+  if (!['png', 'jpeg'].includes(settings.type)) {
+    logger.error("Invalid image type");
+    this.socket.emit('screenshotError', {
+      userId: this.userId,
+      error: 'Invalid image type (must be png or jpeg)'
+    });
+    return;
+  }
📝 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
public captureDirectScreenshot = async (settings: {
fullPage: boolean;
type: 'png' | 'jpeg';
timeout?: number;
animations?: 'disabled' | 'allow';
caret?: 'hide' | 'initial';
scale?: 'css' | 'device';
}): Promise<void> => {
public captureDirectScreenshot = async (settings: {
fullPage: boolean;
type: 'png' | 'jpeg';
timeout?: number;
animations?: 'disabled' | 'allow';
caret?: 'hide' | 'initial';
scale?: 'css' | 'device';
}): Promise<void> => {
// Validate settings
if (settings.timeout && (settings.timeout < 0 || settings.timeout > 60000)) {
logger.error("Invalid timeout value");
this.socket.emit('screenshotError', {
userId: this.userId,
error: 'Invalid timeout value (must be 0-60000ms)'
});
return;
}
if (!['png', 'jpeg'].includes(settings.type)) {
logger.error("Invalid image type");
this.socket.emit('screenshotError', {
userId: this.userId,
error: 'Invalid image type (must be png or jpeg)'
});
return;
}
// …rest of existing implementation…
🤖 Prompt for AI Agents 
In server/src/browser-management/classes/RemoteBrowser.ts around lines 1874 to
1881, the captureDirectScreenshot method accepts settings without validation,
which risks processing invalid or malicious inputs. Add input validation to
check that each setting property (fullPage, type, timeout, animations, caret,
scale) is of the expected type and within allowed values before proceeding. If
any validation fails, throw an appropriate error or reject the input to prevent
further processing.

if (!this.currentPage) {
logger.error("No current page available for screenshot");
this.socket.emit('screenshotError', {
userId: this.userId,
error: 'No active page available'
});
return;
}

try {
this.socket.emit('screenshotCaptureStarted', {
userId: this.userId,
fullPage: settings.fullPage
});

const screenshotBuffer = await this.currentPage.screenshot({
fullPage: settings.fullPage,
type: settings.type || 'png',
timeout: settings.timeout || 30000,
animations: settings.animations || 'allow',
caret: settings.caret || 'hide',
scale: settings.scale || 'device'
});

const base64Data = screenshotBuffer.toString('base64');
const mimeType = `image/${settings.type || 'png'}`;
const dataUrl = `data:${mimeType};base64,${base64Data}`;

this.socket.emit('directScreenshotCaptured', {
userId: this.userId,
screenshot: dataUrl,
mimeType: mimeType,
fullPage: settings.fullPage,
timestamp: Date.now()
});
} catch (error) {
logger.error('Failed to capture direct screenshot:', error);
this.socket.emit('screenshotError', {
userId: this.userId,
error: error instanceof Error ? error.message : 'Unknown error occurred'
});
}
};

/**
* Registers all event listeners needed for the recording editor session.
* Should be called only once after the full initialization of the remote browser.
Expand All @@ -1874,6 +1931,16 @@ export class RemoteBrowser {
public registerEditorEvents = (): void => {
// For each event, include userId to make sure events are handled for the correct browser
logger.log('debug', `Registering editor events for user: ${this.userId}`);

this.socket.on(`captureDirectScreenshot:${this.userId}`, async (settings) => {
logger.debug(`Direct screenshot capture requested for user ${this.userId}`);
await this.captureDirectScreenshot(settings);
});

// For backward compatibility
this.socket.on('captureDirectScreenshot', async (settings) => {
await this.captureDirectScreenshot(settings);
});
Comment on lines +1935 to +1943
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and rate limiting for screenshot events.

The event handlers lack explicit error handling and rate limiting, which could lead to resource abuse or unhandled errors.

+// Add rate limiting for screenshot captures
+private lastScreenshotTime = 0;
+private readonly SCREENSHOT_RATE_LIMIT = 1000; // 1 second

 this.socket.on(`captureDirectScreenshot:${this.userId}`, async (settings) => {
-  logger.debug(`Direct screenshot capture requested for user ${this.userId}`);
-  await this.captureDirectScreenshot(settings);
+  try {
+    const now = Date.now();
+    if (now - this.lastScreenshotTime < this.SCREENSHOT_RATE_LIMIT) {
+      logger.warn(`Screenshot rate limit exceeded for user ${this.userId}`);
+      this.socket.emit('screenshotError', {
+        userId: this.userId,
+        error: 'Rate limit exceeded. Please wait before requesting another screenshot.'
+      });
+      return;
+    }
+    this.lastScreenshotTime = now;
+    
+    logger.debug(`Direct screenshot capture requested for user ${this.userId}`);
+    await this.captureDirectScreenshot(settings);
+  } catch (error) {
+    logger.error(`Error handling screenshot request for user ${this.userId}:`, error);
+    this.socket.emit('screenshotError', {
+      userId: this.userId,
+      error: 'Failed to process screenshot request'
+    });
+  }
 });

 // For backward compatibility
 this.socket.on('captureDirectScreenshot', async (settings) => {
-  await this.captureDirectScreenshot(settings);
+  try {
+    const now = Date.now();
+    if (now - this.lastScreenshotTime < this.SCREENSHOT_RATE_LIMIT) {
+      logger.warn(`Screenshot rate limit exceeded`);
+      this.socket.emit('screenshotError', {
+        userId: this.userId,
+        error: 'Rate limit exceeded. Please wait before requesting another screenshot.'
+      });
+      return;
+    }
+    this.lastScreenshotTime = now;
+    
+    await this.captureDirectScreenshot(settings);
+  } catch (error) {
+    logger.error(`Error handling backward compatibility screenshot request:`, error);
+    this.socket.emit('screenshotError', {
+      userId: this.userId,
+      error: 'Failed to process screenshot request'
+    });
+  }
 });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In server/src/browser-management/classes/RemoteBrowser.ts around lines 1935 to
1943, the event handlers for 'captureDirectScreenshot' lack error handling and
rate limiting. Wrap the handler logic in try-catch blocks to catch and log any
errors during screenshot capture. Implement rate limiting to restrict how
frequently a user can trigger these events, preventing resource abuse. Ensure
both the user-specific and backward-compatible event handlers include these
protections.


// Listen for specific events for this user
this.socket.on(`rerender:${this.userId}`, async () => {
Expand Down
4 changes: 2 additions & 2 deletions src/components/recorder/DOMBrowserRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ export const DOMBrowserRenderer: React.FC<RRWebDOMBrowserRendererProps> = ({

/* Make everything interactive */
* {
cursor: ${isInCaptureMode ? "crosshair" : "pointer"} !important;
cursor: "pointer" !important;
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider restoring conditional cursor styling for better UX.

The hardcoded "pointer" cursor removes the visual distinction between normal navigation and capture mode. Users typically expect a crosshair cursor when selecting elements (isInCaptureMode = true) to clearly indicate the interaction mode.

Consider reverting to conditional cursor styling:

-              cursor: "pointer" !important; 
+              cursor: ${isInCaptureMode ? '"crosshair"' : '"pointer"'} !important;
📝 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
cursor: "pointer" !important;
cursor: ${isInCaptureMode ? '"crosshair"' : '"pointer"'} !important;
🤖 Prompt for AI Agents 
In src/components/recorder/DOMBrowserRenderer.tsx at line 859, the cursor style
is hardcoded to "pointer" which removes the visual distinction between normal
navigation and capture mode. Modify the cursor style to be conditional based on
the isInCaptureMode state, setting it to "crosshair" when isInCaptureMode is
true and "pointer" otherwise, to improve user experience by clearly indicating
the interaction mode.

}

/* Additional CSS from resources */
Expand Down Expand Up @@ -1127,7 +1127,7 @@ export const DOMBrowserRenderer: React.FC<RRWebDOMBrowserRendererProps> = ({
left: 0,
right: 0,
bottom: 0,
cursor: "pointer",
cursor: "pointer !important",
pointerEvents: "none",
zIndex: 999,
borderRadius: "0px 0px 5px 5px",
Expand Down
36 changes: 30 additions & 6 deletions src/components/recorder/RightSidePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export const RightSidePanel: React.FC<RightSidePanelProps> = ({ onFinishCapture
startAction, finishAction
} = useActionContext();

const { browserSteps, updateBrowserTextStepLabel, deleteBrowserStep, addScreenshotStep, updateListTextFieldLabel, removeListTextField, updateListStepLimit, deleteStepsByActionId, updateListStepData } = useBrowserSteps();
const { browserSteps, updateBrowserTextStepLabel, deleteBrowserStep, addScreenshotStep, updateListTextFieldLabel, removeListTextField, updateListStepLimit, deleteStepsByActionId, updateListStepData, updateScreenshotStepData } = useBrowserSteps();
const { id, socket } = useSocketStore();
const { t } = useTranslation();

Expand Down Expand Up @@ -183,6 +183,29 @@ export const RightSidePanel: React.FC<RightSidePanelProps> = ({ onFinishCapture
};
}, [socket, updateListStepData, isDOMMode]);

useEffect(() => {
if (socket) {
const handleDirectScreenshot = (data: any) => {
const screenshotSteps = browserSteps.filter(step =>
step.type === 'screenshot' && step.actionId === currentScreenshotActionId
);

if (screenshotSteps.length > 0) {
const latestStep = screenshotSteps[screenshotSteps.length - 1];
updateScreenshotStepData(latestStep.id, data.screenshot);
}

setCurrentScreenshotActionId('');
};

socket.on('directScreenshotCaptured', handleDirectScreenshot);

return () => {
socket.off('directScreenshotCaptured', handleDirectScreenshot);
};
}
}, [socket, id, notify, t, currentScreenshotActionId, updateScreenshotStepData, setCurrentScreenshotActionId]);

Comment on lines +186 to +208
Copy link

@coderabbitai coderabbitai bot Jun 25, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize dependencies and add error handling.

The socket event handling logic is correct, but consider these improvements:

  1. Remove unused dependencies - id, notify, and t are in the dependency array but not used in the effect, causing unnecessary re-renders.
  2. Add error handling for potentially malformed socket data.

Apply this diff to optimize the effect:

  useEffect(() => {
    if (socket) {
      const handleDirectScreenshot = (data: any) => {
+       try {
+         if (!data?.screenshot) {
+           console.warn('Invalid screenshot data received:', data);
+           return;
+         }
+
          const screenshotSteps = browserSteps.filter(step => 
            step.type === 'screenshot' && step.actionId === currentScreenshotActionId
          );
          
          if (screenshotSteps.length > 0) {
            const latestStep = screenshotSteps[screenshotSteps.length - 1];          
            updateScreenshotStepData(latestStep.id, data.screenshot);
          }
          
          setCurrentScreenshotActionId('');
+       } catch (error) {
+         console.error('Error handling direct screenshot:', error);
+       }
      };

      socket.on('directScreenshotCaptured', handleDirectScreenshot);

      return () => {
        socket.off('directScreenshotCaptured', handleDirectScreenshot);
      };
    }
-  }, [socket, id, notify, t, currentScreenshotActionId, updateScreenshotStepData, setCurrentScreenshotActionId]);
+  }, [socket, currentScreenshotActionId, updateScreenshotStepData, setCurrentScreenshotActionId, browserSteps]);
📝 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 (socket) {
const handleDirectScreenshot = (data: any) => {
const screenshotSteps = browserSteps.filter(step =>
step.type === 'screenshot' && step.actionId === currentScreenshotActionId
);
if (screenshotSteps.length > 0) {
const latestStep = screenshotSteps[screenshotSteps.length - 1];
updateScreenshotStepData(latestStep.id, data.screenshot);
}
setCurrentScreenshotActionId('');
};
socket.on('directScreenshotCaptured', handleDirectScreenshot);
return () => {
socket.off('directScreenshotCaptured', handleDirectScreenshot);
};
}
}, [socket, id, notify, t, currentScreenshotActionId, updateScreenshotStepData, setCurrentScreenshotActionId]);
useEffect(() => {
if (socket) {
const handleDirectScreenshot = (data: any) => {
try {
if (!data?.screenshot) {
console.warn('Invalid screenshot data received:', data);
return;
}
const screenshotSteps = browserSteps.filter(step =>
step.type === 'screenshot' && step.actionId === currentScreenshotActionId
);
if (screenshotSteps.length > 0) {
const latestStep = screenshotSteps[screenshotSteps.length - 1];
updateScreenshotStepData(latestStep.id, data.screenshot);
}
setCurrentScreenshotActionId('');
} catch (error) {
console.error('Error handling direct screenshot:', error);
}
};
socket.on('directScreenshotCaptured', handleDirectScreenshot);
return () => {
socket.off('directScreenshotCaptured', handleDirectScreenshot);
};
}
}, [
socket,
currentScreenshotActionId,
updateScreenshotStepData,
setCurrentScreenshotActionId,
browserSteps,
]);
🤖 Prompt for AI Agents 
In src/components/recorder/RightSidePanel.tsx around lines 186 to 208, remove
the unused dependencies 'id', 'notify', and 't' from the useEffect dependency
array to prevent unnecessary re-renders. Additionally, add error handling inside
the 'handleDirectScreenshot' function to safely handle cases where the socket
data might be malformed, such as checking if 'data' and 'data.screenshot' exist
before calling 'updateScreenshotStepData'.

const extractDataClientSide = useCallback(
(
listSelector: string,
Expand Down Expand Up @@ -649,14 +672,15 @@ export const RightSidePanel: React.FC<RightSidePanelProps> = ({ onFinishCapture
}, [currentListActionId, browserSteps, stopGetList, deleteStepsByActionId, resetListState, setShowPaginationOptions, setShowLimitOptions, setCaptureStage, notify, t]);

const captureScreenshot = (fullPage: boolean) => {
const screenshotSettings: ScreenshotSettings = {
const screenshotSettings = {
fullPage,
type: 'png',
type: 'png' as const,
timeout: 30000,
animations: 'allow',
caret: 'hide',
scale: 'device',
animations: 'allow' as const,
caret: 'hide' as const,
scale: 'device' as const,
};
socket?.emit('captureDirectScreenshot', screenshotSettings);
socket?.emit('action', { action: 'screenshot', settings: screenshotSettings });
addScreenshotStep(fullPage, currentScreenshotActionId);
stopGetScreenshot();
Expand Down
11 changes: 9 additions & 2 deletions src/components/recorder/SidePanelHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import React, { FC, useState } from 'react';
import { InterpretationButtons } from "../run/InterpretationButtons";
import { useSocketStore } from "../../context/socket";

export const SidePanelHeader = () => {
interface SidePanelHeaderProps {
onPreviewClick?: () => void;
}

export const SidePanelHeader = ({ onPreviewClick }: SidePanelHeaderProps) => {

const [steppingIsDisabled, setSteppingIsDisabled] = useState(true);

Expand All @@ -14,7 +18,10 @@ export const SidePanelHeader = () => {

return (
<div style={{ width: 'inherit' }}>
<InterpretationButtons enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)} />
<InterpretationButtons
enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)}
onPreviewComplete={onPreviewClick}
/>
Comment on lines +21 to +24
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the prop name mapping consistency.

The prop onPreviewClick is mapped to onPreviewComplete when passed to InterpretationButtons. While this works functionally, the naming suggests different semantic meanings - "click" vs "complete". Consider aligning the naming for better clarity.

🏁 Script executed:

#!/bin/bash
# Description: Check if onPreviewComplete is consistently used across the codebase
# Expected: Find usages to verify naming consistency

rg -A 3 -B 3 "onPreviewComplete|onPreviewClick"

Length of output: 3638

Align prop naming between SidePanelHeader and InterpretationButtons

To avoid semantic confusion, the “preview” callback should use the same name in both components. Right now:

  • InterpretationButtons defines its callback prop as onPreviewComplete.
  • SidePanelHeader exposes its callback as onPreviewClick and then maps it to onPreviewComplete.

Please consolidate on one name—preferably onPreviewComplete since the callback fires when the preview is done.

Changes needed:

  • In src/components/recorder/SidePanelHeader.tsx:
    • Rename the prop in the interface and component signature from onPreviewClick?: () => void to onPreviewComplete?: () => void.
    • Update the JSX that passes it down: 
      - <InterpretationButtons 
-   enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)} 
-   onPreviewComplete={onPreviewClick}
- />
+ <InterpretationButtons 
+   enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)} 
+   onPreviewComplete={onPreviewComplete}
+ />
  • In src/components/run/InterpretationLog.tsx:
    • Update the usage of SidePanelHeader: 
      - <SidePanelHeader onPreviewClick={() => setShowPreviewData(true)} />
+ <SidePanelHeader onPreviewComplete={() => setShowPreviewData(true)} />

With these updates, both components will share the same prop name and intent.

📝 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
<InterpretationButtons
enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)}
onPreviewComplete={onPreviewClick}
/>
<InterpretationButtons
enableStepping={(isPaused) => setSteppingIsDisabled(!isPaused)}
onPreviewComplete={onPreviewComplete}
/>
🤖 Prompt for AI Agents 
In src/components/recorder/SidePanelHeader.tsx around lines 21 to 24, rename the
prop and interface member from onPreviewClick to onPreviewComplete to align with
InterpretationButtons. Update the component signature and all references
accordingly, including the JSX where the prop is passed down. Then, in
src/components/run/InterpretationLog.tsx, update the usage of SidePanelHeader to
use onPreviewComplete instead of onPreviewClick to maintain consistency across
components.

{/* <Button
variant='outlined'
disabled={steppingIsDisabled}
Expand Down
27 changes: 16 additions & 11 deletions src/components/run/InterpretationButtons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { useTranslation } from "react-i18next";

interface InterpretationButtonsProps {
enableStepping: (isPaused: boolean) => void;
onPreviewComplete?: () => void;
}

interface InterpretationInfo {
Expand All @@ -22,7 +23,7 @@ const interpretationInfo: InterpretationInfo = {
isPaused: false,
};

export const InterpretationButtons = ({ enableStepping }: InterpretationButtonsProps) => {
export const InterpretationButtons = ({ enableStepping, onPreviewComplete }: InterpretationButtonsProps) => {
const { t } = useTranslation();
const [info, setInfo] = useState<InterpretationInfo>(interpretationInfo);
const [decisionModal, setDecisionModal] = useState<{
Expand Down Expand Up @@ -102,16 +103,20 @@ export const InterpretationButtons = ({ enableStepping }: InterpretationButtonsP
}, [socket, finishedHandler, breakpointHitHandler]);

const handlePlay = async () => {
if (!info.running) {
setInfo({ ...info, running: true });
const finished = await interpretCurrentRecording();
setInfo({ ...info, running: false });
if (finished) {
notify('info', t('interpretation_buttons.messages.run_finished'));
} else {
notify('error', t('interpretation_buttons.messages.run_failed'));
}
}
onPreviewComplete?.();
notify('info', t('interpretation_buttons.messages.run_finished'));
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update notification message for accuracy.

The notification message references "run_finished" but the operation is now a preview. This could confuse users about what actually happened.

Consider adding a more appropriate translation key like preview_completed or preview_ready.

🤖 Prompt for AI Agents 
In src/components/run/InterpretationButtons.tsx at line 107, the notification
message uses the translation key 'interpretation_buttons.messages.run_finished',
which inaccurately describes the operation as a run completion instead of a
preview. Update the translation key to a more appropriate one such as
'preview_completed' or 'preview_ready' to accurately reflect the preview
operation in the notification message.


// Legacy code for running the interpretation

// if (!info.running) {
// setInfo({ ...info, running: true });
// // const finished = await interpretCurrentRecording();
// setInfo({ ...info, running: false });
// if (finished) {
// } else {
// notify('error', t('interpretation_buttons.messages.run_failed'));
// }
// }
Comment on lines 105 to +119
Copy link

@coderabbitai coderabbitai bot Jun 28, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical issues with the simplified handlePlay logic.

The refactored handlePlay function has several concerning issues:

  1. Misleading user feedback: The notification shows "run finished" immediately, but no actual processing occurs
  2. Broken state management: The info.running state is never set to true, so the button disable logic won't work properly during async operations
  3. Missing error handling: The callback execution has no try-catch protection
  4. Inconsistent behavior: The button text still shows "preview" but the notification suggests a "run" completed

Consider this improved implementation:

 const handlePlay = async () => {
+  if (info.running) return;
+  
+  try {
+    setInfo({ ...info, running: true });
     onPreviewComplete?.();
-    notify('info', t('interpretation_buttons.messages.run_finished'));
+    notify('info', t('interpretation_buttons.messages.preview_ready'));
+  } catch (error) {
+    notify('error', t('interpretation_buttons.messages.preview_failed'));
+  } finally {
+    setInfo({ ...info, running: false });
+  }
📝 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 handlePlay = async () => {
if (!info.running) {
setInfo({ ...info, running: true });
const finished = await interpretCurrentRecording();
setInfo({ ...info, running: false });
if (finished) {
notify('info', t('interpretation_buttons.messages.run_finished'));
} else {
notify('error', t('interpretation_buttons.messages.run_failed'));
}
}
onPreviewComplete?.();
notify('info', t('interpretation_buttons.messages.run_finished'));
// Legacy code for running the interpretation
// if (!info.running) {
// setInfo({ ...info, running: true });
// // const finished = await interpretCurrentRecording();
// setInfo({ ...info, running: false });
// if (finished) {
// } else {
// notify('error', t('interpretation_buttons.messages.run_failed'));
// }
// }
const handlePlay = async () => {
if (info.running) return;
try {
setInfo({ ...info, running: true });
onPreviewComplete?.();
notify('info', t('interpretation_buttons.messages.preview_ready'));
} catch (error) {
notify('error', t('interpretation_buttons.messages.preview_failed'));
} finally {
setInfo({ ...info, running: false });
}
// Legacy code for running the interpretation
// if (!info.running) {
// setInfo({ ...info, running: true });
// // const finished = await interpretCurrentRecording();
// setInfo({ ...info, running: false });
// if (finished) {
// } else {
// notify('error', t('interpretation_buttons.messages.run_failed'));
// }
// }
};
🤖 Prompt for AI Agents 
In src/components/run/InterpretationButtons.tsx around lines 105 to 119, the
handlePlay function prematurely shows a "run finished" notification without
running any process, does not update the info.running state to true during
execution, lacks try-catch error handling, and causes inconsistent UI feedback.
To fix this, restore the logic to set info.running to true before starting the
async operation, wrap the async call in try-catch to handle errors and notify
accordingly, update info.running to false after completion, and ensure the
notification and button text accurately reflect the current state of the
operation.

};

// pause and stop logic (do not delete - we wil bring this back!)
Expand Down
Loading