fix: Theming Selected new Font is reverted whenever Color is removed #36119
Conversation
jsartisan
requested review from
ApekshaBhosale,
sagar-qa007 and
a team
as code owners
github-actions
bot
added
App Theming Product
WalkthroughThe recent changes encompass enhancements to the theming functionality within the application, focusing on the multi-select widget's theme validation, optimizations in color handling, and adjustments to theme management across various components. Key updates include the activation of previously skipped test cases, restructuring of theme validation logic, and improvements in performance through optimized function handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ThemeEditor
participant ColorPicker
participant ThemeColorControl
User->>ThemeEditor: Select Theme
ThemeEditor->>ColorPicker: Update Theme Properties
ColorPicker->>ThemeColorControl: Change Color
ThemeColorControl->>ThemeEditor: Apply Color Change
ThemeEditor->>User: Display Updated Theme
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/components/propertyControls/ColorPickerComponentV2.tsx (1)
370-374: Great optimization by usinguseMemofor memoization! Just one small suggestion.Instead of depending on the entire
propsobject, consider destructuring only the required props as dependencies. This will ensure thatdebouncedOnChangeis only recreated when those specific props change, avoiding unnecessary recreations.For example:
-}, [props]); +}, [props.changeColor]);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (3 hunks)
- app/client/src/components/propertyControls/ColorPickerComponentV2.tsx (2 hunks)
- app/client/src/entities/AppTheming/index.ts (1 hunks)
- app/client/src/pages/Editor/ThemePropertyPane/ThemeEditor.tsx (2 hunks)
- app/client/src/pages/Editor/ThemePropertyPane/controls/ThemeColorControl.tsx (2 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
Pattern
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.
Additional comments not posted (11)
app/client/src/entities/AppTheming/index.ts (1)
70-70: Great work on enhancing type safety! 👍The change in the
colorsproperty of theAppThemePropertiesinterface is a valuable improvement. By usingExclude<string, number>as the key type, you ensure that only non-numeric string keys can be used within thecolorsobject.This enhancement prevents potential runtime errors or misconfigurations that could arise from unintended usage of numeric keys in theming properties. It enforces stricter adherence to the expected key types, making the code more robust and maintainable.
Well done on implementing this type-safe change! 🌟
app/client/src/pages/Editor/ThemePropertyPane/controls/ThemeColorControl.tsx (5)
2-2: Approved: The import changes look good.
- Using
useCallbackfor memoizing callback functions is a good practice for optimizing rendering.- Importing
objectKeysfrom@appsmith/utilshelps maintain consistency in utility usage across the codebase.Also applies to: 9-9
37-46: Great work with theonColorClickfunction!
- Memoizing the function with
useCallbackis a good practice for optimizing rendering.- Using a
data-attribute to store the color name is a clean way to associate data with DOM elements.- The logic for toggling
autoFocusbased on the selected color is clear and concise.
48-62: TheonChangeColorfunction looks great!
- Memoizing the function with
useCallbackis a good practice for optimizing rendering.- The function correctly updates the theme object by creating a new object with the updated color property, while preserving other existing properties.
- This approach ensures that the theme remains immutable and only the relevant parts are updated.
67-82: The changes in the color box rendering logic look good.
- Using
objectKeysinstead ofObject.keysmaintains consistency with the utility functions used in the codebase.- Adding the
data-color-nameattribute to theColorBoxcomponent allows for better event handling and data management in theonColorClickfunction.
89-89: The changes in theColorPickerComponentlook good.
- Using the memoized
onChangeColorfunction as thechangeColorprop ensures that the color picker component receives a stable reference to the callback function, preventing unnecessary re-renders.- Adding the
data-colorattribute to theColorPickerComponentprovides additional data for testing and debugging purposes.Also applies to: 91-91
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (2)
15-15: Good work declaring thethemeColorvariable.The variable declaration looks good.
100-100: Great job enabling the test case!It's good to see that the previously skipped test case is now active, indicating that the related bug has been fixed.
app/client/src/pages/Editor/ThemePropertyPane/ThemeEditor.tsx (2)
94-94: Verify the impact of the dependency changes on theonClickChangeThemeButtonfunction.The
useEffecthook dependencies have been updated to includedispatchandthemingStackinstead ofsetAppThemingModeStackAction. This change indicates a potential shift in how the theming mode stack is managed, possibly allowing for more dynamic updates based on the current state of the theming stack and the dispatch function.To verify the impact, please run the following script:
The expected results will confirm that
dispatchandthemingStackare being used correctly in thesetAppThemingModeStackActioncall, ensuring the theming mode stack is updated dynamically based on the current state.Verification successful
Let's correct the file type and adjust the search pattern to verify the impact of the dependency changes. We'll re-run the scripts to gather the necessary information.
The dependency changes in the
useEffecthook are correctly implemented.The inclusion of
dispatchandthemingStackas dependencies in theuseEffecthook ensures that theonClickChangeThemeButtonfunction can dynamically respond to changes in the theming stack. This is confirmed by their usage in thesetAppThemingModeStackActioncall, which updates the theming mode stack based on the current state. This change enhances the dynamic management of the theming stack, ensuring that the UI reflects the latest theme settings.
- The
dispatchfunction is used to callsetAppThemingModeStackActionwith a modifiedthemingStack.- The
themingStackis sliced before being passed tosetAppThemingModeStackAction.These changes are crucial for maintaining the correct state of the theming mode stack.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `dispatch` and `themingStack` in the `onClickChangeThemeButton` function. # Test 1: Search for the usage of `dispatch`. Expect: Usage in the `setAppThemingModeStackAction` call. rg --type typescript -A 5 $'dispatch' | rg $'setAppThemingModeStackAction' # Test 2: Search for the usage of `themingStack`. Expect: Usage in the `setAppThemingModeStackAction` call. rg --type typescript -A 5 $'themingStack' | rg $'setAppThemingModeStackAction'Length of output: 215
Script:
#!/bin/bash # Description: Verify the usage of `dispatch` and `themingStack` in the `onClickChangeThemeButton` function. # Test 1: Search for the usage of `dispatch`. Expect: Usage in the `setAppThemingModeStackAction` call. rg --type ts -A 5 'dispatch' | rg 'setAppThemingModeStackAction' # Test 2: Search for the usage of `themingStack`. Expect: Usage in the `setAppThemingModeStackAction` call. rg --type ts -A 5 'themingStack' | rg 'setAppThemingModeStackAction'Length of output: 498
79-79: Verify the impact of the dependency changes on theupdateSelectedThemefunction.The
useEffecthook dependencies have been updated to includeapplicationIdanddispatchinstead ofupdateSelectedAppThemeAction. This change suggests a shift towards ensuring the effect responds to changes in the application ID and the dispatch function.To verify the impact, please run the following script:
The expected results will confirm that the
applicationIdanddispatchare being used correctly in theupdateSelectedAppThemeActioncall, ensuring the theme update logic responds to changes in the application's state.Verification successful
Let's correct the approach and gather the necessary information to verify the impact of the dependency changes. We'll focus on the
updateSelectedThemefunction and ensure we're looking at the right context.
Let's broaden our search to ensure we locate the
updateSelectedThemefunction and verify the usage ofapplicationIdanddispatch.
The dependency changes in the
useEffecthook are correctly implemented.The inclusion of
applicationIdanddispatchas dependencies ensures that theupdateSelectedThemefunction responds appropriately to changes in these variables. This is crucial for maintaining the correct application state and ensuring that theme updates are applied as expected.
- The
applicationIdanddispatchare used in theupdateSelectedAppThemeActioncall within theuseCallbackhook inThemeEditor.tsx.This change enhances the responsiveness of the theme update logic based on the application's state. Well done on making this improvement!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `applicationId` and `dispatch` in the `updateSelectedTheme` function. # Test 1: Search for the usage of `applicationId`. Expect: Usage in the `updateSelectedAppThemeAction` call. rg --type typescript -A 5 $'applicationId' | rg $'updateSelectedAppThemeAction' # Test 2: Search for the usage of `dispatch`. Expect: Usage in the `updateSelectedAppThemeAction` call. rg --type typescript -A 5 $'dispatch' | rg $'updateSelectedAppThemeAction'Length of output: 216
Script:
#!/bin/bash # Description: Locate the `updateSelectedTheme` function and verify the usage of `applicationId` and `dispatch`. # Step 1: Use `ast-grep` to locate the `updateSelectedTheme` function. ast-grep --lang ts --pattern $'function updateSelectedTheme($$$) { $$$ }' # Step 2: Use `rg` to search for `applicationId` and `dispatch` within the `updateSelectedTheme` function context. rg --type ts -A 10 'function updateSelectedTheme' | rg 'applicationId|dispatch'Length of output: 156
Script:
#!/bin/bash # Description: Search for the `updateSelectedTheme` function across the codebase and verify the usage of `applicationId` and `dispatch`. # Step 1: Use `rg` to search for the `updateSelectedTheme` function name across the codebase. rg 'updateSelectedTheme' # Step 2: Once located, examine the context to verify the usage of `applicationId` and `dispatch`. rg --type ts -A 10 'updateSelectedTheme' | rg 'applicationId|dispatch'Length of output: 1238
app/client/src/components/propertyControls/ColorPickerComponentV2.tsx (1)
547-559: Excellent work with thehandleChangeColorfunction!
- Using
useCallbackfor memoization is a great performance optimization. It prevents unnecessary recreations of the function on each render.- The dependency on
debouncedOnChangeis correctly specified.- The check for empty input value is a good addition. It prevents the function from proceeding with empty input, avoiding unnecessary calls to
debouncedOnChange.- Validating the color using
isValidColorbefore invokingdebouncedOnChangeensures that only valid color values are processed.Overall, these changes enhance the efficiency and reliability of the color change handling. Well done!
| let themesSection = (sectionName, themeName) => | ||
| "//*[text()='" + | ||
| sectionName + | ||
| "']/following-sibling::div//*[text()='" + | ||
| themeName + | ||
| "']"; | ||
| let applyTheme = (sectionName, themeName) => | ||
| themesSection(sectionName, themeName) + | ||
| "/parent::div/following-sibling::div[contains(@class, 't--theme-card')]//div[text()='Apply theme']"; |
There was a problem hiding this comment.
Use data- attributes for selectors instead of XPaths.*
The themesSection and applyTheme functions use XPath selectors to locate elements. As per the additional instructions, it's recommended to use data-* attributes for selectors instead of XPaths.
Please update the functions to use data-* attributes for selectors. For example:
let themesSection = (sectionName, themeName) =>
`[data-section='${sectionName}'] [data-theme='${themeName}']`;
let applyTheme = (sectionName, themeName) =>
`${themesSection(sectionName, themeName)} [data-cy='apply-theme-button']`;
There was a problem hiding this comment.
@jsartisan can you check this suggestions as well please?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Ok let me update it.
| it("3. Apply theme and validate the color", function () { | ||
| appSettings.OpenAppSettings(); | ||
| appSettings.GoToThemeSettings(); | ||
| //Change the Theme | ||
| cy.get(commonlocators.changeThemeBtn).click({ force: true }); | ||
| cy.get(themelocator.currentTheme).click({ force: true }); | ||
| cy.get(".t--theme-card main > main") | ||
| .first() | ||
| .invoke("css", "background-color") | ||
| .then(() => { | ||
| cy.get(".t--draggable-multiselectwidgetv2:contains('more')") | ||
| .last() | ||
| .invoke("css", "background-color") | ||
| .then((selectedBackgroudColor) => { | ||
| expect("rgba(0, 0, 0, 0)").to.equal(selectedBackgroudColor); | ||
| appSettings.ClosePane(); | ||
| }); | ||
| }); | ||
| cy.xpath(applyTheme("Featured themes", "Sunrise")) | ||
| .click({ force: true }) | ||
| .wait(1000); | ||
| cy.contains("Applied theme") | ||
| .click() | ||
| .parent() | ||
| .siblings() | ||
| .find(".t--theme-card > main > section > div > main") | ||
| .eq(0) | ||
| .invoke("css", "background-color") | ||
| .then((backgroudColor) => { | ||
| themeColor = backgroudColor; | ||
|
|
||
| console.log({ backgroudColor }) | ||
| expect(backgroudColor).to.eq("rgb(239, 68, 68)"); | ||
| }); | ||
|
|
||
| //Publish the App and validate change of Theme across the app in publish mode | ||
| deployMode.DeployApp(); | ||
| cy.xpath("//div[@id='root']//section/parent::div").should( | ||
| "have.css", | ||
| "background-color", | ||
| "rgb(165, 42, 42)", | ||
| ); | ||
|
|
||
| console.log({ themeColor }) | ||
|
|
||
| cy.get(".rc-select-selector").click({force: true}); | ||
| cy.get(".rc-select-item-option-selected .bp3-control-indicator") | ||
| .first() | ||
| .should("have.css", "background-color", "rgb(239, 68, 68)"); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Improve the test case by following best practices.
The test case can be improved in the following ways:
- Use data-* attributes for selectors instead of XPaths and plain strings.
- Avoid using arbitrary waits like
cy.wait(1000). - Use multiple assertions for the
expectstatement.
Here's an example of how the test case can be improved:
it("3. Apply theme and validate the color", function () {
appSettings.OpenAppSettings();
appSettings.GoToThemeSettings();
cy.get(commonlocators.changeThemeBtn).click({ force: true });
cy.get("[data-cy='featured-themes-section'] [data-cy='sunrise-theme']")
.find("[data-cy='apply-theme-button']")
.click({ force: true });
cy.contains("Applied theme")
.click()
.parent()
.siblings()
.find("[data-cy='theme-color']")
.eq(0)
.invoke("css", "background-color")
.then((backgroundColor) => {
themeColor = backgroundColor;
expect(backgroundColor).to.eq("rgb(239, 68, 68)");
});
deployMode.DeployApp();
cy.get("[data-cy='multi-select']").click({force: true});
cy.get("[data-cy='selected-option']")
.first()
.should("have.css", "background-color", themeColor)
.and("eq", "rgb(239, 68, 68)");
});
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10714214512. |
| let themesSection = (sectionName, themeName) => | ||
| "//*[text()='" + | ||
| sectionName + | ||
| "']/following-sibling::div//*[text()='" + | ||
| themeName + | ||
| "']"; | ||
| let applyTheme = (sectionName, themeName) => | ||
| themesSection(sectionName, themeName) + | ||
| "/parent::div/following-sibling::div[contains(@class, 't--theme-card')]//div[text()='Apply theme']"; |
There was a problem hiding this comment.
@jsartisan can you check this suggestions as well please?
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10714228036. |
|
Deploy-Preview-URL: https://ce-36119.dp.appsmith.com |
1 similar comment
|
Deploy-Preview-URL: https://ce-36119.dp.appsmith.com |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10714747038. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
3-3: Review and Improve Cypress Test PracticesThe test file
Theme_MultiSelectWidget_spec.jsshows some adherence to best practices by using data-* attributes. However, there are several instances of plain string selectors that should be replaced with data-* attributes or locator variables to enhance test robustness. Additionally, avoid using arbitrary wait times likecy.wait(1000)to prevent flaky tests.
- Replace plain string selectors such as
cy.contains("Border")andcy.get(".rc-virtual-list .rc-select-item-option")with data-* attributes or locator variables.- Review the use of
cy.waitwith specific times and consider using network aliases or other strategies to wait for elements to be actionable.By addressing these points, your tests will be more reliable and maintainable. Keep up the good work and continue refining your testing practices!
Analysis chain
Added test file: Verify adherence to best practices.
The file
Theme_MultiSelectWidget_spec.jshas been added to the limited tests. Ensure that this test follows best practices for Cypress tests as outlined:
- Use data-* attributes for selectors.
- Avoid using cy.wait, cy.pause, and agHelper.sleep().
- Use locator variables instead of plain strings.
- Ensure multiple assertions per expect statement.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the adherence to Cypress best practices in Theme_MultiSelectWidget_spec.js. # Test: Search for non-adherence patterns in the new test file. rg --type txt -A 5 $'cy.wait|cy.pause|agHelper.sleep|button\[type=submit\]' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.jsLength of output: 1037
Script:
#!/bin/bash # Description: Verify the use of data-* attributes and locator variables in Theme_MultiSelectWidget_spec.js. # Test: Search for the use of data-* attributes in the test file. rg --type js 'data-' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js # Test: Search for plain string selectors to verify the use of locator variables. rg --type js -oP 'cy\.(get|contains|xpath)\(["\']([^#\.\[][^"\']+)["\']\)' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.jsLength of output: 503
Script:
#!/bin/bash # Description: Verify the use of data-* attributes and locator variables in Theme_MultiSelectWidget_spec.js. # Test: Search for the use of data-* attributes in the test file. rg --type js 'data-' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js # Test: Search for plain string selectors to verify the use of locator variables. rg --type js -oP 'cy\.(get|contains|xpath)\(["\'][^"\']+["\']\)' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.jsLength of output: 295
Script:
#!/bin/bash # Description: Verify the use of data-* attributes and locator variables in Theme_MultiSelectWidget_spec.js. # Test: Search for the use of data-* attributes in the test file. rg --type js 'data-' app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js # Test: Search for plain string selectors to verify the use of locator variables. rg --type js -oP "cy\.(get|contains|xpath)\(['\"][^'\"]+['\"]\)" app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.jsLength of output: 761
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (3 hunks)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/locators/ThemeLocators.json (1 hunks)
- app/client/src/pages/Editor/ThemePropertyPane/ThemeCard.tsx (1 hunks)
- app/client/src/pages/Editor/ThemePropertyPane/ThemeSelector.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/ThemePropertyPane/ThemeSelector.tsx
Additional context used
Path-based instructions (3)
app/client/cypress/limited-tests.txt (1)
Pattern
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/locators/ThemeLocators.json (1)
Pattern
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/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
Pattern
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.
Additional comments not posted (6)
app/client/cypress/limited-tests.txt (1)
2-2: Commented out test file: Ensure coverage is maintained.The test file
Fork_Template_spec.jshas been commented out. It's important to ensure that the test scenarios covered by this file are either no longer relevant or are covered by other tests. This helps maintain comprehensive test coverage.app/client/cypress/locators/ThemeLocators.json (1)
13-13: Excellent addition of a new locator!The new locator
"featuredThemeSection"uses adata-testidattribute, which is a best practice in test automation. This ensures that the tests remain robust and maintainable. Well done on enhancing the theming functionality without disrupting existing locators.app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (2)
Line range hint
19-84: Refactor to avoid usingcy.waitand improve selector strategy.
Avoid using
cy.wait: The use ofcy.waitwith arbitrary time values (e.g.,cy.wait(1000)) is discouraged as it can lead to flaky tests. Instead, use more deterministic wait conditions such ascy.wait('@alias')where@aliasis an alias for a network request.Improve Selector Strategy: The use of XPath to select elements is not recommended. Instead, use
data-*attributes to make the tests more robust and maintainable. For example, replace:cy.xpath("//p[text()='App font']/following-sibling::section//div//input")with a
data-*attribute selector if possible.Refactor to Use Fixtures and Aliases Properly: The fixture
fontDatais loaded but not used effectively. Consider using the fixture data directly in your tests or ensure it's used if intended for setup or assertions.General Best Practices: Ensure that the test descriptions are clear and reflect what is being tested. Also, ensure that assertions are specific and meaningful to avoid false positives.
100-113: Optimize the test case for theme color validation.
Avoid Arbitrary Waits: The use of
cy.wait(1000)should be replaced with more deterministic wait conditions. For example, wait for a specific network call to complete or for a UI element to become visible.Use Data Attributes for Selectors: Replace complex CSS selectors and ensure all selectors are using
data-*attributes to improve test stability and readability. For example, replace:cy.get(".rc-select-item-option-selected .bp3-control-indicator")with a
data-*attribute selector.Improve Assertions: Ensure that assertions are specific and check for meaningful properties. For example, verifying the
background-coloris good, but also consider checking other style or functional properties related to the theme application.app/client/src/pages/Editor/ThemePropertyPane/ThemeCard.tsx (2)
180-180: Addition ofdata-testidattribute enhances testability.The addition of the
data-testidattribute to the main container of theThemeCardcomponent is a good practice for improving the testability of the component. This attribute allows for easier selection in automated tests, which is crucial for ensuring that the component behaves as expected under various conditions. The attribute value dynamically incorporates the theme's name, which helps in identifying different theme cards in test scenarios.
180-180: Well-structured component logic and use of Redux.The
ThemeCardcomponent is well-structured and makes appropriate use of Redux for state management. The logic for handling theme selection and deletion is clear and effectively uses conditional rendering and event handlers. This structure ensures that the component is maintainable and scalable, which is essential for future enhancements.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10714747038. |
|
@jsartisan can we please start to use |
|
/ci-test-limit-count run_count=15 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/10715356702. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10715356702. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (2)
Line range hint
1-88: Optimize and improve test case structure and practices.Dear student, let's refine your test case with some best practices:
- Replace
cy.waitwith more deterministic waits: Using arbitrary waits can lead to flaky tests. Instead, use.should()for conditions to stabilize before proceeding.- Use data- attributes instead of XPaths:* This will make your tests more robust and maintainable.
- Consider splitting the test case: This test seems to validate multiple theming properties. Splitting it into smaller tests could improve clarity and focus.
Here's a snippet to illustrate replacing
cy.xpath:cy.get('[data-cy="app-font-input"]').click({ force: true });And replacing
cy.wait:cy.get('[data-cy="border"]').should('have.length', '3');
Line range hint
89-99: Ensure test case independence for font validation.Dear student, while your intention is clear and the focus is commendable, relying on
themeFontset in a previous test can lead to flakiness if tests are run independently. Consider settingthemeFontwithin this test or using a more stable approach to share state across tests, such as Cypress environment variables.Here's how you can set it within the test:
beforeEach(() => { themeFont = 'Inter, sans-serif'; // Set the expected font directly });
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (2 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
Pattern
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.
| it("3. Apply theme and validate the color", function () { | ||
| appSettings.OpenAppSettings(); | ||
| appSettings.GoToThemeSettings(); | ||
| //Change the Theme | ||
| cy.get(commonlocators.changeThemeBtn).click({ force: true }); | ||
| cy.get(themelocator.currentTheme).click({ force: true }); | ||
| cy.get(".t--theme-card main > main") | ||
| .first() | ||
| .invoke("css", "background-color") | ||
| .then(() => { | ||
| cy.get(".t--draggable-multiselectwidgetv2:contains('more')") | ||
| .last() | ||
| .invoke("css", "background-color") | ||
| .then((selectedBackgroudColor) => { | ||
| expect("rgba(0, 0, 0, 0)").to.equal(selectedBackgroudColor); | ||
| appSettings.ClosePane(); | ||
| }); | ||
| }); | ||
| cy.get( | ||
| `${themelocator.featuredThemeSection} [data-testid='t--theme-card-Sunrise']`, | ||
| ) | ||
| .click({ force: true }) | ||
| .wait(1000); | ||
|
|
||
| //Publish the App and validate change of Theme across the app in publish mode | ||
| deployMode.DeployApp(); | ||
| cy.xpath("//div[@id='root']//section/parent::div").should( | ||
| "have.css", | ||
| "background-color", | ||
| "rgb(165, 42, 42)", | ||
| ); | ||
|
|
||
| cy.get(".rc-select-selector").click({ force: true }); | ||
| cy.get(".rc-select-item-option-selected .bp3-control-indicator") | ||
| .first() | ||
| .should("have.css", "background-color", "rgb(239, 68, 68)"); |
There was a problem hiding this comment.
Enhance selector consistency and assertion detail in theme color validation test.
Dear student, your test is well-focused but let's make it even better:
- Consistent use of selectors: Stick to
data-*attributes throughout for consistency. - Enhance assertions: Provide more detailed assertions to ensure that not only the color is correct but also other style properties if relevant.
Example of enhanced assertion:
cy.get('[data-cy="selected-theme-color"]').should('have.css', 'background-color', 'rgb(239, 68, 68)').and('have.css', 'border-color', 'rgb(239, 68, 68)');
| `${themelocator.featuredThemeSection} [data-testid='t--theme-card-Sunrise']`, | ||
| ) | ||
| .click({ force: true }) | ||
| .wait(1000); |
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (3 hunks)
- app/client/cypress/locators/ThemeLocators.json (1 hunks)
- app/client/cypress/locators/WidgetLocators.ts (1 hunks)
- app/client/packages/eslint-plugin/package.json (1 hunks)
Files skipped from review due to trivial changes (2)
- app/client/cypress/locators/ThemeLocators.json
- app/client/packages/eslint-plugin/package.json
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
Pattern
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/locators/WidgetLocators.ts (1)
Pattern
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.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
58-58: Ensure consistency in font validation across components.The test case "2. Publish the App and validate Font across the app" is crucial for ensuring that the font settings are consistently applied across the app. It's good to see that the test validates the font family using the
themeFontvariable. However, ensure that this variable is robustly set and consider adding more detailed assertions to verify other font properties if relevant.expect(themeFont).to.be.a('string').and.satisfy(msg => msg.startsWith('Inter'));This will ensure that the font is not only applied but also matches the expected type and format.
Also applies to: 60-60, 63-63
app/client/cypress/locators/WidgetLocators.ts (1)
179-183: Review of New Multi-Select Widget LocatorsThe newly added
multiSelectWidgetLocatorsare well-defined and adhere to best practices for Cypress testing:
- Use of data- attributes:* The locators use class selectors which are stable and less likely to change compared to other attributes like id or tags. This is good practice in test automation to ensure that tests are less brittle.
- Clarity and specificity: The locators are specific enough to uniquely identify elements without being overly complex. This helps in maintaining the tests easier.
However, there are a few improvements and checks you might consider:
- Verification of selectors: Ensure that these selectors are indeed present in the DOM and are unique to the elements they are supposed to select. This is crucial as any changes in the application's HTML structure could make these selectors invalid.
- Avoiding XPath where not necessary: It's noted that other locators in the file use XPath, which can be slower and more brittle. It's good that the new locators do not use XPath, and this practice should be continued.
Overall, the addition seems appropriate for the intended testing purposes.
| cy.contains("Shadow").click({ force: true }); | ||
|
|
||
| //Font | ||
|
|
There was a problem hiding this comment.
Refactor: Replace XPath with data- attributes for selectors.*
The use of XPath selectors is discouraged due to their fragility and performance issues. It's better to use data-* attributes, which are more robust and maintainable. For instance:
- cy.xpath("//p[text()='App font']/following-sibling::section//div//input")
+ cy.get('[data-cy="app-font-input"]')Please update all instances where XPath is used to use data-* attributes instead.
Also applies to: 34-34, 35-35, 36-36, 39-39, 53-53
| @@ -1,6 +1,7 @@ | |||
| const commonlocators = require("../../../../locators/commonlocators.json"); | |||
| const themelocator = require("../../../../locators/ThemeLocators.json"); | |||
|
|
|||
| import { multiSelectWidgetLocators } from "../../../../locators/WidgetLocators"; | |||
There was a problem hiding this comment.
Refactor: Consolidate imports for better maintainability.
It's commendable to keep all related imports together, but consider consolidating them into fewer lines to enhance readability and maintainability. For example:
- import {
- agHelper,
- locators,
- entityExplorer,
- deployMode,
- appSettings,
- theme,
- draggableWidgets,
- } from "../../../../support/Objects/ObjectsCore";
+ import { agHelper, locators, entityExplorer, deployMode, appSettings, theme, draggableWidgets } from "../../../../support/Objects/ObjectsCore";Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (3 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
Pattern
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.
Additional comments not posted (1)
app/client/cypress/e2e/Regression/ClientSide/ThemingTests/Theme_MultiSelectWidget_spec.js (1)
4-4: Refactor: Consolidate imports for better maintainability.It's commendable to keep all related imports together, but consider consolidating them into fewer lines to enhance readability and maintainability. For example:
- import { - agHelper, - locators, - entityExplorer, - deployMode, - appSettings, - theme, - draggableWidgets, - } from "../../../../support/Objects/ObjectsCore"; + import { agHelper, locators, entityExplorer, deployMode, appSettings, theme, draggableWidgets } from "../../../../support/Objects/ObjectsCore";This change not only simplifies the import statement but also enhances the readability and maintainability of the test code.
Skipped due to learnings
Learnt from: sagar-qa007 PR: appsmithorg/appsmith#34955 File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settingsTe_Spec.ts:33-33 Timestamp: 2024-07-16T08:27:01.667Z Learning: 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.
| cy.fixture("fontData").then(function (testdata) { | ||
| this.testdata = testdata; | ||
| }); | ||
| agHelper.GetNClick($elem); |
There was a problem hiding this comment.
Refactor: Replace XPath with data- attributes for selectors.*
The use of XPath selectors is discouraged due to their fragility and performance issues. It's better to use data-* attributes, which are more robust and maintainable. For instance:
- cy.xpath("//p[text()='App font']/following-sibling::section//div//input")
+ cy.get('[data-cy="app-font-input"]')Please update all instances where XPath is used to use data-* attributes instead.
Also applies to: 36-42, 55-55
| appSettings.ClosePane(); | ||
| }); | ||
|
|
||
| //Skipping due to mentioned bug | ||
| it.skip("2. Publish the App and validate Font across the app + Bug 15007", function () { | ||
| it("2. Publish the App and validate Font across the app", function () { |
There was a problem hiding this comment.
Re-enable and update the skipped test case.
The test case "2. Publish the App and validate Font across the app" is currently skipped. Given the PR's focus on ensuring font settings are retained correctly, this test case should be re-enabled and updated to reflect the new functionality. Ensure that the test case validates the font consistency across different app components after theme changes.
Also applies to: 62-71
| .last() | ||
| .should("have.css", "font-family", themeFont); | ||
| deployMode.NavigateBacktoEditor(); | ||
| }); | ||
|
|
||
| it.skip("3. Validate current theme feature", function () { | ||
| cy.get("#canvas-selection-0").click({ force: true }); | ||
| it("3. Apply theme and validate the color", function () { |
There was a problem hiding this comment.
Optimize theme application and color validation logic.
The test case "3. Apply theme and validate the color" is well-structured to validate the color consistency after theme application. However, consider using more direct selectors and assertions to streamline the process and improve test performance. For example:
- agHelper.GetNClick(`${themelocator.featuredThemeSection} [data-testid='t--theme-card-Sunrise']`)
+ cy.get('[data-cy="theme-card-Sunrise"]').click()This change not only simplifies the selector but also enhances the readability and maintainability of the test code.
Also applies to: 80-93
/ok-to-test tags="@tag.All" This PR fixes a regression that was added by #36119 <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10736250400> > Commit: 11ba202 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10736250400&attempt=1&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank">Cypress dashboard</a>. > Tags: @tag.All > Spec: > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ServerSide/OnLoadTests/ExecuteAction_Spec.ts</ol> > <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">List of identified flaky tests</a>. > <hr>Fri, 06 Sep 2024 11:05:42 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved color input handling to ensure that empty values are processed correctly, enhancing color validation functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Pawan Kumar <pawankumar@Pawans-MacBook-Pro-2.local>
Fixes #15007
/ok-to-test tags="@tag.Theme"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10733545727
Commit: dec2147
Cypress dashboard.
Tags:
@tag.ThemeSpec:
Fri, 06 Sep 2024 06:48:00 UTC
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Improvements