web-next: View and request article translations#280
Merged
Conversation
The legacy `web/` stack supports `/@user/year/slug/{lang}` (e.g. `/ko`)
to display the matching translation of an article. The new `web-next/`
stack was missing this route, so any URL with a trailing language
segment 404'd, breaking parity for articles that already have one or
more translations available.
Add a SolidStart route at
`web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsx` that:
- Restricts `[lang]` via `matchFilters` to a BCP-47-shaped regex so
sibling literal routes (`edit`, `ogimage`) keep precedence, then
runs the captured value through `normalizeLocale()` from
`@hackerspub/models/i18n` for canonicalisation; an unrecognised
locale renders `<HttpStatusCode code={404} />`.
- Defines its own `LangPageQuery` that requires `$language: Locale!`
and selects `articleByYearAndSlug.contents(language: $language)`
inline (so the page can inspect `language` and `originalLanguage`
for redirect/404 decisions) in addition to spreading the shared
`Slug_head` and `Slug_body` fragments with
`@arguments(language: $language)`.
- Returns 404 when no article matches or when no content row matches
the requested language; redirects to the canonical
`/@user/year/slug` URL when the requested language IS the article's
original (i.e. `content.originalLanguage == null`); and redirects
to the actual stored language when locale negotiation returns a
differently-tagged content (e.g. `/ko-KR` -> `/ko` when only `ko`
exists).
- Reuses `ArticleMetaHead` and `ArticleBody` exported from the shared
`index.tsx`, so the entire article rendering stays in one place.
- Uses `<Show when={data() != null}>` around `<Switch>/<Match>` so a
pending preloaded query does not flash a 404 status during the
initial render, and uses `createMemo` for derived values to keep
the conditional branches readable.
To make the shared rendering parameterisable, refactor the existing
`Slug_head` and `Slug_body` Relay fragments to accept an optional
`$language: Locale` argument and pass it to `contents(language:)`; the
unfiltered article-translation list previously read from `contents`
inside `Slug_languageSwitcher` is now selected under an
`allContents: contents` alias so it doesn't conflict (Relay rejects two
selections of the same field with different applied arguments). The
language switcher then receives the currently-displayed language and
its `originalLanguage` from its parent body component instead of
inferring "current" from `contents[0]`. `SlugPageQuery` (used by
`index.tsx`) gains the same nullable `$language` variable and passes
it through; the index loader always sends `language: null`, so the
existing route behaviour is preserved.
Out of scope for this change: SEO metadata gaps (`<link rel="canonical">`,
`og:url`, `og:locale:alternate`) which are missing on the index route too
and would benefit from a separate pass; and visibility for in-progress
translation rows, which is gated behind `Article.contents`'
`includeBeingTranslated` argument whose current resolver semantics
(`where: { beingTranslated: arg ?? false }`) only let callers fetch
*either* completed *or* in-progress rows. The legacy route can show a
"Translating…" placeholder for in-flight translations; matching that
behaviour requires fixing the resolver and is left for a follow-up.
Verified manually against the dev server with the existing
`@hpdev/2025/stop-writing-if-statements-for-your-cli-flags` article:
`/ko` renders the Korean translation, `/ko-KR` redirects to `/ko`,
`/KO` (uppercase) renders, `/en` (the original language) redirects
to the canonical URL, `/xx` (invalid locale) 404s, and the original
page and language switcher continue to work after the refactor.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The legacy `web/` article routes set a `<link rel="canonical">` and
`og:url` to the route's permalink — `/@user/year/slug` for the index
route and `/@user/year/slug/{lang}` for the `[lang]` route — and emit
one `og:locale:alternate` per other available translation alongside
the displayed `og:locale`. Web-next was missing all three: only
`og:locale` was set, and crawlers had no signal that the bare slug URL
and a `/@user/year/slug/{lang}` URL describe the same article in
different languages. This left both the index route and the new
`[lang]` route below parity with the legacy pages.
Add the missing tags to `ArticleMetaHead` in
`web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx`:
- Pull `Link` from `@solidjs/meta` and emit
`<Link rel="canonical" href={canonical} />` and
`<Meta property="og:url" content={canonical} />` whenever the
canonical URL is computable.
- Compute that canonical from `article.url` + an optional
`canonicalLanguage` prop. When the prop is omitted (the index
route), the canonical is the bare article URL. When the prop is
set (the `[lang]` route), append it to the URL's pathname after
trimming a trailing slash, so `https://example/@u/2025/slug` plus
`ko` becomes `https://example/@u/2025/slug/ko`. Wrap the
`new URL(...)` call in try/catch so a malformed `article.url`
silently skips the canonical/og:url tags rather than throwing
during render.
- Select `allContents: contents { language }` on the `Slug_head`
fragment. This is a separate field selection from
`contents(language: $language)` and Relay-safe because the
unfiltered call is aliased. `Slug_languageSwitcher` already
selects the same `allContents`, so the fields merge in the
response without an extra request.
- Emit `<Meta property="og:locale:alternate">` for each entry in
`allContents` whose `language` differs from the currently
displayed language, replacing `-` with `_` to match Facebook
OpenGraph conventions and the legacy stack.
- For consistency, also normalise the existing `og:locale` value
via `.replace("-", "_")`; previously it was being emitted as
`ko-KR` instead of `ko_KR`.
In `[lang].tsx`, pass `canonicalLanguage={content().language}` to
`ArticleMetaHead`. Because the route already redirects locale
mismatches to the stored language URL before rendering, this prop
always carries the same language code that appears in the address
bar at render time, so canonical and og:url stay in sync with the
visible URL.
Verified manually with `document.querySelector` checks on the dev
server against the existing
`@hpdev/2025/stop-writing-if-statements-for-your-cli-flags` article
(English original with a Korean translation):
- `/...slug` → canonical `.../slug`, og:locale `en`,
og:locale:alternate `[ko]`.
- `/...slug/ko` → canonical `.../slug/ko`, og:locale `ko`,
og:locale:alternate `[en]`.
- `/...slug/ko-KR` redirects to `/...slug/ko` and the rendered
canonical/og:url match the post-redirect URL.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
…slated:true
The GraphQL `Article.contents(includeBeingTranslated:)` argument was
intended to let callers opt into seeing in-progress translation
placeholders alongside completed translations. The select callback
emitted `where: { beingTranslated: args.includeBeingTranslated ?? false }`,
which filters the Drizzle relation to rows whose `beingTranslated`
exactly equals the argument value: `true` returned only in-progress
rows, `false` (the default) returned only completed ones — never both.
That makes the field useless for the upcoming `[lang]` route, which
needs to show the "Translating…" placeholder by reading the queued
row alongside whatever else exists for the article.
Drop the `where` entirely when `includeBeingTranslated` is true; keep
the `beingTranslated: false` filter when it's false (the default).
The GraphQL default for the argument is unchanged, so guests and any
existing caller that did not pass the argument observe the same
output as before. No call sites in the repo passed `true`, so this
is a behavior fix rather than a contract break.
Add a `graphql/post.more.test.ts` case that inserts one completed
(`en`) and one in-progress (`ko`) content row for the same article
and asserts both branches: `false` returns `[en]`, `true` returns
`[en, ko]`.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The legacy `web/routes/@[username]/[idOrYear]/[slug]/[lang].tsx`
route, when a logged-in viewer asks for `/{lang}` of an article whose
matching content row does not yet exist, calls the
`startArticleContentTranslation` model function inline and renders
the resulting `beingTranslated: true` placeholder. web-next is
about to grow the same flow on its own `[lang].tsx`, but it cannot
trigger that side-effect from the SolidStart route by itself; it
needs a Relay mutation it can invoke from the client.
Add `requestArticleTranslation` to the GraphQL schema following the
shape of the existing `updateArticle` mutation:
- `RequestArticleTranslationInput { articleId: ID!, targetLanguage: Locale! }`
- `RequestArticleTranslationPayload { article: Article! }`
- errors: `NotAuthenticatedError`, `InvalidInputError`, and a new
`LlmTranslationNotAllowedError { reason: LlmTranslationNotAllowedReason! }`
where the reason is an enum of `DISABLED` (the article's
`allowLlmTranslation` is false) or `SAME_LANGUAGE` (the requested
locale equals the article's original language, in which case
there is nothing to translate).
The resolver's authorization mirrors the legacy route exactly. Any
logged-in viewer may request a translation, but only of articles
they would otherwise be able to read: the post is loaded with its
`actor.followers/blockees/blockers` and `mentions` relations, then
`isPostVisibleTo(post, ctx.account.actor)` runs before any
translation-specific check. An invisible post returns
`InvalidInputError("articleId")` (the same opaque shape used for a
truly missing post), which avoids leaking the article's
`allowLlmTranslation` setting to viewers who shouldn't see the post
at all. Once visibility passes, the resolver verifies the article
allows LLM translation, that the original content row exists, that
the requested locale isn't the same as the original, and only then
calls `startArticleContentTranslation(ctx.fedCtx, ...)` and returns
the post.
Tests cover seven branches: guest rejection, missing article,
non-Article post, hidden article (visibility `direct`),
`allowLlmTranslation: false`, target equals original, and the happy
path. The happy-path test injects a `LanguageModel` whose
`doGenerate`/`doStream` return never-resolving promises so the
background `translate(...).catch(deleteRow)` cleanup branch in
`startArticleContentTranslation` does not race the assertions; the
test then asserts both the mutation response payload (the article
plus the new in-progress content row) and the database row's
`beingTranslated`/`originalLanguage`/`translationRequesterId`
columns.
`deno task test`: 377 passing (was 370 baseline; +7 new tests).
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The article fragments on the slug route hard-coded the default
behavior of `Article.contents(...)` (omit the in-progress filter
arg, which the resolver treats as `false` — i.e. exclude rows whose
`beingTranslated` is true). The next commit will need
`[lang].tsx`'s body view to opt into surfacing those rows so the
existing `ArticleTitle` "Translating…" placeholder activates for an
in-flight translation, but the route itself wires the value through
fragment arguments — so the fragments must accept the argument
first.
Add `includeBeingTranslated: { type: "Boolean", defaultValue: false }`
to `Slug_head` and `Slug_body`'s `@argumentDefinitions` and pass it
into their `contents(...)` selections. The default of `false`
preserves today's behavior on every existing call site (the
`SlugPageQuery` index route does not override it, so both fragments
emit `contents(language: $language, includeBeingTranslated: false)`
and consumers see the same rows they did before).
Relay rejects two selections of `contents` on the same parent with
mismatched applied arguments, so the inline page-level
`contents(language: $language) { language originalLanguage }`
selection on `LangPageQuery` — the one used by the redirect/404
detection path — also gains the explicit
`includeBeingTranslated: false`, matching the args the head and
body fragments now emit. The next commit will lift the body's
value to `true` and update the same call site to keep them aligned.
`deno task test`: 377 passing (no change vs prior commit).
Manually re-verified `/ko` for the existing translated article in
the dev server: still renders the Korean translation with no console
errors.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The fragments now accept `includeBeingTranslated`; lift the
`[lang]` route's `LangPageQuery` to pass `true` everywhere it
selects `Article.contents` — both as `@arguments` to the shared
`Slug_head` and `Slug_body` fragments and on the inline
page-level `contents(...)` selection used for the redirect/404
path.
The body fragment's response now includes any
`beingTranslated: true` row that matches the requested locale, and
the existing `ArticleTitle` component already swaps in
`<h1>{t\`Translating…\`}</h1>` and the `<Show
when={!content()?.beingTranslated && content()?.content}>` guard
hides the rendered body, so a viewer landing on `/lang` for an
in-flight translation now sees the placeholder header alongside
the language switcher instead of either a 404 or a blank body.
The page-level inline selection's switch from `false` to `true`
keeps the redirect/404 detection consistent with what the body
sees: an in-progress row for the requested locale is treated as a
present (placeholder) content rather than as missing, so the route
no longer 404s during a translation that's still running.
Index-route behavior is unchanged: `SlugPageQuery` does not pass
the new argument, both fragments fall back to `defaultValue: false`
in their `@argumentDefinitions`, and `contents(...)` still
excludes in-progress rows there as before.
Manually verified in the dev server by inserting a
`beingTranslated: true` placeholder row directly via psql for a
locale (`ja`) the article does not yet have — `/ja` now renders
the "Translating…" header, hides the body, and shows the language
switcher, matching the legacy `web/`'s `article.beingTranslated`
treatment. `/ko` (an existing completed translation) still
renders the translated body as before; `deno task test` is green
at 377/377.
Assisted-by: Claude Code:claude-opus-4-7
When the legacy `web/routes/@[username]/[idOrYear]/[slug]/[lang].tsx`
route is hit by a logged-in viewer for an article that allows LLM
translation but has no content row matching the requested locale, it
calls `startArticleContentTranslation` server-side and renders the
resulting placeholder. web-next now does the same client-side from
its `[lang].tsx`: when the conditions are met, the route mounts an
inline `<AutoRequestTranslation>` component that fires the new
`requestArticleTranslation` mutation on first effect, shows a
"Translating…" placeholder while the mutation is in flight, and lets
the existing `<Match when={content() != null}>` branch take over once
Relay normalizes the new `beingTranslated: true` row into the
`Slug_body` fragment.
Implementation notes:
* `LangPageQuery` now also fetches `articleByYearAndSlug.id`,
`.language`, `.allowLlmTranslation`, and `viewer.id` so the route
can decide whether the viewer is allowed to request a translation
(signed in, article opted in to LLM translation, requested locale
different from the original).
* The mutation document `LangPage_requestArticleTranslation_Mutation`
spreads `Slug_head` and `Slug_body` on the success payload's
`article` with the same `(language, includeBeingTranslated: true)`
arguments as the page query, so the mutation response refreshes
the same normalized fragment slot the body is already reading —
no manual `updater` needed.
* The auto-trigger component tracks the last-fired
`${articleId}/${language}` key in a closure variable rather than a
simple boolean, because Solid Router can reuse the route
instance across client-side parameter changes (e.g. switching
from a missing `/ja` to a missing `/zh-CN` without unmounting),
and we want each distinct request to fire exactly once.
* The mutation `Disposable` is captured and disposed on unmount and
on key change, so a callback for an in-flight request from a
previous mount cannot fire `setFailed` after the component has
already been replaced.
* Failure paths (`InvalidInputError`, `NotAuthenticatedError`,
`LlmTranslationNotAllowedError`, or transport error) raise a
destructive `showToast({ title: t\`Translation request failed\` })`
and flip the component to `<HttpStatusCode code={404} />`. The
toast copy is intentionally generic for v1; per-error messages
can come later.
Behavior preserved when the auto-trigger conditions are not met:
guest viewers, articles with `allowLlmTranslation: false`, and
requests for the article's original language all still hit the
existing `<Match when={content() == null}>` 404 fallback (unchanged
order: the `canRequestTranslation` Match comes first, then the bare
404, then the redirect Match for negotiated-locale and
original-language cases, then the body Match).
`deno task test`: 377/377 still passing. Manually verified `/ja`
as a guest still 404s without firing the mutation; the logged-in
trigger path will be exercised end-to-end once a session-bearing
account exists in the dev database — the mutation itself is covered
by the seven backend tests added in the previous commit.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
The legacy `web/routes/@[username]/[idOrYear]/[slug]/[lang].tsx` sets a `Refresh: 30` HTTP header on the response while the article content is in the `beingTranslated: true` state, so the user's browser reloads the whole page every 30 seconds until translation completes. web-next can do better: instead of a full page reload, periodically revalidate the same Solid Router cache key the route already loads, which lets Relay swap the new completed content row into the existing fragment slots without a layout flash. Add a `createEffect` inside `ArticleLangPageContent` that observes `content()?.beingTranslated`. While true, it sets a 30-second `setInterval` calling `revalidate(loadLangPageQuery.keyFor(handle, idOrYear, slug, language))` and registers an `onCleanup` to clear the interval. Because the effect tracks `content()` reactively, the cleanup fires automatically on the next run when `beingTranslated` flips to false (the translation finished and the field updated), and also on component unmount, so there's no runaway timer. The page-level inline `contents(...)` selection on `LangPageQuery` also gains `beingTranslated` so the polling effect can read it without resorting to an unrelated fragment; the body fragment selection (Slug_body) already had it. Manually verified by inserting a `beingTranslated: true` placeholder content row directly via psql for `/ja` on an existing article: the page renders the "Translating…" placeholder with no console errors, and the effect schedules the 30-second revalidate while the placeholder is shown. `deno task test` is green at 377/377; no backend changes in this commit. Assisted-by: Claude Code:claude-opus-4-7
The legacy article view, when the viewer is signed in and the
article has `allowLlmTranslation: true`, expands the "Other
languages →" navigation to include each of the viewer's preferred
locales (the `state.locales` array) as a clickable link, even when
no translation row exists for that locale yet. Clicking a link
takes the viewer to `/lang`, where the route then auto-fires the
translation mutation and renders the placeholder.
Bring the same behavior to web-next:
* `Slug_languageSwitcher` fragment now also selects `language`
(the article's original language) and `allowLlmTranslation`,
so the switcher can decide whether to render the extras and
can avoid a "translate to the original language" link.
* `ArticleLanguageSwitcher` accepts a new
`viewerLocales?: readonly string[] | null` prop. When the
article allows LLM translation and the viewer has any
preferred locales, each locale that's neither already in
`allContents`, nor the currently displayed language, nor the
article's original language is appended to the existing nav.
Each extra link uses the same classes as the existing ones
(`text-stone-900 dark:text-stone-100`, `hreflang`, `lang`,
`rel="alternate"`) and points at `${postUrl}/${locale}`.
* `SlugPageQuery` and `LangPageQuery` now select `viewer.locales`
at the page level (alongside the existing `id` and the
`Slug_viewer` spread) so both routes can read the viewer's
locales without forcing every consumer of `Slug_viewer` to
take a fragment ref. The route's render call passes
`viewerLocales={data().viewer?.locales}` down to
`ArticleBody`, which forwards it to the switcher.
* The aside's outer `<Show>` now renders when there are extra
locales to suggest even if `allContents.length <= 1`, so a
viewer with preferred locales can still see translation
suggestions for an article that only has the original.
DESIGN.md compliance: the new links are visually identical to the
existing ones (no new colors, no new icons, the same nav and aside
chrome). No styling changes to the surrounding container — the
hard-coded `border-stone-*`/`text-stone-*` classes there are
pre-existing tech debt to clean up separately.
Manually verified in the dev server: as a guest, the switcher
renders the existing single 한국어 link with no extras
(`viewerLocales == null` skips the new branch), matching the prior
behavior. `deno task test` is green at 377/377; no backend changes
in this commit.
Assisted-by: Claude Code:claude-opus-4-7
Run \`pnpm extract\` to refresh the gettext catalogs after the auto-trigger commit added the destructive-toast string in \`[lang].tsx\`, and fill in the \`msgstr\` for the four non-source locales using the conventions already established by neighboring "Failed to …" strings (e.g. "Failed to bookmark"): ja-JP: 翻訳のリクエストに失敗しました ko-KR: 번역 요청 실패 zh-CN: 翻译请求失败 zh-TW: 翻譯請求失敗 The extract pass also reorders some unrelated message references in the catalogs as line numbers shifted; those are mechanical updates from \`pnpm extract\`. After this commit \`pnpm extract\` reports 0 missing strings across all four target locales. \`deno task test\`: 377/377 still passing; no source changes. Assisted-by: Claude Code:claude-opus-4-7
Commit 47bc22a wired the 30-second translation-completion polling through `revalidate(loadLangPageQuery.keyFor(...))`. End-to-end testing exposed that this does not actually refresh the page: the Solid Router cache entry is invalidated and the next call to the loader is treated as a cache miss, but the loader returns the same already-populated `PreloadedQuery` whose Relay store entry has not been re-fetched, so `createPreloadedQuery` sees no change and the "翻訳中…" placeholder stays on screen indefinitely (verified manually against `/de`: backend marked the row `beingTranslated: false`, but the page only swapped to the German body after a manual reload). Replace the poll body with a direct `fetchQuery` from `relay-runtime`. `fetchQuery` always issues a network request and writes the response into the Relay store; `createPreloadedQuery` observes the store, so the page-level inline contents selection (now seeing `beingTranslated: false`) and the body fragment (now seeing the completed `title`/`content`/`toc`) re-render automatically without any cache-key plumbing. The polling effect's gating (`content()?.beingTranslated`) and `onCleanup` for the interval are unchanged, so the interval still stops on its own as soon as the translation finishes. Verified end-to-end manually: signed in as `hpdev`, navigated to a fresh `/de`; auto-trigger fired the mutation; "翻訳中…" placeholder rendered; about ~2 minutes later the polling round trip swapped the heading and body to the completed German translation ("Hören Sie auf, If-Statements für Ihre CLI-Flags zu schreiben (bearbeitet)") and the matching `<title>` without a manual reload. `deno task test`: 377/377 still passing. Assisted-by: Claude Code:claude-opus-4-7
The "translating…" placeholder previously rendered as a bare
`<h1>{t\`Translating…\`}</h1>`, which gave the user no visual cue
that anything was happening and no sense of how long they should
wait. Replace it with a centered card that explains the wait:
- a spinning Lucide `loader-2` icon (`size-8 animate-spin
opacity-60`) signalling activity;
- "Translating to {language}…" using the localized language name
via `Intl.DisplayNames(i18n.locale, { type: "language" })`,
falling back to the bare "Translating…" if the locale tag can't
be resolved;
- a muted-foreground sentence telling the viewer how long to
expect ("This usually takes about a minute. The page will
update automatically when the translation is ready.") so they
know the polling will swap in the body without further action.
Per `DESIGN.md`, the card uses the semantic `border` token (no
hard-coded stone shades) and `text-muted-foreground` for the
description; no filled background or shadow — just a hairline
bordered box, matching the achromatic + content-over-chrome
aesthetic.
Extract the placeholder into an exported `ArticleTranslationPlaceholder`
component in `index.tsx` and reuse it in two places:
- The new `<Show when={content()?.beingTranslated}>` branch in
`ArticleBody`, which now renders the placeholder below the
language switcher (replacing the old `ArticleTitle`
`beingTranslated` fallback path; `ArticleTitle` is now
title-only).
- The `AutoRequestTranslation` transient state in `[lang].tsx`,
which previously rendered a duplicate bare h1 — now rendering
the same spinner+copy block so there's no visual jump from the
"requesting" phase to the "translating" phase once the mutation
completes.
Add two new i18n strings — `Translating to {0}…` and the
explanation sentence — and fill in ja-JP / ko-KR / zh-CN / zh-TW
translations. Korean uses the explicit `{0}로 번역 중…` form (per
contributor preference) instead of dropping the particle.
`pnpm extract` reports 0 missing across all four target locales;
`deno task test` is green at 377/377; manually verified `/it` (a
fresh, untriggered language) — the auto-trigger still fires, the
new placeholder renders with the localized "イタリア語に翻訳中…"
heading, the description sentence, and a visible spinning icon, all
inside a neat bordered card under the language switcher.
Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds end-to-end article translation: GraphQL schema and Relay mutation to request LLM translations, backend mutation to enqueue translations and expose in-progress content rows, tests for query/mutation behavior, frontend route/components to auto-request and poll translations, and i18n strings for the translation UI. ChangesArticle Translation Flow
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant FrontEnd as Article Route [lang]
participant GraphQL as GraphQL API
participant DB as Database
participant Translator as LLM Service
Client->>FrontEnd: GET /article/[handle]/[slug]/[lang]
FrontEnd->>FrontEnd: Normalize locale, preload LangPageQuery
FrontEnd->>GraphQL: Query article & contents(includeBeingTranslated)
GraphQL->>DB: Fetch article + contents + viewer
DB-->>GraphQL: article data
GraphQL-->>FrontEnd: Preloaded data
FrontEnd->>FrontEnd: Determine missing/stale/script-mismatch
alt needs translation & allowed
FrontEnd->>GraphQL: requestArticleTranslation(articleId, targetLanguage)
GraphQL->>DB: Validate, insert queued in-progress content row
DB-->>GraphQL: queued row
GraphQL->>Translator: startArticleContentTranslation(job)
Translator-->>GraphQL: job started/queued
loop poll every 30s while beingTranslated=true
FrontEnd->>GraphQL: fetchQuery (poll)
GraphQL->>DB: Fetch content status
DB-->>GraphQL: content (maybe completed)
GraphQL-->>FrontEnd: Updated content
end
Translator->>DB: Update content row (beingTranslated=false, content)
FrontEnd->>Client: Render translated article
else not needed / already present
FrontEnd->>Client: Render article
else not allowed / error
FrontEnd->>Client: Render error UI / toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 19 minutes and 10 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements LLM-powered article translations, including a new GraphQL mutation, schema updates for tracking translation status, and a dedicated frontend route for language-specific views. The UI is enhanced with a language switcher that suggests translations based on viewer preferences and a polling mechanism to update the page once a translation is ready. Feedback includes a potential type mismatch in the GraphQL resolver regarding the Locale scalar and a recommendation to add error handling to the background polling logic.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-next/src/routes/`(root)/[handle]/[idOrYear]/[slug]/[lang].tsx:
- Around line 200-210: The polling currently calls fetchQuery(...).subscribe({})
each tick but only clears the setInterval; update the createEffect so you
capture the subscription returned by fetchQuery(...).subscribe(...) into a
variable (e.g., currentSub), unsubscribe the previous subscription before
starting a new fetch to avoid overlapping requests, and also unsubscribe
currentSub inside onCleanup; reference the createEffect callback,
fetchQuery/LangPageQuery call, the subscribe() result, and onCleanup to locate
where to store and dispose the subscription.
In `@web-next/src/routes/`(root)/[handle]/[idOrYear]/[slug]/index.tsx:
- Around line 633-648: extraLocales() currently compares raw viewer locales to
stored article locales, so variants like "zh-TW" slip through; fix by
normalizing locales before deduping: create a small normalizer (e.g.,
normalizeLocale = (s) => s?.split(/[-_]/)[0].toLowerCase()) and use it when
populating existing (from article().allContents.map(c=>c.language)), when
checking article().language and props.currentLanguage, and when tracking seen;
keep returning the original locale strings from props.viewerLocales but use
their normalized form for all comparisons and set membership to avoid offering
duplicates like "zh-TW" when "zh" already exists.
🪄 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: CHILL
Plan: Pro
Run ID: 52ab50a2-2b7c-4b52-830b-06c96c975e1c
📒 Files selected for processing (10)
graphql/post.more.test.tsgraphql/post.tsgraphql/schema.graphqlweb-next/src/locales/en-US/messages.poweb-next/src/locales/ja-JP/messages.poweb-next/src/locales/ko-KR/messages.poweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.poweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsxweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx
![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: 66f0025d76
ℹ️ 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".
Three things in this commit, all about the same polling/auto-request
loop in `[lang].tsx`:
1. Capture the `fetchQuery` subscription each tick and dispose it
in `onCleanup` (and at the start of the next tick), so a route
unmount mid-request doesn't leak a subscription that can write
stale data into the Relay store after the component is gone,
and overlapping requests don't pile up if a fetch happens to
outlast its 30-second window.
2. Attach an `error` handler to the polling subscription so
transient network failures land in the console instead of
vanishing. No toast — the placeholder stays put and the next
tick retries on its own — but the failure is at least
discoverable when something does go wrong.
3. Re-fire `requestArticleTranslation` when the in-progress row
is older than 30 minutes. The model layer's
`startArticleContentTranslation` already has a 30-minute
staleness window for re-queueing a placeholder whose worker
has died, but the route only auto-fired on `content() == null`,
so a stuck row meant the polling loop would refresh the same
unchanging timestamps forever. Add an `isStale` memo, fold it
into the existing `<Match>` for the auto-request branch, and
surface the new `updated` field on the inline `contents`
selection so the memo has a timestamp to compare against. The
30-minute constant lives at the top of the file with a comment
pointing at its `models/article.ts` counterpart.
The auto-request branch's Match condition is now driven by a single
`shouldAutoRequest()` memo (`canRequestTranslation()` &&
(`content() == null || isStaleInProgress()`)) rather than a literal
condition repeated across two branches, so the stale-retry path and
the "no content yet" path stay in sync.
`deno task test`: 377 passing, no test changes needed.
hackers-pub#280 (comment)
hackers-pub#280 (comment)
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
…btag The "extra" links the article language switcher offers when the viewer is signed in and the article allows LLM translation were deduplicated by exact-string match against the article's existing translations and original language. That let a viewer locale of `zh-TW` slip through even when the article already had a `zh` content row, because the strings differ; clicking the offered "中文（台灣）" link would then round-trip through `[lang].tsx` and redirect to `/zh` instead of representing a real new translation target. Compare on the BCP 47 *language* subtag instead, via `new Intl.Locale(...).language`, both for the existing-content set and for tracking which viewer locales have already been suggested. The link text and href still use the viewer's full locale tag so the user sees their preferred display name. No tests for the switcher (no test harness in `web-next/`); spot- checked manually as a logged-in account whose `Account.locales` includes both `zh-CN` and `zh-TW` against an article that has neither — only one Chinese link surfaces now. `deno task test` unchanged at 377/377. hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3edafbe5de
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a system for requesting and viewing article translations via LLMs, including a new GraphQL mutation, schema updates for translation errors, and a dedicated frontend route that handles automatic requests and status polling. Feedback suggests correcting the import order to align with the repository's style guide and adding console logging for failed translation requests to improve debuggability.
The `Locale` GraphQL scalar accepts any well-formed BCP 47 tag,
because it parses through `new Intl.Locale(...)`. But the
`web-next` `[lang]` route only serves locales that pass
`normalizeLocale` from `@hackerspub/models/i18n` (the same
`POSSIBLE_LOCALES` whitelist used everywhere else in the project),
so an API client could enqueue a translation for a tag like `ka-GE`
that the canonical article URL flow can never display.
Run the input through the same `normalizeLocale` check before
queueing. An unrecognised tag now returns
`InvalidInputError("targetLanguage")` instead of silently inserting
a placeholder row that no `/lang` URL can render. The normalized
form is also what gets passed into `startArticleContentTranslation`,
so the row's `language` column always matches the canonical URL the
viewer would land on.
`graphql/post.more.test.ts` gets one new case asserting the new
error path with `ka-GE` (valid BCP 47, missing from
`POSSIBLE_LOCALES`).
`deno task test`: 378 passing (was 377; +1 new test).
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
Three things in this commit, all in the `/lang` route's
auto-translation flow:
1. Lift the auto-request mutation out of the inline
`<AutoRequestTranslation>` child and into the parent
`ArticleLangPageContent` component. The child only existed to
run a `createMutation` once-per-mount, but it short-circuited
in the case Codex flagged: if
`startArticleContentTranslation`'s background failure-cleanup
branch deletes the placeholder row a few seconds after we
queue it, `content()` flips back to null while
`shouldAutoRequest` stays true. The Switch had already
re-mounted the child once, the child's `firedKey` guard had
already recorded the (articleId, language) pair, and the
subsequent `content == null` re-render didn't drive a fresh
mutation. The user got stuck on the placeholder until a
manual reload.
Replace the firedKey with a `requestKey()` memo whose identity
changes whenever the underlying state genuinely calls for a
new request: `missing/{articleId}/{language}/{missingEpoch}`
for the row-not-there branch, where `missingEpoch` is bumped
by a separate effect each time `content()` transitions from
existing to null, and `stale/{articleId}/{language}/{updated}`
for the older-than-30-min branch. A `firedRequestKey` closure
variable still suppresses spurious re-fires for the same key.
The placeholder JSX moves into the parent's Switch as a normal
`<Match>` arm; failure (typed-error payload or network error)
flips `requestFailed` and an earlier `<Match>` renders the
existing 404 fallback.
2. Log the mutation's failing payload (or thrown error) to the
console alongside the destructive toast. The toast tells the
user something went wrong; the console tells the developer
what. Same one-line `console.error(...)` pattern as the
polling effect.
3. Consolidate the three separate `relay-runtime` import
statements (one for values, two for individual types) into one
line that uses inline `type` keywords, matching the convention
in the rest of the codebase. Sort
`__generated__/LangPageQuery` before
`__generated__/LangPage_request*` so the alphabetical order
follows ASCII (`Q` < `_`), matching the style guide and the
ordering ESLint-style importers produce.
End-to-end check on a fresh `/es` (no Spanish translation existed):
auto-request fires, the row appears as `beingTranslated: true`,
the placeholder renders, and the polling swap runs as before; no
console errors. `deno task test` is green at 378/378 (the one
extra test was added in the previous commit for the locale
allow-list check).
hackers-pub#280 (comment)
hackers-pub#280 (comment)
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces LLM-powered article translations, adding a requestArticleTranslation mutation, a new language-specific route ([lang].tsx) with polling capabilities, and updated UI components for language switching. A critical issue was identified in the Article.contents resolver, which currently ignores the language argument; this must be fixed to ensure the frontend receives the correct translation and to prevent potential redirect loops.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fd01e81b9
ℹ️ 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".
The article language switcher's `extraLocales()` builder appended viewer-locale links verbatim, but the `[lang]` route's `matchFilters` and the new `requestArticleTranslation` mutation both gate on `normalizeLocale` (the project-wide `POSSIBLE_LOCALES` allow-list). A viewer whose `Account.locales` included a valid BCP 47 tag outside that list (e.g. `fr-CH`, `ka-GE`) would see a clickable link that deterministically lands on 404 and can't trigger a translation, making the new "request translation" path look broken. Run each viewer locale through `normalizeLocale` first; drop anything it rejects, and use the normalized form (which is what the mutation will end up calling `startArticleContentTranslation` with anyway) as both the link's href segment and the input to `Intl.DisplayNames`. Existing dedup logic against `article.language`, the currently displayed language, and `article.allContents` is unchanged. `deno task test`: 379 passing, no test changes (no test harness exists for the switcher). hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
The 30 s polling effect was calling `pending?.unsubscribe()` at the top of every tick before kicking off a fresh `fetchQuery`. Relay treats `unsubscribe()` as request cancellation, so a poll that takes longer than 30 seconds (slow network or slow upstream) would get aborted before its response could land in the store, and each subsequent tick would do the same; the placeholder could then sit on the page indefinitely even though the translation eventually finished server-side. Skip starting a new poll while a previous one is still in flight, and clear the `pending` reference from the subscription's `complete`/`error` callbacks instead. A genuinely stuck network just means we wait one tick longer; cancellation only happens on `onCleanup` (interval cleared, `beingTranslated` flipped, or component unmounted), which is the only place we actually want to abandon a request. `deno task test`: 379 passing, no test changes. hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements LLM-powered article translations, adding a requestArticleTranslation mutation and a new language-specific route that handles auto-requesting and polling. Feedback focuses on the translation logic, specifically that using language subtags for validation prevents requesting translations between supported regional variants like Simplified and Traditional Chinese. It was also recommended to update GraphQL fragments and SEO metadata to ensure Open Graph tags only reference completed translations instead of in-progress placeholders.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d48abf6f82
ℹ️ 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".
The previous SAME_LANGUAGE check rejected any target whose BCP 47 language subtag matched the source's, which is right for regional variants (`en` source rejecting `en-US`, because the negotiation inside `Article.contents` would round-trip the new placeholder back to the existing source content) but wrong for cross-script translations: `zh-CN` source against a `zh-TW` target was refused even though Simplified and Traditional Chinese genuinely produce different translation outputs and the negotiation result keeps each script in its own canonical URL slot. Compare on the maximized language *and* script subtags instead. `zh-CN` and `zh-TW` maximize to `zh-Hans-CN` and `zh-Hant-TW` (same language, different script): allowed. `en` and `en-US` both maximize to `en-Latn-US` (same language, same script): still blocked. `ko` against `ko-KR` (`ko-Kore-KR` either way): still blocked. Add a test for the cross-script allowed path (`zh-CN` -> `zh-TW`), and parameterize the existing test mutation document with `$language` so the response's `contents(language: ...)` selection can introspect rows queued in something other than Korean. The previously-added same-family rejection test (`en` -> `en-US`) is unchanged and still covers the language+script-equal case. `deno task test`: 380 passing (was 379; +1 new test). hackers-pub#280 (comment) hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
Two related fixes for how the article-translation flow handles
script differences (e.g., Simplified vs Traditional Chinese), to
match the just-relaxed `requestArticleTranslation` rule on the
backend:
1. `[lang].tsx`: when `Article.contents(language: ...)` negotiates
a request for `/zh-TW` against a `zh-CN`-only article and
hands back the `zh-CN` row, the route was redirecting to
`/zh-CN`, which silently swept the user's request away even
though `requestArticleTranslation` would now happily queue a
`zh-TW` translation. Add a `matchesLanguageScript` helper
(mirroring the backend rule, on `Intl.Locale.maximize()`'s
`language` + `script`), and use it to extend `canRequestTranslation`,
`shouldAutoRequest`, and `redirectHref`: when the negotiated
content is in a different script *and* the viewer can request
a translation, fall through to the auto-request branch instead
of redirecting. Guests (or viewers on articles with LLM
translation disabled) still get the existing redirect, since
they have no way to queue a fresh translation.
2. `Slug_languageSwitcher`: the `extraLocales()` builder was
deduping viewer locales against `article.language` and
`article.allContents` on the language subtag alone, so a viewer
whose `Account.locales` listed `zh-TW` saw no link on a
`zh-CN`-only article (the existing `zh-CN` row consumed the
`zh` slot). Switch the `subtag()` helper to return
`${max.language}-${max.script}` so `zh-CN` and `zh-TW` are
distinct entries; regional-only variants (`en-US` vs `en-GB`,
both maximizing to `en-Latn-*`) still collapse correctly.
`deno task test`: 380 passing, no test changes (no test harness
exists for the route or the switcher).
hackers-pub#280 (comment)
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
`Slug_head`'s `allContents` selection enumerates every content row on the article (including `beingTranslated: true` placeholder rows, because of the `includeBeingTranslated: true` argument introduced when the route started rendering them) and feeds the `og:locale:alternate` `<For>` directly. A crawler indexing the canonical article URL while a translation was in flight would see the in-progress locale advertised as an alternate translation it can fetch, but visiting `/lang` would only surface the "translating…" placeholder, not actual content; the listing was advertising something that didn't exist yet (and might never, if the background translator failed and the cleanup branch deleted the row). Add `beingTranslated` to the `allContents` selection in `Slug_head` and filter `c.beingTranslated` out of the `og:locale:alternate` `<For>`. The language switcher (`Slug_languageSwitcher`) keeps showing in-progress rows so a logged-in viewer sees the queued target locale and can revisit later. `deno task test`: 380 passing, no test changes (no test harness exists for the route). hackers-pub#280 (comment) hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a feature allowing users to request AI-powered translations for articles. Key changes include the requestArticleTranslation GraphQL mutation, a new language-specific route in the frontend that manages automatic translation queuing and polling, and updated UI components for language switching and translation status. Feedback identifies a performance risk in the backend mutation where full actor relationship lists are fetched for visibility checks, and a potential infinite loop in the frontend translation logic if requests consistently fail.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
graphql/post.more.test.ts (1)
1444-1494: ⚡ Quick winConsider adding a test for duplicate / idempotent translation requests.
The suite covers all validation branches but doesn't exercise the case where
requestArticleTranslationis called for a language that already has a row — either an in-progress (beingTranslated: true) placeholder or a finished translation. If the resolver doesn't defend against that path, a second call will attempt to insert a row that violates the unique constraint on(sourceId, language), surfacing as an unhandled error rather than a clean union-type response.Two cases worth adding:
- Re-request while
beingTranslated: trueis already present — should return the existing row (or a well-typed error), not a DB exception.- Re-request when a completed translation already exists — likely the same expected behaviour.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graphql/post.more.test.ts` around lines 1444 - 1494, Add idempotency tests and make the resolver defend against duplicate inserts: update tests to call requestArticleTranslationMutation twice (using requestArticleTranslationMutation and makeUserContextWithStubbedTranslator) for the same articleId and targetLanguage and assert the second response returns the existing translation row (or a well-typed GraphQL error) instead of bubbling a DB unique-constraint error; to fix code, modify the requestArticleTranslation resolver to check articleContentTable for an existing row by (sourceId, language) before inserting (handle both beingTranslated=true and completed cases), and if found return that row (or map to the proper union error) rather than attempting a second insert, ensuring translationRequesterId and beingTranslated behavior remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-next/src/routes/`(root)/[handle]/[idOrYear]/[slug]/index.tsx:
- Around line 828-840: The generated locale links use each entry from
article().allContents but set href to c.url directly, which can be null for
"being-translated" placeholders; update the mapping inside the For so the href
uses the same null-fallback used by sourceUrl() — e.g. use c?.url ?? postUrl()
(or equivalent) for the allContents branch, leaving the extraLocales() branch
unchanged, so placeholder rows get postUrl() instead of null/empty hrefs.
---
Nitpick comments:
In `@graphql/post.more.test.ts`:
- Around line 1444-1494: Add idempotency tests and make the resolver defend
against duplicate inserts: update tests to call
requestArticleTranslationMutation twice (using requestArticleTranslationMutation
and makeUserContextWithStubbedTranslator) for the same articleId and
targetLanguage and assert the second response returns the existing translation
row (or a well-typed GraphQL error) instead of bubbling a DB unique-constraint
error; to fix code, modify the requestArticleTranslation resolver to check
articleContentTable for an existing row by (sourceId, language) before inserting
(handle both beingTranslated=true and completed cases), and if found return that
row (or map to the proper union error) rather than attempting a second insert,
ensuring translationRequesterId and beingTranslated behavior remain correct.
🪄 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: CHILL
Plan: Pro
Run ID: 0692540e-ecb6-4f2d-865d-b42add0336e8
📒 Files selected for processing (4)
graphql/post.more.test.tsgraphql/post.tsweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsxweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- graphql/post.ts
- web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df4978fd66
ℹ️ 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".
The auto-request `createEffect` cleared `requestFailed` at the top of every run and didn't read the signal at all, so a stuck-backend loop could go around indefinitely: mutation succeeds with an empty `contents` payload (the cleanup branch deleted the placeholder before serialization), `onCompleted` flips `requestFailed` to true and surfaces the retry UI, then the row's transition from existing to null bumps `missingEpoch`, `requestKey` changes, the effect runs again, the top-of-effect `setRequestFailed(false)` hides the retry UI, the mutation re-fires, the same thing happens. In practice the user could see the placeholder forever while the server quietly piled up failed requests. Gate the early return on `requestFailed()` and drop the implicit `setRequestFailed(false)`. The failure UI's "Try again" button (`handleRetry`) already clears `requestFailed` before bumping `retryAttempt`, so manual recovery still produces a fresh `requestKey` and re-fires the mutation; the only thing that's gone is the silent re-fire path. `deno task test`: 380 passing, no test changes (no test harness exists for the route). hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
`Slug_languageSwitcher`'s `allContents` selection includes
`includeBeingTranslated: true`, so the array can contain placeholder
rows whose `url` field is null (the server hasn't assigned a
canonical URL until the translation completes). The existing-
translations branch of the language switcher's `<For>` was passing
`href: c.url` straight through to the `<a>`, which renders as
`href=""` when the value is null and navigates the user to the
current page instead of anywhere meaningful.
Use the same fallback the source-link path (`sourceUrl()` a few
lines above) and the `extraLocales()` path already use:
`c.url ?? \`${postUrl()}/${c.language}\``. The `/lang` route
handles unknown locales gracefully (it auto-fires the translation
mutation if the viewer can request one, falls through to 404
otherwise), so a placeholder-row link now lands on the same URL the
viewer would have reached by clicking the "Other languages" entry
the first time.
`deno task test`: 380 passing, no test changes (no test harness
exists for the switcher).
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a feature for requesting and viewing article translations powered by LLM. It adds a new GraphQL mutation, requestArticleTranslation, and a dedicated frontend route for language-specific article views that supports automatic translation queuing and status polling. Review feedback suggests optimizing the mutation's database query by fetching actor relations only when visibility is restricted and adding a server-side check to prevent redundant translation jobs if a completed version already exists.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ba16b1d2
ℹ️ 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".
…tion exists `startArticleContentTranslation` is already idempotent against this case (it inspects the conflicting row and returns early without calling the translator), but the resolver was paying for an extra DB round-trip and a hop into the model layer for a no-op. Use the `articleSource.contents` array already loaded by the visibility- check query: if any row is non-`beingTranslated` and (after `normalizeLocale`) matches the requested target locale, skip the `startArticleContentTranslation` call and return the post directly. In-progress and stale-but-in-progress rows are intentionally not short-circuited here so the model layer's 30-minute staleness retry keeps working. Add a test that proves the precheck fires (and the translator does not get invoked) by stubbing `models.translator` with one that throws on either `doGenerate` or `doStream` and asserting the mutation still returns the existing row untouched, with the same `translationRequesterId` it had before. `deno task test`: 381 passing (was 380; +1 new test). hackers-pub#280 (comment) Assisted-by: Claude Code:claude-opus-4-7
Solid Router reuses the `ArticleLangPageContent` instance across
param-only navigations (e.g., `/zh-TW` -> `/ja` on the same article,
or `/ja` on a different article), so any local state the previous
URL left behind carries over to the next visit. Most reactive
state was already covered: `requestKey()` includes both
`props.language` and `article()?.id`, so a navigation produces a
fresh key and the auto-request effect fires for it. But
`requestFailed` (signal) and `firedRequestKey` (let ref) stayed
sticky; in particular the `requestFailed` gate added in the
previous commit meant that a failure on `/zh-TW` would silently
suppress auto-request for the next `/{lang}` URL even though the
visit was unrelated, leaving the user on a 404-ish state with no
affordance to recover short of reloading.
Add a `createEffect` that reads each route prop, then resets
`requestFailed`, `firedRequestKey`, and `prevContentExisted` (the
`missingEpoch` effect's previous-value ref, which would otherwise
spuriously bump `missingEpoch` when the new query temporarily
shows `content == null` while loading); cancel any in-flight
mutation too, since its result no longer applies to the new URL.
The existing-route case (component first mount) is a no-op because
all values are already at their initial state.
`deno task test`: 381 passing, no test changes (no test harness
exists for the route).
hackers-pub#280 (comment)
Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
@codex review |
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a feature for requesting and viewing LLM-powered article translations, including a new GraphQL mutation and a dedicated frontend route for language-specific content. Feedback identifies a likely runtime error in the mutation resolver regarding scalar property access, suggests adding a timeout to the frontend polling mechanism for better resource management, and recommends adjusting canonical URL logic to avoid redundant language tags for original content.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The legacy
web/stack has two article-translation features that web-next didn't carry over: the/@user/year/slug/{lang}URL for viewing an existing translation, and the on-demand LLM translation flow that triggers when a logged-in viewer opens/{lang}for a language that hasn't been translated yet. Until now, every translation link in the new stack returned404 Not Found, and there was no way to ask for a translation in the first place. This PR brings both over, and adds the canonical/Open Graph tags the new article routes were missing while it's at it.Viewing existing translations
web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsx now loads the article with the requested language. If locale negotiation lands somewhere else, it redirects to the canonical URL:
/zh-TWbecomes/zhif onlyzhexists,/enbecomes the bare slug if English is the original. If no matching content exists at all, it returns404 Not Found. The sharedSlug_headandSlug_bodyRelay fragments on the index route gain alanguageargument so the same render path works for both routes.I also added
<link rel="canonical">,og:url, andog:locale:alternatetags both routes were missing, since the legacy article view emits them and the new one didn't.On-demand LLM translation
A new
requestArticleTranslationGraphQL mutation lets any logged-in viewer kick off an LLM translation of an article that hasallowLlmTranslation: true. It mirrors the legacy authorization gate exactly (any signed-in viewer who can see the article, no ownership check), refuses requests where the target locale equals the article's original, and calls the existingstartArticleContentTranslationmodel function to insert abeingTranslated: trueplaceholder row and fire the translation in the background.In the UI, [lang].tsx now handles the piece the legacy route handled on the server. When content for the requested language is missing and the viewer is allowed to request it, a small inline component fires the mutation on mount. If the mutation fails, it shows a destructive toast and returns
404 Not Found. If the placeholder row lands in the Relay store, the normal article render takes over.While the translation is in flight, a
createEffectpolls every 30 seconds viafetchQueryfromrelay-runtime. The first attempt usedrevalidate(loadLangPageQuery.keyFor(...))and quietly didn't work: the Solid Router cache entry was invalidated, butcreatePreloadedQuerykept reading the already-populated Relay store and never showed the completed body until the user reloaded.fetchQuerywrites the fresh response straight into the store, which the preloaded query observes. Verified end-to-end against/de: the page swapped from the placeholder to “Hören Sie auf, If-Statements für Ihre CLI-Flags zu schreiben (bearbeitet)” without a manual reload.The language switcher now adds the viewer's
Account.localesas extra translation links when the article allows LLM translation, deduplicated against existing translations and against the article's original language. Clicking one navigates to/{lang}, where the auto-trigger picks up.Backend resolver fix
Article.contents(includeBeingTranslated: true)was filtering withwhere: { beingTranslated: true }, which returned only in-progress rows instead of "include both completed and in-progress." The new[lang]flow needs both, so the resolver drops the filter whenincludeBeingTranslatedis true. No existing caller passedtrue, so this is a behavior fix rather than a contract break.Translating placeholder
The first cut rendered the placeholder as a bare
<h1>{t`Translating…`}</h1>and gave the viewer no signal that anything was happening. The current placeholder is a bordered card with the Lucideloader-2spinner, a localizedTranslating to {language}…label fromIntl.DisplayNames, and a short note that the page will update when the translation is ready. Used both during the brief “requesting” phase in [lang].tsx and during the long-runningbeingTranslatedphase, so there's no visual jump between them.Tests
The backend gets eight new tests in graphql/post.more.test.ts covering the resolver fix and the mutation branches: guest, missing article, non-Article post, hidden article,
allowLlmTranslation: false, target equals original, and happy path. The happy-path test injects aLanguageModelwhosedoGenerateanddoStreamreturn never-resolving promises so the background failure-cleanup branch instartArticleContentTranslationdoesn't race the assertions.deno task test: 377 passing (was 370 before this PR).The web-next side has no UI tests yet (no test harness in place). I checked the visible paths with Playwright as a logged-in account: auto-request, polling swap, switcher links, redirects, and
404 Not Foundresponses. Translations intoja-JP,ko-KR,zh-CN, andzh-TWare filled in following the existing glossary conventions.Out of scope
The new placeholder card uses the semantic
bordertoken per DESIGN.md. The surrounding language-switcher container still uses the olderborder-stone-200 dark:border-stone-700classes, kept as-is for visual consistency with adjacent code; converting the rest is a separate pass.