Skip to content

Feature/sc 46895/linker bulk corrector - #3678

Open
nsantacruz wants to merge 29 commits into
masterfrom
feature/sc-46895/linker-bulk-corrector
Open

nsantacruz wants to merge 29 commits into
masterfrom
feature/sc-46895/linker-bulk-corrector

Conversation

@nsantacruz

Copy link
Copy Markdown
Contributor

This pull request introduces a new "Linker Bulk Corrector" feature for staff users, providing a UI and backend API for searching, navigating, and bulk re-parsing stored linker citation spans. It also adds support for tracking reasons when adding dataset examples and includes several related backend enhancements and optimizations.

New Feature: Linker Bulk Corrector

  • Adds a staff-only "Linker Bulk Corrector" page and route to the frontend (ReaderApp.jsx, ReaderPanel.jsx, reader/views.py, sefaria/urls_library.py, static/css/linker-bulk-corrector.css). [1] [2] [3] [4] [5] [6] [7] [8]
  • Implements corresponding API endpoints for search, navigation, and (re-)parsing citations and datasets (api/linker_bulk_corrector_views.py, sefaria/urls_shared.py). [1] [2] [3]

Backend Enhancements for Linker Tasks

  • Adds batch parsing and bulk dataset re-parsing Celery tasks, and synchronous helpers for batch operations (sefaria/helper/linker/tasks.py, sefaria/helper/linker_resource_panel_admin.py). [1] [2] [3]
  • Registers the new "bulk_reparse_dataset" action in the linker editor history (sefaria/model/linker_editor_history.py).

Dataset Example Improvements

  • Adds support for an optional reason field when adding or updating linker dataset examples, including backend, model, and schema updates (sefaria/helper/linker_resource_panel_admin.py, sefaria/model/linker_dataset_example.py). [1] [2] [3] [4] [5]

Database and Model Optimizations

  • Adds new database indices for marked_up_text_chunks and linker_output collections to optimize queries by ref, versionTitle, and language (sefaria/system/database.py).
  • Adds a disqualificationReason field to the LinkerOutput model schema to track why a citation span might be disqualified (sefaria/model/marked_up_text_chunk.py).

These changes collectively provide staff with powerful tools for managing and correcting linker citations at scale, while improving tracking and performance for related data operations.

@gitvelocity-reviewer

Copy link
Copy Markdown

📊 Code Quality Score: 61/100

Base Score 61 × ESF 1.0 (Extra Large: 1360 effective lines, 16 files) = 61

Category Score Factors
🔭 Scope 16/20 The change spans 16 files across backend helper (linker_bulk_corrector.py, 576 lines), Celery tasks (tasks.py), API views (linker_bulk_corrector_views.py), model schema additions (linker_dataset_example.py, marked_up_text_chunk.py, linker_editor_history.py), DB index definitions (database.py), URL routing (urls_library.py, urls_shared.py), Django view (reader/views.py), frontend page (LinkerBulkCorrectorPage.jsx, 514 lines), CSS (linker-bulk-corrector.css, 331 lines), and template (base.html). Four new API endpoints and one new page route are introduced.
🏗️ Architecture 13/20 A new linker_bulk_corrector helper module is introduced, mirroring the existing linker_resource_panel_admin pattern. Two new Celery tasks (parse_linker_citations_batch_task, bulk_reparse_dataset_task) are added to the existing tasks.py. A new view module linker_bulk_corrector_views.py follows the linker_admin_views pattern with a shared _handle base class. Compound indices on (ref, versionTitle, language) are added to marked_up_text_chunks and linker_output collections. No new external dependencies are introduced.
⚙️ Implementation 15/20 group_citation_spans groups and sorts citation spans across LinkerOutput documents by a composite key. navigate_dataset implements cursor-based forward/backward navigation with a NAVIGATION_SCAN_LIMIT guard, calling parse_linker_citation_sync per item in a loop. persist_citation_resolution coordinates updates across LinkerOutput, MarkedUpTextChunk, and Link collections, including reuse of existing links via _update_generated_link. render_citation_snippet generates a windowed HTML snippet and applies _mark_snippet_anchors via regex substitution. The React component manages dataset state, localStorage persistence, keyboard shortcuts (ArrowRight/l, ArrowLeft/h, r), Celery task polling at 1500ms intervals, and a 20-iteration navigation loop.
⚠️ Risk 12/20 persist_citation_resolution writes to LinkerOutput and MarkedUpTextChunk documents and calls tracker.delete on Link objects, making it a multi-collection mutation path. bulk_reparse_dataset_task runs as a Celery task and iterates over all filtered citations in a dataset, calling persist_citation_resolution for each. All endpoints are gated by StaffRequiredMixin. No feature flag or rollback plan is documented. DB index additions are additive and safe.
✅ Quality 2/15 No tests accompany the PR. linker_bulk_corrector.py (576 lines), linker_bulk_corrector_views.py, LinkerBulkCorrectorPage.jsx, and the two new Celery tasks in tasks.py have no test coverage. parse_linker_citations_batch_sync and parse_linker_citations_batch in linker_resource_panel_admin.py are also untested.
🔒 Perf / Security 3/5 Compound indices on (ref, versionTitle, language) are added proactively for the query patterns used in _book_query and _load_item. search_citations dispatches a single batch Celery task via parse_linker_citations_batch_sync rather than N individual tasks. navigate_dataset makes one synchronous Celery call per item in a loop (up to NAVIGATION_SCAN_LIMIT = 200), which is inconsistent with the batch approach used in search. All four API endpoints require staff authentication via StaffRequiredMixin.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

Scored by GitVelocity · How are scores calculated?

nsantacruz and others added 24 commits September 17, 2026 10:16
The Book Title input now accepts any parseable ref (book/section/segment/range),
not just an exact index title. Entering a ref jumps the search to that point
in the book's citation list, surfaces a "citations passed" stat alongside the
usual book-wide stats, and normalizes the input back to the canonical book
title once resolved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The "+ Ref Dataset" button now stores possiblyBadCitation on the saved
LinkerDatasetExample: the normalized-text offset of the citation the
corrector was looking at when they clicked, since that citation is
usually the one that was misclassified and prompted the resave.

Also fix the bulk corrector sidebar (Stats/History/Shortcuts), which
had no bounded height or overflow and so could never scroll on its own.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sidebar stats and the nav overlay count different things (whole book
vs. the current status filter), which was confusing with no labels.
Now the sidebar names the book and states it covers all citations,
and the nav overlay shows which status(es) it's counting.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The sidebar was set to a full 100vh but starts 60px down the page
(below the site header), so its bottom edge overshot the viewport
and cut off the last section (Shortcuts). Match the calc(100vh - 60px)
convention already used by the parent .linkerBulkCorrector container.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Swap the Search button's label for an animated spinner while a search
is in flight, matching the spinner pattern already used for "Search"
buttons in the modtools admin pages (static/css/modtools.css), scoped
here to the bulk corrector's own stylesheet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Back/Forward previously rendered as bare, unstyled buttons. Give them
the same "button" class (Sefaria's standard blue button) already used
by Search/Re-parse Results, and swap their label for the same
lbcSpinner used on Search while a navigate request is in flight.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The sidebar was rendering dataset.bookTitle directly, the same
controlled state bound to the Book Title text field, so the label
updated on every keystroke instead of only once a search actually
completed. Track the book the displayed stats belong to separately
(statsBookTitle), set only from a search response.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Search/Back/Forward all shared one loading flag, so pressing any one
of them spun all three. Track which specific action is in flight
(activeAction) so only the pressed button shows the spinner, while
the other two get the standard greyed-out disabled button style.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Opens /linker-editor?book=<current book> in a new tab, next to Search
and Re-parse Results, using the same window.open(..., "_blank",
"noopener") pattern already used by LinkerAdminBox.jsx for cross-links
between the linker admin tools. Widened the search row's grid to fit
the extra button.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The field now also accepts a full Sefaria URL (any scheme/domain
variation, e.g. sefaria.org or sefaria.org.il), or a bare URL-encoded
ref path segment (e.g. Responsa_Rav_Pealim%2C_Volume_I%2C_Yoreh_Deah.1),
as a fallback when the raw input doesn't resolve directly as a book
title or ref. Strips the domain/query/trailing lang-version path and
percent-decodes before retrying; Ref() itself already normalizes the
remaining underscores and dots.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Shorten the button label and reduce the search row's column-gap from
18px to 10px so the status filter toggles have more room now that a
7th column (Linker Editor) shares the row.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add a checkbox under Forward that, when checked, steps to the next
already-filter-matching citation using its stored status instead of
the normal scan (which re-parses and persists every citation it
passes over, up to 200 at a time). Backend: navigate_dataset takes an
autoCorrect flag; when false, _navigate_without_reparsing does a plain
index lookup into the status-filtered list with no live parsing or
DB writes. Scoped to Forward only per request, to compare feel/speed
against the existing auto-correcting Back/Forward behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The real cost on every Search/Forward/Back click, in both normal and
fast mode, was _full_grouped_items(dataset): a full Mongo fetch of
every LinkerOutput doc for the book plus grouping/sorting all of it
in Python, including a Ref(ref).order_id() parse per unique citation.
The new "fast" mode only skipped the live re-parse/persist step, not
this, so it stayed slow on anything but tiny books.

Cache the grouped/sorted item list per (bookTitle, versionTitle, lang)
with a generation counter per book. persist_citation_resolution (the
one write path all mutations funnel through: single reparse, the
auto-correcting scan, and bulk reparse) bumps the book's generation,
invalidating every cached variant for that book. A 600s TTL is kept
as a safety net, not the correctness mechanism.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
render_citation_snippet was re-querying LinkerOutput from Mongo for
every single displayed citation, purely to get the other spans in
that segment for context highlighting - even though the whole book's
docs were already fetched (and, since the last commit, cached) to
build the sorted item list moments earlier.

Restructure the cache to store spans keyed by (ref, versionTitle,
language) as the single source of truth for a book generation, and
have both group_citation_spans and render_citation_snippet read from
it. This removes one full Mongo round-trip per citation shown, on
top of the whole-book fetch already being cached.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The checkbox only gated the forward direction. Extend it to backward
too, and move it out from under the Forward button to the neutral
position/status area between the two buttons, since it now affects
both.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_navigate_without_reparsing was skipping the live parse entirely, so
serialize_citation_result had no parse_result to draw refParts/
parsings from and those panels rendered empty. The actual cost that
made auto-correcting navigation slow was the up-to-200-item scan that
re-parses and persists every citation it passes over - not parsing
the single item you land on. Do that one parse (same cost search
already pays for its displayed item) so the review panels render,
while still skipping the multi-item scan and any DB writes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd search

Fast (no-auto-correct) navigation and search already re-parse the
landed-on citation to populate ref parts/parsings, but discarded that
result for the status badge and snippet coloring, falling back to the
stored (possibly stale) failed/ambiguous flags. A citation fixed via
the linker editor since it was last persisted would still show red
even though it now parses fine. Reuse the live parse result to derive
the displayed status/coloring instead, without persisting.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
.lbcSidebar sized itself via height: calc(100vh - 60px), assuming the
real page header above it is always exactly 60px. Whenever that's off
(taller header/banner, short viewport, more history entries), the box
was mis-sized with no internal scrollbar to reveal it, clipping the
last section (Shortcuts, including the 'r' re-parse shortcut). Switch
to a self-sizing max-height (100dvh) with box-sizing: border-box so
padding is included, and align-self/min-height so the flex/grid
defaults don't fight the cap.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_navigate_without_reparsing located the current cursor (ref+charRange)
inside the status-filtered item list. Re-parsing the current citation
can change its status (e.g. unparsed -> parsed), dropping it out of
that filtered list right when you re-parse it, so the next Forward/
Back click couldn't find the cursor and fell back to idx=-1/len(list)
- silently restarting from the beginning (or end) of the book instead
of continuing from where the user was.

Locate the cursor in the full, unfiltered item list instead (it always
still contains the just-viewed citation regardless of status changes),
then walk forward/backward from there checking each item's status
against the filter, mirroring what the auto-correcting navigation path
already does. Recompute the "position" field (used for the N/total
display) from the full list so its meaning is unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ector

The auto-correct path re-parsed and persisted every citation it scanned
past while navigating, which was slow and had its own class of bugs
(e.g. the cursor-tracking issue just fixed for fast mode). The fast
mode is a strict improvement on its own - cheap, no side effects during
navigation - so drop the slower option entirely instead of maintaining
both.

Backend: merge _navigate_without_reparsing into navigate_dataset (now
the only implementation), removing the autoCorrect branch, the
200-item scan-and-persist walk, NAVIGATION_SCAN_LIMIT, and the
now-unused continuationCursor/_cursor_for_item plumbing.

Frontend: remove the fastForward checkbox/state and the autoCorrect
flag, and simplify navigate() to a single request/response instead of
a retry loop chasing a continuation cursor.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ctor

The result header ref is now an <a> to the segment. A new section below
Ref Parts shows the CRRD test string for the citation, matching the
markup/styles of the linker admin connections panel's CRRD Test String
section (click to copy).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the sidebar's calc(100vh - 60px)/sticky layout with an
absolutely-positioned root (inset to the viewport) and independent
scroll containers for the sidebar and main column, so each scrolls on
its own rather than relying on viewport-height math tied to a
hardcoded header height. Also gives lbcMain extra bottom padding so
content can scroll up above the fixed forward/back nav overlay.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The snippet was always rendered ltr, so Hebrew citations displayed
with the wrong reading direction and default alignment. dir is now set
per-item ('rtl' for he, 'ltr' otherwise).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant