fix(search): restore start-date priority in comp_start_date (sc-46790)

[YishaiGlasner]

Summary

• Fixes sc-46790: search results sorted by chronology were ordering by end date instead of start date, and refs with a start/end year of 0 were incorrectly falling back to the default (3000).
• PR #3095 rewrote the comp-date logic as getattr(tp, 'end', None) or getattr(tp, 'start', 3000), which unintentionally flipped priority from start→end to end→start, and used or — so a legitimate year of 0 (falsy) was treated as missing and replaced with 3000.
• make_book_index_document (the Books entity index) had the identical regression, mirroring the same buggy expression. Extracted a shared _comp_date_from_time_period() helper and use it in both the text index (TextIndexer) and the book index builders so they can't drift out of sync again.
• Restores the original intent everywhere: prefer start, fall back to end, default to 3000 (or None for the sparse book-index field) only when both are actually None — via explicit is not None checks rather than truthiness.

Test plan

• Confirm search sorted by chronology now orders by start date (e.g. Genesis before Psalms) rather than end date
• Confirm refs with a start/end year of 0 are no longer pushed to the end of chronological results
• Reindex the Books entity index — existing compDate values baked in under the old logic will need a reindex to pick up the fix

[gitvelocity-reviewer]

📊 Code Quality Score: 4/100

Base Score 38 × ESF 0.1 = 3.8, rounded to 4

Category	Score	Factors
🔭 Scope	14/20	One file (sefaria/search.py) is modified, touching a single expression in make_text_index_document. The change affects search result ordering for all texts that have a best_time_period with a start attribute, which is a broad behavioral surface despite the narrow code footprint.
🏗️ Architecture	0/20	No module boundary changed. The fix stays within the existing method and uses the same getattr pattern.
⚙️ Implementation	10/20	The fix replaces a Python or-chain (which treats 0 as falsy) with explicit is not None guards, and reverses the attribute lookup order from end-first to start-first. The three-branch ternary handles: start present, start absent but end present, and neither present (defaulting to 3000).
⚠️ Risk	12/20	The change alters search result ordering for any text whose best_time_period has a start attribute, which is a user-visible ranking change across the Sefaria corpus. The original or-based logic would have produced wrong results when end was 0; the new logic changes which attribute wins when both are present, which could reorder results for texts that previously resolved via end.
✅ Quality	2/15	No test accompanies the fix. The inline comment explaining the 3000 sentinel value was removed and not replaced. The three cases (has start, has only end, has neither) and the start=0 / end=0 edge cases that motivated the is not None change are not covered by any test in the diff.
🔒 Perf / Security	0/5	No performance or security work is present in this change.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}

[Copilot]

Pull request overview

Fixes a regression in how text-index documents derive comp_date for chronological sorting by restoring “start year first” semantics and ensuring year 0 is treated as a valid year (not as “missing”).

Changes:

• Replaces truthiness-based fallback logic with explicit is not None checks when deriving comp_date.
• Restores priority order to prefer TimePeriod.start over TimePeriod.end, defaulting to 3000 only when both are None.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[yitzhakc]

Force merge

[gitvelocity-reviewer]

📊 Code Quality Score: 6/100

Base 23 × ESF 0.25 = 5.75, rounded to 6

Category	Score	Factors
🔭 Scope	4/20	One file modified (sefaria/search.py), two call sites updated (make_text_index_document and make_book_index_document), and one new private function added. No new subsystems or public APIs touched.
🏗️ Architecture	5/20	_comp_date_from_time_period is extracted as a module-level private function, consolidating two previously divergent inline expressions into a single definition. The make_book_index_document call site retains its distinct None-when-no-tp behavior via an outer guard rather than folding it into the helper. No new module boundary or dependency is introduced.
⚙️ Implementation	6/20	The original getattr(tp, 'end', None) or getattr(tp, 'start', 3000) expression treats end=0 as falsy and falls through to start, which is incorrect for year 0. The new helper uses is not None guards, matching the pattern in _author_sort_year, so year 0 survives as a real value. The make_book_index_document site also switches from getattr(tp, 'end', None) or getattr(tp, 'start', 3000) to _comp_date_from_time_period(tp) with the same fix applied.
⚠️ Risk	5/20	Affects sort ordering of search index documents for texts whose TimePeriod has end=0 or start=0; a regression would silently mis-rank those texts in search results. The change is reversible by reverting the file. No migration, schema change, or external API is involved.
✅ Quality	3/15	No test file accompanies the change. The docstring on _comp_date_from_time_period names the year-0 edge case and the is not None rationale, but no automated test drives the year-0 path or the tp=None fallback in either call site.
🔒 Perf / Security	0/5	No performance or security work is present in this diff.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}