Skip to content

fix(billing): merge tax-code Stripe+ID atomically - #4467

Merged
borbelyr-kong merged 7 commits into
mainfrom
refactor/billing-taxconfig
Jun 3, 2026
Merged

fix(billing): merge tax-code Stripe+ID atomically #4467
borbelyr-kong merged 7 commits into
mainfrom
refactor/billing-taxconfig

Conversation

@borbelyr-kong

@borbelyr-kong borbelyr-kong commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai

Summary by CodeRabbit

  • New Features

    • Added a billing-layer TaxConfig type with conversion, clone, validate, equality, and merge helpers.
  • Improvements

    • Unified tax-config mapping across read/write/API paths and invoice storage; consistent merge semantics for paired fields.
    • Avoided accidental stamping/persistence of resolved tax entities in intent-level structures.
  • Bug Fixes

    • Improved tax-config equality to detect tax-code differences and added safer concurrent tax-code conflict handling.
  • Tests

    • Added and updated regression tests covering tax-code stamping, equality, and merge behaviors.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0ebb121f-ce7a-4c31-a688-acc34f736cae

📥 Commits

Reviewing files that changed from the base of the PR and between 78041fd and 0d65647.

📒 Files selected for processing (1)
  • openmeter/taxcode/taxcode.go

📝 Walkthrough

Walkthrough

Introduces billing.TaxConfig (with resolved TaxCode snapshots), removes TaxCode from productcatalog, migrates line models/persistence to billing configs, updates adapters/HTTP/Stripe mappings to wrap/unwrap configs, refactors merge/stamping flows, updates tests, and improves taxcode conflict handling.

Changes

Tax Configuration Layering and Stamping

Layer / File(s) Summary
Billing TaxConfig core and productcatalog cleanup
openmeter/billing/taxconfig.go, openmeter/productcatalog/tax.go, openmeter/taxcode/taxcode.go, openmeter/productcatalog/tax_test.go
Adds billing.TaxConfig (embedding intent-level fields + resolved TaxCode), conversion helpers (FromProductCatalog, ToProductCatalog), methods (Equal, Validate, Clone), MergeTaxConfigs, removes TaxCode from productcatalog.TaxConfig, and adds TaxCode.Equal.
Line models and persistence boundaries
openmeter/billing/stdinvoiceline.go, openmeter/billing/stdinvoice.go, openmeter/ent/schema/billing.go, openmeter/billing/gatheringinvoice.go
Switches line structs and ent schema to billing.TaxConfig, updates conversions at model boundaries to wrap/unwrap productcatalog configs.
Adapter backfill and API mapping conversions
openmeter/billing/adapter/stdinvoicelinemapper.go, openmeter/billing/adapter/gatheringlines.go, openmeter/billing/httpdriver/invoiceline.go, openmeter/app/stripe/appinvoice.go, api/v3/handlers/billingprofiles/convert.go
Refactors adapter backfill and read/write mappings to use billing.TaxConfig in persistence, unwraps to productcatalog.TaxConfig when calling productcatalog logic, updates HTTP/Stripe mappings, and moves the goverter ignore directive.
Invoice tax-config snapshot merge and tests
openmeter/billing/service/invoicecalc/taxconfig.go, openmeter/billing/service/invoicecalc/taxconfig_test.go, openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go, test/billing/taxcode_dual_write_test.go
Routes snapshot merging through billing.MergeTaxConfigs/FromProductCatalog, adds TestTaxConfigEqualDetectsTaxCode, and updates tests to assert on billing.TaxConfig (using ToProductCatalog() where needed).
Tax code orphaned-key error and retry handling
openmeter/taxcode/errors.go, openmeter/taxcode/service/taxcode.go
Adds ErrTaxCodeOrphanedKey and IsTaxCodeOrphanedKeyError, and updates GetOrCreateByAppMapping to retry on generic conflicts and map unresolved retries to the orphaned-key sentinel.
Misc small mapping and handler tweaks
api/v3/handlers/billingprofiles/convert.go, openmeter/app/stripe/appinvoice.go, openmeter/billing/adapter/gatheringlines.go
Minor relocations of goverter directive and small type adjustments so Stripe and handler code accept *billing.TaxConfig and perform proper conversions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • turip
  • tothandras
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main objective: atomically merging Stripe and TaxCodeID as a coupled identity pair in tax config handling, which is the central theme across all file changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/billing-taxconfig

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
openmeter/billing/adapter/stdinvoicelines.go (1)

329-333: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same nil dereference risk as in gatheringlines.go:224.

At line 330, *billing.FromProductCatalog(line.TaxConfig) dereferences the result. If FromProductCatalog can return nil, this will panic. This is the same concern raised for gatheringlines.go.

🤖 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 `@openmeter/billing/adapter/stdinvoicelines.go` around lines 329 - 333, The
code dereferences billing.FromProductCatalog(line.TaxConfig) when setting tax
config in the stdinvoicelines flow, which can panic if FromProductCatalog
returns nil; update the block that builds create (the code calling
SetTaxConfig(...).SetNillableTaxCodeID(...).SetNillableTaxBehavior(...)) to
first call billing.FromProductCatalog(line.TaxConfig) into a local variable,
check that the result is non-nil before passing it to SetTaxConfig, and only
call SetTaxConfig when that local is non-nil (still preserving the existing
Nillable setters using line.TaxConfig fields as before).
openmeter/billing/stdinvoiceline.go (1)

409-409: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same nil receiver concern as before.

At line 409, i.TaxConfig.ToProductCatalog() will panic if i.TaxConfig is nil and ToProductCatalog() doesn't handle nil receivers. This is the same issue flagged in multiple other files.

🤖 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 `@openmeter/billing/stdinvoiceline.go` at line 409, The assignment TaxConfig:
i.TaxConfig.ToProductCatalog() can panic when i.TaxConfig is nil; update the
struct construction to guard the call by checking i.TaxConfig for nil and only
call ToProductCatalog() when non-nil (e.g., compute a local variable taxCfg :=
nil; if i.TaxConfig != nil { taxCfg = i.TaxConfig.ToProductCatalog() } and
assign TaxConfig: taxCfg), ensuring ToProductCatalog is invoked only on non-nil
receivers.
openmeter/billing/httpdriver/invoiceline.go (1)

365-365: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same nil receiver concern for ToProductCatalog() calls.

At lines 365 and 376, line.TaxConfig.ToProductCatalog() is called. If line.TaxConfig is nil and ToProductCatalog() doesn't handle nil receivers, this will panic. This is the same issue flagged in other files.

Also applies to: 376-376

🤖 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 `@openmeter/billing/httpdriver/invoiceline.go` at line 365, The call to
line.TaxConfig.ToProductCatalog() can panic if line.TaxConfig is nil; update the
invoiceline construction (where TaxConfig:
mapTaxConfigToAPI(line.TaxConfig.ToProductCatalog()) appears) to defensively
handle a nil TaxConfig by checking if line.TaxConfig == nil and passing nil into
mapTaxConfigToAPI (or an appropriate empty value) instead of calling
ToProductCatalog on a nil receiver; alternatively add a nil-safe
ToProductCatalog receiver implementation, but prefer the explicit nil check near
the use-site (referencing line.TaxConfig, ToProductCatalog, and
mapTaxConfigToAPI) to avoid panics.
openmeter/billing/adapter/stdinvoicelinemapper.go (1)

217-221: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same nil-safety concern as in gatheringlines.go.

At line 218, billing.FromProductCatalog(lo.EmptyableToPtr(dbLine.TaxConfig)) may receive nil if the tax config is empty. Please verify that FromProductCatalog handles nil input correctly. Additionally, line 221's .ToProductCatalog() call may panic if the preceding result is nil and the method doesn't handle nil receivers.

🤖 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 `@openmeter/billing/adapter/stdinvoicelinemapper.go` around lines 217 - 221,
The call chain around backfillTaxConfigReferences may pass or return nil: avoid
passing a nil pointer into billing.FromProductCatalog and avoid calling
.ToProductCatalog() on a nil result; update the code around
backfillTaxConfigReferences(lo.EmptyableToPtr(dbLine.TaxConfig),
dbLine.TaxBehavior, taxCodeFromDetailedLineV2Edge(dbLine)) so that you first
guard lo.EmptyableToPtr(dbLine.TaxConfig) (use a non-nil zero-value or
skip/backfill when TaxConfig is empty) or make billing.FromProductCatalog robust
to nil, and ensure you only call .ToProductCatalog() after checking the returned
value from backfillTaxConfigReferences is non-nil (or make ToProductCatalog a
safe method on nil receivers); reference functions/types:
backfillTaxConfigReferences, billing.FromProductCatalog, lo.EmptyableToPtr,
dbLine.TaxConfig, taxCodeFromDetailedLineV2Edge, and ToProductCatalog when
applying the fix.
🧹 Nitpick comments (1)
openmeter/taxcode/taxcode.go (1)

89-90: ⚡ Quick win

Clarify which fields are actually compared.

The comment says "all other fields are compared," but the implementation only compares ID, Namespace, Key, Name, Description, and AppMappings. It doesn't compare Metadata or Annotations, which might be intentional for semantic equality, but the comment is misleading.

Consider rewording to explicitly list the compared fields or say "key semantic fields" instead of "all other fields."

📝 Suggested comment clarification
-// Equal returns true when both TaxCode values carry identical semantic data.
-// ManagedModel timestamps (CreatedAt, UpdatedAt, DeletedAt) are excluded; all other fields are compared.
+// Equal returns true when both TaxCode values carry identical semantic data.
+// Compares ID, Namespace, Key, Name, Description, and AppMappings.
+// ManagedModel timestamps (CreatedAt, UpdatedAt, DeletedAt), Metadata, and Annotations are excluded.
🤖 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 `@openmeter/taxcode/taxcode.go` around lines 89 - 90, The comment for Equal in
TaxCode is misleading because it claims "all other fields are compared" while
the implementation only compares specific fields (ID, Namespace, Key, Name,
Description, AppMappings) and excludes Metadata/Annotations and timestamps;
update the comment for the Equal method (TaxCode.Equal) to explicitly list the
fields being compared or replace "all other fields" with a phrase like "key
semantic fields (ID, Namespace, Key, Name, Description, AppMappings)" so the doc
matches the implementation.
🤖 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 `@openmeter/billing/adapter/stdinvoicelinemapper.go`:
- Around line 345-367: The function backfillTaxConfigReferences calls
snapshottedTaxConfig.ToProductCatalog() without guarding for a nil receiver; add
a nil check for snapshottedTaxConfig at the top of backfillTaxConfigReferences
and handle the nil case (e.g., pass nil into productcatalog.BackfillTaxConfig or
short-circuit to return billing.FromProductCatalog(nil) as appropriate). Ensure
subsequent logic that inspects backfilledTaxConfig and resolvedTaxCode still
behaves correctly when backfilledTaxConfig is nil (the code paths that check
backfilledTaxConfig.TaxCodeID and backfilledTaxConfig.Stripe should only run
when backfilledTaxConfig != nil).

---

Duplicate comments:
In `@openmeter/billing/adapter/stdinvoicelinemapper.go`:
- Around line 217-221: The call chain around backfillTaxConfigReferences may
pass or return nil: avoid passing a nil pointer into billing.FromProductCatalog
and avoid calling .ToProductCatalog() on a nil result; update the code around
backfillTaxConfigReferences(lo.EmptyableToPtr(dbLine.TaxConfig),
dbLine.TaxBehavior, taxCodeFromDetailedLineV2Edge(dbLine)) so that you first
guard lo.EmptyableToPtr(dbLine.TaxConfig) (use a non-nil zero-value or
skip/backfill when TaxConfig is empty) or make billing.FromProductCatalog robust
to nil, and ensure you only call .ToProductCatalog() after checking the returned
value from backfillTaxConfigReferences is non-nil (or make ToProductCatalog a
safe method on nil receivers); reference functions/types:
backfillTaxConfigReferences, billing.FromProductCatalog, lo.EmptyableToPtr,
dbLine.TaxConfig, taxCodeFromDetailedLineV2Edge, and ToProductCatalog when
applying the fix.

In `@openmeter/billing/adapter/stdinvoicelines.go`:
- Around line 329-333: The code dereferences
billing.FromProductCatalog(line.TaxConfig) when setting tax config in the
stdinvoicelines flow, which can panic if FromProductCatalog returns nil; update
the block that builds create (the code calling
SetTaxConfig(...).SetNillableTaxCodeID(...).SetNillableTaxBehavior(...)) to
first call billing.FromProductCatalog(line.TaxConfig) into a local variable,
check that the result is non-nil before passing it to SetTaxConfig, and only
call SetTaxConfig when that local is non-nil (still preserving the existing
Nillable setters using line.TaxConfig fields as before).

In `@openmeter/billing/httpdriver/invoiceline.go`:
- Line 365: The call to line.TaxConfig.ToProductCatalog() can panic if
line.TaxConfig is nil; update the invoiceline construction (where TaxConfig:
mapTaxConfigToAPI(line.TaxConfig.ToProductCatalog()) appears) to defensively
handle a nil TaxConfig by checking if line.TaxConfig == nil and passing nil into
mapTaxConfigToAPI (or an appropriate empty value) instead of calling
ToProductCatalog on a nil receiver; alternatively add a nil-safe
ToProductCatalog receiver implementation, but prefer the explicit nil check near
the use-site (referencing line.TaxConfig, ToProductCatalog, and
mapTaxConfigToAPI) to avoid panics.

In `@openmeter/billing/stdinvoiceline.go`:
- Line 409: The assignment TaxConfig: i.TaxConfig.ToProductCatalog() can panic
when i.TaxConfig is nil; update the struct construction to guard the call by
checking i.TaxConfig for nil and only call ToProductCatalog() when non-nil
(e.g., compute a local variable taxCfg := nil; if i.TaxConfig != nil { taxCfg =
i.TaxConfig.ToProductCatalog() } and assign TaxConfig: taxCfg), ensuring
ToProductCatalog is invoked only on non-nil receivers.

---

Nitpick comments:
In `@openmeter/taxcode/taxcode.go`:
- Around line 89-90: The comment for Equal in TaxCode is misleading because it
claims "all other fields are compared" while the implementation only compares
specific fields (ID, Namespace, Key, Name, Description, AppMappings) and
excludes Metadata/Annotations and timestamps; update the comment for the Equal
method (TaxCode.Equal) to explicitly list the fields being compared or replace
"all other fields" with a phrase like "key semantic fields (ID, Namespace, Key,
Name, Description, AppMappings)" so the doc matches the implementation.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8dfb5647-b6a9-4705-85d8-bcb5a4dc6126

📥 Commits

Reviewing files that changed from the base of the PR and between dcfaf7d and 5040fff.

⛔ Files ignored due to path filters (6)
  • openmeter/ent/db/billinginvoiceline.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_create.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_update.go is excluded by !**/ent/db/**
  • openmeter/ent/db/entmixinaccessor.go is excluded by !**/ent/db/**
  • openmeter/ent/db/mutation.go is excluded by !**/ent/db/**
  • openmeter/ent/db/setorclear.go is excluded by !**/ent/db/**
📒 Files selected for processing (18)
  • api/v3/handlers/billingprofiles/convert.go
  • openmeter/billing/adapter/gatheringlines.go
  • openmeter/billing/adapter/stdinvoicelinemapper.go
  • openmeter/billing/adapter/stdinvoicelines.go
  • openmeter/billing/gatheringinvoice.go
  • openmeter/billing/httpdriver/invoiceline.go
  • openmeter/billing/service/invoicecalc/details.go
  • openmeter/billing/service/invoicecalc/taxconfig.go
  • openmeter/billing/service/invoicecalc/taxconfig_test.go
  • openmeter/billing/service/stdinvoicestate.go
  • openmeter/billing/stdinvoiceline.go
  • openmeter/billing/taxconfig.go
  • openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
  • openmeter/ent/schema/billing.go
  • openmeter/productcatalog/tax.go
  • openmeter/productcatalog/tax_test.go
  • openmeter/taxcode/taxcode.go
  • test/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
  • api/v3/handlers/billingprofiles/convert.go

Comment thread openmeter/billing/adapter/stdinvoicelinemapper.go
Comment thread openmeter/billing/adapter/stdinvoicelines.go Outdated
Comment thread openmeter/billing/taxconfig.go

@turip turip left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's branch off main

@borbelyr-kong
borbelyr-kong force-pushed the refactor/billing-taxconfig branch from 4ca0e9e to 7a181a9 Compare June 2, 2026 09:15
@borbelyr-kong
borbelyr-kong changed the base branch from fix/dedupe-tax-codes-by-app-mapping to main June 2, 2026 09:15

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
openmeter/billing/taxconfig.go (1)

86-111: ⚡ Quick win

Reuse the embedded Clone() and slices.Clone to keep this honest.

This re-implements productcatalog.TaxConfig.Clone() by hand for Behavior/Stripe/TaxCodeID. If productcatalog.TaxConfig ever grows a field, this clone will silently drop it. Delegating to the embedded Clone() keeps them in sync. Also, the AppMappings copy can use slices.Clone per the standard-helpers guideline.

♻️ Proposed refactor
 func (c TaxConfig) Clone() TaxConfig {
-	out := TaxConfig{}
-
-	if c.Behavior != nil {
-		out.Behavior = lo.ToPtr(*c.Behavior)
-	}
-
-	if c.Stripe != nil {
-		out.Stripe = lo.ToPtr(c.Stripe.Clone())
-	}
-
-	if c.TaxCodeID != nil {
-		out.TaxCodeID = lo.ToPtr(*c.TaxCodeID)
-	}
+	out := TaxConfig{
+		TaxConfig: c.TaxConfig.Clone(),
+	}
 
 	if c.TaxCode != nil {
 		tc := *c.TaxCode
-		tc.AppMappings = append(taxcode.TaxCodeAppMappings(nil), c.TaxCode.AppMappings...)
+		tc.AppMappings = slices.Clone(c.TaxCode.AppMappings)
 		if c.TaxCode.Description != nil {
 			tc.Description = lo.ToPtr(*c.TaxCode.Description)
 		}
 		out.TaxCode = &tc
 	}
 
 	return out
 }

Add "slices" to the imports (and drop the now-unused taxcode reference here if it isn't used elsewhere in the file).

As per coding guidelines: "use slices.Clone(s) ... instead of creating local ... wrappers."

🤖 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 `@openmeter/billing/taxconfig.go` around lines 86 - 111, The custom
TaxConfig.Clone implementation duplicates productcatalog.TaxConfig.Clone and
risks dropping future fields; refactor TaxConfig.Clone to delegate to the
embedded productcatalog.TaxConfig.Clone (call that Clone first and assign its
result to out) and then only deep-copy the mutable nested slices/ptrs that need
special handling; replace the manual AppMappings copy with
slices.Clone(c.TaxCode.AppMappings) and remove the custom
taxcode.TaxCodeAppMappings usage, ensuring Behavior, Stripe, TaxCodeID and
TaxCode are preserved by first calling the embedded Clone and then applying any
required deep copies using slices.Clone and existing Clone methods.
🤖 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 `@openmeter/billing/taxconfig.go`:
- Around line 43-62: The Equality check in TaxConfig.Equal wrongly treats both
TaxCode == nil as unequal; update the logic in TaxConfig.Equal so that after
confirming embedded c.TaxConfig.Equal(&v.TaxConfig) returns true, handle the
TaxCode pair: if both c.TaxCode and v.TaxCode are nil return true, if only one
is nil return false, otherwise return c.TaxCode.Equal(v.TaxCode). Make this
change inside the TaxConfig.Equal method to ensure two unresolved (both-nil) tax
codes are considered equal.
- Around line 121-148: MergeTaxConfigs currently drops the resolved TaxCode only
in the both-non-nil branch but returns clones that preserve TaxCode snapshots in
the single-side branches; make behavior consistent by clearing the TaxCode
snapshot and TaxCodeID on single-side clones so they match the two-sided merge
contract: in MergeTaxConfigs, when returning a clone from overrides.Clone() or
base.Clone(), set c.TaxCode = nil and c.TaxCodeID = nil before returning (refer
to MergeTaxConfigs, TaxConfig, Clone, TaxCode, TaxCodeID).

---

Nitpick comments:
In `@openmeter/billing/taxconfig.go`:
- Around line 86-111: The custom TaxConfig.Clone implementation duplicates
productcatalog.TaxConfig.Clone and risks dropping future fields; refactor
TaxConfig.Clone to delegate to the embedded productcatalog.TaxConfig.Clone (call
that Clone first and assign its result to out) and then only deep-copy the
mutable nested slices/ptrs that need special handling; replace the manual
AppMappings copy with slices.Clone(c.TaxCode.AppMappings) and remove the custom
taxcode.TaxCodeAppMappings usage, ensuring Behavior, Stripe, TaxCodeID and
TaxCode are preserved by first calling the embedded Clone and then applying any
required deep copies using slices.Clone and existing Clone methods.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3875bce6-cb8d-406d-9d15-96ab4462b3de

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca0e9e and ad39754.

⛔ Files ignored due to path filters (6)
  • openmeter/ent/db/billinginvoiceline.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_create.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_update.go is excluded by !**/ent/db/**
  • openmeter/ent/db/entmixinaccessor.go is excluded by !**/ent/db/**
  • openmeter/ent/db/mutation.go is excluded by !**/ent/db/**
  • openmeter/ent/db/setorclear.go is excluded by !**/ent/db/**
📒 Files selected for processing (18)
  • api/v3/handlers/billingprofiles/convert.go
  • openmeter/billing/adapter/gatheringlines.go
  • openmeter/billing/adapter/stdinvoicelinemapper.go
  • openmeter/billing/adapter/stdinvoicelines.go
  • openmeter/billing/gatheringinvoice.go
  • openmeter/billing/httpdriver/invoiceline.go
  • openmeter/billing/service/invoicecalc/details.go
  • openmeter/billing/service/invoicecalc/taxconfig.go
  • openmeter/billing/service/invoicecalc/taxconfig_test.go
  • openmeter/billing/service/stdinvoicestate.go
  • openmeter/billing/stdinvoiceline.go
  • openmeter/billing/taxconfig.go
  • openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
  • openmeter/ent/schema/billing.go
  • openmeter/productcatalog/tax.go
  • openmeter/productcatalog/tax_test.go
  • openmeter/taxcode/taxcode.go
  • test/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
  • api/v3/handlers/billingprofiles/convert.go
🚧 Files skipped from review as they are similar to previous changes (16)
  • openmeter/billing/adapter/gatheringlines.go
  • openmeter/billing/adapter/stdinvoicelines.go
  • test/billing/taxcode_dual_write_test.go
  • openmeter/ent/schema/billing.go
  • openmeter/billing/service/invoicecalc/details.go
  • openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
  • openmeter/taxcode/taxcode.go
  • openmeter/billing/adapter/stdinvoicelinemapper.go
  • openmeter/billing/httpdriver/invoiceline.go
  • openmeter/billing/gatheringinvoice.go
  • openmeter/billing/stdinvoiceline.go
  • openmeter/billing/service/invoicecalc/taxconfig.go
  • openmeter/productcatalog/tax.go
  • openmeter/billing/service/stdinvoicestate.go
  • openmeter/billing/service/invoicecalc/taxconfig_test.go
  • openmeter/productcatalog/tax_test.go

Comment thread openmeter/billing/taxconfig.go
Comment thread openmeter/billing/taxconfig.go
@borbelyr-kong
borbelyr-kong force-pushed the refactor/billing-taxconfig branch from 528a770 to 78041fd Compare June 3, 2026 11:17
@borbelyr-kong borbelyr-kong changed the title fix(billing): merge tax-code Stripe+ID atomically and refresh tax-config snapshot at sync fix(billing): merge tax-code Stripe+ID atomically Jun 3, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@openmeter/taxcode/taxcode.go`:
- Around line 111-118: The current equality check compares t.AppMappings and
v.AppMappings by index which is order-sensitive; change it to be
order-insensitive by comparing them as multisets: build a frequency map (e.g.,
map[AppMapping]int) for t.AppMappings and decrement/validate counts while
iterating v.AppMappings (or sort both slices by a stable key and compare), then
return false if any counts mismatch or leftover entries remain; update the
equality block that references t.AppMappings and v.AppMappings accordingly.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 797f60e7-578e-45c1-a48d-23b59a6cdd97

📥 Commits

Reviewing files that changed from the base of the PR and between 528a770 and 78041fd.

⛔ Files ignored due to path filters (6)
  • openmeter/ent/db/billinginvoiceline.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_create.go is excluded by !**/ent/db/**
  • openmeter/ent/db/billinginvoiceline_update.go is excluded by !**/ent/db/**
  • openmeter/ent/db/entmixinaccessor.go is excluded by !**/ent/db/**
  • openmeter/ent/db/mutation.go is excluded by !**/ent/db/**
  • openmeter/ent/db/setorclear.go is excluded by !**/ent/db/**
📒 Files selected for processing (19)
  • api/v3/handlers/billingprofiles/convert.go
  • openmeter/app/stripe/appinvoice.go
  • openmeter/billing/adapter/gatheringlines.go
  • openmeter/billing/adapter/stdinvoicelinemapper.go
  • openmeter/billing/gatheringinvoice.go
  • openmeter/billing/httpdriver/invoiceline.go
  • openmeter/billing/service/invoicecalc/taxconfig.go
  • openmeter/billing/service/invoicecalc/taxconfig_test.go
  • openmeter/billing/stdinvoice.go
  • openmeter/billing/stdinvoiceline.go
  • openmeter/billing/taxconfig.go
  • openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
  • openmeter/ent/schema/billing.go
  • openmeter/productcatalog/tax.go
  • openmeter/productcatalog/tax_test.go
  • openmeter/taxcode/errors.go
  • openmeter/taxcode/service/taxcode.go
  • openmeter/taxcode/taxcode.go
  • test/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
  • api/v3/handlers/billingprofiles/convert.go

Comment thread openmeter/taxcode/taxcode.go
@borbelyr-kong
borbelyr-kong merged commit da83a64 into main Jun 3, 2026
43 of 44 checks passed
@borbelyr-kong
borbelyr-kong deleted the refactor/billing-taxconfig branch June 3, 2026 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants