Detect a feed card's language on the server instead of in every browser - #1618
Conversation
Code Review by Qodo
1. Unsafe description trim
|
|
Warning Review limit reached
Next review available in: 18 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
PR Summary by QodoMove feed-card language detection to server-side slim rows
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 855d742f3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const lang = item.slim?.lang; | ||
| const merged = { ...item, ...updated }; | ||
| if (lang === undefined || !updated.slim || updated.slim.lang !== undefined) return merged; | ||
| return { ...merged, slim: { ...updated.slim, lang } }; |
There was a problem hiding this comment.
When the trending/hot/created poll refreshes a cross-post, the shallow spread replaces the cached original_entry with the browser-fetched copy, but this helper preserves only the outer row's slim.lang. Because feed cards unwrap original_entry and pass that nested entry to the translation gate, a cross-post whose chip has not mounted before the poll loses its server hint and falls back to rendering the summary and loading franc-min. Preserve hints recursively for original_entry as well as at the top level.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken: mergePreservingHint now recurses into original_entry, so a cross-post's nested original keeps its hint when the poll replaces it; covered by the merge spec.
855d742 to
34bb849
Compare
Code Review by Qodo
1. Unsafe description trim
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const raw = entry.body | ||
| ? "" // a full body is the post page's business; hints are for slim rows | ||
| : ((entry.json_metadata?.description as string | null | undefined) ?? "").trim(); | ||
| if (raw.length < MIN_DETECT_CHARS) return null; |
There was a problem hiding this comment.
2. Unsafe description trim 🐞 Bug ☼ Reliability
hintFor() calls .trim() on json_metadata.description without validating it’s a string, so a malformed metadata shape can throw and break annotateLanguageHints() (and thus the server queryFn). This contradicts the “Never throws” behavior promised for language hints and can cause SSR/prefetch failures for affected rows/pages.
Agent Prompt
### Issue description
`apps/web/src/core/entries/language-hint.ts` assumes `entry.json_metadata.description` is always a string and does `(... ?? "").trim()`. In real data, `json_metadata` fields are not trustworthy (other code already defends against this), and a non-string `description` will throw before the internal try/catch, breaking the server-side query.
### Issue Context
The module comment says hint annotation “Never throws”. This currently isn’t guaranteed because `.trim()` executes outside the guarded `try { ... }`.
### Fix Focus Areas
- apps/web/src/core/entries/language-hint.ts[57-71]
### Suggested change
- Read `description` as `unknown` and only trim/use it if `typeof description === "string"`.
- (Optional) Move the raw extraction inside the `try` block or add a small `try/catch` around it to uphold the “never throws” contract.
Example:
```ts
const desc = entry.json_metadata?.description;
const raw = entry.body
? ""
: (typeof desc === "string" ? desc.trim() : "");
```
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Taken: hintFor now only trims a string description (anything else counts as no text) and the whole hint path, including the per-row annotate, is inside try/catch, so a malformed row carries no hint instead of failing the query. Spec covers undefined, null, number, object and array descriptions.
The Translate chip on a card needs the content language. Every visitor's browser detected it: one markdown render per card plus the franc-min detector chunk (about 47 KB gzipped), on idle after every feed and post view, for the majority of readers whose language matches the content. The slim step already derives each card's summary on the server, so the server now detects the language from that same summary once per fetch and ships it as slim.lang (ISO-639-1, or null when the text is too short or the detector is unsure). The sample is rendered to plain text with the same bounds the client applied, so an author description that is only an image link or markup counts as no text rather than as a language. The client gate reads the hint, caches it under the summary key as before and skips both the render and the chunk. Rows the browser fetches itself (later pages of an infinite feed) carry no hint and keep the on-idle path, and the feed poll's merge preserves the hint on rows it refreshes; the post page keeps detecting on the full body. The detector is imported lazily inside the server branch only, so it never enters a client bundle, and a failing detector leaves the rows without a hint rather than failing the query. Closes #1597
34bb849 to
2badef4
Compare
1 participant
The Translate chip on a card needs the content language. Until now every visitor's browser detected it: one markdown render per card plus the franc-min detector chunk (about 47 KB gzipped), on idle after every feed and post view, for the majority of readers whose language matches the content.
The slim step already derives each card's summary on the server, so the server now detects the language from that same summary once per fetch and ships it as
slim.lang(ISO-639-1, ornullwhen the text is too short or the detector is unsure). The sample is rendered to plain text with the same bounds the client applied, so an author description that is only an image link or markup counts as no text rather than as a language. The client gate reads the hint, caches it under the summary key as before, and skips both the render and the chunk. Rows the browser fetches itself (later pages of an infinite feed) carry no hint and keep the on-idle path, the feed poll's merge preserves the hint on rows it refreshes, and the post page keeps detecting on the full body. The detector is imported lazily inside the server branch only, so it never enters a client bundle; a failing import leaves rows without a hint (and is retried on the next fetch) rather than failing the query.Measured on the production build served locally, against the current develop build on staging: the franc chunk is requested 0 times on
/trendingand/trending/spanish(1 time each before), the SSR payload carries the hints (35 on/trending), and the chips still render where languages differ (10 on/trending/spanishfor an English reader, same as before).Test plan
language-hint.spec.ts(node): Spanish/English summaries getes/en, short text getsnull, cross-post originals are covered, rows without the slim marker and non-array pages are untouched, a throwing detector never escapes, link/markup-only descriptions count as no text, and the poll merge keeps the hint.language-hint-client.spec.ts(jsdom): no-op in the browser.entry-translate-language-gate.spec.tsx: a hinted slim row decides without loading franc, anullhint offers nothing, a full body ignores a stale hint.tsc --noEmitandnext lintclean.Closes #1597