fix: fix issue with forking #35988
fix: fix issue with forking #35988
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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
|
github-actions
bot
added
Artifact Platform Product
![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 (2)
- app/client/src/ce/sagas/ApplicationSagas.tsx (1 hunks)
- app/client/src/pages/Editor/index.tsx (4 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: ankitakinger PR: appsmithorg/appsmith#29965 File: app/client/src/ce/sagas/ApplicationSagas.tsx:736-742 Timestamp: 2024-01-03T09:53:22.824Z Learning: The use of `editorId` instead of `appId` is an intentional change in the PR and is part of reverting to a previous functionality.app/client/src/ce/sagas/ApplicationSagas.tsx (1)
Learnt from: ankitakinger PR: appsmithorg/appsmith#29965 File: app/client/src/ce/sagas/ApplicationSagas.tsx:736-742 Timestamp: 2024-01-03T09:53:22.824Z Learning: The use of `editorId` instead of `appId` is an intentional change in the PR and is part of reverting to a previous functionality.
Biome
app/client/src/pages/Editor/index.tsx
[error] 125-125: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Additional comments not posted (4)
app/client/src/pages/Editor/index.tsx (3)
80-81: Good job addingprevPageIdproperty.This property will help in tracking the previous page ID effectively.
116-128: Nice improvement incomponentDidUpdatemethod.Caching
prevPageIdand checking for page ID updates enhances the component's ability to track page transitions.Tools
Biome
[error] 125-125: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
129-162: Well done on streamlining the conditional block.The logic now effectively prevents unnecessary re-initialization of the editor when navigating back to the default page.
app/client/src/ce/sagas/ApplicationSagas.tsx (1)
237-237: Great use of the spread operator.The spread operator simplifies the construction of the
requestobject and makes the code more concise.
|
@brayn003 Should we consider updating an existing test case or adding a new one for unit testing? |
|
@sagar-qa007 think we need to add new test cases for this. I will pick this up as a separate task |
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/Editor/index.tsx (4 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: ankitakinger PR: appsmithorg/appsmith#29965 File: app/client/src/ce/sagas/ApplicationSagas.tsx:736-742 Timestamp: 2024-01-03T09:53:22.824Z Learning: The use of `editorId` instead of `appId` is an intentional change in the PR and is part of reverting to a previous functionality.
Biome
app/client/src/pages/Editor/index.tsx
[error] 125-125: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Additional comments not posted (4)
app/client/src/pages/Editor/index.tsx (4)
80-80: Looks good!The new
prevPageIdproperty will help track page transitions more effectively.
117-123: Great improvement!The updated logic for determining page ID changes is more accurate and reliable. It takes into account the
prevPageIdand compares it with the current page ID derived from thebasePageIdvalues in the props.
125-127: Good thinking!Caching the
prevPageIdvalue in theEditorclass property is a smart move. It will be useful in future lifecycle methods to accurately determine if the page has changed.Tools
Biome
[error] 125-125: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
161-164: Nice simplification!The conditional block that updates the current page and sets up the page has been streamlined. It now checks if both
pageIdandprevPageIdare defined and if the page ID has been updated. This prevents unnecessary re-initialization of the editor when navigating back to the default page.
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/Editor/index.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/index.tsx
Additional context used
Learnings (1)
Common learnings
Learnt from: ankitakinger PR: appsmithorg/appsmith#29965 File: app/client/src/ce/sagas/ApplicationSagas.tsx:736-742 Timestamp: 2024-01-03T09:53:22.824Z Learning: The use of `editorId` instead of `appId` is an intentional change in the PR and is part of reverting to a previous functionality.
Description
Fix issue with forking
Fixes #35835
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/10715600770
Commit: 9a2678c
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Sep 2024 08:15:19 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes