Feat/frontend/chat/invite pong #227
Conversation
|
Warning Rate Limit Exceeded@kotto5 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 27 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on enhancing chat functionality, particularly around invitation handling in a pong game context. New error handling and event emission logic have been added to the backend, improving the user experience during invite cancellation. The frontend now includes additional states and functions to manage pong match invitations and responses, along with new toast notifications. Changes also reflect a shift from private to public user entities in the chat service, affecting how users are stored and message entities are constructed. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| export class PublicUserEntity { | ||
| constructor(partial: Partial<PublicUserEntity>) { |
There was a problem hiding this comment.
The constructor of PublicUserEntity does not check for the presence of partial before accessing its properties. This could lead to a runtime error if partial is null or undefined.
constructor(partial: Partial<PublicUserEntity>) {
+ if (!partial) {
+ throw new Error('PublicUserEntity constructor requires a partial object');
+ }
this.id = partial.id;
this.name = partial.name;
this.avatarURL = partial.avatarURL;
}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.
| export class PublicUserEntity { | |
| constructor(partial: Partial<PublicUserEntity>) { | |
| export class PublicUserEntity { | |
| constructor(partial: Partial<PublicUserEntity>) { | |
| if (!partial) { | |
| throw new Error('PublicUserEntity constructor requires a partial object'); | |
| } | |
| this.id = partial.id; | |
| this.name = partial.name; | |
| this.avatarURL = partial.avatarURL; | |
| } |
There was a problem hiding this comment.
null が入ったらどうなるのが正解なのか。
undefined だったら別に 全部のメンバがundefined でよさそうだけど
runtime error になっちゃうのか
There was a problem hiding this comment.
@kotto5 通常、TypeScriptではPartial<Type>を使用すると、Typeのすべてのプロパティがオプショナルになります。しかし、nullを渡すと、nullはプロパティを持たないため、Partial<Type>の契約を満たしません。undefinedが渡された場合、すべてのメンバ変数がundefinedになることは許容されるかもしれませんが、これは意図的な設計かどうかによります。
コンストラクタがnullやundefinedを許容し、すべてのメンバ変数をundefinedに設定することが適切か、それとも将来的な問題を防ぐためにエラーを投げるべきかは、アプリケーションの設計とエラーハンドリング戦略に依存します。もしPublicUserEntityが有効な状態でのみ存在するべきなら、nullやundefinedを渡すことによるランタイムエラーを防ぐために、引数のチェックを行うことが推奨されます。それにより、不正な状態でのオブジェクトの作成を防ぎ、デバッグを容易にすることができます。
もしundefinedでのメンバ変数の初期化が許容される場合は、コンストラクタの引数チェックを省略し、partialがundefinedまたはnullの場合には、すべてのメンバ変数をundefinedで初期化することができます。ただし、これは後のコードで予期せぬundefinedの扱いによるエラーを引き起こす可能性があるため、注意が必要です。
どちらのアプローチを取るかは、アプリケーションの要件とエラーハンドリングの方針によります。もしエラーチェックを行う方針であれば、提案したコードが適切です。もしundefinedを許容する方針であれば、コンストラクタの引数チェックを省略することができます。どちらの方針を取るかについてご意見をいただければ、それに基づいてさらにサポートさせていただきます。
| export type InviteEvent = { | ||
| userId: number; | ||
| }; |
There was a problem hiding this comment.
The InviteEvent type only includes userId but does not specify the type of invitation or any other context. This might be insufficient for handling different types of invitations in the future.
Consider adding more context to the InviteEvent type to make it more extensible.
| const showInvitePongToast = (message: InviteEvent) => { | ||
| toast({ | ||
| title: `user id: ${message.userId}`, | ||
| description: ` invited you to play pong!`, | ||
| action: ( | ||
| <ToastAction altText="approve" asChild> | ||
| <> | ||
| <button | ||
| onClick={() => { | ||
| socket.emit("approve-pong", { | ||
| userId: message.userId, | ||
| }); | ||
| }} | ||
| > | ||
| approve | ||
| </button> | ||
| <button | ||
| onClick={() => { | ||
| socket.emit("deny-pong", { | ||
| userId: message.userId, | ||
| }); | ||
| }} | ||
| > | ||
| Deny | ||
| </button> | ||
| </> | ||
| </ToastAction> | ||
| ), | ||
| }); |
There was a problem hiding this comment.
The showInvitePongToast function emits socket events without any error handling or acknowledgment mechanism. This could lead to unhandled errors or a lack of user feedback if the socket operation fails.
Implement an acknowledgment callback for the socket events to handle potential errors and provide user feedback.
There was a problem hiding this comment.
acknowledgment って返信みたいに捉えてるけどどうやって実装されているんだろう
ws.on(event) の event も emit する情報に含まれてる?
ちなみにerror 処理ってやること何があるんですか?
There was a problem hiding this comment.
@kotto5 Acknowledgment in the context of WebSocket events, especially when using libraries like socket.io, is typically implemented by providing a callback function when emitting an event. This callback is called by the server to acknowledge the receipt of the event. Here's a simplified example of how you might implement it:
socket.emit("approve-pong", { userId: message.userId }, (response) => {
// Handle acknowledgment here
if (response.error) {
// Handle the error case
} else {
// Proceed with the success case
}
});In this example, the third argument to emit is a callback function that the server should call to acknowledge the event. The server-side code would need to support this by calling the callback with the appropriate response.
Regarding error handling, there are a few things you might consider:
- Handling errors when the socket connection fails, which could involve retrying the connection or informing the user that the service is currently unavailable.
- Handling cases where the server does not acknowledge the event within a certain timeframe, which could involve setting a timeout and providing feedback to the user.
- Dealing with exceptions that may occur during the event handling process, which could involve try-catch blocks and user-friendly error messages.
Each of these would require additional code to handle the various scenarios that could occur during WebSocket communication.
| const showDenyPongToast = (data: DenyEvent) => { | ||
| toast({ | ||
| title: `Your invite was denied`, | ||
| }); |
There was a problem hiding this comment.
The showDenyPongToast function displays a generic denial message without specifying which invitation was denied. This could be confusing for users with multiple pending invitations.
Include information about the specific invitation that was denied to provide clearer feedback to the user.
There was a problem hiding this comment.
いや、invite は同時に一人しかできないので大丈夫
でも、invite が同時に一人しか出来ないのは user から全く確認できないようになってるなあ
There was a problem hiding this comment.
@kotto5 了解しました。ユーザーが同時に一人だけに招待を送れるという制限は理解しました。ユーザーインターフェースでこの制限がユーザーに明確でないという点については、将来的にユーザーにもっと透明にすることを検討する価値があるかもしれませんね。フィードバックありがとうございます。
| @@ -71,6 +71,7 @@ export default function SidebarItem({ | |||
| const [isMuted, setIsMuted] = useState( | |||
| mutedUsers?.some((u: PublicUserEntity) => u.id === user.userId), | |||
| ); | |||
| const [isInviting, setIsInviting] = useState(false); | |||
There was a problem hiding this comment.
The isInviting state is used to manage the invitation status, but there is no error handling or acknowledgment mechanism for the invitation process. This could lead to a mismatch between the UI state and the actual invitation status.
Implement an acknowledgment mechanism for the invitation process to ensure the UI state accurately reflects the actual status.
There was a problem hiding this comment.
確かに。
でもどう解決するのかわからない
state のチェックは backend に api 飛ばした返り値で更新するとか?
invite/who みたいな
There was a problem hiding this comment.
@kotto5 確かに、フロントエンドの状態をバックエンドのAPIの応答で更新することは一つの解決策です。しかし、リアルタイムのアプリケーションでは、WebSocketのイベントリスナーやacknowledgmentを使用してUIの状態を更新するのが一般的です。例えば、socket.emitで招待を送信する際に、第三引数としてacknowledgment callbackを提供することができます。これにより、サーバーからの応答をリアルタイムで受け取り、状態を適切に更新することが可能になります。
const invite = () => {
socket.emit("invite-pong", { userId: user.userId }, (response) => {
// ここでサーバーからの応答に基づいてUIの状態を更新
if (response.status === "success") {
setIsInviting(true);
} else {
// エラーハンドリング
}
});
};または、サーバーからの特定のイベントをリッスンして、それに応じて状態を更新することもできます。
useEffect(() => {
socket.on("invite-pong-ack", (data) => {
if (data.userId === user.userId) {
setIsInviting(data.isInviting);
}
});
// 必要に応じてイベントリスナーを解除
return () => {
socket.off("invite-pong-ack");
};
}, [user.userId]);これらの方法を使用すると、ユーザーのアクションに対する即時のフィードバックをUIに提供し、状態の不一致を防ぐことができます。
| const invite = () => { | ||
| socket.emit("invite-pong", { userId: user.userId }); | ||
| setIsInviting(true); | ||
| }; | ||
| const cancelInvite = () => { | ||
| socket.emit("invite-cancel-pong", { userId: user.userId }); | ||
| setIsInviting(false); |
There was a problem hiding this comment.
The invite and cancelInvite functions emit socket events without any error handling or acknowledgment mechanism. This could lead to unhandled errors or a lack of user feedback if the socket operation fails.
Implement an acknowledgment callback for the socket events to handle potential errors and provide user feedback.
kotto5
force-pushed
the
feat/frontend/chat/invite-pong
branch
from
b7037c2 to
79157f5
Compare
kotto5
force-pushed
the
feat/frontend/chat/invite-pong
branch
from
79157f5 to
c6db1ec
Compare
kotto5
force-pushed
the
feat/frontend/chat/invite-pong
branch
from
563980c to
3d131eb
Compare
|
|
||
| class PrivateUserEntity { | ||
| constructor(partial: Partial<PrivateUserEntity>) { | ||
| export class PublicUserEntity { |
There was a problem hiding this comment.
PublicUserEntityという命名はuser/entities/public-user.entity.tsに重複する名前のものがあるため別のものにした方が良いです。
| this.content = partial.content; | ||
| this.roomId = partial.roomId; | ||
| this.user = new PrivateUserEntity(user); | ||
| this.user = user; |
There was a problem hiding this comment.
MessageEntityのコードは元のコードの方が良いと思います。
MessageEntityで除外できていた要素をMessageEntityの外で処理しなければいけなくなるので。
| canceler.ws.emit('invite-cancel-pong'); | ||
| }); | ||
| it('should receive an error when canceling without an existing invite', () => | ||
| errorCtx); |
There was a problem hiding this comment.
1423行目のテストは主語がないと誰がerrorを受け取るのか少し分かりづらいかもです。(cancelingをした人であることは自明ではあるのですが一応)
また、inviteeがinvite-cancel-pongのイベントを受け取らないことを確認するテストもあるとより良いと思います。
frontend で、invite を実行できるようにしました
TODO
(code rabbit)