fix: sort feed by newest, remove relevance scoring#8
Conversation
Articles were sorting by relevance_score DESC which is null for most articles (Agent 3 assigns scores but ingestion path skips it), causing unpredictable ordering. Feed now sorts purely by published_at DESC. Removed: TrendingSection component, calculateTrendingScore(), score legend, relevance composite Firestore index. Added ingestion-complete event listener to auto-refresh feed after ingestion completes. Relevance_score field kept in Firestore and agents — only dashboard display and query ordering changed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary by QodoSort feed by newest, remove relevance scoring from dashboard
WalkthroughsDescription• Feed now sorts by published_at DESC instead of relevance score • Removed Trending section, score legend, and relevance badges • Added ingestion-complete event listener for auto-refresh • Removed stale Firestore composite index on relevance_score Diagramflowchart LR
A["Feed Query"] -->|"Changed from relevance_score DESC"| B["published_at DESC only"]
C["Article Card"] -->|"Removed relevance badge"| D["Time-ago badge"]
E["Trending Section"] -->|"Removed"| F["Simplified UI"]
G["Firestore Index"] -->|"Deleted composite"| H["Single-field index"]
I["Ingestion Complete"] -->|"Event listener"| J["Auto-refresh feed"]
File Changes1. dashboard/src/pages/Articles.tsx
|
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the user experience on the dashboard by prioritizing recency in the article feed and streamlining the interface. It addresses issues with inconsistent article ordering by simplifying the sorting mechanism and removes less utilized or problematic UI elements related to relevance scoring. Additionally, it improves data freshness by enabling automatic feed updates post-ingestion and cleans up the database by removing an obsolete index. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemove trending score feature and replace with time-based ordering. Frontend updates remove trending_score field, TrendingSection component, and calculateTrendingScore function, replacing them with time-ago indicators and event-listener-based refresh. Firestore indexes are updated to remove the relevance_score index, and tests are updated to verify newest-first ordering instead of relevance-based queries. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Dashboard)
participant Firestore as Firestore
participant EventSystem as Event System
rect rgba(100, 150, 200, 0.5)
Note over Client,EventSystem: Old Flow - Pre-load all articles
Client->>Firestore: Fetch all articles
Firestore-->>Client: Return articles with relevance_score
Client->>Client: Calculate trending scores
Client->>Client: Render with trending badges
end
rect rgba(150, 200, 100, 0.5)
Note over Client,EventSystem: New Flow - Event-driven refresh
EventSystem->>Client: Emit ingestion-complete event
Client->>Firestore: Fetch articles by category<br/>(orderBy: published_at DESC)
Firestore-->>Client: Return articles
Client->>Client: Render with time-ago indicator
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the article feed to sort by publication date instead of the deprecated relevance score. The changes are well-executed across the frontend component, Firestore index configuration, and integration tests. The removal of the trending score logic and related UI elements simplifies the codebase significantly. I've added one suggestion to improve the maintainability of the new auto-refresh mechanism.
| useEffect(() => { | ||
| const fetchAllArticles = async () => { | ||
| try { | ||
| const articlesRef = collection(db, 'articles') | ||
| const q = query( | ||
| articlesRef, | ||
| orderBy('relevance_score', 'desc'), | ||
| orderBy('published_at', 'desc'), | ||
| limit(200) | ||
| ) | ||
| const snapshot = await getDocs(q) | ||
| const fetched: Article[] = [] | ||
| snapshot.forEach((doc) => { | ||
| fetched.push({ id: doc.id, ...doc.data() } as Article) | ||
| }) | ||
| setAllArticles(fetched) | ||
| } catch (err) { | ||
| console.error('Error fetching all articles:', err) | ||
| } | ||
| } | ||
| fetchAllArticles() | ||
| const handler = () => setRefreshKey(k => k + 1) | ||
| window.addEventListener('ingestion-complete', handler) | ||
| return () => window.removeEventListener('ingestion-complete', handler) | ||
| }, []) |
There was a problem hiding this comment.
Using a string literal for the event name 'ingestion-complete' is brittle because it's duplicated in IngestionButton.tsx. This could lead to silent failures if the string is changed in one file but not the other.
To improve maintainability and prevent potential bugs, I recommend defining this event name as a shared constant. For example, you could create a file like src/lib/events.ts:
// src/lib/events.ts
export const INGESTION_COMPLETE_EVENT = 'ingestion-complete';Then, import and use this constant in both Articles.tsx and IngestionButton.tsx.
While window events are simple for this use case, for future enhancements, consider using a more structured approach for cross-component communication, such as React Context, to make dependencies more explicit.
|
| Filename | Overview |
|---|---|
| dashboard/src/pages/Articles.tsx | Removed Trending section, relevance scoring UI, and dual-fetch pattern. Simplified to single published_at DESC query. Added ingestion-complete event listener for auto-refresh. Minor issue: time-ago is displayed twice per card. |
| firestore.indexes.json | Removed (relevance_score DESC, published_at DESC) composite index and fixed trailing comma. However, test_firestore_indexes.py:test_articles_relevance_published_index still asserts this index exists — test will fail. |
| tests/firebase/test_dashboard_queries.py | Renamed test and updated query to match new published_at DESC ordering. Error message correctly notes single-field orderBy shouldn't need composite index. |
Sequence Diagram
sequenceDiagram
participant User
participant Articles as Articles.tsx
participant Firestore
participant IngBtn as IngestionButton
participant MCP as MCP Service
User->>Articles: Load feed
Articles->>Firestore: query(articles, orderBy published_at DESC, limit 100)
Firestore-->>Articles: Article documents
User->>IngBtn: Click "Run Ingestion"
IngBtn->>MCP: POST /trigger/ingestion
MCP-->>IngBtn: 202 Accepted (run_id)
loop Poll status
IngBtn->>MCP: GET /trigger/ingestion/{run_id}
MCP-->>IngBtn: {phase, stats, status}
end
IngBtn->>IngBtn: dispatchEvent('ingestion-complete')
IngBtn-->>Articles: CustomEvent('ingestion-complete')
Articles->>Firestore: Re-fetch articles (refreshKey incremented)
Firestore-->>Articles: Updated article documents
Last reviewed commit: 1d02f30
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| "indexes": [ |
There was a problem hiding this comment.
Existing index test will break
Removing the (relevance_score DESC, published_at DESC) composite index here is correct for the new query, but tests/firebase/test_firestore_indexes.py:112 — test_articles_relevance_published_index — still asserts this index must exist in the file. That test will fail after this change.
Either remove or update the test in test_firestore_indexes.py to match the new index set.
| {/* Time indicator */} | ||
| <div className="w-auto px-2 h-8 rounded-lg flex items-center justify-center text-xs font-medium shrink-0 bg-muted text-muted-foreground tabular-nums"> | ||
| {formatTimeAgo(article.published_at)} | ||
| </div> |
There was a problem hiding this comment.
Duplicate time-ago display
formatTimeAgo(article.published_at) is now rendered twice per card: once here in the new time indicator badge, and again on line 116 in the source metadata row. Previously the badge showed the relevance score, so they were distinct. Now both positions display the same value, which is visually redundant.
Consider removing one of the two instances — either this badge or the inline text on line 116.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Code Review by Qodo
1. formatTimeAgo() lacks date validation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| {/* Time indicator */} | ||
| <div className="w-auto px-2 h-8 rounded-lg flex items-center justify-center text-xs font-medium shrink-0 bg-muted text-muted-foreground tabular-nums"> | ||
| {formatTimeAgo(article.published_at)} | ||
| </div> |
There was a problem hiding this comment.
1. formattimeago() lacks date validation 📘 Rule violation ⛯ Reliability
The new time-ago badge calls formatTimeAgo(article.published_at) without handling missing/invalid dates, which can render Invalid Date or misleading values. This violates the requirement to explicitly handle null/empty/boundary cases for user-visible behavior.
Agent Prompt
## Issue description
The newly added time-ago badge calls `formatTimeAgo(article.published_at)` for every article, but `formatTimeAgo()` does not handle invalid/missing timestamps (e.g., `undefined`, empty string, unparsable strings) and can display `Invalid Date`, `NaN`, or negative time values.
## Issue Context
Articles are hydrated from Firestore via `doc.data()` and cast to `Article` without runtime validation, so `published_at` may be absent or malformed in some documents.
## Fix Focus Areas
- dashboard/src/pages/Articles.tsx[69-81]
- dashboard/src/pages/Articles.tsx[94-97]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| { | ||
| "indexes": [ | ||
| { | ||
| "collectionGroup": "articles", | ||
| "queryScope": "COLLECTION", | ||
| "fields": [ | ||
| { | ||
| "fieldPath": "relevance_score", | ||
| "order": "DESCENDING" | ||
| }, | ||
| { | ||
| "fieldPath": "published_at", | ||
| "order": "DESCENDING" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "collectionGroup": "articles", | ||
| "queryScope": "COLLECTION", |
There was a problem hiding this comment.
3. Ci fails: missing relevance index 🐞 Bug ⛯ Reliability
The PR removes the articles(relevance_score DESC, published_at DESC) composite index, but the repo still has a test that asserts this index must exist. This will fail CI even though the dashboard query no longer uses relevance_score.
Agent Prompt
## Issue description
The PR removed the Firestore composite index `articles(relevance_score DESC, published_at DESC)`, but the repo still has an index-validation test that asserts this index must exist. This mismatch will fail CI.
## Issue Context
Dashboard queries now sort by `published_at` only, so the composite index likely isn’t required anymore (unless another feature still uses it).
## Fix Focus Areas
- tests/firebase/test_firestore_indexes.py[112-137]
- firestore.indexes.json[1-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -1,19 +1,5 @@ | |||
| { | |||
| "indexes": [ | |||
| { | |||
There was a problem hiding this comment.
2. Firestore index docs outdated 📘 Rule violation ✧ Quality
This PR removes the articles(relevance_score DESC, published_at DESC) composite index but does not update STATUS.md and the Firestore schema/operator docs to reflect the new index expectations. This breaks the requirement to document noteworthy configuration/operational changes.
Agent Prompt
## Issue description
The Firestore index configuration changed (relevance-based composite index removed), but repository operational documentation was not updated. This makes operator guidance and schema/index expectations inaccurate.
## Issue Context
`firestore.indexes.json` is the source of truth for deployed composite indexes. `000-docs/001-AT-ARCH-firestore-schema.md` currently documents a relevance-based composite index and field names that no longer match the actual index configuration. `STATUS.md` also lacks a note about index deployment expectations.
## Fix Focus Areas
- firestore.indexes.json[1-47]
- 000-docs/001-AT-ARCH-firestore-schema.md[397-435]
- STATUS.md[1-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
published_at DESCinstead ofrelevance_score DESC, published_at DESC— most articles have null relevance scores (Agent 3 not run during ingestion), causing unpredictable orderingarticles: relevance_score DESC, published_at DESC)ingestion-completeevent listener so feed auto-refreshes after ingestion finishesrelevance_scorekept in Firestore and agent tools (no backend changes)Test plan
npx tsc --noEmitpasses cleanfirestore.indexes.jsonvalid JSON, stale index removedtest_dashboard_queries.pyupdated and syntax-validfirebase deploy --only firestore:indexes🤖 Generated with Claude Code
Summary by CodeRabbit