added an endpoint without side effect#463
Merged
Merged
Conversation
📝 Walkthroughウォークスルーこのプルリクエストは、ウェブパック設定を2つの独立したエントリーポイント（embed.ts と embed-core.ts）をサポートする構造に再構築し、TypeScript型定義を共有型モジュールに集約し、SimpleStyleクラスにイベントハンドラー追跡と削除機能を追加し、package.jsonにエクスポートフィールドを導入し、E2Eテストを更新します。 変更内容
推定コードレビュー工数🎯 3 (中程度) | ⏱️ ~25 分 詩
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/webpack.config.js`:
- Around line 7-9: The output override spreads embedConfig.output
(embedConfig.output) which carries clean: true and then sets path: __dirname
(docs/), causing the docs directory to be wiped on build; explicitly override
the inherited clean flag by setting clean: false in the output object where you
spread embedConfig.output (the same block that sets path: __dirname and
filename: 'embed') so that clean: false replaces the inherited clean: true and
prevents deletion of docs/.
In `@e2e/external-style.spec.ts`:
- Around line 60-63: The code inside page.evaluate assumes
document.querySelector('#map canvas.maplibregl-canvas') returns a non-null
HTMLCanvasElement and calls getContext directly; add a null guard for the canvas
variable (inside the page.evaluate callback) and return false early if canvas is
null so getContext isn't invoked on null; update the canvas declaration and
control flow around canvas/getContext in the page.evaluate block that computes
isNotBlank.
In `@src/lib/simplestyle.ts`:
- Around line 352-374: The remove() implementation currently assumes this.map is
set and will throw if called before addTo() or called twice; add a guard at the
start of remove() (e.g., if (!this.map) return) to make the method idempotent
and safe to call when uninitialized, and after successfully removing
layers/sources clear the stored map reference (set this.map = undefined/null) so
subsequent calls are no-ops; refer to the remove() method, this.map field, and
the addTo() entry point when making the change.
- Around line 352-374: The remove() method currently removes layers/sources but
does not unregister event handlers added by setPopup() and setCluster(), causing
duplicate handlers and leaks when re-adding; capture the handler functions
passed to this.map.on(...) in instance properties (e.g.,
this._popupClickHandler, this._popupEnterHandler, this._popupLeaveHandler,
this._clusterClickHandler, this._clusterEnterHandler, this._clusterLeaveHandler)
when setPopup() and setCluster() register 'click', 'mouseenter', and
'mouseleave' listeners, and then call this.map.off(event, layerOrType, handler)
for each stored handler in remove() to fully clean them up before removing
layers/sources.
In `@src/types.ts`:
- Around line 3-34: EmbedAttributes currently types every attribute as string
but code in parse-atts.ts (e.g., defaults like lat: 0, lng: 0, zoom: 0, maxZoom:
20) and util.ts (calls parseFloat(atts.lat)) use numeric defaults, causing a
type mismatch; update the EmbedAttributes type so numeric keys accept string |
number (at least for lat, lng, zoom, bearing, pitch, minZoom, maxZoom and any
other numeric-like keys) or change the defaults to strings to match the exported
type, then run type-checks to ensure parse-atts.ts and util.ts compile without
assertions — refer to the EmbedAttributes type, parse-atts.ts defaults, and
util.ts parseFloat usage when making the change.
In `@webpack.config.js`:
- Around line 47-64: The two multi-compiler configs (embedConfig and
embedCoreConfig) race because output.clean is true for embedConfig and execution
order isn't guaranteed; fix by making execution deterministic: add unique name
properties (e.g., embedCoreConfig.name = 'embed-core', embedConfig.name =
'embed') and set embedConfig.dependencies = ['embed-core'] so embed-core builds
first (or alternatively set both outputs to clean: false and use an explicit
cleaning step), ensuring embedCoreConfig artifacts aren't deleted by
embedConfig's clean step.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/e2e/external-style.htmldocs/webpack.config.jse2e/basic.spec.tse2e/external-style.spec.tse2e/map.spec.tspackage.jsonsrc/embed-core.tssrc/embed.tssrc/lib/parse-atts.tssrc/lib/simplestyle.tssrc/types.tswebpack.config.js
yuiseki
self-requested a review
March 5, 2026 06:10
Contributor
There was a problem hiding this comment.
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 (1)
src/lib/simplestyle.ts (1)
274-286:⚠️ Potential issue | 🟠 Majorクリーンアップ後も古い async ハンドラが
mapを更新できます
setPopup()とsetCluster()の async ハンドラで、await sanitizeDescription()/await getClusterExpansionZoom()の後に世代確認がないため、remove()直前に発火したハンドラがクリーンアップ後もPopup.addTo()やmap.easeTo()を実行できます。また、表示中の popup をremove()で閉じていません。💡 修正イメージ
export class SimpleStyle { + private _mountSeq = 0; + private _activePopup?: maplibregl.Popup; private _eventHandlers: { event: string; layer: string; handler: (...args) => void }[] = []; addTo(map) { this.map = map; + this._mountSeq += 1; const features = this.geojson.features; ... } async setPopup(map, source) { + const mountSeq = this._mountSeq; const clickHandler = async (e) => { ... if (description) { const sanitizedDescription = await sanitizeDescription(description); + if (mountSeq !== this._mountSeq || this.map !== map) return; + this._activePopup?.remove(); + this._activePopup = new maplibregl.Popup() .setLngLat(center) .setHTML(sanitizedDescription) .addTo(map); } }; } setCluster() { + const mountSeq = this._mountSeq; + const map = this.map; ... const clusterClickHandler = async (e) => { ... const zoom = await map .getSource(`${this.options.id}-points`) .getClusterExpansionZoom(clusterId); + if (mountSeq !== this._mountSeq || this.map !== map) return; map.easeTo({ center: features[0].geometry.coordinates, zoom: zoom, }); }; } remove() { if (!this.map) return this; + this._mountSeq += 1; + this._activePopup?.remove(); + this._activePopup = undefined; ... + this.map = undefined; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/simplestyle.ts` around lines 274 - 286, The async handlers (clickHandler used by setPopup() and the handler used by setCluster()) call await sanitizeDescription() / await getClusterExpansionZoom() but don't verify the map/instance hasn't changed afterward and they never close the displayed popup on cleanup; fix by capturing the current map instance or an incrementing generation/token (e.g., store currentMap or handlerGeneration before awaiting) and compare it immediately after each await to bail out if different, and ensure any created maplibregl.Popup() is kept in a variable that is removed/closed in the component cleanup (call popup.remove() and avoid calling popup.addTo(map) or map.easeTo() if the generation check fails); apply these checks around sanitizeDescription, getClusterExpansionZoom, Popup.addTo, and map.easeTo to prevent stale handlers from modifying the map.
🤖 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/lib/simplestyle.ts`:
- Around line 290-298: The mouse cursor can remain set to 'pointer' if remove()
is called before mouseleave fires; update the remove() method to explicitly
reset the canvas cursor to default by calling this.map.getCanvas().style.cursor
= '' at the end of the method, and do the same for any other cleanup paths;
ensure this change addresses both the basic feature handlers (mouseEnterHandler
/ mouseLeaveHandler) and the cluster handlers so the cursor is always cleared
when the layer/interaction is removed.
---
Outside diff comments:
In `@src/lib/simplestyle.ts`:
- Around line 274-286: The async handlers (clickHandler used by setPopup() and
the handler used by setCluster()) call await sanitizeDescription() / await
getClusterExpansionZoom() but don't verify the map/instance hasn't changed
afterward and they never close the displayed popup on cleanup; fix by capturing
the current map instance or an incrementing generation/token (e.g., store
currentMap or handlerGeneration before awaiting) and compare it immediately
after each await to bail out if different, and ensure any created
maplibregl.Popup() is kept in a variable that is removed/closed in the component
cleanup (call popup.remove() and avoid calling popup.addTo(map) or map.easeTo()
if the generation check fails); apply these checks around sanitizeDescription,
getClusterExpansionZoom, Popup.addTo, and map.easeTo to prevent stale handlers
from modifying the map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5a95967-bd11-4f40-bfac-1e8f8f449bc6
📒 Files selected for processing (2)
docs/webpack.config.jssrc/lib/simplestyle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/webpack.config.js
Contributor
Author
bougan1160
self-requested a review
March 13, 2026 01:50
This was referenced Mar 16, 2026
yuiseki
added
Priority: High
This was referenced Mar 16, 2026
Merged
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
3 participants
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.
React ラッパー向け embed 改修（Phase 1）
@geolonia/react-embed を作るための前提となる改修です。
tl;dr
変更点を示します。
embed.tsは読み込んだ瞬間に副作用が色々走るため、importして使いやすいようにはなっていません。これはreactなどで利用する際の障壁になります。embed-core.tsで副作用のない新しいエンドポイントを作りました。geolonia-map.ts,parse-atts.ts,embed.ts,types.tsはその際に型定義を整理したものになります。simplestyle.tsの変更が多いのは、部品化の際にreactなどで使えることを見越して削除機能を追加したことによります。背景
現状の embed.ts は window.geolonia の設定や DOM スキャン（renderGeoloniaMap()）などの副作用を持つため、React コンポーネントからそのまま import すると SSR
時にクラッシュしたり、マップが二重初期化されるなどの問題が起きます。React ラッパーはクラスだけを import できる副作用なしのエントリポイントを必要としています。
変更内容
src/embed-core.ts を新規追加
副作用なしのエントリポイント。renderGeoloniaMap() も window.geolonia の設定もしません。PMTiles プロトコル登録（全利用者に必要なため残す）だけを行い、GeoloniaMap・SimpleStyle
等のクラスと型をエクスポートします。@geolonia/react-embed はこちらを import します。
src/types.ts を新規追加・循環依存の解消
EmbedAttributes・EmbedPlugin 型を embed.ts からここに移動しました。これにより parse-atts.ts が embed.ts を import していた循環依存が解消されます。
SimpleStyle.remove() を追加
React の useEffect クリーンアップで必要です。addTo() で追加したレイヤーとソースをまとめて削除します。これがないとコンポーネントの再レンダリング時にレイヤーが重複してエラーになります。
useEffect(() => {
const ss = new SimpleStyle(geojson).addTo(map);
return () => ss.remove();
}, [map, geojson]);
ビルド・パッケージ設定
e2e テスト整備
動作確認
dist/embed.js # 従来どおり（後方互換性あり）
dist/embed-core.js # 新規追加
両バンドルのビルド成功・ユニットテスト 51 件・e2e テスト 24 件（Chromium / Firefox / WebKit）すべてパスを確認しています。
Summary by CodeRabbit
リリースノート
新機能
バグ修正
その他