Firebase Contextual Errors

[maneesht]

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

• Read the contribution guidelines (CONTRIBUTING.md).
• If this has been discussed in an issue, make sure to link to the issue here.
  If not, go file an issue about this before creating a pull request to discuss.

Testing

• Make sure all existing tests in the repository pass after your change.
• If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

• Changes that affect the public API will require internal review. Before making a
  PR that changes the public API, we would suggest first proposing your change in an
  issue so that we can discuss it together.

[changeset-bot]

⚠️ No Changeset found

Latest commit: abd258d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[gemini-code-assist]

Code Review

This pull request implements a contextual error enrichment system for the Firebase JS SDK, primarily focusing on Firestore. It allows errors to carry operational metadata such as resource paths, operation types, and authentication state to improve debuggability. Key changes include refactoring error classes to support generic custom data, capturing ID tokens at request time, and implementing observers to wrap errors with context. The PR also adds a documentation guide and updates internal scripts. Review feedback identified several critical issues, including an accidental IDE import of the TypeScript compiler API in browser code, a circular dependency introduced in the utility package, and the loss of original stack traces during error enrichment. Additionally, the reviewer noted the presence of debug logs, unresolved TODOs, and a regression in script error handling that should be resolved.

[gemini-code-assist]

This import appears to be an accidental IDE auto-import. HighlightSpanKind is part of the TypeScript compiler API and should not be imported in production browser code.

[gemini-code-assist]

Adding @firebase/auth as a dependency to @firebase/util is problematic as it likely creates a circular dependency (since auth depends on util). Additionally, util is intended to be a low-level package and should not depend on high-level SDKs. The current changes in util/src/errors.ts use internal JWT utilities and do not appear to require the full auth package.

[gemini-code-assist]

The copyWithAuthInfo method creates a new FirestoreError instance, which causes the original stack trace to be lost. To maintain debuggability, the stack trace should be preserved by copying it from the current instance to the new one.

  copyWithAuthInfo(idToken: string | null): FirestoreError {
    if (idToken === null) {
      return this;
    }
    const authInfo = parseIdTokenToAuthInfo(idToken);
    const error = new FirestoreError(this.code, this.message, {
      ...this.customData,
      authInfo
    });
    if (this.stack) {
      Object.defineProperty(error, 'stack', {
        value: this.stack,
        writable: false,
        configurable: true
      });
    }
    return error;
  }

[gemini-code-assist]

In firestoreToContextualError, creating a new FirestoreError loses the original stack trace of the input err. The stack trace should be explicitly preserved to ensure that error reports remain useful for debugging.

Suggested change

	const newFirestoreErr = new FirestoreError(
	firestoreErr.code,
	firestoreErr.message,
	customData
	);
	const newFirestoreErr = new FirestoreError(
	firestoreErr.code,
	firestoreErr.message,
	customData
	);
	if (firestoreErr.stack) {
	Object.defineProperty(newFirestoreErr, 'stack', {
	value: firestoreErr.stack,
	writable: false,
	configurable: true
	});
	}

[gemini-code-assist]

Debug logging (console.log) should be removed before merging.

[gemini-code-assist]

This section contains unresolved TODOs and an incomplete comment (// syncEngineImpl.). These should be addressed or removed to ensure the implementation is complete and maintainable.

[gemini-code-assist]

The error handling for the async main function was removed (previously handled by .catch()). This can lead to silent failures or unhandled promise rejections if the script encounters an error. Please restore the error handling logic.

[gemini-code-assist]

Contextual error enrichment is currently disabled for batch writes containing more than one mutation (mutations.length === 1 ? wrappedDeferred : deferred). This limits the effectiveness of the feature for transactions and bulk operations. As noted in the adjacent comment, this gap should be addressed to provide context even for multi-mutation batches.