enhance(carousel): Close carousel on browser back navigation #2262

brymut · 2025-07-05T13:00:58Z

Description

~~Add to browser history stack when opening carousel~~ use URL hashes to track modal state, so that navigating back on browser should close the carousel modal, not leave the post entirely.

closes #1508

Screenshots

Screen.Recording.2025-08-18.at.17.36.26.mov

(Updated screen recording)

Additional Context

Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?

Checklist

Are your changes backward compatible? Please answer below:
Y
For example, a change is not backward compatible if you removed a GraphQL field or dropped a database column.

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
10

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Y

Did you introduce any new environment variables? If so, call them out explicitly here:
N

Copilot

Pull Request Overview

This PR enhances the carousel modal so that opening it pushes a history state and pressing the browser back button closes the carousel instead of navigating away.

Integrates Next.js router in CarouselProvider to push a #carousel hash when opening and call router.back() on close
Updates Carousel to re-run the overflow option via setOptions whenever the index or source changes
Adds useRouter import and adjusts effect/callback dependencies for proper behavior

Comments suppressed due to low confidence (3)

components/carousel.js:121

[nitpick] The variable url is ambiguous—consider renaming it to basePath or pathWithoutHash to clarify that it strips the hash fragment.

    const url = router.asPath.split('#')[0]

components/carousel.js:120

