title, description and public#630
Merged
alexanderkirtzel merged 6 commits intomainfrom Apr 21, 2026
Merged
Conversation
Closed
|
Important Review skippedToo many files! This PR contains 173 files, which is 23 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (173)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds optional per-step example metadata ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MCP UI
participant API as MCP API tool handler
participant FS as Flow examples source (repo)
participant Schema as Examples output schema
UI->>API: request flow_examples (includeHidden? false)
API->>FS: load flow settings & examples
FS-->>API: examples list (with example.public flags)
API->>API: filter out examples where public === false (unless includeHidden)
API->>Schema: attach title/description/public to each example item
API-->>UI: return filtered examples list (count + examples)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
skills/walkeros-create-source/SKILL.md (1)
282-319:⚠️ Potential issue | 🟡 MinorMake the copyable examples follow the new metadata guidance.
The new comment requires
titleanddescriptionfor public examples, but both sampleFlow.StepExampleobjects below are public by default and omit those fields.Suggested template update
export const pageView: Flow.StepExample = { + title: 'Page view', + description: 'A browser load trigger emits a page view event.', trigger: { type: 'load', options: { url: 'https://example.com/docs', @@ // Server example: HTTP POST carrying a walker event payload. export const orderComplete: Flow.StepExample = { + title: 'Order complete', + description: 'An HTTP POST carrying an order payload emits an order complete event.', trigger: { type: 'POST' }, in: { method: 'POST',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/walkeros-create-source/SKILL.md` around lines 282 - 319, Both public Flow.StepExample objects (pageView and orderComplete) are missing the required title and description metadata; update the pageView and orderComplete objects to include a descriptive "title" and a short "description" at the top-level of each example, and if either example is intended to be test-only instead of public, add "public: false" to that object instead of metadata. Ensure the fields are present on the exported objects (pageView, orderComplete) per the new guidance.skills/walkeros-create-transformer/SKILL.md (1)
119-135:⚠️ Potential issue | 🟡 MinorUpdate the template to include the required public metadata.
The text now tells authors to set
titleanddescriptionfor public examples, but the copyableexamples/step.tssnippet still omits both on public-by-default entries.Suggested template update
export const step = { 'order-passes': { + title: 'Order passes', + description: 'A valid order event passes through unchanged.', in: { name: 'order complete', data: { id: 'ORD-123' } }, out: { name: 'order complete', data: { id: 'ORD-123' } }, }, 'debug-filtered': { + public: false, in: { name: 'debug test', data: { message: 'noise' } }, out: false, // Transformer rejects this event }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/walkeros-create-transformer/SKILL.md` around lines 119 - 135, The examples/step.ts snippet defines the exported const step with entries 'order-passes' and 'debug-filtered' but omits the required public metadata; update the template so each public example entry (e.g., step['order-passes']) includes title and description properties and if an entry is only a test fixture (e.g., debug-filtered) mark it with public: false; ensure the README/example text and the example object structure both demonstrate adding title and description for public examples and using public: false for test-only fixtures to keep the docs and copyable snippet consistent.skills/walkeros-create-destination/SKILL.md (1)
235-260:⚠️ Potential issue | 🟡 MinorUpdate the adjacent template to follow the new metadata rule.
The new note says public examples should set
titleanddescription, but thepurchasetemplate immediately below still omits both. Add the fields to the copy-paste example so new destinations follow the rule by default.Proposed fix
export const purchase: Flow.StepExample = { + title: 'Purchase', + description: 'Send a purchase event with transaction value and currency.', in: getEvent('order complete', { timestamp: 1700000100 }), mapping: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/walkeros-create-destination/SKILL.md` around lines 235 - 260, The example Flow.StepExample named "purchase" is missing the required public metadata fields; update the purchase object (export const purchase: Flow.StepExample) to include top-level title and description properties and keep the existing in/mapping/out structure, e.g. add a descriptive title string and a brief description string to the purchase object so public examples follow the new metadata rule.
🧹 Nitpick comments (1)
skills/walkeros-using-step-examples/SKILL.md (1)
237-310: Minor inconsistency in description length guidance.Best Practices (line 238) states descriptions should be "10-25 words", but the example
purchasedescription ("A completed order mapped to the Meta Pixel Purchase standard event.") is 11 words — fine — while several of this PR's actual examples (e.g.,defaultEventForwardingat ~14 words,gtagPurchaseat ~22 words) are within range. However, a few across the PR push close to or just over 25 (e.g., the PostHoguserLoginIdentifyandorderCompleteWithIncludedescriptions). Consider either relaxing the upper bound in guidance or tightening those examples so the documentation and practice stay aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/walkeros-using-step-examples/SKILL.md` around lines 237 - 310, The guidance for example descriptions currently says "10-25 words" but some examples (e.g., userLoginIdentify, orderCompleteWithInclude) exceed 25 words; update either the guidance or the examples so they match: either relax the upper bound in the Best Practices sentence (change "10-25 words" to a larger max) or shorten long descriptions (for examples like defaultEventForwarding, gtagPurchase, userLoginIdentify, orderCompleteWithInclude, and purchase) to <=25 words; edit the SKILL.md metadata guidance or adjust the specific Flow.StepExample description strings accordingly to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/destinations/customerio/src/examples/step.ts`:
- Around line 131-133: Update the description string in the example object (the
entry with title 'Destination identify') to avoid implying a repeated identify
on every track; change the wording from “before each track” to something
narrower such as “before the first track push” or “on the first push before this
track call” so it matches the fixture behavior that only fires an identify once
prior to the demonstrated track.
In `@packages/server/destinations/gcp/src/bigquery/examples/step.ts`:
- Around line 71-73: The description text currently implies only arrays are
JSON-stringified when, in fact, expectedRow() stringifies the entire nested data
object; update the example's description (the title/description block) to
explicitly say that the whole nested data object is JSON-stringified into the
data field (not only arrays) and mention expectedRow() as the source of that
behavior so readers understand the resulting row shape.
In `@packages/server/destinations/mixpanel/src/examples/step.ts`:
- Around line 229-231: The title/description metadata ("title" and "description"
in step.ts) overstate coverage because the example doesn't exercise delete_user;
either update the title and description to soften the claim (e.g., "Common
Mixpanel people operations" or "Most Mixpanel people operations") or add a
delete_user scenario to the example so the metadata is accurate; locate the
"title" and "description" properties in the example object and either change the
wording to avoid "All"/"full" or implement and document a delete_user case to
match the current wording.
In `@packages/server/destinations/rudderstack/src/examples/step.ts`:
- Around line 81-84: The description for the exported constant
destinationIdentify (type RudderStackStepExample) incorrectly says it "fires
analytics.identify with traits before each track call"; update the description
string to match the documented "first push" behavior (e.g., "fires
analytics.identify with traits on the first push only") so the metadata aligns
with the docblock and MCP docs; locate destinationIdentify and replace the
description text accordingly.
In `@packages/server/destinations/segment/src/examples/step.ts`:
- Around line 52-55: The description in the SegmentStepExample object
mappedEventName contains a typo/awkward phrase "Segments canonical"; update the
user-facing string in mappedEventName (type SegmentStepExample) to use correct
possessive and clearer wording—e.g. "the Segment's canonical 'Order Completed'
name" or "Segment's canonical event name 'Order Completed'"—so docs/metadata
show the corrected phrase.
- Around line 81-84: The description for the example object destinationIdentify
(type SegmentStepExample) is inaccurate; update the description string to match
the documented “first push” behavior (e.g., state that destination-level
identify fires on the first push rather than before each track call) so the
example’s wording matches the adjacent docblock and MCP docs.
In `@packages/web/destinations/amplitude/src/examples/step.ts`:
- Around line 106-108: The description string for the "Destination identify"
example currently claims identity setters run "before each track" but the
implementation (the cached identity logic in the example) avoids re-firing when
values are unchanged; update the description to reflect that identity setters
set userId/deviceId/sessionId on the Amplitude client before sending events and
only re-run when those values change (i.e., cached values are not re-applied).
Locate the title 'Destination identify' and its description string and replace
the inaccurate phrase "before each track" with wording that makes the cached
behavior explicit.
In `@website/src/components/snippets/_configuration.mdx`:
- Around line 41-49: The Examples heading is rendered even when all step
examples are filtered out; compute the filtered list from
props.data?.examples?.step (e.g., const visible =
Object.entries(props.data.examples.step).filter(([,ex]) => ex?.public !==
false)) and only render the <h2>Examples</h2> and the mapped
stepTitle/StepExample entries when visible.length > 0; update the JSX around
stepTitle and <StepExample> to iterate over this filtered list instead of
calling filter/flatMap inline so the section is fully gated.
In `@website/static/schema/flow/v3.json`:
- Around line 1161-1163: The schema uses non-standard "id" properties in
definitions like "MappingValue" and "WalkerOSConsent" while targeting draft-07;
remove those "id" fields (leave "title"/"description" intact) so the definitions
rely on the existing `#/definitions/` pointers, and verify no code references the
removed "id" keys (update any that expect an "id" string to use the definition
name or $id if you intentionally need a global identifier).
---
Outside diff comments:
In `@skills/walkeros-create-destination/SKILL.md`:
- Around line 235-260: The example Flow.StepExample named "purchase" is missing
the required public metadata fields; update the purchase object (export const
purchase: Flow.StepExample) to include top-level title and description
properties and keep the existing in/mapping/out structure, e.g. add a
descriptive title string and a brief description string to the purchase object
so public examples follow the new metadata rule.
In `@skills/walkeros-create-source/SKILL.md`:
- Around line 282-319: Both public Flow.StepExample objects (pageView and
orderComplete) are missing the required title and description metadata; update
the pageView and orderComplete objects to include a descriptive "title" and a
short "description" at the top-level of each example, and if either example is
intended to be test-only instead of public, add "public: false" to that object
instead of metadata. Ensure the fields are present on the exported objects
(pageView, orderComplete) per the new guidance.
In `@skills/walkeros-create-transformer/SKILL.md`:
- Around line 119-135: The examples/step.ts snippet defines the exported const
step with entries 'order-passes' and 'debug-filtered' but omits the required
public metadata; update the template so each public example entry (e.g.,
step['order-passes']) includes title and description properties and if an entry
is only a test fixture (e.g., debug-filtered) mark it with public: false; ensure
the README/example text and the example object structure both demonstrate adding
title and description for public examples and using public: false for test-only
fixtures to keep the docs and copyable snippet consistent.
---
Nitpick comments:
In `@skills/walkeros-using-step-examples/SKILL.md`:
- Around line 237-310: The guidance for example descriptions currently says
"10-25 words" but some examples (e.g., userLoginIdentify,
orderCompleteWithInclude) exceed 25 words; update either the guidance or the
examples so they match: either relax the upper bound in the Best Practices
sentence (change "10-25 words" to a larger max) or shorten long descriptions
(for examples like defaultEventForwarding, gtagPurchase, userLoginIdentify,
orderCompleteWithInclude, and purchase) to <=25 words; edit the SKILL.md
metadata guidance or adjust the specific Flow.StepExample description strings
accordingly to ensure consistency.
🪄 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: bc9b1d44-4335-4b87-bdea-338254299d89
📒 Files selected for processing (80)
.changeset/step-example-metadata.mdpackages/core/src/schemas/__tests__/step-example-metadata.test.tspackages/core/src/schemas/flow.tspackages/core/src/types/flow.tspackages/mcps/mcp/src/__tests__/tools/examples.test.tspackages/mcps/mcp/src/schemas/output.tspackages/mcps/mcp/src/tools/examples.tspackages/server/destinations/amplitude/src/examples/step.tspackages/server/destinations/api/src/examples/step.tspackages/server/destinations/aws/src/firehose/examples/step.tspackages/server/destinations/bing/src/examples/step.tspackages/server/destinations/criteo/src/examples/step.tspackages/server/destinations/customerio/src/examples/step.tspackages/server/destinations/datamanager/src/examples/step.tspackages/server/destinations/file/src/examples/step.tspackages/server/destinations/gcp/src/bigquery/examples/step.tspackages/server/destinations/hubspot/src/examples/step.tspackages/server/destinations/kafka/src/examples/step.tspackages/server/destinations/klaviyo/src/examples/step.tspackages/server/destinations/linkedin/src/examples/step.tspackages/server/destinations/meta/src/examples/step.tspackages/server/destinations/mixpanel/src/examples/step.tspackages/server/destinations/mparticle/src/examples/step.tspackages/server/destinations/pinterest/src/examples/step.tspackages/server/destinations/posthog/src/examples/step.tspackages/server/destinations/reddit/src/examples/step.tspackages/server/destinations/redis/src/examples/step.tspackages/server/destinations/rudderstack/src/examples/step.tspackages/server/destinations/segment/src/examples/step.tspackages/server/destinations/slack/src/examples/step.tspackages/server/destinations/snapchat/src/examples/step.tspackages/server/destinations/sqlite/src/examples/step.tspackages/server/destinations/tiktok/src/examples/step.tspackages/server/destinations/twitter/src/examples/step.tspackages/server/sources/aws/src/lambda/examples/step.tspackages/server/sources/express/src/examples/step.tspackages/server/sources/fetch/src/examples/step.tspackages/server/sources/gcp/src/cloudfunction/examples/step.tspackages/server/stores/fs/src/examples/step.tspackages/server/stores/gcs/src/examples/step.tspackages/server/stores/s3/src/examples/step.tspackages/server/transformers/file/src/examples/step.tspackages/server/transformers/fingerprint/src/examples/step.tspackages/stores/memory/src/examples/step.tspackages/transformers/demo/src/examples/step.tspackages/transformers/validator/src/examples/step.tspackages/web/destinations/amplitude/src/examples/step.tspackages/web/destinations/api/src/examples/step.tspackages/web/destinations/clarity/src/examples/step.tspackages/web/destinations/demo/src/examples/step.tspackages/web/destinations/fullstory/src/examples/step.tspackages/web/destinations/gtag/src/examples/step.tspackages/web/destinations/heap/src/examples/step.tspackages/web/destinations/hotjar/src/examples/step.tspackages/web/destinations/linkedin/src/examples/step.tspackages/web/destinations/matomo/src/examples/step.tspackages/web/destinations/meta/src/examples/step.tspackages/web/destinations/mixpanel/src/examples/step.tspackages/web/destinations/optimizely/src/examples/step.tspackages/web/destinations/pinterest/src/examples/step.tspackages/web/destinations/piwikpro/src/examples/step.tspackages/web/destinations/plausible/src/examples/step.tspackages/web/destinations/posthog/src/examples/step.tspackages/web/destinations/segment/src/examples/step.tspackages/web/destinations/snowplow/src/examples/step.tspackages/web/destinations/tiktok/src/examples/step.tspackages/web/sources/browser/src/examples/step.tspackages/web/sources/cmps/cookiefirst/src/examples/step.tspackages/web/sources/cmps/cookiepro/src/examples/step.tspackages/web/sources/cmps/usercentrics/src/examples/step.tspackages/web/sources/dataLayer/src/examples/step.tspackages/web/sources/demo/src/examples/step.tspackages/web/sources/session/src/examples/step.tsskills/walkeros-create-destination/SKILL.mdskills/walkeros-create-source/SKILL.mdskills/walkeros-create-transformer/SKILL.mdskills/walkeros-using-step-examples/SKILL.mdwebsite/src/components/snippets/StepExample.tsxwebsite/src/components/snippets/_configuration.mdxwebsite/static/schema/flow/v3.json
Contributor
Preview deployed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/explorer/src/themes/lighthouse.ts`:
- Around line 30-33: The numericConstant, operator, and operatorKeyword color
values in lighthouse theme are too light for a white background; update the hex
values for numericConstant, operator, and operatorKeyword (and optionally
keyword if needed) to higher-contrast, darker variants—e.g., replace `#fb923c`
with a deeper orange like `#d97706`, replace `#01b5e2` with a darker cyan like
`#0b66c2`, and replace `#0086B3` with a deeper teal like `#00608f`—by editing the
numericConstant, operator, and operatorKeyword entries in the lighthouse theme
so tokens are more readable on the light theme.
🪄 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: 1468cb5d-e25e-4d33-bd99-2e0788f9c896
📒 Files selected for processing (8)
apps/explorer/src/components/atoms/code-static.tsxapps/explorer/src/components/atoms/code.tsxapps/explorer/src/styles/index.scssapps/explorer/src/themes/index.tsapps/explorer/src/themes/lighthouse.tsapps/explorer/src/themes/names.tsapps/explorer/src/themes/palenight.tsapps/explorer/src/themes/token-groups.ts
✅ Files skipped from review due to trivial changes (2)
- apps/explorer/src/styles/index.scss
- apps/explorer/src/themes/names.ts
Comment on lines
+30
to
+33
| numericConstant: 'fb923c', | ||
| keyword: '6F42C1', | ||
| operator: '01b5e2', // Brand cyan — `operator` / `operators` tokens only | ||
| operatorKeyword: '0086B3', // Darker teal — `keyword.operator*` |
There was a problem hiding this comment.
Darken the light-theme orange/cyan tokens for readability.
#fb923c, #01b5e2, and #0086B3 are low-contrast on the light theme’s white background, so numeric constants/operators can be hard to read. Consider darker equivalents.
🎨 Proposed palette adjustment
- numericConstant: 'fb923c',
+ numericConstant: 'B45309',
keyword: '6F42C1',
- operator: '01b5e2', // Brand cyan — `operator` / `operators` tokens only
- operatorKeyword: '0086B3', // Darker teal — `keyword.operator*`
+ operator: '007A9E', // Brand cyan — `operator` / `operators` tokens only
+ operatorKeyword: '006D8F', // Darker teal — `keyword.operator*`📝 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.
Suggested change
| numericConstant: 'fb923c', | |
| keyword: '6F42C1', | |
| operator: '01b5e2', // Brand cyan — `operator` / `operators` tokens only | |
| operatorKeyword: '0086B3', // Darker teal — `keyword.operator*` | |
| numericConstant: 'B45309', | |
| keyword: '6F42C1', | |
| operator: '007A9E', // Brand cyan — `operator` / `operators` tokens only | |
| operatorKeyword: '006D8F', // Darker teal — `keyword.operator*` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/explorer/src/themes/lighthouse.ts` around lines 30 - 33, The
numericConstant, operator, and operatorKeyword color values in lighthouse theme
are too light for a white background; update the hex values for numericConstant,
operator, and operatorKeyword (and optionally keyword if needed) to
higher-contrast, darker variants—e.g., replace `#fb923c` with a deeper orange like
`#d97706`, replace `#01b5e2` with a darker cyan like `#0b66c2`, and replace `#0086B3`
with a deeper teal like `#00608f`—by editing the numericConstant, operator, and
operatorKeyword entries in the lighthouse theme so tokens are more readable on
the light theme.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
scripts/generate-flow-schema.ts (1)
22-33: Avoid rewriting literal schema payloads while stripping schemaids.The recursive walk removes any string-valued
id, including inside data-bearing JSON Schema keywords likeexamples,default,const, orenum. If those payloads ever include{ id: "..." }, the generated public schema will silently corrupt the example/default value rather than only removing Zod’s schema metadata.♻️ Proposed safer traversal
-function stripNonStandardIds(value: unknown): unknown { - if (Array.isArray(value)) return value.map(stripNonStandardIds); +const literalValueKeywords = new Set(['default', 'examples', 'const', 'enum']); + +function stripNonStandardIds(value: unknown, preserveLiteralValue = false): unknown { + if (preserveLiteralValue) return value; + if (Array.isArray(value)) return value.map((item) => stripNonStandardIds(item)); if (value && typeof value === 'object') { const out: Record<string, unknown> = {}; for (const [k, v] of Object.entries(value as Record<string, unknown>)) { if (k === 'id' && typeof v === 'string') continue; - out[k] = stripNonStandardIds(v); + out[k] = stripNonStandardIds(v, literalValueKeywords.has(k)); } return out; } return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-flow-schema.ts` around lines 22 - 33, The current stripNonStandardIds function removes any string-valued "id" anywhere (including inside examples/default/const/enum), corrupting literal payloads; update stripNonStandardIds so when iterating object entries it preserves values for data-bearing JSON Schema keywords (e.g., "examples","default","const","enum") by copying them verbatim without recursing or stripping "id" inside them, while still continuing to strip schema metadata "id" elsewhere; modify the loop in stripNonStandardIds to check if k is in a dataKeywords set and if so assign out[k] = v (no recursion/remove), otherwise keep the existing logic (drop string "id" keys and recurse for other keys).packages/collector/src/store.ts (1)
35-52: Use the scoped store logger for hook warnings.
storeLoggeralready includes thestore/storeIdscope. Passing it touseHooksmakes failures inStoreGet,StoreSet, andStoreDeleteeasier to attribute.🔎 Proposed logging-scope tweak
instance.get = useHooks( originalGet, 'StoreGet', collector.hooks, - collector.logger, + storeLogger, ); instance.set = useHooks( originalSet, 'StoreSet', collector.hooks, - collector.logger, + storeLogger, ); instance.delete = useHooks( originalDelete, 'StoreDelete', collector.hooks, - collector.logger, + storeLogger, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/collector/src/store.ts` around lines 35 - 52, The hook wrappers for store operations are using the global collector.logger instead of the scoped storeLogger, so change the calls to useHooks for instance.get/instance.set/instance.delete to pass storeLogger (which already includes the store/storeId scope) instead of collector.logger; update the three calls that currently wrap originalGet, originalSet, originalDelete with 'StoreGet'/'StoreSet'/'StoreDelete' to supply storeLogger so hook warnings are attributed to the specific store.packages/collector/src/destination.ts (1)
508-513: Prefer the scoped destination logger for hook warnings.
destLoggeris already created for these destination paths. Passing it intouseHookskeeps hook failure warnings scoped to the destination instead of emitting from the root collector logger.🔎 Proposed logging-scope tweak
const configResult = await useHooks( destination.init, 'DestinationInit', collector.hooks, - collector.logger, + destLogger, )(context);useHooks( destination.pushBatch!, 'DestinationPushBatch', collector.hooks, - collector.logger, + destLogger, )(currentBatched, batchContext);const response = await useHooks( destination.push, 'DestinationPush', collector.hooks, - collector.logger, + destLogger, )(processed.event, context);Also applies to: 634-639, 661-666
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/collector/src/destination.ts` around lines 508 - 513, The useHooks call is using the root collector.logger instead of the per-destination logger, so change the logger argument passed to useHooks to destLogger to scope hook warnings to the destination; update the three occurrences where useHooks is invoked for destination.init (the call with 'DestinationInit') and the other similar calls around the same block (the calls at the spots referencing collector.hooks and collector.logger) to pass destLogger instead of collector.logger, ensuring DestinationInit and related hook invocations use destLogger and keep context as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/cli.ts`:
- Around line 497-511: The CLI action currently calls createPreview(...) before
validating options.url which can create a remote preview that later fails
formatting; move or perform URL validation for options.url before invoking
createPreview (either reuse the validation logic from printPreviewCreated or
call a shared validateUrl helper) so that invalid URLs are rejected and the
action exits prior to calling createPreview; update the action block that
references createPreview, printPreviewCreated, and options.url to validate
options.url first and only call createPreview when the URL is valid.
In `@packages/cli/src/commands/bundle/bundler.ts`:
- Around line 1764-1778: The HEAD probe using await fetch(__previewSrc, {
method: 'HEAD' }) can hang indefinitely; wrap this probe in an AbortController
with a short timeout (e.g. 1500–3000ms) and pass controller.signal to fetch,
clear the timeout on success, and let the existing catch branch call
__clearPreviewCookie on abort/error so the production walker runs; update the
code around __probe, __previewSrc and __clearPreviewCookie (the async IIFE /
startFlow probe logic) to create controller, schedule controller.abort() via
setTimeout, and clean up the timer, and add a jsdom test that mocks fetch
returning a never-resolving Promise to assert the timeout path executes and
production walker initializes.
In `@packages/cli/src/commands/previews/index.ts`:
- Around line 93-104: The deletePreview function currently calls response.json()
unconditionally which throws on 204 No Content or empty 200 responses; update
deletePreview (and its handling of the apiFetch response) to check
response.status (or response.headers.get('content-length')/response.bodyUsed)
and if the status is 204 or the body is empty return null (or an empty object)
instead of calling response.json(), otherwise parse and return response.json();
ensure the error path unchanged (keep throwApiError(body, 'Failed to delete
preview') when !response.ok).
In `@packages/cli/src/commands/previews/output.ts`:
- Around line 66-73: The spawn call that opens the preview URL should use a
Windows-compatible opener and handle spawn errors: replace the Windows branch
that currently returns 'start' with 'rundll32.exe' and pass args
['url.dll,FileProtocolHandler', stdoutLast] for Windows, keep 'open' for darwin
and 'xdg-open' for other platforms, and attach an 'error' listener to the
ChildProcess returned by spawn (the current spawn(...) call around options.open
&& options.url using stdoutLast) to log or handle failures instead of letting an
unhandled 'error' event crash the process.
In `@packages/core/src/__tests__/useHooks.test.ts`:
- Around line 49-98: Add tests that cover async hook failures by creating
preTest and postTest hooks that return rejected promises (e.g.,
jest.fn().mockRejectedValue(new Error('...'))) and asserting that
useHooks-wrapped calls still return the original result and that warnSpy was
called with the hook name and an Error; specifically add one test for a rejected
preTest hook (useHooks(fn, 'Test', { preTest })) and one for a rejected postTest
hook (useHooks(fn, 'Test', { postTest })), and for each ensure wrapped(5) yields
10 (await if wrapped is async) and warnSpy received
expect.stringContaining('preTest'/'postTest') and expect.any(Error).
In `@packages/core/src/useHooks.ts`:
- Around line 39-58: The preHook/postHook wrapper currently uses synchronous
try/catch which won't catch rejected promises and may double-invoke fn; update
the logic around preHookFn and postHookFn to detect promise-like returns (e.g.,
result && typeof result.then === 'function'), attach a .catch handler to handle
async rejections and call warn there, and avoid immediately calling fn when a
preHook returned a promise—instead wait for the promise to resolve or reject;
also introduce a boolean flag (e.g., didCallOriginal) scoped with the wrapper to
track whether fn(...args) has already been invoked (so callers inside a preHook
can't cause a duplicate call) and only call fn if !didCallOriginal after
resolving the preHook result; apply the same promise-detection + rejection
handling for postHookFn to preserve original behavior without unhandled
rejections.
In `@packages/mcps/mcp/src/tools/api.ts`:
- Around line 399-436: The code creates a preview via createPreviewApi() before
validating siteUrl, which can produce orphan previews if siteUrl is invalid;
update the preview.create branch to validate siteUrl (e.g., attempt new
URL(siteUrl) inside a try/catch or use a URL validation helper) before calling
createPreviewApi(), only proceed to create the preview when siteUrl is either
absent or valid, and if validation fails throw or return a user-facing error;
keep references to createPreviewApi, siteUrl, new URL(siteUrl), preview, and
enriched.activationUrl/deactivationUrl when reordering the logic.
In `@packages/web/core/src/__tests__/storage.test.ts`:
- Around line 176-190: The test "cookie storageWrite does not throw when
document.cookie setter throws" currently only ensures no exception escapes;
update it to also assert that storageWrite('k', 'v', 30, 'cookie', undefined,
env) returns an empty value on failure (matching local/session behavior). Locate
the test block in packages/web/core/src/__tests__/storage.test.ts and add an
assertion that the returned value is '' (or the empty return value your
implementation uses) after calling storageWrite, while keeping the existing
no-throw expectation.
---
Nitpick comments:
In `@packages/collector/src/destination.ts`:
- Around line 508-513: The useHooks call is using the root collector.logger
instead of the per-destination logger, so change the logger argument passed to
useHooks to destLogger to scope hook warnings to the destination; update the
three occurrences where useHooks is invoked for destination.init (the call with
'DestinationInit') and the other similar calls around the same block (the calls
at the spots referencing collector.hooks and collector.logger) to pass
destLogger instead of collector.logger, ensuring DestinationInit and related
hook invocations use destLogger and keep context as-is.
In `@packages/collector/src/store.ts`:
- Around line 35-52: The hook wrappers for store operations are using the global
collector.logger instead of the scoped storeLogger, so change the calls to
useHooks for instance.get/instance.set/instance.delete to pass storeLogger
(which already includes the store/storeId scope) instead of collector.logger;
update the three calls that currently wrap originalGet, originalSet,
originalDelete with 'StoreGet'/'StoreSet'/'StoreDelete' to supply storeLogger so
hook warnings are attributed to the specific store.
In `@scripts/generate-flow-schema.ts`:
- Around line 22-33: The current stripNonStandardIds function removes any
string-valued "id" anywhere (including inside examples/default/const/enum),
corrupting literal payloads; update stripNonStandardIds so when iterating object
entries it preserves values for data-bearing JSON Schema keywords (e.g.,
"examples","default","const","enum") by copying them verbatim without recursing
or stripping "id" inside them, while still continuing to strip schema metadata
"id" elsewhere; modify the loop in stripNonStandardIds to check if k is in a
dataKeywords set and if so assign out[k] = v (no recursion/remove), otherwise
keep the existing logic (drop string "id" keys and recurse for other keys).
🪄 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: 6f3b0d8e-4be0-4913-877e-e6f391fcefc5
📒 Files selected for processing (37)
.changeset/cli-previews-command.md.changeset/mcp-preview-actions.md.changeset/preview-preflight-self-heal.md.changeset/usehooks-error-boundary.md.changeset/web-core-storage-try-catch.mdpackages/cli/src/__tests__/unit/bundle/preview-preflight-jsdom.test.tspackages/cli/src/cli.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/commands/previews/__tests__/output.test.tspackages/cli/src/commands/previews/__tests__/previews.test.tspackages/cli/src/commands/previews/index.tspackages/cli/src/commands/previews/output.tspackages/cli/src/index.tspackages/collector/src/command.tspackages/collector/src/destination.tspackages/collector/src/push.tspackages/collector/src/store.tspackages/collector/src/transformer.tspackages/core/src/__tests__/useHooks.test.tspackages/core/src/useHooks.tspackages/mcps/mcp/src/__tests__/tools/api-preview.test.tspackages/mcps/mcp/src/tools/api.tspackages/server/destinations/customerio/src/examples/step.tspackages/server/destinations/gcp/src/bigquery/examples/step.tspackages/server/destinations/mixpanel/src/examples/step.tspackages/server/destinations/rudderstack/src/examples/step.tspackages/server/destinations/segment/src/examples/step.tspackages/web/core/src/__tests__/storage.test.tspackages/web/core/src/storage.tspackages/web/destinations/amplitude/src/examples/step.tsscripts/generate-flow-schema.tsskills/walkeros-using-cli/SKILL.mdskills/walkeros-using-cli/commands-reference.mdwebsite/docs/apps/cli.mdxwebsite/docs/apps/mcp.mdxwebsite/src/components/snippets/_configuration.mdxwebsite/static/schema/flow/v3.json
✅ Files skipped from review due to trivial changes (14)
- skills/walkeros-using-cli/SKILL.md
- .changeset/cli-previews-command.md
- .changeset/preview-preflight-self-heal.md
- .changeset/web-core-storage-try-catch.md
- .changeset/mcp-preview-actions.md
- .changeset/usehooks-error-boundary.md
- packages/collector/src/transformer.ts
- packages/server/destinations/gcp/src/bigquery/examples/step.ts
- skills/walkeros-using-cli/commands-reference.md
- packages/cli/src/commands/previews/tests/previews.test.ts
- packages/server/destinations/customerio/src/examples/step.ts
- website/docs/apps/cli.mdx
- packages/server/destinations/rudderstack/src/examples/step.ts
- packages/server/destinations/mixpanel/src/examples/step.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- website/static/schema/flow/v3.json
- packages/web/destinations/amplitude/src/examples/step.ts
Comment on lines
+66
to
+73
| if (options.open && options.url) { | ||
| const opener = | ||
| process.platform === 'darwin' | ||
| ? 'open' | ||
| : process.platform === 'win32' | ||
| ? 'start' | ||
| : 'xdg-open'; | ||
| spawn(opener, [stdoutLast], { stdio: 'ignore', detached: true }).unref(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js child_process.spawn Windows start command shell built-in error event
💡 Result:
When using Node.js child_process.spawn on Windows to execute the built-in 'start' command (typically via shell: true or spawning cmd.exe /c start ...), the 'error' event is not triggered for command failures. The 'start' command starts a separate process asynchronously and immediately returns exit code 0 to the cmd.exe shell process, even if the started application fails to launch or encounters issues. The Node.js child process (cmd.exe) thus completes successfully with code 0, emitting 'close' or 'exit' with 0, without propagating the target application's failure via the 'error' event. To handle errors in the started process: - Use {detached: true, stdio: 'ignore'} and child.unref to fully detach, but this loses event access. - Listen to 'exit'/'close' events (code will be 0 for success). - For critical apps, avoid 'start' and spawn the executable directly with appropriate options. - The 'error' event only fires for spawn failures (e.g., ENOENT if command not found without shell: true), not runtime issues in shell commands like 'start'. Official Node.js docs confirm spawn with shell: true runs commands via shell, and 'error' is for spawn failures only. Stack Overflow examples show shell commands like 'dir' succeed with shell: true, no 'error' for invalid subcommands.
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://stackoverflow.com/questions/17749055/nodejs-throw-er-unhandled-error-event-events-js72-when-using-child-proc
- 3: https://stackoverflow.com/questions/60386867/node-spawn-child-process-not-working-in-windows
- 4: https://stackoverflow.com/questions/44986830/nodejs-script-with-child-process-spawn-on-windows-why-i-need-shell-true-for
- 5: https://stackoverflow.com/questions/37459717/error-spawn-enoent-on-windows
- 6: https://stackoverflow.com/questions/43587271/windows-start-command-not-executing-with-child-process-exec-in-vs-code-plugin
- 7: https://stackoverflow.com/questions/18334181/spawn-on-node-js-windows-server-2012
- 8: https://stackoverflow.com/questions/26791900/node-js-why-does-a-child-process-not-start-right-away
🏁 Script executed:
# Check if file exists and read the relevant section
if [ -f "packages/cli/src/commands/previews/output.ts" ]; then
echo "=== File found ==="
wc -l packages/cli/src/commands/previews/output.ts
echo "=== Lines 60-80 ==="
sed -n '60,80p' packages/cli/src/commands/previews/output.ts
else
echo "File not found"
fiRepository: elbwalker/walkerOS
Length of output: 646
🏁 Script executed:
# Check if there are any error handlers on spawn results elsewhere in the file
rg -n "spawn\(" packages/cli/src/commands/previews/output.ts -A 3 -B 2Repository: elbwalker/walkerOS
Length of output: 204
🏁 Script executed:
# Check Node.js version constraints in the project
fd -e package.json | head -5 | xargs grep -l "engines.*node" 2>/dev/null || echo "Checking for Node versions in main package.json"
cat package.json | grep -A 5 '"engines"' || cat package.json | grep '"node"' || echo "No explicit Node version constraint found"Repository: elbwalker/walkerOS
Length of output: 199
Add error handling to the spawn call and use Windows-compatible opener.
spawn('start', ...) will fail on Windows because start is a shell built-in, not an executable. The unhandled error event will crash the command. Use rundll32.exe to open URLs on Windows and add an error listener on all platforms.
Proposed fix
if (options.open && options.url) {
- const opener =
- process.platform === 'darwin'
- ? 'open'
- : process.platform === 'win32'
- ? 'start'
- : 'xdg-open';
- spawn(opener, [stdoutLast], { stdio: 'ignore', detached: true }).unref();
+ const spawnOptions = {
+ stdio: 'ignore',
+ detached: true,
+ windowsHide: true,
+ } as const;
+ const child =
+ process.platform === 'darwin'
+ ? spawn('open', [stdoutLast], spawnOptions)
+ : process.platform === 'win32'
+ ? spawn(
+ 'rundll32',
+ ['url.dll,FileProtocolHandler', stdoutLast],
+ spawnOptions,
+ )
+ : spawn('xdg-open', [stdoutLast], spawnOptions);
+
+ child.on('error', (error) => {
+ process.stderr.write(`Could not open preview URL: ${error.message}\n`);
+ });
+ child.unref();
}📝 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.
Suggested change
| if (options.open && options.url) { | |
| const opener = | |
| process.platform === 'darwin' | |
| ? 'open' | |
| : process.platform === 'win32' | |
| ? 'start' | |
| : 'xdg-open'; | |
| spawn(opener, [stdoutLast], { stdio: 'ignore', detached: true }).unref(); | |
| if (options.open && options.url) { | |
| const spawnOptions = { | |
| stdio: 'ignore', | |
| detached: true, | |
| windowsHide: true, | |
| } as const; | |
| const child = | |
| process.platform === 'darwin' | |
| ? spawn('open', [stdoutLast], spawnOptions) | |
| : process.platform === 'win32' | |
| ? spawn( | |
| 'rundll32', | |
| ['url.dll,FileProtocolHandler', stdoutLast], | |
| spawnOptions, | |
| ) | |
| : spawn('xdg-open', [stdoutLast], spawnOptions); | |
| child.on('error', (error) => { | |
| process.stderr.write(`Could not open preview URL: ${error.message}\n`); | |
| }); | |
| child.unref(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/previews/output.ts` around lines 66 - 73, The spawn
call that opens the preview URL should use a Windows-compatible opener and
handle spawn errors: replace the Windows branch that currently returns 'start'
with 'rundll32.exe' and pass args ['url.dll,FileProtocolHandler', stdoutLast]
for Windows, keep 'open' for darwin and 'xdg-open' for other platforms, and
attach an 'error' listener to the ChildProcess returned by spawn (the current
spawn(...) call around options.open && options.url using stdoutLast) to log or
handle failures instead of letting an unhandled 'error' event crash the process.
Comment on lines
+49
to
+98
| test('preHook error falls back to original fn and warns via console', () => { | ||
| const fn = jest.fn((x: number) => x * 2); | ||
| const preHook = jest.fn(() => { | ||
| throw new Error('preHook exploded'); | ||
| }); | ||
| const wrapped = useHooks(fn, 'Test', { | ||
| preTest: preHook, | ||
| } as unknown as Hooks.Functions); | ||
|
|
||
| expect(wrapped(5)).toBe(10); | ||
| expect(fn).toHaveBeenCalledWith(5); | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('preTest'), | ||
| expect.any(Error), | ||
| ); | ||
| }); | ||
|
|
||
| test('postHook error returns original result and warns via console', () => { | ||
| const fn = jest.fn((x: number) => x * 2); | ||
| const postHook = jest.fn(() => { | ||
| throw new Error('postHook exploded'); | ||
| }); | ||
| const wrapped = useHooks(fn, 'Test', { | ||
| postTest: postHook, | ||
| } as unknown as Hooks.Functions); | ||
|
|
||
| expect(wrapped(5)).toBe(10); | ||
| expect(fn).toHaveBeenCalledWith(5); | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('postTest'), | ||
| expect.any(Error), | ||
| ); | ||
| }); | ||
|
|
||
| test('both hooks erroring keeps pipeline alive', () => { | ||
| const fn = jest.fn((x: number) => x * 2); | ||
| const preHook = jest.fn(() => { | ||
| throw new Error('pre'); | ||
| }); | ||
| const postHook = jest.fn(() => { | ||
| throw new Error('post'); | ||
| }); | ||
| const wrapped = useHooks(fn, 'Test', { | ||
| preTest: preHook, | ||
| postTest: postHook, | ||
| } as unknown as Hooks.Functions); | ||
|
|
||
| expect(wrapped(5)).toBe(10); | ||
| expect(warnSpy).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the useHooks implementation
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "export.*useHooks\|function useHooks" | head -20Repository: elbwalker/walkerOS
Length of output: 122
🏁 Script executed:
# Look for the useHooks function definition
fd -e ts -e tsx | xargs grep -l "useHooks" | grep -E "(src/|lib/)" | grep -v test | head -20Repository: elbwalker/walkerOS
Length of output: 291
🏁 Script executed:
# Search in packages/core
fd -e ts -e tsx packages/core/src | grep -v test | xargs grep -l "useHooks" 2>/dev/nullRepository: elbwalker/walkerOS
Length of output: 437
🏁 Script executed:
# Examine the useHooks implementation
cat -n packages/core/src/useHooks.tsRepository: elbwalker/walkerOS
Length of output: 2347
🏁 Script executed:
# Check how useHooks is used in collector - particularly with async functions
grep -A 5 -B 5 "useHooks" packages/collector/src/destination.ts | head -50Repository: elbwalker/walkerOS
Length of output: 1163
🏁 Script executed:
# Check collector push for async usage
grep -A 5 -B 5 "useHooks" packages/collector/src/push.ts | head -50Repository: elbwalker/walkerOS
Length of output: 712
🏁 Script executed:
# Examine the test file to see what's currently tested
cat -n packages/core/src/__tests__/useHooks.test.tsRepository: elbwalker/walkerOS
Length of output: 4652
Add async rejection coverage for hook isolation.
The current tests only validate synchronous throws. Since useHooks wraps async collector/destination functions in production (e.g., destination.init, destination.push), add test cases for rejected-promise pre- and post-hook scenarios to ensure hook failures are handled gracefully in async contexts.
🧪 Suggested tests
+ test('async preHook rejection falls back to original fn and warns', async () => {
+ const fn = jest.fn(async (x: number) => x * 2);
+ const preHook = jest.fn(async () => {
+ throw new Error('async preHook exploded');
+ });
+ const wrapped = useHooks(fn, 'Test', {
+ preTest: preHook,
+ } as unknown as Hooks.Functions);
+
+ await expect(wrapped(5)).resolves.toBe(10);
+ expect(fn).toHaveBeenCalledWith(5);
+ expect(warnSpy).toHaveBeenCalledWith(
+ expect.stringContaining('preTest'),
+ expect.any(Error),
+ );
+ });
+
+ test('async postHook rejection keeps original result and warns', async () => {
+ const fn = jest.fn(async (x: number) => x * 2);
+ const postHook = jest.fn(async () => {
+ throw new Error('async postHook exploded');
+ });
+ const wrapped = useHooks(fn, 'Test', {
+ postTest: postHook,
+ } as unknown as Hooks.Functions);
+
+ await expect(wrapped(5)).resolves.toBe(10);
+ expect(fn).toHaveBeenCalledWith(5);
+ expect(warnSpy).toHaveBeenCalledWith(
+ expect.stringContaining('postTest'),
+ expect.any(Error),
+ );
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/__tests__/useHooks.test.ts` around lines 49 - 98, Add tests
that cover async hook failures by creating preTest and postTest hooks that
return rejected promises (e.g., jest.fn().mockRejectedValue(new Error('...')))
and asserting that useHooks-wrapped calls still return the original result and
that warnSpy was called with the hook name and an Error; specifically add one
test for a rejected preTest hook (useHooks(fn, 'Test', { preTest })) and one for
a rejected postTest hook (useHooks(fn, 'Test', { postTest })), and for each
ensure wrapped(5) yields 10 (await if wrapped is async) and warnSpy received
expect.stringContaining('preTest'/'postTest') and expect.any(Error).
Comment on lines
39
to
+58
| if (preHookFn) { | ||
| // Call the original function within the preHook | ||
| result = preHookFn({ fn }, ...args); | ||
| try { | ||
| result = preHookFn({ fn }, ...args); | ||
| } catch (error) { | ||
| warn( | ||
| `Hook ${String(preHook)} failed, falling back to original function`, | ||
| error, | ||
| ); | ||
| result = fn(...args); | ||
| } | ||
| } else { | ||
| // Regular function call | ||
| result = fn(...args); | ||
| } | ||
|
|
||
| if (postHookFn) { | ||
| // Call the post-hook function with fn, result, and the original args | ||
| result = postHookFn({ fn, result }, ...args); | ||
| try { | ||
| result = postHookFn({ fn, result }, ...args); | ||
| } catch (error) { | ||
| warn(`Hook ${String(postHook)} failed, keeping original result`, error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l packages/core/src/useHooks.tsRepository: elbwalker/walkerOS
Length of output: 94
🏁 Script executed:
cat -n packages/core/src/useHooks.tsRepository: elbwalker/walkerOS
Length of output: 2347
🏁 Script executed:
rg "useHooks" --type ts --type tsx -A 3 -B 3 | head -100Repository: elbwalker/walkerOS
Length of output: 89
🏁 Script executed:
rg "Hook\[" --type ts --type tsx -A 5 -B 5 | head -150Repository: elbwalker/walkerOS
Length of output: 89
🏁 Script executed:
rg "useHooks\(" --type ts -A 2 -B 2Repository: elbwalker/walkerOS
Length of output: 5799
🏁 Script executed:
rg "pre\w+\s*[:=]|post\w+\s*[:=]" --type ts -A 3 -B 1 | head -200Repository: elbwalker/walkerOS
Length of output: 12395
🏁 Script executed:
rg "hooks\[" --type ts -A 5Repository: elbwalker/walkerOS
Length of output: 558
🏁 Script executed:
fd -t f "\.ts$" packages/core/src | head -20Repository: elbwalker/walkerOS
Length of output: 964
🏁 Script executed:
cat -n packages/core/src/__tests__/useHooks.test.tsRepository: elbwalker/walkerOS
Length of output: 4652
🏁 Script executed:
rg "async.*preHook|async.*postHook" packages/core/src/__tests__/useHooks.test.ts -A 5 -B 2Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
cat -n packages/collector/src/push.ts | head -50Repository: elbwalker/walkerOS
Length of output: 1911
🏁 Script executed:
cat -n packages/collector/src/destination.ts | sed -n '1,150p'Repository: elbwalker/walkerOS
Length of output: 5090
🏁 Script executed:
cat -n packages/collector/src/transformer.ts | sed -n '1,100p'Repository: elbwalker/walkerOS
Length of output: 4198
🏁 Script executed:
rg "await useHooks" packages/collector/src -A 2 -B 2Repository: elbwalker/walkerOS
Length of output: 1379
🏁 Script executed:
rg "Hooks.HookFn|HookFn<" packages/core/src/types -A 3Repository: elbwalker/walkerOS
Length of output: 335
🏁 Script executed:
cat -n packages/collector/src/transformer.ts | sed -n '150,250p'Repository: elbwalker/walkerOS
Length of output: 3784
🏁 Script executed:
rg "const configResult = await useHooks" packages/collector/src -A 10 -B 2Repository: elbwalker/walkerOS
Length of output: 1768
🏁 Script executed:
cat -n packages/collector/src/push.ts | sed -n '40,100p'Repository: elbwalker/walkerOS
Length of output: 2473
🏁 Script executed:
rg "await useHooks" packages/collector/src -B 2 -A 5Repository: elbwalker/walkerOS
Length of output: 2105
🏁 Script executed:
rg "type.*HookFn|interface.*HookFn" packages/core/src/types -A 5Repository: elbwalker/walkerOS
Length of output: 335
🏁 Script executed:
cat -n packages/core/src/types/index.ts | grep -i hookRepository: elbwalker/walkerOS
Length of output: 102
🏁 Script executed:
cat -n packages/core/src/types/hooks.tsRepository: elbwalker/walkerOS
Length of output: 859
🏁 Script executed:
rg "preDestinationPush|postDestinationPush|preTransformerPush|postTransformerPush" packages/core/src -A 3 -B 1Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
rg "async.*Hook|Hook.*async" packages --type ts -A 2 -B 2 | head -60Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
cat -n packages/collector/src/destination.ts | sed -n '200,350p'Repository: elbwalker/walkerOS
Length of output: 6737
🏁 Script executed:
rg "tryCatchAsync" packages/core/src/types -A 5Repository: elbwalker/walkerOS
Length of output: 44
🏁 Script executed:
cat -n packages/collector/src/destination.ts | sed -n '340,370p'Repository: elbwalker/walkerOS
Length of output: 1234
Handle async hook failures and prevent double invocation of wrapped functions.
Lines 40–48 wrap async functions in synchronous try/catch, which cannot catch rejected promises from async hooks. Additionally, line 47 can invoke fn(...args) a second time if a pre-hook calls fn and then throws synchronously, duplicating side effects in non-idempotent operations like external API calls or database writes. Tests cover only synchronous scenarios and do not exercise async hooks or rejections.
The proposed solution correctly detects promise-like results and attaches rejection handlers before falling back, while tracking whether the original function was already called to prevent duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/useHooks.ts` around lines 39 - 58, The preHook/postHook
wrapper currently uses synchronous try/catch which won't catch rejected promises
and may double-invoke fn; update the logic around preHookFn and postHookFn to
detect promise-like returns (e.g., result && typeof result.then === 'function'),
attach a .catch handler to handle async rejections and call warn there, and
avoid immediately calling fn when a preHook returned a promise—instead wait for
the promise to resolve or reject; also introduce a boolean flag (e.g.,
didCallOriginal) scoped with the wrapper to track whether fn(...args) has
already been invoked (so callers inside a preHook can't cause a duplicate call)
and only call fn if !didCallOriginal after resolving the preHook result; apply
the same promise-detection + rejection handling for postHookFn to preserve
original behavior without unhandled rejections.
Comment on lines
+176
to
+190
| test('cookie storageWrite does not throw when document.cookie setter throws', () => { | ||
| const env = { | ||
| document: { | ||
| get cookie() { | ||
| return ''; | ||
| }, | ||
| set cookie(_val: string) { | ||
| throw new DOMException('Cookies disabled', 'SecurityError'); | ||
| }, | ||
| } as unknown as Document, | ||
| }; | ||
| expect(() => | ||
| storageWrite('k', 'v', 30, 'cookie', undefined, env), | ||
| ).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Assert the cookie write failure return value too.
storageWrite now returns an empty value on write failure; this cookie test only checks that no exception escapes, unlike the local/session cases.
🧪 Proposed test assertion
- expect(() =>
- storageWrite('k', 'v', 30, 'cookie', undefined, env),
- ).not.toThrow();
+ expect(storageWrite('k', 'v', 30, 'cookie', undefined, env)).toBe('');📝 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.
Suggested change
| test('cookie storageWrite does not throw when document.cookie setter throws', () => { | |
| const env = { | |
| document: { | |
| get cookie() { | |
| return ''; | |
| }, | |
| set cookie(_val: string) { | |
| throw new DOMException('Cookies disabled', 'SecurityError'); | |
| }, | |
| } as unknown as Document, | |
| }; | |
| expect(() => | |
| storageWrite('k', 'v', 30, 'cookie', undefined, env), | |
| ).not.toThrow(); | |
| }); | |
| test('cookie storageWrite does not throw when document.cookie setter throws', () => { | |
| const env = { | |
| document: { | |
| get cookie() { | |
| return ''; | |
| }, | |
| set cookie(_val: string) { | |
| throw new DOMException('Cookies disabled', 'SecurityError'); | |
| }, | |
| } as unknown as Document, | |
| }; | |
| expect(storageWrite('k', 'v', 30, 'cookie', undefined, env)).toBe(''); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/core/src/__tests__/storage.test.ts` around lines 176 - 190, The
test "cookie storageWrite does not throw when document.cookie setter throws"
currently only ensures no exception escapes; update it to also assert that
storageWrite('k', 'v', 30, 'cookie', undefined, env) returns an empty value on
failure (matching local/session behavior). Locate the test block in
packages/web/core/src/__tests__/storage.test.ts and add an assertion that the
returned value is '' (or the empty return value your implementation uses) after
calling storageWrite, while keeping the existing no-throw expectation.
Contributor
|
📦 Pre-release published (next) Packages Install: 🐳 Docker images published
Docker: |
Contributor
|
🚀 Stable release published Packages Install: 🐳 Docker images published
Docker: |
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 by CodeRabbit
New Features
Documentation
Tests