Add query string integration for the search - #4628
Conversation
📝 WalkthroughWalkthroughThe v7 and v8 search views now synchronize search state with the ChangesSearch route synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The search URL integration can leave users seeing stale results or lose the active query when changing album scope, and mounted searches may not update when the URL changes. These are concrete correctness issues that should be fixed before merging. Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02ca59e5-9369-4ab4-b96c-9b2a122b19a4
📒 Files selected for processing (3)
resources/js/composables/photo/photoRoute.tsresources/js/v7/views/gallery-panels/Search.vueresources/js/v8/views/gallery-panels/Search.vue
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.
| function onClear() { | ||
| searchStore.clear(); | ||
| searchStore.searchTerm = undefined; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Invalidate pending searches before clearing in both search views.
Both handlers clear local state without invalidating an active SearchState.search() request. A late response can restore results after q and searchTerm were removed.
resources/js/v7/views/gallery-panels/Search.vue#L218-L220: invalidate or cancel the active request before clearing.resources/js/v8/views/gallery-panels/Search.vue#L218-L220: apply the same request invalidation.
📍 Affects 2 files
resources/js/v7/views/gallery-panels/Search.vue#L218-L220(this comment)resources/js/v8/views/gallery-panels/Search.vue#L218-L220
| if (typeof route.query.q === "string" && route.query.q.length > 0) { | ||
| // Bookmarked/shared URL: run the search that the query string encodes. | ||
| onSearch(route.query.q); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
for file in resources/js/v7/views/gallery-panels/Search.vue resources/js/v8/views/gallery-panels/Search.vue; do
echo "===== $file: outline and relevant symbols ====="
ast-grep outline "$file" 2>/dev/null | sed -n '1,220p'
echo "===== $file: route/search lifecycle references ====="
rg -n -C 8 'route\.query|useRoute|onMounted|watch|onSearch|clear|searchTerm|router\.' "$file"
doneRepository: LycheeOrg/Lychee
Length of output: 20041
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== Search.vue watcher tails ====="
for file in resources/js/v7/views/gallery-panels/Search.vue resources/js/v8/views/gallery-panels/Search.vue; do
echo "--- $file ---"
sed -n '580,700p' "$file"
done
echo "===== SearchState store ====="
search_state=$(fd -i -t f 'SearchState' . | head -n 1)
printf '%s\n' "$search_state"
sed -n '1,280p' "$search_state"
echo "===== search route definitions and query navigation ====="
rg -n -C 10 'name:\s*["'\'']search["'\'']|path:.*search|query:\s*\{[^}]*q|query\.q' resources/js routes tests 2>/dev/null | sed -n '1,320p'
echo "===== Search view tests/references ====="
rg -n -C 8 'gallery-panels/Search|query.*q|SearchState|onClear|clearScope' tests resources/js --glob '*.{ts,tsx,js,jsx,vue}' 2>/dev/null | sed -n '1,360p'Repository: LycheeOrg/Lychee
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("resources/js/v7/views/gallery-panels/Search.vue"),
Path("resources/js/v8/views/gallery-panels/Search.vue"),
]
route_file = Path("resources/js/router/paths.ts")
route_text = route_file.read_text()
route_match = re.search(
r'name:\s*"search",\s*\n\s*path:\s*"([^"]+)",\s*\n\s*props:\s*true',
route_text,
)
assert route_match, "search route definition not found"
print(f"search route: name=search path={route_match.group(1)} props=true")
for path in files:
text = path.read_text()
mounted = re.search(
r'onMounted\(async \(\) => \{(?P<body>.*?)\n\}\);',
text,
re.S,
)
assert mounted, f"{path}: onMounted block not found"
body = mounted.group("body")
q_mount_calls_search = bool(
re.search(
r'if\s*\(\s*typeof route\.query\.q === "string"'
r'.*?onSearch\(route\.query\.q\)',
body,
re.S,
)
)
q_watchers = re.findall(
r'watch\s*\(\s*\(\)\s*=>\s*route\.query\.q',
text,
)
query_read_count = len(re.findall(r'route\.query\.q', text))
param_watchers = re.findall(
r'watch\s*\(\s*\(\)\s*=>\s*route\.params\.[A-Za-z0-9_]+',
text,
)
assert q_mount_calls_search, f"{path}: mount-time q search was not found"
assert not q_watchers, f"{path}: unexpected q watcher found"
print(
f"{path}: mount_q_search=True, q_watchers={len(q_watchers)}, "
f"route_query_q_reads={query_read_count}, param_watchers={param_watchers}"
)
# Model the relevant lifecycle transition without importing repository code:
# the same named route/path keeps the mounted Search component, so only
# onMounted and the existing param watcher can react to a query-only change.
states = [
("mounted", "q=alpha", "onMounted runs and calls onSearch(alpha)"),
("same Search route", "q=beta", "no onMounted call and no q watcher"),
("same Search route", "q absent", "no onMounted call and no q watcher"),
]
print("query-only transition model:")
for state in states:
print(f" {state[0]}: {state[1]} -> {state[2]}")
PYRepository: LycheeOrg/Lychee
Length of output: 776
Synchronize q after mount in both search views.
When route.query.q changes on the mounted search route, neither view starts the new search or clears stale results. Add a watcher in both files that searches for a non-empty q and clears the store when q is absent or empty.
📍 Affects 2 files
resources/js/v7/views/gallery-panels/Search.vue#L562-L566(this comment)resources/js/v8/views/gallery-panels/Search.vue#L575-L578
| if (route.query.q !== terms) { | ||
| router.replace({ query: { ...route.query, q: terms } }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '205,276p' resources/js/v8/views/gallery-panels/Search.vueRepository: LycheeOrg/Lychee
Length of output: 2591
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Search.vue route/search references ---'
rg -n -C 5 'clearScope|onSearch|searchTerm|router\.replace|name:\s*"search"' resources/js/v8/views/gallery-panels/Search.vue
printf '%s\n' '--- Search route definitions and navigation helpers ---'
rg -n -C 4 'name:\s*["'\"']search|path:.*search|albumId.*photoId|usePhotoRoute' resources/js resources -g '*.ts' -g '*.js' -g '*.vue' | head -240
printf '%s\n' '--- Search store implementation and usages ---'
rg -n -C 5 'searchTerm|function search|search\(' resources/js/v8 -g '*.ts' -g '*.vue' | head -300Repository: LycheeOrg/Lychee
Length of output: 34191
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Candidate router files ---'
fd -t f -e ts -e js resources/js | rg -i 'router|route'
printf '%s\n' '--- SearchState implementation ---'
fd -t f resources/js | rg 'SearchState'
for f in $(fd -t f resources/js | rg 'SearchState'); do
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f"
done
printf '%s\n' '--- SearchState source ---'
f="$(fd -t f resources/js | rg 'SearchState' | head -n 1)"
sed -n '1,260p' "$f"
printf '%s\n' '--- Router construction and search route ---'
for f in $(fd -t f -e ts -e js resources/js | rg -i 'router|route'); do
rg -n -C 5 'createRouter|createWebHistory|name:\s*["'\"']search|path:.*search' "$f" || true
doneRepository: LycheeOrg/Lychee
Length of output: 539
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Candidate router files ---'
fd -t f -e ts -e js . resources/js | rg -i 'router|route' || true
printf '%s\n' '--- SearchState files ---'
fd -t f . resources/js | rg 'SearchState' || true
printf '%s\n' '--- Router and SearchState references ---'
rg -n -C 5 'createRouter|createWebHistory|name:\s*["'\"']search|path:.*search|searchTerm|function search' resources/js -g '*.ts' -g '*.js' | head -400Repository: LycheeOrg/Lychee
Length of output: 8426
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- SearchState actions ---'
sed -n '1,125p' resources/js/stores/SearchState.ts
printf '%s\n' '--- v8 route assembly ---'
sed -n '1,180p' resources/js/v8/router/routes.ts
sed -n '1,115p' resources/js/router/paths.ts
printf '%s\n' '--- Vue Router dependency versions ---'
rg -n '"vue-router"|"vue":' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -40 || true
printf '%s\n' '--- All clearScope implementations ---'
rg -n -C 12 'function clearScope|clear-scope' resources/js/v8 resources/js/composablesRepository: LycheeOrg/Lychee
Length of output: 21862
Preserve q when clearing the album scope. clearScope() starts a q-less router.replace() and then calls onSearch() synchronously. When route.query.q already equals terms, onSearch() skips its replacement, so the navigation can remove q. Chain the navigation with .then(), or include q in the same navigation while preserving other query parameters.
Source: Coding guidelines
1 participant
Summary by CodeRabbit