refactor: classifier config from env vars to DB-only per-company#173
Merged
Conversation
Classification is now OFF by default per company. When enabled via Settings > Classifier, the company's URL, token, and thresholds are used. No more env var fallback for classifier config. - Behaviour callbacks accept config map (url + api_token) - Client reads URL/token from config param, not Application env - Worker gates on ClassifierConfig.enabled before calling classifier - Remove INVOICE_CLASSIFIER_URL, INVOICE_CLASSIFIER_API_TOKEN, CATEGORY_CONFIDENCE_THRESHOLD, TAG_CONFIDENCE_THRESHOLD env vars - Remove env_defaults/0 from ServiceConfig - Remove classifier from global health checks (per-company now) - Extract default thresholds/URL to ClassifierConfig schema constants - Add thresholds_for_company/1 for LiveView threshold display - UI: "Enable invoice classification" toggle, no env override language - Seeds: create disabled ClassifierConfig for all companies - Add classifier_config factory for tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMoves invoice-classifier from global env/runtime config to per-company DB-backed ClassifierConfig; client/behaviour arities and worker/liveviews/tests updated to accept per-call config; removes global thresholds and global health/warm-up checks; adds DB seeding for missing per-company configs. Changes
Sequence DiagramsequenceDiagram
participant Worker as Worker
participant SC as ServiceConfig
participant IC as InvoiceClassifier
participant Client as ClassifierClient
participant API as External API
Worker->>SC: get_classifier_config(company_id)
SC-->>Worker: ClassifierConfig (enabled?, url, api_token, thresholds)
alt disabled
Worker->>Worker: cancel job (classification not enabled)
else enabled
Worker->>IC: predict_and_apply(invoice, config)
IC->>IC: decrypt api_token (if present)
IC->>Client: predict_category(input, config)
Client->>Client: fetch_url(config) / build_req
Client->>API: POST /predict_category (auth from config)
API-->>Client: category + confidence
Client-->>IC: category result
IC->>IC: apply category if >= config.category_threshold
IC->>Client: predict_tag(input, config)
Client->>API: POST /predict_tag
API-->>Client: tags + confidences
Client-->>IC: tag results
IC->>IC: filter tags by config.tag_threshold
IC-->>Worker: updated invoice / result
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub/service_config.ex (1)
87-89:⚠️ Potential issue | 🟡 MinorFix the formatter failure before merge.
CI reports
mix format --check-formattedfailing in this file near EOF. Please runmix formatand commit the formatting-only fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/service_config.ex` around lines 87 - 89, Run mix format on the affected file and commit the formatting-only change: reformat lib/ksef_hub/service_config.ex (ensure the file is formatted per project .formatter.exs, that trailing whitespace is removed and the file ends with a single newline at EOF) and push the commit; this will resolve the mix format --check-formatted failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub/invoice_classifier.ex`:
- Around line 136-138: The assignment to cat_threshold and the following
variable start (confident_category?) is misformatted; run mix format and adjust
the wrapping so the assignment uses parentheses or line breaks that satisfy
formatter rules — specifically update the lines around cat_threshold =
config.category_confidence_threshold ||
ClassifierConfig.default_category_threshold() and the subsequent
confident_category? expression in module InvoiceClassifier (look for the
variables cat_threshold, config.category_confidence_threshold,
ClassifierConfig.default_category_threshold, and confident_category?) so they
are wrapped/indented per mix format (e.g., keep the full fallback call on one
line or break before the ||) to resolve the formatter check.
- Around line 30-40: thresholds_for_company/1 currently insists both
category_confidence_threshold and tag_confidence_threshold are floats and
otherwise returns both defaults; change it to preserve per-field overrides by
fetching the config via ServiceConfig.get_classifier_config(company_id), then
for each field (category_confidence_threshold and tag_confidence_threshold) use
the configured value if it is a float and fallback to
ClassifierConfig.default_category_threshold() or
ClassifierConfig.default_tag_threshold() respectively if not; update the
function to perform individual is_float checks (so behavior matches the
per-field fallback in resolve_predictions/4) while keeping references to
ClassifierConfig.default_category_threshold/ default_tag_threshold and
ServiceConfig.get_classifier_config.
In `@test/ksef_hub/invoice_classifier_test.exs`:
- Line 225: The test headers/assertions are failing mix format; run mix format
(or manually wrap long lines) to fix formatting in the tests, specifically in
the test "sets predicted when only tag is above tag threshold" and the tests
covering the assertions between the later block (around the
invoice_classifier_test cases), ensuring long assertion lines and string
concatenations are wrapped to conform to Elixir formatter conventions so that
mix format --check-formatted passes; locate the tests by their names in
invoice_classifier_test.exs and re-run mix format to apply the changes.
In `@test/ksef_hub/invoice_classifier/client_test.exs`:
- Around line 85-89: The long chained assertions in the test for
Client.predict_tag are exceeding line length; break them into multiple
assertions or wrap the map access onto separate lines so each assert is short:
keep the call assert {:ok, result} = Client.predict_tag(%{invoice_title:
"Monthly subscription"}, `@test_config`) on its own line, then assert
result["predicted_label"] == "benefit-books-formula", assert
result["confidence"] == 0.88, and assert result["model_version"] == "1.0.0" each
on their own lines (or split any long expressions across lines) to satisfy the
formatter and CI.
- Around line 29-34: The long single-line assertion causes CI formatting
failures; refactor the final assertion in the test for Client.predict_category
to wrap across multiple lines by extracting nested values into temporaries and
asserting them separately (e.g., bind result["probabilities"] to a variable and
then assert that variable["operations:infrastructure"] == 0.95), or split the
assertion into two shorter asserts so that the line length is reduced while
keeping existing checks for predicted_label, confidence, model_version and
probabilities.
---
Outside diff comments:
In `@lib/ksef_hub/service_config.ex`:
- Around line 87-89: Run mix format on the affected file and commit the
formatting-only change: reformat lib/ksef_hub/service_config.ex (ensure the file
is formatted per project .formatter.exs, that trailing whitespace is removed and
the file ends with a single newline at EOF) and push the commit; this will
resolve the mix format --check-formatted failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17fa7d7f-23a3-44ed-b920-bb82876d1237
📒 Files selected for processing (22)
.env.exampleconfig/config.exsconfig/runtime.exslib/ksef_hub/invoice_classifier.exlib/ksef_hub/invoice_classifier/behaviour.exlib/ksef_hub/invoice_classifier/client.exlib/ksef_hub/invoice_classifier/worker.exlib/ksef_hub/service_config.exlib/ksef_hub/service_config/classifier_config.exlib/ksef_hub/service_health.exlib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/settings_live/services.expriv/repo/seeds.exstest/ksef_hub/invoice_classifier/client_test.exstest/ksef_hub/invoice_classifier/worker_test.exstest/ksef_hub/invoice_classifier_test.exstest/ksef_hub_web/controllers/health_controller_test.exstest/ksef_hub_web/live/invoice_live/upload_test.exstest/ksef_hub_web/live/settings_live/services_test.exstest/support/classifier_stub.extest/support/factory.ex
💤 Files with no reviewable changes (4)
- config/runtime.exs
- .env.example
- config/config.exs
- test/ksef_hub_web/live/invoice_live/upload_test.exs
Comment on lines
+30
to
+40
| case ServiceConfig.get_classifier_config(company_id) do | ||
| %ClassifierConfig{ | ||
| category_confidence_threshold: cat, | ||
| tag_confidence_threshold: tag | ||
| } | ||
| when is_float(cat) and is_float(tag) -> | ||
| {cat, tag} | ||
|
|
||
| _ -> | ||
| {ClassifierConfig.default_category_threshold(), ClassifierConfig.default_tag_threshold()} | ||
| end |
There was a problem hiding this comment.
Preserve partial threshold overrides in thresholds_for_company/1.
Current logic requires both thresholds to be floats; if only one is configured, both fall back to defaults. That can show incorrect values compared to actual prediction behavior (which falls back per field in resolve_predictions/4).
Suggested fix
def thresholds_for_company(company_id) do
case ServiceConfig.get_classifier_config(company_id) do
- %ClassifierConfig{
- category_confidence_threshold: cat,
- tag_confidence_threshold: tag
- }
- when is_float(cat) and is_float(tag) ->
- {cat, tag}
+ %ClassifierConfig{} = config ->
+ {
+ config.category_confidence_threshold ||
+ ClassifierConfig.default_category_threshold(),
+ config.tag_confidence_threshold ||
+ ClassifierConfig.default_tag_threshold()
+ }
_ ->
{ClassifierConfig.default_category_threshold(), ClassifierConfig.default_tag_threshold()}
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoice_classifier.ex` around lines 30 - 40,
thresholds_for_company/1 currently insists both category_confidence_threshold
and tag_confidence_threshold are floats and otherwise returns both defaults;
change it to preserve per-field overrides by fetching the config via
ServiceConfig.get_classifier_config(company_id), then for each field
(category_confidence_threshold and tag_confidence_threshold) use the configured
value if it is a float and fallback to
ClassifierConfig.default_category_threshold() or
ClassifierConfig.default_tag_threshold() respectively if not; update the
function to perform individual is_float checks (so behavior matches the
per-field fallback in resolve_predictions/4) while keeping references to
ClassifierConfig.default_category_threshold/ default_tag_threshold and
ServiceConfig.get_classifier_config.
The SSRF private-IP block prevented health checks from reaching localhost/private-network classifiers. Since the URL is configured by admins (manage_services permission), not untrusted users, the SSRF check is unnecessary and counterproductive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Run mix format across all changed files - thresholds_for_company/1 now falls back per-field instead of returning both defaults when either is nil (matches resolve_predictions) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/settings_live/services.ex (1)
307-319:⚠️ Potential issue | 🟡 MinorProbe only validated submitted URLs.
resolve_check_url/2currently accepts any non-empty string, and Line 458 falls back to the persisted URL when the submitted one is blank. That means the save/check-health flow can probe a value different from what the user is trying to save, and malformed URLs still send the user through the Save anyway? path even thoughlib/ksef_hub/service_config/classifier_config.ex:97-107will reject them afterward.Suggested fix
defp resolve_check_url(params, socket) do enabled = params["enabled"] in ["true", true] url = params["url"] cond do - enabled && is_binary(url) && url != "" -> url - enabled -> socket.assigns.config.url + enabled && valid_http_url?(url) -> url true -> nil end end `@spec` check_health_async(ServiceConfig.ClassifierConfig.t()) :: reference() defp check_health_async(config) do - url = if config.enabled, do: config.url + url = if(config.enabled && valid_http_url?(config.url), do: config.url) if url do %{ref: ref} = Task.Supervisor.async_nolink(KsefHub.TaskSupervisor, fn -> {:health_result, check_url_health(url)} @@ ref end end + + `@spec` valid_http_url?(term()) :: boolean() + defp valid_http_url?(url) when is_binary(url) do + case URI.parse(url) do + %URI{scheme: scheme, host: host} + when scheme in ["http", "https"] and is_binary(host) and host != "" -> + true + + _ -> + false + end + end + + defp valid_http_url?(_), do: falseAlso applies to: 335-336, 451-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 307 - 319, resolve_check_url/2 is allowing fallback to the persisted URL when the submitted URL is blank, causing health checks to probe a different value than the user is saving; change the flow so the health probe only runs for a submitted URL that is non-blank and minimally validated (e.g., well-formed scheme/host) and does not fall back to the persisted URL. Specifically, update resolve_check_url/2 to return nil when the submitted URL is blank or fails basic validation, ensure the Task.Supervisor async call to check_url_health only happens when resolve_check_url/2 returns a validated submitted URL, and keep do_save/2 unchanged so classifier validation (classifier_config) still runs on save errors. Reference: resolve_check_url/2, check_url_health/1, do_save/2, and the classifier validation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Line 12: The LiveView currently aliases and calls
KsefHub.ServiceConfig.ClassifierConfig directly for placeholder/default values;
instead, add and call a context function on KsefHub.ServiceConfig (e.g.,
get_classifier_defaults/0 or classifier_defaults/0) that returns those defaults,
and update this module to call KsefHub.ServiceConfig.get_classifier_defaults()
(or the chosen name) everywhere you previously referenced ClassifierConfig
(including the occurrences around lines 87-122) and remove the direct alias to
KsefHub.ServiceConfig.ClassifierConfig so callers use the ServiceConfig context
API only.
- Around line 40-43: The code currently starts a probe on every connected mount
by calling check_health_async(config) and assigning active_health_ref and
health, which causes disabled configs (where check_health_async/1 returns
:not_configured) to display as "Unreachable"; change the logic in the socket
setup so you only call check_health_async/1 and assign active_health_ref/health
when the config is enabled (e.g., check a flag on config or use a helper like
config_enabled?(config)); for disabled configs skip the probe and set health to
a neutral/disabled value (or leave it as nil) instead of :checking so the UI
does not mark a disabled config as unhealthy (refer to connected?(socket),
check_health_async/1, active_health_ref and health assigns).
---
Outside diff comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 307-319: resolve_check_url/2 is allowing fallback to the persisted
URL when the submitted URL is blank, causing health checks to probe a different
value than the user is saving; change the flow so the health probe only runs for
a submitted URL that is non-blank and minimally validated (e.g., well-formed
scheme/host) and does not fall back to the persisted URL. Specifically, update
resolve_check_url/2 to return nil when the submitted URL is blank or fails basic
validation, ensure the Task.Supervisor async call to check_url_health only
happens when resolve_check_url/2 returns a validated submitted URL, and keep
do_save/2 unchanged so classifier validation (classifier_config) still runs on
save errors. Reference: resolve_check_url/2, check_url_health/1, do_save/2, and
the classifier validation path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a778d062-9bb1-4a6e-83dd-f169e4c0fbca
📒 Files selected for processing (2)
lib/ksef_hub_web/live/settings_live/services.extest/ksef_hub_web/live/settings_live/services_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ksef_hub_web/live/settings_live/services_test.exs
- Add ServiceConfig.classifier_defaults/0 context function and remove direct ClassifierConfig alias from the LiveView - Skip health probe on mount when config is disabled (avoids showing "Unreachable" for disabled classifiers) - resolve_check_url no longer falls back to persisted URL — only probes the submitted URL the user is actually saving Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The async health check to an unreachable URL takes variable time depending on CI load. Replace Process.sleep(200) with a polling loop that waits up to 2s for the "Save anyway" dialog to appear. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/ksef_hub_web/live/settings_live/services.ex (1)
435-448: Consider exposing a type alias from the context module.The
@specreferencesServiceConfig.ClassifierConfig.t()directly. While this is acceptable for type specs (the guideline targets function calls, not types), exposing a type alias likeServiceConfig.classifier_config()would maintain cleaner module boundaries.💡 Optional: Add type alias to ServiceConfig
In
lib/ksef_hub/service_config.ex:`@type` classifier_config :: ClassifierConfig.t()Then update the spec:
- `@spec` build_form(ServiceConfig.ClassifierConfig.t()) :: Phoenix.HTML.Form.t() + `@spec` build_form(ServiceConfig.classifier_config()) :: Phoenix.HTML.Form.t()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 435 - 448, Add a public type alias in the ServiceConfig module (e.g., `@type` classifier_config :: ClassifierConfig.t()) and then update the `@spec` on build_form to reference the alias (change ServiceConfig.ClassifierConfig.t() to ServiceConfig.classifier_config()). This keeps type references cleaner while leaving the function implementation (build_form/1) and its use of ServiceConfig.change_classifier_config/2 unchanged.lib/ksef_hub/invoice_classifier.ex (1)
99-107: Consider logging decryption failures for observability.When
Encryption.decrypt/1fails, the function silently returnsnil. While this allows classification to proceed (perhaps the service doesn't require auth), it could mask configuration issues. A warning-level log would help operators diagnose misconfigured tokens without blocking classification.💡 Optional: Add warning log on decryption failure
defp decrypt_token(encrypted) do case Encryption.decrypt(encrypted) do {:ok, token} -> token - {:error, _} -> nil + {:error, reason} -> + Logger.warning("Failed to decrypt classifier API token: #{inspect(reason)}") + nil end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoice_classifier.ex` around lines 99 - 107, In decrypt_token/1 in the InvoiceClassifier module, keep the existing behavior but log a warning when Encryption.decrypt(encrypted) returns {:error, reason}; update the clause handling {:error, _} to call Logger.warn/1 (or your app logger) with a clear message including the encrypted input identifier and the inspected reason (masked or truncated if sensitive) before returning nil so decryption failures are observable without changing return semantics.test/ksef_hub/invoice_classifier_test.exs (1)
440-460: Consider using distinct values in first test to verify config is actually read.The test at line 442 asserts
{0.71, 0.95}which are both the configured values and the defaults. This test would pass even ifthresholds_for_company/1always returned defaults. The test at lines 449-458 correctly uses distinct values (0.5, 0.8) to prove custom config is read.Consider updating the setup thresholds to differ from defaults, or this specific test is essentially a duplicate of "returns defaults when no config exists".
💡 Optional: Use distinct thresholds in setup
`@category_threshold` 0.71 `@tag_threshold` 0.95 + # Note: These match defaults. For clearer testing, consider using distinct values: + # `@category_threshold` 0.75 + # `@tag_threshold` 0.90Or update the first test to use the
other_companypattern with distinct thresholds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoice_classifier_test.exs` around lines 440 - 460, The "returns thresholds from classifier config" test is ineffective because it asserts values identical to the defaults; update the test so it verifies actual config lookup by configuring distinct thresholds for the existing setup company (or follow the "other_company" pattern) and then assert InvoiceClassifier.thresholds_for_company(company.id) returns those distinct values; reference the test description "returns thresholds from classifier config" and the function InvoiceClassifier.thresholds_for_company/1 when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 435-448: Add a public type alias in the ServiceConfig module
(e.g., `@type` classifier_config :: ClassifierConfig.t()) and then update the
`@spec` on build_form to reference the alias (change
ServiceConfig.ClassifierConfig.t() to ServiceConfig.classifier_config()). This
keeps type references cleaner while leaving the function implementation
(build_form/1) and its use of ServiceConfig.change_classifier_config/2
unchanged.
In `@lib/ksef_hub/invoice_classifier.ex`:
- Around line 99-107: In decrypt_token/1 in the InvoiceClassifier module, keep
the existing behavior but log a warning when Encryption.decrypt(encrypted)
returns {:error, reason}; update the clause handling {:error, _} to call
Logger.warn/1 (or your app logger) with a clear message including the encrypted
input identifier and the inspected reason (masked or truncated if sensitive)
before returning nil so decryption failures are observable without changing
return semantics.
In `@test/ksef_hub/invoice_classifier_test.exs`:
- Around line 440-460: The "returns thresholds from classifier config" test is
ineffective because it asserts values identical to the defaults; update the test
so it verifies actual config lookup by configuring distinct thresholds for the
existing setup company (or follow the "other_company" pattern) and then assert
InvoiceClassifier.thresholds_for_company(company.id) returns those distinct
values; reference the test description "returns thresholds from classifier
config" and the function InvoiceClassifier.thresholds_for_company/1 when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90e2da22-e105-449b-a4aa-b9758f33bc0f
📒 Files selected for processing (5)
lib/ksef_hub/invoice_classifier.exlib/ksef_hub/service_config.exlib/ksef_hub_web/live/settings_live/services.extest/ksef_hub/invoice_classifier/client_test.exstest/ksef_hub/invoice_classifier_test.exs
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ksef_hub/service_config.ex
- test/ksef_hub/invoice_classifier/client_test.exs
- decrypt_token/1 now logs a warning when decryption fails instead of silently returning nil - thresholds_for_company test uses distinct values (0.60/0.85) instead of matching defaults, proving actual config lookup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ksef_hub_web/live/settings_live/services_test.exs`:
- Around line 145-164: The test relies on a real network failure; replace that
with a Mox-based mock: introduce or use an HTTP client behaviour (the calls in
check_url_health/1 that currently call Req.get) and update check_url_health/1 to
call the injected client module instead of Req directly, then in
test/ksef_hub_web/live/settings_live/services_test.exs set a Mox expectation for
the client to return a deterministic error response for "http://127.0.0.1:19999"
before rendering/submitting the form so the async health-check task immediately
sees the mocked failure and the test can assert the "Save anyway" UI without
real network timing. Ensure the test config sets the app to use the mock client
and verify the mock is verified after the test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: efe4418b-7c36-4e6a-8386-b5bfe5dd85cb
📒 Files selected for processing (1)
test/ksef_hub_web/live/settings_live/services_test.exs
Comment on lines
+145
to
+164
| url: "http://127.0.0.1:19999", | ||
| category_confidence_threshold: "0.80", | ||
| tag_confidence_threshold: "0.90" | ||
| } | ||
|
|
||
| # Submit with a private IP — shows confirmation dialog | ||
| # Submit with an unreachable URL — shows confirmation dialog | ||
| view | ||
| |> form("form[phx-submit=save]", classifier: form_params) | ||
| |> render_submit() | ||
|
|
||
| # Wait for the async health-check task to deliver its result | ||
| Process.sleep(50) | ||
| html = render(view) | ||
| # Poll until the async health-check task completes and renders "Save anyway" | ||
| html = | ||
| Enum.reduce_while(1..20, nil, fn _, _acc -> | ||
| Process.sleep(100) | ||
| html = render(view) | ||
|
|
||
| if html =~ "Save anyway", | ||
| do: {:halt, html}, | ||
| else: {:cont, html} | ||
| end) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this test still relies on real network timing and does not use Mox in this flow.
rg -n -C3 --type=elixir '127\.0\.0\.1:19999|Process\.sleep|Enum\.reduce_while|Mox|expect\(' test/ksef_hub_web/live/settings_live/services_test.exs
rg -n -C3 --type=elixir 'check_url_health\(' lib/ksef_hub_web/live/settings_live/services.exRepository: appunite/appunite-ksef-ex
Length of output: 1573
Mock pre-save health checks instead of relying on real unreachable URLs.
This test violates the coding guideline "Use Mox for mocking external services in tests" by relying on real network failure timing. The check_url_health/1 function at line 477-480 in services.ex makes real HTTP requests via Req.get(), and this test expects that call to fail on 127.0.0.1:19999. Instead, mock the Req call to return a deterministic error response, eliminating flakiness and improving test speed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ksef_hub_web/live/settings_live/services_test.exs` around lines 145 -
164, The test relies on a real network failure; replace that with a Mox-based
mock: introduce or use an HTTP client behaviour (the calls in check_url_health/1
that currently call Req.get) and update check_url_health/1 to call the injected
client module instead of Req directly, then in
test/ksef_hub_web/live/settings_live/services_test.exs set a Mox expectation for
the client to return a deterministic error response for "http://127.0.0.1:19999"
before rendering/submitting the form so the async health-check task immediately
sees the mocked failure and the test can assert the "Save anyway" UI without
real network timing. Ensure the test config sets the app to use the mock client
and verify the mock is verified after the test.
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
INVOICE_CLASSIFIER_URL,INVOICE_CLASSIFIER_API_TOKEN,CATEGORY_CONFIDENCE_THRESHOLD,TAG_CONFIDENCE_THRESHOLD)ClassifierConfig.enabledbefore calling the classifier serviceChanges
Core plumbing
Behaviourcallbacks acceptconfigmap (%{url, api_token}) as 2nd argClientreads URL/token from config param, notApplication.get_envInvoiceClassifier.predict_and_apply/2acceptsClassifierConfigstructClassifierConfigschema constants (single source of truth)Worker
ClassifierConfigby company_id, cancels if not enabledConfig cleanup
config.exs,runtime.exs,.env.exampleServiceConfig.env_defaults/0/healthz/serviceshealth checks (per-company now)UI
ClassifierConfigdefaults (was env var values)Seeds
seeds.exscreates disabledClassifierConfigfor all companies without oneTest plan
mix test— 2155 tests pass, 0 failures/healthz/servicesreturns only pdf_renderer + invoice_extractormix run priv/repo/seeds.exs— seeds classifier configs🤖 Generated with Claude Code
Summary by CodeRabbit
Configuration Changes
UI
Health & Operations
Data
Tests