use live collections to fetch grammar#281
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a grammarPoints live collection with a custom loader, switches grammar pages to Astro live collection reads, and adds shared error-page routing plus middleware bypasses for signup and error paths. ChangesLive Collection migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/live.config.ts`:
- Around line 19-62: The loadCollection implementation repeats the same
entry-to-response mapping in all branches, and the ids path is missing the
collection-level cacheHint tags used elsewhere. Extract the shared mapping into
a small helper near loadCollection, then update the ids branch to return the
same top-level cacheHint with tags: ["grammar-points"] while keeping the
per-entry grammar-point-... tags unchanged.
In `@src/pages/grammar/`[id]/index.astro:
- Around line 14-26: The guard around getLiveEntry in the grammar page has a
duplicated !grammarPointEntry check in the same redirect condition. Update the
conditional in the Astro page’s grammar entry handling so it checks each failure
case only once, keeping the existing redirect to /grammar when grammarPointEntry
is missing or grammarPointError is present.
In `@src/pages/grammar/`[id]/practice.astro:
- Around line 6-16: The practice page is swallowing backend failures from
getLiveCollection by defaulting to an empty exercises list, so add explicit
error handling in the Astro page using the returned error state from
getLiveCollection, similar to the logic in [id]/index.astro. If
getLiveCollection("grammarPoints", ...) reports an error, stop rendering the
practice content and redirect or otherwise fail clearly instead of continuing
with simpleShuffle and grammarPoints. Keep the fix centered around the
getLiveCollection call and the exercises derivation in practice.astro.
In `@src/pages/grammar/index.astro`:
- Around line 6-23: The grammar page currently ignores failures from
getLiveCollection("grammarPoints") and falls back to an empty list, which hides
loader errors. Update the grammar page logic to check the returned collection’s
error state before rendering Grammar, and surface the failure the same way the
[id]/index.astro page does instead of passing grammar.entries?.map(...) ?? []
when loading fails. Use the getLiveCollection result and the Grammar component
as the key points to locate the fix.
In `@src/pages/grammar/practice/result.astro`:
- Line 16: The grammar entries fetch-and-map logic is duplicated across this
page and the other result/index grammar pages, so centralize it in a shared
helper. Extract a reusable function such as getGrammarPointsData(Astro) that
wraps getLiveCollection, maps entries to entry.data, and handles logging/error
behavior in one place. Update the relevant Astro pages to call that helper
instead of repeating entries?.map((entry) => entry.data) ?? [].
- Around line 7-16: The grammar points loader failure is being masked by
defaulting to an empty array in the Practice results page. Update the logic in
result.astro around getLiveCollection and the SessionResult props to explicitly
handle the error case returned by grammarPointsLoader.loadCollection, similar to
the sibling [id]/index.astro flow: detect when grammar has an error instead of
entries, and either log/redirect/render an error state before passing data
onward. Keep the grammar.entries mapping only for the success path so
SessionResult does not receive a silent empty list.
In `@src/pages/sr/review/`[sessionId]/result.astro:
- Around line 13-24: The grammar live collection load in result.astro is hiding
failures by turning missing entries into an empty array. Update the
`getLiveCollection` handling around `grammar` and `SessionResult` so
`grammar.error` is checked explicitly and a visible fallback/error state is
rendered when it is present, instead of always passing
`grammar.entries?.map(...) ?? []`. Keep the existing “No session result found”
branch for session data, and add a similar user-facing branch for grammar loader
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f427991f-476f-414e-bb65-4ce66a4b0611
📒 Files selected for processing (6)
src/live.config.tssrc/pages/grammar/[id]/index.astrosrc/pages/grammar/[id]/practice.astrosrc/pages/grammar/index.astrosrc/pages/grammar/practice/result.astrosrc/pages/sr/review/[sessionId]/result.astro
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/middleware.ts (1)
7-16: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSwitch
pathHasto exact path matchingurl.pathname.includes(part)will treat short ignore tokens likeerrorandsignupas substrings, so a future route containing either text could bypass auth unintentionally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middleware.ts` around lines 7 - 16, The path ignore check is too broad because pathHas currently uses substring matching, which can accidentally match routes containing tokens like error or signup. Update the middleware path matching logic to use exact pathname segment or full-path equality instead of includes, and adjust the route checks around pathHas so only the intended ignored routes bypass auth.src/pages/grammar/[id]/index.astro (1)
18-26: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLoader errors are conflated with "not found" and silently sent to
/grammar.Unlike the sibling pages in this PR (
grammar/index.astro,[id]/practice.astro,practice/result.astro), which redirect specifically to/errorwhengetLiveCollectionreports anerror, this page treatsgrammarPointErrorthe same as a missing entry and redirects to/grammareither way. A genuine backend/loader failure (as opposed to an invalid/nonexistent id) is thus hidden behind the same UX as a routine 404, with no differentiation for observability or user feedback.🛡️ Proposed fix to distinguish failure modes
-if (!grammarPointEntry || grammarPointError) { +if (grammarPointError) { + return Astro.redirect("/error"); +} + +if (!grammarPointEntry) { return Astro.redirect("/grammar"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/grammar/`[id]/index.astro around lines 18 - 26, The `getLiveEntry` result in `grammar/[id]/index.astro` is treating `grammarPointError` the same as a missing `grammarPointEntry`, which hides loader/backend failures. Update the redirect logic to distinguish the two cases: when `grammarPointError` is present, follow the same `/error` handling used in `grammar/index.astro`, `[id]/practice.astro`, and `practice/result.astro`; only redirect to `/grammar` when the entry is actually absent with no error. Use the `grammarPointEntry`, `grammarPointError`, and `getLiveEntry` symbols to keep the behavior consistent with the sibling pages.
♻️ Duplicate comments (2)
src/features/grammar-point/loader/index.ts (1)
31-54: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
idsbranch omits collection-levelcacheHinttags.The
elsebranch tags the response withgrammar-points-${role}, but thefilter.idsbranch (Lines 33-37) returns no collection-levelcacheHintat all — only the per-entrygrammar-point-${id}tags survive. This was already flagged in a prior review round when this logic lived insrc/live.config.ts; the code has since moved here but the gap remains.♻️ Proposed fix to add a collection-level tag to the `ids` branch
if (filter && "ids" in filter) { - return grammarPointsToEntries( - await fetchGrammarPoints(filter.ids, filter.context), - ); + return { + ...grammarPointsToEntries( + await fetchGrammarPoints(filter.ids, filter.context), + ), + cacheHint: { + tags: ["grammar-points"], + }, + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/grammar-point/loader/index.ts` around lines 31 - 54, The loadCollection method in the grammar points loader is missing a collection-level cacheHint in the filter.ids branch, so only per-entry tags are returned. Update the ids path in loadCollection to include a cacheHint.tags value alongside grammarPointsToEntries(await fetchGrammarPoints(...)), using a stable collection tag consistent with the non-ids branch and the existing grammar-points-* naming. Keep the error handling in the catch block unchanged and make sure both branches return similarly tagged loader responses.src/pages/grammar/practice/result.astro (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
entries?.map((entry) => entry.data) ?? []pattern across pages.The same fetch-and-map logic repeats here, in
grammar/index.astro, and insr/review/[sessionId]/result.astro. Extracting a shared helper (e.g.getGrammarPointsData(Astro)) wrappinggetLiveCollection+ mapping + error handling would reduce this duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/grammar/practice/result.astro` at line 20, The grammar points fetch-and-map logic is duplicated across this page, grammar/index.astro, and sr/review/[sessionId]/result.astro. Extract the shared `getLiveCollection` + `entry.data` mapping + fallback/error handling into a reusable helper like `getGrammarPointsData(Astro)`, then update the `grammar` prop assignment here to call that helper instead of repeating the inline `grammar?.map((entry) => entry.data) ?? []` pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/middleware.ts`:
- Around line 7-16: The path ignore check is too broad because pathHas currently
uses substring matching, which can accidentally match routes containing tokens
like error or signup. Update the middleware path matching logic to use exact
pathname segment or full-path equality instead of includes, and adjust the route
checks around pathHas so only the intended ignored routes bypass auth.
In `@src/pages/grammar/`[id]/index.astro:
- Around line 18-26: The `getLiveEntry` result in `grammar/[id]/index.astro` is
treating `grammarPointError` the same as a missing `grammarPointEntry`, which
hides loader/backend failures. Update the redirect logic to distinguish the two
cases: when `grammarPointError` is present, follow the same `/error` handling
used in `grammar/index.astro`, `[id]/practice.astro`, and
`practice/result.astro`; only redirect to `/grammar` when the entry is actually
absent with no error. Use the `grammarPointEntry`, `grammarPointError`, and
`getLiveEntry` symbols to keep the behavior consistent with the sibling pages.
---
Duplicate comments:
In `@src/features/grammar-point/loader/index.ts`:
- Around line 31-54: The loadCollection method in the grammar points loader is
missing a collection-level cacheHint in the filter.ids branch, so only per-entry
tags are returned. Update the ids path in loadCollection to include a
cacheHint.tags value alongside grammarPointsToEntries(await
fetchGrammarPoints(...)), using a stable collection tag consistent with the
non-ids branch and the existing grammar-points-* naming. Keep the error handling
in the catch block unchanged and make sure both branches return similarly tagged
loader responses.
In `@src/pages/grammar/practice/result.astro`:
- Line 20: The grammar points fetch-and-map logic is duplicated across this
page, grammar/index.astro, and sr/review/[sessionId]/result.astro. Extract the
shared `getLiveCollection` + `entry.data` mapping + fallback/error handling into
a reusable helper like `getGrammarPointsData(Astro)`, then update the `grammar`
prop assignment here to call that helper instead of repeating the inline
`grammar?.map((entry) => entry.data) ?? []` pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bbc2564-d8e2-4e1f-bbea-13044f469072
📒 Files selected for processing (10)
src/features/grammar-point/loader/errors.tssrc/features/grammar-point/loader/index.tssrc/live.config.tssrc/middleware.tssrc/pages/error.astrosrc/pages/grammar/[id]/index.astrosrc/pages/grammar/[id]/practice.astrosrc/pages/grammar/index.astrosrc/pages/grammar/practice/result.astrosrc/pages/sr/review/[sessionId]/result.astro
|
@CodeRabbit review |
✅ Action performedReview finished.
|
1 participant
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes