fix: resolve duplicate uniqueId for identical and pending transactions#840
Open
Saharariel wants to merge 1 commit intodaniel-hauser:mainfrom
Open
fix: resolve duplicate uniqueId for identical and pending transactions#840Saharariel wants to merge 1 commit intodaniel-hauser:mainfrom
Saharariel wants to merge 1 commit intodaniel-hauser:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded batch-level duplicate-ID disambiguation and adjusted unique-ID generation: for pending transactions with chargedAmount === 0 the ID uses originalAmount; after transaction creation batches are post-processed to append numeric suffixes to duplicate uniqueIds. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[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
🤖 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/bot/storage/utils.test.ts`:
- Around line 304-356: Add tests covering pre-suffixed collisions and the
non-pending zero-amount boundary: add a case that calls disambiguateDuplicateIds
with transactionRow items having uniqueId values ["a", "a_1", "a"] and assert
the resulting uniqueId array is fully unique (e.g. ["a","a_1","a_2"]) to ensure
pre-suffixed IDs don’t cause collisions; also add a test that constructs a
transactionRow with transactionUniqueId set and chargedAmount: 0 with pending:
false and assert the resulting uniqueId behavior matches the intended contract
(use transactionUniqueId in that non-pending zero-amount case). Ensure you
reference the existing helpers and function names (disambiguateDuplicateIds,
transactionRow, transactionUniqueId, chargedAmount, pending) when adding the two
tests.
In `@src/bot/storage/utils.ts`:
- Around line 67-77: The disambiguation in disambiguateDuplicateIds can still
produce collisions (e.g., ["coffee","coffee_1","coffee"] -> duplicate
"coffee_1"); fix it by making the function track already emitted uniqueIds (a
Set) and, when a base repeats, loop/increment the suffix (using counts.get(base)
as the starting counter) until the candidate `${base}_${n}` is not present in
the seen set (and also not equal to any original tx.uniqueId that has already
been emitted), then emit that candidate and update both counts and seen; keep
returning the original tx when no suffix is needed.
- Around line 44-47: The current amount fallback always replaces a zero
tx.chargedAmount with tx.originalAmount, which changes IDs for non-pending rows;
update the logic where amount is computed so the fallback to tx.originalAmount
only occurs for pending transactions (e.g., check tx.status === 'pending' or a
pending flag such as tx.isPending) and preserve chargedAmount==0 for non-pending
transactions. Locate the amount assignment (const amount = tx.chargedAmount ||
tx.originalAmount) and change it to conditionally use tx.originalAmount only
when the transaction is pending and chargedAmount is zero, otherwise keep
tx.chargedAmount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bddfc4b-2443-4165-b785-f15551872d55
📒 Files selected for processing (3)
src/bot/storage/index.tssrc/bot/storage/utils.test.tssrc/bot/storage/utils.ts
- Use originalAmount as fallback when chargedAmount is 0 only for pending transactions, preserving chargedAmount for non-pending rows - Add disambiguateDuplicateIds to append _1, _2 suffixes for transactions that are legitimately identical within a batch (e.g. buying coffee twice on the same day with no bank-assigned identifier) - Pre-populate seen set to avoid collisions with pre-suffixed natural IDs Fixes daniel-hauser#827
Owner
|
Thanks for the PR, i prefer fixing the identifier in the scraper code (e.g. eshaham/israeli-bank-scrapers#1052) rather than here. Specifically for Max, i added the |
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.
Problem
Two distinct bugs both cause a:
and result in transactions being silently dropped or merged:
Pending transactions (e.g. Max) : pending transactions report chargedAmount: 0 regardless of the actual amount.
Two pending transactions from the same merchant on the same day, even with different amounts, would produce identical uniqueIds.
Legitimately identical transactions : buying the same item twice on the same day (same merchant, same amount, no bank-assigned identifier) produces identical uniqueIds.
The second transaction gets dropped in Google Sheets or overwrites the first in SQL.
Both issues only occur when identifier is not set by the bank. When identifier is present, the existing logic works correctly.
Fix
For pending transactions: fall back to originalAmount when chargedAmount is 0, since originalAmount reflects the real transaction amount even while pending.
For identical-looking transactions: add disambiguateDuplicateIds at the batch level - after all transactions are assembled, any colliding uniqueIds get a _1, _2 suffix appended.
The first occurrence keeps its original uniqueId (backwards-compatible with existing stored data).
Reproduction
// Before fix — both produce the same uniqueId:
pending ₪50 → 2024-11-01_max_1234_0_COFFEE SHOP_
pending ₪30 → 2024-11-01_max_1234_0_COFFEE SHOP_ ← collision
// After fix:
pending ₪50 → 2024-11-01_max_1234_50_COFFEE SHOP_
pending ₪30 → 2024-11-01_max_1234_30_COFFEE SHOP_ ← distinct
Notes
This is an inherent limitation when the bank provides no identifier.
Fixes #827
Summary by CodeRabbit
Bug Fixes
Tests