chore: Updating the UX related to Action Response tabs #37859
Conversation
…o chore/open-response-tab-always
WalkthroughThe changes in this pull request primarily involve modifications to several components related to API response handling and error display. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12084157350. |
![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 (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponseHeaders.tsx (1)
87-93: Consider more explicit status code validationThe
isEmptycheck on statusCode might treat valid status code 0 as empty. Consider using a more explicit validation.- {isEmpty(props.actionResponse.statusCode) ? ( + {props.actionResponse.statusCode === undefined ? (app/client/src/PluginActionEditor/components/PluginActionResponse/PluginActionResponse.tsx (1)
78-96: Consider potential race condition between useEffectsWhile the logic is correct, there could be a race condition between this useEffect and the error handling useEffect. Consider combining them or adding a priority mechanism.
Here's a suggested improvement:
useEffect( function openDefaultTabWhenNoTabIsSelected() { + // Skip if error handling effect has already set the tab + if (executionFailed) return; + if (showSchema && !selectedTab) { dispatch( setPluginActionEditorDebuggerState({ open: true, selectedTab: DEBUGGER_TAB_KEYS.SCHEMA_TAB, }), ); } else if (plugin.type === PluginType.API && !selectedTab) { dispatch( setPluginActionEditorDebuggerState({ open: true, selectedTab: DEBUGGER_TAB_KEYS.RESPONSE_TAB, }), ); } }, - [showSchema, selectedTab, dispatch, plugin.type], + [showSchema, selectedTab, dispatch, plugin.type, executionFailed], );app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponse.tsx (1)
145-147: Consider adding error message styling consistencyThe body display implementation looks good, but consider wrapping the message in the same styling components used for plugin errors for visual consistency.
{body && ( - <div className="t--debugger-log-downstream-message">{body}</div> + <ResponseTabErrorContent> + <div className="t--debugger-log-downstream-message">{body}</div> + </ResponseTabErrorContent> )}app/client/src/components/editorComponents/ApiResponseView.tsx (1)
61-73: Consider adding cleanup to useEffectWhile the implementation correctly handles the default tab selection for API plugins, consider adding a cleanup function to handle component unmounting scenarios.
useEffect( function openDefaultTabWhenNoTabIsSelected() { if (currentActionConfig.pluginType === PluginType.API && !selectedTab) { dispatch( setPluginActionEditorDebuggerState({ open: true, selectedTab: DEBUGGER_TAB_KEYS.RESPONSE_TAB, }), ); } + return () => { + // Reset tab state on unmount if needed + dispatch( + setPluginActionEditorDebuggerState({ + selectedTab: undefined, + }), + ); + }; }, [selectedTab, dispatch, currentActionConfig.pluginType], );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/PluginActionResponse.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponse.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponseHeaders.tsx(1 hunks)app/client/src/components/editorComponents/ApiResponseView.tsx(3 hunks)
🔇 Additional comments (8)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponseHeaders.tsx (1)
83-84: LGTM: Simplified error presentation
The streamlined error callout without debug buttons aligns well with the UX improvement objectives.
app/client/src/PluginActionEditor/components/PluginActionResponse/PluginActionResponse.tsx (2)
1-1: LGTM: Import changes are appropriate
The new imports support the added functionality for plugin type checking and error state detection.
Also applies to: 15-16
65-76: LGTM: Error handling behavior matches requirements
The useEffect correctly implements the requirement to automatically open the response tab when an error occurs.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/ApiResponse.tsx (3)
49-50: LGTM: Styling changes improve readability
The font-size and line-height values provide appropriate spacing for error messages.
124-124: LGTM: Improved error message condition
The enhanced condition properly handles both plugin errors and body content.
96-96: Verify body property usage across components
The addition of body to destructured properties looks good, but let's ensure consistent handling across related components.
✅ Verification successful
Let me search for more information about the usage of these properties.
Let me search for the ActionResponse interface to understand its structure and verify the body property usage.
Addition of body property is correctly implemented
The body property is a standard part of the ActionResponse interface and is consistently used across multiple components in the PluginActionResponse directory for displaying response data, error handling, and format processing. The destructuring in ApiResponse.tsx aligns with the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might need similar updates
ast-grep --pattern 'const { $$$, messages, pluginErrorDetails $$$} = $_'
Length of output: 250
Script:
#!/bin/bash
# Search for usage of actionResponse object to find related components
rg "actionResponse" -t typescript -t javascript
# Search for components handling plugin responses
ast-grep --pattern 'interface $_ {
messages?: $_
pluginErrorDetails?: $_
$$$
}'
Length of output: 170
Script:
#!/bin/bash
# Search for actionResponse usage with proper file extension
rg "actionResponse" --type-add 'ts:*.{ts,tsx}' -t ts
# Look for interface definitions related to plugin responses
rg "interface.*Response" --type-add 'ts:*.{ts,tsx}' -t ts -A 5
Length of output: 37308
Script:
#!/bin/bash
# Search for ActionResponse interface definition
rg "interface ActionResponse" --type-add 'ts:*.{ts,tsx}' -t ts -A 10
# Search for body property usage in components
rg "\.body" --type-add 'ts:*.{ts,tsx}' -t ts -g 'PluginActionResponse/**'
Length of output: 985
Script:
#!/bin/bash
# Search for body property usage in components with correct path
rg "\.body" --type-add 'ts:*.{ts,tsx}' -t ts app/client/src/PluginActionEditor/components/PluginActionResponse/
# Search for other components using actionResponse to verify consistency
ast-grep --pattern 'const { $$$, messages $$$} = $_?.actionResponse'
Length of output: 3419
app/client/src/components/editorComponents/ApiResponseView.tsx (2)
20-20: LGTM: Clean import addition
The import addition follows TypeScript best practices by using the type import syntax.
61-73: Verify tab behavior across different scenarios
Let's verify the tab behavior implementation across the codebase.
✅ Verification successful
Tab behavior implementation is consistent and well-structured
The implementation in ApiResponseView.tsx follows the established pattern for tab management. The usePluginActionResponseTabs hook shows that:
- Response tab is properly configured for API plugin types
- Tab state management is consistent across different plugin types
- Default tab selection aligns with the component's purpose
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be managing the same tab state
ast-grep --pattern 'setPluginActionEditorDebuggerState({
$$$
selectedTab: $_,
$$$
})'
# Look for potential conflicting tab management logic
rg -l "DEBUGGER_TAB_KEYS.RESPONSE_TAB"
Length of output: 4936
Script:
#!/bin/bash
# Let's examine the implementation of PluginActionResponse to understand the tab behavior
ast-grep --pattern 'function usePluginActionResponseTabs() {
$$$
}'
# Check the reducer implementation
ast-grep --pattern 'case ReduxActionTypes.SET_PLUGIN_ACTION_EDITOR_DEBUGGER_STATE: {
$$$
}'
# Look for any tab-related tests
rg -l "test.*tab.*debugger" --type=test
Length of output: 16987
app/client/src/PluginActionEditor/components/PluginActionResponse/PluginActionResponse.tsx
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-37859.dp.appsmith.com |
…o chore/open-response-tab-always
ankitakinger
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12095990785. |
|
Deploy-Preview-URL: https://ce-37859.dp.appsmith.com |
ankitakinger
added
ok-to-test
ankitakinger
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApiError_spec.ts (3)
15-19: Test description accurately reflects the new error handling behavior.The test aligns well with the PR objectives of showing API errors directly in the response tab.
Consider moving the error code "PE-RST-5000" to a constants file for better maintainability:
// constants/errorCodes.ts export const API_ERROR_CODES = { RUNTIME_ERROR: 'PE-RST-5000' };
Line range hint
20-33: Test verifies error tab functionality but could be more comprehensive.The test correctly verifies error display in both normal and split-screen modes.
Add assertions to verify the new simplified error message format mentioned in PR objectives:
// Add after line 32 _.debuggerHelper.AssertErrorMessageFormat({ shouldHaveHeaders: false, shouldHaveDebugButton: false });
Line range hint
11-14: Consider using a more realistic API endpoint for testing.The fake API URL could be moved to a test constants file and potentially use a more realistic endpoint.
// constants/testEndpoints.ts export const TEST_ENDPOINTS = { ERROR_ENDPOINT: 'https://api.example.com/error-simulation' };app/client/cypress/support/Pages/ApiPage.ts (1)
473-475: Consider enhancing error debugging methodThe
DebugErrormethod could benefit from additional error handling and validation.Consider adding error state validation:
public DebugError() { + this.agHelper.AssertElementVisibility(this._responseTabError); this.agHelper.GetNClick(this._responseTabError); + return this; }app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EntityBottomBar_spec.ts (2)
Line range hint
1-159: Remove usage of agHelper.Sleep() and optimize wait patterns.The test contains
agHelper.Sleep(1000)which violates our coding guidelines. Replace sleep calls with proper Cypress wait commands that wait for specific conditions.Consider replacing:
- _.agHelper.Sleep(1000); + _.agHelper.WaitUntilToastDisappear(); // or + _.agHelper.WaitUntilAllToastsDisappear(); // or wait for specific network request + _.agHelper.WaitUntilRequestResponse(requestMatcher);
Line range hint
1-159: Test structure improvements needed.Several improvements needed based on our guidelines:
- Use data-* attributes for selectors
- Move login/logout operations to use API calls
- Consider combining similar test scenarios
Consider refactoring the test setup to use:
before(() => { _.agHelper.LoginFromAPI() }); after(() => { _.agHelper.LogOutviaAPI() });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApiError_spec.ts(1 hunks)app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EntityBottomBar_spec.ts(2 hunks)app/client/cypress/support/Pages/ApiPage.ts(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/QueryResponseTab/styles.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApiError_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/ClientSide/OtherUIFeatures/EntityBottomBar_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/ApiPage.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/ApiPage.ts (2)
85-85: LGTM! Good use of data-testid attribute
The new _responseTabError property follows Cypress best practices by using data-testid for element selection.
Line range hint 1-497: Verify test coverage for error scenarios
The class handles various API response scenarios. Ensure comprehensive test coverage for all error cases, especially with the new error tab functionality.
Let's check the test coverage:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Error handling test coverage appears comprehensive
The codebase includes dedicated test files covering error scenarios:
ApiError_spec.tstests API error responses and debugger integrationErrorMessages_spec.tsverifies error message display and handling- Multiple test files validate error states through
AssertDebugError
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find related test files and verify error scenario coverage
# Look for test files that use ApiPage
fd -e ts -e js --full-path '.*spec|.*test' | xargs rg "ApiPage"
# Check for error scenario tests
rg -A 5 "DebugError|_responseTabError"
Length of output: 7062
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/EntityBottomBar_spec.ts
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/ApiPage.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/ApiPage.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 (1)
app/client/cypress/support/Pages/ApiPage.ts (1)
85-85: Verify the debugger selector's uniqueness
The new selector .t--debugger-count should be unique across the application to ensure reliable test execution.
✅ Verification successful
The .t--debugger-count selector is consistently used across the application
The selector is properly defined in the common locators and used consistently:
- As a debugger trigger button in the UI component
- Referenced uniformly across test files for assertions
- Properly shared between ApiPage and DebuggerHelper test utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for uniqueness of the debugger selector
# Test: Search for all occurrences of the debugger selector
rg -l "t--debugger-count"
Length of output: 385
Script:
#!/bin/bash
# Check the usage context of the selector to ensure it's used consistently
rg "t--debugger-count" -C 2
Length of output: 4223
…o chore/open-response-tab-always
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12120760247. |
|
Deploy-Preview-URL: https://ce-37859.dp.appsmith.com |
…o chore/open-response-tab-always
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/cypress/support/Pages/ApiPage.ts (1)
Line range hint
1-500: Replace sleep/wait calls with proper assertionsSeveral instances of
agHelper.Sleep()andcy.wait()were found in the code. According to the guidelines, these should be replaced with proper assertions:
- Line 134:
this.agHelper.Sleep()- Line 479:
this.agHelper.Sleep(500)Consider using Cypress's built-in retry-ability with assertions instead:
- this.agHelper.Sleep() //Is needed for the entered url value to be registered + this.agHelper.AssertElementContains(ApiEditor.dataSourceField, url) - this.agHelper.Sleep(500) //Clicking import after value settled + this.agHelper.AssertElementEnabled(ApiEditor.curlImportBtn)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/cypress/snapshots/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js/formattedJSONBodyAfterSave.snap.pngis excluded by!**/*.png
📒 Files selected for processing (1)
app/client/cypress/support/Pages/ApiPage.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/ApiPage.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/ApiPage.ts (2)
86-86: LGTM! Property follows naming conventions
The new _debugger property follows the established pattern of using data-testid selectors.
473-473: Consider adding validation for debugger element
The DebugError method should verify the debugger element's presence and state before clicking.
Description
Updating the UX related to Action Response tabs:
Fixes #37836 #37767 #37766
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12174790103
Commit: 0690211
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Dec 2024 10:00:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style