fix: editing and saving share dialog triggers hover state#8708
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:
WalkthroughThreads an optional Changes
Sequence Diagram(s)sequenceDiagram
participant JC as JourneyCard
participant JCM as JourneyCardMenu
participant DM as DefaultMenu
participant SI as ShareItem
participant UI as OverlayHover
JC->>JCM: render (pass setHasOpenDialog)
JCM->>DM: render (propagate setHasOpenDialog)
DM->>SI: render (propagate setHasOpenDialog)
SI->>SI: user clicks Share -> open dialog
SI->>DM: setHasOpenDialog?.(true)
DM->>JCM: setHasOpenDialog?.(true)
JCM->>JC: setHasOpenDialog?.(true)
JC->>JC: hasOpenDialog = true
UI->>JC: mouse enter
JC-->>UI: ignore overlay change (hasOpenDialog prevents hover)
SI->>SI: user closes share dialog
SI->>DM: setHasOpenDialog?.(false)
DM->>JCM: setHasOpenDialog?.(false)
JCM->>JC: setHasOpenDialog?.(false)
JC->>JC: hasOpenDialog = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
jaycnz
added
type: fix
|
View your CI Pipeline Execution ↗ for commit f29ac12
☁️ Nx Cloud last updated this comment at |
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx`:
- Around line 104-106: The effect that sets setIsDialogOpen currently only
considers anchorEl, causing parent to be told dialogs are closed when
sub-dialogs (showSlugDialog, showEmbedDialog, showQrCodeDialog) remain open;
update the useEffect that calls setIsDialogOpen to compute open state as
Boolean(anchorEl) || showSlugDialog || showEmbedDialog || showQrCodeDialog and
include all those symbols in the dependency array so the parent correctly tracks
any visible dialog.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.spec.tsx (2)
255-257:waitForwrappingfireEventis a misuse of the API.
waitForis designed for polling assertions until they pass, not for firing events. SincefireEvent.click(line 253) synchronously flushes React state in RTL with React 18'sactwrapper,setIsDialogOpen(true)is already applied by the time you reach line 255. Just callfireEvent.mouseLeavedirectly.♻️ Suggested fix
fireEvent.click(screen.getByTestId('open-dialog')) - await waitFor(() => { - fireEvent.mouseLeave(card) - }) + fireEvent.mouseLeave(card) fireEvent.mouseEnter(card) expect(overlay).toHaveStyle({ opacity: '0' })With this change the test no longer needs to be
async(unless you add async assertions elsewhere).
231-262: Consider adding coverage for the dialog-close path.The test verifies that hover is suppressed while the dialog is open, but doesn't verify that hover behavior resumes after
setIsDialogOpen(false). Without this, a bug that permanently locks hover suppression would go undetected. Adding a second mock button (or toggling the existing one) to callsetIsDialogOpen(false)and asserting thatmouseEnterrestoresopacity: 1would complete the cycle.
|
The latest updates on your projects.
|
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/Team/CopyToTeamMenuItem/CopyToTeamMenuItem.tsx (1)
119-119:⚠️ Potential issue | 🔴 Critical
setIsDialogOpen?.(false)is missing on all non-cancel close paths — hover state will stay stuck.The
onClosehandler (Cancel button) correctly callssetIsDialogOpen?.(false), but every other path that closes the dialog (successful copy, successful translation, translation error, duplication error) only callssetDuplicateTeamDialogOpen(false)without notifying the parent. This means the card's hover overlay will remain visible after a successful copy or any error — the exact bug this PR aims to fix.Add
setIsDialogOpen?.(false)before eachsetDuplicateTeamDialogOpen(false):Proposed fix
@@ line 118-119 handleCloseMenu() // Close menu when translation completes + setIsDialogOpen?.(false) setDuplicateTeamDialogOpen(false) // Close dialog when translation completes @@ line 129-130 handleCloseMenu() // Close menu on translation error + setIsDialogOpen?.(false) setDuplicateTeamDialogOpen(false) // Close dialog on translation error @@ line 165-166 handleCloseMenu() + setIsDialogOpen?.(false) setDuplicateTeamDialogOpen(false) @@ line 196-197 handleCloseMenu() + setIsDialogOpen?.(false) setDuplicateTeamDialogOpen(false)Also applies to: 130-130, 166-166, 197-197
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.spec.tsx`:
- Around line 440-474: The test opens the ShareItem dialog but doesn't set up
mockedUseRouter like the sibling tests, so router.push and router.events.on may
be undefined; before rendering in the 'should call setIsDialogOpen when opening
and closing dialog' test, call mockedUseRouter.mockReturnValue(...) to return an
object containing query: { tab: 'active' }, a push jest.fn(), and events: { on:
jest.fn() } so the ShareItem component's router usage (router.push,
router.events.on) is present during dialog open/close; update the test setup
near the existing setIsDialogOpen mock and before render to mirror other tests
that configure mockedUseRouter.
In
`@apps/journeys-admin/src/components/Team/CopyToTeamMenuItem/CopyToTeamMenuItem.spec.tsx`:
- Around line 889-922: The test is clicking the menu before the teams query
resolves and misses verifying success/error close behavior; update the test to
await getLastActiveTeamIdAndTeamsMock.result (use waitFor or await that promise)
before firing the menu click, and add two new assertions/tests that simulate a
successful duplication and a failing duplication to assert
setIsDialogOpen(false) is called in both cases. Also update the
CopyToTeamMenuItem component's duplication mutation handlers (the
mutation/method that performs the duplicate action) to call
setIsDialogOpen?.(false) in both the onCompleted and onError paths so the dialog
closes on success or failure. Ensure references:
getLastActiveTeamIdAndTeamsMock, setIsDialogOpen, and CopyToTeamMenuItem (and
the duplicate mutation handler) are updated accordingly.
🧹 Nitpick comments (3)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/JourneyCardMenu.tsx (2)
43-50: Pre-existing: Webpack chunk name mismatch.Line 46's chunk name comment says
"RestoreJourneyDialog"but the import resolvesTrashJourneyDialog. This won't cause a runtime bug but may confuse bundle analysis.const TrashJourneyDialog = dynamic( async () => await import( - /* webpackChunkName: "RestoreJourneyDialog" */ + /* webpackChunkName: "TrashJourneyDialog" */ './TrashJourneyDialog' ).then((mod) => mod.TrashJourneyDialog), { ssr: false } )
83-99: JSDoc is missing the newsetIsDialogOpenprop.The docblock documents all other props but omits the newly added
setIsDialogOpen. Consider adding a line for completeness.apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/JourneyCardMenu.spec.tsx (1)
315-457: Refactor query method for consistency with existing patterns.The new tests properly verify both the
truecall on open andfalsecall on close for Edit Details, Access, Translate, and Trash dialogs. Two observations:
Inconsistent query usage: Lines 380 and 452 use
screen.getByTestId(...)while the existing access/trash dialog tests at lines 167–227 destructuregetByTestIdfromrender(). For consistency, addgetByTestIdto the destructured queries in the new test cases.Share dialog coverage: The Share dialog is tested separately in
ShareItem.spec.tsx(line 440), which includes setIsDialogOpen verification. This is acceptable and provides isolated coverage of the Share flow.
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
|
The latest updates on your projects.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx`:
- Around line 92-96: The hover state isn't reset when a dialog closes because
updateHoverState blocks changes while hasOpenDialog is true; add logic to reset
isCardHovered when hasOpenDialog transitions to false — e.g., add a useEffect
that watches hasOpenDialog and calls setIsCardHovered(false) when hasOpenDialog
becomes false so the overlay clears immediately; locate updateHoverState,
isCardHovered, setIsCardHovered and hasOpenDialog in JourneyCard and implement
the effect to clear hover on dialog close.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/JourneyCardMenu.spec.tsx`:
- Around line 315-457: Add a new test in JourneyCardMenu.spec.tsx that mirrors
the other dialog tests to assert Share dialog signals via setHasOpenDialog:
render JourneyCardMenu with a jest.fn() setHasOpenDialog, open the menu
(fireEvent.click(getByRole('button'))), find and click the 'Share' menu item
(await findByRole('menuitem', { name: 'Share' }) or the label used by
ShareItem), await expect(setHasOpenDialog).toHaveBeenCalledWith(true) and then
close the dialog (use screen.getByTestId('dialog-close-button') or the dialog's
Cancel button) and await expect(setHasOpenDialog).toHaveBeenCalledWith(false)
and total call count of 2; reference JourneyCardMenu, ShareItem, and
setHasOpenDialog to locate where to add the test.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/Team/CopyToTeamMenuItem/CopyToTeamMenuItem.spec.tsx (1)
892-910: Inconsistent provider nesting order.Every other test in this file wraps providers as
MockedProvider > SnackbarProvider > TeamProvider, but this test usesSnackbarProvider > MockedProvider > TeamProvider. Keep the order consistent to avoid subtle behavioral differences.Proposed fix
const { getByRole } = render( - <SnackbarProvider> - <MockedProvider - mocks={[ - updateLastActiveTeamIdMock, - mockLanguage, - getLastActiveTeamIdAndTeamsMock - ]} - > + <MockedProvider + mocks={[ + updateLastActiveTeamIdMock, + mockLanguage, + getLastActiveTeamIdAndTeamsMock + ]} + > + <SnackbarProvider> <TeamProvider> <CopyToTeamMenuItem id="journeyId" handleCloseMenu={handleCloseMenu} setHasOpenDialog={setHasOpenDialog} /> </TeamProvider> - </MockedProvider> - </SnackbarProvider> + </SnackbarProvider> + </MockedProvider> )apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/JourneyCardMenu.spec.tsx (1)
380-380: Inconsistent query source:screenvs destructured render result.These two lines use
screen.getByTestId(...)while the rest of each test uses destructured queries (getByRole,findByRole) fromrender(). Pick one approach per test for consistency.Suggested fix
- const closeButton = screen.getByTestId('dialog-close-button') + const closeButton = getByTestId('dialog-close-button')You'd need to add
getByTestIdto the destructured render result for the affected tests (lines 355 and 427).Also applies to: 452-452
github-actions
Bot
requested a deployment
to
Preview - journeys-admin
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
…ton-triggers-hover-state
github-actions
Bot
temporarily deployed
to
Preview - journeys-admin
jaycnz
deleted the
jaychen/nes-209-editing-and-saving-share-button-triggers-hover-state
branch
2 participants
Summary by CodeRabbit