[TRUNK-17695] Document impacted targets visualization in merge graph UI#458
Conversation
|
😎 Merged successfully - details. |
|
Claude finished @samgutentag's task in 2m 22s —— View job Review complete
The new documentation is clearly written and well-structured overall. A few issues to address before merging: Blocking concerns:
Non-blocking suggestions:
|
Add sections for impacted targets tooltips, overlapping targets on graph edges, and the impacted targets modal on the PR detail page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8304fc9 to
5d2a43a
Compare
|
Verification status (2026-05-06): Verified: customers can use this. Ready to publish.
Pre-merge blockers:
Suggested next action: rebase on |
Resolves conflict in monitor-queue-status.md by keeping both the existing Priority badges section (from main) and the new Impacted targets sections (from this PR), in that order under Graph view. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @samgutentag's task in 1m 49s —— View job Review complete
The new content is clearly written and well-organized, and the merge from Blocking concerns:
Non-blocking suggestions (new this round):
Setting |
Verified against trunk2: - Toggle is "Show targets on hover" (graph-toolbar.tsx:83), not "Show impacted targets" - Button is Title Case "View Impacted Targets" (merge-item-header.tsx:71) - The button only renders in Parallel mode; in Single mode the readiness object has no hasImpactedTargets field (readiness_checker.ts:374-385) - The third modal state is an explicitly empty list (depends on nothing, nothing depends on it), not "no targets uploaded" (view-impacted-targets-modal.tsx:70-86) Also drops one em dash in favor of a colon. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @samgutentag's task in 2m 25s —— View job Review complete
The latest commit (
Blocking concerns still unresolved:
New this round:
Non-blocking:
Setting |
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
Claude finished @samgutentag's task in 1m 32s —— View job Review complete
The latest commit (
Blocking concerns still unresolved:
Resolved in earlier commits (for reference):
Setting Branch • |
Per code review feedback, the impacted targets modal section was nested under Graph view but actually documents PR detail page UI rendered by merge-item-header.tsx. Moved it under the existing Pull request details section as a new ### View impacted targets subsection, where it lives alongside the other PR detail page content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @samgutentag's task in 1m 30s —— View job Review complete
The latest commit (
Resolved in earlier commits (for reference):
Blocking concerns still unresolved:
Setting Branch • |
|
Claude finished @samgutentag's task in 1m 19s —— View job Review complete
The latest synchronize event is the merge commit Resolved across earlier commits (for reference):
Blocking concerns still unresolved:
Setting Branch • |
Per recurring review feedback. PRs are tested by the queue, not by themselves, so "must be tested together" reads more correctly than "must test together." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @samgutentag's task in 1m 0s —— View job Review complete
The latest commit (
That was the last content blocker carried over from earlier reviews. Nothing on the changed lines (101–111 and 140–147) needs further wording, formatting, or structural changes from me this round. Resolved across earlier commits (for reference):
Blocking concern still unresolved:
Setting |
The Trunk Check job on PR #458 was failing because vale flagged "tooltips" as an unknown word on every push (the word sits in a recently-modified hunk, so the diff-based linter kept surfacing it as a "new" issue). Adding the regex pattern so both singular and plural pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @samgutentag's task in 1m 3s —— View job Review complete
The latest commit ( Resolved across earlier commits (for reference):
Blocking concern still unresolved:
No new inline comments posted — all prior content feedback is now resolved, and no new wording, formatting, or structural issues surfaced in the latest commit. Setting |
|
Pre-merge blockers update (2026-05-07) All three blockers from the 2026-05-06 verification status comment are now resolved:
Plus a few extras picked up during review:
Screenshots are the only deferred item and are not a blocker for this content-only PR. Ready for merge queue. |
Posted a follow-up comment confirming all blockers from the 2026-05-06 verification-status comment are resolved (screenshots explicitly deferred to a follow-up PR).
|
Claude finished @samgutentag's task in 1m 3s —— View job Review complete
The latest commit ( The deferral rationale holds up:
All prior-round blockers resolved (final check):
No new issues in the latest commits. No new inline comments to post. Setting Branch • |
Summary
• Added "Impacted targets in the graph" subsection to the Graph view on the Monitor queue status page — covers per-PR tooltips and overlapping targets toggle
• Added "Impacted targets on the PR detail page" subsection — covers the View impacted targets modal with IMPACTS_ALL, specific targets, and no targets states
Note: The metrics filtering by impacted targets is already documented in
merge-queue/administration/metrics.md, so no update needed there.Context links
Files changed
merge-queue/using-the-queue/monitor-queue-status.md— new subsections under Graph viewOpen questions
Test plan
🤖 Generated with Claude Code