Skip to content

562 sorting on model settings#679

Merged
AAfghahi merged 3 commits intodevfrom
562-sorting-on-model-settings
Apr 4, 2025
Merged

562 sorting on model settings#679
AAfghahi merged 3 commits intodevfrom
562-sorting-on-model-settings

Conversation

@memisrose
Copy link
Copy Markdown
Contributor

Description

Bug fix for Settings pages sorting options.

Changes Made

Adding in sort options to the MyEngines reactor. Changing style and adding tooltips for sorting/views.

How to Test

  1. Sort by Name
  2. Sort By Date Created

Notes

pending merge of BE to test
SEMOSS/Semoss#479

@memisrose memisrose requested a review from a team as a code owner March 7, 2025 16:34
@memisrose memisrose linked an issue Mar 7, 2025 that may be closed by this pull request
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 7, 2025

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

562 sorting on model settings


User description

Description

Bug fix for Settings pages sorting options.

Changes Made

Adding in sort options to the MyEngines reactor. Changing style and adding tooltips for sorting/views.

How to Test

  1. Sort by Name
  2. Sort By Date Created

Notes

pending merge of BE to test
SEMOSS/Semoss#479


PR Type

  • Enhancement

Description

  • Updated sort state initialization value

  • Added sort order state with ASC/DESC toggle

  • Introduced tooltips for sorting and view icons

  • Modified API query to include sort parameters


Changes walkthrough 📝

Relevant files
Enhancement
EngineSettingsIndexPage.tsx
Updated sorting options and added tooltips                             

packages/client/src/pages/settings/EngineSettingsIndexPage.tsx

  • Changed sort default from 'Name' to 'ENGINENAME'
  • Introduced sortOrder state with toggle buttons
  • Wrapped icons in Tooltip components
  • Updated query and dependency list for sorting
  • +48/-10 

    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 Mar 7, 2025

    @CodiumAI-Agent /review

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Mar 7, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Dependency Array

    The useEffect hook updated on line 188 now depends on sort but does not include sortOrder even though sortOrder is used in the API query. Please verify if sortOrder should also be added to the dependency array to ensure state consistency.

    }, [adminMode, search, sort]);
    Event Handling

    The onClick handlers for the ToggleButton components (lines 439–456) use the signature (e, v) => setSortOrder(v). Ensure these callbacks correctly capture the intended new value and consider if using an onChange handler might improve clarity and consistency with other input handling.

    <ToggleButton
        onClick={(e, v) => setSortOrder(v)}
        value={'DESC'}
        aria-label={'Descending Order'}
    >
        <Tooltip title={'Descending Order'}>
            <ArrowDownward />
        </Tooltip>
    </ToggleButton>
    <ToggleButton
        onClick={(e, v) => setSortOrder(v)}
        value={'ASC'}
        aria-label={'Ascending Order'}
    >
        <Tooltip title={'Ascending Order'}>
            <ArrowUpward />
        </Tooltip>
    </ToggleButton>

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Update dependency array

    Add the sortOrder state to the dependency array to ensure state updates trigger
    re-execution of the effect.

    packages/client/src/pages/settings/EngineSettingsIndexPage.tsx [188]

    -}, [adminMode, search, sort]);
    +}, [adminMode, search, sort, sortOrder]);
    Suggestion importance[1-10]: 8

    __

    Why: Adding sortOrder to the dependency array ensures that state changes trigger the necessary effect, which is a critical fix for keeping the view in sync; this adjustment directly addresses a potential bug in state management.

    Medium
    Use group onChange

    Replace individual onClick handlers with a single onChange on the ToggleButtonGroup
    to correctly capture the selected value.

    packages/client/src/pages/settings/EngineSettingsIndexPage.tsx [434-457]

     <ToggleButtonGroup
         size={'small'}
         value={sortOrder}
         color="primary"
    +    onChange={(e, newOrder) => newOrder && setSortOrder(newOrder)}
     >
    -    <ToggleButton
    -        onClick={(e, v) => setSortOrder(v)}
    -        value={'DESC'}
    -        aria-label={'Descending Order'}
    -    >
    +    <ToggleButton value={'DESC'} aria-label={'Descending Order'}>
             <Tooltip title={'Descending Order'}>
                 <ArrowDownward />
             </Tooltip>
         </ToggleButton>
    -    <ToggleButton
    -        onClick={(e, v) => setSortOrder(v)}
    -        value={'ASC'}
    -        aria-label={'Ascending Order'}
    -    >
    +    <ToggleButton value={'ASC'} aria-label={'Ascending Order'}>
             <Tooltip title={'Ascending Order'}>
                 <ArrowUpward />
             </Tooltip>
         </ToggleButton>
     </ToggleButtonGroup>
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion proposes consolidating individual onClick handlers into an onChange handler which improves conciseness and consistency, though it may slightly alter event handling semantics; the overall impact is moderate.

    Low
    Consolidate toggle events

    Modify the view toggle group to use an onChange handler on the ToggleButtonGroup for
    consistent state updates.

    packages/client/src/pages/settings/EngineSettingsIndexPage.tsx [459-480]

     <ToggleButtonGroup
         size={'small'}
         value={view}
         color="primary"
    +    onChange={(e, newView) => newView && setView(newView)}
     >
    -    <ToggleButton onClick={(e, v) => setView(v)} value={'tile'}>
    +    <ToggleButton value={'tile'} aria-label={'Tile View'}>
             <Tooltip title={'Tile View'}>
                 <SpaceDashboardOutlined />
             </Tooltip>
         </ToggleButton>
    -    <ToggleButton onClick={(e, v) => setView(v)} value={'list'}>
    +    <ToggleButton value={'list'} aria-label={'List View'}>
             <Tooltip title={'List View'}>
                 <FormatListBulletedOutlined />
             </Tooltip>
         </ToggleButton>
     </ToggleButtonGroup>
    Suggestion importance[1-10]: 6

    __

    Why: Similar to suggestion 1, this change streamlines event handling for view toggles by moving to an onChange handler; it is a stylistic improvement with moderate benefit assuming the ToggleButtonGroup supports such handling.

    Low

    @johbaxter johbaxter requested a review from AAfghahi April 4, 2025 18:13
    @AAfghahi AAfghahi merged commit 66b5175 into dev Apr 4, 2025
    4 checks passed
    @AAfghahi AAfghahi deleted the 562-sorting-on-model-settings branch April 4, 2025 18:55
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Apr 4, 2025

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-04-04

    Fixed

    • Corrected sorting functionality on settings pages.

    Changed

    • Updated sort options and enhanced UI tooltips.

    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.

    Sorting on Model Settings

    3 participants