From c9c299ff18f9be6b8173df338ab0f71995fab825 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 26 Sep 2026 20:13:56 -0400 Subject: [PATCH] fix(web,server): a direct item share labels and colours its field chips like a collection share, from defs that disclose only what the item shows (TASK-2248 U3) (#1587) * fix(web,server): a direct item share labels and colours its field chips like a collection share, from defs that disclose only what the item shows (TASK-2248 U3) Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9 * test(server): the U3 leak scan says what it asserts, and names the label/type disclosure as the reviewed decision (TASK-2248 U3, codex round 1) Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9 --- internal/server/handlers_share_links.go | 71 +++++++++++ internal/server/share_item_field_defs_test.go | 106 +++++++++++++++++ .../components/share/PublicFieldChips.svelte | 61 ++++++++++ .../share/PublicItemExpansion.svelte | 111 ++---------------- web/src/lib/components/share/shareView.ts | 50 ++++++++ web/src/lib/types/index.ts | 4 + web/src/routes/s/[token]/+page.svelte | 20 +++- .../s/[token]/shareFieldChips.svelte.test.ts | 78 ++++++++++++ 8 files changed, 395 insertions(+), 106 deletions(-) create mode 100644 internal/server/share_item_field_defs_test.go create mode 100644 web/src/lib/components/share/PublicFieldChips.svelte create mode 100644 web/src/routes/s/[token]/shareFieldChips.svelte.test.ts diff --git a/internal/server/handlers_share_links.go b/internal/server/handlers_share_links.go index 4c583f312..417b5b5cf 100644 --- a/internal/server/handlers_share_links.go +++ b/internal/server/handlers_share_links.go @@ -390,6 +390,68 @@ func (s *Server) requireShareLinkTargetVisible(w http.ResponseWriter, r *http.Re // `writeJSON(w, ..., item)` would publish destinations on every public share // link. TestMovedTo_ShareLinkNeverCarriesPointer pins both the key set and // that specific omission. +// publicShareFieldDef is what a single-item share tells its reader about one +// field (TASK-2248 U3, audit C121). The direct item share used to carry no +// schema at all, so its field chips printed raw keys (`in_progress`) with no +// colour, while the same item expanded in a COLLECTION share rendered them +// properly. It is deliberately NOT the FieldDef: a link to one item never +// disclosed its collection's schema, so this carries only what renders the +// values the item already shows. There are no option lists, no relation +// target, no defaults and no validation rules. TerminalOptions is at most the +// item's own current value, and only when that value is terminal, because +// that is the one fact the chip colour needs. +type publicShareFieldDef struct { + Key string `json:"key"` + Label string `json:"label"` + Type string `json:"type"` + TerminalOptions []string `json:"terminal_options,omitempty"` +} + +// publicShareFieldDefs returns, in schema order, the defs for the fields the +// item CARRIES A VALUE for. Computed fields are left out, as the collection +// share's expansion leaves them out. Returns nil when there is nothing to +// describe, or when the schema or the fields do not parse, so the caller omits +// the key and the page falls back to printing the values as it always did. +func publicShareFieldDefs(schemaJSON, fieldsJSON string) []publicShareFieldDef { + s := strings.TrimSpace(schemaJSON) + f := strings.TrimSpace(fieldsJSON) + if s == "" || f == "" { + return nil + } + var schema models.CollectionSchema + if err := models.UnmarshalItemFieldSchema([]byte(s), &schema); err != nil { + return nil + } + var values map[string]any + if err := json.Unmarshal([]byte(f), &values); err != nil { + return nil + } + var defs []publicShareFieldDef + for _, fd := range schema.Fields { + if fd.Computed { + continue + } + v, ok := values[fd.Key] + if !ok || v == nil { + continue + } + if str, isStr := v.(string); isStr && strings.TrimSpace(str) == "" { + continue + } + def := publicShareFieldDef{Key: fd.Key, Label: fd.Label, Type: fd.Type} + if str, isStr := v.(string); isStr { + for _, t := range fd.TerminalOptions { + if t == str { + def.TerminalOptions = []string{str} + break + } + } + } + defs = append(defs, def) + } + return defs +} + func publicShareItemDTO(item *models.Item) map[string]interface{} { dto := map[string]interface{}{ "title": item.Title, @@ -555,6 +617,15 @@ func (s *Server) handleResolveShareLink(w http.ResponseWriter, r *http.Request) if refs := s.mintShareAttachmentRefs(link, item.Content); refs != nil { resp["attachment_refs"] = refs } + // Beside the item, not inside it: the item DTO's key set is pinned + // (TestMovedTo_ShareLinkNeverCarriesPointer). Scoped by the item's + // own collection ID. A lookup failure omits the key rather than + // failing the share, which then renders exactly as it did before U3. + if coll, err := s.store.GetCollection(item.CollectionID); err == nil && coll != nil { + if defs := publicShareFieldDefs(coll.Schema, item.Fields); defs != nil { + resp["field_defs"] = defs + } + } writeJSON(w, http.StatusOK, resp) case "collection": diff --git a/internal/server/share_item_field_defs_test.go b/internal/server/share_item_field_defs_test.go new file mode 100644 index 000000000..dd2ce7b44 --- /dev/null +++ b/internal/server/share_item_field_defs_test.go @@ -0,0 +1,106 @@ +package server + +import ( + "encoding/json" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TASK-2248 U3 (audit C121). A single-item share carries field defs for the +// fields the item shows, so its chips can be labelled and coloured, and +// carries nothing else about the schema. A link to one item never disclosed its +// collection's option lists, relation targets, defaults or rules, and must not +// start to. +const u3Schema = `{"fields":[ + {"key":"status","label":"Status","type":"select","options":["todo","doing","shipped","dropped"],"terminal_options":["shipped","dropped"],"abandoned_options":["dropped"],"default":"todo","required":true}, + {"key":"owner","label":"Owner","type":"relation","collection":"people"}, + {"key":"secret_codename","label":"Codename","type":"text"}, + {"key":"score","label":"Score","type":"number","computed":true}, + {"key":"effort","label":"Effort","type":"select","options":["s","m","l"]} +]}` + +func shareFieldDefsFor(t *testing.T, srv *Server, wsID, collID, ownerID, fields string) (map[string]any, []map[string]any) { + t.Helper() + it, err := srv.store.CreateItem(wsID, collID, models.ItemCreate{Title: "Shared", Fields: fields}) + if err != nil { + t.Fatalf("create item: %v", err) + } + link, err := srv.store.CreateShareLink(wsID, "item", it.ID, "view", ownerID, nil) + if err != nil { + t.Fatalf("create share link: %v", err) + } + rr := doRequest(srv, "GET", "/api/v1/s/"+link.Token, nil) + if rr.Code != http.StatusOK { + t.Fatalf("resolve share: %d: %s", rr.Code, rr.Body.String()) + } + var raw map[string]any + if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil { + t.Fatalf("decode: %v", err) + } + var defs []map[string]any + if d, ok := raw["field_defs"]; ok { + b, _ := json.Marshal(d) + _ = json.Unmarshal(b, &defs) + } + return raw, defs +} + +func TestItemShareCarriesDefsOnlyForTheFieldsItShows(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + ws, err := srv.store.GetWorkspaceBySlug(slug) + if err != nil || ws == nil { + t.Fatalf("get workspace: %v", err) + } + owner, err := srv.store.CreateUser(models.UserCreate{Email: "u3-owner@test.com", Name: "Owner", Password: "pw-owner"}) + if err != nil { + t.Fatalf("create user: %v", err) + } + coll, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{Name: "Features", Schema: u3Schema}) + if err != nil { + t.Fatalf("create collection: %v", err) + } + + t.Run("a terminal value: its own def, in schema order, and nothing else", func(t *testing.T) { + raw, defs := shareFieldDefsFor(t, srv, ws.ID, coll.ID, owner.ID, `{"status":"shipped","effort":"m","score":7,"secret_codename":""}`) + want := []map[string]any{ + {"key": "status", "label": "Status", "type": "select", "terminal_options": []any{"shipped"}}, + {"key": "effort", "label": "Effort", "type": "select"}, + } + if !reflect.DeepEqual(defs, want) { + t.Fatalf("field_defs = %#v\nwant %#v", defs, want) + } + // The label and type of each shown field ARE schema facts, sent on + // purpose: that disclosure is the reviewed decision in #1587, and the + // DeepEqual above pins exactly what it is. What this scan adds is + // narrower. None of the listed schema facts beyond those appears + // anywhere in the response: option lists, the other options, + // abandoned options, defaults, required, the relation target, and the + // labels of fields this item carries no value for. + body, _ := json.Marshal(raw) + for _, leak := range []string{`"options"`, "doing", "dropped", "abandoned_options", `"default"`, `"required"`, "people", "Codename", "Score"} { + if strings.Contains(string(body), leak) { + t.Errorf("the item share discloses %s: %s", leak, body) + } + } + }) + + t.Run("a non-terminal value carries no terminal list at all", func(t *testing.T) { + _, defs := shareFieldDefsFor(t, srv, ws.ID, coll.ID, owner.ID, `{"status":"doing"}`) + want := []map[string]any{{"key": "status", "label": "Status", "type": "select"}} + if !reflect.DeepEqual(defs, want) { + t.Fatalf("field_defs = %#v\nwant %#v", defs, want) + } + }) + + t.Run("an item with no values carries no key, so the page renders as before", func(t *testing.T) { + raw, _ := shareFieldDefsFor(t, srv, ws.ID, coll.ID, owner.ID, `{}`) + if _, ok := raw["field_defs"]; ok { + t.Fatalf("field_defs present on an item with no values: %v", raw["field_defs"]) + } + }) +} diff --git a/web/src/lib/components/share/PublicFieldChips.svelte b/web/src/lib/components/share/PublicFieldChips.svelte new file mode 100644 index 000000000..069ee0f8c --- /dev/null +++ b/web/src/lib/components/share/PublicFieldChips.svelte @@ -0,0 +1,61 @@ + + +{#if chips.length > 0} +
+ {#each chips as { field, text, placeholder, color } (field.key)} +
+
{field.label || formatLabel(field.key)}
+ {#if placeholder} +
{text}
+ {:else} +
{text}
+ {/if} +
+ {/each} +
+{/if} + + diff --git a/web/src/lib/components/share/PublicItemExpansion.svelte b/web/src/lib/components/share/PublicItemExpansion.svelte index f82986c7f..b70067249 100644 --- a/web/src/lib/components/share/PublicItemExpansion.svelte +++ b/web/src/lib/components/share/PublicItemExpansion.svelte @@ -12,15 +12,8 @@ // place avoids introducing a second, divergent {@html} source. import type { FieldDef } from '$lib/types'; import type { PublicItem } from './shareView'; - import { - visibleFields, - formatLabel, - formatFieldValue, - fieldValueColor, - publicRelationText, - PUBLIC_RELATION_TITLE, - } from './shareView'; - import { isRelationType } from '$lib/items/relationFieldTypes'; + import { fieldChips } from './shareView'; + import PublicFieldChips from './PublicFieldChips.svelte'; import StaleBodyNotice from '$lib/components/common/StaleBodyNotice.svelte'; import { mermaidBlocks } from './shareMermaid'; @@ -36,67 +29,14 @@ let { item, fields, html, id }: Props = $props(); - // Schema fields that carry a value on this item, in schema order. Computed - // fields are dropped (visibleFields) — they aren't part of the shared - // snapshot's meaningful data. - // A relation NEVER prints its stored value here (BUG-3016). The value is an - // item id and a share payload has no index behind it, so the entry says what - // it holds — the same placeholder the shared table renders, from the same - // helper. `publicRelationText` carries why resolving it is a visibility - // question rather than a payload one; IDEA-3066 is the unit that would. - let displayFields = $derived( - visibleFields(fields) - .map((f) => ({ - field: f, - value: isRelationType(f.type) - ? publicRelationText(f, item.fields[f.key]) - : formatFieldValue(item.fields[f.key]), - placeholder: isRelationType(f.type) - })) - .filter((entry) => entry.value !== '') - ); - - // Categorical fields (status/priority/select) carry kebab/snake option keys - // the owner sees as title-cased labels — so we label-format + color those. - // Everything else is a LITERAL value (dates, IDs, slugs, URLs, free text) - // and must render verbatim — title-casing `2026-05-31` or `api_token` would - // corrupt it. Mirrors the single-item share view, which renders field values - // raw. - function isCategorical(field: FieldDef): boolean { - return field.key === 'status' || field.key === 'priority' || field.type === 'select'; - } - - function colorFor(field: FieldDef): string | undefined { - const raw = item.fields[field.key]; - if (typeof raw !== 'string') return undefined; - if (isCategorical(field)) return fieldValueColor(field, raw); - return undefined; - } - - function displayValue(field: FieldDef, value: string): string { - // Title Case for categorical values — matches the chip pills on the - // card/list/table surfaces (PLAN-2290 Phase 3; no more SHOUTED status). - const base = isCategorical(field) ? formatLabel(value) : value; - return field.suffix ? `${base} ${field.suffix}` : base; - } + // Schema fields that carry a value on this item, in schema order, as the + // shared rule builds them. The direct item share renders through the same + // rule (TASK-2248 U3); `fieldChips` carries the reasoning. + let chips = $derived(fieldChips(item.fields, fields));
- {#if displayFields.length > 0} -
- {#each displayFields as { field, value, placeholder } (field.key)} - {@const color = colorFor(field)} -
-
{field.label || formatLabel(field.key)}
- {#if placeholder} -
{value}
- {:else} -
{displayValue(field, value)}
- {/if} -
- {/each} -
- {/if} + {#if html} {#if item.contentStale}{/if} @@ -105,7 +45,7 @@
{@html html}
- {:else if displayFields.length === 0} + {:else if chips.length === 0}

No additional details.

{/if}
@@ -124,41 +64,6 @@ border-radius: 0 0 var(--radius-lg) var(--radius-lg); } - .expansion-fields { - display: flex; - flex-wrap: wrap; - gap: var(--space-2); - margin: 0; - } - - .field-chip { - display: inline-flex; - align-items: center; - gap: var(--space-1); - padding: var(--space-1) var(--space-3); - background: var(--bg-tertiary); - border-radius: 999px; - font-size: 0.82em; - } - - .field-chip-label { - color: var(--text-muted); - font-weight: 500; - margin: 0; - } - - .field-chip-value { - color: var(--text-primary); - margin: 0; - } - - /* A value this share cannot resolve (BUG-3016) — reads as a note about the - field rather than as its content. Matches PublicTableView's cell. */ - .field-chip-value.is-placeholder { - color: var(--text-muted); - font-style: italic; - } - .expansion-empty { color: var(--text-muted); font-size: 0.88em; diff --git a/web/src/lib/components/share/shareView.ts b/web/src/lib/components/share/shareView.ts index fa85449af..c0f9495ae 100644 --- a/web/src/lib/components/share/shareView.ts +++ b/web/src/lib/components/share/shareView.ts @@ -482,3 +482,53 @@ export function groupItems( } return result; } + +/** One field chip as a share renders it (see `fieldChips`). */ +export interface FieldChip { + field: FieldDef; + /** What the chip prints: label-cased for a categorical value, with any suffix. */ + text: string; + /** A relation, which a share cannot resolve; printed as a note (BUG-3016). */ + placeholder: boolean; + color?: string; +} + +// Categorical fields (status/priority/select) carry kebab/snake option keys +// the owner sees as title-cased labels, so those are label-formatted and +// coloured. Everything else is a LITERAL value (dates, IDs, slugs, URLs, free +// text) and renders verbatim: title-casing `2026-05-31` or `api_token` would +// corrupt it. +function isCategorical(field: FieldDef): boolean { + return field.key === 'status' || field.key === 'priority' || field.type === 'select'; +} + +/** + * The chips a shared item shows: schema fields carrying a value, in schema + * order, computed fields dropped. ONE rule for both share views (TASK-2248 + * U3). The collection inline-expand and the direct item share used to + * disagree, and the direct share printed raw keys (`in_progress`) with no + * colour. A relation never prints its stored value, which is an item id a + * share has no index for (BUG-3016). + */ +export function fieldChips(values: Record, fields: FieldDef[]): FieldChip[] { + const out: FieldChip[] = []; + for (const field of visibleFields(fields)) { + const raw = values[field.key]; + if (isRelationType(field.type)) { + const text = publicRelationText(field, raw); + if (text !== '') out.push({ field, text, placeholder: true }); + continue; + } + const value = formatFieldValue(raw); + if (value === '') continue; + const categorical = isCategorical(field); + const base = categorical ? formatLabel(value) : value; + out.push({ + field, + text: field.suffix ? `${base} ${field.suffix}` : base, + placeholder: false, + color: categorical && typeof raw === 'string' ? fieldValueColor(field, raw) : undefined, + }); + } + return out; +} diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index 579f977af..c8761bcd0 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -138,6 +138,10 @@ export interface SharePayload { /** BUG-3000 marker: `content` is behind the live document (BUG-3050 U3). */ content_state?: 'applied_pending_flush'; }; + /** Single-item shares only (TASK-2248 U3): defs for the fields the item + * shows, in schema order. Deliberately NOT the schema: key, label, type, + * and terminal_options holding at most the item's own current value. */ + field_defs?: Pick[]; collection?: PublicShareCollection; items?: PublicShareItem[]; // Renderable image-attachment refs embedded in the shared content diff --git a/web/src/routes/s/[token]/+page.svelte b/web/src/routes/s/[token]/+page.svelte index 685cfaf42..aae37c1eb 100644 --- a/web/src/routes/s/[token]/+page.svelte +++ b/web/src/routes/s/[token]/+page.svelte @@ -16,10 +16,13 @@ parsePublicItems, filterEvaluable, matchesFilter, + fieldChips, type PublicCollection, type PublicItem } from '$lib/components/share/shareView'; + import PublicFieldChips from '$lib/components/share/PublicFieldChips.svelte'; import type { + FieldDef, PublicShareCollection, PublicShareItem, PublicShareView, @@ -66,6 +69,9 @@ collection_name?: string; collection_icon?: string; item_ref?: string; + /** Defs for the fields this item shows (TASK-2248 U3). Empty from an + * older server, which then renders the values raw, as before. */ + fieldDefs: FieldDef[]; } | null>(null); // Raw `collection` + `items` branches of the share payload, fed straight to // PublicCollectionView (which parses settings/schema/fields defensively and @@ -444,7 +450,8 @@ contentStale: isBodyStale(data.item), collection_name: data.item?.collection_name, collection_icon: data.item?.collection_icon, - item_ref: data.item?.ref ?? data.item?.item_ref + item_ref: data.item?.ref ?? data.item?.item_ref, + fieldDefs: Array.isArray(data.field_defs) ? data.field_defs : [] }; } else if (data.type === 'collection') { shareType = 'collection'; @@ -520,7 +527,8 @@ contentStale: isBodyStale(data.item), collection_name: data.item?.collection_name, collection_icon: data.item?.collection_icon, - item_ref: data.item?.ref ?? data.item?.item_ref + item_ref: data.item?.ref ?? data.item?.item_ref, + fieldDefs: Array.isArray(data.field_defs) ? data.field_defs : [] }; } else if (data.type === 'collection') { shareType = 'collection'; @@ -614,7 +622,13 @@

{itemData.title}

- {#if Object.keys(itemData.fields).length > 0} + {#if itemData.fieldDefs.length > 0} + +
+ +
+ {:else if Object.keys(itemData.fields).length > 0}
{#each Object.entries(itemData.fields) as [key, value] (key)} {#if value !== null && value !== undefined && value !== ''} diff --git a/web/src/routes/s/[token]/shareFieldChips.svelte.test.ts b/web/src/routes/s/[token]/shareFieldChips.svelte.test.ts new file mode 100644 index 000000000..434c1eecb --- /dev/null +++ b/web/src/routes/s/[token]/shareFieldChips.svelte.test.ts @@ -0,0 +1,78 @@ +// TASK-2248 U3 (audit C121): a direct item share renders its field chips by the +// same rule as the collection inline-expand (labelled, coloured, relations as +// a note) whenever the server sends defs, and exactly as before when it does +// not (an older server). +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { cleanup, render } from '@testing-library/svelte'; +import { fieldChips } from '$lib/components/share/shareView'; +import type { FieldDef } from '$lib/types'; + +const share = vi.hoisted(() => ({ payload: null as unknown })); +vi.mock('$lib/api/client', () => ({ + api: { share: { get: vi.fn(async () => share.payload) } }, + withRequestDeadline: vi.fn(), +})); + +import { page } from '$app/state'; +import SharePage from './+page.svelte'; + +afterEach(() => cleanup()); + +async function settle() { + for (let i = 0; i < 20; i++) await Promise.resolve(); +} + +const defs = [ + { key: 'status', label: 'Status', type: 'select', terminal_options: ['shipped'] }, + { key: 'owner', label: 'Owner', type: 'relation' }, + { key: 'effort', label: 'Effort', type: 'select' }, + { key: 'hours', label: 'Hours', type: 'number', suffix: 'h' }, +] as FieldDef[]; + +describe('fieldChips: one rule for both share views', () => { + it('labels a categorical value, colours a terminal one, notes a relation, keeps a literal verbatim', () => { + const chips = fieldChips({ status: 'shipped', owner: 'u-123', effort: 'in_progress', hours: 3, extra: 'x' }, defs); + expect(chips.map((c) => [c.field.key, c.text, c.placeholder])).toEqual([ + ['status', 'Shipped', false], + ['owner', expect.not.stringContaining('u-123'), true], + ['effort', 'In Progress', false], + ['hours', '3 h', false], + ]); + expect(chips[0].color).toBe('var(--accent-green)'); + // An undeclared key has no def, so it is not a chip. + expect(chips.some((c) => c.field.key === 'extra')).toBe(false); + }); + + it('a computed field is not a chip', () => { + expect(fieldChips({ score: 7 }, [{ key: 'score', label: 'Score', type: 'number', computed: true } as FieldDef])).toEqual([]); + }); +}); + +describe('share page: direct item field chips (C121)', () => { + function mount(extra: Record) { + page.params = { token: 'tok' }; + share.payload = { + type: 'item', + item: { title: 'Feature', ref: 'FEAT-1', fields: JSON.stringify({ status: 'shipped', effort: 'in_progress', owner: 'u-123' }), content: '' }, + ...extra, + }; + return render(SharePage); + } + const chipTexts = (c: HTMLElement) => [...c.querySelectorAll('.item-fields .field-chip')].map((el) => el.textContent?.replace(/\s+/g, ' ').trim()); + + it('with defs from the server: labelled, coloured, the relation as a note and never its id', async () => { + const { container } = mount({ field_defs: defs }); + await settle(); + expect(chipTexts(container)[0]).toBe('Status Shipped'); + expect(chipTexts(container)).toContain('Effort In Progress'); + expect(container.querySelector('.item-fields')!.textContent).not.toContain('u-123'); + const status = [...container.querySelectorAll('.field-chip-value')].find((d) => d.textContent === 'Shipped') as HTMLElement; + expect(status.style.color).toBe('var(--accent-green)'); + }); + + it('without defs (an older server): the raw values, as before', async () => { + const { container } = mount({}); + await settle(); + expect(chipTexts(container)).toContain('Effort in_progress'); + }); +});