Conversation
- sessionId를 사용하여 세션별로 기기 목록 및 별명을 localStorage에 분리 저장합니다. - 기기 상세 페이지의 더미 데이터를 제거하고 실제 히스토리 API 연동을 통해 차트를 구현합니다. - 앱 진입 시 마지막 세션 경로를 유지하기 위한 리다이렉트 로직을 추가합니다.
- useState의 지연 초기화(lazy initialization)를 사용하여 로컬 스토리지에서 기기 목록을 직접 불러오도록 수정합니다. - 불필요한 useEffect 호출을 제거하여 초기 렌더링 시의 데이터 정합성을 확보하고 불필요한 재렌더링을 방지합니다.
📝 WalkthroughWalkthroughThis PR implements session-based routing and device history tracking. It adds localStorage-backed session persistence to the sidebar navigation, updates the routing system to support dynamic session IDs, and integrates API-driven device history fetching with dynamic chart visualization on the detail page. Device data is now stored and managed per-session rather than globally. Changes
Sequence DiagramsequenceDiagram
actor User
participant App
participant Storage as localStorage
participant Sidebar
participant DetailPage
participant API as Backend API
User->>Sidebar: Click session
Sidebar->>Storage: Save lastSessionPath
Sidebar->>App: Route to /:sessionId
App->>Storage: Retrieve lastSessionPath
App->>DetailPage: Render with sessionId
DetailPage->>DetailPage: Extract serialNumber from route
DetailPage->>API: Fetch device history
API-->>DetailPage: Return DeviceHistoryPoint[]
DetailPage->>DetailPage: mapHistoryToChartData()
DetailPage->>User: Display dynamic chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/sidebar/sidebar.tsx (1)
74-82:⚠️ Potential issue | 🟡 MinorPotential ID collision when items are deleted in the future.
The ID generation logic uses
prev.length + 1, which could produce duplicate IDs if section deletion is added later (e.g., delete "section-2", then add new → creates another "section-3"). Consider using a UUID or tracking a monotonically increasing counter in state/localStorage.🛡️ Suggested approach using a counter
+const getNextSectionId = (items: SidebarNavItem[]): number => { + const ids = items.map((i) => parseInt(i.id.replace("section-", ""), 10) || 0); + return Math.max(0, ...ids) + 1; +}; + const handleAddItem = useCallback(() => { setNavItems((prev) => { - const nextIndex = prev.length + 1; + const nextIndex = getNextSectionId(prev); const newId = `section-${nextIndex}`; const newLabel = `Section ${nextIndex}`; const newPath = `/${newId}`; return [...prev, { id: newId, label: newLabel, path: newPath }]; }); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/sidebar.tsx` around lines 74 - 82, handleAddItem currently builds IDs from prev.length+1 which can collide after deletions; change the ID generation to use a monotonic counter or a UUID instead of list length. Add a persistent counter (e.g., useRef or state like nextSectionIndex) that you increment on each add and use to create newId/newLabel/newPath in handleAddItem (or call crypto.randomUUID() if you prefer UUIDs) so IDs remain unique even when items are removed. Update any code that assumes numeric sequential IDs if necessary (e.g., label/path creation) to derive values from the new counter/UUID.
🧹 Nitpick comments (2)
src/hooks/useDeviceData.ts (2)
31-37: Consider adding cleanup to avoid stale subscriptions on session change.If
sessionIdchanges and devices are reinitialized (per the fix above), old device subscriptions won't be cleaned up automatically. Adding a cleanup function would ensure proper resource management:Proposed improvement
useEffect(() => { if (!isConnected) return; + const subscribedIds: string[] = []; devices.forEach((device) => { subscribeDevice(device.id); + subscribedIds.push(device.id); console.log(`소켓 구독 ${device.id}`); }); + + return () => { + subscribedIds.forEach((id) => { + unsubscribeDevice(id); + console.log(`소켓 구독 해제 (cleanup) ${id}`); + }); + }; - }, [devices, subscribeDevice, isConnected]); + }, [devices, subscribeDevice, unsubscribeDevice, isConnected]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDeviceData.ts` around lines 31 - 37, The effect that subscribes to devices (useEffect using isConnected, devices, subscribeDevice) needs a cleanup to unsubscribe previous subscriptions when session/devices change; implement a cleanup function that iterates the currently subscribed device ids and calls unsubscribeDevice(device.id) (or add an unsubscribeDevice prop if missing) and return that cleanup from the useEffect, and ensure unsubscribeDevice (or a stable wrapper) is included in the dependency array so stale subscriptions are removed on session changes.
78-80: Avoid side effects insidesetStateupdater functions.The
localStorage.setItemcall inside thesetDevicescallback is a side effect. React may invoke updater functions multiple times (e.g., in StrictMode), causing redundant writes. While functionally harmless here due to idempotency, it's cleaner to separate state updates from persistence.Alternative approach using useEffect for persistence
+ useEffect(() => { + const ids = devices.map((device) => device.id); + localStorage.setItem(deviceIdsStorageKey, JSON.stringify(ids)); + }, [devices, deviceIdsStorageKey]); const addDevice = (id: string) => { const savedNames: Record<string, string> = JSON.parse( localStorage.getItem(deviceNamesStorageKey) || "{}", ); setDevices((prev) => { if (prev.some((device) => device.id === id)) { return prev; } const deviceName = savedNames[id] || `기기 ${prev.length + 1}`; const newDevice: Device = { id: id, name: deviceName, temperature: 0, warning: false, hasShownWarning: false, }; - const newDevices = [...prev, newDevice]; - const newIds = newDevices.map((device) => device.id); - localStorage.setItem(deviceIdsStorageKey, JSON.stringify(newIds)); - - return newDevices; + return [...prev, newDevice]; }); };This would also simplify
deleteDevicesimilarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDeviceData.ts` around lines 78 - 80, The state updater passed to setDevices currently performs a side effect (localStorage.setItem) inside the updater; move persistence out of the updater by removing localStorage writes from the setDevices callback and instead add a useEffect that watches the devices state (the same state variable updated by setDevices) and writes device IDs to localStorage there (use the existing deviceIdsStorageKey and mapping logic that builds newIds from devices). Also update deleteDevice to only call setDevices (no localStorage call) so persistence is handled consistently by the new useEffect.
🤖 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/apis/deviceHistory.ts`:
- Around line 6-8: The code uses const baseUrl =
import.meta.env.VITE_API_BASE_URL and builds the fetch URL with serialNumber
without checking baseUrl; add a guard that validates baseUrl is a non-empty
string before using it (e.g., in the same module or entry function that calls
the fetch), and if it's missing throw a descriptive Error like
"VITE_API_BASE_URL is not set" so the call to
fetch(`${baseUrl}/histories/all/${encodeURIComponent(serialNumber)}`) cannot
proceed with "undefined"; update any callers if you centralize the check into a
helper (e.g., getBaseUrl) and ensure serialNumber is still encoded as before.
In `@src/hooks/useDeviceData.ts`:
- Around line 10-29: The devices state is only initialized once by the useState
lazy initializer and doesn't update when effectiveSessionId changes; add a
useEffect that watches effectiveSessionId (or sessionId) and re-reads
localStorage keys deviceIdsStorageKey and deviceNamesStorageKey, constructs the
Device[] (same shape as the initializer) and calls setDevices to reinitialize
devices when the session changes; keep existing useState initializer for mount
but ensure the effect mirrors that logic and runs whenever effectiveSessionId
changes.
In `@src/pages/detail.tsx`:
- Around line 40-42: mapHistoryToChartData returns temperatureData but the
component only destructures labels and riskData, so temperatureData is never
passed to the chart; update the React.useMemo destructuring to include
temperatureData and add a second dataset to the chart data where the existing
dataset uses riskData and the new dataset uses temperatureData (matching the
chart config used in this file), referencing mapHistoryToChartData,
temperatureData, labels, and riskData to locate the code to change.
---
Outside diff comments:
In `@src/components/sidebar/sidebar.tsx`:
- Around line 74-82: handleAddItem currently builds IDs from prev.length+1 which
can collide after deletions; change the ID generation to use a monotonic counter
or a UUID instead of list length. Add a persistent counter (e.g., useRef or
state like nextSectionIndex) that you increment on each add and use to create
newId/newLabel/newPath in handleAddItem (or call crypto.randomUUID() if you
prefer UUIDs) so IDs remain unique even when items are removed. Update any code
that assumes numeric sequential IDs if necessary (e.g., label/path creation) to
derive values from the new counter/UUID.
---
Nitpick comments:
In `@src/hooks/useDeviceData.ts`:
- Around line 31-37: The effect that subscribes to devices (useEffect using
isConnected, devices, subscribeDevice) needs a cleanup to unsubscribe previous
subscriptions when session/devices change; implement a cleanup function that
iterates the currently subscribed device ids and calls
unsubscribeDevice(device.id) (or add an unsubscribeDevice prop if missing) and
return that cleanup from the useEffect, and ensure unsubscribeDevice (or a
stable wrapper) is included in the dependency array so stale subscriptions are
removed on session changes.
- Around line 78-80: The state updater passed to setDevices currently performs a
side effect (localStorage.setItem) inside the updater; move persistence out of
the updater by removing localStorage writes from the setDevices callback and
instead add a useEffect that watches the devices state (the same state variable
updated by setDevices) and writes device IDs to localStorage there (use the
existing deviceIdsStorageKey and mapping logic that builds newIds from devices).
Also update deleteDevice to only call setDevices (no localStorage call) so
persistence is handled consistently by the new useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6e6da16-9b18-4dd3-836b-125a3fbd0b15
📒 Files selected for processing (9)
src/App.tsxsrc/apis/deviceHistory.tssrc/components/sidebar/sidebar.tsxsrc/hooks/useDeviceData.tssrc/hooks/useDeviceHistory.tssrc/pages/detail.tsxsrc/pages/home/home.tsxsrc/types/deviceHistory.tssrc/utils/deviceHistoryUtils.ts
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
2 participants
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes