feat: group action inputs by type #854
Conversation
WalkthroughRefactors the input handling in RobotEditPage component by replacing a single inputs array with three separate collections (textInputs, screenshotInputs, listInputs), introduces categorization logic using switch statements, and updates rendering to display inputs grouped by type with conditional subtitles and margin adjustments. Additionally, removes a maxHeight constraint from RobotConfigPage to allow height expansion and rely on internal scrolling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/robot/pages/RobotEditPage.tsx (1)
591-616: Use i18n for subtitle strings.The subtitle strings "Texts", "Screenshots", and "Lists" are hard-coded, which breaks consistency with the rest of the component that uses
t()for internationalization.Consider applying this pattern:
- <Typography variant="subtitle1" style={{ marginBottom: '8px' }}> - Texts - </Typography> + <Typography variant="subtitle1" style={{ marginBottom: '8px' }}> + {t('Texts')} + </Typography>Apply similar changes for "Screenshots" (line 603) and "Lists" (line 612).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/robot/pages/RobotEditPage.tsx(3 hunks)
🔇 Additional comments (4)
src/components/robot/pages/RobotEditPage.tsx (4)
509-511: LGTM! Improved spacing consistency.The added top margin enhances the visual separation of the List Limits section.
542-579: Well-structured refactor for grouping inputs by action type.The categorization logic correctly maps each action type to its corresponding collection. The switch statement appropriately handles all three editable action types without requiring a default case.
582-583: LGTM! Correctly adapted empty check for new structure.The
hasAnyInputscheck appropriately replaces the previousinputs.lengthcheck to work with the three separate collections.
587-589: LGTM! Better semantic heading hierarchy.Changing from
body1toh6improves the semantic structure, making the "Actions" heading more accessible and better styled as a section header.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/robot/pages/RobotEditPage.tsx (1)
558-566: Consider adding labels to TextFields for accessibility.The TextFields lack explicit
labelprops, relying on group subtitles for context. Adding labels would improve accessibility for screen reader users and provide clearer input identification.Example approach - add a label based on action type:
const textField = ( <TextField key={`action-name-${pairIndex}-${actionIndex}`} + label={`${t(`actions.${action.action}`)} ${textInputs.length + screenshotInputs.length + listInputs.length + 1}`} type="text" value={currentName} onChange={(e) => handleActionNameChange(pairIndex, actionIndex, e.target.value)} style={{ marginBottom: '12px' }} fullWidth /> );Note: This would require tracking indices per group or generating labels differently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/robot/pages/RobotConfigPage.tsx(0 hunks)src/components/robot/pages/RobotEditPage.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/robot/pages/RobotConfigPage.tsx
🔇 Additional comments (2)
src/components/robot/pages/RobotEditPage.tsx (2)
509-511: LGTM! Improved visual hierarchy.The typography upgrade to
h6and added top margin improve the section hierarchy and spacing.
542-580: Well-structured categorization logic.The refactoring successfully separates action inputs by type using clear switch-case logic. The filtering at line 550 ensures only relevant actions are processed, making the absence of a default case acceptable.
| {textInputs.length > 0 && ( | ||
| <> | ||
| <Typography variant="subtitle1" style={{ marginBottom: '8px' }}> | ||
| Texts | ||
| </Typography> | ||
| {textInputs} | ||
| </> | ||
| )} | ||
|
|
||
| {screenshotInputs.length > 0 && ( | ||
| <> | ||
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: textInputs.length > 0 ? '16px' : '0' }}> | ||
| Screenshots | ||
| </Typography> | ||
| {screenshotInputs} | ||
| </> | ||
| )} | ||
|
|
||
| {listInputs.length > 0 && ( | ||
| <> | ||
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: (textInputs.length > 0 || screenshotInputs.length > 0) ? '16px' : '0' }}> | ||
| Lists | ||
| </Typography> | ||
| {listInputs} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
Use translations for subtitle strings.
The subtitle strings "Texts", "Screenshots", and "Lists" are hardcoded instead of using the t() function, breaking i18n consistency with the rest of the component.
Apply this diff to use translations:
- <Typography variant="subtitle1" style={{ marginBottom: '8px' }}>
- Texts
+ <Typography variant="subtitle1" style={{ marginBottom: '8px' }}>
+ {t('actions.texts')}
</Typography>- <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: textInputs.length > 0 ? '16px' : '0' }}>
- Screenshots
+ <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: textInputs.length > 0 ? '16px' : '0' }}>
+ {t('actions.screenshots')}
</Typography>- <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: (textInputs.length > 0 || screenshotInputs.length > 0) ? '16px' : '0' }}>
- Lists
+ <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: (textInputs.length > 0 || screenshotInputs.length > 0) ? '16px' : '0' }}>
+ {t('actions.lists')}
</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.
| {textInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px' }}> | |
| Texts | |
| </Typography> | |
| {textInputs} | |
| </> | |
| )} | |
| {screenshotInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: textInputs.length > 0 ? '16px' : '0' }}> | |
| Screenshots | |
| </Typography> | |
| {screenshotInputs} | |
| </> | |
| )} | |
| {listInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: (textInputs.length > 0 || screenshotInputs.length > 0) ? '16px' : '0' }}> | |
| Lists | |
| </Typography> | |
| {listInputs} | |
| </> | |
| )} | |
| {textInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px' }}> | |
| {t('actions.texts')} | |
| </Typography> | |
| {textInputs} | |
| </> | |
| )} | |
| {screenshotInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: textInputs.length > 0 ? '16px' : '0' }}> | |
| {t('actions.screenshots')} | |
| </Typography> | |
| {screenshotInputs} | |
| </> | |
| )} | |
| {listInputs.length > 0 && ( | |
| <> | |
| <Typography variant="subtitle1" style={{ marginBottom: '8px', marginTop: (textInputs.length > 0 || screenshotInputs.length > 0) ? '16px' : '0' }}> | |
| {t('actions.lists')} | |
| </Typography> | |
| {listInputs} | |
| </> | |
| )} |
🤖 Prompt for AI Agents
In src/components/robot/pages/RobotEditPage.tsx around lines 591 to 616, the
subtitles "Texts", "Screenshots", and "Lists" are hardcoded; replace them with
translation calls (e.g. t('texts'), t('screenshots'), t('lists')) using the
component's i18n hook/namespace (or the existing translation keys if present) so
the Typography labels use t(...) instead of raw strings; ensure the file
imports/uses the same t from useTranslation already used elsewhere in the
component and keep existing conditional spacing logic unchanged.
ref: #847
Summary by CodeRabbit
Style & Refactor
Bug Fixes