Skip to content

fix: broken preview embed#19223

Merged
hariombalhara merged 2 commits intomainfrom
handle-empty-embedLibUrl
Feb 11, 2025
Merged

fix: broken preview embed#19223
hariombalhara merged 2 commits intomainfrom
handle-empty-embedLibUrl

Conversation

@hariombalhara
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara commented Feb 10, 2025

What does this PR do?

Fallback when EMBED_LIB_URL is empty string

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Hariom Balhara seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hariombalhara hariombalhara changed the title Fallback if embedLibUrl is empty string fix: broken preview embed Feb 10, 2025
@keithwillcode keithwillcode added core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO labels Feb 10, 2025
const EMBED_LIB_URL = process.env.EMBED_PUBLIC_EMBED_LIB_URL ?? WEBAPP_URL;
const IS_E2E = process.env.NEXT_PUBLIC_IS_E2E === "1";
const WEBAPP_URL = process.env.EMBED_PUBLIC_WEBAPP_URL || "";
if (!WEBAPP_URL) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the check above to make it clear that EMBED_LIB_URL will always have a value.

if (!WEBAPP_URL) {
throw new Error("WEBAPP_URL is not set");
}
const EMBED_LIB_URL = process.env.EMBED_PUBLIC_EMBED_LIB_URL || WEBAPP_URL;
Copy link
Copy Markdown
Member Author

@hariombalhara hariombalhara Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed Nullish colaesing operator as we want non falsy value. Empty string isn't a valid URL causing isSafeUrlCheck to crash

Normally, env variable is undefined but the way we are defining right now in vite, we always provide empty string fallback, which is the cause of the issue.

@hariombalhara hariombalhara marked this pull request as ready for review February 10, 2025 16:01
@graphite-app graphite-app Bot requested a review from a team February 10, 2025 16:02
@dosubot dosubot Bot added the embed area: embed, widget, react embed label Feb 10, 2025
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Feb 10, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (02/10/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (02/11/25)

1 label was added to this PR based on Keith Williams's automation.

@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 11, 2025 3:11am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Feb 11, 2025 3:11am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Feb 11, 2025 3:11am

@github-actions
Copy link
Copy Markdown
Contributor

E2E results are ready!

@hariombalhara hariombalhara merged commit 552d88a into main Feb 11, 2025
@hariombalhara hariombalhara deleted the handle-empty-embedLibUrl branch February 11, 2025 09:26
MuhammadAimanSulaiman pushed a commit to hit-pay/cal.com that referenced this pull request Feb 25, 2025
Co-authored-by: Hariom Balhara <hariombalhara@gmgmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only embed area: embed, widget, react embed enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants