fix: auto join tenant by schwannden · Pull Request #220 · schwannden/chabod

schwannden · 2026-01-03T07:09:03Z

No description provided.

Copilot

Pull request overview

This PR implements an automatic tenant membership feature for OAuth users and adds Google Calendar integration for events and service schedules. The main purpose is to allow users who sign up via OAuth to automatically join a tenant without requiring explicit invitation, while also enabling users to add events to their Google Calendar.

Key Changes

Database migration enabling public tenant joins with user limit checks
Auto-association logic for OAuth users during signup/join flows
Google Calendar link integration for events and service schedules with mobile-optimized behavior

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
supabase/migrations/20260103120000_allow_public_tenant_join.sql	Adds RLS policy allowing authenticated users to self-join tenants as members
src/pages/tenant/AuthPage.tsx	Implements auto-association logic for OAuth users with tenant membership
src/components/Auth/GoogleOAuthButton.tsx	Preserves flow parameter through OAuth redirect
src/lib/googleCalendar.ts	New utility module for Google Calendar URL generation and date parsing
src/components/shared/AddToGoogleCalendarLink.tsx	Reusable component for "Add to Calendar" links with mobile/desktop handling
src/components/ServiceEvents/ServiceEventRow.tsx	Integrates Google Calendar link and i18n for service events
src/components/Events/EventCard.tsx	Adds Google Calendar link and restructures layout for better organization
src/components/Events/EventActions.tsx	Makes positioning flexible via className prop
tests/ui/components/ServiceEvents/ServiceEventRow.test.tsx	Tests for Google Calendar link rendering on mobile and desktop
tests/ui/components/Events/EventCard.test.tsx	Tests for Google Calendar link rendering on mobile and desktop
tests/ui-setup.ts	Adds polyfills for fetch, matchMedia, and new icon mocks
public/locales/{en,zh-TW}/{services,events,auth}.json	Translation keys for new Google Calendar and auto-join features

Comments suppressed due to low confidence (1)

src/pages/tenant/AuthPage.tsx:140

There's a potential race condition between the auto-association effect (lines 67-109) and the checkUserMembership effect (lines 111-140). If checkUserMembership runs after auto-association starts but before it completes, it could redirect the user before the association is done, or show an error message when association is still in progress. Consider adding isAssociating to the dependencies of checkUserMembership or adding a check for isAssociating at the beginning of checkUserMembership to prevent it from running during association.

  useEffect(() => {
    const checkUserMembership = async () => {
      if (!isLoading && user && tenant && slug) {
        try {
          const hasAccess = await checkUserTenantAccess(user.id, slug);

          if (hasAccess) {
            // Redirect to original page if specified
            if (redirectTo && redirectTo.startsWith(`/tenant/${slug}`)) {
              navigate(redirectTo);
            } else {
              navigate(`/tenant/${slug}`);
            }
          } else if (!inviteToken) {
            // ADD: User-facing error message instead of console.log
            setError(t("auth:noPermissionToEnterChurch"));
            toast({
              title: t("auth:loginFailed"),
              description: t("auth:noPermissionToEnterChurch"),
              variant: "destructive",
            });
          }
        } catch (error) {
          console.error("Error checking tenant membership:", error);
        }
      }
    };

    checkUserMembership();
  }, [user, isLoading, navigate, tenant, slug, inviteToken, redirectTo, t, toast]);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-03T07:15:43Z

tests/ui/components/Events/EventCard.test.tsx

+    date: "2026-01-03",
+    description: "Test description",
+    start_time: "09:00",
+    end_time: "10:00",
+    event_link: null,
+    created_at: "2026-01-01T00:00:00Z",
+    updated_at: "2026-01-01T00:00:00Z",


The test dates are set to "2026-01-03" and "2026-01-01", which appear to be current dates in the context (January 3, 2026). For better test maintainability and to avoid future issues, consider using a more static date in the past or a clearly future date, or dynamically generating dates relative to a fixed point in time to ensure tests remain valid regardless of when they're run.

Suggested change

date: "2026-01-03",

description: "Test description",

start_time: "09:00",

end_time: "10:00",

event_link: null,

created_at: "2026-01-01T00:00:00Z",

updated_at: "2026-01-01T00:00:00Z",

date: "2000-01-03",

description: "Test description",

start_time: "09:00",

end_time: "10:00",

event_link: null,

created_at: "2000-01-01T00:00:00Z",

updated_at: "2000-01-01T00:00:00Z",

Copilot · 2026-01-03T07:15:44Z

src/lib/googleCalendar.ts

+  const [hStr, mStr, sStr] = trimmed.split(":");
+  const hours = Number(hStr);
+  const minutes = Number(mStr ?? "0");
+  const seconds = Number(sStr ?? "0");


The function parseTimeToParts doesn't handle edge cases properly. If the input time string is malformed (e.g., contains non-numeric characters or has fewer than one colon), it could return null or produce incorrect results. Additionally, the check uses Number.isNaN which only checks after parsing, but doesn't validate the format before splitting. Consider adding validation for the expected format (HH:mm or HH:mm:ss) before attempting to parse.

Suggested change

const [hStr, mStr, sStr] = trimmed.split(":");

const hours = Number(hStr);

const minutes = Number(mStr ?? "0");

const seconds = Number(sStr ?? "0");

if (!trimmed) return null;

const parts = trimmed.split(":");

// Expect HH:mm or HH:mm:ss

if (parts.length < 2 || parts.length > 3) return null;

if (!parts.every((p) => /^[0-9]+$/.test(p))) return null;

const [hStr, mStr, sStr] = parts;

const hours = Number(hStr);

const minutes = Number(mStr);

const seconds = sStr !== undefined ? Number(sStr) : 0;

Copilot · 2026-01-03T07:15:44Z

src/components/Events/EventCard.tsx

+  const calendarStart = hasAnyTime
+    ? ((event.start_time && combineDateAndTimeLocal(event.date, event.start_time)) ??
+      parseISODateLocal(event.date) ??
+      new Date())
+    : (parseISODateLocal(event.date) ?? new Date());
+  const calendarEnd = hasAnyTime
+    ? ((event.end_time && combineDateAndTimeLocal(event.date, event.end_time)) ??
+      new Date(calendarStart.getTime() + 60 * 60 * 1000))
+    : calendarStart;


The calendar start/end date calculations are complex and harder to maintain inline. The nested ternary operators and fallback logic make the code difficult to read and reason about. Consider extracting this logic into a separate helper function or useMemo hook for better readability and testability.

Copilot · 2026-01-03T07:15:44Z

src/components/shared/AddToGoogleCalendarLink.tsx

+import { useMemo } from "react";
+import { CalendarPlus } from "lucide-react";
+import { Button } from "@/components/ui/button";
+import { useIsMobile } from "@/hooks/use-mobile";
+import { buildGoogleCalendarTemplateUrl } from "@/lib/googleCalendar";
+
+type AddToGoogleCalendarLinkProps = {
+  title: string;
+  start: Date;
+  end: Date;
+  isAllDay?: boolean;
+  details?: string;
+  location?: string;
+  className?: string;
+  label: string;
+  showLabel?: boolean;
+};
+
+export function AddToGoogleCalendarLink({
+  title,
+  start,
+  end,
+  isAllDay,
+  details,
+  location,
+  className,
+  label,
+  showLabel,
+}: AddToGoogleCalendarLinkProps) {
+  const isMobile = useIsMobile();
+  const effectiveShowLabel = showLabel ?? !isMobile;
+
+  const href = useMemo(
+    () =>
+      buildGoogleCalendarTemplateUrl({
+        title,
+        start,
+        end,
+        isAllDay,
+        details,
+        location,
+      }),
+    [title, start, end, isAllDay, details, location],
+  );
+
+  // Desktop: open in new tab. Mobile: allow universal link to hand off to app.
+  const target = isMobile ? undefined : "_blank";
+  const rel = target ? "noopener noreferrer" : undefined;
+
+  return (
+    <Button asChild variant="link" className={className}>
+      <a href={href} target={target} rel={rel} aria-label={label}>
+        <CalendarPlus className="h-4 w-4" />
+        {effectiveShowLabel ? <span>{label}</span> : null}
+      </a>
+    </Button>
+  );
+}


The new AddToGoogleCalendarLink component lacks dedicated test coverage. While it's tested indirectly through ServiceEventRow and EventCard tests, having dedicated unit tests for the component would help ensure its behavior is properly validated, especially for edge cases like invalid dates, mobile vs desktop rendering, and different prop combinations.

Copilot · 2026-01-03T07:15:44Z

src/components/Auth/GoogleOAuthButton.tsx

-      // Append redirect parameter if it exists
+      // Preserve both flow and redirect parameters
+      const queryParams = new URLSearchParams();
+      if (flowParam) queryParams.set("flow", flowParam);


The flow parameter is now being preserved in the OAuth redirect, but there's no validation that the flow parameter is a valid AuthFlowStep value. If a malformed or invalid flow parameter is passed, it could cause unexpected behavior in the auth flow. Consider adding validation to ensure the flow parameter is one of the expected values before preserving it.

Suggested change

if (flowParam) queryParams.set("flow", flowParam);

if (flowParam && /^[a-zA-Z0-9_-]+$/.test(flowParam)) {

queryParams.set("flow", flowParam);

}

Copilot · 2026-01-03T07:15:46Z

src/lib/googleCalendar.ts

+export function parseISODateLocal(dateStr: string) {
+  const [yStr, mStr, dStr] = dateStr.split("-");
+  const year = Number(yStr);
+  const monthIndex = Number(mStr) - 1;
+  const day = Number(dStr);
+  if ([year, monthIndex, day].some((n) => Number.isNaN(n))) return null;
+  return new Date(year, monthIndex, day, 0, 0, 0, 0);
+}


Similar to combineDateAndTimeLocal, parseISODateLocal doesn't validate the input format before parsing. Consider adding validation for the expected ISO date format (yyyy-MM-dd) to handle malformed inputs gracefully.

Copilot · 2026-01-03T07:15:46Z

src/components/ServiceEvents/ServiceEventRow.tsx

+            {(() => {
+              const start =
+                combineDateAndTimeLocal(event.date, event.start_time) ??
+                parseISODateLocal(event.date) ??
+                new Date();
+              const end =
+                combineDateAndTimeLocal(event.date, event.end_time) ??
+                new Date(start.getTime() + 60 * 60 * 1000);
+              const title = `${event.service.name}${event.subtitle ? ` - ${event.subtitle}` : ""}`;
+
+              return (
+                <AddToGoogleCalendarLink
+                  title={title}
+                  start={start}
+                  end={end}
+                  className="shrink-0 px-0"
+                  label={t("addToGoogleCalendar")}
+                />
+              );
+            })()}


The inline IIFE (Immediately Invoked Function Expression) for creating the AddToGoogleCalendarLink is inefficient and harder to maintain. The calendar date calculations execute on every render even though the event data doesn't change. Consider extracting this logic into a useMemo hook or a helper function to improve readability and performance.

Copilot · 2026-01-03T07:15:46Z

src/pages/tenant/AuthPage.tsx

+  // Auto-associate OAuth users with tenant when they complete signup/join flow
+  useEffect(() => {
+    const autoAssociateOAuthUser = async () => {
+      // Guard conditions
+      if (!user || !tenant || !flowStep || isLoading || isTenantLoading) return;
+      if (flowStep !== "signup" && flowStep !== "join-signin") return;
+      if (inviteToken) return; // Invite flow handles its own association
+      if (hasAttemptedAssociation) return; // Prevent double-association
+
+      // Check if user already has access
+      const hasAccess = await checkUserTenantAccess(user.id, slug!);
+      if (hasAccess) return; // Already a member
+
+      // Associate user with tenant
+      setIsAssociating(true);
+      try {
+        await associateUserWithTenant(user.id, tenant.id);
+        setHasAttemptedAssociation(true);
+
+        toast({
+          title: t("auth:joinedChurch"),
+          description: t("auth:joinedChurchDesc", { tenantName: tenant.name }),
+        });
+      } catch (error) {
+        console.error("Auto-association error:", error);
+        setError(error instanceof Error ? error.message : t("auth:cannotJoinChurch"));
+      } finally {
+        setIsAssociating(false);
+      }
+    };
+
+    autoAssociateOAuthUser();
+  }, [
+    user,
+    tenant,
+    flowStep,
+    isLoading,
+    isTenantLoading,
+    slug,
+    inviteToken,
+    hasAttemptedAssociation,
+    t,
+    toast,
+  ]);


The new auto-association logic in AuthPage (lines 66-109) lacks test coverage. Given that there's an existing test file for tenant AuthPage (tests/ui/pages/tenant/AuthPage.test.tsx), tests should be added to verify the auto-association behavior works correctly for OAuth users in signup and join-signin flows, handles errors properly, prevents double-association, and doesn't interfere with invite token flows.

Copilot · 2026-01-03T07:15:46Z

tests/ui/components/ServiceEvents/ServiceEventRow.test.tsx

+  beforeEach(() => {
+    // Desktop
+    window.innerWidth = 1024;
+  });


The window.innerWidth assignments in the tests don't trigger the matchMedia event listeners or the resize event. The useIsMobile hook relies on window.matchMedia and addEventListener to detect mobile state. Simply setting window.innerWidth won't cause the hook to update. Consider using window.matchMedia mock to properly simulate mobile/desktop states, or trigger the resize event after setting innerWidth.

Copilot · 2026-01-03T07:15:47Z

supabase/migrations/20260103120000_allow_public_tenant_join.sql

+CREATE POLICY "Users can join tenant as member"
+ON "public"."tenant_members"
+FOR INSERT
+WITH CHECK (
+  -- User can only add themselves
+  user_id = auth.uid()
+  -- Must be member role (not owner)
+  AND role = 'member'
+  -- Tenant must not exceed user limit
+  AND check_tenant_user_limit(tenant_id)


The new CREATE POLICY "Users can join tenant as member" on public.tenant_members allows any authenticated user to insert a membership row for any tenant_id as long as user_id = auth.uid() and role = 'member'. Because other RLS policies (e.g., on services, events, groups, service_notes) use public.is_tenant_member(tenant_id) / direct tenant_members lookups to gate access, an attacker can self-add to arbitrary tenants and gain read/write access to cross-tenant data simply by choosing a target tenant_id obtained from the publicly-readable tenants table. You should further constrain this insert policy (for example by requiring a validated invitation or tenant-specific join token) so that users can only join tenants they are explicitly authorized to access, rather than any tenant in the system.

fix: auto join tenant

8e39d1f

Copilot AI review requested due to automatic review settings January 3, 2026 07:09

Copilot started reviewing on behalf of schwannden January 3, 2026 07:09 View session

schwannden merged commit 8bec39f into main Jan 3, 2026
10 checks passed

schwannden mentioned this pull request Jan 3, 2026

release: release 1.2.2 #221

Merged

Copilot AI reviewed Jan 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: auto join tenant#220

fix: auto join tenant#220
schwannden merged 1 commit intomainfrom
fix/auto-join-tenant

schwannden commented Jan 3, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-  const [hStr, mStr, sStr] = trimmed.split(":");
-  const hours = Number(hStr);
-  const minutes = Number(mStr ?? "0");
-  const seconds = Number(sStr ?? "0");
+  if (!trimmed) return null;
+  const parts = trimmed.split(":");
+  // Expect HH:mm or HH:mm:ss
+  if (parts.length < 2 || parts.length > 3) return null;
+  if (!parts.every((p) => /^[0-9]+$/.test(p))) return null;
+  const [hStr, mStr, sStr] = parts;
+  const hours = Number(hStr);
+  const minutes = Number(mStr);
+  const seconds = sStr !== undefined ? Number(sStr) : 0;

Uh oh!

Conversation

schwannden commented Jan 3, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant