feat: 차트 옵션 unit 추가#76
Merged
Merged
Conversation
Closed
1 task
📝 WalkthroughWalkthrough이 PR은 차트 시스템에 데이터 단위(unit) 필드를 추가합니다. API 타입과 스토어를 확장하고, ChartOptionPanel에서 단위를 입력할 수 있도록 UI를 구성하며, 소켓 핸들러를 통해 실시간 동기화와 사용자 피드백(토스트)을 제공합니다. Changes차트 단위 기능 추가
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sockets/chartSocketHandler.ts (1)
50-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCHART_UPDATED에서 리스트 상태를 부분 갱신만 하고 있습니다.
현재
name만 갱신되어unit/xaxis/yaxis가 오래된 값으로 남습니다. 업데이트 payload의 필드를 함께 반영해 주세요.As per coding guidelines, "Predictability: Names match behavior, no hidden side effects."제안 diff
chartListState.charts = chartListState.charts.map(chart => - chart.id === updatedChart.id ? { ...chart, name: updatedChart.name } : chart + chart.id === updatedChart.id + ? { + ...chart, + name: updatedChart.name, + xaxis: updatedChart.xaxis, + yaxis: updatedChart.yaxis, + unit: updatedChart.unit, + } + : chart );🤖 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/sockets/chartSocketHandler.ts` around lines 50 - 52, The CHART_UPDATED handler only copies the name into the existing chart, leaving unit/xaxis/yaxis stale; update the merge logic in the chartListState.charts mapping (the function referencing chart and updatedChart) to merge all fields from updatedChart into the existing chart (or replace the chart object with updatedChart) so unit, xaxis, yaxis and any other fields are updated atomically; locate the map callback where chart.id === updatedChart.id and change the result from only updating name to spreading in updatedChart (e.g., produce a merged object) to ensure no stale fields remain.
🤖 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 111-118: The onSave handler in ChartModal (the async IIFE that
calls api.patch with `/canvas/charts/${targetData.chartId}/${targetData.id}`)
lacks failure handling; wrap the await api.patch call in a try/catch, only
apply/commit local state changes after a successful response, and in the catch
block revert any optimistic updates (or refetch the chart data) and show an
error to the user (e.g., via existing toast/modal error handler) so the UI
doesn't remain inconsistent on failure.
In `@src/chart/components/ChartOptionPanel.tsx`:
- Around line 16-24: The onClickedChangeChart handler calls
api.patch(`/canvas/charts/${chartId}`...) inside an async IIFE without error
handling, so failed save requests are swallowed; wrap the await api.patch call
in a try/catch inside onClickedChangeChart (or inside the async IIFE) to catch
errors from api.patch, handle the failure by logging or surfacing it (e.g.,
processLogger/console.error and showing a user-facing error/toast or setting an
error state), and keep references to chartId and chartState when composing the
patch payload so the catch block can include contextual info about the failed
save.
In `@src/sockets/chartModalSocketHandler.ts`:
- Line 14: The socket handlers call openToastMessage with toastLayerRef.current
without guarding for null, which can throw if the ref is empty; update the code
in chartModalSocketHandler.ts to check that toastLayerRef.current is truthy
before calling openToastMessage (for both occurrences where openToastMessage({
dom: toastLayerRef.current, ... }) is used), e.g., wrap the call in an if
(toastLayerRef.current) { ... } or early-return to skip toasting when the DOM
ref is missing so the socket handlers remain safe.
---
Outside diff comments:
In `@src/sockets/chartSocketHandler.ts`:
- Around line 50-52: The CHART_UPDATED handler only copies the name into the
existing chart, leaving unit/xaxis/yaxis stale; update the merge logic in the
chartListState.charts mapping (the function referencing chart and updatedChart)
to merge all fields from updatedChart into the existing chart (or replace the
chart object with updatedChart) so unit, xaxis, yaxis and any other fields are
updated atomically; locate the map callback where chart.id === updatedChart.id
and change the result from only updating name to spreading in updatedChart
(e.g., produce a merged object) to ensure no stale fields remain.
🪄 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: 2a711186-13fa-400a-82a9-7e6e464a0605
📒 Files selected for processing (8)
src/chart/components/ChartModal.tsxsrc/chart/components/ChartOptionPanel.tsxsrc/chart/components/FieldRow.tsxsrc/chart/store/chartOptionStore.tssrc/chart/utils/openChartModal.tsxsrc/sockets/chartModalSocketHandler.tssrc/sockets/chartSocketHandler.tssrc/types/api/chartAPI.ts
Comment on lines
111
to
118
| onSave={() => { | ||
| void (async () => { | ||
| await api | ||
| .patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, { | ||
| name: targetData.name, | ||
| value: targetData.value, | ||
| // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity, | ||
| }) | ||
| .then(res => { | ||
| openToastMessage({ dom: toastLayerRef.current, type: 'success', message: res.message }); | ||
| }); | ||
| await api.patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, { | ||
| name: targetData.name, | ||
| value: targetData.value, | ||
| // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity, | ||
| }); | ||
| })(); |
There was a problem hiding this comment.
바 수정 저장 실패 분기가 없습니다.
onSave의 PATCH 실패를 처리하지 않아 실패 시 상태 불일치가 남을 수 있습니다. 실패 분기를 추가해 주세요.
제안 diff
onSave={() => {
void (async () => {
- await api.patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, {
- name: targetData.name,
- value: targetData.value,
- // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity,
- });
+ try {
+ await api.patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, {
+ name: targetData.name,
+ value: targetData.value,
+ // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity,
+ });
+ } catch {
+ // TODO: 저장 실패 피드백 처리
+ }
})();
}}📝 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.
Suggested change
| onSave={() => { | |
| void (async () => { | |
| await api | |
| .patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, { | |
| name: targetData.name, | |
| value: targetData.value, | |
| // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity, | |
| }) | |
| .then(res => { | |
| openToastMessage({ dom: toastLayerRef.current, type: 'success', message: res.message }); | |
| }); | |
| await api.patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, { | |
| name: targetData.name, | |
| value: targetData.value, | |
| // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity, | |
| }); | |
| })(); | |
| onSave={() => { | |
| void (async () => { | |
| try { | |
| await api.patch(`/canvas/charts/${targetData.chartId}/${targetData.id}`, { | |
| name: targetData.name, | |
| value: targetData.value, | |
| // opacity: targetData.opacity > 1 ? targetData.opacity / 100 : targetData.opacity, | |
| }); | |
| } catch { | |
| // TODO: 저장 실패 피드백 처리 | |
| } | |
| })(); |
🤖 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 111 - 118, The onSave
handler in ChartModal (the async IIFE that calls api.patch with
`/canvas/charts/${targetData.chartId}/${targetData.id}`) lacks failure handling;
wrap the await api.patch call in a try/catch, only apply/commit local state
changes after a successful response, and in the catch block revert any
optimistic updates (or refetch the chart data) and show an error to the user
(e.g., via existing toast/modal error handler) so the UI doesn't remain
inconsistent on failure.
Comment on lines
16
to
24
| const onClickedChangeChart = () => { | ||
| void (async () => { | ||
| await api | ||
| .patch(`/canvas/charts/${chartId}`, { | ||
| xAxis: chartState.xAxisName, | ||
| yAxis: chartState.yAxisName, | ||
| }) | ||
| .then(res => { | ||
| openToastMessage({ dom: toastLayerRef.current, type: 'success', message: res.message }); | ||
| }); | ||
| await api.patch(`/canvas/charts/${chartId}`, { | ||
| xAxis: chartState.xAxisName, | ||
| yAxis: chartState.yAxisName, | ||
| name: chartState.name, | ||
| unit: chartState.unit, | ||
| }); | ||
| })(); |
There was a problem hiding this comment.
저장 요청 실패 처리가 빠져 있습니다.
api.patch 실패 시 예외가 처리되지 않아 저장 실패가 조용히 누락됩니다. 최소한 try/catch로 실패 분기를 명시해 주세요.
제안 diff
const onClickedChangeChart = () => {
void (async () => {
- await api.patch(`/canvas/charts/${chartId}`, {
- xAxis: chartState.xAxisName,
- yAxis: chartState.yAxisName,
- name: chartState.name,
- unit: chartState.unit,
- });
+ try {
+ await api.patch(`/canvas/charts/${chartId}`, {
+ xAxis: chartState.xAxisName,
+ yAxis: chartState.yAxisName,
+ name: chartState.name,
+ unit: chartState.unit,
+ });
+ } catch {
+ // TODO: 저장 실패 피드백 처리
+ }
})();
};📝 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.
Suggested change
| const onClickedChangeChart = () => { | |
| void (async () => { | |
| await api | |
| .patch(`/canvas/charts/${chartId}`, { | |
| xAxis: chartState.xAxisName, | |
| yAxis: chartState.yAxisName, | |
| }) | |
| .then(res => { | |
| openToastMessage({ dom: toastLayerRef.current, type: 'success', message: res.message }); | |
| }); | |
| await api.patch(`/canvas/charts/${chartId}`, { | |
| xAxis: chartState.xAxisName, | |
| yAxis: chartState.yAxisName, | |
| name: chartState.name, | |
| unit: chartState.unit, | |
| }); | |
| })(); | |
| const onClickedChangeChart = () => { | |
| void (async () => { | |
| try { | |
| await api.patch(`/canvas/charts/${chartId}`, { | |
| xAxis: chartState.xAxisName, | |
| yAxis: chartState.yAxisName, | |
| name: chartState.name, | |
| unit: chartState.unit, | |
| }); | |
| } catch { | |
| // TODO: 저장 실패 피드백 처리 | |
| } | |
| })(); | |
| }; |
🤖 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/ChartOptionPanel.tsx` around lines 16 - 24, The
onClickedChangeChart handler calls api.patch(`/canvas/charts/${chartId}`...)
inside an async IIFE without error handling, so failed save requests are
swallowed; wrap the await api.patch call in a try/catch inside
onClickedChangeChart (or inside the async IIFE) to catch errors from api.patch,
handle the failure by logging or surfacing it (e.g., processLogger/console.error
and showing a user-facing error/toast or setting an error state), and keep
references to chartId and chartState when composing the patch payload so the
catch block can include contextual info about the failed save.
| chartSocket.enterChartSession(chartId, message => { | ||
| switch (message.action) { | ||
| case 'CHART_BAR_UPDATED': { | ||
| openToastMessage({ dom: toastLayerRef.current, type: 'success', message: '차트 데이터 값이 변경되었습니다.' }); |
There was a problem hiding this comment.
토스트 레이어 null 가드가 없어 런타임 오류 위험이 있습니다.
toastLayerRef.current가 비어있는 시점에 소켓 이벤트가 오면 토스트 호출이 깨질 수 있습니다. 가드 후 호출로 맞춰 주세요.
제안 diff
- openToastMessage({ dom: toastLayerRef.current, type: 'success', message: '차트 데이터 값이 변경되었습니다.' });
+ if (toastLayerRef.current) {
+ openToastMessage({ dom: toastLayerRef.current, type: 'success', message: '차트 데이터 값이 변경되었습니다.' });
+ }
- openToastMessage({ dom: toastLayerRef.current, type: 'success', message: '차트 바가 삭제되었습니다.' });
+ if (toastLayerRef.current) {
+ openToastMessage({ dom: toastLayerRef.current, type: 'success', message: '차트 바가 삭제되었습니다.' });
+ }Also applies to: 33-33
🤖 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/sockets/chartModalSocketHandler.ts` at line 14, The socket handlers call
openToastMessage with toastLayerRef.current without guarding for null, which can
throw if the ref is empty; update the code in chartModalSocketHandler.ts to
check that toastLayerRef.current is truthy before calling openToastMessage (for
both occurrences where openToastMessage({ dom: toastLayerRef.current, ... }) is
used), e.g., wrap the call in an if (toastLayerRef.current) { ... } or
early-return to skip toasting when the DOM ref is missing so the socket handlers
remain safe.
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.
🛠 관련 이슈
🌱 PR 포인트
📸 스크린샷 / 링크
📎 레퍼런스
❗ 이슈사항 / 기타사항 / 에러슈팅
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선사항