Enhance UI, add save button #3333
Conversation
🔧 Biome Check ReportIssues Found: 0📊 Analysis Summary
🔍 Issue Breakdown
🎉 All Issues Resolved!Your code is now 100% clean! Great job! 🏆
🤖 Auto-generated by Biome Check workflow • Last updated: 7/24/2025, 8:26:17 AM
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughThe changes update the layout and alignment of several UI components related to plan availability and details, introducing explicit styling adjustments for improved centering and spacing. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AvailabilityPlanner
participant TimeBlockingProvider
participant Server
User->>AvailabilityPlanner: Edit time blocks
AvailabilityPlanner->>AvailabilityPlanner: Mark as dirty
User->>AvailabilityPlanner: Click "Save"
AvailabilityPlanner->>TimeBlockingProvider: syncTimeBlocks()
TimeBlockingProvider->>Server: Fetch current time blocks
TimeBlockingProvider->>Server: Add/remove time blocks as needed
Server-->>TimeBlockingProvider: Responds with updated time blocks
TimeBlockingProvider->>AvailabilityPlanner: Update local state
AvailabilityPlanner->>User: Show saved state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @datnguyen-en, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the user experience by providing explicit control over saving time block availability. It refactors the underlying synchronization mechanism from an automatic process to a user-initiated action via a new 'Save' button, alongside general UI improvements for better presentation and responsiveness.
Highlights
- Manual Save Functionality: Implemented a 'Save' button within the
AvailabilityPlannercomponent. This button allows users to explicitly save their time block changes, tracking a 'dirty' state to indicate unsaved modifications and displaying a 'Saving...' state during the process. - Time Block Synchronization Refactor: The logic for synchronizing time blocks with the backend, previously an automatic process triggered by a
useEffectinTimeBlockingProvider, has been refactored into an explicitsyncTimeBlocksfunction. This function is now exposed via theTimeBlockContextfor manual invocation by the new save button. - UI Enhancements: Minor layout and styling adjustments were made across several components (
AllAvailabilities,AvailabilityPlanner,PlanDetailsClient) to improve centering, spacing, and overall responsiveness, including setting minimum widths for certain UI elements.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3333 +/- ##
==========================================
- Coverage 1.50% 1.49% -0.01%
==========================================
Files 1918 1918
Lines 248066 248083 +17
Branches 2605 2601 -4
==========================================
- Hits 3721 3718 -3
- Misses 242779 242799 +20
Partials 1566 1566 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx (1)
277-390: Refactor complex sync function and add error handlingThe
syncTimeBlocksfunction is quite complex and lacks error handling. Consider breaking it down into smaller functions and adding proper error handling.Here's a suggested refactoring approach:
+ const compareTimeblocks = (tb1: Timeblock, tb2: Timeblock) => + tb1.date === tb2.date && + tb1.start_time === tb2.start_time && + tb1.end_time === tb2.end_time; + + const findTimeblocksToSync = (serverTimeblocks: Timeblock[], localTimeblocks: Timeblock[]) => { + const toRemove = serverTimeblocks.filter( + server => !localTimeblocks.some(local => compareTimeblocks(local, server)) + ); + const toAdd = localTimeblocks.filter( + local => !serverTimeblocks.some(server => compareTimeblocks(server, local)) + ); + return { toRemove, toAdd }; + }; const syncTimeBlocks = useCallback(async () => { if (!plan.id || !user?.id) return; const fetchCurrentTimeBlocks = async (planId: string) => { - const res = await fetch(`/api/meet-together/plans/${planId}/timeblocks`); - if (!res.ok) return []; - const timeblocks = (await res.json()) as Timeblock[]; + try { + const res = await fetch(`/api/meet-together/plans/${planId}/timeblocks`); + if (!res.ok) throw new Error(`Failed to fetch timeblocks: ${res.status}`); + const timeblocks = (await res.json()) as Timeblock[]; + } catch (error) { + console.error('Error fetching timeblocks:', error); + throw error; + }Also consider implementing batch operations instead of multiple individual API calls.
🧹 Nitpick comments (4)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/all-availabilities.tsx (1)
38-38: Remove redundanttext-centerclassThe parent div already has
text-centerclass, making this redundant.- <div className="text-center font-semibold"> + <div className="font-semibold">apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/plan-details-client.tsx (1)
146-146: Consider reducing the extra-large gap valueThe
xl:gap-80(20rem/320px) gap seems excessive and might cause layout issues on ultra-wide screens, pushing the content too far apart.- <div className="mx-auto mt-8 flex w-full flex-col items-center justify-center gap-8 md:flex-row md:gap-16 lg:gap-32 xl:gap-80"> + <div className="mx-auto mt-8 flex w-full flex-col items-center justify-center gap-8 md:flex-row md:gap-16 lg:gap-32 xl:gap-48">apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx (1)
64-72: Extract inline styles to Tailwind classesThe extensive inline styles on the button could be replaced with Tailwind classes for better maintainability.
<button - className="mx-auto mt-8 block rounded-full bg-gradient-to-r from-indigo-500 via-sky-500 to-emerald-500 px-8 py-3 text-lg font-semibold text-white shadow-md transition-all duration-150 ease-in-out hover:scale-105 hover:from-indigo-600 hover:via-sky-600 hover:to-emerald-600 focus:ring-2 focus:ring-sky-400 focus:ring-offset-2 focus:outline-none active:scale-100 disabled:cursor-not-allowed disabled:from-gray-400 disabled:via-gray-400 disabled:to-gray-400" - style={{ - minWidth: '120px', - minHeight: '48px', - letterSpacing: '0.01em', - }} + className="mx-auto mt-8 block min-w-[120px] min-h-12 rounded-full bg-gradient-to-r from-indigo-500 via-sky-500 to-emerald-500 px-8 py-3 text-lg font-semibold text-white shadow-md transition-all duration-150 ease-in-out hover:scale-105 hover:from-indigo-600 hover:via-sky-600 hover:to-emerald-600 focus:ring-2 focus:ring-sky-400 focus:ring-offset-2 focus:outline-none active:scale-100 disabled:cursor-not-allowed disabled:from-gray-400 disabled:via-gray-400 disabled:to-gray-400 tracking-tight" onClick={handleSave} disabled={!dirty || isSaving} >apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx (1)
325-327: Optimize API calls with batch operationsMultiple sequential API calls using
Promise.allwith individual requests could be optimized with batch endpoints.Instead of:
await Promise.all( localTimeblocks.map((timeblock) => addTimeBlock(timeblock)) );Consider implementing batch endpoints:
await fetch(`/api/meet-together/plans/${planId}/timeblocks/batch`, { method: 'POST', body: JSON.stringify({ timeblocks: localTimeblocks, user_id: user?.id }), });This would reduce the number of HTTP requests and improve performance, especially when syncing many timeblocks.
Also applies to: 331-333, 376-384
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/all-availabilities.tsx(1 hunks)apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx(2 hunks)apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/plan-details-client.tsx(1 hunks)apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DennieDan
PR: tutur3u/platform#2891
File: packages/ui/src/hooks/use-calendar.tsx:1511-1513
Timestamp: 2025-05-21T09:22:15.348Z
Learning: In the `GoogleCalendarSettings` component's `handleSyncNow` function, the boolean return value from `syncGoogleCalendarNow` is used to determine if changes were made during a successful sync, while errors are handled through a separate try/catch block with detailed error messages.
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx (1)
Learnt from: DennieDan
PR: #2891
File: packages/ui/src/hooks/use-calendar.tsx:1511-1513
Timestamp: 2025-05-21T09:22:15.348Z
Learning: In the GoogleCalendarSettings component's handleSyncNow function, the boolean return value from syncGoogleCalendarNow is used to determine if changes were made during a successful sync, while errors are handled through a separate try/catch block with detailed error messages.
🧬 Code Graph Analysis (2)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/plan-details-client.tsx (1)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/edit-plan-dialog.tsx (1)
EditPlanDialog(53-235)
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx (2)
packages/types/src/primitives/MeetTogetherPlan.ts (1)
MeetTogetherPlan(1-13)apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx (1)
useTimeBlocking(435-435)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
- GitHub Check: Verify generated types
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx
Show resolved
Hide resolved
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx
Show resolved
Hide resolved
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/availability-planner.tsx
Show resolved
Hide resolved
apps/web/src/app/[locale]/(marketing)/meet-together/plans/[planId]/time-blocking-provider.tsx
Outdated
Show resolved
Hide resolved
|
Can you resolve all of the AI-generated reviews? If it looks good integrate them into the code, if not you can just resolve them and report back. I'll put this as draft for now, you can decide again later on. |
There was a problem hiding this comment.
Code Review
The pull request enhances the UI by adding a save button and improving the layout of the availability planner. It also introduces a syncTimeBlocks function to synchronize time blocks between the client and server. The review focuses on error handling and potential improvements to the useEffect hook.
| ); | ||
| }; |
There was a problem hiding this comment.
It's crucial to handle potential errors when calling removeTimeBlock. If removeTimeBlock fails, the application state might become inconsistent between the client and server. Implement error handling to catch any exceptions thrown by removeTimeBlock and take appropriate action, such as logging the error or displaying an error message to the user. This will improve the reliability and robustness of the application.
if (timeblocksToRemove.length > 0)
await Promise.all(
timeblocksToRemove.map(async (timeblock) => {
try {
if (timeblock.id) await removeTimeBlock(timeblock);
} catch (error) {
console.error("Failed to remove timeblock:", error);
// Handle error appropriately, e.g., display a message to the user
}
})
);| if (serverTimeblocks.length === 0 && localTimeblocks.length > 0) { | ||
| await Promise.all( | ||
| localTimeblocks.map((timeblock) => addTimeBlock(timeblock)) | ||
| ); |
There was a problem hiding this comment.
It's crucial to handle potential errors when calling addTimeBlock. If addTimeBlock fails, the application state might become inconsistent between the client and server. Implement error handling to catch any exceptions thrown by addTimeBlock and take appropriate action, such as logging the error or displaying an error message to the user. This will improve the reliability and robustness of the application.
if (serverTimeblocks.length === 0 && localTimeblocks.length > 0) {
await Promise.all(
localTimeblocks.map(async (timeblock) => {
try {
await addTimeBlock(timeblock);
} catch (error) {
console.error("Failed to add timeblock:", error);
// Handle error appropriately, e.g., display a message to the user
}
})
);
return;
}| if (timeblocksToAdd.length > 0) | ||
| await Promise.all( | ||
| timeblocksToAdd.map((timeblock) => addTimeBlock(timeblock)) | ||
| ); |
There was a problem hiding this comment.
It's crucial to handle potential errors when calling addTimeBlock. If addTimeBlock fails, the application state might become inconsistent between the client and server. Implement error handling to catch any exceptions thrown by addTimeBlock and take appropriate action, such as logging the error or displaying an error message to the user. This will improve the reliability and robustness of the application.
if (timeblocksToAdd.length > 0)
await Promise.all(
timeblocksToAdd.map(async (timeblock) => {
try {
await addTimeBlock(timeblock);
} catch (error) {
console.error("Failed to add timeblock:", error);
// Handle error appropriately, e.g., display a message to the user
}
})
);| const syncedServerTimeblocks = await fetchCurrentTimeBlocks(plan?.id); | ||
| setSelectedTimeBlocks({ | ||
| planId: plan.id, | ||
| data: syncedServerTimeblocks, | ||
| }); | ||
| }, [plan.id, user, selectedTimeBlocks]); |
There was a problem hiding this comment.
Consider adding a try-catch block around the fetchCurrentTimeBlocks call to handle potential network errors or server-side issues. If the fetch fails, log the error and potentially retry the fetch after a delay. This will make the application more resilient to transient network issues and improve the user experience.
try {
const syncedServerTimeblocks = await fetchCurrentTimeBlocks(plan?.id);
setSelectedTimeBlocks({
planId: plan.id,
data: syncedServerTimeblocks,
});
} catch (error) {
console.error("Failed to fetch current timeblocks:", error);
// Handle error appropriately, e.g., display a message to the user or retry the fetch
}| useEffect(() => { | ||
| if (!editing.enabled) { | ||
| setDirty(true); | ||
| } | ||
| }, [editing.enabled]); |
There was a problem hiding this comment.
Consider adding a dependency array to the useEffect hook that includes dirty. This will ensure that the effect is triggered whenever the dirty state changes, preventing potential issues where the component doesn't react to changes in the dirty state as expected. This is especially important if other parts of your component rely on the dirty state to be up-to-date.
useEffect(() => {
if (!editing.enabled) {
setDirty(true);
}
}, [editing.enabled, dirty]);| @@ -290,12 +304,10 @@ const TimeBlockingProvider = ({ | |||
|
|
|||
| const removeTimeBlock = async (timeblock: Timeblock) => { | |||
There was a problem hiding this comment.
Consider adding a check to ensure timeblock.id exists before attempting to remove the timeblock. This will prevent potential errors if a timeblock without an ID is accidentally passed to the removeTimeBlock function.
if (timeblocksToRemove.length > 0)
await Promise.all(
timeblocksToRemove.map((timeblock) =>
timeblock.id ? removeTimeBlock(timeblock) : null
)
);
Summary by CodeRabbit
New Features
Improvements
Refactor