Skip to content

refactor(core): BreadcrumbStore added#175

Open
Reversean wants to merge 2 commits intomasterfrom
refactor/breadcrumb-store
Open

refactor(core): BreadcrumbStore added#175
Reversean wants to merge 2 commits intomasterfrom
refactor/breadcrumb-store

Conversation

@Reversean
Copy link
Copy Markdown
Member

@Reversean Reversean commented Mar 25, 2026

Motivation

Part of the broader effort to extract environment-agnostic logic into @hawk.so/core (#151). BreadcrumbManager was a concrete, browser-coupled class living in @hawk.so/javascriptCatcher depended directly on it, making breadcrumb support impossible to share or override in a non-browser runtime.

What changed and why

BreadcrumbStore is an interface extracted into @hawk.so/core. Catcher now depends on that interface rather than the browser implementation. BrowserBreadcrumbStore (the renamed BreadcrumbManager) is the browser-specific implementation that stays in @hawk.so/javascript.

Core defines the contract, concrete catchers wire in the environment-appropriate implementation. A future NodeCatcher can provide its own BreadcrumbStore without inheriting any browser code.

Changes

  • core: BreadcrumbStore interface added and exported
  • javascript: BreadcrumbManager renamed to BrowserBreadcrumbStore, now implements BreadcrumbStore; Catcher updated to depend on the interface

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors breadcrumb handling by introducing a shared BreadcrumbStore contract in @hawk.so/core and updating the JavaScript SDK to use a browser-specific implementation (BrowserBreadcrumbStore) instead of the previous BreadcrumbManager.

Changes:

  • Added BreadcrumbStore (plus related types) to @hawk.so/core and re-exported them from the core entrypoint.
  • Refactored the JavaScript SDK breadcrumbs implementation to BrowserBreadcrumbStore and updated Catcher to use the new store API (add/get/clear).
  • Updated JavaScript tests and types to align with the new names and API surface (and removed the local breadcrumbs-api.ts type).

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
packages/javascript/tests/catcher.user.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/catcher.transport.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/catcher.test.ts Updates imports/reset and helper ordering.
packages/javascript/tests/catcher.global-handlers.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/catcher.context.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/catcher.breadcrumbs.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/catcher.addons.test.ts Updates singleton reset/import to BrowserBreadcrumbStore.
packages/javascript/tests/breadcrumbs.test.ts Renames the suite and updates API calls to add/get/clear.
packages/javascript/src/types/index.ts Re-exports breadcrumb store types from @hawk.so/core.
packages/javascript/src/types/breadcrumbs-api.ts Removes the local BreadcrumbsAPI definition in favor of core types.
packages/javascript/src/catcher.ts Switches from BreadcrumbManager to BrowserBreadcrumbStore and updates the public breadcrumbs getter typing.
packages/javascript/src/addons/breadcrumbs.ts Introduces BrowserBreadcrumbStore implementing the core BreadcrumbStore contract and renames browser hint type.
packages/core/src/index.ts Re-exports breadcrumb store types from the core package entrypoint.
packages/core/src/breadcrumbs/breadcrumb-store.ts Adds the new shared breadcrumb store contract and associated types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/javascript/src/addons/breadcrumbs.ts
Comment thread packages/javascript/src/catcher.ts Outdated
Comment thread packages/javascript/tests/breadcrumbs.test.ts
Comment thread packages/javascript/tests/catcher.user.test.ts Outdated
Comment thread packages/javascript/tests/catcher.transport.test.ts Outdated
Comment thread packages/javascript/tests/catcher.test.ts Outdated
Comment thread packages/javascript/tests/catcher.global-handlers.test.ts Outdated
Comment thread packages/javascript/tests/catcher.context.test.ts Outdated
Comment thread packages/javascript/tests/catcher.breadcrumbs.test.ts Outdated
Comment thread packages/javascript/tests/catcher.addons.test.ts Outdated
@neSpecc neSpecc requested a review from Dobrunia April 1, 2026 17:54
@Dobrunia
Copy link
Copy Markdown
Member

Dobrunia commented Apr 1, 2026

don't forget to bump version

@Reversean
Copy link
Copy Markdown
Member Author

don't forget to bump version

Thx for reminder.

@Reversean Reversean force-pushed the refactor/breadcrumb-store branch from 9890ddb to afc73fa Compare April 4, 2026 09:13
public init(options: BreadcrumbsOptions = {}): void {
if (this.isInitialized) {
log('[BreadcrumbManager] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn');
log('[BrowserBreadcrumbStore] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn');
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.

I don't think users should see such a log. It's better to remove it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Log shortened.

public init(options: BreadcrumbsOptions = {}): void {
if (this.isInitialized) {
log('[BreadcrumbManager] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn');
log('Breadcrumbs store already initialized', 'warn');
Copy link
Copy Markdown
Member

@neSpecc neSpecc Apr 22, 2026

Choose a reason for hiding this comment

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

Please, remove this log. Users should not see our debug logs.

If init() is somehow called twice, then we should either:

  • throw exception (if it is not normal case)
  • hand it silently (if it is ok)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Log removed.

I guess it's not critical to silently return on second init.

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.

Please, explain how init can be called twice? Are you sure it is a regular case, not an exception?

Comment thread packages/javascript/src/catcher.ts Outdated
Comment on lines +209 to +211
this.breadcrumbStore = BrowserBreadcrumbStore.getInstance();
this.breadcrumbStore.init(settings.breadcrumbs ?? {});
this.messageProcessors.push(new BreadcrumbsMessageProcessor());
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.

can we initialize BrowserBreadcrumbStore inside the BreadcrumbsMessageProcessor to incapsulate breadcrumbs logic in there and do not pass breadcrumbs to all processors? and get rid of ErrorSnapshot term

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now we call init inside of BrowserBreadcrumbsMessageProcessor.

However we still need BreadcrumbStore as Catcher field since we have Catcher.breadcrumbs(), so breadcrumbs logic is not fully contained inside message-processor.

@Reversean Reversean force-pushed the refactor/breadcrumb-store branch from acafb78 to 31d54df Compare April 28, 2026 20:52
@Reversean Reversean requested a review from neSpecc April 29, 2026 07:14
@Reversean Reversean force-pushed the refactor/breadcrumb-store branch 2 times, most recently from b8c9b5c to c049bd4 Compare April 29, 2026 07:43
…ted in catcher event processing pipeline

- MessageProcessor interface and MessageHint type
- BrowserMessageProcessor, BreadcrumbsMessageProcessor, ConsoleCatcherMessageProcessor, DebugMessageProcessor
- replaced inline addon logic with sequential MessageProcessor pipeline
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.

I dont like the idea to create a "messages" folder just for a single file. What is "messages" in case of core architecture?

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.

Do we need to create a /breadcrumbs/ directory? It is supposed to see only architecture-like terms on the first level.

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.

still do not understand what /messages/ are stand for

* Used to prepare event message before send.
* May modify original event payload or return null to drop it.
*/
messageProcessors?: MessageProcessor<'errors/javascript'>[];
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.

should be documented somewhere, maybe at readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants