feat: fullscreen search modal + back button for mobile#303
Open
Ante042 wants to merge 7 commits into
Open
Conversation
Replaces the inline search input in the mobile navbar with a fullscreen search modal overlay. This solves several mobile UX issues: 1. Keyboard no longer closes when navigating between search and results 2. Search query and results persist across navigation (tap result → view details → go back → modal reopens with previous search intact) 3. Modal stays visible until the destination page has loaded (no flash of the home page during navigation) Also adds a back button (chevron-left) in the top-left corner on non-main pages for easy navigation on mobile devices. New file: src/lib/components/search-modal.svelte - Fullscreen overlay with auto-focusing input - 300ms debounced search via TMDB API (movies + TV merged by popularity) - Results displayed as portrait cards in a 2-column grid - Escape/X clears state, tapping a result preserves it for back-navigation - Smooth transitions (fly + fade) with instant skip during navigation Modified: src/lib/components/mobile-nav.svelte - Search input replaced with tap-to-open button that triggers the modal - Back button with glass-morphism style, hidden on main pages
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced the inline mobile search input with a modal-driven search (new Changes
Sequence DiagramsequenceDiagram
actor User
participant MobileNav
participant SearchModal
participant TMDB_API
participant Router
User->>MobileNav: Tap search trigger
MobileNav->>SearchModal: open()
activate SearchModal
SearchModal->>User: Render & focus input
User->>SearchModal: Type query
Note over SearchModal: 300ms debounce, abort previous requests
SearchModal->>TMDB_API: Fetch /api/tmdb/search/movie
SearchModal->>TMDB_API: Fetch /api/tmdb/search/tv
TMDB_API-->>SearchModal: Movie results
TMDB_API-->>SearchModal: TV results
SearchModal->>SearchModal: Merge & sort by popularity
SearchModal->>User: Display results
User->>SearchModal: Select result
SearchModal->>Router: goto("/details/media/{id}/{media_type}")
Router-->>SearchModal: afterNavigate
SearchModal->>SearchModal: close()
deactivate SearchModal
User->>Router: Browser back (popstate)
Router->>SearchModal: popstate handler
activate SearchModal
SearchModal->>SearchModal: restore saved query/results
SearchModal->>User: reopen modal with results
deactivate SearchModal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/mobile-nav.svelte`:
- Around line 24-32: Replace the inline onclick that calls history.back() with a
dedicated handler (e.g., handleBack) that uses SvelteKit navigation as a safe
fallback: check if history.length > 1 and call history.back(), otherwise call
SvelteKit's goto(...) to navigate to a safe route (like the main page); update
the button to call handleBack and import goto from $app/navigation so the back
action is predictable for users who landed directly on the page (refer to
isMainPage, the onclick handler, and the ChevronLeft button in
mobile-nav.svelte).
In `@src/lib/components/search-modal.svelte`:
- Around line 27-28: The debounceTimer variable can fire after the component is
destroyed; add cleanup in the component's teardown by using Svelte's onDestroy
(or a $: effect cleanup) to clearTimeout(debounceTimer) if debounceTimer is not
null and set it to null; also abort any in-flight request by calling
abortController?.abort() and nulling abortController to fully clean up resources
(reference debounceTimer and abortController in search-modal.svelte).
- Around line 186-190: ESLint flags the unused "_" in the skeleton loop; replace
the array so the loop only binds the index: generate the indexes first (e.g.
Array.from({length: 8}, (_, i) => i)) and change the each block to iterate "as i
(i)" and render <PortraitCardSkeleton /> per index, so there is no unused
variable (reference the each block in search-modal.svelte and the
PortraitCardSkeleton component).
- Around line 98-105: The goto call in handleResultClick constructs a dynamic
route string directly, which triggers svelte/no-navigation-without-resolve;
import resolve from $app/paths and use it to build the path (e.g.,
resolve(`/details/media/${item.id}/${item.media_type}`)) before passing it to
goto, keeping the existing
savedQuery/savedResults/navigatedFromModal/pendingNavigation assignments and
behavior unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: dc9249ef-7e1d-4d50-a30d-670f7a0d813e
📒 Files selected for processing (2)
src/lib/components/mobile-nav.sveltesrc/lib/components/search-modal.svelte
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/components/search-modal.svelte (1)
105-112:⚠️ Potential issue | 🟠 MajorResolve the details route before calling
goto().Line 111 still builds the pathname manually, so the
svelte/no-navigation-without-resolveerror remains unresolved.♻️ Proposed fix
- import { goto, afterNavigate } from "$app/navigation"; + import { goto, afterNavigate } from "$app/navigation"; + import { resolve } from "$app/paths"; @@ - goto(`/details/media/${item.id}/${item.media_type}`); + goto(resolve(`/details/media/${item.id}/${item.media_type}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/search-modal.svelte` around lines 105 - 112, The code constructs the details URL manually in handleResultClick and calls goto directly, triggering svelte/no-navigation-without-resolve; update handleResultClick to call resolve on the pathname and pass that result into goto (i.e., import resolve from '$app/navigation' if not already), e.g., build the path as before but call goto(resolve(`/details/media/${item.id}/${item.media_type}`)); keep the savedQuery/savedResults/navigatedFromModal/pendingNavigation assignments unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/mobile-nav.svelte`:
- Around line 25-34: The onclick handler for the back/fallback button uses
goto('/') without resolving the app base path; update the handler to call
resolve('/') and pass its result to goto instead. Import resolve from
"$app/paths" at the top alongside any existing imports, and in the onclick
anonymous function inside the {`#if` !isMainPage} block replace the fallback
branch goto('/') with goto(resolve('/')) so internal routing respects the app
base path.
In `@src/lib/components/search-modal.svelte`:
- Around line 55-58: The debounce logic in handleInput and the modal-close path
doesn't cancel queued timeouts or in-flight searches, allowing stale search
results to repopulate results after the modal is cleared; update the search flow
to track and cancel both the debounce timer (debounceTimer) and an
AbortController (e.g., searchAbortController) before scheduling or starting a
new search, pass searchAbortController.signal into the fetch used by search, and
in the modal close/clear logic reset results and call
clearTimeout(debounceTimer) and searchAbortController.abort() (then recreate
controller before the next search) so queued and in-flight requests are aborted
and cannot overwrite cleared state.
- Around line 29-35: There is a duplicate declaration of the abortController
variable; remove the second `let abortController: AbortController | null =
null;` so only the single declaration (used by onDestroy) remains, ensuring
`abortController` is declared once at top of the script and the onDestroy block
continues to call `abortController?.abort()` without redeclaring it.
---
Duplicate comments:
In `@src/lib/components/search-modal.svelte`:
- Around line 105-112: The code constructs the details URL manually in
handleResultClick and calls goto directly, triggering
svelte/no-navigation-without-resolve; update handleResultClick to call resolve
on the pathname and pass that result into goto (i.e., import resolve from
'$app/navigation' if not already), e.g., build the path as before but call
goto(resolve(`/details/media/${item.id}/${item.media_type}`)); keep the
savedQuery/savedResults/navigatedFromModal/pendingNavigation assignments
unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: eaac6158-872b-40a2-a2f4-a9a77f046741
📒 Files selected for processing (2)
src/lib/components/mobile-nav.sveltesrc/lib/components/search-modal.svelte
Collaborator
|
Can screenshots be provided please. I’ve been thinking the same issues as fixed here. Need the critical comment address first |
- Remove duplicate abortController declaration - Use resolve() from $app/paths for all goto() calls - Cancel debounce timer and abort in-flight requests on modal close Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/components/search-modal.svelte (1)
55-69:⚠️ Potential issue | 🟠 MajorAbort the previous request as soon as the query changes.
Right now the in-flight search is only canceled when the next debounced
search()starts. If the user types again or clears the input before that timer fires, the older request can still win the race and repopulateresultsfor a query that is no longer shown. This is the same stale-result path previously flagged around this debounce flow.Proposed fix
function handleInput() { - if (debounceTimer) clearTimeout(debounceTimer); + if (debounceTimer) clearTimeout(debounceTimer); + abortController?.abort(); + abortController = null; debounceTimer = setTimeout(search, 300); } async function search() { const q = query.trim(); if (!q) { + abortController?.abort(); + abortController = null; results = []; loading = false; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/search-modal.svelte` around lines 55 - 69, The in-flight search must be aborted immediately when the user changes the input so stale responses can't overwrite current results: inside handleInput() call abortController.abort() (and recreate a new AbortController if your flow requires one) before starting/setting the debounce timer so any prior fetch started in search() is canceled as soon as query changes; keep the existing abort logic in search() as a fallback. Target the handleInput and search functions and the abortController variable when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/mobile-nav.svelte`:
- Line 97: The SearchModal is mounted outside the md:hidden mobile navbar so if
opened on mobile then the viewport crosses the md breakpoint the fullscreen
overlay will still render on desktop; fix by gating mounting of SearchModal to
mobile only — either move the <SearchModal ... /> into the existing md:hidden
navbar container or add a desktop-guard around it (e.g., use an isMobile boolean
or a media-query hook and only render SearchModal when isMobile is true) and
keep the same props (searchModalOpen, onclose/onopen) so the mobile-only
behavior is enforced.
In `@src/lib/components/search-modal.svelte`:
- Around line 105-129: Save the page URL that launched the modal in
handleResultClick (e.g., set a new savedOrigin variable right before calling
goto in handleResultClick) and on afterNavigate only call reopenFromNav when the
navigation is a popstate that returns to that exact savedOrigin (in addition to
the existing checks for navigatedFromModal and savedQuery); clear savedOrigin
when you consume it (when reopening or when pendingNavigation flows complete) so
the modal won't reopen for unrelated later navigations.
---
Duplicate comments:
In `@src/lib/components/search-modal.svelte`:
- Around line 55-69: The in-flight search must be aborted immediately when the
user changes the input so stale responses can't overwrite current results:
inside handleInput() call abortController.abort() (and recreate a new
AbortController if your flow requires one) before starting/setting the debounce
timer so any prior fetch started in search() is canceled as soon as query
changes; keep the existing abort logic in search() as a fallback. Target the
handleInput and search functions and the abortController variable when making
this change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: eea26244-d2a6-4cd4-a045-a92e3c33f806
📒 Files selected for processing (2)
src/lib/components/mobile-nav.sveltesrc/lib/components/search-modal.svelte
…ck origin URL - Abort in-flight fetch immediately in handleInput before debounce - Wrap SearchModal in md:hidden to prevent desktop rendering - Track savedOrigin URL to only reopen modal on exact popstate match Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/mobile-nav.svelte`:
- Around line 19-21: MAIN_PAGES in mobile-nav.svelte is missing the "/auth"
route, causing isMainPage to be false on the Profile page; update the MAIN_PAGES
array used by isMainPage to include "/auth" so it matches sidebar.svelte's
navItems (keep MAIN_PAGES = ["/", "/explore", "/dashboard", "/library",
"/settings", "/calendar", "/logs", "/auth"] or equivalent) to prevent the back
button from appearing on Profile.
In `@src/lib/components/search-modal.svelte`:
- Around line 94-99: The merged array created from movies.results and tv.results
(variable merged of type TMDBTransformedListItem[] which is then assigned to
results) is unbounded and can produce 40+ items; limit the number to a
reasonable cap to avoid rendering overhead (e.g., define a MAX_RESULTS constant
and after sorting slice the merged array to merged.slice(0, MAX_RESULTS)) so
results only contains the top N items; update the assignment of results to use
the sliced, sorted merged array and keep references to movies.results and
tv.results unchanged.
- Around line 186-206: The close button in the search modal (the button using
the X icon and onclick={clearAndClose}) lacks an explicit type, which can cause
unintended form submissions if the component is later nested in a form; update
that button element to include type="button" to make its behavior explicit and
defensive while leaving onclick={clearAndClose} and styling unchanged.
- Around line 219-227: The buttons rendering search results (the button with
onclick={() => handleResultClick(item)} wrapping the PortraitCard) are missing
an explicit type attribute, which can cause them to act as form submitters; add
type="button" to that button to prevent unintended form submission (and likewise
ensure the modal close button has type="button" if it doesn't already). Update
the button element(s) so the result-click handler (handleResultClick) and the
PortraitCard usage remain unchanged, only adding the type attribute for
defensive coding.
- Around line 78-90: The current fetch handling in search-modal.svelte silently
substitutes { results: [] } when movieRes.ok or tvRes.ok is false, hiding API
errors; update the Promise.all resolution to detect non-ok responses for
movieRes and tvRes (using their status and response text/json), log or surface
the error (include response.status and body) and either throw or propagate the
error so it isn’t swallowed into movies/tv; adjust the code that consumes movies
and tv to handle thrown errors or empty results accordingly, referencing
movieRes, tvRes, movies, tv and the fetch calls to /api/tmdb/search/movie and
/api/tmdb/search/tv.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: be84c444-5494-4d03-b33b-20382124d751
📒 Files selected for processing (2)
src/lib/components/mobile-nav.sveltesrc/lib/components/search-modal.svelte
…ain pages - Limit merged search results to 20 items to avoid rendering overhead - Add type="button" to close and result buttons - Log non-ok TMDB API responses before fallback - Add /auth to MAIN_PAGES to prevent back button on Profile page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Author
|
Collaborator
|
looking good, is it possible to allow the modal to infinitely scroll the same way the main search view does instead of capping to 20 items - as you scroll it continues populating |
Replace the fixed 20-result cap with infinite scroll that loads the next page from TMDB when the user scrolls near the bottom. Deduplicates results across pages and shows skeleton loaders while fetching more. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
Done! Replaced the 20-result cap with infinite scroll — it now fetches the next page from TMDB when you scroll near the bottom. Results are deduplicated across pages and skeleton loaders show while loading more. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/search-modal.svelte`:
- Around line 61-66: The input handler cancels the debounce but doesn't reset
pagination, so if the user edits the query and scrolls before debounce fires the
subsequent search(false) can run with stale pagination; update handleInput (and
the other places calling search) to reset/clear pagination state and current
results when the query changes (e.g., reset page/index/nextCursor and clear
results or set a "freshQuery" flag) before scheduling or calling search, and
ensure search respects that reset flag to fetch from the first page instead of
appending to old results; reference handleInput, debounceTimer, abortController,
and the search(...) calls to locate and implement the change.
- Around line 70-78: The early-return branch that handles an empty query in
search-modal.svelte currently resets abortController, results, currentPage,
hasMorePages, currentQuery, and loading but forgets to reset loadingMore; add a
statement to set loadingMore = false in that same block (alongside
abortController = null and loading = false) so the "loading more" state cannot
remain stale after abort/clear operations.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ca552c80-f415-45ff-95dd-c2dc065945fc
📒 Files selected for processing (2)
src/lib/components/mobile-nav.sveltesrc/lib/components/search-modal.svelte
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR adds
Fullscreen search modal for mobile
Replaces the inline search input in the mobile navbar with a fullscreen modal overlay. This solves several UX issues on mobile:
How it works
afterNavigatewithpopstatedetection reopens the modal on backBack button on mobile
Adds a chevron-left button (top-left, glass-morphism style) on non-main pages. Hidden on main navigation pages (/, /explore, /dashboard, /library, /settings, /calendar, /logs).
Files
src/lib/components/search-modal.svelte— the fullscreen search overlaysrc/lib/components/mobile-nav.svelte— search trigger + back buttonScreenshots
Mobile-only changes. Desktop search is unaffected.
Summary by CodeRabbit
New Features
Refactor