デバイス/シーンの並び替え対応#11
Conversation
|
@codex review |
Summary of ChangesHello @0suu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、ユーザーがアプリケーション内のデバイスとシーンの表示順序をカスタマイズできるようにする重要な機能を追加します。ドラッグ＆ドロップによる直感的な操作と、その順序をアプリケーション全体で永続化する機能を提供することで、ユーザーエクスペリエンスが大幅に向上します。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
このプルリクエストは、デバイスとシーンのリストをユーザーが並べ替えられるようにする新機能を追加するもので、素晴らしい改善です。コード全体がよく構成されており、ドラッグ＆ドロップ機能の実装も堅実です。
いくつかの改善点として、DeviceListScreen.tsxとScenesScreen.tsxで重複しているロジックの共通化、未使用コードの削除、そしていくつかのロジックの簡潔化を提案します。これらの変更により、コードの保守性と可読性がさらに向上するでしょう。
詳細は各ファイルのコメントをご確認ください。
| const applyDeviceOrder = (devices: AnyDevice[], desiredOrder: string[]) => { | ||
| const idToDevice = new Map(devices.map((device) => [device.deviceId, device])); | ||
| const seen = new Set<string>(); | ||
| const orderedDevices: AnyDevice[] = []; | ||
|
|
||
| desiredOrder.forEach((id) => { | ||
| const device = idToDevice.get(id); | ||
| if (device && !seen.has(id)) { | ||
| orderedDevices.push(device); | ||
| seen.add(id); | ||
| } | ||
| }); | ||
|
|
||
| devices.forEach((device) => { | ||
| if (!seen.has(device.deviceId)) { | ||
| orderedDevices.push(device); | ||
| seen.add(device.deviceId); | ||
| } | ||
| }); | ||
|
|
||
| const normalizedOrder = orderedDevices.map((device) => device.deviceId); | ||
| return { orderedDevices, normalizedOrder }; | ||
| }; |
There was a problem hiding this comment.
このapplyDeviceOrder関数は定義されていますが、スライス内のどこからも呼び出されていないようです。DeviceListScreen.tsxでは同様のロジックがuseMemo内で再実装されています。
この関数は現在使用されていないため、コードの簡潔さを保つために削除することを検討してください。
| const orderedDevices = useMemo(() => { | ||
| const idToDevice = new Map<string, AnyDevice>(); | ||
| devices.forEach((device) => { | ||
| idToDevice.set(device.deviceId, device); | ||
| }); | ||
| const seen = new Set<string>(); | ||
| const ordered: AnyDevice[] = []; | ||
|
|
||
| deviceOrder.forEach((id) => { | ||
| const device = idToDevice.get(id); | ||
| if (device && !seen.has(id)) { | ||
| ordered.push(device); | ||
| seen.add(id); | ||
| } | ||
| }); | ||
|
|
||
| devices.forEach((device) => { | ||
| if (!seen.has(device.deviceId)) { | ||
| ordered.push(device); | ||
| } | ||
| }); | ||
|
|
||
| return ordered; | ||
| }, [devices, deviceOrder]); |
There was a problem hiding this comment.
このuseMemoフック内の並べ替えロジックは、より簡潔で効率的に記述できます。現在の実装は正しいですが、Mapオブジェクトをうまく活用することで、可読性を向上させることができます。
以下に修正案を提案します。Mapから要素をdeleteすることで、2回目のdevices配列のループが不要になります。
const orderedDevices = useMemo(() => {
const deviceMap = new Map(devices.map((d) => [d.deviceId, d]));
const ordered: AnyDevice[] = [];
// 保存された順序でデバイスを追加
for (const id of deviceOrder) {
const device = deviceMap.get(id);
if (device) {
ordered.push(device);
deviceMap.delete(id); // 重複を避けるためにマップから削除
}
}
// 残りのデバイスを末尾に追加
ordered.push(...deviceMap.values());
return ordered;
}, [devices, deviceOrder]);
| const handleReorder = (visibleOrder: string[]) => { | ||
| setReorderList(visibleOrder); | ||
| }; |
There was a problem hiding this comment.
このhandleReorder関数は定義されていますが、コンポーネント内で使用されていないようです。以前の実装の名残かもしれません。
コードをクリーンに保つために、この未使用の関数は削除することをお勧めします。
| const orderedScenes: SceneSummary[] = useMemo(() => { | ||
| const idToScene = new Map<string, SceneSummary>(); | ||
| scenes.forEach((scene) => idToScene.set(scene.sceneId, scene)); | ||
| const seen = new Set<string>(); | ||
| const ordered: SceneSummary[] = []; | ||
|
|
||
| sceneOrder.forEach((id) => { | ||
| const scene = idToScene.get(id); | ||
| if (scene && !seen.has(id)) { | ||
| ordered.push(scene); | ||
| seen.add(id); | ||
| } | ||
| }); | ||
|
|
||
| scenes.forEach((scene) => { | ||
| if (!seen.has(scene.sceneId)) { | ||
| ordered.push(scene); | ||
| } | ||
| }); | ||
|
|
||
| return ordered; | ||
| }, [scenes, sceneOrder]); |
There was a problem hiding this comment.
DeviceListScreen.tsxと同様に、このuseMemoフック内の並べ替えロジックも、より簡潔で効率的に記述できます。
以下に修正案を提案します。
const orderedScenes: SceneSummary[] = useMemo(() => {
const sceneMap = new Map(scenes.map((s) => [s.sceneId, s]));
const ordered: SceneSummary[] = [];
// 保存された順序でシーンを追加
for (const id of sceneOrder) {
const scene = sceneMap.get(id);
if (scene) {
ordered.push(scene);
sceneMap.delete(id); // 重複を避けるためにマップから削除
}
}
// 残りのシーンを末尾に追加
ordered.push(...sceneMap.values());
return ordered;
}, [scenes, sceneOrder]);
| const handleReorder = (visibleOrder: string[]) => { | ||
| setReorderList(visibleOrder); | ||
| }; |
There was a problem hiding this comment.
このhandleReorder関数は定義されていますが、コンポーネント内で使用されていないようです。DeviceListScreen.tsxと同様に、以前の実装の名残である可能性があります。
コードの整理のため、この未使用の関数を削除することをお勧めします。
| const finishReorder = () => { | ||
| const currentOrder = orderedScenes.map((scene) => scene.sceneId); | ||
| const mergedOrder = | ||
| reorderList.length > 0 | ||
| ? [...reorderList, ...currentOrder.filter((id) => !reorderList.includes(id))] | ||
| : currentOrder; | ||
| dispatch(setSceneOrder(mergedOrder)); | ||
| setIsReorderMode(false); | ||
| }; |
There was a problem hiding this comment.
finishReorder関数のロジックは、現在の実装では少し冗長になっています。シーンのリストには非表示の項目がないため、reorderListが常にすべてのシーンIDを含んでいます。そのため、mergedOrderの計算は単純化できます。
これにより、コードがより直感的になります。
const finishReorder = () => {
dispatch(setSceneOrder(reorderList));
setIsReorderMode(false);
};
![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.
ℹ️ 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".
| clearScenesState: () => ({ | ||
| ...initialState, | ||
| }), |
There was a problem hiding this comment.
The new clearScenesState reducer now resets sceneOrder and sceneOrderLoaded to their empty defaults. Because clearScenesState is triggered whenever the API token is missing/invalid (ScenesScreen.tsx lines 101-105) and loadSceneOrder is only dispatched once at app startup (App.tsx lines 57-60), any user-defined scene order is wiped for the rest of the session as soon as credentials are edited or cleared; the list falls back to API order until the app is restarted even though the order was persisted. Consider preserving the stored order when clearing scenes or reloading it after clearing so reordering survives credential changes within the same session.
Useful? React with 👍 / 👎.
1 participant
No description provided.