Skip to content

[TRUNK-17695] Document impacted targets visualization in merge graph UI#458

Merged
trunk-staging-io[bot] merged 9 commits intomainfrom
sam-gutentag/impacted-targets-graph-ui
May 7, 2026
Merged

[TRUNK-17695] Document impacted targets visualization in merge graph UI#458
trunk-staging-io[bot] merged 9 commits intomainfrom
sam-gutentag/impacted-targets-graph-ui

Conversation

@samgutentag
Copy link
Copy Markdown
Member

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

  • trunk-io/trunk2#2406, #2414, #2457, #3043, #3099 (all merged)

Files changed

  • merge-queue/using-the-queue/monitor-queue-status.md — new subsections under Graph view

Open questions

  1. Is the toggle labeled "Show impacted targets" or something else?
  2. Are there screenshots available for the graph with impacted targets enabled?
  3. Does the impacted targets modal show on PRs in Single mode, or only Parallel mode?

Test plan

  • Check GitBook preview
  • Verify link to parallel queues page works

🤖 Generated with Claude Code

@trunk-staging-io
Copy link
Copy Markdown

trunk-staging-io Bot commented Mar 12, 2026

😎 Merged successfully - details.

@samgutentag samgutentag changed the title Document impacted targets visualization in merge graph UI [TRUNK-17695] Document impacted targets visualization in merge graph UI Mar 12, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 12, 2026

Claude finished @samgutentag's task in 2m 22s —— View job


Review complete

  • Gather context
  • Read changed file
  • Review for typos, grammar, formatting, and documentation best practices
  • Post inline comments for specific issues
  • Final summary

The new documentation is clearly written and well-structured overall. A few issues to address before merging:

Blocking concerns:

  1. Open questions unresolved — The PR body flags three open questions about content accuracy (toggle label, screenshots, Single vs. Parallel mode applicability). These should be resolved before merging to avoid shipping inaccurate docs.
  2. Structural placement — The #### Impacted targets on the PR detail page subsection is nested under ### Graph view, but it describes a different part of the UI. See inline comment at line 59.

