feat: Add support for notification push type widgets#194
feat: Add support for notification push type widgets#194dwrss wants to merge 3 commits intoparse-community:masterfrom
widgets#194Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds APS support for a "content-changed" flag: TypeScript typings extended, runtime APS setter added, a generated chainable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @index.d.ts:
- Line 270: The union type NotificationPushType includes an invalid value
'widgets'; remove 'widgets' from the NotificationPushType declaration so the
type union matches Apple APNs valid push types (alert, background, location,
voip, complication, fileprovider, mdm, liveactivity, pushtotalk). Also audit the
code that sets the apns-push-type header (the logic around pushType usage in
lib/notification/index.js) and, if needed, add validation to reject or map
unsupported values before sending to APNs so only documented types are allowed.
dwrss
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #194 +/- ##
==========================================
+ Coverage 95.86% 95.87% +0.01%
==========================================
Files 23 23
Lines 846 849 +3
==========================================
+ Hits 811 814 +3
Misses 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mtrezza
changed the title
widgets
dwrss
force-pushed
the
add-widgets-to-NotificationPushType
branch
from
d102112 to
d059556
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.d.ts`:
- Around line 370-373: The declaration for contentChanged currently uses boolean
but the runtime setter (see lib/notification/apsProperties.js) treats allowed
set values as true | 1 and clears the field on undefined/other falsy values;
update the index.d.ts declaration for the public property contentChanged to
reflect the runtime contract (allow true and numeric 1 as valid set values and
undefined to clear) so TypeScript callers can express setting and clearing
correctly (locate the contentChanged property in the type/class declaration and
change its type accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7909c75-4ee3-4280-b713-64be07eabf0a
📒 Files selected for processing (4)
index.d.tslib/notification/apsProperties.jslib/notification/index.jstest/notification/apsProperties.js
✅ Files skipped from review due to trivial changes (2)
- lib/notification/apsProperties.js
- test/notification/apsProperties.js
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/notification/index.js
| /** | ||
| * Setting this to true will specify "content-changed" in the payload when it is compiled | ||
| */ | ||
| public contentChanged: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration/runtime/test contract for contentChanged
rg -n "contentChanged|content-changed" index.d.ts lib/notification/apsProperties.js test/notification/apsProperties.js -C3Repository: parse-community/node-apn
Length of output: 3811
Widen contentChanged type to match runtime setter behavior.
Line 373 declares contentChanged as boolean, but the runtime setter (lib/notification/apsProperties.js:148–154) accepts true | 1 and clears on undefined and other falsy values. TS users cannot currently express the full contract.
Proposed typing fix
- public contentChanged: boolean;
+ public contentChanged: boolean | 1 | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Setting this to true will specify "content-changed" in the payload when it is compiled | |
| */ | |
| public contentChanged: boolean; | |
| /** | |
| * Setting this to true will specify "content-changed" in the payload when it is compiled | |
| */ | |
| public contentChanged: boolean | 1 | undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.d.ts` around lines 370 - 373, The declaration for contentChanged
currently uses boolean but the runtime setter (see
lib/notification/apsProperties.js) treats allowed set values as true | 1 and
clears the field on undefined/other falsy values; update the index.d.ts
declaration for the public property contentChanged to reflect the runtime
contract (allow true and numeric 1 as valid set values and undefined to clear)
so TypeScript callers can express setting and clearing correctly (locate the
contentChanged property in the type/class declaration and change its type
accordingly).
There was a problem hiding this comment.
That wouldn't be an unreasonable type declaration, but it doesn't match how other properties (such as mutableContent are declared
There was a problem hiding this comment.
@dwrss, that's a fair point. Keeping contentChanged: boolean is consistent with how mutableContent (and similar properties) are declared, even though the runtime setter accepts 1 as well. The simplified boolean type is a reasonable abstraction for TypeScript callers. I'll withdraw this suggestion.
(^_^) 🐇
✏️ Learnings added
Learnt from: dwrss
Repo: parse-community/node-apn PR: 194
File: index.d.ts:370-373
Timestamp: 2026-04-04T01:53:22.032Z
Learning: In parse-community/node-apn (`index.d.ts`), APS convenience properties like `mutableContent`, `contentAvailable`, and `contentChanged` are intentionally typed as `boolean` even though their runtime setters in `lib/notification/apsProperties.js` also accept `1` (numeric). The simplified `boolean` type is the established pattern and should not be widened to `boolean | 1 | undefined` for consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Required for sending Widget update notifications
dwrss
force-pushed
the
add-widgets-to-NotificationPushType
branch
from
d059556 to
29c6a90
Compare
3 participants
As per Apple's documentation "widgets" is now a valid push type: https://developer.apple.com/documentation/widgetkit/updating-widgets-with-widgetkit-push-notifications
Summary by CodeRabbit
New Features
Tests