Skip to content

Conversation

@rajatsukhadeve
Copy link

@rajatsukhadeve rajatsukhadeve commented Jan 2, 2026

Description

Tip

This PR fixes an issue where the Firestore plugin ignores the custom Database URL provided in the datasource configuration. Previously, it would default to the main project instance even if a specific URL was provided

Fixes #38121

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags=""

🔍 Cypress test results

Caution

If you modify the content in this section, you are likely to disrupt the CI result for your PR.

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes

    • Improved Firestore datasource initialization with safer recovery when initialization fails and added cleanup of stale app instances during datasource teardown.
  • Refactor

    • Formatting, comment and type-handling refinements across the Firestore plugin for improved readability and maintainability; no public API changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 2, 2026

Caution

Review failed

An error occurred during the review process. Please try again later.

Walkthrough

Datasource initialization and teardown for the Firestore plugin were modified: FirebaseApp is now initialized with provided options (and reinitialized on IllegalStateException after deleting an existing app), and datasourceDestroy removes the FirebaseApp instance when present. Several non-functional formatting and TypeReference adjustments were made.

Changes

Cohort / File(s) Summary
Firestore plugin core
app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
Changed datasourceCreate to call FirebaseApp.initializeApp(options, projectId) when options are available; on IllegalStateException it retrieves, deletes, then reinitializes the existing app. Added explicit cleanup in datasourceDestroy to delete the FirebaseApp for the projectId when present.
Formatting / TypeReference updates
app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java (same file — multiple methods)
Reflowed comments/whitespace and replaced several anonymous TypeReference initializations with inline anonymous classes across methods like getStructure, methodGetCollection, resultToMap, and insertFieldValues. No public method signature changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🔧 Firebase apps now find their place,
Options first, no default race.
If one persists, we delete and try—
Clean teardown, then reapply. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: prioritizing custom database URL in Firestore plugin, which directly addresses the linked issue #38121.
Description check ✅ Passed The description follows the template structure, includes the issue reference (#38121), explains the fix, and marks the DevRel/Marketing communication checkbox.
Linked Issues check ✅ Passed The PR addresses issue #38121 by implementing null-checking on options before initializing FirebaseApp, prioritizing provided configuration over default instance retrieval.
Out of Scope Changes check ✅ Passed Changes are focused on the datasourceCreate initialization flow to honor custom Database URL; minor formatting adjustments are incidental to the core fix scope.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f25927 and b34d396.

📒 Files selected for processing (1)
  • app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
🔇 Additional comments (5)
app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java (5)

147-150: LGTM!

Minor comment reformatting for readability. No behavioral change.


207-208: LGTM!

Straightforward reformatting of the ternary expression.


949-953: Core fix looks good — custom database URL is now passed to FirebaseOptions.

This addresses the PR objective of prioritizing the custom database URL. However, see the comment below regarding the fallback behavior that may undermine this fix.


383-384: LGTM!

Diamond operator usage with TypeReference<> is idiomatic and cleaner.

Also applies to: 430-432


1032-1056: LGTM!

Reformatting of getStructure method improves readability without changing behavior.

@rajatsukhadeve
Copy link
Author

Hi Appsmith Team! I've addressed the initial feedback from CodeRabbit regarding stale FirebaseApp instances. I've updated datasourceCreate to delete existing instances before re-initialization and implemented datasourceDestroy for proper cleanup. Ready for review when a maintainer is available. Thanks!

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.

1 participant