Improve quality UX #8865
Improve quality UX #8865
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces enhancements to the quality control components in the CVAT UI. The changes focus on improving the user interface, adding new functionality like search and download capabilities for allocation tables, and refining the styling of quality control pages. The modifications span multiple files, including component logic, styles, and test selectors, with a primary emphasis on making the quality management tab more responsive and informative. Changes
Sequence DiagramsequenceDiagram
participant User
participant QualityManagementTab
participant AllocationTable
participant QualityTableHeader
User->>QualityManagementTab: Access Quality Control
QualityManagementTab->>AllocationTable: Render with pageWidth
AllocationTable->>QualityTableHeader: Use for Table Header
User->>QualityTableHeader: Perform Search
QualityTableHeader->>AllocationTable: Filter Data
User->>QualityTableHeader: Download CSV
QualityTableHeader-->>User: Trigger CSV Download
Poem
Finishing Touches
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: 0
🧹 Nitpick comments (7)
cvat-ui/src/components/quality-control/task-quality/quality-table-header.tsx (3)
12-17: Consider creating a more explicit type for theonDownloadreturn value
Defining a stronger contract (e.g., an interface) for{ filename: string; data: ... }may improve clarity and reduce maintenance overhead, especially if additional metadata or file formats are supported in the future.
25-33: Potential large-file performance concern
For very large data sets, usingBlobwith an in-memory string can be memory-intensive. You may consider a streaming approach or chunked writing if there's a possibility of generating particularly large CSV files.
35-74: Recommend adding debounce for the search bar
The search function executes on eachenterButtonpress by default, which is fine, but if you plan to add "onChange" searching or a heavier search logic in the future, consider a debounce to reduce performance overhead. Also, the component structure is well-structured and consistent with your styling approach.cvat-ui/src/components/quality-control/task-quality/quality-magement-tab.tsx (1)
46-59: Responsive logic withuseLayoutEffect
Listening to window resize events is an intuitive approach for responsive table design. Consider a small throttle or debounce if the parent container triggers many consecutive resize events (e.g., rapid window resizing).cvat-ui/src/components/quality-control/task-quality/allocation-table.tsx (1)
80-80: CalculatingframeNameWidth
Using the offset approach to derive a column’s width is pragmatic. However, ensure it scales well at very small or large screen sizes by possibly clamping (e.g. max/min).cvat-ui/src/components/quality-control/styles.scss (2)
139-144: Consider removing!importantflagWhile the padding adjustments are good for consistency, using
!importantshould be avoided. Consider increasing selector specificity instead:- .ant-table-tbody .ant-table-cell { - padding: $grid-unit-size * 0.5 $grid-unit-size !important; - } + .cvat-frame-allocation-table .ant-table-container .ant-table-tbody .ant-table-cell { + padding: $grid-unit-size * 0.5 $grid-unit-size; + }
155-170: Review negative margin impact on responsive layoutsThe negative margins in
.cvat-quality-control-management-tab-summarymight cause layout issues on smaller screens or when the content overflows. Consider:
- Using padding on the parent container instead
- Adding media queries for responsive behavior
- Setting a min-width to prevent unwanted wrapping
.cvat-quality-control-management-tab-summary { - margin-left: - $grid-unit-size; - margin-right: - $grid-unit-size; + padding: 0 $grid-unit-size; + min-width: 320px; // Add minimum width to prevent unwanted wrapping >.ant-col { padding: 0 $grid-unit-size; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cvat-ui/src/components/quality-control/quality-control-page.tsx(1 hunks)cvat-ui/src/components/quality-control/styles.scss(3 hunks)cvat-ui/src/components/quality-control/task-quality/allocation-table.tsx(6 hunks)cvat-ui/src/components/quality-control/task-quality/quality-magement-tab.tsx(3 hunks)cvat-ui/src/components/quality-control/task-quality/quality-table-header.tsx(1 hunks)tests/cypress/e2e/features/ground_truth_jobs.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cvat-ui/src/components/quality-control/quality-control-page.tsx
🔇 Additional comments (21)
cvat-ui/src/components/quality-control/task-quality/quality-table-header.tsx (1)
19-23: Be cautious about CSV injection
When data contains malicious inputs (e.g., entries beginning with '=' or '+'), spreadsheet programs might interpret them as formulas. Consider sanitizing each cell to defend against CSV injection attacks.
Would you like help implementing a sanitization approach for cells starting with special characters?
cvat-ui/src/components/quality-control/task-quality/quality-magement-tab.tsx (7)
5-12: Multiple new imports for layout effect, references, and typing
Imports look appropriate for the planned responsive design changes, referencing both useLayoutEffect and useState. The usage of Text from antd/lib/typography/Text is consistent with the rest of the codebase’s styling approach.
32-34: Ref usage for container width
Using a ref to capture the container width is a neat way to dynamically size child components. Be mindful of cross-browser differences in how clientWidth is computed (e.g., if padding or borders matter).
[approve]
38-45: Fine-grained validation mode text
Assigning descriptive validationModeText provides clarity for the user. Ensuring all relevant modes are covered will prevent fallback to null.
63-76: Clear usage of AnalyticsCard
These cards neatly display important metrics (total, excluded, active). Implementation is straightforward and should help maintain clarity for end users.
83-91: Conditional rendering of validation mode
The conditionally rendered note on validation mode is straightforward. This text is helpful for user awareness.
93-93: Ref container reference for the table
Building on the earlier observation, referencing DOM elements is a standard practice. Looks good.
102-102: Passing pageWidth to <AllocationTable>
Sending the computed container width down to child components is a solid approach for dynamic interface adaptation.
cvat-ui/src/components/quality-control/task-quality/allocation-table.tsx (8)
21-21: Importing QualityTableHeader
Integrating the new header component is consistent with the modular approach to building UI components.
29-29: New prop pageWidth in Props
Clear addition for responsive layouts. Keep an eye on usage contexts to avoid null or undefined references if parent-level logic changes.
40-41: Offset constant
Defining COLUMNS_OFFSET_PERCENT offers clarity rather than using a numeric literal. Good for maintainability.
66-72: Implementing search logic
Filtering data based on user query is basic but effective. For extended search logic (fuzzy match, advanced queries), consider more sophisticated libraries.
74-78: Download logic
Exporting only the rest of fields and removing key in CSV is consistent with typical CSV structures. The approach is straightforward and aligns perfectly with the QualityTableHeader usage.
108-127: Rendering the “Name” column with a clickable button
The combination of a tooltip and clickable area is user-friendly. Providing href improves accessibility for screen readers. Great job.
159-190: Integrating QualityTableHeader
The new search & download features live neatly in the header. Conditional rendering of group operations based on selectedRowKeys is also well-handled.
200-200: Updating dataSource to filteredData
Ensures the table instantly re-renders upon search. Straightforward approach, consistent with the rest of the filtering logic.
tests/cypress/e2e/features/ground_truth_jobs.js (2)
277-277: Selector changed to .cvat-quality-control-management-tab-summary
Renaming the summary selector to match the new layout is consistent. Ensure references throughout the test suite are aligned with the updated naming to avoid breakages in future merges.
286-286: Verification checks for summary metrics
These additional assertions confirm that summary cards accurately reflect totals, excluded frames, and active frames. Very clear approach to E2E coverage.
cvat-ui/src/components/quality-control/styles.scss (3)
8-8: LGTM! Improved scroll handling
The change from overflow-y: auto to overflow: hidden auto is a good improvement that prevents unwanted horizontal scrollbars while maintaining vertical scrolling when needed.
151-151: Verify height adjustment across different screen sizes
The reduction in height calculation (from 33 to 30 grid units) might affect the content visibility. Please verify that this works well across different screen sizes and doesn't cause content truncation.
101-107: Verify RTL direction impact on LTR text
While the text overflow handling is good, using direction: rtl might affect the display of left-to-right text. Please verify that this doesn't cause issues with:
- Text alignment for different languages
- Special characters or numbers
- Text selection behavior
Run this script to check for potential text content:
✅ Verification successful
The previous script failed because the file type 'tsx' wasn't recognized. Let's try with standard TypeScript/JavaScript files and also look directly in the quality control component directory for any text content that might be displayed in this button.
Let me analyze the button's text content and usage based on the search results. I found that the button is used in the allocation table component and displays frame names with tooltips:
RTL direction is safe to use in this context
The direction: rtl is used in conjunction with text overflow handling properties for a specific purpose - to show text truncation from the left side instead of the default right side. This is a common CSS pattern when you want to show the end of the text rather than the beginning when it's truncated.
Looking at the code:
- The button is used to display frame names in the allocation table
- It's a link-type button that navigates to specific frames
- The RTL direction is specifically scoped to this button class and won't affect other components
- The text content is simple frame names/numbers that won't be affected by RTL direction
The combination of:
word-wrap: break-word;
word-break: break-word;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
direction: rtl;is a valid pattern for left-side text truncation and won't cause issues with:
- Text alignment (due to nowrap)
- Special characters (due to isolated button scope)
- Text selection (handled by browser)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for text content passed to this button component
rg -l --type ts --type tsx "cvat-open-frame-button"
Length of output: 79
Script:
#!/bin/bash
# Search for text content in TypeScript/JavaScript files
rg -l "cvat-open-frame-button" --type ts --type js
# Also search in the specific component directory for any text content
rg -A 5 "text|label|title" "cvat-ui/src/components/quality-control/"
Length of output: 37051
Script:
#!/bin/bash
# Let's check for any frame name or text content handling in the component
rg -A 5 "cvat-open-frame-button" "cvat-ui/src/components/quality-control/task-quality/allocation-table.tsx"
# Also check for any RTL-specific text or translations
rg "rtl|direction:" "cvat-ui/src/components/quality-control/"
Length of output: 966
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8865 +/- ##
===========================================
- Coverage 73.93% 73.89% -0.05%
===========================================
Files 411 412 +1
Lines 44245 44261 +16
Branches 3993 3993
===========================================
- Hits 32714 32706 -8
- Misses 11531 11555 +24
|
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
QualityTableHeadercomponent for improved search and download functionalities.Improvements
Bug Fixes