feat: add functionality to handle user inputs (part 1) #268
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces enhanced functionalities for handling date, dropdown, and time selection events across multiple components. It includes the addition of new functions and interfaces for managing these events on the server side, as well as the creation of React components for user interaction. The implementation facilitates dynamic interactions through socket-based communication, ensuring proper event handling and state management for date and time inputs. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Canvas
participant Socket
participant Server
participant WorkflowGenerator
participant Browser
Client->>Canvas: Trigger Date Selection
Canvas->>Socket: Emit showDatePicker
Socket->>Server: Broadcast Event
Server->>WorkflowGenerator: Trigger onClick
WorkflowGenerator->>Browser: Show Date Picker
Client->>DatePicker: Select Date
DatePicker->>Socket: Emit input:date
Socket->>Server: Send Date Event
Server->>WorkflowGenerator: onDateSelection
WorkflowGenerator->>Browser: Fill Date Input
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🪧 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: 4
🧹 Nitpick comments (5)
server/src/types/index.ts (1)
27-31: Consider using a more specific type for thevaluepropertyThe
valueproperty could benefit from a more specific type to ensure date string format consistency.export interface DatePickerEventData { coordinates: Coordinates; selector: string; - value: string; + value: `${number}-${number}-${number}`; // YYYY-MM-DD format }src/components/atoms/DatePicker.tsx (1)
29-41: Consider handling window boundaries for positioningThe absolute positioning might cause the date picker to render outside viewport bounds.
<div style={{ position: 'absolute', left: `${coordinates.x}px`, top: `${coordinates.y}px`, zIndex: 1000, backgroundColor: 'white', boxShadow: '0 2px 10px rgba(0,0,0,0.1)', padding: '10px', - borderRadius: '4px' + borderRadius: '4px', + maxWidth: '300px', + maxHeight: '400px', + overflow: 'auto' }} + role="dialog" + aria-modal="true" >server/src/browser-management/inputHandlers.ts (2)
226-241: Consider enhancing error handling in handleDateSelection.The implementation follows the established patterns and is well-documented. However, the error handling could be improved by:
- Using the logger instead of console.error
- Propagating the error to allow proper handling upstream
const handleDateSelection = async (generator: WorkflowGenerator, page: Page, data: DatePickerEventData) => { const { selector, value } = data; try { await page.fill(selector, value); } catch (error) { - console.error("Failed to fill date value:", error); + logger.log('error', `Failed to fill date value: ${error}`); + throw error; } const pair: WhereWhatPair = { where: { url: this.getBestUrl(page.url()) }, what: [{ action: 'fill', args: [selector, value], }], }; await this.addPairToWorkflowAndNotifyClient(pair, page); };
Line range hint
289-302: Consider using type predicates for better type safety.The implementation correctly handles date input detection and event emission. However, the type checking could be more explicit using TypeScript type predicates.
- const isDateInput = await page.evaluate(({x, y}) => { - const element = document.elementFromPoint(x, y); - return element instanceof HTMLInputElement && element.type === 'date'; - }, coordinates); + const isDateInput = await page.evaluate(({x, y}): boolean => { + const element = document.elementFromPoint(x, y); + function isDateInputElement(el: Element | null): el is HTMLInputElement { + return el instanceof HTMLInputElement && el.type === 'date'; + } + return isDateInputElement(element); + }, coordinates);server/src/workflow-management/classes/Generator.ts (1)
258-276: Consider adding retry logic for resilience.The implementation is solid but could benefit from retry logic for intermittent failures during date filling operations, which are common in browser automation.
public onDateSelection = async (page: Page, data: DatePickerEventData) => { const { selector, value } = data; + const maxRetries = 3; + let lastError: Error | null = null; + + for (let attempt = 1; attempt <= maxRetries; attempt++) { try { await page.fill(selector, value); + break; } catch (error) { - console.error("Failed to fill date value:", error); + lastError = error as Error; + logger.log('warn', `Failed to fill date value (attempt ${attempt}/${maxRetries}): ${error}`); + if (attempt === maxRetries) { + logger.log('error', `All attempts to fill date value failed: ${error}`); + throw lastError; + } + await page.waitForTimeout(1000 * attempt); // Exponential backoff } } const pair: WhereWhatPair = { where: { url: this.getBestUrl(page.url()) }, what: [{ action: 'fill', args: [selector, value], }], }; await this.addPairToWorkflowAndNotifyClient(pair, page); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/browser-management/inputHandlers.ts(3 hunks)server/src/types/index.ts(1 hunks)server/src/workflow-management/classes/Generator.ts(3 hunks)src/components/atoms/DatePicker.tsx(1 hunks)src/components/atoms/canvas.tsx(4 hunks)
🔇 Additional comments (1)
server/src/browser-management/inputHandlers.ts (1)
9-9: LGTM!
The import of DatePickerEventData type is correctly added to support the new date picker functionality.
| <input | ||
| type="date" | ||
| onChange={handleDateChange} | ||
| value={selectedDate} | ||
| className="p-2 border rounded" | ||
| autoFocus | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance date input validation and accessibility
The date input field needs validation and additional accessibility attributes.
<input
type="date"
onChange={handleDateChange}
value={selectedDate}
- className="p-2 border rounded"
+ className="p-2 border rounded focus:ring-2 focus:ring-blue-500"
autoFocus
+ aria-label="Select date"
+ min="1900-01-01"
+ max="2100-12-31"
+ required
/>📝 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.
| <input | |
| type="date" | |
| onChange={handleDateChange} | |
| value={selectedDate} | |
| className="p-2 border rounded" | |
| autoFocus | |
| /> | |
| <input | |
| type="date" | |
| onChange={handleDateChange} | |
| value={selectedDate} | |
| className="p-2 border rounded focus:ring-2 focus:ring-blue-500" | |
| autoFocus | |
| aria-label="Select date" | |
| min="1900-01-01" | |
| max="2100-12-31" | |
| required | |
| /> |
| const handleConfirm = () => { | ||
| if (socket && selectedDate) { | ||
| socket.emit('input:date', { | ||
| selector, | ||
| value: selectedDate | ||
| }); | ||
| onClose(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for socket emission
The socket event emission should include error handling to manage connection issues.
const handleConfirm = () => {
if (socket && selectedDate) {
+ try {
socket.emit('input:date', {
selector,
value: selectedDate
});
onClose();
+ } catch (error) {
+ console.error('Failed to emit date selection:', error);
+ // Consider adding user feedback for failed submission
+ }
}
};Committable suggestion skipped: line range outside the PR's diff.
src/components/atoms/canvas.tsx
Outdated
| useEffect(() => { | ||
| if (socket) { | ||
| socket.on('showDatePicker', (info: {coordinates: Coordinates, selector: string}) => { | ||
| setDatePickerInfo(info); | ||
| }); | ||
|
|
||
| return () => { | ||
| socket.off('showDatePicker'); | ||
| }; | ||
| } | ||
| }, [socket]); |
There was a problem hiding this comment.
Prevent potential memory leaks in socket effect
The socket effect should clean up properly when the component unmounts.
useEffect(() => {
+ let isSubscribed = true;
if (socket) {
socket.on('showDatePicker', (info: {coordinates: Coordinates, selector: string}) => {
+ if (isSubscribed) {
setDatePickerInfo(info);
+ }
});
return () => {
+ isSubscribed = false;
socket.off('showDatePicker');
};
}
}, [socket]);📝 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.
| useEffect(() => { | |
| if (socket) { | |
| socket.on('showDatePicker', (info: {coordinates: Coordinates, selector: string}) => { | |
| setDatePickerInfo(info); | |
| }); | |
| return () => { | |
| socket.off('showDatePicker'); | |
| }; | |
| } | |
| }, [socket]); | |
| useEffect(() => { | |
| let isSubscribed = true; | |
| if (socket) { | |
| socket.on('showDatePicker', (info: {coordinates: Coordinates, selector: string}) => { | |
| if (isSubscribed) { | |
| setDatePickerInfo(info); | |
| } | |
| }); | |
| return () => { | |
| isSubscribed = false; | |
| socket.off('showDatePicker'); | |
| }; | |
| } | |
| }, [socket]); |
| {datePickerInfo && ( | ||
| <DatePicker | ||
| coordinates={datePickerInfo.coordinates} | ||
| selector={datePickerInfo.selector} | ||
| onClose={() => setDatePickerInfo(null)} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for DatePicker component
Wrap the DatePicker in an error boundary to prevent crashes from affecting the canvas.
+ class DatePickerErrorBoundary extends React.Component {
+ state = { hasError: false };
+ static getDerivedStateFromError(error) {
+ return { hasError: true };
+ }
+ render() {
+ if (this.state.hasError) {
+ return null;
+ }
+ return this.props.children;
+ }
+ }
{datePickerInfo && (
+ <DatePickerErrorBoundary>
<DatePicker
coordinates={datePickerInfo.coordinates}
selector={datePickerInfo.selector}
onClose={() => setDatePickerInfo(null)}
/>
+ </DatePickerErrorBoundary>
)}Committable suggestion skipped: line range outside the PR's diff.
amhsirak
changed the title
Adding functionality to handle different user inputs:
closes: #159