[Feat]: Report Activity Data productivity #3613
Conversation
|
Warning Rate limit exceeded@Innocent-Akim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request refactors various dashboard components and hooks to improve data handling and type safety. The changes remove redundant state management in favor of dedicated handler functions, update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DH as DashboardHeader
participant AU as AppUrls Component
participant UR as useReportActivity Hook
participant PT as ProductivityTable
U->>DH: Selects new GroupBy option
DH->>AU: Triggers onGroupByChange (handleGroupByChange)
AU->>UR: Calls handleGroupByChange with new groupBy value
UR->>UR: Fetches updated activity report (based on APPS-URLS)
UR-->>AU: Returns updated report data and loading state
AU->>PT: Passes updated activityReport and loading flags
Possibly related PRs
Suggested labels
Suggested reviewers
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/web/app/[locale]/dashboard/app-url/components/ProductivityTable.tsx (3)
78-91: Consider iterating over all projects.
Right now, the code only accessesemployeeData.projects[0]?.activity. If employees can have multiple projects, the other projects' activities are effectively ignored. Verify whether this is intentional or refactor to iterate through all projects as needed.
142-142: Avoid unnecessary type conversion.
Ifactivity.durationoriginates as a number, there is no need to convert it with.toString(). Instead, pass the numeric value directly intoformatDuration()to reduce potential type-parsing pitfalls.
166-173: Handle invalid or negative durations.
Currently,formatDurationassumes valid non-negative numeric input. Consider adding validation or defaulting to"00:00:00"if parsing fails to make the function more robust.apps/web/app/hooks/features/useReportActivity.ts (2)
240-279: Improve error handling in update handlers.The use of Promise.all for concurrent API calls is good for performance, but error handling could be more specific to help with debugging.
Apply this diff to improve error handling:
- ]).catch(console.error); + ]).catch((error) => { + console.error('Failed to update report data:', { + error, + filters: newFilters, + type: types + }); + });
293-316: Consider memoizing returned object for better performance.The returned object contains multiple values that could benefit from memoization to prevent unnecessary re-renders in consuming components.
Apply this diff to memoize the returned object:
+ const returnValue = useMemo( + () => ({ + // Loading states + loadingTimeLogReportDailyChart, + loadingTimeLogReportDaily, + loadingTimesheetStatisticsCounts, + loadingActivityReport, + + // Data states + rapportChartActivity, + rapportDailyActivity, + statisticsCounts, + activityReport, + + // Update handlers + updateDateRange, + updateFilters, + handleGroupByChange, + + // Other states + currentFilters, + setStatisticsCounts, + isManage + }), + [ + loadingTimeLogReportDailyChart, + loadingTimeLogReportDaily, + loadingTimesheetStatisticsCounts, + loadingActivityReport, + rapportChartActivity, + rapportDailyActivity, + statisticsCounts, + activityReport, + updateDateRange, + updateFilters, + handleGroupByChange, + currentFilters, + setStatisticsCounts, + isManage + ] + ); + - return { - // Loading states - loadingTimeLogReportDailyChart, - loadingTimeLogReportDaily, - loadingTimesheetStatisticsCounts, - loadingActivityReport, - - // Data states - rapportChartActivity, - rapportDailyActivity, - statisticsCounts, - activityReport, - - // Update handlers - updateDateRange, - updateFilters, - handleGroupByChange, - - // Other states - currentFilters, - setStatisticsCounts, - isManage - }; + return returnValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/[locale]/dashboard/app-url/[teamId]/page.tsx(6 hunks)apps/web/app/[locale]/dashboard/app-url/components/GroupBySelect.tsx(1 hunks)apps/web/app/[locale]/dashboard/app-url/components/ProductivityTable.tsx(3 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/dashboard-header.tsx(1 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx(4 hunks)apps/web/app/hooks/features/useReportActivity.ts(10 hunks)apps/web/lib/features/unverified-email.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/lib/features/unverified-email.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (6)
apps/web/app/[locale]/dashboard/app-url/components/ProductivityTable.tsx (1)
15-18: Verify correct type usage.
The propdatais defined asIActivityReport[], yet it’s cast toIActivityReportGroupByDate[]. Ensure that these types are truly compatible or consider changing the prop type to match this usage more explicitly to avoid confusion or runtime errors.apps/web/app/[locale]/dashboard/app-url/components/GroupBySelect.tsx (1)
3-7: Ensure values match the enum.
TheonGroupByChangeprop expects aGroupByType, yet string literals like"date"and"project"are passed in. Confirm that these strings align exactly with the union or enum definition ofGroupByTypeto avoid runtime or type-checking issues.apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/dashboard-header.tsx (1)
10-10: Aligned callback signature.
Switching theonGroupByChangetype fromstringtoGroupByTypeprovides better type safety and consistency withGroupBySelect. This ensures only valid grouping values are passed around.Also applies to: 18-18
apps/web/app/hooks/features/useReportActivity.ts (1)
21-34: LGTM! Well-structured type definitions.The new
GroupByTypetype and updatedUseReportActivityPropsinterface provide good type safety while maintaining backward compatibility.Also applies to: 66-70
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (1)
28-38: LGTM! Clean and well-structured changes.The refactoring improves code organization while maintaining functionality. Good use of the new useReportActivity hook with proper typing.
Also applies to: 50-51, 62-63
apps/web/app/[locale]/dashboard/app-url/[teamId]/page.tsx (1)
131-131: LGTM! Good use of loading state.Properly handling loading state in the ProductivityTable component improves user experience.
Innocent-Akim
added
bug
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/web/app/hooks/features/useReportActivity.ts (1)
21-34: 🛠️ Refactor suggestionImprove type safety by using
GroupByTypeforgroupByproperty.The
groupByproperty inUseReportActivityPropsshould be constrained toGroupByTypeinstead ofstringto ensure type safety.Apply this diff:
- groupBy?: string; + groupBy?: GroupByType;Also, consider removing the redundant
GroupByOptionsinterface and directly use{ groupBy: GroupByType }where needed.Also applies to: 66-70
🧹 Nitpick comments (3)
apps/web/app/hooks/features/useReportActivity.ts (3)
72-72: Improve type safety by using const assertion fortypesparameter.Use a const assertion to make the
typesparameter more type-safe and self-documenting.Apply this diff:
-export function useReportActivity({ types }: { types?: 'TEAM-DASHBOARD' | 'APPS-URLS' }) +const REPORT_TYPES = ['TEAM-DASHBOARD', 'APPS-URLS'] as const; +type ReportType = typeof REPORT_TYPES[number]; +export function useReportActivity({ types }: { types?: ReportType })
143-192: Improve error handling with specific error types.While the error handling is good, it could be improved by using more specific error types and error messages.
Apply this diff:
- } catch (err) { - console.error('Failed to fetch report:', err); + } catch (err: unknown) { + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + console.error(`Failed to fetch report with query ${queryFn.name}:`, errorMessage);
257-267: Improve error handling inhandleGroupByChange.The error handling in
handleGroupByChangecould be more robust and provide better error feedback.Apply this diff:
- } catch (error) { - console.error('Failed to update activity grouping:', error); + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + console.error('Failed to update activity grouping:', errorMessage); + // Consider adding error state management or error callback + throw new Error(`Failed to update activity grouping: ${errorMessage}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/hooks/features/useReportActivity.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (2)
apps/web/app/hooks/features/useReportActivity.ts (2)
96-140: LGTM! Props merging logic is well-implemented.The memoized props merging function correctly handles all edge cases and has proper dependency tracking.
281-291: LGTM! Initial data fetch is well-implemented.The initial data fetch correctly handles conditional fetching and has proper dependency tracking.
- Simplify mergedProps and setData validation - Remove redundant setData([]) initialization - Add consistent mergedProps check in fetchStatisticsCounts - Optimize dependency array in useMemo - Improve type safety with null coalescing for tenantId
- Simplify mergedProps and setData validation - Remove redundant setData([]) initialization - Add consistent mergedProps check in fetchStatisticsCounts - Optimize dependency array in useMemo - Improve type safety with null coalescing for tenantId
Description
Please include a summary of the changes and the related issues.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of the previous status
Current screenshots
Please add here videos or images of the current (new) status
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes