fix literally 2 pixels in the river #2497
Conversation
WalkthroughThe PR updates the Britannia map manifest with adjusted land tile counts across three map variants and reformats nation coordinate arrays to a multi-line layout for improved readability. No structural map changes are made. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
map-generator/assets/maps/britannia/image.pngis excluded by!**/*.pngresources/maps/britannia/map.binis excluded by!**/*.binresources/maps/britannia/map16x.binis excluded by!**/*.binresources/maps/britannia/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (1)
resources/maps/britannia/manifest.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
📚 Learning: 2025-08-19T11:00:55.422Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
Applied to files:
resources/maps/britannia/manifest.json
🪛 GitHub Actions: 🧪 CI
resources/maps/britannia/manifest.json
[warning] 1-1: Prettier formatting issues found in resources/maps/britannia/manifest.json. Run 'npx prettier --write' to fix.
🔇 Additional comments (2)
resources/maps/britannia/manifest.json (2)
4-4: Tile count reductions align with river fix.The land tile count decreases across all map variants are proportional and logically consistent with removing a disconnected river section.
Also applies to: 9-9, 14-14
1-227: Clarify test additions claim; manifest and map name translation are correct.Good news: the manifest structure is correct. The britannia nation names are correctly NOT added to en.json—this matches the established OpenFrontIO pattern where only map names are translated, never the nation names displayed on maps.
The en.json modification is valid: "britannia": "Britannia" (the map name translation) was added on line 190. This is appropriate.
However, your second checklist claim needs clarification:
- Tests claim unverified: No britannia-specific test files were found in the codebase. Please confirm whether you added tests for this map, and if so, which files contain them. If no tests were added, uncheck that item in your PR description.
The code itself is sound—nation names correctly remain untranslated as per the pattern.
| "coordinates": [ | ||
| 960, | ||
| 1258 | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
Fix Prettier formatting error before merge.
The CI pipeline reports Prettier formatting issues. Although the multi-line coordinate format improves readability, it does not match your project's Prettier configuration. Run npx prettier --write resources/maps/britannia/manifest.json locally and commit the corrected formatting.
Also applies to: 29-32, 38-41, 47-50, 56-59, 65-68, 74-77, 83-86, 92-95, 101-104, 110-113, 119-122, 128-131, 137-140, 146-149, 155-158, 164-167, 173-176, 182-185, 191-194, 200-203, 209-212, 218-221
I need to verify the claims in the review comment by inspecting the actual file and Prettier configuration.
🏁 Script executed:
#!/bin/bash
# Check if manifest.json exists and inspect the coordinates section
fd "manifest.json" --type f | head -5Length of output: 169
🏁 Script executed:
#!/bin/bash
# Look for Prettier configuration files
fd -e prettierrc -e "prettier.config.*" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check package.json for Prettier config and check first 50 lines of manifest
cat package.json | grep -A 20 "prettier" || echo "No prettier config in package.json"Length of output: 2195
🏁 Script executed:
#!/bin/bash
# Get full content of the manifest.json to verify actual formatting
cat resources/maps/britannia/manifest.json 2>/dev/null || echo "File not found at expected path"Length of output: 3623
I found the file and can see it uses multi-line coordinate formatting. Let me verify Prettier's default behavior for JSON arrays.
---
🌐 Web query:
```web
Prettier default JSON array formatting behavior short arrays
💡 Result:
Short answer: Prettier prints JSON arrays on a single line when the entire array (including brackets, items, commas and surrounding context) fits within its printWidth and the array is simple; otherwise it breaks the array across lines. Single-line arrays never get trailing commas. See Prettier’s options/behavior (printWidth + trailingComma). [1][2]
Sources:
- Prettier options (printWidth behavior). [1]
- Prettier options (single-line arrays do not get trailing commas / trailingComma option). [2]
Format coordinates to match Prettier defaults.
The multi-line coordinate arrays do not match Prettier's default formatting. Prettier prints JSON arrays on a single line when the entire array fits within its printWidth. The short arrays like [960, 1258] should be formatted on one line. Run npx prettier --write resources/maps/britannia/manifest.json to apply the correct formatting.
Also applies to: lines 29–32, 38–41, 47–50, 56–59, 65–68, 74–77, 83–86, 92–95, 101–104, 110–113, 119–122, 128–131, 137–140, 146–149, 155–158, 164–167, 173–176, 182–185, 191–194, 200–203, 209–212, 218–221
🤖 Prompt for AI Agents
In resources/maps/britannia/manifest.json around lines 20 to 23, the short
numeric coordinate arrays are split across multiple lines instead of using
Prettier's single-line formatting (e.g. [960, 1258]); update those arrays (and
the other listed occurrences) to single-line arrays or run the formatter to fix
them automatically. Run npx prettier --write
resources/maps/britannia/manifest.json (or manually collapse each short
coordinate array to one line) so all coordinate arrays that fit within
printWidth are formatted as single-line arrays.
Duwibi
left a comment
Duwibi
left a comment
There was a problem hiding this comment.
It looks good! If you've tested it and it works without problem, it should be ready :)
|
can you run |
LoackyBit
left a comment
LoackyBit
left a comment
There was a problem hiding this comment.
To pass prettier check, replace this formatting:
{
"coordinates": [
918,
1088
],
with the original one:
{
"coordinates": [1114, 1108],
4 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Found again another bit of a river that wasn't connected. It's the River Teme, for those curious.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
dxtron_25592