refactor(ui-client): improve Callbacks class typing v2#38627
refactor(ui-client): improve Callbacks class typing v2#38627TheRazorbill wants to merge 4 commits into
Conversation
|
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: 89fc991 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:
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. 📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
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.
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.
|
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! |
TheRazorbill
force-pushed
the
improve/ui-client-callbacks-typing
branch
from
1bc6538 to
b4d3417
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add semantic callback signature types (EventLikeCallbackSignature returns void, ChainedCallbackSignature returns any) enforcing structural distinction - Replace loose index-signature generic constraints with Record<string, ...> - Add Awaited<> wrapper on run() return types for correct async/sync handling - Restore function-type generic overload on Callbacks.create for backward compat - Remove redundant 'as any' casts in static create method, use typed callbacks - Make Cb.add constant parameter optional for flexibility - Add ActionEventListener<T> type replacing inline (data: any) listeners - Add structured GetRoomPathAvatar type matching actual implementation - Narrow useRoomAvatarPath return type to GetRoomPathAvatar - Add changeset for both ui-client and ui-contexts packages
TheRazorbill
force-pushed
the
improve/ui-client-callbacks-typing
branch
from
b4d3417 to
0fd796f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38627 +/- ##
===========================================
- Coverage 70.67% 70.65% -0.02%
===========================================
Files 3191 3191
Lines 112963 112965 +2
Branches 20451 20488 +37
===========================================
- Hits 79832 79814 -18
- Misses 31085 31098 +13
- Partials 2046 2053 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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)
Closes: #38827
Steps to test or reproduce
Example: