Skip to content

Commit 615cb13

Browse files
Valentin Grünerclaude
andcommitted
refactor(webapp): drop what the profile never rendered
`PracticeTrendPanel` had no caller outside its own story and test: the detail page shows `PracticeTrendChip` and nothing else. Its 214 lines went, and with them `formatTrendGap` and `formatTrendCoverage`, which nothing else called, and the `barClassName` and `trendPolarity` fields its stacked bar was the only reader of. `isVerdictStanding` and `ringClass` were exported and never read. `ReviewRunTimeline` also carried an empty state its only caller made unreachable — and a worse one: it said "matches these filters" whether or not a filter was set, while the caller already tells the two apart. The rule moves into a comment, and the case the deleted test named is now asserted where the decision is actually made. The trend provenance sentence counts "pieces of reviewed work" rather than "work items", which is what `practice-feedback-language.md` calls the unit. Also fixes the operator note on the practice-standing changeset, which told callers to replace `/practice-groups` with `/practice-groups`. The retired path is `/practice-areas`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAQxdMw579siMh1wXKEEh4
1 parent 561253b commit 615cb13

9 files changed

Lines changed: 43 additions & 79 deletions

File tree

.changeset/practice-standing-feedback-api.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,4 @@
44

55
The application API can now answer where a developer stands in each practice group: the current standing and its guidance, how the group has developed across recently reviewed work, which kinds of work contributed feedback, a filterable observation history, and the complete review runs behind it. An undecided observation remains visible in that history without being presented as a verdict. Developers can also replace or delete their response to delivered feedback, recording whether it was helpful, how they handled it, and an optional explanation. Every endpoint answers only for the signed-in developer.
66

7-
**Operators:** direct API callers must replace `/practice-groups` with `/practice-groups`, use the corresponding group schema and field names, replace `/practices/developer` with `/practices/reviewed`, and move from the older reaction endpoint to the combined response endpoint. Existing response history is preserved; `MIGRATION.md` lists the contract changes. The generated Hephaestus web client is updated in the same release.
7+
**Operators:** direct API callers must replace `/practice-areas` with `/practice-groups`, use the corresponding group schema and field names, replace `/practices/developer` with `/practices/reviewed`, and move from the older reaction endpoint to the combined response endpoint. Existing response history is preserved; `MIGRATION.md` lists the contract changes. The generated Hephaestus web client is updated in the same release.

webapp/src/components/profile/PracticeGroupDetailPage.test.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,27 @@ describe("PracticeGroupDetailPage", () => {
5757
expect(onSelectPractice).toHaveBeenCalledWith("small-changes");
5858
});
5959

60+
it("tells an empty filtered feed apart from a group that was never reviewed", () => {
61+
// One empty feed, two meanings. Saying "no review runs match your filters" to someone who set
62+
// none reads as a fault in the page; saying "none yet" to someone filtering hides their filter.
63+
const { rerender } = render(
64+
<PracticeGroupDetailPage group={group} reviewRuns={[]} isLoading={false} />,
65+
);
66+
screen.getByText("Review runs appear here once your work has been reviewed.");
67+
68+
rerender(
69+
<PracticeGroupDetailPage
70+
group={group}
71+
reviewRuns={[]}
72+
reviewRunFilters={{ sources: ["scm.pull_request"], severities: [] }}
73+
isLoading={false}
74+
/>,
75+
);
76+
screen.getByText(
77+
"No review runs match the current filters. Clear them to see every review in this group.",
78+
);
79+
});
80+
6081
it("offers navigation when the group is missing", () => {
6182
const onBack = vi.fn();
6283
render(<PracticeGroupDetailPage isLoading={false} onBack={onBack} />);

webapp/src/components/profile/ReviewObservationRow.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ import { Button } from "@/components/ui/button";
1515
import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@/components/ui/collapsible";
1616
import { Skeleton } from "@/components/ui/skeleton";
1717
import { cn } from "@/lib/utils";
18-
import { EvidenceFileBlock } from "./EvidenceFileBlock";
1918
import { toEvidenceLocations } from "./evidence";
19+
import { EvidenceFileBlock } from "./EvidenceFileBlock";
2020
import {
2121
OBSERVATION_OUTCOME_PRESENTATION,
2222
type ObservationOutcome,

webapp/src/components/profile/ReviewRunTimeline.tsx

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { PracticeGroupReviewObservation, PracticeGroupReviewRun } from "@/api/types.gen";
2-
import { ReviewRunCard } from "./ReviewRunCard";
32
import type { FeedbackUsefulness, ObservationDetailState } from "./review-runs";
3+
import { ReviewRunCard } from "./ReviewRunCard";
44

55
export type { FeedbackUsefulness, ObservationDetailState } from "./review-runs";
66

@@ -24,14 +24,8 @@ export function ReviewRunTimeline({
2424
onChangeUsefulness,
2525
pendingFeedbackId,
2626
}: ReviewRunTimelineProps) {
27-
if (runs.length === 0) {
28-
return (
29-
<div className="rounded-lg border border-dashed p-6 text-center text-sm text-muted-foreground">
30-
No reviewed work matches these filters.
31-
</div>
32-
);
33-
}
34-
27+
// No empty state here: the caller decides what an empty feed means — no runs at all, or none
28+
// matching the filters — and renders that itself. A second one here could only ever be wrong.
3529
return (
3630
<ol className="flex min-w-0 flex-col" aria-label="Review runs">
3731
{runs.map((run) => (

webapp/src/components/profile/observation-outcome.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@ describe("observation outcome contract", () => {
1515
// the practice did not apply to this work, INCONCLUSIVE means it did and the reviewer could not
1616
// tell. Collapsing them would report "not assessed" for a practice that WAS looked at.
1717
expect(observationOutcome({ presence: "INCONCLUSIVE" })).toBe("INCONCLUSIVE");
18-
expect(OBSERVATION_OUTCOME_PRESENTATION.INCONCLUSIVE.trendPolarity).toBeNull();
18+
expect(OBSERVATION_OUTCOME_PRESENTATION.INCONCLUSIVE.label).not.toBe(
19+
OBSERVATION_OUTCOME_PRESENTATION.NOT_APPLICABLE.label,
20+
);
1921
});
2022

2123
it("presents every outcome, including the one the outcome vector does not count", () => {
2224
// The first five mirror the server's OutcomeVector 1:1. INCONCLUSIVE is display-only — it is shown
23-
// to the developer but contributes no cell to the trend, which is why it carries no polarity.
25+
// to the developer but contributes no cell to the trend.
2426
expect(Object.keys(OBSERVATION_OUTCOME_PRESENTATION)).toStrictEqual([
2527
"PRESENT_GOOD",
2628
"ABSENT_GOOD",

webapp/src/components/profile/observation-outcome.ts

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,46 +18,31 @@ export const OBSERVATION_OUTCOME_PRESENTATION = {
1818
PRESENT_GOOD: {
1919
label: "Strength shown",
2020
className: "text-success",
21-
barClassName: "bg-success",
22-
trendPolarity: 1,
2321
},
2422
ABSENT_GOOD: {
2523
label: "Risk avoided",
2624
className: "text-success",
27-
barClassName: "bg-success/45",
28-
trendPolarity: 1,
2925
},
3026
PRESENT_BAD: {
3127
label: "Problem observed",
3228
className: "text-destructive",
33-
barClassName: "bg-destructive",
34-
trendPolarity: -1,
3529
},
3630
ABSENT_BAD: {
3731
label: "Expected practice missing",
3832
className: "text-destructive",
39-
barClassName: "bg-destructive/45",
40-
trendPolarity: -1,
4133
},
4234
NOT_APPLICABLE: {
4335
label: "Not assessed",
4436
className: "text-muted-foreground",
45-
barClassName: "bg-muted-foreground/30",
46-
trendPolarity: null,
4737
},
4838
// A distinct silence from NOT_APPLICABLE: the reviewer looked at work that DID offer the opportunity
4939
// and could not claim either way. Collapsing the two would report "no opportunity" for a practice the
5040
// reviewer was simply unsure about.
5141
INCONCLUSIVE: {
5242
label: "Not certain enough to say",
5343
className: "text-muted-foreground",
54-
barClassName: "bg-muted-foreground/30",
55-
trendPolarity: null,
5644
},
57-
} as const satisfies Record<
58-
ObservationOutcome,
59-
{ label: string; className: string; barClassName: string; trendPolarity: 1 | -1 | null }
60-
>;
45+
} as const satisfies Record<ObservationOutcome, { label: string; className: string }>;
6146

6247
/**
6348
* Derives the complete 2×2 observation state plus the two verdict-less states. The API guarantees no
Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
import { describe, expect, it } from "vitest";
22
import type { TrendSupport } from "@/api/types.gen";
3-
import {
4-
formatTrendCoverage,
5-
formatTrendGap,
6-
formatTrendProvenance,
7-
} from "./practice-trend-presentation";
3+
import { formatTrendProvenance } from "./practice-trend-presentation";
84

95
const support = (overrides: Partial<TrendSupport> = {}): TrendSupport => ({
106
currentOpportunities: 4,
@@ -19,7 +15,7 @@ const support = (overrides: Partial<TrendSupport> = {}): TrendSupport => ({
1915
describe("practice trend copy", () => {
2016
it("formats a complete comparison from support", () => {
2117
expect(formatTrendProvenance(support({ calendarSpanDays: 9 }))).toBe(
22-
"Compared your latest 4 reviewed work items with the 4 before them, spanning 9 days.",
18+
"Compared your latest 4 pieces of reviewed work with the 4 before them, spanning 9 days.",
2319
);
2420
});
2521

@@ -28,31 +24,11 @@ describe("practice trend copy", () => {
2824
formatTrendProvenance(
2925
support({ currentOpportunities: 1, previousOpportunities: 0, calendarSpanDays: 1 }),
3026
),
31-
).toBe("Based on 1 reviewed work item, spanning 1 day.");
27+
).toBe("Based on 1 piece of reviewed work, spanning 1 day.");
3228
expect(
3329
formatTrendProvenance(
3430
support({ currentOpportunities: 0, previousOpportunities: 0, calendarSpanDays: undefined }),
3531
),
36-
).toBe("No reviewed work items are available yet.");
37-
});
38-
39-
it("formats singular, plural, and zero comparison gaps", () => {
40-
expect(formatTrendGap(support({ opportunitiesUntilComparable: 1 }))).toBe(
41-
"1 more reviewed work item will make a comparison possible.",
42-
);
43-
expect(formatTrendGap(support({ opportunitiesUntilComparable: 2 }))).toBe(
44-
"2 more reviewed work items will make a comparison possible.",
45-
);
46-
expect(formatTrendGap(support())).toBe("A comparison is available.");
47-
});
48-
49-
it("formats group coverage and omits it for a practice", () => {
50-
expect(formatTrendCoverage(support({ comparablePractices: 3, eligiblePractices: 5 }))).toBe(
51-
"3 of 5 practices in this group had comparable evidence.",
52-
);
53-
expect(formatTrendCoverage(support({ comparablePractices: 1, eligiblePractices: 1 }))).toBe(
54-
"1 of 1 practice in this group had comparable evidence.",
55-
);
56-
expect(formatTrendCoverage(support())).toBeUndefined();
32+
).toBe("No reviewed work is available yet.");
5733
});
5834
});

webapp/src/components/profile/practice-trend-presentation.ts

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -40,39 +40,24 @@ export const PRACTICE_TREND_PRESENTATION = {
4040
},
4141
} as const satisfies Record<TrendDirection, TrendPresentation>;
4242

43-
function reviewedItems(count: number): string {
44-
return `${count} reviewed ${count === 1 ? "work item" : "work items"}`;
43+
/** The counted unit is a *piece of reviewed work* — `practice-feedback-language.md` names it. */
44+
function reviewedWork(count: number): string {
45+
return `${count} ${count === 1 ? "piece" : "pieces"} of reviewed work`;
4546
}
4647

4748
/** Formats visible provenance from the returned evidence support, never from an assumed time bin. */
4849
export function formatTrendProvenance(support: TrendSupport): string {
4950
const current = support.currentOpportunities;
5051
const previous = support.previousOpportunities;
5152
const total = current + previous;
52-
if (total === 0) return "No reviewed work items are available yet.";
53+
if (total === 0) return "No reviewed work is available yet.";
5354

5455
const span = support.calendarSpanDays;
5556
const spanText = span ? `, spanning ${span} ${span === 1 ? "day" : "days"}` : "";
5657
if (previous === 0) {
57-
return `Based on ${reviewedItems(current)}${spanText}.`;
58+
return `Based on ${reviewedWork(current)}${spanText}.`;
5859
}
59-
return `Compared your latest ${reviewedItems(current)} with the ${previous} before ${
60+
return `Compared your latest ${reviewedWork(current)} with the ${previous} before ${
6061
previous === 1 ? "it" : "them"
6162
}${spanText}.`;
6263
}
63-
64-
/** Formats the unmet evidence precondition as a concrete next milestone. */
65-
export function formatTrendGap(support: TrendSupport): string {
66-
const missing = support.opportunitiesUntilComparable;
67-
if (missing === 0) return "A comparison is available.";
68-
return `${missing} more reviewed ${missing === 1 ? "work item" : "work items"} will make a comparison possible.`;
69-
}
70-
71-
/** Formats group coverage; practice-scoped support has no coverage sentence. */
72-
export function formatTrendCoverage(support: TrendSupport): string | undefined {
73-
const comparable = support.comparablePractices;
74-
const eligible = support.eligiblePractices;
75-
if (comparable === undefined || eligible === undefined) return undefined;
76-
if (eligible === 0) return "No practices are eligible for comparison in this group.";
77-
return `${comparable} of ${eligible} ${eligible === 1 ? "practice" : "practices"} in this group had comparable evidence.`;
78-
}

webapp/src/routes/_authenticated/w/$workspaceSlug/user/$username/practice-groups.$groupSlug.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
} from "@/components/profile/PracticeGroupDetailPage";
2121
import type { ObservationDetailState } from "@/components/profile/review-runs";
2222
import { useAuth } from "@/integrations/auth/AuthContext";
23+
import { loadedPages } from "@/integrations/tanstack-query/spring-page";
2324
import { problemDetailOf } from "@/lib/problem-detail";
2425

2526
const ACTIVITY_PAGE_SIZE = 10;
@@ -144,7 +145,7 @@ function PracticeGroupDetail() {
144145
return [practice.slug, nextStep];
145146
}),
146147
);
147-
const reviewRuns = activityQuery.data.pages.flatMap((page) => page.content);
148+
const reviewRuns = loadedPages(activityQuery.data).flatMap((page) => page.content);
148149

149150
const observationDetail: ObservationDetailState | undefined = openObservationId
150151
? {

0 commit comments

Comments
 (0)