QA 후속 작업 진행 - #308
Merged
Merged
Conversation
- 낙관적 업데이트 도입 후, 끄기 요청 진행 중 바로 다시 누르면 두 번째 호출이 켜기 분기로 들어가 두 비동기 흐름이 동시에 진행되고 늦게 끝나는 쪽이 최신 의도를 덮어쓸 수 있었음(예: 토큰은 삭제됐는데 UI는 켜짐으로 표시). - 요청별 버전 번호를 두어, 더 최신 토글이 시작된 뒤에 끝난 완료는 상태/localStorage를 건드리지 않도록 수정.
- removeFcmToken(서버 토큰 삭제)과 localStorage.setItem(로컬 마커 저장)이 같은 try 블록에 있어, 저장만 실패해도 서버 삭제 성공 여부와 무관하게 토글이 켜짐으로 롤백되고 거짓 실패 토스트가 뜨던 문제 수정 - 두 작업의 실패 처리를 분리: 서버 삭제 실패 시에만 롤백 + 에러 토스트, 로컬 저장 실패는 Sentry로만 기록하고 토글 상태는 유지
- Capacitor 네이티브에서 헤더와 동일한 상태바 높이만큼 스포트라이트를 추가로 내리도록 했던 보정(isBottomNavTarget)이 실기기에서 과하게 적용되어 화살표가 실제 타겟보다 훨씬 아래를 가리키는 문제 발생. - 원인을 확실히 특정하지 못한 상태였고 실측 값(getBoundingClientRect)만 사용하는 이전 방식이 더 안전하다고 판단해 관련 코드를 전부 되돌림 — CoachmarkStep.isBottomNavTarget 필드, 5개 스텝의 플래그, nativeStatusBar.ts 유틸 삭제. yOffset은 다시 step.yOffsetMobile만 반영.
- 권한이 granted된 이후 단계(서비스워커 등록, Firebase 초기화 등)에서 실패하면 pushEnabled를 메모리에서만 false로 되돌려, 새로고침 시 초기화 로직이 granted 권한만 보고 다시 켜짐으로 되살리던 문제 수정 - 롤백 시 push_notifications_disabled 마커도 함께 기록해 재마운트 후에도 실패 상태가 유지되도록 함
H-sooyeon
added
🐞 bug
Contributor
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesPush notification toggle
Coachmark offset simplification
Login service introduction styling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Setting
participant PushPermissionAPI
participant localStorage
User->>Setting: Toggle push notifications
Setting->>PushPermissionAPI: Check or request permission
PushPermissionAPI-->>Setting: Return permission result
Setting->>localStorage: Persist current disabled-state marker
Setting-->>User: Apply non-stale result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Coverage Report for Frontend (frontend)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/app/`(main)/profile/_components/Setting.tsx:
- Around line 215-219: Update the registration success flow in Setting so
failures from localStorage marker removal at the existing removeItem calls do
not enter the rollback that calls setPushEnabled(false) or persists
push_notifications_disabled. Handle marker removal with its own best-effort
try/catch, and retain the enabled state after successful token registration.
- Around line 59-61: Update the initial permission hydration effect to capture
pushRequestIdRef.current when the async load begins, then guard all hydration
state and localStorage writes with a check that the captured ID is still
current. Ensure a toggle that increments pushRequestIdRef causes stale hydration
results to be ignored, while unchanged hydration behavior remains intact.
- Around line 149-150: The push toggle flow around requestId and isStale must
serialize server-side mutations, not merely suppress stale UI updates. Queue
enable/disable operations per platform/user so each mutation completes before
the next begins, preserving the latest toggle order while retaining stale-result
protection for UI work; alternatively enforce an equivalent request revision in
the notification API, including removeFcmToken’s unconditional deletion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89a7430d-d1dd-495d-8765-024f935664ec
⛔ Files ignored due to path filters (1)
mobile-app/ios/App/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
frontend/src/app/(login)/login/_components/ServiceIntro.tsxfrontend/src/app/(main)/_components/homeCoachmarkSteps.tsfrontend/src/app/(main)/profile/_components/Setting.tsxfrontend/src/app/(post)/group/[groupId]/(root)/_components/groupDetailCoachmarkSteps.tsfrontend/src/components/Coachmark.tsxfrontend/src/hooks/useCoachmark.tsfrontend/src/lib/utils/nativeStatusBar.ts
💤 Files with no reviewable changes (3)
- frontend/src/lib/utils/nativeStatusBar.ts
- frontend/src/app/(post)/group/[groupId]/(root)/_components/groupDetailCoachmarkSteps.ts
- frontend/src/hooks/useCoachmark.ts
- 마운트 시 실행되는 권한 하이드레이션 effect가 pushRequestIdRef를 확인하지 않고 무조건 상태를 써서, 하이드레이션이 끝나기 전에 사용자가 토글하면 뒤늦게 도착한 하이드레이션 결과가 방금의 낙관적 업데이트를 오래된 스냅샷으로 덮어쓸 수 있음 - 하이드레이션 시작 시점의 요청 번호를 캡처해 그 사이 토글이 일어났으면 완료 시점의 상태 갱신을 건너뛰도록 수정.
- isStale()이 완료 후 UI 반영만 막을 뿐 네트워크 요청 자체는 막지 않아, disable/enable이 겹쳐 나가면 늦게 도착한 예전 요청이 최신 요청 이후 서버에 반영되며 UI와 서버 상태가 어긋날 수 있었음 (예: UI는 켜짐인데 서버엔 토큰 없음). - 서버로 나가는 등록/삭제 요청을 Promise 체인으로 직렬화해, 항상 가장 최근 토글의 요청이 마지막에 서버에 반영되도록 수정.
- localStorage.removeItem('push_notifications_disabled')이 토큰 등록 성공 이후에 실행되는데도 바깥의 실패 롤백 try/catch 안에 있어서, 마커 제거만 실패해도 서버엔 토큰이 등록된 채로 UI는 꺼짐 처리되고
disabled 마커까지 저장되는 불일치가 있었음.
- 마커 제거를 자체 best-effort try/catch로 분리해 등록 성공 시 켜짐 상태를 유지하도록 수정.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/app/`(main)/profile/_components/Setting.tsx:
- Around line 195-214: Update registerAndroidToken and its registration listener
so failures from registerFcmToken are propagated instead of swallowed by the
current catch. Ensure the Settings enable flow around registerAndroidToken
treats Android registration failure as an error, reaches its existing outer
failure handling, and persists push_notifications_disabled rather than leaving
the toggle enabled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dbb50a69-fc4d-4f77-98f8-a348096be2f9
📒 Files selected for processing (1)
frontend/src/app/(main)/profile/_components/Setting.tsx
- registerAndroidToken()이 registerFcmToken 실패/registrationError/ 타임아웃을 전부 조용히 삼키고 항상 정상 종료해서, Settings의 enable 흐름이 OS 권한만 확인하고 서버 등록 실패를 감지하지 못해 토글이 켜진 채로 남고 서버엔 토큰이 없는 상태가 될 수 있었음. - registerAndroidToken()이 실제 등록 성공 여부를 boolean으로 반환하도록 바꾸고, Settings에서 권한은 허용됐지만 등록이 실패한 경우 기존 실패 처리 경로(롤백/마커 저장)로 넘어가도록 수정. - registerFcmToken 실패 시 false를 반환하는지 확인하는 회귀 테스트 추가.
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
1 participant
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.
요약 (연관 이슈 번호 포함)
작업 내용 + 스크린샷
푸시 알림 토글 안정성
랜딩 페이지
sm 미만 너비에서 텍스트가 이미지 대비 과도하게 크게 보이던 문제 수정
코치마크
실제 걸린 시간
3h
작업하며 고민했던 점(선택)
테스트 실행 여부
리뷰 참고사항(선택)
시각 자료(이미지/영상, 있다면)(선택)
Summary by CodeRabbit
sizeshandling) for more consistent rendering.