Implement authored continent sector maps#1377
Open
MathiasGruber wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughIntroduces a full sector-map authoring system: new ChangesSector Map System
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin Browser
participant SectorMapEditor
participant worldMapRouter
participant normalizeTiledSectorMap
participant DB
Admin->>SectorMapEditor: Upload .json file + set sector/region/name
Admin->>SectorMapEditor: Click "Preview"
SectorMapEditor->>worldMapRouter: previewTiledSectorMap(tiledJson, sector, region)
worldMapRouter->>normalizeTiledSectorMap: validate + normalize
normalizeTiledSectorMap-->>worldMapRouter: map / errors / warnings
worldMapRouter-->>SectorMapEditor: validation summary
Admin->>SectorMapEditor: Click "Publish"
SectorMapEditor->>worldMapRouter: saveTiledSectorMap(tiledJson, sector, publish=true)
worldMapRouter->>DB: BEGIN TRANSACTION
worldMapRouter->>DB: UPDATE existing PUBLISHED → ARCHIVED
worldMapRouter->>DB: INSERT new SectorMap (status=PUBLISHED)
DB-->>worldMapRouter: inserted record
worldMapRouter-->>SectorMapEditor: success + normalized map
SectorMapEditor->>worldMapRouter: invalidate listSectorMaps + getSectorMap
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
size:XXL
MathiasGruber
force-pushed
the
new-tiled-map-system
branch
from
506ca10 to
e44b42c
Compare
Contributor
Confidence Score: 2/5The admin save and publish mutations will throw at runtime because PlanetScale rejects transaction calls; these paths are blocked until rewritten. The saveTiledSectorMap and publishSectorMap mutations both call ctx.drizzle.transaction(), which the hosting database (PlanetScale/Vitess) does not support. Any attempt by an admin to save or publish a sector map will result in a runtime exception. The travel moveInSector mutation also adds a sequential DB fetch ahead of its existing Promise.all, adding latency to the most frequent game endpoint. The rest of the change — schema, migration, Tiled normalizer, validation helpers, UI — is well-structured and correct. app/src/server/api/routers/worldMap.ts requires the transaction calls to be replaced with sequential CAS-guarded writes before merging; app/src/server/api/routers/travel.ts needs the sectorMap fetch moved into the parallel batch. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant TravelRouter
participant WorldMapRouter
participant SectorMapUtil
participant DB
Note over Client,DB: moveInSector (hot path)
Client->>TravelRouter: moveInSector(sector, longitude, latitude)
TravelRouter->>SectorMapUtil: fetchPublishedSectorMap(sector)
SectorMapUtil->>DB: "SELECT sectorMap WHERE sector=? AND status=PUBLISHED"
DB-->>SectorMapUtil: NormalizedSectorMap (or legacy fallback)
SectorMapUtil-->>TravelRouter: sectorMap
TravelRouter->>TravelRouter: validateWalkable(cur) + validateWalkable(target)
TravelRouter->>TravelRouter: BFS isReachable(cur to target)
TravelRouter->>DB: Promise.all([fetchUser, UPDATE userData, fetchSectorVillage])
DB-->>TravelRouter: results
TravelRouter-->>Client: move result
Note over Client,DB: saveTiledSectorMap (admin)
Client->>WorldMapRouter: saveTiledSectorMap(sector, tiledJson, publish)
WorldMapRouter->>DB: ensureWorldRegion (findFirst + insert)
WorldMapRouter->>DB: getNextSectorMapVersion (findFirst)
WorldMapRouter->>DB: transaction.begin - PlanetScale unsupported
WorldMapRouter->>DB: "UPDATE sectorMap SET status=ARCHIVED"
WorldMapRouter->>DB: INSERT sectorMap
WorldMapRouter->>DB: transaction.commit - throws
WorldMapRouter-->>Client: runtime error
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant TravelRouter
participant WorldMapRouter
participant SectorMapUtil
participant DB
Note over Client,DB: moveInSector (hot path)
Client->>TravelRouter: moveInSector(sector, longitude, latitude)
TravelRouter->>SectorMapUtil: fetchPublishedSectorMap(sector)
SectorMapUtil->>DB: "SELECT sectorMap WHERE sector=? AND status=PUBLISHED"
DB-->>SectorMapUtil: NormalizedSectorMap (or legacy fallback)
SectorMapUtil-->>TravelRouter: sectorMap
TravelRouter->>TravelRouter: validateWalkable(cur) + validateWalkable(target)
TravelRouter->>TravelRouter: BFS isReachable(cur to target)
TravelRouter->>DB: Promise.all([fetchUser, UPDATE userData, fetchSectorVillage])
DB-->>TravelRouter: results
TravelRouter-->>Client: move result
Note over Client,DB: saveTiledSectorMap (admin)
Client->>WorldMapRouter: saveTiledSectorMap(sector, tiledJson, publish)
WorldMapRouter->>DB: ensureWorldRegion (findFirst + insert)
WorldMapRouter->>DB: getNextSectorMapVersion (findFirst)
WorldMapRouter->>DB: transaction.begin - PlanetScale unsupported
WorldMapRouter->>DB: "UPDATE sectorMap SET status=ARCHIVED"
WorldMapRouter->>DB: INSERT sectorMap
WorldMapRouter->>DB: transaction.commit - throws
WorldMapRouter-->>Client: runtime error
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+281
to
+316
|
|
||
| const normalizedMap = result.map; | ||
| const status: SectorMapStatus = input.publish ? "PUBLISHED" : "DRAFT"; | ||
| const id = nanoid(); | ||
| const now = new Date(); | ||
|
|
||
| await ctx.drizzle.transaction(async (tx) => { | ||
| const version = await getNextSectorMapVersion(tx, input.sector); | ||
| if (input.publish) { | ||
| await tx | ||
| .update(sectorMap) | ||
| .set({ status: "ARCHIVED", updatedAt: now }) | ||
| .where( | ||
| and( | ||
| eq(sectorMap.sector, input.sector), | ||
| eq(sectorMap.status, "PUBLISHED"), | ||
| ), | ||
| ); | ||
| } | ||
| await tx.insert(sectorMap).values({ | ||
| id, | ||
| sector: input.sector, | ||
| regionId: dbRegion.id, | ||
| name: input.name, | ||
| width: normalizedMap.width, | ||
| height: normalizedMap.height, | ||
| status, | ||
| version, | ||
| sourceHash, | ||
| rawTiledJson: input.tiledJson, | ||
| normalizedJson: normalizedMap, | ||
| publishedAt: input.publish ? now : null, | ||
| publishedByUserId: input.publish ? ctx.userId : null, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }); |
Contributor
There was a problem hiding this comment.
Both saveTiledSectorMap and publishSectorMap wrap their multi-step writes in ctx.drizzle.transaction(...). The CLAUDE.md explicitly forbids this: "NEVER use database transactions — PlanetScale does not support traditional transactions." These calls will throw at runtime.
For saveTiledSectorMap, the archive-then-insert sequence should be re-expressed without a transaction: archive existing PUBLISHED rows first using a WHERE predicate, then insert the new record. The getNextSectorMapVersion query inside the transaction also creates a version-number race condition — two concurrent saves for the same sector can both read the same latest.version, yield the same next version, and then one insert will violate the SectorMap_sector_version_key UNIQUE constraint. Consider deriving the version atomically via a MAX(version) + 1 sub-select in the INSERT, or using an auto-increment surrogate keyed on sector.
publishSectorMap has the same transaction issue; its archive + status-update pair should use a CAS WHERE status = 'PUBLISHED' guard on the archive step (which already exists) executed sequentially without a transaction wrapper.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/worldMap.ts
Line: 281-316
Comment:
**PlanetScale does not support database transactions**
Both `saveTiledSectorMap` and `publishSectorMap` wrap their multi-step writes in `ctx.drizzle.transaction(...)`. The `CLAUDE.md` explicitly forbids this: *"NEVER use database transactions — PlanetScale does not support traditional transactions."* These calls will throw at runtime.
For `saveTiledSectorMap`, the archive-then-insert sequence should be re-expressed without a transaction: archive existing PUBLISHED rows first using a WHERE predicate, then insert the new record. The `getNextSectorMapVersion` query inside the transaction also creates a version-number race condition — two concurrent saves for the same sector can both read the same `latest.version`, yield the same next version, and then one insert will violate the `SectorMap_sector_version_key` UNIQUE constraint. Consider deriving the version atomically via a `MAX(version) + 1` sub-select in the INSERT, or using an auto-increment surrogate keyed on sector.
`publishSectorMap` has the same transaction issue; its archive + status-update pair should use a CAS `WHERE status = 'PUBLISHED'` guard on the archive step (which already exists) executed sequentially without a transaction wrapper.
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/studietech-aps-org-2/github/studie-tech/theninjarpg/-/custom-context?memory=0827434e-cdb7-4415-b790-c54f1a8e8feb))
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+26
to
+28
| const regionSlugSchema = z.enum(WorldRegionSlugs); | ||
| const sectorSchema = z.number().int().min(MAP_SECTOR_ID_MIN).max(MAP_SECTOR_ID_MAX); | ||
| const sectorMapStatusSchema = z.enum(SectorMapStatuses); |
Contributor
There was a problem hiding this comment.
sectorSchema is defined inline here but sectorIdSchema in app/src/validators/travel.ts already covers the same range with the same constants. CLAUDE.md requires all Zod schemas to live in @/validators/, not inside router files. Using the shared schema also removes the duplication risk if MAP_SECTOR_ID_MIN/MAX bounds drift.
Suggested change
| const regionSlugSchema = z.enum(WorldRegionSlugs); | |
| const sectorSchema = z.number().int().min(MAP_SECTOR_ID_MIN).max(MAP_SECTOR_ID_MAX); | |
| const sectorMapStatusSchema = z.enum(SectorMapStatuses); | |
| import { sectorIdSchema } from "@/validators/travel"; | |
| const regionSlugSchema = z.enum(WorldRegionSlugs); | |
| const sectorSchema = sectorIdSchema; | |
| const sectorMapStatusSchema = z.enum(SectorMapStatuses); |
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/worldMap.ts
Line: 26-28
Comment:
`sectorSchema` is defined inline here but `sectorIdSchema` in `app/src/validators/travel.ts` already covers the same range with the same constants. `CLAUDE.md` requires all Zod schemas to live in `@/validators/`, not inside router files. Using the shared schema also removes the duplication risk if `MAP_SECTOR_ID_MIN/MAX` bounds drift.
```suggestion
import { sectorIdSchema } from "@/validators/travel";
const regionSlugSchema = z.enum(WorldRegionSlugs);
const sectorSchema = sectorIdSchema;
const sectorMapStatusSchema = z.enum(SectorMapStatuses);
```
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/studietech-aps-org-2/github/studie-tech/theninjarpg/-/custom-context?memory=0827434e-cdb7-4415-b790-c54f1a8e8feb))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+67
to
+97
| to: SectorCoordinate, | ||
| ) => { | ||
| if (!isWalkableCoordinate(map, from) || !isWalkableCoordinate(map, to)) return false; | ||
| if (from.x === to.x && from.y === to.y) return true; | ||
|
|
||
| const visited = new Set<string>(); | ||
| const queue: SectorCoordinate[] = [from]; | ||
| visited.add(getSectorTileKey(from.x, from.y)); | ||
|
|
||
| while (queue.length > 0) { | ||
| const current = queue.shift(); | ||
| if (!current) continue; | ||
|
|
||
| for (const neighbor of getNeighborCoordinates(current)) { | ||
| const key = getSectorTileKey(neighbor.x, neighbor.y); | ||
| if (visited.has(key) || !isWalkableCoordinate(map, neighbor)) continue; | ||
| if (neighbor.x === to.x && neighbor.y === to.y) return true; | ||
| visited.add(key); | ||
| queue.push(neighbor); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }; | ||
|
|
||
| export const resolveSectorAnchor = ( | ||
| map: Pick<NormalizedSectorMap, "anchors">, | ||
| key: string, | ||
| ) => { | ||
| return map.anchors.find((anchor) => anchor.key === key); | ||
| }; |
Contributor
There was a problem hiding this comment.
queue.shift() on the move hot-path
isReachableCoordinate is called on every moveInSector request. Array.shift() is O(n), so for a 64×64 map (4 096 tiles) worst-case cost is O(n²) ≈ 16 M operations per request. For the default 20×26 map this is only ~270 K operations, but maps can be up to the 64×64 limit. Consider replacing the SectorCoordinate[] queue with a simple index pointer (let head = 0; const current = queue[head++]) to make dequeue O(1), or use a typed Uint16Array ring buffer. This avoids quadratic degradation as authored maps grow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/libs/sector-map/validation.ts
Line: 67-97
Comment:
**BFS with O(n) `queue.shift()` on the move hot-path**
`isReachableCoordinate` is called on every `moveInSector` request. `Array.shift()` is O(n), so for a 64×64 map (4 096 tiles) worst-case cost is O(n²) ≈ 16 M operations per request. For the default 20×26 map this is only ~270 K operations, but maps can be up to the 64×64 limit. Consider replacing the `SectorCoordinate[]` queue with a simple index pointer (`let head = 0; const current = queue[head++]`) to make dequeue O(1), or use a typed `Uint16Array` ring buffer. This avoids quadratic degradation as authored maps grow.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/layout/Sector.tsx (1)
625-661:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not require a custom avatar before moving.
Line 627 blocks
moveInSectorfor users whose nullableavataris unset, even though the mutation payload already falls back toIMG_AVATAR_DEFAULTon Lines 652-653. This prevents default-avatar users from moving.Suggested fix
- if (target && originRef.current && pathFinderRef.current && userData?.avatar) { + if (target && originRef.current && pathFinderRef.current && userData) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/layout/Sector.tsx` around lines 625 - 661, The useEffect hook in the movement logic unnecessarily requires userData?.avatar to be set before allowing movement, but the move function already provides fallback values to IMG_AVATAR_DEFAULT on lines 652-653. Remove the userData?.avatar check from the condition at line 625 (the if statement checking target, originRef.current, pathFinderRef.current, and userData?.avatar) to allow users without custom avatars to move. Keep all other guards in place, and rely on the move function's built-in fallback logic to handle cases where avatar or avatarLight are not defined.
🧹 Nitpick comments (5)
app/drizzle/seeds/worldRegion.ts (1)
8-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse atomic upsert instead of read-then-write in region seeding.
The
findFirst→insert/updateflow is race-prone under concurrent seed execution and does extra roundtrips. A single upsert statement avoids duplicate-key races and is simpler.♻️ Proposed refactor
-import { eq } from "drizzle-orm"; import { worldRegion } from "`@/drizzle/schema`"; import { WORLD_REGION_DEFINITIONS } from "`@/libs/sector-map/regions`"; import type { DrizzleClient } from "`@/server/db`"; export const seedWorldRegions = async (client: DrizzleClient) => { console.log("\nSyncing world regions..."); for (const region of WORLD_REGION_DEFINITIONS) { - const existing = await client.query.worldRegion.findFirst({ - where: eq(worldRegion.slug, region.slug), - }); const values = { id: region.slug, slug: region.slug, name: region.name, sortOrder: region.sortOrder, description: region.description, isActive: true, }; - if (existing) { - await client - .update(worldRegion) - .set(values) - .where(eq(worldRegion.slug, region.slug)); - } else { - await client.insert(worldRegion).values(values); - } + await client + .insert(worldRegion) + .values(values) + .onDuplicateKeyUpdate({ + set: { + name: values.name, + sortOrder: values.sortOrder, + description: values.description, + isActive: values.isActive, + }, + }); } console.log("Done syncing world regions!"); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/drizzle/seeds/worldRegion.ts` around lines 8 - 27, The current implementation in the region seeding loop uses a read-then-write pattern with findFirst followed by conditional update or insert operations, which creates race conditions under concurrent execution and requires multiple database roundtrips. Replace this pattern with a single atomic upsert operation on the worldRegion table that handles both insert and update in one statement. This eliminates the separate findFirst query and the conditional logic, using Drizzle's onConflictDoUpdate functionality to update existing records by slug or insert new ones if they don't exist.app/src/layout/WorldRegionMap.tsx (1)
8-49: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the project component structure and a named export.
Move
WorldRegionMapaboveRegionIcon/helpers/types and export it as a named component; then update imports toimport { WorldRegionMap } from "@/layout/WorldRegionMap". As per coding guidelines,**/*.tsx: “Component file structure: exported component → subcomponents → helpers → types” and**/*/*.tsx: “Prefer named exports for components.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/layout/WorldRegionMap.tsx` around lines 8 - 49, The file structure violates the project's component organization guidelines which require: exported component → subcomponents → helpers → types. Reorganize the WorldRegionMap.tsx file by moving the WorldRegionMap function declaration to appear before the RegionIcon subcomponent and the constant definitions (regionTone, regionLayout, etc.). Change the export from default export to a named export by replacing "export default function WorldRegionMap" with "export function WorldRegionMap". Then update all import statements throughout the project that import this component to use the named import syntax "import { WorldRegionMap }" instead of the default import pattern.Source: Coding guidelines
app/src/libs/sector-map/validation.ts (1)
36-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid repeated linear tile scans during BFS reachability checks.
Line 40 does a linear lookup, and Lines 80-83 trigger it for each neighbor expansion. With max-size maps, this becomes an unnecessary CPU hotspot in traversal validation.
⚡ Proposed optimization
export const isReachableCoordinate = ( map: Pick<NormalizedSectorMap, "width" | "height" | "tiles">, from: SectorCoordinate, to: SectorCoordinate, ) => { - if (!isWalkableCoordinate(map, from) || !isWalkableCoordinate(map, to)) return false; + const tileByKey = new Map( + map.tiles.map((tile) => [getSectorTileKey(tile.x, tile.y), tile] as const), + ); + const isWalkable = (coordinate: SectorCoordinate) => { + if (!isCoordinateInSectorMap(map, coordinate)) return false; + const tile = tileByKey.get(getSectorTileKey(coordinate.x, coordinate.y)); + return !!tile && !tile.blocked && tile.walkCost > 0; + }; + + if (!isWalkable(from) || !isWalkable(to)) return false; if (from.x === to.x && from.y === to.y) return true; const visited = new Set<string>(); const queue: SectorCoordinate[] = [from]; visited.add(getSectorTileKey(from.x, from.y)); - while (queue.length > 0) { - const current = queue.shift(); - if (!current) continue; + for (let index = 0; index < queue.length; index += 1) { + const current = queue[index]!; for (const neighbor of getNeighborCoordinates(current)) { const key = getSectorTileKey(neighbor.x, neighbor.y); - if (visited.has(key) || !isWalkableCoordinate(map, neighbor)) continue; + if (visited.has(key) || !isWalkable(neighbor)) continue; if (neighbor.x === to.x && neighbor.y === to.y) return true; visited.add(key); queue.push(neighbor); } }Also applies to: 64-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/libs/sector-map/validation.ts` around lines 36 - 40, The getSectorTile function uses a linear Array.find() lookup on the tiles array, which is called repeatedly during BFS neighbor expansion checks in the reachability validation (lines 80-83 and broader context 64-86). Instead of doing multiple linear scans for each neighbor, create an efficient lookup structure (such as a Map keyed by coordinate strings or a 2D grid structure) before the BFS traversal begins. Pre-process the tiles into this lookup structure once, then use it for O(1) coordinate-based tile lookups throughout the neighbor expansion loop to eliminate the repeated linear scans.app/src/server/api/routers/worldMap.ts (1)
134-167: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPush optional filters into the DB query instead of filtering in memory.
Line 134 currently loads all rows and Line 162 filters in JS. Moving
sector/status/regionSlugpredicates intofindMany.wherewill reduce query payload and memory growth as version history expands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/server/api/routers/worldMap.ts` around lines 134 - 167, Move the three filter conditions currently applied in the JavaScript filter function (checking input?.sector, input?.status, and input?.regionSlug) into the where clause of the findMany query on the sectorMap table. Instead of fetching all maps and filtering in memory, construct appropriate where predicates using Drizzle's query builder to filter at the database level, which will reduce the payload size and improve performance as the data grows.app/src/app/manual/travel/sector-maps/page.tsx (1)
227-247: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer shared Shadcn/Radix form primitives for consistency.
These ranges introduce native
<select>/<textarea>controls. In this repo, prefer shared Shadcn/Radix components for consistent styling, behavior, and accessibility across pages.As per coding guidelines: "Use Shadcn UI and Radix components for UI development."
Also applies to: 261-271, 357-368
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/app/manual/travel/sector-maps/page.tsx` around lines 227 - 247, The native HTML select element with id="sector-map-region" should be replaced with a Shadcn/Radix Select component to maintain consistency with the codebase's UI component standards. Replace the select element and its onChange handler (which calls setRegionSlug with the event target value) with the corresponding Shadcn Select component, ensuring the same functionality is preserved for the regionSlug state management. Apply this same change to the other native select/textarea controls mentioned in the comment at lines 261-271 and 357-368.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/app/manual/travel/page.tsx`:
- Around line 24-32: The topRightContent section has a Button component nested
inside a Link component, which creates invalid nested interactive elements. To
fix this, add the asChild prop to the Button component to allow it to render the
Link as its child without creating nested interactive elements. This way, the
Link will be the actual interactive element while maintaining the Button's
styling.
In `@app/src/app/manual/travel/sector-maps/page.tsx`:
- Around line 93-95: The sortedRegions useMemo hook is directly calling .sort()
on the regions array, which mutates the original query cache data. To fix this,
create a non-mutating copy of the regions array before sorting by either using
the spread operator to copy the array first (like [...regions].sort(...)) or by
using the toSorted() method (regions?.toSorted(...)) which returns a new sorted
array without mutating the original. This ensures the query cache data remains
immutable.
- Around line 213-218: Replace the hardcoded sector bounds (0 and 491) in the
min and max attributes of the input field with the shared constants
MAP_SECTOR_ID_MIN and MAP_SECTOR_ID_MAX imported from `@/drizzle/constants.ts`.
Additionally, in the onChange handler, sanitize the numeric input by clamping
the nextSector value between these min and max bounds before passing it to
setSector to prevent invalid values from being stored.
In `@app/src/app/travel/page.tsx`:
- Around line 410-417: The issue is that showGlobal on line 415 does not require
currentSectorMap to be loaded, but when it's undefined, the isAtEdge function on
line 413 receives undefined dimensions and falls back to legacy sector
dimensions, causing incorrect edge routing on custom-sized maps. Add a check for
currentSectorMap in the showGlobal condition (similar to the showSector check on
line 416-417) to ensure actual sector dimensions are available before enabling
global travel UI, preventing isAtEdge from falling back to legacy dimensions
when currentSectorDimensions is undefined.
In `@app/src/layout/Sector.tsx`:
- Around line 161-174: The code is mutating the `structures` array (which may
reference cached tRPC data) by using `structures.push(shrine)` after checking
with `structures.find()`. Instead of mutating the original array, create a
derived variable that returns a new array containing the original structures
plus the synthetic shrine if it does not already exist. This prevents mutation
of cached tRPC data and avoids issues with stale shrine coordinates if
`sectorMap.anchors` changes. Replace the conditional push pattern with a
computed/derived approach that builds the final structures array without
modifying the original source data.
In `@app/src/libs/sector-map/legacy.ts`:
- Line 48: The hardcoded zone: "village" assignment at line 48 in the legacy
fallback marks every tile as a village, incorrectly altering travel and combat
behavior. Create a helper function (e.g., isLegacyVillageTile) that takes x and
y coordinates and implements the legacy-compatible village footprint/bounds
logic, then replace the hardcoded "village" string with a conditional that calls
this helper to determine the appropriate zone value for each tile.
In `@app/src/libs/sector-map/tiled.ts`:
- Around line 292-301: Add validation for targetSector after it is converted
using asNumber to ensure it is an integer within a valid sector range. Currently
the code accepts any finite number without checking if it is a whole number or
falls within acceptable sector bounds. After the line assigning targetSector
using asNumber, verify that the value is an integer and within the valid sector
range before proceeding to push to the anchors and exits arrays. If validation
fails, skip adding this exit to maintain data integrity.
In `@app/src/libs/threejs/sector.ts`:
- Around line 289-300: For authored maps, the code unconditionally sets
tile.blocked to false when a structure or wall is found, overriding intentional
author-set blocking and causing a mismatch between client-side pathfinding and
server-side validation. In both the hasStructure block and the hasWall block
(where tile.blocked = false is currently set), preserve the original
tile.blocked value instead of unconditionally clearing it, so that tiles
intentionally blocked by authors remain blocked.
In `@app/src/server/api/routers/travel.ts`:
- Around line 562-570: Instead of hard-failing when
validateWalkableSectorCoordinate returns false for the current position, use the
sector-map utility to coerce curLongitude and curLatitude to the nearest
walkable tile within the sectorMap. Update the coordinates to this normalized
position and then allow movement to continue from that corrected origin point,
removing the early error return that causes player lockout.
In `@app/src/server/api/routers/worldMap.ts`:
- Around line 123-168: The listSectorMaps procedure and the other endpoint at
line 170 currently expose draft and unpublished authoring metadata to any
authenticated user without role-based access control. Add authorization
middleware to both procedures to enforce the same content-editor role gate that
is used in the preview, save, publish, and archive endpoints. This will restrict
access to these endpoints so only authenticated users with the appropriate
content editor role can view the draft sector map history and metadata.
- Around line 287-317: Remove the ctx.drizzle.transaction wrapper from the
sector map update logic and instead use guarded WHERE predicates with
rowsAffected verification. Keep the logic flow the same but execute the
sectorMap update and insert operations directly without the transaction context.
When updating the status to ARCHIVED with the update(sectorMap) call, add
appropriate WHERE guard clauses to ensure the update is atomic. After the
insert(sectorMap) operation with getNextSectorMapVersion, verify the operation
succeeded rather than relying on transaction rollback behavior. This approach
uses compare-and-swap style predicates to maintain data consistency without
explicit transaction blocks per repository guidelines.
---
Outside diff comments:
In `@app/src/layout/Sector.tsx`:
- Around line 625-661: The useEffect hook in the movement logic unnecessarily
requires userData?.avatar to be set before allowing movement, but the move
function already provides fallback values to IMG_AVATAR_DEFAULT on lines
652-653. Remove the userData?.avatar check from the condition at line 625 (the
if statement checking target, originRef.current, pathFinderRef.current, and
userData?.avatar) to allow users without custom avatars to move. Keep all other
guards in place, and rely on the move function's built-in fallback logic to
handle cases where avatar or avatarLight are not defined.
---
Nitpick comments:
In `@app/drizzle/seeds/worldRegion.ts`:
- Around line 8-27: The current implementation in the region seeding loop uses a
read-then-write pattern with findFirst followed by conditional update or insert
operations, which creates race conditions under concurrent execution and
requires multiple database roundtrips. Replace this pattern with a single atomic
upsert operation on the worldRegion table that handles both insert and update in
one statement. This eliminates the separate findFirst query and the conditional
logic, using Drizzle's onConflictDoUpdate functionality to update existing
records by slug or insert new ones if they don't exist.
In `@app/src/app/manual/travel/sector-maps/page.tsx`:
- Around line 227-247: The native HTML select element with
id="sector-map-region" should be replaced with a Shadcn/Radix Select component
to maintain consistency with the codebase's UI component standards. Replace the
select element and its onChange handler (which calls setRegionSlug with the
event target value) with the corresponding Shadcn Select component, ensuring the
same functionality is preserved for the regionSlug state management. Apply this
same change to the other native select/textarea controls mentioned in the
comment at lines 261-271 and 357-368.
In `@app/src/layout/WorldRegionMap.tsx`:
- Around line 8-49: The file structure violates the project's component
organization guidelines which require: exported component → subcomponents →
helpers → types. Reorganize the WorldRegionMap.tsx file by moving the
WorldRegionMap function declaration to appear before the RegionIcon subcomponent
and the constant definitions (regionTone, regionLayout, etc.). Change the export
from default export to a named export by replacing "export default function
WorldRegionMap" with "export function WorldRegionMap". Then update all import
statements throughout the project that import this component to use the named
import syntax "import { WorldRegionMap }" instead of the default import pattern.
In `@app/src/libs/sector-map/validation.ts`:
- Around line 36-40: The getSectorTile function uses a linear Array.find()
lookup on the tiles array, which is called repeatedly during BFS neighbor
expansion checks in the reachability validation (lines 80-83 and broader context
64-86). Instead of doing multiple linear scans for each neighbor, create an
efficient lookup structure (such as a Map keyed by coordinate strings or a 2D
grid structure) before the BFS traversal begins. Pre-process the tiles into this
lookup structure once, then use it for O(1) coordinate-based tile lookups
throughout the neighbor expansion loop to eliminate the repeated linear scans.
In `@app/src/server/api/routers/worldMap.ts`:
- Around line 134-167: Move the three filter conditions currently applied in the
JavaScript filter function (checking input?.sector, input?.status, and
input?.regionSlug) into the where clause of the findMany query on the sectorMap
table. Instead of fetching all maps and filtering in memory, construct
appropriate where predicates using Drizzle's query builder to filter at the
database level, which will reduce the payload size and improve performance as
the data grows.
🪄 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: ef7150b1-a97c-41b0-becf-33169aa15f67
📒 Files selected for processing (30)
app/drizzle/constants.tsapp/drizzle/migrations/0018_numerous_tiger_shark.sqlapp/drizzle/migrations/meta/0018_snapshot.jsonapp/drizzle/migrations/meta/_journal.jsonapp/drizzle/schema.tsapp/drizzle/seed.tsapp/drizzle/seeds/worldRegion.tsapp/src/app/manual/travel/page.tsxapp/src/app/manual/travel/sector-maps/page.tsxapp/src/app/travel/page.tsxapp/src/hooks/tutorial.tsxapp/src/layout/Sector.tsxapp/src/layout/WarSystem.tsxapp/src/layout/WorldRegionMap.tsxapp/src/libs/hexgrid.tsapp/src/libs/quest.tsapp/src/libs/sector-map/legacy.tsapp/src/libs/sector-map/regions.tsapp/src/libs/sector-map/tiled.tsapp/src/libs/sector-map/types.tsapp/src/libs/sector-map/validation.tsapp/src/libs/threejs/sector.tsapp/src/libs/travel.tsapp/src/server/api/root.tsapp/src/server/api/routers/combat.tsapp/src/server/api/routers/travel.tsapp/src/server/api/routers/worldMap.tsapp/src/server/utils/sectorMap.tsapp/src/validators/travel.tsapp/tests/libs/sector-map.test.ts
Comment on lines
+24
to
+32
| topRightContent={ | ||
| canEditMaps ? ( | ||
| <Link href="/manual/travel/sector-maps"> | ||
| <Button> | ||
| <MapPlus className="mr-2 h-5 w-5" /> | ||
| Sector Maps | ||
| </Button> | ||
| </Link> | ||
| ) : undefined |
Contributor
There was a problem hiding this comment.
Avoid nesting a button inside a link.
Link renders an anchor, and Button renders a <button> by default, creating invalid nested interactive elements. Use the existing asChild API instead.
Suggested fix
- canEditMaps ? (
- <Link href="/manual/travel/sector-maps">
- <Button>
+ canEditMaps ? (
+ <Button asChild>
+ <Link href="/manual/travel/sector-maps">
<MapPlus className="mr-2 h-5 w-5" />
Sector Maps
- </Button>
- </Link>
+ </Link>
+ </Button>
) : undefined📝 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
| topRightContent={ | |
| canEditMaps ? ( | |
| <Link href="/manual/travel/sector-maps"> | |
| <Button> | |
| <MapPlus className="mr-2 h-5 w-5" /> | |
| Sector Maps | |
| </Button> | |
| </Link> | |
| ) : undefined | |
| topRightContent={ | |
| canEditMaps ? ( | |
| <Button asChild> | |
| <Link href="/manual/travel/sector-maps"> | |
| <MapPlus className="mr-2 h-5 w-5" /> | |
| Sector Maps | |
| </Link> | |
| </Button> | |
| ) : undefined |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/app/manual/travel/page.tsx` around lines 24 - 32, The topRightContent
section has a Button component nested inside a Link component, which creates
invalid nested interactive elements. To fix this, add the asChild prop to the
Button component to allow it to render the Link as its child without creating
nested interactive elements. This way, the Link will be the actual interactive
element while maintaining the Button's styling.
Comment on lines
+93
to
+95
| const sortedRegions = useMemo(() => { | ||
| return regions?.sort((a, b) => a.sortOrder - b.sortOrder) ?? []; | ||
| }, [regions]); |
Contributor
There was a problem hiding this comment.
Avoid mutating query cache data when sorting regions.
Line 94 calls .sort(...) on regions, which mutates the original array from the query cache. Copy first ([...regions]) or use toSorted(...) to keep cache data immutable.
Suggested fix
- const sortedRegions = useMemo(() => {
- return regions?.sort((a, b) => a.sortOrder - b.sortOrder) ?? [];
- }, [regions]);
+ const sortedRegions = useMemo(() => {
+ return regions ? [...regions].sort((a, b) => a.sortOrder - b.sortOrder) : [];
+ }, [regions]);📝 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
| const sortedRegions = useMemo(() => { | |
| return regions?.sort((a, b) => a.sortOrder - b.sortOrder) ?? []; | |
| }, [regions]); | |
| const sortedRegions = useMemo(() => { | |
| return regions ? [...regions].sort((a, b) => a.sortOrder - b.sortOrder) : []; | |
| }, [regions]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/app/manual/travel/sector-maps/page.tsx` around lines 93 - 95, The
sortedRegions useMemo hook is directly calling .sort() on the regions array,
which mutates the original query cache data. To fix this, create a non-mutating
copy of the regions array before sorting by either using the spread operator to
copy the array first (like [...regions].sort(...)) or by using the toSorted()
method (regions?.toSorted(...)) which returns a new sorted array without
mutating the original. This ensures the query cache data remains immutable.
Comment on lines
+213
to
+218
| min={0} | ||
| max={491} | ||
| value={sector} | ||
| onChange={(event) => { | ||
| const nextSector = Number(event.target.value); | ||
| setSector(nextSector); |
Contributor
There was a problem hiding this comment.
Use shared sector bounds constants and sanitize numeric input.
Line 213-214 hardcodes sector limits, and Line 217 stores raw Number(...) output. Use MAP_SECTOR_ID_MIN/MAX and clamp/guard invalid values to prevent transient invalid query args and contract drift with backend validation.
As per coding guidelines: "When displaying game-related values in the UI (costs, thresholds, damage values, etc.), always import and use the actual constants from @/drizzle/constants.ts rather than hardcoding values."
Suggested fix
-import { WorldRegionSlugs } from "`@/drizzle/constants`";
+import {
+ MAP_SECTOR_ID_MAX,
+ MAP_SECTOR_ID_MIN,
+ WorldRegionSlugs,
+} from "`@/drizzle/constants`";
...
- min={0}
- max={491}
+ min={MAP_SECTOR_ID_MIN}
+ max={MAP_SECTOR_ID_MAX}
value={sector}
onChange={(event) => {
- const nextSector = Number(event.target.value);
+ const rawValue = Number(event.target.value);
+ if (!Number.isFinite(rawValue)) return;
+ const nextSector = Math.min(
+ MAP_SECTOR_ID_MAX,
+ Math.max(MAP_SECTOR_ID_MIN, rawValue),
+ );
setSector(nextSector);📝 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
| min={0} | |
| max={491} | |
| value={sector} | |
| onChange={(event) => { | |
| const nextSector = Number(event.target.value); | |
| setSector(nextSector); | |
| min={MAP_SECTOR_ID_MIN} | |
| max={MAP_SECTOR_ID_MAX} | |
| value={sector} | |
| onChange={(event) => { | |
| const rawValue = Number(event.target.value); | |
| if (!Number.isFinite(rawValue)) return; | |
| const nextSector = Math.min( | |
| MAP_SECTOR_ID_MAX, | |
| Math.max(MAP_SECTOR_ID_MIN, rawValue), | |
| ); | |
| setSector(nextSector); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/app/manual/travel/sector-maps/page.tsx` around lines 213 - 218,
Replace the hardcoded sector bounds (0 and 491) in the min and max attributes of
the input field with the shared constants MAP_SECTOR_ID_MIN and
MAP_SECTOR_ID_MAX imported from `@/drizzle/constants.ts`. Additionally, in the
onChange handler, sanitize the numeric input by clamping the nextSector value
between these min and max bounds before passing it to setSector to prevent
invalid values from being stored.
Source: Coding guidelines
Comment on lines
+410
to
+417
| const currentSectorDimensions = currentSectorMap | ||
| ? { width: currentSectorMap.width, height: currentSectorMap.height } | ||
| : undefined; | ||
| const onEdge = isAtEdge(currentPosition, currentSectorDimensions); | ||
| const isGlobal = activeTab === globalLink; | ||
| const showGlobal = villages && globe && isGlobal; | ||
| const showSector = villages && currentSector && currentTile && !isGlobal; | ||
| const showGlobal = villages && worldRegions && isGlobal; | ||
| const showSector = | ||
| villages && hasCurrentSector && currentTile && currentSectorMap && !isGlobal; |
Contributor
There was a problem hiding this comment.
Gate world travel on loaded sector dimensions.
When currentSectorMap is still undefined, Line 413 and Line 829 pass undefined dimensions, so isAtEdge/findNearestEdge fall back to legacy sector dimensions. On authored maps with different sizes, players can be routed to the wrong edge before global travel starts.
Suggested fix
const currentSectorDimensions = currentSectorMap
? { width: currentSectorMap.width, height: currentSectorMap.height }
- : undefined;
- const onEdge = isAtEdge(currentPosition, currentSectorDimensions);
+ : null;
+ const onEdge = currentSectorDimensions
+ ? isAtEdge(currentPosition, currentSectorDimensions)
+ : false;
const isGlobal = activeTab === globalLink;
- const showGlobal = villages && worldRegions && isGlobal;
+ const showGlobal = villages && worldRegions && currentSectorDimensions && isGlobal;
@@
- {showModal && globe && userData && targetSector !== null && (
+ {showModal &&
+ globe &&
+ userData &&
+ targetSector !== null &&
+ currentSectorDimensions && (
<Modal2Also applies to: 817-830
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/app/travel/page.tsx` around lines 410 - 417, The issue is that
showGlobal on line 415 does not require currentSectorMap to be loaded, but when
it's undefined, the isAtEdge function on line 413 receives undefined dimensions
and falls back to legacy sector dimensions, causing incorrect edge routing on
custom-sized maps. Add a check for currentSectorMap in the showGlobal condition
(similar to the showSector check on line 416-417) to ensure actual sector
dimensions are available before enabling global travel UI, preventing isAtEdge
from falling back to legacy dimensions when currentSectorDimensions is
undefined.
Comment on lines
161
to
174
| // If we're in an active sector war, then we add a shrine to the center of the sector | ||
| if (!structures.find((s) => s.route === "/shrine")) { | ||
| const shrineAnchor = sectorMap.anchors.find( | ||
| (anchor) => anchor.key === "shrine.default", | ||
| ); | ||
| const shrine = createGenericStructure({ | ||
| name: "Sector Shrine", | ||
| route: "/shrine", | ||
| image: WAR_SHRINE_IMAGE, | ||
| longitude: 10, | ||
| latitude: 5, | ||
| longitude: shrineAnchor?.x ?? 10, | ||
| latitude: shrineAnchor?.y ?? 5, | ||
| }); | ||
| structures.push(shrine); | ||
| } |
Contributor
There was a problem hiding this comment.
Derive shrine structures without mutating query data.
structures can reference villageData.structures; pushing the synthetic shrine into it during render mutates cached tRPC data. With anchor-driven placement, this can also leave a shrine at stale coordinates if sectorMap.anchors changes.
Suggested refactor
- const structures = villageData?.structures || [];
+ const structures = useMemo(() => {
+ const baseStructures = villageData?.structures ?? [];
+ if (baseStructures.find((s) => s.route === "/shrine")) return baseStructures;
+
+ const shrineAnchor = sectorMap.anchors.find(
+ (anchor) => anchor.key === "shrine.default",
+ );
+ const shrine = createGenericStructure({
+ name: "Sector Shrine",
+ route: "/shrine",
+ image: WAR_SHRINE_IMAGE,
+ longitude: shrineAnchor?.x ?? 10,
+ latitude: shrineAnchor?.y ?? 5,
+ });
+
+ return [...baseStructures, shrine];
+ }, [villageData?.structures, sectorMap.anchors]);
@@
- if (!structures.find((s) => s.route === "/shrine")) {
- const shrineAnchor = sectorMap.anchors.find(
- (anchor) => anchor.key === "shrine.default",
- );
- const shrine = createGenericStructure({
- name: "Sector Shrine",
- route: "/shrine",
- image: WAR_SHRINE_IMAGE,
- longitude: shrineAnchor?.x ?? 10,
- latitude: shrineAnchor?.y ?? 5,
- });
- structures.push(shrine);
- }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/Sector.tsx` around lines 161 - 174, The code is mutating the
`structures` array (which may reference cached tRPC data) by using
`structures.push(shrine)` after checking with `structures.find()`. Instead of
mutating the original array, create a derived variable that returns a new array
containing the original structures plus the synthetic shrine if it does not
already exist. This prevents mutation of cached tRPC data and avoids issues with
stale shrine coordinates if `sectorMap.anchors` changes. Replace the conditional
push pattern with a computed/derived approach that builds the final structures
array without modifying the original source data.
Comment on lines
+292
to
+301
| const targetSector = asNumber(props.targetSector); | ||
| const targetAnchor = asString(props.targetAnchor) ?? "spawn.default"; | ||
| if (anchorKey && targetSector !== undefined) { | ||
| anchors.push({ key: anchorKey, x: coordinate.x, y: coordinate.y }); | ||
| exits.push({ | ||
| fromAnchor: anchorKey, | ||
| targetSector, | ||
| targetAnchor, | ||
| travelCost: asNumber(props.travelCost), | ||
| }); |
Contributor
There was a problem hiding this comment.
exit.targetSector is not validated as an integer in valid sector range.
Line 292 accepts any finite number, and current map validation does not reject invalid targetSector values. This allows malformed exits to be saved into normalizedJson.
Proposed fix
+import { isValidSectorId } from "`@/libs/sector-map/regions`";
...
- const targetSector = asNumber(props.targetSector);
+ const targetSector = asNumber(props.targetSector);
const targetAnchor = asString(props.targetAnchor) ?? "spawn.default";
- if (anchorKey && targetSector !== undefined) {
+ if (
+ anchorKey &&
+ targetSector !== undefined &&
+ Number.isInteger(targetSector) &&
+ isValidSectorId(targetSector)
+ ) {
anchors.push({ key: anchorKey, x: coordinate.x, y: coordinate.y });
exits.push({
fromAnchor: anchorKey,
targetSector,
targetAnchor,
travelCost: asNumber(props.travelCost),
});
} else {
- warnings.push(
- `Exit object ${normalizedObject.id} is missing anchorKey or targetSector`,
- );
+ errors.push(
+ `Exit object ${normalizedObject.id} has invalid anchorKey or targetSector`,
+ );
}📝 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
| const targetSector = asNumber(props.targetSector); | |
| const targetAnchor = asString(props.targetAnchor) ?? "spawn.default"; | |
| if (anchorKey && targetSector !== undefined) { | |
| anchors.push({ key: anchorKey, x: coordinate.x, y: coordinate.y }); | |
| exits.push({ | |
| fromAnchor: anchorKey, | |
| targetSector, | |
| targetAnchor, | |
| travelCost: asNumber(props.travelCost), | |
| }); | |
| import { isValidSectorId } from "`@/libs/sector-map/regions`"; | |
| const targetSector = asNumber(props.targetSector); | |
| const targetAnchor = asString(props.targetAnchor) ?? "spawn.default"; | |
| if ( | |
| anchorKey && | |
| targetSector !== undefined && | |
| Number.isInteger(targetSector) && | |
| isValidSectorId(targetSector) | |
| ) { | |
| anchors.push({ key: anchorKey, x: coordinate.x, y: coordinate.y }); | |
| exits.push({ | |
| fromAnchor: anchorKey, | |
| targetSector, | |
| targetAnchor, | |
| travelCost: asNumber(props.travelCost), | |
| }); | |
| } else { | |
| errors.push( | |
| `Exit object ${normalizedObject.id} has invalid anchorKey or targetSector`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/libs/sector-map/tiled.ts` around lines 292 - 301, Add validation for
targetSector after it is converted using asNumber to ensure it is an integer
within a valid sector range. Currently the code accepts any finite number
without checking if it is a whole number or falls within acceptable sector
bounds. After the line assigning targetSector using asNumber, verify that the
value is an integer and within the valid sector range before proceeding to push
to the anchors and exits arrays. If validation fails, skip adding this exit to
maintain data integrity.
Comment on lines
289
to
+300
| if (hasStructure) { | ||
| tile.level = minStructureLevel; | ||
| tile.hasStructure = true; | ||
| tile.blocked = false; | ||
| return tile; | ||
| } | ||
| // Check walls | ||
| const hasWall = wallPlacements.find((w) => w.x === tile.col && w.y === tile.row); | ||
| if (hasWall) { | ||
| tile.level = minStructureLevel; | ||
| tile.hasStructure = true; | ||
| tile.blocked = false; |
Contributor
There was a problem hiding this comment.
Do not override authored blocked tiles for structures.
For authored maps, Line 292 clears tile.blocked even when the normalized tile was intentionally blocked. That makes the client pathfinder/click layer treat the tile as walkable while server-side sector-map validation can still reject movement to it.
Suggested fix
if (hasStructure) {
tile.level = minStructureLevel;
tile.hasStructure = true;
- tile.blocked = false;
+ tile.blocked = authoredTile ? tile.blocked : false;
return tile;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/libs/threejs/sector.ts` around lines 289 - 300, For authored maps,
the code unconditionally sets tile.blocked to false when a structure or wall is
found, overriding intentional author-set blocking and causing a mismatch between
client-side pathfinding and server-side validation. In both the hasStructure
block and the hasWall block (where tile.blocked = false is currently set),
preserve the original tile.blocked value instead of unconditionally clearing it,
so that tiles intentionally blocked by authors remain blocked.
Comment on lines
+562
to
+570
| const sectorMap = await fetchPublishedSectorMap(ctx.drizzle, sector); | ||
| if ( | ||
| !validateWalkableSectorCoordinate(sectorMap, { | ||
| x: curLongitude, | ||
| y: curLatitude, | ||
| }) | ||
| ) { | ||
| return errorResponse("Your current location is not reachable"); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent player lockout when published maps invalidate current coordinates.
Hard-failing when the current tile is non-walkable can strand users permanently after map publishes or legacy position drift. Coerce the current coordinate to the nearest walkable tile (using the sector-map utility added for this migration) before reachability checks, then continue movement from that normalized origin.
Suggested patch direction
import {
+ coerceToWalkableSectorCoordinate,
fetchPublishedSectorMap,
validateReachableSectorPath,
validateWalkableSectorCoordinate,
} from "`@/server/utils/sectorMap`";
const sectorMap = await fetchPublishedSectorMap(ctx.drizzle, sector);
-if (
- !validateWalkableSectorCoordinate(sectorMap, {
- x: curLongitude,
- y: curLatitude,
- })
-) {
- return errorResponse("Your current location is not reachable");
-}
+const currentCoordinate = validateWalkableSectorCoordinate(sectorMap, {
+ x: curLongitude,
+ y: curLatitude,
+})
+ ? { x: curLongitude, y: curLatitude }
+ : coerceToWalkableSectorCoordinate(sectorMap, { x: curLongitude, y: curLatitude });
+
+if (!currentCoordinate) {
+ return errorResponse("Your current location is not reachable");
+}
if (!validateWalkableSectorCoordinate(sectorMap, { x: longitude, y: latitude })) {
return errorResponse("Target location is not reachable");
}
if (
!validateReachableSectorPath(
sectorMap,
- { x: curLongitude, y: curLatitude },
+ currentCoordinate,
{ x: longitude, y: latitude },
)
) {
return errorResponse("No reachable path to target location");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/travel.ts` around lines 562 - 570, Instead of
hard-failing when validateWalkableSectorCoordinate returns false for the current
position, use the sector-map utility to coerce curLongitude and curLatitude to
the nearest walkable tile within the sectorMap. Update the coordinates to this
normalized position and then allow movement to continue from that corrected
origin point, removing the early error return that causes player lockout.
Comment on lines
+123
to
+168
| listSectorMaps: protectedProcedure | ||
| .input( | ||
| z | ||
| .object({ | ||
| sector: sectorSchema.optional(), | ||
| regionSlug: regionSlugSchema.optional(), | ||
| status: sectorMapStatusSchema.optional(), | ||
| }) | ||
| .optional(), | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const maps = await ctx.drizzle.query.sectorMap.findMany({ | ||
| columns: { | ||
| id: true, | ||
| sector: true, | ||
| regionId: true, | ||
| name: true, | ||
| width: true, | ||
| height: true, | ||
| status: true, | ||
| version: true, | ||
| sourceHash: true, | ||
| publishedAt: true, | ||
| publishedByUserId: true, | ||
| createdAt: true, | ||
| updatedAt: true, | ||
| }, | ||
| with: { | ||
| region: true, | ||
| publishedBy: { | ||
| columns: { | ||
| userId: true, | ||
| username: true, | ||
| }, | ||
| }, | ||
| }, | ||
| orderBy: (table, { desc }) => [desc(table.createdAt)], | ||
| }); | ||
|
|
||
| return maps.filter((map) => { | ||
| if (input?.sector !== undefined && map.sector !== input.sector) return false; | ||
| if (input?.status && map.status !== input.status) return false; | ||
| if (input?.regionSlug && map.region?.slug !== input.regionSlug) return false; | ||
| return true; | ||
| }); | ||
| }), |
Contributor
There was a problem hiding this comment.
Restrict sector-map history/list reads to content editors.
Line 123 and Line 170 expose draft/history metadata to any authenticated user. These endpoints should enforce the same content-role gate as preview/save/publish/archive to avoid leaking unpublished authoring data.
Also applies to: 170-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/worldMap.ts` around lines 123 - 168, The
listSectorMaps procedure and the other endpoint at line 170 currently expose
draft and unpublished authoring metadata to any authenticated user without
role-based access control. Add authorization middleware to both procedures to
enforce the same content-editor role gate that is used in the preview, save,
publish, and archive endpoints. This will restrict access to these endpoints so
only authenticated users with the appropriate content editor role can view the
draft sector map history and metadata.
Comment on lines
+287
to
+317
| await ctx.drizzle.transaction(async (tx) => { | ||
| const version = await getNextSectorMapVersion(tx, input.sector); | ||
| if (input.publish) { | ||
| await tx | ||
| .update(sectorMap) | ||
| .set({ status: "ARCHIVED", updatedAt: now }) | ||
| .where( | ||
| and( | ||
| eq(sectorMap.sector, input.sector), | ||
| eq(sectorMap.status, "PUBLISHED"), | ||
| ), | ||
| ); | ||
| } | ||
| await tx.insert(sectorMap).values({ | ||
| id, | ||
| sector: input.sector, | ||
| regionId: dbRegion.id, | ||
| name: input.name, | ||
| width: normalizedMap.width, | ||
| height: normalizedMap.height, | ||
| status, | ||
| version, | ||
| sourceHash, | ||
| rawTiledJson: input.tiledJson, | ||
| normalizedJson: normalizedMap, | ||
| publishedAt: input.publish ? now : null, | ||
| publishedByUserId: input.publish ? ctx.userId : null, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify transaction usage and CAS-result checks in this router.
rg -n '\.transaction\(' app/src/server/api/routers/worldMap.ts
rg -n 'rowsAffected' app/src/server/api/routers/worldMap.tsRepository: studie-tech/TheNinjaRPG
Length of output: 264
Replace transaction blocks with guarded CAS-style updates per repo DB rules.
Lines 287 and 341 use ctx.drizzle.transaction(...). Router mutations must avoid transactions and rely on guarded WHERE predicates plus rowsAffected verification for atomic updates.
Per coding guidelines: "NEVER use database transactions - use guard clauses with WHERE conditions instead" and "Use compare-and-swap predicates and verify rowsAffected before granting irreversible rewards in tRPC endpoints."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/worldMap.ts` around lines 287 - 317, Remove the
ctx.drizzle.transaction wrapper from the sector map update logic and instead use
guarded WHERE predicates with rowsAffected verification. Keep the logic flow the
same but execute the sectorMap update and insert operations directly without the
transaction context. When updating the status to ARCHIVED with the
update(sectorMap) call, add appropriate WHERE guard clauses to ensure the update
is atomic. After the insert(sectorMap) operation with getNextSectorMapVersion,
verify the operation succeeded rather than relying on transaction rollback
behavior. This approach uses compare-and-swap style predicates to maintain data
consistency without explicit transaction blocks per repository guidelines.
Source: Coding guidelines
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
/manual/travel/sector-mapsand focused sector-map unit tests.Verification
git diff --check origin/main...HEADbunx vitest --config vitest.unit.config.ts tests/libs/sector-map.test.ts --runbun run typecheckcurrently fails on currentorigin/maininapp/tests/libs/combat/damage_modifiers.test.ts(1087,19). This branch has no diff for that file.NEXT_PUBLIC_*vars and returned 200 for/travel,/manual/travel, and/manual/travel/sector-maps.Summary by CodeRabbit
Release Notes
New Features
Improvements