Add clipped field to annotation summary types #2078
Conversation
WalkthroughExported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client code
participant DefaultMessage as DefaultMessage.fromEncoded
participant Message as Message.expandFields
participant Summary as Summary entries
Client->>DefaultMessage: call fromEncoded(encodedMsg)
DefaultMessage->>Message: construct Message and call expandFields()
Message->>Summary: iterate summary entries
alt type ends with :distinct.v1 / :unique.v1 / :multiple.v1
Note right of Summary#00DD99: nested entries checked\nmissing `clipped` → set false
Summary-->>Message: set nested entries' `clipped = false` if undefined
else type ends with :flag.v1
Note right of Summary#F6C85F: top-level flag normalized\nmissing `clipped` → set false
Summary-->>Message: set top-level `clipped = false` if undefined
else
Summary-->>Message: preserve existing `clipped` if present
end
Message-->>DefaultMessage: return normalized Message
DefaultMessage-->>Client: return Message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (6)
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 |
github-actions
bot
temporarily deployed
to
staging/pull/2078/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2078/features
github-actions
bot
temporarily deployed
to
staging/pull/2078/typedoc
![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
🧹 Nitpick comments (1)
ably.d.ts (1)
3148-3151: Clipped flag on SummaryClientIdList looks good; consider a tiny doc tweakDoc is clear. Optional nit: call out that
clientIdsmay be a subset when clipped.Apply to tighten the JSDoc:
- /** Whether the list of clientIds has been clipped due to exceeding the maximum number of - * clients. */ + /** Whether the list of clientIds has been clipped due to exceeding the maximum number of + * clients. When true, `clientIds` contains only a subset; use `total` for the full count. */ clipped?: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ably.d.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
ably.d.ts
[warning] 3165-3165:
Expected JSDoc block to be aligned
🪛 GitHub Actions: Lint
ably.d.ts
[warning] 1-1: Prettier formatting issues found in ably.d.ts. Run 'prettier --write' to fix.
⏰ 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). (7)
- GitHub Check: test-npm-package
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (2)
ably.d.ts (2)
1-1: Fix Prettier formatting to unblock lint pipelineCI reports Prettier issues in ably.d.ts. Please run the project formatter (e.g.,
pnpm formatorprettier --write ably.d.ts) to normalize spacing, including the JSDoc alignment noted above.
3165-3169: Align JSDoc formatting and clarifytotalClientIdswording/** Whether the list of clientIds has been clipped due to exceeding the maximum number of - * clients. */ + * clients. */ clipped?: boolean; - /** The total number of distinct clientIds in the map (equal to length of map if clipped is false). */ + /** The total number of distinct identified clientIds in `clientIds` + * (equals `Object.keys(clientIds).length` when `clipped` is false). Excludes + * unidentified contributions counted in `totalUnidentified`. */ totalClientIds: number;Confirm the service always provides
totalClientIdsin production payloads (otherwise make it optional to avoid a breaking change).
ably.d.ts
Outdated
| clientIds: string[]; | ||
| /** Whether the list of clientIds has been clipped due to exceeding the maximum number of | ||
| * clients. */ | ||
| clipped?: boolean; |
There was a problem hiding this comment.
Is this likely to become non-nullable in the future? Would it be worth making it non-nullable and then having it just forced to false in expandFields()?
There was a problem hiding this comment.
It's omitempty and that's a good thing I'd say, so not sure we'll ever explicitly set clipped: false?
There was a problem hiding this comment.
I think this will be mainly driven by what other languages will do and how much faff it introduces there - in languages like Go and C# it would require null checks to do a simple if (clipped).
There was a problem hiding this comment.
I don't imagine in Go we will make it
clipped *bool `json:"omitempty"`
but rather
clipped bool `json:"omitempty"`
For languages where it matters there's no point to have a third "this wasn't set" state, just use false, but in JS this maps to the JSON representation and all checks are identical (if (x.clipped) {} is the same even if clipped field is optional).
There was a problem hiding this comment.
I went away and thought about it for a bit and my gut feeling is that the field should match its semantics. There's no current case for unset here (we always know if its clipped) so I think it should be a non-nullable boolean.
There was a problem hiding this comment.
I agree with Andy. We should use a nullable type in the client API if and only if it makes semantic sense for it to be nullable, i.e. it can be unset. And whichever way we decide, we should be consistent about it between SDKs.
So a Message might or might not have a clientId: if it was published by an unidentified client, it just won't have one. So it is right for that field to be nullable. But in the case of 'clipped', there's no semantic 'unset' distinct from false, if the summary isn't clipped it should just say false.
in JS this maps to the JSON representation
This is not a strong consideration for the question of what the SDK type should be.There are plenty of fields we don't populate on the wire to save space but are still populated (and typed as such) by the SDK when a message arrives, like message.timestamp/id/connectionId (populated by the sdk from the ChannelMessage), or the version from the serial for CREATEs, etc. The SDK is expected to unpack the wire representation a bit to present a nicer API.
There was a problem hiding this comment.
updated it to be explicitly set
vladvelici
force-pushed
the
add-clipped-summary-field
branch
from
c8d861e to
b70acb7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2078/features
github-actions
bot
temporarily deployed
to
staging/pull/2078/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2078/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2078/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2078/features
github-actions
bot
temporarily deployed
to
staging/pull/2078/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/common/lib/types/message.ts (2)
124-139: Default ‘clipped’ only when missing (avoid generic falsy checks)Use undefined checks so explicit false is preserved without redundant writes.
Apply:
- if (type.endsWith(':distinct.v1') || type.endsWith(':unique.v1') || type.endsWith(':multiple.v1')) { - for (const [, entry] of Object.entries(summaryEntry)) { - if (!entry.clipped) { - entry.clipped = false; - } - } - } else if (type.endsWith(':flag.v1')) { - if (!(summaryEntry as API.SummaryClientIdList).clipped) { - (summaryEntry as API.SummaryClientIdList).clipped = false; - } - } + if (type.endsWith(':distinct.v1') || type.endsWith(':unique.v1') || type.endsWith(':multiple.v1')) { + for (const [, entry] of Object.entries(summaryEntry)) { + if (entry.clipped === undefined) { + entry.clipped = false; + } + } + } else if (type.endsWith(':flag.v1')) { + if ((summaryEntry as API.SummaryClientIdList).clipped === undefined) { + (summaryEntry as API.SummaryClientIdList).clipped = false; + } + }If TS target supports it,
entry.clipped ??= false;is even cleaner.
124-139: Future‑proof type detectionString suffix checks couple behavior to versioned keys. Consider a shape/type‑guard approach (e.g., detect a dict of entries vs a flat list object) or narrow to a common value type (e.g.,
Record<string, { clipped?: boolean }>), to make v2/vN extensions safer.test/realtime/message.test.js (2)
1371-1371: Remove debug logging from test
console.log(message.summary);adds noise to CI output.Apply:
- console.log(message.summary); + // no-op
1430-1431: Clean up commented assertionLeftover debug line can be dropped.
Apply:
- // expect(false).to.equal(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ably.d.ts(3 hunks)src/common/lib/types/message.ts(2 hunks)test/realtime/message.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/lib/types/message.ts (1)
ably.d.ts (2)
SummaryEntry(3180-3185)SummaryClientIdList(3141-3151)
⏰ 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). (6)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (3)
src/common/lib/types/message.ts (2)
124-139: In‑place mutation — confirm invariantsexpandFields() mutates
this.summary(and nested entries). GivenWireMessage.decodeWithErrshallow‑copies from the encoded values, this also mutates the caller‑provided object. Confirm this is intended and won’t surprise integrators relying on the original reference.
111-111: Change not applied — Message.summary still typed as any; update types, regenerate d.ts, audit call sites
- src/common/lib/types/message.ts:170 still has
summary?: any.- ably.d.ts references message.summary — regenerate the typings after changing the source type to
Record<string, API.SummaryEntry>.- Audit Message.fromValues usages in src/common/lib/client/restchannel.ts and src/common/lib/client/realtimechannel.ts for code that assumes
any.Likely an incorrect or invalid review comment.
test/realtime/message.test.js (1)
1334-1431: Coverage for defaulting ‘clipped’ looks goodThe assertions thoroughly exercise distinct/unique/multiple/flag cases and preserve explicit trues. Nice.
src/common/lib/types/message.ts
Outdated
| this.createdAt = this.timestamp; | ||
| } | ||
| } | ||
| if (this.summary) { |
There was a problem hiding this comment.
If we don't have a spec entry for this behaviour, we should add one (and add a comment here)
| expect(updateMessage.serial).to.equal(undefined); | ||
| }); | ||
|
|
||
| it('should ensure clipped field is set to false where not explicitly provided', async function () { |
vladvelici
force-pushed
the
add-clipped-summary-field
branch
from
052a0f0 to
51ca01a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2078/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2078/typedoc
vladvelici
force-pushed
the
add-clipped-summary-field
branch
from
51ca01a to
354a6ae
Compare
|
Added spec points reference, spec points are in this PR: ably/specification#382 |
github-actions
bot
temporarily deployed
to
staging/pull/2078/features
github-actions
bot
temporarily deployed
to
staging/pull/2078/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2078/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/common/lib/types/message.ts (1)
142-143: Consider using explicit undefined checks for better clarity.The current condition
!entry.clippedwill be truthy for bothundefinedandfalsevalues, which means it would incorrectly override an explicitly setfalsevalue. Consider using explicit undefined checks to ensure only missing fields are normalized.Apply this diff to use explicit undefined checks:
- if (!entry.clipped) { + if (entry.clipped === undefined) { entry.clipped = false; }- if (!(summaryEntry as API.SummaryClientIdList).clipped) { + if ((summaryEntry as API.SummaryClientIdList).clipped === undefined) { (summaryEntry as API.SummaryClientIdList).clipped = false; }Also applies to: 148-149
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ably.d.ts(3 hunks)src/common/lib/types/message.ts(1 hunks)test/realtime/message.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/realtime/message.test.js
- ably.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/lib/types/message.ts (1)
ably.d.ts (1)
SummaryClientIdList(3143-3153)
⏰ 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). (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
🔇 Additional comments (2)
src/common/lib/types/message.ts (2)
136-153: LGTM! Correct implementation of clipping defaults normalization.The logic correctly implements the normalization behavior for different summary entry types:
- For entries ending with
:distinct.v1,:unique.v1, or:multiple.v1, it traverses nested entries and setsclippedtofalseif not explicitly defined- For entries ending with
:flag.v1, it sets the top-levelclippedfield tofalseif not explicitly definedThe implementation aligns with the spec references (TM7c1c, TM7d1c) and matches the expected behavior described in the AI summary.
140-141: Resolve — iteration is type-safe: 'clipped' exists on mapped entry types.ably.d.ts defines SummaryDistinctValues/SummaryUniqueValues as Record<string, SummaryClientIdList> and SummaryMultipleValues as Record<string, SummaryClientIdCounts>, and both SummaryClientIdList and SummaryClientIdCounts declare clipped: boolean; the loop in src/common/lib/types/message.ts (lines 137–146) is safe — no changes needed.
Add
clippedandtotalClientIdsfields to summary types. https://ably.atlassian.net/browse/CHA-1100https://ably.atlassian.net/browse/CHA-1178
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests
Chores