Skip to content

fix: restore location when navigates from a nested route to a new tab#781

Open
tomrndom wants to merge 5 commits intomasterfrom
fix/sponsor-nested-route-fix
Open

fix: restore location when navigates from a nested route to a new tab#781
tomrndom wants to merge 5 commits intomasterfrom
fix/sponsor-nested-route-fix

Conversation

@tomrndom
Copy link

@tomrndom tomrndom commented Feb 5, 2026

ref: https://app.clickup.com/t/86b8d9fdp

Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com

Summary by CodeRabbit

  • Refactor
    • Tab navigation now syncs with URL fragments, initializes a default fragment when missing, and better handles nested form routes (push vs replace behavior).
  • UX
    • Nested form routes display a breadcrumb and preserve intended tab focus; "Manage Items" navigation includes a fragment to target the correct tab.
  • Tests
    • Expanded coverage for fragment-based tab behavior, nested-route scenarios, and updated test scaffolding.
  • Chores
    • Made form-list default state publicly accessible for reuse.

…a new tab

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
@tomrndom tomrndom requested a review from smarcet February 5, 2026 21:20
@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

Reworks tab ↔ URL-fragment handling on the sponsor edit page: introduces getTabFromFragment(location), derives nested-route awareness from match.params.form_id, synchronizes selected tab with location.hash via effects, initializes a default hash when empty, and routes-aware tab changes using history.push for nested routes or history.replace for same-path navigation.

Changes

Cohort / File(s) Summary
Sponsor Edit Navigation
src/pages/sponsors/edit-sponsor-page.js
Replaced getTabFromUrlFragment with getTabFromFragment(location); moved hash sync into React effects; detect nested form via match.params.form_id; initialize default hash when missing; change tab navigation to history.push for nested routes or history.replace for same-path hash updates; removed window.hashchange listener and per-tab onClick handlers.
Tests — EditSponsorPage
src/pages/sponsors/__tests__/edit-sponsor-page.test.js
Renamed imports to getTabFromFragment; added mocks for sponsor action creators; included currentSponsorFormsDefaultState; updated tests to assert history.replace for same-path tab changes and history.push for nested-route tab navigation; adjusted fragment-change and state initialization tests.
Reducer export
src/reducers/sponsors/sponsor-page-forms-list-reducer.js
Made DEFAULT_STATE a named export (export const DEFAULT_STATE) so tests/consumers can import the default forms-list state.
Routing / Layout
src/layouts/sponsor-id-layout.js
Changed sponsor-forms/:form_id/items route to use the render prop and wrap EditSponsorPage with a Breadcrumb for the nested form-items route.
Forms Tab navigation target
src/pages/sponsors/sponsor-forms-tab/index.js
Updated Manage Items link to include #forms fragment in the URL when navigating to item-items view.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Tabs as Tabs(UI)
    participant Edit as EditSponsorPage
    participant Router as History/Router
    participant Location as BrowserLocation

    User->>Tabs: select tab (e.g., "forms")
    Tabs->>Edit: onChange(fragment)
    alt nested form route active (match.params.form_id)
        Edit->>Router: history.push("/app/summits/.../sponsors/:id/sponsor-forms/:form_id/items#forms")
        Router->>Location: update URL (push)
    else same-path
        Edit->>Router: history.replace(currentPath + "#forms")
        Router->>Location: update hash (replace)
    end
    Location->>Edit: location.hash updated
    Edit->>Tabs: useEffect sync selectedTab from location.hash
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • smarcet

Poem

