[WEB-4734] feat: replace emoji picker with frimousse#7639
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new emoji/icon picker UI and helpers under packages/propel, exports it, updates Popover to accept a positionerClassName, switches Logo to a local implementation that renders emojis via stringToEmoji/getEmojiSize, and integrates the new picker across web forms while simplifying emoji payloads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as Parent Component
participant Picker as EmojiPicker
participant Pop as Popover
participant Tabs as Tabs
participant Emoji as EmojiRoot
participant Icon as IconRoot
participant AppLogo as Logo
User->>Form: open logo control
Form->>Picker: render(isOpen, handleToggle, defaultOpen)
Picker->>Pop: open popover (positionerClassName forwarded)
Pop->>Tabs: render Emoji / Icon tabs
alt Emoji selected
Tabs->>Emoji: show search/skin-tone
User->>Emoji: select emoji
Emoji-->>Picker: onChange({ type: EMOJI, value: "dash-separated-decimals" })
else Icon selected
Tabs->>Icon: show icons + color picker
User->>Icon: select icon+color
Icon-->>Picker: onChange({ type: ICON, value: { name, color } })
end
Picker-->>Form: onChange(payload)
Form->>AppLogo: render updated logo_props
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/core/components/project/create/header.tsx (1)
69-99: Fix icon rendering (missing lucide type) and hardening for undefined value
Logodefaults to "material". When users pick a lucide icon, rendering will fall back to Material Symbols text unlesstype="lucide"is provided. Also,value.in_useaccess can throw ifvalueis undefined.Apply this diff:
- label={ + label={ <span className="grid h-11 w-11 place-items-center rounded-md bg-custom-background-80"> - <Logo logo={value} size={20} /> + <Logo logo={value} size={20} type="lucide" /> </span> } @@ - defaultIconColor={value.in_use && value.in_use === "icon" ? value.icon?.color : undefined} + defaultIconColor={value?.in_use && value.in_use === "icon" ? value.icon?.color : undefined} defaultOpen={ - value.in_use && value.in_use === "emoji" ? EmojiIconPickerTypes.EMOJI : EmojiIconPickerTypes.ICON + value?.in_use && value.in_use === "emoji" ? EmojiIconPickerTypes.EMOJI : EmojiIconPickerTypes.ICON }apps/web/core/components/project/form.tsx (1)
206-234: SetLogoto lucide and type the picker change payload
- Without
type="lucide", icon logos render as Material text.- Prefer a discriminated union for the picker payload to avoid shape drift.
Apply this diff:
- label={<Logo logo={value} size={28} />} + label={<Logo logo={value} size={28} type="lucide" />} @@ - onChange={(val) => { - let logoValue = {}; - - if (val?.type === "emoji") - logoValue = { - value: val.value, - }; - else if (val?.type === "icon") logoValue = val.value; - - onChange({ - in_use: val?.type, - [val?.type]: logoValue, - }); + onChange={(val: { type: "emoji" | "icon"; value: any }) => { + const payload = + val.type === "emoji" + ? { in_use: "emoji", emoji: { value: val.value } } + : { in_use: "icon", icon: val.value }; + onChange(payload as any); setIsOpen(false); }}apps/web/core/components/common/logo.tsx (1)
40-51: Don’t gate emoji rendering on Material Symbols font readinessThe skeleton placeholder is shown until the Material font loads, even for emojis which don’t depend on that font. This can cause unnecessary flashing or delayed emoji display.
Apply this diff to gate only the Material icon branch:
- if (!isMaterialSymbolsFontLoaded) { - return ( - <span - style={{ - height: size, - width: size, - }} - className="rounded animate-pulse bg-custom-background-80" - /> - ); - } + // Defer the Material Symbols loading check to the material icon branch, + // so emoji and lucide icons render immediately. @@ // emoji if (in_use === "emoji") { return ( <span className="flex items-center justify-center" style={{ fontSize: `${getEmojiSize(size)}rem`, lineHeight: `${getEmojiSize(size)}rem`, height: size, width: size, }} > {stringToEmoji(emoji?.value || "")} </span> ); } @@ // icon if (in_use === "icon") { return ( <> - {type === "lucide" ? ( + {type === "lucide" ? ( <> {lucideIcon && ( <lucideIcon.element style={{ color: color, height: size, width: size, }} /> )} </> ) : ( - <span - className="material-symbols-rounded" - style={{ - fontSize: size, - color: color, - scale: "115%", - }} - > - {value} - </span> + <> + {!isMaterialSymbolsFontLoaded ? ( + <span + style={{ height: size, width: size }} + className="rounded animate-pulse bg-custom-background-80" + /> + ) : ( + <span + className="material-symbols-rounded" + style={{ fontSize: size, color: color, scale: "115%" }} + > + {value} + </span> + )} + </> )} </> ); }Also applies to: 54-66
🧹 Nitpick comments (17)
packages/ui/src/emoji/emoji-icon-helper.tsx (1)
140-153: Fix doc mismatch (comma vs hyphen) and harden stringToEmoji against empty/invalid inputDocs mention “comma-separated” while implementation uses hyphens. Also,
stringToEmoji("")currently produces a NUL character. Guard and sanitize.Apply:
- * This creates a comma-separated string of decimal values + * This creates a hyphen-separated string of decimal values @@ -export function emojiToString(emoji: string): string { +export function emojiToString(emoji: string): string { const decimals = emojiToDecimalEnhanced(emoji); - return decimals.join("-"); + return decimals.join("-"); } @@ - * @param emojiString - Comma-separated string of decimal values + * @param emojiString - Hyphen-separated string of decimal values * @returns The reconstructed emoji string */ -export function stringToEmoji(emojiString: string): string { - const decimals = emojiString.split("-").map(Number); - return decimalToEmojiEnhanced(decimals); -} +export function stringToEmoji(emojiString: string): string { + if (!emojiString || !emojiString.trim()) return ""; + const decimals = emojiString + .split("-") + .map((s) => Number.parseInt(s, 10)) + .filter((n) => Number.isFinite(n) && n > 0); + return decimalToEmojiEnhanced(decimals); +}packages/editor/src/core/extensions/callout/logo-selector.tsx (1)
54-63: Clear stale data-emoji-url when switching to string-based emojisYou’re updating
data-emoji-unicodebut not clearingdata-emoji-url. To avoid stale attributes lingering in the document, explicitly unset it during emoji updates (assumingupdateAttributesremovesundefined).Apply:
- if (val.type === "emoji") { - newLogoValue = { - "data-emoji-unicode": val.value, - }; + if (val.type === "emoji") { + newLogoValue = { + "data-emoji-unicode": val.value, + "data-emoji-url": undefined, + }; newLogoValueToStoreInLocalStorage = { in_use: "emoji", emoji: { value: val.value, }, }; }If
updateAttributesdoes not removeundefinedkeys, set it tonullinstead or extend the utility to delete keys.packages/ui/src/emoji/logo.tsx (1)
61-73: Emoji render block: add aria-hidden for decorative usageSince this is typically a decorative logo inside pickers/buttons, hiding it from screen readers avoids redundant announcements.
Apply:
- return ( - <span + return ( + <span + aria-hidden="true" className="flex items-center justify-center" style={{ fontSize: `${getEmojiSize(size)}rem`, lineHeight: `${getEmojiSize(size)}rem`, height: size, width: size, }} > {stringToEmoji(emoji?.value || "")} </span> );apps/web/core/components/views/form.tsx (2)
177-191: Strongly type the onChange payload and avoid using an untyped intermediate objectUsing an untyped
{}forlogoValueweakens type safety and can let invalid shapes slip through. Prefer a discriminated union and build the payload in one go.Apply this diff:
- onChange={(val) => { - let logoValue = {}; - - if (val?.type === "emoji") - logoValue = { - value: val.value, - }; - else if (val?.type === "icon") logoValue = val.value; - - setValue("logo_props", { - in_use: val?.type, - [val?.type]: logoValue, - }); - setIsOpen(false); - }} + onChange={(val: { type: "emoji" | "icon"; value: any }) => { + const payload = + val.type === "emoji" + ? { in_use: "emoji", emoji: { value: val.value } } + : { in_use: "icon", icon: val.value }; + + setValue("logo_props", payload as any); + setIsOpen(false); + }}
161-166: Tailwind class typo: use flex-shrink-0Minor, but this breaks the utility and may affect layout in narrow containers.
Apply this diff:
- className="flex items-center justify-center flex-shrink0" + className="flex items-center justify-center flex-shrink-0"apps/web/core/components/project/create/header.tsx (1)
79-93: Avoidanyin onChange; use a discriminated union to preserve typesKeeps
logo_propsshape consistent and prevents accidental regressions.Apply this diff:
- onChange={(val: any) => { - let logoValue = {}; - - if (val?.type === "emoji") - logoValue = { - value: val.value, - }; - else if (val?.type === "icon") logoValue = val.value; - - onChange({ - in_use: val?.type, - [val?.type]: logoValue, - }); + onChange={(val: { type: "emoji" | "icon"; value: any }) => { + const payload = + val.type === "emoji" + ? { in_use: "emoji", emoji: { value: val.value } } + : { in_use: "icon", icon: val.value }; + onChange(payload as any); setIsOpen(false); }}apps/web/core/components/pages/modals/page-form.tsx (1)
64-105: Harden the picker button styling and keep types explicit
- Typo in Tailwind class (
flex-shrink0).- Add a typed
onChangepayload as in other components for consistency.Apply this diff:
- <EmojiIconPicker + <EmojiIconPicker isOpen={isOpen} handleToggle={(val: boolean) => setIsOpen(val)} - className="flex items-center justify-center flex-shrink0" + className="flex items-center justify-center flex-shrink-0" @@ - onChange={(val) => { + onChange={(val: { type: "emoji" | "icon"; value: any }) => { let logoValue = {}; if (val?.type === "emoji") logoValue = { value: val.value, }; else if (val?.type === "icon") logoValue = val.value; handleFormData("logo_props", { in_use: val?.type, [val?.type]: logoValue, }); setIsOpen(false); }}apps/web/core/components/project/form.tsx (1)
206-234: Optional: close-on-select behavior is contradictory
closeOnSelect={false}but you immediatelysetIsOpen(false)on change. Either remove the prop or set it totruefor clarity.Apply this diff:
- closeOnSelect={false} + closeOnSelectpackages/ui/src/emoji/emoji-icon-picker.tsx (3)
77-78: Guard against invalid defaultIndex when tab key is not foundIf
defaultOpenis not present inTABS_LIST,findIndexreturns -1. Headless UI expects a non-negativedefaultIndex. Add a safe fallback.Apply this diff:
- defaultIndex={TABS_LIST.findIndex((tab) => tab.key === defaultOpen)} + defaultIndex={Math.max(0, TABS_LIST.findIndex((tab) => tab.key === defaultOpen))}
26-27: Remove or use the unusedthemeprop
themeis destructured but never used, which may trigger lint warnings and confuses readers.Apply this diff to drop it from the destructuring:
- searchPlaceholder = "Search", - theme, + searchPlaceholder = "Search",Alternatively, if theming is intended, consider threading
themetoEmojiPickerComponent(e.g., via data-attributes or a context).
94-94: Consider responsive height instead of fixedh-80A hard height can clip the picker on smaller viewports. Prefer a max height with natural content height for better UX.
Example change:
- <Tab.Panels as="div" className="h-80 w-full overflow-hidden overflow-y-auto"> + <Tab.Panels as="div" className="max-h-[80vh] h-80 w-full overflow-hidden overflow-y-auto"This keeps the existing look while preventing overflow on short screens.
packages/ui/src/emoji/emoji-icon-picker-new.tsx (4)
98-98: Remove strayconsole.logfrom the selection handlerLeftover logging in UI components adds noise and can leak PII in some contexts.
Apply this diff:
- onEmojiSelect={(val) => { - console.log(val); + onEmojiSelect={(val) => {
77-78: Guard against invalid defaultIndex when tab key is not foundSame concern as the other picker: ensure a non-negative index.
- defaultIndex={TABS_LIST.findIndex((tab) => tab.key === defaultOpen)} + defaultIndex={Math.max(0, TABS_LIST.findIndex((tab) => tab.key === defaultOpen))}
26-27: Remove or use the unusedthemeprop
themeis destructured but unused.- searchPlaceholder = "Search", - theme, + searchPlaceholder = "Search",
107-115: Remove commented scaffoldingThe inline comment
{/* data-slot="emoji-picker-search-wrapper" */}appears to be a leftover from earlier scaffolding.- {/* data-slot="emoji-picker-search-wrapper" */}packages/ui/src/emoji/emoji-picker.tsx (2)
41-41: Verify non-standard Tailwind classoutline-hidden
outline-hiddenisn’t a standard Tailwind utility. If you don’t have a custom plugin for it, it won’t apply. Useoutline-noneinstead.- className={cn("relative flex-1 outline-hidden", className)} + className={cn("relative flex-1 outline-none", className)}If you do have a custom utility, feel free to ignore this.
67-99: Optional: stabilize thecomponentsobject withuseMemoto reduce re-rendersThe inline
componentsobject and its lambdas are recreated on every render, which can trigger re-renders downstream. Memoizing improves stability.Example:
const listComponents = React.useMemo( () => ({ CategoryHeader: ({ category, className, ...rest }: any) => ( <div {...rest} data-slot="emoji-picker-list-category-header" className={cn("bg-custom-background-100 text-custom-text-300 px-3 pb-1.5 text-xs font-medium", className)}> {category.label} </div> ), Row: ({ children, className, ...rest }: any) => ( <div {...rest} data-slot="emoji-picker-list-row" className={cn("scroll-my-1.5 px-1.5", className)}> {children} </div> ), Emoji: ({ emoji, className, ...rest }: any) => ( <button {...rest} type="button" data-slot="emoji-picker-list-emoji" className={cn("data-active:bg-accent flex size-8 items-center justify-center rounded-md text-lg", className)} aria-label={emoji.label} title={emoji.label}> {emoji.emoji} </button> ), }), [] ); // ... <BaseEmojiPicker.List components={listComponents} ... />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/web/core/components/common/logo.tsx(2 hunks)apps/web/core/components/pages/modals/page-form.tsx(2 hunks)apps/web/core/components/project/create/header.tsx(2 hunks)apps/web/core/components/project/form.tsx(2 hunks)apps/web/core/components/views/form.tsx(2 hunks)apps/web/core/store/pages/base-page.ts(1 hunks)apps/web/package.json(0 hunks)packages/editor/src/core/extensions/callout/logo-selector.tsx(2 hunks)packages/ui/package.json(1 hunks)packages/ui/src/emoji/emoji-icon-helper.tsx(3 hunks)packages/ui/src/emoji/emoji-icon-picker-new.tsx(2 hunks)packages/ui/src/emoji/emoji-icon-picker.tsx(2 hunks)packages/ui/src/emoji/emoji-picker.tsx(1 hunks)packages/ui/src/emoji/helpers.ts(0 hunks)packages/ui/src/emoji/logo.tsx(2 hunks)packages/utils/src/emoji.ts(0 hunks)
💤 Files with no reviewable changes (3)
- apps/web/package.json
- packages/utils/src/emoji.ts
- packages/ui/src/emoji/helpers.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-14T13:16:23.323Z
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/web/core/components/project/create/header.tsx
🧬 Code graph analysis (4)
packages/ui/src/emoji/logo.tsx (1)
packages/ui/src/emoji/emoji-icon-helper.tsx (2)
getEmojiSize(155-157)stringToEmoji(150-153)
apps/web/core/components/common/logo.tsx (1)
packages/ui/src/emoji/emoji-icon-helper.tsx (2)
getEmojiSize(155-157)stringToEmoji(150-153)
packages/ui/src/emoji/emoji-icon-picker.tsx (2)
packages/ui/src/emoji/emoji-picker.tsx (1)
EmojiPickerComponent(116-123)packages/ui/src/emoji/emoji-icon-helper.tsx (1)
emojiToString(140-143)
packages/ui/src/emoji/emoji-icon-picker-new.tsx (2)
packages/ui/src/emoji/emoji-picker.tsx (1)
EmojiPickerComponent(116-123)packages/ui/src/emoji/emoji-icon-helper.tsx (1)
emojiToString(140-143)
🪛 GitHub Check: Build and lint web apps
packages/ui/src/emoji/emoji-icon-helper.tsx
[failure] 155-155:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
🪛 GitHub Actions: Build and lint web apps
packages/ui/src/emoji/emoji-icon-helper.tsx
[error] 155-155: ESLint: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>. (arrow-body-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
packages/ui/package.json (1)
44-44: Cleanup verification complete – no lingeringemoji-picker-reactusage• No imports or requires of
emoji-picker-reactdetected across the repo
• No references to removed helpers (emojiCodeToUnicode,convertHexEmojiToDecimal) remain
•frimousseis correctly declared in packages/ui/package.json and imported in packages/ui/src/emoji/emoji-picker.tsx
• Lockfiles only referencefrimousse; zero occurrences ofemoji-picker-reactfoundpackages/ui/src/emoji/emoji-icon-helper.tsx (4)
19-23: Event payload for emoji now a string — consistent with storage approachThe change to
value: stringfor the emoji variant matches the new “hyphen-joined code points” storage format used elsewhere in the PR. Call sites appear to have been updated accordingly.
48-49: Theme type narrowed to literal unionSwitching
themeto"light" | "dark"is clearer and avoids importing theme types from external libs.
109-123: Emoji → decimal conversion is correct for ZWJ sequences and skin tonesUsing
Array.fromensures code point–aware iteration and preserves the full sequence (including variation selectors and ZWJs) when paired withfromCodePointon the way back.
130-133: Decimal → emoji reconstruction is sound
String.fromCodePoint(...).join("")correctly reassembles complex emoji sequences.apps/web/core/store/pages/base-page.ts (1)
443-447: ✅ Confirmed: Backend no longer expectslogo_props.emoji.urlI’ve checked the API code and migration history and found:
- No references to an
emoji.urlfield in any serializer, view, or model in apps/api/plane – the model now has a simpleemoji: CharFieldand anicon_prop: JSONField, and there’s nologo_props.emoji.urlusage in the codebase.- The Django migration 0061_project_logo_props sets any legacy
urlvalues to an empty string, and subsequent serializers/tests only surface the emoji’s value (and unicode), not a URL.- The contract test in tests/contract/app/test_project_app.py asserts a response shape of
with no"logo_props": { "in_use": "emoji", "emoji": { "value": "🚀", "unicode": "1f680" } }urlproperty present.All signs confirm the backend/API has moved to the new string-based emoji format and will not break when the front end drops the
urlfield.packages/ui/src/emoji/logo.tsx (1)
5-5: New helpers imported — matches the string-based emoji pipelineImporting
getEmojiSizeandstringToEmojikeeps sizing consistent across components and centralizes the encoding/decoding logic.apps/web/core/components/views/form.tsx (1)
22-22: Utils import cleanup looks goodRemoving the emoji conversion util aligns with the new string-based emoji flow. No issues spotted.
apps/web/core/components/project/create/header.tsx (1)
10-14: Import changes align with the refactorSwitching to
CustomEmojiIconPickerand localLogoimport is consistent with the new emoji picker and rendering path.apps/web/core/components/pages/modals/page-form.tsx (1)
10-10: Import cleanup is consistent with the migrationOnly
getTabIndexremains from utils—looks good.apps/web/core/components/project/form.tsx (1)
21-21: Import changes OKRemoving the conversion util is aligned with the new emoji value model.
apps/web/core/components/common/logo.tsx (2)
10-10: Import updates align with string-based emoji renderingUsing
stringToEmojiandgetEmojiSizeis consistent with the refactor. No issues with the import itself.
54-66: Emoji picker confirmed to emit decimal codepoints
I verified that the new picker’sonEmojiSelecthandler usesemojiToString, which always converts the selected emoji into a hyphen-separated string of decimal Unicode codepoints. Consequently,stringToEmoji—which splits on “-” and casts viaNumber()—will always receive decimal strings and will not produceNaN. No changes required.packages/ui/src/emoji/emoji-icon-picker.tsx (1)
96-119: Nice integration of the new EmojiPickerComponent and value conversionThe
onEmojiSelecthandler correctly converts the selection withemojiToStringand respectscloseOnSelect. The header composition (Search + SkinToneSelector) looks clean and extensible.packages/ui/src/emoji/emoji-icon-picker-new.tsx (1)
123-135: Align panel height strategy with the first picker or make it responsiveThis panel uses
h-80while the other file sets the second panel toh-full. The mismatch may cause subtle layout differences across screens.If
LucideIconsListvirtualizes content assuming a fixed height, keeph-80; otherwise consider:- <Tab.Panel className="h-80 w-full relative overflow-hidden overflow-y-auto"> + <Tab.Panel className="h-full w-full relative overflow-hidden overflow-y-auto">Or switch both pickers to a consistent, responsive height (e.g.,
max-h-[80vh] h-80).packages/ui/src/emoji/emoji-picker.tsx (1)
5-13: Clean, minimal wrapper for BaseEmojiPicker.RootThe compound-component approach and prop passthrough look solid, enabling a neat API for consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/ui/src/emoji/emoji-icon-picker-new.tsx (5)
94-96: Avoid nested scroll containers; keep a single vertical scroller.
Tab.Panelssets bothoverflow-hidden overflow-y-auto, and the first panel’s internal list likely scrolls too. This can cause nested scrolling and wheel-event quirks. Prefer one scroller (either here or inside the panel).Apply this diff to keep the outer container non-scrollable and let inner content handle scrolling:
- <Tab.Panels as="div" className="h-80 w-full overflow-hidden overflow-y-auto"> + <Tab.Panels as="div" className="h-80 w-full overflow-hidden">
96-105: Remove debug logging and guard against undefined selections.
console.log(val)will ship to users and pollute logs; also add a small null-guard in case the picker ever emits an incomplete value.Apply this diff:
- <EmojiPickerComponent - onEmojiSelect={(val) => { - console.log(val); - onChange({ - type: EmojiIconPickerTypes.EMOJI, - value: emojiToString(val.emoji), - }); - if (closeOnSelect) handleToggle(false); - }} - className="h-full w-full border-none p-2" - > + <EmojiPickerComponent + onEmojiSelect={(val) => { + const native = val?.emoji; + if (!native) return; + onChange({ + type: EmojiIconPickerTypes.EMOJI, + value: emojiToString(native), + }); + if (closeOnSelect) handleToggle(false); + }} + className="h-full w-full border-none p-2" + >
107-115: Simplify the search wrapper styles; remove redundant attribute selectors.You already pass
className="flex-grow"toEmojiPickerComponent.Search. The attribute selectors for[data-slot='emoji-picker-search-wrapper']are redundant and add specificity noise.Apply this diff:
- <div className="flex items-center gap-2 justify-between [&>[data-slot='emoji-picker-search-wrapper']]:flex-grow [&>[data-slot='emoji-picker-search-wrapper']]:p-0 px-1.5 py-2 sticky top-0 z-10 bg-custom-background-100"> + <div className="flex items-center gap-2 justify-between px-1.5 py-2 sticky top-0 z-10 bg-custom-background-100">
73-79: Guard defaultIndex against -1 to prevent runtime issues.If
defaultOpenis ever out-of-sync withTABS_LIST,findIndexreturns-1, which Headless UI Tabs does not accept.Apply this diff:
- defaultIndex={TABS_LIST.findIndex((tab) => tab.key === defaultOpen)} + defaultIndex={Math.max(0, TABS_LIST.findIndex((tab) => tab.key === defaultOpen))}
2-2: Close on Escape for better a11y and parity with expectations.Since open/close is managed externally, Popover’s internal ESC handling won’t update
isOpen. Add an ESC key handler so users can dismiss via keyboard.Apply this diff:
-import React, { useRef, useState } from "react"; +import React, { useEffect, useRef, useState } from "react"; @@ // close dropdown on outside click useOutsideClickDetector(containerRef, () => handleToggle(false)); + // close on Escape + useEffect(() => { + if (!isOpen) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape") handleToggle(false); + }; + document.addEventListener("keydown", onKeyDown); + return () => document.removeEventListener("keydown", onKeyDown); + }, [isOpen, handleToggle]);Also applies to: 45-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/ui/src/emoji/emoji-icon-picker-new.tsx(2 hunks)packages/ui/src/emoji/emoji-icon-picker.tsx(2 hunks)packages/ui/src/emoji/emoji-picker.tsx(1 hunks)packages/utils/src/emoji.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/utils/src/emoji.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/emoji/emoji-picker.tsx
- packages/ui/src/emoji/emoji-icon-picker.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/emoji/emoji-icon-picker-new.tsx (2)
packages/ui/src/emoji/emoji-picker.tsx (1)
EmojiPickerComponent(116-123)packages/ui/src/emoji/emoji-icon-helper.tsx (1)
emojiToString(140-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (2)
packages/ui/src/emoji/emoji-icon-picker-new.tsx (2)
96-121: Overall integration with the new EmojiPickerComponent looks clean.Good use of composed subcomponents (Search, SkinToneSelector, Content) and a sane class structure. The selection flow with
closeOnSelectreads well.
6-8: Verify emoji decimal-string contractI audited how
emojiToString(which outputs hyphen-joined decimal sequences) flows through pickers and handlers:
- Both
emoji-icon-picker.tsxandemoji-icon-picker-new.tsxemitvalue: emojiToString(val.emoji)to theonChangecallback.- Downstream consumers—such as
updatePageLogoinapps/web/core/store/pages/base-page.tsandhandleFormData("logo_props")inpage-form.tsx—acceptTChangeHandlerProps.value: stringand don’t appear to expect a native emoji character or image URL.- Rendering via the
<Logo>component then usesstringToEmoji()to convert these decimal codes back into a displayable emoji.No direct call sites were found that assume a raw emoji or URL instead of a decimal code. However, please double-check that:
- Your API endpoints or contract tests (e.g.
test_project_app.py) handle hyphenated decimal codes correctly when receiving or returninglogo_props.emoji.value.- Any legacy integrations, plugins, or custom scripts that consume
logo_propscan tolerate this format.Optional refactor: consider renaming
emojiToString→emojiToDecimalStringfor greater clarity.
…e-emoji-picker-with-frimousse
…e-emoji-picker-with-frimousse
…e-emoji-picker-with-frimousse
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/editor/src/core/extensions/callout/logo-selector.tsx (4)
22-32: Remove stale data-emoji-url and fix TLogoProps conformance
TLogoProps.emojino longer carriesurl. Keeping it here will break type-checking and runtime expectations.Apply:
- emoji: { - value: blockAttributes["data-emoji-unicode"]?.toString(), - url: blockAttributes["data-emoji-url"], - }, + emoji: { + value: blockAttributes["data-emoji-unicode"] ?? DEFAULT_CALLOUT_BLOCK_ATTRIBUTES["data-emoji-unicode"], + },
36-44: EmojiPicker has no className prop
EmojiPickerexposesbuttonClassNameanddropdownClassName, notclassName. This prop is ignored and fails TS if strict.Apply:
- className="flex-shrink-0 grid place-items-center"
55-66: Use enum constants instead of string literals for type checksPrevents drift if the enum values change and improves TS narrowing.
Apply:
- if (val.type === "emoji") { + if (val.type === EmojiIconPickerTypes.EMOJI) { @@ - } else if (val.type === "icon") { + } else if (val.type === EmojiIconPickerTypes.ICON) {
1-95: Remove all lingeringdata-emoji-urlreferences
data-emoji-urlis no longer used and should be removed across the repo:
- In
packages/editor/src/core/extensions/callout/logo-selector.tsx, drop theblockAttributes["data-emoji-url"]reference.- In
apps/api/plane/seeds/data/issues.json, remove or update anydata-emoji-urlattributes.
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/callout/logo-selector.tsx (1)
70-77: Optional: preserve last emoji when switching to icon (current behavior OK)You’re keeping
data-emoji-unicodeas-is when picking an icon, which lets users switch back without losing the previous emoji. If you prefer strict cleanliness over convenience, clear it here as well.Apply (only if you want symmetry):
newLogoValue = { "data-icon-name": val.value.name, "data-icon-color": val.value.color, + "data-emoji-unicode": undefined, };packages/editor/src/core/extensions/callout/utils.ts (2)
34-46: Harden parsing with a shape check before trusting parsedDataParsing to
TLogoPropsassumes shape correctness. Add a narrow type guard to avoid malformed localStorage crashing downstream.Example guard (outside selected lines for context):
const isLogoProps = (x: any): x is TLogoProps => x && (x.in_use === "emoji" || x.in_use === "icon") && (x.in_use === "emoji" ? typeof x.emoji?.value === "string" : typeof x.icon?.name === "string"); let parsedDataUnknown: unknown; try { parsedDataUnknown = JSON.parse(storedData); } catch { /* existing catch */ } if (!isLogoProps(parsedDataUnknown)) return fallBackValues; const parsedData = parsedDataUnknown;
61-64: Consider guarding setItem to avoid quota errorsLocalStorage can throw; a try/catch keeps the editor resilient.
Apply:
-export const updateStoredLogo = (value: TLogoProps): void => { - if (typeof window === "undefined") return; - localStorage.setItem("editor-calloutComponent-logo", JSON.stringify(value)); -}; +export const updateStoredLogo = (value: TLogoProps): void => { + if (typeof window === "undefined") return; + try { + localStorage.setItem("editor-calloutComponent-logo", JSON.stringify(value)); + } catch (e) { + console.error("Failed to persist callout logo to localStorage", e); + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/editor/src/core/extensions/callout/logo-selector.tsx(4 hunks)packages/editor/src/core/extensions/callout/utils.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/callout/logo-selector.tsx (2)
packages/propel/src/emoji-icon-picker/emoji-picker.tsx (1)
EmojiPicker(10-136)packages/editor/src/core/extensions/callout/utils.ts (1)
DEFAULT_CALLOUT_BLOCK_ATTRIBUTES(12-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/editor/src/core/extensions/callout/logo-selector.tsx (3)
87-89: Defaults wiring looks correct
defaultIconColoranddefaultOpencorrectly derive from current attrs; no change needed.
46-86: Sanity: localStorage payload shape is consistent
updateStoredLogoreceives the minimalTLogoPropspayloads for both emoji and icon branches; good.
2-5: Imports align with the new picker and typesSwitch to
@plane/propel/emoji-icon-pickerandTLogoPropsis consistent across the PR.packages/editor/src/core/extensions/callout/utils.ts (2)
2-2: Type source switch to @plane/types is correct
TLogoPropscentralization is consistent with storage and UI usage.
12-19: Defaults look sane and match picker encoding
data-emoji-unicode: "128161"aligns with string-based codepoint storage.
…e-emoji-picker-with-frimousse
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/propel/package.json (2)
41-41: Add frimousse dependency — consider pinning narrower for 0.x and validate SSR/tree-shaking.^0.3.0 is fine but still pre-1.0; if you want stricter updates, pin to ~0.3.x.
- "frimousse": "^0.3.0", + "frimousse": "~0.3.0",Also verify: no window access at import-time, package has ESM exports and sideEffects:false (tree-shakable).
47-47: use-font-face-observer as runtime dep — ensure client-only usage.Make sure calls live inside effects/dynamic client code to avoid SSR/hydration mismatches. If fonts gating is optional, lazy-load to keep initial bundle lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/propel/package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/propel/package.json (2)
46-46: tailwind-merge version entry — LGTM.
29-29: Confirm compiled artifacts foremoji-icon-pickerexport
No committed build output found underpackages/propel; ensure the build/publish step emits and includesemoji-icon-picker/index.jsandemoji-icon-picker/index.d.tsso Storybook and downstream consumers can resolve this subpath without path aliases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pnpm-workspace.yaml (2)
11-11: Keep @types/react-dom in sync with @types/react to avoid subtle type mismatches.Consider bumping to 18.3.11 as well for parity with React 18.3.x types.
Apply:
- "@types/react-dom": 18.3.1 + "@types/react-dom": 18.3.11
7-14: Confirm catalog pinning strategy.If your guideline prefers exact patch pins, this is fine; otherwise consider a patch range to reduce churn (e.g., 18.3.x). Align with repo standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
pnpm-workspace.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
pnpm-workspace.yaml (1)
10-10: Bump to @types/react 18.3.11: verify workspace-wide compatibility. The automated check didn’t list any versions nor run a full type-check (tsc not found). Manually confirm:
- All workspaces resolve
@types/reactat 18.3.11 with no mixed versions of@types/reactor@types/react-dom- A full workspace type-check (TS 5.8.3) passes without errors
…e-emoji-picker-with-frimousse
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/propel/package.json (2)
1-33: Optional: declare sideEffects for CSS to improve tree-shaking without dropping fonts CSS.Mark only the CSS export as side-effectful; everything else tree-shakeable.
Apply this diff:
{ "name": "@plane/propel", "version": "1.0.0", "private": true, "license": "AGPL-3.0", + "sideEffects": [ + "./src/styles/fonts/index.css" + ], "scripts": {
48-48: Specify Tailwind CSS peer dependency or adjusttailwind-mergeversion
Propel currently usestailwind-merge@^3.3.1(which requires Tailwind CSS v4+). Since you don’t declare anytailwindcsspeerDependency, consumers on v3 will silently break. Either:
- Add a peerDependency
"tailwindcss": ">=4.0.0"to package.json- Or if you must support Tailwind v3, pin
tailwind-mergeto^2.xinstead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/web/core/components/project/form.tsx(3 hunks)packages/propel/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/core/components/project/form.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/propel/package.json (1)
43-43: Approve frimousse integration — Published as ESM with tree-shakeable exports, ships built-in TypeScript declarations, MIT-licensed, and is a client-only React component (add “use client” in Next.js).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/ui/src/emoji/icons-list.tsx (9)
35-42: Fix font-load detection: avoid mismatched stretch value.Using stretch: "condensed" may never match the loaded face and keep placeholders forever. Prefer normal (or omit).
Apply:
const isMaterialSymbolsFontLoaded = useFontFaceObserver([ { family: `Material Symbols Rounded`, style: `normal`, - weight: `normal`, - stretch: `condensed`, + weight: `normal`, + stretch: `normal`, }, ]);
19-19: Keep activeColor in sync with defaultColor and apply contrast adjustment.Currently, activeColor won’t update if defaultColor prop changes, and initial render may skip contrast adjustment.
- const [activeColor, setActiveColor] = useState(defaultColor); + const [activeColor, setActiveColor] = useState(() => adjustColorForContrast(defaultColor)); @@ - useEffect(() => { - if (DEFAULT_COLORS.includes(defaultColor.toLowerCase())) setShowHexInput(false); - else { - setHexValue(defaultColor.slice(1, 7)); - setShowHexInput(true); - } - }, [defaultColor]); + useEffect(() => { + const isPalette = DEFAULT_COLORS.includes(defaultColor.toLowerCase()); + setShowHexInput(!isPalette); + const adjusted = adjustColorForContrast(defaultColor); + setActiveColor(adjusted); + setHexValue(adjusted.replace("#", "").slice(0, 6)); + }, [defaultColor]);Also applies to: 25-31
89-101: Also adjust palette selections for contrast.Guarantees the same behavior as hex input.
- <button + <button key={curCol} type="button" className="grid place-items-center size-5" onClick={() => { - setActiveColor(curCol); - setHexValue(curCol.slice(1, 7)); + const adjusted = adjustColorForContrast(curCol); + setActiveColor(adjusted); + setHexValue(adjusted.replace("#", "").slice(0, 6)); }} >
66-76: Preview dot should reflect the adjusted color, not raw hex.Prevents mismatch between preview and the applied color after contrast fixes.
- <span - className="h-4 w-4 flex-shrink-0 rounded-full mr-1" - style={{ - backgroundColor: `#${hexValue}`, - }} - /> + <span + className="h-4 w-4 flex-shrink-0 rounded-full mr-1" + style={{ backgroundColor: activeColor }} + />
76-87: Constrain HEX input to valid 6-digit values.Improves UX and prevents partial values.
<Input type="text" value={hexValue} onChange={(e) => { const value = e.target.value; setHexValue(value); if (/^[0-9A-Fa-f]{6}$/.test(value)) setActiveColor(adjustColorForContrast(`#${value}`)); }} - className="flex-grow pl-0 text-xs text-custom-text-200" + className="flex-grow pl-0 text-xs text-custom-text-200" + maxLength={6} + pattern="[0-9A-Fa-f]{6}" + spellCheck={false} + autoCapitalize="off" + aria-label="Hex color (6 digits)" mode="true-transparent" autoFocus />
55-61: Improve search input accessibility.Label the control and use the appropriate input type.
<Input - placeholder="Search" + placeholder="Search" + aria-label="Search icons" + type="search" value={query} onChange={(e) => setQuery(e.target.value)} className="text-[1rem] border-none p-0 h-full w-full " />
33-33: Memoize filtering to avoid recomputation on every keystroke.Small win when the icon list grows.
-import React, { useEffect, useState } from "react"; +import React, { useEffect, useMemo, useState } from "react"; @@ - const filteredArray = MATERIAL_ICONS_LIST.filter((icon) => icon.name.toLowerCase().includes(query.toLowerCase())); + const filteredArray = useMemo(() => { + const q = query.toLowerCase().trim(); + return q ? MATERIAL_ICONS_LIST.filter((icon) => icon.name.toLowerCase().includes(q)) : MATERIAL_ICONS_LIST; + }, [query]);Also applies to: 4-4
127-137: Add accessible labels to icon buttons.Helps screen readers; keeps UI unchanged.
- <button + <button key={icon.name} type="button" className="h-9 w-9 select-none text-lg grid place-items-center rounded hover:bg-custom-background-80" + aria-label={`Choose icon ${icon.name}`} + title={icon.name} onClick={() => { onChange({ name: icon.name, color: activeColor, }); }} >
7-7: Import MATERIAL_ICONS_LIST directly from its defining module
Replace the barrel import to improve treeshaking and avoid any indirect circular reference:packages/ui/src/emoji/icons-list.tsx:7
- import { MATERIAL_ICONS_LIST } from ".."; + import { MATERIAL_ICONS_LIST } from "../constants/icons";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/propel/src/emoji-icon-picker/icon/material-root.tsx(1 hunks)packages/ui/src/emoji/icons-list.tsx(1 hunks)packages/ui/src/emoji/logo.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/propel/src/emoji-icon-picker/icon/material-root.tsx
- packages/ui/src/emoji/logo.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T08:14:49.260Z
Learnt from: sriramveeraghanta
PR: makeplane/plane#7697
File: apps/web/app/(all)/[workspaceSlug]/(projects)/header.tsx:12-13
Timestamp: 2025-09-02T08:14:49.260Z
Learning: The star-us-link.tsx file in apps/web/app/(all)/[workspaceSlug]/(projects)/ already has "use client" directive at the top, making it a proper Client Component for hook usage.
Applied to files:
packages/ui/src/emoji/icons-list.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/ui/src/emoji/icons-list.tsx (1)
1-2: "use client" is appropriate here.Needed for hooks and font loading; no concerns.
Changes
Notes
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Chores