feat(snackbar): add immediate/queued strategy for multi-snackbar behavior#1413
feat(snackbar): add immediate/queued strategy for multi-snackbar behavior#1413
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSnackbar 생성 시 기존 표시 항목 처리 전략이 도입되어, 프로바이더 기본값과 개별 생성 옵션으로 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant SnackbarProvider
participant useSnackbarState
participant SnackbarRenderer
User->>Component: 클릭 → adapter.create(options)
Component->>SnackbarProvider: create(options)
SnackbarProvider->>useSnackbarState: push(option { strategy? })
alt option.strategy == "immediate"
useSnackbarState->>SnackbarRenderer: 현재 교체하거나 즉시 활성화
else option.strategy == "queued"
useSnackbarState->>useSnackbarState: 큐에 추가 (대기)
note right of useSnackbarState: 비활성 시 큐에서 pop → activate
useSnackbarState->>SnackbarRenderer: 순차적 활성화
end
SnackbarRenderer->>User: 표시 → timeout → dismiss() → removeDelay → 제거
예상 코드 리뷰 난이도🎯 4 (Complex) | ⏱️ ~45 minutes 시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Alpha Preview (Stackflow SPA)
|
Alpha Preview (Storybook)
|
Alpha Preview (Docs)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-headless/snackbar/src/useSnackbar.ts (1)
117-136: "immediate" 모드에서 대기열에 있던 스낵바의onClose콜백이 호출되지 않을 수 있습니다.
state가 "inactive"가 아닐 때setQueue([option])으로 기존 대기열을 덮어쓰면, 이전에 대기 중이던 스낵바들의onClose콜백이 호출되지 않고 폐기됩니다. 소비자가onClose에 정리 로직을 의존하는 경우 예상치 못한 동작이 발생할 수 있습니다.이 동작이 의도된 것이라면, 문서에 "immediate 모드에서 대기 중인 스낵바는
onClose호출 없이 폐기된다"는 점을 명시하는 것이 좋겠습니다.🔧 대기열 덮어쓰기 전 onClose 호출을 고려한 예시
} else if (state === "dismissing") { setQueue([option]); } else { + // 덮어쓰기 전 대기 중인 스낵바의 onClose 호출 (선택적) + setQueue((prev) => { + prev.forEach((item) => item.onClose?.()); + return [option]; + }); - setQueue([option]); setState("dismissing"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-headless/snackbar/src/useSnackbar.ts` around lines 117 - 136, In push (useSnackbar) when effectiveStrategy === "immediate" the code overwrites the existing queue via setQueue([option]) which discards queued snackbars without calling their onClose; before replacing the queue (both in the state === "dismissing" and the else branch) iterate the current queued items (the existing queue variable) and call each item's onClose callback if present, then replace the queue with [option]; this ensures queued snackbars' cleanup runs when they are discarded by push, while leaving the rest of the logic (setCurrentSnackbar, setState, etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-headless/snackbar/src/useSnackbar.ts`:
- Around line 130-136: When queuing in "queued" mode multiple synchronous calls
can all see the stale closure value of `state` and each call will run `pop()`;
fix by using a functional state updater so the transition from
"inactive"→"active" is performed atomically and `pop()` only runs once: replace
the direct `if (state === "inactive") { pop(); setState("active"); }` with a
single `setState(prev => { if (prev === "inactive") { pop(); return "active"; }
return prev; })` (or use a dedicated ref/flag guarded in `useSnackbar`),
ensuring you call `push(option)` before invoking the updater and keep references
to `push`, `pop`, and `setState` intact.
---
Nitpick comments:
In `@packages/react-headless/snackbar/src/useSnackbar.ts`:
- Around line 117-136: In push (useSnackbar) when effectiveStrategy ===
"immediate" the code overwrites the existing queue via setQueue([option]) which
discards queued snackbars without calling their onClose; before replacing the
queue (both in the state === "dismissing" and the else branch) iterate the
current queued items (the existing queue variable) and call each item's onClose
callback if present, then replace the queue with [option]; this ensures queued
snackbars' cleanup runs when they are discarded by push, while leaving the rest
of the logic (setCurrentSnackbar, setState, etc.) unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 708af3e0-c669-4702-8187-63ef837a4408
📒 Files selected for processing (2)
packages/react-headless/snackbar/src/useSnackbar.test.tsxpackages/react-headless/snackbar/src/useSnackbar.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-headless/snackbar/src/useSnackbar.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/stackflow-spa/src/activities/ActivityHome.tsx (1)
231-241: "workaround" 패턴의 사용 목적에 대한 주석 추가를 권장합니다.이 예제는
dismiss()후setTimeout으로 새 스낵바를 생성하는 패턴을 보여주고 있습니다. 예제 코드로서 이 패턴이 언제, 왜 필요한지 설명하는 주석이 있으면 사용자 이해에 도움이 될 것 같습니다.또한
setTimeout의 delay 인자를 명시적으로0으로 지정하면 의도가 더 명확해집니다.♻️ 제안
{ + // immediate strategy가 기본값일 때, 기존 스낵바를 즉시 닫고 + // 새 스낵바를 표시하려는 경우의 workaround 패턴 title: "Snackbar (dismiss+setTimeout workaround)", onClick: () => { snackbarAdapter.dismiss(); - setTimeout(() => { + setTimeout(() => { snackbarAdapter.create({ render: () => <Snackbar message="Workaround Snackbar" />, }); - }); + }, 0); }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/stackflow-spa/src/activities/ActivityHome.tsx` around lines 231 - 241, Add an inline comment above the button handler that explains why the "workaround" pattern is used: that calling snackbarAdapter.dismiss() immediately followed by creating a new snackbar needs to defer creation to the next tick to avoid race/DOM/state conflicts, and make the setTimeout explicit by passing 0 as the delay; reference the handler's use of snackbarAdapter.dismiss(), setTimeout(..., 0), snackbarAdapter.create({...}), and the Snackbar component so reviewers can see the intent clearly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/stackflow-spa/src/activities/ActivityHome.tsx`:
- Around line 231-241: Add an inline comment above the button handler that
explains why the "workaround" pattern is used: that calling
snackbarAdapter.dismiss() immediately followed by creating a new snackbar needs
to defer creation to the next tick to avoid race/DOM/state conflicts, and make
the setTimeout explicit by passing 0 as the delay; reference the handler's use
of snackbarAdapter.dismiss(), setTimeout(..., 0), snackbarAdapter.create({...}),
and the Snackbar component so reviewers can see the intent clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4df7638d-3b61-4187-a372-00f6d8d1ea66
📒 Files selected for processing (2)
docs/examples/react/snackbar/strategy.tsxexamples/stackflow-spa/src/activities/ActivityHome.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/examples/react/snackbar/strategy.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-headless/snackbar/src/useSnackbar.ts (1)
160-164:⚠️ Potential issue | 🟠 Major
dismiss()호출 시onClose콜백이 두 번 실행됩니다.
dismiss()함수에서invokeOnClose()를 직접 호출하고 (line 163), "dismissing" 상태의 effect에서도invokeOnClose()를 호출합니다 (line 107). 사용자가 닫기 버튼을 클릭하면onClose콜백이 두 번 실행됩니다.
dismiss()에서는 상태 전환만 수행하고,onClose호출은 effect에서 일관되게 처리하는 것이 좋습니다.🐛 중복 호출 제거를 위한 수정 제안
dismiss: () => { if (state === "active" || state === "persist") { setState("dismissing"); - invokeOnClose(); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-headless/snackbar/src/useSnackbar.ts` around lines 160 - 164, The dismiss() implementation is directly calling invokeOnClose() and the effect that watches state also calls invokeOnClose(), causing duplicate onClose invocations; update dismiss() (in useSnackbar.ts) to only change state via setState("dismissing") when state is "active" or "persist" and remove the direct invokeOnClose() call, relying on the existing effect that observes state === "dismissing" to call invokeOnClose(); keep references to state, setState, dismiss, and invokeOnClose so reviewers can locate the change.
🧹 Nitpick comments (1)
packages/react-headless/snackbar/src/useSnackbar.ts (1)
167-167: 의존성 배열에서state제거 검토
events.push는 functional updater로state를 직접 참조하지 않지만,pause,resume,dismiss는 클로저에서state를 읽습니다.state가 변경될 때마다 새로운events객체가 생성되어 하위 컴포넌트의 불필요한 리렌더링을 유발할 수 있습니다.
pause,resume,dismiss도 functional updater 패턴으로 변경하면state의존성을 제거할 수 있습니다.♻️ functional updater 패턴 적용 예시
pause: () => { - if (state === "active") { - if (pauseOnInteraction) { - setState("persist"); - } - } + setState((prev) => { + if (prev === "active" && pauseOnInteraction) { + return "persist"; + } + return prev; + }); }, resume: () => { - if (state === "persist") { - setState("active"); - } + setState((prev) => (prev === "persist" ? "active" : prev)); }, dismiss: () => { - if (state === "active" || state === "persist") { - setState("dismissing"); - } + setState((prev) => { + if (prev === "active" || prev === "persist") { + return "dismissing"; + } + return prev; + }); },이 변경 후 의존성 배열에서
state를 제거할 수 있습니다.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-headless/snackbar/src/useSnackbar.ts`:
- Around line 122-134: The updater passed to setState in useSnackbar (the
anonymous updater in lines around setState) must not perform side effects like
calling setCurrentSnackbar, setQueue or pop(); refactor the related state
(state, queue, currentSnackbar) into a single reducer or a single state object
and replace the multiple setters with a useReducer-based state machine (or an
atomic state setter) so transitions like "inactive" -> "active", enqueueing, and
dismissing are handled inside reducer logic (move the logic currently in the
setState updater and in pop() into reducer action handlers) and ensure
setCurrentSnackbar, setQueue and pop() are only called from effectful places if
needed after reducer state updates.
---
Outside diff comments:
In `@packages/react-headless/snackbar/src/useSnackbar.ts`:
- Around line 160-164: The dismiss() implementation is directly calling
invokeOnClose() and the effect that watches state also calls invokeOnClose(),
causing duplicate onClose invocations; update dismiss() (in useSnackbar.ts) to
only change state via setState("dismissing") when state is "active" or "persist"
and remove the direct invokeOnClose() call, relying on the existing effect that
observes state === "dismissing" to call invokeOnClose(); keep references to
state, setState, dismiss, and invokeOnClose so reviewers can locate the change.
🪄 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: CHILL
Plan: Pro
Run ID: 5dfa20f8-d755-401f-86e8-e3716ff6fcd8
📒 Files selected for processing (1)
packages/react-headless/snackbar/src/useSnackbar.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…vior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erve animations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ants Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ansitions Fixes two real bugs that arose from mixing side-effects into setState updaters and splitting onClose invocation across event handler and effect: - StrictMode double-invocation of setState updaters caused pop() (which itself mutated queue via setQueue) to run twice, corrupting the queue on rapid synchronous pushes. - dismiss() invoked onClose directly while the dismissing effect also invoked it after removeDelay, resulting in duplicate calls. Consolidates state, queue, and currentSnackbar into a single reducer so all transitions are pure. Timers and onClose now live only in the effect. events memo drops its state dependency as a side benefit.
Adds a brief comment explaining the pattern and makes the setTimeout delay explicit (0) so readers don't have to infer intent from an omitted argument.
Pins the intended contract of onClose: it fires only for snackbars that were actually presented (became currentSnackbar). Queued items cleared by an immediate-strategy push never entered the display lifecycle, so their onClose must not be invoked.
a332ae9 to
4c5dde6
Compare
| style: { | ||
| position: "relative", | ||
| pointerEvents: "auto", | ||
| "--snackbar-remove-delay": `${currentSnackbar?.removeDelay ?? 0}ms`, |
junghyeonsu
left a comment
There was a problem hiding this comment.
changeset만 작성해주시면 될듯 굿
| removeCurrentSnackbar(); | ||
| }, removeDelay); | ||
| return () => clearTimeout(timeout); | ||
| const visibleDuration = currentSnackbar?.timeout ?? 5000; |
Summary by CodeRabbit
새로운 기능
문서
테스트
데모