660 in build store#661
Open
alexanderkirtzel wants to merge 4 commits into
Open
Conversation
Open
📝 WalkthroughWalkthroughThis PR promotes several internal helpers to public exports, introduces declarative per-step ChangesPublic API Expansion and DataLayer Restructure
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/references.ts`:
- Line 30: FLOW_REF_PATTERN currently allows flow ids that start with digits or
contain hyphens causing false positives; update the regex to mirror REF_FLOW's
grammar by requiring the id to start with a letter and only contain word
characters (no hyphens). Replace const FLOW_REF_PATTERN = /\$flow\.([\w-]+)/g
with a pattern that enforces a leading ASCII letter followed by zero or more
alphanumeric/underscore characters (e.g. /\$flow\.([A-Za-z][A-Za-z0-9_]*)/g) so
FLOW_REF_PATTERN and REF_FLOW accept the same tokens.
- Around line 38-47: The recursive scanner scanFlowRefs can blow the stack on
cyclic object graphs; modify scanFlowRefs to accept or create a visited set
(preferably a WeakSet<object>) and mark each non-null object/array before
recursing, skipping recursion if the object is already visited; ensure you use
the same visited set across recursive calls and still return the accumulated
refs Set at the end (update the recursive call sites in scanFlowRefs to pass the
visited set along).
🪄 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: f1cbf5bd-106f-4c19-9229-b74ad925efe9
📒 Files selected for processing (14)
.changeset/collector-public-chain-helpers.md.changeset/datalayer-source-mapping-examples.mdapps/explorer/src/types/intellisense.tspackages/cli/examples/flow-complete.jsonpackages/cli/examples/flow-complete.mdpackages/collector/src/index.tspackages/core/src/__tests__/references.test.tspackages/core/src/__tests__/route.test.tspackages/core/src/index.tspackages/core/src/references.tspackages/core/src/route.tspackages/web/sources/dataLayer/src/examples/mapping.tswebsite/docs/guides/reference-syntax.mdxwebsite/docs/sources/web/dataLayer/index.mdx
| export const REF_SECRET = /^\$secret\.([A-Z0-9_]+)$/; | ||
| export const REF_CODE_PREFIX = '$code:'; | ||
|
|
||
| const FLOW_REF_PATTERN = /\$flow\.([\w-]+)/g; |
There was a problem hiding this comment.
Align FLOW_REF_PATTERN with REF_FLOW grammar to avoid false positives.
Line 30 currently matches tokens like $flow.1abc / $flow.a-2, while REF_FLOW (Lines 24-25) rejects them. This can produce flow references that the resolver cannot actually resolve.
Proposed fix
-const FLOW_REF_PATTERN = /\$flow\.([\w-]+)/g;
+const FLOW_REF_PATTERN =
+ /\$flow\.([a-zA-Z_][a-zA-Z0-9_]*)(?=\.|[^a-zA-Z0-9_-]|$)/g;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/references.ts` at line 30, FLOW_REF_PATTERN currently
allows flow ids that start with digits or contain hyphens causing false
positives; update the regex to mirror REF_FLOW's grammar by requiring the id to
start with a letter and only contain word characters (no hyphens). Replace const
FLOW_REF_PATTERN = /\$flow\.([\w-]+)/g with a pattern that enforces a leading
ASCII letter followed by zero or more alphanumeric/underscore characters (e.g.
/\$flow\.([A-Za-z][A-Za-z0-9_]*)/g) so FLOW_REF_PATTERN and REF_FLOW accept the
same tokens.
Comment on lines
+38
to
+47
| export function scanFlowRefs(value: unknown, into?: Set<string>): Set<string> { | ||
| const refs = into ?? new Set<string>(); | ||
| if (typeof value === 'string') { | ||
| for (const m of value.matchAll(FLOW_REF_PATTERN)) refs.add(m[1]); | ||
| return refs; | ||
| } | ||
| if (value && typeof value === 'object') { | ||
| for (const v of Object.values(value as Record<string, unknown>)) | ||
| scanFlowRefs(v, refs); | ||
| } |
There was a problem hiding this comment.
Guard recursive scanning against cyclic object graphs.
Lines 44-47 recurse without visited tracking; cyclic objects will recurse forever and crash with a stack overflow.
Proposed fix
export function scanFlowRefs(value: unknown, into?: Set<string>): Set<string> {
const refs = into ?? new Set<string>();
- if (typeof value === 'string') {
- for (const m of value.matchAll(FLOW_REF_PATTERN)) refs.add(m[1]);
- return refs;
- }
- if (value && typeof value === 'object') {
- for (const v of Object.values(value as Record<string, unknown>))
- scanFlowRefs(v, refs);
- }
+ const seen = new WeakSet<object>();
+
+ const visit = (current: unknown): void => {
+ if (typeof current === 'string') {
+ for (const m of current.matchAll(FLOW_REF_PATTERN)) refs.add(m[1]);
+ return;
+ }
+ if (!current || typeof current !== 'object') return;
+ if (seen.has(current as object)) return;
+ seen.add(current as object);
+ for (const v of Object.values(current as Record<string, unknown>)) visit(v);
+ };
+
+ visit(value);
return refs;
}📝 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
| export function scanFlowRefs(value: unknown, into?: Set<string>): Set<string> { | |
| const refs = into ?? new Set<string>(); | |
| if (typeof value === 'string') { | |
| for (const m of value.matchAll(FLOW_REF_PATTERN)) refs.add(m[1]); | |
| return refs; | |
| } | |
| if (value && typeof value === 'object') { | |
| for (const v of Object.values(value as Record<string, unknown>)) | |
| scanFlowRefs(v, refs); | |
| } | |
| export function scanFlowRefs(value: unknown, into?: Set<string>): Set<string> { | |
| const refs = into ?? new Set<string>(); | |
| const seen = new WeakSet<object>(); | |
| const visit = (current: unknown): void => { | |
| if (typeof current === 'string') { | |
| for (const m of current.matchAll(FLOW_REF_PATTERN)) refs.add(m[1]); | |
| return; | |
| } | |
| if (!current || typeof current !== 'object') return; | |
| if (seen.has(current as object)) return; | |
| seen.add(current as object); | |
| for (const v of Object.values(current as Record<string, unknown>)) visit(v); | |
| }; | |
| visit(value); | |
| return refs; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/references.ts` around lines 38 - 47, The recursive scanner
scanFlowRefs can blow the stack on cyclic object graphs; modify scanFlowRefs to
accept or create a visited set (preferably a WeakSet<object>) and mark each
non-null object/array before recursing, skipping recursion if the object is
already visited; ensure you use the same visited set across recursive calls and
still return the accumulated refs Set at the end (update the recursive call
sites in scanFlowRefs to pass the visited set along).
Contributor
Preview deployed |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
website/docs/core/server.mdx (1)
85-87: ⚡ Quick winDocument MD5 as compatibility-only here.
This sits directly under the privacy/compliance guidance, so please add a brief note that new integrations should keep
sha256and usemd5only when an external partner explicitly requires it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/core/server.mdx` around lines 85 - 87, Add a brief compatibility note under the privacy/compliance guidance next to the getHashServer API docs: explicitly state that the default algorithm is 'sha256' and that '{ algorithm: "md5" }' is supported only for compatibility with external partners and should not be used for new integrations; recommend keeping sha256 for all new integrations and only using md5 when an external party explicitly requires it, referencing the getHashServer signature and the algorithm option.packages/core/src/types/state.ts (1)
7-20: ⚡ Quick winMake
valuerequired for the currently supported modes.Both
'get'and'set'needvalue, sovalue?:lets invalid configs type-check and pushes the failure to runtime validation. A future'delete'mode can still be added later as a new union member.Suggested type shape
-export interface State { - /** Direction. 'delete' is reserved for a later release. */ - mode: 'get' | 'set'; - /** Store id; defaults to the in-memory `__cache` store when omitted. */ - store?: string; - /** Resolves against the event to the store key. */ - key: Mapping.Value; - /** - * set: resolves to the payload to store. - * get: its `key`/bare-string path is the event write-target. - * Optional at the type level to keep a future `delete` mode non-breaking; - * validation requires it for get/set. - */ - value?: Mapping.Value; -} +export interface State { + /** Direction. 'delete' is reserved for a later release. */ + mode: 'get' | 'set'; + /** Store id; defaults to the in-memory `__cache` store when omitted. */ + store?: string; + /** Resolves against the event to the store key. */ + key: Mapping.Value; + /** + * set: resolves to the payload to store. + * get: its `key`/bare-string path is the event write-target. + */ + value: Mapping.Value; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/types/state.ts` around lines 7 - 20, The State type allows invalid configs because value is optional; change the single interface State into a discriminated union so that for modes 'get' and 'set' the value property is required while still permitting an optional value for a future 'delete' mode. Concretely, replace the current interface State with a union like { mode: 'get' | 'set'; key: Mapping.Value; value: Mapping.Value; store?: string } | { mode: 'delete'; key: Mapping.Value; store?: string; value?: Mapping.Value } so callers of State (the State type, its value property, and the mode discriminant) will correctly type-check.packages/collector/src/__tests__/transformer-state.test.ts (1)
11-29: ⚡ Quick winThis case does not cover
statevsmappingorder yet.The title says
setruns after step mapping, but the step never uses themappingoption. As written it only proves thatstatesees the event returned frompush(), so a realmapping-ordering regression would still pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/collector/src/__tests__/transformer-state.test.ts` around lines 11 - 29, The test name claims "set runs after the step mapping" but the transformer 'enricher' never uses a mapping, so the test only proves state sees push() output; update the test to add a mapping on the enricher step that writes data.token (e.g. mapping: { 'data.token': '<some expression or source>' }) and remove token assignment from the push() implementation so the token is produced by the mapping, then keep the state config state: { mode: 'set', key: 'user.id', value: 'data.token' } and assert the stored value equals the mapping-written token; this ensures the ordering between mapping and state is actually exercised.apps/explorer/src/components/molecules/code-box.tsx (1)
505-572: ⚡ Quick winMove the new badge wrapper styles into SCSS.
These new wrappers add inline
style={{ position: 'relative' }}in the explorer UI. Please replace them with a class so the component stays within the styling rules and keeps presentation in SCSS.As per coding guidelines,
apps/explorer/src/**/*.{ts,tsx,scss}: "Don't use inlinestyleattributes in components."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/explorer/src/components/molecules/code-box.tsx` around lines 505 - 572, The div wrappers around the error and warning badges use inline style={{ position: 'relative' }}; replace these inline styles by adding a semantic CSS class (e.g. elb-codebox-marker-wrapper) on the divs that host ref={openMarkerMenu === 'error' ? markerMenuRef : undefined} and ref={openMarkerMenu === 'warning' ? markerMenuRef : undefined}, then move the position: relative rule into the component SCSS (targeting .elb-codebox-marker-wrapper) so markerCounts, openMarkerMenu, markerMenuRef, and the MarkerMenu usage remain unchanged in logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/explorer/src/components/molecules/code-box.tsx`:
- Around line 411-440: boxTabs is being invalidated because restCodeProps (and
settingsProps) are rebuilt each render; fix by replacing those broad objects in
the useMemo dependency list with the specific, stable props that Code actually
needs: explicitly destructure the individual properties you spread into the
<Code /> (from the rest destructure at the top of this file) into local
variables and use those variable names in the dependency array, then spread them
into <Code /> as before and remove restCodeProps and settingsProps from the deps
of the boxTabs useMemo so the memo only depends on stable, explicit Code props.
In `@packages/collector/src/destination.ts`:
- Around line 641-654: The current code applies state[set] immediately after
destinationPush(), but destinationPush() can return the BATCHED_RESULT sentinel
before the batch actually flushes, causing premature state writes; modify the
flow so applyState is not called for batched sends: detect when
destinationPush() returns BATCHED_RESULT and skip this dStateSet apply here
(remove/guard the block referencing applyState, dStateSet, getStateStore,
processedEvent), and instead invoke applyState from the successful flush path
(after pushBatch()/the batch success callback) so state writes occur only on
confirmed send completions.
In `@packages/collector/src/source.ts`:
- Around line 358-371: The current state application block (using stateEntries,
applyState, getStateStore, collector over events) runs unconditionally and
mutates/enriches events before the hasUserTerminalPush check; change it so
source-state processing is skipped when hasUserTerminalPush is true (i.e., guard
the block with if (!hasUserTerminalPush && stateEntries && stateEntries.length >
0) or move the applyState call into the collector-push path where collector.push
handles user-provided env.push), ensuring raw events are preserved for user
terminal pushes.
In `@packages/collector/src/transformer.ts`:
- Around line 243-249: The merge of definition-level state into context.config
(using transformerDef/state and configWithState) only sets context.config and
can be lost if a custom transformer factory returns a fresh instance.config;
after factory initialization you must propagate the resolved state into the
created transformer's config just like you already patch before/next: after the
factory returns (where you currently patch instance.before/instance.next), check
if transformerDef.state is defined and instance.config.state is undefined, and
copy transformerDef/state (or configWithState.state) into instance.config.state
so runTransformerChain() will always see the state on transformer.config.
- Around line 1052-1061: The state[set] write is currently unreachable for
branched transformer results because all `{ next }` branches and the array
fan-out return/continue before this block; move the settled-event write (the
applyState call that uses stateSet, getStateStore, processedEvent, collector) to
execute before any routing branches and ensure it is invoked for each fork of a
fan-out: call applyState immediately after computing stateSet (i.e., as soon as
stateSet is non-empty) and, when the transformer returns multiple outputs,
invoke applyState per fork with that fork's processedEvent prior to
dispatching/returning from any `{ next }` path so the write happens "before the
`next` dispatch."
In `@packages/core/src/schemas/state.ts`:
- Around line 45-76: The current validation for mode === 'get' accepts any
object that contains a string key even if it has extra operator fields; update
the check around data.value so that when value is an object and has a string key
you also verify Object.keys(value) is exactly ['key'] (or at least contains no
disallowed operator keys like 'fn','value','map','loop','set'), and if extra
keys exist call ctx.addIssue with the same 'custom' code and message explaining
only a bare key is allowed; keep the existing path.includes('*') check unchanged
and use the same ctx.addIssue pattern and path:['value'] to report the error.
In `@packages/core/src/state.ts`:
- Around line 43-74: The tryCatchAsync wrapper around the getMappingValue calls
(inside applyState) currently logs errors via collector.logger but never
rethrows, which causes FatalError from getMappingValue to be swallowed; update
the error handler passed to tryCatchAsync (the inline function that calls
collector.logger?.error) to detect FatalError (from getMappingValue) and rethrow
it (e.g., if (error instanceof FatalError) throw error) after logging so
FatalError propagates while other errors are still logged and swallowed;
reference the tryCatchAsync invocation in applyState, the getMappingValue calls,
and the collector.logger error handler.
In `@packages/core/src/types/source.ts`:
- Line 238: The new optional property added to the type (InitSource.state) is
not present in the runtime schema (InitSourceSchema), causing valid typed
configs to fail validation; update the schema in
packages/core/src/schemas/source.ts to include the same optional state shape as
declared in packages/core/src/types/source.ts (match import('./state').State or
State[] semantics), ensure the schema accepts both a single State and an array
of State, and keep naming aligned with InitSource/state so InitSourceSchema and
the InitSource type remain consistent.
In `@packages/server/destinations/criteo/src/__tests__/hash.test.ts`:
- Around line 64-68: The test "sha256_md5 is the SHA-256 of the md5 hex,
distinct from sha256" currently only checks format and inequality; update it to
compute the expected SHA-256 of the MD5 hex and assert equality: call
pushEmail('user@example.com'), compute
sha256(createHash('sha256').update(result.md5!).digest('hex')) and expect
result.sha256_md5 to equal that value, then keep the existing
expect(result.sha256_md5).not.toBe(result.sha256) to ensure it's distinct from
result.sha256; reference the test name, pushEmail, result.md5,
result.sha256_md5, and result.sha256 when making the changes.
---
Nitpick comments:
In `@apps/explorer/src/components/molecules/code-box.tsx`:
- Around line 505-572: The div wrappers around the error and warning badges use
inline style={{ position: 'relative' }}; replace these inline styles by adding a
semantic CSS class (e.g. elb-codebox-marker-wrapper) on the divs that host
ref={openMarkerMenu === 'error' ? markerMenuRef : undefined} and
ref={openMarkerMenu === 'warning' ? markerMenuRef : undefined}, then move the
position: relative rule into the component SCSS (targeting
.elb-codebox-marker-wrapper) so markerCounts, openMarkerMenu, markerMenuRef, and
the MarkerMenu usage remain unchanged in logic.
In `@packages/collector/src/__tests__/transformer-state.test.ts`:
- Around line 11-29: The test name claims "set runs after the step mapping" but
the transformer 'enricher' never uses a mapping, so the test only proves state
sees push() output; update the test to add a mapping on the enricher step that
writes data.token (e.g. mapping: { 'data.token': '<some expression or source>'
}) and remove token assignment from the push() implementation so the token is
produced by the mapping, then keep the state config state: { mode: 'set', key:
'user.id', value: 'data.token' } and assert the stored value equals the
mapping-written token; this ensures the ordering between mapping and state is
actually exercised.
In `@packages/core/src/types/state.ts`:
- Around line 7-20: The State type allows invalid configs because value is
optional; change the single interface State into a discriminated union so that
for modes 'get' and 'set' the value property is required while still permitting
an optional value for a future 'delete' mode. Concretely, replace the current
interface State with a union like { mode: 'get' | 'set'; key: Mapping.Value;
value: Mapping.Value; store?: string } | { mode: 'delete'; key: Mapping.Value;
store?: string; value?: Mapping.Value } so callers of State (the State type, its
value property, and the mode discriminant) will correctly type-check.
In `@website/docs/core/server.mdx`:
- Around line 85-87: Add a brief compatibility note under the privacy/compliance
guidance next to the getHashServer API docs: explicitly state that the default
algorithm is 'sha256' and that '{ algorithm: "md5" }' is supported only for
compatibility with external partners and should not be used for new
integrations; recommend keeping sha256 for all new integrations and only using
md5 when an external party explicitly requires it, referencing the getHashServer
signature and the algorithm option.
🪄 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: ae1f7e39-6d21-4170-bb8a-00b513c65a94
📒 Files selected for processing (40)
.changeset/codebox-toolbar-memo.md.changeset/gethashserver-algorithm-option.md.changeset/state-block.mdapps/explorer/src/components/molecules/code-box.tsxpackages/collector/src/__tests__/destination-state.test.tspackages/collector/src/__tests__/source-state.test.tspackages/collector/src/__tests__/state-store.test.tspackages/collector/src/__tests__/transformer-state.test.tspackages/collector/src/cache.tspackages/collector/src/destination.tspackages/collector/src/source.tspackages/collector/src/transformer.tspackages/core/src/__tests__/state.test.tspackages/core/src/__tests__/step-entry.test.tspackages/core/src/index.tspackages/core/src/schemas/__tests__/state.test.tspackages/core/src/schemas/destination.tspackages/core/src/schemas/index.tspackages/core/src/schemas/source.tspackages/core/src/schemas/state.tspackages/core/src/schemas/transformer.tspackages/core/src/state.tspackages/core/src/step-entry.tspackages/core/src/types/destination.tspackages/core/src/types/index.tspackages/core/src/types/source.tspackages/core/src/types/state.tspackages/core/src/types/transformer.tspackages/server/core/src/__tests__/getHashServer.test.tspackages/server/core/src/getHashServer.tspackages/server/destinations/criteo/src/__tests__/hash.test.tspackages/server/destinations/criteo/src/hash.tspackages/server/destinations/criteo/src/push.tsskills/walkeros-understanding-stores/SKILL.mdskills/walkeros-understanding-transformers/SKILL.mdskills/walkeros-using-store-cache/SKILL.mdwebsite/docs/collector/state.mdxwebsite/docs/core/server.mdxwebsite/docs/stores/index.mdxwebsite/docs/transformers/index.mdx
💤 Files with no reviewable changes (1)
- packages/server/destinations/criteo/src/hash.ts
✅ Files skipped from review due to trivial changes (10)
- website/docs/transformers/index.mdx
- .changeset/state-block.md
- .changeset/gethashserver-algorithm-option.md
- website/docs/stores/index.mdx
- skills/walkeros-understanding-stores/SKILL.md
- skills/walkeros-using-store-cache/SKILL.md
- skills/walkeros-understanding-transformers/SKILL.md
- .changeset/codebox-toolbar-memo.md
- packages/core/src/types/index.ts
- website/docs/collector/state.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/index.ts
Comment on lines
+411
to
+440
| const boxTabs = useMemo( | ||
| () => | ||
| tabs?.map((tab) => ({ | ||
| id: tab.id, | ||
| label: tab.label, | ||
| content: ( | ||
| <Code | ||
| code={tab.code} | ||
| language={tab.language ?? language} | ||
| onChange={onChange} | ||
| disabled={disabled} | ||
| autoHeight={autoHeight} | ||
| onMount={handleEditorMount} | ||
| onMarkerCounts={handleMarkerCounts} | ||
| {...restCodeProps} | ||
| {...settingsProps} | ||
| /> | ||
| ), | ||
| })), | ||
| [ | ||
| tabs, | ||
| language, | ||
| onChange, | ||
| disabled, | ||
| autoHeight, | ||
| settingsProps, | ||
| restCodeProps, | ||
| handleEditorMount, | ||
| handleMarkerCounts, | ||
| ], |
There was a problem hiding this comment.
restCodeProps still invalidates this memo on every render.
restCodeProps is rebuilt by the object rest destructure at Line 133, so keeping it in this dependency list means boxTabs is recreated on every parent render. That defeats the “stable Monaco subtree” goal and can still reflash tabs when marker state changes. Please depend on stable, explicit Code props here instead of the spread object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/explorer/src/components/molecules/code-box.tsx` around lines 411 - 440,
boxTabs is being invalidated because restCodeProps (and settingsProps) are
rebuilt each render; fix by replacing those broad objects in the useMemo
dependency list with the specific, stable props that Code actually needs:
explicitly destructure the individual properties you spread into the <Code />
(from the rest destructure at the top of this file) into local variables and use
those variable names in the dependency array, then spread them into <Code /> as
before and remove restCodeProps and settingsProps from the deps of the boxTabs
useMemo so the memo only depends on stable, explicit Code props.
Comment on lines
+641
to
+654
| // state[set]: stash from the event after a successful send. | ||
| if ( | ||
| !pushFailed && | ||
| dStateSet && | ||
| dStateSet.length > 0 && | ||
| processedEvent | ||
| ) { | ||
| processedEvent = await applyState( | ||
| dStateSet, | ||
| (storeId) => getStateStore(storeId, collector), | ||
| processedEvent, | ||
| collector, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Delay state[set] until the batch actually flushes.
destinationPush() returns BATCHED_RESULT as soon as the event is queued. This block then writes state before pushBatch() runs, so a later batch failure still leaves the store mutated. That breaks the new "set-after-send" contract for batched destinations. At minimum, skip the sentinel here; the real fix is to apply the write from the successful flush path instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/collector/src/destination.ts` around lines 641 - 654, The current
code applies state[set] immediately after destinationPush(), but
destinationPush() can return the BATCHED_RESULT sentinel before the batch
actually flushes, causing premature state writes; modify the flow so applyState
is not called for batched sends: detect when destinationPush() returns
BATCHED_RESULT and skip this dStateSet apply here (remove/guard the block
referencing applyState, dStateSet, getStateStore, processedEvent), and instead
invoke applyState from the successful flush path (after pushBatch()/the batch
success callback) so state writes occur only on confirmed send completions.
Comment on lines
+358
to
+371
| // Apply declarative state per event before handing off to the collector. | ||
| // Entries run sequentially in array order at this single pipeline point. | ||
| if (stateEntries && stateEntries.length > 0) { | ||
| events = await Promise.all( | ||
| events.map((event) => | ||
| applyState( | ||
| stateEntries, | ||
| (id) => getStateStore(id, collector), | ||
| event, | ||
| collector, | ||
| ), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Skip source-state processing for env.push overrides.
This now mutates/enriches events before the hasUserTerminalPush branch, but the surrounding contract in this file still says a user-provided env.push bypasses the pipeline and receives the raw event. That semantic change can break custom sinks and existing test spies that rely on pre-pipeline input. Gate this block behind !hasUserTerminalPush, or move it into the collector-push path only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/collector/src/source.ts` around lines 358 - 371, The current state
application block (using stateEntries, applyState, getStateStore, collector over
events) runs unconditionally and mutates/enriches events before the
hasUserTerminalPush check; change it so source-state processing is skipped when
hasUserTerminalPush is true (i.e., guard the block with if (!hasUserTerminalPush
&& stateEntries && stateEntries.length > 0) or move the applyState call into the
collector-push path where collector.push handles user-provided env.push),
ensuring raw events are preserved for user terminal pushes.
Comment on lines
+243
to
+249
| // Merge definition-level state into config for runtime access. A | ||
| // config-level `state` (if present) takes precedence. | ||
| const { state } = transformerDef; | ||
| const configWithState = | ||
| state !== undefined && configWithCache.state === undefined | ||
| ? { ...configWithCache, state } | ||
| : configWithCache; |
There was a problem hiding this comment.
Definition-level state can still disappear after factory init.
This only adds state to context.config. If a custom transformer factory returns a fresh instance.config instead of spreading ctx.config, runTransformerChain() later reads transformer.config.state as undefined. You already patch before/next after init for this exact pattern; state needs the same post-init propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/collector/src/transformer.ts` around lines 243 - 249, The merge of
definition-level state into context.config (using transformerDef/state and
configWithState) only sets context.config and can be lost if a custom
transformer factory returns a fresh instance.config; after factory
initialization you must propagate the resolved state into the created
transformer's config just like you already patch before/next: after the factory
returns (where you currently patch instance.before/instance.next), check if
transformerDef.state is defined and instance.config.state is undefined, and copy
transformerDef/state (or configWithState.state) into instance.config.state so
runTransformerChain() will always see the state on transformer.config.
Comment on lines
+1052
to
+1061
| // state[set]: write to the store from the settled event, after the | ||
| // mapping and before the `next` dispatch. | ||
| if (stateSet && stateSet.length > 0) { | ||
| processedEvent = await applyState( | ||
| stateSet, | ||
| (id) => getStateStore(id, collector), | ||
| processedEvent, | ||
| collector, | ||
| ); | ||
| } |
There was a problem hiding this comment.
state[set] is unreachable for branched transformer results.
Every { next } path above returns or continues before this block, and array fan-out exits even earlier. So state writes are skipped on the exact paths where the new feature says they should happen "before the next dispatch". Please apply the settled-event write before those routing branches, and per fork when the transformer fans out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/collector/src/transformer.ts` around lines 1052 - 1061, The
state[set] write is currently unreachable for branched transformer results
because all `{ next }` branches and the array fan-out return/continue before
this block; move the settled-event write (the applyState call that uses
stateSet, getStateStore, processedEvent, collector) to execute before any
routing branches and ensure it is invoked for each fork of a fan-out: call
applyState immediately after computing stateSet (i.e., as soon as stateSet is
non-empty) and, when the transformer returns multiple outputs, invoke applyState
per fork with that fork's processedEvent prior to dispatching/returning from any
`{ next }` path so the write happens "before the `next` dispatch."
Comment on lines
+45
to
+76
| if (data.mode === 'get') { | ||
| const value: unknown = data.value; | ||
| let path: string | undefined; | ||
| if (typeof value === 'string') { | ||
| path = value; | ||
| } else if ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| !Array.isArray(value) && | ||
| 'key' in value && | ||
| typeof value.key === 'string' | ||
| ) { | ||
| path = value.key; | ||
| } | ||
|
|
||
| if (path === undefined) { | ||
| ctx.addIssue({ | ||
| code: 'custom', | ||
| message: | ||
| 'For mode "get", `value` must be a bare string path or a ValueConfig with a `key` (no value/fn/map/loop/set).', | ||
| path: ['value'], | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (path.includes('*')) { | ||
| ctx.addIssue({ | ||
| code: 'custom', | ||
| message: 'For mode "get", the `value` path may not contain `*`.', | ||
| path: ['value'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Reject mixed ValueConfig shapes for mode: "get".
This only checks for a string key, so configs like { key: 'data.fetched', fn: '...' } or { key: 'data.fetched', value: 'static' } still pass. The runtime contract here is "write fetched data to this path", so those extra operators will be silently ignored even though the schema/docs say they are invalid.
Suggested tightening
if (data.mode === 'get') {
const value: unknown = data.value;
let path: string | undefined;
if (typeof value === 'string') {
path = value;
} else if (
typeof value === 'object' &&
value !== null &&
!Array.isArray(value) &&
'key' in value &&
typeof value.key === 'string'
) {
+ const extraKeys = ['value', 'fn', 'map', 'loop', 'set'].filter(
+ (key) => key in value,
+ );
+ if (extraKeys.length > 0) {
+ ctx.addIssue({
+ code: 'custom',
+ message:
+ 'For mode "get", `value` may only provide a `key` path.',
+ path: ['value'],
+ });
+ return;
+ }
path = value.key;
}📝 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 (data.mode === 'get') { | |
| const value: unknown = data.value; | |
| let path: string | undefined; | |
| if (typeof value === 'string') { | |
| path = value; | |
| } else if ( | |
| typeof value === 'object' && | |
| value !== null && | |
| !Array.isArray(value) && | |
| 'key' in value && | |
| typeof value.key === 'string' | |
| ) { | |
| path = value.key; | |
| } | |
| if (path === undefined) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| message: | |
| 'For mode "get", `value` must be a bare string path or a ValueConfig with a `key` (no value/fn/map/loop/set).', | |
| path: ['value'], | |
| }); | |
| return; | |
| } | |
| if (path.includes('*')) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| message: 'For mode "get", the `value` path may not contain `*`.', | |
| path: ['value'], | |
| }); | |
| } | |
| if (data.mode === 'get') { | |
| const value: unknown = data.value; | |
| let path: string | undefined; | |
| if (typeof value === 'string') { | |
| path = value; | |
| } else if ( | |
| typeof value === 'object' && | |
| value !== null && | |
| !Array.isArray(value) && | |
| 'key' in value && | |
| typeof value.key === 'string' | |
| ) { | |
| const extraKeys = ['value', 'fn', 'map', 'loop', 'set'].filter( | |
| (key) => key in value, | |
| ); | |
| if (extraKeys.length > 0) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| message: | |
| 'For mode "get", `value` may only provide a `key` path.', | |
| path: ['value'], | |
| }); | |
| return; | |
| } | |
| path = value.key; | |
| } | |
| if (path === undefined) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| message: | |
| 'For mode "get", `value` must be a bare string path or a ValueConfig with a `key` (no value/fn/map/loop/set).', | |
| path: ['value'], | |
| }); | |
| return; | |
| } | |
| if (path.includes('*')) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| message: 'For mode "get", the `value` path may not contain `*`.', | |
| path: ['value'], | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/schemas/state.ts` around lines 45 - 76, The current
validation for mode === 'get' accepts any object that contains a string key even
if it has extra operator fields; update the check around data.value so that when
value is an object and has a string key you also verify Object.keys(value) is
exactly ['key'] (or at least contains no disallowed operator keys like
'fn','value','map','loop','set'), and if extra keys exist call ctx.addIssue with
the same 'custom' code and message explaining only a bare key is allowed; keep
the existing path.includes('*') check unchanged and use the same ctx.addIssue
pattern and path:['value'] to report the error.
Comment on lines
+43
to
+74
| await tryCatchAsync( | ||
| async () => { | ||
| const store = getStore(entry.store); | ||
| if (!store) return; // unknown store is rejected at validation; guard anyway | ||
|
|
||
| const rawKey = await getMappingValue(result, entry.key, { | ||
| collector, | ||
| event: result, | ||
| }); | ||
| if (!isString(rawKey)) return; | ||
| // namespace state keys on the shared default store to avoid cache collisions | ||
| const key = entry.store ? rawKey : `state:${rawKey}`; | ||
|
|
||
| if (entry.mode === 'set') { | ||
| const payload = await getMappingValue(result, entry.value, { | ||
| collector, | ||
| event: result, | ||
| }); | ||
| if (!isDefined(payload)) return; // skip writing undefined | ||
| await store.set(key, payload); | ||
| } else { | ||
| const targetPath = resolveTargetPath(entry.value); | ||
| if (!targetPath) return; // validation already rejects this | ||
| const fetched = await store.get(key); | ||
| if (!isDefined(fetched)) return; // miss: leave event unchanged | ||
| result = setByPath(result, targetPath, fetched); | ||
| } | ||
| }, | ||
| (error) => { | ||
| collector.logger?.error?.('[state] operation failed', error); | ||
| }, | ||
| )(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/core/src/tryCatch.ts
sed -n '1,220p' packages/core/src/fatalError.ts
sed -n '1,260p' packages/core/src/state.ts
rg -n -C3 'tryCatchAsync|FatalError|fatal' packages/core/srcRepository: elbwalker/walkerOS
Length of output: 17122
Fix: applyState swallows FatalError from getMappingValue
applyState wraps both getMappingValue(...) calls in tryCatchAsync, but the tryCatchAsync error handler in applyState only logs and never rethrows. Since getMappingValue can throw FatalError (it rethrows when the caught error is a FatalError), those FatalErrors get converted into fail-open behavior, contradicting the docblock claim that only FatalError rethrows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/state.ts` around lines 43 - 74, The tryCatchAsync wrapper
around the getMappingValue calls (inside applyState) currently logs errors via
collector.logger but never rethrows, which causes FatalError from
getMappingValue to be swallowed; update the error handler passed to
tryCatchAsync (the inline function that calls collector.logger?.error) to detect
FatalError (from getMappingValue) and rethrow it (e.g., if (error instanceof
FatalError) throw error) after logging so FatalError propagates while other
errors are still logged and swallowed; reference the tryCatchAsync invocation in
applyState, the getMappingValue calls, and the collector.logger error handler.
| next?: Route; | ||
| before?: Route; | ||
| cache?: import('./cache').Cache; | ||
| state?: import('./state').State | import('./state').State[]; |
There was a problem hiding this comment.
Type/runtime contract drift for InitSource.state.
Line 238 adds InitSource.state to the public type, but packages/core/src/schemas/source.ts InitSourceSchema still omits that field, so valid typed configs can fail schema validation.
Suggested fix
diff --git a/packages/core/src/schemas/source.ts b/packages/core/src/schemas/source.ts
@@
export const InitSourceSchema = z
.object({
code: InitSchema.describe('Source initialization function'),
config: PartialConfigSchema.optional().describe(
'Partial configuration overrides',
),
env: BaseEnvSchema.partial()
.optional()
.describe('Partial environment overrides'),
+ state: z
+ .union([StateSchema, z.array(StateSchema)])
+ .optional()
+ .describe(
+ 'Declarative store get/set operations applied around this source',
+ ),
primary: z
.boolean()
.optional()
.describe('Mark as primary source (only one can be primary)'),
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/types/source.ts` at line 238, The new optional property
added to the type (InitSource.state) is not present in the runtime schema
(InitSourceSchema), causing valid typed configs to fail validation; update the
schema in packages/core/src/schemas/source.ts to include the same optional state
shape as declared in packages/core/src/types/source.ts (match
import('./state').State or State[] semantics), ensure the schema accepts both a
single State and an array of State, and keep naming aligned with
InitSource/state so InitSourceSchema and the InitSource type remain consistent.
Comment on lines
+64
to
68
| test('sha256_md5 is the SHA-256 of the md5 hex, distinct from sha256', async () => { | ||
| const result = await pushEmail('user@example.com'); | ||
| expect(result.sha256_md5).toMatch(/^[a-f0-9]{64}$/); | ||
| // Distinct from sha256 of the raw email | ||
| expect(result.sha256_md5).not.toBe(result.sha256); | ||
| }); |
There was a problem hiding this comment.
Assert the actual sha256(md5Hex) value here.
This only checks that sha256_md5 is 64 hex chars and differs from sha256, so an unrelated digest would still pass. Please compute the expected SHA-256 of the MD5 hex and compare directly.
🔍 Suggested tightening
import { createHash } from 'node:crypto';
// ...
test('sha256_md5 is the SHA-256 of the md5 hex, distinct from sha256', async () => {
const result = await pushEmail('user@example.com');
expect(result.sha256_md5).toBe(
createHash('sha256').update(result.md5!).digest('hex'),
);
expect(result.sha256_md5).not.toBe(result.sha256);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/destinations/criteo/src/__tests__/hash.test.ts` around lines
64 - 68, The test "sha256_md5 is the SHA-256 of the md5 hex, distinct from
sha256" currently only checks format and inequality; update it to compute the
expected SHA-256 of the MD5 hex and assert equality: call
pushEmail('user@example.com'), compute
sha256(createHash('sha256').update(result.md5!).digest('hex')) and expect
result.sha256_md5 to equal that value, then keep the existing
expect(result.sha256_md5).not.toBe(result.sha256) to ensure it's distinct from
result.sha256; reference the test name, pushEmail, result.md5,
result.sha256_md5, and result.sha256 when making the changes.
Contributor
|
📦 Pre-release published (next) 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