Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements first-class OpenAI-compatible video generation as a new model capability in DeepChat. It extends the existing image-generation and TTS frameworks to support Seedance and other video-capable providers through ChangesOpenAI-Compatible Video Generation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentRuntimePresenter/index.ts (1)
3327-3350:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
videoGenerationis missing from persistence methods, causing settings to not be saved.Both
buildPersistedGenerationSettingsPatchandbuildPersistedGenerationSettingsReplacementincludeimageGenerationbut omitvideoGeneration. This means video generation settings will be normalized and applied in-memory but never persisted to the database when updating or replacing session settings.🐛 Proposed fix to add videoGeneration to persistence
if (Object.prototype.hasOwnProperty.call(requestedPatch, 'imageGeneration')) { patch.imageGeneration = sanitized.imageGeneration } + if (Object.prototype.hasOwnProperty.call(requestedPatch, 'videoGeneration')) { + patch.videoGeneration = sanitized.videoGeneration + } return patch } private buildPersistedGenerationSettingsReplacement( settings: SessionGenerationSettings ): Partial<SessionGenerationSettings> { return { systemPrompt: settings.systemPrompt, temperature: settings.temperature, contextLength: settings.contextLength, maxTokens: settings.maxTokens, timeout: settings.timeout, thinkingBudget: settings.thinkingBudget, reasoningEffort: settings.reasoningEffort, reasoningVisibility: settings.reasoningVisibility, verbosity: settings.verbosity, forceInterleavedThinkingCompat: settings.forceInterleavedThinkingCompat, - imageGeneration: settings.imageGeneration + imageGeneration: settings.imageGeneration, + videoGeneration: settings.videoGeneration } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 3327 - 3350, The replacement and patch builders omit videoGeneration so those settings never persist; update both buildPersistedGenerationSettingsReplacement and buildPersistedGenerationSettingsPatch to include the videoGeneration property in their returned Partial<SessionGenerationSettings> objects (mirror how imageGeneration is handled) so normalized videoGeneration is written to the database on update/replace.
🧹 Nitpick comments (2)
src/shared/contracts/common.ts (1)
64-69: ⚖️ Poor tradeoffConsider adding format validation for video parameter strings.
The
seconds,size,ratio, andresolutionfields accept arbitrary strings without format constraints. Depending on provider API requirements, you might want to add validation patterns or enums for expected formats (e.g.,ratio: z.enum(['16:9', '9:16', '4:3'])or regex patterns for resolution like/^\d+x\d+$/).This would catch configuration errors earlier and provide better type safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/common.ts` around lines 64 - 69, The string fields seconds, size, ratio and resolution currently accept any string; update their Zod schemas to enforce expected formats (e.g., use z.enum for known ratios like ratio: z.enum(['16:9','9:16','4:3']) or z.string().regex(/^\d+x\d+$/) for resolution, and appropriate regexes for seconds and size such as numeric or unit-suffixed patterns) so invalid formats are rejected early; modify the schema definitions where seconds, size, ratio and resolution are declared to replace z.string().optional() with the stricter z.enum(...) or z.string().regex(...).optional() variants and keep duration and watermark validation unchanged.src/shared/videoGenerationSettings.ts (1)
284-285: ⚡ Quick winAvoid duplicating model ID prefix checks.
The hardcoded check
modelId.startsWith('doubao-seedance-')duplicates the prefix already defined inVIDEO_GENERATION_MODEL_ID_PREFIXES(line 64). This creates a maintenance burden where changes to the prefix list must be synchronized in two places.Consider checking if the modelId matches any prefix in
FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTSor create a separate constant for flat-top-level model ID patterns to keep the logic DRY.♻️ Suggested refactor
+const FLAT_TOP_LEVEL_VIDEO_MODEL_ID_PREFIXES = ['doubao-seedance-'] as const + const FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS = ['aihubmix'] as constThen update the check:
if ( FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS.some( (hint) => providerId.includes(hint) || providerApiType.includes(hint) || providerKind.includes(hint) || providerOptionsKey.includes(hint) || baseUrl.includes(hint) - ) + ) || + FLAT_TOP_LEVEL_VIDEO_MODEL_ID_PREFIXES.some((prefix) => modelId.startsWith(prefix)) ) { return 'flat-top-level' } - - if (modelId.startsWith('doubao-seedance-')) { - return 'flat-top-level' - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/videoGenerationSettings.ts` around lines 284 - 285, The code currently uses a hardcoded startsWith('doubao-seedance-') check; replace that duplication by checking the modelId against the existing prefix collection instead. Update the condition in the function that references modelId to test whether any prefix in FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS (or, if more appropriate, a new constant e.g. FLAT_TOP_LEVEL_MODEL_ID_PREFIXES) is a prefix of modelId rather than the literal string; use Array.prototype.some(...) over the prefix list or keys to decide the branch so maintenance relies on a single source of truth (referencing VIDEO_GENERATION_MODEL_ID_PREFIXES / FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS and modelId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/presenter/llmProviderPresenter/aiSdk/runtime.ts`:
- Around line 1023-1032: The fetchBinary function is leaking provider
credentials by always adding Authorization and defaultHeaders to requests for
arbitrary task.url; change fetchBinary (and the similar block at the other
location around lines ~1076-1080) to only attach Authorization
(provider.oauthToken || provider.apiKey) and provider-specific defaultHeaders
when the download URL is on the provider’s origin/host (compare new
URL(task.url).host or origin against provider.baseUrl/provider.origin/host),
otherwise send a minimal fetch with no Authorization and only necessary CORS
headers; keep using controller.signal and dispatcher as before (symbols:
fetchBinary, task.url, defaultHeaders, provider.oauthToken, provider.apiKey,
controller.signal, dispatcher).
- Around line 997-1001: The polling helper creates a local AbortController
(controller/timeoutId) but never composes it with a caller-provided AbortSignal,
so caller cancellation doesn't stop the background polling; modify the helper
(the function that builds controller/timeoutId and uses proxyUrl/dispatcher) to
accept an optional caller AbortSignal parameter, wire the caller signal into the
local controller by adding a listener that calls controller.abort() (and remove
the listener during cleanup), ensure the local timeout still aborts as before,
pass controller.signal to all fetch/stream calls, clear the timeoutId and remove
the listener when finished, and apply the same change to the analogous block
around lines 1066-1073.
In `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts`:
- Around line 1714-1718: The branch that classifies models as
ModelType.VideoGeneration (the normalizedRawType checks around
ModelType.VideoGeneration) is not mirrored in the provider config sync in
fetchNewApiModels(), which still maps non-image/TTS models to
ApiEndpointType.Chat; update fetchNewApiModels() (the provider-managed config
sync) to detect video-generation models using the same
normalizedRawType/supportedEndpointTypes logic and set apiEndpoint to
'video-generation' (or the corresponding ApiEndpointType) when assigning model
metadata so persisted models have matching type, endpointType, and apiEndpoint.
In `@src/renderer/src/components/message/MessageBlockVideo.vue`:
- Around line 17-18: videoError in MessageBlockVideo.vue remains true after a
single failure and isn't cleared when the video source changes; add logic to
reset it whenever a new source is set or successfully loads. In
MessageBlockVideo.vue, reset the data property videoError to false when the
bound src prop changes (add a watcher for the prop used for :src) and/or clear
videoError on a successful load event (e.g., handle `@loadedmetadata` or `@canplay`
to set videoError = false); ensure the existing `@error` listener still sets
videoError = true. Update any methods that swap sources to also set videoError =
false before switching so stale error UI can't appear after a successful source
change.
- Around line 42-58: The hardcoded English fallbacks in keyMap cause user-facing
strings to bypass i18n; remove those literal values and make the code return the
i18n keys when no translation exists. Specifically, remove or change keyMap
entries ('keyMap') so they do not contain "Video" or "Request failed", ensure
the i18n fallback in the immediately-invoked function that returns useI18n().t
(symbol: i18n) returns the input key (identity) on error, and update
translate(key) to return the key when translated === key (instead of
keyMap[key]) so rendering remains fully key-based and driven by vue-i18n
(symbols: translate, i18n, useI18n).
- Around line 28-30: The current v-else branch in MessageBlockVideo.vue always
renders a loading spinner (Icon "lucide:loader-2") when no media data exists,
causing an endless loader for failed/cancelled blocks; update the render logic
to check the block/message terminal state (e.g., a status prop like "failed" or
"cancelled" or message.status) before showing the spinner and render a terminal
view instead—use a failure icon/text and optional retry/cancel UI when status is
failed/cancelled, otherwise keep the spinner for in-progress states; locate the
conditional around the loader (the v-else that renders Icon) and replace it with
an explicit status check that chooses between loading, failed/cancelled, and
empty states.
In `@src/renderer/src/components/message/MessageItemAssistant.vue`:
- Around line 246-258: isVideoBlock currently only looks at block.image_data and
uses includes() on URLs which can false-positive; update isVideoBlock (operating
on DisplayAssistantMessageBlock / MessageBlockVideo) to also check legacy
payloads like block.content for mimeType, data, or url, and when inspecting URL
paths convert to lowercase and check the pathname or filename (not the whole
URL) for video extensions using VIDEO_EXTENSIONS with a strict endsWith or exact
extension match rather than includes; also preserve the existing mimeType/data
checks (data:video/ and video/*) and ensure null-safe access to both image_data
and content fields.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 466-467: Several newly added strings in the Danish locale remain
in English; update the JSON values for the keys such as "video",
"videoGeneration" and the API-related option labels ("apiEndpoint", "apiType" or
equivalent keys used for endpoint/type options) plus the other recently added
entries so they are translated to Danish, keeping the original JSON keys intact
and matching the style/phrasing used elsewhere in the da-DK file to ensure a
consistent Danish UI.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Line 533: The Persian locale file contains new English strings for the
video-generation UI (e.g., the "video" key currently set to "Video Generation"
and other keys added around the same area at lines noted), so replace those
English values with their Persian translations for the corresponding keys
(update the "video" key and all newly added video-generation keys between the
same block — the group around lines 547 and 615–644) so the settings UI is fully
localized; locate the keys in src/renderer/src/i18n/fa-IR/settings.json (search
for "video" and the surrounding new keys) and provide proper Persian
translations for each user-facing string.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Line 533: The French locale file still contains English values for the
video-generation UI labels — replace the English strings with French
translations for the JSON keys in that section (e.g., "video": "Video
Generation" and the other keys in the same block around lines 533, 547 and
615-644); do not change the property names (keys), only update their values to
idiomatic French (e.g., "Video Generation" → "Génération de vidéo" and similarly
translate placeholders/descriptions for the related keys) and keep JSON syntax
intact.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Line 533: The Hebrew locale file src/renderer/src/i18n/he-IL/settings.json
contains new English strings (e.g., the "video" key with value "Video
Generation" and other user-facing strings added around the same region) and must
be replaced with proper Hebrew translations; locate the affected keys ("video"
and the contiguous keys introduced at the same block around lines 547 and
615-644) and substitute each English value with an accurate Hebrew translation,
preserving any placeholders, punctuation, and JSON quoting/escaping exactly as
in the originals so keys like "video" and the related setting labels/tooltips
remain intact and properly localized.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Line 533: The ja-JP settings file contains English text in the Video
Generation section (the "video" key and its neighboring video-generation keys);
replace those English values with appropriate Japanese translations so the
settings panel is fully localized (update the "video" value and all adjacent
video-generation string values that mirror the en-US keys), keeping the JSON
keys identical to the source language and preserving encoding/escaping.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Line 533: The ko-KR settings JSON contains English labels that must be
localized: replace the value for the "video" key (currently "Video Generation")
and any other English strings found in the same file ranges (around the keys
present at the blocks near lines 547 and 615–644) with their proper Korean
translations, preserving the JSON keys and punctuation; update only the string
values so the UI no longer mixes English and Korean.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Line 533: The pt-BR locale still contains English strings in the
video-generation block: replace the value for the "video" key ("Video
Generation") and all other English strings in the same block (the
video-generation section around the subsequent entries) with correct Portuguese
(pt-BR) translations, keeping the JSON keys unchanged and preserving
punctuation/formatting; locate the block by the "video" key and the surrounding
video-generation entries and update their values to proper pt-BR text.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Line 533: The ru-RU locale file contains English text for the new
video-generation entries (e.g., the "video" key currently set to "Video
Generation" and other recently added keys around that block); update the string
values for those keys to their correct Russian translations while leaving the
JSON keys intact, preserving any placeholders/formatting (quotes, interpolation
tokens) and punctuation; locate the same block of keys near "video" and the
adjacent entries that were added (the other strings mentioned in the review) and
replace their English values with Russian equivalents, validate the JSON syntax
after edits.
In `@test/main/presenter/llmProviderPresenter/aiSdkRuntime.test.ts`:
- Around line 561-663: The tests in aiSdkRuntime.test.ts are waiting real time
between mocked poll responses because the runAiSdkCoreStream poll loop uses
VIDEO_GENERATION_POLL_INTERVAL_MS; replace the real wait by installing fake
timers (e.g., vi.useFakeTimers()/jest.useFakeTimers()) or stubbing the internal
delay used by runAiSdkCoreStream so the submitted->completed transition is
advanced deterministically (advance timers or resolve the stubbed delay) before
collecting events; locate the test block using runAiSdkCoreStream and references
to VIDEO_GENERATION_POLL_INTERVAL_MS and ensure timers are restored/cleared
after the test to avoid cross-test pollution.
---
Outside diff comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 3327-3350: The replacement and patch builders omit videoGeneration
so those settings never persist; update both
buildPersistedGenerationSettingsReplacement and
buildPersistedGenerationSettingsPatch to include the videoGeneration property in
their returned Partial<SessionGenerationSettings> objects (mirror how
imageGeneration is handled) so normalized videoGeneration is written to the
database on update/replace.
---
Nitpick comments:
In `@src/shared/contracts/common.ts`:
- Around line 64-69: The string fields seconds, size, ratio and resolution
currently accept any string; update their Zod schemas to enforce expected
formats (e.g., use z.enum for known ratios like ratio:
z.enum(['16:9','9:16','4:3']) or z.string().regex(/^\d+x\d+$/) for resolution,
and appropriate regexes for seconds and size such as numeric or unit-suffixed
patterns) so invalid formats are rejected early; modify the schema definitions
where seconds, size, ratio and resolution are declared to replace
z.string().optional() with the stricter z.enum(...) or
z.string().regex(...).optional() variants and keep duration and watermark
validation unchanged.
In `@src/shared/videoGenerationSettings.ts`:
- Around line 284-285: The code currently uses a hardcoded
startsWith('doubao-seedance-') check; replace that duplication by checking the
modelId against the existing prefix collection instead. Update the condition in
the function that references modelId to test whether any prefix in
FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS (or, if more appropriate, a new constant
e.g. FLAT_TOP_LEVEL_MODEL_ID_PREFIXES) is a prefix of modelId rather than the
literal string; use Array.prototype.some(...) over the prefix list or keys to
decide the branch so maintenance relies on a single source of truth (referencing
VIDEO_GENERATION_MODEL_ID_PREFIXES / FLAT_TOP_LEVEL_VIDEO_PROVIDER_HINTS and
modelId).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16793bd6-fa59-4351-81ea-33a8e9de5876
📒 Files selected for processing (62)
docs/features/openai-compatible-video-generation/plan.mddocs/features/openai-compatible-video-generation/spec.mddocs/features/openai-compatible-video-generation/tasks.mddocs/issues/openai-compatible-video-prompt-duration-fallback/plan.mddocs/issues/openai-compatible-video-prompt-duration-fallback/spec.mddocs/issues/openai-compatible-video-prompt-duration-fallback/tasks.mdsrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/configPresenter/providerModelHelper.tssrc/main/presenter/index.tssrc/main/presenter/llmProviderPresenter/aiSdk/runtime.tssrc/main/presenter/llmProviderPresenter/index.tssrc/main/presenter/llmProviderPresenter/providers/aiSdkProvider.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/renderer/settings/components/ProviderModelList.vuesrc/renderer/src/components/ChatConfig.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/components/message/MessageBlockVideo.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/components/settings/OpenAIVideoGenerationSettingsFields.vuesrc/renderer/src/composables/useModelTypeDetection.tssrc/renderer/src/i18n/da-DK/model.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/model.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/model.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/model.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/model.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/model.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/model.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/model.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/model.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/model.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/model.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/model.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/modelStore.tssrc/renderer/src/stores/ui/draft.tssrc/shared/contracts/common.tssrc/shared/contracts/domainSchemas.tssrc/shared/model.tssrc/shared/types/agent-interface.d.tssrc/shared/types/model-db.tssrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/llmprovider.presenter.d.tssrc/shared/videoGenerationSettings.tstest/main/presenter/llmProviderPresenter/aiSdkRuntime.test.tstest/main/presenter/llmProviderPresenter/aihubmixProvider.test.ts
9 tasks
* fix(agent): bypass chat budget for image routes (#1636) * Initial plan * merge(gen-video): resolve agent runtime conflict Agent-Logs-Url: https://github.com/ThinkInAIXYZ/deepchat/sessions/7c9b17d6-272d-44a2-ab2d-57a554c773ab Co-authored-by: zhangmo8 <43628500+zhangmo8@users.noreply.github.com> --------- Co-authored-by: duskzhen <zerob13@gmail.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
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
2 participants
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.
link #1237
Summary by CodeRabbit
New Features
Localization