Feat/invoice pagination#37
Merged
Merged
Conversation
Add pg_trgm extension and indexes to support paginated invoice queries: - Compound index on (company_id, issue_date, inserted_at) for default sort - Compound index on (company_id, type, status) for filtered listings - GIN trigram indexes on invoice_number, seller_name, buyer_name for ILIKE Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Exclude xml_content from list queries via @list_fields select - Add LIMIT/OFFSET pagination with :page and :per_page filters - Add count_invoices/2 for counting with same filter logic - Add list_invoices_paginated/2 returning entries + metadata map - Add extract_pagination/1 helper with clamping (per_page max 100) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create PaginationMeta OpenAPI schema for page/per_page/total metadata - Update InvoiceListResponse to include meta alongside data - Add page and per_page query parameters to index operation spec - Controller index action now calls list_invoices_paginated/2 - Add maybe_put_integer/3 helper for parsing pagination params Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use list_invoices_paginated/2 instead of list_invoices/2 - Assign page, per_page, total_count, total_pages to socket - Parse page param from URL, filter change resets to page 1 - Render DaisyUI join button group: Prev / page numbers / Next - Show "Showing X-Y of Z invoices" text - visible_pages/2: show all when <= 7, otherwise 5-page window - pagination_params/2: preserves current filters with target page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Context tests: - Pagination defaults (page 1, per_page 25) - page/per_page respected, pages don't overlap - per_page capped at 100 - xml_content excluded from list results - count_invoices/2 with company scoping and filters - list_invoices_paginated/2 returns correct metadata API controller tests: - Paginated response includes meta key - Default pagination (page 1, per_page 25) - Custom page/per_page params work - xml_content not in list response LiveView tests: - Pagination controls render when >1 page - No pagination controls for single page - Page 2 navigation shows correct range - Filter change resets to page 1 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents decisions on offset-based pagination, two-query approach, pg_trgm extension for ILIKE search, and xml_content exclusion from list queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughImplements offset-based pagination for invoices across API and LiveView, returns data plus pagination meta, excludes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Invoices as InvoicesModule
participant DB as Database
Client->>Controller: GET /api/invoices?page=2&per_page=10
Controller->>Controller: parse page/per_page into filters
Controller->>Invoices: list_invoices_paginated(company_id, filters)
Invoices->>Invoices: extract_pagination / clamp values
Invoices->>DB: COUNT(*) with filters
DB-->>Invoices: total_count
Invoices->>DB: SELECT limited fields LIMIT 10 OFFSET 10
DB-->>Invoices: entries
Invoices-->>Controller: %{entries, page, per_page, total_count, total_pages}
Controller-->>Client: JSON {data: entries, meta: {page, per_page, total_count, total_pages}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 313-326: The `@spec` for maybe_put_integer is too narrow; update it
to reflect that the third argument can be an integer as well as a string or nil.
Change the `@spec` for maybe_put_integer/3 to something like
maybe_put_integer(map(), atom(), String.t() | integer() | nil) :: map() so it
matches the clauses in the function (see maybe_put_integer/3).
🧹 Nitpick comments (5)
priv/repo/migrations/20260212000001_add_invoice_pagination_indexes.exs (2)
7-10: Consider adding explicit column order for DESC sorting.The default ORDER BY in
list_invoices/2usesdesc: i.issue_date, desc: i.inserted_at. B-tree indexes scan backwards for DESC but are more efficient when column order matches the query. Consider:CREATE INDEX ... ON invoices (company_id, issue_date DESC, inserted_at DESC)♻️ Proposed fix
- create index(:invoices, [:company_id, :issue_date, :inserted_at], - name: :invoices_company_date_idx - ) + execute(""" + CREATE INDEX invoices_company_date_idx + ON invoices (company_id, issue_date DESC, inserted_at DESC) + """)And update
down/0:- drop_if_exists index(:invoices, [:company_id, :issue_date, :inserted_at], - name: :invoices_company_date_idx - ) + execute("DROP INDEX IF EXISTS invoices_company_date_idx")
44-44: Dropping pg_trgm extension may affect other features.If
pg_trgmis used by other parts of the application or added by a different migration, dropping it here could cause issues during rollback. Consider removing this line or checking if other dependencies exist.♻️ Safer rollback
- execute("DROP EXTENSION IF EXISTS pg_trgm") + # Note: pg_trgm extension left in place as it may be used by other featureslib/ksef_hub/invoices.ex (1)
78-83: Minor redundancy:extract_paginationis called twice.
list_invoices_paginated/2callsextract_pagination(filters)on line 79, then merges the result back into filters on line 80, which is then passed tolist_invoices/2whereextract_paginationis called again (line 34). The merge is unnecessary sincelist_invoicesre-extracts pagination.This doesn't cause bugs but adds slight overhead.
♻️ Remove redundant merge
def list_invoices_paginated(company_id, filters \\ %{}) do {page, per_page} = extract_pagination(filters) - filters = Map.merge(filters, %{page: page, per_page: per_page}) - entries = list_invoices(company_id, filters) + entries = list_invoices(company_id, Map.put(filters, :page, page) |> Map.put(:per_page, per_page)) total_count = count_invoices(company_id, filters)Or simply pass filters unchanged since
list_invoiceshandles extraction.lib/ksef_hub_web/live/invoice_live/index.ex (2)
93-101: Missing@specformaybe_put_page.For consistency with the other
maybe_put_*helpers and to comply with coding guidelines requiring type specifications for all functions, consider adding a spec.📝 Proposed fix
+ `@spec` maybe_put_page(map(), atom(), String.t() | nil) :: map() defp maybe_put_page(map, _key, nil), do: map defp maybe_put_page(map, _key, ""), do: map
107-130: Consider using a pipeline for cleaner construction.The function is correct but verbose. A more idiomatic approach would use pipelines with
maybe_puthelpers. This is optional since the current implementation works correctly.♻️ Proposed refactor using pipelines
`@spec` pagination_params(map(), pos_integer()) :: map() defp pagination_params(filters, target_page) do - params = %{} - params = if filters[:type], do: Map.put(params, "type", filters[:type]), else: params - params = if filters[:status], do: Map.put(params, "status", filters[:status]), else: params - - params = - if filters[:date_from], - do: Map.put(params, "date_from", Date.to_iso8601(filters[:date_from])), - else: params - - params = - if filters[:date_to], - do: Map.put(params, "date_to", Date.to_iso8601(filters[:date_to])), - else: params - - params = if filters[:query], do: Map.put(params, "query", filters[:query]), else: params - - if target_page > 1 do - Map.put(params, "page", Integer.to_string(target_page)) - else - params - end + %{} + |> maybe_put("type", filters[:type]) + |> maybe_put("status", filters[:status]) + |> maybe_put("date_from", filters[:date_from] && Date.to_iso8601(filters[:date_from])) + |> maybe_put("date_to", filters[:date_to] && Date.to_iso8601(filters[:date_to])) + |> maybe_put("query", filters[:query]) + |> maybe_put("page", if(target_page > 1, do: Integer.to_string(target_page))) end
- Use explicit DESC column order in pagination index for efficient scans - Keep pg_trgm extension on rollback to avoid breaking other features - Remove redundant pagination merge in list_invoices_paginated/2 - Add missing @SPEC for maybe_put_page/3 in LiveView - Widen maybe_put_integer/3 spec to accept integer values - Refactor pagination_params/2 to use pipeline with maybe_put/3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/invoices.ex`:
- Around line 284-289: Update the `@spec` for clamp to reflect that the first
argument can be any term (not only integer), e.g. change the head spec from
clamp(integer(), integer(), integer()) :: integer() to clamp(term(), integer(),
integer()) :: integer() so it matches both the guarded clause (defp clamp(value,
min_val, max_val) when is_integer(value)) and the fallback clause (defp clamp(_,
min_val, _max_val)), which returns an integer.
🧹 Nitpick comments (1)
lib/ksef_hub/invoices.ex (1)
78-83: Minor: Redundant pagination extraction.
extract_paginationis called here at line 79, thenlist_invoicesat line 81 calls it again internally. Consider passing the already-extracted values to avoid the duplicate computation, or accept thatlist_invoicesshould remain self-contained.♻️ Optional: Add internal helper to avoid duplication
def list_invoices_paginated(company_id, filters \\ %{}) do {page, per_page} = extract_pagination(filters) - entries = list_invoices(company_id, filters) + entries = do_list_invoices(company_id, filters, page, per_page) total_count = count_invoices(company_id, filters) total_pages = max(ceil(total_count / per_page), 1) # ... end + defp do_list_invoices(company_id, filters, page, per_page) do + Invoice + |> where([i], i.company_id == ^company_id) + |> apply_filters(filters) + |> order_by([i], desc: i.issue_date, desc: i.inserted_at) + |> select([i], struct(i, ^@list_fields)) + |> limit(^per_page) + |> offset(^((page - 1) * per_page)) + |> Repo.all() + end
…action Also widen clamp/3 spec to accept term() matching its fallback clause. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 23, 2026
Merged
Merged
This was referenced Mar 4, 2026
This was referenced Mar 12, 2026
Merged
This was referenced Mar 23, 2026
This was referenced Mar 31, 2026
This was referenced Apr 15, 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
Performance
Tests
Documentation