Revert "chore: Remove HTML column type feature flag and related code"#39169
Revert "chore: Remove HTML column type feature flag and related code"#39169jacquesikot wants to merge 1 commit intoreleasefrom
Conversation
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request introduces a new feature flag for managing HTML column types in the Table widget. The changes add the flag in the feature flag file with a default value of Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TW as TableWidgetV2
participant FF as FeatureFlag
U->>TW: Interact with Table widget
TW->>FF: Check release_table_html_column_type_enabled flag
alt HTML feature enabled
TW->>TW: Render HTML column normally
else HTML feature disabled
TW->>TW: Convert HTML column to TEXT
end
TW->>U: Display updated table layout
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (2)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
917-953: Safety net converting HTML columns to TEXT
This block prevents stale HTML types when the feature is disabled by forcibly converting them to TEXT, which avoids undefined behavior. However, silently toggling the column type risks user confusion. Consider providing an in-app notification or an explicit mention in the property pane indicating that HTML columns are reverted to TEXT whenever the feature flag is off.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/HTMLCell_spec.ts (1)
15-17: Enabled feature flag for HTML Cell tests
Enabling therelease_table_html_column_type_enabledflag ensures coverage of the HTML column scenario. Consider adding tests for the disabled state to confirm the fallback behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/HTMLCell_spec.ts(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/widgets/TableWidgetV2/constants.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(5 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data/ColumnType.ts(3 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/HTMLCell_spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (17)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (9)
1-1: Import for React, lazy, and Suspense
No issues identified; this is a standard import approach.
22-23: Imports from BaseWidget
These imports for WidgetProps, WidgetState, and BaseWidget look standard and raise no concerns.
29-30: New imports for EventType and Skeleton
Imports appear valid for usage in event handling and fallback UI rendering.
61-62: ConstantHTML_COLUMN_TYPE_ENABLEDimport
This new feature flag constant is a welcome addition for clearer feature toggling. Ensure references to this flag are fully tested.
87-94: Additional imports (contentConfig, styleConfig, etc.)
These imports for widget property configuration appear consistent; no further issues noted.
95-97: Importing definitions and utilities
Bringing in DefaultAutocompleteDefinitions, sanitizeKey, etc., is straightforward. No problems found.
118-120: Additional type imports for transformData
Enriches functionality for data transformations. Looks good.
127-129: Imports for widget query generation types
No immediate concerns; the types are likely used for advanced query configurations.
137-144: Imports for layout, icons, and HTMLCell
Introducing HTMLCell and klonaRegularWithTelemetry indicates deeper feature handling and cloning. No issues noted.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data/ColumnType.ts (5)
3-3: ImportingHTML_COLUMN_TYPE_ENABLED
Brings in the feature flag for conditional logic. No issues found.
8-8: Importing Widget reference
Likely needed for accessing static methods (e.g., getFeatureFlag). No concerns.
71-77:columnTypeWithHtmlarray
Inserting “HTML” into the type list is straightforward, maintaining alphabetical order.
98-106:hiddenlogic forcolumnTypeConfig
The condition returns true whenHTML_COLUMN_TYPE_ENABLEDis true or the column type isEDIT_ACTIONS. Verify that this branch is intentional, as it inverts typical “hidden” checks.
109-136:columnTypeWithHtmlConfigexport
Provides an alternate config incorporating HTML column type. The structure aligns well with existing pattern.app/client/src/ce/entities/FeatureFlag.ts (1)
42-43: LGTM! Feature flag follows naming convention.The feature flag
release_table_html_column_type_enabledis properly defined and initialized with a default value offalse.Also applies to: 92-92
app/client/src/widgets/TableWidgetV2/constants.ts (1)
247-248: LGTM! HTML column type constant is properly defined.The constant is correctly derived from the feature flag and HTML column type is consistently defined across all necessary type definitions.
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data/index.ts (1)
12-12: LGTM! HTML column type configuration is properly integrated.The HTML column type configuration is correctly imported and added to the panel configuration.
Also applies to: 18-18
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
1 participant
Reverts #39108
Summary by CodeRabbit