From 3a23157505d2fd84c8d6bda4a855a49b2ff22f8c Mon Sep 17 00:00:00 2001 From: dovvnloading <157447210+dovvnloading@users.noreply.github.com> Date: Tue, 1 Sep 2026 12:39:27 -0400 Subject: [PATCH] Stop notes inventing a colour, and mark system prompts by shape SceneNode.color's own contract is "None means use the kind's own default colour, a rendering fallback that is entirely the frontend's job", and NoteNodeView already honours it (backgroundColor: data.color ?? undefined). The save path overrode that on every write with a hard-coded "#4a7c59", so a note nobody ever coloured came back from a save/reload permanently green - in the middle of an otherwise monochrome canvas, and in a colour that is not one of the picker's eight swatches (its Green is #3f8f5c), so it could not be deliberately chosen or re-chosen either. The column is TEXT NOT NULL, which is why a value was being invented at all; the empty string is the storable spelling of "no colour chosen" and normalises back to None on read. Rows already carrying the old default are normalised the same way on load rather than rewritten, so existing databases render neutral with no destructive migration. A note whose colour was genuinely chosen still round-trips as that hex. Separately, a system-prompt note took the colour picker's own "Purple" swatch as its border colour. That put a SEMANTIC marker (this note governs the branch) and a USER CHOICE (I coloured this note purple) in the same visual channel, indistinguishable the moment anyone picks Purple for a note body - and spent a hue in a palette whose whole design is monochrome. The meaning was already carried by shape: .note-node.system-prompt's dashed, heavier border, plus the header badge. That border now takes a theme token, and the inline colour override is gone. Co-Authored-By: Claude --- backend/chat_library.py | 41 ++++++- backend/session_load.py | 18 +++- backend/tests/test_session_format_adr009.py | 112 ++++++++++++++++++++ web_ui/src/app/canvas/GroupColorPicker.tsx | 5 - web_ui/src/app/canvas/NoteNodeView.test.tsx | 6 ++ web_ui/src/app/canvas/NoteNodeView.tsx | 14 ++- web_ui/src/app/styles.css | 7 ++ 7 files changed, 191 insertions(+), 12 deletions(-) diff --git a/backend/chat_library.py b/backend/chat_library.py index aabc147c..99bc9f51 100644 --- a/backend/chat_library.py +++ b/backend/chat_library.py @@ -128,6 +128,29 @@ class ConcurrentSaveConflict(RuntimeError): _TIMESTAMP_DISPLAY_FORMATS = ("%Y-%m-%d %H:%M:%S", "%Y-%m-%d %H:%M:%S.%f") +# The colour every note was silently stamped with on save before this was +# fixed. It is NOT one of GroupColorPicker's own eight swatches, so a note +# carrying it demonstrably got it from that old default rather than from a +# user's choice - which is what makes normalising it back to None safe. +LEGACY_NOTE_DEFAULT_COLOR = "#4a7c59" + + +def _legacy_default_to_none(color): + """Treats the old forced default as "no colour chosen". + + Applied on the way IN (save) and on the way OUT (load, see + backend/session_load.py's _restore_notes), so existing rows render + neutral without this needing a destructive migration over the database. + A user who genuinely wants a green note still has the picker's own + Green, and picking it stores that hex, which is left alone.""" + if color is None: + return None + text = str(color).strip() + if not text or text.lower() == LEGACY_NOTE_DEFAULT_COLOR: + return None + return text + + def _parse_stored_timestamp(value: Any) -> datetime | None: raw = str(value) for fmt in _TIMESTAMP_DISPLAY_FORMATS: @@ -1789,7 +1812,23 @@ def save_chat_atomically_row( float(position.get("y", 0.0)), float(size.get("width", 0.0)), float(size.get("height", 0.0)), - str(note.get("color") or "#4a7c59"), + # None, not a hex. SceneNode.color's own contract is + # "None means use the kind's own default colour, a + # rendering fallback that is entirely the frontend's + # job" - and NoteNodeView already honours it + # (backgroundColor: data.color ?? undefined). Forcing + # "#4a7c59" here overrode that on every save, so a + # note the user never coloured came back from a + # save/reload permanently green - in a colour that is + # not even in the picker's palette (its Green is + # #3f8f5c), so it could never be chosen again either. + # "" (the column is NOT NULL - see _ensure_schema's + # `color TEXT NOT NULL`, which is why a hex was being + # invented here in the first place). The empty string + # is the storable spelling of "no colour chosen", and + # _legacy_default_to_none maps it straight back to + # None on the way out. + _legacy_default_to_none(note.get("color")) or "", note.get("header_color"), 1 if note.get("is_system_prompt") else 0, 1 if note.get("is_summary_note") else 0, diff --git a/backend/session_load.py b/backend/session_load.py index 1bc9661e..2eaa3b87 100644 --- a/backend/session_load.py +++ b/backend/session_load.py @@ -1108,6 +1108,14 @@ def _restore_children( continue +def _legacy_default_to_none(color): + """Deferred import: backend/chat_library.py owns the sentinel and the + rule, and importing it at module scope here would be circular.""" + from backend.chat_library import _legacy_default_to_none as impl + + return impl(color) + + def _restore_notes(document: SceneDocument, notes_data: list) -> dict[int, str]: notes_map: dict[int, str] = {} if not isinstance(notes_data, list): @@ -1123,7 +1131,15 @@ def _restore_notes(document: SceneDocument, notes_data: list) -> dict[int, str]: is_summary_note=bool(note_payload.get("is_summary_note", False)), ) document.set_note_content(note.id, str(note_payload.get("content", ""))) - document.set_group_color(note.id, note_payload.get("color"), note_payload.get("header_color")) + # Rows saved before the forced-default fix carry + # LEGACY_NOTE_DEFAULT_COLOR whether or not the user ever chose a + # colour; normalising on read means they render neutral without + # a destructive migration over anyone's saved database. + document.set_group_color( + note.id, + _legacy_default_to_none(note_payload.get("color")), + note_payload.get("header_color"), + ) # ADR-002 Workstream 1 ("Branch status and lifecycle") - # confirmed, pre-existing gap fixed inline: is_branch_comparison # (Compare Branches) already synced live to the frontend via diff --git a/backend/tests/test_session_format_adr009.py b/backend/tests/test_session_format_adr009.py index 0de6ce53..75ca8ce1 100644 --- a/backend/tests/test_session_format_adr009.py +++ b/backend/tests/test_session_format_adr009.py @@ -288,6 +288,118 @@ def test_store_rejects_a_ref_that_is_not_a_content_digest(tmp_path): # -- note edges survive a real DB round-trip (not just the in-memory one) -- +def test_an_uncoloured_note_stays_uncoloured_across_a_db_round_trip(tmp_path): + """SceneNode.color's contract is "None means use the kind's own default + colour", resolved by the frontend. The save path used to override that + with a hard-coded "#4a7c59", so a note nobody ever coloured came back + from a save/reload permanently green - and in a colour absent from the + picker's own palette, so it could not be chosen or re-chosen either.""" + from backend.chat_library import ( + load_chat_row, + load_notes_rows, + load_pins_rows, + save_chat_atomically_row, + ) + + db_path = tmp_path / "chats.db" + doc = SceneDocument() + chat = doc.add_chat_node(0, 0, "hello", is_user=True) + note = doc.add_note(0, -120) + doc.set_note_content(note.id, "a plain note") + doc.connect(note.id, chat.id) + assert doc.nodes[note.id].color is None, "precondition: nothing chose a colour" + + chat_data = build_chat_data(doc) + notes_data = chat_data.pop("notes_data") + pins_data = chat_data.pop("pins_data") + chat_id, _ = save_chat_atomically_row(db_path, None, "t", chat_data, notes_data, pins_data) + + # Reloaded exactly as chat_library.loadChat does - through the notes + # table, not from the in-memory notes_data above. + restored = SceneDocument() + restore_chat_into_document( + restored, + load_chat_row(db_path, chat_id), + load_notes_rows(db_path, chat_id), + load_pins_rows(db_path, chat_id), + ) + + restored_note = next(n for n in restored.nodes.values() if n.kind == "note") + assert restored_note.color is None + + +def test_a_chosen_note_colour_survives_a_db_round_trip(tmp_path): + """The other half: normalising the old default must not flatten a real + choice. The picker's own Green is #3f8f5c, a different value.""" + from backend.chat_library import ( + load_chat_row, + load_notes_rows, + load_pins_rows, + save_chat_atomically_row, + ) + + db_path = tmp_path / "chats.db" + doc = SceneDocument() + doc.add_chat_node(0, 0, "hello", is_user=True) + note = doc.add_note(0, -120) + doc.set_group_color(note.id, "#3f8f5c", None) + + chat_data = build_chat_data(doc) + notes_data = chat_data.pop("notes_data") + pins_data = chat_data.pop("pins_data") + chat_id, _ = save_chat_atomically_row(db_path, None, "t", chat_data, notes_data, pins_data) + + # Reloaded exactly as chat_library.loadChat does - through the notes + # table, not from the in-memory notes_data above. + restored = SceneDocument() + restore_chat_into_document( + restored, + load_chat_row(db_path, chat_id), + load_notes_rows(db_path, chat_id), + load_pins_rows(db_path, chat_id), + ) + + restored_note = next(n for n in restored.nodes.values() if n.kind == "note") + assert restored_note.color == "#3f8f5c" + + +def test_a_legacy_note_row_carrying_the_old_forced_default_loads_uncoloured(tmp_path): + """Rows written before the fix carry the forced default whether or not + anyone chose it. Normalising on READ means they render neutral without a + destructive migration over an existing database.""" + from backend.chat_library import ( + load_chat_row, + load_notes_rows, + load_pins_rows, + save_chat_atomically_row, + ) + + db_path = tmp_path / "chats.db" + doc = SceneDocument() + doc.add_chat_node(0, 0, "hello", is_user=True) + doc.add_note(0, -120) + + chat_data = build_chat_data(doc) + notes_data = chat_data.pop("notes_data") + pins_data = chat_data.pop("pins_data") + # Exactly what the pre-fix save path wrote. + notes_data[0]["color"] = "#4a7c59" + chat_id, _ = save_chat_atomically_row(db_path, None, "t", chat_data, notes_data, pins_data) + + # Reloaded exactly as chat_library.loadChat does - through the notes + # table, not from the in-memory notes_data above. + restored = SceneDocument() + restore_chat_into_document( + restored, + load_chat_row(db_path, chat_id), + load_notes_rows(db_path, chat_id), + load_pins_rows(db_path, chat_id), + ) + + restored_note = next(n for n in restored.nodes.values() if n.kind == "note") + assert restored_note.color is None + + def test_note_edges_survive_a_full_db_round_trip(tmp_path): """Regression for the note-edge data-loss bug. diff --git a/web_ui/src/app/canvas/GroupColorPicker.tsx b/web_ui/src/app/canvas/GroupColorPicker.tsx index 292c6f8f..d37a9993 100644 --- a/web_ui/src/app/canvas/GroupColorPicker.tsx +++ b/web_ui/src/app/canvas/GroupColorPicker.tsx @@ -51,11 +51,6 @@ export const GROUP_MONO_COLORS: NamedColor[] = [ { name: "Dark Gray", hex: "#454545" }, ]; -// Exported so NoteNodeView's isSystemPrompt dashed border can stay visually -// consistent with the popover's own "Purple" swatch, rather than picking an -// unrelated one-off purple. -export const NOTE_SYSTEM_PROMPT_BORDER_COLOR = GROUP_NAMED_COLORS[2].hex; - export interface GroupColorPickerProps { color: string | null; headerColor: string | null; diff --git a/web_ui/src/app/canvas/NoteNodeView.test.tsx b/web_ui/src/app/canvas/NoteNodeView.test.tsx index d6d9a58a..93b160a6 100644 --- a/web_ui/src/app/canvas/NoteNodeView.test.tsx +++ b/web_ui/src/app/canvas/NoteNodeView.test.tsx @@ -138,6 +138,12 @@ describe("NoteNodeView", () => { expect(container.querySelector(".note-node.system-prompt")).not.toBeNull(); expect(screen.getByTitle("System Prompt")).toBeInTheDocument(); expect(screen.queryByTitle("Summary Note")).toBeNull(); + // The marker is carried by shape (the .system-prompt class's dashed, + // heavier border) and the badge above - never by an inline colour. It + // used to take the colour picker's own "Purple" swatch, which made a + // semantic marker indistinguishable from a note somebody had simply + // coloured purple. + expect(container.querySelector(".note-node")!.style.borderColor).toBe(""); rerender( diff --git a/web_ui/src/app/canvas/NoteNodeView.tsx b/web_ui/src/app/canvas/NoteNodeView.tsx index 3aacc28b..45aa3dc9 100644 --- a/web_ui/src/app/canvas/NoteNodeView.tsx +++ b/web_ui/src/app/canvas/NoteNodeView.tsx @@ -1,6 +1,6 @@ import { type Node, type NodeProps } from "@xyflow/react"; import { memo, useEffect, useRef, useState } from "react"; -import { GroupColorPicker, NOTE_SYSTEM_PROMPT_BORDER_COLOR } from "./GroupColorPicker"; +import { GroupColorPicker } from "./GroupColorPicker"; import { NodeMarkdown } from "./NodeMarkdown"; import { NodeShell } from "./NodeShell"; @@ -116,10 +116,14 @@ function NoteNodeViewImpl({ data, selected }: NodeProps) { // see this file's own module doc - so this is always false, never // wired to useLodVisibility. collapsed={false} - style={{ - backgroundColor: data.color ?? undefined, - borderColor: data.isSystemPrompt ? NOTE_SYSTEM_PROMPT_BORDER_COLOR : undefined, - }} + // No borderColor override for a system prompt. It used to be the + // picker's own "Purple" swatch, which put a SEMANTIC marker (this note + // governs the branch) and a USER CHOICE (I coloured this note purple) + // in the same visual channel - indistinguishable the moment someone + // picks Purple for a note body. The meaning is carried by shape + // instead: .note-node.system-prompt's dashed 2px border, plus the ⚙ + // badge in the header. See styles.css for the token it borders in. + style={{ backgroundColor: data.color ?? undefined }} header={
diff --git a/web_ui/src/app/styles.css b/web_ui/src/app/styles.css index e366cf07..f603130a 100644 --- a/web_ui/src/app/styles.css +++ b/web_ui/src/app/styles.css @@ -7500,9 +7500,16 @@ mark.document-view-search-match-current { max-width: 360px; } +/* A system prompt is marked by SHAPE, not by hue: a dashed, heavier border + plus the header's own badge. It used to also take the colour picker's + "Purple" swatch as its border colour, which put a semantic marker and a + user's own colour choice in the same channel - a note the user coloured + purple was then indistinguishable from one governing the branch. The + token below keeps it legible in both themes without spending a colour. */ .note-node.system-prompt { border-style: dashed; border-width: 2px; + border-color: var(--gl-surface-text-muted); } .note-node-header {