feat(studio): link trace detail to test case row click (FP-185)#341
Closed
walston wants to merge 4 commits into
Closed
feat(studio): link trace detail to test case row click (FP-185)#341walston wants to merge 4 commits into
walston wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesInline Trace Detail Panel
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/packages/studio/src/routes/IntakeTraceDetailRoute/index.tsx (1)
59-59: ⚡ Quick winRedundant trace fetch in both components.
IntakeTraceDetailContentfetches the trace only to computetraceBreadcrumbLabel, butIntakeTraceDetailBodyimmediately re-fetches it. TanStack Query dedupes these calls, but the code is confusing. Consider removing the fetch fromIntakeTraceDetailContentand usingtraceIddirectly for the breadcrumb until the trace loads.♻️ Suggested simplification
const IntakeTraceDetailContent: FC<IntakeTraceDetailContentProps> = ({ traceId }) => { const workspace = useWorkspaceFromPath(); - const { data: trace } = useGetTrace(workspace, traceId, { mode: 'detailed' }); const { setBreadcrumbs } = useBreadcrumbs(); - const traceBreadcrumbLabel = trace ? getTraceDisplayName(trace) : traceId; + const traceBreadcrumbLabel = traceId; // Body component handles full trace fetch useEffect(() => { setBreadcrumbs([Also applies to: 110-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/IntakeTraceDetailRoute/index.tsx` at line 59, Remove the redundant useGetTrace call in IntakeTraceDetailContent (the one used only for computing traceBreadcrumbLabel at line 59) and instead use the traceId directly for the breadcrumb display until the trace data loads. Keep the useGetTrace call in IntakeTraceDetailBody (lines 110-116) as it is the primary consumer of the trace data. This eliminates the confusing redundant fetch while maintaining the same functionality since TanStack Query will still dedupe any queries, but the code intent becomes clearer.web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsx (1)
22-22: ⚖️ Poor tradeoffConsider relocating IntakeTraceDetailBody to components directory.
Importing a component from a route file works but inverts typical dependency flow. Since
IntakeTraceDetailBodyis now a reusable component, it could live in@studio/components/IntakeTraceDetailBodyfor clearer separation of concerns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsx` at line 22, The IntakeTraceDetailBody component is currently imported from a route file (`@studio/routes/IntakeTraceDetailRoute`), which inverts the typical dependency flow where components should be imported from the components directory. Move the IntakeTraceDetailBody component to a new file in the components directory at `@studio/components/IntakeTraceDetailBody`, then update the import statement in ExperimentSessionsDataView/index.tsx to import from the new components location. Check for any other files that import IntakeTraceDetailBody from the old routes location and update those imports as well to maintain consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsx`:
- Line 215: The Stack panel that displays trace details lacks independent
scrolling capability because it has overflow-hidden without a height constraint.
Locate the Stack component that is conditionally rendered based on the
showTraceDetail variable and add the CSS classes overflow-y-auto and
max-h-[calc(100vh-2rem)] to it. This will constrain the panel's height to the
viewport and enable vertical scrolling within the panel instead of forcing
full-page scroll when content exceeds the viewport height.
---
Nitpick comments:
In
`@web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsx`:
- Line 22: The IntakeTraceDetailBody component is currently imported from a
route file (`@studio/routes/IntakeTraceDetailRoute`), which inverts the typical
dependency flow where components should be imported from the components
directory. Move the IntakeTraceDetailBody component to a new file in the
components directory at `@studio/components/IntakeTraceDetailBody`, then update
the import statement in ExperimentSessionsDataView/index.tsx to import from the
new components location. Check for any other files that import
IntakeTraceDetailBody from the old routes location and update those imports as
well to maintain consistency.
In `@web/packages/studio/src/routes/IntakeTraceDetailRoute/index.tsx`:
- Line 59: Remove the redundant useGetTrace call in IntakeTraceDetailContent
(the one used only for computing traceBreadcrumbLabel at line 59) and instead
use the traceId directly for the breadcrumb display until the trace data loads.
Keep the useGetTrace call in IntakeTraceDetailBody (lines 110-116) as it is the
primary consumer of the trace data. This eliminates the confusing redundant
fetch while maintaining the same functionality since TanStack Query will still
dedupe any queries, but the code intent becomes clearer.
🪄 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: Enterprise
Run ID: 3b532aaf-fc98-46af-a942-1aabd4c7d974
📒 Files selected for processing (2)
web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsxweb/packages/studio/src/routes/IntakeTraceDetailRoute/index.tsx
Contributor
|
Clicking a session row in the experiment detail page now opens an inline trace detail panel to the right of the table. The panel shows Trace Summary and Experiment Context cards; the spans table is omitted in this context. - Extract IntakeTraceDetailBody from IntakeTraceDetailRoute so the trace content can be rendered without route-level concerns (breadcrumbs, PageHeader). Adds optional showSpans prop (default true) to skip the spans table when rendering in the panel. - Wire ExperimentSessionsDataView: row click sets selectedTraceId, which drives a CSS-transition inline panel (same pattern as FilterPanel). Close button lives at the top of the panel column. Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…-185) - Move IntakeTraceDetailBody and its props interface out of IntakeTraceDetailRoute into @studio/components/IntakeTraceDetailBody - Remove redundant useGetTrace call from IntakeTraceDetailContent; use traceId directly for breadcrumb/heading until trace data loads - Update ExperimentSessionsDataView import to new component location Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…Body move - Fix import/order lint error in ExperimentSessionsDataView - Update IntakeTraceDetailRoute tests: heading now shows traceId directly (no longer waits for trace fetch), so findByText for Experiment Context needs to be async too Signed-off-by: Nathan Walston <nwalston@nvidia.com>
walston
force-pushed
the
nwalston/traces-view
branch
from
1c2386d to
71e2342
Compare
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Contributor
Author
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
1 participant
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.
Closes FP-185
Screen.Recording.2026-06-15.at.14.19.38.mov
Summary
Clicking a session row in the experiment detail page now opens an inline trace detail panel to the right of the table, using the same CSS transition pattern as the filter panel. The panel shows Trace Summary and Experiment Context cards; the spans table is omitted in this narrower context.
To enable reuse, IntakeTraceDetailBody is extracted from IntakeTraceDetailRoute — separating the route-level concerns (breadcrumbs, PageHeader) from the content. A showSpans prop (default true) controls whether the spans table renders, leaving the Intake full-page view unchanged.
Test plan
Summary by CodeRabbit