Skip to content

Resolve: 'Individual checkbox on Members Settings table selects all rows instead of one'#558

Merged
antgibson96 merged 1 commit intodevfrom
551-individual-checkbox-on-members-settings-table-selects-all-rows-instead-of-one
Feb 16, 2025
Merged

Resolve: 'Individual checkbox on Members Settings table selects all rows instead of one'#558
antgibson96 merged 1 commit intodevfrom
551-individual-checkbox-on-members-settings-table-selects-all-rows-instead-of-one

Conversation

@JoshRome26
Copy link
Copy Markdown
Contributor

Closes #551

@JoshRome26 JoshRome26 self-assigned this Feb 7, 2025
@JoshRome26 JoshRome26 requested a review from a team as a code owner February 7, 2025 18:19
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 7, 2025

@CodiumAI-Agent /describe

@JoshRome26 JoshRome26 requested review from a team and removed request for a team February 7, 2025 18:19
@QodoAI-Agent
Copy link
Copy Markdown

Title

Resolve: 'Individual checkbox on Members Settings table selects all rows instead of one'


User description

Closes #551


PR Type

Bug fix


Description

  • Fixed individual checkbox selection logic in UserTable.

  • Updated property references from userid to id.

  • Improved code readability and consistency in UserTable.

  • Ensured proper handling of user selection and deselection.


Changes walkthrough 📝

Relevant files
Bug fix
UserTable.tsx
Fix and improve checkbox selection logic in `UserTable`   

