Repository navigation
π dashboard: merged toast shows the PR number - #15279
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this hive's repositories have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
There was a problem hiding this comment.
Read the change against the tree, not just the diff. Looks correct to me.
Root cause confirmed. esc() at src/pkg/dashboard/static/index.html:39773-39774 returns '' for any non-string, and stageTrackerCelebrateMerges (index.html:19698) passes number as a Number, so #${esc(info.number)} at index.html:19816 drew a bare #. The old test stub (stage_tracker_cues_test.go:243, String(s).replaceβ¦) did not reproduce that; the new stub mirrors the page, and the new assertions (#1, nested pr.number, URL-only, empty field, no number) would fail against the old banner.
Adjacent fix is warranted. The "Recent merges" popover at index.html:19465-19470 had the same latent blank: txt.today/week/total30d/count are numeric (index.html:19401) and e.number goes through esc() unwrapped. The PR body doesn't mention this part, but it is the same mechanism and the esc(String(β¦)) pattern already used at index.html:36479 and 54685.
Minor, not a defect: in stageTrackerMergeBanner the recent lookup (index.html:19800) still keys on b.number, so a banner whose number lives only in pr.number/url won't pick up the ledger's title/merged_by. It still links and reads #N merged, which is what the issue asks for.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the tree, not just the diff β this looks correct to me.
- Root cause verified:
esc()atsrc/pkg/dashboard/static/index.html:39774returns''for any non-string, and the live caller atindex.html:19698-19699passesnumberas aNumber(...), so the old#${esc(info.number)}could only ever render#. The old test stub (stage_tracker_cues_test.go:243) usedString(s), which is exactly why the harness never caught it; mirroring the page'sescin the stub is the right fix. - Preview path checked:
STAGE_TRACKER_CELEBRATE_TEST_KEY = 'preview/pr#0'(index.html:20309) withnumber: 0β the new key fallback matches/pr#0butNumber('0') > 0is false, so the test banner readsmerged (test)with no bare#, and no URL is fabricated for it (!info.previewguard kept). - The merged-list popover rows (
index.html:19443-19449) now wrape.number,e.repoand the counts inString(), which fixes the same class of drop in the "Recent merges" list β a small scope extension beyond the toast, but same bug, same file.
No findings.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
The merge banner rendered #${esc(info.number)} with a Number; the page's
esc() returns '' for any non-string, so the head read a bare "# merged".
Resolve the number as a string (field, nested pr record, GitHub URL, key),
never render a bare "#", and make the node harness's esc stub mirror the
real one so the case is covered.
Fixes #15277
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
β¦n-string drop) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
4b9d5c8 to
262cd10
Compare
There was a problem hiding this comment.
Read the fix against the page, not just the diff β looks correct to me.
Root cause confirmed. esc() at src/pkg/dashboard/static/index.html:39826 is if (typeof s !== 'string') return '';, and the live caller stageTrackerCelebrateMerges (index.html:19736) passes number as a Number(...), so #${esc(info.number)} really did render # with nothing after it. The old test stub (stage_tracker_cues_test.go:243, String(s).replaceβ¦) would never have caught it; the new stub mirrors the page's behaviour, so the regression test now fails without the fix.
Fallback chain checked. stageTrackerMergeBannerNumber β number / pr_number / pr.number β /pull/N in the URL β /pr#N in the key β ''. The preview banner (index.html:20355, key preview/pr#0, number: 0) now resolves to '' and reads merged (test) instead of #0 merged (test) β a small, sensible change in the test-celebration head, worth knowing about but not a defect. The "Open on GitHub" link is now built from the same resolved number, so head and link cannot disagree.
The String(...) wrapping in the recent-merges popover (:19501β:19507) fixes the same class of bug for today/week/total30d/count, which were also numeric and would have rendered empty.
No findings. (I can't run go test on this host; the node harness assertions read correctly against the page.)
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Fixes #15277
Symptom: Activity Monitor merge toasts read
β # mergedβ title and "Open on GitHub" present, number missing (seen for the containment and never-adopt merges).Root cause:
stageTrackerMergeBannerrendered#${esc(info.number)};info.numberis a Number andesc()returns''for any non-string. The node-harness test stubbedescasString(s)β¦, which hid it.Fix:
stageTrackerMergeBannerNumber(b): number as a string fromnumber/pr_number/ nestedpr.number, else the GitHub URL (/pull/N), else therepo/pr#Nkey;''when none.#15122 merged; with no number anywhere it readsmergedβ never a bare#. "Open on GitHub" is built from the same resolved number.escnow mirrors the page's; new cases: number only in nestedpr, only in URL, empty field β head#15122 mergedand the link targets the same PR; no number β no bare#.changelog.d/fixed-merged-toast-number.md.