Skip to content

Conversation

@TomTasche
Copy link
Member

@TomTasche TomTasche commented Dec 13, 2025

Fixes #446

@claude
Copy link

claude bot commented Dec 13, 2025

Claude finished @TomTasche's task —— View job


Code Review for PR #448: Get rid of Firebase dependencies

I've completed my review of this PR that removes Firebase dependencies from the Android app. Here's my comprehensive feedback:

Review Todo List:

  • Analyze Firebase dependency removal approach
  • Review changes to OnlineLoader (document fetching)
  • Review analytics implementation changes
  • Review crash reporting implementation changes
  • Review configuration management changes
  • Assess new infrastructure setup (transfer.sh deployment)
  • Check for security implications
  • Verify no Firebase references remain

Summary

This PR successfully removes Firebase dependencies including Analytics, Crashlytics, Remote Config, and Cloud Storage. The implementation follows a reasonable approach by replacing Firebase services with simpler alternatives, though there are some areas that need attention.

Positive Changes ✅

  1. Clean Firebase removal: All Firebase dependencies, configuration files (google-services.json), and build plugins have been properly removed.

  2. Infrastructure replacement: The new self-hosted transfer.sh deployment using Terraform on Scaleway is a good privacy-focused alternative to Firebase Storage.

  3. Simplified implementations: The stub implementations for analytics, crash reporting, and config management properly maintain the interfaces while removing Firebase dependencies.

Issues and Recommendations ⚠️

  1. Analytics logging tag inconsistency (app/src/main/java/at/tomtasche/reader/nonfree/AnalyticsManager.java:37):

    • The code review suggests using Log.i("ODR", event) but the current code uses Log.i("smn", event)
    • Recommend accepting the reviewer's suggestion for consistency with other logging
  2. OnlineLoader reversion requested (app/src/main/java/at/tomtasche/reader/background/OnlineLoader.java):

    • The reviewer requested reverting changes to this file, but no specific reason was provided
    • The changes appear to replace Firebase Storage URLs with a self-hosted transfer.sh instance
    • Consider clarifying with the reviewer why this needs reverting
  3. Missing functionality:

    • Analytics: Now only logs to console - no actual analytics collection
    • Crash reporting: Only logs to console - no crash reporting service
    • Remote config: Always returns default values - no dynamic configuration
    • Consider documenting these limitations or implementing alternative services if needed
  4. Security considerations:

    • The new transfer.sh deployment exposes a public upload endpoint
    • Current configuration has reasonable limits (50MB max upload, 1-day retention)
    • Consider adding rate limiting or authentication if abuse becomes an issue
  5. Infrastructure concerns:

    • Single instance deployment (min_scale = 1, max_scale = 1) may have availability issues
    • No backup or redundancy for uploaded files
    • Consider scaling configuration for production use

Test Coverage Gaps

  • No tests added for the new stub implementations
  • No tests for the transfer.sh upload functionality
  • Integration tests should verify the app works without Firebase

Performance Considerations

  • The self-hosted transfer.sh may have different performance characteristics than Firebase Storage
  • Network latency could be higher depending on server location
  • Monitor upload success rates and performance after deployment

Recommendations

  1. Accept the reviewer's suggestion to change the analytics log tag to "ODR"
  2. Clarify with the reviewer about the OnlineLoader reversion request
  3. Add basic unit tests for the stub implementations
  4. Document the limitations of removed Firebase features in the README
  5. Consider adding monitoring for the transfer.sh deployment
  6. Plan for scaling the infrastructure if usage grows

Overall, this is a solid implementation that successfully removes Firebase dependencies while maintaining app functionality. The main concerns are around the requested changes from the reviewer and ensuring adequate monitoring and scaling for the new infrastructure.

@TomTasche TomTasche marked this pull request as ready for review December 14, 2025 20:57
@TomTasche TomTasche merged commit 03368f3 into main Dec 14, 2025
1 of 2 checks passed
@TomTasche TomTasche deleted the issue-446 branch December 14, 2025 20:57
@TomTasche TomTasche mentioned this pull request Dec 14, 2025
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.

Remove Firebase dependencies

2 participants