-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
chore: Change the default value of isLoading in settings reducer to true #34490
Conversation
WalkthroughThis update modifies the initial state of the settings reducer within a client-side application. Specifically, the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9678906211. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
app/client/src/ce/reducers/settingsReducer.ts (2)
Line range hint
52-52
: ReplacehasOwnProperty
withObject.hasOwn()
.Using
Object.hasOwn()
is recommended overhasOwnProperty
for better safety and standard compliance.- if (action.payload?.tenantConfiguration?.hasOwnProperty(key)) { + if (Object.hasOwn(action.payload?.tenantConfiguration, key)) {
Line range hint
71-71
: ReplacehasOwnProperty
withObject.hasOwn()
.As previously noted,
Object.hasOwn()
is safer and more compliant thanhasOwnProperty
.- if (action.payload?.tenantConfiguration?.hasOwnProperty(key)) { + if (Object.hasOwn(action.payload?.tenantConfiguration, key)) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/reducers/settingsReducer.ts (1 hunks)
Additional context used
Biome
app/client/src/ce/reducers/settingsReducer.ts
[error] 52-52: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 71-71: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
Additional comments not posted (1)
app/client/src/ce/reducers/settingsReducer.ts (1)
11-11
: Change of default value forisLoading
approved.The change to set
isLoading
totrue
by default is straightforward. However, ensure that this does not lead to a confusing user experience by showing loading indicators where they aren't necessary.
Deploy-Preview-URL: https://ce-34490.dp.appsmith.com |
…rue (appsmithorg#34490) ## Description This PR makes sure that the initial value of `isLoading` in the settings reducer is set to true. This flag is used to conditionally render the loading component and the AdminSettings component in `app/client/src/pages/AdminSettings/index.tsx`. This change makes sure that the Admin Page components are not mounted and unmounted on initial render. The mouting and unmounting used to happen earlier because the flag switched from false => true => false. Fixes #`Issue Number` _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.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/9737924972> > Commit: 553b867 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9737924972&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved initial loading state of settings to enhance user experience during app startup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
This PR makes sure that the initial value of
isLoading
in the settings reducer is set to true.This flag is used to conditionally render the loading component and the AdminSettings component in
app/client/src/pages/AdminSettings/index.tsx
. This change makes sure that the Admin Page components are not mounted and unmounted on initial render. The mouting and unmounting used to happen earlier because the flag switched from false => true => false.Fixes #
Issue Number
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9737924972
Commit: 553b867
Cypress dashboard.
Tags:
@tag.Sanity
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit