fix: regroup IP geo details and add map panel#1032
Conversation
📝 WalkthroughWalkthrough添加基于 MapLibre 的客户端地图组件及相关样式、在 IP 详情中引入可折叠的“国家与时区”分组并在有意义坐标时展示地图，放宽坐标有效性判定，扩展多语言本地化条目，增加测试与依赖更新，并调整启动预热与健康检查对 DSN 的处理逻辑。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
![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 introduces a map visualization feature for IP details using maplibre-gl. It includes a new reusable Map component with support for markers, popups, and controls, alongside a LocationMapCard and a refactored LocationSection that organizes geographic data into permanent and collapsible sections. The PR also updates coordinate validation logic to allow non-zero coordinates without accuracy data, adds internationalization for the new UI sections, and enhances the database health check to verify the DSN environment variable. Review feedback suggests improving the Map component's synchronization logic, handling style loading more robustly to avoid unstyled content, and providing better user feedback for geolocation failures.
| </div> | ||
|
|
||
| <div className="h-52 sm:h-60"> | ||
| <Map center={[longitude, latitude]} zoom={resolveZoom(accuracyRadiusKm)}> |
There was a problem hiding this comment.
The Map component only synchronizes viewport updates when the viewport prop is used. By passing center and zoom as direct props, the map will initialize correctly but will not update if the coordinates change (e.g., if the user switches to a different IP while the dialog remains open). Using the viewport prop ensures the map stays in sync with the data.
<Map
viewport={{
center: [longitude, latitude],
zoom: resolveZoom(accuracyRadiusKm),
}}
>
| */ | ||
| export function hasLocationContent(result: IpGeoLookupResult): boolean { | ||
| const { location, timezone } = result; | ||
| if (location.country.code !== "ZZ") return true; |
There was a problem hiding this comment.
This condition causes the "Location details" section to render even when only the country is known. Since the country is already displayed in the hero card, this creates redundant information. Removing this line ensures the section only appears when there is additional geographic data (like city, region, or coordinates) to show, adhering to the logic described in the comment on line 36.
|
|
||
| function useMapLibreStyles() { | ||
| useEffect(() => { | ||
| void import("maplibre-gl/dist/maplibre-gl.css"); |
There was a problem hiding this comment.
Dynamically importing the MapLibre CSS inside a useEffect can lead to a Flash of Unstyled Content (FOUC) where the map container appears broken before the styles are applied. It is generally safer to import the CSS at the top level of the file to ensure it is available when the component mounts.
| const internalUpdateRef = useRef(false); | ||
| const resolvedTheme = useResolvedTheme(themeProp); | ||
|
|
||
| const isControlled = viewport !== undefined && onViewportChange !== undefined; |
There was a problem hiding this comment.
The current logic requires both viewport and onViewportChange to be present for the map to synchronize with the viewport prop. This prevents simple one-way data flow where a parent component just wants to move the map to a specific location. Relaxing this requirement allows the viewport prop to drive the map state independently.
| const isControlled = viewport !== undefined && onViewportChange !== undefined; | |
| const isControlled = viewport !== undefined; |
| // Delay to ensure style is fully processed before allowing layer operations | ||
| // This is a workaround to avoid race conditions with the style loading | ||
| // else we have to force update every layer on setStyle change | ||
| styleTimeoutRef.current = setTimeout(() => { |
There was a problem hiding this comment.
Using setTimeout with a fixed 100ms delay to handle style loading is brittle and can lead to race conditions. A more robust approach is to check map.isStyleLoaded() or listen for the idle event to ensure the style is fully processed before allowing layer operations.
| setWaitingForLocation(false); | ||
| }, | ||
| (error) => { | ||
| console.error("Error getting location:", error); |
There was a problem hiding this comment.
When geolocation fails, the error is only logged to the console. It would be better to provide visual feedback to the user (e.g., via a toast notification) so they understand why the "locate" action failed.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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: bbbd44559d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!dsn) { | ||
| return { status: "unchecked", message: "Database not configured" }; |
There was a problem hiding this comment.
Returning unchecked when DSN is empty makes the readiness path report the instance as healthy, because checkReadiness() only treats database.status === "down" as unhealthy. In a misconfigured deployment (missing DB connection string), /readiness will still return HTTP 200 and the pod can receive traffic even though the required database is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Suggested fix (if DB is required for readiness): don’t treat missing DSN as a healthy state.
Option A (simplest): mark DB as down when DSN is missing:
const dsn = process.env.DSN?.trim();
if (!dsn) {
return { status: "down", message: "DSN not configured" };
}Option B: keep unchecked, but make readiness fail fast:
if (database.status !== "up") {
status = "unhealthy";
}Add a unit test that asserts checkReadiness() becomes unhealthy when DSN is unset (and /api/health/ready returns HTTP 503).
| const handleLocate = useCallback(() => { | ||
| setWaitingForLocation(true); | ||
| if ("geolocation" in navigator) { | ||
| navigator.geolocation.getCurrentPosition( |
There was a problem hiding this comment.
handleLocate sets waitingForLocation to true before checking for navigator.geolocation, but only clears it inside geolocation callbacks. In environments where the API is unavailable (e.g., unsupported/insecure contexts), users who click locate will be left with a permanently disabled control and spinner.
Useful? React with 👍 / 👎.
| <ControlButton onClick={handleZoomIn} label="Zoom in"> | ||
| <Plus className="size-4" /> | ||
| </ControlButton> | ||
| <ControlButton onClick={handleZoomOut} label="Zoom out"> | ||
| <Minus className="size-4" /> |
There was a problem hiding this comment.
This file introduces user-facing control text as hardcoded English ("Zoom in", "Zoom out", etc.) rather than i18n keys, which breaks localized UX/accessibility in non-English locales and violates the repository rule in CLAUDE.md (“All user-facing strings must use i18n”). The same pattern also appears in popup close labels in this component.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Suggested fix:
import { useTranslations } from "next-intl";
const tCommon = useTranslations("common");
const tMap = useTranslations("ui.mapControls");
<ControlButton onClick={handleZoomIn} label={tMap("zoomIn")} />
<ControlButton onClick={handleZoomOut} label={tMap("zoomOut")} />
<button aria-label={tCommon("close")}>
<X className="h-4 w-4" />
<span className="sr-only">{tCommon("close")}</span>
</button>Add ui.mapControls.zoomIn|zoomOut|findMyLocation|toggleFullscreen|resetBearing|closePopup to messages/*/ui.json for all locales.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const mapTitle = cityName ?? regionName ?? countryName ?? t("hero.location"); | ||
| const mapSubtitle = [regionName, countryName].filter(Boolean).join(" · ") || null; |
There was a problem hiding this comment.
mapSubtitle is built from [regionName, countryName] without filtering values that duplicate mapTitle or each other. For a location like Hong Kong (where city, region, and country all share the same name), this produces a subtitle of "Hong Kong · Hong Kong" under a title of "Hong Kong" — fully redundant.
| const mapTitle = cityName ?? regionName ?? countryName ?? t("hero.location"); | |
| const mapSubtitle = [regionName, countryName].filter(Boolean).join(" · ") || null; | |
| const mapSubtitle = | |
| [...new Set([regionName, countryName].filter(Boolean))] | |
| .filter((v) => v !== mapTitle) | |
| .join(" · ") || null; |
This deduplicates repeated labels within the subtitle and suppresses any part that already appears in the title.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/ip-details/location-section.tsx
Line: 85-86
Comment:
**Redundant subtitle when region equals country name**
`mapSubtitle` is built from `[regionName, countryName]` without filtering values that duplicate `mapTitle` or each other. For a location like Hong Kong (where city, region, and country all share the same name), this produces a subtitle of `"Hong Kong · Hong Kong"` under a title of `"Hong Kong"` — fully redundant.
```suggestion
const mapSubtitle =
[...new Set([regionName, countryName].filter(Boolean))]
.filter((v) => v !== mapTitle)
.join(" · ") || null;
```
This deduplicates repeated labels within the subtitle and suppresses any part that already appears in the title.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| describe("checkReadiness", () => { | ||
| it("returns healthy when all components are up", async () => { | ||
| process.env.DSN = "postgres://test"; | ||
| process.env.REDIS_URL = "redis://localhost:6379"; | ||
| mocks.dbExecute.mockResolvedValue([{ "?column?": 1 }]); | ||
| mocks.getRedisClient.mockReturnValue({ | ||
| status: "ready", | ||
| ping: vi.fn().mockResolvedValue("PONG"), | ||
| }); | ||
| mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 })); | ||
| const { checkReadiness } = await import("@/lib/health/checker"); | ||
| const result = await checkReadiness(); | ||
| expect(result.status).toBe("healthy"); | ||
| expect(result.version).toBe("0.6.8"); | ||
| expect(result.uptime).toBeGreaterThanOrEqual(0); | ||
| expect(result.components?.database?.status).toBe("up"); | ||
| expect(result.components?.redis?.status).toBe("up"); | ||
| expect(result.components?.proxy?.status).toBe("up"); | ||
| delete process.env.DSN; | ||
| delete process.env.REDIS_URL; | ||
| }); | ||
|
|
||
| it("returns degraded when Redis is down but DB and proxy are up", async () => { | ||
| process.env.DSN = "postgres://test"; | ||
| process.env.REDIS_URL = "redis://localhost:6379"; | ||
| mocks.dbExecute.mockResolvedValue([{ "?column?": 1 }]); | ||
| mocks.getRedisClient.mockReturnValue({ | ||
| status: "ready", | ||
| ping: vi.fn().mockRejectedValue(new Error("ECONNRESET")), | ||
| }); | ||
| mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 })); | ||
| const { checkReadiness } = await import("@/lib/health/checker"); | ||
| const result = await checkReadiness(); | ||
| expect(result.status).toBe("degraded"); | ||
| expect(result.components?.database?.status).toBe("up"); | ||
| expect(result.components?.redis?.status).toBe("down"); | ||
| delete process.env.DSN; | ||
| delete process.env.REDIS_URL; | ||
| }); | ||
|
|
||
| it("returns degraded when proxy is down but DB and Redis are up", async () => { | ||
| process.env.DSN = "postgres://test"; | ||
| process.env.REDIS_URL = "redis://localhost:6379"; | ||
| mocks.dbExecute.mockResolvedValue([{ "?column?": 1 }]); | ||
| mocks.getRedisClient.mockReturnValue({ | ||
| status: "ready", | ||
| ping: vi.fn().mockResolvedValue("PONG"), | ||
| }); | ||
| mocks.v1App.request.mockRejectedValue(new Error("middleware crashed")); | ||
| const { checkReadiness } = await import("@/lib/health/checker"); | ||
| const result = await checkReadiness(); | ||
| expect(result.status).toBe("degraded"); | ||
| expect(result.components?.proxy?.status).toBe("down"); | ||
| delete process.env.DSN; | ||
| delete process.env.REDIS_URL; | ||
| }); | ||
|
|
||
| it("returns unhealthy when DB is down", async () => { | ||
| process.env.DSN = "postgres://test"; | ||
| process.env.REDIS_URL = "redis://localhost:6379"; | ||
| mocks.dbExecute.mockRejectedValue(new Error("connection refused")); | ||
| mocks.getRedisClient.mockReturnValue({ | ||
| status: "ready", | ||
| ping: vi.fn().mockResolvedValue("PONG"), | ||
| }); | ||
| mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 })); | ||
| const { checkReadiness } = await import("@/lib/health/checker"); | ||
| const result = await checkReadiness(); | ||
| expect(result.status).toBe("unhealthy"); | ||
| expect(result.components?.database?.status).toBe("down"); | ||
| delete process.env.DSN; | ||
| delete process.env.REDIS_URL; | ||
| }); | ||
|
|
||
| it("returns healthy when Redis is unchecked (not configured)", async () => { | ||
| process.env.DSN = "postgres://test"; | ||
| mocks.dbExecute.mockResolvedValue([{ "?column?": 1 }]); | ||
| mocks.getRedisClient.mockReturnValue(null); | ||
| mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 })); | ||
| const { checkReadiness } = await import("@/lib/health/checker"); | ||
| const result = await checkReadiness(); | ||
| expect(result.status).toBe("healthy"); | ||
| expect(result.components?.redis?.status).toBe("unchecked"); | ||
| delete process.env.DSN; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
checkReadiness test for DB-unchecked scenario
The new "DSN not configured → unchecked" path is well-covered at the checkDatabase unit level (line 54–59), but the checkReadiness integration suite has no case where process.env.DSN is absent. Adding that case would confirm that the overall status stays "healthy" (not "unhealthy") when the database is intentionally unchecked — the central change this PR introduces for DSN-less test environments.
it("returns healthy when DB is unchecked (DSN not configured)", async () => {
delete process.env.DSN;
mocks.getRedisClient.mockReturnValue({ status: "ready", ping: vi.fn().mockResolvedValue("PONG") });
mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 }));
const { checkReadiness } = await import("@/lib/health/checker");
const result = await checkReadiness();
expect(result.status).toBe("healthy");
expect(result.components?.database?.status).toBe("unchecked");
});Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/lib/health-checker.test.ts
Line: 213-299
Comment:
**Missing `checkReadiness` test for DB-unchecked scenario**
The new "DSN not configured → `unchecked`" path is well-covered at the `checkDatabase` unit level (line 54–59), but the `checkReadiness` integration suite has no case where `process.env.DSN` is absent. Adding that case would confirm that the overall status stays `"healthy"` (not `"unhealthy"`) when the database is intentionally unchecked — the central change this PR introduces for DSN-less test environments.
```ts
it("returns healthy when DB is unchecked (DSN not configured)", async () => {
delete process.env.DSN;
mocks.getRedisClient.mockReturnValue({ status: "ready", ping: vi.fn().mockResolvedValue("PONG") });
mocks.v1App.request.mockResolvedValue(new Response('{"status":"pong"}', { status: 200 }));
const { checkReadiness } = await import("@/lib/health/checker");
const result = await checkReadiness();
expect(result.status).toBe("healthy");
expect(result.components?.database?.status).toBe("unchecked");
});
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/unit/lib/health-checker.test.ts (1)
37-40: 统一清理环境变量，避免失败用例污染后续测试。这些测试在断言后手动
delete process.env.DSN；一旦中途失败，清理不会执行，后续 “DSN 未设置” 场景可能被污染。建议在beforeEach中统一清理，再按用例显式设置需要的变量。建议修改
beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); + delete process.env.DSN; + delete process.env.REDIS_URL; });应用后，各测试末尾的
delete process.env.DSN/delete process.env.REDIS_URL可以移除，减少重复清理逻辑。Also applies to: 54-91, 216-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/health-checker.test.ts` around lines 37 - 40, Add unified environment cleanup in the test setup by clearing relevant env vars inside the existing beforeEach (the function containing vi.clearAllMocks() and vi.resetModules()), e.g., delete process.env.DSN and delete process.env.REDIS_URL (or set them to undefined) so each test starts with a clean environment; then remove per-test cleanup lines that manually delete process.env.DSN / process.env.REDIS_URL at the end of individual tests (those scattered around the file and the ranges noted) to avoid duplication and ensure failed tests don’t leak env state.src/app/globals.css (1)
147-152: Tailwind v4!后缀语法正确，但建议补充底色与可读性兜底。
bg-transparent! shadow-none! p-0! rounded-none!与hidden!的语法符合 Tailwind v4 规范（重要修饰符位于类名末尾）。两点建议：
- 这些覆盖置于
@layer base中会被任何后续 utility 覆盖，通常此类第三方 UI 覆盖放到@layer components更稳妥，以保证优先级符合预期但仍可被业务显式覆盖。- 将 popup tip 直接
hidden!会让 popup 失去指向锚点的视觉箭头，请确认这是设计意图（结合地图卡片上已有 title/subtitle 呈现的情况下通常是可接受的）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 147 - 152, Move the MapLibre popup overrides out of the `@layer` base into `@layer` components and add a readable fallback background/text for .maplibregl-popup-content instead of only bg-transparent! — update the selector .maplibregl-popup-content to live inside `@layer` components, keep the important modifiers (bg-transparent! shadow-none! p-0! rounded-none!) but add a fallback like a light background and readable text color to ensure legibility when transparency is overridden, and leave .maplibregl-popup-tip handling as-is only if the removal of the arrow is intentional (otherwise remove hidden! from .maplibregl-popup-tip) so designers can still override in business styles.src/app/[locale]/dashboard/_components/ip-details/location-section.tsx (1)
85-86: 地图卡片标题/副标题可能出现重复值当
city、region、country相同（如测试中的 Hong Kong：三者都是Hong Kong），会渲染出title="Hong Kong"且subtitle="Hong Kong · Hong Kong"，标题与副标题各片段之间重复。另外当cityName为空、mapTitle回退到regionName时，subtitle仍然会再次包含相同的regionName。建议在
subtitle组装时过滤掉与mapTitle相同的片段，并对片段自身去重：♻️ 建议改动
- const mapTitle = cityName ?? regionName ?? countryName ?? t("hero.location"); - const mapSubtitle = [regionName, countryName].filter(Boolean).join(" · ") || null; + const mapTitle = cityName ?? regionName ?? countryName ?? t("hero.location"); + const subtitleParts = Array.from( + new Set([regionName, countryName].filter((v): v is string => !!v && v !== mapTitle)) + ); + const mapSubtitle = subtitleParts.length > 0 ? subtitleParts.join(" · ") : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/ip-details/location-section.tsx around lines 85 - 86, The title/subtitle logic can produce duplicated values (e.g., city/region/country all "Hong Kong"); update mapTitle and mapSubtitle handling so you build parts from [cityName, regionName, countryName], pick mapTitle as the first non-empty (cityName ?? regionName ?? countryName ?? t("hero.location")), then assemble mapSubtitle by taking the remaining non-empty parts, removing any entries equal to the chosen mapTitle, and de-duplicating while preserving order before joining with " · " (or returning null if empty). Change the existing mapTitle and mapSubtitle expressions to use these symbols (mapTitle, mapSubtitle, cityName, regionName, countryName, t("hero.location")) so subtitle never repeats the title or includes duplicate segments.src/components/ui/map.tsx (1)
454-478: 在渲染阶段直接变更 MapLibre 实例，违反 React 渲染纯度第 454-478 行在函数组件渲染过程中对
marker实例直接调用setLngLat/setDraggable/setOffset/setRotation/setRotationAlignment/setPitchAlignment。同样的模式也出现在MarkerPopup（551-562 行，调用popup.setOffset/popup.setMaxWidth）、MarkerTooltip（635-646 行，调用tooltip.setOffset/tooltip.setMaxWidth）与MapPopup（957-971 行，调用popup.setLngLat/popup.setOffset/popup.setMaxWidth）。这些都是宿主对象上的副作用，应放在
useEffect中。React 要求组件渲染必须是纯函数，避免在渲染过程中变更外部可变对象状态：
- React 19 StrictMode 在开发模式下对组件执行双次渲染，虽然当前代码通过等值判断做了幂等保护，但仍非推荐写法。
- 在 concurrent rendering 中，React 可能中止某次渲染，已经写入 MapLibre 实例的状态与最终 commit 后 props 所期望的状态会出现短暂不一致，可能导致 UI 显示错误。
建议将每项 setter 包装到以对应 prop 为依赖的
useEffect内，例如：♻️ 建议改动（以 MapMarker 为例）
- if (marker.getLngLat().lng !== longitude || marker.getLngLat().lat !== latitude) { - marker.setLngLat([longitude, latitude]); - } - if (marker.isDraggable() !== draggable) { - marker.setDraggable(draggable); - } - - const currentOffset = marker.getOffset(); - const newOffset = markerOptions.offset ?? [0, 0]; - const [newOffsetX, newOffsetY] = Array.isArray(newOffset) - ? newOffset - : [newOffset.x, newOffset.y]; - if (currentOffset.x !== newOffsetX || currentOffset.y !== newOffsetY) { - marker.setOffset(newOffset); - } - - if (marker.getRotation() !== markerOptions.rotation) { - marker.setRotation(markerOptions.rotation ?? 0); - } - if (marker.getRotationAlignment() !== markerOptions.rotationAlignment) { - marker.setRotationAlignment(markerOptions.rotationAlignment ?? "auto"); - } - if (marker.getPitchAlignment() !== markerOptions.pitchAlignment) { - marker.setPitchAlignment(markerOptions.pitchAlignment ?? "auto"); - } + useEffect(() => { + marker.setLngLat([longitude, latitude]); + }, [marker, longitude, latitude]); + + useEffect(() => { + marker.setDraggable(draggable); + }, [marker, draggable]); + + useEffect(() => { + marker.setOffset(markerOptions.offset ?? [0, 0]); + marker.setRotation(markerOptions.rotation ?? 0); + marker.setRotationAlignment(markerOptions.rotationAlignment ?? "auto"); + marker.setPitchAlignment(markerOptions.pitchAlignment ?? "auto"); + }, [ + marker, + markerOptions.offset, + markerOptions.rotation, + markerOptions.rotationAlignment, + markerOptions.pitchAlignment, + ]);
MarkerPopup/MarkerTooltip/MapPopup中setOffset/setMaxWidth/setLngLat也建议同样处理。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/map.tsx` around lines 454 - 478, The code mutates MapLibre instances during render (calls like marker.setLngLat, marker.setDraggable, marker.setOffset, marker.setRotation, marker.setRotationAlignment, marker.setPitchAlignment) — move all such setter calls out of the render path into one or more useEffect hooks that run when the relevant props change; for example, wrap the marker updates (referencing marker, markerOptions, longitude, latitude, draggable) in a useEffect with [longitude, latitude, draggable, markerOptions] (and deconstruct offsets/rotation/alignment) as dependencies, and do the same for MarkerPopup (popup.setOffset/setMaxWidth), MarkerTooltip (tooltip.setOffset/setMaxWidth) and MapPopup (popup.setLngLat/setOffset/setMaxWidth) so that all side effects run only inside effects rather than during render.
🤖 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/app/v1/`[...route]/route.ts:
- Around line 22-29: Currently the code skips sensitiveWordDetector.reload()
whenever process.env.DSN is missing, which allows ProxySensitiveWordGuard to
pass all requests if the detector is empty; change the logic to only skip (and
log a non-fatal message) when running in tests (e.g., process.env.NODE_ENV ===
'test' or a dedicated TEST env flag), and for non-test environments treat a
missing/blank DSN as a fatal misconfiguration by failing fast: either throw an
error or exit the process and log a clear error so the app/agent does not start
with sensitive-word protection disabled; update the block around
sensitiveWordDetector.reload(), the related logging, and any startup/failure
handling so ProxySensitiveWordGuard and sensitiveWordDetector.isEmpty() cannot
silently allow traffic in production.
In `@src/components/ui/map.tsx`:
- Around line 578-582: MarkerPopup, MapPopup, MapControls (including Zoom
in/Zoom out, Find my location, Toggle fullscreen) and CompassButton currently
hardcode English aria-label/sr-only strings; update them to use i18n either by
(A) importing next-intl's useTranslations in src/components/ui/map.tsx and
replacing all hardcoded strings with translation keys like ui.map.markerClose,
ui.map.zoomIn, ui.map.zoomOut, ui.map.findLocation, ui.map.toggleFullscreen,
ui.map.compass (and use t(...) where MarkerPopup, MapPopup, MapControls,
CompassButton render aria-label / sr-only) or (B) add a labels prop to
MapControls/MapPopup/MarkerPopup (e.g., labels: { close, zoomIn, zoomOut,
findLocation, toggleFullscreen, compass }) and update LocationMapCard and other
callers to pass translated strings from useTranslations; ensure all user-facing
strings reference ui.map.* keys and support zh-CN/zh-TW/en/ja/ru.
In `@src/lib/health/checker.ts`:
- Around line 41-44: The checkDatabase() currently returns status "unchecked"
when process.env.DSN is missing, which allows checkReadiness() to treat a
required database as healthy; change the behavior so that when DSN is missing
outside of test environments (e.g., NODE_ENV !== "test") checkDatabase() returns
a failing state (e.g., status "failed" with message "Database not configured")
OR update checkReadiness() to treat an "unchecked" result from checkDatabase()
as a failure for required components; locate the DSN check in checkDatabase()
and the readiness aggregation logic in checkReadiness() and implement one of
these fixes so production missing DSN fails readiness while allowing "unchecked"
only in tests.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/ip-details/location-section.tsx:
- Around line 85-86: The title/subtitle logic can produce duplicated values
(e.g., city/region/country all "Hong Kong"); update mapTitle and mapSubtitle
handling so you build parts from [cityName, regionName, countryName], pick
mapTitle as the first non-empty (cityName ?? regionName ?? countryName ??
t("hero.location")), then assemble mapSubtitle by taking the remaining non-empty
parts, removing any entries equal to the chosen mapTitle, and de-duplicating
while preserving order before joining with " · " (or returning null if empty).
Change the existing mapTitle and mapSubtitle expressions to use these symbols
(mapTitle, mapSubtitle, cityName, regionName, countryName, t("hero.location"))
so subtitle never repeats the title or includes duplicate segments.
In `@src/app/globals.css`:
- Around line 147-152: Move the MapLibre popup overrides out of the `@layer` base
into `@layer` components and add a readable fallback background/text for
.maplibregl-popup-content instead of only bg-transparent! — update the selector
.maplibregl-popup-content to live inside `@layer` components, keep the important
modifiers (bg-transparent! shadow-none! p-0! rounded-none!) but add a fallback
like a light background and readable text color to ensure legibility when
transparency is overridden, and leave .maplibregl-popup-tip handling as-is only
if the removal of the arrow is intentional (otherwise remove hidden! from
.maplibregl-popup-tip) so designers can still override in business styles.
In `@src/components/ui/map.tsx`:
- Around line 454-478: The code mutates MapLibre instances during render (calls
like marker.setLngLat, marker.setDraggable, marker.setOffset,
marker.setRotation, marker.setRotationAlignment, marker.setPitchAlignment) —
move all such setter calls out of the render path into one or more useEffect
hooks that run when the relevant props change; for example, wrap the marker
updates (referencing marker, markerOptions, longitude, latitude, draggable) in a
useEffect with [longitude, latitude, draggable, markerOptions] (and deconstruct
offsets/rotation/alignment) as dependencies, and do the same for MarkerPopup
(popup.setOffset/setMaxWidth), MarkerTooltip (tooltip.setOffset/setMaxWidth) and
MapPopup (popup.setLngLat/setOffset/setMaxWidth) so that all side effects run
only inside effects rather than during render.
In `@tests/unit/lib/health-checker.test.ts`:
- Around line 37-40: Add unified environment cleanup in the test setup by
clearing relevant env vars inside the existing beforeEach (the function
containing vi.clearAllMocks() and vi.resetModules()), e.g., delete
process.env.DSN and delete process.env.REDIS_URL (or set them to undefined) so
each test starts with a clean environment; then remove per-test cleanup lines
that manually delete process.env.DSN / process.env.REDIS_URL at the end of
individual tests (those scattered around the file and the ranges noted) to avoid
duplication and ensure failed tests don’t leak env state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37f5d0ae-7c96-459b-a7c6-f69e824180d7
📒 Files selected for processing (16)
components.jsonmessages/en/ipDetails.jsonmessages/ja/ipDetails.jsonmessages/ru/ipDetails.jsonmessages/zh-CN/ipDetails.jsonmessages/zh-TW/ipDetails.jsonpackage.jsonsrc/app/[locale]/dashboard/_components/ip-details-dialog.test.tsxsrc/app/[locale]/dashboard/_components/ip-details/atoms.tsxsrc/app/[locale]/dashboard/_components/ip-details/location-map-card.tsxsrc/app/[locale]/dashboard/_components/ip-details/location-section.tsxsrc/app/globals.csssrc/app/v1/[...route]/route.tssrc/components/ui/map.tsxsrc/lib/health/checker.tstests/unit/lib/health-checker.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The change is scoped cleanly: the hasMeaningfulCoordinates relaxation correctly guards NaN/Infinity while preserving the (0,0) sentinel check, the regrouped location layout preserves all prior fields under a new collapsible, and the DSN-less guards in route.ts and checker.ts are symmetric (warmup skip + unchecked status) and covered by tests. The bulk of the diff (src/components/ui/map.tsx, 1426 lines) is vendored from the @mapcn shadcn registry and is out of review scope.
PR Size: XL
- Lines changed: 2014 (+1866 / -148)
- Files changed: 16
- Split suggestion (for future similar PRs): the vendored
map.tsxregistry import could land as its own commit/PR separate from the IP-details regrouping and the DSN-less health/proxy fixes — three concerns bundled here would each be reviewable in isolation.
Review Coverage
- Logic and correctness - Clean (coordinate validation correctly handles NaN/Infinity/null-island;
hasLocationContentshort-circuits are consistent with display logic) - Security (OWASP Top 10) - Clean (no new sinks; MapLibre tile URLs are registry-sourced constants, not user input)
- Error handling - Clean (
checker.tsreturns structureduncheckedstatus rather than silent failure; warmup skip is explicitly logged at info level) - Type safety - Clean (no
anyintroduced in PR-specific code;hasMeaningfulCoordinatesparameter is properly typed) - Documentation accuracy - Clean (JSDoc on
hasMeaningfulCoordinatescorrectly reflects the new permissive behavior) - Test coverage - Adequate (null-accuracy rendering, map mount, country/timezone collapse, and the new DSN-absent
checkDatabasepath are all covered) - Code clarity - Good
- i18n - Complete (
sections.countryTimezoneadded in all 5 locales)
Automated review by Claude AI
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/lib/health-checker.test.ts (1)
56-62: 冗余的delete process.env.DSN
beforeEach（第 40 行）已经在每个用例前清理process.env.DSN，这里第 57 行的delete是多余的，可以删除以保持简洁。当然保留也能更清晰地表达该用例的前置条件，属于可选风格取舍。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/health-checker.test.ts` around lines 56 - 62, The test contains a redundant delete of process.env.DSN inside the "returns unchecked when DSN is not set" case; remove the line `delete process.env.DSN` from that it-block to rely on the existing beforeEach cleanup and keep the test concise, leaving the rest of the test (importing "@/lib/health/checker" and asserting checkDatabase() returns status "unchecked" and a message containing "not configured") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/lib/health-checker.test.ts`:
- Around line 56-62: The test contains a redundant delete of process.env.DSN
inside the "returns unchecked when DSN is not set" case; remove the line `delete
process.env.DSN` from that it-block to rely on the existing beforeEach cleanup
and keep the test concise, leaving the rest of the test (importing
"@/lib/health/checker" and asserting checkDatabase() returns status "unchecked"
and a message containing "not configured") unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ada1bb43-d2cb-4182-8ed9-ad47dc8e0fce
📒 Files selected for processing (4)
src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsxsrc/app/[locale]/dashboard/_components/ip-details/location-section.tsxsrc/app/globals.csstests/unit/lib/health-checker.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/app/globals.css
- src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/[locale]/dashboard/_components/ip-details/location-section.tsx
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0412e7b3dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| population: 0, | ||
| borders: ["CN"], | ||
| is_eu_member: false, | ||
| flag: { emoji: "🇭🇰", unicode: "U+1F1ED U+1F1F0", svg: null, png: null }, |
There was a problem hiding this comment.
This commit introduces an emoji string literal ("🇭🇰") in source code, which violates the repository’s AGENTS.md critical rule “No Emoji in Code” (applies to code, comments, and string literals). Because this rule is explicit and repo-wide, leaving this value in will keep the branch non-compliant and likely force repeated cleanup in subsequent changes.
Useful? React with 👍 / 👎.
| if (dashArray) { | ||
| map.setPaintProperty(layerId, "line-dasharray", dashArray); | ||
| } |
There was a problem hiding this comment.
In MapRoute, the paint update only calls setPaintProperty("line-dasharray", ...) when dashArray is truthy, so switching a route from dashed to solid (dashArray from [x,y] to undefined) leaves the old dash pattern applied. This causes stale visual state for any consumer that toggles dashed/solid routes at runtime.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,1426 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] New map UI wrapper has no unit tests
Why this is a problem: CLAUDE.md says: **Test Coverage** - All new features must have unit test coverage of at least 80%. src/components/ui/map.tsx adds a large wrapper (theme resolution, controlled viewport sync, controls, popups, routes, clustering) but no tests were added; the dashboard test suite mocks this module, so it doesn’t exercise any of this behavior.
Suggested fix:
// src/components/ui/map.test.tsx
import { describe, expect, it, vi } from "vitest";
vi.mock("maplibre-gl", () => {
class MapMock {
getZoom = vi.fn(() => 3);
zoomTo = vi.fn();
resetNorthPitch = vi.fn();
getContainer = vi.fn(() => document.createElement("div"));
on = vi.fn();
off = vi.fn();
remove = vi.fn();
}
return { default: { Map: MapMock, Popup: class {}, Marker: class {} } };
});
// Render <Map><MapControls showZoom /></Map> and assert zoomTo() is called on button clicks.Start by covering MapControls zoom in/out + popup close behavior; expand to controlled viewport (jumpTo) and style/theme switching.
There was a problem hiding this comment.
Code Review Summary
XL PR bundles three unrelated concerns (IP details UI refactor, new MapLibre-based map component + deps, and health-check behavior changes), which makes review and rollback riskier.
The readiness path can now report healthy (HTTP 200) when DSN is missing due to the new database unchecked state, potentially allowing misconfigured deployments to receive traffic.
The new map wrapper also adds user-facing control labels without i18n and currently has no unit tests.
PR Size: XL
- Lines changed: 2014
- Files changed: 16
- Split suggestion: (1)
maplibre-gl+src/components/ui/map.tsx+globals.cssas a standalone PR, (2) IP details UI + i18n keys/tests, (3) health checker +/v1warmup gating.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 1 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/lib/health/checker.ts:43[LOGIC-BUG]checkDatabase()returnsuncheckedwhenDSNis missing, butcheckReadiness()only treatsdatabase.status === "down"as unhealthy → readiness can stayhealthy/HTTP 200 on DB-misconfigured instances. Confidence: 95
High Priority Issues (Should Fix)
src/components/ui/map.tsx:814[STANDARD-VIOLATION] Hardcoded user-facing control labels / popup close labels (aria-label + sr-only) bypass i18n. Confidence: 90src/components/ui/map.tsx:1[TEST-MISSING-CRITICAL] New map UI wrapper added without unit tests (dashboard tests mock the module, so wrapper behavior isn’t covered). Confidence: 80
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Identified PR
#1032and applied labelsize/XL. - Posted inline review feedback (including a new comment on
src/components/ui/map.tsx:1for missing unit tests, plus fix suggestions on the existing threads forsrc/components/ui/map.tsxi18n labels andsrc/lib/health/checker.ts:43readiness/DSN behavior). - Submitted the required “Code Review Summary” via
gh pr review --comment.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/components/ui/map.tsx (2)
507-523:⚠️ Potential issue | 🟠 Major移除英文 aria-label fallback，避免通用组件被直接使用时绕过 i18n。
closeLabel = "Close popup"以及mergedLabels里的"Zoom in"等默认值仍是可触达的用户可见文本；如果其他调用方未传labels/closeLabel，屏幕阅读器会读到英文。建议让这些 label 必传，或在组件内部统一接入next-intl。这个问题与之前的 i18n 评论一致。As per coding guidelines: "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
Also applies to: 693-778, 917-942
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/map.tsx` around lines 507 - 523, The MarkerPopup component currently provides hardcoded English fallback labels (e.g., closeLabel = "Close popup" and entries in mergedLabels) which violates i18n rules; update MarkerPopup to remove English defaults and instead either (A) make the closeLabel and any labels in mergedLabels required props so callers must supply localized strings, or (B) integrate next-intl inside MarkerPopup to load translations for the five supported locales and use those for defaults; adjust the MarkerPopup function signature and any mergedLabels logic accordingly (refer to MarkerPopup, closeLabel, and mergedLabels) and apply the same change pattern to the other similar sections called out (around the mergedLabels usage at the other ranges).
1-1:⚠️ Potential issue | 🟠 Major请补上这个 Map wrapper 的直接单元测试。
当前新增的是一套复杂通用组件，但 PR 摘要里提到的测试主要是 mock
@/components/ui/map，无法覆盖控件点击、popup 关闭、受控 viewport、style/theme 切换等行为。这个问题与之前的测试缺口评论一致。可用下面的只读检查确认当前分支是否已有直接覆盖：
#!/bin/bash # 检查是否存在直接覆盖 src/components/ui/map.tsx 的测试。 fd -i 'map\.(test|spec)\.(ts|tsx)$' . rg -n -C2 '@/components/ui/map|from ".*components/ui/map"|<MapControls|MapPopup|MapMarker|MapRoute|MapClusterLayer' \ --glob '*.{test,spec}.{ts,tsx}' \ --glob '!**/node_modules/**'Based on learnings: "All new features must have unit test coverage of at least 80%".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/map.tsx` at line 1, 为 src/components/ui/map.tsx 补上直接单元测试：编写一个或多个 test/spec 文件（例如 map.test.tsx）直接导入 Map 组件并使用 React Testing Library + user-event 模拟交互，覆盖控件点击（MapControls）、弹窗打开与关闭（MapPopup）、受控 viewport 行为（通过 props 更新并断言地图视图变化）、以及切换 style/theme 导致的 DOM/prop 变化（MapMarker/MapRoute/MapClusterLayer 可见性或 class 变化）；在测试中可 mock 地图底层实现（maplibre/mapbox）但不要只 mock '@/components/ui/map' 本身，确保断言实际组件输出与交互结果以把覆盖率提升到要求并满足 PR 要求的直接覆盖检查。
🤖 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/components/ui/map.tsx`:
- Around line 1104-1116: The effect that updates the route source (the useEffect
referencing isLoaded, map, coordinates, sourceId) currently returns early when
coordinates.length < 2, leaving any previously drawn route on the map; change
the logic so you still fetch the source via map.getSource(sourceId) as
MapLibreGL.GeoJSONSource and when coordinates.length < 2 call source.setData
with an empty geometry (e.g., an empty Feature or FeatureCollection) to clear
the layer, otherwise set the LineString as before; ensure this handles the case
where source is undefined before calling setData.
- Around line 1291-1299: The useEffect currently skips updates when data is a
string, preventing GeoJSON URL changes from being applied; update the effect in
the MapClusterLayer component to remove the typeof data === "string" check so it
always retrieves the source (via map.getSource(sourceId)) and calls
source.setData(data) when isLoaded, map, and source exist; keep the dependency
array [isLoaded, map, data, sourceId] and ensure source is cast to
MapLibreGL.GeoJSONSource before calling setData.
- Around line 237-267: Replace the 100ms timeout workaround in styleDataHandler
with a robust readiness check: remove use of styleTimeoutRef and the fixed
setTimeout, and instead wait until map.isStyleLoaded() returns true or listen
for the map "idle" event before calling setIsStyleLoaded and applying
projection; update the registration from map.on("styledata", styleDataHandler)
to either use map.on("idle", idleHandler) or have styleDataHandler poll
map.isStyleLoaded() (with a short retry/backoff) before calling setIsStyleLoaded
and map.setProjection(projection), so subsequent calls from
MapRoute/MapClusterLayer to addSource/addLayer only run after the style is fully
loaded.
In `@tests/unit/lib/health-checker.test.ts`:
- Around line 284-294: The test for checkReadiness is missing a REDIS_URL, so
the Redis ready mock (mocks.getRedisClient) may be ignored and the DB/Redis
"unchecked" state isn't isolated; before importing checkReadiness in the test
case, set process.env.REDIS_URL to a valid value (e.g. "redis://localhost:6379")
so the mocked getRedisClient and ping are applied, then import checkReadiness
and run assertions; reference the test's use of mocks.getRedisClient,
mocks.v1App.request and the checkReadiness import to locate where to insert the
env assignment and optionally restore or clear REDIS_URL after the test.
- Around line 37-42: Save the original values of process.env.DSN and
process.env.REDIS_URL at the start of the test (e.g., capture them before
calling vi.clearAllMocks()/vi.resetModules() in the beforeEach or at module
scope), and add an afterEach that restores those originals (setting them back to
the saved values or deleting them if they were undefined); update the existing
beforeEach/afterEach vicinity where vi.clearAllMocks and vi.resetModules are
used to reference the saved originals so the environment is restored after each
test.
---
Duplicate comments:
In `@src/components/ui/map.tsx`:
- Around line 507-523: The MarkerPopup component currently provides hardcoded
English fallback labels (e.g., closeLabel = "Close popup" and entries in
mergedLabels) which violates i18n rules; update MarkerPopup to remove English
defaults and instead either (A) make the closeLabel and any labels in
mergedLabels required props so callers must supply localized strings, or (B)
integrate next-intl inside MarkerPopup to load translations for the five
supported locales and use those for defaults; adjust the MarkerPopup function
signature and any mergedLabels logic accordingly (refer to MarkerPopup,
closeLabel, and mergedLabels) and apply the same change pattern to the other
similar sections called out (around the mergedLabels usage at the other ranges).
- Line 1: 为 src/components/ui/map.tsx 补上直接单元测试：编写一个或多个 test/spec 文件（例如
map.test.tsx）直接导入 Map 组件并使用 React Testing Library + user-event
模拟交互，覆盖控件点击（MapControls）、弹窗打开与关闭（MapPopup）、受控 viewport 行为（通过 props
更新并断言地图视图变化）、以及切换 style/theme 导致的 DOM/prop 变化（MapMarker/MapRoute/MapClusterLayer
可见性或 class 变化）；在测试中可 mock 地图底层实现（maplibre/mapbox）但不要只 mock '@/components/ui/map'
本身，确保断言实际组件输出与交互结果以把覆盖率提升到要求并满足 PR 要求的直接覆盖检查。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3de1e2a5-fbcb-4dc0-8dea-7005ce10d8cb
📒 Files selected for processing (10)
messages/en/ipDetails.jsonmessages/ja/ipDetails.jsonmessages/ru/ipDetails.jsonmessages/zh-CN/ipDetails.jsonmessages/zh-TW/ipDetails.jsonsrc/app/[locale]/dashboard/_components/ip-details/location-map-card.tsxsrc/app/v1/[...route]/route.tssrc/components/ui/map.tsxsrc/lib/health/checker.tstests/unit/lib/health-checker.test.ts
✅ Files skipped from review due to trivial changes (2)
- messages/ja/ipDetails.json
- messages/zh-TW/ipDetails.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/health/checker.ts
- messages/zh-CN/ipDetails.json
- src/app/[locale]/dashboard/_components/ip-details/location-map-card.tsx
- messages/ru/ipDetails.json
- messages/en/ipDetails.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27d977bc20
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <MapPopup | ||
| longitude={longitude} | ||
| latitude={latitude} | ||
| className="w-56" | ||
| closeLabel={t("map.closePopup")} |
There was a problem hiding this comment.
LocationMapCard mounts MapPopup without disabling map-click close behavior, so a background click closes the popup and there is no marker or control to bring it back. In this card that leaves users with a blank map until the dialog remounts, which hides the location context and coordinates after an accidental click.
Useful? React with 👍 / 👎.
|
|
||
| // When coordinates change, update the source data | ||
| useEffect(() => { | ||
| if (!isLoaded || !map || coordinates.length < 2) return; |
There was a problem hiding this comment.
The route update effect returns early when coordinates.length < 2, so if a previously rendered route is later cleared (or reduced to one point), the old LineString stays in the source and remains visible. This produces stale map output in dynamic route flows; the source should be explicitly reset to an empty geometry in this case.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/`[locale]/dashboard/_components/ip-details/location-map-card.tsx:
- Around line 49-57: The Popup is closing when the user clicks the map because
MapLibre's PopupOptions.closeOnClick defaults to true; update the MapPopup usage
in location-map-card.tsx so the popup does not auto-close by passing the Popup
option to disable that behavior (e.g., pass closeOnClick: false via a prop such
as popupOptions={{ closeOnClick: false }} or closeOnClick={false} depending on
MapPopup's API), ensuring the popup remains visible after map clicks and still
provides a manual close or marker-based reopen mechanism if needed.
- Around line 43-49: hasMeaningfulCoordinates() only checks finiteness and
non-(0,0) so invalid latitude values (e.g., 91 or -91) can reach Map and cause
MapLibre to throw; update the component that renders <Map> (the block using
viewport with center: [longitude, latitude] and zoom:
resolveZoom(accuracyRadiusKm)) to validate latitude is within -90..90 (and
optionally longitude within -180..180) before passing to Map; if out of range,
either clamp the values to the valid ranges or return/skip rendering the
Map/MapPopup to avoid initializing MapLibre with invalid LngLat — reference
hasMeaningfulCoordinates(), longitude, latitude, resolveZoom(), Map, and
MapPopup when applying the check.
In `@src/components/ui/__tests__/map.test.tsx`:
- Around line 419-482: The test replaces window.navigator.geolocation and calls
unmount() but doesn’t guarantee restoration or unmount on assertion failures;
wrap the body of the test that mutates geolocation and renders the component in
a try/finally so you always call unmount() and restore the original geolocation
descriptor: capture the original descriptor in geolocationDescriptor, set the
stubbed value, then in finally call unmount() if defined and if
geolocationDescriptor exists restore it with
Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor)
else delete the own property (or redefine configurable undefined only when
necessary) so the original prototype property is visible again; update the test
around Map/MapControls, geolocationDescriptor, and unmount to use this
try/finally pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d37cc5a7-2bbe-40f1-9390-bfd4cc70cee7
📒 Files selected for processing (4)
src/app/[locale]/dashboard/_components/ip-details/location-map-card.tsxsrc/components/ui/__tests__/map.test.tsxsrc/components/ui/map.tsxtests/unit/lib/health-checker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/map.tsx
| <Map | ||
| viewport={{ | ||
| center: [longitude, latitude], | ||
| zoom: resolveZoom(accuracyRadiusKm), | ||
| }} | ||
| > | ||
| <MapPopup longitude={longitude} latitude={latitude} className="w-56"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/[locale]/dashboard/_components/ip-details/location-map-card.tsxRepository: ding113/claude-code-hub
Length of output: 2885
🏁 Script executed:
# Search for coordinate validation logic in the codebase
rg "latitude|longitude" --type ts --type tsx -B 2 -A 2 | head -100Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Check if there's existing coordinate validation in parent components
fd -e tsx -e ts | xargs rg "LocationMapCard" -B 3 -A 3 | head -100Repository: ding113/claude-code-hub
Length of output: 2065
🌐 Web query:
MapLibre GL JS LngLat constructor latitude range validation error
💡 Result:
The MapLibre GL JS LngLat constructor validates latitude values and throws an error if they are outside the range -90 to 90 degrees. Specifically, after checking for NaN values, it executes: if (this.lat > 90 || this.lat < -90) { throw new Error('Invalid LngLat latitude value: must be between -90 and 90'); } This error occurs when passing a latitude value exceeding these bounds to new LngLat(lng, lat), such as in map centering, markers, or bounds. Longitude is not range-validated in the constructor but can be wrapped using the wrap method to (-180, 180). To fix, ensure latitude inputs are clamped or normalized to [-90, 90]. For example, common issues arise from invalid data sources or miscomputed bounds from map.getBounds when crossing the antimeridian or using world copies.
Citations:
- 1: https://github.com/maplibre/maplibre-gl-js/blob/main/src/geo/lng_lat.ts
- 2: https://github.com/maplibre/maplibre-gl-js/blob/6ed25c0698c4987af5e538412ad9c665d9e2126d/src/geo/lng_lat.ts
- 3: https://www.maplibre.org/maplibre-gl-js/docs/API/classes/LngLat/
- 4: https://github.com/mapbox/mapbox-gl-js/blob/6ff7eb9990cd63e234194d2d163508db38f691c0/src/geo/lng_lat.ts
🏁 Script executed:
# Check if there's coordinate validation at the data source level
rg "coordinates" --type ts --type js -B 3 -A 3 | grep -E "(latitude|longitude|validate|clamp)" -B 3 -A 3Repository: ding113/claude-code-hub
Length of output: 7588
🏁 Script executed:
# Check location-section to see if validation happens before passing coords
cat -n src/app/[locale]/dashboard/_components/ip-details/location-section.tsx | head -100Repository: ding113/claude-code-hub
Length of output: 4677
🏁 Script executed:
# Search for any existing coordinate bounds checks in codebase
rg "90|accuracy" --type ts --type js | grep -i "lat\|valid\|bound" | head -20Repository: ding113/claude-code-hub
Length of output: 2738
🏁 Script executed:
# Find the hasMeaningfulCoordinates function definition
rg "hasMeaningfulCoordinates" --type ts --type js -A 10 | head -50Repository: ding113/claude-code-hub
Length of output: 4920
🏁 Script executed:
# Check the atoms file where hasMeaningfulCoordinates is defined
cat -n src/app/[locale]/dashboard/_components/ip-details/atoms.ts | grep -A 15 "hasMeaningfulCoordinates"Repository: ding113/claude-code-hub
Length of output: 159
🏁 Script executed:
# Find atoms file in ip-details directory
fd "atoms" src/app/[locale]/dashboard/_components/ip-details/Repository: ding113/claude-code-hub
Length of output: 126
🏁 Script executed:
# Search for hasMeaningfulCoordinates definition
rg "function hasMeaningfulCoordinates|export.*hasMeaningfulCoordinates" -A 10Repository: ding113/claude-code-hub
Length of output: 2188
🏁 Script executed:
# Verify that hasMeaningfulCoordinates is the ONLY validation before LocationMapCard
cat -n src/app/[locale]/dashboard/_components/ip-details/atoms.tsx | grep -A 5 "hasMeaningfulCoordinates"Repository: ding113/claude-code-hub
Length of output: 366
向 MapLibre 传递坐标前需要验证纬度范围
hasMeaningfulCoordinates() 仅检查值是否有限且非 (0,0)，但 MapLibre 的 LngLat 构造函数要求纬度在 -90 到 90 范围内，超出此范围会抛出错误。当异常地理数据提供纬度=91 或 -91 时，坐标能通过父组件的 hasMeaningfulCoordinates() 校验，却在此组件的 Map 视口初始化时导致渲染失败。
建议添加范围校验或提前返回以跳过地图渲染：
建议修复
+function hasMapCompatibleCoordinates(latitude: number, longitude: number): boolean {
+ return (
+ Number.isFinite(latitude) &&
+ Number.isFinite(longitude) &&
+ latitude >= -90 &&
+ latitude <= 90
+ );
+}
+
export function LocationMapCard({
latitude,
longitude,
accuracyRadiusKm,
title,
@@
}) {
const t = useTranslations("ipDetails");
+
+ if (!hasMapCompatibleCoordinates(latitude, longitude)) {
+ return null;
+ }
return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/_components/ip-details/location-map-card.tsx
around lines 43 - 49, hasMeaningfulCoordinates() only checks finiteness and
non-(0,0) so invalid latitude values (e.g., 91 or -91) can reach Map and cause
MapLibre to throw; update the component that renders <Map> (the block using
viewport with center: [longitude, latitude] and zoom:
resolveZoom(accuracyRadiusKm)) to validate latitude is within -90..90 (and
optionally longitude within -180..180) before passing to Map; if out of range,
either clamp the values to the valid ranges or return/skip rendering the
Map/MapPopup to avoid initializing MapLibre with invalid LngLat — reference
hasMeaningfulCoordinates(), longitude, latitude, resolveZoom(), Map, and
MapPopup when applying the check.
| <MapPopup longitude={longitude} latitude={latitude} className="w-56"> | ||
| <div className="space-y-1"> | ||
| <p className="text-sm font-semibold">{title}</p> | ||
| {subtitle && <p className="text-xs text-muted-foreground">{subtitle}</p>} | ||
| <p className="font-mono text-xs"> | ||
| {latitude.toFixed(4)}, {longitude.toFixed(4)} | ||
| </p> | ||
| </div> | ||
| </MapPopup> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "location-map-card.tsx" -type fRepository: ding113/claude-code-hub
Length of output: 140
🏁 Script executed:
cat -n ./src/app/[locale]/dashboard/_components/ip-details/location-map-card.tsxRepository: ding113/claude-code-hub
Length of output: 2885
🏁 Script executed:
find . -path "*/components/ui/map*" -type f | head -20Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
cat -n ./src/components/ui/map.tsxRepository: ding113/claude-code-hub
Length of output: 50379
🏁 Script executed:
sed -n '903,1018p' ./src/components/ui/map.tsxRepository: ding113/claude-code-hub
Length of output: 3072
🌐 Web query:
MapLibre PopupOptions closeOnClick default value documentation
💡 Result:
The default value of closeOnClick in MapLibre GL JS PopupOptions is true. When set to true (the default), the popup closes when the map is clicked.
Citations:
- 1: https://www.maplibre.org/maplibre-gl-js/docs/API/type-aliases/PopupOptions/
- 2: https://github.com/maplibre/maplibre-gl-js/blob/1fe69fd961d62c9b017debfc7eb49c32c53e5339/src/ui/popup.ts
- 3: https://github.com/maplibre/maplibre-gl-js/blob/a54d3d666eaae1650a6e99bbabafdfe88e9f22e2/src/ui/popup.ts
固定展示的弹窗需要禁用点击地图后自动关闭。
MapPopup 这里没有可重新打开弹窗的 marker 或状态；MapLibre PopupOptions.closeOnClick 默认是 true，用户点击地图后弹窗会消失且无法恢复。
建议修复
- <MapPopup longitude={longitude} latitude={latitude} className="w-56">
+ <MapPopup
+ longitude={longitude}
+ latitude={latitude}
+ className="w-56"
+ closeOnClick={false}
+ >📝 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.
| <MapPopup longitude={longitude} latitude={latitude} className="w-56"> | |
| <div className="space-y-1"> | |
| <p className="text-sm font-semibold">{title}</p> | |
| {subtitle && <p className="text-xs text-muted-foreground">{subtitle}</p>} | |
| <p className="font-mono text-xs"> | |
| {latitude.toFixed(4)}, {longitude.toFixed(4)} | |
| </p> | |
| </div> | |
| </MapPopup> | |
| <MapPopup | |
| longitude={longitude} | |
| latitude={latitude} | |
| className="w-56" | |
| closeOnClick={false} | |
| > | |
| <div className="space-y-1"> | |
| <p className="text-sm font-semibold">{title}</p> | |
| {subtitle && <p className="text-xs text-muted-foreground">{subtitle}</p>} | |
| <p className="font-mono text-xs"> | |
| {latitude.toFixed(4)}, {longitude.toFixed(4)} | |
| </p> | |
| </div> | |
| </MapPopup> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/_components/ip-details/location-map-card.tsx
around lines 49 - 57, The Popup is closing when the user clicks the map because
MapLibre's PopupOptions.closeOnClick defaults to true; update the MapPopup usage
in location-map-card.tsx so the popup does not auto-close by passing the Popup
option to disable that behavior (e.g., pass closeOnClick: false via a prop such
as popupOptions={{ closeOnClick: false }} or closeOnClick={false} depending on
MapPopup's API), ensuring the popup remains visible after map clicks and still
provides a manual close or marker-based reopen mechanism if needed.
| test("MapControls drives map actions and locate never gets stuck without geolocation", async () => { | ||
| const labels = { | ||
| zoomIn: "放大地图", | ||
| zoomOut: "缩小地图", | ||
| locate: "定位到我的位置", | ||
| fullscreen: "切换全屏地图", | ||
| compass: "重置朝向", | ||
| }; | ||
| const geolocationDescriptor = Object.getOwnPropertyDescriptor(window.navigator, "geolocation"); | ||
| Object.defineProperty(window.navigator, "geolocation", { | ||
| configurable: true, | ||
| value: undefined, | ||
| }); | ||
|
|
||
| const { container, unmount } = render( | ||
| <div className="h-60 w-60"> | ||
| <Map viewport={{ center: [120, 30], zoom: 5 }}> | ||
| <MapControls showZoom showCompass showLocate showFullscreen labels={labels} /> | ||
| </Map> | ||
| </div> | ||
| ); | ||
|
|
||
| await flushMicrotasks(); | ||
|
|
||
| const map = maplibreMocks.maps.at(-1); | ||
| expect(map).toBeTruthy(); | ||
|
|
||
| const zoomInButton = container.querySelector( | ||
| `[aria-label="${labels.zoomIn}"]` | ||
| ) as HTMLButtonElement; | ||
| const zoomOutButton = container.querySelector( | ||
| `[aria-label="${labels.zoomOut}"]` | ||
| ) as HTMLButtonElement; | ||
| const locateButton = container.querySelector( | ||
| `[aria-label="${labels.locate}"]` | ||
| ) as HTMLButtonElement; | ||
| const fullscreenButton = container.querySelector( | ||
| `[aria-label="${labels.fullscreen}"]` | ||
| ) as HTMLButtonElement; | ||
| const compassButton = container.querySelector( | ||
| `[aria-label="${labels.compass}"]` | ||
| ) as HTMLButtonElement; | ||
|
|
||
| click(zoomInButton); | ||
| click(zoomOutButton); | ||
| click(compassButton); | ||
| click(fullscreenButton); | ||
| click(locateButton); | ||
|
|
||
| expect(map?.zoomTo).toHaveBeenCalledTimes(2); | ||
| expect(map?.resetNorthPitch).toHaveBeenCalledTimes(1); | ||
| expect(map?.container.requestFullscreen).toHaveBeenCalledTimes(1); | ||
| expect(locateButton.disabled).toBe(false); | ||
|
|
||
| if (geolocationDescriptor) { | ||
| Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor); | ||
| } else { | ||
| Object.defineProperty(window.navigator, "geolocation", { | ||
| configurable: true, | ||
| value: undefined, | ||
| }); | ||
| } | ||
|
|
||
| unmount(); |
There was a problem hiding this comment.
请确保 geolocation 和组件卸载在失败路径也能恢复。
这里如果 geolocation 原本来自原型链，测试结束后会留下一个 own geolocation: undefined，后续同 worker 的测试会继续看不到原始实现；另外任一断言提前失败时也会跳过恢复和 unmount()。建议用 try/finally，并在原本没有 own property 时删除覆盖值。
建议调整
const labels = {
zoomIn: "放大地图",
zoomOut: "缩小地图",
locate: "定位到我的位置",
fullscreen: "切换全屏地图",
compass: "重置朝向",
};
const geolocationDescriptor = Object.getOwnPropertyDescriptor(window.navigator, "geolocation");
+ const hadOwnGeolocation = Object.prototype.hasOwnProperty.call(
+ window.navigator,
+ "geolocation"
+ );
Object.defineProperty(window.navigator, "geolocation", {
configurable: true,
value: undefined,
});
const { container, unmount } = render(
<div className="h-60 w-60">
<Map viewport={{ center: [120, 30], zoom: 5 }}>
<MapControls showZoom showCompass showLocate showFullscreen labels={labels} />
</Map>
</div>
);
- await flushMicrotasks();
+ try {
+ await flushMicrotasks();
- const map = maplibreMocks.maps.at(-1);
- expect(map).toBeTruthy();
+ const map = maplibreMocks.maps.at(-1);
+ expect(map).toBeTruthy();
- const zoomInButton = container.querySelector(
- `[aria-label="${labels.zoomIn}"]`
- ) as HTMLButtonElement;
+ const zoomInButton = container.querySelector(
+ `[aria-label="${labels.zoomIn}"]`
+ ) as HTMLButtonElement;
- click(zoomInButton);
- click(zoomOutButton);
- click(compassButton);
- click(fullscreenButton);
- click(locateButton);
+ click(zoomInButton);
+ click(zoomOutButton);
+ click(compassButton);
+ click(fullscreenButton);
+ click(locateButton);
- expect(map?.zoomTo).toHaveBeenCalledTimes(2);
- expect(map?.resetNorthPitch).toHaveBeenCalledTimes(1);
- expect(map?.container.requestFullscreen).toHaveBeenCalledTimes(1);
- expect(locateButton.disabled).toBe(false);
-
- if (geolocationDescriptor) {
- Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor);
- } else {
- Object.defineProperty(window.navigator, "geolocation", {
- configurable: true,
- value: undefined,
- });
+ expect(map?.zoomTo).toHaveBeenCalledTimes(2);
+ expect(map?.resetNorthPitch).toHaveBeenCalledTimes(1);
+ expect(map?.container.requestFullscreen).toHaveBeenCalledTimes(1);
+ expect(locateButton.disabled).toBe(false);
+ } finally {
+ if (geolocationDescriptor) {
+ Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor);
+ } else if (!hadOwnGeolocation) {
+ delete (window.navigator as Navigator & { geolocation?: Geolocation }).geolocation;
+ }
+ unmount();
}
-
- unmount();📝 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.
| test("MapControls drives map actions and locate never gets stuck without geolocation", async () => { | |
| const labels = { | |
| zoomIn: "放大地图", | |
| zoomOut: "缩小地图", | |
| locate: "定位到我的位置", | |
| fullscreen: "切换全屏地图", | |
| compass: "重置朝向", | |
| }; | |
| const geolocationDescriptor = Object.getOwnPropertyDescriptor(window.navigator, "geolocation"); | |
| Object.defineProperty(window.navigator, "geolocation", { | |
| configurable: true, | |
| value: undefined, | |
| }); | |
| const { container, unmount } = render( | |
| <div className="h-60 w-60"> | |
| <Map viewport={{ center: [120, 30], zoom: 5 }}> | |
| <MapControls showZoom showCompass showLocate showFullscreen labels={labels} /> | |
| </Map> | |
| </div> | |
| ); | |
| await flushMicrotasks(); | |
| const map = maplibreMocks.maps.at(-1); | |
| expect(map).toBeTruthy(); | |
| const zoomInButton = container.querySelector( | |
| `[aria-label="${labels.zoomIn}"]` | |
| ) as HTMLButtonElement; | |
| const zoomOutButton = container.querySelector( | |
| `[aria-label="${labels.zoomOut}"]` | |
| ) as HTMLButtonElement; | |
| const locateButton = container.querySelector( | |
| `[aria-label="${labels.locate}"]` | |
| ) as HTMLButtonElement; | |
| const fullscreenButton = container.querySelector( | |
| `[aria-label="${labels.fullscreen}"]` | |
| ) as HTMLButtonElement; | |
| const compassButton = container.querySelector( | |
| `[aria-label="${labels.compass}"]` | |
| ) as HTMLButtonElement; | |
| click(zoomInButton); | |
| click(zoomOutButton); | |
| click(compassButton); | |
| click(fullscreenButton); | |
| click(locateButton); | |
| expect(map?.zoomTo).toHaveBeenCalledTimes(2); | |
| expect(map?.resetNorthPitch).toHaveBeenCalledTimes(1); | |
| expect(map?.container.requestFullscreen).toHaveBeenCalledTimes(1); | |
| expect(locateButton.disabled).toBe(false); | |
| if (geolocationDescriptor) { | |
| Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor); | |
| } else { | |
| Object.defineProperty(window.navigator, "geolocation", { | |
| configurable: true, | |
| value: undefined, | |
| }); | |
| } | |
| unmount(); | |
| test("MapControls drives map actions and locate never gets stuck without geolocation", async () => { | |
| const labels = { | |
| zoomIn: "放大地图", | |
| zoomOut: "缩小地图", | |
| locate: "定位到我的位置", | |
| fullscreen: "切换全屏地图", | |
| compass: "重置朝向", | |
| }; | |
| const geolocationDescriptor = Object.getOwnPropertyDescriptor(window.navigator, "geolocation"); | |
| const hadOwnGeolocation = Object.prototype.hasOwnProperty.call( | |
| window.navigator, | |
| "geolocation" | |
| ); | |
| Object.defineProperty(window.navigator, "geolocation", { | |
| configurable: true, | |
| value: undefined, | |
| }); | |
| const { container, unmount } = render( | |
| <div className="h-60 w-60"> | |
| <Map viewport={{ center: [120, 30], zoom: 5 }}> | |
| <MapControls showZoom showCompass showLocate showFullscreen labels={labels} /> | |
| </Map> | |
| </div> | |
| ); | |
| try { | |
| await flushMicrotasks(); | |
| const map = maplibreMocks.maps.at(-1); | |
| expect(map).toBeTruthy(); | |
| const zoomInButton = container.querySelector( | |
| `[aria-label="${labels.zoomIn}"]` | |
| ) as HTMLButtonElement; | |
| const zoomOutButton = container.querySelector( | |
| `[aria-label="${labels.zoomOut}"]` | |
| ) as HTMLButtonElement; | |
| const locateButton = container.querySelector( | |
| `[aria-label="${labels.locate}"]` | |
| ) as HTMLButtonElement; | |
| const fullscreenButton = container.querySelector( | |
| `[aria-label="${labels.fullscreen}"]` | |
| ) as HTMLButtonElement; | |
| const compassButton = container.querySelector( | |
| `[aria-label="${labels.compass}"]` | |
| ) as HTMLButtonElement; | |
| click(zoomInButton); | |
| click(zoomOutButton); | |
| click(compassButton); | |
| click(fullscreenButton); | |
| click(locateButton); | |
| expect(map?.zoomTo).toHaveBeenCalledTimes(2); | |
| expect(map?.resetNorthPitch).toHaveBeenCalledTimes(1); | |
| expect(map?.container.requestFullscreen).toHaveBeenCalledTimes(1); | |
| expect(locateButton.disabled).toBe(false); | |
| } finally { | |
| if (geolocationDescriptor) { | |
| Object.defineProperty(window.navigator, "geolocation", geolocationDescriptor); | |
| } else if (!hadOwnGeolocation) { | |
| delete (window.navigator as Navigator & { geolocation?: Geolocation }).geolocation; | |
| } | |
| unmount(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/__tests__/map.test.tsx` around lines 419 - 482, The test
replaces window.navigator.geolocation and calls unmount() but doesn’t guarantee
restoration or unmount on assertion failures; wrap the body of the test that
mutates geolocation and renders the component in a try/finally so you always
call unmount() and restore the original geolocation descriptor: capture the
original descriptor in geolocationDescriptor, set the stubbed value, then in
finally call unmount() if defined and if geolocationDescriptor exists restore it
with Object.defineProperty(window.navigator, "geolocation",
geolocationDescriptor) else delete the own property (or redefine configurable
undefined only when necessary) so the original prototype property is visible
again; update the test around Map/MapControls, geolocationDescriptor, and
unmount to use this try/finally pattern.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65c0f741b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if (canSkipDsnWarmup) { | ||
| logger.info("[App] SensitiveWordDetector warmup skipped: DSN not configured"); | ||
| } else { | ||
| throw new Error("[App] DSN is required for SensitiveWordDetector warmup"); |
There was a problem hiding this comment.
This branch throws during module initialization whenever DSN is missing outside test/build, so in NODE_ENV=development any request hitting /v1/* fails before the Hono app is even created. That is a behavior regression from the prior log-and-continue path and blocks common local workflows that run UI/proxy code without a database; development should skip warmup rather than hard-fail at import time.
Useful? React with 👍 / 👎.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Regroup the IP details location section so geographic fields (country, region, city, coordinates) stay expanded while country metadata and timezone info collapse into a single default-closed group. Add an interactive map panel powered by maplibre-gl (via the mapcn registry) for IPs with valid coordinates. Fix health checks and proxy startup in DSN-less test environments.
Problem
After the IP details dialog redesign in #1031, the location section had two issues:
hasMeaningfulCoordinatesrejected any IP whereaccuracy_radius_kmwasnull, even when the upstream returned real non-zero lat/lng values (e.g., Hong Kong at 22.28, 114.15). This hid valid coordinates and prevented map display for many IPs.Additionally, the health checker and proxy startup failed in test environments without a configured DSN.
Follow-up to: #1031 (IP details dialog redesign)
Builds on: #1027 (IP geo lookup foundation)
Solution
hasMeaningfulCoordinatesto accept any finite, non-(0,0) coordinate pair regardless ofaccuracy_radius_kmstatus. The accuracy field is now shown independently when present.LocationMapCardcomponent using MapLibre GL JS (maplibre-glv5.23+), sourced from the mapcn shadcn registry (@mapcn). Shows a dark/light theme-aware map with zoom controls and a popup showing location name and coordinates. Zoom level adapts toaccuracy_radius_kmwhen available.uncheckedstatus whenDSNis not configured (instead of erroring). Proxy startup skips sensitive-word detector warmup whenDSNis absent.Changes
Core Changes
atoms.tsx: RelaxedhasMeaningfulCoordinates-- accepts any finite non-(0,0) coordinates regardless ofaccuracy_radius_kmlocation-section.tsx: Restructured layout -- geo fields expanded by default; country metadata + timezone collapsed into a single "Country & timezone" sub-card; addedLocationMapCardintegrationlocation-map-card.tsx(new): MapLibre GL map card component with adaptive zoom, themed popup, and controlsmap.tsx(new, 1426 lines): Reusable MapLibre GL wrapper (Map,MapMarker,MapPopup,MapControls,MapRoute) sourced from@mapcnregistry with dark/light theme auto-detectionSupporting Changes
components.json: Added@mapcnregistry entry for map componentspackage.json: Addedmaplibre-gl@^5.23.0; upgradedlucide-reactto^1.8.0globals.css: Added MapLibre popup/tip style overrides for transparent popupschecker.ts: Returnuncheckedstatus when DSN is not configuredroute.ts: Guard sensitive-word warmup behind DSN checki18n (5 languages)
sections.countryTimezonekey across en, ja, ru, zh-CN, zh-TWTests
ip-details-dialog.test.tsx: Added 157 lines -- tests for null-accuracy coordinate display, map rendering, country/timezone collapse behavior, unknown-country handlinghealth-checker.test.ts: Added test foruncheckedstatus when DSN is absentTesting
bun run lint/lint:fix/typecheck/test/buildall passDescription enhanced by Claude AI
Greptile Summary
This PR regroups the IP details location section so geographic fields stay expanded while country metadata and timezone info collapse into a single "Country & timezone" sub-card, adds an interactive MapLibre GL map panel for IPs with valid coordinates, and fixes DSN-less test/build environments for health checks and proxy startup.
The coordinate fix (
hasMeaningfulCoordinatesno longer requiringaccuracy_radius_km !== null) is well-justified and all five i18n locales correctly include the newsections.countryTimezonekey.Confidence Score: 5/5
Safe to merge — no P0/P1 issues found; all logic changes are well-tested and intentional.
All remaining findings are P2 or lower. The coordinate validation fix is correct and well-covered by tests. The health checker and route DSN guards behave intentionally across test/build/production modes. The prior review's concerns (mapSubtitle deduplication, checkReadiness DSN-unchecked integration test) are addressed in this iteration. The MapLibre map component is a large addition but thoroughly tested with a FakeMap harness.
No files require special attention.
Important Files Changed
hasMeaningfulCoordinatesto accept any finite non-(0,0) pair regardless ofaccuracy_radius_km— clean, well-tested change.LocationMapCard, collapsed country/timezone metadata by default; mapSubtitle deduplication viaSet+!== mapTitlefilter correctly handles same-name city/region/country cases.MapPopup+MapControlsinsideMap; adaptive zoom logic is straightforward and correct.uncheckedfor missing DSN in test mode (not production), keeping overall healthhealthyin CI environments; well-covered by the new test.sensitiveWordDetector.reload()behind DSN presence; hard-throws at module level for non-test, non-build production deployments missing DSN — intentional fail-fast behavior.Sequence Diagram
sequenceDiagram participant Dialog as IpDetailsDialog participant LocSec as LocationSection participant Atoms as hasMeaningfulCoordinates participant MapCard as LocationMapCard participant Map as Map (MapLibre) participant Carto as Carto Tile Server Dialog->>LocSec: render(result) LocSec->>Atoms: hasMeaningfulCoordinates(coords) Note over Atoms: finite & non-(0,0) — accuracy_radius_km no longer required Atoms-->>LocSec: true/false alt coords valid LocSec->>MapCard: render(lat, lng, accuracy, title, subtitle) MapCard->>Map: viewport={center, zoom=resolveZoom(accuracy)} Map->>Carto: fetch tile style JSON Carto-->>Map: dark/light style Map-->>MapCard: map loaded MapCard->>Map: MapPopup(lat,lng) always-open MapCard->>Map: MapControls(zoomIn/Out) end LocSec-->>Dialog: geo fields + map + collapsed country/tz SubCardReviews (4): Last reviewed commit: "fix(review): cover map behaviors directl..." | Re-trigger Greptile