refactor: move robot modals to legacy #820
Open
+1,245
−0
Conversation
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
legacy/src/RobotDuplicate.tsx(1 hunks)legacy/src/RobotEdit.tsx(1 hunks)legacy/src/RobotSettings.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
legacy/src/RobotSettings.tsx (3)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(183-183)src/api/storage.ts (1)
getStoredRecording(80-92)src/components/ui/GenericModal.tsx (1)
GenericModal(13-29)
legacy/src/RobotEdit.tsx (3)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(183-183)src/api/storage.ts (2)
getStoredRecording(80-92)updateRecording(31-48)src/components/ui/GenericModal.tsx (1)
GenericModal(13-29)
legacy/src/RobotDuplicate.tsx (3)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(183-183)src/api/storage.ts (2)
getStoredRecording(80-92)duplicateRecording(50-64)src/components/ui/GenericModal.tsx (1)
GenericModal(13-29)
🪛 ast-grep (0.39.5)
legacy/src/RobotDuplicate.tsx
[warning] 130-130: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
legacy/src/RobotDuplicate.tsx
[error] 131-131: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
Comment on lines
+56
to
+87
| export const RobotDuplicationModal = ({ isOpen, handleStart, handleClose, initialSettings }: RobotSettingsProps) => { | ||
| const { t } = useTranslation(); | ||
| const [targetUrl, setTargetUrl] = useState<string | undefined>(''); | ||
| const [robot, setRobot] = useState<RobotSettings | null>(null); | ||
| const { recordingId, notify, setRerenderRobots } = useGlobalInfoStore(); | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen) { | ||
| getRobot(); | ||
| } | ||
| }, [isOpen]); | ||
|
|
||
| useEffect(() => { | ||
| if (robot) { | ||
| const lastPair = robot?.recording.workflow[robot?.recording.workflow.length - 1]; | ||
| const url = lastPair?.what.find(action => action.action === "goto")?.args?.[0]; | ||
| setTargetUrl(url); | ||
| } | ||
| }, [robot]); | ||
|
|
||
| const getRobot = async () => { | ||
| if (recordingId) { | ||
| const robot = await getStoredRecording(recordingId); | ||
| setRobot(robot); | ||
| } else { | ||
| notify('error', t('robot_duplication.notifications.robot_not_found')); | ||
| } | ||
| } | ||
|
|
||
| const handleTargetUrlChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| setTargetUrl(e.target.value); | ||
| }; |
There was a problem hiding this comment.
Keep the target URL input controlled
targetUrl flips between '' and undefined, so the <TextField> toggles from controlled to uncontrolled, triggering React warnings. Keep the state as a string and normalize the fetched URL to '' when absent.
- const [targetUrl, setTargetUrl] = useState<string | undefined>('');
+ const [targetUrl, setTargetUrl] = useState<string>('');
...
- const url = lastPair?.what.find(action => action.action === "goto")?.args?.[0];
- setTargetUrl(url);
+ const url = lastPair?.what.find(action => action.action === "goto")?.args?.[0];
+ setTargetUrl(url ?? '');📝 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 const RobotDuplicationModal = ({ isOpen, handleStart, handleClose, initialSettings }: RobotSettingsProps) => { | |
| const { t } = useTranslation(); | |
| const [targetUrl, setTargetUrl] = useState<string | undefined>(''); | |
| const [robot, setRobot] = useState<RobotSettings | null>(null); | |
| const { recordingId, notify, setRerenderRobots } = useGlobalInfoStore(); | |
| useEffect(() => { | |
| if (isOpen) { | |
| getRobot(); | |
| } | |
| }, [isOpen]); | |
| useEffect(() => { | |
| if (robot) { | |
| const lastPair = robot?.recording.workflow[robot?.recording.workflow.length - 1]; | |
| const url = lastPair?.what.find(action => action.action === "goto")?.args?.[0]; | |
| setTargetUrl(url); | |
| } | |
| }, [robot]); | |
| const getRobot = async () => { | |
| if (recordingId) { | |
| const robot = await getStoredRecording(recordingId); | |
| setRobot(robot); | |
| } else { | |
| notify('error', t('robot_duplication.notifications.robot_not_found')); | |
| } | |
| } | |
| const handleTargetUrlChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| setTargetUrl(e.target.value); | |
| }; | |
| export const RobotDuplicationModal = ({ | |
| isOpen, | |
| handleStart, | |
| handleClose, | |
| initialSettings, | |
| }: RobotSettingsProps) => { | |
| const { t } = useTranslation(); | |
| const [targetUrl, setTargetUrl] = useState<string>(''); | |
| const [robot, setRobot] = useState<RobotSettings | null>(null); | |
| const { recordingId, notify, setRerenderRobots } = useGlobalInfoStore(); | |
| useEffect(() => { | |
| if (isOpen) { | |
| getRobot(); | |
| } | |
| }, [isOpen]); | |
| useEffect(() => { | |
| if (robot) { | |
| const lastPair = | |
| robot.recording.workflow[robot.recording.workflow.length - 1]; | |
| const url = lastPair.what.find(a => a.action === 'goto')?.args?.[0]; | |
| setTargetUrl(url ?? ''); | |
| } | |
| }, [robot]); | |
| const getRobot = async () => { | |
| if (recordingId) { | |
| const robot = await getStoredRecording(recordingId); | |
| setRobot(robot); | |
| } else { | |
| notify('error', t('robot_duplication.notifications.robot_not_found')); | |
| } | |
| }; | |
| const handleTargetUrlChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| setTargetUrl(e.target.value); | |
| }; | |
| // … | |
| } |
🤖 Prompt for AI Agents
In legacy/src/RobotDuplicate.tsx around lines 56 to 87, the targetUrl state
toggles between '' and undefined causing controlled/uncontrolled React warnings;
change the useState to consistently be a string (initialize
useState<string>('')), and when deriving the URL from the robot normalize absent
values to an empty string (e.g. setTargetUrl(url || '')) so the TextField
remains controlled; keep the input change handler as-is.
Comment on lines
+128
to
+137
| {t('robot_duplication.descriptions.purpose')} | ||
| </span> | ||
| <br /> | ||
| <span dangerouslySetInnerHTML={{ | ||
| __html: t('robot_duplication.descriptions.example', { | ||
| url1: '<code>producthunt.com/topics/api</code>', | ||
| url2: '<code>producthunt.com/topics/database</code>' | ||
| }) | ||
| }} /> | ||
| <br /> |
There was a problem hiding this comment.
Remove dangerouslySetInnerHTML
Injecting translation strings through dangerouslySetInnerHTML bypasses React’s XSS guardrails (see lint warning). Use <Trans> so placeholders stay safe without manual HTML injection.
-import { useTranslation } from 'react-i18next';
+import { Trans, useTranslation } from 'react-i18next';
...
- <span dangerouslySetInnerHTML={{
- __html: t('robot_duplication.descriptions.example', {
- url1: '<code>producthunt.com/topics/api</code>',
- url2: '<code>producthunt.com/topics/database</code>'
- })
- }} />
+ <Trans
+ i18nKey="robot_duplication.descriptions.example"
+ components={{ code1: <code />, code2: <code /> }}
+ values={{
+ url1: 'producthunt.com/topics/api',
+ url2: 'producthunt.com/topics/database'
+ }}
+ />🧰 Tools
🪛 ast-grep (0.39.5)
[warning] 130-130: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 131-131: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In legacy/src/RobotDuplicate.tsx around lines 128 to 137, remove the
dangerouslySetInnerHTML usage and replace it with react-i18next's <Trans> so
translation placeholders are rendered safely; update the translation key to use
React nodes (e.g. <0></0> for code segments) and render <Trans
i18nKey="robot_duplication.descriptions.example" components={[<code key="code1"
/>, <code key="code2" />]} values={{ /* keep any values if needed */ }} /> (or
the equivalent components prop), ensuring you import Trans from react-i18next
and pass the two <code> elements as components instead of injecting raw HTML.
Comment on lines
+323
to
+369
| const handleLimitChange = (pairIndex: number, actionIndex: number, argIndex: number, newLimit: number) => { | ||
| setRobot((prev) => { | ||
| if (!prev) return prev; | ||
|
|
||
| const updatedWorkflow = [...prev.recording.workflow]; | ||
| if ( | ||
| updatedWorkflow.length > pairIndex && | ||
| updatedWorkflow[pairIndex]?.what && | ||
| updatedWorkflow[pairIndex].what.length > actionIndex && | ||
| updatedWorkflow[pairIndex].what[actionIndex].args && | ||
| updatedWorkflow[pairIndex].what[actionIndex].args.length > argIndex | ||
| ) { | ||
| updatedWorkflow[pairIndex].what[actionIndex].args[argIndex].limit = newLimit; | ||
|
|
||
| setScrapeListLimits(prev => { | ||
| return prev.map(item => { | ||
| if (item.pairIndex === pairIndex && | ||
| item.actionIndex === actionIndex && | ||
| item.argIndex === argIndex) { | ||
| return { ...item, currentLimit: newLimit }; | ||
| } | ||
| return item; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return { ...prev, recording: { ...prev.recording, workflow: updatedWorkflow } }; | ||
| }); | ||
| }; | ||
|
|
||
| const handleTargetUrlChange = (newUrl: string) => { | ||
| setRobot((prev) => { | ||
| if (!prev) return prev; | ||
|
|
||
| const updatedWorkflow = [...prev.recording.workflow]; | ||
| const lastPairIndex = updatedWorkflow.length - 1; | ||
|
|
||
| if (lastPairIndex >= 0) { | ||
| const gotoAction = updatedWorkflow[lastPairIndex]?.what?.find(action => action.action === "goto"); | ||
| if (gotoAction && gotoAction.args && gotoAction.args.length > 0) { | ||
| gotoAction.args[0] = newUrl; | ||
| } | ||
| } | ||
|
|
||
| return { ...prev, recording: { ...prev.recording, workflow: updatedWorkflow } }; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Avoid mutating the existing workflow state
Both handleLimitChange and handleTargetUrlChange clone only the outer workflow array, then mutate nested objects in place. That mutates prev state directly, breaking React’s immutability expectations and leading to subtle bugs. Rebuild only the touched branch immutably before writing the new limit / URL.
- const updatedWorkflow = [...prev.recording.workflow];
- if (
- updatedWorkflow.length > pairIndex &&
- updatedWorkflow[pairIndex]?.what &&
- updatedWorkflow[pairIndex].what.length > actionIndex &&
- updatedWorkflow[pairIndex].what[actionIndex].args &&
- updatedWorkflow[pairIndex].what[actionIndex].args.length > argIndex
- ) {
- updatedWorkflow[pairIndex].what[actionIndex].args[argIndex].limit = newLimit;
-
- setScrapeListLimits(prev => {
+ const updatedWorkflow = prev.recording.workflow.map((pair, idx) => {
+ if (idx !== pairIndex || !pair.what) return pair;
+
+ return {
+ ...pair,
+ what: pair.what.map((action, aIdx) => {
+ if (aIdx !== actionIndex || !action.args || action.args.length <= argIndex) {
+ return action;
+ }
+
+ const args = [...action.args];
+ const arg = args[argIndex];
+ if (typeof arg !== 'object' || arg === null) {
+ return action;
+ }
+
+ args[argIndex] = { ...arg, limit: newLimit };
+ return { ...action, args };
+ })
+ };
+ });
+
+ setScrapeListLimits(prev => {
return prev.map(item => {
if (item.pairIndex === pairIndex &&
item.actionIndex === actionIndex &&
item.argIndex === argIndex) {
return { ...item, currentLimit: newLimit };
}
return item;
});
- });
- }
+ });
...
- const updatedWorkflow = [...prev.recording.workflow];
- const lastPairIndex = updatedWorkflow.length - 1;
-
- if (lastPairIndex >= 0) {
- const gotoAction = updatedWorkflow[lastPairIndex]?.what?.find(action => action.action === "goto");
- if (gotoAction && gotoAction.args && gotoAction.args.length > 0) {
- gotoAction.args[0] = newUrl;
- }
- }
+ const updatedWorkflow = prev.recording.workflow.map((pair, idx) => {
+ if (idx !== prev.recording.workflow.length - 1 || !pair.what) {
+ return pair;
+ }
+
+ return {
+ ...pair,
+ what: pair.what.map(action => {
+ if (action.action !== "goto" || !action.args || action.args.length === 0) {
+ return action;
+ }
+
+ const args = [...action.args];
+ args[0] = newUrl;
+ return { ...action, args };
+ })
+ };
+ });📝 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 handleLimitChange = (pairIndex: number, actionIndex: number, argIndex: number, newLimit: number) => { | |
| setRobot((prev) => { | |
| if (!prev) return prev; | |
| const updatedWorkflow = [...prev.recording.workflow]; | |
| if ( | |
| updatedWorkflow.length > pairIndex && | |
| updatedWorkflow[pairIndex]?.what && | |
| updatedWorkflow[pairIndex].what.length > actionIndex && | |
| updatedWorkflow[pairIndex].what[actionIndex].args && | |
| updatedWorkflow[pairIndex].what[actionIndex].args.length > argIndex | |
| ) { | |
| updatedWorkflow[pairIndex].what[actionIndex].args[argIndex].limit = newLimit; | |
| setScrapeListLimits(prev => { | |
| return prev.map(item => { | |
| if (item.pairIndex === pairIndex && | |
| item.actionIndex === actionIndex && | |
| item.argIndex === argIndex) { | |
| return { ...item, currentLimit: newLimit }; | |
| } | |
| return item; | |
| }); | |
| }); | |
| } | |
| return { ...prev, recording: { ...prev.recording, workflow: updatedWorkflow } }; | |
| }); | |
| }; | |
| const handleTargetUrlChange = (newUrl: string) => { | |
| setRobot((prev) => { | |
| if (!prev) return prev; | |
| const updatedWorkflow = [...prev.recording.workflow]; | |
| const lastPairIndex = updatedWorkflow.length - 1; | |
| if (lastPairIndex >= 0) { | |
| const gotoAction = updatedWorkflow[lastPairIndex]?.what?.find(action => action.action === "goto"); | |
| if (gotoAction && gotoAction.args && gotoAction.args.length > 0) { | |
| gotoAction.args[0] = newUrl; | |
| } | |
| } | |
| return { ...prev, recording: { ...prev.recording, workflow: updatedWorkflow } }; | |
| }); | |
| }; | |
| const handleLimitChange = ( | |
| pairIndex: number, | |
| actionIndex: number, | |
| argIndex: number, | |
| newLimit: number | |
| ) => { | |
| setRobot((prev) => { | |
| if (!prev) return prev; | |
| const updatedWorkflow = prev.recording.workflow.map((pair, idx) => { | |
| if (idx !== pairIndex || !pair.what) return pair; | |
| return { | |
| ...pair, | |
| what: pair.what.map((action, aIdx) => { | |
| if (aIdx !== actionIndex || !action.args || action.args.length <= argIndex) { | |
| return action; | |
| } | |
| const args = [...action.args]; | |
| const arg = args[argIndex]; | |
| // Only update if it's an object with a limit property | |
| if (typeof arg !== 'object' || arg === null) { | |
| return action; | |
| } | |
| args[argIndex] = { ...arg, limit: newLimit }; | |
| return { ...action, args }; | |
| }), | |
| }; | |
| }); | |
| setScrapeListLimits((prev) => | |
| prev.map((item) => | |
| item.pairIndex === pairIndex && | |
| item.actionIndex === actionIndex && | |
| item.argIndex === argIndex | |
| ? { ...item, currentLimit: newLimit } | |
| : item | |
| ) | |
| ); | |
| return { | |
| ...prev, | |
| recording: { ...prev.recording, workflow: updatedWorkflow }, | |
| }; | |
| }); | |
| }; | |
| const handleTargetUrlChange = (newUrl: string) => { | |
| setRobot((prev) => { | |
| if (!prev) return prev; | |
| const updatedWorkflow = prev.recording.workflow.map((pair, idx) => { | |
| // only change the last pair in the workflow | |
| if (idx !== prev.recording.workflow.length - 1 || !pair.what) { | |
| return pair; | |
| } | |
| return { | |
| ...pair, | |
| what: pair.what.map((action) => { | |
| if (action.action !== "goto" || !action.args || action.args.length === 0) { | |
| return action; | |
| } | |
| const args = [...action.args]; | |
| args[0] = newUrl; | |
| return { ...action, args }; | |
| }), | |
| }; | |
| }); | |
| return { | |
| ...prev, | |
| recording: { ...prev.recording, workflow: updatedWorkflow }, | |
| }; | |
| }); | |
| }; |
Comment on lines
+401
to
+431
| const renderCredentialFields = (selectors: string[], headerText: string, defaultType: 'text' | 'password' = 'text') => { | ||
| if (selectors.length === 0) return null; | ||
|
|
||
| return ( | ||
| <> | ||
| {selectors.map((selector, index) => { | ||
| const isVisible = showPasswords[selector]; | ||
|
|
||
| return ( | ||
| <TextField | ||
| key={selector} | ||
| type={isVisible ? 'text' : 'password'} | ||
| label={headerText === 'Other' ? `${`Input`} ${index + 1}` : headerText} | ||
| value={credentials[selector]?.value || ''} | ||
| onChange={(e) => handleCredentialChange(selector, e.target.value)} | ||
| style={{ marginBottom: '20px' }} | ||
| InputProps={{ | ||
| endAdornment: ( | ||
| <InputAdornment position="end"> | ||
| <IconButton | ||
| aria-label="Show input" | ||
| onClick={() => handleClickShowPassword(selector)} | ||
| edge="end" | ||
| disabled={!credentials[selector]?.value} | ||
| > | ||
| {isVisible ? <Visibility /> : <VisibilityOff />} | ||
| </IconButton> | ||
| </InputAdornment> | ||
| ), | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Unintentionally masking non-password credential inputs
Every credential field defaults to type="password" unless toggled because the code ignores defaultType. As a result, usernames/emails render hidden, which is a functional regression. Respect the intended default type and only attach the show/hide toggle to password fields.
- const isVisible = showPasswords[selector];
+ const isVisible = showPasswords[selector] ?? false;
+ const isPasswordField = defaultType === 'password';
+ const inputType = isPasswordField
+ ? (isVisible ? 'text' : 'password')
+ : 'text';
...
- <TextField
+ <TextField
key={selector}
- type={isVisible ? 'text' : 'password'}
+ type={inputType}
...
- InputProps={{
- endAdornment: (
- <InputAdornment position="end">
- <IconButton
- aria-label="Show input"
- onClick={() => handleClickShowPassword(selector)}
- edge="end"
- disabled={!credentials[selector]?.value}
- >
- {isVisible ? <Visibility /> : <VisibilityOff />}
- </IconButton>
- </InputAdornment>
- ),
- }}
+ InputProps={isPasswordField ? {
+ endAdornment: (
+ <InputAdornment position="end">
+ <IconButton
+ aria-label={t('robot_edit.toggle_password_visibility')}
+ onClick={() => handleClickShowPassword(selector)}
+ edge="end"
+ disabled={!credentials[selector]?.value}
+ >
+ {isVisible ? <Visibility /> : <VisibilityOff />}
+ </IconButton>
+ </InputAdornment>
+ ),
+ } : undefined}📝 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 renderCredentialFields = (selectors: string[], headerText: string, defaultType: 'text' | 'password' = 'text') => { | |
| if (selectors.length === 0) return null; | |
| return ( | |
| <> | |
| {selectors.map((selector, index) => { | |
| const isVisible = showPasswords[selector]; | |
| return ( | |
| <TextField | |
| key={selector} | |
| type={isVisible ? 'text' : 'password'} | |
| label={headerText === 'Other' ? `${`Input`} ${index + 1}` : headerText} | |
| value={credentials[selector]?.value || ''} | |
| onChange={(e) => handleCredentialChange(selector, e.target.value)} | |
| style={{ marginBottom: '20px' }} | |
| InputProps={{ | |
| endAdornment: ( | |
| <InputAdornment position="end"> | |
| <IconButton | |
| aria-label="Show input" | |
| onClick={() => handleClickShowPassword(selector)} | |
| edge="end" | |
| disabled={!credentials[selector]?.value} | |
| > | |
| {isVisible ? <Visibility /> : <VisibilityOff />} | |
| </IconButton> | |
| </InputAdornment> | |
| ), | |
| }} | |
| /> | |
| const renderCredentialFields = ( | |
| selectors: string[], | |
| headerText: string, | |
| defaultType: 'text' | 'password' = 'text' | |
| ) => { | |
| if (selectors.length === 0) return null; | |
| return ( | |
| <> | |
| {selectors.map((selector, index) => { | |
| const isVisible = showPasswords[selector] ?? false; | |
| const isPasswordField = defaultType === 'password'; | |
| const inputType = isPasswordField | |
| ? (isVisible ? 'text' : 'password') | |
| : 'text'; | |
| return ( | |
| <TextField | |
| key={selector} | |
| type={inputType} | |
| label={ | |
| headerText === 'Other' | |
| ? `${`Input`} ${index + 1}` | |
| : headerText | |
| } | |
| value={credentials[selector]?.value || ''} | |
| onChange={(e) => | |
| handleCredentialChange(selector, e.target.value) | |
| } | |
| style={{ marginBottom: '20px' }} | |
| - InputProps={{ | |
| - endAdornment: ( | |
| - <InputAdornment position="end"> | |
| - <IconButton | |
| - aria-label="Show input" | |
| - onClick={() => handleClickShowPassword(selector)} | |
| - edge="end" | |
| - disabled={!credentials[selector]?.value} | |
| - > | |
| - {isVisible ? <Visibility /> : <VisibilityOff />} | |
| - </IconButton> | |
| - </InputAdornment> | |
| - ), | |
| InputProps={ | |
| isPasswordField | |
| ? { | |
| endAdornment: ( | |
| <InputAdornment position="end"> | |
| <IconButton | |
| aria-label={t( | |
| 'robot_edit.toggle_password_visibility' | |
| )} | |
| onClick={() => | |
| handleClickShowPassword(selector) | |
| } | |
| edge="end" | |
| disabled={ | |
| !credentials[selector]?.value | |
| } | |
| > | |
| {isVisible ? ( | |
| <Visibility /> | |
| ) : ( | |
| <VisibilityOff /> | |
| )} | |
| </IconButton> | |
| </InputAdornment> | |
| ), | |
| } | |
| : undefined | |
| } | |
| /> | |
| ); | |
| })} | |
| </> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In legacy/src/RobotEdit.tsx around lines 401 to 431, the input type is always
treated as a password because the code ignores the defaultType param and always
toggles visibility; change it so the field uses defaultType by default and only
applies the show/hide behavior when defaultType === 'password'. Concretely,
compute the input type as: if defaultType === 'password' then use isVisible ?
'text' : 'password' else use defaultType, and only render the
InputAdornment/IconButton toggle when defaultType === 'password' (keeping the
disabled logic when the credential value is empty).
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
legacy/src/ScheduleSettings.tsx (1)
113-120: AddrecordingIdto the dependency array.The
useEffectusesrecordingIdfrom the global store but doesn't include it in the dependency array. WhilerecordingIdfrom global context is likely stable, explicitly including it follows React best practices and prevents potential stale closure issues.Apply this diff:
useEffect(() => { if (isOpen) { const fetchSchedule = async () => { await getRobotSchedule(); }; fetchSchedule(); } - }, [isOpen]); + }, [isOpen, recordingId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
legacy/src/ScheduleSettings.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
legacy/src/ScheduleSettings.tsx (5)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(183-183)src/api/storage.ts (2)
deleteSchedule(256-268)getSchedule(242-254)src/components/ui/GenericModal.tsx (1)
GenericModal(13-29)src/components/ui/DropdownMui.tsx (1)
Dropdown(15-32)src/constants/const.ts (1)
validMomentTimezones(11-604)
Comment on lines
+84
to
+102
| const deleteRobotSchedule = () => { | ||
| if (recordingId) { | ||
| deleteSchedule(recordingId); | ||
| setSchedule(null); | ||
| notify('success', t('Schedule deleted successfully')); | ||
| } else { | ||
| console.error('No recording id provided'); | ||
| } | ||
|
|
||
| setSettings({ | ||
| runEvery: 1, | ||
| runEveryUnit: 'HOURS', | ||
| startFrom: 'MONDAY', | ||
| dayOfMonth: '', | ||
| atTimeStart: '00:00', | ||
| atTimeEnd: '01:00', | ||
| timezone: 'UTC' | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Await the async deleteSchedule call and add error handling.
The deleteSchedule function is async but not awaited (line 86). This can cause race conditions where the state is reset and success notification is shown before the deletion completes or even if it fails.
Apply this diff to fix the issue:
- const deleteRobotSchedule = () => {
+ const deleteRobotSchedule = async () => {
if (recordingId) {
- deleteSchedule(recordingId);
- setSchedule(null);
- notify('success', t('Schedule deleted successfully'));
+ const success = await deleteSchedule(recordingId);
+ if (success) {
+ setSchedule(null);
+ notify('success', t('Schedule deleted successfully'));
+ setSettings({
+ runEvery: 1,
+ runEveryUnit: 'HOURS',
+ startFrom: 'MONDAY',
+ dayOfMonth: '',
+ atTimeStart: '00:00',
+ atTimeEnd: '01:00',
+ timezone: 'UTC'
+ });
+ } else {
+ notify('error', t('Failed to delete schedule'));
+ }
} else {
console.error('No recording id provided');
}
-
- setSettings({
- runEvery: 1,
- runEveryUnit: 'HOURS',
- startFrom: 'MONDAY',
- dayOfMonth: '',
- atTimeStart: '00:00',
- atTimeEnd: '01:00',
- timezone: 'UTC'
- });
};📝 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 deleteRobotSchedule = () => { | |
| if (recordingId) { | |
| deleteSchedule(recordingId); | |
| setSchedule(null); | |
| notify('success', t('Schedule deleted successfully')); | |
| } else { | |
| console.error('No recording id provided'); | |
| } | |
| setSettings({ | |
| runEvery: 1, | |
| runEveryUnit: 'HOURS', | |
| startFrom: 'MONDAY', | |
| dayOfMonth: '', | |
| atTimeStart: '00:00', | |
| atTimeEnd: '01:00', | |
| timezone: 'UTC' | |
| }); | |
| }; | |
| const deleteRobotSchedule = async () => { | |
| if (recordingId) { | |
| const success = await deleteSchedule(recordingId); | |
| if (success) { | |
| setSchedule(null); | |
| notify('success', t('Schedule deleted successfully')); | |
| setSettings({ | |
| runEvery: 1, | |
| runEveryUnit: 'HOURS', | |
| startFrom: 'MONDAY', | |
| dayOfMonth: '', | |
| atTimeStart: '00:00', | |
| atTimeEnd: '01:00', | |
| timezone: 'UTC' | |
| }); | |
| } else { | |
| notify('error', t('Failed to delete schedule')); | |
| } | |
| } else { | |
| console.error('No recording id provided'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In legacy/src/ScheduleSettings.tsx around lines 84 to 102, the
deleteRobotSchedule handler calls the async deleteSchedule without awaiting it
and lacks error handling; change the function to be async, await
deleteSchedule(recordingId) inside a try/catch, only call setSchedule(null) and
notify('success', ...) after the awaited deletion succeeds, and in the catch
call notify('error', ...) (and optionally console.error) with the error message;
preserve the final settings reset either in finally or after success as
appropriate.
| {schedule !== null ? ( | ||
| <> | ||
| <Typography>{t('schedule_settings.run_every')}: {schedule.runEvery} {schedule.runEveryUnit.toLowerCase()}</Typography> | ||
| <Typography>{['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.start_from') : t('schedule_settings.start_from')}: {schedule.startFrom.charAt(0).toUpperCase() + schedule.startFrom.slice(1).toLowerCase()}</Typography> |
There was a problem hiding this comment.
Remove redundant ternary operator.
The ternary operator evaluates to the same translation key regardless of condition. This is likely a copy-paste error or incomplete implementation.
Apply this diff to simplify:
- <Typography>{['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.start_from') : t('schedule_settings.start_from')}: {schedule.startFrom.charAt(0).toUpperCase() + schedule.startFrom.slice(1).toLowerCase()}</Typography>
+ <Typography>{t('schedule_settings.start_from')}: {schedule.startFrom.charAt(0).toUpperCase() + schedule.startFrom.slice(1).toLowerCase()}</Typography>📝 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
| <Typography>{['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.start_from') : t('schedule_settings.start_from')}: {schedule.startFrom.charAt(0).toUpperCase() + schedule.startFrom.slice(1).toLowerCase()}</Typography> | |
| <Typography>{t('schedule_settings.start_from')}: {schedule.startFrom.charAt(0).toUpperCase() + schedule.startFrom.slice(1).toLowerCase()}</Typography> |
🤖 Prompt for AI Agents
In legacy/src/ScheduleSettings.tsx around line 159, the ternary operator around
the translation call is redundant because both branches return the same key;
replace the entire ternary expression with a single call to
t('schedule_settings.start_from') so the JSX becomes a single Typography that
uses t('schedule_settings.start_from') followed by the formatted
schedule.startFrom value (preserve the existing capitalization logic).
Comment on lines
+199
to
+201
| <Typography sx={{ marginBottom: '5px', marginRight: '25px' }}> | ||
| {['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.labels.start_from_label') : t('schedule_settings.labels.start_from_label')} | ||
| </Typography> |
There was a problem hiding this comment.
Remove redundant ternary operator.
The ternary operator evaluates to the same translation key regardless of condition. This appears to be duplicate code from line 159.
Apply this diff to simplify:
<Typography sx={{ marginBottom: '5px', marginRight: '25px' }}>
- {['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.labels.start_from_label') : t('schedule_settings.labels.start_from_label')}
+ {t('schedule_settings.labels.start_from_label')}
</Typography>📝 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
| <Typography sx={{ marginBottom: '5px', marginRight: '25px' }}> | |
| {['MONTHS', 'WEEKS'].includes(settings.runEveryUnit) ? t('schedule_settings.labels.start_from_label') : t('schedule_settings.labels.start_from_label')} | |
| </Typography> | |
| <Typography sx={{ marginBottom: '5px', marginRight: '25px' }}> | |
| {t('schedule_settings.labels.start_from_label')} | |
| </Typography> |
🤖 Prompt for AI Agents
In legacy/src/ScheduleSettings.tsx around lines 199 to 201, the JSX uses a
ternary that returns the same translation key for both branches; remove the
redundant conditional and replace the entire expression with a single call to
t('schedule_settings.labels.start_from_label') so the Typography simply renders
that translation.
Comment on lines
+269
to
+271
| {validMomentTimezones.map((tz) => ( | ||
| <MenuItem key={tz} value={tz}>{tz.charAt(0).toUpperCase() + tz.slice(1).toLowerCase()}</MenuItem> | ||
| ))} |
There was a problem hiding this comment.
Timezone display may not capitalize correctly.
The capitalization logic tz.charAt(0).toUpperCase() + tz.slice(1).toLowerCase() will convert "America/New_York" to "America/new_york", which is incorrect. Most timezone names in validMomentTimezones contain slashes and underscores that should be preserved.
Consider displaying timezones as-is or using a more sophisticated formatting approach:
{validMomentTimezones.map((tz) => (
- <MenuItem key={tz} value={tz}>{tz.charAt(0).toUpperCase() + tz.slice(1).toLowerCase()}</MenuItem>
+ <MenuItem key={tz} value={tz}>{tz}</MenuItem>
))}📝 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
| {validMomentTimezones.map((tz) => ( | |
| <MenuItem key={tz} value={tz}>{tz.charAt(0).toUpperCase() + tz.slice(1).toLowerCase()}</MenuItem> | |
| ))} | |
| {validMomentTimezones.map((tz) => ( | |
| <MenuItem key={tz} value={tz}>{tz}</MenuItem> | |
| ))} |
🤖 Prompt for AI Agents
In legacy/src/ScheduleSettings.tsx around lines 269-271 the current
capitalization logic corrupts timezone names (e.g. "America/New_York" ->
"America/new_york"); replace the inline transform with a proper formatter:
either render the timezone string as-is, or implement a small formatter that
splits on '/' and for each segment replaces '_' with ' ', capitalizes each word
(first letter only) and then joins segments with " / " so "America/New_York"
becomes "America / New York"; keep the MenuItem key/value as the original tz.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
moves all robot modals to legacy since we moved to pages.