Feat/invoice amount parsing review workflow#55
Merged
emilwojtaszek merged 12 commits intoFeb 25, 2026
Conversation
Previously the parser only read P_13_1 (net at 23%) and P_14_1 (VAT at 23%). FA(3) XML splits amounts by rate — P_13_2 (8%), P_13_3 (5%), etc. Invoices at non-23% rates got nil for net/vat amounts. Now sums P_13_1..P_13_11 for net and P_14_1..P_14_5 for VAT. Returns nil when no fields present (preserves behavior for truly missing data). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KSeF invoices now get extraction_status computed before upsert, using the same critical field logic as PDF uploads. This means invoices with missing amounts (e.g. non-23% VAT rates) are flagged as :partial. - Add determine_extraction_status_from_attrs/1 public function - Add :extraction_status to @upsert_replace_fields for re-sync updates - Compute extraction_status in invoice_fetcher before upsert Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add extraction_badge/1 component (orange "Incomplete", red "Failed") - Show badge in invoice show page header and index list status column - Show warning banner on show page for partial/failed extraction - Highlight missing amount rows with warning background color Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Users can now edit any invoice field directly on the show page. When an invoice has extraction_status :partial or :failed, the edit form opens automatically. - Add edit_changeset/2 to Invoice schema for manual field edits - Add update_invoice_fields/2 to Invoices context that recalculates extraction_status and enqueues prediction when going from partial to complete - Add Edit button, toggle/validate/save/cancel event handlers - Show edit form with all fields: number, date, seller/buyer, amounts, currency - Improved approve error: specific message for incomplete extraction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix mixed atom/string key issue in update_invoice_fields/2 by normalizing all keys to strings before passing to edit_changeset - Add update_invoice_fields/2 unit tests (complete, partial, invalid) - Add extraction status display tests (badge, warning banner) - Add approval error test for incomplete extraction - Add edit form UI tests (toggle, auto-open, cancel, save, validation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove duplicated compute_extraction_status_for_edit/2 and normalize_keys/2 — reuse determine_extraction_status_from_attrs/1 as the single source of truth for extraction status computation - Simplify recalculate_extraction_status/2 to delegate to the shared function - Use with/1 in update_invoice_fields/2 for cleaner error passthrough - Use string key for extraction_status in form params to avoid mixed atom/string key maps (Ecto.CastError) - Simplify toggle_edit handler (Edit button only opens, cancel closes) - Collapse extraction_badge nil/complete clauses with guard - Add attr declaration to field_error component Co-Authored-By: Claude Opus 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:
📝 WalkthroughWalkthroughAdds an extraction_status field with computation and persistence, aggregates multi-bucket net/vat parsing, injects status into sync/upsert flows, exposes inline edit UI with validation and status-aware behavior, and expands tests and fixtures for parsing and extraction-status logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Parser (XML)
participant Fetcher as Sync.InvoiceFetcher
participant Invoices as Invoices context
participant Repo as DB/Repo
participant Worker as PredictionQueue
rect rgba(200,200,255,0.5)
Parser->>Fetcher: parse invoice -> attrs (net, vat, gross, fields...)
end
rect rgba(200,255,200,0.5)
Fetcher->>Invoices: build attrs + extraction_status = determine_extraction_status_from_attrs(attrs)
Invoices->>Repo: upsert_invoice(attrs with extraction_status)
Repo-->>Invoices: upsert result (insert/update)
end
rect rgba(255,200,200,0.5)
Note right of Invoices: On update via update_invoice_fields/2\nif status transitioned partial/failed -> complete
Invoices->>Worker: enqueue prediction job
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (7)
lib/ksef_hub/sync/invoice_fetcher.ex (1)
165-167: Consider a simpler form —then/2with a one-liner can be flattened.The
then/2wrapper is readable, but the transformation can be expressed more concisely as a plainMap.putafter theMap.droppipeline if preferred. This is purely a style note.♻️ Optional simplification
- |> Map.drop([:line_items]) - |> then(fn a -> - Map.put(a, :extraction_status, Invoices.determine_extraction_status_from_attrs(a)) - end) + |> Map.drop([:line_items]) + |> then(&Map.put(&1, :extraction_status, Invoices.determine_extraction_status_from_attrs(&1)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/sync/invoice_fetcher.ex` around lines 165 - 167, The pipeline uses then/2 with a one-line Map.put which can be simplified: replace the |> then(fn a -> Map.put(a, :extraction_status, Invoices.determine_extraction_status_from_attrs(a)) end) usage with a direct |> Map.put(:extraction_status, Invoices.determine_extraction_status_from_attrs(...)) style by calling Map.put directly after the prior pipeline value; locate the occurrence by the then/2 call and the referenced Invoices.determine_extraction_status_from_attrs function and inline the transformation to remove the anonymous fn wrapper for brevity and readability.lib/ksef_hub_web/components/invoice_components.ex (1)
102-111: Catch-all silently labels unknown statuses as "Failed".The
elsebranch in{if@status== :partial, do: "Incomplete", else: "Failed"}will render a red "Failed" badge for any atom other than:partial. If a newextraction_statusvalue is introduced, it will silently misclassify. Consider using acaseor raising on unexpected values.♻️ Suggested fix
- {if `@status` == :partial, do: "Incomplete", else: "Failed"} + {case `@status` do + :partial -> "Incomplete" + :failed -> "Failed" + end}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/components/invoice_components.ex` around lines 102 - 111, The extraction_badge/1 currently treats any non-:partial status as "Failed" which mislabels unknown statuses; update extraction_badge (use the `@status` assign) to pattern-match explicitly (e.g., handle :partial and :failed cases) using a case or cond and provide a clear fallback that either raises (or logs and shows a neutral/unknown badge) for unexpected atoms so new extraction_status values are not silently rendered as "Failed".test/ksef_hub/invoices_test.exs (1)
708-747:determine_extraction_status_from_attrs/1tests cover amounts but not identity fields.The describe block tests
net_amountandgross_amountas critical fields but doesn't include a case whereseller_niporinvoice_numberis absent. Ifall_critical_fields_present?/1checks those fields too (as suggested by theupdate_invoice_fieldstest at Line 823–829), adding a test for a missing identity field would complete the boundary coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices_test.exs` around lines 708 - 747, Add tests to cover missing identity fields for determine_extraction_status_from_attrs/1: create at least one test where seller_nip is nil (and all other critical fields present) and assert the function returns :partial, and similarly add a test where invoice_number is nil and assert :partial; reference the existing describe block and the helper all_critical_fields_present?/1 (and update_invoice_fields tests) to ensure identity-field coverage aligns with the amount-field cases.test/ksef_hub_web/live/invoice_live/show_test.exs (2)
335-362: Flash and validation text assertions also use raw HTML matching.Lines 358 and 379 use
html =~for flash message and changeset error text. Preferhas_element?/3with a text selector where the structure allows it, and useassert has_element?(view, "[role=alert]", "Invoice updated")or adata-testidattribute for the flash container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/show_test.exs` around lines 335 - 362, Update the assertions in the "saving edit updates invoice and exits edit mode" test to avoid raw HTML matching: replace the flash assertion `assert html =~ "Invoice updated"` with a DOM query like `assert has_element?(view, "[role=alert]", "Invoice updated")` (or target a flash container using a `data-testid` if present), and replace the changeset/validation assertion `refute html =~ "missing data"` with a DOM query such as `refute has_element?(view, ".changeset-error", "missing data")` or an appropriate selector for the validation message; keep using the same `view`, `form("form[phx-submit=save_edit]")`, and `render_submit()` calls but switch to `has_element?/3` checks for both the success flash and the validation text.
253-294: Raw HTML assertions (html =~) violate the LiveView testing guideline.Lines 261–262 and 271–272 use
html =~to assert text content. As per coding guidelines, "Never test against raw HTML — always useelement/2,has_element?/2, and similar functions with selectors."♻️ Suggested approach
- assert html =~ "Incomplete" - assert html =~ "missing data" + assert has_element?(view, "span", "Incomplete") + assert has_element?(view, "[data-testid=extraction-warning]") - refute html =~ "Incomplete" - refute html =~ "missing data" + refute has_element?(view, "span", "Incomplete") + refute has_element?(view, "[data-testid=extraction-warning]")Add a
data-testid="extraction-warning"attribute to the warning banner element in the show template to make it selector-addressable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/show_test.exs` around lines 253 - 294, Replace raw HTML substring assertions in invoice_live show tests with selector-based checks: add a data-testid="extraction-warning" attribute to the warning banner in the show template (where extraction_status :partial is rendered), then update the tests in test/ksef_hub_web/live/invoice_live/show_test.exs to use LiveView helpers instead of html =~ — e.g. after mounting the view assert has_element?(view, ~s([data-testid="extraction-warning"])) and assert render(element(view, ~s([data-testid="extraction-warning"]))) =~ "missing data" for the partial case, and refute has_element?(view, ~s([data-testid="extraction-warning"])) for the complete case; keep the Approve button test logic but target the banner via the same data-testid when asserting the specific error text if needed.test/ksef_hub/invoices/parser_test.exs (1)
81-137: Move inline XML to a fixture file.The 49-line inline XML string should be extracted to
test/support/fixtures/sample_8pct_vat.xmland loaded withFile.read!/1, consistent with the pattern used for all other parser tests. Based on learnings, "Store sample FA(3) XML test fixtures intest/support/fixtures/including valid invoices and edge cases."♻️ Suggested approach
- test "parses invoice with only 8% VAT rate (no P_13_1)" do - # Build XML with only P_13_2 and P_14_2 (8% rate), no 23% fields - xml = """ - ...49 lines of XML... - """ + test "parses invoice with only 8% VAT rate (no P_13_1)" do + xml = File.read!(Path.join(`@fixtures_path`, "sample_8pct_vat.xml"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices/parser_test.exs` around lines 81 - 137, In the test "parses invoice with only 8% VAT rate (no P_13_1)" replace the large inline XML string assigned to xml with a fixture load (e.g. xml = File.read!("sample_8pct_vat.xml")): extract the 49-line XML content into a new fixture file named sample_8pct_vat.xml and update the test to read that file before calling Parser.parse, leaving the rest of the test (asserts using invoice.net_amount, invoice.vat_amount, invoice.gross_amount and the Parser.parse call) unchanged.lib/ksef_hub_web/live/invoice_live/show.ex (1)
385-587: Extract the edit form block into a function component.This section is now very large and mixes page-level concerns with form rendering details; moving it to a dedicated component will reduce render complexity and improve testability.
Based on learnings: "Applies to **/*_live.ex : Extract reusable logic into function components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 385 - 587, The edit form block is too large—extract it into a function component (e.g., invoice_edit_form/1) that accepts assigns (at minimum :edit_form and :phx_target if needed) and renders the form with the same inputs and event handlers (phx-change "validate_edit", phx-submit "save_edit", phx-click "cancel_edit") using the same field names (`@edit_form`[:...].name/value/errors) and classes; then replace the big inline <% if `@editing` do %> ... </.form> block with a call to the new component (<.invoice_edit_form edit_form={`@edit_form`} />) so the show LiveView (symbols: `@editing`, `@edit_form`, validate_edit, save_edit, cancel_edit) delegates rendering to the component. Ensure the component is defined in the same module or imported and keeps identical behavior/assigns and error rendering (.field_error).
🤖 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/invoice_live/show.ex`:
- Around line 85-91: Update the flash text used in the {:error,
:incomplete_extraction} clause in invoice_live/show.ex to accurately reflect
that approval is blocked by the broader extraction_status completeness checks
rather than only net/gross amounts; modify the message produced by put_flash
(the branch matching {:error, :incomplete_extraction} that uses
put_flash(socket, :error, ...)) to state that required extracted fields are
incomplete per the extraction_status and advise the user to review and complete
all missing extracted fields before approving.
In `@lib/ksef_hub/invoices.ex`:
- Line 245: The extraction status is being computed from raw merged attrs which
treats whitespace-only strings as present; update the logic so extraction_status
is derived from normalized values: trim all string values in the merged attrs
(and convert empty/whitespace-only strings to nil) before calling
determine_extraction_status_from_attrs. Locate all call sites that pass
merged/raw attrs (e.g., the new_status =
determine_extraction_status_from_attrs(merged) call and the other similar calls
around the determine_extraction_status_from_attrs usage) and either normalize
the attrs in-place (trim/blank? -> nil) or add normalization inside
determine_extraction_status_from_attrs so it always evaluates trimmed/clean
values. Ensure the normalization handles nested maps if applicable and that
tests covering whitespace-only fields are updated accordingly.
In `@lib/ksef_hub/invoices/invoice.ex`:
- Line 146: The changeset currently allows external assignment of
extraction_status by including :extraction_status in the cast call (cast(attrs,
`@edit_fields` ++ [:extraction_status])); remove :extraction_status from the cast
so it cannot be written from params (use only `@edit_fields`), and instead set
extraction_status programmatically where appropriate (e.g., via put_change/3 or
change in the specific function that derives it). Update the changeset function
that uses `@edit_fields` and any callers that need to set extraction_status to
assign it explicitly after casting.
---
Nitpick comments:
In `@lib/ksef_hub_web/components/invoice_components.ex`:
- Around line 102-111: The extraction_badge/1 currently treats any non-:partial
status as "Failed" which mislabels unknown statuses; update extraction_badge
(use the `@status` assign) to pattern-match explicitly (e.g., handle :partial and
:failed cases) using a case or cond and provide a clear fallback that either
raises (or logs and shows a neutral/unknown badge) for unexpected atoms so new
extraction_status values are not silently rendered as "Failed".
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 385-587: The edit form block is too large—extract it into a
function component (e.g., invoice_edit_form/1) that accepts assigns (at minimum
:edit_form and :phx_target if needed) and renders the form with the same inputs
and event handlers (phx-change "validate_edit", phx-submit "save_edit",
phx-click "cancel_edit") using the same field names
(`@edit_form`[:...].name/value/errors) and classes; then replace the big inline <%
if `@editing` do %> ... </.form> block with a call to the new component
(<.invoice_edit_form edit_form={`@edit_form`} />) so the show LiveView (symbols:
`@editing`, `@edit_form`, validate_edit, save_edit, cancel_edit) delegates rendering
to the component. Ensure the component is defined in the same module or imported
and keeps identical behavior/assigns and error rendering (.field_error).
In `@lib/ksef_hub/sync/invoice_fetcher.ex`:
- Around line 165-167: The pipeline uses then/2 with a one-line Map.put which
can be simplified: replace the |> then(fn a -> Map.put(a, :extraction_status,
Invoices.determine_extraction_status_from_attrs(a)) end) usage with a direct |>
Map.put(:extraction_status,
Invoices.determine_extraction_status_from_attrs(...)) style by calling Map.put
directly after the prior pipeline value; locate the occurrence by the then/2
call and the referenced Invoices.determine_extraction_status_from_attrs function
and inline the transformation to remove the anonymous fn wrapper for brevity and
readability.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 335-362: Update the assertions in the "saving edit updates invoice
and exits edit mode" test to avoid raw HTML matching: replace the flash
assertion `assert html =~ "Invoice updated"` with a DOM query like `assert
has_element?(view, "[role=alert]", "Invoice updated")` (or target a flash
container using a `data-testid` if present), and replace the
changeset/validation assertion `refute html =~ "missing data"` with a DOM query
such as `refute has_element?(view, ".changeset-error", "missing data")` or an
appropriate selector for the validation message; keep using the same `view`,
`form("form[phx-submit=save_edit]")`, and `render_submit()` calls but switch to
`has_element?/3` checks for both the success flash and the validation text.
- Around line 253-294: Replace raw HTML substring assertions in invoice_live
show tests with selector-based checks: add a data-testid="extraction-warning"
attribute to the warning banner in the show template (where extraction_status
:partial is rendered), then update the tests in
test/ksef_hub_web/live/invoice_live/show_test.exs to use LiveView helpers
instead of html =~ — e.g. after mounting the view assert has_element?(view,
~s([data-testid="extraction-warning"])) and assert render(element(view,
~s([data-testid="extraction-warning"]))) =~ "missing data" for the partial case,
and refute has_element?(view, ~s([data-testid="extraction-warning"])) for the
complete case; keep the Approve button test logic but target the banner via the
same data-testid when asserting the specific error text if needed.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 708-747: Add tests to cover missing identity fields for
determine_extraction_status_from_attrs/1: create at least one test where
seller_nip is nil (and all other critical fields present) and assert the
function returns :partial, and similarly add a test where invoice_number is nil
and assert :partial; reference the existing describe block and the helper
all_critical_fields_present?/1 (and update_invoice_fields tests) to ensure
identity-field coverage aligns with the amount-field cases.
In `@test/ksef_hub/invoices/parser_test.exs`:
- Around line 81-137: In the test "parses invoice with only 8% VAT rate (no
P_13_1)" replace the large inline XML string assigned to xml with a fixture load
(e.g. xml = File.read!("sample_8pct_vat.xml")): extract the 49-line XML content
into a new fixture file named sample_8pct_vat.xml and update the test to read
that file before calling Parser.parse, leaving the rest of the test (asserts
using invoice.net_amount, invoice.vat_amount, invoice.gross_amount and the
Parser.parse call) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/invoices/parser.exlib/ksef_hub/sync/invoice_fetcher.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub/invoices/parser_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/support/fixtures/sample_mixed_vat.xml
| def update_invoice_fields(%Invoice{} = invoice, attrs) do | ||
| old_status = invoice.extraction_status | ||
| merged = invoice |> Map.from_struct() |> Map.merge(atomize_known_keys(attrs)) | ||
| new_status = determine_extraction_status_from_attrs(merged) |
There was a problem hiding this comment.
Derive extraction_status from normalized values to avoid false :complete.
On Line 245 and Line 268/279, status is computed from raw merged attrs. Whitespace-only strings are currently treated as present, which can incorrectly flip status to :complete.
💡 Proposed fix
def update_invoice_fields(%Invoice{} = invoice, attrs) do
old_status = invoice.extraction_status
merged = invoice |> Map.from_struct() |> Map.merge(atomize_known_keys(attrs))
new_status = determine_extraction_status_from_attrs(merged)
changeset =
Invoice.edit_changeset(invoice, Map.put(attrs, "extraction_status", new_status))
@@
defp all_critical_fields_present?(map) do
Enum.all?(`@critical_extraction_fields`, fn field ->
value = Map.get(map, field) || Map.get(map, Atom.to_string(field))
- value != nil && value != ""
+ present_value?(value)
end)
end
+
+@spec present_value?(term()) :: boolean()
+defp present_value?(value) when is_binary(value), do: String.trim(value) != ""
+defp present_value?(nil), do: false
+defp present_value?(_), do: trueAlso applies to: 267-269, 279-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` at line 245, The extraction status is being
computed from raw merged attrs which treats whitespace-only strings as present;
update the logic so extraction_status is derived from normalized values: trim
all string values in the merged attrs (and convert empty/whitespace-only strings
to nil) before calling determine_extraction_status_from_attrs. Locate all call
sites that pass merged/raw attrs (e.g., the new_status =
determine_extraction_status_from_attrs(merged) call and the other similar calls
around the determine_extraction_status_from_attrs usage) and either normalize
the attrs in-place (trim/blank? -> nil) or add normalization inside
determine_extraction_status_from_attrs so it always evaluates trimmed/clean
values. Ensure the normalization handles nested maps if applicable and that
tests covering whitespace-only fields are updated accordingly.
Inline fixes: - Update incomplete_extraction flash message to reference extraction status broadly rather than specific fields - Add whitespace normalization in all_critical_fields_present? so whitespace-only strings are treated as missing - Remove :extraction_status from edit_changeset cast; set it via put_change in the caller to prevent external assignment Nitpicks: - Pattern-match extraction_badge explicitly on :partial and :failed so unexpected atoms raise FunctionClauseError - Extract 135-line edit form into invoice_edit_form/1 component - Replace then/2 wrapper with direct Map.put in invoice_fetcher - Add data-testid to extraction warning banner; use selector-based assertions in LiveView tests - Add determine_extraction_status_from_attrs tests for missing seller_nip, invoice_number, and whitespace-only strings - Extract inline 8% VAT XML to sample_8pct_vat.xml fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/ksef_hub/invoices/parser_test.exs (1)
82-82: Use@fixtures_pathconsistently in the new fixture test.The hardcoded path works, but using the shared module attribute keeps fixture loading uniform and easier to refactor.
♻️ Small consistency tweak
- xml = File.read!("test/support/fixtures/sample_8pct_vat.xml") + xml = File.read!(Path.join(`@fixtures_path`, "sample_8pct_vat.xml"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices/parser_test.exs` at line 82, Replace the hardcoded fixture path in the test where xml is assigned (the xml = File.read!(... ) line in parser_test.exs) with the shared module attribute `@fixtures_path` so the test uses File.read!(Path.join(`@fixtures_path`, "sample_8pct_vat.xml")) (or equivalent) to keep fixture loading consistent with other tests and enable easier refactoring.lib/ksef_hub_web/live/invoice_live/show.ex (1)
553-553: Add a typespec forinvoice_edit_form/1.This new private function component is missing a
@spec, while the file otherwise keeps specs consistent.🧩 Suggested addition
+ `@spec` invoice_edit_form(map()) :: Phoenix.LiveView.Rendered.t() defp invoice_edit_form(assigns) doAs per coding guidelines: "Every function (public and private) must have a
@spectypespec."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` at line 553, Add a `@spec` for the private function invoice_edit_form/1: declare its parameter type (assigns :: map() or more specifically Phoenix.LiveView.Socket.assigns/struct if available) and its return type as Phoenix.LiveView.Rendered.t() (the rendered HEEx/component result). Place the `@spec` immediately above defp invoice_edit_form(assigns) and keep it consistent with other specs in the file.
🤖 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/invoices.ex`:
- Around line 237-239: The docstring starting "Updates invoice fields from a
manual edit, recalculates extraction_status, and enqueues prediction if status
changed from partial to complete." is out of date; update it to state that the
function enqueues a prediction when extraction_status transitions from :partial
or :failed to :complete (not just :partial). Modify the docstring in
lib/ksef_hub/invoices.ex for the function whose docstring begins with that text
so it accurately reflects the enqueue transition rule (i.e., mention both
:partial and :failed → :complete).
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Line 553: Add a `@spec` for the private function invoice_edit_form/1: declare
its parameter type (assigns :: map() or more specifically
Phoenix.LiveView.Socket.assigns/struct if available) and its return type as
Phoenix.LiveView.Rendered.t() (the rendered HEEx/component result). Place the
`@spec` immediately above defp invoice_edit_form(assigns) and keep it consistent
with other specs in the file.
In `@test/ksef_hub/invoices/parser_test.exs`:
- Line 82: Replace the hardcoded fixture path in the test where xml is assigned
(the xml = File.read!(... ) line in parser_test.exs) with the shared module
attribute `@fixtures_path` so the test uses File.read!(Path.join(`@fixtures_path`,
"sample_8pct_vat.xml")) (or equivalent) to keep fixture loading consistent with
other tests and enable easier refactoring.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/sync/invoice_fetcher.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub/invoices/parser_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/support/fixtures/sample_8pct_vat.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ksef_hub_web/live/invoice_live/show_test.exs
- Fix docstring for update_invoice_fields/2 to mention both :partial and :failed → :complete transition - Add @SPEC to invoice_edit_form/1 private component - Use @fixtures_path module attribute for sample_8pct_vat.xml fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ksef_hub/invoices.ex (1)
244-245:⚠️ Potential issue | 🟠 Major
extraction_statuscan still be miscomputed when merged maps contain both atom and string keys.Line 452 currently prefers atom keys first. If
atomize_known_keys/1falls back to raw string attrs (e.g., one unknown key), stale atom values can override incoming edits and incorrectly keep:complete.💡 Proposed hardening
defp all_critical_fields_present?(map) do Enum.all?(`@critical_extraction_fields`, fn field -> - value = Map.get(map, field) || Map.get(map, Atom.to_string(field)) + value = extraction_field_value(map, field) present_value?(value) end) end + +@spec extraction_field_value(map(), atom()) :: term() +defp extraction_field_value(map, field) do + string_key = Atom.to_string(field) + + if Map.has_key?(map, string_key) do + Map.get(map, string_key) + else + Map.get(map, field) + end +endAlso applies to: 269-270, 452-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` around lines 244 - 245, The merge can miscompute extraction_status because Map.from_struct() and atomize_known_keys(attrs) may use mixed key types so stale atom keys override incoming string-keyed edits; normalize keys to a single canonical form before merging (e.g., convert both the struct_map produced by Map.from_struct() and the attrs map returned by atomize_known_keys/1 to the same key type), and ensure the merge order preserves incoming attrs precedence when calling determine_extraction_status_from_attrs/1; also harden atomize_known_keys/1 so it never falls back to leaving unknown keys as strings (or document/handle that case) and add a small helper (e.g., normalize_keys/1) used at all three locations (the merged assignments around determine_extraction_status_from_attrs) to avoid duplicate mistakes.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
554-692: Consider splittinginvoice_edit_form/1into smaller field-group components.This block is getting large; extracting seller/buyer/amount sections would reduce cognitive load and simplify future edits.
Based on learnings
Applies to **/*_live.ex : Extract reusable logic into function components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 554 - 692, Split the large invoice_edit_form/1 HEEx template by extracting the Seller, Buyer and Amounts blocks into separate function components (e.g. seller_fields(assigns), buyer_fields(assigns), amounts_fields(assigns)) that accept the same assigns (or `@edit_form` subset) and render their respective inputs and <.field_error/> calls; then replace the inline blocks in invoice_edit_form/1 with calls to these components (e.g. <.seller_fields edit_form={`@edit_form`}/>), keeping the form tag, phx handlers and submit buttons in invoice_edit_form/1 so behavior is unchanged.
🤖 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/invoice_live/show.ex`:
- Around line 182-227: Add a single `@spec` for handle_event/3 above the first
handle_event clause that covers all three new clauses; e.g. declare
handle_event(event :: String.t(), params :: map() | any(), socket ::
Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()} so the
public callback has a typespec and matches the return shape used in the module
(reference the existing handle_event/3 function clauses and the
Phoenix.LiveView.Socket type).
---
Duplicate comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 244-245: The merge can miscompute extraction_status because
Map.from_struct() and atomize_known_keys(attrs) may use mixed key types so stale
atom keys override incoming string-keyed edits; normalize keys to a single
canonical form before merging (e.g., convert both the struct_map produced by
Map.from_struct() and the attrs map returned by atomize_known_keys/1 to the same
key type), and ensure the merge order preserves incoming attrs precedence when
calling determine_extraction_status_from_attrs/1; also harden
atomize_known_keys/1 so it never falls back to leaving unknown keys as strings
(or document/handle that case) and add a small helper (e.g., normalize_keys/1)
used at all three locations (the merged assignments around
determine_extraction_status_from_attrs) to avoid duplicate mistakes.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 554-692: Split the large invoice_edit_form/1 HEEx template by
extracting the Seller, Buyer and Amounts blocks into separate function
components (e.g. seller_fields(assigns), buyer_fields(assigns),
amounts_fields(assigns)) that accept the same assigns (or `@edit_form` subset) and
render their respective inputs and <.field_error/> calls; then replace the
inline blocks in invoice_edit_form/1 with calls to these components (e.g.
<.seller_fields edit_form={`@edit_form`}/>), keeping the form tag, phx handlers
and submit buttons in invoice_edit_form/1 so behavior is unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/ksef_hub/invoices.exlib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub/invoices/parser_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ksef_hub/invoices/parser_test.exs
- Add @SPEC for handle_event/3 matching codebase convention - Fix atomize_known_keys to rescue per-key instead of per-map, preventing mixed key types when an unknown string key is encountered - Split invoice_edit_form into seller_fields, buyer_fields, and amount_fields sub-components for readability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
lib/ksef_hub/invoices.ex (1)
245-245: Whitespace-normalisation concern from previous review is now resolved.The addition of
present_value?/1(Lines 462–465) and its integration intoall_critical_fields_present?/1(Line 458) correctly addresses the earlier flag that whitespace-only strings could produce a false:completestatus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` at line 245, The whitespace-only value issue is fixed by adding present_value?/1 and using it inside all_critical_fields_present?/1; ensure determine_extraction_status_from_attrs(merged) (the call that sets new_status) relies on that updated all_critical_fields_present?/1 so whitespace-only strings are treated as absent—verify present_value?/1 exists and is referenced by all_critical_fields_present?/1 and that determine_extraction_status_from_attrs/1 uses that predicate when computing :complete vs :incomplete.lib/ksef_hub_web/live/invoice_live/show.ex (2)
74-75:@spec handle_event/3added correctly — LGTM.All new
handle_event/3clauses are now covered by the single spec at Lines 74–75, consistent with the module's existinghandle_eventreturn shape.Also applies to: 184-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 74 - 75, The module has a single `@spec` for handle_event/3 at the top of the first clause but the other handle_event/3 clauses (the ones around the second group at lines 184–228) lack the same spec annotation; add the same `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()} immediately before the second group of handle_event/3 clauses (or move the existing `@spec` to a shared place above all handle_event definitions) so all handle_event/3 clauses are clearly covered by the spec.
87-93::incomplete_extractionflash message updated correctly — LGTM.The message now accurately reflects the broader extraction-status completeness requirement rather than only net/gross amounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 87 - 93, No change required: the flash message in the handle for {:error, :incomplete_extraction} (the put_flash call in InvoiceLive.Show's clause) was updated correctly to reflect full extraction completeness; approve and keep the updated string as-is and ensure the same (:error) flash key is used throughout related approval flows.
🧹 Nitpick comments (2)
lib/ksef_hub/invoices.ex (1)
449-452:determine_extraction_status/1duplicatesdetermine_extraction_status_from_attrs/1— delegate instead.The private
determine_extraction_status/1(Line 449–452) has a body identical to the new publicdetermine_extraction_status_from_attrs/1(Line 280–282).♻️ Proposed fix
`@spec` determine_extraction_status(map()) :: atom() defp determine_extraction_status(extracted) do - if all_critical_fields_present?(extracted), do: :complete, else: :partial + determine_extraction_status_from_attrs(extracted) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` around lines 449 - 452, The private function determine_extraction_status/1 duplicates logic from determine_extraction_status_from_attrs/1; replace its body to delegate to the existing implementation instead of repeating the check (i.e., have determine_extraction_status(extracted) call determine_extraction_status_from_attrs(extracted)), keeping the private function signature so callers remain unchanged and relying on all_critical_fields_present?/1 only via the delegated function.lib/ksef_hub_web/live/invoice_live/show.ex (1)
558-562: Add an explicitidto the edit form to avoid potential LiveView diffing collisions.Without an explicit
id, Phoenix derives one from the form name ("invoice"). If a second component on the same page (now or in the future) also usesas: :invoice, LiveView will have two elements withid="invoice", which breaks DOM patching and form feedback.💡 Proposed fix
<.form for={`@edit_form`} + id="invoice-edit-form" phx-change="validate_edit" phx-submit="save_edit" class="space-y-3" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 558 - 562, The edit form rendered with .form for={`@edit_form`} (handling "validate_edit" and "save_edit") lacks an explicit id which can collide with other forms named "invoice"; update the .form call to add a unique id (e.g., "edit-invoice-form" or similar) so the form element has an explicit id attribute to prevent LiveView diffing collisions and ensure correct DOM patching and form feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 74-75: The module has a single `@spec` for handle_event/3 at the top
of the first clause but the other handle_event/3 clauses (the ones around the
second group at lines 184–228) lack the same spec annotation; add the same `@spec`
handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply,
Phoenix.LiveView.Socket.t()} immediately before the second group of
handle_event/3 clauses (or move the existing `@spec` to a shared place above all
handle_event definitions) so all handle_event/3 clauses are clearly covered by
the spec.
- Around line 87-93: No change required: the flash message in the handle for
{:error, :incomplete_extraction} (the put_flash call in InvoiceLive.Show's
clause) was updated correctly to reflect full extraction completeness; approve
and keep the updated string as-is and ensure the same (:error) flash key is used
throughout related approval flows.
In `@lib/ksef_hub/invoices.ex`:
- Line 245: The whitespace-only value issue is fixed by adding present_value?/1
and using it inside all_critical_fields_present?/1; ensure
determine_extraction_status_from_attrs(merged) (the call that sets new_status)
relies on that updated all_critical_fields_present?/1 so whitespace-only strings
are treated as absent—verify present_value?/1 exists and is referenced by
all_critical_fields_present?/1 and that determine_extraction_status_from_attrs/1
uses that predicate when computing :complete vs :incomplete.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 558-562: The edit form rendered with .form for={`@edit_form`}
(handling "validate_edit" and "save_edit") lacks an explicit id which can
collide with other forms named "invoice"; update the .form call to add a unique
id (e.g., "edit-invoice-form" or similar) so the form element has an explicit id
attribute to prevent LiveView diffing collisions and ensure correct DOM patching
and form feedback.
In `@lib/ksef_hub/invoices.ex`:
- Around line 449-452: The private function determine_extraction_status/1
duplicates logic from determine_extraction_status_from_attrs/1; replace its body
to delegate to the existing implementation instead of repeating the check (i.e.,
have determine_extraction_status(extracted) call
determine_extraction_status_from_attrs(extracted)), keeping the private function
signature so callers remain unchanged and relying on
all_critical_fields_present?/1 only via the delegated function.
- Add id="edit-invoice-form" to edit form to prevent DOM patching issues - Delegate determine_extraction_status/1 to determine_extraction_status_from_attrs/1 instead of duplicating the logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/invoice_live/show.ex`:
- Around line 553-719: Add programmatic label associations by giving each input
an id and matching each label a for attribute in invoice_edit_form/1,
seller_fields/1, buyer_fields/1, and amount_fields/1; for each field (e.g.
:invoice_number, :issue_date, :seller_name, :seller_nip, :buyer_name,
:buyer_nip, :net_amount, :vat_amount, :gross_amount, :currency) set the input id
to a unique string derived from the field key and update the corresponding
<label> to include for="that-id" so screen readers and click-to-focus work
correctly across all forms. Ensure ids are unique within the page and use the
same mapping for phx form names so existing form handling
(validate_edit/save_edit) continues to work.
In `@lib/ksef_hub/invoices.ex`:
- Around line 268-271: recalculate_extraction_status/2 currently always writes
:extraction_status as an atom which can produce mixed atom/string keyed maps;
change it to detect the incoming attrs key type (e.g., check if any
Map.keys(attrs) are binaries via Enum.any?(Map.keys(attrs), &is_binary/1)) and
then insert the computed extraction status under a string key
"extraction_status" when attrs is string-keyed, otherwise keep the atom
:extraction_status; alternatively you can insert both "extraction_status" and
:extraction_status to be defensive. Use the existing
determine_extraction_status_from_attrs(merged) result and Map.put/Map.put_new
with the chosen key(s) in recalculate_extraction_status/2.
Comment on lines
+553
to
+719
| attr :edit_form, :map, required: true | ||
|
|
||
| @spec invoice_edit_form(map()) :: Phoenix.LiveView.Rendered.t() | ||
| defp invoice_edit_form(assigns) do | ||
| ~H""" | ||
| <.form | ||
| for={@edit_form} | ||
| id="edit-invoice-form" | ||
| phx-change="validate_edit" | ||
| phx-submit="save_edit" | ||
| class="space-y-3" | ||
| > | ||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Invoice Number</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:invoice_number].name} | ||
| value={@edit_form[:invoice_number].value} | ||
| class="input input-sm input-bordered" | ||
| /> | ||
| <.field_error errors={@edit_form[:invoice_number].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Issue Date</span></label> | ||
| <input | ||
| type="date" | ||
| name={@edit_form[:issue_date].name} | ||
| value={@edit_form[:issue_date].value} | ||
| class="input input-sm input-bordered" | ||
| /> | ||
| <.field_error errors={@edit_form[:issue_date].errors} /> | ||
| </div> | ||
|
|
||
| <.seller_fields edit_form={@edit_form} /> | ||
| <.buyer_fields edit_form={@edit_form} /> | ||
| <.amount_fields edit_form={@edit_form} /> | ||
|
|
||
| <div class="flex gap-2 pt-2"> | ||
| <button type="submit" class="btn btn-sm btn-primary">Save</button> | ||
| <button type="button" phx-click="cancel_edit" class="btn btn-sm btn-ghost"> | ||
| Cancel | ||
| </button> | ||
| </div> | ||
| </.form> | ||
| """ | ||
| end | ||
|
|
||
| attr :edit_form, :map, required: true | ||
|
|
||
| @spec seller_fields(map()) :: Phoenix.LiveView.Rendered.t() | ||
| defp seller_fields(assigns) do | ||
| ~H""" | ||
| <div class="divider text-xs my-1">Seller</div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Seller Name</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:seller_name].name} | ||
| value={@edit_form[:seller_name].value} | ||
| class="input input-sm input-bordered" | ||
| /> | ||
| <.field_error errors={@edit_form[:seller_name].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Seller NIP</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:seller_nip].name} | ||
| value={@edit_form[:seller_nip].value} | ||
| class="input input-sm input-bordered" | ||
| maxlength="10" | ||
| /> | ||
| <.field_error errors={@edit_form[:seller_nip].errors} /> | ||
| </div> | ||
| """ | ||
| end | ||
|
|
||
| attr :edit_form, :map, required: true | ||
|
|
||
| @spec buyer_fields(map()) :: Phoenix.LiveView.Rendered.t() | ||
| defp buyer_fields(assigns) do | ||
| ~H""" | ||
| <div class="divider text-xs my-1">Buyer</div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Buyer Name</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:buyer_name].name} | ||
| value={@edit_form[:buyer_name].value} | ||
| class="input input-sm input-bordered" | ||
| /> | ||
| <.field_error errors={@edit_form[:buyer_name].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Buyer NIP</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:buyer_nip].name} | ||
| value={@edit_form[:buyer_nip].value} | ||
| class="input input-sm input-bordered" | ||
| maxlength="10" | ||
| /> | ||
| <.field_error errors={@edit_form[:buyer_nip].errors} /> | ||
| </div> | ||
| """ | ||
| end | ||
|
|
||
| attr :edit_form, :map, required: true | ||
|
|
||
| @spec amount_fields(map()) :: Phoenix.LiveView.Rendered.t() | ||
| defp amount_fields(assigns) do | ||
| ~H""" | ||
| <div class="divider text-xs my-1">Amounts</div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Netto</span></label> | ||
| <input | ||
| type="text" | ||
| inputmode="decimal" | ||
| name={@edit_form[:net_amount].name} | ||
| value={@edit_form[:net_amount].value} | ||
| class="input input-sm input-bordered font-mono" | ||
| /> | ||
| <.field_error errors={@edit_form[:net_amount].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">VAT</span></label> | ||
| <input | ||
| type="text" | ||
| inputmode="decimal" | ||
| name={@edit_form[:vat_amount].name} | ||
| value={@edit_form[:vat_amount].value} | ||
| class="input input-sm input-bordered font-mono" | ||
| /> | ||
| <.field_error errors={@edit_form[:vat_amount].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Brutto</span></label> | ||
| <input | ||
| type="text" | ||
| inputmode="decimal" | ||
| name={@edit_form[:gross_amount].name} | ||
| value={@edit_form[:gross_amount].value} | ||
| class="input input-sm input-bordered font-mono" | ||
| /> | ||
| <.field_error errors={@edit_form[:gross_amount].errors} /> | ||
| </div> | ||
|
|
||
| <div class="form-control"> | ||
| <label class="label"><span class="label-text text-xs">Currency</span></label> | ||
| <input | ||
| type="text" | ||
| name={@edit_form[:currency].name} | ||
| value={@edit_form[:currency].value} | ||
| class="input input-sm input-bordered" | ||
| maxlength="3" | ||
| /> | ||
| <.field_error errors={@edit_form[:currency].errors} /> | ||
| </div> | ||
| """ |
There was a problem hiding this comment.
Accessibility: labels are not programmatically associated with their inputs.
None of the <input> elements in invoice_edit_form/1, seller_fields/1, buyer_fields/1, or amount_fields/1 have an id attribute, and none of the <label> elements have a for attribute. Without this linkage, screen readers cannot announce which label belongs to which field, and clicking the label will not focus the input.
♿ Proposed fix — add matching `for`/`id` pairs (example for one field group; apply the same pattern throughout)
- <label class="label"><span class="label-text text-xs">Invoice Number</span></label>
+ <label class="label" for="edit-invoice-number">
+ <span class="label-text text-xs">Invoice Number</span>
+ </label>
<input
type="text"
+ id="edit-invoice-number"
name={`@edit_form`[:invoice_number].name}
value={`@edit_form`[:invoice_number].value}
class="input input-sm input-bordered"
/>Apply the same id/for pairing to every field in seller_fields, buyer_fields, and amount_fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 553 - 719, Add
programmatic label associations by giving each input an id and matching each
label a for attribute in invoice_edit_form/1, seller_fields/1, buyer_fields/1,
and amount_fields/1; for each field (e.g. :invoice_number, :issue_date,
:seller_name, :seller_nip, :buyer_name, :buyer_nip, :net_amount, :vat_amount,
:gross_amount, :currency) set the input id to a unique string derived from the
field key and update the corresponding <label> to include for="that-id" so
screen readers and click-to-focus work correctly across all forms. Ensure ids
are unique within the page and use the same mapping for phx form names so
existing form handling (validate_edit/save_edit) continues to work.
Comment on lines
268
to
+271
| def recalculate_extraction_status(%Invoice{} = invoice, attrs) do | ||
| atom_attrs = atomize_known_keys(attrs) | ||
| merged = Map.merge(Map.from_struct(invoice), atom_attrs) | ||
| new_status = if all_critical_fields_present?(merged), do: :complete, else: :partial | ||
| Map.put(attrs, :extraction_status, new_status) | ||
| merged = invoice |> Map.from_struct() |> Map.merge(atomize_known_keys(attrs)) | ||
| Map.put(attrs, :extraction_status, determine_extraction_status_from_attrs(merged)) | ||
| end |
There was a problem hiding this comment.
recalculate_extraction_status/2 inserts an atom-keyed :extraction_status into a potentially string-keyed attrs map.
Map.put(attrs, :extraction_status, ...) always uses an atom key. If the caller passes string-keyed attrs (e.g., from a Phoenix form submission or KSeF sync where map keys are strings), the returned map has mixed atom/string keys. Any downstream code reading attrs["extraction_status"] will see nil while attrs[:extraction_status] returns the value. Callers that pass the result directly to Invoice.changeset/2 are unaffected since cast/3 normalises keys, but callers that inspect the key directly could silently miss it.
💡 Proposed fix – normalise to the same key type as the input
`@spec` recalculate_extraction_status(Invoice.t(), map()) :: map()
def recalculate_extraction_status(%Invoice{} = invoice, attrs) do
merged = invoice |> Map.from_struct() |> Map.merge(atomize_known_keys(attrs))
- Map.put(attrs, :extraction_status, determine_extraction_status_from_attrs(merged))
+ status = determine_extraction_status_from_attrs(merged)
+
+ if Map.has_key?(attrs, "extraction_status") or
+ (not Map.has_key?(attrs, :extraction_status) and has_string_keys?(attrs)) do
+ Map.put(attrs, "extraction_status", status)
+ else
+ Map.put(attrs, :extraction_status, status)
+ endAlternatively, always insert under both keys, or document that callers must treat the return as atom-keyed. The simplest fix if all callers in the sync path use atom-keyed maps is to keep the current behaviour and add a note to the spec/doc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` around lines 268 - 271,
recalculate_extraction_status/2 currently always writes :extraction_status as an
atom which can produce mixed atom/string keyed maps; change it to detect the
incoming attrs key type (e.g., check if any Map.keys(attrs) are binaries via
Enum.any?(Map.keys(attrs), &is_binary/1)) and then insert the computed
extraction status under a string key "extraction_status" when attrs is
string-keyed, otherwise keep the atom :extraction_status; alternatively you can
insert both "extraction_status" and :extraction_status to be defensive. Use the
existing determine_extraction_status_from_attrs(merged) result and
Map.put/Map.put_new with the chosen key(s) in recalculate_extraction_status/2.
Add id attributes to all edit form inputs and matching for attributes on labels so screen readers and click-to-focus work correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 2, 2026
Merged
Merged
Closed
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
Improvements
Tests
Bug Fixes