fix(webapp): preserve complete practice feedback responses (#1698)

Author: FelixTJDietrich

.changeset/practice-group-response-hardening.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+Disputing feedback now waits for the required explanation before saving the response, and selected practices and expanded observations remain available when a practice-group page is refreshed or shared.

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

@@ -4,4 +4,4 @@
 
 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.
 
-**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.
+**Operators:** direct API callers must replace `/practice-areas` with `/practice-groups`, use the corresponding group schema and field names, replace `/practices/learner` 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.

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

@@ -6,19 +6,21 @@ bundled web client need no changes; it ships updated in this release.
 
 **Before**: practice groupings were served under `/workspaces/{workspaceSlug}/practice-areas`, with
 `PracticeArea` schemas and `areaSlug` parameters. A developer's practices came from
-`/workspaces/{workspaceSlug}/practices/developer`, and a reaction to delivered feedback was recorded
+`/workspaces/{workspaceSlug}/practices/learner`, and a reaction to delivered feedback was recorded
 through its own endpoint.
 
 **After**: the same groupings are served under `/workspaces/{workspaceSlug}/practice-groups`, with
-`PracticeGroup` schemas and `groupSlug` parameters — *practice area*, `PracticeArea`, `areaSlug` and
+`PracticeGroup` schemas and `groupSlug` parameters — _practice area_, `PracticeArea`, `areaSlug` and
 `/practice-areas` are retired names, not synonyms, and no alias remains. The developer practice list
 is `/workspaces/{workspaceSlug}/practices/reviewed`. A developer's response to delivered feedback —
-whether it was helpful, how it was handled, and an optional explanation — is written through one
-combined response endpoint that replaces the earlier reaction endpoint. Every one of these answers
-only for the signed-in developer.
+whether it was helpful, how it was handled, and an optional explanation — is written with `PUT
+/workspaces/{workspaceSlug}/practices/feedback/{feedbackId}/response`. Map helpfulness to
+`usefulness`, handling to `resolution`, and the optional explanation to `comment`. This combined
+response endpoint replaces the earlier reaction endpoint. Every one of these answers only for the
+signed-in developer.
 
 **Migration**: update each caller's paths, parameter names and response field names to the group
-spelling, move any caller of `/practices/developer` to `/practices/reviewed`, and switch reaction
+spelling, move any caller of `/practices/learner` to `/practices/reviewed`, and switch reaction
 writes to the combined response endpoint. Response history recorded before the upgrade is preserved
 and readable through the new endpoint, so nothing needs re-entering. Regenerate any client
 built from `server/openapi.yaml`.

webapp/src/components/practice-vocabulary/feedback-resolution-defs.ts

@@ -5,15 +5,6 @@ import type { PracticeGroupReviewObservation } from "@/api/types.gen";
 import type { StatusDefs } from "./status-def";
 
 export type FeedbackResolution = NonNullable<PracticeGroupReviewObservation["feedbackResolution"]>;
-
-/**
- * What the developer did about a piece of delivered feedback — the half of a response about the
- * work, where `feedback-usefulness-defs` is about the review. The two are independent on the wire
- * and stay independent here.
- *
- * `DISPUTED` is the only value the server demands an explanation for, so its description says so
- * rather than leaving the reader to find out when the form refuses.
- */
 export const FEEDBACK_RESOLUTION_DEFS: StatusDefs<FeedbackResolution> = {
 	ADDRESSED: {
 		label: "Addressed",

webapp/src/components/practice-vocabulary/feedback-usefulness-defs.ts

@@ -5,12 +5,6 @@ import type { PracticeGroupReviewObservation } from "@/api/types.gen";
 import type { StatusDefs } from "./status-def";
 
 export type FeedbackUsefulness = NonNullable<PracticeGroupReviewObservation["feedbackUsefulness"]>;
-
-/**
- * Whether delivered feedback was worth receiving — the half of a response about the review, where
- * `feedback-resolution-defs` is about the work. Neither derives the other: feedback can be useful
- * and still not apply.
- */
 export const FEEDBACK_USEFULNESS_DEFS: StatusDefs<FeedbackUsefulness> = {
 	HELPFUL: {
 		label: "Helpful",

webapp/src/components/practice-vocabulary/practice-group-standing-defs.ts

@@ -15,15 +15,6 @@ export type PracticeGroupStandingValue = PracticeGroupStanding["standing"];
 export interface PracticeGroupStandingDef extends StatusDef {
 	shortLabel: string;
 }
-
-/**
- * Where a developer stands in a practice group, worst first. `DEVELOPING` means problems
- * *predominate* — `MIXED` has them too — and `NO_OPPORTUNITY` covers evidence that settled nothing
- * as well as work that offered no occasion.
- *
- * Both silences render outline, so the icon separates them: dashed for "nothing seen yet", a slash
- * for "nothing to see". `shortLabel` is the ring legend's wording, where five entries share a row.
- */
 export const PRACTICE_GROUP_STANDING_DEFS: Record<
 	PracticeGroupStandingValue,
 	PracticeGroupStandingDef

webapp/src/components/practice-vocabulary/practice-trend-defs.ts

@@ -5,11 +5,6 @@ import type { PracticeTrend } from "@/api/types.gen";
 import type { StatusDefs } from "./status-def";
 
 export type TrendDirection = PracticeTrend["direction"];
-
-/**
- * Which way recent evidence points. `UNCERTAIN` means the comparison was made and did not separate;
- * `INSUFFICIENT_EVIDENCE` that none was possible. Both render muted, so the icon separates them.
- */
 export const PRACTICE_TREND_DEFS: StatusDefs<TrendDirection> = {
 	IMPROVING: {
 		label: "More positive recently",

webapp/src/components/profile/EvidenceFileBlock.stories.tsx

@@ -45,8 +45,6 @@ export const Default: Story = {
 		},
 	},
 };
-
-/** A single named line, the shape most observations carry. */
 export const SingleLine: Story = {
 	args: {
 		location: {
@@ -60,8 +58,6 @@ export const SingleLine: Story = {
 		},
 	},
 };
-
-/** Collapsed: the second and later files of a observation start closed so the panel stays scannable. */
 export const Collapsed: Story = {
 	args: {
 		defaultOpen: false,
@@ -76,8 +72,6 @@ export const Collapsed: Story = {
 		},
 	},
 };
-
-/** The quote was withheld and the app cannot know why: it says only that, and names the place. */
 export const Redacted: Story = {
 	args: {
 		location: {
@@ -89,11 +83,6 @@ export const Redacted: Story = {
 		},
 	},
 };
-
-/**
- * The secret scanner is the one detector allowed to omit a quote, so here the reason *is* knowable
- * and gets said — the text was never stored, and the reader is pointed at the line to read it there.
- */
 export const RedactedBySecretScanner: Story = {
 	args: {
 		location: {
@@ -107,8 +96,6 @@ export const RedactedBySecretScanner: Story = {
 		detector: "secret-diff-scanner",
 	},
 };
-
-/** No directory to absorb truncation — the file name carries the whole identity. */
 export const BareFileName: Story = {
 	args: {
 		location: {
@@ -121,8 +108,6 @@ export const BareFileName: Story = {
 		},
 	},
 };
-
-/** A line far wider than the block: it scrolls, and the gutter stays pinned to the left edge. */
 export const LongLines: Story = {
 	args: {
 		location: {
@@ -138,12 +123,6 @@ export const LongLines: Story = {
 		},
 	},
 };
-
-/**
- * A quote from an object source. Its numbers are offsets into a serialised context file, not lines
- * of anything the reader could open, so no gutter and no range are shown — only the source and the
- * quote. This is the case that used to render a Slack message as though it were code.
- */
 export const ObjectSource: Story = {
 	args: {
 		location: {
@@ -160,12 +139,6 @@ export const ObjectSource: Story = {
 		},
 	},
 };
-
-/**
- * A quote from the old side of the change: the line is marked, because it is not what the file says
- * now. The new side carries no marker — it is where nearly every quote comes from, so labelling it
- * would say nothing.
- */
 export const QuotedFromBeforeTheChange: Story = {
 	args: {
 		location: {

webapp/src/components/profile/EvidenceFileBlock.tsx

@@ -7,25 +7,13 @@ import {
 import { Badge } from "@/components/ui/badge";
 import { cn } from "@/lib/utils";
 import { type EvidenceLocation, evidenceLineRangeLabel, splitPath } from "./evidence";
-
-/** The one detector the server lets omit a quote, so the reason is knowable when it is the one that ran. */
 const SECRET_SCANNER = "secret-diff-scanner";
 
 interface EvidenceFileBlockProps {
 	location: EvidenceLocation;
-	/**
-	 * Which detector produced the evidence. Only used to explain a withheld quote: for the secret
-	 * scanner the reason can be stated, for anything else the app does not know it and says so.
-	 */
 	detector?: string;
 	defaultOpen?: boolean;
 }
-
-/**
- * A quoted citation, rendered as what its source is. Only a `code` source is located by line: for an
- * `object` source the numbers index a stored copy, not a place the reader could open, so it gets no
- * gutter. `evidence-source-defs` owns that ruling.
- */
 export function EvidenceFileBlock({
 	location,
 	detector,
@@ -64,9 +52,6 @@ export function EvidenceFileBlock({
 						{evidenceLineRangeLabel(location)}
 					</span>
 				)}
-				{/* Only the old side is marked. Nearly every quote comes from the new one, so saying
-				    "after" on all of them is noise — while "before" is the thing a reader has to know:
-				    the line quoted here is not what the file says now. */}
 				{locatedByLine && location.side === "OLD" && (
 					<Badge variant="outline" className="shrink-0">
 						{DIFF_SIDE_LABELS.OLD}

webapp/src/components/profile/FeedbackComment.stories.tsx

@@ -29,24 +29,18 @@ const meta = {
 
 export default meta;
 type Story = StoryObj<typeof meta>;
-
-/** Nothing written yet: the field is optional, and there is nothing to save or cancel. */
 export const Empty: Story = {
 	play: async ({ canvas }) => {
 		await expect(canvas.getByRole("button", { name: "Save comment" })).toBeDisabled();
 		await expect(canvas.queryByRole("button", { name: "Cancel" })).toBeNull();
 	},
 };
-
-/** A comment already recorded. Saving stays disabled until the text actually differs. */
 export const Recorded: Story = {
 	args: { comment: "Split into two commits so the reasoning reads on its own." },
 	play: async ({ canvas }) => {
 		await expect(canvas.getByRole("button", { name: "Save comment" })).toBeDisabled();
 	},
 };
-
-/** Typing offers both actions; cancelling puts the recorded text back rather than clearing it. */
 export const Editing: Story = {
 	args: { comment: "Split into two commits." },
 	play: async ({ canvas, userEvent }) => {
@@ -58,11 +52,6 @@ export const Editing: Story = {
 		await expect(field).toHaveValue("Split into two commits.");
 	},
 };
-
-/**
- * Disputing an observation: the server rejects an empty explanation, so the field says it is
- * required and saving stays out of reach until something is written.
- */
 export const RequiredForDispute: Story = {
 	args: { isRequired: true },
 	play: async ({ args, canvas, userEvent }) => {
@@ -77,8 +66,6 @@ export const RequiredForDispute: Story = {
 		);
 	},
 };
-
-/** While the response is in flight every control is out of action, including cancel. */
 export const Saving: Story = {
 	args: { comment: "Handled in the follow-up.", isPending: true },
 };