Redesign session detail view#1327
Merged
Merged
Conversation
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Review limit reached
More reviews will be available in 46 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughRefactors the session details viewer into a capture-centered tabbed interface, removes legacy playback components, consolidates Nova UI Kit styling/props (checkbox, tabs, column settings), introduces timeline and process controls, and updates i18n keys and string mappings. ChangesSession Details Page Redesign & UI Kit Consolidation
Sequence Diagram(s)sequenceDiagram
participant Page as SessionDetailsPage
participant Content
participant CaptureViewer as Capture
participant Timeline as TimelineSlider
participant Settings as ViewSettings
Page->>Content: load session, set activeCapture
Content->>CaptureViewer: render active capture + controls
CaptureViewer->>Timeline: read activeCapture.date
Settings->>Content: toggle settings (showDetections/snap)
Content->>CaptureViewer: update overlay/rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/pages/session-details/activity-plot/activity-plot.tsx (1)
35-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the empty-
timelinecase before computing axis bounds.If
timelineis empty, Line 37 producesNaNand Line 40 produces-Infinity, which then flows intoyaxis.rangeand breaks the plot. A small early return or fallback range here would make the chart safe during empty-data/loading states.🤖 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 `@ui/src/pages/session-details/activity-plot/activity-plot.tsx` around lines 35 - 46, Guard against an empty timeline before computing avgCaptures, maxDeviation, yAxisMin, and yAxisMax: check if timeline.length === 0 (or !timeline || timeline.length === 0) and return a safe fallback (e.g., set yAxisMin = 0 and yAxisMax = 1 or early-return rendering placeholder) instead of calculating using reduce/map; update the logic around avgCaptures, maxDeviation, yAxisMin, and yAxisMax in activity-plot.tsx so the plot receives a valid numeric range when timeline is empty.
🧹 Nitpick comments (2)
ui/src/pages/session-details/session-details.tsx (2)
88-88: 💤 Low valueSimplify boolean conversion.
The ternary
session.numDetections ? true : falseis unnecessarily verbose.♻️ Proposed simplification
- snapToDetections: session.numDetections ? true : false, + snapToDetections: Boolean(session.numDetections),🤖 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 `@ui/src/pages/session-details/session-details.tsx` at line 88, Replace the verbose ternary used to set snapToDetections by using a direct boolean conversion of session.numDetections (e.g., use !!session.numDetections or Boolean(session.numDetections)) so that snapToDetections: session.numDetections ? true : false becomes a concise boolean expression referencing session.numDetections.
122-122: 💤 Low valueRemove unnecessary optional chaining.
The optional chaining
session?.numImagesis unnecessary here—sessionis a guaranteed prop passed toContentand cannot be undefined.♻️ Proposed cleanup
- subTitle={translate(STRING.RESULTS_CAPTURES, { - total: session?.numImages ?? 0, - })} + subTitle={translate(STRING.RESULTS_CAPTURES, { + total: session.numImages ?? 0, + })}🤖 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 `@ui/src/pages/session-details/session-details.tsx` at line 122, The code uses unnecessary optional chaining on session when assigning total: replace session?.numImages with session.numImages in the total: assignment (in the Content component / session-details.tsx) because session is a required prop; update the expression to total: session.numImages to remove the redundant ?. and ensure types still align with the prop definition.
🤖 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 `@ui/src/nova-ui-kit/components/info-block/info-block.tsx`:
- Line 65: Replace the hard wrap class on the value label span so tokens aren't
split mid-word: in the InfoBlock component update the span rendering
{valueLabel} (the <span className="body-base break-all"> element) to use a
softer wrap, e.g. replace "break-all" with "break-words" or add/replace with CSS
that applies "overflow-wrap: anywhere" (or "overflow-wrap: break-word") so long
paths and filenames wrap without being mangled.
In `@ui/src/nova-ui-kit/global.scss`:
- Around line 91-92: The SCSS include uses argumentless parentheses and should
be changed to an argumentless call: update the `@include` of the mixin
bubble-label in global.scss from "`@include` bubble-label();" to use no
parentheses ("`@include` bubble-label;"), ensuring you don't add any arguments and
keeping the include location (the .bubble-label rule) unchanged so it satisfies
the scss/at-mixin-argumentless-call-parentheses rule.
In `@ui/src/pages/session-details/capture-navigation.tsx`:
- Around line 89-106: The prev/next buttons currently base disabled on
activeCapture?.prevCaptureId / nextCaptureId but their handlers (goToPrev /
goToNext) use findClosestCaptureId when snapToDetections is true, causing
enabled buttons to become no-ops; update the disabled logic to also account for
snapToDetections by calling the same snap lookup (findClosestCaptureId) and
disabling the button when that lookup returns no valid capture id (i.e., combine
checks for activeCapture?.prevCaptureId / nextCaptureId OR a non-null result
from findClosestCaptureId depending on the value of snapToDetections) so the
button state matches the actual navigation target.
In `@ui/src/pages/session-details/process/process-now.tsx`:
- Around line 17-27: The button can be clicked while the createJob request is in
flight causing duplicate jobs; include the createJob in-flight state
(useCreateJob's isLoading) in the disabled condition and guard the onClick to
no-op when isLoading. Concretely, update the disabled calculation to include
isLoading (e.g., const disabled = capture.hasJobInProgress || !pipelineId ||
!canProcess || isLoading) and also add a check inside the Button onClick handler
to return early if isLoading before calling createJob() to prevent racey
double-clicks.
In `@ui/src/pages/session-details/process/process.tsx`:
- Around line 58-73: The "Latest job status" InputContent is being rendered when
capture.numJobs exists even if capture.currentJob is null, producing an empty
value; update the conditional that wraps the FormSection to only render the
Latest job status row when capture.currentJob is truthy (i.e., change the guard
involving capture.currentJob ?? capture.numJobs to a check like
capture.currentJob) and leave the Jobs InputContent unchanged; locate the JSX
around the FormSection/InputContent elements referencing capture.currentJob and
capture.numJobs to make this change.
- Around line 23-26: The pipelineId state is only initialized once from
project?.settings.defaultProcessingPipeline?.id, so when useProjectDetails
resolves the picker stays empty; add a useEffect that watches
project?.settings.defaultProcessingPipeline?.id (and project) and calls
setPipelineId(...) to sync the state after project loads (optionally only when
pipelineId is null/undefined to avoid clobbering a user selection). Update
references: useProjectDetails, project, pipelineId, setPipelineId.
In `@ui/src/pages/session-details/session-details.tsx`:
- Around line 113-115: The component currently returns null when
session.firstCapture is missing which leaves users with a blank page; replace
that silent return with a user-facing empty state (e.g., render a reusable
EmptyState/Placeholder component or a short message and optional action like "No
captures yet" and a button to go back) inside the same conditional instead of
null, or call a provided parent handler prop to let the parent render an
appropriate UI; update the logic surrounding session.firstCapture in
session-details.tsx (the if (!session.firstCapture) block) to render that empty
UI so users get clear feedback.
- Line 74: The meta tag for Open Graph image should only be emitted when an
image URL exists; in the SessionDetails component update the JSX around the
existing <meta name="og:image" content={session.exampleCaptures[0]?.src} /> to
conditionally render the entire tag (e.g., {session.exampleCaptures?.[0]?.src &&
<meta name="og:image" content={session.exampleCaptures[0].src} />}) so you never
produce a meta tag without a content attribute; reference the
session.exampleCaptures array and the component that currently renders the meta
tag in session-details.tsx.
In `@ui/src/pages/session-details/star-button.tsx`:
- Around line 23-41: The tooltip won't show when Button is actually disabled
because BasicTooltip uses Radix Trigger asChild; change the markup so the
tooltip attaches to a non-disabled wrapper (e.g., wrap the <Button> in a
span/div and keep BasicTooltip asChild on that wrapper) or alternatively remove
the native disabled prop and use aria-disabled controlled by canStar (replace
disabled={!canStar} with aria-disabled={!canStar} and prevent clicks inside
onClick by checking canStar in starCapture). Update the usage around
BasicTooltip, Button, canStar, isLoading, isStarred, and starCapture so
hover/focus events reach the tooltip while preserving the disabled UX and
preventing actions when canStar is false.
In `@ui/src/pages/session-details/timeline-slider/styles.module.scss`:
- Around line 8-10: Replace argumentless mixin includes that use parentheses and
ensure a blank line after them: change `@include body-overline();` to `@include
body-overline;` and add a blank line between that include and the subsequent
declarations (e.g., between `@include body-overline;` and `font-weight: 600;
color: var(--color-primary);`). Do the same for the other occurrence(s) of
`@include` in this module (the second block referenced around lines 20-21).
In `@ui/src/pages/session-details/timeline-slider/timeline-slider.tsx`:
- Around line 82-97: The slider is currently only exposing a raw 0–100 value to
assistive tech; update the Radix slider usage in timeline-slider.tsx so the
control has an accessible name and exposes a human-readable time string: add an
aria-label (e.g., "Timeline position") to _Slider.Root (or _Slider.Thumb) and
set aria-valuetext on _Slider.Thumb to the formatted timestamp (use the existing
valueLabel if present or call the same formatter used to render valueLabel for
the current value). Ensure you use the component symbols _Slider.Root and
_Slider.Thumb and the value/valueLabel props so screen readers receive the
formatted timestamp for the current thumb value.
In `@ui/src/pages/session-details/view-settings.tsx`:
- Around line 47-84: The three Checkbox label props in view-settings.tsx (the
Checkbox components for id="show-detections", id="default-filters", and
id="snap-to-detections") are using raw strings; wrap each label value with the
translation helper translate("...") so the labels pass through the i18n layer
(e.g., label={translate("Show detections")} etc.), leaving all other props
(checked, onCheckedChange, disabled) and the DefaultFiltersTooltip usage
unchanged.
---
Outside diff comments:
In `@ui/src/pages/session-details/activity-plot/activity-plot.tsx`:
- Around line 35-46: Guard against an empty timeline before computing
avgCaptures, maxDeviation, yAxisMin, and yAxisMax: check if timeline.length ===
0 (or !timeline || timeline.length === 0) and return a safe fallback (e.g., set
yAxisMin = 0 and yAxisMax = 1 or early-return rendering placeholder) instead of
calculating using reduce/map; update the logic around avgCaptures, maxDeviation,
yAxisMin, and yAxisMax in activity-plot.tsx so the plot receives a valid numeric
range when timeline is empty.
---
Nitpick comments:
In `@ui/src/pages/session-details/session-details.tsx`:
- Line 88: Replace the verbose ternary used to set snapToDetections by using a
direct boolean conversion of session.numDetections (e.g., use
!!session.numDetections or Boolean(session.numDetections)) so that
snapToDetections: session.numDetections ? true : false becomes a concise boolean
expression referencing session.numDetections.
- Line 122: The code uses unnecessary optional chaining on session when
assigning total: replace session?.numImages with session.numImages in the total:
assignment (in the Content component / session-details.tsx) because session is a
required prop; update the expression to total: session.numImages to remove the
redundant ?. and ensure types still align with the prop definition.
🪄 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: 52faa40e-5e8e-4279-87f1-9e1aaa61af3c
⛔ Files ignored due to path filters (1)
ui/src/nova-ui-kit/components/slider/dial.svgis excluded by!**/*.svg
📒 Files selected for processing (47)
ui/src/components/filtering/default-filter-control.tsxui/src/nova-ui-kit/components/checkbox/checkbox.module.scssui/src/nova-ui-kit/components/checkbox/checkbox.tsxui/src/nova-ui-kit/components/info-block/info-block.tsxui/src/nova-ui-kit/components/slider/timestamp-slider.tsxui/src/nova-ui-kit/components/status/status-marker/status-marker.module.scssui/src/nova-ui-kit/components/table/column-settings/column-settings.module.scssui/src/nova-ui-kit/components/table/column-settings/column-settings.tsxui/src/nova-ui-kit/components/tabs/tabs.tsxui/src/nova-ui-kit/global.scssui/src/nova-ui-kit/index.tsui/src/pages/deployments/deployment-columns.tsxui/src/pages/project/capture-sets/capture-set-columns.tsxui/src/pages/session-details/activity-plot/activity-plot.tsxui/src/pages/session-details/activity-plot/lazy-activity-plot.tsxui/src/pages/session-details/activity-plot/useDynamicPlotWidth.tsui/src/pages/session-details/capture-info.tsxui/src/pages/session-details/capture-navigation.tsxui/src/pages/session-details/capture/capture.module.scssui/src/pages/session-details/capture/capture.tsxui/src/pages/session-details/hooks/useActiveCapture.tsui/src/pages/session-details/hooks/useActiveOccurrences.tsui/src/pages/session-details/playback/activity-plot/lazy-activity-plot.module.scssui/src/pages/session-details/playback/activity-plot/types.tsui/src/pages/session-details/playback/capture-details/capture-details.module.scssui/src/pages/session-details/playback/capture-details/capture-details.tsxui/src/pages/session-details/playback/capture-details/capture-job/capture-job-dialog.tsxui/src/pages/session-details/playback/capture-details/capture-job/capture-job.tsxui/src/pages/session-details/playback/capture-details/capture-job/process-now.tsxui/src/pages/session-details/playback/capture-navigation/capture-navigation.module.scssui/src/pages/session-details/playback/frame/types.tsui/src/pages/session-details/playback/playback.module.scssui/src/pages/session-details/playback/playback.tsxui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsxui/src/pages/session-details/process/process-now.tsxui/src/pages/session-details/process/process.tsxui/src/pages/session-details/session-details.module.scssui/src/pages/session-details/session-details.tsxui/src/pages/session-details/session-info.tsxui/src/pages/session-details/session-info/session-info.module.scssui/src/pages/session-details/session-plots.tsxui/src/pages/session-details/star-button.tsxui/src/pages/session-details/timeline-slider/styles.module.scssui/src/pages/session-details/timeline-slider/timeline-slider.tsxui/src/pages/session-details/utils.tsxui/src/pages/session-details/view-settings.tsxui/src/utils/language.ts
💤 Files with no reviewable changes (17)
- ui/src/pages/session-details/playback/activity-plot/lazy-activity-plot.module.scss
- ui/src/pages/session-details/playback/activity-plot/types.ts
- ui/src/pages/session-details/playback/frame/types.ts
- ui/src/pages/session-details/playback/capture-navigation/capture-navigation.module.scss
- ui/src/pages/session-details/playback/capture-details/capture-job/process-now.tsx
- ui/src/pages/session-details/playback/capture-details/capture-details.tsx
- ui/src/pages/session-details/playback/playback.module.scss
- ui/src/nova-ui-kit/components/table/column-settings/column-settings.module.scss
- ui/src/pages/session-details/playback/capture-details/capture-details.module.scss
- ui/src/pages/session-details/playback/playback.tsx
- ui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx
- ui/src/pages/session-details/playback/capture-details/capture-job/capture-job-dialog.tsx
- ui/src/pages/session-details/playback/capture-details/capture-job/capture-job.tsx
- ui/src/pages/session-details/session-details.module.scss
- ui/src/nova-ui-kit/components/slider/timestamp-slider.tsx
- ui/src/pages/session-details/session-info/session-info.module.scss
- ui/src/nova-ui-kit/components/checkbox/checkbox.module.scss
| </Link> | ||
| ) : ( | ||
| <span className="body-base">{valueLabel}</span> | ||
| <span className="body-base break-all">{valueLabel}</span> |
Contributor
There was a problem hiding this comment.
Prefer break-words/overflow-wrap:anywhere here.
break-all will split filenames and paths in the middle of tokens, which makes the new capture metadata panel much harder to scan. A softer wrap mode still prevents overflow without mangling the text.
Suggested fix
- <span className="body-base break-all">{valueLabel}</span>
+ <span className="body-base break-words">{valueLabel}</span>📝 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
| <span className="body-base break-all">{valueLabel}</span> | |
| <span className="body-base break-words">{valueLabel}</span> |
🤖 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 `@ui/src/nova-ui-kit/components/info-block/info-block.tsx` at line 65, Replace
the hard wrap class on the value label span so tokens aren't split mid-word: in
the InfoBlock component update the span rendering {valueLabel} (the <span
className="body-base break-all"> element) to use a softer wrap, e.g. replace
"break-all" with "break-words" or add/replace with CSS that applies
"overflow-wrap: anywhere" (or "overflow-wrap: break-word") so long paths and
filenames wrap without being mangled.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ui/src/pages/session-details/capture-navigation.tsx`:
- Around line 106-116: The tooltip doesn't show because BasicTooltip with
asChild attaches to a native disabled Button (lines using BasicTooltip and
Button around goToPrevWithDetections/goToNextWithDetections), and disabled
buttons don't emit hover/focus events; fix by moving the asChild onto a
non-disabled wrapper (e.g., wrap the Button in a span/div and make that wrapper
the tooltip child) or instead remove the native disabled prop and use
aria-disabled on the Button while preventing action in the click handlers (gate
inside goToPrevWithDetections/goToNextWithDetections using
activeCapture?.prevCaptureId / activeCapture?.nextCaptureId) so the tooltip can
receive hover/focus.
🪄 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: 847ea731-f5b1-4599-a259-4860b1548d89
📒 Files selected for processing (7)
ui/src/nova-ui-kit/global.scssui/src/pages/session-details/capture-navigation.tsxui/src/pages/session-details/process/process-now.tsxui/src/pages/session-details/session-details.tsxui/src/pages/session-details/star-button.tsxui/src/pages/session-details/view-settings.tsxui/src/utils/language.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/src/pages/session-details/process/process-now.tsx
- ui/src/pages/session-details/view-settings.tsx
- ui/src/nova-ui-kit/global.scss
- ui/src/pages/session-details/session-details.tsx
annavik
force-pushed
the
layout/session-detail-redesign
branch
from
6521a02 to
1a9ffeb
Compare
Merged
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.
Summary
In this PR we redesign the session detail view. The current view felt a bit chaotic and didn't scale well (no room for new fields and settings). Also the session fields and charts were easy to miss for users.
In this PR we introduce a tab layout for session and capture details, to make it possible to fit more information in the same space. We also introduce dedicated popovers for processing and view settings, instead of mixing this with the data. The idea is also that the popovers can be extended with more settings in future. For example in the processing popover we could add quick action to process the full session?
Since we now have more room for capture detail fields, I also took the opportunity to include some data we didn't display before, for example resolution, filename and path.
Lastly, we update the style to default light mode. This will make the views in the app feel more connected, also we could simplify the UI code. In future we might want to support dark mode, but then I think we should do it using theme variables and apply for the full app.
Next steps
Next to the view settings, I have left room for zoom controls...
Detailed Description
Screenshots
Before:
After:
Deployment Notes
Only UI updates in this PR!
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores