refactor: pagination and table components #33

Overall, this is a solid refactoring that introduces reusable table and pagination components. The code is clean, passes linting, type checking, and formatting checks. Here are my findings:

🔴 Critical Issue

1. Invalid HTML tag in TableBodyCell.svelte

src/components/table/TableBodyCell.svelte uses <tb> as the wrapper element, which is not valid HTML. This should be <td> (table data cell).

<!-- Current (invalid) -->
<tb>
  {@render children()}
</tb>

<!-- Should be -->
<td>
  {@render children()}
</td>

This is a clear typo that will break table rendering in all browsers.

⚠️ Important Issues

2. Pagination loses sort state on page change

In Pagination.svelte, the onPageChange handler only updates the page number:

onPageChange={async (pageNumber) => {
  await goto(createPaginationUrl(page.url, { page: pageNumber }));
}}

This means when a user sorts a column and then clicks to page 2, the sort is lost. The handler should preserve sortColumn and sortDirection:

onPageChange={async (pageNumber) => {
  await goto(createPaginationUrl(page.url, {
    page: pageNumber,
    sortColumn: paginationOptions.sortColumn,
    sortDirection: paginationOptions.sortDirection,
  }));
}}

3. Sort column names don't match database columns

TableHeadCell references sort columns like "name" and "username", but the database schema uses createdAt (not name or username as direct sort columns in the DB). The PaginationSortColumn type correctly lists ["createdAt", "name", "username"], so this is fine at the type level — but the display label "Creato" in the users page maps to createdAt which is correct. No actual bug here, just noting that the column naming convention is consistent.

💡 Suggestions

4. Table component lacks class prop

The Table component doesn't accept a class prop, so there's no way to apply Tailwind styling. Consider adding one:

interface Props {
  class?: string;
  head: Snippet;
  body: Snippet;
}

5. TableHeadCell generic type parameter is unused

The generics="C extends string" is declared but doesn't add meaningful type safety — the columnName prop is typed as C but there's no constraint enforcement at the call site. Consider simplifying or removing the generic.

6. TableHeadCell URL generation drops sort direction

When clicking a column header to sort by a different column, the URL is generated without the current sortDirection. This means clicking "Name" when already sorted by "Username (asc)" will default to desc (the initial state). This might be intentional behavior, but worth confirming.

📝 Minor / Cosmetic

7. AGENTS.md has a duplicate "Package.json scripts" section

The section appears to have been added while the old "Testing" and "CI/CD" sections were still present. The diff shows the new section was prepended but the old content wasn't fully removed (the resulting file looks correct, but the diff could be cleaner).

8. README.md has trailing whitespace

