Feature/sc 45836/allow admins to delete bad links created by

[nsantacruz]

This pull request introduces a comprehensive set of features and improvements to support advanced Linker admin and editor workflows, including new API endpoints, a Django management command, and backend logic for handling deleted citations and schema editing. The most important changes are grouped below.

---

New Linker Admin & Editor API Endpoints

• Added multiple class-based views in api/views.py for staff-only Linker admin actions (delete/recreate/parse citations, rerun segment, and editor endpoints for match templates, address types, and non-unique terms), with robust JSON body handling and error responses. [1] [2]
• Introduced a StaffRequiredMixin to enforce staff-only access for these endpoints.

Backend Logic for Deleted Citations

• Enhanced citation handling in sefaria/helper/linker/tasks.py by tracking and merging deleted citation spans across reruns, ensuring deduplication and correct propagation of deleted state. [1] [2] [3] [4]
• Updated logic to exclude deleted citations from ambiguous and non-segment citation case loaders and link calculation routines. [1] [2] [3] [4] [5]
• Modified the logic for collecting linked references to skip deleted citations.

Management Command for NonUniqueTerm Index

• Added a new Django management command build_nonuniqueterm_index to rebuild the NonUniqueTerm-to-MatchTemplate usage index, ensuring editor dropdowns and search features are accurate after deploys.

Reader UI Integration

• Registered a new sidebar mode "LinkerAdmin" and added a staff-only view for the Linker Editor in the reader UI, enabling access to the new editor interface. [1] [2]

Supporting Imports

• Imported linker_admin and linker_editor helpers in api/views.py to support the new API endpoints.

[gitvelocity-reviewer]

I'll analyze this PR systematically, reviewing the code quality, architecture, implementation, and test coverage.

PR Summary

This PR implements a Linker Editor — a staff-only tool for editing linker metadata (MatchTemplates, AddressTypes, NonUniqueTerms) on schema nodes, plus a LinkerAdmin sidebar panel for debugging and managing linker citations in the reader. Key components:

1. Backend: New linker_admin.py and linker_editor.py helpers, nonuniqueterm_index.py (Redis-backed reverse index), new API views, URL routes
2. Frontend: LinkerEditorPage.jsx (710 lines), LinkerAdminBox in ConnectionsPanel.jsx, CSS
3. Bug fixes: Deleted span handling in tasks.py, error handling in linker_entity_recognizer.py, debug pruning in ref_resolver.py
4. Tests: linker_editor_test.py, linker_entity_recognizer_test.py, additions to linker_test.py, marked_up_text_chunk.py

---

Issues Found

🔴 Potential Bugs

1. linker_admin.py — rerun_linker_for_segment called with wrong payload on recreate

# sefaria/helper/linker_admin.py:130
else:
    rerun_linker_for_segment(normalized, user_id)

normalized has key "lang" but rerun_linker_for_segment calls _required(payload, "lang") — this is fine. However, normalized["ref"] is already normalized, and rerun_linker_for_segment calls Ref(_required(payload, "ref")).normal() again — double normalization, which is harmless but redundant.

2. linker_admin.py — _ibid_history.last_refs assignment (private API)

# sefaria/helper/linker_admin.py:207
ref_resolver._ibid_history.last_refs = Ref(prev_tref)

This directly mutates a private attribute (_ibid_history) with a single Ref object, but last_refs likely expects a list or specific type. This could silently fail or produce incorrect ibid resolution.

3. tasks.py — _mutc_deleted_spans_from_linker_output called unconditionally

# sefaria/helper/linker/tasks.py:797-800
linker_output_deleted_spans = _mutc_deleted_spans_from_linker_output(chunk.ref, chunk.versionTitle, chunk.language)
if existing:
    chunk.spans = _merge_deleted_spans(chunk.spans, existing.spans + linker_output_deleted_spans)

_mutc_deleted_spans_from_linker_output performs a DB query on every call to _replace_existing_chunk, even when there are no deleted spans. This adds a DB round-trip to every linker run. Consider lazy evaluation or caching.

4. ConnectionsPanel.jsx — useEffect dependency array issue

useEffect(() => {
    if (initialTestString) {
      setTestString(initialTestString);
      setSelectedSpan(selectedCitationData?.linkerAdminSpan || getSelectedLinkerAdminSpan(initialTestString));
      parseCitation(initialTestString);
    }
  }, [initialTestString]);

parseCitation is defined in the component body but not in the dependency array. This is a React hooks lint violation (react-hooks/exhaustive-deps). parseCitation closes over testString, selectedCitationData, etc., so stale closures could cause subtle bugs.

5. nonuniqueterm_index.py — Race condition in add_usage_entry

def add_usage_entry(entry: dict) -> None:
    for slug in entry["term_slugs"]:
        entries = get_term_usages(slug)
        if _entry_identity(entry) not in {_entry_identity(e) for e in entries}:
            entries.append(entry)
            set_term_usages(slug, entries)

