Repository navigation
Add health check endpoint, error pages, and environment validation - #271
Merged
Merged
Conversation
Add foundational production features that were missing: - Environment validation with Zod schema at startup - Validates required env vars (DATABASE_URL, JWT_SECRET, etc.) - Prevents runtime failures from missing configuration - packages/lib/src/config/env-validation.ts - Health check endpoint for load balancers and orchestration - GET /api/health with database connectivity check - Memory usage metrics and service info - Proper Cache-Control headers - apps/web/src/app/api/health/route.ts - Global error pages for better UX - error.tsx with retry and navigation options - not-found.tsx with branded 404 page - apps/web/src/app/error.tsx, not-found.tsx - Request ID middleware for distributed tracing - Preserves incoming X-Request-Id for tracing - Generates new ID if not present - apps/web/src/lib/request-id/request-id.ts - Fetch with timeout utility for external API calls - Prevents hanging requests from exhausting resources - Predefined timeout constants (SHORT, MEDIUM, LONG) - TimeoutError class for proper error handling - packages/lib/src/utils/fetch-with-timeout.ts All features include comprehensive TDD test coverage. https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
Contributor
📝 WalkthroughWalkthroughAdds a health check API route and tests, request-id utilities and middleware integration, global Error and NotFound UI pages, environment validation and timeout-aware fetch utilities with tests, and adjusts several tests' DB cleanup ordering and imports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthRoute as Health Route
participant DB as Database
participant Logger
Client->>HealthRoute: GET /api/health
HealthRoute->>DB: lightweight query (SELECT 1)
DB-->>HealthRoute: success / error / timeout
HealthRoute->>Logger: info / warn / error (based on DB result)
HealthRoute->>HealthRoute: gather process.memoryUsage(), timestamp, version
HealthRoute-->>Client: 200 (healthy) or 503 (degraded) with JSON + Cache-Control:no-store, no-cache, must-revalidate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/health/route.ts`:
- Around line 60-66: The route currently constructs responses with new
Response(JSON.stringify(response), { status: statusCode, headers: {
'Content-Type': 'application/json', ... } }); — replace these with
Response.json(response, { status: statusCode, headers: { 'Cache-Control':
'no-store, no-cache, must-revalidate' } }) so Content-Type is set automatically;
do the same replacement for the error response that builds JSON manually (the
block using JSON.stringify(errorResponse) / statusCode for errors) and remove
the explicit 'Content-Type' header in both places.
In `@apps/web/src/app/error.tsx`:
- Around line 31-34: Update the misleading copy inside CardDescription (near
CardTitle "Something went wrong" in error.tsx) so it no longer claims the team
was notified; either replace the sentence with a neutral message like "An
unexpected error occurred. Please try again or contact support." or wire actual
telemetry by invoking your telemetry/logging function (e.g., call trackException
or sendErrorReport from your analytics/logger in the error boundary that renders
this Card) and then keep the original "team has been notified" text only if that
telemetry call is added and awaited/handled.
In `@apps/web/src/app/not-found.tsx`:
- Around line 12-43: This file is a Server Component but uses client-only
features (onClick handler and history.back()) in NotFoundPage; add the 'use
client' directive as the very first line of the file to opt into a Client
Component so the Button onClick and history.back() will run in the browser, then
keep the existing onClick={() => history.back()} (or optionally replace with
window.history.back() or next/router's router.back() inside the same component)
to ensure the back action executes on the client.
In `@apps/web/src/lib/request-id/request-id.ts`:
- Around line 10-21: The isValidRequestId function is too permissive; update it
to only accept CUID2 IDs by replacing the generic alphanumeric/underscore/hyphen
check with a strict CUID2 validation. Locate isValidRequestId and either call
the project’s CUID2 validator/util (preferred) or replace the regex with the
canonical CUID2 pattern from the spec (lowercase alphanumeric structure and
exact length constraints used across PageSpace) so only valid CUID2 strings
pass.
In `@packages/lib/src/config/__tests__/env-validation.test.ts`:
- Around line 17-27: The test suite leaks host environment into tests causing
flakiness; update the beforeEach in the 'env-validation' describe block so it
does not restore the full originalEnv but instead resets process.env to a
minimal, deterministic set (or an empty object) and explicitly set any required
vars used by validateEnv()/getEnvErrors(); keep afterEach restoring originalEnv
to avoid side effects. Locate the beforeEach/afterEach block and change
process.env = { ...originalEnv } to something like process.env = { NODE_ENV:
'test', MY_REQUIRED_VAR: 'value' } or {} and explicitly clear optional URL/env
keys that can break validation so validateEnv() and getEnvErrors() run
deterministically.
In `@packages/lib/src/utils/fetch-with-timeout.ts`:
- Around line 43-73: The fetchWithTimeout function currently overwrites the
caller's AbortSignal and treats all AbortError as timeouts; change it to compose
the caller signal with the internal AbortController by adding an event listener
to the caller's signal (using AbortSignal.addEventListener) that calls
controller.abort() and set a boolean flag (e.g., timedOut) in the timeout
callback to indicate the internal timer fired, ensure you remove the external
listener and clear the timeout in the finally block, and only map an AbortError
to a TimeoutError when that timedOut flag is true (otherwise rethrow the
original abort error).
🧹 Nitpick comments (1)
apps/web/src/app/api/health/__tests__/route.test.ts (1)
33-96: Optional: extract small helpers to reduce repetition.The healthy-path tests repeat request creation and DB setup; a tiny helper improves readability and keeps future additions consistent.
♻️ Suggested refactor
+const createHealthRequest = () => + new Request('https://example.com/api/health', { method: 'GET' }); + +const mockDbHealthy = () => { + mockExecute.mockResolvedValue([{ '1': 1 }]); +}; + describe('GET /api/health', () => { beforeEach(() => { vi.clearAllMocks(); }); describe('healthy system', () => { it('given database is connected, should return healthy status', async () => { - mockExecute.mockResolvedValue([{ '1': 1 }]); + mockDbHealthy(); - const request = new Request('https://example.com/api/health', { - method: 'GET', - }); + const request = createHealthRequest();
- Make CSRF_SECRET and ENCRYPTION_KEY optional in test environment - Use Zod superRefine for conditional validation at parse time - Add tests for test environment validation behavior - Fix unused request parameter lint error in health route https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
- Remove docblocks that reiterate what code/names already express - Remove redundant inline comments (e.g., regex explanation) - Keep minimal docblocks for public API functions that add value - Clean up test file headers (describe blocks are self-explanatory) https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
- Use Response.json() instead of new Response(JSON.stringify()) in health route - Fix misleading "team notified" copy in error.tsx - Add 'use client' to not-found.tsx for onClick/history.back() - Make isValidRequestId CUID2-only using official isCuid validator - Fix env-validation test isolation (reset to minimal deterministic env) - Fix fetchWithTimeout to compose AbortSignals properly (caller vs timeout) https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
Delete tables in foreign key order (children first) to avoid deadlock from cascade contention when cleaning up test data: 1. pagePermissions 2. pages 3. driveMembers 4. drives 5. users This prevents the PostgreSQL deadlock that occurred in CI when cascading deletes from the users table tried to acquire locks on multiple tables simultaneously. https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/error.tsx`:
- Around line 37-47: Replace direct rendering of error.message (and optionally
error.digest) with an environment-gated display: in the component that renders
CardContent, change the JSX to show a generic user-facing message like "An
unexpected error occurred" when process.env.NODE_ENV !== 'development', and only
render the raw error.message (and detailed digest) when process.env.NODE_ENV ===
'development' so detailed error text is available for dev-only views; update
both occurrences where error.message/error.digest are used to follow this
pattern.
🧹 Nitpick comments (3)
packages/lib/src/utils/__tests__/fetch-with-timeout.test.ts (2)
100-112: Consider adding assertion for actual signal composition.This test verifies that passing a caller signal doesn't break the function, but it doesn't verify that the composition actually works—e.g., that aborting the caller's signal mid-request triggers the abort. Consider adding a test that aborts
callerControllerwhile fetch is pending and verifies the originalAbortErrorpropagates.
114-134: Consider verifying the default timeout was actually applied.These tests confirm that zero/negative timeouts don't cause errors, but they don't assert that
DEFAULT_TIMEOUT_MSwas used. You could spy onsetTimeoutto verify it was called with30000.💡 Example assertion
it('given zero timeout, should use default timeout', async () => { const mockResponse = new Response('ok', { status: 200 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); const setTimeoutSpy = vi.spyOn(global, 'setTimeout'); await fetchWithTimeout('https://api.example.com/data', { timeout: 0, }); expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), DEFAULT_TIMEOUT_MS); });apps/web/src/app/error.tsx (1)
50-63: Use router-based navigation to preserve client state and keep navigation in-app.Replace
window.location.href = '/dashboard'withrouter.push('/dashboard')from Next.js'suseRouterhook. Error boundaries are client components, so hooks are fully supported.♻️ Suggested refactor
import { useEffect } from 'react'; +import { useRouter } from 'next/navigation'; import { Button } from '@/components/ui/button'; @@ export default function ErrorPage({ error, reset }: ErrorPageProps) { + const router = useRouter(); useEffect(() => { console.error('Global error boundary caught:', error); }, [error]); @@ <Button variant="outline" - onClick={() => (window.location.href = '/dashboard')} + onClick={() => router.push('/dashboard')} className="w-full" >
Comment on lines
+37
to
+47
| <CardContent className="space-y-4"> | ||
| <div className="bg-muted p-3 rounded-md text-sm"> | ||
| <div className="font-medium mb-1">Error Details:</div> | ||
| <div className="text-muted-foreground break-words"> | ||
| {error.message || 'An unexpected error occurred'} | ||
| </div> | ||
| {error.digest && ( | ||
| <div className="text-xs text-muted-foreground mt-2"> | ||
| Error ID: {error.digest} | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
Avoid leaking raw error messages to end users in production.
error.message can expose sensitive details; prefer a generic message outside dev and keep detailed info for dev-only views.
🔒 Suggested fix: gate detailed messages to development
export default function ErrorPage({ error, reset }: ErrorPageProps) {
useEffect(() => {
console.error('Global error boundary caught:', error);
}, [error]);
+ const IS_DEV = process.env.NODE_ENV === 'development';
return (
@@
<div className="text-muted-foreground break-words">
- {error.message || 'An unexpected error occurred'}
+ {IS_DEV && error.message
+ ? error.message
+ : 'An unexpected error occurred'}
</div>
@@
- {process.env.NODE_ENV === 'development' && error.stack && (
+ {IS_DEV && error.stack && (Also applies to: 66-76
🤖 Prompt for AI Agents
In `@apps/web/src/app/error.tsx` around lines 37 - 47, Replace direct rendering of
error.message (and optionally error.digest) with an environment-gated display:
in the component that renders CardContent, change the JSX to show a generic
user-facing message like "An unexpected error occurred" when
process.env.NODE_ENV !== 'development', and only render the raw error.message
(and detailed digest) when process.env.NODE_ENV === 'development' so detailed
error text is available for dev-only views; update both occurrences where
error.message/error.digest are used to follow this pattern.
Only show error.message in development to prevent information disclosure. The error.digest (Error ID) is still shown in production for support reference. Addresses CodeRabbit review comment on PR #271. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds critical infrastructure improvements for system reliability and observability, including a health check endpoint for load balancers, global error handling pages, request ID propagation for distributed tracing, and comprehensive environment variable validation.
Key Changes
Health Check Endpoint (
/api/health)GET /api/healthendpoint that returns system health status with HTTP 200 (healthy) or 503 (degraded)Error Handling Pages
error.tsx): Catches unhandled errors with user-friendly UI, error details, and development stack tracesnot-found.tsx): Custom page for missing routes with navigation options and GitHub issue reporting linkRequest ID Propagation
request-idutility module for distributed tracing supportgetOrCreateRequestId(): Extracts incomingX-Request-Idheader or generates new CUID2 IDisValidRequestId(): Validates IDs to prevent header injection attacks (alphanumeric, hyphens, underscores, max 128 chars)Environment Variable Validation
env-validationmodule with Zod schema for server-side configurationDATABASE_URL,JWT_SECRET,JWT_ISSUER,JWT_AUDIENCE,CSRF_SECRET,ENCRYPTION_KEYvalidateEnv()(throws),getEnvErrors()(returns array),isEnvValid()(returns boolean)getValidatedEnv()to validate only onceFetch Utility Enhancement
fetchWithTimeoututility for external API calls with timeout protectionTimeoutErrorclass for distinguishing timeout errorsImplementation Details
https://claude.ai/code/session_01YKdWhqHpkjMH9Eobi9NpvH
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.