Some Timetracking view improvements #8170
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Request time tracking data
Frontend->>Backend: Fetch time tracking data with annotationState
Backend->>Database: Query with annotationState filter
Database-->>Backend: Return filtered results
Backend-->>Frontend: Send data with annotationState
Frontend-->>User: Display time tracking data
Assessment against linked issues
Poem
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 full review |
✅ Actions performedFull review triggered. |
| options={filterOptions} | ||
| optionFilterProp="label" | ||
| value={selectedFilters} | ||
| popupMatchSelectWidth={false} |
There was a problem hiding this comment.
This prop enables fully showing long project names as stated in the issue.
But the antd docs to this prop state:
Determine whether the popup menu and the select input are the same width. Default set min-width same as input. Will ignore when value less than select width. false will disable virtual scroll.
Where the last sentence is important IMO -> virtual scrolling is now disabled by this change. This means that for an organization with many projects, that the list might be slow.
Do you think this is acceptable?
There was a problem hiding this comment.
I'm not aware of any orga making such extensive use of projects that this would be an issue. An alternative could be to set a reasonable width (somewhat larger than before) and then show the full project name on hover. No real preference on my side.
There was a problem hiding this comment.
An alternative could be to set a reasonable width (somewhat larger than before) and then show the full project name on hover. No real preference on my side.
That's definitely also doable :)
I'll switch to this behavior 👍
![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 (12)
frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts (1)
Line range hint
31-39: Consider adding test cases for different annotation statesWhile testing with
AnnotationStateFilterEnum.ALLis good, we should also verify the filtering functionality works correctly with other states (e.g., ACTIVE, FINISHED).Consider adding these test cases:
test("getTimeTrackingForUserSpans with active annotations", async (t) => { const timeTrackingForUser = await api.getTimeTrackingForUserSpans( activeUser.id, dayjs("20180101", "YYYYMMDD").valueOf(), dayjs("20181001", "YYYYMMDD").valueOf(), "Task", AnnotationStateFilterEnum.ACTIVE, ); t.true(timeTrackingForUser.length >= 0); t.snapshot(replaceVolatileValues(timeTrackingForUser)); }); test("getTimeTrackingForUserSpans with finished annotations", async (t) => { const timeTrackingForUser = await api.getTimeTrackingForUserSpans( activeUser.id, dayjs("20180101", "YYYYMMDD").valueOf(), dayjs("20181001", "YYYYMMDD").valueOf(), "Task", AnnotationStateFilterEnum.FINISHED, ); t.true(timeTrackingForUser.length >= 0); t.snapshot(replaceVolatileValues(timeTrackingForUser)); });frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx (3)
90-92: Consider adding loading state handlingBased on the linked issue #7788 mentioning the need to "implement a spinner for the expanding view", consider adding loading state handling using the
useFetchhook's loading state.+ const { data: userData, loading } = useFetch( - const userData = useFetch( async () => { return await getTimeTrackingForUserSummedPerAnnotation( props.userId, dayjs(props.dateRange[0]), dayjs(props.dateRange[1]), props.annotationType, props.annotationState, props.projectIds, ); }, [], [props], ); + if (loading) { + return <Spin size="large" />; + } const [annotationRows, taskRows] = renderRow(userData);
Line range hint
47-54: Address stats column alignment issuePer issue #7788, the stats column should be left-aligned to reduce visual noise caused by centered alignment with varying number widths.
<Col span={STATISTICS_SPAN}> <AnnotationStats stats={aggregateStatsForAllLayers(timeEntry.annotationLayerStats)} asInfoBlock={false} withMargin={false} + style={{ textAlign: 'left' }} /> </Col>
Line range hint
63-66: Improve long project name visibilityTo address the PR objective of ensuring long project names are fully visible:
<Row style={{ fontWeight: "bold", margin: "5px 20px" }}> - <Col>{project}</Col> + <Col style={{ + wordBreak: 'break-word', + whiteSpace: 'normal', + maxWidth: '100%' + }}>{project}</Col> </Row>,frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (2)
49-55: Consider adding "All" state to filter options.While
AnnotationStateFilterEnumincludes anALLstate, it's not present in the filter options. Consider adding it for consistency and to allow users to explicitly reset the filter.const ANNOTATION_STATE_FILTERS: NestedSelectOptions = { label: "Filter by state", options: [ + { label: "All", value: AnnotationStateFilterEnum.ALL }, { label: "Active", value: AnnotationStateFilterEnum.ACTIVE }, { label: "Finished / Archived", value: AnnotationStateFilterEnum.FINISHED_OR_ARCHIVED }, ], };
81-88: LGTM! Consider extracting the condition for better readability.The state management logic is correct, but the condition could be more readable.
- const selectedKeys = - selectedAnnotationState !== AnnotationStateFilterEnum.ALL ? [selectedAnnotationState] : []; + const shouldIncludeAnnotationState = selectedAnnotationState !== AnnotationStateFilterEnum.ALL; + const selectedKeys = shouldIncludeAnnotationState ? [selectedAnnotationState] : [];app/models/user/time/TimeSpan.scala (1)
183-190: Consider using named constants for tuple indices.While the JSON field mappings are correct, using magic numbers for tuple indices (e.g.,
_6,_7) makes the code harder to maintain. Consider introducing named constants for better readability and maintainability.Here's a suggested improvement:
private object TimeSpanTupleIndices { val UserId = 1 val UserEmail = 2 val DatasetOrg = 3 val DatasetName = 4 val AnnotationId = 5 val AnnotationState = 6 val TaskId = 7 val ProjectName = 8 val TaskTypeId = 9 val TaskTypeSummary = 10 val TimeSpanId = 11 val TimeSpanCreated = 12 val TimeSpanTimeMillis = 13 } private def formatTimespanTuple(tuple: (...)) = { import TimeSpanTupleIndices._ Json.obj( "userId" -> tuple.productElement(UserId), "userEmail" -> tuple.productElement(UserEmail), // ... and so on ) }frontend/javascripts/admin/statistic/time_tracking_overview.tsx (1)
195-202: Consider adding error handling for the download operationWhile the integration of
selectedStateis correct, consider adding error handling and user feedback for the download operation.onClick={async () => { + try { downloadTimeSpans( user.id, startDate, endDate, selectedTypes, selectedState, selectedProjectIds, ); + } catch (error) { + Toast.error(messages["timetracking.download_failed"] || "Failed to download time spans"); + } }}frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.md (1)
Line range hint
14-276: Consider adding test cases for mixed annotation states.The current snapshots show separate lists where all entries have the same annotation state ('Active'). To thoroughly test the filtering functionality, consider adding test cases that:
- Mix active and finished annotations in the same response
- Test the filter parameter explicitly
- Verify the CSV export format
This would help ensure the new filtering feature works correctly in all scenarios.
frontend/javascripts/types/api_flow_types.ts (1)
586-586: Consider using a string literal union type for annotationState.The addition of
annotationStatealigns well with the PR objectives. However, since the PR mentions specific states ("active", "finished/archived"), consider using a string literal union type to restrict possible values and improve type safety.- annotationState: string; + annotationState: "active" | "finished" | "archived";frontend/javascripts/admin/admin_rest_api.ts (2)
Line range hint
1726-1740: Consider refactoring URL parameter handling for consistency.The annotation state filtering logic is correctly implemented, but there are opportunities for improvement:
- Use URLSearchParams.append consistently for all parameters instead of mixing direct assignment and append
- Consider extracting the repeated annotation states logic into a helper function since it appears in multiple places
const params = new URLSearchParams({ - start: startDate.valueOf().toString(), - end: endDate.valueOf().toString(), }); +params.append("start", startDate.valueOf().toString()); +params.append("end", endDate.valueOf().toString()); if (annotationTypes != null) params.append("annotationTypes", annotationTypes); if (projectIds != null && projectIds.length > 0) params.append("projectIds", projectIds.join(","));
1753-1768: Maintain consistent parameter naming across related functions.While the implementation is correct, the parameter naming is inconsistent with the previous function:
annotationStatein getTimeTrackingForUserSummedPerAnnotationselectedStatein getTimeTrackingForUserSpansConsider using consistent parameter naming across these related functions.
export async function getTimeTrackingForUserSpans( userId: string, startDate: number, endDate: number, annotationTypes: "Explorational" | "Task" | "Task,Explorational", - selectedState: AnnotationStateFilterEnum, + annotationState: AnnotationStateFilterEnum, projectIds?: string[] | null, ): Promise<Array<APITimeTrackingSpan>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
app/models/user/time/TimeSpan.scala(4 hunks)frontend/javascripts/admin/admin_rest_api.ts(5 hunks)frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx(6 hunks)frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx(3 hunks)frontend/javascripts/admin/statistic/time_tracking_overview.tsx(9 hunks)frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts(3 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.md(18 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)
🔇 Additional comments (16)
frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts (2)
13-13: LGTM: Import addition aligns with new filtering functionality
The import of AnnotationStateFilterEnum is appropriate for implementing the annotation state filtering feature.
Line range hint 44-52: Consider making the user ID reference more maintainable
The test uses a hardcoded user ID (770b9f4d2a7c0e4d008da6ef) which could make the test brittle. Consider creating this user as part of the test setup or retrieving it dynamically.
Let's verify if this user ID is consistently created during database reset:
Consider refactoring to make the test more robust:
// In the before hook:
let userC: APIUser;
test.before("Reset database and initialize values", async () => {
// ... existing setup ...
userC = await api.createTestUser("UserC"); // or retrieve from a known test dataset
});
// In the test:
const timeTrackingForUser = await api.getTimeTrackingForUserSpans(
userC.id,
// ... rest of the parameters ...
);frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx (1)
2-5: LGTM: Type definitions are properly structured
The addition of AnnotationStateFilterEnum and its integration into TimeTrackingDetailViewProps is well-implemented and type-safe.
Also applies to: 19-19
frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (5)
16-28: LGTM! Well-structured enum and type definitions.
The new AnnotationStateFilterEnum and its integration into ProjectAndTypeDropdownProps are well-defined and align with the requirements for filtering by annotation state.
56-64: LGTM! Clean props integration.
The new annotation state props are properly integrated into the component's parameter destructuring.
105-108: LGTM! Robust selection handling.
The selection and deselection logic for annotation states is well-implemented and maintains consistency with the existing type filter handling.
Also applies to: 121-122
137-137: LGTM! Fixed dropdown width issue.
The addition of popupMatchSelectWidth={false} effectively addresses the PR objective of ensuring long project names are fully visible in the dropdown menu.
Line range hint 128-136: Verify project name search functionality.
The PR objectives mention enabling search by project name. While optionFilterProp="label" is set correctly to allow this, let's verify it works as expected.
✅ Verification successful
Project name search functionality is properly implemented
The implementation correctly supports project name search functionality through:
- The
optionFilterProp="label"property on the Select component - Project options are properly constructed with
label: project.name - The component follows the same pattern as other searchable dropdowns in the codebase
- The Select component is from Ant Design which provides built-in filtering capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any tests covering the project name search functionality
rg -l "project.*name.*search" --type=test
# Look for related components that might need updates
ast-grep --pattern 'Select $props={$$$} optionFilterProp="label"'
Length of output: 140
Script:
#!/bin/bash
# Search for test files containing project name search related tests with common test file extensions
rg -l "project.*name.*search" -g "*.test.*" -g "*.spec.*"
# Look for Select components with optionFilterProp in the codebase
rg -A 2 'optionFilterProp="label"'
# Check if there are any similar dropdown components for reference
fd -e tsx -e ts -e jsx -e js | xargs rg -l "Select.*mode=\"multiple\""
Length of output: 4862
Script:
#!/bin/bash
# Let's check the implementation details of the component and its filterOptions
rg -B 10 -A 5 'filterOptions.*=' frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx
# Check if there are any tests for this specific component
rg -l "project_and_annotation_type_dropdown" -g "*.test.*" -g "*.spec.*"
# Look for the label property construction in the component
rg -B 5 'label:.*name' frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx
Length of output: 1242
app/models/user/time/TimeSpan.scala (2)
Line range hint 169-176: LGTM! Type signature correctly updated.
The tuple type signature has been properly updated to include the annotation state field while maintaining type safety.
Line range hint 130-152: LGTM! Consider verifying query performance.
The SQL query changes correctly incorporate annotation state filtering. The joins and filtering conditions are properly structured.
Let's verify the query performance with different annotation state combinations:
✅ Verification successful
Based on the search results, I can see that there are no explicit indexes defined for the annotations_ table in the codebase. However, the query in question is already well-structured with appropriate joins and filtering conditions. The addition of the annotation state filter is unlikely to cause significant performance issues because:
- The query already filters on other columns like
ts._user,ts.created, anda.typ - The result set is bounded by the time range (
startandendparameters) - The annotation state filter is applied using an IN clause with a tuple, which is an efficient way to handle multiple values
Query performance should be acceptable without additional indexes
The query is well-optimized for its purpose:
- Uses proper JOIN conditions
- Has appropriate time-based filtering
- Applies filters early in the execution plan
- Uses efficient IN clause filtering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any existing indexes that can be utilized for the new annotation state filter
# Test: Look for existing indexes on the annotations_ table
rg -A 5 "CREATE.*INDEX.*annotations_"
# Test: Look for any performance-related comments or existing optimizations
rg -A 5 "performance|optimization.*annotations_"
Length of output: 82705
frontend/javascripts/admin/statistic/time_tracking_overview.tsx (4)
9-10: LGTM: Import changes are well-organized
The addition of AnnotationStateFilterEnum is properly grouped with related imports.
27-27: LGTM: CSV header updated to include annotation state
The addition of annotationState to the CSV header aligns with the PR objective to include annotation state in exports.
255-256: LGTM: UI component integration
The annotation state filtering is properly integrated into both the dropdown and detail view components.
Let's verify the component props are properly typed:
#!/bin/bash
# Check ProjectAndAnnotationTypeDropdown component props
ast-grep --pattern 'interface $_ {
$$$
selectedAnnotationState: $_
setSelectedAnnotationState: $_
$$$
}'Also applies to: 307-307
54-54: LGTM: State management and data fetching implementation
The state management for annotation filtering is well-implemented with proper dependency tracking for data refetching.
Let's verify the API integration:
Also applies to: 64-64, 71-71
✅ Verification successful
✓ API integration verified - parameter is properly handled
The verification shows that:
- The
getTimeEntriesAPI function inadmin_rest_api.tsexplicitly acceptsselectedState: AnnotationStateFilterEnumas a parameter - The function call in
time_tracking_overview.tsxcorrectly passes theselectedStateparameter - The implementation aligns with the state management setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the API endpoint accepts the new parameter
rg -A 2 "getTimeEntries.*selectedState" --type ts
Length of output: 114
Script:
#!/bin/bash
# Let's search for the API endpoint definition and usage
rg -A 5 "getTimeEntries" --type ts
# Also search for any interfaces or types related to the API parameters
rg -A 5 "interface.*TimeEntr(y|ies)" --type ts
# Check for any annotation state related API parameters
ast-grep --pattern 'function getTimeEntries($_) {
$$$
}'
Length of output: 1922
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.md (1)
14-14: 🛠️ Refactor suggestion
Verify test coverage for different annotation states.
While the addition of annotationState: 'Active' is consistent across all entries, the snapshots only cover the 'Active' state. Consider adding test cases for other annotation states (finished/archived) to ensure complete test coverage of the new filtering functionality.
Let's verify if there are any test cases for other annotation states:
Also applies to: 29-29, 44-44, 59-59, 74-74, 89-89, 104-104, 119-119, 134-134, 149-149, 164-164, 179-179, 194-194, 216-216, 231-231, 246-246, 261-261, 276-276
frontend/javascripts/admin/admin_rest_api.ts (1)
106-109: LGTM! Clean import changes.
The imports are well-structured, using type imports appropriately and importing the new enum needed for annotation state filtering.
| selectedState: AnnotationStateFilterEnum, | ||
| projectIds: string[], | ||
| ): Promise<Array<APITimeTrackingPerUser>> { | ||
| const params = new URLSearchParams({ | ||
| start: startMs.toString(), | ||
| end: endMs.toString(), | ||
| annotationTypes: selectedTypes, | ||
| }); | ||
| if (selectedState !== AnnotationStateFilterEnum.ALL) { | ||
| params.append("annotationStates", selectedState); | ||
| } else { | ||
| params.append("annotationStates", "Active,Finished"); | ||
| } |
There was a problem hiding this comment.
Remove duplicate annotationStates parameter.
There is a critical issue where the annotationStates parameter is being set twice:
- First in the conditional block (lines 1785-1789)
- Then again unconditionally (line 1793)
This could lead to unexpected behavior as the second append will add a duplicate parameter to the URL.
if (selectedState !== AnnotationStateFilterEnum.ALL) {
params.append("annotationStates", selectedState);
} else {
params.append("annotationStates", "Active,Finished");
}
// Omit empty parameters in request
if (projectIds.length > 0) params.append("projectIds", projectIds.join(","));
if (teamIds.length > 0) params.append("teamIds", teamIds.join(","));
- params.append("annotationStates", "Active,Finished");
return await Request.receiveJSON(`api/time/overview?${params}`);Also applies to: 1793-1793
There was a problem hiding this comment.
nice, thanks for saving me here 🙏
There was a problem hiding this comment.
@MichaelBuessemeyer, glad I could help!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts (2)
Line range hint
31-39: Add test cases for different annotation states.The test only covers the
ALLfilter case. Consider adding test cases for otherAnnotationStateFilterEnumvalues to ensure proper filtering behavior.Example additional test cases:
test("getTimeTrackingForUserSpans with active annotations", async (t) => { const timeTrackingForUser = await api.getTimeTrackingForUserSpans( activeUser.id, dayjs("20180101", "YYYYMMDD").valueOf(), dayjs("20181001", "YYYYMMDD").valueOf(), "Task", AnnotationStateFilterEnum.ACTIVE_ONLY ); t.true(timeTrackingForUser.length >= 0); t.snapshot(replaceVolatileValues(timeTrackingForUser)); }); test("getTimeTrackingForUserSpans with finished annotations", async (t) => { const timeTrackingForUser = await api.getTimeTrackingForUserSpans( activeUser.id, dayjs("20180101", "YYYYMMDD").valueOf(), dayjs("20181001", "YYYYMMDD").valueOf(), "Task", AnnotationStateFilterEnum.FINISHED_ONLY ); t.true(timeTrackingForUser.length >= 0); t.snapshot(replaceVolatileValues(timeTrackingForUser)); });
Line range hint
31-52: Document the new filter parameter.Add comments explaining the purpose of the annotation state filter parameter and its expected behavior.
test("getTimeTrackingForUserSpans", async (t) => { + // Retrieves time tracking spans filtered by annotation state (ALL includes both active and finished annotations) const timeTrackingForUser = await api.getTimeTrackingForUserSpans(frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx (2)
90-93: Consider adding error handling for the API callWhile the implementation is correct, consider adding error handling for cases where the API call fails.
return await getTimeTrackingForUserSummedPerAnnotation( props.userId, dayjs(props.dateRange[0]), dayjs(props.dateRange[1]), props.annotationType, props.annotationState, props.projectIds, -); +).catch(error => { + console.error('Failed to fetch time tracking data:', error); + // Consider showing a user-friendly error message + return []; +});
Line range hint
22-24: Address column layout issues mentioned in PR objectivesThe current fixed column spans might not handle long project names well, and the stats column alignment needs adjustment.
Consider these improvements:
- Make column spans responsive:
-const ANNOTATION_OR_TASK_NAME_SPAN = 16; -const STATISTICS_SPAN = 4; -const TIMESPAN_SPAN = 4; +const COLUMN_SPANS = { + name: { xs: 24, sm: 16, md: 16 }, + stats: { xs: 24, sm: 4, md: 4 }, + time: { xs: 24, sm: 4, md: 4 } +};
- Add left alignment to stats column:
<Col span={STATISTICS_SPAN}> <AnnotationStats stats={aggregateStatsForAllLayers(timeEntry.annotationLayerStats)} asInfoBlock={false} withMargin={false} + style={{ textAlign: 'left' }} /> </Col>frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (1)
16-28: Add JSDoc comments to document the enum states.Consider adding JSDoc comments to explain each annotation state for better code documentation and maintainability.
+/** + * Enum representing the possible states for annotation filtering + */ export enum AnnotationStateFilterEnum { + /** Show all annotations regardless of state */ ALL = "All", + /** Show only active annotations */ ACTIVE = "Active", + /** Show only finished or archived annotations */ FINISHED_OR_ARCHIVED = "Finished", }app/models/user/time/TimeSpan.scala (1)
Line range hint
169-190: Consider refactoring tuple to a case class for better maintainability.The current implementation uses a 13-element tuple which can be error-prone and hard to maintain. Consider refactoring this to use a case class for better type safety and maintainability.
Example refactoring:
case class TimeSpanResult( userId: String, userEmail: String, datasetOrganization: String, datasetName: String, annotationId: String, annotationState: String, taskId: Option[String], projectName: Option[String], taskTypeId: Option[String], taskTypeSummary: Option[String], timeSpanId: String, timeSpanCreated: Instant, timeSpanTimeMillis: Long ) private def formatTimespanTuple(result: TimeSpanResult) = Json.obj( "userId" -> result.userId, "userEmail" -> result.userEmail, // ... remaining fields )frontend/javascripts/admin/statistic/time_tracking_overview.tsx (1)
195-202: Consider improving function call formattingWhile the functionality is correct, the function call's formatting could be improved for better readability.
- downloadTimeSpans( - user.id, - startDate, - endDate, - selectedTypes, - selectedState, - selectedProjectIds, - ); + downloadTimeSpans( + user.id, + startDate, + endDate, + selectedTypes, + selectedState, + selectedProjectIds + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
app/models/user/time/TimeSpan.scala(4 hunks)frontend/javascripts/admin/admin_rest_api.ts(5 hunks)frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx(6 hunks)frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx(3 hunks)frontend/javascripts/admin/statistic/time_tracking_overview.tsx(9 hunks)frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts(3 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.md(18 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)
🔇 Additional comments (23)
frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts (2)
13-13: LGTM: Import statement is correctly placed and follows conventions.
Line range hint 44-52: Verify snapshot updates for annotation state.
The PR mentions including annotation state in the results. Please ensure that the test snapshots have been updated to reflect this change.
frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx (2)
2-5: LGTM! Clean import organization
The new AnnotationStateFilterEnum type is properly imported and grouped with related types.
18-21: Verify all component usages for the new required prop
The addition of the required annotationState prop is a breaking change. Let's ensure all component usages are updated.
✅ Verification successful
All usages of TimeTrackingDetailView are properly updated
The verification shows that the component is only used once in time_tracking_overview.tsx and it correctly provides all required props including the new annotationState prop. The usage is:
<TimeTrackingDetailView
userId={entry.user.id}
dateRange={[startDate.valueOf(), endDate.valueOf()]}
annotationType={selectedTypes}
annotationState={selectedState}
projectIds={selectedProjectIds}
/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TimeTrackingDetailView component usage
ast-grep --pattern 'TimeTrackingDetailView $$$'
# Also search for direct imports of the component
rg "import.*TimeTrackingDetailView" --type tsx
Length of output: 126
Script:
#!/bin/bash
# Search for TimeTrackingDetailView component usage with proper file extension
rg "TimeTrackingDetailView" -t ts -t tsx -t jsx -t js
# Also search for direct imports of the component
rg "import.*TimeTrackingDetailView" -t ts -t tsx -t jsx -t js
# Find the component file itself to understand its location
fd -e ts -e tsx -e js -e jsx "time_tracking_detail_view"
Length of output: 297
Script:
#!/bin/bash
# Search for TimeTrackingDetailView usage using basic grep
grep -r "TimeTrackingDetailView" .
# Search specifically in the admin/statistic directory
ls -la frontend/javascripts/admin/statistic/
cat frontend/javascripts/admin/statistic/index.tsx 2>/dev/null || true
Length of output: 1339
Script:
#!/bin/bash
# Let's check the usage in time_tracking_overview.tsx to see if the new prop is provided
cat frontend/javascripts/admin/statistic/time_tracking_overview.tsx
Length of output: 11125
frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (5)
49-55: LGTM! Clear and consistent filter options.
The new filter options are well-structured and consistent with the existing type filters.
62-63: LGTM! Well-typed props following existing patterns.
The new props maintain consistency with the existing prop naming and typing patterns.
81-88: LGTM! Clean state management implementation.
The state management logic correctly handles the annotation state filtering while maintaining the existing functionality.
97-101: LGTM! Logical filter ordering.
The filters are arranged in a sensible order: type filters, state filters, and then project filters.
105-108: LGTM! Robust selection handling.
The selection and deselection logic properly handles the annotation state changes with appropriate type safety.
Also applies to: 121-122
app/models/user/time/TimeSpan.scala (1)
Line range hint 130-152: Consider adding indexes for performance optimization.
The SQL query joins multiple tables and filters on several conditions. While the query structure is correct, consider adding indexes on frequently filtered columns to improve performance, especially:
timespans_.createdannotations_.typannotations_.state
Let's verify the existing indexes:
frontend/javascripts/admin/statistic/time_tracking_overview.tsx (6)
9-11: LGTM: Import changes align with new annotation state filtering feature
The addition of AnnotationStateFilterEnum is consistent with the PR's objective to support annotation state filtering.
26-28: LGTM: CSV header updated to include annotation state
The addition of annotationState to TIMETRACKING_CSV_HEADER_SPANS ensures that the exported data includes the new filtering capability.
54-54: LGTM: State management properly implemented for annotation state
The implementation follows React best practices:
- State initialization with appropriate default value
- Proper inclusion in the dependency array of
useFetch
Also applies to: 71-71
Line range hint 92-106: LGTM: CSV export properly includes annotation state
The CSV row transformation correctly includes the annotationState field, maintaining consistency with the header structure.
255-256: LGTM: Component props properly updated for annotation state
The new props for annotation state management are consistently implemented across components:
ProjectAndAnnotationTypeDropdownreceives both state and setter propsTimeTrackingDetailViewreceives the state prop
Also applies to: 307-307
64-64: Verify API endpoint compatibility with new parameter
The selectedState parameter has been added to API calls. Please ensure that the corresponding backend endpoints have been updated to handle this parameter.
Also applies to: 88-88
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/timetracking.e2e.js.md (2)
216-216: LGTM! Consistent implementation across user contexts.
The annotationState property has been correctly added to the time tracking entries for different users, maintaining consistency in the data structure.
Also applies to: 231-231, 246-246, 261-261, 276-276
14-14: Consider adding test cases for different annotation states.
While the snapshot correctly includes the new annotationState property, it only covers the 'Active' state. To ensure comprehensive test coverage, consider adding test cases for 'Finished' and 'Archived' states as well.
Let's check if there are other test cases covering different annotation states:
Also applies to: 29-29, 44-44, 59-59, 74-74, 89-89, 104-104, 119-119, 134-134, 149-149, 164-164, 179-179, 194-194
frontend/javascripts/types/api_flow_types.ts (1)
586-586: LGTM! Verify handling of the new required property.
The addition of annotationState aligns with the PR objectives to include annotation state in exports. Since this is a required property, ensure all existing code that creates or handles APITimeTrackingSpan objects is updated accordingly.
Let's verify the impact:
✅ Verification successful
Property addition appears safe - type only used in API response
Based on the code analysis, the APITimeTrackingSpan type is primarily used as a return type for the API response in admin_rest_api.ts. Since the data is coming from the backend and not being constructed in the frontend, adding the required annotationState property is safe as long as the backend provides this field in its response.
- The type is only imported and used in
admin_rest_api.ts - No object creation or type assertions were found in the frontend code
- The type is used as a return type for
getTimeTrackingSpansAPI method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usages of APITimeTrackingSpan to ensure they're updated
# to handle the new annotationState property
# Search for APITimeTrackingSpan type usage
echo "Searching for APITimeTrackingSpan usage..."
rg -l "APITimeTrackingSpan" --type ts
# Search for object literals that might need the new property
echo "Searching for potential object literals that need updating..."
rg "(?s)\{[^}]*timeSpanTimeMillis[^}]*\}" --type ts
Length of output: 409
Script:
#!/bin/bash
# Let's check the actual usage context in admin_rest_api.ts
echo "Checking APITimeTrackingSpan usage in admin_rest_api.ts:"
rg -A 5 -B 5 "APITimeTrackingSpan" frontend/javascripts/admin/admin_rest_api.ts
# Let's also check if there are any type casts or type assertions to APITimeTrackingSpan
echo -e "\nChecking for type assertions/casts:"
ast-grep --pattern 'as APITimeTrackingSpan'
# Let's check for object creation patterns that might be affected
echo -e "\nChecking for object creation patterns:"
rg "(?s)export const createTimeTrackingSpan|(?s)new APITimeTrackingSpan" --type ts
Length of output: 1132
Script:
#!/bin/bash
# Let's check the API response handling since it seems the type is used in API return type
echo "Checking for API response handling and mapping:"
rg -A 10 "getTimeTrackingSpans" --type ts
# Let's also check the implementation of the API endpoint
echo -e "\nChecking for backend implementation:"
rg -A 10 "timeTrackingSpans" --type ts --type js
# Let's look for any test files that might show object creation
echo -e "\nChecking test files for object creation:"
fd -e test.ts -e test.js -x rg -l "TimeTrackingSpan"
Length of output: 434
frontend/javascripts/admin/admin_rest_api.ts (4)
106-109: LGTM: Clean import changes
The imports are properly organized, with a clear separation between the enum import and the type import.
Line range hint 1726-1740: LGTM: Well-implemented annotation state filtering
The function getTimeTrackingForUserSummedPerAnnotation has been properly updated to handle annotation state filtering with clear conditional logic.
1753-1768: LGTM: Consistent implementation of state filtering
The function getTimeTrackingForUserSpans follows the same pattern as the previous function, maintaining consistency in the codebase.
1777-1789:
Fix duplicate annotationStates parameter
The annotationStates parameter is being set twice:
- First in the conditional block (lines 1785-1789)
- Then again unconditionally (line 1793)
This could lead to unexpected behavior as the second append will add a duplicate parameter to the URL.
Apply this diff to fix the issue:
if (selectedState !== AnnotationStateFilterEnum.ALL) {
params.append("annotationStates", selectedState);
} else {
params.append("annotationStates", "Active,Finished");
}
// Omit empty parameters in request
if (projectIds.length > 0) params.append("projectIds", projectIds.join(","));
if (teamIds.length > 0) params.append("teamIds", teamIds.join(","));
- params.append("annotationStates", "Active,Finished");
return await Request.receiveJSON(`api/time/overview?${params}`);Also applies to: 1793-1793
| options={filterOptions} | ||
| optionFilterProp="label" | ||
| value={selectedFilters} | ||
| popupMatchSelectWidth={false} |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate a final response. The code shows that the component fetches all projects at once without pagination, and the search results reveal that virtual scrolling is commonly used throughout the codebase for performance optimization, particularly in components with large datasets.
Consider keeping virtual scrolling enabled for better performance
Setting popupMatchSelectWidth={false} disables virtual scrolling in the antd Select component. While this allows showing full project names, it could impact performance as the component loads all projects at once. The codebase shows extensive use of virtualization for performance optimization in similar scenarios.
Alternative solutions that maintain virtual scrolling:
- Use CSS ellipsis with tooltip:
text-overflow: ellipsisin combination with antd'stitleprop - Set a fixed
dropdownStyle={{ width: 'max-content', maxWidth: '500px' }}that's wider than the input but still enables virtual scrolling - Use antd's built-in
ellipsisprop withtooltipfor long text
🔗 Analysis chain
Consider performance impact of disabled virtual scrolling.
Setting popupMatchSelectWidth={false} addresses the requirement to show long project names but disables virtual scrolling. This could impact performance for organizations with many projects.
Consider these alternatives:
- Use CSS to handle long text (e.g., text-overflow with tooltip) while keeping virtual scrolling
- Set a reasonable fixed width for the popup that's wider than the input but still enables virtual scrolling
Let's check how many projects might be affected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for project-related components to understand the scale
rg -l 'projects.*map' --type ts --type tsx
Length of output: 72
Script:
#!/bin/bash
# Search for project-related components and mapping patterns in TypeScript files
rg -l 'projects.*map' --type typescript
# Look for Select/Dropdown components with project lists
rg -l 'Select.*projects' --type typescript
# Search for array patterns that might indicate project lists
rg -A 5 'projects: \[' --type typescript
# Check the specific component's file for project handling
cat "frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx"
Length of output: 5410
Script:
#!/bin/bash
# Let's try with proper file extensions for TypeScript/React files
rg -l '\.tsx?$' | xargs rg -l 'projects.*map'
# Look for project data fetching to understand the scale
rg -A 5 'getProjects\(' --type-add 'ts:*.{ts,tsx}'
# Check for any pagination or virtualization related code
rg -A 3 'virtualiz|pagination' --type-add 'ts:*.{ts,tsx}'
# Look for Select component usage patterns
rg -A 3 '<Select.*mode="multiple"' --type-add 'ts:*.{ts,tsx}'
Length of output: 24571
|
@daniel-wer Do you have time to review this? |
There was a problem hiding this comment.
Well done, I tried to test this to the best of my knowledge, but maybe @fm3 wants to do a quick test as well? :)
| options={filterOptions} | ||
| optionFilterProp="label" | ||
| value={selectedFilters} | ||
| popupMatchSelectWidth={false} |
There was a problem hiding this comment.
I'm not aware of any orga making such extensive use of projects that this would be an issue. An alternative could be to set a reasonable width (somewhat larger than before) and then show the full project name on hover. No real preference on my side.
Yeah, I also thought about this and implemented it now this way :). There is already a title for each option in the filter select dropdown. I only adjusted the width of the dropdown to be at 800 px. If you think that's too much just tell me. This is how it looks with a long name (the is also a title available for each dropdown menu item, but its not included in the screenshot): |
…nts and increase constant width of timetracking filter dropdown
frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx
Outdated
Show resolved
Hide resolved
…pe_dropdown.tsx Co-authored-by: Daniel <daniel.werner@scalableminds.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (2)
39-45: Consider using enum values for labelsThe constant is well-structured, but consider deriving the labels from the enum values to maintain consistency and ease maintenance.
const ANNOTATION_STATE_FILTERS: NestedSelectOptions = { label: "Filter by state", options: [ - { label: "Active", value: AnnotationStateFilterEnum.ACTIVE }, - { label: "Finished / Archived", value: AnnotationStateFilterEnum.FINISHED_OR_ARCHIVED }, + { + label: AnnotationStateFilterEnum.ACTIVE.replace(/_/g, ' ').toLowerCase(), + value: AnnotationStateFilterEnum.ACTIVE + }, + { + label: AnnotationStateFilterEnum.FINISHED_OR_ARCHIVED.replace(/_/g, ' ').toLowerCase(), + value: AnnotationStateFilterEnum.FINISHED_OR_ARCHIVED + }, ], };
71-78: Consider extracting the filter logic for better readabilityThe state management logic is correct but could be more readable by extracting the filter computation.
- const selectedKeys = - selectedAnnotationState !== AnnotationStateFilterEnum.ALL ? [selectedAnnotationState] : []; - if (selectedProjectIds.length > 0) { - setSelectedFilters([...selectedProjectIds, ...selectedKeys]); - } else { - setSelectedFilters([selectedAnnotationType, ...selectedKeys]); - } + const getAnnotationStateFilter = () => + selectedAnnotationState !== AnnotationStateFilterEnum.ALL ? [selectedAnnotationState] : []; + + const getSelectedFilters = () => { + const stateFilters = getAnnotationStateFilter(); + return selectedProjectIds.length > 0 + ? [...selectedProjectIds, ...stateFilters] + : [selectedAnnotationType, ...stateFilters]; + }; + + setSelectedFilters(getSelectedFilters());frontend/javascripts/oxalis/constants.ts (2)
490-494: LGTM! Consider adding JSDoc comments.The enum is well-structured and follows consistent naming conventions. Consider adding JSDoc comments to document the purpose and usage of each enum value.
+/** + * Enum for filtering annotations by their type. + * @enum {string} + */ export enum AnnotationTypeFilterEnum { + /** Only show explorational annotations */ ONLY_ANNOTATIONS_KEY = "Explorational", + /** Only show task annotations */ ONLY_TASKS_KEY = "Task", + /** Show both task and explorational annotations */ TASKS_AND_ANNOTATIONS_KEY = "Task,Explorational", }
496-500: LGTM! Consider adding JSDoc and a type guard.The enum aligns well with the PR objectives for filtering by annotation state. Consider adding JSDoc comments and a type guard function to ensure type safety when handling string values.
+/** + * Enum for filtering annotations by their state. + * @enum {string} + */ export enum AnnotationStateFilterEnum { + /** Show all annotations regardless of state */ ALL = "All", + /** Show only active annotations */ ACTIVE = "Active", + /** Show only finished or archived annotations */ FINISHED_OR_ARCHIVED = "Finished", } +/** + * Type guard to check if a string is a valid AnnotationStateFilterEnum value + */ +export function isAnnotationStateFilter(value: string): value is AnnotationStateFilterEnum { + return Object.values(AnnotationStateFilterEnum).includes(value as AnnotationStateFilterEnum); +}frontend/javascripts/admin/admin_rest_api.ts (3)
Line range hint
1722-1736: Consider extracting the annotation state handling logic.The annotation state filtering logic is duplicated across multiple functions. Consider extracting it into a helper function for better maintainability.
+ function appendAnnotationStateParam(params: URLSearchParams, state: AnnotationStateFilterEnum) { + if (state !== AnnotationStateFilterEnum.ALL) { + params.append("annotationStates", state); + } else { + params.append("annotationStates", "Active,Finished"); + } + } export async function getTimeTrackingForUserSummedPerAnnotation( userId: string, startDate: dayjs.Dayjs, endDate: dayjs.Dayjs, annotationTypes: "Explorational" | "Task" | "Task,Explorational", annotationState: AnnotationStateFilterEnum, projectIds?: string[] | null, ): Promise<Array<APITimeTrackingPerAnnotation>> { const params = new URLSearchParams({ start: startDate.valueOf().toString(), end: endDate.valueOf().toString(), }); if (annotationTypes != null) params.append("annotationTypes", annotationTypes); if (projectIds != null && projectIds.length > 0) params.append("projectIds", projectIds.join(",")); - if (annotationState !== AnnotationStateFilterEnum.ALL) { - params.append("annotationStates", annotationState); - } else { - params.append("annotationStates", "Active,Finished"); - } + appendAnnotationStateParam(params, annotationState);
1749-1764: Apply the same helper function here.The annotation state handling logic is duplicated here as well.
export async function getTimeTrackingForUserSpans( userId: string, startDate: number, endDate: number, annotationTypes: "Explorational" | "Task" | "Task,Explorational", selectedState: AnnotationStateFilterEnum, projectIds?: string[] | null, ): Promise<Array<APITimeTrackingSpan>> { const params = new URLSearchParams({ start: startDate.toString(), end: endDate.toString(), }); if (annotationTypes != null) params.append("annotationTypes", annotationTypes); if (projectIds != null && projectIds.length > 0) { params.append("projectIds", projectIds.join(",")); } - if (selectedState !== AnnotationStateFilterEnum.ALL) { - params.append("annotationStates", selectedState); - } else { - params.append("annotationStates", "Active,Finished"); - } + appendAnnotationStateParam(params, selectedState);
1773-1785: Apply the same helper function here as well.The annotation state handling logic is duplicated here too.
export async function getTimeEntries( startMs: number, endMs: number, teamIds: string[], selectedTypes: AnnotationTypeFilterEnum, selectedState: AnnotationStateFilterEnum, projectIds: string[], ): Promise<Array<APITimeTrackingPerUser>> { const params = new URLSearchParams({ start: startMs.toString(), end: endMs.toString(), annotationTypes: selectedTypes, }); - if (selectedState !== AnnotationStateFilterEnum.ALL) { - params.append("annotationStates", selectedState); - } else { - params.append("annotationStates", "Active,Finished"); - } + appendAnnotationStateParam(params, selectedState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
frontend/javascripts/admin/admin_rest_api.ts(5 hunks)frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx(6 hunks)frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx(2 hunks)frontend/javascripts/admin/statistic/time_tracking_overview.tsx(9 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx
- frontend/javascripts/admin/statistic/time_tracking_overview.tsx
- frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts
🔇 Additional comments (6)
frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx (5)
9-9: LGTM: Type definitions are well-structured
The new type definitions for annotation state filtering are properly typed and integrated into the component's props interface.
Also applies to: 16-17
52-53: LGTM: Props are properly integrated
The new annotation state props are correctly destructured and follow the component's existing patterns.
87-87: LGTM: Filter options are properly updated
The annotation state filters are correctly integrated into the options array.
95-98: LGTM: Selection handling is robust
The selection and deselection logic for annotation states is well-implemented with proper type checking and state updates.
Also applies to: 111-112
127-127: LGTM: Dropdown width is properly adjusted
The fixed width of 400px is a good compromise that addresses the previous feedback about long project names while maintaining virtual scrolling functionality.
frontend/javascripts/admin/admin_rest_api.ts (1)
70-71: LGTM!
The import of AnnotationStateFilterEnum is correctly added to support the new annotation state filtering functionality.
3 participants
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
AnnotationStateFilterEnumfor filtering time tracking data based on annotation states (ALL, ACTIVE, FINISHED_OR_ARCHIVED).Improvements
Bug Fixes