ci: fix callback in permissions.helper.ts for promise resolution#39390
ci: fix callback in permissions.helper.ts for promise resolution#39390
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39390 +/- ##
===========================================
- Coverage 70.91% 70.90% -0.01%
===========================================
Files 3196 3196
Lines 113254 113254
Branches 20514 20516 +2
===========================================
- Hits 80309 80301 -8
- Misses 30900 30902 +2
- Partials 2045 2051 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 1 file
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="apps/meteor/tests/data/permissions.helper.ts">
<violation number="1" location="apps/meteor/tests/data/permissions.helper.ts:16">
P2: This callback uses short-circuit side effects that call `reject(err)` even on the success path, making Promise resolution logic unclear and brittle. Use an explicit `if (err)` branch so only one settlement path executes.
(Based on your team's feedback about avoiding unnecessary complexity in error handling and keeping control flow simple.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/data/permissions.helper.ts`:
- Line 16: The helper currently resolves immediately in the .end callback (the
line with .end((err?: Error) => (!err && resolve()) || reject(err))); which can
race with backend permission propagation (notifyOnPermissionChangedById is
fire-and-forget). Change the success branch so it does not call resolve()
immediately: wait for permission propagation (either poll the API/event that
reflects permission changes, await a helper like notifyOnPermissionChangedById
confirmation, or add a short retry/poll loop with timeout) and only call
resolve() after propagation is observed; keep reject(err) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 763887eb-b5f5-41db-aed7-1f748f2decdc
📒 Files selected for processing (1)
apps/meteor/tests/data/permissions.helper.ts
📜 Review details
⏰ 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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
🧰 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/tests/data/permissions.helper.ts
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/data/permissions.helper.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/data/permissions.helper.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/data/permissions.helper.ts
| .expect('Content-Type', 'application/json') | ||
| .expect(200) | ||
| .end((err?: Error) => setTimeout(() => (!err && resolve()) || reject(err), 100)); | ||
| .end((err?: Error) => (!err && resolve()) || reject(err)); |
There was a problem hiding this comment.
Reintroduce propagation wait to prevent permission-test race conditions
On Line 16, resolving immediately after the HTTP response can race with backend permission propagation (notifyOnPermissionChangedById is fire-and-forget in the API handler), so assertions that run right after this helper can become flaky.
Suggested fix
- .end((err?: Error) => (!err && resolve()) || reject(err));
+ .end((err?: Error) =>
+ setTimeout(() => {
+ if (err) {
+ reject(err);
+ return;
+ }
+ resolve();
+ }, 100),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .end((err?: Error) => (!err && resolve()) || reject(err)); | |
| .end((err?: Error) => | |
| setTimeout(() => { | |
| if (err) { | |
| reject(err); | |
| return; | |
| } | |
| resolve(); | |
| }, 100), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/tests/data/permissions.helper.ts` at line 16, The helper
currently resolves immediately in the .end callback (the line with .end((err?:
Error) => (!err && resolve()) || reject(err))); which can race with backend
permission propagation (notifyOnPermissionChangedById is fire-and-forget).
Change the success branch so it does not call resolve() immediately: wait for
permission propagation (either poll the API/event that reflects permission
changes, await a helper like notifyOnPermissionChangedById confirmation, or add
a short retry/poll loop with timeout) and only call resolve() after propagation
is observed; keep reject(err) behavior unchanged.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
/jira ARCH-2021 |
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Task: ARCH-2045