.coderabbit.yaml

# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
# More information at this link: https://docs.coderabbit.ai/configure-coderabbit

language: "en-US"
early_access: false
chat:
  auto_reply: true
issue_enrichment:
  auto_enrich:
    enabled: false
reviews:
  profile: "assertive"
  poem: false
  request_changes_workflow: true
  high_level_summary: true
  assess_linked_issues: true
  review_status: true
  review_details: false
  auto_apply_labels: false
  suggested_labels: false
  collapse_walkthrough: false
  auto_review:
    enabled: true
    drafts: false
    base_branches:
      - develop
      - main
  path_filters:
    - "!**/docs/docs/**"
    - "!*.html"
    - "!*.md"
    - "!*.svg"
  tools:
    ast-grep:
      enabled: true
      rule_dirs:
        - .coderabbit/ast-grep-rules
      essential_rules: true

  path_instructions:
    # GraphQL-specific review instructions
    - path: "src/graphql/**"
      instructions: |
        ## GraphQL Implementation Review

        **Resolver Implementation:**
        - Verify resolvers follow existing patterns in the codebase
        - Check that queries target correct database tables/entities
        - Validate foreign key relationships are correctly enforced
        - Confirm business logic constraints are properly implemented

        **Error Handling (6a Approach):**
        - Use Errors Union List + Interface Contract pattern
        - Distinguish between user errors and developer errors
        - Reference: docs/docs/docs/developer-resources/best-practices/error-handling.md

        **Pagination Standards:**
        - Require Relay-style connections for list queries
        - Validate `where` and `sortedBy` input naming conventions
        - Reference: docs/docs/docs/developer-resources/best-practices/pagination.md

        **Testing Requirements:**
        - Ensure corresponding test exists in test/graphql/types/{Mutation|Query}/
        - Tests must use document nodes from test/graphql/types/documentNodes.ts
        - Prefer mercuriusClient integration tests over vi.mock unit tests

    # Source code review instructions
    - path: "src/**"
      instructions: |
        ## Source Code Review

        **Implementation Correctness:**
        - Verify implementation logic is correct and free of bugs
        - Validate queries target correct database tables/entities
        - Check foreign key relationships are correctly enforced
        - Confirm business logic constraints are properly implemented
        - Verify proper error handling and status codes
        - CRITICAL: Any bugs or logic errors discovered MUST be fixed before approval

        **Code Quality:**
        - No console.log statements in production code
        - No unused imports or dead code (validated by knip)
        - No debug code left in production files
        - Follow existing patterns in src/utilities/ for reusable functions

        **Security & Best Practices:**
        - No exposed API keys, tokens, or credentials
        - Proper input validation and sanitization
        - No SQL injection or XSS vulnerabilities
        - Proper error handling without leaking sensitive information

        **Documentation:**
        - JSDoc/TSDoc comments required on public functions
        - Comments must include `@param` and `@returns` tags
        - Run `pnpm run generate:docs` if modifying public APIs (Check-AutoDocs enforces this)

        **Reusable Components:**
        - Follow reusable component policy: https://docs-api.talawa.io/docs/developer-resources/reusable-components

    # Test file review instructions
    - path: "test/**"
      instructions: |
        ## Test Review

        **Timezone Safety & Test Determinism (CRITICAL):**
        - REQUIRED: Use UTC date methods in all test assertions:
          - ✅ Use: `.getUTCDate()`, `.getUTCMonth()`, `.getUTCDay()`, `.getUTCHours()`, `.getUTCMinutes()`, `.getUTCSeconds()`
          - ❌ NEVER use: `.getDate()`, `.getMonth()`, `.getDay()`, `.getHours()`, `.getMinutes()`, `.getSeconds()`
        - All test dates must use fixed UTC timestamps (e.g., `"2025-01-01T10:00:00Z"`)
        - Avoid `new Date()` without arguments (uses current time)
        - Avoid `Date.now()` in tests (use fixed timestamps or mock timers)
        - Flag ANY usage of local timezone methods as CRITICAL - these cause CI flakiness in non-UTC environments

        **Test Pattern Requirements:**
        - Use mercuriusClient integration test pattern (preferred over vi.mock unit tests)
        - Import GraphQL document nodes from test/graphql/types/documentNodes.ts
        - Follow black box testing philosophy per test/graphql/types/README.md
        - Tests must be able to run concurrently

        **Test File Location:**
        - Mutation tests: test/graphql/types/Mutation/{operationName}.test.ts
        - Query tests: test/graphql/types/Query/{operationName}.test.ts
        - NOT in test/routes/graphql/ (that's for unit tests of route setup)

        **Coverage Requirements:**
        - 100% code coverage for new/modified implementation files
        - Identify specific line numbers lacking coverage
        - Cover all edge cases: success paths, error paths, boundary conditions
        - Include authentication/authorization checks
        - No redundant or duplicate test cases

        **Mock Accuracy & Correctness:**
        - Mocks accurately simulate production behavior they replace
        - Mocked functions verify correct arguments are passed (e.g., verify inArray receives correct IDs per batch)
        - Sequential mocks (mockResolvedValueOnce) return batch-specific data, not all data on every call
        - Mock return values match the actual API/function signature and data structure

        **Mock Isolation Anti-Patterns (CRITICAL for Sharded CI):**
        - **Direct Mock Assignment Detection:**
          - ❌ CRITICAL: Flag ANY occurrence of direct assignment to mock object methods:
            ```typescript
            // These bypass vitest mock tracking and cause test pollution:
            server.minio.client.putObject = vi.fn()
            server.drizzleClient.transaction = vi.fn()
            context.someService.someMethod = vi.fn()
            obj.prop = vi.fn()
            ```
          - ✅ REQUIRE: Use vi.spyOn() for all method mocking:
            ```typescript
            const spy = vi.spyOn(server.minio.client, 'putObject');
            const txSpy = vi.spyOn(server.drizzleClient, 'transaction');
            ```
          
        - **Why This Matters:**
          - Direct assignment bypasses vitest's mock registry
          - vi.restoreAllMocks() will NOT restore these mocks
          - Causes test pollution and flaky failures in sharded CI
          - Root cause of issue `#5169` (MinIO test flakiness)

        - **Detection Patterns:**
          - Search for: `= vi.fn(` in test files
          - Check if left-hand side is a property access (obj.prop)
          - If found, require refactor to vi.spyOn()
          - Even with try/finally cleanup, prefer vi.spyOn() for safety

        - **Common Offenders:**
          - server.minio.client.* methods
          - server.drizzleClient.* methods
          - Any context.* property assignments
          - Any obj.nested.prop = vi.fn() patterns

        **Test Cleanup & Hygiene:**

        - **Database Cleanup Required:**
          - ❌ CRITICAL: `afterEach` with ONLY `vi.restoreAllMocks()` is INSUFFICIENT
          - ✅ REQUIRED: Database cleanup must happen in `afterEach` for all integration tests
          - Tests creating database entities (orgs, users, events, etc.) MUST clean them up
          - Options: transaction rollback, explicit cleanup functions, or test database isolation
          - Flag missing database cleanup as HIGH PRIORITY blocking issue

        - **Resource Creation Patterns:**
          - ❌ AVOID: Creating shared resources (auth tokens, users) in `beforeAll` for use across multiple tests
          - ✅ PREFER: Create per-test resources in `beforeEach` or within individual tests
          - `beforeAll` acceptable only for truly immutable shared setup (e.g., test utilities, constants)
          - Auth tokens should be created per-test to avoid cascading failures

        - **Data Accumulation Risks:**
          - **CRITICAL:** Identify tests that create data without cleanup
          - Calculate accumulation: (number of tests) × (entities per test) = risk level
          - Flag files with >10 tests and no cleanup as HIGH RISK for sharded execution
          - Warn about shared database state contamination across parallel test shards

        - **Cleanup Order Validation:**
          - Cleanup MUST occur in reverse dependency order
          - Example: commentVotes → comments → posts → organizations → users
          - Check for proper foreign key cascade awareness
          - Flag any cleanup that deletes parent entities before children

        - **Transaction Isolation (Preferred):**
          - Prefer transaction-based isolation: `drizzleClient.transaction(async (tx) => {...})`
          - Transactions COMMIT on success; to force rollback for test isolation, throw an error
          - Example: Wrap transaction in expect().rejects.toThrow() to force rollback
          - Otherwise, use explicit afterEach cleanup even with transactions
          - If transactions are NOT used, explicit afterEach/afterAll is REQUIRED
          - Flag any test that modifies database state outside transactions without cleanup

        - **Mandatory Cleanup Hooks:**
          - Every test file MUST include cleanup hooks (afterEach, afterAll, or both)
          - Flag any test file that creates database records without cleanup
          - Verify cleanup hooks are present for tests that:
            * Call createRegularUserUsingAdmin() or similar user creation
            * Call createTestOrganization() or similar entity creation
            * Insert directly into drizzle tables (drizzleClient.insert)
            * Create cascading dependencies (user → org → post → comment → vote)

        - **Test Isolation Verification:**
          - Each test must be runnable in isolation and in any order
          - Tests must not depend on data created by previous tests
          - No database state should leak between tests
          - Verify tests pass when run with `--shard` and `--isolate` flags

        - **Cleanup Completeness Check:**
          - For every `create*()` helper call, verify corresponding cleanup exists
          - For every database write (insert, update), verify cleanup or rollback
          - Watch for: Organizations, Events, Users, Folders, Categories, Agenda Items, etc.
          - Example pattern:
            ```typescript
            afterEach(async () => {
              vi.restoreAllMocks();
              // Clean up test data
              await db.delete(agendaItems).where(eq(agendaItems.id, testItemId));
              await db.delete(categories).where(eq(categories.id, testCategoryId));
              // ... etc
            });
            ```       
        - Proper beforeEach/afterEach hooks for all state management (timers, mocks, global state)
        - vi.useFakeTimers() is paired with vi.useRealTimers() in afterEach, not inline
        - No floating-point arithmetic in test expectations without proper precision handling
        - Mock state is properly reset between tests (vi.clearAllMocks() in beforeEach)
        - No test pollution - each test can run independently in any order
        - Use faker.string.uuid() for unique test data to enable concurrent execution
        - Avoid hardcoded timeouts; handle variable CI response times
        - Proper mock cleanup between tests
        - Check for potential race conditions in parallel test execution

        **Mock Verification Depth:**
        - When testing batch processing, verify each batch receives/processes correct subset of data
        - When testing filtering logic (WHERE, inArray, etc.), verify filters receive correct parameters
        - Spy on function calls and assert on arguments, not just return values
        - Mock call order matters for sequential operations - verify with mock.calls[] inspection

        **Test Structure:**
        - Sign in as admin using Query_signIn when authentication required
        - Pass auth token via headers: { authorization: `bearer ${authToken}` }
        - Assert both data and errors as appropriate
        - Use expect.objectContaining for flexible error matching

    # 1. Field Resolver Tests (User/, Event/, Organization/, etc.) - Should use unit tests
    - path: "test/graphql/types/*/!(Mutation|Query|Subscription)/**/*.test.ts"
      instructions: |
        ## Testing Pattern for Field Resolvers

        **Required approach:** Use `createMockGraphQLContext()` for unit testing

        **Check for:**
        1. ❌ If the test imports `mercuriusClient` from `../../testClient/mercuriusClient`, flag this as incorrect:
           - "Field resolver tests should use `createMockGraphQLContext()` for isolated unit testing, not `mercuriusClient`."
           - "This approach is 10-100x faster, doesn't require database setup, and avoids cleanup complexity."
           - "See `test/graphql/types/User/eventsAttended.test.ts` for the correct pattern."

        2. ✅ If the test imports `createMockGraphQLContext` from the mock context creator, verify:
           - Tests mock database queries using `mocks.drizzleClient.query.*.findFirst.mockResolvedValue()`
           - Context is properly typed as `GraphQLContext`
           - No real database operations occur

        3. ⚠️ Rare exception: If this is genuinely testing cross-resolver integration, the test should include a comment explaining why E2E is necessary.

        **Example correct pattern:**
        ```typescript
        import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator/mockContextCreator";

        describe("User field resolver", () => {
          let ctx: GraphQLContext;
          let mocks: ReturnType<typeof createMockGraphQLContext>["mocks"];
          
          beforeEach(() => {
            const { context, mocks: newMocks } = createMockGraphQLContext(true, "user-123");
            ctx = context;
            mocks = newMocks;
          });
          
          it("returns data when authorized", async () => {
            mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue({...});
            // Test resolver logic
          });
        });
        ```

    # 2. Operation Tests (Mutation/, Query/) - E2E is appropriate
    - path: "test/graphql/types/{Mutation,Query,Subscription}/**/*.test.ts"
      instructions: |
        ## Testing Pattern for GraphQL Operations

        **Accepted approach:** `mercuriusClient` (E2E) is appropriate for top-level operations

        **Required cleanup checks:**
        1. 🔍 For every test that calls `Mutation_createUser`, verify there's a corresponding `Mutation_deleteUser`:
           - Count `mercuriusClient.mutate(Mutation_createUser` occurrences
           - Count `mercuriusClient.mutate(Mutation_deleteUser` occurrences
           - If counts don't match, flag: "Created users must be deleted to prevent orphaned data in sharded CI environments."

        2. 🔍 For any mutation that creates resources (createOrganization, createEvent, etc.), verify cleanup:
           - Check for corresponding delete mutations in the same test or afterEach/afterAll hooks
           - Flag missing cleanup as a flakiness risk

        3. ✅ Verify tests use `faker.string.uuid()` or similar for unique identifiers to avoid conflicts in parallel execution

        **Pattern to encourage:**
        ```typescript
        it("creates and manages user", async () => {
          const result = await mercuriusClient.mutate(Mutation_createUser, {...});
          const userId = result.data.createUser.user.id;
          
          // ... perform tests ...
          
          // Cleanup: delete the created user
          await mercuriusClient.mutate(Mutation_deleteUser, {
            headers: { authorization: `bearer ${token}` },
            variables: { input: { id: userId } }
          });
        });
        ```

    # Universal review instructions for all files
    - path: "**/*"
      instructions: |
        ## Universal Review Criteria

        **PR Requirements Validation:**
        - Verify PR meets requirements of linked issue
        - All acceptance criteria addressed
        - No out-of-scope changes included
        - All previous review comments addressed
        - Proper title, description, and issue linkage
        - Target branch is correct (typically `develop`)

        **Test File Specific Checks:**
        - In test files (test/**/*.test.ts), watch for direct mock assignments
        - Pattern: `object.property = vi.fn()` indicates mock isolation risk
        - These bypass vi.restoreAllMocks() and cause CI flakiness
        - Reference: Issue `#5169`, PR `#5180`

        **CI/CD Pipeline Compatibility:**
        - Validate changes won't break any of the 26 CI jobs across 11 workflows
        - Key checks: Check-AutoDocs, Check-Sensitive-Files (monitors 43 file patterns), python_checks, check_type_errors
        - Pre-commit requirements: Prettier formatting, ESLint, TypeScript type checking
        - Check-Sensitive-Files can be bypassed with `ignore-sensitive-files-pr` label if needed
        - Note: codecov/project may fail when adding tests due to meta-coverage; acceptable if codecov/patch passes

        **Documentation:**
        - Auto-generated docs must be current (run `pnpm run generate:docs`)
        - README.md updated if new features added
        - Adequate JSDoc comments on public functions

        **Identify Issues:**
        - Unnecessary files (build artifacts, node_modules, .env files, IDE configs)
        - Files unrelated to PR scope
        - Generated files that should be in .gitignore

        **Issue Requirement Validation:**
        - Evaluate whether goals of the linked issue are met
        - All acceptance criteria from issue satisfied
        - Implementation matches described solution approach

        **Review Decision Criteria:**

        REQUEST CHANGES when ANY of these apply:
        - Missing test coverage or invalid/insufficient tests
        - Code quality issues, security concerns, or bugs
        - Incomplete implementation of issue requirements
        - Bugs or logic errors in implementation files under test
        - Non-compliance with previous feedback
        - Presence of unnecessary files
        - Failing CI checks requiring code changes
        - Test file creates database records without cleanup hooks
        - No afterEach/afterAll and no transaction isolation
        - Cleanup order is incorrect (deletes parents before children)
        - High risk of data accumulation (>10 tests, no cleanup)

        APPROVE ONLY when ALL of these are satisfied:
        - Complete test coverage (≥95% new/modified code, ≥80% overall)
        - All tests follow proper patterns (mercuriusClient integration tests)
        - No code quality issues, security concerns, or bugs
        - Full implementation of issue requirements
        - Implementation files under test are correct and bug-free
        - Compliance with all previous feedback
        - No unnecessary files
        - All CI checks passing (or failures unrelated to PR)

        **Approval Process:**
        - Only append [approve] tag when approving
        - Zero changes required (not even trivial ones)
        - Confident PR is ready to merge

        **Important Notes:**
        - Test-focused PRs should maintain clear scope, BUT bugs in files being tested must be fixed in same PR
        - Bugs in unrelated files may be deferred to separate issues
        - Prioritize substantive issues over minor stylistic suggestions
        - Focus on issues causing CI failures or production bugs
        - Post response as single easily readable comment