feat(ingestion): /e alias, preview detection, strict SDK URL validation#12
Merged
Merged
Conversation
… SDK URL validation - Register `/e` as short alias for `/ingest` endpoint; SDK now hits `/e` by default - Detect Vercel preview hostnames (via origin or host header) and tag events with `isPreview` - SDK no longer falls back to localhost when no ingest URL configured — drops event and logs error instead - Expand ops script with verification menu and new unit tests - Dashboard: refactor queries, sidebar, signal-stream, world-map, geo-details components Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @remcostoeten, your pull request is larger than the review limit of 150000 diff characters
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (43)
📝 WalkthroughWalkthroughThis PR introduces date-range-based query parameter state management for the dashboard, adds a new Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant Dashboard as Dashboard UI
participant Router as Next.js Router
participant API as Analytics API
participant DB as Database
User->>Dashboard: Select project & time range
Dashboard->>Router: Update URL params<br/>(projectId, timeRange)
Router->>Dashboard: Reflect query params
Dashboard->>API: Fetch analytics with<br/>from/to/limit params
API->>DB: Query KPI/geo/trend<br/>with time window
DB-->>API: Return filtered results
API-->>Dashboard: JSON response
Dashboard->>Dashboard: Render metrics & geo-details
Dashboard-->>User: Display analytics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/example-dashboard/lib/queries.ts (2)
833-850:⚠️ Potential issue | 🟡 MinorWire back
is_internalfrom database to response or remove the field.The
SELECTquery excludesis_internaldespite it being populated in ingestion with real values. The response hardcodesfalse, dropping signal that is operationally meaningful (used for filtering internal vs. public traffic). The type definition invisitors-table.tsxalso lacks this field, creating a mismatch. Either include it properly from the database or remove it from the response shape entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries.ts` around lines 833 - 850, The returned visitor mapping currently hardcodes isInternal:false and the SELECT that populates results omits is_internal; fix by selecting is_internal in the query that produces results and map it through in the results.map (e.g., set isInternal based on r.is_internal, converting to boolean as needed) and also add the isInternal field to the visitor type in visitors-table.tsx so types align; alternatively, if you intend to drop this signal, remove the isInternal property from the map and the type instead—update the SELECT, the mapping in the results.map block, and the visitor type in visitors-table.tsx accordingly.
439-451:⚠️ Potential issue | 🟡 MinorRename
quality.localhostTraffictoquality.localhostRateand align with mock data.
localhostTrafficstores a percentage (0–100) fromlocalhostRateKPI.valuebut the field name implies a traffic count. Mock data sets it to 2587 (a raw count), contradicting the implementation. The field is not currently rendered anywhere in the codebase, but rename it tolocalhostRateto match semantics, and update the mock value to a percentage in [0, 100].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries.ts` around lines 439 - 451, Rename the quality field localhostTraffic to localhostRate and ensure its value represents a percentage (0–100); update the object built in queries.ts (the quality: { ... } block) to use quality.localhostRate = Number(localhostRateKPI.value) (or clamp/validate it to the 0–100 range) and adjust the mock data to provide a percentage rather than a raw count (e.g., a value within 0–100); also search for and update any references to localhostTraffic to use localhostRate (look for the quality object and anywhere localhostTraffic is consumed).apps/ingestion/src/app.ts (1)
208-214:⚠️ Potential issue | 🟡 MinorDocument the new
/ealias on the index page.The SDK now defaults to
POST /e; the API overview should list it alongside/ingestso the landing page reflects supported routes.📝 Suggested addition
+ <div class="row"> + <div class="left"> + <div class="method post">POST</div> + <div class="path">/e</div> + </div> + <div class="desc">Ingest analytics events (short alias)</div> + </div> + <div class="row"> <div class="left"> <div class="method post">POST</div> <div class="path">/ingest</div> </div> <div class="desc">Ingest analytics events</div> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/src/app.ts` around lines 208 - 214, Add a new API route row to the index page beside the existing "/ingest" entry: duplicate the existing div.row block that contains the method/post and path "/ingest" and change the path text to "/e" and the desc text to something like "Alias for /ingest (SDK default)". Ensure the method element remains "POST" (class "method post") so the page shows both supported routes and indicates that the SDK now defaults to POST /e.apps/example-dashboard/components/geo-map.tsx (1)
334-348:⚠️ Potential issue | 🟡 MinorMirror
fillOpacityinto thehoverstate to prevent opacity flash on low-traffic countries.
react-simple-mapsreplaces the entire style object per state rather than merging them. SincefillOpacityonly exists indefault, hovering a low-traffic country reverts to full opacity, flattening the data-driven shading visually. Add the samefillOpacitylogic tohover.Proposed fix
style={{ default: { outline: "none", cursor: hasData ? "pointer" : "default", fillOpacity: hasData ? 0.2 + (countryData.percentage / maxPercentage) * 0.8 : 1, }, hover: { fill: hasData ? "hsl(var(--primary))" : "hsl(var(--muted-foreground) / 0.2)", outline: "none", cursor: hasData ? "pointer" : "default", + fillOpacity: hasData + ? 0.2 + (countryData.percentage / maxPercentage) * 0.8 + : 1, }, pressed: { outline: "none" }, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/geo-map.tsx` around lines 334 - 348, The hover style currently omits fillOpacity so react-simple-maps replaces the style and resets low-traffic countries to full opacity; update the hover style in the same style object used for the map feature (the block referencing default and hover) to include the same computed fillOpacity expression (use hasData ? 0.2 + (countryData.percentage / maxPercentage) * 0.8 : 1) so hover preserves the data-driven opacity; locate where hasData, countryData.percentage and maxPercentage are used in the style and add the fillOpacity key to the hover branch.packages/sdk/src/api/track.ts (2)
41-58:⚠️ Potential issue | 🟠 Major
DEFAULT_INGEST_URLis not normalized; trailing slash in env yields a//eendpoint.The new option-provided
ingestUrlis run throughnormalizeIngestUrl(line 136), but the env-derived value returned here is not. IfNEXT_PUBLIC_ANALYTICS_URL=https://api.example.com/, then${baseUrl}/eresolves tohttps://api.example.com//e, which most ingestion routers will treat as a different/non-matching path or 404.🛡️ Normalize before returning
- if (typeof window !== "undefined" && !validateIngestUrl(url)) { - console.error(`[Analytics] Invalid ingestUrl: "${url}". Must be a valid http/https URL.`); - return ""; - } - - return url; + if (typeof window !== "undefined" && !validateIngestUrl(url)) { + console.error(`[Analytics] Invalid ingestUrl: "${url}". Must be a valid http/https URL.`); + return ""; + } + + return normalizeIngestUrl(url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/api/track.ts` around lines 41 - 58, resolveDefaultIngestUrl returns the env URL without normalizing, causing double slashes when concatenated; update resolveDefaultIngestUrl to run the chosen env value through normalizeIngestUrl (the same normalizer used for option-provided ingestUrl) after validateIngestUrl and before returning so callers get a normalized base URL (refer to resolveDefaultIngestUrl, validateIngestUrl, and normalizeIngestUrl).
1-1:⚠️ Potential issue | 🔴 CriticalCI is failing on
oxfmt --checkfor this file.The pipeline log shows oxfmt reported format issues. Run
bun run fmt(oroxfmt apps packages '!**/dist/**') to fix before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/api/track.ts` at line 1, CI fails because this file is not formatted with oxfmt; run the project's formatter and commit the result: run "bun run fmt" (or run "oxfmt apps packages '!**/dist/**'") to reformat the file that contains the import getVisitorId in the track module, then stage and commit the changed file so the CI oxfmt check passes.
🧹 Nitpick comments (8)
apps/example-dashboard/lib/queries.ts (1)
29-39: Keep this regex in sync with the ingestion handler's preview detector.
VERCEL_PREVIEW_PATTERNhere must stay byte-for-byte equivalent to the regex inapps/ingestion/src/handlers/ingest.ts(isVercelPreviewHost). Right now they match, but they live in two places — any drift will cause inconsistent classification (events tagged but not filtered, or vice versa). Consider exporting the pattern from a shared module, or at minimum add a unit test asserting parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries.ts` around lines 29 - 39, VERCEL_PREVIEW_PATTERN is duplicated here and must remain byte-for-byte identical to the regex used by isVercelPreviewHost in the ingestion handler; update the code so the pattern is defined once and reused (or add a parity unit test). Specifically, extract the pattern into a shared constant and import it into this file, replacing the local VERCEL_PREVIEW_PATTERN used by publicTraffic and publicTrafficEvents, or alternatively add a unit test that compares this file's VERCEL_PREVIEW_PATTERN with the ingestion handler's pattern to fail on any drift.apps/ingestion/tests/unit/ingest.test.ts (2)
28-29: Add coverage for the new/ealias.The alias route is the SDK's default and has no unit test. A single assertion that
POST /ebehaves identically to/ingestwould lock the contract and guard against accidental removal ofapp.post("/e", handleIngest).🛠 Suggested addition
const app = new Hono(); app.post("/ingest", handleIngest); +app.post("/e", handleIngest); + +// ... and add a smoke test: +// test("alias /e accepts pageview", async () => { +// const r = await app.request("/e", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ projectId: "x", type: "pageview", path: "/" }) }); +// expect(r.status).toBe(200); +// });Based on learnings: "Run E2E and load tests when changes affect ingestion flow, SDK tracking lifecycle, or dashboard critical paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/tests/unit/ingest.test.ts` around lines 28 - 29, Add a unit test in ingest.test.ts that mirrors the existing POST "/ingest" assertion but targets the alias path POST "/e": instantiate the Hono app that registers both routes (or ensure app.post("/e", handleIngest) is present), send an equivalent request to "/e" and assert the response status and body (and any side-effects or mocks tied to handleIngest) match the "/ingest" test so the contract is locked; reference the Hono app instance and handleIngest handler to locate where to add the new test.
4-15: MockinsertedEventsis shared global state; consider per-test isolation.The array is module-scoped and only reset inside the two preview tests. Future tests that don't reset will inherit prior inserts and risk cross-test bleed. A
beforeEach(() => { insertedEvents.length = 0; })keeps assertions hermetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/tests/unit/ingest.test.ts` around lines 4 - 15, The test suite uses a module-scoped array insertedEvents that accumulates state across tests; add test-level isolation by resetting it before each test (e.g., in a beforeEach) so assertions stay hermetic—locate the insertedEvents declaration and the mock dbModule/db.insert/... mock and add a beforeEach hook that sets insertedEvents.length = 0 to clear prior inserts before every test.packages/sdk/tsup.config.ts (1)
15-15: Couples SDK build to Bun unnecessarily.The post-build script only uses Node built-ins (
node:fs,node:path,import.meta.dirname). Usingbun runhere forces every contributor / CI runner that builds the SDK package to have Bun installed. Prefer plainnode(>= 20.11) so the SDK build remains runtime-agnostic.🛠 Proposed fix
- onSuccess: "bun run scripts/client-directive.ts", + onSuccess: "node scripts/client-directive.ts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/tsup.config.ts` at line 15, The onSuccess hook currently invokes Bun ("onSuccess: \"bun run scripts/client-directive.ts\"") which unnecessarily requires Bun; change it to use Node instead by updating the onSuccess command to "node scripts/client-directive.ts" (ensure CI/doc notes Node >= 20.11 if needed for import.meta.dirname and node: built-ins). Locate the onSuccess property in tsup.config.ts and replace the "bun run" invocation with "node" so the post-build script remains runtime-agnostic.apps/ingestion/scripts/ops.ts (1)
562-601:smokeposts to/ingest, not the new/ealias.Both routes are mapped to the same handler (
apps/ingestion/src/app.tsline 304-305), so this still works, but the whole point of the PR is to make/ethe canonical SDK ingestion path. Pointing the verification command at/eexercises the alias every time you run the smoke check, which is the same path real beacons take.♻️ Smoke the canonical alias
- const response = await fetch(`${config.ingestUrl}/ingest`, { + const response = await fetch(`${config.ingestUrl}/e`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/scripts/ops.ts` around lines 562 - 601, The smoke function currently posts to the /ingest endpoint; update it to use the canonical SDK alias by changing the fetch target from `${config.ingestUrl}/ingest` to `${config.ingestUrl}/e` so the verification exercise hits the alias every run; locate the fetch call inside the smoke(config, overrides) function and replace the path while keeping headers/payload unchanged (ensure any tests or comments referencing `/ingest` are updated if needed).apps/example-dashboard/components/dashboard-content.tsx (2)
118-120: Redundant double cast.The expression already coerces with
as SignalEvent["type"] | null; the trailingas | SignalEvent["type"] | "all"is a no-op and obscures intent.♻️ Simplify the cast
- const typeFilter = ((searchParams.get("status") as SignalEvent["type"] | null) || "all") as - | SignalEvent["type"] - | "all"; + const typeFilter = (searchParams.get("status") ?? "all") as SignalEvent["type"] | "all";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/dashboard-content.tsx` around lines 118 - 120, The code uses a redundant double cast when defining typeFilter; simplify by removing the trailing "as SignalEvent['type'] | 'all'" and use the existing cast plus nullish coalescing to default to "all". Specifically, update the expression that sets typeFilter (the variable named typeFilter that reads searchParams.get("status")) to something like using (searchParams.get("status") as SignalEvent["type"] | null) ?? "all" so the value is typed correctly without the no-op second cast.
128-156: Hardcoded/path in URL handlers; logic is duplicated inapp-sidebar.tsx.
router.push(/?${...})will navigate to the root, regardless of the current pathname. IfDashboardContentis ever mounted under a different segment, switching project/time/type will silently redirect users home. The same three handlers (setSelectedProject,setTimeRangewith the30ddeletion rule, and theviewHrefstyle logic) are also defined inapps/example-dashboard/components/app-sidebar.tsx.♻️ Drop the hardcoded `/` and extract a shared helper
+import { usePathname } from "next/navigation"; @@ const router = useRouter(); + const pathname = usePathname(); const searchParams = useSearchParams(); @@ - const setSelectedProject = (projectId: string | null) => { - const newParams = new URLSearchParams(searchParams.toString()); - if (projectId) { - newParams.set("projectId", projectId); - } else { - newParams.delete("projectId"); - } - router.push(`/?${newParams.toString()}`); - }; + const setSelectedProject = (projectId: string | null) => { + updateQuery("projectId", projectId); + };Then introduce a small
useQueryParamhook (or similar utility) shared between the sidebar and dashboard content to avoid the three near-identical setter implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/dashboard-content.tsx` around lines 128 - 156, The three handlers setSelectedProject, setTimeRange and setTypeFilter hardcode the root path when pushing query changes; replace router.push(`/?${...}`) with a push that preserves the current pathname (e.g., router.push(`${router.pathname}?${newParams.toString()}`) or use usePathname() from next/navigation) and then extract these three near-identical behaviors into a shared helper or small useQueryParam hook used by both DashboardContent and app-sidebar (hook should accept the searchParams, key/value logic and the special deletion rules: timeRange "30d" => delete key, type "all" => delete status). Ensure the helper exposes setters (or a setQueryParam function) and update setSelectedProject, setTimeRange, and setTypeFilter to call it instead of duplicating logic.apps/example-dashboard/components/app-sidebar.tsx (1)
271-274:projects.find(...)?.id || selectedProjectis a no-op lookup.
ProjectOptiononly carriesidandeventCount, so whenselectedProjectis set,projects.find((project) => project.id === selectedProject)?.idresolves to eitherselectedProject(match) orundefined(no match), making the OR fallback always returnselectedProject. Either drop the lookup, or surface a real human-readable name onProjectOption(e.g.,namefrom the API) and use it for the display value.♻️ Either simplify or wire a real name field
- const displayName = selectedProject - ? projects.find((project) => project.id === selectedProject)?.id || selectedProject - : "All Projects"; + const displayName = selectedProject ?? "All Projects";If you intend a friendlier label, extend
ProjectOptionwith anamefield from/api/analytics?metric=projectsand useproject.name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/app-sidebar.tsx` around lines 271 - 274, The displayName computation in ProjectSwitcher is doing a no-op lookup: projects.find((project) => project.id === selectedProject)?.id || selectedProject always returns selectedProject; either simplify by removing the find and just use selectedProject (or "All Projects" when falsy) in ProjectSwitcher, or update the ProjectOption type and the API to include a human-readable name (e.g., add a name field from /api/analytics?metric=projects) and change the lookup to projects.find(p => p.id === selectedProject)?.name || selectedProject so the UI shows a friendly project.name; locate ProjectSwitcher and ProjectOption to implement the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/example-dashboard/app/api/analytics/route.ts`:
- Around line 7-16: Validate the timeRange query param against an explicit
whitelist before computing hours: check the value returned by
searchParams.get("timeRange") (used as the timeRange variable) and if it's not
one of "all","30d","60d","90d","180d" return a 400 response; only after
validation compute hours (the hours variable) and derive from/to as currently
done. Update the route handler that reads searchParams (where timeRange, hours,
from, to are defined) to perform this validation and short-circuit with a clear
400 error for unknown values.
In `@apps/example-dashboard/app/layout.tsx`:
- Line 33: The fallback for analyticsUrl includes a trailing slash causing
double slashes when the SDK appends "/e"; update the analyticsUrl constant
(analyticsUrl in app/layout.tsx) to trim any trailing slashes from the
environment fallback so the SDK receives a clean base URL (e.g., derive
analyticsUrl from process.env.NEXT_PUBLIC_ANALYTICS_URL or the fallback and call
a rtrim like replace(/\/+$/,'') so `${analyticsUrl}/e` will produce a single
slash).
In `@apps/example-dashboard/components/signal-stream.tsx`:
- Around line 161-184: The UI uses truthy checks that hide valid numeric zeros:
change the conditional render checks for statusCode and duration in the
signal-stream component so they test for absence explicitly (e.g., statusCode
!== null/undefined or typeof statusCode === "number") instead of using
`{statusCode && ...}` / `{duration && ...}`; update the JSX around the
statusCode and duration spans (the blocks referencing statusCode, duration and
the numberMeta source) to render when the value is a number (or !== null) so `0`
displays correctly while preserving current styling and className logic.
- Around line 70-77: The hasDetails check is wrong because metadata always
contains deviceType; change it to reflect only the fields actually rendered in
the expanded panel by testing those normalized values (endpoint, method,
statusCode, duration, region, requestId, userAgent) instead of
Object.keys(metadata).length > 0; update the hasDetails computation so it
returns true if any of endpoint || method || statusCode || duration || region ||
requestId || userAgent is present (truthy/non-null) so rows with only deviceType
won’t show the empty expanded panel.
In `@apps/example-dashboard/tsconfig.json`:
- Line 16: The tsconfig.json "types" array includes "bun-types", which is
unnecessary for this Next.js app and not installed; remove "bun-types" from the
"types" array in tsconfig.json (the entry "types": ["bun-types", "node"]) so
only needed typings (e.g., "node") remain, and if Bun APIs are ever required
later replace with the official "@types/bun" package and add it to package.json
instead.
In `@apps/ingestion/scripts/ops.ts`:
- Around line 455-458: getRecent currently assumes the recent query is at
presets[0], which is brittle; change getRecent (and similarly the code in
runCmd("query")) to locate the correct preset by its unique id (e.g., use
presets.find(p => p.id === "<recent-preset-id>")) instead of indexing into
presets[0], and throw/log a clear error if the preset isn't found; update the
call sites (getRecent and runCmd("query")) to use the found preset's .sql and
parameters so reordering the presets won't change behavior.
In `@apps/ingestion/src/db/client.ts`:
- Around line 38-46: The getDbClient function silently falls back to
createFallbackDb when process.env.DATABASE_URL is missing; update getDbClient to
log an error (using the project's existing logger, e.g., processLogger if
available, otherwise console.error) before returning createFallbackDb so missing
DATABASE_URL is visible in logs — reference getDbClient, DATABASE_URL,
createFallbackDb and createDb when making the change.
- Around line 12-36: createFallbackDb returns plain arrays and non-thenable
objects which breaks real drizzle usage (e.g., db.select().from(...).where(...)
and await db.insert(...).values(...)); update createFallbackDb to either
throw/log and fail fast when DATABASE_URL is missing, or implement a chainable
thenable Proxy-based stub that returns a builder for any method chain and whose
awaited value resolves to { rows: [] } or an empty array as appropriate; ensure
the returned stub honors thenable semantics for insert(...).values(...) and
chainable methods for select().from(...) and also add explicit logging when
falling back so misconfiguration isn’t silent.
In `@apps/ingestion/tests/unit/ingest.test.ts`:
- Around line 103-143: The handler currently trusts payload.host to set
meta.isPreview which lets clients spoof preview status; update the ingest
handler so isVercelPreviewHost is called only on the trusted header-derived host
(use getHostFromOrigin(origin) or the request Host header) and remove any use of
payload.host when computing meta.isPreview (references: isVercelPreviewHost,
getHostFromOrigin, payload.host, meta.isPreview). After fixing the logic, add
unit tests covering the new /e endpoint and assert that preview detection is
driven solely by headers (send Origin/Host headers) and that client-sent
payload.host cannot change meta.isPreview.
In `@apps/ingestion/tests/unit/ops.test.ts`:
- Around line 41-57: The test assumes loadConfig(parseArgs(...)) will fall back
to "http://localhost:3000" but it doesn't unset environment vars used by
loadConfig (NEXT_PUBLIC_ANALYTICS_URL and INGEST_URL), so CI/dev shells can make
the assertion flaky; update the test to save and restore
process.env.NEXT_PUBLIC_ANALYTICS_URL and process.env.INGEST_URL around the call
to loadConfig (similar to how DATABASE_URL and ADMIN_SECRET are handled) so
ingestUrl is forced to the localhost fallback, leaving loadConfig, parseArgs and
the existing assertions unchanged.
In `@packages/sdk/src/api/track.ts`:
- Line 148: Tests in packages/sdk/__tests__/track.test.ts still assert the old
"/ingest" path while the SDK now sets endpoint = `${baseUrl}/e` in
packages/sdk/src/api/track.ts; update the tests to assert "/e" (or derive the
expected path from the same baseUrl/endpoint logic) wherever they check for
"/ingest"—search for assertions in track.test.ts referencing "/ingest" and
replace them to match the new endpoint symbol (endpoint / the track request
assertions) so test expectations align with the updated track.ts behavior.
---
Outside diff comments:
In `@apps/example-dashboard/components/geo-map.tsx`:
- Around line 334-348: The hover style currently omits fillOpacity so
react-simple-maps replaces the style and resets low-traffic countries to full
opacity; update the hover style in the same style object used for the map
feature (the block referencing default and hover) to include the same computed
fillOpacity expression (use hasData ? 0.2 + (countryData.percentage /
maxPercentage) * 0.8 : 1) so hover preserves the data-driven opacity; locate
where hasData, countryData.percentage and maxPercentage are used in the style
and add the fillOpacity key to the hover branch.
In `@apps/example-dashboard/lib/queries.ts`:
- Around line 833-850: The returned visitor mapping currently hardcodes
isInternal:false and the SELECT that populates results omits is_internal; fix by
selecting is_internal in the query that produces results and map it through in
the results.map (e.g., set isInternal based on r.is_internal, converting to
boolean as needed) and also add the isInternal field to the visitor type in
visitors-table.tsx so types align; alternatively, if you intend to drop this
signal, remove the isInternal property from the map and the type instead—update
the SELECT, the mapping in the results.map block, and the visitor type in
visitors-table.tsx accordingly.
- Around line 439-451: Rename the quality field localhostTraffic to
localhostRate and ensure its value represents a percentage (0–100); update the
object built in queries.ts (the quality: { ... } block) to use
quality.localhostRate = Number(localhostRateKPI.value) (or clamp/validate it to
the 0–100 range) and adjust the mock data to provide a percentage rather than a
raw count (e.g., a value within 0–100); also search for and update any
references to localhostTraffic to use localhostRate (look for the quality object
and anywhere localhostTraffic is consumed).
In `@apps/ingestion/src/app.ts`:
- Around line 208-214: Add a new API route row to the index page beside the
existing "/ingest" entry: duplicate the existing div.row block that contains the
method/post and path "/ingest" and change the path text to "/e" and the desc
text to something like "Alias for /ingest (SDK default)". Ensure the method
element remains "POST" (class "method post") so the page shows both supported
routes and indicates that the SDK now defaults to POST /e.
In `@packages/sdk/src/api/track.ts`:
- Around line 41-58: resolveDefaultIngestUrl returns the env URL without
normalizing, causing double slashes when concatenated; update
resolveDefaultIngestUrl to run the chosen env value through normalizeIngestUrl
(the same normalizer used for option-provided ingestUrl) after validateIngestUrl
and before returning so callers get a normalized base URL (refer to
resolveDefaultIngestUrl, validateIngestUrl, and normalizeIngestUrl).
- Line 1: CI fails because this file is not formatted with oxfmt; run the
project's formatter and commit the result: run "bun run fmt" (or run "oxfmt apps
packages '!**/dist/**'") to reformat the file that contains the import
getVisitorId in the track module, then stage and commit the changed file so the
CI oxfmt check passes.
---
Nitpick comments:
In `@apps/example-dashboard/components/app-sidebar.tsx`:
- Around line 271-274: The displayName computation in ProjectSwitcher is doing a
no-op lookup: projects.find((project) => project.id === selectedProject)?.id ||
selectedProject always returns selectedProject; either simplify by removing the
find and just use selectedProject (or "All Projects" when falsy) in
ProjectSwitcher, or update the ProjectOption type and the API to include a
human-readable name (e.g., add a name field from /api/analytics?metric=projects)
and change the lookup to projects.find(p => p.id === selectedProject)?.name ||
selectedProject so the UI shows a friendly project.name; locate ProjectSwitcher
and ProjectOption to implement the chosen approach.
In `@apps/example-dashboard/components/dashboard-content.tsx`:
- Around line 118-120: The code uses a redundant double cast when defining
typeFilter; simplify by removing the trailing "as SignalEvent['type'] | 'all'"
and use the existing cast plus nullish coalescing to default to "all".
Specifically, update the expression that sets typeFilter (the variable named
typeFilter that reads searchParams.get("status")) to something like using
(searchParams.get("status") as SignalEvent["type"] | null) ?? "all" so the value
is typed correctly without the no-op second cast.
- Around line 128-156: The three handlers setSelectedProject, setTimeRange and
setTypeFilter hardcode the root path when pushing query changes; replace
router.push(`/?${...}`) with a push that preserves the current pathname (e.g.,
router.push(`${router.pathname}?${newParams.toString()}`) or use usePathname()
from next/navigation) and then extract these three near-identical behaviors into
a shared helper or small useQueryParam hook used by both DashboardContent and
app-sidebar (hook should accept the searchParams, key/value logic and the
special deletion rules: timeRange "30d" => delete key, type "all" => delete
status). Ensure the helper exposes setters (or a setQueryParam function) and
update setSelectedProject, setTimeRange, and setTypeFilter to call it instead of
duplicating logic.
In `@apps/example-dashboard/lib/queries.ts`:
- Around line 29-39: VERCEL_PREVIEW_PATTERN is duplicated here and must remain
byte-for-byte identical to the regex used by isVercelPreviewHost in the
ingestion handler; update the code so the pattern is defined once and reused (or
add a parity unit test). Specifically, extract the pattern into a shared
constant and import it into this file, replacing the local
VERCEL_PREVIEW_PATTERN used by publicTraffic and publicTrafficEvents, or
alternatively add a unit test that compares this file's VERCEL_PREVIEW_PATTERN
with the ingestion handler's pattern to fail on any drift.
In `@apps/ingestion/scripts/ops.ts`:
- Around line 562-601: The smoke function currently posts to the /ingest
endpoint; update it to use the canonical SDK alias by changing the fetch target
from `${config.ingestUrl}/ingest` to `${config.ingestUrl}/e` so the verification
exercise hits the alias every run; locate the fetch call inside the
smoke(config, overrides) function and replace the path while keeping
headers/payload unchanged (ensure any tests or comments referencing `/ingest`
are updated if needed).
In `@apps/ingestion/tests/unit/ingest.test.ts`:
- Around line 28-29: Add a unit test in ingest.test.ts that mirrors the existing
POST "/ingest" assertion but targets the alias path POST "/e": instantiate the
Hono app that registers both routes (or ensure app.post("/e", handleIngest) is
present), send an equivalent request to "/e" and assert the response status and
body (and any side-effects or mocks tied to handleIngest) match the "/ingest"
test so the contract is locked; reference the Hono app instance and handleIngest
handler to locate where to add the new test.
- Around line 4-15: The test suite uses a module-scoped array insertedEvents
that accumulates state across tests; add test-level isolation by resetting it
before each test (e.g., in a beforeEach) so assertions stay hermetic—locate the
insertedEvents declaration and the mock dbModule/db.insert/... mock and add a
beforeEach hook that sets insertedEvents.length = 0 to clear prior inserts
before every test.
In `@packages/sdk/tsup.config.ts`:
- Line 15: The onSuccess hook currently invokes Bun ("onSuccess: \"bun run
scripts/client-directive.ts\"") which unnecessarily requires Bun; change it to
use Node instead by updating the onSuccess command to "node
scripts/client-directive.ts" (ensure CI/doc notes Node >= 20.11 if needed for
import.meta.dirname and node: built-ins). Locate the onSuccess property in
tsup.config.ts and replace the "bun run" invocation with "node" so the
post-build script remains runtime-agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c9b9672-d120-4578-a7dc-9a30f1864c3c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
apps/example-dashboard/app/api/analytics/route.tsapps/example-dashboard/app/layout.tsxapps/example-dashboard/components/app-sidebar.tsxapps/example-dashboard/components/command-palette.tsxapps/example-dashboard/components/dashboard-content.tsxapps/example-dashboard/components/dashboard-header.tsxapps/example-dashboard/components/geo-details.tsxapps/example-dashboard/components/geo-map.tsxapps/example-dashboard/components/signal-stream.tsxapps/example-dashboard/components/world-map.tsxapps/example-dashboard/lib/queries.tsapps/example-dashboard/package.jsonapps/example-dashboard/tests/integration/queries.test.tsapps/example-dashboard/tsconfig.jsonapps/ingestion/package.jsonapps/ingestion/scripts/ops.tsapps/ingestion/src/app.tsapps/ingestion/src/db/client.tsapps/ingestion/src/db/index.tsapps/ingestion/src/db/migrations/0001_archive_legacy_tables.sqlapps/ingestion/src/db/schema.tsapps/ingestion/src/db/seed.tsapps/ingestion/src/handlers/ingest.tsapps/ingestion/tests/unit/ingest.test.tsapps/ingestion/tests/unit/ops.test.tsapps/ingestion/tests/unit/schema.test.tspackage.jsonpackages/sdk/scripts/client-directive.tspackages/sdk/src/api/track.tspackages/sdk/src/components/analytics.tsxpackages/sdk/src/index.tspackages/sdk/src/observers/performance.tspackages/sdk/src/types/index.tspackages/sdk/src/utilities/enrichment.tspackages/sdk/tsconfig.jsonpackages/sdk/tsup.config.ts
💤 Files with no reviewable changes (2)
- packages/sdk/src/index.ts
- apps/ingestion/src/db/schema.ts
Comment on lines
+103
to
+143
| test("marks feature branch vercel deployments as preview", async () => { | ||
| insertedEvents.length = 0; | ||
|
|
||
| const response = await app.request("/ingest", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| projectId: "example.com", | ||
| type: "pageview", | ||
| path: "/preview-vercel", | ||
| host: "analytics-git-feature-remco.vercel.app", | ||
| }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| const event = insertedEvents.at(-1); | ||
| expect(event).toBeDefined(); | ||
| if (!event) throw new Error("event missing"); | ||
| expect((event.meta as Record<string, unknown>).isPreview).toBe(true); | ||
| }); | ||
|
|
||
| test("keeps production vercel deployments as public traffic", async () => { | ||
| insertedEvents.length = 0; | ||
|
|
||
| const response = await app.request("/ingest", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| projectId: "example.com", | ||
| type: "pageview", | ||
| path: "/production-vercel", | ||
| host: "analytics.vercel.app", | ||
| }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| const event = insertedEvents.at(-1); | ||
| expect(event).toBeDefined(); | ||
| if (!event) throw new Error("event missing"); | ||
| expect((event.meta as Record<string, unknown>).isPreview).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'ingest.ts' apps/ingestion/src/handlers --exec rg -nP -C3 '\b(host|isVercelPreviewHost|payload\.host|c\.req\.header\(.host.\))' {}Repository: remcostoeten/analytics
Length of output: 1048
🏁 Script executed:
#!/bin/bash
# Find where origin variable is set in ingest.ts
fd 'ingest.ts' apps/ingestion/src/handlers --exec rg -n 'const origin|origin =' {} \
| head -30
echo "---"
# Search for /e route tests
fd '\.test\.ts' apps/ingestion --exec rg -l '"/e"' {}
echo "---"
# Check if origin comes from request headers
fd 'ingest.ts' apps/ingestion/src/handlers --exec rg -nB5 'const origin' {}Repository: remcostoeten/analytics
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Check if /e endpoint is tested anywhere
fd '\.test\.ts' apps/ingestion --exec rg '/e|"/e' {}Repository: remcostoeten/analytics
Length of output: 546
🏁 Script executed:
#!/bin/bash
# Check if /e route exists in app code
fd 'app.ts' apps/ingestion/src --exec rg -n '"/e"' {}
echo "---"
# Check ingest.test.ts for /e references
fd 'ingest.test.ts' --exec rg -n '"/e"' {}Repository: remcostoeten/analytics
Length of output: 104
Preview detection uses client-supplied payload.host instead of request headers.
These tests put host in the JSON body, but the ingest handler at line 179 evaluates isVercelPreviewHost(payload.host) || isVercelPreviewHost(getHostFromOrigin(origin)). Since the tests don't send an origin header, the second condition is false, leaving preview detection entirely dependent on the client-supplied payload.host field. An attacker can falsely mark their traffic as preview (or production) by manipulating the request body, undermining the preview-vs-production split. The handler should trust only the request headers; payload.host should not influence meta.isPreview.
Additionally, the new /e endpoint (line 304 in app.ts) has zero unit-test coverage and should be tested to lock in its contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ingestion/tests/unit/ingest.test.ts` around lines 103 - 143, The handler
currently trusts payload.host to set meta.isPreview which lets clients spoof
preview status; update the ingest handler so isVercelPreviewHost is called only
on the trusted header-derived host (use getHostFromOrigin(origin) or the request
Host header) and remove any use of payload.host when computing meta.isPreview
(references: isVercelPreviewHost, getHostFromOrigin, payload.host,
meta.isPreview). After fixing the logic, add unit tests covering the new /e
endpoint and assert that preview detection is driven solely by headers (send
Origin/Host headers) and that client-sent payload.host cannot change
meta.isPreview.
Comment on lines
+41
to
+57
| test("reports missing env state", () => { | ||
| const beforeDatabase = process.env.DATABASE_URL; | ||
| const beforeAdmin = process.env.ADMIN_SECRET; | ||
| delete process.env.DATABASE_URL; | ||
| delete process.env.ADMIN_SECRET; | ||
|
|
||
| const config = loadConfig(parseArgs(["--target", "local"]), "/tmp/no-analytics-env"); | ||
|
|
||
| if (beforeDatabase === undefined) delete process.env.DATABASE_URL; | ||
| else process.env.DATABASE_URL = beforeDatabase; | ||
| if (beforeAdmin === undefined) delete process.env.ADMIN_SECRET; | ||
| else process.env.ADMIN_SECRET = beforeAdmin; | ||
|
|
||
| expect(config.ingestUrl).toBe("http://localhost:3000"); | ||
| expect(config.missing).toContain("DATABASE_URL"); | ||
| expect(config.missing).toContain("ADMIN_SECRET"); | ||
| }); |
There was a problem hiding this comment.
Test depends on host env not setting NEXT_PUBLIC_ANALYTICS_URL / INGEST_URL.
loadConfig resolves ingestUrl from process.env.NEXT_PUBLIC_ANALYTICS_URL || ... || process.env.INGEST_URL || ... before falling back to http://localhost:3000 (apps/ingestion/scripts/ops.ts line 316-322). The test only deletes DATABASE_URL and ADMIN_SECRET, so when CI or a dev shell exports either ingestion variable the expect(config.ingestUrl).toBe("http://localhost:3000") assertion will fail.
🛡️ Make the test self-contained
- const beforeDatabase = process.env.DATABASE_URL;
- const beforeAdmin = process.env.ADMIN_SECRET;
- delete process.env.DATABASE_URL;
- delete process.env.ADMIN_SECRET;
+ const snapshot = {
+ DATABASE_URL: process.env.DATABASE_URL,
+ ADMIN_SECRET: process.env.ADMIN_SECRET,
+ NEXT_PUBLIC_ANALYTICS_URL: process.env.NEXT_PUBLIC_ANALYTICS_URL,
+ INGEST_URL: process.env.INGEST_URL,
+ };
+ delete process.env.DATABASE_URL;
+ delete process.env.ADMIN_SECRET;
+ delete process.env.NEXT_PUBLIC_ANALYTICS_URL;
+ delete process.env.INGEST_URL;
const config = loadConfig(parseArgs(["--target", "local"]), "/tmp/no-analytics-env");
- if (beforeDatabase === undefined) delete process.env.DATABASE_URL;
- else process.env.DATABASE_URL = beforeDatabase;
- if (beforeAdmin === undefined) delete process.env.ADMIN_SECRET;
- else process.env.ADMIN_SECRET = beforeAdmin;
+ for (const [key, value] of Object.entries(snapshot)) {
+ if (value === undefined) delete process.env[key];
+ else process.env[key] = value;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ingestion/tests/unit/ops.test.ts` around lines 41 - 57, The test assumes
loadConfig(parseArgs(...)) will fall back to "http://localhost:3000" but it
doesn't unset environment vars used by loadConfig (NEXT_PUBLIC_ANALYTICS_URL and
INGEST_URL), so CI/dev shells can make the assertion flaky; update the test to
save and restore process.env.NEXT_PUBLIC_ANALYTICS_URL and
process.env.INGEST_URL around the call to loadConfig (similar to how
DATABASE_URL and ADMIN_SECRET are handled) so ingestUrl is forced to the
localhost fallback, leaving loadConfig, parseArgs and the existing assertions
unchanged.
…it queries Phase 1 — quick wins: - Unify preview detection: single `isPreviewEnvironment` in geo.ts (Vercel-specific regex + staging/preview patterns), remove duplicate `isVercelPreviewHost` from ingest.ts - Add `getHostFromOrigin` to geo.ts as canonical export - Cache ORIGIN_ALLOWLIST with env-invalidation (lazy cache, not stale module-level const) - Export `requireAdminAuth` from admin.ts; apply it to GET /metrics (was unauthenticated) - Add /e alias to HTML landing route listing - Wire per-event-type dedupe TTL via `getDedupeWindow` into `dedupeCache.add()` Phase 2 — schema: - Add `is_preview`, `bot_detected`, `is_internal` boolean columns to events table - Add indexes on is_preview, bot_detected - Migration 0002 backfills from existing JSONB meta - Ingest handler writes flags to first-class columns instead of meta - Bot detection queries use `bot_detected = true OR meta fallback` for backwards compat Phase 3 — split queries.ts (851 lines) into domain modules: - lib/queries/filters.ts — shared filter fns, PREVIEW_PATTERN, range helpers - lib/queries/kpis.ts — KPI query functions - lib/queries/content.ts — pages, referrers, geo, paths - lib/queries/audience.ts — devices, browsers, OS, languages, screens - lib/queries/sessions.ts — session stats, engagement, heatmap, retention, web vitals, UTM - lib/queries/realtime.ts — live now, recent events, recent visitors - lib/queries/overview.ts — trends, getDashboardData, projects, segments - lib/queries/index.ts — barrel re-export (all imports unchanged) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- layout.tsx: remove trailing slash from fallback ingest URL (was producing //e double-slash → 404 on Hono) - track.ts: resolve ingest URL lazily per call instead of at module load (fixes test env isolation + allows runtime env changes) - track.test.ts: update /ingest assertions to /e, set NEXT_PUBLIC_ANALYTICS_URL in beforeEach - signal-stream.tsx: fix hasDetails to check actual renderable fields instead of metadata key count (was always true); use !== null for numeric statusCode/duration checks (was suppressing 0 values) - route.ts: validate timeRange against explicit whitelist, return 400 for unknown values - client.ts: log error when DATABASE_URL missing instead of silently returning stub - tsconfig.json: remove bun-types from Next.js app types (not a Bun runtime app); add tests/tsconfig.json that extends root and adds bun-types for test files only Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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
/eas short-form alias for/ingestendpoint; SDK switches to/eby default (smaller beacon payload)isPreviewflag for filteringlocalhost:3001— unconfigured URL drops the event and logs an error, preventing phantom local data in prod dashboardsTest plan
bun testpasses inapps/ingestionandapps/example-dashboard/ein network tab (not/ingest)*.vercel.apppreview URLs haveisPreview: truein DBNEXT_PUBLIC_ANALYTICS_URLlogs error and drops event (no fetch)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements