Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Chat] 전송 실패 메시지 처리 방식 변경 #2938

Merged
merged 5 commits into from
Oct 13, 2023
Merged

Conversation

guswl98
Copy link
Contributor

@guswl98 guswl98 commented Oct 10, 2023

PR 설명

기존 코드에서 전송 실패한 메시지를 재전송할 때 정상 메시지들이 모두 없어지는 오류가 있었습니다. 현재는 reducer의 messages 내에서 정상 메시지들과 전송 실패 메시지들을 한 번에 다루고 있습니다. 전송 실패 메시지를 재전송 후 전송이 성공했을 때, 재전송에 성공한 메시지를 필터링하는 과정에서 조건이 잘못 걸려 이전 메시지들이 모두 사라지고 있었습니다.

필터링 조건의 간소화와 가독성, 메시지들의 유지 보수의 간편화를 위해 reducer에 failedMessages 상태를 추가하고, REMOVE_FROM_FAILED 액션을 추가했습니다.

변경 내역

  • onRetry, onCancel 적용
  • reducer에 failed 메시지 관련 상태 및 액션 추가

체크리스트

스크린샷 & URL

@guswl98 guswl98 self-assigned this Oct 10, 2023
@guswl98 guswl98 added the chat label Oct 10, 2023
@guswl98 guswl98 added this to the v13.7.0 milestone Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (ce8bd99) 12.21% compared to head (a2bba8f) 12.20%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2938      +/-   ##
==========================================
- Coverage   12.21%   12.20%   -0.01%     
==========================================
  Files         685      685              
  Lines       36113    36124      +11     
  Branches     8919     8920       +1     
==========================================
  Hits         4410     4410              
- Misses      31696    31707      +11     
  Partials        7        7              
Files Coverage Δ
packages/chat/src/chat-bubble/chat-bubble.tsx 0.00% <ø> (ø)
packages/chat/src/chat/chat-container.tsx 0.00% <ø> (ø)
packages/chat/src/types/index.ts 0.00% <ø> (ø)
packages/chat/src/chat/reducer.ts 0.00% <0.00%> (ø)
packages/chat/src/chat/chat.tsx 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guswl98
Copy link
Contributor Author

guswl98 commented Oct 10, 2023

release-canary

@github-actions
Copy link

v13.6.1-pr-2938.9 has been published!

@guswl98 guswl98 force-pushed the feature/chat/failed branch from ff122fb to 7335ac9 Compare October 11, 2023 01:57
@guswl98 guswl98 requested a review from dongoc October 11, 2023 04:03
@guswl98 guswl98 marked this pull request as ready for review October 11, 2023 04:03
@guswl98 guswl98 requested a review from a team as a code owner October 11, 2023 04:03
@guswl98 guswl98 requested review from gigibean and seulgiyoon and removed request for a team October 11, 2023 04:03
</li>
))}
</ul>
<ul id="failed_messages_list">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messages와 failedMessages의 렌더링 차이가 없어보여서 [...messages, ...failedMessages]로 합쳐서 렌더링해도 괜찮을 것 같아요!
아니면 ul 태그의 id를 사용하는 경우가 있어 별도로 렌더링을 하는 방법을 택하신 걸까요?👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chat-web은 메시지의 아이디가 숫자로 1씩 올라가게 됩니다. 시간을 기반으로 getTime()을 한 숫자와 많은 차이가 있겠지만 혹시 id가 겹칠 염려가 있어 두 개를 분리해두었습니다. 추후 메시지 id를 따라 스크롤이 된다거나 하는 등의 기능이 추가될 때를 대비한 바도 있습니다!

@guswl98 guswl98 force-pushed the feature/chat/failed branch from 7335ac9 to a2bba8f Compare October 11, 2023 06:57
@guswl98
Copy link
Contributor Author

guswl98 commented Oct 11, 2023

release-canary

@github-actions
Copy link

v13.6.1-pr-2938.12 has been published!

@@ -43,7 +43,7 @@ export interface RoomListResultWithPagingInterface
export interface RoomMetadata {
name: string
memberCounts: number
faqId?: string
articleId?: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 타입이 변경된 히스토리를 알고 싶습니다!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2939
앗 여기 PR에 설명이 있군요 브랜치가 섞인 걸까요??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 cananry 충돌을 방지하기 위해 #2939 내용을 추가하신 것 같네요..!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chat-web에서 카나리 테스트를 할 때 타입 오류를 방지하기 위해 해당 브랜치 리베이스를 진행했었습니다...!

@guswl98 guswl98 merged commit 7f4a9c8 into main Oct 13, 2023
@guswl98 guswl98 deleted the feature/chat/failed branch October 13, 2023 04:20
@dongoc dongoc mentioned this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants