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
Bundle sizes — No significant size changes
Total: 6.15 MB → 6.15 MB (+407 B (+0.01%)) |
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughIntroduces a news alert feature that periodically fetches RSS feed items via a new cron job, caches them in the database, and displays unread news items to instructors on the home page with the ability to dismiss alerts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/prairielearn/src/pages/home/home.tsx`:
- Around line 18-22: Remove the leftover Git conflict markers (<<<<<<< HEAD,
=======, >>>>>>> master) and reconcile the duplicated imports so the module
imports are valid TypeScript; specifically ensure there's a single import for
assertNever and keep the correct import for typedAsyncHandler (or remove it if
unused) in the top of home.tsx (refer to symbols typedAsyncHandler and
assertNever) so only one assertNever import remains and no conflict markers are
left.
nwalters512
left a comment
nwalters512
left a comment
There was a problem hiding this comment.
Overall looking good! Biggest outstanding questions:
- Should we merge the legacy news items removal so that we can use the
news_itemstable for this functionality? - Can/should we improve the design?
apps/prairielearn/src/lib/config.ts
Outdated
| cronIntervalWorkspaceHostTransitionsSec: z.number().default(10), | ||
| cronIntervalChunksHostAutoScalingSec: z.number().default(10), | ||
| cronIntervalCleanTimeSeriesSec: z.number().default(10 * 60), | ||
| cronIntervalFetchNewsItemsSec: z.number().default(60 * 60 * 3), |
There was a problem hiding this comment.
I feel like we could do this much more often - maybe every 5 minutes?
It might be nice to give global admins a button to click to perform this sync in case something goes wrong, like we need to correct a title or something.
There was a problem hiding this comment.
Already addressed — the cron interval defaults to 5 minutes (cronIntervalFetchNewsItemsSec: z.number().default(5 * 60)), the cron job is gated on config.newsFeedUrl, and there's a "Sync news feed" button on the admin settings page.
🤖 Comment by Claude Code
apps/prairielearn/src/cron/index.ts
Outdated
| { | ||
| name: 'fetchNewsItems', | ||
| module: await import('./fetchNewsItems.js'), | ||
| intervalSec: config.cronOverrideAllIntervalsSec || config.cronIntervalFetchNewsItemsSec, | ||
| }, |
There was a problem hiding this comment.
Consider making this conditional on the presence of config.newsFeedUrl - no need to run if it's not set.
There was a problem hiding this comment.
Already addressed — cron job is conditional on config.newsFeedUrl (line 119 of cron/index.ts).
🤖 Comment by Claude Code
| const body = BodySchema.parse(req.body); | ||
|
|
||
| if (body.__action === 'dismiss_news_alert') { | ||
| await markNewsItemsAsReadForUser(authn_user_id); |
There was a problem hiding this comment.
Nit, consider having this take an entire User object.
There was a problem hiding this comment.
Updated to accept User instead of Pick<User, 'id'> in a5fd221.\n\n🤖 Comment by Claude Code
There was a problem hiding this comment.
Nit: conventionally model files/function match the name of the table, which in this case is actually cached_news_items.
Presumably the only reason we're not actually using news_items is because that's already taken by the legacy implementation? Should we merge #13747 before this PR now that we have a concrete plan to replace it?
There was a problem hiding this comment.
The table is now named news_items (the legacy tables were dropped in #14049 which has been merged).
🤖 Comment by Claude Code
| 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; | ||
| } |
There was a problem hiding this comment.
Say we accidentally publish something and want to remove it - this doesn't handle that case, right?
Note saying we need to, I think that'll be rare. If it does happen, I guess we can always fix it in the database manually? I think that's acceptable, just want to make sure this is considered.
There was a problem hiding this comment.
The admin settings page has a "Hide" button per news item (sets hidden_at), which removes it from the user-facing alert. For the rare accidental publish case, this should be sufficient — admins can hide the item immediately.
🤖 Comment by Claude Code
| logger.verbose('news-feed: Cached news items', { count: newsItems.length }); | ||
| } | ||
| } catch (err) { | ||
| logger.error('news-feed: Error fetching or parsing RSS feed', { feedUrl, err }); |
There was a problem hiding this comment.
Nit, a given installation will only ever have one feed URL, so IMO no need to log it - there will never be any ambiguity as to which feed we're trying to fetch. Ditto above.
There was a problem hiding this comment.
Already addressed — the feed URL is no longer logged. Line 33 just logs 'news-feed: Fetching RSS feed' without the URL.\n\n🤖 Comment by Claude Code
| logger.verbose('news-feed: Cached news items', { count: newsItems.length }); | ||
| } | ||
| } catch (err) { | ||
| logger.error('news-feed: Error fetching or parsing RSS feed', { feedUrl, err }); |
There was a problem hiding this comment.
Consider reporting errors here to Sentry for visibility.
There was a problem hiding this comment.
Already addressed — errors are reported to Sentry at line 74: Sentry.captureException(err).
🤖 Comment by Claude Code
There was a problem hiding this comment.
I'd feel much more comfortable kicking this into another PR for review discussion. I don't understand why this is here and I've never seen these errors.
If you want a specific question (again, I think we should do this in another PR), I would expect this to have to be in the subprocess where the PL server is actually running, not the parent. Why is this here?
There was a problem hiding this comment.
The serverUtils.ts changes have been reverted — the file now matches master with no diff.
🤖 Comment by Claude Code
There was a problem hiding this comment.
I think we can zhuzh up the design a bit. I've been throwing prompts to v0 as a way to get something to discuss as a starting point. Here's one such generation:
Things I like about this:
- Includes dates
- No bullets (IMO they look too plain for something like this)
- "View all posts" link
- Less "blue" - at a minimum, I'd like to avoid blue link text on top of a blue alert
I also think we should consider constraining the width a bit - 100% width leaves us with a ton of empty space:
I'm happy to take a crack at making some improvements if you want, let me know what you think of any of these directions.
There was a problem hiding this comment.
Updated design includes: dates per item, no bullets, "View all posts" link, muted blue accent (left border only, no blue background), and constrained width (maxWidth: 720px).
🤖 Comment by Claude Code
- Reduce cron interval from 3h to 5min for faster news updates - Only register fetchNewsItems cron job when newsFeedUrl is configured - Accept User objects instead of bare strings in news model functions - Remove feed URL from log messages (only one per installation) - Add Sentry error reporting for RSS fetch failures - Add "Sync news feed" button to administrator settings page - Redesign NewsAlert as a card with blue accent border, structured item layout with dates, and optional "View all posts" link - Revert unrelated serverUtils.ts changes - Update E2E test selectors for new component structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # apps/prairielearn/package.json # yarn.lock
Add hidden_at column to news_items table for soft-deleting news items. Filter hidden items from user-facing queries and add an admin UI table on the administrator settings page to view and hide news items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bundle sizes — No significant size changes
Total: 6.18 MB → 6.18 MB (+407 B (+0.01%)) |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rts on newsFeedUrl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
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.