fix: close manage movie sidebar without back reopening#2590
fix: close manage movie sidebar without back reopening#2590john-fletcher wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMovieDetails and TvDetails now open the manager slide-over only when an effect detects ChangesManager Panel Navigation and History Handling
Sequence Diagram(s)sequenceDiagram
participant Browser
participant MovieDetails
participant Router
Browser->>MovieDetails: navigate to /movie/:id?manage=1
MovieDetails->>MovieDetails: initialize showManager = false
Note over MovieDetails: useEffect detects router.query.manage === '1'
MovieDetails->>MovieDetails: set showManager = true
MovieDetails->>Router: router.replace(query: { movieId })
Router->>Browser: replace history entry (remove manage)
Note over MovieDetails: user closes panel
MovieDetails->>Router: router.replace(pathname, query: { movieId })
Router->>Browser: replace history entry (panel closed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
john-fletcher
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes browser history behavior around the Movie/TV “Manage” slide-over so closing the panel doesn’t add a history entry that causes the panel to reopen when users press the back button.
Changes:
- Use
router.replace(instead ofrouter.push) when closingManageSlideOveron Movie and TV details pages. - Add a
replace?: booleanprop to the sharedBadgecomponent and use it fromStatusBadgeto avoid same-page duplicate history entries when opening?manage=1. - Add a Cypress regression test covering open
?manage=1→ close → back navigation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/TvDetails/index.tsx | Close handler now uses router.replace to prevent “back reopens panel” behavior. |
| src/components/MovieDetails/index.tsx | Same as TV details: replace on close to avoid extra history entries. |
| src/components/StatusBadge/index.tsx | Uses Badge replace when linking to manage on the same details page to prevent duplicate same-page history entries. |
| src/components/Common/Badge/index.tsx | Adds replace prop passthrough to Next.js Link. |
| cypress/e2e/movie-details.cy.ts | Adds regression coverage for closing manager panel and using browser back. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gauthier-th
left a comment
gauthier-th
left a comment
There was a problem hiding this comment.
I find this implementation very messy.
You could have done that in a much easier way, with only a few changes:
- Set
showManageras false by default here:const [showManager, setShowManager] = useState(false);
- Modify the
useEffecthook to show the sidebar here only whenquery.manage === '1', and then remove this query param withrouter.replace.useEffect(() => { if (router.query.manage === '1') { setShowManager(true); } router.replace({ pathname: router.pathname, query: { movieId: router.query.movieId }, }); }, [router.query.manage]);
Same thing in TvDetails, and that's it.
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
|
This PR was closed because it has been stalled for 30 days with no activity. You can reopen it once you address the feedback or provide the requested changes. |
Fix media manage sidebar history handling so closing it does not create extra browser history entries that reopen the panel on Back. Use router.replace when dismissing the movie/TV manage slide-over, add replace support to shared badge links, and have status-badge manage links use replace only when already on the same media details page. Add a Cypress regression test that verifies opening ?manage=1, closing the panel, and pressing Back keeps the panel closed instead of reopening it.
gauthier-th
force-pushed
the
fix/manage-sidebar-history
branch
3 times, most recently
from
d627c86 to
f8604fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/MovieDetails/index.tsx (3)
158-166: 💤 Low valueSimplify useEffect dependencies to avoid redundancy.
The dependency array
[router, router.query.manage]is redundant because therouterobject reference is stable in Next.js. Including only[router.query.manage]is sufficient and more idiomatic. Additionally, this effect will run twice when the manager opens: once whenmanageis'1'(performing the work), and again whenrouter.replaceclears it toundefined(skipping theifblock). Consider adding an early-return guard or using a ref to prevent the second execution if needed.♻️ Suggested dependency simplification
useEffect(() => { if (router.query.manage === '1') { setShowManager(true); router.replace({ pathname: router.pathname, query: { movieId: router.query.movieId }, }); } - }, [router, router.query.manage]); + }, [router.query.manage, router.pathname, router.query.movieId]);🤖 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/MovieDetails/index.tsx` around lines 158 - 166, The useEffect currently lists redundant dependencies and can re-run twice; change its dependency array to depend only on router.query.manage and add an early-return guard (or a ref flag) to avoid reacting to the second run after router.replace. Specifically, update the effect that calls setShowManager and router.replace (the useEffect referencing router, router.query.manage, setShowManager, and router.replace) to use [router.query.manage] as the dependency and short-circuit if manage is not '1' or if the effect has already handled the open action (useRef handledOnce or similar) so router.replace clearing the query doesn't trigger the work again.
286-286: 💤 Low valueFragment key construction assumes
.keyproperty exists on React elements.The pattern
Fragment key={\${prev.key}-${curr.key}`}assumes that the React elements returned frommap()expose a reliable.keyproperty. While this often works, it's fragile—keys may beundefined` or synthetic. Consider using the array index or a stable identifier derived from the data instead.♻️ Alternative: use index-based keys
For line 286:
data.genres .map((g) => ( <Link href={`/discover/movies?genre=${g.id}`} key={`genre-${g.id}`} className="hover:underline" > {g.name} </Link> )) - .reduce((prev, curr) => ( - <Fragment key={`${prev.key}-${curr.key}`}> + .reduce((prev, curr, idx) => ( + <Fragment key={`fragment-${idx}`}> {intl.formatMessage(globalMessages.delimitedlist, { a: prev, b: curr, })} </Fragment> ))Apply a similar change for line 560.
Also applies to: 560-560
🤖 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/MovieDetails/index.tsx` at line 286, The Fragment key is being built from React element `.key` properties which can be undefined or synthetic (see Fragment key={`${prev.key}-${curr.key}`}`) — change both occurrences (the Fragment wrapping pairs of `prev` and `curr`) to use a stable identifier instead: derive a key from the underlying data (e.g., an ID field on the items you mapped) or, if no stable id exists, use the map index (e.g., the index of the pair) to construct the key; update the key expression on the Fragment instances and any other places using `prev.key`/`curr.key` so they use the chosen stable id or index-based key.
351-356: 💤 Low valueConsider logging caught errors for debugging.
The empty
catchblocks suppress error details. While the generic toast is appropriate for users, logging the error to the console would aid debugging without affecting UX.♻️ Suggested logging
- } catch { + } catch (error) { + console.error('Watchlist operation failed:', error); addToast(intl.formatMessage(messages.watchlistError), { appearance: 'error', autoDismiss: true, }); }Apply the same pattern to both catch blocks (lines 351 and 378).
Also applies to: 378-383
🤖 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/MovieDetails/index.tsx` around lines 351 - 356, The catch blocks that call addToast(intl.formatMessage(messages.watchlistError), ...) are swallowing errors; change each to catch (err) { console.error('Error toggling watchlist:', err); addToast(intl.formatMessage(messages.watchlistError), { appearance: 'error', autoDismiss: true }); } — apply this pattern to both catch blocks surrounding the addToast calls in the MovieDetails component so errors are logged via console.error while the user-facing toast remains unchanged.
🤖 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/MovieDetails/index.tsx`:
- Around line 158-166: The useEffect currently lists redundant dependencies and
can re-run twice; change its dependency array to depend only on
router.query.manage and add an early-return guard (or a ref flag) to avoid
reacting to the second run after router.replace. Specifically, update the effect
that calls setShowManager and router.replace (the useEffect referencing router,
router.query.manage, setShowManager, and router.replace) to use
[router.query.manage] as the dependency and short-circuit if manage is not '1'
or if the effect has already handled the open action (useRef handledOnce or
similar) so router.replace clearing the query doesn't trigger the work again.
- Line 286: The Fragment key is being built from React element `.key` properties
which can be undefined or synthetic (see Fragment
key={`${prev.key}-${curr.key}`}`) — change both occurrences (the Fragment
wrapping pairs of `prev` and `curr`) to use a stable identifier instead: derive
a key from the underlying data (e.g., an ID field on the items you mapped) or,
if no stable id exists, use the map index (e.g., the index of the pair) to
construct the key; update the key expression on the Fragment instances and any
other places using `prev.key`/`curr.key` so they use the chosen stable id or
index-based key.
- Around line 351-356: The catch blocks that call
addToast(intl.formatMessage(messages.watchlistError), ...) are swallowing
errors; change each to catch (err) { console.error('Error toggling watchlist:',
err); addToast(intl.formatMessage(messages.watchlistError), { appearance:
'error', autoDismiss: true }); } — apply this pattern to both catch blocks
surrounding the addToast calls in the MovieDetails component so errors are
logged via console.error while the user-facing toast remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3d208e0-bbe0-44ec-a061-d5b39094aa3c
📒 Files selected for processing (4)
cypress/e2e/movie-details.cy.tsnext-env.d.tssrc/components/Common/Badge/index.tsxsrc/components/MovieDetails/index.tsx
✅ Files skipped from review due to trivial changes (1)
- next-env.d.ts
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/TvDetails/index.tsx`:
- Around line 159-161: The router.replace call is overwriting the TV route by
setting query.movieId instead of preserving the current tvId; update the
router.replace invocation to clear only the manage param while keeping the
existing tvId (from router.query.tvId) and any other relevant query params so
the path remains /tv/[tvId]; locate the router.replace usage in this component
(the effect that clears `manage`) and build the new query object by copying
existing router.query and removing `manage` (or explicitly setting query.tvId =
router.query.tvId) before calling router.replace.
🪄 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: CHILL
Plan: Pro
Run ID: 49a11f16-d99d-44e0-9565-12ca1652718e
📒 Files selected for processing (2)
src/components/MovieDetails/index.tsxsrc/components/TvDetails/index.tsx
gauthier-th
force-pushed
the
fix/manage-sidebar-history
branch
from
f8604fd to
cd45cca
Compare
Fix media manage sidebar history handling so closing it does not create extra browser history entries that reopen the panel when clicking the back button. Use router.replace when dismissing the movie/TV manage slide panel, add replace support to shared badge links, and have status badge manage links use replace only when already on the same media details page. Added a Cypress regression test that verifies opening ?manage=1, closing the panel, and pressing the back button keeps the panel closed instead of reopening it.
AI Disclaimer:
I used GPT 5.3-Codex in order to gain context of the project. I had it come up with a plan to fix the bug I have encountered. I then verified, implemented, and tested
Description
Updated the
ManageSlideOvercomponent in Movie/TVDetails to userouter.replaceso it does not add the?manage=1query parameter to history. This is better as opening the slide over is more of a component state change and not a navigation change.Added a new
replaceprop to the Badge component in order to prevent duplicate same page history entries when toggling manage from a status badge on the same page. This can be done from Deleted or Requested tag. This was needed in order to make the back button act intuitively where pressing it once actually navigated to the previous page.Added a new cypress test to ensure proper states.
How Has This Been Tested?
Tested manually by going to a specific movie (ex: http://localhost:5055/movie/1387382). Requested the movie and clicked the
requestedbadge to open the slide over. Closed it and pressed back to go back to previous page without re-opening the slide over.Added Cypress Test.
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Bug Fixes
Tests