Feat/classification UI refactor#94
Merged
Merged
Conversation
|
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 Anthropic-backed emoji generator, migrates category primary key from name→identifier (schema, DB, validations, APIs), refactors Category/Tag LiveViews into Index/Form routes, introduces an Invoice classify LiveView, updates serializers, router, tests, and adds a migration and runtime/config wiring for ANTHROPIC_API_KEY. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Form
participant LiveView as CategoryLive.Form
participant Supervisor as Task.Supervisor
participant Emoji as EmojiGenerator.Client
participant Claude as Anthropic API
rect rgba(200,200,255,0.5)
User->>LiveView: click "Generate Emoji"
LiveView->>Supervisor: start async task (Task.Supervisor.async_nolink)
Supervisor->>Emoji: generate_emoji(context)
end
rect rgba(200,255,200,0.5)
Emoji->>Emoji: build_prompt(identifier,name,description,examples)
Emoji->>Claude: POST /v1/messages (x-api-key header)
Claude-->>Emoji: response (text content)
Emoji->>Emoji: extract_first_emoji(text)
Emoji-->>Supervisor: {:ok, emoji} or {:error, reason}
Supervisor-->>LiveView: handle_info callback (update form)
LiveView-->>User: render form with emoji
end
sequenceDiagram
participant User as User/UI
participant Classify as InvoiceLive.Classify
participant Invoices as Invoices Context
participant DB as Database
User->>Classify: mount with invoice id
Classify->>Invoices: get_invoice(company_id,id)
Invoices->>DB: fetch invoice
DB-->>Invoices: invoice
Invoices-->>Classify: invoice struct
Classify->>Invoices: list_categories(company_id)
Invoices->>DB: fetch categories
DB-->>Invoices: categories
Invoices-->>Classify: categories list
User->>Classify: select category / toggle tag
Classify->>Classify: update local state (selected, expanded)
User->>Classify: save classification
Classify->>Invoices: with_manual_prediction(invoice, attrs)
Invoices->>DB: update invoice prediction/tags
DB-->>Invoices: result
Invoices-->>Classify: {:ok, updated_invoice}
Classify-->>User: navigate to invoice show
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ksef_hub_web/controllers/api/category_controller.ex (2)
89-91:⚠️ Potential issue | 🟡 MinorUpdate validation error description to reflect identifier-based semantics.
The error description still mentions "name is required and must be unique" but the schema now requires
identifierto be unique per company, notname.📝 Proposed fix
422 => - {"Validation error — name is required and must be unique within the company", + {"Validation error — identifier is required and must be unique within the company", "application/json", Schemas.ErrorResponse}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/category_controller.ex` around lines 89 - 91, Update the 422 error description in lib/ksef_hub_web/controllers/api/category_controller.ex where the response map contains the tuple 422 => {"Validation error — name is required and must be unique within the company", "application/json", Schemas.ErrorResponse}; change the human-readable message to state that "identifier is required and must be unique within the company" (or similar identifier-based wording) so it matches the schema semantics for identifier uniqueness while leaving the content-type and Schemas.ErrorResponse reference intact.
130-132:⚠️ Potential issue | 🟡 MinorUpdate validation error description for update operation.
Same issue as create — the description mentions "name must be unique" but the unique constraint is now on
identifier.📝 Proposed fix
422 => - {"Validation error — name must be unique within the company", "application/json", + {"Validation error — identifier must be unique within the company", "application/json", Schemas.ErrorResponse}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/category_controller.ex` around lines 130 - 132, The 422 error mapping in CategoryController's update response currently says "name must be unique within the company" but the unique constraint is on identifier; update the 422 tuple string to reference "identifier must be unique within the company" so the error description matches the constraint (look for the 422 => {"Validation error — name must be unique within the company", "application/json", Schemas.ErrorResponse} tuple in lib/ksef_hub_web/controllers/api/category_controller.ex and replace the message text accordingly).
🧹 Nitpick comments (12)
lib/ksef_hub_web/live/category_live/form.ex (1)
98-103: Consider adding a timeout for emoji generation.If
EmojiGenerator.generate_emoji/1hangs or takes too long, theemoji_loadingstate will remaintrueindefinitely. Consider adding aProcess.send_aftertimeout to reset the loading state.💡 Optional: Add timeout handling
Task.Supervisor.async_nolink(KsefHub.TaskSupervisor, fn -> EmojiGenerator.generate_emoji(context) end) + # Reset loading state after 30 seconds if no response + Process.send_after(self(), :emoji_generation_timeout, :timer.seconds(30)) + {:noreply, socket} end endThen add a handler:
def handle_info(:emoji_generation_timeout, %{assigns: %{emoji_loading: true}} = socket) do {:noreply, socket |> assign(emoji_loading: false) |> put_flash(:error, "Emoji generation timed out.")} end def handle_info(:emoji_generation_timeout, socket), do: {:noreply, socket}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/category_live/form.ex` around lines 98 - 103, The async task that calls EmojiGenerator.generate_emoji/1 via Task.Supervisor.async_nolink currently has no timeout, so emoji_loading can remain true forever; add a Process.send_after/3 when starting the task to send a timeout message (e.g. :emoji_generation_timeout) after a chosen interval, and implement handle_info clauses to catch :emoji_generation_timeout — one that checks %{assigns: %{emoji_loading: true}} and resets assign(:emoji_loading, false) and sets a flash error, and a fallback handle_info(:emoji_generation_timeout, socket) -> {:noreply, socket}; keep references to the existing Task.Supervisor.async_nolink call, EmojiGenerator.generate_emoji, the emoji_loading assign, and the new :emoji_generation_timeout handler names so the change integrates with the current flow.lib/ksef_hub/emoji_generator/behaviour.ex (1)
4-9: Makename,description, andexampleskeys optional in thecategory_contexttypespec.The implementation accesses
identifierdirectly (context.identifier) but uses bracket syntax for the other fields (context[:name],context[:description],context[:examples]), which gracefully handle missing keys. Test cases already pass partial maps with only an identifier. Update the typespec to reflect this:`@type` category_context :: %{ - identifier: String.t(), - name: String.t() | nil, - description: String.t() | nil, - examples: String.t() | nil + required(:identifier) => String.t(), + optional(:name) => String.t() | nil, + optional(:description) => String.t() | nil, + optional(:examples) => String.t() | nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/emoji_generator/behaviour.ex` around lines 4 - 9, The typespec for category_context currently marks name, description and examples as required but the code accesses them with bracket syntax and tests pass maps that only include identifier; update the `@type` category_context to make these keys optional by changing the map spec to use required(:identifier) for identifier and optional(:name), optional(:description), optional(:examples) for the other keys (each typed as String.t() | nil) so the typespec matches runtime usage in the behaviour module.test/ksef_hub_web/live/invoice_live/classify_test.exs (1)
184-187: Scope the checked assertion to the created tag to reduce flakiness.
has_element?(view, ~s(input[checked]))is broad and can pass due to unrelated checked inputs.Based on learnings: "Always reference the key element IDs you added in the LiveView templates in your tests for
Phoenix.LiveViewTestfunctions likeelement/2,has_element/2, selectors, etc".🤖 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/classify_test.exs` around lines 184 - 187, The checked-input assertion is too broad (has_element?/2 with ~s(input[checked])) and can be triggered by unrelated inputs; update the test after render(view) to scope the selector to the specific tag you created (use the tag's DOM id or a unique surrounding element) and call has_element?(view, "<scoped selector for that tag>") or use element(view, "<scoped selector>") then assert its checked state—target the unique id/class for the "brand-new-tag" input so only that input is matched.lib/ksef_hub_web/live/tag_live/form.ex (1)
10-20: Add@docfor public callbacks in this LiveView.Public callbacks are typed but undocumented.
As per coding guidelines: "Every public function must have
@docdocumentation in Elixir".Also applies to: 61-77, 129-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 10 - 20, Add `@doc` strings for all public callbacks and functions in this LiveView module: at minimum document mount/3 and handle_params/3 (both are public callbacks), and likewise add `@doc` for the other public functions in the file referenced (the functions in the blocks you noted around lines 61-77 and 129-166). For each function (e.g., mount, handle_params and any public helpers or callbacks in those ranges) add a short description of purpose, the expected args and return shape (socket updates or {:noreply, socket}), and any important side effects so the module adheres to the "every public function must have `@doc`" guideline.lib/ksef_hub_web/live/tag_live/index.ex (1)
13-52: Add@docfor public LiveView callbacks.
mount/3,handle_event/3, andrender/1are public and currently undocumented.As per coding guidelines: "Every public function must have
@docdocumentation in Elixir".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/index.ex` around lines 13 - 52, Add `@doc` annotations to the public LiveView callbacks mount/3, handle_event/3, and render/1 describing their purpose and expected parameters/returns; for mount/3 explain it initializes assigns (page_title and company_id) and streams tags, for handle_event/3 describe handling the "delete" event and possible outcomes (invalid id, not found, deletion error), and for render/1 state that it returns the LiveView rendered template using assigns—place each `@doc` immediately above the corresponding function definition (mount, handle_event, render).lib/ksef_hub_web/live/category_live/index.ex (1)
12-50: Add@docto public callbacks in this module.
mount/3,handle_event/3, andrender/1should be documented to satisfy module-level API standards.As per coding guidelines: "Every public function must have
@docdocumentation in Elixir".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/category_live/index.ex` around lines 12 - 50, Add `@doc` documentation for the public callbacks mount/3, handle_event/3, and render/1 in this LiveView module: place a short descriptive `@doc` above the mount/3 function explaining it mounts the socket and assigns page_title and streams categories for the current company, above handle_event/3 describing the "delete" event flow (ID casting, lookup, deletion, and flash/stream updates), and above render/1 stating it returns the LiveView rendering; include brief notes on params and return types consistent with existing `@specs` and keep the docs concise.lib/ksef_hub_web/live/invoice_live/show.ex (1)
517-519: Dropfind_category/2and render the preloaded association directly.
Invoices.get_invoice_with_details/3already preloads:category, so keeping a full@categoriesassign just toEnum.find/2the current one adds an extra query and more socket state than this page needs.Based on learnings: Applies to **/*_live.ex : Keep socket assigns minimal and flat when possible.
Also applies to: 746-750
🤖 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 517 - 519, Remove the find_category/2 helper and stop assigning `@categories`; Invoices.get_invoice_with_details/3 already preloads the :category association, so update templates/assigns to use the preloaded invoice.category directly (replace calls to find_category(invoice.category_id, `@categories`) or similar with invoice.category), remove the `@categories` socket assign and any Enum.find usage, and delete the private function find_category/2; apply the same change for the other occurrence referenced (the block around the second occurrence).test/ksef_hub_web/live/tag_live_test.exs (2)
24-24: Mirror theTagLive.Index/TagLive.Formsplit in the test layout too.These describe blocks now cover two different LiveViews, but they still live in one catch-all test module. Moving them into mirrored
index_test.exs/form_test.exsfiles will make failures much easier to localize.As per coding guidelines:
test/**/*_test.exs: Test files must mirror source path structure (e.g.,test/ksef_hub/invoices_test.exsforlib/ksef_hub/invoices.ex).Also applies to: 70-70, 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/tag_live_test.exs` at line 24, The test module currently mixes two LiveView specs; split the describe blocks for TagLive.Index and TagLive.Form into two separate test files mirroring the source layout: create test/ksef_hub_web/live/tag_live/index_test.exs for tests targeting TagLive.Index and test/ksef_hub_web/live/tag_live/form_test.exs for tests targeting TagLive.Form, move the respective describe blocks and any helper setup into the corresponding files, update module names and describe titles to reflect TagLive.Index and TagLive.Form, and ensure any shared setup or helpers are extracted into a common support module or imported so both new test files run independently and follow the test/**/*_test.exs naming guideline.
25-130: Use selectors instead of raw HTML/text matches in these LiveView tests.
html =~andrender(view)here are tied to incidental markup/text, so small template tweaks will make the suite noisy. The views already expose stable hooks like#tags,#tag-form, anddata-testid="tag-name-#{tag.id}"; please switch these checks tohas_element?/2andelement/2.Based on learnings: Applies to /test//*_live_test.{exs,ex} : Never test against raw HTML - always use
element/2,has_element/2, and similar functions with selectors likeassert has_element?(view, "#my-form").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/tag_live_test.exs` around lines 25 - 130, Replace raw HTML/text assertions with selector-based LiveView helpers: use has_element?/2 and element/2 instead of html =~ or render(view) checks. For list/show tests assert has_element?(view_or_html, "#tags") and assert has_element?(view, ~s([data-testid="tag-name-#{tag.id}"])) to check tag presence; for deletes use element(view, ~s([data-testid="tag-name-#{tag.id}"])) |> render_click(...) (or render_click on the delete button selector) and then refute has_element?(view, ~s([data-testid="tag-name-#{tag.id}"])). For forms keep element("form#tag-form") and render_submit/2, and keep assert_redirect(view, ...) and flash checks as-is (assert_redirect/2, render_submit/1, render_click/1, has_element?/2, element/2).test/ksef_hub_web/live/invoice_live/show_test.exs (2)
277-316: Scope these classification assertions to the new selectors.The page now exposes
category-display,tags-display, andedit-classification, but these tests still fall back torender(view)text matching. That can pass if the same text shows up elsewhere on the page and won't catch a bad classify href.Based on learnings: Applies to /test//*_live_test.{exs,ex} : Always reference the key element IDs you added in the LiveView templates in your tests for
Phoenix.LiveViewTestfunctions likeelement/2,has_element/2, selectors, etc.🤖 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 277 - 316, Update the tests to assert against the specific LiveView DOM selectors instead of relying on render(view) text matching: for the "displays category name and emoji" test use has_element?/3 or element(view, "[data-testid=category-display]") and assert its rendered content includes "Invoices" and "💰"; for the "shows 'No category' placeholder when nil" test keep the has_element? call but target "[data-testid=category-display]" and check the inner text equals "-" via element/2+render_change or has_element?/3; for the tags assertion, replace html =~ "quarterly-report" with has_element?(view, "[data-testid=tags-display]", "quarterly-report") or element(view, "[data-testid=tags-display]") and check its text; for the edit link test ensure you call has_element?(view, ~s([data-testid="edit-classification"])) (or element(view, selector) and assert the href) so each assertion targets category-display, tags-display, and edit-classification explicitly rather than matching the whole page render.
114-120: Avoid regex-matching the whole header HTML here.This check is fragile against
<.header>markup changes and doesn't anchor to any stable element. Add a selector/test id for the action bar, or assert directly on the specific add-payment control instead.Based on learnings: Applies to /test//*_live_test.{exs,ex} : Never test against raw HTML - always use
element/2,has_element/2, and similar functions with selectors likeassert has_element?(view, "#my-form").🤖 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 114 - 120, The test "does not show Add payment button in top action bar" is fragile because it regex-matches the whole header HTML; change it to bind the LiveView (use {:ok, view, _html} = live(conn, ~p"...") instead of {:ok, _view, html}) and replace the regex refute with a DOM assertion such as refute has_element?(view, "#top-action-bar .add-payment") or refute has_element?(view, "[data-testid=\"add-payment-button\"]"); if no stable selector exists, add a data-testid or id to the action bar or add-payment control in the template and use that selector with element/2 / has_element?/2.test/ksef_hub_web/live/category_live_test.exs (1)
28-31: Prefer stable selectors over raw HTML/text matches in these LiveView tests.These cases are still coupled to copy (
html =~ ...), button labels (element("button", "Delete")), and broad substrings likeassert html =~ "5"even though the views already expose stable hooks like#categories,#category-form, anddata-testid="category-identifier-#{cat.id}". Switching tohas_element?/2/element/2against IDs or testids will make the suite much less brittle; the action buttons should get stable ids/testids if you need to click them.As per coding guidelines, "Never test against raw HTML - always use
element/2,has_element/2, and similar functions with selectors likeassert has_element?(view, "#my-form")" and "Always reference the key element IDs you added in the LiveView templates in your tests forPhoenix.LiveViewTestfunctions likeelement/2,has_element/2, selectors, etc".Also applies to: 37-39, 42-52, 55-60, 65-68, 110-115, 167-170, 202-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/category_live_test.exs` around lines 28 - 31, The tests in category_live_test.exs are matching raw HTML/text (e.g., using html =~ "Categories", html =~ "New Category", raw counts like "5" and button text) which is brittle; update the assertions to use LiveViewTest selectors: replace html =~ checks with assert has_element?(view, "#categories") and assert has_element?(view, "#category-form"), and use element/2 or has_element?/2 with stable IDs/testids for specific rows and buttons (e.g., assert has_element?(view, " [data-testid=\"category-identifier-#{cat.id}\"]") and use element(view, "button[data-testid=\"delete-category-#{cat.id}\"]") to click), and change any raw count assertions to check for the specific element containing the count via its selector instead of matching the text "5".
🤖 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/classify.ex`:
- Around line 119-132: The current handle_event implementations for "create_tag"
and "save" use `unless ... else` and trigger Credo warnings; refactor them to
use pattern-matched function heads or `if` so the unauthorized and authorized
flows are explicit. For example, add separate clauses for
handle_event("create_tag", %{"name" => name}, socket = %{assigns:
%{can_manage_tags: false}}) that immediately return {:noreply, put_flash(...)}
and a second clause for when can_manage_tags is true that calls
create_tag(socket, name); do the same for handle_event("save", _params, socket)
using can_set_category or can_set_tags (pattern-match the authorized case or use
an `if` for the combined condition) so the unauthorized path is its own clause
and the authorized path calls save_classification(socket).
- Around line 104-113: The handler handle_event("toggle_tag", ...) currently
trusts the client-provided "tag-id" and saves selected_tag_ids later via
save_classification/1 -> Invoices.set_invoice_tags/2; update both toggle_tag and
save_classification/1 to revalidate/sanitize tag IDs against the canonical list
in socket.assigns (e.g., socket.assigns.all_tags or company-scoped tag IDs)
before mutating state or calling Invoices.set_invoice_tags/2: on toggle, ignore
or reject tag_id values not present in the allowed set (and coerce types
consistently), and in save_classification/1 filter the selected_tag_ids set to
only include allowed IDs before passing to Invoices.set_invoice_tags/2; apply
the same whitelist validation to the other similar handler block around the
177-189 region.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 735-743: The UI currently nests a <.button> inside a <.link> (in
the render block that checks `@can_set_category/`@can_set_tags and navigates to
~p"/c/#{`@current_company.id`}/invoices/#{`@invoice.id`}/classify"), which creates
two interactive controls; remove the nested control and render a single
navigable element instead—either turn the outer <.link> into a button-styled
link (apply the button classes/variant and keep the <.icon
name="hero-pencil-square" /> and "Edit" label) or keep a <.button> and move the
navigate attribute onto it so it performs the navigation directly; update only
the fragment around the <.link>/<.button> that contains
data-testid="edit-classification" to ensure one accessible control remains.
In `@lib/ksef_hub/emoji_generator/client.ex`:
- Around line 44-62: The current Req.post result handling treats any
non-matching 200 body as a generic non-200 API error; add a dedicated clause for
{:ok, %{status: 200, body: resp_body}} that runs when the successful-status
pattern (the existing %{"content" => [%{"text" => text} | _]}) didn't match: log
a clear warning including resp_body and context.identifier (use Logger.warning
similar to other branches), return a distinct error like {:error,
{:unexpected_body, resp_body}} (or {:error, :unexpected_200_body}) so callers
can distinguish unexpected 200 payloads from real API errors; update the case
handling around Req.post/2 (the existing match that extracts text and the
fallback {:ok, %{status: status, body: resp_body}}) to include this new
200/unexpected-body branch.
In `@lib/ksef_hub/exports/csv_builder.ex`:
- Around line 120-122: The pattern matching in format_category/1 currently
checks for category.name before category.identifier, so when both exist the
identifier is ignored; reorder the clauses in format_category/1 to check for
%{category: %{identifier: id}} (is_binary(id)) before the %{category: %{name:
name}} clause so identifier is returned when present, keeping the fallback defp
format_category(_), do: "" unchanged.
In `@lib/ksef_hub/invoices/tag.ex`:
- Around line 29-32: The unique_constraint call in the Tag changeset uses the
wrong DB index name; update the unique_constraint invocation (the call to
unique_constraint/2 in the Tag changeset pipeline) to use name:
:tags_company_id_name_index so it matches the actual migration-created index
(unique_index(:tags, [:company_id, :name])) and ensure error_key: :name remains
unchanged.
In `@test/ksef_hub_web/live/category_live_test.exs`:
- Around line 191-200: The test currently asserts absence of "Generating" on
initial render and sends a synthetic {:DOWN} which doesn't exercise the real
flow; instead, drive the Auto generation flow on the LiveView (use the same
event you use to start generation — e.g., call render_submit or element(...,
"click"/"submit") against the view) so the UI enters the loading state (assert
render(view) =~ "Generating"), then simulate the failure path and assert
recovery/error UI (e.g., stub the async job to fail or send the {:DOWN, ...}
after the generation event started) and finally assert the LiveView is still
alive and shows the expected error/recovered state (keep references to view,
render, render_submit/element and the "Generating" string to locate code).
- Around line 90-105: The test currently only checks for the static hint text
from lib/ksef_hub_web/live/category_live/form.ex, so change the assertion to
verify the identifier field's actual validation feedback after submit: keep
using the same live page and form ("form#category-form") and after render_submit
assert that the rendered view contains the identifier field error by checking
the phx-feedback element for that input (selector with
phx-feedback-for="category[identifier]") or the exact error message shown for
invalid format; in short, replace assert html =~ "group:target" with an
assertion that the view includes the validation error for category[identifier]
(e.g., element("[phx-feedback-for=\"category[identifier]\"]") or the concrete
error string).
In `@test/ksef_hub/emoji_generator/client_test.exs`:
- Around line 9-145: The tests mutate Application.put_env(:ksef_hub,
:anthropic_api_key, ...) but use invalid `after` blocks so cleanup never runs;
replace those `after` blocks with proper cleanup using
ExUnit.Callbacks.on_exit/1 or wrap each mutation in try ... after inside the
test body. Concretely, in tests that call Application.put_env before invoking
KsefHub.EmojiGenerator.Client.generate_emoji (and in stubs that read the request
body), ensure you call on_exit(fn -> Application.delete_env(:ksef_hub,
:anthropic_api_key) end) (or wrap the put/delete in try ... after) so the
Application.delete_env is always executed; keep references to
Client.generate_emoji and Req.Test.stub usages intact while moving the cleanup
logic into on_exit/1 or try...after.
---
Outside diff comments:
In `@lib/ksef_hub_web/controllers/api/category_controller.ex`:
- Around line 89-91: Update the 422 error description in
lib/ksef_hub_web/controllers/api/category_controller.ex where the response map
contains the tuple 422 => {"Validation error — name is required and must be
unique within the company", "application/json", Schemas.ErrorResponse}; change
the human-readable message to state that "identifier is required and must be
unique within the company" (or similar identifier-based wording) so it matches
the schema semantics for identifier uniqueness while leaving the content-type
and Schemas.ErrorResponse reference intact.
- Around line 130-132: The 422 error mapping in CategoryController's update
response currently says "name must be unique within the company" but the unique
constraint is on identifier; update the 422 tuple string to reference
"identifier must be unique within the company" so the error description matches
the constraint (look for the 422 => {"Validation error — name must be unique
within the company", "application/json", Schemas.ErrorResponse} tuple in
lib/ksef_hub_web/controllers/api/category_controller.ex and replace the message
text accordingly).
---
Nitpick comments:
In `@lib/ksef_hub_web/live/category_live/form.ex`:
- Around line 98-103: The async task that calls EmojiGenerator.generate_emoji/1
via Task.Supervisor.async_nolink currently has no timeout, so emoji_loading can
remain true forever; add a Process.send_after/3 when starting the task to send a
timeout message (e.g. :emoji_generation_timeout) after a chosen interval, and
implement handle_info clauses to catch :emoji_generation_timeout — one that
checks %{assigns: %{emoji_loading: true}} and resets assign(:emoji_loading,
false) and sets a flash error, and a fallback
handle_info(:emoji_generation_timeout, socket) -> {:noreply, socket}; keep
references to the existing Task.Supervisor.async_nolink call,
EmojiGenerator.generate_emoji, the emoji_loading assign, and the new
:emoji_generation_timeout handler names so the change integrates with the
current flow.
In `@lib/ksef_hub_web/live/category_live/index.ex`:
- Around line 12-50: Add `@doc` documentation for the public callbacks mount/3,
handle_event/3, and render/1 in this LiveView module: place a short descriptive
`@doc` above the mount/3 function explaining it mounts the socket and assigns
page_title and streams categories for the current company, above handle_event/3
describing the "delete" event flow (ID casting, lookup, deletion, and
flash/stream updates), and above render/1 stating it returns the LiveView
rendering; include brief notes on params and return types consistent with
existing `@specs` and keep the docs concise.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 517-519: Remove the find_category/2 helper and stop assigning
`@categories`; Invoices.get_invoice_with_details/3 already preloads the :category
association, so update templates/assigns to use the preloaded invoice.category
directly (replace calls to find_category(invoice.category_id, `@categories`) or
similar with invoice.category), remove the `@categories` socket assign and any
Enum.find usage, and delete the private function find_category/2; apply the same
change for the other occurrence referenced (the block around the second
occurrence).
In `@lib/ksef_hub_web/live/tag_live/form.ex`:
- Around line 10-20: Add `@doc` strings for all public callbacks and functions in
this LiveView module: at minimum document mount/3 and handle_params/3 (both are
public callbacks), and likewise add `@doc` for the other public functions in the
file referenced (the functions in the blocks you noted around lines 61-77 and
129-166). For each function (e.g., mount, handle_params and any public helpers
or callbacks in those ranges) add a short description of purpose, the expected
args and return shape (socket updates or {:noreply, socket}), and any important
side effects so the module adheres to the "every public function must have `@doc`"
guideline.
In `@lib/ksef_hub_web/live/tag_live/index.ex`:
- Around line 13-52: Add `@doc` annotations to the public LiveView callbacks
mount/3, handle_event/3, and render/1 describing their purpose and expected
parameters/returns; for mount/3 explain it initializes assigns (page_title and
company_id) and streams tags, for handle_event/3 describe handling the "delete"
event and possible outcomes (invalid id, not found, deletion error), and for
render/1 state that it returns the LiveView rendered template using
assigns—place each `@doc` immediately above the corresponding function definition
(mount, handle_event, render).
In `@lib/ksef_hub/emoji_generator/behaviour.ex`:
- Around line 4-9: The typespec for category_context currently marks name,
description and examples as required but the code accesses them with bracket
syntax and tests pass maps that only include identifier; update the `@type`
category_context to make these keys optional by changing the map spec to use
required(:identifier) for identifier and optional(:name),
optional(:description), optional(:examples) for the other keys (each typed as
String.t() | nil) so the typespec matches runtime usage in the behaviour module.
In `@test/ksef_hub_web/live/category_live_test.exs`:
- Around line 28-31: The tests in category_live_test.exs are matching raw
HTML/text (e.g., using html =~ "Categories", html =~ "New Category", raw counts
like "5" and button text) which is brittle; update the assertions to use
LiveViewTest selectors: replace html =~ checks with assert has_element?(view,
"#categories") and assert has_element?(view, "#category-form"), and use
element/2 or has_element?/2 with stable IDs/testids for specific rows and
buttons (e.g., assert has_element?(view, "
[data-testid=\"category-identifier-#{cat.id}\"]") and use element(view,
"button[data-testid=\"delete-category-#{cat.id}\"]") to click), and change any
raw count assertions to check for the specific element containing the count via
its selector instead of matching the text "5".
In `@test/ksef_hub_web/live/invoice_live/classify_test.exs`:
- Around line 184-187: The checked-input assertion is too broad (has_element?/2
with ~s(input[checked])) and can be triggered by unrelated inputs; update the
test after render(view) to scope the selector to the specific tag you created
(use the tag's DOM id or a unique surrounding element) and call
has_element?(view, "<scoped selector for that tag>") or use element(view,
"<scoped selector>") then assert its checked state—target the unique id/class
for the "brand-new-tag" input so only that input is matched.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 277-316: Update the tests to assert against the specific LiveView
DOM selectors instead of relying on render(view) text matching: for the
"displays category name and emoji" test use has_element?/3 or element(view,
"[data-testid=category-display]") and assert its rendered content includes
"Invoices" and "💰"; for the "shows 'No category' placeholder when nil" test
keep the has_element? call but target "[data-testid=category-display]" and check
the inner text equals "-" via element/2+render_change or has_element?/3; for the
tags assertion, replace html =~ "quarterly-report" with has_element?(view,
"[data-testid=tags-display]", "quarterly-report") or element(view,
"[data-testid=tags-display]") and check its text; for the edit link test ensure
you call has_element?(view, ~s([data-testid="edit-classification"])) (or
element(view, selector) and assert the href) so each assertion targets
category-display, tags-display, and edit-classification explicitly rather than
matching the whole page render.
- Around line 114-120: The test "does not show Add payment button in top action
bar" is fragile because it regex-matches the whole header HTML; change it to
bind the LiveView (use {:ok, view, _html} = live(conn, ~p"...") instead of {:ok,
_view, html}) and replace the regex refute with a DOM assertion such as refute
has_element?(view, "#top-action-bar .add-payment") or refute has_element?(view,
"[data-testid=\"add-payment-button\"]"); if no stable selector exists, add a
data-testid or id to the action bar or add-payment control in the template and
use that selector with element/2 / has_element?/2.
In `@test/ksef_hub_web/live/tag_live_test.exs`:
- Line 24: The test module currently mixes two LiveView specs; split the
describe blocks for TagLive.Index and TagLive.Form into two separate test files
mirroring the source layout: create
test/ksef_hub_web/live/tag_live/index_test.exs for tests targeting TagLive.Index
and test/ksef_hub_web/live/tag_live/form_test.exs for tests targeting
TagLive.Form, move the respective describe blocks and any helper setup into the
corresponding files, update module names and describe titles to reflect
TagLive.Index and TagLive.Form, and ensure any shared setup or helpers are
extracted into a common support module or imported so both new test files run
independently and follow the test/**/*_test.exs naming guideline.
- Around line 25-130: Replace raw HTML/text assertions with selector-based
LiveView helpers: use has_element?/2 and element/2 instead of html =~ or
render(view) checks. For list/show tests assert has_element?(view_or_html,
"#tags") and assert has_element?(view, ~s([data-testid="tag-name-#{tag.id}"]))
to check tag presence; for deletes use element(view,
~s([data-testid="tag-name-#{tag.id}"])) |> render_click(...) (or render_click on
the delete button selector) and then refute has_element?(view,
~s([data-testid="tag-name-#{tag.id}"])). For forms keep element("form#tag-form")
and render_submit/2, and keep assert_redirect(view, ...) and flash checks as-is
(assert_redirect/2, render_submit/1, render_click/1, has_element?/2, element/2).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bc87c56-f300-402f-b3f9-0c17ef5a0d47
📒 Files selected for processing (45)
.env.exampleCLAUDE.mdcloud-run/service.yamlconfig/config.exsconfig/runtime.exsconfig/test.exsdocs/sidecar-services.mdlib/ksef_hub/emoji_generator.exlib/ksef_hub/emoji_generator/behaviour.exlib/ksef_hub/emoji_generator/client.exlib/ksef_hub/exports/csv_builder.exlib/ksef_hub/invoice_classifier.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/category.exlib/ksef_hub/invoices/tag.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/controllers/api/category_controller.exlib/ksef_hub_web/json_helpers.exlib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/category_live/form.exlib/ksef_hub_web/live/category_live/index.exlib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.exlib/ksef_hub_web/live/tag_live/form.exlib/ksef_hub_web/live/tag_live/index.exlib/ksef_hub_web/router.exlib/ksef_hub_web/schemas/category.exlib/ksef_hub_web/schemas/create_category_request.exlib/ksef_hub_web/schemas/update_category_request.expriv/repo/migrations/20260318090451_extend_categories.exstest/ksef_hub/emoji_generator/client_test.exstest/ksef_hub/emoji_generator_test.exstest/ksef_hub/invoice_classifier_test.exstest/ksef_hub/invoices/categories_test.exstest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/live/category_live_test.exstest/ksef_hub_web/live/invoice_live/classify_test.exstest/ksef_hub_web/live/invoice_live/index_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/tag_live_test.exstest/support/factory.extest/test_helper.exs
💤 Files with no reviewable changes (3)
- cloud-run/service.yaml
- lib/ksef_hub_web/live/category_live.ex
- lib/ksef_hub_web/live/tag_live.ex
Comment on lines
+44
to
+62
| case Req.post(req(), url: "/v1/messages", body: body) do | ||
| {:ok, %{status: 200, body: %{"content" => [%{"text" => text} | _]}}} -> | ||
| emoji = text |> String.trim() |> extract_first_emoji() | ||
|
|
||
| if emoji do | ||
| Logger.info("[EmojiGenerator] Generated #{emoji} for #{inspect(context.identifier)}") | ||
| {:ok, emoji} | ||
| else | ||
| Logger.warning( | ||
| "[EmojiGenerator] No emoji found in response #{inspect(text)} for #{inspect(context.identifier)}" | ||
| ) | ||
|
|
||
| {:error, :no_emoji_in_response} | ||
| end | ||
|
|
||
| {:ok, %{status: status, body: resp_body}} -> | ||
| Logger.warning("[EmojiGenerator] API error status=#{status} body=#{inspect(resp_body)}") | ||
|
|
||
| {:error, {:api_error, status}} |
There was a problem hiding this comment.
Handle unexpected 200 bodies separately from non-200 API errors.
A successful HTTP response with an empty or non-text content payload currently falls through to {:error, {:api_error, 200}}. That makes diagnostics and retry behavior misleading; add a dedicated 200/unexpected-body branch.
🩹 Suggested response split
- {:ok, %{status: status, body: resp_body}} ->
+ {:ok, %{status: 200, body: resp_body}} ->
+ Logger.warning("[EmojiGenerator] Unexpected 200 response body=#{inspect(resp_body)}")
+ {:error, :unexpected_response}
+
+ {:ok, %{status: status, body: resp_body}} ->
Logger.warning("[EmojiGenerator] API error status=#{status} body=#{inspect(resp_body)}")
{:error, {:api_error, status}}📝 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
| case Req.post(req(), url: "/v1/messages", body: body) do | |
| {:ok, %{status: 200, body: %{"content" => [%{"text" => text} | _]}}} -> | |
| emoji = text |> String.trim() |> extract_first_emoji() | |
| if emoji do | |
| Logger.info("[EmojiGenerator] Generated #{emoji} for #{inspect(context.identifier)}") | |
| {:ok, emoji} | |
| else | |
| Logger.warning( | |
| "[EmojiGenerator] No emoji found in response #{inspect(text)} for #{inspect(context.identifier)}" | |
| ) | |
| {:error, :no_emoji_in_response} | |
| end | |
| {:ok, %{status: status, body: resp_body}} -> | |
| Logger.warning("[EmojiGenerator] API error status=#{status} body=#{inspect(resp_body)}") | |
| {:error, {:api_error, status}} | |
| case Req.post(req(), url: "/v1/messages", body: body) do | |
| {:ok, %{status: 200, body: %{"content" => [%{"text" => text} | _]}}} -> | |
| emoji = text |> String.trim() |> extract_first_emoji() | |
| if emoji do | |
| Logger.info("[EmojiGenerator] Generated #{emoji} for #{inspect(context.identifier)}") | |
| {:ok, emoji} | |
| else | |
| Logger.warning( | |
| "[EmojiGenerator] No emoji found in response #{inspect(text)} for #{inspect(context.identifier)}" | |
| ) | |
| {:error, :no_emoji_in_response} | |
| end | |
| {:ok, %{status: 200, body: resp_body}} -> | |
| Logger.warning("[EmojiGenerator] Unexpected 200 response body=#{inspect(resp_body)}") | |
| {:error, :unexpected_response} | |
| {:ok, %{status: status, body: resp_body}} -> | |
| Logger.warning("[EmojiGenerator] API error status=#{status} body=#{inspect(resp_body)}") | |
| {:error, {:api_error, status}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/emoji_generator/client.ex` around lines 44 - 62, The current
Req.post result handling treats any non-matching 200 body as a generic non-200
API error; add a dedicated clause for {:ok, %{status: 200, body: resp_body}}
that runs when the successful-status pattern (the existing %{"content" =>
[%{"text" => text} | _]}) didn't match: log a clear warning including resp_body
and context.identifier (use Logger.warning similar to other branches), return a
distinct error like {:error, {:unexpected_body, resp_body}} (or {:error,
:unexpected_200_body}) so callers can distinguish unexpected 200 payloads from
real API errors; update the case handling around Req.post/2 (the existing match
that extracts text and the fallback {:ok, %{status: status, body: resp_body}})
to include this new 200/unexpected-body branch.
Comment on lines
120
to
122
| defp format_category(%{category: %{name: name}}) when is_binary(name), do: name | ||
| defp format_category(%{category: %{identifier: id}}) when is_binary(id), do: id | ||
| defp format_category(_), do: "" |
There was a problem hiding this comment.
Prioritize category.identifier before category.name in format_category/1.
At Line 120, the name clause matches first, so the new Line 121 identifier clause is skipped when both fields are present. This prevents identifier-based CSV output.
Suggested fix
- defp format_category(%{category: %{name: name}}) when is_binary(name), do: name
- defp format_category(%{category: %{identifier: id}}) when is_binary(id), do: id
+ defp format_category(%{category: %{identifier: id}}) when is_binary(id), do: id
+ defp format_category(%{category: %{name: name}}) when is_binary(name), do: name
defp format_category(_), do: ""📝 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
| defp format_category(%{category: %{name: name}}) when is_binary(name), do: name | |
| defp format_category(%{category: %{identifier: id}}) when is_binary(id), do: id | |
| defp format_category(_), do: "" | |
| defp format_category(%{category: %{identifier: id}}) when is_binary(id), do: id | |
| defp format_category(%{category: %{name: name}}) when is_binary(name), do: name | |
| defp format_category(_), do: "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/exports/csv_builder.ex` around lines 120 - 122, The pattern
matching in format_category/1 currently checks for category.name before
category.identifier, so when both exist the identifier is ignored; reorder the
clauses in format_category/1 to check for %{category: %{identifier: id}}
(is_binary(id)) before the %{category: %{name: name}} clause so identifier is
returned when present, keeping the fallback defp format_category(_), do: ""
unchanged.
Comment on lines
+29
to
+32
| |> unique_constraint([:company_id, :name], | ||
| error_key: :name, | ||
| name: :tags_company_id_name_type_index | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual index name in migrations for the tags table
rg -n "create.*index.*tags" --type=elixir
rg -n "unique_index.*tags" --type=elixirRepository: appunite/appunite-ksef-ex
Length of output: 712
🏁 Script executed:
# Check all migrations that touch the tags table
rg -n "tags" priv/repo/migrations/ --type=elixir -B2 -A2
# Also check the exact line in the tags migration to confirm
cat -n priv/repo/migrations/20260223063932_create_tags.exs | sed -n '14,35p'Repository: appunite/appunite-ksef-ex
Length of output: 2686
🏁 Script executed:
# Get the full tags table definition from the migration
cat -n priv/repo/migrations/20260223063932_create_tags.exs | sed -n '5,18p'
# Also check the Tag schema to see all columns
cat -n lib/ksef_hub/invoices/tag.exRepository: appunite/appunite-ksef-ex
Length of output: 1816
Constraint name must match the actual database index.
The constraint name tags_company_id_name_type_index does not match the database index created in the migration. The migration creates unique_index(:tags, [:company_id, :name]) which generates the index name tags_company_id_name_index (without _type_). There is no :type column in the tags table.
Change the constraint name to tags_company_id_name_index to match the actual database index. Otherwise, validation errors won't be properly matched to this constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/tag.ex` around lines 29 - 32, The unique_constraint
call in the Tag changeset uses the wrong DB index name; update the
unique_constraint invocation (the call to unique_constraint/2 in the Tag
changeset pipeline) to use name: :tags_company_id_name_index so it matches the
actual migration-created index (unique_index(:tags, [:company_id, :name])) and
ensure error_key: :name remains unchanged.
Comment on lines
+90
to
105
| test "shows error for invalid identifier format", %{conn: conn, company: company} do | ||
| {:ok, view, _html} = live(conn, ~p"/c/#{company.id}/categories/new") | ||
|
|
||
| view | ||
| |> element("form#category-form") | ||
| |> render_submit(%{ | ||
| category: %{name: "no-colon", emoji: "", description: "", sort_order: "0"} | ||
| category: %{ | ||
| identifier: "no-colon", | ||
| emoji: "", | ||
| description: "", | ||
| sort_order: "0" | ||
| } | ||
| }) | ||
|
|
||
| html = render(view) | ||
| assert html =~ "group:target" |
There was a problem hiding this comment.
The invalid-identifier test can pass without any validation error.
lib/ksef_hub_web/live/category_live/form.ex already renders Used by the ML classifier. Format: group:target unconditionally, so assert html =~ "group:target" does not prove the submit produced an error. Please assert on the identifier field's actual error state/message instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ksef_hub_web/live/category_live_test.exs` around lines 90 - 105, The
test currently only checks for the static hint text from
lib/ksef_hub_web/live/category_live/form.ex, so change the assertion to verify
the identifier field's actual validation feedback after submit: keep using the
same live page and form ("form#category-form") and after render_submit assert
that the rendered view contains the identifier field error by checking the
phx-feedback element for that input (selector with
phx-feedback-for="category[identifier]") or the exact error message shown for
invalid format; in short, replace assert html =~ "group:target" with an
assertion that the view includes the validation error for category[identifier]
(e.g., element("[phx-feedback-for=\"category[identifier]\"]") or the concrete
error string).
emilwojtaszek
force-pushed
the
feat/classification-ui-refactor
branch
from
a7aa131 to
9f4f423
Compare
Member
Author
|
@rabbitcodeai please make full review, not sure if I applied previos suggestions, so if not please let me know |
1 similar comment
Member
Author
|
@rabbitcodeai please make full review, not sure if I applied previos suggestions, so if not please let me know |
Member
Author
|
@coderabbitai please make full review, not sure if I applied previos suggestions, so if not please let me know |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lib/ksef_hub_web/live/invoice_live/classify.ex (1)
106-115:⚠️ Potential issue | 🟠 MajorWhitelist tag IDs before updating or saving the selection.
toggle_tagtrusts whatever"tag-id"the client sends, andsave_classification/1forwards that set straight intoInvoices.set_invoice_tags/2. A forged LiveView event can therefore attach tags outside@all_tagsand outside the current company scope.🔒 Revalidate against the server-side tag list
def handle_event("toggle_tag", %{"tag-id" => tag_id}, socket) do if socket.assigns.can_set_tags do current = socket.assigns.selected_tag_ids + allowed_tag_ids = + socket.assigns.all_tags + |> Enum.map(& &1.id) + |> MapSet.new() - updated = - if MapSet.member?(current, tag_id), - do: MapSet.delete(current, tag_id), - else: MapSet.put(current, tag_id) - - {:noreply, assign(socket, :selected_tag_ids, updated)} + if MapSet.member?(allowed_tag_ids, tag_id) do + updated = + if MapSet.member?(current, tag_id), + do: MapSet.delete(current, tag_id), + else: MapSet.put(current, tag_id) + + {:noreply, assign(socket, :selected_tag_ids, updated)} + else + {:noreply, socket} + end else {:noreply, put_flash(socket, :error, "You don't have permission to manage tags.")} end end ... defp save_classification(socket) do invoice = socket.assigns.invoice category_id = socket.assigns.selected_category_id - tag_ids = MapSet.to_list(socket.assigns.selected_tag_ids) + allowed_tag_ids = + socket.assigns.all_tags + |> Enum.map(& &1.id) + |> MapSet.new() + + tag_ids = + socket.assigns.selected_tag_ids + |> MapSet.intersection(allowed_tag_ids) + |> MapSet.to_list() company = socket.assigns.current_companyAlso applies to: 180-190
🤖 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/classify.ex` around lines 106 - 115, In handle_event("toggle_tag", %{"tag-id" => tag_id}, socket) you must whitelist the incoming tag_id against the server-side list of allowed tags (e.g. socket.assigns[:all_tags] or the assign that holds company-scoped tags) before mutating socket.assigns.selected_tag_ids; reject or ignore any tag_id not present in that list. Do the same validation in the other handler/save path referenced (save_classification/1) before passing selected_tag_ids to Invoices.set_invoice_tags/2 so only permitted tag IDs are persisted. Ensure you reference the existing symbols handle_event("toggle_tag"), selected_tag_ids, save_classification/1 and Invoices.set_invoice_tags/2 when adding the whitelist check.test/ksef_hub/emoji_generator/client_test.exs (1)
9-145:⚠️ Potential issue | 🟠 MajorReplace these
afterblocks withon_exit/1ortry ... after.
test ... do ... after ... endis not ExUnit cleanup syntax. The:anthropic_api_keymutation can leak into later cases and make this suite order-dependent.🧹 Suggested cleanup pattern
+ setup do + on_exit(fn -> + Application.delete_env(:ksef_hub, :anthropic_api_key) + end) + + :ok + end + describe "generate_emoji/1" do test "returns error when API key is not configured" do Application.put_env(:ksef_hub, :anthropic_api_key, nil) assert {:error, :missing_api_key} = Client.generate_emoji(`@context`) - after - Application.delete_env(:ksef_hub, :anthropic_api_key) endDoes ExUnit's `test` macro support an `after` block, or should cleanup use `on_exit/1` or `try ... after` inside the test?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/emoji_generator/client_test.exs` around lines 9 - 145, The tests currently use invalid `after` blocks (e.g., in tests that call Client.generate_emoji and set Application.put_env(:ksef_hub, :anthropic_api_key, ...)), which can leak environment state; replace each `after` block with either an on_exit(fn -> Application.delete_env(:ksef_hub, :anthropic_api_key) end) placed immediately after setting the env in the test, or wrap the test body in try do ... after Application.delete_env(:ksef_hub, :anthropic_api_key) end; update every test that mutates :anthropic_api_key (including those stubbing Req.Test for KsefHub.EmojiGenerator.Client and the context-prompt tests) to use one of these cleanup patterns so the env is always cleared even on failures.lib/ksef_hub_web/live/invoice_live/show.ex (1)
735-743:⚠️ Potential issue | 🟡 MinorRender the classify navigation as a single interactive control.
<.link>wrapping<.button>nests two interactive elements. That's invalid HTML and can break focus and click behavior.♿ Single-control version
- <.link - :if={`@can_set_category` || `@can_set_tags`} - navigate={~p"/c/#{`@current_company.id`}/invoices/#{`@invoice.id`}/classify"} - data-testid="edit-classification" - > - <.button size="sm" variant="outline"> - <.icon name="hero-pencil-square" class="size-4" /> Edit - </.button> - </.link> + <.button + :if={`@can_set_category` || `@can_set_tags`} + size="sm" + variant="outline" + navigate={~p"/c/#{`@current_company.id`}/invoices/#{`@invoice.id`}/classify"} + data-testid="edit-classification" + > + <.icon name="hero-pencil-square" class="size-4" /> Edit + </.button>🤖 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 735 - 743, The code nests a <.button> inside a <.link> (in invoice_live/show.ex around the <.link ... data-testid="edit-classification">), which creates two interactive controls; instead, make the navigation element a single control by removing the nested <.button> and applying the button styling/attributes to the <.link> itself (or replace the <.link> with a <button> that performs the navigation via push_patch/push_navigate). Specifically, keep the <.link navigate={~p"..."} data-testid="edit-classification"> and move the <.icon> and "Edit" label directly inside it, and copy the size="sm" variant="outline" styling from the <.button> to the <.link> (or implement push_patch in the surrounding LiveView and use a single <button> with phx-click), ensuring only one interactive element remains.
🧹 Nitpick comments (4)
priv/repo/migrations/20260318090451_extend_categories.exs (1)
23-23: Consider adding an index for category listing order path.You already added uniqueness on
(company_id, identifier). Forlist_categories/1ordering bysort_order, identifier, a supporting composite index can reduce sorting cost as data grows.Possible addition
create unique_index(:categories, [:company_id, :identifier]) + create index(:categories, [:company_id, :sort_order, :identifier])Based 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/20260318090451_extend_categories.exs` at line 23, Add a composite non-unique index to support list_categories/1 ordering by sort_order and identifier: in the migration that currently creates unique_index(:categories, [:company_id, :identifier]) add a line creating index(:categories, [:company_id, :sort_order, :identifier]) (optionally with a descriptive name like :categories_company_sort_order_identifier_index) so queries ordering by sort_order, identifier filtered by company_id can use the index; ensure the index creation is in the same up/forward migration and will be removed in down/rollback if you maintain symmetric migration logic.test/ksef_hub_web/live/tag_live_test.exs (1)
25-31: Prefer stable selectors over raw HTML/text matches in these LiveView tests.A lot of the new assertions use
html =~ ...orelement("button", "Delete"), even though the templates already expose stable hooks likedata-testid="tag-name-..."inlib/ksef_hub_web/live/tag_live/index.ex:67-137and#tag-forminlib/ksef_hub_web/live/tag_live/form.ex:141-178. That makes the suite brittle to copy/style changes, and the delete test will start targeting the wrong row as soon as more than one tag is rendered.As per coding guidelines, "Never test against raw HTML - always use
element/2,has_element/2, and similar functions with selectors likeassert has_element?(view, "#my-form")" and "Always reference the key element IDs you added in the LiveView templates..."Also applies to: 33-59, 67-73, 80-81, 87-100, 112-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/tag_live_test.exs` around lines 25 - 31, Replace fragile raw HTML/text assertions in the tag_live_test (e.g., lines testing "Tags", "New Tag", "Expense", "Income" and other occurrences) with stable selector-based assertions: use has_element?/2 or element/2 with the template IDs and test hooks exposed in the LiveViews (for example use has_element?(view, "#tag-form") to assert the form is present and has_element?(view, "[data-testid=\"tag-name-<id>\"]") or a more generic data-testid selector to assert tag rows, and use element(view, "[data-testid=\"delete-tag-<id>\"]") for delete buttons rather than matching "Delete" text). Update all mentioned test blocks (including the delete-row test) to target those data-testid attributes and `#tag-form` so tests remain stable when markup or copy changes.test/ksef_hub_web/live/category_live_test.exs (1)
188-192:Process.sleepintroduces flakiness in async test.Using
Process.sleep(50)to wait for async results is fragile—timing may vary across CI environments. Consider:
- Ensuring the mock returns synchronously so no sleep is needed
- Using
assert_receiveif an async message is expected- Increasing timeout if sleep is truly necessary
- # Wait for the async task result to be delivered - Process.sleep(50) html = render(view) + # If the mock returns synchronously, no sleep needed. + # Otherwise, consider using render_async(view) or assert_receive. assert html =~ "Failed to generate emoji"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/category_live_test.exs` around lines 188 - 192, Replace the brittle Process.sleep(50) by making the async result deterministic: either have the mocked emoji generation return synchronously in this test or wait for the completion message instead of sleeping—remove Process.sleep(50), then use assert_receive with the expected message from the async task (or assert_receive {:emoji_failed, _} with a timeout) before calling html = render(view) and asserting html =~ "Failed to generate emoji"; alternatively, adjust the mock used in this test to perform the failure synchronously so render(view) immediately contains the failure text.lib/ksef_hub/emoji_generator/client.ex (1)
85-92: Emoji regex may miss certain sequences.The regex
\p{So}matches "Symbol, Other" Unicode category. This covers most standard emojis but may miss:
- Keycap sequences (e.g.,
1️⃣)- Regional indicator flags (e.g.,
🇺🇸)- Some skin-tone modified emojis
For Claude's emoji output (typically standard single emojis), this is likely sufficient. Consider expanding if edge cases arise in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/emoji_generator/client.ex` around lines 85 - 92, The current extract_first_emoji/1 uses a Regex anchored on \p{So} which can miss keycap sequences, regional indicator flags and some modifier sequences; update the regex used in extract_first_emoji to match the full set of emoji sequences (use Unicode emoji properties and sequence pieces) — e.g. switch to a pattern that leverages \p{Emoji} (or \p{Emoji_Presentation}) plus optional emoji modifiers (\p{Emoji_Modifier}), optional variation selector \x{FE0F}, ZWJ sequences (200D) and regional indicator pairs, and explicit keycap sequences (digit + FE0F? + 20E3) so Regex.run in extract_first_emoji captures compound/ZWJ, modifier and flag/keycap emojis rather than only \p{So}.
🤖 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/category_live/form.ex`:
- Around line 11-23: The mount/3 and handle_params/3 public callbacks in this
module have `@spec` but no `@doc`; add concise `@doc` strings above each public
function (mount/3 and handle_params/3) describing purpose, params, and returned
socket tuple, matching repo style; do the same for the other public functions in
this module that currently lack `@doc` (add a one- or two-line description for
each public callback/function, keeping text consistent with existing module
docs).
In `@lib/ksef_hub_web/live/tag_live/form.ex`:
- Around line 93-98: The redirect after successful create/update and the Cancel
handler must preserve the current tag type query param so the UI stays on the
same tab; update the push_navigate targets in the create path (the case handling
Invoices.create_tag), the update path (Invoices.update_tag), and the Cancel
event to append "?type=#{socket.assigns.active_type}" (or use
socket.assigns[:active_type]) to the
~p"/c/#{socket.assigns.current_company.id}/tags" path so the active_type tab is
retained. Ensure you update the push_navigate calls in the form component (the
handlers that call Invoices.create_tag and Invoices.update_tag and the cancel
handler) to include that query param.
In `@lib/ksef_hub/invoice_classifier.ex`:
- Around line 104-108: The code is always assigning matching_category and
matching_tag even when confidence flags are false; change the map construction
(around build_prediction_attrs, matching_category, matching_tag and
confident_category?/confident_tag?) so that category: is set to
matching_category only when confident_category? is true (otherwise nil) and tag:
is set to matching_tag only when confident_tag? is true (otherwise nil); keep
build_prediction_attrs unchanged but ensure downstream cat/tag fields are
conditional on the corresponding confident_* boolean to prevent low-confidence
auto-apply.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 106-115: In handle_event("toggle_tag", %{"tag-id" => tag_id},
socket) you must whitelist the incoming tag_id against the server-side list of
allowed tags (e.g. socket.assigns[:all_tags] or the assign that holds
company-scoped tags) before mutating socket.assigns.selected_tag_ids; reject or
ignore any tag_id not present in that list. Do the same validation in the other
handler/save path referenced (save_classification/1) before passing
selected_tag_ids to Invoices.set_invoice_tags/2 so only permitted tag IDs are
persisted. Ensure you reference the existing symbols handle_event("toggle_tag"),
selected_tag_ids, save_classification/1 and Invoices.set_invoice_tags/2 when
adding the whitelist check.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 735-743: The code nests a <.button> inside a <.link> (in
invoice_live/show.ex around the <.link ... data-testid="edit-classification">),
which creates two interactive controls; instead, make the navigation element a
single control by removing the nested <.button> and applying the button
styling/attributes to the <.link> itself (or replace the <.link> with a <button>
that performs the navigation via push_patch/push_navigate). Specifically, keep
the <.link navigate={~p"..."} data-testid="edit-classification"> and move the
<.icon> and "Edit" label directly inside it, and copy the size="sm"
variant="outline" styling from the <.button> to the <.link> (or implement
push_patch in the surrounding LiveView and use a single <button> with
phx-click), ensuring only one interactive element remains.
In `@test/ksef_hub/emoji_generator/client_test.exs`:
- Around line 9-145: The tests currently use invalid `after` blocks (e.g., in
tests that call Client.generate_emoji and set Application.put_env(:ksef_hub,
:anthropic_api_key, ...)), which can leak environment state; replace each
`after` block with either an on_exit(fn -> Application.delete_env(:ksef_hub,
:anthropic_api_key) end) placed immediately after setting the env in the test,
or wrap the test body in try do ... after Application.delete_env(:ksef_hub,
:anthropic_api_key) end; update every test that mutates :anthropic_api_key
(including those stubbing Req.Test for KsefHub.EmojiGenerator.Client and the
context-prompt tests) to use one of these cleanup patterns so the env is always
cleared even on failures.
---
Nitpick comments:
In `@lib/ksef_hub/emoji_generator/client.ex`:
- Around line 85-92: The current extract_first_emoji/1 uses a Regex anchored on
\p{So} which can miss keycap sequences, regional indicator flags and some
modifier sequences; update the regex used in extract_first_emoji to match the
full set of emoji sequences (use Unicode emoji properties and sequence pieces) —
e.g. switch to a pattern that leverages \p{Emoji} (or \p{Emoji_Presentation})
plus optional emoji modifiers (\p{Emoji_Modifier}), optional variation selector
\x{FE0F}, ZWJ sequences (200D) and regional indicator pairs, and explicit keycap
sequences (digit + FE0F? + 20E3) so Regex.run in extract_first_emoji captures
compound/ZWJ, modifier and flag/keycap emojis rather than only \p{So}.
In `@priv/repo/migrations/20260318090451_extend_categories.exs`:
- Line 23: Add a composite non-unique index to support list_categories/1
ordering by sort_order and identifier: in the migration that currently creates
unique_index(:categories, [:company_id, :identifier]) add a line creating
index(:categories, [:company_id, :sort_order, :identifier]) (optionally with a
descriptive name like :categories_company_sort_order_identifier_index) so
queries ordering by sort_order, identifier filtered by company_id can use the
index; ensure the index creation is in the same up/forward migration and will be
removed in down/rollback if you maintain symmetric migration logic.
In `@test/ksef_hub_web/live/category_live_test.exs`:
- Around line 188-192: Replace the brittle Process.sleep(50) by making the async
result deterministic: either have the mocked emoji generation return
synchronously in this test or wait for the completion message instead of
sleeping—remove Process.sleep(50), then use assert_receive with the expected
message from the async task (or assert_receive {:emoji_failed, _} with a
timeout) before calling html = render(view) and asserting html =~ "Failed to
generate emoji"; alternatively, adjust the mock used in this test to perform the
failure synchronously so render(view) immediately contains the failure text.
In `@test/ksef_hub_web/live/tag_live_test.exs`:
- Around line 25-31: Replace fragile raw HTML/text assertions in the
tag_live_test (e.g., lines testing "Tags", "New Tag", "Expense", "Income" and
other occurrences) with stable selector-based assertions: use has_element?/2 or
element/2 with the template IDs and test hooks exposed in the LiveViews (for
example use has_element?(view, "#tag-form") to assert the form is present and
has_element?(view, "[data-testid=\"tag-name-<id>\"]") or a more generic
data-testid selector to assert tag rows, and use element(view,
"[data-testid=\"delete-tag-<id>\"]") for delete buttons rather than matching
"Delete" text). Update all mentioned test blocks (including the delete-row test)
to target those data-testid attributes and `#tag-form` so tests remain stable when
markup or copy changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3fc5569-739b-4974-be1b-841e40e882a0
📒 Files selected for processing (45)
.env.exampleCLAUDE.mdcloud-run/service.yamlconfig/config.exsconfig/runtime.exsconfig/test.exsdocs/sidecar-services.mdlib/ksef_hub/emoji_generator.exlib/ksef_hub/emoji_generator/behaviour.exlib/ksef_hub/emoji_generator/client.exlib/ksef_hub/exports/csv_builder.exlib/ksef_hub/invoice_classifier.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/category.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/controllers/api/category_controller.exlib/ksef_hub_web/json_helpers.exlib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/category_live/form.exlib/ksef_hub_web/live/category_live/index.exlib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.exlib/ksef_hub_web/live/tag_live/form.exlib/ksef_hub_web/live/tag_live/index.exlib/ksef_hub_web/router.exlib/ksef_hub_web/schemas/category.exlib/ksef_hub_web/schemas/create_category_request.exlib/ksef_hub_web/schemas/update_category_request.exlib/ksef_hub_web/schemas/update_tag_request.expriv/repo/migrations/20260318090451_extend_categories.exstest/ksef_hub/emoji_generator/client_test.exstest/ksef_hub/emoji_generator_test.exstest/ksef_hub/invoice_classifier_test.exstest/ksef_hub/invoices/categories_test.exstest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/live/category_live_test.exstest/ksef_hub_web/live/invoice_live/classify_test.exstest/ksef_hub_web/live/invoice_live/index_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/tag_live_test.exstest/support/factory.extest/test_helper.exs
💤 Files with no reviewable changes (3)
- lib/ksef_hub_web/live/category_live.ex
- lib/ksef_hub_web/live/tag_live.ex
- cloud-run/service.yaml
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/sidecar-services.md
- test/test_helper.exs
- lib/ksef_hub_web/json_helpers.ex
- lib/ksef_hub_web/live/invoice_live/index.ex
- lib/ksef_hub_web/components/invoice_components.ex
- test/ksef_hub/emoji_generator_test.exs
- lib/ksef_hub/emoji_generator/behaviour.ex
- config/config.exs
- lib/ksef_hub_web/schemas/create_category_request.ex
- config/test.exs
- lib/ksef_hub/invoices/category.ex
- test/support/factory.ex
- test/ksef_hub/invoice_classifier_test.exs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ksef_hub_web/live/invoice_live/classify.ex (1)
106-115:⚠️ Potential issue | 🟠 MajorThe earlier tag-id whitelist gap is still open.
"toggle_tag"still trusts raw"tag-id"from the client, andsave_classification/1forwards the accumulated set straight intoInvoices.set_invoice_tags/2. A forged event can keep ids that never came from the company-scoped@all_tags. Whitelist againstsocket.assigns.all_tagsbefore mutatingselected_tag_idsand again before persisting.Based on learnings: Applies to **/*.live.ex : Validate all state changes in LiveView events.
Also applies to: 183-189
🤖 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/classify.ex` around lines 106 - 115, The "toggle_tag" event handler currently trusts the raw "tag-id" from the client; validate the incoming tag_id against the server-side whitelist in socket.assigns.all_tags before mutating socket.assigns.selected_tag_ids in handle_event("toggle_tag") (only allow toggle if tag_id exists in all_tags), and also enforce the same whitelist in save_classification/1 before passing tags to Invoices.set_invoice_tags/2 (filter selected_tag_ids to include only ids present in socket.assigns.all_tags) so forged events cannot add out-of-scope tag ids.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/classify.ex (1)
83-85: Complete the public callback docs/spec coverage.
mount/3is documented/spec'ed, buthandle_event/3has no@doc, andrender/1is missing both@docand@spec. The repo rules forlib/**/*.exrequire those callback declarations too.As per coding guidelines:
lib/**/*.ex: Every public function must have@docdocumentation in Elixir. Every function (public and private) must have@spectype specification in Elixir.Also applies to: 212-213
🤖 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/classify.ex` around lines 83 - 85, Add missing public docs and specs: add an `@doc` for handle_event/3 describing accepted event names and payload shape and an `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}; add both `@doc` and `@spec` for render/1 (e.g., `@spec` render(Phoenix.LiveView.Socket.t() | map()) :: Phoenix.HTML.Rendered.t() or the appropriate return type used in this module); also ensure any other public callbacks around lines mentioned (e.g., the functions at 212-213) have `@doc` and all functions (public and private) include `@specs` per project rules.
🤖 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/classify.ex`:
- Around line 121-123: The authorization check is too broad: inside handle_event
"create_tag" you call create_tag and later save_classification/1 while only one
permission may be intended; update the save path so save_classification/1 only
persists category changes when socket.assigns.can_set_category is true and only
persists tag changes when socket.assigns.can_set_tags is true. Constrain
create_tag/2 so it does not auto-select or persist tags unless can_set_tags is
true, and similarly guard any category setter calls (e.g., in
save_classification/1 and related handlers) with
socket.assigns.can_set_category; apply the same per-permission checks for the
other affected handlers mentioned (lines ~129-131, ~157-166, ~186-189) to ensure
persistence decisions are made at the persistence boundary, not based on earlier
role bits.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 106-115: The "toggle_tag" event handler currently trusts the raw
"tag-id" from the client; validate the incoming tag_id against the server-side
whitelist in socket.assigns.all_tags before mutating
socket.assigns.selected_tag_ids in handle_event("toggle_tag") (only allow toggle
if tag_id exists in all_tags), and also enforce the same whitelist in
save_classification/1 before passing tags to Invoices.set_invoice_tags/2 (filter
selected_tag_ids to include only ids present in socket.assigns.all_tags) so
forged events cannot add out-of-scope tag ids.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 83-85: Add missing public docs and specs: add an `@doc` for
handle_event/3 describing accepted event names and payload shape and an `@spec`
handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply,
Phoenix.LiveView.Socket.t()}; add both `@doc` and `@spec` for render/1 (e.g., `@spec`
render(Phoenix.LiveView.Socket.t() | map()) :: Phoenix.HTML.Rendered.t() or the
appropriate return type used in this module); also ensure any other public
callbacks around lines mentioned (e.g., the functions at 212-213) have `@doc` and
all functions (public and private) include `@specs` per project rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5a412d7-0059-4849-8cf5-187c3c76afbd
📒 Files selected for processing (1)
lib/ksef_hub_web/live/invoice_live/classify.ex
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
test/ksef_hub/emoji_generator/client_test.exs (1)
86-98: Consider using the updated conn fromread_body/1.After
Plug.Conn.read_body/1, the returned conn should typically be used for subsequent operations. Here, the updated conn is ignored (_conn) and the originalconnis passed toReq.Test.json/2. This may work fine in the Req.Test stub context since it manages the response lifecycle, but using the returned conn would be more idiomatic Plug usage.♻️ Proposed refactor
Req.Test.stub(KsefHub.EmojiGenerator.Client, fn conn -> - {:ok, body, _conn} = Plug.Conn.read_body(conn) + {:ok, body, conn} = Plug.Conn.read_body(conn) decoded = Jason.decode!(body) [%{"content" => prompt}] = decoded["messages"] assert prompt =~ "Identifier: finance:invoices" assert prompt =~ "Name: Invoices" assert prompt =~ "Description: Invoice processing" assert prompt =~ "Examples: Monthly recurring" Req.Test.json(conn, %{ "content" => [%{"type" => "text", "text" => "💰"}] }) end)The same pattern appears in lines 114-127.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/emoji_generator/client_test.exs` around lines 86 - 98, The stub reads the request body but ignores the updated conn returned by Plug.Conn.read_body/1 (bound as _conn) and then calls Req.Test.json/2 with the old conn; update the code to capture and use the returned conn (e.g., change _conn to new_conn and pass new_conn to Req.Test.json/2) so subsequent operations use the updated connection; apply the same fix for the second stub instance around the later block (the other Plug.Conn.read_body/1 and Req.Test.json/2 pair).test/ksef_hub/invoice_classifier_test.exs (1)
370-378: Minor:@specreturn type doesn't match actual return.The
@specdeclares:: :ok, butMox.expect/3chains return the mock module. Since callers don't use the return value, this is cosmetic, but for accuracy:📝 Suggested fix
- `@spec` expect_predictions(keyword()) :: :ok + `@spec` expect_predictions(keyword()) :: module()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoice_classifier_test.exs` around lines 370 - 378, The `@spec` for expect_predictions is incorrect (declares :: :ok) because the Mox.expect call chain returns the mock module rather than :ok; update the spec on expect_predictions to reflect the actual return (for example :: module() or :: any()), or remove the `@spec` entirely; locate the expect_predictions function and adjust its `@spec` accordingly (referencing KsefHub.InvoiceClassifier.Mock and the expect_predictions/1 function).lib/ksef_hub_web/live/invoice_live/classify.ex (1)
193-199: Consider scoping persistence calls to the user's actual permissions.While the UI events (
select_category,toggle_tag) are properly guarded,save_classification/1unconditionally calls bothset_invoice_categoryandset_invoice_tags. A user with onlycan_set_tagsstill triggers the category setter (though with the unchanged value).For defense-in-depth, scope each persistence call to its corresponding permission:
🛡️ Suggested approach
result = Invoices.with_manual_prediction(invoice, fn -> - with {:ok, updated} <- Invoices.set_invoice_category(invoice, category_id), - {:ok, _tags} <- Invoices.set_invoice_tags(updated.id, tag_ids) do - {:ok, updated} + with {:ok, updated} <- maybe_set_category(socket, invoice, category_id), + {:ok, _tags} <- maybe_set_tags(socket, updated, tag_ids) do + {:ok, Invoices.get_invoice!(company.id, updated.id)} end end) + + defp maybe_set_category(socket, invoice, category_id) do + if socket.assigns.can_set_category, + do: Invoices.set_invoice_category(invoice, category_id), + else: {:ok, invoice} + end + + defp maybe_set_tags(socket, invoice, tag_ids) do + if socket.assigns.can_set_tags, + do: Invoices.set_invoice_tags(invoice.id, tag_ids), + else: {:ok, []} + endBased on learnings: Applies to **/{*controller.ex,*live.ex} : Check authorization in controllers and LiveViews.
🤖 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/classify.ex` around lines 193 - 199, The save_classification/1 flow currently calls Invoices.set_invoice_category and Invoices.set_invoice_tags unconditionally inside Invoices.with_manual_prediction; change it so each persistence call is executed only when the current user has the corresponding permission: call Invoices.set_invoice_category(invoice, category_id) only when the user can set categories, and call Invoices.set_invoice_tags(updated.id, tag_ids) only when the user can_set_tags; preserve the surrounding Invoices.with_manual_prediction usage and ensure the final {:ok, updated} return path still returns the up-to-date invoice after whichever allowed updates ran.lib/ksef_hub_web/live/invoice_live/show.ex (1)
988-991: Consider browser support forfield-sizing: content.This CSS property is relatively new (available in Chrome 123+, Firefox 123+, Safari 17.4+). The
oninputJavaScript fallback provides good progressive enhancement, but thefield-sizing: contentdeclaration will be ignored in older browsers, leaving the textarea at a fixed height until user input triggers the JS resize.This is acceptable for most use cases—just noting for awareness.
Also applies to: 1018-1022
🤖 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 988 - 991, The inline textarea style uses the new CSS property "field-sizing: content" which older browsers ignore; update the markup around the textarea (the element with the inline style and the oninput handler) to provide a safe fallback by adding an initial height behavior (e.g., height: auto and a sensible min-height) or a CSS class that applies those fallback rules, keep the existing oninput auto-resize JS as-is, and apply the same change to the other textarea instance noted in the diff so non-supporting browsers don't render a fixed-height textarea before user input.lib/ksef_hub_web/live/tag_live/form.ex (4)
140-142: Missing@docfor public callbackrender/1.`@impl` true + `@doc` """ + Renders the tag form. + """ `@spec` render(map()) :: Phoenix.LiveView.Rendered.t()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 140 - 142, Add a short `@doc` above the public callback function render/1 explaining its purpose and contract (what assigns is and what the function returns), e.g., a one- or two-line description mentioning it renders the LiveView form given assigns and returns a Phoenix.LiveView.Rendered struct; place the `@doc` directly above the `@impl` true and def render(assigns) to document render/1 for tools and Dialyzer.
64-80: Missing@docfor public callbackhandle_event/3.Add documentation for the event handlers.
`@impl` true + `@doc` """ + Handles form validation and save events. + """ `@spec` handle_event(String.t(), map(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 64 - 80, Add a `@doc` string above the public callback handle_event/3 describing the supported events ("validate" and "save"), the expected params shape (map with "tag" key), and the return shape ({:noreply, Phoenix.LiveView.Socket.t()}), placing the doc immediately above the `@impl` true for handle_event/3 so both pattern clauses (the "validate" clause that builds a changeset and assigns form, and the "save" clause that delegates to create_tag/2 or update_tag/3) are covered.
10-14: Missing@docfor public callbackmount/3.Per coding guidelines, every public function must have
@docdocumentation. Add a brief description of the mount callback behavior.`@impl` true + `@doc` """ + Initializes the LiveView socket. + """ `@spec` mount(map(), map(), Phoenix.LiveView.Socket.t()) :: {:ok, Phoenix.LiveView.Socket.t()} def mount(_params, _session, socket) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 10 - 14, The public LiveView callback mount/3 (def mount(_params, _session, socket) in TagLive.Form) is missing `@doc`; add a short `@doc` string immediately above mount/3 that states it initializes the socket for the form LiveView, describes the parameters (_params, _session, socket) and documents the returned value {:ok, socket} (or any socket assigns initialized), e.g., a one- or two-sentence description of what state/assigns are set by mount/3 for this LiveView.
16-21: Missing@docfor public callbackhandle_params/3.`@impl` true + `@doc` """ + Handles route parameters and applies the appropriate action (`:new` or `:edit`). + """ `@spec` handle_params(map(), String.t(), Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 16 - 21, Add a descriptive `@doc` for the public LiveView callback handle_params/3 that explains its purpose, expected parameters (params map, _uri string, Phoenix.LiveView.Socket), and the return value {:noreply, Phoenix.LiveView.Socket.t()}; update the module containing handle_params/3 (which calls apply_action(socket, socket.assigns.live_action, params)) to include this `@doc` immediately above the `@spec/`@impl so the public API is documented for callers and generated docs.
🤖 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/category_live/form.ex`:
- Around line 69-72: When rebuilding the changeset after the async emoji result
(the same place changeset_for(socket, params) is used and where assign(socket,
form: to_form(...)) is called), preserve the existing validation state by
ensuring the new changeset keeps action: :validate instead of dropping it;
update the code that creates the fresh changeset (and the similar block around
lines 118-129) to set the changeset's action to :validate (e.g., set
Map.put(changeset, :action, :validate) or build it via %{changeset_for(...) |
action: :validate}) before passing it to to_form and assign.
- Around line 197-205: current_form_params/1 is converting the sort_order field
into a string and later atomize_params/1 normalizes blanks/invalids to 0, which
silently changes nil/invalid values; instead, preserve the raw or nil value for
:sort_order so Category.changeset/2 can perform casting/validation. Update
current_form_params/1 (and the similar block at lines 208-223) to special-case
the :sort_order field: do not coerce blank or non-numeric values to "0" or "0"
string—pass through the original form value (or nil) for
Atom.to_string("sort_order") and let atomize_params/1 or Category.changeset/2
handle validation. Ensure you reference the fields list (~w(identifier name
emoji description examples sort_order)a) and adjust only handling of the
:sort_order mapping so other fields remain stringified as before.
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 189-191: The three expressions (allowed_tag_ids, tag_ids, company)
are formatted on a long line that fails mix format; split the long pipeline into
multiple shorter lines so each assignment is within line length limits.
Specifically, rewrite the computation for allowed_tag_ids using MapSet.new/2
with a short anonymous fun, then compute tag_ids by piping
socket.assigns.selected_tag_ids into MapSet.intersection(allowed_tag_ids) and
then into MapSet.to_list() on its own line(s), and leave company =
socket.assigns.current_company on its own line; update the assignments for
allowed_tag_ids and tag_ids (referencing allowed_tag_ids, tag_ids,
socket.assigns.selected_tag_ids, MapSet.intersection, MapSet.to_list) to ensure
each line is under the formatter width.
In `@lib/ksef_hub_web/live/tag_live/form.ex`:
- Around line 93-102: The case block handling Invoices.create_tag has formatting
issues; run mix format or reformat the case expression so the pipeline operators
are aligned and the |> calls on the success branch are on their own lines (the
branch that calls put_flash and push_navigate) and the error branch assigns the
form via assign(socket, form: to_form(changeset, as: :tag)); specifically
reformat the block around the Invoices.create_tag call and the {:ok, _tag} /
{:error, %Ecto.Changeset{} = changeset} branches to match mix formatter rules.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 193-199: The save_classification/1 flow currently calls
Invoices.set_invoice_category and Invoices.set_invoice_tags unconditionally
inside Invoices.with_manual_prediction; change it so each persistence call is
executed only when the current user has the corresponding permission: call
Invoices.set_invoice_category(invoice, category_id) only when the user can set
categories, and call Invoices.set_invoice_tags(updated.id, tag_ids) only when
the user can_set_tags; preserve the surrounding Invoices.with_manual_prediction
usage and ensure the final {:ok, updated} return path still returns the
up-to-date invoice after whichever allowed updates ran.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 988-991: The inline textarea style uses the new CSS property
"field-sizing: content" which older browsers ignore; update the markup around
the textarea (the element with the inline style and the oninput handler) to
provide a safe fallback by adding an initial height behavior (e.g., height: auto
and a sensible min-height) or a CSS class that applies those fallback rules,
keep the existing oninput auto-resize JS as-is, and apply the same change to the
other textarea instance noted in the diff so non-supporting browsers don't
render a fixed-height textarea before user input.
In `@lib/ksef_hub_web/live/tag_live/form.ex`:
- Around line 140-142: Add a short `@doc` above the public callback function
render/1 explaining its purpose and contract (what assigns is and what the
function returns), e.g., a one- or two-line description mentioning it renders
the LiveView form given assigns and returns a Phoenix.LiveView.Rendered struct;
place the `@doc` directly above the `@impl` true and def render(assigns) to document
render/1 for tools and Dialyzer.
- Around line 64-80: Add a `@doc` string above the public callback handle_event/3
describing the supported events ("validate" and "save"), the expected params
shape (map with "tag" key), and the return shape ({:noreply,
Phoenix.LiveView.Socket.t()}), placing the doc immediately above the `@impl` true
for handle_event/3 so both pattern clauses (the "validate" clause that builds a
changeset and assigns form, and the "save" clause that delegates to create_tag/2
or update_tag/3) are covered.
- Around line 10-14: The public LiveView callback mount/3 (def mount(_params,
_session, socket) in TagLive.Form) is missing `@doc`; add a short `@doc` string
immediately above mount/3 that states it initializes the socket for the form
LiveView, describes the parameters (_params, _session, socket) and documents the
returned value {:ok, socket} (or any socket assigns initialized), e.g., a one-
or two-sentence description of what state/assigns are set by mount/3 for this
LiveView.
- Around line 16-21: Add a descriptive `@doc` for the public LiveView callback
handle_params/3 that explains its purpose, expected parameters (params map, _uri
string, Phoenix.LiveView.Socket), and the return value {:noreply,
Phoenix.LiveView.Socket.t()}; update the module containing handle_params/3
(which calls apply_action(socket, socket.assigns.live_action, params)) to
include this `@doc` immediately above the `@spec/`@impl so the public API is
documented for callers and generated docs.
In `@test/ksef_hub/emoji_generator/client_test.exs`:
- Around line 86-98: The stub reads the request body but ignores the updated
conn returned by Plug.Conn.read_body/1 (bound as _conn) and then calls
Req.Test.json/2 with the old conn; update the code to capture and use the
returned conn (e.g., change _conn to new_conn and pass new_conn to
Req.Test.json/2) so subsequent operations use the updated connection; apply the
same fix for the second stub instance around the later block (the other
Plug.Conn.read_body/1 and Req.Test.json/2 pair).
In `@test/ksef_hub/invoice_classifier_test.exs`:
- Around line 370-378: The `@spec` for expect_predictions is incorrect (declares
:: :ok) because the Mox.expect call chain returns the mock module rather than
:ok; update the spec on expect_predictions to reflect the actual return (for
example :: module() or :: any()), or remove the `@spec` entirely; locate the
expect_predictions function and adjust its `@spec` accordingly (referencing
KsefHub.InvoiceClassifier.Mock and the expect_predictions/1 function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be8ab229-f99b-4a28-9af4-41ffdcf0f386
📒 Files selected for processing (8)
lib/ksef_hub/invoice_classifier.exlib/ksef_hub_web/live/category_live/form.exlib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live/form.extest/ksef_hub/emoji_generator/client_test.exstest/ksef_hub/invoice_classifier_test.exstest/ksef_hub_web/live/tag_live_test.exs
Comment on lines
+69
to
+72
| changeset = | ||
| %{changeset_for(socket, params) | action: :validate} | ||
|
|
||
| {:noreply, assign(socket, form: to_form(changeset, as: :category))} |
There was a problem hiding this comment.
Preserve the validation state when applying the generated emoji.
The "validate" path explicitly sets action: :validate, but the async success path rebuilds a fresh changeset and drops that action. As a result, any validation feedback already shown to the user disappears as soon as the emoji result lands.
💡 Suggested fix
- changeset = changeset_for(socket, params)
+ current_action = socket.assigns.form.source.action
+ changeset = %{changeset_for(socket, params) | action: current_action}Also applies to: 118-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub_web/live/category_live/form.ex` around lines 69 - 72, When
rebuilding the changeset after the async emoji result (the same place
changeset_for(socket, params) is used and where assign(socket, form:
to_form(...)) is called), preserve the existing validation state by ensuring the
new changeset keeps action: :validate instead of dropping it; update the code
that creates the fresh changeset (and the similar block around lines 118-129) to
set the changeset's action to :validate (e.g., set Map.put(changeset, :action,
:validate) or build it via %{changeset_for(...) | action: :validate}) before
passing it to to_form and assign.
Comment on lines
+197
to
+205
| @spec current_form_params(Phoenix.LiveView.Socket.t()) :: map() | ||
| defp current_form_params(socket) do | ||
| form = socket.assigns.form | ||
| fields = ~w(identifier name emoji description examples sort_order)a | ||
|
|
||
| Map.new(fields, fn field -> | ||
| value = form[field].value | ||
| {Atom.to_string(field), if(is_binary(value), do: value, else: to_string(value))} | ||
| end) |
There was a problem hiding this comment.
Don't coerce blank or invalid sort_order values to 0.
current_form_params/1 turns a missing sort order into "", and atomize_params/1 then normalizes every blank/invalid value to 0. That means editing a category with no sort order and saving it will silently rewrite nil to 0, and invalid input never reaches Category.changeset/2 as a cast error. Since lib/ksef_hub/invoices/category.ex already casts :sort_order, this helper should pass the raw value through (or preserve nil) instead of forcing a fallback.
💡 Suggested fix
defp atomize_params(params) do
- sort_order =
- case Integer.parse(params["sort_order"] || "0") do
- {n, ""} -> n
- _ -> 0
- end
-
%{
identifier: params["identifier"] || "",
name: blank_to_nil(params["name"]),
emoji: blank_to_nil(params["emoji"]),
description: blank_to_nil(params["description"]),
examples: blank_to_nil(params["examples"]),
- sort_order: sort_order
+ sort_order: blank_to_nil(params["sort_order"])
}
endAlso applies to: 208-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub_web/live/category_live/form.ex` around lines 197 - 205,
current_form_params/1 is converting the sort_order field into a string and later
atomize_params/1 normalizes blanks/invalids to 0, which silently changes
nil/invalid values; instead, preserve the raw or nil value for :sort_order so
Category.changeset/2 can perform casting/validation. Update
current_form_params/1 (and the similar block at lines 208-223) to special-case
the :sort_order field: do not coerce blank or non-numeric values to "0" or "0"
string—pass through the original form value (or nil) for
Atom.to_string("sort_order") and let atomize_params/1 or Category.changeset/2
handle validation. Ensure you reference the fields list (~w(identifier name
emoji description examples sort_order)a) and adjust only handling of the
:sort_order mapping so other fields remain stringified as before.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/ksef_hub_web/live/tag_live/form.ex (1)
39-59:⚠️ Potential issue | 🟡 MinorPreserve tag type on the not-found redirect path.
When edit fails with
:not_found, navigation drops?type=..., so users can be sent back to the wrong tab.↩️ Suggested fix
-defp apply_action(socket, :edit, %{"id" => id}) do +defp apply_action(socket, :edit, %{"id" => id} = params) do company_id = socket.assigns.current_company.id + requested_type = parse_tag_type(params["type"]) case Invoices.get_tag(company_id, id) do {:ok, tag} -> changeset = Tag.changeset(tag, %{}) socket |> assign( page_title: "Edit Tag", tag: tag, company_id: company_id, tag_type: tag.type ) |> assign(form: to_form(changeset, as: :tag)) {:error, :not_found} -> socket |> put_flash(:error, "Tag not found.") - |> push_navigate(to: ~p"/c/#{company_id}/tags") + |> push_navigate(to: ~p"/c/#{company_id}/tags?type=#{requested_type}") end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/tag_live/form.ex` around lines 39 - 59, When handling the :not_found branch in apply_action/3 for :edit, include the current tag type from socket assigns in the redirect so the UI stays on the same tab; change the push_navigate call that currently uses ~p"/c/#{company_id}/tags" to include the type query param (e.g. use socket.assigns.tag_type or a fallback and build the path like ~p"/c/#{company_id}/tags?type=#{tag_type}") so the ?type=... is preserved when navigating away on error.lib/ksef_hub_web/live/invoice_live/classify.ex (1)
197-203:⚠️ Potential issue | 🟠 MajorEnforce individual permissions when persisting classification.
The save handler allows entry if either
can_set_categoryorcan_set_tagsis true, butsave_classification/1unconditionally executes both setters. A user with only tag permissions can modify the category, and vice versa.🔒 Proposed fix to respect individual permissions
defp save_classification(socket) do invoice = socket.assigns.invoice - category_id = socket.assigns.selected_category_id allowed_tag_ids = MapSet.new(socket.assigns.all_tags, & &1.id) tag_ids = - socket.assigns.selected_tag_ids |> MapSet.intersection(allowed_tag_ids) |> MapSet.to_list() + socket.assigns.selected_tag_ids + |> MapSet.intersection(allowed_tag_ids) + |> MapSet.to_list() company = socket.assigns.current_company + can_set_category = socket.assigns.can_set_category + can_set_tags = socket.assigns.can_set_tags result = Invoices.with_manual_prediction(invoice, fn -> - with {:ok, updated} <- Invoices.set_invoice_category(invoice, category_id), - {:ok, _tags} <- Invoices.set_invoice_tags(updated.id, tag_ids) do - {:ok, updated} + with {:ok, updated} <- maybe_set_category(invoice, socket.assigns.selected_category_id, can_set_category), + {:ok, _tags} <- maybe_set_tags(updated.id, tag_ids, can_set_tags) do + {:ok, updated} end end)Add helper functions:
`@spec` maybe_set_category(Invoice.t(), String.t() | nil, boolean()) :: {:ok, Invoice.t()} | {:error, term()} defp maybe_set_category(invoice, _category_id, false), do: {:ok, invoice} defp maybe_set_category(invoice, category_id, true), do: Invoices.set_invoice_category(invoice, category_id) `@spec` maybe_set_tags(String.t(), [String.t()], boolean()) :: {:ok, [Tag.t()]} | {:error, term()} defp maybe_set_tags(_invoice_id, _tag_ids, false), do: {:ok, []} defp maybe_set_tags(invoice_id, tag_ids, true), do: Invoices.set_invoice_tags(invoice_id, tag_ids)Based on learnings: Applies to **/{*controller.ex,*live.ex} : Check authorization in controllers and LiveViews.
🤖 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/classify.ex` around lines 197 - 203, The current save path inside the Invoices.with_manual_prediction block unconditionally calls Invoices.set_invoice_category and Invoices.set_invoice_tags so a user with only one permission can change both; create helper functions maybe_set_category(invoice, category_id, can_set_category) and maybe_set_tags(invoice_id, tag_ids, can_set_tags) (as described) and replace the direct calls in the anonymous function passed to Invoices.with_manual_prediction (the code that currently does with {:ok, updated} <- Invoices.set_invoice_category(...), {:ok, _tags} <- Invoices.set_invoice_tags(...)) to instead call maybe_set_category and then maybe_set_tags, passing the current can_set_category and can_set_tags booleans so each setter runs only when that specific permission is true and otherwise returns {:ok, unchanged}.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/classify.ex (1)
192-193: Split the long pipeline for readability.The pipeline exceeds line length limits and should be broken into multiple lines per Elixir formatting conventions.
♻️ Proposed fix
tag_ids = - socket.assigns.selected_tag_ids |> MapSet.intersection(allowed_tag_ids) |> MapSet.to_list() + socket.assigns.selected_tag_ids + |> MapSet.intersection(allowed_tag_ids) + |> MapSet.to_list()As per coding guidelines: Break long pipes into multiple assignments for clarity.
🤖 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/classify.ex` around lines 192 - 193, The one-line pipeline assigning tag_ids is too long; split it into multiple steps for readability by first binding the selected set (e.g., selected_tags = socket.assigns.selected_tag_ids), then computing the intersection (e.g., intersected = MapSet.intersection(selected_tags, allowed_tag_ids)), and finally converting to a list (e.g., tag_ids = MapSet.to_list(intersected)); update references to use these intermediate variables and keep each function call on its own line.
🤖 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/tag_live/form.ex`:
- Around line 10-20: Add short `@doc` one-line annotations for each public
LiveView callback shown (mount/3, handle_params/3) and for the other public
callbacks mentioned (handle_event/3 and render/1) so every public function has
documentation; place a concise descriptive `@doc` string immediately above each
function definition (preserve existing `@impl` and `@spec` lines) summarizing the
function's purpose and inputs/behavior. Ensure the `@doc` strings are brief (one
sentence), cover mount/3, handle_params/3, handle_event/3, and render/1, and
likewise add `@doc` to the other public functions referenced around lines 64-80
and 144-146.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 197-203: The current save path inside the
Invoices.with_manual_prediction block unconditionally calls
Invoices.set_invoice_category and Invoices.set_invoice_tags so a user with only
one permission can change both; create helper functions
maybe_set_category(invoice, category_id, can_set_category) and
maybe_set_tags(invoice_id, tag_ids, can_set_tags) (as described) and replace the
direct calls in the anonymous function passed to Invoices.with_manual_prediction
(the code that currently does with {:ok, updated} <-
Invoices.set_invoice_category(...), {:ok, _tags} <-
Invoices.set_invoice_tags(...)) to instead call maybe_set_category and then
maybe_set_tags, passing the current can_set_category and can_set_tags booleans
so each setter runs only when that specific permission is true and otherwise
returns {:ok, unchanged}.
In `@lib/ksef_hub_web/live/tag_live/form.ex`:
- Around line 39-59: When handling the :not_found branch in apply_action/3 for
:edit, include the current tag type from socket assigns in the redirect so the
UI stays on the same tab; change the push_navigate call that currently uses
~p"/c/#{company_id}/tags" to include the type query param (e.g. use
socket.assigns.tag_type or a fallback and build the path like
~p"/c/#{company_id}/tags?type=#{tag_type}") so the ?type=... is preserved when
navigating away on error.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/classify.ex`:
- Around line 192-193: The one-line pipeline assigning tag_ids is too long;
split it into multiple steps for readability by first binding the selected set
(e.g., selected_tags = socket.assigns.selected_tag_ids), then computing the
intersection (e.g., intersected = MapSet.intersection(selected_tags,
allowed_tag_ids)), and finally converting to a list (e.g., tag_ids =
MapSet.to_list(intersected)); update references to use these intermediate
variables and keep each function call on its own line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5eaf3a4f-1761-4441-b2e7-f7307868b645
📒 Files selected for processing (2)
lib/ksef_hub_web/live/invoice_live/classify.exlib/ksef_hub_web/live/tag_live/form.ex
Previously, resolve_predictions only set category/tag when confidence was above threshold. Below-threshold predictions left category_id as nil even when a matching company category existed. Now the best prediction is always applied, with the threshold only determining prediction_status (:predicted vs :needs_review). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New /invoices/:id/classify page with collapsible category groups (grouped by prefix before ':'), tag checkboxes, inline tag creation, and atomic save via with_manual_prediction. The page tracks local state before persisting, allowing cancel without side effects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace inline category dropdown and tag checkboxes in the invoice detail page with read-only category_badge and tag_list components. Add Edit button linking to the new /classify page. Remove set_category, toggle_tag, and create_and_add_tag event handlers and related assigns (category_form, new_tag_form, tag_form_key, all_tags) from show.ex. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Matches the Details and Classification section button style for consistency across the invoice detail page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix conflicting text-sm/text-xs classes on Category and Tags labels. Use text-sm text-muted-foreground matching the Details table style. Normalize heading margin to mb-2 across all three sidebar cards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comments card was placed outside the grid, causing it to span the full page width. Move it into the sidebar column (space-y-4 div) alongside Details, Classification, and Note cards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 1bee847.
- Smaller avatar (w-6 h-6 instead of w-8 h-8) - Comment body aligned with sender name (nested inside same flex column) - Reduced vertical spacing between comments (space-y-2 instead of space-y-4) - Single-row textarea with resize-none for compact input - Smaller Post button matching input height Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use divide-y with py-2 instead of space-y-2 for tighter comment rows - Comment body left-aligned with sender name via ml-7 (matching avatar+gap) - Flat layout: name/time/actions on one line, body directly below - Smaller avatar (w-5 h-5) - Auto-growing textareas via field-sizing:content + oninput fallback - No empty space below single-line comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace <p> with <div> for comment body to eliminate browser default margins. Reduce row padding from py-2 to py-1.5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Slack-style compact layout: bold name + timestamp on one line, body text directly below at the same indent level. No avatars, no indentation offsets. Minimal spacing with space-y-0.5. Body in muted color to distinguish from the header. Plain icon buttons instead of button components for actions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename category `name` to `identifier` (ML model key, group:target format) - Add `name` (display name), `examples` fields to categories - Add Claude Haiku-powered emoji auto-generation via Anthropic API - Split CategoryLive and TagLive into Index + Form pages - Fix tag unique constraint name to match actual DB index - Update all references across classifier, UI, API, and exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix current_form_params losing sort_order=0 (falsy integer → empty string) - Add emoji_generator_req_options to test config (prevent real API calls) - Remove duplicated editing?/1 helper, inline @live_action == :edit - Add EmojiGenerator.Client unit tests (API responses, prompt building, emoji extraction, error handling) - Add CategoryLive.Form emoji generation tests (success, failure, crash) - Add migration docstring explaining schema evolution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…verage - Add server-side permission guards on classify save/create_tag events - Extract prediction_hint component into InvoiceComponents (was duplicated) - Fix cond fallback in prediction_hint for nil confidence edge case - Fix migration index drop to use explicit name after column rename - Fix emoji generator client test to use async: false (Application env mutation) - Add permission enforcement tests for classify page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After rebasing onto main (which merged feat/expense-categories-typed-tags), the refactored split LiveViews were missing typed tag support: - tag_live/index: add Expense/Income tabs with ?type= query param filtering - tag_live/form: pass tag type from query param, show type in header - category_live/index: restore "Expense Categories" title - classify: filter tags by invoice type, pass type on inline tag creation - tests: use type: :expense for invoices needing categories, fix async mox Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add missing `type` enum to UpdateTagRequest OpenAPI schema - Hide category section on classify page for income invoices - Handle :expense_only error explicitly in save_classification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ixes - Gate category/tag auto-apply on confidence threshold (was applying regardless of confidence when a matching record existed) - Preserve tag type query param across create/update/cancel redirects - Whitelist tag IDs against server-side allowed tags in classify page - Remove nested <.button> inside <.link> for accessibility compliance - Add @doc strings to CategoryLive.Form public callbacks - Standardize emoji generator test cleanup with shared setup/on_exit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace nested if/else with cond to fix Credo nesting depth warning. Auto-format long lines in classify.ex and tag form. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nnotations - Add maybe_set_category/3 and maybe_set_tags/3 helpers so each setter only runs when the user holds that specific permission - Preserve ?type= query param on tag not-found redirect - Add @doc to all public callbacks in TagLive.Form - Split tag_ids pipeline for readability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… Edit button - Remove "Add payment" button from header (intentionally kept in Payment Requests section only) - Add @data_editable guard to Details Edit button so KSeF invoices hide it - Add data-testid to Edit button for precise test targeting - Update tag redirect test to expect preserved ?type= query param Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
emilwojtaszek
force-pushed
the
feat/classification-ui-refactor
branch
from
46a46de to
b6ff96a
Compare
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
Docs & Config
Migrations & Tests