diff --git a/src/thoughtsync/labeling.py b/src/thoughtsync/labeling.py index 9b07f3f..094dc05 100644 --- a/src/thoughtsync/labeling.py +++ b/src/thoughtsync/labeling.py @@ -2,7 +2,7 @@ labels, leave the tag-sourced ones alone" logic was duplicated line-for-line between the labels-picker API (notes.set_note_labels) and sync push (sync._apply_note_manual_labels). Single home so both stay in lockstep. via_tag=True rows track the body #tags and are -governed by _reconcile_tags — this function never touches them.""" +governed by _lift_and_reconcile_tags — this function never touches them.""" from __future__ import annotations from sqlalchemy import select diff --git a/src/thoughtsync/notes/__init__.py b/src/thoughtsync/notes/__init__.py index 1a744ca..9162127 100644 --- a/src/thoughtsync/notes/__init__.py +++ b/src/thoughtsync/notes/__init__.py @@ -72,7 +72,7 @@ from .import_export import ( _usec_to_dt, ) from .tags import ( - _reconcile_tags, + _lift_and_reconcile_tags, parse_tags, ) from .recurrence import REMINDER_RECURRENCES, next_occurrence, normalize_recurrence @@ -87,7 +87,7 @@ __all__ = [ "normalize_color", "normalize_recurrence", "next_occurrence", - "_reconcile_tags", + "_lift_and_reconcile_tags", "_serialize_notes", "_safe_filename", "_attachment_ext", @@ -418,7 +418,7 @@ async def create_note(): db.add(note) await db.flush() # assign note.id before writing links # The FOLDED body: an item can carry a #tag too. - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) await db.commit() await db.refresh(note) # After the commit, never before it: the note is saved and the response is @@ -473,7 +473,7 @@ async def update_note(note_id: str): note.recurrence = normalize_recurrence(data["recurrence"]) if "body" in data: note.display_title = derive_display_title(note.body) - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) # Version history: snapshot the PRE-edit body, once per editing session # rather than once per write — see revisions.should_snapshot. Writing often # is what lets a client autosave instead of hoarding text until it closes. @@ -532,7 +532,7 @@ async def restore_revision(note_id: str, rev_id: str): db.add(NoteRevision(note_id=note.id, body=note.body)) note.body = rev.body note.display_title = derive_display_title(note.body) - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) await db.commit() await db.refresh(note) return jsonify(await _serialize_note(db, note)) @@ -586,7 +586,7 @@ async def _rewrite_body(db, note: Note, body: str): db.add(NoteRevision(note_id=note.id, body=old_body)) note.body = body note.display_title = derive_display_title(body) - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) await db.commit() await db.refresh(note) if note.body != old_body: diff --git a/src/thoughtsync/notes/import_export.py b/src/thoughtsync/notes/import_export.py index 93cbe73..e1da5b8 100644 --- a/src/thoughtsync/notes/import_export.py +++ b/src/thoughtsync/notes/import_export.py @@ -28,7 +28,7 @@ from .helpers import ( is_empty_note, ) from .checklist import append_item -from .tags import _find_or_create_label, _reconcile_tags +from .tags import _find_or_create_label, _lift_and_reconcile_tags from .recurrence import normalize_recurrence @@ -293,7 +293,7 @@ async def _create_imported_note( return False # Items fold into the body, which is where a checklist lives now (M304). Done - # before the Note is built so display_title and _reconcile_tags both see the + # before the Note is built so display_title and _lift_and_reconcile_tags both see the # finished text — an imported item can carry a #tag like any other line. for it in items: text = (it.get("text") or "").strip() @@ -325,7 +325,7 @@ async def _create_imported_note( await db.flush() # assign note.id before labels/attachments/links # Explicit (picker-style) labels are manual — via_tag=False. Inline #tags in the - # body are handled by _reconcile_tags below, same as a normal create. + # body are handled by _lift_and_reconcile_tags below, same as a normal create. for name in spec.get("labels") or []: name = (name or "").strip() if not name: @@ -341,5 +341,5 @@ async def _create_imported_note( if isinstance(att, dict): _import_attachment(db, note, zf, att, budget) - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) return True diff --git a/src/thoughtsync/notes/tags.py b/src/thoughtsync/notes/tags.py index cfd4561..56bc94c 100644 --- a/src/thoughtsync/notes/tags.py +++ b/src/thoughtsync/notes/tags.py @@ -1,6 +1,11 @@ -"""#tags — parsing note bodies and keeping the derived tag-sourced note_labels rows -in sync with the text. Manual (picker) labels are NOT touched here (see the labeling -module). +"""#tags — parsing note bodies, LIFTING the standalone ones out of the text, and +keeping the still-in-text ones in sync with the note_labels rows. Manual (picker) +labels are NOT touched here (see the labeling module). + +A tag used to be shown twice: once as the `#todo` you typed and once as a chip. The +chip moved to the top of the card (M311) and the text now goes, but only when the tag +was standing on its own — see `split_body_tags` for the rule and why it is the +conservative one. Was `links.py`, and also owned `[[wiki-links]]` until they were removed (note 2897): this app is an intermediary surface for capture and recall, and a linking system is @@ -15,6 +20,7 @@ from sqlalchemy import func, select from ..models.label import Label, NoteLabel from ..models.note import Note +from .helpers import derive_display_title # A #tag: `#` at the start of the body or after whitespace, then a word char and # word chars/hyphens. A URL fragment (foo#bar) or mid-word `#` is not preceded by @@ -22,24 +28,93 @@ from ..models.note import Note _TAG_RE = re.compile(r"(?:^|(?<=\s))#(\w[\w-]*)") -def parse_tags(body: str | None) -> list[str]: - """Distinct #hashtags from a note body, in order, deduped case-insensitively. - A tag must contain a letter, so #2024 or #_ are ignored (avoids numeric noise).""" - if not body: - return [] +# A fence opens or closes a code block. A `#tag` inside one is CODE — the shell +# comment in a snippet someone pasted — and lifting it would delete a line of their +# example. It still becomes a label, because it always has and that is a separate +# question from whether the text may be touched. +_FENCE_RE = re.compile(r"^\s*(?:```|~~~)") + + +def _is_tag(name: str) -> bool: + """A tag must contain a letter, so #2024 and #_ are ignored (avoids numeric noise).""" + return any(c.isalpha() for c in name) + + +def _dedupe(names: list[str]) -> list[str]: + """First-seen order, deduped case-insensitively — tags are case-insensitive.""" out: list[str] = [] seen: set[str] = set() - for match in _TAG_RE.finditer(body): - tag = match.group(1) - if not any(c.isalpha() for c in tag): - continue - norm = tag.lower() + for name in names: + norm = name.lower() if norm not in seen: seen.add(norm) - out.append(tag) + out.append(name) return out +def parse_tags(body: str | None) -> list[str]: + """Distinct #hashtags from a note body, in order, deduped case-insensitively.""" + if not body: + return [] + return _dedupe([m.group(1) for m in _TAG_RE.finditer(body) if _is_tag(m.group(1))]) + + +def split_body_tags(body: str | None) -> tuple[list[str], list[str], str]: + """Split a body's tags by whether the text around them can be taken away. + + Returns `(standalone, inline, lifted_body)`. + + THE RULE: a line containing nothing but tags and whitespace is removed. Anything + else is left exactly as written. + + That is deliberately the conservative reading of "standalone". The looser one — + also stripping a trailing tag off a prose line — was rejected because a trailing + tag is ambiguous and the text does not say which it is: `buy milk #grocery` is + filing, `remember to call #mom` is the sentence's object, and lifting the second + leaves "remember to call". Mangling a sentence to save a duplicate chip is a bad + trade. A tag sharing a line with words keeps its words. + + `standalone` tags become ORDINARY labels — the text no longer backs them, so + nothing can derive them any more, and the way to remove one becomes the chip's × + rather than deleting the text. `inline` tags stay derived exactly as before. That + is the whole meaning of `via_tag` after this change: backed by text still present. + """ + if not body: + return [], [], body or "" + standalone: list[str] = [] + inline: list[str] = [] + kept: list[str] = [] + in_fence = False + for line in body.split("\n"): + if _FENCE_RE.match(line): + in_fence = not in_fence + kept.append(line) + continue + matches = [m for m in _TAG_RE.finditer(line) if _is_tag(m.group(1))] + names = [m.group(1) for m in matches] + # Cutting the tags out and finding nothing left is what "standalone" means. + remainder = line + for m in reversed(matches): + remainder = remainder[: m.start()] + remainder[m.end() :] + if in_fence or not matches or remainder.strip(): + inline.extend(names) + kept.append(line) + else: + standalone.extend(names) + lifted = re.sub(r"\n{3,}", "\n\n", "\n".join(kept)).strip("\n") + if body.strip() and not lifted.strip(): + # The note was NOTHING but tags. Lifting would leave a blank card, which is a + # worse outcome than a duplicated chip — so leave it alone and let its tags + # stay derived. + return [], _dedupe(standalone + inline), body + # A tag that ALSO appears in prose stays derived: the prose copy still backs it, + # so deleting that copy should still detach the label. + inline_names = _dedupe(inline) + inline_lower = {n.lower() for n in inline_names} + standalone_names = [n for n in _dedupe(standalone) if n.lower() not in inline_lower] + return standalone_names, inline_names, lifted + + async def _find_or_create_label(db, owner_id, name: str): """Owner's label id for `name` (case-insensitive match), creating it if absent.""" existing = await db.scalar( @@ -53,23 +128,65 @@ async def _find_or_create_label(db, owner_id, name: str): return label.id -async def _reconcile_tags(db, note: Note) -> None: - """Sync tag-sourced labels (via_tag=True) with the #hashtags in the note body: - attach labels for current tags, detach tag-labels whose #tag was removed. Manual - picker labels (via_tag=False) are never touched.""" - tag_label_ids: set = set() - for name in parse_tags(note.body): - tag_label_ids.add(await _find_or_create_label(db, note.owner_id, name)) +async def _lift_and_lift_and_reconcile_tags(db, note: Note) -> None: + """Attach the note's tag labels, LIFT its standalone tags out of the body, and + re-derive display_title if the body moved. + + NAMED FOR THE MUTATION. It used to be `_lift_and_reconcile_tags` and only touched rows; + it now rewrites `note.body`, and a caller that does not expect that will compute + a display_title from text this function is about to delete. + + Which is why the lift and the re-derivation both live HERE rather than at the + seven call sites that would each have to remember. Spreading a derived-value + update across every place a body can be written is precisely the failure #2965 + named about label minting: "easy to miss, and it is the common one". + + The two kinds of tag are handled differently, and that difference IS what + `via_tag` means from here on — backed by text still in the body: + + standalone lifted out, attached as an ORDINARY label (via_tag=False). Nothing + derives it any more because nothing is left to derive it from, and + the way to remove it becomes the chip's × — which both editors + already offer for exactly this class of label. + inline left in place, attached via_tag=True, and still detached when its + text goes. Unchanged from before. + """ + standalone, inline, lifted = split_body_tags(note.body) + standalone_ids: set = set() + for name in standalone: + standalone_ids.add(await _find_or_create_label(db, note.owner_id, name)) + inline_ids: set = set() + for name in inline: + inline_ids.add(await _find_or_create_label(db, note.owner_id, name)) + rows = (await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all() - attached_ids = {r.label_id for r in rows} - # Detach tag-labels no longer backed by a #tag in the body. + attached: set = set() for r in rows: - if r.via_tag and r.label_id not in tag_label_ids: - await db.delete(r) - attached_ids.discard(r.label_id) - # Attach new tags — skip labels already attached (in any form) to respect the PK - # and leave a manually-added label of the same name as-is. - for lid in tag_label_ids: - if lid not in attached_ids: + if not r.via_tag: + attached.add(r.label_id) # manual already: a #tag of the same name changes nothing + elif r.label_id in standalone_ids: + # It GRADUATED. The text backing it is about to be deleted, so the row has + # to become the record instead. This must run BEFORE the detach below, or + # the same row would be dropped for no longer being in the body — which is + # the bug that makes a naive lift delete every tag it touches. + r.via_tag = False + attached.add(r.label_id) + elif r.label_id in inline_ids: + attached.add(r.label_id) + else: + await db.delete(r) # its #tag was deleted from the text + + # Skip anything already attached in ANY form: it respects the PK, and it leaves a + # manually-added label of the same name as the manual row it already is. + for lid in standalone_ids: + if lid not in attached: + db.add(NoteLabel(note_id=note.id, label_id=lid, via_tag=False)) + attached.add(lid) + for lid in inline_ids: + if lid not in attached: db.add(NoteLabel(note_id=note.id, label_id=lid, via_tag=True)) - attached_ids.add(lid) + attached.add(lid) + + if lifted != note.body: + note.body = lifted + note.display_title = derive_display_title(note.body) diff --git a/src/thoughtsync/sync.py b/src/thoughtsync/sync.py index f5e141a..a324c7a 100644 --- a/src/thoughtsync/sync.py +++ b/src/thoughtsync/sync.py @@ -27,7 +27,7 @@ from .models.note import Note from .models.note_revision import NoteRevision from .revisions import should_snapshot from .notes import ( - _reconcile_tags, + _lift_and_reconcile_tags, _serialize_notes, derive_display_title, normalize_color, @@ -222,7 +222,7 @@ def _assign_note_fields(note: Note, ch: dict) -> None: async def _apply_note_manual_labels(db, note: Note, ch: dict) -> None: """Set the note's MANUAL (picker) label memberships from client label_ids, leaving - tag-sourced (via_tag) rows to _reconcile_tags. Only labels the caller owns count.""" + tag-sourced (via_tag) rows to _lift_and_reconcile_tags. Only labels the caller owns count.""" raw = ch.get("label_ids") if not isinstance(raw, list): return @@ -289,7 +289,7 @@ async def _apply_note(db, ch: dict) -> dict: if not creating and await should_snapshot(db, note.id, old_body, note.body): db.add(NoteRevision(note_id=note.id, body=old_body)) await db.flush() # assign note.id before items/labels/links - await _reconcile_tags(db, note) + await _lift_and_reconcile_tags(db, note) await _apply_note_manual_labels(db, note, ch) await db.flush() await db.refresh(note, ["sync_revision"]) diff --git a/tests/test_integration.py b/tests/test_integration.py index 3b2ff66..527033e 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -25,8 +25,10 @@ from sqlalchemy import select, text from thoughtsync import ratelimit from thoughtsync.app import create_app from thoughtsync.db import dispose_engine, session_scope +from thoughtsync.models.label import NoteLabel from thoughtsync.models.note import Note from thoughtsync.models.user import User +from thoughtsync.notes.tags import _lift_and_reconcile_tags from thoughtsync.settings import get_setting, live, refresh_live, reset_live, set_settings from thoughtsync.notes.checklist import parse_items, set_item_checked from thoughtsync.notes.helpers import derive_display_title @@ -199,6 +201,74 @@ async def test_ticking_an_item_is_a_body_edit(db, owner): assert stored.splitlines()[0] == "packing" +async def test_a_standalone_tag_leaves_the_body_and_becomes_an_ordinary_label(db, owner): + """M311. The tag was being shown twice — as text and as a chip — so the text goes. + + `via_tag=False` is the load-bearing half. It is what makes the chip's × appear in + both editors (they gate it on exactly this), which matters because deleting the + text is no longer a way to remove the tag: there is no text. + """ + note = Note(owner_id=owner.id, body="#todo\nreorganize the homepage", display_title="#todo") + db.add(note) + await db.flush() + await _lift_and_reconcile_tags(db, note) + await db.commit() + + assert note.body == "reorganize the homepage" + # Re-derived by the lift itself. Every caller sets display_title BEFORE calling, + # so if the function did not do this the note would be named after a line it had + # just deleted. + assert note.display_title == "reorganize the homepage" + + rows = (await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all() + assert len(rows) == 1 + assert rows[0].via_tag is False + + +async def test_a_tag_moved_onto_its_own_line_graduates_instead_of_vanishing(db, owner): + """The bug a naive lift has, pinned. + + A tag that is still in prose stays derived. Move it to its own line and it must + become an ordinary label — NOT be detached for no longer appearing in the body, + which is what happens if the row is dropped before it is graduated. + """ + note = Note(owner_id=owner.id, body="call #mom tomorrow", display_title="call #mom tomorrow") + db.add(note) + await db.flush() + await _lift_and_reconcile_tags(db, note) + await db.commit() + + rows = (await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all() + assert len(rows) == 1 + assert rows[0].via_tag is True + assert note.body == "call #mom tomorrow", "a tag inside a sentence is left alone" + + note.body = "#mom\ncall tomorrow" + await _lift_and_reconcile_tags(db, note) + await db.commit() + + rows = (await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all() + assert len(rows) == 1, "the label survived the move" + assert rows[0].via_tag is False + assert note.body == "call tomorrow" + + +async def test_deleting_an_inline_tag_still_detaches_it(db, owner): + """The old behaviour, unchanged where the text is unchanged. A tag still living in + prose is still owned by that prose.""" + note = Note(owner_id=owner.id, body="call #mom tomorrow", display_title="call #mom tomorrow") + db.add(note) + await db.flush() + await _lift_and_reconcile_tags(db, note) + await db.commit() + assert len((await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all()) == 1 + + note.body = "call tomorrow" + await _lift_and_reconcile_tags(db, note) + await db.commit() + assert (await db.scalars(select(NoteLabel).where(NoteLabel.note_id == note.id))).all() == [] + + async def test_a_note_with_only_a_list_still_has_a_name(db, owner): """The hole that made removing the title unsafe, still closed — by a different mechanism. There is no item table to fall back to any more; the name comes from diff --git a/tests/test_notes.py b/tests/test_notes.py index ef83f31..c1d3940 100644 --- a/tests/test_notes.py +++ b/tests/test_notes.py @@ -31,6 +31,7 @@ from thoughtsync.notes import ( parse_list_items, parse_tags, ) +from thoughtsync.notes.tags import split_body_tags @pytest.fixture @@ -168,6 +169,57 @@ def test_derive_display_title_caps_length(): assert derive_display_title(f"- [ ] {long}") == "x" * 200 +def test_split_body_tags_lifts_only_a_line_that_is_nothing_else(): + """The rule, in the cases it exists to get right. + + A tag on a line of its own is filing and the line can go. A tag sharing a line + with words is part of what was written, and taking it out would leave "remember to + call" — so the line is left exactly as typed. The trailing-tag case (`buy milk + #grocery`) is deliberately on the conservative side of the line: it reads like + filing, but nothing in the text distinguishes it from `remember to call #mom`, and + guessing wrong mangles a sentence to save a duplicate chip. + """ + assert split_body_tags("#todo\nreorganize the homepage") == (["todo"], [], "reorganize the homepage") + assert split_body_tags("needs a tauri app\n#todo") == (["todo"], [], "needs a tauri app") + assert split_body_tags("#todo #work\nreal text") == (["todo", "work"], [], "real text") + + unchanged = "remember to call #mom tomorrow" + assert split_body_tags(unchanged) == ([], ["mom"], unchanged) + trailing = "buy milk #grocery" + assert split_body_tags(trailing) == ([], ["grocery"], trailing) + + +def test_split_body_tags_leaves_the_note_readable(): + # Removing a line must not leave a hole where it was. + assert split_body_tags("foo\n\n#todo\n\nbar") == (["todo"], [], "foo\n\nbar") + + # A note that is NOTHING but tags would be blanked. A duplicated chip beats an + # empty card, so it keeps its text and its tags stay derived. + assert split_body_tags("#todo") == ([], ["todo"], "#todo") + assert split_body_tags("#todo #work") == ([], ["todo", "work"], "#todo #work") + + # A tag in a code fence is CODE — a shell comment in somebody's snippet. It still + # becomes a label, because it always has, but the line is never touched. + fenced = "code:\n```\n#!/bin/sh\n#deploy\n```\ndone" + assert split_body_tags(fenced) == ([], ["deploy"], fenced) + + # Not a tag at all (no letter), so not a tag-only line either. + assert split_body_tags("#2024\nreal") == ([], [], "#2024\nreal") + + assert split_body_tags("") == ([], [], "") + assert split_body_tags(None) == ([], [], "") + + +def test_split_body_tags_keeps_a_tag_derived_when_prose_still_carries_it(): + """Appearing on its own line does NOT lift a tag that is also written in a + sentence — the sentence still backs it, so deleting that sentence should still + detach the label. Standalone and inline are not both true of one tag.""" + standalone, inline, body = split_body_tags("#todo\nremember the #todo list") + assert standalone == [] + assert inline == ["todo"] + assert body == "remember the #todo list" + + def test_parse_tags(): assert parse_tags("buy milk #groceries and #to-do now") == ["groceries", "to-do"] # case-insensitive dedup, first spelling wins