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 project and all related code/DI registrations across the solution, adds per-notification sound propagation in push/Novu plumbing, and hardens the .github/changerawr-sync.yml workflow to extract release notes and tolerate API/comment failures. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Checkout as actions/checkout@v4
participant Extract as "Extract & Prepare Release Notes"
participant Changerawr as "Post to Changerawr API"
participant PR as GitHub Pull Request
Note over GH,Checkout: Workflow start
GH->>Checkout: fetch repo (fetch-depth:10)
Checkout-->>GH: repository available
GH->>Extract: read PR body/title/commits
Extract-->>GH: RELEASE_NOTES output (cleaned or fallback)
GH->>Changerawr: invoke with RELEASE_NOTES env
Changerawr->>Changerawr: validate CHANGERAWR_API_KEY & PROJECT_ID
alt credentials missing
Changerawr-->>GH: skip submission (log)
else credentials present
Changerawr->>PR: attempt to post comment (non-fatal)
Changerawr->>Changerawr: POST payload (title, release_notes, commit_sha, metadata)
Changerawr-->>GH: parse response (robust JSON handling)
alt API indicates failure or non-JSON
Changerawr-->>GH: warn and continue (do not fail)
else success
Changerawr-->>GH: log success
end
end
sequenceDiagram
participant Caller as Application code
participant PushSvc as PushService
participant Novu as NovuProvider
participant Platform as APNS/FCM
Caller->>PushSvc: Send message/notification (type)
PushSvc->>Novu: GetSoundFileNameFromType(type)
Novu-->>PushSvc: sound string
PushSvc->>Novu: SendNotification(..., sound)
Novu->>Platform: Build payload including sound in android/apns sections
Platform-->>Novu: delivery result
Novu-->>PushSvc: response
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"
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs(6 hunks)
🧰 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:
Providers/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code graph analysis (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (3)
Core/Resgrid.Config/ChatConfig.cs (1)
ChatConfig(3-21)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)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
| { | ||
| ["badge"] = count, | ||
| ["sound"] = new | ||
| { | ||
| badge = count, | ||
| sound = new | ||
| { | ||
| name = GetSoundFileNameFromType(Platforms.iOS, type), | ||
| critical = channelName == "calls" ? 1 : 0, | ||
| volume = 1.0f | ||
| }, | ||
| type = type, | ||
| category = channelName, | ||
| eventCode = eventCode, | ||
| name = sound, | ||
| critical = channelName == "calls" ? 1 : 0, | ||
| volume = 1.0f | ||
| }, | ||
|
|
||
| ["type"] = type, | ||
| ["category"] = channelName, | ||
| ["eventCode"] = eventCode, | ||
| ["gcm.message_id"] = "123" | ||
| }, |
There was a problem hiding this comment.
Restore iOS .caf sound mappings.
The APNS override now emits callemergency.wav/callhigh.wav, but the iOS app only ships the .caf variants (see Providers/Resgrid.Providers.Bus/UnitNotificationProvider.GetSoundFileNameFromType, which still returns .caf for Platforms.iOS). APNS will not find the requested file, so critical alerts fall back to the default tone. Please keep a dedicated .caf mapping for Apple while continuing to supply the .wav/.mp3 names for Novu/Android.
Apply this diff to supply the correct value:
- ["sound"] = new
- {
- name = sound,
+ ["sound"] = new
+ {
+ name = GetAppleSoundFileNameFromType(type),
critical = channelName == "calls" ? 1 : 0,
volume = 1.0fand add the helper alongside the existing sound utilities:
+ private string GetAppleSoundFileNameFromType(string type)
+ {
+ if (type == ((int)PushSoundTypes.CallEmergency).ToString())
+ return "callemergency.caf";
+ if (type == ((int)PushSoundTypes.CallHigh).ToString())
+ return "callhigh.caf";
+ if (type == ((int)PushSoundTypes.CallMedium).ToString())
+ return "callmedium.caf";
+ if (type == ((int)PushSoundTypes.CallLow).ToString())
+ return "calllow.caf";
+ if (type == ((int)PushSoundTypes.Notifiation).ToString())
+ return "notification.caf";
+ if (type == ((int)PushSoundTypes.Message).ToString())
+ return "message.caf";
+ return $"{type}.caf";
+ }Committable suggestion skipped: line range outside the PR's diff.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
Bug Fixes
Chores
Chores