Add new Navigation for editor #1163
Conversation
|
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces multiple changes across various files, including the removal of a page component for creating posts, the restructuring of layout components, and enhancements to the editor's functionality. A new navigation component for the editor is added, along with a not-found page. The handling of post statuses has been updated, and session management has been integrated into the editor's create component. Overall, the changes streamline the user experience and improve session handling within the editor interface. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
NiallJoeMaher
had a problem deploying
to
production
GitHub Actions
Failure
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
app/(editor)/create/[[...paramsArr]]/not-found.tsx (1)
3-5: Rename component to follow React naming conventions.The component's functionality is correct, but its name should be in PascalCase to follow React component naming conventions.
Apply this change:
-const notfound = () => { +const NotFound = () => { return <NotFound />; };utils/post.ts (1)
Line range hint
7-11: Structured export of status constantsThe introduction of the
statusobject is a good approach. It provides a structured way to access the status constants while maintaining backwards compatibility. The use of shorthand property names is clean and follows modern JavaScript practices.For consistency with the
PostStatustype, consider usingas constfor the entire object:export const status = { SCHEDULED, PUBLISHED, DRAFT, } as const;This would make
statusa readonly object with literal type values, further enhancing type safety.app/(editor)/create/[[...paramsArr]]/page.tsx (2)
5-28: LGTM: Comprehensive metadata with room for improvement.The metadata object is well-structured and provides excellent SEO optimization. It includes relevant title, description, keywords, and Open Graph properties, which are crucial for discoverability and social sharing.
Consider using an environment variable for the
metadataBaseURL to make it more flexible across different environments:- metadataBase: new URL("https://www.codu.co"), + metadataBase: new URL(process.env.NEXT_PUBLIC_BASE_URL || "https://www.codu.co"),Don't forget to add the
NEXT_PUBLIC_BASE_URLto your environment variables.
30-37: LGTM: Well-structured server-side rendered component with proper authentication.The Page component effectively handles server-side authentication and renders the appropriate content based on the user's session status. It follows best practices for Next.js server-side rendering and authentication flow.
Consider adding error handling to catch any potential issues with
getServerAuthSession():export default async function Page() { - const session = await getServerAuthSession(); + try { + const session = await getServerAuthSession(); + if (!session) { + redirect("/get-started"); + } + return <Content session={session} />; + } catch (error) { + console.error("Error fetching session:", error); + redirect("/error"); // Redirect to an error page + } - if (!session) { - redirect("/get-started"); - } - - return <Content session={session} />; }This change will help gracefully handle any unexpected errors during the authentication process.
app/(app)/layout.tsx (1)
Provider Components Still in Use:
ThemeProvideris still imported and used inapp/layout.tsxandapp/(app)/layout.tsx.TRPCReactProviderremains imported and utilized in bothapp/layout.tsxandapp/(app)/layout.tsx.AuthProvideris still present inapp/layout.tsxandapp/(app)/layout.tsx.ProgressBarcontinues to be used in multiple files, includingapp/layout.tsxandapp/(app)/layout.tsx.These components were intended to be removed from
app/(app)/layout.tsx, but they are still actively used, which may lead to inconsistencies in theming, state management, authentication, and loading indicators.🔗 Analysis chain
Line range hint
1-15: Verify the impact of removed provider componentsSeveral important provider components have been removed from the imports and the layout structure:
ThemeProviderTRPCReactProviderAuthProviderProgressBarThese changes could potentially affect the application's theming, state management, authentication, and loading indicators.
Please ensure that these functionalities are still available where needed throughout the application. Run the following script to check for their usage in other parts of the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of removed components in other files echo "Checking for ThemeProvider usage:" rg "ThemeProvider" echo "Checking for TRPCReactProvider usage:" rg "TRPCReactProvider" echo "Checking for AuthProvider usage:" rg "AuthProvider" echo "Checking for ProgressBar usage:" rg "ProgressBar"Length of output: 2586
Script:
#!/bin/bash # Description: Check for usage of removed components in other files echo "Checking for ThemeProvider usage:" rg "ThemeProvider" echo "Checking for TRPCReactProvider usage:" rg "TRPCReactProvider" echo "Checking for AuthProvider usage:" rg "AuthProvider" echo "Checking for ProgressBar usage:" rg "ProgressBar"Length of output: 2694
app/(editor)/create/[[...paramsArr]]/navigation.tsx (1)
30-32: Handle the notification query more effectivelyThe
useQueryhook for fetching notification count is enabled based on the session:const { data: count } = api.notification.getCount.useQuery(undefined, { enabled: !!session, });Consider initializing
countto0when there is no session to avoid potentialundefinedissues and simplify the usage:-const { data: count } = api.notification.getCount.useQuery(undefined, { +const { data: count = 0 } = api.notification.getCount.useQuery(undefined, {app/(editor)/create/[[...paramsArr]]/_client.tsx (3)
Line range hint
358-364: Optimize auto-save logic in useEffectThe auto-save useEffect depends on several conditions. To improve readability and prevent unnecessary saves, consider restructuring the conditions:
- Combine related conditions using logical operators.
- Add comments to explain the purpose of each condition.
- Ensure that
savePost()is only called when necessary.
389-392: Add missing dependency to 'handlePublish' functionIf
handlePublishdepends on any state or props (e.g.,isDisabled), ensure that these are included in its dependency array ifhandlePublishis wrapped withuseCallback. This prevents stale values from being used when the function is invoked.
776-778: Remove unnecessary commentsThe comments
{/* Remove the save status messages */}and{/* Next button already removed */}might be outdated or unnecessary. Consider removing them to keep the codebase clean and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- app/(app)/create/[[...paramsArr]]/page.tsx (0 hunks)
- app/(app)/layout.tsx (1 hunks)
- app/(app)/my-posts/_client.tsx (4 hunks)
- app/(editor)/create/[[...paramsArr]]/_client.tsx (10 hunks)
- app/(editor)/create/[[...paramsArr]]/navigation.tsx (1 hunks)
- app/(editor)/create/[[...paramsArr]]/not-found.tsx (1 hunks)
- app/(editor)/create/[[...paramsArr]]/page.tsx (1 hunks)
- app/layout.tsx (2 hunks)
- utils/post.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/(app)/create/[[...paramsArr]]/page.tsx
🧰 Additional context used
🔇 Additional comments (22)
app/(editor)/create/[[...paramsArr]]/not-found.tsx (3)
1-1: LGTM: Import statement is correct and follows good practices.The import statement correctly uses the
@alias for importing from the project root, and the component name matches the file name.
7-7: LGTM: Default export is correct.The default export statement is correct. If you rename the component as suggested earlier, remember to update this line as well:
-export default notfound; +export default NotFound;
1-7: Implementation aligns well with PR objectives.This new
not-found.tsxfile successfully implements a dedicated UI for scenarios where content is not found in the editor create route. It aligns with the PR objectives of enhancing the editor view and improving navigation. The implementation is simple and effective, leveraging an existingNotFoundcomponent.Consider the following suggestions to further improve the code:
- Rename the component to
NotFound(PascalCase) to follow React naming conventions.- Update the default export statement if you rename the component.
These minor adjustments will enhance code consistency and maintainability.
utils/post.ts (3)
1-3: Improved type safety withas constassertionsThe use of
as constfor the status constants is a good practice. It creates readonly literal types, which enhances type inference and safety when these constants are used throughout the codebase. This change maintains the existing behavior while providing better type checking.
5-5: Enhanced type definition for PostStatusThe new
PostStatustype definition as a union of constant types is an improvement. It ensures thatPostStatusalways accurately reflects the available status constants, reducing the risk of inconsistencies. This approach is more type-safe and flexible than the previous enum-like object.
Line range hint
1-38: Overall improvement in post status managementThe changes in this file significantly enhance the type safety and organization of post status-related code. By using
as constassertions, introducing a union type forPostStatus, and providing a structuredstatusobject, the code becomes more robust and less prone to errors.These modifications align well with the PR objectives of improving the editor's functionality and streamlining the user experience. They provide a solid foundation for consistent handling of post statuses across the application.
The utility functions
getPostStatusandisValidScheduleTimeremain unchanged, preserving existing behavior while benefiting from the improved type definitions.app/(editor)/create/[[...paramsArr]]/page.tsx (2)
1-3: LGTM: Imports are appropriate and follow best practices.The imports are concise and relevant to the file's functionality. They include necessary Next.js utilities, a local component, and an authentication helper.
1-37: Overall, excellent implementation of the editor page.This new file successfully implements the server-side rendered page for editing posts, aligning well with the PR objectives. It includes proper authentication checks, comprehensive metadata for SEO optimization, and a clean component structure. The changes contribute to the new navigation system for the editor and address the requirements outlined in the linked issue #1079.
A few minor suggestions were made to improve environment flexibility and error handling, but these are not critical issues. Great job on this implementation!
app/(app)/layout.tsx (2)
82-88: Simplified layout structureThe layout structure has been significantly simplified:
- Removed wrapping components like
ThemeProvider,AuthProvider, andTRPCReactProvider.- Directly rendering
Nav,children, andFooter.While this simplification can improve readability, it's crucial to ensure that the removed context providers don't negatively impact the functionality of child components.
Please verify that all child components still have access to necessary context and state management. You may need to update components that relied on these providers.
82-86: Consistency with PR objectivesThe changes to the
Navcomponent align with the PR objectives:
- The navigation bar is still present, which is consistent with the goal of implementing a sticky navigation bar.
- The
sessionprop is passed, which could be used for displaying user-specific information.- The
algoliaSearchConfigis provided, which might be used for search functionality in the navigation.- The
usernameis passed, which aligns with the objective of including a user icon in the navigation.However, there are some aspects of the PR objectives that are not directly visible in this file:
- The "Publish" button relocation
- The save status indicator
- Dark mode toggle
Please ensure that these features are implemented in the
Navcomponent or other relevant parts of the application. Run the following script to check for their implementation:#!/bin/bash # Description: Check for implementation of required features echo "Checking for Publish button implementation:" rg -i "publish.*button" echo "Checking for save status indicator:" rg -i "save.*status|status.*indicator" echo "Checking for dark mode toggle:" rg -i "dark.*mode.*toggle|theme.*toggle"app/(app)/my-posts/_client.tsx (5)
136-138: LGTM! Correct usage of the new status object.The post status determination logic has been correctly updated to use the new
status.DRAFTinstead ofPostStatus.DRAFT. This change is consistent with the import statement modification and maintains the existing logic.
161-161: LGTM! Correct usage of the new status object for scheduled posts.The condition for checking scheduled post status has been properly updated to use
status.SCHEDULED. This change is consistent with the new status object structure and maintains the existing logic.
165-165: LGTM! Consistent usage of the new status object for published and draft posts.The conditions for checking published and draft post statuses have been correctly updated to use
status.PUBLISHEDandstatus.DRAFTrespectively. These changes are consistent with the new status object structure and maintain the existing logic.Also applies to: 177-177
Line range hint
1-248: Summary: Successful migration to new status object structureThe changes in this file successfully migrate the usage of
PostStatusenum to the newstatusobject structure. All references to post statuses have been updated consistently, maintaining the existing logic while improving type safety and clarity. These changes align with the PR objectives and the restructuring mentioned in the AI-generated summary.Key points:
- Import statement updated to use
statusandgetPostStatus.- All post status checks (draft, scheduled, published) now use the new
statusobject.- The overall functionality of the
MyPostscomponent remains unchanged.These updates contribute to a more maintainable and type-safe codebase.
21-21: LGTM! Verify consistency across the codebase.The import statement has been updated to use the new
statusobject andgetPostStatusfunction. This change aligns with the restructuring ofPostStatusmentioned in the AI summary.To ensure consistency, let's verify that this change has been applied across the codebase:
app/layout.tsx (3)
1-1: Import Statement Added CorrectlyThe import of
headersfrom"next/headers"is appropriate for accessing request headers in a server component.
9-13: New Providers Imported SuccessfullyThe imports for
ThemeProvider,TRPCReactProvider,AuthProvider,ProgressBar, andPromptProviderhave been added correctly and reference the appropriate modules.
71-80: Providers Integrated Properly into LayoutThe nesting of the new providers within the
A11yProviderenhances the modularity and state management of the application. WrappingchildrenwithAuthProvider,ThemeProvider,TRPCReactProvider, andPromptProviderensures that these contexts are available throughout the app.app/(editor)/create/[[...paramsArr]]/navigation.tsx (1)
60-157:⚠️ Potential issueAdd the dark mode toggle to the navigation bar
According to the PR objectives, the navigation bar should include a dark mode toggle. The current implementation does not include this feature. Please add the dark mode toggle to align with the design specifications.
app/(editor)/create/[[...paramsArr]]/_client.tsx (3)
247-249: Ensure correct computation of 'currentPostStatus'The calculation of
currentPostStatusdepends ondata.published. Verify thatdata.publishedis always a valid date when the post is not a draft. Additionally, consider handling cases wheredata.publishedmight be undefined or invalid to prevent runtime errors.
396-403: Verify props passed to 'EditorNav' componentEnsure that all props passed to
EditorNavare correct and necessary:
session: Confirm thatEditorNavrequires the session object.username: Double-check thatsession?.user?.nameis the correct property for the username.postStatus,unsavedChanges,onPublish,isDisabled: Verify that these props are used appropriately withinEditorNav.
Line range hint
577-583: Handle 'Save draft' button rendering logicThe
Save draftbutton is conditionally rendered whencurrentPostStatus === status.DRAFT. Ensure thatcurrentPostStatusaccurately reflects the post's status to prevent the button from appearing or disappearing unexpectedly.
| import { Menu, Transition } from "@headlessui/react"; | ||
| import { BellIcon } from "@heroicons/react/20/solid"; | ||
| import { signOut } from "next-auth/react"; | ||
| import { PromptLink as Link } from "@/components/PromptService/PromptLink"; |
There was a problem hiding this comment.
Confirm the correct Link component and to prop usage
The code imports PromptLink as Link:
import { PromptLink as Link } from "@/components/PromptService/PromptLink";Please verify that PromptLink is the intended component for navigation links and that it supports the to prop used throughout the file. If you intended to use Next.js's Link component, consider importing it from next/link and using the href prop instead.
| <Link to="/" className="flex items-center"> | ||
| <Logo className="h-6 w-auto" /> | ||
| </Link> |
There was a problem hiding this comment.
Update Link component to use the href prop
In the logo link, the Link component uses the to prop:
<Link to="/" className="flex items-center">
<Logo className="h-6 w-auto" />
</Link>If using Next.js's Link component, it should use the href prop. Modify the code as follows:
-<Link to="/" className="flex items-center">
+<Link href="/" className="flex items-center">| <Link | ||
| to="/notifications" | ||
| className="focus-style relative rounded-full p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-600 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-white" |
There was a problem hiding this comment.
Ensure Link component uses the correct prop for navigation
Here, the Link component is used with the to prop:
<Link
to="/notifications"
className="focus-style relative rounded-full p-1 text-neutral-500 hover:bg-neutral-100 hover:text-neutral-600 dark:text-neutral-400 dark:hover:bg-neutral-800 dark:hover:text-white"
>If Link should use the href prop, update it accordingly:
-<Link
- to="/notifications"
+<Link
+ href="/notifications"| to={item.href} | ||
| className={`${ | ||
| active ? "bg-neutral-100" : "" | ||
| } block px-4 py-2 text-sm text-neutral-700`} | ||
| > | ||
| {item.name} | ||
| </Link> | ||
| ) |
There was a problem hiding this comment.
Modify Link component to use href instead of to
Within the user menu, the Link component uses to={item.href}:
<Link
to={item.href}
className={`${
active ? "bg-neutral-100" : ""
} block px-4 py-2 text-sm text-neutral-700`}
>
{item.name}
</Link>Ensure the Link component uses the href prop:
-<Link
- to={item.href}
+<Link
+ href={item.href}| const getStatusText = () => { | ||
| if (postStatus === null) return null; | ||
|
|
||
| switch (postStatus) { | ||
| case status.DRAFT: | ||
| return unsavedChanges ? "Draft - Unsaved changes" : "Draft - Saved"; | ||
| case status.PUBLISHED: | ||
| return unsavedChanges ? "Published - Unsaved changes" : "Published"; | ||
| case status.SCHEDULED: | ||
| return unsavedChanges ? "Scheduled - Unsaved changes" : "Scheduled"; | ||
| default: | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the getStatusText function
You can streamline the getStatusText function by reducing repetition:
const getStatusText = () => {
- if (postStatus === null) return null;
+ if (!postStatus) return null;
const statusMap = {
[status.DRAFT]: "Draft",
[status.PUBLISHED]: "Published",
[status.SCHEDULED]: "Scheduled",
};
- switch (postStatus) {
- case status.DRAFT:
- return unsavedChanges ? "Draft - Unsaved changes" : "Draft - Saved";
- case status.PUBLISHED:
- return unsavedChanges ? "Published - Unsaved changes" : "Published";
- case status.SCHEDULED:
- return unsavedChanges ? "Scheduled - Unsaved changes" : "Scheduled";
- default:
- return null;
- }
+ const baseText = statusMap[postStatus];
+ if (!baseText) return null;
+ return unsavedChanges ? `${baseText} - Unsaved changes` : baseText;
};📝 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 getStatusText = () => { | |
| if (postStatus === null) return null; | |
| switch (postStatus) { | |
| case status.DRAFT: | |
| return unsavedChanges ? "Draft - Unsaved changes" : "Draft - Saved"; | |
| case status.PUBLISHED: | |
| return unsavedChanges ? "Published - Unsaved changes" : "Published"; | |
| case status.SCHEDULED: | |
| return unsavedChanges ? "Scheduled - Unsaved changes" : "Scheduled"; | |
| default: | |
| return null; | |
| } | |
| }; | |
| const getStatusText = () => { | |
| if (!postStatus) return null; | |
| const statusMap = { | |
| [status.DRAFT]: "Draft", | |
| [status.PUBLISHED]: "Published", | |
| [status.SCHEDULED]: "Scheduled", | |
| }; | |
| const baseText = statusMap[postStatus]; | |
| if (!baseText) return null; | |
| return unsavedChanges ? `${baseText} - Unsaved changes` : baseText; | |
| }; |
| import { type Session } from "next-auth"; | ||
|
|
||
| const Create = () => { | ||
| const Create = ({ session }: { session: Session | null }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass 'session' prop to 'EditorNav' component
The session prop is now passed to the Create component and then forwarded to the EditorNav component. Ensure that this prop is necessary and used appropriately within EditorNav. If EditorNav can access the session internally (e.g., via a context or a hook), consider removing the prop to simplify the component interface.
| const [uploadStatus, setUploadStatus] = useState< | ||
| "loading" | "error" | "success" | "default" | ||
| >("default"); | ||
| const [postStatus, setPostStatus] = useState<PostStatus | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize 'postStatus' with consistent default value
The postStatus state is initialized with null. Since postStatus is used to determine the UI state, consider initializing it with a default status, such as status.DRAFT, to avoid potential null checks elsewhere in the code.
| setPostStatus( | ||
| published ? getPostStatus(new Date(published)) : status.DRAFT, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set 'postStatus' consistently after data fetch
After fetching the post data, postStatus is updated based on published. Ensure that this logic aligns with how postStatus is used elsewhere, and that it doesn't conflict with currentPostStatus. Consolidating these statuses might improve code clarity.
| if ((title + body).length < 5) { | ||
| setPostStatus(null); | ||
| } else if (postStatus === null) { | ||
| setPostStatus(status.DRAFT); | ||
| } | ||
| }, [title, body]); |
There was a problem hiding this comment.
Avoid setting 'postStatus' to null based on content length
Setting postStatus to null when the combined length of title and body is less than 5 might lead to unintended UI states. Instead, consider using a separate state or validation function to handle content length requirements without altering the post status.
| {currentPostStatus === status.PUBLISHED | ||
| ? "Save changes" | ||
| : currentPostStatus === status.DRAFT | ||
| ? isPostScheduled | ||
| ? "Schedule" | ||
| : "Publish now" | ||
| : currentPostStatus === status.SCHEDULED | ||
| ? isPostScheduled | ||
| ? "Update schedule" | ||
| : "Publish now" | ||
| : "Publish"} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested ternary operators for button label
The nested ternary operators used to determine the button label make the code difficult to read and maintain. Consider extracting this logic into a separate function or variable to improve readability.
Apply this refactor:
- {currentPostStatus === status.PUBLISHED
- ? "Save changes"
- : currentPostStatus === status.DRAFT
- ? isPostScheduled
- ? "Schedule"
- : "Publish now"
- : currentPostStatus === status.SCHEDULED
- ? isPostScheduled
- ? "Update schedule"
- : "Publish now"
- : "Publish"}
+ {getButtonLabel(currentPostStatus, isPostScheduled)}Define the getButtonLabel function:
const getButtonLabel = (postStatus, isPostScheduled) => {
if (postStatus === status.PUBLISHED) {
return "Save changes";
} else if (postStatus === status.DRAFT) {
return isPostScheduled ? "Schedule" : "Publish now";
} else if (postStatus === status.SCHEDULED) {
return isPostScheduled ? "Update schedule" : "Publish now";
} else {
return "Publish";
}
};
NiallJoeMaher
had a problem deploying
to
production
GitHub Actions
Error
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
e2e/articles.spec.ts (1)
184-185: LGTM! Consider adding a check for the new navigation bar.The changes correctly update the test to use the new "Publish" button, which aligns with the PR objectives of relocating the "Publish" button to the navigation bar. This update accurately reflects the new user interface and workflow for publishing an article.
To further improve the test coverage and align with the PR objectives, consider adding a check for the visibility of the new navigation bar before interacting with the "Publish" button. This could help ensure that the new layout is correctly implemented. For example:
await expect(page.getByTestId('editor-navigation-bar')).toBeVisible(); await expect(page.getByRole("button", { name: "Publish" })).toBeVisible(); await page.getByRole("button", { name: "Publish" }).click();Replace 'editor-navigation-bar' with the actual test ID or selector for the new navigation bar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/(editor)/create/[[...paramsArr]]/_client.tsx (9 hunks)
- app/(editor)/create/[[...paramsArr]]/navigation.tsx (1 hunks)
- e2e/articles.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/(editor)/create/[[...paramsArr]]/navigation.tsx
🧰 Additional context used
🔇 Additional comments (4)
app/(editor)/create/[[...paramsArr]]/_client.tsx (4)
25-25: LGTM: Import statements and type definitions updated appropriately.The new imports and type definitions align well with the changes in the component's functionality, including improved navigation, post status management, and session handling.
Also applies to: 29-34, 38-39
41-41: LGTM: Component signature and state updated for session and post status.The addition of the
sessionprop andpostStatusstate variable aligns with the new session handling and post status management functionality.Also applies to: 62-62
396-403: LGTM: EditorNav component integrated effectively.The addition of the
EditorNavcomponent with appropriate props improves the code structure by separating the navigation logic.
Line range hint
1-858: Overall improvements with some suggestions for further refinement.The changes in this file align well with the PR objectives, improving the editor's navigation, post status management, and overall structure. The addition of the
EditorNavcomponent and enhanced post status handling are particularly noteworthy improvements.However, there are a few areas that could be further refined:
- Consider improving error handling for draft fetch failures.
- Reconsider the approach of setting
postStatusto null based on content length.- Simplify complex conditional logic, such as the nested ternary operators for the publish button label.
Addressing these points would further enhance the code's readability and maintainability.
| notFound(); | ||
| } | ||
| }, [draftFetchError, isError, isSuccess]); | ||
| }, [draftFetchError, isError]); |
There was a problem hiding this comment.
Consider alternative error handling for draft fetch failures.
While using notFound() for draft fetch errors improves the UX by avoiding error toasts, it might confuse users who expect to edit their draft. Consider displaying an error message or redirecting to a dedicated error page that explains the issue more clearly.
| const currentPostStatus = data?.published | ||
| ? getPostStatus(new Date(data.published)) | ||
| : PostStatus.DRAFT; | ||
| : status.DRAFT; |
There was a problem hiding this comment.
Reconsider setting postStatus to null based on content length.
While the changes improve post status management, setting postStatus to null when the combined length of title and body is less than 5 might lead to unintended UI states. Instead, consider using a separate state or validation function to handle content length requirements without altering the post status.
Also applies to: 345-356
| const handlePublish = () => { | ||
| if (isDisabled) return; | ||
| setOpen(true); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested ternary operators for button label.
The nested ternary operators used to determine the button label make the code difficult to read and maintain. Consider extracting this logic into a separate function or variable to improve readability.
Apply this refactor:
- {currentPostStatus === status.PUBLISHED
- ? "Save changes"
- : currentPostStatus === status.DRAFT
- ? isPostScheduled
- ? "Schedule"
- : "Publish now"
- : currentPostStatus === status.SCHEDULED
- ? isPostScheduled
- ? "Update schedule"
- : "Publish now"
- : "Publish"}
+ {getButtonLabel(currentPostStatus, isPostScheduled)}Define the getButtonLabel function:
const getButtonLabel = (postStatus, isPostScheduled) => {
if (postStatus === status.PUBLISHED) {
return "Save changes";
} else if (postStatus === status.DRAFT) {
return isPostScheduled ? "Schedule" : "Publish now";
} else if (postStatus === status.SCHEDULED) {
return isPostScheduled ? "Update schedule" : "Publish now";
} else {
return "Publish";
}
};Also applies to: 577-616
|
I see you dont have much confidence the alpha editor will be ready any time soon 🤣 |
Aha I do! I was going to migrate it to the main editor with a feature flag or the ability so folks can start using it. It needs to come out of alpha into here anyway. |
2 participants
✨ Codu Pull Request 💻
Fixes #1079
Pull Request details
Any Breaking changes
Associated Screenshots
Screen.Recording.2024-10-21.at.00.40.58.mov