Update sync code for unified-accounting

[openint-bot]

Important

Enhance unified-accounting sync by adding merchant support and handling null values in upsert operations.

• Behavior:
  • Add merchant to sourceOutputEntities and sourceOutputEntity in def.ts.
  • Update sourceSync in server.ts to handle merchant entities.
  • Add handling for null values in upsert-sql.spec.ts.
• Mappers:
  • Add merchant mapper in plaid-adapter/mapper.ts.
  • Update transaction mapper to include vendor_id in plaid-adapter/mapper.ts.
  • Update vendor_id handling in qbo-adapter/mapper.ts.
• Models:
  • Add vendor_id to transaction model in unifiedModels.ts.
• Misc:
  • Update unifiedAccountingLink.ts to handle merchant as vendor stream.

^{This description was created by for dfcc5a3. It will automatically update as commits are pushed.}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
openint	❌ Failed (Inspect)			Mar 4, 2025 11:43am

[recurseml]

Non-null assertion operator (!) on iso_currency_code is unsafe. The currency field might be undefined, which could cause runtime errors. Should handle the null/undefined case explicitly.

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

No null check on t.EntityRef.value before using it in makeQboId. If value is undefined/null but type is 'Vendor', this will create an invalid ID. Should add null check for value property.

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

The optional chaining operator (?.) on txn.lines could lead to undefined being passed to reduce(), which would cause a runtime error. Should add a null check or provide default empty array: txn.lines?.reduce(...) ?? 0

---

React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)

[recurseml]

😱 Found 3 issues. Time to roll up your sleeves! 😱

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 30b212e in 4 minutes and 56 seconds

More details

• Looked at 335 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 19 drafted comments based on config settings.

1. connectors/connector-plaid/server.ts:327

• Draft comment:
  Using the non-null assertion (t.merchant_name!) may be risky if the field isn’t present. Consider a safer check or validation.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Looking at the code, merchant_name is only accessed when merchant_entity_id exists. The code is already guarded by 'merchant_entity_id' in t && t.merchant_entity_id check. If a transaction has a merchant_entity_id, it should always have a merchant_name. The ! assertion seems reasonable here. Additionally, suggesting 'Unknown' as a fallback doesn't make sense since null entries are filtered out anyway on line 334.
  I could be wrong about the Plaid API guarantees - maybe there are edge cases where a transaction could have merchant_entity_id but no merchant_name.
  Even if such edge cases exist, the code handles them by filtering out null entries. A crash here would indicate a more fundamental API contract issue that should be fixed upstream.
  The comment should be deleted. The non-null assertion is reasonably safe given the guards, and the suggested change would not improve the code.

2. connectors/connector-postgres/utils.ts:73

• Draft comment:
  Migration update: renaming columns (_openint_customer_id and _openint_source_id) changes schema. Ensure data migration and backward compatibility if applicable.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure data migration and backward compatibility, which violates the rule against asking the author to ensure things. It doesn't provide a specific suggestion or point out a specific issue with the code.

3. packages/db/lib/upsert-sql.spec.ts:218

• Draft comment:
  Tests now check null value handling. Ensure that expected SQL and params align with production behavior for null columns.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that the expected SQL and parameters align with production behavior for null columns. This is a request for confirmation, which violates the rule against asking the author to ensure behavior is intended or tested.

4. unified/unified-accounting/adapters/plaid-adapter/mapper.ts:41

• Draft comment:
  Mapping 'vendor_id' from 'merchant_entity_id' looks intentional; verify consistency with unified model expectations.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This comment violates several rules. It's asking for verification without pointing out a specific issue. It's speculative - if there was an actual inconsistency with the unified model, the comment should state it directly. The mapping looks straightforward and there's a merchant mapper below that uses the same field name consistently.
  Maybe there could be some important unified model constraint that this mapping violates that isn't visible in the code shown?
  If there was a real model constraint issue, the comment should state it explicitly. Asking for verification without pointing out a specific problem is not actionable.
  Delete this comment as it asks for verification without identifying any specific issue, making it speculative and not actionable.

5. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:39

• Draft comment:
  Returning an empty string for vendor_id when not present may have unintended effects. Consider returning undefined if that fits your domain.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%

1. The code author explicitly notes they tried null and it caused issues with JSON encoding. 2. Empty string is a valid choice for representing "no vendor". 3. The comment doesn't provide evidence that undefined would work better. 4. The current solution with empty string seems intentional and working. 5. Without more context about the unified model's expectations, suggesting undefined could introduce new issues.
   I could be wrong if undefined is actually the standard way to represent missing values in this codebase's unified model. The comment's author may have more context about the domain model that I'm missing.
   However, the code's comment explicitly states they tried null and had issues. Undefined would likely have similar issues since both null and undefined often get special JSON handling. The current solution seems intentional.
   Delete the comment. The current empty string approach is an intentional choice with documented reasoning, and there's no strong evidence that undefined would be better.

6. unified/unified-accounting/unifiedAccountingLink.ts:86

• Draft comment:
  Renaming connection and customer ID fields to _openint_connection_id and _openint_customer_id is clear, but ensure consistency across connectors.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is suggesting to ensure consistency across connectors when renaming fields. This is a general suggestion to double-check something, which violates the rule against asking the PR author to ensure consistency or double-check things.

7. unified/unified-accounting/unifiedAccountingLink.ts:75

• Draft comment:
  There is an extra space in the warning message string ('Transaction does not balance'); consider removing the double space.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While technically correct, this is an extremely minor formatting issue in a logging message that doesn't affect functionality. Console messages are for developers, not users. The rules say not to make comments that are obvious or unimportant. This seems like the definition of unimportant.
  The extra space could be considered a small inconsistency in code style. Some might argue that maintaining consistent formatting everywhere helps with code quality.
  While consistency is good, this is far too minor to warrant a PR comment. It's in a debug message and doesn't impact functionality or readability in any meaningful way.
  Delete this comment as it points out an extremely minor formatting issue that doesn't meaningfully impact code quality.

8. unified/unified-accounting/unifiedModels.ts:40

• Draft comment:
  Changes in the unified models: vendor url now marked as nullish—ensure this doesn’t break downstream consumers expecting a string.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that the change doesn't break downstream consumers, which is against the rules. It doesn't provide a specific suggestion or ask for a specific test to be written. It is more of a general cautionary note.

9. connectors/connector-postgres/utils.ts:78

• Draft comment:
  The generated column 'connector_name' still references 'source_id', but the column was renamed to '_openint_source_id'. Update the expression to use '_openint_source_id'.
• Reason this comment was not posted:
  Comment was on unchanged code.

10. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:14

• Draft comment:
  Typo: 'tranactionMappers' should be renamed to 'transactionMappers' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:39

• Draft comment:
  Using an empty string for vendor_id instead of null is non‐intuitive. Consider adding a comment or revising the approach to handling null values.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

12. packages/db/lib/upsert-sql.spec.ts:291

• Draft comment:
  The 'casing cache bug' tests reveal an issue where a column name is rendered as 'undefined'. Ensure that this workaround is well documented and plan to address the underlying casing cache issue.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. unified/unified-accounting/unifiedAccountingLink.ts:36

• Draft comment:
  Avoid using '@ts-expect-error' if possible. Consider updating the mapper type definitions to remove the need for this bypass.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. connectors/connector-plaid/def.ts:131

• Draft comment:
  Typo: In the comment on line 131, 'unkonwn' should be corrected to 'unknown'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. connectors/connector-plaid/server.ts:170

• Draft comment:
  Typo in comment: consider changing 'does nto appear' to 'does not appear' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. connectors/connector-postgres/utils.ts:19

• Draft comment:
  Typo detected: The word 'lightweig' in the comment appears to be misspelled. Did you mean 'lightweight' or a similar term?
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:14

• Draft comment:
  Typo in variable name: 'tranactionMappers' should likely be renamed to 'transactionMappers' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. unified/unified-accounting/unifiedAccountingLink.ts:76

• Draft comment:
  Typographical error: There is an extra space in the logged message. Consider changing "Transaction does not balance: ${trialBalance} ${txn.id}" to "Transaction does not balance: ${trialBalance} ${txn.id}".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This is an extremely minor formatting issue in a debug/warning message that doesn't affect functionality. The message is still perfectly readable and understandable. This kind of nitpick about spacing in logging messages doesn't meet the bar for "clearly a code change required" and falls into the "obvious or unimportant" category.
  The extra space could be considered a small inconsistency in the codebase. Some teams might value absolute consistency in their logging messages.
  While consistency is good, this is just a debug message and the extra space doesn't impact functionality or readability. This is too minor to warrant a PR comment.
  Delete this comment as it's too minor and doesn't meet the bar for "clearly a code change required".

19. unified/unified-accounting/unifiedAccountingLink.ts:29

• Draft comment:
  Typographical Minor: The TODO comment uses a semicolon instead of a colon. Consider updating "// TODO; generalize" to "// TODO: generalize" for consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_YIoKO6bY7f9fQ2Nr

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Consider stricter validation (e.g. .strict()) for the 'merchant' entity if extra props aren’t allowed.

Suggested change

	entity: zCast<{merchant_entity_id: string; name: string}>(),
	entity: zCast<{merchant_entity_id: string; name: string}>().strict(),

[ellipsis-dev]

The trial balance check uses strict equality to zero, which may be error‐prone due to floating‐point rounding. Consider comparing with a small epsilon tolerance.

Suggested change

	if (trialBalance !== 0) {
	if (Math.abs(trialBalance) > Number.EPSILON) {

[ellipsis-dev]

👍 Looks good to me! Incremental review on 8452d9b in 1 minute and 47 seconds

More details

• Looked at 16 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. connectors/connector-postgres/utils.ts:73

• Draft comment:
  Renaming '_openint_source_id' to 'source_id'. Ensure backward compatibility if existing data references the old column.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  Since this is a CREATE TABLE IF NOT EXISTS statement, it will only create new tables and won't modify existing ones. If a table already exists, this statement will be skipped entirely. Therefore, there is no backward compatibility concern here - existing tables keep their old column names, and new tables get the new names. The comment is raising a non-existent issue.
  Could there be database migrations or other code that depends on the old column names that I'm not seeing? Could this be part of a larger refactoring effort?
  Even if there are dependencies on old column names, this CREATE TABLE statement itself poses no risk since it only affects new tables. Any migration of existing data would need to be handled separately.
  The comment should be deleted because it raises a non-issue - CREATE TABLE IF NOT EXISTS doesn't modify existing tables, so there is no backward compatibility concern here.

2. connectors/connector-postgres/utils.ts:75

• Draft comment:
  Renaming/reordering '_openint_customer_id' to 'customer_id'. Verify that this schema change won’t break existing integrations.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment asks the PR author to verify that a schema change won't break existing integrations. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.

3. connectors/connector-postgres/utils.ts:73

• Draft comment:
  Renamed _openint_source_id to source_id. Ensure all dependent code and migrations are updated for this breaking change.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that all dependent code and migrations are updated for a breaking change. This falls under the rule of not asking the author to ensure things are tested or verified, which is not allowed.

4. connectors/connector-postgres/utils.ts:75

• Draft comment:
  Renamed _openint_customer_id to customer_id and made it nullable. Verify if this change in nullability is intentional.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  In PostgreSQL, omitting NOT NULL constraint makes a column nullable by default. The suggestion to add explicit NULL is purely stylistic and doesn't change the behavior. The comment also asks to verify intentionality, which violates our rules about not asking authors to verify things. The nullability change appears intentional given the clean diff.
  The comment does identify a real change in the schema. Maybe the nullability change could have unintended consequences in the application code?
  While the schema change is real, asking for verification violates our rules. If there were application issues, they would be caught by tests. The explicit NULL suggestion is purely stylistic.
  Delete this comment. It asks for unnecessary verification and suggests a purely stylistic change that doesn't affect functionality.

5. connectors/connector-postgres/utils.ts:78

• Draft comment:
  The computed column 'connector_name' uses split_part on source_id assuming an '' delimiter. Confirm that source_id consistently contains '' to avoid unexpected results.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. connectors/connector-postgres/utils.ts:19

• Draft comment:
  There's a typographical error on line 19: 'lightweig' should probably be 'lightweight'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_M9X9P7rh18aIzU1o

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on dfcc5a3 in 1 minute and 14 seconds

More details

• Looked at 30 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. unified/unified-accounting/adapters/plaid-adapter/mapper.ts:62

• Draft comment:
  Explicitly defaulting to 'USD' when currency codes are missing may be too presumptive. Consider defining a constant or config for default currency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The code is handling Plaid transaction data, which is a third-party API. The change actually improves handling by first checking unofficial_currency_code before defaulting to USD. Plaid is a US-focused financial API, so USD is a reasonable default. The suggestion to use a constant doesn't add much value since this is adapter-specific code.
  The comment raises a valid point about hardcoding currency defaults. In a global financial system, assumptions about currency can be dangerous.
  However, this is specifically a Plaid adapter, and Plaid is primarily US-focused. The code already handles multiple currency code sources before falling back to USD.
  The comment should be deleted. The code's approach is reasonable for a Plaid adapter, and moving the USD default to a constant wouldn't meaningfully improve the code.

2. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:40

• Draft comment:
  Adding a check for t.EntityRef.value is good for avoiding falsy values. Ensure this matches expected input, as skipping might hide valid edge cases.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  The comment is asking the author to ensure that a check matches expected input, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to confirm or ensure behavior. The comment does not provide a specific suggestion or point out a clear issue, making it less useful.

3. unified/unified-accounting/adapters/plaid-adapter/mapper.ts:63

• Draft comment:
  Verify fallback logic for currency. Ensure default 'USD' is appropriate when both iso and unofficial codes are absent.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the fallback logic for currency, which is against the rules as it asks for confirmation of intention. It does not provide a specific suggestion or point out a clear issue.

4. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:40

• Draft comment:
  Ensure EntityRef carries a valid value before generating vendor_id to prevent unintended values.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

5. unified/unified-accounting/adapters/qbo-adapter/mapper.ts:14

• Draft comment:
  Typographical error: The constant 'tranactionMappers' appears to be misspelled. Consider renaming it to 'transactionMappers' to improve clarity and avoid confusion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_zNQJT6ufBSCkWw4c

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.