fix: Wrong pageId used for segment navigation #38247
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Hook as useSegmentNavigation
participant Selector as getCurrentBasePageId
participant History as history.push
Hook->>Selector: Get base page ID
Selector-->>Hook: Return basePageId
Hook->>History: Construct URL with basePageId
History-->>Hook: Navigate to segment
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 (1)
app/client/src/pages/Editor/IDE/hooks.ts (1)
Line range hint
92-115: Good architectural pattern: Consistent pageId source across hooks.The consistent usage of
getCurrentBasePageIdselector across hooks (useGetPageFocusUrlanduseIDETabClickHandlers) maintains a single source of truth for page identification.Consider documenting this pattern in the codebase to ensure future modifications follow the same approach.
Also applies to: 166-209
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/IDE/hooks.ts(2 hunks)
🔇 Additional comments (1)
app/client/src/pages/Editor/IDE/hooks.ts (1)
61-61: LGTM: Consistent usage of basePageId across segment navigation.
The changes correctly implement consistent pageId usage across all segment navigation paths.
Let's verify that all navigation paths are using the correct pageId:
Also applies to: 73-85
✅ Verification successful
Verified: getCurrentBasePageId is consistently used across the codebase
The changes in the IDE hooks align with the codebase-wide pattern of using getCurrentBasePageId for page navigation. All relevant components and sagas are using the same selector for base page ID retrieval, ensuring consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of potentially incorrect page ID usage in navigation
rg -g '*.{ts,tsx}' 'history\.push.*pageId(?!.*getCurrentBasePageId)'
Length of output: 342
Script:
#!/bin/bash
# Search for history.push calls with pageId in the URL construction
rg -g '*.{ts,tsx}' 'history\.push.*pageId'
# Also search for getCurrentBasePageId usage to cross-reference
rg -g '*.{ts,tsx}' 'getCurrentBasePageId'
Length of output: 10620
github-actions
bot
added
Community Reported
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #38216 ## Automation /ok-to-test tags="@tag.Git, @tag.IDE" ### 🔍 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/12406790663> > Commit: 194f2ec > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12406790663&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git, @tag.IDE` > Spec: > <hr>Thu, 19 Dec 2024 06:19:57 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced navigation management in the editor for improved user experience. - Streamlined retrieval of the base entity ID for navigation purposes. - **Bug Fixes** - Improved consistency in widget selection behavior based on current focus entity and keyboard events. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #38216
Automation
/ok-to-test tags="@tag.Git, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12406790663
Commit: 194f2ec
Cypress dashboard.
Tags:
@tag.Git, @tag.IDESpec:
Thu, 19 Dec 2024 06:19:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes