feat: display currently running robot run #417
Conversation
RohitR311
added
Type: Enhancement
WalkthroughThe pull request introduces enhancements to the application's run and recording management system by adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/components/run/RunsTable.tsx (1)
83-89: Consider using a more robust URL parsing approach.While the current regex-based URL parameter extraction works, consider using a more maintainable approach.
Consider using the
useParamshook from react-router-dom:-const getUrlParams = () => { - const match = location.pathname.match(/\/runs\/([^\/]+)(?:\/run\/([^\/]+))?/); - return { - robotMetaId: match?.[1] || null, - urlRunId: match?.[2] || null - }; -}; +import { useParams } from 'react-router-dom'; + +// In component: +const { robotMetaId: urlRobotMetaId, runId: urlRunId } = useParams<{ + robotMetaId: string; + runId: string; +}>();server/src/routes/storage.ts (1)
504-504: LGTM! Consider adding a type definition for the response.The addition of
robotMetaIdto the response object is correct and aligns with the PR objective. However, to improve type safety and documentation, consider defining an interface for the response object.+interface RunResponse { + browserId: string; + runId: string; + robotMetaId: string; +} router.put('/runs/:id', requireSignIn, async (req: AuthenticatedRequest, res) => { try { // ... existing code ... - return res.send({ + const response: RunResponse = { browserId: id, runId: plainRun.runId, robotMetaId: recording.recording_meta.id, - }); + }; + return res.send(response);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/routes/storage.ts(1 hunks)src/api/storage.ts(1 hunks)src/components/run/ColapsibleRow.tsx(2 hunks)src/components/run/RunsTable.tsx(4 hunks)src/pages/MainPage.tsx(4 hunks)
🔇 Additional comments (9)
src/pages/MainPage.tsx (3)
28-28: LGTM! Enhanced response type with robotMetaId.The addition of robotMetaId to CreateRunResponse interface improves type safety and documentation.
45-46: LGTM! Proper state initialization.The state initialization correctly includes the new robotMetaId field with an empty string default.
95-97: LGTM! Enhanced navigation with robotMetaId.The navigation implementation properly uses both robotMetaId and runId for precise routing to specific runs.
src/api/storage.ts (1)
164-164: LGTM! Consistent error response structure.The error case properly returns an object with empty strings for all fields, maintaining type safety and consistency with the success case.
src/components/run/ColapsibleRow.tsx (3)
38-38: LGTM! Added URL run ID prop.The urlRunId prop enables the component to sync its state with URL parameters.
66-68: LGTM! Proper state synchronization.The useEffect hook correctly synchronizes the open state with URL parameters and isOpen prop.
73-77: LGTM! Enhanced navigation logic.The navigation implementation properly handles both expansion and collapse states using robotMetaId.
src/components/run/RunsTable.tsx (2)
93-95: LGTM! Clear accordion expansion logic.The isAccordionExpanded function properly determines expansion state based on URL parameters.
263-267: LGTM! Comprehensive row state handling.The row state properly considers both URL parameters and runtime state (current run).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/run/RunsTable.tsx (3)
90-96: Enhance URL parameter handling robustness.The URL parameter extraction could be more robust with validation and error handling.
Consider this enhanced implementation:
const getUrlParams = () => { - const match = location.pathname.match(/\/runs\/([^\/]+)(?:\/run\/([^\/]+))?/); + try { + const match = location.pathname.match(/\/runs\/([^\/]+)(?:\/run\/([^\/]+))?/); + const robotMetaId = match?.[1]; + const urlRunId = match?.[2]; + + if (robotMetaId && !isValidRobotMetaId(robotMetaId)) { + console.warn(`Invalid robotMetaId format: ${robotMetaId}`); + } + + return { + robotMetaId: robotMetaId || null, + urlRunId: urlRunId || null + }; + } catch (error) { + console.error('Error parsing URL parameters:', error); + return { robotMetaId: null, urlRunId: null }; + } };Add this helper function at the top of the file:
const isValidRobotMetaId = (id: string): boolean => { // Implement validation logic based on your robotMetaId format return /^[a-zA-Z0-9-_]+$/.test(id); };
332-336: Simplify row expansion logic and consider prop drilling.The row expansion logic could be simplified, and the
urlRunIdprop might indicate prop drilling.Consider these improvements:
- isOpen={urlRunId === row.runId || (runId === row.runId && runningRecordingName === row.name)} + isOpen={isRowExpanded(row, { urlRunId, runId, runningRecordingName })} currentLog={currentInterpretationLog} abortRunHandler={abortRunHandler} runningRecordingName={runningRecordingName} - urlRunId={urlRunId} + expansionState={{ urlRunId, runId, runningRecordingName }}Add this helper function to improve readability:
const isRowExpanded = ( row: Data, { urlRunId, runId, runningRecordingName }: { urlRunId: string | null; runId: string; runningRecordingName: string; } ): boolean => { const isUrlMatch = urlRunId === row.runId; const isRunningMatch = runId === row.runId && runningRecordingName === row.name; return isUrlMatch || isRunningMatch; };Consider using React Context if
urlRunIdis needed by multiple child components to avoid prop drilling.
Line range hint
1-479: Consider architectural improvements for better maintainability.The component handles multiple responsibilities (URL parsing, data management, UI rendering) and could benefit from further modularization.
Consider these architectural improvements:
- Extract URL parameter handling into a custom hook:
// hooks/useRunsUrlParams.ts export const useRunsUrlParams = () => { const location = useLocation(); return useMemo(() => getUrlParams(location), [location]); };
- Extract table state management into a separate context or custom hook:
// hooks/useRunsTableState.ts export const useRunsTableState = (initialState: TableState) => { // Move pagination, sorting, and filtering logic here };
- Consider splitting the component into smaller, focused components:
- RunsTableHeader
- RunsTableFilters
- RunsTablePagination
This would improve maintainability and make the code easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/run/RunsTable.tsx(3 hunks)
🔇 Additional comments (1)
src/components/run/RunsTable.tsx (1)
100-102: LGTM! Clean and efficient implementation.The accordion expansion logic is well-implemented with proper memoization and clear conditional logic.
Closes: #416
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
robotMetaIdto run response for better context.The updates focus on improving the user experience by providing more detailed run information and more intuitive navigation between runs and recordings.