This is a read-modify-write pattern on Redis without any locking. In a multi-server environment, concurrent calls could result in lost updates. For a staff-only tool this is low risk, but worth noting.

6. ref_resolver.py — annotate_disqualified_matches called twice in remove_incorrect_matches

def remove_incorrect_matches(resolved_refs: List[ResolvedRef]) -> List[ResolvedRef]:
    ResolvedRefPruner.annotate_disqualified_matches(resolved_refs)
    temp_resolved_refs = list(filter(ResolvedRefPruner.is_match_correct, resolved_refs))
    if len(temp_resolved_refs) == 0:
        temp_resolved_refs = ResolvedRefPruner._merge_subset_matches(resolved_refs)
        ResolvedRefPruner.annotate_disqualified_matches(temp_resolved_refs)  # second call

The second annotation call is on temp_resolved_refs (merged subset), which is correct. But the first annotation on resolved_refs sets disqualification_reason on objects that may be reused in the normal (non-debug) path, potentially leaking debug state into production responses. This is a subtle side effect.

7. LinkerEditorPage.jsx — rebuildLinker uses POST to an admin endpoint without CSRF

rebuildLinker: () => Sefaria.apiRequestWithBody('/admin/reset/linker', {}, {}, 'POST'),

The rebuild_linker view in sefaria/views.py is decorated with @staff_member_required but not @ensure_csrf_cookie. The apiRequestWithBody presumably handles CSRF, but the admin endpoint pattern differs from the _api/ prefix pattern. Verify CSRF token is properly sent.

🟡 Code Quality Issues

8. linker_admin.py — Inconsistent error handling for rerun_linker_for_segment on recreate

else:
    rerun_linker_for_segment(normalized, user_id)

The return value (async result with task_id) is discarded. The caller gets no indication that a rerun was queued. This is intentional but undocumented.

9. LinkerEditorPage.jsx — alert() used for user feedback

if (!confirm(Sefaria._('Delete this MatchTemplate?'))) { return; }
// ...
alert(Sefaria._('Linker rebuilt.'));
alert(`Completed linker rerun: ${rerunRef}`);

Using alert()/confirm() is generally discouraged in modern React UIs. These block the main thread and are not accessible. Consider using a toast/notification system consistent with the rest of the app.

10. GeneralAutocomplete.jsx — renderItems receives wrappedGetInputProps unnecessarily

{shouldDisplaySuggestions(isOpen) && renderItems(suggestions, highlightedIndex, getItemProps, wrappedGetInputProps)}

renderItems typically doesn't need getInputProps at all. Passing wrappedGetInputProps here is a backward-compatible change but adds confusion about the API contract.

11. sefaria/views.py — rebuild_linker missing @ensure_csrf_cookie

@staff_member_required
def rebuild_linker(request):

Compare with linker_editor in reader/views.py which has @ensure_csrf_cookie. The admin reset endpoint is called via fetch from the frontend, so CSRF handling needs to be verified.

12. nonuniqueterm_index.py — rebuild() catches all exceptions silently

except Exception as e:  # noqa - one bad index shouldn't abort the whole rebuild

While the comment explains the intent, swallowing all exceptions means corrupted indexes could go unnoticed. Consider at minimum tracking a failure count and returning it alongside the success count.

13. linker_editor.py — all_address_type_names() uses runtime class introspection

def recurse(cls):
    for sub in cls.__subclasses__():
        names.append(sub.__name__[len("Address"):])
        recurse(sub)
recurse(AddressType)

This relies on AddressType subclasses being imported/registered at call time. If subclasses are lazily imported, this could return an incomplete list. The test test_all_address_type_names validates known types, which is good.

🟢 Positive Observations

• _merge_deleted_spans deduplication is well-designed and tested
• StaffRequiredMixin is a clean, reusable pattern
• _parse_api_response in linker_entity_recognizer.py is a good defensive improvement
• prune_refined_ref_part_matches_for_debug cleanly separates debug from production pruning
• nonuniqueterm_index.py module-level docstring is excellent
• The _api/ prefix separation for staff-only endpoints is a good architectural decision
• Tests cover both pure logic and DB-backed scenarios with appropriate separation

---

Test Coverage Assessment

New production code: ~700 lines (Python helpers + views)
New test code: ~300 lines across 3 test files

Coverage is approximately 40-45% of new Python code, below the 70% threshold. Key gaps:

• linker_admin.py: set_linker_citation_deleted, parse_linker_citation, rerun_linker_for_segment have integration tests in linker_test.py but _update_deleted_marker, _delete_generated_link are untested
• api/views.py new views: No unit tests for the view layer (auth enforcement, request parsing)
• nonuniqueterm_index.py: rebuild() function is untested
• Frontend LinkerEditorPage.jsx and LinkerAdminBox: No tests (acceptable for staff tools)

Suggestions:

1. Add tests for _update_deleted_marker with mock MongoDB objects
2. Add a test for rebuild() in nonuniqueterm_index.py with a mock library
3. Add view-level tests for StaffRequiredMixin (403 for non-staff)

---

Code Quality Score

Scope (S): 18/20

• Touches 31 files across frontend, backend, model, helper, URL routing, CSS, templates
• New staff-only page (/linker-editor), new sidebar mode (LinkerAdmin), new API prefix (_api/)
• New management command, new Redis-backed index module
• Cross-cutting: reader, connections panel, text segment, schema model, linker pipeline

Architecture (A): 16/20

• New _api/ URL namespace for staff-only endpoints (clean separation)
• nonuniqueterm_index.py introduces a new Redis-backed reverse index pattern
• StaffRequiredMixin is a reusable CBV pattern
• linker_admin.py / linker_editor.py cleanly separate concerns from views
• prune_refined_ref_part_matches_for_debug cleanly extends the pruner without breaking production path
• Minor: some coupling between LinkerAdminBox and global Sefaria._linkerOutputMap

Implementation (I): 16/20

• nonuniqueterm_index.py: non-trivial Redis-backed reverse index with rebuild/surgical-update duality
• ref_resolver.py: refactoring is_match_correct → get_disqualification_reason with reason propagation is elegant
• _merge_deleted_spans with identity-based deduplication
• LinkerEditorPage.jsx: 710-line recursive tree component with expand/collapse, alt-struct support, term detail panel
• parse_linker_citation: reconstructs RawRef from parts with proper span alignment
• linkerPartsFromSpan: range flattening logic mirrors server-side reconstruction

Risk (R): 12/20

• Staff-only tool limits blast radius significantly
• _ibid_history.last_refs direct mutation is risky (private API)
• annotate_disqualified_matches side-effects on ResolvedRef objects in production path
• Extra DB query per linker run (_mutc_deleted_spans_from_linker_output)
• Redis race condition in add_usage_entry (low risk for staff tool)
• No feature flag; changes to tasks.py affect all linker runs

Quality (Q): 10/15

• Good test separation (pure logic vs. DB-backed vs. Redis cache)
• Tests cover key edge cases (deduplication, deleted spans, range refs)
• Missing coverage for rebuild(), view auth enforcement, _update_deleted_marker
• linker_entity_recognizer_test.py is minimal but targeted
• Module-level docstrings are excellent
• No API documentation for new _api/ endpoints

Performance/Security (P): 3/5

• @staff_member_required on all new endpoints
• _api/ prefix excluded from middleware processing
• Extra DB query per linker run is a performance concern
• No rate limiting on admin endpoints (acceptable for staff-only)

Base Score: 18 + 16 + 16 + 12 + 10 + 3 = 75

Effort Scale: 2463 effective lines → Extra Large tier (ESF: 1.0x), 31 files → Extra Large tier. No bump needed.

Final Score: 75 × 1.0 = 75

Code Quality Data (JSON)

{
  "_schema": "code_quality_v5",
  "total_score": 75,
  "total_factors": "75 × 1.0 (Extra Large ESF) = 75",
  "scope_score": 18,
  "scope_factors": "31 files across frontend/backend/model/routing/CSS/templates; new /linker-editor page, LinkerAdmin sidebar mode, _api/ URL namespace, management command, Redis-backed index module; cross-cutting reader, connections panel, text segment, schema model, linker pipeline",
  "architecture_score": 16,
  "architecture_factors": "New _api/ URL namespace for staff-only endpoints; nonuniqueterm_index.py introduces Redis-backed reverse index pattern; StaffRequiredMixin reusable CBV; linker_admin/linker_editor cleanly separate from views; debug pruning path cleanly separated from production; minor coupling via global Sefaria._linkerOutputMap",
  "implementation_score": 16,
  "implementation_factors": "Redis-backed reverse index with rebuild/surgical-update duality; is_match_correct refactored to get_disqualification_reason with reason propagation; _merge_deleted_spans with identity-based deduplication; 710-line recursive tree component with expand/collapse and alt-struct support; parse_linker_citation reconstructs RawRef from parts with span alignment; linkerPartsFromSpan range flattening mirrors server-side reconstruction",
  "risk_score": 12,
  "risk_factors": "Staff-only tool limits blast radius; _ibid_history.last_refs direct private API mutation; annotate_disqualified_matches side-effects on ResolvedRef in production path; extra DB query per linker run (_mutc_deleted_spans_from_linker_output); Redis race condition in add_usage_entry; no feature flag for tasks.py changes affecting all linker runs",
  "quality_score": 10,
  "quality_factors": "Good test separation (pure logic/DB/Redis); covers key edge cases (deduplication, deleted spans, range refs); missing coverage for rebuild(), view auth enforcement, _update_deleted_marker; linker_