Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request renames the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
sacOO7
force-pushed
the
fix/channels-presence
branch
from
c6581fd to
33d1679
Compare
sacOO7
force-pushed
the
fix/channels-presence
branch
from
33d1679 to
f740053
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/commands/channels/presence/update.test.ts (2)
132-139:⚠️ Potential issue | 🔴 CriticalTest will fail: holding status is now at
lines[2], notlines[1].Same issue as above—with the new warning line, the holding status has shifted to index 2.
Proposed fix
const lines = stdout.trim().split("\n").filter(Boolean); - expect(lines.length).toBeGreaterThanOrEqual(2); + expect(lines.length).toBeGreaterThanOrEqual(3); - const status = JSON.parse(lines[1]); + // lines[0] = warning, lines[1] = result, lines[2] = holding + const status = JSON.parse(lines[2]); expect(status.type).toBe("status"); expect(status.status).toBe("holding"); expect(status.message).toContain("Holding presence");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/presence/update.test.ts` around lines 132 - 139, The test assumes the "holding" status is at lines[1] but a new warning line pushed it to lines[2]; update the assertion in the test (in test/unit/commands/channels/presence/update.test.ts) to parse and assert against lines[2] (or more robustly locate the JSON object whose type is "status" and status is "holding" instead of assuming a fixed index) by changing the JSON parse/expect reference from lines[1] to lines[2] (or implement a find over lines to pick the correct status object) so the test checks the actual holding status line.
105-116:⚠️ Potential issue | 🔴 CriticalTest will fail: JSON output indexes are off by one due to new warning line.
The implementation at
update.ts:62now emits a warning status as the first JSON line when--jsonis used. This means:
lines[0]= warning status (type: "status",status: "warning")lines[1]= result (type: "result")lines[2]= holding status (type: "status",status: "holding")The test expects
lines[0]to be the result, which will fail.Proposed fix
const lines = stdout.trim().split("\n").filter(Boolean); - expect(lines.length).toBeGreaterThanOrEqual(1); + expect(lines.length).toBeGreaterThanOrEqual(2); - const result = JSON.parse(lines[0]); + // lines[0] is the warning status, lines[1] is the result + const warning = JSON.parse(lines[0]); + expect(warning.type).toBe("status"); + expect(warning.status).toBe("warning"); + + const result = JSON.parse(lines[1]); expect(result.type).toBe("result");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/presence/update.test.ts` around lines 105 - 116, The test assumes the first JSON line is the result but update.ts now emits a warning as the first JSON line (see update.ts:62), so change the assertion to locate the JSON object with type === "result" instead of using lines[0]; parse the JSON lines array (variable lines) and pick the entry where parsed.type === "result" (or filter for type "result" and take the first match) and run the existing presenceMessage assertions against that object.
🧹 Nitpick comments (1)
src/commands/channels/presence/update.ts (1)
34-37: Flag description differs from the sharedclientIdFlagconvention.The local definition uses a shorter description than the shared
clientIdFlaginsrc/flags.ts, which includes context about the "none" option and token authentication. If this simplification is intentional for this command, consider documenting the rationale. Otherwise, align with the shared flag's description for consistency.Additionally, the description ends with a period while most flag descriptions in the codebase do not.
Suggested description alignment
"client-id": Flags.string({ - description: "ClientId of a channel presence member.", + description: + 'Client ID for the presence member. Use "none" to explicitly set no client ID', required: true, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/channels/presence/update.ts` around lines 34 - 37, The "client-id" flag in update.ts diverges from the shared clientIdFlag convention: update the "client-id" Flags.string description to match the shared clientIdFlag wording (include the "none" option and token authentication context) and remove the trailing period to match codebase style so the local flag description is consistent with clientIdFlag used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 1945: The fenced code block in the README under the presence-get usage
(the block showing the "USAGE $ ably channels presence get CHANNEL ..." example)
is unlabeled and triggers markdownlint MD040; update the opening fence from ```
to ```text (i.e., add the language label "text" to the opening triple backticks
for the presence-get example) so the block is explicitly labeled while leaving
the content and closing fence unchanged.
---
Outside diff comments:
In `@test/unit/commands/channels/presence/update.test.ts`:
- Around line 132-139: The test assumes the "holding" status is at lines[1] but
a new warning line pushed it to lines[2]; update the assertion in the test (in
test/unit/commands/channels/presence/update.test.ts) to parse and assert against
lines[2] (or more robustly locate the JSON object whose type is "status" and
status is "holding" instead of assuming a fixed index) by changing the JSON
parse/expect reference from lines[1] to lines[2] (or implement a find over lines
to pick the correct status object) so the test checks the actual holding status
line.
- Around line 105-116: The test assumes the first JSON line is the result but
update.ts now emits a warning as the first JSON line (see update.ts:62), so
change the assertion to locate the JSON object with type === "result" instead of
using lines[0]; parse the JSON lines array (variable lines) and pick the entry
where parsed.type === "result" (or filter for type "result" and take the first
match) and run the existing presenceMessage assertions against that object.
---
Nitpick comments:
In `@src/commands/channels/presence/update.ts`:
- Around line 34-37: The "client-id" flag in update.ts diverges from the shared
clientIdFlag convention: update the "client-id" Flags.string description to
match the shared clientIdFlag wording (include the "none" option and token
authentication context) and remove the trailing period to match codebase style
so the local flag description is consistent with clientIdFlag used elsewhere.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 927f8ad1-cac4-4c5b-9a7d-17367172f833
📒 Files selected for processing (5)
README.mdsrc/commands/channels/presence/get.tssrc/commands/channels/presence/update.tstest/unit/commands/channels/presence/get.test.tstest/unit/commands/channels/presence/update.test.ts
There was a problem hiding this comment.
Pull request overview
Renames the Channels Presence command from get-all to get, and adjusts docs/tests accordingly; also updates channels presence update to emit a warning and require a client ID.
Changes:
- Rename
channels presence get-allcommand/test/docs tochannels presence get. - Update
channels presence updateto require--client-id, emit a warning, and adjust how--datais parsed/used. - Refresh README command references and “See code” links for the renamed command.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/channels/presence/update.test.ts | Updates tests to pass --client-id for the update command. |
| test/unit/commands/channels/presence/get.test.ts | Updates tests to use the new channels:presence:get command ID. |
| src/commands/channels/presence/update.ts | Adds a warning, changes --client-id/--data flag handling, and adjusts presence enter/update behavior. |
| src/commands/channels/presence/get.ts | Renames the command class/telemetry identifiers and updates examples to get. |
| README.md | Renames documentation references from get-all to get and updates the source link. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sacOO7
force-pushed
the
fix/channels-presence
branch
from
f740053 to
d824bdb
Compare
sacOO7
force-pushed
the
fix/channels-presence
branch
from
d824bdb to
07b9fee
Compare
sacOO7
force-pushed
the
fix/channels-presence
branch
from
07b9fee to
f1b47f1
Compare
sacOO7
force-pushed
the
fix/channels-presence
branch
from
f1b47f1 to
2681d90
Compare
sacOO7
force-pushed
the
fix/channels-presence
branch
from
2681d90 to
dd0d8d8
Compare
- Removed channels presence update command
sacOO7
force-pushed
the
fix/channels-presence
branch
from
dd0d8d8 to
8aca26c
Compare
channels presence get-alltochannels presence getpresence updatesince exact same behaviour can be achieved usingpresence entertwice ( added description for the same )Summary by CodeRabbit
Documentation
ably channels presence get-allrenamed toably channels presence get.Command Updates
channels presence updatecommand now requires the--client-idflag.