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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a FormMessage component and SCSS classes, reverses several model job-selection sort comparators, injects form guidance messages, resets upload hook state when dialogs open, tweaks select/value handling and various layout/spacing/styles, and updates translation keys and tooltips. (50 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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 (2)
ui/src/design-system/components/select/entity-picker.tsx (1)
19-26: Thekey={value}pattern causes excessive remounts on every selection.The current implementation forces a remount of the Select component whenever
valuechanges—including when the user picks a new valid entity, not just when entities finish loading or validity status flips. This leads to unnecessary React reconciliation and could disrupt user interaction (e.g., dropdown closes mid-interaction, focus lost).If the goal is to force a re-render when the resolved value's validity changes (e.g., transitioning from
''to a valid ID after entities load), consider keying on the validity state rather than the value itself:♻️ Suggested fix: key on validity transition, not every value
- const value = entities.some((e) => e.id === _value) ? _value : '' + const isValueValid = entities.some((e) => e.id === _value) + const value = isValueValid ? _value : '' return ( <Select.Root - key={value} + key={`${isLoading}-${isValueValid}`} disabled={isLoading || entities.length === 0} onValueChange={onValueChange} value={value} >This keys on whether entities are still loading and whether the current value is valid, triggering remounts only when: (a) loading completes, or (b) the value transitions from valid to invalid or vice versa—rather than on every user selection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/design-system/components/select/entity-picker.tsx` around lines 19 - 26, The Select.Root is being keyed by the selected value (value) which causes a full remount on every selection; change the key to reflect only the validity/loading transition instead (e.g., derive a boolean like isValid = entities.some(e => e.id === _value) and use a key based on isLoading and isValid) so the component only remounts when loading finishes or validity flips; update the key usage where Select.Root is rendered and ensure you reference the existing symbols (_value, entities, isLoading, value, Select.Root) when implementing the new key.ui/src/pages/captures/upload-images-dialog/upload-images-dialog.tsx (1)
64-70: Consider addingresetHookto the dependency array or wrapping it withuseCallback.The
useEffectusesresetHookbut doesn't include it in the dependency array. While this works for the intended use case (resetting onisOpenchange), it may trigger ESLint'sreact-hooks/exhaustive-depswarning.Since
resetHookis recreated on each render inuseUploadCaptures, adding it to deps could cause extra effect runs. If you want to suppress the lint warning while keeping current behavior, consider either:
- Wrapping the
resetfunction inuseCallbackwithinuseUploadCaptures- Adding an ESLint disable comment if the current behavior is intentional
Option: Stabilize reset with useCallback in useUploadCaptures.ts
// In useUploadCaptures.ts +import { useCallback, useState } from 'react' -import { useState } from 'react' // ... +const resetFn = useCallback(() => { + setResults([]) + reset() +}, [reset]) return { // ... - reset: () => { - setResults([]) - reset() - }, + reset: resetFn, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/captures/upload-images-dialog/upload-images-dialog.tsx` around lines 64 - 70, The effect in upload-images-dialog.tsx calls resetHook but omits it from the dependency array, which triggers exhaustives-deps warnings; fix this by stabilizing resetHook in the provider hook (wrap the reset function in useCallback inside useUploadCaptures) so its identity is stable and then add resetHook to the useEffect deps in upload-images-dialog.tsx (or, if the current behavior is intentionally tied only to isOpen, add an inline ESLint disable comment on that useEffect). Reference resetHook, useEffect in upload-images-dialog.tsx and the reset function inside useUploadCaptures when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/components/form/layout/layout.module.scss`:
- Around line 60-63: The SCSS has an argumentless mixin called with parentheses
and a missing blank line before a declaration: replace the call "@include
paragraph-small();" with the argumentless form "@include paragraph-small" in the
.formMessage block (and the other occurrence), and add a blank line before the
"padding: 8px 16px;" declaration so there is an empty line before that property.
In
`@ui/src/pages/captures/upload-images-dialog/select-images-section/select-images-section.tsx`:
- Around line 72-85: The button rendered in SelectImagesSection (the <button>
that uses onClick, buttonVariants and contains <PlusIcon>) needs an explicit
type attribute to avoid defaulting to submit; update that element to include
type="button" so it won't trigger parent form submissions when clicked.
In
`@ui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx`:
- Line 25: The showLabels computation dereferences session.endDate before the
early-return null guard, causing a crash when endDate is missing; move the const
showLabels = session.startDate.getTime() !== session.endDate.getTime() so it
runs after the existing endDate guard (or change it to a safe check using
session.endDate?.getTime()), updating the symbol showLabels in
session-captures-slider.tsx and keeping the early return logic intact.
- Around line 27-31: The effect in useEffect that computes
setValue(dateToValue({ date: activeCapture.date, startDate, endDate })) only
lists activeCapture as a dependency, causing stale values when session bounds
change; modify the dependency array for that effect to include startDate and
endDate (and any stable helpers like dateToValue or setValue if they are not
stable) so the effect re-runs whenever activeCapture, startDate, or endDate
change.
In `@ui/src/utils/language.ts`:
- Around line 619-621: Update the tooltip strings to correct grammar and tighten
wording: change the standalone tooltip (the first string) to use "Examples of
such tasks include processing captures, syncing captures, and generating
exports." and update STRING.TOOLTIP_LATEST_JOB_STATUS to a tighter sentence such
as "A job is a background task that takes time to complete; this shows the
status of the latest job related to each {{type}}. Hover the status label for
details." Locate and update the two relevant constants (the standalone tooltip
string and STRING.TOOLTIP_LATEST_JOB_STATUS) to these improved phrasings.
---
Nitpick comments:
In `@ui/src/design-system/components/select/entity-picker.tsx`:
- Around line 19-26: The Select.Root is being keyed by the selected value
(value) which causes a full remount on every selection; change the key to
reflect only the validity/loading transition instead (e.g., derive a boolean
like isValid = entities.some(e => e.id === _value) and use a key based on
isLoading and isValid) so the component only remounts when loading finishes or
validity flips; update the key usage where Select.Root is rendered and ensure
you reference the existing symbols (_value, entities, isLoading, value,
Select.Root) when implementing the new key.
In `@ui/src/pages/captures/upload-images-dialog/upload-images-dialog.tsx`:
- Around line 64-70: The effect in upload-images-dialog.tsx calls resetHook but
omits it from the dependency array, which triggers exhaustives-deps warnings;
fix this by stabilizing resetHook in the provider hook (wrap the reset function
in useCallback inside useUploadCaptures) so its identity is stable and then add
resetHook to the useEffect deps in upload-images-dialog.tsx (or, if the current
behavior is intentionally tied only to isOpen, add an inline ESLint disable
comment on that useEffect). Reference resetHook, useEffect in
upload-images-dialog.tsx and the reset function inside useUploadCaptures when
making the change.
🪄 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: db545678-b323-472c-9a06-f7a4f6341cb6
📒 Files selected for processing (25)
ui/src/components/form/layout/layout.module.scssui/src/components/form/layout/layout.tsxui/src/components/header/user-info-dialog/user-info-image-upload/user-info-image-upload.tsxui/src/data-services/hooks/captures/useUploadCaptures.tsui/src/data-services/models/capture-details.tsui/src/data-services/models/capture-set.tsui/src/data-services/models/deployment.tsui/src/design-system/components/file-input/file-input.module.scssui/src/design-system/components/image-upload/image-upload.tsxui/src/design-system/components/select/entity-picker.tsxui/src/pages/captures/upload-images-dialog/select-images-section/select-images-section.tsxui/src/pages/captures/upload-images-dialog/upload-images-dialog.tsxui/src/pages/deployments/deployment-columns.tsxui/src/pages/jobs/jobs-columns.tsxui/src/pages/project/capture-sets/capture-set-columns.tsxui/src/pages/project/entities/details-form/capture-set-details-form.tsxui/src/pages/project/entities/details-form/export-details-form.tsxui/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/playback.module.scssui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsxui/src/pages/session-details/playback/utils.tsxui/src/pages/session-details/session-details.tsxui/src/utils/language.tsui/src/utils/useFilters.ts
💤 Files with no reviewable changes (2)
- ui/src/utils/useFilters.ts
- ui/src/pages/jobs/jobs-columns.tsx
ui/src/pages/captures/upload-images-dialog/select-images-section/select-images-section.tsx
Show resolved
Hide resolved
ui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx
Show resolved
Hide resolved
ui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx
Show resolved
Hide resolved
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 `@ui/src/utils/language.ts`:
- Line 506: Update the UI tip string in ui/src/utils/language.ts (the
capture-set help text) to improve grammar and readability: replace 'To define a
capture set for all captures, use method "Full" without filters set.' with a
tighter variant such as 'To define a capture set for all captures, use the
"Full" method with no filters.' so it reads naturally in the UI; ensure you
update the same string literal used by the capture-set help function/constant in
that file.
🪄 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: 008e533e-edab-4f1d-ba4f-1a0f84cf2694
📒 Files selected for processing (3)
ui/src/pages/captures/upload-images-dialog/select-images-section/select-images-section.tsxui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsxui/src/utils/language.ts
✅ Files skipped from review due to trivial changes (1)
- ui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/pages/captures/upload-images-dialog/select-images-section/select-images-section.tsx
annavik
force-pushed
the
chore/ui-cleanup
branch
from
b5c0691 to
266f1b5
Compare
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 of changes
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor