fix: make channels.close idempotent to prevent error toast on already…#38844
fix: make channels.close idempotent to prevent error toast on already…#38844sriramsowmithri9807 wants to merge 3 commits into
Conversation
…-hidden channels Previously, clicking Hide on an already-hidden channel returned an API failure with 'The channel is already closed to the sender', which surfaced as an error toast in the UI. This changes the endpoint to return success silently, making the hide action idempotent.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughThe channels.close API endpoint behavior was modified to return success instead of failure when a user attempts to close an already-closed channel. Additionally, package.json was updated with a one-line modification. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/channels.ts (1)
369-371: Consider makingchannels.opensymmetric.
channels.closeis now idempotent, but its counterpart still returns a 400-level failure whensub.openis alreadytrue. The same UX / API-consistency argument applies in both directions.♻️ Proposed change to make `channels.open` consistent
- if (sub.open) { - return API.v1.failure(`The channel, ${findResult.name}, is already open to the sender`); - } + if (sub.open) { + return API.v1.success(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/channels.ts` around lines 369 - 371, The channels.open handler currently returns API.v1.failure when sub.open is already true, making open non-idempotent and inconsistent with channels.close; update the logic in the channels.open flow (the branch that checks sub.open and builds the response referencing findResult.name) to treat an already-open subscription as a successful no-op instead of a 400-level failure—return the same success shape used for a fresh open (or API.v1.success) and ensure no duplicate state changes are attempted so the operation becomes symmetric with channels.close.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/channels.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/channels.ts
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/channels.ts (1)
604-606: LGTM — correct idempotent behavior.Returning
API.v1.success()when!sub.openis the right choice. The subscription is already in the desired state, so the operation is a no-op; making the endpoint idempotent is well-aligned with REST API design principles and directly eliminates the spurious error toast described in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/v1/channels.ts`:
- Around line 369-371: The channels.open handler currently returns
API.v1.failure when sub.open is already true, making open non-idempotent and
inconsistent with channels.close; update the logic in the channels.open flow
(the branch that checks sub.open and builds the response referencing
findResult.name) to treat an already-open subscription as a successful no-op
instead of a 400-level failure—return the same success shape used for a fresh
open (or API.v1.success) and ensure no duplicate state changes are attempted so
the operation becomes symmetric with channels.close.
2 participants
Fix the
channels.closeAPI endpoint to make the Hide channel action idempotent and prevent unnecessary error responses when the channel is already hidden.Proposed Changes
This PR introduces a one-line change in the
channels.closeendpoint so that attempting to hide an already-hidden channel returns a successful response instead of an error.Before
If
subscription.open === false, the API returned:This produced an error toast in the UI when users attempted to hide a channel that was already hidden.
After
The endpoint now returns:
The operation becomes a silent no-op, matching expected UX behaviour and making the endpoint idempotent.
Changed File
if (!sub.open) { - return API.v1.failure(`The channel, ${findResult.name}, is already closed to the sender`); + return API.v1.success(); }Issue Addressed
Previously, hiding a channel could trigger an error under the following scenario:
Ctrl + K)Because opening via search does not call
openRoom(), the subscription’sopenflag remainsfalse.The API then incorrectly reports the channel as “already closed”, causing an unnecessary UI error toast.
This PR ensures the hide action behaves consistently regardless of how the room was reopened.
Steps to Reproduce
#newone)Before this fix
"The channel, newone, is already closed to the sender"After this fix
Impact
channels.closeidempotentSummary by CodeRabbit