diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index acd61170..f10fa1d2 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -2,6 +2,7 @@ ## Mandatory File Consultations +**Nullable Types** → **Never write `T | null` or `T | undefined` by default.** Use `Maybe` or `Result` from `true-myth`. See `.claude/coding-standards.md` § "Strict Typing and Nullability" for the full hierarchy and when bare `null` is acceptable. **Code Implementation/Editing** → Read `.claude/coding-standards.md` FIRST **Pull Request Operations** → Read `.claude/pr-instructions.md` FIRST diff --git a/.claude/coding-standards.md b/.claude/coding-standards.md index d6bcd6d0..3c6a755e 100644 --- a/.claude/coding-standards.md +++ b/.claude/coding-standards.md @@ -4,23 +4,87 @@ This document outlines coding conventions and standards for this project. ## Strict Typing and Nullability -Prefer strict, explicit typings and clear nullability rules; don't auto-widen. +**Never default to `T | null` or `T | undefined`.** LLM training data makes nullable unions the path of least resistance — actively resist this. When tempted to write `Foo | null`, stop and reach for `Maybe` or `Result` instead. -- In TypeScript, lean on strict null checks and intentional nullability. Enable `strict: true` and `noImplicitAny`. Use exact types rather than permissive unions, and reserve `null`/`undefined` for truly absent states. +This project uses [true-myth](https://true-myth.js.org/) for safe nullable and error handling types. -- Prefer discriminated unions and "presence" wrappers over sprinkling null: for example, `{ kind: "loaded", value: T } | { kind: "loading" } | { kind: "error", message: string }` instead of `T | null`. +### The Nullable Type Hierarchy -- Use Optional types at boundaries only. Accept `string | undefined` from inputs, but normalize immediately inside functions to a definitive shape so internals don't propagate nullability. +Reach for these in order. Pick the first one that fits: -- Write function contracts that eliminate nullability with guards. Parse and validate early, then operate on a non-null `T`. +1. **`Maybe`** — when a value may or may not be present. Replaces `T | null` and `T | undefined`. -- Favor exact object shapes over partials. Use `type ExactUser = { id: string; name: string }` instead of `Partial`, and avoid `Record` unless unavoidable. +```typescript +import Maybe from "true-myth/maybe"; + +// ❌ WRONG +function findUser(id: string): User | null { ... } + +// ✅ CORRECT +function findUser(id: string): Maybe { ... } + +// Wrapping a nullable value from an external API: +const maybeUser = Maybe.of(nullableApiResponse); // Just(user) or Nothing + +// Safely transforming: +const name = maybeUser.map((u) => u.name).unwrapOr("Unknown"); +``` + +2. **`Result`** — when an operation can succeed or fail. Replaces nullable returns and thrown exceptions at async/fallible boundaries. + +```typescript +import Result from "true-myth/result"; + +// ❌ WRONG +async function fetchSession(id: string): Promise { ... } + +// ✅ CORRECT +async function fetchSession(id: string): Promise> { + try { + const session = await api.get(id); + return Result.ok(session); + } catch (e) { + return Result.err(`Failed to fetch session: ${e}`); + } +} +``` + +3. **Custom discriminated unions** — only when you need more than two states and `Maybe`/`Result` don't fit (e.g. `loading | loaded | error` for React state). + +```typescript +type UserState = + | { kind: "loading" } + | { kind: "loaded"; user: User } + | { kind: "error"; message: string }; +``` + +### Supporting Rules + +- **Normalize at boundaries, keep internals strict.** Accept `string | undefined` from external inputs (URL params, form fields, API responses), but parse and narrow immediately. Use `Maybe.of()` to wrap nullable values at the edge. Internal functions should never accept or return bare nullable types. + +```typescript +// ✅ Boundary function wraps nullable input immediately +import Maybe from "true-myth/maybe"; + +function parseSessionDate(raw: string | undefined): Maybe { + return Maybe.of(raw) + .map((r) => DateTime.fromISO(r)) + .andThen((dt) => (dt.isValid ? Maybe.just(dt) : Maybe.nothing())); +} +``` + +- **Exact object shapes over partials.** Use `type ExactUser = { id: string; name: string }` instead of `Partial`. Avoid `Record` unless unavoidable. + +- **No `T | null | undefined` double-nullable.** If a value can be absent, use `Maybe` — never combine both `null` and `undefined`. -- At async boundaries, return Result types rather than nullable payloads. +### When Bare `null` Is Acceptable -- Do not use `T | null | undefined` unless a value is truly optional. Prefer discriminated unions or Result types. Assume strict null checks. Provide exact types; no lazy unions. +Reserve bare `null` for cases where it is forced by external APIs or React conventions: +- A React ref before mount (`useRef(null)`) +- A third-party library that requires `null` in its API contract +- Zustand/Redux state where `null` is the established convention for "not yet loaded" and migrating to `Maybe` would touch too many files at once -If you inherit nullable APIs, normalize at the edge and keep your core strict. Model absence as a deliberate, named state rather than a catch-all union. +For domain types, always prefer `Maybe` over `T | null`. If you inherit nullable APIs from external libraries or backend responses, wrap with `Maybe.of()` at the edge and keep your core strict. ## Exhaustive Switch Statements @@ -261,3 +325,4 @@ When reviewing or writing code, ensure: - [ ] TypeScript types are properly defined and used - [ ] Leaf components receive `locale` and config values via props, not `siteConfig` imports - [ ] No `|| ""` fallbacks for nullable IDs or dates -- use render guards instead +- [ ] New types use `Maybe` or `Result` from `true-myth` instead of `T | null` diff --git a/__tests__/components/ui/actions/build-initial-order.test.ts b/__tests__/components/ui/actions/build-initial-order.test.ts new file mode 100644 index 00000000..b9609bb2 --- /dev/null +++ b/__tests__/components/ui/actions/build-initial-order.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect } from "vitest"; +import { buildInitialOrder } from "@/components/ui/actions/utils"; + +describe("buildInitialOrder", () => { + it("returns null when IDs have not changed", () => { + const previous = new Map([ + ["a", 0], + ["b", 1], + ]); + expect(buildInitialOrder(previous, ["a", "b"])).toBeNull(); + }); + + it("returns null for empty previous and empty current", () => { + expect(buildInitialOrder(new Map(), [])).toBeNull(); + }); + + it("returns a new map when IDs are added", () => { + const previous = new Map([["a", 0]]); + const result = buildInitialOrder(previous, ["a", "b"]); + expect(result).not.toBeNull(); + expect(result!.get("a")).toBe(0); + expect(result!.get("b")).toBe(1); + }); + + it("returns a new map when IDs are removed", () => { + const previous = new Map([ + ["a", 0], + ["b", 1], + ]); + const result = buildInitialOrder(previous, ["a"]); + expect(result).not.toBeNull(); + expect(result!.get("a")).toBe(0); + expect(result!.has("b")).toBe(false); + }); + + it("preserves order for existing IDs when new ones are appended", () => { + const previous = new Map([ + ["x", 0], + ["y", 1], + ]); + const result = buildInitialOrder(previous, ["x", "y", "z"]); + expect(result).not.toBeNull(); + expect(result!.get("x")).toBe(0); + expect(result!.get("y")).toBe(1); + expect(result!.get("z")).toBe(2); + }); + + it("builds a fresh map from empty previous with new IDs", () => { + const result = buildInitialOrder(new Map(), ["a", "b", "c"]); + expect(result).not.toBeNull(); + expect(result!.get("a")).toBe(0); + expect(result!.get("b")).toBe(1); + expect(result!.get("c")).toBe(2); + }); +}); diff --git a/__tests__/lib/api/coaching-sessions.test.ts b/__tests__/lib/api/coaching-sessions.test.ts index 64c34632..9ff6eb36 100644 --- a/__tests__/lib/api/coaching-sessions.test.ts +++ b/__tests__/lib/api/coaching-sessions.test.ts @@ -23,6 +23,20 @@ vi.mock('@/site.config', () => ({ }, })) +describe('CoachingSessionApi.list — null relationship ID', () => { + it('returns empty array immediately when relationshipId is null', async () => { + const result = await CoachingSessionApi.list( + null, + DateTime.fromISO('2025-07-01'), + DateTime.fromISO('2025-07-31') + ) + + expect(result).toEqual([]) + // Must NOT call the API + expect(EntityApi.listFn).not.toHaveBeenCalled() + }) +}) + describe('CoachingSessionApi - Sorting Functionality', () => { const mockRelationshipId = 'rel-123' const mockFromDate = DateTime.fromISO('2025-07-01') diff --git a/__tests__/lib/api/swr-hooks-null-path.test.ts b/__tests__/lib/api/swr-hooks-null-path.test.ts new file mode 100644 index 00000000..6c9dbf82 --- /dev/null +++ b/__tests__/lib/api/swr-hooks-null-path.test.ts @@ -0,0 +1,247 @@ +/** + * Tests verifying that SWR wrapper hooks do NOT fetch when given falsy IDs. + * + * When an ID is absent (null, undefined, or ""), the hook must pass a null + * URL/key to SWR so that no network request is made. Each hook is tested + * with its actual implementation — only EntityApi is mocked. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { TestProviders } from "@/test-utils/providers"; +import { DateTime } from "ts-luxon"; + +// Mock EntityApi — the single dependency all SWR hooks share +vi.mock("@/lib/api/entity-api", () => ({ + EntityApi: { + useEntity: vi.fn().mockReturnValue({ + entity: undefined, + isLoading: false, + isError: undefined, + refresh: vi.fn(), + }), + useEntityList: vi.fn().mockReturnValue({ + entities: [], + isLoading: false, + isError: undefined, + refresh: vi.fn(), + }), + listFn: vi.fn(), + getFn: vi.fn(), + createFn: vi.fn(), + updateFn: vi.fn(), + deleteFn: vi.fn(), + listNestedFn: vi.fn(), + }, +})); + +vi.mock("@/site.config", () => ({ + siteConfig: { + env: { + backendServiceURL: "http://localhost:4000", + }, + }, +})); + +import { EntityApi } from "@/lib/api/entity-api"; +import { useCoachingSession, useCoachingSessionList } from "@/lib/api/coaching-sessions"; +import { useOrganization } from "@/lib/api/organizations"; +import { + useCoachingRelationship, + useCoachingRelationshipList, +} from "@/lib/api/coaching-relationships"; +import { useOverarchingGoalBySession } from "@/lib/api/overarching-goals"; + +describe("SWR hooks — null/falsy ID path", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("useCoachingSession", () => { + it("passes null URL to useEntity when id is empty string", () => { + renderHook(() => useCoachingSession(""), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + null, // URL must be null to skip fetch + expect.any(Function), + expect.anything() // defaultValue + ); + }); + + it("passes a real URL to useEntity when id is present", () => { + renderHook(() => useCoachingSession("session-123"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + expect.stringContaining("/coaching_sessions/session-123"), + expect.any(Function), + expect.anything() + ); + }); + }); + + describe("useOrganization", () => { + it("passes null URL to useEntity when id is empty string", () => { + renderHook(() => useOrganization(""), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + null, + expect.any(Function), + expect.anything() + ); + }); + + it("passes a real URL to useEntity when id is present", () => { + renderHook(() => useOrganization("org-456"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + expect.stringContaining("/organizations/org-456"), + expect.any(Function), + expect.anything() + ); + }); + }); + + describe("useCoachingRelationship", () => { + it("passes null URL to useEntity when both IDs are empty", () => { + renderHook(() => useCoachingRelationship("", ""), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + null, + expect.any(Function), + expect.anything() + ); + }); + + it("passes null URL to useEntity when organizationId is empty", () => { + renderHook(() => useCoachingRelationship("", "rel-123"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + null, + expect.any(Function), + expect.anything() + ); + }); + + it("passes null URL to useEntity when relationshipId is empty", () => { + renderHook(() => useCoachingRelationship("org-456", ""), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + null, + expect.any(Function), + expect.anything() + ); + }); + + it("passes a real URL to useEntity when both IDs are present", () => { + renderHook(() => useCoachingRelationship("org-456", "rel-123"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntity).toHaveBeenCalledWith( + expect.stringContaining("/organizations/org-456/coaching_relationships/rel-123"), + expect.any(Function), + expect.anything() + ); + }); + }); + + describe("useCoachingRelationshipList", () => { + it("passes null conditional key when organizationId is null", () => { + renderHook(() => useCoachingRelationshipList(null), { + wrapper: TestProviders, + }); + + // Third arg (params/conditional) is null → useEntityList sets SWR key = null + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.any(String), // URL template (always constructed even with null) + expect.any(Function), + null + ); + }); + + it("passes organizationId as conditional key when present", () => { + renderHook(() => useCoachingRelationshipList("org-456"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.stringContaining("/organizations/org-456/coaching_relationships"), + expect.any(Function), + "org-456" + ); + }); + }); + + describe("useCoachingSessionList", () => { + const from = DateTime.fromISO("2025-01-01"); + const to = DateTime.fromISO("2025-12-31"); + + it("passes undefined conditional key when relationshipId is null", () => { + renderHook(() => useCoachingSessionList(null, from, to), { + wrapper: TestProviders, + }); + + // When relationshipId is null, params = undefined → SWR key = null + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.any(String), + expect.any(Function), + undefined + ); + }); + + it("passes params object when relationshipId is present", () => { + renderHook(() => useCoachingSessionList("rel-123", from, to), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.any(String), + expect.any(Function), + expect.objectContaining({ + coaching_relationship_id: "rel-123", + }) + ); + }); + }); + + describe("useOverarchingGoalBySession", () => { + it("passes empty string as conditional key (falsy → null SWR key)", () => { + renderHook(() => useOverarchingGoalBySession(""), { + wrapper: TestProviders, + }); + + // useOverarchingGoalBySession → useOverarchingGoalList → useEntityList + // Third arg is coachingSessionId="" (falsy) → useEntityList sets key = null + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.any(String), + expect.any(Function), + "" + ); + }); + + it("passes session ID as conditional key when present", () => { + renderHook(() => useOverarchingGoalBySession("session-789"), { + wrapper: TestProviders, + }); + + expect(EntityApi.useEntityList).toHaveBeenCalledWith( + expect.any(String), + expect.any(Function), + "session-789" + ); + }); + }); +}); diff --git a/__tests__/lib/utils/redirect.test.ts b/__tests__/lib/utils/redirect.test.ts new file mode 100644 index 00000000..e16caa5e --- /dev/null +++ b/__tests__/lib/utils/redirect.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { + validateRedirectUrl, + sanitizeCallbackUrl, + createLoginUrlWithCallback, +} from "@/lib/utils/redirect"; + +describe("validateRedirectUrl", () => { + it("returns true for valid internal path", () => { + expect(validateRedirectUrl("/dashboard")).toBe(true); + }); + + it("returns false for empty string", () => { + expect(validateRedirectUrl("")).toBe(false); + }); + + it("returns false for root path", () => { + expect(validateRedirectUrl("/")).toBe(false); + }); + + it("returns false for external URL", () => { + expect(validateRedirectUrl("https://evil.com/steal")).toBe(false); + }); +}); + +describe("sanitizeCallbackUrl", () => { + it("returns null when url is null", () => { + expect(sanitizeCallbackUrl(null)).toBeNull(); + }); + + it("returns null when url is undefined", () => { + expect(sanitizeCallbackUrl(undefined)).toBeNull(); + }); + + it("returns null when url is empty string", () => { + expect(sanitizeCallbackUrl("")).toBeNull(); + }); + + it("returns pathname for valid internal URL", () => { + expect(sanitizeCallbackUrl("/coaching-sessions/abc")).toBe( + "/coaching-sessions/abc" + ); + }); + + it("returns null for external URL", () => { + expect(sanitizeCallbackUrl("https://evil.com/steal")).toBeNull(); + }); + + it("preserves query string and hash", () => { + expect(sanitizeCallbackUrl("/sessions?tab=notes#section")).toBe( + "/sessions?tab=notes#section" + ); + }); +}); + +describe("createLoginUrlWithCallback", () => { + it("creates encoded callback URL", () => { + const result = createLoginUrlWithCallback("/coaching-sessions/123"); + expect(result).toBe("/?callbackUrl=%2Fcoaching-sessions%2F123"); + }); +}); diff --git a/package-lock.json b/package-lock.json index 87f466bb..f16a3cec 100644 --- a/package-lock.json +++ b/package-lock.json @@ -76,6 +76,7 @@ "swr": "^2.3.4", "tailwind-merge": "^2.5.5", "tailwindcss-animate": "^1.0.7", + "true-myth": "^9.3.1", "ts-luxon": "^5.0.7-beta.0", "y-prosemirror": "^1.2.15", "y-websocket": "^2.1.0", @@ -14358,6 +14359,15 @@ "url": "https://github.com/sponsors/wooorm" } }, + "node_modules/true-myth": { + "version": "9.3.1", + "resolved": "https://registry.npmjs.org/true-myth/-/true-myth-9.3.1.tgz", + "integrity": "sha512-a+u2L3y6XF3HLBJrSox8pRmod2o/3k28hpXX8OD56mLnffmXOwzQFBDBT0gy8ROIReLvdVvMPtQYtCsgFldfMw==", + "license": "MIT", + "engines": { + "node": "18.* || >= 20.*" + } + }, "node_modules/ts-api-utils": { "version": "2.4.0", "resolved": "https://registry.npmjs.org/ts-api-utils/-/ts-api-utils-2.4.0.tgz", diff --git a/package.json b/package.json index 2679ceba..d12d8170 100644 --- a/package.json +++ b/package.json @@ -86,6 +86,7 @@ "swr": "^2.3.4", "tailwind-merge": "^2.5.5", "tailwindcss-animate": "^1.0.7", + "true-myth": "^9.3.1", "ts-luxon": "^5.0.7-beta.0", "y-prosemirror": "^1.2.15", "y-websocket": "^2.1.0", diff --git a/src/app/coaching-sessions/[id]/page.tsx b/src/app/coaching-sessions/[id]/page.tsx index f3c5de09..3bb68d61 100644 --- a/src/app/coaching-sessions/[id]/page.tsx +++ b/src/app/coaching-sessions/[id]/page.tsx @@ -114,35 +114,37 @@ export default function CoachingSessionsPage() { return ( // Never grow wider than the site-header
- -
-
- -
- +
+
+ +
+ +
-
-
- -
+
+ +
- + - - + + + )}
); } diff --git a/src/components/ui/coaching-sessions/coaching-tabs-container.tsx b/src/components/ui/coaching-sessions/coaching-tabs-container.tsx index dde42f1e..7f8469eb 100644 --- a/src/components/ui/coaching-sessions/coaching-tabs-container.tsx +++ b/src/components/ui/coaching-sessions/coaching-tabs-container.tsx @@ -77,11 +77,12 @@ const CoachingTabsContainer = ({ isLoading: isActionMutating, } = useActionMutation(); - // Agreement CRUD handlers + // Agreement CRUD handlers — only callable when AgreementsList is rendered, + // which is guarded by currentCoachingSessionId being truthy. const handleAgreementAdded = (body: string): Promise => { const newAgreement: Agreement = { ...defaultAgreement(), - coaching_session_id: currentCoachingSessionId || "", + coaching_session_id: currentCoachingSessionId!, user_id: userId, body, }; @@ -92,7 +93,7 @@ const CoachingTabsContainer = ({ const updatedAgreement: Agreement = { ...defaultAgreement(), id, - coaching_session_id: currentCoachingSessionId || "", + coaching_session_id: currentCoachingSessionId!, user_id: userId, body, }; @@ -199,15 +200,17 @@ const CoachingTabsContainer = ({
- + {currentCoachingSessionId && ( + + )}
diff --git a/src/components/ui/dashboard/coaching-session-list.tsx b/src/components/ui/dashboard/coaching-session-list.tsx index 401351a2..0841e4ce 100644 --- a/src/components/ui/dashboard/coaching-session-list.tsx +++ b/src/components/ui/dashboard/coaching-session-list.tsx @@ -32,7 +32,7 @@ export default function CoachingSessionList({ }: CoachingSessionListProps) { const { currentOrganizationId } = useCurrentOrganization(); const { currentCoachingRelationshipId } = useCurrentCoachingRelationship(); - const { relationships } = useCoachingRelationshipList(currentOrganizationId || ""); + const { relationships } = useCoachingRelationshipList(currentOrganizationId); // TODO: for now we hardcode a 2 month window centered around now, // eventually we want to make this be configurable somewhere // (either on the page or elsewhere)