HYDI-54: Append PR status suffixes for non-open links - #49
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
PR Review — Principal Engineer Assessment
Verdict: Ready with fixes
Risk: Medium
Scope: d1a3d66d9283a73f3b72f7d7d43dbc37ef74c2ac..fab425b9e8c7777ef8cff3f71c2a6bd6b07f1b33
Requirements: HYDI-54 via PR description (jira-view HYDI-54 unavailable in this environment)
Strengths
- PR display-state resolution correctly distinguishes merged/draft/open/closed using GitHub pull fields.
- Dashboard rendering logic appends explicit suffixes only when a non-open presentation state is available.
- Route-level tests cover merged/draft/closed/open label rendering regressions.
Architecture Assessment
- Vertical slice quality: Good end-to-end wiring (GitHub helper, route rendering, tests, docs) with graceful best-effort error handling.
- Deep module quality: Helper implementation is straightforward and consistent with existing pull-request client usage.
- Testability/reasoning quality: UI-output assertions are strong, but external-call scope/cost behavior is not covered.
Third-Party Contract Verification
- Verified intended usage of GitHub pull fields from pulls.get response: merged_at, state, and draft are sufficient to derive display state.
- No contract mismatch found in the field mapping itself.
Unmapped Findings
(findings whose lines are not in the diff — inline comment not possible)
- None.
Action Plan For Implementing Agent
- [REV-001] Limit PR display-state lookups to rows that can render PR links
actions:
- id: REV-001
severity: important
location: src/routes/dashboard.ts:271
change_required: Restrict lookups to runs that can actually render PR links (at minimum status === 'succeeded' with pr_url), and deduplicate by PR URL before calling GitHub.
acceptance_check: Add/extend a route test asserting getPullRequestDisplayState is not called for non-succeeded rows and that call count matches unique succeeded PR URLs rendered.| @@ -265,6 +270,20 @@ dashboardRouter.get("/", async (c) => { | |||
| } | |||
| }) | |||
There was a problem hiding this comment.
[REV-001] Important — Limit PR display-state lookups to rows that can render PR links
Problem: The dashboard prefetch loop calls getPullRequestDisplayState for every run that has a parseable pr_url, including non-succeeded rows that never render a PR link in the table.
Impact: Each dashboard request performs avoidable GitHub API calls, increasing latency and rate-limit pressure as run volume grows.
Required fix: Restrict lookups to runs that can actually render PR links (at minimum status === 'succeeded' with pr_url), and deduplicate by PR URL before calling GitHub.
Acceptance check: Add/extend a route test asserting getPullRequestDisplayState is not called for non-succeeded rows and that call count matches unique succeeded PR URLs rendered.
Implements HYDI-54
Changes
open,draft,merged, andclosedstates for dashboard rendering.(Merged),(Draft), or(Closed).Validation
npm --prefix /workspace/hyper-dispatch test -- src/routes/dashboard.test.tsnpm --prefix /workspace/hyper-dispatch testnpm --prefix /workspace/hyper-dispatch run test:coveragenpm --prefix /workspace/hyper-dispatch run typecheckCo-Authored-By: Oz oz-agent@warp.dev
Conversation: https://app.warp.dev/conversation/8334b925-81f8-4838-835e-ea710d9db62f
Run: https://oz.warp.dev/runs/019eb19a-e7f9-708a-97df-7940d5009cc8
This PR was generated with Oz.