Repository navigation
feat(socket): add the remaining server-side query wrappers - #32
Conversation
|
Warning Review limit reached
Next review available in: 49 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/Socket/index.ts | Composes the new server-query method factory into each socket instance without colliding with existing methods. |
| src/Socket/server-queries.ts | Implements the five query wrappers, per-socket media metadata state, input validation, and two explicit unsupported-operation responses. |
| src/tests/server-queries-compatibility.test.ts | Covers result mapping, bot deduplication, timestamp handling, media refresh behavior, error propagation, and unsupported operations. |
Reviews (7): Last reviewed commit: "fix(media): fetch on the first media con..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
64b870c to
ca0d62b
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![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: ca0d62b456
ℹ️ 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".
| ): Promise<Omit<MediaConnInfo, 'hosts'> & { hosts: { hostname: string }[] }> => { | ||
| const conn = await (await ctx.getClient()).getMediaConn(forceGet) | ||
| mediaHost = conn.hosts[0]?.hostname ?? mediaHost | ||
| return { auth: conn.auth, ttl: conn.ttl, hosts: conn.hosts, fetchDate: new Date() } |
There was a problem hiding this comment.
When refreshMediaConn(false) is called repeatedly before expiry, the bridge may return its cached media connection, but this line assigns a new fetchDate on every access. Consumers that use fetchDate + ttl to schedule renewal can therefore treat old credentials as valid for another full TTL and refresh them after they have actually expired. Preserve the prior fetch time for cached results, resetting it only when the connection is genuinely fetched again or the call is forced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 0b36f75. Upstream only stamps a fetchDate inside the branch that actually re-queries (messages-send.js:40-61); a cached read returns the node with its original date. Restamping on every call meant a consumer renewing on fetchDate + ttl kept pushing the deadline forward and would carry expired credentials for another full ttl.
The date is now kept alongside the credentials it belongs to and only moves when the credentials do, keyed on auth, so a connection the engine served from its cache reports the moment it was fetched. Two tests: a repeated call keeps the first date, and new credentials get a later one. Both checked by reverting the line.
Generated by Claude Code
| ): Promise<Omit<MediaConnInfo, 'hosts'> & { hosts: { hostname: string }[] }> => { | ||
| const conn = await (await ctx.getClient()).getMediaConn(forceGet) | ||
| mediaHost = conn.hosts[0]?.hostname ?? mediaHost | ||
| return { auth: conn.auth, ttl: conn.ttl, hosts: conn.hosts, fetchDate: new Date() } |
There was a problem hiding this comment.
When UserFacingSocketConfig.customUploadHosts is provided, refreshMediaConn still returns only the bridge-supplied hosts because the factory receives no configuration and this is the only host construction in the repository. This leaves the documented custom-host option ineffective for callers that obtain upload targets through the media connection API; merge the configured hosts into the returned host list, as the compatible socket contract requires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not merging them here, because upstream does not either. refreshMediaConn in upstream returns only the server-supplied hosts (messages-send.js:53-61); the merge happens one layer out, in getWAUploadToServer, which does const hosts = [...customUploadHosts, ...uploadInfo.hosts] (Utils/messages-media.js:648). Merging here would make this method disagree with the upstream one it mirrors, and would double the custom hosts for anything that later goes through an upload path that merges them again.
You are right that the option is currently inert in this package, though: customUploadHosts appears only in src/Defaults/index.ts and src/Types/Socket.ts, with no reader. That is the upload path, not this one, and it predates this PR. Worth its own change rather than being smuggled into the media-conn wrapper, so I have left it out of scope here.
Generated by Claude Code
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b36f7590d
ℹ️ 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".
| if (!fetched || fetched.auth !== conn.auth) { | ||
| fetched = { auth: conn.auth, at: new Date() } | ||
| } |
There was a problem hiding this comment.
When refreshMediaConn(true) forces the bridge to fetch a connection but the server returns the same auth token (for example, while renewing the TTL or host list), this auth-only condition preserves the old fetchDate. Consumers then calculate expiry from a timestamp that predates the forced fetch and may renew unnecessarily early. The fresh evidence in this revision is the new fetched.auth check, which still ignores the forceGet signal; a successful forced fetch should always receive a new timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 6f8618d. Keying only on auth meant a forced refresh that came back with the same token but a renewed ttl kept the previous date, so a caller measuring fetchDate + ttl renewed earlier than it needed to.
forceGet now restamps unconditionally, since a forced call is a fetch whatever comes back. The unforced path still keeps the date when the engine serves the connection from its cache, which is the case the previous commit was for. A third test covers it: forced, with the same auth, gets a later date. Reverting to the auth-only condition makes it fail.
Generated by Claude Code
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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: Adds new public WASocket methods with deliberate API-shape choices (flattening bot lists across sections, optional quota field mapping, MediaConnInfo divergence), so a human should sign off on the exposed contract before consumers depend on it.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f8618dda2
ℹ️ 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".
| if (forceGet || !fetched || fetched.auth !== conn.auth) { | ||
| fetched = { auth: conn.auth, at: new Date() } |
There was a problem hiding this comment.
When the bridge's cached connection expires and a normal refreshMediaConn(false) triggers a server fetch that renews the TTL or hosts without rotating auth, this condition preserves the expired fetchDate. A consumer checking fetchDate + ttl will therefore continue treating the renewed result as already expired and may repeatedly request refreshes. The fresh evidence in this revision is that the forced same-auth path was added explicitly, while the equivalent expired non-forced path remains gated solely on an auth change; track whether the prior TTL has elapsed (or obtain an actual-fetch signal) as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it is the mirror of the forced case, so both are now covered by one rule in 1761fdd.
The root of both is that the engine gives no signal for which calls were actual fetches, so the previous conditions were guessing from auth and forceGet alone. The stamp is now kept only while the connection it describes is still live: past its own ttl, on a forced call, or on rotated credentials, whatever comes back is treated as a fetch.
That leaves both failure modes closed. Always restamping pushed the caller's deadline forward forever, which was the first finding; never restamping left it renewing against a moment already gone, which is this one. A fourth test covers the expired-and-unforced path with a zero ttl, and removing the liveness check makes it fail.
Generated by Claude Code
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: Adds new public socket methods whose shapes deliberately diverge from upstream (flattened bot list, MediaConnInfo omitting maxContentLengthBytes) and always-refusing stubs; these API/contract choices need human sign-off.
Re-trigger cubic
Five wrappers and two refusals. `getBotListV2` returns every section, deduplicated, rather than only the one the server types `all`. Upstream reads that section alone; the core refused to, because the real client walks all of them and uses the type for layout, so a bot that appears solely under a category is dropped by the narrow reading. Both readings are defensible and this one was chosen because a caller iterating the list copes with extra entries, while one that silently lost a bot has no way to notice. `fetchNewChatMessageCap` renames to upstream's snake case and stringifies the three timestamps it types as strings. Absent quotas stay absent: `0` in this API means the quota is spent, so defaulting would report the opposite of "unknown". The core's `remainingQuota` derivation is carried through as an extra field. `refreshMediaConn` takes the force flag the bridge already accepted, and `getMediaHost` reads the host the last refresh saw, staying synchronous as upstream has it. `maxContentLengthBytes` is not invented: the core's hosts carry a hostname and nothing else. `cleanDirtyBits` accepts the string timestamp upstream allows and rejects one that is not a number, rather than sending NaN. `createCallLink` and `sendPeerDataOperationMessage` reject, and the messages name which layer refused and why: the first sits behind the core's voip feature, which the bridge does not compile, and the second would duplicate a request the engine already makes on decryption failure. The history request a consumer actually drives is `fetchMessageHistory`.
The engine serves a live media connection from its own cache, so a second refreshMediaConn call is not a second fetch. Stamping a new fetchDate on every call let a consumer renewing on fetchDate + ttl treat expired credentials as good for another full ttl. The date is now kept alongside the credentials it belongs to and only moves when the credentials do, which is what upstream's cached mediaConn does.
A forced refresh that came back with the same auth token kept the previous fetch date, so a caller renewing on fetchDate + ttl measured a renewed connection from the moment the old one was obtained and would renew earlier than it needed to.
Three follow-ups from review of this branch.
The fetch date is handed out as a copy. The stored instant decides when
the next call restamps, and returning it by reference let a consumer move
that decision.
cleanDirtyBits rejects a blank timestamp string. Number('') is the epoch,
so a caller meaning "no timestamp" was asking the server to clean from
the beginning of time.
The capping test that claimed to guard the zero-means-spent rule only
exercised full omission. A second case sends a zero quota and asserts it
survives, so a truthiness check would fail it.
The engine gives no signal for which calls were actual fetches, so keeping the date on any same-auth result left a connection that had expired and been refetched reporting the moment the previous one was obtained. A caller renewing on fetchDate + ttl then measured against a deadline already passed and refreshed on every call. The stamp is now kept only while the connection it describes is still live.
1761fdd to
0633e98
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0633e980e9
ℹ️ 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".
| ): Promise<Omit<MediaConnInfo, 'hosts'> & { hosts: { hostname: string }[] }> => { | ||
| const conn = await (await ctx.getClient()).getMediaConn(forceGet) | ||
| mediaHost = conn.hosts[0]?.hostname ?? mediaHost | ||
| if (forceGet || !fetched || fetched.auth !== conn.auth || !isLive(fetched)) { |
There was a problem hiding this comment.
When waUploadToServer/uploadMedia has already populated the engine's media-connection cache before the first public refreshMediaConn(false) call, !fetched stamps that cached connection as newly fetched even though part of its TTL has already elapsed. A caller using fetchDate + ttl can consequently retain the returned credentials past their real expiry. The fresh evidence beyond the earlier timestamp findings is the existing waUploadToServer path in src/Socket/index.ts, which can acquire a media connection before this wrapper has any local fetched state; the initial call therefore needs an actual-fetch signal or must force a fetch rather than assuming every first observation is fresh.
Useful? React with 👍 / 👎.
The engine may already hold a connection an upload acquired, and it gives no signal for how old that one is, so treating the first observation as freshly fetched reported credentials as newer than they were and let a caller renewing on fetchDate + ttl keep them past their real expiry. The first call now forces a fetch, which is what upstream's first call does as well, so the date it stamps describes a fetch that happened.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
On the "don't restamp an already-populated bridge cache" finding in There is no actual-fetch signal to read, so the first call now forces one. That is also what upstream does: its After that, Generated by Claude Code |
Summary
Five wrappers land, two refuse. The items have nothing in common but their shape, so this is a set of loose ends rather than a domain.
In:
getBotListV2,fetchNewChatMessageCap,cleanDirtyBits,refreshMediaConn,getMediaHost.Refusing:
createCallLink,sendPeerDataOperationMessage.Layer classification
getBotListV2getBotList(), flattened; see the choice belowfetchNewChatMessageCapfetchNewChatMessageCappingInfo(), renamed to upstream's snake casecleanDirtyBitscleanDirtyBits(type, timestamp)refreshMediaConngetMediaConn(force), which already took the flaggetMediaHostcreateCallLinksendPeerDataOperationMessageNo gap prompt was opened. Neither refusal is a gap: one is a deliberate dependency decision in the bridge, the other is an operation the engine already performs.
Design
The bot list has a real choice in it, and I took the wider one
Upstream reads only the section the server types
all, and extractsjidandpersona_id. The core deliberately refused that filter: the real client walks every section and usestype/display_typeonly for layout, so a bot appearing solely under a category section is dropped by the narrow reading.I return every section, flattened and deduplicated by JID, under upstream's signature.
Both readings are defensible, and this is why I picked this one: a caller iterating a bot list copes with more entries than it expected, while a caller that silently lost a bot has no way to notice it happened. The cost is that a consumer replicating upstream's exact set will see extras. There is a test for a category-only bot surviving, and for a bot listed twice appearing once.
Zero is not "unknown"
fetchNewChatMessageCaprenames to upstream'stotal_quota,used_quotaand friends, and stringifies the three timestamps it types as strings. Every field is optional because the server omits what does not apply to the account's tier.An absent quota stays absent.
0in this API means the quota is spent, so defaulting an unknown to0would tell the caller the opposite of the truth. The core's ownremainingQuotaderivation is carried through asremaining_quota, an extra field beyond upstream's type.maxContentLengthBytesis not inventedMediaConnInfodeclares it upstream. The core's hosts carry a hostname and nothing else, which #26 already established when the bridge corrected its own stale doc comment. So the field is absent here and the return type diverges, rather than being filled with a number nobody sent.getMediaHoststays synchronous as upstream has it, reading the host the lastrefreshMediaConnsaw. That is a cache of a value we fetched ourselves, one field wide, not a shadow of protocol state the core owns.The prompt's note that
getMediaConnalready accepted the force flag is correct: the earlier claim that it was missing was wrong, and there was nothing to add below this layer. There is a test that forcing and not forcing reach the bridge with different arguments, since a silently ignored flag is exactly the kind of thing nobody notices.The two refusals name which layer refused
createCallLinkis not a forgotten wrapper.create_call_linkexists in the core behind itsvoip-runtimefeature, and the bridge pins the core with default features off, so the method is not compiled into the wasm artifact at all. Reaching it means pullingwebrtc-dtls,webrtc-sctp,webrtc-dataandrustlsinto a wasm bundle. That is a size and dependency decision someone has to take deliberately, so the message says so rather than implying a missing binding.sendPeerDataOperationMessageis refused on the merits. The core already fires a peer data request when a message fails to decrypt, with its own age policy, so a second one from here would duplicate it. And the PDO a consumer actually drives, asking for history, is alreadyfetchMessageHistory. The message points there.Deferred
Nothing, and that is a claim rather than an omission: both open items are answered decisions rather than pending work. If the voip build question is ever revisited, it belongs in the bridge, not here.
Coverage
npm run compat:auditbefore: 97.15% (21717/22354)after: 97.26% (21741/22354)
The two refusals and the
MediaConnInfodivergence do not score.refreshMediaConnreturns hosts withoutmaxContentLengthBytes, andcreateCallLink/sendPeerDataOperationMessagereturnPromise<never>against upstream's declared results, so all three count as mismatches. Scoring them would mean inventing a byte limit and pretending two operations exist.Validation
npm run formatnpm run lint(tscplusoxlint)npm test: 711 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:
allsection fails 1 testgetMediaHostupdate fails 1 testGenerated by Claude Code
Summary by cubic
Adds the remaining server-side query wrappers, clarifies two unsupported operations, and hardens media connection handling by forcing a real fetch on the first call and stamping the actual fetch time.
New Features
getBotListV2: returns bots from all sections; flattened and deduped by JID.fetchNewChatMessageCap: snake_case fields; timestamps as strings; preserves omissions; addsremaining_quota.cleanDirtyBits: forwards type; accepts number or string timestamp; rejects non-numeric or blank strings (400).refreshMediaConn: forwardsforce; first call forces a fetch; omitsmaxContentLengthBytes; returnsfetchDatewhen credentials were fetched; restamps on force, credential change, or TTL expiry; returnsfetchDateby value; updates cached host forgetMediaHost.getMediaHost: sync; returns the last host seen byrefreshMediaConn.Migration
createCallLink: not available; requires the core VOIP feature in the wasm bridge.sendPeerDataOperationMessage: not supported; usefetchMessageHistory. The engine already issues its own peer data request on decryption failures.Written for commit f5c1de0. Summary will update on new commits.