Self-review: removing one chunk from a big selection deleted 143 others (#144)

`normalizeRects` sliced its INPUT at 256 rectangles, and the removal path
expanded the selection to one rectangle per chunk before filtering. So a
20x20 region — one rectangle, 400 chunks — became 400 rectangles, then
399, then the first 256. A hundred and forty-three chunks disappeared
with no error, no warning, and nothing to say which ones.

The first test used a row of four chunks, which is exactly why it stayed
green. Restoring the slice now fails with "removing one chunk from 400
left 256".

Removal no longer expands anything. It splits the rectangle AROUND the
chunk into the at most four pieces beside it, which touches only the
rectangle involved and cannot grow the list by more than three, whatever
the selection's size. The test now removes an interior chunk, a corner,
an edge, the only chunk, and one that was never selected — and checks
every one of the other 399 is covered exactly once, so a split that
overlapped would fail too.

The same slice had two more consequences, both fixed by refusing instead
of trimming:

  An API caller who POSTed a hundred scattered chunks got a 200 with
  sixty-four of them stored. `checkArea` counted the chunks and the
  emptiness but never noticed rectangles had been dropped. There is now
  a named `too-many-rects`, documented beside `too-many-chunks`, and the
  cap is checked before normalising as well as after — a shape too
  complex to store is refused, never silently trimmed. Three hundred
  chunks in a ROW still pass, because they merge to one rectangle.

  The panel's own tidy had no cap at all, so above 256 it and the app
  disagreed about what had been selected. The panel now splits the same
  way, and the smoke checks its answer against `subtractChunk`'s on the
  400-chunk case rather than only on four.

Also: the desktop editor was permanently open, against its own comment
saying it should not be — drawing areas and editing them are two
decisions, which is how the web panel already had it. And
`forgetServerAreas` was written, documented as "called when a server is
forgotten", and called by nothing; `removeServer` calls it now, so a
later server issued the same id cannot inherit someone else's
annotations.

Verified: 12/12 gates.

Author: CaYatur

docs/openapi.json

@@ -2887,7 +2887,7 @@
       "post": {
         "operationId": "postServersIdAreas",
         "summary": "Create an area, or edit one by passing `areaId`.",
-        "description": "Scope `settings` on the server.\n\nBody fields:\n- `areaId` — Omit to create; pass an existing id to replace that area.\n- `name` — Shown on hover and on click. 1-48 characters.\n- `note` — The second line of the tooltip, e.g. who owns the area. Up to 280 characters.\n- `colour` — `#rrggbb` (`#rgb` is expanded). Anything else falls back to the first palette colour.\n- `dim` — `overworld` | `nether` | `end`, or a modded key. An area belongs to exactly one.\n- `rects` — Array of `{x1,z1,x2,z2}` in CHUNK coordinates, inclusive, corners in any order.\n- `hidden` — Keep it off every map but the operator's own.\n\nRectangles are tidied on the way in: duplicates and contained rects are dropped and neighbours that share a full edge are merged, so the stored shape covers exactly the chunks you sent. Refusals name themselves: `name-required`, `name-too-long`, `note-too-long`, `no-chunks`, `too-many-chunks` (max 65536), `too-many-areas` (max 200), `area-not-found`.",
+        "description": "Scope `settings` on the server.\n\nBody fields:\n- `areaId` — Omit to create; pass an existing id to replace that area.\n- `name` — Shown on hover and on click. 1-48 characters.\n- `note` — The second line of the tooltip, e.g. who owns the area. Up to 280 characters.\n- `colour` — `#rrggbb` (`#rgb` is expanded). Anything else falls back to the first palette colour.\n- `dim` — `overworld` | `nether` | `end`, or a modded key. An area belongs to exactly one.\n- `rects` — Array of `{x1,z1,x2,z2}` in CHUNK coordinates, inclusive, corners in any order.\n- `hidden` — Keep it off every map but the operator's own.\n\nRectangles are tidied on the way in: duplicates and contained rects are dropped and neighbours that share a full edge are merged, so the stored shape covers exactly the chunks you sent. Refusals name themselves: `name-required`, `name-too-long`, `note-too-long`, `no-chunks`, `too-many-chunks` (max 65536), `too-many-rects` (a shape of more than 64 separate pieces after tidying, or more than 1024 sent), `too-many-areas` (max 200), `area-not-found`. A shape too complex to store is refused, never silently trimmed.",
         "tags": [
           "areas"
         ],

src/main/core/serverRegistry.ts

@@ -6,6 +6,7 @@ import { resolveBaseDir, dataDir } from '../paths'
 import { detectServer } from './serverDetect'
 import * as metrics from './metrics'
 import * as events from './events'
+import * as chunkAreas from './chunkAreas'
 import { log } from '../logger'
 import { PROXY_TYPES } from '@shared/types'
 import type { ServerConfig, ServerType, JavaArgsConfig } from '@shared/types'
@@ -119,6 +120,10 @@ export function removeServer(id: string, deleteFiles: boolean): void {
   })
   metrics.dropServer(id)
   events.dropServer(id)
+  // Areas are keyed by server id and live in their own file, so nothing else
+  // removes them. A later server issued the same id would inherit somebody
+  // else's map annotations.
+  chunkAreas.forgetServerAreas(id)
   if (deleteFiles && target && existsSync(target.path)) {
     try {
       rmSync(target.path, { recursive: true, force: true })

src/main/smoke.ts

@@ -2279,6 +2279,67 @@ export async function runWorldsSmoke(): Promise<void> {
         return fail('two diagonal chunks were merged into one rect')
       }
 
+      // Removing ONE chunk from a big selection. This is the case the first
+      // version got wrong and no test caught: a 20x20 region is a single rect,
+      // and expanding it to one rect per chunk to filter produced 400 — past
+      // the input ceiling, so 143 chunks vanished with no error at all. The
+      // fix splits the rectangle around the chunk instead, so the size of the
+      // selection cannot matter.
+      {
+        const big = areasMod.normalizeRects(at([[0, 0, 19, 19]]))
+        if (big.length !== 1 || areasMod.areaChunkCount({ rects: big }) !== 400) {
+          return fail('a 20x20 selection is not one 400-chunk rect')
+        }
+        const cut = areasMod.subtractChunk(big, 10, 10)
+        if (areasMod.areaChunkCount({ rects: cut }) !== 399) {
+          return fail('removing one chunk from 400 left ' + areasMod.areaChunkCount({ rects: cut }))
+        }
+        if (areasMod.areaHas({ rects: cut }, 10, 10)) return fail('the removed chunk is still covered')
+        // Every other chunk survives, and none is covered twice — a split that
+        // overlaps would make the area larger than the shape it draws.
+        for (let x = 0; x <= 19; x++) {
+          for (let z = 0; z <= 19; z++) {
+            const hits = cut.filter((r) => areasMod.rectHas(r, x, z)).length
+            const want = x === 10 && z === 10 ? 0 : 1
+            if (hits !== want) return fail('chunk ' + x + ',' + z + ' is covered ' + hits + ' times')
+          }
+        }
+        // Removing a corner, an edge and the last chunk of all.
+        if (areasMod.areaChunkCount({ rects: areasMod.subtractChunk(big, 0, 0) }) !== 399) {
+          return fail('removing the corner went wrong')
+        }
+        if (areasMod.areaChunkCount({ rects: areasMod.subtractChunk(big, 19, 5) }) !== 399) {
+          return fail('removing an edge chunk went wrong')
+        }
+        const one = areasMod.normalizeRects(at([[7, 7, 7, 7]]))
+        if (areasMod.subtractChunk(one, 7, 7).length !== 0) return fail('removing the only chunk left something')
+        // A chunk that was never in the selection changes nothing.
+        if (areasMod.areaChunkCount({ rects: areasMod.subtractChunk(big, 99, 99) }) !== 400) {
+          return fail('removing an unselected chunk changed the selection')
+        }
+      }
+
+      // Too many pieces is REFUSED, not trimmed. `normalizeRects` used to slice
+      // its input, so an API caller who sent a hundred scattered chunks got a
+      // 200 and lost most of them — with nothing to say which.
+      {
+        const scattered = at(
+          Array.from({ length: areasMod.MAX_RECTS_PER_AREA + 20 }, (_, i) => [i * 2, 0, i * 2, 0])
+        )
+        const c = areasMod.checkArea({ name: 'swiss cheese', rects: scattered })
+        if (c.ok) return fail('a shape with too many pieces was accepted')
+        if (c.error !== 'too-many-rects') return fail('wrong reason: ' + c.error)
+        const flood = at(Array.from({ length: areasMod.MAX_INPUT_RECTS + 1 }, (_, i) => [i * 2, 0, i * 2, 0]))
+        const c2 = areasMod.checkArea({ name: 'flood', rects: flood })
+        if (c2.ok || c2.error !== 'too-many-rects') return fail('a flood of rects was not refused: ' + JSON.stringify(c2))
+        // ...and a shape that merges down to few enough pieces is still fine,
+        // however many rectangles it arrived as.
+        const contiguous = at(Array.from({ length: 300 }, (_, i) => [i, 0, i, 0]))
+        const c3 = areasMod.checkArea({ name: 'a long road', rects: contiguous })
+        if (!c3.ok) return fail('300 chunks in a row were refused: ' + c3.error)
+        if (c3.value.rects.length !== 1) return fail('300 chunks in a row did not merge to one rect')
+      }
+
       // Dimension scoping. Without it, an area drawn in the overworld paints the
       // same rectangle over the nether, where it means nothing.
       const over = mk({ id: 'o', rects: at([[0, 0, 9, 9]]) })
@@ -6754,6 +6815,7 @@ export async function runWebSmoke(): Promise<void> {
           if (areasMod.areaChunkCount({ rects: pnl.AREA_PICK }) !== 4) return fail('the picker lost a chunk')
           // Taking one out of the MIDDLE is the case that matters: the rect it
           // sits in covers three others, and dropping the rect drops them too.
+          // The panel splits the rectangle, the same as the app does.
           pctx['areaPickChunk'](1, 0)
           if (areasMod.areaChunkCount({ rects: pnl.AREA_PICK }) !== 3) {
             return fail('removing one chunk took ' + (4 - areasMod.areaChunkCount({ rects: pnl.AREA_PICK })) + ' with it')
@@ -6781,6 +6843,19 @@ export async function runWebSmoke(): Promise<void> {
           if (canon(theirsTidy) !== canon(mineTidy)) {
             return fail('the panel tidies differently: ' + canon(theirsTidy) + ' vs ' + canon(mineTidy))
           }
+          // And the panel must survive the big-selection case too — its own
+          // removal is a second implementation, so it gets the same test.
+          pnl.AREA_PICK = [{ x1: 0, z1: 0, x2: 19, z2: 19 }]
+          pctx['areaPickChunk'](10, 10)
+          if (areasMod.areaChunkCount({ rects: pnl.AREA_PICK }) !== 399) {
+            return fail(
+              'the panel lost chunks removing one from 400: ' +
+              areasMod.areaChunkCount({ rects: pnl.AREA_PICK })
+            )
+          }
+          if (canon(pnl.AREA_PICK) !== canon(areasMod.subtractChunk([{ x1: 0, z1: 0, x2: 19, z2: 19 }], 10, 10))) {
+            return fail('the panel splits a rectangle differently from the app')
+          }
           pnl.AREA_PICKING = false
           pnl.AREA_PICK = []
         }

src/main/web/panelHtml.ts

@@ -991,13 +991,18 @@ function areaPickChunk(cx,cz){
  for(var i=0;i<AREA_PICK.length;i++){var r=AREA_PICK[i];
   if(cx>=r.x1&&cx<=r.x2&&cz>=r.z1&&cz<=r.z2)has=true}
  if(has){
-  /* Expand and drop the one chunk. Removing the whole rectangle that happens to
-     contain it would throw away the forty others it was merged with. */
-  var flat=[];
+  /* Split the rectangle AROUND the chunk. Expanding the selection to one rect
+     per chunk and filtering was the first version, and it lost data on any
+     selection past a few hundred chunks. Splitting touches only the rectangle
+     involved and can add at most three. */
+  var cut=[];
   for(var j=0;j<AREA_PICK.length;j++){var q=AREA_PICK[j];
-   for(var x=q.x1;x<=q.x2;x++)for(var z=q.z1;z<=q.z2;z++)
-    if(!(x===cx&&z===cz))flat.push({x1:x,z1:z,x2:x,z2:z})}
-  AREA_PICK=areaTidy(flat)}
+   if(!(cx>=q.x1&&cx<=q.x2&&cz>=q.z1&&cz<=q.z2)){cut.push(q);continue}
+   if(cx>q.x1)cut.push({x1:q.x1,z1:q.z1,x2:cx-1,z2:q.z2});
+   if(cx<q.x2)cut.push({x1:cx+1,z1:q.z1,x2:q.x2,z2:q.z2});
+   if(cz>q.z1)cut.push({x1:cx,z1:q.z1,x2:cx,z2:cz-1});
+   if(cz<q.z2)cut.push({x1:cx,z1:cz+1,x2:cx,z2:q.z2})}
+  AREA_PICK=areaTidy(cut)}
  else AREA_PICK=areaTidy(AREA_PICK.concat([{x1:cx,z1:cz,x2:cx,z2:cz}]));
  areaCountText();mapDraw()}
 function areaAddTyped(){

src/renderer/src/components/LiveMap.tsx

@@ -14,7 +14,7 @@ import {
   areasFor,
   chunkOf,
   normalizeRects,
-  expandRects,
+  subtractChunk,
   parseChunkInput,
   checkArea,
   areaChunkCount,
@@ -232,6 +232,8 @@ export function LiveMap({ serverId }: { serverId: string }): JSX.Element {
   // label the operator wrote for people to read, so hiding it defeats the point.
   const [areas, setAreas] = useState<ChunkArea[]>([])
   const [showAreas, setShowAreas] = useState(true)
+  // Drawing them is on; the editor is not. Two separate decisions.
+  const [showAreaCard, setShowAreaCard] = useState(false)
   const [pinned, setPinned] = useState<ChunkArea | null>(null)
   const [editing, setEditing] = useState<ChunkArea | 'new' | null>(null)
   // Chunks picked by clicking, while the picker is open. A mode rather than a
@@ -687,14 +689,32 @@ export function LiveMap({ serverId }: { serverId: string }): JSX.Element {
         <button className={`btn sm ${showAreas ? 'primary' : ''}`} onClick={() => setShowAreas((v) => !v)}>
           <Shapes size={13} /> {t('map.areas')}
         </button>
+        <button
+          className={`btn sm ${showAreaCard ? 'primary' : ''}`}
+          onClick={() => {
+            const next = !showAreaCard
+            setShowAreaCard(next)
+            // Editing implies looking. Opening the editor over a map that is not
+            // drawing areas would be picking chunks against an invisible shape.
+            if (next) setShowAreas(true)
+            else {
+              setEditing(null)
+              setPicking(false)
+              setPicked([])
+            }
+          }}
+        >
+          {t('map.areaEdit')}
+        </button>
         <button className={`btn sm ${showPerf ? 'primary' : ''}`} onClick={() => setShowPerf((v) => !v)}>
           <Gauge size={13} /> {t('map.performance')}
         </button>
       </div>
 
-      {/* The area editor. Open only when asked for: the map is for looking at,
-          and a permanently visible editing panel would take a third of it. */}
-      {showAreas && (
+      {/* Open only when asked for: the map is for looking at, and a permanently
+          visible editing panel takes a third of it. Drawing areas and EDITING
+          them are two decisions — the web panel keeps them apart the same way. */}
+      {showAreaCard && (
         <div className="card" style={{ padding: 12, display: 'grid', gap: 8 }}>
           <div style={{ display: 'flex', alignItems: 'center', gap: 8, flexWrap: 'wrap' }}>
             <b style={{ fontSize: 13 }}>{t('map.areas')}</b>
@@ -905,12 +925,10 @@ export function LiveMap({ serverId }: { serverId: string }): JSX.Element {
               const has = picked.some((r) => c.cx >= r.x1 && c.cx <= r.x2 && c.cz >= r.z1 && c.cz <= r.z2)
               setPicked(
                 has
-                  ? // Rects are merged on the way in, so removing a chunk means
-                    // re-deriving the selection without it rather than dropping
-                    // whichever rectangle happens to contain it.
-                    normalizeRects(
-                      expandRects(picked).filter((p) => !(p.x1 === c.cx && p.z1 === c.cz))
-                    )
+                  ? // Splits the rectangle around the chunk rather than dropping
+                    // it: rects are merged on the way in, so the one under the
+                    // pointer usually covers dozens of others.
+                    subtractChunk(picked, c.cx, c.cz)
                   : normalizeRects([...picked, { x1: c.cx, z1: c.cz, x2: c.cx, z2: c.cz }])
               )
               return

src/renderer/src/locales/en.ts

@@ -147,6 +147,8 @@ export default {
     allStructures: 'All structures',
     areas: 'Areas',
     areasCount: '{{n}} in {{dim}}',
+    areaEdit: 'Edit areas',
+    'areaErr_too-many-rects': 'That shape has too many separate pieces — join some up or make fewer areas.',
     areaNew: 'New area',
     areaName: 'Area name, e.g. spawn town',
     areaNote: 'Note — shown under the name, e.g. owner: CaYatur',

src/renderer/src/locales/tr.ts

@@ -149,6 +149,8 @@ const tr: typeof en = {
     allStructures: 'Tüm yapılar',
     areas: 'Alanlar',
     areasCount: '{{dim}} içinde {{n}} tane',
+    areaEdit: 'Alanları düzenle',
+    'areaErr_too-many-rects': 'Bu şekil çok fazla ayrı parçadan oluşuyor — birleştirin ya da daha az alan yapın.',
     areaNew: 'Yeni alan',
     areaName: 'Alan adı, örn. spawn kasabası',
     areaNote: 'Ek açıklama — adın altında görünür, örn. bu alan sahibi: CaYatur',

src/shared/apiSurface.ts

@@ -219,7 +219,7 @@ export const API_ROUTES: ApiRoute[] = [
       rects: 'Array of `{x1,z1,x2,z2}` in CHUNK coordinates, inclusive, corners in any order.',
       hidden: 'Keep it off every map but the operator\'s own.'
     },
-    notes: 'Rectangles are tidied on the way in: duplicates and contained rects are dropped and neighbours that share a full edge are merged, so the stored shape covers exactly the chunks you sent. Refusals name themselves: `name-required`, `name-too-long`, `note-too-long`, `no-chunks`, `too-many-chunks` (max 65536), `too-many-areas` (max 200), `area-not-found`.'
+    notes: 'Rectangles are tidied on the way in: duplicates and contained rects are dropped and neighbours that share a full edge are merged, so the stored shape covers exactly the chunks you sent. Refusals name themselves: `name-required`, `name-too-long`, `note-too-long`, `no-chunks`, `too-many-chunks` (max 65536), `too-many-rects` (a shape of more than 64 separate pieces after tidying, or more than 1024 sent), `too-many-areas` (max 200), `area-not-found`. A shape too complex to store is refused, never silently trimmed.'
   },
   { method: 'DELETE', path: '/servers/{id}/areas', gate: 'settings', group: 'areas', summary: 'Delete one area.', params: [serverId, { name: 'areaId', in: 'query', required: true, description: 'Area to delete.' }] },