Add multimodal auto translate#40
Closed
jokester wants to merge 13 commits into
Closed
Conversation
Contributor
WalkthroughReplaces legacy Moeflow/MIT multimodal OCR pipeline with an LLM-driven preprocessing and batch-translation flow: adds LLM config UI, modal orchestration hook, batch translation modal, integrates into FileList, updates dependencies, removes multiple legacy AI modules, and adds a dev-only guard in user saga. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant FL as FileList
participant Hook as useAiTranslate
participant M1 as Modal_ModelConfig
participant M2 as Modal_BatchTranslate
participant LLM as llmPreprocessFile
participant API as Moeflow API / Save
U->>FL: Click "AI Translate"
FL->>Hook: api.start(onFileSaved)
Hook->>M1: Open model config modal
U-->>M1: Provide model/baseUrl/apiKey
M1-->>Hook: Confirm -> LLMConf
Hook->>M2: Open batch translate modal with files, target, LLMConf
loop per file (concurrent pools)
M2->>LLM: preprocess(file image data + prompt)
alt success
LLM-->>M2: FilePreprocessResult (texts, imageW/H)
M2->>API: save text blocks (mapped coords)
API-->>M2: saved
M2-->>Hook: invoke onFileSaved(file)
else failure
LLM-->>M2: error -> mark file error
end
end
M2-->>U: Done (summary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ai/BatchTranslateModal.tsx (1)
56-58: Tasks orchestration bug: allSettled resolves immediately.You’re passing a single-element array containing an array of promises; nothing is awaited. Use the mapped array directly.
- const tasksEnded = Promise.allSettled([ - files.map((f, idx) => fileLimiter.use(() => translateFile(f, idx))), - ]); + const tasksEnded = Promise.allSettled( + files.map((f, idx) => fileLimiter.use(() => translateFile(f, idx))), + );
🧹 Nitpick comments (14)
package.json (1)
13-15: Pin beta deps to exact versions to avoid surprise breaksCaret on pre-release tags can pull incompatible beta builds. Pin exactly.
- "@xsai-ext/providers-cloud": "^0.4.0-beta.2", - "@xsai/generate-object": "^0.4.0-beta.2", + "@xsai-ext/providers-cloud": "0.4.0-beta.2", + "@xsai/generate-object": "0.4.0-beta.2", ... - "xsai": "^0.4.0-beta.2", - "zod": "^3.25.76", - "zod-to-json-schema": "^3.24.6" + "xsai": "0.4.0-beta.2", + "zod": "3.25.76", + "zod-to-json-schema": "3.24.6"Also applies to: 48-51
src/components/project/FileList.tsx (2)
86-91: Avoid O(n×m) lookup when building selected filesBuild a Set once and filter items; simpler and faster for large lists.
- const [aiEnabled, aiTranslateApi, aiModalHolder] = useAiTranslate( - [...new Set(selectedFileIds)] - .map((id) => items.find((item) => item.id === id)) - .filter(Boolean) as MFile[], - target, - ); + const selectedIdSet = new Set(selectedFileIds); + const [aiEnabled, aiTranslateApi, aiModalHolder] = useAiTranslate( + items.filter((item) => selectedIdSet.has(item.id)), + target, + );
387-401: Hide AI action when nothing is selectedPrevents launching an empty batch.
- {aiEnabled && aiTranslateApi && ( + {aiEnabled && aiTranslateApi && selectedFileIds.length > 0 && (src/components/ai/index.tsx (1)
4-4: Avoid deep type import from antd internalsUse the public type from Modal.useModal to reduce breakage risk.
-import { ModalStaticFunctions } from 'antd/lib/modal/confirm'; +type ModalAPI = ReturnType<typeof Modal.useModal>[0]; ... -function bind( +function bind( files: MFile[], target: Target, - modal: ModalStaticFunctions, + modal: ModalAPI, ): TranslatorApi { ... - () => bind(files, target, modal as ModalStaticFunctions), + () => bind(files, target, modal as ModalAPI),Also applies to: 22-26, 87-89
src/components/ai/ModelConfigForm.tsx (3)
63-91: Preset auto-detection: use nullish coalescing and consistent Form API.
- Prefer ?? over || to avoid treating empty strings as “unset”.
- Use setFieldsValue for consistency/antd v4 compatibility.
- const handleFormChange = (changedValues: any, allValues: any) => { + const handleFormChange = (changedValues: any, allValues: any) => { // Check if model or baseUrl was changed and update preset accordingly if ( changedValues.model !== undefined || changedValues.baseUrl !== undefined ) { - const currentModel = allValues.model || changedValues.model; - const currentBaseUrl = allValues.baseUrl || changedValues.baseUrl; + const currentModel = allValues.model ?? changedValues.model; + const currentBaseUrl = allValues.baseUrl ?? changedValues.baseUrl; // Find matching preset const matchingPresetIndex = LlmService.llmPresets.findIndex( (preset) => preset.model === currentModel && preset.baseUrl === currentBaseUrl, ); // Update preset to match the current values if (matchingPresetIndex >= 0) { // Found a matching preset, switch to it - if (allValues.preset !== matchingPresetIndex) { - form.setFieldValue('preset', matchingPresetIndex); - } + if (allValues.preset !== matchingPresetIndex) { + form.setFieldsValue({ preset: matchingPresetIndex }); + } } else { // No preset matches, set to custom (-1) - if (allValues.preset !== -1) { - form.setFieldValue('preset', -1); - } + if (allValues.preset !== -1) { + form.setFieldsValue({ preset: -1 }); + } } }
115-119: Fix typos in user-facing text.- The LLM API should use the OpenAI-compatible format and API key - authencation. The model should support image input and structured + The LLM API should use the OpenAI-compatible format and API key + authentication. The model should support image input and structured output. - <p>This configuration is only used and saved inside in your browser.</p> + <p>This configuration is only used and saved in your browser.</p>
164-170: Expose “Extra prompt” to match LLMConf and downstream usage.BatchTranslateModal uses llmConf.extraPrompt, but there’s no field here. Add an optional textarea.
<Form.Item label="API Key" name="apiKey" rules={[{ required: true, message: 'Please enter your API key' }]} > <Input.Password placeholder="Enter your API key" autoComplete="off" /> </Form.Item> + <Form.Item label="Extra prompt (optional)" name="extraPrompt"> + <Input.TextArea + placeholder="Additional instructions for the model" + maxLength={1000} + autoSize={{ minRows: 2, maxRows: 6 }} + /> + </Form.Item>src/components/ai/BatchTranslateModal.tsx (7)
31-38: Prop order mismatch is fine; but consider removing unused intl.intl is unused; safe to drop to reduce noise. Functional behavior unaffected.
- const intl = useIntl(); + // const intl = useIntl();
43-43: Remove dead stub or implement.startWork is never called. Remove or wire it up.
- async function startWork() {} + // TODO: remove or implement startWork if needed
101-115: Prompt assembly: trim extra whitespace when extraPrompt is absent.Minor polish to avoid trailing spaces.
- text: `Please translate the image to ${target.language.enName}. ${llmConf.extraPrompt || ''}`, + text: `Please translate the image to ${target.language.enName}. ${llmConf.extraPrompt ?? ''}`.trim(),
156-160: Avoid status flicker on empty results.Return early after marking “no text blocks”; otherwise it gets overwritten by “saving/success”.
async function saveTranslations(f: MFile, r: FilePreprocessResult) { if (r.texts.length === 0) { setFileState(f, 'done: no text blocks'); - } + return; + }
188-194: Add onerror handling for FileReader.Reject on read errors to surface failures cleanly.
async function img2dataurl(img: Blob) { return new Promise<string>((resolve, reject) => { const reader = new FileReader(); reader.onloadend = () => resolve(reader.result as string); + reader.onerror = () => reject(reader.error ?? new Error('read failed')); + reader.onabort = () => reject(new Error('read aborted')); reader.readAsDataURL(img); }); }
92-95: Defensive: handle missing file URL.If resData.url is falsy, skip early instead of asserting non-null.
- const imgBlob = await fetch(resData.url!, {}).then( + if (!resData.url) { + setFileState(f, 'skip: missing file URL'); + return; + } + const imgBlob = await fetch(resData.url, {}).then( (r) => r.blob(), () => null, );
140-145: HandlecreateSourcefailures before usingsrc.data.id
InsaveTextBlock(BatchTranslateModal.tsx:137–145), add a guard after callingapi.source.createSource, e.g.:const src = await api.source.createSource({ … }); if (src.type !== resultTypes.SUCCESS) { setFileState(f, 'error: create source failed'); return; }This prevents accessing
src.data.idwhen the API returns a failure result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.json(2 hunks)src/components/ai/BatchTranslateModal.tsx(6 hunks)src/components/ai/ModelConfigForm.tsx(1 hunks)src/components/ai/index.tsx(1 hunks)src/components/project/FileList.tsx(4 hunks)src/services/ai/TranslateCompanion.tsx(0 hunks)src/services/ai/llm_preprocess.ts(1 hunks)src/services/ai/mit_preprocess.ts(0 hunks)src/services/ai/multimodal_recognize.ts(0 hunks)src/services/ai/use_moeflow_companion.ts(0 hunks)src/services/labelplus_packager.ts(0 hunks)src/store/user/sagas.ts(1 hunks)
💤 Files with no reviewable changes (5)
- src/services/labelplus_packager.ts
- src/services/ai/TranslateCompanion.tsx
- src/services/ai/use_moeflow_companion.ts
- src/services/ai/multimodal_recognize.ts
- src/services/ai/mit_preprocess.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/ai/ModelConfigForm.tsx (1)
src/services/ai/llm_preprocess.ts (1)
LLMConf(4-10)
src/components/ai/index.tsx (5)
src/utils/debug-logger.ts (1)
createDebugLogger(3-5)src/services/ai/llm_preprocess.ts (1)
LLMConf(4-10)src/interfaces/target.ts (1)
Target(4-12)src/components/ai/ModelConfigForm.tsx (1)
ModelConfigForm(24-176)src/components/ai/BatchTranslateModal.tsx (1)
BatchTranslateModalContent(31-186)
src/components/project/FileList.tsx (1)
src/components/ai/index.tsx (1)
useAiTranslate(80-92)
src/components/ai/BatchTranslateModal.tsx (4)
src/utils/debug-logger.ts (1)
createDebugLogger(3-5)src/services/ai/llm_preprocess.ts (3)
LLMConf(4-10)llmPreprocessFile(58-82)FilePreprocessResult(50-50)src/interfaces/target.ts (1)
Target(4-12)src/components/ai/index.tsx (1)
ModalHandle(13-13)
🔇 Additional comments (6)
src/store/user/sagas.ts (1)
14-25: Double-check logout flows in development
DispatchingsetUserToken({ token: '' })currently hits the dev‐only early return in your saga, so the AxiosAuthorizationheader, cookie, and store’s user info aren’t cleared. Ensure your explicit logout actions bypass this guard or otherwise clear auth state in development mode.src/components/project/FileList.tsx (2)
593-594: LGTM — modal holder integrationRendering aiModalHolder at root is correct for antd’s context.
394-397: The search shows only the interface definition forTranslatorApi—no implementation or wiring ofonFileSavedis found. You’ll need to verify manually thatstartinvokes the passed callback when files are saved inside the AI modal; if it doesn’t, remove the unused parameter or hook it up.src/components/ai/index.tsx (1)
61-66: No TDZ risk: getHandle is invoked inside an effect after mount — the call at lines 62–66 is within a useEffect’s async callback (with a cancelled guard), so the modal handle is always assigned before use.Likely an incorrect or invalid review comment.
src/components/ai/ModelConfigForm.tsx (1)
121-135: No action needed: project uses antd ^4.24.16, so Select.Option is supported.src/services/ai/llm_preprocess.ts (1)
63-81: Pass abortSignal directly; otherwise LGTM.The generateObject call already receives abortSignal; no change needed here. Just confirming the usage is correct after fixing imports/schema.
Comment on lines
+15
to
+21
| interface TranslatorApi { | ||
| start( | ||
| onFileSaved: (f: MFile) => void, | ||
| onConfigured?: () => void, | ||
| ): Promise<void>; | ||
| testModel(modelConf: LLMConf): Promise<{ worked: boolean; message: string }>; | ||
| } |
Contributor
There was a problem hiding this comment.
start() ignores its parameters; wire callbacks and i18n
- The method signature advertises onFileSaved/onConfigured but they aren’t used.
- Hardcoded "Start translate" should be localized.
-interface TranslatorApi {
- start(
- onFileSaved: (f: MFile) => void,
- onConfigured?: () => void,
- ): Promise<void>;
+interface TranslatorApi {
+ start(
+ onFileSaved: (f: MFile) => void,
+ onConfigured?: () => void,
+ ): Promise<void>;
testModel(modelConf: LLMConf): Promise<{ worked: boolean; message: string }>;
}
...
- async function start() {
+ async function start(
+ onFileSaved: (f: MFile) => void,
+ onConfigured?: () => void,
+ ) {
const llmConf = await new Promise<LLMConf | null>((resolve, reject) => {
let confValue: LLMConf | null = null;
const onChange = (conf: LLMConf) => {
debugLogger('model configured', conf);
confValue = conf;
if (confValue.model && confValue.baseUrl && confValue.apiKey) {
handle.update({ okButtonProps: {} });
}
};
const handle = modal.confirm({
icon: null,
content: <ModelConfigForm onChange={onChange} />,
- okText: `Start translate`,
+ okText: /* TODO(i18n) */ `Start translate`,
okButtonProps: { disabled: true },
onOk: () => {
- resolve(confValue);
+ onConfigured?.();
+ resolve(confValue);
},
onCancel: () => {
resolve(null);
},
});
});Also applies to: 31-57
Comment on lines
+39
to
+49
| useEffect(() => { | ||
| if (initialValue) { | ||
| const presetIndex = findPresetIndex(initialValue); | ||
| form.setFieldsValue({ | ||
| preset: presetIndex, | ||
| model: initialValue.model, | ||
| baseUrl: initialValue.baseUrl, | ||
| apiKey: initialValue.apiKey, | ||
| }); | ||
| } | ||
| }, [initialValue, form]); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit initial config to parent on mount (provider included).
setFieldsValue doesn’t trigger onValuesChange, so onChange never fires for initialValue. Emit once after populating fields and derive provider from the preset.
useEffect(() => {
if (initialValue) {
const presetIndex = findPresetIndex(initialValue);
form.setFieldsValue({
preset: presetIndex,
model: initialValue.model,
baseUrl: initialValue.baseUrl,
apiKey: initialValue.apiKey,
});
+ const provider =
+ presetIndex >= 0
+ ? LlmService.llmPresets[presetIndex].provider
+ : initialValue.provider ?? '';
+ onChange?.({
+ provider,
+ model: initialValue.model,
+ baseUrl: initialValue.baseUrl,
+ apiKey: initialValue.apiKey,
+ });
}
}, [initialValue, form]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (initialValue) { | |
| const presetIndex = findPresetIndex(initialValue); | |
| form.setFieldsValue({ | |
| preset: presetIndex, | |
| model: initialValue.model, | |
| baseUrl: initialValue.baseUrl, | |
| apiKey: initialValue.apiKey, | |
| }); | |
| } | |
| }, [initialValue, form]); | |
| useEffect(() => { | |
| if (initialValue) { | |
| const presetIndex = findPresetIndex(initialValue); | |
| form.setFieldsValue({ | |
| preset: presetIndex, | |
| model: initialValue.model, | |
| baseUrl: initialValue.baseUrl, | |
| apiKey: initialValue.apiKey, | |
| }); | |
| const provider = | |
| presetIndex >= 0 | |
| ? LlmService.llmPresets[presetIndex].provider | |
| : initialValue.provider ?? ''; | |
| onChange?.({ | |
| provider, | |
| model: initialValue.model, | |
| baseUrl: initialValue.baseUrl, | |
| apiKey: initialValue.apiKey, | |
| }); | |
| } | |
| }, [initialValue, form]); |
🤖 Prompt for AI Agents
In src/components/ai/ModelConfigForm.tsx around lines 39 to 49, setFieldsValue
currently populates the form on mount but does not trigger onValuesChange so the
parent never receives the initial config; after setting fields, call the same
onChange handler (or explicitly emit the initial values) once to send the
populated config to the parent, deriving the provider from the computed
presetIndex (e.g., lookup provider from presets[presetIndex]) and include model,
baseUrl, apiKey and provider in the emitted object; ensure this runs only when
initialValue is present to avoid duplicate emissions.
Comment on lines
+51
to
+61
| // Handle preset selection change | ||
| const handlePresetChange = (presetIndex: number) => { | ||
| if (presetIndex >= 0 && presetIndex < LlmService.llmPresets.length) { | ||
| const preset = LlmService.llmPresets[presetIndex]; | ||
| form.setFieldsValue({ | ||
| model: preset.model, | ||
| baseUrl: preset.baseUrl, | ||
| }); | ||
| } | ||
| // For custom preset (index -1), don't auto-fill fields | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Selecting a preset should also notify parent.
setFieldsValue doesn’t fire onValuesChange; parent won’t get updates after preset pick. Call onChange with the derived config.
const handlePresetChange = (presetIndex: number) => {
if (presetIndex >= 0 && presetIndex < LlmService.llmPresets.length) {
const preset = LlmService.llmPresets[presetIndex];
form.setFieldsValue({
model: preset.model,
baseUrl: preset.baseUrl,
});
+ onChange?.({
+ provider: preset.provider,
+ model: preset.model,
+ baseUrl: preset.baseUrl,
+ apiKey: form.getFieldValue('apiKey'),
+ });
}
// For custom preset (index -1), don't auto-fill fields
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle preset selection change | |
| const handlePresetChange = (presetIndex: number) => { | |
| if (presetIndex >= 0 && presetIndex < LlmService.llmPresets.length) { | |
| const preset = LlmService.llmPresets[presetIndex]; | |
| form.setFieldsValue({ | |
| model: preset.model, | |
| baseUrl: preset.baseUrl, | |
| }); | |
| } | |
| // For custom preset (index -1), don't auto-fill fields | |
| }; | |
| // Handle preset selection change | |
| const handlePresetChange = (presetIndex: number) => { | |
| if (presetIndex >= 0 && presetIndex < LlmService.llmPresets.length) { | |
| const preset = LlmService.llmPresets[presetIndex]; | |
| form.setFieldsValue({ | |
| model: preset.model, | |
| baseUrl: preset.baseUrl, | |
| }); | |
| onChange?.({ | |
| provider: preset.provider, | |
| model: preset.model, | |
| baseUrl: preset.baseUrl, | |
| apiKey: form.getFieldValue('apiKey'), | |
| }); | |
| } | |
| // For custom preset (index -1), don't auto-fill fields | |
| }; |
🤖 Prompt for AI Agents
In src/components/ai/ModelConfigForm.tsx around lines 51 to 61, selecting a
preset currently calls form.setFieldsValue but that does not trigger
onValuesChange so the parent is not notified; after setting the form values for
a valid preset, construct the derived config object (model, baseUrl and any
other fields the parent expects) and call the component's
onChange/onConfigChange callback with that config so the parent receives the
update; do not call the callback for the custom preset index (-1).
Comment on lines
+27
to
+48
| const FilePreprocessResultSchema = z.object({ | ||
| imageW: z.number({ message: 'the width of the image in PX' }), | ||
| imageH: z.number({ message: 'the height of the image in PX' }), | ||
| texts: z.array( | ||
| z.object({ | ||
| left: z.number({ | ||
| message: 'left coordinate of the text in PX, in the whole image', | ||
| }), | ||
| top: z.number({ | ||
| message: 'top coordinate of the text in PX, in the whole image', | ||
| }), | ||
| width: z.number({ message: 'width of the text in PX' }), | ||
| height: z.number({ message: 'height of the text in PX' }), | ||
| textLines: z.array(z.string(), { message: 'the text lines' }), | ||
| text: z.string({ message: 'concatencated text' }), | ||
| translated: z.string({ message: 'translated text' }), | ||
| comment: z.string({ | ||
| message: 'additional comment of the text, or the translation', | ||
| }), | ||
| }), | ||
| ), | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema options misuse: replace unsupported “message” params with .describe; make comment optional.
The object-literal option “message” isn’t a valid Zod constructor option; use .describe(). Also “comment” should likely be optional with a default.
-const FilePreprocessResultSchema = z.object({
- imageW: z.number({ message: 'the width of the image in PX' }),
- imageH: z.number({ message: 'the height of the image in PX' }),
+const FilePreprocessResultSchema = z.object({
+ imageW: z.number().describe('the width of the image in PX'),
+ imageH: z.number().describe('the height of the image in PX'),
texts: z.array(
z.object({
- left: z.number({
- message: 'left coordinate of the text in PX, in the whole image',
- }),
- top: z.number({
- message: 'top coordinate of the text in PX, in the whole image',
- }),
- width: z.number({ message: 'width of the text in PX' }),
- height: z.number({ message: 'height of the text in PX' }),
- textLines: z.array(z.string(), { message: 'the text lines' }),
- text: z.string({ message: 'concatencated text' }),
- translated: z.string({ message: 'translated text' }),
- comment: z.string({
- message: 'additional comment of the text, or the translation',
- }),
+ left: z.number().describe('left coordinate of the text in PX'),
+ top: z.number().describe('top coordinate of the text in PX'),
+ width: z.number().describe('width of the text in PX'),
+ height: z.number().describe('height of the text in PX'),
+ textLines: z.array(z.string()).describe('the text lines'),
+ text: z.string().describe('concatenated text'),
+ translated: z.string().describe('translated text'),
+ comment: z.string().optional().default('').describe('additional comment'),
}),
),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const FilePreprocessResultSchema = z.object({ | |
| imageW: z.number({ message: 'the width of the image in PX' }), | |
| imageH: z.number({ message: 'the height of the image in PX' }), | |
| texts: z.array( | |
| z.object({ | |
| left: z.number({ | |
| message: 'left coordinate of the text in PX, in the whole image', | |
| }), | |
| top: z.number({ | |
| message: 'top coordinate of the text in PX, in the whole image', | |
| }), | |
| width: z.number({ message: 'width of the text in PX' }), | |
| height: z.number({ message: 'height of the text in PX' }), | |
| textLines: z.array(z.string(), { message: 'the text lines' }), | |
| text: z.string({ message: 'concatencated text' }), | |
| translated: z.string({ message: 'translated text' }), | |
| comment: z.string({ | |
| message: 'additional comment of the text, or the translation', | |
| }), | |
| }), | |
| ), | |
| }); | |
| const FilePreprocessResultSchema = z.object({ | |
| imageW: z.number().describe('the width of the image in PX'), | |
| imageH: z.number().describe('the height of the image in PX'), | |
| texts: z.array( | |
| z.object({ | |
| left: z.number().describe('left coordinate of the text in PX'), | |
| top: z.number().describe('top coordinate of the text in PX'), | |
| width: z.number().describe('width of the text in PX'), | |
| height: z.number().describe('height of the text in PX'), | |
| textLines: z.array(z.string()).describe('the text lines'), | |
| text: z.string().describe('concatenated text'), | |
| translated: z.string().describe('translated text'), | |
| comment: z.string().optional().default('').describe('additional comment'), | |
| }), | |
| ), | |
| }); |
🤖 Prompt for AI Agents
In src/services/ai/llm_preprocess.ts around lines 27 to 48, the Zod schemas are
using an invalid object-literal "message" option; replace those usages by
calling .describe('...') on the schema types (e.g., z.number().describe('...'),
z.string().describe('...'), z.array(...).describe('...')) and make the "comment"
field optional with a sensible default (e.g., z.string().optional().default('')
or z.string().optional()) so validation no longer uses unsupported options and
handles missing comments gracefully.
Comment on lines
+14
to
+18
| if (process.env.NODE_ENV === 'development') { | ||
| // do nothing in dev: vite hot reloading may create APIClient multiple times, | ||
| // causing 401 and an empty token being set | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Vite env flag; current dev guard may never run
process.env.NODE_ENV isn’t defined by Vite by default; use import.meta.env.DEV to ensure this guard works during local dev.
- if (process.env.NODE_ENV === 'development') {
+ if (import.meta.env.DEV) {
// do nothing in dev: vite hot reloading may create APIClient multiple times,
// causing 401 and an empty token being set
return;
}🤖 Prompt for AI Agents
In src/store/user/sagas.ts around lines 14 to 18, the current dev guard uses
process.env.NODE_ENV which Vite does not define by default, so replace that
check with Vite's runtime flag: use import.meta.env.DEV (or guard using (typeof
import_meta !== "undefined" && import.meta.env && import.meta.env.DEV) if your
linter/TS flags require it) to skip the dev-only code path; update the
conditional to check import.meta.env.DEV (or a combined fallback of
process.env.NODE_ENV === 'development' || import.meta.env.DEV) so the guard
reliably runs during local Vite development.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/services/ai/llm_preprocess.ts (2)
1-1: Good fix: correct Zod import.Named import is correct for zod.
30-51: Fix Zod schema: remove unsupported “message” options; make comment optional.Zod ignores the object literal “message” here. Also, comment should be optional with a sensible default.
-const FilePreprocessResultSchema = z.object({ - imageW: z.number({ message: 'the width of the image in PX' }), - imageH: z.number({ message: 'the height of the image in PX' }), +const FilePreprocessResultSchema = z.object({ + imageW: z.number().describe('the width of the image in PX'), + imageH: z.number().describe('the height of the image in PX'), texts: z.array( z.object({ left: z .number() .describe('left coordinate of the text in PX, in the whole image'), top: z .number() .describe('top coordinate of the text in PX, in the whole image'), width: z.number().describe('width of the text in PX'), height: z.number().describe('height of the text in PX'), textLines: z.array(z.string()).describe('the text lines'), text: z.string().describe('concatenated text'), translated: z.string().describe('translated text'), - comment: z - .string() - .describe('additional comment of the text, or the translation'), + comment: z + .string() + .optional() + .default('') + .describe('additional comment of the text, or the translation'), }), ), });src/components/ai/index.tsx (1)
15-21: Wire start() parameters and i18n; avoid ignoring callbacks.start() currently ignores onFileSaved/onConfigured and hardcodes UI text. Wire callbacks and mark i18n.
interface TranslatorApi { start( onFileSaved: (f: MFile) => void, onConfigured?: () => void, ): Promise<void>; testModel(modelConf: LLMConf): Promise<{ worked: boolean; message: string }>; } @@ - async function start() { + async function start( + onFileSaved: (f: MFile) => void, + onConfigured?: () => void, + ) { const llmConf = await new Promise<LLMConf | null>((resolve, reject) => { let confValue: LLMConf | null = null; const onChange = (conf: LLMConf) => { debugLogger('model configured', conf); confValue = conf; if (confValue.model && confValue.baseUrl && confValue.apiKey) { handle.update({ okButtonProps: {} }); } }; const handle = modal.confirm({ icon: null, content: <ModelConfigForm onChange={onChange} />, - okText: `Start translate`, + okText: /* TODO(i18n) */ `Start translate`, okButtonProps: { disabled: true }, onOk: () => { - resolve(confValue); + onConfigured?.(); + resolve(confValue); }, onCancel: () => { resolve(null); }, }); }); @@ - const finished = await new Promise<boolean>((resolve, reject) => { - const handle = modal.confirm({ + modal.confirm({ icon: null, content: ( <BatchTranslateModalContent llmConf={llmConf} files={files} target={target} getHandle={() => handle as ModalHandle} + onFileSaved={onFileSaved} /> ), okButtonProps: { disabled: true }, - onOk: () => { - resolve(true); - }, - onCancel: () => { - resolve(false); - }, + onOk: () => { + debugLogger('batch translate finished'); + }, + onCancel: () => { + debugLogger('batch translate canceled'); + }, - }); - }); + });Also applies to: 31-53, 62-77
src/components/ai/BatchTranslateModal.tsx (1)
140-145: Pass AbortController.signal to LLM call to enable cancelation.Otherwise requests continue after unmount.
- const result = await llmPreprocessFile(llmConf, userMessage).catch( + const result = await llmPreprocessFile( + llmConf, + userMessage, + abort.signal, + ).catch( (e) => { debugLogger('translate failed', e); return null; }, );
🧹 Nitpick comments (3)
src/components/ai/index.tsx (1)
4-5: Avoid deep import for Modal types; rely on public typings.Use the type from Modal.useModal to prevent brittle imports.
-import { ModalStaticFunctions } from 'antd/lib/modal/confirm'; +type ModalFns = ReturnType<typeof Modal.useModal>[0]; @@ -function bind( +function bind( files: MFile[], target: Target, - modal: ModalStaticFunctions, + modal: ModalFns, ): TranslatorApi { @@ - const api = useMemo( - () => bind(files, target, modal as ModalStaticFunctions), + const api = useMemo( + () => bind(files, target, modal),Also applies to: 25-26, 88-91
src/components/ai/BatchTranslateModal.tsx (2)
183-187: Avoid contradictory states when no text blocks; return early.Prevents “saving” then “success: 0 marks” after a “done: no text blocks” message.
async function saveTranslations(f: MFile, r: FilePreprocessResult) { if (r.texts.length === 0) { setFileState(f, 'done: no text blocks', stateIcons.skip); + return; }
206-216: Localize user-facing strings (modal text, status messages).Use react-intl instead of hardcoded English.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ai/BatchTranslateModal.tsx(1 hunks)src/components/ai/ModelConfigForm.tsx(1 hunks)src/components/ai/index.tsx(1 hunks)src/services/ai/llm_preprocess.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ai/ModelConfigForm.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/ai/BatchTranslateModal.tsx (6)
src/utils/debug-logger.ts (1)
createDebugLogger(3-5)src/services/ai/llm_preprocess.ts (3)
LLMConf(7-13)llmPreprocessFile(61-96)FilePreprocessResult(53-53)src/interfaces/target.ts (1)
Target(4-12)src/components/ai/index.tsx (1)
ModalHandle(13-13)src/apis/index.ts (2)
api(289-313)resultTypes(88-95)src/utils/index.ts (1)
toLowerCamelCase(147-147)
src/services/ai/llm_preprocess.ts (1)
src/utils/debug-logger.ts (1)
createDebugLogger(3-5)
src/components/ai/index.tsx (5)
src/utils/debug-logger.ts (1)
createDebugLogger(3-5)src/services/ai/llm_preprocess.ts (1)
LLMConf(7-13)src/interfaces/target.ts (1)
Target(4-12)src/components/ai/ModelConfigForm.tsx (1)
ModelConfigForm(10-163)src/components/ai/BatchTranslateModal.tsx (1)
BatchTranslateModalContent(40-221)
Comment on lines
+40
to
+46
| export const BatchTranslateModalContent: FC<{ | ||
| llmConf: LLMConf; | ||
| files: MFile[]; | ||
| target: Target; | ||
| getHandle(): ModalHandle; | ||
| }> = ({ files, target, getHandle, llmConf }) => { | ||
| const intl = useIntl(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose onFileSaved callback and invoke it after successful save per file.
Matches TranslatorApi contract and enables UI updates.
-export const BatchTranslateModalContent: FC<{
+export const BatchTranslateModalContent: FC<{
llmConf: LLMConf;
files: MFile[];
target: Target;
getHandle(): ModalHandle;
-}> = ({ files, target, getHandle, llmConf }) => {
+ onFileSaved?: (f: MFile) => void;
+}> = ({ files, target, getHandle, llmConf, onFileSaved }) => {
@@
await Promise.all(
r.texts.map((tb) =>
moeflowApiLimiter.use(() => saveTextBlock(f, r, tb)),
),
);
setFileState(
f,
`success: recognized ${r.texts.length} text marks`,
stateIcons.success,
);
+ onFileSaved?.(f);Also applies to: 45-45, 189-199
🤖 Prompt for AI Agents
In src/components/ai/BatchTranslateModal.tsx around lines 40-46 (and also apply
changes near lines 45 and 189-199), add an optional onFileSaved callback to the
component props (e.g., onFileSaved?: (file: MFile) => void), destructure it from
props, and invoke onFileSaved(file) after each file is successfully saved to
match the TranslatorApi contract and allow the UI to update; ensure the prop is
optional, check for its existence before calling, and add appropriate typing and
minor null checks where saves occur in the save loop at 189-199.
Comment on lines
+55
to
+59
| export async function testModel( | ||
| modelConf: LLMConf, | ||
| ): Promise<{ worked: boolean; message: string }> { | ||
| return { worked: true, message: 'test model worked' }; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement a real connectivity check for testModel (avoid always-true stub).
Return actual success/failure using a trivial generation; this prevents false positives in UI.
-export async function testModel(
- modelConf: LLMConf,
-): Promise<{ worked: boolean; message: string }> {
- return { worked: true, message: 'test model worked' };
-}
+export async function testModel(
+ modelConf: LLMConf,
+): Promise<{ worked: boolean; message: string }> {
+ if (!modelConf.model || !modelConf.baseUrl || !modelConf.apiKey) {
+ return { worked: false, message: 'Missing model/baseUrl/apiKey' };
+ }
+ try {
+ const PingSchema = z.object({ ok: z.boolean() });
+ await generateObject({
+ messages: [
+ { role: 'system', content: 'Reply with a JSON object: {"ok": true}.' },
+ { role: 'user', content: 'Return {"ok": true} only.' },
+ ],
+ schema: PingSchema,
+ baseURL: modelConf.baseUrl,
+ model: modelConf.model,
+ apiKey: modelConf.apiKey,
+ });
+ return { worked: true, message: 'Connected' };
+ } catch (e) {
+ const msg = e instanceof Error ? e.message : String(e);
+ return { worked: false, message: msg };
+ }
+}🤖 Prompt for AI Agents
In src/services/ai/llm_preprocess.ts around lines 55 to 59, testModel is a stub
that always returns success; replace it with a real connectivity check that
performs a trivial generation against the configured model: instantiate or fetch
the LLM client using modelConf (reuse the project’s existing LLM client factory
if available), send a short prompt like "ping" or "hello" with a low token limit
and a reasonable timeout, then return worked:true and a concise success message
if a non-empty response is received, or worked:false with the error or failure
reason if the call throws, times out, or returns an empty result; ensure you
catch errors, avoid logging secrets, and surface only safe diagnostic text in
the returned message.
Comment on lines
+85
to
+96
| if (conf.model?.startsWith('gemini-')) { | ||
| debugLogger('gemini workaround: set coords to 1000 scale'); | ||
| ret = { | ||
| ...ret, | ||
| // workaround: gemini returns coords in [0, 1000] scale | ||
| // see https://ai.google.dev/gemini-api/docs/image-understanding | ||
| imageH: 1000, | ||
| imageW: 1000, | ||
| }; | ||
| } | ||
| return res.object; | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: Gemini workaround result is computed but not returned.
You compute ret then return res.object, dropping the fix.
- let ret = res.object;
+ let ret = res.object;
if (conf.model?.startsWith('gemini-')) {
debugLogger('gemini workaround: set coords to 1000 scale');
ret = {
...ret,
// workaround: gemini returns coords in [0, 1000] scale
// see https://ai.google.dev/gemini-api/docs/image-understanding
imageH: 1000,
imageW: 1000,
};
}
- return res.object;
+ return ret;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (conf.model?.startsWith('gemini-')) { | |
| debugLogger('gemini workaround: set coords to 1000 scale'); | |
| ret = { | |
| ...ret, | |
| // workaround: gemini returns coords in [0, 1000] scale | |
| // see https://ai.google.dev/gemini-api/docs/image-understanding | |
| imageH: 1000, | |
| imageW: 1000, | |
| }; | |
| } | |
| return res.object; | |
| } | |
| // initialize return value from the response object | |
| let ret = res.object; | |
| if (conf.model?.startsWith('gemini-')) { | |
| debugLogger('gemini workaround: set coords to 1000 scale'); | |
| ret = { | |
| ...ret, | |
| // workaround: gemini returns coords in [0, 1000] scale | |
| // see https://ai.google.dev/gemini-api/docs/image-understanding | |
| imageH: 1000, | |
| imageW: 1000, | |
| }; | |
| } | |
| // return the possibly adjusted object instead of the original | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In src/services/ai/llm_preprocess.ts around lines 85 to 96, the function builds
a corrected result in the local variable ret when handling gemini models but
then returns res.object, dropping the workaround; change the final return so the
function returns ret (the processed object) instead of res.object, and ensure
ret is initialized/assigned in all code paths so the returned value has the
expected shape/type.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Removal
Chores