chore: Side by Side exits beta phase #38347
Conversation
WalkthroughThe pull request introduces a feature flag mechanism for conditional rendering in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)app/client/cypress/support/commands.js (1)Pattern
app/client/cypress/support/Objects/Registry.ts (1)Pattern
🔇 Additional comments (3)app/client/cypress/support/Objects/Registry.ts (2)
Also applies to: 47-47, 56-56, 65-65, 74-74, 83-83, 92-92, 101-101, 110-110, 119-119, 128-128, 137-137, 146-146, 155-155, 164-164, 173-173, 182-182, 191-191, 200-200, 209-209, 218-218, 227-227, 236-236, 245-245, 254-254, 263-263, 272-272, 281-281, 290-290, 299-299, 308-308, 317-317, 326-326, 335-335
app/client/cypress/support/commands.js (1)
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
|
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12480287402. |
![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/pages/Editor/IDE/EditorPane/components/Announcement.tsx (1)
47-50: Consider user experience if disabling the announcement.
While returningnulleffectively hides the announcement, confirm this aligns with any product requirement for existing users.app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (2)
26-52: Styling approach looks consistent.
The styled components maintain a uniform design language. Consider consolidating shared styles or using a theme object if these styles are repeated elsewhere.
115-147: Popover logic is well-structured.
Adding the nudge overlay is straightforward. Watch out for potential overuse, which may distract users with too many prompts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/IDE/EditorPane/components/Announcement.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx(4 hunks)
🔇 Additional comments (5)
app/client/src/pages/Editor/IDE/EditorPane/components/Announcement.tsx (2)
7-8: Good use of feature flags.
Your imports are organized correctly, and you've appropriately used the useFeatureFlag hook and FEATURE_FLAG constants.
27-29: Check feature usage across the codebase.
Ensure all references to release_actions_redesign_enabled are intentional and consistently applied.
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (3)
1-12: Relevant imports and usage.
The newly added imports match their usage in the subsequent code. This is concise and clear.
94-95: Intuitive default state.
Using true helps users discover the new popover nudge. Just ensure it doesn't impede existing workflows.
148-148: Clean fallback flow.
The fallback to a simpler tooltip ensures minimal impact if the nudge is dismissed.
|
Deploy-Preview-URL: https://ce-38347.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/IDE/Components/Nudge/styles.ts (3)
8-12: Consider using theme variables for the box shadow color.
It is often preferable to replace raw hex values with theme tokens to ensure consistency and maintainability of your design.
14-22: Avoid frequent usage of!importantin styling.
Using!importantcan lead to specificity conflicts in your stylesheet. Consider alternative approaches, such as more specific selectors, to maintain style priority.
24-38: Use theme-based color tokens instead of hardcoded hex values.
Replacing#ffffffand#ffffff33with theme color tokens can improve maintainability and consistency across the UI.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/IDE/Components/Nudge/Nudge.tsx(1 hunks)app/client/src/IDE/Components/Nudge/index.ts(1 hunks)app/client/src/IDE/Components/Nudge/styles.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/IDE/Components/Nudge/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx
🔇 Additional comments (2)
app/client/src/IDE/Components/Nudge/Nudge.tsx (2)
6-12: Interface looks clear and concise.
The optional props (align, side) and mandatory fields are well-defined.
14-48: The component design is straightforward and handles cleanup properly.
The setTimeout approach is clean and the unmount cleanup prevents memory leaks.
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12481531692. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/Editor/IDE/hooks.ts (1)
206-217: Consider re-checking state when conditions change.
This hook uses the initial condition forwidgetBindingsExistandisActionRedesignEnabled. If either changes at runtime, thevaluemight not reflect the new state. For dynamic updates, consider a derived state or a useEffect.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/IDE/Components/Nudge/Nudge.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx(4 hunks)app/client/src/pages/Editor/IDE/hooks.ts(2 hunks)
🔇 Additional comments (11)
app/client/src/pages/Editor/IDE/hooks.ts (1)
32-35: Looks good!
These additional imports cleanly separate concerns for feature flags and state hooks.
app/client/src/IDE/Components/Nudge/Nudge.tsx (3)
1-5: Imports are well-structured.
These libraries align with the intended UI and positioning functionality.
6-13: Prop definitions are concise.
The optional props and clear naming make this component self-explanatory and straightforward to use.
15-52: Practical popover usage with delayed open.
The managed visibility and cleanup logic are properly implemented. Ensure that the onDismissClick callback also toggles the open state if needed.
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (7)
17-18: Selective imports for the new feature.
Bringing in Nudge and useShowSideBySideNudge is aligned with the new side-by-side encouragement functionality.
46-47: Clear naming for nudge toggles.
These state variables convey their purpose well.
53-54: Dismissing the nudge when switching modes is appropriate.
This ensures the UI hint doesn’t linger unnecessarily.
62-63: Dependency array includes dismissNudge.
Great for ensuring the callback behavior remains consistent.
64-77: Memoized button approach is efficient.
Helps avoid unnecessary re-renders, leveraging useMemo effectively.
98-110: Nudge integration effectively guides users.
The user flow to open and dismiss the nudge is straightforward.
115-117: Tooltip alignment and usage are neat.
Maintains UX consistency when the nudge is not in use.
github-actions
bot
added
IDE Pod
hetunandu
changed the title
github-actions
bot
added
the
skip-changelog
|
Deploy-Preview-URL: https://ce-38347.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/utils/localStorage.tsx (1)
34-34: Good reference usage.
Linking to Modernizr’s local storage detection logic is helpful as a reference for future maintainers.app/client/src/pages/Editor/IDE/hooks.ts (1)
206-227: Custom hook is clearly structured.
This hook reliably manages the “side-by-side mode” nudge state based on local storage and feature flags.Consider parsing the stored value (
"false") into a boolean for explicit type safety, e.g.JSON.parse()or a custom parser. This would reduce reliance on string literals if the key value evolves.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/IDE/hooks.ts(2 hunks)app/client/src/utils/localStorage.tsx(2 hunks)
🔇 Additional comments (3)
app/client/src/utils/localStorage.tsx (2)
13-13: New local storage key is consistent.
The new NUDGE_SHOWN_SPLIT_PANE key aligns well with existing naming conventions and provides an intuitive identifier for the nudge feature.
18-18: Explicitly setting the error name is a good practice.
Ensuring the LocalStorageNotSupportedError name accurately reflects the error source improves clarity when handling exceptions elsewhere.
app/client/src/pages/Editor/IDE/hooks.ts (1)
32-36: Imports are well-organized and purposeful.
Using useBoolean, isWidgetActionConnectionPresent, and useFeatureFlag is appropriate for enhancing this feature’s logic in a modular way.
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx
Outdated
Show resolved
Hide resolved
| @@ -198,3 +203,25 @@ export const useIDETabClickHandlers = () => { | |||
|
|
|||
| return { addClickHandler, tabClickHandler, closeClickHandler }; | |||
| }; | |||
|
|
|||
| export const useShowSideBySideNudge: () => [boolean, () => void] = () => { | |||
There was a problem hiding this comment.
It's not a big deal at this point, but I would suggest to put hooks in separate files. [nit]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (2)
17-18: Consider using absolute imports for better maintainabilityReplace relative imports with absolute paths to make the code more maintainable and resistant to file structure changes.
-import { Nudge } from "IDE/Components/Nudge"; -import { useShowSideBySideNudge } from "../hooks"; +import { Nudge } from "@appsmith/IDE/Components/Nudge"; +import { useShowSideBySideNudge } from "@appsmith/pages/Editor/IDE/hooks";
98-109: Move hardcoded message to constantsThe nudge message should be moved to the messages constants file for better maintainability and internationalization support.
+import { SIDE_BY_SIDE_NUDGE_MESSAGE } from "ee/constants/messages"; <Nudge align="center" delayOpen={500} - message="Write code and configure UI elements side by side" + message={createMessage(SIDE_BY_SIDE_NUDGE_MESSAGE)} onDismissClick={dismissNudge} side="left" trigger={minimiseButton} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/IDE/Components/Nudge/Nudge.tsx(1 hunks)app/client/src/IDE/Components/Nudge/styles.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/IDE/Components/Nudge/Nudge.tsx
- app/client/src/IDE/Components/Nudge/styles.ts
🔇 Additional comments (3)
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (3)
46-62: Clean implementation of nudge state management
The integration of the nudge visibility state and its cleanup in the callback is well implemented. The dependency array is properly maintained.
64-77: Maintain consistent naming convention between testid and variable name
The testid uses "minimize" while the variable uses "minimise". Consider standardizing the spelling across the codebase.
- data-testid={"t--ide-minimize"}
+ data-testid={"t--ide-minimise"}115-117: LGTM!
The tooltip placement enhancement and reuse of the memoized button component is well implemented.
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12501627600. |
|
Deploy-Preview-URL: https://ce-38347.dp.appsmith.com |
hetunandu
added
ok-to-test
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
| }, [dispatch, isAnimatedIDEEnabled]); | ||
| }, [dispatch, dismissNudge, isAnimatedIDEEnabled]); | ||
|
|
||
| const minimiseButton = useMemo( |
There was a problem hiding this comment.
What's the purpose of using component memoization here?
There was a problem hiding this comment.
There is a lint warning if I define the component before hand instead of inside the return function. I define is before hand because it is used twice.
Description
Update user communication about Side by Side mode.
Fixes #38305
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12515820871
Commit: d2b13a3
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts
List of identified flaky tests.Fri, 27 Dec 2024 16:29:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores