Skip to content

feat(client): deleting_app_redirects_to_new_app_library_ui#1246

Merged
anurag91jain merged 5 commits intodevfrom
Deleting-apps-redirects-to-new-App-Library-UI
Jun 5, 2025
Merged

feat(client): deleting_app_redirects_to_new_app_library_ui#1246
anurag91jain merged 5 commits intodevfrom
Deleting-apps-redirects-to-new-App-Library-UI

Conversation

@aditiiisinhaa
Copy link
Copy Markdown
Contributor

Description

On deleting app from App Library, it redirects to the new App Library UI instead of the old UI.

Changes Made

When user delete app from app library user is redirected to the new UI of App Library which is earlier been redirected to the old UI i.e. App Settings folder view from Settings. Also if user delete app from App setting then it is redirected to the folder view of App Settings only.

How to Test

For App Library:
App Library -> Select App -> Edit -> Settings -> delete

For App Settings:
Open Settings -> App Settings -> Select App -> Delete

@aditiiisinhaa aditiiisinhaa requested review from a team, AAfghahi, anurag91jain and johbaxter June 3, 2025 07:33
@aditiiisinhaa aditiiisinhaa self-assigned this Jun 3, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 3, 2025

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

feat(client): deleting_app_redirects_to_new_app_library_ui


User description

Description

On deleting app from App Library, it redirects to the new App Library UI instead of the old UI.

Changes Made

When user delete app from app library user is redirected to the new UI of App Library which is earlier been redirected to the old UI i.e. App Settings folder view from Settings. Also if user delete app from App setting then it is redirected to the folder view of App Settings only.

How to Test

For App Library:
App Library -> Select App -> Edit -> Settings -> delete

For App Settings:
Open Settings -> App Settings -> Select App -> Delete


PR Type

Enhancement


Description

  • Add conditional redirect on app deletion

  • Redirect to /settings/app from app settings

  • Redirect to / when deleting from App Library


Changes walkthrough 📝

Relevant files
Enhancement
SettingsPanel.tsx
Conditional navigation on delete                                                 

packages/client/src/components/workspace/panels/SettingsPanel.tsx

  • Extended onDelete handler with path check
  • If path starts with /settings/app/, navigate to /settings/app
  • Otherwise, navigate to /
  • +11/-1   

    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 Jun 3, 2025

    @CodiumAI-Agent /review

    @QodoAI-Agent
    Copy link
    Copy Markdown

    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

    Path Matching Edge Case

    The startsWith('/settings/app/') check will not match the exact /settings/app route (no trailing slash), causing deletes on that path to redirect to the App Library instead of App Settings.

    if (
        location.pathname.startsWith(
            '/settings/app/',
        )
    ) {
    Undefined `location`

    The code uses location.pathname directly; verify that location comes from React Router's useLocation() or explicitly from window.location, otherwise location may be undefined.

    if (
        location.pathname.startsWith(
            '/settings/app/',
        )

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Jun 3, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    QodoAI-Agent commented Jun 3, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 40734bd

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Broaden path matching

    Expand the path check to include the exact /settings/app route by matching on
    /settings/app instead of /settings/app/, which also covers URLs without a trailing
    slash.

    packages/client/src/components/workspace/panels/SettingsPanel.tsx [138-148]

    -if (
    -    location.pathname.startsWith(
    -        '/settings/app/',
    -    )
    -) {
    -    // If in app settings
    +if (location.pathname.startsWith('/settings/app')) {
         navigate('/settings/app');
     } else {
    -    // If in App Library
         navigate('/');
     }
    Suggestion importance[1-10]: 7

    __

    Why: The original startsWith('/settings/app/') misses the /settings/app route without a trailing slash, so broadening to '/settings/app' ensures correct navigation across both cases.

    Medium
    General
    Simplify navigation logic

    Simplify the deletion navigation logic by using a single-line conditional expression
    to determine the target path.

    packages/client/src/components/workspace/panels/SettingsPanel.tsx [137-149]

    -onDelete={() => {
    -    if (
    -        location.pathname.startsWith(
    -            '/settings/app/',
    -        )
    -    ) {
    -        // If in app settings
    -        navigate('/settings/app');
    -    } else {
    -        // If in App Library
    -        navigate('/');
    -    }
    -}}
    +onDelete={() =>
    +    navigate(
    +        location.pathname.startsWith('/settings/app')
    +            ? '/settings/app'
    +            : '/'
    +    )
    +}
    Suggestion importance[1-10]: 6

    __

    Why: Converting the if block to a single-line ternary inside navigate(...) keeps the same logic while improving readability and reducing boilerplate.

    Low

    Previous suggestions

    Suggestions up to commit 40734bd
    CategorySuggestion                                                                                                                                    Impact
    General
    Prevent extra history entries

    Use the replace: true option when calling navigate to prevent adding extra entries
    to the browser history stack during redirection.

    packages/client/src/components/workspace/panels/SettingsPanel.tsx [144-147]

    -navigate('/settings/app');
    +navigate('/settings/app', { replace: true });
     ...
    -navigate('/');
    +navigate('/', { replace: true });
    Suggestion importance[1-10]: 6

    __

    Why: Adding { replace: true } to navigate calls avoids cluttering the history stack, improving the user experience without altering functionality.

    Low
    Possible issue
    Use window.location explicitly

    Replace the bare location reference with window.location to avoid a potential
    undefined variable error in React. This ensures the code consistently uses the
    browser’s global location object.

    packages/client/src/components/workspace/panels/SettingsPanel.tsx [138-142]

     if (
    -    location.pathname.startsWith(
    +    window.location.pathname.startsWith(
             '/settings/app/',
         )
     ) {
    Suggestion importance[1-10]: 4

    __

    Why: Using window.location prevents potential undefined location reference and makes the global object explicit, but this is a minor stylistic change.

    Low

    @aditiiisinhaa
    Copy link
    Copy Markdown
    Contributor Author

    Here are the screenshots:

    If in App Settings:

    AppSettings

    If in App Library:

    AppLibrary

    @anurag91jain anurag91jain merged commit f5e8649 into dev Jun 5, 2025
    3 checks passed
    @anurag91jain anurag91jain deleted the Deleting-apps-redirects-to-new-App-Library-UI branch June 5, 2025 19:30
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Jun 5, 2025

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-06-05 *

    Changed

    • Redirect to new App Library UI after deleting an app.
    • Keep deletion redirect for App Settings in the folder view.

    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.

    [BUG] Deleting app redirects to old UI instead of new App Library UI

    5 participants