Skip to content

feat(client): after adding member, admin doesn't turn off #1208

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

Merged
merged 1 commit into from
May 28, 2025

Conversation

pallabi2303
Copy link
Contributor

Description

feat(client): after adding member, admin doesn't turn off

Changes Made

After Adding user, the page doesn't get refreshed automatically. So, 'Admin On' remains as it is.

How to Test

  1. Log in as an Admin.
  2. Navigate to the Members settings page for an app or engine that user don't have an access for.
  3. On 'Admin On', Add Member.
  4. User can see that after Member being added Admin remains on.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pallabi2303 pallabi2303 self-assigned this May 23, 2025
@pallabi2303 pallabi2303 requested a review from a team as a code owner May 23, 2025 11:00
Copy link

@CodiumAI-Agent /describe

@CodiumAI-Agent
Copy link

Title

feat(client): after adding member, admin doesn't turn off


User description

Description

feat(client): after adding member, admin doesn't turn off

Changes Made

After Adding user, the page doesn't get refreshed automatically. So, 'Admin On' remains as it is.

How to Test

  1. Log in as an Admin.
  2. Navigate to the Members settings page for an app or engine that user don't have an access for.
  3. On 'Admin On', Add Member.
  4. User can see that after Member being added Admin remains on.

PR Type

Bug fix


Description

  • Deleted window.location.reload on success

  • Ensures overlay closes without refresh


Changes walkthrough 📝

Relevant files
Bug fix
MembersAddOverlay.tsx
Remove page reload logic after adding member                         

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

  • Removed window.location.reload in finally block
  • Streamlined overlay close without page refresh
  • +0/-4     

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

    @CodiumAI-Agent /review

    @CodiumAI-Agent
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Missing UI refresh

    The auto-reload after successfully adding a member was removed, but there is no alternative state update or data fetching to refresh the members list or admin status in the UI.

        } finally {
            // close the overlay
            closeOverlay(type, success);
        }
    };
    Stale admin status

    Without reloading or updating the component state, the 'Admin On' label may remain unchanged and confuse users about current permissions.

        } finally {
            // close the overlay
            closeOverlay(type, success);
        }
    };

    Copy link

    @CodiumAI-Agent /improve

    @CodiumAI-Agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    General
    Trigger member list refresh

    Instead of reloading the entire window to show updated by/timestamps, call a
    member‐list refetch or update function after closing the overlay. This will refresh
    just the data you need without a full page reload.

    packages/client/src/components/settings/MembersAddOverlay.tsx [471]

     closeOverlay(type, success);
    +props.refetchMembers();
    Suggestion importance[1-10]: 7

    __

    Why: Adding a targeted props.refetchMembers() avoids a full page reload and efficiently updates the member list, improving UX and performance without major risk.

    Medium
    Reset admin toggle state

    Reset the admin toggle state after a successful add so that the switch isn’t left on
    for the next addition. Call your state setter (e.g. setIsAdmin(false)) immediately
    after closing.

    packages/client/src/components/settings/MembersAddOverlay.tsx [471]

     closeOverlay(type, success);
    +setIsAdmin(false);
    Suggestion importance[1-10]: 5

    __

    Why: Clearing the admin toggle with setIsAdmin(false) prevents unintended state carry-over, but it’s a minor UI convenience and not critical to core functionality.

    Low

    @pallabi2303
    Copy link
    Contributor Author

    @ishumita
    You mentioned you have added the refresh so that the updated by and timestamp show up, but even if we don't refresh the page it is already behaving like that. So, I have removed the refresh as the Admin was turning off because of it, and we were getting error.

    @CodiumAI-Agent
    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @pallabi2303 pallabi2303 linked an issue May 23, 2025 that may be closed by this pull request
    3 tasks
    @ishumita
    Copy link
    Contributor

    @pallabi2303 sure, thank you for letting me know:)

    @johbaxter johbaxter merged commit b0ed3d3 into dev May 28, 2025
    4 checks passed
    @johbaxter johbaxter deleted the AddMember-AdminOffIssue branch May 28, 2025 20:32
    Copy link

    @CodiumAI-Agent /update_changelog

    @CodiumAI-Agent
    Copy link

    Changelog updates: 🔄

    2025-05-28 *

    Fixed

    • Stop automatic page reload after adding a member so the admin toggle state persists.

    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.

    After adding user, admin turns off
    6 participants