fix: prevent duplicate error notifications on flow publish#3929
fix: prevent duplicate error notifications on flow publish#3929srushti-55 wants to merge 7 commits into
Conversation
When clicking "Publish & go back", publishLoading was never set, leaving both publish buttons active. A second click would fire a second mutation, and both onError callbacks would call setNotification, showing the same error toast twice. Guard handlePublishFlow with publishLoading and centralise setPublishLoading(true) there so no duplicate mutations can fire regardless of which publish button is clicked. Fixes glific#3499
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds an early-return guard in FlowEditor.handlePublishFlow: if publishLoading is true the handler returns immediately, and it now sets publishLoading = true before invoking publishFlow. A new test simulates a publish in progress, verifies the confirmation OK button is disabled during loading, attempts a second publish, and asserts the error/validation dialog appears. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (81.24%) is below the target coverage (82.38%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3929 +/- ##
==========================================
+ Coverage 81.19% 81.24% +0.04%
==========================================
Files 319 319
Lines 13871 13872 +1
Branches 3248 3249 +1
==========================================
+ Hits 11263 11270 +7
+ Misses 1615 1610 -5
+ Partials 993 992 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/floweditor/FlowEditor.test.tsx (1)
186-210: ⚡ Quick winThe test doesn't actually verify that only one mutation is fired.
The stated intent is "should not fire a second publish mutation while one is already in progress," but the final assertion only checks that the error dialog text appears — which is produced by the first mutation's
onErrorcallback and would appear regardless of whether the guard fires or not.If the guard in
handlePublishFlowwere removed, the second click onmiddle-buttonwould dispatch a secondpublishFlowmutation.activeFlowMockscontains only onepublishFlowmock, so the second mutation would receive an Apollo "no more mocked responses" error. ItsonErrorcallback would still callsetNotification, andscreen.getByText(...)would still find the dialog — meaning this test would still pass without the guard.To meaningfully assert the guard's invariant, spy on the mutation directly:
🔧 Proposed fix: spy on the mutation call-count
import * as Apollo from '@apollo/client';test('should not fire a second publish mutation while one is already in progress', async () => { + const useMutationSpy = vi.spyOn(Apollo, 'useMutation'); const { getByTestId } = render(defaultWrapper); await waitFor(() => expect(getByTestId('button')).toBeInTheDocument()); fireEvent.click(getByTestId('button')); await waitFor(() => expect(getByTestId('ok-button')).toBeInTheDocument()); + // Capture the publish mutation's execute function after first render + const publishMutationFn = vi.fn(); + // ... OR alternatively, track via MockedProvider by asserting no + // "No more mocked responses" console.error fires: + const consoleErrorSpy = vi.spyOn(console, 'error'); + // First publish — sets publishLoading=true fireEvent.click(getByTestId('ok-button')); await waitFor(() => expect(getByTestId('ok-button')).toBeDisabled()); // Second attempt via middle button while loading — guard should block it fireEvent.click(getByTestId('middle-button')); await waitFor(() => { expect( screen.getByText('Errors were detected in the flow. Would you like to continue modifying?') ).toBeInTheDocument(); }); + + // Guard invariant: no "No more mocked responses" error means only one mutation fired + expect(consoleErrorSpy).not.toHaveBeenCalledWith( + expect.stringContaining('No more mocked responses') + ); + consoleErrorSpy.mockRestore(); });A simpler alternative is asserting that
console.errorwas not called with Apollo's "No more mocked responses forPublishFlow..." message. SinceactiveFlowMocksprovides exactly onepublishFlowmock, any second mutation dispatch will cause that error — making it a reliable canary for duplicate mutations.🤖 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/floweditor/FlowEditor.test.tsx` around lines 186 - 210, The test currently only checks for the error dialog but not whether a second publish mutation was dispatched; update the test to assert the mutation was called only once by spying on the GraphQL mutation or on console.error: e.g., create a spy for the PublishFlow mutation call (or jest.spyOn(console, 'error')) before triggering clicks, trigger the first publish via ok-button and the second click on middle-button, then assert the PublishFlow mutation spy was called exactly once (or that console.error was not called with Apollo's "No more mocked responses for `PublishFlow`..." message). Reference handlePublishFlow, publishFlow, activeFlowMocks, ok-button and middle-button when locating the code to change.
🤖 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.
Nitpick comments:
In `@src/components/floweditor/FlowEditor.test.tsx`:
- Around line 186-210: The test currently only checks for the error dialog but
not whether a second publish mutation was dispatched; update the test to assert
the mutation was called only once by spying on the GraphQL mutation or on
console.error: e.g., create a spy for the PublishFlow mutation call (or
jest.spyOn(console, 'error')) before triggering clicks, trigger the first
publish via ok-button and the second click on middle-button, then assert the
PublishFlow mutation spy was called exactly once (or that console.error was not
called with Apollo's "No more mocked responses for `PublishFlow`..." message).
Reference handlePublishFlow, publishFlow, activeFlowMocks, ok-button and
middle-button when locating the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62217987-5f27-4757-8f4d-9a0cb27450cc
📒 Files selected for processing (1)
src/components/floweditor/FlowEditor.test.tsx
|
E2E ci test case will fail. you can ignore it |
priyanshu6238
left a comment
priyanshu6238
left a comment
There was a problem hiding this comment.
test case is failing can you fix it?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/floweditor/FlowEditor.test.tsx`:
- Around line 195-218: The test creates a console error spy (consoleErrorSpy)
but calls consoleErrorSpy.mockRestore() at the end unguarded, so if waitFor() or
assertions throw the spy is leaked; wrap the test body (the fireEvent clicks,
waitFor(...) and expect(...) checks) in a try/finally and call
consoleErrorSpy.mockRestore() from the finally block to guarantee cleanup of
consoleErrorSpy regardless of test failures.
🪄 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: 4736a1f6-1aac-48a5-9250-ae9b844c9d8a
📒 Files selected for processing (1)
src/components/floweditor/FlowEditor.test.tsx
|
Hi, could you please approve the CI workflows to run? |
2 participants
When clicking "Publish & go back", publishLoading was never set, leaving both publish buttons active. A second click would fire a second mutation, and both onError callbacks would call setNotification, showing the same error toast twice.
Guard handlePublishFlow with publishLoading and centralise setPublishLoading(true) there so no duplicate mutations can fire regardless of which publish button is clicked.
Fixes #3499
Summary
Test Plan
Summary by CodeRabbit
Bug Fixes
Tests