feat: add success toast notification after saving image (#14)#16
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitリリースノート
Walkthrough画像保存成功時に短時間表示されるトースト通知を追加しました。CSS（配置・アニメーション）、コンポーネントの状態/タイマー管理、保存成功時のトースト表示呼び出し、および条件付きのARIA対応レンダリングを実装しています。 Changesトースト通知機能
Sequence DiagramsequenceDiagram
participant User
participant App as App Component
participant SaveAPI as Save API
participant Toast as Toast State
participant Render as UI Render
User->>App: Click save image
App->>SaveAPI: POST image
SaveAPI-->>App: Success response
App->>Toast: showToast("Image saved successfully")
Toast->>Toast: setToastMessage(message)
Toast->>Render: Update state
Render->>Render: Render toast-container with message
Note over Render: Toast animates in, fades out after delay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a lightweight success notification after exporting an image, addressing the lack of positive feedback noted in #14.
Changes:
- Introduces toast state + helper in
App.tsxand triggers it on successful save. - Adds toast UI markup to the main layout.
- Adds toast styling + fade-in/out animation in
App.css.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/App.tsx | Adds toast state/helper, shows toast on successful save, renders toast markup. |
| frontend/src/App.css | Styles toast container/toast and defines fade-in/out animation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
40
|
|
||
| const showToast = (message: string) => { | ||
| setToastMessage(message); | ||
| setTimeout(() => setToastMessage(null), 3000); | ||
| }; | ||
|
|
||
| useEffect(() => { |
|
|
||
| {toastMessage && ( | ||
| <div className="toast-container"> | ||
| <div className="toast success">{toastMessage}</div> |
Comment on lines
+35
to
+38
| const showToast = (message: string) => { | ||
| setToastMessage(message); | ||
| setTimeout(() => setToastMessage(null), 3000); | ||
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@frontend/src/App.tsx`:
- Around line 350-353: The success toast currently rendered when toastMessage is
truthy does not announce to assistive tech; update the toast markup in App.tsx
(the JSX that uses toastMessage and renders the divs with className
"toast-container" and "toast success") to make it accessible by adding an ARIA
live region and status role (e.g., aria-live="polite" aria-atomic="true"
role="status") to the container or the inner toast element, and ensure any
visual-only styling still exposes the text to screen readers (or include a
visually-hidden element inside the toast for announcement) so screen readers are
reliably notified of the save-success message.
- Around line 35-38: The showToast implementation causes timer races and missed
re-renders; fix by tracking the timeout in a ref (e.g., toastTimerRef =
useRef<number | null>(null)), clearing any existing timer before scheduling a
new one (window.clearTimeout(toastTimerRef.current)), force a reflow/re-render
by resetting state to null before setting the message using
requestAnimationFrame, schedule the hide with setTimeout and save its id into
toastTimerRef, and add a cleanup useEffect that clears toastTimerRef on unmount;
update uses of setToastMessage, showToast, toastTimerRef and the useEffect
cleanup accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6485dae6-5d4a-4235-afb7-34d4b7b446dc
📒 Files selected for processing (2)
frontend/src/App.cssfrontend/src/App.tsx
- Use useRef to track toast timer and clear previous timers to prevent overlapping or racing toasts. - Reset state to null and use requestAnimationFrame before showing a new toast to force a React re-render and trigger the CSS animation. - Add aria-live="polite", aria-atomic="true", and role="status" to the toast container for screen reader accessibility.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/App.tsx (1)
36-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
requestAnimationFrameの未管理でトースト競合が残っていますLine 42 で
requestAnimationFrameを登録していますが、前回分をキャンセルしていないため、同一フレーム内の連続showToast呼び出しで古いコールバック由来の timeout が残り、トーストが早く消える競合が再発します。RAFもrefで追跡してクリアしてください。修正案
@@ - const toastTimerRef = useRef<number | null>(null); + const toastTimerRef = useRef<number | null>(null); + const toastRafRef = useRef<number | null>(null); @@ const showToast = (message: string) => { if (toastTimerRef.current !== null) { window.clearTimeout(toastTimerRef.current); + toastTimerRef.current = null; + } + if (toastRafRef.current !== null) { + window.cancelAnimationFrame(toastRafRef.current); + toastRafRef.current = null; } // Force a re-render by clearing the state first setToastMessage(null); - requestAnimationFrame(() => { + toastRafRef.current = window.requestAnimationFrame(() => { + toastRafRef.current = null; setToastMessage(message); toastTimerRef.current = window.setTimeout(() => { setToastMessage(null); toastTimerRef.current = null; }, 3000); }); }; @@ useEffect(() => { return () => { if (toastTimerRef.current !== null) { window.clearTimeout(toastTimerRef.current); } + if (toastRafRef.current !== null) { + window.cancelAnimationFrame(toastRafRef.current); + } }; }, []);Also applies to: 51-57
🤖 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 `@frontend/src/App.tsx` around lines 36 - 48, The showToast implementation schedules a requestAnimationFrame without canceling prior RAFs, causing stale callbacks to set timeouts and prematurely clear toasts; add a new ref (e.g., toastRafRef) to store the RAF id, call cancelAnimationFrame on any existing id before assigning a new one in showToast, clear toastRafRef after the callback runs, and also update the component cleanup (useEffect return) to cancel both the pending RAF and the toastTimerRef timeout to avoid leaks.
🤖 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.
Duplicate comments:
In `@frontend/src/App.tsx`:
- Around line 36-48: The showToast implementation schedules a
requestAnimationFrame without canceling prior RAFs, causing stale callbacks to
set timeouts and prematurely clear toasts; add a new ref (e.g., toastRafRef) to
store the RAF id, call cancelAnimationFrame on any existing id before assigning
a new one in showToast, clear toastRafRef after the callback runs, and also
update the component cleanup (useEffect return) to cancel both the pending RAF
and the toastTimerRef timeout to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cfeb5754-f2bb-44b1-ad63-0206d9531284
📒 Files selected for processing (1)
frontend/src/App.tsx
If showToast was called multiple times within the same animation frame, the previous requestAnimationFrame callback was not cancelled. This caused multiple callbacks to fire in the next frame, overwriting the toastTimerRef with a new timeout ID while leaking the older timeout, which would then prematurely clear the toast. This commit adds a toastRafRef to track and cancel any pending RAF callbacks, ensuring only the most recent showToast call schedules a timeout.
Comment on lines
+36
to
+48
|
|
||
| const showToast = (message: string) => { | ||
| if (toastTimerRef.current !== null) { | ||
| window.clearTimeout(toastTimerRef.current); | ||
| toastTimerRef.current = null; | ||
| } | ||
| if (toastRafRef.current !== null) { | ||
| window.cancelAnimationFrame(toastRafRef.current); | ||
| toastRafRef.current = null; | ||
| } | ||
|
|
||
| // Force a re-render by clearing the state first | ||
| setToastMessage(null); |
Comment on lines
+42
to
+47
| if (toastRafRef.current !== null) { | ||
| window.cancelAnimationFrame(toastRafRef.current); | ||
| toastRafRef.current = null; | ||
| } | ||
|
|
||
| // Force a re-render by clearing the state first |
Hardcoding the duration in both CSS (animation) and JS (setTimeout) creates a risk of timing drift and cutoff bugs if only one side is updated. This commit introduces a single TOAST_DURATION_MS constant in JS and passes it to CSS via the inline animationDuration style to ensure they are always perfectly synchronized.
Comment on lines
+39
to
+58
| const showToast = (message: string) => { | ||
| if (toastTimerRef.current !== null) { | ||
| window.clearTimeout(toastTimerRef.current); | ||
| toastTimerRef.current = null; | ||
| } | ||
| if (toastRafRef.current !== null) { | ||
| window.cancelAnimationFrame(toastRafRef.current); | ||
| toastRafRef.current = null; | ||
| } | ||
|
|
||
| // Force a re-render by clearing the state first | ||
| setToastMessage(null); | ||
| toastRafRef.current = requestAnimationFrame(() => { | ||
| setToastMessage(message); | ||
| toastTimerRef.current = window.setTimeout(() => { | ||
| setToastMessage(null); | ||
| toastTimerRef.current = null; | ||
| }, TOAST_DURATION_MS); | ||
| toastRafRef.current = null; | ||
| }); |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
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.
Closes #14