[Feat/#57] 수정 필요한 부분 수정하는 PR #59
Conversation
WalkthroughRemoved chat button support and styles; added a Fabric.js SimpleWhiteboard (component + styles + route); introduced a Toast component and styles and wired toasts into My and ReviewNotes; replaced LoginCallback empty render with Loading; adjusted bottom paddings; and added runtime deps ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant Router as App Router
participant WB as SimpleWhiteboardPage
participant Canvas as Fabric Canvas
participant FI as FileInput (hidden)
participant DL as Download action
U->>Router: Navigate to /simple-whiteboard
Router->>WB: mount SimpleWhiteboardPage
WB->>Canvas: init Fabric canvas & PencilBrush
U->>WB: select tool / color / size
WB->>Canvas: update brush/eraser settings
U->>FI: choose image
FI-->>WB: file data
WB->>Canvas: add image (non-selectable)
U->>WB: click "Save"
WB->>Canvas: export to PNG
Canvas-->>WB: data URL
WB->>DL: trigger download
DL-->>U: PNG file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (4)
src/pages/reviewNotes/ReviewNotes.tsx (1)
111-119: Consider using stable keys for toast list.Using array indices as keys can cause React to lose track of which toast is which when toasts are dismissed out of order, potentially leading to incorrect animations or state updates.
Consider generating unique IDs for each toast:
- const [toasts, setToasts] = useState<string[]>([]); + const [toasts, setToasts] = useState<{ id: number; message: string }[]>([]); + let toastIdCounter = 0; const showToast = (msg: string) => { - setToasts((prev) => [...prev, msg]); + setToasts((prev) => [...prev, { id: toastIdCounter++, message: msg }]); };Then update the rendering:
- {toasts.map((msg, i) => ( + {toasts.map((toast) => ( <Toast - key={i} - message={msg} + key={toast.id} + message={toast.message} onClose={() => - setToasts((prev) => prev.filter((_, index) => index !== i)) + setToasts((prev) => prev.filter((t) => t.id !== toast.id)) } /> ))}src/shared/components/toast/toast.css.ts (1)
14-29: Consider adding responsive width constraints.The fixed
minWidth: '32rem'(320px) could cause horizontal overflow on very small mobile devices (e.g., iPhone SE with 320px viewport width accounting for padding).Add responsive width handling:
export const toast = style({ position: 'fixed', bottom: '2rem', left: '50%', transform: 'translateX(-50%)', padding: '1rem 2rem', borderRadius: '10px', - minWidth: '32rem', + minWidth: '28rem', + maxWidth: 'calc(100vw - 4rem)', textAlign: 'center', backgroundColor: themeVars.color.gray800, color: themeVars.color.white000, ...themeVars.font.bodySmall, zIndex: themeVars.zIndex.five, whiteSpace: 'pre-line', });This ensures the toast fits within the viewport while maintaining a reasonable minimum width.
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (2)
173-191: SimplifyhandleAddImagelogic.The function handles both event-based and direct file input access, making it harder to follow. Since the function is only called from
onChange(line 291), the extra fallback logic adds unnecessary complexity.Apply this diff to simplify:
- const handleAddImage = (e?: React.ChangeEvent<HTMLInputElement>) => { - const input = e?.target ?? fileInputRef.current; - const file = input && 'files' in input ? input.files?.[0] : undefined; + const handleAddImage = (e: React.ChangeEvent<HTMLInputElement>) => { + const file = e.target.files?.[0]; if (!file) { return; } const reader = new FileReader(); reader.onload = () => { const result = reader.result; if (typeof result === 'string') { setUploadedImage(result); } }; reader.readAsDataURL(file); - if (input) { - input.value = ''; - } + e.target.value = ''; };
294-305: Consider using a distinct icon for the clear drawing action.Both the eraser tool (line 220) and the clear drawing button (line 304) use
<FaEraser />, which may confuse users about their different functions—one toggles eraser mode, the other clears all drawings.Consider using
FaTrashorFaBroomfromreact-icons/fafor the clear action:import { FaPen, FaEraser, FaRegImage, FaRegWindowClose, FaSave, + FaTrash, } from 'react-icons/fa';onClick={handleClearDrawing} title="풀이 전체 지우기 (드로잉만)" > - <FaEraser size={18} /> + <FaTrash size={18} /> </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
package.json(1 hunks)src/pages/login/login.css.ts(1 hunks)src/pages/loginCallback/LoginCallback.tsx(2 hunks)src/pages/my/My.tsx(5 hunks)src/pages/reviewNotes/ReviewNotes.tsx(4 hunks)src/pages/signup/signup.css.ts(1 hunks)src/pages/simpleWhiteboard/SimpleWhiteboard.tsx(1 hunks)src/pages/simpleWhiteboard/simpleWhiteboard.css.ts(1 hunks)src/routes/globalRoutes.tsx(2 hunks)src/routes/lazy.tsx(1 hunks)src/routes/routePath.ts(1 hunks)src/shared/components/toast/Toast.tsx(1 hunks)src/shared/components/toast/toast.css.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/globalRoutes.tsx (2)
src/routes/routePath.ts (1)
routePath(1-11)src/routes/lazy.tsx (1)
SimpleWhiteboardPage(17-19)
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (1)
src/pages/simpleWhiteboard/simpleWhiteboard.css.ts (1)
uploadedImage(80-86)
src/pages/simpleWhiteboard/simpleWhiteboard.css.ts (1)
src/shared/styles/theme.css.ts (1)
themeVars(21-21)
src/pages/my/My.tsx (2)
src/routes/routePath.ts (1)
routePath(1-11)src/pages/my/my.css.ts (1)
button(187-200)
🪛 GitHub Actions: CI/CD Pipeline
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx
[error] 25-25: TypeScript error: 'canvasOffset' is declared but its value is never read. (TS6133) during build (pnpm build).
🪛 GitHub Check: build
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx
[failure] 25-25:
'canvasOffset' is declared but its value is never read.
🔇 Additional comments (19)
src/pages/signup/signup.css.ts (1)
33-33: LGTM! Improved bottom spacing.The increased bottom padding from
1.6remto2.4remprovides better visual breathing room for the fixed button container, which is a good UX refinement for mobile layouts.src/pages/login/login.css.ts (1)
34-34: LGTM! Consistent spacing adjustment.The padding change matches the signup page adjustment, maintaining design consistency across authentication flows.
src/pages/loginCallback/LoginCallback.tsx (1)
6-6: Good UX improvement!Rendering a
<Loading />component instead of an emptydivprovides better visual feedback to users during the OAuth callback flow.Also applies to: 97-97
package.json (1)
26-26: LGTM!Adding
react-iconsprovides a convenient icon library for the UI enhancements.src/routes/lazy.tsx (1)
17-19: LGTM!The lazy-loaded
SimpleWhiteboardPagefollows the established pattern for route-based code splitting.src/routes/routePath.ts (1)
10-10: LGTM!The new route path follows the existing naming convention and integrates cleanly with the type system.
src/routes/globalRoutes.tsx (1)
10-10: LGTM!The
SimpleWhiteboardPageis correctly registered as a protected route, ensuring authentication is required before access.Also applies to: 50-53
src/pages/my/My.tsx (4)
8-8: LGTM!Toast component import and state initialization are correctly implemented.
Also applies to: 25-25
37-43: LGTM!The viewport-based conditional navigation appropriately restricts the whiteboard feature to larger screens and provides user feedback via toast on smaller devices.
194-197: LGTM!The new "연습장" button maintains consistency with the existing button styling and layout.
218-223: LGTM!Toast rendering is properly gated by state and the
onClosecallback correctly resets the visibility. The multi-line message formatting will render correctly given thewhiteSpace: 'pre-line'styling.src/pages/reviewNotes/ReviewNotes.tsx (2)
7-7: LGTM!The toast queuing system is correctly implemented to support multiple concurrent toast notifications.
Also applies to: 28-28, 30-32
44-44: Good UX improvement!Providing user feedback when no questions are selected for PDF export is more helpful than a silent no-op.
src/shared/components/toast/toast.css.ts (2)
4-12: LGTM!The slide animations maintain horizontal centering during vertical motion, ensuring smooth visual transitions.
31-37: LGTM!Animation classes are correctly configured with appropriate durations and easing functions.
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (1)
147-169: Verify that saving only the canvas (not the uploaded image) aligns with requirements.The save function captures only the canvas drawing area as PNG, excluding the uploaded problem image displayed in the separate
imageDisplayWrapperdiv. The button title indicates this is intentional ("풀이 영역만"), but confirm this matches the expected behavior described in issue #57.src/shared/components/toast/Toast.tsx (1)
1-41: LGTM! Clean toast implementation.The component correctly manages its lifecycle with proper cleanup, coordinates animation timing (300ms exit matches CSS), and follows React best practices. The implementation is straightforward and effective.
src/pages/simpleWhiteboard/simpleWhiteboard.css.ts (2)
65-78: Verify responsive behavior on mobile devices.The
imageDisplayWrapperuses a fixed width of34rem(~544px), which may not work well on smaller mobile screens. Given that issue #57 mentions mobile problems with the solve feature, ensure this layout has been tested on mobile devices.If mobile support is required, consider using a responsive approach:
export const imageDisplayWrapper = style({ position: 'relative', display: 'flex', justifyContent: 'flex-start', alignItems: 'flex-start', - width: '34rem', + width: '100%', + maxWidth: '34rem', height: '100%', padding: '0.8rem', boxSizing: 'border-box', pointerEvents: 'none', borderRight: '2px solid #ddd', backgroundColor: '#f9f9f9', zIndex: themeVars.zIndex.one, + '@media': { + '(max-width: 768px)': { + maxWidth: '100%', + }, + }, });
1-97: LGTM! Well-structured vanilla-extract styles.The styling tokens are correctly organized and properly use theme variables. The layout structure (toolbar, canvas wrapper, image display) is clear and maintainable.
|
🎩 빌드에 성공했습니다! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (8)
64-73: Drawings don’t scale with container on resize → overlay will drift relative to the image.setWidth/setHeight updates the canvas size but existing paths keep original coordinates. When the container resizes, the image scales while strokes don’t, causing misalignment. Scale the viewport based on an initial baseline.
Apply this diff (add a baseline ref and scale viewport in resize):
@@ const problemAreaRef = useRef<HTMLDivElement | null>(null); + const baseSizeRef = useRef<{ w: number; h: number } | null>(null); @@ - const resize = () => { + const resize = () => { if (!canvas || !currentWrapper || !currentProblemArea) { return; } - const rect = currentWrapper.getBoundingClientRect(); - canvas.setWidth(Math.floor(rect.width)); - canvas.setHeight(Math.floor(rect.height)); - drawBackground(canvas); + const rect = currentWrapper.getBoundingClientRect(); + const w = Math.floor(rect.width); + const h = Math.floor(rect.height); + canvas.setWidth(w); + canvas.setHeight(h); + if (!baseSizeRef.current) { + baseSizeRef.current = { w, h }; + } else { + const scaleX = w / baseSizeRef.current.w; + const scaleY = h / baseSizeRef.current.h; + canvas.setViewportTransform([scaleX, 0, 0, scaleY, 0, 0]); + } + drawBackground(canvas);Optionally, keep stroke thickness visually consistent by dividing brush.width by the current scale in the effect that updates brush props. I can provide that patch if you want.
Please verify overlay alignment by drawing, rotating the device, and checking if strokes stay on the same image features across breakpoints.
41-44: Prevent page scroll while drawing on touch devices.Setting touch-action: none on the canvas avoids scroll/zoom interference during strokes.
canvasEl.style.width = '100%'; canvasEl.style.height = '100%'; canvasEl.style.display = 'block'; + canvasEl.style.touchAction = 'none';
144-166: More robust export: use toBlob with a fallback; handle in‑app browsers where download may be ignored.is often blocked/ignored in in‑app browsers (KakaoTalk, Instagram). Use toBlob when available, fall back to dataURL, and open a new tab if download isn’t supported.
const handleSave = () => { const canvas = canvasRef.current; if (!canvas) { return; } canvas.discardActiveObject(); canvas.renderAll(); - const dataUrl = canvas.toDataURL({ - format: 'png', - multiplier: 2, - left: 0, - top: 0, - width: canvas.getWidth(), - height: canvas.getHeight(), - }); - - const link = document.createElement('a'); - link.href = dataUrl; - link.download = 'whiteboard-solution.png'; - link.click(); + const el = (canvas as any).lowerCanvasEl as HTMLCanvasElement; + const downloadOrOpen = (url: string) => { + const a = document.createElement('a'); + if ('download' in a) { + a.href = url; + a.download = 'whiteboard-solution.png'; + document.body.appendChild(a); + a.click(); + a.remove(); + } else { + window.open(url, '_blank'); + } + }; + if (el && el.toBlob) { + el.toBlob((blob) => { + if (!blob) return; + const url = URL.createObjectURL(blob); + downloadOrOpen(url); + setTimeout(() => URL.revokeObjectURL(url), 1000); + }, 'image/png'); + } else { + const dataUrl = canvas.toDataURL({ format: 'png', multiplier: 2 }); + downloadOrOpen(dataUrl); + } };Please test in KakaoTalk/Naver in‑app browsers on Android/iOS.
194-206: A11y and semantics: add aria-pressed/aria-label and type="button" to icon buttons.Improves screen‑reader UX and prevents accidental form submission defaults.
Example for the first two buttons (replicate to others):
-<button +<button + type="button" className={css.iconButton} style={{ /* … */ }} onClick={togglePenMode} - title={isPenActive ? '펜 끄기' : '펜 켜기'} + title={isPenActive ? '펜 끄기' : '펜 켜기'} + aria-label={isPenActive ? '펜 끄기' : '펜 켜기'} + aria-pressed={isPenActive} > <FaPen size={18} /> </button> -<button +<button + type="button" className={css.iconButton} style={{ /* … */ }} onClick={toggleEraserMode} - title={isEraser ? '지우개 끄기' : '지우개 켜기'} + title={isEraser ? '지우개 끄기' : '지우개 켜기'} + aria-label={isEraser ? '지우개 끄기' : '지우개 켜기'} + aria-pressed={isEraser} > <FaEraser size={18} /> </button>For the other icon buttons, add type="button" and aria-label matching the title.
Also applies to: 207-218, 263-269, 270-282, 292-302, 307-318
33-40: Remove unused problemAreaRef dependency in resize guard.The guard checks currentProblemArea but never reads its dimensions anymore. Simplify.
- const currentProblemArea = problemAreaRef.current; + // const currentProblemArea = problemAreaRef.current; // no longer needed @@ - if (!canvas || !currentWrapper || !currentProblemArea) { + if (!canvas || !currentWrapper) { return; }Also applies to: 64-67
284-289: Mobile camera capture hint; confirm single vs multi image requirement.If this component is used in the “solve” flow, input currently supports one image. If multiple images are desired (per linked issue), the state/model must change. Regardless, adding capture="environment" improves mobile UX.
Do we need multi-image here? If yes, I can draft a state + UI change.
<input ref={fileInputRef} type="file" accept="image/*" + capture="environment" style={{ display: 'none' }} onChange={handleAddImage} />
153-160: Large export multiplier can OOM on mobile.multiplier: 2 can create very large bitmaps on tablets. Consider capping by max dimension.
const maxSide = Math.max(canvas.getWidth(), canvas.getHeight()); const scale = Math.min(2, 4096 / Math.max(1, maxSide)); // cap by ~4K const dataUrl = canvas.toDataURL({ format: 'png', multiplier: scale });
2-2: Optional: lazy‑load Fabric to reduce initial bundle and avoid SSR pitfalls.If SSR or code‑splitting matters, import Fabric inside the effect:
useEffect(() => { let disposed = false; (async () => { const { fabric } = await import('fabric'); // init with fabric here... if (disposed) return; // ... })(); return () => { disposed = true; /* cleanup */ }; }, []);This also sidesteps window access at import time in SSR environments.
Is SSR or route-level code-splitting a goal for this page?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (1)
src/pages/simpleWhiteboard/simpleWhiteboard.css.ts (1)
uploadedImage(80-86)
🔇 Additional comments (1)
src/pages/simpleWhiteboard/SimpleWhiteboard.tsx (1)
53-63: Non-interactive objects: confirm future needs.Forcing selectable:false and evented:false on all added objects disables any future select/erase-by-object features. If only freehand strokes + white “eraser” are intended, this is fine. Otherwise consider scoping this rule to specific types.
|
🎩 빌드에 성공했습니다! |
📌 Related Issues
✅ 체크 리스트
📄 Tasks
📷 Screenshot
🔔 ETC
Summary by CodeRabbit
New Features
Refactor
Style
Chores