Conversation
Treat the first resolved domain as canonical (keeps the `hub.` name prefix); secondary-domain registrations get routed through a prefixSecondaryDomainRoutes() helper that renames only newly-added routes with a domain-qualified slug prefix. Clears the remaining 25 hub.* collisions that originate in this vendored package — the pattern mirrors what already landed for app.*, api.docs.*, lthn.*, mcp.* in lthn.ai (commit 0de9ad9). Closes tasks.lthn.sh/view.php?id=88 Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
…er + 2 providers) (#855) - SearchProvider interface (search/getLabel/getPriority) - SearchResult DTO (title/subtitle/url/icon/category/score) — readonly with legacy registry compatibility preserved - SearchDispatcher — register providers, gather, sort by score desc - UserSearchProvider, WorkspaceSearchProvider as built-ins - Pest Feature tests _Good/_Bad/_Ugly per AX-10 - pint/pest skipped (vendor binaries missing in sandbox) Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=855
…act (#854) - AdminMenuProvider interface (getMenuItems/getMenuGroups/getPriority) - MenuItem + MenuGroup readonly DTOs - AdminMenuRegistry: provider registration, RFC default groups, custom group merging, sorted grouped resolution, flat items() - src/Boot.php registers the registry singleton - Pest Feature tests _Good/_Bad/_Ugly per AX-10 (replaces legacy menu-system tests that tested the prior implementation) - pint/pest skipped (vendor binaries missing in sandbox) Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=854
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new admin menu subsystem (provider interface, registry, MenuGroup/MenuItem models, and service-provider binding) and a search subsystem (provider interface, dispatcher, SearchResult refactor, concrete user/workspace providers, and LIKE-term helper). Also changes Hub route handling to prefix secondary-domain route names and updates many Blade/views to use new hubRoute helpers; adds tests and build/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Registry as AdminMenuRegistry
participant Provider as AdminMenuProvider[]
participant Group as MenuGroup[]
participant Item as MenuItem[]
Client->>Registry: request resolved menu
Registry->>Provider: iterate registered providers (by priority+index)
Provider-->>Registry: return getMenuGroups(), getMenuItems()
Registry->>Group: normalise/merge provider groups with defaults
Group-->>Registry: MenuGroup instances
Registry->>Item: normalise items, ensure group exists
Item-->>Registry: MenuItem instances
Registry->>Registry: sort by composite key (groupPrio,itemPrio,providerPrio,indices,label)
Registry-->>Client: grouped, deterministically-ordered menu structure
sequenceDiagram
actor User
participant Dispatcher as SearchDispatcher
participant Provider as SearchProvider[]
participant DB as Database/Eloquent
participant Results as SearchResult[]
User->>Dispatcher: search(query)
alt query blank
Dispatcher-->>User: return []
else
Dispatcher->>Provider: call search(query) (try/catch per provider)
loop per provider
Provider->>DB: perform LIKE queries (escaped)
DB-->>Provider: model rows
Provider-->>Dispatcher: mapped SearchResult[] (with score)
end
Dispatcher->>Results: annotate results with provider priority + index
Results->>Results: sort by score desc, provider priority desc, index asc
Results-->>Dispatcher: ordered results
Dispatcher-->>User: return ordered SearchResult[]
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/Website/Hub/Boot.php (1)
135-138: Use a collision-safe domain prefix for route namesLine 137 maps both
.and-to_, which is not injective (e.g.a-b.canda.b-cboth collapse toa_b_c). That can reintroduce secondary-route name collisions.Proposed hardening diff
private static function domainRoutePrefix(string $domain): string { - return strtr($domain, ['.' => '_', '-' => '_']).'.'; + $normalised = strtolower($domain); + $slug = preg_replace('/[^a-z0-9]+/', '_', $normalised) ?? 'domain'; + $hash = substr(hash('sha256', $normalised), 0, 8); + + return $slug.'_'.$hash.'.'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Website/Hub/Boot.php` around lines 135 - 138, The current domainRoutePrefix in function domainRoutePrefix maps both '.' and '-' to '_' and is not injective; change it to produce an injective, collision-safe prefix (e.g. replace the simple char mapping with a reversible encoding of the domain such as a URL-safe base64 or hex encoding) so different domains always produce different prefixes; update domainRoutePrefix to encode the full $domain with that reversible encoding and then append the trailing dot as before (use the domainRoutePrefix function name to locate the code and replace the strtr logic).src/Menu/AdminMenuProvider.php (1)
22-34: Sync the phpdoc with the supported payload shapes.
AdminMenuRegistryand the new tests accept raw array items/groups and lazy wrappers, but this contract only advertisesMenuItem[]/MenuGroup[]. Either narrow the registry or widen the phpdoc here so module authors do not code against the wrong public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Menu/AdminMenuProvider.php` around lines 22 - 34, The phpdoc for getMenuItems() and getMenuGroups() is too narrow: update their `@return` annotations to reflect that implementations may return concrete MenuItem/MenuGroup instances, raw array payloads, or lazy wrappers/callables accepted by AdminMenuRegistry and the tests; e.g. change `@return` array<int, MenuItem> to `@return` array<int, MenuItem|array|callable> for getMenuItems() and similarly change `@return` array<int, MenuGroup> to `@return` array<int, MenuGroup|array|callable> for getMenuGroups() so module authors see the actual supported payload shapes.src/Search/SearchResult.php (1)
117-145: Minor: Consider edge case where legacy subtitle is purely numeric.The heuristic
is_numeric($arguments[5])distinguishes between the new DTO shape (where position 5 isscore) and the legacy shape (where position 5 issubtitle). This works correctly for typical subtitles like email addresses, but a purely numeric subtitle (e.g.,'12345') would be misinterpreted as a score.Given that subtitles are typically descriptive strings, this is unlikely to occur in practice. However, if stricter detection is desired, you could check additional positions (e.g., whether
$arguments[4]looks like an icon class vs. a category string).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/SearchResult.php` around lines 117 - 145, The is_numeric check in normaliseConstructorArguments misclassifies a purely numeric legacy subtitle at $arguments[5] as a score; update the conditional that detects the new DTO shape so it requires both that $arguments[5] is numeric AND that $arguments[4] also looks like the new-shape category (e.g. validate $arguments[4] is a non-empty category-like string and not an icon/class token), or otherwise treat it as the legacy shape — change the if that inspects $arguments[5] to include this extra $arguments[4] validation to avoid misinterpreting numeric subtitles.
🤖 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/Menu/MenuGroup.php`:
- Around line 23-50: The constructor and fromArray currently only trim for
validation but store $key and $label with original whitespace; update the code
so the stored properties are trimmed: in the __construct (or just before
assignment) ensure $this->key and $this->label are the trimmed values (e.g. use
trim($key) and trim($label)), and adjust fromArray to pass trimmed values
(compute $key = trim((string)($data['key'] ?? '')) and label =
trim((string)($data['label'] ?? self::labelFromKey($key)))) so labelFromKey
receives the cleaned key and whitespace-only inputs are normalized.
In `@src/Search/Providers/UserSearchProvider.php`:
- Around line 44-52: The current query applies limit($this->limit) before
computing scores, which can drop better matches; remove the DB-level limit and
instead fetch all matching models (or a safely large cap), map them via
resultFor($user, $query) to SearchResult objects, then sort the resulting array
by the SearchResult->score() (or the comparator used downstream) and finally
slice the top $this->limit items before returning; update both occurrences
around the query (the shown block and the similar block at lines ~86-100) to use
this fetch-then-rank-then-limit approach so ranking is applied to the full
candidate set.
- Around line 44-48: The code in UserSearchProvider that escapes '%' and '_' by
prefixing with backslashes is not portable across SQL drivers; instead, build a
properly escaped search pattern and use an explicit ESCAPE clause so all
backends treat your escape character consistently. Concretely: when constructing
$term, replace '%' and '_' with an escaped form using a chosen escape char (e.g.
backslash), then use a raw where with parameter binding like "name LIKE ? ESCAPE
'\\'" (and same for email) so you pass the escaped pattern as a bound parameter;
update the closure in the query (the Builder lambda in UserSearchProvider) and
the similar block referenced at lines 81-84 to use this ESCAPE-aware where
clause for both name and email checks.
In `@src/Search/Providers/WorkspaceSearchProvider.php`:
- Around line 44-52: The query currently calls limit() before applying scoring,
so the database trims results prior to ranking; update the search flow in
WorkspaceSearchProvider so you apply score() (or otherwise compute/order by the
relevance score) before calling limit(), then call limit($this->limit) and only
after that ->get()->map(fn(Model $workspace) => $this->resultFor($workspace,
$query))->all(); ensure the same change is applied to the other occurrence that
mirrors lines 86-100 so resultFor is only run on the top-ranked items.
- Around line 44-48: Replace raw "like" usage that interpolates $term in
WorkspaceSearchProvider (the anonymous closure passed to
$modelClass::query()->where(...) and the similar block around lines 81-84) with
a portable escaped LIKE helper: call the project's likeTerm() utility (or
implement an escapeAndWrapLike($term) helper) to escape SQL wildcard characters
and pass the returned pattern to ->where('name', 'like', $escapedTerm) /
->orWhere('slug', 'like', $escapedTerm); ensure the helper accounts for
driver-specific ESCAPE behaviour (adds an explicit ESCAPE clause or driver-aware
escaping) so both the closure in the method using Builder $builder and the other
similar search block use the same escaped pattern helper.
In `@src/Search/SearchDispatcher.php`:
- Around line 62-74: The loop in SearchDispatcher that iterates $this->providers
and calls $provider->search($query) can let a thrown exception from any provider
abort aggregation; wrap the call to $provider->search($query) and its inner
iteration in a try/catch so a thrown exception from a single provider is caught,
reported (e.g. via a logger or by adding an error entry to results) and the loop
continues to the next provider; ensure you still filter by instanceof
SearchResult and preserve priority/index behavior when collecting into $ranked
so healthy providers' results are not lost.
In `@src/Website/Hub/Boot.php`:
- Around line 108-110: The secondary-domain renaming makes hardcoded route
lookups (route('hub.*') and request()->routeIs('hub.*')) in adminMenuItems() and
view files (e.g., SiteSettings.php) fail on secondary domains; update the code
to resolve both canonical and prefixed names: add a small domain-aware resolver
(e.g., hubRouteName($name) / hubRouteIs($pattern)) that checks the unprefixed
name first then the prefixed name produced by prefixSecondaryDomainRoutes(), and
replace direct calls to route('hub.*') and request()->routeIs('hub.*') in
adminMenuItems() and view closures with these helpers (or alternatively
implement a primary-domain redirect earlier in Boot.php before rendering).
Ensure references to prefixSecondaryDomainRoutes and adminMenuItems are used to
form the prefixed name so the helper covers all renamed routes.
---
Nitpick comments:
In `@src/Menu/AdminMenuProvider.php`:
- Around line 22-34: The phpdoc for getMenuItems() and getMenuGroups() is too
narrow: update their `@return` annotations to reflect that implementations may
return concrete MenuItem/MenuGroup instances, raw array payloads, or lazy
wrappers/callables accepted by AdminMenuRegistry and the tests; e.g. change
`@return` array<int, MenuItem> to `@return` array<int, MenuItem|array|callable> for
getMenuItems() and similarly change `@return` array<int, MenuGroup> to `@return`
array<int, MenuGroup|array|callable> for getMenuGroups() so module authors see
the actual supported payload shapes.
In `@src/Search/SearchResult.php`:
- Around line 117-145: The is_numeric check in normaliseConstructorArguments
misclassifies a purely numeric legacy subtitle at $arguments[5] as a score;
update the conditional that detects the new DTO shape so it requires both that
$arguments[5] is numeric AND that $arguments[4] also looks like the new-shape
category (e.g. validate $arguments[4] is a non-empty category-like string and
not an icon/class token), or otherwise treat it as the legacy shape — change the
if that inspects $arguments[5] to include this extra $arguments[4] validation to
avoid misinterpreting numeric subtitles.
In `@src/Website/Hub/Boot.php`:
- Around line 135-138: The current domainRoutePrefix in function
domainRoutePrefix maps both '.' and '-' to '_' and is not injective; change it
to produce an injective, collision-safe prefix (e.g. replace the simple char
mapping with a reversible encoding of the domain such as a URL-safe base64 or
hex encoding) so different domains always produce different prefixes; update
domainRoutePrefix to encode the full $domain with that reversible encoding and
then append the trailing dot as before (use the domainRoutePrefix function name
to locate the code and replace the strtr logic).
🪄 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: CHILL
Plan: Pro
Run ID: 46f6b3c2-b89c-430a-b203-95ee67c1560e
📒 Files selected for processing (13)
src/Boot.phpsrc/Menu/AdminMenuProvider.phpsrc/Menu/AdminMenuRegistry.phpsrc/Menu/MenuGroup.phpsrc/Menu/MenuItem.phpsrc/Search/Providers/UserSearchProvider.phpsrc/Search/Providers/WorkspaceSearchProvider.phpsrc/Search/SearchDispatcher.phpsrc/Search/SearchProvider.phpsrc/Search/SearchResult.phpsrc/Website/Hub/Boot.phptests/Feature/Menu/AdminMenuSystemTest.phptests/Feature/Search/SearchSystemTest.php
Snider
changed the title
10 CodeRabbit findings + pre-merge checks + CI annotations dispositioned. Code: - MenuGroup: trims stored key/label - User/workspace search: explicit LIKE ... ESCAPE '!' (was unsafe); ranks all candidates before provider-limit application - SearchDispatcher: reports provider failures best-effort + continues (was failing fast on first provider error) - Hub routes: domain-aware helpers across Hub PHP/Blade files - AdminMenuProvider: PHPDoc widened - SearchResult: no longer misclassifies numeric legacy subtitles - Secondary-domain route prefixes: collision-safe via hex encoding Doc / metadata: - PHPDoc @example blocks added across touched PHP APIs - composer.json: laravel/pint + pestphp/pest dev tooling + plugin config - package.json + package-lock.json: Tailwind 4 PostCSS via @tailwindcss/postcss - .github/workflows/ci.yml: actions/setup-node v6 + Node 24 Disposition replies: - GHAS/SonarCloud: no exposed findings via PR comments / check annotations / API; Dependabot 0 open alerts; secret scanning disabled. RESOLVED-COMMENT. Verification: php -l clean, composer validate --strict clean, pint --dirty + pint --test --dirty pass, pest on touched test files 26 tests / 83 assertions pass. npm ci + npm run build pass. Pre-existing test-env failure noted (out of scope): vendor/lthn/php references missing Core\Front\Client\Boot — affects unrelated app-booted tests + one SearchProviderRegistry expectation. Tracked in php repo's pending Service-module work. Closes findings on #10 Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/Search/Providers/WorkspaceSearchProvider.php (1)
113-116: Consider extracting sharedlikeTerm()to a trait or base class.Both
UserSearchProviderandWorkspaceSearchProviderhave identicallikeTerm()implementations. If more providers follow this pattern, consider extracting to a shared concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/Providers/WorkspaceSearchProvider.php` around lines 113 - 116, Both WorkspaceSearchProvider and UserSearchProvider duplicate the likeTerm() logic; extract this shared behavior into a single reusable unit (e.g., a trait LikeTermTrait with a protected function likeTerm(string $query): string) and have both classes use that trait (or alternatively create a common abstract base class with likeTerm and have both providers extend it), then remove the duplicate likeTerm() methods from WorkspaceSearchProvider and UserSearchProvider so they use the centralized implementation.src/Search/Providers/UserSearchProvider.php (1)
50-55: Consider adding a safety cap on unbounded queries.Fetching all matching records before applying the limit could be problematic for large user tables with common search terms. Consider adding a generous but bounded
->limit(1000)(or configurable cap) at the database level as a safety net, whilst still preserving the rank-then-trim behaviour.💡 Optional safeguard
return $modelClass::query() ->where(function (Builder $builder) use ($term): void { $builder->whereRaw("name LIKE ? ESCAPE '!'", [$term]) ->orWhereRaw("email LIKE ? ESCAPE '!'", [$term]); }) + ->limit(1000) // Safety cap before in-memory ranking ->get() ->map(fn (Model $user): SearchResult => $this->resultFor($user, $query))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/Providers/UserSearchProvider.php` around lines 50 - 55, The query in UserSearchProvider that returns $modelClass::query()->where(...)->get() is unbounded and can load too many rows; add a database-level safety cap (e.g. ->limit(1000)) before ->get() to bound results and make the cap configurable (inject a config value or class property) so it can be tuned, while still performing your existing rank-and-trim step afterwards; update the query chain where $modelClass::query() is used in UserSearchProvider to include ->limit($this->searchLimit ?? config('search.user_limit', 1000)) before calling ->get().src/Search/SearchResult.php (1)
66-79: Consider delegating to the constructor to reduce duplication.The
fromArray()method duplicates the normalisation logic that the constructor also performs. Since the constructor already handles named arguments,fromArray()could delegate directly.💡 Potential simplification
public static function fromArray(array $data): static { - return new self( - id: (string) ($data['id'] ?? uniqid('', true)), - title: (string) ($data['title'] ?? ''), - url: (string) ($data['url'] ?? '#'), - type: (string) ($data['type'] ?? $data['category'] ?? 'unknown'), - icon: (string) ($data['icon'] ?? 'document'), - subtitle: $data['subtitle'] ?? null, - meta: $data['meta'] ?? [], - category: (string) ($data['category'] ?? $data['type'] ?? 'unknown'), - score: (int) ($data['score'] ?? 0), - ); + return new self(...$data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/SearchResult.php` around lines 66 - 79, The fromArray method duplicates constructor normalization; replace the manual field-by-field construction by building an associative $args array whose keys match the constructor parameter names (id, title, url, type, icon, subtitle, meta, category, score), apply only minimal defaults/casts there, and then delegate to the constructor via new self(...$args) (i.e. call new self with the unpacked associative array) so all normalization lives in the constructor (functions/methods referenced: fromArray, __construct).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Search/Providers/UserSearchProvider.php`:
- Around line 50-55: The query in UserSearchProvider that returns
$modelClass::query()->where(...)->get() is unbounded and can load too many rows;
add a database-level safety cap (e.g. ->limit(1000)) before ->get() to bound
results and make the cap configurable (inject a config value or class property)
so it can be tuned, while still performing your existing rank-and-trim step
afterwards; update the query chain where $modelClass::query() is used in
UserSearchProvider to include ->limit($this->searchLimit ??
config('search.user_limit', 1000)) before calling ->get().
In `@src/Search/Providers/WorkspaceSearchProvider.php`:
- Around line 113-116: Both WorkspaceSearchProvider and UserSearchProvider
duplicate the likeTerm() logic; extract this shared behavior into a single
reusable unit (e.g., a trait LikeTermTrait with a protected function
likeTerm(string $query): string) and have both classes use that trait (or
alternatively create a common abstract base class with likeTerm and have both
providers extend it), then remove the duplicate likeTerm() methods from
WorkspaceSearchProvider and UserSearchProvider so they use the centralized
implementation.
In `@src/Search/SearchResult.php`:
- Around line 66-79: The fromArray method duplicates constructor normalization;
replace the manual field-by-field construction by building an associative $args
array whose keys match the constructor parameter names (id, title, url, type,
icon, subtitle, meta, category, score), apply only minimal defaults/casts there,
and then delegate to the constructor via new self(...$args) (i.e. call new self
with the unpacked associative array) so all normalization lives in the
constructor (functions/methods referenced: fromArray, __construct).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58fa76bb-0bc9-4fc3-90da-ecb41b581a06
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.github/workflows/ci.ymlcomposer.jsonpackage.jsonpostcss.config.jssrc/Menu/AdminMenuProvider.phpsrc/Menu/AdminMenuRegistry.phpsrc/Menu/MenuGroup.phpsrc/Menu/MenuItem.phpsrc/Search/Providers/UserSearchProvider.phpsrc/Search/Providers/WorkspaceSearchProvider.phpsrc/Search/SearchDispatcher.phpsrc/Search/SearchProvider.phpsrc/Search/SearchResult.phpsrc/Website/Hub/Boot.phpsrc/Website/Hub/View/Blade/admin/account-usage.blade.phpsrc/Website/Hub/View/Blade/admin/boost-purchase.blade.phpsrc/Website/Hub/View/Blade/admin/components/header.blade.phpsrc/Website/Hub/View/Blade/admin/components/sidebar.blade.phpsrc/Website/Hub/View/Blade/admin/content-editor.blade.phpsrc/Website/Hub/View/Blade/admin/content-manager.blade.phpsrc/Website/Hub/View/Blade/admin/content-manager/list.blade.phpsrc/Website/Hub/View/Blade/admin/content.blade.phpsrc/Website/Hub/View/Blade/admin/dashboard.blade.phpsrc/Website/Hub/View/Blade/admin/platform-user.blade.phpsrc/Website/Hub/View/Blade/admin/platform.blade.phpsrc/Website/Hub/View/Blade/admin/profile.blade.phpsrc/Website/Hub/View/Blade/admin/services-admin.blade.phpsrc/Website/Hub/View/Blade/admin/sites.blade.phpsrc/Website/Hub/View/Modal/Admin/ContentManager.phpsrc/Website/Hub/View/Modal/Admin/PlatformUser.phpsrc/Website/Hub/View/Modal/Admin/SiteSettings.phpsrc/Website/Hub/View/Modal/Admin/Sites.phpsrc/Website/Hub/View/Modal/Admin/WorkspaceSwitcher.phptests/Feature/Menu/AdminMenuSystemTest.phptests/Feature/Search/SearchSystemTest.phptests/Pest.php
✅ Files skipped from review due to trivial changes (8)
- package.json
- src/Website/Hub/View/Blade/admin/components/sidebar.blade.php
- src/Website/Hub/View/Blade/admin/content-editor.blade.php
- src/Website/Hub/View/Blade/admin/boost-purchase.blade.php
- src/Search/SearchProvider.php
- tests/Pest.php
- src/Menu/AdminMenuProvider.php
- src/Website/Hub/View/Blade/admin/platform-user.blade.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Menu/MenuGroup.php
- tests/Feature/Search/SearchSystemTest.php
…Result normalisation on PR #10 Round 2 follow-up to 9e19d67. Closes residual CodeRabbit findings. Code: - src/Search/Providers/UserSearchProvider.php: configurable user candidate cap before ranking (default 1000) — was unbounded - src/Search/Concerns/BuildsLikeTerms.php (new): shared LIKE pattern escaping trait used by both user + workspace providers - src/Search/Providers/WorkspaceSearchProvider.php: uses BuildsLikeTerms - src/Search/SearchResult.php: fromArray() delegates normalisation through constructor (was duplicating logic) Tests: - tests/Feature/Search/SearchSystemTest.php: 'Ugly' test for the configurable cap path (cap-before-ranking behaviour verified) Verification: php -l clean, pint --dirty + pint --test --dirty pass, pest on touched test files pass (20 + 7 = 27 tests). Closes residual findings on #10 Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Search/Providers/WorkspaceSearchProvider.php (1)
53-63: Bound candidates before in-memory ranking to avoid unbounded loads.Line 58 currently fetches all matching workspaces before scoring and slicing. On large datasets, this can create avoidable memory and latency spikes. Consider adding a bounded candidate cap (same pattern as
UserSearchProvider) beforeget().💡 Suggested refactor
public function __construct( private readonly string $modelClass = Workspace::class, - private readonly int $limit = 10 + private readonly int $limit = 10, + private readonly int $candidateLimit = 1000 ) {} @@ return $modelClass::query() ->where(function (Builder $builder) use ($term): void { $builder->whereRaw("name LIKE ? ESCAPE '!'", [$term]) ->orWhereRaw("slug LIKE ? ESCAPE '!'", [$term]); }) + ->limit($this->candidateLimit()) ->get() ->map(fn (Model $workspace): SearchResult => $this->resultFor($workspace, $query)) ->sortByDesc(static fn (SearchResult $result): int => $result->score) ->take($this->limit) ->values() ->all(); + +private function candidateLimit(): int +{ + return max($this->limit, $this->candidateLimit, 1); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/Providers/WorkspaceSearchProvider.php` around lines 53 - 63, The query in WorkspaceSearchProvider currently calls get() and then scores/slices, leading to unbounded loads; modify the query built from $modelClass::query() to apply a bounded candidate cap (use the same pattern as UserSearchProvider) before calling get() so you fetch at most a safe number of candidates (e.g., based on $this->limit or a multiplier) and then perform map($this->resultFor) / sortByDesc / take($this->limit) in-memory; update the code around the anonymous where closure and the subsequent get() call to include that pre-get limit.src/Search/SearchResult.php (1)
105-120: Keep serialisation symmetric with the expanded DTO fields.Lines 112-120 omit
categoryandscore, even though the DTO now carries both andfromArray()accepts both. Including them avoids silent data loss during array/JSON round-trips.💡 Suggested refactor
- * `@return` array{id: string, title: string, subtitle: ?string, url: string, type: string, icon: string, meta: array} + * `@return` array{id: string, title: string, subtitle: ?string, url: string, type: string, icon: string, category: string, score: int, meta: array} @@ return [ 'id' => $this->id, 'title' => $this->title, 'subtitle' => $this->subtitle, 'url' => $this->url, 'type' => $this->type, 'icon' => $this->icon, + 'category' => $this->category, + 'score' => $this->score, 'meta' => $this->meta, ];Also applies to: 126-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Search/SearchResult.php` around lines 105 - 120, The toArray() method in SearchResult is missing the category and score fields, causing asymmetric serialization with fromArray(); update SearchResult::toArray() to include 'category' => $this->category and 'score' => $this->score so array/JSON round-trips preserve all DTO fields and match SearchResult::fromArray().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Search/Providers/WorkspaceSearchProvider.php`:
- Around line 53-63: The query in WorkspaceSearchProvider currently calls get()
and then scores/slices, leading to unbounded loads; modify the query built from
$modelClass::query() to apply a bounded candidate cap (use the same pattern as
UserSearchProvider) before calling get() so you fetch at most a safe number of
candidates (e.g., based on $this->limit or a multiplier) and then perform
map($this->resultFor) / sortByDesc / take($this->limit) in-memory; update the
code around the anonymous where closure and the subsequent get() call to include
that pre-get limit.
In `@src/Search/SearchResult.php`:
- Around line 105-120: The toArray() method in SearchResult is missing the
category and score fields, causing asymmetric serialization with fromArray();
update SearchResult::toArray() to include 'category' => $this->category and
'score' => $this->score so array/JSON round-trips preserve all DTO fields and
match SearchResult::fromArray().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afb15f0a-0873-43ed-be8e-cf24fe8e3825
📒 Files selected for processing (5)
src/Search/Concerns/BuildsLikeTerms.phpsrc/Search/Providers/UserSearchProvider.phpsrc/Search/Providers/WorkspaceSearchProvider.phpsrc/Search/SearchResult.phptests/Feature/Search/SearchSystemTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature/Search/SearchSystemTest.php
… completeness on PR #10 Round 3 follow-up to 48b764a. Code: - WorkspaceSearchProvider.php: configurable candidateLimit (default 1000) applied before get() — symmetry with UserSearchProvider's cap. In-memory rank then trim preserved. - SearchResult.php: toArray() + jsonSerialize() include category + score (were missing); phpDoc return shapes updated. Tests: - SearchSystemTest.php: workspace candidate-cap 'Ugly' coverage; round-trip serialisation coverage for category + score. Verification: php -l clean, pint --dirty + pint --test --dirty pass, pest 22 tests / 79 assertions pass. Closes residual r3 findings on #10 Co-authored-by: Codex <noreply@openai.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
1 participant
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.
Routine dev→main PR opened by Hephaestus PR-cadence task.
This PR exists to:
ahead_by: 3 commit(s) (per gh api compare)If CodeRabbit clears with no blocking comments, this PR is eligible for
gh pr merge --merge(real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.Co-authored-by: Hephaestus hephaestus@cladius
Summary by CodeRabbit
New Features
Tests
Chores