🐰 I hopped through hashes and tabs with delight,
If nested I push, if simple I replace in one flight.
Breadcrumbs show forms tucked in a lane,
Tabs and fragments now dance in refrain —
A rabbit applauds this tidy new sight!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly relates to the main change: refactoring tab navigation to restore location state when navigating from nested routes, which is the core objective of this pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/sponsor-nested-route-fix

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
@@ -122,7 +122,16 @@ const EditSponsorPage = (props) => {

const handleTabChange = (event, newValue) => {
Copy link

Choose a reason for hiding this comment

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

@tomrndom

const sponsorFormItemRoute =

This string-matching against location.pathname is fragile and bypasses React `Router entirely. The app already has two separate entries in sponsor-id-layout.js:144-150 that render
EditSponsorPage:

<Switch>
  <Route exact path={match.url} component={EditSponsorPage}/>
  <Route
    path={`${match.url}/sponsor-forms/:form_id/items`}
    component={EditSponsorPage}
  />
</Switch>

The nested route already captures :form_id as a route param. The proper fix is to use match.params:

const sponsorFormItemRoute = !!match.params?.form_id;

This eliminates the URL sniffing entirely and leverages React Router as intended.

setSelectedTab(newValue);
window.location.hash = getFragmentFromValue(newValue);

const basePath = `/app/summits/${currentSummit.id}/sponsors/${entity.id}`;
Copy link

Choose a reason for hiding this comment

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

just use a variable
const isNestedFormItemRoute = !!match.params?.form_id;
u could use it to at https://github.com/fntechgit/summit-admin/pull/781/changes#r2775041275
and then this method could be simple

const handleTabChange = (event, newValue) => {
setSelectedTab(newValue);
const fragment = getFragmentFromValue(newValue);
if (match.params?.form_id) {
history.push(
/app/summits/${currentSummit.id}/sponsors/${entity.id}#${fragment}
);
} else {
history.replace({ ...location, hash: #${fragment} });
}
}

@smarcet
Copy link

smarcet commented Feb 6, 2026

@tomrndom here

onClick={() => handleTabChange(null, t.value)}

 <Tabs onChange={handleTabChange}>

already fires handleTabChange when a child is clicked. The additional onClick on each causes handleTabChange to fire twice per click, which now
means a double history.push on nested routes.

@smarcet
Copy link

smarcet commented Feb 6, 2026

react router already provide these props

 const {                                                                                                                                                                                           
      entity,                                     
      member,                                                                                                                                                                                       
      history,                                                                                                                                                                                      
      location,  // ← React Router provides this                                                                                                                                                    
      match,     // ← and this           
      ...
  } = props;

use location react router instead of windows.location
so here

export const getTabFromUrlFragment = () => {

refactor it
to

  export const getTabFromFragment = (location) => {                  
    const currentHash = (location.hash || "").replace("#", "");                                                                                                                                     
    const result = tabsToFragmentMap.indexOf(currentHash);         
    if (result > -1) return result;                                                                                                                                                                 
    return 0;                                                                                                                                                                                       
  };   

and here

lets refactor to

 useEffect(() => {             
     setSelectedTab(getTabFromUrlFragment(location));                                                                                                                                              
   }, [location.hash]);                 
  
  useEffect(() => {
     if (!location.hash) {
       history.replace({ ...location, hash: `#${getFragmentFromValue(0)}` });
     }
  }, []);

This way:

  • First useEffect — syncs tab state whenever the hash changes (React Router handles all navigation events)
  • Second useEffect — on mount only, sets the default #general hash in the URL if none exists, without calling handleTabChange or relying on stale closures

Copy link

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@tomrndom please review comments

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/pages/sponsors/__tests__/edit-sponsor-page.test.js`:
- Around line 83-100: The first test uses a nested shape for
currentSummitSponsorshipListState (an object with a sponsorships property
containing the list and pagination) but the reducer expects a flat shape; update
the test's state for currentSummitSponsorshipListState to the flat structure
(sponsorships, currentPage, lastPage, perPage, order, orderDir,
totalSponsorships) to match the reducer and the second test; ensure
currentSponsorState and sponsorPageFormsListState remain unchanged.
🧹 Nitpick comments (2)
src/pages/sponsors/edit-sponsor-page.js (1)

193-193: Remove duplicate variable.

sponsorFormItemRoute is identical to isNestedFormItemRoute defined at line 123. Use the existing variable instead.

♻️ Suggested fix
-  const sponsorFormItemRoute = !!match.params?.form_id;
-
   return (
     <Box>
       ...
         <CustomTabPanel value={selectedTab} index={4}>
-          {sponsorFormItemRoute ? (
+          {isNestedFormItemRoute ? (
             <SponsorFormsManageItems match={match} />
src/pages/sponsors/__tests__/edit-sponsor-page.test.js (1)

105-105: Misleading variable name.

The variable is named usersTabReference but it selects "edit_sponsor.tab.forms". Rename to formsTabReference for clarity.

♻️ Suggested fix
-      const usersTabReference = screen.getByText("edit_sponsor.tab.forms");
+      const formsTabReference = screen.getByText("edit_sponsor.tab.forms");

       await act(async () => {
-        await userEvent.click(usersTabReference);
+        await userEvent.click(formsTabReference);
       });

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
@tomrndom tomrndom requested a review from smarcet February 6, 2026 22:14
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