ITADN

Literal "null" is rendered in the pull request timeline after CI finishes

#38738Openlunny 创建于 27 天前
type/bug
L
lunnycommented
## What to build Leaving a pull request page open while its CI is still running can render the literal text `null` in the conversation timeline, and the merge box disappears until the page is reloaded. Root cause: - While the PR status is pending/checking, the merge box is refreshed every 5s by `GET /{owner}/{repo}/pulls/{index}/merge_box`, and the response is applied with `createElementFromHTML(await resp.text())` followed by `el.replaceWith(newEl)`. - `createElementFromHTML()` returns `div.firstChild`, which is `null` for an empty/whitespace body, while its declared return type claims a non-null element. - `ParentNode.replaceWith()` accepts `(Node or DOMString)`, so `null` is converted to the string `"null"`: the merge box element is destroyed and a literal `null` text node is inserted into the timeline. Because the element is gone, `data-global-init` never re-registers the refresh timer, so the merge box never comes back without a full page reload. - The fragment can legitimately be an empty `200` response: the whole merge box template is wrapped in `{{if $data.ShowMergeBox}}`, and `ShowMergeBox` is false for a merged PR whose head branch is not deletable (branch deleted after merge, no delete permission, or the session expired so the poll became anonymous). So when a PR is merged while the page is polling - for example auto-merge firing once CI turns green - the next refresh returns nothing and the timeline shows `null`. - The same symptom can be produced by any template execution error in this fragment, because the page renderer writes the HTTP status before executing the template, so a render failure still returns `200` with an empty body. Fix direction: - The merge box fragment endpoint must never return an empty body: render a minimal empty `.pull-merge-box` placeholder element when there is no merge box to show, so a refresh keeps replacing exactly one element and simply stops reloading. - Make the empty/invalid fragment unrepresentable on the frontend too: `createElementFromHTML()` must express that it can return nothing, and the merge box refresh must bail out instead of injecting `null`. ## Acceptance criteria - [ ] `GET /{owner}/{repo}/pulls/{index}/merge_box` always responds with exactly one `.pull-merge-box` element, including for a merged pull request whose head branch no longer exists. - [ ] The rendered pull request page looks unchanged for merged pull requests that currently show no merge box. - [ ] A merge box refresh that receives an empty or non-element response leaves the timeline untouched and never inserts a `null` text node. - [ ] `createElementFromHTML()` no longer claims a non-null return value, and callers that can receive nothing handle it. - [ ] Unit test covers the empty-fragment refresh path, and an integration test covers the endpoint always returning a merge box element. ## Blocked by - None - can start immediately. --- Generated by Codet
1 条评论