feat: allow to reuse a browser by passing a browserContext

[daniel-hauser]

This pull request changes the page creation logic by alowing a browserContext to be passes instead of a browser (that is currently being closed when the scraping ends)

Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.

[baruchiro]

Do you have to split the companyServiceOptions from options?

[daniel-hauser]

These fields are not ScraperOptions but amex/isracard specific values that are internal to this file - defined inside fetchData and merged with the scraper options.

[baruchiro]

I need to review it, but I don't have a time for it

[baruchiro]

@daniel-hauser sorry, I really want to review this, but I found myself playing מצא את ההבדלים from my phone when I start to review it.

Usually, this PR is OK, but since we are trying to work on it on our non-existent free time, I need you to explain to me what you did so I will understand what I see.

[daniel-hauser]

@baruchiro, I aggrege that reviewing large changes in GH mobile is hard.
Attached here is the o1 answer for the changes, LMK if it makes the review easier.

---

AI Generated:

Longe explanation:

Changes Introduced:

• Added Support for External BrowserContext:
  • New Logic Added:
    • In the initializePage() method, we now first check if a browserContext is provided in this.options.
    • If a browserContext is provided:
      • We create a new page using browserContext.newPage().
      • This allows users to supply their own BrowserContext, offering more flexibility when the browser context is managed externally.

---

Logic Unchanged:

• Existing Browser Initialization Logic Remains the Same:

  • If no browserContext is provided:
    • We proceed to check if a browser is provided in this.options (same as before).
      • If a browser is provided:
        • We log a debug message: 'Using the browser instance provided in options'.
        • We create a new page using browser.newPage().
        • For backward compatibility, if skipCloseBrowser is not set to true, we add a cleanup function to close the browser after scraping.
      • If no browser is provided:
        • We launch a new browser instance as before, using options like executablePath, args, showBrowser, and timeout from this.options.
        • We add a cleanup function to close the browser after scraping.
        • If prepareBrowser is provided, we call it with the new browser instance.

• Resource Cleanup Logic Remains the Same:

  • We maintain a cleanups array to manage cleanup functions.
  • During termination (terminate() method), we execute all cleanup functions in reverse order to ensure proper resource disposal.
  • This includes closing pages and browsers as needed.

• Page Creation and Return:

  • After handling the browser or browser context, we create a new page and return it.
  • This part of the logic is unchanged.

Summary:

• What's New:

  • Added support for an external BrowserContext in initializePage(), allowing more flexibility in browser management.

• What's Unchanged:

  • The existing logic for:
    • Handling an external Browser instance.
    • Launching a new browser if none is provided.
    • Preparing the browser with prepareBrowser if provided.
    • Managing resource cleanup with the cleanups array.
    • Creating and returning a new page for the scraper to use.
  • All existing behavior remains intact, ensuring backward compatibility.

[Copilot]

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/scrapers/base-scraper-with-browser.ts:85

• [nitpick] The word 'bang' might be confusing. It would be clearer to use 'exclamation mark (!)' instead.

NOTICE - it is discouraged to use bang (!) in general.

src/scrapers/base-isracard-amex.ts:33

• The ExtendedScraperOptions interface was removed, but there was no mention of its removal in the context provided. Ensure that this interface is not used elsewhere in the codebase.

type CompanyServiceOptions = {

src/scrapers/base-isracard-amex.ts:258

• [nitpick] The parameter name 'options' is of type 'CompanyServiceOptions'. It would be clearer to rename it to 'companyServiceOptions'.

async function getExtraScrapTransaction(page: Page, options:CompanyServiceOptions, month: Moment, accountIndex: number, transaction: Transaction): Promise<Transaction> {

[Copilot]

The word 'didn't' should be 'did not' for consistency with formal writing.

Suggested change

	* For backward compatibility, we will close the browser even if we didn't create it
	* For backward compatibility, we will close the browser even if we did not create it