refactor(ui-contexts): improve ActionManager and AvatarUrl typing#38626
refactor(ui-contexts): improve ActionManager and AvatarUrl typing#38626TheRazorbill wants to merge 15 commits into
Conversation
- Replace generic `any` types with typed ActionEventListener in IActionManager - Create GetRoomPathAvatar type with explicit function overloads for getRoomPathAvatar - Maintain destructuring pattern for 'busy' event listener ergonomics - Add JSDoc documentation for clarity - All tests pass without breaking changes Improvements: - Remove unnecessary `any` type casts - Add semantic type definitions for event listeners - Improve IDE autocomplete and type inference - Maintain backward compatibility with existing implementations
|
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 |
🦋 Changeset detectedLatest commit: 728da5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughType-only refinements across packages/ui-contexts: ActionManagerContext introduces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors @rocket.chat/ui-contexts type definitions to improve TypeScript safety for the ActionManager event listeners and the room-avatar URL getter.
Changes:
- Introduces
ActionEventListenerto replaceanylistener typings inIActionManagerwhile keeping the"busy"overload. - Replaces
getRoomPathAvatar: (...args: any) => stringwith a new overloadedGetRoomPathAvatartype. - Adds/updates inline JSDoc comments intended to improve IDE autocomplete/documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/ui-contexts/src/AvatarUrlContext.ts |
Adds a new overloaded GetRoomPathAvatar type and applies it to AvatarUrlContextValue. |
packages/ui-contexts/src/ActionManagerContext.ts |
Replaces any listener types with a new ActionEventListener type for on/off. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In `@packages/ui-contexts/src/ActionManagerContext.ts`:
- Line 8: The ActionEventListener type currently returns unknown and doesn't
match the ActionManager implementation signatures; change the type to
ActionEventListener = (data: unknown) => void (or (data: any) => void if you
prefer) and update the ActionManager methods on() and off() to accept listeners
typed as ActionEventListener (or (data: unknown) => void) so the interface and
implementation signatures align (also ensure any overloads such as the 'busy'
event use the same void return type).
🧹 Nitpick comments (3)
packages/ui-contexts/src/AvatarUrlContext.ts (2)
5-7: Remove the JSDoc comment per coding guidelines.As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".Proposed fix
-/** - * Type for room avatar path getter function with multiple overloads - */ type GetRoomPathAvatar = {
8-20: Consider aligning the style withgetUserPathAvatar.
getUserPathAvatar(lines 15–18) defines its overloads inline, whilegetRoomPathAvataruses an extracted type. For consistency, either inlineGetRoomPathAvataror extractgetUserPathAvatarinto a named type as well.packages/ui-contexts/src/ActionManagerContext.ts (1)
5-7: Remove the JSDoc comment per coding guidelines.As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".Proposed fix
-/** - * Type for generic action manager event listener - */ type ActionEventListener = (data: unknown) => unknown;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui-contexts/src/ActionManagerContext.tspackages/ui-contexts/src/AvatarUrlContext.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:
packages/ui-contexts/src/ActionManagerContext.tspackages/ui-contexts/src/AvatarUrlContext.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). (3)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Replace generic `any` types with typed ActionEventListener<T> generic - Make ActionEventListener return void (idiomatic for event handlers) - Type listener parameter with UiKit.ServerInteraction for improved type safety - Export GetRoomPathAvatar type for reuse across package - Add function overload support to GetRoomPathAvatar for flexibility - Maintain destructuring pattern for 'busy' event listener ergonomics - All tests pass without breaking changes Improvements: - Better type inference at callsites via generic parameter - Generic type parameter supports future extensions - Exported types prevent any leakage in dependent code - Improved JSDoc documentation for clarity - Supports multiple ways to call getRoomPathAvatar
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui-contexts/src/AvatarUrlContext.ts`:
- Line 9: GetRoomPathAvatar's params are all optional and mismatch actual
callers (RoomAvatar.tsx) while useRoomAvatarPath returns loose any, so tighten
the API: change the GetRoomPathAvatar type to require either roomId or roomName
(e.g., a discriminated union like ({ roomId: string } | { roomName: string }) &
{ etag?: string; type?: string }), or provide overloads that mirror
getUserPathAvatar to support both positional and object styles; rename or map
param keys to match caller fields (_id, t, avatarETag) or adapt callers to the
declared keys; and update useRoomAvatarPath's return signature from (...args:
any) => string to the new GetRoomPathAvatar type so callers are properly
type-checked (references: GetRoomPathAvatar, useRoomAvatarPath, RoomAvatar.tsx,
getUserPathAvatar).
🧹 Nitpick comments (1)
packages/ui-contexts/src/AvatarUrlContext.ts (1)
5-8: Remove the JSDoc comment per coding guidelines.As per coding guidelines,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation."Proposed fix
-/** - * Type for room avatar path getter function. - * The provider implementation expects a single parameter object. - */ type GetRoomPathAvatar = (params: { roomId?: string; roomName?: string; etag?: string; type?: string }) => string;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui-contexts/src/AvatarUrlContext.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:
packages/ui-contexts/src/AvatarUrlContext.ts
🧠 Learnings (2)
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
packages/ui-contexts/src/AvatarUrlContext.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
packages/ui-contexts/src/AvatarUrlContext.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)
packages/ui-contexts/src/AvatarUrlContext.ts (1)
17-17: Type integration looks good.Replacing the permissive
(...args: any) => stringwith a named type improves discoverability and type safety — contingent on the type definition concerns raised above being addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![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 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/ui-contexts/src/ActionManagerContext.ts">
<violation number="1" location="packages/ui-contexts/src/ActionManagerContext.ts:16">
P2: Overload order causes on('busy', ...) to match the generic string signature first, breaking contextual typing for the busy event listener.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tarPath typing - Update useRoomAvatarPath to return AvatarUrlContextValue['getRoomPathAvatar'] instead of (...args: any) => string - Export GetRoomPathAvatar type for external consumers - Aligns with useUserAvatarPath pattern for consistency
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lementation
- Change from overloaded signatures to single function signature accepting room object
- Matches actual AvatarUrlProvider implementation which expects { _id, type?, t?, avatarETag?, ... }
- Addresses Copilot feedback about type mismatch with provider implementation
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/ui-contexts/src/AvatarUrlContext.ts">
<violation number="1" location="packages/ui-contexts/src/AvatarUrlContext.ts:9">
P2: Exported GetRoomPathAvatar type was narrowed to only accept a room object, removing previously supported overloads (roomId string or params). This is a breaking API change for consumers still using the old signatures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
TheRazorbill
force-pushed
the
improve/ui-contexts-typing
branch
from
1f3246f to
19c3f86
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38626 +/- ##
===========================================
+ Coverage 70.67% 70.68% +0.01%
===========================================
Files 3191 3191
Lines 112963 112963
Branches 20451 20480 +29
===========================================
+ Hits 79832 79853 +21
+ Misses 31085 31064 -21
Partials 2046 2046
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including screenshots)
This PR improves TypeScript type safety in
ui-contextsby refining the typings of the ActionManager and Avatar URL contexts.Changes included:
anylistener types inIActionManagerwith a semanticActionEventListener"busy"event listenerGetRoomPathAvatartype with explicit overloads forgetRoomPathAvatarThese updates are purely type-level improvements with no runtime or behavioral impact, maintaining backward compatibility with existing implementations.
Before / After
Issue(s)
Closes: #38826
Steps to test or reproduce
Run TypeScript checks locally and ensure the project builds successfully with no type errors: