feat(dashboard): neutralize colored icons, chart theming, DB migrations#13
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>
…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>
…migrations - Replace amber/sky notice banners with neutral muted tokens - Geo map hover + legend gradient use CSS vars instead of hardcoded blue - Add drizzle migrations for events/visitors schema with indexes - Add sync-vercel-env script 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
📝 WalkthroughWalkthroughThis pull request introduces a new analytics data layer with database schema and query modules, extends the analytics API to support new event-based metrics, refactors dashboard components to use URL-based state management, and adds styling updates for tooltips and data visualization across multiple dashboard components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/example-dashboard/components/world-map.tsx (1)
99-119:⚠️ Potential issue | 🟡 MinorHardcoded blue HSL still present despite PR objectives.
The PR description states "Geo map hover fill and legend gradient now use CSS variables instead of a hardcoded HSL blue", but
getCountryColorstill returns hardcodedhsl(210, 80%, …)andhsl(210, 70%, …)blue values for hovered/default fills. The legend at line 200 also still useshsl(210, 70%, ${l}%). Either the neutralization was missed for this file or the description overstates the scope.🎨 Suggested neutralization using muted/foreground tokens
- if (hoveredCountry === countryName || hoveredCountry === countryCode) { - return `hsl(210, 80%, ${lightness - 10}%)`; - } - - return `hsl(210, 70%, ${lightness}%)`; + const alpha = 0.15 + intensity * 0.6; + if (hoveredCountry === countryName || hoveredCountry === countryCode) { + return `hsl(var(--foreground) / ${Math.min(alpha + 0.15, 1)})`; + } + return `hsl(var(--foreground) / ${alpha})`;Apply the same swap to the legend swatches at line 200.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/world-map.tsx` around lines 99 - 119, getCountryColor currently returns hardcoded blue HSL strings for the selected/hovered/default fills; replace those hardcoded "hsl(210, …)" values with the CSS variables used elsewhere (e.g. var(--chart-1) for selected and a neutral/foreground-based variable for default/hover states) so colors follow the PR goal. Update the logic inside getCountryColor (referencing selectedCountry and hoveredCountry) to return CSS variable-based values (or CSS color-mix/opacity of var(--chart-1) and var(--muted)/var(--foreground) to preserve lighter/darker variants) and also change the legend swatches rendering (the legend swatch code that currently uses `hsl(210, 70%, ${l}%)`) to use the same CSS variables so the map hover fill and legend gradient are neutralized consistently.
🟡 Minor comments (18)
plugins/caveman/.codex-plugin/plugin.json-29-30 (1)
29-30:⚠️ Potential issue | 🟡 Minor
privacyPolicyURL/termsOfServiceURLpoint to README and LICENSE.These two fields advertise that the plugin has a privacy policy and terms of service, but the URLs link to
README.mdandLICENSErespectively, neither of which is the document the field name implies. If no real policies exist, prefer omitting the fields (if the schema allows) or hosting actual policy pages; otherwise marketplace consumers may be misled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/caveman/.codex-plugin/plugin.json` around lines 29 - 30, Update the plugin metadata in plugin.json to avoid misleading links: either remove the "privacyPolicyURL" and "termsOfServiceURL" keys if you don't have actual policy pages, or replace their values with real URLs that point to formal privacy policy and terms documents (not README.md or LICENSE). Locate the keys "privacyPolicyURL" and "termsOfServiceURL" in the plugin.json and ensure the values reference dedicated policy pages or omit the keys entirely so consumers are not misled.plugins/caveman/skills/compress/scripts/detect.py-73-74 (1)
73-74:⚠️ Potential issue | 🟡 Minor
.csvmisclassified ascode.The ternary at line 74 returns
configonly for{.json, .yaml, .yml, .toml, .ini, .cfg, .env}andcodefor everything else inSKIP_EXTENSIONS. That means.csv(data) and.dockerfile/.makefile(build configs) are reported ascode. Since the only consumer isshould_compress, classification accuracy doesn't affect the skip decision, but the printedtype=label in the CLI (__main__at line 121) will be misleading for these.Consider expanding the config set to include
.csv,.dockerfile,.makefile, or simplify by mapping these explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/caveman/skills/compress/scripts/detect.py` around lines 73 - 74, The current ternary in detect.py that returns "code" vs "config" for extensions in SKIP_EXTENSIONS misclassifies files like .csv, .dockerfile, and .makefile; update the classification logic (the branch that checks ext) to use an explicit CONFIG_EXTENSIONS set (or an EXT_TO_TYPE mapping) that includes ".csv", ".dockerfile", ".makefile" (and the existing ".json", ".yaml", ".yml", ".toml", ".ini", ".cfg", ".env") and return "config" when ext is in that set, otherwise "code", so the CLI label printed in __main__ reflects the correct type and SKIP_EXTENSIONS/should_compress behavior remains unchanged..codex/hooks.json-1-17 (1)
1-17:⚠️ Potential issue | 🟡 MinorRepo-level hook unconditionally activates caveman mode on every session.
Committing this
.codex/hooks.jsonto the repo will force the caveman-mode preamble on every Codex session/resume for any contributor who clones the repo. If that's intended only for the author's local use, consider keeping it out of version control (e.g., move to a personal config or.gitignoreit). If it's meant to be repo-wide, document that inREADME.mdso collaborators understand why their assistant suddenly starts speaking in fragments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/hooks.json around lines 1 - 17, The repository-level SessionStart hook in .codex/hooks.json unconditionally injects "caveman mode" on startup (see the "SessionStart" array with matcher "startup|resume" and the command string containing "CAVEMAN MODE ACTIVE"); remove this hook from the committed file (or delete the entire .codex/hooks.json entry), keep a local copy instead (add to .gitignore) if you want it for personal use, or if it must be repo-wide, keep the hook but add documentation in README.md explaining the behavior and intent so contributors aren't surprised.plugins/caveman/skills/caveman/SKILL.md-44-44 (1)
44-44:⚠️ Potential issue | 🟡 MinorMalformed
wenyan-fullexample.
useMemo .Wrap之has a stray leading space/period beforeWrap, mixes English ("Wrap") into what the table on line 36 describes as "Maximum classical terseness ... Fully 文言文". Compared to the surrounding examples this looks like a typo. Consider:-- wenyan-full: "物出新參照，致重繪。useMemo .Wrap之。" +- wenyan-full: "物出新參照，致重繪。以 useMemo 包之。"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/caveman/skills/caveman/SKILL.md` at line 44, The `wenyan-full` example value is malformed: remove the stray leading space/period and replace the English "Wrap" with a fully 文言文 equivalent to match other examples (i.e., change `"物出新參照，致重繪。useMemo .Wrap之。"` to a corrected classical-vernacular phrase without the extra " . " and without English, using the same tense/structure as surrounding entries); update the `wenyan-full` mapping so it reads as a proper 文言文 sentence consistent with the table semantics.plugins/caveman/skills/compress/scripts/benchmark.py-58-65 (1)
58-65:⚠️ Potential issue | 🟡 Minor
tests_dirpath is inconsistent with its comment.Line 58 comments that the glob mode uses
repo_root/tests/caveman-compress/, butPath(__file__).parent.parent.parentfromplugins/caveman/skills/compress/scripts/benchmark.pyresolves toplugins/caveman/skills/, makingtests_dir = plugins/caveman/skills/tests/caveman-compress. This mismatch between comment and actual path is confusing. Additionally,tests_dir.glob("*.original.md")on line 65 is non-recursive; if test fixtures are nested in subdirectories, they will not be discovered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/caveman/skills/compress/scripts/benchmark.py` around lines 58 - 65, The tests_dir path calculation is wrong: tests_dir is built from Path(__file__).parent.parent.parent (which points to plugins/caveman/skills/) and the comment states repo_root/tests/caveman-compress; fix by locating the repository root (e.g. walk up to repo root from Path(__file__).resolve() or use Path(__file__).parents[...] to reach the repo root) and construct tests_dir = <repo_root> / "tests" / "caveman-compress"; also change the discovery to be recursive by using tests_dir.rglob("*.original.md") instead of tests_dir.glob(...) and update the inline comment to accurately reflect the resolved path; look for symbols tests_dir, benchmark.py, and the glob usage to apply these edits.apps/ingestion/src/db/migrations/0000_create_events.sql-47-47 (1)
47-47:⚠️ Potential issue | 🟡 MinorRedundant index on
visitors.fingerprint.The
UNIQUE("fingerprint")constraint on line 47 already creates a unique btree index in Postgres, soidx_visitors_fingerprinton line 61 is duplicated storage and write overhead with no query benefit.🛠️ Proposed fix
-CREATE INDEX "idx_visitors_last_seen" ON "visitors" USING btree ("last_seen");--> statement-breakpoint -CREATE INDEX "idx_visitors_fingerprint" ON "visitors" USING btree ("fingerprint"); +CREATE INDEX "idx_visitors_last_seen" ON "visitors" USING btree ("last_seen");Also applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/src/db/migrations/0000_create_events.sql` at line 47, Remove the redundant unique btree index by deleting the explicit CREATE INDEX for idx_visitors_fingerprint since the UNIQUE("fingerprint") constraint (visitors_fingerprint_unique) already creates the same index; locate the migration SQL that defines CONSTRAINT "visitors_fingerprint_unique" and the CREATE INDEX "idx_visitors_fingerprint" and remove the CREATE INDEX statement (or wrap it in a conditional that skips creation if the constraint exists), ensuring no other migrations or code depend on the separate index name before committing.apps/ingestion/src/db/migrations/0000_create_events.sql-10-13 (1)
10-13:⚠️ Potential issue | 🟡 MinorIncomplete fix:
is_localhoststill lacksNOT NULLconstraint.Migration
0002_add_flag_columns.sqlcorrected three of the four boolean flag columns (is_preview,bot_detected,is_internal) toNOT NULL DEFAULT falsefor parity withvisitors.is_internal. However,is_localhostwas never updated and remains nullable in the events table. This inconsistency is still present and forces defensiveIS NULLchecks in queries likepublicTraffic()in filters.ts. Recommend addingis_localhostto a migration:ALTER TABLE events ALTER COLUMN is_localhost SET NOT NULL.Additionally, the TypeScript schema definition (schema.ts lines 23-26) is out of sync with the actual database schema—it still lacks
.notNull()on three columns that are alreadyNOT NULLin 0002.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ingestion/src/db/migrations/0000_create_events.sql` around lines 10 - 13, The events table still has "is_localhost" nullable—add a new migration that runs ALTER TABLE events ALTER COLUMN is_localhost SET NOT NULL (mirror 0002_add_flag_columns.sql) and ensure the TypeScript schema (schema.ts around lines 23-26) is updated to mark the boolean flags as .notNull() (is_preview, bot_detected, is_internal and is_localhost if present) so the TS schema matches the DB; after migration you can remove defensive IS NULL checks in consumers such as publicTraffic() in filters.ts.apps/example-dashboard/next.config.mjs-12-14 (1)
12-14:⚠️ Potential issue | 🟡 MinorRemove the invalid
cache.directoryconfig option.
cache.directoryis not a recognized Next.js 16 configuration option and will be silently ignored. Next.js 16 handles caching through other mechanisms:
- Build output: Use
distDirto control the directory (default:.next)- Incremental Static Regeneration (ISR): Use
incrementalCacheHandlerPathorcacheHandleruse cachedirectives: UsecacheHandlers- Dev filesystem caching: Enable
experimental.turbopackFileSystemCacheForDev(stable in 16.1+)If the intent is to configure caching, use the appropriate option above instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/next.config.mjs` around lines 12 - 14, Remove the invalid Next.js config key `cache.directory` from the exported config (the `cache: { directory: '.next/cache' }` block) because Next.js 16 does not recognize it; if you intended to change the build output directory use `distDir`, for ISR use `incrementalCacheHandlerPath`/`cacheHandler` or `cacheHandlers`, and for dev filesystem caching consider `experimental.turbopackFileSystemCacheForDev` instead—update the config to delete the `cache.directory` entry and replace it with the appropriate supported option for your goal.packages/sdk/scripts/client-directive.ts-7-12 (1)
7-12:⚠️ Potential issue | 🟡 MinorEnforce Node ≥ 20.11 and add defensive file checks.
Two valid concerns:
import.meta.dirnamerequires Node ≥ 20.11 (or 21.2+). Add"engines": { "node": ">=20.11" }topackages/sdk/package.jsonto enforce this requirement.While tsup is configured to emit both ESM and CJS formats, the script has no safeguard if a file is missing. Add
existsSynccheck to gracefully skip:Guard
-import { readFileSync, writeFileSync } from "node:fs"; +import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; const files = ["index.js", "index.cjs"]; const directive = '"use client";'; for (const file of files) { const path = join(import.meta.dirname, "..", "dist", file); + if (!existsSync(path)) continue; const source = readFileSync(path, "utf8"); if (source.startsWith(directive)) continue; writeFileSync(path, `${directive}\n${source}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/scripts/client-directive.ts` around lines 7 - 12, Add an engines field to packages/sdk/package.json setting "node": ">=20.11" to enforce the runtime requirement for import.meta.dirname, and update the script that iterates over files (the for (const file of files) loop using import.meta.dirname, readFileSync and writeFileSync) to defensively check for file existence with existsSync before reading; if the file does not exist, skip it gracefully so the script handles missing ESM/CJS outputs from tsup without throwing.apps/example-dashboard/components/signal-stream.tsx-115-115 (1)
115-115:⚠️ Potential issue | 🟡 MinorAlways-appended ellipsis on slices.
requestId.slice(0, 12) + "..."anduserAgent.slice(0, 80) + "..."show "…" even when the source string is shorter than the slice limit. Minor UI polish.💡 Suggested fix
- {requestId.slice(0, 12)}... + {requestId.length > 12 ? `${requestId.slice(0, 12)}…` : requestId}- {userAgent.slice(0, 80)}... + {userAgent.length > 80 ? `${userAgent.slice(0, 80)}…` : userAgent}Also applies to: 205-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/signal-stream.tsx` at line 115, The UI is appending "..." unconditionally after slices causing ellipses on short strings; update the rendering for requestId and userAgent so the ellipsis is only added when the original string is longer than the slice limit (e.g., for requestId check length > 12 before using slice + "..." and for userAgent check length > 80), and apply the same conditional logic to the other occurrence of userAgent/requestId in the component (look for the JSX that references requestId.slice(0, 12) and userAgent.slice(0, 80)).apps/example-dashboard/components/geo-details.tsx-102-110 (1)
102-110:⚠️ Potential issue | 🟡 MinorCity row key can collide when
regionisnull.
regionis typedstring | null(Line 23). Two distinct cities with the same name in the same country andnullregion will produce the same React key ("Paris-null-FR"), causing the toggle state to apply to multiple rows. Including a stable identifier or row index disambiguates.♻️ Suggestion
- {cities.slice(0, 8).map((city) => { - const key = `${city.city}-${city.region}-${city.country}`; + {cities.slice(0, 8).map((city, index) => { + const key = `${city.country}-${city.region ?? ""}-${city.city}-${index}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/components/geo-details.tsx` around lines 102 - 110, The React key generation in the cities mapping uses `${city.city}-${city.region}-${city.country}` which can collide when city.region is null; update the key to include a stable unique identifier (e.g., `city.id`) or fall back to the map index to disambiguate (so change the `key` binding and the `expandedCity` comparison to use the new composite key), keeping `labelText(city.city)`/`labelText(city.country)` usage intact; reference the mapping block where `cities.slice(0, 8).map((city) => {` and the `key` and `expandedCity` variables are defined.apps/example-dashboard/components/dashboard-content.tsx-759-770 (1)
759-770:⚠️ Potential issue | 🟡 MinorHostname check evaluates once at render and uses an env var that may be undefined.
Two small concerns:
process.env.NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAMEis replaced at build time. If the env var isn't set,isPersonalDashboardbecomeshostname === undefined→ alwaysfalse, which is fine, but worth ensuring the var is configured in Vercel for the personal deployment (matches the PR's "Confirm DATABASE_URL is set in Vercel" test plan).- The check runs on every render — moving it into the
useEffect(or auseMemo) with a hostname comparison guards against SSR/CSR mismatch warnings if the component ever renders on the server.♻️ Move into effect to avoid hydration drift
function DemoDataNotice() { const [dismissed, setDismissed] = useState(false); - const isPersonalDashboard = typeof window !== "undefined" && - window.location.hostname === process.env.NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAME; + const [isPersonalDashboard, setIsPersonalDashboard] = useState(false); useEffect(() => { + setIsPersonalDashboard( + window.location.hostname === process.env.NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAME, + ); if (sessionStorage.getItem("demo-notice-dismissed") === "true") { setDismissed(true); } }, []);🤖 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 759 - 770, The hostname check in DemoDataNotice (isPersonalDashboard) runs during render and uses build-time env NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAME which may be undefined; move that logic into a client-only effect (or initialize a state via useEffect/useMemo) so the comparison runs only in the browser to avoid SSR/CSR hydration drift and guard against undefined env by treating a missing NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAME as non-matching; specifically, compute isPersonalDashboard inside the existing useEffect (or set a dedicated state via useEffect) using window.location.hostname === process.env.NEXT_PUBLIC_PERSONAL_DASHBOARD_HOSTNAME and then use that state in the render-return check.apps/example-dashboard/components/dashboard-content.tsx-117-120 (1)
117-120:⚠️ Potential issue | 🟡 Minor
typeFilteraccepts unvalidated URL input.
searchParams.get("status")is cast straight toSignalEvent["type"] | "all"without checking against the allowed set. A user-tampered URL like?status=bananawill propagate that string into<SignalStream typeFilter={...}>and the type assertion silently lies to TypeScript. Suggest narrowing to a known set with a fallback.♻️ Suggested guard
- const typeFilter = ((searchParams.get("status") as SignalEvent["type"] | null) || "all") as - | SignalEvent["type"] - | "all"; + const ALLOWED_STATUS = new Set<SignalEvent["type"] | "all">(["all", "ok", "error", "warn", "info"]); + const rawStatus = (searchParams.get("status") ?? "all") as SignalEvent["type"] | "all"; + const typeFilter = ALLOWED_STATUS.has(rawStatus) ? rawStatus : "all";(adjust the allowed set to match the real
SignalEvent["type"]union)🤖 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 117 - 120, typeFilter is trusting unvalidated URL input from searchParams.get("status") and asserting it as SignalEvent["type"] | "all"; validate the value against the allowed SignalEvent types before using it (e.g., build an allowedTypes set/array that mirrors SignalEvent["type"] values and check searchParams.get("status") against it), set typeFilter to the matched value or fallback to "all", and pass that safe value into <SignalStream typeFilter={...}> instead of the raw asserted string.apps/ingestion/tests/unit/ops.test.ts-12-27 (1)
12-27:⚠️ Potential issue | 🟡 MinorAddress the hardcoded raw IP address in the test.
The test assertion about
target: "custom"is correct—theparseArgsimplementation intentionally setstargetto"custom"whenever--urlis supplied (line 264 of ops.ts), regardless of prior--targetvalues. However, the test hardcodes"http://127.0.0.1:3000", which violates the guideline forapps/ingestion/**/*.{ts,tsx}files: never store raw IP addresses. Use an environment variable, placeholder, or mock URL instead.🤖 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 12 - 27, Replace the hardcoded raw IP URL in the "parses cli args" test with a non-IP configurable value: define a test URL variable (e.g., from process.env.INGESTION_TEST_URL or a safe placeholder like "http://localhost:3000") and pass that variable into parseArgs instead of "http://127.0.0.1:3000", then update the expect(args.url) assertion to compare against that variable; locate the test by the test name and the parseArgs call to make the change.scripts/sync-vercel-env.sh-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorUnused
ENVSvariable.Shellcheck SC2034 flags
ENVSas unused. The actual env lists are passed inline ("production preview", etc.), so this constant is dead. Either remove it or use it as the canonical source where applicable.Suggested fix
-ENVS="production preview development"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-vercel-env.sh` at line 9, The ENVS variable defined in scripts/sync-vercel-env.sh is unused (Shellcheck SC2034); either remove the ENVS="production preview development" declaration or replace the inline environment lists with this constant so it becomes the canonical source; update any occurrences that currently use literal lists (e.g., "production preview", "production development") to reference ENVS (or remove ENVS if you opt to inline) and run shellcheck to confirm the warning is resolved.apps/example-dashboard/lib/queries/audience.ts-21-41 (1)
21-41:⚠️ Potential issue | 🟡 MinorDead
'Unknown'fallback when filter excludes nulls.
WHERE ... AND meta->>'browser' IS NOT NULLalready strips rows where the key is missing or SQL-NULL, soCOALESCE(meta->>'browser', 'Unknown')can never produce'Unknown'. Either drop theIS NOT NULLguard (let COALESCE bucket nulls into "Unknown") or drop the COALESCE. Same applies togetOSDetailedat lines 32–41.Suggested fix (keep "Unknown" bucket)
- await sql`SELECT COALESCE(meta->>'browser', 'Unknown') as browser, COUNT(*) as count FROM events WHERE ${publicTraffic()} AND ts >= ${from} AND ts <= ${to} AND meta->>'browser' IS NOT NULL ${projectId ? sql`AND project_id = ${projectId}` : sql``} GROUP BY browser ORDER BY count DESC LIMIT 10`; + await sql`SELECT COALESCE(meta->>'browser', 'Unknown') as browser, COUNT(*) as count FROM events WHERE ${publicTraffic()} AND ts >= ${from} AND ts <= ${to} ${projectId ? sql`AND project_id = ${projectId}` : sql``} GROUP BY browser ORDER BY count DESC LIMIT 10`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries/audience.ts` around lines 21 - 41, In getBrowsersDetailed and getOSDetailed the COALESCE(..., 'Unknown') is dead because the query also includes meta->>'browser' IS NOT NULL / meta->>'os' IS NOT NULL; decide to keep the "Unknown" bucket by removing those IS NOT NULL filters (so NULL/missing values fall into COALESCE) or remove the COALESCE if you prefer to exclude unknowns; update the SQL in getBrowsersDetailed and getOSDetailed accordingly so the COALESCE and WHERE clauses are consistent.apps/example-dashboard/lib/queries/content.ts-47-60 (1)
47-60:⚠️ Potential issue | 🟡 Minor
LIKEagainst unsanitizeddomaintreats%/_as wildcards.
domainis interpolated as a parameter (so SQL injection is fine), but the surrounding'%' || domain || '%'makes any%or_in the input act as wildcards. An emptydomainmatches every referrer, and_matches any single character. Consider exact-host matching (referrer ILIKE 'http%://' || domain || '/%') or escaping the wildcards before concatenation, and reject empty strings early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries/content.ts` around lines 47 - 60, getReferrerDetail currently builds referrer LIKE patterns with "%" + domain + "%" which treats user-supplied % and _ as wildcards and allows empty domain to match everything; update getReferrerDetail to reject empty/blank domain early, and replace the ad-hoc LIKE pattern in both queries with either (a) host-aware matching using a parameterized ILIKE pattern such as matching referrer ILIKE 'http%://' || domain || '/%' (or 'http%://%' || domain || '/%') to ensure exact-host matches, or (b) escape user input before concatenating (escape % and _ and use an explicit ESCAPE clause) so the domain is treated literally; ensure both SQL calls in getReferrerDetail (the stats and landingPages queries) are updated and remain parameterized to avoid injection.apps/ingestion/scripts/ops.ts-931-938 (1)
931-938:⚠️ Potential issue | 🟡 MinorHardcoded
/usr/bin/fzfskips macOS Homebrew installs.
existsSync("/usr/bin/fzf")will be false for most macOS dev machines (Homebrew installs to/opt/homebrew/bin/fzfon Apple Silicon or/usr/local/bin/fzfon Intel) and any user-local install (~/.fzf/bin). Those users will silently getnullfrom the palette command even though fzf is installed. Resolve via$PATHinstead.🛠️ Proposed fix — resolve via PATH
- const choice = existsSync("/usr/bin/fzf") ? await chooseFzf(all) : null; + const choice = await hasFzf() ? await chooseFzf(all) : null;async function hasFzf(): Promise<boolean> { const probe = await run(["sh", "-c", "command -v fzf"]); return probe.code === 0 && probe.output.trim().length > 0; }Alternatively, attempt
chooseFzfdirectly and catchENOENTfromspawn.🤖 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 931 - 938, The palette branch currently checks existsSync("/usr/bin/fzf") which misses Homebrew and user installs; replace that hardcoded probe with a PATH-based check or a safe try-call: implement a helper like hasFzf() that runs "sh -c 'command -v fzf'" (or uses spawn and resolves true if exit code 0 and output non-empty), then use if (await hasFzf()) ? await chooseFzf(all) : null; or simply call chooseFzf(all) inside a try/catch and treat ENOENT as "not installed" before setting/clearing input.setRawMode and calling handleChoice(state, choice) — update the code around the "palette" key and the chooseFzf call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 756539db-6a3d-4063-b858-de5570c6607c
⛔ Files ignored due to path filters (5)
bun.lockis excluded by!**/*.lockplugins/caveman/assets/caveman-small.svgis excluded by!**/*.svgplugins/caveman/assets/caveman.svgis excluded by!**/*.svgplugins/caveman/skills/caveman/assets/caveman-small.svgis excluded by!**/*.svgplugins/caveman/skills/caveman/assets/caveman.svgis excluded by!**/*.svg
📒 Files selected for processing (79)
.agents/plugins/marketplace.json.codex/config.toml.codex/hooks.jsonapps/example-dashboard/app/api/analytics/route.tsapps/example-dashboard/app/globals.cssapps/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/data-table.tsxapps/example-dashboard/components/geo-details.tsxapps/example-dashboard/components/geo-map.tsxapps/example-dashboard/components/kpi-cards.tsxapps/example-dashboard/components/signal-stream.tsxapps/example-dashboard/components/trend-chart.tsxapps/example-dashboard/components/world-map.tsxapps/example-dashboard/lib/mock-data.tsapps/example-dashboard/lib/queries.tsapps/example-dashboard/lib/queries/audience.tsapps/example-dashboard/lib/queries/content.tsapps/example-dashboard/lib/queries/filters.tsapps/example-dashboard/lib/queries/index.tsapps/example-dashboard/lib/queries/kpis.tsapps/example-dashboard/lib/queries/overview.tsapps/example-dashboard/lib/queries/realtime.tsapps/example-dashboard/lib/queries/sessions.tsapps/example-dashboard/lib/types.tsapps/example-dashboard/next.config.mjsapps/example-dashboard/package.jsonapps/example-dashboard/tests/integration/queries.test.tsapps/example-dashboard/tests/tsconfig.jsonapps/example-dashboard/tsconfig.jsonapps/example-dashboard/tsconfig.tsbuildinfoapps/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/0000_create_events.sqlapps/ingestion/src/db/migrations/0001_archive_legacy_tables.sqlapps/ingestion/src/db/migrations/0002_add_flag_columns.sqlapps/ingestion/src/db/migrations/001_add_missing_columns.sqlapps/ingestion/src/db/migrations/meta/0000_snapshot.jsonapps/ingestion/src/db/migrations/meta/_journal.jsonapps/ingestion/src/db/schema.tsapps/ingestion/src/db/seed.tsapps/ingestion/src/handlers/admin.tsapps/ingestion/src/handlers/ingest.tsapps/ingestion/src/handlers/metrics.tsapps/ingestion/src/utilities/dedupe.tsapps/ingestion/src/utilities/geo.tsapps/ingestion/tests/unit/ingest.test.tsapps/ingestion/tests/unit/ops.test.tsapps/ingestion/tests/unit/schema.test.tspackage.jsonpackages/sdk/__tests__/track.test.tspackages/sdk/package.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.tsplugins/caveman/.codex-plugin/plugin.jsonplugins/caveman/skills/caveman/SKILL.mdplugins/caveman/skills/caveman/agents/openai.yamlplugins/caveman/skills/compress/SKILL.mdplugins/caveman/skills/compress/scripts/__init__.pyplugins/caveman/skills/compress/scripts/__main__.pyplugins/caveman/skills/compress/scripts/benchmark.pyplugins/caveman/skills/compress/scripts/cli.pyplugins/caveman/skills/compress/scripts/compress.pyplugins/caveman/skills/compress/scripts/detect.pyplugins/caveman/skills/compress/scripts/validate.pyscripts/sync-vercel-env.sh
💤 Files with no reviewable changes (2)
- packages/sdk/src/index.ts
- apps/example-dashboard/lib/queries.ts
| }, | ||
| }; | ||
|
|
||
| const analyticsUrl = process.env.NEXT_PUBLIC_ANALYTICS_URL || "https://ingestion.remcostoeten.nl"; |
There was a problem hiding this comment.
Hardcoded fallback leaks analytics to a personal domain.
process.env.NEXT_PUBLIC_ANALYTICS_URL || "https://ingestion.remcostoeten.nl" means any fork, preview, or misconfigured deploy that omits the env var will silently send pageviews to a single author-controlled endpoint. Two safer options:
- Keep the previous "empty string / no-op" behavior so missing config is a no-op rather than a cross-tenant leak.
- Throw at boot in non-development when the env var is missing, so the misconfiguration is loud.
The PR test plan calls out "Confirm DATABASE_URL is set in Vercel for both apps" — applying the same explicit-config discipline to the public ingest URL would be consistent.
🛡️ Proposed fix (no-op fallback)
-const analyticsUrl = process.env.NEXT_PUBLIC_ANALYTICS_URL || "https://ingestion.remcostoeten.nl";
+const analyticsUrl = process.env.NEXT_PUBLIC_ANALYTICS_URL ?? "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const analyticsUrl = process.env.NEXT_PUBLIC_ANALYTICS_URL || "https://ingestion.remcostoeten.nl"; | |
| const analyticsUrl = process.env.NEXT_PUBLIC_ANALYTICS_URL ?? ""; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/app/layout.tsx` at line 33, Replace the hardcoded
personal-domain fallback for the NEXT_PUBLIC_ANALYTICS_URL by making
analyticsUrl a no-op when unset: change the initialization of the analyticsUrl
constant (the symbol analyticsUrl) to use process.env.NEXT_PUBLIC_ANALYTICS_URL
|| "" (empty string) so missing env config does not leak to a third-party
endpoint; alternatively, if you prefer loud failures in non-development, add a
boot-time check where, when process.env.NEXT_PUBLIC_ANALYTICS_URL is falsy and
NODE_ENV !== "development", the app throws or logs a fatal error to prevent
startup—update the usage sites that expect analyticsUrl accordingly so they skip
sending telemetry when analyticsUrl is falsy.
| import { publicTraffic, getRange, getTimeRangeFilter, formatNumber, calculateTrend } from "./filters"; | ||
| import { | ||
| getPageviewsKPI, | ||
| getUniqueVisitorsKPI, | ||
| getSessionsKPI, | ||
| getEventsKPI, | ||
| getBotRateKPI, | ||
| getErrorCountKPI, | ||
| getLocalhostRateKPI, | ||
| } from "./kpis"; | ||
| import { getTopPages, getTopReferrers, getGeoDistribution, getEntryExitPages } from "./content"; | ||
| import { getDeviceBreakdown } from "./audience"; | ||
| import { getRecentEvents } from "./realtime"; |
There was a problem hiding this comment.
Remove unused imports — currently breaking CI.
ESLint is failing on formatNumber, calculateTrend (line 3) and getEntryExitPages (line 13), which are imported but never used in this module. The entryPages/exitPages fields below are returned as empty arrays anyway, so either drop the imports or actually wire getEntryExitPages into Promise.all and the response.
🛠️ Proposed minimal fix (just remove unused)
-import { publicTraffic, getRange, getTimeRangeFilter, formatNumber, calculateTrend } from "./filters";
+import { publicTraffic, getRange, getTimeRangeFilter } from "./filters";
@@
-import { getTopPages, getTopReferrers, getGeoDistribution, getEntryExitPages } from "./content";
+import { getTopPages, getTopReferrers, getGeoDistribution } from "./content";🧰 Tools
🪛 GitHub Actions: ci
[error] 3-3: ESLint (no-unused-vars): Identifier 'formatNumber' is imported but never used.
[error] 3-3: ESLint (no-unused-vars): Identifier 'calculateTrend' is imported but never used.
[error] 13-13: ESLint (no-unused-vars): Identifier 'getEntryExitPages' is imported but never used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/lib/queries/overview.ts` around lines 3 - 15, The
imports formatNumber and calculateTrend and the import getEntryExitPages are
unused and causing ESLint/CI failures; either remove those three imports from
the top of this module or wire getEntryExitPages into the data fetch so
entryPages/exitPages are populated. To fix quickly, delete formatNumber and
calculateTrend from the import list on the first line and remove
getEntryExitPages from the content import, or alternatively add
getEntryExitPages to the Promise.all call that gathers KPIs/content and map its
result into the entryPages and exitPages response fields so the symbols
(getEntryExitPages, entryPages, exitPages, Promise.all) are actually used.
| const results = await sql` | ||
| SELECT | ||
| id, | ||
| fingerprint, | ||
| first_seen, | ||
| last_seen, | ||
| visit_count, | ||
| COALESCE(device_type, 'Unknown') as device_type, | ||
| COALESCE(browser, 'Unknown') as browser, | ||
| COALESCE(os, 'Unknown') as os, | ||
| COALESCE(browser_version, 'Unknown') as browser_version, | ||
| COALESCE(os_version, 'Unknown') as os_version, | ||
| COALESCE(screen_resolution, 'Unknown') as screen_resolution, | ||
| COALESCE(language, 'Unknown') as language, | ||
| country, | ||
| region, | ||
| city | ||
| FROM visitors | ||
| WHERE EXISTS (SELECT 1 FROM events WHERE events.visitor_id = visitors.fingerprint AND ${publicTrafficEvents()} AND events.ts >= ${range.from} AND events.ts <= ${range.to} ${projectId ? sql`AND events.project_id = ${projectId}` : sql``}) | ||
| ORDER BY last_seen DESC | ||
| LIMIT ${limit} | ||
| `; | ||
| return results.map((r) => ({ | ||
| id: String(r.id), | ||
| fingerprint: r.fingerprint, | ||
| firstSeen: r.first_seen, | ||
| lastSeen: r.last_seen, | ||
| visitCount: Number(r.visit_count), | ||
| isInternal: false, | ||
| deviceType: r.device_type, | ||
| browser: r.browser, | ||
| os: r.os, | ||
| browserVersion: r.browser_version, | ||
| osVersion: r.os_version, | ||
| screenResolution: r.screen_resolution, | ||
| language: r.language, | ||
| country: COUNTRY_NAME_TO_ISO[r.country] || r.country, | ||
| region: r.region, | ||
| city: r.city, | ||
| })); |
There was a problem hiding this comment.
isInternal is hardcoded to false and ignores the column in visitors.
The visitors table has an is_internal column (per the new migration snapshot), but this query doesn't SELECT it and the mapper sets isInternal: false unconditionally. Internal visitors will be misclassified in any UI consuming this list.
Suggested fix
COALESCE(language, 'Unknown') as language,
country,
region,
- city
+ city,
+ is_internal
FROM visitors
@@
- isInternal: false,
+ isInternal: Boolean(r.is_internal),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const results = await sql` | |
| SELECT | |
| id, | |
| fingerprint, | |
| first_seen, | |
| last_seen, | |
| visit_count, | |
| COALESCE(device_type, 'Unknown') as device_type, | |
| COALESCE(browser, 'Unknown') as browser, | |
| COALESCE(os, 'Unknown') as os, | |
| COALESCE(browser_version, 'Unknown') as browser_version, | |
| COALESCE(os_version, 'Unknown') as os_version, | |
| COALESCE(screen_resolution, 'Unknown') as screen_resolution, | |
| COALESCE(language, 'Unknown') as language, | |
| country, | |
| region, | |
| city | |
| FROM visitors | |
| WHERE EXISTS (SELECT 1 FROM events WHERE events.visitor_id = visitors.fingerprint AND ${publicTrafficEvents()} AND events.ts >= ${range.from} AND events.ts <= ${range.to} ${projectId ? sql`AND events.project_id = ${projectId}` : sql``}) | |
| ORDER BY last_seen DESC | |
| LIMIT ${limit} | |
| `; | |
| return results.map((r) => ({ | |
| id: String(r.id), | |
| fingerprint: r.fingerprint, | |
| firstSeen: r.first_seen, | |
| lastSeen: r.last_seen, | |
| visitCount: Number(r.visit_count), | |
| isInternal: false, | |
| deviceType: r.device_type, | |
| browser: r.browser, | |
| os: r.os, | |
| browserVersion: r.browser_version, | |
| osVersion: r.os_version, | |
| screenResolution: r.screen_resolution, | |
| language: r.language, | |
| country: COUNTRY_NAME_TO_ISO[r.country] || r.country, | |
| region: r.region, | |
| city: r.city, | |
| })); | |
| const results = await sql` | |
| SELECT | |
| id, | |
| fingerprint, | |
| first_seen, | |
| last_seen, | |
| visit_count, | |
| COALESCE(device_type, 'Unknown') as device_type, | |
| COALESCE(browser, 'Unknown') as browser, | |
| COALESCE(os, 'Unknown') as os, | |
| COALESCE(browser_version, 'Unknown') as browser_version, | |
| COALESCE(os_version, 'Unknown') as os_version, | |
| COALESCE(screen_resolution, 'Unknown') as screen_resolution, | |
| COALESCE(language, 'Unknown') as language, | |
| country, | |
| region, | |
| city, | |
| is_internal | |
| FROM visitors | |
| WHERE EXISTS (SELECT 1 FROM events WHERE events.visitor_id = visitors.fingerprint AND ${publicTrafficEvents()} AND events.ts >= ${range.from} AND events.ts <= ${range.to} ${projectId ? sql`AND events.project_id = ${projectId}` : sql``}) | |
| ORDER BY last_seen DESC | |
| LIMIT ${limit} | |
| `; | |
| return results.map((r) => ({ | |
| id: String(r.id), | |
| fingerprint: r.fingerprint, | |
| firstSeen: r.first_seen, | |
| lastSeen: r.last_seen, | |
| visitCount: Number(r.visit_count), | |
| isInternal: Boolean(r.is_internal), | |
| deviceType: r.device_type, | |
| browser: r.browser, | |
| os: r.os, | |
| browserVersion: r.browser_version, | |
| osVersion: r.os_version, | |
| screenResolution: r.screen_resolution, | |
| language: r.language, | |
| country: COUNTRY_NAME_TO_ISO[r.country] || r.country, | |
| region: r.region, | |
| city: r.city, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/lib/queries/realtime.ts` around lines 60 - 99, The
query in realtime.ts currently ignores the visitors.is_internal column and
hardcodes isInternal: false; update the SQL SELECT (the query that assigns const
results = await sql`...`) to include is_internal (e.g., SELECT ..., is_internal,
...) and then update the results.map(...) mapper to set isInternal based on the
returned row (e.g., map r.is_internal to isInternal using a boolean conversion)
so internal visitors are correctly represented; the relevant symbols are the SQL
block that calls publicTrafficEvents(), range.from, range.to, projectId, LIMIT
${limit}, the const results variable, and the results.map(...) mapping function.
| export async function getRetention(projectId: string | null) { | ||
| const fiveWeeksAgo = new Date(Date.now() - 35 * 24 * 60 * 60 * 1000); | ||
| const retention = | ||
| await sql`WITH visitor_cohorts AS (SELECT visitor_id, DATE_TRUNC('week', MIN(ts)) as cohort_week FROM events WHERE ${publicTraffic()} AND TRUE ${projectId ? sql`AND project_id = ${projectId}` : sql``} GROUP BY visitor_id), weekly_activity AS (SELECT e.visitor_id, vc.cohort_week, DATE_TRUNC('week', e.ts) as activity_week, EXTRACT(WEEK FROM e.ts) - EXTRACT(WEEK FROM vc.cohort_week) as weeks_since_cohort FROM events e JOIN visitor_cohorts vc ON e.visitor_id = vc.visitor_id WHERE e.ts >= ${fiveWeeksAgo} ${projectId ? sql`AND e.project_id = ${projectId}` : sql``}) SELECT cohort_week, weeks_since_cohort::int as week_number, COUNT(DISTINCT visitor_id) as visitors FROM weekly_activity WHERE weeks_since_cohort >= 0 AND weeks_since_cohort <= 4 GROUP BY cohort_week, weeks_since_cohort ORDER BY cohort_week, week_number`; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL EXTRACT WEEK across year boundary behavior
💡 Result:
PostgreSQL's EXTRACT(WEEK FROM date) follows the ISO 8601 week-numbering system. Weeks start on Monday, and the first week of a year is the one containing January 4th (i.e., the week with the first Thursday of the year). Across year boundaries: - Early January dates may belong to the previous year's week 52 or 53. Examples: 2005-01-01 is week 53 of 2004; 2006-01-01 is week 52 of 2005; 2016-01-03 may be week 53 of 2015. - Late December dates may belong to the next year's week 1. Example: 2012-12-31 is week 1 of 2013. To get consistent week-year pairing, use EXTRACT(ISOYEAR FROM date) with EXTRACT(WEEK FROM date). EXTRACT(YEAR FROM date) gives the Gregorian year, which may differ. This behavior is unchanged in recent versions (up to PostgreSQL 18 docs).
Citations:
- 1: https://www.postgresql.org/docs/current/functions-datetime.html
- 2: http://www.postgres.com/docs/18/functions-datetime.html
- 3: https://stackoverflow.com/questions/66492551/how-can-i-get-proper-week-number-in-the-start-of-the-year
- 4: https://dba.stackexchange.com/questions/172982/extracting-week-from-date-seems-to-give-incorrect-value-at-the-start-of-the-year
- 5: https://www.postgresql.org/message-id/df5f54d230b61e4d6a3c9161c3f7ca06%40mail.gmail.com
- 6: https://stackoverflow.com/questions/34050103/extract-week-number-from-date-postgres
🏁 Script executed:
find . -name "sessions.ts" -type f | head -20Repository: remcostoeten/analytics
Length of output: 115
🏁 Script executed:
cat -n ./apps/example-dashboard/lib/queries/sessions.tsRepository: remcostoeten/analytics
Length of output: 7023
Fix week-boundary bug in retention calculation.
EXTRACT(WEEK FROM date) returns ISO week 1–53 and resets yearly. Cohorts spanning late December to early January produce negative weeks_since_cohort (e.g., 1 - 52 = -49) and are silently filtered out by WHERE weeks_since_cohort >= 0 AND weeks_since_cohort <= 4, corrupting retention stats. Subtract timestamp objects directly instead:
Fix
- await sql`WITH visitor_cohorts AS (SELECT visitor_id, DATE_TRUNC('week', MIN(ts)) as cohort_week FROM events WHERE ${publicTraffic()} AND TRUE ${projectId ? sql`AND project_id = ${projectId}` : sql``} GROUP BY visitor_id), weekly_activity AS (SELECT e.visitor_id, vc.cohort_week, DATE_TRUNC('week', e.ts) as activity_week, EXTRACT(WEEK FROM e.ts) - EXTRACT(WEEK FROM vc.cohort_week) as weeks_since_cohort FROM events e JOIN visitor_cohorts vc ON e.visitor_id = vc.visitor_id WHERE e.ts >= ${fiveWeeksAgo} ${projectId ? sql`AND e.project_id = ${projectId}` : sql``}) SELECT cohort_week, weeks_since_cohort::int as week_number, COUNT(DISTINCT visitor_id) as visitors FROM weekly_activity WHERE weeks_since_cohort >= 0 AND weeks_since_cohort <= 4 GROUP BY cohort_week, weeks_since_cohort ORDER BY cohort_week, week_number`;
+ await sql`WITH visitor_cohorts AS (SELECT visitor_id, DATE_TRUNC('week', MIN(ts)) as cohort_week FROM events WHERE ${publicTraffic()} ${projectId ? sql`AND project_id = ${projectId}` : sql``} GROUP BY visitor_id), weekly_activity AS (SELECT e.visitor_id, vc.cohort_week, DATE_TRUNC('week', e.ts) as activity_week, ((DATE_TRUNC('week', e.ts) - vc.cohort_week) / INTERVAL '1 week')::int as weeks_since_cohort FROM events e JOIN visitor_cohorts vc ON e.visitor_id = vc.visitor_id WHERE e.ts >= ${fiveWeeksAgo} ${projectId ? sql`AND e.project_id = ${projectId}` : sql``}) SELECT cohort_week, weeks_since_cohort as week_number, COUNT(DISTINCT visitor_id) as visitors FROM weekly_activity WHERE weeks_since_cohort >= 0 AND weeks_since_cohort <= 4 GROUP BY cohort_week, weeks_since_cohort ORDER BY cohort_week, week_number`;Also removes stray AND TRUE.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/lib/queries/sessions.ts` at line 54, The retention
query in sessions.ts uses EXTRACT(WEEK FROM ...) subtraction
(weeks_since_cohort) which breaks across year boundaries and also contains a
stray "AND TRUE"; replace the weeks_since_cohort expression in the
weekly_activity CTE with a proper week-difference computed from truncated
timestamps (e.g. compute weeks_since_cohort as FLOOR(EXTRACT(EPOCH FROM
(DATE_TRUNC('week', e.ts) - DATE_TRUNC('week', vc.cohort_week))) / 604800) or an
equivalent interval/CAST-to-int calculation) and remove the unnecessary "AND
TRUE" in the visitor_cohorts WHERE clause; keep the existing uses of
visitor_cohorts, weekly_activity, fiveWeeksAgo, publicTraffic() and projectId
filters intact.
| experimental: { | ||
| optimizePackageInputs: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js 16 experimental optimizePackageInputs vs optimizePackageImports
💡 Result:
In Next.js 16, there is no experimental.optimizePackageInputs option. The correct and only documented experimental configuration for optimizing package imports is experimental.optimizePackageImports. experimental.optimizePackageImports is an experimental feature that tree-shakes large packages with many exports (e.g., icon libraries like lucide-react, date-fns, lodash-es). It analyzes barrel exports and loads only the used modules, improving dev and prod performance by reducing bundle sizes and startup times. Configuration example: module.exports = { experimental: { optimizePackageImports: ['package-name'], }, }; Several popular libraries are optimized by default, including lucide-react, date-fns, antd, @mui/material, recharts, and others (full list in docs). This feature remains under experimental in Next.js 16, as no changes were noted in release notes or upgrading guides. It is not recommended for production due to potential changes. No sources mention optimizePackageInputs; searches across docs, blogs, GitHub issues, and changelogs returned zero results for it. It may be a misspelling or confusion with optimizePackageImports. Related options include serverExternalPackages (for excluding from server bundling) but differ in purpose.
Citations:
- 1: https://nextjs.org/docs/app/api-reference/config/next-config-js/optimizePackageImports
- 2: https://nextjs.org/docs/15/app/api-reference/config/next-config-js/optimizePackageImports
- 3: https://nextjs.org/docs/15/app/guides/package-bundling
- 4: https://nextjs.org/blog/next-16
- 5: https://tigerabrodi.blog/optimizepackageimports-in-nextjs
- 6: https://nextjs.org/docs/app/api-reference/next-config-js/optimizePackageImports
- 7: https://nextjs.org/docs/app/guides/upgrading/version-16
- 8: https://nextjs.org/docs/app/api-reference/next-config-js
- 9: https://nextjs.org/docs/pages/api-reference/next-config-js
Fix typo: optimizePackageInputs should be optimizePackageImports
optimizePackageInputs is not a valid Next.js experimental flag and will be silently ignored. The correct option is optimizePackageImports, which accepts an array of package names to tree-shake for better performance.
🔧 Suggested fix
experimental: {
- optimizePackageInputs: true,
+ optimizePackageImports: ['lucide-react', 'recharts', 'date-fns'],
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/next.config.mjs` around lines 3 - 5, The Next.js
config uses the invalid experimental property `optimizePackageInputs`; update
the `experimental` object in next.config.mjs to use `optimizePackageImports`
instead and provide the appropriate array of package names to tree-shake (e.g.,
set experimental.optimizePackageImports = ['package-a', 'package-b']) so the
flag is recognized and effective; locate and replace the `optimizePackageInputs`
key in the `experimental` block in next.config.mjs.
| CREATE INDEX CONCURRENTLY IF NOT EXISTS events_is_preview_idx ON events (is_preview); | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS events_bot_detected_idx ON events (bot_detected); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'drizzle.config' apps/ingestion --exec cat
fd -e sql . apps/ingestion/src/db/migrations --exec head -5 {}Repository: remcostoeten/analytics
Length of output: 1033
🏁 Script executed:
cat -n apps/ingestion/src/db/migrations/0002_add_flag_columns.sqlRepository: remcostoeten/analytics
Length of output: 949
CREATE INDEX CONCURRENTLY cannot run inside a transaction block.
Drizzle Kit wraps each migration file in a single transaction by default. Your drizzle.config.ts contains no breakpoint directives or transaction-disabling configuration, so this migration will fail with ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block.
Fix this by removing CONCURRENTLY (acceptable for a fresh Neon DB where the events table is empty and lock contention is not a concern):
Recommended fix
-CREATE INDEX CONCURRENTLY IF NOT EXISTS events_is_preview_idx ON events (is_preview);
-CREATE INDEX CONCURRENTLY IF NOT EXISTS events_bot_detected_idx ON events (bot_detected);
+CREATE INDEX IF NOT EXISTS events_is_preview_idx ON events (is_preview);
+CREATE INDEX IF NOT EXISTS events_bot_detected_idx ON events (bot_detected);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE INDEX CONCURRENTLY IF NOT EXISTS events_is_preview_idx ON events (is_preview); | |
| CREATE INDEX CONCURRENTLY IF NOT EXISTS events_bot_detected_idx ON events (bot_detected); | |
| CREATE INDEX IF NOT EXISTS events_is_preview_idx ON events (is_preview); | |
| CREATE INDEX IF NOT EXISTS events_bot_detected_idx ON events (bot_detected); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ingestion/src/db/migrations/0002_add_flag_columns.sql` around lines 6 -
7, The migration uses "CREATE INDEX CONCURRENTLY" which fails because Drizzle
Kit wraps migrations in a transaction; edit the migration in
0002_add_flag_columns.sql and change the two statements that create
events_is_preview_idx and events_bot_detected_idx to use plain "CREATE INDEX"
(remove the CONCURRENTLY keyword) so the migration can run inside Drizzle's
transaction, or alternatively adjust Drizzle config/breakpoint to disable
transactional wrapping if you intentionally need CONCURRENTLY.
| ALTER TABLE events ADD COLUMN IF NOT EXISTS is_preview BOOLEAN DEFAULT FALSE; | ||
| ALTER TABLE events ADD COLUMN IF NOT EXISTS bot_detected BOOLEAN DEFAULT FALSE; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect migration journal and any other migration filenames for naming/ordering inconsistencies.
fd -t f . apps/ingestion/src/db/migrations
fd -t f 'journal\.json$' apps/ingestion/src/db/migrations -x cat {}
rg -nP -C2 'is_preview|bot_detected' apps/ingestion/src/db/migrationsRepository: remcostoeten/analytics
Length of output: 6552
Delete 001_add_missing_columns.sql — it is redundant and uses incorrect naming.
This file is a pure no-op:
-
Redundancy:
0000_create_events.sql(lines 11–12) already defines bothis_previewandbot_detectedwithDEFAULT false. TheIF NOT EXISTSclause ensures this file does nothing on a fresh database, making its presence misleading and unnecessary. -
Naming violation: The filename uses a 3-digit prefix (
001_) while Drizzle's convention is 4-digit (0000_,0001_,0002_). Additionally, the_journal.jsondoes not track this file — only0000_create_eventsis listed — indicating Drizzle Kit did not generate it and will not recognize it as a managed migration. -
Additional redundancy:
0002_add_flag_columns.sqlalso adds these same columns. While that file includes legitimate UPDATE logic to populate values from JSON metadata, this file (001_add_missing_columns.sql) adds nothing beyond the redundant ALTER statements.
Delete this file and clean up the migrations directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ingestion/src/db/migrations/001_add_missing_columns.sql` around lines 1
- 2, Remove the redundant migration file 001_add_missing_columns.sql: delete the
file from the migrations directory because its ALTER TABLE statements for
is_preview and bot_detected are already defined in 0000_create_events.sql and
also handled (with update logic) in 0002_add_flag_columns.sql, and the filename
violates the 4-digit Drizzle convention and is not tracked in the migrations
_journal.json; after deleting, ensure the migrations folder and the journal only
contain the intended Drizzle-managed files (e.g., 0000_create_events.sql,
0002_add_flag_columns.sql) so the migration sequence and naming conventions
remain consistent.
| export function resolveDefaultIngestUrl(): string { | ||
| const env = getEnv(); | ||
| const url = env.NEXT_PUBLIC_ANALYTICS_URL || env.VITE_ANALYTICS_URL || "http://localhost:3001"; | ||
| const url = env.NEXT_PUBLIC_ANALYTICS_URL || env.VITE_ANALYTICS_URL; | ||
|
|
||
| if (!url) { | ||
| if (typeof window !== "undefined") { | ||
| console.error("[Analytics] No ingest URL configured. Set NEXT_PUBLIC_ANALYTICS_URL or VITE_ANALYTICS_URL."); | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| if (typeof window !== "undefined" && !validateIngestUrl(url)) { | ||
| console.error(`[Analytics] Invalid ingestUrl: "${url}". Must be a valid http/https URL.`); | ||
| return "http://localhost:3001"; | ||
| return ""; | ||
| } | ||
|
|
||
| return url; | ||
| } |
There was a problem hiding this comment.
Normalize the env-derived URL to avoid //e in the request path.
resolveDefaultIngestUrl validates a normalized copy but returns the raw url. If NEXT_PUBLIC_ANALYTICS_URL (or VITE_ANALYTICS_URL) is set with a trailing slash (e.g. https://api.example.com/), the caller computes endpoint = "${baseUrl}/e" and ends up with https://api.example.com//e. The options.ingestUrl path already runs through normalizeIngestUrl, but the env path does not.
🛠️ Proposed fix
export function resolveDefaultIngestUrl(): string {
const env = getEnv();
const url = env.NEXT_PUBLIC_ANALYTICS_URL || env.VITE_ANALYTICS_URL;
if (!url) {
if (typeof window !== "undefined") {
console.error("[Analytics] No ingest URL configured. Set NEXT_PUBLIC_ANALYTICS_URL or VITE_ANALYTICS_URL.");
}
return "";
}
if (typeof window !== "undefined" && !validateIngestUrl(url)) {
console.error(`[Analytics] Invalid ingestUrl: "${url}". Must be a valid http/https URL.`);
return "";
}
- return url;
+ 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
currently validates the raw env URL but returns it unnormalized, causing double
slashes when callers append paths; update resolveDefaultIngestUrl to normalize
the env-derived URL before returning (use the existing normalizeIngestUrl helper
and/or validate the normalized value with validateIngestUrl) so the function
returns a cleaned URL string (reference symbols: resolveDefaultIngestUrl,
normalizeIngestUrl, validateIngestUrl).
| # Step 2: Validate + Retry | ||
| for attempt in range(MAX_RETRIES): | ||
| print(f"\nValidation attempt {attempt + 1}") | ||
|
|
||
| result = validate(backup_path, filepath) | ||
|
|
||
| if result.is_valid: | ||
| print("Validation passed") | ||
| break | ||
|
|
||
| print("❌ Validation failed:") | ||
| for err in result.errors: | ||
| print(f" - {err}") | ||
|
|
||
| if attempt == MAX_RETRIES - 1: | ||
| # Restore original on failure | ||
| filepath.write_text(original_text) | ||
| backup_path.unlink(missing_ok=True) | ||
| print("❌ Failed after retries — original restored") | ||
| return False | ||
|
|
||
| print("Fixing with Claude...") | ||
| compressed = call_claude( | ||
| build_fix_prompt(original_text, compressed, result.errors) | ||
| ) | ||
| filepath.write_text(compressed) |
There was a problem hiding this comment.
Retry budget is off by one — only 1 fix attempt actually runs.
MAX_RETRIES = 2 combined with range(MAX_RETRIES) means the loop executes twice, but each iteration is validate-then-(maybe)-fix with the last iteration short-circuiting on failure. Trace:
- attempt 0: validate → fail → call Claude fix → write
- attempt 1: validate → fail → restore original, return False
So Claude only ever gets one chance to fix validation errors, not two. But SKILL.md line 33 says "retry up to 2 times" and the PR description echoes that. To actually allow up to two fix attempts, the loop needs to validate one more time after the second fix:
♻️ Proposed fix
-MAX_RETRIES = 2
+MAX_FIX_ATTEMPTS = 2
@@
- # Step 2: Validate + Retry
- for attempt in range(MAX_RETRIES):
- print(f"\nValidation attempt {attempt + 1}")
-
- result = validate(backup_path, filepath)
-
- if result.is_valid:
- print("Validation passed")
- break
-
- print("❌ Validation failed:")
- for err in result.errors:
- print(f" - {err}")
-
- if attempt == MAX_RETRIES - 1:
- # Restore original on failure
- filepath.write_text(original_text)
- backup_path.unlink(missing_ok=True)
- print("❌ Failed after retries — original restored")
- return False
-
- print("Fixing with Claude...")
- compressed = call_claude(
- build_fix_prompt(original_text, compressed, result.errors)
- )
- filepath.write_text(compressed)
-
- return True
+ # Step 2: Validate, then up to MAX_FIX_ATTEMPTS fix passes
+ for attempt in range(MAX_FIX_ATTEMPTS + 1):
+ print(f"\nValidation attempt {attempt + 1}")
+ result = validate(backup_path, filepath)
+ if result.is_valid:
+ print("Validation passed")
+ return True
+
+ print("❌ Validation failed:")
+ for err in result.errors:
+ print(f" - {err}")
+
+ if attempt == MAX_FIX_ATTEMPTS:
+ filepath.write_text(original_text)
+ backup_path.unlink(missing_ok=True)
+ print("❌ Failed after retries — original restored")
+ return False
+
+ print("Fixing with Claude...")
+ compressed = call_claude(
+ build_fix_prompt(original_text, compressed, result.errors)
+ )
+ filepath.write_text(compressed)
+
+ return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/caveman/skills/compress/scripts/compress.py` around lines 200 - 225,
The loop currently restores on the last iteration before giving Claude a final
fix, so with MAX_RETRIES=2 you only get one fix attempt; change the branch so
that after a failed validation you only restore/return when attempt ==
MAX_RETRIES - 1, otherwise call_claude to produce a new compressed and write it;
in other words, inside the for attempt in range(MAX_RETRIES) block keep
validate(result), print errors, then if attempt < MAX_RETRIES - 1
call_claude(build_fix_prompt(original_text, compressed, result.errors)) and
filepath.write_text(compressed) to retry, but if attempt == MAX_RETRIES - 1
restore original_text to filepath and backup_path.unlink(...) and return False.
| set_env() { | ||
| local name="$1" value="$2" envs="$3" | ||
| for env in $envs; do | ||
| if env_exists "$name" "$env"; then | ||
| log "Removing existing $name ($env)..." | ||
| echo | vercel env rm "$name" "$env" --yes 2>/dev/null || true | ||
| fi | ||
| log "Setting $name ($env)..." | ||
| echo "$value" | vercel env add "$name" "$env" >/dev/null 2>&1 | ||
| ok "$name → $env" | ||
| done | ||
| } |
There was a problem hiding this comment.
vercel env add failures are silenced — script reports ok on failure.
>/dev/null 2>&1 swallows stderr from vercel env add, so a failed add (network error, auth issue, validation failure) still falls through to ok "$name → $env". Same issue with the rm call: only 2>/dev/null || true, but at minimum the add should propagate failure.
Suggested fix
log "Setting $name ($env)..."
- echo "$value" | vercel env add "$name" "$env" >/dev/null 2>&1
- ok "$name → $env"
+ if echo "$value" | vercel env add "$name" "$env" >/dev/null; then
+ ok "$name → $env"
+ else
+ warn "Failed to set $name → $env"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set_env() { | |
| local name="$1" value="$2" envs="$3" | |
| for env in $envs; do | |
| if env_exists "$name" "$env"; then | |
| log "Removing existing $name ($env)..." | |
| echo | vercel env rm "$name" "$env" --yes 2>/dev/null || true | |
| fi | |
| log "Setting $name ($env)..." | |
| echo "$value" | vercel env add "$name" "$env" >/dev/null 2>&1 | |
| ok "$name → $env" | |
| done | |
| } | |
| set_env() { | |
| local name="$1" value="$2" envs="$3" | |
| for env in $envs; do | |
| if env_exists "$name" "$env"; then | |
| log "Removing existing $name ($env)..." | |
| echo | vercel env rm "$name" "$env" --yes 2>/dev/null || true | |
| fi | |
| log "Setting $name ($env)..." | |
| if echo "$value" | vercel env add "$name" "$env" >/dev/null; then | |
| ok "$name → $env" | |
| else | |
| warn "Failed to set $name → $env" | |
| exit 1 | |
| fi | |
| done | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-vercel-env.sh` around lines 28 - 39, The set_env function
currently silences failures from vercel commands so the script always prints ok
even when vercel env add fails; change set_env to stop discarding vercel env add
errors (remove the >/dev/null 2>&1), capture and check the exit status of the
vercel env add call (and similarly avoid swallowing rm failures in the
env_exists removal path), and only call ok "$name → $env" when the add
succeeded; on failure log an error with the command output (use log/error
functions) and exit non‑zero or return a non‑zero status so callers can detect
the failure—refer to set_env, env_exists, ok and log in the diff to locate where
to make these changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/example-dashboard/lib/queries/skriuw.ts (3)
23-32: Operational: consider expression indexes on the JSONB extractions used as filters/group keys.Every Skriuw query filters or groups on
meta->>'eventName'(and one onmeta->>'method', one onmeta->>'query'). Combined withproject_idandtspredicates, these will benefit from expression indexes once theeventstable grows, e.g.:CREATE INDEX events_meta_event_name_ts_idx ON events (project_id, ((meta->>'eventName')), ts DESC) WHERE type = 'event';Worth pairing with the new Drizzle migrations being added in this PR so the dashboard doesn't regress on a hot path once Skriuw traffic scales.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries/skriuw.ts` around lines 23 - 32, The queries in skriuw.ts repeatedly filter/group on JSONB extractions (meta->>'eventName', meta->>'method', meta->>'query') combined with project_id and ts (and WHERE type = 'event'), which will need expression indexes to stay performant; add Drizzle migration(s) that create expression indexes on the events table such as indexes over (project_id, ((meta->>'eventName')), ts DESC) with WHERE type = 'event' and analogous indexes for ((meta->>'method')) and ((meta->>'query')) so the SELECT/ GROUP BY in the SQL query that references meta->>'eventName' and the other queries use the new indexes.
116-137: Use a namedtypefor the return shape instead ofRecord<string, unknown>[].The function shapes the result into a known structure (
ts,path,eventName,meta,visitorId,sessionId), but the return type erases all of it, forcing every caller to re-type or guess the fields. Define aRecentEventtype and returnRecentEvent[]so the route handler and any UI consumers get type safety.♻️ Proposed fix
+export type RecentEvent = { + ts: unknown; + path: unknown; + eventName: unknown; + meta: unknown; + visitorId: unknown; + sessionId: unknown; +}; + -export async function getSkriuwRecentEvents(projectId: string, limit: number = 50, from?: Date, to?: Date): Promise<Record<string, unknown>[]> { +export async function getSkriuwRecentEvents(projectId: string, limit: number = 50, from?: Date, to?: Date): Promise<RecentEvent[]> {(Tighten the inner field types —
Date,string | null, etc. — once you confirm what neon returns for each column.)As per coding guidelines: "Use
type, neverinterface" — and using a typed shape overRecord<string, unknown>keeps the API contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries/skriuw.ts` around lines 116 - 137, Define a named return type RecentEvent and use it instead of Record<string, unknown>[] for getSkriuwRecentEvents: add a type RecentEvent = { ts: Date; path: string | null; eventName: string; meta: any; visitorId: string | null; sessionId: string | null } (adjust nullable types to match Neon column behavior), change the function signature to Promise<RecentEvent[]>, and ensure the results.map in getSkriuwRecentEvents returns objects matching RecentEvent (use the existing mapped fields ts, path, eventName, meta, visitorId, sessionId).
20-114: Optional: extract a shared event-aggregation helper.
getSkriuwEventCounts,getSkriuwNotesActivity,getSkriuwJournalActivity, andgetSkriuwAuthMetricsshare the same query skeleton (date range +publicTraffic()+project_id+type='event'), differing only in the SELECT key and theeventNamepredicate. A small helper that takes the SELECT key ('eventName'/'method') and a list ofeventNamefilters would deduplicate ~80% of these bodies and make future schema changes single-touch. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/example-dashboard/lib/queries/skriuw.ts` around lines 20 - 114, The four functions getSkriuwEventCounts, getSkriuwNotesActivity, getSkriuwJournalActivity and getSkriuwAuthMetrics repeat the same query skeleton; extract a single helper (e.g., aggregateEventByKey) that accepts parameters for projectId, range (from/to via getRange), the JSON key to SELECT (meta->>'eventName' or meta->>'method'), optional eventName filter list, GROUP/ORDER clauses, and the publicTraffic() predicate, build and run the parametrized SQL, and return normalized { keyValue, count } rows; then refactor each of the four functions to call this helper (mapping keyValue to eventName or method and converting count to Number) so the DB logic is centralized and future schema/condition changes are single-touch.
🤖 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 110-112: The case "skriuw-recent" uses
parseInt(searchParams.get("limit") || "50") without a radix or NaN handling,
allowing NaN or excessively large values to reach query.getSkriuwRecentEvents;
change to parse with a radix (parseInt(..., 10)), validate the result (fallback
to a safe default like 50 on NaN), and clamp it to a reasonable max (e.g. 1000)
before passing to query.getSkriuwRecentEvents; also wrap the case body in braces
so skriuwLimit is block-scoped (resolving the noSwitchDeclarations warning) and
then call NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ??
"skriuw", skriuwLimit, from, to)).
- Around line 100-114: The handler is incorrectly hardcoding "skriuw" as a
fallback (projectFilter ?? "skriuw") for all skriuw metrics which leaks
tenant-specific behavior; change the seven cases to pass projectFilter (allowing
undefined) instead of forcing "skriuw", and update the corresponding query
functions in lib/queries/skriuw.ts (e.g., getSkriuwEventCounts,
getSkriuwEventTrend, getSkriuwNotesActivity, getSkriuwJournalActivity,
getSkriuwAuthMetrics, getSkriuwRecentEvents, getSkriuwTopSearches) to accept
projectId?: string and omit the "AND project_id = ..." predicate when projectId
is not provided so the route remains multi-tenant and unfiltered when no
projectFilter is supplied.
---
Nitpick comments:
In `@apps/example-dashboard/lib/queries/skriuw.ts`:
- Around line 23-32: The queries in skriuw.ts repeatedly filter/group on JSONB
extractions (meta->>'eventName', meta->>'method', meta->>'query') combined with
project_id and ts (and WHERE type = 'event'), which will need expression indexes
to stay performant; add Drizzle migration(s) that create expression indexes on
the events table such as indexes over (project_id, ((meta->>'eventName')), ts
DESC) with WHERE type = 'event' and analogous indexes for ((meta->>'method'))
and ((meta->>'query')) so the SELECT/ GROUP BY in the SQL query that references
meta->>'eventName' and the other queries use the new indexes.
- Around line 116-137: Define a named return type RecentEvent and use it instead
of Record<string, unknown>[] for getSkriuwRecentEvents: add a type RecentEvent =
{ ts: Date; path: string | null; eventName: string; meta: any; visitorId: string
| null; sessionId: string | null } (adjust nullable types to match Neon column
behavior), change the function signature to Promise<RecentEvent[]>, and ensure
the results.map in getSkriuwRecentEvents returns objects matching RecentEvent
(use the existing mapped fields ts, path, eventName, meta, visitorId,
sessionId).
- Around line 20-114: The four functions getSkriuwEventCounts,
getSkriuwNotesActivity, getSkriuwJournalActivity and getSkriuwAuthMetrics repeat
the same query skeleton; extract a single helper (e.g., aggregateEventByKey)
that accepts parameters for projectId, range (from/to via getRange), the JSON
key to SELECT (meta->>'eventName' or meta->>'method'), optional eventName filter
list, GROUP/ORDER clauses, and the publicTraffic() predicate, build and run the
parametrized SQL, and return normalized { keyValue, count } rows; then refactor
each of the four functions to call this helper (mapping keyValue to eventName or
method and converting count to Number) so the DB logic is centralized and future
schema/condition changes are single-touch.
🪄 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: 4d754468-881d-4915-b06c-2ac6c0d90cf5
📒 Files selected for processing (4)
apps/example-dashboard/app/api/analytics/route.tsapps/example-dashboard/lib/queries/index.tsapps/example-dashboard/lib/queries/skriuw.tsapps/example-dashboard/tsconfig.tsbuildinfo
✅ Files skipped from review due to trivial changes (1)
- apps/example-dashboard/lib/queries/index.ts
| case "skriuw-events": | ||
| return NextResponse.json(await query.getSkriuwEventCounts(projectFilter ?? "skriuw", from, to)); | ||
| case "skriuw-trend": | ||
| return NextResponse.json(await query.getSkriuwEventTrend(projectFilter ?? "skriuw", from, to)); | ||
| case "skriuw-notes": | ||
| return NextResponse.json(await query.getSkriuwNotesActivity(projectFilter ?? "skriuw", from, to)); | ||
| case "skriuw-journal": | ||
| return NextResponse.json(await query.getSkriuwJournalActivity(projectFilter ?? "skriuw", from, to)); | ||
| case "skriuw-auth": | ||
| return NextResponse.json(await query.getSkriuwAuthMetrics(projectFilter ?? "skriuw", from, to)); | ||
| case "skriuw-recent": | ||
| const skriuwLimit = parseInt(searchParams.get("limit") || "50"); | ||
| return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ?? "skriuw", skriuwLimit, from, to)); | ||
| case "skriuw-searches": | ||
| return NextResponse.json(await query.getSkriuwTopSearches(projectFilter ?? "skriuw", 20, from, to)); |
There was a problem hiding this comment.
Hardcoded "skriuw" project fallback leaks tenant-specific behavior into the shared API route.
Every new branch uses projectFilter ?? "skriuw", but elsewhere in this handler the convention is to pass projectFilter (undefined → unfiltered) or projectId (null). Consequences:
- A request like
GET /api/analytics?metric=skriuw-eventswith noprojectIdsilently scopes results to a project literally identified by the string"skriuw". On any deployment whose project id isn't"skriuw"(or whose schema uses UUIDs), this returns empty data with no error. - It bakes a single-tenant assumption into a route that the rest of the file treats as multi-project.
The root cause is that getSkriuw* in lib/queries/skriuw.ts declares projectId: string as required. Recommend making projectId optional in those query functions (omit the AND project_id = ... predicate when not provided), and then dropping the ?? "skriuw" here so behavior matches the other case branches.
♻️ Proposed direction
In skriuw.ts:
-export async function getSkriuwEventCounts(projectId: string, from?: Date, to?: Date): Promise<EventCount[]> {
+export async function getSkriuwEventCounts(projectId?: string, from?: Date, to?: Date): Promise<EventCount[]> {
const range = getRange(from, to);
- const results =
- await sql`... AND project_id = ${projectId} ...`;
+ const projectClause = projectId ? sql`AND project_id = ${projectId}` : sql``;
+ const results =
+ await sql`... ${projectClause} ...`;In route.ts:
- case "skriuw-events":
- return NextResponse.json(await query.getSkriuwEventCounts(projectFilter ?? "skriuw", from, to));
+ case "skriuw-events":
+ return NextResponse.json(await query.getSkriuwEventCounts(projectFilter, from, to));(Apply to all seven skriuw-* cases.)
🧰 Tools
🪛 Biome (2.4.12)
[error] 111-111: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/app/api/analytics/route.ts` around lines 100 - 114,
The handler is incorrectly hardcoding "skriuw" as a fallback (projectFilter ??
"skriuw") for all skriuw metrics which leaks tenant-specific behavior; change
the seven cases to pass projectFilter (allowing undefined) instead of forcing
"skriuw", and update the corresponding query functions in lib/queries/skriuw.ts
(e.g., getSkriuwEventCounts, getSkriuwEventTrend, getSkriuwNotesActivity,
getSkriuwJournalActivity, getSkriuwAuthMetrics, getSkriuwRecentEvents,
getSkriuwTopSearches) to accept projectId?: string and omit the "AND project_id
= ..." predicate when projectId is not provided so the route remains
multi-tenant and unfiltered when no projectFilter is supplied.
| case "skriuw-recent": | ||
| const skriuwLimit = parseInt(searchParams.get("limit") || "50"); | ||
| return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ?? "skriuw", skriuwLimit, from, to)); |
There was a problem hiding this comment.
parseInt without radix or NaN guard for limit.
parseInt(searchParams.get("limit") || "50") will return NaN for non-numeric inputs (e.g. ?limit=abc), which then flows into the SQL LIMIT clause and 500s. Also no upper bound, so ?limit=1000000 is allowed. Validate and clamp at the boundary.
🛡️ Proposed fix
- case "skriuw-recent":
- const skriuwLimit = parseInt(searchParams.get("limit") || "50");
- return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ?? "skriuw", skriuwLimit, from, to));
+ case "skriuw-recent": {
+ const parsed = Number(searchParams.get("limit"));
+ const skriuwLimit = Number.isFinite(parsed) && parsed > 0 ? Math.min(parsed, 500) : 50;
+ return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter, skriuwLimit, from, to));
+ }The added braces also resolve the Biome noSwitchDeclarations warning on line 111 by scoping skriuwLimit to its own block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "skriuw-recent": | |
| const skriuwLimit = parseInt(searchParams.get("limit") || "50"); | |
| return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ?? "skriuw", skriuwLimit, from, to)); | |
| case "skriuw-recent": { | |
| const parsed = Number(searchParams.get("limit")); | |
| const skriuwLimit = Number.isFinite(parsed) && parsed > 0 ? Math.min(parsed, 500) : 50; | |
| return NextResponse.json(await query.getSkriuwRecentEvents(projectFilter, skriuwLimit, from, to)); | |
| } |
🧰 Tools
🪛 Biome (2.4.12)
[error] 111-111: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/example-dashboard/app/api/analytics/route.ts` around lines 110 - 112,
The case "skriuw-recent" uses parseInt(searchParams.get("limit") || "50")
without a radix or NaN handling, allowing NaN or excessively large values to
reach query.getSkriuwRecentEvents; change to parse with a radix (parseInt(...,
10)), validate the result (fallback to a safe default like 50 on NaN), and clamp
it to a reasonable max (e.g. 1000) before passing to
query.getSkriuwRecentEvents; also wrap the case body in braces so skriuwLimit is
block-scoped (resolving the noSwitchDeclarations warning) and then call
NextResponse.json(await query.getSkriuwRecentEvents(projectFilter ?? "skriuw",
skriuwLimit, from, to)).
1 participant
Summary
border-border bg-muted/30 text-muted-foregroundtokenshsl(210 100% 50%)blueevents+visitorsschema (with indexes)scripts/sync-vercel-env.shfor Vercel env syncTest plan
DATABASE_URLis set in Vercel for both apps🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Chores