Merged
Conversation
🦋 Changeset detectedLatest commit: a70adf0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements infinite-scroll scores UI with a new ScoresDataList and cell components; refactors TracesDataList to use a wrapper; replaces DataKeysAndValues.ValueLink's required LinkComponent prop with an optional as; adds NoScoresInfo, UI tweaks, and score detail panel/page layout changes. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/playground/src/pages/scorers/scorer/index.tsx (1)
117-132:⚠️ Potential issue | 🟠 MajorKeep
selectedScoreIdin sync even when the target row is not loaded yet.If back/forward changes
scoreIdto a value outside the currently loaded page set, this effect returns early and leaves the previous selection active. The side panel can then keep showing stale details even though the URL points at a different score.💡 Minimal fix
useEffect(() => { const urlScoreId = searchParams.get('scoreId') ?? undefined; if (urlScoreId === selectedScoreId) return; if (!urlScoreId) { setSelectedScoreId(undefined); return; } - const matchingScore = scores.find(score => score.id === urlScoreId); - if (!matchingScore) return; - setSelectedScoreId(urlScoreId); // eslint-disable-next-line react-hooks/exhaustive-deps }, [searchParams, scores]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 117 - 132, The effect that syncs URL scoreId with component state (useEffect referencing searchParams, scores) currently early-returns when the target score isn't in the loaded page, leaving selectedScoreId stale; change the logic in that effect to always call setSelectedScoreId(urlScoreId) whenever urlScoreId !== selectedScoreId (and to set undefined when urlScoreId is falsy) instead of checking for a matchingScore in scores, so the side panel reflects the URL even if the row isn’t loaded; keep the existing dependencies and the special-case that sets undefined when no urlScoreId is present.
🧹 Nitpick comments (2)
packages/playground/src/domains/traces/components/score-data-panel.tsx (1)
102-109: Minor: Consider usingmy-6instead ofmt-6 mb-6.This is a small Tailwind cleanup opportunity.
Suggested change
- <div className="mt-6 mb-6 flex justify-end "> + <div className="my-6 flex justify-end">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/domains/traces/components/score-data-panel.tsx` around lines 102 - 109, The div wrapping the save Button uses redundant Tailwind classes "mt-6 mb-6"; replace them with the simpler "my-6" to achieve the same top/bottom spacing. Update the className on the div in the ScoreDataPanel component (the element that contains Button with onClick={() => setDatasetDialogOpen(true)} and SaveIcon) to use "my-6 flex justify-end" instead of "mt-6 mb-6 flex justify-end".packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsx (1)
55-63: Consider adding try-catch forJSON.stringifyon arbitrary input.While score input is typically serializable,
JSON.stringifycan throw on objects with circular references. This is likely an edge case given the data source, but defensive coding could prevent a potential runtime error.Optional defensive approach
export function ScoresDataListInputCell({ input }: ScoresDataListInputCellProps) { + let displayValue = '-'; + if (input != null) { + try { + displayValue = JSON.stringify(input); + } catch { + displayValue = '[Unable to display]'; + } + } return ( <DataListCell height="compact" className="min-w-0"> - <span className="block text-neutral3 text-ui-smd font-mono truncate"> - {input != null ? JSON.stringify(input) : '-'} - </span> + <span className="block text-neutral3 text-ui-smd font-mono truncate">{displayValue}</span> </DataListCell> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsx` around lines 55 - 63, The ScoresDataListInputCell currently calls JSON.stringify(input) which can throw on circular references; update ScoresDataListInputCell to defensively stringify by wrapping JSON.stringify in a try-catch inside the component (or extract a small safeStringify helper) and fall back to a safe representation like '-' or a string such as '<unserializable>' when serialization fails, ensuring the component returns the safe fallback inside the same span instead of throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/playground/src/pages/scorers/scorer/index.tsx`:
- Around line 154-163: The page-level auth/error logic currently treats any
scores query error as fatal and also always surfaces scoresError to ScoresList;
update the checks so only a fatal scores error (detect via scores.length === 0
&& scorerError) is considered for page-level auth/error gating and for passing
an error message into ScoresList. Concretely, change the
isUnauthorized/isForbidden/hasOtherError calculations to use scorerError only
when scores.length === 0 && scorerError, and likewise only pass scorerError into
ScoresList when scores.length === 0 && scorerError (leaving incremental/refetch
errors to be handled locally by the scores query). Ensure you update all
occurrences noted (the current block around
isUnauthorized/isForbidden/hasOtherError and the ScoresList error prop usage).
- Around line 31-36: The component currently initializes and drives entity
filtering from selectedEntityOption (defaulting to 'all'), which causes an
unfiltered fetch and prevents reset when the URL loses ?entity; instead derive
the filter from the URL param and drive state from it: read the entity query
param (same source as scoreIdFromUrl), pass that value into useScoresByScorerId
so the request is driven by the URL, initialize selectedEntityOption from the
parsed URL value (or undefined) rather than defaulting to 'all', and update the
sync useEffect that watches options to also clear selectedEntityOption (call
setSelectedEntityOption(undefined)) when the URL has no entity so navigation
resets the filter; change references to
selectedEntityOption/useScoresByScorerId/setSelectedEntityOption/scoreIdFromUrl
accordingly.
- Around line 150-152: The early return "if (!scorer) return null;" prevents the
auth/error fallbacks (SessionExpired, PermissionDenied, ErrorState) from
rendering when useScorer fails; remove that guard (or replace it with a proper
loading state check) so the component continues rendering and allows
SessionExpired/PermissionDenied/ErrorState to run when scorer is undefined;
update the component that calls useScorer (referencing scorer and useScorer) to
only short-circuit for an explicit loading flag instead of blindly returning
null.
---
Outside diff comments:
In `@packages/playground/src/pages/scorers/scorer/index.tsx`:
- Around line 117-132: The effect that syncs URL scoreId with component state
(useEffect referencing searchParams, scores) currently early-returns when the
target score isn't in the loaded page, leaving selectedScoreId stale; change the
logic in that effect to always call setSelectedScoreId(urlScoreId) whenever
urlScoreId !== selectedScoreId (and to set undefined when urlScoreId is falsy)
instead of checking for a matchingScore in scores, so the side panel reflects
the URL even if the row isn’t loaded; keep the existing dependencies and the
special-case that sets undefined when no urlScoreId is present.
---
Nitpick comments:
In
`@packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsx`:
- Around line 55-63: The ScoresDataListInputCell currently calls
JSON.stringify(input) which can throw on circular references; update
ScoresDataListInputCell to defensively stringify by wrapping JSON.stringify in a
try-catch inside the component (or extract a small safeStringify helper) and
fall back to a safe representation like '-' or a string such as
'<unserializable>' when serialization fails, ensuring the component returns the
safe fallback inside the same span instead of throwing.
In `@packages/playground/src/domains/traces/components/score-data-panel.tsx`:
- Around line 102-109: The div wrapping the save Button uses redundant Tailwind
classes "mt-6 mb-6"; replace them with the simpler "my-6" to achieve the same
top/bottom spacing. Update the className on the div in the ScoreDataPanel
component (the element that contains Button with onClick={() =>
setDatasetDialogOpen(true)} and SaveIcon) to use "my-6 flex justify-end" instead
of "mt-6 mb-6 flex justify-end".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c550d34-c606-4499-952d-7282f68d319b
📒 Files selected for processing (15)
packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values-value-link.tsxpackages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values.stories.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list.tsxpackages/playground-ui/src/ds/components/DataList/TracesDataList/traces-data-list.tsxpackages/playground-ui/src/ds/components/DataList/index.tspackages/playground-ui/src/ds/components/PageHeader/page-header-description.tsxpackages/playground-ui/src/ds/components/PageHeader/page-header-root.tsxpackages/playground/src/domains/scores/components/no-scores-info.tsxpackages/playground/src/domains/scores/components/scorer-combobox.tsxpackages/playground/src/domains/scores/components/scores-list.tsxpackages/playground/src/domains/scores/hooks/use-scorers.tsxpackages/playground/src/domains/scores/index.tspackages/playground/src/domains/traces/components/score-data-panel.tsxpackages/playground/src/pages/scorers/scorer/index.tsx
greglobinski
force-pushed
the
scorer-page-ui-update
branch
from
fd0f414 to
e2f6a1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/playground/src/pages/scorers/scorer/index.tsx (3)
150-152:⚠️ Potential issue | 🟠 MajorDon't return before the auth/error fallbacks render.
useScorersetsscorertonullon failures, so this guard exits beforeSessionExpired,PermissionDenied, orErrorStatecan render. Only short-circuit on an explicit loading state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 150 - 152, The early return "if (!scorer) return null;" prevents auth/error fallbacks from rendering; instead, change the guard to only short-circuit on an explicit loading flag from useScorer (e.g., isLoading or loading) and allow the component to render when scorer is null so SessionExpired, PermissionDenied, or ErrorState can show; update the code paths that reference scorer to handle null safely (e.g., conditional render where needed) and remove or replace the "if (!scorer)" check so useScorer, scorer, SessionExpired, PermissionDenied, and ErrorState behave as intended.
154-163:⚠️ Potential issue | 🟠 MajorOnly treat scores failures as fatal when nothing is loaded yet.
Right now an initial
scoresErrorfalls through toNoScoresInfo, while a later-page/refetch error is always passed intoScoresListand can replace already loaded rows. Gate both the page-level auth/error checks anderrorMsgoffscores.length === 0 && scoresError.Also applies to: 205-223, 249-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 154 - 163, The page-level auth/error checks currently treat any scorerError/agentsError/workflowsError as fatal even when scores are already loaded; change the logic so isUnauthorized, isForbidden, and hasOtherError only consider errors when scores.length === 0 && scoresError (i.e., gate the is401UnauthorizedError/is403ForbiddenError checks behind scores.length === 0 and the specific scoresError variable), and likewise ensure the errorMsg passed into ScoresList is only set when scores.length === 0 && scoresError; update all occurrences (the blocks using is401UnauthorizedError, is403ForbiddenError, hasOtherError and where errorMsg is derived/passed to ScoresList) to use this gated condition.
31-36:⚠️ Potential issue | 🟠 MajorDrive the entity filter from the URL, not
selectedEntityOption.A deep link like
?entity=...still issues the firstuseScoresByScorerIdrequest as “all”, because the query reads local state that starts at'all'. The render-time sync on Line 81 also never clears the filter when navigation removesentity. Make the URL param the source of truth for the request, and move the sync out of render.Also applies to: 49-53, 78-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 31 - 36, The code currently seeds and drives the entity filter from local state (selectedEntityOption) which defaults to 'all', causing useScoresByScorerId to fire with the wrong entity when a URL ?entity=... is present and the render-time sync never clears when the URL param is removed; instead, derive the entity filter from the URL param (scoreIdFromUrl / the parsed entity query param) and pass that value into useScoresByScorerId, remove the render-time assignment that mutates selectedEntityOption during render, and replace it with a useEffect that updates setSelectedEntityOption only when the URL entity param changes (including clearing it when navigation removes entity) so the URL is the single source of truth for the request and for the selectedEntityOption state.
🧹 Nitpick comments (1)
packages/playground/src/pages/scorers/scorer/index.tsx (1)
42-53: Add E2E coverage for the new scorer flow.This swaps page-based pagination for infinite scroll and changes the score-details interaction model. Please cover deep links, back/forward navigation, entity filtering, and next-page loading before merge.
As per coding guidelines,
packages/playground/**/*: This package is primarily validated with E2E coverage inpackages/playground/- treat changes as product-behavior changes requiring E2E testing.Also applies to: 134-148, 230-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 42 - 53, The change replaces page-based pagination with infinite scroll and alters score-details interactions, so add E2E tests under packages/playground to validate the new scorer flow: write tests that use the scorerId route to assert deep links open the correct score-details, verify back/forward navigation restores the open/closed detail state, assert entity filtering via selectedEntityOption (both value/type combinations) updates the list, and exercise infinite scroll by triggering setEndOfListElement to load subsequent pages (isFetchingNextPage/hasNextPage behavior); target the flows touched by useScoresByScorerId, scorerId, selectedEntityOption, and setEndOfListElement and include coverage for the other modified ranges referenced (lines ~134-148, ~230-259).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/playground/src/domains/scores/components/scores-list.tsx`:
- Around line 38-47: The component currently falls back to internalSelectedId
when controlledSelectedId is undefined, causing stale selection when the parent
clears the prop; add a sync effect to update internalSelectedId whenever
controlledSelectedId changes so the component becomes fully controlled when the
prop is provided. Specifically, in the ScoresList component add a useEffect that
calls setInternalSelectedId(controlledSelectedId) with controlledSelectedId in
its dependency array, and keep existing state initialization
(useState<string|undefined>(controlledSelectedId)); this ensures selectedScoreId
(controlledSelectedId ?? internalSelectedId) reflects parent changes and
handleScoreClick (which uses selectedScoreId and setInternalSelectedId) behaves
correctly.
---
Duplicate comments:
In `@packages/playground/src/pages/scorers/scorer/index.tsx`:
- Around line 150-152: The early return "if (!scorer) return null;" prevents
auth/error fallbacks from rendering; instead, change the guard to only
short-circuit on an explicit loading flag from useScorer (e.g., isLoading or
loading) and allow the component to render when scorer is null so
SessionExpired, PermissionDenied, or ErrorState can show; update the code paths
that reference scorer to handle null safely (e.g., conditional render where
needed) and remove or replace the "if (!scorer)" check so useScorer, scorer,
SessionExpired, PermissionDenied, and ErrorState behave as intended.
- Around line 154-163: The page-level auth/error checks currently treat any
scorerError/agentsError/workflowsError as fatal even when scores are already
loaded; change the logic so isUnauthorized, isForbidden, and hasOtherError only
consider errors when scores.length === 0 && scoresError (i.e., gate the
is401UnauthorizedError/is403ForbiddenError checks behind scores.length === 0 and
the specific scoresError variable), and likewise ensure the errorMsg passed into
ScoresList is only set when scores.length === 0 && scoresError; update all
occurrences (the blocks using is401UnauthorizedError, is403ForbiddenError,
hasOtherError and where errorMsg is derived/passed to ScoresList) to use this
gated condition.
- Around line 31-36: The code currently seeds and drives the entity filter from
local state (selectedEntityOption) which defaults to 'all', causing
useScoresByScorerId to fire with the wrong entity when a URL ?entity=... is
present and the render-time sync never clears when the URL param is removed;
instead, derive the entity filter from the URL param (scoreIdFromUrl / the
parsed entity query param) and pass that value into useScoresByScorerId, remove
the render-time assignment that mutates selectedEntityOption during render, and
replace it with a useEffect that updates setSelectedEntityOption only when the
URL entity param changes (including clearing it when navigation removes entity)
so the URL is the single source of truth for the request and for the
selectedEntityOption state.
---
Nitpick comments:
In `@packages/playground/src/pages/scorers/scorer/index.tsx`:
- Around line 42-53: The change replaces page-based pagination with infinite
scroll and alters score-details interactions, so add E2E tests under
packages/playground to validate the new scorer flow: write tests that use the
scorerId route to assert deep links open the correct score-details, verify
back/forward navigation restores the open/closed detail state, assert entity
filtering via selectedEntityOption (both value/type combinations) updates the
list, and exercise infinite scroll by triggering setEndOfListElement to load
subsequent pages (isFetchingNextPage/hasNextPage behavior); target the flows
touched by useScoresByScorerId, scorerId, selectedEntityOption, and
setEndOfListElement and include coverage for the other modified ranges
referenced (lines ~134-148, ~230-259).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f290340-dbe6-4e83-9817-2bc9610b2726
📒 Files selected for processing (19)
examples/agent/src/mastra/agents/model-v2-agent.tsexamples/agent/src/mastra/index.tsexamples/agent/src/mastra/public/mastra-observability.duckdb.walpackages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values-value-link.tsxpackages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values.stories.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list.tsxpackages/playground-ui/src/ds/components/DataList/TracesDataList/traces-data-list.tsxpackages/playground-ui/src/ds/components/DataList/index.tspackages/playground-ui/src/ds/components/PageHeader/page-header-description.tsxpackages/playground-ui/src/ds/components/PageHeader/page-header-root.tsxpackages/playground/src/domains/scores/components/no-scores-info.tsxpackages/playground/src/domains/scores/components/scorer-combobox.tsxpackages/playground/src/domains/scores/components/scores-list.tsxpackages/playground/src/domains/scores/hooks/use-scorers.tsxpackages/playground/src/domains/scores/index.tspackages/playground/src/domains/traces/components/score-data-panel.tsxpackages/playground/src/pages/scorers/scorer/index.tsxpackages/playground/vite.config.ts
✅ Files skipped from review due to trivial changes (7)
- packages/playground-ui/src/ds/components/PageHeader/page-header-root.tsx
- packages/playground-ui/src/ds/components/PageHeader/page-header-description.tsx
- packages/playground-ui/src/ds/components/DataList/index.ts
- packages/playground/src/domains/scores/index.ts
- packages/playground/src/domains/scores/components/scorer-combobox.tsx
- packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list.tsx
- packages/playground/src/domains/traces/components/score-data-panel.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values-value-link.tsx
- packages/playground/src/domains/scores/components/no-scores-info.tsx
- packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values.stories.tsx
- packages/playground-ui/src/ds/components/DataList/TracesDataList/traces-data-list.tsx
- packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsx
greglobinski
force-pushed
the
scorer-page-ui-update
branch
from
e2f6a1e to
c66f9cb
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/playground/src/domains/scores/components/scores-list.tsx (1)
38-39:⚠️ Potential issue | 🟠 MajorKeep
selectedScoreIdfully controlled once the prop is supplied.When the parent clears
controlledSelectedId(e.g., via browser back/forward), the fallbackcontrolledSelectedId ?? internalSelectedIdcauses the component to revert to the last clicked score instead of clearing the selection. This leaves the side panel open even after the URL no longer specifies ascoreId.Add a sync effect to update
internalSelectedIdwhenevercontrolledSelectedIdchanges:Proposed fix
-import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; ... const [internalSelectedId, setInternalSelectedId] = useState<string | undefined>(controlledSelectedId); + + useEffect(() => { + setInternalSelectedId(controlledSelectedId); + }, [controlledSelectedId]); + const selectedScoreId = controlledSelectedId ?? internalSelectedId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/domains/scores/components/scores-list.tsx` around lines 38 - 39, The component falls back to internalSelectedId when controlledSelectedId becomes undefined, causing stale selection; add a sync effect that runs on changes to controlledSelectedId and calls setInternalSelectedId(controlledSelectedId) so internalSelectedId mirrors the prop (including being cleared) and selectedScoreId = controlledSelectedId ?? internalSelectedId behaves correctly; reference internalSelectedId, setInternalSelectedId, controlledSelectedId, and selectedScoreId.packages/playground/src/pages/scorers/scorer/index.tsx (3)
31-36:⚠️ Potential issue | 🟠 MajorDrive the entity filter from the URL, not from
selectedEntityOptionstate default.The state initializes to
'all', so a deep link like?entity=agent-123issues an unfiltereduseScoresByScorerIdrequest first, then only applies the filter afterentityOptionsresolves and the sync block (lines 78-83) runs. Additionally, when browser navigation removes?entity, the filter doesn't reset because the sync only handles matched options.Consider initializing from the URL and handling the missing case:
Suggested approach
const scoreIdFromUrl = searchParams.get('scoreId') ?? undefined; +const entityFromUrl = searchParams.get('entity') ?? 'all'; const [selectedScoreId, setSelectedScoreId] = useState<string | undefined>(scoreIdFromUrl); -const [selectedEntityOption, setSelectedEntityOption] = useState<EntityOptions | undefined>({ - value: 'all', - label: 'All Entities', - type: 'ALL' as const, -}); +const [selectedEntityOption, setSelectedEntityOption] = useState<EntityOptions | undefined>(undefined); // Later, drive the query from URL directly: // entityId: entityFromUrl === 'all' ? undefined : entityFromUrl,And update the sync effect to also handle clearing when entity is removed from URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 31 - 36, The entity filter is currently defaulted to 'all' via selectedEntityOption which causes an unfiltered useScoresByScorerId call and prevents clearing when ?entity is removed; instead read the entity query param (same source as scoreIdFromUrl) to initialize selectedEntityOption (use undefined when no param) and update the effect that syncs entityOptions -> selectedEntityOption (the block referencing entityOptions and setSelectedEntityOption) to also clear selectedEntityOption when the URL param is absent or doesn't match any option; ensure you still map the URL value to the matching EntityOptions entry when present so useScoresByScorerId is driven by the URL-first state.
150-152:⚠️ Potential issue | 🟠 MajorEarly return prevents error fallbacks from rendering when scorer fails to load.
When
useScorerreturns an error,scorerisundefined, causing this guard to returnnullbefore the auth/error UI (SessionExpired,PermissionDenied,ErrorState) can render. Users see a blank page instead of helpful error messaging.Consider gating on the loading state instead:
Proposed fix
- if (!scorer) { + if (!scorer && isScorerLoading) { return null; }This allows the error fallbacks to render when loading completes without a valid scorer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 150 - 152, The early return "if (!scorer) return null" prevents error fallbacks from rendering when useScorer returns an error; update the guard to check the scorer loading state instead (e.g., use the hook's loading or isLoading flag returned by useScorer) so that the component only returns null while loading, allowing SessionExpired, PermissionDenied, and ErrorState to render when loading completes without a valid scorer; locate the useScorer invocation and replace the scorer-presence check with a loading-based check (or invert logic to render error/UI when !loading && !scorer) and keep the existing SessionExpired/PermissionDenied/ErrorState render branches intact.
154-163:⚠️ Potential issue | 🟠 MajorInclude scores query errors in auth/error classification when no scores have loaded.
Currently, if the scores query returns 401/403/500 before any data loads, users see
NoScoresInfoinstead ofSessionExpired/PermissionDenied/ErrorState. The past review suggested gating on a "fatal" scores error whenscores.length === 0.Suggested approach
+ const fatalScoresError = scores.length === 0 ? scoresError : undefined; + const isUnauthorized = is401UnauthorizedError(scorerError) || is401UnauthorizedError(agentsError) || - is401UnauthorizedError(workflowsError); + is401UnauthorizedError(workflowsError) || + is401UnauthorizedError(fatalScoresError); - const isForbidden = scorerError && is403ForbiddenError(scorerError); + const isForbidden = + (scorerError && is403ForbiddenError(scorerError)) || + (fatalScoresError && is403ForbiddenError(fatalScoresError)); const hasOtherError = !isUnauthorized && !isForbidden && - (scorerError || agentsError || workflowsError); + (scorerError || agentsError || workflowsError || fatalScoresError); - const hasNoScores = !isLoadingScores && scores.length === 0; + const hasNoScores = !isLoadingScores && !fatalScoresError && scores.length === 0;Also update
errorMessagederivation andScoresList.errorMsgto usefatalScoresError?.message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playground/src/pages/scorers/scorer/index.tsx` around lines 154 - 163, The auth/error classification should consider scores query errors when no scores have loaded: detect a fatalScoresError when scores.length === 0 and the scores query errored, then include that fatalScoresError in the isUnauthorized/isForbidden/hasOtherError logic (in addition to scorerError/agentsError/workflowsError) so 401/403/other errors show SessionExpired/PermissionDenied/ErrorState instead of NoScoresInfo; also update the errorMessage derivation and pass fatalScoresError?.message into ScoresList.errorMsg so the scores-specific error text is displayed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/playground/src/domains/scores/components/scores-list.tsx`:
- Around line 38-39: The component falls back to internalSelectedId when
controlledSelectedId becomes undefined, causing stale selection; add a sync
effect that runs on changes to controlledSelectedId and calls
setInternalSelectedId(controlledSelectedId) so internalSelectedId mirrors the
prop (including being cleared) and selectedScoreId = controlledSelectedId ??
internalSelectedId behaves correctly; reference internalSelectedId,
setInternalSelectedId, controlledSelectedId, and selectedScoreId.
In `@packages/playground/src/pages/scorers/scorer/index.tsx`:
- Around line 31-36: The entity filter is currently defaulted to 'all' via
selectedEntityOption which causes an unfiltered useScoresByScorerId call and
prevents clearing when ?entity is removed; instead read the entity query param
(same source as scoreIdFromUrl) to initialize selectedEntityOption (use
undefined when no param) and update the effect that syncs entityOptions ->
selectedEntityOption (the block referencing entityOptions and
setSelectedEntityOption) to also clear selectedEntityOption when the URL param
is absent or doesn't match any option; ensure you still map the URL value to the
matching EntityOptions entry when present so useScoresByScorerId is driven by
the URL-first state.
- Around line 150-152: The early return "if (!scorer) return null" prevents
error fallbacks from rendering when useScorer returns an error; update the guard
to check the scorer loading state instead (e.g., use the hook's loading or
isLoading flag returned by useScorer) so that the component only returns null
while loading, allowing SessionExpired, PermissionDenied, and ErrorState to
render when loading completes without a valid scorer; locate the useScorer
invocation and replace the scorer-presence check with a loading-based check (or
invert logic to render error/UI when !loading && !scorer) and keep the existing
SessionExpired/PermissionDenied/ErrorState render branches intact.
- Around line 154-163: The auth/error classification should consider scores
query errors when no scores have loaded: detect a fatalScoresError when
scores.length === 0 and the scores query errored, then include that
fatalScoresError in the isUnauthorized/isForbidden/hasOtherError logic (in
addition to scorerError/agentsError/workflowsError) so 401/403/other errors show
SessionExpired/PermissionDenied/ErrorState instead of NoScoresInfo; also update
the errorMessage derivation and pass fatalScoresError?.message into
ScoresList.errorMsg so the scores-specific error text is displayed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08488955-0750-488c-b76d-fc36d6dd610e
📒 Files selected for processing (15)
packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values-value-link.tsxpackages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values.stories.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsxpackages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list.tsxpackages/playground-ui/src/ds/components/DataList/TracesDataList/traces-data-list.tsxpackages/playground-ui/src/ds/components/DataList/index.tspackages/playground-ui/src/ds/components/PageHeader/page-header-description.tsxpackages/playground-ui/src/ds/components/PageHeader/page-header-root.tsxpackages/playground/src/domains/scores/components/no-scores-info.tsxpackages/playground/src/domains/scores/components/scorer-combobox.tsxpackages/playground/src/domains/scores/components/scores-list.tsxpackages/playground/src/domains/scores/hooks/use-scorers.tsxpackages/playground/src/domains/scores/index.tspackages/playground/src/domains/traces/components/score-data-panel.tsxpackages/playground/src/pages/scorers/scorer/index.tsx
✅ Files skipped from review due to trivial changes (6)
- packages/playground/src/domains/scores/index.ts
- packages/playground-ui/src/ds/components/PageHeader/page-header-root.tsx
- packages/playground-ui/src/ds/components/DataList/index.ts
- packages/playground/src/domains/scores/components/scorer-combobox.tsx
- packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list.tsx
- packages/playground-ui/src/ds/components/PageHeader/page-header-description.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values.stories.tsx
- packages/playground-ui/src/ds/components/DataKeysAndValues/data-keys-and-values-value-link.tsx
- packages/playground/src/domains/scores/components/no-scores-info.tsx
- packages/playground-ui/src/ds/components/DataList/TracesDataList/traces-data-list.tsx
- packages/playground-ui/src/ds/components/DataList/ScoresDataList/scores-data-list-cells.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/thin-numbers-pump.md:
- Line 5: Rewrite the changeset to be developer-facing and mark it as a breaking
(major) change: state that a new ScoresDataList component was added to render
score evaluation results and that DataKeysAndValues.ValueLink now accepts an as
prop for custom link components instead of the previous LinkComponent prop;
explicitly include migration instructions telling users to replace usages like
DataKeysAndValues.ValueLink LinkComponent={MyLink} with
DataKeysAndValues.ValueLink as={MyLink}; remove internal implementation terms
(e.g., "compound component", "cells", "DataList pattern") and use short, direct
sentences describing what changed and how to migrate.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1108702c-bf01-412f-8d11-bf8192b36002
📒 Files selected for processing (1)
.changeset/thin-numbers-pump.md
greglobinski
force-pushed
the
scorer-page-ui-update
branch
from
1b7228c to
87730fe
Compare
mfrachet
approved these changes
Apr 17, 2026
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
2 participants
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.
Description
Apply new UI layout for a Scorer page, the score details opens in a side panel similarly to the trace/log side panel on other pages.
before
after
CleanShot.2026-04-14.at.14.46.05.mp4
Related Issue(s)
Type of Change
Checklist
ELI5
Click a score to open its details in a panel on the right (like other pages) instead of a modal, and the score list now loads more results automatically as you scroll instead of using page buttons.
Changes Overview
Moves the scorer page from a modal/dialog UX to a right-side detail panel and replaces page-based pagination with infinite-scroll. Adds a reusable ScoresDataList (with compact, truncating cells), updates data fetching/hooks for infinite fetching and sentinel-based loading, and updates the score detail panel to match traces/logs (including trace/span links and a "Save as Dataset Item" dialog). Also includes small UI/prop changes and an empty-state component.
Key Additions & Modifications
Scores list & cells
Infinite scrolling & hook changes
Score details panel
Page & selection integration
Empty state and misc UI tweaks
API / Public Surface Changes
Files / Exports Affected (high level)
Migration / Review Notes