Open
Conversation
i-meant-to-be
added
feat
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
🚀 Preview 배포 완료!
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a STOMP-based WebSocket communication system, featuring a singleton SocketManager for global connection handling and a useSocket React hook for component-level interactions. The implementation includes automated reconnection logic with exponential backoff and comprehensive unit tests. Key feedback includes addressing a potential issue where the hook's cleanup logic might prematurely disconnect the global socket for other components, removing unused configuration options like jitterMs, restricting debug logs to development environments, and making STOMP headers optional in the publish function to match the underlying API.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSTOMP 기반 WebSocket 클라이언트 스택이 추가되었습니다: 런타임/타입 의존성 추가, 싱글톤 SocketManager, 메시지 타입 정의, React 훅(useSocket), 관련 단위 테스트 및 MockServiceWorker 버전 업데이트가 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
actor React as React Component
participant Hook as useSocket Hook
participant Manager as SocketManager
participant STOMP as STOMP Client
participant Server as WebSocket Server
React->>Hook: connect(options)
Hook->>Manager: connect(options)
Manager->>STOMP: new Client(), configure, activate()
STOMP->>Server: WebSocket 연결 / STOMP CONNECT
Server-->>STOMP: CONNECTED
STOMP-->>Manager: onConnect
Manager-->>Hook: onConnect 이벤트 전달
React->>Hook: subscribe(destination, cb)
Hook->>Manager: subscribe(destination, cb)
Manager->>STOMP: client.subscribe(destination, cb)
Server-->>STOMP: MESSAGE
STOMP-->>Manager: callback 호출
Manager-->>Hook: 메시지 전달
React->>Hook: publish(dest, msg)
Hook->>Manager: publish(dest, msg)
Manager->>STOMP: client.publish({destination: dest, body: JSON.stringify(msg)})
Note over STOMP,Manager: 연결 종료 처리
STOMP->>Manager: onWebSocketClose
Manager->>Manager: calculateBackoffDelay() & set reconnectDelay
Manager->>STOMP: activate() 재시도
React->>Hook: disconnect()
Hook->>Manager: disconnect()
Manager->>STOMP: deactivate()
Manager-->>Hook: 구독 정리 완료
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/apis/sockets/SocketManager.ts (1)
18-19:jitterMs는 현재 dead config입니다.
Line 19와Line 37에서 공개한jitterMs가Line 217-221의 계산식에서 한 번도 쓰이지 않아, 호출자가 값을 바꿔도 재연결 동작이 달라지지 않습니다. 풀 지터만 유지할 계획이면 이 옵션과 문서를 제거하고, 아니라면 계산식에 실제로 반영해 주세요.Also applies to: 35-40, 217-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apis/sockets/SocketManager.ts` around lines 18 - 19, The jitterMs option declared on the SocketManager (property jitterMs) is unused in the reconnect delay computation around the reconnect logic (currently at the block handling delay math), so either remove the jitterMs option and its docs if you intend to keep only full random jitter, or modify the reconnect delay calculation in the SocketManager reconnect/backoff routine to incorporate jitterMs; locate the class SocketManager and update the function that computes the reconnection delay (the block around the existing delay math used at reconnect time) to apply a random offset scaled by this.jitterMs (or use it as max jitter) so caller-provided jitterMs actually affects retry timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apis/sockets/SocketManager.test.ts`:
- Around line 38-45: The mock client starts with active = true which masks the
duplicate-connection guard; change the MockClient so its active property
initializes to false and ensure the activate and deactivate mocks actually
toggle that property (i.e., wrap or replace activate/deactivate vi.fn()
implementations to set this.active = true/false respectively) so tests exercise
real state transitions for MockClient and let connect()/disconnect() guards be
validated.
In `@src/apis/sockets/type.ts`:
- Around line 19-21: The TimerDataPayload interface uses Omit on a string union
which is invalid; replace the type on the timerType property to use
Exclude<TimeBoxType, 'FEEDBACK'> so that timerType is narrowed from TimeBoxType
to 'NORMAL' | 'TIME_BASED' (update the TimerDataPayload interface and the
timerType property accordingly).
In `@src/hooks/useSocket.ts`:
- Around line 72-84: The cleanup currently calls the global
socketManager.disconnect() inside useSocket's effect cleanup, which will close
the shared singleton connection for all consumers; remove the
socketManager.disconnect() call from the return cleanup in useSocket so the hook
only unsubscribes its own subscriptions (subscriptions.current.forEach(...);
subscriptions.current.clear()) and leave connection lifecycle to the
higher-level owner/provider (or add an explicit ownership check if you intend
this hook to sometimes own the socket). Ensure the unique symbols referenced are
useEffect/useSocket, subscriptions.current, and socketManager.disconnect.
- Around line 21-24: The publish callback in useSocket is overly strict by
requiring headers, which prevents header-less publishes; update the useCallback
signature for publish to accept headers?: StompHeaders (making headers optional)
and ensure the forwarding call uses the same parameters when calling
SocketManager.publish so the hook's types match SocketManager.publish and allow
calls without headers.
- Around line 48-58: The current dedupe uses
subscriptions.current.has(destination) which blocks resubscribe after a
disconnect because the old entry remains; update the subscribe callback to
retrieve the existing subscription (subscriptions.current.get(destination)) and
only skip creating a new subscription if that existing subscription is still
active (e.g., check a truthy active property or absence of a closed flag /
presence of an unsubscribe method on the stored object). If the stored
subscription is inactive or closed, call socketManager.subscribe(destination,
callback) to create a new one and replace the entry in subscriptions.current;
reference the subscribe function, subscriptions.current, and
socketManager.subscribe to locate and change the logic.
---
Nitpick comments:
In `@src/apis/sockets/SocketManager.ts`:
- Around line 18-19: The jitterMs option declared on the SocketManager (property
jitterMs) is unused in the reconnect delay computation around the reconnect
logic (currently at the block handling delay math), so either remove the
jitterMs option and its docs if you intend to keep only full random jitter, or
modify the reconnect delay calculation in the SocketManager reconnect/backoff
routine to incorporate jitterMs; locate the class SocketManager and update the
function that computes the reconnection delay (the block around the existing
delay math used at reconnect time) to apply a random offset scaled by
this.jitterMs (or use it as max jitter) so caller-provided jitterMs actually
affects retry timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 520c108b-671d-488b-8e66-8d4bc06908ea
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonpublic/mockServiceWorker.jssrc/apis/sockets/SocketManager.test.tssrc/apis/sockets/SocketManager.tssrc/apis/sockets/type.tssrc/hooks/useSocket.ts
jaeml06
reviewed
Mar 28, 2026
Contributor
jaeml06
left a comment
jaeml06
left a comment
There was a problem hiding this comment.
코드 리뷰: 4가지 항목에 대한 피드백입니다.
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
Contributor
Author
|
/noti @jaeml06 짚어주신 부분 반영하였습니다! 빠른 리뷰 감사합니당 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apis/sockets/SocketManager.ts`:
- Around line 69-75: The new connect() call must start a fresh retry session:
reset the retryCount to zero when initiating a new connection (inside
SocketManager.connect() right after merging currentOptions and/or when a fresh
client connection begins) so previous exhausted retries aren't carried forward;
ensure the same reset is applied where manual reconnects are kicked off (related
code around handleReconnection() and the reconnection start path referenced near
lines 183-190) so handleReconnection() uses the new zeroed retryCount for its
retry loop.
- Around line 211-215: The current calculateBackoffDelay can return 0 (disabling
reconnect in `@stomp/stompjs`); update calculateBackoffDelay to clamp the result
to a minimum of 1ms: compute maxExponentialDelay as now (using
this.currentOptions.baseRetryDelayMs and currentRetryCount), ensure
maxExponentialDelay is at least 1, then return Math.max(1,
Math.floor(Math.random() * maxExponentialDelay)). Reference the
calculateBackoffDelay method and this.currentOptions.baseRetryDelayMs when
making the change.
In `@src/hooks/useSocket.ts`:
- Around line 27-29: The hook currently calls socketManager.connect(options) in
the connect useCallback but only cleans up local subscriptions, leaving
callbacks like onConnect registered inside the singleton socketManager and
causing stale callbacks after unmount; update the hook so it registers a
reconnect/onConnect handler tied to the hook lifecycle and removes it on cleanup
— e.g., have connect (in useCallback) register a wrapper callback with
socketManager (or call a socketManager.connect variant that returns an
unsubscribe/token) and store that token in hook state, then remove the handler
in the effect cleanup by calling the corresponding socketManager.off/unsubscribe
with that token; ensure any places referenced (connect useCallback,
socketManager.connect, onConnect handlers and the cleanup logic around lines
noted) consistently register and deregister the reconnect handler so callbacks
don’t persist after unmount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e02894f2-ff35-4401-9779-d4f7ce10a796
📒 Files selected for processing (4)
src/apis/sockets/SocketManager.test.tssrc/apis/sockets/SocketManager.tssrc/apis/sockets/type.tssrc/hooks/useSocket.ts
✅ Files skipped from review due to trivial changes (1)
- src/apis/sockets/type.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apis/sockets/SocketManager.test.ts
jaeml06
approved these changes
Mar 30, 2026
Contributor
jaeml06
left a comment
jaeml06
left a comment
There was a problem hiding this comment.
확인했습니다. 반영감사합니다. 추가로 코드래빗이 리뷰가 달려있으니 그 부분만 한번 확인해주시면 감사하겠습니다.
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apis/sockets/SocketManager.ts`:
- Around line 110-113: The onWebSocketClose handler currently reads and mutates
mutable this.client so a late-arriving close event from an old connection can
corrupt a new session's retry state; modify the reconnection/close handlers to
capture the specific Client instance when you create it (e.g., const client =
this.client or pass the instance into onWebSocketClose) and have
onWebSocketClose operate only on that captured client instead of this.client,
and ensure you unregister/remove the old handlers when replacing the client so
retryCount and reconnectDelay on the new Client are not touched by stale events
(apply the same pattern where similar handlers are registered around lines
referenced by 139-146 and 206-225).
- Around line 26-27: The declared onConnect? callback in SocketManager is never
invoked: although connect() stores the option and connectListeners are called on
success, onConnect is omitted; modify the successful connection flow inside the
connect() method (and the same success paths referenced by connectListeners at
the other locations) to call this.onConnect?.() after or before invoking
connectListeners to ensure the documented completion hook runs; ensure optional
chaining and null checks around onConnect so it remains optional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 545e3796-869b-4a89-8388-0a24d68766d2
📒 Files selected for processing (3)
src/apis/sockets/SocketManager.test.tssrc/apis/sockets/SocketManager.tssrc/hooks/useSocket.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useSocket.ts
- src/apis/sockets/SocketManager.test.ts
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/apis/sockets/SocketManager.ts (1)
26-32: 불필요한Omit타입 사용
SocketOptions인터페이스에onConnect속성이 없으므로Omit<SocketOptions, 'onConnect'>는 불필요합니다. 코드 가독성을 위해 단순화하는 것이 좋겠습니다.♻️ 제안 수정
-const DEFAULT_OPTIONS: Required<Omit<SocketOptions, 'onConnect'>> = { +const DEFAULT_OPTIONS: Required<SocketOptions> = { maxRetries: 3, baseRetryDelayMs: 1000, heartbeatInMs: 10000, heartbeatOutMs: 10000, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apis/sockets/SocketManager.ts` around lines 26 - 32, The DEFAULT_OPTIONS constant currently uses an unnecessary Omit (Required<Omit<SocketOptions, 'onConnect'>>): remove the Omit and simplify the type to Required<SocketOptions> (or an appropriate Required<Pick<SocketOptions, 'maxRetries'|'baseRetryDelayMs'|'heartbeatInMs'|'heartbeatOutMs'>> if you want to be explicit), update the DEFAULT_OPTIONS declaration to use that simplified type, and ensure the object shape remains unchanged; reference the DEFAULT_OPTIONS constant and the SocketOptions type when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apis/sockets/SocketManager.ts`:
- Around line 102-103: The code builds wsUrl from
import.meta.env.VITE_API_BASE_URL without validating it, which yields
"undefined/ws" if the env var is missing; in SocketManager.ts locate the wsUrl
construction (symbol: wsUrl) and add a check that
import.meta.env.VITE_API_BASE_URL is a non-empty string, and if not either throw
a clear Error or call the module's logger (or throw new Error) with a message
like "VITE_API_BASE_URL is not set" before attempting to use wsUrl; ensure the
check prevents creating the WebSocket URL and downstream connection attempts
when the env is invalid.
- Around line 214-222: When max retries are exceeded in SocketManager (check:
retryCount vs currentOptions.maxRetries), deactivate() alone can still allow the
stale client's onWebSocketClose to fire and re-enter handleReconnection; after
calling this.client.deactivate() set this.client = null (or otherwise mark the
manager as permanently closed and remove the client's listeners) to prevent the
stale client from passing the `this.client !== newClient` check and
re-triggering reconnection logic (update the block that sets
this.client.reconnectDelay and calls this.client.deactivate()).
---
Nitpick comments:
In `@src/apis/sockets/SocketManager.ts`:
- Around line 26-32: The DEFAULT_OPTIONS constant currently uses an unnecessary
Omit (Required<Omit<SocketOptions, 'onConnect'>>): remove the Omit and simplify
the type to Required<SocketOptions> (or an appropriate
Required<Pick<SocketOptions,
'maxRetries'|'baseRetryDelayMs'|'heartbeatInMs'|'heartbeatOutMs'>> if you want
to be explicit), update the DEFAULT_OPTIONS declaration to use that simplified
type, and ensure the object shape remains unchanged; reference the
DEFAULT_OPTIONS constant and the SocketOptions type when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7ee7100-a84b-47b0-b775-e86d3b7ec44d
📒 Files selected for processing (1)
src/apis/sockets/SocketManager.ts
i-meant-to-be
temporarily deployed
to
PREVIEW_ENV
GitHub Actions
Inactive
Contributor
Author
|
/noti @jaeml06 바쁘실 것으로 알아 죄송합니다만, 로직에 조금 규모 있는 변경이 있어 리뷰를 다시 요청드립니다. 변경 사항을 요약해드리면 안정적인 재시도 및 구독 유지를 위한 관찰자 패턴(observer pattern) 도입이며, 보셔야 할 코드는 다음과 같습니다:
위에 적어둔 부분만 확인해주시면 됩니다! 다른 부분에 대한 노티를 드려야 할 정도로 큰 변경은 이것 말고는 없습니다. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚩 연관 이슈
closed #433
📝 작업 내용
웹 소켓 통신을 위한 기반 코드를 추가합니다! 크게 아래의 내용들로 구성되어 있습니다:
SocketManager구현useSocket구현또한 대부분의 함수에 JSDoc을 통해 자세한 함수 사용 방법을 남겨 두었습니다. 아무래도 웹 소켓 관련 기술 논의가 저랑 콜리 사이에서만 진행되었고 그 내용이 프론트에 명시적으로 공유된 게 아니기 때문에, 코드 리뷰 또는 향후 제가 작업한 소켓 코드들을 재활용하실 때 모르는 내용이 있으실까봐 최대한 설명을 상세히 적어 두었으니 참고 바랍니다. 그럼에도 궁금한 점이 있으시다면, PR 리뷰나 Discord를 통해 편하게 질의 남겨주세요.
소켓 구현을 위한 타입 정의
소켓 통신만을 위한 타입과 인터페이스를 src/apis/sockets/type.ts에 정의했습니다. 소켓 이벤트는 크게 타이머 데이터가 담기는 "타이머 이벤트"와 타이머 데이터가 담기지 않는 "비-타이머 이벤트"로 나뉘는데요. 각 이벤트들에 대한 설명은 아래와 같습니다:
타이머 이벤트 타입
TimerEventTypesNEXT다음 차례로 넘기기STOP타이머 정지BEFORE이전 차례로 돌리기PLAY타이머 시작RESET타이머 초기화TEAM_SWITCH자유 토론 중 팀 전환비-타이머 이벤트 타입
NonTimerEventTypeFINISHED토론 종료ERROR서버 측에서 발생한 오류소켓 메시지 전송을 위한 페이로드 인터페이스 정의
{ "eventType": "NEXT", "data" : { "timerType": "NORMAL", "sequence": 2, "currentTeam": null, "remainingTime": 180 } } { "eventType": "FINISHED", "data" : null }소켓 통신 시 서버와 주고받을 메시지의 형식을 src/apis/sockets/type.ts에 정의했습니다. 기본적으로 모든 페이로드는 이벤트와 관계 없이 아래와 공통 구조를 가집니다:
공통 메시지 구조
SocketMessage주목하셔야 할 부분은 타이머 이벤트 타입에 따라
data가 달라진다는 거예요! 만약 이벤트가 타이머 정보를 필요로 하지 않는NonTimerEventType일 경우data는null로 지정돼요. 그러나 타이머를 조작하는 등 타이머 정보를 필요로 하는 경우,data는 바로 아래에 있는 타이머 데이터를 기술하는TimerDataPayload로 지정됩니다.타이머 정보를 담는 페이로드
TimerDataPayload소켓 연결을 담당하는
SocketManager구현소켓 연결을 직접 담당하는
SocketManager를 src/apis/sockets/SocketManager.ts에 구현했습니다. 이 매니저는 아래와 같은 기술적 특징을 지녀요:싱글톤 구현
이 매니저는 싱글톤으로 작성되어 있습니다. 생성자는 외부에서 접근 불가능하게
private로 막혀 있고, 클래스 수준 함수인getInstance를 통해서만 인스턴스를 공급받을 수 있어요.소켓 설정
SocketOptions소켓 사용 시 정책을 코드 안에서 유연하게 바꿀 수 있도록 인터페이스
SocketOptions를 추가했습니다. 각 옵션과 설명은 아래와 같아요:maxRetires최대 재시도 횟수 (기본값 3회)baseRetryDelayMs기본 재시도 대기 시간 (기본값 1000 ms)heartbeatInMs수신 하트비트 주기 (기본값 10000 ms)heartbeatOutMs발신 하트비트 주기 (기본값 10000 ms)참고로, 이 모든 기본값들은 콜리와의 회의를 통해 저희 서비스 정책으로 고정된 것들입니다. 아마 웬만해서는 변동 가능성이 없을 것 같아요.
재시도 정책
재시도는 아래 정책에 따릅니다:
1초 * 2 ^ 누적 재시도 횟수로 계산또한 재시도를 위해
SocketManager와 이를 사용하는 훅useSocket간에 관찰자 패턴(observer pattern)을 적용했습니다.useSocket과 훅을 통해 수립된 구독들SocketManager동작 방식은 아래와 같습니다:
React 단에서 소켓 사용을 위해 React 훅
useSocket구현React에서 소켓을 사용하기 위해
SocketManager를 더 고수준에서 다루는 훅useSocket을 src/hooks/useSockets.ts에 추가했습니다.이 훅은
SocketManager가 지원하는 여러 함수를 React 컴포넌트 수준에서 안전히 다룰 수 있게 해 주며, 무엇보다도useEffect에 언마운트 시 소켓 연결을 끊도록 명시적으로 지정해 주어, 언마운트나 페이지 이동 시에도 메모리 누수가 발생하지 않게 구현되어 있습니다.그 외 변경 사항
보안 문제 해결을 위한 패키지 의존성 업데이트 진행
🏞️ 스크린샷 (선택)
없음
🗣️ 리뷰 요구사항 (선택)
로직 및 테스트 스위트에 문제 없을지 확인해주시면 감사하겠습니다!
Summary by CodeRabbit
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항
테스트
Chores