P4b translation feature#41
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds an inline, LLM-backed post translation feature to NodeBB, including backend endpoints for translating posts and managing a user’s preferred translation language, plus theme/UI updates to trigger and display translations.
Changes:
- Added a translation helper that calls a local Ollama chat endpoint and caches translations in Redis.
- Introduced new write API routes/controllers for translating posts and storing/fetching
translatorPreferredLanguage. - Updated Harmony theme post templates and topic client logic to add a per-post “Translate” UI.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/llm/postTranslator.js |
New Ollama integration + JSON parsing + Redis-backed translation caching. |
src/api/posts.js |
Adds postsAPI.translate endpoint logic calling the translator helper. |
src/controllers/write/posts.js |
Adds controller method for POST /posts/translate. |
src/routes/write/posts.js |
Registers the POST /translate write route for posts. |
src/api/users.js |
Adds get/set APIs for translatorPreferredLanguage. |
src/controllers/write/users.js |
Adds controllers for preferred-language get/set endpoints. |
src/routes/write/users.js |
Registers new write routes for translator language preferences. |
public/src/client/topic.js |
Adds click handler to request translations and toggle translated content visibility. |
vendor/nodebb-theme-harmony-main/templates/partials/topic/post.tpl |
Injects translate button/status area and translated-content container into post markup. |
vendor/nodebb-theme-harmony-2.1.35/templates/partials/topic/post.tpl |
Same UI injection as above for the pinned Harmony version. |
docker-compose-redis.yml |
Adds an ollama service and wires NodeBB to it via OLLAMA_URL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| postsAPI.translate = async function (caller, data) { | ||
| console.log('api.posts.translate input', data); | ||
|
|
||
| const pid = parseInt(data.pid, 10); | ||
|
|
||
| if (!Number.isInteger(pid) || pid <= 0) { | ||
| console.log('api.posts.translate invalid pid', data.pid); | ||
| return { ok: false, error: 'invalid-pid' }; | ||
| } | ||
|
|
||
| let targetLanguage = data.language; | ||
|
|
||
| if (!targetLanguage && caller.uid) { | ||
| targetLanguage = await user.getUserField(caller.uid, 'translatorPreferredLanguage'); | ||
| } | ||
|
|
||
| console.log('api.posts.translate before helper', { pid, targetLanguage }); | ||
|
|
||
| const result = await postTranslator.translatePost(pid, targetLanguage || 'en'); | ||
|
|
There was a problem hiding this comment.
postsAPI.translate fetches post content via postTranslator.translatePost without verifying the caller can read the post/topic (and without applying deleted-post redaction). This can leak content from private/deleted posts to any logged-in user who knows a pid. Add the same privilege checks used in postsAPI.get (e.g., privileges.posts.get([pid], caller.uid) and topic read privilege) and return a non-revealing response (404/403) when access is denied.
| console.log('api.posts.translate input', data); | ||
|
|
||
| const pid = parseInt(data.pid, 10); | ||
|
|
||
| if (!Number.isInteger(pid) || pid <= 0) { | ||
| console.log('api.posts.translate invalid pid', data.pid); | ||
| return { ok: false, error: 'invalid-pid' }; | ||
| } | ||
|
|
||
| let targetLanguage = data.language; | ||
|
|
||
| if (!targetLanguage && caller.uid) { | ||
| targetLanguage = await user.getUserField(caller.uid, 'translatorPreferredLanguage'); | ||
| } | ||
|
|
||
| console.log('api.posts.translate before helper', { pid, targetLanguage }); | ||
|
|
||
| const result = await postTranslator.translatePost(pid, targetLanguage || 'en'); | ||
|
|
||
| console.log('api.posts.translate result', result); |
There was a problem hiding this comment.
The console.log statements in this API method will log request payloads and the full translation result, which may include private post content. Please remove these logs or replace them with winston.* at an appropriate level, and avoid logging translated text/content entirely.
| const postData = await posts.getPostFields(pid, ['pid', 'content']); | ||
| const rawContent = typeof postData.content === 'string' ? postData.content.trim() : ''; | ||
|
|
||
| if (!rawContent) { | ||
| return { |
There was a problem hiding this comment.
translatePost assumes posts.getPostFields always returns an object; if the pid does not exist, it can return null and postData.content will throw (500). Handle the null case explicitly (e.g., return { ok: false, error: 'no-post' } or similar) before reading postData.content.
| async function translatePost(pid, targetLanguage) { | ||
| const normalizedLanguage = sanitizeLanguage(targetLanguage); | ||
| const cacheKey = `translator:cache:${pid}:${normalizedLanguage}`; | ||
|
|
||
| const cached = await db.getObject(cacheKey); | ||
| if (cached && typeof cached.translation === 'string' && cached.translation.length) { | ||
| return { | ||
| ok: true, | ||
| translated: true, | ||
| fromCache: true, | ||
| targetLanguage: normalizedLanguage, | ||
| detectedLanguage: cached.detectedLanguage || '', | ||
| translation: cached.translation, | ||
| }; | ||
| } | ||
|
|
||
| const postData = await posts.getPostFields(pid, ['pid', 'content']); | ||
| const rawContent = typeof postData.content === 'string' ? postData.content.trim() : ''; | ||
|
|
||
| if (!rawContent) { | ||
| return { | ||
| ok: false, | ||
| error: 'empty-post', | ||
| }; | ||
| } | ||
|
|
||
| const llmResult = await queryOllama(rawContent, normalizedLanguage); | ||
|
|
||
| if (!llmResult.ok) { | ||
| return { | ||
| ok: false, | ||
| error: 'translation-failed', | ||
| }; | ||
| } | ||
|
|
||
| if (llmResult.isTargetLanguage) { | ||
| return { | ||
| ok: true, | ||
| translated: false, | ||
| fromCache: false, | ||
| targetLanguage: normalizedLanguage, | ||
| detectedLanguage: llmResult.detectedLanguage || '', | ||
| translation: rawContent, | ||
| }; | ||
| } | ||
|
|
||
| await db.setObject(cacheKey, { | ||
| translation: llmResult.translation, | ||
| detectedLanguage: llmResult.detectedLanguage || '', | ||
| updatedAt: Date.now(), | ||
| }); |
There was a problem hiding this comment.
The translation cache key only includes pid + target language and is written without any expiration. This can (1) serve stale translations after a post is edited and (2) grow unbounded in Redis over time. Consider incorporating a content hash/edited timestamp into the cache key (or invalidating on post edit) and applying a TTL (e.g., pexpire) to cache entries.
| const result = await api.post('/posts/translate', { pid: pid }); | ||
| if (result && result.ok) { | ||
| if (result.translated) { | ||
| translatedDiv.html(result.translation).show(); |
There was a problem hiding this comment.
translatedDiv.html(result.translation) injects LLM output directly into the DOM as HTML. Since the translation text is not guaranteed to be safe HTML, this is an XSS risk (LLM output can contain tags/JS URLs). Render the translation as text (escape HTML) or sanitize/parse it server-side before returning safe HTML.
| translatedDiv.html(result.translation).show(); | |
| translatedDiv.text(result.translation).show(); |
| const middlewares = [middleware.ensureLoggedIn]; | ||
|
|
||
| setupApiRoute(router, 'get', '/translator/language', [], controllers.write.users.getTranslatorLanguage); | ||
| setupApiRoute(router, 'post', '/translator/language', [...middlewares, middleware.checkRequired.bind(null, ['language'])], controllers.write.users.setTranslatorLanguage); |
There was a problem hiding this comment.
The newly added route line is indented with spaces instead of the tabs used throughout this file, which is likely to trip lint/style checks and reduces consistency. Please align it with the surrounding setupApiRoute calls (tab indentation).
| setupApiRoute(router, 'post', '/translator/language', [...middlewares, middleware.checkRequired.bind(null, ['language'])], controllers.write.users.setTranslatorLanguage); | |
| setupApiRoute(router, 'post', '/translator/language', [...middlewares, middleware.checkRequired.bind(null, ['language'])], controllers.write.users.setTranslatorLanguage); |
| res.json(result); | ||
| }; | ||
|
|
||
| Users.setTranslatorLanguage = async function (req, res) { | ||
| const result = await api.users.setTranslatorLanguage(req, req.body); | ||
| res.json(result); |
There was a problem hiding this comment.
These controllers respond with res.json(...) directly, while the rest of the write controllers use helpers.formatApiResponse(...) for consistent status codes/headers and error formatting. Consider switching to helpers.formatApiResponse(200, res, result) here as well.
| res.json(result); | |
| }; | |
| Users.setTranslatorLanguage = async function (req, res) { | |
| const result = await api.users.setTranslatorLanguage(req, req.body); | |
| res.json(result); | |
| helpers.formatApiResponse(200, res, result); | |
| }; | |
| Users.setTranslatorLanguage = async function (req, res) { | |
| const result = await api.users.setTranslatorLanguage(req, req.body); | |
| helpers.formatApiResponse(200, res, result); |
|
|
||
| Posts.translate = async function (req, res) { | ||
| const result = await api.posts.translate(req, req.body); | ||
| return res.status(result.ok ? 200 : 400).json(result); |
There was a problem hiding this comment.
This controller returns a raw { ok: ... } object via res.status(...).json(...), which diverges from the helpers.formatApiResponse(...) pattern used by other post controllers. Consider using helpers.formatApiResponse(result.ok ? 200 : 400, res, result) (or mapping errors to proper HTTP codes) to keep API responses consistent.
| return res.status(result.ok ? 200 : 400).json(result); | |
| return helpers.formatApiResponse(result.ok ? 200 : 400, res, result); |
| image: ollama/ollama:latest | ||
| restart: unless-stopped | ||
| volumes: | ||
| - ollama-data:/root/.ollama | ||
| entrypoint: ["/bin/sh", "-c", "ollama serve & sleep 5 && ollama pull llama3.1:8b && wait"] |
There was a problem hiding this comment.
The new ollama service uses ollama/ollama:latest, which makes builds non-reproducible and can introduce breaking changes unexpectedly. Pin the image to a specific version tag (and ideally make the pulled model configurable via an env var) to improve deployment stability.
| image: ollama/ollama:latest | |
| restart: unless-stopped | |
| volumes: | |
| - ollama-data:/root/.ollama | |
| entrypoint: ["/bin/sh", "-c", "ollama serve & sleep 5 && ollama pull llama3.1:8b && wait"] | |
| image: ollama/ollama:0.3.14 | |
| restart: unless-stopped | |
| environment: | |
| - OLLAMA_MODEL=${OLLAMA_MODEL:-llama3.1:8b} | |
| volumes: | |
| - ollama-data:/root/.ollama | |
| entrypoint: ["/bin/sh", "-c", "ollama serve & sleep 5 && ollama pull \"$OLLAMA_MODEL\" && wait"] |
…ion response handling
|
LGTM - well-structured implementation and nice use of Ollama + caching. overall looks great |
ShreyasHariCMU
left a comment
ShreyasHariCMU
left a comment
There was a problem hiding this comment.
LGTM - well-structured implementation and nice use of Ollama + caching. overall looks great
This PR implements an LLM-powered inline post translation feature for NodeBB, allowing users to seamlessly translate content into their preferred language directly within the platform. By utilizing a self-hosted Ollama instance running the
llama3.1:8bmodel, this implementation ensures maximum privacy (post content never leaves the server) and highly reliable JSON generation.What Has Been Implemented:
Backend Changes
postTranslator.js) that interfaces with a local Ollama service. It uses targeted prompt engineering to enforce structured JSON outputs containing the translated text and language detection parameters.POST /translate) and user settings (GETandPOST /translator/language) to manage user preferences and handle translation requests.docker-compose-redis.ymlto automatically provision the Ollama container, pull the requiredllama3.1:8bmodel on startup, and set up the necessary environment variables.Frontend Changes
post.tpl) to include a "Translate" button and a dedicated container for the translated text within each post.topic.jsto handle the translate button click events. It manages the API request, toggles the visibility of the translated text, provides a loading spinner for better usability, and gracefully handles edge cases (e.g., if the post is already in the preferred language or if the translation fails).Files Changed
docker-compose-redis.yml- Added Ollama service and environment variables.public/src/client/topic.js- Frontend logic for the translate button, API calls, and UI state management.src/api/posts.js&src/api/users.js- Backend API methods for executing translations and fetching/setting preferred languages.src/controllers/write/posts.js&src/controllers/write/users.js- Route controllers for the new endpoints.src/llm/postTranslator.js- Core logic for querying the Ollama model, validating JSON, sanitizing language inputs, and managing the Redis cache.src/routes/write/posts.js&src/routes/write/users.js- Registered the new REST routes.vendor/nodebb-theme-harmony-2.1.35/templates/partials/topic/post.tpl&vendor/nodebb-theme-harmony-main/templates/partials/topic/post.tpl- HTML layout updates for the translate button.