[_]: fix: update draft handling in compose message component and hooks#70
Open
jzunigax2 wants to merge 1 commit into
Open
[_]: fix: update draft handling in compose message component and hooks#70jzunigax2 wants to merge 1 commit into
jzunigax2 wants to merge 1 commit into
Conversation
- Introduced `flushPendingDraftSave` to manage draft saving more effectively. - Replaced `draftId` with `resolveDraftId` in `useComposeSend` for better draft resolution. - Enhanced `saveDraft` logic to prevent duplicate saves during concurrent operations. - Updated tests to reflect changes in draft handling and ensure correct functionality.
|
📝 WalkthroughWalkthroughDraft autosave logic in ChangesDraft save coordination and send integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-message/hooks/useComposeSend.test.ts`:
- Around line 314-315: The affected test descriptions in useComposeSend.test
should be rewritten to follow the “When…then…” style without naming
implementation details like sendEmail or draftId. Update the descriptions for
the related tests in the useComposeSend test suite to describe the behavior in
user-facing terms, while keeping the assertions and setup unchanged.
In `@src/components/compose-message/hooks/useDraftMessage.ts`:
- Around line 130-164: Extract the repeated autosave timer cleanup in
useDraftMessage into a shared helper (for example a clearAutosaveTimer-style
function) and reuse it in saveDraft, flushPendingDraftSave, clearDraftRef, and
scheduleAutosave. Make sure the helper still nulls timerRef.current after
clearTimeout, and update each useCallback’s dependency array to include the new
helper so the teardown logic stays consistent and DRY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 680672d6-f875-4472-8563-a33674b6e4d6
📒 Files selected for processing (5)
src/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/hooks/useDraftMessage.test.tssrc/components/compose-message/hooks/useDraftMessage.tssrc/components/compose-message/index.tsx
Comment on lines
+314
to
+315
| test('When sending an email opened from a draft, then the resolved draftId is forwarded to sendEmail so the server consumes the draft', async () => { | ||
| const resolveDraftId = vi.fn().mockResolvedValue('draft-42'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rephrase test descriptions to avoid referencing function and variable names.
Per the coding guidelines, test descriptions should use the "When…then…" pattern without referencing variable, function, or type names. Both descriptions mention sendEmail (function) and draftId (parameter name).
✏️ Suggested rephrasing
- test('When sending an email opened from a draft, then the resolved draftId is forwarded to sendEmail so the server consumes the draft', async () => {
+ test('When sending an email opened from a draft, then the saved draft identifier is included in the send request so the server can consume it', async () => {- test('When sending a brand-new email (no draft saved), then sendEmail is called without a draftId', async () => {
+ test('When sending a brand-new email with no saved draft, then the send request omits any draft identifier', async () => {Also applies to: 330-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useComposeSend.test.ts` around lines 314
- 315, The affected test descriptions in useComposeSend.test should be rewritten
to follow the “When…then…” style without naming implementation details like
sendEmail or draftId. Update the descriptions for the related tests in the
useComposeSend test suite to describe the behavior in user-facing terms, while
keeping the assertions and setup unchanged.
Source: Coding guidelines
Comment on lines
130
to
+164
| const saveDraft = useCallback(async () => { | ||
| if (timerRef.current) { | ||
| clearTimeout(timerRef.current); | ||
| timerRef.current = null; | ||
| } | ||
|
|
||
| const previous = pendingSaveRef.current ?? Promise.resolve(); | ||
| const current = previous.catch(() => undefined).then(performSave); | ||
| pendingSaveRef.current = current; | ||
|
|
||
| activeSavesRef.current += 1; | ||
| setIsSaving(true); | ||
| try { | ||
| const payload = await buildPayload(); | ||
| if (!payload) return; | ||
|
|
||
| if (!draftIdRef.current && isPayloadEmpty(payload, editor)) return; | ||
|
|
||
| if (draftIdRef.current) { | ||
| const { id: newDraftId, receivedAt } = await updateDraft({ draftId: draftIdRef.current, payload }).unwrap(); | ||
| draftIdRef.current = newDraftId; | ||
| draftReceivedAtRef.current = receivedAt; | ||
| } else { | ||
| const { id, receivedAt } = await createDraft(payload).unwrap(); | ||
| draftIdRef.current = id; | ||
| draftReceivedAtRef.current = receivedAt; | ||
| } | ||
| await current; | ||
| } finally { | ||
| setIsSaving(false); | ||
| activeSavesRef.current -= 1; | ||
| if (activeSavesRef.current === 0) setIsSaving(false); | ||
| if (pendingSaveRef.current === current) pendingSaveRef.current = null; | ||
| } | ||
| }, [buildPayload, createDraft, updateDraft, editor]); | ||
| }, [performSave]); | ||
|
|
||
| const flushPendingDraftSave = useCallback(async (): Promise<string | null> => { | ||
| if (timerRef.current) { | ||
| clearTimeout(timerRef.current); | ||
| timerRef.current = null; | ||
| } | ||
| if (pendingSaveRef.current) { | ||
| try { | ||
| await pendingSaveRef.current; | ||
| } catch { | ||
| // Save failed; proceed with the last id that reached the server. no op | ||
| } | ||
| } | ||
| return draftIdRef.current; | ||
| }, []); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Queuing/flush logic is correct; optionally DRY the timer teardown.
The pending-chain serialization and activeSavesRef/isSaving accounting look correct, and flushPendingDraftSave safely awaits the in-flight save before returning the id. The if (timerRef.current) { clearTimeout(...); timerRef.current = null; } block is repeated here in saveDraft and flushPendingDraftSave, and again in clearDraftRef (Lines 172-175); scheduleAutosave (Line 182) also duplicates the clear. Consider extracting a single helper.
♻️ Extract a shared timer helper
+ const clearAutosaveTimer = useCallback(() => {
+ if (timerRef.current) {
+ clearTimeout(timerRef.current);
+ timerRef.current = null;
+ }
+ }, []);
+
const saveDraft = useCallback(async () => {
- if (timerRef.current) {
- clearTimeout(timerRef.current);
- timerRef.current = null;
- }
+ clearAutosaveTimer();Then reuse clearAutosaveTimer() in flushPendingDraftSave, clearDraftRef, and scheduleAutosave, adding it to their dependency arrays.
📝 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.
Suggested change
| const saveDraft = useCallback(async () => { | |
| if (timerRef.current) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| const previous = pendingSaveRef.current ?? Promise.resolve(); | |
| const current = previous.catch(() => undefined).then(performSave); | |
| pendingSaveRef.current = current; | |
| activeSavesRef.current += 1; | |
| setIsSaving(true); | |
| try { | |
| const payload = await buildPayload(); | |
| if (!payload) return; | |
| if (!draftIdRef.current && isPayloadEmpty(payload, editor)) return; | |
| if (draftIdRef.current) { | |
| const { id: newDraftId, receivedAt } = await updateDraft({ draftId: draftIdRef.current, payload }).unwrap(); | |
| draftIdRef.current = newDraftId; | |
| draftReceivedAtRef.current = receivedAt; | |
| } else { | |
| const { id, receivedAt } = await createDraft(payload).unwrap(); | |
| draftIdRef.current = id; | |
| draftReceivedAtRef.current = receivedAt; | |
| } | |
| await current; | |
| } finally { | |
| setIsSaving(false); | |
| activeSavesRef.current -= 1; | |
| if (activeSavesRef.current === 0) setIsSaving(false); | |
| if (pendingSaveRef.current === current) pendingSaveRef.current = null; | |
| } | |
| }, [buildPayload, createDraft, updateDraft, editor]); | |
| }, [performSave]); | |
| const flushPendingDraftSave = useCallback(async (): Promise<string | null> => { | |
| if (timerRef.current) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| if (pendingSaveRef.current) { | |
| try { | |
| await pendingSaveRef.current; | |
| } catch { | |
| // Save failed; proceed with the last id that reached the server. no op | |
| } | |
| } | |
| return draftIdRef.current; | |
| }, []); | |
| const clearAutosaveTimer = useCallback(() => { | |
| if (timerRef.current) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| }, []); | |
| const saveDraft = useCallback(async () => { | |
| clearAutosaveTimer(); | |
| const previous = pendingSaveRef.current ?? Promise.resolve(); | |
| const current = previous.catch(() => undefined).then(performSave); | |
| pendingSaveRef.current = current; | |
| activeSavesRef.current += 1; | |
| setIsSaving(true); | |
| try { | |
| await current; | |
| } finally { | |
| activeSavesRef.current -= 1; | |
| if (activeSavesRef.current === 0) setIsSaving(false); | |
| if (pendingSaveRef.current === current) pendingSaveRef.current = null; | |
| } | |
| }, [performSave]); | |
| const flushPendingDraftSave = useCallback(async (): Promise<string | null> => { | |
| if (timerRef.current) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| if (pendingSaveRef.current) { | |
| try { | |
| await pendingSaveRef.current; | |
| } catch { | |
| // Save failed; proceed with the last id that reached the server. no op | |
| } | |
| } | |
| return draftIdRef.current; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useDraftMessage.ts` around lines 130 -
164, Extract the repeated autosave timer cleanup in useDraftMessage into a
shared helper (for example a clearAutosaveTimer-style function) and reuse it in
saveDraft, flushPendingDraftSave, clearDraftRef, and scheduleAutosave. Make sure
the helper still nulls timerRef.current after clearTimeout, and update each
useCallback’s dependency array to include the new helper so the teardown logic
stays consistent and DRY.
xabg2
reviewed
Jul 9, 2026
| toRecipients, | ||
| inReplyTo: inReplyItemId, | ||
| draftId: draftId ?? undefined, | ||
| resolveDraftId: flushPendingDraftSave, |
Contributor
There was a problem hiding this comment.
Would be better resolvePendingDrafts or smth like that. What do you think?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
flushPendingDraftSaveto manage draft saving more effectively.draftIdwithresolveDraftIdinuseComposeSendfor better draft resolution.saveDraftlogic to prevent duplicate saves during concurrent operations.