fix: fixed back button to redirect to applications page #35900
Conversation
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UserProfile
participant Settings
participant Router
User->>UserProfile: Click "Back"
UserProfile->>Router: Call history.push("/applications")
Router-->>UserProfile: Navigate to applications page
User->>Settings: Click "Back"
Settings->>Router: Call history.push("/workspace")
Router-->>Settings: Navigate to workspace page
Poem
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
Early access features: disabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/client/src/pages/UserProfile/index.test.tsx (3 hunks)
- app/client/src/pages/UserProfile/index.tsx (1 hunks)
- app/client/src/pages/workspace/tests/settings.test.tsx (3 hunks)
- app/client/src/pages/workspace/settings.tsx (1 hunks)
Additional comments not posted (11)
app/client/src/pages/UserProfile/index.tsx (1)
59-59: Good job!The
goToprop is correctly added to theBackButtonto redirect users to the/applicationspage. This change aligns with the PR objective to improve navigation.The code changes are approved.
app/client/src/pages/UserProfile/index.test.tsx (3)
3-3: Nice addition!The
screenandfireEventimports from@testing-library/reactare correctly added to facilitate the new test case.The code changes are approved.
95-102: Well done!The mocked
useHistoryhook is correctly implemented to simulate navigation actions within the tests.The code changes are approved.
145-159: Excellent test case!The new test case is correctly implemented to verify that clicking the "Back" button triggers a call to
history.pushwith the expected path (/applications). This enhances the test coverage.The code changes are approved.
app/client/src/pages/workspace/settings.tsx (1)
130-130: Great enhancement!The
goToprop is correctly added to theBackButtonto dynamically construct a URL using theworkspaceId. This change enhances the navigation functionality by providing a direct link that is contextually relevant to the user's current settings.The code changes are approved.
app/client/src/pages/workspace/__tests__/settings.test.tsx (6)
4-4: Good job importingBrowserRouter!This ensures that the routing functionality is properly tested.
This change is approved.
6-10: Great addition offireEvent!This enables interaction simulation within the tests, which is crucial for testing user interactions.
This change is approved.
13-14: Nice use ofredux-mock-store!This helps in creating a mock store for testing purposes.
This change is approved.
162-163: Good introduction ofmockHistoryPush!This mock function is essential for simulating navigation in your tests.
This change is approved.
164-165: Well done configuring the mock store!This is necessary for providing a mock Redux store to the component during tests.
This change is approved.
169-171: Excellent job mockinguseHistory!This ensures that the
pushfunction is correctly tested.This change is approved.
| it("should redirect to workspace page if user wants to go back", () => { | ||
| testRender( | ||
| <BrowserRouter> | ||
| <Provider store={mockStore(mockWorkspaceData)}> | ||
| <Settings /> | ||
| </Provider> | ||
| </BrowserRouter>, | ||
| ); | ||
| const backBtn = testScreen.getByText("Back"); | ||
| fireEvent.click(backBtn); | ||
| expect(mockHistoryPush).toHaveBeenCalledWith( | ||
| `/applications?workspaceId=${mockWorkspaceData.id}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Great addition of the back button test case!
This test case ensures that the back button redirects to the correct page, improving the robustness of your tests. However, consider adding a comment to explain the purpose of this test case for future maintainers.
Add a comment to explain the purpose of this test case:
it("should redirect to workspace page if user wants to go back", () => {
+ // This test case ensures that clicking the back button redirects to the workspace page.
testRender(
<BrowserRouter>
<Provider store={mockStore(mockWorkspaceData)}>
<Settings />
</Provider>
</BrowserRouter>,
);
const backBtn = testScreen.getByText("Back");
fireEvent.click(backBtn);
expect(mockHistoryPush).toHaveBeenCalledWith(
`/applications?workspaceId=${mockWorkspaceData.id}`,
);
});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should redirect to workspace page if user wants to go back", () => { | |
| testRender( | |
| <BrowserRouter> | |
| <Provider store={mockStore(mockWorkspaceData)}> | |
| <Settings /> | |
| </Provider> | |
| </BrowserRouter>, | |
| ); | |
| const backBtn = testScreen.getByText("Back"); | |
| fireEvent.click(backBtn); | |
| expect(mockHistoryPush).toHaveBeenCalledWith( | |
| `/applications?workspaceId=${mockWorkspaceData.id}`, | |
| ); | |
| }); | |
| it("should redirect to workspace page if user wants to go back", () => { | |
| // This test case ensures that clicking the back button redirects to the workspace page. | |
| testRender( | |
| <BrowserRouter> | |
| <Provider store={mockStore(mockWorkspaceData)}> | |
| <Settings /> | |
| </Provider> | |
| </BrowserRouter>, | |
| ); | |
| const backBtn = testScreen.getByText("Back"); | |
| fireEvent.click(backBtn); | |
| expect(mockHistoryPush).toHaveBeenCalledWith( | |
| `/applications?workspaceId=${mockWorkspaceData.id}`, | |
| ); | |
| }); |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10591004329. |
|
@saicharan-zemoso Could you please check and fix the checks that are failing? You will be able to see the errors when you click "Details" for the specific check. |
|
Deploy-Preview-URL: https://ce-35900.dp.appsmith.com |
|
Ok I will check why they are failing |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/pages/UserProfile/index.test.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/UserProfile/index.test.tsx
|
@saicharan-zemoso There are still some failures in prettier and lint checks. |
|
Hello @ankitakinger , and is it ok to add a line to make prettier ignore this Or is there any other way to solve this as we cannot add a "." in that line. |
|
@saicharan-zemoso It's actually not a dot but a single space. If you are using VSCode as your editor, you can install "ESLint" and "Prettier - Code formatter" extensions and that will take care of fixing these errors. If you're using any other editor, I'm sure there would be a way to get this done automatically on save, you'll just have to search for the same. The space there needs to be added before |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/pages/UserProfile/index.tsx (1 hunks)
- app/client/src/pages/workspace/settings.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/UserProfile/index.tsx
- app/client/src/pages/workspace/settings.tsx
…)" This reverts commit 420dc9b.
…e" (#36191) Reverts #35900 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10778653732> > Commit: b0516d0 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10778653732&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Mon, 09 Sep 2024 18:26:51 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Simplified navigation with the `BackButton` component, now relying on default behavior instead of a specified route in the `UserProfile` and `Settings` components. - **Bug Fixes** - Refined testing strategy by removing outdated navigation-related tests and imports for the `UserProfile` and `Settings` components, streamlining the test suite. - **Documentation** - Updated comments and documentation to reflect changes in navigation behavior and testing focus. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
added the changes to redirect to application pages instead of going back in the history.
issue: #34431
fixed the navigation to not go back in history
The navigation of the back button used to go back in history. Now it will be redirected to the expected page.
added the test for profile page:
added the tests for settings page:
video:
back.button.fix.mp4
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10659602683
Commit: 23f2285
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 02 Sep 2024 04:52:56 UTC
Summary by CodeRabbit
New Features
Bug Fixes
Tests