Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion backend/chat_library.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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,
Expand Down
18 changes: 17 additions & 1 deletion backend/session_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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
Expand Down
112 changes: 112 additions & 0 deletions backend/tests/test_session_format_adr009.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
5 changes: 0 additions & 5 deletions web_ui/src/app/canvas/GroupColorPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions web_ui/src/app/canvas/NoteNodeView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLElement>(".note-node")!.style.borderColor).toBe("");

rerender(
<ReactFlowProvider>
Expand Down
14 changes: 9 additions & 5 deletions web_ui/src/app/canvas/NoteNodeView.tsx
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -116,10 +116,14 @@ function NoteNodeViewImpl({ data, selected }: NodeProps<NoteFlowNode>) {
// 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={
<div className="scene-node-title note-node-header" style={{ backgroundColor: data.headerColor ?? undefined }}>
<span className="note-node-badges">
Expand Down
7 changes: 7 additions & 0 deletions web_ui/src/app/styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading