fix: 차트 모달 수정#77
Conversation
📝 WalkthroughWalkthrough차트 편집 기능을 안정화하기 위해 입력 범위 제약(최소값 1), 팝오버 동작 강화(최소 데이터 개수 검증), 공통 Input 컴포넌트 확장(onKeyDown, min, step), UI 라벨 국제화(영어 → 한국어), 소켓 메시지 타입 확장(CHART_UPDATED)을 구현합니다. Changes차트 입력 검증 및 UI 개선
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (1)
src/components/popover/Popover.tsx (1)
50-64: ⚡ Quick win숫자 입력에
min/step을 같이 설정해 브라우저 기본 제약도 활용해 주세요.현재는 키 입력만 필터링하고 있어요.
min={1},step={1}를 함께 주면 스피너/휠/모바일 키패드 경로까지 제약이 일관되어 유지보수가 쉬워집니다.제안 diff
<Input id={`${id}-input`} type="number" value={value} + min={1} + step={1} onInput={e => { const nextValue = Number((e.currentTarget as HTMLInputElement).value); if (!Number.isFinite(nextValue)) return; onValueInput?.(nextValue); }} onKeyDown={e => { if (['-', '.', 'e', 'E', '+'].includes(e.key)) { e.preventDefault(); } }} />As per coding guidelines "Fundamentals: Predictability: Names match behavior, no hidden side effects." 입력 제약은 브라우저 기본 동작까지 포함해 일관되게 보장하는 편이 예측 가능합니다.
🤖 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/popover/Popover.tsx` around lines 50 - 64, Add explicit numeric constraints to the Input element so browser-native controls/validation match the JavaScript key filtering: update the Input rendered in Popover (the element using id={`${id}-input`}, value, onInput and onKeyDown) to include min={1} and step={1} (or other appropriate min/step values for your domain) so spinners, wheel input and mobile keypads are constrained consistently with the existing onKeyDown filtering.
🤖 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/chart/components/ChartModal.tsx`:
- Around line 92-93: The current check uses `if (!nextValue)` so negative values
slip through; change the update in the `targetData.value` assignment (in
ChartModal.tsx where `targetData.value` and `nextValue` are used) to always
clamp to a minimum of 1 (e.g., compute a numeric value from `nextValue` and set
`targetData.value = Math.max(1, parsedValue)`), ensuring any non-numeric
fallback is handled consistently so the value is never below 1.
In `@src/chart/components/Section.tsx`:
- Line 4: The Section component's title prop is now optional but the JSX always
renders the title span, creating empty space when title is undefined; update the
Section component (the function or class named Section in
src/chart/components/Section.tsx) to render the title element conditionally—only
output the title <span> (or whatever element currently used for the header) when
the title prop is truthy (e.g., wrap the existing title markup in a conditional
expression or early-return logic) so no empty span or gap is emitted when title
is absent.
In `@src/components/modal/Modal.tsx`:
- Line 60: The close button in Modal (the element with the class string
currently containing "outline-none") removes the browser focus indicator;
replace "outline-none" with an accessible focus pattern by using
"outline-hidden" and add explicit focus-visible styles (e.g.,
focus-visible:outline and a visible focus ring like focus-visible:ring-2
focus-visible:ring-indigo-500 or similar) so keyboard users and forced-colors
environments still see focus; update the class on the close button inside the
Modal component accordingly.
---
Nitpick comments:
In `@src/components/popover/Popover.tsx`:
- Around line 50-64: Add explicit numeric constraints to the Input element so
browser-native controls/validation match the JavaScript key filtering: update
the Input rendered in Popover (the element using id={`${id}-input`}, value,
onInput and onKeyDown) to include min={1} and step={1} (or other appropriate
min/step values for your domain) so spinners, wheel input and mobile keypads are
constrained consistently with the existing onKeyDown filtering.
🪄 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 Plus
Run ID: b2eccfdc-d4a6-4e24-926a-3f023384afc2
📒 Files selected for processing (10)
src/chart/components/ChartModal.tsxsrc/chart/components/ChartOptionPanel.tsxsrc/chart/components/Section.tsxsrc/chart/core/createChart.tssrc/components/common/Input.tsxsrc/components/modal/Modal.tsxsrc/components/popover/Popover.tsxsrc/sockets/chartModalSocketHandler.tssrc/sockets/chartSocket.tssrc/sockets/chartSocketHandler.ts
💤 Files with no reviewable changes (1)
- src/sockets/chartSocketHandler.ts
| if (!nextValue) targetData.value = 1; | ||
| else targetData.value = nextValue; |
There was a problem hiding this comment.
최솟값 보정 조건이 음수를 통과시킵니다.
if (!nextValue)는 0만 보정하고 음수는 그대로 저장됩니다. 이러면 “값 최소 1” 제약이 입력 경로에 따라 깨집니다. 상태 갱신 지점에서 >= 1로 클램프해 주세요.
제안 diff
onValueInput={(nextValue: number) => {
- if (!nextValue) targetData.value = 1;
- else targetData.value = nextValue;
+ const normalized = Number.isFinite(nextValue) ? Math.max(1, Math.round(nextValue)) : 1;
+ targetData.value = normalized;
chartControlsRef.current?.redraw();
}}As per coding guidelines "Fundamentals: Predictability: Names match behavior, no hidden side effects." 최소값 제약은 모든 입력 경로에서 동일하게 적용되어야 예측 가능합니다.
📝 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.
| if (!nextValue) targetData.value = 1; | |
| else targetData.value = nextValue; | |
| const normalized = Number.isFinite(nextValue) ? Math.max(1, Math.round(nextValue)) : 1; | |
| targetData.value = normalized; |
🤖 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/chart/components/ChartModal.tsx` around lines 92 - 93, The current check
uses `if (!nextValue)` so negative values slip through; change the update in the
`targetData.value` assignment (in ChartModal.tsx where `targetData.value` and
`nextValue` are used) to always clamp to a minimum of 1 (e.g., compute a numeric
value from `nextValue` and set `targetData.value = Math.max(1, parsedValue)`),
ensuring any non-numeric fallback is handled consistently so the value is never
below 1.
|
|
||
| type SectionProps = { | ||
| title: string; | ||
| title?: string; |
There was a problem hiding this comment.
title optional 변경에 맞춰 제목 영역을 조건부 렌더링해 주세요.
title이 없는 경우에도 제목 <span>이 렌더링되어 빈 행/간격이 생길 수 있습니다. title 존재 시에만 렌더링되도록 맞추는 게 안전합니다.
제안 diff
export const Section = ({ title, children }: SectionProps) => {
return (
<div className="flex flex-col gap-4">
- <span className="text-xs font-semibold tracking-widest text-primary">{title}</span>
+ {title ? <span className="text-xs font-semibold tracking-widest text-primary">{title}</span> : null}
{children}
</div>
);
};As per coding guidelines, "Predictability: Names match behavior, no hidden side effects."
📝 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.
| title?: string; | |
| export const Section = ({ title, children }: SectionProps) => { | |
| return ( | |
| <div className="flex flex-col gap-4"> | |
| {title ? <span className="text-xs font-semibold tracking-widest text-primary">{title}</span> : null} | |
| {children} | |
| </div> | |
| ); | |
| }; |
🤖 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/chart/components/Section.tsx` at line 4, The Section component's title
prop is now optional but the JSX always renders the title span, creating empty
space when title is undefined; update the Section component (the function or
class named Section in src/chart/components/Section.tsx) to render the title
element conditionally—only output the title <span> (or whatever element
currently used for the header) when the title prop is truthy (e.g., wrap the
existing title markup in a conditional expression or early-return logic) so no
empty span or gap is emitted when title is absent.
| type="button" | ||
| aria-label="Close modal" | ||
| class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full hover:bg-zinc-100/60" | ||
| class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full outline-none hover:bg-zinc-100/60" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4.2.x에서 outline-none 과 outline-hidden 의 차이와, 버튼 접근성 권장 포커스 스타일(focus-visible) 패턴을 공식 문서 기준으로 확인해줘.
💡 Result:
Tailwind CSS v4.2.x에서 outline-none과 outline-hidden의 차이점과 버튼 접근성을 위한 권장 패턴은 다음과 같습니다. 1. outline-none vs outline-hidden 차이점 Tailwind CSS v4에서는 기존 v3의 동작을 명확히 구분하기 위해 두 유틸리티의 역할을 재정의했습니다 [1][2]. - outline-none: 단순히 CSS 속성 outline-style: none을 적용합니다 [1][2][3]. 브라우저의 기본 포커스 아웃라인을 완전히 제거하므로, 사용자가 직접 접근성 있는 포커스 스타일을 구현하지 않을 경우 키보드 사용자가 포커스 위치를 알 수 없게 됩니다 [3]. - outline-hidden: v3의 outline-none이 수행하던 역할을 이어받은 유틸리티입니다 [1][2]. 단순히 아웃라인을 숨기는 것뿐만 아니라, 'forced colors mode'(고대비 모드)와 같은 접근성 환경에서도 포커스 표시가 유지되도록 보장합니다 [3]. 따라서 접근성을 고려하여 기본 아웃라인을 제거하고 싶을 때는 outline-hidden을 사용하는 것이 권장됩니다 [3]. 2. 버튼 접근성 권장 포커스 스타일 (focus-visible) 패턴 공식 문서 및 접근성 모범 사례에 따르면, 기본 아웃라인을 제거할 경우 반드시 대체할 수 있는 명확한 포커스 스타일을 제공해야 합니다 [3]. - focus-visible 사용: 마우스 클릭 시에는 포커스 스타일을 숨기고, 키보드 탐색 시에만 스타일을 표시하기 위해 focus-visible 변형(variant)을 사용하는 것이 표준입니다 [4]. - 구현 패턴: outline-hidden으로 기본 아웃라인을 제거한 뒤, focus-visible을 사용하여 사용자 정의 포커스 스타일(예: outline-2, ring 등)을 적용합니다 [3][4]. 예시 코드: 버튼 이 패턴은 키보드 사용자에게는 명확한 시각적 피드백을 제공하면서, 마우스 사용자에게는 불필요한 포커스 링을 노출하지 않아 깔끔한 UI를 유지할 수 있게 합니다 [3][4].
Citations:
- 1: [v4] :focus-visible no longer overrides default outline for some reason tailwindlabs/tailwindcss#15152
- 2: Replace
outline-nonewithoutline-hidden, add newoutline-nonetailwindlabs/tailwindcss#14926 - 3: https://tailwindcss.com/docs/outline-style
- 4: https://tailwindcss.com/docs/hover-focus-and-other-states
🏁 Script executed:
# Modal.tsx 파일 위치 및 라인 60 주변 코드 확인
find . -name "Modal.tsx" -type f | head -5Repository: thinkfree-2026/thinkchart-fe
Length of output: 105
🏁 Script executed:
# tailwindcss 버전 확인
fd -t f "package.json" | head -1 | xargs cat | grep -A2 -B2 "tailwindcss"Repository: thinkfree-2026/thinkchart-fe
Length of output: 403
🏁 Script executed:
cat -n ./src/components/modal/Modal.tsx | sed -n '50,70p'Repository: thinkfree-2026/thinkchart-fe
Length of output: 901
포커스 표시를 제거하지 말고 접근성을 고려한 포커스 스타일을 적용하세요.
outline-none은 브라우저의 기본 포커스 아웃라인을 완전히 제거하여 키보드 사용자의 포커스 위치 인지를 방해합니다. Tailwind CSS v4.2.3에서는 outline-hidden을 사용하는 것이 권장되며(forced colors mode 등 접근성 환경에서도 포커스 유지), 함께 명시적인 focus-visible 스타일을 제공해야 합니다.
제안 수정안
- class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full outline-none hover:bg-zinc-100/60"
+ class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full outline-hidden hover:bg-zinc-100/60 focus-visible:ring-2 focus-visible:ring-zinc-500 focus-visible:ring-offset-2"📝 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.
| class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full outline-none hover:bg-zinc-100/60" | |
| class="absolute top-6 right-6 h-10 w-10 cursor-pointer rounded-full outline-hidden hover:bg-zinc-100/60 focus-visible:ring-2 focus-visible:ring-zinc-500 focus-visible:ring-offset-2" |
🤖 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/modal/Modal.tsx` at line 60, The close button in Modal (the
element with the class string currently containing "outline-none") removes the
browser focus indicator; replace "outline-none" with an accessible focus pattern
by using "outline-hidden" and add explicit focus-visible styles (e.g.,
focus-visible:outline and a visible focus ring like focus-visible:ring-2
focus-visible:ring-indigo-500 or similar) so keyboard users and forced-colors
environments still see focus; update the class on the close button inside the
Modal component accordingly.
1 participant
🛠 관련 이슈
🌱 PR 포인트
📸 스크린샷 / 링크
📎 레퍼런스
❗ 이슈사항 / 기타사항 / 에러슈팅
Summary by CodeRabbit
릴리스 노트
New Features
Bug Fixes
Style