554 snowplow web destination #585
Conversation
📝 WalkthroughWalkthroughThis PR introduces unified dynamic pattern syntax ($var, $env, $def, $code) for flow configuration, refactors collector queue lifecycle to queue on() events until init completes, adds complete Snowplow destination integration with dual tracker modes, enhances CLI bundling with implicit collector injection and inline code serialization, updates core pattern resolution, and bumps package versions across the monorepo. Changes
Sequence DiagramssequenceDiagram
participant CLI as CLI Bundler
participant Flow as Flow Config
participant Resolver as Pattern Resolver
participant Serializer as Code Serializer
participant Imports as Import Generator
CLI->>Flow: Load flow.json
Flow->>Resolver: Detect $var, $env, $def, $code patterns
Resolver->>Resolver: Recursively resolve patterns<br/>(defs → vars → env vars)
Resolver-->>Serializer: Resolved values
Serializer->>Serializer: Serialize with $code support<br/>(emit raw JS for $code:)
Serializer-->>Imports: Config + code references
Imports->>Imports: Auto-detect default exports<br/>Generate named imports<br/>Inject collector if needed<br/>Add startFlow for collector
Imports-->>CLI: Bundled entry point
sequenceDiagram
participant App as Application
participant Collector as Collector Init
participant Dest as Destination
participant Queue as Event Queue
participant On as Lifecycle Handler
App->>Collector: Start flow with destinations
Collector->>Queue: Initialize queuePush (event queue)
Collector->>Queue: Initialize queueOn (lifecycle queue)
App->>On: Call destination.on('consent')
On->>Queue: Queue event (queueOn)<br/>dest not initialized yet
Collector->>Dest: Call destinationInit()
Dest-->>Collector: init() completes
Collector->>Queue: Flush queueOn events
Collector->>On: callDestinationOn for each queued
On-->>App: Lifecycle events delivered
App->>Dest: Push regular events
Dest->>Dest: process via adapter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview deployed |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/commands/push/index.ts (1)
135-161:process.exit(0)bypasses thefinallyblock, leaving temp directories uncleaned.Calling
process.exit(0)on line 138 terminates the process immediately, preventing thefinallyblock (lines 154-161) from executing. This means temp directories created during successful pushes will never be cleaned up, potentially causing disk space issues over time.Consider cleaning up before exiting:
Proposed fix
// Explicit exit on success to avoid hanging from open handles // (JSDOM instances, esbuild workers, HTTP connections, etc.) + if (tempDir) { + await fs.remove(tempDir).catch(() => { + // Ignore cleanup errors + }); + } process.exit(0);packages/cli/src/commands/bundle/bundler.ts (2)
79-137: Cache key is computed before implicit collector injection.Because the collector is added after the cache lookup, implicit-collector flows won’t hit the cache and will store under a different key. Normalize packages before cache key generation (or include the auto-add in
generateCacheKeyContent) so cache hits work.🧩 Suggested reordering
- // Check build cache if caching is enabled - if (buildOptions.cache !== false) { - const configContent = generateCacheKeyContent(flowConfig, buildOptions); - ... - } - - try { - // Step 1: Ensure temporary directory exists - await fs.ensureDir(TEMP_DIR); - - // Step 1.5: Auto-add collector if sources/destinations exist but collector not specified - const hasSourcesOrDests = ... - if (hasSourcesOrDests && !buildOptions.packages['@walkeros/collector']) { - buildOptions.packages['@walkeros/collector'] = {}; - } + // Auto-add collector before cache key generation + const hasSourcesOrDests = ... + if (hasSourcesOrDests && !buildOptions.packages['@walkeros/collector']) { + buildOptions.packages['@walkeros/collector'] = {}; + } + + // Check build cache if caching is enabled + if (buildOptions.cache !== false) { + const configContent = generateCacheKeyContent(flowConfig, buildOptions); + ... + } + + try { + // Step 1: Ensure temporary directory exists + await fs.ensureDir(TEMP_DIR);
557-625: Mixed explicit + implicit usage of a package can drop the default import.When any explicit code exists for a package,
generateImportStatementsskips the default import entirely. If another source/destination omitscode,buildConfigObjectstill references the default import variable, leading to a runtimeReferenceError. Track which packages need a default import per usage and emit it whenever any usage is implicit.✅ One way to preserve default imports when needed
+function detectPackagesNeedingDefaultImport(flowConfig: Flow.Config): Set<string> { + const needsDefault = new Set<string>(); + const sources = + (flowConfig as { sources?: Record<string, { package?: string; code?: string }> }) + .sources || {}; + const destinations = + (flowConfig as { + destinations?: Record<string, { package?: string; code?: string }>; + }).destinations || {}; + + for (const cfg of [...Object.values(sources), ...Object.values(destinations)]) { + if (cfg?.package && (!cfg.code || cfg.code.startsWith('_'))) { + needsDefault.add(cfg.package); + } + } + return needsDefault; +} @@ - const { importStatements } = generateImportStatements( + const defaultImportPackages = detectPackagesNeedingDefaultImport(flowConfig); + const { importStatements } = generateImportStatements( buildOptions.packages, destinationPackages, sourcePackages, explicitCodeImports, + defaultImportPackages, ); @@ -function generateImportStatements( +function generateImportStatements( packages: BuildOptions['packages'], destinationPackages: Set<string>, sourcePackages: Set<string>, explicitCodeImports: Map<string, Set<string>>, + defaultImportPackages: Set<string>, ): ImportGenerationResult { @@ - if (isUsedByDestOrSource && !hasExplicitCode) { + if (defaultImportPackages.has(packageName)) { const varName = packageNameToVariable(packageName); importStatements.push(`import ${varName} from '${packageName}';`); }
🤖 Fix all issues with AI agents
In `@apps/quickstart/CHANGELOG.md`:
- Around line 7-10: Remove the duplicate "Updated dependencies [b65b773]" entry
in the changelog so it only appears once; locate the repeated line string
"Updated dependencies [b65b773]" (the two identical entries surrounding the
"@walkeros/collector@1.0.1" bullet) and delete the redundant occurrence so the
changelog lists each update reference only once.
In `@packages/cli/CHANGELOG.md`:
- Around line 19-25: The example in the CHANGELOG shows a semicolon after the
arrow function inside the object literal (property "fn"), which is a syntax nit;
edit the example so the object property uses no trailing semicolon (remove the
semicolon after the arrow function in the "fn" property) to produce a valid
JavaScript object literal in the docs.
In `@packages/cli/src/commands/bundle/bundler.ts`:
- Around line 222-225: Remove the invalid call to esbuild.stop() in the finally
block of the bundler logic: delete the await esbuild.stop() line (esbuild.stop()
does not exist in the Node.js API) and leave no explicit cleanup for single-shot
esbuild.build() calls; if you later add watch/serve behavior use the proper APIs
(context.dispose() for context/watch or server.stop() for serve) instead of
esbuild.stop().
In `@packages/collector/src/destination.ts`:
- Line 146: The call to concat on destination.queuePush discards the result so
skippedEvents never get added; update the code that currently calls
destination.queuePush.concat(skippedEvents) to actually merge the arrays (e.g.,
replace with an assignment destination.queuePush =
destination.queuePush.concat(skippedEvents) or use
destination.queuePush.push(...skippedEvents)) so skippedEvents are appended to
destination.queuePush.
In `@packages/collector/src/on.ts`:
- Around line 38-59: callDestinationOn builds a Destination.Context but omits
the required setIngest field so on() handlers get an undefined or fail
type-checking; update callDestinationOn to include context.setIngest by wiring
the same setIngest function used in the destination push context (i.e., source
it from the collector or the shared context builder used for destination pushes)
so Destination.Context in callDestinationOn (used by callDestinationOn,
destination.on, mergeEnvironments, tryCatch) matches the type in packages/core
and provides the correct setIngest implementation to handlers.
- Around line 107-117: The current logic queues on-events whenever
destination.config.init is falsy, causing permanent queuing for destinations
that implement on() but have no init(); change the gate to check the actual
presence/state of an init step: use destination.init (or set
destination.config.init = true when destination.init is undefined) before
pushing to destination.queueOn so only destinations with a pending init get
queued; locate the logic in the loop over collector.destinations (symbols:
collector.destinations, destination.on, destination.config.init,
destination.init, destination.queueOn, callDestinationOn) and update it so
non-init destinations call callDestinationOn immediately while only destinations
with a defined/pending init have their on-events enqueued.
In `@packages/core/src/flow.ts`:
- Around line 69-78: Add an exact-match branch before the inline replacement to
preserve non-string types: test value against the full-match regex
/^\$var\.([a-zA-Z_][a-zA-Z0-9_]*)$/ and if it matches, return variables[name]
directly (or call throwError if undefined) instead of stringifying; otherwise
keep the existing value.replace(...) inline substitution logic that uses
String(variables[name]) and throwError for missing variables. Use the same
capture group/name logic as in the current /\$var\.([a-zA-Z_][a-zA-Z0-9_]*)/g
code and keep references to value, variables, throwError and the current
replacement behavior.
- Around line 58-67: The code in resolvePatterns currently follows $def
references (defMatch / defName) without cycle detection, so add a tracking set
or stack (e.g., pass a visitedDefs Set<string> or resolutionStack) into
resolvePatterns and check it before recursing: if defName is already in the
set/stack, throw a clear error like 'Cyclic definition reference: <defName>';
otherwise add defName, recurse to resolvePatterns(definitions[defName],
variables, definitions, visitedDefs), and remove defName after recursion
completes to prevent false positives. Ensure all call sites of resolvePatterns
are updated to propagate the visited set (initializing it empty at the top
level).
In `@packages/transformers/validator/package.json`:
- Line 4: The package.json in the validator package was bumped to "2.0.0"
without any breaking changes; change the "version" field in
packages/transformers/validator/package.json from "2.0.0" to "1.1.0" (or "1.0.1"
if you prefer patch) to reflect the dependency-only update, and adjust the
CHANGELOG entry to match the chosen semantic version so the release metadata
remains consistent with the actual changes.
In `@packages/web/destinations/snowplow/package.json`:
- Around line 40-45: Replace the two devDependencies that use the floating
"latest" tag—@walkeros/collector and `@walkeros/config`—in package.json with
explicit version ranges (e.g., ^x.y.z) to ensure reproducible installs; update
the entries for "@walkeros/collector" and "@walkeros/config" to the chosen
pinned versions, run npm install to refresh package-lock.json/workspaces, and
ensure the same pinned versions are used consistently across other destination
packages.
In `@packages/web/destinations/snowplow/README.md`:
- Around line 369-405: Replace the bold inline "headings" like "**Option 1:
Auto-track on init**", "**Option 2: Track via walkerOS event**", and "**Option
3: Custom event name**" with proper Markdown headings (e.g., "### Option 1:
Auto-track on init") in the README content to satisfy MD036; update the three
heading lines so they use actual heading syntax and keep the surrounding code
blocks and examples unchanged (look for the exact text "Option 1: Auto-track on
init", "Option 2: Track via walkerOS event", and "Option 3: Custom event name"
to locate the replacements).
- Around line 1076-1080: Replace the bare URL in the README step "4. **Inspect
events**" by wrapping it as a Markdown link to satisfy MD034: update the line
under "- **Web UI**" that currently reads "Open http://localhost:9090/micro/ui
in your browser" to use the link form, e.g. "Open
[http://localhost:9090/micro/ui](http://localhost:9090/micro/ui) in your
browser".
In `@packages/web/destinations/snowplow/src/examples/mapping.ts`:
- Around line 93-102: The is_guest mapping under SCHEMAS.USER currently returns
true when user.id exists (inverted); update the fn for is_guest to return false
when (event as { user?: { id?: string } }).user?.id is present and true (or
undefined when unknown), and return true when no user.id (anonymous); modify the
anonymous arrow function inside the is_guest mapping to invert its boolean logic
accordingly.
In `@packages/web/destinations/snowplow/src/examples/walkeros-events.ts`:
- Around line 125-182: The transaction example functions transaction and
transactionMultipleProducts use data.revenue and data.tax but the Snowplow
mapping expects data.total and data.taxes; update those event payloads in
getEvent calls (inside transaction and transactionMultipleProducts) to rename
revenue -> total and tax -> taxes (and corresponding comments/examples) so the
example fields match the Snowplow mapping rules.
In `@packages/web/destinations/snowplow/src/index.ts`:
- Around line 191-193: The check for the required collectorUrl calls
logger.throw('Config settings collectorUrl missing') but doesn't stop execution;
update the code handling for the collectorUrl validation so the function returns
immediately after logger.throw (or replace logger.throw with an actual thrown
Error) to prevent proceeding with an undefined collectorUrl; locate the
collectorUrl validation and ensure either a return statement follows the
logger.throw call or throw a new Error there so subsequent uses of collectorUrl
(e.g., where non-null assertions are used) are safe.
In `@packages/web/destinations/snowplow/src/setup.ts`:
- Around line 20-34: The dedupe check in addScript currently uses collectorUrl
but should use the script source URL; update the function (addScript) to check
loadedScripts.has(src) (or script.src after resolving DEFAULT_SCRIPT_URL) and to
add src (or script.src) to loadedScripts after appending the element, so
duplicate script src values are prevented regardless of collectorUrl; keep the
rest of the logic (getEnv/document manipulation, async/script insertion)
unchanged.
In `@website/docs/destinations/server/meta-capi.mdx`:
- Around line 103-105: The value for "test_event_code" is using the $env pattern
but has an unintended trailing colon; update the "test_event_code" entry (the
"test_event_code" key referencing $env.META_TEST_CODE) to either remove the
trailing ':' so it matches other entries or provide a sensible default after the
colon (e.g., $env.META_TEST_CODE:defaultValue) so the env fallback pattern is
consistent.
🧹 Nitpick comments (12)
package.json (1)
48-48: Consider updating the version constraint for consistency.The devDependency specifies
^1.0.1, butpackages/config/package.jsonis being bumped to1.0.2. While semver caret will accept1.0.2, updating to^1.0.2would provide clearer alignment with the actual version being released.Suggested change
- "@walkeros/config": "^1.0.1", + "@walkeros/config": "^1.0.2",packages/web/destinations/plausible/CHANGELOG.md (1)
3-7: Minor formatting inconsistency in changelog entry.The dependency update entry on line 7 uses a simpler format compared to other changelogs in this PR (e.g.,
packages/server/sources/fetch/CHANGELOG.mduses"Updated dependencies [commit_hash]"format). Consider using the same format for consistency across the monorepo:### Patch Changes -- `@walkeros/web-core`@1.0.1 +- Updated dependencies [commit_hash] + - `@walkeros/web-core`@1.0.1packages/web/destinations/meta/CHANGELOG.md (1)
3-7: Minor formatting inconsistency.This changelog entry differs from other changelogs in this PR (e.g.,
packages/server/sources/aws/CHANGELOG.md) which use the format:- Updated dependencies [commit_hash] - `@walkeros/package`@versionConsider aligning with the consistent format used elsewhere for easier tracking of changes.
packages/cli/src/commands/simulate/index.ts (1)
99-101: Typestring | unknownsimplifies tounknown.The union
string | unknownis redundant sinceunknownis a supertype of all types, making thestring |part meaningless. If the intent is to accept either a string path or an object config, consider using a more specific union type.Suggested fix
export async function simulate( - configOrPath: string | unknown, + configOrPath: string | Record<string, unknown>, event: unknown,Or if only string paths are currently supported (as the implementation suggests):
export async function simulate( - configOrPath: string | unknown, + configOrPath: string, event: unknown,packages/collector/src/__tests__/on-queue.test.ts (1)
69-108: Assert replay order explicitly to match the test name.
Right now it only checkscontains, not ordering.Proposed assertion tightening
- // Queued events come after init - expect(callOrder.slice(1)).toContain('on:consent'); + // Queued events come after init and keep order + const runIdx = callOrder.indexOf('on:run'); + const consentIdx = callOrder.indexOf('on:consent'); + expect(runIdx).toBeGreaterThan(0); + expect(consentIdx).toBeGreaterThan(runIdx);packages/web/destinations/snowplow/src/schemas/mapping.ts (1)
8-13: Consider tightening thedataschema.The
datafield usesz.record(z.string(), z.unknown())which is very permissive. The TypeScript interface showsdata: WalkerOSMapping.Mapwhich may have more specific structure. Ifunknownis intentional for flexibility, this is fine.packages/web/destinations/snowplow/src/adapter.ts (1)
170-173: Silent no-op may surprise users.The
call()method silently does nothing in browser-tracker mode. Consider logging a warning when called, so users understand why their URL-based plugin enable calls aren't working.💡 Suggested improvement
call(method: string, ...args: unknown[]) { // In browser-tracker mode, arbitrary method calls are not supported // URL-based plugins that need enable methods won't work here + console.warn( + `[snowplow] call('${method}') not supported in browser-tracker mode`, + ); },packages/web/destinations/snowplow/src/setup.ts (2)
36-40: Duplicate interface definition.
SnowplowFunctionis already exported from./types/index.ts(lines 57-60). Consider importing it to avoid definition drift.♻️ Proposed refactor
-// Snowplow function interface -interface SnowplowFunction { - (...args: unknown[]): void; - q?: unknown[]; -} +import type { SnowplowFunction } from './types';
10-11: AddscriptUrlto SettingsSchema to allow users to override the tracker script URL.The code includes a helpful warning comment about using
@latest, but the override mechanism isn't exposed to users through the configuration interface. TheaddScriptfunction accepts a customsrcparameter, but users would need direct function access to use it. Consider adding ascriptUrlfield toSettingsSchemaso users can pin a specific version without modifying code.packages/web/destinations/snowplow/src/push.ts (1)
170-191: Consider adding type validation for loop expansion values.The casts on lines 174 and 181 assume
scopeis a string anditemMappingis valid mapping data, butisLoopContextDataonly validates the structure has alooparray with 2 elements. If malformed data is provided, this could fail silently or cause unexpected behavior ingetMappingValue.💡 Optional: Add defensive type checks
if (isLoopContextData(contextDef.data)) { const [scope, itemMapping] = contextDef.data.loop; + // Validate scope is a string path + if (!isString(scope)) { + continue; + } + // Get the source array using getMappingValue with the scope - const sourceArray = await getMappingValue(event, scope as string); + const sourceArray = await getMappingValue(event, scope);packages/web/destinations/snowplow/src/schemas/settings.ts (1)
144-158: Usage ofz.any()violates coding guidelines.Per the coding guidelines,
anytype should not be used in production code. Lines 146 and 155 usez.any()which defeats runtime validation and type safety.Consider defining more specific schemas for these complex types:
♻️ Suggested improvement for plugins schema
// Plugins - validated loosely since BrowserPlugin is complex plugins: z - .array(z.union([UrlBasedPluginSchema, z.any()])) + .array(z.union([ + UrlBasedPluginSchema, + z.object({ code: z.unknown(), config: z.record(z.string(), z.unknown()).optional() }), + z.unknown() // BrowserPlugin instances - can't validate at runtime + ])) .optional() .describe('Snowplow plugins'),Note: While
z.unknown()is also permissive, it's safer thanz.any()as it requires explicit type narrowing when used. Based on coding guidelines.packages/web/destinations/snowplow/src/types/index.ts (1)
403-412: Documentation says "Required" but type is optional.The
collectorUrlproperty comment states "Required" but the type isstring | undefined. This is intentional to allow runtime validation, but could be confusing. Consider using a clearer pattern.💡 Optional: Clarify required vs optional distinction
/** * Snowplow collector endpoint URL * - * Required. The URL of your Snowplow collector. + * The URL of your Snowplow collector. + * This is required at runtime but optional in the type to enable + * partial configuration and runtime validation. * * `@example` "https://collector.example.com" */ collectorUrl?: string;
| - Updated dependencies [b65b773] | ||
| - Updated dependencies [20eca6e] | ||
| - Updated dependencies [b65b773] | ||
| - @walkeros/collector@1.0.1 |
There was a problem hiding this comment.
Remove duplicate “Updated dependencies” entry.
Line 7 and Line 9 repeat the same commit reference, which reads like an accidental duplication.
✅ Suggested fix
- - Updated dependencies [b65b773]
- - Updated dependencies [20eca6e]
- - Updated dependencies [b65b773]
+ - Updated dependencies [b65b773]
+ - Updated dependencies [20eca6e]📝 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.
| - Updated dependencies [b65b773] | |
| - Updated dependencies [20eca6e] | |
| - Updated dependencies [b65b773] | |
| - @walkeros/collector@1.0.1 | |
| - Updated dependencies [b65b773] | |
| - Updated dependencies [20eca6e] | |
| - `@walkeros/collector`@1.0.1 |
🤖 Prompt for AI Agents
In `@apps/quickstart/CHANGELOG.md` around lines 7 - 10, Remove the duplicate
"Updated dependencies [b65b773]" entry in the changelog so it only appears once;
locate the repeated line string "Updated dependencies [b65b773]" (the two
identical entries surrounding the "@walkeros/collector@1.0.1" bullet) and delete
the redundant occurrence so the changelog lists each update reference only once.
| Outputs: | ||
|
|
||
| ```javascript | ||
| { | ||
| fn: (value) => value.toUpperCase(); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Minor syntax error in example output.
The example shows a semicolon after the function inside the object literal, but object properties should be separated by commas. This is a documentation nit.
Suggested fix
Outputs:
```javascript
{
- fn: (value) => value.toUpperCase();
+ fn: (value) => value.toUpperCase()
}</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Outputs:
🤖 Prompt for AI Agents
In `@packages/cli/CHANGELOG.md` around lines 19 - 25, The example in the CHANGELOG
shows a semicolon after the arrow function inside the object literal (property
"fn"), which is a syntax nit; edit the example so the object property uses no
trailing semicolon (remove the semicolon after the arrow function in the "fn"
property) to produce a valid JavaScript object literal in the docs.
| } finally { | ||
| // Clean up esbuild worker threads to allow process to exit | ||
| await esbuild.stop(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check esbuild version in manifests/lockfiles
rg -n -C2 '"esbuild"' --glob 'package.json' --glob '*lock*'Repository: elbwalker/walkerOS
Length of output: 9597
🌐 Web query:
esbuild stop() API documentation version support
💡 Result:
stop() is not a general esbuild JS API method for the Node/npm package; in Node you typically end long‑running activity by disposing a build context (e.g. ctx.dispose() for watch/serve) instead. [1]
Where stop() is documented/supported:
- Deno wrapper (
deno.land/x/esbuild): The Deno package addsawait esbuild.stop()because Deno won’t exit while esbuild’s internal child process is still running; the docs explicitly say you should call it when you’re done. [2] - Version note (Deno
stop()): By esbuild v0.11.19, callingstop()no longer permanently prevents further use; esbuild can be used again afterstop()(it will restart internally, and you’ll need to callstop()again before exit). [3]
Common confusion:
server.stop()exists on the object returned by the serve API (to stop the dev server), but that’s different fromesbuild.stop(). [4]
Sources: esbuild API docs [1], esbuild “Getting Started” (Deno section) [2], esbuild changelog entry for 0.11.19 [3], serve API example showing server.stop() [4].
🏁 Script executed:
# Check how esbuild is used in bundler.ts
cat packages/cli/src/commands/bundle/bundler.tsRepository: elbwalker/walkerOS
Length of output: 28863
Remove the await esbuild.stop() call—it does not exist in the esbuild Node.js API.
The esbuild npm package (v0.27.1) does not provide a stop() method. The stop() API exists only in the Deno wrapper. For Node.js, esbuild.build() requires no explicit cleanup; the promise resolves after the build completes. If you later add watch mode or serve mode, use context.dispose() or server.stop() respectively, but for single-shot builds, simply remove this line to prevent a runtime error.
🤖 Prompt for AI Agents
In `@packages/cli/src/commands/bundle/bundler.ts` around lines 222 - 225, Remove
the invalid call to esbuild.stop() in the finally block of the bundler logic:
delete the await esbuild.stop() line (esbuild.stop() does not exist in the
Node.js API) and leave no explicit cleanup for single-shot esbuild.build()
calls; if you later add watch/serve behavior use the proper APIs
(context.dispose() for context/watch or server.stop() for serve) instead of
esbuild.stop().
|
|
||
| // Add skipped events back to the queue | ||
| destination.queue.concat(skippedEvents); | ||
| destination.queuePush.concat(skippedEvents); |
There was a problem hiding this comment.
Bug: concat() result is discarded.
concat() returns a new array and does not mutate the original. The skipped events are never actually added back to the queue.
🐛 Proposed fix
// Add skipped events back to the queue
- destination.queuePush.concat(skippedEvents);
+ destination.queuePush.push(...skippedEvents);📝 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.
| destination.queuePush.concat(skippedEvents); | |
| destination.queuePush.push(...skippedEvents); |
🤖 Prompt for AI Agents
In `@packages/collector/src/destination.ts` at line 146, The call to concat on
destination.queuePush discards the result so skippedEvents never get added;
update the code that currently calls destination.queuePush.concat(skippedEvents)
to actually merge the arrays (e.g., replace with an assignment
destination.queuePush = destination.queuePush.concat(skippedEvents) or use
destination.queuePush.push(...skippedEvents)) so skippedEvents are appended to
destination.queuePush.
| export function callDestinationOn( | ||
| collector: Collector.Instance, | ||
| destination: Destination.Instance, | ||
| destId: string, | ||
| type: On.Types, | ||
| data: unknown, | ||
| ) { | ||
| if (!destination.on) return; | ||
|
|
||
| const destType = destination.type || 'unknown'; | ||
| const destLogger = collector.logger.scope(destType).scope('on').scope(type); | ||
|
|
||
| const context: Destination.Context = { | ||
| collector, | ||
| logger: destLogger, | ||
| id: destId, | ||
| config: destination.config, | ||
| data: data as Destination.Data, | ||
| env: mergeEnvironments(destination.env, destination.config.env), | ||
| }; | ||
|
|
||
| tryCatch(destination.on)(type, context); |
There was a problem hiding this comment.
Destination.Context is missing setIngest.
Destination.Context includes setIngest (see packages/core/src/types/destination.ts), but the new helper doesn’t supply it. This will either fail type-checking or leave context.setIngest undefined for on() handlers that rely on it. Please wire it in (ideally via the same context builder used for destination pushes).
🤖 Prompt for AI Agents
In `@packages/collector/src/on.ts` around lines 38 - 59, callDestinationOn builds
a Destination.Context but omits the required setIngest field so on() handlers
get an undefined or fail type-checking; update callDestinationOn to include
context.setIngest by wiring the same setIngest function used in the destination
push context (i.e., source it from the collector or the shared context builder
used for destination pushes) so Destination.Context in callDestinationOn (used
by callDestinationOn, destination.on, mergeEnvironments, tryCatch) matches the
type in packages/core and provides the correct setIngest implementation to
handlers.
| // User entity - is_guest: true when user.id exists | ||
| { | ||
| schema: SCHEMAS.USER, | ||
| data: { | ||
| id: 'user.id', | ||
| email: 'user.email', | ||
| is_guest: { | ||
| fn: (event) => | ||
| (event as { user?: { id?: string } }).user?.id ? true : undefined, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Snowplow ecommerce user context is_guest field definition
💡 Result:
In Snowplow’s Ecommerce User context/entity (iglu:com.snowplowanalytics.snowplow.ecommerce/user/jsonschema/1-0-0), the field:
is_guest(boolean, optional): Whether or not the user is a guest (i.e., a guest/anonymous checkout rather than a logged-in/registered user). [1][2]
Sources: Snowplow “Ecommerce events and entities” docs [1] and the web tracker ecommerce plugin docs for setEcommerceUser({ id, is_guest, email }) [2].
[1] (docs.snowplow.io)
[2] (docs.snowplow.io)
Citations:
- 1: https://docs.snowplow.io/docs/events/ootb-data/ecommerce-events/?utm_source=openai
- 2: https://docs.snowplow.io/docs/sources/web-trackers/tracking-events/ecommerce/?utm_source=openai
Fix inverted is_guest calculation.
The logic is inverted: when user.id exists (indicating a logged-in user), the code returns true, marking them as a guest. According to Snowplow's ecommerce user context schema, is_guest should be true for anonymous users and false for authenticated users. Invert the logic so false is returned when user.id is present.
Suggested fix
data: {
id: 'user.id',
email: 'user.email',
is_guest: {
fn: (event) =>
- (event as { user?: { id?: string } }).user?.id ? true : undefined,
+ (event as { user?: { id?: string } }).user?.id ? false : true,
},
},📝 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.
| // User entity - is_guest: true when user.id exists | |
| { | |
| schema: SCHEMAS.USER, | |
| data: { | |
| id: 'user.id', | |
| email: 'user.email', | |
| is_guest: { | |
| fn: (event) => | |
| (event as { user?: { id?: string } }).user?.id ? true : undefined, | |
| }, | |
| // User entity - is_guest: true when user.id exists | |
| { | |
| schema: SCHEMAS.USER, | |
| data: { | |
| id: 'user.id', | |
| email: 'user.email', | |
| is_guest: { | |
| fn: (event) => | |
| (event as { user?: { id?: string } }).user?.id ? false : true, | |
| }, |
🤖 Prompt for AI Agents
In `@packages/web/destinations/snowplow/src/examples/mapping.ts` around lines 93 -
102, The is_guest mapping under SCHEMAS.USER currently returns true when user.id
exists (inverted); update the fn for is_guest to return false when (event as {
user?: { id?: string } }).user?.id is present and true (or undefined when
unknown), and return true when no user.id (anonymous); modify the anonymous
arrow function inside the is_guest mapping to invert its boolean logic
accordingly.
| export function transaction(): WalkerOS.Event { | ||
| return getEvent('order complete', { | ||
| data: { | ||
| id: 'ORD-123', | ||
| revenue: 999, | ||
| currency: 'USD', | ||
| payment_method: 'credit_card', | ||
| total_quantity: 1, | ||
| tax: 80, | ||
| shipping: 10, | ||
| }, | ||
| nested: [ | ||
| { | ||
| entity: 'product', | ||
| data: { | ||
| id: 'P123', | ||
| name: 'Laptop', | ||
| category: 'Electronics', | ||
| price: 999, | ||
| currency: 'USD', | ||
| quantity: 1, | ||
| }, | ||
| nested: [], | ||
| }, | ||
| ], | ||
| context: { shopping: ['complete', 0] }, | ||
| globals: { pagegroup: 'shop' }, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Transaction with Multiple Products | ||
| * walkerOS: elb('order complete', { | ||
| * id: 'ORD-456', | ||
| * revenue: 1097, | ||
| * tax: 120, | ||
| * shipping: 15, | ||
| * currency: 'USD', | ||
| * payment_method: 'paypal' | ||
| * }, { | ||
| * nested: [ | ||
| * { type: 'product', data: { id: 'P123', name: 'Laptop', price: 999, quantity: 1 } }, | ||
| * { type: 'product', data: { id: 'P456', name: 'Mouse', price: 49, quantity: 2 } } | ||
| * ] | ||
| * }) | ||
| * Generated when order with multiple products is completed | ||
| */ | ||
| export function transactionMultipleProducts(): WalkerOS.Event { | ||
| return getEvent('order complete', { | ||
| data: { | ||
| id: 'ORD-456', | ||
| revenue: 1097, | ||
| currency: 'USD', | ||
| payment_method: 'paypal', | ||
| total_quantity: 3, | ||
| tax: 120, | ||
| shipping: 15, | ||
| }, |
There was a problem hiding this comment.
Align transaction example fields with mapping expectations.
Line 129-135 and Line 175-181 set revenue/tax, but the Snowplow mapping rules resolve data.total/data.taxes, so these examples won’t reflect the values you set. Consider renaming the fields (or update the mapping if revenue/tax is the canonical shape).
✅ Suggested fix
export function transaction(): WalkerOS.Event {
return getEvent('order complete', {
data: {
id: 'ORD-123',
- revenue: 999,
+ total: 999,
currency: 'USD',
payment_method: 'credit_card',
total_quantity: 1,
- tax: 80,
+ taxes: 80,
shipping: 10,
}, export function transactionMultipleProducts(): WalkerOS.Event {
return getEvent('order complete', {
data: {
id: 'ORD-456',
- revenue: 1097,
+ total: 1097,
currency: 'USD',
payment_method: 'paypal',
total_quantity: 3,
- tax: 120,
+ taxes: 120,
shipping: 15,
},📝 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.
| export function transaction(): WalkerOS.Event { | |
| return getEvent('order complete', { | |
| data: { | |
| id: 'ORD-123', | |
| revenue: 999, | |
| currency: 'USD', | |
| payment_method: 'credit_card', | |
| total_quantity: 1, | |
| tax: 80, | |
| shipping: 10, | |
| }, | |
| nested: [ | |
| { | |
| entity: 'product', | |
| data: { | |
| id: 'P123', | |
| name: 'Laptop', | |
| category: 'Electronics', | |
| price: 999, | |
| currency: 'USD', | |
| quantity: 1, | |
| }, | |
| nested: [], | |
| }, | |
| ], | |
| context: { shopping: ['complete', 0] }, | |
| globals: { pagegroup: 'shop' }, | |
| }); | |
| } | |
| /** | |
| * Transaction with Multiple Products | |
| * walkerOS: elb('order complete', { | |
| * id: 'ORD-456', | |
| * revenue: 1097, | |
| * tax: 120, | |
| * shipping: 15, | |
| * currency: 'USD', | |
| * payment_method: 'paypal' | |
| * }, { | |
| * nested: [ | |
| * { type: 'product', data: { id: 'P123', name: 'Laptop', price: 999, quantity: 1 } }, | |
| * { type: 'product', data: { id: 'P456', name: 'Mouse', price: 49, quantity: 2 } } | |
| * ] | |
| * }) | |
| * Generated when order with multiple products is completed | |
| */ | |
| export function transactionMultipleProducts(): WalkerOS.Event { | |
| return getEvent('order complete', { | |
| data: { | |
| id: 'ORD-456', | |
| revenue: 1097, | |
| currency: 'USD', | |
| payment_method: 'paypal', | |
| total_quantity: 3, | |
| tax: 120, | |
| shipping: 15, | |
| }, | |
| export function transaction(): WalkerOS.Event { | |
| return getEvent('order complete', { | |
| data: { | |
| id: 'ORD-123', | |
| total: 999, | |
| currency: 'USD', | |
| payment_method: 'credit_card', | |
| total_quantity: 1, | |
| taxes: 80, | |
| shipping: 10, | |
| }, | |
| nested: [ | |
| { | |
| entity: 'product', | |
| data: { | |
| id: 'P123', | |
| name: 'Laptop', | |
| category: 'Electronics', | |
| price: 999, | |
| currency: 'USD', | |
| quantity: 1, | |
| }, | |
| nested: [], | |
| }, | |
| ], | |
| context: { shopping: ['complete', 0] }, | |
| globals: { pagegroup: 'shop' }, | |
| }); | |
| } | |
| /** | |
| * Transaction with Multiple Products | |
| * walkerOS: elb('order complete', { | |
| * id: 'ORD-456', | |
| * total: 1097, | |
| * taxes: 120, | |
| * shipping: 15, | |
| * currency: 'USD', | |
| * payment_method: 'paypal' | |
| * }, { | |
| * nested: [ | |
| * { type: 'product', data: { id: 'P123', name: 'Laptop', price: 999, quantity: 1 } }, | |
| * { type: 'product', data: { id: 'P456', name: 'Mouse', price: 49, quantity: 2 } } | |
| * ] | |
| * }) | |
| * Generated when order with multiple products is completed | |
| */ | |
| export function transactionMultipleProducts(): WalkerOS.Event { | |
| return getEvent('order complete', { | |
| data: { | |
| id: 'ORD-456', | |
| total: 1097, | |
| currency: 'USD', | |
| payment_method: 'paypal', | |
| total_quantity: 3, | |
| taxes: 120, | |
| shipping: 15, | |
| }, |
🤖 Prompt for AI Agents
In `@packages/web/destinations/snowplow/src/examples/walkeros-events.ts` around
lines 125 - 182, The transaction example functions transaction and
transactionMultipleProducts use data.revenue and data.tax but the Snowplow
mapping expects data.total and data.taxes; update those event payloads in
getEvent calls (inside transaction and transactionMultipleProducts) to rename
revenue -> total and tax -> taxes (and corresponding comments/examples) so the
example fields match the Snowplow mapping rules.
| // Required collector URL | ||
| if (!collectorUrl) logger.throw('Config settings collectorUrl missing'); | ||
|
|
There was a problem hiding this comment.
Missing early return after logger.throw for required collectorUrl.
After calling logger.throw on line 192, the function continues execution. If logger.throw doesn't actually throw an exception, collectorUrl could still be undefined, leading to the non-null assertions on lines 210, 225, and 232 being invalid.
🐛 Proposed fix
// Required collector URL
- if (!collectorUrl) logger.throw('Config settings collectorUrl missing');
+ if (!collectorUrl) {
+ logger.throw('Config settings collectorUrl missing');
+ return false;
+ }📝 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.
| // Required collector URL | |
| if (!collectorUrl) logger.throw('Config settings collectorUrl missing'); | |
| // Required collector URL | |
| if (!collectorUrl) { | |
| logger.throw('Config settings collectorUrl missing'); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/destinations/snowplow/src/index.ts` around lines 191 - 193, The
check for the required collectorUrl calls logger.throw('Config settings
collectorUrl missing') but doesn't stop execution; update the code handling for
the collectorUrl validation so the function returns immediately after
logger.throw (or replace logger.throw with an actual thrown Error) to prevent
proceeding with an undefined collectorUrl; locate the collectorUrl validation
and ensure either a return statement follows the logger.throw call or throw a
new Error there so subsequent uses of collectorUrl (e.g., where non-null
assertions are used) are safe.
| export function addScript( | ||
| collectorUrl: string, | ||
| env?: DestinationWeb.Env, | ||
| src = DEFAULT_SCRIPT_URL, | ||
| ): void { | ||
| // Prevent loading the same script multiple times | ||
| if (loadedScripts.has(collectorUrl)) return; | ||
|
|
||
| const { document } = getEnv(env); | ||
| const script = (document as Document).createElement('script'); | ||
| script.src = src; | ||
| script.async = true; | ||
| (document as Document).head.appendChild(script); | ||
| loadedScripts.add(collectorUrl); | ||
| } |
There was a problem hiding this comment.
Deduplication key uses collectorUrl instead of script src.
The loadedScripts set uses collectorUrl as the key, but this doesn't prevent loading the same script URL multiple times if called with different collector URLs. If the intent is to deduplicate by script source, use src instead.
🐛 Proposed fix
export function addScript(
collectorUrl: string,
env?: DestinationWeb.Env,
src = DEFAULT_SCRIPT_URL,
): void {
// Prevent loading the same script multiple times
- if (loadedScripts.has(collectorUrl)) return;
+ if (loadedScripts.has(src)) return;
const { document } = getEnv(env);
const script = (document as Document).createElement('script');
script.src = src;
script.async = true;
(document as Document).head.appendChild(script);
- loadedScripts.add(collectorUrl);
+ loadedScripts.add(src);
}🤖 Prompt for AI Agents
In `@packages/web/destinations/snowplow/src/setup.ts` around lines 20 - 34, The
dedupe check in addScript currently uses collectorUrl but should use the script
source URL; update the function (addScript) to check loadedScripts.has(src) (or
script.src after resolving DEFAULT_SCRIPT_URL) and to add src (or script.src) to
loadedScripts after appending the element, so duplicate script src values are
prevented regardless of collectorUrl; keep the rest of the logic
(getEnv/document manipulation, async/script insertion) unchanged.
| "accessToken": "$env.META_ACCESS_TOKEN", | ||
| "pixelId": "$env.META_PIXEL_ID", | ||
| "test_event_code": "$env.META_TEST_CODE:" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for $env patterns with trailing colons to see if this is consistent usage
rg '\$env\.[A-Z_]+:' --multiline
# Also search for any documentation about the $env syntax
rg '\$env\.' docs/ -A 2 -B 2 | head -100Repository: elbwalker/walkerOS
Length of output: 1665
Remove or complete the trailing colon in test_event_code.
The trailing colon in "test_event_code": "$env.META_TEST_CODE:" appears to be incomplete. While the $env.NAME:default syntax is intentional throughout the codebase (allowing environment variables with optional fallback defaults), this line has a colon with no default value, making it inconsistent with all other uses of the pattern (e.g., $env.AWS_REGION:eu-central-1, $env.BQ_DATASET:walkeros). Either remove the colon to match the pattern on lines 103-104, or add an appropriate default value after it.
🤖 Prompt for AI Agents
In `@website/docs/destinations/server/meta-capi.mdx` around lines 103 - 105, The
value for "test_event_code" is using the $env pattern but has an unintended
trailing colon; update the "test_event_code" entry (the "test_event_code" key
referencing $env.META_TEST_CODE) to either remove the trailing ':' so it matches
other entries or provide a sensible default after the colon (e.g.,
$env.META_TEST_CODE:defaultValue) so the env fallback pattern is consistent.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.