Skip to content

Fix 500 on /publisher/[id] when a D1 read fails - #11

Merged
ClausValcaTD merged 2 commits into
mainfrom
fix/publisher-route-500
Nov 3, 2026
Merged

ClausValcaTD merged 2 commits into
mainfrom
fix/publisher-route-500

Conversation

@ClausValcaTD

@ClausValcaTD ClausValcaTD commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

What changed

/publisher/<id> returned an unhandled HTTP 500. The page called getPublisherProfile() with no try/catch, so when D1 threw, the exception escaped the frontmatter and Cloudflare returned a 500. / stayed 200, which pointed at this route specifically.

Root cause

Reproduced locally by dropping the publishers table (the state an unapplied migration leaves behind):

✘ [ERROR] Error: D1_ERROR: no such table: publishers: SQLITE_ERROR
GET /publisher/clausvalca  500 Internal Server Error
GET /                       200 OK

Two things hid it:

  • The route was null-safe (if (!profile) return 404), but a thrown error never reaches that check — only a returned null does.
  • Production returned 404, not 500, because its catalog is empty. The empty-data path masked the crash.

The underlying operational gap: nothing in .github/workflows/ applies migrations to remote D1, so a migration can be recorded as applied while its table is missing.

The fix

loadPublisherProfile() returns a tagged result instead of collapsing every failure to null:

{ status: 'ok'; profile } | { status: 'not-found' } | { status: 'error'; error }

Callers now distinguish the two failure modes and render 200 / 404 / 503.

The same unguarded-read pattern is fixed in the other routes that had it: dashboard/publisher/[id], dashboard/settings, api/publishers/[id] (GET and PATCH), and api/user/profile. This matches the convention already used by index.astro, dashboard.astro, plugins.json.ts, and download/[id].ts.

Follow-up commit: resolve the binding inside the guard

CodeRabbit caught a real gap in the first commit. getDB(locals) throws when the DB binding is missing, and it was being evaluated as an argument — before loadPublisherProfile entered its try. So a missing binding still produced a 500, and the comment claiming otherwise was wrong.

Verified by commenting out [[d1_databases]] and rebuilding: /publisher/<id> and /api/publishers/<id> both returned 500.

loadPublisherProfile now also accepts a getter, so resolving the binding happens inside the guarded region:

await loadPublisherProfile(() => getDB(locals), id);   // not getDB(locals)

The two PATCH handlers move getDB into their existing try. A plain binding is still accepted, so this only widens the guarded region.

Verification

Every row below was exercised against wrangler pages dev with the built worker, in all three DB states (healthy, table dropped, binding removed):

Scenario Before After
Healthy DB, existing publisher 200 200
Healthy DB, missing publisher 404 404
Healthy DB, invalid slug 400 400
publishers table missing (page) 500 503 + fallback
publishers table missing (API) 500 503 JSON
DB binding missing (page) 500 503 + fallback
DB binding missing (API) 500 503 JSON
Homepage in every broken state 200 200

The page shows a real fallback — "Publisher unavailable" with a retry on a server error, "Publisher not found" otherwise.

  • npm run build — clean
  • npx tsc --noEmit — src/ clean (only the pre-existing astro.config.mjs vite-plugin error remains)
  • npm test — 47 passing (6 new: error / not-found / success / empty-profile / throwing-getter / plain-binding)

Follow-up required (not in this PR)

This stops the 500 but does not restore the feature on production: the remote publishers table is genuinely missing, so the page will now render the 503 fallback. To actually fix the live site:

npm run db:migrate:remote
npm run db:seed:remote   # if the catalog is empty

I could not run this — wrangler needs CLOUDFLARE_API_TOKEN for remote access, which isn't available in this environment. The README now documents that migrations are not applied by CI, so the next schema change does not hit the same trap.


This PR was created by an AI agent (OpenHands) on behalf of the user.

Summary by CodeRabbit

  • Bug Fixes
    • Publisher and user profile pages now handle database outages gracefully, showing temporary-unavailability responses instead of failing unexpectedly.
    • Publisher pages distinguish unavailable data from missing profiles and offer a retry option when profile data can’t be loaded.
    • Settings remain accessible with basic session information when a profile lookup fails.
  • Documentation
    • Clarified that remote database migrations must be applied manually and how to check migration status when routes are unavailable.

The publisher page called getPublisherProfile() with no try/catch. When D1
throws — a missing table from an unapplied migration, or an unavailable
binding — the exception escaped the frontmatter and Cloudflare returned an
unhandled 500. Reproduced locally by dropping the publishers table:

  D1_ERROR: no such table: publishers: SQLITE_ERROR
  GET /publisher/clausvalca -> 500 (GET / stayed 200)

The route was already null-safe, but a thrown error never reaches the null
check. Production returned 404 rather than 500 because its catalog is empty,
so the empty-data path masked the crash.

loadPublisherProfile() now returns a tagged result — ok / not-found / error —
instead of collapsing every failure to null, so callers can tell "no such
publisher" (404) from "the database failed" (503). The page renders a
"Publisher unavailable" fallback with a retry instead of crashing.

The same unguarded read pattern is fixed in the other routes that had it:
dashboard/publisher/[id], dashboard/settings, api/publishers/[id] (GET and
PATCH), and api/user/profile. This matches the convention already used by
index.astro, dashboard.astro, plugins.json.ts and download/[id].ts.

README notes that migrations are not applied by CI, so a schema change needs a
manual `npm run db:migrate:remote` — the state that produced this bug.

Tests: 45 passing (4 new, covering the error / not-found / success paths).
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Walkthrough

Publisher profile loading now distinguishes database errors from missing publishers. Publisher and user profile paths handle read failures with temporary-unavailability responses or fallback values. The publisher page renders separate unavailable and not-found states.

Changes: Profile Read Handling

Layer / File(s) Summary
Publisher profile result and tests
src/lib/publisher-profile.ts, tests/publisher-profile-page.test.ts, README.md
loadPublisherProfile returns ok, not-found, or error. Tests cover these results and profile composition. The README lists the helper and its test.
Publisher route error handling
src/pages/api/publishers/[id].ts, src/pages/publisher/[id].astro, src/pages/dashboard/publisher/[id].astro
Publisher API and page paths return 503 for database errors and retain 404 handling for missing publishers. The public publisher page renders unavailable or not-found content, with a retry button for database errors.
User profile handling and migration notes
src/pages/api/user/profile.ts, src/pages/dashboard/settings.astro, README.md
User profile routes return 503 on read errors, while settings uses session-derived fallback values. The README documents manual remote migrations and the 503 fallback for missing tables.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: openhands-agent

Merge Risk: Moderate · up to 877a2

A transient settings read failure can silently overwrite saved profile customizations when the user submits the displayed fallback form; prevent editing until the profile loads successfully.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing a 500 response on /publisher/[id] when a D1 read fails.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 4 files. (4 skipped: 4 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 66566f32-5782-4cf3-8f3e-f7fd91fee706
Commits

Reviewing files that changed from the base of the PR and between 975e4aa and 9400668.

Files selected for processing: 8
  • README.md
  • src/lib/publisher-profile.ts
  • src/pages/api/publishers/[id].ts
  • src/pages/api/user/profile.ts
  • src/pages/dashboard/publisher/[id].astro
  • src/pages/dashboard/settings.astro
  • src/pages/publisher/[id].astro
  • tests/publisher-profile-page.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Deploying openrom-store with  Cloudflare Pages  Cloudflare Pages

Latest commit: 877a270
Status: ✅  Deploy successful!
Preview URL: https://8786cebf.openrom-store.pages.dev
Branch Preview URL: https://fix-publisher-route-500.openrom-store.pages.dev

View logs

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread src/pages/api/publishers/[id].ts Resolved
Comment thread src/pages/api/user/profile.ts Resolved
Comment thread src/pages/publisher/[id].astro Resolved
CodeRabbit caught a real gap: `getDB(locals)` throws when the `DB` binding is
missing, and it was evaluated as an argument — before loadPublisherProfile
entered its try. Both routes still returned 500 for that case, so the comment
claiming "unavailable binding" was covered was wrong.

Verified by removing the [[d1_databases]] block and building: /publisher/<id>
and /api/publishers/<id> both returned 500.

loadPublisherProfile now also accepts a getter, so resolving the binding
happens inside the try. Call sites pass `() => getDB(locals)`; the two PATCH
handlers move `getDB` into their existing try. Same inputs as before — plain
bindings still work — so this is purely a widening of the guarded region.

After: missing binding -> 503 (page fallback + JSON), missing table -> 503,
healthy -> 200/404. Tests: 47 passing (2 new covering the getter path).
@ClausValcaTD
ClausValcaTD merged commit 84adfed into main Nov 3, 2026
3 of 4 checks passed
@ClausValcaTD
ClausValcaTD deleted the fix/publisher-route-500 branch November 3, 2026 22:07
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.

2 participants