# Notification Diff Batching Optimization

## Problem

`ActionResultFilter.filterForPlayer` generates per-action-result game state diffs, which is expensive. Profiling shows significant time spent in `filteredGameStateDiff` → `GameStateViewFilter.filteredGameState` (called twice per result) → `GameStateViewDiffer.diff`.

A potential optimization is to batch diffs: instead of computing N diffs for N action results, compute one combined diff representing the final state change. However, this is blocked by how client notification generators work.

## Current Architecture

### Server Side
1. `ActionResultFilter.filterForPlayer` processes each action result
2. For each result, computes `filteredGameStateDiff(before, after, factionId)`
3. Returns `Vector[ActionResultView]` where each view has its own `gameStateDiff`

### Client Side
1. `EagleGameModel.HandleNewHistoryEntry` processes each `ActionResultView`:
   ```csharp
   private void HandleNewHistoryEntry(ActionResultView arv) {
       _currentModel.HistoryCount++;
       MaybeSendNotification(arv);        // Uses currentModel state
       ApplyGameStateViewDiff(arv.GameStateDiff);  // Updates currentModel
   }
   ```

2. Notification generators receive `(ActionResultView, IGameModel)` and look up display data from the model:
   ```csharp
   var province = currentModel.Provinces[details.ProvinceId];
   var factionName = currentModel.FactionName(details.FactionId);
   var hero = currentModel.Heroes[heroId];
   var affectedProvinces = currentModel.ProvincesForFaction(factionId);
   ```

### The Problem
Notification for action N sees model state after actions 1..N-1 have been applied. If we batch diffs, notification N would see model state before ANY actions, potentially showing stale data.

Example:
1. Action 1: Province X conquered by Faction B (was Faction A)
2. Action 2: Notification needs to show Province X's current ruler

With batching, action 2's notification would incorrectly show Faction A.

## Audit of Client Notification Generators

~50 generators access `currentModel`. Key lookup patterns:

| Lookup | Count | Mutable? | Risk |
|--------|-------|----------|------|
| `currentModel.PlayerId` | 22 | No | Safe |
| `currentModel.Provinces[id]` | 17 | Yes | `RulingFactionId` changes on conquest |
| `currentModel.Heroes[id]` | ~20 | Mostly safe | Hero data stable, used for display |
| `currentModel.FactionName(id)` | ~10 | No | Names don't change |
| `currentModel.MaybeDestroyedFaction(id)` | ~15 | Yes | Faction may be destroyed |
| `currentModel.ProvincesForFaction(id)` | ~15 | Yes | Changes on conquest |

### State-Changing Action Types
- `ProvinceConquered` - changes province ownership
- `FactionDestroyed` - removes faction
- `FactionLeaderRemoved` - changes faction head

## Proposed Solution: Server-Side Display Data

Eliminate client model lookups by including all display data in server-generated notifications.

### Current Notification Structure
```scala
case class NotificationC(
    details: NotificationDetails,
    targetFactionIds: Vector[FactionId],
    affectedProvinceIds: Vector[ProvinceId],  // Already exists, underused
    affectedHeroIds: Vector[HeroId],          // Already exists, underused
    llm: NotificationT.Llm,
    deferred: Boolean
)
```

### Proposed Changes

**Option A: Add fields to NotificationC**
```scala
case class NotificationC(
    details: NotificationDetails,
    targetFactionIds: Vector[FactionId],
    affectedProvinceIds: Vector[ProvinceId],
    affectedHeroIds: Vector[HeroId],
    // New fields:
    factionNames: Map[FactionId, String],
    displayedHeroViews: Vector[HeroView],
    provinceNames: Map[ProvinceId, String],
    llm: NotificationT.Llm,
    deferred: Boolean
)
```

**Option B: Enrich each NotificationDetails type**
```scala
case class TruceAccepted(
    offeringFactionId: FactionId,
    offeringFactionName: String,      // New
    targetFactionId: FactionId,
    targetFactionName: String,        // New
    ambassadorHeroId: HeroId,
    ambassadorHeroView: HeroView      // New
)
```

### Implementation Steps

1. **Proto changes**: Add new fields to `Notification` message
2. **Server**: Populate display fields when creating notifications
3. **Client**: Update ~50 generators to use notification fields
4. **Test**: Add test that greps for `currentModel.` access in generators (excluding `PlayerId`)
5. **Server optimization**: With client decoupled from model state, batch diffs in `filterForPlayer`

### Trade-offs

**Pros:**
- Clean separation: server provides all display data
- Enables diff batching optimization
- Easier to reason about notification correctness
- Test can enforce the invariant

**Cons:**
- Larger notification messages (includes names, hero views)
- Proto changes required
- ~50 generators need updating (mechanical but tedious)
- Server must know what display data each notification type needs

## Alternative Approaches Considered

### A: Selective Per-Action Diffs
Only generate individual diffs for action results with notifications that need mutable model state. Requires tracking which notification types need which state.

**Rejected because:** Fragile; easy to add a new generator that breaks the invariant.

### B: State-Change-Triggered Diffs
If batch contains state-changing action types (ProvinceConquered, etc.), generate individual diffs from that point. Otherwise batch.

**Rejected because:** Still conservative; many batches would fall back to individual diffs.

## Status

**Deferred** - Current performance is acceptable. This doc captures the analysis for future reference if optimization becomes necessary.

## References

- `ActionResultFilter.scala` - Server-side filtering
- `EagleGameModel.cs` - Client-side model updates
- `Assets/Eagle/Notifications/` - All notification generators
- `NotificationT.scala` - Server notification types