Conversation
… forgot password functionality - Added `AuthModal` component to manage user authentication flows, including sign in, sign up, and password recovery. - Introduced `AccountIcon` for triggering the authentication modal. - Created forms for sign in, sign up, and forgot password, utilizing Zod for validation. - Implemented tab navigation between sign in and sign up views. - Integrated Google OAuth sign-in functionality. - Established context and hooks for managing modal state and feature flags. - Added comprehensive tests for modal behavior and form validation.
There was a problem hiding this comment.
Pull request overview
Adds an MVP email/password authentication UI shell to the web app by introducing an auth modal (sign in / sign up / forgot password) and a header entry point via an account icon, plus Zod validation and tests.
Changes:
- Render a new
AccountIconin Day/Week/Now headers and mountAuthModalviaCompassProvider. - Add
AuthModalstate management (context + hooks) and auth forms with Zod-based validation schemas. - Add unit/integration-style tests for modal behavior and schema validation.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/views/Day/components/Header/Header.tsx | Adds AccountIcon to Day/Now header. |
| packages/web/src/views/Calendar/components/Header/Header.tsx | Adds AccountIcon to Week header. |
| packages/web/src/components/CompassProvider/CompassProvider.tsx | Mounts AuthModalProvider + AuthModal around app content. |
| packages/web/src/components/AuthModal/index.ts | New barrel re-export for auth modal modules. |
| packages/web/src/components/AuthModal/hooks/useAuthModal.ts | Adds modal state + view switching hook/context. |
| packages/web/src/components/AuthModal/hooks/useAuthFeatureFlag.ts | Adds URL-param-based gating for auth UI. |
| packages/web/src/components/AuthModal/forms/SignUpForm.tsx | Adds validated sign-up form UI. |
| packages/web/src/components/AuthModal/forms/SignInForm.tsx | Adds validated sign-in form UI + forgot-password link. |
| packages/web/src/components/AuthModal/forms/ForgotPasswordForm.tsx | Adds validated forgot-password request form + generic success state. |
| packages/web/src/components/AuthModal/components/AuthTabs.tsx | Adds tab UI to switch sign-in/sign-up. |
| packages/web/src/components/AuthModal/components/AuthInput.tsx | Adds styled input with error messaging/ARIA. |
| packages/web/src/components/AuthModal/components/AuthButton.tsx | Adds styled auth button variants. |
| packages/web/src/components/AuthModal/AuthModalProvider.tsx | Provides auth modal context to the tree. |
| packages/web/src/components/AuthModal/AuthModal.tsx | Implements modal content + Google button + form routing. |
| packages/web/src/components/AuthModal/AuthModal.test.tsx | Adds modal/account icon behavior tests. |
| packages/web/src/components/AuthModal/AccountIcon.tsx | Adds phosphor-based icon trigger for opening modal. |
| packages/web/src/auth/schemas/auth.schemas.ts | Adds Zod schemas + inferred types for auth forms. |
| packages/web/src/auth/schemas/auth.schemas.test.ts | Adds schema unit tests. |
packages/web/src/components/AuthModal/forms/ForgotPasswordForm.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/components/AuthModal/hooks/useAuthFeatureFlag.ts
Outdated
Show resolved
Hide resolved
…st coverage - Renamed `User` icon to `UserIcon` for clarity in `AccountIcon`. - Enhanced `AuthModal` and its forms with consistent button labels and improved comments for future implementation. - Updated test cases to use semantic queries and ensure consistent button labels across tabs. - Refactored `AuthInput` error handling styles for better clarity. - Removed unused `index.ts` file from `AuthModal` directory to streamline imports. - Adjusted import paths in various components to reflect the new structure.
…tionality - Replaced `UserIcon` with `UserCircleDashedIcon` and `UserCircleIcon` for better visual representation based on authentication state. - Enhanced tooltip descriptions to reflect user login status dynamically. - Updated tests to verify correct rendering of icons and tooltip descriptions based on authentication state. - Adjusted test cases to ensure consistent labeling and improved clarity in user interactions.
…ility - Updated test cases in `AuthModal.test.tsx` to utilize semantic queries for button interactions, replacing `data-testid` with `role` and `name` attributes for better accessibility. - Refactored mock components for `GoogleButton` and `TooltipWrapper` to align with semantic best practices, improving test clarity and maintainability. - Adjusted `AuthModal.tsx` to include a `DotIcon` for improved visual separation in the privacy and terms section, enhancing user experience.
…d styling - Replaced the existing GoogleButtonBase with a custom button component that includes a monochrome Google "G" logo. - Added styling to align with Google design guidelines, including padding, border radius, and hover effects. - Improved accessibility by ensuring proper aria-label usage and button interaction handling. - Enhanced the button's visual appearance and user experience through refined layout and interaction feedback.
- Moved the Google Sign In button to a more visually distinct position within the AuthModal component for improved user experience. - Updated button styling to maintain consistency with the overall design. - Cleaned up the code by removing redundant elements and ensuring better readability.
…experience - Modified the AuthModal component to display personalized greetings based on the user's sign-up name, enhancing user engagement. - Updated test cases in AuthModal.test.tsx to reflect changes in greeting messages, ensuring accurate verification of modal behavior. - Adjusted styling in AuthTabs to improve visual consistency and accessibility. - Enhanced SignUpForm to include a callback for name changes, facilitating dynamic updates in the greeting.
…ved test coverage - Added a new test case to verify that the greeting updates correctly when the user types their name in the AuthModal. - Modified the greeting message format in AuthModal.tsx for better clarity and user engagement. - Updated styling in OverlayPanel for improved layout and responsiveness.
…ty in AuthModal tests - Updated the mock implementation of TooltipWrapper in AuthModal.test.tsx to support keyboard interactions, allowing users to trigger actions via the Enter and Space keys. - Improved accessibility by adding role and tabIndex attributes to the mock component, ensuring it behaves like a button when onClick is defined.
| const tipDescription = authenticated ? "You're logged in" : "Log in"; | ||
|
|
||
| return ( | ||
| <TooltipWrapper description={tipDescription} onClick={handleClick}> | ||
| {authenticated ? ( | ||
| <UserCircleIcon | ||
| size={24} | ||
| className="cursor-pointer text-white/70 transition-colors hover:text-white" | ||
| aria-label="Your account" | ||
| /> |
There was a problem hiding this comment.
The tipDescription is computed based on authenticated status, but the component already returns early (line 18-19) when authenticated is true, making lines 26 and 30-35 unreachable. This dead code should be removed to improve clarity and maintainability.
…nsistency - Replaced hardcoded z-index values with constants from ZIndex in StyledHeaderRow, StyledReminderContainer, AgendaEventPreview, BaseContextMenu, StorageInfoModal, and StyledMenu. - Renamed Shortcut component to ShortcutTip for clarity and improved code organization. - Adjusted related imports and component usage to reflect the changes in naming and z-index management.
…kdrop prop - Removed the backdrop prop from OverlayPanel, streamlining its interface and usage. - Updated AuthModal to reflect the removal of the backdrop prop, enhancing code clarity. - Cleaned up SyncEventsOverlay by removing the backdrop prop, ensuring consistency across components.
…label support - Updated AuthInput to make the label prop optional, allowing for placeholder-only styles. - Introduced ariaLabel prop for accessibility when the label is omitted. - Adjusted usage in SignInForm, SignUpForm, and ForgotPasswordForm to utilize the new ariaLabel prop for improved accessibility. - Enhanced ShortcutTip component to support hover interactions for displaying keyboard shortcuts.
…s and optional children prop - Introduced a new ShortcutBadge component for displaying keyboard shortcuts. - Updated ShortcutTip to support hover interactions, showing the shortcut when the component is hovered over. - Added an optional children prop to allow for more flexible usage of the ShortcutTip component.
… view switching - Removed the AuthTabs component to streamline the AuthModal interface, replacing tab navigation with a button for switching between Sign In and Sign Up views. - Updated the AuthModal to include a dynamic title action for switching views, improving user experience. - Adjusted tests in AuthModal.test.tsx to reflect changes in view switching and button interactions, ensuring accurate verification of modal behavior. - Enhanced the OverlayPanel component to support optional title actions, improving layout flexibility.
…experience - Updated ForgotPasswordForm, SignInForm, and SignUpForm to clear validation errors when the user types, enhancing real-time feedback. - Modified error display logic to only show validation errors on blur for fields with input, preventing unnecessary error messages for empty required fields. - Streamlined dependencies in useCallback hooks for better performance and clarity.
… in Auth components - Introduced the useZodForm hook to manage form state and validation using Zod, enhancing the handling of form inputs in ForgotPasswordForm, SignInForm, and SignUpForm. - Updated forms to utilize the new hook, simplifying validation logic and improving user experience by showing errors only on blur and clearing them during input. - Added comprehensive tests for useZodForm to ensure correct validation behavior and form state management.
- Changed button labels in SignInForm and SignUpForm from "Enter" and "Create account" to "Log in" and "Sign up" respectively for better clarity. - Updated Google sign-in button label in AuthModal from "Sign in with Google" to "Continue with Google" to align with user expectations. - Introduced AuthButton component to standardize button styles and improve code organization within AuthModal. - Adjusted tests in AuthModal.test.tsx, SignInForm.test.tsx, and SignUpForm.test.tsx to reflect the updated button labels, ensuring accurate test coverage.
- Updated button transition effects in AuthButton to use 'transition-all duration-150' for smoother animations. - Modified hover styles for the outline variant to improve visual feedback, changing from 'hover:bg-[#f8f8f8]' to 'hover:bg-[#f0f0f0] hover:border-[#151515]'.
… yarn.lock - Deleted react-google-button dependency from package.json to streamline project dependencies. - Updated yarn.lock to reflect the removal of react-google-button, ensuring consistency in the dependency tree.
- Renamed schema variables in auth.schemas.ts from camelCase to PascalCase for consistency and clarity. - Updated AuthModal and its related components to use the new schema names, including LogInSchema and SignUpSchema. - Refactored AuthModal to handle URL parameters for opening specific authentication views, enhancing user experience. - Added tests for URL parameter handling to ensure correct modal behavior based on query strings. - Introduced LogInForm component to replace SignInForm, aligning with the new naming conventions.
4782397 to
c09b555
Compare
packages/web/src/views/Day/components/Agenda/Events/AgendaEventPreview/AgendaEventPreview.tsx
Show resolved
Hide resolved
| onMouseEnter={(e) => { | ||
| if (!disabled) { | ||
| e.currentTarget.style.backgroundColor = "#f8f8f8"; | ||
| } |
There was a problem hiding this comment.
The hover behavior is implemented by imperatively mutating e.currentTarget.style. This is brittle (can get out of sync with React props/state and makes theming harder). Prefer expressing hover styles via CSS/Tailwind (:hover classes) or styled-components so the hover state is declarative.
| // Outline variant (white/black, matches Google button) | ||
| "h-10 w-full border border-[#1f1f1f] bg-white px-4 text-[#1f1f1f]": | ||
| variant === "outline", | ||
| "hover:border-[#151515] hover:bg-[#f0f0f0]": |
There was a problem hiding this comment.
The outline variant uses hard-coded hex colors (border-[#1f1f1f], bg-white, text-[#1f1f1f]). Repo guidance prefers semantic color tokens from the Tailwind theme (see AGENTS.md Styling section) so components remain consistent across themes. Consider switching these to semantic classes/variables (or define a semantic token for the "Google outline" style).
| // Outline variant (white/black, matches Google button) | |
| "h-10 w-full border border-[#1f1f1f] bg-white px-4 text-[#1f1f1f]": | |
| variant === "outline", | |
| "hover:border-[#151515] hover:bg-[#f0f0f0]": | |
| // Outline variant (semantic colors, matches Google-style outline) | |
| "h-10 w-full border border-border bg-bg-primary px-4 text-text": | |
| variant === "outline", | |
| "hover:border-border hover:bg-bg-secondary": |
| aria-labelledby="event-title" | ||
| aria-describedby={event?.description ? "event-description" : undefined} | ||
| className="z-50 max-w-80 min-w-64 rounded-lg p-4 shadow-lg" | ||
| className={`z-${ZIndex.LAYER_5} max-w-80 min-w-64 rounded-lg p-4 shadow-lg`} |
There was a problem hiding this comment.
This builds a Tailwind class name dynamically (z-${ZIndex.LAYER_5} → z-5). Tailwind won't generate classes from runtime template strings, and z-5 is not in the default z-index scale. Since z-index is already set via the inline style (maxZIndex + 1), drop the dynamic z-* class (or switch to an inline zIndex / literal z-[...] class).
| if (value.trim() !== "") { | ||
| const error = validateField(field, value); | ||
| setErrors((prev) => ({ ...prev, [field]: error })); | ||
| } else { | ||
| setErrors((prev) => ({ ...prev, [field]: undefined })); | ||
| } |
There was a problem hiding this comment.
handleBlur skips validation when value.trim() === "" and clears the error instead. For schemas that preprocess/trim (e.g., NameSchema / EmailSchema), whitespace-only input should still surface a "required" error on blur, but this logic suppresses it. Validate the field even when the trimmed value is empty (or delegate entirely to the schema) so required-field errors appear consistently on blur.
| if (value.trim() !== "") { | |
| const error = validateField(field, value); | |
| setErrors((prev) => ({ ...prev, [field]: error })); | |
| } else { | |
| setErrors((prev) => ({ ...prev, [field]: undefined })); | |
| } | |
| const error = validateField(field, value); | |
| setErrors((prev) => ({ ...prev, [field]: error })); |
| style={{ | ||
| display: "inline-flex", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| gap: "10px", // Google guideline: 10px between icon and text | ||
| height: "40px", | ||
| paddingLeft: "12px", // Google guideline: 12px left padding | ||
| paddingRight: "12px", // Google guideline: 12px right padding | ||
| backgroundColor: "#ffffff", | ||
| border: "1px solid #1f1f1f", | ||
| borderRadius: "9999px", // Pill shape | ||
| cursor: disabled ? "not-allowed" : "pointer", | ||
| opacity: disabled ? 0.6 : 1, | ||
| fontFamily: "'Roboto', sans-serif", | ||
| fontSize: "14px", | ||
| fontWeight: 500, | ||
| color: "#1f1f1f", | ||
| whiteSpace: "nowrap", | ||
| transition: "background-color 0.2s ease", | ||
| ...style, | ||
| }} | ||
| onMouseEnter={(e) => { | ||
| if (!disabled) { | ||
| e.currentTarget.style.backgroundColor = "#f8f8f8"; | ||
| } | ||
| }} | ||
| onMouseLeave={(e) => { | ||
| e.currentTarget.style.backgroundColor = "#ffffff"; | ||
| }} |
There was a problem hiding this comment.
This button hard-codes non-semantic colors (e.g. #ffffff, #1f1f1f) and a specific font family, which diverges from the repo's styling guidance to use semantic Tailwind theme tokens (see AGENTS.md Styling section). Prefer Tailwind classes / CSS variables derived from the app theme (or centralize these values) so the Google button stays consistent with theming and dark-mode expectations.
| style={{ | |
| display: "inline-flex", | |
| alignItems: "center", | |
| justifyContent: "center", | |
| gap: "10px", // Google guideline: 10px between icon and text | |
| height: "40px", | |
| paddingLeft: "12px", // Google guideline: 12px left padding | |
| paddingRight: "12px", // Google guideline: 12px right padding | |
| backgroundColor: "#ffffff", | |
| border: "1px solid #1f1f1f", | |
| borderRadius: "9999px", // Pill shape | |
| cursor: disabled ? "not-allowed" : "pointer", | |
| opacity: disabled ? 0.6 : 1, | |
| fontFamily: "'Roboto', sans-serif", | |
| fontSize: "14px", | |
| fontWeight: 500, | |
| color: "#1f1f1f", | |
| whiteSpace: "nowrap", | |
| transition: "background-color 0.2s ease", | |
| ...style, | |
| }} | |
| onMouseEnter={(e) => { | |
| if (!disabled) { | |
| e.currentTarget.style.backgroundColor = "#f8f8f8"; | |
| } | |
| }} | |
| onMouseLeave={(e) => { | |
| e.currentTarget.style.backgroundColor = "#ffffff"; | |
| }} | |
| className="inline-flex items-center justify-center gap-2 h-10 px-3 rounded-full border bg-background text-foreground border-border text-sm font-medium font-sans whitespace-nowrap transition-colors hover:bg-accent hover:text-accent-foreground disabled:cursor-not-allowed disabled:opacity-60" | |
| style={style} |
| className="fixed inset-0 flex items-center justify-center" | ||
| style={{ | ||
| zIndex: ZIndex.LAYER_5, | ||
| }} |
There was a problem hiding this comment.
The modal container z-index was reduced from Tailwind's z-50 to ZIndex.LAYER_5 (which is 5). This is likely too low and can cause the modal to render behind other UI that still uses higher z-index utilities (e.g., z-50 dropdowns or z-[1000] overlays). Use a higher z-index (or reuse the same overlay z-index strategy used by OverlayPanel, e.g. z-[1000]) so the modal reliably sits above the app UI.
| ), | ||
| style: { | ||
| ...props.style, | ||
| zIndex: ZIndex.LAYER_5, | ||
| }, |
There was a problem hiding this comment.
BaseContextMenu now forces zIndex: ZIndex.LAYER_5 (5) and removes the previous high z-index class. This overrides any z-index coming from Floating UI (props.style) and can cause context menus to appear behind other overlays/menus. Avoid hardcoding a low z-index here; either preserve props.style.zIndex or set a consistently high overlay z-index (or accept a z-index prop) so menus always layer correctly.
| search: urlObj.search, | ||
| hash: urlObj.hash, | ||
| }, | ||
| writable: true, |
There was a problem hiding this comment.
Object.defineProperty(window, "location", ...) is called repeatedly across tests, but this definition is missing configurable: true. After the first call, subsequent calls can throw TypeError: Cannot redefine property: location. Set configurable: true in this helper (matching other tests in the repo) so setWindowLocation is safe to call in beforeEach.
| writable: true, | |
| writable: true, | |
| configurable: true, |
| // Don't show if user is already authenticated or feature is disabled | ||
| if (authenticated || !isEnabled) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Because the component returns null when authenticated is true, any later branching/rendering based on authenticated becomes unreachable. Consider removing the dead authenticated-only logic below (or, if you intend to render something when authenticated, remove/adjust this early return).
Related to #1452
AuthModalcomponent to manage user authentication flows, including sign in, sign up, and password recovery.AccountIconfor triggering the authentication modal.PW-PLAN.md