Feat/invoice tagging frontend#54
Merged
Merged
Conversation
…etails/3 - list_invoices_paginated now preloads [:category, :tags] on entries - Add non-raising get_invoice_with_details/3 with preloads - Add tests for preloads and the new function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add category_badge, tag_list, prediction_indicator components - Add Category, Tags, and Review indicator columns to index table - Add tests for new column rendering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use get_invoice_with_details to preload associations on mount - Load categories and all_tags into assigns for editing UI - Add Classification card with category select dropdown and tag checkboxes - Add set_category event handler (marks prediction as manual) - Add toggle_tag event handler for adding/removing tags - Add create_and_add_tag event handler for inline tag creation - Show prediction indicator in subtitle - Add tests for display, category editing, tag toggling, inline tag creation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create CategoryLive with stream-based table - Inline form for create/edit with validation - Delete with confirmation dialog - Company-scoped categories - Add /categories and /tags routes - Add tests for mount, create, edit, delete, company scoping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create TagLive with stream-based table showing usage counts - Inline form for create/edit using .input component for error display - Delete with confirmation dialog - Fix unique_constraint error_key on Tag and Category schemas to display validation errors on :name field instead of :company_id - Use .input component in CategoryLive for consistent error display - Add tests for mount, create, edit, delete, duplicates, company scoping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add nav links visible to all roles (after Invoices) - Categories uses hero-squares-2x2 icon - Tags uses hero-tag icon - Update role-based nav tests to verify new links Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix formatting in tag_live.ex and category_live_test.exs - Extract do_create_and_add_tag/2 and changeset_message/1 to reduce nesting depth (credo --strict compliance) - Update existing duplicate-name tests to assert error on :name field instead of :company_id after error_key change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Load categories and all_tags into assigns on mount - Add Category and Tag select dropdowns to filter bar - Parse category_id (UUID) and tag_id URL params into filters - Preserve category/tag filters across pagination - Backend already supports :category_id and :tag_ids filters - Add tests for filtering, URL updates, and dropdown rendering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the detail link to the Seller name column to save horizontal space. Update tests to assert on seller_name instead of invoice_number. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace .input component with manual form-control divs in both CategoryLive and TagLive forms. The .input component generates its own wrapper markup which broke alignment when mixed with manual inputs in a CSS grid. Use consistent flex-wrap layout instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace DaisyUI .label/.form-control classes with plain block labels. The .label class applies display:flex which causes labels to sit beside inputs instead of above them in a flex-wrap context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add db service (postgres:17-alpine) with healthcheck and persistent volume so developers can run locally without pointing to production. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
39 categories across 8 groups (people, office, assets, operations, recruitment, marketing, sales, others) with emoji, descriptions, and sort_order preserving the original group ordering. Idempotent via ON CONFLICT DO NOTHING — safe to re-run on production. Usage: psql $DATABASE_URL -v company_id="'<UUID>'" -f priv/repo/seed_categories.sql Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds company-scoped categories and tags across backend and UI: DB service in docker-compose, context preloads and new getters, schema error_key tweaks, UI components, LiveViews/routes for category/tag CRUD and invoice classification (filters, assignment, inline tag creation), seeds, and tests. Changes
Sequence DiagramsequenceDiagram
participant User as User (Browser)
participant LV as Invoice LiveView
participant Ctx as Invoices Context
participant DB as Database
User->>LV: open invoice show
LV->>Ctx: get_invoice_with_details(company_id, invoice_id, role)
Ctx->>DB: SELECT invoice JOIN categories,tags
DB-->>Ctx: invoice + category + tags
Ctx-->>LV: detailed invoice
LV-->>User: render invoice with category selector and tag list
User->>LV: set_category(category_id)
LV->>Ctx: set_category(invoice_id, category_id, user)
Ctx->>DB: UPDATE invoice (category_id,prediction_status)
DB-->>Ctx: ok
Ctx->>DB: SELECT updated invoice + preloads
DB-->>Ctx: updated invoice
Ctx-->>LV: updated invoice
LV-->>User: re-render with new category
User->>LV: toggle_tag(tag_id)
LV->>Ctx: toggle_tag(invoice_id, tag_id, user)
Ctx->>DB: INSERT/DELETE invoice_tag
DB-->>Ctx: ok
Ctx->>DB: SELECT updated invoice + preloads
DB-->>Ctx: updated invoice
Ctx-->>LV: updated invoice
LV-->>User: re-render with updated tags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
lib/ksef_hub_web/live/category_live.ex (1)
153-260: Consider extracting form/table into function components.The render block is doing two large responsibilities (editor + listing). Splitting these into components will simplify future changes and test updates.
Based on learnings, "Extract reusable logic into function components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/category_live.ex` around lines 153 - 260, Render is doing two responsibilities (editor + listing); extract the form and table into function components to simplify the LiveView. Create two components (e.g., category_form(assigns) and category_table(assigns)) that move the form block (inputs using `@form`, `@editing`, translate_error, phx-submit="save", phx-change="validate", phx-click handlers, cancel_edit) and the .table block (rows=@streams.categories, row_id/row_item lambdas, data-testid on category name, edit/delete phx handlers and confirm) respectively, keep all original attribute names/IDs/handlers and validation display, then replace the inline markup in the render with calls to <.category_form ... /> and <.category_table ... /> passing the needed assigns (form, editing, streams, etc.); ensure the components live in the same module or are imported so existing calls to edit/delete/save/validate still work.docker-compose.yml (1)
4-9: Avoid committed default DB credentials and broad host exposure.This works, but keeping static credentials in source and binding Postgres to all host interfaces is a avoidable risk even for dev environments.
🔧 Suggested hardening
db: image: postgres:17-alpine ports: - - "5432:5432" + - "127.0.0.1:5432:5432" environment: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: postgres - POSTGRES_DB: ksef_hub_dev + POSTGRES_USER: ${POSTGRES_USER:-postgres} + POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} + POSTGRES_DB: ${POSTGRES_DB:-ksef_hub_dev}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 4 - 9, The docker-compose service exposes Postgres to all interfaces and hardcodes credentials; change the ports mapping from "5432:5432" to bind localhost (e.g., "127.0.0.1:5432:5432") or remove the host mapping to avoid broad host exposure, and remove hardcoded POSTGRES_USER and POSTGRES_PASSWORD values from the environment block by loading them from an external .env or Docker secret (keep POSTGRES_DB configurable via env but not committed with real credentials); update references to the POSTGRES_USER, POSTGRES_PASSWORD, and ports mapping in the compose service so secrets/.env are used instead of literal values.lib/ksef_hub/invoices.ex (1)
120-128: Consider extracting a shared invoice-details query helper.
get_invoice_with_details!/3andget_invoice_with_details/3now duplicate the same query pipeline; a private builder would keep this easier to maintain.♻️ Optional refactor sketch
+ `@spec` invoice_with_details_query(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) :: Ecto.Query.t() + defp invoice_with_details_query(company_id, id, opts) do + Invoice + |> where([i], i.company_id == ^company_id and i.id == ^id) + |> maybe_scope_type_by_role(opts[:role]) + |> preload([:category, :tags]) + end + `@doc` "Fetches an invoice by UUID with category and tags preloaded." `@spec` get_invoice_with_details!(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) :: Invoice.t() def get_invoice_with_details!(company_id, id, opts \\ []) do - Invoice - |> where([i], i.company_id == ^company_id and i.id == ^id) - |> maybe_scope_type_by_role(opts[:role]) - |> preload([:category, :tags]) - |> Repo.one!() + company_id + |> invoice_with_details_query(id, opts) + |> Repo.one!() end `@doc` "Fetches an invoice by UUID with category and tags preloaded, returning nil if not found." `@spec` get_invoice_with_details(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) :: Invoice.t() | nil def get_invoice_with_details(company_id, id, opts \\ []) do - Invoice - |> where([i], i.company_id == ^company_id and i.id == ^id) - |> maybe_scope_type_by_role(opts[:role]) - |> preload([:category, :tags]) - |> Repo.one() + company_id + |> invoice_with_details_query(id, opts) + |> Repo.one() end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices.ex` around lines 120 - 128, Extract the shared query pipeline used by get_invoice_with_details/3 and get_invoice_with_details!/3 into a private helper (e.g., build_invoice_with_details_query/3) that accepts company_id, id, and role; move the Invoice |> where(...) |> maybe_scope_type_by_role(opts[:role]) |> preload([:category, :tags]) logic into that helper and have get_invoice_with_details/3 call Repo.one(build_invoice_with_details_query(...)) and get_invoice_with_details!/3 call Repo.one!(build_invoice_with_details_query(...)) so the query construction is centralized and both functions only differ by the Repo retrieval function.test/ksef_hub_web/live/invoice_live/show_test.exs (1)
146-147: Prefer selector-based assertions over raw HTML string checks in these new LiveView tests.The new assertions are readable, but they’re brittle against markup changes;
has_element?/3with stable selectors would make these tests more resilient.🧪 Example direction
- html = render(view) - assert html =~ "finance:invoices" + assert has_element?(view, "[data-testid=category-select]") + assert has_element?(view, "[data-testid=category-name]", "finance:invoices")- {:ok, _view, html} = live(conn, ~p"/invoices/#{invoice.id}") - assert html =~ "quarterly-report" + {:ok, view, _html} = live(conn, ~p"/invoices/#{invoice.id}") + assert has_element?(view, "[data-testid=tag-badge]", "quarterly-report")Also applies to: 155-157, 162-164, 180-182, 244-246
🤖 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 146 - 147, Replace brittle raw HTML string assertions that use render(view) and assert html =~ "..." with LiveView helper has_element?/3 calls to target stable selectors; e.g., instead of capturing html = render(view) and asserting the substring, call assert has_element?(view, "CSS_OR_DATA_TEST_SELECTOR", "finance:invoices") (and similarly for the other occurrences). Update the tests in invoice_live/show_test.exs to use meaningful selectors (prefer data-test attributes or component classes) and assert via has_element?(view, selector, expected_text) for each of the spots currently using assert html =~.lib/ksef_hub_web/live/invoice_live/index.ex (2)
13-21: Categories and tags loaded once inmountwill become stale during the LiveView session.If a user (or another tab) adds/deletes a category or tag, the filter dropdowns won't reflect that until the page is fully remounted. Consider reloading
categoriesandall_tagsinhandle_paramsalongside invoices, so apush_patchpicks up changes.This is non-critical since a page reload fixes it, but it's worth noting for UX consistency—especially since the tag/category management pages are separate LiveViews that could be used in a parallel tab.
🤖 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/index.ex` around lines 13 - 21, Currently categories and all_tags are only loaded in mount (assign in mount), which becomes stale; update the LiveView to reload categories and all_tags inside handle_params (the same place invoices are refreshed) by calling Invoices.list_categories(current_company.id) and Invoices.list_tags(current_company.id) and assigning the results to :categories and :all_tags on the socket so that push_patch navigation picks up changes; keep mount minimal (e.g., noop or initial assigns) but ensure handle_params uses socket.assigns.current_company (or company_id) to refetch and assign categories/all_tags whenever params change.
50-53: Operator precedence on tag_id extraction is correct but somewhat opaque.The expression
filters[:tag_ids] |> List.wrap() |> List.first() || ""works because||binds more loosely than|>, but it can surprise readers. A minor clarity improvement:♻️ Optional: explicit parentheses for readability
- "tag_id" => filters[:tag_ids] |> List.wrap() |> List.first() || "" + "tag_id" => (filters[:tag_ids] |> List.wrap() |> List.first()) || ""🤖 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/index.ex` around lines 50 - 53, The current "tag_id" value expression in the map building uses a pipeline whose result is then fallback-ed with || which relies on operator precedence; update the expression to make precedence explicit by wrapping the pipeline in parentheses so the fallback applies to the pipeline result (i.e., change the "tag_id" entry where filters[:tag_ids] |> List.wrap() |> List.first() || "" is used). This clarifies intent for future readers; alternatively you may assign the pipeline result to a temp variable (e.g., tag_id) and then use tag_id || "" before returning the map.lib/ksef_hub_web/live/invoice_live/show.ex (3)
214-217:tag_assigned?/2is a duplicate of the inline check at line 131.Both line 131 and line 215 do
Enum.any?(invoice.tags, &(&1.id == tag_id)). The private function is only used in the template. Consider also using it at line 131 for consistency.♻️ Optional: reuse tag_assigned? in toggle_tag
def handle_event("toggle_tag", %{"tag-id" => tag_id}, socket) do invoice = socket.assigns.invoice - currently_assigned = Enum.any?(invoice.tags, &(&1.id == tag_id)) + currently_assigned = tag_assigned?(invoice, tag_id)🤖 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 214 - 217, Replace the duplicated inline check Enum.any?(invoice.tags, &(&1.id == tag_id)) with the existing private helper tag_assigned?(invoice, tag_id) to avoid duplication; locate the inline check in the toggle_tag logic (the earlier usage around line 131) and call tag_assigned?/2 there instead of repeating the Enum.any? expression so the template and toggle_tag both use the same helper.
160-182:create_tag+add_invoice_tagis not wrapped in a transaction.If
create_tagsucceeds butadd_invoice_tagfails, an orphaned tag will exist (created but not associated with the invoice). While this is a low-probability scenario (the tag is in the same company), it could leave stale data.Consider wrapping in
Ecto.MultiorRepo.transaction/1for atomicity. This is non-critical since the orphaned tag is still a valid tag in the company and would appear in the tags list.🤖 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 160 - 182, do_create_and_add_tag currently calls Invoices.create_tag and Invoices.add_invoice_tag separately, which can leave an orphaned tag if the second call fails; change this to perform both steps in a single atomic operation (use Ecto.Multi or Repo.transaction) so that tag creation is rolled back when add_invoice_tag fails. Implement the transaction inside do_create_and_add_tag (or add a helper in Invoices) to run Invoices.create_tag and Invoices.add_invoice_tag as one multi/transaction, preserve the existing branch behavior on {:ok, ...} and map transactional errors back to the same changeset or generic error flash, and refresh invoice/all_tags via reload_details and Invoices.list_tags on success.
370-382:phx-keyup="new_tag_input"on every keystroke without debounce.Every keystroke triggers a round-trip to the server. Consider adding
phx-debounce="300"to reduce unnecessary traffic, consistent with the search input inindex.ex(line 296).♻️ Optional: add debounce
name="name" value={`@new_tag_name`} phx-keyup="new_tag_input" + phx-debounce="300" placeholder="New tag..."🤖 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 370 - 382, The input field handling tag creation currently fires phx-keyup="new_tag_input" on every keystroke causing excessive server round-trips; update the tag input element (the input with name="name" and phx-keyup="new_tag_input" used in the create_and_add_tag form) to include phx-debounce="300" so keyup events are debounced (match the debounce used in the search input), keeping server updates less frequent while preserving the current create_and_add_tag submit behavior.lib/ksef_hub_web/live/tag_live.ex (1)
30-33:validateevent doesn't run changeset validation — intentional?This mirrors the pattern in
CategoryLive, but the form won't show real-time validation errors (e.g., name uniqueness, required field) until submission. If that's acceptable UX, this is fine.🤖 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.ex` around lines 30 - 33, The "validate" handle_event currently just wraps raw params with to_form so no changeset validations run; update handle_event("validate", %{"tag" => params}, socket) to build and validate a changeset (e.g., call Tag.changeset/2 with the params), set the changeset action to :validate, and assign the form from that validated changeset (use to_form(changeset)) so real-time validation errors (required/uniqueness) are shown; reference handle_event/3, the "validate" event, Tag.changeset/2, and to_form.
🤖 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.ex`:
- Around line 12-25: Add missing `@doc` and `@spec` annotations for each public
callback in this module (e.g., mount/3 and the other public functions in ranges
29-83 and 145-262). For mount/3 add a brief `@doc` describing that it initializes
assigns and streams categories, and a `@spec` like mount(map(), map(),
Phoenix.LiveView.Socket.t()) :: {:ok, Phoenix.LiveView.Socket.t()}; apply the
same pattern to other public callbacks: add a one-line `@doc` describing purpose
and a `@spec` with concrete argument and return types (use
Phoenix.LiveView.Socket.t(), list/map types, and {:noreply, Socket.t()} or {:ok,
Socket.t()} as appropriate) so every public function has both annotations.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 184-189: The current changeset_message/1 discards translation
parameters by calling Ecto.Changeset.traverse_errors(fn {msg, _} -> msg end);
update changeset_message to call traverse_errors with a translator that applies
the error opts (e.g., &translate_error/1 or a function that calls
Gettext.dngettext/translate with the {msg, opts} tuple) so parameterized
messages like "should be at most %{count} characters" are interpolated
correctly; keep the rest of the function (Enum.map_join) intact so keys and
joined messages are returned as before.
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 84-101: Created/updated tag structs lack the virtual usage_count
(computed in list_tags), so update create_tag and update_tag to reload the saved
tag with usage_count before streaming: after Invoices.create_tag /
Invoices.update_tag returns {:ok, tag}, call a function that returns the tag
including the select_merge usage_count (e.g.
Invoices.get_tag_with_usage_count(tag.id) or fetch from
Invoices.list_tags(socket.assigns.current_company.id) and find by id) and then
pass that reloaded tag into stream_insert(:tags, ...) or stream_replace(:tags,
...); keep the existing assigns (form: new_form() or to_form(...)) and flash
behavior. Ensure you reference the reloaded tag (with usage_count) instead of
the raw Repo result when calling stream_insert/stream_replace.
In `@test/ksef_hub_web/live/category_live_test.exs`:
- Around line 25-29: Replace raw HTML string assertions in the "renders
categories page" test (and the other listed tests) with selector-driven LiveView
assertions: use element/2 and has_element?/2 (or render_change/render_click on
element/2) to assert presence of "#categories" container, "#category-form" form,
and specific rows/fields via their data-testid attributes (e.g.,
data-testid="category-row" or data-testid="category-name") in the test "renders
categories page" and corresponding tests instead of matching html =~
"Categories" or "New Category"; update the assertions to target those stable
selectors so they assert DOM presence and structure rather than raw text.
In `@test/ksef_hub_web/live/tag_live_test.exs`:
- Around line 26-29: The tests in tag_live_test.exs are asserting raw HTML
fragments from the live(conn, ~p"/tags") response (e.g., checking "Tags", "New
Tag", and usage counts) which is fragile; update each assertion to use LiveView
test selector helpers (element/2, has_element?/2, render_click/render_submit
where needed) targeting stable IDs or data attributes you added in the templates
(for example replace assert html =~ "Tags" with assert has_element?(view,
"#tags-heading") and assert html =~ "New Tag" with assert has_element?(view,
"#new-tag-button"); for usage counts replace text matching with
has_element?(view, "#tag-#{tag.id}-usage", "> 0") or by querying element(view,
"#tag-#{tag.id}-usage") and asserting its rendered text). Ensure you update
assertions around live/2, render_click, and render_submit calls (and all other
similar assertions noted in the comment) to use element/2 and has_element?/2
with the element IDs or data-testid attributes from your LiveView templates.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 4-9: The docker-compose service exposes Postgres to all interfaces
and hardcodes credentials; change the ports mapping from "5432:5432" to bind
localhost (e.g., "127.0.0.1:5432:5432") or remove the host mapping to avoid
broad host exposure, and remove hardcoded POSTGRES_USER and POSTGRES_PASSWORD
values from the environment block by loading them from an external .env or
Docker secret (keep POSTGRES_DB configurable via env but not committed with real
credentials); update references to the POSTGRES_USER, POSTGRES_PASSWORD, and
ports mapping in the compose service so secrets/.env are used instead of literal
values.
In `@lib/ksef_hub_web/live/category_live.ex`:
- Around line 153-260: Render is doing two responsibilities (editor + listing);
extract the form and table into function components to simplify the LiveView.
Create two components (e.g., category_form(assigns) and category_table(assigns))
that move the form block (inputs using `@form`, `@editing`, translate_error,
phx-submit="save", phx-change="validate", phx-click handlers, cancel_edit) and
the .table block (rows=@streams.categories, row_id/row_item lambdas, data-testid
on category name, edit/delete phx handlers and confirm) respectively, keep all
original attribute names/IDs/handlers and validation display, then replace the
inline markup in the render with calls to <.category_form ... /> and
<.category_table ... /> passing the needed assigns (form, editing, streams,
etc.); ensure the components live in the same module or are imported so existing
calls to edit/delete/save/validate still work.
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Around line 13-21: Currently categories and all_tags are only loaded in mount
(assign in mount), which becomes stale; update the LiveView to reload categories
and all_tags inside handle_params (the same place invoices are refreshed) by
calling Invoices.list_categories(current_company.id) and
Invoices.list_tags(current_company.id) and assigning the results to :categories
and :all_tags on the socket so that push_patch navigation picks up changes; keep
mount minimal (e.g., noop or initial assigns) but ensure handle_params uses
socket.assigns.current_company (or company_id) to refetch and assign
categories/all_tags whenever params change.
- Around line 50-53: The current "tag_id" value expression in the map building
uses a pipeline whose result is then fallback-ed with || which relies on
operator precedence; update the expression to make precedence explicit by
wrapping the pipeline in parentheses so the fallback applies to the pipeline
result (i.e., change the "tag_id" entry where filters[:tag_ids] |> List.wrap()
|> List.first() || "" is used). This clarifies intent for future readers;
alternatively you may assign the pipeline result to a temp variable (e.g.,
tag_id) and then use tag_id || "" before returning the map.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 214-217: Replace the duplicated inline check
Enum.any?(invoice.tags, &(&1.id == tag_id)) with the existing private helper
tag_assigned?(invoice, tag_id) to avoid duplication; locate the inline check in
the toggle_tag logic (the earlier usage around line 131) and call
tag_assigned?/2 there instead of repeating the Enum.any? expression so the
template and toggle_tag both use the same helper.
- Around line 160-182: do_create_and_add_tag currently calls Invoices.create_tag
and Invoices.add_invoice_tag separately, which can leave an orphaned tag if the
second call fails; change this to perform both steps in a single atomic
operation (use Ecto.Multi or Repo.transaction) so that tag creation is rolled
back when add_invoice_tag fails. Implement the transaction inside
do_create_and_add_tag (or add a helper in Invoices) to run Invoices.create_tag
and Invoices.add_invoice_tag as one multi/transaction, preserve the existing
branch behavior on {:ok, ...} and map transactional errors back to the same
changeset or generic error flash, and refresh invoice/all_tags via
reload_details and Invoices.list_tags on success.
- Around line 370-382: The input field handling tag creation currently fires
phx-keyup="new_tag_input" on every keystroke causing excessive server
round-trips; update the tag input element (the input with name="name" and
phx-keyup="new_tag_input" used in the create_and_add_tag form) to include
phx-debounce="300" so keyup events are debounced (match the debounce used in the
search input), keeping server updates less frequent while preserving the current
create_and_add_tag submit behavior.
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 30-33: The "validate" handle_event currently just wraps raw params
with to_form so no changeset validations run; update handle_event("validate",
%{"tag" => params}, socket) to build and validate a changeset (e.g., call
Tag.changeset/2 with the params), set the changeset action to :validate, and
assign the form from that validated changeset (use to_form(changeset)) so
real-time validation errors (required/uniqueness) are shown; reference
handle_event/3, the "validate" event, Tag.changeset/2, and to_form.
In `@lib/ksef_hub/invoices.ex`:
- Around line 120-128: Extract the shared query pipeline used by
get_invoice_with_details/3 and get_invoice_with_details!/3 into a private helper
(e.g., build_invoice_with_details_query/3) that accepts company_id, id, and
role; move the Invoice |> where(...) |> maybe_scope_type_by_role(opts[:role]) |>
preload([:category, :tags]) logic into that helper and have
get_invoice_with_details/3 call Repo.one(build_invoice_with_details_query(...))
and get_invoice_with_details!/3 call
Repo.one!(build_invoice_with_details_query(...)) so the query construction is
centralized and both functions only differ by the Repo retrieval function.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 146-147: Replace brittle raw HTML string assertions that use
render(view) and assert html =~ "..." with LiveView helper has_element?/3 calls
to target stable selectors; e.g., instead of capturing html = render(view) and
asserting the substring, call assert has_element?(view,
"CSS_OR_DATA_TEST_SELECTOR", "finance:invoices") (and similarly for the other
occurrences). Update the tests in invoice_live/show_test.exs to use meaningful
selectors (prefer data-test attributes or component classes) and assert via
has_element?(view, selector, expected_text) for each of the spots currently
using assert html =~.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docker-compose.ymllib/ksef_hub/invoices.exlib/ksef_hub/invoices/category.exlib/ksef_hub/invoices/tag.exlib/ksef_hub_web/components/invoice_components.exlib/ksef_hub_web/components/layouts.exlib/ksef_hub_web/live/category_live.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/router.expriv/repo/seed_categories.sqltest/ksef_hub/invoices/categories_test.exstest/ksef_hub/invoices/tags_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/live/category_live_test.exstest/ksef_hub_web/live/invoice_live/index_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/role_based_nav_test.exstest/ksef_hub_web/live/tag_live_test.exs
Comment on lines
+84
to
+101
| defp create_tag(socket, params) do | ||
| company_id = socket.assigns.current_company.id | ||
|
|
||
| case Invoices.create_tag(company_id, %{ | ||
| name: params["name"], | ||
| description: params["description"] | ||
| }) do | ||
| {:ok, tag} -> | ||
| {:noreply, | ||
| socket | ||
| |> stream_insert(:tags, tag) | ||
| |> assign(form: new_form()) | ||
| |> put_flash(:info, "Tag created.")} | ||
|
|
||
| {:error, %Ecto.Changeset{} = changeset} -> | ||
| {:noreply, assign(socket, form: to_form(changeset, as: :tag))} | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Tag schema and check how usage_count is defined
fd -t f -e ex -e exs | xargs grep -l "schema.*tag" -iRepository: appunite/appunite-ksef-ex
Length of output: 538
🏁 Script executed:
# Search for the Tag schema definition
rg -n "^defmodule.*Tag" --type=elixirRepository: appunite/appunite-ksef-ex
Length of output: 1292
🏁 Script executed:
# Look for usage_count field definition
rg -n "usage_count" --type=elixir -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 4661
🏁 Script executed:
# Check the list_tags implementation
rg -n "def list_tags" --type=elixir -A 20Repository: appunite/appunite-ksef-ex
Length of output: 1313
🏁 Script executed:
# Check create_tag and update_tag implementations
rg -n "def create_tag|def update_tag" --type=elixir -A 15Repository: appunite/appunite-ksef-ex
Length of output: 1574
🏁 Script executed:
# Verify the template at line 203 of tag_live.ex
sed -n '200,210p' lib/ksef_hub_web/live/tag_live.exRepository: appunite/appunite-ksef-ex
Length of output: 503
Stream newly created and updated tags with usage_count populated.
When create_tag and update_tag return tags from Repo.insert() and Repo.update(), they lack the usage_count virtual field that is computed via select_merge in list_tags. The template at line 203 renders {tag.usage_count}, which displays nil (blank) instead of 0 for newly inserted or updated stream entries.
🔧 Proposed fix
For create_tag:
{:ok, tag} ->
{:noreply,
socket
- |> stream_insert(:tags, tag)
+ |> stream_insert(:tags, %{tag | usage_count: 0})
|> assign(form: new_form())
|> put_flash(:info, "Tag created.")}For update_tag, preserve the existing count:
{:ok, updated} ->
+ updated = %{updated | usage_count: tag.usage_count}
{:noreply,
socket
|> stream_insert(:tags, updated)🤖 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.ex` around lines 84 - 101, Created/updated tag
structs lack the virtual usage_count (computed in list_tags), so update
create_tag and update_tag to reload the saved tag with usage_count before
streaming: after Invoices.create_tag / Invoices.update_tag returns {:ok, tag},
call a function that returns the tag including the select_merge usage_count
(e.g. Invoices.get_tag_with_usage_count(tag.id) or fetch from
Invoices.list_tags(socket.assigns.current_company.id) and find by id) and then
pass that reloaded tag into stream_insert(:tags, ...) or stream_replace(:tags,
...); keep the existing assigns (form: new_form() or to_form(...)) and flash
behavior. Ensure you reference the reloaded tag (with usage_count) instead of
the raw Repo result when calling stream_insert/stream_replace.
Comment on lines
+25
to
+29
| test "renders categories page", %{conn: conn} do | ||
| {:ok, _view, html} = live(conn, ~p"/categories") | ||
| assert html =~ "Categories" | ||
| assert html =~ "New Category" | ||
| end |
There was a problem hiding this comment.
Use selector-driven assertions instead of raw HTML matching in this LiveView suite.
These checks should target stable selectors (#category-form, #categories, data-testid rows/fields) to avoid brittle failures from markup/text shifts.
🔧 Example refactor pattern
- {:ok, _view, html} = live(conn, ~p"/categories")
- assert html =~ "finance:invoices"
+ {:ok, view, _html} = live(conn, ~p"/categories")
+ assert has_element?(view, ~s([data-testid="category-name-#{cat.id}"]), "finance:invoices")- html = render(view)
- assert html =~ "Category deleted."
- refute html =~ "delete:me"
+ assert has_element?(view, "#categories")
+ refute has_element?(view, ~s([data-testid="category-name-#{cat.id}"]))Also applies to: 34-37, 55-57, 69-71, 82-85, 100-103, 111-115, 123-130, 138-140
🤖 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 25 - 29, Replace
raw HTML string assertions in the "renders categories page" test (and the other
listed tests) with selector-driven LiveView assertions: use element/2 and
has_element?/2 (or render_change/render_click on element/2) to assert presence
of "#categories" container, "#category-form" form, and specific rows/fields via
their data-testid attributes (e.g., data-testid="category-row" or
data-testid="category-name") in the test "renders categories page" and
corresponding tests instead of matching html =~ "Categories" or "New Category";
update the assertions to target those stable selectors so they assert DOM
presence and structure rather than raw text.
Comment on lines
+26
to
+29
| {:ok, _view, html} = live(conn, ~p"/tags") | ||
| assert html =~ "Tags" | ||
| assert html =~ "New Tag" | ||
| end |
There was a problem hiding this comment.
Replace raw HTML and markup-fragment assertions with stable selector assertions.
This suite currently depends on text and exact HTML fragments (notably the usage count assertion), which is fragile for LiveView rendering changes.
🔧 Example refactor pattern
- {:ok, _view, html} = live(conn, ~p"/tags")
- assert html =~ "counted"
- assert html =~ ">1</span>"
+ {:ok, view, _html} = live(conn, ~p"/tags")
+ assert has_element?(view, ~s([data-testid="tag-name-#{tag.id}"]), "counted")
+ assert has_element?(view, "#tags", "1")- html = render(view)
- assert html =~ "Tag deleted."
- refute html =~ "delete-me"
+ assert has_element?(view, "#tags")
+ refute has_element?(view, ~s([data-testid="tag-name-#{tag.id}"]))Also applies to: 34-36, 43-47, 58-60, 72-74, 85-88, 101-104, 112-116, 124-131, 139-140
🤖 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 26 - 29, The tests in
tag_live_test.exs are asserting raw HTML fragments from the live(conn,
~p"/tags") response (e.g., checking "Tags", "New Tag", and usage counts) which
is fragile; update each assertion to use LiveView test selector helpers
(element/2, has_element?/2, render_click/render_submit where needed) targeting
stable IDs or data attributes you added in the templates (for example replace
assert html =~ "Tags" with assert has_element?(view, "#tags-heading") and assert
html =~ "New Tag" with assert has_element?(view, "#new-tag-button"); for usage
counts replace text matching with has_element?(view, "#tag-#{tag.id}-usage", ">
0") or by querying element(view, "#tag-#{tag.id}-usage") and asserting its
rendered text). Ensure you update assertions around live/2, render_click, and
render_submit calls (and all other similar assertions noted in the comment) to
use element/2 and has_element?/2 with the element IDs or data-testid attributes
from your LiveView templates.
- Add @doc/@SPEC to all public callbacks in CategoryLive and TagLive - Fix changeset_message/1 to use translate_error/1 instead of discarding interpolation opts (e.g. %{count} in error messages) - Fix TagLive usage_count: set virtual field after create (0) and preserve existing count after update, so table doesn't show nil - Use tag_assigned?/2 helper in toggle_tag instead of inline Enum.any? - Add phx-debounce="300" to new tag input to reduce server round-trips - Extract first_tag_id variable in filter_assigns for clarity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CategoryLive and TagLive: - Assign company_id once in mount, reference from assigns everywhere - Use real changeset validation in "validate" events so errors show on keystrokes (not just on submit) - Use changeset-backed forms throughout (new_changeset_form replaces raw param maps) - Edit handler builds form from changeset instead of manual map show.ex: - Consolidate two set_category handlers (empty vs non-empty) into one by normalizing "" to nil upfront Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The category <select> had phx-change directly on it without being inside a <form>, which can cause the change event to not fire properly in the browser with LiveView 1.x. This led to category selections reverting after a few seconds (socket reconnect with unsaved state). Also separated mark_prediction_manual from the category save flow so a prediction status failure cannot silently prevent the category update from being reflected in the UI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
17-36:⚠️ Potential issue | 🟡 Minor
socket.assigns.current_companycould raiseKeyErrorif the key is absent.Line 18 uses dot-access on
socket.assigns, which is a plain map. Ifcurrent_companyis never set (e.g., during a socket reconnect or a plug bypass), this raises aKeyErrorbefore thecondguard on line 27 gets a chance to check fornil.🛡️ Safer access
- company = socket.assigns.current_company + company = socket.assigns[:current_company]🤖 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 17 - 36, In mount/3 replace the unsafe dot-access of socket.assigns.current_company with a safe fetch (e.g., company = Map.get(socket.assigns, :current_company) or company = socket.assigns[:current_company]) so accessing the missing key does not raise; then continue to use that local company variable in the existing cond branch that calls mount_invoice/3 and in the nil check branch so behavior remains identical but safe on reconnects or missing assigns.
♻️ Duplicate comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
177-182: Past issue resolved —changeset_message/1now uses&translate_error/1.This addresses the earlier review comment about discarded error opts. The parameterized messages (e.g.,
%{count}) will now be interpolated correctly.🤖 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 177 - 182, The changeset_message/1 now correctly pipes errors through &translate_error/1, but ensure translate_error/1 is actually available and used: import or alias the module that defines translate_error/1 (commonly ErrorHelpers or your Web.ErrorHelpers) so the function resolves at compile time; also normalize the field key in the Enum.map_join by converting k to a readable string (handle atoms and binaries) before joining the error texts so the output shows a humanized field name plus the interpolated messages (e.g., transform k via Atom.to_string/1 or leave as-is if already a string, replace underscores and capitalize).
🧹 Nitpick comments (5)
lib/ksef_hub_web/live/invoice_live/show.ex (2)
364-376: Minor UX:phx-keyup+ server-assignedvalue=may cause input flickering.The
phx-keyup="new_tag_input"updates@new_tag_nameon the server, which re-renders the input withvalue={@new_tag_name}. Combined withphx-debounce="300", this can cause the cursor to jump or characters to appear out of order under latency. Since this is a small "tag name" field (short strings, low frequency), the impact is minimal — but if you notice UX issues in testing, consider dropping thephx-keyupbinding entirely and relying solely on the form submit params.🤖 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 364 - 376, The input uses phx-keyup="new_tag_input" which updates `@new_tag_name` on the server and re-renders the input value={`@new_tag_name`}, causing cursor flicker under latency; remove the real-time binding by deleting phx-keyup="new_tag_input" (and any server handler new_tag_input if now unused) so the field is only submitted via the form action "create_and_add_tag" and read from params, leaving phx-debounce out or only on client-side handlers if you later reintroduce live updates.
66-99: Inconsistent@docon@implcallbacks.Line 66 has a
@docfor thehandle_event("approve"/…)callbacks, but the newhandle_eventcallbacks at lines 103, 121, 140, and 145 lack@docannotations. Per coding guidelines, every public function must have a@doc. Even for@implcallbacks, keeping them consistent is good practice.As per coding guidelines: "Every public function must have a
@docannotation".Also applies to: 103-151
🤖 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 66 - 99, Add missing `@doc` annotations for all public handle_event/3 clauses to match the existing pattern: each handle_event("approve", _params, socket) and handle_event("reject", _params, socket) already have docs, so add brief `@doc` strings above the other handle_event/3 implementations (the remaining handle_event clauses in this module) describing the event handled and its behavior (e.g., "Handles X action for invoices" or similar). Ensure every public function (all handle_event/3 clauses) has a `@doc` line just above the `@impl` true and before the def so the module follows the "every public function must have a `@doc`" guideline.lib/ksef_hub_web/live/invoice_live/index.ex (2)
42-55: Duplicated tag-id extraction — consider a small helper.The pattern
filters[:tag_ids] |> List.wrap() |> List.first()appears identically here (line 44) and inpagination_params(line 176). A tiny private helper (e.g.,first_tag_id/1) would keep both call sites in sync.♻️ Proposed helper
+ defp first_tag_id(filters), do: filters[:tag_ids] |> List.wrap() |> List.first()Then replace both occurrences with
first_tag_id(filters).🤖 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/index.ex` around lines 42 - 55, The duplicated extraction of the first tag id (filters[:tag_ids] |> List.wrap() |> List.first()) should be extracted into a tiny private helper like first_tag_id(filters) to avoid duplication; add a private function first_tag_id/1 that returns that expression and replace the uses in filter_assigns (currently computing first_tag_id) and in pagination_params to call first_tag_id(filters), keeping behavior identical.
111-123: Missing@specon several private helpers.Per coding guidelines, every function (public and private) must have a
@spectypespec. The following private functions lack one:
maybe_put_date/3(line 111)maybe_put_search/3(line 121)maybe_put/3(line 158)♻️ Example specs
+ `@spec` maybe_put_date(map(), atom(), String.t() | nil) :: map() defp maybe_put_date(map, _key, nil), do: map + `@spec` maybe_put_search(map(), atom(), String.t() | nil) :: map() defp maybe_put_search(map, _key, nil), do: map + `@spec` maybe_put(map(), String.t(), String.t() | nil) :: map() defp maybe_put(map, _key, nil), do: mapAs per coding guidelines: "Every function (public and private) must have a
@spectypespec".Also applies to: 136-160
🤖 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/index.ex` around lines 111 - 123, Add `@spec` annotations for the private helper functions: declare maybe_put_date(map(), atom(), String.t() | nil) :: map(), maybe_put_search(map(), atom(), String.t() | nil) :: map(), and maybe_put(map(), atom(), any()) :: map() (or adjust the value type in maybe_put/3 to the precise type used). Place each `@spec` immediately above the corresponding defp (maybe_put_date/3, maybe_put_search/3, maybe_put/3) to satisfy the module's typespec requirement.lib/ksef_hub_web/live/tag_live.ex (1)
37-41: Prefer struct update syntax overMap.put/3on a changeset.
%{changeset | action: :validate}is the idiomatic Elixir way to update a struct field and makes the intent clearer.♻️ Proposed refactor
- changeset = - changeset_for(socket.assigns.editing, params) - |> Map.put(:action, :validate) + changeset = %{changeset_for(socket.assigns.editing, params) | action: :validate}🤖 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.ex` around lines 37 - 41, Replace the Map.put/3 call with struct update syntax to set the changeset action: after computing changeset via changeset_for(socket.assigns.editing, params) update the action using the struct form (e.g. %{changeset | action: :validate}) instead of Map.put so the intent is clearer and idiomatic; ensure you reference the changeset variable produced by changeset_for/2 when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 126-133: Change new_changeset to accept and use the real
company_id instead of the string "placeholder" (e.g. defp new_changeset(params
\\ %{}, company_id) and build %Tag{company_id: company_id} |>
Tag.changeset(...)) and add explicit `@specs` for both arities (0/1 and 2) so the
default-arg head is covered; update new_changeset_form and all call sites to
pass socket.assigns.company_id into new_changeset (e.g. new_changeset(%{},
socket.assigns.company_id)) so form-level validation and foreign key checks use
the actual company_id.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 17-36: In mount/3 replace the unsafe dot-access of
socket.assigns.current_company with a safe fetch (e.g., company =
Map.get(socket.assigns, :current_company) or company =
socket.assigns[:current_company]) so accessing the missing key does not raise;
then continue to use that local company variable in the existing cond branch
that calls mount_invoice/3 and in the nil check branch so behavior remains
identical but safe on reconnects or missing assigns.
---
Duplicate comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 177-182: The changeset_message/1 now correctly pipes errors
through &translate_error/1, but ensure translate_error/1 is actually available
and used: import or alias the module that defines translate_error/1 (commonly
ErrorHelpers or your Web.ErrorHelpers) so the function resolves at compile time;
also normalize the field key in the Enum.map_join by converting k to a readable
string (handle atoms and binaries) before joining the error texts so the output
shows a humanized field name plus the interpolated messages (e.g., transform k
via Atom.to_string/1 or leave as-is if already a string, replace underscores and
capitalize).
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Around line 42-55: The duplicated extraction of the first tag id
(filters[:tag_ids] |> List.wrap() |> List.first()) should be extracted into a
tiny private helper like first_tag_id(filters) to avoid duplication; add a
private function first_tag_id/1 that returns that expression and replace the
uses in filter_assigns (currently computing first_tag_id) and in
pagination_params to call first_tag_id(filters), keeping behavior identical.
- Around line 111-123: Add `@spec` annotations for the private helper functions:
declare maybe_put_date(map(), atom(), String.t() | nil) :: map(),
maybe_put_search(map(), atom(), String.t() | nil) :: map(), and maybe_put(map(),
atom(), any()) :: map() (or adjust the value type in maybe_put/3 to the precise
type used). Place each `@spec` immediately above the corresponding defp
(maybe_put_date/3, maybe_put_search/3, maybe_put/3) to satisfy the module's
typespec requirement.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 364-376: The input uses phx-keyup="new_tag_input" which updates
`@new_tag_name` on the server and re-renders the input value={`@new_tag_name`},
causing cursor flicker under latency; remove the real-time binding by deleting
phx-keyup="new_tag_input" (and any server handler new_tag_input if now unused)
so the field is only submitted via the form action "create_and_add_tag" and read
from params, leaving phx-debounce out or only on client-side handlers if you
later reintroduce live updates.
- Around line 66-99: Add missing `@doc` annotations for all public handle_event/3
clauses to match the existing pattern: each handle_event("approve", _params,
socket) and handle_event("reject", _params, socket) already have docs, so add
brief `@doc` strings above the other handle_event/3 implementations (the remaining
handle_event clauses in this module) describing the event handled and its
behavior (e.g., "Handles X action for invoices" or similar). Ensure every public
function (all handle_event/3 clauses) has a `@doc` line just above the `@impl` true
and before the def so the module follows the "every public function must have a
`@doc`" guideline.
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 37-41: Replace the Map.put/3 call with struct update syntax to set
the changeset action: after computing changeset via
changeset_for(socket.assigns.editing, params) update the action using the struct
form (e.g. %{changeset | action: :validate}) instead of Map.put so the intent is
clearer and idiomatic; ensure you reference the changeset variable produced by
changeset_for/2 when making this change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ksef_hub_web/live/category_live.ex
- tag_live/category_live: replace "placeholder" company_id in
new_changeset with real company_id from assigns; use idiomatic
struct update (%{changeset | action: :validate}) instead of Map.put
- show.ex: use safe bracket access for socket.assigns[:current_company]
consistent with [:current_user]; remove phx-keyup="new_tag_input"
round-trips and use form id keying for input reset on submit
- index.ex: extract first_tag_id/1 helper to DRY duplicated expression;
add missing @SPEC to maybe_put_date/3, maybe_put_search/3, maybe_put/3
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/index.ex (1)
13-21: Consider pattern matching for clarity and safety.Line 14 uses
socket.assigns[:current_company] && socket.assigns.current_company.id, which works but is less idiomatic than extracting via pattern matching. Acaseor direct match would be clearer and avoid the double access.♻️ Suggested refactor
def mount(_params, _session, socket) do - company_id = socket.assigns[:current_company] && socket.assigns.current_company.id - - {:ok, - assign(socket, - page_title: "Invoices", - categories: if(company_id, do: Invoices.list_categories(company_id), else: []), - all_tags: if(company_id, do: Invoices.list_tags(company_id), else: []) - )} + {categories, all_tags} = + case socket.assigns[:current_company] do + %{id: company_id} -> + {Invoices.list_categories(company_id), Invoices.list_tags(company_id)} + _ -> + {[], []} + end + + {:ok, + assign(socket, + page_title: "Invoices", + categories: categories, + all_tags: all_tags + )} endThis mirrors the existing
casepattern already used inhandle_params(line 31), maintaining consistency within the module.🤖 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/index.ex` around lines 13 - 21, In mount/3 replace the boolean-and access of socket.assigns to extract company_id with explicit pattern matching or a case on socket.assigns (e.g., match %{current_company: %Company{id: id}} or case socket.assigns do %{current_company: %{id: id}} -> id; _ -> nil end) so you only access current_company once and get a clear company_id value for building the assigns passed to assign/2 (refer to mount/3 and the socket.assigns[:current_company] usage in the diff).
🤖 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.ex`:
- Around line 69-80: The else branch currently treats any {:error, _} from
Invoices.get_category/2 the same, masking {:error, :not_found} (concurrent
deletion) as a deletion failure; change handle_event("delete", ...) to
explicitly match {:error, :not_found} and {:error, reason} separately — for
{:error, :not_found} remove the stale entry from the stream (call
stream_delete(:categories, id) or otherwise remove by id) and put a neutral info
flash like "Category not found or already deleted", and for {:error, _} keep the
existing put_flash(:error, "Failed to delete category.") behavior; keep the
successful {:ok, category} flow as-is.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 123-139: In handle_event("toggle_tag") validate the incoming
"tag-id" against socket.assigns.all_tags before calling tag_assigned?,
Invoices.add_invoice_tag or Invoices.remove_invoice_tag: first ensure the tag-id
exists in socket.assigns.all_tags (and normalize/convert its type as needed), if
it is invalid return {:noreply, put_flash(socket, :error, "Invalid tag")},
otherwise continue with the existing logic and call reload_details on success;
do not call Invoices.* functions for unknown/tampered IDs.
- Around line 104-118: The handle_event "set_category" should validate and
sanitize the incoming raw_id (from the raw_id param) before passing it to
Invoices.set_invoice_category — e.g. treat empty string as nil, parse numeric
ids to integers and reject/return an error flash for any malformed values — and
then handle the result of Invoices.mark_prediction_manual/1 instead of ignoring
it: after a successful {:ok, updated} from Invoices.set_invoice_category call,
call Invoices.mark_prediction_manual(updated) and pattern-match on its return
(handle {:ok, _} and {:error, reason}) to either proceed with assign(socket,
:invoice, reload_details(...)) or rollback/show a flash on failure; reference
the handle_event "set_category" function, the raw_id/category_id variables,
Invoices.set_invoice_category/2, Invoices.mark_prediction_manual/1, and
reload_details/2 when making these changes.
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 53-63: The edit handler currently assigns a raw Tag from
Invoices.get_tag/2 which lacks the select_merge-calculated usage_count, so when
update_tag/3 calls with_usage_count(updated, tag.usage_count) it passes nil and
resets usage to 0; fix by either A) after fetching tag in handle_event("edit",
...) load the usage_count from DB (e.g., call a new
Invoices.get_tag_with_usage_count/2 or re-run the list_tags select_merge for
that id) and assign that enriched struct to socket.assigns.editing, or B)
preserve the streamed entry’s usage_count by looking up the tag via the existing
list_tags query (which includes select_merge) when assigning editing; update
code locations: handle_event("edit", ...), Invoices.get_tag/2 usage, and ensure
update_tag/3 continues to call with_usage_count using the enriched tag.
- Around line 70-81: In handle_event("delete", %{"id" => id}, socket) within
TagLive (function handle_event), adjust the else clause to distinguish {:error,
:not_found} from other errors so that errors from Invoices.get_tag(company_id,
id) return put_flash(socket, :error, "Tag not found.") while other {:error, _}
still return "Failed to delete tag."; update pattern matching order in the else
(or refactor to case) to check for {:error, :not_found} before the generic
{:error, _}, and keep existing stream_delete/put_flash success path for
Invoices.delete_tag.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 140-148: The test "displays category name and emoji" currently
only asserts the category name; update it to also assert the emoji is rendered
by checking the rendered HTML or the category element for the emoji (use the
existing variables category and emoji created via insert(:category, ...), the
live view returned as view from live(conn, ~p"/invoices/#{invoice.id}"), and the
rendered HTML by render(view)); for example add an assertion that html contains
the emoji (e.g., assert html =~ "💰") or assert has_element?(view,
"[data-testid=category-select]", "💰") so the emoji rendering will be validated.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Around line 13-21: In mount/3 replace the boolean-and access of socket.assigns
to extract company_id with explicit pattern matching or a case on socket.assigns
(e.g., match %{current_company: %Company{id: id}} or case socket.assigns do
%{current_company: %{id: id}} -> id; _ -> nil end) so you only access
current_company once and get a clear company_id value for building the assigns
passed to assign/2 (refer to mount/3 and the socket.assigns[:current_company]
usage in the diff).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.extest/ksef_hub_web/live/invoice_live/show_test.exs
- Add get_tag_with_usage_count/2 to preserve usage_count on tag edit - Distinguish :not_found from other errors in category/tag delete handlers - Validate category_id as UUID and handle mark_prediction_manual result - Validate tag-id against all_tags before toggling - Add emoji assertion in show test - Use pattern matching for company_id in index mount Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
170-171:create_and_add_tagshould be a single atomic context operation.Right now, if tag creation succeeds and association fails, you keep a partially applied write (tag created, not attached). Consider moving this into one context function with transactional semantics and call that from the LiveView.
As per coding guidelines, "Use Ecto transactions and unique constraints for atomicity when performing multi-step database operations."
Also applies to: 179-185
🤖 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 170 - 171, The two-step tag creation + association (Invoices.create_tag followed by Invoices.add_invoice_tag) must be combined into a single transactional context function (e.g., Invoices.create_and_add_tag/3) that wraps Repo.transaction, creates the tag, inserts the association (add_invoice_tag logic) and enforces unique constraints so both succeed or both roll back; then update the LiveView call site in invoice_live/show.ex to call that new Invoices.create_and_add_tag(invoice.id, company_id, %{name: name}) and handle {:ok, result} / {:error, reason} responses instead of calling create_tag and add_invoice_tag separately.test/ksef_hub_web/live/invoice_live/show_test.exs (1)
146-148: Prefer selector assertions over rawrender(view)string matching.These checks are brittle and can pass on unrelated text. Use
has_element?/2/has_element?/3with stable selectors for category, tag, and indicator assertions.🔧 Example direction
- html = render(view) - assert html =~ "finance:invoices" - assert html =~ "💰" + assert has_element?(view, "[data-testid=category-select] option", "💰 finance:invoices")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").Also applies to: 157-158, 164-165, 181-183, 245-247
🤖 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 146 - 148, Replace brittle string assertions that match raw HTML from render(view) with selector-based assertions using Phoenix.LiveViewTest helpers: instead of calling render(view) and asserting substring matches for the category ("finance:invoices"), the tag/icon ("💰"), etc., use element/2 or element/3 and assert has_element?(view, selector) (e.g., assert has_element?(view, "#invoice-category", "finance:invoices") or assert has_element?(view, ".invoice-tag", "💰")) so tests target stable DOM nodes; update the assertions around render(view), the category/tag/indicator checks, and any similar occurrences (including the other noted ranges) to use has_element?/element selectors against the view variable.
🤖 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.ex`:
- Around line 141-143: The Integer.parse usage in the sort_order handling (case
Integer.parse(params["sort_order"] || "0") do ...) accepts partially-parsed
strings like "12abc"; change the match to require full consumption by matching
{n, ""} -> n and treat any other result (including {_, _} and :error) as 0;
update the same pattern wherever you parse params["sort_order"] in this LiveView
(CategoryLive) and audit other LiveView event handlers (*.live.ex) to validate
and fully-consume parsed numeric state changes before applying them.
- Around line 53-55: Validate the incoming "id" param in the LiveView before
calling the Invoices context: in handle_event("edit", %{"id" => id}, socket) and
handle_event("delete", %{"id" => id}, socket) first parse/validate id (e.g.,
Integer.parse/1 for numeric IDs or Ecto.UUID.cast/1 for UUIDs) and branch on the
result; if invalid, call put_flash(socket, :error, "Invalid category ID") (or
similar) and halt without invoking Invoices.get_category or
Invoices.delete_category, otherwise call the context with the
validated/converted id.
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 348-364: The category (and new-tag) form in invoice_live/show.ex
currently renders a raw <form> and <select> (phx-change="set_category") instead
of using the project's LiveView form convention; update mount and relevant
handle_event(s) (e.g., mount and the "set_category" handler) to build and assign
a to_form/2-driven assign (e.g., `@category_form` and `@new_tag_form`, following
tag_live.ex), then replace the raw <form> with the component-style <.form
for={`@category_form`} phx-change="set_category"> and bind the select to
`@category_form`[:category_id] (and similarly use `@new_tag_form`[:field] for tags)
so the template uses the managed form assigns and field helpers consistently
with the codebase.
In `@lib/ksef_hub_web/live/tag_live.ex`:
- Around line 54-56: Validate the incoming id in the "edit" and "delete" event
handlers using Ecto.UUID.cast() before calling Invoices functions: call
Ecto.UUID.cast(id) in the handle_event("edit", %{"id" => id}, socket) path
(which currently calls Invoices.get_tag_with_usage_count) and in the
handle_event("delete", %{"id" => id}, socket) path (which calls
Invoices.delete_tag or similar), branch on the cast result and when it returns
:error push an error flash to the socket and halt further processing; only pass
the cast UUID to the Invoices functions when cast returns {:ok, uuid}. Ensure
you reference the existing handler functions ("edit" and "delete") and the
context calls (Invoices.get_tag_with_usage_count / Invoices.delete_tag) when
making the change.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 170-171: The two-step tag creation + association
(Invoices.create_tag followed by Invoices.add_invoice_tag) must be combined into
a single transactional context function (e.g., Invoices.create_and_add_tag/3)
that wraps Repo.transaction, creates the tag, inserts the association
(add_invoice_tag logic) and enforces unique constraints so both succeed or both
roll back; then update the LiveView call site in invoice_live/show.ex to call
that new Invoices.create_and_add_tag(invoice.id, company_id, %{name: name}) and
handle {:ok, result} / {:error, reason} responses instead of calling create_tag
and add_invoice_tag separately.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 146-148: Replace brittle string assertions that match raw HTML
from render(view) with selector-based assertions using Phoenix.LiveViewTest
helpers: instead of calling render(view) and asserting substring matches for the
category ("finance:invoices"), the tag/icon ("💰"), etc., use element/2 or
element/3 and assert has_element?(view, selector) (e.g., assert
has_element?(view, "#invoice-category", "finance:invoices") or assert
has_element?(view, ".invoice-tag", "💰")) so tests target stable DOM nodes;
update the assertions around render(view), the category/tag/indicator checks,
and any similar occurrences (including the other noted ranges) to use
has_element?/element selectors against the view variable.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/ksef_hub/invoices.exlib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.extest/ksef_hub_web/live/invoice_live/show_test.exs
- Validate incoming IDs with Ecto.UUID.cast before calling context in
category and tag LiveView edit/delete handlers
- Fix Integer.parse sort_order to require full consumption ({n, ""})
- Convert raw <form> to <.form> with managed assigns in show.ex
(category_form, new_tag_form) for consistency with codebase conventions
- Add transactional Invoices.create_and_add_tag/3 to atomically create
a tag and associate it with an invoice
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 138-154: In the handle_event "toggle_tag" code path (the function
handling tag toggles in invoice_live/show.ex), after a successful tag addition
or removal (the {:ok, _} branch where you call Invoices.add_invoice_tag/3 or
Invoices.remove_invoice_tag/2 and then reload details), also call
Invoices.mark_prediction_manual(invoice.id) so the invoice prediction is marked
manual; update both occurrences of this success branch (the other similar block
around the second toggle handler) to call Invoices.mark_prediction_manual/1
before assigning the reloaded invoice to socket and returning {:noreply, ...}.
- Around line 111-116: The reload is happening before updating the manual
prediction status, causing stale prediction_status in assigns; change the flow
in the show LiveView so you call Invoices.mark_prediction_manual(updated) first
and only on {:ok, _} call reload_details(updated, socket) and then assign the
freshly reloaded invoice and category_form(reloaded) to socket (update the block
around Invoices.mark_prediction_manual/1, reload_details/2, assign/2 and ensure
prediction_status from the reloaded invoice is used).
- Around line 40-44: mount_invoice/3 currently passes the raw route id into
Invoices.get_invoice_with_details which can raise Ecto.CastError for malformed
IDs; validate the id first by using Ecto.UUID.cast(id) (same approach as
validate_category_id/1) and if the cast returns :error or nil perform the same
graceful redirect/handle_error flow you use elsewhere (assigning flash/redirect
or returning {:halt, socket} as appropriate), otherwise call
Invoices.get_invoice_with_details with the validated UUID; reference
mount_invoice/3, Invoices.get_invoice_with_details and Ecto.UUID.cast/1 when
making the change.
In `@lib/ksef_hub/invoices.ex`:
- Around line 788-794: In create_and_add_tag/3 wrap the add flow with an
explicit invoice/company consistency check: inside the Repo.transaction block
(where create_tag and add_invoice_tag are called) load the invoice by invoice_id
(e.g., via Repo.get or the existing get_invoice function), verify its company_id
matches the caller-provided company_id, and if not return {:error,
:invoice_company_mismatch} (or Repo.rollback with a clear reason) before calling
add_invoice_tag; keep the existing create_tag and add_invoice_tag calls and only
proceed when the invoice belongs to the same company.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/ksef_hub/invoices.exlib/ksef_hub_web/live/category_live.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/tag_live.ex
Comment on lines
+40
to
+44
| defp mount_invoice(socket, company, id) do | ||
| role = socket.assigns[:current_role] | ||
|
|
||
| case Invoices.get_invoice_with_details(company.id, id, role: role) do | ||
| nil -> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the actual code around line 40-44
if [ -f "lib/ksef_hub_web/live/invoice_live/show.ex" ]; then
echo "=== File exists ==="
wc -l lib/ksef_hub_web/live/invoice_live/show.ex
echo "=== Lines 35-50 ==="
sed -n '35,50p' lib/ksef_hub_web/live/invoice_live/show.ex | cat -n
else
echo "File not found"
fiRepository: appunite/appunite-ksef-ex
Length of output: 715
🏁 Script executed:
# Check the Invoices module to understand get_invoice_with_details signature
fd -t f "invoices\.ex" --exec grep -l "get_invoice_with_details" {} \;Repository: appunite/appunite-ksef-ex
Length of output: 95
🏁 Script executed:
# Search for the exact function definition
rg "def get_invoice_with_details" -A 5Repository: appunite/appunite-ksef-ex
Length of output: 843
🏁 Script executed:
# Check the Invoice schema to see what type the id field is
fd -t f "invoice\.ex" | grep -E "schemas|models" | head -5Repository: appunite/appunite-ksef-ex
Length of output: 104
🏁 Script executed:
# Search for Invoice schema definition
rg "schema \"invoices\"" -A 10Repository: appunite/appunite-ksef-ex
Length of output: 794
🏁 Script executed:
# Check if id is explicitly defined as UUID in the schema
rg "field :id" lib/ksef_hub/invoices/ -B 2 -A 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Check how Phoenix/Ecto handles UUID casting in queries
rg "Ecto\.UUID\.cast"Repository: appunite/appunite-ksef-ex
Length of output: 875
🏁 Script executed:
# Look for similar UUID validation patterns in the codebase
rg "Ecto\.UUID" -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 50381
🏁 Script executed:
# Check if there are any tests for mount_invoice with invalid UUIDs
rg "mount_invoice" test/ -A 5 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Verify Ecto's casting behavior in queries by checking any error handling
rg "CastError\|cast.*error" lib/ test/ -B 2 -A 2 | head -50Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Check the actual route definition to confirm id is a string param
rg "invoice_live.*show" lib/ksef_hub_web/router.ex -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Verify that Ecto implicitly casts query parameters or if explicit cast is needed
rg "def get_invoice_with_details" lib/ksef_hub/invoices.ex -A 8Repository: appunite/appunite-ksef-ex
Length of output: 740
🏁 Script executed:
# Verify the route definition
fd router.ex | xargs cat | rg "invoice.*show" -A 3 -B 3Repository: appunite/appunite-ksef-ex
Length of output: 640
🏁 Script executed:
# Search for live view routes specifically
rg "invoice_live.*show|live.*invoice.*show" lib/ksef_hub_web/router.ex -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Look for all invoice_live routes
rg "invoice_live" lib/ksef_hub_web/router.ex -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 51
🏁 Script executed:
# Check the mount function of the show.ex live view to see how id parameter flows in
sed -n '1,50p' lib/ksef_hub_web/live/invoice_live/show.ex | cat -nRepository: appunite/appunite-ksef-ex
Length of output: 1773
🏁 Script executed:
# Search for where this LiveView is mounted
rg "invoice_live/show" lib/ -A 2 -B 2Repository: appunite/appunite-ksef-ex
Length of output: 51
Validate the invoice ID before querying in mount_invoice/3.
Passing the raw route parameter directly to get_invoice_with_details can cause an Ecto.CastError when the ID is malformed, crashing the mount instead of redirecting gracefully. The same file already uses Ecto.UUID.cast/1 for this purpose in validate_category_id/1.
🔧 Suggested fix
defp mount_invoice(socket, company, id) do
role = socket.assigns[:current_role]
- case Invoices.get_invoice_with_details(company.id, id, role: role) do
- nil ->
- {:ok,
- socket
- |> put_flash(:error, "Invoice not found.")
- |> redirect(to: ~p"/invoices")}
-
- invoice ->
- {:ok,
- socket
- |> assign(
- page_title: "Invoice #{invoice.invoice_number}",
- invoice: invoice,
- html_preview: generate_preview(invoice),
- categories: Invoices.list_categories(company.id),
- all_tags: Invoices.list_tags(company.id),
- category_form: category_form(invoice),
- new_tag_form: new_tag_form(),
- tag_form_key: 0
- )}
+ with {:ok, uuid} <- Ecto.UUID.cast(id) do
+ case Invoices.get_invoice_with_details(company.id, uuid, role: role) do
+ nil ->
+ {:ok,
+ socket
+ |> put_flash(:error, "Invoice not found.")
+ |> redirect(to: ~p"/invoices")}
+
+ invoice ->
+ {:ok,
+ socket
+ |> assign(
+ page_title: "Invoice #{invoice.invoice_number}",
+ invoice: invoice,
+ html_preview: generate_preview(invoice),
+ categories: Invoices.list_categories(company.id),
+ all_tags: Invoices.list_tags(company.id),
+ category_form: category_form(invoice),
+ new_tag_form: new_tag_form(),
+ tag_form_key: 0
+ )}
+ end
+ else
+ :error ->
+ {:ok,
+ socket
+ |> put_flash(:error, "Invalid invoice ID.")
+ |> redirect(to: ~p"/invoices")}
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| defp mount_invoice(socket, company, id) do | |
| role = socket.assigns[:current_role] | |
| case Invoices.get_invoice_with_details(company.id, id, role: role) do | |
| nil -> | |
| defp mount_invoice(socket, company, id) do | |
| role = socket.assigns[:current_role] | |
| with {:ok, uuid} <- Ecto.UUID.cast(id) do | |
| case Invoices.get_invoice_with_details(company.id, uuid, role: role) do | |
| nil -> | |
| {:ok, | |
| socket | |
| |> put_flash(:error, "Invoice not found.") | |
| |> redirect(to: ~p"/invoices")} | |
| invoice -> | |
| {:ok, | |
| socket | |
| |> assign( | |
| page_title: "Invoice #{invoice.invoice_number}", | |
| invoice: invoice, | |
| html_preview: generate_preview(invoice), | |
| categories: Invoices.list_categories(company.id), | |
| all_tags: Invoices.list_tags(company.id), | |
| category_form: category_form(invoice), | |
| new_tag_form: new_tag_form(), | |
| tag_form_key: 0 | |
| )} | |
| end | |
| else | |
| :error -> | |
| {:ok, | |
| socket | |
| |> put_flash(:error, "Invalid invoice ID.") | |
| |> redirect(to: ~p"/invoices")} | |
| 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/invoice_live/show.ex` around lines 40 - 44,
mount_invoice/3 currently passes the raw route id into
Invoices.get_invoice_with_details which can raise Ecto.CastError for malformed
IDs; validate the id first by using Ecto.UUID.cast(id) (same approach as
validate_category_id/1) and if the cast returns :error or nil perform the same
graceful redirect/handle_error flow you use elsewhere (assigning flash/redirect
or returning {:halt, socket} as appropriate), otherwise call
Invoices.get_invoice_with_details with the validated UUID; reference
mount_invoice/3, Invoices.get_invoice_with_details and Ecto.UUID.cast/1 when
making the change.
Comment on lines
+111
to
+116
| reloaded = reload_details(updated, socket) | ||
|
|
||
| case Invoices.mark_prediction_manual(updated) do | ||
| {:ok, _} -> | ||
| {:noreply, assign(socket, invoice: reloaded, category_form: category_form(reloaded))} | ||
|
|
There was a problem hiding this comment.
Reload occurs before manual-status update, leaving stale prediction_status in assigns.
Line 111 reloads first, then Line 113 updates prediction status. On success, the assigned invoice can still show the old status.
🔧 Suggested fix
- with :ok <- validate_category_id(category_id),
- {:ok, updated} <- Invoices.set_invoice_category(socket.assigns.invoice, category_id) do
- reloaded = reload_details(updated, socket)
-
- case Invoices.mark_prediction_manual(updated) do
- {:ok, _} ->
- {:noreply, assign(socket, invoice: reloaded, category_form: category_form(reloaded))}
+ with :ok <- validate_category_id(category_id),
+ {:ok, updated} <- Invoices.set_invoice_category(socket.assigns.invoice, category_id) do
+ case Invoices.mark_prediction_manual(updated) do
+ {:ok, _} ->
+ reloaded = reload_details(updated, socket)
+ {:noreply, assign(socket, invoice: reloaded, category_form: category_form(reloaded))}🤖 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 111 - 116, The
reload is happening before updating the manual prediction status, causing stale
prediction_status in assigns; change the flow in the show LiveView so you call
Invoices.mark_prediction_manual(updated) first and only on {:ok, _} call
reload_details(updated, socket) and then assign the freshly reloaded invoice and
category_form(reloaded) to socket (update the block around
Invoices.mark_prediction_manual/1, reload_details/2, assign/2 and ensure
prediction_status from the reloaded invoice is used).
Comment on lines
+138
to
+154
| def handle_event("toggle_tag", %{"tag-id" => tag_id}, socket) do | ||
| if Enum.any?(socket.assigns.all_tags, &(&1.id == tag_id)) do | ||
| invoice = socket.assigns.invoice | ||
| currently_assigned = tag_assigned?(invoice, tag_id) | ||
|
|
||
| result = | ||
| if currently_assigned, | ||
| do: Invoices.remove_invoice_tag(invoice.id, tag_id), | ||
| else: Invoices.add_invoice_tag(invoice.id, tag_id, invoice.company_id) | ||
|
|
||
| case result do | ||
| {:ok, _} -> | ||
| {:noreply, assign(socket, :invoice, reload_details(invoice, socket))} | ||
|
|
||
| {:error, _} -> | ||
| {:noreply, put_flash(socket, :error, "Failed to update tags.")} | ||
| end |
There was a problem hiding this comment.
Tag mutations should also mark prediction status as manual.
Both tag success paths change classification but never call Invoices.mark_prediction_manual/1, so the invoice may remain in a non-manual prediction state after user overrides.
🔧 Suggested fix
case result do
{:ok, _} ->
- {:noreply, assign(socket, :invoice, reload_details(invoice, socket))}
+ _ = Invoices.mark_prediction_manual(invoice)
+ {:noreply, assign(socket, :invoice, reload_details(invoice, socket))}
{:error, _} ->
{:noreply, put_flash(socket, :error, "Failed to update tags.")}
end
@@
case Invoices.create_and_add_tag(invoice.id, company_id, %{name: name}) do
{:ok, _tag} ->
+ _ = Invoices.mark_prediction_manual(invoice)
{:noreply,
socket
|> assign(
invoice: reload_details(invoice, socket),Also applies to: 174-183
🤖 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 138 - 154, In the
handle_event "toggle_tag" code path (the function handling tag toggles in
invoice_live/show.ex), after a successful tag addition or removal (the {:ok, _}
branch where you call Invoices.add_invoice_tag/3 or
Invoices.remove_invoice_tag/2 and then reload details), also call
Invoices.mark_prediction_manual(invoice.id) so the invoice prediction is marked
manual; update both occurrences of this success branch (the other similar block
around the second toggle handler) to call Invoices.mark_prediction_manual/1
before assigning the reloaded invoice to socket and returning {:noreply, ...}.
Comment on lines
+788
to
+794
| Repo.transaction(fn -> | ||
| with {:ok, tag} <- create_tag(company_id, attrs), | ||
| {:ok, _it} <- add_invoice_tag(invoice_id, tag.id, company_id) do | ||
| tag | ||
| else | ||
| {:error, reason} -> Repo.rollback(reason) | ||
| end |
There was a problem hiding this comment.
Enforce invoice/company consistency inside create_and_add_tag/3.
Line 790 uses caller-provided company_id to validate tag ownership, but never verifies that invoice_id belongs to the same company. This can create cross-company tag associations if mismatched IDs reach this API.
🔧 Suggested fix
def create_and_add_tag(invoice_id, company_id, attrs) do
Repo.transaction(fn ->
- with {:ok, tag} <- create_tag(company_id, attrs),
+ with true <-
+ Invoice
+ |> where([i], i.id == ^invoice_id and i.company_id == ^company_id)
+ |> Repo.exists?(),
+ {:ok, tag} <- create_tag(company_id, attrs),
{:ok, _it} <- add_invoice_tag(invoice_id, tag.id, company_id) do
tag
else
+ false -> Repo.rollback(:invoice_not_in_company)
{:error, reason} -> Repo.rollback(reason)
end
end)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Repo.transaction(fn -> | |
| with {:ok, tag} <- create_tag(company_id, attrs), | |
| {:ok, _it} <- add_invoice_tag(invoice_id, tag.id, company_id) do | |
| tag | |
| else | |
| {:error, reason} -> Repo.rollback(reason) | |
| end | |
| Repo.transaction(fn -> | |
| with true <- | |
| Invoice | |
| |> where([i], i.id == ^invoice_id and i.company_id == ^company_id) | |
| |> Repo.exists?(), | |
| {:ok, tag} <- create_tag(company_id, attrs), | |
| {:ok, _it} <- add_invoice_tag(invoice_id, tag.id, company_id) do | |
| tag | |
| else | |
| false -> Repo.rollback(:invoice_not_in_company) | |
| {:error, reason} -> Repo.rollback(reason) | |
| end | |
| end) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` around lines 788 - 794, In create_and_add_tag/3
wrap the add flow with an explicit invoice/company consistency check: inside the
Repo.transaction block (where create_tag and add_invoice_tag are called) load
the invoice by invoice_id (e.g., via Repo.get or the existing get_invoice
function), verify its company_id matches the caller-provided company_id, and if
not return {:error, :invoice_company_mismatch} (or Repo.rollback with a clear
reason) before calling add_invoice_tag; keep the existing create_tag and
add_invoice_tag calls and only proceed when the invoice belongs to the same
company.
This was referenced Feb 25, 2026
Merged
Merged
Merged
This was referenced Mar 3, 2026
Merged
Merged
This was referenced Mar 13, 2026
This was referenced Apr 1, 2026
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
Bug Fixes
Tests
Chores