[Feat]: Improve timesheet and Activity reporting #3590
Conversation
- Enhance date handling in useTimesheet hook - Fix dependency array in useTimelogFilterOptions - Update team dashboard components for better activity tracking - Improve chart visualization and filtering options - Update timer log interfaces - Update yarn dependencies Components affected: - activity-modal - team-dashboard-filter - team-stats-chart - useReportActivity - useTimelogFilterOptions - useTimesheet - ITimerLog interface
|
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
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsxOops! 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/hooks/features/useTimesheet.tsOops! 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. WalkthroughThis pull request applies styling updates and code refinements across several team dashboard components and hooks. The UI changes include modifications to font sizes, colors, transitions, and dark mode classes. Hooks have been updated with additional filtering properties, dependency adjustments, and improved date handling using moment.js. Additionally, an interface has been extended with extra optional identifier fields. Overall, these changes focus on visual and code structure enhancements without modifying the core component functionality. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat are http dependencies?Contains a dependency which resolves to a remote HTTP URL which could be used to inject untrusted code and reduce overall package reliability. Publish the HTTP URL dependency to npm or a private package repository and consume it from there. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/app/hooks/features/useTimesheet.ts (2)
18-23: Add JSDoc documentation for the TimesheetParams interface.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Interface for timesheet parameters + * @interface TimesheetParams + * @property {Date | string} [startDate] - Start date for the timesheet period + * @property {Date | string} [endDate] - End date for the timesheet period + * @property {'ListView' | 'CalendarView'} [timesheetViewMode] - View mode for displaying timesheet + * @property {string} [inputSearch] - Search string to filter timesheet entries + */ interface TimesheetParams { startDate?: Date | string; endDate?: Date | string; timesheetViewMode?: 'ListView' | 'CalendarView'; inputSearch?: string; }
298-298: Translate French comment to English for consistency.The codebase should maintain consistent language usage in comments.
-// Effet pour déclencher la requête quand les dates changent +// Effect to trigger the request when dates change🧰 Tools
🪛 GitHub Check: Cspell
[warning] 298-298:
Unknown word (Effet)
[warning] 298-298:
Unknown word (déclencher)
[warning] 298-298:
Unknown word (requête)
[warning] 298-298:
Unknown word (quand)
[warning] 298-298:
Unknown word (changent)🪛 GitHub Actions: Check Spelling and Typos with cspell
[warning] 298-298: Unknown word (Effet)
[warning] 298-298: Unknown word (déclencher)
[warning] 298-298: Unknown word (requête)
[warning] 298-298: Unknown word (quand)
[warning] 298-298: Unknown word (changent)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/activity-modal.tsx (1)
107-107: Consider a more moderate font size for the modal header.The 28px font size for the employee name might be too large for a modal header, potentially causing layout issues on smaller screens. Consider using a more moderate size (e.g., 24px) that maintains hierarchy while ensuring better responsiveness.
-<h3 className="text-xl font-medium text-gray-900 dark:text-gray-100 text-[28px]"> +<h3 className="text-xl font-medium text-gray-900 dark:text-gray-100 text-[24px]">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/server-web/release/app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/activity-modal.tsx(3 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx(3 hunks)apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsx(2 hunks)apps/web/app/hooks/features/useReportActivity.ts(1 hunks)apps/web/app/hooks/features/useTimelogFilterOptions.ts(1 hunks)apps/web/app/hooks/features/useTimesheet.ts(2 hunks)apps/web/app/interfaces/timer/ITimerLog.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/hooks/features/useTimesheet.ts
[error] 118-118: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 GitHub Check: Cspell
apps/web/app/hooks/features/useTimesheet.ts
[warning] 298-298:
Unknown word (Effet)
[warning] 298-298:
Unknown word (déclencher)
[warning] 298-298:
Unknown word (requête)
[warning] 298-298:
Unknown word (quand)
[warning] 298-298:
Unknown word (changent)
🪛 GitHub Actions: Check Spelling and Typos with cspell
apps/web/app/hooks/features/useTimesheet.ts
[warning] 298-298: Unknown word (Effet)
[warning] 298-298: Unknown word (déclencher)
[warning] 298-298: Unknown word (requête)
[warning] 298-298: Unknown word (quand)
[warning] 298-298: Unknown word (changent)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
- GitHub Check: deploy
🔇 Additional comments (12)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (1)
72-74: LGTM! Dependency array correctly updated.The addition of
setSelectTimesheetIdto the dependency array is correct as it's used in the cleanup function. This prevents potential stale closure issues.apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-dashboard-filter.tsx (3)
50-50: LGTM! Dark mode styling added.The addition of
dark:bg-dark--theme-lightclass improves accessibility in dark mode.
64-66: LGTM! Enhanced UX with selection counts.Adding the count of selected items to the clear button labels improves user feedback.
Also applies to: 89-90
69-69: LGTM! More descriptive localStorage keys.The localStorage keys have been updated to be more specific and descriptive:
team-dashboard-select-filter-teamteam-dashboard-select-filter-employeeAlso applies to: 93-93
apps/web/app/interfaces/timer/ITimerLog.ts (1)
182-183: LGTM! Interface extended with optional identifiers.The addition of optional
teamIdanduserIdproperties enhances filtering capabilities while maintaining backward compatibility.apps/web/app/hooks/features/useReportActivity.ts (3)
15-27: LGTM! Interface extended with filtering properties.The UseReportActivityProps interface has been enhanced with additional filtering capabilities:
- projectIds
- employeeIds
- teamIds
29-57: LGTM! Default props properly initialized.The defaultProps object correctly initializes the new filtering arrays as empty:
employeeIds: [], projectIds: [], teamIds: []
95-96: LGTM! Efficient mapping of filter IDs.The getMergedProps function efficiently maps team and user IDs:
employeeIds: alluserState?.map(({ employee: { id } }) => id).filter(Boolean), teamIds: allteamsState?.map(({ id }) => id).filter(Boolean),apps/web/app/hooks/features/useTimesheet.ts (1)
406-410: Well-implemented type guard for TimesheetStatus.The type guard implementation is clean and ensures type safety when working with timesheet statuses.
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/activity-modal.tsx (2)
31-31: LGTM! Smooth transition effect added.The addition of the transition class with ease-in-out timing provides a smooth animation effect when the circle's properties change.
46-54: Verify color contrast accessibility.While the styling changes provide consistent text sizes and dark mode support, please verify that the text color
#60646Cmeets WCAG color contrast requirements, especially for the 14px font size.✅ Verification successful
🌐 Web query:
Does text color #60646C on white background meet WCAG 2.1 AA requirements for 14px text?Length of output: 3420
Color Contrast Compliance Verified
The text color
#60646Con a white background meets the WCAG 2.1 AA requirements for 14px non-bold text. The contrast ratio exceeds the minimum 4.5:1 threshold, ensuring sufficient readability.
- Contrast Ratio: The contrast ratio for
#60646Cagainst white is inferred to be greater than 4.5:1.- Text Size: At 14px and non-bold, the text qualifies as normal text under WCAG guidelines.
- Dark Mode Consideration: The presence of dark mode support doesn't affect this specific evaluation.
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsx (1)
51-51: LGTM!The formatting change improves code readability and follows TypeScript conventions.
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsx
Outdated
Show resolved
Hide resolved
|
Report too large to display inline |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/app/hooks/features/useTimesheet.ts (1)
298-302:⚠️ Potential issueRemove duplicate useEffect for getTaskTimesheet.
There are two useEffect hooks that trigger getTaskTimesheet with similar dependencies. This could cause duplicate API calls and unnecessary re-renders.
Combine the effects into a single useEffect:
-useEffect(() => { - if (startDate || endDate) { - getTaskTimesheet({ startDate, endDate }); - } -}, [startDate, endDate, getTaskTimesheet]); useEffect(() => { getTaskTimesheet({ startDate, endDate }); -}, [getTaskTimesheet, startDate, endDate, timesheetGroupByDays, inputSearch]); +}, [getTaskTimesheet, startDate, endDate, timesheetGroupByDays, inputSearch, user]);Also applies to: 518-520
🧹 Nitpick comments (1)
apps/web/app/hooks/features/useTimesheet.ts (1)
105-159: Consider optimizing the grouping function.The current implementation creates intermediate objects that could be memory-intensive for large datasets. Consider using Map for better performance.
const createGroupingFunction = (getKey: GroupingKeyFunction) => (items: TimesheetLog[]): GroupedTimesheet[] => { if (!items?.length) return []; - const groupedByDate = items.reduce( + const groupedByDate = new Map(); + items.forEach(item => { // ... existing validation ... - const key = getKey(date); - if (!acc[key]) { - acc[key] = { + const key = getKey(date); + if (!groupedByDate.has(key)) { + groupedByDate.set(key, { date: key, timesheets: {} - }; + }); } // ... rest of the logic ... - }, {});🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsx(3 hunks)apps/web/app/hooks/features/useTimesheet.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/[locale]/dashboard/team-dashboard/[teamId]/components/team-stats-chart.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/hooks/features/useTimesheet.ts
[error] 118-118: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
- GitHub Check: deploy
🔇 Additional comments (3)
apps/web/app/hooks/features/useTimesheet.ts (3)
18-28: Interface definitions look good!The interfaces are well-defined with proper optional fields and clear type definitions.
224-239: Well-implemented date handling with proper error handling!The date handling implementation is robust with:
- Proper fallback to defaults
- Comprehensive error handling
- Clear validation of date formats
Also applies to: 246-266
118-118:⚠️ Potential issueReplace unsafe isNaN with Number.isNaN.
The global
isNaNfunction performs type coercion which can lead to unexpected results.- if (isNaN(date.getTime())) { + if (Number.isNaN(date.getTime())) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Components affected:
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