Repository navigation
✨ feat(dashboard): show a draft pill in the activity tracker - #14573
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 repository 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
|
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
|
@clubanderson this PR is held at Why:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
Read the Go change against the tree at 4e6ec94 plus the JS it feeds; the logic looks correct to me. One blocker that isn't the code: GitHub reports this CONFLICTING against v5 (conflicts-with-open-pr, DIRTY) — #14571 landed on v5 after this branched and touches stage_tracker.go and stage_tracker_ui_test.go, so it needs a rebase before the merge lane can take it. CI on the current head is otherwise green.
What I verified:
src/pkg/dashboard/stage_tracker.go:642-651— the label loop lower-cases before the switch, so theprowWIPLabelcase and theEqualFoldin theDeleteFuncfilter agree on case/whitespace;holdLabels(bands.go:571) matchesdo-not-merge/*by prefix, and the delete removes only the exact WIP label, sodo-not-merge/holdstill setsmodHold. The output order atstage_tracker.go:665keepsholdahead ofdraft, which is what thehold +1fold in the UI test relies on.static/index.html:13906-13920—openDraftis gated on!openHold && !openBlocked, the stale tint is suppressed only for the draft word, andopenSincestill computes for drafts so the age shows.--band-draftexists in both themes (index.html:87,190), so.ham-open-draftresolves.- PR body matches the diff (payload, frontend, tests, docs, changelog).
One cosmetic note, not a defect: stageTrackerHoldLabels (index.html:13829-13836) still matches do-not-merge/*, so for a PR carrying both do-not-merge/hold and do-not-merge/work-in-progress, the headline at index.html:13788 will read Open · on hold (do-not-merge/hold, do-not-merge/work-in-progress). WIP-only rows are unaffected since mods no longer contains hold. Fine to leave or filter the WIP label there too for consistency with the Go side.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read the modifier path and its caller, not just the diff. This looks correct to me.
src/pkg/dashboard/stage_tracker.go:642-650—do-not-merge/work-in-progressnow setsdraftand is filtered out ofholdLabels(...)before theholdmodifier fires. Verified the body's claim that it used to read as a hold:src/pkg/dashboard/bands.go:571matches anydo-not-merge/prefix, so a WIP-only PR did getholdbefore this change. A realhold/do-not-merge/holdstill survives theDeleteFuncand leads the pill (hold +1), as the body says.static/index.html:openDraftis only true whenn === 0 && !openHold && !openBlocked, andstaleis masked by!openDraft— matches the stated "hold and blocked still win the cell / never tints stale".- Docs and changelog fragment (root
changelog.d/, correct location) match the behaviour.
One thing blocking it: the branch no longer merges into v5 — src/pkg/dashboard/static/index.html conflicts with the head that landed via #14571 (I reproduced it with a local merge). Needs a rebase; nothing else from me.
Open question (not a defect, out of this PR's scope): bands.go:571 still classifies a WIP-only draft as held for the Overview band / heldReason, so the band and the tracker now disagree on what WIP means. Worth a follow-up if the band should match.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
4e6ec94 to
949c724
Compare
|
Rebased onto On |
|
@clubanderson this PR is held at Why:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
Read stage_tracker.go, bands.go and the index.html segment/pill code at this head. The tracker change looks correct to me: stageTrackerModifiers (stage_tracker.go:709-718) now sets draft on prow's do-not-merge/work-in-progress and strips only that label from the holdLabels count, so a draft PR wearing nothing else reads draft instead of hold; the JS orders openDraft after hold and blocked and excludes drafts from the stale tint, matching the Go priority list.
One low-severity note, not a blocker and arguably out of #14572's scope: the same WIP label still counts as a hold everywhere else that calls holdLabels — bands.go:209 puts the PR in the waiting band (so the draft band at bands.go:224 stays unreachable for any prow-labelled draft), bands.go:242 adds the ⏸ held signal, and api_hive_advice.go:184 reports Held: true. After this lands a maintainer sees "Draft" in the tracker and "⏸ held / waiting" in the band for the same PR. If you want the two views to agree, the WIP exclusion probably belongs inside holdLabels itself (or a shared helper) rather than at the one tracker call site; happy to leave that to a follow-up.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Draft PRs on the Hive activity monitor rendered as a bare "Open · since 18h" row with no status pill, which read as a stuck item. The tracker now derives a muted draft modifier from the PR's draft bit or prow's do-not-merge/work-in-progress label (which it reads as a draft rather than a hold), renders a draft pill with an explanatory tooltip, and shows the Open cell as "Draft · since …" without the stale tint. Hold still leads a draft. Fixes #14572 Signed-off-by: Andy Anderson <andy@clubanderson.com>
The draft fixture is a PR, whose ham-pr-ref tooltip starts with the PR title, so match the 'Open · draft' headline anywhere in it rather than at the start. Signed-off-by: Andy Anderson <andy@clubanderson.com>
a3b7caf to
e54c67f
Compare
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at e54c67f. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
There was a problem hiding this comment.
Read the Go and the tracker JS against v5; looks correct to me.
src/pkg/dashboard/stage_tracker.go:777:do-not-merge/work-in-progressnow setsdraftand is filtered out ofholdLabels()before themodHoldcheck, so a PR wearing only prow's WIP label readsdraftrather thanhold. Any otherdo-not-merge/*orholdlabel still setsmodHold(bands.go:571), so a real maintainer hold on a draft PR is not masked.modDraftalready existed instageTrackerModifierOrderandstageTrackerSunkMods, so ordering and sink behaviour are unchanged.static/index.html: theDraftword /✎chevron apply only whenn === 0 && state === 'active' && !openHold && !openBlocked, and the stale tint is suppressed only in that same case, so a blocked or held Open cell keeps its existing stale signal.
Matches the scope of #14572 as the body states.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Fixes #14572
What
Draft PRs on the dashboard Hive activity monitor (e.g. #14427 / #14428 while
isDraft=true+ prowdo-not-merge/work-in-progress) rendered as a bare "Open · since 18h" row with no status pill, which read as a stuck item. The hive's review and merge lanes skip drafts on purpose, so the wait is the author's — the row should say so.Changes
stage_tracker.go): thedraftmodifier now fires when the PR snapshot's draft bit is set or the PR carries prow'sdo-not-merge/work-in-progresslabel. That WIP label is read as a draft on the tracker rather than ahold(it used to matchholdLabels'do-not-merge/*prefix), so a WIP-only PR readsdraft; a realhold/do-not-merge/holdstill leads (hold +1).static/index.html): muteddraftpill (same.ham-badgetreatment ashold, not red) with tooltip "Draft PR — hive review and merge lanes skip drafts until the author marks it ready for review."; Open cell reads "Draft · since 18h" with a ✎ glyph in the draft tint and never takes the stale red tint; headline/tooltip say "Open · draft". Hold and blocked still win the cell.TestStageTrackerModifiers_Draft(draft bit; WIP label only; neither; draft+hold →["hold","draft"]; WIP+hold; hold only; draft+blocked). Node fixture assertions inTestStageTrackerRowsfor the Open cell, segment class, pill HTML/tooltip,hold +1fold, stage-50 draft, and current-stage HTML. Notime.Sleep.src/docs/dashboard-sections.mdactivity-monitor paragraph. Changelogchangelog.d/added-tracker-draft-pill.md.Webhook plumbing (
ready_for_review/converted_to_draft) already refreshesDraftviapatchPullRequest/patchWorkPRAny; no change needed there.