-
-
Notifications
You must be signed in to change notification settings - Fork 310
fix: sidebar thread group by date #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@mayur9210 is attempting to deploy a commit to the cgoinglove's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
/gemini reivew |
|
It looks like there might be a typo in your command. If you intended to request a code review, please use |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses the issue of incorrect thread date grouping caused by timezone discrepancies. The approach of treating database timestamps as UTC and then converting to the user's local timezone in the frontend is correct. The implementation is solid, but I've identified a couple of areas for improvement. One is a potential issue with date calculations around Daylight Saving Time, and the other is a brittle check for timezone information in timestamp strings. My review includes suggestions to make the code more robust in these areas.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes timezone handling issues when displaying chat threads grouped by date. The changes ensure that timestamp comparisons correctly account for UTC storage in the database versus local timezone display in the UI.
Key changes:
- Modified PostgreSQL repository to explicitly treat timestamps as UTC by appending 'Z' suffix when converting to JavaScript timestamps
- Updated thread grouping logic to use consistent local timezone date calculations for bucketing threads into "Today", "Yesterday", "Last Week", and "Older" categories
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib/db/pg/repositories/chat-repository.pg.ts | Added UTC timestamp handling logic to properly convert PostgreSQL timestamp strings to JavaScript timestamps with explicit UTC treatment |
| src/components/layouts/app-sidebar-threads.tsx | Refactored date comparison logic to use consistent local timezone calculations for thread grouping, replacing the previous approach that mixed date mutations and comparisons |
Comments suppressed due to low confidence (1)
src/components/layouts/app-sidebar-threads.tsx:1
- Using
now.getDate() - 1andnow.getDate() - 7directly in the Date constructor can produce incorrect results when crossing month boundaries. For example, if today is March 1st (getDate() = 1),now.getDate() - 1equals 0, which creates an invalid date. Use the same pattern astodayLocalby creating the date first, then usingsetDate()or subtracting milliseconds fromtodayStart.
"use client";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request fixes the way thread dates are handled and grouped in the sidebar, ensuring that threads are accurately categorized as "today," "yesterday," "last week," or "older" based on the user's local timezone. It also fixes potential issues with interpreting timestamps from the database, especially those lacking timezone information.
As a test in before/after the todays chat was going on Last 7 days section , this fix will make sure to use TZ timezone, the other way around is changing schema from
timestamptotimestampzfor all dates.Before
timezone.mov
After
AppSidebarThreadsto consistently use the user's local timezone when determining if a thread belongs to "today," "yesterday," or "last week." This prevents misclassification of threads near midnight or in different timezones. [1] [2]Database timestamp interpretation:
pgChatRepositoryto treatlastMessageAttimestamps from PostgreSQL as UTC if they lack timezone information, ensuring accurate conversion to JavaScript timestamps regardless of server or client timezone.