Feature/api store currency locale - #211
Merged
Merged
Conversation
A schema_org strategy could be tested but not saved. meta-extraction requires
`value` to be null for schema_org and non-empty for every other type, while the
write endpoints required it unconditionally (create) or whenever a type was
present (update). The extension's Tune tab defaults all three fields to
schema_org and omits `value`, so "Test all" succeeded and "Save to store" 422'd
on the identical strategy. No single payload satisfied both, so any store whose
fields are detected via schema.org could not be saved at all.
Extracts the meta-extraction closure into App\Rules\ScrapeStrategyValue and
points all three requests at it. The rule locates the sibling `type` relative to
the attribute under validation, so it works at any path without a configured
prefix, and is implicit so it still runs when `value` is absent - which is both
how a schema_org strategy is legitimately sent and how a selector with no
expression is caught. Which types need a value stays owned by
ScraperStrategyType::needsValue().
A saved schema_org strategy round-trips as `{"type": "schema_org"}` with no
value key at all - StandardStrategyDto drops null values rather than persisting
them - and that shape is valid input to all three endpoints. Covered by a test
that creates a store, reads it back and posts it to meta-extraction.
Also adds the stores_filter_domain capability to /api/client-config, so clients
stop inferring filter[domain] support from products_filter_url.
POST /api/meta-extraction is synchronous and user-facing but had no ceiling. The scrape was bounded at 10s; the healing it can trigger was bounded only by the provider's own timeout (120s on this instance), so one request could block for over two minutes. Worse, healing is reached from the auto-create path - a URL matching no store - which is the first thing a user hits on a new retailer. Verified against a wedged provider: the same request went from HTTP 200 after 120.1s to HTTP 200 after 0.08s by default, and 24.1s with heal=true. Healing is now opt-in via a `heal` boolean on the request, defaulting to false. A caller testing selectors it just typed wants a fast deterministic answer, and healing may propose a different strategy than the one under test; "work it out for me" is a separate, explicitly slow action. Every request also carries a wall-clock budget (config price_buddy.meta_extraction.budget_seconds, default 25) spanning the scrape, any browser re-scrape and the AI call, which is handed the time remaining instead of the provider's timeout. Healing is declined outright below heal_floor_seconds rather than started and cut off. The agent's own tool fetches draw from the same budget and throw once it is gone - they run in-process between model calls, so the model call timeout does not cover them. Healing never fails the request: every failure mode returns 200 with the deterministic result and a `healing` object saying what happened (attempted/applied/reason, reason being disabled|not_needed|timeout|error), so a client can explain a thin result instead of showing empty fields. /api/client-config gains limits.meta_extraction_timeout_seconds so clients set their abort from the instance's real ceiling rather than a hardcoded guess. The admin UI heal path is unaffected - it calls previewForUrl() without a budget, which keeps the provider timeout, the right ceiling when a human chose to wait. BREAKING: API callers that relied on healing running by default must now send heal: true. The four API tests that exercised healing were updated accordingly.
Bounding the meta-extraction request stopped a wedged provider costing 120s, but it still cost the full 25s budget on every single request. A provider that has failed the last N calls will almost certainly fail the next one, and finding out is the expensive part. AiProviderHealth tracks consecutive failures per provider id in the cache. After failure_threshold consecutive failures (default 3) the provider is treated as unavailable for cooldown_seconds (default 300), which doubles as the counting window so thinly spread failures never accumulate. Any success closes it immediately. AiService feeds it, since that is the only layer that knows whether a provider call succeeded. The Ollama reachability check deliberately does not close the breaker: listing models proves the HTTP front end is up, not that generation works, which is exactly the state this instance is in. Only callers on a deadline consult it. The meta-extraction endpoint skips healing with the new reason `provider_unavailable`; the admin UI heal button and queued healing keep exercising the provider, so they act as the probe that notices recovery rather than waiting out the cooldown. Verified live against the wedged provider: three requests at 24.1s each, then the fourth returned in 0.105s with reason `provider_unavailable`. Note for API clients: `provider_unavailable` is a new value in the `healing.reason` set. It means the instance is degraded rather than the page being unextractable, so the same URL is worth retrying later.
Contributor
📝 WalkthroughWalkthroughThe PR adds opt-in AI healing for metadata extraction, with shared request budgets, healing outcome metadata, and AI provider circuit breaking. It also centralizes scrape-strategy validation and updates product deal scoring and presentation. ChangesMeta extraction and validation
Product insights
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MetaExtractionController
participant MetaExtractionService
participant AiConfigHealer
participant AiService
Client->>MetaExtractionController: Submit extraction with heal=true
MetaExtractionController->>MetaExtractionService: Start extraction with shared budget
MetaExtractionService->>AiConfigHealer: Request budget-aware healing
AiConfigHealer->>AiService: Run provider call with remaining timeout
AiService-->>AiConfigHealer: Return result or failure
AiConfigHealer-->>MetaExtractionService: Return healed configuration or no result
MetaExtractionService-->>MetaExtractionController: Return extraction and healing outcome
MetaExtractionController-->>Client: Serialize API response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/AiConfigHealer.php (1)
402-415: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the selector/regex validation tools stop when the extraction budget is exhausted.
HealingContext::validate()only checks whether HTML is loaded before runningStandardStrategyDtoand parsing the page offline. SinceTestCssSelectorToolandTestRegexToolcallvalidate()without checking$context->budget?->isExhausted(), the agent can continue looping past the remaining budget after the fetch tool has already stopped. Add the same budget exhaustion guard tovalidate()before executing a selector/regex test.🤖 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 `@app/Services/AiConfigHealer.php` around lines 402 - 415, Update HealingContext::validate() to check $context->budget?->isExhausted() before loading StandardStrategyDto or parsing HTML, and stop consistently when the extraction budget is exhausted. Preserve the existing HTML-loaded validation for remaining selector and regex test calls, including TestCssSelectorTool and TestRegexTool.
🧹 Nitpick comments (4)
tests/Feature/Api/MetaExtractionApiTest.php (1)
264-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSome sibling opt-out tests no longer prove their condition.
Adding
heal => truehere is correct. However,test_does_not_heal_when_store_opted_outandtest_does_not_heal_when_healing_feature_is_disabledstill post without the flag. Healing is now skipped for those requests becausehealdefaults to false, so the assertions pass regardless of the store opt-out and the feature flag. Add'heal' => trueto both requests so they exercise the paths they name.MetaExtractionBudgetTestcovers the opt-out case, but these tests are now tautological.🤖 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/MetaExtractionApiTest.php` at line 264, Update the requests in test_does_not_heal_when_store_opted_out and test_does_not_heal_when_healing_feature_is_disabled to include heal => true, ensuring each test actually reaches and validates its respective opt-out condition while preserving their existing assertions.tests/Feature/Api/MetaExtractionBudgetTest.php (1)
241-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe elapsed-time bound is tight enough to flake.
self::BUDGETis 2 seconds, and this assertion covers the whole request, including bootstrapping, Sanctum authentication, the database round trip, and the faked scrape. On a loaded CI runner that can exceed 2 seconds even though no provider call was made. The other timing assertions useself::BUDGET + 3. Consider asserting against a value that is clearly below the blocking path but above normal request overhead, or assert thatrunAgentwas never called instead of measuring time.🤖 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/MetaExtractionBudgetTest.php` around lines 241 - 245, Relax the elapsed-time assertion in this test to avoid measuring unrelated request overhead, using the established self::BUDGET + 3 tolerance used by the other timing assertions; alternatively, verify directly that runAgent was never called while preserving the assertion that the known-failing provider is skipped.app/Services/Ai/AiProviderHealth.php (1)
46-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe failure window slides; the comment states a fixed window.
Cache::putrewrites the counter with a full$cooldownTTL on every failure. Failures that arrive slightly faster than the cooldown therefore keep the counter alive and accumulate to the threshold, which the comment says cannot happen. Either accept the sliding window and correct the comment, or keep the first-failure expiry by writing the counter only when it is absent.♻️ Option: preserve the original window
- $failures = (int) Cache::get($this->failureKey($provider), 0) + 1; - Cache::put($this->failureKey($provider), $failures, $cooldown); + $key = $this->failureKey($provider); + // add() only writes when the key is absent, so the window starts at the first + // failure and is not extended by later ones. + Cache::add($key, 0, $cooldown); + $failures = (int) Cache::increment($key);🤖 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 `@app/Services/Ai/AiProviderHealth.php` around lines 46 - 55, Update the failure-counting logic in AiProviderHealth so the cooldown represents a fixed window from the first failure: preserve the existing counter TTL when incrementing and only establish it when the counter is absent, while keeping threshold handling and open-breaker behavior unchanged.app/Services/MetaExtractionService.php (1)
163-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed throwable.
$eis captured but never used, so an unexpected failure frompreviewForUrlleaves no trace. The healer logsAiProviderExceptionitself, but any other throwable disappears here. Add a log line so this failure mode is diagnosable. Static analysis may also flag the unused variable.♻️ Proposed change
} catch (Throwable $e) { + Log::warning('Meta-extraction healing failed.', [ + 'url' => $url, + 'exception' => $e::class, + 'message' => $e->getMessage(), + ]); + $outcome->failed($this->healFailureReason($budget));Add
use Illuminate\Support\Facades\Log;to the imports.🤖 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 `@app/Services/MetaExtractionService.php` around lines 163 - 169, Update the catch block around AiConfigHealer::previewForUrl in MetaExtractionService to log the caught Throwable via Laravel’s Log facade before marking the outcome failed and returning null; add the Log import and include useful exception details while preserving the existing failure flow.
🤖 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 `@app/Services/ExtractionBudget.php`:
- Around line 44-47: Update app/Services/ExtractionBudget.php in
remainingSecondsForTimeout() to return an explicit no-safe-start state when less
than one second remains instead of rounding up to one. In
app/Services/Ai/HealingContext.php, make fetch() reject that state and cap any
explicit timeout to the remaining budget. Update
tests/Unit/Services/ExtractionBudgetTest.php to assert rejection of the
no-safe-start state rather than expecting a one-second timeout.
---
Outside diff comments:
In `@app/Services/AiConfigHealer.php`:
- Around line 402-415: Update HealingContext::validate() to check
$context->budget?->isExhausted() before loading StandardStrategyDto or parsing
HTML, and stop consistently when the extraction budget is exhausted. Preserve
the existing HTML-loaded validation for remaining selector and regex test calls,
including TestCssSelectorTool and TestRegexTool.
---
Nitpick comments:
In `@app/Services/Ai/AiProviderHealth.php`:
- Around line 46-55: Update the failure-counting logic in AiProviderHealth so
the cooldown represents a fixed window from the first failure: preserve the
existing counter TTL when incrementing and only establish it when the counter is
absent, while keeping threshold handling and open-breaker behavior unchanged.
In `@app/Services/MetaExtractionService.php`:
- Around line 163-169: Update the catch block around
AiConfigHealer::previewForUrl in MetaExtractionService to log the caught
Throwable via Laravel’s Log facade before marking the outcome failed and
returning null; add the Log import and include useful exception details while
preserving the existing failure flow.
In `@tests/Feature/Api/MetaExtractionApiTest.php`:
- Line 264: Update the requests in test_does_not_heal_when_store_opted_out and
test_does_not_heal_when_healing_feature_is_disabled to include heal => true,
ensuring each test actually reaches and validates its respective opt-out
condition while preserving their existing assertions.
In `@tests/Feature/Api/MetaExtractionBudgetTest.php`:
- Around line 241-245: Relax the elapsed-time assertion in this test to avoid
measuring unrelated request overhead, using the established self::BUDGET + 3
tolerance used by the other timing assertions; alternatively, verify directly
that runAgent was never called while preserving the assertion that the
known-failing provider is skipped.
🪄 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: c456b027-5f7d-407b-897c-64bcb7a2ced4
📒 Files selected for processing (24)
app/Dto/HealingOutcomeDto.phpapp/Enums/HealingReason.phpapp/Filament/Resources/StoreResource/Api/Requests/CreateStoreRequest.phpapp/Filament/Resources/StoreResource/Api/Requests/UpdateStoreRequest.phpapp/Http/Controllers/Api/ClientConfigController.phpapp/Http/Controllers/Api/MetaExtractionController.phpapp/Http/Requests/MetaExtractionRequest.phpapp/Http/Resources/MetaExtractionResource.phpapp/Rules/ScrapeStrategyValue.phpapp/Services/Ai/AiProviderHealth.phpapp/Services/Ai/HealingContext.phpapp/Services/AiConfigHealer.phpapp/Services/AiService.phpapp/Services/ExtractionBudget.phpapp/Services/MetaExtractionService.phpconfig/price_buddy.phpdocs/docs/api.mdtests/Feature/Api/ClientConfigApiTest.phptests/Feature/Api/MetaExtractionApiTest.phptests/Feature/Api/MetaExtractionBudgetTest.phptests/Feature/Api/StoreScrapeStrategyValidationTest.phptests/Feature/Services/AiServiceHealthTest.phptests/Unit/Services/AiProviderHealthTest.phptests/Unit/Services/ExtractionBudgetTest.php
A product said "Great time to buy" the moment it was added. With a flat or single-point history the beat fraction is 0, but the current price is trivially the lowest ever seen, so isAllTimeLow fired and floored the score at 9.5. The claim was about a price nothing had been compared to. DealScoreCalculator now takes hasPriceVariation and beatsOtherListings, and short-circuits before the percentile and all-time-low logic when the price has never moved: - no variation, not the cheapest listing -> new `unknown` verdict, "Not enough data yet", score 0 - no variation but cheaper than the product's other URLs -> "Great time to buy", score 8. Not history, but real evidence: it is the best place to buy this today - variation -> unchanged isAllTimeLow is false in both no-variation cases. A low only means something next to a high, and the flag also drives the hero's "cheapest it has ever been" line and the dashboard's ordering. Side effect worth having: the dashboard's "Buy now" section filters on dealScore.score >= 6, so every newly added product used to land in it at 9.5. An `unknown` product scores 0 and stays out. Recomputed against real data: 3 of 11 products change, all of them two readings at an identical price on a single URL. The rest are untouched. Existing products keep their cached verdict until the next price change or `artisan buddy:regenerate-price-cache`.
The "should I buy right now?" card had a hardcoded teal gradient, so a red "Wait — it's expensive right now" sat on a card that read as good news. The verdict now picks a Filament palette (great/good -> primary, average/unknown -> gray, pricey -> warning, wait -> danger) which drives the gradient, the ring, the heading, the verdict text and the percentile pill together. Colours go through Filament's runtime CSS variables rather than text-*/bg-* utilities, since the palettes are injected by the panel and are not in the compiled CSS. The shades are also set as local custom properties on the card so the text keeps a dark-mode variant, which an inline style cannot carry on its own. The inline border-color this replaces was dead: the card uses ring-1, not a border, so it never rendered. It is now a --tw-ring-color override, which also tints the outline in dark mode. Verified by screenshotting three real products: wait renders red, pricey amber, great teal as before.
remainingSecondsForTimeout() rounded up to one second whenever less than one remained. Since isExhausted() only covers <= 0, a fetch with 0.4s left was both allowed and given a full second, overspending the deadline on every call - up to 25 times through the agent's tool loop. It now returns null below one second: an explicit "cannot start", never confused with Guzzle's "0 means no timeout". HealingContext::fetch() rejects that state - the null check subsumes its old isExhausted() guard - and caps any explicit timeout at the remaining budget, so the parameter cannot become a way past the deadline. Two other call sites needed the same distinction: - AiConfigHealer passes this value straight to runAgent(timeout:), where null means "use the provider's timeout". Passing the nullable value through would have silently restored the unbounded 120s call this whole mechanism exists to prevent, so it now separates "no budget" (unbudgeted callers keep the provider timeout) from "budget with nothing left" (stop). - MetaExtractionService::scrapeTimeout() does min($this->timeout, ...), and min(10, null) is null in PHP, which would have hit setConnectTimeout(int). The one-second floor now lives there, where it belongs: the scrape runs first, so the budget is always full, and the floor only bites on a near-zero budget. The new test pins the actual bug: with 2 seconds of budget and 1.2 elapsed the budget is not exhausted and has real time left, but has no whole second to hand out. The old code returned 1 there. Live re-check against the wedged provider is unchanged: HTTP 200 in 24.16s with reason "timeout".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/Services/ExtractionBudget.php`:
- Around line 48-50: Update app/Services/ExtractionBudget.php lines 48-50 in
remainingSecondsForTimeout() to sample remainingSeconds() once, returning null
when the sampled value is below one second and otherwise returning its floored
integer value. Update tests/Unit/Services/ExtractionBudgetTest.php lines 52-57
to assert that any non-null timeout is at least 1.
🪄 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: fd0b2149-384e-4ae0-b21c-798353a786ca
📒 Files selected for processing (5)
app/Services/Ai/HealingContext.phpapp/Services/AiConfigHealer.phpapp/Services/ExtractionBudget.phpapp/Services/MetaExtractionService.phptests/Unit/Services/ExtractionBudgetTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Services/AiConfigHealer.php
- app/Services/Ai/HealingContext.php
- app/Services/MetaExtractionService.php
Comment on lines
+48
to
+50
| public function remainingSecondsForTimeout(): ?int | ||
| { | ||
| return $this->hasAtLeast(1) ? (int) floor($this->remainingSeconds()) : null; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent a zero timeout at the one-second boundary.
remainingSecondsForTimeout() samples the clock twice. If the first sample reports at least one second and the second sample crosses below one second, the method returns 0 instead of null. The current test also accepts 0.
app/Services/ExtractionBudget.php#L48-L50: storeremainingSeconds()once, then returnnullwhen that value is below one second.tests/Unit/Services/ExtractionBudgetTest.php#L52-L57: assert that a non-null timeout is at least1.
🛠️ Proposed fix
public function remainingSecondsForTimeout(): ?int
{
- return $this->hasAtLeast(1) ? (int) floor($this->remainingSeconds()) : null;
+ $remaining = $this->remainingSeconds();
+
+ return $remaining >= 1 ? (int) floor($remaining) : null;
} $this->assertNotNull($timeout);
+ $this->assertGreaterThanOrEqual(1, $timeout);
$this->assertLessThanOrEqual(10, $timeout);📝 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.
Suggested change
| public function remainingSecondsForTimeout(): ?int | |
| { | |
| return $this->hasAtLeast(1) ? (int) floor($this->remainingSeconds()) : null; | |
| public function remainingSecondsForTimeout(): ?int | |
| { | |
| $remaining = $this->remainingSeconds(); | |
| return $remaining >= 1 ? (int) floor($remaining) : null; |
Suggested change
| public function remainingSecondsForTimeout(): ?int | |
| { | |
| return $this->hasAtLeast(1) ? (int) floor($this->remainingSeconds()) : null; | |
| public function test_the_timeout_value_is_rounded_down_so_the_budget_is_not_overspent(): void | |
| { | |
| $timeout = (new ExtractionBudget(10))->remainingSecondsForTimeout(); | |
| $this->assertNotNull($timeout); | |
| $this->assertGreaterThanOrEqual(1, $timeout); | |
| $this->assertLessThanOrEqual(10, $timeout); |
📍 Affects 2 files
app/Services/ExtractionBudget.php#L48-L50(this comment)tests/Unit/Services/ExtractionBudgetTest.php#L52-L57
🤖 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 `@app/Services/ExtractionBudget.php` around lines 48 - 50, Update
app/Services/ExtractionBudget.php lines 48-50 in remainingSecondsForTimeout() to
sample remainingSeconds() once, returning null when the sampled value is below
one second and otherwise returning its floored integer value. Update
tests/Unit/Services/ExtractionBudgetTest.php lines 52-57 to assert that any
non-null timeout is at least 1.
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