feat(webapp): integrate practice findings into profile view - #952
Closed
FelixTJDietrich wants to merge 11 commits into
Closed
feat(webapp): integrate practice findings into profile view#952FelixTJDietrich wants to merge 11 commits into
FelixTJDietrich wants to merge 11 commits into
Conversation
Add a Practices section to the profile page that displays per-practice summary cards and a filterable, paginated findings list. Adjust the home route redirect to silently navigate to the profile when practices are enabled and the leaderboard is disabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a Practices feature: profile-integrated PracticeSection and UI components (summary grid/cards, findings list/items, badges, engagement overview), supporting hook-driven data (paginated queries, filters), fixtures, helpers, Storybook stories, sidebar integration, and home-route redirect/toast logic tied to the Changes
sequenceDiagram
participant Profile as ProfilePage
participant Section as PracticeSection
participant Hook as usePracticeFindings
participant API as Backend API
participant UI as SummaryGrid/FindingsList
Profile->>Section: render(workspaceSlug)
Section->>Hook: usePracticeFindings(workspaceSlug)
Hook->>API: fetch summaries, findings (paged), engagement
API-->>Hook: return summaries / pages / engagement
Hook-->>Section: provide data, handlers (onPracticeSelect, onVerdictChange, fetchMore, retry)
Section->>UI: render SummaryGrid + FindingsList + EngagementOverview
UI->>Hook: onPracticeSelect / onVerdictChange / fetchMore
Hook->>API: fetch next page or filtered data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
webapp
…s-profile-integration
Contributor
There was a problem hiding this comment.
Pull request overview
Integrates automated practice-detection “findings” into the existing profile page by adding a new Practices section (summary cards + filterable/paginated findings list) and refines the workspace home redirect UX when the leaderboard feature is disabled.
Changes:
- Add
PracticeSectionto the profile page, backed by a newusePracticeFindingshook using TanStack Query (summary + infinite findings). - Introduce new presentational practice UI components (badges, summary grid/cards, findings list/items) plus Storybook coverage.
- Adjust
/w/:workspaceSlug/behavior to silently redirect to profile when leaderboard is off but practices are enabled (info toast otherwise).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/routes/_authenticated/w/$workspaceSlug/index.tsx | Redirect/toast behavior updated for leaderboard-disabled workspaces, with practices-aware UX. |
| webapp/src/hooks/use-practice-findings.ts | New hook encapsulating feature gating, summary query, infinite findings query, and local filter state. |
| webapp/src/components/profile/ProfilePage.tsx | Appends PracticeSection below existing ProfileContent. |
| webapp/src/components/practice/verdict-styles.ts | Centralized verdict/severity style mappings + VerdictFilter runtime guard. |
| webapp/src/components/practice/VerdictBadge.tsx | New verdict pill badge component. |
| webapp/src/components/practice/VerdictBadge.stories.tsx | Storybook stories for verdict badge variants. |
| webapp/src/components/practice/SeverityBadge.tsx | New severity pill badge component. |
| webapp/src/components/practice/SeverityBadge.stories.tsx | Storybook stories for severity badge variants. |
| webapp/src/components/practice/PracticeSummaryGrid.tsx | Grid wrapper for practice summary cards + loading skeleton layout. |
| webapp/src/components/practice/PracticeSummaryGrid.stories.tsx | Storybook stories for summary grid states/variants. |
| webapp/src/components/practice/PracticeSummaryCard.tsx | Interactive summary card with selection state and ratio meter. |
| webapp/src/components/practice/PracticeSummaryCard.stories.tsx | Storybook stories for summary card variants. |
| webapp/src/components/practice/PracticeSection.tsx | Container component orchestrating loading/error/empty states and wiring hook → presentational children. |
| webapp/src/components/practice/PracticeSection.stories.tsx | Storybook story wrapper rendering the composed section with mock data. |
| webapp/src/components/practice/FindingsListItem.tsx | Row UI for an individual finding (badges + relative timestamp + verdict accent). |
| webapp/src/components/practice/FindingsListItem.stories.tsx | Storybook stories for list item variants (verdict/severity/edge cases). |
| webapp/src/components/practice/FindingsList.tsx | Filter controls (practice + verdict) and paginated list with “show more” CTA. |
| webapp/src/components/practice/FindingsList.stories.tsx | Storybook stories for findings list states (loading/empty/filtered/pagination). |
Comment on lines
+20
to
+22
| <Card className="border-l-3 p-4 gap-2 flex flex-col"> | ||
| <div className="flex items-center gap-2"> | ||
| <Skeleton className="h-5 w-24" /> |
There was a problem hiding this comment.
border-l-3 is not a standard Tailwind border-width utility (so the left accent likely won’t render). Use a supported width class (e.g. border-l-4) or an arbitrary value (e.g. border-l-[3px]) to ensure consistent styling.
Comment on lines
+38
to
+39
| <Card className={cn("border-l-3 p-4 gap-1.5 flex flex-col", verdictStyle.borderColor)}> | ||
| {/* Metadata row */} |
There was a problem hiding this comment.
border-l-3 is not a standard Tailwind border-width utility, so this verdict accent border may not apply. Switch to a supported class (e.g. border-l-4) or an arbitrary border width (e.g. border-l-[3px]).
Comment on lines
+49
to
+52
| aria-pressed={isSelected} | ||
| aria-label={`${practiceName}: ${positiveCount} positive, ${negativeCount} negative findings`} | ||
| aria-description="Click to filter findings by this practice" | ||
| className={cn( |
There was a problem hiding this comment.
aria-description has limited support across assistive technologies; for broader compatibility consider using aria-describedby pointing at visually-hidden helper text (or fold this hint into an existing aria-label).
| } | ||
|
|
||
| const meta = { | ||
| title: "Components/Practice/PracticeSection", |
There was a problem hiding this comment.
Most Storybook stories in this PR rely on auto-generated titles; this one sets an explicit title, which makes naming inconsistent and can become stale if the file path changes. Consider omitting title and letting Storybook derive it from the file/location (or align with whatever naming convention you want across all new practice stories).
Suggested change
| title: "Components/Practice/PracticeSection", |
…nt overview Findings now expand on click to reveal guidance, evidence, reasoning, and target info fetched on demand. Users can provide feedback (Applied, Disputed, N/A) with an optional explanation for disputes. An engagement ring shows review progress across all findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rewrite FeedbackButtons → read-only FeedbackBadge (feedback is automatic) - Remove dead isLoading prop from EngagementOverview (never reachable) - Fix spurious `items` prop on Select, fragile snippet key - Fix GuidanceMethod type derivation to use PracticeFindingDetail - Extract FindingDetail subcomponent from FindingsListItem - Extract shared mock data to __fixtures__/mock-data.ts (DRY stories) - Rename FeedbackButtons.tsx → FeedbackBadge.tsx (file matches export) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
webapp/src/components/practice/FeedbackBadge.stories.tsx (1)
5-7: Move QueryClient instantiation inside the decorator to avoid state leakage between stories.Creating
QueryClientat module scope shares cache state across all stories and test runs, which can cause flaky behavior in Chromatic snapshots or interaction tests.♻️ Proposed fix
-const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false, staleTime: Number.POSITIVE_INFINITY } }, -}); - /** * Read-only badge showing the latest feedback status for a finding. * Feedback is created automatically when users respond to finding guidance * (e.g., implementing a suggestion marks it as "Applied"). */ const meta = { component: FeedbackBadge, parameters: { layout: "padded", docs: { description: { component: "Read-only badge showing feedback status. Feedback is recorded automatically when users act on findings.", }, }, }, tags: ["autodocs"], decorators: [ - (Story) => ( - <QueryClientProvider client={queryClient}> - <div className="max-w-md"> - <Story /> - </div> - </QueryClientProvider> - ), + (Story) => { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false, staleTime: Number.POSITIVE_INFINITY } }, + }); + return ( + <QueryClientProvider client={queryClient}> + <div className="max-w-md"> + <Story /> + </div> + </QueryClientProvider> + ); + }, ], } satisfies Meta<typeof FeedbackBadge>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FeedbackBadge.stories.tsx` around lines 5 - 7, The QueryClient is created at module scope (const queryClient = new QueryClient(...)) which shares cache between stories; move the QueryClient instantiation into the Storybook decorator (or into each story) so a fresh new QueryClient is created per story run, e.g., instantiate new QueryClient(...) inside the decorator function and provide it via QueryClientProvider around the story; keep the same defaultOptions/staleTime settings but ensure you don't reuse the module-scoped queryClient to avoid cross-story state leakage.webapp/src/components/practice/SeverityBadge.tsx (1)
4-4: Use the@/*alias for imports in webapp code.Line 4 should use the configured alias instead of a relative path for consistency and tooling alignment.
Suggested fix
-import { SEVERITY_STYLES } from "./verdict-styles"; +import { SEVERITY_STYLES } from "@/components/practice/verdict-styles";As per coding guidelines,
Import with the@/* alias defined in tsconfig.json. Keep relative paths shallow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/SeverityBadge.tsx` at line 4, Replace the relative import of SEVERITY_STYLES in SeverityBadge.tsx with the project alias path: change the import source "./verdict-styles" to the `@/`* aliased path (e.g., "@/components/practice/verdict-styles") so SEVERITY_STYLES is imported via the configured tsconfig/webpack alias rather than a relative path.webapp/src/components/practice/EngagementOverview.stories.tsx (1)
61-65: Add a real zero-total edge story.
ZeroEngagementcurrently hastotalFindings: 10, so it doesn’t validate thetotalFindings === 0path. Please make this one a true zero-total case (or add a separate edge story).As per coding guidelines,
Stories should be colocated with components and cover default state, all variants, loading state, error state, and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/EngagementOverview.stories.tsx` around lines 61 - 65, The ZeroEngagement story is not exercising the zero-total branch because totalFindings is 10; update the story (or add a new edge story) so totalFindings === 0 to cover that path: modify the ZeroEngagement story's args to set totalFindings: 0 (keeping engagement: { applied: 0, disputed: 0, notApplicable: 0 }) or create a new story like ZeroTotalEngagement with those args; ensure the story name and exported Story object (ZeroEngagement or ZeroTotalEngagement) match existing patterns so the component's zero-total UI is tested.webapp/src/components/practice/EngagementOverview.tsx (1)
22-22: Replace fixed inline sizing with Tailwind utilities.Line 22 has fixed width/height in inline styles; this can be expressed with utility classes.
Suggested fix
- <div className="relative shrink-0" style={{ width: RING_SIZE, height: RING_SIZE }}> + <div className="relative shrink-0 h-20 w-20">As per coding guidelines,
Use Tailwind CSS utility classes in JSX instead of inline styles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/EngagementOverview.tsx` at line 22, The div in EngagementOverview with className "relative shrink-0" currently uses inline style {{ width: RING_SIZE, height: RING_SIZE }}; replace that inline sizing with Tailwind utility classes instead (either use a fixed w-/h- utility if RING_SIZE maps to a known size or use arbitrary value classes like w-[<value>] and h-[<value>] derived from RING_SIZE), removing the style prop; update the JSX where RING_SIZE is referenced so the element uses Tailwind classes (on the same div) and, if needed, move RING_SIZE to a constant that documents the tailwind-compatible value or compute the className string dynamically in the component.webapp/src/components/practice/finding-helpers.ts (1)
24-27: Locations parsing could throw on malformed data.The cast
(raw.locations as EvidenceLocation[]).filter(...)assumes array elements are objects with apathproperty. If an element isnull,undefined, or a primitive, accessingl.pathwill succeed (returningundefined) but the type cast is misleading.Consider a more defensive approach:
Proposed fix
locations: Array.isArray(raw.locations) - ? (raw.locations as EvidenceLocation[]).filter((l) => typeof l.path === "string") + ? (raw.locations as unknown[]) + .filter((l): l is EvidenceLocation => + l != null && typeof l === "object" && typeof (l as EvidenceLocation).path === "string" + ) : [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/finding-helpers.ts` around lines 24 - 27, The current locations parsing casts raw.locations to EvidenceLocation[] and then checks l.path, which is unsafe for null/primitive entries; update the logic around raw.locations to first verify it's an array, then filter using a defensive predicate that confirms each element is non-null, of type "object" and has a string path (e.g., a type guard that narrows to EvidenceLocation), and only then map/return the items as EvidenceLocation; reference raw.locations, EvidenceLocation, and l.path when implementing the safer filter.webapp/src/components/practice/FindingsListItem.tsx (1)
143-155: Consider using index in key for evidence locations.The key
${loc.path}:${loc.startLine}could collide if the same file/line appears multiple times (e.g., multiple findings at the same location). While unlikely in practice, using the index as a fallback would be safer:Proposed fix
- <li - key={`${loc.path}:${loc.startLine}`} + <li + key={`${loc.path}:${loc.startLine}:${index}`} className="font-mono text-xs text-muted-foreground" >Note: You'd need to add
indexto the.map()callback:evidence.locations.map((loc, index) => ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FindingsListItem.tsx` around lines 143 - 155, The current key for the list items in the evidence.locations.map callback (using `${loc.path}:${loc.startLine}`) can collide if the same path/line appears more than once; update the map callback to accept the index (evidence.locations.map((loc, index) => ...)) and include it in the key (e.g. `${loc.path}:${loc.startLine}:${index}` or fall back to index when path/startLine are missing) so each <li> rendered by the FindingsListItem component has a stable, unique key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/components/practice/EngagementOverview.tsx`:
- Around line 16-17: The computed engagementRate (from totalResponded /
totalFindings) can exceed 100 or drop below 0 if counts are inconsistent; clamp
the percentage to the 0..100 range before using it in the ring math. Update the
logic that calculates engagementRate (and subsequently offset using
CIRCUMFERENCE) to first compute the raw percentage (handling totalFindings ===
0) and then apply a clamp like Math.max(0, Math.min(100, rawPercent)) so offset
is always valid.
In `@webapp/src/components/practice/PracticeSection.tsx`:
- Around line 40-44: The retry path does not refresh the engagement query, so
update the retry handler to also invalidate/refetch the engagementQuery; locate
the engagementQuery created with useQuery (symbol: engagementQuery) and either
call queryClient.invalidateQueries(engagementQuery.queryKey) or
engagementQuery.refetch inside the existing retry function from
usePracticeFindings (symbol: retry) so that EngagementOverview will be retried
and rendered when engagement previously failed.
In `@webapp/src/components/practice/PracticeSummaryCard.tsx`:
- Line 21: Clamp the computed positiveRatio in PracticeSummaryCard to the 0..100
range before it's used for meter semantics and inline width styles: after
computing positiveRatio (the variable defined as positiveCount/totalFindings),
coerce it with Math.max(0, Math.min(100, positiveRatio)) (or equivalent) and use
that clamped value for aria-valuenow and any style width calculations; apply the
same clamping wherever positiveRatio is reused (including the other occurrences
around the meter rendering lines ~73-81).
In `@webapp/src/components/practice/PracticeSummaryGrid.stories.tsx`:
- Line 68: The story uses a non-deterministic timestamp (lastFindingAt: new
Date()) which breaks snapshot stability; replace the new Date() usage in the
PracticeSummaryGrid story data with a fixed, deterministic Date (e.g., new
Date('2023-01-01T00:00:00Z') or a numeric epoch) where the property name
lastFindingAt appears so snapshots remain stable across runs.
In `@webapp/src/components/practice/SeverityBadge.tsx`:
- Around line 12-16: The code dereferences SEVERITY_STYLES[severity] without
guarding against unknown keys; update the SeverityBadge component to safely
resolve style by using optional chaining and nullish coalescing so a fallback is
used when SEVERITY_STYLES[severity] is undefined (e.g., const style =
SEVERITY_STYLES[severity] ?? FALLBACK_STYLE). Reference the existing symbols
SEVERITY_STYLES, severity, and style (used in the Badge/className via cn and
className) and ensure the fallback provides bgColor, fgColor and label to avoid
runtime errors when rendering Badge.
---
Nitpick comments:
In `@webapp/src/components/practice/EngagementOverview.stories.tsx`:
- Around line 61-65: The ZeroEngagement story is not exercising the zero-total
branch because totalFindings is 10; update the story (or add a new edge story)
so totalFindings === 0 to cover that path: modify the ZeroEngagement story's
args to set totalFindings: 0 (keeping engagement: { applied: 0, disputed: 0,
notApplicable: 0 }) or create a new story like ZeroTotalEngagement with those
args; ensure the story name and exported Story object (ZeroEngagement or
ZeroTotalEngagement) match existing patterns so the component's zero-total UI is
tested.
In `@webapp/src/components/practice/EngagementOverview.tsx`:
- Line 22: The div in EngagementOverview with className "relative shrink-0"
currently uses inline style {{ width: RING_SIZE, height: RING_SIZE }}; replace
that inline sizing with Tailwind utility classes instead (either use a fixed
w-/h- utility if RING_SIZE maps to a known size or use arbitrary value classes
like w-[<value>] and h-[<value>] derived from RING_SIZE), removing the style
prop; update the JSX where RING_SIZE is referenced so the element uses Tailwind
classes (on the same div) and, if needed, move RING_SIZE to a constant that
documents the tailwind-compatible value or compute the className string
dynamically in the component.
In `@webapp/src/components/practice/FeedbackBadge.stories.tsx`:
- Around line 5-7: The QueryClient is created at module scope (const queryClient
= new QueryClient(...)) which shares cache between stories; move the QueryClient
instantiation into the Storybook decorator (or into each story) so a fresh new
QueryClient is created per story run, e.g., instantiate new QueryClient(...)
inside the decorator function and provide it via QueryClientProvider around the
story; keep the same defaultOptions/staleTime settings but ensure you don't
reuse the module-scoped queryClient to avoid cross-story state leakage.
In `@webapp/src/components/practice/finding-helpers.ts`:
- Around line 24-27: The current locations parsing casts raw.locations to
EvidenceLocation[] and then checks l.path, which is unsafe for null/primitive
entries; update the logic around raw.locations to first verify it's an array,
then filter using a defensive predicate that confirms each element is non-null,
of type "object" and has a string path (e.g., a type guard that narrows to
EvidenceLocation), and only then map/return the items as EvidenceLocation;
reference raw.locations, EvidenceLocation, and l.path when implementing the
safer filter.
In `@webapp/src/components/practice/FindingsListItem.tsx`:
- Around line 143-155: The current key for the list items in the
evidence.locations.map callback (using `${loc.path}:${loc.startLine}`) can
collide if the same path/line appears more than once; update the map callback to
accept the index (evidence.locations.map((loc, index) => ...)) and include it in
the key (e.g. `${loc.path}:${loc.startLine}:${index}` or fall back to index when
path/startLine are missing) so each <li> rendered by the FindingsListItem
component has a stable, unique key.
In `@webapp/src/components/practice/SeverityBadge.tsx`:
- Line 4: Replace the relative import of SEVERITY_STYLES in SeverityBadge.tsx
with the project alias path: change the import source "./verdict-styles" to the
`@/`* aliased path (e.g., "@/components/practice/verdict-styles") so
SEVERITY_STYLES is imported via the configured tsconfig/webpack alias rather
than a relative path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59db987e-f044-4ea9-b2ea-19a834fddb5b
📒 Files selected for processing (24)
webapp/src/components/practice/EngagementOverview.stories.tsxwebapp/src/components/practice/EngagementOverview.tsxwebapp/src/components/practice/FeedbackBadge.stories.tsxwebapp/src/components/practice/FeedbackBadge.tsxwebapp/src/components/practice/FindingsList.stories.tsxwebapp/src/components/practice/FindingsList.tsxwebapp/src/components/practice/FindingsListItem.stories.tsxwebapp/src/components/practice/FindingsListItem.tsxwebapp/src/components/practice/PracticeSection.stories.tsxwebapp/src/components/practice/PracticeSection.tsxwebapp/src/components/practice/PracticeSummaryCard.stories.tsxwebapp/src/components/practice/PracticeSummaryCard.tsxwebapp/src/components/practice/PracticeSummaryGrid.stories.tsxwebapp/src/components/practice/PracticeSummaryGrid.tsxwebapp/src/components/practice/SeverityBadge.stories.tsxwebapp/src/components/practice/SeverityBadge.tsxwebapp/src/components/practice/VerdictBadge.stories.tsxwebapp/src/components/practice/VerdictBadge.tsxwebapp/src/components/practice/__fixtures__/mock-data.tswebapp/src/components/practice/finding-helpers.tswebapp/src/components/practice/verdict-styles.tswebapp/src/components/profile/ProfilePage.tsxwebapp/src/hooks/use-practice-findings.tswebapp/src/routes/_authenticated/w/$workspaceSlug/index.tsx
| }: PracticeSummaryCardProps) { | ||
| const { practiceName, category, positiveCount, negativeCount, totalFindings, practiceSlug } = | ||
| summary; | ||
| const positiveRatio = totalFindings > 0 ? (positiveCount / totalFindings) * 100 : 0; |
Contributor
There was a problem hiding this comment.
Clamp ratio before using it for meter semantics and width.
positiveRatio should be bounded to 0..100 before feeding aria-valuenow and width style to prevent invalid values when data is inconsistent.
Suggested fix
- const positiveRatio = totalFindings > 0 ? (positiveCount / totalFindings) * 100 : 0;
+ const rawPositiveRatio = totalFindings > 0 ? (positiveCount / totalFindings) * 100 : 0;
+ const positiveRatio = Math.min(100, Math.max(0, rawPositiveRatio));Also applies to: 73-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/practice/PracticeSummaryCard.tsx` at line 21, Clamp the
computed positiveRatio in PracticeSummaryCard to the 0..100 range before it's
used for meter semantics and inline width styles: after computing positiveRatio
(the variable defined as positiveCount/totalFindings), coerce it with
Math.max(0, Math.min(100, positiveRatio)) (or equivalent) and use that clamped
value for aria-valuenow and any style width calculations; apply the same
clamping wherever positiveRatio is reused (including the other occurrences
around the meter rendering lines ~73-81).
Comment on lines
+12
to
+16
| const style = SEVERITY_STYLES[severity]; | ||
| return ( | ||
| <Badge className={cn(style.bgColor, style.fgColor, "border-transparent", className)}> | ||
| {style.label} | ||
| </Badge> |
Contributor
There was a problem hiding this comment.
Guard against unknown severity values before dereferencing style.
Line 12 assumes the server will always send a known key. If an unexpected severity arrives, Lines 14-15 will throw. Add a fallback style.
Suggested fix
export function SeverityBadge({ severity, className }: SeverityBadgeProps) {
- const style = SEVERITY_STYLES[severity];
+ const style = SEVERITY_STYLES[severity] ?? SEVERITY_STYLES.INFO;
return (
<Badge className={cn(style.bgColor, style.fgColor, "border-transparent", className)}>
{style.label}
</Badge>
);
}As per coding guidelines, Use optional chaining (?.) and nullish coalescing (??) operators when working with generated API responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/practice/SeverityBadge.tsx` around lines 12 - 16, The
code dereferences SEVERITY_STYLES[severity] without guarding against unknown
keys; update the SeverityBadge component to safely resolve style by using
optional chaining and nullish coalescing so a fallback is used when
SEVERITY_STYLES[severity] is undefined (e.g., const style =
SEVERITY_STYLES[severity] ?? FALLBACK_STYLE). Reference the existing symbols
SEVERITY_STYLES, severity, and style (used in the Badge/className via cn and
className) and ensure the fallback provides bgColor, fgColor and label to avoid
runtime errors when rendering Badge.
…fixtures Move engagement query and totalFindings derivation from PracticeSection into usePracticeFindings hook to maintain container/presentational split. Replace non-deterministic new Date() calls with fixed dates for reproducible Storybook snapshots. Add error state for finding detail fetch failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cases Add conditional Practices sidebar item when practicesEnabled is true, linking to the user's profile where practice findings are displayed. Fix aria-busy string→boolean, clamp engagement rate to 100%, use deterministic skeleton dates, remove explicit Storybook title. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ries Add #practices hash to sidebar Practices link so it scrolls to the practices section on the profile page. Fix ToggleGroup deselect defaulting to ALL instead of silent no-op. Fix evidence location key collision by adding index. Add practicesEnabled to NavDashboards stories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
webapp/src/components/core/sidebar/NavDashboards.tsx (2)
17-23: Export the prop interface for reusability.The inline type definition should be extracted and exported as an interface per coding guidelines. This enables other modules to reuse the prop types.
♻️ Proposed refactor
+export interface NavDashboardsProps { + username: string; + workspaceSlug: string; + achievementsEnabled?: boolean; + leaderboardEnabled?: boolean; + practicesEnabled?: boolean; +} + export function NavDashboards({ username, workspaceSlug, achievementsEnabled = true, leaderboardEnabled = true, practicesEnabled = false, -}: { - username: string; - workspaceSlug: string; - achievementsEnabled?: boolean; - leaderboardEnabled?: boolean; - practicesEnabled?: boolean; -}) { +}: NavDashboardsProps) {As per coding guidelines: "Export prop interfaces from components" for
webapp/src/components/**/*.{ts,tsx}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/core/sidebar/NavDashboards.tsx` around lines 17 - 23, The component currently uses an inline anonymous prop type; extract and export a named interface (e.g., export interface NavDashboardsProps) with the fields username, workspaceSlug, achievementsEnabled?, leaderboardEnabled?, practicesEnabled? and replace the inline type in the NavDashboards function signature to use NavDashboardsProps so other modules can import and reuse the prop types.
42-58: Consider adding explicit scroll-to-hash logic when navigating within the profile page.The
hash="practices"link correctly targets theid="practices"element onPracticeSection, and TanStack Router hasscrollRestoration: trueconfigured. However, when the user is already on the profile page and clicks the Practices link, the route doesn't change—only the URL hash changes. TanStack Router's scroll restoration may not auto-scroll in this scenario. Add an effect inProfilePagethat watches the hash and scrolls to the element usingscrollIntoView({ behavior: "smooth" }), or verify TanStack Router's hash fragment handling works as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/core/sidebar/NavDashboards.tsx` around lines 42 - 58, The ProfilePage component needs an effect to handle intra-page hash navigation when the route doesn't change: add a useEffect inside ProfilePage that listens to the current location.hash (from TanStack Router's useLocation or routerState) and when it equals "practices" (matching the Link's hash="practices"), query document.getElementById("practices") and call element.scrollIntoView({ behavior: "smooth" }) (guard null), and trigger this whenever the hash changes so clicking the Practices SidebarMenuButton while already on the profile will scroll to the PracticeSection.webapp/src/components/core/sidebar/NavDashboards.stories.tsx (1)
77-85: LGTM! Good coverage for the newpracticesEnabledprop.The
PracticesEnabledstory correctly tests the practices-first configuration. Consider adding anAllFeaturesEnabledstory to cover the case where all feature flags are true, ensuring no visual overlap or layout issues.💡 Optional: Add story for all features enabled
/** * All optional features enabled — full sidebar. */ export const AllFeaturesEnabled: Story = { args: { achievementsEnabled: true, leaderboardEnabled: true, practicesEnabled: true, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/core/sidebar/NavDashboards.stories.tsx` around lines 77 - 85, Add an additional story named AllFeaturesEnabled to exercise the full-sidebar case (similar to the existing PracticesEnabled story); create an exported const AllFeaturesEnabled: Story with args setting achievementsEnabled: true, leaderboardEnabled: true, and practicesEnabled: true so the component is rendered with all feature flags on and can be visually inspected for overlap/layout issues; place it alongside PracticesEnabled in NavDashboards.stories.tsx and ensure it follows the same Story typing and export pattern.webapp/src/components/practice/FindingsList.tsx (2)
13-14: Use@/*aliases for local practice imports.Switch these relative imports to the configured path alias for consistency with the webapp import convention.
♻️ Suggested fix
-import { FindingsListItem } from "./FindingsListItem"; -import { isVerdictFilter, type VerdictFilter } from "./verdict-styles"; +import { FindingsListItem } from "@/components/practice/FindingsListItem"; +import { isVerdictFilter, type VerdictFilter } from "@/components/practice/verdict-styles";As per coding guidelines, "Import with the
@/*alias defined intsconfig.jsoninstead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FindingsList.tsx` around lines 13 - 14, Replace the two relative imports with the project alias imports: change the import of FindingsListItem to use the alias path that maps to the practice components (referencing FindingsListItem) and change the import of isVerdictFilter and VerdictFilter to the aliased path for verdict-styles (referencing isVerdictFilter and VerdictFilter) so both use the configured `@/`* tsconfig alias instead of relative paths.
86-92: Add an explicit accessible label to the practice filter trigger.The verdict toggle has an ARIA label, but the practice select trigger does not. Adding one improves screen-reader clarity in the filter bar.
♿ Suggested fix
-<SelectTrigger size="sm"> +<SelectTrigger size="sm" aria-label="Filter by practice"> <SelectValue placeholder="All practices" /> </SelectTrigger>As per coding guidelines, "Follow shadcn patterns for accessibility in webapp components, wire up ARIA roles."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FindingsList.tsx` around lines 86 - 92, The SelectTrigger for the practice filter lacks an accessible label; update the SelectTrigger component used with Select (the one paired with SelectValue and SelectValue placeholder "All practices") to include an explicit ARIA label (e.g., aria-label="Practice filter" or aria-labelledby pointing to a visible label) so screen readers can identify the control; ensure the change is applied alongside the existing Select props (value/onValueChange, selectedPracticeSlug, onPracticeSelect) following shadcn accessibility patterns.webapp/src/components/practice/PracticeSection.stories.tsx (2)
125-164: Add Storybookplaycoverage for interactive states.These stories cover static states well, but they currently skip interaction assertions (e.g., retry click, findings filter interactions), which weakens regression detection for presentational behavior.
As per coding guidelines, "Use Storybook play functions for interactive flows instead of end-to-end tests when the surface is presentational."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/PracticeSection.stories.tsx` around lines 125 - 164, Add Storybook play functions to the stories (WithFindings, Empty, Loading, ErrorState) to cover interactive flows: for WithFindings write a play that simulates user interactions with the findings filter UI and asserts expected DOM changes using the story's args (mockFindings/mockSummaries); for ErrorState add a play that queries the retry button and fires a click, asserting the provided onRetry handler was called or the retry UI state changes; for Empty add a play that asserts the empty-state message and any CTA are present and clickable; for Loading add a play that verifies skeleton/loading placeholders are visible and not interactive. Use the story exports (WithFindings, ErrorState, Empty, Loading) and their args (isError, isEmpty, isLoading, findings) to locate where to attach the play functions.
7-10: Use@/*aliases instead of relative imports in this story file.♻️ Suggested fix
-import { mockFindings, mockSummaries } from "./__fixtures__/mock-data"; -import { EngagementOverview } from "./EngagementOverview"; -import { FindingsList } from "./FindingsList"; -import { PracticeSummaryGrid } from "./PracticeSummaryGrid"; +import { mockFindings, mockSummaries } from "@/components/practice/__fixtures__/mock-data"; +import { EngagementOverview } from "@/components/practice/EngagementOverview"; +import { FindingsList } from "@/components/practice/FindingsList"; +import { PracticeSummaryGrid } from "@/components/practice/PracticeSummaryGrid";As per coding guidelines, "Import with the
@/*alias defined intsconfig.jsoninstead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/PracticeSection.stories.tsx` around lines 7 - 10, The story file currently imports test data and components with relative paths; update the imports for mockFindings, mockSummaries, EngagementOverview, FindingsList, and PracticeSummaryGrid to use the project alias pattern (e.g. `@/`...) from tsconfig instead of relative paths so the file follows the codebase import conventions—locate the import statements at the top of PracticeSection.stories.tsx and replace each "./__fixtures__/mock-data", "./EngagementOverview", "./FindingsList", and "./PracticeSummaryGrid" import with the corresponding "@/..." aliased module paths.webapp/src/components/practice/FindingsListItem.stories.tsx (2)
2-3: Use@/*alias imports instead of relative imports.Line 2 and Line 3 use local relative imports, which breaks the webapp import convention.
Suggested diff
-import { mockFindings } from "./__fixtures__/mock-data"; -import { FindingsListItem } from "./FindingsListItem"; +import { mockFindings } from "@/components/practice/__fixtures__/mock-data"; +import { FindingsListItem } from "@/components/practice/FindingsListItem";As per coding guidelines, "Import with the
@/* alias defined in tsconfig.json instead of relative paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FindingsListItem.stories.tsx` around lines 2 - 3, Replace the relative imports in FindingsListItem.stories.tsx with the project alias imports: update the import of mockFindings and the FindingsListItem component to use the "@/..." alias (referencing the mockFindings fixture and the FindingsListItem export) so they follow the tsconfig-defined `@/`* convention; ensure paths match the repository's alias structure and adjust any export paths if necessary.
40-142: Add aplayinteraction story to test expand/collapse behavior.The existing stories cover visual states well, but interaction testing is missing. Add at least one
playfunction that clicks the row to verify expanded content loads and error states are handled properly. This aligns with the component's expandable design and testing guidelines for interactive surfaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/practice/FindingsListItem.stories.tsx` around lines 40 - 142, Add a Storybook play interaction to one of the stories (e.g., Positive or Negative) to test expand/collapse: implement a play function on the chosen Story export (e.g., Positive.play) that uses Storybook testing-library utilities (userEvent and within from `@storybook/testing-library`) to click the findings row element (identify via role, text, or data-testid rendered by FindingsListItem) and then await the expanded content to appear with findByText/findByRole; also test the collapse by clicking again and asserting the expanded content is not present, and include an additional assertion for the loading/error state by using the Loading story or setting isLoading/error props inside the same play to assert skeleton or error message is shown. Ensure the play references the component/story name (Positive or Loading) and targets the DOM selectors used by FindingsListItem for expand/collapse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/components/practice/FindingsList.tsx`:
- Around line 63-89: The select currently uses the literal "all" as a sentinel
which can collide with a real practice slug; change the sentinel to an
impossible value (e.g. "__ALL__" or similar) and update selectItems, the Select
value binding, and the onValueChange handler to map that sentinel to null for
onPracticeSelect; specifically modify the selectItems array creation, the value
prop that uses selectedPracticeSlug (in FindingsList), and the onValueChange
arrow (which currently compares to "all") so that only the sentinel maps to null
while real practice slugs are passed through to onPracticeSelect.
In `@webapp/src/components/practice/PracticeSection.stories.tsx`:
- Around line 93-95: The story is using hasMore={!isLoading && findings.length >
0} which differs from production where pagination uses findingsQuery.hasNextPage
(see use-practice-findings hook); update the story in
PracticeSection.stories.tsx to set the hasMore prop explicitly from story args
(e.g., pass hasMore: true/false per story) or mirror production by using
findingsQuery.hasNextPage in the mocked data, and ensure components that read
hasMore receive that explicit value instead of deriving it from findings.length
so Storybook matches real behavior.
---
Nitpick comments:
In `@webapp/src/components/core/sidebar/NavDashboards.stories.tsx`:
- Around line 77-85: Add an additional story named AllFeaturesEnabled to
exercise the full-sidebar case (similar to the existing PracticesEnabled story);
create an exported const AllFeaturesEnabled: Story with args setting
achievementsEnabled: true, leaderboardEnabled: true, and practicesEnabled: true
so the component is rendered with all feature flags on and can be visually
inspected for overlap/layout issues; place it alongside PracticesEnabled in
NavDashboards.stories.tsx and ensure it follows the same Story typing and export
pattern.
In `@webapp/src/components/core/sidebar/NavDashboards.tsx`:
- Around line 17-23: The component currently uses an inline anonymous prop type;
extract and export a named interface (e.g., export interface NavDashboardsProps)
with the fields username, workspaceSlug, achievementsEnabled?,
leaderboardEnabled?, practicesEnabled? and replace the inline type in the
NavDashboards function signature to use NavDashboardsProps so other modules can
import and reuse the prop types.
- Around line 42-58: The ProfilePage component needs an effect to handle
intra-page hash navigation when the route doesn't change: add a useEffect inside
ProfilePage that listens to the current location.hash (from TanStack Router's
useLocation or routerState) and when it equals "practices" (matching the Link's
hash="practices"), query document.getElementById("practices") and call
element.scrollIntoView({ behavior: "smooth" }) (guard null), and trigger this
whenever the hash changes so clicking the Practices SidebarMenuButton while
already on the profile will scroll to the PracticeSection.
In `@webapp/src/components/practice/FindingsList.tsx`:
- Around line 13-14: Replace the two relative imports with the project alias
imports: change the import of FindingsListItem to use the alias path that maps
to the practice components (referencing FindingsListItem) and change the import
of isVerdictFilter and VerdictFilter to the aliased path for verdict-styles
(referencing isVerdictFilter and VerdictFilter) so both use the configured `@/`*
tsconfig alias instead of relative paths.
- Around line 86-92: The SelectTrigger for the practice filter lacks an
accessible label; update the SelectTrigger component used with Select (the one
paired with SelectValue and SelectValue placeholder "All practices") to include
an explicit ARIA label (e.g., aria-label="Practice filter" or aria-labelledby
pointing to a visible label) so screen readers can identify the control; ensure
the change is applied alongside the existing Select props (value/onValueChange,
selectedPracticeSlug, onPracticeSelect) following shadcn accessibility patterns.
In `@webapp/src/components/practice/FindingsListItem.stories.tsx`:
- Around line 2-3: Replace the relative imports in FindingsListItem.stories.tsx
with the project alias imports: update the import of mockFindings and the
FindingsListItem component to use the "@/..." alias (referencing the
mockFindings fixture and the FindingsListItem export) so they follow the
tsconfig-defined `@/`* convention; ensure paths match the repository's alias
structure and adjust any export paths if necessary.
- Around line 40-142: Add a Storybook play interaction to one of the stories
(e.g., Positive or Negative) to test expand/collapse: implement a play function
on the chosen Story export (e.g., Positive.play) that uses Storybook
testing-library utilities (userEvent and within from `@storybook/testing-library`)
to click the findings row element (identify via role, text, or data-testid
rendered by FindingsListItem) and then await the expanded content to appear with
findByText/findByRole; also test the collapse by clicking again and asserting
the expanded content is not present, and include an additional assertion for the
loading/error state by using the Loading story or setting isLoading/error props
inside the same play to assert skeleton or error message is shown. Ensure the
play references the component/story name (Positive or Loading) and targets the
DOM selectors used by FindingsListItem for expand/collapse.
In `@webapp/src/components/practice/PracticeSection.stories.tsx`:
- Around line 125-164: Add Storybook play functions to the stories
(WithFindings, Empty, Loading, ErrorState) to cover interactive flows: for
WithFindings write a play that simulates user interactions with the findings
filter UI and asserts expected DOM changes using the story's args
(mockFindings/mockSummaries); for ErrorState add a play that queries the retry
button and fires a click, asserting the provided onRetry handler was called or
the retry UI state changes; for Empty add a play that asserts the empty-state
message and any CTA are present and clickable; for Loading add a play that
verifies skeleton/loading placeholders are visible and not interactive. Use the
story exports (WithFindings, ErrorState, Empty, Loading) and their args
(isError, isEmpty, isLoading, findings) to locate where to attach the play
functions.
- Around line 7-10: The story file currently imports test data and components
with relative paths; update the imports for mockFindings, mockSummaries,
EngagementOverview, FindingsList, and PracticeSummaryGrid to use the project
alias pattern (e.g. `@/`...) from tsconfig instead of relative paths so the file
follows the codebase import conventions—locate the import statements at the top
of PracticeSection.stories.tsx and replace each "./__fixtures__/mock-data",
"./EngagementOverview", "./FindingsList", and "./PracticeSummaryGrid" import
with the corresponding "@/..." aliased module paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fe42041-1446-415b-ac40-60ffad45f71f
📒 Files selected for processing (13)
webapp/src/components/core/sidebar/AppSidebar.tsxwebapp/src/components/core/sidebar/NavDashboards.stories.tsxwebapp/src/components/core/sidebar/NavDashboards.tsxwebapp/src/components/practice/EngagementOverview.tsxwebapp/src/components/practice/FindingsList.tsxwebapp/src/components/practice/FindingsListItem.stories.tsxwebapp/src/components/practice/FindingsListItem.tsxwebapp/src/components/practice/PracticeSection.stories.tsxwebapp/src/components/practice/PracticeSection.tsxwebapp/src/components/practice/PracticeSummaryGrid.stories.tsxwebapp/src/components/practice/PracticeSummaryGrid.tsxwebapp/src/components/practice/__fixtures__/mock-data.tswebapp/src/hooks/use-practice-findings.ts
✅ Files skipped from review due to trivial changes (1)
- webapp/src/components/practice/PracticeSummaryGrid.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- webapp/src/components/practice/EngagementOverview.tsx
- webapp/src/components/practice/fixtures/mock-data.ts
- webapp/src/components/practice/PracticeSummaryGrid.tsx
- webapp/src/components/practice/FindingsListItem.tsx
- webapp/src/components/practice/PracticeSection.tsx
- webapp/src/hooks/use-practice-findings.ts
Comment on lines
+63
to
+89
| const selectItems = [{ value: "all", label: "All practices" }, ...practiceOptions]; | ||
|
|
||
| const hasActiveFilters = selectedPracticeSlug !== null || selectedVerdict !== "ALL"; | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <div className="flex flex-col gap-3" aria-busy={true}> | ||
| <h3 className="text-lg font-semibold">Findings</h3> | ||
| <ul aria-label="Findings loading" className="flex flex-col gap-2"> | ||
| {Array.from({ length: 3 }, (_, i) => ( | ||
| <FindingsListItem key={i} finding={SKELETON_FINDING} isLoading /> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-3"> | ||
| <h3 className="text-lg font-semibold">Findings</h3> | ||
|
|
||
| {/* Filter bar */} | ||
| <div className="flex flex-wrap items-center gap-2"> | ||
| <Select | ||
| value={selectedPracticeSlug ?? "all"} | ||
| onValueChange={(value) => value && onPracticeSelect(value === "all" ? null : value)} | ||
| > |
Contributor
There was a problem hiding this comment.
Avoid "all" sentinel collisions with real practice slugs.
Using "all" as a synthetic select value can collide with a real practiceSlug === "all", making that practice impossible to target via the filter.
💡 Suggested fix
+const ALL_PRACTICES_VALUE = "__all_practices__" as const;
+
-const selectItems = [{ value: "all", label: "All practices" }, ...practiceOptions];
+const selectItems = [{ value: ALL_PRACTICES_VALUE, label: "All practices" }, ...practiceOptions];
<Select
- value={selectedPracticeSlug ?? "all"}
- onValueChange={(value) => value && onPracticeSelect(value === "all" ? null : value)}
+ value={selectedPracticeSlug ?? ALL_PRACTICES_VALUE}
+ onValueChange={(value) => onPracticeSelect(value === ALL_PRACTICES_VALUE ? null : value)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const selectItems = [{ value: "all", label: "All practices" }, ...practiceOptions]; | |
| const hasActiveFilters = selectedPracticeSlug !== null || selectedVerdict !== "ALL"; | |
| if (isLoading) { | |
| return ( | |
| <div className="flex flex-col gap-3" aria-busy={true}> | |
| <h3 className="text-lg font-semibold">Findings</h3> | |
| <ul aria-label="Findings loading" className="flex flex-col gap-2"> | |
| {Array.from({ length: 3 }, (_, i) => ( | |
| <FindingsListItem key={i} finding={SKELETON_FINDING} isLoading /> | |
| ))} | |
| </ul> | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <div className="flex flex-col gap-3"> | |
| <h3 className="text-lg font-semibold">Findings</h3> | |
| {/* Filter bar */} | |
| <div className="flex flex-wrap items-center gap-2"> | |
| <Select | |
| value={selectedPracticeSlug ?? "all"} | |
| onValueChange={(value) => value && onPracticeSelect(value === "all" ? null : value)} | |
| > | |
| const ALL_PRACTICES_VALUE = "__all_practices__" as const; | |
| const selectItems = [{ value: ALL_PRACTICES_VALUE, label: "All practices" }, ...practiceOptions]; | |
| const hasActiveFilters = selectedPracticeSlug !== null || selectedVerdict !== "ALL"; | |
| if (isLoading) { | |
| return ( | |
| <div className="flex flex-col gap-3" aria-busy={true}> | |
| <h3 className="text-lg font-semibold">Findings</h3> | |
| <ul aria-label="Findings loading" className="flex flex-col gap-2"> | |
| {Array.from({ length: 3 }, (_, i) => ( | |
| <FindingsListItem key={i} finding={SKELETON_FINDING} isLoading /> | |
| ))} | |
| </ul> | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <div className="flex flex-col gap-3"> | |
| <h3 className="text-lg font-semibold">Findings</h3> | |
| {/* Filter bar */} | |
| <div className="flex flex-wrap items-center gap-2"> | |
| <Select | |
| value={selectedPracticeSlug ?? ALL_PRACTICES_VALUE} | |
| onValueChange={(value) => onPracticeSelect(value === ALL_PRACTICES_VALUE ? null : value)} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/practice/FindingsList.tsx` around lines 63 - 89, The
select currently uses the literal "all" as a sentinel which can collide with a
real practice slug; change the sentinel to an impossible value (e.g. "__ALL__"
or similar) and update selectItems, the Select value binding, and the
onValueChange handler to map that sentinel to null for onPracticeSelect;
specifically modify the selectItems array creation, the value prop that uses
selectedPracticeSlug (in FindingsList), and the onValueChange arrow (which
currently compares to "all") so that only the sentinel maps to null while real
practice slugs are passed through to onPracticeSelect.
Comment on lines
+93
to
+95
| hasMore={!isLoading && findings.length > 0} | ||
| isLoading={isLoading} | ||
| /> |
Contributor
There was a problem hiding this comment.
hasMore in the story does not match production behavior.
This story shows pagination based on findings.length, while production uses findingsQuery.hasNextPage (webapp/src/hooks/use-practice-findings.ts, Line 138). That can render an inaccurate “Show more findings” state in Storybook.
💡 Suggested fix
interface PracticeSectionStoryProps {
summaries: typeof mockSummaries;
findings: typeof mockFindings;
engagement?: FindingFeedbackEngagement;
isLoading?: boolean;
isEmpty?: boolean;
isError?: boolean;
+ hasMore?: boolean;
}
function PracticeSectionStory({
summaries,
findings,
engagement,
isLoading = false,
isEmpty = false,
isError = false,
+ hasMore = false,
}: PracticeSectionStoryProps) {
@@
<FindingsList
findings={findings}
practiceOptions={practiceOptions}
selectedPracticeSlug={null}
selectedVerdict="ALL"
onPracticeSelect={() => {}}
onVerdictChange={() => {}}
- hasMore={!isLoading && findings.length > 0}
+ hasMore={hasMore}
isLoading={isLoading}
/>Then set hasMore explicitly per story args where needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hasMore={!isLoading && findings.length > 0} | |
| isLoading={isLoading} | |
| /> | |
| <FindingsList | |
| findings={findings} | |
| practiceOptions={practiceOptions} | |
| selectedPracticeSlug={null} | |
| selectedVerdict="ALL" | |
| onPracticeSelect={() => {}} | |
| onVerdictChange={() => {}} | |
| hasMore={hasMore} | |
| isLoading={isLoading} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/practice/PracticeSection.stories.tsx` around lines 93 -
95, The story is using hasMore={!isLoading && findings.length > 0} which differs
from production where pagination uses findingsQuery.hasNextPage (see
use-practice-findings hook); update the story in PracticeSection.stories.tsx to
set the hasMore prop explicitly from story args (e.g., pass hasMore: true/false
per story) or mirror production by using findingsQuery.hasNextPage in the mocked
data, and ensure components that read hasMore receive that explicit value
instead of deriving it from findings.length so Storybook matches real behavior.
Change getWorkspaceFeatures() defaults from true to false. The previous true default meant that if the workspace query failed (network error, 500), all gated features became visible — a correctness hazard. All consumers already guard with isLoading checks before reading flags, so the default only matters on query failure where hiding features is safer than leaking them. Also align NavDashboards prop defaults to all be false for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/hooks/use-workspace-features.ts (1)
33-36:⚠️ Potential issue | 🟡 Minor
isLoadingdoes not account forauthLoading, potentially misleading consumers.In TanStack Query v5, when the query is disabled (
enabled: false),isLoadingisfalsebecause it's defined asisPending && isFetching. IfauthLoadingistrue, the query is disabled, soquery.isLoadingreturnsfalsewhileactiveWorkspaceis stillundefinedand all flags default tofalse.Consumers checking
isLoading === falsemay incorrectly assume feature flags are reliable when auth is still loading.Proposed fix to include auth loading state
return { ...getWorkspaceFeatures(activeWorkspace), - isLoading: query.isLoading, + isLoading: authLoading || query.isLoading, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/hooks/use-workspace-features.ts` around lines 33 - 36, The returned isLoading currently exposes only query.isLoading which is false when the query is disabled during authentication; update the return value in use-workspace-features (the function that calls getWorkspaceFeatures(activeWorkspace)) to include authLoading (e.g., isLoading: query.isLoading || authLoading) so consumers see true while auth is still resolving; locate activeWorkspace, query, authLoading and the call to getWorkspaceFeatures in use-workspace-features.ts and change the isLoading expression accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@webapp/src/hooks/use-workspace-features.ts`:
- Around line 33-36: The returned isLoading currently exposes only
query.isLoading which is false when the query is disabled during authentication;
update the return value in use-workspace-features (the function that calls
getWorkspaceFeatures(activeWorkspace)) to include authLoading (e.g., isLoading:
query.isLoading || authLoading) so consumers see true while auth is still
resolving; locate activeWorkspace, query, authLoading and the call to
getWorkspaceFeatures in use-workspace-features.ts and change the isLoading
expression accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 389ed25c-21d6-44c7-8b4a-85795ebaa9f6
📒 Files selected for processing (2)
webapp/src/components/core/sidebar/NavDashboards.tsxwebapp/src/hooks/use-workspace-features.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/components/core/sidebar/NavDashboards.tsx
…s-profile-integration
After merging main, PracticeFindingTargetType changed from string to a 'PULL_REQUEST' literal type. Update mock data, skeleton placeholder, and formatTargetLabel to match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FelixTJDietrich
marked this pull request as draft
June 8, 2026 12:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Practices section to the profile page showing per-practice summary cards, a filterable/paginated findings list, and an engagement overview ring. Adds a Practices sidebar link (hash-anchored) and adjusts the home route redirect for practices-first workspaces.
Fixes #932
Feature flag safety
All practice UI is gated behind
practicesEnabled:getWorkspaceFeatures()defaults all flags tofalse(safe on query failure)usePracticeFindingshook — all 3 queries useenabled: practicesEnabled && !featuresLoadingPracticeSectionreturnsnullwhen disabled or loadingpracticesEnabledis trueNew components (
webapp/src/components/practice/)PracticeSectionusePracticeFindingsto children; feature-flag gating, loading/error/empty statesPracticeSummaryGridPracticeSummaryCardFindingsListFindingsListItemEngagementOverviewFeedbackBadgeVerdictBadge/SeverityBadgeverdict-styles.tsVerdictFiltertype + guardfinding-helpers.tsparseEvidence(),formatTargetLabel(), guidance method labelsNew hook (
webapp/src/hooks/use-practice-findings.ts)Encapsulates all practice data fetching and filter state:
queryClient.invalidateQueriesacross all 3 queriesModified files
ProfilePage.tsx— appends<PracticeSection>NavDashboards.tsx— conditional Practices sidebar item with#practiceshash anchorAppSidebar.tsx— passespracticesEnabledpropuse-workspace-features.ts— defaults all flags tofalsefor safetyArchitecture decisions
PracticeSectiontouches hooks; children are pure propsisLoadingchecks, so false default only matters on query failure where hiding > leakingFeedbackBadgedisplays auto-detected status, not user-initiated actionsStorybook coverage
All components have CSF3 stories with
satisfies Meta,tags: ["autodocs"]:Test plan
practicesEnabled→ profile shows Practices section with findingspracticesEnabled→ no practice UI anywhere (section, sidebar link, API calls)#practicessectionnpm run buildsucceeds🤖 Generated with Claude Code