Return full pericope references on request - #332
henrique221 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesPericope retrieval
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant PericopesRoute
participant PericopesService
participant PericopesRepository
participant PostgreSQL
Client->>PericopesRoute: GET chapter pericopes with query option
PericopesRoute->>PericopesRoute: Validate query parameter
PericopesRoute->>PericopesService: Request chapter pericopes
PericopesService->>PericopesRepository: Pass full-pericope option
PericopesRepository->>PostgreSQL: Query chapter or intersecting pericope groups
PostgreSQL-->>PericopesRepository: Ordered verse rows
PericopesRepository-->>PericopesService: Pericope verses
PericopesService-->>PericopesRoute: Grouped pericopes
PericopesRoute-->>Client: HTTP response
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The new option is wired through validation, service, repository filtering, and integration coverage without an identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Review
What was checked:
- With
includeFullPericopes=true, a pericope that crosses a chapter boundary comes back whole from either chapter (e.g. Mark 8:31–9:1). - Without the parameter, or with
false, the response is exactly what it was before, same verses and same order. Existing callers see no change. - An invalid value returns 400.
- Groups are matched on set, book, section and pericope number, so FCBH (where numbers repeat per section) and FIA (where section is null) both work. I walked through the test fixtures and the query pulls in neither too many verses nor too few.
- Auth and the AI queuing path are untouched.
- The PGlite test is a real one: it runs the actual route, service and SQL, not mocks.
Follow-ups (not blocking)
1. The true path reads more rows than it needs.
When the flag is on, the outer query no longer filters by chapter. The chapter filter moved inside the EXISTS subquery, which runs against the chapter_verses alias. So the outer query picks up every verse of the book for that pericope set, then runs one EXISTS check per row.
In practice this should still be fast, because idx_pericope_verses_set_book_pericope covers the subquery lookup. But the cost now scales with the size of the book instead of the size of the chapter. Two things worth doing:
- Run
EXPLAIN ANALYZEon a long book (Psalms) to confirm it is actually fine. - If it isn't, split it into two steps: first find which
(pericopeNumber, section)pairs touch the requested chapter — that part is cheap and indexed — then fetch the verses for just those pairs.
One detail that may matter for the plan: the subquery mixes eq() with a raw IS NOT DISTINCT FROM. Postgres can usually only flatten an EXISTS into a fast semi-join when every condition is a plain =, so this one may end up re-running per row.
2. The parameter accepts fewer values than a similar one elsewhere.
includeFullPericopes accepts 'true' and 'false'. But includeContent, in translation-resources.types.ts, also accepts '1' and '0'. Same kind of on/off flag, two different rules. So ?includeFullPericopes=1 returns a 400 here even though the equivalent works on the other endpoint.
Either match that enum, or pull a small shared booleanQueryParam() helper out so the next flag doesn't drift again.
Nits
pericopes.integration.test.tsrebuilds the auth mock setup thatpericopes.test.tsalready provides asasAuthenticatedUser.- The hand-written
CREATE TABLEblock in that test can drift fromschema.ts. It matches on the columns that matter today, but it leaves outidx_pericope_verses_set_book_pericope, which is the index the newEXISTSrelies on. So the test can't catch an index regression. - The subquery compares
chapter_verses.pericope_set_idandbook_idagainst the outer row's columns instead of thepericopeSetIdandbookIdparameters that are already bound. Same result, since the outerWHEREpins them, but using the parameters directly reads more clearly. - The raw
sqlfragment forIS NOT DISTINCT FROMisn't visible to the typed query builder, so a column rename wouldn't be caught at compile time.or(and(isNull(a), isNull(b)), eq(a, b))does the same thing and is type-checked.
Reviewed with AI assistance.
2 participants
The chapter pericope endpoint can now return complete groups with
?includeFullPericopes=true. Mark 8 and Mark 9 both return the full Mark 8:31-9:1 group. Omitted or false keeps the existing chapter-only response, and invalid values return 400.The query expands only groups intersecting the requested chapter, matching set, book, section and pericope number. Authorization, the response shape and AI queuing stay unchanged. A PostgreSQL integration test uses PGlite as a dev dependency to exercise the real route, service and SQL without an external database.
Validation: 616 tests passed, including the new cross-chapter and isolation cases; typecheck, build, scoped lint and formatting passed.
Required by eten-tech-foundation/fluent-web#487. Deploy this API support before that web change. Refs eten-tech-foundation/fluent-web#486.
Summary by CodeRabbit
New Features
includeFullPericopesquery parameter to chapter pericope requests.400 Bad Requestresponse.Tests