Feature/server side url matching - #209
Merged
Merged
Conversation
Addresses code review findings on task 1: - test_env_extra_appends_rather_than_replaces now sets both _EXTRA env vars to real values and re-requires the config file, so a regression that replaces the defaults instead of appending is actually caught. - config/url_matching.php filters empty/whitespace-only entries out of the exploded env value before merging, so the arrays contain exactly the documented defaults when no _EXTRA env var is set.
- filter[url] and filter[domain]: Spatie splits filter values on commas, turning a comma-bearing value into an array before the callback runs. (string) on an array raised an uncaught ErrorException -> HTTP 500. Rejoin array values with ',' before normalising in both handlers. - filter[url]=: ConvertEmptyStringsToNull turns it into null, and Spatie skips non-nullable callback filters on null, so the filter silently vanished and returned the caller's whole product list. Add ->nullable(), matching the existing filter[domain] pattern. - ProductTransformer::toArray(): price_cache decoration ran unconditionally, so a sparse fieldset excluding price_cache got a phantom price_cache: [] key back, misreporting an untracked product. Only decorate when the key is actually present. Also drop null product ids (from fields[products] without id) before the current_url whereIn lookup in PaginationHandler.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesURL matching and API filtering
Client configuration endpoint
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Sanctum
participant ClientConfigController
Client->>Sanctum: GET /api/client-config with token
Sanctum->>ClientConfigController: authorize ClientConfigRead
ClientConfigController-->>Client: version, capabilities, ETag, cache headers
Client->>ClientConfigController: If-None-Match request
ClientConfigController-->>Client: 304 Not Modified
sequenceDiagram
participant ProductAPI
participant UrlModel
participant ProductTransformer
ProductAPI->>UrlModel: resolve normalized current_url matches
UrlModel-->>ProductAPI: matching URL IDs
ProductAPI->>ProductTransformer: configure current URL and matching IDs
ProductTransformer-->>ProductAPI: price_cache entries with is_current
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 5
🧹 Nitpick comments (1)
tests/Feature/Api/ProductApiTest.php (1)
826-826: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused fixture variables.
Both assignments trigger PHPMD unused-variable warnings.
tests/Feature/Api/ProductApiTest.php#L826-L826: CallproductWithTrackedUrl()without assigning$product.tests/Feature/Api/StoreApiTest.php#L459-L459: Create the fixture without assigning$store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Api/ProductApiTest.php` at line 826, Remove the unused fixture assignments: in tests/Feature/Api/ProductApiTest.php lines 826-826, call productWithTrackedUrl() without assigning $product; in tests/Feature/Api/StoreApiTest.php lines 459-459, create the fixture without assigning $store.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/url_matching.php`:
- Around line 20-25: Update the tracking-parameter environment value handling in
the tracking_params configuration and the corresponding additional matching
values around the related entries to trim each exploded value before filtering
empty strings and merging. Preserve the existing exclusion of blank values while
ensuring inputs such as “foo, bar” produce normalized parameter names without
leading or trailing whitespace.
In `@docs/docs/api.md`:
- Around line 136-140: Add the text language identifier to the fenced code block
containing the example URLs in the API documentation, leaving the URL examples
unchanged.
In `@tests/Feature/Api/ClientConfigApiTest.php`:
- Around line 30-36: Update test_readable_with_a_minimal_ability_token to
authenticate with only ApiAbility::ClientConfigRead->value, removing
ApiAbility::UserDetail and ApiAbility::MetaExtractionExtract while preserving
the rest of the test.
In `@tests/Unit/Models/UrlNormalizeForMatchTest.php`:
- Around line 90-117: Update test_env_extra_appends_rather_than_replaces() to
snapshot the existing getenv(), $_ENV, and $_SERVER values for both environment
keys before overwriting them, then restore those prior states in finally instead
of always removing the entries. Preserve whether each variable was previously
absent or present, including its original value.
---
Nitpick comments:
In `@tests/Feature/Api/ProductApiTest.php`:
- Line 826: Remove the unused fixture assignments: in
tests/Feature/Api/ProductApiTest.php lines 826-826, call productWithTrackedUrl()
without assigning $product; in tests/Feature/Api/StoreApiTest.php lines 459-459,
create the fixture without assigning $store.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b2fbaaf-f051-4edf-97af-357c2d20e2e8
📒 Files selected for processing (21)
.ai/guidelines/general-preferences.blade.phpapp/Console/Commands/RenormalizeUrls.phpapp/Enums/ApiAbility.phpapp/Filament/Resources/ProductResource/Api/Handlers/DetailHandler.phpapp/Filament/Resources/ProductResource/Api/Handlers/PaginationHandler.phpapp/Filament/Resources/ProductResource/Api/Transformers/ProductTransformer.phpapp/Filament/Resources/StoreResource/Api/Handlers/PaginationHandler.phpapp/Http/Controllers/Api/ClientConfigController.phpapp/Models/Url.phpconfig/url_matching.phpdatabase/migrations/2026_08_07_000000_add_url_normalized_to_urls_table.phpdocs/docs/advanced.mddocs/docs/api.mddocs/docs/installation.mdroutes/api.phptests/Feature/Api/ClientConfigApiTest.phptests/Feature/Api/ProductApiTest.phptests/Feature/Api/StoreApiTest.phptests/Feature/Console/RenormalizeUrlsTest.phptests/Unit/Models/UrlNormalizeForMatchTest.phptests/Unit/Models/UrlNormalizedColumnTest.php
Comment on lines
+130
to
+138
| $normalized = Url::normalizeForMatch($currentUrl); | ||
|
|
||
| if ($normalized === '') { | ||
| return []; | ||
| } | ||
|
|
||
| return $this->resource->urls() | ||
| ->where('url_normalized', $normalized) | ||
| ->pluck('id') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove final path slashes during URL normalization.
Url::normalizeForMatch() preserves the final slash in a non-root path. Therefore, https://shop.com/p/x and https://shop.com/p/x/ resolve to different keys.
This breaks the trailing-slash contract used by current_url and filter[url]. Normalize the path without final slashes before building url_normalized.
Proposed upstream fix in app/Models/Url.php
- $path = $uri->path();
- $path = $path === '/' ? '' : '/'.Str::lower($path);
+ $path = rtrim((string) $uri->path(), '/');
+ $path = $path === '' ? '' : '/'.Str::lower(ltrim($path, '/'));- config/url_matching.php: trim each exploded env value before filtering empties, so "foo, bar" yields normalised names without whitespace - ClientConfigApiTest: authenticate the minimal-token test with only client-config:read, proving the endpoint is not gated on other abilities - UrlNormalizeForMatchTest: snapshot and restore the env vars instead of unconditionally unsetting them, and pin the trimming behaviour - api.md: tag the URL example fence as text - drop two unused fixture assignments
filter[url][]= and filter[domain][]= produce a nested array once Spatie splits each element on the comma delimiter, which reached implode() and raised "Array to string conversion" -> ErrorException -> HTTP 500. Adds ApiHelperTrait::filterValueToString(), which flattens before rejoining with ',' so the original value is reconstructed exactly, and uses it from both filter callbacks in place of the duplicated inline cast.
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.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes