Repository navigation
feat(newsletter): complete the upstream newsletter surface - #30
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
| Filename | Overview |
|---|---|
| src/Compatibility/newsletter-results.ts | Adds explicit translation of bridge newsletter metadata fields and enum values into the upstream-compatible shape. |
| src/Socket/newsletter.ts | Expands the newsletter socket API, dispatches key-specific and picture operations, preserves legacy aliases, and translates results. |
| src/tests/newsletter-compatibility.test.ts | Adds broad unit coverage for newsletter delegation, compatibility signatures, result mapping, unsupported arguments, and error propagation. |
Sequence Diagram
sequenceDiagram
participant App
participant Compatibility as Newsletter compatibility layer
participant Bridge
App->>Compatibility: Upstream newsletter method
Compatibility->>Compatibility: Translate arguments
Compatibility->>Bridge: Delegate bridge operation
Bridge-->>Compatibility: Bridge result
Compatibility->>Compatibility: Map metadata/result shape
Compatibility-->>App: Upstream-compatible result
Reviews (4): Last reviewed commit: "fix(newsletter): keep the legacy subscri..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- In
src/Socket/newsletter.ts, validation currently treatssince=0andafter=0as absent, so unsupported cursor/epoch inputs can be silently ignored instead of failing as documented, which risks inconsistent client behavior and harder-to-debug pagination/sync bugs—check argument presence with!== undefinedso any supplied unsupported value is rejected.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Socket/newsletter.ts">
<violation number="1" location="src/Socket/newsletter.ts:102">
P2: Epoch `since=0` and cursor `after=0` are silently dropped instead of rejected; test for presence (`!== undefined`) so every supplied unsupported argument fails as documented.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| newsletterMute: async (jid: string, mute: boolean) => { | ||
| await (await ctx.getClient()).newsletterMute(jid, mute) | ||
| newsletterFetchMessages: async (jid: string, count: number, since?: number, after?: number) => { | ||
| if (since) { |
There was a problem hiding this comment.
P2: Epoch since=0 and cursor after=0 are silently dropped instead of rejected; test for presence (!== undefined) so every supplied unsupported argument fails as documented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Socket/newsletter.ts, line 102:
<comment>Epoch `since=0` and cursor `after=0` are silently dropped instead of rejected; test for presence (`!== undefined`) so every supplied unsupported argument fails as documented.</comment>
<file context>
@@ -1,31 +1,147 @@
- newsletterMute: async (jid: string, mute: boolean) => {
- await (await ctx.getClient()).newsletterMute(jid, mute)
+ newsletterFetchMessages: async (jid: string, count: number, since?: number, after?: number) => {
+ if (since) {
+ throw new Boom('newsletterFetchMessages: `since` is not supported, the message query carries no time filter', {
+ statusCode: 400
</file context>
There was a problem hiding this comment.
Keeping the truthiness check, and the code now says why rather than leaving it to be read as an oversight.
For after, upstream itself uses the same guard: newsletter.js:130 is if (after) { messageUpdateAttrs.after = after.toString() }, so upstream also treats 0 as no cursor. Rejecting it would refuse a call upstream accepts.
For since, upstream does include it whenever it is a number (if (typeof since === 'number')), so 0 reaches the wire there. But since is a unix timestamp and 0 is the epoch, which filters nothing: the unfiltered query this delegates to returns the same set. So the intent is met, and by the rule the rest of this series follows, that is a no-op rather than a rejection.
What this protects is the common (jid, count, 0, 0) call, which is what a caller writes when the signature makes both required and they want neither. A non-zero value in either still rejects, since mapping after onto the core's backward before cursor would page the wrong direction and return a plausible wrong answer. Three tests cover the two rejections and the zero case.
Generated by Claude Code
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
One fixed, one answered. The wasted metadata read is real ( The zero handling is deliberate, and I have made the comment say so. Switching to It is the same distinction that came up on #28 and that I now apply across this series: reject when the caller's intent cannot be met, do nothing when it already is.
Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a08691eba2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| newsletterFollow: async (jid: string): Promise<NewsletterMetadata> => { | ||
| return bridgeNewsletterMetadataToBaileys(await (await ctx.getClient()).newsletterSubscribe(jid)) | ||
| }, | ||
|
|
||
| newsletterUnfollow: async (jid: string): Promise<void> => { |
There was a problem hiding this comment.
Existing baileyrs clients that call the previously public newsletterSubscribe or newsletterUnsubscribe methods now receive a missing-method error because these entries were replaced by newsletterFollow/newsletterUnfollow. The same bridge operations remain available, so retain the old method names as compatibility aliases rather than silently removing them; unlike the other documented breaking signatures, this removal has no stated migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and restored. Removing them was accidental rather than a decision: the rename to newsletterFollow/newsletterUnfollow took the old names with it, and unlike the signature changes this PR does document, that one had no migration behind it.
Both are back, delegating to the same bridge operations, with the follow-up in the thread below making newsletterSubscribe return the bridge result unmapped so the shape does not shift either.
Generated by Claude Code
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Correct, and fixed in Adding upstream's Generated by Claude Code |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 217c6ec3fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| newsletterSubscribe: async (jid: string): Promise<NewsletterMetadata> => { | ||
| return bridgeNewsletterMetadataToBaileys(await (await ctx.getClient()).newsletterSubscribe(jid)) |
There was a problem hiding this comment.
Although newsletterSubscribe has now been restored, this revision newly transforms its result from the bridge's NewsletterMetadataResult into NewsletterMetadata. Existing callers of this legacy method that read fields such as result.jid, result.subscriberCount, or result.inviteCode will now receive undefined because the mapper renames them to id, subscribers, and invite; keep the raw bridge result for this compatibility alias and reserve the mapped shape for the new newsletterFollow method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed. newsletterSubscribe kept its name while quietly changing its shape, which is the worse half of a rename: the callers it was restored for read jid, subscriberCount and inviteCode, and the mapper renames all three.
It now returns the bridge result untouched, typed NewsletterMetadataResult. newsletterFollow stays the one that speaks upstream's names, so the two names mean two shapes on purpose rather than by accident.
Two tests pin it: one asserting the alias returns the bridge result verbatim, one asserting newsletterFollow answers with id/subscribers. Checked by putting the mapper back and watching the first fail.
Generated by Claude Code
Fifteen methods that did not exist, and three signatures that did not
match, all delegating to the bridge. The neutral to Baileys translation
lives in `src/Compatibility/newsletter-results.ts`.
`newsletterMetadata` is the one worth reading twice. Upstream takes
`(type, key)`; this package took `(jid)`. Code written against upstream
calls `newsletterMetadata('jid', id)`, and the old signature read that as
a newsletter whose JID is the literal string `'jid'`. It type checked on
the caller's side and failed against the server. The `'invite'` path now
works rather than being unreachable, because the core has a lookup per
key kind and the dispatch belongs in this layer.
`newsletterMute(jid, boolean)` splits into upstream's `newsletterMute` and
`newsletterUnmute`. Both go to `newsletterFollowerMute`, which is the mute
a subscriber toggles; the core's other newsletter mute is for admin
activity and is a different control, so the ambiguous alias is avoided.
Three places where the shapes genuinely do not line up, each answered
rather than papered over:
`newsletterAdminCount` rides on the admin-info result and the server omits
it. Absent is reported as absent, because `0` here reads as "no admins",
which no newsletter can be.
`newsletterFetchMessages` takes `since` and `after`; the core's query
carries no time filter and pages backward from a `before` cursor. Mapping
`after` onto `before` would page the other direction and return a
plausible wrong answer, so either argument is refused by name.
`NewsletterMetadata` has four fields the bridge result has no source for:
`owner`, `mute_state`, `reaction_codes` and `thread_metadata`. They stay
absent. `state` is the newsletter's lifecycle, not a mute, and filling
`mute_state` from it would have been the easy wrong answer.
…date
`newsletterUpdate({picture})` fetched the metadata first and then threw
the result away, because every picture write already answers with the
refreshed metadata. The read now happens only when the delta asked for
nothing at all and there is otherwise nothing to return.
Also says why a zero `since` or `after` is allowed through rather than
refused: the epoch and no-cursor are what the unfiltered query already
does, so `(jid, count, 0, 0)` is a caller asking for nothing special, not
a caller asking for something we cannot do.
Adding upstream's `newsletterFollow` and `newsletterUnfollow` replaced the two names this package already had, which is a removal nobody asked for and the only one in this PR without a migration note. Both are back, delegating to the same bridge calls.
… result newsletterSubscribe predates the upstream names this branch adds, and its callers read jid, subscriberCount and inviteCode off what it returns. Mapping it to upstream's field names renamed all three, so the alias kept its name while quietly changing its shape. It now returns the bridge result untouched, and newsletterFollow stays the one that speaks upstream's names.
217c6ec to
e5b5be2
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Summary
Fifteen methods that did not exist, plus the three signatures that did not match, all delegating to the bridge. The neutral to Baileys translation lives in
src/Compatibility/newsletter-results.ts.New:
newsletterAdminCount,newsletterChangeOwner,newsletterDelete,newsletterDemote,newsletterFetchMessages,newsletterFollow,newsletterRemovePicture,newsletterSubscribers,newsletterUnfollow,newsletterUnmute,newsletterUpdate,newsletterUpdateDescription,newsletterUpdateName,newsletterUpdatePicture,subscribeNewsletterUpdates.Breaking
Three public shapes changed. Pre-1.0, and each is a fix rather than a preference.
newsletterMetadata(jid)becomesnewsletterMetadata(type, key). This is the one that matters. Code written against upstream callsnewsletterMetadata('jid', id), and the old signature read that as a newsletter whose JID is the literal string'jid'. It type checked on the caller's side and failed against the server, which is the worst kind of incompatibility and why it outranks any missing method.Migration:
newsletterMetadata(jid)becomesnewsletterMetadata('jid', jid).A real gain comes with it: the
'invite'path now works. The core has a lookup per key kind (newsletterMetadata,newsletterMetadataByInvite), and choosing between them on a discriminator is exactly the translation this layer exists for.newsletterMute(jid, mute)splits intonewsletterMute(jid)andnewsletterUnmute(jid), as upstream has them.Migration:
newsletterMute(jid, true)becomesnewsletterMute(jid);newsletterMute(jid, false)becomesnewsletterUnmute(jid).Both route to
newsletterFollowerMute, not to the bridge's ambiguousnewsletterMutealias. The core has two newsletter mutes: follower activity, which is what a subscriber toggles, and admin activity, which is a different control for owners and admins. Upstream's is the subscriber one, so the explicit spelling is used to make that choice visible instead of resting on an alias.newsletterCreatereturnsNewsletterMetadatain upstream's field names, not the bridge'sNewsletterMetadataResult. Field renames:jidtoid,subscriberCounttosubscribers,inviteCodetoinvite,creationTimetocreation_time,pictureUrltopicture.url.Layer classification
Every item is (a), implementable here by delegation. Nothing needed the core or the bridge, and no gap prompt was opened. That matches the prompt's expectation after bridge 0.7.0.
newsletterCreatenewsletterCreatenewsletterMetadata('jid'|'invite', key)newsletterMetadata/newsletterMetadataByInvitenewsletterUpdatenewsletterUpdate+newsletterSetPicture/newsletterRemovePicturenewsletterUpdateName/newsletterUpdateDescriptionnewsletterUpdate(jid, name, description)newsletterUpdatePicturenewsletterSetPicturegenerateProfilePicturenewsletterRemovePicturenewsletterRemovePicturenewsletterFollow/newsletterUnfollownewsletterSubscribe/newsletterUnsubscribenewsletterMute/newsletterUnmutenewsletterFollowerMute(jid, bool)newsletterSubscribersnewsletterMetadata(jid).subscriberCountnewsletterFetchMessagesnewsletterMessagessubscribeNewsletterUpdatesnewsletterSubscribeLiveUpdates{ duration: string }newsletterAdminCountnewsletterAdminInfo(jid).adminCountnewsletterChangeOwnernewsletterChangeOwnernewsletterDemotenewsletterDemoteAdminnewsletterDeletenewsletterDeleteThree places the shapes do not line up
newsletterAdminCountwhen the server omits the count. It rides on the admin-info result and is optional there. It rejects rather than returning0, because0reads as "no admins", which no newsletter can be, and a caller acting on that would draw the wrong conclusion silently.newsletterFetchMessages(jid, count, since, after)refusessinceandafterby name. I checked the core rather than mapping by resemblance:get_messagesbuilds<messages count before>and has no time filter. Upstream'safterpages forward from a point; the core'sbeforepages backward from one. They are opposite directions, so mappingafterontobeforewould return a plausible wrong page, which is worse than refusing.countworks, which covers the common call.Four
NewsletterMetadatafields have no source and stay absent:owner(the bridge result carries the viewer's own role, not the owner's JID),mute_state,reaction_codes,thread_metadata.statewas the tempting wrong answer formute_state: it is the newsletter's lifecycle,Active/Suspended/Geosuspended, not a mute.This also settles a question left open in #26. The bridge sends
verificationin the core's camel case (Verified), while upstream's type is'VERIFIED' | 'UNVERIFIED'. The mapper writes the pairs out rather than upper-casing blindly, so a variant the core adds later arrives asundefinedinstead of as a string nobody declared.Deferred
Nothing. No gap prompt was written for the bridge or the core.
Out of scope by the task's own boundary and untouched: channel directory, recommendations, insights, admin invites, pinning a message.
Coverage
npm run compat:auditbefore: 97.15% (21717/22354)after: 97.34% (21759/22354)
Missing dropped sharply: functions 243 to 198, fields 235 to 190.
Mismatches rose (functions 70 to 94, fields 27 to 51) and that needs an honest explanation rather than being left to look like damage. They are the return types, and every one is stronger than upstream's:
Upstream types most of these
Promise<unknown>because its MEX executor returns an unparsed result. Returning the parsed metadata is assignable tounknown, so code written against upstream compiles and runs unchanged; only the auditor's string comparison counts it as a difference.newsletterFetchMessagesis the same story withPromise<any>againstPromise<NewsletterMessageResult[]>, plus optional-vs-required on the two parameters it refuses.Making these
Promise<unknown>would score better and be worse to use. Leaving them is the deliberate choice; the signature-mismatch task is where this class of item gets triaged in bulk.Validation
npm run formatnpm run lint(tscplusoxlint)npm test: 698 tests, 0 failures, up from 666, with no existing test adaptednpm run compat:auditNot run:
npm run test:e2e, which needs a mock server this environment does not have.Mutation checked by making the edit and running the suite, not by reading the assertions:
newsletterMetadataignoretypeand always look up by JID fails 1 testnewsletterAdminCountfall back to0fails 1 testafteronto thebeforecursor instead of refusing fails 1 testGenerated by Claude Code
Summary by cubic
Completes the upstream newsletter API: adds 15 missing methods, fixes 3 signatures, and aligns results to upstream types. Keeps legacy subscribe/unsubscribe, optimizes picture-only updates, and rejects unsupported paging flags.
New Features
newsletterAdminCount,newsletterChangeOwner,newsletterDelete,newsletterDemote,newsletterFetchMessages,newsletterFollow,newsletterRemovePicture,newsletterSubscribers,newsletterUnfollow,newsletterUnmute,newsletterUpdate,newsletterUpdateDescription,newsletterUpdateName,newsletterUpdatePicture,subscribeNewsletterUpdates.src/Compatibility/newsletter-results.ts; supportnewsletterMetadata('invite', code).newsletterFetchMessagesrejects non-zerosince/after(zeros pass),subscribeNewsletterUpdatesreturns{ duration: string },newsletterAdminCountthrows if the server omits the count.newsletterUpdate({ picture })skips an extra metadata read; picture writes already return refreshed metadata.Migration
newsletterMetadata(jid)→newsletterMetadata('jid', jid); also enablesnewsletterMetadata('invite', code).newsletterMute(jid, true|false)→newsletterMute(jid)andnewsletterUnmute(jid)(maps to follower-activity mute).newsletterCreatenow returns upstreamNewsletterMetadatafield names:jid→id,subscriberCount→subscribers,inviteCode→invite,creationTime→creation_time,pictureUrl→picture.url.newsletterSubscribe/newsletterUnsubscribe; they remain.newsletterSubscribekeeps the bridge result shape; usenewsletterFollowfor upstream field names.Written for commit e5b5be2. Summary will update on new commits.