refac: meetings page #674
refac: meetings page #674
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several updates across multiple components within the frontend application. Key changes include enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 9
🧹 Outside diff range and nitpick comments (21)
apps/frontend/src/components/meetings/MeetNotes.tsx (1)
20-26: Consider including all usedmeetDataproperties in the dependency array.Currently, the
useEffectdepends on[meetData?.id], but accesses other properties likemeetData.titleandmeetData.description. To ensure the effect runs when any relevantmeetDataproperty changes, consider includingmeetDatain the dependency array.Apply this diff to update the dependency array:
- }, [meetData?.id]) + }, [meetData])apps/frontend/src/components/header/meta-details.tsx (1)
11-13: Handle cases whereurlis undefined to prevent rendering issues.Since
urlis optional, if it's undefined, the anchor tag'shrefwill beundefined, which may cause unexpected behavior. Consider conditionally rendering the anchor tag only whenurlis provided.Apply this diff to handle undefined
url:<Details> - <a href={url} target="_blank" rel="noopener noreferrer" className="flex items-center gap-2"> - <span>{url}</span>, {duration} - </a> + {url ? ( + <a href={url} target="_blank" rel="noopener noreferrer" className="flex items-center gap-2"> + <span>{url}</span>, {duration} + </a> + ) : ( + <span>{duration}</span> + )} </Details>apps/frontend/src/app/api/auth/google-calendar/url/route.ts (1)
6-6: Remove debuggingconsole.logstatement.The
console.log("hi from api")appears to be for debugging and should be removed to clean up console output in production.Apply this diff to remove the statement:
export async function GET(request: NextRequest) { - console.log("hi from api")apps/frontend/src/components/header/action-header.tsx (1)
Line range hint
23-27: Handle the case whenonAddis undefined.Since
onAddis optional, if it's not provided, the "Add" button will have anonClickofundefined, which may lead to unexpected behavior. Consider disabling the button or conditionally rendering it whenonAddis available.Apply this diff to conditionally render the "Add" button:
{!loading ? ( + onAdd ? ( <button onClick={onAdd} className="hover-bg flex items-center gap-1 truncate rounded-md px-1 text-secondary-foreground" > <PlusIcon /> </button> + ) : null ) : ( <div className="flex items-center gap-1 rounded-md px-1 text-secondary-foreground"> <span>loading...</span> </div> )}apps/frontend/src/components/ActionHeader.tsx (2)
9-9: Consider using a more specific return type foronAddThe
Promise<any>type is too permissive and could lead to type-safety issues. Consider defining a more specific return type based on the actual value being returned.- onAdd: () => Promise<any> + onAdd: () => Promise<void> // or the specific return type
23-36: Enhance accessibility for loading stateThe loading state could be improved for better accessibility:
- Add aria-busy attribute
- Use a more semantic loading indicator
- <div className="flex items-center gap-1 rounded-md px-1 text-secondary-foreground"> - <span>loading...</span> + <div + className="flex items-center gap-1 rounded-md px-1 text-secondary-foreground" + role="status" + aria-busy="true" + > + <span className="sr-only">Loading</span> + <span aria-hidden="true">loading...</span>apps/frontend/src/components/meetings/useMeetState.ts (2)
9-26: Consider memoizing setter functionsSince these setter functions are passed down to child components, memoizing them with useCallback would prevent unnecessary re-renders.
+import { useReducer, useCallback } from "react" - const setMeet = (newMeet: Meet) => + const setMeet = useCallback((newMeet: Meet) => dispatch({ type: "SET_MEET", payload: newMeet }) + , []) - const setTitle = (newTitle: string) => + const setTitle = useCallback((newTitle: string) => dispatch({ type: "SET_TITLE", payload: newTitle }) + , [])
1-5: Add type definitions for action creatorsConsider adding TypeScript type definitions for the action creators to improve type safety and developer experience.
type MeetAction = | { type: "SET_MEET"; payload: Meet } | { type: "SET_TITLE"; payload: string } | { type: "SET_CONTENT"; payload: string } // ... other action types const setMeet = useCallback((newMeet: Meet): void => { dispatch({ type: "SET_MEET", payload: newMeet } as MeetAction) }, [])apps/frontend/src/components/modals/StackModal.tsx (1)
33-50: Enhance modal accessibility and performanceThe modal container needs accessibility improvements and potential performance optimizations:
- Add ARIA attributes for modal role
- Consider virtualizing the list if it can grow large
-<div className="fixed right-[150px] top-[38%] z-50 h-[535px] w-[371px] -translate-y-1/2 overflow-y-scroll rounded-lg border border-border bg-background p-6"> +<div + role="dialog" + aria-modal="true" + aria-label="Stack of Notes" + className="fixed right-[150px] top-[38%] z-50 h-[535px] w-[371px] -translate-y-1/2 overflow-y-auto rounded-lg border border-border bg-background p-6">apps/frontend/src/components/meetings/reducer.ts (2)
26-36: Document initial state choices and add error handlingThe initial state could benefit from documentation explaining certain choices (e.g., why
contentis initialized with"<p></p>"). Consider adding error handling state.const initialState: MeetState = { meet: null, title: "", content: "<p></p>", // Consider documenting why this specific initial value isSaved: true, isLoading: false, notFound: false, + error: null, // Add error state closeToggle: true, isInitialLoad: true, isEditingTitle: false, }
15-24: Consider adding error action typeThe action types could be extended to handle error states for better error management.
type Action = | { type: "SET_MEET"; payload: Meet } | { type: "SET_TITLE"; payload: string } | { type: "SET_CONTENT"; payload: string } | { type: "SET_SAVED"; payload: boolean } | { type: "SET_LOADING"; payload: boolean } | { type: "SET_NOT_FOUND"; payload: boolean } | { type: "SET_CLOSED"; payload: boolean } | { type: "SET_INITIAL_LOAD"; payload: boolean } - | { type: "SET_EDITING_TITLE"; payload: boolean } + | { type: "SET_EDITING_TITLE"; payload: boolean } + | { type: "SET_ERROR"; payload: Error | null }apps/frontend/src/utils/meet.ts (1)
40-42: Consider using date-fns for duration calculationSince the codebase already uses date-fns (imported at the top), consider using its
differenceInMinutesfunction for consistency and reliability.- const minutes = Math.round( - (endDate.getTime() - startDate.getTime()) / (1000 * 60) - ) + const minutes = differenceInMinutes(endDate, startDate)apps/frontend/src/components/editor/space-editor.tsx (1)
10-20: Improve TypeScript type safety and props consistencyThe props interface could be improved in the following ways:
- Add explicit return type for handlers
- Be consistent with optional props - some handlers are optional while others aren't, but there's no clear pattern why
interface SpaceEditorProps { note: Meet title: string editor: Editor | null - handleTitleChange: (e: React.ChangeEvent<HTMLTextAreaElement>) => void - handleTextareaKeyDown: (e: React.KeyboardEvent<HTMLTextAreaElement>) => void - handleTitleFocus?: () => void - handleTitleBlur?: () => void - handleSaveNote?: () => void + handleTitleChange: (e: React.ChangeEvent<HTMLTextAreaElement>) => Promise<void> + handleTextareaKeyDown: (e: React.KeyboardEvent<HTMLTextAreaElement>) => void + handleTitleFocus: () => void + handleTitleBlur: () => void + handleSaveNote: () => Promise<void> textareaRef: React.RefObject<HTMLTextAreaElement> }apps/frontend/src/components/meetings/useMeetHandlers.ts (2)
12-21: Consider adding retry logic for server operationsThe
saveMeetToServerfunction could benefit from retry logic for transient failures, especially since it's a network operation.const saveMeetToServer = useCallback( async (meet: Meet): Promise<void> => { + const maxRetries = 3; + let retries = 0; try { - await updateMeet(session, meet, meet.id) + while (retries < maxRetries) { + try { + await updateMeet(session, meet, meet.id) + break + } catch (error) { + retries++ + if (retries === maxRetries) throw error + await new Promise(resolve => setTimeout(resolve, 1000 * retries)) + } + } } catch (error) { console.error("Error saving meet:", error) + throw error } }, [session, updateMeet] )
23-69: Reduce code duplication in handlersThe
handleSaveContentandhandleSaveTitlefunctions share similar structure. Consider extracting the common logic into a reusable function.+ const handleSaveField = useCallback( + async (fieldName: 'title' | 'description', value: string) => { + if (!meet) return + try { + dispatch({ type: "SET_SAVED", payload: false }) + await updateMeet(session, { ...meet, [fieldName]: value }, meet.id) + dispatch({ type: "SET_SAVED", payload: true }) + } catch (error) { + console.error(`Error saving ${fieldName}:`, error) + } + }, + [meet, session, updateMeet, dispatch] + ) const handleSaveTitle = useCallback( async (newTitle: string) => { - if (!meet) return - try { - dispatch({ type: "SET_SAVED", payload: false }) - await updateMeet(session, { ...meet, title: newTitle }, meet.id) - dispatch({ type: "SET_SAVED", payload: true }) - } catch (error) { - console.error("Error saving title:", error) - } + await handleSaveField('title', newTitle) }, - [meet, session, updateMeet, dispatch] + [handleSaveField] )apps/frontend/src/components/meetings/MeetingsPage.tsx (2)
26-28: Consider using a more descriptive state nameThe
closeTogglestate name could be more descriptive. Consider something likeisStackModalVisibleto better reflect its purpose.- const [closeToggle, setCloseToggle] = usePersistedState("closeToggle", true) + const [isStackModalVisible, setIsStackModalVisible] = usePersistedState("isStackModalVisible", false)
74-76: Add loading skeleton for better UXInstead of showing a simple "loading..." text, consider implementing a loading skeleton to improve user experience.
- if (isLoading && meets.length === 0) { - return <div>loading...</div> - } + if (isLoading && meets.length === 0) { + return ( + <div className="flex size-full gap-16 bg-background p-10 pl-60"> + <div className="animate-pulse space-y-4"> + <div className="h-4 w-48 bg-gray-200 rounded"></div> + <div className="h-32 w-96 bg-gray-200 rounded"></div> + </div> + </div> + ) + }apps/frontend/src/components/meetings/InitialMeet.tsx (1)
Line range hint
21-115: Consider consolidating loading statesThe component has multiple loading states (
loading,isLoading,userLoading) which could be consolidated for better maintainability. Consider creating a custom hook to manage loading states.Example approach:
const useLoadingState = () => { const [loading, setLoading] = useState(true) const { isLoading: userLoading } = useUserStore() const isLoading = loading || userLoading return { loading, setLoading, isLoading } }apps/frontend/src/components/Notes/NotesPage.tsx (3)
126-130: Consider adding type definitions for ActionHeader propsThe ActionHeader component is used without visible prop types. Consider adding explicit type definitions for better type safety and documentation.
interface ActionHeaderProps { closeToggle: boolean loading: boolean onAdd: () => void onClose: () => void }
Line range hint
21-164: Consider breaking down NotesPage componentThe component has grown quite large and handles multiple responsibilities (note editing, state management, UI rendering). Consider breaking it down into smaller, more focused components.
Suggested structure:
- NotePageContainer (main layout and state management)
- NoteEditorContainer (editor-specific logic)
- NoteHeaderSection (header and actions)
- NoteSidePanel (stack modal logic)
Line range hint
126-164: Optimize performance with useMemo for handlersConsider memoizing the event handlers passed to ActionHeader to prevent unnecessary re-renders.
+ const memoizedHandlers = useMemo(() => ({ + onAdd: addNewNote, + onClose: handleClose + }), [addNewNote, handleClose]); <ActionHeader closeToggle={closeToggle} loading={loading} - onAdd={addNewNote} - onClose={handleClose} + {...memoizedHandlers} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/frontend/src/app/(app)/spaces/[spaceId]/blocks/[blockId]/items/[itemId]/page.tsx(1 hunks)apps/frontend/src/app/api/auth/google-calendar/url/route.ts(1 hunks)apps/frontend/src/components/ActionHeader.tsx(1 hunks)apps/frontend/src/components/Notes/NotesPage.tsx(2 hunks)apps/frontend/src/components/Notes/components/NoteModal/NotesModal.tsx(1 hunks)apps/frontend/src/components/editor/space-editor.tsx(1 hunks)apps/frontend/src/components/header/action-header.tsx(2 hunks)apps/frontend/src/components/header/meta-details.tsx(1 hunks)apps/frontend/src/components/meetings/InitialMeet.tsx(1 hunks)apps/frontend/src/components/meetings/MeetNotes.tsx(2 hunks)apps/frontend/src/components/meetings/MeetingsPage.tsx(1 hunks)apps/frontend/src/components/meetings/Stack.tsx(0 hunks)apps/frontend/src/components/meetings/reducer.ts(1 hunks)apps/frontend/src/components/meetings/useMeetEffects.ts(1 hunks)apps/frontend/src/components/meetings/useMeetHandlers.ts(1 hunks)apps/frontend/src/components/meetings/useMeetState.ts(1 hunks)apps/frontend/src/components/modals/StackModal.tsx(1 hunks)apps/frontend/src/utils/meet.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/frontend/src/components/meetings/Stack.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/src/components/Notes/components/NoteModal/NotesModal.tsx
🔇 Additional comments (8)
apps/frontend/src/components/meetings/MeetNotes.tsx (3)
5-7: Imports of new hooks are correctly added.
The new hooks useMeetEffects, useMeetHandlers, and useMeetState are imported properly, enhancing modularity and code organization.
13-13: Props are explicitly typed for MeetNotes component.
Defining the prop types enhances type safety and code clarity, preventing potential type-related bugs.
91-100: Props passed to SpaceEditor are appropriate.
The SpaceEditor component is correctly utilized with the necessary props, ensuring seamless integration.
apps/frontend/src/app/api/auth/google-calendar/url/route.ts (1)
10-10:
Verify the exposure of GOOGLE_CLIENT_ID to the client.
Changing process.env.GOOGLE_CLIENT_ID to process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID exposes the client ID to the browser. Ensure that this is intentional and doesn't introduce security concerns.
Run the following script to check client-side usage of NEXT_PUBLIC_GOOGLE_CLIENT_ID:
✅ Verification successful
Using NEXT_PUBLIC_GOOGLE_CLIENT_ID is appropriate in this context
The usage of NEXT_PUBLIC_GOOGLE_CLIENT_ID is correct and secure in this context because:
- The client ID is used in two places:
- In
layout.tsxfor Google OAuth client-side integration - In the server-side route
url/route.tsto generate the OAuth authorization URL
- In
Google Client IDs are meant to be public and are designed to be exposed in client-side code for OAuth flows. The critical secret that must be kept private is the client secret, which is correctly handled in the backend (apps/backend/src/loaders/google.loader.js) using the non-public GOOGLE_CLIENT_ID environment variable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of NEXT_PUBLIC_GOOGLE_CLIENT_ID in the client-side code.
rg 'NEXT_PUBLIC_GOOGLE_CLIENT_ID' --type js --type ts --type jsx --type tsx
Length of output: 103
Script:
#!/bin/bash
# Search for usages of NEXT_PUBLIC_GOOGLE_CLIENT_ID with ripgrep, without file type filters
rg "NEXT_PUBLIC_GOOGLE_CLIENT_ID"
# Also search for GOOGLE_CLIENT_ID to understand the context of its usage
rg "GOOGLE_CLIENT_ID"
# Let's also check the file content to understand the context better
cat "apps/frontend/src/app/api/auth/google-calendar/url/route.ts"
Length of output: 2437
apps/frontend/src/components/header/action-header.tsx (1)
6-18: Component props have been updated appropriately.
The ActionHeader component's props have been renamed and updated to improve clarity and reusability.
apps/frontend/src/components/ActionHeader.tsx (1)
26-26: Verify custom CSS classes
The hover-bg and hover-text classes are used but may need verification to ensure they're properly defined in your CSS system.
Also applies to: 38-40
✅ Verification successful
Let me check the actual CSS definitions to ensure these classes are properly defined.
CSS classes are properly defined and functional
The hover-bg and hover-text classes are correctly defined in apps/frontend/src/styles/main.css:
hover-bgapplies Tailwind'shover:bg-background-hoverandactive:bg-background-activestyleshover-textapplies Tailwind'shover:text-primary-foregroundandactive:text-foregroundstyles
Both classes are properly integrated with the theme system using CSS variables for colors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hover-bg and hover-text class definitions
rg -g '*.css' -g '*.scss' -g '*.module.*' 'hover-(bg|text)'
Length of output: 486
Script:
#!/bin/bash
# Get the content of the CSS file to see the actual definitions
cat apps/frontend/src/styles/main.css
Length of output: 5049
apps/frontend/src/app/(app)/spaces/[spaceId]/blocks/[blockId]/items/[itemId]/page.tsx (1)
35-35: LGTM! Verify prop usage in MeetingPage
The addition of spaceId and blockId props maintains consistency with the NotesPage implementation.
✅ Verification successful
Props are properly typed and used in MeetingPage component
The verification confirms that spaceId and blockId props are:
- Correctly defined in the
MeetingPagePropsinterface - Properly passed to the component
- Utilized within the component for generating note links in
simplifiedNotes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify MeetingPage component properly handles these props
ast-grep --pattern 'interface $interface {
$$$
spaceId: string
blockId: string
$$$
}' src/components/meetings/MeetingsPage.tsx
Length of output: 224
Script:
#!/bin/bash
# First, let's find the correct path for MeetingPage component
fd -t f "Meeting.*\.(tsx|ts)"
# Then, let's search for prop interfaces or type definitions
ast-grep --pattern 'interface $interface {
$$$
meetId: string
$$$
}'
# Also search for direct prop destructuring
ast-grep --pattern 'function $name({ $$$, spaceId, blockId, $$$}'
# Backup search with ripgrep
rg "MeetingPage.*=.*{.*spaceId.*blockId.*}" -A 5
Length of output: 512
Script:
#!/bin/bash
# Let's examine the MeetingsPage component content
cat apps/frontend/src/components/meetings/MeetingsPage.tsx
# Also search for MeetingPage usage with better regex
rg "MeetingPage.*props" -A 3
# And check prop types/interfaces with a more specific pattern
ast-grep --pattern 'type $name = {
$$$
spaceId: string
$$$
}'
ast-grep --pattern 'export function MeetingsPage($props) {
$$$
}'
Length of output: 3065
apps/frontend/src/components/editor/space-editor.tsx (1)
64-64: LGTM! Good use of memoization
Memoizing the component is a good practice to prevent unnecessary re-renders, especially for an editor component.
| useEffect(() => { | ||
| if (!meetData?._id || meetData._id === state.meet?._id) return | ||
|
|
||
| dispatch({ type: "SET_MEET", payload: meetData }) | ||
| dispatch({ type: "SET_TITLE", payload: meetData.title || "" }) | ||
| dispatch({ | ||
| type: "SET_CONTENT", | ||
| payload: meetData.description || "<p></p>", | ||
| }) | ||
| }, [meetData?._id, state.meet?._id]) // Only depend on IDs changing |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor redundant useEffect hooks for initializing state.
There are two useEffect hooks initializing state when meetData changes. Combining them improves readability and prevents potential inconsistencies.
Apply this diff to combine the useEffect hooks:
-useEffect(() => {
- if (!meetData?._id || meetData._id === state.meet?._id) return
-
- dispatch({ type: "SET_MEET", payload: meetData })
- dispatch({ type: "SET_TITLE", payload: meetData.title || "" })
- dispatch({
- type: "SET_CONTENT",
- payload: meetData.description || "<p></p>",
- })
-}, [meetData?._id, state.meet?._id]) // Only depend on IDs changing
-
-// Initialize state once when meetData changes
-useEffect(() => {
- if (!meetData || meet?.id === meetData.id) return
-
- const newContent = meetData.description || "<p></p>"
- dispatch({ type: "SET_MEET", payload: meetData })
- dispatch({ type: "SET_TITLE", payload: meetData.title || "" })
- dispatch({ type: "SET_CONTENT", payload: newContent })
-}, [meetData?.id]) // Only depend on the ID changing
+useEffect(() => {
+ if (!meetData || meetData._id === state.meet?._id) return
+
+ const newContent = meetData.description || "<p></p>"
+ dispatch({ type: "SET_MEET", payload: meetData })
+ dispatch({ type: "SET_TITLE", payload: meetData.title || "" })
+ dispatch({ type: "SET_CONTENT", payload: newContent })
+}, [meetData]) // Depend on the entire meetData objectAlso applies to: 48-55
| const MetaDetails: React.FC<NoteDetailsProps> = ({ url, duration }) => { | ||
| return ( | ||
| <Details> | ||
| <a href={url} target="_blank" className="flex items-center gap-2"> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to the anchor tag.
When using target="_blank", it's recommended to include rel="noopener noreferrer" to prevent security risks like reverse tabnabbing.
Apply this diff to fix the issue:
- <a href={url} target="_blank" className="flex items-center gap-2">
+ <a href={url} target="_blank" rel="noopener noreferrer" className="flex items-center gap-2">📝 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.
| <a href={url} target="_blank" className="flex items-center gap-2"> | |
| <a href={url} target="_blank" rel="noopener noreferrer" className="flex items-center gap-2"> |
| <div> | ||
| <div | ||
| className="fixed inset-0 z-50 cursor-default" | ||
| role="button" | ||
| onClick={handleClose} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape" || e.key === "Esc") { | ||
| handleClose() | ||
| } | ||
| }} | ||
| tabIndex={0} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for improved accessibility
The modal backdrop should have proper ARIA attributes for better screen reader support.
<div>
<div
className="fixed inset-0 z-50 cursor-default"
role="button"
+ aria-hidden="true"
onClick={handleClose}
onKeyDown={(e) => {📝 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.
| <div> | |
| <div | |
| className="fixed inset-0 z-50 cursor-default" | |
| role="button" | |
| onClick={handleClose} | |
| onKeyDown={(e) => { | |
| if (e.key === "Escape" || e.key === "Esc") { | |
| handleClose() | |
| } | |
| }} | |
| tabIndex={0} | |
| /> | |
| <div> | |
| <div | |
| className="fixed inset-0 z-50 cursor-default" | |
| role="button" | |
| aria-hidden="true" | |
| onClick={handleClose} | |
| onKeyDown={(e) => { | |
| if (e.key === "Escape" || e.key === "Esc") { | |
| handleClose() | |
| } | |
| }} | |
| tabIndex={0} | |
| /> |
| export const calculateMeetDuration = (start: string, end: string): string => { | ||
| const startDate = new Date(start) | ||
| const endDate = new Date(end) | ||
|
|
||
| const minutes = Math.round( | ||
| (endDate.getTime() - startDate.getTime()) / (1000 * 60) | ||
| ) | ||
|
|
||
| if (minutes < 60) { | ||
| return `${minutes}min` | ||
| } | ||
|
|
||
| const hours = Math.floor(minutes / 60) | ||
| const remainingMinutes = minutes % 60 | ||
|
|
||
| if (remainingMinutes === 0) { | ||
| return `${hours}h` | ||
| } | ||
|
|
||
| return `${hours}h ${remainingMinutes}min` | ||
| } |
There was a problem hiding this comment.
Add input validation and handle edge cases
The calculateMeetDuration function needs input validation and edge case handling:
- Validate input strings
- Handle invalid dates
- Handle negative durations
- Handle same start/end times
export const calculateMeetDuration = (start: string, end: string): string => {
+ if (!start || !end) {
+ throw new Error('Start and end times are required');
+ }
+
const startDate = new Date(start)
const endDate = new Date(end)
+ if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) {
+ throw new Error('Invalid date format');
+ }
+
const minutes = Math.round(
(endDate.getTime() - startDate.getTime()) / (1000 * 60)
)
+ if (minutes < 0) {
+ throw new Error('End time must be after start time');
+ }
+
+ if (minutes === 0) {
+ return '0min';
+ }📝 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.
| export const calculateMeetDuration = (start: string, end: string): string => { | |
| const startDate = new Date(start) | |
| const endDate = new Date(end) | |
| const minutes = Math.round( | |
| (endDate.getTime() - startDate.getTime()) / (1000 * 60) | |
| ) | |
| if (minutes < 60) { | |
| return `${minutes}min` | |
| } | |
| const hours = Math.floor(minutes / 60) | |
| const remainingMinutes = minutes % 60 | |
| if (remainingMinutes === 0) { | |
| return `${hours}h` | |
| } | |
| return `${hours}h ${remainingMinutes}min` | |
| } | |
| export const calculateMeetDuration = (start: string, end: string): string => { | |
| if (!start || !end) { | |
| throw new Error('Start and end times are required'); | |
| } | |
| const startDate = new Date(start) | |
| const endDate = new Date(end) | |
| if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { | |
| throw new Error('Invalid date format'); | |
| } | |
| const minutes = Math.round( | |
| (endDate.getTime() - startDate.getTime()) / (1000 * 60) | |
| ) | |
| if (minutes < 0) { | |
| throw new Error('End time must be after start time'); | |
| } | |
| if (minutes === 0) { | |
| return '0min'; | |
| } | |
| if (minutes < 60) { | |
| return `${minutes}min` | |
| } | |
| const hours = Math.floor(minutes / 60) | |
| const remainingMinutes = minutes % 60 | |
| if (remainingMinutes === 0) { | |
| return `${hours}h` | |
| } | |
| return `${hours}h ${remainingMinutes}min` | |
| } |
| textareaRef, | ||
| }: SpaceEditorProps) => { | ||
| return ( | ||
| <div onBlur={handleSaveNote}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider save trigger implementation
The onBlur event on the outer div could trigger unexpected saves when clicking between inner elements. Consider moving the save trigger to specific user actions or using a debounced auto-save approach.
| /* eslint-disable-next-line jsx-a11y/no-autofocus */ | ||
| autoFocus={!title || title.trim() === ""} |
There was a problem hiding this comment.
💡 Codebase verification
Consider removing autoFocus or adding skip navigation
The codebase shows consistent use of autoFocus in input components, primarily in "add item" or editor scenarios. While this pattern improves usability for keyboard users, it can be disorienting for screen reader users. Consider either:
- Removing autoFocus and letting users manually focus using standard navigation
- Adding a skip navigation link that allows screen reader users to bypass these auto-focused inputs
- Limiting autoFocus to only the most critical scenarios (e.g., main editor) rather than all input fields
Affected files:
- apps/frontend/src/components/Inbox/InboxAddItem.tsx
- apps/frontend/src/components/Notes/components/NoteEditor/NoteEditor.tsx
- apps/frontend/src/components/Reading/AddItemForm.tsx
- apps/frontend/src/components/atoms/ItemAdd.tsx
🔗 Analysis chain
Review accessibility implications of autoFocus
While the eslint rule is disabled, consider if autoFocus is really necessary here. It can be disorienting for screen reader users and should be used sparingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other autoFocus usage in the codebase
rg -i "autoFocus.*=.*true" --type=tsx
Length of output: 67
Script:
#!/bin/bash
# Check for autoFocus usage in the codebase with proper file extension
rg -i "autoFocus.*=.*" --type=typescript --type=tsx -A 2
Length of output: 93
Script:
#!/bin/bash
# Find all TypeScript/TSX files that might contain autoFocus
fd -e tsx -e ts | xargs grep -l "autoFocus"
Length of output: 320
Script:
#!/bin/bash
# Use ast-grep to find JSX attributes with autoFocus
ast-grep --pattern 'autoFocus={$_}'
Length of output: 37
Script:
#!/bin/bash
# Check the content of files that use autoFocus to understand the context
for file in apps/frontend/src/components/Inbox/InboxAddItem.tsx \
apps/frontend/src/components/Notes/components/NoteEditor/NoteEditor.tsx \
apps/frontend/src/components/Reading/AddItemForm.tsx \
apps/frontend/src/components/atoms/ItemAdd.tsx; do
echo "=== $file ==="
rg -A 3 -B 3 "autoFocus" "$file"
done
Length of output: 3926
Script:
#!/bin/bash
# Check if there are any accessibility-related comments or patterns near autoFocus
rg -i "a11y|accessibility|screen.?reader" --type=ts --type=tsx -A 2 -B 2
Length of output: 102
| export const useMeetHandlers = (state, dispatch) => { | ||
| const { session } = useAuth() | ||
| const { updateMeet } = useMeetsStore() | ||
| const { meet, title, content, isSaved } = state | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types for state and dispatch parameters
The hook parameters lack TypeScript types, which could lead to runtime errors. Consider adding proper types for better type safety.
- export const useMeetHandlers = (state, dispatch) => {
+ interface MeetState {
+ meet: Meet | null
+ title: string
+ content: string
+ isSaved: boolean
+ }
+ type MeetDispatch = React.Dispatch<{ type: string; payload: any }>
+ export const useMeetHandlers = (state: MeetState, dispatch: MeetDispatch) => {Committable suggestion skipped: line range outside the PR's diff.
| const initialize = async () => { | ||
| if (!session || !meetId) return | ||
|
|
||
| setLoading(true) | ||
| try { | ||
| await Promise.all([ | ||
| !currentMeeting || currentMeeting.id !== meetId | ||
| ? fetchMeetByid(session, meetId) | ||
| : Promise.resolve(), | ||
| meets.length === 0 ? fetchMeets(session) : Promise.resolve(), | ||
| ]) | ||
| // Fetch meets list if needed | ||
| if (meets.length === 0) { | ||
| await fetchMeets(session) | ||
| } | ||
| // Fetch specific meet | ||
| await fetchMeetByid(session, meetId) | ||
| } finally { | ||
| setInitialLoading(false) | ||
| if (mounted) { | ||
| setLoading(false) | ||
| setIsInitialLoad(false) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and user feedback
The initialization process should handle errors gracefully and provide feedback to users when operations fail.
const initialize = async () => {
if (!session || !meetId) return
setLoading(true)
try {
if (meets.length === 0) {
await fetchMeets(session)
}
await fetchMeetByid(session, meetId)
} catch (error) {
+ console.error('Failed to initialize meeting:', error)
+ // Consider adding a toast notification or error state
+ if (mounted) {
+ setError(error.message)
+ }
} finally {
if (mounted) {
setLoading(false)
setIsInitialLoad(false)
}
}
}Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <p className="size-full overflow-auto bg-background px-8 text-secondary-foreground"> | ||
| loading... | ||
| </p> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to loading states
The loading state should include proper accessibility attributes to improve screen reader support.
- return (
- <p className="size-full overflow-auto bg-background px-8 text-secondary-foreground">
+ return (
+ <p
+ className="size-full overflow-auto bg-background px-8 text-secondary-foreground"
+ role="status"
+ aria-live="polite"
+ >
loading...
</p>
)📝 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.
| return ( | |
| <p className="size-full overflow-auto bg-background px-8 text-secondary-foreground"> | |
| loading... | |
| </p> | |
| ) | |
| return ( | |
| <p | |
| className="size-full overflow-auto bg-background px-8 text-secondary-foreground" | |
| role="status" | |
| aria-live="polite" | |
| > | |
| loading... | |
| </p> | |
| ) |
What did you ship?
Fixes:
Checklist:
OR: