feat(tagger): Leaderboard Fix & Enhancements #548
Conversation
✅ Deploy Preview for btcmap ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
- Fixed tagger filter, which had broken at some point. - Enhanced tagger filter with notes for clarity. - Moved to sortable & searchable TanStack table like the other leaderboards. - Added different date range options.
…dback when switching dates.
escapedcat
force-pushed
the
tagger-leadboard-fix-enhance
branch
from
f26cddb to
fb9737d
Compare
|
Rebased this hot mess with |
escapedcat
changed the title
There was a problem hiding this comment.
Pull request overview
This PR enhances the tagger leaderboard with filtering, period selection, and pagination by migrating to TanStack table throughout the tagger-related leaderboards. The changes align the tagger leaderboard with the patterns already used in the area leaderboard, providing a consistent user experience across all leaderboard pages.
Key Changes:
- Migrated tagger leaderboard and profile activity tables to TanStack table with sorting, searching, and pagination
- Added date range filtering (3/6/12 months, all-time) to the main tagger leaderboard
- Enhanced excluded tagger documentation with explanatory notes and modified the exclusion list
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/routes/tagger/[id]/components/ProfileActivity.svelte |
Migrated from simple pagination to TanStack table with search, sort, and filter capabilities |
src/routes/leaderboard/+page.svelte |
Complete rewrite to use TanStack table pattern, added period selection dropdown, replaced skeleton loading with icon-based loading states |
src/routes/leaderboard/+page.server.ts |
Added period-based filtering with configurable date ranges (3-months, 6-months, 12-months, all-time) |
src/lib/store.ts |
Enhanced excluded tagger list with descriptive notes; removed 3 IDs and added 1 new exclusion |
src/components/leaderboard/TaggerLeaderboardMobileCard.svelte |
New component for mobile-optimized tagger leaderboard cards with responsive layout |
src/components/leaderboard/TaggerLeaderboardDesktopTable.svelte |
New component for desktop tagger leaderboard table with sortable columns |
src/routes/leaderboard/components/LeaderboardItem.svelte |
Minor style cleanup removing lg:inline class from Tip component |
| <div | ||
| class="flex items-center justify-between border-t border-gray-300 px-5 py-3 dark:border-white/95" | ||
| class="flex w-full flex-col gap-5 px-5 pt-2.5 pb-5 text-primary md:flex-row md:items-center md:justify-between dark:text-white" | ||
| > | ||
| <div class="text-sm text-body dark:text-white"> | ||
| Page {currentPage} of {totalPages} ({eventElements.length} total) | ||
| </div> | ||
| <div class="flex space-x-2"> | ||
| <button | ||
| on:click={() => (currentPage = Math.max(1, currentPage - 1))} | ||
| disabled={currentPage === 1} | ||
| class="rounded border border-gray-300 px-3 py-1 text-sm | ||
| text-primary transition-colors | ||
| hover:bg-gray-50 disabled:cursor-not-allowed disabled:opacity-50 | ||
| dark:border-white/95 dark:text-white dark:hover:bg-white/5" | ||
| > | ||
| Previous | ||
| </button> | ||
| <button | ||
| on:click={() => (currentPage = Math.min(totalPages, currentPage + 1))} | ||
| disabled={currentPage === totalPages} | ||
| class="rounded border border-gray-300 px-3 py-1 text-sm | ||
| text-primary transition-colors | ||
| hover:bg-gray-50 disabled:cursor-not-allowed disabled:opacity-50 | ||
| dark:border-white/95 dark:text-white dark:hover:bg-white/5" | ||
| > | ||
| Next | ||
| </button> | ||
| <select | ||
| value={$table?.getState().pagination.pageSize} | ||
| on:change={(e) => { | ||
| // @ts-expect-error Select onChange event target type assertion for page size change | ||
| $table?.setPageSize(Number(e.target?.value)); | ||
| }} | ||
| class="cursor-pointer bg-transparent focus:outline-primary dark:focus:outline-white" | ||
| aria-label="Items per page" | ||
| > | ||
| {#each pageSizes as pageSize (pageSize)} | ||
| <option value={pageSize}> | ||
| Show {pageSize} | ||
| </option> | ||
| {/each} | ||
| </select> | ||
|
|
||
| <div class="flex flex-col gap-5 md:flex-row md:items-center"> | ||
| <div class="flex items-center justify-between gap-5 md:justify-start"> | ||
| <div class="flex items-center gap-5"> | ||
| <button | ||
| type="button" | ||
| class="text-xl font-bold {!$table?.getCanPreviousPage() | ||
| ? 'cursor-not-allowed opacity-50' | ||
| : ''}" | ||
| on:click={() => $table?.firstPage()} | ||
| disabled={!$table?.getCanPreviousPage()} | ||
| aria-label="Go to first page" | ||
| > | ||
| << | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="text-xl font-bold {!$table?.getCanPreviousPage() | ||
| ? 'cursor-not-allowed opacity-50' | ||
| : ''}" | ||
| on:click={() => $table?.previousPage()} | ||
| disabled={!$table?.getCanPreviousPage()} | ||
| aria-label="Go to previous page" | ||
| > | ||
| < | ||
| </button> | ||
| </div> | ||
| <div class="flex items-center gap-5"> | ||
| <button | ||
| type="button" | ||
| class="text-xl font-bold {!$table?.getCanNextPage() | ||
| ? 'cursor-not-allowed opacity-50' | ||
| : ''}" | ||
| on:click={() => $table?.nextPage()} | ||
| disabled={!$table?.getCanNextPage()} | ||
| aria-label="Go to next page" | ||
| > | ||
| > | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="text-xl font-bold {!$table?.getCanNextPage() | ||
| ? 'cursor-not-allowed opacity-50' | ||
| : ''}" | ||
| on:click={() => $table?.lastPage()} | ||
| disabled={!$table?.getCanNextPage()} | ||
| aria-label="Go to last page" | ||
| > | ||
| >> | ||
| </button> | ||
| </div> | ||
| </div> | ||
|
|
||
| <span class="flex items-center justify-center gap-1 md:justify-start" aria-live="polite"> | ||
| <div>Page</div> | ||
| <strong> | ||
| {$table?.getState().pagination.pageIndex + 1} of | ||
| {$table?.getPageCount().toLocaleString()} | ||
| </strong> | ||
| </span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The pagination UI (lines 295-374) duplicates the exact same logic that exists in the LeaderboardPagination.svelte component. The component is already being used elsewhere in the codebase (e.g., in AreaLeaderboard.svelte and in the main leaderboard page at line 436).
For consistency and maintainability, consider extracting this pagination section to use the LeaderboardPagination component instead:
<LeaderboardPagination table={$table} {pageSizes} />This would reduce code duplication and ensure consistent pagination behavior across all leaderboard tables. The only potential difference is the conditional rendering ({#if $table.getFilteredRowModel().rows.length > 0}) which could be handled by the component itself or kept as a wrapper.
| const columns: ColumnDef<ActivityEvent>[] = [ | ||
| { | ||
| id: 'location', | ||
| header: 'Location', | ||
| accessorFn: (row) => row.location, | ||
| enableSorting: false, | ||
| filterFn: fuzzyFilter, | ||
| enableGlobalFilter: true, | ||
| cell: ({ row }) => { | ||
| const event = row.original; | ||
| return ` | ||
| <a href="${resolve(`/merchant/${event.merchantId}`)}" class="text-link transition-colors hover:text-hover"> | ||
| ${event.location} | ||
| </a> | ||
| `; | ||
| } | ||
| }, | ||
| { | ||
| id: 'type', | ||
| header: 'Action', | ||
| accessorFn: (row) => row.type, | ||
| enableSorting: true, | ||
| filterFn: fuzzyFilter, | ||
| enableGlobalFilter: true, | ||
| cell: ({ row }) => { | ||
| const event = row.original; | ||
| const colorClass = | ||
| event.type === 'create' | ||
| ? 'bg-green-100 text-green-800 dark:bg-green-900 dark:text-green-200' | ||
| : event.type === 'update' | ||
| ? 'bg-blue-100 text-blue-800 dark:bg-blue-900 dark:text-blue-200' | ||
| : 'bg-red-100 text-red-800 dark:bg-red-900 dark:text-red-200'; | ||
| return ` | ||
| <span class="inline-flex items-center rounded-full px-2 py-1 text-xs font-medium ${colorClass}"> | ||
| ${event.type} | ||
| </span> | ||
| `; | ||
| } | ||
| }, | ||
| { | ||
| id: 'created_at', | ||
| header: 'Date', | ||
| accessorFn: (row) => row.created_at, | ||
| enableSorting: true, | ||
| filterFn: fuzzyFilter, | ||
| enableGlobalFilter: true, | ||
| cell: ({ row }) => { | ||
| return format(new Date(row.original.created_at), 'MMM d, yyyy HH:mm'); | ||
| } | ||
| } | ||
| ]; |
There was a problem hiding this comment.
The column definitions use a cell property that returns HTML strings, but this pattern is inconsistent with how TanStack table is used elsewhere in the codebase. In AreaLeaderboard.svelte, the cell property returns either primitives (for flexRender) or the row object itself (to be rendered in the template).
The current implementation returns HTML strings from the cell functions, but the template doesn't use flexRender or {@html} to render them - instead, it reimplements the same logic inline in the template (lines 264-284). This makes the cell definitions unused and redundant.
Recommendation: Either:
- Remove the
cellproperty from these column definitions (since the rendering is done inline in the template), OR - Use
flexRenderin the template and keep thecelldefinitions, similar toAreaLeaderboardDesktopTable.svelte
For consistency with the existing codebase pattern, option 1 (removing unused cell properties) is preferable.
src/routes/leaderboard/+page.svelte
Outdated
| return users | ||
| .filter((user) => !excluded.has(user.id)) | ||
| .map((user) => { | ||
| const avatar = user.image_url || '/images/satoshi-nakamoto.png'; |
There was a problem hiding this comment.
[nitpick] The avatar fallback uses /images/satoshi-nakamoto.png in the normalizeUsers function, but the fallback image path in the old LeaderboardItem.svelte component (line 49) also uses this path. However, there's an inconsistency in how the avatar is initially set.
In line 109, when user.image_url is falsy, the avatar is set to /images/satoshi-nakamoto.png. But in the original code (old leaderboard, not shown in diff), it was set to the string 'satoshi' which would need to be transformed elsewhere.
This is actually an improvement - the new code correctly sets the full path. However, there's still a fallback in the on:error handlers (lines 48-49 in TaggerLeaderboardMobileCard, lines 96-97 in TaggerLeaderboardDesktopTable) that sets target.src = '/images/satoshi-nakamoto.png'. This redundant fallback is fine as defensive programming but creates duplication.
Consider whether the initial normalization fallback is sufficient, or if the on:error handler should remain for network issues.
|
Have a look if you can tackle any of copilots comments. I can have a closer look as well when I find some time. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
On it. Thanks for unbunging it! Co-pilot PR review is v good. |
Co-authored-by: Copilot <[email protected]>
Removed unused import of 'resolve' from paths.
|
Let me know if I can help with any of this |
Tests are passing locally and so I'm in some env mismatch hell or something. 🫠 Would appreciate walking through it when you're back so I can learn. |
Fixes: #471
Fixed tagger filter, which had broken at some point.
Enhanced tagger filter with notes for clarity.
Moved to sortable & searchable TanStack table like the other leaderboards.
Added different date range options.
Additionally, apply TanStack to the Tagger activity pane too.