Repository navigation
[FEAT] 선거방송토론위원회 요구사항 반영 - #25
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough자유토론 타이머에 발언시간 초과 허용, 단조 시계 계산, 빠른 복귀 재사용 및 종료 판정을 추가했다. 전체화면 토글, Windows 빌드 워크플로우, 아이콘 자산과 기본 토론 설정도 갱신했다. Changes발언시간 초과 허용 및 타이머 동작
전체화면 토글 기능
Windows 빌드 및 브랜딩
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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/page/TimerPage/TimerPage.tsx (1)
74-84: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
aria-label추가를 권장합니다.이 버튼은 아이콘만 포함하고 있어 스크린 리더 사용자에게 접근 가능한 이름이 필요합니다.
title속성이 접근 가능한 이름 계산의 최하위 우선순위 소스이므로,aria-label을 추가하면 모든 스크린 리더에서 버튼이 안정적으로 안내됩니다.[accessibility]
♿ 접근성 개선 제안
<IconButton icon={ isFullscreen ? ( <RiFullscreenExitFill size={24} /> ) : ( <RiFullscreenFill size={24} /> ) } title="전체 화면" + aria-label="전체 화면" onClick={toggleFullscreen} />🤖 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/page/TimerPage/TimerPage.tsx` around lines 74 - 84, The IconButton in TimerPage lacks an explicit accessible name because it only renders an icon, so add an aria-label to the IconButton alongside the existing title. Update the fullscreen toggle button in TimerPage.tsx (the one using toggleFullscreen and isFullscreen) so screen readers can reliably announce it, using a label that matches the current state or action.src/page/TimerPage/components/TimeBasedTimer.tsx (1)
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value음수 시간 표시 로직이 올바릅니다.
speakingMinute/speakingSecond에Math.abs()가 적용되어 있으므로, 음수 부호 prefix와 조합하여 정확한 음수 시간을 표시합니다.선택 사항: 부호 요소가 조건부 렌더링되므로, 타이머가 양수→음수로 전환될 때 미세한 레이아웃 시프트가 발생할 수 있습니다. 항상 공간을 예약하려면 아래와 같이 수정할 수 있습니다.
♻️ 레이아웃 시프트 개선 제안
<div className="flex flex-row items-center justify-center text-center xl:space-x-3"> - {(speakingTimer ?? 0) < 0 && ( - <p className="w-[30px] lg:w-[45px]">-</p> - )} + <p className="w-[30px] lg:w-[45px]"> + {(speakingTimer ?? 0) < 0 ? '-' : ''} + </p> <p className="w-[110px] lg:w-[150px] xl:w-[170px]"> {speakingMinute} </p>🤖 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/page/TimerPage/components/TimeBasedTimer.tsx` around lines 113 - 115, The negative sign in TimeBasedTimer is conditionally rendered, which can cause a small layout shift when speakingTimer changes sign. In the TimeBasedTimer component, keep the sign placeholder space reserved at all times instead of mounting/unmounting the “-” element, so the timer width stays stable across positive and negative states.
🤖 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 @.github/workflows/build-windows.yml:
- Around line 8-10: The build-windows workflow is still triggered only by the
feature branch, so it will stop running once the PR is merged. Update the
trigger in the workflow definition to target the PR base branch used by the repo
(for example develop or main), or remove the push trigger entirely and keep only
workflow_dispatch. Make the change in the workflow’s top-level triggers where
push.branches is defined.
- Around line 17-18: The Checkout step in the build-windows workflow is
persisting GitHub credentials by default; update the actions/checkout@v4 usage
to set persist-credentials to false. Make this change on the existing “Checkout
code” step so the workflow no longer stores the token in git config while
keeping the checkout behavior unchanged.
---
Nitpick comments:
In `@src/page/TimerPage/components/TimeBasedTimer.tsx`:
- Around line 113-115: The negative sign in TimeBasedTimer is conditionally
rendered, which can cause a small layout shift when speakingTimer changes sign.
In the TimeBasedTimer component, keep the sign placeholder space reserved at all
times instead of mounting/unmounting the “-” element, so the timer width stays
stable across positive and negative states.
In `@src/page/TimerPage/TimerPage.tsx`:
- Around line 74-84: The IconButton in TimerPage lacks an explicit accessible
name because it only renders an icon, so add an aria-label to the IconButton
alongside the existing title. Update the fullscreen toggle button in
TimerPage.tsx (the one using toggleFullscreen and isFullscreen) so screen
readers can reliably announce it, using a label that matches the current state
or action.
🪄 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: dc79fd06-ded5-44b8-846a-89e4743400f9
⛔ Files ignored due to path filters (6)
build/icon.icois excluded by!**/*.icopackage-lock.jsonis excluded by!**/package-lock.jsonpublic/debate_logo.jpgis excluded by!**/*.jpgpublic/dt-logo.icois excluded by!**/*.icopublic/dt-logo.pngis excluded by!**/*.pngpublic/icon.icois excluded by!**/*.ico
📒 Files selected for processing (20)
.github/workflows/build-windows.ymlelectron/constants.tselectron/main.tselectron/type.tsindex.htmlpackage.jsonsrc/hooks/useFullscreen.test.tssrc/hooks/useFullscreen.tssrc/page/TableComposition/components/TimerCreationContent/TimerCreationContent.tsxsrc/page/TimerPage/TimerPage.tsxsrc/page/TimerPage/components/TimeBasedTimer.tsxsrc/page/TimerPage/hooks/useBellSound.test.tssrc/page/TimerPage/hooks/useBellSound.tssrc/page/TimerPage/hooks/useNormalTimer.tssrc/page/TimerPage/hooks/useTimeBasedTimer.test.tsxsrc/page/TimerPage/hooks/useTimeBasedTimer.tssrc/page/TimerPage/hooks/useTimerPageState.test.tssrc/page/TimerPage/hooks/useTimerPageState.tssrc/type/fullscreen.d.tssrc/type/type.ts
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
persist-credentials: false를 추가하세요.
actions/checkout@v4는 기본적으로 GitHub 토큰을 git config에 영속화합니다. CI 환경에서 인증 정보 노출 위험을 방지하기 위해 persist-credentials: false를 설정하는 것이 권장됩니다.
🔒️ 제안하는 수정
- name: Checkout code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 17-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/build-windows.yml around lines 17 - 18, The Checkout step
in the build-windows workflow is persisting GitHub credentials by default;
update the actions/checkout@v4 usage to set persist-credentials to false. Make
this change on the existing “Checkout code” step so the workflow no longer
stores the token in git config while keeping the checkout behavior unchanged.
Source: Linters/SAST tools
i-meant-to-be
left a comment
i-meant-to-be
left a comment
There was a problem hiding this comment.
고생했습니다! 풀스택 개발자 콜리...
시간 관련 코멘트만 좀 확인해주세요! Wall-clock과 Monotonic clock이 혼용되고 있네요.
There was a problem hiding this comment.
권장: 스크립트 제거
이 파일의 타겟 브랜치가 이 기능 개발 브랜치 feat/#24-time-out-option에만 한정된 것을 보면 이 PR 병합 후에는 실질적으로 사용되지 않을 브랜치로 보입니다.
추측상 콜리가 Windows 바이너리 설치를 쉽게 해주기 위해 별도로 빼준 것으로 보이는데, Git 저장소를 복제해서 로컬에서 직접 빌드해도 되기 때문에 필요하지 않을 것 같습니다. 저장소 관리 용이를 위해 지우는 게 나을 것 같아요.
There was a problem hiding this comment.
이 부분에 대한 설명이 부족했던 것 같습니다.
스크립트이 목적 : 맥 유저들에게 installer 설치를 간편화
해당 빌드는 맥을 사용하는 유저들이 window installer build를 가능하게 해주는 트리거입니다. 제 로컬에서는 electron app build를 위한 설정이 window 환경에 비해 매우 복잡했습니다.
따라서 git actions를 통해 이를 자동화하는 것이 1목표였습니다.
2목표는 같은 installer app을 통일하는 것입니다. 아무래도 각자 로컬 build를 하게 되면 본인의 작업내용이 어디까지 반영되었는지에 따라 서로 소통하는 app version이 달라질 수 있기에 특정 스냅샷을 github artifact로 만들고 서로 같은 버전의 installer로 소통하고자 하는 것이 2목표였습니다.
트리거의 경우, 본 브랜치 Push 시 동작하도록 한 것은 바로 해당 워크플로우를 작동시키게 하기 위함이며 본 브랜치 트리거를 제거하고 수동으로 브랜치를 입력해 트리거 할 수 있도록 바꾸었습니다.
| * 이 팀이 발언권을 넘긴(팀 전환으로 비활성화된) 시각을 기록 | ||
| */ | ||
| const markYielded = useCallback(() => { | ||
| lastYieldedAtRef.current = Date.now(); |
There was a problem hiding this comment.
권장: Wall-clock을 Monotonic clock으로 변경
콜리가 PR 설명 Notion에서 시간 정확성을 위해 Monotonic clock으로 변경했다고 말씀하신 바 있습니다. 그러나 현재 이 코드는 여전히 Wall-clock을 쓰고 있네요. 이 부분을 일관성 있게 Monotonic clock으로 통일하는 게 어떤가 제안합니다.
| speakingTimer > 0 && // 1회당 발언시간을 다 쓰지 않고 떠났을 때만 이어가기 | ||
| !isOpponentDone && | ||
| lastYieldedAt !== null && | ||
| now - lastYieldedAt <= thresholdMs |
There was a problem hiding this comment.
권장: 반환값 now가 Monotonic clock인지 확인
이 반환값 now가 Wall-clock에서 오는지, Monotonic clock에서 오는지 확실치가 않네요. 확인 부탁합니다.
There was a problem hiding this comment.
사용처 전수조사 결과, monotic clock임을 확인하였습니다.
추가로, 프로젝트 내에서 Date.now()가 없도록 코드를 개선하였습니다.
| speakingTimer: nextTimer.speakingTimer, | ||
| isOpponentDone, | ||
| lastYieldedAt: nextTimer.getLastYieldedAt(), | ||
| now: Date.now(), |
There was a problem hiding this comment.
권장: Wall-clock을 Monotonic clock으로 변경
콜리가 PR 설명 Notion에서 시간 정확성을 위해 Monotonic clock으로 변경했다고 말씀하신 바 있습니다. 그러나 현재 이 코드는 여전히 Wall-clock을 쓰고 있네요. 이 부분을 일관성 있게 Monotonic clock으로 통일하는 게 어떤가 제안합니다.
|
/noti @i-meant-to-be 주신 피드백 반영하였습니다! 시간 괜찮으실때 천천히 검토부탁드립니다~ |
i-meant-to-be
left a comment
i-meant-to-be
left a comment
There was a problem hiding this comment.
승인! 요거 머지되면 타우리 도입할게용
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
풀스택 콜리 감사합니다 !!!! 이미 숀이 꼼꼼하게 봐 주셔서 코멘트 달 것이 별로 없었네용 ㅎㅎ 몇 가지 코멘트만 확인해 주세요 ~!! 👍🍀
| } | ||
| ], | ||
| "icon": "build/icon.ico", | ||
| "icon": "public/dt-logo.png", |
There was a problem hiding this comment.
저도 이부분 우려되어서 ico로 렌더링해둔 파일 하나 더 넣어두었는데 바꾸질 않았네요.. ico 파일로 바꾸었습니다!
| # `npm run electron:build -- --win`으로는 플래그가 electron-builder에 제대로 전달되지 않는다. | ||
| # (compound 스크립트 + npm `--` 전달 이슈) → 두 단계로 분리해 직접 호출한다. | ||
| - name: Build web assets (vite) | ||
| run: npx vite build |
There was a problem hiding this comment.
vite build는 타입 에러가 있어도 걸러주지를 못 한다고 하네요! 결과물이 타입 체크를 통과할 수 있도록 해야 안전한 결과물이 만들어질 것 같아욥
- name: Typecheck
run: npm run lint:tsc
를 전에 추가하는 방법은 어떨까요?
There was a problem hiding this comment.
[✅ 반영 완료]
오.. 실제로 런타임이 되어서 자바스크립트로 타입스크립트 문법이 변환될 때가 되어서야 타입체크 에러가 발생하네요..
왜 그런지 검색해봤는데 vite t설계철학 자체가 대형프로젝트에서의 속도를 중시하여 tsc 자체 컴파일러를 사용하지 않고, esbuild로 일단 코드 변환을 해서 빠르게 실행하는게 목적이라고 하더라고요.
|
/noti 썬데이 리뷰 모두 반영하여 CI 도는 것 확인 이후 머지합니다. |
🚩 연관 이슈
closed #
📝 작업 내용
이제 리뷰해주세요! 이곳에서 작업내용 확인하실 수 있습니다.
https://bustling-bathtub-b3a.notion.site/3991550c60cf802a85f7e859b7a87c30?source=copy_link
🏞️ 스크린샷 (선택)
🗣️ 리뷰 요구사항 (선택)
Summary by CodeRabbit
요약