[FIX] 타이머 화면의 음소거 아이콘 미반영 버그 및 NormalTimer 레이아웃 문제를 수정#442
[FIX] 타이머 화면의 음소거 아이콘 미반영 버그 및 NormalTimer 레이아웃 문제를 수정#442
Conversation
spec, plan, research, data-model, test-contracts, tasks, checklists 포함 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 팀명/토론자 각각 독립 DOM 요소 렌더링 검증 - 팀명 요소 truncate, 토론자 요소 truncate 없음 검증 - h1 text-center 클래스 존재 검증 (한글/영어 순서명) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- DTDebate 아이콘 제거 - 제목 텍스트 중앙 정렬(text-center) 적용 - 팀명과 발언자를 각 줄로 분리, 중간 구분선 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- volume === 0 일 때 RiVolumeMuteFill 아이콘 표시 - 볼륨 있을 때 기존 DTVolume 아이콘 유지 - 각 아이콘에 data-testid 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 볼륨 > 0일 때 일반 아이콘 표시 확인 - 볼륨 = 0일 때 음소거 아이콘 표시 확인 - 음소거 버튼 클릭 시 아이콘 즉시 변경 확인 - 음소거 해제 시 아이콘 복원 확인 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
개요음소거 상태 변경 시 헤더 아이콘 반영, NormalTimer의 팀/토론자 정보를 별도 줄로 분리 표시, 제목 정렬 개선, 관련 번역 추가 및 테스트 커버리지 확대. 변경사항
예상 코드 리뷰 난이도🎯 3 (중간) | ⏱️ ~20분 관련 가능성 있는 PR
추천 검수자
시
🚥 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 |
🚀 Preview 배포 완료!
|
jaeml06
changed the title
![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 mute icon in the header that reflects the volume state and refactors the NormalTimer component to a two-line centered layout for improved readability. It also updates the repository's bash scripts to support categorized specifications (e.g., fix, feat) and adds comprehensive documentation and TDD-based tests for the changes. Review feedback suggests improving the robustness of specification lookups for legacy branch names and notes the undocumented removal of the DTDebate icon during the UI refactor.
| local type_prefix="feat" # default for legacy branches | ||
| if [[ "$branch_name" =~ ^([a-z]+)/#[0-9]+ ]]; then | ||
| type_prefix="${BASH_REMATCH[1]}" | ||
| fi |
There was a problem hiding this comment.
The type_prefix defaults to "feat" for legacy branch names (those not following the type/#number pattern). This means if a legacy branch (e.g., 441-mute-timer-layout-fix) refers to a specification that has been moved to specs/fix/, this script will fail to locate it. To improve robustness, consider iterating through all subdirectories of specs/ to find a matching issue number if the branch name doesn't explicitly include a type prefix, similar to the logic implemented in get_current_branch.
| {item.speaker && | ||
| t(' | {{speaker}} 토론자', { speaker: t(item.speaker) })} | ||
| </p> | ||
| <span className="flex w-full max-w-[600px] flex-col items-center justify-center gap-y-[8px] xl:gap-y-[12px]"> |
There was a problem hiding this comment.
The DTDebate icon (previously located at line 84 in the old version) has been removed in this refactor. While the transition to a two-line layout improves readability for long team names, the removal of this visual element isn't explicitly mentioned in the PR description or the research document. If the icon was removed intentionally to simplify the UI, please update the documentation to confirm this. Otherwise, consider restoring it (e.g., centered above the team name) to maintain visual consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/page/TimerPage/components/NormalTimer.test.tsx (1)
60-69: 두 요소 동시 표시 케이스에 구분선 검증도 추가하면 더 견고합니다.현재는 팀/토론자 존재만 확인하므로, separator(
<hr>) 존재까지 함께 검증하면 레이아웃 계약 회귀를 더 잘 잡아낼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/page/TimerPage/components/NormalTimer.test.tsx` around lines 60 - 69, The test case in NormalTimer.test.tsx should also assert that the visual separator is rendered when both team and speaker are present: update the test that calls renderNormalTimer('찬성', '발언자 1') to additionally query for the separator element (e.g., getByRole('separator') or getByTestId if the component uses data-testid) and expect it to be in the document; keep the existing assertions for teamEl and speakerEl and the identity check so the test verifies team/speaker DOM nodes and the presence of the <hr> separator produced by the NormalTimer rendering path.src/page/TimerPage/components/NormalTimer.tsx (1)
94-96: 발언자 줄의 긴 무공백 문자열 대비 줄바꿈 유틸 클래스 추가를 고려해 주세요.현재는 truncate를 제거한 의도가 맞지만, 공백 없는 긴 문자열에서 가로 overflow 가능성이 있어
break-words(또는break-all)를 추가하면 UI 안정성이 더 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/page/TimerPage/components/NormalTimer.tsx` around lines 94 - 96, In the NormalTimer component, update the speaker <p> element (the one with className "w-full min-w-0 text-center text-[20px] xl:text-[28px]") to include a word-breaking utility such as "break-words" (or "break-all" if you prefer more aggressive breaking) so long no-space speaker strings cannot overflow; just add the chosen class to that className list to ensure safe wrapping for very long tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/fix/441-mute-timer-layout-fix/test-contracts/NormalTimer.md`:
- Around line 28-30: The example inputs use teamName values that include the
suffix "팀" (e.g., teamName = '찬성 팀'), which will double-render when the
component outputs "{{team}} 팀" and produce "찬성 팀 팀"; update the examples to pass
the raw team identifier (e.g., teamName = '찬성') so that the component's
rendering rule ({{team}} 팀) produces the correct display, and ensure all rows
that reference teamName and speaker in this spec use the raw team name format
consistently.
In `@src/page/TimerPage/TimerPage.test.tsx`:
- Around line 48-81: Add a new test in TimerPage.test.tsx that covers the
missing path of lowering the VolumeBar's range input to 0: render the page with
renderTimerPage(), open the VolumeBar by clicking the volume button
(getByRole('button', { name: '볼륨 조절' })), query the range input (e.g., by role
'slider' or by label used in VolumeBar), fire a change/input event to set its
value to '0', then assert the header icon updates to muted (expect
getByTestId('volume-icon-muted') to be present) and localStorage or component
state reflects 0; reference existing helpers and queries used in the file
(renderTimerPage, volume button selectors, and test ids
'volume-icon-muted'/'volume-icon-normal') when adding the test.
---
Nitpick comments:
In `@src/page/TimerPage/components/NormalTimer.test.tsx`:
- Around line 60-69: The test case in NormalTimer.test.tsx should also assert
that the visual separator is rendered when both team and speaker are present:
update the test that calls renderNormalTimer('찬성', '발언자 1') to additionally
query for the separator element (e.g., getByRole('separator') or getByTestId if
the component uses data-testid) and expect it to be in the document; keep the
existing assertions for teamEl and speakerEl and the identity check so the test
verifies team/speaker DOM nodes and the presence of the <hr> separator produced
by the NormalTimer rendering path.
In `@src/page/TimerPage/components/NormalTimer.tsx`:
- Around line 94-96: In the NormalTimer component, update the speaker <p>
element (the one with className "w-full min-w-0 text-center text-[20px]
xl:text-[28px]") to include a word-breaking utility such as "break-words" (or
"break-all" if you prefer more aggressive breaking) so long no-space speaker
strings cannot overflow; just add the chosen class to that className list to
ensure safe wrapping for very long tokens.
🪄 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: c88f9ce0-2b42-4473-bf75-5a11c7b3dee5
📒 Files selected for processing (13)
.specify/scripts/bash/common.shspecs/fix/441-mute-timer-layout-fix/checklists/requirements.mdspecs/fix/441-mute-timer-layout-fix/data-model.mdspecs/fix/441-mute-timer-layout-fix/plan.mdspecs/fix/441-mute-timer-layout-fix/research.mdspecs/fix/441-mute-timer-layout-fix/spec.mdspecs/fix/441-mute-timer-layout-fix/tasks.mdspecs/fix/441-mute-timer-layout-fix/test-contracts/NormalTimer.mdspecs/fix/441-mute-timer-layout-fix/test-contracts/TimerPage-mute-icon.mdsrc/page/TimerPage/TimerPage.test.tsxsrc/page/TimerPage/TimerPage.tsxsrc/page/TimerPage/components/NormalTimer.test.tsxsrc/page/TimerPage/components/NormalTimer.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.specify/scripts/bash/common.sh (2)
43-43: SC2155: 선언과 할당을 분리하면 에러 마스킹을 방지할 수 있습니다.Line 40의
type_name처리와 일관성을 위해, 명령 실패 시 반환값이 마스킹되지 않도록 분리하는 것이 좋습니다.♻️ 제안된 수정
- local dirname=$(basename "$dir") + local dirname + dirname=$(basename "$dir")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/common.sh at line 43, The assignment local dirname=$(basename "$dir") masks command failures per SC2155; change it to split declaration and assignment so errors aren't hidden by using separate steps: first declare the local variable (local dirname) then assign it (dirname=$(basename "$dir")), and apply the same pattern used for type_name to keep behavior consistent and ensure any failure in basename is not masked.
138-138: SC2155: 선언과 할당 분리 권장
extract_issue_number나printf실패 시 에러 코드가local선언의 성공(0)으로 마스킹될 수 있습니다. 중요한 경로이므로 분리를 고려해 주세요.♻️ 제안된 수정
# Extract issue number from branch name - local issue_num=$(extract_issue_number "$branch_name") + local issue_num + issue_num=$(extract_issue_number "$branch_name") if [[ -z "$issue_num" ]]; then # If no issue number found, fall back to exact match under specs/{type}/ echo "$specs_dir/$branch_name" return fi # Zero-pad to 3 digits for matching - local padded=$(printf "%03d" "$((10#$issue_num))") + local padded + padded=$(printf "%03d" "$((10#$issue_num))")Also applies to: 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.specify/scripts/bash/common.sh at line 138, Split the declaration and assignment to avoid masking failures: replace lines like local issue_num=$(extract_issue_number "$branch_name") with a two-step form (declare local issue_num; issue_num="$(extract_issue_number "$branch_name")" or local issue_num; issue_num="$(printf ... )" where printf is used) so that a non-zero exit from extract_issue_number or printf is not hidden by the local declaration; apply the same change for the other occurrence referenced (line 147) and ensure you quote command substitutions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.specify/scripts/bash/common.sh:
- Line 43: The assignment local dirname=$(basename "$dir") masks command
failures per SC2155; change it to split declaration and assignment so errors
aren't hidden by using separate steps: first declare the local variable (local
dirname) then assign it (dirname=$(basename "$dir")), and apply the same pattern
used for type_name to keep behavior consistent and ensure any failure in
basename is not masked.
- Line 138: Split the declaration and assignment to avoid masking failures:
replace lines like local issue_num=$(extract_issue_number "$branch_name") with a
two-step form (declare local issue_num; issue_num="$(extract_issue_number
"$branch_name")" or local issue_num; issue_num="$(printf ... )" where printf is
used) so that a non-zero exit from extract_issue_number or printf is not hidden
by the local declaration; apply the same change for the other occurrence
referenced (line 147) and ensure you quote command substitutions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bdfd077-dbf0-4b87-9126-cfdea0a6b10c
📒 Files selected for processing (3)
.specify/scripts/bash/common.shspecs/fix/441-mute-timer-layout-fix/test-contracts/NormalTimer.mdsrc/page/TimerPage/TimerPage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/page/TimerPage/TimerPage.test.tsx
- specs/fix/441-mute-timer-layout-fix/test-contracts/NormalTimer.md
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
오잉 왜 치코 이 pr 프리뷰가 안열리는걸까요??? 그리고 이렇게 정의한 문서들이 많이 남는데 남겨두는 것이 좋을지 고민되네용 .. 다들 어떻게 생각하시는지도 궁금합니다!
PR 프리뷰가 제거 없을 떄, 만들어진것 같은데 어떻게 이용하는 것이가요? 제 화경에서는 local에서는 열리긴해요. |
There was a problem hiding this comment.
변경 사항 확인 완료했습니다! 먼저, 저번 회의 때 말씀드린 대로 리뷰에 오랜 시간이 걸렸는데 이 점 양해의 말씀 다시 드리고 이해해주신 치코께 감사의 인사도 남깁니다. 크게 2가지 파트로 나누어 리뷰하겠습니다: 1) 로직 자체와 2) 썬데이가 언급해 준 스펙 문서의 PR 포함 여부에 대한 의견입니다.
로직 자체
문제 없어 보입니다. 스펙 상 굉장히 간단한 수정 사항이라 조건 분기해서 아이콘 다르게 쓰도록 구현하는 걸 리뷰하는 게 큰 일은 아니었네요!
임시 스펙 문서의 PR 포함 여부에 관한 의견
여기가 사실상 본문입니다. 저는 썬데이의 의견처럼 "특정 스펙 작업 과정에서 생성된 임시 문서는 PR에 포함하지 않아야 한다"는 의견입니다. 핵심만 먼저 말씀드리면, 저는 임시 문서를 PR에 포함했을 때 얻는 것보다 잃는 것이 많다고 생각하기 때문에 이 방안에 반대합니다. 일단 먼저, 임시 문서를 포함해야 한다고 말씀해주신 치코의 의견을 살펴봅시다.
✅ PR에 문서 포함 시 장점
요약
- GitHub 상의 코드 리뷰 에이전트가 문서를 참고하여 리뷰할 수 있다.
GitHub 상의 코드 리뷰 에이전트가 문서 참고 가능
이건 분명한 장점입니다. 만약 임시 문서를 올리지 않는다면, GitHub 상의 에이전트는 순수하게 코드 변경 사항만 보고 PR 요약 및 리뷰를 생성해야 하니까요. 아무래도 여기에 문맥이 조금 더해지면 GitHub 상의 에이전트가 판단하는 데 분명 도움이 될 거라고 생각합니다. 컨텍스트 크기를 고려해봐도 문서 양이 많지 않아 - 물론 이건 이 PR의 작업량이 극도로 적기 때문에 볼륨이 더 있는 PR에서는 두고 볼 문제긴 하지만요 - 문맥을 해치지 않을 것 같기도 하고요.
❌ PR의 문서 포함 시 단점
요약
- 임시 문서의 양이 많아 PR 리뷰 시 집중도가 매우 떨어짐
- 임시 문서는 PR 병합 후 그 존재 가치가 없음
- 임시 문서를 포함한 리뷰를 GitHub가 아닌 다른 맥락에서 진행할 수 있음
임시 문서의 양이 많아 PR 리뷰 집중도 감소
가장 중요도가 낮은 문제입니다. AI 시대에 로직의 탄탄함은 에이전트가 담당하므로, 개발자는 보통 코드를 보고 나서 이 구현 방향이 맞는지, 다른 방향은 어떨지, 그 대안은 적절한지 여부를 검토할 수 있어야 합니다. 그러나 지금처럼 임시 문서를 포함하게 되면 PR 리뷰에 읽을 필요가 없는 임시 문서가 포함되어, 바람직하지 않은 리뷰 경험을 초래할 수 있습니다. 게다가 GitHub가 변경 사항을 보여줄 때에는 Markdown 문서를 변경 사항 위주로 표시하기 때문에 가독성이 다소 덜어진다는 점도 고려해야 한다고 봐요.
따라서 인간이 리뷰에만 집중할 수 있게 임시 문서를 포함하지 않는 것이 좋다고 생각합니다.
임시 문서는 PR 병합 후 존재 가치 없음
생각해보시면 우리 저장소에 있는 파일은 대부분 PR이 병합된 이후에도 그 역할을 다합니다.
- 핵심 코드들은 당연히 UI 렌더링에 사용되고,
- GitHub Actions 워크플로는 배포 및 프리뷰 등 다양한 목적을 위해 상시로 동작하며,
- GitHub의 문서 템플릿도 PR 및 이슈 생성 시에 사용됩니다.
그러나 스펙 주도 개발 과정 중 발생한 임시 문서의 마지막 사용처는 (치코의 말대로 임시 문서를 PR에 포함한다면) GitHub 상의 코드 에이전트가 PR 리뷰 및 요약을 작성할 때입니다. 그 이후에는 사실상 아무런 사용처가 없습니다. 즉, PR 병합 이후에는 좀 세게 말하면 무가치한 파일들에 불과해진다는 겁니다. 물론 우리가 이 저장소에는 영구적으로 사용할 파일들만 저장하자고 명시적으로 합의한 바는 없습니다. 하지만 지금까지 암묵적으로 그래 왔고, 그러한 관행을 준수한 채로 작업 환경이 구성되어 왔다면, 여기서 불필요한 파일을 굳이 포용하는 것은 기존의 작업 흐름과는 명백히 반대되며 우리의 개발 경험에 악영향을 끼칠 가능성이 있다고 봅니다.
따라서, PR 병합 후 아무런 가치 없는 파일을 굳이 PR에 추가할 필요가 없다고 생각합니다.
임시 문서를 포함한 리뷰는 다른 맥락에서도 가능
먼저 한 가지 짚고 넘어가봅시다: 현재 우리 저장소에는 구현 후 동작하는 마지막 스킬인 'checklist.md' - 정확히는 Claude 기준 '.claude/commands/speckits/checklist.md' - 가 존재합니다. 아래 치코가 작성해준 AGENTS.md에 따르면요:
/speckits/specify— Create feature specification/speckits/clarify— Clarify ambiguities in spec/speckits/plan— Generate TDD-driven implementation plan/speckits/tasks— Break plan into ordered tasks/speckits/analyze— Cross-artifact consistency check/speckits/implement— Execute tasks/speckits/checklist— Generate quality checklist
그러나 checklist.md 내부의 내용을 살펴보면 오직 스펙 문서에 대한 평가를 진행하고 있으며 1) 구현된 코드에 대한 리뷰 및 2) 에이전트의 작업 중 행동(규칙 준수 여부, 위반 여부 등)에 대한 평가가 모두 존재하지 않습니다. 물론 구현 후에 스펙 문서를 다시 돌아보며 초반 설계에 문제가 있진 않았는지 되돌아볼 수 있으나, AI가 작성한 코드에 대한 평가와 AI 에이전트 자체에 대한 평가가 부족하다는 점에서 어쩌면 개선의 여지가 있는 워크플로라고 볼 수 있을 것 같습니다.
또한 '.claude/commands/speckits/implement.md'에서 구현한 코드에 대한 리뷰를 진행하기는 하나, 이 리뷰는 문서에서 작성된 것을 잘 구현하였는지 작업 완료 여부를 검사하는 것이지, 작성한 코드의 방향성 자체가 맞고 틀리며 개선 방향이 있는지 등 코드 품질을 리뷰하는 것은 아닌 듯합니다.
따라서 저는 맨 마지막 8번에 코드 자체에 대한 리뷰를 문서를 기반으로 진행하는 새로운 스킬(또는 커맨드)을 추가할 것을 제안합니다. 왜냐하면 이렇게 하면 치코가 찬성 근거로 내세웠던 '문서를 포함한 리뷰 진행하기'가 가능해지거든요. 여기에 추가로 부가 효과가 있다면, 우리 GitHub 저장소에는 이미 AI 리뷰 에이전트가 붙어 있기 때문에 아래의 2중 리뷰가 가능합니다:
- 로컬 개발자 개인 환경에서, 커밋되지 않은 지역 변경 사항과 스펙 문서를 대상으로, 코드 리뷰 진행
- GitHub 저장소 원격 브랜치 및 Actions 러너에서, PR로 올라온 변경 사항을 대상으로, 코드 리뷰 진행
두 리뷰 절차는 맥락(로컬, GitHub 상)도 다르고 내용(문서 포함 여부)도 다르며 심지어는 리뷰 주체도 다릅니다. 물론 알기로는 CodeRabbit CLI를 로컬 환경에 도입할 수 있다고는 하지만, 그럼에도 불구하고 리뷰를 2중으로 진행하는 것은 AI에게 코드 작성을 맡겼을 때의 부작용을 방지하기에 효과적일 수 있다고 봅니다.
향후 방향
이 PR에서는
위와 같은 이유로 장점보다 단점이 많다고 보는 저는, 일단 이 PR에서는 임시 문서를 제거하는 것이 좋다고 생각합니다.
앞으로 우리는
더 나아가, 일단 저도 아무 생각 없이 치코의 AI 도입 PR을 그냥 리뷰하고 병합한 것이 적절하지 않았다는 생각이 이제야 듭니다. 치코가 PR을 올려주신 덕에 공부를 좀 하다 보니, AI 도입은 생산성을 향상시킬 수 있는 여지를 제공하지만, 신중한 접근이 없다면 오히려 기존 작업 플로우를 깨뜨리고 다른 부분에서 효율을 낮출 가능성이 높다는 생각이 들더라구요. 따라서 다음 페이즈 또는 시간이 있을 때 프론트 내에서 AI 도입에 대한 협의 및 커맨드와 스킬에 대한 재정의 등 회의를 한 번 가지면 좋을 것 같습니다. 더 행복하고 할 일 줄어드는 AI 경험을 위해 꼭 필요할 거라고 생각해요.
P.S.
혹시라도 제가 잘못 파악한 내용이 있다면 언제든 편하게 지적해주셔도 좋아요. 치코 덕분에 얻어가는 게 많은 최근입니다. 다시 한 번 고맙다는 말을 남겨요 😆
좋은 의견 감사합니다. 숀이 걱정하신 부분 공감합니다. 아직 워크플로우가 완전하지 않은 상태에서 성급하게 머지된 감도 있는것 같아요! 이 부분은 꼭 팀적으로 회의되고 같이 발전시켰으면 좋겠습니다 |
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
치코 ~! 몇 가지 코멘트를 달았는데 편하게 확인해 주세요 !! 감사합니다 !!!!
| )} | ||
| {item.speaker && ( | ||
| <p className="w-full min-w-0 text-center text-[20px] xl:text-[28px]"> | ||
| {t('{{speaker}} 토론자', { speaker: t(item.speaker) })} |
There was a problem hiding this comment.
해당 부분이 번역되지 않는 문제가 있습니다!
원인은 번역키가 ' ㅣ {{speaker}} 토론자'로 되어 있는데 구조를 바꾸면서 |를 없애고 '{{speaker}} 토론자'로 수정됐는데 키에는 반영하지 않았기 때문입니다.
키가 알맞게 대응되도록 수정해 주시면 번역이 될 것 같아요~!
| expect(h1).toHaveClass('text-center'); | ||
| }); | ||
|
|
||
| it('영어 순서명 렌더링 시 h1 요소에 text-center 클래스가 존재한다', () => { |
There was a problem hiding this comment.
전체적으로 test의 내용이 사용자의 행동 시나리오라기 보다는 어떠한 클래스가 존재한다. DOM이 렌더링 된다. 이런 식으로 작성되어 있는 것 같아요. 하지만 테스트인만큼 사용자 시나리오대로 테스트 항목이 작성된다면 그 자체가 명세가 되어 더 좋을 것 같아요!!
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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/page/TimerPage/components/NormalTimer.test.tsx`:
- Around line 91-101: The test assertions in NormalTimer.test.tsx use long
matcher calls on a single line; update the expect statements for both tests (the
one calling renderNormalTimer(..., '입론') and the one with 'Opening Statement')
to break the matcher chain across multiple lines so they conform to the
formatter: call expect(...) on one line, call .getByRole/arguments on the next
line if needed, and place .toHaveTextContent(...) on its own line; adjust the
two occurrences that use screen.getByRole and toHaveTextContent accordingly
while keeping the test names and renderNormalTimer usage unchanged.
- Around line 84-87: The test "토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다" is passing a
short name ('발언자 1') so it doesn't exercise long-name layout; update the test to
pass a truly long string (e.g., a > max characters username) into
renderNormalTimer instead of '발언자 1' and assert the full long name is rendered
with screen.getByText; locate the test case using the test description and the
render helper renderNormalTimer to change the input value and keep the existing
expectation but matching the full long name.
🪄 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: e40ae038-8cff-4c4a-abaa-3298c12c4c05
📒 Files selected for processing (3)
public/locales/en/translation.jsonpublic/locales/ko/translation.jsonsrc/page/TimerPage/components/NormalTimer.test.tsx
✅ Files skipped from review due to trivial changes (1)
- public/locales/ko/translation.json
| it('토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다', () => { | ||
| renderNormalTimer(null, '발언자 1'); | ||
|
|
||
| expect(screen.getByText('발언자 1 토론자')).toBeInTheDocument(); |
There was a problem hiding this comment.
긴 토론자 이름 케이스가 실제로는 검증되지 않습니다.
테스트명은 긴 이름을 검증한다고 되어 있지만 입력값이 발언자 1이라서 max-character 증가/긴 문자열 레이아웃 회귀를 잡지 못합니다.
🧪 제안 수정
it('토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다', () => {
- renderNormalTimer(null, '발언자 1');
+ const longSpeakerName = '아주 긴 이름의 토론자 1';
- expect(screen.getByText('발언자 1 토론자')).toBeInTheDocument();
+ renderNormalTimer(null, longSpeakerName);
+
+ expect(screen.getByText(`${longSpeakerName} 토론자`)).toBeInTheDocument();
});📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다', () => { | |
| renderNormalTimer(null, '발언자 1'); | |
| expect(screen.getByText('발언자 1 토론자')).toBeInTheDocument(); | |
| it('토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다', () => { | |
| const longSpeakerName = '아주 긴 이름의 토론자 1'; | |
| renderNormalTimer(null, longSpeakerName); | |
| expect(screen.getByText(`${longSpeakerName} 토론자`)).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/page/TimerPage/components/NormalTimer.test.tsx` around lines 84 - 87, The
test "토론자 이름이 긴 경우에도 전체 이름이 화면에 표시된다" is passing a short name ('발언자 1') so it
doesn't exercise long-name layout; update the test to pass a truly long string
(e.g., a > max characters username) into renderNormalTimer instead of '발언자 1'
and assert the full long name is rendered with screen.getByText; locate the test
case using the test description and the render helper renderNormalTimer to
change the input value and keep the existing expectation but matching the full
long name.
| describe('NormalTimer - 순서명 정렬 (US3)', () => { | ||
| it('한글 순서명이 타이머 화면에 표시된다', () => { | ||
| renderNormalTimer(null, null, '입론'); | ||
|
|
||
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('입론'); | ||
| }); | ||
|
|
||
| it('영어 순서명이 타이머 화면에 표시된다', () => { | ||
| renderNormalTimer(null, null, 'Opening Statement'); | ||
|
|
||
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('Opening Statement'); |
There was a problem hiding this comment.
포맷팅 경고를 반영해주세요.
정적 분석 경고대로 긴 matcher 호출을 여러 줄로 나누면 현재 포맷 규칙과 맞습니다.
🧹 제안 수정
- expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('Opening Statement');
+ expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent(
+ 'Opening Statement',
+ );📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('NormalTimer - 순서명 정렬 (US3)', () => { | |
| it('한글 순서명이 타이머 화면에 표시된다', () => { | |
| renderNormalTimer(null, null, '입론'); | |
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('입론'); | |
| }); | |
| it('영어 순서명이 타이머 화면에 표시된다', () => { | |
| renderNormalTimer(null, null, 'Opening Statement'); | |
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('Opening Statement'); | |
| describe('NormalTimer - 순서명 정렬 (US3)', () => { | |
| it('한글 순서명이 타이머 화면에 표시된다', () => { | |
| renderNormalTimer(null, null, '입론'); | |
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent('입론'); | |
| }); | |
| it('영어 순서명이 타이머 화면에 표시된다', () => { | |
| renderNormalTimer(null, null, 'Opening Statement'); | |
| expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent( | |
| 'Opening Statement', | |
| ); |
🧰 Tools
🪛 GitHub Check: test
[warning] 101-101:
Replace 'Opening·Statement' with ⏎······'Opening·Statement',⏎····
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/page/TimerPage/components/NormalTimer.test.tsx` around lines 91 - 101,
The test assertions in NormalTimer.test.tsx use long matcher calls on a single
line; update the expect statements for both tests (the one calling
renderNormalTimer(..., '입론') and the one with 'Opening Statement') to break the
matcher chain across multiple lines so they conform to the formatter: call
expect(...) on one line, call .getByRole/arguments on the next line if needed,
and place .toHaveTextContent(...) on its own line; adjust the two occurrences
that use screen.getByRole and toHaveTextContent accordingly while keeping the
test names and renderNormalTimer usage unchanged.
3 participants
🚩 연관 이슈
closed #441
📝 작업 내용
이 PR은 타이머 화면의 음소거 아이콘 미반영 버그 및 NormalTimer 레이아웃 문제를 수정합니다.
🔇 US1: 음소거 아이콘 미반영 수정
음소거 상태가 변경되어도 헤더의 볼륨 아이콘이 업데이트되지 않던 버그를 수정합니다.
TimerPage헤더 볼륨 아이콘이 음소거 상태(isMuted)를 반영하도록 변경VolumeX, OFF 시Volume2아이콘으로 전환📐 US2·US3: NormalTimer 발언자/팀 정보 레이아웃 개선
최대 글자수 확대 이후 발언자·팀 정보 한 줄 표시 시 발생하던 정렬 불일치와 잘림 문제를 개선합니다.
🧪 테스트 추가
TimerPage음소거 아이콘 동작 테스트 (US1)NormalTimer두 줄 레이아웃 및 순서명 정렬 테스트 (US2·US3)📄 문서
specs/fix/441-mute-timer-layout-fix/스펙 및 설계 문서 추가🏞️ 스크린샷 (선택)
🗣️ 리뷰 요구사항 (선택)
Summary by CodeRabbit
릴리스 노트
새 기능
개선 사항