From 4aff8c70a0c626774c469fd66ccdf2d2ed8a9126 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 24 May 2026 20:52:11 -0400 Subject: [PATCH] =?UTF-8?q?feat(backlinks):=20suppress=20parent=E2=86=94ch?= =?UTF-8?q?ild=20mentions=20from=20"Mentioned=20in"=20panel=20(TASK-1607)?= =?UTF-8?q?=20(#625)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a parent's page (typically a PLAN with many child TASKs), the "Mentioned in" panel was dominated by child tasks that wiki-link back to their parent. Those children are already listed in the Children section directly above the panel, so the same items appeared twice on screen and buried genuine cross-references (sibling plans, retro docs, etc.). Symmetric problem on a child's page: the parent shows up in "Mentioned in" even though it's already in the "Parent: …" header. Suppression is server-side in two parts, modeled on the existing self-link filter in GetBacklinks (s.id != targetItemID): 1. Children-suppression — always-on. Source rows where s.parent_id = targetItemID are dropped. Uses the existing targetItemID parameter; no API change. NULL-safe form so orphan items still surface (raw `s.parent_id != ?` would silently drop NULL parent_id rows under SQL three-valued logic). 2. Parent-suppression — opt-in. New TargetParentID *string field on BacklinksVisibility (zero value = nil = no parent suppression, keeping the ~50 existing test callsites valid). When set, the source row whose id == TargetParentID is dropped. The handler passes item.ParentID from the resolved target. Both apply to CountBacklinks identically so the handlers_backlinks.go same-ws/cross-ws pagination math (which depends on count-vs-fetch agreeing) stays correct. GetCrossWorkspaceBacklinks is unaffected — parent_id is workspace-scoped, so a cross-ws source can't be the target's parent or child by construction. Doc comment added noting this. Tests: - ChildMentionOfParentSuppressed: headline case - ParentMentionOnChildPageSuppressed: symmetric case with/without TargetParentID - SiblingMentionsNotSuppressed: control — siblings of a shared parent that wiki-link each other still surface - OrphanBacklinksUnaffected: regression for the NULL-safe form - PaginationStableAfterSuppression: page 1 + page 2 hit all filtered rows exactly once with no duplicates Promoted from IDEA-1601. Follow-up to [[PLAN-1593]] (which shipped the original wiki-link backlinks reverse index). --- internal/server/handlers_backlinks.go | 8 + internal/store/wiki_links.go | 62 +++++- internal/store/wiki_links_test.go | 263 ++++++++++++++++++++++++++ 3 files changed, 329 insertions(+), 4 deletions(-) diff --git a/internal/server/handlers_backlinks.go b/internal/server/handlers_backlinks.go index acbf3aee..16e6d5f9 100644 --- a/internal/server/handlers_backlinks.go +++ b/internal/server/handlers_backlinks.go @@ -98,6 +98,14 @@ func (s *Server) handleGetItemBacklinks(w http.ResponseWriter, r *http.Request) vis.FullCollectionIDs = fullCollIDs vis.GrantedItemIDs = grantedItemIDs } + // Pass the target's parent_id (when present) so the store layer + // can suppress the parent's own "Mentioned in" entry on the + // target's page — the parent is already on screen in the + // "Parent: …" header above the panel, so its backlink would + // duplicate UI. Children-suppression is unconditional in the + // store; only parent-suppression needs the extra context. + // TASK-1607 / IDEA-1601. + vis.TargetParentID = item.ParentID // Union pagination across same-ws + cross-ws tiers. // diff --git a/internal/store/wiki_links.go b/internal/store/wiki_links.go index b6e5206d..6db740e8 100644 --- a/internal/store/wiki_links.go +++ b/internal/store/wiki_links.go @@ -862,6 +862,15 @@ type BacklinksVisibility struct { Unrestricted bool FullCollectionIDs []string GrantedItemIDs []string + + // TargetParentID, when non-nil and non-empty, suppresses the + // target's own parent from the backlinks result set. Set by the + // handler from items.ParentID. Rationale: the parent already + // appears in the "Parent: …" header on the target's page, so a + // child's wiki-link mention of its parent only duplicates UI + // already on screen. Symmetric with the always-on children- + // suppression in GetBacklinks/CountBacklinks. TASK-1607 / IDEA-1601. + TargetParentID *string } // GetBacklinks returns the items in `workspaceID` that contain a @@ -875,6 +884,18 @@ type BacklinksVisibility struct { // "Mentioned in" panel (PLAN-1593 behavior decision). The row stays // in the index for completeness; the filter is purely cosmetic. // +// Children-suppression is always-on for the same UI-duplication +// reason: a source item whose parent_id == targetItemID is a direct +// child, and children are already listed above the backlinks panel +// in the Child Items section. The child's body almost always +// references the parent (`[[Parent Title]]` somewhere in the +// narrative), which would otherwise dominate the backlinks list +// for any plan with many children. Parent-suppression is the +// symmetric case but opt-in via vis.TargetParentID: when the caller +// passes the target's parent_id, that one source row (the parent's +// mention of the target) is dropped because the parent appears in +// the target's "Parent: …" header. TASK-1607 / IDEA-1601. +// // Ordering: most-recently-updated source first; within an updated_at // tie (e.g. two backlinks land in the same second), break by // position ASC so the order is at least deterministic. @@ -900,6 +921,24 @@ func (s *Store) GetBacklinks(targetItemID, workspaceID string, limit, offset int return nil, nil } + // Relational-suppression clause. Always-on children-suppression + // (s.parent_id != targetItemID) mirrors the self-link filter + // (s.id != targetItemID) two lines down: both hide rows that + // already appear elsewhere in the target's page UI (Children + // section above the backlinks panel) and so carry no novel + // information. NULL-safe form because parent_id is nullable — + // orphan items have parent_id IS NULL and would otherwise be + // erroneously dropped by `s.parent_id != ?`. Parent-suppression + // is opt-in (only when vis.TargetParentID is set) because callers + // that don't have the target's parent_id handy can skip it + // without changing existing semantics. TASK-1607 / IDEA-1601. + relClause := " AND (s.parent_id IS NULL OR s.parent_id != ?)" + args := []interface{}{targetItemID, workspaceID, targetItemID, targetItemID} + if vis.TargetParentID != nil && *vis.TargetParentID != "" { + relClause += " AND s.id != ?" + args = append(args, *vis.TargetParentID) + } + // Build the visibility predicate. Unrestricted → omit. Otherwise // `collection_id IN (...)` OR `id IN (...)` — either branch alone // is acceptable so a granted-item-only access still resolves; an @@ -907,7 +946,6 @@ func (s *Store) GetBacklinks(targetItemID, workspaceID string, limit, offset int // predicate evaluates to FALSE for that branch without breaking // Postgres's empty-list rejection. visClause := "" - args := []interface{}{targetItemID, workspaceID, targetItemID} if !vis.Unrestricted { collClause := "FALSE" if len(vis.FullCollectionIDs) > 0 { @@ -940,7 +978,7 @@ func (s *Store) GetBacklinks(targetItemID, workspaceID string, limit, offset int WHERE wl.target_item_id = ? AND s.workspace_id = ? AND s.deleted_at IS NULL - AND s.id != ?`+visClause+` + AND s.id != ?`+relClause+visClause+` ORDER BY s.updated_at DESC, wl.position ASC LIMIT ? OFFSET ? `), args...) @@ -998,8 +1036,19 @@ func (s *Store) CountBacklinks(targetItemID, workspaceID string, vis BacklinksVi if !vis.Unrestricted && len(vis.FullCollectionIDs) == 0 && len(vis.GrantedItemIDs) == 0 { return 0, nil } + // Relational-suppression clause — must mirror GetBacklinks + // exactly. The handler's same-ws/cross-ws pagination math + // depends on CountBacklinks returning the same number of rows + // GetBacklinks would yield under identical vis; a drift here + // would let suppressed rows consume LIMIT slots and shrink + // pages silently. TASK-1607 / IDEA-1601. + relClause := " AND (s.parent_id IS NULL OR s.parent_id != ?)" + args := []interface{}{targetItemID, workspaceID, targetItemID, targetItemID} + if vis.TargetParentID != nil && *vis.TargetParentID != "" { + relClause += " AND s.id != ?" + args = append(args, *vis.TargetParentID) + } visClause := "" - args := []interface{}{targetItemID, workspaceID, targetItemID} if !vis.Unrestricted { collClause := "FALSE" if len(vis.FullCollectionIDs) > 0 { @@ -1030,7 +1079,7 @@ func (s *Store) CountBacklinks(targetItemID, workspaceID string, vis BacklinksVi WHERE wl.target_item_id = ? AND s.workspace_id = ? AND s.deleted_at IS NULL - AND s.id != ?`+visClause+` + AND s.id != ?`+relClause+visClause+` `), args...).Scan(&n) if err != nil { return 0, fmt.Errorf("count backlinks: %w", err) @@ -1081,6 +1130,11 @@ func (s *Store) CountBacklinks(targetItemID, workspaceID string, vis BacklinksVi // workspace A still leaked cross-ws backlinks from workspace B // because the cross-ws path enumerated via the user's full // workspace list). +// +// No parent↔child suppression here (the GetBacklinks pair adds +// that for same-ws): item.parent_id is workspace-scoped, so a +// cross-ws source row can never be the target's parent or child +// by construction. TASK-1607. func (s *Store) GetCrossWorkspaceBacklinks(targetWorkspaceID, targetRef, requesterUserID string, allowedWorkspaceSlugs []string, limit, offset int) ([]models.Backlink, error) { if limit <= 0 || limit > 300 { limit = 50 diff --git a/internal/store/wiki_links_test.go b/internal/store/wiki_links_test.go index 9bf918ec..42683f46 100644 --- a/internal/store/wiki_links_test.go +++ b/internal/store/wiki_links_test.go @@ -1240,6 +1240,269 @@ func TestWikiLinks_DuplicateSlashTitleNoTheft(t *testing.T) { } } +// createChildItem creates a test item with parent_id set in one +// CreateItem call. The wiki_links tests use this for parent↔child +// suppression coverage where createTestItem (which doesn't set +// parent_id) isn't enough. TASK-1607. +func createChildItem(t *testing.T, s *Store, workspaceID, collectionID, parentID, title, content string) *models.Item { + t.Helper() + pid := parentID + item, err := s.CreateItem(workspaceID, collectionID, models.ItemCreate{ + Title: title, + Content: content, + Fields: `{"status":"open"}`, + ParentID: &pid, + }) + if err != nil { + t.Fatalf("createChildItem: %v", err) + } + return item +} + +// TestWikiLinks_ChildMentionOfParentSuppressed: the headline TASK-1607 +// case. A direct child of `parent` mentioning `parent` in its body +// should NOT appear in parent's "Mentioned in" panel — children are +// already listed in the Child Items section above. Symmetric with +// the long-standing self-link suppression. +func TestWikiLinks_ChildMentionOfParentSuppressed(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent plan", "") + // Child links to parent. Without TASK-1607, this would surface + // as a backlink on parent's page. + child := createChildItem(t, s, ws.ID, col.ID, parent.ID, "Child task", + "See [["+refOf(parent)+"]] for context.") + + got, err := s.GetBacklinks(parent.ID, ws.ID, 50, 0, BacklinksVisibility{Unrestricted: true}) + if err != nil { + t.Fatalf("GetBacklinks: %v", err) + } + if len(got) != 0 { + t.Errorf("expected 0 backlinks (child filtered), got %d: %+v", len(got), got) + } + + // CountBacklinks must agree — pagination math in + // handlers_backlinks.go's same-ws/cross-ws split depends on it. + n, err := s.CountBacklinks(parent.ID, ws.ID, BacklinksVisibility{Unrestricted: true}) + if err != nil { + t.Fatalf("CountBacklinks: %v", err) + } + if n != 0 { + t.Errorf("CountBacklinks: got %d, want 0", n) + } + + // Sanity: the wiki-link row IS in the index (we only suppress + // at read time, not at write time — keeps the index complete + // for future broken-link reports etc.). + var raw int + if err := s.db.QueryRow(s.q(` + SELECT COUNT(*) FROM item_wiki_links + WHERE source_item_id = ? AND target_item_id = ? + `), child.ID, parent.ID).Scan(&raw); err != nil { + t.Fatalf("raw count: %v", err) + } + if raw != 1 { + t.Errorf("raw wiki_links row: got %d, want 1 (index stays complete)", raw) + } +} + +// TestWikiLinks_ParentMentionOnChildPageSuppressed: the symmetric +// case. When the parent's body mentions its child by wiki-link, that +// mention should NOT appear in the child's "Mentioned in" panel — +// the parent is already on screen in the "Parent: …" header above +// the panel. Requires vis.TargetParentID to be set; the handler +// passes item.ParentID from the resolved target. +func TestWikiLinks_ParentMentionOnChildPageSuppressed(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent plan", "") + child := createChildItem(t, s, ws.ID, col.ID, parent.ID, "Child task", "") + + // Now the parent mentions the child. Without the suppression, + // this would dominate the child's backlinks panel. + body := "Tracking work in [[" + refOf(child) + "]]." + if _, err := s.UpdateItem(parent.ID, models.ItemUpdate{Content: &body}); err != nil { + t.Fatalf("UpdateItem parent: %v", err) + } + + // Without TargetParentID: the parent shows up (no suppression). + plain, err := s.GetBacklinks(child.ID, ws.ID, 50, 0, BacklinksVisibility{Unrestricted: true}) + if err != nil { + t.Fatalf("GetBacklinks (plain): %v", err) + } + if len(plain) != 1 { + t.Fatalf("baseline: expected 1 backlink (parent mention), got %d", len(plain)) + } + + // With TargetParentID set to the parent's ID: the parent's + // mention is suppressed. + pid := parent.ID + vis := BacklinksVisibility{Unrestricted: true, TargetParentID: &pid} + got, err := s.GetBacklinks(child.ID, ws.ID, 50, 0, vis) + if err != nil { + t.Fatalf("GetBacklinks (suppressed): %v", err) + } + if len(got) != 0 { + t.Errorf("expected 0 backlinks (parent filtered), got %d: %+v", len(got), got) + } + + n, err := s.CountBacklinks(child.ID, ws.ID, vis) + if err != nil { + t.Fatalf("CountBacklinks: %v", err) + } + if n != 0 { + t.Errorf("CountBacklinks: got %d, want 0", n) + } +} + +// TestWikiLinks_SiblingMentionsNotSuppressed: control case. Sibling +// items (same parent) that wiki-link each other are genuine +// cross-references — they aren't implied by any other UI surface, so +// they must still appear in "Mentioned in". +func TestWikiLinks_SiblingMentionsNotSuppressed(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent plan", "") + siblingA := createChildItem(t, s, ws.ID, col.ID, parent.ID, "Sibling A", "") + siblingB := createChildItem(t, s, ws.ID, col.ID, parent.ID, "Sibling B", + "Coordinates with [["+refOf(siblingA)+"]] on the API surface.") + + // siblingA's backlinks should include siblingB. Children- + // suppression filters by `s.parent_id != targetItemID` — i.e. + // items whose parent IS siblingA. siblingB's parent is the + // parent plan, not siblingA, so siblingB is not a child of + // siblingA and must not be filtered. + got, err := s.GetBacklinks(siblingA.ID, ws.ID, 50, 0, BacklinksVisibility{Unrestricted: true}) + if err != nil { + t.Fatalf("GetBacklinks: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 sibling backlink, got %d: %+v", len(got), got) + } + if got[0].SourceItemID != siblingB.ID { + t.Errorf("expected sibling B as source, got %s", got[0].SourceItemID) + } + + // Symmetric for the parent-suppression direction: if siblingA + // passes TargetParentID=parent.ID (as the handler would since + // siblingA's parent IS parent), siblingB is still NOT the + // parent, so it survives. + pid := parent.ID + vis := BacklinksVisibility{Unrestricted: true, TargetParentID: &pid} + got, err = s.GetBacklinks(siblingA.ID, ws.ID, 50, 0, vis) + if err != nil { + t.Fatalf("GetBacklinks (with parent vis): %v", err) + } + if len(got) != 1 { + t.Errorf("expected 1 backlink with parent vis set, got %d", len(got)) + } +} + +// TestWikiLinks_OrphanBacklinksUnaffected: regression guard for the +// NULL-safe parent_id predicate. Items with parent_id IS NULL +// (orphan / root-level items) must still surface as backlinks; the +// naive form `s.parent_id != ?` would silently drop them because +// SQL three-valued logic treats `NULL != x` as NULL (not TRUE). +func TestWikiLinks_OrphanBacklinksUnaffected(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + target := createTestItem(t, s, ws.ID, col.ID, "Target", "") + // Plain createTestItem produces an orphan item (no parent_id). + orphan := createTestItem(t, s, ws.ID, col.ID, "Orphan source", + "Mentions [["+refOf(target)+"]] from the root.") + + got, err := s.GetBacklinks(target.ID, ws.ID, 50, 0, BacklinksVisibility{Unrestricted: true}) + if err != nil { + t.Fatalf("GetBacklinks: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected orphan source to surface, got %d backlinks", len(got)) + } + if got[0].SourceItemID != orphan.ID { + t.Errorf("expected orphan as source, got %s", got[0].SourceItemID) + } +} + +// TestWikiLinks_PaginationStableAfterSuppression: the suppression +// applies in SQL, so LIMIT/OFFSET counts the filtered set. Mixing +// suppressed and unsuppressed sources, pages 1 and 2 together must +// equal the full set without skipping or doubling. Regression guard +// for the GetBacklinks/CountBacklinks lockstep that the +// handlers_backlinks.go same-ws/cross-ws math depends on. +func TestWikiLinks_PaginationStableAfterSuppression(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "Test") + col := createTestCollection(t, s, ws.ID, "Tasks") + + parent := createTestItem(t, s, ws.ID, col.ID, "Parent plan", "") + + // 3 non-children that mention parent — these should all surface. + var realRefs []string + for i := 0; i < 3; i++ { + src := createTestItem(t, s, ws.ID, col.ID, "Real referrer "+itoa(i), + "Mentions [["+refOf(parent)+"]].") + realRefs = append(realRefs, src.ID) + } + // 5 children that also mention parent — these should be hidden. + for i := 0; i < 5; i++ { + createChildItem(t, s, ws.ID, col.ID, parent.ID, + "Child "+itoa(i), + "Refs [["+refOf(parent)+"]].") + } + + vis := BacklinksVisibility{Unrestricted: true} + + // CountBacklinks must reflect the filtered set, not the raw set. + n, err := s.CountBacklinks(parent.ID, ws.ID, vis) + if err != nil { + t.Fatalf("CountBacklinks: %v", err) + } + if n != 3 { + t.Fatalf("CountBacklinks: got %d, want 3 (3 real, 5 children filtered)", n) + } + + // Page through with LIMIT=2; combined results must hit all 3 + // real referrers exactly once, in updated_at DESC order. + page1, err := s.GetBacklinks(parent.ID, ws.ID, 2, 0, vis) + if err != nil { + t.Fatalf("GetBacklinks page 1: %v", err) + } + page2, err := s.GetBacklinks(parent.ID, ws.ID, 2, 2, vis) + if err != nil { + t.Fatalf("GetBacklinks page 2: %v", err) + } + if len(page1) != 2 { + t.Errorf("page 1: got %d, want 2", len(page1)) + } + if len(page2) != 1 { + t.Errorf("page 2: got %d, want 1", len(page2)) + } + + combined := make(map[string]bool) + for _, bl := range append(page1, page2...) { + if combined[bl.SourceItemID] { + t.Errorf("duplicate source across pages: %s", bl.SourceItemID) + } + combined[bl.SourceItemID] = true + } + if len(combined) != 3 { + t.Errorf("combined unique sources: got %d, want 3", len(combined)) + } + for _, want := range realRefs { + if !combined[want] { + t.Errorf("missing real referrer %s from combined pages", want) + } + } +} + // itoa is strconv.Itoa renamed to keep test bodies readable when // they're already heavy on ref-formatting. func itoa(n int) string {