[Feature/#213] 설정 페이지 탭 UI 및 알림 채널 설정 추가#216
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough설정 페이지에 브라우저 푸시·이메일 알림 채널 UI를 추가하고, Setting을 프로필/비밀번호/알림으로 분리해 알림 상태를 draft/saved로 관리하며 사이드바의 알림 항목을 제거했습니다. 변경 사항설정 페이지 탭화 및 알림 채널 통합
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant NotificationSection as NotificationSection UI
participant SettingPage as Setting.tsx
participant State as draftNotification / savedNotification
participant Toast as toast
User->>NotificationSection: 토글 클릭 (브라우저푸시/이메일)
NotificationSection->>SettingPage: on*Change(반전값)
SettingPage->>State: draftNotification 업데이트
Note right of SettingPage: 저장 시 handleSave에서\nsavedNotification 갱신 / updateMyInfo 분기
SettingPage->>Toast: 성공 메시지 표시
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
제안 리뷰어
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
📚 Storybook 배포 완료
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/setting/NotificationSection.tsx (1)
25-27: ⚡ Quick win헤더의 BellIcon에 색상 클래스가 누락되었습니다.
Line 32의 BellIcon은
text-text-muted클래스가 적용되어 있지만, 헤더의 BellIcon에는 색상 클래스가 없습니다. 코딩 가이드라인에 따라 SVG 요소에는text-*클래스를 통해 색상을 적용해야 합니다.🔧 색상 클래스 추가 제안
<header className="mb-7 flex items-center gap-4"> - <BellIcon /> + <BellIcon className="h-6 w-6 text-text-title" /> <h2 className="font-heading4 text-text-title">알림 채널 설정</h2> </header>As per coding guidelines: "For SVG elements, use
fill/stroke="currentColor"and apply colors viatext-*classes only."🤖 Prompt for 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. In `@src/components/setting/NotificationSection.tsx` around lines 25 - 27, The BellIcon in NotificationSection (inside the header element) is missing the color class; update the JSX rendering of BellIcon in NotificationSection.tsx to apply the same text-* color class used elsewhere (e.g., text-text-muted) so the SVG inherits color via currentColor per guidelines; locate the BellIcon usage in the header and add the appropriate className prop (matching other instances) so the SVG uses fill/stroke="currentColor" and the color is applied via Tailwind text-* classes.src/pages/setting/Setting.tsx (1)
192-210: ⚡ Quick win탭 접근성 패턴이 불완전합니다.
현재
role="tab"과aria-selected가 적용되어 있지만, WAI-ARIA 탭 패턴을 완전히 준수하려면 추가 속성이 필요합니다:
- 탭 버튼들의 컨테이너에
role="tablist"필요- 각 탭 콘텐츠 영역에
role="tabpanel"및aria-labelledby필요aria-controls로 탭과 패널 연결♿ 접근성 개선 예시
- <nav aria-label="설정 탭" className="flex border-b border-surface-300"> + <nav aria-label="설정 탭" role="tablist" className="flex border-b border-surface-300"> {TABS.map((tab) => ( <button key={tab.id} type="button" role="tab" aria-selected={activeTab === tab.id} + aria-controls={`panel-${tab.id}`} + id={`tab-${tab.id}`} onClick={() => setActiveTab(tab.id)} ... >콘텐츠 영역에도 적용:
<div role="tabpanel" id={`panel-${activeTab}`} aria-labelledby={`tab-${activeTab}`} className="flex flex-col gap-6" >As per coding guidelines: "접근성: 시맨틱 HTML, ARIA 속성 사용 확인."
🤖 Prompt for 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. In `@src/pages/setting/Setting.tsx` around lines 192 - 210, The nav and buttons implement part of the ARIA tabs pattern but miss container and panel linkage: add role="tablist" to the nav wrapping TABS, give each tab button a stable id (e.g., tab-{tab.id}) and aria-controls pointing to the matching panel id (panel-{tab.id}) while retaining aria-selected and onClick in setActiveTab, and ensure each tab content container uses role="tabpanel", id="panel-{activeTab}" and aria-labelledby="tab-{activeTab}" to fully connect tabs and panels (update components that render the panel for activeTab to include these attributes).
🤖 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 `@src/pages/setting/Setting.tsx`:
- Around line 85-86: The hasChanges computation incorrectly treats the
"notifications" tab the same as "security"; update the logic in the Setting
component so hasChanges checks activeTab explicitly for "profile" (use
hasProfileChanges), for "security" (use hasPasswordChanges), and returns false
for "notifications" (or any other tabs) so the Save button remains disabled
until API integration. Modify the expression that assigns hasChanges
(referencing activeTab, hasProfileChanges, hasPasswordChanges) to branch for
"profile", "security", and default to false for notifications.
- Around line 257-259: The notifications tab is missing the loading-state guard:
when activeTab === "notifications" you should mirror the Profile/Security tab
behavior by showing the skeleton while isLoading is true and only render
<NotificationSection email={draftProfile.email} /> once loading finishes; update
the conditional that renders NotificationSection to check !isLoading and, inside
NotificationSection props, ensure draftProfile.email is used with a safe
fallback (e.g., empty string or undefined) so the UI doesn't break when
draftProfile.email is empty; look for activeTab, isLoading, draftProfile.email
and NotificationSection in Setting.tsx to implement the change.
---
Nitpick comments:
In `@src/components/setting/NotificationSection.tsx`:
- Around line 25-27: The BellIcon in NotificationSection (inside the header
element) is missing the color class; update the JSX rendering of BellIcon in
NotificationSection.tsx to apply the same text-* color class used elsewhere
(e.g., text-text-muted) so the SVG inherits color via currentColor per
guidelines; locate the BellIcon usage in the header and add the appropriate
className prop (matching other instances) so the SVG uses
fill/stroke="currentColor" and the color is applied via Tailwind text-* classes.
In `@src/pages/setting/Setting.tsx`:
- Around line 192-210: The nav and buttons implement part of the ARIA tabs
pattern but miss container and panel linkage: add role="tablist" to the nav
wrapping TABS, give each tab button a stable id (e.g., tab-{tab.id}) and
aria-controls pointing to the matching panel id (panel-{tab.id}) while retaining
aria-selected and onClick in setActiveTab, and ensure each tab content container
uses role="tabpanel", id="panel-{activeTab}" and
aria-labelledby="tab-{activeTab}" to fully connect tabs and panels (update
components that render the panel for activeTab to include these attributes).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61e3e172-62cd-4710-a436-fcae55ce6d9b
⛔ Files ignored due to path filters (1)
src/assets/logo/social-logo/plain/slack.svgis excluded by!**/*.svgand included bysrc/**
📒 Files selected for processing (3)
src/components/setting/NotificationSection.tsxsrc/constants/sidebarNav.tssrc/pages/setting/Setting.tsx
💤 Files with no reviewable changes (1)
- src/constants/sidebarNav.ts
Collaborator
|
P2: 현재 보안/알림 페이지는 각 페이지에 카드가 하나씩만 있는 구조라, 탭으로 페이지를 분리하는 것은 오버 엔지니어링에 가까워 보입니다. 사용자가 소속된 조직 정보는 이미 워크스페이스 영역에서 확인 가능하므로, 설정 페이지에 중복해서 포함할 필요는 없을 것 같습니다. |
Collaborator
|
P2: 생각해보니 워크스페이스 화면이 존재해서 설정에서 소속 조직은 빼도 될 것 같습니다. 그렇게 되면 탭 없이 한페이지에 배치해도 괜찮을 것 같아요! |
Collaborator
Author
|
저도 중첩 nav랑 세부url 둘다 애매해서 고민중이었는데, 의견 감사합니다! |
Collaborator
Author
탭형식 제거하여 수정 진행했습니다! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/setting/Setting.tsx (1)
121-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick win저장 중 재클릭을 막는 상태가 필요합니다.
현재
isLoading은 초기 조회에만 쓰이고 있어서updateMyInfo요청이 진행되는 동안에도 버튼이 계속 활성화됩니다. 그 상태에서 연속 클릭하면 프로필/비밀번호 수정 요청이 중복으로 날아가고, 토스트/로컬 상태도 서로 엇갈릴 수 있습니다.🔧 제안 수정
+ const [isSaving, setIsSaving] = useState(false); + const handleSave = async () => { + if (isSaving) return; + if (hasPasswordChanges) { const errors = validatePassword(); setPasswordErrors(errors); if (Object.values(errors).some(Boolean)) return; } else { @@ } try { + setIsSaving(true); + if (hasAccountChanges) { const res = await updateMyInfo({ name: draftProfile.name, oldPassword: currentPassword || undefined, newPassword: newPassword || undefined, @@ } catch (e) { const error = e as IApiErrorResponse; toast.error(error.message ?? "회원정보수정에 실패했습니다"); + } finally { + setIsSaving(false); } }; @@ - disabled={!hasChanges || isLoading} + disabled={!hasChanges || isLoading || isSaving}Also applies to: 255-263
🤖 Prompt for 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. In `@src/pages/setting/Setting.tsx` around lines 121 - 173, The save flow in handleSave allows duplicate requests because the existing isLoading only covers initial fetch; add a dedicated "isSaving" (or reuse isLoading but ensure semantics) boolean state and guard at the top of handleSave to return early if isSaving is true, set isSaving = true before awaiting updateMyInfo (and other async work), and set isSaving = false in a finally block so UI and toast updates can't interleave; also update the Save button (and the other handler at the 255-263 region) to use this isSaving flag to disable clicks while the request is in progress.
♻️ Duplicate comments (1)
src/pages/setting/Setting.tsx (1)
87-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick win알림 설정은 아직 서버에 저장되지 않는데 저장 완료로 확정하고 있습니다.
hasChanges에 알림 변경을 포함해서 저장 버튼을 활성화하고,handleSave에서는setSavedNotification(...)만 한 뒤 성공 토스트를 띄우고 있습니다. 지금 상태로는 새로고침하면 값이 사라질 수 있는데도 사용자는 저장된 줄 알게 됩니다. 계정 정보와 함께 저장하는 경우에도 알림까지 반영된 것처럼 보입니다. API 연동 전까지는 알림 변경만으로 저장 가능 상태가 되지 않게 막거나, 최소한 “준비 중”으로 분리해 주세요.Also applies to: 157-168, 255-263
🤖 Prompt for 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. In `@src/pages/setting/Setting.tsx` around lines 87 - 90, The code incorrectly treats notification-only edits as saved: change the save logic so notification changes don't enable the main "Save" flow until API support exists by removing hasNotificationChanges from the main hasChanges/Save gating (leave hasAccountChanges = hasProfileChanges || hasPasswordChanges and set hasChanges = hasAccountChanges only), and introduce a separate flag (e.g., hasPendingNotificationChanges or keep hasNotificationChanges) to track notification edits; update handleSave (the function that calls setSavedNotification and shows the success toast) so it does not show a full-success toast or mark the overall form saved when only notification changes exist—either disable the save button for notification-only changes or show a distinct “notification changes pending / saving not yet supported” UI message instead. Ensure references to setSavedNotification, hasChanges, hasAccountChanges, hasNotificationChanges, and handleSave are updated consistently and that the Save button enabling logic uses the new separation.
🤖 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 `@src/components/setting/NotificationSection.tsx`:
- Around line 76-78: The "연동" Button in NotificationSection is currently
interactive but not implemented; update the Button element in
NotificationSection.tsx (the Button that renders "연동") to reflect a
disabled/unavailable state until Slack OAuth is implemented: either add the
disabled prop to the Button and optionally change its label to "준비 중" (or render
a tooltip/aria-label explaining it's coming soon) so it appears non-clickable
and accessible; ensure any onClick handler is removed or guarded so no action is
attempted while disabled.
- Around line 44-49: The badge currently shows "권한 허용됨" based solely on the
boolean browserPush which is the user setting, not the browser permission;
change the UI so it doesn't assert granted permission: either (A) only render
the Badge when both browserPush is true and the actual permission is confirmed
(check Notification.permission === 'granted' or accept a new prop like
browserPermission === 'granted'), or (B) keep the Badge but change its text to a
non-assertive state like "알림 켜짐" / "설정 활성화됨" and remove any wording that claims
permission; update the render around Badge, Toggle and onBrowserPushChange
accordingly.
---
Outside diff comments:
In `@src/pages/setting/Setting.tsx`:
- Around line 121-173: The save flow in handleSave allows duplicate requests
because the existing isLoading only covers initial fetch; add a dedicated
"isSaving" (or reuse isLoading but ensure semantics) boolean state and guard at
the top of handleSave to return early if isSaving is true, set isSaving = true
before awaiting updateMyInfo (and other async work), and set isSaving = false in
a finally block so UI and toast updates can't interleave; also update the Save
button (and the other handler at the 255-263 region) to use this isSaving flag
to disable clicks while the request is in progress.
---
Duplicate comments:
In `@src/pages/setting/Setting.tsx`:
- Around line 87-90: The code incorrectly treats notification-only edits as
saved: change the save logic so notification changes don't enable the main
"Save" flow until API support exists by removing hasNotificationChanges from the
main hasChanges/Save gating (leave hasAccountChanges = hasProfileChanges ||
hasPasswordChanges and set hasChanges = hasAccountChanges only), and introduce a
separate flag (e.g., hasPendingNotificationChanges or keep
hasNotificationChanges) to track notification edits; update handleSave (the
function that calls setSavedNotification and shows the success toast) so it does
not show a full-success toast or mark the overall form saved when only
notification changes exist—either disable the save button for notification-only
changes or show a distinct “notification changes pending / saving not yet
supported” UI message instead. Ensure references to setSavedNotification,
hasChanges, hasAccountChanges, hasNotificationChanges, and handleSave are
updated consistently and that the Save button enabling logic uses the new
separation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fae24040-22af-4995-bfb2-cba31cc62dcb
📒 Files selected for processing (4)
src/components/setting/NotificationSection.tsxsrc/components/setting/ProfileSection.tsxsrc/components/setting/ProfileSectionSkeleton.tsxsrc/pages/setting/Setting.tsx
💤 Files with no reviewable changes (2)
- src/components/setting/ProfileSectionSkeleton.tsx
- src/components/setting/ProfileSection.tsx
Collaborator
|
P4: 가로 너비가 길어서 레이아웃이 어색해 보이는 듯합니다! 아래 처럼 구성하면 어떨까요?
|
Collaborator
Author
수정하는게 더 나을듯합니다 반영하겠습니다! |
Collaborator
Author
비밀번호 section과 알림 채널 설정이 같은 라인에 배치하는 방향으로 구현해보니, 둘 사이에 내부 내용의 높이 차이가 있어 변경하지 않고 현재 스타일로 유지하였습니다.
현재로써는 내부의 내용의 높이 차이가 있어서 다른 열로 두는게 더 자연스러워보입니다.
추후에 알림 채널 설정 내부에 추가 내용이 생겨 내부 높이가 길어지면 같은 열로 두는것을 고려해보겠습니다!
|
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
3 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 #213
✨ 변경사항
✏️ 작업 내용
💻 작업 화면
알림 설정 화면
😅 미완성 작업
📢 논의 사항 및 참고 사항
알림 탭에서 저장버튼이 현재는 disabled 상태입니다. API연동하면 활성화될예정입니다.
알림 설정은 프로필과 비밀번호변경과 마찬가지로 즉시저장이 아니라 저장 버튼 방식으로 일관성을 높였습니다.
UI 변경관련
+) 추가로 profileSection에 소속 조직 나열을 아코디언으로 펼치고 닫는 구현은 추가 PR로 올릴예정입니다
수정사항 : 탭형식 제거. profileSection내 소속 조직정보 제거. 알림토글 상태변경시 저장하기 버튼이용하여 저장되도록 구현.
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항