Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f69129267b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(value) | ||
| } | ||
| }) | ||
| .or_else(|| Some(LEGACY_TERREXTRA_URL.to_string())) |
There was a problem hiding this comment.
Preserve explicit opt-out for TERRITORY_EXTRA_URL
The new territory_extra_url() logic cannot actually disable supplemental fetches: when TERRITORY_EXTRA_URL is set to "0", "false", "none", "off", or empty, the and_then returns None, but the trailing .or_else(|| Some(LEGACY_TERREXTRA_URL.to_string())) immediately restores the default URL. In deployments that intentionally set an opt-out value, the server will still call the legacy gist every refresh cycle, which contradicts the intended behavior and makes the opt-out ineffective.
Useful? React with 👍 / 👎.
| let data = load_extra_data(&state.http_client).await; | ||
| let count = data.len(); | ||
| *state.extra_terr.write().await = data; | ||
| state.extra_data_dirty.store(true, Ordering::Release); | ||
| info!("loaded extra territory data for {count} territories"); | ||
| } |
There was a problem hiding this comment.
State reset on transient remote fetch failure
On every tick, *state.extra_terr.write().await = data unconditionally replaces the stored state with the result of load_extra_data. When the remote fetch fails, load_extra_data returns only bundled data (no connections), erasing any connection data that was successfully loaded on the previous tick. The old code left state untouched on failure; this change removes that safety net. After a single failed refresh, all territory connection data from the remote feed disappears until the next successful fetch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/extra_data_loader.rs
Line: 18-23
Comment:
**State reset on transient remote fetch failure**
On every tick, `*state.extra_terr.write().await = data` unconditionally replaces the stored state with the result of `load_extra_data`. When the remote fetch fails, `load_extra_data` returns only bundled data (no connections), erasing any connection data that was successfully loaded on the previous tick. The old code left state untouched on failure; this change removes that safety net. After a single failed refresh, all territory connection data from the remote feed disappears until the next successful fetch.
How can I resolve this? If you propose a fix, please make it concise.| fn merge_extra_data( | ||
| target: &mut HashMap<String, ExtraTerrInfo>, | ||
| source: HashMap<String, ExtraTerrInfo>, | ||
| ) { | ||
| for (name, info) in source { | ||
| let entry = target.entry(name).or_default(); | ||
| if !info.resources.is_empty() { | ||
| entry.resources = info.resources; | ||
| } | ||
| if !info.connections.is_empty() { | ||
| entry.connections = info.connections; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bundled data permanently overrides remote resource values
merge_extra_data is called with target = remote and source = bundled, so bundled resource values unconditionally replace whatever the remote feed provides for any Fruma territory. If the supplemental feed carries more up-to-date resource counts for those territories (e.g., after a game balance patch), the bundled values will silently shadow them. The PR description frames bundled data as filling in resources "without waiting on the supplemental feed," which suggests the intent is to provide defaults — but the current merge direction makes bundled values sticky overrides instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/extra_data_loader.rs
Line: 50-63
Comment:
**Bundled data permanently overrides remote resource values**
`merge_extra_data` is called with `target = remote` and `source = bundled`, so bundled resource values unconditionally replace whatever the remote feed provides for any Fruma territory. If the supplemental feed carries more up-to-date resource counts for those territories (e.g., after a game balance patch), the bundled values will silently shadow them. The PR description frames bundled data as filling in resources "without waiting on the supplemental feed," which suggests the intent is to provide defaults — but the current merge direction makes bundled values sticky overrides instead.
How can I resolve this? If you propose a fix, please make it concise.
Summary
TERRITORY_EXTRA_URL, including an opt-out value for deployments that do not want to fetch it.Validation
cargo test -p sequoia-server extra_data_loader -- --nocapturecargo check -p sequoia-serverGreptile Summary
This PR bundles Fruma territory resource metadata directly into the server so map territories render resource highlights immediately on startup, without waiting for the hourly supplemental feed. It also makes the legacy supplemental feed URL configurable via
TERRITORY_EXTRA_URLwith explicit opt-out support.merge_extra_dataapplies them only when the remote feed has no resources for a territory, so live feed values always take precedence.TERRITORY_EXTRA_URLconfig: Supportsnone/0/false/offopt-out values; defaults to the legacy gist URL when unset.Confidence Score: 5/5
Safe to merge; the bundled data fills gaps without displacing live feed values, and the error path now preserves cached state rather than overwriting it.
All core changes — merge semantics, error-path state preservation, and env-var opt-out — behave correctly and are backed by targeted tests. The only gap is a missing warn! when TERRITORY_EXTRA_URL contains non-unicode bytes, which is a silent no-op that could confuse operators but does not affect correctness.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[run loop tick] --> B[load_extra_data] B --> C{territory_extra_url} C -- Some url --> D[fetch_extra_data\nHTTP GET + error_for_status] C -- None opt-out --> E[empty HashMap] D -- Ok --> F[remote data] D -- Err --> G[propagate error] F --> H[merge_extra_data\ntarget=remote, source=bundled] E --> H H --> I[Ok data] I --> J[overwrite state.extra_terr\nset dirty flag] G --> K[error path:\nwrite-lock existing state\nmerge bundled into cache\nset dirty flag\nwarn log] subgraph merge_extra_data logic M[for each territory in source] M --> N{target.resources is_empty?} N -- yes --> O[copy source resources] N -- no --> P[keep target resources] M --> Q{source.connections non-empty?} Q -- yes --> R[override connections] Q -- no --> S[keep target connections] endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(server): preserve territory extra op..." | Re-trigger Greptile