Feat/pdf invoice upload#50
Merged
Merged
Conversation
Add pdf_content (binary), extraction_status (string), and original_filename (string) columns to invoices table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Define behaviour for PDF extraction sidecar and implement HTTP client using multipart upload with Bearer token auth and 120s timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add pdf_content, extraction_status, original_filename fields. Add "pdf_upload" to valid sources with extraction status validation. Refactor changeset to use source-specific required fields: - ksef: xml_content + core fields - manual: core fields + buyer + amounts - pdf_upload: only pdf_content required Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add create_pdf_upload_invoice/3 with extraction, field mapping, and duplicate detection - Exclude pdf_content from list queries (like xml_content) - Add "pdf_upload" to source filter - Block approval of invoices with partial extraction status - Add helpers for date/decimal parsing from extraction data Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add unstructured_client config to config.exs and test mock to test.exs - Load UNSTRUCTURED_URL and UNSTRUCTURED_API_TOKEN in runtime.exs - Add Mox mock definition for Unstructured.Behaviour - Add StubService for unstructured extraction - Stub unstructured mock in DataCase and ConnCase setup - Add pdf_upload_invoice factory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- POST /api/invoices/upload — multipart PDF upload with extraction - PATCH /api/invoices/:id — update pdf_upload invoices, recalc status - Serve original PDF for pdf_upload invoices on GET pdf endpoint - Block approval of partial-extraction invoices - Increase Plug.Parsers upload limit to 12MB - Add extraction_status and original_filename to JSON response - Add UploadInvoiceRequest OpenAPI schema - Use pattern matching in function heads for source dispatch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Client unit tests for missing URL config - Context tests for complete/partial/failed extraction - Context tests for duplicate detection via extracted ksef_number - Context tests for approve blocking on partial extraction - Context tests for pdf_upload source filter and list exclusion - Controller tests for upload (201, 422, 415, 502 scenarios) - Controller tests for PATCH update on pdf_upload invoices - Controller tests for PDF/XML/HTML download behavior - Only enqueue predictions for complete extractions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add au-ksef-unstructured service to docker-compose.yml - ADR 0020: PDF invoice upload flow (Accepted) - ADR 0021: Future invoice documents table refactoring (Proposed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `if extraction_status == "complete"` string comparison with pattern-matched function heads in maybe_enqueue_prediction/2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Validate PDF magic bytes instead of trusting client Content-Type header - Move extraction status recalculation from controller to context (removes duplicated @critical_fields and inconsistent key types) - Block approval of invoices with "failed" extraction_status (not just "partial") - Add "pdf_upload" to OpenAPI source filter enum in index operation - Fix OpenAPI response schema: seller_nip/buyer_nip/issue_date now nullable - Fix wrong typespec on validate_file_present - Create dedicated UpdateInvoiceRequest schema (was reusing CreateInvoiceRequest) - Add filename length validation (max 255) in invoice changeset - Remove String.to_existing_atom fragility in extraction field lookup - Remove default arg on behaviour implementations (arity mismatch with Mox) - Simplify do_create_pdf_upload_failed (remove pass-through case wrapper) - Remove dead opts["type"] fallback in create_pdf_upload_invoice - Fix maybe_enqueue_prediction return typespec - Remove unnecessary id parameter threading through serve_pdf/do_pdf - Add tests: failed extraction approval, nil extraction approval, recalculate_extraction_status, income-type upload, invalid update data, non-PDF file magic bytes validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… tests Documents two review findings deferred from the PDF upload feature: 1. Duplicated create-then-retry-as-duplicate pattern in invoices context 2. Thin HTTP client test coverage for Unstructured and PredictionService Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the codebase-wide pattern of using :string fields with validate_inclusion for enum values, and proposes migrating to Ecto.Enum for compile-time safety and idiomatic atom-based pattern matching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds PDF invoice upload support: stores PDF binaries and original filename, calls an external unstructured extraction sidecar, tracks extraction_status (complete|partial|failed), exposes POST /invoices/upload and PATCH /invoices/:id, adjusts schemas/validation, adds DB migration, client behaviour/implementation, controller flow, tests, and ADRs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Controller as InvoiceController
participant Invoices as Invoices Module
participant Unstructured as Unstructured Sidecar
participant DB as Database
Client->>Controller: POST /invoices/upload (file + type)
activate Controller
Controller->>Controller: validate file (presence, type, magic bytes, size)
alt invalid
Controller-->>Client: 422 / 415
else valid
Controller->>Invoices: create_pdf_upload_invoice(company_id, pdf_binary, type)
activate Invoices
Invoices->>Unstructured: extract(pdf_binary, opts)
activate Unstructured
alt success
Unstructured-->>Invoices: {:ok, extracted_map}
else error
Unstructured-->>Invoices: {:error, reason}
end
deactivate Unstructured
Invoices->>Invoices: determine_extraction_status(extracted_map)
Invoices->>DB: insert/upsert invoice (with pdf_content, extraction_status...)
DB-->>Invoices: {:ok, invoice} | {:error, reason}
alt created && extraction complete
Invoices->>Invoices: maybe_enqueue_prediction(invoice)
end
Invoices-->>Controller: {:ok, invoice} | {:error, reason}
deactivate Invoices
Controller-->>Client: 201 Created / error
end
deactivate Controller
sequenceDiagram
participant Client as HTTP Client
participant Controller as InvoiceController
participant Invoices as Invoices Module
participant DB as Database
Client->>Controller: PATCH /invoices/:id (attrs)
activate Controller
Controller->>Invoices: recalculate_extraction_status(invoice, attrs)
activate Invoices
Invoices->>Invoices: merge attrs, check critical fields
Invoices-->>Controller: attrs with updated extraction_status
deactivate Invoices
Controller->>DB: update invoice
DB-->>Controller: {:ok, invoice} | {:error, reason}
Controller-->>Client: 200 OK / error
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 6
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/schemas/invoice.ex (1)
148-182:⚠️ Potential issue | 🟡 MinorExample doesn't demonstrate
pdf_upload-specific fields.The example uses
source: "ksef"but omitsextraction_statusandoriginal_filename. Consider adding a second example (or replacing this one with apdf_uploadvariant) to help API consumers understand the new fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/schemas/invoice.ex` around lines 148 - 182, Update the schema example in Invoice (module/schema defined in invoice.ex) to include a pdf_upload variant showing the new PDF-specific fields: add "source": "pdf_upload" and include "extraction_status" (e.g., "pending"|"completed"|"failed") and "original_filename" (string) in the example map; alternatively add a second example entry demonstrating both the existing ksef example and a pdf_upload example so API consumers can see the new fields alongside the existing fields like ksef_number and ksef_acquisition_date.
🧹 Nitpick comments (8)
docker-compose.yml (1)
13-14: Pin the image tag for reproducibility.Using
latestmeans the service version can silently drift between pulls, breaking reproducible dev environments. Pin to a specific digest or semantic version tag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 13 - 14, The docker-compose service "unstructured" is using the floating image tag "ghcr.io/appunite/au-ksef-unstructured:latest", which makes builds non-reproducible; update the image reference in the "unstructured" service to a fixed semantic version tag or an immutable digest (e.g., replace :latest with a specific version like :v1.2.3 or `@sha256`:<digest>) so pulls are deterministic and reproducible.lib/ksef_hub/unstructured/behaviour.ex (1)
1-13: LGTM — well-structured behaviour following the project pattern.Optional: Adding
@docannotations before each@callback(e.g. describing return shapes and error semantics) would improve discoverability in generated docs without any behavioral cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/unstructured/behaviour.ex` around lines 1 - 13, Add descriptive `@doc` annotations to the KsefHub.Unstructured.Behaviour module for each callback: place a short doc above `@callback` extract/2 describing the expected pdf_binary input, opts, the success return map shape and possible error forms, and another `@doc` above `@callback` health/0 describing the health map shape and error semantics; keep descriptions minimal and focused to improve generated docs without changing behaviour.lib/ksef_hub_web/endpoint.ex (1)
45-49: Scope the increased body limit to multipart only.
length: 12_000_000at thePlug.Parserslevel increases the maximum allowed body size for all parsers (urlencoded, multipart, and JSON). Only the multipart parser needs the expanded limit for PDF uploads. Plug supports per-parser length configuration via tuple syntax:{:multipart, length: 20_000_000}, which keeps the default 8 MB cap in place for JSON and urlencoded bodies.♻️ Proposed refactor
plug Plug.Parsers, - parsers: [:urlencoded, :multipart, :json], + parsers: [:urlencoded, {:multipart, length: 12_000_000}, :json], pass: ["*/*"], - length: 12_000_000, json_decoder: Phoenix.json_library()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/endpoint.ex` around lines 45 - 49, The global length: 12_000_000 on the Plug.Parsers plug is widening the body size for all parsers; change the config to keep default limits for :urlencoded and :json and only increase the multipart limit by replacing the top-level length with a per-parser tuple for multipart (e.g., use {:multipart, length: 12_000_000} or 20_000_000) in the Plug.Parsers plug configuration in the Endpoint module so only the multipart parser gets the larger upload cap.docs/adr/0020-pdf-invoice-upload-flow.md (1)
45-47: 120-second synchronous extraction blocks request handler threads under concurrent load.A 120-second HTTP call held on the web server's request-handler thread means N concurrent PDF uploads can occupy N threads for up to 2 minutes each, potentially starving all other requests. Consider one of:
- Lower the timeout — empirically profile the 95th-percentile extraction latency and set a tighter ceiling (e.g., 30–45 s).
- Dedicated upload worker pool — accept the upload, enqueue extraction in an Oban job, and return a
202 Acceptedwith the new invoice ID so the client pollsextraction_status. ADR 0021 may be a good place to revisit this.- Concurrency cap — rate-limit the
:uploadroute so at most k extractions run simultaneously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0020-pdf-invoice-upload-flow.md` around lines 45 - 47, Update the "Synchronous extraction" section that states "Extraction is synchronous (up to 120s timeout)" to remove or revise the 120s blocking claim and add mitigation options: (1) recommend lowering the timeout based on measured 95th-percentile latency (e.g., 30–45s), (2) describe accepting uploads and enqueuing extraction in a background worker (e.g., Oban job) returning 202 Accepted and polling extraction_status, and (3) suggest an upload-route concurrency cap/rate-limit to bound simultaneous extractions; reference the existing "Synchronous extraction" heading and the extraction_status behavior so readers can find and update that paragraph and tie to ADR 0021 for further design changes.priv/repo/migrations/20260224000021_add_pdf_upload_support.exs (1)
4-9: Consider indexingextraction_statusif it will be queried.If background jobs or API filters query by status, an index will avoid full scans.
♻️ Suggested migration tweak
def change do alter table(:invoices) do add :pdf_content, :binary add :extraction_status, :string add :original_filename, :string end + + create index(:invoices, [:extraction_status]) endBased on learnings: Use database indexes on frequently queried columns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs` around lines 4 - 9, The migration adds extraction_status to table(:invoices) but doesn’t create an index; if extraction_status will be used in queries or background jobs, modify the change/alter block to add an index on :extraction_status (e.g., via create index(:invoices, [:extraction_status])) so lookups avoid full-table scans; update the migration function (change) that alters table(:invoices) to include the index creation and ensure the index name or options match your conventions.test/ksef_hub/invoices_test.exs (1)
686-722:recalculate_extraction_statustests look correct but consider an edge case.The two tests cover "all fields present → complete" and "critical field nil → partial". Consider adding a test where a critical field is set to an empty string
""— the implementation checksvalue != nil && value != "", so this edge case should be verified.🤖 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 686 - 722, Add a third test in the recalculate_extraction_status/2 describe block that mirrors the "critical field is missing" case but sets a critical field (e.g., seller_nip) to an empty string "" instead of nil, call Invoices.recalculate_extraction_status(invoice, attrs) and assert the returned extraction_status is "partial"; this verifies the implementation check value != nil && value != "" correctly treats empty string as missing.test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
1072-1084: Temp files are never cleaned up after tests.
create_temp_pdf/0andcreate_temp_non_pdf/0write files toSystem.tmp_dir!()but never remove them. While the OS cleans/tmpperiodically, explicit cleanup avoids accumulation during CI runs.Proposed approach: return path and register cleanup
You could register cleanup in each test's setup or use
on_exit:defp create_temp_pdf do path = Path.join(System.tmp_dir!(), "test_invoice_#{System.unique_integer([:positive])}.pdf") File.write!(path, "%PDF-1.4 fake test content") on_exit(fn -> File.rm(path) end) path endNote:
on_exit/1must be called from the test process (insidetestorsetupblocks), so this approach would require restructuring. An alternative is to useExUnit.Callbacks.on_exit/1in asetupblock that creates all needed files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 1072 - 1084, create_temp_pdf/0 and create_temp_non_pdf/0 create temp files in System.tmp_dir!() but never remove them; modify tests to register cleanup with ExUnit.on_exit or move file creation into a setup block so the test process can call on_exit(fn -> File.rm(path) end) for each generated path; either change the helper functions to return the path (do not call on_exit inside them) and have the setup/test call on_exit with File.rm(path), or refactor helpers to accept a callback that registers the cleanup in the test process—update usages of create_temp_pdf and create_temp_non_pdf accordingly so all temp files are removed after each test.docs/adr/0022-tech-debt-invoice-creation-and-client-tests.md (1)
9-192: ADR structure deviates from the required format.Per coding guidelines, ADR files must follow the format: Status, Context, Decision, Consequences. This file uses
Context → Issue 1 → Issue 2 → Recommendationinstead. Consider restructuring to use aDecisionsection (the proposed refactors) and aConsequencessection (benefits, trade-offs, scope) for consistency. As per coding guidelines, "ADR files must follow the format: Status, Context, Decision, Consequences with filename patternNNNN-short-title.md."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0022-tech-debt-invoice-creation-and-client-tests.md` around lines 9 - 192, The ADR file's structure doesn't follow required "Status, Context, Decision, Consequences" ordering and filename pattern; update the document to include a top "Status" line (e.g., Proposed/Accepted), keep the existing Context section but move the "Proposed refactoring" text into a new "Decision" section (clearly naming the extracted function create_invoice_with_duplicate_retry/3 and the affected functions create_manual_invoice/2, do_create_pdf_upload/5, retry_as_duplicate/2), and add a "Consequences" section listing benefits, trade-offs, scope, and tests impacted (test/ksef_hub/unstructured/client_test.exs and test/ksef_hub/predictions/prediction_service_test.exs and the invoices refactor scope); also rename the file to match the NNNN-short-title.md pattern per guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0022-ecto-enum-for-string-enums.md`:
- Around line 1-102: This ADR incorrectly reuses ADR number "0022" in the header
"# 0022. Replace String Enums with Ecto.Enum"; rename the ADR to the next free
number (e.g., "0023") by updating the top header and the filename, and update
any internal references/links and the Date if needed so the repository's ADR
index and TOC remain consistent; ensure the table of fields and examples remain
unchanged aside from the numeric ID.
In `@docs/adr/0022-tech-debt-invoice-creation-and-client-tests.md`:
- Around line 1-8: This ADR file duplicates the ADR number 0022; rename the ADR
identifier and header occurrences from "0022" to "0023" (update the filename
prefix and the H1/title line "0022. Tech Debt: Invoice Creation..." to "0023.
...") and update any internal references or index entries that mention
"0022-tech-debt-..." so the numbering is unique and does not conflict with the
existing "0022-ecto-enum-..." ADR; ensure the new filename and the H1 remain
consistent.
In `@lib/ksef_hub_web/schemas/invoice.ex`:
- Around line 42-45: Update the schema entries for net_amount, vat_amount,
gross_amount, and currency in the Invoice schema so they allow nulls: add
nullable: true to each %Schema{...} for the fields net_amount, vat_amount,
gross_amount and currency (the definitions you can find as %Schema{type:
:string, description: ...} in invoice.ex) so the OpenAPI schema matches possible
nil values for partial/failed extraction_status responses.
In `@lib/ksef_hub/invoices/invoice.ex`:
- Around line 87-89: The changeset/2 currently includes :pdf_content in the cast
list which allows caller-supplied binary data; remove :pdf_content from the cast
in the Invoice schema changeset and instead set it programmatically where the
PDF is read (e.g., in do_create_pdf_upload/5 or the context function that builds
the invoice) by calling Ecto.Changeset.put_change(:pdf_content, pdf_binary) on
the changeset before Repo.insert/update. Ensure all code paths that previously
relied on attrs[:pdf_content] are updated to supply the binary via put_change
rather than through params.
In `@lib/ksef_hub/unstructured/client.ex`:
- Around line 64-67: The current clause matching {:ok, %{status: status, body:
body}} logs the full response body which may contain sensitive invoice data;
change the Logger.error call in that match to omit the body and log only the
status and a minimal/sanitized message (e.g., "Unstructured service returned
<status> for /extract") or a redacted token instead of body; update the match in
the same function/clauses that handle the extract response so no response body
is included in logs.
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 590-612: Rename the misleading test title to match its assertions:
change the test name "returns 502 when extraction service fails" to something
like "creates invoice with failed extraction status when extraction service
fails" in the test block that calls create_owner_with_token(),
Mox.expect(KsefHub.Unstructured.Mock, :extract, ...), builds the Plug.Upload and
posts to "/api/invoices/upload"; keep the assertions that expect status 201 and
data["extraction_status"] == "failed" unchanged so the name reflects the actual
behavior.
---
Outside diff comments:
In `@lib/ksef_hub_web/schemas/invoice.ex`:
- Around line 148-182: Update the schema example in Invoice (module/schema
defined in invoice.ex) to include a pdf_upload variant showing the new
PDF-specific fields: add "source": "pdf_upload" and include "extraction_status"
(e.g., "pending"|"completed"|"failed") and "original_filename" (string) in the
example map; alternatively add a second example entry demonstrating both the
existing ksef example and a pdf_upload example so API consumers can see the new
fields alongside the existing fields like ksef_number and ksef_acquisition_date.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 13-14: The docker-compose service "unstructured" is using the
floating image tag "ghcr.io/appunite/au-ksef-unstructured:latest", which makes
builds non-reproducible; update the image reference in the "unstructured"
service to a fixed semantic version tag or an immutable digest (e.g., replace
:latest with a specific version like :v1.2.3 or `@sha256`:<digest>) so pulls are
deterministic and reproducible.
In `@docs/adr/0020-pdf-invoice-upload-flow.md`:
- Around line 45-47: Update the "Synchronous extraction" section that states
"Extraction is synchronous (up to 120s timeout)" to remove or revise the 120s
blocking claim and add mitigation options: (1) recommend lowering the timeout
based on measured 95th-percentile latency (e.g., 30–45s), (2) describe accepting
uploads and enqueuing extraction in a background worker (e.g., Oban job)
returning 202 Accepted and polling extraction_status, and (3) suggest an
upload-route concurrency cap/rate-limit to bound simultaneous extractions;
reference the existing "Synchronous extraction" heading and the
extraction_status behavior so readers can find and update that paragraph and tie
to ADR 0021 for further design changes.
In `@docs/adr/0022-tech-debt-invoice-creation-and-client-tests.md`:
- Around line 9-192: The ADR file's structure doesn't follow required "Status,
Context, Decision, Consequences" ordering and filename pattern; update the
document to include a top "Status" line (e.g., Proposed/Accepted), keep the
existing Context section but move the "Proposed refactoring" text into a new
"Decision" section (clearly naming the extracted function
create_invoice_with_duplicate_retry/3 and the affected functions
create_manual_invoice/2, do_create_pdf_upload/5, retry_as_duplicate/2), and add
a "Consequences" section listing benefits, trade-offs, scope, and tests impacted
(test/ksef_hub/unstructured/client_test.exs and
test/ksef_hub/predictions/prediction_service_test.exs and the invoices refactor
scope); also rename the file to match the NNNN-short-title.md pattern per
guidelines.
In `@lib/ksef_hub_web/endpoint.ex`:
- Around line 45-49: The global length: 12_000_000 on the Plug.Parsers plug is
widening the body size for all parsers; change the config to keep default limits
for :urlencoded and :json and only increase the multipart limit by replacing the
top-level length with a per-parser tuple for multipart (e.g., use {:multipart,
length: 12_000_000} or 20_000_000) in the Plug.Parsers plug configuration in the
Endpoint module so only the multipart parser gets the larger upload cap.
In `@lib/ksef_hub/unstructured/behaviour.ex`:
- Around line 1-13: Add descriptive `@doc` annotations to the
KsefHub.Unstructured.Behaviour module for each callback: place a short doc above
`@callback` extract/2 describing the expected pdf_binary input, opts, the success
return map shape and possible error forms, and another `@doc` above `@callback`
health/0 describing the health map shape and error semantics; keep descriptions
minimal and focused to improve generated docs without changing behaviour.
In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs`:
- Around line 4-9: The migration adds extraction_status to table(:invoices) but
doesn’t create an index; if extraction_status will be used in queries or
background jobs, modify the change/alter block to add an index on
:extraction_status (e.g., via create index(:invoices, [:extraction_status])) so
lookups avoid full-table scans; update the migration function (change) that
alters table(:invoices) to include the index creation and ensure the index name
or options match your conventions.
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 1072-1084: create_temp_pdf/0 and create_temp_non_pdf/0 create temp
files in System.tmp_dir!() but never remove them; modify tests to register
cleanup with ExUnit.on_exit or move file creation into a setup block so the test
process can call on_exit(fn -> File.rm(path) end) for each generated path;
either change the helper functions to return the path (do not call on_exit
inside them) and have the setup/test call on_exit with File.rm(path), or
refactor helpers to accept a callback that registers the cleanup in the test
process—update usages of create_temp_pdf and create_temp_non_pdf accordingly so
all temp files are removed after each test.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 686-722: Add a third test in the recalculate_extraction_status/2
describe block that mirrors the "critical field is missing" case but sets a
critical field (e.g., seller_nip) to an empty string "" instead of nil, call
Invoices.recalculate_extraction_status(invoice, attrs) and assert the returned
extraction_status is "partial"; this verifies the implementation check value !=
nil && value != "" correctly treats empty string as missing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
config/config.exsconfig/runtime.exsconfig/test.exsdocker-compose.ymldocs/adr/0020-pdf-invoice-upload-flow.mddocs/adr/0021-future-invoice-documents-table.mddocs/adr/0022-ecto-enum-for-string-enums.mddocs/adr/0022-tech-debt-invoice-creation-and-client-tests.mdlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/unstructured/behaviour.exlib/ksef_hub/unstructured/client.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/endpoint.exlib/ksef_hub_web/router.exlib/ksef_hub_web/schemas/invoice.exlib/ksef_hub_web/schemas/update_invoice_request.exlib/ksef_hub_web/schemas/upload_invoice_request.expriv/repo/migrations/20260224000021_add_pdf_upload_support.exstest/ksef_hub/invoices_test.exstest/ksef_hub/unstructured/client_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/support/conn_case.extest/support/data_case.extest/support/factory.extest/support/unstructured_stub.extest/test_helper.exs
Comment on lines
+1
to
+102
| # 0022. Replace String Enums with Ecto.Enum | ||
|
|
||
| Date: 2026-02-24 | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed | ||
|
|
||
| ## Context | ||
|
|
||
| All enum-like fields across the codebase are stored as `:string` in Ecto schemas and validated with `validate_inclusion/3` against word lists (`@valid_types ~w(income expense)`, etc.). Business logic then compares these values with string equality checks: | ||
|
|
||
| ```elixir | ||
| # Current: string comparisons everywhere | ||
| def approve_invoice(%Invoice{type: "expense", status: "pending"}) do | ||
| if socket.assigns[:current_role] == "owner" do | ||
| where(query, [i], i.type == "expense") | ||
| ``` | ||
|
|
||
| This has several problems: | ||
|
|
||
| - **No compile-time safety** — a typo like `"expens"` compiles fine and fails silently at runtime. | ||
| - **Not idiomatic Elixir** — atoms are the language's tool for named constants; strings are for user-facing text. | ||
| - **Verbose pattern matching** — `%Invoice{status: "pending"}` instead of `%Invoice{status: :pending}`. | ||
| - **No exhaustiveness hints** — Dialyzer cannot warn about unhandled enum values in string-based case/cond. | ||
|
|
||
| ## Decision | ||
|
|
||
| Convert all string-based enum fields to `Ecto.Enum`. This stores strings in PostgreSQL (no migration needed) but presents atoms in Elixir code. | ||
|
|
||
| ### Fields to convert | ||
|
|
||
| | Module | Field | Values | | ||
| |--------|-------|--------| | ||
| | `Invoice` | `type` | `:income`, `:expense` | | ||
| | `Invoice` | `status` | `:pending`, `:approved`, `:rejected` | | ||
| | `Invoice` | `source` | `:ksef`, `:manual`, `:pdf_upload` | | ||
| | `Invoice` | `extraction_status` | `:complete`, `:partial`, `:failed` | | ||
| | `Invoice` | `duplicate_status` | `:suspected`, `:confirmed`, `:dismissed` | | ||
| | `Invoice` | `prediction_status` | `:pending`, `:predicted`, `:needs_review`, `:manual` | | ||
| | `Checkpoint` | `checkpoint_type` | `:income`, `:expense` | | ||
| | `Membership` | `role` | `:owner`, `:accountant`, `:reviewer` | | ||
| | `Invitation` | `role` | `:accountant`, `:reviewer` | | ||
| | `Invitation` | `status` | `:pending`, `:accepted`, `:cancelled` | | ||
|
|
||
| ### Schema change pattern | ||
|
|
||
| ```elixir | ||
| # Before | ||
| field :type, :string | ||
| @valid_types ~w(income expense) | ||
| validate_inclusion(:type, @valid_types) | ||
|
|
||
| # After | ||
| field :type, Ecto.Enum, values: [:income, :expense] | ||
| # validate_inclusion is no longer needed — Ecto.Enum rejects invalid values at cast time | ||
| ``` | ||
|
|
||
| ### Code change pattern | ||
|
|
||
| ```elixir | ||
| # Before | ||
| def approve_invoice(%Invoice{type: "expense"} = invoice) do | ||
| where(query, [i], i.status == "pending") | ||
|
|
||
| # After | ||
| def approve_invoice(%Invoice{type: :expense} = invoice) do | ||
| where(query, [i], i.status == :pending) | ||
| ``` | ||
|
|
||
| ### API layer | ||
|
|
||
| JSON serialization is unaffected. `Jason.encode/1` converts atoms to strings, so API responses remain `"status": "pending"`. Incoming JSON strings are cast to atoms by `Ecto.Enum` during changeset processing. OpenAPI schemas stay as `type: :string, enum: [...]`. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Benefits | ||
|
|
||
| - Compile-time atom existence checks (misspelled atoms warn in some editors) | ||
| - Idiomatic Elixir — atoms for internal constants, strings for external boundaries | ||
| - Cleaner pattern matching: `%Invoice{status: :pending}` reads naturally | ||
| - `validate_inclusion` calls can be removed — `Ecto.Enum` enforces valid values at cast time | ||
| - Dialyzer can reason about atom values better than arbitrary strings | ||
|
|
||
| ### Trade-offs | ||
|
|
||
| - Large cross-cutting diff (~10 fields, ~150+ string comparison sites across lib/, test/, templates) | ||
| - Every test that inserts or asserts enum values must change from `"pending"` to `:pending` | ||
| - Factory defaults change from `status: "pending"` to `status: :pending` | ||
| - LiveView templates change from `@type == "income"` to `@type == :income` | ||
| - Ecto query filters from API params need explicit cast (string params to atoms) — but `Ecto.Enum` handles this via changesets | ||
|
|
||
| ### Recommendation | ||
|
|
||
| Do this as a standalone refactor after the pdf_upload feature is merged. Tackle in priority order: | ||
|
|
||
| 1. `Invoice.type` and `Invoice.status` (highest usage, ~53 sites) | ||
| 2. `Membership.role` (~22 sites, affects auth layer) | ||
| 3. `Invoice.source` (~12 sites) | ||
| 4. Remaining fields (lower usage, straightforward) | ||
|
|
||
| Each field can be a separate commit — no database migration required since PostgreSQL column type stays `text`. |
There was a problem hiding this comment.
Well-written ADR with proper format, but duplicate ADR number.
This file shares the 0022 number with 0022-tech-debt-invoice-creation-and-client-tests.md. One of them needs to be renumbered (e.g., this one to 0023). The content and structure of this ADR are solid — the phased migration plan per field is pragmatic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0022-ecto-enum-for-string-enums.md` around lines 1 - 102, This ADR
incorrectly reuses ADR number "0022" in the header "# 0022. Replace String Enums
with Ecto.Enum"; rename the ADR to the next free number (e.g., "0023") by
updating the top header and the filename, and update any internal
references/links and the Date if needed so the repository's ADR index and TOC
remain consistent; ensure the table of fields and examples remain unchanged
aside from the numeric ID.
Comment on lines
+1
to
+8
| # 0022. Tech Debt: Invoice Creation Pattern and HTTP Client Tests | ||
|
|
||
| Date: 2026-02-23 | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed | ||
|
|
There was a problem hiding this comment.
Duplicate ADR number: both 0022-tech-debt-... and 0022-ecto-enum-... use the same 0022 prefix.
ADR numbers must be unique. Renumber one of these files (e.g., this one to 0023).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0022-tech-debt-invoice-creation-and-client-tests.md` around lines 1
- 8, This ADR file duplicates the ADR number 0022; rename the ADR identifier and
header occurrences from "0022" to "0023" (update the filename prefix and the
H1/title line "0022. Tech Debt: Invoice Creation..." to "0023. ...") and update
any internal references or index entries that mention "0022-tech-debt-..." so
the numbering is unique and does not conflict with the existing
"0022-ecto-enum-..." ADR; ensure the new filename and the H1 remain consistent.
Inline comments: - Renumber ADR 0022 conflicts: ecto-enum → 0023, tech-debt → 0024 - Add nullable: true to net_amount, vat_amount, gross_amount, currency in OpenAPI Invoice schema (partial extractions can have nil values) - Remove :pdf_content from changeset cast; set via put_change in create_invoice/1 to prevent caller-supplied binary injection - Stop logging response body in Unstructured.Client error handler (may contain sensitive invoice data) - Rename misleading test "returns 502" → "creates invoice with failed extraction status" to match its 201 assertion Outside diff: - Add pdf_upload example to Invoice OpenAPI schema showing extraction_status and original_filename fields Nitpicks: - Pin unstructured docker image to v0.1.0 instead of :latest - Add synchronous extraction mitigations to ADR 0020 (timeout tuning, async path via Oban, concurrency cap) - Restructure ADR 0024 to follow Status/Context/Decision/Consequences - Scope Plug.Parsers 12MB length to multipart parser only - Add @doc annotations to Unstructured.Behaviour callbacks - Add index on extraction_status in migration - Clean up temp files in tests via on_exit - Add empty-string test for recalculate_extraction_status Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
priv/repo/migrations/20260224000021_add_pdf_upload_support.exs (1)
4-12: Migration is clean and reversible.The index on
extraction_statusis a good addition for filtering invoices needing review.Two optional hardening suggestions:
- Consider using
:string, size: 255fororiginal_filenameto match the schema-levelvalidate_length(max: 255)at the DB level.- A partial index (
where: "extraction_status IS NOT NULL") would be more space-efficient since most invoices (ksef/manual) won't have anextraction_status.Optional: partial index and sized column
alter table(:invoices) do add :pdf_content, :binary add :extraction_status, :string - add :original_filename, :string + add :original_filename, :string, size: 255 end -create index(:invoices, [:extraction_status]) +create index(:invoices, [:extraction_status], where: "extraction_status IS NOT NULL")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs` around lines 4 - 12, Update the migration's change/alter block to make original_filename a sized string and make the index partial: change add :original_filename, :string to add :original_filename, :string, size: 255 to enforce the 255-char limit at DB level, and change the index creation create index(:invoices, [:extraction_status]) to a partial index create index(:invoices, [:extraction_status], where: "extraction_status IS NOT NULL") so the index only stores non-null extraction_status values and saves space.lib/ksef_hub/invoices/invoice.ex (1)
175-194: Source-specific validation forpdf_uploadis clean and consistent.The
validate_extraction_status/1helper mirrors thevalidate_duplicate_status/1pattern — validate inclusion only when the field is present. Sinceextraction_statusis always determined by the context layer (not user input), this is appropriate.One optional tightening: for the
"pdf_upload"source specifically, you couldvalidate_required([:extraction_status])to guarantee it's always set on PDF upload invoices, catching any context-layer bug that forgets to set it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/invoice.ex` around lines 175 - 194, Add a required-field check for extraction_status when source is "pdf_upload": in the case branch handling "pdf_upload" (the function that builds the changeset), after validate_required([:pdf_content]) also call validate_required([:extraction_status]) so extraction_status is enforced for PDF uploads; keep the existing validate_extraction_status/1 helper as-is (it can still validate inclusion when present) and reference the "pdf_upload" branch and validate_extraction_status/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.
Inline comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 892-893: Update the list_invoices documentation to include
"pdf_upload" as an accepted value for the :source filter (in addition to "ksef"
and "manual"); locate the `@doc` or module docs around the list_invoices function
in lib/ksef_hub/invoices.ex (the clause matching {:source, source}, q when
source in ~w(ksef manual pdf_upload) is the code change) and add "pdf_upload" to
the documented allowed values and any examples or typespecs that enumerate
source values.
- Around line 343-349: The Logger.warning call inside the
unstructured_client().extract error branch currently logs the raw reason via
inspect(reason), which may leak tokens; change the logging to a sanitized
message without including the raw error or its fields (e.g., "PDF extraction
failed for filename: #{filename || "invoice.pdf"}" or include only a safe error
code), and keep the flow that calls do_create_pdf_upload_failed(company_id,
pdf_binary, type, filename); update the Logger.warning usage in the {:error,
reason} clause of extract/2 (the case handling in invoices.ex) to remove
inspect(reason) and emit only non-sensitive context.
In `@lib/ksef_hub/unstructured/client.ex`:
- Around line 64-71: The code currently logs the full Req error via
Logger.error("... #{inspect(reason)}") in the {:error, reason} match (the
{:error, reason} clause and its Logger.error call), which can leak tokens;
change it to log a sanitized message only — e.g., remove inspect(reason) and
instead log the error type or a safe message such as Logger.error("Unstructured
service request failed for /extract: #{inspect(reason.__struct__)}") or extract
only a non-sensitive :message field if present (Map.get/Pattern match) so no
headers, auth tokens or certificates are ever logged.
---
Nitpick comments:
In `@lib/ksef_hub/invoices/invoice.ex`:
- Around line 175-194: Add a required-field check for extraction_status when
source is "pdf_upload": in the case branch handling "pdf_upload" (the function
that builds the changeset), after validate_required([:pdf_content]) also call
validate_required([:extraction_status]) so extraction_status is enforced for PDF
uploads; keep the existing validate_extraction_status/1 helper as-is (it can
still validate inclusion when present) and reference the "pdf_upload" branch and
validate_extraction_status/1 when making the change.
In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs`:
- Around line 4-12: Update the migration's change/alter block to make
original_filename a sized string and make the index partial: change add
:original_filename, :string to add :original_filename, :string, size: 255 to
enforce the 255-char limit at DB level, and change the index creation create
index(:invoices, [:extraction_status]) to a partial index create
index(:invoices, [:extraction_status], where: "extraction_status IS NOT NULL")
so the index only stores non-null extraction_status values and saves space.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docker-compose.ymldocs/adr/0020-pdf-invoice-upload-flow.mddocs/adr/0023-ecto-enum-for-string-enums.mddocs/adr/0024-tech-debt-invoice-creation-and-client-tests.mdlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/unstructured/behaviour.exlib/ksef_hub/unstructured/client.exlib/ksef_hub_web/endpoint.exlib/ksef_hub_web/schemas/invoice.expriv/repo/migrations/20260224000021_add_pdf_upload_support.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exs
- Update list_invoices docs to include "pdf_upload" as accepted source filter - Sanitize Logger.warning in create_pdf_upload_invoice: remove inspect(reason) which could leak auth tokens from Req error structs - Sanitize Logger.error in Unstructured.Client: log only exception struct name instead of full error (which may contain request headers with bearer token) - Require extraction_status in pdf_upload changeset validation (defensive) - Add size: 255 to original_filename column in migration - Use partial index on extraction_status (WHERE NOT NULL) to save space Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
lib/ksef_hub/invoices.ex (1)
395-404:determine_extraction_status/1andrecalculate_extraction_status/2duplicate the critical-field presence check.Both functions implement the same
Enum.all?(@critical_extraction_fields, fn field -> ...)logic but differ only in how they access field values (string-keyed raw extraction map vs atom-keyed merged struct map). Extract a shared predicate:♻️ Suggested extraction
`@spec` all_critical_fields_present?(map()) :: boolean() defp all_critical_fields_present?(map) do Enum.all?(`@critical_extraction_fields`, fn field -> value = Map.get(map, field) value != nil && value != "" end) endThen
determine_extraction_status/1would normalize to atom keys before calling this helper, andrecalculate_extraction_status/2would call it on the already-merged atom-keyed map.🤖 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 395 - 404, Extract the duplicated presence-check into a private helper all_critical_fields_present?(map) that iterates `@critical_extraction_fields` and uses Map.get(map, field) to verify non-nil/non-empty values; update determine_extraction_status/1 to first normalize the incoming string-keyed extraction to atom keys (so Map.get can be used) and call all_critical_fields_present?/1, and change recalculate_extraction_status/2 to call all_critical_fields_present?/1 on the merged atom-keyed map instead of repeating the Enum.all? logic.priv/repo/migrations/20260224000021_add_pdf_upload_support.exs (1)
6-6: Operational note: PDF binaries stored in the primaryinvoicestable.
pdf_content :binarymaps tobyteain PostgreSQL. Postgres TOAST will handle the storage transparently for large values, but keeping large blobs in the primary table still increases heap fragmentation, VACUUM pressure, and backup sizes over time. The referenced ADR 0021 already acknowledges a futureinvoice_documentstable; consider tracking this technical debt explicitly and setting abyteacolumn-level size budget in application validation (enforced in the changesetvalidate_length) to prevent unbounded uploads until the migration happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs` at line 6, The migration adds pdf_content :binary to the invoices table which will store potentially large blobs; to limit technical debt, add an application-level max size check on the Invoice changeset (e.g., in the changeset function for the Invoice schema) using validate_length/2 or a custom validator to enforce a byte-size budget for pdf_content, and add a TODO comment referencing the future invoice_documents table (ADR 0021) so the schema change and the validation are tracked until the separate invoice_documents migration is implemented.
🤖 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 228-239: The function recalculate_extraction_status/2 currently
merges an atom-keyed map from Map.from_struct(invoice) with attrs that may have
string keys, so string-keyed updates don't override the struct values; convert
attrs to an atom-keyed map before merging (e.g. transform each {k,v} where k is
binary into {String.to_existing_atom(k), v}, leaving atom keys unchanged) using
String.to_existing_atom for safety, then merge into Map.from_struct(invoice) and
compute all_present? against the merged map and `@critical_extraction_fields` as
before.
- Around line 474-481: The parse_decimal/1 function currently accepts partial
parses from Decimal.parse/1 which can silently accept values like "1000.00 EUR";
change it to only return a Decimal when Decimal.parse(value_trimmed) returns
{decimal, ""} (i.e., exact full-string parse) and return nil for any other
result; to be safe call String.trim(value) before parsing to allow surrounding
whitespace, and update the `@spec/`@doc to state that only exact numeric strings
are accepted (or explicitly document/implement the lenient behavior if that is
intended).
- Around line 139-150: The Map.pop call only handles atom keys and will miss
string-keyed input; update the create_invoice/1 logic to extract pdf_content
from either atom or string keys (e.g., check Map.has_key?(attrs, :pdf_content)
first and Map.has_key?(attrs, "pdf_content") second) and remove that key from
attrs before building the base changeset so Invoice.changeset/1 and the
Ecto.Changeset.put_change call receive the correct pdf_content; keep the
existing behavior of only calling Ecto.Changeset.put_change(base, :pdf_content,
pdf_content) when pdf_content is present.
- Around line 363-374: The retry path currently bypasses the extraction_status
guard by calling enqueue_prediction unconditionally in retry_as_duplicate/2;
update retry_as_duplicate/2 (or its callers) so it does not enqueue predictions
blindly: either remove the enqueue_prediction call from retry_as_duplicate/2 and
let the caller (the create_invoice case) invoke
maybe_enqueue_prediction(extraction_status, invoice) after a successful retry,
or refactor retry_as_duplicate/2 to accept an optional callback or flag (e.g.,
post_insert_callback or enqueue_on_complete boolean) and only call
enqueue_prediction when the extraction_status == "complete"; reference functions
to change: create_invoice handling code in the case block,
maybe_enqueue_prediction/2, retry_as_duplicate/2, and enqueue_prediction/1.
In `@lib/ksef_hub/unstructured/client.ex`:
- Around line 18-24: The current extract/2 clause only matches when pdf_binary
is a binary and will raise FunctionClauseError for other input types; add a
second clause def extract(pdf, _opts) when not is_binary(pdf) do {:error,
:invalid_pdf} end (or similar {:error, reason}) to return a typed error instead
of crashing, keeping the original def extract(pdf_binary, opts) when
is_binary(pdf_binary) as-is so callers receive {:ok, ...} | {:error, term()} as
the spec promises.
- Around line 54-60: Sanitize the user-supplied filename before embedding it in
the multipart Content-Disposition: replace/strip CR and LF (and other control)
characters from filename (e.g., produce sanitized_filename =
Regex.replace(~r/[\r\n]/, filename, "_") or an equivalent) and use
sanitized_filename in the multipart tuple ({pdf_binary, filename:
sanitized_filename, content_type: "application/pdf"}) passed to Req.post (the
call that uses pdf_binary, filename, token and `@receive_timeout` in the client
module). Ensure the sanitized variable replaces all uses of the original
filename in that multipart payload to prevent header injection.
---
Nitpick comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 395-404: Extract the duplicated presence-check into a private
helper all_critical_fields_present?(map) that iterates
`@critical_extraction_fields` and uses Map.get(map, field) to verify
non-nil/non-empty values; update determine_extraction_status/1 to first
normalize the incoming string-keyed extraction to atom keys (so Map.get can be
used) and call all_critical_fields_present?/1, and change
recalculate_extraction_status/2 to call all_critical_fields_present?/1 on the
merged atom-keyed map instead of repeating the Enum.all? logic.
In `@priv/repo/migrations/20260224000021_add_pdf_upload_support.exs`:
- Line 6: The migration adds pdf_content :binary to the invoices table which
will store potentially large blobs; to limit technical debt, add an
application-level max size check on the Invoice changeset (e.g., in the
changeset function for the Invoice schema) using validate_length/2 or a custom
validator to enforce a byte-size budget for pdf_content, and add a TODO comment
referencing the future invoice_documents table (ADR 0021) so the schema change
and the validation are tracked until the separate invoice_documents migration is
implemented.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/unstructured/client.expriv/repo/migrations/20260224000021_add_pdf_upload_support.exs
Comment on lines
+139
to
150
| {pdf_content, attrs} = Map.pop(attrs, :pdf_content) | ||
|
|
||
| %Invoice{} | ||
| |> Ecto.Changeset.change(%{company_id: company_id}) | ||
| base = | ||
| %Invoice{} | ||
| |> Ecto.Changeset.change(%{company_id: company_id}) | ||
|
|
||
| base = | ||
| if pdf_content, do: Ecto.Changeset.put_change(base, :pdf_content, pdf_content), else: base | ||
|
|
||
| base | ||
| |> Invoice.changeset(attrs) | ||
| |> Repo.insert() |
There was a problem hiding this comment.
Map.pop(attrs, :pdf_content) only handles atom-keyed maps.
If create_invoice/1 is ever called with string-keyed attrs (e.g., %{"pdf_content" => ...}), the pop will miss the key, pdf_content will stay in attrs, be silently ignored by cast/3, and never reach the changeset via put_change. All current internal call sites use atom-keyed maps, but consider guarding against future regressions:
💡 Defensive fix
- {pdf_content, attrs} = Map.pop(attrs, :pdf_content)
+ {pdf_content, attrs} =
+ case Map.pop(attrs, :pdf_content) do
+ {nil, attrs} -> Map.pop(attrs, "pdf_content")
+ result -> result
+ end📝 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
| {pdf_content, attrs} = Map.pop(attrs, :pdf_content) | |
| %Invoice{} | |
| |> Ecto.Changeset.change(%{company_id: company_id}) | |
| base = | |
| %Invoice{} | |
| |> Ecto.Changeset.change(%{company_id: company_id}) | |
| base = | |
| if pdf_content, do: Ecto.Changeset.put_change(base, :pdf_content, pdf_content), else: base | |
| base | |
| |> Invoice.changeset(attrs) | |
| |> Repo.insert() | |
| {pdf_content, attrs} = | |
| case Map.pop(attrs, :pdf_content) do | |
| {nil, attrs} -> Map.pop(attrs, "pdf_content") | |
| result -> result | |
| end | |
| base = | |
| %Invoice{} | |
| |> Ecto.Changeset.change(%{company_id: company_id}) | |
| base = | |
| if pdf_content, do: Ecto.Changeset.put_change(base, :pdf_content, pdf_content), else: base | |
| base | |
| |> Invoice.changeset(attrs) | |
| |> Repo.insert() |
🤖 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 139 - 150, The Map.pop call only
handles atom keys and will miss string-keyed input; update the create_invoice/1
logic to extract pdf_content from either atom or string keys (e.g., check
Map.has_key?(attrs, :pdf_content) first and Map.has_key?(attrs, "pdf_content")
second) and remove that key from attrs before building the base changeset so
Invoice.changeset/1 and the Ecto.Changeset.put_change call receive the correct
pdf_content; keep the existing behavior of only calling
Ecto.Changeset.put_change(base, :pdf_content, pdf_content) when pdf_content is
present.
- Fix recalculate_extraction_status to handle string-keyed attrs by
converting to atom keys before merging with struct
- Extract shared all_critical_fields_present?/1 helper used by both
determine_extraction_status and recalculate_extraction_status
- Reject partial decimal parses (e.g. "1000.00 EUR") — only accept
exact numeric strings with optional whitespace trim
- Fix retry_as_duplicate blindly enqueuing predictions: extract
create_or_retry_duplicate/2 so callers control post-insert logic
(manual invoices always enqueue, pdf_upload respects extraction_status)
- Add fallback extract/2 clause for non-binary input (returns
{:error, :invalid_pdf} instead of FunctionClauseError)
- Sanitize filename in multipart Content-Disposition: strip CR/LF and
control characters to prevent header injection to sidecar
- Add pdf_content byte-size validation (max 10MB) in invoice changeset
with TODO referencing ADR 0021 for future extraction
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ADR 0021 already tracks the invoice_documents refactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/ksef_hub/invoices.ex (1)
139-147:Map.pop(attrs, :pdf_content)misses string-keyed"pdf_content"— defensive fix still unaddressed.All current internal callers use atom-keyed maps, so this is safe today, but any future caller using string-keyed attrs will silently skip the pop, leave
pdf_contentin attrs (ignored bycast), and never reachput_change.🛡️ Defensive fix
- {pdf_content, attrs} = Map.pop(attrs, :pdf_content) + {pdf_content, attrs} = + case Map.pop(attrs, :pdf_content) do + {nil, attrs} -> Map.pop(attrs, "pdf_content") + result -> result + 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 139 - 147, The current Map.pop(attrs, :pdf_content) only removes atom-keyed :pdf_content and will miss string-keyed "pdf_content"; update the extraction to handle both keys so pdf_content is removed regardless of key type and still applied via Ecto.Changeset.put_change. Replace the single Map.pop call with a small branch that first tries Map.pop(attrs, :pdf_content) and, if that returns {nil, _} or unchanged, falls back to Map.pop(attrs, "pdf_content"), then use the resulting pdf_content when building base and calling Ecto.Changeset.put_change(base, :pdf_content, pdf_content) (referencing the variables pdf_content, attrs and the base change/Ecto.Changeset.put_change code paths).
🧹 Nitpick comments (5)
lib/ksef_hub/invoices.ex (2)
462-468:parse_date/1silently returnsnilfor datetime strings that the extraction service may return.
Date.from_iso8601/1accepts only bare dates ("2026-02-20"). If the unstructured sidecar returns an ISO 8601 datetime ("2026-02-20T10:30:00"),parse_datereturnsnil, leavingissue_dateabsent and potentially downgradingextraction_statusto"partial".♻️ Proposed fix — strip time component before parsing
defp parse_date(value) do - case Date.from_iso8601(value) do - {:ok, date} -> date - _ -> nil + # Handle "YYYY-MM-DDThh:mm:ss…" from OCR output by taking only the date part + date_string = value |> String.split("T") |> List.first() + + case Date.from_iso8601(date_string) do + {:ok, date} -> date + _ -> nil end 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 462 - 468, parse_date/1 currently returns nil for ISO8601 datetime strings (e.g. "2026-02-20T10:30:00"); update the function to pre-normalize the input by stripping any time component before calling Date.from_iso8601 — for example, split on "T" (or whitespace) and use the date portion, then call Date.from_iso8601 and return the {:ok, date} result or nil on failure; make the change in the private function parse_date/1 to ensure issue_date is populated when the extraction service returns datetimes.
343-344:@specdeclaresoptsasmap()but the function uses keyword-list access.
opts[:type]andopts[:filename]are idiomatic keyword access; the correct type annotation iskeyword().-@spec create_pdf_upload_invoice(Ecto.UUID.t(), binary(), map()) :: +@spec create_pdf_upload_invoice(Ecto.UUID.t(), binary(), keyword()) :: {:ok, Invoice.t()} | {:error, term()}🤖 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 343 - 344, The `@spec` for create_pdf_upload_invoice currently types the third parameter as map(), but the implementation accesses opts with keyword-list syntax (opts[:type], opts[:filename]); update the `@spec` to use keyword() for the third arg to match usage (i.e., change the third param type from map() to keyword()) and ensure any callers pass a keyword list matching the expected keys (type, filename).lib/ksef_hub/unstructured/client.ex (2)
63-70: A 200 response with a non-map body from/extractfalls into the generic{:ok, %{status: status}}clause, emitting a misleading log and returning{:error, {:unstructured_service_error, 200}}.
health/0handles this explicitly with{:ok, %{status: 200, body: body}} -> {:error, {:invalid_payload, body}}.do_extract/4should match the same pattern for consistency and easier debugging.♻️ Proposed fix
{:ok, %{status: 200, body: body}} when is_map(body) -> {:ok, body} + {:ok, %{status: 200, body: body}} -> + Logger.error("Unstructured service returned unexpected payload type for /extract") + {:error, {:invalid_payload, body}} + {:ok, %{status: status}} -> Logger.error("Unstructured service returned #{status} for /extract") {:error, {:unstructured_service_error, status}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/unstructured/client.ex` around lines 63 - 70, The do_extract/4 success clause currently treats any {:ok, %{status: status}} the same and returns {:error, {:unstructured_service_error, 200}} for a 200 with non-map body; change do_extract/4 to mirror health/0 by adding an explicit match for {:ok, %{status: 200, body: body}} that returns {:error, {:invalid_payload, body}} (and keep the existing {:ok, %{status: 200, body: body}} -> {:ok, body} for map bodies), leaving the generic {:ok, %{status: status}} clause for other statuses and adjust the Logger message only for non-200 statuses so 200 with invalid payload is reported correctly.
31-49:health/0uses the same 2-minute extraction timeout; consider a shorter, dedicated timeout.
@receive_timeout 120_000is appropriate for a multipart PDF upload but is excessive for a health probe — a hung sidecar would block the caller for 2 minutes. Consider a separate module attribute, e.g.@health_timeout 5_000.Additionally, the health endpoint is called without an Authorization header. Please verify that
/healthis intentionally unauthenticated on the sidecar side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/unstructured/client.ex` around lines 31 - 49, health/0 currently reuses `@receive_timeout` (120_000) which is too long for a probe; add a dedicated shorter module attribute (e.g. `@health_timeout` 5_000) and use it in the Req.get call inside health/0 instead of `@receive_timeout`, and verify whether the sidecar /health endpoint requires authentication — if it does, include the appropriate Authorization header in the Req.get request (update health/0 to add headers to the Req.get call); reference functions/values: health/0, `@receive_timeout`, add `@health_timeout`, Req.get, and fetch_url().lib/ksef_hub/invoices/invoice.ex (1)
175-175: Pipeline failure: Credo flags the TODO comment at line 175.The CI pipeline reports:
[warning] 175-175: Credo: Found a TODO tag in a comment: # TODO: move pdf_content to invoice_documents table (ADR 0021).Would you like me to open a tracking issue for this migration (ADR 0021) so the TODO can be removed before merging?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/invoice.ex` at line 175, Create a tracking issue for the ADR 0021 migration and remove the "TODO" tag from the comment in lib/ksef_hub/invoices/invoice.ex (the comment "# TODO: move pdf_content to invoice_documents table (ADR 0021)"). Replace it with a non-TODO note that links to the created issue or references ADR 0021 (e.g., "# ADR 0021: move pdf_content to invoice_documents table — tracked in ISSUE-123") so Credo no longer flags it; update the code comment and rerun CI.
🤖 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 236-243: The current atomize_known_keys/1 rescues ArgumentError
for the whole conversion, causing all keys to remain strings when any single key
is unknown and leading recalculate_extraction_status/2 to read stale atom keys;
change atomize_known_keys/1 to perform a per-key safe conversion (e.g., for each
{k,v} try String.to_existing_atom(k) and if it raises or k is not binary, keep
the original key) so unknown keys are left as strings but known keys become
atoms; this ensures Map.merge(Map.from_struct(invoice), atomized_attrs)
correctly overwrites struct atom fields and lets all_critical_fields_present?/1
and recalculate_extraction_status/2 evaluate against the incoming patch values.
In `@lib/ksef_hub/invoices/invoice.ex`:
- Around line 87-89: Remove :extraction_status and :original_filename from the
cast list in changeset/2 so server-computed fields are not user-writable; keep
recalculate_extraction_status/2 as the place that programmatically sets
extraction_status (it should still Map.put the value after casting), and ensure
original_filename is only assigned on creation (not via changeset updates) —
move any creation-only assignment to the create flow or a separate
create_changeset and do not include these two keys in the cast list of
changeset/2.
---
Duplicate comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 139-147: The current Map.pop(attrs, :pdf_content) only removes
atom-keyed :pdf_content and will miss string-keyed "pdf_content"; update the
extraction to handle both keys so pdf_content is removed regardless of key type
and still applied via Ecto.Changeset.put_change. Replace the single Map.pop call
with a small branch that first tries Map.pop(attrs, :pdf_content) and, if that
returns {nil, _} or unchanged, falls back to Map.pop(attrs, "pdf_content"), then
use the resulting pdf_content when building base and calling
Ecto.Changeset.put_change(base, :pdf_content, pdf_content) (referencing the
variables pdf_content, attrs and the base change/Ecto.Changeset.put_change code
paths).
---
Nitpick comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 462-468: parse_date/1 currently returns nil for ISO8601 datetime
strings (e.g. "2026-02-20T10:30:00"); update the function to pre-normalize the
input by stripping any time component before calling Date.from_iso8601 — for
example, split on "T" (or whitespace) and use the date portion, then call
Date.from_iso8601 and return the {:ok, date} result or nil on failure; make the
change in the private function parse_date/1 to ensure issue_date is populated
when the extraction service returns datetimes.
- Around line 343-344: The `@spec` for create_pdf_upload_invoice currently types
the third parameter as map(), but the implementation accesses opts with
keyword-list syntax (opts[:type], opts[:filename]); update the `@spec` to use
keyword() for the third arg to match usage (i.e., change the third param type
from map() to keyword()) and ensure any callers pass a keyword list matching the
expected keys (type, filename).
In `@lib/ksef_hub/invoices/invoice.ex`:
- Line 175: Create a tracking issue for the ADR 0021 migration and remove the
"TODO" tag from the comment in lib/ksef_hub/invoices/invoice.ex (the comment "#
TODO: move pdf_content to invoice_documents table (ADR 0021)"). Replace it with
a non-TODO note that links to the created issue or references ADR 0021 (e.g., "#
ADR 0021: move pdf_content to invoice_documents table — tracked in ISSUE-123")
so Credo no longer flags it; update the code comment and rerun CI.
In `@lib/ksef_hub/unstructured/client.ex`:
- Around line 63-70: The do_extract/4 success clause currently treats any {:ok,
%{status: status}} the same and returns {:error, {:unstructured_service_error,
200}} for a 200 with non-map body; change do_extract/4 to mirror health/0 by
adding an explicit match for {:ok, %{status: 200, body: body}} that returns
{:error, {:invalid_payload, body}} (and keep the existing {:ok, %{status: 200,
body: body}} -> {:ok, body} for map bodies), leaving the generic {:ok, %{status:
status}} clause for other statuses and adjust the Logger message only for
non-200 statuses so 200 with invalid payload is reported correctly.
- Around line 31-49: health/0 currently reuses `@receive_timeout` (120_000) which
is too long for a probe; add a dedicated shorter module attribute (e.g.
`@health_timeout` 5_000) and use it in the Req.get call inside health/0 instead of
`@receive_timeout`, and verify whether the sidecar /health endpoint requires
authentication — if it does, include the appropriate Authorization header in the
Req.get request (update health/0 to add headers to the Req.get call); reference
functions/values: health/0, `@receive_timeout`, add `@health_timeout`, Req.get, and
fetch_url().
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/unstructured/client.ex
Comment on lines
+236
to
+243
| defp atomize_known_keys(attrs) do | ||
| Map.new(attrs, fn | ||
| {k, v} when is_binary(k) -> {String.to_existing_atom(k), v} | ||
| {k, v} -> {k, v} | ||
| end) | ||
| rescue | ||
| ArgumentError -> attrs | ||
| end |
There was a problem hiding this comment.
atomize_known_keys/1 rescue is all-or-nothing: one unknown key abandons all atom conversions, causing recalculate_extraction_status/2 to evaluate against stale struct values.
When String.to_existing_atom/1 raises for any key, the rescue returns the unmodified attrs with string keys. Map.merge(Map.from_struct(invoice), string_keyed_attrs) then has both :seller_nip => "old_value" (atom, from struct) and "seller_nip" => "new_value" (string, from attrs) coexisting. all_critical_fields_present? checks the atom key first via Map.get(map, field) — if it is non-nil and non-empty, the || short-circuits and the new string-keyed value is never seen. extraction_status is therefore computed against the old invoice values, not the incoming patch.
A per-key safe conversion avoids the all-or-nothing failure:
🛡️ Proposed fix
`@spec` atomize_known_keys(map()) :: map()
defp atomize_known_keys(attrs) do
- Map.new(attrs, fn
- {k, v} when is_binary(k) -> {String.to_existing_atom(k), v}
- {k, v} -> {k, v}
- end)
- rescue
- ArgumentError -> attrs
+ known = Invoice.__schema__(:fields) |> MapSet.new(&Atom.to_string/1)
+
+ Map.new(attrs, fn
+ {k, v} when is_binary(k) ->
+ if MapSet.member?(known, k),
+ do: {String.to_existing_atom(k), v},
+ else: {k, v}
+
+ {k, v} ->
+ {k, v}
+ end)
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 236 - 243, The current
atomize_known_keys/1 rescues ArgumentError for the whole conversion, causing all
keys to remain strings when any single key is unknown and leading
recalculate_extraction_status/2 to read stale atom keys; change
atomize_known_keys/1 to perform a per-key safe conversion (e.g., for each {k,v}
try String.to_existing_atom(k) and if it raises or k is not binary, keep the
original key) so unknown keys are left as strings but known keys become atoms;
this ensures Map.merge(Map.from_struct(invoice), atomized_attrs) correctly
overwrites struct atom fields and lets all_critical_fields_present?/1 and
recalculate_extraction_status/2 evaluate against the incoming patch values.
Comment on lines
+87
to
89
| :extraction_status, | ||
| :original_filename | ||
| ]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the file structure and the changeset function
cd lib/ksef_hub/invoices
if [ -f invoice.ex ]; then
echo "=== invoice.ex context around lines 87-89 ==="
sed -n '70,110p' invoice.ex
fiRepository: appunite/appunite-ksef-ex
Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Find the controller that handles PATCH /invoices/:id
find . -type f -name "*.ex" | xargs grep -l "patch.*invoices" | head -5Repository: appunite/appunite-ksef-ex
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Search for extraction_status being set/used
rg -n "extraction_status" --type=ex -B2 -A2 | head -80Repository: appunite/appunite-ksef-ex
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check for original_filename usage
rg -n "original_filename" --type=ex -B2 -A2 | head -60Repository: appunite/appunite-ksef-ex
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check router for PATCH routes on invoices
grep -n "invoices" lib/ksef_hub_web/router.ex | head -20Repository: appunite/appunite-ksef-ex
Length of output: 1228
🏁 Script executed:
#!/bin/bash
# Look for extraction_status in context or service files
grep -rn "extraction_status" lib/ksef_hub --include="*.ex" | grep -E "(put_change|assign|update)" | head -20Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check the invoices context module for update functions
find lib/ksef_hub -name "*.ex" | xargs grep -l "def update" | head -5Repository: appunite/appunite-ksef-ex
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Look at invoice_live/index.ex for the update handler
if [ -f lib/ksef_hub_web/live/invoice_live/index.ex ]; then
echo "=== Checking invoice_live/index.ex for handle_event update ==="
grep -n "def handle_event" lib/ksef_hub_web/live/invoice_live/index.ex | head -10
fiRepository: appunite/appunite-ksef-ex
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Find the InvoiceController
find . -name "*invoice*controller*" -type fRepository: appunite/appunite-ksef-ex
Length of output: 310
🏁 Script executed:
#!/bin/bash
# Look at the update function in InvoiceController
grep -n "def update" lib/ksef_hub_web/controllers/invoice_controller.ex -A 20Repository: appunite/appunite-ksef-ex
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Check if there's any param filtering for update endpoint
grep -n "update\|permitted_params\|cast" lib/ksef_hub_web/controllers/invoice_controller.ex | head -20Repository: appunite/appunite-ksef-ex
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Also check the invoices context module to see what the update function does
grep -n "def update" lib/ksef_hub/invoices.ex -A 15Repository: appunite/appunite-ksef-ex
Length of output: 1890
🏁 Script executed:
#!/bin/bash
# Check the API InvoiceController update function
grep -n "def update" lib/ksef_hub_web/controllers/api/invoice_controller.ex -A 20Repository: appunite/appunite-ksef-ex
Length of output: 959
🏁 Script executed:
#!/bin/bash
# Check what params are used in the update function
grep -n "update\|params" lib/ksef_hub_web/controllers/api/invoice_controller.ex -B2 -A5 | head -50Repository: appunite/appunite-ksef-ex
Length of output: 2064
🏁 Script executed:
#!/bin/bash
# Check the full update function in API controller
sed -n '1,200p' lib/ksef_hub_web/controllers/api/invoice_controller.ex | grep -n "def update" -A 30Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check all calls to update_invoice
grep -rn "update_invoice" lib/ --include="*.ex"Repository: appunite/appunite-ksef-ex
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Check how extraction_status is actually set (via put_change as mentioned)
grep -rn "put_change.*extraction_status\|extraction_status.*put_change" lib/ --include="*.ex"Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look at the recalculate_extraction_status function more completely
sed -n '227,270p' lib/ksef_hub/invoices.exRepository: appunite/appunite-ksef-ex
Length of output: 1524
Remove extraction_status and original_filename from the changeset/2 cast list — these are server-computed fields and should never be user-modifiable.
The API controller currently filters these fields via @update_allowed_keys, mitigating the immediate risk. However, this violates the security guideline that programmatically-set fields must not appear in cast calls. Since extraction_status is calculated by recalculate_extraction_status/2 (which sets it programmatically via Map.put), and original_filename is immutable after upload, both belong outside the cast list per design.
Recommended approach:
- Remove both fields from
changeset/2's cast list. - Continue setting
extraction_statusprogrammatically viarecalculate_extraction_status/2(already correct). - For
original_filename, ensure it's set only during invoice creation, never during updates. - If future updates to other fields are needed, rely on the changeset validation, not controller-level filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/invoice.ex` around lines 87 - 89, Remove
:extraction_status and :original_filename from the cast list in changeset/2 so
server-computed fields are not user-writable; keep
recalculate_extraction_status/2 as the place that programmatically sets
extraction_status (it should still Map.put the value after casting), and ensure
original_filename is only assigned on creation (not via changeset updates) —
move any creation-only assignment to the create flow or a separate
create_changeset and do not include these two keys in the cast list of
changeset/2.
This was referenced Feb 24, 2026
Merged
Merged
Merged
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
Tests
Chores