Spotlight search - #4656
Conversation
|
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:
📝 WalkthroughWalkthroughChangesSpotlight application
JavaScript CI matrix
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds spotlight search and theme persistence, but rapid theme changes can leave the saved theme inconsistent with the visible state, while unresolved search, navigation, translation, and thumbnail lifecycle issues may produce stale results, incorrect loading behavior, broken keyboard navigation, missing labels, or leaked resources. These bounded correctness issues should be fixed or explicitly accepted before merge. 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 |
0cfea45 to
35e1062
Compare
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76b16a7c-d6fc-4ce1-a367-43b2eb461f4a
📒 Files selected for processing (25)
lang/ar/search-palette.phplang/bg/search-palette.phplang/cz/search-palette.phplang/de/search-palette.phplang/el/search-palette.phplang/en/search-palette.phplang/es/search-palette.phplang/fa/search-palette.phplang/fr/search-palette.phplang/hu/search-palette.phplang/it/search-palette.phplang/ja/search-palette.phplang/nl/search-palette.phplang/no/search-palette.phplang/pl/search-palette.phplang/pt/search-palette.phplang/ru/search-palette.phplang/sk/search-palette.phplang/sv/search-palette.phplang/tr/search-palette.phplang/vi/search-palette.phplang/zh_CN/search-palette.phplang/zh_TW/search-palette.phpresources/js/v8/components/modals/SpotlightSearch.vueresources/js/v8/views/App.vue
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
fa47a95 to
6ff0ee3
Compare
35e1062 to
aa90464
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 343a8dd0-74cb-4b73-839d-630105a2a4d3
📒 Files selected for processing (26)
lang/ar/search-palette.phplang/bg/search-palette.phplang/cz/search-palette.phplang/de/search-palette.phplang/el/search-palette.phplang/en/search-palette.phplang/es/search-palette.phplang/fa/search-palette.phplang/fr/search-palette.phplang/hu/search-palette.phplang/it/search-palette.phplang/ja/search-palette.phplang/nl/search-palette.phplang/no/search-palette.phplang/pl/search-palette.phplang/pt/search-palette.phplang/ru/search-palette.phplang/sk/search-palette.phplang/sv/search-palette.phplang/tr/search-palette.phplang/vi/search-palette.phplang/zh_CN/search-palette.phplang/zh_TW/search-palette.phpresources/js/app-v8.tsresources/js/v8/components/modals/SpotlightSearch.vueresources/js/v8/composables/useDarkMode.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lang/tr/search-palette.php
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
649ef26 to
7321e50
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
resources/js/v8/components/modals/SpotlightSearch.vue (1)
219-231: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild album breadcrumbs in one pass.
albumListStore.buildBreadcrumb(row.id)scans every row to find the target and then filters all rows for ancestors. Calling it once per album makes this mapping O(n²). On instances with several thousand albums, the first palette open blocks the main thread while the computed evaluates.Precompute the breadcrumbs once from the sorted nested-set rows, for example by adding a
breadcrumbsgetter toAlbumListState.tsthat walks the rows with an ancestor stack and returns aMap<string, string>, then read from that map here.resources/js/v8/composables/useLanguageSwitcher.ts (1)
24-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle rejected asynchronous requests.
The language-switch and remote-search flows attach success/finalization handlers without a rejection path. A failed request can produce an unhandled promise rejection and leave the user without feedback. Add rejection handling for both flows, preserving the existing loading-state cleanup for the active search.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bef3bc3f-34dd-44c7-91d6-784505f1305c
📒 Files selected for processing (7)
resources/js/app-v8.tsresources/js/services/thumb-asset-service.tsresources/js/stores/AlbumListState.tsresources/js/v8/components/admin/AdminTileLink.vueresources/js/v8/components/modals/SpotlightSearch.vueresources/js/v8/composables/useLanguageSwitcher.tsresources/js/v8/i18n.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (2)
resources/js/v8/components/modals/SpotlightSearch.vue (1)
219-231: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild album breadcrumbs in one pass.
albumListStore.buildBreadcrumb(row.id)scans every row to find the target and then filters all rows for ancestors. Calling it once per album makes this mapping O(n²). On instances with several thousand albums, the first palette open blocks the main thread while the computed evaluates.Precompute the breadcrumbs once from the sorted nested-set rows, for example by adding a
breadcrumbsgetter toAlbumListState.tsthat walks the rows with an ancestor stack and returns aMap<string, string>, then read from that map here.resources/js/v8/composables/useLanguageSwitcher.ts (1)
24-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle rejected asynchronous requests.
The language-switch and remote-search flows attach success/finalization handlers without a rejection path. A failed request can produce an unhandled promise rejection and leave the user without feedback. Add rejection handling for both flows, preserving the existing loading-state cleanup for the active search.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bef3bc3f-34dd-44c7-91d6-784505f1305c
📒 Files selected for processing (7)
resources/js/app-v8.tsresources/js/services/thumb-asset-service.tsresources/js/stores/AlbumListState.tsresources/js/v8/components/admin/AdminTileLink.vueresources/js/v8/components/modals/SpotlightSearch.vueresources/js/v8/composables/useLanguageSwitcher.tsresources/js/v8/i18n.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
🛑 Comments failed to post (2)
resources/js/services/thumb-asset-service.ts (1)
108-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Increment
refCountbeforeenforceCacheLimit().The new entry is inserted with
refCount: 0, andenforceCacheLimit()runs before Line 113 increments it. If the cache exceedsMAX_CACHE_SIZEand every older entry is still referenced, the loop reaches the newest entry and evicts it while its request is in flight.The caller still holds the promise, so the
thenhandler creates an object URL that no cache entry owns.release()then fails thecache.get(key) === acquiredcheck, so the URL is never revoked and the request is never aborted.Claim the reference before enforcing the limit.
🐛 Proposed fix
entry = { promise, controller, refCount: 0, settled: false, expiresAt: undefined }; + entry.refCount++; cache.set(key, entry); enforceCacheLimit(); + } else { + entry.refCount++; } - entry.refCount++; const acquired = entry;Note: this restructuring also needs the recency bump in the
entry !== undefinedbranch to stay ahead of the increment, so keep Lines 79-85 as they are.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.entry = { promise, controller, refCount: 0, settled: false, expiresAt: undefined }; entry.refCount++; cache.set(key, entry); enforceCacheLimit(); } else { entry.refCount++; }resources/js/v8/components/admin/AdminTileLink.vue (1)
9-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' resources/js/v8/components/admin/AdminTileLink.vue printf '\n--- references ---\n' rg -n "AdminTileLink|`@keydown`\.enter|function navigate|const navigate" resources tests 2>/dev/null | head -200Repository: LycheeOrg/Lychee
Length of output: 4862
🏁 Script executed:
printf '%s\n' '--- git diff/stat ---' git diff --stat -- resources/js/v8/components/admin/AdminTileLink.vue git diff -- resources/js/v8/components/admin/AdminTileLink.vue | sed -n '1,180p' printf '%s\n' '--- related admin components ---' sed -n '80,155p' resources/js/v8/views/admin/AdminDashboard.vue sed -n '220,275p' resources/js/v7/views/admin/AdminDashboard.vue printf '%s\n' '--- coding conventions ---' sed -n '1,220p' docs/specs/3-reference/coding-conventions.md 2>/dev/null | rg -n -C 3 "event|keyboard|Vue|function|handler|modifier"Repository: LycheeOrg/Lychee
Length of output: 6637
🌐 Web query:
HTML anchor keyboard activation Ctrl+Enter Meta+Enter keydown preventDefault browser behavior💡 Result:
For an HTML anchor element (
<a>) with a validhrefattribute, the browser provides native keyboard activation behavior [1]. When the element is focused, pressing the Enter key triggers the link's activation behavior (e.g., navigation) [2][1]. Regarding the specific key combinations and behavior: 1. Ctrl+Enter / Meta+Enter: In many modern browsers (such as Chrome), pressing Ctrl+Enter (or Command+Enter on macOS) while a link is focused will trigger the link's activation in a new tab [3]. This is a simulated browser-level behavior where the browser effectively performs a "new tab" click action associated with the link [3]. 2.keydownandpreventDefault: When the Enter key is pressed on a focused anchor, the browser internally dispatches akeydownevent followed by a simulatedclickevent [2][4]. If you attach akeydownevent listener to the anchor and callevent.preventDefault()when the Enter key is detected, you can successfully suppress the browser's default navigation or simulated "new tab" behavior [5][6]. 3. Inconsistency: It is important to note that while Enter is natively supported for anchor elements with anhref, the behavior of other modifiers (like Shift+Enter, Alt+Enter, or Meta+Enter) can vary significantly across different browsers and operating systems [7][8]. Browser implementations of keyboard shortcuts are often influenced by platform-level conventions rather than a single, strictly enforced web standard [7][8]. In summary, you can usekeydownlisteners to intercept and prevent the default browser actions triggered by Enter or Ctrl+Enter on anchor tags, but rely on native browser behavior for consistent user experience whenever possible [7][6]. For custom interactive components that are not truly links (e.g., using<a>as a button), it is generally recommended to use a<button>element instead to avoid these complex accessibility and keyboard handling issues [1].Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/a
- 2: https://chromium.googlesource.com/chromium/src/+/b1b31c055dbf36a3fe44e8fe8c7f6617a2db0598/third_party/blink/renderer/core/html/html_anchor_element.cc
- 3: https://issues.chromium.org/issues/40401749
- 4: SiebeVE/user-event@8a34d0b
- 5: https://stackoverflow.com/questions/40453472/raise-anchor-ng-click-on-enter-keypress-for-accessibility-in-angular
- 6: https://accessibility.huit.harvard.edu/technique-keyboard-operable-custom-controls
- 7: https://ericwbailey.design/published/basic-keyboard-shortcut-support-for-focused-links/
- 8: testing-library/user-event#589
🌐 Web query:
Vue 3 event modifiers prevent handler order@keydown.enter.preventimplementation💡 Result:
In Vue 3, the order of event modifiers matters because Vue generates the corresponding JavaScript code in the exact order the modifiers are written in the template [1][2][3]. When you use a chain like
@keydown.enter.prevent, Vue wraps your handler in a generated function that executes the modifiers from left to right before calling your actual event handler [4]. Specifically, for@keydown.enter.prevent: 1. The.enter modifier acts as a key filter; the generated code checks if the key event corresponds to the Enter key [1][4]. If it does not, the handler is not executed [4]. 2. The.prevent modifier executes event.preventDefault [1][4]. Because the order determines the execution sequence of the generated code, swapping them or adding other modifiers can change the final behavior [1][2]. For example, chaining.self (a conditional check) with.prevent (a side effect) yields different outcomes based on which is processed first:@click.prevent.selfprevents the default action regardless of the target, whereas@click.self.preventonly prevents it if the target matches the element [1][2]. For most use cases, such as@keydown.enter.prevent, the order is straightforward as they serve different purposes (filtering vs. preventing default). However, if you are chaining multiple side-effect modifiers (like.stop and.prevent) or conditional modifiers (like.self), always order them intentionally based on whether you want the action to occur before or after the condition check [2]. If you find the behavior of chained modifiers confusing or non-deterministic in complex scenarios, the recommended best practice is to move the logic into a standard JavaScript function, where you can explicitly control the execution order of event.stopPropagation, event.preventDefault, and your own application logic [1][2].Citations:
- 1: https://vuejs.org/guide/essentials/event-handling.html
- 2: https://vuewiki.dev/q/event-modifier-order
- 3: https://github.com/vuejs/docs/blob/5a1e8c27/src/guide/essentials/event-handling.md
- 4: https://github.com/vuejs/core/blob/main/packages/runtime-dom/src/directives/vOn.ts
🏁 Script executed:
python3 - <<'PY' from pathlib import Path import re p = Path("resources/js/v8/components/admin/AdminTileLink.vue") s = p.read_text() template = s.split("<script", 1)[0] script = s.split("<script", 1)[1] assert '`@keydown.enter.prevent`="navigate"' in template assert 'function navigate()' in script assert 'router.push(props.tile.to)' in script assert 'window.open(props.tile.to, "_blank", "noopener,noreferrer")' in script guards = [ "event.defaultPrevented", "event.metaKey", "event.ctrlKey", "event.shiftKey", "event.altKey", ] print("native href:", ':href="href"' in template) print("unconditional Enter prevention:", '`@keydown.enter.prevent`="navigate"' in template) print("internal navigation:", "router.push(props.tile.to)" in script) print("external navigation:", 'window.open(props.tile.to, "_blank", "noopener,noreferrer")' in script) print("suggested guard terms absent:", [g for g in guards if g not in script]) PYRepository: LycheeOrg/Lychee
Length of output: 294
Preserve modified keyboard activation.
@keydown.enter.preventcallsnavigate()for modified or already-prevented Enter events and suppresses the anchor’s native activation. HandleKeyboardEventexplicitly. When the event is already prevented or has any modifier key, return without routing. Otherwise, callpreventDefault()andnavigate().
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04ef1ec4-6037-4848-8fe3-dc6d96bf8998
📒 Files selected for processing (2)
.github/workflows/js_check.ymlresources/js/v8/composables/useDarkMode.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
|
@coderabbitai review please and approve if ready. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Diff stat ==='
git diff --stat HEAD^ HEAD
echo '=== Key file outlines ==='
for f in \
resources/js/v8/components/modals/SpotlightSearch.vue \
resources/js/v8/composables/useDarkMode.ts \
resources/js/v8/composables/useLanguageSwitcher.ts \
resources/js/v8/i18n.ts \
resources/js/app-v8.ts \
resources/js/v8/views/App.vue
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo '=== Key changed-file diffs ==='
git diff --unified=80 HEAD^ HEAD -- \
resources/js/v8/components/modals/SpotlightSearch.vue \
resources/js/v8/composables/useDarkMode.ts \
resources/js/v8/composables/useLanguageSwitcher.ts \
resources/js/v8/i18n.ts \
resources/js/app-v8.ts \
resources/js/v8/views/App.vue \
.github/workflows/js_check.yml
echo '=== Related implementations and call sites ==='
rg -n -C 4 \
'class SettingsService|SettingsService|function useAuth|useAuth\(|canEdit|search.*photo|Photo.*search|search.*Photo|loadLocale|availableLocales|resolveLocale|dark_mode|darkMode|localStorage' \
resources/js routes app lang \
-g '!resources/js/v8/components/modals/SpotlightSearch.vue' \
-g '!resources/js/v8/composables/useDarkMode.ts' \
-g '!resources/js/v8/composables/useLanguageSwitcher.ts' \
-g '!resources/js/v8/i18n.ts' || trueLength of output: 197456 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed non-translation files ==='
git diff --name-only HEAD^ HEAD | grep -v '^lang/' || true
echo '=== SpotlightSearch.vue ==='
cat -n resources/js/v8/components/modals/SpotlightSearch.vue
echo '=== Changed composables and i18n ==='
for f in \
resources/js/v8/composables/useDarkMode.ts \
resources/js/v8/composables/useLanguageSwitcher.ts \
resources/js/v8/i18n.ts \
resources/js/app-v8.ts \
resources/js/v8/views/App.vue \
resources/js/services/search-service.ts \
resources/js/services/settings-service.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo '=== Route definitions relevant to selected search results ==='
rg -n -C 5 'name:\s*["'\''](album|photo|search|settings)|path:.*(album|photo|settings)' resources/js/v8 resources/js -g '*.ts' || true
echo '=== User store authorization fields ==='
fd -i 'UserState*' resources/js -t f -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {} \;
echo '=== Translation generation and Vite config references ==='
rg -n -C 5 'php_.*json|lang/php_|search-palette|import\.meta\.glob|laravel-vue-i18n' \
vite.config.* package.json resources/js scripts app -g '!lang/**' || true
echo '=== Search API route and resource shapes ==='
rg -n -C 8 'SearchController|search\(|ResultsResource|class ResultsResource|Route::.*search|search/' \
routes app/Http/Controllers/Gallery/SearchController.php app/Http/Resources/Search \
-g '*.php' || trueLength of output: 202008 🏁 Script executed: #!/bin/bash
set -euo pipefail
for f in \
resources/js/v8/components/modals/SpotlightSearch.vue \
resources/js/v8/composables/useDarkMode.ts \
resources/js/v8/composables/useLanguageSwitcher.ts \
resources/js/v8/i18n.ts \
resources/js/app-v8.ts \
resources/js/v8/views/App.vue \
resources/js/services/search-service.ts \
resources/js/services/settings-service.ts \
resources/js/stores/UserState.ts \
resources/js/routes.ts \
resources/js/router.ts \
vite.config.ts \
package.json \
app/Http/Resources/Search/ResultsResource.php
do
if [ -f "$f" ]; then
echo "===== $f ====="
cat -n "$f"
fi
done
echo '===== Exact changed files ====='
git diff --name-status HEAD^ HEAD
echo '===== Translation source and generated-file references ====='
rg -n -C 3 'php_.*\.json|php_\$\{|php_\$\{|\blang/php_|search-palette' \
vite.config.ts package.json resources/js/app-v8.ts resources/js/v8/i18n.ts \
resources/js/v8/composables/useLanguageSwitcher.ts || trueLength of output: 50643 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref=""
for candidate in origin/master master; do
if git rev-parse --verify --quiet "$candidate" >/dev/null; then
base_ref="$candidate"
break
fi
done
if [ -z "$base_ref" ]; then
echo "ERROR: No master reference is available for full pull request comparison."
exit 0
fi
merge_base="$(git merge-base "$base_ref" HEAD)"
echo "=== Base reference: $base_ref ==="
echo "=== Merge base: $merge_base ==="
echo "=== Full PR changed files ==="
git diff --name-status "$merge_base"...HEAD
echo "=== Full PR stat ==="
git diff --stat "$merge_base"...HEAD
echo "=== Full PR source diff, excluding translations ==="
git diff --unified=40 "$merge_base"...HEAD -- \
':(exclude)lang/**'
echo "=== v8 route definitions ==="
if [ -f resources/js/v8/router/routes.ts ]; then
cat -n resources/js/v8/router/routes.ts
fi
echo "=== Existing command shortcut usage ==="
rg -n -C 3 'defineShortcuts\(|meta_k|ctrl_k' resources/js/v8 -g '*.vue' -g '*.ts' || true
echo "=== GitHub PR checks ==="
gh pr checks 4656 --repo LycheeOrg/Lychee || trueLength of output: 46597
I cannot approve The completed TypeScript analysis, CodeQL, dependency review, and CodeRabbit checks passed. You are interacting with an AI system. |
Your opinion does not depend/wait on my CICD to complete. Approve if you think it is ready. |
|
I found no blocking issue in 🐇 ✅ ✏️ Learnings added
✅ Action performedComments resolved and changes approved. |
1 participant
Summary by CodeRabbit