ステータスバーに表示できるデバイスを追加 - #49
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the EnvironmentStatusBar to support pinning multiple devices and adds power consumption display for 'plug' devices. The settingsSlice was updated to manage an array of pinned device IDs and includes a migration path for legacy single-device settings. Feedback was provided to improve the rendering logic for dividers between device readings to prevent empty separators and to move side-effect-heavy cleanup logic out of the Redux reducer for better architectural alignment and efficiency.
| {visibleDevices.map((device, idx) => { | ||
| const readings = renderDeviceReadings(device.deviceId, device.deviceType); | ||
| if (!readings) return null; | ||
| return ( | ||
| <React.Fragment key={device.deviceId}> | ||
| {idx > 0 && ( | ||
| <Divider | ||
| orientation="vertical" | ||
| flexItem | ||
| sx={{ mx: 0.25, my: 0.5, borderColor: "divider" }} | ||
| /> | ||
| )} | ||
| <Box sx={{ display: "flex", alignItems: "center", gap: 0 }}> | ||
| {readings} | ||
| </Box> | ||
| </React.Fragment> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
visibleDevices のループ内で Divider を表示する判定に idx > 0 を使用していますが、renderDeviceReadings が null を返すデバイス（ステータスが未取得のデバイスなど）がリストの最初の方に含まれている場合、実際に表示される最初の要素の前に不要な Divider が表示されてしまう可能性があります。
表示内容があるデバイスのみを事前にフィルタリングしてから map を回すことで、この問題を回避できます。また、これにより renderDeviceReadings の呼び出し回数も最適化されます。
{visibleDevices
.map((device) => ({
device,
readings: renderDeviceReadings(device.deviceId, device.deviceType),
}))
.filter((item) => item.readings !== null)
.map(({ device, readings }, idx) => (
<React.Fragment key={device.deviceId}>
{idx > 0 && (
<Divider
orientation="vertical"
flexItem
sx={{ mx: 0.25, my: 0.5, borderColor: "divider" }}
/>
)}
<Box sx={{ display: "flex", alignItems: "center", gap: 0 }}>
{readings}
</Box>
</React.Fragment>
))}
| deleteSetting("pinnedEnvironmentDeviceId", "pinned environment device"); | ||
| deleteSetting("pinnedEnvironmentDeviceIds", "pinned status bar devices"); | ||
| } | ||
| deleteSetting("pinnedEnvironmentDeviceId", "legacy pinned environment device"); |
There was a problem hiding this comment.
レガシーな設定キー pinnedEnvironmentDeviceId の削除処理が setPinnedEnvironmentDeviceIds reducer 内に含まれていますが、これによりピン留め設定を変更するたびに削除処理が実行されます。移行処理は loadApiCredentials thunk 内で完結させるか、一度削除されたら実行されないように制御することを検討してください。
また、Redux の reducer 内で deleteSetting などのサイドエフェクトを直接実行するのは推奨されないパターン（純粋関数であるべき）ですが、プロジェクトの既存の慣習に従っているものと理解しています。
Deploying switchbotcontroller with
|
| Latest commit: |
d878bd3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eaa40fac.switchbotcontroller.pages.dev |
| Branch Preview URL: | https://add-status-bar-compatible-de.switchbotcontroller.pages.dev |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c854a6762f
ℹ️ 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".
| .filter((d): d is (typeof candidateDevices)[number] => Boolean(d)); | ||
|
|
||
| const pinnedDevice = pinnedId ? envDevices.find((d) => d.deviceId === pinnedId) : undefined; | ||
| const visibleDevices = pinnedDevices.slice(0, maxVisible); |
There was a problem hiding this comment.
visibleDevices is sliced from pinnedDevices before checking whether those devices actually have renderable readings, so on smaller breakpoints a couple of offline/unreported pins can consume all slots and hide later pins that do have data. In EnvironmentStatusBar, this makes the bar show — even when a pinned device further down the list has valid readings (for example, pinned order [A(no status), B(no status), C(has data)] with maxVisible=2). Consider filtering to devices with readings (or otherwise skipping unreadable entries) before applying the maxVisible limit.
Useful? React with 👍 / 👎.
c854a67 to
d878bd3
Compare
1 participant
No description provided.