chore: add rollup config for wds #39397
Conversation
WalkthroughThis pull request updates dependency versions in one package configuration and enhances the design system widget package. The changes include upgrading the Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
|
![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 (1)
app/client/packages/design-system/widgets/rollup.config.js (1)
29-38: Consider adding minification for production build.The output configuration looks good, but consider adding terser for production builds to optimize bundle size.
output: { file: path.resolve(BUILD_DIR, "bundle.js"), format: "esm", sourcemap: true, inlineDynamicImports: true, + plugins: [ + process.env.NODE_ENV === 'production' && terser() + ].filter(Boolean), globals: { react: "React", "react-dom": "ReactDOM", }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
app/client/package.json(1 hunks)app/client/packages/design-system/widgets/package.json(2 hunks)app/client/packages/design-system/widgets/rollup.config.js(1 hunks)app/client/packages/design-system/widgets/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/package.json
🔇 Additional comments (4)
app/client/packages/design-system/widgets/package.json (2)
3-3: Version bump from 1.0.0 to 1.0.6 seems large for build config changes.Consider using a smaller version increment (e.g., 1.0.1) since this change only adds build configuration.
41-64: LGTM! Dependencies are well organized and versions are up to date.The build tooling dependencies are properly configured with specific versions.
app/client/packages/design-system/widgets/rollup.config.js (1)
79-85: LGTM! TypeScript configuration is properly set up.The TypeScript plugin is well configured with declaration files and proper output directories.
app/client/packages/design-system/widgets/src/index.ts (1)
44-44: LGTM! Theming utilities are properly exported.The theming utilities are correctly exported from the package.
| @@ -34,12 +39,31 @@ | |||
| "usehooks-ts": "*" | |||
| }, | |||
| "devDependencies": { | |||
| "@babel/core": "^7.23.9", | |||
There was a problem hiding this comment.
We have a conflict with package versions. The build (yarn start) does not work with these versions. We have to use the same package versions that are currently used in the app.
app/client/package.json
Outdated
| @@ -423,4 +423,4 @@ | |||
| "@types/react": "^17.0.2", | |||
| "postcss": "8.4.31" | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
After installing the dependencies, the GIT shows 3 modified files, could you check and fix it?
| @@ -4,12 +4,17 @@ | |||
| "main": "src/index.ts", | |||
| "author": "Valera Melnikov <[email protected]>, Pawan Kumar <[email protected]>", | |||
| "license": "MIT", | |||
| "type": "module", | |||
There was a problem hiding this comment.
We need to check if this affects the current work of the package in the main app.
jsartisan
force-pushed
the
chore/add-rollup-config-for-wds
branch
from
8b05e28 to
a3fe76c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/client/packages/design-system/widgets/rollup.config.js (3)
27-38: Output configuration needs UMD/CJS consideration.While ESM is good for modern environments, consider adding UMD or CJS format outputs for broader compatibility.
Consider adding multiple output formats to support different environments:
output: { - file: path.resolve(BUILD_DIR, "bundle.js"), - format: "esm", + dir: BUILD_DIR, + formats: ["esm", "cjs"], + entryFileNames: "bundle.[format].js", sourcemap: true, inlineDynamicImports: true, globals: { react: "React", "react-dom": "ReactDOM", }, },
42-48: Environment variables are hardcoded to production.The current configuration always sets NODE_ENV and REACT_APP_ENV to "production", which may not be suitable for development builds.
Consider making the environment configurable:
replace({ preventAssignment: true, values: { - "process.env.NODE_ENV": JSON.stringify("production"), - "process.env.REACT_APP_ENV": JSON.stringify("production"), + "process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV || "production"), + "process.env.REACT_APP_ENV": JSON.stringify(process.env.REACT_APP_ENV || "production"), }, }),
27-93: Overall Rollup configuration is solid.The configuration is comprehensive and should work well for building the widget package. Consider adding package.json entries for "main", "module", and "types" to point to the generated files.
In your package.json, make sure to add:
{ "main": "build/bundle.js", "module": "build/bundle.js", "types": "build/index.d.ts", "files": ["build"] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/client/package.json(3 hunks)app/client/packages/design-system/widgets/package.json(2 hunks)app/client/packages/design-system/widgets/rollup.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/package.json
- app/client/packages/design-system/widgets/package.json
🔇 Additional comments (9)
app/client/packages/design-system/widgets/rollup.config.js (9)
1-4: Module imports look good.The necessary path and URL modules are properly imported, along with Rollup's defineConfig.
6-11: PostCSS plugins are correctly imported.Good selection of PostCSS plugins for handling nesting, imports, variables, and other CSS features.
12-13: Proper ESM file path handling.The code correctly uses fileURLToPath with import.meta.url to get the current file's path in an ESM context.
15-21: Rollup plugins imports are appropriate.All necessary plugins for handling JSON, resolving modules, transpiling TypeScript, and processing CSS are imported.
23-25: Constants defined clearly.BUILD_DIR and EXTERNALS are well-defined constants that improve readability and maintainability.
55-66: Babel configuration is well-defined.The Babel configuration is comprehensive with proper presets for React and TypeScript.
67-78: PostCSS setup is appropriate.The PostCSS configuration with CSS modules and the selected plugins should handle most modern CSS needs.
86-91: Module resolution configuration is good.The resolve plugin is properly configured with correct extensions and browser settings.
49-54:❓ Verification inconclusive
CommonJS configuration might need adjustment.
The
requireReturnsDefault: "preferred"setting can cause issues with some CommonJS modules.
🏁 Script executed:
#!/bin/bash # Check if there are CommonJS dependencies that might have issues with this setting grep -r "module.exports" ./node_modules --include="*.js" | head -n 5Length of output: 116
Action Required: Verify CommonJS Plugin Configuration
The configuration in
app/client/packages/design-system/widgets/rollup.config.js(lines 49–54) usesrequireReturnsDefault: "preferred", which may cause issues with CommonJS modules lacking default exports. However, our automated script couldn’t verify this because thenode_modulesdirectory wasn’t found in the repository.
- Next Steps:
- Manually Verify: Please run the build and tests in an environment with dependencies installed to confirm that no CommonJS modules are misbehaving due to this setting.
- Adjust if Needed: If issues occur, consider modifying the
requireReturnsDefaultsetting.
| typescript({ | ||
| tsconfig: "./tsconfig.json", | ||
| declaration: true, | ||
| declarationDir: path.resolve(BUILD_DIR), | ||
| rootDir: "src", | ||
| outDir: path.resolve(BUILD_DIR), | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify TypeScript configuration exists.
Make sure the referenced tsconfig.json exists and has compatible settings.
🏁 Script executed:
#!/bin/bash
# Check if tsconfig.json exists and has compatible settings
if [ -f "./tsconfig.json" ]; then
echo "tsconfig.json exists"
grep -E "\"jsx\":|\"jsxFactory\":|\"target\":|\"module\":" ./tsconfig.json
else
echo "tsconfig.json does not exist"
fiLength of output: 95
Action Required: Update the TypeScript configuration reference.
The rollup configuration references ./tsconfig.json (lines 79–85), but our check confirms that this file does not exist in the repo root. Please add the missing tsconfig.json with the necessary settings (such as "jsx", "target", "module", etc.) or update the reference path to point to the correct location.
jsartisan
force-pushed
the
chore/add-rollup-config-for-wds
branch
from
a3fe76c to
4909464
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/widgets/package.json (1)
15-16: Review new build script for package creation.
The new"build:package"script cleans thebuilddirectory and then runs Rollup. For better cross-platform compatibility (especially on Windows), consider using a tool likerimraf(e.g.,"rimraf build && rollup -c rollup.config.js") instead ofrm -rf.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/client/package.json(2 hunks)app/client/packages/design-system/widgets/package.json(2 hunks)app/client/packages/design-system/widgets/rollup.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/package.json
- app/client/packages/design-system/widgets/rollup.config.js
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (4)
app/client/packages/design-system/widgets/package.json (4)
7-7: Ensure ES module compatibility with "type": "module".
The addition of"type": "module"signals that the package is intended to be used as an ES module. Please verify that all scripts and dependencies (including the new Rollup configuration) are compatible with ESM, and that no CommonJS modules cause runtime issues.
8-10: Confirm inclusion of build artifacts via "files".
Adding the"files": ["build"]property helps ensure that only the built assets are published. Make sure that the build process reliably generates thebuildfolder before publishing the package.
42-65: Validate new devDependencies for build process enhancement.
Numerous new devDependencies (Babel presets, Rollup plugins, and related tools) have been added to support the recent build improvements. Please ensure these versions are compatible with the rest of the monorepo and do not conflict with existing dependency versions—as highlighted in previous feedback.
66-69: Validate peerDependencies for React compatibility.
The updated peer dependencies for"react"and"react-dom"should be consistent with the versions used by consuming projects. Double-check that these ranges meet the needs of both current and legacy consumers to avoid version mismatches.
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13517960615. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/packages/design-system/widgets/rollup.config.js (3)
1-3: Redundant path import.You're importing both the full
pathmodule and thedirnamefunction separately, butdirnameis already available in the fullpathmodule.import path from "path"; -import { dirname } from "path"; import { fileURLToPath } from "url";Then update line 13 to use:
const __dirname = path.dirname(__filename);
27-38: Define input and output paths as constants.Consider extracting the input and output paths as constants at the top of the file for better maintainability.
const BUILD_DIR = path.resolve(__dirname, "build"); +const INPUT_FILE = path.resolve(__dirname, "src/index.ts"); +const OUTPUT_FILE = path.resolve(BUILD_DIR, "bundle.js"); const EXTERNALS = ["react", "react-dom"]; export default defineConfig({ - input: path.resolve(__dirname, "src/index.ts"), + input: INPUT_FILE, output: { - file: path.resolve(BUILD_DIR, "bundle.js"), + file: OUTPUT_FILE, format: "esm", sourcemap: true, inlineDynamicImports: true, globals: { react: "React", "react-dom": "ReactDOM", }, },
42-48: Make environment configuration more flexible.Currently, environment values are hardcoded to "production". Consider making this configurable based on the build environment.
replace({ preventAssignment: true, values: { - "process.env.NODE_ENV": JSON.stringify("production"), - "process.env.REACT_APP_ENV": JSON.stringify("production"), + "process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV || "production"), + "process.env.REACT_APP_ENV": JSON.stringify(process.env.REACT_APP_ENV || "production"), }, }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/widgets/jest.config.js(1 hunks)app/client/packages/design-system/widgets/rollup.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/packages/design-system/widgets/jest.config.js (1)
3-3: Updated export syntax to ES Module format.This change from CommonJS
module.exportsto ES Moduleexport defaultaligns with the project's move to ES modules.app/client/packages/design-system/widgets/rollup.config.js (4)
79-85: Update the TypeScript configuration reference.The rollup configuration references
./tsconfig.json, but this file might not exist at the expected location according to previous reviews.#!/bin/bash # Check if tsconfig.json exists at specified locations if [ -f "./tsconfig.json" ]; then echo "tsconfig.json exists at repo root" elif [ -f "./app/client/packages/design-system/widgets/tsconfig.json" ]; then echo "tsconfig.json exists in the widgets directory" echo "Consider updating the reference to: tsconfig: path.resolve(__dirname, 'tsconfig.json')" else echo "tsconfig.json not found in expected locations" echo "Searching for any tsconfig.json files in the design system directory:" fd -t f "tsconfig.json" ./app/client/packages/design-system fi
67-78: The PostCSS configuration is well-structured.Good use of PostCSS plugins for nesting, imports, variables, and other CSS enhancements.
55-66: Well-configured Babel setup for React and TypeScript.The Babel configuration correctly handles JavaScript and TypeScript files with appropriate presets.
86-91: Good node-resolve configuration.The resolve plugin is properly configured to handle JS/TS extensions and deduplicate external dependencies.
|
Deploy-Preview-URL: https://ce-39397.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (3)
140-156: Leverage optional chaining for clarity.
To simplify checks onselectedWidgets, consider using optional chaining. This also aligns with the static analysis hint.- const multipleWidgetsSelected = selectedWidgets && selectedWidgets.length; + const multipleWidgetsSelected = selectedWidgets?.length;🧰 Tools
🪛 Biome (1.9.4)
[error] 141-141: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
178-391: Hotkeys array is extensive but effective.
- Each key combo is well-labeled and grouped for discoverability.
- Consider extracting repeated patterns (e.g.
stopPropagationIfWidgetSelected) into shared helpers if it grows further.
350-353: Revisit the label "Cut Widgets for grouping."
The label doesn’t fully match the actual action (groupSelectedWidget). Consider updating it for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/components/editorComponents/GlobalSearch/GlobalSearchHotKeys.tsx(1 hunks)app/client/src/git/components/HotKeys/HotKeysView.tsx(0 hunks)app/client/src/git/components/HotKeys/index.tsx(1 hunks)app/client/src/git/components/HotKeys/useHotKeysView.ts(1 hunks)app/client/src/git/index.ts(1 hunks)app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/git/components/HotKeys/HotKeysView.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx
[error] 141-141: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (12)
app/client/src/git/index.ts (1)
5-5:❓ Verification inconclusive
API change: GitHotKeys component replaced with useHotKeys hook
This change replaces the component-based approach with a hook-based approach, which aligns with modern React patterns but may affect consuming components.
🏁 Script executed:
#!/bin/bash # Check for usages of the removed GitHotKeys component rg "GitHotKeys" --type ts --type tsxLength of output: 64
Please run the above script to manually verify that there are no remaining references to
GitHotKeysin the codebase.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for any references to the removed GitHotKeys component in TypeScript files (.ts and .tsx) rg "GitHotKeys" -g "*.ts" -g "*.tsx"Length of output: 367
Action: Verify and Clarify the Import Alias Usage
The API change to replace the old GitHotKeys component with the new hook (useHotKeys) is in place via the export in
app/client/src/git/index.ts. However, in
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsxthe hook is imported as an alias—useHotKeys as useGitHotKeys—and then invoked. This aliasing may introduce confusion, so please confirm that:
- The alias
useGitHotKeysis intentionally retained for backward compatibility or clarity.- No consuming components are mistakenly treating the alias as if it were still a component.
If the alias is not needed or causes ambiguity, consider updating it to match the hook’s naming convention (e.g., using
useHotKeysdirectly).app/client/src/git/components/HotKeys/index.tsx (1)
1-8: Good refactoring to hook-based approachConverting the HotKeys component to a useHotKeys hook follows modern React best practices and improves composability.
app/client/src/components/editorComponents/GlobalSearch/GlobalSearchHotKeys.tsx (1)
2-2:❓ Verification inconclusive
Verify hotkey functionality after HotkeysTarget removal
The HotkeysTarget decorator has been removed from the import, but the component still renders Hotkeys manually. Ensure that hotkey functionality still works as expected.
🏁 Script executed:
#!/bin/bash # Check if this component is still being used and how it's being initialized rg "GlobalSearchHotKeys" --type tsx -A 3Length of output: 68
Hotkey Functionality Check – Verify Post HotkeysTarget Removal
Please ensure that the manual hotkeys behavior is still working after removing the HotkeysTarget decorator. It appears that our previous search for usages of the
GlobalSearchHotKeyscomponent usingrg --type tsxfailed due to an unrecognized file type. To verify its usage, please run the following updated script:#!/bin/bash # Search for usages of GlobalSearchHotKeys in TSX files using glob pattern matching rg "GlobalSearchHotKeys" -g '*.tsx' -A 3
- Confirm that this component is correctly integrated into the application and instantiated.
- Manually test the hotkey functionality to ensure it behaves as expected.
app/client/src/git/components/HotKeys/useHotKeysView.ts (1)
5-22: Well-implemented hook for Git hotkeysThe hook is properly implemented with useCallback for the handler and returns the necessary hotkey configuration. Good use of proper typing with the interface.
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (8)
1-3: Imports look solid.
These imports correctly utilize React hooks and TypeScript types.
9-14: Aliased imports seem consistent.
Renaming imported actions with an underscore avoids naming collisions and keeps the code clear.
38-43: New imports for preview mode and git hotkeys.
Bringing in these hooks and selectors fits the revised architecture, though ensure you keep usage consistent and tested.
45-53: Interface definition is straightforward.
Defining explicit types for props promotes clarity and scalability.
55-69: Functional component approach looks good.
The transition to hooks forGlobalHotKeysis neat, and usinguseDispatch,useSelector, and context ensures clear separation of concerns.
71-129: Callbacks for dispatch actions are well-defined.
Each callback is neatly wrapped withuseCallback; make sure you're mindful of any dependencies that might need to be added to the dep arrays in future updates.
158-174: Omnibar logic is intuitive.
Your hotkey handler properly prevents default actions and adjusts global search categories, ensuring a smooth user experience.
393-398: Final rendering with useHotkeys is clean.
Registering hotkeys viauseHotkeys(hotkeys)and returning children in a simple wrapper keeps things lightweight.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (2)
139-139: Use optional chaining for better readability.The current check can be simplified using optional chaining.
- const multipleWidgetsSelected = selectedWidgets && selectedWidgets.length; + const multipleWidgetsSelected = selectedWidgets?.length;🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-394: Consider breaking down this large component into smaller hooks.While the refactoring to a functional component is excellent, this component still has many responsibilities. Consider extracting some of the logic into custom hooks for better maintainability.
For example:
- A hook for widget selection-related hotkeys
- A hook for navigation-related hotkeys
- A hook for action execution hotkeys
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (5)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (5)
1-49: Good work on modernizing imports and function naming.The refactoring to use React hooks and improved naming conventions with underscore prefixes for imported actions helps distinguish them from local functions.
50-68: Clean component conversion from class to functional.The Props interface is well-defined, and the component properly uses hooks for state access. Good job extracting the state selectors.
69-127: Well-structured action dispatchers with proper memoization.Each dispatch function is correctly wrapped in useCallback with appropriate dependencies, preventing unnecessary re-creation of functions on re-renders.
173-389: Clean hotkey configuration with conditional rendering.The approach of using a conditional array for hotkeys based on application state is clean and maintainable. Good integration with Git hotkeys when enabled.
390-396: Simplified component rendering with Blueprint's useHotkeys hook.The rendering is appropriately simplified, letting the Blueprint library handle the hotkey registration under the hood.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (1)
56-394: 🛠️ Refactor suggestionRefactored Keyboard Shortcut Logic.
The new functional approach effectively consolidates hotkey handling. However, one improvement stands out from the static analysis:
- Lines 138-139: Replace explicit checks with optional chaining for clarity and safety:
- const multipleWidgetsSelected = selectedWidgets && selectedWidgets.length; + const multipleWidgetsSelected = selectedWidgets?.length;🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (2)
app/client/src/components/editorComponents/GlobalSearch/GlobalSearchHotKeys.tsx (1)
18-72: Great functional refactor.Switching to a functional component with
useHotkeysimproves clarity and alignment with modern React. However, the explicit cast toanyindocument.activeElement as any(line 49) may mask potential type issues.Consider assigning a more precise type or leveraging optional chaining:
- const activeElement = document.activeElement as any; + const activeElement = document.activeElement as HTMLElement | null;app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (1)
9-14: Naming consistency check.Renaming
deleteSelectedWidgetto_deleteSelectedWidgetmight reduce clarity if the underscore prefix isn't a standard in your codebase. Consider keeping naming consistent across actions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/components/editorComponents/GlobalSearch/GlobalSearchHotKeys.tsx(2 hunks)app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx(4 hunks)app/client/src/pages/Editor/GlobalHotKeys/index.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/src/pages/Editor/GlobalHotKeys/index.tsx (2)
5-5: Confirm Blueprint dependencies.This new import for
HotkeysProviderrelies on Blueprint; please verify that the required version is pinned and compatible with the rest of the dependencies.
15-23: Nicely integrated hotkeys context.Wrapping
GlobalHotKeysinsideHotkeysProvidercentralizes hotkey handling and aligns with recommended patterns in Blueprint.app/client/src/components/editorComponents/GlobalSearch/GlobalSearchHotKeys.tsx (1)
2-2: Validate Blueprint usage.Ensure the installed
@blueprintjs/coreversion supportsuseHotkeysandHotkeyConfigto avoid runtime errors.app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (2)
1-3: Functional conversion looks good.Reviewing the new imports and hook-based approach, everything appears well-organized. Good job transitioning to
useDispatchanduseSelector.
45-55: Interfaces and new imports.These lines define props and introduce additional selectors and hooks. It all seems correct and coherent with the overall Redux usage.
/ok-to-test tags="@tag.Sanity"
Summary by CodeRabbit
Summary by CodeRabbit
browserslistdependency for improved browser compatibility.GlobalHotKeysfrom class-based to functional component using React hooks for improved performance and clarity.HotKeysViewcomponent to simplify hotkey management.GlobalHotKeyswithHotkeysProviderfor enhanced context and functionality.Important
🟣 🟣 🟣 Your tests are running.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13521660461
Commit: 4287ae7
Workflow:
PR Automation test suiteTags:
@tag.SanitySpec: ``
Tue, 25 Feb 2025 12:52:54 UTC