Repository navigation
perf(extension): reduce extension bundle size with lighter runtime deps - #3936
Conversation
Replace heavier runtime dependencies and simplify popup controls to lower extension bundle size while preserving existing behavior and test coverage.
Extension Size Change: -36.30 KB ✅
This commit looks good, cheers 👏 |
📝 WalkthroughWalkthroughThis PR migrates from the ahooks library to Mantine hooks across the codebase. It updates package.json files to add 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Extension Size Change: -36.30 KB ✅
This commit looks good, cheers 👏 |
…-hot-toast refactor(extension): update env configuration using @t3-oss/env-core refactor(extension): implement Combobox component for improved person selection chore(deps): update dependencies and remove react-hot-toast
Extension Size Change: -14.25 KB ✅
This commit looks good, cheers 👏 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/package.json (1)
19-32:⚠️ Potential issue | 🟡 MinorRemoval of
tailwind-mergechangescn()class conflict resolution behavior.The removal from peerDependencies changes how conflicting Tailwind classes are handled. Previously,
tailwind-mergewould intelligently resolve conflicts (e.g.,cn("p-2", "p-4")→"p-4"). Now with onlyclsx, both classes are concatenated, with the last one winning via CSS cascade.Inspection of cn() usage across the codebase shows most calls follow safe patterns where className overrides are positioned last, making them naturally win in the CSS cascade. No obvious conflicting utility pairs were detected. However, some components pass multiple class strings that could theoretically have conflicts; runtime testing should verify no regressions occur.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/package.json` around lines 19 - 32, Removing tailwind-merge from peerDependencies changes cn() behavior so conflicting Tailwind utilities are no longer merged; restore deterministic class resolution by either re-adding "tailwind-merge" to peerDependencies or updating the cn() implementation to call tailwind-merge (or an equivalent merge function) after clsx; search for the cn function and calls in the codebase and ensure cn(...) uses tailwind-merge to collapse conflicting utilities (e.g., resolve "p-2" vs "p-4") and add tests or runtime checks for components that pass multiple class strings to validate no regressions.
🧹 Nitpick comments (1)
packages/ui/package.json (1)
28-30: Consider alphabetizing peerDependencies.
react-domappearing aftersonnerbreaks the alphabetical ordering convention typically used in package.json. This is a minor consistency nit.♻️ Suggested reordering
"react": "19.2.4", - "sonner": "2.0.7", "react-dom": "19.2.4", + "sonner": "2.0.7", "tailwindcss": "4.2.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/package.json` around lines 28 - 30, The peerDependencies list in package.json is out of alphabetical order; reorder the entries so keys are alphabetized (e.g., place "react-dom" after "react" and before "sonner") to maintain consistency in the package.json file; update the "react", "react-dom", and "sonner" entries accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ui/package.json`:
- Around line 19-32: Removing tailwind-merge from peerDependencies changes cn()
behavior so conflicting Tailwind utilities are no longer merged; restore
deterministic class resolution by either re-adding "tailwind-merge" to
peerDependencies or updating the cn() implementation to call tailwind-merge (or
an equivalent merge function) after clsx; search for the cn function and calls
in the codebase and ensure cn(...) uses tailwind-merge to collapse conflicting
utilities (e.g., resolve "p-2" vs "p-4") and add tests or runtime checks for
components that pass multiple class strings to validate no regressions.
---
Nitpick comments:
In `@packages/ui/package.json`:
- Around line 28-30: The peerDependencies list in package.json is out of
alphabetical order; reorder the entries so keys are alphabetized (e.g., place
"react-dom" after "react" and before "sonner") to maintain consistency in the
package.json file; update the "react", "react-dom", and "sonner" entries
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d79497f-f7ac-47a7-8641-54235f48ad12
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/extension/package.jsonpackages/ui/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/extension/package.json
Extension Size Change: -6.09 KB ✅
This commit looks good, cheers 👏 |
|
Extension version is updated from |
|
/gemini review |
Extension Size Change: -6.09 KB ✅
This commit looks good, cheers 👏 |
There was a problem hiding this comment.
Code Review
This pull request successfully replaces ahooks with the lighter @mantine/hooks to reduce the extension's bundle size. The refactoring of state management hooks like useDisclosure and useDebouncedValue is well-executed. However, the migration to @mantine/hooks' useHotkeys has introduced functional regressions for several keyboard shortcuts. Specifically, Escape, mod+S (Save), and mod+F (Search) no longer function when an input field is focused, which was not the case with the previous implementation. These issues can be resolved by adding the { allowInInputs: true } option to the respective useHotkeys calls to restore the original, expected behavior.
Check if the Pull Request fulfils these requirements
Greptile Summary
This PR replaces
ahookswith@mantine/hooksacross the extension and shared packages to reduce bundle size, migratinguseDebounce→useDebouncedValue/useDebouncedState,useKeyPress/useEventListener→useHotkeys,useResponsive→useMediaQuery,useSize→useElementSize, anduseStateopen/close pairs →useDisclosure.Key concerns from this review (excluding previously discussed threads):
removePersonFromBookmarktest missing deselection assertion (apps/extension/tests/utils/bookmarks-panel.ts:154–159):addPersonToBookmarkasserts the person name is visible after selection, but the removal function omits the equivalent negative assertion. If@base-ui/react'sSelectdoes not support toggle-deselect for an already-selected option, the test passes silently while removal silently fails.usePlatform.tspotential initial layout flash:useMediaQueryfrom Mantine may returnundefinedon the first render when using its SSR-safe deferred initialization mode.isMobile = !undefined = truemeans the hook incorrectly reports "mobile" on first render inapps/web, which could cause a visible layout shift. Consider settinggetInitialValueInEffect: falseto restore the synchronous behavior of the originaluseResponsive.Confidence Score: 2/5
useHotkeyswithout{ allowInInputs: true }silently drops keyboard shortcuts (Escape, mod+S, mod+F) when focus is inside input fields — regressions for core workflows. ThePersonSelectmigration removed the search/filter capability and has an unverified render-function-child pattern. The test for person removal lacks a deselection assertion. TheuseMediaQuerychange may introduce an initial layout flash in the web app. Combined, these represent meaningful behavioral regressions for an ostensibly performance-only change.apps/extension/src/entrypoints/popup/components/Global.tsx,packages/shared/src/components/Search.tsx,apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarksHeader.tsx(hotkey input-suppression regressions),packages/shared/src/hooks/usePlatform.ts(initial media query value), andapps/extension/tests/utils/bookmarks-panel.ts(missing deselection assertion).Comments Outside Diff (1)
apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarksHeader.tsx, line 205-216 (link)mod+Ssave won't fire when focus is in a form inputuseHotkeysfrom@mantine/hookssilently skips hotkeys when the focused element is anINPUT,TEXTAREA, orSELECT. The save shortcut (mod+S) is most commonly used while the user's cursor is still inside a bookmark title or URL field, which means the shortcut will never fire in that scenario — a direct functional regression from theuseKeyPressimplementation, which had no such restriction.Add
{ allowInInputs: true }to the options to match the original behavior:Prompt To Fix With AI
Last reviewed commit: ae9f07b