refactor(google_adwords_remarketing_lists): migrate to TypeScript#5037
Merged
shekhar-rudder merged 4 commits intodevelopfrom Mar 13, 2026
Merged
Conversation
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Config & Exports src/v0/destinations/google_adwords_remarketing_lists/config.ts |
Switched to ES module exports, added explicit types for TYPEOFLIST, attributeMapping, consentConfigMap, added and exported offlineDataJobsMapping, addressInfoMapping, and destType. |
Network / HTTP flow src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts |
Converted to ES imports/exports and added detailed TypeScript signatures for networkHandler, proxy request (gaAudienceProxyRequest), job lifecycle (createJob, addUserToJob, runTheJob) and response handlers; explicit this typing for bound helpers. |
Record processing src/v0/destinations/google_adwords_remarketing_lists/recordTransform.ts |
Migrated to ES modules, added RecordInput/RecordEventContext types, annotated internal processing functions, added safer defaults/non-null assertions, exported processRecordInputs as named export. |
Transform logic src/v0/destinations/google_adwords_remarketing_lists/transform.ts |
Replaced CommonJS with ES modules and added TS types for createPayload, processEvent, process, and processRouterDest; adjusted payload typing and named exports. |
Types added src/v0/destinations/google_adwords_remarketing_lists/types.ts |
New file: declares and exports GARLDestinationConfig, RecordEventContext, Message, RecordInput, OfflineDataJobPayload, and GARLDestination. |
Utilities & tests src/v0/destinations/google_adwords_remarketing_lists/util.ts, src/v0/destinations/google_adwords_remarketing_lists/util.test.ts |
Converted to ES modules, added/adjusted TypeScript signatures for utilities (populateIdentifiers, responseBuilder, etc.), tightened null checks and error typing; tests updated for ES imports and explicit error casting. |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
- saikumarrs
- achettyiitr
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description is incomplete relative to the template. It lacks the Linear task reference, broader objectives explanation, developer/reviewer checklists, and several required sections. | Add the missing template sections including: Linear task reference (Resolves INT-XXX), broader objectives explanation, and complete both developer and reviewer checklists. | |
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main change: migrating the google_adwords_remarketing_lists destination to TypeScript with proper type annotations. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/int-6071-migrate-garl-transformer-code-to-typescript
📝 Coding Plan
- Generate coding plan for human review comments
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/v0/destinations/google_adwords_remarketing_lists/util.ts (1)
26-33:⚠️ Potential issue | 🟡 MinorType safety:
sha256expects a string argument.The
object[key]value is typed asunknown, butsha256typically expects astring. If a non-string value is passed, it may produce unexpected results or runtime errors.🛡️ Suggested fix to ensure string input
const hashEncrypt = (object: Record<string, unknown>) => { Object.keys(object).forEach((key) => { - if (hashAttributes.includes(key) && object[key]) { + if (hashAttributes.includes(key) && object[key] && typeof object[key] === 'string') { // eslint-disable-next-line no-param-reassign - object[key] = sha256(object[key]); + object[key] = sha256(object[key] as string); } }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/util.ts` around lines 26 - 33, The hashEncrypt function calls sha256 with object[key] typed as unknown; ensure type safety by checking that the value is a non-empty string before hashing (use typeof object[key] === 'string' && object[key] !== ''), and then pass it to sha256 with a string type assertion or after assigning to a typed const; reference the hashEncrypt function, the hashAttributes array check, and the sha256 call when making this change so only string values are hashed and non-strings are left unchanged.
🧹 Nitpick comments (5)
src/v0/destinations/google_adwords_remarketing_lists/transform.ts (2)
152-152: Input type is too narrow forsimpleProcessRouterDestusage.The parameter type
{ message: Message }[]only exposesmessage, but per the context snippet fromsrc/v0/util/index.js:1819-1838,simpleProcessRouterDestalso accessesinput.metadataandinput.destination. While this works at runtime (JS doesn't enforce types), the typing doesn't reflect the actual data structure.Consider using a more complete type:
♻️ Suggested type improvement
-const processRouterDest = async (inputs: { message: Message }[], reqMetadata: unknown) => { +const processRouterDest = async ( + inputs: { message: Message; metadata: Record<string, unknown>; destination: GARLDestination }[], + reqMetadata: unknown, +) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts` at line 152, The parameter type for processRouterDest is too narrow—currently declared as inputs: { message: Message }[] but simpleProcessRouterDest also expects input.metadata and input.destination; update the inputs type used by processRouterDest (and any local aliases) to a full input shape that includes message, metadata, and destination (or reuse the existing shared Input type/interface used elsewhere) so that callers of simpleProcessRouterDest have correct typings for processRouterDest and related functions.
166-168: Double type assertion bypasses type safety.
as unknown as RecordInput[]effectively disables type checking for this conversion. This pattern can mask genuine type mismatches between the grouped inputs andRecordInput.If the structures genuinely differ, consider narrowing with a type guard or using a proper mapping function. If they match, a single assertion should suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts` around lines 166 - 168, The code is using a double type assertion ("as unknown as RecordInput[]") which bypasses type safety; update the call to processRecordInputs by removing the double-cast and either (a) narrow groupedInputs.record to RecordInput[] with a single cast if you are certain of the type, or (b) implement a small type guard or mapping step that converts/validates groupedInputs.record into the RecordInput shape before passing it to processRecordInputs; reference groupedInputs.record and the processRecordInputs function and prefer explicit validation/mapping over the "unknown as" bypass.src/v0/destinations/google_adwords_remarketing_lists/util.test.ts (2)
179-194: Same issue: unreachable assertion in the second error test.The
expect(response).toEqual(undefined)on line 190 will never execute when the error is thrown.♻️ Suggested refactor
it('Should throw error if operationAudienceId is not defined', () => { - try { - const destination1 = Object.create(baseDestination); - destination1.Config.audienceId = ''; - const response = responseBuilder( + const destination1 = Object.create(baseDestination); + destination1.Config.audienceId = ''; + expect(() => + responseBuilder( accessToken, body, destination1, getOperationAudienceId(baseDestination.Config.audienceId, message), consentBlock, - ); - expect(response).toEqual(undefined); - } catch (error: unknown) { - expect((error as Error).message).toEqual(`List ID is a mandatory field`); - } + ), + ).toThrow('List ID is a mandatory field'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/util.test.ts` around lines 179 - 194, The test contains an unreachable assertion because it calls responseBuilder inside a try block and then asserts expect(response).toEqual(undefined) even though an exception is expected; update the test for the scenario using the functions responseBuilder and getOperationAudienceId (and the test setup using baseDestination / destination1) to assert the thrown error instead of using try/catch — e.g., replace the try/catch pattern with an expectation that invoking responseBuilder(...) throws and assert the error message equals "List ID is a mandatory field".
159-176: Dead assertion:expect(response).toEqual(undefined)is unreachable.If
responseBuilderthrows (as expected), execution jumps to the catch block and line 171 is never reached. If it doesn't throw, the test would incorrectly pass. Consider restructuring withexpect(...).toThrow():♻️ Suggested refactor using expect().toThrow()
it('Should throw error if subaccount is true and loginCustomerId is not defined', () => { - try { - const destination2 = Object.create(baseDestination); - destination2.Config.subAccount = true; - destination2.Config.loginCustomerId = ''; - const response = responseBuilder( + const destination2 = Object.create(baseDestination); + destination2.Config.subAccount = true; + destination2.Config.loginCustomerId = ''; + expect(() => + responseBuilder( accessToken, body, destination2, getOperationAudienceId(baseDestination.Config.audienceId, message), consentBlock, - ); - expect(response).toEqual(undefined); - } catch (error: unknown) { - expect((error as Error).message).toEqual( - `loginCustomerId is required as subAccount is true.`, - ); - } + ), + ).toThrow('loginCustomerId is required as subAccount is true.'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/util.test.ts` around lines 159 - 176, The test contains a dead assertion because it uses try/catch around responseBuilder and then asserts on a response that will never be reached when an error is thrown; replace the try/catch pattern with Jest's expect(...).toThrow() by invoking responseBuilder inside a function wrapper and asserting that it throws the message `loginCustomerId is required as subAccount is true.`; update the test case that references responseBuilder and getOperationAudienceId so it uses expect(() => responseBuilder(...)).toThrow('loginCustomerId is required as subAccount is true.') (or the appropriate error text) instead of the unreachable expect(response).toEqual(undefined).src/v0/destinations/google_adwords_remarketing_lists/recordTransform.ts (1)
212-218: Type assertion may not accurately represent the transformed structure.The mapped records have
fieldsset torecord.message.identifiers, butRecordInput.message.fieldsis typed asRecord<string, unknown>whileidentifiersisRecord<string, string | number>. The assertionas RecordInput[]works but could hide type mismatches if the structure evolves.This is acceptable for now, but consider defining a more precise intermediate type if this pattern is reused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/recordTransform.ts` around lines 212 - 218, The current mapInBatches call constructs records where message.fields is assigned from message.identifiers, but the result is force-cast to RecordInput[] which can hide mismatches; update the transformation to use a precise intermediate type instead of as RecordInput[] (e.g., define an interface where message.fields: Record<string, string|number> or a new TransformedRecord type) and make mapInBatches return that typed array (or explicitly map/convert identifiers to the expected Record<string, unknown> shape) so the types of groupedRecordInputs, mapInBatches, and the resulting records align (refer to mapInBatches, groupedRecordInputs, RecordInput, message.fields, and message.identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts`:
- Around line 173-184: The code currently assumes jobId exists by using jobId!
when calling addUserToJob; instead validate and extract it safely from
firstResponse.response.data.resourceName (check existence and that split('/')
has at least 4 segments) before calling addUserToJob, and handle the
missing/malformed case by throwing/logging a clear error or returning early;
update the jobId assignment logic near the firstResponse handling and replace
the unsafe jobId! usage in the addUserToJob call with the validated jobId
variable (or bail out) so addUserToJob is never invoked with undefined.
In `@src/v0/destinations/google_adwords_remarketing_lists/recordTransform.ts`:
- Line 51: The code uses a non-null assertion on constructPayload (const
outputPayload = constructPayload(message, offlineDataJobsMapping)!) which can
crash if it returns null/undefined; update recordTransform to check the return
value of constructPayload (referencing constructPayload, outputPayload, and
offlineDataJobsMapping) and either throw a descriptive error (e.g., "Failed to
construct payload: mapping mismatch") or return a handled error/empty result
before accessing outputPayload.operations, ensuring downstream code only runs
when outputPayload is truthy.
In `@src/v0/destinations/google_adwords_remarketing_lists/util.ts`:
- Around line 158-169: getOperationAudienceId currently uses a non-null
assertion on objectType returned by getDestinationExternalIDInfoForRetl which
can mask undefined values; replace the objectType! usage with defensive
handling: call getDestinationExternalIDInfoForRetl in getOperationAudienceId,
check whether objectType is defined before assigning to operationAudienceId, and
if undefined emit a debug/warn log (or return undefined) so downstream code
doesn't receive a falsely asserted value; update references in
getOperationAudienceId, use MappedToDestinationKey when deciding to call
getDestinationExternalIDInfoForRetl, and avoid any non-null assertions on
objectType.
---
Outside diff comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/util.ts`:
- Around line 26-33: The hashEncrypt function calls sha256 with object[key]
typed as unknown; ensure type safety by checking that the value is a non-empty
string before hashing (use typeof object[key] === 'string' && object[key] !==
''), and then pass it to sha256 with a string type assertion or after assigning
to a typed const; reference the hashEncrypt function, the hashAttributes array
check, and the sha256 call when making this change so only string values are
hashed and non-strings are left unchanged.
---
Nitpick comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/recordTransform.ts`:
- Around line 212-218: The current mapInBatches call constructs records where
message.fields is assigned from message.identifiers, but the result is
force-cast to RecordInput[] which can hide mismatches; update the transformation
to use a precise intermediate type instead of as RecordInput[] (e.g., define an
interface where message.fields: Record<string, string|number> or a new
TransformedRecord type) and make mapInBatches return that typed array (or
explicitly map/convert identifiers to the expected Record<string, unknown>
shape) so the types of groupedRecordInputs, mapInBatches, and the resulting
records align (refer to mapInBatches, groupedRecordInputs, RecordInput,
message.fields, and message.identifiers).
In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts`:
- Line 152: The parameter type for processRouterDest is too narrow—currently
declared as inputs: { message: Message }[] but simpleProcessRouterDest also
expects input.metadata and input.destination; update the inputs type used by
processRouterDest (and any local aliases) to a full input shape that includes
message, metadata, and destination (or reuse the existing shared Input
type/interface used elsewhere) so that callers of simpleProcessRouterDest have
correct typings for processRouterDest and related functions.
- Around line 166-168: The code is using a double type assertion ("as unknown as
RecordInput[]") which bypasses type safety; update the call to
processRecordInputs by removing the double-cast and either (a) narrow
groupedInputs.record to RecordInput[] with a single cast if you are certain of
the type, or (b) implement a small type guard or mapping step that
converts/validates groupedInputs.record into the RecordInput shape before
passing it to processRecordInputs; reference groupedInputs.record and the
processRecordInputs function and prefer explicit validation/mapping over the
"unknown as" bypass.
In `@src/v0/destinations/google_adwords_remarketing_lists/util.test.ts`:
- Around line 179-194: The test contains an unreachable assertion because it
calls responseBuilder inside a try block and then asserts
expect(response).toEqual(undefined) even though an exception is expected; update
the test for the scenario using the functions responseBuilder and
getOperationAudienceId (and the test setup using baseDestination / destination1)
to assert the thrown error instead of using try/catch — e.g., replace the
try/catch pattern with an expectation that invoking responseBuilder(...) throws
and assert the error message equals "List ID is a mandatory field".
- Around line 159-176: The test contains a dead assertion because it uses
try/catch around responseBuilder and then asserts on a response that will never
be reached when an error is thrown; replace the try/catch pattern with Jest's
expect(...).toThrow() by invoking responseBuilder inside a function wrapper and
asserting that it throws the message `loginCustomerId is required as subAccount
is true.`; update the test case that references responseBuilder and
getOperationAudienceId so it uses expect(() =>
responseBuilder(...)).toThrow('loginCustomerId is required as subAccount is
true.') (or the appropriate error text) instead of the unreachable
expect(response).toEqual(undefined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80f35233-4f50-4d89-8cb6-ed63641537a5
📒 Files selected for processing (7)
src/v0/destinations/google_adwords_remarketing_lists/config.tssrc/v0/destinations/google_adwords_remarketing_lists/networkHandler.tssrc/v0/destinations/google_adwords_remarketing_lists/recordTransform.tssrc/v0/destinations/google_adwords_remarketing_lists/transform.tssrc/v0/destinations/google_adwords_remarketing_lists/types.tssrc/v0/destinations/google_adwords_remarketing_lists/util.test.tssrc/v0/destinations/google_adwords_remarketing_lists/util.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5037 +/- ##
========================================
Coverage 92.28% 92.28%
========================================
Files 657 657
Lines 35955 35979 +24
Branches 8483 8484 +1
========================================
+ Hits 33180 33204 +24
Misses 2537 2537
Partials 238 238 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maheshkutty
reviewed
Mar 13, 2026
src/v0/destinations/google_adwords_remarketing_lists/transform.ts
Outdated
Show resolved
Hide resolved
maheshkutty
reviewed
Mar 13, 2026
… on output payload type - Add OfflineDataJobPayload concrete type to types.ts - Replace Record<string, unknown> with Partial<Record<'create' | 'remove', OfflineDataJobPayload>> in createPayload Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…yload type Remove unused optional fields (validateOnly, enablePartialFailure, enableWarnings) from OfflineDataJobPayload interface Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/v0/destinations/google_adwords_remarketing_lists/transform.ts (1)
172-176: Pass explicit emptyprocessParamsinstead ofundefinedforsimpleProcessRouterDest.Using
{}here makes intent explicit (intentionally no extra process params) and avoids relying on callee defaults.Small clarity tweak
transformedAudienceEvent = await simpleProcessRouterDest( groupedInputs.audiencelist, process, reqMetadata, - undefined, + {}, );Based on learnings: In
src/v0/destinations/singular/transform.ts,processRouterDestexplicitly passes{}as the fourth parameter tosimpleProcessRouterDestto indicate intentionally emptyprocessParams.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts` around lines 172 - 176, The call to simpleProcessRouterDest currently passes undefined for processParams; replace that undefined with an explicit empty object {} to make the intent clear (i.e., in the transformedAudienceEvent assignment where simpleProcessRouterDest(groupedInputs.audiencelist, process, reqMetadata, undefined, ...) is invoked). Update the fourth argument to {} so processParams is explicit and consistent with other uses like processRouterDest in singular/transform.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts`:
- Around line 152-168: processRouterDest's parameter is incorrectly typed as {
message: Message }[] which omits destination/metadata and forces an unsafe cast
when calling processRecordInputs; update the function signature to the actual
router event shape (include message, metadata and destination fields to match
RecordInput), remove the "as unknown as RecordInput[]" cast when passing
groupedInputs.record to processRecordInputs, and when calling
simpleProcessRouterDest later replace the undefined fourth argument with an
explicit {} for consistency (refer to processRouterDest, RecordInput,
processRecordInputs, groupByInBatches and simpleProcessRouterDest to locate the
changes).
---
Nitpick comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts`:
- Around line 172-176: The call to simpleProcessRouterDest currently passes
undefined for processParams; replace that undefined with an explicit empty
object {} to make the intent clear (i.e., in the transformedAudienceEvent
assignment where simpleProcessRouterDest(groupedInputs.audiencelist, process,
reqMetadata, undefined, ...) is invoked). Update the fourth argument to {} so
processParams is explicit and consistent with other uses like processRouterDest
in singular/transform.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7cf8ecb8-8f8f-4d27-b150-46216cc0a31e
📒 Files selected for processing (2)
src/v0/destinations/google_adwords_remarketing_lists/transform.tssrc/v0/destinations/google_adwords_remarketing_lists/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/v0/destinations/google_adwords_remarketing_lists/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/v0/destinations/google_adwords_remarketing_lists/types.ts (1)
53-57: Modeloperationsas an exclusive union to prevent invalid payload states.Current typing permits
{}and{ create, remove }in a single operation object. The API flow appears to expect exactly one operation per item.Proposed type-safe refactor
+type OfflineDataJobOperation = + | { create: { userIdentifiers: Record<string, unknown>[] }; remove?: never } + | { remove: { userIdentifiers: Record<string, unknown>[] }; create?: never }; + export interface OfflineDataJobPayload { - operations: Array<{ - create?: { userIdentifiers: Record<string, unknown>[] }; - remove?: { userIdentifiers: Record<string, unknown>[] }; - }>; + operations: OfflineDataJobOperation[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/types.ts` around lines 53 - 57, The operations array on OfflineDataJobPayload currently allows invalid shapes (empty object or both create and remove); change it to an exclusive union so each operation is exactly one of create or remove. Define a union type (e.g., Operation = { create: { userIdentifiers: Record<string, unknown>[] }; remove?: never } | { remove: { userIdentifiers: Record<string, unknown>[] }; create?: never }) and use Array<Operation> for the operations field on OfflineDataJobPayload to prevent {} and simultaneous create+remove states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/types.ts`:
- Around line 1-59: Prettier formatting failed for this file; run the project's
formatter (e.g., npm/yarn run format or Prettier) on
src/v0/destinations/google_adwords_remarketing_lists/types.ts and commit the
result so CI passes. Ensure you only reformat (no logic changes) and keep
exported symbols like GARLDestinationConfig, RecordEventContext, Message,
RecordInput, GARLDestination, and OfflineDataJobPayload intact; fix spacing,
trailing commas, and line breaks as Prettier expects and stage the formatted
file for commit.
- Around line 15-25: The RecordEventContext interface declares userDataConsent
and personalizationConsent as unknown but GARLDestinationConfig and calls to
populateConsentFromConfig expect string | undefined; update the
RecordEventContext fields userDataConsent and personalizationConsent to type
string | undefined to match GARLDestinationConfig and the actual usage (while
optionally consider narrowing message if desired); ensure you modify the
RecordEventContext declaration accordingly so type checking aligns with
populateConsentFromConfig.
---
Nitpick comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/types.ts`:
- Around line 53-57: The operations array on OfflineDataJobPayload currently
allows invalid shapes (empty object or both create and remove); change it to an
exclusive union so each operation is exactly one of create or remove. Define a
union type (e.g., Operation = { create: { userIdentifiers: Record<string,
unknown>[] }; remove?: never } | { remove: { userIdentifiers: Record<string,
unknown>[] }; create?: never }) and use Array<Operation> for the operations
field on OfflineDataJobPayload to prevent {} and simultaneous create+remove
states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae144781-438e-4aa5-876c-d60f6dc717ce
📒 Files selected for processing (1)
src/v0/destinations/google_adwords_remarketing_lists/types.ts
maheshkutty
reviewed
Mar 13, 2026
src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/v0/destinations/google_adwords_remarketing_lists/types.ts (1)
23-24:⚠️ Potential issue | 🟡 MinorAlign
RecordEventContextconsent fields with config type.Line 23 and Line 24 use
unknown, but config and downstream consent mapping treat these as optional strings. Keeping themunknownweakens type safety and allows invalid values through compile-time checks.🔧 Suggested fix
export interface RecordEventContext { message: unknown; destination: { Config: GARLDestinationConfig }; accessToken: string; audienceId: string; typeOfList: string; userSchema?: string[]; isHashRequired: boolean; - userDataConsent?: unknown; - personalizationConsent?: unknown; + userDataConsent?: string; + personalizationConsent?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/types.ts` around lines 23 - 24, RecordEventContext currently declares userDataConsent and personalizationConsent as unknown which weakens type safety; change both fields to optional strings (e.g., string | undefined or make them optional string properties) in src/v0/destinations/google_adwords_remarketing_lists/types.ts so they match the config and downstream consent mapping; update any uses of RecordEventContext (constructors, mappers) to assume string | undefined for userDataConsent and personalizationConsent to preserve compile-time checks.src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts (1)
174-185:⚠️ Potential issue | 🟠 MajorGuard
jobIdextraction before invoking step2/step3 calls.Line 182 and Line 198 rely on
jobId!. IfresourceNameparsing fails, this becomes a runtime request corruption (.../undefined:addOperations/.../undefined:run) instead of a clear failure.🔧 Suggested fix
// step2: putting users into the job - let jobId: string | undefined; - if (firstResponse?.response?.data?.resourceName) - // eslint-disable-next-line prefer-destructuring - jobId = firstResponse.response.data.resourceName.split('/')[3]; + const resourceName = firstResponse?.response?.data?.resourceName; + const jobId = typeof resourceName === 'string' ? resourceName.split('/')[3] : undefined; + if (!jobId) { + return { + success: false, + response: { + status: 500, + data: { error: 'Unable to extract jobId from create response' }, + }, + }; + } const secondResponse = await addUserToJob({ endpoint, headers, method, - jobId: jobId!, + jobId, body, metadata, }); @@ - const thirdResponse = await runTheJob({ endpoint, headers, method, jobId: jobId!, metadata }); + const thirdResponse = await runTheJob({ endpoint, headers, method, jobId, metadata });Based on learnings: "HTTP-wrapping utilities should not throw ... callers must inspect response.status or response.success rather than using try/catch to handle errors."
Also applies to: 198-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts` around lines 174 - 185, The code extracts jobId from firstResponse.response.data.resourceName and then calls addUserToJob with jobId! which can produce invalid requests if parsing fails; update the logic around the extraction of jobId (and any other places using jobId!, e.g., subsequent run/add calls) to explicitly verify that firstResponse?.response?.data?.resourceName exists and that splitting yields a valid segment before proceeding, and if not, return or throw a clear error (or set a failure response) instead of using the non-null assertion; ensure addUserToJob and downstream calls are only invoked when jobId is a validated non-empty string and log/propagate a descriptive error when validation fails.src/v0/destinations/google_adwords_remarketing_lists/transform.ts (1)
155-171:⚠️ Potential issue | 🟠 MajorFix router input contract and remove the unsafe double-cast.
Line 155 types router inputs as
{ message: Message }[], but Line 170 forcesgroupedInputs.recordintoRecordInput[]viaunknown. This masks missingmetadata/destination/connectionshape issues until runtime.🔧 Suggested fix
+type RouterInput = + | RecordInput + | { + message: Message; + metadata: Record<string, unknown>; + destination: GARLDestination; + }; + -const processRouterDest = async (inputs: { message: Message }[], reqMetadata: unknown) => { +const processRouterDest = async (inputs: RouterInput[], reqMetadata: unknown) => { const respList: unknown[] = []; const groupedInputs = await groupByInBatches(inputs, (input) => input.message.type?.toLowerCase(), ); let transformedRecordEvent: unknown[] = []; let transformedAudienceEvent: unknown[] = []; if (groupedInputs.record) { - transformedRecordEvent = await processRecordInputs( - groupedInputs.record as unknown as RecordInput[], - ); + transformedRecordEvent = await processRecordInputs(groupedInputs.record as RecordInput[]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts` around lines 155 - 171, The router currently types inputs as { message: Message }[] in processRouterDest and then double-casts groupedInputs.record to RecordInput[] which hides missing fields; fix by updating the router input contract to accept the proper typed array (e.g., RecordInput[] | AudienceInput[] or a union of the actual input shapes) and make groupByInBatches/generic call return a correctly typed groupedInputs so you can use groupedInputs.record directly as RecordInput[] (remove the unknown cast); ensure RecordInput includes required metadata/destination/connection fields so TypeScript enforces shape at compile time and adjust any callers to pass the correct typed objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/v0/destinations/google_adwords_remarketing_lists/networkHandler.ts`:
- Around line 174-185: The code extracts jobId from
firstResponse.response.data.resourceName and then calls addUserToJob with jobId!
which can produce invalid requests if parsing fails; update the logic around the
extraction of jobId (and any other places using jobId!, e.g., subsequent run/add
calls) to explicitly verify that firstResponse?.response?.data?.resourceName
exists and that splitting yields a valid segment before proceeding, and if not,
return or throw a clear error (or set a failure response) instead of using the
non-null assertion; ensure addUserToJob and downstream calls are only invoked
when jobId is a validated non-empty string and log/propagate a descriptive error
when validation fails.
In `@src/v0/destinations/google_adwords_remarketing_lists/transform.ts`:
- Around line 155-171: The router currently types inputs as { message: Message
}[] in processRouterDest and then double-casts groupedInputs.record to
RecordInput[] which hides missing fields; fix by updating the router input
contract to accept the proper typed array (e.g., RecordInput[] | AudienceInput[]
or a union of the actual input shapes) and make groupByInBatches/generic call
return a correctly typed groupedInputs so you can use groupedInputs.record
directly as RecordInput[] (remove the unknown cast); ensure RecordInput includes
required metadata/destination/connection fields so TypeScript enforces shape at
compile time and adjust any callers to pass the correct typed objects.
In `@src/v0/destinations/google_adwords_remarketing_lists/types.ts`:
- Around line 23-24: RecordEventContext currently declares userDataConsent and
personalizationConsent as unknown which weakens type safety; change both fields
to optional strings (e.g., string | undefined or make them optional string
properties) in src/v0/destinations/google_adwords_remarketing_lists/types.ts so
they match the config and downstream consent mapping; update any uses of
RecordEventContext (constructors, mappers) to assume string | undefined for
userDataConsent and personalizationConsent to preserve compile-time checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f764a0e-f601-4865-97d9-9cfc8e93aaf3
📒 Files selected for processing (3)
src/v0/destinations/google_adwords_remarketing_lists/networkHandler.tssrc/v0/destinations/google_adwords_remarketing_lists/transform.tssrc/v0/destinations/google_adwords_remarketing_lists/types.ts
|
maheshkutty
approved these changes
Mar 13, 2026
shekhar-rudder
deleted the
feat/int-6071-migrate-garl-transformer-code-to-typescript
branch
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
2 participants
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
google_adwords_remarketing_listsdestination files from JavaScript to TypeScript (.js→.ts)types.tswith type definitions for the destinationrequire/module.exportswith ES moduleimport/exportsyntaxTest plan
util.test.ts)🤖 Generated with Claude Code