fix: design improvements for log in screen for cal video#22359
fix: design improvements for log in screen for cal video#22359CarinaWolli merged 91 commits intocalcom:mainfrom
Conversation
Signed-off-by: bandhan-majumder <bandhanmajumder16@gmail.com>
|
@bandhan-majumder 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
⚡ Quick Wins
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/09/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/09/25)1 label was added to this PR based on Keith Williams's automation. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
|
hi @Devanshusharma2005 , can I get a review if you get some time |
…r/cal.com into fix-21754-design
|
hey @Udit-takkar , can I get a review on this PR? Please let me know if any change is required. |
WalkthroughAdds a call-frame lifecycle and guest join flow to the video view: new state (userNameForCall, isUserNameConfirmed, isCallFrameReady), createCallFrame(userName?) to build/join a DailyIframe with themed options and localized tray labels, useEffect-driven join/cleanup, and guards for login overlay and name confirmation. Reworks LogInOverlay to accept guest name input, Enter-key handling, validation, loading/error states, and sign-in redirect. Prefills guest name from overrideName or loggedInUserName. Adds English localization strings for recording, transcription, and guest UI. Assessment against linked issues
Out-of-scope changes
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/lib/server/locales/en/common.json (1)
649-661: Potential duplication with existing “continue_as_guest” and semantic overlapThe new keys:
"continue_as_guest": "Continue as Guest", "join_as_guest": "Join as Guest", "ideal_for_one_time_calls": "Ideal for one-time calls",all describe the guest-join action. Unless the UI explicitly needs two different verbs (“Continue” vs “Join”), consider consolidating to a single key to avoid unnecessary translation effort and future maintenance confusion.
If both are really needed, add developer comments in the JSON explaining the contextual difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/web/modules/videos/views/videos-single-view.tsx(6 hunks)packages/lib/server/locales/en/common.json(2 hunks)
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (5)
apps/web/modules/videos/views/videos-single-view.tsx (5)
45-50: Well-structured state management for user flow control.The state initialization properly handles the fallback logic and correctly ties the confirmation state to the overlay display condition.
51-126: Excellent refactoring of call frame creation logic.The extraction into a
useCallbackimproves code organization and reusability. The error handling with fallback to existing instance is a good defensive programming practice.
175-185: Good defensive rendering practice.Conditioning the transcription UI on
isCallFrameReadyprevents potential runtime errors from accessing the call frame before it's initialized.
349-403: Excellent UI implementation with clear user flows.The redesigned overlay provides a clean separation between guest and sign-in options, with proper input validation, loading states, and accessibility features like autoFocus.
278-278: Effect dependency is correct
I verified thatstartingTime(derived fromprops.startTime) is included in theuseEffectdependency array in apps/web/modules/videos/views/videos-single-view.tsx. The effect will re-run whenever the start time changes, so no changes are needed here.Likely an incorrect or invalid review comment.
|
@kart1ka My guess is: |
|
@kart1ka |
|
@kart1ka WDYT |
|
The local development is too slow. It's in the issue section now too here. That's taking time while signing up and upon double clicking, it's showing that. |
|
@kart1ka can you please check once. The last time it caused because of the slow environment. I have added a loom where it did not happen |
|
This PR is being marked as stale due to inactivity. |
|
@kart1ka @Devanshusharma2005 @anikdhabal |
I will review it today. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
apps/web/modules/videos/views/videos-single-view.tsx (5)
208-217: Gate overlay to guests only; stop passing auth-related props.Render the overlay only when the user truly needs it (guest without overrideName). This also aligns with earlier feedback to remove
isLoggedInusage in the overlay.- {displayLogInOverlay && !isUserNameConfirmed && ( - <LogInOverlay - isLoggedIn={!!loggedInUserName} - bookingUid={booking.uid} - loggedInUserName={loggedInUserName ?? undefined} - overrideName={overrideName} - onJoinAsGuest={handleJoinAsGuest} - onUserNameConfirmed={handleUserNameConfirmed} - /> - )} + {displayLogInOverlay && !isUserNameConfirmed && !loggedInUserName && !overrideName && ( + <LogInOverlay bookingUid={booking.uid} onJoinAsGuest={handleJoinAsGuest} /> + )}
293-300: TrimLogInOverlayPropsto guest-only needs.Remove
isLoggedIn,loggedInUserName,overrideName, andonUserNameConfirmedfrom props; parent now gates visibility.interface LogInOverlayProps { - isLoggedIn: boolean; bookingUid: string; - loggedInUserName?: string; - overrideName?: string; onJoinAsGuest: (guestName: string) => void; - onUserNameConfirmed?: () => void; }
302-311: Simplify overlay internal state (guest-only) and fix unused confirm callback.Default dialog open for guests; drop prefilled names and
onUserNameConfirmed.export function LogInOverlay(props: LogInOverlayProps) { const { t } = useLocale(); - const { bookingUid, isLoggedIn, loggedInUserName, overrideName, onJoinAsGuest, onUserNameConfirmed } = - props; + const { bookingUid, onJoinAsGuest } = props; - const [isOpen, setIsOpen] = useState(!isLoggedIn); - const [userName, setUserName] = useState(overrideName ?? loggedInUserName ?? ""); + const [isOpen, setIsOpen] = useState(true); + const [userName, setUserName] = useState(""); const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState<string | null>(null); const handleContinueAsGuest = useCallback(async () => { const trimmedName = userName.trim(); if (!trimmedName) { return; } setIsLoading(true); setError(null); try { onJoinAsGuest(trimmedName); - onUserNameConfirmed?.(); setIsOpen(false); } catch (error) { console.error("Error joining as guest:", error); const errorMessage = error instanceof Error ? error.message : t("failed_to_join_call"); setError(errorMessage); } finally { setIsLoading(false); } - }, [userName, onJoinAsGuest, onUserNameConfirmed, t]); + }, [userName, onJoinAsGuest, t]);Also applies to: 312-334
407-414: URL-encodecallbackUrlin login redirect.Prevents broken redirects and query parsing issues.
- onClick={() => - (window.location.href = `${WEBAPP_URL}/auth/login?callbackUrl=${WEBAPP_URL}/video/${bookingUid}`) - }> + onClick={() => { + const callbackUrl = encodeURIComponent(`${WEBAPP_URL}/video/${bookingUid}`); + window.location.href = `${WEBAPP_URL}/auth/login?callbackUrl=${callbackUrl}`; + }}>
50-51: Auto-confirm username when authenticated oroverrideNameexists.Prevents rendering the (now guest-only) overlay on logged-in or preset-name paths.
- const [isUserNameConfirmed, setIsUserNameConfirmed] = useState<boolean>(!displayLogInOverlay); + const [isUserNameConfirmed, setIsUserNameConfirmed] = useState<boolean>( + !displayLogInOverlay || !!loggedInUserName || !!overrideName + );
🧹 Nitpick comments (2)
apps/web/modules/videos/views/videos-single-view.tsx (2)
108-110: Remove redundantsetUserNameafter passinguserNametocreateFrame.It's already provided via the
userNameoption.- if (userName) { - callFrame.setUserName(userName); - }
120-135: SetisCallFrameReadyfrom Daily events instead of immediately.Avoids UI flashes before the frame is actually ready.
- setIsCallFrameReady(true); - - callFrame?.join(); + callFrame?.on("loaded", () => setIsCallFrameReady(true)); + callFrame?.join();Also applies to: 173-184
📜 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 (1)
apps/web/modules/videos/views/videos-single-view.tsx(7 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:
apps/web/modules/videos/views/videos-single-view.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:
apps/web/modules/videos/views/videos-single-view.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:
apps/web/modules/videos/views/videos-single-view.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
apps/web/modules/videos/views/videos-single-view.tsx
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
apps/web/modules/videos/views/videos-single-view.tsx
🧬 Code graph analysis (1)
apps/web/modules/videos/views/videos-single-view.tsx (2)
packages/lib/constants.ts (3)
RECORDING_DEFAULT_ICON(204-206)TRANSCRIPTION_STOPPED_ICON(200-202)WEBAPP_URL(12-18)apps/web/modules/videos/cal-video-premium-features.tsx (1)
CalVideoPremiumFeatures(163-247)
⏰ 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). (4)
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
apps/web/modules/videos/views/videos-single-view.tsx (1)
113-115: Verify fallback togetCallInstance()is intended.If
createFramefails, returning a stale/global instance may attach to an unexpected call. Consider returningnulland handling gracefully.Would you like me to scan usages of
DailyIframe.getCallInstance()in the repo to confirm expectations?
|
@kart1ka |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
apps/web/modules/videos/views/videos-single-view.tsx (6)
304-311: Default dialog open and remove auth-derived defaultsWith guest-only rendering, open by default and start with empty name.
-export function LogInOverlay(props: LogInOverlayProps) { +export function LogInOverlay(props: LogInOverlayProps) { const { t } = useLocale(); - const { bookingUid, isLoggedIn, loggedInUserName, overrideName, onJoinAsGuest, onUserNameConfirmed } = - props; + const { bookingUid, onJoinAsGuest } = props; - const [isOpen, setIsOpen] = useState(!isLoggedIn); - const [userName, setUserName] = useState(overrideName ?? loggedInUserName ?? ""); + const [isOpen, setIsOpen] = useState(true); + const [userName, setUserName] = useState(""); const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState<string | null>(null);
321-333: Remove onUserNameConfirmed and tidy depsParent already sets confirmation in onJoinAsGuest.
- onJoinAsGuest(trimmedName); - onUserNameConfirmed?.(); + onJoinAsGuest(trimmedName); setIsOpen(false); @@ - }, [userName, onJoinAsGuest, onUserNameConfirmed, t]); + }, [userName, onJoinAsGuest, t]);
208-217: Gate overlay to guests only and drop auth-related propsDon’t render the overlay for logged-in users or when overrideName exists. This also lets you drop isLoggedIn/loggedInUserName/overrideName/onUserNameConfirmed from props. (Matches earlier review thread.)
- {displayLogInOverlay && !isUserNameConfirmed && ( - <LogInOverlay - isLoggedIn={!!loggedInUserName} - bookingUid={booking.uid} - loggedInUserName={loggedInUserName ?? undefined} - overrideName={overrideName} - onJoinAsGuest={handleJoinAsGuest} - onUserNameConfirmed={handleUserNameConfirmed} - /> - )} + {displayLogInOverlay && !isUserNameConfirmed && !loggedInUserName && !overrideName && ( + <LogInOverlay bookingUid={booking.uid} onJoinAsGuest={handleJoinAsGuest} /> + )}
293-301: Slim LogInOverlayProps to guest-only surfaceThese props aren’t needed when the parent gates to guests.
-interface LogInOverlayProps { - isLoggedIn: boolean; - bookingUid: string; - loggedInUserName?: string; - overrideName?: string; - onJoinAsGuest: (guestName: string) => void; - onUserNameConfirmed?: () => void; -} +interface LogInOverlayProps { + bookingUid: string; + onJoinAsGuest: (guestName: string) => void; +}
50-50: Auto-confirm username when logged-in or overrideName is presentPrevents rendering a closed overlay for authenticated users and aligns flow with requirements.
- const [isUserNameConfirmed, setIsUserNameConfirmed] = useState<boolean>(!displayLogInOverlay); + const [isUserNameConfirmed, setIsUserNameConfirmed] = useState<boolean>( + !displayLogInOverlay || !!loggedInUserName || !!overrideName + );
407-414: URL-encode callbackUrl in login redirectPrevents broken redirects and injection via unescaped query params.
- onClick={() => - (window.location.href = `${WEBAPP_URL}/auth/login?callbackUrl=${WEBAPP_URL}/video/${bookingUid}`) - }> + onClick={() => { + const callbackUrl = encodeURIComponent(`${WEBAPP_URL}/video/${bookingUid}`); + window.location.href = `${WEBAPP_URL}/auth/login?callbackUrl=${callbackUrl}`; + }}>
🧹 Nitpick comments (6)
apps/web/modules/videos/views/videos-single-view.tsx (6)
108-111: Remove redundant setUserNameYou already pass userName in createFrame options.
- if (userName) { - callFrame.setUserName(userName); - } + // userName already provided in createFrame options
113-115: Avoid reusing a stale Daily call instance on failureReturning getCallInstance() can mask errors by binding to an old call object.
- } catch (err) { - return DailyIframe.getCallInstance(); - } + } catch (err) { + console.error("createFrame failed:", err); + return undefined; + }
127-133: Set isCallFrameReady on an event, not immediatelyPrevents showing premium UI before the call is actually joined.
- setIsCallFrameReady(true); - - callFrame?.join(); + const onJoined = () => setIsCallFrameReady(true); + callFrame?.on("joined-meeting", onJoined); + callFrame?.join(); @@ - if (callFrame) { + if (callFrame) { + try { + callFrame.off?.("joined-meeting", onJoined); + } catch {} try { callFrame.destroy(); } catch (error) { console.error("Error destroying call frame:", error); } }Also applies to: 137-144
167-169: Remove unused handler after parent gatingOnce overlay confirms via onJoinAsGuest, this callback becomes dead code.
- const handleUserNameConfirmed = useCallback(() => { - setIsUserNameConfirmed(true); - }, []); + // handleUserNameConfirmed no longer needed; remove
7-7: Memoize startingTime and fix dependency to propReduces effect thrash and follows cleaner dependency contracts.
-import { useState, useEffect, useRef, useCallback } from "react"; +import { useState, useEffect, useRef, useCallback, useMemo } from "react"; @@ - const startingTime = dayjs(startTime).second(0).millisecond(0); + const startingTime = useMemo(() => dayjs(startTime).second(0).millisecond(0), [startTime]); @@ - }, [startingTime]); + }, [startTime]);Also applies to: 233-236, 276-276
278-288: Guard divide-by-zero and clamp percentagePrevents NaN/Infinity and overflow when startDuration ≤ 0 or duration drifts.
- const prev = startDuration - duration; - const percentage = prev * (100 / startDuration); + const prev = startDuration - duration; + const safeStart = Math.max(1, startDuration); + const percentage = Math.min(100, Math.max(0, (prev * 100) / safeStart));
📜 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 (1)
apps/web/modules/videos/views/videos-single-view.tsx(7 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:
apps/web/modules/videos/views/videos-single-view.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:
apps/web/modules/videos/views/videos-single-view.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:
apps/web/modules/videos/views/videos-single-view.tsx
🧠 Learnings (3)
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
apps/web/modules/videos/views/videos-single-view.tsx
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
apps/web/modules/videos/views/videos-single-view.tsx
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
apps/web/modules/videos/views/videos-single-view.tsx
🧬 Code graph analysis (1)
apps/web/modules/videos/views/videos-single-view.tsx (2)
packages/lib/constants.ts (3)
RECORDING_DEFAULT_ICON(204-206)TRANSCRIPTION_STOPPED_ICON(200-202)WEBAPP_URL(12-18)apps/web/modules/videos/cal-video-premium-features.tsx (1)
CalVideoPremiumFeatures(163-247)
⏰ 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). (4)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
apps/web/modules/videos/views/videos-single-view.tsx (2)
157-165: LGTM: guest join handlerTrim/validate then update state and confirm. Looks good.
355-383: LGTM: overlay copy is localized and accessibleStrings are wrapped in t(); input handles Enter and disabled/loading states.
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
Thanks for your contribution and for your patience while this was pending. We really appreciate your efforts. Approving now!
|
@kart1ka Thanks for continuously giving feedback. |
|
@CarinaWolli @anikdhabal |
CarinaWolli
dismissed stale reviews from Devanshusharma2005 and themself
dismissed
E2E results are ready! |
7 participants
What does this PR do?
Visual Demo (For contributors especially)
Light mode
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
https://www.loom.com/share/b5a321de1a624bfd8a6a1c5db011bdb7?sid=0614c919-22a4-40ae-8eef-e6e7ad933aba
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
By confirming a booking and going to the cal video link with having
displayLogInOverlayas trueyes (DAILY_API_KEY)
dailyco api key and two users
hardcode
displayLogInOverlayto trueChecklist
Summary by cubic
Improved the log in overlay for Cal Video by updating the design, adding clearer guest and sign-in options, and refining text for better user experience.