Changes frozen to waiting #1551
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/components/primitives/Select.tsxOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe changes in this pull request focus on enhancing the user interface and visual feedback of various components within the web application. Modifications were made to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
apps/webapp/app/components/primitives/Select.tsx (1)
485-489: LGTM! Consider adjusting transition durationThe hover state implementation for the ShortcutKey looks good and aligns with the PR objectives. However, setting
duration-0effectively disables the transition, which might make the hover effect feel abrupt.Consider using a short duration instead of disabling the transition completely:
- className={cn("size-4 flex-none transition duration-0 group-hover:border-charcoal-600")} + className={cn("size-4 flex-none transition duration-75 group-hover:border-charcoal-600")}apps/webapp/app/components/runs/v3/RunFilters.tsx (1)
Line range hint
673-920: Consider refactoring ID-related dropdowns to reduce code duplication.The RunId, BatchId, and ScheduleId dropdown components share similar structure, validation logic, and error messages. Consider extracting these into a reusable component to improve maintainability.
Create a common component like this:
type IdDropdownProps = { idType: 'run' | 'batch' | 'schedule'; prefix: string; length: number; value?: string; onChange: (value?: string) => void; // ... other common props }; function IdDropdown({ idType, prefix, length, value, onChange, ...props }: IdDropdownProps) { const [id, setId] = useState(value); let error: string | undefined = undefined; if (id) { if (!id.startsWith(prefix)) { error = `${idType} IDs start with '${prefix}'`; } else if (id.length !== length) { error = `${idType} IDs are ${length} characters long`; } } // ... rest of the common implementation }This would reduce duplication and make future maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/webapp/app/components/primitives/Select.tsx(1 hunks)apps/webapp/app/components/runs/v3/RunFilters.tsx(1 hunks)apps/webapp/app/components/runs/v3/TaskRunAttemptStatus.tsx(4 hunks)apps/webapp/app/components/runs/v3/TaskRunStatus.tsx(6 hunks)docs/runs.mdx(1 hunks)
🔇 Additional comments (4)
apps/webapp/app/components/runs/v3/TaskRunAttemptStatus.tsx (1)
10-10: LGTM! Changes align with the PR objectives.
The changes consistently replace the "Frozen" state with "Waiting" across icons, colors, and text labels. The new visual representation using HourglassIcon and text-charcoal-500 better conveys the waiting state and maintains consistency with other similar states.
Also applies to: 67-67, 93-93, 119-119
apps/webapp/app/components/runs/v3/TaskRunStatus.tsx (2)
57-71: LGTM! Status descriptions are more consistent and clearer.
The descriptions now consistently end with periods and provide clearer explanations of each state.
128-128: LGTM! Visual representation changes are consistent.
The changes to icon, color, and text for the "WAITING_TO_RESUME" state maintain consistency with TaskRunAttemptStatus.tsx and align with the PR objectives.
Also applies to: 167-167, 202-202
docs/runs.mdx (1)
45-46: LGTM! Documentation accurately reflects the UI changes.
The updated description clearly explains when the "Waiting" state occurs and provides helpful links to related documentation (triggerAndWait(), batchTriggerAndWait(), and wait functions).
| @@ -356,7 +356,7 @@ function StatusDropdown({ | |||
| <TooltipTrigger className="group flex w-full flex-col py-0"> | |||
| <TaskRunStatusCombo status={item.value} iconClassName="animate-none" /> | |||
| </TooltipTrigger> | |||
| <TooltipContent side="right" sideOffset={9}> | |||
| <TooltipContent side="right" sideOffset={50}> | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the comprehensive analysis of the codebase, I can now provide a final response:
Inconsistent tooltip offset needs standardization
The increased sideOffset={50} in RunFilters.tsx is inconsistent with the identical component structure in BatchFilters.tsx (sideOffset={9}). Both components:
- Use the same tooltip structure
- Display status descriptions in the same way
- Are used in similar dropdown contexts
- Share similar component hierarchy
This inconsistency should be standardized to maintain UI consistency across similar components.
apps/webapp/app/components/runs/v3/RunFilters.tsx: Line 359 - Revert sideOffset to 9 to match BatchFiltersapps/webapp/app/components/runs/v3/BatchFilters.tsx: Line 359 - Keep current sideOffset of 9
🔗 Analysis chain
Verify tooltip positioning with the increased offset.
The significant increase in sideOffset from 9 to 50 pixels might cause layout issues on smaller screens or place the tooltip too far from its trigger element.
The search results show an inconsistency in the sideOffset values between similar components. Let me verify if these components are related or if there's a pattern for different offset values.
The context shows that both tooltips are used in similar dropdown components (BatchFilters and RunFilters) to display status descriptions. However, they have significantly different offsets (9px vs 50px) despite having identical structure and purpose. Let me check if there are any UI-specific requirements or recent changes that justify this difference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other TooltipContent components to compare offsets
rg "TooltipContent.*sideOffset" -A 1
Length of output: 480
Script:
#!/bin/bash
# Check the context of both tooltip usages to understand their placement and purpose
rg "TooltipContent" -B 5 -A 5 apps/webapp/app/components/runs/v3/RunFilters.tsx apps/webapp/app/components/runs/v3/BatchFilters.tsx
# Search for any design system documentation or constants related to tooltip offsets
fd -e ts -e tsx -e js -e jsx | xargs rg "tooltip.*offset|TOOLTIP.*OFFSET|TooltipContent.*DEFAULT"
Length of output: 4882
Script:
#!/bin/bash
# Check recent changes to these files
git log -p --since="3 months ago" -- apps/webapp/app/components/runs/v3/RunFilters.tsx apps/webapp/app/components/runs/v3/BatchFilters.tsx | grep -A 5 -B 5 "sideOffset"
# Check if there are any related UI components or design system files
fd -e tsx -e ts | xargs rg "TaskRunStatusCombo|BatchStatusCombo" -A 2 -B 2
Length of output: 15001
Replaced "Frozen" with "Waiting"
Docs page update
Also improved the hover state on the Shortcut icon in the Select component. It now has a visible border on hover.
Summary by CodeRabbit
New Features
SelectItemcomponent with updated styles.Bug Fixes
Documentation