RE1-T88 Firebase cleanup and updating Novu values sent #262
Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
WalkthroughThis PR removes the Firebase provider module entirely from the solution, including project files, implementations, and all dependency injections across multiple projects. Additionally, it enhances notification sound handling in the push service layer and updates the changerawr-sync workflow to extract and validate release notes with improved logging and error resilience. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
324-353: Restore .caf filenames for iOS custom sounds
GetSoundFileNameFromTypenow always returns a.wavfilename.CreateAppleNotification(Line 459) forwards that value straight into the APNS payload, so every iOS push will reference a.wavwe don't ship—our iOS builds only include the.cafvariants (see Providers/Resgrid.Providers.Bus/NotificationProvider.cs Lines 370-423 for the existing mapping). iOS will silently fall back to the default alert, breaking all custom tones for calls/messages. Please keep platform-specific extensions (e.g.,.caffor Apple) or compute a dedicated Apple sound string before constructing the APNS payload. One way to fix it is to restore the platform-aware mapping:- private string GetSoundFileNameFromType(string type) + private string GetSoundFileNameFromType(string type, Platforms platform) { if (type == ((int)PushSoundTypes.CallEmergency).ToString()) { - return "callemergency.wav"; + return platform == Platforms.iOS ? "callemergency.caf" : "callemergency.wav"; } else if (type == ((int)PushSoundTypes.CallHigh).ToString()) { - return "callhigh.wav"; + return platform == Platforms.iOS ? "callhigh.caf" : "callhigh.wav"; } …Make sure the APNS path calls
GetSoundFileNameFromType(type, Platforms.iOS)while the Android/default paths usePlatforms.Android.
🧹 Nitpick comments (2)
.github/workflows/changerawr-sync.yml (2)
71-71: Minor: Simplify redundant empty string check.The condition
[ -z "$NOTES" ] || [ "$NOTES" = "" ]is redundant—-zalready tests for empty strings.- if [ -z "$NOTES" ] || [ "$NOTES" = "" ]; then + if [ -z "$NOTES" ]; then
90-171: Excellent error resilience and security posture in API submission.The updated step validates credentials before proceeding, handles both JSON and non-JSON responses robustly, logs extensively for observability, and gracefully tolerates API or comment posting failures without failing the workflow. This prevents broken API integrations from blocking releases.
Consider adding a metric or alert in your observability system to track when Changerawr submissions fail silently, since the workflow won't fail. This ensures issues with the changelog system don't go unnoticed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/changerawr-sync.yml(3 hunks)Core/Resgrid.Services/PushService.cs(3 hunks)Core/Resgrid.Services/Resgrid.Services.csproj(0 hunks)Providers/Resgrid.Providers.Email/Resgrid.Providers.Email.csproj(0 hunks)Providers/Resgrid.Providers.Firebase/FirebaseAuthProvider.cs(0 hunks)Providers/Resgrid.Providers.Firebase/FirebaseProviderModule.cs(0 hunks)Providers/Resgrid.Providers.Firebase/Resgrid.Providers.Firebase.csproj(0 hunks)Providers/Resgrid.Providers.Firebase/app.config(0 hunks)Providers/Resgrid.Providers.Firebase/packages.config(0 hunks)Providers/Resgrid.Providers.Messaging/NovuProvider.cs(7 hunks)Resgrid.sln(1 hunks)Tests/Resgrid.Intergration.Tests/Resgrid.Intergration.Tests.csproj(0 hunks)Tests/Resgrid.Tests/Bootstrapper.cs(0 hunks)Tests/Resgrid.Tests/Resgrid.Tests.csproj(0 hunks)Tools/Resgrid.Console/Program.cs(0 hunks)Tools/Resgrid.Console/Resgrid.Console.csproj(0 hunks)Web/Resgrid.Web.Eventing/Resgrid.Web.Eventing.csproj(0 hunks)Web/Resgrid.Web.Eventing/Startup.cs(0 hunks)Web/Resgrid.Web.Services/Resgrid.Web.Services.csproj(0 hunks)Web/Resgrid.Web.Services/Startup.cs(0 hunks)Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs(0 hunks)Web/Resgrid.Web/Areas/User/Controllers/TypesController.cs(0 hunks)Web/Resgrid.Web/Resgrid.Web.csproj(0 hunks)Web/Resgrid.Web/Startup.cs(0 hunks)Web/Resgrid.Web/WebBootstrapper.cs(0 hunks)Workers/Resgrid.Workers.Console/Resgrid.Workers.Console.csproj(0 hunks)Workers/Resgrid.Workers.Framework/Bootstrapper.cs(0 hunks)Workers/Resgrid.Workers.Framework/Resgrid.Workers.Framework.csproj(0 hunks)
💤 Files with no reviewable changes (24)
- Tests/Resgrid.Tests/Bootstrapper.cs
- Workers/Resgrid.Workers.Framework/Resgrid.Workers.Framework.csproj
- Tools/Resgrid.Console/Resgrid.Console.csproj
- Providers/Resgrid.Providers.Email/Resgrid.Providers.Email.csproj
- Providers/Resgrid.Providers.Firebase/packages.config
- Web/Resgrid.Web.Services/Startup.cs
- Web/Resgrid.Web.Services/Resgrid.Web.Services.csproj
- Web/Resgrid.Web.Eventing/Resgrid.Web.Eventing.csproj
- Providers/Resgrid.Providers.Firebase/FirebaseAuthProvider.cs
- Core/Resgrid.Services/Resgrid.Services.csproj
- Web/Resgrid.Web/WebBootstrapper.cs
- Web/Resgrid.Web.Eventing/Startup.cs
- Web/Resgrid.Web/Startup.cs
- Web/Resgrid.Web/Areas/User/Controllers/TypesController.cs
- Workers/Resgrid.Workers.Framework/Bootstrapper.cs
- Tests/Resgrid.Intergration.Tests/Resgrid.Intergration.Tests.csproj
- Tests/Resgrid.Tests/Resgrid.Tests.csproj
- Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs
- Tools/Resgrid.Console/Program.cs
- Workers/Resgrid.Workers.Console/Resgrid.Workers.Console.csproj
- Providers/Resgrid.Providers.Firebase/Resgrid.Providers.Firebase.csproj
- Web/Resgrid.Web/Resgrid.Web.csproj
- Providers/Resgrid.Providers.Firebase/FirebaseProviderModule.cs
- Providers/Resgrid.Providers.Firebase/app.config
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Core/Resgrid.Services/PushService.csProviders/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code graph analysis (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (2)
Providers/Resgrid.Providers.Bus/NotificationProvider.cs (2)
GetSoundFileNameFromType(371-424)FormatForAndroidNativePush(426-432)Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs (2)
GetSoundFileNameFromType(400-460)FormatForAndroidNativePush(462-468)
🪛 actionlint (1.7.8)
.github/workflows/changerawr-sync.yml
30-30: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
.github/workflows/changerawr-sync.yml (3)
5-5: Appropriate permissions and checkout configuration.The addition of
pull-requests: readpermission is correct for the workflow's needs, and the checkout step withfetch-depth: 10properly enables access to recent commit history for the fallback logic.Also applies to: 21-24
26-88: Robust release notes extraction with good fallback logic.The extraction logic correctly strips CodeRabbit-generated sections, attempts to isolate a "Release Notes" section, and gracefully falls back to the PR title plus recent commits when necessary. The multiline output handling via EOF delimiter is properly implemented.
73-73: Mitigate potential script injection from untrusted PR metadata.Per static analysis, the template expression
${{ github.event.pull_request.title }}is used directly in the shell script. Although this is common in GitHub Actions, PR titles containing special characters (e.g., backticks,$(...)) could potentially be executed. Pass the value through an environment variable for defense in depth.- - name: Extract and prepare release notes + - name: Extract and prepare release notes id: prepare_notes env: GH_TOKEN: ${{ github.token }} + PR_TITLE: ${{ github.event.pull_request.title }} run: | set -eo pipefail ... - NOTES="## ${{ github.event.pull_request.title }}" + NOTES="## $PR_TITLE"
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores