feat: Move conversion flow under feature flag. #32490
Conversation
marks0351
changed the title
…o task/disable-conversion-flow
github-actions
bot
added
Anvil Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8598589473. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
…o task/disable-conversion-flow
marks0351
added
ok-to-test
github-actions
bot
removed
the
skip-changelog
marks0351
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8613961803. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
| @@ -87,6 +87,13 @@ export const setConversionStop = () => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const setConversionFlowOverrideFlagAction = (flag: boolean) => { | |||
There was a problem hiding this comment.
Let's make a common abstract method. We can pass the flag name and values to the payload, right? That if we need to do something similar for other flags, then we can use this method.
marks0351
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8627358585. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
marks0351
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8628081747. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
| setFeatureFlagOverrideValues, | ||
| } from "utils/storage"; | ||
|
|
||
| export const FeaturesToOverride: FeatureFlag[] = [ |
There was a problem hiding this comment.
| export const FeaturesToOverride: FeatureFlag[] = [ | |
| export const AvailableFeaturesToOverride: FeatureFlag[] = [ |
WalkthroughThe recent updates focus on improving the application's layout conversion capabilities and feature flag management. Notable changes include enhancing mobile responsiveness, adding a new layout conversion button under a feature flag, and refining feature flag overrides. These modifications aim to streamline testing processes, enhance layout conversion features, and enable dynamic management of feature flags for controlled feature releases and efficient testing. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
app/client/src/utils/storage.ts
Outdated
| export const getFeatureFlagOverrideValues = async ( | ||
| flagsToFetch = FeaturesToOverride, | ||
| ) => { | ||
| const featureFlagValues: OverriddenFeatureFlags = {}; | ||
| for (const flag of flagsToFetch) { | ||
| featureFlagValues[flag] = (await store.getItem(flag)) as boolean; | ||
| } | ||
| return featureFlagValues; |
There was a problem hiding this comment.
Consider using null checks for flag values in getFeatureFlagOverrideValues.
for (const flag of flagsToFetch) {
- featureFlagValues[flag] = (await store.getItem(flag)) as boolean;
+ const value = await store.getItem(flag);
+ featureFlagValues[flag] = value === null ? false : (value as boolean);
}This ensures that if a flag is not found in the storage, it defaults to false instead of null, maintaining consistent boolean type expectations.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export const getFeatureFlagOverrideValues = async ( | |
| flagsToFetch = FeaturesToOverride, | |
| ) => { | |
| const featureFlagValues: OverriddenFeatureFlags = {}; | |
| for (const flag of flagsToFetch) { | |
| featureFlagValues[flag] = (await store.getItem(flag)) as boolean; | |
| } | |
| return featureFlagValues; | |
| export const getFeatureFlagOverrideValues = async ( | |
| flagsToFetch = FeaturesToOverride, | |
| ) => { | |
| const featureFlagValues: OverriddenFeatureFlags = {}; | |
| for (const flag of flagsToFetch) { | |
| const value = await store.getItem(flag); | |
| featureFlagValues[flag] = value === null ? false : (value as boolean); | |
| } | |
| return featureFlagValues; |
| for (const [flag, value] of Object.entries(featureFlagValues)) { | ||
| await store.setItem(flag, value); |
There was a problem hiding this comment.
Ensure error handling in setFeatureFlagOverrideValues.
for (const [flag, value] of Object.entries(featureFlagValues)) {
+ try {
await store.setItem(flag, value);
+ } catch (error) {
+ log.error(`Error setting feature flag override for ${flag}:`, error);
+ }
}Adding try-catch blocks around store.setItem calls can prevent the function from failing silently in case of errors, improving reliability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| for (const [flag, value] of Object.entries(featureFlagValues)) { | |
| await store.setItem(flag, value); | |
| for (const [flag, value] of Object.entries(featureFlagValues)) { | |
| try { | |
| await store.setItem(flag, value); | |
| } catch (error) { | |
| log.error(`Error setting feature flag override for ${flag}:`, error); | |
| } |
| import { | ||
| selectFeatureFlagCheck, | ||
| selectFeatureFlags, |
There was a problem hiding this comment.
Ensure consistent import ordering.
import {
- selectFeatureFlagCheck,
- selectFeatureFlags,
+ selectFeatureFlags,
+ selectFeatureFlagCheck,
} from "@appsmith/selectors/featureFlagsSelectors";Reordering imports alphabetically can improve readability and maintainability of the code.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| import { | |
| selectFeatureFlagCheck, | |
| selectFeatureFlags, | |
| import { | |
| selectFeatureFlags, | |
| selectFeatureFlagCheck, | |
| } from "@appsmith/selectors/featureFlagsSelectors"; |
| @@ -851,7 +854,7 @@ | |||
| showDebugger, | |||
| scopeValue, | |||
| isPluginAuthFailed, | |||
| featureFlags: state.ui.users.featureFlag.data, | |||
| featureFlags: selectFeatureFlags(state), | |||
There was a problem hiding this comment.
Utilize selectFeatureFlags correctly in mapStateToProps.
- featureFlags: selectFeatureFlags(state),
+ featureFlags: selectFeatureFlags(state, props.datasourceId),Ensure that selectFeatureFlags is called with the appropriate arguments if it requires specific identifiers to fetch relevant flags. This change assumes selectFeatureFlags might need a datasourceId to fetch flags specific to a datasource. Adjust the argument as necessary based on the actual implementation of selectFeatureFlags.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| featureFlags: selectFeatureFlags(state), | |
| featureFlags: selectFeatureFlags(state, props.datasourceId), |
| overrides: OverriddenFeatureFlags, | ||
| ) => { | ||
| return { | ||
| type: ReduxActionTypes.FETCH_OVERRIDDEN_FEATURE_FLAGS, |
There was a problem hiding this comment.
Consider renaming the action type to SET_OVERRIDDEN_FEATURE_FLAGS to more accurately reflect the action's purpose of setting overrides rather than fetching data.
…o task/disable-conversion-flow
marks0351
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8641992011. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
|
/build-deploy-preview airgap-enabled=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8644585505. |
|
Deploy-Preview-URL: https://ce-32490.dp.appsmith.com |
ramsaptami
requested review from
ramsaptami and
albinAppsmith
and removed request for
albinAppsmith
Description
Auto Layout System is being deprecated in favor of Anvil.
So we will no longer update Auto Layout and hence we are removing conversion flow to make sure no new Auto Layout Apps are created.
However we still want to be able to help users who really have mission critical use cases to convert.
two ways to do this
overrideFeatureFlag({release_layout_conversion_enabled: true})) to enable conversion.(for users not connected to internet)Implementation:
selectFeatureFlagsselector.overrideFeatureFlagwhich can take an object of featureflags and save them to indexed db.selectFeatureFlagsis where the values are combined and consumed by all components.Fixes #32140
or
Fixes
Issue URLWarning
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/8641988198
Commit: de8e067
Cypress dashboard url: Click here!