feat: add save memo #11
Conversation
WalkthroughThe update enhances the activities feature by integrating memo editing directly into the activities start page, allowing users to edit memos associated with activities in real-time. It simplifies the database schema by merging memos with activities, thereby streamlining the user experience and backend logic. The addition of a debounced editor ensures efficient memo updates, making the application more responsive and user-friendly. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/app/activities/[activity-id]/page.tsx (2 hunks)
- src/app/activities/action.ts (2 hunks)
- src/app/activities/model.ts (2 hunks)
- src/app/activities/repository.ts (2 hunks)
- src/app/memos/features/Editor.tsx (1 hunks)
- src/hooks/useDebounce.ts (1 hunks)
- src/lib/ddl.sql (1 hunks)
Additional comments: 8
src/hooks/useDebounce.ts (1)
- 3-14: The implementation of the
useDebouncehook correctly creates a memoized version of the debounced function and cleans up by cancelling any pending executions when the component unmounts or the dependencies change. This is a good use of bothuseMemoanduseEffectto ensure that the debounced function behaves as expected across re-renders.src/app/activities/[activity-id]/page.tsx (1)
- 17-17: The early return pattern used here when
activityisnullorundefinedis a clean way to handle the absence of an activity. This prevents the rest of the component from rendering when there is no activity to display, which is a good practice for improving the user experience and performance.src/app/activities/action.ts (1)
- 23-29: The
updateActivityActionfunction correctly updates an activity using theupdateActivityfunction from the model layer. This function is well-structured and follows best practices for async actions, including awaiting the result of theupdateActivitycall. It's a good practice to handle potential errors that might occur during the update process, which could be done using a try-catch block or ensuring that error handling is performed at a higher level in the application.src/app/memos/features/Editor.tsx (1)
- 8-34: The
Editorcomponent is well-implemented, providing a user interface for editing memo content associated with an activity. The use of theuseDebouncehook for handling updates is a good practice, as it improves performance by reducing the frequency of update operations. However, it's important to ensure that theupdateActivityActionfunction is properly handling asynchronous operations and potential errors, as this could affect the reliability of memo updates. Consider adding error handling within the debounced callback or ensuring that theupdateActivityActionfunction itself properly handles errors.src/lib/ddl.sql (1)
- 14-14: The addition of the
memoTEXT field to theactivitiestable is correctly implemented. This change supports storing memo content directly within each activity, simplifying the data model by eliminating the need for a separatememostable. Ensure that any existing data migration scripts or procedures are updated to accommodate this schema change, including migrating any existing memo content from the old structure to the new one.src/app/activities/model.ts (2)
- 11-11: The addition of the
memofield to theActivitytype is correctly implemented, aligning with the PR objectives to support memo functionality within the application. This change allows theActivitytype to accurately represent the data model, including memo content associated with activities.- 63-71: The
updateActivityfunction is correctly implemented to update activities in the repository, including their memo content. This function is an essential part of the application logic to support the new memo functionality. Ensure that error handling is adequately addressed either within this function or at the call sites to manage any issues that may arise during the update process.src/app/activities/repository.ts (1)
- 9-9: The addition of the
updatemethod to theActivityRepositoryinterface is correctly implemented, enabling the updating of activities in the repository. This change is crucial for supporting the memo functionality by allowing updates to thememofield of activities.
| function debounce(callback: Function, delay: number, stop: boolean = false) { | ||
| let timer: string | number | NodeJS.Timeout | undefined; | ||
|
|
||
| function debounced(...args: any[]) { | ||
| clearTimeout(timer); | ||
| timer = setTimeout(() => { | ||
| callback(...args); | ||
| }, delay); | ||
| } | ||
|
|
||
| debounced.cancel = () => clearTimeout(timer); | ||
|
|
||
| return debounced; | ||
| } |
There was a problem hiding this comment.
The debounce function correctly implements the debouncing logic, including the ability to cancel a pending execution. However, the type of the timer variable is overly broad (string | number | NodeJS.Timeout | undefined). Since this code is intended to run in a browser environment (as indicated by the use of setTimeout and clearTimeout), the type should be simplified to NodeJS.Timeout | undefined for Node.js environments or number | undefined for browser environments, depending on the target environment of this application.
Additionally, there's no direct handling of the stop parameter in the function body, which is declared but never used. If the intention was to provide a way to immediately invoke the callback without waiting for the delay, this functionality is not implemented. Consider removing the stop parameter if it's not needed or implementing the intended functionality.
- function debounce(callback: Function, delay: number, stop: boolean = false) {
+ function debounce(callback: Function, delay: number) {
- let timer: string | number | NodeJS.Timeout | undefined;
+ let timer: number | undefined; // Adjust based on target environmentCommittable 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.
| function debounce(callback: Function, delay: number, stop: boolean = false) { | |
| let timer: string | number | NodeJS.Timeout | undefined; | |
| function debounced(...args: any[]) { | |
| clearTimeout(timer); | |
| timer = setTimeout(() => { | |
| callback(...args); | |
| }, delay); | |
| } | |
| debounced.cancel = () => clearTimeout(timer); | |
| return debounced; | |
| } | |
| function debounce(callback: Function, delay: number) { | |
| let timer: number | undefined; // Adjust based on target environment | |
| function debounced(...args: any[]) { | |
| clearTimeout(timer); | |
| timer = setTimeout(() => { | |
| callback(...args); | |
| }, delay); | |
| } | |
| debounced.cancel = () => clearTimeout(timer); | |
| return debounced; | |
| } |
| <BasicLayout> | ||
| {!activity ? ( | ||
| "존재하지 않는 작업이에요!" | ||
| ) : ( | ||
| <div className="flex flex-col items-center w-full gap-y-4"> | ||
| <Timer activity={activity} /> | ||
| <Editor activity={activity} /> | ||
| </div> | ||
| )} | ||
| </BasicLayout> |
There was a problem hiding this comment.
The conditional rendering logic correctly displays a message when the activity object is not present and renders the Editor component along with the Timer component when it is. This approach is effective in handling the UI based on the activity object's existence. However, the conditional check within the BasicLayout (!activity ? ... : ...) is redundant because of the early return pattern implemented earlier. Since the early return ensures activity is not null or undefined at this point, the conditional rendering can be simplified by directly rendering the Editor and Timer components without the check.
- {!activity ? (
- "존재하지 않는 작업이에요!"
- ) : (
<div className="flex flex-col items-center w-full gap-y-4">
<Timer activity={activity} />
<Editor activity={activity} />
</div>
- )}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.
| <BasicLayout> | |
| {!activity ? ( | |
| "존재하지 않는 작업이에요!" | |
| ) : ( | |
| <div className="flex flex-col items-center w-full gap-y-4"> | |
| <Timer activity={activity} /> | |
| <Editor activity={activity} /> | |
| </div> | |
| )} | |
| </BasicLayout> | |
| <BasicLayout> | |
| <div className="flex flex-col items-center w-full gap-y-4"> | |
| <Timer activity={activity} /> | |
| <Editor activity={activity} /> | |
| </div> | |
| </BasicLayout> |
| async update({ activity }: { activity: Activity }) { | ||
| const session = await auth(); | ||
|
|
||
| await sql.query(`UPDATE activities | ||
| SET memo = '${activity.memo}' | ||
| WHERE "userId" = ${session.user.id} | ||
| AND id = ${activity.id}`); | ||
| }, |
There was a problem hiding this comment.
The implementation of the update method in the repository correctly updates the memo field of an activity. However, the use of template literals to construct the SQL query introduces a risk of SQL injection. Consider using parameterized queries to mitigate this risk and ensure the application's security.
- await sql.query(`UPDATE activities SET memo = '${activity.memo}' WHERE "userId" = ${session.user.id} AND id = ${activity.id}`);
+ await sql.query('UPDATE activities SET memo = $1 WHERE "userId" = $2 AND id = $3', [activity.memo, session.user.id, activity.id]);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.
| async update({ activity }: { activity: Activity }) { | |
| const session = await auth(); | |
| await sql.query(`UPDATE activities | |
| SET memo = '${activity.memo}' | |
| WHERE "userId" = ${session.user.id} | |
| AND id = ${activity.id}`); | |
| }, | |
| async update({ activity }: { activity: Activity }) { | |
| const session = await auth(); | |
| await sql.query('UPDATE activities SET memo = $1 WHERE "userId" = $2 AND id = $3', [activity.memo, session.user.id, activity.id]); | |
| }, |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/app/activities/[activity-id]/page.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/app/activities/[activity-id]/page.tsx
|
Summary by CodeRabbit
New Features
Editorcomponent to allow users to edit and update memos associated with activities.memofield for user notes.Refactor
ActivitiesStartPagecomponent to conditionally render theEditorcomponent based on activity availability.useDebouncehook for optimizing performance during memo editing.Database Changes
activitiestable schema to include amemofield directly, removing the need for a separatememostable.