Fix mixed HTML+JSON error response when issue comment creation fails#36667
Fix mixed HTML+JSON error response when issue comment creation fails#36667silverwind wants to merge 2 commits intogo-gitea:mainfrom
Conversation
When NewComment's CreateIssueComment call failed, ctx.ServerError wrote an HTML 500 page but the deferred function still ran and appended a JSON redirect to the response body, producing an invalid mixed HTML+JSON response shown as a toast error. Fix by adding a ctx.Written() guard in the defer to skip writing when a response was already sent. Also make post-creation errors in CreateIssueComment (FindAndUpdateIssueMentions, GetIssueByID) non-fatal since the comment is already persisted at that point, preventing duplicate comments on retry. Co-Authored-By: Claude Opus 4.6 <[email protected]>
GiteaBot
added
the
lgtm/need 2
There was a problem hiding this comment.
Pull request overview
Fixes a malformed response scenario in the web issue comment creation flow where an error page response could be followed by a deferred JSON redirect, producing a mixed/invalid response body.
Changes:
- Add a
ctx.Written()guard inNewComment’s deferred redirect to avoid writing after an error response is already sent. - Make mention-update and issue-reload failures in
CreateIssueCommentnon-fatal (log and continue) to avoid retry-caused duplicate comments after persistence. - Add an integration test ensuring failed comment creation returns clean JSON without an appended redirect.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/integration/issue_test.go | Adds an integration test asserting error responses are valid JSON and don’t include a deferred redirect. |
| services/issue/comments.go | Logs (instead of returning) post-persistence errors during mention update / issue reload. |
| routers/web/repo/issue_comment.go | Prevents deferred JSON redirect from appending to an already-written error response. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use a temporary variable so issue is only overwritten on success, preventing a nil dereference in notify_service.CreateIssueComment. Co-Authored-By: Claude Opus 4.6 <[email protected]>
| mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content) | ||
| if err != nil { | ||
| return nil, err | ||
| log.Error("FindAndUpdateIssueMentions: %v", err) |
| issue, err = issues_model.GetIssueByID(ctx, issue.ID) | ||
| if err != nil { | ||
| return nil, err | ||
| if reloadedIssue, err := issues_model.GetIssueByID(ctx, issue.ID); err != nil { |
You need to figure out why they return errors, and only ignore the errors you understand |
I have no way to reproduce this currently, the actual cause for the 500s should be visible in gitea.com logs. Maybe it is something specific to that instance. In any case, the garbled HTML+JSON response is definitely a bug. |
silverwind
changed the title
|
#36772 likely another case of mixed HTML+JSON response. |
No, it can't be. |
|
Maybe not mixed, but HTML response on a JSON endpoint is definitely wrong. |
Yes, it is wrong. I have spent a lot of time on the "route handler" framework, but there are still a lot of things to do, while nobody is interested in the following up changes. |
|
I guess I will do a validation pass here with Claude to find any endpoints that expect JSON on frontend but can end up returning HTML in backend (like in case of errors). That should hopefully eliminate all these |
If it can do right, but not just keeps introducing duplicate or messy code. |
|
Closing as per #37077 (comment). Maybe I will re-visit it later after that is merged. |
While posting issue comments on gitea.com, I notice it sometimes fails with toasts that show a HTML string:
I checked the HTTP response and found a garbled response for
POST https://gitea.com/<owner>/<repo>/issues/<id>/commentsof content-typetext/html; charset=utf-8that contains both html and json:This PR is the result of Claude investigating this broken response. Full details:
When NewComment's CreateIssueComment call failed, ctx.ServerError wrote an HTML 500 page but the deferred function still ran and appended a JSON redirect to the response body, producing an invalid mixed HTML+JSON response shown as a toast error.
Fix by adding a ctx.Written() guard in the defer to skip writing when a response was already sent. Also make post-creation errors in CreateIssueComment (FindAndUpdateIssueMentions, GetIssueByID) non-fatal since the comment is already persisted at that point, preventing duplicate comments on retry.