Skip to content

#23/socket connection#25

Merged
JAEJUN090210 merged 7 commits intomainfrom
#23/socket-connection
Mar 12, 2026
Merged

#23/socket connection#25
JAEJUN090210 merged 7 commits intomainfrom
#23/socket-connection

Conversation

@yunho09
Copy link
Copy Markdown
Member

@yunho09 yunho09 commented Mar 11, 2026

소켓 연동 및 라우터 설정
#23

Summary by CodeRabbit

  • New Features

    • Real-time device updates with live temperature and risk status
    • Persistent device storage across sessions
    • Direct navigation to individual device detail views
  • Improvements

    • Enhanced device card hover interactions and adjusted layout spacing
    • Improved device registration with input trimming, validation, and clearer error alerts
    • Targeted warning modal to avoid repeated/global alerts
  • Chores

    • Removed base URL env entry and updated runtime/network config

@yunho09 yunho09 self-assigned this Mar 11, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 11, 2026

📝 Walkthrough

Walkthrough

Adds STOMP-over-SockJS real-time device streaming and subscriptions, persists devices in localStorage with merged live telemetry, wires id-based navigation to device detail pages, updates device registration API, adds WebSocket deps, and tweaks UI styling and layout.

Changes

Cohort / File(s) Summary
WebSocket hook
src/hooks/useDeviceStomp.ts
New hook managing a SockJS+STOMP client, connection state, per-device subscriptions, incoming JSON parsing, and subscribe/unsubscribe APIs.
Device data hook
src/hooks/useDeviceData.ts
Loads/persists devices to localStorage, integrates with useDeviceStomp, computes liveDevices (temperature, warning, showModal), exposes devices as merged live data, and adds addDevice, deleteDevice, updateDeviceName.
Types
src/types/device.ts
Adds optional hasShownWarning?: boolean to Device interface.
UI & routing
src/components/DeviceCardItem.tsx, src/components/DeviceDetail.tsx, src/pages/home/home.tsx
DeviceCardItem passes id to DeviceDetail and adjusts hover styling; DeviceDetail now requires id and uses useNavigate to route to /detail/{id}; Home selects a targeted warningModalDevice, renders DeviceAddCard, and uses device.warning; layout margin updated.
Modal API
src/components/modal/DeviceRegisterModal.tsx
addDevice signature changed to (serialNumber: string, name: string) => void; trims/validates serial input, sends trimmed serial to mutation, adds onError handling, and invokes addDevice(serialNumber, name) on success.
Config & deps
.env, package.json, vite.config.ts
Removed VITE_API_BASE_URL from .env; added @stomp/stompjs, sockjs-client, and @types/sockjs-client to package.json; vite.config.ts adds define: { global: 'window' }.
Misc
src/components/..., other small edits
Minor comment fix, hover styling relocation, small JSX/formatting tweaks across components.

Sequence Diagram(s)

sequenceDiagram
    participant Home as Home Page
    participant useDeviceData as useDeviceData Hook
    participant useDeviceStomp as useDeviceStomp Hook
    participant SockJS as SockJS/STOMP Client
    participant Server as Backend Server

    Home->>useDeviceData: initialize
    useDeviceData->>useDeviceData: load devices from localStorage
    useDeviceData->>useDeviceStomp: init(serverUrl)
    useDeviceStomp->>SockJS: create & activate client
    SockJS->>Server: open WebSocket (STOMP)
    Server-->>SockJS: accept connection
    SockJS-->>useDeviceStomp: isConnected = true

    useDeviceData->>useDeviceStomp: subscribeDevice(deviceId)
    useDeviceStomp->>Server: SUBSCRIBE /sub/device/{deviceId}
    Server-->>useDeviceStomp: SEND device payload (JSON)
    useDeviceStomp->>useDeviceData: update deviceData[deviceId]
    useDeviceData->>useDeviceData: compute liveDevices (merge local + live)
    useDeviceData-->>Home: return liveDevices
    Home->>Home: render DeviceCardItem(s)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

enhancement

Poem

🐰
I hopped through sockets, soft and bright,
nibbling packets in the night,
temps that whisper, warnings beep,
devices waking from their sleep,
a tiny hop—real-time delight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '#23/socket connection' clearly describes the main objective of adding socket connectivity. It directly relates to the primary changes: new STOMP WebSocket integration via useDeviceStomp hook and related device data live-sync functionality.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch #23/socket-connection

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.env:
- Line 1: The checked-in .env currently hardcodes VITE_API_BASE_URL to a
developer-specific LAN IP; change the committed .env to a shared default (e.g.,
localhost or an empty placeholder) and move the machine-specific value into
.env.local; update VITE_API_BASE_URL in the file (referencing the
VITE_API_BASE_URL variable) to the shared default and ensure .env.local is used
for overrides and is gitignored so personal endpoints aren't committed.

In `@src/components/DeviceDetail.tsx`:
- Line 9: The DeviceDetail component currently types the prop as id?: string but
always constructs `/device/${id}`; either make the prop required by changing
id?: string to id: string on the DeviceDetail props, or add a guard where the
navigation is built (the code that constructs `/device/${id}` / the
onClick/navigation handler or MenuItem click) to early-return or disable/hide
the navigation action when id is falsy; update the UI to disable the menu item
or avoid calling navigate/link construction when id is undefined so you never
route to `/device/undefined`.

In `@src/components/modal/DeviceRegisterModal.tsx`:
- Line 7: The addDevice prop on DeviceRegisterModal is two-arg (serialNumber,
name) but the hook useDeviceData currently exposes addDevice(id: string); update
the hook and its persistence path to accept (serialNumber: string, name?:
string) and persist the name: change the addDevice function in useDeviceData to
signature addDevice(serialNumber: string, name?: string), update its internal
logic that reads/writes localStorage (or save/load helpers) to store the name
alongside the serial, update the hook's return typing to the new signature, and
adjust any callers of useDeviceData/addDevice to pass both values (or handle
optional name) so DeviceRegisterModal's generated name is actually saved.

In `@src/hooks/useDeviceData.ts`:
- Around line 41-62: The addDevice function currently checks duplicates against
the render snapshot and calls subscribeDevice, which can allow race conditions;
instead, move the duplicate check into the setDevices updater by using
setDevices(prev => { if (prev.some(d => d.id === id)) return prev; return
[...prev, newDevice]; }) so the addition is atomic, and remove the
subscribeDevice(id) call and related console.log from addDevice so the existing
effect that watches devices can handle socket subscription side effects. Ensure
you still construct the same newDevice object (id, name, temperature, warning,
hasShownWarning) inside the updater path so state stays identical.
- Around line 9-10: The hook currently parses localStorage entries directly
(e.g., in useDeviceData.ts where useState<Device[]>(...) initializes devices
from JSON.parse(localStorage.getItem("devices") || "[]")) which will throw on
malformed data; add a small shared safe JSON parser (e.g., safeParseJSON<T>(raw:
string | null, fallback: T): T) and replace direct JSON.parse calls for
"devices" and "deviceNames" (and the similar reads at the other occurrences
referenced) with safeParseJSON(localStorage.getItem("devices"), []) and
safeParseJSON(localStorage.getItem("deviceNames"), {}) so the hook falls back to
sensible defaults instead of throwing during render. Ensure the helper is used
for all occurrences (initializers and any subsequent localStorage reads/writes)
and returns typed defaults for Device[] and the device name mapping.
- Around line 13-15: The current persistence stores a permanent hasShownWarning
boolean (see checkWarning() and the effect that sets localStorage key
"devices"), which prevents future modals for a device after one acknowledgment;
change the logic to reset hasShownWarning to false when the device's risk drops
below the warning threshold (or implement an episode-based token instead of a
permanent boolean) and update the data written by the useEffect that saves
"devices" so it reflects the reset; ensure the same fix is applied to the other
similar flag usages in this file (the other checkWarning-like spots referenced).

In `@src/hooks/useDeviceStomp.ts`:
- Around line 50-56: UnsubscribeDevice currently only removes the Stomp
subscription; also remove any stale cached telemetry for that device so
reconnections don't reuse old frames—after calling subscription.unsubscribe()
and StompSubscriptionRef.current.delete(deviceId) add a call to clear the
device's cached payload in the useDeviceData cache (e.g. delete from the device
telemetry cache reference or call a clearDeviceTelemetry(deviceId) helper).
Update unsubscribeDevice to reference the same cache/clearing API used by
useDeviceData so the cached entry for deviceId is removed when unsubscribing.
- Around line 13-23: The hook useDeviceStomp must rebuild subscriptions inside
the Client.onConnect handler and detect unexpected disconnects via
Client.onWebSocketClose: maintain a separate set/map of "desired" deviceIds to
be subscribed (instead of relying on stale StompSubscriptionRef entries), clear
or mark all existing StompSubscriptionRef entries as invalid on
onWebSocketClose, and in onConnect iterate the desired set and call
client.subscribe() for each only after verifying client.connected is true,
replacing the stored StompSubscriptionRef with the new subscription; also ensure
any subscribe/unsubscribe helpers update the desired set so re-subscription on
reconnect reflects current intent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8010a26c-be8f-4163-817b-b44bc20471a5

📥 Commits

Reviewing files that changed from the base of the PR and between 586d76e and 948bfa9.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • .env
  • package.json
  • src/components/DeviceCardItem.tsx
  • src/components/DeviceDetail.tsx
  • src/components/modal/DeviceRegisterModal.tsx
  • src/hooks/useDeviceData.ts
  • src/hooks/useDeviceStomp.ts
  • src/pages/home/home.tsx
  • src/types/device.ts
  • vite.config.ts

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/pages/home/home.tsx (2)

2-2: ⚠️ Potential issue | 🟡 Minor

Unused import: Sidebar.

Sidebar is imported but not used anywhere in the component. This suggests the refactoring is incomplete or the import should be removed.

-import { Sidebar } from "../../components/sidebar/sidebar.tsx";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/pages/home/home.tsx` at line 2, Remove the unused Sidebar import to clean
up the module: delete the import statement that references the Sidebar symbol
(import { Sidebar } from "../../components/sidebar/sidebar.tsx") since Sidebar
is not referenced anywhere in the home component; alternatively, if Sidebar was
meant to be used, add it into the JSX of the Home component where appropriate
(e.g., include <Sidebar />) and ensure any related props are passed and tests
updated.

35-70: ⚠️ Potential issue | 🔴 Critical

Critical: Broken JSX structure — incomplete edit or merge conflict.

The code has overlapping JSX fragments that result in invalid syntax. Lines 35-50 introduce a new <Header> and <CardGrid> structure, but lines 51-70 retain old code that should have been removed. This causes:

  • Unclosed <CardGrid> at line 42
  • Mismatched </Header> at line 53
  • Duplicate <Header>, <CardGrid>, and <DeviceCardItem> blocks
  • Reference to <MainContent> closing tag (line 70) without an opening tag

The pipeline failures confirm the code cannot be parsed or compiled.

🐛 Proposed fix — remove duplicate code and close tags properly
       <Header>
         <DeviceText>연결된 기기</DeviceText>
         <DeviceDeleteButton
           onClick={() => toggleDeleteMode(devices.map((device) => device.id))}
           isDeleteMode={isDeleteMode}
         />
       </Header>
       <CardGrid>
         <DeviceAddCard onClick={() => toggleAddMode()} />
         {warningDevices.map((device) => (
           <DeviceCardItem
             key={device.id}
             id={device.id}
             deviceName={device.name}
             temperature={device.temperature}
             warning={device.warning}
-            isDeleteMode={isDeleteMode}
-          />
-        </Header>
-        <CardGrid>
-          <DeviceAddCard onClick={() => toggleAddMode()} />
-          {warningDevices.map((device) => (
-            <DeviceCardItem
-              key={device.id}
-              id={device.id}
-              deviceName={device.name}
-              temperature={device.temperature}
-              warning={device.temperature > 70}
-              isDeleteMode={isDeleteMode}
-              isSelected={selectedItems.includes(device.id)}
-              onSelect={() => toggleItemSelection(device.id)}
-              updateDeviceName={updateDeviceName}
-            />
-          ))}
-        </CardGrid>
-      </MainContent>
+            isDeleteMode={isDeleteMode}
+            isSelected={selectedItems.includes(device.id)}
+            onSelect={() => toggleItemSelection(device.id)}
+            updateDeviceName={updateDeviceName}
+          />
+        ))}
+      </CardGrid>
       {warningDevices.some((device) => device.warning) && (

Also review which props you intend to keep for DeviceCardItem — the old code has isSelected, onSelect, and updateDeviceName which are missing from the new fragment, and warning calculation differs (device.warning vs device.temperature > 70).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/pages/home/home.tsx` around lines 35 - 70, Remove the
duplicated/malformed JSX fragments so the component has one consistent
Header/CardGrid/MainContent structure: keep a single <Header> with DeviceText
and DeviceDeleteButton (using toggleDeleteMode and isDeleteMode), then a single
<CardGrid> beginning with <DeviceAddCard onClick={toggleAddMode} /> followed by
the intended list of <DeviceCardItem> elements; ensure the CardGrid and Header
tags are properly closed and the lone closing </MainContent> has a matching
opening. Reconcile which DeviceCardItem props you want to keep—either preserve
device.warning or compute warning as device.temperature > 70, and include
isSelected, onSelect (toggleItemSelection), and updateDeviceName if needed—then
remove the old duplicate block that left mismatched </Header> and unclosed tags
so the JSX parses cleanly.
🧹 Nitpick comments (2)
src/pages/home/home.tsx (2)

112-116: Unused styled component: MainContent.

The MainContent styled component is defined but no longer used after the refactoring (assuming the duplicate code at lines 53-70 is removed). Either remove this definition or ensure it's integrated into the corrected JSX structure if the sidebar layout is still needed.

-// Sidebar가 position: fixed(폭 280px)이므로
-// 메인 콘텐츠는 그 오른쪽에서 시작하도록 여백을 둠
-const MainContent = styled.div`
-  margin-left: 432px;
-  padding: 100px 40px 40px;
-  box-sizing: border-box;
-`;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/pages/home/home.tsx` around lines 112 - 116, MainContent styled component
(const MainContent = styled.div`) is defined but unused; either delete the
MainContent declaration to remove dead code or reintroduce it into the JSX where
the main page content should be wrapped (replace the current top-level content
container with <MainContent> or wrap the existing content inside MainContent) so
the sidebar/margin/padding layout is applied; update any imports/exports if
needed and remove the unused styled.div symbol if you choose deletion.

71-83: Inefficient repeated find() calls for the same warning device.

warningDevices.find((device) => device.warning) is called three times to retrieve the same device. This duplicates iteration and is fragile if the list changes between calls.

♻️ Proposed refactor — compute once

Use a single variable (similar to warningModalDevice at line 31, but for the warning flag):

const activeWarningDevice = warningDevices.find((device) => device.warning);

// Then in JSX:
{activeWarningDevice && (
  <WarningModal
    deviceName={activeWarningDevice.name}
    deviceTemp={activeWarningDevice.temperature}
    checkWarning={() => checkWarning(activeWarningDevice.id)}
  />
)}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/pages/home/home.tsx` around lines 71 - 83, Compute the warning device
once and reuse it instead of calling warningDevices.find(...) multiple times:
create a local const (e.g., activeWarningDevice) by calling
warningDevices.find((device) => device.warning) and then render WarningModal
only if activeWarningDevice is truthy, passing activeWarningDevice.name,
activeWarningDevice.temperature, and a checkWarning callback that calls
checkWarning(activeWarningDevice.id). Update the JSX that currently calls
warningDevices.find(...) three times to reference this single variable.
🤖 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/pages/home/home.tsx`:
- Line 31: Remove or integrate the unused variable `warningModalDevice`: either
delete the line that defines `warningModalDevice` (which searches
`warningDevices` for `device.showModal`) or change the `WarningModal` usage to
use that found object; specifically, update the `WarningModal` render logic
(currently using `device.warning`) to accept the `warningModalDevice` (so it
checks `warningModalDevice?.showModal` and uses its data) or drop
`warningModalDevice` and consistently use `device.warning`/existing props from
`warningDevices`. Ensure you modify references around the `WarningModal`
component so there are no remaining unused variables.

---

Outside diff comments:
In `@src/pages/home/home.tsx`:
- Line 2: Remove the unused Sidebar import to clean up the module: delete the
import statement that references the Sidebar symbol (import { Sidebar } from
"../../components/sidebar/sidebar.tsx") since Sidebar is not referenced anywhere
in the home component; alternatively, if Sidebar was meant to be used, add it
into the JSX of the Home component where appropriate (e.g., include <Sidebar />)
and ensure any related props are passed and tests updated.
- Around line 35-70: Remove the duplicated/malformed JSX fragments so the
component has one consistent Header/CardGrid/MainContent structure: keep a
single <Header> with DeviceText and DeviceDeleteButton (using toggleDeleteMode
and isDeleteMode), then a single <CardGrid> beginning with <DeviceAddCard
onClick={toggleAddMode} /> followed by the intended list of <DeviceCardItem>
elements; ensure the CardGrid and Header tags are properly closed and the lone
closing </MainContent> has a matching opening. Reconcile which DeviceCardItem
props you want to keep—either preserve device.warning or compute warning as
device.temperature > 70, and include isSelected, onSelect (toggleItemSelection),
and updateDeviceName if needed—then remove the old duplicate block that left
mismatched </Header> and unclosed tags so the JSX parses cleanly.

---

Nitpick comments:
In `@src/pages/home/home.tsx`:
- Around line 112-116: MainContent styled component (const MainContent =
styled.div`) is defined but unused; either delete the MainContent declaration to
remove dead code or reintroduce it into the JSX where the main page content
should be wrapped (replace the current top-level content container with
<MainContent> or wrap the existing content inside MainContent) so the
sidebar/margin/padding layout is applied; update any imports/exports if needed
and remove the unused styled.div symbol if you choose deletion.
- Around line 71-83: Compute the warning device once and reuse it instead of
calling warningDevices.find(...) multiple times: create a local const (e.g.,
activeWarningDevice) by calling warningDevices.find((device) => device.warning)
and then render WarningModal only if activeWarningDevice is truthy, passing
activeWarningDevice.name, activeWarningDevice.temperature, and a checkWarning
callback that calls checkWarning(activeWarningDevice.id). Update the JSX that
currently calls warningDevices.find(...) three times to reference this single
variable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f7a4c0d-d575-4ea3-aa71-dd7bdbc1e019

📥 Commits

Reviewing files that changed from the base of the PR and between 948bfa9 and 9fa7f7b.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • package.json
  • src/components/DeviceCardItem.tsx
  • src/pages/home/home.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • src/components/DeviceCardItem.tsx

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/pages/home/home.tsx (1)

14-18: Add an explicit return type to useDeviceData() that includes the showModal property.

The hook adds showModal to devices at runtime (line 34), but the shared Device type doesn't declare this field. Export an explicit return type from useDeviceData (e.g., by typing the return statement or creating a WarningDevice type that extends Device with showModal) to make this contract clear to consumers and improve type safety.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/pages/home/home.tsx` around lines 14 - 18, The hook useDeviceData
currently augments devices with a runtime-only showModal property but its return
type isn't updated; update useDeviceData's exported return type so consumers see
showModal (e.g., create a WarningDevice (or DeviceWithShowModal) type that
extends Device with showModal: boolean and use that in the hook's return type or
explicitly type the returned devices array and warningDevices), then ensure
callers like warningModalDevice = warningDevices.find((device) =>
device.showModal) rely on the new typed shape; this makes the contract explicit
and fixes TS errors when accessing showModal.
🤖 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/pages/home/home.tsx`:
- Around line 79-83: MainContent is using a hardcoded margin-left: 624px which
mismatches the actual Sidebar width (280px); update MainContent (styled.div) to
derive its left margin from the Sidebar width instead of the magic number—either
reference the existing Sidebar width constant/variable (e.g., use the same
SCSS/CSS variable or theme value used by Sidebar) or replace the hardcoded 624px
with a computed value like the Sidebar width (280px) or a CSS variable/calc that
reads the single source of truth; ensure you update any related layout styles so
MainContent aligns with Sidebar without the oversized gutter.

---

Nitpick comments:
In `@src/pages/home/home.tsx`:
- Around line 14-18: The hook useDeviceData currently augments devices with a
runtime-only showModal property but its return type isn't updated; update
useDeviceData's exported return type so consumers see showModal (e.g., create a
WarningDevice (or DeviceWithShowModal) type that extends Device with showModal:
boolean and use that in the hook's return type or explicitly type the returned
devices array and warningDevices), then ensure callers like warningModalDevice =
warningDevices.find((device) => device.showModal) rely on the new typed shape;
this makes the contract explicit and fixes TS errors when accessing showModal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4fd68469-b6d2-47ca-9f90-9f657f7c2125

📥 Commits

Reviewing files that changed from the base of the PR and between 9fa7f7b and 5fffe6e.

📒 Files selected for processing (3)
  • .env
  • src/components/DeviceDetail.tsx
  • src/pages/home/home.tsx
💤 Files with no reviewable changes (1)
  • .env
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/components/DeviceDetail.tsx

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/hooks/useDeviceData.ts (2)

13-15: ⚠️ Potential issue | 🟠 Major

Warning acknowledgement is still permanent.

showModal is gated by !device.hasShownWarning, checkWarning() only ever flips that flag to true, and the persistence effect stores it. After one acknowledgement, later risk spikes for the same device will never show the modal again unless the device is deleted.

Also applies to: 33-34, 69-70

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useDeviceData.ts` around lines 13 - 15, The device
warning-acknowledgement flag (device.hasShownWarning) is being persisted by the
useEffect that calls localStorage.setItem("devices", ...) and never reset, so
once checkWarning() sets hasShownWarning=true the modal (showModal) will never
reappear for future higher-risk spikes; modify checkWarning (or the update path
that computes device.risk) to clear or re-evaluate hasShownWarning when the
device's risk falls below the warning threshold or when the new risk exceeds the
previous acknowledged risk (e.g., set device.hasShownWarning = false when risk <
threshold or when currentRisk > lastAcknowledgedRisk), or implement an
expiry/version/timestamp for acknowledgements so the persisted devices array
stored by the useEffect will no longer permanently suppress showModal for
subsequent risk changes.

10-10: ⚠️ Potential issue | 🟠 Major

Handle malformed localStorage values before parsing.

These JSON.parse(...) calls still throw if "devices" or "deviceNames" already contains invalid JSON, so the hook can fail during initial render or on add/rename. The safe parser requested in the earlier review still needs to be applied at all three read sites.

Also applies to: 42-42, 75-77

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useDeviceData.ts` at line 10, Replace the direct
JSON.parse(localStorage.getItem("devices") || "[]") and the two other JSON.parse
reads for the "deviceNames" key with a safe parse that catches malformed JSON
and returns a sensible default; implement or reuse a small helper (e.g.,
safeParseJson) and use it at each read site instead of JSON.parse so on error
you return [] (for devices) or {}/[] (for deviceNames as appropriate) and
optionally log a warning, ensuring all three occurrences in useDeviceData.ts use
the helper.
🤖 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/hooks/useDeviceData.ts`:
- Around line 17-23: The effect in useDeviceData calls subscribeDevice for every
device on each render, which can create duplicate STOMP subscriptions; update
the subscribeDevice implementation in useDeviceStomp (and related helpers like
unsubscribeDevice) so it maintains a Map/Record keyed by device.id, checks
map.has(id) and returns early if already subscribed, stores the Subscription
object when subscribing, and ensures unsubscribeDevice removes the entry and
calls subscription.unsubscribe(); also ensure any reconnect/cleanup paths
consult this map so subscriptions are not duplicated.

---

Duplicate comments:
In `@src/hooks/useDeviceData.ts`:
- Around line 13-15: The device warning-acknowledgement flag
(device.hasShownWarning) is being persisted by the useEffect that calls
localStorage.setItem("devices", ...) and never reset, so once checkWarning()
sets hasShownWarning=true the modal (showModal) will never reappear for future
higher-risk spikes; modify checkWarning (or the update path that computes
device.risk) to clear or re-evaluate hasShownWarning when the device's risk
falls below the warning threshold or when the new risk exceeds the previous
acknowledged risk (e.g., set device.hasShownWarning = false when risk <
threshold or when currentRisk > lastAcknowledgedRisk), or implement an
expiry/version/timestamp for acknowledgements so the persisted devices array
stored by the useEffect will no longer permanently suppress showModal for
subsequent risk changes.
- Line 10: Replace the direct JSON.parse(localStorage.getItem("devices") ||
"[]") and the two other JSON.parse reads for the "deviceNames" key with a safe
parse that catches malformed JSON and returns a sensible default; implement or
reuse a small helper (e.g., safeParseJson) and use it at each read site instead
of JSON.parse so on error you return [] (for devices) or {}/[] (for deviceNames
as appropriate) and optionally log a warning, ensuring all three occurrences in
useDeviceData.ts use the helper.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fc468476-cee2-4515-95d6-48289eaed205

📥 Commits

Reviewing files that changed from the base of the PR and between 5fffe6e and a1297b5.

📒 Files selected for processing (1)
  • src/hooks/useDeviceData.ts

@JAEJUN090210 JAEJUN090210 merged commit 4c8ec80 into main Mar 12, 2026
3 checks passed
@JAEJUN090210 JAEJUN090210 deleted the #23/socket-connection branch March 12, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants