Skip to content

fix(server): keep /api/diff selection state intact when diff parsing fails#440

Open
kiakiraki wants to merge 1 commit into
yoshiko-pg:mainfrom
kiakiraki:fix/api-diff-error-recovery
Open

fix(server): keep /api/diff selection state intact when diff parsing fails#440
kiakiraki wants to merge 1 commit into
yoshiko-pg:mainfrom
kiakiraki:fix/api-diff-error-recovery

Conversation

@kiakiraki

Copy link
Copy Markdown

Summary

  • move the currentSelection update in the /api/diff handler to after the diff is successfully resolved, so a request with an invalid ref no longer poisons the server state used as the fallback for subsequent requests
  • return a JSON 500 error when diff parsing fails, matching the error handling pattern of the other API endpoints (previously Express's default HTML stack trace was returned)
  • add a regression test covering both behaviors, replacing the previously skipped handles invalid commit gracefully placeholder

Root cause

In the /api/diff handler, currentSelection = requestedSelection; ran before parser.parseDiff(). When a request contained an invalid ref (e.g. /api/diff?target=deleted-branch — a branch deleted while switching revisions, or a hand-edited URL), the parse failed but the invalid selection had already overwritten currentSelection. Every subsequent parameter-less request (such as the initial fetch on page reload) then fell back to the poisoned selection and kept returning 500 until the server was restarted. The handler also had no try/catch, so the failure surfaced as Express's default HTML error page instead of JSON.

Reproduction (before this fix)

difit HEAD --no-open --port 4901 &
curl "http://localhost:4901/api/diff?target=nonexistent-branch"  # → 500 (HTML stack trace)
curl "http://localhost:4901/api/diff"                            # → 500 forever, until restart

Verification

  • pnpm check / pnpm format / pnpm knip / pnpm test / pnpm build all pass
  • confirmed the new regression test fails against the unfixed handler and passes with the fix
  • manually verified with the built CLI: the first request now returns a JSON 500, and the following parameter-less request recovers with 200

🤖 Generated with Claude Code

…fails

A single request with an invalid ref (e.g. /api/diff?target=deleted-branch)
permanently broke the server: currentSelection was overwritten before
parseDiff succeeded, so every subsequent parameter-less request reused the
bad ref and returned 500 until restart. The handler also had no try/catch,
so failures surfaced as Express's default HTML stack trace.

Move the currentSelection update after the diff is successfully resolved
and return a JSON 500 error on parse failure, matching the error handling
pattern of the other API endpoints.

Add a regression test covering the JSON 500 response and the recovery of
subsequent requests, replacing the previously skipped placeholder test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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