Rollback Changes + Implement the UI, Desktop and PWA Changes #13
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe updates introduce significant improvements focused on mobile responsiveness, drag-and-drop control, and UI polish. A custom React hook detects mobile devices to disable drag-and-drop in the main todo list, with corresponding changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeClient
participant TodoList
participant useIsMobile
participant Server
User->>HomeClient: Load app
HomeClient->>useIsMobile: Check screen width
useIsMobile-->>HomeClient: isMobile flag
HomeClient->>TodoList: Render with disableDrag = isMobile
User->>TodoList: Attempt drag-and-drop
alt isMobile = true
TodoList-->>User: Drag-and-drop disabled
else isMobile = false
TodoList->>HomeClient: handleDragEnd event
HomeClient->>Server: Update todo order
Server-->>HomeClient: Response
end
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
components/SettingsDialog.tsx (1)
126-131: 🛠️ Refactor suggestionValidate numeric input & enforce the declared
minvalue
parseInt(e.target.value)will happily return0,NaN, or any other number below the<Input min={1} …>constraint if users clear the field or type non-digits.
Consider clamping the parsed value (or falling back to1) so the component state can never drift below the UI constraint.- reminderMinutes: parseInt(e.target.value) || 30 + const val = Math.max(1, parseInt(e.target.value) || 30) + reminderMinutes: valcomponents/todo-item.tsx (1)
136-186:⚠️ Potential issueReset
isReminderCommandon failure to avoid a stuck shine border
setIsReminderCommand(false)is only invoked inside thetryblock.
If the POST to/api/remindersfails, the catch‐branch adds an error comment but never clears the flag, soShineBorderwill keep pulsing indefinitely, misleading the user into thinking a reminder is still being created.- } catch (error) { + } catch (error) { console.error('Error creating reminder:', error); ... + // Clear the visual indicator even on failure + setIsReminderCommand(false); } finally { setIsProcessingReminder(false); }
🧹 Nitpick comments (6)
components/RemindersDialog.tsx (1)
93-101:90vhdialog height may collide with mobile safe-area / on-screen keyboardCapping to
h-[90vh]can push the confirm buttons under iOS/Android home-indicator or the soft-keyboard.
Addingpb-[env(safe-area-inset-bottom)](orsafe bottomclasses offered by Tailwind 3.4+) keeps critical UI reachable.components/todo-list.tsx (1)
198-282:StaticColumnkeeps dead code & unused state
hasOverflow,opacityand the bounce indicator are now commented out, but the expensiveResizeObserveranduseScrollhooks still run on every render.
Removing them (or re-enabling the indicator) will save work on low-power devices.desktop-build.js (2)
154-166:originalWebpack(config)signature mismatch – may drop build-time params
next.config’s custom webpack fn receives(config, options).
CallingoriginalWebpack(config)omits theoptionsargument, potentially breaking plugins that rely on it.- config = originalWebpack(config); + config = originalWebpack(config, { ...arguments[1] });Also guard with
typeof originalWebpack === 'function'to avoid runtime errors when the parent config exports an object, not a function.
174-176: Redundant copy ofnext.config.mjsThe original
next.config.mjsis first copied (line 42) and then overwritten by the desktop-specific version (line 174).
You can skip the initial copy to save I/O and avoid confusion.app/HomeClient.tsx (2)
57-76:useIsMobileaddsresizelistener on every mount – consider debouncingFrequent window-resize events can fire dozens of times per second, triggering a full React render cascade because state updates every pixel change.
- window.addEventListener('resize', checkMobile); + const debounced = _.debounce(checkMobile, 150); // lodash/throttle utils + window.addEventListener('resize', debounced); ... - return () => window.removeEventListener('resize', checkMobile); + return () => window.removeEventListener('resize', debounced);At minimum, wrap the handler in
requestAnimationFrameor a small throttle to cut unnecessary renders on rapid resizing.
357-411: Extensive console logging – strip before productionThe reschedule flow logs >10 messages per invocation. While helpful during development, it bloats production bundles and clutters the JS console.
Recommend gating behind
process.env.NODE_ENV !== 'production'or removing entirely before release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/noise-light.pngis excluded by!**/*.png
📒 Files selected for processing (7)
app/HomeClient.tsx(2 hunks)components/RemindersDialog.tsx(3 hunks)components/SettingsDialog.tsx(5 hunks)components/todo-input.tsx(6 hunks)components/todo-item.tsx(8 hunks)components/todo-list.tsx(3 hunks)desktop-build.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
components/todo-list.tsx (1)
lib/db/schema.ts (1)
todos(76-86)
🔇 Additional comments (3)
components/todo-input.tsx (1)
124-127: Verify static asset paths referenced in pseudo-elements
after:bg-[url('/noise-light.png')]will 404 if the texture isn’t placed in/public/noise-light.pngat build time (both PWA and desktop build).
Please confirm the asset is present or adjust the path tonext/imageor a CDN.components/todo-item.tsx (1)
278-285: Nice visual polish on the completion toggleThe revised classes give the checkbox a much softer, modern look while still conveying state clearly.
No functional issues spotted here. 👍app/HomeClient.tsx (1)
57-76: Memory-safe but still SSR-safe 👍Good job guarding
windowaccess insideuseEffect; this keeps the hook SSR-friendly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/HomeClient.tsx (2)
53-76: ConsideruseLayoutEffectto avoid first-paint flicker when detecting mobile
useIsMobileinitialisesisMobiletofalseand only updates inside an asyncuseEffect.
During the first paint the component therefore believes it is on desktop, renders the draggable list, and only then re-renders as non-draggable once the effect runs. This can cause a perceptible layout jump and hydration warning flashes.-const useIsMobile = () => { - const [isMobile, setIsMobile] = useState(false); +const useIsMobile = () => { + // Initialise from `window.innerWidth` when possible to avoid a mismatch + const getMatch = () => + typeof window !== 'undefined' ? window.innerWidth < 768 : false; + + const [isMobile, setIsMobile] = useState(getMatch); + + // useLayoutEffect runs before paint → no flicker + useLayoutEffect(() => { + if (typeof window === 'undefined') return; + const => setIsMobile(getMatch()); + window.addEventListener('resize', onResize); + return () => window.removeEventListener('resize', onResize); + }, []); return isMobile; };This keeps the first render consistent with the eventual hydrated state.
136-156: Keyboard shortcut listener leaks if effect re-runs frequently
handleKeyDownis re-declared wheneverworkspaceschanges, leading to a remove-then-add cycle every render. Consider usinguseCallback+ empty-dependencyuseEffector add the listener just once and read the latest state via refs.This avoids excessive listener churn on large workspace lists.
desktop-build.js (1)
42-46: Redundant copy:next.config.mjsis overwritten later
The originalnext.config.mjsis copied into the temp dir (line 42) and overwritten at line 178 with the desktop variant.
Removing the first copy eliminates one FS operation and avoids confusion.-fs.copyFileSync('next.config.mjs', path.join(tempDir, 'next.config.mjs'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/HomeClient.tsx(2 hunks)components/todo-list.tsx(3 hunks)desktop-build.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/todo-list.tsx
🔇 Additional comments (1)
desktop-build.js (1)
158-172:process/browseralias requires the polyfill to be installed
The alias relies onprocess/browser(provided byprocesspackage). Ensure it is declared inpackage.jsondependencies; otherwise the export build will fail at module resolution.If the alias is only needed for a minority of packages, consider importing
processexplicitly where required instead of a global alias.
app/HomeClient.tsx
Outdated
| } | ||
|
|
||
| if (columnIndex === 0) { | ||
| // Today's column | ||
| newDueDate = new Date(); | ||
| } else { | ||
| // Upcoming column | ||
| newDueDate.setDate(newDueDate.getDate() + 1); | ||
| } | ||
| } | ||
|
|
||
| // Create new array with updated todo | ||
| const newTodos = todos.filter(t => t.id !== draggableId); | ||
| newTodos.splice(destination.index, 0, updatedTodo); | ||
|
|
||
| // Update state | ||
| setTodos(newTodos); | ||
|
|
||
| // Log the update | ||
| console.log(`Todo "${todo.title}" moved to ${destination.droppableId} at index ${destination.index}`); | ||
|
|
||
| // Update the database after animations finish | ||
| if (session?.user) { | ||
| setTimeout(async () => { | ||
| try { | ||
| const res = await fetch('/api/todos', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ id: draggableId, dueDate: updatedTodo.dueDate }), | ||
| }); | ||
| if (!res.ok) throw new Error('Failed to update todo via drag-and-drop'); | ||
| const serverTodo = await res.json(); | ||
| // Sync state with server response | ||
| setTodos(prev => prev.map(t => t.id === draggableId ? serverTodo : t)); | ||
| console.log('✅ Todo dueDate updated on server via drag:', serverTodo); | ||
| } catch (error) { | ||
| console.error('❌ Error updating todo via drag:', error); | ||
| } | ||
| }, 350); | ||
| } | ||
| // Update the todo's due date | ||
| const updatedTodo = { | ||
| ...todo, | ||
| dueDate: newDueDate.toISOString() | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="min-h-screen flex flex-col bg-gray-100 dark:bg-[#09090B] text-gray-900 dark:text-white p-4 transition-colors duration-200"> | ||
| <div className="flex flex-row items-center justify-left"> | ||
| <Image src="/logo.png" alt="agenda.dev" width={32} height={32} className="mr-2" /> | ||
| <h1 className="text-xl">agenda.dev</h1> | ||
| </div> | ||
| <div className="relative mx-auto mb-4 flex items-center space-x-2 justify-center md:absolute md:top-4 md:right-4 md:mb-0 md:mx-0 md:justify-start"> | ||
| {session?.user && ( | ||
| <WorkspaceSwitcher | ||
| workspaces={workspaces} | ||
| currentWorkspace={currentWorkspace} | ||
| > | ||
| => setIsNewWorkspaceDialogOpen(true)} | ||
| > | ||
| todos={todos} | ||
| /> | ||
| )} | ||
| <CompletedToggle showCompleted={showCompleted} setShowCompleted={setShowCompleted} /> | ||
| <ViewToggle isTableView={isTableView} setIsTableView={setIsTableView} /> | ||
| <ThemeToggle /> | ||
| <FeedbackWidget /> | ||
| <LoginButton /> | ||
| </div> | ||
|
|
||
| // Create new array with updated todo | ||
| const newTodos = todos.filter(t => t.id !== draggableId); | ||
| newTodos.splice(destination.index, 0, updatedTodo); | ||
|
|
||
| // Update state | ||
| setTodos(newTodos); | ||
|
|
||
| // Log the update | ||
| console.log(`Todo "${todo.title}" moved to ${destination.droppableId} at index ${destination.index}`); | ||
|
|
||
| // Update the database after animations finish | ||
| if (session?.user) { | ||
| setTimeout(async () => { | ||
| try { | ||
| const res = await fetch('/api/todos', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ id: draggableId, dueDate: updatedTodo.dueDate }), | ||
| }); | ||
| if (!res.ok) throw new Error('Failed to update todo via drag-and-drop'); | ||
| const serverTodo = await res.json(); | ||
| // Sync state with server response | ||
| setTodos(prev => prev.map(t => t.id === draggableId ? serverTodo : t)); | ||
| console.log('✅ Todo dueDate updated on server via drag:', serverTodo); | ||
| } catch (error) { | ||
| console.error('❌ Error updating todo via drag:', error); | ||
| } | ||
| }, 350); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Date logic for ‘Next 7 days’ and ‘Upcoming’ columns is off & time-zone sensitive
- Dropping into column index
1adds only 1 day, not up to 7. - Column index
2adds 8 days unconditionally, skipping the 2-7-day window. new Date().toISOString()converts to UTC which may shift the date (e.g. 23:00-01:00). Storing an ISO date-time while the rest of the app treatsdueDateas a plain date string will create off-by-one-day errors.
-} else if (columnIndex === 1) {
- // Next 7 days column - set to tomorrow
- newDueDate.setDate(newDueDate.getDate() + 1);
-} else {
- // Upcoming column - set to next week
- newDueDate.setDate(newDueDate.getDate() + 8);
+} else if (columnIndex === 1) {
+ // Any day within the next 7 days – default to +3 days
+ newDueDate.setDate(newDueDate.getDate() + 3);
+} else {
+ // Upcoming (>7 days) – default to +14 days
+ newDueDate.setDate(newDueDate.getDate() + 14);
}
...
- dueDate: newDueDate.toISOString()
+ // Persist as date-only string to avoid TZ shifts
+ dueDate: newDueDate.toISOString().slice(0, 10)Please review the UX requirements and adjust the offsets / storage format accordingly.
Fix due date calculation and storage format
The current logic for the “Next 7 days” and “Upcoming” columns always jumps by exactly 1 day or 8 days, and storing via toISOString() can introduce off-by-one errors across time zones.
- Column index 1 (“Next 7 days”) should place the due date anywhere within the next 7 days, not strictly tomorrow.
- Column index 2 (“Upcoming”) should cover > 7 days, not always 8 days.
- Using
newDueDate.toISOString()persists a full UTC timestamp, which may shift the date when your UI treatsdueDateas a plain date.
Please adjust offsets to match your UX requirements and persist only a date-only string to avoid TZ shifts. For example:
-} else if (columnIndex === 1) {
- // Next 7 days column - set to tomorrow
- newDueDate.setDate(newDueDate.getDate() + 1);
-} else {
- // Upcoming column - set to next week
- newDueDate.setDate(newDueDate.getDate() + 8);
+} else if (columnIndex === 1) {
+ // Any day within the next 7 days – e.g. +3 days
+ newDueDate.setDate(newDueDate.getDate() + 3);
+} else {
+ // >7 days out – e.g. +14 days
+ newDueDate.setDate(newDueDate.getDate() + 14);
}
…
- dueDate: newDueDate.toISOString()
+ // Persist only the YYYY-MM-DD to prevent TZ-related shifts
+ dueDate: newDueDate.toISOString().substring(0, 10)Location: app/HomeClient.tsx lines 577–663
📝 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.
| const handleDragEnd = (result: DropResult) => { | |
| if (isMobile) return; // Prevent drag end handling on mobile | |
| const { destination, source, draggableId } = result; | |
| // If there's no destination or the item was dropped in its original position | |
| if (!destination || | |
| (destination.droppableId === source.droppableId && | |
| destination.index === source.index)) { | |
| return; | |
| } | |
| // Update the todo's due date | |
| const updatedTodo = { | |
| ...todo, | |
| dueDate: newDueDate.toISOString() | |
| }; | |
| // Find the todo that was dragged | |
| const todo = todos.find(t => t.id === draggableId); | |
| if (!todo) return; | |
| // Calculate new due date based on destination column | |
| let newDueDate = new Date(); | |
| if (destination.droppableId.startsWith('desktop')) { | |
| const columnIndex = Number.parseInt(destination.droppableId.split('-')[2] ?? '', 10); | |
| if (Number.isNaN(columnIndex)) { | |
| console.warn('Unhandled droppableId:', destination.droppableId); | |
| return; | |
| } | |
| if (columnIndex === 0) { | |
| // Today's column - keep current date | |
| newDueDate = new Date(); | |
| } else if (columnIndex === 1) { | |
| // Next 7 days column - set to tomorrow | |
| newDueDate.setDate(newDueDate.getDate() + 1); | |
| } else { | |
| // Upcoming column - set to next week | |
| newDueDate.setDate(newDueDate.getDate() + 8); | |
| } | |
| } else if (destination.droppableId.startsWith('tablet')) { | |
| const columnIndex = Number.parseInt(destination.droppableId.split('-')[2] ?? '', 10); | |
| if (Number.isNaN(columnIndex)) { | |
| console.warn('Unhandled droppableId:', destination.droppableId); | |
| return; | |
| } | |
| if (columnIndex === 0) { | |
| // Today's column | |
| newDueDate = new Date(); | |
| } else { | |
| // Upcoming column | |
| newDueDate.setDate(newDueDate.getDate() + 1); | |
| } | |
| } | |
| // Create new array with updated todo | |
| const newTodos = todos.filter(t => t.id !== draggableId); | |
| newTodos.splice(destination.index, 0, updatedTodo); | |
| // Update state | |
| setTodos(newTodos); | |
| // Log the update | |
| console.log(`Todo "${todo.title}" moved to ${destination.droppableId} at index ${destination.index}`); | |
| // Update the database after animations finish | |
| if (session?.user) { | |
| setTimeout(async () => { | |
| try { | |
| const res = await fetch('/api/todos', { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ id: draggableId, dueDate: updatedTodo.dueDate }), | |
| }); | |
| if (!res.ok) throw new Error('Failed to update todo via drag-and-drop'); | |
| const serverTodo = await res.json(); | |
| // Sync state with server response | |
| setTodos(prev => prev.map(t => t.id === draggableId ? serverTodo : t)); | |
| console.log('✅ Todo dueDate updated on server via drag:', serverTodo); | |
| } catch (error) { | |
| console.error('❌ Error updating todo via drag:', error); | |
| } | |
| }, 350); | |
| } | |
| // Update the todo's due date | |
| const updatedTodo = { | |
| ...todo, | |
| dueDate: newDueDate.toISOString() | |
| }; | |
| return ( | |
| <div className="min-h-screen flex flex-col bg-gray-100 dark:bg-[#09090B] text-gray-900 dark:text-white p-4 transition-colors duration-200"> | |
| <div className="flex flex-row items-center justify-left"> | |
| <Image src="/logo.png" alt="agenda.dev" width={32} height={32} className="mr-2" /> | |
| <h1 className="text-xl">agenda.dev</h1> | |
| </div> | |
| <div className="relative mx-auto mb-4 flex items-center space-x-2 justify-center md:absolute md:top-4 md:right-4 md:mb-0 md:mx-0 md:justify-start"> | |
| {session?.user && ( | |
| <WorkspaceSwitcher | |
| workspaces={workspaces} | |
| currentWorkspace={currentWorkspace} | |
| onSwitch={setCurrentWorkspace} | |
| onCreateNew={() => setIsNewWorkspaceDialogOpen(true)} | |
| onDelete={deleteWorkspace} | |
| todos={todos} | |
| /> | |
| )} | |
| <CompletedToggle showCompleted={showCompleted} setShowCompleted={setShowCompleted} /> | |
| <ViewToggle isTableView={isTableView} setIsTableView={setIsTableView} /> | |
| <ThemeToggle /> | |
| <FeedbackWidget /> | |
| <LoginButton /> | |
| </div> | |
| // Create new array with updated todo | |
| const newTodos = todos.filter(t => t.id !== draggableId); | |
| newTodos.splice(destination.index, 0, updatedTodo); | |
| // Update state | |
| setTodos(newTodos); | |
| // Log the update | |
| console.log(`Todo "${todo.title}" moved to ${destination.droppableId} at index ${destination.index}`); | |
| // Update the database after animations finish | |
| if (session?.user) { | |
| setTimeout(async () => { | |
| try { | |
| const res = await fetch('/api/todos', { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ id: draggableId, dueDate: updatedTodo.dueDate }), | |
| }); | |
| if (!res.ok) throw new Error('Failed to update todo via drag-and-drop'); | |
| const serverTodo = await res.json(); | |
| // Sync state with server response | |
| setTodos(prev => prev.map(t => t.id === draggableId ? serverTodo : t)); | |
| console.log('✅ Todo dueDate updated on server via drag:', serverTodo); | |
| } catch (error) { | |
| console.error('❌ Error updating todo via drag:', error); | |
| } | |
| }, 350); | |
| } | |
| }; | |
| const handleDragEnd = (result: DropResult) => { | |
| if (isMobile) return; // Prevent drag end handling on mobile | |
| const { destination, source, draggableId } = result; | |
| // If there's no destination or the item was dropped in its original position | |
| if ( | |
| !destination || | |
| (destination.droppableId === source.droppableId && destination.index === source.index) | |
| ) { | |
| return; | |
| } | |
| // Find the todo that was dragged | |
| const todo = todos.find(t => t.id === draggableId); | |
| if (!todo) return; | |
| // Calculate new due date based on destination column | |
| let newDueDate = new Date(); | |
| if (destination.droppableId.startsWith('desktop')) { | |
| const columnIndex = Number.parseInt(destination.droppableId.split('-')[2] ?? '', 10); | |
| if (Number.isNaN(columnIndex)) { | |
| console.warn('Unhandled droppableId:', destination.droppableId); | |
| return; | |
| } | |
| if (columnIndex === 0) { | |
| // Today's column - keep current date | |
| newDueDate = new Date(); | |
| } else if (columnIndex === 1) { | |
| // Any day within the next 7 days – default to +3 days | |
| newDueDate.setDate(newDueDate.getDate() + 3); | |
| } else { | |
| // Upcoming (>7 days) – default to +14 days | |
| newDueDate.setDate(newDueDate.getDate() + 14); | |
| } | |
| } else if (destination.droppableId.startsWith('tablet')) { | |
| const columnIndex = Number.parseInt(destination.droppableId.split('-')[2] ?? '', 10); | |
| if (Number.isNaN(columnIndex)) { | |
| console.warn('Unhandled droppableId:', destination.droppableId); | |
| return; | |
| } | |
| if (columnIndex === 0) { | |
| // Today's column | |
| newDueDate = new Date(); | |
| } else { | |
| // Upcoming column | |
| newDueDate.setDate(newDueDate.getDate() + 1); | |
| } | |
| } | |
| // Update the todo's due date | |
| const updatedTodo = { | |
| ...todo, | |
| // Persist only the YYYY-MM-DD to prevent TZ-related shifts | |
| dueDate: newDueDate.toISOString().substring(0, 10) | |
| }; | |
| // Create new array with updated todo | |
| const newTodos = todos.filter(t => t.id !== draggableId); | |
| newTodos.splice(destination.index, 0, updatedTodo); | |
| // Update state | |
| setTodos(newTodos); | |
| // Log the update | |
| console.log( | |
| `Todo "${todo.title}" moved to ${destination.droppableId} at index ${destination.index}` | |
| ); | |
| // Update the database after animations finish | |
| if (session?.user) { | |
| setTimeout(async () => { | |
| try { | |
| const res = await fetch('/api/todos', { | |
| method: 'PUT', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ id: draggableId, dueDate: updatedTodo.dueDate }), | |
| }); | |
| if (!res.ok) throw new Error('Failed to update todo via drag-and-drop'); | |
| const serverTodo = await res.json(); | |
| // Sync state with server response | |
| setTodos(prev => prev.map(t => (t.id === draggableId ? serverTodo : t))); | |
| console.log('✅ Todo dueDate updated on server via drag:', serverTodo); | |
| } catch (error) { | |
| console.error('❌ Error updating todo via drag:', error); | |
| } | |
| }, 350); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/HomeClient.tsx (5)
136-156: Consider improving keyboard shortcut ergonomicsThe current implementation requires both Ctrl AND Meta (Command) keys to be pressed together, which is an uncommon combination. Most applications use either Ctrl (Windows/Linux) OR Meta (macOS).
-if (e.ctrlKey && e.metaKey && !e.altKey && !e.shiftKey) { +if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey) {
289-299: Improve temporary ID generationThe current temporary ID approach is functional but could be more robust by using UUID when available.
-const tempId = `temp-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; +const tempId = `temp-${crypto.randomUUID?.() || `${Date.now()}-${Math.random().toString(36).substr(2, 9)}`}`;
364-419: Consider environment-aware loggingThe extensive console logging in the rescheduleTodo function is helpful for debugging but may clutter the console in production.
Consider wrapping detailed logs in a development-only conditional:
-console.log('🎯 Starting reschedule flow:', { id, newDate }) -console.log('📅 Previous due date:', todoToUpdate.dueDate) +if (process.env.NODE_ENV !== 'production') { + console.log('🎯 Starting reschedule flow:', { id, newDate }) + console.log('📅 Previous due date:', todoToUpdate.dueDate) +}
780-823: Large commented-out CommandPalette sectionThere's a substantial commented-out section for a CommandPalette component with a "Will add in later" comment. This could cause confusion about the component's status.
Consider:
- Moving this code to a separate branch if it's a work in progress
- Creating a TODO issue for tracking this feature
- Adding a more descriptive comment with a timeline or issue reference
159-249: Sync logic only runs on session changeThe server synchronization logic only runs when the session changes, not when todos are modified client-side. This could lead to inconsistencies if multiple devices are used simultaneously.
Consider adding a periodic sync mechanism or triggering sync after successful CRUD operations:
// Example: add periodic sync +useEffect(() => { + if (!session?.user) return; + + const syncInterval = setInterval(() => { + syncWithServer(); + }, 60000); // Sync every minute + + return () => clearInterval(syncInterval); +}, [session?.user]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/HomeClient.tsx(2 hunks)desktop-build.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- desktop-build.js
🔇 Additional comments (5)
app/HomeClient.tsx (5)
54-76: Well-implemented mobile detection hook!The
useIsMobilehook is cleanly implemented with proper event listener cleanup and SSR compatibility checking withtypeof window !== 'undefined'.
607-647: Date calculation logic improvements look goodThe improved date calculation logic for drag and drop operations now correctly:
- Uses safer Number.parseInt with radix and null coalescing
- Adds error handling for invalid column indices
- Uses more intuitive date ranges (today, today+3, today+14)
- Properly formats dates to avoid timezone issues
These changes effectively address previous concerns about date handling.
715-731: Good responsive layout implementationThe split input implementation with fixed positioning at the bottom for mobile and sticky positioning at the top for desktop creates an intuitive user experience on different devices.
750-750: Properly handling mobile drag-disablePassing the
isMobileflag to disable drag functionality on mobile devices improves usability on touch devices.
704-704: Commented-out ViewToggle componentThere's a commented ViewToggle component that suggests incomplete work or a temporarily disabled feature.
Is this intentional or should the ViewToggle be completely removed or re-enabled?
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
lib/auth.ts (1)
24-26:⚠️ Potential issueEnvironment check for Twitter creds is now misleading
You still throw when
TWITTER_CLIENT_ID/SECRETare missing, but the Twitter provider is commented out.
Drop the env-var guard or re-enable the provider to avoid blocking startup unnecessarily.
♻️ Duplicate comments (1)
app/HomeClient.tsx (1)
128-148:⚠️ Potential issue
computeNewDueDatereturns local dates that later get forced to UTC midnightThe helper returns a
Dateat local midnight.
Later (handleDragEnd, lines 760-763) the date is serialized with"T00:00:00.000Z"which shifts the day backwards/forwards for non-UTC users.Recommendation: persist date-only strings and let the server/UI interpret them in the user’s locale:
-const formattedDate = `${yyy}-${mm}-${dd}`; -const updatedTodo = { ...todo, dueDate: `${formattedDate}T00:00:00.000Z` }; +const formattedDate = `${yyy}-${mm}-${dd}`; // YYYY-MM-DD +const updatedTodo = { ...todo, dueDate: formattedDate };Make sure corresponding API routes accept the YYYY-MM-DD format.
🧹 Nitpick comments (14)
README.md (1)
19-23: Missing period after abbreviation "etc"In line 22, "etc" should include a period to follow proper American English conventions.
- - [ ] Add Stripe on trial/subscription end etc emails via resend. + - [ ] Add Stripe on trial/subscription end etc. emails via resend.🧰 Tools
🪛 LanguageTool
[style] ~22-~22: In American English, abbreviations like “etc.” require a period.
Context: ... ] Add Stripe on trial/subscription end etc emails via resend. ## Tech Stack - **...(ETC_PERIOD)
components/workspace-switcher.tsx (2)
101-105: Early-exit already covered – tighten the click handlerThe
onClickhandler callssetIsOpen(false)regardless ofcanCreateNew. Because the button is alsodisabled={!canCreateNew}, the innerifis redundant in practice.
You can simplify the handler to an early return for readability:- => { - if (canCreateNew) { - onCreateNew() - } - setIsOpen(false) -}} + => { + if (!canCreateNew) return; + onCreateNew(); + setIsOpen(false); +}}
106-110: Use a class helper instead of manual concatenationManual string concatenation is error-prone (e.g. missing whitespace) and harder to maintain.
Since the project already uses Tailwind, considerclsx,cn, or an array join:-import clsx from 'clsx' // if not present yet ... -className={ - `w-full ... gap-2 ` + - `${canCreateNew ? 'hover:bg-gray-100 ...' : 'opacity-50 cursor-not-allowed ...'}` -} +className={clsx( + 'w-full px-4 py-2 text-left text-sm transition-colors duration-200 flex items-center gap-2', + canCreateNew + ? 'hover:bg-gray-100 dark:hover:bg-white/5 text-gray-900 dark:text-white' + : 'opacity-50 cursor-not-allowed text-gray-500 dark:text-gray-600' +)}This keeps the Tailwind classes declarative and removes string-glue logic.
components/LoginButton.tsx (1)
103-108: Unused Twitter component import
TwitterSignInButtonis still imported (line 20-21) but its usage was commented out (line 120). This will raise an “unused import” lint error.-import TwitterSignInButton from "./TwitterSignInButton"If you plan to re-enable Twitter later, wrap the import in a TODO comment instead.
app/api/workspaces/route.ts (1)
67-75: Error classification can mis-label 401 vs 403
status = msg.includes('limit') ? 403 : 401;If the error message wording changes, this heuristic fails. Prefer explicit error types from
requireSubscription(e.g., customSubscriptionLimitErrorclass) to decide status codes.lib/auth.ts (1)
72-109: Centralise plan limits to avoid duplication
limits: { workspaces: 5 }here must stay in sync withPLAN_LIMITSused inrequireSubscription.ts.
Consider importing the constants from a single module to prevent drift.lib/requireSubscription.ts (2)
13-53: Consider using custom error types and centralizing defaults.The function implementation is well-structured with clear steps for authorization and limit validation, but has a few areas for improvement:
- Using generic
Errorobjects makes it difficult for callers to distinguish between different error types.- The default workspace limit (2) is duplicated in line 36 and implied in the default case.
Consider implementing these improvements:
+ // Custom error classes for better error handling + export class UnauthorizedError extends Error { + constructor(message: string = "Unauthorized") { + super(message); + this.name = "UnauthorizedError"; + } + } + + export class WorkspaceLimitError extends Error { + constructor(message: string) { + super(message); + this.name = "WorkspaceLimitError"; + } + } + + // Default limits as a constant + const DEFAULT_WORKSPACE_LIMIT = 2; + const PLAN_LIMITS: Record<string, PlanLimits> = { pro: { workspaces: 5 } }; export async function requireSubscription( headers: Headers ): Promise<{ userId: string; limits: PlanLimits }> { // 1) Validate session const session = await auth.api.getSession({ headers }); if (!session?.user?.id) { - throw new Error("Unauthorized"); + throw new UnauthorizedError(); } const userId = session.user.id; // 2) Fetch all subscriptions for this reference const subs = await db.query.subscriptions.findMany({ where: eq(subscriptions.referenceId, userId), }); // 3) Determine active/trialing subscription const active = subs.find( (s) => s.status === "active" || s.status === "trialing" ); // 4) Derive limits: default free users get 2 workspaces const limits: PlanLimits = active - ? PLAN_LIMITS[active.plan] ?? { workspaces: 2 } - : { workspaces: 2 }; + ? PLAN_LIMITS[active.plan] ?? { workspaces: DEFAULT_WORKSPACE_LIMIT } + : { workspaces: DEFAULT_WORKSPACE_LIMIT }; // 5) Count existing workspaces const existingCount = ( await db.query.workspaces.findMany({ where: eq(workspaces.ownerId, userId), }) ).length; // 6) Enforce limit if (existingCount >= limits.workspaces) { - throw new Error( + throw new WorkspaceLimitError( `Workspace limit reached for plan${active?.plan ? ` (${active.plan})` : ""}.` ); } return { userId, limits }; }
13-53: Consider optimizing database queries for performance.The function makes two separate database queries (one for subscriptions and one for workspaces count). This could potentially be optimized for better performance.
Consider combining the queries or utilizing a cache mechanism for the workspace count to reduce database load, especially for high-traffic scenarios.
lib/db/migrations/0009_high_vindicator.sql (1)
1-16: Consider adding indexes for performance optimization.While the table structure is sound, frequently queried fields like
reference_idandstatusshould have indexes to improve query performance, especially as the number of subscriptions grows.CREATE TABLE "subscriptions" ( "id" text PRIMARY KEY NOT NULL, "plan" text NOT NULL, "reference_id" text NOT NULL, "stripe_customer_id" text, "stripe_subscription_id" text, "status" text NOT NULL, "period_start" timestamp, "period_end" timestamp, "cancel_at_period_end" boolean, "seats" integer, "trial_start" timestamp, "trial_end" timestamp, "created_at" timestamp DEFAULT now() NOT NULL, "updated_at" timestamp DEFAULT now() NOT NULL ); + CREATE INDEX idx_subscriptions_reference_id ON "subscriptions" ("reference_id"); + CREATE INDEX idx_subscriptions_status ON "subscriptions" ("status"); + CREATE INDEX idx_subscriptions_period_end ON "subscriptions" ("period_end");lib/db/schema.ts (1)
111-126: Consider adding foreign key constraints for referential integrity.The
referenceIdfield doesn't have a foreign key constraint, which might lead to referential integrity issues if a user or organization is deleted while having active subscriptions.Since
referenceIdcan reference either a user or an organization, consider adding an additional field to specify the reference type, then creating appropriate foreign key constraints:export const subscriptions = pgTable("subscriptions", { id: text('id').primaryKey(), plan: text('plan').notNull(), referenceId: text('reference_id').notNull(), // user-ID or org-ID + referenceType: text('reference_type').notNull(), // 'user' or 'organization' stripeCustomerId: text('stripe_customer_id'), stripeSubscriptionId: text('stripe_subscription_id'), status: text('status').notNull(), // active, trialing, canceled… periodStart: timestamp('period_start'), periodEnd: timestamp('period_end'), cancelAtPeriodEnd: boolean('cancel_at_period_end'), seats: integer('seats'), trialStart: timestamp('trial_start'), trialEnd: timestamp('trial_end'), createdAt: timestamp('created_at').notNull().defaultNow(), updatedAt: timestamp('updated_at').notNull().defaultNow(), });Then you could create foreign key constraints through migration scripts based on the reference type.
components/LandingHero.tsx (3)
31-79: Use relative dates for example todos to prevent staleness.The example todos use absolute dates calculated from the current time, which is good for keeping the examples relevant. However, there's a small inconsistency in how the dates are displayed.
Consider using a more consistent approach for all example dates:
// Example todos for demonstration const exampleTodos: PreviewTodo[] = [ { id: "example-1", title: "Meet with investors", completed: false, - dueDate: new Date(Date.now() + 4 * 24 * 60 * 60 * 1000).toISOString(), // 4 days from now + dueDate: new Date(Date.now() + 4 * 24 * 60 * 60 * 1000).toISOString(), urgency: 5.0, comments: [ // ... ] }, // Apply similar changes to other example todos... ]; + // Helper function to create relative dates for examples + const getDaysFromNow = (days: number) => { + return new Date(Date.now() + days * 24 * 60 * 60 * 1000).toISOString(); + };
287-290: Consider performance optimizations for background gradients.The background gradients with large blur effects can cause performance issues on lower-end devices, especially when combined with animations.
Consider:
- Using smaller blur radii (e.g.,
blur-2xlinstead ofblur-3xl)- Adding the
will-change: opacity, transformCSS property to optimize rendering- Using
@media (prefers-reduced-motion)to disable or reduce animations for users who prefer reduced motion
442-455: Use Lucide React components consistently for SVG icons.While most icons are imported from
lucide-react, some icons are defined inline as SVG elements. This inconsistency can make maintenance more difficult.Extract these inline SVGs to proper Lucide React imports or create custom icon components for consistency:
- icon={<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" className="w-6 h-6 text-[#7c5aff]"><rect x="2" y="3" width="20" height="14" rx="2" ry="2"></rect><line x1="8" y1="21" x2="16" y2="21"></line><line x1="12" y1="17" x2="12" y2="21"></line></svg>} + icon={<Monitor className="w-6 h-6 text-[#7c5aff]" />} - icon={<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" className="w-6 h-6 text-[#7c5aff]"><rect x="5" y="2" width="14" height="20" rx="2" ry="2"></rect><line x1="12" y1="18" x2="12.01" y2="18"></line></svg>} + icon={<Smartphone className="w-6 h-6 text-[#7c5aff]" />}Add the corresponding imports at the top of the file.
app/HomeClient.tsx (1)
754-763: Guard against invalidcolIdx& useNumber.parseIntAlthough you added a
NaNcheck, feedingparseIntan undefined element can still yield unexpected results.
A safer slice avoids mutation by architecture changes:-const parts = destination.droppableId.split('-'); -const colIdx = parseInt(parts[parts.length - 1]!, 10); +const colIdx = Number.parseInt(destination.droppableId.split('-').at(-1) ?? '', 10);Keeps the logic robust if the Droppable ID schema evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
app/opengraph-image.pngis excluded by!**/*.pngapp/twitter-image.pngis excluded by!**/*.pngbun.lockis excluded by!**/*.lockbun.lockbis excluded by!**/bun.lockbpackage-lock.jsonis excluded by!**/package-lock.jsonpublic/noise-dark.pngis excluded by!**/*.png
📒 Files selected for processing (26)
README.md(1 hunks)app/HomeClient.tsx(2 hunks)app/api/[transport]/route.ts(1 hunks)app/api/parse-todo/route.ts(8 hunks)app/api/workspaces/route.ts(3 hunks)changelog.md(0 hunks)components/AgendaIcon.tsx(1 hunks)components/LandingHero.tsx(1 hunks)components/LoginButton.tsx(1 hunks)components/SettingsDialog.tsx(5 hunks)components/workspace-switcher.tsx(3 hunks)lib/auth-client.ts(1 hunks)lib/auth.ts(3 hunks)lib/db/migrations/0009_high_vindicator.sql(1 hunks)lib/db/migrations/0010_young_monster_badoon.sql(1 hunks)lib/db/migrations/meta/0009_snapshot.json(1 hunks)lib/db/migrations/meta/0010_snapshot.json(1 hunks)lib/db/migrations/meta/_journal.json(1 hunks)lib/db/schema.ts(2 hunks)lib/requireSubscription.ts(1 hunks)mcp-research.md(0 hunks)new-hoverbar.md(0 hunks)package.json(3 hunks)spinner.css(0 hunks)spinner.tsx(0 hunks)toast.tsx(0 hunks)
💤 Files with no reviewable changes (6)
- mcp-research.md
- spinner.tsx
- spinner.css
- changelog.md
- new-hoverbar.md
- toast.tsx
✅ Files skipped from review due to trivial changes (5)
- lib/db/migrations/0010_young_monster_badoon.sql
- lib/db/migrations/meta/_journal.json
- lib/db/migrations/meta/0009_snapshot.json
- components/AgendaIcon.tsx
- lib/db/migrations/meta/0010_snapshot.json
🧰 Additional context used
🧬 Code Graph Analysis (4)
components/LoginButton.tsx (3)
components/ui/dialog.tsx (5)
DialogTrigger(123-123)DialogContent(124-124)DialogHeader(125-125)DialogTitle(127-127)DialogDescription(128-128)components/GoogleSignInButton.tsx (1)
GoogleSignInButton(4-21)components/GithubSignInButton.tsx (1)
GithubSignInButton(4-21)
lib/requireSubscription.ts (3)
lib/auth.ts (1)
auth(44-110)lib/db/index.ts (1)
db(10-10)lib/db/schema.ts (2)
subscriptions(111-126)workspaces(60-66)
app/api/workspaces/route.ts (2)
lib/requireSubscription.ts (1)
requireSubscription(13-53)lib/db/schema.ts (1)
workspaceMembers(68-75)
components/LandingHero.tsx (2)
lib/utils.ts (1)
formatDate(9-43)components/ui/button.tsx (1)
Button(52-52)
🪛 LanguageTool
README.md
[style] ~22-~22: In American English, abbreviations like “etc.” require a period.
Context: ... ] Add Stripe on trial/subscription end etc emails via resend. ## Tech Stack - **...
(ETC_PERIOD)
🔇 Additional comments (12)
app/api/[transport]/route.ts (1)
1-16: MCP handler implementation looks well-structuredThe implementation of the dice rolling tool using the MCP adapter is clean and follows good practices. The input validation with zod ensures proper error handling for invalid inputs.
This appears to be related to the "MCP Server for Workspaces" item mentioned in the README as a current implementation task.
package.json (1)
18-18: Dependencies align with implementation needsThe added dependencies for Stripe integration (
@better-auth/stripe,stripe), MCP adapter (@vercel/mcp-adapter), and date parsing (chrono-node) appropriately support the functionality being added in the PR.The fixed version of
better-auth(1.2.6) ensures compatibility with the@better-auth/stripepackage which is at the same version.Also applies to: 58-58, 61-62, 92-92
lib/auth-client.ts (1)
2-2: Well-structured Stripe client integrationThe Stripe client plugin is cleanly integrated into the auth client configuration with clear comments explaining its purpose. The export of the
subscriptionfunctionality alongside other auth methods provides a consistent API for authentication and subscription management.This implementation aligns with the Stripe integration task mentioned in the README.
Also applies to: 6-9, 12-13
components/LoginButton.tsx (1)
111-126: Missing semantic role & focus trapping in custom dialogNice visual improvements! Two accessibility gaps remain:
DialogContentlacksaria-modal="true"and focus trapping for keyboard users. Verify that yourDialogabstraction handles this automatically; if not, add it.- The Terms / Privacy links open in the same tab, potentially losing user context. Consider
target="_blank" rel="noopener noreferrer".lib/requireSubscription.ts (2)
1-5: Clean imports with clear purpose.The imports correctly bring in all the necessary dependencies for authentication, database access, and schema operations.
6-11: Good use of type definition for plan limits.The
PlanLimitstype andPLAN_LIMITSconstant provide a clear mapping between subscription plans and their workspace limits, making it easy to manage and update plan configurations.lib/db/migrations/0009_high_vindicator.sql (1)
1-16: Well-structured table definition with comprehensive fields.The migration script creates a well-designed
subscriptionstable with all necessary fields for subscription management. The schema includes:
- Primary identifiers and references
- Stripe integration fields
- Subscription status and period tracking
- Proper timestamps
The column types and constraints are appropriate for each field.
lib/db/schema.ts (2)
9-9: Good addition of Stripe customer ID to user schema.Adding
stripeCustomerIdas an optional field to the users table is appropriate and aligns with the Stripe integration requirements.
111-126: Comprehensive subscription schema with clear comments.The subscriptions table schema is well-defined with all necessary fields for subscription management. The inline comments provide helpful context about field purposes (e.g.,
// user-ID or org-IDand// active, trialing, canceled…).components/LandingHero.tsx (2)
1-9: Clean imports with correct component dependencies.The imports are well-organized, bringing in necessary React hooks, UI components, and icons.
11-28: Well-defined TypeScript interfaces for preview data.The
PreviewTodoandPreviewCommentinterfaces provide clear type definitions that ensure type safety throughout the component.app/api/parse-todo/route.ts (1)
118-126: Reliance onNEXT_PUBLIC_APP_URLmay break on self-hosted / preview deployments
convertRelativeDate()builds the endpoint with`${process.env.NEXT_PUBLIC_APP_URL}/api/convert-date`If the env-var is unset (common in Vercel preview or local dev) the URL becomes
"undefined/api/convert-date"→ENOTFOUND.Prefer deriving the base URL from the incoming request or falling back to an absolute path:
-const response = await fetch(`${process.env.NEXT_PUBLIC_APP_URL}/api/convert-date`, { +const base = + process.env.NEXT_PUBLIC_APP_URL ?? + process.env.VERCEL_URL && `https://${process.env.VERCEL_URL}` ?? + ''; // empty string fallback => relative call +const response = await fetch(`${base}/api/convert-date`, {This keeps the function environment-agnostic.
components/workspace-switcher.tsx
Outdated
| canCreateNew: boolean | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prop is now mandatory – update all call-sites
canCreateNew was added as a required boolean prop, which will break any existing usage of <WorkspaceSwitcher /> that has not been updated.
Consider giving it a sensible default (true) or making it optional to avoid runtime/TypeScript errors across the codebase.
app/api/workspaces/route.ts
Outdated
| const cookieStore = await cookies(); | ||
|
|
||
| const session = await auth.api.getSession({ | ||
| headers: new Headers({ | ||
| cookie: cookieStore.toString() | ||
| }) | ||
| }); | ||
|
|
||
| // More robust session validation | ||
| if (!session?.user?.id) { | ||
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); | ||
| const headers = new Headers({ cookie: cookieStore.toString() }); | ||
| let userId: string; |
There was a problem hiding this comment.
cookies() is synchronous – remove the unnecessary await
cookies() from next/headers returns a ReadonlyRequestCookies instance, not a Promise.
Using await here (and in GET/DELETE above) causes a TypeScript error:
-const cookieStore = await cookies();
+const cookieStore = cookies();Apply the same fix where await cookies() appears elsewhere in this file.
📝 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.
| const cookieStore = await cookies(); | |
| const session = await auth.api.getSession({ | |
| headers: new Headers({ | |
| cookie: cookieStore.toString() | |
| }) | |
| }); | |
| // More robust session validation | |
| if (!session?.user?.id) { | |
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); | |
| const headers = new Headers({ cookie: cookieStore.toString() }); | |
| let userId: string; | |
| const cookieStore = cookies(); | |
| const headers = new Headers({ cookie: cookieStore.toString() }); | |
| let userId: string; |
lib/auth.ts
Outdated
| const stripeClient = new Stripe(process.env.STRIPE_SECRET_KEY!, { | ||
| apiVersion: "2025-02-24.acacia", | ||
| }); |
There was a problem hiding this comment.
Invalid Stripe API version string
"2025-02-24.acacia" is not a valid Stripe API version (latest as of 2024 is 2023-10-16).
Using an unknown version will make the SDK throw on initialization.
- apiVersion: "2025-02-24.acacia",
+ apiVersion: "2023-10-16",📝 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.
| const stripeClient = new Stripe(process.env.STRIPE_SECRET_KEY!, { | |
| apiVersion: "2025-02-24.acacia", | |
| }); | |
| const stripeClient = new Stripe(process.env.STRIPE_SECRET_KEY!, { | |
| apiVersion: "2023-10-16", | |
| }); |
lib/requireSubscription.ts
Outdated
| // 3) Determine active/trialing subscription | ||
| const active = subs.find( | ||
| (s) => s.status === "active" || s.status === "trialing" | ||
| ); | ||
|
|
||
| // 4) Derive limits: default free users get 2 workspaces | ||
| const limits: PlanLimits = active | ||
| ? PLAN_LIMITS[active.plan] ?? { workspaces: 2 } | ||
| : { workspaces: 2 }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the subscription status logic for edge cases.
The current implementation checks for "active" or "trialing" status to determine an active subscription, but there might be other valid statuses in Stripe that aren't being handled explicitly.
🏁 Script executed:
#!/bin/bash
# Check for all possible subscription statuses in the Stripe API documentation
curl -s https://api.stripe.com/v1/subscription_items -G \
-d limit=1 \
-H "Authorization: Bearer dummy_key" 2>&1 | grep -i "error" | grep -i "status"
# Alternatively, we can search the codebase for places where subscription status is set
rg "s(ub)?\.status\s*(=|==|===|!=|!==|!===)\s*['\"]" --type tsLength of output: 512
🌐 Web query:
List all possible `status` values for a Stripe Subscription object according to the Stripe API documentation
💡 Result:
According to the Stripe API documentation, a Subscription object can have the following status values:
incompleteincomplete_expiredtrialingactivepast_duecanceledunpaidpaused
These statuses represent various stages in the lifecycle of a subscription, from initial creation to cancellation or pausing. (docs.stripe.com)
Citations:
Audit Stripe subscription statuses
Stripe supports these subscription statuses: incomplete, incomplete_expired, trialing, active, past_due, canceled, unpaid, and paused (see Stripe docs¹). Currently, in lib/requireSubscription.ts (and similarly in SettingsDialog.tsx and HomeClient.tsx), only active and trialing are treated as active subscriptions:
const active = subs.find(
(s) => s.status === "active" || s.status === "trialing"
);Please review whether other statuses—most notably past_due—should also count as “active” for workspace limits. If so, update the predicate and consider centralizing this logic for clarity:
• File: lib/requireSubscription.ts (lines 28–37)
• Files with the same pattern:
– git/components/SettingsDialog.tsx
– git/app/HomeClient.tsx
Suggested improvements:
- Extend the check, e.g.:
- (s) => s.status === "active" || s.status === "trialing" + (s) => ["active", "trialing", "past_due"].includes(s.status)
- Encapsulate status logic in a helper or enum.
- Add unit tests covering all Stripe statuses.
components/LandingHero.tsx
Outdated
| <ul className="space-y-4 mb-8 flex-grow"> | ||
| <li className="flex items-start"> | ||
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | ||
| <span className="text-gray-700 dark:text-gray-300">Up to 3 workspaces</span> | ||
| </li> | ||
| <li className="flex items-start"> | ||
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | ||
| <span className="text-gray-700 dark:text-gray-300">Add 1 collaborator per workspace</span> | ||
| </li> | ||
| <li className="flex items-start"> | ||
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | ||
| <span className="text-gray-700 dark:text-gray-300">Basic AI model with limited context</span> | ||
| </li> | ||
| <li className="flex items-start"> | ||
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | ||
| <span className="text-gray-700 dark:text-gray-300">Unlimited todos</span> | ||
| </li> |
There was a problem hiding this comment.
Subscription plan inconsistency with implementation.
Line 491 states that the free plan allows "Up to 3 workspaces", but in requireSubscription.ts the default for free users is set to 2 workspaces. This discrepancy could confuse users.
<li className="flex items-start">
<span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span>
- <span className="text-gray-700 dark:text-gray-300">Up to 3 workspaces</span>
+ <span className="text-gray-700 dark:text-gray-300">Up to 2 workspaces</span>
</li>Make sure to align the advertised limits with the actual implementation to avoid confusion and potential customer complaints.
📝 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.
| <ul className="space-y-4 mb-8 flex-grow"> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Up to 3 workspaces</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Add 1 collaborator per workspace</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Basic AI model with limited context</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Unlimited todos</span> | |
| </li> | |
| <ul className="space-y-4 mb-8 flex-grow"> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| - <span className="text-gray-700 dark:text-gray-300">Up to 3 workspaces</span> | |
| + <span className="text-gray-700 dark:text-gray-300">Up to 2 workspaces</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Add 1 collaborator per workspace</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Basic AI model with limited context</span> | |
| </li> | |
| <li className="flex items-start"> | |
| <span className="text-green-500 mr-2.5 flex-shrink-0 mt-0.5">✓</span> | |
| <span className="text-gray-700 dark:text-gray-300">Unlimited todos</span> | |
| </li> | |
| </ul> |
components/LandingHero.tsx
Outdated
| const openAuthDialog = () => { | ||
| const loginButton = document.querySelector('button[aria-label="Sign In"]') as HTMLButtonElement | ||
| if (loginButton) { | ||
| loginButton.click() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct DOM manipulation for dialog opening.
The openAuthDialog function uses direct DOM manipulation to find and click a login button, which creates a tight coupling to the DOM structure and can be brittle if the structure changes.
Consider using a more declarative approach by lifting the authentication state up and sharing it between components:
- const openAuthDialog = () => {
- const loginButton = document.querySelector('button[aria-label="Sign In"]') as HTMLButtonElement
- if (loginButton) {
- loginButton.click()
- }
- }
+ import { useAuthDialog } from "@/lib/auth-client"; // Assuming this context/hook exists
+
+ // In the LandingHero component
+ const { openAuthDialog } = useAuthDialog();This approach would be more maintainable and less prone to breaking when the DOM structure changes.
Committable suggestion skipped: line range outside the PR's diff.
components/LandingHero.tsx
Outdated
| const formatDate = (dateStr: string) => { | ||
| const dueDate = new Date(dateStr) | ||
| const now = new Date() | ||
| const diffDays = Math.round((dueDate.getTime() - now.getTime()) / (1000 * 60 * 60 * 24)) | ||
|
|
||
| const month = dueDate.toLocaleString('default', { month: 'short' }) | ||
| const day = dueDate.getDate() | ||
|
|
||
| if (diffDays === 0) return 'Today' | ||
| if (diffDays === 1) return 'Tomorrow' | ||
| if (diffDays < 7) return `${diffDays} days` | ||
| return `${month} ${day}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse the existing date formatting utility.
You've implemented a formatDate function that duplicates functionality already available in lib/utils.ts. This can lead to inconsistencies in date formatting across the application.
Replace this implementation with the existing utility:
- const formatDate = (dateStr: string) => {
- const dueDate = new Date(dateStr)
- const now = new Date()
- const diffDays = Math.round((dueDate.getTime() - now.getTime()) / (1000 * 60 * 60 * 24))
-
- const month = dueDate.toLocaleString('default', { month: 'short' })
- const day = dueDate.getDate()
-
- if (diffDays === 0) return 'Today'
- if (diffDays === 1) return 'Tomorrow'
- if (diffDays < 7) return `${diffDays} days`
- return `${month} ${day}`
- }
+ import { formatDate } from "@/lib/utils";Or if you need slightly different formatting for the landing page, create a variant that builds on the existing utility.
components/SettingsDialog.tsx
Outdated
| try { | ||
| const resp = await subscription.list() | ||
| const subs = resp.data ?? [] | ||
| const active = subs.find((s: any) => s.status === "active" || s.status === "trialing") | ||
| setPlan(active?.plan ?? "free") | ||
| } catch (err) { | ||
| console.error("Error fetching subscription:", err) | ||
| // Don't show error toast for the known date error |
There was a problem hiding this comment.
Normalize plan value before equality checks
subscription.list() often returns identifiers such as "pro-monthly" or "pro-yearly".
The subsequent UI logic (plan === 'pro') assumes the value is exactly "pro", which will silently downgrade Pro users to the Free flow (wrong button label, unexpected cancel vs upgrade call).
-const active = subs.find((s: any) => s.status === "active" || s.status === "trialing")
-setPlan(active?.plan ?? "free")
+const active = subs.find((s: any) =>
+ s.status === "active" || s.status === "trialing",
+)
+// Strip billing cadence suffixes like `-monthly`
+const tier = (active?.plan ?? "free").split("-")[0]
+setPlan(tier)📝 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.
| try { | |
| const resp = await subscription.list() | |
| const subs = resp.data ?? [] | |
| const active = subs.find((s: any) => s.status === "active" || s.status === "trialing") | |
| setPlan(active?.plan ?? "free") | |
| } catch (err) { | |
| console.error("Error fetching subscription:", err) | |
| // Don't show error toast for the known date error | |
| try { | |
| const resp = await subscription.list() | |
| const subs = resp.data ?? [] | |
| const active = subs.find((s: any) => | |
| s.status === "active" || s.status === "trialing", | |
| ) | |
| // Strip billing cadence suffixes like `-monthly` | |
| const tier = (active?.plan ?? "free").split("-")[0] | |
| setPlan(tier) | |
| } catch (err) { | |
| console.error("Error fetching subscription:", err) | |
| // Don't show error toast for the known date error |
components/SettingsDialog.tsx
Outdated
| // Launch Stripe Customer Portal or Upgrade | ||
| const handlePortal = async () => { | ||
| setIsPortalLoading(true) | ||
| try { | ||
| let result: any | ||
| if (plan === 'pro') { | ||
| const { data, error } = await subscription.cancel({ | ||
| returnUrl: new URL('/?settings=true', window.location.origin).toString() | ||
| }); | ||
|
|
||
| if (error) { | ||
| console.error("Error canceling subscription:", error) | ||
| throw error; | ||
| } | ||
|
|
||
| if (data?.url) { | ||
| window.location.href = data.url; | ||
| } | ||
| } else { | ||
| result = await subscription.upgrade({ | ||
| plan: 'pro', | ||
| successUrl: new URL('/?settings=true', window.location.origin).toString(), | ||
| cancelUrl: new URL('/?settings=true', window.location.origin).toString() | ||
| }) | ||
| const url = result.data?.url | ||
| if (url) window.location.href = url | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
handlePortal triggers a cancellation instead of opening the Billing Portal
When plan === 'pro' the code calls subscription.cancel(...).
Users clicking “Manage Subscription” generally expect the Stripe customer-portal where they can update payment methods or change plans, not an immediate cancellation flow.
Unless your SDK’s cancel method is purposely named that way, consider:
-if (plan === 'pro') {
- const { data, error } = await subscription.cancel({
- returnUrl: new URL('/?settings=true', window.location.origin).toString()
- });
+if (plan === 'pro') {
+ // Opens Stripe’s self-service portal; users choose cancel/upgrade themselves
+ const { data, error } = await subscription.portal({
+ returnUrl: new URL('/?settings=true', window.location.origin).toString()
+ });This avoids accidental cancellations and aligns user expectation.
(If your SDK truly has no portal helper, rename the button and warn users explicitly.)
📝 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.
| // Launch Stripe Customer Portal or Upgrade | |
| const handlePortal = async () => { | |
| setIsPortalLoading(true) | |
| try { | |
| let result: any | |
| if (plan === 'pro') { | |
| const { data, error } = await subscription.cancel({ | |
| returnUrl: new URL('/?settings=true', window.location.origin).toString() | |
| }); | |
| if (error) { | |
| console.error("Error canceling subscription:", error) | |
| throw error; | |
| } | |
| if (data?.url) { | |
| window.location.href = data.url; | |
| } | |
| } else { | |
| result = await subscription.upgrade({ | |
| plan: 'pro', | |
| successUrl: new URL('/?settings=true', window.location.origin).toString(), | |
| cancelUrl: new URL('/?settings=true', window.location.origin).toString() | |
| }) | |
| const url = result.data?.url | |
| if (url) window.location.href = url | |
| } | |
| } catch (err) { | |
| // Launch Stripe Customer Portal or Upgrade | |
| const handlePortal = async () => { | |
| setIsPortalLoading(true) | |
| try { | |
| let result: any | |
| if (plan === 'pro') { | |
| // Opens Stripe’s self-service portal; users choose cancel/upgrade themselves | |
| const { data, error } = await subscription.portal({ | |
| returnUrl: new URL('/?settings=true', window.location.origin).toString() | |
| }); | |
| if (error) { | |
| console.error("Error canceling subscription:", error) | |
| throw error; | |
| } | |
| if (data?.url) { | |
| window.location.href = data.url; | |
| } | |
| } else { | |
| result = await subscription.upgrade({ | |
| plan: 'pro', | |
| successUrl: new URL('/?settings=true', window.location.origin).toString(), | |
| cancelUrl: new URL('/?settings=true', window.location.origin).toString() | |
| }) | |
| const url = result.data?.url | |
| if (url) window.location.href = url | |
| } | |
| } catch (err) { |
app/api/parse-todo/route.ts
Outdated
| // Query the subscriptions table for an active subscription | ||
| const userSubscriptions = await db.select({ | ||
| plan: subscriptionsTable.plan, | ||
| status: subscriptionsTable.status, | ||
| }) | ||
| .from(subscriptionsTable) | ||
| .where( | ||
| and( | ||
| eq(subscriptionsTable.referenceId, userId), | ||
| eq(subscriptionsTable.status, 'active') | ||
| ) | ||
| ) | ||
| .limit(1) |
There was a problem hiding this comment.
hasProPlan overlooks “trialing” subscriptions
Front-end logic treats both "active" and "trialing" as Pro (see SettingsDialog) but the DB query here filters only status = 'active'.
Trial users will therefore get the lower-tier model even though the UI advertises Pro features.
-where(
- and(
- eq(subscriptionsTable.referenceId, userId),
- eq(subscriptionsTable.status, 'active')
- )
-)
+where(
+ and(
+ eq(subscriptionsTable.referenceId, userId),
+ subscriptionsTable.status.in(['active', 'trialing'])
+ )
+)(Adjust syntax to drizzle-orm’s inArray helper if needed.)
📝 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.
| // Query the subscriptions table for an active subscription | |
| const userSubscriptions = await db.select({ | |
| plan: subscriptionsTable.plan, | |
| status: subscriptionsTable.status, | |
| }) | |
| .from(subscriptionsTable) | |
| .where( | |
| and( | |
| eq(subscriptionsTable.referenceId, userId), | |
| eq(subscriptionsTable.status, 'active') | |
| ) | |
| ) | |
| .limit(1) | |
| // Query the subscriptions table for an active or trialing subscription | |
| const userSubscriptions = await db.select({ | |
| plan: subscriptionsTable.plan, | |
| status: subscriptionsTable.status, | |
| }) | |
| .from(subscriptionsTable) | |
| .where( | |
| and( | |
| eq(subscriptionsTable.referenceId, userId), | |
| // include both active and trialing as “Pro” | |
| subscriptionsTable.status.in(['active', 'trialing']) | |
| ) | |
| ) | |
| .limit(1) |
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores