fix: remove invalid setDebouncedSearch call in clearAll#198
Conversation
|
@ShreyaSinghal22 is attempting to deploy a commit to the Kunal Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
🎉 Welcome to CodeLens — Thank You for Your Contribution!Hey @ShreyaSinghal22! 👋 We are genuinely excited to have you here. Every single PR — big or small — makes CodeLens better, and yours is no exception. Take a moment to review the checklist below to help us merge your work quickly and smoothly. ✅ Before Requesting a Review
💬 Join Our Community Channel — This is MandatoryBeing part of our communication channel is compulsory for all contributors, not optional. Why join? This is where all important announcements, PR review updates, contribution discussions, and maintainer decisions happen in real time. Contributors who are not in the channel regularly miss critical context and updates, which often leads to duplicated or misaligned work. Staying connected here is what keeps the community strong and your contributions impactful. We are rooting for you! If you have any questions, drop them in the channel or comment right here on this PR. Let's build something great together. 🚀✨ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRemoves one invalid line from the ChangesFix clearAll crash on Practice Page
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 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 |
|
@coderabbitai please check whether work done in this PR is good to go and is ready to merge ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PracticePage.jsx full content ==="
fd "PracticePage.jsx" --exec cat {}
echo ""
echo "=== useDebounce hook ==="
fd -i "useDebounce" --exec cat {}
echo ""
echo "=== Search for setDebouncedSearch across entire codebase ==="
rg -rn "setDebouncedSearch" --type jsx --type js --type tsx --type ts
echo ""
echo "=== Search for debouncedSearch usage ==="
rg -rn "debouncedSearch"
echo ""
echo "=== clearAll function references across codebase ==="
rg -rn "clearAll"
echo ""
echo "=== PR diff via GitHub CLI ==="
gh pr view 198 --repo kunalverma2512/CodeLens --json files,body,title,state,additions,deletions
echo ""
echo "=== PR diff ==="
gh pr diff 198 --repo kunalverma2512/CodeLensLength of output: 34812 🏁 Script executed: #!/bin/bash
echo "=== Directory structure of frontend/src/pages ==="
fd . frontend/src/pages --type f
echo ""
echo "=== Directory structure of frontend/src/hooks ==="
fd . frontend/src/hooks --type f
echo ""
echo "=== Frontend package.json ==="
fd "package.json" --max-depth 3 --exec cat {} \; -x echo "---FILE: {}"Length of output: 3199
✅ Verdict: The Fix is Correct and Safe to MergeThe change is a single-line removal that resolves a genuine, reproducible 🔍 Root Cause AnalysisThe export function useDebounce(value, delay) {
const [debouncedValue, setDebouncedValue] = useState(value);
useEffect(() => {
const handler = setTimeout(() => setDebouncedValue(value), delay);
return () => clearTimeout(handler);
}, [value, delay]);
return debouncedValue; // ← scalar only, no [value, setter] tuple
}
const debouncedSearch = useDebounce(searchQuery, 300);There is no ✅ Is the Fix Sufficient?Yes, completely. When useEffect(() => {
setPage(1);
}, [debouncedSearch]);So pagination is doubly covered — both by the
|
| Aspect | Assessment |
|---|---|
| Fix correctness | ✅ Correct |
| Fix completeness | ✅ Sufficient |
| Scope discipline | ✅ Minimal, no unrelated changes |
| PR title | ✅ Follows conventional commits (fix:) |
| PR description | ✅ Technically accurate, professionally written |
| Issue linkage | ✅ Closes #161 correctly |
| Testing | ✅ Manual testing described; no automated tests (pre-existing gap) |
| Breaking changes | ✅ None |
🏁 Summary
The PR correctly identifies and eliminates the one invalid line causing a ReferenceError crash on the Practice Page. The useDebounce contract is respected, the debounce behavior for normal typing is unaffected, and all filter-reset state setters now execute without interruption. The codebase compatibility is fully maintained — no other files are impacted.
This is ready to merge. The pre-existing items above are good candidates for a follow-up polish PR if desired.
🐇✨
|
@coderabbitai Please make a new issue for what you said about first point you said in pre existing minor observation in your recent comment Also I want before making the new issue please read this comment which you said in issue of this PR where you told one bug related to - "useDebounce to return a [value, setValue] tuple" is both things said in that issue comment and the one you said in this PR comment are same ? Make sure new issue must have complete reasoning and detailed description about whats that issue and where it makes the codebase bad and how to work on that new issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the specific comment from issue `#161`
gh api repos/kunalverma2512/CodeLens/issues/161/comments --jq '.[] | select(.id == 4701401810 or (.html_url | contains("4701401810"))) | {id: .id, body: .body, url: .html_url}'
echo ""
echo "=== All comments on issue 161 ==="
gh api repos/kunalverma2512/CodeLens/issues/161/comments --jq '.[] | {id: .id, url: .html_url, snippet: .body[0:300]}'Length of output: 12626 🏁 Script executed: #!/bin/bash
gh issue create \
--repo kunalverma2512/CodeLens \
--assignee kunalverma2512 \
--title "UX: Active filter chips flicker/persist ~300ms after clicking \"Clear All\" on Practice Page" \
--body "## Summary
After clicking either **\"Clear All\"** button on the Practice Page, active filter chips (e.g., the search badge \`Active: \"foo\"\`) remain visible for approximately **300 milliseconds** before disappearing. This creates a visual flicker that makes the UI feel unresponsive and inconsistent — the user clicks the button, the list resets, but the chip badges linger briefly before going away.
This is a pre-existing UX defect, discovered during the review of PR `#198` (which fixed the related crash \`ReferenceError: setDebouncedSearch is not defined\`).
---
## Root Cause
In \`frontend/src/pages/PracticePage.jsx\`, the computed variable \`hasFilters\` — which controls the visibility of the active filter chip section — reads from \`debouncedSearch\` instead of \`searchQuery\`:
\`\`\`js
// Current (problematic)
const hasFilters = debouncedSearch || selectedTags.length > 0 || ratingIdx !== 0 || sortBy !== \"rating-asc\";
\`\`\`
\`debouncedSearch\` is produced by the \`useDebounce\` hook (\`frontend/src/hooks/useDebounce.js\`) with a 300ms delay. It only updates after the timer fires. So when \`clearAll()\` calls \`setSearchQuery(\"\")\`:
1. \`searchQuery\` becomes \`\"\"\" immediately — the input field clears ✅
2. \`debouncedSearch\` stays at the old value for ~300ms ⚠️
3. Because \`hasFilters\` depends on \`debouncedSearch\`, the chip section remains rendered for that 300ms window ⚠️
4. After 300ms, \`debouncedSearch\` catches up, \`hasFilters\` becomes falsy, and the chips vanish
This is **semantically incorrect**: \`hasFilters\` is a UI visibility gate for _what the user has entered_, not a gate for _what query is currently being debounced to the filter logic_. The two should not be conflated.
---
## Why This Matters for the Codebase
- **Inconsistency:** Other filter states (\`selectedTags\`, \`ratingIdx\`, \`sortBy\`) reset and hide their chips instantly because they are not debounced. Only the search chip has this lag, creating an uneven UX.
- **Semantic incorrectness:** Using a debounced value to drive immediate UI visibility is architecturally wrong. \`debouncedSearch\` exists to throttle expensive operations (e.g., API/filter calls), not to gate chip visibility.
- **User confusion:** The user may click \"Clear All\" again, thinking the first click did not register, because the chip is still showing.
---
## Historical Context
During issue `#161`, an alternative approach was discussed: modifying \`useDebounce\` to return a \`[value, setter]\` tuple so the debounced value could be manually reset to \`\"\"\` in \`clearAll()\`. That approach was explicitly deferred to a separate issue because it adds hook API complexity and is unnecessary — the chip flicker can be fixed entirely at the component level without touching the hook.
---
## Acceptance Criteria
- [ ] Clicking \"Clear All\" (the inline chip button) causes the active filter chip section to disappear **immediately**, with no visible delay
- [ ] Clicking \"Clear All Filters\" (the empty-results button) causes the same immediate disappearance
- [ ] Normal debounce behavior during typing in the search box is **unaffected** — the filter/list results still debounce correctly at 300ms
- [ ] No regression in other filter chip states (tags, rating, sort)
- [ ] No console errors
---
## Files to Investigate
- \`frontend/src/pages/PracticePage.jsx\` — specifically the \`hasFilters\` computed variable and where it controls chip visibility
- \`frontend/src/hooks/useDebounce.js\` — for context on what \`debouncedSearch\` represents (read-only derived value, not a UI state)
---
## References
- PR `#198` — Fix: ReferenceError crash on Clear All (where this was first observed)
- Issue `#161` — Original crash report (where the tuple approach was considered and deferred)
/cc `@kunalverma2512`"Length of output: 4133
🔍 Are Both Things the Same?No — they are siblings targeting the same UX symptom, but through different layers.
Key insight: The ✅ New Issue Created
The issue contains:
🐇✨ |
|
@ShreyaSinghal22 Well done Come up on our communication channel for future collaboration : https://matrix.to/#/#codelensx:matrix.org Thanks |
|
@kunalverma2512 thankyou for assigning this issue to me. I will surely see through and analyse codelens and would love to contribute more in it. |
2 participants
📌 Pull Request Summary
🔗 Related Issue
Closes #161
📝 Description
Fixed a
ReferenceErrorcrash that occurred when clicking the "Clear All"or "Clear All Filters" button on the Practice Page. The page was becoming
completely unresponsive to filter clearing.
Changes Made
setDebouncedSearch("")call fromclearAll()inPracticePage.jsxMotivation
The
useDebouncehook returns only a debounced read-only value — it neverexposes a setter. Calling
setDebouncedSearch("")caused JavaScript to throwa
ReferenceErrorimmediately, which halted execution and prevented allthe reset logic below it (
setSelectedTags,setRatingIdx,setSortBy,setPage) from running. Removing this one invalid line is sufficient —debouncedSearchautomatically followssearchQueryafter the 300msdebounce timer.
🚀 Type of Change
🧪 Testing
Verification
Test Details
/practicepageReferenceErrorpresent ✅📸 Screenshots / Demo (If Applicable)
✅ Checklist
📚 Additional Notes
Two buttons both call
clearAll()and were both broken by this bug:Both are fixed by this single change. No regression to debounce behaviour
during normal typing. No backend changes required.
Summary by CodeRabbit