improve: tighten ingestion and SDK types#11
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR removes legacy database tables ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Reviewer's GuideTightens ingestion and SDK typing, removes legacy resume/visitor_events tables from the active schema with a non-destructive archival migration, fixes ORIGIN_ALLOWLIST enforcement, and adds build/runtime safeguards including a safer DB client fallback and preserving "use client" in built SDK outputs. Sequence diagram for ingestion request with origin allowlist and DB fallbacksequenceDiagram
actor Browser
participant SDK as Analytics_SDK
participant Ingestion as Ingestion_handler
participant Env as Process_env
participant DbFactory as Db_client_factory
participant Postgres
Browser->>SDK: track(...)
SDK->>Ingestion: POST /ingest (EventPayload)
Ingestion->>Env: read ORIGIN_ALLOWLIST
Env-->>Ingestion: comma_separated_origins
Ingestion->>Ingestion: getOriginAllowlist()
Ingestion->>Ingestion: isOriginAllowed(request_origin)
alt origin allowed or no allowlist configured
Ingestion->>DbFactory: getDbClient()
DbFactory->>Env: read DATABASE_URL
alt DATABASE_URL present
Env-->>DbFactory: database_url
DbFactory->>Postgres: connect via neon
DbFactory-->>Ingestion: db_client
else DATABASE_URL missing
Env-->>DbFactory: undefined
DbFactory-->>Ingestion: fallback_db_client
end
Ingestion->>Postgres: insert into events
Postgres-->>Ingestion: insert result
Ingestion-->>SDK: 200 OK
else origin not in allowlist
Ingestion-->>SDK: 403 Forbidden
end
ER diagram for updated analytics ingestion schema and archived legacy tableserDiagram
visitors {
bigint id PK
text visitor_id
text project_id
timestamp created_at
}
events {
bigint id PK
text project_id
text type
timestamp ts
jsonb meta
}
old_resume {
bigint id PK
text event
timestamp ts
text path
jsonb meta
}
old_visitor_events {
bigint id PK
bigint visitor_id
text event_type
timestamp ts
jsonb meta
}
visitors ||--o{ events : has_many
visitors ||--o{ old_visitor_events : had_many
Class diagram for updated SDK analytics types and tracking APIclassDiagram
class JsonPrimitive {
}
class JsonValue {
}
class TrackMeta {
}
class KnownEventType {
}
class EventType {
}
class AnalyticsOptions {
projectId string
ingestUrl string
debug boolean
}
class AnalyticsProps {
projectId string
ingestUrl string
debug boolean
disabled boolean
}
class EventPayload {
type EventType
projectId string
path string
referrer string
userAgent string
screen string
viewport string
lang string
visitorId string
sessionId string
meta TrackMeta
}
class AnalyticsComponent {
+Analytics(projectId string, ingestUrl string, disabled boolean, debug boolean)
}
class TrackApi {
+track(type EventType, meta TrackMeta, options AnalyticsOptions) void
+trackPageView(meta TrackMeta, options AnalyticsOptions) void
+trackEvent(eventName string, meta TrackMeta, options AnalyticsOptions) void
+trackClick(elementName string, meta TrackMeta, options AnalyticsOptions) void
+trackError(error Error, meta TrackMeta, options AnalyticsOptions) void
}
JsonPrimitive <|-- JsonValue
JsonValue <|-- TrackMeta
KnownEventType <|-- EventType
AnalyticsOptions <|-- AnalyticsProps
AnalyticsOptions <.. TrackApi
EventPayload o-- TrackMeta
TrackApi --> EventPayload
AnalyticsComponent --> AnalyticsProps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
remcostoeten
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
client-directive.tsscript relies onimport.meta.dirname, which is still relatively environment-specific; consider resolving the dist directory viafileURLToPath(new URL("../dist", import.meta.url))to avoid breakage if the Node/runtime support forimport.meta.dirnamechanges. - The
createFallbackDbimplementation silently turns all DB operations into no-ops whenDATABASE_URLis missing; it may be safer to at least log a clear warning or throw outside build/test contexts so configuration errors don’t get masked at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `client-directive.ts` script relies on `import.meta.dirname`, which is still relatively environment-specific; consider resolving the dist directory via `fileURLToPath(new URL("../dist", import.meta.url))` to avoid breakage if the Node/runtime support for `import.meta.dirname` changes.
- The `createFallbackDb` implementation silently turns all DB operations into no-ops when `DATABASE_URL` is missing; it may be safer to at least log a clear warning or throw outside build/test contexts so configuration errors don’t get masked at runtime.
## Individual Comments
### Comment 1
<location path="packages/sdk/scripts/client-directive.ts" line_range="8" />
<code_context>
+const directive = '"use client";';
+
+for (const file of files) {
+ const path = join(import.meta.dirname, "..", "dist", file);
+ const source = readFileSync(path, "utf8");
+ if (source.startsWith(directive)) continue;
</code_context>
<issue_to_address>
**issue:** Using `import.meta.dirname` is non-standard and will likely cause type/runtime issues.
`import.meta.dirname` isn’t part of the standard `ImportMeta` and isn’t supported by Node’s ESM runtime (which only provides `import.meta.url`), so TypeScript will flag it and the script becomes Bun-specific.
To keep this portable and type-safe, you can derive the directory from `import.meta.url` instead:
```ts
import { fileURLToPath } from "node:url";
const __dirname = fileURLToPath(new URL("..", import.meta.url));
const path = join(__dirname, "dist", file);
```
This stays within standard ESM semantics and avoids custom `ImportMeta` typing.
</issue_to_address>
### Comment 2
<location path="apps/ingestion/src/db/client.ts" line_range="12-21" />
<code_context>
+
+type DbClient = ReturnType<typeof createDb>;
+
+function createFallbackDb(): DbClient {
+ return {
+ select() {
+ return {
+ from() {
+ return [];
+ },
+ };
+ },
+ insert() {
+ return {
+ values() {
+ return {
+ returning() {
+ return [];
+ },
+ };
+ },
+ };
+ },
+ async execute() {
+ return { rows: [] };
+ },
+ } as unknown as DbClient;
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Fallback DB client shape diverges from the real drizzle client and may cause subtle runtime differences.
The fallback client is cast to `DbClient`, but its query methods are synchronous and return plain arrays/objects, while the real drizzle client returns Promises (e.g. `select().from()`, `insert().values().returning()`). This can break code that uses Promise-specific behavior (`.then`, `Promise.all`, assuming async `execute()`). Consider making these methods `async` and returning Promise-based results that mirror drizzle’s shapes to keep the async contract consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const directive = '"use client";'; | ||
|
|
||
| for (const file of files) { | ||
| const path = join(import.meta.dirname, "..", "dist", file); |
There was a problem hiding this comment.
issue: Using import.meta.dirname is non-standard and will likely cause type/runtime issues.
import.meta.dirname isn’t part of the standard ImportMeta and isn’t supported by Node’s ESM runtime (which only provides import.meta.url), so TypeScript will flag it and the script becomes Bun-specific.
To keep this portable and type-safe, you can derive the directory from import.meta.url instead:
import { fileURLToPath } from "node:url";
const __dirname = fileURLToPath(new URL("..", import.meta.url));
const path = join(__dirname, "dist", file);This stays within standard ESM semantics and avoids custom ImportMeta typing.
Comment on lines
+12
to
+21
| function createFallbackDb(): DbClient { | ||
| return { | ||
| select() { | ||
| return { | ||
| from() { | ||
| return []; | ||
| }, | ||
| }; | ||
| }, | ||
| insert() { |
There was a problem hiding this comment.
suggestion (bug_risk): Fallback DB client shape diverges from the real drizzle client and may cause subtle runtime differences.
The fallback client is cast to DbClient, but its query methods are synchronous and return plain arrays/objects, while the real drizzle client returns Promises (e.g. select().from(), insert().values().returning()). This can break code that uses Promise-specific behavior (.then, Promise.all, assuming async execute()). Consider making these methods async and returning Promise-based results that mirror drizzle’s shapes to keep the async contract consistent.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Tightens the analytics ingestion and SDK type surface while cleaning up legacy schema usage.
Changes
ORIGIN_ALLOWLISTso configured allowlists actually reject unlisted origins.resumeandvisitor_eventstables.old_resumeandold_visitor_events.AnalyticsProps, JSON-safeTrackMeta, and custom string event names.anyusage from ingestion and SDK source paths."use client"in built SDK outputs with a post-build helper.distoutput from root formatting checks.Validation
bun run typecheckbun run testbun run lintbun run fmt:checkbun run buildinpackages/sdkDB Migration
Migration file added at
apps/ingestion/src/db/migrations/0001_archive_legacy_tables.sql. It renames existing legacy tables only if present and does not drop data.Summary by Sourcery
Tighten ingestion and SDK typing while removing legacy database usage and preserving client runtime behavior in the built SDK.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores:
Summary by CodeRabbit
Bug Fixes
Refactor