[WEB-4692]chore: added toast to propel package#7640
[WEB-4692]chore: added toast to propel package#7640sriramveeraghanta merged 11 commits intopreviewfrom
Conversation
* chore: added toast to propel package
WalkthroughAdds a new toast subsystem (components, API, and barrel), two spinner components with an index, updates build entries and package exports to expose the toast module, and makes a minor formatting tweak in a tooltip component. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant ToastAPI as Toast API
participant Manager as Toast Manager
participant UI as Toast Provider/Viewport
App->>ToastAPI: setToast({ type, title, message, ... })
ToastAPI->>Manager: create(toastData)
Manager-->>UI: state update (add toast)
UI-->>App: renders toast in viewport
App->>ToastAPI: updateToast(id, { ... })
ToastAPI->>Manager: update(id, data)
Manager-->>UI: state update (modify toast)
App->>UI: User clicks close
UI->>Manager: dismiss(id)
Manager-->>UI: state update (remove toast)
sequenceDiagram
autonumber
actor App
participant ToastAPI as Toast API
participant Manager as Toast Manager
participant UI as Toast UI
participant Promise as Async Task
Note over App,UI: Promise-driven toast lifecycle
App->>ToastAPI: setPromiseToast(promise, { loading, success, error })
ToastAPI->>Manager: create(LOADING toast)
Manager-->>UI: render loading (uses CircularBarSpinner)
ToastAPI->>Promise: await
Promise-->>ToastAPI: resolved(value)
ToastAPI->>Manager: update(id, SUCCESS toast)
Promise--x ToastAPI: rejected(error)
ToastAPI->>Manager: update(id, ERROR toast)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
|
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
🧹 Nitpick comments (8)
packages/propel/src/spinners/circular-spinner.tsx (1)
5-9: Prop forwarding and typing: accept and pass through standard SVG propsYou extend
React.SVGAttributes<SVGElement>but only useheight|width|className. Forward the rest (e.g.,aria-label,onClick,role,focusable) to<svg>for flexibility.-export interface ISpinner extends React.SVGAttributes<SVGElement> { - height?: string; - width?: string; - className?: string | undefined; -} +export interface ISpinner extends React.SVGAttributes<SVGElement> { + height?: string; + width?: string; + className?: string; +} @@ -export const Spinner: React.FC<ISpinner> = ({ height = "32px", width = "32px", className = "" }) => ( +export const Spinner: React.FC<ISpinner> = ({ height = "32px", width = "32px", className = "", ...svgProps }) => ( <div role="status"> <svg aria-hidden="true" height={height} width={width} - className={clsx("animate-spin text-custom-text-200", className)} + className={clsx("animate-spin text-custom-text-200", className)} viewBox="0 0 100 101" fill="none" xmlns="http://www.w3.org/2000/svg" + {...svgProps} >Also applies to: 11-33
packages/propel/src/spinners/circular-bar-spinner.tsx (2)
3-7: Prop forwarding + a11y: allow passing ARIA props and provide an accessible labelForward remaining SVG props and provide screen-reader text to announce progress. The current
<div role="status">has no readable contents.-interface ICircularBarSpinner extends React.SVGAttributes<SVGElement> { +interface ICircularBarSpinner extends React.SVGAttributes<SVGElement> { height?: string; width?: string; - className?: string | undefined; + className?: string; } @@ -export const CircularBarSpinner: React.FC<ICircularBarSpinner> = ({ +export const CircularBarSpinner: React.FC<ICircularBarSpinner> = ({ height = "16px", width = "16px", - className = "", -}) => ( + className = "", + ...svgProps +}) => ( <div role="status"> - <svg xmlns="http://www.w3.org/2000/svg" width={width} height={height} viewBox="0 0 24 24" className={className}> + <svg + xmlns="http://www.w3.org/2000/svg" + width={width} + height={height} + viewBox="0 0 24 24" + className={className} + aria-hidden="true" + {...svgProps} + > @@ </svg> + <span className="sr-only">Loading…</span> </div> )Also applies to: 9-16
24-31: Reduced motion: consider disabling animation for users preferring reduced motionSMIL animations don’t respect Tailwind’s
motion-safeutilities. Consider aprefers-reduced-motionmedia query to skipanimateTransform.I can draft a CSS snippet and conditional render if you’d like.
packages/propel/src/toast/index.tsx (5)
45-51: Remove unused type or prefix with_to satisfy no-unused-vars
ToastContentPropsis defined but unused, causing an ESLint warning.-type ToastContentProps = { - toastId: string | number; - icon?: React.ReactNode; - textColorClassName: string; - backgroundColorClassName: string; - borderColorClassName: string; -}; +// Removed unused ToastContentPropsAlternatively, keep it as
_ToastContentPropsif you plan to use it soon.
59-67: Optional: defaultthemeto "system" for simpler usageDefaulting props improves ergonomics for most consumers.
-export const Toast = (props: ToastProps) => ( +export const Toast = ({ theme = "system" }: ToastProps) => ( <BaseToast.Provider toastManager={toastManager}> <BaseToast.Portal> - <BaseToast.Viewport data-theme={props.theme}> + <BaseToast.Viewport data-theme={theme}> <ToastList /> </BaseToast.Viewport> </BaseToast.Portal> </BaseToast.Provider> );
88-93: Useundefinedinstead of empty fragment for absence of icon
<></>mounts an unnecessary node. Preferundefinedto reflect “no icon”.[TOAST_TYPE.INFO]: { - icon: <></>, + icon: undefined, textColorClassName: "text-toast-text-info", backgroundColorClassName: "bg-toast-background-info", borderColorClassName: "border-toast-border-info", },
101-104: Type safety: alignidtype with manager and usage
ToastListpassestoast.iddirectly; if the manager emits numeric IDs,ToastRender’sid: stringis too narrow. Make itstring | number.-const ToastList = () => { +const ToastList = () => { const { toasts } = BaseToast.useToastManager(); return toasts.map((toast) => <ToastRender key={toast.id} id={toast.id} toast={toast} />); }; @@ -const ToastRender = ({ id, toast }: { id: string; toast: BaseToast.Root.ToastObject }) => { +const ToastRender = ({ id, toast }: { id: string | number; toast: BaseToast.Root.ToastObject }) => {Also applies to: 106-110
1-8: Fix import order to satisfyimport/orderAdjust the external imports so they’re alphabetized (case-insensitive, per your ESLint config) after the special
reactgroup and before any parent imports. The correct ordering is:
- React
- External imports (alphabetically: @base-ui-components/react/toast, clsx, lucide-react)
- Parent imports (e.g. your spinner)
Apply this diff in
packages/propel/src/toast/index.tsx:-import * as React from "react"; -import clsx from "clsx"; -import { Toast as BaseToast } from "@base-ui-components/react/toast"; -import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; -// spinner -import { CircularBarSpinner } from "../spinners/circular-bar-spinner"; +import * as React from "react"; + +import { Toast as BaseToast } from "@base-ui-components/react/toast"; +import clsx from "clsx"; +import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; + +// spinner +import { CircularBarSpinner } from "../spinners/circular-bar-spinner";This ensures:
reactis in its own pathGroup and stays first.- The remaining external modules sort as:
@base-ui-components/react/toast→clsx→lucide-react(since “@” precedes letters in Unicode)- Your relative (
parent) imports follow.
📜 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/propel/src/spinners/circular-bar-spinner.tsx(1 hunks)packages/propel/src/spinners/circular-spinner.tsx(1 hunks)packages/propel/src/spinners/index.ts(1 hunks)packages/propel/src/toast/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/toast/index.tsx (1)
packages/propel/src/spinners/circular-bar-spinner.tsx (1)
CircularBarSpinner(9-35)
🪛 GitHub Check: Build and lint web apps
packages/propel/src/toast/index.tsx
[failure] 208-208:
Unexpected block statement surrounding arrow body; parenthesize the returned value and move it immediately after the =>
[failure] 198-198:
Unexpected block statement surrounding arrow body; parenthesize the returned value and move it immediately after the =>
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/toast/index.tsx
[warning] 3-3: ESLint: react import should occur before import of @base-ui-components/react/toast (import/order)
[warning] 7-7: ESLint: clsx import should occur before import of lucide-react (import/order)
[warning] 45-45: ESLint: 'ToastContentProps' is defined but never used. Allowed unused vars must match /^_/u (@typescript-eslint/no-unused-vars)
[error] 198-198: ESLint: Unexpected block statement surrounding arrow body; parenthesize the returned value and move it 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 (3)
packages/propel/src/spinners/index.ts (1)
1-2: LGTM: Barrel exports improve DXConsolidated exports are clean and make imports consistent.
packages/propel/src/toast/index.tsx (2)
115-124: Check for gesture interference:onMouseDownmay prevent swipe-to-dismissStopping propagation and default on the root can block toast-library gestures (swipe/dismiss). Confirm this is needed; otherwise remove or narrow the handler.
I can provide an A/B patch guarded by a feature flag if swiping regresses elsewhere.
Also applies to: 125-129
185-219: VerifytoastManager.promiseAPI parity with Base UI ToastThe
promisehelper is Sonner-like. Ensure@base-ui-components/react/toast’s manager supports this exact shape (loading/success/error object functions). If not, we may need a thin wrapper.I can adjust the adapter to align with Base UI’s manager API once you confirm the expected signature.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/propel/package.json (1)
29-42: Declare clsx as a direct dependency of @plane/propelThe
toastcomponent (packages/propel/src/toast/index.tsx) importsclsx, butclsxis not listed under dependencies in packages/propel/package.json. While this may resolve via hoisting in a monorepo, it will break for external consumers or when using strict package managers.Add
clsxto the dependencies of@plane/propel:--- a/packages/propel/package.json +++ b/packages/propel/package.json @@ dependencies "class-variance-authority": "^0.7.1", + "clsx": "^2.1.1", "lucide-react": "^0.469.0", "react": "^18.3.1",
♻️ Duplicate comments (1)
packages/propel/src/toast/index.tsx (1)
172-192: Pass through optional id to toastManager.add; also allow id in LOADING variantPart of a prior review still applies: id is accepted in the non-loading union but not forwarded. This blocks dedupe/update flows.
Apply:
type SetToastProps = | { + id?: string | number; type: TOAST_TYPE.LOADING; title?: string; } | { id?: string | number; type: Exclude<TOAST_TYPE, TOAST_TYPE.LOADING>; title: string; message?: string; actionItems?: React.ReactNode; }; @@ -export const setToast = (props: SetToastProps) => { - let toastId: string | undefined; - if (props.type !== TOAST_TYPE.LOADING) { - toastId = toastManager.add({ - data: { - type: props.type, - title: props.title, - message: props.message, - actionItems: props.actionItems, - }, - }); - } else { - toastId = toastManager.add({ - data: { - type: props.type, - title: props.title, - }, - }); - } - return toastId; -}; +export const setToast = (props: SetToastProps): string => { + return toastManager.add({ + id: props.id, + data: { + type: props.type, + // For LOADING, title is optional; for others it should be provided. + title: props.title ?? "Loading...", + // These will be undefined for LOADING which is fine. + message: (props as any).message, + actionItems: (props as any).actionItems, + }, + }); +};
🧹 Nitpick comments (6)
packages/propel/src/toast/index.tsx (6)
1-8: Reorder imports to satisfy import/order and silence lint warningsESLint reports import order issues. Move React to the top and place clsx before lucide-react.
Apply:
-import { Toast as BaseToast } from "@base-ui-components/react/toast"; -import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; -import * as React from "react"; -// spinner -import { CircularBarSpinner } from "../spinners/circular-bar-spinner"; -// helper -import clsx from "clsx"; +import * as React from "react"; +import clsx from "clsx"; +import { Toast as BaseToast } from "@base-ui-components/react/toast"; +import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; +// spinner +import { CircularBarSpinner } from "../spinners/circular-bar-spinner";
45-55: Remove unused ToastContentPropsThe ToastContentProps type is declared but never used. It triggers no-unused-vars.
-type ToastContentProps = { - toastId: string | number; - icon?: React.ReactNode; - textColorClassName: string; - backgroundColorClassName: string; - borderColorClassName: string; -};
88-93: INFO icon should be null, not an empty fragment, to avoid unintended spacingSetting icon: <> </> is truthy, so your render path allocates icon space (and pl-4 padding) for INFO toasts. Use null and guard with a nullish check.
[TOAST_TYPE.INFO]: { - icon: <></>, + icon: null, textColorClassName: "text-toast-text-info", backgroundColorClassName: "bg-toast-background-info", borderColorClassName: "border-toast-border-info", },- {data.icon && <div className="flex items-center justify-center">{data.icon}</div>} - <div className={clsx("w-full flex items-center gap-0.5 pr-1", data.icon ? "pl-4" : "pl-1")}> + {data.icon != null && <div className="flex items-center justify-center">{data.icon}</div>} + <div className={clsx("w-full flex items-center gap-0.5 pr-1", data.icon != null ? "pl-4" : "pl-1")}>- {data.icon && <div className="flex items-center justify-center">{data.icon}</div>} - <div className={clsx("flex flex-col gap-0.5 pr-1", data.icon ? "pl-4" : "pl-1")}> + {data.icon != null && <div className="flex items-center justify-center">{data.icon}</div>} + <div className={clsx("flex flex-col gap-0.5 pr-1", data.icon != null ? "pl-4" : "pl-1")}>Also applies to: 132-134, 152-154
106-115: Type id as string | number and drop duplicate key to avoid confusionToastList already sets the React key. Duplicating key inside BaseToast.Root is unnecessary. Also, id is typed as string but may be number.
-const ToastRender = ({ id, toast }: { id: string; toast: BaseToast.Root.ToastObject }) => { +const ToastRender = ({ id, toast }: { id: string | number; toast: BaseToast.Root.ToastObject }) => {- <BaseToast.Root - toast={toast} - key={id} + <BaseToast.Root + toast={toast}
147-149: Add aria-label to the non-loading close button for accessibilityLoading path has aria-label="Close"; the non-loading path should match.
- <BaseToast.Close className="absolute top-2 right-2.5 text-toast-text-secondary hover:text-toast-text-tertiary cursor-pointer"> + <BaseToast.Close aria-label="Close" className="absolute top-2 right-2.5 text-toast-text-secondary hover:text-toast-text-tertiary cursor-pointer">
194-200: Allow updateToast to update title/message/actionItems when providedRight now only type is updated. When flipping LOADING → SUCCESS/ERROR, callers often want to update title/message/action items too.
export const updateToast = (id: string, props: SetToastProps) => { toastManager.update(id, { data: { type: props.type, + ...(props.title !== undefined ? { title: props.title } : {}), + // Only include these when present and applicable (non-LOADING calls) + ...("message" in props && props.message !== undefined ? { message: props.message } : {}), + ...("actionItems" in props && props.actionItems !== undefined ? { actionItems: props.actionItems } : {}), }, }); };
📜 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/package.json(1 hunks)packages/propel/src/spinners/circular-spinner.tsx(1 hunks)packages/propel/src/toast/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/spinners/circular-spinner.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/toast/index.tsx (1)
packages/propel/src/spinners/circular-bar-spinner.tsx (1)
CircularBarSpinner(9-35)
🪛 GitHub Check: Build and lint web apps
packages/propel/src/toast/index.tsx
[failure] 210-210:
Trailing spaces not allowed
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/toast/index.tsx
[warning] 3-3: ESLint: 'react' import should occur before import of '@base-ui-components/react/toast'. (import/order) [Step: eslint . --max-warnings 7]
[warning] 7-7: ESLint: 'clsx' import should occur before import of 'lucide-react'. (import/order) [Step: eslint . --max-warnings 7]
[warning] 45-45: ESLint: 'ToastContentProps' is defined but never used. (no-unused-vars) [Step: eslint . --max-warnings 7]
[error] 210-210: ESLint: Trailing spaces not allowed. (no-trailing-spaces) [Step: eslint . --max-warnings 7]
⏰ 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 (2)
packages/propel/src/toast/index.tsx (2)
215-230: Nice: concise arrow functions resolve arrow-body-style lintThe success/error callbacks now use implicit returns and satisfy ESLint.
125-129: Confirm necessity of stopPropagation/preventDefault on mouse downBlocking mousedown at the root can interfere with selection or BaseToast’s swipe-to-dismiss gestures. If not strictly required, consider removing.
Would you like me to probe BaseToast’s gesture handling to ensure removing this doesn’t regress UX?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/propel/src/toast/toast.tsx (8)
112-115: Remove redundant inner key.The
keyis already used on<ToastRender … />in the parent map. Addingkey={id}on the inner<BaseToast.Root>is redundant and may confuse future maintenance.- <BaseToast.Root - toast={toast} - key={id} - className={clsx( + <BaseToast.Root + toast={toast} + className={clsx(
172-174: Return a stable toast id type fromsetToast.
toastManager.addreturns an id suitable for React keys (string or number). Returningstring | undefinedforces consumers to cast. PreferReact.Key.-export const setToast = (props: SetToastProps) => { - let toastId: string | undefined; +export const setToast = (props: SetToastProps): React.Key => { + let toastId: React.Key;
30-44: Differentiate success and error payload types forsetPromiseToast.Currently,
errorcallbacks are typed with the same generic assuccess, which is incorrect for rejected values and weakens type-safety.-type PromiseToastCallback<ToastData> = (data: ToastData) => string; -type ActionItemsPromiseToastCallback<ToastData> = (data: ToastData) => React.ReactNode; +export type PromiseToastCallback<T> = (data: T) => string; +export type ActionItemsPromiseToastCallback<T> = (data: T) => React.ReactNode; -type PromiseToastData<ToastData> = { +export type PromiseToastData<T> = { title: string; - message?: PromiseToastCallback<ToastData>; - actionItems?: ActionItemsPromiseToastCallback<ToastData>; + message?: PromiseToastCallback<T>; + actionItems?: ActionItemsPromiseToastCallback<T>; }; -type PromiseToastOptions<ToastData> = { +export type PromiseToastOptions<S, E = unknown> = { loading?: string; - success: PromiseToastData<ToastData>; - error: PromiseToastData<ToastData>; + success: PromiseToastData<S>; + error: PromiseToastData<E>; };-export const setPromiseToast = <ToastData,>( - promise: Promise<ToastData>, - options: PromiseToastOptions<ToastData> +export const setPromiseToast = <S, E = unknown>( + promise: Promise<S>, + options: PromiseToastOptions<S, E> ): void => { toastManager.promise(promise, { loading: { data: { title: options.loading ?? "Loading...", type: TOAST_TYPE.LOADING, message: undefined, actionItems: undefined, }, }, - success: (data) => ({ + success: (data: S) => ({ data: { type: TOAST_TYPE.SUCCESS, title: options.success.title, message: options.success.message?.(data), actionItems: options.success.actionItems?.(data), }, }), - error: (data) => ({ + error: (err: E) => ({ data: { type: TOAST_TYPE.ERROR, title: options.error.title, - message: options.error.message?.(data), - actionItems: options.error.actionItems?.(data), + message: options.error.message?.(err), + actionItems: options.error.actionItems?.(err), }, }), }); };Also applies to: 202-231
53-67: Provide a sensible default fortheme.Make
themeoptional and default to"system"so consumers don’t have to pass it every time.-export type ToastProps = { - theme: "light" | "dark" | "system"; -}; +export type ToastProps = { + theme?: "light" | "dark" | "system"; +}; -export const Toast = (props: ToastProps) => ( +export const Toast = ({ theme = "system" }: ToastProps) => ( <BaseToast.Provider toastManager={toastManager}> <BaseToast.Portal> - <BaseToast.Viewport data-theme={props.theme}> + <BaseToast.Viewport data-theme={theme}> <ToastList /> </BaseToast.Viewport> </BaseToast.Portal> </BaseToast.Provider> );
45-51: Remove unusedToastContentProps.This type isn’t referenced anywhere. Dead types add noise.
-type ToastContentProps = { - toastId: string | number; - icon?: React.ReactNode; - textColorClassName: string; - backgroundColorClassName: string; - borderColorClassName: string; -};
88-93: Usenullinstead of an empty fragment for the INFO icon.Slightly cleaner and avoids creating an extra node.
- [TOAST_TYPE.INFO]: { - icon: <></>, + [TOAST_TYPE.INFO]: { + icon: null,
130-144: UseBaseToast.Titlein LOADING markup for consistent a11y semantics.Loading toasts currently render plain text, while other types use
BaseToast.Title. Align them.- <div className={clsx("w-full flex items-center gap-0.5 pr-1", data.icon ? "pl-4" : "pl-1")}> - <div className={clsx("grow text-sm font-semibold", data.textColorClassName)}> - {toastData.title ?? "Loading..."} - </div> + <div className={clsx("w-full flex items-center gap-0.5 pr-1", data.icon ? "pl-4" : "pl-1")}> + <BaseToast.Title className={clsx("grow text-sm font-semibold", data.textColorClassName)}> + {toastData.title ?? "Loading..."} + </BaseToast.Title> <BaseToast.Close
69-100: Consider typingTOAST_DATAmore strictly.Nit: using
as const(or asatisfies Record<…>) would freeze shapes and catch className/shape typos at compile time. No functional change, just safer types.
📜 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/propel/src/toast/index.ts(1 hunks)packages/propel/src/toast/toast.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/toast/toast.tsx (1)
packages/propel/src/spinners/circular-bar-spinner.tsx (1)
CircularBarSpinner(9-35)
⏰ 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 (4)
packages/propel/src/toast/index.ts (1)
1-1: Barrel export looks good and aligns with the new public API surface.This keeps the surface area tidy for consumers importing from
propel/toast.packages/propel/src/toast/toast.tsx (3)
125-129: Blocking default and propagation on mousedown may impact interactions.Preventing default and propagation at the root can interfere with focus and nested interactive content. If this is meant solely to avoid dragging or selection, consider being more surgical (e.g., only on draggable areas or using
pointerdown) and verify keyboard/AT interactions.Minimal test checklist:
- Can you tab to and activate the close button?
- Do click-to-dismiss and swipe gestures still work?
- Do action items remain clickable?
If issues appear, remove the handler or scope it to the specific interaction target.
95-99: Double-check ARIA roles when the spinner is shown.
CircularBarSpinnerrenders withrole="status". If the toast root also conveys a live region, nesting may be noisy for screen readers.Quick audit suggestion:
- Inspect computed roles for
<BaseToast.Root>when loading.- If both are live regions, drop the inner role on the spinner or rely solely on the toast’s role.
17-29: ExportSetToastPropsand remove the unusedidfield
- In
packages/propel/src/toast/toast.tsx(lines 17–29):
• Addexportbefore theSetToastPropsdeclaration.
• Drop theid?: string | number;property—neithersetToastnorupdateToastever consumes it.- type SetToastProps = + export type SetToastProps = | { type: TOAST_TYPE.LOADING; title?: string; } | { - id?: string | number; type: Exclude<TOAST_TYPE, TOAST_TYPE.LOADING>; title: string; message?: string; actionItems?: React.ReactNode; };The
idprop isn’t forwarded intotoastManager.addortoastManager.update, so leaving it here may confuse downstream consumers. If you intended to allow callers to pass a custom ID, please confirm thatcreateToastManager().add()actually supports anidoption in itsToastOptions. If it does not, removingidfromSetToastPropswill keep the API surface clean.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/propel/src/toast/toast.tsx (1)
69-99: Verify Tailwind tokens exist (including secondary/tertiary text).Classes like text-toast-text-success/background/border-* and also text-toast-text-secondary/tertiary must be defined in the shared Tailwind theme; otherwise styles won’t render.
#!/bin/bash # Expect to see definitions for all tokens below in Tailwind config/theme files. tokens=( 'text-toast-text-(success|error|warning|info|loading|secondary|tertiary)' 'bg-toast-background-(success|error|warning|info|loading)' 'border-toast-border-(success|error|warning|info|loading)' ) echo "Searching Tailwind configs for toast tokens..." configs=$(fd -t f -a 'tailwind.config.*') printf "%s\n" "${configs}" for pat in "${tokens[@]}"; do echo -e "\n== Checking pattern: $pat ==" rg -nP "$pat" ${configs} || echo " → Missing in configs" doneAlso applies to: 137-139, 147-149
🧹 Nitpick comments (3)
packages/propel/src/toast/toast.tsx (3)
1-8: Fix import order to satisfy lint rules (React first, clsx before lucide).Reorder imports to match the reported warnings and typical conventions.
-import { Toast as BaseToast } from "@base-ui-components/react/toast"; -import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; -import * as React from "react"; -// spinner -import { CircularBarSpinner } from "../spinners/circular-bar-spinner"; -// helper -import clsx from "clsx"; +import * as React from "react"; +import clsx from "clsx"; +import { Toast as BaseToast } from "@base-ui-components/react/toast"; +import { AlertTriangle, CheckCircle2, X, XCircle } from "lucide-react"; +// spinner +import { CircularBarSpinner } from "../spinners/circular-bar-spinner";
45-51: Remove unused ToastContentProps to quiet lint and reduce noise.Type is never used.
-type ToastContentProps = { - toastId: string | number; - icon?: React.ReactNode; - textColorClassName: string; - backgroundColorClassName: string; - borderColorClassName: string; -};
125-128: Avoid preventing default on mousedown at Root; it can block selection and some interactions.Base UI manages gestures; this handler may hinder text selection and nested controls. Consider removing it.
- onMouseDown={(e) => { - e.stopPropagation(); - e.preventDefault(); - }}
📜 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/propel/package.json(1 hunks)packages/propel/src/toast/toast.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/package.json
🧰 Additional context used
🪛 GitHub Check: Build and lint web apps
packages/propel/src/toast/toast.tsx
[warning] 45-45:
'ToastContentProps' is defined but never used. Allowed unused vars must match /^_/u
[warning] 7-7:
clsx import should occur before import of lucide-react
[warning] 3-3:
react import should occur before import of @base-ui-components/react/toast
vamsikrishnamathala
force-pushed
the
chore-toast_migration_propel
branch
from
f787ac0 to
ebab023
Compare
|
Tsconfig should be updated with building this component. |
* chore: updated exports order
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/propel/src/toast/toast.tsx (5)
22-27: Drop unused id from SetToastProps.ID is passed separately to updateToast and not used by setToast.
| { - id?: string | number; type: Exclude<TOAST_TYPE, TOAST_TYPE.LOADING>; title: string; message?: string; actionItems?: React.ReactNode; };
138-140: Add aria-label to non-loading Close for a11y parity.Matches the loading variant.
- <BaseToast.Close className="absolute top-2 right-2.5 text-toast-text-secondary hover:text-toast-text-tertiary cursor-pointer"> + <BaseToast.Close + className="absolute top-2 right-2.5 text-toast-text-secondary hover:text-toast-text-tertiary cursor-pointer" + aria-label="Close" + >
60-90: Verify Tailwind tokens exist for all toast classes.Confirm text-toast-, bg-toast-, border-toast-* variants are defined in the shared theme; otherwise styles won’t generate.
#!/bin/bash # Expect to find definitions for success|error|warning|info|loading variants rg -n 'toast-(text|background|border)-(success|error|warning|info|loading)' -g '!**/node_modules/**' -C2
163-183: Unify toast id typing with Base UI and always return the id.Use React.Key and simplify the add logic.
-export const setToast = (props: SetToastProps) => { - let toastId: string | undefined; - if (props.type !== TOAST_TYPE.LOADING) { - toastId = toastManager.add({ - data: { - type: props.type, - title: props.title, - message: props.message, - actionItems: props.actionItems, - }, - }); - } else { - toastId = toastManager.add({ - data: { - type: props.type, - title: props.title, - }, - }); - } - return toastId; -}; +export const setToast = (props: SetToastProps): React.Key => { + const data = + props.type === TOAST_TYPE.LOADING + ? { type: TOAST_TYPE.LOADING, title: props.title } + : { + type: props.type, + title: props.title, + message: props.message, + actionItems: props.actionItems, + }; + return toastManager.add({ data }); +};
185-185: Type id as React.Key in updateToast.Matches Base UI (string | number).
-export const updateToast = (id: string, props: SetToastProps) => { +export const updateToast = (id: React.Key, props: SetToastProps) => {
🧹 Nitpick comments (8)
packages/propel/src/spinners/circular-spinner.tsx (1)
5-9: Broaden prop typings to accept number | string for size.Prevents TS friction when callers pass numbers.
export interface ISpinner extends React.SVGAttributes<SVGElement> { - height?: string; - width?: string; + height?: number | string; + width?: number | string; className?: string | undefined; }packages/propel/package.json (1)
18-38: Consider exporting spinners for external reuse.Toast uses internal imports, but consumers may want the spinners too.
"exports": { @@ "./tabs": "./dist/tabs/index.js", + "./spinners": "./dist/spinners/index.js", "./toast": "./dist/toast/index.js", "./tooltip": "./dist/tooltip/index.js", "./utils": "./dist/utils/index.js"packages/propel/src/spinners/index.ts (1)
1-2: Re-export types for better DX (after exporting them in source files).Helps consumers import component types from one place.
export * from "./circular-spinner"; export * from "./circular-bar-spinner"; +export type { ISpinner } from "./circular-spinner"; +export type { ICircularBarSpinner } from "./circular-bar-spinner";packages/propel/src/spinners/circular-bar-spinner.tsx (3)
3-7: Export the interface and widen size prop types.Aligns with circular-spinner and improves DX.
-interface ICircularBarSpinner extends React.SVGAttributes<SVGElement> { +export interface ICircularBarSpinner extends React.SVGAttributes<SVGElement> { - height?: string; - width?: string; + height?: number | string; + width?: number | string; className?: string | undefined; }
14-16: A11y: avoid nested live regions; make spinner decorative.This spinner is often embedded inside another status/announcement (e.g., toast). Remove role and hide the SVG from AT.
- <div role="status"> - <svg xmlns="http://www.w3.org/2000/svg" width={width} height={height} viewBox="0 0 24 24" className={className}> + <div> + <svg xmlns="http://www.w3.org/2000/svg" width={width} height={height} viewBox="0 0 24 24" aria-hidden="true" className={className}>
24-31: SMIL animation compatibility.SMIL works in most browsers but can be brittle; consider a CSS keyframes rotation on the or for broader compatibility and easier prefers-reduced-motion handling.
packages/propel/src/toast/toast.tsx (2)
186-199: Avoid dropping existing fields on partial updates.If message/actionItems are omitted, they’re set to undefined. Consider only writing provided keys.
- toastManager.update(id, { - data: - props.type === TOAST_TYPE.LOADING - ? { - type: TOAST_TYPE.LOADING, - title: props.title, - } - : { - type: props.type, - title: props.title, - message: props.message, - actionItems: props.actionItems, - }, - }); + toastManager.update(id, { + data: + props.type === TOAST_TYPE.LOADING + ? { + type: TOAST_TYPE.LOADING, + ...(props.title !== undefined ? { title: props.title } : {}), + } + : { + type: props.type, + ...(props.title !== undefined ? { title: props.title } : {}), + ...(props.message !== undefined ? { message: props.message } : {}), + ...(props.actionItems !== undefined ? { actionItems: props.actionItems } : {}), + }, + });
102-106: Drop redundant key on Root.Key is already set on in the map.
return ( <BaseToast.Root toast={toast} - key={id} className={cn(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/propel/package.json(1 hunks)packages/propel/src/charts/components/tooltip.tsx(0 hunks)packages/propel/src/spinners/circular-bar-spinner.tsx(1 hunks)packages/propel/src/spinners/circular-spinner.tsx(1 hunks)packages/propel/src/spinners/index.ts(1 hunks)packages/propel/src/toast/index.ts(1 hunks)packages/propel/src/toast/toast.tsx(1 hunks)packages/propel/tsdown.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/propel/src/charts/components/tooltip.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/propel/src/toast/toast.tsx (1)
packages/propel/src/spinners/circular-bar-spinner.tsx (1)
CircularBarSpinner(9-35)
⏰ 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/propel/src/spinners/circular-spinner.tsx (1)
17-29: Nice fix on SVG fills; cross-browser rendering is now correct.Using currentColor for the base ring and moving the accent color to the second path via a class is the right approach.
packages/propel/src/toast/index.ts (1)
1-1: Barrel looks good.Simple and correct re-export.
packages/propel/tsdown.config.ts (2)
20-21: Build entry addition looks correct.Toast is now included in the build outputs.
1-28: JSX and DOM libs are correctly configured inpackages/propel/tsconfig.json; no further changes needed.packages/propel/src/toast/toast.tsx (1)
1-58: API compatibility confirmed. @base-ui-components/react v1.0.0-beta.2 provides Toast.createToastManager(), the promise API, Provider, Portal, Viewport, Root, Close, Title, and Description.
4 participants
Description
In this update toast component is migrated from
SonnertoBase UI. Toast component is added topropelpackageType of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Chores
Style