The project structure diagram has trailing whitespace on some lines (e.g., └── auth/ # Authentication pages).

✅ Positive Notes

The new table components are well-structured and follow Svelte 5 conventions
The paginationSchemaFactory pattern is a nice abstraction for reusable pagination
File reorganization (moving _forms.remote.ts and _schemas.ts) is cleaner
All checks pass (linting, type checking, formatting)
The TableHeadCell with inline sorting support is a great reusable component

Summary

Severity	Count	Key Issue
Critical	1	`<tb>` should be `<td>` in TableBodyCell
Important	1	Pagination loses sort state
Suggestions	4	Various improvements

I'd recommend fixing the <tb> → <td> typo and the pagination sort state issue before merging.

# PR Review: refactor pagination and table components Overall, this is a solid refactoring that introduces reusable table and pagination components. The code is clean, passes linting, type checking, and formatting checks. Here are my findings: ## 🔴 Critical Issue **1. Invalid HTML tag in `TableBodyCell.svelte`** `src/components/table/TableBodyCell.svelte` uses `<tb>` as the wrapper element, which is not valid HTML. This should be `<td>` (table data cell). ```svelte  <tb> {@render children()} </tb>  <td> {@render children()} </td> ``` This is a clear typo that will break table rendering in all browsers. ## ⚠️ Important Issues **2. Pagination loses sort state on page change** In `Pagination.svelte`, the `onPageChange` handler only updates the page number: ```svelte onPageChange={async (pageNumber) => { await goto(createPaginationUrl(page.url, { page: pageNumber })); }} ``` This means when a user sorts a column and then clicks to page 2, the sort is lost. The handler should preserve `sortColumn` and `sortDirection`: ```svelte onPageChange={async (pageNumber) => { await goto(createPaginationUrl(page.url, { page: pageNumber, sortColumn: paginationOptions.sortColumn, sortDirection: paginationOptions.sortDirection, })); }} ``` **3. Sort column names don't match database columns** `TableHeadCell` references sort columns like `"name"` and `"username"`, but the database schema uses `createdAt` (not `name` or `username` as direct sort columns in the DB). The `PaginationSortColumn` type correctly lists `["createdAt", "name", "username"]`, so this is fine at the type level — but the display label `"Creato"` in the users page maps to `createdAt` which is correct. No actual bug here, just noting that the column naming convention is consistent. ## 💡 Suggestions **4. Table component lacks class prop** The `Table` component doesn't accept a `class` prop, so there's no way to apply Tailwind styling. Consider adding one: ```svelte interface Props { class?: string; head: Snippet; body: Snippet; } ``` **5. `TableHeadCell` generic type parameter is unused** The `generics="C extends string"` is declared but doesn't add meaningful type safety — the `columnName` prop is typed as `C` but there's no constraint enforcement at the call site. Consider simplifying or removing the generic. **6. `TableHeadCell` URL generation drops sort direction** When clicking a column header to sort by a *different* column, the URL is generated without the current `sortDirection`. This means clicking "Name" when already sorted by "Username (asc)" will default to `desc` (the initial state). This might be intentional behavior, but worth confirming. ## 📝 Minor / Cosmetic **7. AGENTS.md has a duplicate "Package.json scripts" section** The section appears to have been added while the old "Testing" and "CI/CD" sections were still present. The diff shows the new section was prepended but the old content wasn't fully removed (the resulting file looks correct, but the diff could be cleaner). **8. README.md has trailing whitespace** The project structure diagram has trailing whitespace on some lines (e.g., `└── auth/ # Authentication pages`). ## ✅ Positive Notes - The new table components are well-structured and follow Svelte 5 conventions - The `paginationSchemaFactory` pattern is a nice abstraction for reusable pagination - File reorganization (moving `_forms.remote.ts` and `_schemas.ts`) is cleaner - All checks pass (linting, type checking, formatting) - The `TableHeadCell` with inline sorting support is a great reusable component ## Summary | Severity | Count | Key Issue | |----------|-------|-----------| | Critical | 1 | `<tb>` should be `<td>` in TableBodyCell | | Important | 1 | Pagination loses sort state | | Suggestions | 4 | Various improvements | I'd recommend fixing the `<tb>` → `<td>` typo and the pagination sort state issue before merging.

src/components/navigation/Pagination.svelte

					
				@ -0,0 +21,4 @@

				    count={itemsCount}

				    page={paginationOptions.page}

				    perPage={pageSize}

				    class="flex items-center gap-1 font-semibold text-slate-600"

This should preserve sortColumn and sortDirection to avoid losing sort state on page change

src/components/table/TableBodyCell.svelte Outdated

					
				@ -0,0 +8,4 @@

				  const { children }: Props = $props();

				</script>

				<tb>

Invalid HTML tag - should be , not

Invalid HTML tag - should be <td>, not <tb>

davide added 2 commits

2026-06-29 11:05:56 +00:00

Merge remote-tracking branch 'origin/main' into refactor/pagination-and-table 6f1736f3af

chore: fix lints

check-build.yaml / Check that project builds (pull_request) Successful in 1m2s

Details

check-formatter.yaml / Checks with formatters (pull_request) Successful in 9s

Details

check-linter.yaml / Checks with linters (pull_request) Successful in 11s

Details

check-syntax.yaml / Checks with static tools (pull_request) Successful in 18s

Details

check-yamllint.yaml / Lint yaml files (pull_request) Successful in 2s

Details

10ff282326

davide scheduled this pull request to auto merge when all checks succeed

2026-06-29 11:06:12 +00:00

davide merged commit 0c7b5d3d7a into main

2026-06-29 11:07:46 +00:00

davide deleted branch refactor/pagination-and-table

2026-06-29 11:07:46 +00:00

davide referenced this pull request from a commit

2026-06-29 11:07:47 +00:00

refactor: pagination and table components (#33)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

projects/events-cash-register!33

Rows
Columns