Skip to content

feat: enhance navigation state handling in PageEntity and saga #41081

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2025
Merged

feat: enhance navigation state handling in PageEntity and saga #41081

merged 1 commit into from
Jul 4, 2025

Conversation

rahulbarwal
Copy link
Contributor

@rahulbarwal rahulbarwal commented Jul 4, 2025 •
edited by github-actions bot
Loading

Description

Problem

Navigation state was not included when dispatching the navigateToAnotherPage action, causing the invokedBy property to be unset and leading to flakiness in the cypress spec:
app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Selected_Widgets_spec.js

Root cause

The PageEntity component did not pass navigation state, and the NavigateActionSaga’s pushToHistory function did not effectively handle navigation with state, leading to incomplete navigation data.

Solution

This PR handles updating the PageEntity component to include navigation state when dispatching the navigateToAnotherPage action, ensuring the invokedBy property is set to NavigationMethod.EntityExplorer. It also refactors the pushToHistory function in the NavigateActionSaga to better support navigation with state, preserving backward compatibility. The NavigateToAnotherPagePayload type is updated to make state and query optional for greater flexibility.

Fixes a regression introduced here: #41074
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

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/16065286850
Commit: 548dd7e
Cypress dashboard.
Tags: @tag.All
Spec:

Fri, 04 Jul 2025 05:06:13 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Improved navigation behavior to include additional context about the source of navigation within the app.

  • Bug Fixes

    • Enhanced compatibility for navigation actions, ensuring consistent handling of various navigation payload types.

  • Documentation

    • Added detailed comments to clarify supported navigation patterns and their handling.

@rahulbarwal
feat: enhance navigation state handling in PageEntity and saga
548dd7e 
Updated the PageEntity component to include navigation state when dispatching the navigateToAnotherPage action, ensuring that the invokedBy property is set to NavigationMethod.EntityExplorer. Additionally, refactored the pushToHistory function in the NavigateActionSaga to handle navigation with state more effectively, maintaining backward compatibility with existing navigation patterns. Updated the NavigateToAnotherPagePayload type to make the state and query properties optional for improved flexibility.
@rahulbarwal rahulbarwal self-assigned this Jul 4, 2025
@github-actions github-actions bot added the Enhancement New feature or request label Jul 4, 2025
@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 4, 2025 •
edited
Loading

Walkthrough

This change updates how navigation actions are dispatched and handled. It modifies the navigation payload to include contextual information, adjusts the saga to support multiple payload variants for backward compatibility, and makes certain payload properties optional in their type definition.

Changes

File(s) Change Summary
app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx Dispatches navigateToAnotherPage with an object containing pageURL and a state for navigation context.
app/client/src/sagas/ActionExecution/NavigateActionSaga/index.ts Updates pushToHistory saga to handle payloads with optional query/state, adds detailed documentation.
app/client/src/sagas/ActionExecution/NavigateActionSaga/types.ts Changes NavigateToAnotherPagePayload interface: query and state are now optional properties.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant PageEntity
    participant Redux
    participant Saga
    participant History

    User->>PageEntity: Triggers page switch
    PageEntity->>Redux: Dispatch navigateToAnotherPage({ pageURL, state })
    Redux->>Saga: Handles navigateToAnotherPage action
    Saga->>History: Calls pushToHistory with payload
    History-->>Saga: Navigates using payload (handles string/object/state/query)
Loading

Possibly related PRs

  • appsmithorg/appsmith#41036: Introduced and typed navigateToAnotherPage action and its saga, directly related to the enhanced payload and saga handling in this PR.
  • appsmithorg/appsmith#41074: Centralized navigation through navigateToAnotherPage, ensuring consistent unload behavior, closely tied to the navigation changes here.

Suggested labels

Task, Javascript Product

Suggested reviewers

  • sneha122

Poem

When navigation needs a little more flair,
Context and state now travel with care.
Sagas are ready for payloads anew,
Optional fields, backward paths too!
So onward we code, with history in tow,
To pages unknown—let’s give it a go! 🚀

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea7bfd0 and 548dd7e.

📒 Files selected for processing (3)
  • app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (2 hunks)
  • app/client/src/sagas/ActionExecution/NavigateActionSaga/index.ts (2 hunks)
  • app/client/src/sagas/ActionExecution/NavigateActionSaga/types.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings 
Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: Aishwarya-U-R
PR: appsmithorg/appsmith#29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.

Learnt from: CR
PR: appsmithorg/appsmith#0
File: .cursor/rules/index.mdc:0-0
Timestamp: 2025-06-23T12:22:10.042Z
Learning: Pull request titles in the Appsmith codebase should follow the Conventional Commits specification, using the format 'type(scope): description' with valid types such as feat, fix, docs, style, refactor, perf, test, build, ci, chore, and revert.

Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.

Learnt from: brayn003
PR: appsmithorg/appsmith#38171
File: app/client/src/git/ce/sagas/index.ts:3-8
Timestamp: 2024-12-16T19:47:33.107Z
Learning: When adding actions to `blockingActionSagas` and `nonBlockingActionSagas`, using more specific generic types can lead to TypeScript errors. Therefore, it's acceptable to use `any` for the action payload types in these registries.
app/client/src/sagas/ActionExecution/NavigateActionSaga/types.ts (4) 
Learnt from: brayn003
PR: appsmithorg/appsmith#38171
File: app/client/src/git/ce/sagas/index.ts:3-8
Timestamp: 2024-12-16T19:47:33.107Z
Learning: When adding actions to `blockingActionSagas` and `nonBlockingActionSagas`, using more specific generic types can lead to TypeScript errors. Therefore, it's acceptable to use `any` for the action payload types in these registries.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts:16-24
Timestamp: 2024-12-10T10:53:17.146Z
Learning: In the `getPullBtnStatus` function (`app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts`), default parameter values should be explicitly mentioned to handle component state properly, even if all props are required.

Learnt from: sharat87
PR: appsmithorg/appsmith#34471
File: app/client/src/ce/api/JSActionAPI.tsx:98-111
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJSCollection` method in `JSActionAPI.tsx` uses optional chaining and logical AND to safely handle potential null or undefined values for `action.datasource`.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (3) 
Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:240-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: The `ENTITY_TYPE.MODULE_INSTANCE` case in `EntityProperties.tsx` is intentionally a combination of the logic from both `ENTITY_TYPE.ACTION` and `ENTITY_TYPE.JSACTION`, which explains the presence of what might seem like duplicated code.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29573
File: app/client/src/pages/Editor/Explorer/Entity/EntityProperties.tsx:46-65
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The use of `any` for the `entityProperties` parameter in the `getJSActionBindings` function within `EntityProperties.tsx` is intentional and should not be suggested for refactoring to improve type safety.
app/client/src/sagas/ActionExecution/NavigateActionSaga/index.ts (8) 
Learnt from: brayn003
PR: appsmithorg/appsmith#38171
File: app/client/src/git/ce/sagas/index.ts:3-8
Timestamp: 2024-12-16T19:47:33.107Z
Learning: When adding actions to `blockingActionSagas` and `nonBlockingActionSagas`, using more specific generic types can lead to TypeScript errors. Therefore, it's acceptable to use `any` for the action payload types in these registries.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: In the TypeScript file `app/client/src/git/sagas/deleteBranchSaga.ts`, within the `deleteBranchSaga` function, error handling is managed outside the scope of the catch block. Therefore, casting `error` to `string` in this context is acceptable.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:28-34
Timestamp: 2024-12-10T10:52:38.244Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts`, error handling is managed outside the scope, so casting errors directly to strings is acceptable.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29965
File: app/client/src/utils/storage.ts:129-129
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `migrateAppIdToEditorId` function in `app/client/src/utils/storage.ts` is intended to be called only when `appId` is present and `editorId` is not in the payload. This context should be considered in future reviews related to this function.

Learnt from: ankitakinger
PR: appsmithorg/appsmith#29965
File: app/client/src/utils/storage.ts:129-129
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `migrateAppIdToEditorId` function in `app/client/src/utils/storage.ts` is intended to be called only when `appId` is present and `editorId` is not in the payload. This context should be considered in future reviews related to this function.

Learnt from: ashit-rath
PR: appsmithorg/appsmith#37912
File: app/client/src/git/components/QuickActions/helpers.ts:22-25
Timestamp: 2024-12-03T10:13:43.282Z
Learning: In `app/client/src/git/components/QuickActions/helpers.ts`, the unnecessary `@ts-ignore` comments will be removed in future PRs.

Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts:16-24
Timestamp: 2024-12-10T10:53:17.146Z
Learning: In the `getPullBtnStatus` function (`app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts`), default parameter values should be explicitly mentioned to handle component state properly, even if all props are required.
🧬 Code Graph Analysis (1)
app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (1)
app/client/src/actions/pageActions.tsx (1)
  • navigateToAnotherPage (702-707)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: perform-test / client-build / client-build
🔇 Additional comments (5)
app/client/src/sagas/ActionExecution/NavigateActionSaga/types.ts (1)

5-6: LGTM: Optional properties enhance flexibility

Making query and state optional enables more flexible navigation patterns while maintaining backward compatibility with existing code.

app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (2)

26-26: LGTM: Import added for navigation state

The NavigationMethod import is correctly added to support the enhanced navigation dispatch.

93-100: LGTM: Navigation state properly included

The dispatch now correctly includes the navigation state with invokedBy: NavigationMethod.EntityExplorer, which addresses the missing navigation context issue described in the PR objectives.

app/client/src/sagas/ActionExecution/NavigateActionSaga/index.ts (2)

117-134: LGTM: Excellent documentation of navigation variants

The comprehensive comment block clearly explains the three different navigation scenarios and their corresponding history.push usage patterns. This documentation will help future maintainers understand the rationale for supporting multiple call signatures.

151-155: LGTM: State-only navigation properly handled

The new conditional branch correctly handles payloads that have state but no query by calling history.push(pageURL, state) directly. This addresses the navigation state issue while maintaining backward compatibility.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rahulbarwal rahulbarwal added JS Objects Issues related to JS Objects Query Widgets & IDE Pod All issues related to Query, JS, Eval, Widgets & IDE ok-to-test Required label for CI labels Jul 4, 2025
@rahulbarwal rahulbarwal requested a review from sneha122 July 4, 2025 03:38
@rahulbarwal rahulbarwal marked this pull request as ready for review July 4, 2025 03:39
@rahulbarwal rahulbarwal merged commit ec9d6f7 into release Jul 4, 2025
168 of 172 checks passed
@rahulbarwal rahulbarwal deleted the rahulbarwal/fix-cypress-test branch July 4, 2025 05:08
@sneha122 sneha122 mentioned this pull request Jul 7, 2025
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@sneha122 sneha122 sneha122 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@rahulbarwal rahulbarwal

Labels
Enhancement New feature or request JS Objects Issues related to JS Objects ok-to-test Required label for CI Query Widgets & IDE Pod All issues related to Query, JS, Eval, Widgets & IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rahulbarwal @sneha122