fix: query tab scrollbar padding gap fix #38317
Conversation
WalkthroughThe pull request focuses on enhancing the user interface and scrolling behavior across several components in the Plugin Action Editor. The changes primarily involve modifying overflow properties, introducing a tabbed interface in 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
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12466497577. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (2)
49-55: Header/param count proximity
The getHeadersCount and getParamsCount calls are logically placed; consider consolidating or memoizing if performance becomes a concern in large-scale usage.
58-90: Tab-based layout improves clarity
Excluding the “SETTINGS” tab is straightforward. The string comparison with “==” is functional, though “===” is generally more explicit in TypeScript/JS.- tab == "HEADERS" + tab === "HEADERS"app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (1)
30-32: Deprecation notice is clear
Marking this component as deprecated is good practice. Consider removing it once all references are migrated to the new tab logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx
🧰 Additional context used
📓 Learnings (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (1)
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (1)
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
🔇 Additional comments (11)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts (5)
14-14: No specific issues found.
Empty line addition appears intentional for readability.
15-24: Sticky header approach looks good.
This correctly ensures the header remains visible during scroll.
26-32: Grid layout for Tabs is fine.
It simplifies structural layout, matching the form design needs.
34-36: Height calculation could introduce nested scrolling.
Confirm that this height offset interacts properly with overall parent containers to avoid double scrollbars.
✅ Verification successful
Let me gather information about the component's usage and styling to verify the scrolling concern.
Based on the codebase analysis, I can now provide a final response about the height calculation concern:
Height calculation is properly handled with parent containers
The TabPanel height calculation is part of a well-structured layout system where:
- Parent containers manage overflow appropriately with
overflow: auto/clipproperties - Response container uses explicit height calculations considering scrollbars
- Table components handle their own scrolling independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for TabPanel usage to understand parent containers and layout
rg -l "TabPanel" --type ts --type tsx
# Look for related style definitions that might affect scrolling
rg "height|overflow|scroll" app/client/src/PluginActionEditor/ -A 2 -B 2
Length of output: 60981
2-2: Ensure version compatibility with @appsmith/ads.
Just confirming that these imported components are the correct version for your usage.
✅ Verification successful
Let me verify the imports from @appsmith/ads by checking their usage and compatibility.
Let me check the package versions and dependencies to ensure compatibility.
Let me check one more thing to verify the component structure in the design system.
Import from @appsmith/ads is correct and compatible
The components are properly exported from the design system's Tab module and the workspace-level dependency is correctly configured in package.json. The imported components are being used appropriately in the styled components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check if these components are used consistently across the codebase
ast-grep --pattern 'import { $$$Tabs$$$, $$$TabPanel$$$ } from "@appsmith/ads"'
# Check the actual usage of these components
rg "AdsTabs|AdsTabPanel" -A 2
# Look for any version or compatibility related files
fd "package.json|yarn.lock" --exec grep -l "@appsmith/ads"
Length of output: 1529
Script:
#!/bin/bash
# Check package.json for @appsmith/ads version
cat app/client/package.json | grep -A 5 '"@appsmith/ads"'
# Check the actual Tabs and TabPanel components in ads package
fd "Tabs.tsx|TabPanel.tsx" --exec grep -l "export" app/client/packages/design-system/ads/
Length of output: 1688
Script:
#!/bin/bash
# Check the component structure in both ads packages
rg -g "*.tsx" -g "*.ts" "export.*Tabs|export.*TabPanel" app/client/packages/design-system/ads/
rg -g "*.tsx" -g "*.ts" "export.*Tabs|export.*TabPanel" app/client/packages/design-system/ads-old/
Length of output: 2182
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (5)
7-18: Imports are logically grouped and clearly labeled
These imports appropriately organize style modules, shared constants, and hooks; the organization is clear, promoting maintainability. Good job.
31-37: Destructuring props enhances readability
This makes the code easier to follow; no issues noted.
92-130: Tab panels are well-structured
Body, headers, params, pagination, and authentication panels are neatly separated. This organization is easy to maintain.
48-48: Custom hook usage for tab state is consistent
The useSelectedFormTab hook simplifies tab management logic. Ensure that any invalid tab values are gracefully handled upstream.
✅ Verification successful
Let me verify the usage and error handling of the useSelectedFormTab hook by examining its implementation and usage patterns.
Let me check the actual implementation of the useSelectedFormTab hook to verify its error handling.
Let me check the Redux action and selector implementation to verify how invalid tab values are handled.
Let me check the tab validation and default values in the Redux reducer.
Tab state management is properly handled
The useSelectedFormTab hook is safely integrated with Redux store and uses predefined tab values from API_EDITOR_TABS/EDITOR_TABS enums, ensuring invalid tab values cannot be set. The implementation is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the useSelectedFormTab hook implementation
ast-grep --pattern 'const useSelectedFormTab = () => {
$$$
}'
# Search for all usages of useSelectedFormTab
rg "useSelectedFormTab" -A 2
# Look for any error handling related to tab values
rg "setCurrentTab" -A 3
Length of output: 4581
Script:
#!/bin/bash
# Get the implementation of useSelectedFormTab
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useSelectedFormTab.ts
Length of output: 717
Script:
#!/bin/bash
# Search for the Redux selector and action
rg "getPluginActionConfigSelectedTab|setPluginActionEditorSelectedTab" -A 3
# Look for any tab value validation
ast-grep --pattern 'setPluginActionEditorSelectedTab = ($$$) => {
$$$
}'
Length of output: 8646
Script:
#!/bin/bash
# Look for the reducer handling SET_PLUGIN_ACTION_EDITOR_FORM_SELECTED_TAB
rg "SET_PLUGIN_ACTION_EDITOR_FORM_SELECTED_TAB" -A 5
# Check for any constants defining valid tab values
rg "EDITOR_TABS" -A 5
Length of output: 32546
57-57: Ensure tab accessibility
Using <Styled.Tabs> is effective. Double-check that accessible attributes (like aria-label or role) are properly applied if required.
app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx (1)
15-15: Scroll overflow enables better content visibility
Changing overflow to 'auto' will help prevent hidden content. Confirm no double scrollbars appear as a side effect.
✅ Verification successful
Let me verify if there are any potential double scrollbar issues by checking the component hierarchy and related styles.
Let me try to search for the component's parent structure and any related overflow styles to verify potential double scrollbar issues.
Change to overflow: auto is safe and recommended
The change to overflow: auto is appropriate. The component is a direct child of a Flex container and has a defined width and padding. Parent components use overflow: hidden appropriately, which prevents double scrollbars from appearing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the PluginActionForm component and its parent components to understand the scroll context
ast-grep --pattern 'const PluginActionForm = $$$'
# Search for related Flex components with overflow properties
rg -A 3 -B 3 'overflow.*auto' app/client/src/PluginActionEditor
Length of output: 76629
Script:
#!/bin/bash
# Search for the component file and its content
rg -B 5 -A 5 "PluginActionForm" app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx
# Search for any parent components that might have scroll/overflow properties
rg -B 3 -A 3 "overflow" app/client/src/PluginActionEditor
Length of output: 22582
|
Deploy-Preview-URL: https://ce-38317.dp.appsmith.com |
...ginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx
Outdated
Show resolved
Hide resolved
| theme={theme} | ||
| /> | ||
| </Flex> | ||
| <Styled.Tabs onValueChange={setCurrentTab} value={currentTab}> |
There was a problem hiding this comment.
Seeing this sticky getting cut off when scrolling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (2)
76-81: Replace loose equality with strict equalityUse strict equality (
===) instead of loose equality (==) for type-safe comparisons.- tab == "HEADERS" - ? headersCount - : tab == "PARAMS" - ? paramsCount - : undefined + tab === "HEADERS" + ? headersCount + : tab === "PARAMS" + ? paramsCount + : undefinedConsider extracting this logic into a separate function for better readability:
const getNotificationCount = (tab: string) => { switch (tab) { case "HEADERS": return headersCount; case "PARAMS": return paramsCount; default: return undefined; } };
67-87: Consider extracting tab rendering logicBased on previous discussions, the tab rendering logic will be extracted into separate components. Consider implementing this now to improve maintainability.
The tab rendering logic could be moved to a separate component like
ActionEditorTabsto:
- Reduce component complexity
- Improve maintainability
- Make the code more testable
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts
🧰 Additional context used
📓 Learnings (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (1)
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
🔇 Additional comments (3)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (3)
2-16: Convert relative imports to absolute paths
Previous feedback indicates a preference for absolute imports.
Convert relative imports to absolute paths for consistency:
- import useGetFormActionValues from "./hooks/useGetFormActionValues";
- import { DatasourceConfig } from "./components/DatasourceConfig";
- import { HintMessages } from "./HintMessages";
- import { InfoFields } from "./InfoFields";
+ import useGetFormActionValues from "PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues";
+ import { DatasourceConfig } from "PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/DatasourceConfig";
+ import { HintMessages } from "PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/HintMessages";
+ import { InfoFields } from "PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/InfoFields";Line range hint 18-27: Props interface looks good!
The interface is well-typed and properly documents all required props.
55-128: Verify sticky header behavior
Previous feedback indicates issues with sticky elements being cut off when scrolling.
Please verify that the Styled.FormHeader component's sticky behavior works correctly with the new tabbed layout.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12491187684. |
|
Deploy-Preview-URL: https://ce-38317.dp.appsmith.com |
## Description Remove scrollbar gap in request config. Fixes [#38135](#38135) ## Automation /ok-to-test tags="@tag.IDE" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/12491071853> > Commit: 85399db > Workflow: `PR Automation test suite` > Tags: `@tag.IDE` > Spec: `` > <hr>Wed, 25 Dec 2024 08:14:00 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a tabbed interface in the `CommonEditorForm` for better content organization. - Enhanced usability of the `PluginActionForm` with scrollable content areas. - **Bug Fixes** - Adjusted tooltip positioning to improve visibility in the `EmbeddedDatasourcePathField`. - **Style** - Added new styled components for tabs and headers to enhance layout and design. - **Chores** - Deprecated the `RequestTabs` component in preparation for its removal. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Remove scrollbar gap in request config.
Fixes #38135
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12491071853
Commit: 85399db
Cypress dashboard.
Tags:
@tag.IDESpec:
Wed, 25 Dec 2024 08:47:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores