- Notifications
You must be signed in to change notification settings - Fork11.2k
chore: improve floating schedule button#23873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedSep 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughThe pull request updates the HTML template in packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts. It removes the mr-3 class from the button icon container and adds the ml-3 class to the button text container, adjusting horizontal spacing between the icon and label. No function signatures, exports, logic, or behavior are changed; only presentational markup is modified. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Please see thedocumentation for more information. 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 thisDiscord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts (1)
14-16:Add accessible name when rendering icon-only button.If
buttonTextis empty, this button has no accessible name (WCAG 4.1.2). Providearia-label(ideally from a prop) and fall back tobuttonText.-return `<buttonpl-mi1">+return `<button aria-label="${buttonText || 'Open scheduling'}"auto">If i18n is required, introduce abuttonAriaLabel?: stringparam and prefer it over the fallback.🧹 Nitpick comments (4)
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts (4)
34-34:Avoid phantom spacing when buttonText is empty.With
ml-3on an always-present flex item, you still get a 0‑width item that adds left margin. Either render the text node only when non-empty, or use container gaps.Option A — conditional margin only when text exists:
-<div>${buttonText}</div>+<div>${buttonText}</div>Option B — prefer Tailwind gap so spacing auto-collapses (works LTR/RTL):
- )} flex h-16 origin-center bg-red-50 transform cursor-pointer items-center+ )} flex h-16 origin-center bg-red-50 transform cursor-pointer items-center gap-x-3-<div>${buttonText}</div>+<div>${buttonText}</div>
19-19:Inline style parsing bug (missing semicolon).
z-indexis appended tocolorvalue and ignored by the browser.-style="background-color:${buttonColor}; color:${buttonTextColor} z-index: 10001">+style="background-color:${buttonColor}; color:${buttonTextColor}; z-index: 10001">
21-31:SVG attributes are JSX-style; use kebab-case in plain HTML.
strokeLinecap|strokeLinejoin|strokeWidthwon’t apply in HTML; usestroke-linecap|stroke-linejoin|stroke-width. Also mark decorative SVG as hidden from AT.- <svg-class="h-7 w-7"-fill="none"-stroke="currentColor"-viewBox="0 0 24 24"-xmlns="http://www.w3.org/2000/svg">-<path- strokeLinecap="round"- strokeLinejoin="round"- strokeWidth="2"+ <svg++ fill="none"+ stroke="currentColor"+ viewBox="0 0 24 24"+ xmlns="http://www.w3.org/2000/svg"+ aria-hidden="true" focusable="false">+ <path+ stroke-linecap="round"+ stroke-linejoin="round"+ stroke-width="2"
1-11:Prefer named export over default.Improves tree‑shaking and refactors.
-const getHtml = ({+export const getHtml = ({-export default getHtml;+// export { getHtml }; // if you keep default for now, consider switching in a follow-upThis is breaking for importers; gate behind a codemod or follow-up PR if widely used.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts(2 hunks)🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIsFiles:
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loopsFiles:
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts (1)
20-20:Spacing move off the icon looks good.Removing margin from the icon container is the right direction.
| rounded-full py-4 px-6 text-base outline-none drop-shadow-md transition focus:outline-none fo | ||
| cus:ring-4 focus:ring-gray-600 focus:ring-opacity-50 active:scale-95" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Broken Tailwind class:focus:ring-4 split across lines.
Currently rendersfo andcus:ring-4 as two invalid classes; focus ring won’t apply.
-rounded-full py-4 px-6 text-base outline-none drop-shadow-md transition focus:outline-none fo-cus:ring-4 focus:ring-gray-600 focus:ring-opacity-50 active:scale-95"+rounded-full py-4 px-6 text-base outline-none drop-shadow-md transition focus:outline-none focus:ring-4 focus:ring-gray-600 focus:ring-opacity-50 active:scale-95"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rounded-fullpy-4px-6text-baseoutline-nonedrop-shadow-mdtransitionfocus:outline-nonefo | |
| cus:ring-4focus:ring-gray-600focus:ring-opacity-50active:scale-95" | |
| rounded-fullpy-4px-6text-baseoutline-nonedrop-shadow-mdtransitionfocus:outline-nonefocus:ring-4focus:ring-gray-600focus:ring-opacity-50active:scale-95" |
🤖 Prompt for AI Agents
In packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts aroundlines 17 to 18, a Tailwind class token was accidentally split across the lineinto "fo" and "cus:ring-4", breaking the intended "focus:ring-4" class; restorethe full token by joining the split parts so the class reads exactly"focus:ring-4" (and remove any stray partial tokens) so the focus ring stylingapplies correctly.| </svg> | ||
| </div> | ||
| <div>${buttonText}</div> | ||
| <divx x-first x-last">ml-3font-semibold leading-5 antialiased">${buttonText}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
EscapebuttonText to prevent HTML injection.
buttonText is interpolated into raw HTML. Escape HTML entities to avoid XSS in embed contexts.
-<div>${buttonText}</div>+<div>${(buttonText ?? '').replace(/[&<>"']/g, (c) => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c]!) )}</div>
If you prefer clarity, extract a smallescapeHtml helper above and call it here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <divid="button"class="ml-3 font-semibold leading-5 antialiased">${buttonText}</div> | |
| <divid="button"class="ml-3 font-semibold leading-5 antialiased">${(buttonText??'').replace(/[&<>"']/g,(c)=>({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c]!))}</div> |
🤖 Prompt for AI Agents
In packages/embeds/embed-core/src/FloatingButton/FloatingButtonHtml.ts aroundline 34, the buttonText is directly interpolated into the HTML causing possibleXSS; create a small escapeHtml helper (or import a safe encoder) that replaces&, <, >, ", ' and / with their corresponding HTML entities and then useescapeHtml(buttonText) when building the div string so the rendered HTML issafe.vercelbot commentedSep 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latest updates on your projects. Learn more aboutVercel for GitHub. |
This PR is being marked as stale due to inactivity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No issues found across 1 file
5f8db05 intomainUh oh!
There was an error while loading.Please reload this page.
this allows us to have a button without text without breaking the UI
before
after
