fix(tabs): honor hash-linked nav tabs#12303
fix(tabs): honor hash-linked nav tabs#12303tarunvashishth wants to merge 1 commit intopatternfly:mainfrom
Conversation
WalkthroughThe change adds hash-based tab selection for nav-linked tabs. When Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Preview: https://pf-react-pr-12303.surge.sh A11y report: https://pf-react-pr-12303-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-core/src/components/Tabs/Tabs.tsx`:
- Around line 194-198: Seed the uncontrolledActiveKey state using the
hash-derived key so initial selection matches shownKeys: set
uncontrolledActiveKey to hashActiveKey ?? this.props.defaultActiveKey (mirroring
shownKeys logic that uses hashActiveKey ?? ...). Update the initializer that
defines uncontrolledActiveKey in the Tabs component so it references
hashActiveKey, and ensure any later render-time hash derivation logic is removed
or not used to override user interactions after mount (preserve user changes to
uncontrolledActiveKey afterwards).
- Around line 145-156: Tests are missing for getTabHashActiveKey to verify
hash-based selection; add unit tests in the Tabs spec that set
window.location.hash (use a beforeEach to assign a hash and afterEach to clear
it) and assert getTabHashActiveKey (via rendering Tabs or importing the function
if exported) returns the expected eventKey when a child TabElement has matching
href, and does not return keys for hidden/disabled tabs or when component/isNav
conditions are unmet; target the getTabHashActiveKey logic and exercise
Tabs/TabsProps behavior with TabElement props (href, isHidden, isDisabled,
isAriaDisabled) so hash-based navigation is covered.
- Around line 559-560: The code currently recalculates hashActiveKey each render
(getTabHashActiveKey) and then uses it to set localActiveKey, which causes the
URL hash to override user clicks; instead, remove the per-render usage of
getTabHashActiveKey and seed uncontrolledActiveKey from the hash only on initial
mount/constructor (where you already compute it around line ~186), or use the
existing initial shownKeys/uncontrolledActiveKey initialization to incorporate
the hash-derived key; update Tabs constructor/initialization to set
uncontrolledActiveKey from getTabHashActiveKey once and ensure handleTabClick
updates uncontrolledActiveKey (not the URL hash) so subsequent renders respect
the user's selection rather than recomputing hashActiveKey each render.
🪄 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: 3544653a-d87d-4a92-8b53-8011dd08fcc6
📒 Files selected for processing (1)
packages/react-core/src/components/Tabs/Tabs.tsx
| const getTabHashActiveKey = ({ children, component, isNav }: Pick<TabsProps, 'children' | 'component' | 'isNav'>) => { | ||
| if (!canUseDOM || !(component === TabsComponent.nav || isNav) || !window.location.hash) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return Children.toArray(children) | ||
| .filter((child): child is TabElement => isValidElement(child)) | ||
| .filter(({ props }) => !props.isHidden) | ||
| .find( | ||
| ({ props }) => !props.isDisabled && !props.isAriaDisabled && getHashFromHref(props.href) === window.location.hash | ||
| )?.props.eventKey; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Tabs tests have any hash-related setup
rg -n "location.hash|window.location" packages/react-core/src/components/Tabs/__tests__/Repository: patternfly/patternfly-react
Length of output: 53
Add test coverage for the getTabHashActiveKey function with hash-based navigation.
The new getTabHashActiveKey function accesses window.location.hash and should be tested to ensure the hash-based tab selection works correctly and prevent regressions. Currently, no hash-related test setup exists in the Tabs tests, while the Nav component tests demonstrate the pattern with window.location.hash setup in a beforeEach block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Tabs/Tabs.tsx` around lines 145 - 156,
Tests are missing for getTabHashActiveKey to verify hash-based selection; add
unit tests in the Tabs spec that set window.location.hash (use a beforeEach to
assign a hash and afterEach to clear it) and assert getTabHashActiveKey (via
rendering Tabs or importing the function if exported) returns the expected
eventKey when a child TabElement has matching href, and does not return keys for
hidden/disabled tabs or when component/isNav conditions are unmet; target the
getTabHashActiveKey logic and exercise Tabs/TabsProps behavior with TabElement
props (href, isHidden, isDisabled, isAriaDisabled) so hash-based navigation is
covered.
| shownKeys: | ||
| this.props.defaultActiveKey !== undefined | ||
| ? [hashActiveKey ?? this.props.defaultActiveKey] | ||
| : [hashActiveKey ?? this.props.activeKey], // only for mountOnEnter case | ||
| uncontrolledActiveKey: this.props.defaultActiveKey, |
There was a problem hiding this comment.
Initialize uncontrolledActiveKey with hash-derived key for consistency.
The shownKeys correctly prioritizes hashActiveKey, but uncontrolledActiveKey is still set only to defaultActiveKey. For the hash-based initial selection to persist after user interaction, uncontrolledActiveKey should also be seeded from the hash:
- uncontrolledActiveKey: this.props.defaultActiveKey,
+ uncontrolledActiveKey: hashActiveKey ?? this.props.defaultActiveKey,This change, combined with removing the render-time hash derivation, would ensure the hash is honored on initial load but doesn't override subsequent user selections.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shownKeys: | |
| this.props.defaultActiveKey !== undefined | |
| ? [hashActiveKey ?? this.props.defaultActiveKey] | |
| : [hashActiveKey ?? this.props.activeKey], // only for mountOnEnter case | |
| uncontrolledActiveKey: this.props.defaultActiveKey, | |
| shownKeys: | |
| this.props.defaultActiveKey !== undefined | |
| ? [hashActiveKey ?? this.props.defaultActiveKey] | |
| : [hashActiveKey ?? this.props.activeKey], // only for mountOnEnter case | |
| uncontrolledActiveKey: hashActiveKey ?? this.props.defaultActiveKey, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Tabs/Tabs.tsx` around lines 194 - 198,
Seed the uncontrolledActiveKey state using the hash-derived key so initial
selection matches shownKeys: set uncontrolledActiveKey to hashActiveKey ??
this.props.defaultActiveKey (mirroring shownKeys logic that uses hashActiveKey
?? ...). Update the initializer that defines uncontrolledActiveKey in the Tabs
component so it references hashActiveKey, and ensure any later render-time hash
derivation logic is removed or not used to override user interactions after
mount (preserve user changes to uncontrolledActiveKey afterwards).
| const hashActiveKey = getTabHashActiveKey({ children, component, isNav }); | ||
| const localActiveKey = hashActiveKey ?? (defaultActiveKey !== undefined ? uncontrolledActiveKey : activeKey); |
There was a problem hiding this comment.
Hash takes precedence on every render, potentially overriding user tab selections.
When hashActiveKey is recomputed on each render (line 559), it will continuously override uncontrolledActiveKey even after the user clicks a different tab. The URL hash doesn't automatically update when tabs are clicked via handleTabClick, so the hash-derived key will keep "winning" over the user's selection.
Consider computing the hash-based key only on initial mount (which you already do at line 186), and using it to seed the initial state rather than re-deriving it on every render:
- const hashActiveKey = getTabHashActiveKey({ children, component, isNav });
- const localActiveKey = hashActiveKey ?? (defaultActiveKey !== undefined ? uncontrolledActiveKey : activeKey);
+ const localActiveKey = defaultActiveKey !== undefined ? uncontrolledActiveKey : activeKey;The initial shownKeys and uncontrolledActiveKey initialization in the constructor should handle the initial hash-based selection. Alternatively, you could update uncontrolledActiveKey in the constructor to use the hash:
- uncontrolledActiveKey: this.props.defaultActiveKey,
+ uncontrolledActiveKey: hashActiveKey ?? this.props.defaultActiveKey,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hashActiveKey = getTabHashActiveKey({ children, component, isNav }); | |
| const localActiveKey = hashActiveKey ?? (defaultActiveKey !== undefined ? uncontrolledActiveKey : activeKey); | |
| const localActiveKey = defaultActiveKey !== undefined ? uncontrolledActiveKey : activeKey; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Tabs/Tabs.tsx` around lines 559 - 560, The
code currently recalculates hashActiveKey each render (getTabHashActiveKey) and
then uses it to set localActiveKey, which causes the URL hash to override user
clicks; instead, remove the per-render usage of getTabHashActiveKey and seed
uncontrolledActiveKey from the hash only on initial mount/constructor (where you
already compute it around line ~186), or use the existing initial
shownKeys/uncontrolledActiveKey initialization to incorporate the hash-derived
key; update Tabs constructor/initialization to set uncontrolledActiveKey from
getTabHashActiveKey once and ensure handleTabClick updates uncontrolledActiveKey
(not the URL hash) so subsequent renders respect the user's selection rather
than recomputing hashActiveKey each render.
2 participants
Closes #12094
Summary by CodeRabbit