refactor: async room get #387
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the implementation of asynchronous operations within the Ably Chat SDK. Key modifications include updating the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AndyTWF
force-pushed
the
async-room-get
branch
from
512f93c to
8104c12
Compare
AndyTWF
force-pushed
the
async-room-get
branch
from
8104c12 to
ed28566
Compare
AndyTWF
force-pushed
the
async-room-get
branch
from
ed28566 to
10fba9d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (60)
test/helper/wait-for-eventual-hook.ts (1)
15-26: 🛠️ Refactor suggestion
Enhance value comparison and error handling.
The implementation has good type safety but could be improved in several ways:
- Make the timeout configurable
- Handle undefined values from the getter function more gracefully
- Consider using deep equality for object comparisons
- Consider reusing common logic between the two functions
Here's a suggested improvement:
+interface WaitOptions { + timeout?: number; + useDeepEqual?: boolean; +} + export const waitForEventualHookValue = async <HookReturn, Value>( result: { current: HookReturn }, expected: Value, getValue: (current: HookReturn) => Value | undefined, + options?: WaitOptions, ): Promise<void> => { return vi.waitFor( () => { - expect(getValue(result.current)).toBe(expected); + const value = getValue(result.current); + if (value === undefined) { + throw new Error('Value is undefined'); + } + if (options?.useDeepEqual) { + expect(value).toEqual(expected); + } else { + expect(value).toBe(expected); + } }, - { timeout: 3000 }, + { + timeout: options?.timeout ?? 3000, + message: `Hook value did not match expected value within ${ + options?.timeout ?? 3000 + }ms`, + }, ); };📝 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.interface WaitOptions { timeout?: number; useDeepEqual?: boolean; } export const waitForEventualHookValue = async <HookReturn, Value>( result: { current: HookReturn }, expected: Value, getValue: (current: HookReturn) => Value | undefined, options?: WaitOptions, ): Promise<void> => { return vi.waitFor( () => { const value = getValue(result.current); if (value === undefined) { throw new Error('Value is undefined'); } if (options?.useDeepEqual) { expect(value).toEqual(expected); } else { expect(value).toBe(expected); } }, { timeout: options?.timeout ?? 3000, message: `Hook value did not match expected value within ${ options?.timeout ?? 3000 }ms`, }, ); };test/react/providers/chat-room-provider.integration.test.tsx (2)
22-22: 🛠️ Refactor suggestion
Improve type safety for chat client creation.
The current type assertion using
unknownis unsafe. Consider creating a properly typed mock or helper function.- const chatClient = newChatClient() as unknown as ChatClient; + const chatClient = newChatClient();Also, consider updating the
newChatClienthelper function to return the correct type.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 22-22: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
44-50: 🛠️ Refactor suggestion
Enhance test robustness and error handling.
The current test could be improved in several ways:
- Add error case testing
- Use dynamic timeout based on environment
- Add more assertions for room state
const room = await chatClient.rooms.get(roomId, { reactions: RoomOptionsDefaults.reactions }); + expect(room).toBeDefined(); + await vi.waitFor( () => { expect(room.status).toBe(RoomStatus.Attached); + expect(room.id).toBe(roomId); }, - { timeout: 5000 }, + { timeout: process.env.CI ? 10000 : 5000 }, ); + + // Test error handling + await expect( + chatClient.rooms.get('invalid-room', { reactions: RoomOptionsDefaults.reactions }) + ).rejects.toThrow();📝 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.const room = await chatClient.rooms.get(roomId, { reactions: RoomOptionsDefaults.reactions }); expect(room).toBeDefined(); await vi.waitFor( () => { expect(room.status).toBe(RoomStatus.Attached); expect(room.id).toBe(roomId); }, { timeout: process.env.CI ? 10000 : 5000 }, ); // Test error handling await expect( chatClient.rooms.get('invalid-room', { reactions: RoomOptionsDefaults.reactions }) ).rejects.toThrow();🧰 Tools
🪛 eslint
[error] 44-44: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 44-44: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 44-44: Unsafe member access .rooms on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 44-44: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 44-44: Unsafe member access .reactions on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 47-47: Unsafe member access .status on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 47-47: Unsafe member access .Attached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/react/helper/use-room-context.test.tsx (1)
10-12:
⚠️ Potential issueImprove type safety in the helper function
The double type assertion through
unknownis a TypeScript anti-pattern that bypasses type checking. Consider implementing proper type guards or fixing the underlying type mismatch.-function newChatClient() { - return newChatClientLib() as unknown as ChatClient; +function newChatClient(): ChatClient { + const client = newChatClientLib(); + if (!(client instanceof ChatClient)) { + throw new Error('Invalid chat client instance'); + } + return client;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 11-11: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
test/helper/room.ts (1)
49-49: 💡 Codebase verification
Documentation is missing for the
nonceparameter in DefaultRoom constructorThe constructor's JSDoc is incomplete as it doesn't document the required
nonceparameter, which is the second parameter being passed asrandomId(). The documentation only coversrealtime,chatApi,logger, andinitAfterparameters.
src/core/room.ts: Add documentation for thenonceparameter in the DefaultRoom constructor's JSDoc.🔗 Analysis chain
Verify DefaultRoom constructor documentation for the new parameter.
The addition of
randomId()as a second parameter to the DefaultRoom constructor suggests a change in how rooms are identified.
Let me gather more information about the DefaultRoom constructor documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify DefaultRoom constructor documentation and implementation # Test 1: Check DefaultRoom constructor documentation echo "Checking DefaultRoom constructor documentation..." rg "class DefaultRoom" -A 10 -g "src/core/**/*.ts" # Test 2: Check for other instances of DefaultRoom instantiation echo "Checking other DefaultRoom instantiations..." ast-grep --pattern 'new DefaultRoom($$$)'Length of output: 3459
Script:
#!/bin/bash # Check the DefaultRoom constructor documentation and implementation rg "constructor" -B 5 -A 15 src/core/room.tsLength of output: 693
test/core/rooms.integration.test.ts (1)
41-44:
⚠️ Potential issueImprove type safety for Ably channel assertion
The current type assertion triggers a TypeScript error and could be fragile.
Consider refactoring to:
- const channelFailable = room.messages.channel as Ably.RealtimeChannel & { - notifyState(state: 'failed'): void; - }; + interface FallibleChannel extends Ably.RealtimeChannel { + notifyState(state: 'failed'): void; + } + const channelFailable = room.messages.channel as FallibleChannel;📝 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.interface FallibleChannel extends Ably.RealtimeChannel { notifyState(state: 'failed'): void; } const channelFailable = room.messages.channel as FallibleChannel; channelFailable.notifyState('failed');🧰 Tools
🪛 eslint
[error] 41-43: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 41-41: 'RealtimeChannel' is an 'error' type that acts as 'any' and overrides all other types in this intersection type.
(@typescript-eslint/no-redundant-type-constituents)
[error] 44-44: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 44-44: Unsafe member access .notifyState on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/react/helper/room-promise.test.ts (1)
49-94: 🛠️ Refactor suggestion
Improve test reliability and performance
The test could be made more reliable and efficient with the following improvements:
- Replace the manual waiting loops with a more efficient approach:
- // Wait for 5 intervals of 150ms to confirm the callback was never called - for (let i = 0; i < 5; i++) { - await new Promise((resolve) => setTimeout(resolve, 150)); - } + const VERIFICATION_DELAY = 750; // Same total time but single wait + await new Promise((resolve) => setTimeout(resolve, VERIFICATION_DELAY));
- Add timeout protection:
+ it('should mount and unmount before promise resolution', async () => { + vi.setTimeout(2000); // Protect against hanging tests
- Extract duplicate waiting logic into a helper function:
const verifyNoCallbacks = async (delay: number) => { await new Promise((resolve) => setTimeout(resolve, delay)); };test/core/room.integration.test.ts (1)
27-40: 🛠️ Refactor suggestion
Address type safety for channel access
The TypeScript compiler is flagging unsafe assignments and member access for channel operations. Consider adding type assertions or proper type definitions to ensure type safety.
- const messagesChannel = room.messages.channel; + const messagesChannel = room.messages.channel as Ably.RealtimeChannel;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 27-27: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 28-28: Unsafe member access .state on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 30-30: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 31-31: Unsafe member access .state on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 33-33: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 34-34: Unsafe member access .state on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 36-36: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 37-37: Unsafe member access .state on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 39-39: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 40-40: Unsafe member access .state on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/react/helper/use-eventual-room.test.tsx (2)
38-73: 🛠️ Refactor suggestion
Add test cases for error handling and timeouts
The current test suite covers the happy path well, but consider adding:
- Error handling test cases
- Timeout scenarios
- Edge cases when the room promise is rejected
Example test case to add:
it('handles room promise rejection', async () => { mockRoomContext = { room: Promise.reject(new Error('Failed to get room')) }; const { result } = renderHook(() => useEventualRoom()); expect(result.current).toBeUndefined(); await expect(async () => { await vi.waitFor(() => { expect(result.current).toBeDefined(); }); }).rejects.toThrow('Failed to get room'); });🧰 Tools
🪛 eslint
[error] 40-40: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 40-40: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 40-40: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 43-43: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 47-47: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 52-52: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 52-52: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 52-52: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 55-55: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 59-59: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 63-63: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 66-66: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 70-70: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
9-27: 🛠️ Refactor suggestion
Add type annotations to improve type safety
To address the TypeScript unsafe type warnings and improve code maintainability:
-let mockRoom: Room; -let mockRoomContext: { room: Promise<Room> }; -let mockLogger: Logger; +let mockRoom: Room | undefined; +let mockRoomContext: { room: Promise<Room | undefined> }; +let mockLogger: Logger | undefined; -const updateMockRoom = (newRoom: Room) => { +const updateMockRoom = (newRoom: Room): void => { mockRoom = newRoom; mockRoomContext = { room: Promise.resolve(newRoom) }; };📝 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.let mockRoom: Room | undefined; let mockRoomContext: { room: Promise<Room | undefined> }; let mockLogger: Logger | undefined; vi.mock('../../../src/react/helper/use-room-context.js', () => ({ useRoomContext: () => mockRoomContext, })); vi.mock('../../../src/react/hooks/use-logger.js', () => ({ useLogger: () => mockLogger, })); vi.mock('ably'); const updateMockRoom = (newRoom: Room): void => { mockRoom = newRoom; mockRoomContext = { room: Promise.resolve(newRoom) }; };🧰 Tools
🪛 eslint
[error] 18-18: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 24-24: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
package.json (1)
37-37: 💡 Codebase verification
Demo setup requires additional steps before running build:start-demo
The demo's package.json and README reveal that the script needs prerequisite steps:
- Build the main SDK from root directory first:
(cd .. && npm run build)- Install demo dependencies:
npm install- Set up environment variables by copying
.env.exampleto.envThe current
build:start-demoscript should be run only after these steps are completed. Sincenode_modulesis missing, the script will fail.🔗 Analysis chain
LGTM! Verify demo dependencies.
The new script provides a convenient way to test the async room.get() changes. However, ensure that the demo dependencies are installed before running this script.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify demo dependencies are installed # Test: Check if package.json and node_modules exist in demo directory if [ ! -f "demo/package.json" ]; then echo "Warning: demo/package.json not found" exit 1 fi if [ ! -d "demo/node_modules" ]; then echo "Warning: demo/node_modules not found" exit 1 fiLength of output: 161
Script:
#!/bin/bash # Let's verify if demo directory exists and check its package.json content if [ -d "demo" ]; then echo "Demo directory exists" if [ -f "demo/package.json" ]; then echo "demo/package.json content:" cat demo/package.json else echo "demo/package.json not found" fi else echo "Demo directory not found" fi # Also check if there's any documentation about demo setup if [ -f "demo/README.md" ]; then echo -e "\nDemo README content:" cat demo/README.md fiLength of output: 2612
test/react/hooks/use-typing.integration.test.tsx (1)
39-39:
⚠️ Potential issueFix type safety issues with chat client casting.
The current type casting approach using
as unknown as ChatClientleads to unsafe operations. Consider creating a properly typed test helper.- const chatClientOne = newChatClient() as unknown as ChatClient; - const chatClientTwo = newChatClient() as unknown as ChatClient; + const chatClientOne = await createTypedTestClient(); + const chatClientTwo = await createTypedTestClient(); + // Add to test/helper/chat.ts: + export async function createTypedTestClient(): Promise<ChatClient> { + const client = newChatClient(); + // Add necessary type assertions and validations here + return client as ChatClient; + }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 39-39: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 39-39: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 39-39: Unsafe member access .rooms on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/react/providers/room-provider.test.tsx (1)
174-175: 🛠️ Refactor suggestion
Improve release cleanup handling.
The
voidoperator usage for cleanup is not ideal, and the release operation should be properly awaited.Consider this improvement:
-void chatClient.rooms.release(roomId); +await chatClient.rooms.release(roomId); + +// Verify the room was actually released +await vi.waitFor(() => { + expect(chatClient.rooms.has(roomId)).toBeFalsy(); +});Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 175-175: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 175-175: Unsafe member access .rooms on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/react/helper/use-room-status.test.tsx (1)
96-148: 🛠️ Refactor suggestion
Improve type safety in listener test.
The test uses type assertions that could be made more type-safe.
- expect((mockRoom as unknown as { _lifecycle: { listeners(): unknown[] } })._lifecycle.listeners()).toHaveLength(2); + expect((mockRoom._lifecycle.listeners() as Array<(change: RoomStatusChange) => void>)).toHaveLength(2); - expect((mockRoom as unknown as { _lifecycle: { listeners(): unknown[] } })._lifecycle.listeners()).toBeNull(); + expect(mockRoom._lifecycle.listeners()).toBeNull();Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 100-100: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 100-100: Unsafe construction of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 100-100: Unsafe member access .ErrorInfo on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 102-102: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 102-102: Unsafe member access .Failed on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 103-103: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 107-113: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 107-107: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 124-124: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 124-124: Unsafe member access .Failed on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 125-125: Unsafe member access .previous on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 125-125: Unsafe member access .Initializing on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 126-126: Unsafe member access .error on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 129-129: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 129-129: Unsafe construction of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 129-129: Unsafe member access .ErrorInfo on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 131-131: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 131-131: Unsafe member access .Detached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 132-132: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 141-141: Unsafe member access .current on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 141-141: Unsafe member access .Detached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 142-142: Unsafe member access .previous on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 142-142: Unsafe member access .Failed on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 143-143: Unsafe member access .error on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 146-146: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
src/core/occupancy.ts (1)
114-118: 🛠️ Refactor suggestion
Add type safety to channel initialization.
The static analysis tool flagged a potential type safety issue. Consider adding explicit type annotations to ensure type safety during channel initialization.
- private readonly _channel: Ably.RealtimeChannel; + private readonly _channel: Ably.Types.RealtimeChannelPromise; constructor(roomId: string, realtime: Ably.Realtime, chatApi: ChatApi, logger: Logger) { super(); this._roomId = roomId; - this._channel = this._makeChannel(roomId, realtime); + this._channel = this._makeChannel(roomId, realtime) as Ably.Types.RealtimeChannelPromise;📝 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.private readonly _channel: Ably.Types.RealtimeChannelPromise; constructor(roomId: string, realtime: Ably.Realtime, chatApi: ChatApi, logger: Logger) { super(); this._roomId = roomId; this._channel = this._makeChannel(roomId, realtime) as Ably.Types.RealtimeChannelPromise;🧰 Tools
🪛 eslint
[error] 118-118: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
test/react/hooks/use-occupancy.test.tsx (1)
19-19: 🛠️ Refactor suggestion
Add type annotations to improve type safety.
The mock setup looks good but needs type annotations to address TypeScript safety concerns.
-let mockRoomContext: { room: Promise<Room> }; +let mockRoomContext: { room: Promise<Room> } = { room: Promise.resolve({} as Room) }; vi.mock('../../../src/react/helper/use-room-context.js', () => ({ - useRoomContext: () => mockRoomContext, + useRoomContext: (): { room: Promise<Room> } => mockRoomContext, })); vi.mock('../../../src/react/helper/use-room-status.js', () => ({ - useRoomStatus: () => ({ status: RoomStatus.Attached }), + useRoomStatus: (): { status: RoomStatus } => ({ status: RoomStatus.Attached }), }));Also applies to: 26-31
demo/src/containers/Chat/Chat.tsx (1)
114-125: 🛠️ Refactor suggestion
Consider improving room ID input UX and validation.
While the implementation is functional, consider these improvements:
- Replace the native
prompt()with a modal dialog or inline form for better UX- Add validation for the room ID format/content
- Consider adding a confirmation step for room changes
Example implementation:
function changeRoomId() { // Validate room ID format const isValidRoomId = (id: string) => /^[a-zA-Z0-9-_]+$/.test(id); // Replace with your modal/form implementation const newRoomId = prompt('Enter your new roomId'); if (!newRoomId) return; if (!isValidRoomId(newRoomId)) { chatClient.logger.error('Invalid room ID format'); return; } if (window.confirm('Are you sure you want to change rooms? This will clear your current messages.')) { setMessages([]); setLoading(true); setRoomReactions([]); props.setRoomId(newRoomId); } }src/core/room-reactions.ts (2)
197-197:
⚠️ Potential issueFix type safety issues in publish operation.
Multiple TypeScript warnings about unsafe operations. Consider adding proper type guards and error handling.
- return this._channel.publish(realtimeMessage); + if (!this._channel || typeof this._channel.publish !== 'function') { + return Promise.reject(new Error('Invalid channel state')); + } + return this._channel.publish(realtimeMessage);📝 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.if (!this._channel || typeof this._channel.publish !== 'function') { return Promise.reject(new Error('Invalid channel state')); } return this._channel.publish(realtimeMessage);🧰 Tools
🪛 eslint
[error] 197-197: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 197-197: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 197-197: Unsafe member access .publish on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
109-109: 💡 Codebase verification
Breaking change needs documentation
The change from
Promise<Channel>to directChannelreturn type is a breaking change that requires documentation. While the CHANGELOG.md mentions related changes about room initialization and channel behavior, it doesn't explicitly document this API change. Please:
- Add an entry in CHANGELOG.md under version 0.2.0 about the channel getter return type change
- Document the rationale behind removing the Promise wrapper
- Update any relevant API documentation
The ripgrep results show this pattern is changed across multiple files (
messages.ts,presence.ts,typing.ts, etc.), confirming this is a significant architectural change.🔗 Analysis chain
Verify documentation for breaking API change.
The change from
Promise<Channel>to directChannelreturn type is a breaking change that affects API consumers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the breaking change is documented in changelog/docs rg -i "breaking.*change.*channel" docs/ rg -i "breaking.*change.*channel" CHANGELOG.mdLength of output: 136
Script:
#!/bin/bash # Let's first check if there are any documentation or changelog files in the repository fd -t f -e md # Also check for any mentions of channel type changes in markdown files rg -i "channel.*type" -g "*.md" # Look for any migration guides or breaking changes documentation rg -i "migration|breaking.changes" -g "*.md"Length of output: 185
Script:
#!/bin/bash # Let's check the content of CHANGELOG.md for any recent changes cat CHANGELOG.md # Also check if there are any other files that might reference this channel type change rg -i "promise.*channel|async.*channel" # Look for any interface implementations to understand the impact ast-grep --pattern 'implements RoomReactions'Length of output: 3812
src/core/typing.ts (3)
167-167:
⚠️ Potential issueImprove type safety and error handling in channel operations
The methods using
this._channel.presencehave multiple type safety concerns:
- No null checks before accessing presence
- Unsafe type assertions in presence operations
Add type guards and proper error handling:
get(): Promise<Set<string>> { + if (!this._channel?.presence) { + return Promise.reject(new Error('Channel or presence not available')); + } return this._channel.presence.get().then((members) => new Set<string>(members.map((m) => m.clientId))); } async start(): Promise<void> { + if (!this._channel?.presence) { + throw new Error('Channel or presence not available'); + } // ... existing timer logic ... return this._channel.presence.enterClient(this._clientId).then(); } async stop(): Promise<void> { + if (!this._channel?.presence) { + throw new Error('Channel or presence not available'); + } // ... existing timer logic ... return this._channel.presence.leaveClient(this._clientId); }Also applies to: 203-203, 218-218
🧰 Tools
🪛 eslint
[error] 167-167: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 167-167: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 167-167: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 167-167: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 167-167: Unsafe member access .then on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 167-167: Unsafe argument of type
anyassigned to a parameter of typeIterable<string> | null | undefined.(@typescript-eslint/no-unsafe-argument)
[error] 167-167: Unsafe call of a(n)
anytyped value.(@typescript-eslint/no-unsafe-call)
[error] 167-167: Unsafe member access .map on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
[error] 167-167: Unsafe return of a value of type
any.(@typescript-eslint/no-unsafe-return)
[error] 167-167: Unsafe member access .clientId on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
173-175: 🛠️ Refactor suggestion
Add null check in channel getter
The getter should handle cases where channel initialization failed.
get channel(): Ably.RealtimeChannel { + if (!this._channel) { + throw new Error('Typing channel not initialized'); + } return this._channel; }📝 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.get channel(): Ably.RealtimeChannel { if (!this._channel) { throw new Error('Typing channel not initialized'); } return this._channel; }🧰 Tools
🪛 eslint
[error] 174-174: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
141-144:
⚠️ Potential issueAdd error handling for channel initialization
The direct channel initialization could fail, but there's no visible error handling. Consider:
- Adding try-catch around
_makeChannel- Implementing proper error propagation
constructor(roomId: string, options: TypingOptions, realtime: Ably.Realtime, clientId: string, logger: Logger) { super(); this._clientId = clientId; - this._channel = this._makeChannel(roomId, realtime); + try { + this._channel = this._makeChannel(roomId, realtime); + } catch (error) { + this._logger.error('Failed to initialize typing channel', { error }); + throw new Error(`Failed to initialize typing channel: ${error.message}`); + }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 144-144: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
test/core/room.test.ts (1)
200-212:
⚠️ Potential issueStore channel names before release for safer assertions
Accessing channel properties after release could be unsafe. Consider storing the channel names before releasing:
- await room.release(); - - const messagesChannel = room.messages.channel; - expect(context.realtime.channels.release).toHaveBeenCalledWith(messagesChannel.name); + // Store channel names before release + const channelNames = { + messages: room.messages.channel.name, + presence: room.presence.channel.name, + typing: room.typing.channel.name, + reactions: room.reactions.channel.name, + occupancy: room.occupancy.channel.name, + }; + + await room.release(); + + // Assert using stored names + expect(context.realtime.channels.release).toHaveBeenCalledWith(channelNames.messages);This approach is safer and addresses the static analysis warnings about unsafe member access.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 200-200: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 201-201: Unsafe member access .channels on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 201-201: Unsafe member access .name on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 203-203: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 204-204: Unsafe member access .channels on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 204-204: Unsafe member access .name on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 206-206: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 207-207: Unsafe member access .channels on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 207-207: Unsafe member access .name on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 209-209: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 210-210: Unsafe member access .channels on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 210-210: Unsafe member access .name on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 212-212: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
test/react/hooks/use-messages.integration.test.tsx (1)
39-39:
⚠️ Potential issueAddress type safety concerns in ChatClient assertions
There are multiple unsafe type assertions using
as unknown as ChatClient. This pattern appears across all test cases and could mask potential runtime issues.Consider these improvements:
- Update the
newChatClient()helper to return the correct type- If type assertion is necessary, use a type guard instead:
function isChatClient(client: unknown): client is ChatClient { return client !== null && typeof client === 'object' && 'rooms' in client && typeof (client as any).rooms.get === 'function'; } const chatClient = newChatClient(); if (!isChatClient(chatClient)) { throw new Error('Invalid chat client'); }Also applies to: 83-83, 138-138, 201-201, 335-335
🧰 Tools
🪛 eslint
[error] 39-39: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 39-39: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 39-39: Unsafe member access .rooms on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/core/messages.test.ts (2)
396-396: 🛠️ Refactor suggestion
Consistent type casting needed across channel access points.
Multiple instances of unsafe channel property access are present throughout the test file. This pattern is repeated across different test cases and could lead to runtime type errors.
Consider creating a helper function to handle the channel type casting:
function getTypedChannel(messages: Messages): RealtimeChannel & { properties: { attachSerial?: string; channelSerial?: string; fromSerial?: string; }; } { return messages.channel as RealtimeChannel & { properties: { attachSerial?: string; channelSerial?: string; fromSerial?: string; }; }; }Then use it consistently across all test cases:
- const msgChannel = room.messages.channel; - const channel = msgChannel as RealtimeChannel & { - properties: { - attachSerial: string | undefined; - channelSerial: string | undefined; - }; - }; + const channel = getTypedChannel(room.messages);Also applies to: 456-456, 493-493, 590-590, 678-678, 781-781
🧰 Tools
🪛 eslint
[error] 396-396: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
49-53:
⚠️ Potential issueType safety improvement needed in test setup.
The test setup contains multiple unsafe type assignments when accessing the channel property. Consider adding proper type assertions or type guards to ensure type safety.
Apply this diff to improve type safety:
- const channel = context.room.messages.channel; + const channel = context.room.messages.channel as Ably.RealtimeChannel;📝 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.beforeEach<TestContext>((context) => { context.realtime = new Ably.Realtime({ clientId: 'clientId', key: 'key' }); context.chatApi = new ChatApi(context.realtime, makeTestLogger()); context.room = makeRandomRoom({ chatApi: context.chatApi, realtime: context.realtime }); const channel = context.room.messages.channel as Ably.RealtimeChannel;🧰 Tools
🪛 eslint
[error] 50-50: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 50-50: Unsafe construction of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 50-50: Unsafe member access .Realtime on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 52-52: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 53-53: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
src/react/hooks/use-room.ts (4)
62-63:
⚠️ Potential issueType 'context' to avoid unsafe assignments
The
contextobject retrieved fromuseRoomContext('useRoom')may have an implicitanytype, leading to unsafe property access and assignments likecontext.roomId.To resolve this, ensure that
useRoomContextreturns a properly typed context. You can explicitly typecontextas follows:- const context = useRoomContext('useRoom'); + const context = useRoomContext('useRoom') as RoomContextType;Make sure to import or define
RoomContextTypeappropriately to match the shape of the context object.Committable suggestion skipped: line range outside the PR's diff.
81-81:
⚠️ Potential issueEnsure 'useEventualRoom()' returns a properly typed value
The
roomproperty assigned fromuseEventualRoom()may lack proper typing, causing unsafe assignments.Verify that
useEventualRoom()returns a value of typeRoom | undefined. Update its return type if necessary to match the expected type.- room: useEventualRoom(), + room: useEventualRoom() as Room | undefined,Ensure that all uses of
roomaccount for the possibility of it being undefined.📝 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.room: useEventualRoom() as Room | undefined,🧰 Tools
🪛 eslint
[error] 81-81: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
76-77:
⚠️ Potential issueProperly type 'context.room' to avoid unsafe calls
In the
attachanddetachfunctions,context.roommay be of an unsafe type, leading to issues when calling.then(...)and accessing methods likeroom.attach().Ensure that
context.roomis properly typed as aPromise<Room>. Update the context type or explicitly typecontext.room:- const attach = useCallback(() => context.room.then((room: Room) => room.attach()), [context]); + const attach = useCallback(() => (context.room as Promise<Room>).then((room) => room.attach()), [context]);Alternatively, adjust the
useRoomContextreturn type to include the correct typing forroom.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 76-76: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 76-76: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 76-76: Unsafe member access .attach on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 77-77: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 77-77: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 77-77: Unsafe member access .detach on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
65-65:
⚠️ Potential issueEnsure 'logger' is properly typed to prevent unsafe calls
The
loggerobject obtained fromuseLogger()may be implicitly typed asany, resulting in unsafe calls likelogger.debug(...).Ensure that
useLogger()returns a properly typed logger. Update theuseLoggerhook to specify the return type with the expected logging methods.- const logger = useLogger(); + const logger: LoggerType = useLogger();Define
LoggerTypewith the appropriate method signatures.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 65-65: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 65-65: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/helper/use-eventual-room.ts (3)
1-1:
⚠️ Potential issueImport missing types to resolve type errors.
It appears that there are type errors related to the
RoomandLoggertypes. Ensure that all necessary types are correctly imported to prevent these issues.Apply this diff to import the
Loggertype:-import { Room } from '@ably/chat'; +import type { Room } from '@ably/chat'; +import { Logger } from '../types/logger.js';Committable suggestion skipped: line range outside the PR's diff.
61-62:
⚠️ Potential issueType the
loggervariable explicitly to avoid unsafe member accesses.As with the previous hook, define the type of
loggerto prevent type-related errors.Apply this diff:
-const logger = useLogger(); +const logger: Logger = useLogger();📝 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.const logger: Logger = useLogger(); logger.trace('useEventualRoomProperty();', { roomId: context.roomId });🧰 Tools
🪛 eslint
[error] 61-61: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 62-62: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 62-62: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
20-21:
⚠️ Potential issueEnsure
loggeris properly typed to avoid unsafe calls.The
loggerobtained fromuseLogger()may not have explicit typing, leading to unsafe calls and member accesses. Make sureloggeris correctly typed asLogger.Apply this diff to define the type of
logger:-const logger = useLogger(); +const logger: Logger = useLogger();📝 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.const logger: Logger = useLogger(); logger.trace('useEventualRoom();', { roomId: context.roomId });🧰 Tools
🪛 eslint
[error] 20-20: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 21-21: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 21-21: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/helper/use-room-status.ts (1)
93-97: 🛠️ Refactor suggestion
Handle Unmounting Logic Carefully
In the cleanup function, ensure that all necessary subscriptions are properly disposed of, and handle any potential undefined variables.
Double-check that
offis defined before calling it:return () => { logger.debug('useRoomStatus(); unmounting'); if (off) { logger.debug('useRoomStatus(); unsubscribing from status changes'); off(); } + else { + logger.warn('No subscription to unsubscribe'); + } };📝 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.logger.debug('useRoomStatus(); unmounting'); if (off) { logger.debug('useRoomStatus(); unsubscribing from status changes'); off(); } else { logger.warn('No subscription to unsubscribe'); }🧰 Tools
🪛 eslint
[error] 93-93: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 93-93: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 95-95: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 95-95: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/hooks/use-occupancy.ts (7)
61-61: 🛠️ Refactor suggestion
Ensure
loggeris correctly typed to prevent unsafe calls.The
loggerobject is used for logging throughout the code. Ifloggerlacks proper type definitions, it may lead to TypeScript errors about unsafe calls or member access. Please ensureloggeris appropriately typed.Also applies to: 78-78, 81-81, 95-95, 103-103, 118-118, 121-121
🧰 Tools
🪛 eslint
[error] 61-61: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 61-61: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
131-131:
⚠️ Potential issueHandle possible undefined value from
useEventualRoomProperty.The
useEventualRoomPropertymight returnundefinedif the room isn't available yet. Ensure that any components usingoccupancycan handleundefinedvalues gracefully to prevent runtime errors.🧰 Tools
🪛 eslint
[error] 131-131: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 131-131: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 131-131: Unsafe member access .occupancy on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
115-128:
⚠️ Potential issueCheck for
context.roombefore subscribing user-provided listeners.Before subscribing the user's listener to occupancy events, verify that
context.roomis available to avoid exceptions.Apply this diff to add the null check:
if (!listenerRef) return; + if (!context.room) return; return wrapRoomPromise( context.room, (room) => { logger.debug('useOccupancy(); applying listener', { roomId: context.roomId });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 118-118: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 118-118: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 119-119: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 119-119: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 119-119: Unsafe member access .occupancy on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 121-121: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 121-121: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 122-122: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
98-99: 🛠️ Refactor suggestion
Specify the type of
occupancyEventto avoid unsafe assignments.In the occupancy event subscriber,
occupancyEventmight be implicitly of typeany. Explicitly typing it will prevent unsafe assignments and enhance type safety.Apply this diff to add the type annotation:
const { unsubscribe } = room.occupancy.subscribe((occupancyEvent: OccupancyEvent) => { setOccupancyMetrics({ connections: occupancyEvent.connections, presenceMembers: occupancyEvent.presenceMembers, });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 98-98: Unsafe assignment of an
anyvalue.(@typescript-eslint/no-unsafe-assignment)
[error] 98-98: Unsafe member access .connections on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
[error] 99-99: Unsafe assignment of an
anyvalue.(@typescript-eslint/no-unsafe-assignment)
[error] 99-99: Unsafe member access .presenceMembers on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
57-61:
⚠️ Potential issueAdd null checks for
contextand its properties.The
contextobtained fromuseRoomContext('useOccupancy')might beundefined. Accessingcontext.roomIdwithout verifying its existence could lead to runtime errors. Please ensurecontextand its properties are valid before use.Apply this diff to add a null check:
const context = useRoomContext('useOccupancy'); + if (!context) { + // Handle the absence of context appropriately + return { + occupancy: undefined, + connectionStatus, + connectionError, + roomStatus, + roomError, + connections: occupancyMetrics.connections, + presenceMembers: occupancyMetrics.presenceMembers, + }; + } logger.trace('useOccupancy();', { params, roomId: context.roomId });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 58-58: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 58-58: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 60-60: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 61-61: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 61-61: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
75-88:
⚠️ Potential issueValidate
context.roombefore using it inwrapRoomPromise.
context.roommight beundefined, which could cause errors when passed towrapRoomPromise. Add a check to ensurecontext.roomexists before proceeding.Apply this diff to add the null check:
if (!onDiscontinuityRef) return; + if (!context.room) return; return wrapRoomPromise( context.room, (room) => { logger.debug('useOccupancy(); applying onDiscontinuity listener', { roomId: context.roomId });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 78-78: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 78-78: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 79-79: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 79-79: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 79-79: Unsafe member access .occupancy on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 81-81: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 81-81: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 82-82: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
92-110:
⚠️ Potential issueEnsure
context.roomis defined before subscribing to occupancy events.When subscribing to occupancy events,
context.roommight beundefined. Adding a null check will prevent potential runtime errors.Apply this diff to check for
context.room:useEffect(() => { + if (!context.room) return; return wrapRoomPromise( context.room, (room) => { logger.debug('useOccupancy(); applying internal listener', { roomId: context.roomId });📝 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.if (!context.room) return; return wrapRoomPromise( context.room, (room) => { logger.debug('useOccupancy(); applying internal listener', { roomId: context.roomId }); const { unsubscribe } = room.occupancy.subscribe((occupancyEvent) => { setOccupancyMetrics({ connections: occupancyEvent.connections, presenceMembers: occupancyEvent.presenceMembers, }); }); return () => { logger.debug('useOccupancy(); cleaning up internal listener', { roomId: context.roomId }); unsubscribe(); }; }, logger, context.roomId, ).unmount(); }, [context, logger]);🧰 Tools
🪛 eslint
[error] 95-95: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 95-95: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 96-101: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 96-96: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 96-96: Unsafe member access .occupancy on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 98-98: Unsafe assignment of an
anyvalue.(@typescript-eslint/no-unsafe-assignment)
[error] 98-98: Unsafe member access .connections on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
[error] 99-99: Unsafe assignment of an
anyvalue.(@typescript-eslint/no-unsafe-assignment)
[error] 99-99: Unsafe member access .presenceMembers on an
anyvalue.(@typescript-eslint/no-unsafe-member-access)
[error] 103-103: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 103-103: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 104-104: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
src/react/helper/room-promise.ts (1)
63-66:
⚠️ Potential issueAddress the unsafe error handling and logging.
The static analysis tool has flagged several instances of unsafe error handling and logging. It's important to ensure that error objects are properly typed and accessed to avoid potential runtime issues.
Consider updating the error handling and logging code to use a more type-safe approach. For example, instead of directly accessing properties like
trace,debug, anderroron theerrorobject, you can use a type guard to narrow down the type of theerrorobject before accessing its properties.Here's an example of how you can update the error handling and logging code:
try { // ... } catch (error) { if (error instanceof Error) { this._logger.error('DefaultRoomPromise(); mount error', { roomId: this._roomId, error: error.message }); } else { this._logger.error('DefaultRoomPromise(); mount error', { roomId: this._roomId, error: 'Unknown error' }); } }By using a type guard (
error instanceof Error), you can ensure that theerrorobject is of typeErrorbefore accessing its properties. This helps prevent potential runtime errors and improves the type safety of the code.Do you want me to update the error handling and logging code throughout the file to address the static analysis warnings?
Also applies to: 79-79, 81-81, 86-86, 89-89, 118-118
🧰 Tools
🪛 eslint
[error] 63-63: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 66-66: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 66-66: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/hooks/use-messages.ts (2)
75-79:
⚠️ Potential issueEnsure proper error handling for the room context and status.
The
useRoomContextanduseRoomStatushooks are used to retrieve the room context and status. Make sure to handle any potential errors returned by these hooks appropriately.const { status: roomStatus, error: roomError } = useRoomStatus(params); const logger = useLogger(); + if (roomError) { + logger.error('useMessages(); Error retrieving room status', { roomId: context.roomId, error: roomError }); + // Handle the error, e.g., throw an exception or return an error state + } logger.trace('useMessages();', { params, roomId: context.roomId });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 76-76: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 76-76: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 78-78: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 79-79: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 79-79: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
86-93:
⚠️ Potential issueEnsure proper error handling for the
sendandgetmethods.The
sendandgetmethods now rely on thecontext.roompromise. Make sure to handle any potential errors that may occur when the promise is rejected.const send = useCallback( - (params: SendMessageParams) => context.room.then((room) => room.messages.send(params)), + async (params: SendMessageParams) => { + try { + const room = await context.room; + return room.messages.send(params); + } catch (error) { + logger.error('useMessages(); Error sending message', { roomId: context.roomId, error }); + throw error; + } + }, [context], ); const get = useCallback( - (options: QueryOptions) => context.room.then((room) => room.messages.get(options)), + async (options: QueryOptions) => { + try { + const room = await context.room; + return room.messages.get(options); + } catch (error) { + logger.error('useMessages(); Error retrieving messages', { roomId: context.roomId, error }); + throw error; + } + }, [context], );Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 87-87: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 87-87: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 87-87: Unsafe member access .messages on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 91-91: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 91-91: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 91-91: Unsafe member access .messages on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/hooks/use-presence.ts (1)
98-153:
⚠️ Potential issueEnsure proper error handling and logging.
The updated logic for entering the room now relies on the
wrapRoomPromisehelper function to handle the asynchronous operations. While this improves the clarity of the control flow, it's crucial to ensure that errors are properly caught, handled, and logged.Verify that the
wrapRoomPromisefunction adequately handles errors and logs them appropriately. If not, consider updating the function to include proper error handling and logging.🧰 Tools
🪛 eslint
[error] 98-98: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 98-98: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 103-103: Unsafe member access .status on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 103-103: Unsafe member access .Attached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 107-107: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 107-107: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 107-107: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 107-107: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 113-120: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 113-115: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 113-114: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 113-113: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 115-115: Unsafe member access .then on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 116-116: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 116-116: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 120-120: Unsafe member access .catch on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 121-121: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 121-121: Unsafe member access .error on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 127-127: Unsafe member access .status on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 127-127: Unsafe member access .Attached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 130-130: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 130-130: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 133-133: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 134-134: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 137-144: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 137-139: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 137-138: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 137-137: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 139-139: Unsafe member access .then on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 140-140: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 140-140: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 144-144: Unsafe member access .catch on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 145-145: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 145-145: Unsafe member access .error on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
src/react/hooks/use-typing.ts (4)
48-48:
⚠️ Potential issueEnsure
typingIndicatorsis always defined when the room is attachedChanging
typingIndicatorsfrom required to optional might lead to issues if other parts of the codebase assume it's always defined. SincetypingIndicatorsis only available when the room is attached, consider providing a default value or handling undefined cases wherevertypingIndicatorsis used.Apply this diff to provide a default empty
Typinginstance when the room is not attached:export interface UseTypingResponse extends ChatStatusResponse { // ... /** * Provides access to the underlying {@link Typing} instance of the room. */ - readonly typingIndicators?: Typing; + readonly typingIndicators: Typing; }And update the return statement of
useTypingto ensuretypingIndicatorsis always defined:return { - typingIndicators: useEventualRoomProperty((room) => room.typing), + typingIndicators: useEventualRoomProperty((room) => room.typing) ?? new Typing(), // other properties };Committable suggestion skipped: line range outside the PR's diff.
69-73:
⚠️ Potential issueAddress TypeScript unsafe assignments and member accesses
Static analysis hints indicate unsafe assignments and member accesses starting at line 71. Ensure that the types are correctly asserted or handled to prevent runtime errors.
Apply this diff to safely destructure and type the context:
- const context = useRoomContext('useTyping'); + const context = useRoomContext('useTyping') as { roomId: string; room: Promise<Room>; }; const { status: roomStatus, error: roomError } = useRoomStatus(params); const logger = useLogger(); - logger.trace('useTyping();', { roomId: context.roomId }); + logger.trace(`useTyping(); initialized for roomId: ${context.roomId}`);📝 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.const context = useRoomContext('useTyping') as { roomId: string; room: Promise<Room>; }; const { status: roomStatus, error: roomError } = useRoomStatus(params); const logger = useLogger(); logger.trace(`useTyping(); initialized for roomId: ${context.roomId}`);🧰 Tools
🪛 eslint
[error] 71-71: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 71-71: Unsafe array destructuring of a tuple element with an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 72-72: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 73-73: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 73-73: Unsafe member access .trace on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
182-183:
⚠️ Potential issueHandle possible rejections in
startandstopcallbacksThe
startandstopcallbacks usecontext.room.then, which might reject. Ensure that you handle potential promise rejections to prevent unhandled promise exceptions.Apply this diff to add error handling:
const start = useCallback(() => { - context.room.then((room) => room.typing.start()); + context.room.then((room) => room.typing.start()).catch((error) => { + logger.error(`useTyping(); error in start for roomId: ${context.roomId}`, error); + setError(error); + }); }, [context]); const stop = useCallback(() => { - context.room.then((room) => room.typing.stop()); + context.room.then((room) => room.typing.stop()).catch((error) => { + logger.error(`useTyping(); error in stop for roomId: ${context.roomId}`, error); + setError(error); + }); }, [context]);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 182-182: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 182-182: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 182-182: Unsafe member access .typing on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 183-183: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 183-183: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 183-183: Unsafe member access .typing on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
102-123:
⚠️ Potential issueHandle potential promise rejections
The promise chain starting at line 102 does not have a catch block at the end, which might lead to unhandled promise rejections. Ensure that all promises have appropriate error handling to prevent unexpected behavior.
Apply this diff to add a catch block:
return context.room .then((room) => { // existing code }) - .catch(); + .catch((error) => { + if (!mounted) return; + logger.error(`useTyping(); error in room promise for roomId: ${context.roomId}`, error); + setErrorState(error); + });📝 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.void context.room .then((room) => { // If we're not attached, we can't call typing.get() right now if (room.status === RoomStatus.Attached) { return room.typing .get() .then((currentlyTyping) => { if (!mounted) return; setCurrentlyTyping(currentlyTyping); }) .catch((error: unknown) => { const errorInfo = error as Ably.ErrorInfo; if (!mounted || errorInfoIs(errorInfo, ErrorCodes.RoomIsReleased)) return; setErrorState(errorInfo); }); } else { logger.debug('useTyping(); room not attached, setting currentlyTyping to empty', { roomId: context.roomId }); setCurrentlyTyping(new Set()); } }) .catch((error) => { if (!mounted) return; logger.error(`useTyping(); error in room promise for roomId: ${context.roomId}`, error); setErrorState(error); });🧰 Tools
🪛 eslint
[error] 105-105: Unsafe member access .status on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 105-105: Unsafe member access .Attached on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 106-117: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 106-112: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 106-108: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 106-107: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 106-106: Unsafe member access .typing on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 108-108: Unsafe member access .then on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 110-110: Unsafe argument of type
anyassigned to a parameter of typeSetStateAction<Set<string>>.(@typescript-eslint/no-unsafe-argument)
[error] 112-112: Unsafe member access .catch on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 113-113: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 114-114: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 114-114: Unsafe member access .RoomIsReleased on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 119-119: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 119-119: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/core/rooms.test.ts (3)
87-87:
⚠️ Potential issueResolve TypeScript errors due to 'ErrorCodes' typing
At line 87, static analysis reports unsafe assignment and member access on
ErrorCodes.RoomReleasedBeforeOperationCompleted. This suggests thatErrorCodesmay not be properly typed or imported. Ensure thatErrorCodesis correctly imported with appropriate type definitions.Consider updating the import or checking if the package provides type definitions for
ErrorCodes. This will resolve the TypeScript errors and improve type safety.🧰 Tools
🪛 eslint
[error] 87-87: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 87-87: Unsafe member access .RoomReleasedBeforeOperationCompleted on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
1-1:
⚠️ Potential issueEnsure 'ErrorCodes' is properly imported and typed
At line 1, verify that
ErrorCodesis correctly imported from'@ably/chat'with proper type definitions. Incorrect typing can lead to TypeScript errors in later lines whereErrorCodesis used.If necessary, update the import statement or adjust the type definitions to resolve type errors throughout the test file.
35-36: 🛠️ Refactor suggestion
Use defined error codes instead of hard-coded values
At lines 35-36, you're using a hard-coded error code
40000in the test assertion. For consistency and maintainability, it's better to use the constants provided byErrorCodes.Apply this diff to update the error code:
await expect(room2).rejects.toBeErrorInfo({ statusCode: 400, - code: 40000, + code: ErrorCodes.RoomAlreadyExistsWithDifferentOptions, message: 'room already exists with different options', });Committable suggestion skipped: line range outside the PR's diff.
src/react/providers/chat-room-provider.tsx (2)
93-93: 🛠️ Refactor suggestion
Add error handling in Promise
catchblocks to avoid silent failuresIn the asynchronous calls to
client.rooms.release(...), the.catch()method is invoked without an error handler. This means any errors thrown during the release process will be silently ignored, making debugging difficult.Apply this diff to handle errors properly:
-void client.rooms.release(id).catch(); +void client.rooms.release(id).catch((error) => { + this._logger.error(`Failed to release room ${id}:`, error); +});Repeat similar changes for other instances where
.catch()is called without parameters, such as at line 169:-void client.rooms.release(prev.roomId).catch(); +void client.rooms.release(prev.roomId).catch((error) => { + logger.error(`Failed to release room ${prev.roomId}:`, error); +});Providing error handlers ensures that any issues are logged and can be addressed promptly.
Also applies to: 169-169
🧰 Tools
🪛 eslint
[error] 93-93: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 93-93: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 93-93: Unsafe member access .rooms on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 93-93: Unsafe member access .catch on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
2-2:
⚠️ Potential issueResolve TypeScript 'unsafe' errors by ensuring correct type imports
The TypeScript compiler reports numerous unsafe calls and assignments throughout the file, indicating that
ChatClient,Logger,Room, and related types are not properly recognized. This suggests that TypeScript treats these imports asanyorerrortypes due to incorrect type importing.Here's a suggested fix to explicitly import the types:
-import { ChatClient, Logger, Room, RoomOptions, type RoomOptionsDefaults } from '@ably/chat'; // eslint-disable-line @typescript-eslint/no-unused-vars +import type { ChatClient, Logger, Room, RoomOptions, RoomOptionsDefaults } from '@ably/chat'; // eslint-disable-line @typescript-eslint/no-unused-varsEnsure that the
@ably/chatpackage provides the necessary type definitions and that your TypeScript configuration (tsconfig.json) is correctly set up to handle type imports.Committable suggestion skipped: line range outside the PR's diff.
src/react/hooks/use-presence-listener.ts (5)
99-100:
⚠️ Potential issueFix the unsafe assignments and member access.
The static analysis tool has identified unsafe assignments and member access related to the
errorparameter in thesetErrorStatefunction. Ensure that theerrorparameter is correctly typed and accessed to prevent potential issues.🧰 Tools
🪛 eslint
[error] 99-99: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 99-99: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 99-99: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 100-100: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
107-107:
⚠️ Potential issueFix the unsafe member access.
The static analysis tool has flagged an unsafe member access on the
loggerobject. Make sure that theloggerobject is properly typed and accessed to avoid runtime errors.🧰 Tools
🪛 eslint
[error] 107-107: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 107-107: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
273-287:
⚠️ Potential issueFix the unsafe calls, assignments, and member access.
The effect hook for subscribing the user-provided listener contains unsafe calls, assignments, and member access related to the
roomparameter. Address these issues to ensure type safety and avoid potential runtime errors.🧰 Tools
🪛 eslint
[error] 276-276: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 276-276: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 277-277: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 277-277: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 277-277: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 280-280: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 280-280: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 281-281: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
292-306:
⚠️ Potential issueFix the unsafe calls, assignments, and member access.
The effect hook for subscribing the
onDiscontinuitylistener has unsafe calls, assignments, and member access related to theroomparameter. These issues should be resolved to ensure type safety and prevent potential runtime errors.🧰 Tools
🪛 eslint
[error] 295-295: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 295-295: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 296-296: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 296-296: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 296-296: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 299-299: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
[error] 299-299: Unsafe member access .debug on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
[error] 300-300: Unsafe call of a(n)
errortype typed value.(@typescript-eslint/no-unsafe-call)
309-309:
⚠️ Potential issueFix the unsafe assignment, return, and member access.
The static analysis tool has identified an unsafe assignment, return, and member access related to the
presenceproperty in the returned object. Ensure that thepresenceproperty is correctly typed and accessed to avoid runtime errors.🧰 Tools
🪛 eslint
[error] 309-309: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 309-309: Unsafe return of a value of type error.
(@typescript-eslint/no-unsafe-return)
[error] 309-309: Unsafe member access .presence on an
errortyped value.(@typescript-eslint/no-unsafe-member-access)
test/core/room-lifecycle-manager.test.ts (1)
201-201:
⚠️ Potential issueFix unsafe assignment to 'channel' property
The assignment to
channelin thecontributorobject may result in a type error due to unsafe assignment. Ensure that thechannelproperty is correctly typed to avoid the@typescript-eslint/no-unsafe-assignmenterror.Apply this diff to explicitly type the
channelproperty:contributor: { discontinuityDetected() {}, attachmentErrorCode, detachmentErrorCode, - channel: channel, + channel: channel as Ably.RealtimeChannel, },📝 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.channel: channel as Ably.RealtimeChannel,🧰 Tools
🪛 eslint
[error] 201-201: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
AndyTWF
force-pushed
the
async-room-get
branch
from
10fba9d to
3c55a36
Compare
src/core/rooms.ts
Outdated
| private readonly _rooms: Map<string, DefaultRoom> = new Map<string, DefaultRoom>(); | ||
| private readonly _releasing = new Map<string, { count: number; promise: Promise<void> }>(); | ||
| private readonly _rooms: Map<string, RoomMapEntry> = new Map<string, RoomMapEntry>(); | ||
| private readonly _releasing = new Map<string, { promise: Promise<void> }>(); |
There was a problem hiding this comment.
I think this makes things more confusing.
If I see a Promise<void> I know exactly what I'm dealing with - a standard, well-understood JS type. If I see anything else, I have to go and look up exactly what it is and it's thenable behaviour won't necessarily be obvious to me.
There was a problem hiding this comment.
Problem is I don't understand anything when I look at
private readonly _releasing = new Map<string, { promise: Promise<void> }>();
Maybe we can add typealias for Map<string, { promise: Promise<void> }>(); to better understand what map exactly represents wdyt
There was a problem hiding this comment.
That's reasonable - I've gone a slightly different way and dropped the object wrapping it so its now just Map<string, Promise<void>>, as the wrapper is no longer needed.
AndyTWF
force-pushed
the
async-room-get
branch
from
3c55a36 to
d188b1e
Compare
AndyTWF
force-pushed
the
async-room-get
branch
from
d188b1e to
3513853
Compare
src/core/rooms.ts
Outdated
| @@ -72,25 +107,80 @@ export class DefaultRooms implements Rooms { | |||
| /** | |||
| * @inheritDoc | |||
| */ | |||
| get(roomId: string, options: RoomOptions): Room { | |||
| async get(roomId: string, options: RoomOptions): Promise<Room> { | |||
There was a problem hiding this comment.
Nothing is awaited, doesn't have to be async. Seems the same for release().
If we remove the async we also get referential integrity for the Promise returned.
AndyTWF
force-pushed
the
async-room-get
branch
from
3513853 to
88e73eb
Compare
|
Also |
|
I think all usages of |
Where are you seeing this? My IDE can't find any.
It has?
Yes, that has now been removed - thanks :) |
This change adds a useful helper hook that creates a stable reference to some callback, its a useful alternative to our existing event listener hook that doesn't allow undefined.
this change adds a type that wraps room promises, RoomPromise. Its purpose is to wrap room promises and execute useEffect/callback functions conditionally based on the promise resolution and the current react lifecycle.
AndyTWF
force-pushed
the
async-room-get
branch
from
6d202db to
faff55e
Compare
This change makes the rooms.get() function async. This greatly simplifies the underlying Room logic - we no longer have async initialisation after the instance is returned, and can do away with all the complexity that brings. Instead, whether we need to delay returning a room because the previous is being released is now dealt with at the rooms level. The rooms singleton won't return a value until the previous room has released properly. The change therefore removes all of the async-related code in the DefaultRoom type and beyond. There are also changes to React, namely: - useRoom now returns Room|undefined - the context value for a Room provider now contains a promise to the room - fixed a bug by which a room would be immediately invalidated in state in Strict Mode - this is now handled more gracefully - added new helper hooks to better compartmentalise room resolution and how that relates to React
This was a coderabbit suggestion - to remove our instances of `as unknown as ChatClient`. This was do-able by making sure that typescript only emits declarations for properties that aren't marked as internal.
These are no longer required, so can be removed.
This was missed on the previous iteration, which would occaisionally cause bugs such as channels complaining about mode/options changes. This change fixes the issue by making sure that release promises don't linger after they're done. Also fixes React to handle room changes properly w.r.t release.
Makes it easier to debug
AndyTWF
force-pushed
the
async-room-get
branch
from
faff55e to
1eba39c
Compare
| // In StrictMode this will be called twice one after the other, but that's ok | ||
| const [value, setValue] = useState<ChatRoomContextType>(() => { | ||
| logger.debug(`ChatRoomProvider(); initializing value`, { roomId, options }); | ||
| return { room: client.rooms.get(roomId, options).catch(), roomId: roomId, options: options, client: client }; |
There was a problem hiding this comment.
What's the purpose of this catch()?
If we want to allow the error from rooms.get to propagate, we remove the catch().
If we want to suppress the error, we need to pass an empty function. But I'm guessing we don't want this promise of type Promise<Room> to resolve to undefined, that would be breaking the interface.
There was a problem hiding this comment.
Seems to work, but I've made it a bit better for us :)
AndyTWF
force-pushed
the
async-room-get
branch
from
1eba39c to
f25bfa9
Compare
AndyTWF
force-pushed
the
async-room-get
branch
from
f25bfa9 to
f0c0b6d
Compare
Makes it a bit easier to follow, also better caters for the "changing room options" use-case.
AndyTWF
force-pushed
the
async-room-get
branch
from
f0c0b6d to
e13aa9b
Compare
Context
CHA-668
CHADR-063
Description
Core
rooms.get()is now an async call. It resolves when any pendingreleasecalls for the same room have completed.DefaultRoomassociated with post-get initialization, which was required due torooms.get()previously being syncPromise<Channel>from theirchannelmethodnoncevalue - an internal identifier that lets us see identify room instances across React renders even if they have the same id.React
ChatRoomProviderwhereby a state value of the room would be almost immediately invalidated inStrictMode. This is achieved by staggeringreleaseoperations inuseEffectsuch that they don't immediately occur - and thus don't immediately invalidate the state value.useRoomnow returnsRoom | undefined, and resolves once the room itself resolves.useMessagesand other hooks now return optional values e.g.Messages | undefineduseRoomContextreturns the context value of a room without doing the fulluseRoomcycle.useRoomStatushandles providing room status and error values. This allows our hooks to not depend onuseRoomand its "optional" behaviour internally.Roompromise, and performs useEffect-like lifecycle ops depending on the react lifecycle and the resolution of the room.Checklist
Testing Instructions (Optional)
Thoroughly prod the demo app!
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation