Split Annotation Dashbord into sub components#9424
Open
hotzenklotz wants to merge 1 commit intomasterfrom
Open
Split Annotation Dashbord into sub components#9424hotzenklotz wants to merge 1 commit intomasterfrom
hotzenklotz wants to merge 1 commit intomasterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces two new dashboard components: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (3)
frontend/javascripts/dashboard/dashboard_top_bar.tsx (2)
12-28: Consider extracting the props type for better reusability.The inline props type definition works, but extracting it as a named type would improve readability and enable reuse if needed elsewhere.
Suggested refactor
+type DashboardTopBarProps = { + isAdminView: boolean; + handleOnSearch: SearchProps["onSearch"]; + handleSearchChanged: (event: React.ChangeEvent<HTMLInputElement>) => void; + searchQuery: string; + toggleShowArchived: () => void; + shouldShowArchivedAnnotations: boolean; + archiveAll: () => void; +}; + -export function DashboardTopBar({ - isAdminView, - handleOnSearch, - handleSearchChanged, - searchQuery, - toggleShowArchived, - shouldShowArchivedAnnotations, - archiveAll, -}: { - isAdminView: boolean; - handleOnSearch: SearchProps["onSearch"]; - handleSearchChanged: (event: React.ChangeEvent<HTMLInputElement>) => void; - searchQuery: string; - toggleShowArchived: () => void; - shouldShowArchivedAnnotations: boolean; - archiveAll: () => void; -}) { +export function DashboardTopBar({ + isAdminView, + handleOnSearch, + handleSearchChanged, + searchQuery, + toggleShowArchived, + shouldShowArchivedAnnotations, + archiveAll, +}: DashboardTopBarProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/dashboard/dashboard_top_bar.tsx` around lines 12 - 28, Extract the inline props annotation for DashboardTopBar into a named type/interface (e.g., DashboardTopBarProps) and use it as the function's parameter type; include all current prop names and their types (isAdminView, handleOnSearch, handleSearchChanged, searchQuery, toggleShowArchived, shouldShowArchivedAnnotations, archiveAll) in that new type to improve readability and enable reuse across the codebase. Update the function signature to accept props: DashboardTopBarProps and export the new type if it may be reused elsewhere.
33-42: Consider adding a placeholder for better UX.The
Searchinput would benefit from a placeholder to guide users on what they can search for (e.g., annotation names, tags, IDs).Suggested improvement
const search = ( <Search style={{ width: 200, }} onSearch={handleOnSearch} onChange={handleSearchChanged} value={searchQuery} + placeholder="Search annotations" /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/dashboard/dashboard_top_bar.tsx` around lines 33 - 42, Add a user-facing placeholder to the Search component to improve discoverability; update the JSX for Search (the Search element using props onSearch={handleOnSearch}, onChange={handleSearchChanged}, value={searchQuery}) to include a placeholder prop (e.g., placeholder="Search annotations, tags or IDs") so users know what to search for.frontend/javascripts/dashboard/dashboard_empty_annotations_placeholder.tsx (1)
7-7: Use numeric value forspanprop.The
Colspanprop is typed asnumber | string, but using a number is the standard convention and ensures type consistency.Suggested fix
- <Col span="6"> + <Col span={6}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/javascripts/dashboard/dashboard_empty_annotations_placeholder.tsx` at line 7, The Col component is passing span as a string; change the prop to a numeric value to match the typed convention (number | string) and ensure type consistency: update the JSX usage of Col (the span prop) from a quoted string to a numeric expression (e.g., replace span="6" with span={6}) in dashboard_empty_annotations_placeholder.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/javascripts/dashboard/explorative_annotations_view.tsx`:
- Around line 574-576: The current check shows
DashboardEmptyAnnotationsPlaceholder whenever filteredAndSortedAnnotations is
empty and this.state.isLoading is false, which can mislead users if
filters/search are active; update the conditional around
filteredAndSortedAnnotations to first detect whether any filters/search/tags are
active (e.g. check this.state.query, this.state.selectedTags or the component's
filter props) and only render DashboardEmptyAnnotationsPlaceholder when there
are truly no annotations and no active filters; if filters are active and the
filtered list is empty, render a distinct "no results for current filters"
message or nothing instead of the Create-an-Annotation placeholder so users
aren't confused.
---
Nitpick comments:
In `@frontend/javascripts/dashboard/dashboard_empty_annotations_placeholder.tsx`:
- Line 7: The Col component is passing span as a string; change the prop to a
numeric value to match the typed convention (number | string) and ensure type
consistency: update the JSX usage of Col (the span prop) from a quoted string to
a numeric expression (e.g., replace span="6" with span={6}) in
dashboard_empty_annotations_placeholder.tsx.
In `@frontend/javascripts/dashboard/dashboard_top_bar.tsx`:
- Around line 12-28: Extract the inline props annotation for DashboardTopBar
into a named type/interface (e.g., DashboardTopBarProps) and use it as the
function's parameter type; include all current prop names and their types
(isAdminView, handleOnSearch, handleSearchChanged, searchQuery,
toggleShowArchived, shouldShowArchivedAnnotations, archiveAll) in that new type
to improve readability and enable reuse across the codebase. Update the function
signature to accept props: DashboardTopBarProps and export the new type if it
may be reused elsewhere.
- Around line 33-42: Add a user-facing placeholder to the Search component to
improve discoverability; update the JSX for Search (the Search element using
props onSearch={handleOnSearch}, onChange={handleSearchChanged},
value={searchQuery}) to include a placeholder prop (e.g., placeholder="Search
annotations, tags or IDs") so users know what to search for.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cab5d838-228e-44b3-8968-7ca7d2875192
📒 Files selected for processing (4)
frontend/javascripts/dashboard/dashboard_empty_annotations_placeholder.tsxfrontend/javascripts/dashboard/dashboard_top_bar.tsxfrontend/javascripts/dashboard/explorative_annotations_view.tsxfrontend/javascripts/viewer/view/components/categorization_label.tsx
Comment on lines
+574
to
576
| if (filteredAndSortedAnnotations.length === 0 && !this.state.isLoading) { | ||
| return <DashboardEmptyAnnotationsPlaceholder />; | ||
| } |
Contributor
There was a problem hiding this comment.
Empty placeholder may be misleading when filters are active.
When filteredAndSortedAnnotations is empty due to active search query or tags (not because the user has no annotations), the placeholder displays "Create an Annotation" with a link to the datasets page. This could confuse users who have annotations but are seeing empty results due to their filters.
Consider checking if filters/tags are active and either showing a different message or not showing this placeholder when the empty state is filter-induced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/javascripts/dashboard/explorative_annotations_view.tsx` around lines
574 - 576, The current check shows DashboardEmptyAnnotationsPlaceholder whenever
filteredAndSortedAnnotations is empty and this.state.isLoading is false, which
can mislead users if filters/search are active; update the conditional around
filteredAndSortedAnnotations to first detect whether any filters/search/tags are
active (e.g. check this.state.query, this.state.selectedTags or the component's
filter props) and only render DashboardEmptyAnnotationsPlaceholder when there
are truly no annotations and no active filters; if filters are active and the
filtered list is empty, render a distinct "no results for current filters"
message or nothing instead of the Create-an-Annotation placeholder so users
aren't confused.
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.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)