docs: simplify frontend Sentry config vars#35
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies frontend Sentry configuration by removing build-time VITE_SENTRY_* fallbacks and standardizing on runtime FRONTEND_SENTRY_* values served via /api/runtime-config.
Changes:
- Update
/api/runtime-configto read onlyFRONTEND_SENTRY_*(dropVITE_SENTRY_*fallback). - Update frontend Sentry bootstrap to initialize only from runtime config (no
import.meta.env.VITE_*usage). - Update docs/templates and add a test intended to ensure
VITE_*vars are ignored.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/runtime-config.test.ts | Adds a test for runtime-config behavior around ignoring VITE_SENTRY_*. |
| src/server/index.ts | Removes VITE_SENTRY_* fallback in runtime config endpoint response. |
| src/app/sentry.client.ts | Removes build-time Sentry config fallbacks; relies on runtime config and skips init without DSN. |
| README.md | Updates quick start/docs and removes VITE_SENTRY_* env var documentation. |
| .env.example | Replaces Vite Sentry env vars with FRONTEND_SENTRY_* runtime env vars. |
| .agents/docs/implementation-history.md | Updates internal docs to reflect FRONTEND_SENTRY_* only. |
| .agents/docs/current-state.md | Updates internal docs to reflect runtime-only Sentry configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### Docker Compose | ||
|
|
||
| `edge` tag refers to the default branch. | ||
|
|
There was a problem hiding this comment.
In a Docker Compose context, localhost inside the container refers to the container itself, not sibling services. If this snippet is meant to be a self-contained Compose example, consider adding redis/minio services and using service hostnames (e.g., redis://redis:6379, http://minio:9000). If it’s meant for externally-hosted Redis/MinIO, clarify that in the text so readers don’t copy a non-working default.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("returns FRONTEND_SENTRY_* values when provided", async () => { | ||
| vi.stubEnv("FRONTEND_SENTRY_DSN", "front-dsn"); | ||
| vi.stubEnv("VITE_SENTRY_DSN", "vite-dsn"); | ||
| vi.stubEnv("FRONTEND_SENTRY_ENABLE_LOGS", "false"); | ||
|
|
||
| const app = Fastify(); | ||
| registerRuntimeConfigRoute(app); | ||
|
|
||
| try { | ||
| const res = await app.inject({ method: "GET", url: "/api/runtime-config" }); | ||
| expect(res.statusCode).toBe(200); | ||
| const body = res.json() as any; | ||
| expect(body.sentry.dsn).toBe("front-dsn"); | ||
| expect(body.sentry.enableLogs).toBe(false); | ||
| } finally { | ||
| await app.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The test only verifies two fields (dsn and enableLogs) but the endpoint returns eight fields (dsn, environment, release, tracesSampleRate, enableLogs, enableMetrics, replaysSessionSampleRate, replaysOnErrorSampleRate). Consider adding assertions for the other fields to ensure the endpoint returns complete and correct data, including default values when environment variables are not set.
src/server/runtime-config.ts
Outdated
| return { | ||
| sentry: { | ||
| dsn: sentryDsn, | ||
| environment: process.env.FRONTEND_SENTRY_ENVIRONMENT || config.NODE_ENV, | ||
| release: process.env.FRONTEND_SENTRY_RELEASE, | ||
| tracesSampleRate: process.env.FRONTEND_SENTRY_TRACES_SAMPLE_RATE || "0.1", | ||
| enableLogs: (process.env.FRONTEND_SENTRY_ENABLE_LOGS || "true") !== "false", | ||
| enableMetrics: (process.env.FRONTEND_SENTRY_ENABLE_METRICS || "true") !== "false", | ||
| replaysSessionSampleRate: process.env.FRONTEND_SENTRY_REPLAYS_SESSION_SAMPLE_RATE || "0.1", | ||
| replaysOnErrorSampleRate: process.env.FRONTEND_SENTRY_REPLAYS_ON_ERROR_SAMPLE_RATE || "1.0", | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The return value of this endpoint lacks type safety. The client expects a RuntimeSentryConfig type with optional fields, but this function returns an inline object with no type definition. Consider defining a shared type or extracting the RuntimeSentryConfig type from sentry.client.ts to a shared location to ensure type consistency between client and server.
Only FRONTEND_SENTRY_* env vars are used at runtime, build-time VITE_SENTRY_* options removed from docs and code test: add runtime-config test ensuring VITE vars ignored Co-Authored-By: GitHub Copilot <noreply@agents.md>
Use shared helper in index and tests, removing duplicate implementation docs: fix README grammar for MinIO credentials Co-Authored-By: GitHub Copilot <noreply@agents.md>
close Fastify instances and avoid direct process.env mutation Co-Authored-By: GitHub Copilot <noreply@agents.md>
Add redis/minio services to snippet and note container networking semantics Co-Authored-By: GitHub Copilot <noreply@agents.md>
run oxfmt to satisfy fmt:check Co-Authored-By: GitHub Copilot <noreply@agents.md>
4dacf9c to
571110a
Compare
ensure docs are correctly formatted post-merge Co-Authored-By: GitHub Copilot <noreply@agents.md>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // mount shared route helper | ||
| registerRuntimeConfigRoute(app); | ||
|
|
There was a problem hiding this comment.
This refactor removes the previous /api/runtime-config handler that exposed features.enableS3UriCopy (and resolved it via OpenFeature). Because the UI consumes that field to enable the Copy S3 URI feature, moving the route to registerRuntimeConfigRoute needs to preserve that behavior or the feature flag will stop working.
| | Variable | Required | Default | Description | | ||
| | ---------------------------------------------- | -------- | ---------------- | --------------------------------------------- | | ||
| | `NODE_ENV` | no | `development` | Runtime environment | | ||
| | `PORT` | no | `3000` | API/web app port | | ||
| | `REDIS_URL` | yes | - | Redis connection URL | | ||
| | `SESSION_TTL_SECONDS` | no | `86400` | Session TTL in seconds | | ||
| | `COOKIE_NAME` | no | `atrium_session` | Session cookie name | | ||
| | `BUCKET_SIZE_CALC_INTERVAL_HOURS` | no | `1` | Background bucket-size job interval (hours) | | ||
| | `BUCKET_SIZE_MAX_DURATION_MS` | no | `300000` | Max runtime per bucket size calculation | | ||
| | `BUCKET_SIZE_MAX_OBJECTS` | no | `1000000` | Max objects scanned per calculation | | ||
| | `S3_ENDPOINT` | yes | - | S3-compatible endpoint URL | | ||
| | `S3_REGION` | yes | - | S3 region string | | ||
| | `S3_FORCE_PATH_STYLE` | no | `true` | Use path-style S3 URLs (needed by MinIO) | |
There was a problem hiding this comment.
The environment variables table is duplicated starting at this second header row (the same variables are listed twice). This makes the README confusing; please remove the duplicate table block and keep a single authoritative table.
| | Variable | Required | Default | Description | | |
| | ---------------------------------------------- | -------- | ---------------- | --------------------------------------------- | | |
| | `NODE_ENV` | no | `development` | Runtime environment | | |
| | `PORT` | no | `3000` | API/web app port | | |
| | `REDIS_URL` | yes | - | Redis connection URL | | |
| | `SESSION_TTL_SECONDS` | no | `86400` | Session TTL in seconds | | |
| | `COOKIE_NAME` | no | `atrium_session` | Session cookie name | | |
| | `BUCKET_SIZE_CALC_INTERVAL_HOURS` | no | `1` | Background bucket-size job interval (hours) | | |
| | `BUCKET_SIZE_MAX_DURATION_MS` | no | `300000` | Max runtime per bucket size calculation | | |
| | `BUCKET_SIZE_MAX_OBJECTS` | no | `1000000` | Max objects scanned per calculation | | |
| | `S3_ENDPOINT` | yes | - | S3-compatible endpoint URL | | |
| | `S3_REGION` | yes | - | S3 region string | | |
| | `S3_FORCE_PATH_STYLE` | no | `true` | Use path-style S3 URLs (needed by MinIO) | |
| MINIO_ROOT_USER: "minioadmin" | ||
| MINIO_ROOT_PASSWORD: "minioadmin" | ||
| ports: | ||
| - "9000:9000" |
There was a problem hiding this comment.
The Docker Compose snippet exposes only port 9000 for MinIO, but later in the Quick Start section the README directs users to the MinIO Console on port 9001. Either add a 9001:9001 port mapping (and MinIO console address if needed) or update the later instructions to match what’s actually exposed.
| - "9000:9000" | |
| - "9000:9000" | |
| - "9001:9001" |
| - UI supports both manual pagination (**Load more**) and optional auto-load on scroll. | ||
| - UI supports creating folders with validation and navigation on success. | ||
| - Frontend Sentry is initialized at runtime via `/api/runtime-config` (with Vite env fallback). | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
This file contains an unresolved git merge conflict marker (<<<<<<< HEAD). Please resolve the conflict and remove all conflict markers before merging; otherwise the docs are left in a broken state.
| <<<<<<< HEAD |
tests: assert all returned fields and feature flag docs: expose 9001 port for MinIO in compose snippet Co-Authored-By: GitHub Copilot <noreply@agents.md>
post-pull formatting adjustments Co-Authored-By: GitHub Copilot <noreply@agents.md>
Align server tsconfig rootDir/outDir for shared types and remove server import dependency on app types to fix TS6059 across CI jobs. Co-Authored-By: GPT-5.3-Codex <noreply@agents.md>
…ests The runtime-config route uses OpenFeature.getClient().getBooleanValue() to read feature flags, which requires a provider to be configured. Without initialization, the OpenFeature client throws an error and defaults to false. Added beforeAll hook to set up EnvVarProvider, matching the production server configuration that uses EnvVarProvider as a fallback. This allows tests to read ENABLE_S3_URI_COPY from environment variables via vi.stubEnv(). Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Only FRONTEND_SENTRY_* env vars are used at runtime, build-time VITE_SENTRY_* options removed from docs and code
test: add runtime-config test ensuring VITE vars ignored
Co-Authored-By: GitHub Copilot noreply@agents.md