Skip to content

Conversation

@Aotumuri
Copy link
Member

@Aotumuri Aotumuri commented Jan 27, 2026

Resolves #3041

Description:

  • Add a test to ensure an error is thrown when manifest.json specifies a non-existent flag.
  • Fix the underlying issue by removing the invalid flag specification (see error below).
resources/maps/straitofgibraltar/manifest.json -> nations[0].flag "Rif" does not exist in resources/flags
resources/maps/straitofgibraltar/manifest.json -> nations[5].flag "Shilha" does not exist in resources/flags
resources/maps/straitofgibraltar/manifest.json -> nations[6].flag "Andalusia" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[0].flag "custom:Kingdom of the Two Sicilies" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[3].flag "custom:Tuscany" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[5].flag "custom:Modena" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[6].flag "custom:Parma" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[8].flag "custom:Kingdom of Sardinia" does not exist in resources/flags
resources/maps/italia/manifest.json -> nations[11].flag "custom:Ottoman Empire2" does not exist in resources/flags
resources/maps/britannia/manifest.json -> nations[19].flag "gb-nir" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[0].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[1].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[2].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[4].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[5].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[6].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[7].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[8].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[9].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[10].flag "quebec" does not exist in resources/flags
resources/maps/montreal/manifest.json -> nations[11].flag "quebec" does not exist in resources/flags

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

aotumuri

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

Walkthrough

Removed several flag properties from nation entries across Britannia, Italia, and Strait of Gibraltar maps; normalized flag casing for Montreal entries; added a test that validates all manifest flag references resolve to existing SVG files in resources/flags.

Changes

Cohort / File(s) Summary
Map asset info.json files
map-generator/assets/maps/britannia/info.json, map-generator/assets/maps/italia/info.json, map-generator/assets/maps/straitofgibraltar/info.json
Removed flag property from specific nation entries: Britannia (Fermanagh), Italia (Kingdom of the Two Sicilies, Tuscany, Modena, Parma, Kingdom of Sardinia, Ottoman Empire), Strait of Gibraltar (Rif, Shilha, Andalusia). name and coordinates preserved.
Map resource manifest.json files
resources/maps/britannia/manifest.json, resources/maps/italia/manifest.json, resources/maps/straitofgibraltar/manifest.json
Removed matching flag properties from the resource manifests for the same nation entries; other fields unchanged.
Montreal flag casing updates
map-generator/assets/maps/montreal/info.json, resources/maps/montreal/manifest.json
Normalized flag strings from "quebec" to "Quebec" for multiple Montreal nation entries; no other changes.
Flag validation test
tests/MapManifestFlags.test.ts
New test that finds all resources/maps/**/manifest.json files and verifies each nation's flag is a non-empty, non-negated string and that the corresponding *.svg exists in resources/flags. Accumulates and reports all violations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🚩 Small flags trimmed from crowded lines,
Names now tidy, casing that shines.
A test keeps watch by file and name,
Ensuring flags point to real frames.
Quiet fixes, tidy aims.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding validation for unknown flags in manifest.json files and fixing the underlying issues.
Description check ✅ Passed The description clearly explains the changes: a new test for flag validation and removal of non-existent flag references from multiple manifest.json files.
Linked Issues check ✅ Passed The PR fully addresses issue #3041 by implementing validation for non-existent flags and removing invalid flag references from all affected manifest.json files.
Out of Scope Changes check ✅ Passed All changes directly address the linked issue: flag validation test addition and removal of invalid flag references. The Montreal flag casing fix ('quebec' to 'Quebec') directly supports this validation by fixing a detected invalid flag.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@tests/MapManifestFlags.test.ts`:
- Around line 14-17: Update the case-sensitive flag references in the montreal
manifest: find all occurrences of the string "quebec" in
resources/maps/montreal/manifest.json (11 instances) and change them to "Quebec"
so they exactly match the actual flag file name (Quebec.svg); ensure you only
modify those flag name values (not other keys) and run the tests to confirm the
case mismatch is resolved.
🧹 Nitpick comments (1)
tests/MapManifestFlags.test.ts (1)

38-38: Consider adding a brief comment for the ! prefix convention.

The skip logic for flags starting with "!" is not obvious. A short comment would help future readers understand why these flags are excluded from validation.

+          // Skip flags prefixed with "!" (used for disabled/placeholder entries)
           if (flag.startsWith("!")) return;

@github-project-automation github-project-automation bot moved this from Triage to Development in OpenFront Release Management Jan 27, 2026
@Aotumuri Aotumuri changed the title add validation for unknown flags in manifest.json fix: add validation for unknown flags in manifest.json Jan 27, 2026
Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@github-project-automation github-project-automation bot moved this from Development to Final Review in OpenFront Release Management Jan 27, 2026
@evanpelle evanpelle added this to the v30 milestone Jan 27, 2026
@evanpelle evanpelle merged commit 0cc58a8 into openfrontio:main Jan 27, 2026
9 of 11 checks passed
@github-project-automation github-project-automation bot moved this from Final Review to Complete in OpenFront Release Management Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Italy map nations have flags which don't exist

3 participants