refactor(i18n): extract theme-chrome translation into standalone module (#192) - #210
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
@greptile review |
🚀 Preview DeploymentYour documentation preview is ready! Preview URL: https://pr-210.comapeo-docs-82j.pages.dev 📦 Content: from This preview will update automatically when you push new commits to this PR. Built with commit f5d3062 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isExecutedDirectly = | ||
| process.argv[1] !== undefined && | ||
| path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); |
There was a problem hiding this comment.
The advertised bun scripts/translate-theme command passes the module directory as process.argv[1], but this check only accepts the resolved index.ts path. The condition stays false, so the command exits without calling translateThemeConfig or producing translations. Accept the module directory as a direct invocation, as the repository's notion-fetch-all entry point does.
| const isExecutedDirectly = | |
| process.argv[1] !== undefined && | |
| path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); | |
| const modulePath = fileURLToPath(import.meta.url); | |
| const isExecutedDirectly = | |
| process.argv[1] !== undefined && | |
| (path.resolve(process.argv[1]) === modulePath || | |
| path.resolve(process.argv[1]) === path.dirname(modulePath)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/translate-theme/index.ts
Line: 19-21
Comment:
**Directory Command Does Nothing**
The advertised `bun scripts/translate-theme` command passes the module directory as `process.argv[1]`, but this check only accepts the resolved `index.ts` path. The condition stays false, so the command exits without calling `translateThemeConfig` or producing translations. Accept the module directory as a direct invocation, as the repository's `notion-fetch-all` entry point does.
```suggestion
const modulePath = fileURLToPath(import.meta.url);
const isExecutedDirectly =
process.argv[1] !== undefined &&
(path.resolve(process.argv[1]) === modulePath ||
path.resolve(process.argv[1]) === path.dirname(modulePath));
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| } else { | ||
| try { | ||
| langDirs = await fs.readdir(i18nDir); | ||
| } catch (error) { | ||
| console.warn( | ||
| chalk.yellow( | ||
| `⚠ Could not read i18n directory at ${i18nDir}: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }` | ||
| ) | ||
| ); | ||
| return failures; | ||
| } | ||
| } |
There was a problem hiding this comment.
Any failure to read i18n/, including a missing directory, permission failure, or I/O error, returns an empty failure list. notion-translate then records zero theme failures and can report success without generating navbar or footer translations. Before this extraction, the same error propagated and failed the run.
| } else { | |
| try { | |
| langDirs = await fs.readdir(i18nDir); | |
| } catch (error) { | |
| console.warn( | |
| chalk.yellow( | |
| `⚠ Could not read i18n directory at ${i18nDir}: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }` | |
| ) | |
| ); | |
| return failures; | |
| } | |
| } | |
| } else { | |
| langDirs = await fs.readdir(i18nDir); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/translate-theme/translateTheme.ts
Line: 57-70
Comment:
**Directory Errors Report Success**
Any failure to read `i18n/`, including a missing directory, permission failure, or I/O error, returns an empty failure list. `notion-translate` then records zero theme failures and can report success without generating navbar or footer translations. Before this extraction, the same error propagated and failed the run.
```suggestion
} else {
langDirs = await fs.readdir(i18nDir);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (failures.length > 0) { | ||
| console.warn( | ||
| `Completed with ${failures.length} theme translation failures.` | ||
| ); | ||
| if (failures.some((f) => f.isCritical)) { | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
The standalone entry point exits unsuccessfully only when a returned failure has isCritical, but the default translateJson implementation throws plain Error instances, so every generated failure has isCritical: false. Authentication, quota, or translation failures can therefore prevent all output while the command still exits successfully.
| if (failures.length > 0) { | |
| console.warn( | |
| `Completed with ${failures.length} theme translation failures.` | |
| ); | |
| if (failures.some((f) => f.isCritical)) { | |
| process.exit(1); | |
| } | |
| } | |
| if (failures.length > 0) { | |
| console.warn( | |
| `Completed with ${failures.length} theme translation failures.` | |
| ); | |
| process.exit(1); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/translate-theme/index.ts
Line: 26-33
Comment:
**Translation Failures Exit Successfully**
The standalone entry point exits unsuccessfully only when a returned failure has `isCritical`, but the default `translateJson` implementation throws plain `Error` instances, so every generated failure has `isCritical: false`. Authentication, quota, or translation failures can therefore prevent all output while the command still exits successfully.
```suggestion
if (failures.length > 0) {
console.warn(
`Completed with ${failures.length} theme translation failures.`
);
process.exit(1);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return failures; | ||
| } | ||
| /** | ||
| } /** |
There was a problem hiding this comment.
The extraction joins a closing brace and the next documentation comment as } /**. This violates the repository directive to follow Prettier formatting and must be corrected before merging.
| } /** | |
| } | |
| /** |
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/notion-translate/index.ts
Line: 1239
Comment:
**Comment Breaks Formatting Rule**
The extraction joins a closing brace and the next documentation comment as `} /**`. This violates the repository directive to follow Prettier formatting and must be corrected before merging.
```suggestion
}
/**
```
**Context Used:** CLAUDE.md ([source](https://github.com/digidem/comapeo-docs/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
14f3b8d to
017493c
Compare
017493c to
f5d3062
Compare
|
@greptile review |
🧹 Preview Deployment CleanupThe preview deployment for this PR has been cleaned up. Preview URL was: Note: Cloudflare Pages deployments follow automatic retention policies. Old previews are cleaned up automatically. |
1 participant
Summary
Extracts theme-chrome translation (navbar, footer, and
code.jsontranslation helpers) out of the legacyscripts/notion-translate/into a standalone, modular package inscripts/translate-theme/as planned in Phase 3 of #192.Changes
scripts/translate-theme/:types.ts: Common types for theme config, translatable dictionaries, and translation failure reporting.languageNames.ts: Language name mapping (LANGUAGE_MAP,getLanguageName).navbarFooter.ts: Translatable text extraction (extractTranslatableText) with support for Docusaurus runtime navbar & footer i18n keys.translateJson.ts: OpenAI JSON-mode translation utility with exponential retry jitter and custom OpenAI backend support.translateTheme.ts:translateThemeConfig()orchestrator readingdocusaurus.config.tsand writingi18n/<locale>/docusaurus-theme-classic/{navbar,footer}.json. Supports dependency injection for seamless testing.index.ts: Public exports and CLI entry point (bun scripts/translate-theme).scripts/translate-theme/__tests__/covering all extracted modules (16 tests, 100% pass).scripts/notion-translate/translateCodeJson.tsto maintain complete backward compatibility with existing callers.translateThemeConfiginscripts/notion-translate/index.tswhile preserving all existing test mocks.Verification
bunx vitest run scripts/translate-theme: 16/16 tests passing.bunx vitest run scripts/notion-translate: 191/191 tests passing (8 test files).bun run typecheck --noEmit: Clean (0 errors).bunx eslint: Clean (0 errors, 0 warnings).bunx prettier --write: Formatted.Part of #192
Greptile Summary
Extracts theme-chrome translation into a standalone module while retaining compatibility with the legacy translation workflow.
i18n/errors, and return a failing exit status for any translation failure.Confidence Score: 5/5
PR appears safe to merge; all previous findings are fully addressed and no new actionable failures remain.
Direct directory invocation now reaches the CLI, every returned translation failure produces an unsuccessful exit status, unreadable
i18n/directories propagate errors, and the prior formatting violation is corrected.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[bun scripts/translate-theme] --> B[translateThemeConfig] C[notion-translate main] --> B B --> D[Load Docusaurus config] D --> E[Extract navbar and footer strings] E --> F[Enumerate i18n locales] F --> G[translateJson via OpenAI] G --> H[Write navbar.json and footer.json] G -->|failure| I[Collect TranslationFailure] I --> J[Set unsuccessful CLI exit status]Reviews (3): Last reviewed commit: "refactor(i18n): extract theme-chrome tra..." | Re-trigger Greptile