Conversation
closes: calcom#22661
|
@hemantmm is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR makes environment access browser-safe and exposes build-time env values. packages/platform/atoms/vite.config.ts reads NEXT_PUBLIC_CALCOM_VERSION and NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA and adds them to Vite’s define (serializing values with JSON.stringify). packages/ui/components/credits/Credits.tsx guards access to process and process.env to avoid runtime ReferenceError. packages/ui/components/icon/IconSprites.tsx replaces direct process.env reads with guarded env values, computes a short commit hash defensively, and builds the sprite URL from a computed WEBAPP_URL base plus version/commit query parameter. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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
Status, Documentation and Community
|
github-actions
bot
added
Medium priority
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add community label" took an action on this PR • (08/21/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/ui/components/credits/Credits.tsx (2)
11-12: Remove duplicate ESLint disables and minimize blanket disables.The file repeats eslint-disable rules (Lines 1 and 11–12). Keep a single, scoped disable (or better, refactor to avoid needing it).
Apply this diff to drop the duplicates:
/* eslint-disable prettier/prettier */ -/* eslint-disable turbo/no-undeclared-env-vars */ +/* eslint-disable turbo/no-undeclared-env-vars */ @@ -/* eslint-disable prettier/prettier */ -/* eslint-disable turbo/no-undeclared-env-vars */Optionally, centralize env access (see IconSprites.tsx comment) to drop the turbo/no-undeclared-env-vars disable entirely.
Also applies to: 1-3
14-18: De-duplicate env retrieval across files by centralizing commit SHA.This file and IconSprites.tsx both compute VERCEL_COMMIT_SHA with identical guards. Consider adding a constant in @calcom/lib/constants (e.g., export const VERCEL_COMMIT_SHA = process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA as string;) and importing it here to remove duplication and linter disables.
I can draft the constants addition and update both call sites if you want.
packages/platform/atoms/vite.config.ts (1)
26-29: Use JSON.stringify in Vite define to avoid quoting/escaping pitfalls.Wrapping values manually in quotes can break if they contain quotes or special chars. JSON.stringify is the recommended approach and keeps the pattern consistent.
Apply this diff:
- "process.env.NEXT_PUBLIC_WEBAPP_URL": `"${webAppUrl}"`, - "process.env.NEXT_PUBLIC_CALCOM_VERSION": `"${calcomVersion}"`, - "process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA": `"${vercelCommitSha}"`, + "process.env.NEXT_PUBLIC_WEBAPP_URL": JSON.stringify(webAppUrl), + "process.env.NEXT_PUBLIC_CALCOM_VERSION": JSON.stringify(calcomVersion), + "process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA": JSON.stringify(vercelCommitSha),packages/ui/components/icon/IconSprites.tsx (2)
5-13: Good: env access is browser-safe. Consider centralizing and dropping file-wide env disables.Guarding process prevents browser crashes. To avoid duplication with Credits.tsx and remove the file-wide turbo/no-undeclared-env-vars disable, consider moving these to @calcom/lib/constants (e.g., CALCOM_VERSION and VERCEL_COMMIT_SHA) and importing them.
I can prepare a follow-up that adds VERCEL_COMMIT_SHA to @calcom/lib/constants and updates both sites.
20-20: ConvertIconSpritesto a named exportNo default imports or re-exports of
IconSpriteswere found in the codebase, so you can safely switch to a named export without needing to migrate any call sites.• In
packages/ui/components/icon/IconSprites.tsx, replace the default export:- export default IconSprites; + export { IconSprites };—or, if you prefer, declare it directly as a named export:
export function IconSprites(...) { … }• Verify any dynamic imports or tooling that might assume a default export (e.g., code generators, test setups) and update them if necessary.
📜 Review details
Configuration used: .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/platform/atoms/vite.config.ts(2 hunks)packages/ui/components/credits/Credits.tsx(1 hunks)packages/ui/components/icon/IconSprites.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/ui/components/credits/Credits.tsxpackages/ui/components/icon/IconSprites.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/ui/components/credits/Credits.tsxpackages/platform/atoms/vite.config.tspackages/ui/components/icon/IconSprites.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/ui/components/credits/Credits.tsxpackages/platform/atoms/vite.config.tspackages/ui/components/icon/IconSprites.tsx
**/*.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 APIs
Files:
packages/platform/atoms/vite.config.ts
🧬 Code graph analysis (1)
packages/ui/components/icon/IconSprites.tsx (1)
packages/lib/constants.ts (1)
CALCOM_VERSION(142-142)
⏰ 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 (2)
packages/ui/components/credits/Credits.tsx (1)
14-18: Browser-safe env access is correctly guarded.Using a typeof check around process prevents the “process is not defined” crash in non-Node browser runtimes. This directly addresses the root cause.
packages/platform/atoms/vite.config.ts (1)
10-11: Expose CALCOM_VERSION and VERCEL_COMMIT_SHA at build-time — good call.Reading them via loadEnv and inlining through define ensures downstream consumers don’t need Node globals. This is aligned with the PR goal.
Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
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 (1)
packages/ui/components/credits/Credits.tsx (1)
33-45: Add rel="noopener noreferrer" to external links opened in a new tabPrevents reverse tabnabbing and is a standard hardening for target="_blank".
-<Link href="https://go.cal.com/credits" target="_blank" className="hover:underline"> +<Link href="https://go.cal.com/credits" target="_blank" rel="noopener noreferrer" className="hover:underline"> {COMPANY_NAME} </Link> ... -<Link href="https://go.cal.com/releases" target="_blank" className="hover:underline"> +<Link href="https://go.cal.com/releases" target="_blank" rel="noopener noreferrer" className="hover:underline"> {CalComVersion} </Link> ... - <Link - href={`https://github.com/calcom/cal.com/commit/${vercelCommitHash}`} - target="_blank" - className="hover:underline"> + <Link + href={`https://github.com/calcom/cal.com/commit/${vercelCommitHash}`} + target="_blank" + rel="noopener noreferrer" + className="hover:underline"> {commitHash} </Link>
🧹 Nitpick comments (5)
packages/ui/components/credits/Credits.tsx (3)
1-3: Scope down and dedupe ESLint disablesThere are duplicate file-level disables for prettier and turbo/no-undeclared-env-vars. Keep a single, intentional disable or switch to targeted next-line disables only where needed.
Apply this minimal cleanup:
-/* eslint-disable prettier/prettier */ - -/* eslint-disable turbo/no-undeclared-env-vars */ +/* eslint-disable prettier/prettier */If you want to fully scope the turbo rule, pair this with the inline change suggested in the next comment (Lines 15-19) and drop the file-level turbo disable entirely.
Also applies to: 11-14
15-19: Browser-safe guard looks good; prefer scoping the env-rule disable to this readThe typeof-process check avoids the “process is not defined” crash. To avoid file-wide lint suppression, scope the turbo rule to just this line.
-// Use globalThis.process?.env or fallback to empty string for browser safety -const vercelCommitHash = - typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA +// Use globalThis.process?.env or fallback to empty string for browser safety +/* eslint-disable-next-line turbo/no-undeclared-env-vars */ +const vercelCommitHash = + typeof process !== "undefined" && + process.env && + process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA ? process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA : "";Optional: If we add and export VERCEL_GIT_COMMIT_SHA from @calcom/lib/constants, we can import it here and remove this env access (and the lint disable) entirely. I can open a follow-up for that.
21-22: Avoid “v.undefined-*” when CALCOM_VERSION is not setIf NEXT_PUBLIC_CALCOM_VERSION is missing, the UI can render “v.undefined-h/sh”. Provide a sane default.
-const CalComVersion = `v.${CALCOM_VERSION}-${!IS_SELF_HOSTED ? "h" : "sh"}`; +const CalComVersion = `v.${CALCOM_VERSION || "dev"}-${!IS_SELF_HOSTED ? "h" : "sh"}`;packages/ui/components/icon/IconSprites.tsx (2)
1-1: Minimize env access in shared UI pkg and narrow lint disablesAvoid file-wide turbo/no-undeclared-env-vars disable; import CALCOM_VERSION and use next-line disables only where strictly necessary. This keeps the UI package compliant with the monorepo rule and reduces runtime env reads in the browser.
-/* eslint-disable turbo/no-undeclared-env-vars */ import SVG from "react-inlinesvg"; +import { CALCOM_VERSION } from "@calcom/lib/constants"; -// Use globalThis.process?.env or fallback to empty string for browser safety -const CALCOM_VERSION = - typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_CALCOM_VERSION - ? process.env.NEXT_PUBLIC_CALCOM_VERSION - : ""; const VERCEL_COMMIT_SHA = - typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA + /* eslint-disable-next-line turbo/no-undeclared-env-vars */ + typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA ? process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA : ""; @@ export function IconSprites() { - const WEBAPP_URL = - (typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_WEBAPP_URL) || ""; + /* eslint-disable-next-line turbo/no-undeclared-env-vars */ + const WEBAPP_URL = + (typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_WEBAPP_URL) || "";If we later export VERCEL_GIT_COMMIT_SHA from @calcom/lib/constants, we can eliminate the remaining env reads and remove all lint disables. I can help wire that up.
Also applies to: 5-12, 17-18
14-14: Name nit: align constant with env varFor clarity, consider VERCEL_GIT_COMMIT_SHA to mirror NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA.
-const VERCEL_COMMIT_SHA = +const VERCEL_GIT_COMMIT_SHA = /* eslint-disable-next-line turbo/no-undeclared-env-vars */ typeof process !== "undefined" && process.env && process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA ? process.env.NEXT_PUBLIC_VERCEL_GIT_COMMIT_SHA : ""; -const commitHash = VERCEL_COMMIT_SHA ? `-${VERCEL_COMMIT_SHA.slice(0, 7)}` : ""; +const commitHash = VERCEL_GIT_COMMIT_SHA ? `-${VERCEL_GIT_COMMIT_SHA.slice(0, 7)}` : "";
📜 Review details
Configuration used: .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/platform/atoms/vite.config.ts(2 hunks)packages/ui/components/credits/Credits.tsx(1 hunks)packages/ui/components/icon/IconSprites.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/platform/atoms/vite.config.ts
🧰 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/ui/components/icon/IconSprites.tsxpackages/ui/components/credits/Credits.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/ui/components/icon/IconSprites.tsxpackages/ui/components/credits/Credits.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/ui/components/icon/IconSprites.tsxpackages/ui/components/credits/Credits.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-04T13:14:39.218Z
Learnt from: hariombalhara
PR: calcom/cal.com#22840
File: packages/app-store/webex/api/add.ts:4-4
Timestamp: 2025-08-04T13:14:39.218Z
Learning: Cal.com integrations: `WEBAPP_URL_FOR_OAUTH` intentionally falls back to `http://localhost:3000` when neither production nor development, matching the pattern used by other apps in the repo.
Applied to files:
packages/ui/components/icon/IconSprites.tsx
🧬 Code graph analysis (1)
packages/ui/components/icon/IconSprites.tsx (1)
packages/lib/constants.ts (2)
CALCOM_VERSION(142-142)WEBAPP_URL(12-18)
⏰ 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 (2)
packages/ui/components/credits/Credits.tsx (1)
36-52: Hydration-safety LGTMGating the credits/commit UI behind hasMounted prevents SSR/client mismatch when envs differ. Good call.
packages/ui/components/icon/IconSprites.tsx (1)
22-23: Bug: the slash-collapsing regex breaks “https://” into “https:/”The pattern /([^:]/)/+/g matches the “p//” in “http://”, collapsing “://” to “:/”. That will yield invalid URLs when WEBAPP_URL is set (e.g., https:/app.cal.com/icons/sprite.svg).
Since base already has trailing slashes trimmed, the extra collapse is unnecessary. Drop the replace (or use URL() for robust joining):
-const src = `${base}${path}?v=${CALCOM_VERSION}${commitHash}`.replace(/([^:]\/)\/+/g, "$1"); +const src = `${base}${path}?v=${CALCOM_VERSION}${commitHash}`;Alternatively:
-const src = `${base}${path}?v=${CALCOM_VERSION}${commitHash}`.replace(/([^:]\/)\/+/g, "$1"); +const src = + base + ? new URL(path, base).toString() + `?v=${CALCOM_VERSION}${commitHash}` + : `${path}?v=${CALCOM_VERSION}${commitHash}`;Likely an incorrect or invalid review comment.
Signed-off-by: Hemant M Mehta <hemant29mehta@gmail.com>
|
Hey @eunjae-lee, can you have a look at this PR? |
|
@Ryukemeister, can you review this PR? |
|
@Ryukemeister, can you review this PR? |
|
hey @hemantmm sorry I got sidetracked, will review this. |
There was a problem hiding this comment.
@hemantmm viteconfig changes make sense, however i'm not sure about the changes to icon sprites. whys that? also can you please record a video of before and after you fix and update the PR description, thanks!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/platform/atoms/vite.config.ts">
<violation number="1" location="packages/platform/atoms/vite.config.ts:41">
P1: Duplicate keys in `define` object. The new `JSON.stringify()` definitions are immediately overwritten by the existing template literal definitions below them (JavaScript keeps only the last value for duplicate keys). Either remove the duplicates or replace the old definitions.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Devin AI is completing this stale PRThis PR by @hemantmm has been marked as stale. A Devin session has been created to complete the remaining work. Devin will review the PR, address any feedback, and push updates to complete this PR. |
- Add back vercelCommitSha variable that was accidentally removed - Remove duplicate template literal entries, keeping only JSON.stringify() versions - Use JSON.stringify() for NODE_ENV for consistency Co-authored-by: hemantmm <hemantmm@users.noreply.github.com> Co-Authored-By: unknown <>
Summary of ChangesI've completed the remaining work on this PR to fix the atoms-in-browser issue. The main fix addresses the P1 issue identified by cubic-dev-ai regarding duplicate keys in the Changes MadeFixed
The duplicate keys issue was causing the Original PR GoalThis PR makes environment access browser-safe and exposes build-time env values for the atoms package, fixing issues #22661 and CAL-6131 where The existing changes to Completed by Devin AI on behalf of @hemantmm |
5 participants
closes: #22661
What does this PR do?
This PR makes environment access browser-safe and exposes build-time env values for the atoms package. The changes guard access to
process.envto avoid runtimeReferenceErrorin browser contexts whereprocessis not defined.Files changed:
packages/platform/atoms/vite.config.ts- Exposes env vars via Vite'sdefineconfigpackages/ui/components/credits/Credits.tsx- Guardsprocess.envaccess for browser safetypackages/ui/components/icon/IconSprites.tsx- Guardsprocess.envaccess and builds sprite URL defensivelyUpdates since last revision
Fixed P1 issue identified by cubic-dev-ai regarding duplicate keys in the
defineobject invite.config.ts:vercelCommitShavariable that was accidentally removed during a previous mergeJSON.stringify()versions (the duplicates were causing the JSON.stringify definitions to be overwritten)NODE_ENVto useJSON.stringify()for consistencyHuman Review Checklist
vite.config.tsdefine object no longer has duplicate keysvercelCommitShais properly defined before use on line 11Visual Demo (For contributors especially)
N/A - This is a build configuration fix that prevents runtime errors in browser contexts.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
cd packages/platform/atoms && yarn buildprocess is not definederrorsChecklist
Link to Devin run: https://app.devin.ai/sessions/e90251100d3d48d58706d23fc726f536
Requested by: unknown ()
Original author: @hemantmm (PR completed by Devin AI)