Merge main_upstream: MoneymanDash storage, SaveContext, run tracking#842
Open
theido wants to merge 55 commits intodaniel-hauser:mainfrom
Open
Merge main_upstream: MoneymanDash storage, SaveContext, run tracking#842theido wants to merge 55 commits intodaniel-hauser:mainfrom
theido wants to merge 55 commits intodaniel-hauser:mainfrom
Conversation
# Conflicts: # package-lock.json # package.json # src/bot/storage/monday.ts # src/config.ts # src/storage/index.ts
# Conflicts: # src/bot/storage/index.ts # src/index.ts
# Conflicts: # src/bot/storage/index.ts
# Conflicts: # package-lock.json # src/bot/storage/index.ts # src/bot/storage/sheets.ts # src/bot/transactionTableRow.ts
- Combined license field (MIT from main branch) - Added repository information from main branch - Added hasInstallScript field from main branch
# Conflicts: # src/bot/storage/index.ts
# Conflicts: # src/bot/storage/index.ts
# Conflicts: # src/index.ts
# Conflicts: # src/index.ts
Resolved conflicts in package-lock.json and src/bot/storage/index.ts. Combined invoice creation functionality with logger context wrapper. Updated test snapshots to reflect current date. Co-authored-by: Cursor <cursoragent@cursor.com>
Consolidate scrape-invoices workflow to use single MONEYMAN_CONFIG_INVOICES secret instead of individual environment variables, enabling independent scraping processes for different accounts and Google Sheets. Changes: - Use MONEYMAN_CONFIG_INVOICES secret for complete configuration - Remove individual env vars (ACCOUNTS_JSON, GOOGLE_*, TELEGRAM_*, etc.) - Add DEBUG input parameter for runtime logging control - Offset schedule by 5 minutes (10:10 UTC) to avoid conflicts with main scrape - Remove unused MAX_PARALLEL_SCRAPERS env var - Delete scrape-monday-storage.yml (no longer needed) This allows running two independent workflows: - scrape.yml → MONEYMAN_CONFIG → main accounts/sheet - scrape-invoices.yml → MONEYMAN_CONFIG_INVOICES → invoices accounts/sheet Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update test snapshots to use February 2026 date instead of January. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds GitHub Actions workflow to automatically sync fork's main branch with upstream repository daily. Uses fast-forward-only merge to prevent conflicts and requires manual intervention if needed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ones The previous implementation incorrectly passed only insurance vendor transactions to ALL storage backends. This broke functionality for non-invoice storage systems (Google Sheets, Monday, SQL, etc.) which should receive all transactions. Changes: - All storage backends now receive `allTransactions` - Invoice creation only processes filtered insurance vendor transactions - Invoice enrichment (pdf_link, doc_number) is applied in-place to transactions, so all storages see the enriched data - Fixed early return to check allTransactions.length instead of filtered transactions Resolves code review issue from PR daniel-hauser#813 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add invoice creation workflow with Monday.com integration
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Replaced broad modal pattern detection with consent-specific selectors (id/class/aria-label containing cookie, consent, gdpr, privacy) to avoid accidentally clicking login forms or other dialogs - Added 'אישור' to the dismiss-text heuristics - Removed the fallback "click last button" logic that caused false positives - Updated .gitignore to exclude moneyman.invoices.conf.jsonc - Added CLAUDE.md with project architecture and dev guidelines Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
chore: ignore firebase-debug.log
…racking - Add MoneymanDashStorage backend with tests - Introduce SaveContext to pass per-account scrape results to storage backends - Wrap scraper execution in runContextStore with a unique runId per run - Keep invoice creation logic alongside new context building - Update dependency versions (google-auth-library, jest, @types/node) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements to the moneyman invoicing bot, adding three new storage backends (MoneymanDash, Invoice, and Monday), automated GitHub Actions workflows, async context management with run tracking, banner dismissal capabilities, and extensive documentation for configuration and deployment. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Storage as Storage Index
participant MondayStorage as Monday.com API
participant MoneymmanDash as MoneymanDash Server
participant InvoiceAPI as Invoice API
participant RunContext as Run Context Store
App->>RunContext: Create runId and enter context
RunContext-->>App: Context established
App->>Storage: saveResults(allTransactions, results)
Storage->>Storage: Extract SaveContext (accountStatuses)
Storage->>Storage: Filter insurance vendor transactions
par Parallel Storage Operations
Storage->>InvoiceAPI: createInvoicesForTransactions(filtered_txns)
InvoiceAPI-->>Storage: pdf_link, doc_number per transaction
and
Storage->>MondayStorage: saveTransactions(allTransactions)
MondayStorage-->>MondayStorage: Check existing hashes
MondayStorage->>MondayStorage: Build MondayTransaction format
MondayStorage-->>MoneymmanDash: POST /api/createDoc per transaction
MondayStorage-->>Storage: SaveStats
and
Storage->>MoneymmanDash: saveTransactions(allTransactions)
MoneymmanDash-->>Storage: SaveStats with transactionsAdded
end
Storage->>Storage: Aggregate results
App->>MoneymmanDash: sendLogs(logContent)
MoneymmanDash-->>App: Logs uploaded with X-Run-Id header
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
10-21:⚠️ Potential issue | 🔴 CriticalCritical:
postinstallscript removal breaks patch application.The
postinstall: "patch-package"script was removed, butpatch-packageremains in devDependencies and two patches exist in thepatches/directory (ky+1.14.3.patchandyargs+17.7.2.patch). The Dockerfile copies these patches but they will not be applied duringnpm ciornpm installwithout the postinstall hook, breaking runtime functionality.Proposed fix: Restore the postinstall script
"scripts": { "act": "go run github.com/nektos/act@latest", "start": "node dst/index.js", "start:container": "docker compose up", "build": "tsc", "lint": "prettier --check .", "lint:fix": "prettier --write .", "test": "jest", "test:config": "npm run build && node dst/scripts/verify-config.js", "test:scraper-access": "npm run build && npm run test:scraper-access-without-build", - "test:scraper-access-without-build": "node --test dst/test-scraper-access.js" + "test:scraper-access-without-build": "node --test dst/test-scraper-access.js", + "postinstall": "patch-package" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 10 - 21, Restore the postinstall npm script in package.json so patch-package runs after installs: add "postinstall": "patch-package" into the "scripts" object (ensuring patch-package remains in devDependencies and the existing patch files in patches/ will be applied), so that installs (npm install / npm ci) and the Docker build which copies the patches will apply the ky+1.14.3.patch and yargs+17.7.2.patch correctly.
🧹 Nitpick comments (11)
src/scraper/browser.ts (3)
185-196: Add comment explaining reverse iteration.The
.reverse()call on line 186 appears intentional (accept/dismiss buttons typically appear last in banner markup), but lacks explanation. A brief comment would aid maintainability.📝 Add clarifying comment
- const buttons = banner.querySelectorAll('button, a[class*="button"], a[role="button"]'); - for (const button of Array.from(buttons).reverse()) { + const buttons = banner.querySelectorAll('button, a[class*="button"], a[role="button"]'); + // Iterate in reverse: accept/dismiss buttons are typically last in the DOM order + for (const button of Array.from(buttons).reverse()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scraper/browser.ts` around lines 185 - 196, The reverse iteration over the NodeList (Array.from(buttons).reverse()) needs a short inline comment explaining why we iterate from the end (e.g., accept/dismiss buttons often appear last in banner markup), so add a one-line comment immediately above the Array.from(buttons).reverse() call mentioning that we prefer the last buttons first to correctly target accept/close actions; reference the variables and call names: banner.querySelectorAll, buttons, Array.from(buttons).reverse(), and the click handling in the loop to make the intent obvious to future readers.
146-166: First strategy could be expensive on large DOMs.Querying all
buttonandaelements (line 151) and iterating through them could be slow on pages with many links. Consider adding a more targeted selector or limiting the search scope.Also, the dual condition
text.includes('הבנתי') && text.includes('תודה')(both required) is quite specific—if a banner has only one of these words, it won't be dismissed by this strategy. Verify this matches the actual banner text patterns encountered.♻️ Optional: More targeted selector and flexible matching
const buttonFound = await page.evaluate(() => { // Look for buttons containing the dismiss text - const buttons = Array.from(document.querySelectorAll('button, a')); + // Limit search to elements likely to be in modals/overlays + const buttons = Array.from(document.querySelectorAll('button, [role="button"], a.btn, a.button')); for (const button of buttons) { const text = button.textContent?.trim() || ''; - if (text.includes('הבנתי') && text.includes('תודה')) { + // Match common Israeli privacy banner dismiss patterns + if ((text.includes('הבנתי') && text.includes('תודה')) || + text === 'הבנתי' || text === 'אישור') { (button as HTMLElement).click(); return true; } } return false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scraper/browser.ts` around lines 146 - 166, In dismissPrivacyBanner, the broad page.evaluate that queries all 'button, a' can be slow and the current text check requires both Hebrew words; narrow the DOM scan and make matching more flexible: target likely banner containers or controls (e.g., selectors such as role/dialog, common banner classes, or a small subset like 'header, footer, [role="dialog"] button, .cookie-banner button') and limit the node list size before iterating, and change the text test from requiring both 'הבנתי' && 'תודה' to an OR or regex that matches either token or common variants so banners with only one keyword are dismissed; update the evaluate call inside dismissPrivacyBanner to use the targeted selector and relaxed text matching.
133-136: Consider potential timing interaction with Cloudflare handler.Multiple
framenavigatedlisteners (frominitDomainTracking,initCloudflareSkipping, andinitBannerDismissal) fire independently on the same page navigations. The 2-second delay before banner dismissal could theoretically overlap with an active Cloudflare challenge.In practice, conflicts are unlikely since Cloudflare detection checks for
__cf_chl_rt_tkin the URL while banner dismissal targets specific DOM elements. However, if issues arise with element interaction during active challenges, consider adding a guard to skip dismissal during known Cloudflare solving states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scraper/browser.ts` around lines 133 - 136, The banner dismissal delay can race with Cloudflare challenge handling; update the logic in the framenavigated handlers (initDomainTracking, initCloudflareSkipping, initBannerDismissal) so dismissPrivacyBanner is skipped when Cloudflare solving is in progress: detect the Cloudflare state (e.g., check for __cf_chl_rt_tk in the URL or a shared "solving" flag set by initCloudflareSkipping) before calling sleep/dismissPrivacyBanner, and ensure initCloudflareSkipping sets/clears that flag around challenge handling so dismissPrivacyBanner only runs when not in a Cloudflare solving state.generate-accounts-config.js (2)
52-57: Add bounds checking for numeric bank selection.If the user enters
0or a number greater than the bank list length,Object.keys(supportedBanks)[index]returnsundefined, which is then handled by the subsequent validation. However, it would be clearer to validate bounds explicitly.Additionally, consider adding a radix parameter to
parseIntfor explicitness.♻️ Suggested improvement
if (/^\d+$/.test(bankChoice)) { - const index = parseInt(bankChoice) - 1; - companyId = Object.keys(supportedBanks)[index]; + const index = parseInt(bankChoice, 10) - 1; + const bankKeys = Object.keys(supportedBanks); + companyId = (index >= 0 && index < bankKeys.length) ? bankKeys[index] : undefined; } else { companyId = bankChoice; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate-accounts-config.js` around lines 52 - 57, Validate numeric selections from bankChoice before indexing supportedBanks: when /^\d+$/.test(bankChoice) use parseInt(bankChoice, 10) and compute index = parsed - 1, then check index >= 0 && index < Object.keys(supportedBanks).length; if out of bounds, set companyId to undefined or throw/return a clear validation error instead of using Object.keys(supportedBanks)[index], otherwise assign companyId = Object.keys(supportedBanks)[index]; this ensures proper bounds checking and uses an explicit radix for parseInt.
69-72: Passwords are echoed to the terminal in plain text.When collecting credentials, the password is visible as the user types. For a local helper script this may be acceptable, but consider using a library like
readlinewith hidden input or warning users that input is visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate-accounts-config.js` around lines 69 - 72, The loop that prompts for each field (for (const field of bank.fields) { const value = await question(...); account[field] = value; }) echoes passwords to the terminal; update the prompt logic to detect sensitive fields (e.g., field names like "password", "pass", "secret") and call a hidden-input prompt instead of the plain question() for those fields; implement a helper (e.g., hiddenPrompt) that uses a muted-readline approach or a small library (readline-sync/questionHide or prompt/prompt-sync with hidden option) to suppress echo, and use the normal question() for non-sensitive fields so account[field] gets the hidden value only for credentials.src/bot/cloudFunction.ts (2)
8-15: Add TypeScript types to the handler parameters.The
reqandresparameters lack type annotations, and the caught erroreis implicitlyunknown. Consider adding types for better type safety.Proposed fix using Google Cloud Functions types
+import type { Request, Response } from "@google-cloud/functions-framework"; + // Google Cloud Function HTTP handler -export const scheduledScrape = async (req, res) => { +export const scheduledScrape = async (req: Request, res: Response) => { try { // Run the main scraping logic await runWithStorage(runScraper); res.status(200).send("Scrape completed"); - } catch (e) { + } catch (e: unknown) { - res.status(500).send("Error: " + (e?.message || e)); + const message = e instanceof Error ? e.message : String(e); + res.status(500).send("Error: " + message); } };Alternatively, if not using the Cloud Functions framework types, use Express-compatible types or a minimal inline type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/cloudFunction.ts` around lines 8 - 15, The handler scheduledScrape lacks TypeScript types for its parameters and uses an implicitly typed catch variable e; update the function signature for scheduledScrape to annotate req and res with appropriate types (e.g., Request and Response from the Google Cloud Functions/Express types or minimal inline interfaces) and change the catch to type the error as unknown then narrow it (e.g., check instanceof Error before accessing message) so the response uses a safe error message; adjust imports if you choose Request/Response from a library and keep references to runWithStorage and runScraper unchanged.
2-2: Unused import:RunnerHooks.
RunnerHooksis imported but never used in this file.Proposed fix
import { runWithStorage } from "./index.js"; -import { RunnerHooks } from "../types.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/cloudFunction.ts` at line 2, The import RunnerHooks in cloudFunction.ts is unused; remove the unused import from the top-level import list (the "RunnerHooks" symbol) to clean up the file, or if RunnerHooks was intended to be used, update the code to reference the RunnerHooks type where appropriate (e.g., in function signatures or variable types) so the import becomes necessary; typically the fix is to delete the RunnerHooks import from the import statement.src/bot/storage/monday.ts (2)
219-220: Unused variablesitemNameandcolumnValues.These escaped values are computed but never used—the GraphQL query on lines 233-234 uses the raw
transaction.descriptionandthis.getColumnValues(transaction)instead.Either use the escaped variables or remove them
- const itemName = escapeString(transaction.description); - const columnValues = escapeString(this.getColumnValues(transaction)); - // Headers for the request const headers = { 'Authorization': MONDAY_TOKEN, 'Content-Type': 'application/json' }; // The GraphQL query for creating an item const query = ` mutation { create_item (board_id: ${boardId}, create_labels_if_missing: true, - item_name: "${transaction.description.replace(/"/g, '\\"')}", - column_values: "${this.getColumnValues(transaction)}") { + item_name: "${escapeString(transaction.description)}", + column_values: "${escapeString(this.getColumnValues(transaction))}") { id } } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/storage/monday.ts` around lines 219 - 220, The two escaped variables itemName and columnValues are computed but never used; either remove their declarations or use them in the GraphQL payload. Fix by replacing the raw transaction.description and this.getColumnValues(transaction) in the mutation input with the escaped variables itemName and columnValues (created via escapeString) so the GraphQL string uses the sanitized values, or if escaping is not needed here simply delete the itemName and columnValues lines (and the escapeString calls) to eliminate unused variables; refer to escapeString, itemName, columnValues, getColumnValues, and transaction.description when making the change.
67-67: Use thedebuglogger instead ofconsolemethods.Several places use
console.info/console.errordirectly instead of theloggerfunction created on line 16. This violates the project's logging conventions.Proposed fix
- console.info(`${this.existingTransactionsHashes.size} hashes loaded`); + logger(`${this.existingTransactionsHashes.size} hashes loaded`);if (response.data.errors) { - console.error('Error fetching items:', response.data.errors); + logger('Error fetching items:', response.data.errors); return []; } else { return response.data.data.boards[0].items_page.items; } } catch (error) { - console.error('Error:', error); + logger('Error:', error); return []; }Apply similar changes to lines 247, 252-253, 278-279, and 284.
As per coding guidelines: "Use debug package with 'moneyman:*' namespace for logging."
Also applies to: 106-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/storage/monday.ts` at line 67, Replace direct console calls with the project's debug logger: where you currently call console.info(`${this.existingTransactionsHashes.size} hashes loaded`) (and other console.info/console.error usages around the monday storage class, including the blocks referenced at lines ~106-114, 247, 252-253, 278-279, 284), use the module-level logger created (the debug logger with the 'moneyman:*' namespace) and the appropriate log level method (e.g., logger.debug or logger.error) so messages follow the project's logging conventions; locate uses inside the class that reference this.existingTransactionsHashes and any error handlers and replace console calls with logger.<level>(...) preserving the original message text and include any error objects/stack where applicable.src/config.schema.ts (1)
94-98: Inconsistent email validation syntax.Line 96 uses
z.string().email()while line 15 usesz.email()for the Google service account email. Both are valid in Zod v4, but consider using a consistent approach throughout the schema.♻️ Suggested fix for consistency
export const InvoiceSchema = z.object({ baseUrl: z.url({ error: "Invalid invoice base URL" }), - developerEmail: z.string().email({ error: "Invalid developer email" }), + developerEmail: z.email({ error: "Invalid developer email" }), apiKey: z.string().min(1, { error: "Invoice API key is required" }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.schema.ts` around lines 94 - 98, The developerEmail field in InvoiceSchema uses z.string().email() while the Google service account email uses z.email(); make them consistent by updating the InvoiceSchema developerEmail to use z.email({ error: "Invalid developer email" }) (or vice versa across the file if you prefer z.string().email) so both email validators use the same Zod API; change the developerEmail definition in InvoiceSchema to match the chosen style.src/bot/storage/index.ts (1)
36-53: Hardcoded vendor list may need externalization.The insurance vendor names are hardcoded in Hebrew. Consider moving these to configuration to allow users to customize the list without code changes.
♻️ Suggested approach
Consider adding an
insuranceVendorsarray to the invoice configuration schema:// In config.schema.ts export const InvoiceSchema = z.object({ baseUrl: z.url({ error: "Invalid invoice base URL" }), developerEmail: z.email({ error: "Invalid developer email" }), apiKey: z.string().min(1, { error: "Invoice API key is required" }), insuranceVendors: z.array(z.string()).optional().default([ "ילין", "אלטשולר", "מור", "אנליסט", "מגדל", "כלל", "הראל", "פניקס", "מנורה" ]), });Then read from config in
filterInsuranceVendorTransactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot/storage/index.ts` around lines 36 - 53, The vendor list in filterInsuranceVendorTransactions is hardcoded; move it into configuration by adding an insuranceVendors string array to your invoice config schema (e.g., InvoiceSchema with insuranceVendors defaulting to the current Hebrew names) and update filterInsuranceVendorTransactions to read the vendor list from the config object (e.g., config.invoice.insuranceVendors) and build the RegExp from that array instead of the inline vendors variable so users can customize the list without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.local.json:
- Around line 1-25: The committed .claude/settings.local.json contains
machine-specific absolute paths and must be removed and ignored: add
".claude/settings.local.json" to .gitignore, remove the tracked file from the
repo (git rm --cached .claude/settings.local.json and commit) and replace it in
the repo with a sanitized template named ".claude/settings.local.json.example"
that uses relative paths/placeholders; ensure any CI or developer docs reference
the example and instructions to create a local .claude/settings.local.json from
the example.
In `@CLAUDE.md`:
- Around line 24-41: The fenced code block in CLAUDE.md showing the project
directory tree lacks a language identifier; update that code fence to include a
language token (e.g., "text") so linting and renderers correctly highlight
it—modify the opening triple-backtick in CLAUDE.md for the directory structure
to be ```text (or another appropriate language) while leaving the block contents
unchanged.
In `@generate-accounts-config.js`:
- Around line 91-94: The printed secret names in generate-accounts-config.js are
incorrect; update the console.log messages that currently show
ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES to the actual secret
names used by the workflows (MONEYMAN_CONFIG for scrape.yml and
MONEYMAN_CONFIG_INVOICES for scrape-invoices.yml) so users create matching
repository secrets; alternatively, if you prefer the documented names, change
the secret variables in scrape.yml and scrape-invoices.yml to match the script,
but the simplest fix is to edit the messages in generate-accounts-config.js to
reference MONEYMAN_CONFIG and MONEYMAN_CONFIG_INVOICES.
In `@GITHUB_SECRETS_SETUP.md`:
- Around line 9-14: The docs currently list secret names
ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES and show only a JSON
array, but the workflows expect secrets.MONEYMAN_CONFIG (main) and
secrets.MONEYMAN_CONFIG_INVOICES (invoices) and a full config object; update the
secret names in GITHUB_SECRETS_SETUP.md to MONEYMAN_CONFIG and
MONEYMAN_CONFIG_INVOICES and replace the example payload to the full schema that
contains "accounts" (array), "storage" (object), and "options" (object) so the
secret format matches the runtime parsing used by the workflows (referencing the
MONEYMAN_CONFIG and MONEYMAN_CONFIG_INVOICES usage and the
accounts/storage/options fields).
In `@src/bot/storage/InvoiceCreator.ts`:
- Around line 58-66: The fetch in InvoiceCreator that posts to
`${this.baseUrl}/api/createDoc` lacks a timeout; update the fetch call to
include a signal created with AbortSignal.timeout(timeoutMs) (choose a sensible
timeout value or use a configured constant) by adding signal:
AbortSignal.timeout(...) to the fetch options, and catch the resulting
DOMException/AbortError to throw a clear error (e.g., indicate invoice creation
timed out for txn ${txn.uniqueId || txn.hash}); keep the existing non-OK
response handling for other HTTP errors.
- Around line 69-80: In createInvoicesForTransactions, individual errors from
createInvoiceForTransaction are currently swallowed; update the catch to log the
error (including txn identifier like txn.id or txn.transaction_id) and
optionally attach error details to the txn (e.g., txn.error = e.message or
txn.invoice_error) so callers can see failures; keep the loop behavior (continue
on error) but ensure processLogger or a logger is used to emit a clear message
referencing createInvoicesForTransactions and the failing txn along with the
error stack.
- Around line 28-31: The code calls this.formatDate(txn.processedDate) without
guarding for undefined; update createInvoiceForTransaction to check
txn.processedDate and use an explicit fallback (for example txn.transactionDate
or txn.createdAt) or throw a clear error if none exists, then pass a Date
constructed from that chosen value into formatDate; reference the
createInvoiceForTransaction method and the TransactionRow.processedDate property
when making this change so you don't accidentally rely on new Date(undefined).
- Around line 1-3: Add debug logging using the debug package for this storage
class: import debug from "debug" and create a logger like const dbg =
debug("moneyman:storage:invoice"); then sprinkle dbg(...) calls inside the
InvoiceCreator class (e.g., in the constructor and its main public methods such
as createInvoice, save, or any method that builds/persists TransactionRow) to
log entry, key parameter values (non-sensitive), success/failure results and
error objects; reference TransactionRow and MoneymanConfig where relevant and
avoid printing secrets or full payment data.
In `@src/bot/storage/monday.js`:
- Line 1: The file only contains an unused import and is redundant because the
real implementation is in monday.ts; either delete this
src/bot/storage/monday.js and update any imports in storage index (referencing
MondayStorage in src/bot/storage/index.ts) to import from ./monday.ts, or
convert monday.js into a thin re-export that exports the MondayStorage
implementation from ./monday.ts (e.g., export { MondayStorage } from
"./monday";) so consumers importing from "monday" continue to work. Ensure
references to createLogger are preserved only in the actual implementation file
(monday.ts) and run tests/compile to verify imports resolve.
In `@src/bot/storage/monday.ts`:
- Around line 117-120: The implementation of saveTransactions in
src/bot/storage/monday.ts omits the optional third parameter from the
TransactionStorage interface; update the saveTransactions signature to accept
the optional context?: SaveContext parameter (matching the interface) and ensure
the function uses or forwards that context where needed (e.g., to any helper
functions or when called from src/bot/storage/index.ts), referencing the
saveTransactions method and the SaveContext/TransactionStorage types to keep the
API consistent.
- Around line 183-186: The code is double-counting skipped transactions by
adding stats.existing to stats.otherSkipped after sending to Monday; remove that
extra aggregation so stats.otherSkipped is only incremented where individual
skips occur (i.e., the per-transaction increments already done in the loop).
Locate the post-send block that calls logToPublicLog("transactions sent to
Monday successfully!") and remove or stop adding stats.existing to
stats.otherSkipped there (leave the per-item increments in the loop intact).
In `@src/scraper/browser.ts`:
- Around line 118-144: Wrap the async callbacks in initBannerDismissal with
runInLoggerContext to preserve logger context/runId: specifically wrap the
browserContext.on("targetcreated", ...) handler and the
page.on("framenavigated", ...) handler so their work (including calling
dismissPrivacyBanner) executes inside runInLoggerContext. Replace the
unconditional await sleep(2000) on main-frame navigations with a targeted, short
wait for known banner selectors (e.g., waitForSelector with a small timeout) and
only fall back to a much shorter delay if necessary, then call
dismissPrivacyBanner(page); ensure errors are still caught and logged via logger
inside the runInLoggerContext wrapper.
---
Outside diff comments:
In `@package.json`:
- Around line 10-21: Restore the postinstall npm script in package.json so
patch-package runs after installs: add "postinstall": "patch-package" into the
"scripts" object (ensuring patch-package remains in devDependencies and the
existing patch files in patches/ will be applied), so that installs (npm install
/ npm ci) and the Docker build which copies the patches will apply the
ky+1.14.3.patch and yargs+17.7.2.patch correctly.
---
Nitpick comments:
In `@generate-accounts-config.js`:
- Around line 52-57: Validate numeric selections from bankChoice before indexing
supportedBanks: when /^\d+$/.test(bankChoice) use parseInt(bankChoice, 10) and
compute index = parsed - 1, then check index >= 0 && index <
Object.keys(supportedBanks).length; if out of bounds, set companyId to undefined
or throw/return a clear validation error instead of using
Object.keys(supportedBanks)[index], otherwise assign companyId =
Object.keys(supportedBanks)[index]; this ensures proper bounds checking and uses
an explicit radix for parseInt.
- Around line 69-72: The loop that prompts for each field (for (const field of
bank.fields) { const value = await question(...); account[field] = value; })
echoes passwords to the terminal; update the prompt logic to detect sensitive
fields (e.g., field names like "password", "pass", "secret") and call a
hidden-input prompt instead of the plain question() for those fields; implement
a helper (e.g., hiddenPrompt) that uses a muted-readline approach or a small
library (readline-sync/questionHide or prompt/prompt-sync with hidden option) to
suppress echo, and use the normal question() for non-sensitive fields so
account[field] gets the hidden value only for credentials.
In `@src/bot/cloudFunction.ts`:
- Around line 8-15: The handler scheduledScrape lacks TypeScript types for its
parameters and uses an implicitly typed catch variable e; update the function
signature for scheduledScrape to annotate req and res with appropriate types
(e.g., Request and Response from the Google Cloud Functions/Express types or
minimal inline interfaces) and change the catch to type the error as unknown
then narrow it (e.g., check instanceof Error before accessing message) so the
response uses a safe error message; adjust imports if you choose
Request/Response from a library and keep references to runWithStorage and
runScraper unchanged.
- Line 2: The import RunnerHooks in cloudFunction.ts is unused; remove the
unused import from the top-level import list (the "RunnerHooks" symbol) to clean
up the file, or if RunnerHooks was intended to be used, update the code to
reference the RunnerHooks type where appropriate (e.g., in function signatures
or variable types) so the import becomes necessary; typically the fix is to
delete the RunnerHooks import from the import statement.
In `@src/bot/storage/index.ts`:
- Around line 36-53: The vendor list in filterInsuranceVendorTransactions is
hardcoded; move it into configuration by adding an insuranceVendors string array
to your invoice config schema (e.g., InvoiceSchema with insuranceVendors
defaulting to the current Hebrew names) and update
filterInsuranceVendorTransactions to read the vendor list from the config object
(e.g., config.invoice.insuranceVendors) and build the RegExp from that array
instead of the inline vendors variable so users can customize the list without
code changes.
In `@src/bot/storage/monday.ts`:
- Around line 219-220: The two escaped variables itemName and columnValues are
computed but never used; either remove their declarations or use them in the
GraphQL payload. Fix by replacing the raw transaction.description and
this.getColumnValues(transaction) in the mutation input with the escaped
variables itemName and columnValues (created via escapeString) so the GraphQL
string uses the sanitized values, or if escaping is not needed here simply
delete the itemName and columnValues lines (and the escapeString calls) to
eliminate unused variables; refer to escapeString, itemName, columnValues,
getColumnValues, and transaction.description when making the change.
- Line 67: Replace direct console calls with the project's debug logger: where
you currently call console.info(`${this.existingTransactionsHashes.size} hashes
loaded`) (and other console.info/console.error usages around the monday storage
class, including the blocks referenced at lines ~106-114, 247, 252-253, 278-279,
284), use the module-level logger created (the debug logger with the
'moneyman:*' namespace) and the appropriate log level method (e.g., logger.debug
or logger.error) so messages follow the project's logging conventions; locate
uses inside the class that reference this.existingTransactionsHashes and any
error handlers and replace console calls with logger.<level>(...) preserving the
original message text and include any error objects/stack where applicable.
In `@src/config.schema.ts`:
- Around line 94-98: The developerEmail field in InvoiceSchema uses
z.string().email() while the Google service account email uses z.email(); make
them consistent by updating the InvoiceSchema developerEmail to use z.email({
error: "Invalid developer email" }) (or vice versa across the file if you prefer
z.string().email) so both email validators use the same Zod API; change the
developerEmail definition in InvoiceSchema to match the chosen style.
In `@src/scraper/browser.ts`:
- Around line 185-196: The reverse iteration over the NodeList
(Array.from(buttons).reverse()) needs a short inline comment explaining why we
iterate from the end (e.g., accept/dismiss buttons often appear last in banner
markup), so add a one-line comment immediately above the
Array.from(buttons).reverse() call mentioning that we prefer the last buttons
first to correctly target accept/close actions; reference the variables and call
names: banner.querySelectorAll, buttons, Array.from(buttons).reverse(), and the
click handling in the loop to make the intent obvious to future readers.
- Around line 146-166: In dismissPrivacyBanner, the broad page.evaluate that
queries all 'button, a' can be slow and the current text check requires both
Hebrew words; narrow the DOM scan and make matching more flexible: target likely
banner containers or controls (e.g., selectors such as role/dialog, common
banner classes, or a small subset like 'header, footer, [role="dialog"] button,
.cookie-banner button') and limit the node list size before iterating, and
change the text test from requiring both 'הבנתי' && 'תודה' to an OR or regex
that matches either token or common variants so banners with only one keyword
are dismissed; update the evaluate call inside dismissPrivacyBanner to use the
targeted selector and relaxed text matching.
- Around line 133-136: The banner dismissal delay can race with Cloudflare
challenge handling; update the logic in the framenavigated handlers
(initDomainTracking, initCloudflareSkipping, initBannerDismissal) so
dismissPrivacyBanner is skipped when Cloudflare solving is in progress: detect
the Cloudflare state (e.g., check for __cf_chl_rt_tk in the URL or a shared
"solving" flag set by initCloudflareSkipping) before calling
sleep/dismissPrivacyBanner, and ensure initCloudflareSkipping sets/clears that
flag around challenge handling so dismissPrivacyBanner only runs when not in a
Cloudflare solving state.
🪄 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: e48a6e97-f059-4146-b225-f9afbdf774bb
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/bot/__snapshots__/messages.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
.claude/settings.local.json.gcloudignore.github/workflows/scrape-invoices.yml.github/workflows/scrape.yml.github/workflows/sync-upstream.yml.gitignoreCLAUDE.mdGITHUB_SECRETS_SETUP.mdconfig.example.jsoncgenerate-accounts-config.jspackage.jsonsrc/bot/cloudFunction.tssrc/bot/index.tssrc/bot/storage/InvoiceCreator.tssrc/bot/storage/index.tssrc/bot/storage/monday.jssrc/bot/storage/monday.tssrc/bot/storage/moneyman.test.tssrc/bot/storage/moneyman.tssrc/bot/transactionTableRow.tssrc/config.schema.tssrc/index.tssrc/scraper/browser.tssrc/types.tssrc/utils/asyncContext.tssrc/utils/secure-log.ts
Comment on lines
+1
to
+25
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(cat:*)", | ||
| "Bash(gh run:*)", | ||
| "Bash(git fetch:*)", | ||
| "Bash(git merge:*)", | ||
| "Bash(git:*)", | ||
| "Bash(npm install:*)", | ||
| "Bash(gh secret:*)", | ||
| "Bash(cat /Users/Edoz/Development/moneyman/tsconfig.json && cat /Users/Edoz/Development/moneyman/package.json | python3 -c \"import sys,json; d=json.load\\(sys.stdin\\); print\\('type:', d.get\\('type'\\)\\); print\\('name:', d.get\\('name'\\)\\)\")", | ||
| "Bash(npm run:*)", | ||
| "Bash(MONEYMAN_CONFIG_PATH=moneyman.conf.jsonc MONEYMAN_UNSAFE_STDOUT=true npm run start 2>&1)", | ||
| "Bash(MONEYMAN_CONFIG_PATH=moneyman.invoices.conf.jsonc MONEYMAN_UNSAFE_STDOUT=true npm run start 2>&1)", | ||
| "Bash(find /Users/Edoz/Development/moneyman/node_modules/israeli-bank-scrapers/lib -name \"max*\" -o -name \"visa-cal*\" -o -name \"cal*\" 2>/dev/null | head -20)", | ||
| "Bash(npm view:*)", | ||
| "mcp__plugin_chrome-devtools-mcp_chrome-devtools__new_page", | ||
| "mcp__plugin_chrome-devtools-mcp_chrome-devtools__wait_for", | ||
| "mcp__plugin_chrome-devtools-mcp_chrome-devtools__list_pages", | ||
| "mcp__plugin_chrome-devtools-mcp_chrome-devtools__take_screenshot", | ||
| "mcp__plugin_chrome-devtools-mcp_chrome-devtools__evaluate_script", | ||
| "Skill(commit-commands:commit-push-pr)" | ||
| ] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This local configuration file contains machine-specific paths and should not be committed.
This file contains hardcoded absolute paths (e.g., /Users/Edoz/Development/moneyman/) that are specific to one developer's machine. This configuration won't work for other contributors and exposes local directory structure.
Consider:
- Adding
.claude/settings.local.jsonto.gitignore - Using a
.claude/settings.local.json.exampletemplate with relative paths or placeholders instead
🛠️ Suggested .gitignore addition
moneyman.invoices.conf.jsonc
firebase-debug.log
+.claude/settings.local.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/settings.local.json around lines 1 - 25, The committed
.claude/settings.local.json contains machine-specific absolute paths and must be
removed and ignored: add ".claude/settings.local.json" to .gitignore, remove the
tracked file from the repo (git rm --cached .claude/settings.local.json and
commit) and replace it in the repo with a sanitized template named
".claude/settings.local.json.example" that uses relative paths/placeholders;
ensure any CI or developer docs reference the example and instructions to create
a local .claude/settings.local.json from the example.
Comment on lines
+24
to
+41
| ``` | ||
| src/ | ||
| index.ts # entry point: orchestrates scrape → store | ||
| config.ts # parses MONEYMAN_CONFIG / MONEYMAN_CONFIG_PATH (JSONC, Zod validation) | ||
| config.schema.ts # Zod schema for full config | ||
| types.ts # shared TypeScript types | ||
| runnerMetadata.ts # external IP logging, run metadata | ||
| scraper/ # wraps israeli-bank-scrapers, handles OTP/browser | ||
| bot/ | ||
| index.ts # runWithStorage: wires scraper results to all storage backends | ||
| notifier.ts # Telegram error/status notifications | ||
| storage/ # one file per backend: sheets, sql, ynab, buxfer, actual, | ||
| # azure-data-explorer, telegram, json, web-post, monday | ||
| security/ # domain firewall (per-scraper allow/block rules) | ||
| utils/ # logger, secure-log, currency, promises, timer, jsonc | ||
| scripts/ # verify-config.ts (standalone validation) | ||
| dst/ # compiled JS output (gitignored, regenerated by tsc) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify language for the fenced code block.
The directory structure code block should have a language identifier for better rendering and to satisfy markdown linting rules.
📝 Proposed fix
-```
+```text
src/
index.ts # entry point: orchestrates scrape → store📝 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.
Suggested change
| ``` | |
| src/ | |
| index.ts # entry point: orchestrates scrape → store | |
| config.ts # parses MONEYMAN_CONFIG / MONEYMAN_CONFIG_PATH (JSONC, Zod validation) | |
| config.schema.ts # Zod schema for full config | |
| types.ts # shared TypeScript types | |
| runnerMetadata.ts # external IP logging, run metadata | |
| scraper/ # wraps israeli-bank-scrapers, handles OTP/browser | |
| bot/ | |
| index.ts # runWithStorage: wires scraper results to all storage backends | |
| notifier.ts # Telegram error/status notifications | |
| storage/ # one file per backend: sheets, sql, ynab, buxfer, actual, | |
| # azure-data-explorer, telegram, json, web-post, monday | |
| security/ # domain firewall (per-scraper allow/block rules) | |
| utils/ # logger, secure-log, currency, promises, timer, jsonc | |
| scripts/ # verify-config.ts (standalone validation) | |
| dst/ # compiled JS output (gitignored, regenerated by tsc) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 24 - 41, The fenced code block in CLAUDE.md showing
the project directory tree lacks a language identifier; update that code fence
to include a language token (e.g., "text") so linting and renderers correctly
highlight it—modify the opening triple-backtick in CLAUDE.md for the directory
structure to be ```text (or another appropriate language) while leaving the
block contents unchanged.
Comment on lines
+91
to
+94
| console.log('3. Create a new repository secret with the name:'); | ||
| console.log(' - ACCOUNTS_JSON_MONDAY_STORAGE (for monday-storage branch)'); | ||
| console.log(' - ACCOUNTS_JSON_INVOICES (for invoices branch)'); | ||
| console.log('4. Paste the JSON configuration as the secret value'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which secret names are actually used in workflows
echo "=== Secrets referenced in workflow files ==="
rg -n "secrets\." .github/workflows/ --type yamlRepository: daniel-hauser/moneyman
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Read the specific lines from the script
echo "=== Content of generate-accounts-config.js at lines 91-94 ==="
sed -n '91,94p' generate-accounts-config.js
echo ""
echo "=== Search for ACCOUNTS_JSON references in codebase ==="
rg "ACCOUNTS_JSON" --type js --type yaml
echo ""
echo "=== All workflow files and their secret names ==="
rg "MONEYMAN_CONFIG|ACCOUNTS_JSON" .github/workflows/ --type yaml -nRepository: daniel-hauser/moneyman
Length of output: 1297
Update documentation to reflect actual secret names used in workflows.
The script instructs users to create secrets named ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES, but the workflows actually expect MONEYMAN_CONFIG (in scrape.yml) and MONEYMAN_CONFIG_INVOICES (in scrape-invoices.yml). Update lines 91-94 to match the actual secret names workflows use, or update the workflow files to use the documented names. Users following the current instructions will create secrets that workflows won't recognize, causing failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generate-accounts-config.js` around lines 91 - 94, The printed secret names
in generate-accounts-config.js are incorrect; update the console.log messages
that currently show ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES to
the actual secret names used by the workflows (MONEYMAN_CONFIG for scrape.yml
and MONEYMAN_CONFIG_INVOICES for scrape-invoices.yml) so users create matching
repository secrets; alternatively, if you prefer the documented names, change
the secret variables in scrape.yml and scrape-invoices.yml to match the script,
but the simplest fix is to edit the messages in generate-accounts-config.js to
reference MONEYMAN_CONFIG and MONEYMAN_CONFIG_INVOICES.
Comment on lines
+9
to
+14
| ### 1. `ACCOUNTS_JSON_MONDAY_STORAGE` | ||
| For the `monday-storage` branch workflow | ||
|
|
||
| ### 2. `ACCOUNTS_JSON_INVOICES` | ||
| For the `invoices` branch workflow | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the expected config structure from config.schema.ts
echo "=== Config schema definition ==="
cat src/config.schema.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== Sample config structure (if example exists) ==="
fd -t f "moneyman.conf" --exec head -50 {} 2>/dev/null || echo "No sample config found"Repository: daniel-hauser/moneyman
Length of output: 7149
🏁 Script executed:
# Check scrape-invoices.yml for secret usage around line 48
rg -A 3 -B 3 "secrets\." .github/workflows/scrape-invoices.yml | head -50Repository: daniel-hauser/moneyman
Length of output: 402
🏁 Script executed:
# Check scrape.yml for secret usage
rg -A 3 -B 3 "secrets\." .github/workflows/scrape.yml | head -50Repository: daniel-hauser/moneyman
Length of output: 393
🏁 Script executed:
# Read GITHUB_SECRETS_SETUP.md to see full documentation content
cat -n GITHUB_SECRETS_SETUP.mdRepository: daniel-hauser/moneyman
Length of output: 4541
Secret names and configuration format in documentation don't match workflow usage.
The documentation references ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES, but the workflows use secrets.MONEYMAN_CONFIG (main workflow) and secrets.MONEYMAN_CONFIG_INVOICES (invoices workflow). Additionally, the documentation shows only a JSON array of accounts, but the actual config schema expects a complete configuration object with accounts, storage, and options fields.
Users following this guide will create secrets with incorrect names and invalid configuration format, causing workflow failures.
📝 Suggested fixes
Update the secret names and configuration examples to match actual workflow requirements:
-### 1. `ACCOUNTS_JSON_MONDAY_STORAGE`
+### 1. `MONEYMAN_CONFIG`
For the `monday-storage` branch workflow
-### 2. `ACCOUNTS_JSON_INVOICES`
+### 2. `MONEYMAN_CONFIG_INVOICES`
For the `invoices` branch workflowUpdate the configuration format to show the complete structure with storage and options:
-## ACCOUNTS_JSON Format
+## Configuration Format
-Each secret should contain a JSON array with account configurations. Here's the structure:
+Each secret should contain a complete JSONC configuration. Here's the structure:
```json
{
+ "accounts": [
{
"companyId": "hapoalim",
"userCode": "AB1234",
"password": "your-password"
},
{
"companyId": "visaCal",
"username": "Your Username",
"password": "your-password"
},
{
"companyId": "leumi",
"username": "Your Username",
"password": "your-password"
}
+ ],
+ "storage": {
+ "localJson": {
+ "enabled": true
+ }
+ },
+ "options": {}
}📝 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.
Suggested change
| ### 1. `ACCOUNTS_JSON_MONDAY_STORAGE` | |
| For the `monday-storage` branch workflow | |
| ### 2. `ACCOUNTS_JSON_INVOICES` | |
| For the `invoices` branch workflow | |
| ### 1. `MONEYMAN_CONFIG` | |
| For the `monday-storage` branch workflow | |
| ### 2. `MONEYMAN_CONFIG_INVOICES` | |
| For the `invoices` branch workflow |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GITHUB_SECRETS_SETUP.md` around lines 9 - 14, The docs currently list secret
names ACCOUNTS_JSON_MONDAY_STORAGE and ACCOUNTS_JSON_INVOICES and show only a
JSON array, but the workflows expect secrets.MONEYMAN_CONFIG (main) and
secrets.MONEYMAN_CONFIG_INVOICES (invoices) and a full config object; update the
secret names in GITHUB_SECRETS_SETUP.md to MONEYMAN_CONFIG and
MONEYMAN_CONFIG_INVOICES and replace the example payload to the full schema that
contains "accounts" (array), "storage" (object), and "options" (object) so the
secret format matches the runtime parsing used by the workflows (referencing the
MONEYMAN_CONFIG and MONEYMAN_CONFIG_INVOICES usage and the
accounts/storage/options fields).
Comment on lines
+1
to
+3
| import { TransactionRow } from "../../types"; | ||
| import type { MoneymanConfig } from "../../config.js"; | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing debug logging per coding guidelines.
Per coding guidelines, storage-related code should use the debug package with 'moneyman:*' namespace for logging. This class has no logging, making it difficult to debug invoice creation issues.
♻️ Add debug logging
import { TransactionRow } from "../../types";
import type { MoneymanConfig } from "../../config.js";
+import { createLogger } from "../../utils/logger.js";
+
+const logger = createLogger("InvoiceCreator");
export class InvoiceCreator {As per coding guidelines: "Use debug package with 'moneyman:*' namespace for logging" for src/**/*.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/storage/InvoiceCreator.ts` around lines 1 - 3, Add debug logging
using the debug package for this storage class: import debug from "debug" and
create a logger like const dbg = debug("moneyman:storage:invoice"); then
sprinkle dbg(...) calls inside the InvoiceCreator class (e.g., in the
constructor and its main public methods such as createInvoice, save, or any
method that builds/persists TransactionRow) to log entry, key parameter values
(non-sensitive), success/failure results and error objects; reference
TransactionRow and MoneymanConfig where relevant and avoid printing secrets or
full payment data.
Comment on lines
+69
to
+80
| async createInvoicesForTransactions(txns: Array<TransactionRow>): Promise<TransactionRow[]> { | ||
| for (const txn of txns) { | ||
| try { | ||
| const res = await this.createInvoiceForTransaction(txn); | ||
| if (res.pdf_link) txn.pdf_link = res.pdf_link; | ||
| if (res.doc_number) txn.doc_number = res.doc_number; | ||
| } catch (e: any) { | ||
| // Optionally, you can add error handling fields to txn here | ||
| } | ||
| } | ||
| return txns; | ||
| } |
Contributor
There was a problem hiding this comment.
Silent error swallowing loses failure context.
Individual transaction failures are caught but the error details are discarded. This makes it impossible to diagnose which transactions failed and why. Consider at minimum logging the error.
🐛 Proposed fix with logging
async createInvoicesForTransactions(txns: Array<TransactionRow>): Promise<TransactionRow[]> {
for (const txn of txns) {
try {
const res = await this.createInvoiceForTransaction(txn);
if (res.pdf_link) txn.pdf_link = res.pdf_link;
if (res.doc_number) txn.doc_number = res.doc_number;
} catch (e: any) {
- // Optionally, you can add error handling fields to txn here
+ logger(`Failed to create invoice for txn ${txn.uniqueId || txn.hash}:`, e?.message || e);
}
}
return txns;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/storage/InvoiceCreator.ts` around lines 69 - 80, In
createInvoicesForTransactions, individual errors from
createInvoiceForTransaction are currently swallowed; update the catch to log the
error (including txn identifier like txn.id or txn.transaction_id) and
optionally attach error details to the txn (e.g., txn.error = e.message or
txn.invoice_error) so callers can see failures; keep the loop behavior (continue
on error) but ensure processLogger or a logger is used to emit a clear message
referencing createInvoicesForTransactions and the failing txn along with the
error stack.
| @@ -0,0 +1 @@ | |||
| import { createLogger } from "../../utils/logger.js"; No newline at end of file | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if this file is imported anywhere or if monday.ts exists
echo "=== Checking for imports of monday.js ==="
rg -n "from.*monday\.js" --type ts --type js
echo ""
echo "=== Checking if monday.ts exists ==="
fd -t f "monday.ts" src/
echo ""
echo "=== Checking contents of any monday.ts file ==="
fd -t f "monday.ts" src/ --exec cat {}Repository: daniel-hauser/moneyman
Length of output: 11018
File is incomplete and likely redundant.
This file contains only a single import statement with no exports or implementation. The actual MondayStorage implementation resides in src/bot/storage/monday.ts. Either remove this file and update the import in src/bot/storage/index.ts to reference monday.ts directly, or have monday.js re-export the class from monday.ts if intentionally maintaining both files for a specific reason.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/storage/monday.js` at line 1, The file only contains an unused import
and is redundant because the real implementation is in monday.ts; either delete
this src/bot/storage/monday.js and update any imports in storage index
(referencing MondayStorage in src/bot/storage/index.ts) to import from
./monday.ts, or convert monday.js into a thin re-export that exports the
MondayStorage implementation from ./monday.ts (e.g., export { MondayStorage }
from "./monday";) so consumers importing from "monday" continue to work. Ensure
references to createLogger are preserved only in the actual implementation file
(monday.ts) and run tests/compile to verify imports resolve.
Comment on lines
+117
to
+120
| async saveTransactions( | ||
| txns: Array<TransactionRow>, | ||
| onProgress: (status: string) => Promise<void> | ||
| ) { |
Contributor
There was a problem hiding this comment.
Missing context parameter in saveTransactions signature.
The TransactionStorage interface (per src/types.ts:50-60) declares saveTransactions with an optional third parameter context?: SaveContext. This implementation omits it, which will cause the context passed from src/bot/storage/index.ts:123-130 to be silently ignored.
Proposed fix
+import type { SaveContext } from "../../types.js";
+
async saveTransactions(
txns: Array<TransactionRow>,
- onProgress: (status: string) => Promise<void>
+ onProgress: (status: string) => Promise<void>,
+ context?: SaveContext,
) {📝 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.
Suggested change
| async saveTransactions( | |
| txns: Array<TransactionRow>, | |
| onProgress: (status: string) => Promise<void> | |
| ) { | |
| import type { SaveContext } from "../../types.js"; | |
| async saveTransactions( | |
| txns: Array<TransactionRow>, | |
| onProgress: (status: string) => Promise<void>, | |
| context?: SaveContext, | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/storage/monday.ts` around lines 117 - 120, The implementation of
saveTransactions in src/bot/storage/monday.ts omits the optional third parameter
from the TransactionStorage interface; update the saveTransactions signature to
accept the optional context?: SaveContext parameter (matching the interface) and
ensure the function uses or forwards that context where needed (e.g., to any
helper functions or when called from src/bot/storage/index.ts), referencing the
saveTransactions method and the SaveContext/TransactionStorage types to keep the
API consistent.
Comment on lines
+183
to
+186
|
|
||
| logToPublicLog("transactions sent to Monday successfully!"); | ||
| stats.otherSkipped += stats.existing; | ||
| } |
Contributor
There was a problem hiding this comment.
Double-counting in stats.otherSkipped.
stats.existing is already incremented alongside stats.otherSkipped++ in the loop (lines 140-142, 153-154). Adding it again here causes double-counting when transactions are sent.
Proposed fix
if (txToSend.length > 0) {
// ...
const resp = await this.createItemsFromTransactions(+MONDAY_BOARD_ID, txToSend);
logToPublicLog("transactions sent to Monday successfully!");
- stats.otherSkipped += stats.existing;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/storage/monday.ts` around lines 183 - 186, The code is
double-counting skipped transactions by adding stats.existing to
stats.otherSkipped after sending to Monday; remove that extra aggregation so
stats.otherSkipped is only incremented where individual skips occur (i.e., the
per-transaction increments already done in the loop). Locate the post-send block
that calls logToPublicLog("transactions sent to Monday successfully!") and
remove or stop adding stats.existing to stats.otherSkipped there (leave the
per-item increments in the loop intact).
Comment on lines
+118
to
+144
| async function initBannerDismissal(browserContext: BrowserContext) { | ||
| logger("Setting up banner dismissal"); | ||
| browserContext.on("targetcreated", async (target) => { | ||
| if (target.type() === TargetType.PAGE) { | ||
| const page = await target.page(); | ||
| if (!page) return; | ||
|
|
||
| // Try to dismiss banner after page loads | ||
| page.on("framenavigated", async (frame) => { | ||
| const url = frame.url(); | ||
| if (!url || url === "about:blank") return; | ||
|
|
||
| // Only handle main frame navigations | ||
| if (frame.parentFrame()) return; | ||
|
|
||
| // Wait a bit for the page to render | ||
| try { | ||
| await sleep(2000); | ||
| await dismissPrivacyBanner(page); | ||
| } catch (error) { | ||
| // Silently fail - banner might not be present | ||
| logger("Banner dismissal check failed (might not be present)", error); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing runInLoggerContext wrapper breaks async context tracing.
Unlike initCloudflareSkipping (lines 62-115) and initDomainTracking, the callbacks here are not wrapped with runInLoggerContext. This causes the logger context (including runId) to be lost, breaking tracing consistency.
Additionally, the hard-coded 2000ms delay runs on every main-frame navigation, adding latency even when no banner exists.
♻️ Proposed fix to add async context and reduce latency
async function initBannerDismissal(browserContext: BrowserContext) {
+ const activeContext = loggerContextStore.getStore();
logger("Setting up banner dismissal");
- browserContext.on("targetcreated", async (target) => {
+ browserContext.on("targetcreated", runInLoggerContext(async (target) => {
if (target.type() === TargetType.PAGE) {
const page = await target.page();
if (!page) return;
// Try to dismiss banner after page loads
- page.on("framenavigated", async (frame) => {
+ page.on("framenavigated", runInLoggerContext(async (frame) => {
const url = frame.url();
if (!url || url === "about:blank") return;
// Only handle main frame navigations
if (frame.parentFrame()) return;
// Wait a bit for the page to render
try {
- await sleep(2000);
+ await sleep(1000); // Reduced delay - banner scripts usually load quickly
await dismissPrivacyBanner(page);
} catch (error) {
// Silently fail - banner might not be present
logger("Banner dismissal check failed (might not be present)", error);
}
- });
+ }, activeContext));
}
- });
+ }, activeContext));
}📝 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.
Suggested change
| async function initBannerDismissal(browserContext: BrowserContext) { | |
| logger("Setting up banner dismissal"); | |
| browserContext.on("targetcreated", async (target) => { | |
| if (target.type() === TargetType.PAGE) { | |
| const page = await target.page(); | |
| if (!page) return; | |
| // Try to dismiss banner after page loads | |
| page.on("framenavigated", async (frame) => { | |
| const url = frame.url(); | |
| if (!url || url === "about:blank") return; | |
| // Only handle main frame navigations | |
| if (frame.parentFrame()) return; | |
| // Wait a bit for the page to render | |
| try { | |
| await sleep(2000); | |
| await dismissPrivacyBanner(page); | |
| } catch (error) { | |
| // Silently fail - banner might not be present | |
| logger("Banner dismissal check failed (might not be present)", error); | |
| } | |
| }); | |
| } | |
| }); | |
| } | |
| async function initBannerDismissal(browserContext: BrowserContext) { | |
| const activeContext = loggerContextStore.getStore(); | |
| logger("Setting up banner dismissal"); | |
| browserContext.on("targetcreated", runInLoggerContext(async (target) => { | |
| if (target.type() === TargetType.PAGE) { | |
| const page = await target.page(); | |
| if (!page) return; | |
| // Try to dismiss banner after page loads | |
| page.on("framenavigated", runInLoggerContext(async (frame) => { | |
| const url = frame.url(); | |
| if (!url || url === "about:blank") return; | |
| // Only handle main frame navigations | |
| if (frame.parentFrame()) return; | |
| // Wait a bit for the page to render | |
| try { | |
| await sleep(1000); // Reduced delay - banner scripts usually load quickly | |
| await dismissPrivacyBanner(page); | |
| } catch (error) { | |
| // Silently fail - banner might not be present | |
| logger("Banner dismissal check failed (might not be present)", error); | |
| } | |
| }, activeContext)); | |
| } | |
| }, activeContext)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scraper/browser.ts` around lines 118 - 144, Wrap the async callbacks in
initBannerDismissal with runInLoggerContext to preserve logger context/runId:
specifically wrap the browserContext.on("targetcreated", ...) handler and the
page.on("framenavigated", ...) handler so their work (including calling
dismissPrivacyBanner) executes inside runInLoggerContext. Replace the
unconditional await sleep(2000) on main-frame navigations with a targeted, short
wait for known banner selectors (e.g., waitForSelector with a small timeout) and
only fall back to a much shorter delay if necessary, then call
dismissPrivacyBanner(page); ensure errors are still caught and logged via logger
inside the runInLoggerContext wrapper.
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
MoneymanDashStoragebackend (new storage provider with tests)SaveContextcarrying per-account scrape results, passed through to allsaveTransactionscallsrunContextStorewith a uniquerunIdper run for async tracinggoogle-auth-library,jest, and@types/nodeto latest minor versionsTest plan
npm run buildpasses with no TypeScript errorsnpm testpasses (including newmoneyman.test.ts)SaveContextis populated correctly in storage backends🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores