Conversation
ankitakinger
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request encompasses modifications across multiple files in the client-side application, primarily focusing on restructuring the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/ci-test-limit |
![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
🧹 Nitpick comments (2)
app/client/cypress/limited-tests.txt (1)
Line range hint
1-7: Document the rationale for test selectionConsider adding comments explaining why these specific tests were selected for the limited test suite, especially since they replace the previous template test.
# To run only limited tests - give the spec names in below format: +# API_Edit_spec.js: Tests server-side API functionality cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js +# JSEditorIndent_spec.js: Validates JS editor visual formatting cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.jsapp/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx (1)
15-15: Potential usability concern with hidden overflow
Switching fromoverflow="auto"tooverflow="hidden"may cause out-of-view content to become inaccessible. Consider usingoverflow="auto"oroverflowY="scroll"to maintain scrollability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/limited-tests.txt(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts(0 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts
✅ Files skipped from review due to trivial changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx
🧰 Additional context used
📓 Path-based instructions (1)
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.
📓 Learnings (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (1)
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
🔇 Additional comments (6)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (5)
2-2: New import usage is appropriate
No issues found using Flex from @appsmith/ads.
9-9: RequestTabs import
Importing RequestTabs centralizes request-related logic. This appears to be a clean approach to modularity.
22-22: Prop destructuring
The destructuring of action is concise and consistent with the usage below.
34-40: Container layout
Using Flex to manage layout is fine. Consider verifying that the potential content overflow or resizing behavior is managed well across different screen sizes.
42-65: Component composition
Combining InfoFields, HintMessages, and RequestTabs into a single layout is coherent. Ensure that RequestTabs receives any necessary props for consistent behavior, such as asynchronous validations or error handling, if required by the business logic.
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx (1)
26-26: Review scrolling behavior
Setting overflowY="scroll" is valid for large content. Verify that there is a consistent scrolling experience if this component is nested inside other containers with limited height.
| cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js | ||
| cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js |
There was a problem hiding this comment.
💡 Codebase verification
Missing test files referenced in limited-tests.txt
The test files specified in limited-tests.txt do not exist in the codebase:
cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.jscypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js
🔗 Analysis chain
Verify the existence and implementation of the new test files
Let's ensure the new test files exist and follow the Cypress testing guidelines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the new test files
# Check if the files exist
echo "Checking if test files exist..."
for file in "cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js" "cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js"; do
if [ -f "$file" ]; then
echo "Found: $file"
# Check for prohibited practices
echo "Checking $file for best practices..."
# Check for cy.wait usage
rg "cy\.wait" "$file" || echo "✓ No cy.wait found"
# Check for cy.pause usage
rg "cy\.pause" "$file" || echo "✓ No cy.pause found"
# Check for agHelper.sleep usage
rg "agHelper\.sleep" "$file" || echo "✓ No agHelper.sleep found"
# Check for it.only usage
rg "it\.only" "$file" || echo "✓ No it.only found"
# Check for after/afterEach hooks
rg "after(Each)?\s*\(" "$file" || echo "✓ No after/afterEach hooks found"
echo "---"
else
echo "Missing: $file"
fi
done
Length of output: 833
|
/ci-test-limit-count run_count=5 update_snapshot=false specs_to_run=cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js,cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12503298486. |
alex-golovanov
left a comment
alex-golovanov
left a comment
There was a problem hiding this comment.
Please do not merge this, as it will revert a bug fix in favor of fixing a test. I will have a PR fixing the test, shortly.
|
/ci-test-limit-count run_count=1 update_snapshot=false |
3 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
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=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
CommonEditorFormcomponent, consolidating functionality into a singleRequestTabscomponent.UQIEditorFormcomponent.Bug Fixes
Chores