restrict problematic flags fixes #1690 #1805
Conversation
WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FlagInputModal
participant CountriesData
participant Schemas
User->>FlagInputModal: Type search query
FlagInputModal->>CountriesData: Request country list
CountriesData-->>FlagInputModal: Return countries (some have restricted=true)
FlagInputModal->>FlagInputModal: Exclude restricted, then includedInSearch check
User->>FlagInputModal: Select country
FlagInputModal->>Schemas: Validate selected code
Schemas->>Schemas: Check code against non-restricted countryCodes
Schemas-->>FlagInputModal: Validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
evanpelle
force-pushed
the
evan-restrict-problematic-flags
branch
from
796abe5 to
da0002a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/core/Schemas.ts (3)
193-205: Critical: validation now blocks server-sent NPC flags (violates PR goal)By filtering out restricted codes at the source and using a single FlagSchema, any payload containing a restricted flag (e.g., NPC nation in GameStartInfo) will fail validation. The PR goal says: “prevent users from selecting these flags while allowing nations to continue using them.”
Split schemas:
- ClientFlagSchema: only non-restricted codes (for client-submitted data: SpawnIntent, ClientJoin, etc.).
- ServerFlagSchema: full set (for server-sent Player/records).
Apply this diff here to define both variants and keep FlagSchema as the restricted one for backward compatibility in client inputs:
-export const UsernameSchema = SafeString; -const countryCodes = countries.filter((c) => !c.restricted).map((c) => c.code); -export const FlagSchema = z - .string() - .max(128) - .optional() - .refine( - (val) => { - if (val === undefined || val === "") return true; - if (val.startsWith("!")) return true; - return countryCodes.includes(val); - }, - { message: "Invalid flag: must be a valid country code or start with !" }, - ); +export const UsernameSchema = SafeString; +const allCountryCodes = countries.map((c) => c.code); +const allowedClientCountryCodes = countries + .filter((c) => !c.restricted) + .map((c) => c.code); + +export const ClientFlagSchema = z + .string() + .max(128) + .optional() + .refine( + (val) => { + if (val === undefined || val === "") return true; + if (val.startsWith("!")) return true; + return allowedClientCountryCodes.includes(val); + }, + { message: "Invalid flag: must be a valid country code or start with !" }, + ); + +export const ServerFlagSchema = z + .string() + .max(128) + .optional() + .refine( + (val) => { + if (val === undefined || val === "") return true; + if (val.startsWith("!")) return true; + return allCountryCodes.includes(val); + }, + { message: "Invalid flag: must be a valid country code or start with !" }, + ); + +// Keep existing name for client-submitted usage sites +export const FlagSchema = ClientFlagSchema;Then update usage sites (see next comment) so server-to-client schemas accept ServerFlagSchema.
396-401: Use ServerFlagSchema for server-sent Player dataGameStartInfoSchema.players and GameEndInfo/records include NPC nations. They should be allowed to carry restricted flags. Switch PlayerSchema.flag to ServerFlagSchema.
export const PlayerSchema = z.object({ clientID: ID, username: UsernameSchema, - flag: FlagSchema, + flag: ServerFlagSchema, pattern: PatternSchema, });Also adjust any other server-sent schemas that embed PlayerSchema (GameStartInfoSchema, GameEndInfoSchema) indirectly via this change.
255-262: Keep restricted validation for client-submitted SpawnIntent.flagrg shows the schema still uses FlagSchema (not a client-specific variant) in three places. If you have a client/server FlagSchema split, update these to ClientFlagSchema — otherwise confirm FlagSchema is intentionally an alias of the client variant.
Locations:
- src/core/Schemas.ts — flag: FlagSchema at line 258 (SpawnIntentSchema, ~255–262)
- src/core/Schemas.ts — flag: FlagSchema at line 399
- src/core/Schemas.ts — flag: FlagSchema at line 505
Suggested minimal change:
- flag: FlagSchema,
- flag: ClientFlagSchema,
If FlagSchema already aliases ClientFlagSchema, no change needed — otherwise please make the replacement above.
🧹 Nitpick comments (2)
src/client/FlagInputModal.ts (2)
20-25: Localize visible strings (title and placeholder)These strings should go through your translateText (or equivalent) to match the PR note and the rest of the app i18n.
Apply this small diff (adjust the import according to your i18n setup):
- <o-modal title="Flag Selector Modal" alwaysMaximized> + <o-modal title=${translateText("Flag Selector Modal")} alwaysMaximized> @@ - placeholder="Search..." + placeholder=${translateText("Search...")}If translateText needs importing, add it at the top:
+import { translateText } from "./i18n"; // adjust path as needed
21-27: Prefer the input event over keyup/change for search UXUsing @input alone provides immediate updates and handles all edit cases (typing, paste, clear). It also reduces duplicate handler calls.
- @change=${this.handleSearch} - @keyup=${this.handleSearch} + @input=${this.handleSearch}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/FlagInputModal.ts(1 hunks)src/client/data/countries.json(3 hunks)src/core/Schemas.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
🔇 Additional comments (2)
src/client/data/countries.json (1)
485-489: LGTM: restricted flags are correctly markedThe new "restricted": true markers for Confederate States, Fascist Spain, and German Empire look correct and align with the UI filter.
Caution: with the current FlagSchema change (see Schemas.ts), these codes will be rejected by validation everywhere. If NPC nations still use these codes, server-sent payloads that include them will fail schema validation. See my Schemas.ts comment for a safe split between client-submitted vs. server-sent flags.
Would you like a quick check that no other flagged codes should be restricted now? I can add a script to list possibly sensitive codes and compare to the restricted list.
Also applies to: 704-708, 808-812
src/client/FlagInputModal.ts (1)
33-35: Good: restricted flags are excluded from the pickerThe predicate correctly hides any country with restricted === true, even when the search matches.
No functional issues spotted here.
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
There was a problem hiding this comment.
Please link the related issue: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue
evanpelle
dismissed stale reviews from scottanderson and coderabbitai[bot]
via
32fe3a5
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/client/FlagInputModal.ts (3)
31-36: Make the predicate easier to read and compute the query once.Minor readability/perf polish: precompute the normalized search once and avoid repeated toLowerCase calls inside filter.
You can refactor render() like:
const q = this.search.trim().toLowerCase(); const filtered = Countries.filter(({ restricted, name, code }) => !restricted && (q === "" || name.toLowerCase().includes(q) || code.toLowerCase().includes(q)), ); // ... ${filtered.map((country) => html`...`)}
21-27: Prefer @input over @change/@keyup for live search.@input fires on every value change and removes the need to wire both keyup and change, simplifying logic and improving UX.
- @change=${this.handleSearch} - @keyup=${this.handleSearch} + @input=${this.handleSearch}
48-58: Improve image accessibility, performance, and robustness.
- Add alt text for screen readers.
- Lazy-load and async-decode for perf.
- URL-encode code in the src to handle spaces and special chars safely.
- <img - class="country-flag w-full h-auto" - src="/flags/${country.code}.svg" + <img + class="country-flag w-full h-auto" + src="/flags/${encodeURIComponent(country.code)}.svg" + alt="${country.name} flag" + loading="lazy" + decoding="async" @error=${(e: Event) => { const img = e.currentTarget as HTMLImageElement; const fallback = "/flags/xx.svg"; if (img.src && !img.src.endsWith(fallback)) { img.src = fallback; } }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/client/FlagInputModal.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 🏗️ Build
- GitHub Check: 🔍 ESLint
- GitHub Check: 🎨 Prettier
- GitHub Check: 🔬 Test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
evan-restrict-problematic-flags
branch
from
32fe3a5 to
9a2019c
Compare
evanpelle
changed the title
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
## Description: Some flags are offensive, so don't allow them to be selected by users. I think it's fine for nations continue to use the flags. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] 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: evan
Some flags are offensive, so don't allow them to be selected by users. I think it's fine for nations continue to use the flags. - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: evan
Some flags are offensive, so don't allow them to be selected by users. I think it's fine for nations continue to use the flags. - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: evan
Description:
Some flags are offensive, so don't allow them to be selected by users.
I think it's fine for nations continue to use the flags.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan