-
Notifications
You must be signed in to change notification settings - Fork 4
86b82fa34 - feat: add url fragment linking to tabs on edit sponsor page #751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe edit sponsor page now persists tab selection via URL fragments, enabling direct linking and bookmarking of specific tabs. Helper functions map between tab indices and URL anchors, with state initialization and synchronization managed through useEffect. A minor layout adjustment standardizes Container styling. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Page Component
participant Tab State
Note over Browser,Tab State: Initial Page Load
Browser->>Page Component: Render with URL hash
Page Component->>Browser: Read window.location.hash
Page Component->>Page Component: getTabFromUrlFragment()
Page Component->>Tab State: Initialize selectedTab
Tab State-->>Page Component: State updated
Note over Browser,Tab State: User Changes Tab
Page Component->>Page Component: onTabChange triggered
Page Component->>Page Component: getFragmentFromValue()
Page Component->>Browser: Push URL hash
Page Component->>Tab State: Update selectedTab
Note over Browser,Tab State: Hash Changed (External)
Browser->>Page Component: window.location.hash changed
Page Component->>Page Component: useEffect triggered
Page Component->>Tab State: Resync selectedTab
Tab State-->>Page Component: State updated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this 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/edit-sponsor-page.js:
- Around line 116-118: The useEffect using window.location.hash in the
dependency array won't re-run on hash changes; replace it with a hashchange
event listener: in the effect that currently calls
setSelectedTab(getTabFromUrlFragment()), register
window.addEventListener('hashchange', handler) where handler calls
setSelectedTab(getTabFromUrlFragment()), call the handler once on mount to
initialize, and remove the listener in the cleanup; remove window.location.hash
from the dependency array (use []).
🧹 Nitpick comments (1)
src/pages/sponsors/edit-sponsor-page.js (1)
46-64: Helper functions look good, consider consolidating the tab definition.The mapping and helper functions are well-structured. The fallback to index 0 for unrecognized fragments correctly implements the PR requirement.
Consider deriving
tabsToFragmentMapfrom thetabsarray (or vice versa) to avoid maintaining two separate lists that must stay in sync. For now, this is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors/edit-sponsor-page.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
src/pages/sponsors/edit-sponsor-page.js (2)
120-123: LGTM!Setting
window.location.hashcorrectly updates the URL and adds a browser history entry, enabling back/forward navigation between tabs.
155-155: Formatting change acknowledged.Minor whitespace adjustment in the sx prop—no functional impact.
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niko-exo please review
| const result = tabsToFragmentMap.indexOf( | ||
| window.location.hash.replace("#", "") | ||
| ); | ||
| return result > -1 ? result : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the initial load
general tab should be selected hence #general should be show by default
if (result > -1) return result;
if (window.location.hash) window.location.hash = "general";
return 0
```;
| const [selectedTab, setSelectedTab] = useState(0); | ||
| const [selectedTab, setSelectedTab] = useState(getTabFromUrlFragment()); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niko-exo
rite now the tab selection only updates from the URL hash on initial page load. The effect useEffect(..., [window.location.hash]) won’t reliably fire on hash updates because changing window.location.hash doesn’t trigger a React re-render.
To support SPA-style hash navigation (manual hash edits, back/forward), please replace that effect with a hashchange listener
ref: https://app.clickup.com/t/86b82fa34
86b82fa34 - feat: add url fragment linking to tabs on edit sponsor page
Changelog
Links
86b82fa34 - Add TAB deep-linking using URL fragment
Evidence
2026-01-12_09-56-27.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.