Skip to content

Evaluate and improve unit test quality - #104

Merged
2witstudios merged 3 commits into
masterfrom
claude/improve-unit-tests-F3w6i
Dec 21, 2025
Merged

2witstudios merged 3 commits into
masterfrom
claude/improve-unit-tests-F3w6i

Conversation

@2witstudios

@2witstudios 2witstudios commented Dec 20, 2025 •

Copy link
Copy Markdown
Owner

This commit improves unit test quality by introducing a proper repository seam for authentication-related database operations, following the rubric:

Changes:

  • Add auth-repository.ts with findUserByEmail, findUserById, createRefreshToken
  • Update login route to use authRepository instead of direct ORM calls
  • Update me route to use authRepository instead of direct ORM calls
  • Refactor login.test.ts to mock at repository seam (not ORM chains)
  • Refactor me.test.ts to mock at repository seam (not ORM chains)

Benefits:

  • Tests now mock at the repository boundary, not ORM query builders
  • Removes brittle db.query.users.findFirst chain mocking
  • Tests are more refactor-resistant (internal DB changes won't break tests)
  • Follows the contract-first testing rubric principles

All 2069 web tests pass.

Summary by CodeRabbit

  • Refactor

    • Auth data access moved to a clear repository boundary; improves maintainability. No behavioral changes except explicit 404 when a requested user profile is missing.
  • Tests

    • Auth tests rewritten to use the repository boundary, with new fixtures, broader mock coverage, and expanded scenarios for login, session, rate-limiting, and profile endpoints.

✏️ Tip: You can customize this high-level summary in your review settings.

This commit improves unit test quality by introducing a proper repository
seam for authentication-related database operations, following the rubric:

Changes:
- Add auth-repository.ts with findUserByEmail, findUserById, createRefreshToken
- Update login route to use authRepository instead of direct ORM calls
- Update me route to use authRepository instead of direct ORM calls
- Refactor login.test.ts to mock at repository seam (not ORM chains)
- Refactor me.test.ts to mock at repository seam (not ORM chains)

Benefits:
- Tests now mock at the repository boundary, not ORM query builders
- Removes brittle db.query.users.findFirst chain mocking
- Tests are more refactor-resistant (internal DB changes won't break tests)
- Follows the contract-first testing rubric principles

All 2069 web tests pass.
@coderabbitai

coderabbitai Bot commented Dec 20, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Adds a typed auth repository and switches auth routes and tests to use this repository boundary instead of direct ORM/db access; tests now mock repository methods and routes call repository APIs for user and refresh-token operations.

Changes

Cohort / File(s) Summary
New auth repository
apps/web/src/lib/repositories/auth-repository.ts
New module exporting typed User, RefreshToken*, PlatformType, CreateRefreshTokenInput, AuthRepository and authRepository methods: findUserByEmail, findUserById, createRefreshToken, findRefreshTokenWithUser, deleteRefreshToken, deleteAllUserRefreshTokens, incrementUserTokenVersion, revokeAllUserDeviceTokens, updateUserTokenVersion.
Repository barrel export
apps/web/src/lib/repositories/index.ts
Re-exports the new auth repository (export * from './auth-repository').
Auth routes switched to repository
apps/web/src/app/api/auth/login/route.ts, apps/web/src/app/api/auth/me/route.ts
Replace direct ORM/db queries with authRepository calls (findUserByEmail, findUserById, createRefreshToken); login preserves rate-limiting, device-token linking, token creation via repository; me route returns 404 if user not found and adds dev debug logging.
Tests refactored to repository boundary
apps/web/src/app/api/auth/__tests__/login.test.ts, apps/web/src/app/api/auth/__tests__/me.test.ts
Tests now mock authRepository methods (and import User type), add createRequest() helper, adjust fixtures and expectations, and expand external mocks (bcryptjs, server utils, activity-tracker, onboarding) to align with repository-based interactions.
DB types re-exported
packages/db/src/index.ts
Type-only export expanded to include InferSelectModel and InferInsertModel from drizzle-orm (type exports only).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant LoginRoute as POST /api/auth/login
    participant AuthRepo as authRepository
    participant DB as Database
    participant DeviceSvc as deviceToken flow

    Client->>LoginRoute: POST { email, password, deviceId?, deviceToken?, deviceName? }
    LoginRoute->>AuthRepo: findUserByEmail(email)
    AuthRepo->>DB: query users where email
    DB-->>AuthRepo: User | null
    alt user found and password valid
        LoginRoute->>AuthRepo: createRefreshToken({ token, userId, device, userAgent, ip, expiresAt, deviceTokenId, platform })
        AuthRepo->>DB: insert refresh token row
        DB-->>AuthRepo: OK
        opt deviceToken provided
            LoginRoute->>DeviceSvc: link deviceToken -> deviceTokenRecordId
            DeviceSvc-->>LoginRoute: deviceTokenRecordId
        end
        LoginRoute-->>Client: 200 { accessToken, refreshToken, user }
    else invalid credentials
        LoginRoute-->>Client: 401
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay extra attention to:
    • apps/web/src/lib/repositories/auth-repository.ts — ORM query correctness, joins, null handling, and types.
    • apps/web/src/app/api/auth/login/route.ts — ensure refresh token creation fields (platform, deviceTokenId, ip, userAgent) mirror previous behavior.
    • Tests in apps/web/src/app/api/auth/__tests__/* — verify mocks match exported signatures and cover OAuth/emailVerified variants.

Possibly related PRs

Poem

🐰 I hopped from SQL to tidy code,
Methods lined where queries rode.
Tokens tucked in repository nests,
Tests now mock the public quests.
A joyful twitch — auth feels light and bold.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title "Evaluate and improve unit test quality" is vague and overly generic, failing to convey the specific, substantial changes made in this PR. Replace with a more specific title that highlights the main change: e.g., "Introduce authentication repository seam for database mocking" or "Refactor auth tests to use repository boundary mocking.".
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/improve-unit-tests-F3w6i

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 448a57d and 6f2e267.

📒 Files selected for processing (4)
  • apps/web/src/app/api/auth/__tests__/login.test.ts (9 hunks)
  • apps/web/src/app/api/auth/__tests__/me.test.ts (1 hunks)
  • apps/web/src/lib/repositories/auth-repository.ts (1 hunks)
  • packages/db/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/src/lib/repositories/auth-repository.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Never use any types in TypeScript code - always use proper TypeScript types

**/*.{ts,tsx}: No any types - always use proper TypeScript types
Use kebab-case for filenames (e.g., image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: import getUserAccessLevel and canUserEditPage from @pagespace/lib/permissions
Use Drizzle client from @pagespace/db for all database access
Always structure message content using the message parts structure: { parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode

Files:

  • packages/db/src/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Format code with Prettier and lint with ESLint using the configuration at apps/web/eslint.config.mjs

Files:

  • packages/db/src/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
apps/web/src/app/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST await context.params before destructuring because params are Promise objects
Get request body using const body = await request.json();
Get search params using const { searchParams } = new URL(request.url);
Return JSON responses using return Response.json(data) or return NextResponse.json(data)

Files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/**/*.{ts,tsx}: Use centralized permission functions from @pagespace/lib/permissions for access control, such as getUserAccessLevel() and canUserEditPage()
Always use Drizzle client from @pagespace/db for database access instead of direct database connections
Always use message parts structure with parts array containing objects with type and text fields when constructing messages for AI

Files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
apps/web/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching

Files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
🧠 Learnings (8)
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to packages/db/src/schema.ts : Maintain the Drizzle ORM database schema in `packages/db/src/schema.ts` as the single entry point for schema definitions

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to packages/db/{src/schema.ts,drizzle/**/*.ts} : Database schema must be defined in `packages/db/src/schema.ts` and migrations must be emitted to `packages/db/drizzle/`

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Database schema entry point is at `packages/db/src/schema.ts`; migrations emit to `packages/db/drizzle/`

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:15.319Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.319Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries

Applied to files:

  • packages/db/src/index.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to packages/db/drizzle/*.ts : Database migrations should be generated into `packages/db/drizzle/` directory using `pnpm db:generate` command

Applied to files:

  • packages/db/src/index.ts
🧬 Code graph analysis (1)
apps/web/src/app/api/auth/__tests__/me.test.ts (3)
apps/web/src/lib/repositories/auth-repository.ts (2)
  • User (21-21)
  • authRepository (38-140)
apps/web/src/lib/auth/index.ts (1)
  • isAuthError (204-206)
apps/web/src/app/api/auth/me/route.ts (1)
  • GET (4-28)
⏰ 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). (1)
  • GitHub Check: Unit Tests
🔇 Additional comments (11)
packages/db/src/index.ts (1)

13-13: LGTM! Type exports enhance type safety for repository layer.

Adding InferSelectModel and InferInsertModel provides type-safe inference from Drizzle schema definitions, which aligns perfectly with the repository pattern introduced in this PR.

apps/web/src/app/api/auth/__tests__/login.test.ts (5)

1-25: LGTM! Repository boundary mocking significantly improves test maintainability.

The shift from mocking ORM query chains to mocking at the repository seam is a substantial improvement. This approach:

  • Makes tests resilient to internal DB implementation changes
  • Provides a clearer contract between route handlers and data access
  • Reduces brittle test dependencies on Drizzle's query builder API

90-127: LGTM! Test fixtures and helper function improve test readability.

The createRequest helper function is a clean abstraction that reduces boilerplate across tests. The mockUser fixture is properly typed with the User type from the repository, ensuring type safety.


194-204: LGTM! Repository interaction tests verify boundary contracts correctly.

The tests properly verify:

  • Refresh token storage via authRepository.createRefreshToken with correct arguments
  • Rate limiting prevents unnecessary database queries (line 425)

These assertions confirm the route handler fulfills its obligations to the repository boundary.

Also applies to: 416-426


293-312: LGTM! Security test properly verifies timing-safe comparison.

This test ensures protection against timing attacks by verifying that bcrypt.compare is called even for non-existent users. The bcrypt hash format validation on line 311 is appropriately thorough.


140-503: LGTM! Comprehensive test coverage across all authentication scenarios.

The test suite thoroughly covers:

  • Authentication flows (valid/invalid credentials, OAuth-only users)
  • Input validation (missing/invalid email, missing/empty password)
  • Security (timing attacks, sensitive data leakage prevention)
  • Rate limiting (IP and email-based)
  • Session management (tokens, cookies, device tokens)
  • Edge cases (IP extraction fallbacks, error handling)

The refactoring to repository boundary mocking maintains comprehensive coverage while improving maintainability.

apps/web/src/app/api/auth/__tests__/me.test.ts (5)

1-31: Excellent test structure and documentation.

The file header clearly documents the contract testing approach, and the mock setup properly uses the repository boundary instead of brittle ORM chain mocking. The mock hoisting pattern (defining mocks before importing the actual modules) is correct for vitest.


33-56: Well-structured test fixtures.

The test fixtures are comprehensive and properly typed:

  • mockUser includes all necessary fields from the User type, including sensitive fields that tests verify are properly filtered
  • mockVerifiedDate provides deterministic date handling
  • createRequest helper promotes code reuse and consistency

Also applies to: 65-72


74-107: Comprehensive coverage of happy path and security.

The test cases properly verify:

  • User profile data is returned with correct serialization (Date → ISO string)
  • Sensitive fields (password, tokenVersion) are excluded from responses
  • The beforeEach setup follows best practices by clearing mocks and establishing consistent defaults

108-154: Excellent coverage of authentication and edge cases.

These tests properly cover:

  • Admin role handling with correct boundary mocking
  • Authentication failure (401) scenario
  • Repository call verification (contract testing)
  • Edge case where token is valid but user has been deleted (404)

The comment on line 145 helpfully explains the edge case being tested.


156-193: Thorough coverage of OAuth and email verification scenarios.

These tests effectively cover:

  • OAuth users (Google provider with googleId)
  • Email verification states (null for unverified, ISO string for verified)
  • Proper JSON serialization behavior for different data types

The test cases align well with the route implementation and cover realistic user scenarios.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc9ef93 and b6fdd5f.

📒 Files selected for processing (6)
  • apps/web/src/app/api/auth/__tests__/login.test.ts (9 hunks)
  • apps/web/src/app/api/auth/__tests__/me.test.ts (2 hunks)
  • apps/web/src/app/api/auth/login/route.ts (3 hunks)
  • apps/web/src/app/api/auth/me/route.ts (1 hunks)
  • apps/web/src/lib/repositories/auth-repository.ts (1 hunks)
  • apps/web/src/lib/repositories/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Never use any types in TypeScript code - always use proper TypeScript types

**/*.{ts,tsx}: No any types - always use proper TypeScript types
Use kebab-case for filenames (e.g., image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: import getUserAccessLevel and canUserEditPage from @pagespace/lib/permissions
Use Drizzle client from @pagespace/db for all database access
Always structure message content using the message parts structure: { parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode

Files:

  • apps/web/src/lib/repositories/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/**/*.{ts,tsx}: Use centralized permission functions from @pagespace/lib/permissions for access control, such as getUserAccessLevel() and canUserEditPage()
Always use Drizzle client from @pagespace/db for database access instead of direct database connections
Always use message parts structure with parts array containing objects with type and text fields when constructing messages for AI

Files:

  • apps/web/src/lib/repositories/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Format code with Prettier and lint with ESLint using the configuration at apps/web/eslint.config.mjs

Files:

  • apps/web/src/lib/repositories/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
apps/web/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching

Files:

  • apps/web/src/lib/repositories/index.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
apps/web/src/app/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST await context.params before destructuring because params are Promise objects
Get request body using const body = await request.json();
Get search params using const { searchParams } = new URL(request.url);
Return JSON responses using return Response.json(data) or return NextResponse.json(data)

Files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
  • apps/web/src/app/api/auth/__tests__/login.test.ts
  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
apps/web/src/app/**/route.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 dynamic routes, params are Promise objects and must be awaited before destructuring
Get request body using const body = await request.json();
Return JSON responses using Response.json(data) or NextResponse.json(data) in route handlers

Files:

  • apps/web/src/app/api/auth/me/route.ts
  • apps/web/src/app/api/auth/login/route.ts
🧠 Learnings (8)
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : Get request body using `const body = await request.json();`

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/api/**/*.{ts,tsx} : Use `const body = await request.json();` to extract request bodies, `const { searchParams } = new URL(request.url);` for query parameters, and `return Response.json(data)` or `return NextResponse.json(data)` for responses

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:15.319Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.319Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission logic: import `getUserAccessLevel` and `canUserEditPage` from `pagespace/lib/permissions`

Applied to files:

  • apps/web/src/app/api/auth/login/route.ts
🧬 Code graph analysis (5)
apps/web/src/app/api/auth/__tests__/me.test.ts (3)
apps/web/src/lib/repositories/auth-repository.ts (2)
  • User (11-22)
  • authRepository (43-145)
apps/web/src/lib/auth/index.ts (1)
  • isAuthError (204-206)
apps/web/src/app/api/auth/me/route.ts (1)
  • GET (4-28)
apps/web/src/lib/repositories/auth-repository.ts (1)
packages/db/src/index.ts (5)
  • db (20-20)
  • eq (8-8)
  • sql (8-8)
  • and (8-8)
  • isNull (9-9)
apps/web/src/app/api/auth/__tests__/login.test.ts (2)
apps/web/src/lib/repositories/auth-repository.ts (2)
  • User (11-22)
  • authRepository (43-145)
apps/web/src/app/api/auth/login/route.ts (1)
  • POST (30-192)
apps/web/src/app/api/auth/me/route.ts (2)
apps/web/src/lib/auth/index.ts (1)
  • isAuthError (204-206)
apps/web/src/lib/repositories/auth-repository.ts (1)
  • authRepository (43-145)
apps/web/src/app/api/auth/login/route.ts (1)
apps/web/src/lib/repositories/auth-repository.ts (1)
  • authRepository (43-145)
⏰ 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). (1)
  • GitHub Check: Unit Tests
🔇 Additional comments (15)
apps/web/src/lib/repositories/auth-repository.ts (3)

1-41: LGTM! Well-structured type definitions and imports.

The type definitions are comprehensive and properly typed. The imports follow the project's coding guidelines by using the Drizzle client from @pagespace/db.


85-95: Same type assertion pattern observed.

The findRefreshTokenWithUser method uses the same type assertion pattern as the user lookup methods. Ensure the RefreshTokenRecord interface matches the actual query result shape, including the nested user relationship.

Based on learnings, the Drizzle client from @pagespace/db is the approved approach for database access.


43-145: Well-structured repository implementation.

The repository provides a clean abstraction over database operations with:

  • Clear method signatures and documentation
  • Proper use of Drizzle ORM operations
  • Safe SQL increment operation using parameterized templates
  • Comprehensive token management functionality
apps/web/src/lib/repositories/index.ts (1)

12-12: LGTM! Proper barrel export.

The auth-repository export follows the established pattern for repository exports.

apps/web/src/app/api/auth/__tests__/me.test.ts (3)

1-61: Excellent test setup with repository boundary mocking.

The test file demonstrates good practices:

  • Clear documentation of testing approach
  • Proper mocking at the repository seam (not ORM chains)
  • Type-safe fixtures using the User type from the repository
  • Reusable createRequest helper

87-94: Strong security test coverage.

The test correctly verifies that sensitive fields like password and tokenVersion are not exposed in the API response. This is a critical security assertion.


124-128: Good verification of repository interaction.

The test properly verifies that the repository method is called with the authenticated user's ID, ensuring the contract between the route and repository is maintained.

apps/web/src/app/api/auth/login/route.ts (3)

79-91: Excellent timing-attack prevention.

The code correctly performs bcrypt comparison even for non-existent users using a fake hash. This prevents timing attacks that could reveal whether an email exists in the system.


119-128: Repository usage follows the new pattern correctly.

The refactored code properly uses authRepository.createRefreshToken with all required fields. The integration with device tokens is maintained through the deviceTokenId field.

As per coding guidelines, using the repository abstraction from @/lib/repositories/auth-repository aligns with the centralized database access pattern.


19-28: No changes needed. The error syntax z.string().min(5, { error: "..." }) is the correct Zod v4 pattern, and the code correctly uses this syntax. The error parameter optionally accepts either a string or a function, so the current implementation with a simple string error message is valid.

apps/web/src/app/api/auth/me/route.ts (1)

1-28: Clean refactor to repository pattern.

The route handler has been successfully refactored to use the repository abstraction:

  • User lookup now goes through authRepository.findUserById
  • Explicit 404 handling improves error responses
  • Debug logging is properly guarded by environment variables
  • Response correctly excludes sensitive fields

As per coding guidelines, the route now uses the repository from @/lib/repositories/auth-repository instead of direct database access.

apps/web/src/app/api/auth/__tests__/login.test.ts (4)

1-114: Comprehensive test setup with proper boundary mocking.

The test file demonstrates excellent practices:

  • Clear documentation of testing strategy
  • Repository-level mocking instead of ORM chain mocking
  • Type-safe fixtures using the User type from the repository
  • Reusable createRequest helper
  • Comprehensive mocking of all external boundaries

279-298: Excellent security test for timing-attack prevention.

This test verifies a critical security property: bcrypt comparison is performed even for non-existent users to prevent timing attacks. The test also validates that a proper bcrypt hash format is used for the fake comparison.


402-412: Good verification of rate limiting order.

The test correctly verifies that rate limiting checks occur before database queries, preventing unnecessary database load during rate-limited requests.


180-190: Repository interaction properly tested.

The test verifies that authRepository.createRefreshToken is called with the expected parameters, including the refresh token, userId, and platform. This ensures the route correctly delegates to the repository.

Comment thread apps/web/src/lib/repositories/auth-repository.ts
- Change emailVerified from boolean to Date | null to match DB schema
- Add PlatformType enum for platform field type safety
- Update me.test.ts to use Date objects for emailVerified assertions

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
apps/web/src/lib/repositories/auth-repository.ts (2)

49-54: Type assertion issue flagged in previous review.

The unsafe type assertion (user as User) on line 53 was previously identified as masking critical type mismatches between the database schema and the User interface. This violates the guideline to never use unsafe types and creates runtime error risk.

As noted in the previous review, use Drizzle's InferSelectModel<typeof users> to derive the User type from the schema to ensure type safety.


59-64: Same type assertion issue as findUserByEmail.

Line 63 contains the same unsafe type assertion (user as User) flagged in the previous review. This should be resolved by using InferSelectModel<typeof users> to derive the type.

🧹 Nitpick comments (1)
apps/web/src/lib/repositories/auth-repository.ts (1)

45-147: Consider adding error handling for robustness.

The repository methods currently allow database errors to propagate directly to callers. While this may be intentional (allowing route handlers to provide context), consider whether certain errors should be caught and transformed into domain-specific errors for better error boundaries.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6fdd5f and 448a57d.

📒 Files selected for processing (2)
  • apps/web/src/app/api/auth/__tests__/me.test.ts (1 hunks)
  • apps/web/src/lib/repositories/auth-repository.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Never use any types in TypeScript code - always use proper TypeScript types

**/*.{ts,tsx}: No any types - always use proper TypeScript types
Use kebab-case for filenames (e.g., image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: import getUserAccessLevel and canUserEditPage from @pagespace/lib/permissions
Use Drizzle client from @pagespace/db for all database access
Always structure message content using the message parts structure: { parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode

Files:

  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
apps/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/**/*.{ts,tsx}: Use centralized permission functions from @pagespace/lib/permissions for access control, such as getUserAccessLevel() and canUserEditPage()
Always use Drizzle client from @pagespace/db for database access instead of direct database connections
Always use message parts structure with parts array containing objects with type and text fields when constructing messages for AI

Files:

  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Format code with Prettier and lint with ESLint using the configuration at apps/web/eslint.config.mjs

Files:

  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
apps/web/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching

Files:

  • apps/web/src/lib/repositories/auth-repository.ts
  • apps/web/src/app/api/auth/__tests__/me.test.ts
apps/web/src/app/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST await context.params before destructuring because params are Promise objects
Get request body using const body = await request.json();
Get search params using const { searchParams } = new URL(request.url);
Return JSON responses using return Response.json(data) or return NextResponse.json(data)

Files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
🧠 Learnings (7)
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:15.319Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.319Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to packages/db/src/schema.ts : Maintain the Drizzle ORM database schema in `packages/db/src/schema.ts` as the single entry point for schema definitions

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to packages/db/{src/schema.ts,drizzle/**/*.ts} : Database schema must be defined in `packages/db/src/schema.ts` and migrations must be emitted to `packages/db/drizzle/`

Applied to files:

  • apps/web/src/lib/repositories/auth-repository.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to packages/lib/**/*.test.ts : Write unit tests for `packages/lib` and `apps/processor` with `*.test.ts` files next to source or in `__tests__/` directories

Applied to files:

  • apps/web/src/app/api/auth/__tests__/me.test.ts
🧬 Code graph analysis (1)
apps/web/src/app/api/auth/__tests__/me.test.ts (3)
apps/web/src/lib/repositories/auth-repository.ts (2)
  • User (11-22)
  • authRepository (45-147)
apps/web/src/lib/auth/index.ts (1)
  • isAuthError (204-206)
apps/web/src/app/api/auth/me/route.ts (1)
  • GET (4-28)
⏰ 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). (1)
  • GitHub Check: Unit Tests
🔇 Additional comments (2)
apps/web/src/app/api/auth/__tests__/me.test.ts (1)

1-72: Excellent test structure with proper boundary mocking.

The contract-based testing approach is well-implemented. Mocking at the repository seam (authRepository.findUserById) instead of ORM query chains significantly improves test maintainability and aligns with the PR objectives. The test fixtures are comprehensive and properly typed.

apps/web/src/lib/repositories/auth-repository.ts (1)

1-8: Well-documented repository with proper dependencies.

The purpose is clearly stated, and the use of Drizzle client from @pagespace/db follows the coding guidelines. The repository pattern successfully isolates ORM details from route handlers.

Comment thread apps/web/src/lib/repositories/auth-repository.ts
Address PR #104 code review comments:
- Export InferSelectModel/InferInsertModel from @pagespace/db
- Replace manual User/RefreshTokenRecord interfaces with schema-derived types
- Remove unsafe `as Type` assertions in auth-repository methods
- Update test fixtures with all required schema fields

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@2witstudios
2witstudios merged commit 0c8e7c8 into master Dec 21, 2025
3 checks passed
@2witstudios
2witstudios deleted the claude/improve-unit-tests-F3w6i branch January 29, 2026 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants