Add blog post alert UI on homepage #13815
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13815 +/- ##
==========================================
+ Coverage 52.63% 52.74% +0.10%
==========================================
Files 886 893 +7
Lines 33636 33728 +92
Branches 5093 5103 +10
==========================================
+ Hits 17705 17789 +84
- Misses 14579 14593 +14
+ Partials 1352 1346 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
All images
No significant size changes |
Show up to 3 most recent unread blog posts from the PrairieLearn RSS feed as a dismissible alert to instructors. Includes RSS fetching cron job that runs hourly, database schema for caching posts and tracking read status, and homepage integration with dismiss functionality. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Rename all "blog" references to "news" throughout the codebase - Add RSS category filtering (configurable via newsFeedCategories) - Default newsFeedUrl to null (feature disabled by default) - Add e2e tests for news alert visibility and dismissal Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
reteps
force-pushed
the
reteps/blog-alert-ui
branch
from
a763ffd to
ce422f1
Compare
github-actions
bot
added
the
documentation
|
| Filename | Overview |
|---|---|
| apps/prairielearn/src/migrations/20260116150000_news_alerts__create.sql | Creates two tables for caching news items from RSS feed and tracking user read timestamps |
| apps/prairielearn/src/lib/news-feed.ts | RSS parser service that fetches, filters by category, validates, and caches news items |
| apps/prairielearn/src/models/news-items.ts | Model functions for selecting unread news and marking as read, with sequential upserts |
| apps/prairielearn/src/pages/home/components/NewsAlert.tsx | React component rendering dismissible alert with news items and form submit button |
| apps/prairielearn/src/pages/home/home.tsx | Route handler that fetches unread news for instructors and handles dismiss action via POST |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a news-alert feature: a cron job fetches and caches RSS feed items, new DB tables track cached items and user read timestamps, server models and UI expose unread alerts to instructors with a dismiss action, configuration/schema updates, tests, and an rss-parser dependency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/prairielearn/src/lib/db-types.ts (1)
1559-1661: Add new table names to theTableNamesarray.The new tables
cached_news_itemsanduser_news_read_timestampsshould be added to theTableNamesarray for consistency with other tables that have corresponding Zod schemas. Based on learnings, all database tables should have corresponding entries here.Proposed fix
export const TableNames = [ 'access_logs', 'access_tokens', 'administrators', 'ai_grading_jobs', 'ai_question_generation_prompts', 'alternative_groups', 'assessment_access_rules', 'assessment_instances', 'assessment_modules', 'assessment_question_role_permissions', 'assessment_questions', 'assessment_score_logs', 'assessment_sets', 'assessment_state_logs', 'assessments', 'audit_events', 'audit_logs', 'authn_providers', 'authors', 'batched_migration_jobs', 'batched_migrations', + 'cached_news_items', 'chunks', 'client_fingerprints', ... 'users', + 'user_news_read_timestamps', 'variant_view_logs', ...
🤖 Fix all issues with AI agents
In `@apps/prairielearn/src/lib/news-feed.ts`:
- Around line 14-22: hasMatchingCategory currently returns false for items with
no categories even when allowedCategories is empty; move or adjust the check so
that when allowedCategories.length === 0 the function returns true
unconditionally (i.e., allow all, including uncategorized items). Update the
logic in hasMatchingCategory to check allowedCategories.length === 0
before/independent of the categories existence check, and only perform the
categories empty check when filtering is enabled; keep using the
normalizedAllowed Set and the categories.some(...) match logic for the filtered
case.
- Line 1: hasMatchingCategory currently filters out items without categories
before honoring an empty-allowedCategories rule; change the logic in
hasMatchingCategory so it first returns true if allowedCategories is empty, then
returns false if item.categories is missing, and otherwise checks for any
intersection between item.categories and allowedCategories; also add
"rss-parser" to the apps/prairielearn package.json dependencies (or
workspace-specific deps) so the import Parser from 'rss-parser' is declared.
🧹 Nitpick comments (2)
apps/prairielearn/src/tests/e2e/newsAlert.e2e.spec.ts (1)
47-71: Consider test isolation for dismiss behavior.The test correctly inserts a new news item to avoid dependency on the first test's state. However, if the first test dismisses the alert (it doesn't currently, but could in future refactoring), the
beforeAllitem would already be dismissed for the second test.For better isolation, consider using
test.describe.serial()explicitly or ensuring each test is fully independent by cleaning up read timestamps.apps/prairielearn/src/models/news-items.ts (1)
51-57: Consider batching/parallelizing cached item upserts.Sequential awaits can add noticeable latency if the feed grows. A simple
Promise.allis a low-effort improvement.Possible refactor
export async function upsertCachedNewsItems(items: NewsItemInput[]): Promise<CachedNewsItem[]> { - const results: CachedNewsItem[] = []; - for (const item of items) { - const result = await upsertCachedNewsItem(item); - results.push(result); - } - return results; + return await Promise.all(items.map((item) => upsertCachedNewsItem(item))); }
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/prairielearn/src/tests/e2e/newsAlert.spec.ts`:
- Around line 40-52: The 'can dismiss the news alert' test mutates persistent
state and can race with 'shows news alert to instructors with unread items'; fix
by isolating state: either wrap both tests in a Playwright test.describe.serial
block to enforce order (use test.describe.serial(...) and keep the existing test
names 'can dismiss the news alert' and 'shows news alert to instructors with
unread items'), or add per-test setup/teardown that ensures a fresh news item
and clears dismissals — implement a test.beforeEach that calls your test helper
(e.g., insertNewsItem or API endpoint) to create a fresh news alert and/or a
test.afterEach that resets the dismissal state via the same helper (or call a
resetDismissal API) so 'can dismiss the news alert' no longer affects other
tests.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| title: 'Test News Item for E2E', | ||
| link: 'https://example.com/news/test-e2e-item', | ||
| pub_date: new Date(), | ||
| guid: `test-news-item-${Date.now()}`, |
There was a problem hiding this comment.
style: Using timestamp in GUID could cause test flakiness in parallel execution
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/prairielearn/src/tests/e2e/newsAlert.spec.ts
Line: 22:22
Comment:
**style:** Using timestamp in GUID could cause test flakiness in parallel execution
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Say more? how would it do that
There was a problem hiding this comment.
The issue is that Date.now() returns a timestamp in milliseconds. When tests run in parallel, there's a possibility that multiple tests could execute within the same millisecond, causing GUID collisions. Here's how this could cause flakiness:
- Parallel execution timing: If two test workers start at nearly the same time,
Date.now()might return the same value - Database conflicts: If the GUID is a unique key or used for deduplication, duplicate GUIDs could cause database errors or unexpected behavior
- Test isolation issues: One test's data might interfere with another's expectations
The solution is to use a more unique identifier. Here's the fix:
| guid: `test-news-item-${Date.now()}`, | |
| guid: `test-news-item-${Date.now()}-${Math.random().toString(36).substring(2)}`, |
This combines:
Date.now()for temporal uniquenessMath.random().toString(36).substring(2)for additional randomness to prevent collisions even within the same millisecond
Alternatively, you could use crypto.randomUUID() if available:
| guid: `test-news-item-${Date.now()}`, | |
| guid: `test-news-item-${crypto.randomUUID()}`, |
There was a problem hiding this comment.
huh??
If the GUID is a unique key or used for deduplication, duplicate GUIDs could cause database errors or unexpected behavior
Is that true? I don't think so
reteps
added
do not merge
Description
This PR adds a dismissible alert on the PrairieLearn homepage that displays up to 3 most recent unread blog posts from the RSS feed. The feature is intended for instructors only - students do not see the alert.
Requirements:
Key changes:
cached_blog_postsanduser_blog_read_timestampsfor tracking read statusTesting
To test locally:
After dismissing, the alert should not re-appear.