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:
📝 WalkthroughWalkthroughReplaces plain-term search with a token-based system: adds SearchToken DTO and parser, per-modifier strategy contracts and implementations for photos and albums, request/controller and frontend token integration, colour-distance config migration, many new tests, and i18n strings for an Advanced Search UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/4-architecture/roadmap.md (1)
107-107:⚠️ Potential issue | 🟡 MinorUpdate the roadmap footer timestamp.
Line 107 still shows
*Last updated: 2026-03-03*even though this file was modified. Please refresh it to the actual update date.As per coding guidelines, "
**/*.md: At the bottom of documentation files, add an hr line followed by 'Last updated: [date of the update]'."
🧹 Nitpick comments (8)
lang/fa/gallery.php (1)
48-79: Translation values are English placeholders in a Persian locale file.The new
advancedsearch block adds 32 translation strings, but all values are in English rather than Persian. This follows an existing pattern in the file (e.g., lines 13-20, 87-96), so it's consistent, but these should ideally be translated to Persian for a complete localization.Examples of expected Persian translations based on existing patterns in this file:
'title'→'جستجوی پیشرفته'(similar to line 42)'description'→'توضیحات'(similar to line 181)'location'→'موقعیت'(similar to line 220)'tags'→'برچسبها'(similar to line 228)app/Actions/Search/Strategies/ColourStrategy.php (1)
80-84: Consider validating hex format before returning.The
resolveHexmethod accepts any string starting with#without validating that it's a valid 6-digit hex colour code. Malformed inputs like#xyzor#ffwill pass through and may cause issues inColour::fromHex().♻️ Proposed fix to add hex validation
private function resolveHex(string $value): string { if (str_starts_with($value, '#')) { + $hex = strtolower($value); + if (!preg_match('/^#[0-9a-f]{6}$/', $hex)) { + throw ValidationException::withMessages(['term' => "Invalid hex colour '{$value}'. Use format `#rrggbb`."]); + } - return strtolower($value); + return $hex; }resources/js/components/forms/basic/rating.vue (1)
29-34: Consider usingtoRefinstead of manual watch synchronization.The current pattern of copying props to local refs and using a watcher to sync them can be simplified with Vue's
toReffor reactive prop references, reducing boilerplate.♻️ Simplified approach using toRef
-import { ref, watch } from "vue"; +import { ref, toRef } from "vue"; const props = defineProps<{ loading: boolean; selectedRating: number | undefined; handleRatingClick: (rating: 1 | 2 | 3 | 4 | 5) => void; amber?: boolean; }>(); -const loading = ref(props.loading); +const isLoading = toRef(props, 'loading'); const hoverRating = ref<number | null>(null); -const selectedRating = ref(props.selectedRating); +const currentRating = toRef(props, 'selectedRating'); // ... handlers remain the same ... -watch( - () => [props.loading, props.selectedRating], - ([newLoading, newSelectedRating]) => { - loading.value = newLoading as boolean; - selectedRating.value = newSelectedRating as number | undefined; - }, -);resources/js/components/forms/search/SearchBox.vue (1)
94-94: Move import statement to the top of the script block.The
watchimport on line 94 should be grouped with other Vue imports at line 15 for consistency and readability.♻️ Proposed fix
-import { ref, onMounted, nextTick } from "vue"; +import { ref, onMounted, nextTick, watch } from "vue";Then remove line 94:
-import { watch } from "vue";app/Actions/Search/PhotoSearch.php (1)
95-117: Strategy registry is rebuilt on every search query.The
buildStrategyRegistry()method creates new strategy instances on each call. Since these strategies are stateless, consider caching them as a class property for minor performance improvement in high-traffic scenarios.♻️ Proposed optimization
class PhotoSearch { + /** `@var` array<string, PhotoSearchTokenStrategy>|null */ + private ?array $strategy_registry = null; + public function __construct( protected readonly ConfigManager $config_manager, protected PhotoQueryPolicy $photo_query_policy, ) { } // ... other methods ... private function buildStrategyRegistry(): array { + if ($this->strategy_registry !== null) { + return $this->strategy_registry; + } + - return [ + $this->strategy_registry = [ '' => new PlainTextStrategy(), // ... rest of strategies ]; + + return $this->strategy_registry; } }resources/js/components/forms/search/AdvancedSearchPanel.vue (2)
11-22: Inconsistent label positioning in FloatLabel components.In lines 11-14, the
<label>comes after<InputText>, but in lines 15-18 and 19-22, the<label>comes before<InputText>. While PrimeVue's FloatLabel may handle both, consistency improves maintainability.♻️ Suggested fix for consistency
<FloatLabel variant="on"> - <label class="text-xs font-medium text-muted-color">{{ $t("gallery.search.advanced.description") }}</label> <InputText v-model="description" `@update`:model-value="onFieldChange" /> + <label class="text-xs font-medium text-muted-color">{{ $t("gallery.search.advanced.description") }}</label> </FloatLabel> <FloatLabel variant="on"> - <label class="text-xs font-medium text-muted-color">{{ $t("gallery.search.advanced.location") }}</label> <InputText v-model="location" `@update`:model-value="onFieldChange" /> + <label class="text-xs font-medium text-muted-color">{{ $t("gallery.search.advanced.location") }}</label> </FloatLabel>
319-320: Specify radix parameter forparseInt.
parseIntwithout a radix can lead to unexpected results with certain string formats. Always pass10as the second argument for decimal parsing.♻️ Proposed fix
- ratingMin.value = advanced.ratingMin ? parseInt(advanced.ratingMin) : undefined; - ratingOwn.value = advanced.ratingOwn ? parseInt(advanced.ratingOwn) : undefined; + ratingMin.value = advanced.ratingMin ? parseInt(advanced.ratingMin, 10) : undefined; + ratingOwn.value = advanced.ratingOwn ? parseInt(advanced.ratingOwn, 10) : undefined;tests/Feature_v2/Search/PhotoSearchTest.php (1)
339-351: Consider guarding against nulltaken_at.Line 341 calls
$this->photo1->taken_at->format('Y-m-d')without checking iftaken_atis null. If the fixture changes, this could cause a null pointer exception.🛡️ Defensive check
public function testDateExactMatchesPhotoTakenOnDate(): void { + $this->assertNotNull($this->photo1->taken_at, 'Fixture photo1 must have taken_at set'); $date = $this->photo1->taken_at->format('Y-m-d');
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de5b245b-e5fa-4a79-b707-583c1397e5dd
📒 Files selected for processing (65)
app/Actions/Search/AlbumSearch.phpapp/Actions/Search/ColourNameMap.phpapp/Actions/Search/PhotoSearch.phpapp/Actions/Search/SearchTokenParser.phpapp/Actions/Search/Strategies/Album/AlbumDateStrategy.phpapp/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.phpapp/Actions/Search/Strategies/ColourStrategy.phpapp/Actions/Search/Strategies/DateStrategy.phpapp/Actions/Search/Strategies/FieldLikeStrategy.phpapp/Actions/Search/Strategies/PlainTextStrategy.phpapp/Actions/Search/Strategies/RatingStrategy.phpapp/Actions/Search/Strategies/RatioStrategy.phpapp/Actions/Search/Strategies/TagStrategy.phpapp/Actions/Search/Strategies/TypeStrategy.phpapp/Contracts/Http/Requests/HasSearchTokens.phpapp/Contracts/Search/AlbumSearchTokenStrategy.phpapp/Contracts/Search/PhotoSearchTokenStrategy.phpapp/DTO/Search/SearchToken.phpapp/Http/Controllers/Gallery/SearchController.phpapp/Http/Requests/Search/GetSearchRequest.phpapp/Http/Requests/Traits/HasSearchTokensTrait.phpdatabase/migrations/2026_03_12_000000_add_search_colour_distance_config.phpdocs/specs/4-architecture/features/027-search-refactoring/plan.mddocs/specs/4-architecture/features/027-search-refactoring/spec.mddocs/specs/4-architecture/features/027-search-refactoring/tasks.mddocs/specs/4-architecture/features/028-search-ui-refactor/plan.mddocs/specs/4-architecture/features/028-search-ui-refactor/spec.mddocs/specs/4-architecture/features/028-search-ui-refactor/tasks.mddocs/specs/4-architecture/open-questions.mddocs/specs/4-architecture/roadmap.mdlang/ar/gallery.phplang/bg/gallery.phplang/cz/gallery.phplang/de/gallery.phplang/el/gallery.phplang/en/gallery.phplang/es/gallery.phplang/fa/gallery.phplang/fr/gallery.phplang/hu/gallery.phplang/it/gallery.phplang/ja/gallery.phplang/nl/gallery.phplang/no/gallery.phplang/pl/gallery.phplang/pt/gallery.phplang/ru/gallery.phplang/sk/gallery.phplang/sv/gallery.phplang/vi/gallery.phplang/zh_CN/gallery.phplang/zh_TW/gallery.phpresources/js/components/forms/basic/rating.vueresources/js/components/forms/search/AdvancedSearchPanel.vueresources/js/components/forms/search/SearchBox.vueresources/js/components/forms/search/SearchInputBar.vueresources/js/components/gallery/photoModule/PhotoRatingOverlay.vueresources/js/components/gallery/photoModule/PhotoRatingWidget.vueresources/js/components/gallery/searchModule/SearchPanel.vueresources/js/composables/photo/useRating.tsresources/js/composables/useSearchTokenAssembler.tsresources/js/views/gallery-panels/Search.vuetests/Feature_v2/Search/AlbumSearchTest.phptests/Feature_v2/Search/PhotoSearchTest.phptests/Unit/Actions/Search/SearchTokenParserTest.php
💤 Files with no reviewable changes (1)
- resources/js/composables/photo/useRating.ts
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 183639e4-e9a3-48c5-bf77-1ce744522bb4
📒 Files selected for processing (3)
app/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.phpapp/Actions/Search/Strategies/PlainTextStrategy.phpresources/js/composables/useSearchTokenAssembler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Actions/Search/Strategies/PlainTextStrategy.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
resources/js/composables/useSearchTokenAssembler.ts (1)
69-72:⚠️ Potential issue | 🟠 MajorHandle embedded double quotes before assembling tokens.
assembleStringTokenstill produces malformed tokens when a value contains", which can break the token grammar and backend parsing. This concern was already raised earlier and appears unresolved in this revision.🐛 Proposed fix
function assembleStringToken(modifier: string, value: string): string { - const token = `${modifier}:${value}`; - return value.includes(" ") ? `"${token}"` : token; + // Keep token grammar valid: raw double quotes terminate quoted tokens. + const sanitized = value.replace(/"/g, ""); + const token = `${modifier}:${sanitized}`; + return /\s/.test(sanitized) ? `"${token}"` : token; }
🧹 Nitpick comments (1)
app/Actions/Search/AlbumSearch.php (1)
107-141: Consider reusing the strategy registry instead of rebuilding it per call.
buildAlbumStrategyRegistry()is invoked on eachaddSearchCondition()call; caching it on the class would reduce repeated allocations.♻️ Optional refactor
class AlbumSearch { + /** `@var` array<string, AlbumSearchTokenStrategy> */ + private array $album_strategy_registry; + public function __construct( protected AlbumQueryPolicy $album_query_policy, ) { + $this->album_strategy_registry = $this->buildAlbumStrategyRegistry(); } @@ private function addSearchCondition(array $tokens, AlbumBuilder|TagAlbumBuilder|FixedQueryBuilder $query): void { - $strategies = $this->buildAlbumStrategyRegistry(); + $strategies = $this->album_strategy_registry; $applied = false;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8df85bd2-e23f-4452-9380-59ee00f75f5c
📒 Files selected for processing (4)
app/Actions/Search/AlbumSearch.phpapp/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.phpresources/js/composables/useSearchTokenAssembler.tstests/Feature_v2/Search/AlbumSearchTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Feature_v2/Search/AlbumSearchTest.php
- app/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05fdb3fb-84d3-406d-867c-7d6e8443d180
📒 Files selected for processing (1)
app/Actions/Search/Strategies/DateStrategy.php
1 participant
Summary by CodeRabbit
New Features
Internationalization
Bug Fixes
Documentation
Tests