-
-
Notifications
You must be signed in to change notification settings - Fork 144
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
[fix] Post Sorting and Timestamp Display in Your Posts Section #1095
Conversation
@HarshitT00 is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve updates 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 (
|
Hello @HarshitT00, thanks for opening your first Pull Request. The maintainers will review this Pull Request and provide feedback as soon as possible. Keep up the great work! |
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/(app)/my-posts/_client.tsx (2)
157-158
: Correct typographical errors in the inline commentThere are typographical errors in the inline comment. It should be "one minute" instead of "on minutes". Also, consider rephrasing the comment for clarity.
Suggested correction:
{/*If updatedAt is greater than published by more than on minutes show updated at else show published as on updating published updatedAt is automatically updated and is greater than published*/} +{/* If updatedAt is greater than published by more than one minute, show 'Last updated on'; else show 'Published on'. Since updating a published post automatically updates 'updatedAt', which becomes greater than 'published' */}
159-159
: Avoid magic numbers by defining a constant for the time thresholdUsing the raw value of
60000
milliseconds can make the code less readable. Consider defining a constant to represent the time threshold for better readability and maintainability.Suggested change:
+const ONE_MINUTE_IN_MS = 60000; // Time threshold of one minute in milliseconds ... {(new Date(updatedAt).getTime() - new Date(published).getTime()) >= ONE_MINUTE_IN_MS ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/(app)/my-posts/_client.tsx (2 hunks)
- server/api/router/post.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
server/api/router/post.ts (3)
398-400
: Improved sorting for published postsThe new ordering logic effectively implements the requirement to distinguish between posts that have been published and those that have been updated after publication. By using
GREATEST(${posts.updatedAt}, ${posts.published})
, the sorting now considers both the publish time and the last update time, ensuring that the most recent activity on a post (whether it's the initial publish or a subsequent update) determines its position in the list.This change aligns well with the PR objective of handling the case where the
updatedAt
timestamp might be slightly greater than the published time when a post is first published.
418-418
: Added sorting for draft postsThe addition of
orderBy: (posts, { desc }) => [desc(posts.updatedAt)]
implements the requirement to support sorting of Draft posts. This ensures that drafts are displayed in order of their last update, with the most recently modified drafts appearing first. This is a user-friendly approach that aligns well with typical user expectations when working with drafts.
Line range hint
1-480
: Summary of changesThe modifications in this file successfully implement the sorting requirements outlined in the PR objectives:
- For published posts, the new ordering logic using
GREATEST(${posts.updatedAt}, ${posts.published})
effectively distinguishes between posts that have been newly published and those that have been updated after publication.- For draft posts, the addition of sorting by
updatedAt
in descending order ensures that the most recently modified drafts are displayed first.These changes enhance the user experience by providing more logical and useful ordering of posts in both the published and draft sections. The implementation is clean and aligns well with the stated objectives without introducing any apparent issues or inconsistencies.
app/(app)/my-posts/_client.tsx (1)
116-116
: Ensure 'updatedAt' is included in the API queriesSince
updatedAt
is now being used in the component, please ensure that the API queries fetching the posts include theupdatedAt
field. Missing this field could lead to runtime errors ifupdatedAt
isundefined
.You can run the following script to verify if
updatedAt
is being selected in the API queries:
app/(app)/my-posts/_client.tsx
Outdated
{(new Date(updatedAt).getTime() - new Date(published).getTime()) >= 60000 ? ( | ||
<> | ||
{"Last updated on "} | ||
{new Date(updatedAt).toLocaleDateString()} at{" "} | ||
{new Date(updatedAt).toLocaleTimeString(navigator.language, { | ||
hour: "2-digit", | ||
minute: "2-digit", | ||
hour12: false, | ||
})} | ||
</> | ||
) : ( | ||
<> | ||
{"Published on "} | ||
{new Date(published).toLocaleDateString()} at{" "} | ||
{new Date(published).toLocaleTimeString(navigator.language, { | ||
hour: "2-digit", | ||
minute: "2-digit", | ||
hour12: false, | ||
})} | ||
</> |
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.
🛠️ Refactor suggestion
Refactor date rendering logic to improve maintainability
The rendering logic for displaying dates is duplicated and can be consolidated to reduce repetition. Consider abstracting this logic into a helper function or component.
Suggested refactor:
Define a helper function to render date and time:
const renderDate = (label: string, date: string | Date) => (
<>
{label} {new Date(date).toLocaleDateString()} at{" "}
{new Date(date).toLocaleTimeString(navigator.language, {
hour: "2-digit",
minute: "2-digit",
hour12: false,
})}
</>
);
Update the rendering code:
{(new Date(updatedAt).getTime() - new Date(published).getTime()) >= ONE_MINUTE_IN_MS ? (
<>
- {"Last updated on "}
- {new Date(updatedAt).toLocaleDateString()} at{" "}
- {new Date(updatedAt).toLocaleTimeString(navigator.language, {
- hour: "2-digit",
- minute: "2-digit",
- hour12: false,
- })}
+ {renderDate("Last updated on", updatedAt)}
</>
) : (
<>
- {"Published on "}
- {new Date(published).toLocaleDateString()} at{" "}
- {new Date(published).toLocaleTimeString(navigator.language, {
- hour: "2-digit",
- minute: "2-digit",
- hour12: false,
- })}
+ {renderDate("Published on", published)}
</>
)}
app/(app)/my-posts/_client.tsx
Outdated
): postStatus === PostStatus.DRAFT ? ( | ||
<small> | ||
Last Updated on {" "} | ||
{new Date(updatedAt).toLocaleDateString()} at{" "} | ||
{new Date(updatedAt).toLocaleTimeString( | ||
navigator.language, | ||
{ | ||
hour: "2-digit", | ||
minute: "2-digit", | ||
hour12: false, | ||
}, | ||
)} | ||
</small> | ||
): null} |
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.
🛠️ Refactor suggestion
Refactor date rendering logic in draft posts
Similar to the previous suggestion, the code for rendering the "Last Updated on" date in draft posts can also utilize the helper function to avoid duplication.
Update your code as follows:
<small>
- Last Updated on {" "}
- {new Date(updatedAt).toLocaleDateString()} at{" "}
- {new Date(updatedAt).toLocaleTimeString(
- navigator.language,
- {
- hour: "2-digit",
- minute: "2-digit",
- hour12: false,
- },
- )}
+ {renderDate("Last Updated on", updatedAt)}
</small>
Committable suggestion was skipped due to low confidence.
@NiallJoeMaher could you have a look on this one? |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/(app)/my-posts/_client.tsx (1)
166-178
: LGTM: Enhanced timestamp display logic implementedThe new conditional rendering for published posts accurately implements the PR objectives. It correctly distinguishes between posts that were just published and those that were updated after publication.
Suggestion for improvement:
Consider defining a constant for the one-minute threshold to enhance readability:const ONE_MINUTE_IN_MS = 60000; // Then use it in the condition: new Date(updatedAt).getTime() - new Date(published).getTime() >= ONE_MINUTE_IN_MSThis change would make the code more self-documenting and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/(app)/my-posts/_client.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/(app)/my-posts/_client.tsx (4)
27-36
: Great job implementing therenderDate
helper function!This implementation effectively addresses the previous review comment about refactoring the date rendering logic. It reduces code duplication and improves maintainability. The function is well-structured and follows best practices.
127-135
: LGTM: Destructuring updated to includeupdatedAt
The addition of
updatedAt
to the destructured properties is consistent with the new date rendering logic. This change aligns with the PR objectives and supports the enhanced timestamp display functionality.
162-164
: LGTM: Scheduled post date rendering updatedThe use of the new
renderDate
function for scheduled posts is correct and consistent with the refactoring. This change improves code consistency and readability.
Line range hint
1-265
: Overall assessment: Well-implemented enhancements to post sorting and timestamp displayThe changes in this PR successfully implement the objectives outlined in the PR summary. Key improvements include:
- Introduction of the
renderDate
helper function, which addresses previous code duplication concerns.- Enhanced logic for displaying timestamps, distinguishing between newly published and updated posts.
- Consistent use of the new
renderDate
function throughout the component.These changes improve code maintainability and provide users with more accurate information about post status. The implementation is solid, with only minor suggestions for further improvement.
Great work on this PR!
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
LGTM! Thanks for this! 🌮 🦖
✨ Codu Pull Request 💻
Fixes #(1088)
Pull Request details
Added support for sorted Drat posts
Added support for sorted Published posts
In published posts section as the post is published update post is done so updatedAt is set to slightly greater time than published so use 1 minute time difference to distinguish between a published post and a post that published but updated later
Any Breaking changes
NONE
Associated Screenshots