Non-blocking suggestions:

  • Line 65: IMPACTS_ALL is bold in the bullet label but unformatted in the inline reference in the same bullet. See suggestion at line 65.
  • Consider using backtick code formatting (`IMPACTS_ALL`) for the enum value throughout for semantic clarity.
  • Adding screenshots (open question ok #2) would significantly improve this section's usability.

Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
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>
@samgutentag samgutentag force-pushed the sam-gutentag/impacted-targets-graph-ui branch from 8304fc9 to 5d2a43a Compare March 17, 2026 05:43
@samgutentag
Copy link
Copy Markdown
Member Author

Triage status (2026-05-05): merge conflicts with main in merge-queue/using-the-queue/monitor-queue-status.md. The graph view section has been edited since this PR opened (PRs #487, #545, #556 touched the same file). Resolve before merge.

@samgutentag
Copy link
Copy Markdown
Member Author

samgutentag commented May 6, 2026

Verification status (2026-05-06): live

Verified: customers can use this. Ready to publish.

  • Eng PRs: trunk2#2406, #2414, #2457, #3043, #3099 (all merged Jan and Feb 2026)
  • Linear ticket: TRUNK-17695
  • Flag: none (no useFeatureFlag, useFlag, flags.ts, or LaunchDarkly references in any of the five diffs or PR bodies)
  • Signals: all five merge commits are still on main (status behind for each, no reverts). Feature shipped ungated.

Pre-merge blockers:

Suggested next action: rebase on main, resolve conflicts, address the three open questions and the Claude review feedback, then merge.

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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 49s —— View job


Review complete

  • Read changed file and PR diff
  • Review for typos, grammar, formatting
  • Review for documentation best practices
  • Post inline comments for specific issues
  • Final summary

The new content is clearly written and well-organized, and the merge from main (commit 0c34be7) resolved the conflict flagged in earlier triage. However, several issues from the prior review remain unaddressed and a few new ones surfaced.

Blocking concerns:

  1. Three open questions from the PR description are still unresolved — toggle label ("Show impacted targets"), Single vs. Parallel mode applicability for the View impacted targets modal, and screenshot availability. Per the verification status comment (2026-05-06), these are pre-merge blockers.
  2. Structural placement (still unresolved from prior review)#### Impacted targets on the PR detail page (line 113) is nested under ### Graph view but documents the PR detail page, which is its own section at ## Pull request details (line 135). Either move the subsection, or add a sentence justifying the placement (e.g., "you can reach this from a graph node by clicking through to the PR's detail page"). See prior inline comment at line 113.
  3. Formatting inconsistency for IMPACTS_ALL (still unresolved) — bolded as **IMPACTS\_ALL** on line 117 but unformatted on line 119. Recommend backticks for both since it's an enum value. See new inline comment at line 119.

Non-blocking suggestions (new this round):

  • Line 106: "impacted targets information" — "information" is filler. See inline.
  • Line 109: "PRs with shared targets must test together" → "must be tested together" (PRs don't test themselves). See inline.
  • Line 119: "for safety" reads as informal; consider explaining why (e.g., "because Trunk can't determine which targets are affected, it conservatively assumes all targets are impacted"). Optional.
  • Adding the screenshots from open question ok #2 would significantly improve both new subsections.

Setting review_passed = false due to the three unresolved open questions, the unresolved structural placement issue, and the unresolved IMPACTS_ALL formatting inconsistency.
Branch

Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 2m 25s —— View job


Review complete

  • Read changed file and PR diff
  • Review for typos, grammar, formatting
  • Review for documentation best practices
  • Post inline comments for specific issues
  • Final summary

The latest commit (e968ed1 — "correct impacted targets UI labels and modal states") made meaningful progress on the open questions from the PR body. Resolved this round:

  • Open question ok #1 (toggle label) — now Show targets on hover (was Show impacted targets)
  • Open question Fix links #3 (Single vs. Parallel) — line 115 now scopes the modal to Parallel mode explicitly
  • Empty-list modal semantics — rewritten from "treated as IMPACTS_ALL for safety" to "PR will not depend on anything, and nothing will depend on it" (opposite behavior — flagged inline at line 119 for sanity check, since the prior draft claimed the opposite)

Blocking concerns still unresolved:

  1. Structural placement (line 113) — #### Impacted targets on the PR detail page is still nested under ### Graph view, but documents the PR detail page (which has its own ## Pull request details section at line 135). Raised in 03-12 review and 05-07 review; still unaddressed in e968ed1. Either move the subsection or add a one-sentence bridge ("you can reach this from a graph node by clicking through to the PR's detail page").
  2. Grammar/word-choice carry-overs — line 106 ("impacted targets information") and line 109 ("PRs with shared targets must test together") are unchanged from the prior review. The existing inline comments at those lines still apply.

New this round:

  • Line 117IMPACTS_ALL formatting: with the prior draft's second occurrence rewritten away, the lone **IMPACTS\_ALL** is the only spot where this enum appears; backticks would be cleaner. Suggestion posted inline.
  • Line 119 — semantics of "empty list" state inverted vs. prior draft. Posted inline asking for confirmation against actual behavior.

Non-blocking:

  • Open question ok #2 (screenshots) — still not addressed; would help users locate the toggle and modal in the UI.
  • "View Impacted Targets" (line 115) — title case is unusual for in-text button references, but matches the convention in merge-queue/integration-for-slack.md (Unlink Account, Link Account), so leaving it alone unless the actual UI uses sentence case.

Setting review_passed = false due to the unresolved structural placement issue and the unaddressed line 106 / line 109 grammar nits flagged in the prior review.
Branch

Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 32s —— View job


Review complete

  • Read changed file and PR diff
  • Review for typos, grammar, formatting
  • Review for documentation best practices
  • Post inline comments for specific issues
  • Final summary

The latest commit (d2dda5b — "Apply suggestions from code review") resolved two of the issues flagged in the 05-07 06:29 review:

  • Line 106 wording — "impacted targets information" → "surfaces impacted targets data" (filler word "information" removed)
  • Line 117 IMPACTS_ALL formatting**IMPACTS\_ALL**`IMPACTS_ALL`, eliminating the awkward \_ escape and using semantically correct backticks for an enum value

Blocking concerns still unresolved:

  1. Structural placement (line 113)#### Impacted targets on the PR detail page remains nested under ### Graph view despite documenting the PR detail page (which has its own ## Pull request details section at line 135). This has now been raised across three review rounds (03-12, 05-07 06:23, 05-07 06:29) without being addressed. New inline comment posted at line 113 with both options (move vs. add a bridge sentence).
  2. Line 109 grammar (still unresolved) — "PRs with shared targets must test together" needs passive voice ("must be tested together"). Flagged in the 05-07 06:23 review and unchanged in e968ed1 and d2dda5b. Inline suggestion re-posted at line 109.
  3. Open question ok #2 (screenshots) — Per the verification status comment, screenshots are a pre-merge blocker. Still not added. Inline note at line 115 with the convention used elsewhere in this file.

Resolved in earlier commits (for reference):

  • ✅ Open question ok #1 (toggle label) — Show targets on hover (resolved in e968ed1)
  • ✅ Open question Fix links #3 (Single vs. Parallel mode) — line 115 explicitly scopes the modal to Parallel mode (resolved in e968ed1)
  • ✅ Empty-list modal semantics — line 119 rewritten to "PR will not depend on anything, and nothing will depend on it" (resolved in e968ed1)
  • ✅ Merge conflict from triage — resolved by merge commit 0c34be7

Setting review_passed = false due to the unresolved structural placement issue at line 113 (raised three times), the unresolved line 109 grammar nit (raised twice), and the missing screenshots (called out as a pre-merge blocker in the verification-status comment).

Branch

Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 30s —— View job


Review complete

  • Read changed file and PR diff
  • Review for typos, grammar, formatting
  • Check status of unresolved issues from prior reviews
  • Post inline comments for specific issues
  • Final summary

The latest commit (2743528 — "move View impacted targets under Pull request details") resolved the highest-priority blocker flagged across three prior review rounds:

  • Structural placement### View impacted targets is now at line 141 under ## Pull request details, where it belongs. Promoted from #### to ### to match the new parent heading level. Nice fix.

Resolved in earlier commits (for reference):

  • ✅ Open question ok #1 (toggle label) — Show targets on hover (resolved in e968ed1)
  • ✅ Open question Fix links #3 (Single vs. Parallel mode) — line 143 explicitly scopes the modal to Parallel mode (resolved in e968ed1)
  • ✅ Empty-list modal semantics — line 147 reads "PR will not depend on anything, and nothing will depend on it" (resolved in e968ed1)
  • ✅ Line 106 wording — "surfaces impacted targets data" (resolved in d2dda5b)
  • ✅ Line 145 IMPACTS_ALL formatting — backticks applied (resolved in d2dda5b)
  • ✅ Merge conflict from triage (resolved by merge commit 0c34be7)

Blocking concerns still unresolved:

  1. Line 109 grammar — "PRs with shared targets must test together" still uses active voice for an action PRs don't perform on themselves. Flagged in two prior reviews. Inline suggestion re-posted at line 109.
  2. Open question ok #2 (screenshots) — Per the verification-status comment, screenshots are a pre-merge blocker. Still missing for both the Show targets on hover toggle (line 109) and the View Impacted Targets modal (line 143). Inline note posted at line 143.

Setting review_passed = false due to the unresolved line 109 grammar nit and the missing screenshots called out as a pre-merge blocker. The structural placement fix in 2743528 was the right call.

Branch

Comment thread merge-queue/using-the-queue/monitor-queue-status.md Outdated
Comment thread merge-queue/using-the-queue/monitor-queue-status.md
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 19s —— View job


Review complete

  • Read changed file and PR diff
  • Review for typos, grammar, formatting
  • Check status of unresolved issues from prior reviews
  • Post inline comments for specific issues
  • Final summary

The latest synchronize event is the merge commit 3d27a04 ("Merge branch 'main' into sam-gutentag/impacted-targets-graph-ui") — git diff 2743528..HEAD -- merge-queue/using-the-queue/monitor-queue-status.md is empty, so no content changes since the 07:05 review. The blocker status carries over.

Resolved across earlier commits (for reference):

  • ✅ Structural placement — ### View impacted targets moved under ## Pull request details (line 141), promoted from #### to ### (resolved in 2743528)
  • ✅ Open question ok #1 (toggle label) — Show targets on hover (resolved in e968ed1)
  • ✅ Open question Fix links #3 (Single vs. Parallel mode) — line 143 explicitly scopes the modal to Parallel mode (resolved in e968ed1)
  • ✅ Empty-list modal semantics — line 147 reads "PR will not depend on anything, and nothing will depend on it" (resolved in e968ed1)
  • ✅ Line 106 wording — "surfaces impacted targets data" (resolved in d2dda5b)
  • ✅ Line 145 IMPACTS_ALL formatting — backticks applied (resolved in d2dda5b)
  • ✅ Merge conflict from triage (resolved by merge commit 0c34be7)

Blocking concerns still unresolved:

  1. Line 109 grammar — "PRs with shared targets must test together" still uses active voice for an action PRs don't perform on themselves. Should be "must be tested together". Flagged in 05-07 06:23, 05-07 06:57, and 05-07 07:06 reviews — three rounds, still unaddressed. Fix this →

  2. Open question ok #2 (screenshots) — Per the verification-status comment, screenshots are a pre-merge blocker. Still missing for both the Show targets on hover toggle (line 109) and the View Impacted Targets modal/button (line 143). Adjacent sections in this file already use <figure> blocks (e.g., line 137), so the convention is established.

Setting review_passed = false due to the unresolved line 109 grammar nit (raised across three reviews) and the missing screenshots called out as a pre-merge blocker. No new inline comments posted — the existing inline comments at line 109 and line 143 still apply, and the file has not changed since the prior review.

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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 0s —— View job


Review complete

  • Read the latest changed file
  • Check status of unresolved blockers from prior reviews
  • Review for new typos / grammar / formatting issues
  • Post final summary

The latest commit (c00e2c1 — "docs(merge-queue): fix passive voice on shared-targets sentence") resolved the line 109 grammar nit that had been raised across three prior review rounds. Line 109 now reads:

Overlapping targets explain why PRs are in the same testing sequence: PRs with shared targets must be tested together.

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):

  • ✅ Structural placement — ### View impacted targets moved under ## Pull request details (line 141), promoted from #### to ### (resolved in 2743528)
  • ✅ Open question ok #1 (toggle label) — Show targets on hover (resolved in e968ed1)
  • ✅ Open question Fix links #3 (Single vs. Parallel mode) — line 143 explicitly scopes the modal to Parallel mode (resolved in e968ed1)
  • ✅ Empty-list modal semantics — line 147 reads "PR will not depend on anything, and nothing will depend on it" (resolved in e968ed1)
  • ✅ Line 106 wording — "surfaces impacted targets data" (resolved in d2dda5b)
  • ✅ Line 145 IMPACTS_ALL formatting — backticks applied (resolved in d2dda5b)
  • ✅ Line 109 grammar — "must be tested together" (resolved in c00e2c1)
  • ✅ Merge conflict from triage (resolved by merge commit 0c34be7)

Blocking concern still unresolved:

  1. Open question ok #2 (screenshots) — The verification-status comment from 2026-05-06 lists screenshots as a pre-merge blocker. Two figures are still missing:

    • Show targets on hover toggle in the graph view (line 109)
    • View Impacted Targets button + modal on the PR detail page (line 143)

    Adjacent sections in this file already use <figure> blocks (e.g., line 137), so the convention is established. If the screenshots are genuinely unavailable, suggest noting that explicitly and downgrading the verification-status entry, otherwise the pre-merge blocker stands.

Setting review_passed = false solely on the missing screenshots, which were called out as a pre-merge blocker in the verification-status comment. All other prior-round feedback is now addressed.
Branch

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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 3s —— View job


Review complete

  • Read the latest changed file
  • Check status of unresolved blockers from prior reviews
  • Review for new typos / grammar / formatting issues
  • Post final summary

The latest commit (69ae774 — "chore(vale): accept 'tooltip' / 'tooltips' in terms vocabulary") only updates the Vale vocabulary to accept the word tooltips used at line 108. git diff c00e2c1..HEAD -- merge-queue/using-the-queue/monitor-queue-status.md is empty — no content changes since the 07:19 review. The vocabulary change is correctly scoped (case-insensitive, optional plural) and the alphabetical placement between toolset and (?i)trivy is right.

Resolved across earlier commits (for reference):

  • ✅ Structural placement — ### View impacted targets moved under ## Pull request details (line 141), promoted from #### to ### (resolved in 2743528)
  • ✅ Open question ok #1 (toggle label) — Show targets on hover (resolved in e968ed1)
  • ✅ Open question Fix links #3 (Single vs. Parallel mode) — line 143 explicitly scopes the modal to Parallel mode (resolved in e968ed1)
  • ✅ Empty-list modal semantics — line 147 reads "PR will not depend on anything, and nothing will depend on it" (resolved in e968ed1)
  • ✅ Line 106 wording — "surfaces impacted targets data" (resolved in d2dda5b)
  • ✅ Line 145 IMPACTS_ALL formatting — backticks applied (resolved in d2dda5b)
  • ✅ Line 109 grammar — "must be tested together" (resolved in c00e2c1)
  • ✅ Vale vocabulary — tooltips accepted (resolved in 69ae774)
  • ✅ Merge conflict from triage (resolved by merge commit 0c34be7)

Blocking concern still unresolved:

  1. Open question ok #2 (screenshots) — The verification-status comment from 2026-05-06 lists screenshots as a pre-merge blocker. Two figures are still missing:

    • Show targets on hover toggle in the graph view (line 109)
    • View Impacted Targets button + modal on the PR detail page (line 143)

    Adjacent sections in this file already use <figure> blocks (e.g., line 137), so the convention is established. If the screenshots are genuinely unavailable, suggest noting that explicitly and downgrading the verification-status entry; otherwise the pre-merge blocker stands.

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 review_passed = false solely on the missing screenshots, which were called out as a pre-merge blocker in the verification-status comment.
Branch

@samgutentag
Copy link
Copy Markdown
Member Author

Pre-merge blockers update (2026-05-07)

All three blockers from the 2026-05-06 verification status comment are now resolved:

  • Merge conflict — resolved by 0c34be7 (merged main into branch, kept Priority badges + new Impacted targets sections coexisting under Graph view).
  • Three open questions from PR description:
    • Toggle label: confirmed as Show targets on hover (graph-toolbar.tsx:83), copy updated in e968ed1.
    • Single vs Parallel mode: confirmed Parallel-only (hasImpactedTargets undefined in Single mode, readiness_checker.ts:374-385), copy clarified in e968ed1.
    • Screenshots: deferred to follow-up PR — content is correct and reviewable as text. Native dashboard screenshots can be captured once the docs site is rendered.
  • Structural placement### View impacted targets moved under ## Pull request details in 2743528, where the modal actually lives.

Plus a few extras picked up during review:

  • ✅ Button label corrected to View Impacted Targets (Title Case, matches merge-item-header.tsx:71).
  • ✅ Empty-list semantics rewritten to match view-impacted-targets-modal.tsx:75-79.
  • ✅ Line 109 grammar fix (must be tested together).
  • ✅ Vale dictionary updated to accept tooltip / tooltips.

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
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @samgutentag's task in 1m 3s —— View job


Review complete

  • Read the latest changed files
  • Verify resolution of all prior-round blockers
  • Review for any new issues
  • Post final summary

The latest commit (69968ba — "chore: re-trigger Claude review after clearing pre-merge blockers") is a no-op for content; git diff c00e2c1..HEAD -- merge-queue/using-the-queue/monitor-queue-status.md is empty. The author's pre-merge blockers update explicitly downgrades the screenshot requirement with rationale, which is the path I suggested in the 07:19 review: "If the screenshots are genuinely unavailable, suggest noting that explicitly and downgrading the verification-status entry."

The deferral rationale holds up:

  • Content is reviewable as text (no visual ambiguity in the prose).
  • Native dashboard screenshots benefit from the rendered docs site for proper aspect/zoom.
  • All UI labels are confirmed against the code (graph-toolbar.tsx:83, merge-item-header.tsx:71, view-impacted-targets-modal.tsx:75-79).
  • A follow-up PR is the right venue, and the verification-status comment serves as the audit trail.

All prior-round blockers resolved (final check):

  • ✅ Structural placement — ### View impacted targets at line 141 under ## Pull request details (2743528)
  • ✅ Toggle label — Show targets on hover (e968ed1)
  • ✅ Single vs Parallel mode scope — line 143 explicitly Parallel-only (e968ed1)
  • ✅ Empty-list semantics — line 147 matches view-impacted-targets-modal.tsx:75-79 (e968ed1)
  • ✅ Line 106 wording — "surfaces impacted targets data" (d2dda5b)
  • ✅ Line 145 IMPACTS_ALL formatting — backticks applied (d2dda5b)
  • ✅ Line 109 grammar — "must be tested together" (c00e2c1)
  • ✅ Vale vocabulary — (?i)tooltips? accepted at accept.txt:227 (69ae774)
  • ✅ Merge conflict — resolved by merge commit 0c34be7
  • ✅ Screenshots — deferred with justification (per author's 2026-05-07 update)

No new issues in the latest commits. No new inline comments to post.

Setting review_passed = true. Ready for the merge queue.

Branch

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: Claude code review passed.

@trunk-staging-io trunk-staging-io Bot merged commit 236adef into main May 7, 2026
7 checks passed
@trunk-staging-io trunk-staging-io Bot deleted the sam-gutentag/impacted-targets-graph-ui branch May 7, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant