Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat]: Timesheet Statistics api types #3557

Merged
merged 6 commits into from
Feb 2, 2025

Conversation

Innocent-Akim
Copy link
Contributor

@Innocent-Akim Innocent-Akim commented Jan 30, 2025

Description

Please include a summary of the changes and the related issues.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

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

    • Added dynamic timesheet statistics tracking for team dashboards.
    • Introduced new statistics display with loading indicators.
    • Enhanced time and activity tracking functionality.
  • Bug Fixes

    • Improved data validation for log types.
    • Updated error handling for statistics retrieval.
  • Performance

    • Optimized statistics fetching and rendering.
    • Added conditional rendering for loading states.
  • Improvements

    • Implemented more flexible statistics calculation.
    • Added formatting for time and percentage displays.
    • Simplified breadcrumb logic in the dashboard.

Copy link
Contributor

coderabbitai bot commented Jan 30, 2025

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

apps/web/components/pages/task/ParentTask.tsx

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

  • 1 others

Walkthrough

This pull request introduces a comprehensive enhancement to the timesheet statistics functionality across multiple files in the web application. The changes focus on dynamically rendering team statistics, adding new interfaces for timesheet data, and implementing a more flexible approach to fetching and displaying statistics counts. The modifications span components, hooks, services, and interfaces, creating a more robust and responsive system for tracking and presenting team activity and time logs.

Changes

File Change Summary
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx Replaced hardcoded stats with dynamic props, added formatting functions, introduced conditional rendering for loading and progress states
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx Updated imports, added new props to TeamStatsGrid, simplified breadcrumb logic
apps/web/app/api/timesheet/statistics/counts/route.ts Replaced daily chart request with statistics counts request, added log type validation
apps/web/app/hooks/features/useReportActivity.ts Enhanced hook to fetch and manage timesheet statistics counts, updated interfaces and props
apps/web/app/interfaces/timer/ITimerLog.ts Added new interfaces ITimesheetStatisticsCounts and ITimesheetStatisticsData
apps/web/app/services/client/api/timer/timer-log.ts Added new formatting and statistics fetching functions
apps/web/app/services/server/requests/timesheet.ts Introduced new request function for fetching timesheet statistics counts
apps/web/app/stores/time-logs.ts Added new atom for timesheet statistics counts state
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-table.tsx Removed vertical spacing class from outermost div in TeamStatsTable component
apps/web/components/pages/task/ParentTask.tsx Modified class names for div elements in CreateParentTask component without changing functionality

Possibly related PRs

  • [Feat]: Timesheet-Duration-Display #3352: The changes in the main PR are related to the introduction of new functions and modifications to the TeamStatsGrid component that utilize the secondsToTime helper function, which is also referenced in the retrieved PR's new DisplayTimeForTimesheet function.
  • [Feat]: Team Dashboard UI Enhancements #3530: The changes in the main PR are related to the modifications in the TeamStatsGrid component, which is also referenced in the retrieved PR as a new component with the same name, indicating a direct connection in functionality.
  • [Feat]: Team Dashboard View Team Activity #3548: Updates in this PR involve the TeamDashboard passing similar props to the TeamStatsTable, indicating a direct connection in data management across components.

Suggested labels

Improvement

Poem

🐰 Hop, hop, through stats so bright,
Timesheet data dancing with delight!
Metrics flow, no longer static,
Our dashboard now feels automatic!
Code rabbits cheer, the grid's alive! 📊✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Innocent-Akim Innocent-Akim changed the title [Feat]: Timesheet Dtatistics api types [Feat]: Timesheet Statistics api types Jan 30, 2025
@Innocent-Akim Innocent-Akim self-assigned this Jan 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (17)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (5)

9-12: Guard against negative values or values above 100
Although returning zero for undefined values is fine, consider adding validation for negative or unusually large percentages. Otherwise, you risk displaying misleading progress bars.

-function formatPercentage(value: number | undefined): number {
-  if (!value) return 0;
-  return Math.round(value);
-}
+function formatPercentage(value?: number): number {
+  const sanitized = Math.max(0, Math.min(value ?? 0, 100));
+  return Math.round(sanitized);
+}

14-16: Ensure consistent time format in different locales
formatTime always returns HH:MM:SS in 24-hour format. This is typically acceptable, but if you plan to localize or change formats (e.g., 12-hour clock, textual formatting), consider a more robust solution or a library for date/time handling.


18-26: Consider making progress optional in StatItem
In some use-cases, progress might be undefined if you only need to show the numeric/time value. Making progress optional helps avoid confusion in code usage.

 interface StatItem {
   title: string;
   value: string;
   type: 'number' | 'time';
   color?: string;
-  progress?: number;
+  progress?: number | undefined;
   progressColor?: string;
   showProgress: boolean;
 }

39-82: Improve reusability of stats
While using useMemo for creating a stats array is good for performance, consider extracting it into a helper function, especially if you plan to share or recompose stats in other components.


92-95: Provide alternative loader text for accessibility
The Loader2 spinner has no accompanying text or label. For accessibility, consider adding an aria-label or descriptive text to help screen readers.

 {loadingTimesheetStatisticsCounts ? (
-  <Loader2 className="w-6 h-6 text-gray-500 animate-spin" />
+  <Loader2
+    className="w-6 h-6 text-gray-500 animate-spin"
+    aria-label="Loading timesheet statistics..."
+  />
 ) : (
apps/web/app/hooks/features/useReportActivity.ts (5)

20-28: Separate defaults for clarity
defaultProps merges multiple concerns (date range, activityLevel, etc.). For better maintainability, consider separating default date/time values from default chart/filters to reduce confusion.


37-40: Audit concurrency
You have multiple network queries running in parallel. Ensure that error handling remains consistent for each. If additional concurrency control is needed (e.g., canceling older requests when new ones come in), consider a dedicated approach with React Query or a similar library.


Line range hint 73-102: Handle non-Array responses
The fetchReport method sets setData([]) if something fails. If the response.data is not an array (for instance, if the API changes or unparseable data is returned), it still tries to cast it. Consider more robust type checks.

- if (setData && Array.isArray(response.data)) {
-   setData(response.data as T[]);
- }
+ if (setData) {
+   setData(Array.isArray(response.data) ? response.data as T[] : []);
+ }

116-134: Ensure proper logType usage
Currently, fetchStatisticsCounts hardcodes [TimeLogType.TRACKED] for logType. If you expand the needs for manual or idle types in the future, remember to accept those as well.

Would you like me to open a new issue to adapt fetchStatisticsCounts to allow other time log types too?


170-177: Encapsulate state updates
You return setStatisticsCounts directly. If external components mutate it for reasons outside the intended design, you may see inconsistent data. Provide a setter callback or use a context-based approach to handle updates more predictably.

apps/web/app/stores/time-logs.ts (1)

27-27: Atom naming consistency
timesheetStatisticsCountsState is clear. Ensure that naming remains consistent with other atoms (e.g., timeLogsRapportChartState) so future developers quickly grasp their relationships.

apps/web/app/api/timesheet/statistics/counts/route.ts (1)

68-72: Enhance error logging for better debugging.

The current error logging only includes a generic message. Adding more details would help with debugging.

-console.error('Error fetching timesheet statistics counts:', error);
+const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+console.error('Error fetching timesheet statistics counts:', {
+    error: errorMessage,
+    params: { startDate, endDate, organizationId, tenantId, logTypes }
+});
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (1)

22-22: Fix formatting for better readability.

There's missing spacing after commas in the destructured properties.

-const { rapportChartActivity, updateDateRange, updateFilters, loadingTimeLogReportDailyChart, rapportDailyActivity, loadingTimeLogReportDaily, statisticsCounts,loadingTimesheetStatisticsCounts} = useReportActivity();
+const {
+    rapportChartActivity,
+    updateDateRange,
+    updateFilters,
+    loadingTimeLogReportDailyChart,
+    rapportDailyActivity,
+    loadingTimeLogReportDaily,
+    statisticsCounts,
+    loadingTimesheetStatisticsCounts
+} = useReportActivity();
apps/web/app/interfaces/timer/ITimerLog.ts (1)

268-273: Enhance type safety for statistics data.

Consider making the combined statistics required and adding validation for numeric values.

 export interface ITimesheetStatisticsCounts {
     tracked: ITimesheetStatisticsData | null;
     idle: ITimesheetStatisticsData | null;
     manual: ITimesheetStatisticsData | null;
-    combined: ITimesheetStatisticsData;
+    combined: Required<ITimesheetStatisticsData>;
 }

 export interface ITimesheetStatisticsData {
-    employeesCount: number;
-    projectsCount: number;
-    todayActivities: number;
-    todayDuration: number;
-    weekActivities: number;
-    weekDuration: number;
+    employeesCount: NonNegativeInteger;
+    projectsCount: NonNegativeInteger;
+    todayActivities: NonNegativeInteger;
+    todayDuration: NonNegativeInteger;
+    weekActivities: NonNegativeInteger;
+    weekDuration: NonNegativeInteger;
 }

+type NonNegativeInteger = number & { __brand: 'NonNegativeInteger' };
apps/web/app/services/server/requests/timesheet.ts (1)

160-160: Remove commented-out code.

The commented-out LogType type is redundant as we're now using TimeLogType from the interfaces.

-// export type LogType = 'TRACKED' | 'MANUAL' | 'IDLE';
apps/web/app/services/client/api/timer/timer-log.ts (2)

280-293: Add input validation for seconds parameter.

While the implementation is correct, consider adding input validation to handle negative numbers and non-numeric values.

 export function formatDuration(seconds: number): string {
+    if (typeof seconds !== 'number' || isNaN(seconds) || seconds < 0) {
+        throw new Error('Duration must be a non-negative number');
+    }
     const hours = Math.floor(seconds / 3600);
     const minutes = Math.floor((seconds % 3600) / 60);
     const remainingSeconds = seconds % 60;

295-300: Add input validation and handle edge cases for activity percentage.

Consider adding input validation and clamping the value to ensure it stays within the valid percentage range (0-100).

 export function formatActivity(activity: number): string {
+    if (typeof activity !== 'number' || isNaN(activity)) {
+        throw new Error('Activity must be a number');
+    }
+    const clampedActivity = Math.max(0, Math.min(100, activity));
-    return `${activity.toFixed(2)}%`;
+    return `${clampedActivity.toFixed(2)}%`;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5650129 and ef0fa29.

📒 Files selected for processing (8)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1 hunks)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (3 hunks)
  • apps/web/app/api/timesheet/statistics/counts/route.ts (2 hunks)
  • apps/web/app/hooks/features/useReportActivity.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (1 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (3 hunks)
  • apps/web/app/services/server/requests/timesheet.ts (2 hunks)
  • apps/web/app/stores/time-logs.ts (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: deploy
🔇 Additional comments (9)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1)

35-38: Handle missing or null statisticsCounts
By destructuring and immediately calling secondsToTime(statisticsCounts?.weekDuration || 0), you default to 0 if null. That’s fine, but be mindful of other properties you consume from statisticsCounts that might also be null or undefined.

apps/web/app/hooks/features/useReportActivity.ts (3)

10-18: Clarify optional vs. required fields
UseReportActivityProps marks logType, start, and end as optional. However, the default props enforce them. Make sure this design is intentional to avoid confusion or mixed usage.


50-68: Validate merging logic
Within getMergedProps, the code carefully merges custom props with default parameters. Ensure that date/time boundaries (startDate, endDate, start, end) are validated or sanitized to avoid out-of-range headaches.


144-147: Check for missing dependency
Promise.all is used within updateDateRange and updateFilters. If either function depends on something outside these calls (e.g., user state changes), ensure that no race conditions remain.

Also applies to: 152-155

apps/web/app/stores/time-logs.ts (1)

3-3: Validate broader usage of ITimesheetStatisticsData
You’ve expanded the store to hold ITimesheetStatisticsData. If other parts of the code read this atom incorrectly or fail to handle null states, that may cause unexpected exceptions.

apps/web/app/api/timesheet/statistics/counts/route.ts (1)

6-8: LGTM! Well-implemented type guard.

The type guard effectively validates log types, ensuring type safety at runtime.

apps/web/app/services/server/requests/timesheet.ts (1)

182-198: LGTM! Well-implemented request function.

The implementation includes:

  • Proper parameter handling with destructuring
  • Well-configured qs.stringify options
  • Clear JSDoc documentation
apps/web/app/services/client/api/timer/timer-log.ts (2)

228-231: LGTM! Interface update is backward compatible.

The addition of the optional logType property maintains backward compatibility while extending the interface's functionality.


350-350: Verify the statistics API endpoint and response structure.

Ensure that the /timesheet/statistics/counts endpoint exists and returns data matching the ITimesheetStatisticsData interface.

✅ Verification successful

API endpoint implementation verified successfully

The /timesheet/statistics/counts endpoint is properly implemented with:

  • Matching interface definition in ITimesheetStatisticsData
  • Consistent client and server-side implementations
  • Proper authentication and parameter handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and structure of the statistics API endpoint

# Test 1: Check if the endpoint is documented in API specs
echo "Checking API documentation..."
fd -t f "swagger|openapi" . -X grep -l "/timesheet/statistics/counts"

# Test 2: Check if the interface matches the API response structure
echo "Checking interface definition..."
rg -A 10 "interface ITimesheetStatisticsData" .

Length of output: 816


Script:

#!/bin/bash
# Check for the API endpoint implementation
echo "Checking API implementation..."
rg -g "!*.{spec,test}.*" "/timesheet/statistics/counts" .

# Check for any related controller or route definitions
echo "Checking route definitions..."
ast-grep --pattern 'router.$_("/timesheet/statistics/counts"'

Length of output: 525


Script:

#!/bin/bash
# Check server-side implementation
echo "Checking server implementation..."
rg -A 10 "statistics/counts" ./apps/web/app/services/server/requests/timesheet.ts

# Check for any related tests
echo "Checking related tests..."
rg "statistics/counts" -g "*.{spec,test}.*" .

Length of output: 585

@evereq
Copy link
Member

evereq commented Feb 2, 2025

@Innocent-Akim please fix merge conflicts

- Move ITimesheetStatisticsCounts interface to ITimerLog.ts
- Use existing TimeLogType instead of custom LogType
- Update type references across timesheet API endpoints
- Improve type validation for log types

Technical Details:
- Replace custom LogType with existing TimeLogType from ITimer
- Relocate statistics interface to proper timer interfaces file
- Update return types in statistics request function
- Refactor type guard to use TimeLogType

Related files:
- app/interfaces/timer/ITimerLog.ts
- app/services/server/requests/timesheet.ts
- app/api/timesheet/statistics/counts/route.ts
- Add loading spinner instead of dots
- Enhance stats display with default values
- Add smooth transitions for progress bars
- Fix layout and styling issues
- Improve error handling for null values
- Remove progress bars from Total Hours and Members worked
- Use secondsToTime helper for duration formatting
- Add showProgress property to control progress bar visibility
- Clean up and organize stats properties
- Add StatItem interface for better type safety
- Extract formatTime helper function
- Use useMemo for stats array to prevent unnecessary recalculations
- Reduce code repetition with shared timeValue
- Remove unnecessary fragment
- Improve code organization and readability
@Innocent-Akim Innocent-Akim force-pushed the feat/timesheet-statistics-api-types branch from ef0fa29 to 67f9115 Compare February 2, 2025 09:58
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/web/app/services/client/api/timer/timer-log.ts (1)

326-351: ⚠️ Potential issue

Input validation is still missing.

The implementation needs input validation as suggested in the previous review.

Please refer to the previous review comment about adding comprehensive input validation for required parameters, date format, and activity level range.

🧹 Nitpick comments (6)
apps/web/app/api/timesheet/statistics/counts/route.ts (1)

24-26: Consider using a more flexible approach for log types.

The current implementation limits the number of log types to 10 using Array.from({ length: 10 }). This arbitrary limit could cause issues if more log types are needed.

-const logTypes = Array.from({ length: 10 }, (_, i) => searchParams.get(`logType[${i}]`))
+const logTypes = Array.from(searchParams.entries())
+    .filter(([key]) => key.startsWith('logType['))
+    .map(([_, value]) => value)
     .filter((logType): logType is string => logType !== null)
     .filter(isValidLogType);
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1)

9-12: Consider handling edge cases in formatPercentage.

The function should handle negative values and ensure the result is capped at 100%.

 function formatPercentage(value: number | undefined): number {
 	if (!value) return 0;
-	return Math.round(value);
+	return Math.min(100, Math.max(0, Math.round(value)));
 }
apps/web/app/interfaces/timer/ITimerLog.ts (1)

275-282: Consider adding JSDoc comments for statistics data properties.

Adding documentation for each property would improve code maintainability.

 export interface ITimesheetStatisticsData {
+    /** Number of employees involved in the timesheet period */
     employeesCount: number;
+    /** Number of projects worked on during the timesheet period */
     projectsCount: number;
+    /** Percentage of activities completed today */
     todayActivities: number;
+    /** Total duration of activities today in seconds */
     todayDuration: number;
+    /** Percentage of activities completed this week */
     weekActivities: number;
+    /** Total duration of activities this week in seconds */
     weekDuration: number;
 }
apps/web/app/hooks/features/useReportActivity.ts (1)

44-69: Consider improving type safety in getMergedProps.

While the implementation is correct, the type assertion could be made more type-safe.

Consider this improvement:

-            return merged as Required<UseReportActivityProps>;
+            const result: Required<UseReportActivityProps> = merged;
+            return result;

This change will cause TypeScript to verify the object structure at compile time rather than forcing the type.

apps/web/app/services/client/api/timer/timer-log.ts (2)

280-300: Add input validation to utility functions.

While the implementations are correct, they could benefit from input validation.

Consider these improvements:

 export function formatDuration(seconds: number): string {
+    if (!Number.isFinite(seconds) || seconds < 0) {
+        throw new Error('Duration must be a non-negative number');
+    }
     const hours = Math.floor(seconds / 3600);
     // ... rest of the implementation

 export function formatActivity(activity: number): string {
+    if (!Number.isFinite(activity) || activity < 0 || activity > 100) {
+        throw new Error('Activity must be a number between 0 and 100');
+    }
     return `${activity.toFixed(2)}%`;
 }

302-325: Enhance function documentation with @throws annotation.

The documentation is comprehensive but could benefit from error documentation.

Consider adding @throws annotation:

 /**
  * Get timesheet statistics counts
  * @param params Request parameters including activity levels, log types, and date range
  * @returns Promise with statistics counts data
+ * @throws {Error} When required parameters are missing or invalid
+ * @throws {Error} When the API request fails
  * @example
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef0fa29 and 67f9115.

📒 Files selected for processing (8)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1 hunks)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (3 hunks)
  • apps/web/app/api/timesheet/statistics/counts/route.ts (2 hunks)
  • apps/web/app/hooks/features/useReportActivity.ts (6 hunks)
  • apps/web/app/interfaces/timer/ITimerLog.ts (1 hunks)
  • apps/web/app/services/client/api/timer/timer-log.ts (3 hunks)
  • apps/web/app/services/server/requests/timesheet.ts (2 hunks)
  • apps/web/app/stores/time-logs.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/web/app/stores/time-logs.ts
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: deploy
🔇 Additional comments (9)
apps/web/app/api/timesheet/statistics/counts/route.ts (2)

6-8: LGTM! Well-implemented type guard.

The type guard function effectively ensures type safety for log types.


54-57: 🛠️ Refactor suggestion

Add validation for activity level values.

The activity level parameters are parsed but not validated. This could lead to issues if invalid values are provided.

+const start = parseInt(activityLevelStart || '0');
+const end = parseInt(activityLevelEnd || '100');
+
+if (isNaN(start) || isNaN(end) || start < 0 || end > 100 || start > end) {
+    return NextResponse.json(
+        { error: 'Activity level values must be between 0 and 100, and start must be less than end' },
+        { status: 400 }
+    );
+}
+
 const { data } = await getTimesheetStatisticsCountsRequest({
     activityLevel: {
-        start: parseInt(activityLevelStart || '0'),
-        end: parseInt(activityLevelEnd || '100')
+        start,
+        end
     },

Likely invalid or redundant comment.

apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1)

39-83: 🛠️ Refactor suggestion

Optimize stats array memoization.

The useMemo dependency array is missing dependencies for individual statistics values.

 const stats: StatItem[] = useMemo(
     () => [...],
-    [timeValue, progress, statisticsCounts?.employeesCount]
+    [
+        timeValue,
+        progress,
+        statisticsCounts?.employeesCount,
+        statisticsCounts?.weekDuration,
+        statisticsCounts?.weekActivities
+    ]
 );

Likely invalid or redundant comment.

apps/web/app/interfaces/timer/ITimerLog.ts (1)

268-273: LGTM! Well-structured statistics interface.

The interface clearly separates different types of statistics while maintaining type safety.

apps/web/app/hooks/features/useReportActivity.ts (4)

1-18: LGTM! Well-structured interface with clear optional properties.

The interface design effectively extends ITimeLogReportDailyChartProps while making certain properties optional, providing good flexibility.


20-41: LGTM! Well-typed defaultProps with sensible defaults.

The defaultProps are properly typed using Required<Pick<...>> and include appropriate default values.


136-178: LGTM! Proper Promise.all usage with error handling.

The concurrent execution of fetch operations is well-implemented with proper error handling.


116-134: Review hardcoded logType override.

The function overrides any custom logType with [TimeLogType.TRACKED], which might not be the intended behavior.

Consider if this is intentional:

-            const response = await queryTimesheetStatisticsCounts({...mergedProps,  logType:[TimeLogType.TRACKED]});
+            const response = await queryTimesheetStatisticsCounts(mergedProps);

Also, consider adding more specific error handling:

         } catch (error) {
-            console.error('Error fetching statistics:', error);
+            console.error('Error fetching timesheet statistics:', error instanceof Error ? error.message : 'Unknown error');
             setStatisticsCounts(null);
+            throw error; // Re-throw to allow parent error boundaries to handle it
         }
✅ Verification successful

Review hardcoded logType and error handling improvements.

The current implementation unconditionally sets logType to [TimeLogType.TRACKED], which means any custom logType provided via customProps is ignored. This behavior may be intentional if the function is meant to always query for tracked logs, but if flexibility is desired then merging or conditionally overriding logType would be more appropriate. Additionally, consider using more specific error messages (and potentially re-throwing errors) to better integrate with higher-level error boundaries.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found

apps/web/app/services/client/api/timer/timer-log.ts (1)

217-233: LGTM! Clear and type-safe interface update.

The addition of optional logType is well-typed and maintains backward compatibility.

@Innocent-Akim Innocent-Akim requested a review from evereq February 2, 2025 10:03
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67f9115 and fdac443.

📒 Files selected for processing (4)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (1 hunks)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-table.tsx (1 hunks)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (3 hunks)
  • apps/web/components/pages/task/ParentTask.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-table.tsx
  • apps/web/components/pages/task/ParentTask.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: deploy
🔇 Additional comments (3)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/page.tsx (1)

18-18: LGTM! Clean integration of timesheet statistics.

The changes effectively integrate the new timesheet statistics functionality. The hook usage and prop passing are clean and well-structured.

Also applies to: 22-22, 57-60

apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-grid.tsx (2)

9-16: LGTM! Well-structured component with good UX.

The implementation shows good practices:

  • Clean helper functions
  • Well-defined interface
  • Proper loading state handling
  • Good accessibility with semantic markup

Also applies to: 18-26, 85-115


35-37: Review time value calculations for different stat types.

Currently, the same timeValue is used for 'Manual', 'Idle', and 'Total Hours' stats. This might not accurately represent the different time categories.

Consider using specific duration values from statisticsCounts:

-const { h: hours, m: minutes, s: seconds } = secondsToTime(statisticsCounts?.weekDuration || 0);
-const timeValue = formatTime(hours, minutes, seconds);
+const getTimeValue = (duration: number) => {
+  const { h, m, s } = secondsToTime(duration || 0);
+  return formatTime(h, m, s);
+};
+
+const trackedTime = getTimeValue(statisticsCounts?.weekDuration || 0);
+const manualTime = getTimeValue(statisticsCounts?.manualTime || 0);
+const idleTime = getTimeValue(statisticsCounts?.idleTime || 0);
+const totalTime = getTimeValue(statisticsCounts?.totalTime || 0);

Then update the stats array accordingly:

-value: timeValue,  // in Manual stat
+value: manualTime,

Also applies to: 58-58, 67-67, 76-76

@evereq evereq merged commit 99b69a8 into develop Feb 2, 2025
13 checks passed
@evereq evereq deleted the feat/timesheet-statistics-api-types branch February 2, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants