描画エンジンをsrc/drawing-engine/に分離 + 頂点削除機能を追加#50
Merged
sugama-satsuki merged 3 commits intomainfrom Mar 2, 2026
Merged
Conversation
MapView.tsx（590行）に密結合していた描画ロジックを src/drawing-engine/ として モジュール化。将来的に別リポジトリ・npmパッケージとして切り出せる形にした。 - useDrawingEngine フック新規作成: レイヤー管理・イベント処理・状態管理を統合 - コンポーネント移動: DrawControlPanel, DrawModeSelector, DrawModeIcons + CSS - Hooks 移動: useUndoable, useVertexEditing - ユーティリティ移動: geojson-helpers, csv-helpers, clamp-position + テスト - CSS変数を --de-* プレフィックスで名前空間化しフォールバック付与 - types.ts で DrawMode/PathMode を一元管理し循環依存を解消 - barrel export (index.ts) で公開API を整理 - MapView.tsx を約130行に簡素化（アプリ固有ロジックのみ残存）
📝 WalkthroughWalkthroughMapViewの描画・編集ロジックを新しいuseDrawingEngineフックへ移譲し、頂点編集、undo/redo、CSV/GeoJSONインポート、ラバーバンド選択、コンテキストメニュー等の状態・レイヤ管理をdrawing-engineに集約。CSSトークンと型定義も整理。 Changes
sequenceDiagram
participant User
participant MapView
participant Engine as Drawing Engine Hook
participant Map as MapLibre
participant UI as DrawControlPanel / GeoJSONPanel
User->>MapView: マップ初期化 (initialFeatures)
MapView->>Engine: useDrawingEngine(map, { initialFeatures })
Engine->>Map: ソース/レイヤを作成・登録
Engine-->>MapView: engine オブジェクト返却
User->>UI: モード選択・ボタンクリック
UI-->>MapView: controlPanelProps 経由でアクション発行
MapView->>Engine: engine.setDrawMode / engine.* アクション
User->>Map: クリック/ドラッグ/右クリック
Map-->>Engine: map イベント伝播
Engine->>Engine: features / draft / selection 更新
Engine->>Map: feature source 同期
Engine-->>MapView: 更新された features, rubberBand, contextMenuEvent を通知
User->>MapView: Ctrl+Z / Delete / コンテキスト操作
MapView->>Engine: engine.undo() / engine.deleteSelectedVertex() / engine.closeContextMenu()
Engine->>Map: state 同期更新
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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: 1
🧹 Nitpick comments (6)
src/drawing-engine/hooks/useDrawingEngine.ts (2)
349-357:forEachのコールバックがSet.add()の戻り値を暗黙的に返しています。Biome の
useIterableCallbackReturnルールが指摘する通り、forEachコールバックは値を返すべきではありません。Set.add()はSet自身を返すため、意図しない戻り値が生じています。実害はありませんがリンターを満たすためにブロック文にできます。♻️ 修正案
const next = new Set(prev) - ids.forEach((id) => next.add(id)) + for (const id of ids) next.add(id) return next🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/hooks/useDrawingEngine.ts` around lines 349 - 357, The forEach callback in the setSelectedFeatureIds update implicitly returns the result of Set.add(), which violates the useIterableCallbackReturn rule; update the callback used on ids inside the shiftKey branch (where ids is a Set and next is a new Set(prev)) to use a block-bodied callback or a for..of loop so it does not return a value (e.g., replace ids.forEach((id) => next.add(id)) with a block callback like ids.forEach((id) => { next.add(id) }) or iterate with for (const id of ids) next.add(id) within the setSelectedFeatureIds handler).
71-72:emptyFCをモジュールレベルの定数に昇格させることを検討してください。
emptyFCはレンダーごとに新しいオブジェクトが生成されます。useEffect内でsetDataに渡される際にも毎回新しい参照が使われます。モジュールスコープの定数にすれば不要なオブジェクト生成を回避できます。♻️ 提案
+const EMPTY_FC: GeoJSON.FeatureCollection = { type: 'FeatureCollection', features: [] } + export function useDrawingEngine( map: maplibregl.Map | null, options?: DrawingEngineOptions, ): DrawingEngineResult { - const emptyFC: GeoJSON.FeatureCollection = { type: 'FeatureCollection', features: [] } - const initialFC = options?.initialFeatures ?? emptyFC + const initialFC = options?.initialFeatures ?? EMPTY_FCフック内で
emptyFCを参照している箇所もEMPTY_FCに置き換えてください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/hooks/useDrawingEngine.ts` around lines 71 - 72, emptyFC is recreated on every render causing new references to be passed into initialFC and any setData calls; lift it to a module-level constant (e.g. export const EMPTY_FC = { type: 'FeatureCollection', features: [] } as GeoJSON.FeatureCollection) and replace all local uses of emptyFC and the fallback initialFC with EMPTY_FC (update the declaration that builds initialFC and any places that pass emptyFC into setData or other functions so they use the shared EMPTY_FC reference).src/components/MapView.tsx (2)
55-59:navigator.clipboard.writeTextの Promise rejection ハンドリングが不完全です。Line 57-59 で
.catchはありますが、HTTP 以外のコンテキスト（例:file://）や Permissions API で拒否された場合、writeText自体がundefinedになる可能性があります。既存コードではありますが、独立モジュール化を機にフォールバック（document.execCommand('copy')等）の追加を検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MapView.tsx` around lines 55 - 59, navigator.clipboard.writeText may be unavailable or undefined in some contexts (e.g., file:// or denied by Permissions API); update the MapView copy flow to guard for navigator.clipboard and provide a fallback copy method: if navigator.clipboard?.writeText exists use it and handle its Promise, otherwise fall back to a synchronous copy via a temporary textarea + document.execCommand('copy') (or use the Permissions API to attempt/request permission first), and ensure both paths call handleCopy with success/error messages referencing the same url variable; adjust the copy callsite in MapView and keep handleCopy usage intact.
62-78:engine.featuresを依存配列に含むuseCallbackはフィーチャ変更のたびに再生成されます。
handlePanelFeatureClick（Line 72）はengine.featuresに依存しているため、フィーチャが変更されるたびにコールバックが再生成され、GeoJSONPanelの不要な再レンダーを引き起こす可能性があります。
engine.setFeaturesのように、engine 側でフィーチャ一覧を受け取るflyToFeature(featureId)のようなアクションを提供するか、engine.featuresを ref 経由で参照すればコールバックを安定化できます。パフォーマンスが問題にならない規模であれば現状で問題ありません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MapView.tsx` around lines 62 - 78, handlePanelFeatureClick re-creates on every change because it depends on engine.features; to stabilize it, remove engine.features from the dependency array and read features via a stable accessor instead—either add a method on engine (e.g., engine.getFeatureById or engine.flyToFeature(featureId)) and call that inside handlePanelFeatureClick, or store engine.features in a ref (useRef) and read ref.current inside the callback; ensure the dependency array only contains stable references (map and engine.setSelectedFeatureIds or engine itself if stable) and keep using getFeatureCenter and engine.setSelectedFeatureIds as before.src/drawing-engine/components/DrawControlPanel.tsx (1)
128-130: GeoJSON インポート失敗時のユーザーフィードバックが不足しています。
console.errorのみでユーザーにはエラーが表示されません。既存動作ではありますが、このコンポーネントが独立モジュールとして切り出される以上、onImportGeoJSONと同様にエラーコールバック（例:onError）を props に追加し、ホストアプリ側でトースト等を表示できるようにすることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/components/DrawControlPanel.tsx` around lines 128 - 130, The catch block in DrawControlPanel.tsx only calls console.error and provides no user-facing feedback; add an onError prop to the DrawControlPanel component (update the component's props/interface and default to a no-op) and invoke onError(err, 'Failed to import GeoJSON') inside the catch alongside the existing console.error so host apps can show toasts or handle errors; ensure the prop is documented and used similarly to onImportGeoJSON (pass the actual error object and a concise message).src/drawing-engine/index.ts (1)
1-31: Barrel export の構成は明確で、セクション分けも適切です。将来の npm パッケージ化を見据えた公開 API サーフェスとして整理されています。
一点検討事項として、
useUndoable・useVertexEditingやnextFeatureId・closePolygonRing・clampPositionなどの内部ヘルパーも公開されています。npm パッケージ化の際、意図しない破壊的変更を避けるため、コンシューマ向け API と内部ユーティリティを分離する（例: サブパスdrawing-engine/internalsからの export）ことを将来的に検討してもよいかもしれません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/index.ts` around lines 1 - 31, The barrel currently exposes internal helpers that should not be part of the public API; move the internal exports (useUndoable, useVertexEditing, nextFeatureId, closePolygonRing, clampPosition and similar helpers) out of the main index and into a dedicated internal barrel (e.g., an "internals" export file or subpath like drawing-engine/internals), then update src/drawing-engine/index.ts to only export the public components/hooks/types (DrawControlPanel, DrawModeSelector, useDrawingEngine, createPointFeature/createPathFeature/createDraftFeatureCollection/parseGeoJSONImport, parseCSV, and types) so consumers only get the intended public surface while internal utilities remain available via the new internal entry point.
🤖 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/drawing-engine/components/DrawControlPanel.css`:
- Around line 14-17: Remove the empty line(s) that precede CSS declarations so
Stylelint's declaration-empty-line-before rule is satisfied: delete the blank
line between the -webkit-backdrop-filter declaration and the border declaration
(the blank before the border rule), and likewise remove the blank line that
appears immediately before the transition declaration; this keeps the
backdrop-filter, -webkit-backdrop-filter, border, and transition declarations
contiguous.
---
Nitpick comments:
In `@src/components/MapView.tsx`:
- Around line 55-59: navigator.clipboard.writeText may be unavailable or
undefined in some contexts (e.g., file:// or denied by Permissions API); update
the MapView copy flow to guard for navigator.clipboard and provide a fallback
copy method: if navigator.clipboard?.writeText exists use it and handle its
Promise, otherwise fall back to a synchronous copy via a temporary textarea +
document.execCommand('copy') (or use the Permissions API to attempt/request
permission first), and ensure both paths call handleCopy with success/error
messages referencing the same url variable; adjust the copy callsite in MapView
and keep handleCopy usage intact.
- Around line 62-78: handlePanelFeatureClick re-creates on every change because
it depends on engine.features; to stabilize it, remove engine.features from the
dependency array and read features via a stable accessor instead—either add a
method on engine (e.g., engine.getFeatureById or engine.flyToFeature(featureId))
and call that inside handlePanelFeatureClick, or store engine.features in a ref
(useRef) and read ref.current inside the callback; ensure the dependency array
only contains stable references (map and engine.setSelectedFeatureIds or engine
itself if stable) and keep using getFeatureCenter and
engine.setSelectedFeatureIds as before.
In `@src/drawing-engine/components/DrawControlPanel.tsx`:
- Around line 128-130: The catch block in DrawControlPanel.tsx only calls
console.error and provides no user-facing feedback; add an onError prop to the
DrawControlPanel component (update the component's props/interface and default
to a no-op) and invoke onError(err, 'Failed to import GeoJSON') inside the catch
alongside the existing console.error so host apps can show toasts or handle
errors; ensure the prop is documented and used similarly to onImportGeoJSON
(pass the actual error object and a concise message).
In `@src/drawing-engine/hooks/useDrawingEngine.ts`:
- Around line 349-357: The forEach callback in the setSelectedFeatureIds update
implicitly returns the result of Set.add(), which violates the
useIterableCallbackReturn rule; update the callback used on ids inside the
shiftKey branch (where ids is a Set and next is a new Set(prev)) to use a
block-bodied callback or a for..of loop so it does not return a value (e.g.,
replace ids.forEach((id) => next.add(id)) with a block callback like
ids.forEach((id) => { next.add(id) }) or iterate with for (const id of ids)
next.add(id) within the setSelectedFeatureIds handler).
- Around line 71-72: emptyFC is recreated on every render causing new references
to be passed into initialFC and any setData calls; lift it to a module-level
constant (e.g. export const EMPTY_FC = { type: 'FeatureCollection', features: []
} as GeoJSON.FeatureCollection) and replace all local uses of emptyFC and the
fallback initialFC with EMPTY_FC (update the declaration that builds initialFC
and any places that pass emptyFC into setData or other functions so they use the
shared EMPTY_FC reference).
In `@src/drawing-engine/index.ts`:
- Around line 1-31: The barrel currently exposes internal helpers that should
not be part of the public API; move the internal exports (useUndoable,
useVertexEditing, nextFeatureId, closePolygonRing, clampPosition and similar
helpers) out of the main index and into a dedicated internal barrel (e.g., an
"internals" export file or subpath like drawing-engine/internals), then update
src/drawing-engine/index.ts to only export the public components/hooks/types
(DrawControlPanel, DrawModeSelector, useDrawingEngine,
createPointFeature/createPathFeature/createDraftFeatureCollection/parseGeoJSONImport,
parseCSV, and types) so consumers only get the intended public surface while
internal utilities remain available via the new internal entry point.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/components/MapView.tsxsrc/drawing-engine/components/DrawControlPanel.csssrc/drawing-engine/components/DrawControlPanel.tsxsrc/drawing-engine/components/DrawModeIcons.tsxsrc/drawing-engine/components/DrawModeSelector.csssrc/drawing-engine/components/DrawModeSelector.tsxsrc/drawing-engine/drawing-engine.csssrc/drawing-engine/hooks/useDrawingEngine.tssrc/drawing-engine/hooks/useUndoable.tssrc/drawing-engine/hooks/useVertexEditing.tssrc/drawing-engine/index.tssrc/drawing-engine/lib/__tests__/clamp-position.test.tssrc/drawing-engine/lib/__tests__/csv-helpers.test.tssrc/drawing-engine/lib/__tests__/geojson-helpers.test.tssrc/drawing-engine/lib/__tests__/useUndoable.test.tssrc/drawing-engine/lib/clamp-position.tssrc/drawing-engine/lib/csv-helpers.tssrc/drawing-engine/lib/geojson-helpers.tssrc/drawing-engine/types.tsvite.config.js
Comment on lines
+14
to
+17
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | ||
| -webkit-backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | ||
|
|
||
| border: 1px solid var(--glass-border); | ||
| border: 1px solid var(--de-glass-border, rgba(255, 255, 255, 0.45)); |
There was a problem hiding this comment.
Stylelint: 宣言前の空行を削除してください。
Stylelint (declaration-empty-line-before) が Line 17 の border 宣言の前の空行をエラーとして検出しています。Line 31 の transition 宣言前にも同様のエラーがあります。
🔧 修正案
Line 16 の空行を削除:
backdrop-filter: blur(var(--de-glass-blur-strong, 24px));
-webkit-backdrop-filter: blur(var(--de-glass-blur-strong, 24px));
-
border: 1px solid var(--de-glass-border, rgba(255, 255, 255, 0.45));Line 30 付近の空行も同様に削除してください（Line 31 の transition 宣言前）。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| -webkit-backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| border: 1px solid var(--glass-border); | |
| border: 1px solid var(--de-glass-border, rgba(255, 255, 255, 0.45)); | |
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| -webkit-backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| border: 1px solid var(--de-glass-border, rgba(255, 255, 255, 0.45)); |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 17-17: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/drawing-engine/components/DrawControlPanel.css` around lines 14 - 17,
Remove the empty line(s) that precede CSS declarations so Stylelint's
declaration-empty-line-before rule is satisfied: delete the blank line between
the -webkit-backdrop-filter declaration and the border declaration (the blank
before the border rule), and likewise remove the blank line that appears
immediately before the transition declaration; this keeps the backdrop-filter,
-webkit-backdrop-filter, border, and transition declarations contiguous.
- MapView.tsx: drawing-engineアーキテクチャ維持しつつHelpSidebar・GeoJSONPanelへのimport/share propsを取り込み - DrawControlPanel.css: 不要になったシェア・インポート関連CSSを削除 - useDrawingEngine.ts: controlPanelPropsからimport/share関連を除外し型を整理 - GeoJSONPanel.tsx: parseGeoJSONImportのインポートパスをdrawing-engineに修正
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/drawing-engine/components/DrawControlPanel.css (1)
14-31:⚠️ Potential issue | 🟡 MinorStylelintエラー解消のため、宣言前の空行を削除してください。
Line 17 と Line 31 の直前にある空行が
declaration-empty-line-before違反です。🔧 修正例
backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); -webkit-backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); - border: 1px solid var(--de-glass-border, rgba(255, 255, 255, 0.45)); @@ z-index: 2; user-select: none; - transition: box-shadow var(--de-transition-slow, 0.4s cubic-bezier(0.4, 0, 0.2, 1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/components/DrawControlPanel.css` around lines 14 - 31, Remove the empty lines that appear immediately before the CSS declarations causing the declaration-empty-line-before rule to fail: delete the blank line before the "border: 1px solid var(--de-glass-border...)" declaration and the blank line before the "transition: box-shadow var(--de-transition-slow...)" declaration in DrawControlPanel.css so there are no blank lines between adjacent property declarations.
🤖 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/drawing-engine/hooks/useDrawingEngine.ts`:
- Around line 427-467: After deleting, resetting or replacing features you must
also close the context menu; update deleteSelectedFeatures, resetGeoJSON, and
the 'replace' branch inside importGeoJSON to clear the context menu state (call
the same state updater used for contextMenu, e.g. setContextMenu(null) or
equivalent) immediately after the feature/state changes and any highlightTimer
cleanup so the menu cannot remain open for features that no longer exist.
- Around line 350-354: In setSelectedFeatureIds inside useDrawingEngine.ts
update the shiftKey branch so the forEach callback does not implicitly return a
value; change the ids.forEach((id) => next.add(id)) usage to a block callback
that calls next.add(id) without returning (e.g., ids.forEach((id) => {
next.add(id); }) ) or use a for...of loop over ids to add each id to next,
keeping the created Set named next and returning it from the updater.
---
Duplicate comments:
In `@src/drawing-engine/components/DrawControlPanel.css`:
- Around line 14-31: Remove the empty lines that appear immediately before the
CSS declarations causing the declaration-empty-line-before rule to fail: delete
the blank line before the "border: 1px solid var(--de-glass-border...)"
declaration and the blank line before the "transition: box-shadow
var(--de-transition-slow...)" declaration in DrawControlPanel.css so there are
no blank lines between adjacent property declarations.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/GeoJSONPanel.tsxsrc/components/MapView.tsxsrc/drawing-engine/components/DrawControlPanel.csssrc/drawing-engine/components/DrawControlPanel.tsxsrc/drawing-engine/hooks/useDrawingEngine.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/drawing-engine/components/DrawControlPanel.tsx
Comment on lines
+350
to
+354
| setSelectedFeatureIds((prev) => { | ||
| if (e.originalEvent.shiftKey) { | ||
| const next = new Set(prev) | ||
| ids.forEach((id) => next.add(id)) | ||
| return next |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files src/drawing-engine/hooks/useDrawingEngine.tsRepository: geolonia/geojson-maker
Length of output: 110
🏁 Script executed:
wc -l src/drawing-engine/hooks/useDrawingEngine.tsRepository: geolonia/geojson-maker
Length of output: 114
🏁 Script executed:
sed -n '345,360p' src/drawing-engine/hooks/useDrawingEngine.tsRepository: geolonia/geojson-maker
Length of output: 744
🏁 Script executed:
find . -name "biome.json" -o -name "biome.jsonc" -o -name ".biomeignore" | head -20Repository: geolonia/geojson-maker
Length of output: 78
🏁 Script executed:
cat biome.jsonRepository: geolonia/geojson-maker
Length of output: 1840
🏁 Script executed:
cd src/drawing-engine/hooks && git diff HEAD useDrawingEngine.ts | head -100Repository: geolonia/geojson-maker
Length of output: 48
🏁 Script executed:
git log --oneline -1 src/drawing-engine/hooks/useDrawingEngine.tsRepository: geolonia/geojson-maker
Length of output: 117
🏁 Script executed:
rg -n "forEach\s*\(\s*\(" src/drawing-engine/hooks/useDrawingEngine.tsRepository: geolonia/geojson-maker
Length of output: 214
🏁 Script executed:
# Check if there are other similar patterns in the codebase
rg -n "\.forEach\(\([^)]+\)\s*=>\s*[^{}]" --type ts --type tsx | head -20Repository: geolonia/geojson-maker
Length of output: 93
🌐 Web query:
Biome useIterableCallbackReturn rule forEach implicit return
💡 Result:
Biome’s lint/suspicious/useIterableCallbackReturn enforces that iterable callbacks either always return a value (for methods like map, filter, find, etc.) or never return a value—and forEach is in the “must not return a value” category. If your forEach callback has an implicit return (e.g. an expression-bodied arrow), the rule can flag it as “should not return a value.” [1]
Fixes for forEach “implicit return”
Prefer a block body (no returned expression):
arr.forEach((x) => { doSideEffect(x); });Or explicitly void the expression (allowed by the rule):
arr.forEach((x) => void doSideEffect(x));(void return values don’t trigger the rule.) [1]
If you want Biome to stop checking forEach returns
Biome added an option in v2.4.0:
{
"linter": {
"rules": {
"suspicious": {
"useIterableCallbackReturn": {
"options": { "checkForEach": false }
}
}
}
}
}This skips forEach() callbacks that return a value. [1][2]
Sources: Biome rule docs and option checkForEach [1], Biome v2.4.0 release notes mentioning checkForEach support [2].
[1] (next.biomejs.dev)
[2] (newreleases.io)
Citations:
- 1: https://next.biomejs.dev/uk/linter/rules/use-iterable-callback-return/
- 2: https://newreleases.io/project/github/biomejs/biome/release/%40biomejs%2Fbiome%402.4.0
forEach コールバックで値を返さない形にしてください。
Line 353 の ids.forEach((id) => next.add(id)) は next.add(id) の戻り値を暗黙返却するため、Biome ルール useIterableCallbackReturn に違反します。
修正例
- ids.forEach((id) => next.add(id))
+ ids.forEach((id) => {
+ next.add(id)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setSelectedFeatureIds((prev) => { | |
| if (e.originalEvent.shiftKey) { | |
| const next = new Set(prev) | |
| ids.forEach((id) => next.add(id)) | |
| return next | |
| setSelectedFeatureIds((prev) => { | |
| if (e.originalEvent.shiftKey) { | |
| const next = new Set(prev) | |
| ids.forEach((id) => { | |
| next.add(id) | |
| }) | |
| return next |
🧰 Tools
🪛 Biome (2.4.4)
[error] 353-353: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/drawing-engine/hooks/useDrawingEngine.ts` around lines 350 - 354, In
setSelectedFeatureIds inside useDrawingEngine.ts update the shiftKey branch so
the forEach callback does not implicitly return a value; change the
ids.forEach((id) => next.add(id)) usage to a block callback that calls
next.add(id) without returning (e.g., ids.forEach((id) => { next.add(id); }) )
or use a for...of loop over ids to add each id to next, keeping the created Set
named next and returning it from the updater.
Comment on lines
+427
to
+467
| const deleteSelectedFeatures = useCallback(() => { | ||
| if (selectedFeatureIds.size === 0) return | ||
| setFeatures((prev) => ({ | ||
| ...prev, | ||
| features: prev.features.filter((f) => !selectedFeatureIds.has(f.properties?._id as string)), | ||
| })) | ||
| setSelectedFeatureIds(new Set()) | ||
| }, [selectedFeatureIds]) | ||
|
|
||
| const resetGeoJSON = useCallback(() => { | ||
| if (highlightTimerRef.current) { | ||
| clearTimeout(highlightTimerRef.current) | ||
| highlightTimerRef.current = null | ||
| } | ||
| setFeatures({ type: 'FeatureCollection', features: [] }) | ||
| setDraftCoords([]) | ||
| setSelectedFeatureIds(new Set()) | ||
| setHighlightedPanelFeatureId(null) | ||
| }, []) | ||
|
|
||
| const importCSV = useCallback((text: string) => { | ||
| const rows = parseCSV(text) | ||
| const newFeatures: GeoJSON.Feature[] = rows.map((row) => ({ | ||
| type: 'Feature', | ||
| geometry: { type: 'Point', coordinates: [row.lng, row.lat] }, | ||
| properties: { _id: nextFeatureId(), drawMode: 'point', ...row.properties }, | ||
| })) | ||
| setFeatures((prev) => ({ ...prev, features: [...prev.features, ...newFeatures] })) | ||
| }, []) | ||
|
|
||
| const importGeoJSON = useCallback((importedFeatures: GeoJSON.Feature[], mode: 'replace' | 'merge') => { | ||
| if (mode === 'replace') { | ||
| if (highlightTimerRef.current) { | ||
| clearTimeout(highlightTimerRef.current) | ||
| highlightTimerRef.current = null | ||
| } | ||
| setFeatures({ type: 'FeatureCollection', features: importedFeatures }) | ||
| setDraftCoords([]) | ||
| setSelectedFeatureIds(new Set()) | ||
| setHighlightedPanelFeatureId(null) | ||
| } else { |
There was a problem hiding this comment.
削除/リセット/置換時にコンテキストメニューも閉じてください。
Line 427-467 の処理後、contextMenu が残ると削除済みフィーチャのメニューが表示され続ける可能性があります。
🔧 修正例
const deleteSelectedFeatures = useCallback(() => {
if (selectedFeatureIds.size === 0) return
setFeatures((prev) => ({
...prev,
features: prev.features.filter((f) => !selectedFeatureIds.has(f.properties?._id as string)),
}))
setSelectedFeatureIds(new Set())
+ setContextMenu(null)
}, [selectedFeatureIds])
const resetGeoJSON = useCallback(() => {
@@
setFeatures({ type: 'FeatureCollection', features: [] })
setDraftCoords([])
setSelectedFeatureIds(new Set())
setHighlightedPanelFeatureId(null)
+ setContextMenu(null)
}, [])
const importGeoJSON = useCallback((importedFeatures: GeoJSON.Feature[], mode: 'replace' | 'merge') => {
if (mode === 'replace') {
@@
setFeatures({ type: 'FeatureCollection', features: importedFeatures })
setDraftCoords([])
setSelectedFeatureIds(new Set())
setHighlightedPanelFeatureId(null)
+ setContextMenu(null)
} else {
setFeatures((prev) => ({ ...prev, features: [...prev.features, ...importedFeatures] }))
}
}, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/drawing-engine/hooks/useDrawingEngine.ts` around lines 427 - 467, After
deleting, resetting or replacing features you must also close the context menu;
update deleteSelectedFeatures, resetGeoJSON, and the 'replace' branch inside
importGeoJSON to clear the context menu state (call the same state updater used
for contextMenu, e.g. setContextMenu(null) or equivalent) immediately after the
feature/state changes and any highlightTimer cleanup so the menu cannot remain
open for features that no longer exist.
ポリゴンやラインの特定の頂点を削除する機能を実装: - 頂点を右クリック → コンテキストメニュー「この頂点を削除」 - 頂点をクリック選択（赤くハイライト） → Delete/Backspaceキーで削除 - 最小頂点数チェック（LineString: 2, Polygon: 3）で削除不可の場合はdisabled表示 - ポリゴンのリング閉じ処理を維持 - Undo/Redo対応（既存のonCommitパターンを利用） 新規ファイル: - vertex-helpers.ts: canDeleteVertex / applyVertexDelete ユーティリティ - VertexContextMenu コンポーネント（glassmorphismスタイル） - E2Eテスト: 5シナリオ（右クリック削除、最小頂点数無効、ポリゴン削除、Deleteキー、Undo復元） - ユニットテスト: 12テストケース
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/drawing-engine/hooks/useVertexEditing.ts (1)
72-80:⚠️ Potential issue | 🟠 MajorPolygon頂点移動で内周リング（holes）が消えます。
Line 79 が
coordinates: [ring]固定のため、外周以外のリングが失われます。移動操作でデータ欠損が起きるので保持が必要です。修正案
if (geom.type === 'Polygon') { - const ring = geom.coordinates[0].map((c, i) => { + const rings = geom.coordinates + const ring = rings[0].map((c, i) => { if (i === vertexIndex) return newCoord // 頂点 0 を動かす場合、閉じる点も更新 - if (vertexIndex === 0 && i === geom.coordinates[0].length - 1) return newCoord + if (vertexIndex === 0 && i === rings[0].length - 1) return newCoord return c }) as [number, number][] - return { ...feature, geometry: { ...geom, coordinates: [ring] } } + return { ...feature, geometry: { ...geom, coordinates: [ring, ...rings.slice(1)] } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/hooks/useVertexEditing.ts` around lines 72 - 80, The code that handles Polygon vertex moves replaces geometry.coordinates with only [ring], which drops any interior rings/holes; update the return to preserve all existing rings by replacing only the outer ring: compute ring as you already do and then set geometry.coordinates to [ring, ...geom.coordinates.slice(1)] (or otherwise copy/merge the remaining rings) so holes are retained; keep the existing logic for updating the closing point when vertexIndex === 0 and still return { ...feature, geometry: { ...geom, coordinates: [ring, ...geom.coordinates.slice(1)] } }.
🧹 Nitpick comments (1)
src/drawing-engine/lib/__tests__/vertex-helpers.test.ts (1)
92-128: Polygonの内周リング（holes）保持ケースも追加してください。現行のPolygonテストは外周リングのみで、内周リング消失の回帰を検知できません。削除後に
coordinates.slice(1)が維持される検証を1件追加するのが安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drawing-engine/lib/__tests__/vertex-helpers.test.ts` around lines 92 - 128, Add a test that verifies polygon inner rings (holes) are preserved after vertex deletion: call applyVertexDelete on a Feature with both an outer ring and at least one inner ring, perform a deletion (e.g., delete an outer or inner vertex) and assert that the coordinates array still contains the original inner rings (e.g., coordinates.slice(1) equals the prior inner ring(s)) and that each ring remains properly closed; reference applyVertexDelete and the geometry.coordinates structure when locating where to add the assertion.
🤖 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/drawing-engine/components/VertexContextMenu.css`:
- Around line 7-12: Remove the empty line that precedes the background
declaration so the `background:` property is immediately followed by its
`linear-gradient(...)` value (and ensure `backdrop-filter:` remains directly
after the background block); this fixes the `declaration-empty-line-before`
Stylelint rule violation for the `background` declaration in
VertexContextMenu.css.
In `@src/drawing-engine/lib/vertex-helpers.ts`:
- Around line 28-33: The current Polygon edit logic replaces
geometry.coordinates with only the edited outer ring (closed), dropping any
inner rings (holes); update the return to preserve existing holes by setting
geometry.coordinates to an array combining the edited outer ring and the
remaining rings (e.g. [closed, ...geom.coordinates.slice(1)]), ensuring you
reference the same feature and geom objects and still handle cases with no holes
safely when computing ring/newRing/closed based on vertexIndex.
---
Outside diff comments:
In `@src/drawing-engine/hooks/useVertexEditing.ts`:
- Around line 72-80: The code that handles Polygon vertex moves replaces
geometry.coordinates with only [ring], which drops any interior rings/holes;
update the return to preserve all existing rings by replacing only the outer
ring: compute ring as you already do and then set geometry.coordinates to [ring,
...geom.coordinates.slice(1)] (or otherwise copy/merge the remaining rings) so
holes are retained; keep the existing logic for updating the closing point when
vertexIndex === 0 and still return { ...feature, geometry: { ...geom,
coordinates: [ring, ...geom.coordinates.slice(1)] } }.
---
Nitpick comments:
In `@src/drawing-engine/lib/__tests__/vertex-helpers.test.ts`:
- Around line 92-128: Add a test that verifies polygon inner rings (holes) are
preserved after vertex deletion: call applyVertexDelete on a Feature with both
an outer ring and at least one inner ring, perform a deletion (e.g., delete an
outer or inner vertex) and assert that the coordinates array still contains the
original inner rings (e.g., coordinates.slice(1) equals the prior inner ring(s))
and that each ring remains properly closed; reference applyVertexDelete and the
geometry.coordinates structure when locating where to add the assertion.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
e2e/features/vertex-deletion.featuree2e/step-definitions/vertex-deletion-steps.tse2e/support/helpers/selectors.tssrc/components/MapView.tsxsrc/drawing-engine/components/VertexContextMenu.csssrc/drawing-engine/components/VertexContextMenu.tsxsrc/drawing-engine/hooks/useDrawingEngine.tssrc/drawing-engine/hooks/useVertexEditing.tssrc/drawing-engine/index.tssrc/drawing-engine/lib/__tests__/vertex-helpers.test.tssrc/drawing-engine/lib/vertex-helpers.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/features/vertex-deletion.feature
Comment on lines
+7
to
+12
|
|
||
| background: | ||
| linear-gradient(160deg, | ||
| rgba(255, 255, 255, 0.92) 0%, | ||
| rgba(237, 233, 254, 0.8) 100%); | ||
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); |
There was a problem hiding this comment.
Stylelint違反（空行）を解消してください。
Line 8 の宣言前空行で declaration-empty-line-before に違反しています。
修正案
.vertex-context-menu {
position: fixed;
z-index: 100;
min-width: 160px;
padding: 6px;
border-radius: 10px;
-
background:
linear-gradient(160deg,
rgba(255, 255, 255, 0.92) 0%,
rgba(237, 233, 254, 0.8) 100%);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| background: | |
| linear-gradient(160deg, | |
| rgba(255, 255, 255, 0.92) 0%, | |
| rgba(237, 233, 254, 0.8) 100%); | |
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| .vertex-context-menu { | |
| position: fixed; | |
| z-index: 100; | |
| min-width: 160px; | |
| padding: 6px; | |
| border-radius: 10px; | |
| background: | |
| linear-gradient(160deg, | |
| rgba(255, 255, 255, 0.92) 0%, | |
| rgba(237, 233, 254, 0.8) 100%); | |
| backdrop-filter: blur(var(--de-glass-blur-strong, 24px)); | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 8-11: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/drawing-engine/components/VertexContextMenu.css` around lines 7 - 12,
Remove the empty line that precedes the background declaration so the
`background:` property is immediately followed by its `linear-gradient(...)`
value (and ensure `backdrop-filter:` remains directly after the background
block); this fixes the `declaration-empty-line-before` Stylelint rule violation
for the `background` declaration in VertexContextMenu.css.
Comment on lines
+28
to
+33
| if (geom.type === 'Polygon') { | ||
| const ring = geom.coordinates[0].slice(0, -1) // 閉じ点を除去 | ||
| const newRing = ring.filter((_, i) => i !== vertexIndex) | ||
| // ポリゴンリングを閉じる（先頭 = 末尾） | ||
| const closed = [...newRing, newRing[0]] | ||
| return { ...feature, geometry: { ...geom, coordinates: [closed] } } |
There was a problem hiding this comment.
Polygon編集時に内周リング（holes）が消失します。
Line 33 で coordinates: [closed] を返しており、既存の内周リングが破棄されます。Polygonデータを壊すため修正が必要です。
修正案
if (geom.type === 'Polygon') {
- const ring = geom.coordinates[0].slice(0, -1) // 閉じ点を除去
+ const rings = geom.coordinates
+ const ring = rings[0].slice(0, -1) // 閉じ点を除去
const newRing = ring.filter((_, i) => i !== vertexIndex)
// ポリゴンリングを閉じる（先頭 = 末尾）
const closed = [...newRing, newRing[0]]
- return { ...feature, geometry: { ...geom, coordinates: [closed] } }
+ return { ...feature, geometry: { ...geom, coordinates: [closed, ...rings.slice(1)] } }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (geom.type === 'Polygon') { | |
| const ring = geom.coordinates[0].slice(0, -1) // 閉じ点を除去 | |
| const newRing = ring.filter((_, i) => i !== vertexIndex) | |
| // ポリゴンリングを閉じる（先頭 = 末尾） | |
| const closed = [...newRing, newRing[0]] | |
| return { ...feature, geometry: { ...geom, coordinates: [closed] } } | |
| if (geom.type === 'Polygon') { | |
| const rings = geom.coordinates | |
| const ring = rings[0].slice(0, -1) // 閉じ点を除去 | |
| const newRing = ring.filter((_, i) => i !== vertexIndex) | |
| // ポリゴンリングを閉じる（先頭 = 末尾） | |
| const closed = [...newRing, newRing[0]] | |
| return { ...feature, geometry: { ...geom, coordinates: [closed, ...rings.slice(1)] } } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/drawing-engine/lib/vertex-helpers.ts` around lines 28 - 33, The current
Polygon edit logic replaces geometry.coordinates with only the edited outer ring
(closed), dropping any inner rings (holes); update the return to preserve
existing holes by setting geometry.coordinates to an array combining the edited
outer ring and the remaining rings (e.g. [closed,
...geom.coordinates.slice(1)]), ensuring you reference the same feature and geom
objects and still handle cases with no holes safely when computing
ring/newRing/closed based on vertexIndex.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sugama-satsuki
changed the title
Collaborator
Author
E2E テスト結果頂点削除テスト (5シナリオ追加)
ユニットテスト結果 |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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
src/drawing-engine/に分離し、npmパッケージ化可能な構成に整理useDrawingEngineフックで描画エンジンの全機能を一括提供（モード切替、Undo/Redo、頂点編集、インポートなど）影響範囲
src/components/MapView.tsx:useDrawingEngineフックを利用するように大幅簡素化src/drawing-engine/: 新規ディレクトリ（hooks, components, lib, types を格納）vertex-helpers.test.ts12 テストケース追加変更ファイル一覧
新規作成
src/drawing-engine/ディレクトリ一式（hooks, components, lib, types, index.ts, CSS）src/drawing-engine/lib/vertex-helpers.ts—canDeleteVertex/applyVertexDeletesrc/drawing-engine/components/VertexContextMenu.tsx+ CSSe2e/features/vertex-deletion.feature— 5 シナリオe2e/step-definitions/vertex-deletion-steps.ts変更
src/components/MapView.tsx— drawing-engine フック利用に簡素化 + VertexContextMenu 接続e2e/support/helpers/selectors.ts— 頂点コンテキストメニューセレクタ追加Test plan
npm test— ユニットテスト 98 件全パスnpm run test:e2e— E2E テスト 57 シナリオ / 366 ステップ全パスnpm run build— ビルド成功