adds more content and free movement#121
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a complete Tiled-to-runtime map pipeline for the adventure map: a Tiled source JSON, a CLI converter ( ChangesFree-walk map movement pipeline and runtime
Sequence Diagram(s)sequenceDiagram
participant User as User (keyboard / D-pad)
participant MapDashboardClient
participant useFreeWalkMovement
participant mapGeometry as mapGeometry (canStandAtPoint)
participant MapManifest as map.json manifest
User->>MapDashboardClient: ArrowRight keydown / dpad pointer down
MapDashboardClient->>useFreeWalkMovement: setDirectionHeld("right", true)
useFreeWalkMovement->>useFreeWalkMovement: ensureAnimation → requestAnimationFrame
loop RAF tick
useFreeWalkMovement->>mapGeometry: canStandAtPoint(manifest, candidatePos)
mapGeometry->>MapManifest: read walkable/blocked polygons
MapManifest-->>mapGeometry: polygon data
mapGeometry-->>useFreeWalkMovement: true / false
useFreeWalkMovement->>useFreeWalkMovement: update sourcePosition, renderPosition
end
useFreeWalkMovement-->>MapDashboardClient: renderPosition, nearbyPOI (via getNearestTriggeredPOI)
MapDashboardClient->>MapDashboardClient: show MapInteractionPrompt when nearbyPOI + nearbyLocation
User->>MapDashboardClient: Enter/Space keydown
MapDashboardClient->>MapDashboardClient: setActiveLocationID(nearbyLocation.id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs/map-dashboard-feature-spec.md`:
- Around line 750-752: The final decision statement in the spec contradicts the
implemented behavior documented in earlier sections. The decision currently
defers free movement implementation, but the spec's earlier sections describe
free movement as already implemented in the runtime behavior. Update the
decision text at lines 750-752 to reflect that free movement is implemented and
integrated with the Tiled pipeline and debug overlay, making it consistent with
how it is documented throughout the rest of the specification.
In `@scripts/convert-tiled-map.mjs`:
- Around line 107-112: The enabled property in the returned object defaults to
false when properties.enabled is undefined, causing new POIs to be silently
disabled. Modify the enabled property assignment to explicitly default to true
instead, so that POIs are enabled by default unless the properties.enabled value
is explicitly set to false. Use a conditional check or nullish coalescing
approach to achieve this default behavior.
In `@scripts/validate-map-manifest.mjs`:
- Around line 94-106: The POI validation loop iterating through
manifest.pointsOfInterest currently only validates that triggerRadius and
markerRadius are finite numbers but does not validate the trigger shape type or
its bounds. Add an assert statement to validate that poi.trigger.shape exists
and is either 'circle' or 'ellipse', then add a conditional check that when the
shape is 'ellipse', validates that the ellipse bounds (width and height
properties) are positive finite numbers. Place these new assertions after the
existing triggerRadius validation to ensure all POI trigger configuration is
properly validated before processing.
- Around line 84-92: The spawn validation at lines 84-92 only checks if the
center point (spawnPoint) is inside walkable polygons and outside blocked
polygons, but runtime movement requires the full spawn foot-radius circle to be
valid. To fix this, retrieve the spawn foot-radius value from the manifest and
modify the assertions to validate that the entire circular area around
spawnPoint (not just the center) fits within walkable space and avoids blocked
polygons. This may require checking multiple points along the circle perimeter
using pointInPolygon at the radius distance from the spawn center, or
implementing a proper circle-to-polygon collision check that validates the full
spawn footprint matches runtime collision rules.
In `@src/app/`(frontend)/dashboard/map/mapConfig.ts:
- Around line 166-167: The toMapNodeID function currently silently defaults
unknown node IDs to 'spawn' instead of surfacing configuration errors. Replace
the conditional fallback logic that returns 'spawn' for invalid values with an
error throw or assertion that fails fast when an unmapped node ID is
encountered, so that typos or stale poi.nodeId values in the manifest are caught
as contract violations rather than silently routing to the wrong node. Apply
this same fix to the other location mentioned at line 182.
In `@src/app/`(frontend)/dashboard/map/MapDashboardClient.tsx:
- Around line 96-100: The proximity activation via keyboard entry (Enter and
Space keys) in the code block around line 96-100 does not check if the
nearbyLocation is disabled before calling setActiveLocationID. Add a check for
the disabled property of nearbyLocation to prevent opening intentionally
disabled locations, similar to how marker clicks are handled. Apply the same
disabled location check to the other proximity activation entry point mentioned
at lines 234-239 to maintain consistent behavior across all ways to activate
locations.
- Around line 94-101: The onKeyDown handler within the useEffect is intercepting
Enter and Space key events globally without checking if the keyboard event
originated from an interactive element like a button or link. To fix this,
before calling event.preventDefault() and
setActiveLocationID(nearbyLocation.id), add a check to determine if the
event.target is an interactive control (such as HTMLButtonElement,
HTMLAnchorElement, HTMLInputElement, etc.). If the event originated from such a
control, return early to allow the control to handle its own keyboard behavior
rather than triggering the location action.
In `@src/app/`(frontend)/dashboard/map/useFreeWalkMovement.ts:
- Around line 24-28: The isEditableTarget function currently only detects
contenteditable elements with the exact value "true", which misses other valid
editable states like contenteditable="" (empty string),
contenteditable="plaintext-only", and inherited contenteditable from parent
elements. Update the selector in the closest() method to match any
contenteditable attribute regardless of its value, or better yet, use
isContentEditable property instead of the attribute selector, to properly detect
all editable content scenarios before preventing movement key behavior.
- Around line 185-192: In the useEffect hook that sets up the keydown and keyup
event listeners, add event listeners for 'blur' on the window and
'visibilitychange' on the document to clear the heldDirectionsRef state when
focus is lost or visibility changes. Create handlers for these events that reset
heldDirectionsRef (similar to how the keyboard handlers manage movement state),
and ensure these new listeners are properly removed in the cleanup function
along with the existing key event listeners. Update the dependency array to
include any new state or refs referenced in the new handlers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94ae29de-f263-4872-9447-6cf7fd2078a2
📒 Files selected for processing (16)
docs/map-dashboard-feature-spec.mdpackage.jsonpublic/assets/map/maps/adventure/adventure.tiled.jsonpublic/assets/map/maps/adventure/map.jsonscripts/convert-tiled-map.mjsscripts/validate-map-manifest.mjssrc/app/(frontend)/dashboard/map/MapDashboardClient.tsxsrc/app/(frontend)/dashboard/map/MapDebugOverlay.tsxsrc/app/(frontend)/dashboard/map/MapDialog.tsxsrc/app/(frontend)/dashboard/map/MapInteractionPrompt.tsxsrc/app/(frontend)/dashboard/map/mapConfig.tssrc/app/(frontend)/dashboard/map/mapGeometry.tssrc/app/(frontend)/dashboard/map/mapManifest.tssrc/app/(frontend)/dashboard/map/useFreeWalkMovement.tssrc/app/(frontend)/globals.csstests/e2e/app.spec.ts
Contributor
Author
|
Addressed the PR feedback. I verified each finding against current code; all were still valid, so none were skipped. Fixes made:
|
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.
This PR enables free character movement on the map dashboard using Tiled-authored map metadata.
Map Data Pipeline
public/assets/map/maps/adventure/.map.json.corepack pnpm map:convertto convert and validate Tiled JSON.scripts/convert-tiled-map.mjs.scripts/validate-map-manifest.mjs.map-layers-jsonspike file.Free Movement Runtime
mapManifest.tsto import and type the generated map manifest.mapGeometry.tsfor collision/proximity math:useFreeWalkMovement.ts:Map UI Updates
?mapDebug=1to inspect collision, POIs, and spawn.Tooling
packageManager: pnpm@10.18.3socorepack pnpmresolves consistently.map:convertpackage script.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores