feat: update response tab UI #37352
feat: update response tab UI #37352
Conversation
WalkthroughThe pull request introduces several enhancements across various components within the application. Key changes include the addition of new icons to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
|
github-actions
bot
added
IDE Navigation
![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 and nitpick comments (5)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (3)
13-20: Consider using 'overflow: hidden' instead of 'clip'While
overflow: clipworks,overflow: hiddenhas broader browser support and achieves the same result.- overflow: clip; + overflow: hidden;
43-53: Consider adding transform to the transition for better performanceFor smoother animations, consider adding transform to the transition. GPU-accelerated properties like transform perform better than opacity alone.
- transition: opacity 0.25s; + transition: opacity 0.25s, transform 0.25s; + transform: ${({ $isVisible }) => ($isVisible ? 'scale(1)' : 'scale(0.8)')};
59-66: Consider using design system variable for font-sizeInstead of hardcoding the font-size value, consider using a design system variable for better consistency and maintainability.
- font-size: 13px; + font-size: var(--ads-v2-font-size-4);app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (2)
150-150: Simplify the conditional using optional chaining.You can simplify the condition by using optional chaining.
Apply this diff:
-if (actionResponse.messages && actionResponse.messages.length) { +if (actionResponse.messages?.length) {🧰 Tools
🪛 Biome
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
146-147: Address the TODO: Avoid using 'any' type.Consider refactoring to eliminate the use of the
anytype.Would you like assistance in refactoring this to use a more specific type?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
app/client/packages/design-system/ads/src/__assets__/icons/ads/content-type-json.svgis excluded by!**/*.svgapp/client/packages/design-system/ads/src/__assets__/icons/ads/content-type-raw.svgis excluded by!**/*.svgapp/client/packages/design-system/ads/src/__assets__/icons/ads/content-type-table.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx(5 hunks)app/client/src/IDE/Components/BottomView.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/BindDataButton.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(12 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/index.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts(1 hunks)app/client/src/components/editorComponents/ActionExecutionInProgressView.tsx(2 hunks)app/client/src/components/editorComponents/EntityBottomTabs.tsx(1 hunks)app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx
✅ Files skipped from review due to trivial changes (3)
- app/client/src/IDE/Components/BottomView.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/BindDataButton.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/index.ts
🧰 Additional context used
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (2)
26-35: LGTM! Well-structured StatusBar component
The component properly implements sticky positioning and uses design system variables for z-index and colors.
4-4: Verify if HelpSection component is needed
The HelpSection component is defined but has no styles. If it's not being used, consider removing it to maintain clean code.
app/client/src/components/editorComponents/ActionExecutionInProgressView.tsx (1)
24-24: LGTM! UI changes align well with requirements
The adjustments to spacing and component sizes create a better visual hierarchy:
- Increased gap improves element separation
- Larger spinner emphasizes the loading state
- Smaller cancel button maintains accessibility while being less prominent
Also applies to: 58-58, 66-66
app/client/src/components/editorComponents/EntityBottomTabs.tsx (1)
27-30: LGTM! Proper usage of design system spacing tokens.
The padding updates correctly utilize ADS v2 spacing tokens for consistent spacing across the UI.
✅ Verification successful
LGTM! Proper usage of design system spacing tokens.
All padding updates correctly utilize ADS v2 spacing tokens consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of ADS v2 spacing tokens
rg --type=typescript --type=javascript "ads-v2-spaces-[12]" -A 2 -B 2
Length of output: 106
Script:
#!/bin/bash
# Verify consistent usage of ADS v2 spacing tokens
rg --type=ts --type=js "ads-v2-spaces-[12]" -A 2 -B 2
Length of output: 45225
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (3)
1075-1085: LGTM: New content type icons follow the established import pattern.
The implementation of the new content type icons (table, JSON, raw) follows the consistent pattern used throughout the file.
1165-1167: LGTM: Content type icon entries properly added to lookup table.
The new content type icons are correctly added to the ICON_LOOKUP object with consistent key naming.
1374-1376: LGTM: Icon mappings updated consistently.
The remapping of "view-all" and "view-less" to arrow icons maintains consistency with the icon usage patterns.
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Outdated
Show resolved
Hide resolved
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
68-79: Convert TODO comments to GitHub issues for better trackingThese TODO comments should be converted to GitHub issues to ensure proper tracking and implementation of the remaining tasks.
Would you like me to help create GitHub issues for these remaining tasks?
275-284: Improve type safety in event handlingThe current event handling relies on type assertions and could be more type-safe.
Consider using a more type-safe approach:
- const handleContentTypeChange = useEventCallback((e?: Event) => { - if (e?.target && e.target instanceof HTMLElement) { - const { value } = e.target.dataset; + interface ContentTypeEvent extends Event { + currentTarget: HTMLElement & { + dataset: { + value: string; + }; + }; + } + + const handleContentTypeChange = useEventCallback((e?: ContentTypeEvent) => { + if (e?.currentTarget) { + const { value } = e.currentTarget.dataset; if (typeof value === "string") { setSelectedControl(value); onResponseTabSelect(value); } } });
150-154: Use optional chaining for safer property accessThe current property access could be simplified using optional chaining.
Apply this change:
- if (actionResponse.messages && actionResponse.messages.length) { + if (actionResponse.messages?.length) { //reset error. errorMessage = ""; hintMessages = actionResponse.messages; }🧰 Tools
🪛 Biome
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(12 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/NoResponse.tsx (1)
2-3: Consider migrating from ads-old packageThe use of
@appsmith/ads-oldsuggests technical debt. Consider migrating to the newer@appsmith/adspackage which is already being used for the Button component.app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (3)
5-5: Remove unused empty styled component.The
HelpSectioncomponent appears to be unused and empty. Consider removing it if it's not needed for future implementation.
12-19: Improve layout implementation.Consider these improvements:
- Replace
overflow: clipwith the standardoverflow: hidden- Extract the
-1pxadjustment into a constant or remove if not necessaryexport const DataContainer = styled.div<{ $height: number }>` - height: calc(${({ $height }) => $height}px - 1px); + height: ${({ $height }) => $height}px; display: grid; grid-template-rows: ${TAB_BAR_HEIGHT}px 1fr; grid-template-columns: 100%; position: relative; - overflow: clip; + overflow: hidden; `;
42-52: Extract magic numbers into constants.The FAB implementation uses hardcoded values for positioning. Consider extracting these into themed constants.
+const FAB_SPACING = 20; + export const Fab = styled(Button)<{ $isVisible: boolean }>` - && { - position: absolute; - right: 20px; - bottom: calc(${TAB_BAR_HEIGHT}px + 20px); - box-shadow: 0px 1px 20px 0px rgba(76, 86, 100, 0.11); - z-index: var(--ads-v2-z-index-3); - opacity: ${({ $isVisible }) => ($isVisible ? 1 : 0)}; - transition: opacity 0.25s; - } + position: absolute; + right: ${FAB_SPACING}px; + bottom: calc(${TAB_BAR_HEIGHT}px + ${FAB_SPACING}px); + box-shadow: var(--ads-v2-shadow-md); + z-index: var(--ads-v2-z-index-3); + opacity: ${({ $isVisible }) => ($isVisible ? 1 : 0)}; + transition: opacity 0.25s; `;app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx (1)
Line range hint
1-400: Consider addressing technical debtThe file contains multiple TODO comments and eslint-disable directives. While not blocking for this PR, consider creating technical debt tickets to address these in the future.
Would you like me to help create issues to track these technical debt items?
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
69-82: Remove completed TODO commentsThese TODO comments appear to track completed implementation tasks. They should be removed to maintain clean code.
102-105: Consider debouncing hover detectionThe hover detection might cause frequent re-renders. Consider adding a debounce to the hover detection to optimize performance.
- const isDataContainerHovered = useHover(dataContainerRef); + const isDataContainerHovered = useDebounce(useHover(dataContainerRef), 100);
Line range hint
325-373: Simplify error display logicThe nested conditional rendering can be simplified using early returns or extracted into separate components for better maintainability.
Consider extracting the error display logic into a separate component:
const ErrorDisplay = ({ actionResponse, errorMessage }) => { if (!errorMessage) return null; return ( <ResponseTabErrorContainer> <ResponseTabErrorContent> {/* ... existing error content ... */} </ResponseTabErrorContent> {actionResponse?.request && ( <JsonWrapper> {/* ... existing JSON display ... */} </JsonWrapper> )} </ResponseTabErrorContainer> ); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/src/assets/images/no-response.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/NoResponse.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(12 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/constants.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/constants.ts(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/constants.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/constants.ts
🧰 Additional context used
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 189-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/NoResponse.tsx (3)
17-17: LGTM! Clean layout improvements
The use of TAB_BAR_HEIGHT constant and gap property improves maintainability and consistency.
Also applies to: 22-22
25-29: LGTM! Well-structured styled component
Clean implementation of RunGroup with proper flex layout and consistent spacing.
Line range hint 45-55: Verify the new empty response message
Let's ensure the new EMPTY_RESPONSE_RUN message matches the design requirements.
✅ Verification successful
EMPTY_RESPONSE_RUN message verified
The EMPTY_RESPONSE_RUN message matches the design requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the content of the new message constant
rg "EMPTY_RESPONSE_RUN.*=" -A 2 "app/client/src/ce/constants/messages.ts" "app/client/src/ee/constants/messages.ts"
Length of output: 400
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (2)
25-40: LGTM! Well-structured status bar implementation.
The status bar components are well-implemented with proper positioning, spacing, and layout. The use of CSS variables for colors and spacing promotes consistency.
54-67: LGTM! Clean implementation of loading and text components.
The components are well-implemented with proper type definitions and conditional styling using props.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx (3)
72-72: LGTM: Consistent border styling using design system
The border styling follows design system conventions and provides appropriate visual separation between rows.
337-337: LGTM: Consistent prop handling
The minColumnWidth prop is correctly propagated to the TableWrapper component, maintaining consistency with the new column width behavior.
249-249: Verify minimum column width behavior
The switch to minimum width improves table responsiveness. However, let's verify this works well with various content types.
✅ Verification successful
Minimum column width behavior verified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for table content rendering patterns to ensure compatibility
rg -g '*.{ts,tsx}' 'renderCell|getCellProps|getType' --type ts
Length of output: 110054
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (1)
304-312: Verify edge case handling
Please ensure the component handles these edge cases correctly:
- Empty arrays as response
- Very large responses
- Malformed JSON responses
app/client/src/ce/constants/messages.ts (1)
572-572: LGTM! The new constant follows the established pattern
The new EMPTY_RESPONSE_RUN constant is appropriately placed with other empty response related constants and follows the same implementation pattern.
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11821862241. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (2)
130-135: Enhance error handling with type guards.The error handling could be improved with proper type guards and error object validation.
- if (!actionResponse.isExecutionSuccess) { - errorMessage = actionResponse.readableError - ? getErrorAsString(actionResponse.readableError) - : getErrorAsString(actionResponse.body); + if (!actionResponse.isExecutionSuccess) { + const error = actionResponse.readableError ?? actionResponse.body; + errorMessage = error ? getErrorAsString(error) : 'Unknown error occurred';
437-443: Consider memoizing the ResponseFormatTabs component.The ResponseFormatTabs component might benefit from memoization to prevent unnecessary re-renders when only the container is being hovered.
+const MemoizedResponseFormatTabs = React.memo(ResponseFormatTabs); - <ResponseFormatTabs + <MemoizedResponseFormatTabs data={output} responseType={currentContentType} tableBodyHeight={ responseTabHeight + RESPONSE_TABLE_HEIGHT_OFFSET } />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(12 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 157-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 193-196: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
152-154: Remove type assertion and implement proper typing.
The TODO comment and type assertion indicate a need for proper typing.
447-458: Add ARIA label to FAB button.
The floating action button needs an ARIA label for better accessibility.
199-201: 🛠️ Refactor suggestion
Add null check for initial content type state.
The initial state might be undefined if contentTypeOptions is empty.
- const [selectedContentType, setSelectedContentType] = useState(
- firstContentTypeOption?.value,
- );
+ const [selectedContentType, setSelectedContentType] = useState(
+ firstContentTypeOption?.value ?? 'TABLE'
+ );
|
Deploy-Preview-URL: https://ce-37352.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (4)
5-5: Remove empty styled componentThe
HelpSectioncomponent is empty and doesn't add any styling. Consider removing it if unused or define specific styles if needed.
7-10: Consider using semantic HTML for Root componentThe Root component could benefit from using a semantic HTML element like
mainorsection.-export const Root = styled.div` +export const Root = styled.main` display: flex; flex-direction: column; `;
44-54: Optimize Fab component stylesConsider these improvements:
- The double ampersand (
&&) might be unnecessary unless you're specifically overriding ADS styles.- The bottom positioning calculation could be simplified using CSS variables.
export const Fab = styled(Button)<{ $isVisible: boolean }>` - && { position: absolute; right: 20px; - bottom: calc(${TAB_BAR_HEIGHT}px + 20px); + bottom: calc(var(--tab-bar-height) + 20px); box-shadow: 0px 1px 20px 0px rgba(76, 86, 100, 0.11); z-index: var(--ads-v2-z-index-3); opacity: ${({ $isVisible }) => ($isVisible ? 1 : 0)}; transition: opacity 0.25s; - } `;
66-73: Consider using CSS custom properties for StatusBarTextThe conditional styling could be more maintainable using CSS custom properties for the underline and error colors.
export const StatusBarText = styled(Text)<StatusBarTextProps>` font-size: 13px; ${({ $hasTooltip }) => $hasTooltip && - `text-decoration: underline var(--ads-v2-color-border) dashed;`} + `text-decoration: underline var(--status-bar-underline-color, var(--ads-v2-color-border)) dashed;`} ${({ $isBold }) => $isBold && `font-weight: 700;`} ${({ $isError }) => $isError && - `color: var(--ads-v2-color-fg-on-error);`} + `color: var(--status-bar-error-color, var(--ads-v2-color-fg-on-error));`} `;app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (4)
86-87: Address remaining TODO items for REST and GraphQL API updates.These items appear to be incomplete:
- Update REST API
- Update GQL API
Would you like me to help implement these updates or create GitHub issues to track them?
296-305: Simplify event handler using destructuringThe event handler can be more concise by destructuring the dataset value.
-const handleContentTypeChange = useEventCallback((e?: Event) => { - if (e?.target && e.target instanceof HTMLElement) { - const { value } = e.target.dataset; - - if (typeof value === "string") { - setSelectedContentType(value); - onResponseTabSelect(value); - } - } -}); +const handleContentTypeChange = useEventCallback((e?: Event) => { + const value = e?.target instanceof HTMLElement ? e.target.dataset.value : undefined; + if (typeof value === "string") { + setSelectedContentType(value); + onResponseTabSelect(value); + } +});
424-434: Remove commented out SegmentedControl codeThe SegmentedControl has been replaced with the Menu component. Remove the commented code to improve code maintainability.
- {/* <SegmentedControl - data-testid="t--response-tab-segmented-control" - defaultValue={segmentedControlOptions[0]?.value} - isFullWidth={false} - onChange={(value) => { - setSelectedControl(value); - onResponseTabSelect(value); - }} - options={segmentedControlOptions} - value={selectedControl} - /> */}
166-166: Add optional chaining for safer property accessSeveral property accesses could benefit from optional chaining to prevent potential runtime errors.
Apply optional chaining to these expressions:
-if (actionResponse.messages && actionResponse.messages.length) +if (actionResponse?.messages?.length) -responseDataTypes && responseDataTypes.map +responseDataTypes?.map -responseBodyTabs && responseBodyTabs.map +responseBodyTabs?.map -responseDataTypes && responseDataTypes.findIndex +responseDataTypes?.findIndex -actionResponse && actionResponse.pluginErrorDetails +actionResponse?.pluginErrorDetails -actionResponse && actionResponse.request +actionResponse?.requestAlso applies to: 185-199, 202-205, 219-222, 386-386, 391-391
🧰 Tools
🪛 Biome
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx
🧰 Additional context used
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 185-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 202-205: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 219-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 386-386: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 391-391: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts (1)
27-42: LGTM! Well-structured status bar components
The StatusBar and StatusBarInfo components are well implemented with proper spacing, positioning, and responsive layout.
...src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (29)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts (1)
14-15: Enhance deprecation documentationPlease add migration instructions and reason for deprecation in the JSDoc comment.
-/** @deprecated */ +/** + * @deprecated Use locators.responseType directly instead. + * This method will be removed in the next major version. + */app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.ts (1)
Line range hint
41-41: Remove agHelper.Sleep call.Using Sleep/wait commands is against our Cypress testing guidelines. Instead, use Cypress's built-in retry-ability and wait for actual conditions.
Replace with proper wait condition:
- agHelper.Sleep(1000); + agHelper.WaitUntilAllToastsDisappear(); + // or wait for specific element to be ready + agHelper.WaitUntilElementVisible(BottomPane.response.getResponseTab());app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts (3)
Line range hint
54-58: Replace cy.wait with intercept assertionsUsing
cy.waitis against our Cypress best practices. Instead, use route aliases with proper assertions.- cy.wait("@createNewApi").then((interception) => { - expect(interception.request.body.source).to.equal("SELF"); - }); + cy.intercept("POST", "**/api/create").as("createNewApi"); + cy.get("@createNewApi").should((interception) => { + expect(interception.request.body.source).to.equal("SELF"); + });
Line range hint
81-81: Remove explicit Sleep callUsing
agHelper.Sleep()is against our Cypress best practices. Instead, use Cypress's built-in retry-ability and assertions.- agHelper.Sleep(500); + cy.get("@getDatasourceStructure").should("exist");
Line range hint
62-66: Use constants for assertion messagesString literals in assertions should be moved to constants for better maintainability.
+ const QUERY_COUNT_MESSAGE = (count: number) => `${count} queries in this app`; dataSources .getDatasourceListItemDescription(mockDBName) .then(($queryCount) => - expect($queryCount).to.eq("2 queries in this app"), + expect($queryCount).to.eq(QUERY_COUNT_MESSAGE(2)), );app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.ts (1)
Line range hint
1-112: Consider refactoring test structure for better maintainability.While the test follows most Cypress best practices, consider these improvements:
- Extract Redis commands into constants or a separate configuration file
- Use API calls for datasource setup/teardown instead of UI interactions
- Break down the large test case into smaller, focused test cases
Example refactor:
// constants.ts export const REDIS_COMMANDS = { HSET_RECIPE: `HSET recipe:1 name "Vegetable Stir Fry"...`, HGET_NAME: "HGET recipe:1 name", // ... other commands }; // Redis_Basic_Spec.ts import { REDIS_COMMANDS } from './constants'; describe('Redis DS', () => { beforeEach(() => { // Use API calls for setup cy.loginFromAPI(); dataSources.createDataSourceViaAPI("Redis"); }); it('should create HSET successfully', () => { dataSources.CreateQueryAfterDSSaved(); dataSources.EnterQuery(REDIS_COMMANDS.HSET_RECIPE); dataSources.runQueryAndVerifyResponseViews(1); dataSources.AssertQueryTableResponse(0, "4"); }); it('should read single key from HSET', () => { // ... separate test case }); // ... other focused test cases });app/client/cypress/e2e/Sanity/Datasources/Arango_Basic_Spec.ts (2)
Line range hint
304-307: Replace direct selector with data-cy attributeUsing direct text selectors like
_noRecordFoundis fragile. Replace with data-cy attributes for better maintainability.-agHelper - .GetText(dataSources._noRecordFound) - .then(($noRecMsg) => - expect($noRecMsg).to.eq("No data records to show"), +agHelper + .GetText("[data-cy=no-data-message]") + .then(($noRecMsg) => { + expect($noRecMsg).to.eq("No data records to show") + expect($noRecMsg).to.be.visible + })
Line range hint
16-21: Address FIXME/TODO commentsThe FIXME comment indicates Chrome crashes in EE repo. This should be investigated and resolved rather than skipping tests for EE.
Would you like me to help create an issue to track the Chrome crash investigation in EE?
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (3)
Line range hint
57-57: Replace Sleep with proper wait conditionsUsing
agHelper.Sleep()is discouraged. Instead, wait for specific UI elements or network requests to complete.Replace with a more specific wait condition:
- agHelper.Sleep(3000); + agHelper.WaitUntilAllToastsDisappear(); + agHelper.WaitUntilEleAppear(locators._datasourceCard);
Line range hint
219-228: Replace XPath selectors with data- attributes*Using XPath selectors is discouraged. Use data-* attributes for more reliable and maintainable selectors.
Replace the XPath selector with a data attribute:
- cy.xpath(locators._buttonByText("Update")) + cy.get('[data-testid="update-button"]')Please update the component to include the data-testid attribute.
Line range hint
229-271: Improve test organization
- Remove commented-out skipped tests (it.skip)
- Avoid using after hooks as per guidelines
Consider these changes:
- Either implement or remove the skipped tests
- Move the cleanup logic into the individual test cases or a separate cleanup utility function
Also applies to: 306-310
app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.ts (3)
Line range hint
8-10: Remove usage of Sleep() calls.According to the coding guidelines,
agHelper.Sleep()should be avoided. Instead, use Cypress's built-in retry-ability and wait for specific conditions.Replace sleep calls with appropriate wait conditions:
- agHelper.Sleep(2000); + cy.waitUntil(() => table.getSelectedRow(), { timeout: 2000 }); - agHelper.Sleep(5000); + cy.waitUntil(() => table.isTableLoaded(), { timeout: 5000 });Also applies to: 171-172, 317-318
Line range hint
33-45: Enhance assertions with multiple expectations.According to the coding guidelines, tests should use multiple assertions in expect statements where applicable.
Enhance the assertions:
- table.ReadTableRowColumnData(2, 1, "v1", 200).then(($newV1) => { - expect($oldV1).to.not.eq($newV1); - }); + table.ReadTableRowColumnData(2, 1, "v1", 200).then(($newV1) => { + expect($newV1).to.not.eq($oldV1) + .and.to.be.a('string') + .and.to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i); + });Also applies to: 171-190
Line range hint
33-45: Use data- attributes for selectors.*According to the coding guidelines, tests should use data-* attributes for selectors instead of class names or button text.
Update the selectors:
- locators._buttonByText("Run InsertQuery") + locators._dataAttribute("insert-query-btn") - locators._modal + locators._dataAttribute("modal-component")app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js (5)
Line range hint
31-33: Replace cy.wait() with better alternativesMultiple instances of cy.wait() with hard-coded timeouts are used. This can make tests flaky and slow.
Consider using Cypress's built-in retry-ability and assertions:
- cy.wait("@postExecute").should(({ response }) => { + cy.intercept("POST", "**/postExecute").should(({ response }) => {Also applies to: 73-75, 91-93, 171-173, 266-268, 289-291, 409-411
Line range hint
92-92: Remove cy.setQueryTimeout callsUsing setQueryTimeout can make tests unreliable and slower.
Instead, rely on Cypress's automatic retry mechanism and configure defaultCommandTimeout in cypress.config.js if needed.
Also applies to: 266-266, 409-409
Line range hint
33-38: Use data- attributes for selectors*Current selectors use a mix of CSS and XPath patterns. As per guidelines, use data-* attributes for more reliable test selectors.
Example refactor:
- cy.xpath(queryLocators.copyURLicon) + cy.get('[data-cy="copy-url-icon"]')Also applies to: 171-176, 289-294
Line range hint
25-29: Use LoginFromAPI instead of UI loginThe test setup should use API calls for login as per guidelines.
Replace the current login flow with:
beforeEach(() => { LoginFromAPI(); dataSources.StartDataSourceRoutes(); });
Replace arbitrary wait times with proper Cypress test assertions and intercepts
The test file contains multiple instances of arbitrary wait times (
cy.wait(3000),cy.wait(2000)) and repetitive network request waits. This can make tests flaky and slow. Instead:
- Use
cy.intercept()to properly wait for network requests- Replace arbitrary timeouts with proper assertions using
cy.should()with retry-ability- Chain assertions after network requests instead of using separate waits
- Consider using Cypress's built-in automatic retry and timeout mechanisms
Example:
// Instead of: cy.wait(3000); cy.wait("@postExecute").then(...) // Use: cy.intercept('POST', '/api/...').as('postExecute'); cy.get('@postExecute').should('have.property', 'response.statusCode', 200) .then(...)🔗 Analysis chain
Line range hint
1-576: Verify test reliability with static analysisLet's verify if there are other instances of non-recommended patterns in the test file.
Based on the shell script results, I can see extensive usage of
cy.wait()andcy.xpath()in the test files, which are not recommended practices for writing reliable Cypress tests. Let me verify the specific test file in question.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of sleep, wait, and xpath in test files echo "Checking for sleep/wait usage:" rg -i "cy\.wait|cy\.sleep|agHelper\.sleep" --type js echo "Checking for xpath usage:" rg -i "\.xpath\(|xpath\(" --type js echo "Checking for non data-* selectors:" rg -i "\.get\(['\"](?!data-)[^'\"]+['\"]" --type jsLength of output: 199158
Script:
#!/bin/bash # Check the specific test file for wait and xpath usage rg -i "cy\.wait|cy\.xpath|agHelper\.Sleep" "app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js"Length of output: 2016
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (4)
Line range hint
33-34: Replace XPath selectors with data- attributes*The test uses XPath selectors extensively. This is against the coding guidelines and makes tests brittle.
- cy.xpath(deployMode._jsonFormFieldByName("Store Address", false)) + cy.get('[data-cy="json-form-field-store-address"]')Also applies to: 195-197, 267-271
Line range hint
8-19: Consider splitting the test suiteThe test suite is quite large with multiple test cases. Consider splitting it into smaller, focused test suites for better maintainability and faster test execution.
Line range hint
267-271: Replace modal confirmation checks with data attributesModal confirmation checks use text content assertions which can be fragile if text changes. Use data attributes instead.
- agHelper.AssertElementVisibility( - dataSources._visibleTextSpan( - "Are you sure you want to delete this item?" - ), - ); + cy.get('[data-cy="delete-confirmation-modal"]') + .should('be.visible');Also applies to: 195-197
Line range hint
1-3: Remove commented-out codeThe file contains commented-out code that should be removed to maintain cleanliness.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (2)
Line range hint
1-902: Avoid using explicit waits in testsThe test file contains multiple instances of
agHelper.Sleep()which is an anti-pattern in Cypress testing. Instead of using explicit waits, consider using Cypress's built-in retry-ability and waiting for specific conditions.Example refactor:
-agHelper.Sleep(2000); //for documents value to settle! +cy.get('@postExecute').should('exist');
Line range hint
1-902: Use data- attributes for selectors*The test file uses various selectors that could be brittle. Consider using data-* attributes for more reliable test selectors.
Example:
-cy.get('.bp3-disabled') +cy.get('[data-cy=update-button]')app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (4)
Line range hint
21-24: Remove cy.wait usageReplace
cy.wait("@postExecute")with Cypress's built-in retry-ability and assertions.- cy.wait("@postExecute").then(({ response }) => { + cy.get("@postExecute").should(({ response }) => {
Line range hint
137-137: Replace hardcoded selectors with data- attributes*Using XPath and CSS selectors makes tests brittle. Use data-* attributes instead.
- cy.xpath(locators._buttonByText("Update")) + cy.get('[data-cy="update-button"]')Also applies to: 138-138, 139-139, 140-140, 141-141, 142-142, 143-143, 144-144, 145-145, 146-146, 147-147, 148-148, 149-149, 150-150
Line range hint
92-92: Remove agHelper.Sleep() callsReplace
agHelper.Sleep()with proper Cypress waiting mechanisms using assertions.- agHelper.Sleep(2000); + cy.get(locators._widgetInDeployed(draggableWidgets.TABLE)).should('be.visible');Also applies to: 93-93, 94-94, 95-95, 96-96, 97-97, 98-98, 99-99, 100-100, 101-101, 102-102, 103-103, 104-104, 105-105
Line range hint
1-620: Improve test structure and organizationSeveral improvements needed:
- Use LoginFromAPI instead of UI login
- Group related test cases using describe blocks
- Use before/beforeEach for setup instead of separate test cases
- Remove duplicate assertions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts(2 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts(4 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js(1 hunks)app/client/cypress/e2e/Sanity/Datasources/Arango_Basic_Spec.ts(5 hunks)app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts(2 hunks)app/client/cypress/support/Pages/DataSources.ts(2 hunks)app/client/cypress/support/Pages/IDE/BottomPane/Response.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.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.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_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/e2e/Sanity/Datasources/Arango_Basic_Spec.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.
app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.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.
app/client/cypress/support/Pages/DataSources.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.
app/client/cypress/support/Pages/IDE/BottomPane/Response.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.
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 161-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 178-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 362-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.ts (1)
48-48: LGTM! Good use of page object model.
The changes properly utilize the BottomPane page object for handling response type menu interactions.
Also applies to: 50-50
app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts (1)
45-45: LGTM: Method name change follows camelCase convention
The renaming from RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves readability and follows JavaScript/TypeScript naming conventions.
Also applies to: 60-60
app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.ts (1)
33-33: LGTM: Method renaming follows camelCase convention.
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves consistency with JavaScript/TypeScript naming conventions.
Also applies to: 38-38, 43-43, 49-49, 53-53, 58-58, 73-73, 77-77, 90-90, 94-94
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (1)
166-166: LGTM: Method renaming improves readability
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews follows proper camelCase convention and improves code readability.
Also applies to: 282-282
app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.ts (1)
262-262: LGTM! Method renaming follows consistent naming convention.
The change from RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews follows camelCase naming convention.
Also applies to: 300-300, 317-317
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
137-139: Remove TODO comment and implement proper typing.
The previous review comment about implementing proper typing for the response body is still valid. Consider implementing the suggested type:
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- output = actionResponse.body as any;
+ type ActionResponseBody = Record<string, unknown>[] | string;
+ output = actionResponse.body as ActionResponseBody;283-292: Great implementation of loading state and accessibility!
The loading state handling is well-implemented, and the FAB component includes proper ARIA labels for accessibility.
Also applies to: 435-448
70-472: Well-structured implementation of the response tab UI.
The component successfully implements all the requirements from issue #35290, including:
- Query name prefix in header
- Metadata tooltips
- Content type selection via FAB
- Proper error handling and display
🧰 Tools
🪛 Biome
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 161-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 178-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 362-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js (1)
80-80: LGTM! Method name follows camelCase convention.
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves consistency with JavaScript naming conventions.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (1)
71-71: Method renaming follows camelCase convention
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves code consistency by following JavaScript/TypeScript camelCase naming conventions.
Also applies to: 82-82, 378-378
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (1)
336-336: Method renaming follows camelCase convention
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves consistency with JavaScript/TypeScript naming conventions.
Also applies to: 355-355, 741-741, 902-902
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (2)
64-64: Method rename improves consistency
The renaming of RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews follows camelCase naming convention.
Also applies to: 73-73, 620-620
Line range hint 32-38: Verify table data assertions
Consider using data snapshots or fixtures instead of hardcoded assertions for table data.
app/client/cypress/support/Pages/DataSources.ts (2)
Line range hint 1136-1150: Properly marked as deprecated
The method is correctly marked with the @deprecated JSDoc tag, allowing for graceful deprecation while maintaining backward compatibility.
1156-1175: LGTM! Improved implementation with better UI interaction handling
The new implementation enhances reliability by:
- Opening the response type menu before checking visibility
- Using menu item selectors for better stability
- Following consistent naming conventions
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js(2 hunks)app/client/cypress/support/Pages/DataSources.ts(2 hunks)app/client/cypress/support/Pages/IDE/BottomPane/Response.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/Binding_Bug28287_Spec.ts
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js
- app/client/cypress/support/Pages/DataSources.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/IDE/BottomPane/Response.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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
177-186: Add type safety for content type optionsConsider adding a type definition for the content type options to improve type safety and maintainability.
+type ContentTypeOption = { + value: string; + label: string; +}; -const contentTypeOptions = +const contentTypeOptions: ContentTypeOption[] = responseBodyTabs && responseBodyTabs.map((item) => { return { value: item.key, label: item.title }; });🧰 Tools
🪛 Biome
[error] 178-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
114-158: Simplify error handling logicThe error handling logic can be simplified using early returns and optional chaining.
if (actionResponse) { - if (!actionResponse.isExecutionSuccess) { + if (!actionResponse?.isExecutionSuccess) { errorMessage = actionResponse.readableError ? getErrorAsString(actionResponse.readableError) : getErrorAsString(actionResponse.body); + return; } - } else if (isString(actionResponse.body)) { + if (isString(actionResponse.body)) { errorMessage = ""; try { output = JSON.parse(actionResponse.body); } catch (e) { output = [{ response: actionResponse.body }]; } + return; } errorMessage = ""; output = actionResponse.body as any; }🧰 Tools
🪛 Biome
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
161-175: Improve null checks with optional chainingConsider using optional chaining to simplify null checks in the response tabs logic.
-const responseBodyTabs = - responseDataTypes && - responseDataTypes.map((dataType, index) => { +const responseBodyTabs = responseDataTypes?.map((dataType, index) => { -const contentTypeOptions = - responseBodyTabs && - responseBodyTabs.map((item) => { +const contentTypeOptions = responseBodyTabs?.map((item) => { -const selectedTabIndex = - responseDataTypes && - responseDataTypes.findIndex( +const selectedTabIndex = responseDataTypes?.findIndex(Also applies to: 178-181, 195-198
🧰 Tools
🪛 Biome
[error] 161-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/cypress/support/Pages/DataSources.ts (1)
1155-1175: LGTM! Improved implementation with explicit menu controlThe new implementation enhances the test reliability by:
- Explicitly opening and closing the response type menu
- Using updated locator patterns
- Maintaining a cleaner control flow
Consider adding a JSDoc comment to document the parameters:
+/** + * Runs a query and verifies the response views + * @param expectedRecordsCount - Expected number of records in the response + * @param tableCheck - Whether to verify table view options + */ public runQueryAndVerifyResponseViews( expectedRecordsCount = 1, tableCheck = true, ) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/support/Pages/DataSources.ts(2 hunks)app/client/cypress/support/Pages/IDE/BottomPane/Response.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/Pages/DataSources.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.
app/client/cypress/support/Pages/IDE/BottomPane/Response.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.
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 161-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 178-181: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 362-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-367: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts (2)
46-50: LGTM! Well-implemented validation method
The validateRecordCount method follows Cypress best practices with proper assertions and text matching.
22-25:
Improve deprecated method implementation
Even though this method is deprecated, it should follow Cypress best practices:
- Avoid force: true
- Use data-testid selectors
public switchResponseType(type: string): void {
this.switchToResponseTab();
- cy.xpath(this.locators.responseType(type)).click({ force: true });
+ cy.get(this.locators.responseType(type))
+ .should('be.visible')
+ .click();
}Likely invalid or redundant comment.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (2)
60-68: Props interface is well-structured!
The Props interface is well-defined with appropriate types and clear naming conventions.
436-449: Excellent accessibility implementation!
The FAB button implementation includes proper ARIA labels and clear visual indicators.
app/client/cypress/support/Pages/DataSources.ts (1)
Line range hint 1136-1153: Properly marked as deprecated
The method is correctly marked with JSDoc @deprecated tag. This helps other developers know to use the new runQueryAndVerifyResponseViews method instead.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts (1)
53-99: Enhance error handling in validateRecordCountThe validation logic is good, but consider these improvements:
- Add validation for null/undefined text
- Consider using a more specific regex pattern
- Add assertion messages using cy.assert
public validateRecordCount({ count, operator }: ValidationParams): void { cy.get(this.locators.responseRecordCount) .invoke("text") .then((text) => { + if (!text) { + throw new Error("Record count text is empty"); + } - const extractedCount = parseInt(text.match(/\d+/)?.[0] || "0", 10); + const extractedCount = parseInt(text.match(/^(\d+)/)?.[1] || "0", 10); // ... rest of the code }); }app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx (3)
60-68: Consider making some props optional with default valuesThe props interface could be improved by making
runErrorMessageoptional with a default value, similar to howisRunDisabledis handled.interface Props { actionSource: SourceEntity; isRunDisabled?: boolean; isRunning: boolean; onRunClick: () => void; currentActionConfig: Action; - runErrorMessage?: string; + runErrorMessage: string | undefined; actionName: string; }
94-96: Consider memoizing feature flag valueThe feature flag check could be memoized using
useMemosince it's unlikely to change during component lifecycle.- const isActionRedesignEnabled = useFeatureFlag( - FEATURE_FLAG.release_actions_redesign_enabled, - ); + const isActionRedesignEnabled = useMemo( + () => useFeatureFlag(FEATURE_FLAG.release_actions_redesign_enabled), + [] + );
394-477: Consider extracting response viewer into a separate componentThe response data container logic is complex and could be extracted into a separate component for better maintainability.
Consider creating a
QueryResponseDataViewercomponent to handle the data visualization part.app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js (1)
Line range hint
1-600: Clean up commented code and consider extracting constants.The test file contains:
- Commented code that should be removed (e.g., lines 290-297)
- Hardcoded values that could be moved to constants (e.g., bucket names, file paths)
Consider:
- Removing commented code blocks
- Creating constants for frequently used values:
+ const TEST_BUCKET_NAME = "assets-test.appsmith.com"; + const TEST_FILE_PREFIX = "S3File_";app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (1)
Line range hint
1-600: Consider implementing test data cleanup in afterEachThe test suite should clean up test data after each test to ensure test isolation. This helps prevent test interdependencies and makes tests more reliable.
+ afterEach(() => { + // Clean up test data + const cleanup = `DELETE FROM Stores WHERE store_id = '2105';`; + cy.task('queryDB', { query: cleanup }); + });app/client/cypress/support/Pages/DataSources.ts (1)
Line range hint
1136-1153: Method marked as deprecated, consider removing in future releasesThe method is correctly marked with
@deprecatedJSDoc tag. Consider adding migration instructions in the deprecation notice to guide users to use the newrunQueryAndVerifyResponseViewsmethod instead./** @deprecated */ + /** @deprecated Use runQueryAndVerifyResponseViews instead */ public RunQueryNVerifyResponseViews(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts(13 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js(2 hunks)app/client/cypress/e2e/Sanity/Datasources/Arango_Basic_Spec.ts(5 hunks)app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts(3 hunks)app/client/cypress/support/Pages/DataSources.ts(2 hunks)app/client/cypress/support/Pages/IDE/BottomPane/Response.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/cypress/e2e/Regression/ServerSide/Datasources/Redis_Basic_Spec.ts
- app/client/cypress/e2e/Regression/ServerSide/Postgres_DataTypes/UUID_Spec.ts
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts
- app/client/cypress/e2e/Sanity/Datasources/Arango_Basic_Spec.ts
- app/client/cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.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.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_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/support/Pages/DataSources.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.
app/client/cypress/support/Pages/IDE/BottomPane/Response.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.
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 163-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 180-183: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 197-200: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 368-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 373-373: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts (4)
1-6: LGTM! Well-defined TypeScript types
The ComparisonOperator type and ValidationParams interface are well-structured and provide good type safety.
16-18: Replace XPath selector with data-testid selector
This comment was previously made and is still valid. The XPath selector should be replaced with a data-testid selector.
29-32: Remove force: true from deprecated method
Even though this method is deprecated, it should follow Cypress best practices until removed.
38-40:
Remove force: true from menu interaction
Replace force: true with proper element visibility checks:
public openResponseTypeMenu() {
cy.get(this.locators.responseDataContainer).realHover();
- cy.get(this.locators.responseTypeMenuTrigger).click({ force: true });
+ cy.get(this.locators.responseTypeMenuTrigger)
+ .should('be.visible')
+ .realClick();
}Likely invalid or redundant comment.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js (1)
80-84: Verify consistent usage of the updated method signature.
The method signature change from a single numeric parameter to an object parameter provides better structure. However, ensure all other test files using this method are updated accordingly.
✅ Verification successful
The method signature change is consistently used across the codebase
The search results show that all usages of runQueryAndVerifyResponseViews follow the new object parameter pattern. The method is defined in DataSources.ts with default values { count = 1, operator = "eq" } and is correctly used across various test files with different parameter combinations:
- Most files use
{ count: 1, operator: "gte" } - Some use just
{ count: N }which is valid due to default parameters - A few calls with no parameters, which is also valid due to defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of runQueryAndVerifyResponseViews to verify consistent usage
rg -A 2 "runQueryAndVerifyResponseViews" "app/client/cypress"
Length of output: 15040
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (3)
71-72: Replace hardcoded waits with proper assertions
82-83: LGTM: Method signature update follows best practices
The update to use an object parameter { count: 10 } instead of a direct number improves code readability and maintainability.
378-379: Replace hardcoded waits with proper assertions
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (3)
64-64: Method name follows camelCase convention.
The renaming from RunQueryNVerifyResponseViews to runQueryAndVerifyResponseViews improves readability and follows JavaScript/TypeScript naming conventions.
620-620: LGTM! Consistent method usage.
The method usage is consistent with the renamed version.
73-73: Verify count parameter usage across test suites.
The addition of the count parameter enhances the method's functionality. However, ensure consistent usage across all test suites.
✅ Verification successful
Let me check the implementation of this method to understand if the count parameter is optional or required.
Let me try a different approach to find the method implementation.
The count parameter usage is consistent with the method's implementation
The method runQueryAndVerifyResponseViews has an optional count parameter with a default value of 1. The codebase shows consistent usage where count is explicitly provided when needed (e.g., 10, 5, 8) and omitted when the default behavior is desired.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent usage of runQueryAndVerifyResponseViews
# Test: Search for method calls without count parameter
rg -U "runQueryAndVerifyResponseViews\([^{]" app/client/cypress/
Length of output: 2700
Script:
#!/bin/bash
# Search for the implementation of runQueryAndVerifyResponseViews
ast-grep --pattern 'runQueryAndVerifyResponseViews($$$) {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Search for the implementation using ripgrep with more context
rg -U "runQueryAndVerifyResponseViews.*\{" -A 20 app/client/cypress/
Length of output: 49476
app/client/cypress/support/Pages/DataSources.ts (1)
1155-1181: LGTM! Well-structured replacement method with improved type safety
The new implementation provides:
- Type-safe response type validation
- Flexible count validation with operators
- Better separation of concerns using BottomPane.response methods
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Show resolved
Hide resolved
...ctionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (2)
Line range hint
1-89: Critical: Test coverage has been significantly reducedThe removal of response-related test cases (mockSuccessResponse, mockFailedResponse) without replacement leaves critical functionality untested. This is particularly concerning given the PR's objectives to update the response tab UI.
Consider adding test cases for:
- Query name prefix in header
- Number of records display
- Query duration and HTTP status
- Tooltips functionality
- New header component
- Content type icons
- Result type FAB dropdown
Additional test coverage needed for QueryDebuggerTabs component
The current test only verifies text selection functionality. Based on the implementation, the following critical test cases are missing:
- Response tab visibility and content when
actionResponseis available- Schema tab visibility and content when
showSchemais true- Tab switching behavior and correct panel component rendering
- Height adjustment functionality
- Toggle behavior for opening/closing the debugger pane
🔗 Analysis chain
Line range hint
43-89: Verify test coverage for new UI componentsThe current test suite only verifies text selection functionality. Given the PR's UI changes, additional test coverage is needed.
Let's check for any existing tests that might cover these components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files that might need updates echo "Searching for related test files..." rg -t tsx -t ts --glob '!node_modules' -l "test\.(tsx|ts)$" | grep -i -E "response|result|debugger" # Search for existing test patterns that might need to be replicated echo "Searching for existing test patterns..." rg -t tsx -t ts --glob '!node_modules' "describe\(.*Response|describe\(.*Result" -A 5Length of output: 415
Script:
#!/bin/bash # Let's try a different approach using fd to find test files echo "Searching for related test files..." fd -e tsx -e ts "test\.(tsx|ts)$" | grep -i -E "response|result|debugger" # Search for test patterns in these files echo "Searching for test patterns in QueryDebuggerTabs.test.tsx..." fd "QueryDebuggerTabs.test.tsx" -x cat {} # Look for other debugger-related test files echo "Searching for other debugger-related test files..." fd -e tsx -e ts "debugger.*test" -x cat {}Length of output: 34648
Script:
#!/bin/bash # Let's check for existing test coverage of the debugger tabs component echo "Searching for test coverage of debugger tabs..." fd "QueryDebuggerTabs" -x ast-grep --pattern 'describe($$$) { $$$ it($$$) { $$$ } $$$ }' # Look for test patterns in related components echo "Searching for test patterns in related components..." fd -e tsx -e ts "test" -x ast-grep --pattern 'it($$$) { $$$ expect($$$).toBe($$$) $$$ }' # Check implementation to identify testable features echo "Checking implementation of QueryDebuggerTabs..." fd "QueryDebuggerTabs.tsx" -x cat {} | grep -v "test"Length of output: 7997
app/client/src/components/editorComponents/JSResponseView.test.tsx (2)
Line range hint
89-90: Address TODO comment during this updateSince we're already modifying this file, it would be good to address the TODO comment about fixing the eslint disable.
Consider adding proper typing for the store instead of using
any:- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let store: any; + let store: ReturnType<typeof mockStore>;
Additional test coverage needed for new UI components
The current test coverage is insufficient for the UI updates:
- QueryResponseTab has basic tests for prepared statement warnings but lacks coverage for new UI features
- BindDataButton component has no test coverage at all
- ActionExecutionInProgressView component has no test coverage at all
Required test cases:
- BindDataButton: Test click handlers, disabled states, and tooltip functionality
- ActionExecutionInProgressView: Test loading states and progress indicators
- QueryResponseTab: Add tests for new UI interactions with FAB dropdown
🔗 Analysis chain
Line range hint
88-178: Verify test coverage for new UI featuresThe PR objectives mention several UI updates (tooltips, FAB dropdown, etc.). Please verify if additional test cases are needed to cover these new features.
Let me search for both test files and the actual UI component files to better assess the test coverage needs.
I'll analyze the test coverage and implementation of these components to verify if additional test cases are needed for the UI updates.
I need to examine the existing test coverage and implementation details to determine if additional test cases are needed for the UI updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files that might need updates rg -t tsx -t ts "test\.(tsx|ts)$" | while read -r file; do if rg -q "QueryResponseTab|BindDataButton|ActionExecutionInProgress" "$file"; then echo "Related test file that might need updates: $file" rg "describe|it\(" "$file" fi doneLength of output: 78
Script:
#!/bin/bash # Search for test files and their test cases fd ".*\.test\.(tsx|ts|js)$" | while read -r file; do echo "=== $file ===" rg "describe|it\(" "$file" || true done # Search for UI component files to identify test coverage needs fd ".*\.(tsx|ts|js)$" | rg "(QueryResponseTab|BindDataButton|ActionExecutionInProgress)" || trueLength of output: 385470
Script:
#!/bin/bash # Let's check the test coverage for these components ast-grep --pattern $'describe\("QueryResponseTab", () => { $$$ }' app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab.test.tsx # Check the actual component implementations rg -l "class.*QueryResponseTab|function.*QueryResponseTab|const.*QueryResponseTab" app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx # Check BindDataButton implementation and tests rg -l "class.*BindDataButton|function.*BindDataButton|const.*BindDataButton" app/client/src/PluginActionEditor/components/PluginActionResponse/components/BindDataButton.tsx # Check ActionExecutionInProgress implementation rg -l "class.*ActionExecutionInProgress|function.*ActionExecutionInProgress|const.*ActionExecutionInProgress" app/client/src/components/editorComponents/ActionExecutionInProgressView.tsxLength of output: 1023
Script:
#!/bin/bash # Let's examine the test coverage and implementation details rg -A 10 "describe\(.*QueryResponseTab" app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab.test.tsx # Check the actual implementation of QueryResponseTab for UI elements rg -A 10 "function QueryResponseTab|const QueryResponseTab" app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/QueryResponseTab.tsx # Check if there are any existing tests for BindDataButton rg -A 10 "describe\(.*BindDataButton" app/client/src/PluginActionEditor/components/PluginActionResponse/components/BindDataButton.test.tsx || echo "No test file found for BindDataButton" # Check if there are any existing tests for ActionExecutionInProgress rg -A 10 "describe\(.*ActionExecutionInProgress" app/client/src/components/editorComponents/ActionExecutionInProgressView.test.tsx || echo "No test file found for ActionExecutionInProgress"Length of output: 1595
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js (5)
Line range hint
30-39: Remove commented out afterEach blocksThe commented afterEach blocks should be removed as they contain test-stopping logic that's not in use.
- // afterEach(function() { - // if (this.currentTest.state === "failed") { - // Cypress.runner.stop(); - // } - // }); - - // afterEach(() => { - // if (queryName) - // cy.actionContextMenuByEntityName(queryName); - // });
Line range hint
246-247: Remove hardcoded sleepUsing
agHelper.Sleep(3000)is against the coding guidelines. Replace with proper wait conditions.- agHelper.Sleep(3000); //giving some time for options to load + cy.get(generatePage.selectTableDropdown).should('be.enabled');
Line range hint
272-282: Remove commented out code blocksSeveral blocks of commented-out code should be removed to improve maintainability.
- // cy.get(generatePage.updateBtn) - // .closest("button") - // .then((selector) => { - // cy.get(selector) - // .invoke("attr", "class") - // .then((classes) => { - // cy.log("classes are:" + classes); - // expect(classes).not.contain("bp3-disabled"); - // }); - // });
Line range hint
1-20: Use data- attributes for selectors*The test is using JSON locators imported from files. According to guidelines, we should use data-* attributes.
Update the locators in the respective JSON files to use data-* attributes:
// In QueryEditor.json { "findDocs": "[data-cy=mongo-find-docs]", // ... other locators }
Line range hint
432-440: Improve test assertionsThe test assertions could be more specific and include multiple checks as per guidelines.
- BottomPane.response.validateRecordCount({ count: 10 }); + cy.get('@postExecute').then(({ response }) => { + expect(response.body.data.isExecutionSuccess, 'Query executed successfully').to.be.true; + expect(response.body.data.body, 'Response has correct number of records').to.have.length(3); + expect(response.body.data.body[0], 'First record has correct structure').to.have.all.keys(['_id', 'Från', 'Frõ', 'Leverantör', 'Frö']); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js(5 hunks)app/client/src/components/editorComponents/JSResponseView.test.tsx(3 hunks)app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_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 (2)
app/client/src/components/editorComponents/JSResponseView.test.tsx (2)
15-15: LGTM: Import change aligns with empty state text updates
119-119: LGTM: Test assertions updated consistently
The assertions have been updated to match the new empty state text requirements.
Also applies to: 147-147
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/IDE/BottomPane/Response.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 (2)
app/client/cypress/support/Pages/IDE/BottomPane/Response.ts (2)
1-6: Type and Interface Definitions Look Good
The definitions for ComparisonOperator and ValidationParams are correct.
9-23: Locator Definitions Follow Best Practices
The locators are well-organized and appropriately use data-testid attributes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11948844847. |
|
Deploy-Preview-URL: https://ce-37352.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js (3)
Line range hint
30-35: Remove commented out afterEach blocksThe commented-out
afterEachblocks should be removed as they contain Cypress anti-patterns (Cypress.runner.stop()).- // afterEach(function() { - // if (this.currentTest.state === "failed") { - // Cypress.runner.stop(); - // } - // }); - - // afterEach(() => { - // if (queryName) - // cy.actionContextMenuByEntityName(queryName); - // });
Line range hint
266-267: Remove explicit sleep callsReplace
agHelper.Sleep(3000)with proper Cypress wait commands or assertions.- agHelper.Sleep(3000); //giving some time for options to load + cy.get(generatePage.selectTableDropdown).should('be.enabled');
Line range hint
1-24: Organize imports and remove unused onesThe imports section could be better organized. Some imports like
homePageandapiPageare used much later in the file.Consider grouping related imports:
import EditorNavigation, { EntityType, } from "../../../../support/Pages/EditorNavigation"; + const queryLocators = require("../../../../locators/QueryEditor.json"); const generatePage = require("../../../../locators/GeneratePage.json"); const formControls = require("../../../../locators/FormControl.json"); + import { agHelper, entityExplorer, - homePage, dataSources, entityItems, assertHelper, locators, - apiPage, } from "../../../../support/Objects/ObjectsCore"; + import { Widgets } from "../../../../support/Pages/DataSources"; import BottomPane from "../../../../support/Pages/IDE/BottomPane"; + +// Move these imports closer to where they are used +import { homePage, apiPage } from "../../../../support/Objects/ObjectsCore";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js(5 hunks)app/client/src/ce/constants/messages.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/constants/messages.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_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 (2)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js (2)
74-74: Consistent update of validateRecordCount calls
The changes to use object parameter in validateRecordCount are consistent with the new API. However, consider using dynamic count validation based on the actual response data.
Also applies to: 96-96, 104-104, 112-112, 134-134, 148-148, 436-436
Line range hint 92-97: Replace cy.wait with better assertions
Using cy.wait('@postExecute') should be replaced with proper assertions on the UI elements.
alex-golovanov
added
ok-to-test
alex-golovanov
added
ok-to-test
Description
Query response tab UI update.
Fixes #35290
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11956595469
Commit: 11f5301
Cypress dashboard.
Tags: @tag.All
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Thu, 21 Nov 2024 15:42:16 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
ContentTypeTable,ContentTypeJson, andContentTypeRaw.BindDataButtonlabel to "Display on UI".QueryResponseTabwith a new floating action button, improved error handling, and a redesigned user interface.QueryResponseTab.EMPTY_RESPONSE_RUNto prompt users to run queries for responses.TAB_BAR_HEIGHTfor consistent layout calculations.RESPONSE_TABLE_HEIGHT_OFFSETfor table height adjustments.Bug Fixes
ActionExecutionInProgressViewfor better visual clarity.Style
EntityBottomTabsfor improved user experience.ViewHideButtonfor better alignment.Tablecomponent for a cleaner visual presentation.NoResponsecomponent for clarity.QueryDebuggerTabsby removing unnecessary components.