fix(frontend): reset scroll position to top on new search (#1461) - #1472
fix(frontend): reset scroll position to top on new search (#1461)#1472sanskar-singh-2403 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe main content wrapper now has a stable DOM id. ChangesSearch scroll reset
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Poem
Sequence Diagram(s)sequenceDiagram
participant SearchResults
participant MainScrollContainer
participant SearchResultsTest
SearchResultsTest->>SearchResults: Navigate to a new search query
SearchResults->>MainScrollContainer: Call scrollTo({ top: 0 })
MainScrollContainer-->>SearchResultsTest: Update scrollTop to 0
SearchResultsTest->>SearchResults: Verify the new results render
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/pages/__tests__/SearchResults.test.tsx (1)
384-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed
scrollTomock.
scrollToMock as unknown as typeof window.scrollTobypasses the TypeScript check. Give the mock anHTMLElement['scrollTo']-compatible signature or define the mocked property without the double assertion. Please confirm the replacement with the repository TypeScript check.As per coding guidelines: “In frontend TypeScript, type every export and API boundary, do not add
any, and do not useasto silence real type errors.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/__tests__/SearchResults.test.tsx` around lines 384 - 385, Replace the double assertion assigned to scrollContainer.scrollTo with a properly typed mock matching HTMLElement["scrollTo"], or define the mocked property with that compatible type. Keep the existing test behavior, avoid any and assertion-based type suppression, and verify the change with the repository TypeScript check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/__tests__/SearchResults.test.tsx`:
- Around line 377-405: Update the “resets the main scroll container…” test to
exercise a new-search transition while keeping the same SearchResults render
mounted: perform the initial search, clear scrollToMock, set a non-zero
scrollTop, navigate to a different query or mode through the rendered UI, then
assert the scroll container’s resulting scroll state is zero. Preserve the
existing Jest and React Testing Library setup and verify user-visible behavior
rather than effect implementation details.
---
Nitpick comments:
In `@frontend/src/pages/__tests__/SearchResults.test.tsx`:
- Around line 384-385: Replace the double assertion assigned to
scrollContainer.scrollTo with a properly typed mock matching
HTMLElement["scrollTo"], or define the mocked property with that compatible
type. Keep the existing test behavior, avoid any and assertion-based type
suppression, and verify the change with the repository TypeScript check.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23669a3d-08fd-4c3a-8ebe-bdfd71db8db5
📒 Files selected for processing (3)
frontend/src/layout/layout.tsxfrontend/src/pages/SearchResults/SearchResults.tsxfrontend/src/pages/__tests__/SearchResults.test.tsx
e51c813 to
a99609b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
a99609b to
2489f4b
Compare
Link your account with GitcordThanks for opening this PR, @sanskar-singh-2403! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
|
@rohan-pandeyy this is ready to be reviewed, Thanks! |
|
@sanskar-singh-2403 please attach a demonstration video |
|
@rohan-pandeyy PFA |
952fddb to
36bc408
Compare
36bc408 to
170a4ab
Compare
3 participants
Addressed Issues:
Fixes #1461
Screenshots/Recordings:
DRIVE LINK
Additional Notes:
Root cause
The app has a single scroll container in
layout.tsx(theoverflow-y-autodiv that wraps<Outlet />). Running a new search only changes thevalue/modequery params on/search, so theSearchResultspage stays mounted and that container keeps its previousscrollTop. If the user had scrolled a long result set to the bottom and then ran a search that returns fewer items, the new results render at the top but the viewport stays parked below them, leaving the user looking at an empty area.Fix (scroll-restoration only, as requested in the issue)
layout.tsx: gave the scroll container a stableid="main-scroll-container"so it is addressable.SearchResults.tsx: added an effect that scrolls that container to the top whenever the search changes:Keyed on
[query, mode]so it fires for query changes, category changes, and mode switches (tag / semantic / people). It is a no-op if the container is not present. No changes to search logic, pagination, or caching.SearchResults.test.tsx: added a regression test assertingscrollTo({ top: 0 })is called on a new search.Verification (run locally on latest
main)npm test -- SearchResults-> 14/14 pass (incl. the new regression test)npx jest allPages + PageSanity + Navbar-> 20/20 passnpx tsc --noEmit-> cleannpx eslint --max-warnings 0on the touched files -> cleannpx prettier --checkon the touched files -> cleanAI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: ChatGPT
Checklist
Summary by CodeRabbit
Bug Fixes