🚧 server: kyc account migration #668
Conversation
🦋 Changeset detectedLatest commit: a2b5303 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new KYC migration script that batch-processes unknown accounts, fetches approved inquiries (Panda/other templates), updates or redacts accounts via new Persona helpers, and records detailed per-account migration statistics and error handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as KYC Migration Script
participant PersonaAPI as Persona API
participant DB as Database
participant Processor as Account Processor
User->>Script: run with CLI options
Script->>PersonaAPI: getUnknownAccounts(batchSize, after?, referenceId?)
PersonaAPI->>DB: query unknown accounts (pagination)
DB-->>PersonaAPI: accounts[]
PersonaAPI-->>Script: accounts[]
loop per account
Script->>Processor: processAccount(account)
Processor->>PersonaAPI: getUnknownApprovedInquiry(referenceId, templateId?)
PersonaAPI->>DB: fetch approved inquiry
DB-->>PersonaAPI: inquiry or null
PersonaAPI-->>Processor: inquiry
alt inquiry exists and not redacted
Processor->>Processor: parse template (Panda / Cryptomate / unknown)
alt Panda template
Processor->>PersonaAPI: updateAccount(accountId, attributes)
PersonaAPI->>DB: patch account, add document
DB-->>PersonaAPI: success
else Cryptomate or unknown
Processor->>Processor: log/skip or mark failure
end
else inquiry missing or redacted
Processor->>PersonaAPI: redactAccount(accountId)
PersonaAPI->>DB: patch redaction
DB-->>PersonaAPI: success
end
Processor-->>Script: account result (migrated / redacted / failed)
end
Script->>Script: compile statistics
Script-->>User: print migration summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary of ChangesHello @mainqueg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new server-side script aimed at migrating Know Your Customer (KYC) account data. The primary goal is to automate the process of updating account information and associated documents by leveraging approved Persona inquiries. It includes mechanisms for identifying and processing specific inquiry templates, redacting non-compliant accounts, and ensuring data consistency through robust validation. This change significantly enhances the system's ability to manage and maintain accurate KYC records. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, kyc-migration.ts, to migrate and redact KYC accounts. The review suggests removing an unnecessary eslint disable comment and emphasizes throwing errors for schema validation issues to prevent data corruption, aligning with data integrity best practices. It also modifies the persona utilities, where a comment highlights the importance of avoiding direct object mutation and demonstrates using placeholders with documentation for unavailable required fields, consistent with data adaptation guidelines.
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} failed to parse panda inquiry`, | ||
| buildIssueMessages(pandaInquiry.issues), | ||
| ); |
There was a problem hiding this comment.
Consider throwing an error here, instead of just returning. This will ensure that the migration process stops when there are schema errors, which can prevent data corruption. This aligns with the principle of treating data integrity issues as critical, similar to how missing expected database records should result in an error.
References
- Throw an error when an expected database record (like a card) is not found. This indicates a system or data integrity issue, not a client-side error that can be fixed by the user, so a 5xx-level error is more appropriate than a 4xx.
There was a problem hiding this comment.
The objective is to migrate all possible accounts and flag problematic ones for separate handling. Since the migration is idempotent, it can be rerun safely without side effects. Given the volume of accounts, stopping the migration for individual errors isn’t practical
| let next = initialNext; | ||
| let batch = 0; | ||
| let retries = 0; | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
There was a problem hiding this comment.
The eslint disable comment is unnecessary because the condition can be while (true) without any issues.
while (true) {There was a problem hiding this comment.
Unnecessary conditional, value is always truthy.eslint@typescript-eslint/no-unnecessary-condition
| transform((fields) => { | ||
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | ||
| ); | ||
| fields["new-screen-input-checkbox-2"] = { value: true }; | ||
| return fields; | ||
| } | ||
| return fields; | ||
| }), |
There was a problem hiding this comment.
This transform function modifies the fields object directly and mutates the new-screen-input-checkbox-2 property. This could lead to unexpected side effects if the original object is used elsewhere. It's better to create a new object with the modified property to avoid mutation. Additionally, the suggested code demonstrates how to use a reasonable placeholder ({ value: true }) for a required field when it's unavailable, and documents this substitution with a console.error, aligning with best practices for adapting data formats.
transform((fields) => {
if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) {
// eslint-disable-next-line no-console
console.error(
"❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true",
);
return { ...fields, ["new-screen-input-checkbox-2"]: { value: true } }; // Create a new object instead of mutating the original
}
return fields;
}),References
- When adapting data formats, if a required field is unavailable in the source, use a reasonable placeholder from the available data to satisfy the format and document this substitution with a code comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #668 +/- ##
==========================================
- Coverage 63.66% 61.35% -2.32%
==========================================
Files 169 169
Lines 5882 5240 -642
Branches 1753 1481 -272
==========================================
- Hits 3745 3215 -530
+ Misses 1964 1865 -99
+ Partials 173 160 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@server/script/kyc-migration.ts`:
- Around line 201-239: Extract the duplicated redaction block into a helper
function (e.g. redactAccountWithLogging) that accepts accountId and referenceId
(and optionally a reason string) and encapsulates the logic that logs the
reason, checks onlyLogs, calls persona.redactAccount(...), increments
redactedAccounts on success and failedToRedactAccounts on failure, and uses
log/unexpected/inspect for messages; then replace both duplicated sections with
calls to redactAccountWithLogging(referenceId, accountId, "Account ... has no
approved inquiry" or "Inquiry ... is redacted") to remove duplication while
preserving behavior.
- Around line 50-62: Replace the many standalone counters (migratedAccounts,
redactedAccounts, redactedInquiries, failedToRedactAccounts,
noApprovedInquiryAccounts, unknownTemplates, cryptomateTemplates,
pandaTemplates, schemaErrors, failedAccounts, inquirySchemaErrors,
noReferenceIdAccounts, totalAccounts) with a single statistics object (e.g.,
const statistics = { migrated: 0, redactedAccounts: 0, redactedInquiries: 0,
failedToRedact: 0, noApprovedInquiry: 0, unknownTemplates: 0,
cryptomateTemplates: 0, pandaTemplates: 0, schemaErrors: 0, failed: 0,
inquirySchemaErrors: 0, noReferenceId: 0, total: 0 };), then update every place
that reads or mutates the old variables (e.g., migratedAccounts++, totalAccounts
+= n, logging output, resets) to use the corresponding statistics.<property>
(e.g., statistics.migrated++, statistics.total += n) and add a small TypeScript
interface/type for the object if needed; ensure all logging and tests reference
the object and that any reset logic sets statistics = { ...0s } or zeroes each
property.
- Around line 117-125: The retry delay in the catch block of kyc-migration.ts is
ineffective because Promise.resolve(setTimeout(...)) resolves immediately;
replace that line in the catch block (the code around unexpected(...),
retries++, and the retry logic) with an actual awaited delay by awaiting a
Promise that resolves after 1000ms (i.e., create a new Promise and resolve it
from setTimeout, then await it) so the retry truly waits one second before
continuing. Ensure the surrounding async context (the catch handler) remains
unchanged and keep retries and the abort logic intact.
- Around line 21-44: The option parsing silently allows missing values for flags
parsed with option.split("=")[1] (e.g., setting reference or initialNext to
undefined); update the switch cases that handle "--reference-id=" and "--next="
to validate that option includes "=" and a non-empty value before assigning to
reference or initialNext, and if the value is missing call unexpected(...) or
throw an Error with a clear message; ensure you check both the existence and
non-empty string (trimmed) of the right-hand part and apply the same validation
pattern wherever option.split("=")[1] is used.
- Around line 15-19: The module-level mutable variables reference, all,
onlyLogs, initialNext, and onlyPandaTemplates should not be declared as let;
instead create a single immutable options object (e.g., const migrationOptions =
{ reference: undefined, all: false, onlyLogs: false, initialNext: undefined,
onlyPandaTemplates: false }) or require these values as parameters to main(),
then read/update only within a local scope; update usages where the code
currently reads/writes reference, all, onlyLogs, initialNext, and
onlyPandaTemplates to access properties on the options object or the function
parameters and remove the module-level lets.
- Around line 266-287: The catch for persona.getAccount currently swallows all
errors so non-schema failures (e.g., network) are treated as schemaErrors;
modify the catch in the getAccount call so that if error instanceof ValiError
you call unexpected(...) and allow basicAccount to remain falsy, but for any
other error re-throw the error (or propagate it) instead of swallowing it;
reference persona.getAccount, ValiError, basicAccount, schemaErrors and
unexpected when making the change so non-schema errors aren't misclassified.
In `@server/utils/persona.ts`:
- Around line 59-65: The request URL in getUnknownApprovedInquiry currently
appends `&${templateId ? \`filter[inquiry-template-id]=${templateId}\` : ""}`
which leaves a trailing ampersand when templateId is undefined; fix by
constructing the query string from an array of parameters (e.g., base params
"page[size]=1", "filter[reference-id]=${referenceId}",
"filter[status]=approved") and only push
`filter[inquiry-template-id]=${templateId}` when templateId is provided, then
join with '&' and pass the resulting clean query string into the request call to
eliminate the stray '&'.
- Around line 534-544: Replace the console.error call and in-place mutation
inside the transform callback with structured logging and an immutable update:
inside the transform((fields) => { ... }) callback (the function that checks
"new-screen-2-2-input-checkbox" and "new-screen-input-checkbox-2"), remove the
console.error and instead call the debug/logger provided via the transform
context (e.g., use the passed-in debug/logger on the transform context or
closure) to log a structured message including the field names and reason, and
when setting new-screen-input-checkbox-2 to true return a new object (e.g., {
...fields, "new-screen-input-checkbox-2": { value: true } }) rather than
mutating fields in place.
| let reference: string | undefined; | ||
| let all = false; | ||
| let onlyLogs = false; | ||
| let initialNext: string | undefined; | ||
| let onlyPandaTemplates = false; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer const with explicit initialization for module-level state.
Per coding guidelines, prefer const for all variable declarations. These mutable module-level variables could cause issues if the script is imported multiple times or used in tests.
Consider encapsulating these in an options object or passing them as parameters to main():
Proposed approach
-let reference: string | undefined;
-let all = false;
-let onlyLogs = false;
-let initialNext: string | undefined;
-let onlyPandaTemplates = false;
+interface MigrationOptions {
+ reference?: string;
+ all: boolean;
+ onlyLogs: boolean;
+ initialNext?: string;
+ onlyPandaTemplates: boolean;
+}
+
+function parseOptions(argv: string[]): MigrationOptions {
+ const options: MigrationOptions = {
+ all: false,
+ onlyLogs: false,
+ onlyPandaTemplates: false,
+ };
+ // ... parsing logic
+ return options;
+}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 15 - 19, The module-level
mutable variables reference, all, onlyLogs, initialNext, and onlyPandaTemplates
should not be declared as let; instead create a single immutable options object
(e.g., const migrationOptions = { reference: undefined, all: false, onlyLogs:
false, initialNext: undefined, onlyPandaTemplates: false }) or require these
values as parameters to main(), then read/update only within a local scope;
update usages where the code currently reads/writes reference, all, onlyLogs,
initialNext, and onlyPandaTemplates to access properties on the options object
or the function parameters and remove the module-level lets.
| const options = process.argv.slice(2); | ||
| for (const option of options) { | ||
| switch (true) { | ||
| case option.startsWith("--reference-id="): | ||
| reference = option.split("=")[1]; | ||
| break; | ||
| case option.startsWith("--all"): | ||
| all = true; | ||
| break; | ||
| case option.startsWith("--only-logs"): | ||
| log("Running in only logs mode"); | ||
| onlyLogs = true; | ||
| break; | ||
| case option.startsWith("--next="): | ||
| initialNext = option.split("=")[1]; | ||
| break; | ||
| case option.startsWith("--only-panda-templates"): | ||
| onlyPandaTemplates = true; | ||
| break; | ||
| default: | ||
| unexpected(`❌ unknown option: ${option}`); | ||
| throw new Error(`unknown option: ${option}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation for option value extraction.
option.split("=")[1] returns undefined if the = is missing or value is empty (e.g., --reference-id=), which silently sets reference to undefined.
Proposed fix
case option.startsWith("--reference-id="):
- reference = option.split("=")[1];
+ reference = option.split("=")[1];
+ if (!reference) {
+ unexpected("❌ --reference-id requires a value");
+ throw new Error("--reference-id requires a value");
+ }
break;🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 21 - 44, The option parsing
silently allows missing values for flags parsed with option.split("=")[1] (e.g.,
setting reference or initialNext to undefined); update the switch cases that
handle "--reference-id=" and "--next=" to validate that option includes "=" and
a non-empty value before assigning to reference or initialNext, and if the value
is missing call unexpected(...) or throw an Error with a clear message; ensure
you check both the existence and non-empty string (trimmed) of the right-hand
part and apply the same validation pattern wherever option.split("=")[1] is
used.
| let migratedAccounts = 0; | ||
| let redactedAccounts = 0; | ||
| let redactedInquiries = 0; | ||
| let failedToRedactAccounts = 0; | ||
| let noApprovedInquiryAccounts = 0; | ||
| let unknownTemplates = 0; | ||
| let cryptomateTemplates = 0; | ||
| let pandaTemplates = 0; | ||
| let schemaErrors = 0; | ||
| let failedAccounts = 0; | ||
| let inquirySchemaErrors = 0; | ||
| let noReferenceIdAccounts = 0; | ||
| let totalAccounts = 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider encapsulating statistics in an object.
Multiple global counters could be grouped into a single statistics object for cleaner code and easier reset/testing.
Proposed approach
const statistics = {
migrated: 0,
redactedAccounts: 0,
redactedInquiries: 0,
failedToRedact: 0,
noApprovedInquiry: 0,
unknownTemplates: 0,
cryptomateTemplates: 0,
pandaTemplates: 0,
schemaErrors: 0,
failed: 0,
inquirySchemaErrors: 0,
noReferenceId: 0,
total: 0,
};🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 50 - 62, Replace the many
standalone counters (migratedAccounts, redactedAccounts, redactedInquiries,
failedToRedactAccounts, noApprovedInquiryAccounts, unknownTemplates,
cryptomateTemplates, pandaTemplates, schemaErrors, failedAccounts,
inquirySchemaErrors, noReferenceIdAccounts, totalAccounts) with a single
statistics object (e.g., const statistics = { migrated: 0, redactedAccounts: 0,
redactedInquiries: 0, failedToRedact: 0, noApprovedInquiry: 0, unknownTemplates:
0, cryptomateTemplates: 0, pandaTemplates: 0, schemaErrors: 0, failed: 0,
inquirySchemaErrors: 0, noReferenceId: 0, total: 0 };), then update every place
that reads or mutates the old variables (e.g., migratedAccounts++, totalAccounts
+= n, logging output, resets) to use the corresponding statistics.<property>
(e.g., statistics.migrated++, statistics.total += n) and add a small TypeScript
interface/type for the object if needed; ensure all logging and tests reference
the object and that any reset logic sets statistics = { ...0s } or zeroes each
property.
| if (!unknownInquiry) { | ||
| noApprovedInquiryAccounts++; | ||
| log(`Account ${referenceId}/${accountId} has no approved inquiry. Redacting account...`); | ||
| if (onlyLogs) return; | ||
| await persona | ||
| .redactAccount(accountId) | ||
| .then(() => { | ||
| log(`♻️ Account ${referenceId}/${accountId} redacted successfully`); | ||
| redactedAccounts++; | ||
| }) | ||
| .catch((error: unknown) => { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} redacting failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| failedToRedactAccounts++; | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (unknownInquiry.attributes["redacted-at"]) { | ||
| redactedInquiries++; | ||
| log(`Inquiry ${referenceId}/${accountId} is redacted. Redacting account...`); | ||
| if (onlyLogs) return; | ||
| await persona | ||
| .redactAccount(accountId) | ||
| .then(() => { | ||
| log(`♻️ Account ${referenceId}/${accountId} redacted successfully`); | ||
| redactedAccounts++; | ||
| }) | ||
| .catch((error: unknown) => { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} redacting failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| failedToRedactAccounts++; | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate redaction logic could be extracted.
The redaction code in lines 205-218 and 225-238 is nearly identical. Consider extracting to a helper function.
Proposed approach
async function redactAccountWithLogging(accountId: string, referenceId: string, reason: string) {
log(`${reason}. Redacting account...`);
if (onlyLogs) return;
await persona
.redactAccount(accountId)
.then(() => {
log(`♻️ Account ${referenceId}/${accountId} redacted successfully`);
redactedAccounts++;
})
.catch((error: unknown) => {
unexpected(
`❌ Account ${referenceId}/${accountId} redacting failed`,
inspect(error, { depth: null, colors: true }),
);
failedToRedactAccounts++;
});
}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 201 - 239, Extract the
duplicated redaction block into a helper function (e.g.
redactAccountWithLogging) that accepts accountId and referenceId (and optionally
a reason string) and encapsulates the logic that logs the reason, checks
onlyLogs, calls persona.redactAccount(...), increments redactedAccounts on
success and failedToRedactAccounts on failure, and uses log/unexpected/inspect
for messages; then replace both duplicated sections with calls to
redactAccountWithLogging(referenceId, accountId, "Account ... has no approved
inquiry" or "Inquiry ... is redacted") to remove duplication while preserving
behavior.
| // validate basic scope | ||
| const basicAccount = await persona.getAccount(referenceId, "basic").catch((error: unknown) => { | ||
| if (error instanceof ValiError) { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} failed to get basic scope due to schema errors`, | ||
| buildIssueMessages(error.issues), | ||
| ); | ||
| } else { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} getting basic scope failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| if (!basicAccount) { | ||
| schemaErrors++; | ||
| return unexpected(`❌ Account ${referenceId}/${accountId} failed to get basic scope`); | ||
| } | ||
| log(`✅ PANDA TEMPLATE: Account ${referenceId}/${basicAccount.id} has been migrated and has a valid basic scope`); | ||
| migratedAccounts++; | ||
| return; |
There was a problem hiding this comment.
Error handling in getAccount catch block may not correctly track failures.
The catch block logs errors but doesn't re-throw. The subsequent check if (!basicAccount) will be true, incrementing schemaErrors even for non-schema errors (like network failures).
Proposed fix
const basicAccount = await persona.getAccount(referenceId, "basic").catch((error: unknown) => {
if (error instanceof ValiError) {
+ schemaErrors++;
unexpected(
`❌ Account ${referenceId}/${accountId} failed to get basic scope due to schema errors`,
buildIssueMessages(error.issues),
);
} else {
+ failedAccounts++;
unexpected(
`❌ Account ${referenceId}/${accountId} getting basic scope failed`,
inspect(error, { depth: null, colors: true }),
);
}
+ return undefined;
});
if (!basicAccount) {
- schemaErrors++;
return unexpected(`❌ Account ${referenceId}/${accountId} failed to get basic scope`);
}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 266 - 287, The catch for
persona.getAccount currently swallows all errors so non-schema failures (e.g.,
network) are treated as schemaErrors; modify the catch in the getAccount call so
that if error instanceof ValiError you call unexpected(...) and allow
basicAccount to remain falsy, but for any other error re-throw the error (or
propagate it) instead of swallowing it; reference persona.getAccount, ValiError,
basicAccount, schemaErrors and unexpected when making the change so non-schema
errors aren't misclassified.
| transform((fields) => { | ||
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | ||
| ); | ||
| fields["new-screen-input-checkbox-2"] = { value: true }; | ||
| return fields; | ||
| } | ||
| return fields; | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid console.error in schema transforms; use structured logging.
Schema transforms should not use console.error directly. This bypasses the structured logging setup and may not appear in proper log streams. Consider removing the log or using the debug logger passed through context.
Additionally, the transform mutates the input object directly. While this works, returning a new object would be cleaner.
Proposed fix
transform((fields) => {
if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) {
- // eslint-disable-next-line no-console
- console.error(
- "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true",
- );
- fields["new-screen-input-checkbox-2"] = { value: true };
- return fields;
+ // fallback to true when both checkbox fields are missing
+ return { ...fields, "new-screen-input-checkbox-2": { value: true } };
}
return fields;
}),📝 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.
| transform((fields) => { | |
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | |
| // eslint-disable-next-line no-console | |
| console.error( | |
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | |
| ); | |
| fields["new-screen-input-checkbox-2"] = { value: true }; | |
| return fields; | |
| } | |
| return fields; | |
| }), | |
| transform((fields) => { | |
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | |
| // fallback to true when both checkbox fields are missing | |
| return { ...fields, "new-screen-input-checkbox-2": { value: true } }; | |
| } | |
| return fields; | |
| }), |
🤖 Prompt for AI Agents
In `@server/utils/persona.ts` around lines 534 - 544, Replace the console.error
call and in-place mutation inside the transform callback with structured logging
and an immutable update: inside the transform((fields) => { ... }) callback (the
function that checks "new-screen-2-2-input-checkbox" and
"new-screen-input-checkbox-2"), remove the console.error and instead call the
debug/logger provided via the transform context (e.g., use the passed-in
debug/logger on the transform context or closure) to log a structured message
including the field names and reason, and when setting
new-screen-input-checkbox-2 to true return a new object (e.g., { ...fields,
"new-screen-input-checkbox-2": { value: true } }) rather than mutating fields in
place.
|
@devin-ai-integration review |
Summary by CodeRabbit
Chores
Tests
Changelog
✏️ Tip: You can customize this high-level summary in your review settings.