fix: App card routing #38798
fix: App card routing #38798
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/pages/Applications/ApplicationCard.tsx (3)
480-489: Add TypeScript event type annotation.The event parameter should be properly typed for better type safety.
- const launchApp = useCallback( - (e) => { + const launchApp = useCallback( + (e: React.MouseEvent<HTMLElement>) => {
491-500: Add TypeScript event type annotation.The event parameter should be properly typed for better type safety.
- const editApp = useCallback( - (e) => { + const editApp = useCallback( + (e: React.MouseEvent<HTMLElement>) => {
502-511: Optimize dependencies and add type annotation.
- The dependency array includes redundant dependencies that are already covered by viewModeURL.
- The event parameter should be properly typed.
- const launchMobileApp = useCallback( - (e) => { + const launchMobileApp = useCallback( + (e: React.MouseEvent<HTMLElement>) => { e.preventDefault(); e.stopPropagation(); setURLParams(); history.push(viewModeURL); dispatch(getCurrentUser()); }, - [dispatch, props.application.defaultBasePageId, setURLParams, viewerParams], + [dispatch, viewModeURL, setURLParams],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Applications/ApplicationCard.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
480-511: Implementation effectively addresses the remounting issue.The changes successfully prevent default navigation behavior and implement controlled routing through history.push, which should resolve the application remounting issue.
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12922764782. |
|
Deploy-Preview-URL: https://ce-38798.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
211-227: Consider memoizing permission checks.These permission checks are computed on every render. Consider using
useMemoto optimize performance.- const hasEditPermission = isPermitted( - application.userPermissions ?? [], - PERMISSION_TYPE.MANAGE_APPLICATION, - ); + const hasEditPermission = useMemo(() => + isPermitted( + application.userPermissions ?? [], + PERMISSION_TYPE.MANAGE_APPLICATION, + ), + [application.userPermissions] + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Applications/ApplicationCard.tsx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (6)
app/client/src/pages/Applications/ApplicationCard.tsx (6)
79-90: LGTM! Well-structured styles using design system variables.
165-172: LGTM! Proper color initialization with fallback.
445-470: LGTM! Well-structured URL params management with proper dependencies.
488-533: New tab functionality is enhanced, not removed.Contrary to the previous review comment, this change actually enhances the new tab functionality by adding support for Ctrl/Cmd+Click. This is a more standard approach that aligns with user expectations.
551-552: LGTM! Proper test ID updates.
313-314: Handle potential edge case in initials generation.The current implementation assumes the application name has at least one character. Consider adding a length check before accessing the second character.
- if (initials.length < 2 && application.name.length > 1) { - initials += application.name[1].toUpperCase() || ""; + if (initials.length < 2 && application.name?.length > 1) { + initials += application.name[1]?.toUpperCase() || "";
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12925984329. |
|
Deploy-Preview-URL: https://ce-38798.dp.appsmith.com |
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/cypress/support/WorkspaceCommands.js (1)
66-66: Validate locator usage for best practices.
While asserting element visibility here is helpful, confirm thatappSettings.locators._applicationNamereferences a data-* attribute rather than a CSS or XPath selector. Adhering to data-* attributes aligns with Cypress best practices for stable element targeting.app/client/cypress/support/Pages/HomePage.ts (4)
135-135: Consider verifying the active Apps tab for stability.
A quick assertion after navigating can confirm the tab switch and guard against flaky tests.public SwitchToAppsTab() { this.agHelper.GetNClick(this._homeTab); + this.agHelper.AssertElementVisibility(".t--applications-container"); }
328-328: Use data-test attributes for targeted element visibility checks.
Replacing classes with data-* attributes often yields more robust tests.
802-802: Avoid forcing clicks if not necessary.
“force: true” bypasses Cypress’s actionability checks and can mask real UI issues. Use it only when absolutely needed.
812-816: Offer a method to select an application by name alongside index.
Index-based selection can be brittle; consider adding a companion method that locates the app by name or data-test attribute.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/support/Pages/HomePage.ts(5 hunks)app/client/cypress/support/WorkspaceCommands.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/WorkspaceCommands.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/HomePage.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/cypress/support/WorkspaceCommands.js (1)
15-15: Solid introduction for referencing app settings.
This new constant centralizes the usage of application settings, maintaining better consistency with other helper objects.app/client/cypress/support/Pages/HomePage.ts (2)
6-6: No relevant code change here.
811-811: No meaningful code change here.
|
@hetunandu Is it correct to assume that the entire page was re-rendering because we were passing the href to the "Edit" and "Launch" CTAs on the Application card? |
|
@dvj1988 is correct here @vsvamsi1 href causes the React Router / Browser to load the App.tsx to mount again instead just the sub route. I am preventing default unless the user attempts to open the App in a new tab / window cause that anyways needs a full App mount. there still is a href passed which makes it a link. I think this helps in browser understanding it is a link but if needed we can remove that as well. |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12984045188. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
445-470: Consider simplifying URL params dependency arrayThe current implementation has a large dependency array that could potentially cause unnecessary re-renders.
Consider extracting the page-related data into a useMemo:
+const defaultPage = useMemo( + () => application.pages.find((page) => page.id === application.defaultPageId), + [application.pages, application.defaultPageId] +); const setURLParams = useCallback(() => { - const page: ApplicationPagePayload | undefined = application.pages.find( - (page) => page.id === application.defaultPageId, - ); + if (!defaultPage) return; urlBuilder.updateURLParams( { applicationSlug: application.slug, applicationVersion: application.applicationVersion, baseApplicationId: application.baseId, }, application.pages.map((page) => ({ pageSlug: page.slug, customSlug: page.customSlug, basePageId: page.baseId, })), ); -}, [ - application.applicationVersion, - application.baseId, - application.defaultPageId, - application.pages, - application.slug, -]); +}, [application, defaultPage]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Applications/ApplicationCard.tsx(12 hunks)app/client/src/pages/Editor/AppsmithLink.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (5)
app/client/src/pages/Applications/ApplicationCard.tsx (3)
79-90: LGTM: Icon styling follows design system guidelinesThe styling implementation correctly uses CSS variables for theming and maintains consistency with the design system.
Line range hint
104-128: LGTM: Improved prop access patternDirect access to the application prop with proper type safety through optional chaining improves code readability and reliability.
488-516: LGTM: Improved navigation handling prevents app remountingThe implementation now correctly:
- Prevents app remounting by using React Router for navigation
- Maintains new tab functionality through ctrl/cmd + click
- Sets URL params before navigation
Let's verify that all application card click handlers are consistently implemented:
✅ Verification successful
✅ Click handlers are consistently implemented
Both Edit and Launch buttons in ApplicationCard.tsx follow the same navigation pattern, with proper handling of ctrl/cmd + click and React Router navigation. No other click handlers require updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for click handlers in application card components ast-grep --pattern 'onClick={$_}'Length of output: 35
Script:
#!/bin/bash # First, find all application card related files echo "=== Finding Application Card files ===" fd -e tsx -e ts -e js "Application.*Card" echo -e "\n=== Searching for click handlers ===" # Search for onClick handlers with context rg -U "onClick={.*?}" -g "*Application*Card*" -A 2 -B 2Length of output: 558
Script:
#!/bin/bash # Search for onClick handlers in the application card files echo "=== Searching ApplicationCard.tsx ===" rg "onClick=" app/client/src/pages/Applications/ApplicationCard.tsx -A 5 -B 5 echo -e "\n=== Searching ApplicationCardList files ===" rg "onClick=" app/client/src/ee/pages/Applications/ApplicationCardList.tsx app/client/src/ce/pages/Applications/ApplicationCardList.tsx -A 5 -B 5Length of output: 951
app/client/src/pages/Editor/AppsmithLink.tsx (2)
1-1: LGTM! Good use of useCallback for performance optimization.The implementation correctly memoizes the click handler and uses programmatic navigation.
Also applies to: 7-7, 27-29
Line range hint
33-38: Verify the navigation behavior change.The switch from declarative routing (
toprop) to programmatic navigation (onClick) aligns with the PR objective to prevent full app remounts. However, we should verify this doesn't break keyboard navigation or accessibility.
|
Deploy-Preview-URL: https://ce-38798.dp.appsmith.com |
Description
fixes an issue where App card click causing the whole app to remount and init all redux state
The performance of opening an app in edit has improved drastically post this change
Screen.Recording.2025-01-23.at.11.04.00.AM.mov
Reverts code back to #15168
Changed with https://github.com/appsmithorg/appsmith/pull/27223/files#diff-d346c04dfdbc5fc695a1c5e7308754cd2c676a6ea31e1783b28792d7c3895dd3R461
Precursor for #37648
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12984024751
Commit: daf90eb
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Widget_Error_spec.js
List of identified flaky tests.Mon, 27 Jan 2025 09:17:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes
New Features