From 193dfb9e947ee0c3a70c9b3c892b12faba9057fe Mon Sep 17 00:00:00 2001
From: Bryan Van Deusen <bryan.vandeusen@fabledsword.com>
Date: Mon, 31 Aug 2026 16:46:15 -0400
Subject: [PATCH] tags: renaming onto an existing tag merges them, and the
 older row survives
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The three surfaces did not agree on what renaming a tag onto a name another
one already holds should do, and none of the three answers was good.

I described this wrongly first time and the correction matters. The local
store does NOT silently create a duplicate: `idx_labels_name` is unique on
`lower(name)`, so the bare UPDATE in `rename_label` failed, and the user got
a raw SQLite "UNIQUE constraint failed" as their error message. The server
meanwhile answered 409 "a tag with that name already exists" — and only on
an EXACT match, because its constraint is on the raw name while every
client's index is on `lower(name)`.

That last part is the sharper bug. The server would happily hold "Groceries"
beside "groceries"; no synced client can store both. Creating that pair on
the web armed a pull that fails later, on a phone, in a path with no UI.

Operator's call: a rename onto an existing name means merge — typing an
existing tag's name onto this one says they are the same thing.

  * `store::rename_label` and the server's PATCH now implement one rule.
    THE OLDER ROW SURVIVES and takes the new spelling. Age rather than "the
    one that already held the name", so that renaming A→B and B→A land on
    the same survivor; otherwise the outcome depends on which way round
    someone typed it, and two devices tidying the same pair disagree about
    which id still exists. Ties go to the incumbent, so it stays
    deterministic.

  * The core reuses `merge_labels` rather than reimplementing the move. That
    is the only place that knows to mark every affected NOTE dirty before
    the delete cascades the membership rows away, which is what makes a
    merge reach the server at all.

  * The server's rename and its `/merge` route now share one `_merge_into`
    helper, for the same reason.

  * Both server lookups became case-INSENSITIVE, matching every client. The
    create path is included: it was the one actually minting the unstorable
    pair, so fixing only the rename would have left the door open.

  * The web asks before merging, naming both note counts. A merge cannot be
    undone by repeating it and is now reachable by a typo in a text field —
    the same reasoning as the delete confirmation in #2116. The confirmation
    lives in the shared store, so the desktop gets it too; the FFI does not
    ask, because that belongs to the surface with a person in front of it.

  * The web store detects the merge from the LIST, not the response: the
    survivor may be the row we asked to rename, so an unchanged id proves
    nothing.

Tests: three integration tests over a real database (both rename directions
land on the older row; a case-varied create returns the existing tag) and
two through the Android FFI, which is the binding the phone will use.

Also fixes a straggler from 8c7553d — the delete confirmation still said
"the label".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3MMqUtzX1TJgA1oypvm1c
---
 android/ffi/src/lib.rs        | 91 +++++++++++++++++++++++++++++++----
 core/src/local/store.rs       | 50 +++++++++++++++++++
 frontend/src/stores/labels.ts | 30 +++++++++++-
 src/thoughtsync/labels.py     | 81 +++++++++++++++++++++++--------
 tests/test_integration.py     | 87 +++++++++++++++++++++++++++++++++
 5 files changed, 307 insertions(+), 32 deletions(-)

diff --git a/android/ffi/src/lib.rs b/android/ffi/src/lib.rs
index 96cff38..6cdffb4 100644
--- a/android/ffi/src/lib.rs
+++ b/android/ffi/src/lib.rs
@@ -336,17 +336,19 @@ impl ThoughtSync {
     /// Rename a label. Every note carrying it follows, because notes reference it
     /// by id and never by name.
     ///
-    /// Renaming onto a name that already exists does NOT merge, and does not fail
-    /// either — the core's find-or-create matching only runs on the create path, so
-    /// this leaves two labels whose names differ by case at most.
+    /// Renaming onto a name another tag already holds MERGES the two, and the OLDER
+    /// row is the survivor — it keeps its id and colour and takes the new spelling.
+    /// Matching is case-insensitive, like `find_or_create_label`.
     ///
-    /// The SERVER disagrees: `labels.py` answers that PATCH with 409 "a label with
-    /// that name already exists". So the local stores (this and the desktop, which
-    /// calls the same `store::rename_label`) are more permissive than the REST path
-    /// the web uses, and a duplicate made offline will meet that 409 on sync. Not
-    /// introduced here — it predates Android having a rename at all — but a UI over
-    /// this has to decide what it shows, so it is written down rather than found.
-    /// Merging is the deliberate, irreversible operation and stays a separate button.
+    /// So this call can return a label whose id is NOT the one passed in, and it can
+    /// make another label stop existing. A UI over it should say so before calling:
+    /// the merge cannot be undone by repeating it, and here it is reachable by a
+    /// typo in a text field. The web asks first (`stores/labels.ts`); this binding
+    /// deliberately does not, because a confirmation belongs to the surface that has
+    /// a person in front of it, not to the store.
+    ///
+    /// `store::rename_label` and the server's PATCH implement the same rule, so the
+    /// phone, the desktop and the web agree on which row survives.
     pub fn rename_label(&self, id: String, name: String) -> Result<Label, CoreError> {
         let conn = self.db.conn().map_err(CoreError::store)?;
         local::store::rename_label(&conn, &id, &name)
@@ -963,6 +965,75 @@ mod tests {
         std::fs::remove_dir_all(&dir).ok();
     }
 
+    /// Renaming a tag onto a name another tag already holds MERGES the two, and the
+    /// OLDER row is the survivor.
+    ///
+    /// Before this, the bare UPDATE met `idx_labels_name` — unique on `lower(name)` —
+    /// and the user got a raw "UNIQUE constraint failed" from SQLite. Merging is what
+    /// a person means by typing an existing tag's name onto this one.
+    #[test]
+    fn renaming_onto_an_existing_tag_merges_into_the_older_one() {
+        let dir = scratch_dir();
+        let app = ThoughtSync::new(dir.clone()).expect("a fresh data dir should open");
+
+        let older = app.create_label("grocery".to_string()).expect("older");
+        // `created_at` is RFC3339 to the MILLISECOND. Without a gap the two rows can
+        // share a timestamp, and then the tie-break is under test instead of the age
+        // rule this test is about.
+        std::thread::sleep(std::time::Duration::from_millis(5));
+        let newer = app.create_label("errands".to_string()).expect("newer");
+
+        let one = app.create_note(draft("milk")).expect("note one");
+        let two = app.create_note(draft("stamps")).expect("note two");
+        app.set_note_labels(one.id.clone(), vec![older.id.clone()])
+            .expect("tag one");
+        app.set_note_labels(two.id.clone(), vec![newer.id.clone()])
+            .expect("tag two");
+
+        // The YOUNGER one is renamed onto the older's name, in a different case —
+        // matching is case-insensitive, and the survivor takes the spelling asked for.
+        let survivor = app
+            .rename_label(newer.id.clone(), "Grocery".to_string())
+            .expect("a rename onto an existing name merges instead of failing");
+
+        assert_eq!(survivor.id, older.id, "the older row is the one that survives");
+        assert_eq!(survivor.name, "Grocery", "spelled the way the caller asked");
+
+        let all = app.list_labels().expect("list");
+        assert_eq!(all.len(), 1, "the two became one");
+        assert_eq!(all[0].id, older.id);
+        assert_eq!(all[0].count, Some(2), "carrying every note from both sides");
+
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
+    /// The mirror of the test above. Renaming the OLDER one onto the younger's name
+    /// still leaves the older row standing — it just changes its name.
+    ///
+    /// This is the whole reason age decides rather than "whoever already held the
+    /// name": otherwise the survivor depends on which way round someone typed it,
+    /// and two devices tidying the same pair would disagree about which id exists.
+    #[test]
+    fn the_rename_merge_survivor_does_not_depend_on_the_direction() {
+        let dir = scratch_dir();
+        let app = ThoughtSync::new(dir.clone()).expect("a fresh data dir should open");
+
+        let older = app.create_label("grocery".to_string()).expect("older");
+        std::thread::sleep(std::time::Duration::from_millis(5));
+        let newer = app.create_label("errands".to_string()).expect("newer");
+
+        let survivor = app
+            .rename_label(older.id.clone(), "errands".to_string())
+            .expect("rename");
+
+        assert_eq!(survivor.id, older.id, "age wins in this direction too");
+        assert_eq!(survivor.name, "errands");
+        assert_ne!(survivor.id, newer.id, "the younger row is the one that went");
+        assert_eq!(app.list_labels().expect("list").len(), 1);
+
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
     /// A crude RFC3339 sanity check that doesn't pull a date crate into this
     /// crate's dev-dependencies to assert one field is well-formed.
     fn chrono_free_parse(raw: &str) -> usize {
diff --git a/core/src/local/store.rs b/core/src/local/store.rs
index ccdffe3..dddc51d 100644
--- a/core/src/local/store.rs
+++ b/core/src/local/store.rs
@@ -820,7 +820,57 @@ pub fn create_label(conn: &Connection, name: &str) -> rusqlite::Result<Label> {
     load_label(conn, &id)
 }
 
+/// Rename a label. Renaming ONTO a name another label already holds MERGES the two.
+///
+/// It cannot simply be an UPDATE: `idx_labels_name` is unique on `lower(name)`, so
+/// the bare statement failed with a raw SQLite "UNIQUE constraint failed" that
+/// reached the user as database internals. Merging is the operator's call, and it
+/// is the reading that matches what a person means — typing an existing tag's name
+/// onto this one says "these are the same thing."
+///
+/// THE OLDER ROW SURVIVES, and takes the new spelling. Older rather than "the one
+/// that already held the name" because age is the property neither participant's
+/// role can change: rename A→B and rename B→A must land on the same survivor, or
+/// the result depends on which way round someone happened to type it. Ties (two
+/// labels minted in the same millisecond) go to the incumbent, so the outcome is
+/// still deterministic.
+///
+/// Matching is case-insensitive, agreeing with `find_or_create_label` — "Groceries"
+/// finds "groceries", and the survivor ends up spelled the way the caller asked.
 pub fn rename_label(conn: &Connection, id: &str, name: &str) -> rusqlite::Result<Label> {
+    let clash: Option<(String, String)> = conn
+        .query_row(
+            "SELECT id, created_at FROM labels WHERE lower(name) = lower(?1) AND id <> ?2",
+            params![name, id],
+            |r| Ok((r.get(0)?, r.get(1)?)),
+        )
+        .optional()?;
+
+    if let Some((other_id, other_created)) = clash {
+        let mine_created: String =
+            conn.query_row("SELECT created_at FROM labels WHERE id = ?1", [id], |r| {
+                r.get(0)
+            })?;
+        // `created_at` is RFC3339 to the millisecond with a `Z`, so it is fixed-width
+        // and lexicographic order IS chronological order — no parsing needed.
+        let (survivor, doomed) = if other_created <= mine_created {
+            (other_id, id.to_string())
+        } else {
+            (id.to_string(), other_id)
+        };
+        // Reuse the merge rather than re-implement it: it is the only place that
+        // knows to mark every affected NOTE dirty before the delete cascades the
+        // membership rows away, which is what makes the merge reach the server.
+        merge_labels(conn, &doomed, &survivor)?;
+        // The survivor may still carry the old spelling — it is the one that keeps
+        // existing, so it is the one that has to end up named what was asked for.
+        conn.execute(
+            "UPDATE labels SET name = ?1, updated_at = ?2, dirty = 1 WHERE id = ?3",
+            params![name, now(), survivor],
+        )?;
+        return load_label(conn, &survivor);
+    }
+
     conn.execute(
         "UPDATE labels SET name = ?1, updated_at = ?2, dirty = 1 WHERE id = ?3",
         params![name, now(), id],
diff --git a/frontend/src/stores/labels.ts b/frontend/src/stores/labels.ts
index f64a182..403347d 100644
--- a/frontend/src/stores/labels.ts
+++ b/frontend/src/stores/labels.ts
@@ -33,7 +33,35 @@ export const useLabelsStore = defineStore("labels", () => {
   }
 
   async function rename(id: string, name: string): Promise<void> {
+    // Renaming onto a name another tag already holds MERGES the two — server-side
+    // and in the local store, identically. Detected from the LIST rather than from
+    // the response: the survivor is whichever row is older, so it may well be the
+    // one we asked to rename, and an id that still matches proves nothing happened.
+    const absorbing = items.value.find(
+      (lb) => lb.id !== id && lb.name.toLowerCase() === name.toLowerCase(),
+    );
+    if (absorbing) {
+      // A merge cannot be undone by repeating it, and here it is reachable by a
+      // typo in a text field — so it asks, the way deleting one does. The counts
+      // are named because "40 notes" is the part that makes the consequence real.
+      const mine = items.value.find((lb) => lb.id === id);
+      const confirmed = window.confirm(
+        `A tag called "${absorbing.name}" already exists.\n\n` +
+          `Renaming will MERGE these two into one tag named "${name}", carrying ` +
+          `every note from both (${mine?.count ?? 0} + ${absorbing.count ?? 0}). ` +
+          "The notes are kept; one of the two tags stops existing, and that cannot " +
+          "be undone.",
+      );
+      if (!confirmed) return;
+    }
+
     const updated = await repo.labels.rename(id, name);
+    if (absorbing) {
+      // One row is gone and the survivor's count grew, and this response carries no
+      // count — reload rather than guess which of the two we are now holding.
+      await load();
+      return;
+    }
     const idx = items.value.findIndex((lb) => lb.id === id);
     // The single-label PATCH doesn't recompute the count — keep the one we have.
     if (idx >= 0) items.value[idx] = { ...updated, count: items.value[idx].count };
@@ -53,7 +81,7 @@ export const useLabelsStore = defineStore("labels", () => {
     // notes themselves survive; only the membership goes, which is the part people
     // most need reassuring about.
     const label = items.value.find((lb) => lb.id === id);
-    const subject = label ? `the label "${label.name}"` : "this label";
+    const subject = label ? `the tag "${label.name}"` : "this tag";
     const confirmed = window.confirm(
       `Delete ${subject}?\n\n` +
         "It will be removed from every note that uses it, on every device you sync " +
diff --git a/src/thoughtsync/labels.py b/src/thoughtsync/labels.py
index 483e413..645d06d 100644
--- a/src/thoughtsync/labels.py
+++ b/src/thoughtsync/labels.py
@@ -27,6 +27,36 @@ async def _label_note_count(db, label_id) -> int:
     )
 
 
+async def _merge_into(db, source: Label, target: Label) -> None:
+    """Move every note tagged with `source` onto `target`, then delete `source`.
+
+    Shared by the explicit `/merge` route and by a rename that lands on a name
+    another tag already holds — those are the same operation, and having one body
+    is what stops them drifting into two answers for one question.
+
+    Note-body `#tags` are NOT rewritten, so a note whose body still literally
+    contains the source `#tag` will re-mint that tag on its next edit. Retiring a
+    tag means editing it out of the text; a known, documented nuance.
+    """
+    # Notes already carrying the target: a note can't hold the same label twice
+    # (composite PK), so the source attachment there is just dropped as a dup.
+    target_notes = set(
+        (await db.scalars(select(NoteLabel.note_id).where(NoteLabel.label_id == target.id))).all()
+    )
+    source_rows = (await db.scalars(select(NoteLabel).where(NoteLabel.label_id == source.id))).all()
+    by_note = {r.note_id: r.via_tag for r in source_rows}
+    # Delete the source attachments first, then re-insert under the target — moving
+    # by delete+insert avoids mutating a composite primary-key column in place.
+    for row in source_rows:
+        await db.delete(row)
+    await db.flush()
+    for note_id, via_tag in by_note.items():
+        if note_id not in target_notes:
+            db.add(NoteLabel(note_id=note_id, label_id=target.id, via_tag=via_tag))
+    await db.delete(source)
+    await db.flush()
+
+
 async def _get_owned_label(db, label_id: str) -> Label | None:
     lid = parse_uuid(label_id)
     if lid is None:
@@ -60,8 +90,13 @@ async def create_label():
     if not name:
         return json_error("tag name is required", 400)
     async with session_scope() as db:
-        # Idempotent: creating an existing label just returns it.
-        existing = await db.scalar(select(Label).where(Label.owner_id == g.user_id, Label.name == name))
+        # Idempotent: creating an existing tag just returns it. Case-INSENSITIVE,
+        # like the clients' `find_or_create_label` — a case-sensitive match here was
+        # the path that MINTED the "Groceries" beside "groceries" pair that no synced
+        # client can hold, since their `labels` index is unique on `lower(name)`.
+        existing = await db.scalar(
+            select(Label).where(Label.owner_id == g.user_id, func.lower(Label.name) == name.lower())
+        )
         if existing is not None:
             return jsonify(_serialize_label(existing)), 200
         label = Label(owner_id=g.user_id, name=name, color=normalize_color(data.get("color")))
@@ -87,11 +122,31 @@ async def update_label(label_id: str):
         if label is None:
             return not_found()
         if has_name:
+            # Case-INSENSITIVE, matching the clients' `find_or_create_label` and the
+            # local store's `lower(name)` unique index. The Postgres constraint here
+            # is on the raw name, so the database would happily hold "Groceries"
+            # beside "groceries" — but no synced client can store both, so letting
+            # one be made is letting a pull fail later on a phone.
             clash = await db.scalar(
-                select(Label).where(Label.owner_id == g.user_id, Label.name == name, Label.id != label.id)
+                select(Label).where(
+                    Label.owner_id == g.user_id,
+                    func.lower(Label.name) == name.lower(),
+                    Label.id != label.id,
+                )
             )
             if clash is not None:
-                return json_error("a tag with that name already exists", 409)
+                # Renaming onto an existing tag MERGES the two rather than failing:
+                # typing an existing tag's name onto this one says they are the same
+                # thing. The OLDER row survives and takes the new spelling — age is
+                # the one property that does not depend on which of the two the
+                # caller happened to be renaming, so A→B and B→A agree. Ties go to
+                # the incumbent. Mirrors `store::rename_label` exactly.
+                if clash.created_at <= label.created_at:
+                    survivor, doomed = clash, label
+                else:
+                    survivor, doomed = label, clash
+                await _merge_into(db, doomed, survivor)
+                label = survivor
             label.name = name
         if has_color:
             label.color = normalize_color(data.get("color"))
@@ -129,23 +184,7 @@ async def merge_label(label_id: str):
             return not_found()
         if source.id == target.id:
             return json_error("cannot merge a tag into itself", 400)
-        # Notes already carrying the target: a note can't hold the same label twice
-        # (composite PK), so the source attachment there is just dropped as a dup.
-        target_notes = set(
-            (await db.scalars(select(NoteLabel.note_id).where(NoteLabel.label_id == target.id))).all()
-        )
-        source_rows = (await db.scalars(select(NoteLabel).where(NoteLabel.label_id == source.id))).all()
-        by_note = {r.note_id: r.via_tag for r in source_rows}
-        # Delete the source attachments first, then re-insert under the target — moving
-        # by delete+insert avoids mutating a composite primary-key column in place.
-        for row in source_rows:
-            await db.delete(row)
-        await db.flush()
-        for note_id, via_tag in by_note.items():
-            if note_id not in target_notes:
-                db.add(NoteLabel(note_id=note_id, label_id=target.id, via_tag=via_tag))
-        await db.delete(source)
-        await db.flush()
+        await _merge_into(db, source, target)
         count = await _label_note_count(db, target.id)
         await db.commit()
         return jsonify(_serialize_label(target, count))
diff --git a/tests/test_integration.py b/tests/test_integration.py
index 527033e..d926738 100644
--- a/tests/test_integration.py
+++ b/tests/test_integration.py
@@ -543,3 +543,90 @@ async def test_a_revision_inside_the_window_blocks_another(db, owner):
     await db.commit()
 
     assert await should_snapshot(db, note.id, note.body, "draft revised") is False
+
+
+async def test_renaming_a_tag_onto_an_existing_one_merges_into_the_older(app_client, db):
+    """Renaming a tag onto a name another tag holds merges them, and the OLDER row
+    is the survivor — whichever side the caller happened to be renaming.
+
+    Runs against a real database because the whole question is about `created_at`
+    ordering and the note_labels rows moving, neither of which a unit test sees.
+
+    Age decides, rather than "the one that already held the name", so that renaming
+    A→B and renaming B→A land on the same row. If the incumbent won, the survivor
+    would depend on which way round someone typed it, and two clients racing the
+    same tidy-up would disagree about which id still exists.
+    """
+    reg = await app_client.post(
+        "/api/auth/register",
+        json={"email": "tags@example.test", "password": "a-long-enough-password"},
+    )
+    assert reg.status_code == 201
+
+    # Two separate requests, so two transactions and two distinct `func.now()`s.
+    older = await (await app_client.post("/api/labels", json={"name": "grocery"})).get_json()
+    newer = await (await app_client.post("/api/labels", json={"name": "errands"})).get_json()
+
+    one = await (await app_client.post("/api/notes", json={"body": "milk"})).get_json()
+    two = await (await app_client.post("/api/notes", json={"body": "stamps"})).get_json()
+    await app_client.put(f"/api/notes/{one['id']}/labels", json={"label_ids": [older["id"]]})
+    await app_client.put(f"/api/notes/{two['id']}/labels", json={"label_ids": [newer["id"]]})
+
+    # Rename the YOUNGER onto the older's name, with different casing — matching is
+    # case-insensitive, and the survivor must end up spelled the way we asked.
+    resp = await app_client.patch(f"/api/labels/{newer['id']}", json={"name": "Grocery"})
+    assert resp.status_code == 200
+    survivor = await resp.get_json()
+
+    assert survivor["id"] == older["id"], "the older row is the one that keeps existing"
+    assert survivor["name"] == "Grocery", "the survivor takes the spelling that was asked for"
+
+    listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
+    assert len(listing) == 1, "the two became one"
+    assert listing[0]["id"] == older["id"]
+    assert listing[0]["count"] == 2, "it carries every note from both sides"
+
+
+async def test_the_rename_merge_survivor_does_not_depend_on_the_direction(app_client, db):
+    """The mirror of the test above: rename the OLDER onto the younger's name. The
+    older still survives — it just changes its name — so the two directions agree."""
+    reg = await app_client.post(
+        "/api/auth/register",
+        json={"email": "tags2@example.test", "password": "a-long-enough-password"},
+    )
+    assert reg.status_code == 201
+
+    older = await (await app_client.post("/api/labels", json={"name": "grocery"})).get_json()
+    newer = await (await app_client.post("/api/labels", json={"name": "errands"})).get_json()
+
+    resp = await app_client.patch(f"/api/labels/{older['id']}", json={"name": "errands"})
+    assert resp.status_code == 200
+    survivor = await resp.get_json()
+
+    assert survivor["id"] == older["id"], "age wins in this direction too"
+    assert survivor["name"] == "errands"
+    assert newer["id"] != older["id"]
+
+    listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
+    assert [lb["id"] for lb in listing] == [older["id"]]
+
+
+async def test_creating_a_tag_that_differs_only_in_case_returns_the_existing_one(app_client, db):
+    """A case-sensitive match here used to mint "Groceries" beside "groceries". No
+    synced client can hold both — their `labels` index is unique on `lower(name)` —
+    so the pair was a pull that would fail later, on a phone, with no UI in the path.
+    """
+    reg = await app_client.post(
+        "/api/auth/register",
+        json={"email": "tags3@example.test", "password": "a-long-enough-password"},
+    )
+    assert reg.status_code == 201
+
+    first = await app_client.post("/api/labels", json={"name": "groceries"})
+    assert first.status_code == 201
+    second = await app_client.post("/api/labels", json={"name": "Groceries"})
+    assert second.status_code == 200, "an existing tag is returned, not a second one made"
+
+    assert (await second.get_json())["id"] == (await first.get_json())["id"]
+    listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
+    assert len(listing) == 1