Add account packages route for browsing saved package metadata - #745
Conversation
…inite scroll Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds an authenticated ChangesAccount Packages
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountPackagesRoute
participant accountPackagesRouteLoader
participant createAccountPackagesApiHandler
participant loadAccountPackagesData
Client->>AccountPackagesRoute: navigate or change filters
AccountPackagesRoute->>accountPackagesRouteLoader: load URL state
accountPackagesRouteLoader->>createAccountPackagesApiHandler: GET account packages JSON
createAccountPackagesApiHandler->>loadAccountPackagesData: load authenticated package data
loadAccountPackagesData-->>createAccountPackagesApiHandler: list and selected package
createAccountPackagesApiHandler-->>accountPackagesRouteLoader: JSON payload
accountPackagesRouteLoader-->>AccountPackagesRoute: route loader data
AccountPackagesRoute-->>Client: render list and package details
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔎 Preview deployed: https://kody-pr-745.kody-a99.workers.dev Worker: Mocks:
|
![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
🧹 Nitpick comments (5)
packages/worker/src/package-registry/repo.ts (1)
245-255: 🚀 Performance & Scalability | 🔵 TrivialFull-table scan on every search due to
LOWER(...)on unindexed columns.Every text column (and
tags_json) is wrapped inLOWER(...)for case-insensitive matching, which prevents SQLite from using any index and forces a full scan ofsaved_packagesper query. At current expected per-user row counts this is likely fine, but worth keeping in mind if package counts grow substantially.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-registry/repo.ts` around lines 245 - 255, Update the search condition construction in the query-handling block to avoid wrapping indexed text columns in LOWER(...) for case-insensitive matching; use the schema’s supported case-insensitive comparison or indexed representation while preserving matching across name, kody_id, description, search_text, and tags_json and the existing escaped LIKE parameters.packages/worker/client/routes/account-packages.tsx (1)
319-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffLoad-latch logic duplicates the shared
route-load-latch.tspattern.This render-time block (last-seen/last-failed/needsLoad tracking) closely mirrors the
needsLoad/latch logic already factored out inroute-load-latch.ts(seen in other routes), just keyed ondataKeyinstead ofhrefwith an addedloadingDataKeyguard. Not blocking, but worth considering generalizing the shared helper to accept a configurable key so this route doesn't carry its own copy of the same failure-latch semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/client/routes/account-packages.tsx` around lines 319 - 346, Generalize and reuse the shared route-load-latch helper from route-load-latch.ts for the render-time tracking around applyRouteLoaderData, replacing the local lastSeenDataKey, lastFailedDataKey, and needsLoad logic in the returned function. Extend the helper’s key handling to support dataKey and preserve this route’s loadingDataKey guard and stale-navigation refresh behavior.packages/worker/src/app/account-packages-data.ts (2)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated route base path constant.
accountPackagesBasePathre-hardcodes/account/packages, which already exists asroutes.accountPackagesinpackages/worker/src/app/routes.ts. If the route path ever changes there, this constant silently drifts out of sync.♻️ Proposed fix
+import { routes } from '`#app/routes.ts`' ... -const accountPackagesBasePath = '/account/packages' +const accountPackagesBasePath = routes.accountPackages🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/account-packages-data.ts` at line 20, Replace the hard-coded value in accountPackagesBasePath with the existing routes.accountPackages symbol imported from the routes module, keeping all consumers of accountPackagesBasePath unchanged.
24-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer
routes.accountPackageshere (packages/worker/src/app/account-packages-data.ts:20).accountPackagesBasePathduplicates the shared route string; reuse the exported route constant instead to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/account-packages-data.ts` around lines 24 - 39, Update readAccountPackagesSelectedPackageId to derive detailPrefix from the shared routes.accountPackages constant instead of accountPackagesBasePath, removing the duplicated route-string dependency while preserving the existing URL parsing and selection behavior.packages/worker/src/app/handlers/account-packages.node.test.ts (1)
41-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
createAccountPackagesHandler(SSR page handler) has no test coverage here.
readAuthSessionResult,redirectToLogin,redirectToLoginWhenUnauthenticated, andrenderAppPageare all mocked (Lines 41-52), but onlycreateAccountPackagesApiHandleris imported and tested (Line 61-62). The SSR handler's session/auth redirect logic andrenderAppPageinvocation are never exercised, despite the mocks being purpose-built for that.Consider adding tests for
createAccountPackagesHandlercovering: no session → redirect, no authenticated user → redirect, and happy path →renderAppPagecalled with{ accountPackages }loaderData; or remove the now-unused mocks if SSR coverage is intentionally out of scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/handlers/account-packages.node.test.ts` around lines 41 - 62, The SSR handler createAccountPackagesHandler lacks coverage despite its authentication and rendering dependencies being mocked. Add tests that exercise no session, no authenticated user, and authenticated success paths, verifying redirects and that renderAppPage receives loaderData containing accountPackages; alternatively remove the unused SSR mocks if this handler is intentionally out of scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/client/routes/account-packages.tsx`:
- Around line 605-637: Reorder the items array in the metadata grid so the
3-column row-major layout is: Kody id, Source id, Package id on the first row,
followed by Created, App, Updated on the second row. Keep each existing field’s
label and value unchanged.
---
Nitpick comments:
In `@packages/worker/client/routes/account-packages.tsx`:
- Around line 319-346: Generalize and reuse the shared route-load-latch helper
from route-load-latch.ts for the render-time tracking around
applyRouteLoaderData, replacing the local lastSeenDataKey, lastFailedDataKey,
and needsLoad logic in the returned function. Extend the helper’s key handling
to support dataKey and preserve this route’s loadingDataKey guard and
stale-navigation refresh behavior.
In `@packages/worker/src/app/account-packages-data.ts`:
- Line 20: Replace the hard-coded value in accountPackagesBasePath with the
existing routes.accountPackages symbol imported from the routes module, keeping
all consumers of accountPackagesBasePath unchanged.
- Around line 24-39: Update readAccountPackagesSelectedPackageId to derive
detailPrefix from the shared routes.accountPackages constant instead of
accountPackagesBasePath, removing the duplicated route-string dependency while
preserving the existing URL parsing and selection behavior.
In `@packages/worker/src/app/handlers/account-packages.node.test.ts`:
- Around line 41-62: The SSR handler createAccountPackagesHandler lacks coverage
despite its authentication and rendering dependencies being mocked. Add tests
that exercise no session, no authenticated user, and authenticated success
paths, verifying redirects and that renderAppPage receives loaderData containing
accountPackages; alternatively remove the unused SSR mocks if this handler is
intentionally out of scope.
In `@packages/worker/src/package-registry/repo.ts`:
- Around line 245-255: Update the search condition construction in the
query-handling block to avoid wrapping indexed text columns in LOWER(...) for
case-insensitive matching; use the schema’s supported case-insensitive
comparison or indexed representation while preserving matching across name,
kody_id, description, search_text, and tags_json and the existing escaped LIKE
parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 398665da-a43d-490f-bbf9-7adc89d2ab08
📒 Files selected for processing (11)
packages/worker/client/routes/account-management-components.tsxpackages/worker/client/routes/account-packages.tsxpackages/worker/client/routes/index.tsxpackages/worker/src/app/account-packages-data.tspackages/worker/src/app/handlers/account-packages.node.test.tspackages/worker/src/app/handlers/account-packages.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/package-registry/repo-search.workers.test.tspackages/worker/src/package-registry/repo.ts
| items={[ | ||
| { label: 'Kody id', value: selectedPackage.kodyId }, | ||
| { | ||
| label: 'Package id', | ||
| value: ( | ||
| <code mix={css({ overflowWrap: 'anywhere' })}> | ||
| {selectedPackage.id} | ||
| </code> | ||
| ), | ||
| }, | ||
| { | ||
| label: 'App', | ||
| value: selectedPackage.hasApp | ||
| ? 'Declares a package app' | ||
| : 'No app', | ||
| }, | ||
| { | ||
| label: 'Source id', | ||
| value: ( | ||
| <code mix={css({ overflowWrap: 'anywhere' })}> | ||
| {selectedPackage.sourceId} | ||
| </code> | ||
| ), | ||
| }, | ||
| { | ||
| label: 'Created', | ||
| value: formatTimestamp(selectedPackage.createdAt), | ||
| }, | ||
| { | ||
| label: 'Updated', | ||
| value: formatTimestamp(selectedPackage.updatedAt), | ||
| }, | ||
| ]} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Metadata grid field order doesn't match the intended 3-column layout.
With columns={3}, the grid fills row-major: row 1 = Kody id / Package id / App, row 2 = Source id / Created / Updated. The PR's own reference screenshots/description show row 1 = Kody id / Source id / Package id, row 2 = Created / App / Updated. Reorder the items array to match.
🎨 Proposed fix
<MetadataGrid
columns={3}
items={[
{ label: 'Kody id', value: selectedPackage.kodyId },
- {
- label: 'Package id',
- value: (
- <code mix={css({ overflowWrap: 'anywhere' })}>
- {selectedPackage.id}
- </code>
- ),
- },
- {
- label: 'App',
- value: selectedPackage.hasApp
- ? 'Declares a package app'
- : 'No app',
- },
{
label: 'Source id',
value: (
<code mix={css({ overflowWrap: 'anywhere' })}>
{selectedPackage.sourceId}
</code>
),
},
+ {
+ label: 'Package id',
+ value: (
+ <code mix={css({ overflowWrap: 'anywhere' })}>
+ {selectedPackage.id}
+ </code>
+ ),
+ },
{
label: 'Created',
value: formatTimestamp(selectedPackage.createdAt),
},
+ {
+ label: 'App',
+ value: selectedPackage.hasApp
+ ? 'Declares a package app'
+ : 'No app',
+ },
{
label: 'Updated',
value: formatTimestamp(selectedPackage.updatedAt),
},
]}
/>📝 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.
| items={[ | |
| { label: 'Kody id', value: selectedPackage.kodyId }, | |
| { | |
| label: 'Package id', | |
| value: ( | |
| <code mix={css({ overflowWrap: 'anywhere' })}> | |
| {selectedPackage.id} | |
| </code> | |
| ), | |
| }, | |
| { | |
| label: 'App', | |
| value: selectedPackage.hasApp | |
| ? 'Declares a package app' | |
| : 'No app', | |
| }, | |
| { | |
| label: 'Source id', | |
| value: ( | |
| <code mix={css({ overflowWrap: 'anywhere' })}> | |
| {selectedPackage.sourceId} | |
| </code> | |
| ), | |
| }, | |
| { | |
| label: 'Created', | |
| value: formatTimestamp(selectedPackage.createdAt), | |
| }, | |
| { | |
| label: 'Updated', | |
| value: formatTimestamp(selectedPackage.updatedAt), | |
| }, | |
| ]} | |
| items={[ | |
| { label: 'Kody id', value: selectedPackage.kodyId }, | |
| { | |
| label: 'Source id', | |
| value: ( | |
| <code mix={css({ overflowWrap: 'anywhere' })}> | |
| {selectedPackage.sourceId} | |
| </code> | |
| ), | |
| }, | |
| { | |
| label: 'Package id', | |
| value: ( | |
| <code mix={css({ overflowWrap: 'anywhere' })}> | |
| {selectedPackage.id} | |
| </code> | |
| ), | |
| }, | |
| { | |
| label: 'Created', | |
| value: formatTimestamp(selectedPackage.createdAt), | |
| }, | |
| { | |
| label: 'App', | |
| value: selectedPackage.hasApp | |
| ? 'Declares a package app' | |
| : 'No app', | |
| }, | |
| { | |
| label: 'Updated', | |
| value: formatTimestamp(selectedPackage.updatedAt), | |
| }, | |
| ]} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/client/routes/account-packages.tsx` around lines 605 - 637,
Reorder the items array in the metadata grid so the 3-column row-major layout
is: Kody id, Source id, Package id on the first row, followed by Created, App,
Updated on the second row. Keep each existing field’s label and value unchanged.
There was a problem hiding this comment.
The current order is intentional — the screenshots in the PR description were rendered by this exact code, so there's no divergent "intended" layout to match. Row 1 groups the identifiers and app declaration (Kody id / Package id / App), row 2 groups provenance (Source id / Created / Updated). Skipping the reorder.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
3 participants
What
Adds
/account/packages— a read-only account section for browsing your saved packages and their metadata, following the same list/detail layout as/account/secrets, with server-backed search, filters, sort, and infinite scroll (same pattern as/admin/users).qmatches name, kody id, description, search text, and tags), an app filter (all / with app / without app), and a sort select (recently updated — default, recently created, name). Server-paginated with an infinite-scroll sentinel plus a "Load more" fallback button and a "Showing X of Y" footer./account/packages/:packageIdshowing name, description, tag chips, kody id, package id, app declaration, source id, created/updated timestamps, and search text.searchSavedPackagesByUserIdrepo query (user-scoped, shared WHERE clause for page + total, LIKE-escaped search).Packagesentry in the account section nav.How it works
routes.ts:accountPackages,accountPackageDetail,accountPackagesApi(/account/packages.json, GET-only — this surface is read-only; packages are created/edited through MCP tools).account-packages-data.ts+handlers/account-packages.tsmirror the secrets/invocation-tokens handler pattern (session auth → load data →renderAppPage/ JSON).client/routes/account-packages.tsxblends the secrets layout (path detail, URL-backed filters) with the admin-users infinite list (createInfiniteList+infiniteScrollSentinel).Testing
npm run validatepasses (format, lint, typecheck, unit tests, Playwright E2E, MCP E2E).account-packages.node.test.ts: API handler pagination/filter/sort/selection parsing, auth, and method guards.repo-search.workers.test.ts: real-D1 coverage of the search SQL — user scoping, sort orders, case-insensitive multi-column matching, LIKE wildcard escaping, app filter, and paging totals.discord→ 3), app filter (→ 12), name sort, direct full-page detail loads, and the "Package not found." state (see video and screenshots above).System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@18f67ce3· Head:f5f05073Classification: extends — adds a new account route to the browser app and a new user-scoped query to the saved-packages data layer. No new primitives, no schema changes.
Primitives touched
app-ui/account/packages(.json)routes, handler, loader data, client route, navsaved-packagessearchSavedPackagesByUserIdfiltered/paged query in the registry repod1-app-dbsaved_packagestableSystem map
The packages account page flows from the browser app through the saved-packages registry repo into D1.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Per-user isolation: every query in
searchSavedPackagesByUserIdand the detail lookup is scoped byuser_id = ?from the authenticated session; the count and page share one WHERE clause.Summary by CodeRabbit
New Features
Bug Fixes
Tests