Conversation
|
@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
$2K
app-store
github-actions
bot
added
🐛 bug
WalkthroughReplaced static imports of KBar exports with client-side dynamic imports using next/dynamic and SSR disabled. In packages/features/shell/Shell.tsx KBarRoot and KBarContent are lazy-loaded with a minimal hidden loading placeholder. In packages/features/shell/TopNav.tsx and packages/features/shell/navigation/Navigation.tsx KBarTrigger is lazy-loaded with SSR disabled and the same small placeholder. No public API signatures changed and JSX usage remains unchanged. Assessment against linked issues
✨ 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 |
dosubot
bot
added
the
🧹 Improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/features/shell/Shell.tsx (3)
27-31: Avoid hardcoded “Loading...” string; either localize or return null to meet i18n guidance.Since the placeholder is hidden, simplest is to render null and drop the string.
Apply this diff:
-const KBarRoot = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarRoot), { - ssr: false, - loading: () => <div className="hidden">Loading...</div>, // Hidden loading state to prevent layout shift -}); +const KBarRoot = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarRoot), { + ssr: false, + loading: () => null, +});
32-36: Same i18n/placeholder fix for KBarContent.Drop the hardcoded string.
Apply this diff:
-const KBarContent = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarContent), { - ssr: false, - loading: () => <div className="hidden">Loading...</div>, // Hidden loading state to prevent layout shift -}); +const KBarContent = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarContent), { + ssr: false, + loading: () => null, +});
27-36: Follow-up: consider a lightweight KBarTrigger entrypoint to further shrink the dynamic chunk.If KBarTrigger currently lives in the same barrel as KBarRoot/Content, split it (e.g., @calcom/features/kbar/trigger) so Trigger doesn’t drag the entire KBar tree. This typically shaves more from dev compile and initial JS.
If helpful, I can draft a minimal “trigger-only” module plus lazy import helpers used by TopNav/Navigation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/features/shell/Shell.tsx(2 hunks)packages/features/shell/TopNav.tsx(1 hunks)packages/features/shell/navigation/Navigation.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/shell/navigation/Navigation.tsxpackages/features/shell/Shell.tsxpackages/features/shell/TopNav.tsx
**/*.{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 loops
Files:
packages/features/shell/navigation/Navigation.tsxpackages/features/shell/Shell.tsxpackages/features/shell/TopNav.tsx
**/*.{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/features/shell/navigation/Navigation.tsxpackages/features/shell/Shell.tsxpackages/features/shell/TopNav.tsx
🔇 Additional comments (4)
packages/features/shell/TopNav.tsx (1)
2-2: LGTM on adopting next/dynamic.This aligns with the PR’s goal of deferring heavy KBar codepaths.
packages/features/shell/navigation/Navigation.tsx (1)
2-2: LGTM on adding next/dynamic.Consistent with the Shell/TopNav approach.
packages/features/shell/Shell.tsx (2)
4-4: LGTM on introducing next/dynamic here.Client-only lazy load for KBar root/content is the right call.
99-108: Verified: KBar only dynamically imported with ssr:false and no static imports remain.
| const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), { | ||
| ssr: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also set KBarTrigger to ssr: false here for consistent, maximal dev perf gains.
Mirrors the TopNav change; otherwise the server may still include the heavy chunk for this route.
Apply this diff:
-const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), {
- ssr: true,
-});
+const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), {
+ ssr: false,
+ loading: () => <span className="inline-block h-4 w-4" aria-hidden />,
+});📝 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.
| const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), { | |
| ssr: true, | |
| }); | |
| const KBarTrigger = dynamic( | |
| () => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), | |
| { | |
| ssr: false, | |
| loading: () => <span className="inline-block h-4 w-4" aria-hidden />, | |
| } | |
| ); |
🤖 Prompt for AI Agents
In packages/features/shell/navigation/Navigation.tsx around lines 17 to 19, the
dynamic import for KBarTrigger is using ssr: true which allows the heavy chunk
to be included on the server; change the dynamic call to use ssr: false to
prevent server-side bundling and match TopNav. Update the dynamic options to set
ssr: false, keeping the same import and export selection (mod.KBarTrigger) and
ensure the rest of the file still references KBarTrigger as before.
| const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), { | ||
| ssr: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make KBarTrigger client-only to fully avoid heavy server bundles (set ssr: false).
Keeping ssr: true can still pull the heavy KBar chunk into the server build, undermining dev-time wins. Load it client-side only and add a tiny non-shifting fallback.
Apply this diff:
-const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), {
- ssr: true,
-});
+const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), {
+ ssr: false,
+ loading: () => <span className="inline-block h-4 w-4" aria-hidden />,
+});📝 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.
| const KBarTrigger = dynamic(() => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), { | |
| ssr: true, | |
| }); | |
| const KBarTrigger = dynamic( | |
| () => import("@calcom/features/kbar/Kbar").then((mod) => mod.KBarTrigger), | |
| { | |
| ssr: false, | |
| loading: () => <span className="inline-block h-4 w-4" aria-hidden />, | |
| } | |
| ); |
🤖 Prompt for AI Agents
In packages/features/shell/TopNav.tsx around lines 12-14, the dynamic import
currently uses ssr: true which pulls the heavy KBar chunk into the server build;
change the dynamic import to ssr: false so KBarTrigger is loaded client-side
only, and provide a tiny non-shifting fallback element (e.g., a minimal
placeholder button or visually-hidden span with same dimensions) to avoid layout
jump while the client bundle loads. Ensure the import still resolves to
mod.KBarTrigger and update any typing/props usage if needed to match the
client-only component.
keithwillcode
left a comment
keithwillcode
left a comment
There was a problem hiding this comment.
I don't see any perf improvements. Also, it's been confirmed that dynamic imports are still bundled in during local dev so I don't think we'll see a benefit from trying this approach. (happy to be proven wrong though)
@keithwillcode Thanks for the feedback! However, the ~5s load time reduction comes from deferring KBar’s client-side JS (~500KB-1MB, e.g., I have attached here a detailed video showing :
Thus a reduction in ~5s was observed. Kindly watch the video and share your thoughts. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/shell/TopNav.tsx (1)
38-43: Fix a11y/HTML nesting: Link inside button is invalid.Use a single interactive element (Link styled as button) to avoid nested interactive controls.
Apply:
- <button className="hover:bg-muted hover:text-subtle text-muted rounded-full p-1 transition focus:outline-none focus:ring-2 focus:ring-black focus:ring-offset-2"> - <span className="sr-only">{t("settings")}</span> - <Link href="/settings/my-account/profile"> - <Icon name="settings" className="text-default h-4 w-4" aria-hidden="true" /> - </Link> - </button> + <Link + href="/settings/my-account/profile" + className="hover:bg-muted hover:text-subtle text-muted rounded-full p-1 transition focus:outline-none focus:ring-2 focus:ring-black focus:ring-offset-2" + aria-label={t("settings")} + > + <Icon name="settings" className="text-default h-4 w-4" aria-hidden="true" /> + </Link>
🧹 Nitpick comments (1)
packages/features/shell/TopNav.tsx (1)
35-37: Optional: Preload KBar on intent to cut first-click latency.Prefetch the chunk on pointer hover/idle without impacting initial paint.
Apply:
- <span className="hover:bg-muted hover:text-emphasis text-default group flex items-center rounded-full text-sm font-medium transition lg:hidden"> + <span + className="hover:bg-muted hover:text-emphasis text-default group flex items-center rounded-full text-sm font-medium transition lg:hidden" + onPointerEnter={() => { import("@calcom/features/kbar/Kbar"); }} + > <KBarTrigger /> </span>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/shell/TopNav.tsx(1 hunks)packages/features/shell/navigation/Navigation.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/shell/navigation/Navigation.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/shell/TopNav.tsx
**/*.{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 loops
Files:
packages/features/shell/TopNav.tsx
**/*.{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/features/shell/TopNav.tsx
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
packages/features/shell/TopNav.tsx (2)
12-15: Good call: client-only KBarTrigger with a tiny non-shifting fallback.This keeps the heavy KBar code out of the server bundle and defers client JS; the 16px placeholder avoids layout shift. Looks correct.
2-2: Client boundary: confirm or add"use client".
TopNav.tsx uses client-only hooks (useSession,useLocale) but lacks the"use client"directive; ensure it’s imported by a Client Component or add"use client"at the top.
|
As suggested by coderabbit, made ssr:false for server components (with an intention to reduce initial chunk size). |
|
This PR is being marked as stale due to inactivity. |
|
Thanks for your work. |
4 participants
What does this PR do?
The
import { KBarContent, KBarRoot } from "@calcom/features/kbar/Kbar";being heavy import is imported statically through the Shell componentpackages/features/shell/Shell.tsxThis Shell component is imported in apps/web/app/(use-page-wrapper)/(main-nav)/layout.tsx
By importing KBarContent, KBarRoot, KBarTrigger dynamically using next/dynamic, the initial load time reduces by ~5secs.
In my local, before changes the load was 14.5secs.
After this changes the load is 9.5secs.
Visual Demo (For contributors especially)
Video Demo (if applicable):
https://www.loom.com/share/f6bc2704cd024cd3a1e99e04661ff740?sid=585872e7-1d54-4302-86ed-545f50e20ad1
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?