packages/client/src/components/settings/UserTable.tsx

  • Corrected logic for individual checkbox selection.
  • Replaced userid references with id for consistency.
  • Enhanced readability and structure of the UserTable component.
  • Ensured proper handling of user selection and deselection.
  • +199/-192

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Feb 7, 2025

    @CodiumAI-Agent /review

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    551 - Partially compliant

    Compliant requirements:

    • Ensure that selecting an individual checkbox on the Members Settings table only selects that specific user.
    • The 'Select All' checkbox should still select all users as expected.
    • Verify that the issue where selecting an individual checkbox selects all rows is resolved.

    Non-compliant requirements:

    []

    Requires further human verification:

    • Verify the behavior in the UI to ensure the fix works as intended.
    • Confirm that no regressions were introduced in the Members Settings table functionality.
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    Ensure that the logic for determining isSelected and updating selectedMembers is functioning correctly and does not introduce any edge cases or regressions.

    {displayedUsers.map((x, i) => {
        const user = displayedUsers[i];
        let isSelected = false;
        if (user) {
            isSelected =
                selectedMembers.some(
                    (value) => {
                        return (
                            value.id ===
                            user.id
                        );
                    },
                );
            if (user) {
                return (
                    <Table.Row
                        key={user.id}
                    >
                        <StyledTableCell
                            size="medium"
                            padding="checkbox"
                        >
                            <StyledCheckbox
                                checked={
                                    isSelected
                                }
                                onChange={() => {
                                    if (
                                        isSelected
                                    ) {
                                        const selMembers =
                                            [];
                                        selectedMembers.forEach(
                                            (
                                                u,
                                            ) => {
                                                if (
                                                    u.id !==
                                                    user.id
                                                )
                                                    selMembers.push(
                                                        u,
                                                    );
                                            },
                                        );
                                        setSelectedMembers(
                                            selMembers,
                                        );
                                    } else {
                                        setSelectedMembers(
                                            [
                                                ...selectedMembers,
                                                user,
                                            ],
                                        );
                                    }
                                }}
                            />
                        </StyledTableCell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <AvatarWrapper>
                                    <Avatar>
                                        {user.name[0].toUpperCase()}
                                    </Avatar>
                                </AvatarWrapper>
                                <Stack
                                    direction={
                                        'column'
                                    }
                                    spacing={
                                        0
                                    }
                                    flex={1}
                                >
                                    <StyledPrimaryText
                                        variant="body1"
                                        noWrap={
                                            true
                                        }
                                        title={`Name: ${user.name}`}
                                    >
                                        {user.name || (
                                            <>
                                                &nbsp;
                                            </>
                                        )}
                                    </StyledPrimaryText>
                                    <Stack
                                        direction={
                                            'row'
                                        }
                                        alignItems={
                                            'center'
                                        }
                                        spacing={
                                            1
                                        }
                                        width={
                                            '150px'
                                        }
                                        title={`Id: ${user.id}`}
                                    >
                                        <StyledSecondaryText
                                            variant="body2"
                                            noWrap={
                                                true
                                            }
                                        >
                                            ID:
                                        </StyledSecondaryText>
                                        <StyledPrimaryText
                                            variant="body2"
                                            noWrap={
                                                true
                                            }
                                        >
                                            {user.id || (
                                                <>
                                                    &nbsp;
                                                </>
                                            )}
                                        </StyledPrimaryText>
                                    </Stack>
                                </Stack>
                            </StyledCenteredBox>
                        </Table.Cell>
                        <Table.Cell>
                            {user.email}
                        </Table.Cell>
                        <Table.Cell>
                            {user.type}
                        </Table.Cell>
                        <Table.Cell>
                            {formatModelLimitValue(
                                user?.model_usage_restriction,
                            )}
                        </Table.Cell>
                        <Table.Cell>
                            {user?.model_usage_restriction ===
                                'compute' &&
                                `${user?.model_max_response_time?.toLocaleString()} ms`}
    
                            {user?.model_usage_restriction ===
                                'token' &&
                                `${user?.model_max_tokens?.toLocaleString()}`}
                        </Table.Cell>
                        <Table.Cell>
                            {formatModelLimitValue(
                                user?.model_usage_frequency,
                            )}
                        </Table.Cell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <Checkbox
                                    label="Publisher"
                                    checked={
                                        user.publisher
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                publisher:
                                                    !user.publisher,
                                            },
                                        );
                                    }}
                                />
                                <Checkbox
                                    label="Exporter"
                                    checked={
                                        user.exporter
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                exporter:
                                                    !user.exporter,
                                            },
                                        );
                                    }}
                                />
                                <Checkbox
                                    label="Admin"
                                    checked={
                                        user.admin
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                admin: !user.admin,
                                            },
                                        );
                                    }}
                                />
                            </StyledCenteredBox>
                        </Table.Cell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <IconButton
                                    onClick={() => {
                                        setAddModalOpen(
                                            true,
                                        );
    
                                        setAddModalUser(
                                            user,
                                        );
                                    }}
                                >
                                    <Edit />
                                </IconButton>
                                <IconButton
                                    onClick={() => {
                                        deleteUser(
                                            user,
                                        );
                                    }}
                                >
                                    <Delete />
                                </IconButton>
                            </StyledCenteredBox>
                        </Table.Cell>
                    </Table.Row>
                );
            }
        }
    
        return null;
    })}
    Code Duplication

    The new implementation appears to duplicate a significant amount of logic from the removed code. Consider refactoring to reduce redundancy and improve maintainability.

    {displayedUsers.map((x, i) => {
        const user = displayedUsers[i];
        let isSelected = false;
        if (user) {
            isSelected =
                selectedMembers.some(
                    (value) => {
                        return (
                            value.id ===
                            user.id
                        );
                    },
                );
            if (user) {
                return (
                    <Table.Row
                        key={user.id}
                    >
                        <StyledTableCell
                            size="medium"
                            padding="checkbox"
                        >
                            <StyledCheckbox
                                checked={
                                    isSelected
                                }
                                onChange={() => {
                                    if (
                                        isSelected
                                    ) {
                                        const selMembers =
                                            [];
                                        selectedMembers.forEach(
                                            (
                                                u,
                                            ) => {
                                                if (
                                                    u.id !==
                                                    user.id
                                                )
                                                    selMembers.push(
                                                        u,
                                                    );
                                            },
                                        );
                                        setSelectedMembers(
                                            selMembers,
                                        );
                                    } else {
                                        setSelectedMembers(
                                            [
                                                ...selectedMembers,
                                                user,
                                            ],
                                        );
                                    }
                                }}
                            />
                        </StyledTableCell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <AvatarWrapper>
                                    <Avatar>
                                        {user.name[0].toUpperCase()}
                                    </Avatar>
                                </AvatarWrapper>
                                <Stack
                                    direction={
                                        'column'
                                    }
                                    spacing={
                                        0
                                    }
                                    flex={1}
                                >
                                    <StyledPrimaryText
                                        variant="body1"
                                        noWrap={
                                            true
                                        }
                                        title={`Name: ${user.name}`}
                                    >
                                        {user.name || (
                                            <>
                                                &nbsp;
                                            </>
                                        )}
                                    </StyledPrimaryText>
                                    <Stack
                                        direction={
                                            'row'
                                        }
                                        alignItems={
                                            'center'
                                        }
                                        spacing={
                                            1
                                        }
                                        width={
                                            '150px'
                                        }
                                        title={`Id: ${user.id}`}
                                    >
                                        <StyledSecondaryText
                                            variant="body2"
                                            noWrap={
                                                true
                                            }
                                        >
                                            ID:
                                        </StyledSecondaryText>
                                        <StyledPrimaryText
                                            variant="body2"
                                            noWrap={
                                                true
                                            }
                                        >
                                            {user.id || (
                                                <>
                                                    &nbsp;
                                                </>
                                            )}
                                        </StyledPrimaryText>
                                    </Stack>
                                </Stack>
                            </StyledCenteredBox>
                        </Table.Cell>
                        <Table.Cell>
                            {user.email}
                        </Table.Cell>
                        <Table.Cell>
                            {user.type}
                        </Table.Cell>
                        <Table.Cell>
                            {formatModelLimitValue(
                                user?.model_usage_restriction,
                            )}
                        </Table.Cell>
                        <Table.Cell>
                            {user?.model_usage_restriction ===
                                'compute' &&
                                `${user?.model_max_response_time?.toLocaleString()} ms`}
    
                            {user?.model_usage_restriction ===
                                'token' &&
                                `${user?.model_max_tokens?.toLocaleString()}`}
                        </Table.Cell>
                        <Table.Cell>
                            {formatModelLimitValue(
                                user?.model_usage_frequency,
                            )}
                        </Table.Cell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <Checkbox
                                    label="Publisher"
                                    checked={
                                        user.publisher
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                publisher:
                                                    !user.publisher,
                                            },
                                        );
                                    }}
                                />
                                <Checkbox
                                    label="Exporter"
                                    checked={
                                        user.exporter
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                exporter:
                                                    !user.exporter,
                                            },
                                        );
                                    }}
                                />
                                <Checkbox
                                    label="Admin"
                                    checked={
                                        user.admin
                                    }
                                    onChange={() => {
                                        updateUser(
                                            {
                                                ...user,
                                                admin: !user.admin,
                                            },
                                        );
                                    }}
                                />
                            </StyledCenteredBox>
                        </Table.Cell>
                        <Table.Cell>
                            <StyledCenteredBox>
                                <IconButton
                                    onClick={() => {
                                        setAddModalOpen(
                                            true,
                                        );
    
                                        setAddModalUser(
                                            user,
                                        );
                                    }}
                                >
                                    <Edit />
                                </IconButton>
                                <IconButton
                                    onClick={() => {
                                        deleteUser(
                                            user,
                                        );
                                    }}
                                >
                                    <Delete />
                                </IconButton>
                            </StyledCenteredBox>
                        </Table.Cell>
                    </Table.Row>
                );
            }
        }
    
        return null;
    })}

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Feb 7, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate user properties before access

    Ensure that the user.name and user.id properties are properly validated or checked
    for null/undefined values before accessing them to avoid potential runtime errors.

    packages/client/src/components/settings/UserTable.tsx [613]

    -{user.name[0].toUpperCase()}
    +{user.name ? user.name[0].toUpperCase() : ''}
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion addresses a potential runtime error by ensuring user.name is validated before accessing its first character. This is crucial for robustness, especially since user data might be incomplete or malformed.

    High
    Add null check for user object

    Add a null/undefined check for user before accessing its properties to prevent
    potential runtime errors when user is not defined.

    packages/client/src/components/settings/UserTable.tsx [690-692]

    -{user?.model_usage_restriction === 'compute' && `${user?.model_max_response_time?.toLocaleString()} ms`}
    +{user && user.model_usage_restriction === 'compute' && `${user.model_max_response_time?.toLocaleString()} ms`}
    Suggestion importance[1-10]: 8

    __

    Why: Adding a null check for the user object ensures that the code does not throw runtime errors when user is undefined. This is a valid and important improvement for handling edge cases.

    Medium
    General
    Optimize checkbox change handler logic

    Optimize the onChange handler for the StyledCheckbox to avoid creating a new array
    and function on every render, which can impact performance.

    packages/client/src/components/settings/UserTable.tsx [577-605]

     onChange={() => {
    -    if (isSelected) {
    -        const selMembers = [];
    -        selectedMembers.forEach((u) => {
    -            if (u.id !== user.id) selMembers.push(u);
    -        });
    -        setSelectedMembers(selMembers);
    -    } else {
    -        setSelectedMembers([...selectedMembers, user]);
    -    }
    +    setSelectedMembers((prev) =>
    +        isSelected
    +            ? prev.filter((u) => u.id !== user.id)
    +            : [...prev, user]
    +    );
     }}
    Suggestion importance[1-10]: 7

    __

    Why: The optimization reduces unnecessary array creation and improves performance by using a functional state update. While not critical, it enhances efficiency and readability.

    Medium
    Handle errors in updateUser function

    Ensure that the updateUser function is properly handling asynchronous updates or
    errors when toggling user roles to avoid inconsistent state.

    packages/client/src/components/settings/UserTable.tsx [711-715]

     updateUser({
         ...user,
         publisher: !user.publisher,
    +}).catch((error) => {
    +    console.error('Failed to update user:', error);
     });
    Suggestion importance[1-10]: 6

    __

    Why: Adding error handling to the updateUser function improves reliability by ensuring that failures are logged and can be addressed. However, the suggestion could be more impactful if it included user feedback or retry mechanisms.

    Low

    @antgibson96 antgibson96 merged commit e7ec9b9 into dev Feb 16, 2025
    @antgibson96 antgibson96 deleted the 551-individual-checkbox-on-members-settings-table-selects-all-rows-instead-of-one branch February 16, 2025 22:22
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-02-16

    Fixed

    • Resolved an issue where individual checkboxes in the Members Settings table incorrectly selected all rows instead of one.

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Individual checkbox on Members Settings table selects all rows instead of one

    3 participants