change(firestore): close Firestore instance more gracefully when "Clear Site Data" button pressed in browser

[dconeybe]

Real PR: firebase#9118

[gemini-code-assist]

Summary of Changes

Hello @dconeybe, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness and user experience when the Firestore SDK's IndexedDB database is unexpectedly deleted, such as by a user clicking 'Clear Site Data' in their browser or another tab clearing persistence. It introduces a more granular event system for database deletion notifications, enabling the SDK to provide more informative error messages and ensure a more graceful termination process.

Highlights

• Improved IndexedDB Deletion Handling: Introduced specific event types (ClearSiteDataDatabaseDeletedEvent, VersionChangeDatabaseDeletedEvent) to differentiate between various IndexedDB database deletion scenarios, providing more context to the DatabaseDeletedListener.
• Enhanced Firestore Termination: Modified the setDatabaseDeletedListener to receive detailed event information, allowing for more specific logging and error handling during Firestore termination.
• Specific Error for 'Clear Site Data': When a 'Clear Site Data' event is detected, a FirestoreError of type failed-precondition is now thrown. This error provides a clear message to the user about potential IndexedDB corruption and suggests a page reload, and is thrown after attempting to terminate Firestore.
• Robust IndexedDB Open Logic: Refactored SimpleDb.open() to defer handling of 'Clear Site Data' detection from the onupgradeneeded event to the onsuccess event. This change prevents generic errors and allows for more precise and actionable error reporting to the databaseDeletedListener.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request addresses the issue of gracefully terminating Firestore when the "Clear Site Data" button is pressed in a web browser. It introduces new event types for database deletion listeners and modifies the Firestore client to handle these events, throwing a FirestoreError to prevent retries and logging relevant information. The changes also include modifications to the IndexedDB implementation to detect and handle potential database corruption due to the "Clear Site Data" action.

[Copilot]

Pull Request Overview

This patch improves handling of IndexedDB “clear site data” and version-change deletions by queuing specific deletion events and terminating Firestore with more descriptive errors.

• Introduce ClearSiteDataDatabaseDeletedEvent and VersionChangeDatabaseDeletedEvent to distinguish deletion causes.
• Update SimpleDb to detect “clear site data” in onupgradeneeded, queue the event, and dispatch it in onsuccess instead of throwing directly.
• Enhance the Firestore client to log context and throw a FirestoreError with guidance when these events occur.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
packages/firestore/src/local/simple_db.ts	Added event queue for Clear Site Data, removed direct throw, and dispatch queued events on success.
packages/firestore/src/local/persistence.ts	Defined two new event classes and updated DatabaseDeletedListener to accept event objects.
packages/firestore/src/core/firestore_client.ts	Enhanced listener to branch on event type, log details, and throw FirestoreError for clear-data.
.changeset/hot-birds-report.md	Added changelog entry noting graceful termination on “Clear Site Data.”

Comments suppressed due to low confidence (3)

packages/firestore/src/local/simple_db.ts:314

• [nitpick] This variable is an array but named in singular. Rename it to clearSiteDataEvents and iterate through all entries instead of only handling the first.

        const clearSiteDataEvent: ClearSiteDataDatabaseDeletedEvent[] = [];

packages/firestore/src/local/persistence.ts:108

• [nitpick] The VersionChangeDatabaseDeletedEvent only captures eventNewVersion. Consider also including oldVersion for consistency with ClearSiteDataDatabaseDeletedEvent and richer diagnostic context.

export class VersionChangeDatabaseDeletedEvent {

packages/firestore/src/local/simple_db.ts:23

• The logError import is not used in this file; consider removing it to avoid unused imports.

import { logDebug, logError } from '../util/log';

[dconeybe]

@gemini-code-assist give me 5 suggestions on ways to improve the changes in this PR to be more idiomatic

[gemini-code-assist]

Hi @dconeybe,

Thanks for reaching out! I've reviewed the changes in your pull request, and here are 5 suggestions to make the code more idiomatic, focusing on common TypeScript/JavaScript patterns and best practices:

1. Review synchronous error throwing in asynchronous event handlers (packages/firestore/src/core/firestore_client.ts):
   In firestore_client.ts, the setDatabaseDeletedListener callback now synchronously throws a FirestoreError for ClearSiteDataDatabaseDeletedEvent after initiating an asynchronous client.terminate() call. Synchronous throws from event listeners can lead to unhandled exceptions and unexpected behavior, especially when the listener itself is part of an asynchronous flow (even if terminate() is fire-and-forget from the listener's perspective). If the intent is to immediately halt execution and signal a critical error, ensure the mechanism calling setDatabaseDeletedListener is prepared to catch synchronous exceptions from it. A common pattern for event handlers that encounter critical errors is to log the error and then potentially trigger a more controlled shutdown or error reporting mechanism.

2. Clarify asynchronous flow and error handling in packages/firestore/src/core/firestore_client.ts:
   Following up on the previous point, the client.terminate() call is an asynchronous operation, but the if (error) { throw error; } block executes synchronously. This means the FirestoreError is thrown before the terminate() promise has necessarily resolved or rejected. If the intention is for the FirestoreError to be the ultimate outcome of the terminate() operation (e.g., if terminate() itself should fail with this specific error), the error should be part of the terminate() promise chain. Consider declaring the databaseDeletedListener as an async function and awaiting client.terminate() if the error should only be thrown after the termination attempt.

3. Consider flattening event data properties (packages/firestore/src/local/persistence.ts):
   The new event classes (VersionChangeDatabaseDeletedEvent, ClearSiteDataDatabaseDeletedEvent) encapsulate their details within a data property (e.g., event.data.eventId). While valid, for properties that are universally present and frequently accessed, it can sometimes be more idiomatic and ergonomic to expose them directly on the event object (e.g., event.eventId). This is a minor stylistic preference but can improve code readability and reduce verbosity when accessing event details.

4. Refine logging levels for lifecycle events (packages/firestore/src/core/firestore_client.ts, packages/firestore/src/local/simple_db.ts):
   The PR introduces more detailed logging with event.type and event.data. Ensure that the chosen logging levels (logWarn, logDebug) accurately reflect the severity and importance of the events. For instance, a successful termination following a ClearSiteDataDatabaseDeletedEvent is an important lifecycle event, and logDebug might be too low a level if this information is crucial for understanding application behavior in production. Consider if logInfo might be more appropriate for successful, but significant, lifecycle events like terminate() completion after a database deletion event, to ensure these events are visible without being overly verbose.

5. Explore a common base interface/class for DatabaseDeletedListenerEvent types (packages/firestore/src/local/persistence.ts):
   The DatabaseDeletedListenerEvent is a union type of VersionChangeDatabaseDeletedEvent and ClearSiteDataDatabaseDeletedEvent. Both classes share the type discriminator and eventId (within their data property). If future event types are anticipated or if shared logic for these events becomes more complex, defining a common base interface or abstract class could improve code organization and reusability. This interface could define common properties like type and eventId, allowing for more generic handling of DatabaseDeletedListenerEvent instances where only these common properties are needed.

Let me know if you'd like to discuss any of these in more detail!