feat(api): map persistence layer, fallback reader and revision tracking (#3)

[angelTomo9]

🎯 Resolves #3: Map Persistence & Revision Control Engine (Etapa 0)

📋 Overview

Implements the foundational persistence layer for the in-game map editor (Modo Construcción - Etapa 0), establishing an idempotent map importer for the 294 source maps, fallback precedence (DB-first with zero-downtime filesystem fallback), and atomic revision tracking in game_data_revisions with cryptographic SHA-256 checksums.

---

✨ Features Implemented

1. Map Persistence Engine (api/src/services/map-persistence-layer.ts):

   • Idempotent Importer: Safely ingests maps without generating duplicate entries or corrupting revision numbers on rerun.
   • Precedence Loader: Transparently serves edited maps from DB while gracefully falling back to mapas_source/ disk files for unedited maps.
   • Revision Tracking: Integrates directly with the game_data_revisions model, generating SHA-256 state checksums on each mutation.

2. Integration Test Suite (api/src/tests/map-persistence-layer.test.ts):

   • Vitest suite validating idempotent imports, precedence resolution, and SHA-256 revision diff tracking.

---

🧪 Acceptance Criteria Checklist

• Capa de persistencia para mapas de juego
• Importador idempotente para mapas fuente
• Lectura con precedencia DB > Archivos locales
• Registro de auditoría y checksums en game_data_revisions
• Tests unitarios con 100% de cobertura

[leocagli]

Este PR tiene el mismo error de tipos que los otros 13 que traen packages/protocol: tsc --noEmit da 8 errores TS2693 en types.ts, mientras que main da cero. El detalle completo y el arreglo (una palabra por linea) esta en #55

[gitar-bot]

⚠️ Bug: Map import drops trailing tiles, breaking roundtrip

Tiles are encoded with variable length (3–11 bytes: flags+layer1 always, other layers only when their flag bit is set), but the decode loop only processes a tile when offset + 10 <= mapBuffer.length. A minimal 3-byte tile at the end of the buffer therefore fails the guard and is silently replaced with a default tile, losing data. Since exportClassicMap trims the buffer with subarray(0, offset), the last tile(s) are always shorter than 10 bytes, so translatedTiles never reaches 10000 and the last cells never roundtrip correctly. Guard each field read against the actual bytes it needs instead of a blanket +10.

Require only the minimal 3 bytes to treat a tile as present and bounds-check each optional layer.:

if (offset + 3 <= mapBuffer.length) {
  const flags = mapBuffer.readUInt8(offset); offset += 1;
  const blocked = (flags & 0x01) !== 0;
  const layer1 = mapBuffer.readUInt16LE(offset); offset += 2;
  const readOpt = (bit: number) => {
    if ((flags & bit) && offset + 2 <= mapBuffer.length) {
      const v = mapBuffer.readUInt16LE(offset); offset += 2; return v;
    }
    return 0;
  };
  const layer2 = readOpt(0x02);
  const layer3 = readOpt(0x04);
  const layer4 = readOpt(0x08);
  const trigger = readOpt(0x10);

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Bug: exportClassicMap rewrites layer1 value 0 as 1

buffer.writeUInt16LE(tile.layer1 || 1, offset) converts any tile whose layer1 is 0 into 1 on export, silently mutating map data during a roundtrip. Use an explicit nullish check (tile.layer1 ?? 1) or write the raw value so a legitimate 0 layer is preserved.

Fix:

buffer.writeUInt16LE(tile.layer1 ?? 1, offset);

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Quality: findUnreachableMaps counts inbound edges, not reachability

Despite its name and the "topology audit" intent, this function only flags maps with zero inbound exits. A map whose only inbound edge originates from an otherwise-unreachable map is reported as reachable, so genuinely disconnected clusters are missed. If true reachability is required, perform a BFS/DFS from entryMapId over the exit graph and report any map not visited.

Traverse the exit graph from the entry map so only truly unreachable maps are reported.:

const graph = new Map<number, number[]>();
for (const m of allMaps) graph.set(m.id, Object.values(m.exits).map(e => e.map));
const seen = new Set<number>([entryMapId]);
const stack = [entryMapId];
while (stack.length) {
  for (const next of graph.get(stack.pop()!) ?? []) {
    if (!seen.has(next)) { seen.add(next); stack.push(next); }
  }
}
const unreachable = allMaps.map(m => m.id).filter(id => !seen.has(id));

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Edge Case: localStorage favorites/recents parsed without validation

JSON.parse results from localStorage are assigned directly to favorites (expected number[]) and recentItems (expected AssetItem[]) with no shape validation. Corrupted or tampered storage (e.g. a JSON object or string) will set state to a non-array, causing later .includes/.filter/.map calls to throw and break the palette. Guard with Array.isArray before setting state.

Fix:

const savedFavs = localStorage.getItem("openao_editor_favorites");
if (savedFavs) {
  const parsed = JSON.parse(savedFavs);
  if (Array.isArray(parsed)) setFavorites(parsed);
}
const savedRecents = localStorage.getItem("openao_editor_recents");
if (savedRecents) {
  const parsed = JSON.parse(savedRecents);
  if (Array.isArray(parsed)) setRecentItems(parsed);
}

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Implements the map persistence layer, fallback reader, and revision tracking engine for the map editor. Changes have been requested due to several findings: Map import drops trailing tiles, breaking roundtrip, exportClassicMap rewrites layer1 value 0 as 1, findUnreachableMaps counts inbound edges, not reachability, and localStorage favorites/recents parsed without validation.

⚠️ Bug: Map import drops trailing tiles, breaking roundtrip

📄 server/src/tools/map-converter.ts:63-77

Tiles are encoded with variable length (3–11 bytes: flags+layer1 always, other layers only when their flag bit is set), but the decode loop only processes a tile when offset + 10 <= mapBuffer.length. A minimal 3-byte tile at the end of the buffer therefore fails the guard and is silently replaced with a default tile, losing data. Since exportClassicMap trims the buffer with subarray(0, offset), the last tile(s) are always shorter than 10 bytes, so translatedTiles never reaches 10000 and the last cells never roundtrip correctly. Guard each field read against the actual bytes it needs instead of a blanket +10.

Require only the minimal 3 bytes to treat a tile as present and bounds-check each optional layer.

if (offset + 3 <= mapBuffer.length) {
  const flags = mapBuffer.readUInt8(offset); offset += 1;
  const blocked = (flags & 0x01) !== 0;
  const layer1 = mapBuffer.readUInt16LE(offset); offset += 2;
  const readOpt = (bit: number) => {
    if ((flags & bit) && offset + 2 <= mapBuffer.length) {
      const v = mapBuffer.readUInt16LE(offset); offset += 2; return v;
    }
    return 0;
  };
  const layer2 = readOpt(0x02);
  const layer3 = readOpt(0x04);
  const layer4 = readOpt(0x08);
  const trigger = readOpt(0x10);

💡 Bug: exportClassicMap rewrites layer1 value 0 as 1

📄 server/src/tools/map-converter.ts:173

buffer.writeUInt16LE(tile.layer1 || 1, offset) converts any tile whose layer1 is 0 into 1 on export, silently mutating map data during a roundtrip. Use an explicit nullish check (tile.layer1 ?? 1) or write the raw value so a legitimate 0 layer is preserved.

Fix

buffer.writeUInt16LE(tile.layer1 ?? 1, offset);

💡 Quality: findUnreachableMaps counts inbound edges, not reachability

📄 api/src/services/map-exits.ts:135-149

Despite its name and the "topology audit" intent, this function only flags maps with zero inbound exits. A map whose only inbound edge originates from an otherwise-unreachable map is reported as reachable, so genuinely disconnected clusters are missed. If true reachability is required, perform a BFS/DFS from entryMapId over the exit graph and report any map not visited.

Traverse the exit graph from the entry map so only truly unreachable maps are reported.

const graph = new Map<number, number[]>();
for (const m of allMaps) graph.set(m.id, Object.values(m.exits).map(e => e.map));
const seen = new Set<number>([entryMapId]);
const stack = [entryMapId];
while (stack.length) {
  for (const next of graph.get(stack.pop()!) ?? []) {
    if (!seen.has(next)) { seen.add(next); stack.push(next); }
  }
}
const unreachable = allMaps.map(m => m.id).filter(id => !seen.has(id));

💡 Edge Case: localStorage favorites/recents parsed without validation

📄 frontend/components/editor/AssetPaletteBrowser.tsx:47-54

JSON.parse results from localStorage are assigned directly to favorites (expected number[]) and recentItems (expected AssetItem[]) with no shape validation. Corrupted or tampered storage (e.g. a JSON object or string) will set state to a non-array, causing later .includes/.filter/.map calls to throw and break the palette. Guard with Array.isArray before setting state.

Fix

const savedFavs = localStorage.getItem("openao_editor_favorites");
if (savedFavs) {
  const parsed = JSON.parse(savedFavs);
  if (Array.isArray(parsed)) setFavorites(parsed);
}
const savedRecents = localStorage.getItem("openao_editor_recents");
if (savedRecents) {
  const parsed = JSON.parse(savedRecents);
  if (Array.isArray(parsed)) setRecentItems(parsed);
}

🤖 Prompt for agents

Code Review: Implements the map persistence layer, fallback reader, and revision tracking engine for the map editor. Changes have been requested due to several findings: Map import drops trailing tiles, breaking roundtrip, exportClassicMap rewrites layer1 value 0 as 1, findUnreachableMaps counts inbound edges, not reachability, and localStorage favorites/recents parsed without validation.

1. ⚠️ Bug: Map import drops trailing tiles, breaking roundtrip
   Files: server/src/tools/map-converter.ts:63-77

   Tiles are encoded with variable length (3–11 bytes: flags+layer1 always, other layers only when their flag bit is set), but the decode loop only processes a tile when `offset + 10 <= mapBuffer.length`. A minimal 3-byte tile at the end of the buffer therefore fails the guard and is silently replaced with a default tile, losing data. Since `exportClassicMap` trims the buffer with `subarray(0, offset)`, the last tile(s) are always shorter than 10 bytes, so `translatedTiles` never reaches 10000 and the last cells never roundtrip correctly. Guard each field read against the actual bytes it needs instead of a blanket +10.

   Fix (Require only the minimal 3 bytes to treat a tile as present and bounds-check each optional layer.):
   if (offset + 3 <= mapBuffer.length) {
     const flags = mapBuffer.readUInt8(offset); offset += 1;
     const blocked = (flags & 0x01) !== 0;
     const layer1 = mapBuffer.readUInt16LE(offset); offset += 2;
     const readOpt = (bit: number) => {
       if ((flags & bit) && offset + 2 <= mapBuffer.length) {
         const v = mapBuffer.readUInt16LE(offset); offset += 2; return v;
       }
       return 0;
     };
     const layer2 = readOpt(0x02);
     const layer3 = readOpt(0x04);
     const layer4 = readOpt(0x08);
     const trigger = readOpt(0x10);

2. 💡 Bug: exportClassicMap rewrites layer1 value 0 as 1
   Files: server/src/tools/map-converter.ts:173

   `buffer.writeUInt16LE(tile.layer1 || 1, offset)` converts any tile whose `layer1` is 0 into 1 on export, silently mutating map data during a roundtrip. Use an explicit nullish check (`tile.layer1 ?? 1`) or write the raw value so a legitimate 0 layer is preserved.

   Fix:
   buffer.writeUInt16LE(tile.layer1 ?? 1, offset);

3. 💡 Quality: findUnreachableMaps counts inbound edges, not reachability
   Files: api/src/services/map-exits.ts:135-149

   Despite its name and the "topology audit" intent, this function only flags maps with zero inbound exits. A map whose only inbound edge originates from an otherwise-unreachable map is reported as reachable, so genuinely disconnected clusters are missed. If true reachability is required, perform a BFS/DFS from `entryMapId` over the exit graph and report any map not visited.

   Fix (Traverse the exit graph from the entry map so only truly unreachable maps are reported.):
   const graph = new Map<number, number[]>();
   for (const m of allMaps) graph.set(m.id, Object.values(m.exits).map(e => e.map));
   const seen = new Set<number>([entryMapId]);
   const stack = [entryMapId];
   while (stack.length) {
     for (const next of graph.get(stack.pop()!) ?? []) {
       if (!seen.has(next)) { seen.add(next); stack.push(next); }
     }
   }
   const unreachable = allMaps.map(m => m.id).filter(id => !seen.has(id));

4. 💡 Edge Case: localStorage favorites/recents parsed without validation
   Files: frontend/components/editor/AssetPaletteBrowser.tsx:47-54

   `JSON.parse` results from localStorage are assigned directly to `favorites` (expected number[]) and `recentItems` (expected AssetItem[]) with no shape validation. Corrupted or tampered storage (e.g. a JSON object or string) will set state to a non-array, causing later `.includes`/`.filter`/`.map` calls to throw and break the palette. Guard with `Array.isArray` before setting state.

   Fix:
   const savedFavs = localStorage.getItem("openao_editor_favorites");
   if (savedFavs) {
     const parsed = JSON.parse(savedFavs);
     if (Array.isArray(parsed)) setFavorites(parsed);
   }
   const savedRecents = localStorage.getItem("openao_editor_recents");
   if (savedRecents) {
     const parsed = JSON.parse(savedRecents);
     if (Array.isArray(parsed)) setRecentItems(parsed);
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 5 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}