Skip to content

Rework commit deploy indicators on service charts#138

Open
JeremyFunk wants to merge 4 commits into
mainfrom
feat/commit-indicator-rework-v2
Open

Rework commit deploy indicators on service charts#138
JeremyFunk wants to merge 4 commits into
mainfrom
feat/commit-indicator-rework-v2

Conversation

@JeremyFunk

Copy link
Copy Markdown
Collaborator

Summary

Reworks the commit/deploy indicators on the service dashboard charts into a dedicated, unit-tested overlay layer. The old single-marker approach (commit-marker.tsx, release-markers.ts, reference-markers.tsx) is replaced by a commit-markers/ module that separates pure layout geometry from the React/recharts overlay.

What changed

  • New commit-markers/ module (apps/web/src/components/vcs/commit-markers/):
    • marker-layout.ts — pure geometry/derivation (bucket → marker, label layout, sha shortening), kept free of React so it unit-tests cleanly. Covered by marker-layout.test.ts.
    • commit-markers-layer.tsx — thin recharts overlay rendering dashes + labels, with hover-card coordination and zIndex layering.
    • use-commit-markers.tsx — hook wiring commit data into the layer.
  • Richer hover card (commit-sha-hover-card.tsx) — CommitDetailBody / CommitListBody for single vs. clustered commits.
  • Chart integration — apdex / error-rate / throughput / latency charts now consume the shared overlay; packages/ui chart.tsx gains tooltip-suppression coordination so the commit card and the data tooltip don't fight.
  • Removed the superseded commit-marker.tsx, release-markers.ts(+test), and _shared/reference-markers.tsx.

Notes

  • Labels short-sha full git SHAs but leave tags/version/non-hex deploy ids intact; the full SHA is always one hover away.
  • Local dev tooling (scripts/ingest-dummy.ts, ingest:dummy script) used to test this was intentionally left out of the PR.

🤖 Generated with Claude Code

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — two minor cleanup observations below.

Reviewed changes — the rework of the service-detail charts' commit/deploy indicators from a per-reference-line marker into a dedicated, unit-tested overlay module.

  • New commit-markers/ modulemarker-layout.ts holds pure geometry (buildCommitMarkers first-seen-per-commit + bucket snapping, layoutMarkerLabels greedy non-overlapping sweep, estimateLabelWidth, shortLabel) kept React-free and covered by marker-layout.test.ts; commit-markers-layer.tsx is the recharts overlay (hoverable chip row that overflows above the plot, per-group hover timers, anchored MarkerCard); use-commit-markers.tsx wires releases→markers and resolves the representative SHA to a message subject in the background.
  • Richer hover cardcommit-sha-hover-card.tsx exports CommitDetailBody (with a compact variant) and adds CommitListBody/CommitListRow* for clustered deploys plus CommitPlain for non-resolvable refs; deriveAvatarUrl, formatExact, formatRelativeShort extracted.
  • Tooltip suppressionpackages/ui chart.tsx gains ChartTooltipSuppressionProvider/useSuppressChartTooltip (id-keyed) so a marker card quiets the data tooltip across a synced grid, keeping the tooltip mounted-transparent so its follow transition resumes rather than snapping from the origin.
  • Chart surface changechart-types.ts drops ChartReferenceLine + referenceLines/renderReferenceMarker and adds overlay, yAxisWidth, and MARKER_OVERLAY_CLASS; the four time-series charts render {overlay} and honor yAxisWidth; metrics-grid.tsx threads both through and $serviceName.tsx pins yAxisWidth={72}.
  • API + removalsServiceDetailOverviewResult.releases widened to string and passed raw (snapping handles format); removed commit-marker.tsx, release-markers.ts(+test), _shared/reference-markers.tsx, and the web wrapper getServiceReleasesTimeline (all references grep-clean).

ℹ️ Orphaned serviceReleases endpoint after the web wrapper removal

This PR removes getServiceReleasesTimeline, the only web-side consumer of the standalone serviceReleases HTTP endpoint. The releases data the charts need now rides along on serviceDetailOverview, so the dedicated endpoint, its request schema, and the atom-client method it backs are dead public surface on the web side.

The underlying ClickHouse query is still shared, so nothing breaks — but the standalone HTTP route is now unreachable from the app.

Technical details
# Orphaned `serviceReleases` endpoint

## Affected sites
- `apps/api/src/routes/query-engine.http.ts:445` — standalone `serviceReleases` handler; no web caller remains (grep for `.serviceReleases(` under `apps/web` returns nothing).
- `packages/domain/src/http/query-engine.ts:242` / `:1297``ServiceReleasesRequest` class + `POST /service-releases` endpoint definition, now only reachable by the dead handler.
- KEEP: `packages/query-engine/src/ch/queries/services.ts:130` `serviceReleasesTimelineQuery` — still used by the `serviceDetailOverview` handler (`query-engine.http.ts:561`). Do NOT remove this.

## Required outcome
- Either drop the now-unreachable standalone `serviceReleases` endpoint + `ServiceReleasesRequest` + atom-client method, or leave a note documenting it as an intentionally-kept public API.

## Open questions for the human
- Is the standalone `serviceReleases` endpoint a supported public API surface (e.g. consumed outside this repo), or purely an internal helper that can be deleted now that the overview query subsumes it?

ℹ️ Nitpicks

  • firstLine is defined identically in both apps/web/src/components/vcs/commit-markers/use-commit-markers.tsx:11 and apps/web/src/components/vcs/commit-sha-hover-card.tsx:289. The latter already exports commitQueryAtom/isResolvableSha, so exporting firstLine from it and importing would drop the duplicate.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix it ➔View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — the aa3868e "some cleanup" commit on top of the previously-reviewed overlay rework. These are all polish/consolidation, including a fix for the prior review's firstLine duplication nit.

  • Deduped firstLinecommit-sha-hover-card.tsx now exports firstLine and use-commit-markers.tsx imports it, dropping the duplicate helper flagged in the prior review.
  • Consolidated relative-time formattingformatRelativeShort and formatRelative merged into a single formatRelative(epochMs, { short }); default keeps the "3h ago" form with a seconds tier and "just now" floor, short keeps the "3h" / "now" form. Both call sites updated.
  • Capped the cluster badgecommit-markers-layer.tsx renders +{Math.min(badge, 99)} so a 100+ commit bucket can't exceed the fixed BADGE_PX reserved width (estimateLabelWidth uses the same constant, so layout and render stay in agreement).
  • Popover theme tokens — hover-card / chip surfaces switched from bg-background/foreground to bg-popover/popover-foreground (plus a backdrop-blur-sm and a moved cursor-pointer).
  • Extracted overlayChartClassNamechart-types.ts gains a shared helper replacing the inline overlay ? ... : className ternary across the apdex / error-rate / throughput / latency charts; behavior is identical.

The consolidated formatRelative and the overlayChartClassName extraction are behavior-preserving, and the badge cap is self-consistent with the layout width estimate. The orphaned serviceReleases endpoint noted in the prior review is unchanged by this commit and remains an open question for the author.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — the dc959ac..061c3f3 "more fixes" commit on top of the previously-reviewed overlay rework. Two substantive moves and one cleanup, all sound.

  • Avatar derivation moved frontend → backendGithubProvider.ts gains githubAvatarUrl(htmlUrl, login), which resolves <host>/<login>.png?size=64 against the commit's own html URL (so github.com and GitHub Enterprise both stay correct) and returns null when there's no login or the URL can't be parsed. It's wired into both normalizeFetchedCommit (REST avatar_url fallback) and the push-webhook path (previously hardcoded null), and the web-side deriveAvatarUrl fallback is dropped so both hover-card call sites read commit.authorAvatarUrl directly.
  • Avatar test coverage — three new vcs.test.ts cases assert the github.com host, the GHE host, and the null-login fallback for push commits.
  • Tooltip suppression timing — in commit-markers-layer.tsx, suppression and blockChart now key off openKey rather than hoverKey, so merely passing over a dash no longer blinks the chart's data tooltip away; only an opened card quiets it. hoverKey still drives the card's active highlight, so the hover affordance is intact.
  • Removed orphaned web wrappergetServiceReleasesTimeline and its now-unused ServiceReleasesRequest import are gone from services.ts, partially addressing the prior review's orphaned-endpoint observation on the web side.

The avatar host derivation is correct (origin taken from the commit URL, not hardcoded), the suppression change is internally consistent, and the removals are grep-clean. The standalone serviceReleases HTTP endpoint + ServiceReleasesRequest domain class remain (this commit removed only the last web wrapper), but that's the unchanged carry-forward from the prior review and stays an open question for the author — not a new issue.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — the 061c3f3..f1f2030 "add commit sha cache" commit on top of the previously-reviewed overlay rework. A single, well-scoped caching fix to the per-SHA commit-detail query atom, plus a guard test.

  • Per-SHA atom now memoized + TTL'dcommitQueryAtom in commit-sha-hover-card.tsx changes from a bare (sha) => MapleApiAtomClient.query(...) into Atom.family((sha) => ...query(..., { timeToLive: COMMIT_DETAIL_TTL_MS })). The Atom.family string-key memoization is what actually lets the prefetch subscriber, the popup body, the deploy markers, and the commit-list rows resolve to one shared atom — one in-flight fetch, one cached result — instead of each minting a fresh request object and refetching.
  • Idle TTLCOMMIT_DETAIL_TTL_MS (5 min) is threaded as timeToLive, which AtomHttpApi maps to Atom.setIdleTTL for finite durations. Re-subscribing resets the timer, so a commit in active view never expires while truly-idle atoms are swept — bounding family growth.
  • Memoization contract test — new commit-sha-hover-card.test.tsx (jsdom) asserts same SHA → identical atom instance and distinct SHA → distinct instance, the exact invariant the bare-function implementation violated.

The timeToLivesetIdleTTL mapping checks out (AtomHttpApi.ts: finite → idle TTL, infinite → keep-alive), so the documented "resets on re-subscribe, swept after last reader unmounts" behavior is accurate and the family won't leak. The standalone serviceReleases HTTP endpoint flagged in the initial review is untouched by this commit and remains the author's open call — not a new issue.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Claude Opus𝕏

Comment on lines +63 to +68
useEffect(() => {
const text = Result.builder(result)
.onSuccess((commit) => firstLine(commit.message))
.orElse(() => null)
if (text) onResolved(sha, text)
}, [result, sha, onResolved])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this should be a useCallback instead of this and build diff this seems hacky i think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants