Skip to content

refactor(lib): move info modal to dedicated /info route#657

Open
antontranelis wants to merge 6 commits intomainfrom
feature/info-modal-route
Open

refactor(lib): move info modal to dedicated /info route#657
antontranelis wants to merge 6 commits intomainfrom
feature/info-modal-route

Conversation

@antontranelis
Copy link
Member

Summary

  • Move Modal from always-rendered to route-based rendering at /info
  • Update Modal component to use MapOverlayPage instead of dialog element
  • Update ModalContent to use useNavigate for closing (navigates to /)
  • Add automatic redirect to /info when map.info_open is true on first load

Changes

This PR extracts the info modal changes from PR #260 into a separate, focused PR.

Files changed:

  • app/src/App.tsx - Remove always-rendered Modal, add /info route, add redirect logic
  • app/src/ModalContent.tsx - Use useNavigate for closing instead of dialog.close()
  • lib/src/Components/Gaming/Modal.tsx - Use MapOverlayPage instead of dialog element

Benefits

  • Info modal is now accessible via URL (/info)
  • Cleaner architecture using existing routing system
  • Modal can be bookmarked/shared via URL

Test plan

  • Visit /info route directly - modal should appear
  • Close modal - should navigate to /
  • If map.info_open is true, visiting / should redirect to /info
  • Modal styling should match previous appearance

🤖 Generated with Claude Code

- Move Modal from always-rendered to route-based rendering at /info
- Update Modal component to use MapOverlayPage instead of dialog element
- Update ModalContent to use useNavigate for closing (navigates to /)
- Add automatic redirect to /info when map.info_open is true on first load

This change makes the info modal accessible via URL and improves
the architecture by using the existing routing system.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@antontranelis antontranelis changed the title refactor: move info modal to dedicated /info route refactor(lib): move info modal to dedicated /info route Jan 12, 2026
antontranelis and others added 2 commits January 12, 2026 19:34
- Add InfoRedirect component for one-time redirect to /info on page load
- Simplify Modal component to pure presentation (remove redirect logic)
- Change NavBar info button from showModal() to Link to /info route
- Remove unused window.my_modal_3 global type declaration
- Fix modal centering by removing conflicting position classes

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the info modal from an always-rendered dialog element to a route-based modal accessible at /info. The changes improve the architecture by leveraging the existing routing system and make the modal shareable via URL.

Changes:

  • Removed the old Gaming Modal component and its window.my_modal_3 global interface
  • Created new Modal component using MapOverlayPage in AppShell
  • Added InfoRedirect component to handle automatic redirect when map.info_open is true
  • Updated NavBar to use Link instead of button for opening info modal

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
lib/src/index.tsx Removed global Window interface declaration for my_modal_3
lib/src/Components/Gaming/index.tsx Removed Modal export from Gaming components
lib/src/Components/Gaming/Modal.tsx Deleted old dialog-based Modal component
lib/src/Components/AppShell/index.tsx Added exports for InfoRedirect and Modal components
lib/src/Components/AppShell/NavBar.tsx Changed info button to Link component pointing to /info route
lib/src/Components/AppShell/Modal.tsx New Modal component wrapping MapOverlayPage with appropriate styling
lib/src/Components/AppShell/InfoRedirect.tsx New component to redirect to /info on initial page load when enabled
app/src/ModalContent.tsx Updated close function to use useNavigate instead of dialog.close()
app/src/App.tsx Removed always-rendered Modal, added /info route, integrated InfoRedirect

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mahula
Copy link
Collaborator

mahula commented Feb 4, 2026

augment review

@augmentcode
Copy link

augmentcode bot commented Feb 4, 2026

🤖 Augment PR Summary

Summary: This PR refactors the “info” modal to be rendered via routing instead of an always-mounted dialog.

Changes:

  • Adds a dedicated /info route that renders the modal content via <Routes>
  • Introduces InfoRedirect to automatically navigate to /info on first load when map.info_open is enabled
  • Replaces the old dialog-based modal with an AppShell-level Modal built on MapOverlayPage
  • Updates the NavBar help icon to link to /info
  • Removes the legacy Gaming Modal and the global window.my_modal_3 typing

Technical Notes: The info overlay is now URL-addressable/bookmarkable, and closing behavior is handled through React Router navigation.

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@@ -86,7 +79,6 @@ export const ModalContent = ({ map }: { map: any }) => {
clickAction1={() => {
close()
setTimeout(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since close() now navigates away from /info, ModalContent will unmount and this setTimeout can fire after unmount (state update on an unmounted component). Consider removing the delayed setChapter reset here or ensuring the timeout is cleaned up.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antontranelis This is a valid point. Since close() now navigates to /, ModalContent unmounts and the setTimeout fires on an unmounted component.

However, the fix is simpler than just cleanup — the setTimeout + setChapter(1) can be removed entirely. Since the component unmounts on close and remounts fresh on the next /info visit, chapter will already be 1 (the initial useState value). The reset is unnecessary.

  clickAction1={() => {
    close()
-   setTimeout(() => {
-     setChapter(1)
-   }, 1000)
  }}

I'll approve once this is addressed.

Copy link
Collaborator

@mahula mahula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a clean, well-scoped refactoring. The architectural direction (route-based modals over imperative DOM) is the right call, and the removal of the window.my_modal_3 global is a welcome cleanup.

One change requested: Remove the unnecessary setTimeout + setChapter(1) in ModalContent.tsx — since the component now unmounts on close and remounts fresh, the state reset is redundant and fires after unmount.

The other two bot-flagged issues (InfoRedirect useRef persistence and duplicate close buttons) are non-issues — see inline replies for reasoning.

Will approve once the setTimeout is removed.

mahula added a commit that referenced this pull request Feb 6, 2026
- Remove setTimeout firing after unmount in ModalContent (CR1)
- 5 Vitest unit tests for InfoRedirect guard logic
- 4 Cypress E2E tests for route, navigation, and close flows
mahula added a commit that referenced this pull request Feb 6, 2026
- Remove setTimeout firing after unmount in ModalContent (CR1)
- 4 Vitest unit tests for InfoRedirect guard logic
- 3 Cypress E2E tests for route, navigation, and close flow
- Restore window.location in afterEach to prevent cross-suite leaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments