Skip to content

feat(vps): port admin to HttpApiBuilder.group (Step 6)#163

Merged
guidefari merged 3 commits into
migration/effect-http-apifrom
migration/6-admin-group
Jul 11, 2026
Merged

feat(vps): port admin to HttpApiBuilder.group (Step 6)#163
guidefari merged 3 commits into
migration/effect-http-apifrom
migration/6-admin-group

Conversation

@guidefari

@guidefari guidefari commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Why

Step 6 continues: port admin (GET /api/admin/overview,
GET /api/admin/frontend-errors/:scenario, GET /api/admin/newsletter-subscribers)
from the Hono fallback onto the Effect router.

What to look for

  • packages/api/src/admin.ts -- effect@4.0.0-beta.93's HttpApiError has no
    built-in 429. The frontend error simulator (apps/www/src/routes/admin/frontend-errors.tsx,
    a QA tool exercising www's error boundary and the res.status >= 500
    Sentry-reporting threshold in http-client.ts against every real status
    code) needs one, so SimulatedRateLimitError is declared the same way
    Phase 2 of the migration doc ports any other domain error
    (Schema.TaggedErrorClass + httpApiStatus: 429).
  • The other simulated-error branches (bad-request/not-found/error/unavailable)
    use the built-in HttpApiError classes directly, which are bodyless, rather
    than preserving the old Hono handler's { error, scenario } response body.
    Confirmed via apps/www/src/lib/http-client.ts that the frontend only
    branches on res.status, not the response body shape, so this is a safe
    simplification, not a silent behavior loss.
  • All three endpoints are gated by .middleware(AuthMiddleware) plus an
    in-handler requireAdmin role check (apps/vps/src/http/admin.handlers.ts),
    matching the pattern already used for music-artists' write endpoints
    (music.handlers.ts's requireAdmin).
  • Auth-gating order matters and is counter-intuitive: AuthMiddleware runs
    before param schema validation, so an invalid :scenario literal still
    401s (not 400) when there's no session cookie -- verified empirically via a
    new blackbox test, not assumed from endpoint declaration order (my first
    draft of that test asserted 400 and was wrong).
  • getAdminOverview's ~30-query aggregation batch has no dedicated service
    layer, same as the old Hono handler -- ported as one Effect.tryPromise
    wrapping the whole batch (loadAdminOverview()), matching the old
    handler's single try/catch around one Promise.all rather than converting
    each query to its own tagged-error Effect, which would be new
    failure-isolation behavior, not a faithful port.
  • No apps/www changes -- fetcher() calls the same /api/admin/* paths
    unchanged; this PR only swaps which server-side code answers them.
  • apps/vps/src/db/admin-overview.schema.ts (the old Zod schema) is
    intentionally not deleted, unlike the other superseded Hono files --
    apps/www/src/routes/admin/-overview.data.ts still imports its
    AdminOverview/AdminOverviewContentBreakdown types from
    @gbfm/vps/schemas for useAdminOverview's return type. Nothing in
    apps/vps imports it anymore (admin.handlers.ts's loadAdminOverview()
    was written directly against the real query results, independently of this
    schema, and matches packages/api/src/admin.ts's AdminOverviewResponse
    field-for-field). It's now a type-only shim for apps/www, not a runtime
    schema -- worth retiring once a step-6b PR swaps useAdminOverview onto
    the typed client and can import the type from packages/api/src/admin.ts
    instead. Flagging this explicitly rather than leaving unexplained
    leftover-looking code: an earlier adversarial review pass on this PR
    flagged the file as fully dead and safe to delete, which was wrong --
    it missed this type-only cross-package import.

Test evidence

apps/www's admin overview dashboard and newsletter tab both consume these
endpoints directly (useAdminOverview, useAdminNewsletterSubscribers in
apps/www/src/lib/http.ts / -overview.data.ts), so this isn't
screenshot-exempt. Verified against a real dev server connected to the
actual prod database (read-only GETs only), logged in as a real admin
account:

Admin overview dashboard rendering Total Users/Newsletter/Published Mixes/Total Plays from the new Effect-ported /api/admin/overview endpoint

Newsletter tab summary cards (Active subscribers/New in 30 days/Unsubscribed/Sendable mixes) rendering from the new Effect-ported /api/admin/newsletter-subscribers endpoint

Browser console clean after login (no errors from either page beyond the
expected pre-login 401 on the initial unauthenticated load).

bun run typecheck (vps and api packages): clean.

vitest run --config vitest.unit.config.ts (vps): added 4 new blackbox
cases (401 for all three endpoints without a session cookie, plus the
auth-before-param-validation ordering case above) -- all 4 pass. Pre-existing
DB-connectivity failures (7, same as unmodified migration/effect-http-api:
health/music-artists/search/profile blackbox tests failing 500 instead of
200/404 because the local testcontainers DB isn't reachable in this
environment) are unchanged by this PR.

guidefari added a commit that referenced this pull request Jul 11, 2026
- Stacked PR merged into a feature branch that itself never merged
  forward into migration/effect-http-api (#158/resolve, fixed by #162).
- Adversarial review flagging a file as dead based on a same-package
  grep, missing a cross-package type-only import (#163/admin).
@guidefari

Copy link
Copy Markdown
Owner Author

Fixed: extracted the shared dieOnDatabaseError helper (flagged in review on #162) to apps/vps/src/http/handler-utils.ts and updated admin/search/music/profile handlers to use it instead of each keeping an identical copy. Typecheck clean, same 7 pre-existing DB-connectivity test failures as before (no regressions).

guidefari added a commit that referenced this pull request Jul 11, 2026
Addresses review comment: dieOnDatabaseError was an identical
copy-paste across search/music/profile/resolve handlers, differing only
in the log-tag prefix string. Extracted to
apps/vps/src/http/handler-utils.ts as a tag -> wrapper factory; this PR
updates resolve.handlers.ts to use it. The other three handler files'
duplicate copies are updated on migration/6-admin-group (PR #163), which
also introduces the shared helper -- once both branches merge into
migration/effect-http-api, all four groups share one definition.
Ports GET /api/admin/overview, /api/admin/frontend-errors/:scenario, and
GET /api/admin/newsletter-subscribers from the Hono/zod-openapi router
onto the Effect router. All three are admin-gated via .middleware(AuthMiddleware)
plus an in-handler role check (requireAdmin), matching the pattern already
used for music-artists' write endpoints.

effect@4.0.0-beta.93's HttpApiError has no built-in 429 -- the frontend
error simulator (a QA tool exercising www's error boundary/Sentry-reporting
threshold against every real status code) needs one, so
SimulatedRateLimitError is declared in packages/api/src/admin.ts the same
way Phase 2 of the migration doc ports any other domain error. The other
simulated-error branches use the built-in HttpApiError classes directly
(bodyless) rather than preserving the old handler's { error, scenario }
response body -- confirmed via apps/www/src/lib/http-client.ts that the
frontend only branches on res.status, not response body shape.

getAdminOverview's ~30-query aggregation batch has no dedicated service
layer (same as the old Hono handler); ported as one Effect.tryPromise
wrapping the whole batch, matching the old handler's single try/catch
around one Promise.all rather than converting each query individually.

Deletes the superseded routes/admin/{admin.routes,admin.index,admin.handlers}.ts
Hono trio and its app.ts mount. No apps/www changes -- fetcher() calls the
same /api/admin/* paths unchanged, verified end-to-end against the real
prod-connected dev server (admin overview dashboard + newsletter tab both
render real data through the new handler, logged in via a local dev
account).
- Stacked PR merged into a feature branch that itself never merged
  forward into migration/effect-http-api (#158/resolve, fixed by #162).
- Adversarial review flagging a file as dead based on a same-package
  grep, missing a cross-package type-only import (#163/admin).
Review on #162 flagged dieOnDatabaseError as a likely duplicate --
correct: search, music, profile, and this PR's admin handlers each had
an identical copy differing only in the log-tag prefix string. Extracted
to apps/vps/src/http/handler-utils.ts as a tag -> wrapper factory, with
each handler file partially applying its own tag once at module scope
so call sites stay unchanged (dieOnDatabaseError(effect)).
@guidefari guidefari force-pushed the migration/6-admin-group branch from d879976 to e2c405e Compare July 11, 2026 16:13
@guidefari guidefari merged commit dc5fb09 into migration/effect-http-api Jul 11, 2026
@guidefari guidefari deleted the migration/6-admin-group branch July 11, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant