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]: Add sortable headers to Timesheet component with dynamic sorting #3367

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

Innocent-Akim
Copy link
Contributor

@Innocent-Akim Innocent-Akim commented Nov 25, 2024

Description

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

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 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 previous status

Current screenshots

Please add here videos or images of previous status

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the TimesheetLoader component for improved loading experience.
    • Enhanced useTimelogFilterOptions hook to manage selections based on status and date.
    • Added sorting functionality to the DataTableTimeSheet component with new header elements.
    • Updated TimesheetView component to handle loading states more explicitly.
  • Bug Fixes

    • Updated rendering logic to display 'REJECTED' instead of 'DENIED' for specific statuses.
  • Refactor

    • Improved modularity and readability of the DataTableTimeSheet component by introducing HeaderRow and HeaderColumn components.
    • Streamlined rendering logic in the TimesheetView component to enhance readability.

Copy link
Contributor

coderabbitai bot commented Nov 25, 2024

Walkthrough

A new React functional component named TimesheetLoader has been introduced, which manages a loading state with visual dots. Modifications have been made to the useTimelogFilterOptions hook by adding a state variable for selected items and a function to manage selections based on status and date. The DataTableTimeSheet component has been updated to include sorting functionality and improved rendering logic, with new header components added. The TimesheetView component now handles loading states more explicitly.

Changes

File Change Summary
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx New component TimesheetLoader added, managing loading state with visual dots.
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx Export statement for TimesheetLoader added: export * from './TimesheetLoader'.
apps/web/app/hooks/features/useTimelogFilterOptions.ts New state selectedItems added; new function handleSelectRowByStatusAndDate introduced to manage selections.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx Updates to DataTableTimeSheet for sorting functionality; new components HeaderRow and HeaderColumn added.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx TimesheetView updated to include a loading prop for improved loading state handling.
apps/web/app/[locale]/timesheet/[memberId]/page.tsx useTimesheet hook updated to include loadingTimesheet, passed to TimesheetView.

Possibly related PRs

Suggested reviewers

  • evereq
  • Cedric921

🐰 In the meadow, a loader hops,
Bringing timesheets, never stops.
With status and dates, it plays its part,
A functional friend, straight from the heart.
So let’s cheer for the code that’s new,
Timesheet magic, just for you! 🌼

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]/timesheet/[memberId]/components/TimesheetView.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.


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 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 self-assigned this Nov 25, 2024
@Innocent-Akim Innocent-Akim added the Don't Merge Do not merge this PR until label not removed label Nov 25, 2024
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

🧹 Outside diff range and nitpick comments (6)
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx (1)

12-12: Add documentation for the new component

Since this appears to be a new component that could be crucial for UX during sorting operations, please add:

  1. JSDoc comments explaining the component's purpose
  2. Usage examples in the component's file
apps/web/app/hooks/features/useTimelogFilterOptions.ts (3)

39-40: Remove unnecessary empty lines.

These consecutive empty lines don't add to code readability and should be removed.


31-37: Consider improving the selection handler.

While the current implementation works, there are several potential improvements:

  1. The function name could be more descriptive (e.g., toggleItemSelection)
  2. Consider adding parameter validation
  3. The toggle logic could be simplified

Here's a suggested improvement:

-const handleSelectRowByStatusAndDate = (status: string, date: string) => {
-    setSelectedItems((prev) =>
-        prev.some((item) => item.status === status && item.date === date)
-            ? prev.filter((item) => !(item.status === status && item.date === date))
-            : [...prev, { status, date }]
-    );
-}
+const toggleItemSelection = (status: string, date: string) => {
+    if (!status || !date) {
+        console.warn('Invalid status or date provided');
+        return;
+    }
+    setSelectedItems((prev) => {
+        const isSelected = prev.some(
+            (item) => item.status === status && item.date === date
+        );
+        return isSelected
+            ? prev.filter((item) => !(item.status === status && item.date === date))
+            : [...prev, { status, date }];
+    });
+};

Line range hint 13-56: Consider performance optimization for large datasets.

While the current implementation is sound, consider these architectural improvements:

  1. Implement memoization for the selection check using useMemo if the dataset is large
  2. Consider using a Map or Set data structure for more efficient lookups if dealing with many selections
  3. Add debouncing if the selection handler is called frequently (e.g., from a rapidly updating UI)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2)

571-602: Enhance HeaderColumn to indicate current sort order

The HeaderColumn component displays static sort arrows, which may not clearly indicate the current sort state to the user. Updating the icon based on currentSort can improve usability by providing visual feedback on the sort order.

Consider updating the component as follows:

 const HeaderColumn = ({
     label,
     onSort,
     currentSort,
 }: {
     label: string;
     onSort: () => void;
     currentSort: SortOrder | null;
 }) => (
     <div className="flex gap-x-2">
         <span>{label}</span>
         <button
             onClick={onSort}
             className="flex flex-col items-center leading-none gap-0"
         >
-            <MdKeyboardArrowUp
-                style={{
-                    height: 10,
-                    color: "#71717A",
-                }}
-            />
-            <MdKeyboardArrowDown
-                style={{
-                    height: 10,
-                    color: "#71717A",
-                }}
-            />
+            {currentSort === 'ASC' && (
+                <MdKeyboardArrowUp style={{ height: 16, color: "#71717A" }} />
+            )}
+            {currentSort === 'DESC' && (
+                <MdKeyboardArrowDown style={{ height: 16, color: "#71717A" }} />
+            )}
+            {!currentSort && (
+                <ArrowUpDownIcon className="w-4 h-4 ml-2" />
+            )}
         </button>
     </div>
 );

This change updates the icon to reflect the current sort order, improving the user experience.


Line range hint 262-348: Centralize status mapping from 'DENIED' to 'REJECTED'

The condition {status === 'DENIED' ? 'REJECTED' : status} is used in multiple places to display the status. To enhance maintainability and avoid duplication, consider creating a utility function to handle this status mapping.

For example, create a function:

const displayStatus = (status: string) => (status === 'DENIED' ? 'REJECTED' : status);

And replace occurrences with:

- {status === 'DENIED' ? 'REJECTED' : status}
+ {displayStatus(status)}

Apply this change wherever the status needs to be displayed with this mapping.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 528362c and 87bd11d.

📒 Files selected for processing (4)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx (1 hunks)
  • apps/web/app/hooks/features/useTimelogFilterOptions.ts (3 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (9 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx
🔇 Additional comments (3)
apps/web/app/hooks/features/useTimelogFilterOptions.ts (3)

13-13: LGTM! State declaration is well-typed and properly initialized.

The new state for tracking selected items is appropriately typed and initialized with an empty array.


55-56: LGTM! Return object properly updated.

The new handler and state are correctly exposed in the hook's return object.


31-37: Verify integration with DataTableTimeSheet component.

Since this hook is used for sortable headers in the Timesheet component, let's verify the integration.

✅ Verification successful

Integration with DataTableTimeSheet component is properly implemented

The verification shows that handleSelectRowByStatusAndDate is correctly integrated within the DataTableTimeSheet component:

  • It's used in the HeaderRow component for checkbox selection functionality
  • The function is passed down from useTimelogFilterOptions hook to handle row selection by status and date
  • The implementation aligns with the sortable headers feature as it's part of the filtering and selection system in the timesheet table
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of handleSelectRowByStatusAndDate in DataTableTimeSheet
rg -l "handleSelectRowByStatusAndDate" "apps/web/lib/features/integrations"

# Check for potential sorting-related components
ast-grep --pattern 'const DataTableTimeSheet = $_'

Length of output: 191


Script:

#!/bin/bash
# Let's check the implementation details of table-time-sheet.tsx
cat "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"

# Also check for any sorting-related props or handlers
ast-grep --pattern 'interface DataTableTimeSheetProps {
  $$$
}'

Length of output: 20795

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

🧹 Outside diff range and nitpick comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)

570-601: Improve HeaderColumn component implementation

The HeaderColumn component has room for improvement:

  1. The sort icons are always the same color regardless of active state
  2. No visual indication of current sort direction

Consider this enhancement:

const HeaderColumn = ({
    label,
    onSort,
    currentSort,
}: {
    label: string;
    onSort: () => void;
    currentSort: SortOrder | null;
}) => (
    <div className="flex gap-x-2">
        <span>{label}</span>
        <button
            onClick={onSort}
            className="flex flex-col items-start leading-none gap-0"
        >
            <MdKeyboardArrowUp
                style={{
                    height: 10,
-                   color: "#71717A",
+                   color: currentSort === "ASC" ? "#000000" : "#71717A",
                }}
            />
            <MdKeyboardArrowDown
                style={{
                    height: 10,
-                   color: "#71717A",
+                   color: currentSort === "DESC" ? "#000000" : "#71717A",
                }}
            />
        </button>
    </div>
);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 87bd11d and 770ef16.

📒 Files selected for processing (1)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (10 hunks)
🔇 Additional comments (5)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (5)

41-43: LGTM: Icon imports for sorting functionality

The new imports for sorting icons align well with the implementation of sortable headers.


158-158: Implement sorting logic in handleSort

The handleSort function currently only logs to the console and doesn't trigger actual sorting.

This issue was previously identified. The function should update the sorting state to trigger the table re-render:

const handleSort = (key: string, order: SortOrder) => {
-    console.log(`Sorting ${key} in ${order} order`);
+    setSorting([{ id: key, desc: order === 'DESC' }]);
};

Also applies to: 200-202


261-265: LGTM: Improved AccordionItem rendering and status display

Good improvements:

  • Conditional rendering based on rows length
  • Consistent status terminology (DENIED → REJECTED)
  • Added row count display

Also applies to: 282-282


348-348: LGTM: Consistent status terminology

Good consistency in displaying 'REJECTED' instead of 'DENIED' across the component.


298-304: Fix incorrect onCheckedChange handler in HeaderRow

The checkbox handler is still incorrectly implemented. It returns the function reference without executing it and missing the date parameter:

<Checkbox 
-    onCheckedChange={() => handleSelectRowByStatusAndDate}
+    onCheckedChange={() => handleSelectRowByStatusAndDate(status, plan.date)}
    className="w-5 h-5"
/>

Also applies to: 624-625

@Innocent-Akim Innocent-Akim added bug Something isn't working feature WEB Web app Ever Teams and removed Don't Merge Do not merge this PR until label not removed labels Nov 26, 2024
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

🧹 Outside diff range and nitpick comments (12)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx (4)

1-4: Consider adding explicit TypeScript interface for props

For better type safety and documentation, consider defining a dedicated interface for the component props.

+interface TimesheetLoaderProps {
+  show?: boolean;
+}

-export function TimesheetLoader({ show = false }: { show?: boolean }) {
+export function TimesheetLoader({ show = false }: TimesheetLoaderProps) {

7-18: Consider extracting constants and improving animation logic

The implementation works but could be more maintainable with named constants and a more declarative approach.

+const ANIMATION_INTERVAL = 1000; // 1 second
+const MAX_DOTS = 3;
+const DOT_CHAR = '.';

 useEffect(() => {
     if (!show) {
         setDots(""); // Reset the dots when loader is hidden
         return;
     }

     const interval = setInterval(() => {
-        setDots((prev) => (prev.length < 3 ? prev + "." : ""));
+        setDots((prev) => (prev.length < MAX_DOTS ? prev + DOT_CHAR : ""));
-    }, 1000); // Update dots every second
+    }, ANIMATION_INTERVAL);

     return () => clearInterval(interval);
 }, [show]);

20-22: Enhance accessibility for screen readers

Consider adding ARIA attributes to improve accessibility and provide a better experience for screen readers.

 return (
-    <BackdropLoader show={show} title={`Loading${dots}`} />
+    <BackdropLoader
+      show={show}
+      title={`Loading${dots}`}
+      aria-live="polite"
+      aria-busy={show}
+      role="status"
+    />
 );

1-23: Consider moving to shared components

Since this is a generic loading component, consider:

  1. Moving it to the shared components directory if it could be reused across other features
  2. Adding JSDoc documentation to describe its usage and props
+/**
+ * A loading indicator component that displays animated dots.
+ * @component
+ * @param {Object} props - Component props
+ * @param {boolean} [props.show=false] - Controls the visibility of the loader
+ * @example
+ * <TimesheetLoader show={isLoading} />
+ */
 export function TimesheetLoader({ show = false }: { show?: boolean }) {
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (2)

8-14: Enhance loading state UI and prevent layout shifts

While the loading state implementation is functional, it could benefit from:

  1. Using a proper loader component for better user experience
  2. Adding min-height constraint to prevent layout shifts
 if (!data) {
   return (
-    <div className="grow h-full w-full bg-[#FFFFFF] dark:bg-dark--theme flex items-center justify-center">
+    <div className="grow h-full w-full bg-[#FFFFFF] dark:bg-dark--theme flex items-center justify-center min-h-[280px]">
-      <p>{t('pages.timesheet.LOADING')}</p>
+      <TimesheetLoader />
     </div>
   );
 }

16-22: Enhance empty state with visual feedback

Consider improving the empty state UX by adding an icon or illustration to make it more visually appealing and informative.

 if (data.length === 0) {
   return (
     <div className="grow h-full w-full bg-[#FFFFFF] dark:bg-dark--theme flex flex-col items-center justify-center h-full min-h-[280px]">
+      <EmptyStateIcon className="w-16 h-16 mb-4 text-gray-400" />
       <p>{t('pages.timesheet.NO_ENTRIES_FOUND')}</p>
+      <p className="text-sm text-gray-500 mt-2">{t('pages.timesheet.NO_ENTRIES_FOUND_DESCRIPTION')}</p>
     </div>
   );
 }
apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)

46-49: LGTM! Consider adding type safety for loadingTimesheet.

The addition of loadingTimesheet state is a good practice for handling loading states. However, consider adding TypeScript type annotations for better type safety.

-const { timesheet, statusTimesheet, loadingTimesheet } = useTimesheet({
+const { 
+  timesheet, 
+  statusTimesheet, 
+  loadingTimesheet 
+}: { 
+  timesheet: Array<any>; // Replace 'any' with your actual type
+  statusTimesheet: Record<string, Array<any>>; // Replace 'any' with your actual type
+  loadingTimesheet: boolean;
+} = useTimesheet({
   startDate: dateRange.from ?? '',
   endDate: dateRange.to ?? ''
 });

198-199: Improve code formatting for better readability.

Consider formatting the component props on separate lines for better readability.

-<TimesheetView data={filterDataTimesheet}
-  loading={loadingTimesheet} />
+<TimesheetView 
+  data={filterDataTimesheet}
+  loading={loadingTimesheet} 
+/>
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4)

261-262: Consider using early return pattern for better readability

The current implementation uses a boolean condition within JSX. Consider using an early return pattern for better readability.

 {Object.entries(getStatusTimesheet(plan.tasks)).map(([status, rows]) => {
-    return rows.length > 0 && status && <AccordionItem
+    if (!rows.length || !status) return null;
+    return <AccordionItem

Also applies to: 264-265


625-629: Consider adding error handling for sorting operations

The handleSort function in HeaderRow should include error handling to manage potential sorting failures gracefully.

 const handleSort = (key: string) => {
+  try {
     const newOrder = sortState[key] === "ASC" ? "DESC" : "ASC";
     setSortState({ ...sortState, [key]: newOrder });
     onSort(key, newOrder);
+  } catch (error) {
+    console.error(`Error sorting by ${key}:`, error);
+    // Consider showing a user-friendly error message
+  }
 };

571-571: Consider using enum for SortOrder type

Using a string literal type for SortOrder is good, but an enum would provide better type safety and IDE support.

-type SortOrder = "ASC" | "DESC";
+enum SortOrder {
+  ASC = "ASC",
+  DESC = "DESC"
+}

617-623: Consider moving sort state initialization to a constant

The initial sort state object could be moved to a constant to improve maintainability and reusability.

+const INITIAL_SORT_STATE = {
+  Task: null,
+  Project: null,
+  Employee: null,
+  Status: null,
+} as const;

-const [sortState, setSortState] = React.useState<{ [key: string]: SortOrder | null }>({
-  Task: null,
-  Project: null,
-  Employee: null,
-  Status: null,
-});
+const [sortState, setSortState] = React.useState<{ [key: string]: SortOrder | null }>(INITIAL_SORT_STATE);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 770ef16 and dcee5bd.

📒 Files selected for processing (4)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (1 hunks)
  • apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (10 hunks)
🔇 Additional comments (7)
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetLoader.tsx (1)

5-5: LGTM!

The state management is clean and follows React best practices.

apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (1)

24-28: Consider memoizing the component for performance

For better performance, consider memoizing the DataTableTimeSheet component if it's rendering large datasets.

+ import { memo } from 'react';

- <DataTableTimeSheet data={data} />
+ const MemoizedDataTableTimeSheet = memo(DataTableTimeSheet);
+ <MemoizedDataTableTimeSheet data={data} />

Also, verify that the GroupedTimesheet[] type matches the expected prop type of DataTableTimeSheet.

apps/web/app/[locale]/timesheet/[memberId]/page.tsx (2)

Line range hint 54-71: Consider sorting order in data filtering.

The current filtering implementation might need to account for the sorting order of the timesheet data. Ensure that filtering and sorting work together correctly.


198-199: Verify implementation of sortable headers.

The PR objective mentions implementing sortable headers, but I don't see the sorting implementation in this file.

✅ Verification successful

Let me try a different search strategy to verify the sorting implementation.


I notice that TimesheetView is using a DataTableTimeSheet component from 'lib/features/integrations/calendar'. Let's check this component for sorting implementation.


Let me try one more search with corrected paths since the library might be in a different location.


Now I can see the correct path. Let's check the table-time-sheet component for sorting implementation.


Sorting functionality is implemented in the DataTableTimeSheet component

The sorting implementation is present in the codebase:

  • The table uses TanStack Table with built-in sorting functionality
  • Column headers have sort toggles with ascending/descending states
  • Sorting is implemented for Project, Employee, and Status columns
  • The implementation includes:
    • SortingState management
    • getSortedRowModel for data sorting
    • Column-level sort toggles with ArrowUpDownIcon indicators
    • HeaderRow component with sort handlers for each column
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for sorting-related code in the TimesheetView component and its children

# Search for sort-related code in TimesheetView component
echo "Searching for sort implementation in TimesheetView component..."
rg -A 5 "sort" "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx"

# Search for table header components that might contain sorting logic
echo "Searching for table headers with sorting..."
ast-grep --pattern 'const $header = $$$sort$$$'

Length of output: 372


Script:

#!/bin/bash
# Let's check the TimesheetView component and its related files more thoroughly

# First, let's see the content of TimesheetView component
echo "Content of TimesheetView component:"
cat "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx"

# Look for any table-related components that might contain sorting
echo -e "\nSearching for table-related code:"
rg -l "Table|Header|Column" "apps/web/app/[locale]/timesheet/[memberId]/components/"

# Search for any sorting state or handlers
echo -e "\nSearching for sorting state or handlers:"
rg -A 5 "onSort|sortBy|sortDirection" "apps/web/app/[locale]/timesheet/[memberId]/components/"

Length of output: 1633


Script:

#!/bin/bash
# Let's examine the DataTableTimeSheet component

# Find the exact location of DataTableTimeSheet component
echo "Looking for DataTableTimeSheet component:"
fd -e tsx -e ts "DataTableTimeSheet" lib/

# If found, check its contents
echo -e "\nChecking DataTableTimeSheet implementation:"
rg -A 10 "DataTableTimeSheet" lib/

# Look for sorting-related code in the component
echo -e "\nLooking for sorting implementation:"
rg -A 5 "sort|Sort" lib/features/integrations/calendar/

Length of output: 630


Script:

#!/bin/bash
# Let's try to find the DataTableTimeSheet component in the entire codebase

# Search for the file containing DataTableTimeSheet
echo "Looking for DataTableTimeSheet component:"
fd -e tsx -e ts "DataTableTimeSheet"

# Search for any imports of DataTableTimeSheet to find its location
echo -e "\nSearching for DataTableTimeSheet imports:"
rg "import.*DataTableTimeSheet"

# Look for any table components with sorting functionality
echo -e "\nSearching for table components with sorting:"
rg -A 5 "Table.*sort|sort.*Table" --type ts --type tsx

Length of output: 732


Script:

#!/bin/bash
# Check the table-time-sheet component implementation

echo "Checking table-time-sheet component:"
cat "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"

# In case the file is not found at that exact location, let's also search nearby
echo -e "\nSearching for sorting implementation in calendar integration files:"
rg -A 5 "sort|Sort" "apps/web/lib/features/integrations/calendar/"

Length of output: 33941

apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)

41-43: LGTM: Icon imports for sorting functionality

The addition of arrow icons from react-icons/md is appropriate for implementing the sorting UI.


200-202: ⚠️ Potential issue

Implement sorting logic in handleSort

The handleSort function currently only logs to the console and doesn't implement the actual sorting logic.

Apply this diff to implement the sorting:

 const handleSort = (key: string, order: SortOrder) => {
-    console.log(`Sorting ${key} in ${order} order`);
+    setSorting([{ id: key, desc: order === 'DESC' }]);
 };

Likely invalid or redundant comment.


298-305: ⚠️ Potential issue

Fix incorrect handleSelectRowByStatusAndDate implementation

The handler is being passed as a function that returns the actual handler, which means it won't execute when the checkbox is clicked.

 <HeaderRow
     handleSelectRowByStatusAndDate={
-        () => handleSelectRowByStatusAndDate(status, plan.date)}
+        handleSelectRowByStatusAndDate}
     data={rows}
     status={status}
     onSort={handleSort}
     date={plan.date}
 />

Likely invalid or redundant comment.

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: 2

🧹 Outside diff range and nitpick comments (3)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (3)

Line range hint 261-282: Consider using an enum for status values

The status text transformation from 'DENIED' to 'REJECTED' is repeated in multiple places. This could lead to maintenance issues if the mapping needs to change.

Create a status enum or mapping object:

+const STATUS_DISPLAY_MAP = {
+    'DENIED': 'REJECTED'
+} as const;
+
+const getDisplayStatus = (status: string) => STATUS_DISPLAY_MAP[status] || status;

-status === 'DENIED' ? 'REJECTED' : status
+getDisplayStatus(status)

619-624: Use TypeScript enum for sort state keys

The sort state keys should be typed to prevent potential typos and improve maintainability.

Consider using an enum:

+enum SortableColumns {
+    Task = 'Task',
+    Project = 'Project',
+    Employee = 'Employee',
+    Status = 'Status'
+}

-const [sortState, setSortState] = React.useState<{ [key: string]: SortOrder | null }>({
+const [sortState, setSortState] = React.useState<{ [key in SortableColumns]: SortOrder | null }>({

626-630: Add error handling to sort handler

The handleSort function should validate the key parameter to ensure it's a valid sortable column.

Add validation:

 const handleSort = (key: string) => {
+    if (!Object.keys(sortState).includes(key)) {
+        console.error(`Invalid sort key: ${key}`);
+        return;
+    }
     const newOrder = sortState[key] === "ASC" ? "DESC" : "ASC";
     setSortState({ ...sortState, [key]: newOrder });
     onSort(key, newOrder);
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dcee5bd and 3269eaa.

📒 Files selected for processing (2)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx (1 hunks)
  • apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetView.tsx
🔇 Additional comments (1)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)

200-202: ⚠️ Potential issue

Implement sorting logic in handleSort function

The handleSort function currently only logs to the console and doesn't implement actual sorting functionality.

Apply this diff to implement the sorting logic:

 const handleSort = (key: string, order: SortOrder) => {
-    console.log(`Sorting ${key} in ${order} order`);
+    setSorting([{ id: key, desc: order === 'DESC' }]);
 };

Likely invalid or redundant comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ever Teams feature WEB Web app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants