fix: Reset lastClicked in NavLink when route matches#3796
fix: Reset lastClicked in NavLink when route matches#3796doubleface wants to merge 2 commits intomasterfrom
Conversation
Also added unit tests for these cases
doubleface
requested review from
JF-Cozy,
lethemanh,
rezk2ll and
zatteo
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR updates Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BundleMonFiles updated (1)
Unchanged files (20)
Total files change +71B 0% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
|
@coderabbitai Does it fix also this use case? #3793 (comment) I can't see a UT for this one |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the referenced issue comment and the test file to check coverage. 🏁 Script executed: Length of output: 79 🏁 Script executed: Length of output: 4726 The referenced comment is a video recording. Let me fetch the full issue thread to understand the specific scenario it demonstrates. 🏁 Script executed: Length of output: 384 🏁 Script executed: Length of output: 1359
Here's the gap: The useEffect(() => {
if (!lastClicked) return
if (navLinkMatch(rx, lastClicked, location.pathname)) {
setLastClicked(null) // ✅ route reached the destination → clear
}
// ❌ Missing: what if the route goes somewhere ELSE (e.g., back button)?
}, [location.pathname, rx, lastClicked, setLastClicked])The uncovered scenario (likely what the video demonstrates):
The existing test even encodes this broken behavior as a success: it('does not reset lastClicked when route changes to a different destination', async () => {
// Route changed to /trash, not /folder → expects setLastClicked NOT called ✅ (but this is the bug)
expect(setLastClicked).not.toHaveBeenCalledWith(null)
})To fully fix this, the effect should also reset useEffect(() => {
if (!lastClicked) return
if (navLinkMatch(rx, lastClicked, location.pathname)) {
setLastClicked(null) // route arrived at destination
} else if (!navLinkMatch(rx, to, location.pathname)) {
setLastClicked(null) // route went somewhere else (e.g., back button)
}
}, [location.pathname, rx, lastClicked, to, setLastClicked])And a corresponding unit test would be: it('resets lastClicked when route changes away from the clicked destination (e.g. back button)', async () => {
const setLastClicked = jest.fn()
useLocation.mockReturnValue({ pathname: '/recent' })
const rx = /\/(folder|nextcloud)(\/.*)?/
const { rerender } = render(
<NavLink to="/folder" rx={rx} clickState={['/folder', setLastClicked]}>
Drive
</NavLink>
)
// Back button takes us somewhere that is NOT /folder
useLocation.mockReturnValue({ pathname: '/recent' })
await act(async () => {
rerender(
<NavLink to="/folder" rx={rx} clickState={['/folder', setLastClicked]}>
Drive
</NavLink>
)
})
expect(setLastClicked).toHaveBeenCalledWith(null)
})Could you confirm this matches what the video shows, 🧠 Learnings used |
Good catch on the missing case. However the proposed fix has a subtle flaw: The fix I went with instead tracks the previous This way:
Also updated the existing test that was encoding the bug ( |
There was a problem hiding this comment.
Gates Failed
(1 new file with code health below 10.00)
Gates Passed
See analysis details in CodeScene
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| NavLink.spec.jsx | 1 rule | 9.39 | Suppress |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| it('resets lastClicked once the route matches rx', async () => { | ||
| const setLastClicked = jest.fn() | ||
| useLocation.mockReturnValue({ pathname: '/recent' }) | ||
| const rx = /\/(folder|nextcloud)(\/.*)?/ | ||
|
|
||
| const { rerender } = render( | ||
| <NavLink to="/folder" rx={rx} clickState={['/folder', setLastClicked]}> | ||
| Drive | ||
| </NavLink> | ||
| ) | ||
|
|
||
| useLocation.mockReturnValue({ pathname: '/folder' }) | ||
| await act(async () => { | ||
| rerender( | ||
| <NavLink | ||
| to="/folder" | ||
| rx={rx} | ||
| clickState={['/folder', setLastClicked]} | ||
| > | ||
| Drive | ||
| </NavLink> | ||
| ) | ||
| }) | ||
|
|
||
| expect(setLastClicked).toHaveBeenCalledWith(null) | ||
| }) |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 3 functions with similar structure: 'resets lastClicked once the route matches rx','resets lastClicked once the route matches without rx','resets lastClicked when route changes to a different destination (e.g. back button)'
2 participants
Keeps the optimistic highlight in the sidebar but also works when using the browser back button.
Also added unit tests for these cases
fixes #3793
Summary by CodeRabbit
Bug Fixes
Tests