[nitpick] The new history‐push and back‐navigation behavior isn’t covered by existing tests. Add a test to simulate opening the carousel and pressing the back button to ensure it closes correctly.

  const showCarousel = useCallback(({ src }) => {

components/carousel.js:123

Pushing history state alone won't close the carousel when the user hits the back button. You should listen for popstate or Next.js router events (e.g., router.events.on('routeChangeStart')) and call the modal's close() callback when the URL no longer contains #carousel.

      router.push(url, url + '#carousel', { shallow: true })

brymut · 2025-07-10T06:41:02Z

Happy to have a look again based on the cursor feedback, but I can confirm that the modal does close automatically upon navigating backwards, as shown in the PR description screen recording.

ekzyis

Closing the carousel now always scrolls us to the top:

2025-07-24.21-37-19.mp4

brymut · 2025-07-24T19:44:54Z

Closing the carousel now always scrolls us to the top:

Let me have a look at that

brymut · 2025-08-18T14:38:51Z

Closing the carousel now always scrolls us to the top:

Should work now, switched to checking the URL hash instead, less complicated and seems to not have the scrolling issue.

Screen.Recording.2025-08-18.at.17.36.26.mov

brymut · 2025-08-22T03:54:51Z

I don't know I had accidentally deleted this one 😅

Soxasora

It works really well! This will be a great addition to the mobile experience.

Something I don't like is the fact that on every click it generates browser history, so if I open the carousel 5 times, I would need to go back 5 times without any kind of feedback (the carousel stays closed).

My personal view is that this doesn't block your PR, it's something that I see everywhere something similar is used.

I left some nitpicks that I'd like for you to address before approving, and some comments. I didn't find something else that could block your PR, great job!

Soxasora · 2025-08-22T14:23:45Z

components/carousel.js

+  useEffect(() => {
+    if (typeof window === 'undefined') return
+
+    if (window.location.hash === '#carousel') {
+      const confirmedEntries = Array.from(media.current.entries())
+        .filter(([, entry]) => entry.confirmed)
+
+      if (confirmedEntries.length > 0) {
+        showCarousel({ src: confirmedEntries[0][0] })
+      }
+    }
+  }, [showCarousel])


This seems redundant. It’s very rare that it would find any confirmed entries since this runs before any media has been fully loaded; confirmMedia already does what this should do!

I thought so too, but cursor bot thought otherwise, seems to be quite strict on it

What did Mr. Cursor say? It has the tendency of being unaware of context sometimes.

Let me get rid of this and see if it re-comments, keeping cursor at bay with the idea I had initially has been quite confusing.

Soxasora · 2025-08-22T14:27:56Z

components/carousel.js

+        const confirmedEntries = Array.from(media.current.entries())
+          .filter(([, entry]) => entry.confirmed)
+        if (confirmedEntries.length >= 1) {
+          showCarousel({ src })
+        }


nitpick: this is duplicate code, I wonder if this check can be done directly in showCarousel or with an helper function

let me have another look

Soxasora · 2025-08-22T14:53:09Z

components/modal.js

+    const currentHash = window.location.hash
+    if (currentHash) {
+      const hasModalWithHash = modalStack.current.some(
+        modal => modal.options?.hash && `#${modal.options.hash}` === currentHash
+      )
+      if (hasModalWithHash) {
+        window.history.replaceState(window.history.state, '', window.location.pathname + window.location.search)
+      }
+    }
+
    while (modalStack.current.length) {
      getCurrentContent()?.options?.onClose?.()
      modalStack.current.pop()
    }


I see that this preserves any other hashes (like table of contents navigation) from being wrongly removed, so it might justify O(n) checks (modalStack.current.some).

I don't see this being that big of an impact performance-wise, and to avoid O(n) checks you would need to store the original hash in some way (like a ref), so it's ok.

Soxasora · 2025-08-22T14:58:32Z

components/carousel.js

@@ -114,20 +114,39 @@ function CarouselOverflow ({ originalSrc, rel }) {
 export function CarouselProvider ({ children }) {
  const media = useRef(new Map())
  const showModal = useShowModal()
+  const [isCarouselOpen, setIsCarouselOpen] = useState(false)


Reasonable solution to avoid duplicate modals for the way they work as of now. Maybe in the future we'd need to add deduplication to the modal system.

*edit: this conflicts with confirmMedia automatic opening of carousel, better to use a ref for this.

Soxasora

Thanks for fixing the duplicate carousel, it works correctly and overall you did a good job.

I didn't request changes before because I wanted to discover more about the bug with you (that I discovered was present regardless of the useEffect you were using), so I'm going to do that now. Also left a question, I'll approve after!

Soxasora · 2025-08-23T12:03:09Z

components/carousel.js

@@ -137,8 +151,18 @@ export function CarouselProvider ({ children }) {
    if (mediaItem) {
      mediaItem.confirmed = true
      media.current.set(src, mediaItem)
+
+      if (typeof window !== 'undefined' && window.location.hash === '#carousel' && !isCarouselOpenRef.current) {
+        if (!isOpeningCarouselRef.current) {


Is this secondary ref needed?

ahhh, right. All the cursor bot changes had me running in loops. Forgot that's not needed anymore. Removed. Sorry about that.

Soxasora

Approved!

Now a tip for your next contributions: don't amend your commit and force push! At least after making it ready for review.

It makes the review process slower and harder than it could be, because I can't compare your changes easily and I have to do git sorcery or a full review.

brymut · 2025-08-23T13:35:36Z

Got it, didn't want to pollute with many small commit fixes.

huumn · 2025-08-25T17:55:27Z

I haven't looked very closely at this, but I wonder if this was made more complicated by choosing to use a hash rather than a query parameter which would allow nextjs to manage the history for us. Again, I don't know, but window.location has a smell to it.

brymut · 2025-08-27T10:05:29Z

I haven't looked very closely at this, but I wonder if this was made more complicated by choosing to use a hash rather than a query parameter which would allow nextjs to manage the history for us. Again, I don't know, but window.location
has a smell to it.

I just think that for a particularly client-side issue and I think that's what hash was intended for, to handle UI state. Also looking at the URL, /?carousel=true compared to /#carousel might make it seem like there's server action happening when there isn't. Happy to have another go with using params with nextjs router if that's what you're looking for particularly.

huumn · 2025-08-29T17:45:32Z

Happy to have another go with using params with nextjs router if that's what you're looking for particularly.

It's mostly a:

lines-of-code thing for me, I do not suspect it requires this much code to do this, and
a going behind nextjs router's back using window.location

If neither is related to the hash vs query param - great. If this is the best way to do it - that's fine too, but I will not be able to merge it until I've tried alternative approaches myself.

brymut · 2025-09-02T09:47:37Z

lines-of-code thing for me, I do not suspect it requires this much code to do this, and

You were right, I overshot this, updated to use nextjs router instead. Mostly based off of the solution you were thinking about here

huumn

if I close (either by going back or not), then go fwd, we have an imageId in the url that doesn't pop the modal.

I suspect the way to make this work as expected is to push on image click and showModal as a side effect of seeing imageId in router.query.

I tried calling router.back() instead of router.replace in onClose, but that didn't seem to work as expected.

brymut · 2025-09-03T05:51:36Z

I tried calling router.back() instead of router.replace in onClose, but that didn't seem to work as expected.

I think adding router.replace is better for not polluting the history stack, so just .push on opening and .replace on close and then track router.query.imageId

Screen.Recording.2025-09-03.at.08.47.06.mov

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch 3 times, most recently from 7607088 to b76ced7 Compare July 8, 2025 15:06

brymut marked this pull request as ready for review July 8, 2025 15:06

brymut requested review from ekzyis, Copilot and huumn and removed request for Copilot July 8, 2025 15:06

Copilot AI reviewed Jul 8, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

ekzyis mentioned this pull request Jul 24, 2025

Fix carousel dropdown #2326

Merged

This comment was marked as outdated.

Sign in to view

ekzyis requested changes Jul 24, 2025

View reviewed changes

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from c85d3b1 to df72819 Compare July 25, 2025 23:12

huumn marked this pull request as draft August 12, 2025 22:18

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch 2 times, most recently from 668ade3 to 615b2f0 Compare August 18, 2025 14:34

brymut marked this pull request as ready for review August 18, 2025 14:38

brymut requested a review from ekzyis August 18, 2025 14:39

brymut closed this Aug 18, 2025

brymut deleted the enhance/browser-back-forward-navigation-carousel branch August 18, 2025 15:28

brymut restored the enhance/browser-back-forward-navigation-carousel branch August 22, 2025 03:54

brymut reopened this Aug 22, 2025

This comment was marked as outdated.

Sign in to view

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 615b2f0 to 9b5ee24 Compare August 22, 2025 09:51

This comment was marked as outdated.

Sign in to view

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch 2 times, most recently from aaed9cd to 61f12cc Compare August 22, 2025 10:11

This comment was marked as outdated.

Sign in to view

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 61f12cc to 4985b92 Compare August 22, 2025 10:26

Soxasora self-assigned this Aug 22, 2025

Soxasora reviewed Aug 22, 2025

View reviewed changes

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch 2 times, most recently from 0cf1812 to 2c08b92 Compare August 22, 2025 16:39

This comment was marked as outdated.

Sign in to view

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 2c08b92 to 505d661 Compare August 23, 2025 04:58

brymut requested a review from Soxasora August 23, 2025 05:04

Soxasora requested changes Aug 23, 2025

View reviewed changes

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 505d661 to 126a6e5 Compare August 23, 2025 12:51

Soxasora approved these changes Aug 23, 2025

View reviewed changes

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 126a6e5 to b44d747 Compare September 2, 2025 09:41

huumn requested changes Sep 2, 2025

View reviewed changes

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from b44d747 to 2af1ed3 Compare September 3, 2025 04:39

feat(carousel): use shallow query param to manage modal open/close

3b575f1

brymut force-pushed the enhance/browser-back-forward-navigation-carousel branch from 2af1ed3 to 3b575f1 Compare September 3, 2025 05:48

Uh oh!

enhance(carousel): Close carousel on browser back navigation #2262

Are you sure you want to change the base?

enhance(carousel): Close carousel on browser back navigation #2262

Uh oh!

Conversation

brymut commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Additional Context

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

This comment was marked as outdated.

Uh oh!

brymut commented Jul 10, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ekzyis left a comment

Choose a reason for hiding this comment

Uh oh!

brymut commented Jul 24, 2025

Uh oh!

brymut commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brymut commented Aug 22, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Soxasora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soxasora Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Soxasora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soxasora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brymut commented Aug 23, 2025

Uh oh!

huumn commented Aug 25, 2025

Uh oh!

brymut commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

brymut commented Jul 5, 2025 •

edited

Loading

brymut commented Aug 18, 2025 •

edited

Loading

Soxasora left a comment •

edited

Loading

Soxasora Aug 22, 2025 •

edited

Loading

Soxasora left a comment •

edited

Loading

brymut commented Aug 27, 2025 •

edited

Loading

huumn left a comment •

edited

Loading