refactor(ui-client): improve Callbacks class typing v2 #38627
Conversation
- Replace generic constraints `[key: string]: (item: any, constant?: any) => any` with `Record<string, ...Signature>` types - Add semantic callback signature types: EventLikeCallbackSignature and ChainedCallbackSignature - Reduce unnecessary `any` type casts in static create method - Make callback constant parameter optional for better flexibility - Improve JSDoc comments for better type documentation - All existing tests pass without breaking changes This improves type safety while maintaining backward compatibility with existing callback 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 |
|
|
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:
WalkthroughTightens TypeScript typings across UI packages: refactors Callbacks signatures and generics, introduces a typed ActionEventListener for action manager listeners, and adds a structured GetRoomPathAvatar type (propagated to the useRoomAvatarPath hook); also adds a changeset entry documenting the callbacks typing change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
- 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 - Improve type specificity with explicit function overloads - Maintain destructuring pattern for 'busy' event listener ergonomics - All tests pass without breaking changes Improvements: - Better type inference at callsites - Generic type parameter supports future extensions - Exported types prevent any leakage in dependent code - JSDoc documentation for clarity
There was a problem hiding this comment.
Pull request overview
This PR refactors packages/ui-client’s legacy Callbacks utility to improve TypeScript typing by introducing semantic callback signature aliases and tightening generic constraints.
Changes:
- Introduces
EventLikeCallbackSignature/ChainedCallbackSignaturehelper types and uses them inCallbacksgeneric constraints. - Refactors
Callbacks.createfactory typing and reducesanycasting. - Makes the “constant” callback parameter optional in the deprecated
Cbhelper type and adds a changeset.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/ui-client/src/lib/callbacks/Callbacks.ts | Adds new signature types, changes generic constraints, and refactors Callbacks.create/Cb typing. |
| .changeset/improve-callbacks-typing.md | Declares a patch release note for the typing refactor. |
💡 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: 2
🤖 Fix all issues with AI agents
In `@packages/ui-client/src/lib/callbacks/Callbacks.ts`:
- Around line 236-241: Remove the redundant no-op cast by deleting the typedHook
variable and use the existing hook parameter directly; update the return object
so add/remove/run call callbacks.add(hook,...), callbacks.remove(hook,...), and
callbacks.run(hook,...) instead of referencing typedHook, ensuring you only
change the alias usage and not the callbacks methods themselves.
- Around line 16-26: Change EventLikeCallbackSignature to return void (not "void
| unknown") so it is distinct from ChainedCallbackSignature; update the type
definition named EventLikeCallbackSignature in Callbacks.ts to enforce void and
keep ChainedCallbackSignature as returning unknown, and update any related uses
(e.g., event callbacks like afterCreateChannel, afterSaveMessage, usernameSet in
clientCallbacks.ts) if necessary to satisfy the stricter void return. Also
remove the redundant cast "const typedHook = hook as string" and use the
existing hook parameter directly (the parameter named hook is already a string),
eliminating the unused/duplicative typedHook variable.
🧹 Nitpick comments (1)
packages/ui-client/src/lib/callbacks/Callbacks.ts (1)
16-19: JSDoc comments added on type declarations.The coding guideline for
**/*.{ts,tsx,js}states: "Avoid code comments in the implementation." The file already has JSDoc comments on methods (lines 125–132, 205–212, etc.), so this is consistent with the existing pattern, but these new comments are additions in this PR.As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".Also applies to: 22-25
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/improve-callbacks-typing.mdpackages/ui-client/src/lib/callbacks/Callbacks.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-client/src/lib/callbacks/Callbacks.ts
🧬 Code graph analysis (1)
packages/ui-client/src/lib/callbacks/Callbacks.ts (2)
apps/meteor/server/lib/callbacks.ts (1)
callbacks(218-226)packages/livechat/src/widget.ts (1)
callback(509-511)
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
🔇 Additional comments (3)
packages/ui-client/src/lib/callbacks/Callbacks.ts (2)
40-43: LGTM!The updated generic constraints using
Record<string, ChainedCallbackSignature>andRecord<string, EventLikeCallbackSignature>are cleaner than the previous index-signature approach and compatible with the existing usage inapps/meteor/server/lib/callbacks.ts.
250-257: LGTM!Making
constantoptional in theCbtype is consistent with how it's treated throughout the class (e.g., line 29, line 149, line 203) and aligns with the runtime behavior where the constant is frequently omitted..changeset/improve-callbacks-typing.md (1)
1-5: LGTM!Changeset correctly identifies this as a
patch— appropriate for a non-breaking typing improvement.
✏️ 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
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-client/src/lib/callbacks/Callbacks.ts">
<violation number="1" location="packages/ui-client/src/lib/callbacks/Callbacks.ts:20">
P2: The return type `void | unknown` simplifies to `unknown` in TypeScript (since `unknown` is the top type), making `EventLikeCallbackSignature` structurally identical to `ChainedCallbackSignature`. This defeats the semantic distinction between side-effect callbacks and value-transforming callbacks. Consider using just `void` to enforce the intended behavior for event-like callbacks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ibility - Change EventLikeCallbackSignature to return only void (not void | unknown) - Replace unknown with any in callback parameters for strictFunctionTypes compatibility - Update Callback<H> type to use any instead of Promise<unknown> for consistency - Add overload to create() method for backward compatibility with legacy code - Fix Cb.add return type to () => void to match actual implementation - Remove unnecessary type cast in create() method
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
I've opened this second PR to refine the Callbacks utility typing in @rocket.chat/ui-client. My goal was to replace the loose any constraints with stricter semantic signatures (EventLike and Chained) to prevent future typing regressions. I have already: Addressed the feedback from the AI reviewers regarding return types (void vs unknown). Thanks! |
2 participants
Proposed changes
This PR improves TypeScript type safety in the
Callbacksutility by:[key: string]: (item: any, constant?: any) => any)with stricter
Record<string, ...Signature>typesEventLikeCallbackSignature(side-effect callbacks)ChainedCallbackSignature(value-transforming callbacks)anycasts in the staticcreatemethodThese changes improve maintainability and typing accuracy while preserving backward compatibility and runtime behavior.
Proposed changes (including screenshots)
Before / After
Issue(s)
N/A (typing improvement / refactor)
Steps to test or reproduce
Example: