ステータスバーを追加#48
Hidden character warning
Conversation
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces an EnvironmentStatusBar component to display real-time environmental data in the application header. Key changes include moving device fetching to the App level, adding a persistent pinnedEnvironmentDeviceId setting to the Redux store, and updating the UI to allow sensor selection. The review feedback focuses on performance optimizations within the new component, specifically recommending the use of useMemo and useCallback to memoize device filtering, device lookup, and value formatting logic.
| @@ -0,0 +1,176 @@ | |||
| import React, { useState } from "react"; | |||
| const envDevices = devices.filter((device) => { | ||
| const def = findDeviceDefinition(device.deviceType); | ||
| return def && ENVIRONMENT_DEVICE_KEYS.has(def.key); | ||
| }); |
There was a problem hiding this comment.
devices 配列のフィルタリング処理は計算コストがかかる可能性があり、また devices は Redux の状態更新(ポーリング等)により頻繁に更新されるため、useMemo を使用して最適化することをお勧めします。
| const envDevices = devices.filter((device) => { | |
| const def = findDeviceDefinition(device.deviceType); | |
| return def && ENVIRONMENT_DEVICE_KEYS.has(def.key); | |
| }); | |
| const envDevices = useMemo(() => { | |
| return devices.filter((device) => { | |
| const def = findDeviceDefinition(device.deviceType); | |
| return def && ENVIRONMENT_DEVICE_KEYS.has(def.key); | |
| }); | |
| }, [devices]); |
| // Nothing to show if no environment-capable devices exist | ||
| if (envDevices.length === 0) return null; | ||
|
|
||
| const pinnedDevice = pinnedId ? envDevices.find((d) => d.deviceId === pinnedId) : undefined; |
| const getFormattedValue = (deviceId: string, fieldKey: string): string | undefined => { | ||
| const device = devices.find((d) => d.deviceId === deviceId); | ||
| if (!device) return undefined; | ||
| const statusFields = getStatusFieldsForDevice(device.deviceType); | ||
| const field = statusFields.find((f: any) => f.key === fieldKey); | ||
| if (!field) return undefined; | ||
| const status = statusMap[deviceId]; | ||
| const raw = status ? (status as any)[field.key] : undefined; | ||
| if (raw === undefined || raw === null) return undefined; | ||
| return field.formatter ? String(field.formatter(raw, status)) : String(raw); | ||
| }; |
There was a problem hiding this comment.
getFormattedValue 関数はレンダリングのたびに再生成されています。また、この関数は Menu 内のループでも呼び出されるため、useCallback でラップしてメモ化することをお勧めします。併せて、statusFields.find の引数における不要な : any 型指定も削除しています。
| const getFormattedValue = (deviceId: string, fieldKey: string): string | undefined => { | |
| const device = devices.find((d) => d.deviceId === deviceId); | |
| if (!device) return undefined; | |
| const statusFields = getStatusFieldsForDevice(device.deviceType); | |
| const field = statusFields.find((f: any) => f.key === fieldKey); | |
| if (!field) return undefined; | |
| const status = statusMap[deviceId]; | |
| const raw = status ? (status as any)[field.key] : undefined; | |
| if (raw === undefined || raw === null) return undefined; | |
| return field.formatter ? String(field.formatter(raw, status)) : String(raw); | |
| }; | |
| const getFormattedValue = useCallback((deviceId: string, fieldKey: string): string | undefined => { | |
| const device = devices.find((d) => d.deviceId === deviceId); | |
| if (!device) return undefined; | |
| const statusFields = getStatusFieldsForDevice(device.deviceType); | |
| const field = statusFields.find((f) => f.key === fieldKey); | |
| if (!field) return undefined; | |
| const status = statusMap[deviceId]; | |
| const raw = status ? (status as any)[field.key] : undefined; | |
| if (raw === undefined || raw === null) return undefined; | |
| return field.formatter ? String(field.formatter(raw, status)) : String(raw); | |
| }, [devices, statusMap]); |
| const temp = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "temperature") : undefined; | ||
| const hum = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "humidity") : undefined; | ||
| const co2 = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "CO2") : undefined; |
There was a problem hiding this comment.
ステータスバーに表示する主要な環境数値(温度・湿度・CO2)は、依存関係が変更されたときのみ再計算されるように useMemo でメモ化することをお勧めします。
| const temp = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "temperature") : undefined; | |
| const hum = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "humidity") : undefined; | |
| const co2 = pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "CO2") : undefined; | |
| const { temp, hum, co2 } = useMemo(() => ({ | |
| temp: pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "temperature") : undefined, | |
| hum: pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "humidity") : undefined, | |
| co2: pinnedDevice ? getFormattedValue(pinnedDevice.deviceId, "CO2") : undefined, | |
| }), [pinnedDevice, getFormattedValue]); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 242f71628b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isTokenValid && storedToken) { | ||
| dispatch(fetchDevices()); | ||
| } |
There was a problem hiding this comment.
Clear cached devices when auth is no longer valid
This effect fetches devices after validation succeeds but has no corresponding branch to clear previously loaded devices when credentials are removed or become invalid. In that scenario, devices/deviceStatuses stay populated and the new global EnvironmentStatusBar can continue showing stale sensor data from the prior authenticated session until another successful fetch occurs. Add a cleanup path (for example dispatching clearDevices() when !isTokenValid or token is missing) so UI state matches auth state.
Useful? React with 👍 / 👎.
No description provided.