fix(pingxingshijie): enforce asset upload model - #4578
Closed
Fankouzu wants to merge 66 commits into
Closed
Conversation
Updated the AliVideoInput structure to include a new Media field for PixVerse media assets. Introduced the AliMedia type to handle different media types. Adjusted resolution options to support 360P and 540P. Implemented logic for PixVerse model handling in the billing calculation, ensuring accurate pricing based on model and audio settings. This enhancement improves the adaptability of the Ali adaptor for various video generation scenarios.
feat(AliAdaptor): enhance video input structure and add PixVerse support
Made-with: Cursor
…-latest Chore/bump-lobehub-icons-visactor-latest
Content metadata items with type draft_task include a nested draft_task.id field for Seedance draft-to-video. The previous ContentItem struct dropped this field on JSON round-trip; add DraftTaskRef so upstream receives it. Made-with: Cursor
…ontent fix(doubao): forward Seedance draft_task content to Volcengine
Ark rejects draft_task mixed with other content types; the adaptor always appended prompt as a text item. Omit that append when metadata.content includes draft_task. Made-with: Cursor
…-mix fix(doubao): avoid mixing draft_task with prompt text for Seedance
…td; axios 1.15.0) Made-with: Cursor
…ubao) - Add ChannelTypePingXingShiJie (58) with default Ark base URL - New relay/channel/task/pingxingshijie package; register in GetTaskAdaptor - Admin UI: CHANNEL_OPTIONS label 平行视界, icon same as Doubao video - Channel test: unsupported like other async video task channels Made-with: Cursor
…sync flows - Route video/image/asset through PingXingShiJie base URL and unified envelope - Add upstream_kind on tasks, FetchTask branching, and asset POST /v2/asset/status polling - Extend /v1 routes for async images and assets; distributor and RelayTask fetch handling - Add docs: API reference, compatibility, downstream curl pointer; ignore local vendor API md Made-with: Cursor
…:task_id
Path2RelayMode maps any /v1/images/generations prefix to RelayModeImagesGenerations,
which is not registered in fetchRespBuilders, causing a nil respBuilder panic on
RelayTaskFetch. Prefer c.Get("relay_mode") when set so VideoFetchByID applies.
Also guard against missing fetch builder and add optional debug NDJSON logs to
.cursor/debug-1b0c95.log for verification.
Made-with: Cursor
…edream Made-with: Cursor
…ge preview Made-with: Cursor
…AIVideo + url fallback Made-with: Cursor
… upstream field Made-with: Cursor
…sted data (draft upscale) Made-with: Cursor
…nd task id parsing - Forward minimal upstream body for draft_task (no default generate_audio, no seconds->duration) - Normalize 720p/1080p for seedance-1-5-pro; clear metadata draft flag when present - Deep-extract video create task id from nested envelope/data-string responses - Document draft_task downstream format; add contract tests Made-with: Cursor
fix(pingxingshijie): Seedance 1.5 Pro draft_task upscale and video task id parsing
…nvelope, enable channel test Made-with: Cursor
… text relay Made-with: Cursor
…completions and enhance envelope handling - Changed upstream URL for text chat from /v1/chat/completions to /v2/chat/completions to align with API family. - Improved response handling by normalizing PingXing envelope for error codes and messages. - Updated documentation to reflect changes in API behavior and response structure.
…el-test fix(pingxingshijie): channel 58 text relay uses /v2/chat/completions
Sync upstream changes into main after resolving conflicts and preserving local customizations. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Bring GitHub main's Pingxingshijie channel updates onto the upstream-synced main before pushing. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
fix(ui): add default channel logos for PingXingShiJie and KieAI
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
feat(logs): show task media results in default UI
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two new channels (PingXingShiJie: 58, KieAI: 59) with adaptors, model registrations, async image/video/asset endpoints and converters, upstream-envelope normalization, upstream_kind propagation, result URL extraction, middleware/router/frontend wiring, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Gateway
participant Relay
participant Adaptor
participant Upstream
participant DB
Client->>Gateway: POST /v1/images/generations/async (OpenAI-shape)
Gateway->>Relay: RelayTask submit (model, metadata, relay_mode)
Relay->>Adaptor: BuildRequestURL/Body/Header (includes upstream_kind)
Adaptor->>Upstream: HTTP Create Task (v1/v2 endpoint)
Upstream-->>Adaptor: 200 with envelope (code/data/taskId)
Adaptor->>Relay: DoResponse (unwrap envelope, return task id & data)
Relay->>DB: Persist Task (PrivateData.UpstreamKind, ResultURL/taskId)
Client->>Gateway: GET /v1/images/generations/:task_id (poll)
Gateway->>Relay: RelayTaskFetch (includes upstream_kind)
Relay->>Adaptor: FetchTask (routes by upstream_kind)
Adaptor->>Upstream: Poll status
Upstream-->>Adaptor: status/data
Adaptor->>Relay: ConvertToOpenAIAsyncImage/Asset/Video (normalize)
Relay->>Gateway: Return OpenAI-shaped task status to Client
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/model.go (1)
84-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister
taskpxsj.ModelListinopenAIModelsas well.This block adds
taskkie.ModelListtoopenAIModels, but PingXingShiJie is only added tochannelId2Models. That leavesopenAIModelsMapunaware ofpingxingshijie-asset, soGET /v1/models/pingxingshijie-assetstill falls through tomodel_not_found, and list metadata degrades toOwnedBy: "custom"instead of the channel name when the model is enabled.💡 Suggested fix
+ for _, modelName := range taskpxsj.ModelList { + openAIModels = append(openAIModels, dto.OpenAIModels{ + Id: modelName, + Object: "model", + Created: 1626777600, + OwnedBy: taskpxsj.ChannelName, + }) + } for _, modelName := range taskkie.ModelList { openAIModels = append(openAIModels, dto.OpenAIModels{ Id: modelName, Object: "model", Created: 1626777600, OwnedBy: taskkie.ChannelName, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/model.go` around lines 84 - 113, The code populates openAIModels with entries from taskkie.ModelList but never adds taskpxsj.ModelList, so models like "pingxingshijie-asset" are missing from openAIModels and thus openAIModelsMap; update the block that builds openAIModels to also append dto.OpenAIModels entries for each modelName in taskpxsj.ModelList (similar to the existing loop for taskkie.ModelList), using Object:"model", Created:1626777600 and OwnedBy: taskpxsj.ChannelName, so openAIModels, openAIModelsMap and channelId2Models remain consistent for constant.ChannelTypePingXingShiJie handling.
🧹 Nitpick comments (6)
docs/token-management-api-reference.md (1)
76-76: ⚡ Quick winAdd language specifiers to HTTP endpoint code blocks.
The HTTP endpoint declarations are missing language specifiers, which triggers markdownlint warnings and prevents proper syntax highlighting.
📝 Proposed fix
Add
httpafter the opening backticks for all endpoint declarations. For example:-``` +```http GET /api/token/Apply this pattern to all 11 endpoint blocks at lines 76, 136, 197, 256, 294, 390, 520, 560, 609, 671, and 743. </details> Also applies to: 136-136, 197-197, 256-256, 294-294, 390-390, 520-520, 560-560, 609-609, 671-671, 743-743 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/token-management-api-reference.mdat line 76, Several HTTP endpoint
code blocks (e.g., the block starting with "GET /api/token/") are missing
language specifiers which causes markdownlint warnings; open each endpoint's
fenced code block and add the language tag "http" immediately after the three
backticks (e.g., changetohttp) for all listed endpoint blocks (lines
referenced in the comment include the blocks for GET /api/token/ and the other
10 endpoint declarations). Ensure you update every endpoint code fence so all 11
HTTP endpoint blocks use ```http for proper syntax highlighting and linting.</details> </blockquote></details> <details> <summary>controller/model_list_test.go (1)</summary><blockquote> `214-220`: _⚡ Quick win_ **Make this test fail fast when the source model list is empty.** Right now the loop can pass with zero iterations. Add an explicit non-empty assertion so registry wiring regressions don’t go unnoticed. <details> <summary>♻️ Proposed update</summary> ```diff func TestKieModelsAppearInGlobalModelRegistry(t *testing.T) { + require.NotEmpty(t, taskkie.ModelList, "taskkie.ModelList should not be empty") for _, modelName := range taskkie.ModelList { aiModel, ok := openAIModelsMap[modelName] require.True(t, ok, "missing Kie model %s", modelName) require.Equal(t, taskkie.ChannelName, aiModel.OwnedBy) } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@controller/model_list_test.go` around lines 214 - 220, The test TestKieModelsAppearInGlobalModelRegistry currently iterates over taskkie.ModelList but will silently succeed if that slice is empty; add a precondition assertion (e.g., require.NotEmpty with t and taskkie.ModelList) before the for-loop to fail fast when the source model list is empty, then keep the existing loop that checks openAIModelsMap entries and aiModel.OwnedBy. ``` </details> </blockquote></details> <details> <summary>web/default/src/features/channels/lib/channel-utils.ts (1)</summary><blockquote> `86-87`: _⚡ Quick win_ **Clarify the helper contract now that it returns URLs too.** Line 86 and Line 87 return raw URLs, while this helper is documented as returning icon *names*. Please align the docs/name to avoid misuse in future call sites. <details> <summary>♻️ Proposed update</summary> ```diff -/** - * Get channel type icon name for getLobeIcon - * Maps channel types to Lobe icon names using type number (language-independent) - */ +/** + * Get channel type icon identifier for UI. + * Returns either a Lobe icon name or a remote icon URL. + */ export function getChannelTypeIcon(type: number): string { - const TYPE_TO_ICON: Record<number, string> = { + const TYPE_TO_ICON_OR_URL: Record<number, string> = { @@ - return TYPE_TO_ICON[type] || 'OpenAI' + return TYPE_TO_ICON_OR_URL[type] || 'OpenAI' } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@web/default/src/features/channels/lib/channel-utils.ts` around lines 86 - 87, The helper in web/default/src/features/channels/lib/channel-utils.ts currently returns raw icon URLs while its docs and name indicate it returns icon "names"; update the helper contract so it is unambiguous: either (A) change the function name and JSDoc to reflect it returns icon URLs (e.g., rename to getIconUrl / update return type to string URL and update callers/tests), or (B) preserve the “name” contract by converting those URL entries (like 'https://www.pingxingshijie.cn/favicon.ico' and 'https://kie.ai/logo.png') to icon identifiers and return only names (and map names to URLs in a separate resolver). Make the chosen change consistently in the function signature, exported type, JSDoc, and any callers in the repo. ``` </details> </blockquote></details> <details> <summary>model/task_result_url.go (1)</summary><blockquote> `3-8`: **The current code complies with JSON wrapper guidelines; removing `encoding/json` is optional.** The file correctly uses `common.Unmarshal()` instead of direct `encoding/json` calls. The `encoding/json` import is present only because the function accepts `json.RawMessage` as a parameter type. According to the coding guidelines, `json.RawMessage` and `json.Number` may still be referenced as types, making this import and type usage compliant with the JSON wrapper rule. Replacing `json.RawMessage` with `[]byte` is a valid stylistic refactor (the types are equivalent at runtime), but it is not required by the documented guidelines. The suggestion assumes a stricter interpretation that goes beyond the stated exception. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@model/task_result_url.go` around lines 3 - 8, The import of encoding/json and use of the json.RawMessage type in model/task_result_url.go is acceptable because the function parameter uses json.RawMessage and the code uses common.Unmarshal rather than direct json calls; no change required—leave the import and the parameter type as-is, or if you prefer a stylistic refactor you may change the parameter to []byte and remove the encoding/json import, but do not modify the common.Unmarshal usage (refer to json.RawMessage and common.Unmarshal in the function signature to locate the code). ``` </details> </blockquote></details> <details> <summary>web/default/src/features/usage-logs/components/dialogs/task-media-results-dialog.tsx (1)</summary><blockquote> `23-27`: _💤 Low value_ **Consider adding error handling for clipboard API.** The clipboard API can fail (e.g., when permissions are denied or in insecure contexts). While not critical, catching the rejection would prevent silent failures. <details> <summary>♻️ Proposed enhancement</summary> ```diff function copyUrl(url: string, successMessage: string): void { - void navigator.clipboard.writeText(url).then(() => { - toast.success(successMessage) - }) + void navigator.clipboard.writeText(url) + .then(() => toast.success(successMessage)) + .catch(() => toast.error('Failed to copy')) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@web/default/src/features/usage-logs/components/dialogs/task-media-results-dialog.tsx` around lines 23 - 27, The copyUrl function currently writes to navigator.clipboard without handling failures; update copyUrl(url: string, successMessage: string) to catch promise rejections from navigator.clipboard.writeText and surface an error (e.g., toast.error with a helpful message) so clipboard permission/secure-context failures are not silent; keep the success toast on then() and add a .catch() (or try/catch if you convert to async/await) to log or toast the error and optionally console.error the caught error for debugging. ``` </details> </blockquote></details> <details> <summary>relay/channel/task/kie/adaptor.go (1)</summary><blockquote> `312-328`: _💤 Low value_ **Consider handling the case where `cfg.ImageKey` is empty for unknown models.** When `modelName` doesn't match `ModelSeedance2` or `ModelHappyHorseImageToVideo`, and `cfg.ImageKey` is empty, images are silently dropped. This may be intentional, but if a new model is added without an image key config, image inputs would be lost without any warning. <details> <summary>🔧 Optional: Add debug logging for dropped images</summary> ```diff default: if cfg.ImageKey != "" { input[cfg.ImageKey] = images + } else if len(images) > 0 { + // Consider logging that images were provided but model has no image key configured } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/kie/adaptor.go` around lines 312 - 328, When images are present but fall through the switch (i.e., modelName is not ModelSeedance2 or ModelHappyHorseImageToVideo) and getModelConfig(modelName).ImageKey is empty, explicitly handle that case instead of silently dropping images: update the block in the function that calls getModelConfig, requestImages and builds input to detect images > 0 && cfg.ImageKey == "" and then either log a warning (including modelName and number of images) or return an error so callers know images were ignored; reference cfg.ImageKey, ModelSeedance2, ModelHappyHorseImageToVideo, requestImages, getModelConfig and createTaskRequest to locate where to add the check. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@controller/channel-test.go:
- Around line 125-128: When forcing the PingXingShiJie image test path you set
requestPath but forgot to set endpointType, causing buildTestRequest(...) to
produce a non-image request; update the branch that checks channel.Type ==
constant.ChannelTypePingXingShiJie &&
strings.Contains(strings.ToLower(testModel), "seedream") to also set
endpointType to the image type used by your request (e.g., the image/images
enum/value your code expects) so buildTestRequest(...) builds an image request
and image conversion won't fail.In
@dto/openai_request.go:
- Around line 395-397: Change the conditional that sets a fallback MIME so it
does not depend on file.FileName: whenever mimeType == "" and there is a file
payload, set mimeType = "application/octet-stream" regardless of file.FileName.
Locate the logic that currently checks mimeType == "" && file.FileName != ""
(using the mimeType and file.FileName variables) and remove the file.FileName
check so the fallback is unconditional for empty mimeType on file payloads.In
@middleware/distributor_test.go:
- Around line 32-39: Update TestGetModelRequestAssetUploadRejectsBlankModel to
also exercise the explicit empty-string model case by calling newJSONContext
with a body that includes"model":""(e.g.,
{"model":"","image_url":"https://example.com/a.jpg","asset_type":"Image"}),
call getModelRequest for that context, assert err is non-nil, and assert the
returned error message contains the expected placeholder constant
taskpxsj.AssetPlaceholderModel; keep the existing missing-model case as well so
both scenarios are covered.In
@model/task_result_url.go:
- Around line 37-55: walkFirstImageLikeURL currently iterates a map
(map[string]any) which yields nondeterministic order; change the map descent in
walkFirstImageLikeURL so keys are collected, sorted (e.g., lexicographically),
and then iterated in that sorted order before recursing into values so the
"first" image-like url is stable across runs; keep the existing url check
(x["url"]) and recursion logic but replace the direct range over the map with a
deterministic sorted-key loop.In
@model/task.go:
- Around line 137-146: GetResultURL currently returns t.FailReason verbatim when
PrivateData.ResultURL is empty, which can leak data: base64 payloads; change the
logic in GetResultURL so that after selecting u := t.PrivateData.ResultURL or u
= t.FailReason, you validate u with isVideoProxyContentURL(u, t.TaskID) and if
that function indicates a disallowed data: URL, return an empty string instead
of u; keep references to t.PrivateData.ResultURL, t.FailReason,
isVideoProxyContentURL, and TaskID so reviewers can find and update the
GetResultURL implementation accordingly.In
@relay/channel/task/doubao/adaptor.go:
- Around line 303-308: When contentHasDraftTask(r.Content) is true, don’t just
remove "text" items—enforce exclusivity by keeping only items where Type ==
"draft_task" and removing everything else (filter r.Content to retain only
draft_task entries); update the block in convertToRequestPayload that currently
uses lo.Reject on "text" and make the same change in the analogous later block
(the second contentHasDraftTask branch) so both places only preserve draft_task
items.In
@relay/channel/task/pingxingshijie/asset_model_validation_test.go:
- Around line 44-52: Update the test
TestValidateRequestAndSetActionRejectsBlankAssetUploadModel to also exercise an
explicit empty model value: create a second context (using
newAssetUploadContext) whose payload includes"model":""(e.g.
{"image_url":"https://...","asset_type":"Image","model":""}) and call
(&TaskAdaptor{}).ValidateRequestAndSetAction with that context and the same
info, asserting that the returned error is non-nil; this ensures both missing
and explicitly-blank model cases are covered.In
@relay/relay_task_test.go:
- Around line 10-18: The test
TestApplyOtherRatiosToQuotaMultipliesBeforeTruncating is flaky because it uses a
map (ratios) whose iteration order can change and the chosen fractions land near
an integer boundary; update the test(s) that call applyOtherRatiosToQuota (also
the similar cases around lines 21-34) to use values that are well clear of
truncation boundaries (so any multiplication order yields the same integer) or
replace the map with an ordered helper that tests both multiplication orders
explicitly; reference the test name
TestApplyOtherRatiosToQuotaMultipliesBeforeTruncating and the function
applyOtherRatiosToQuota when making the change.In
@service/file_service.go:
- Around line 352-359: The current MIME-sniffing in the block using
http.DetectContentType ignores the case where DetectContentType returns
"application/octet-stream" (a generic unknown) so valid files stay
misclassified; update the condition in the block that inspects sniffed (from
http.DetectContentType(decodedData)) to treat "" and "application/octet-stream"
as unknown and only set mimeType when sniffed is non-empty and not
"application/octet-stream" (still stripping any ";charset=..." via strings.Index
as done now). Locate the mimeType variable assignment and the
http.DetectContentType call in this function and change the condition
accordingly so application/octet-stream does not get accepted as a real MIME
type.In
@service/task_polling.go:
- Around line 362-369: FetchTask calls currently only pass baseURL, key, body,
proxy so polling drops per-task resolved header overrides; update the FetchTask
interface and all adaptor.FetchTask implementations to accept and forward the
resolved header overrides and then populate the fetchBody with
task.PrivateData.ResolvedHeaderOverride (falling back to existing derivation
only if empty), similar to how task.PrivateData.Key is preferred; ensure the new
parameter is threaded through callers and that fetchBody includes
"resolved_header_override" (or equivalent key used by downstream) so follow-up
polls reuse submit-time auth context and do not re-derive headers.In
@web/classic/package.json:
- Around line 10-12: The package.json lists incompatible chart packages:
@visactor/vchart-semi-theme@1.12.3 declares a peerDependency for
@visactor/vchart1.x while the project depends on@visactor/vchart@2.0.21 and
@visactor/react-vchart@2.0.21; resolve by either upgrading
@visactor/vchart-semi-themeto a release that supports vchart 2.x (replace the
dependency entry for "@visactor/vchart-semi-theme" with the compatible version)
or by downgrading "@visactor/vchart" and "@visactor/react-vchart" to matching
1.x versions (update both entries) and ensure package-lock/yarn.lock is
regenerated and CI installs succeed.In
@web/classic/src/components/table/task-logs/modals/ContentModal.jsx:
- Around line 39-48: The effect that resets preview state only depends on
isModalOpen, isVideo, and isImage, so it won't rerun when the parent swaps the
URL (modalContent) while the modal remains open; update the dependency array of
the useEffect that sets setVideoError(false), setImageError(false), and
setIsLoading(true) to also include modalContent (or the prop that carries the
new URL) so the effect runs whenever modalContent changes and the preview state
is properly reset when a new asset is loaded.In
@web/default/src/features/usage-logs/lib/task-action-label.ts:
- Around line 9-14: The function in task-action-label.ts currently returns
hardcoded user-facing strings for log.upstream_kind ('asset' -> 'Asset Upload',
'image' -> 'Image Generation'); replace those literal returns with stable i18n
keys (e.g. 'usage.task.assetUpload', 'usage.task.imageGeneration') so the
function returns keys only, and update consuming components to call
useTranslation() and t(returnedKey) at render time; update any tests/fixtures to
expect keys instead of English text and add the new keys to the locale files.In
@web/default/src/features/usage-logs/lib/task-media-results.ts:
- Around line 117-121: extractTaskMediaResults currently seeds a task-proxy
result (using isTaskVideoProxyUrl) before calling walkTaskData, which lets a
stale proxy win even when a real image exists inside data; change the logic to
treat proxy entries as fallbacks: either (A) do not push the seeded proxy into
results up-front — instead keep it in a local fallbackCandidate and only append
it after walkTaskData if no concrete image result was produced, or (B) after
walkTaskData completes, scan results and remove any entries whose URL is a
task-proxy (isTaskVideoProxyUrl(url, source)) whenever a concrete image result
exists; apply the same change to the analogous block referenced at lines 183-185
so proxy fallbacks cannot override real images.- Around line 68-74: Update the image-proxy heuristic to treat upstream_kind ===
'asset' the same as 'image': in isStaleImageProxyUrl (and the other predicate
that similarly checks source.upstream_kind for image/video heuristics later in
the file), include source.upstream_kind === 'asset' alongside 'image' so asset
uploads use the image-path logic and won't be misclassified as video or skipped.
Outside diff comments:
In@controller/model.go:
- Around line 84-113: The code populates openAIModels with entries from
taskkie.ModelList but never adds taskpxsj.ModelList, so models like
"pingxingshijie-asset" are missing from openAIModels and thus openAIModelsMap;
update the block that builds openAIModels to also append dto.OpenAIModels
entries for each modelName in taskpxsj.ModelList (similar to the existing loop
for taskkie.ModelList), using Object:"model", Created:1626777600 and OwnedBy:
taskpxsj.ChannelName, so openAIModels, openAIModelsMap and channelId2Models
remain consistent for constant.ChannelTypePingXingShiJie handling.
Nitpick comments:
In@controller/model_list_test.go:
- Around line 214-220: The test TestKieModelsAppearInGlobalModelRegistry
currently iterates over taskkie.ModelList but will silently succeed if that
slice is empty; add a precondition assertion (e.g., require.NotEmpty with t and
taskkie.ModelList) before the for-loop to fail fast when the source model list
is empty, then keep the existing loop that checks openAIModelsMap entries and
aiModel.OwnedBy.In
@docs/token-management-api-reference.md:
- Line 76: Several HTTP endpoint code blocks (e.g., the block starting with "GET
/api/token/") are missing language specifiers which causes markdownlint
warnings; open each endpoint's fenced code block and add the language tag "http"
immediately after the three backticks (e.g., changetohttp) for all
listed endpoint blocks (lines referenced in the comment include the blocks for
GET /api/token/ and the other 10 endpoint declarations). Ensure you update every
endpoint code fence so all 11 HTTP endpoint blocks use ```http for proper syntax
highlighting and linting.In
@model/task_result_url.go:
- Around line 3-8: The import of encoding/json and use of the json.RawMessage
type in model/task_result_url.go is acceptable because the function parameter
uses json.RawMessage and the code uses common.Unmarshal rather than direct json
calls; no change required—leave the import and the parameter type as-is, or if
you prefer a stylistic refactor you may change the parameter to []byte and
remove the encoding/json import, but do not modify the common.Unmarshal usage
(refer to json.RawMessage and common.Unmarshal in the function signature to
locate the code).In
@relay/channel/task/kie/adaptor.go:
- Around line 312-328: When images are present but fall through the switch
(i.e., modelName is not ModelSeedance2 or ModelHappyHorseImageToVideo) and
getModelConfig(modelName).ImageKey is empty, explicitly handle that case instead
of silently dropping images: update the block in the function that calls
getModelConfig, requestImages and builds input to detect images > 0 &&
cfg.ImageKey == "" and then either log a warning (including modelName and number
of images) or return an error so callers know images were ignored; reference
cfg.ImageKey, ModelSeedance2, ModelHappyHorseImageToVideo, requestImages,
getModelConfig and createTaskRequest to locate where to add the check.In
@web/default/src/features/channels/lib/channel-utils.ts:
- Around line 86-87: The helper in
web/default/src/features/channels/lib/channel-utils.ts currently returns raw
icon URLs while its docs and name indicate it returns icon "names"; update the
helper contract so it is unambiguous: either (A) change the function name and
JSDoc to reflect it returns icon URLs (e.g., rename to getIconUrl / update
return type to string URL and update callers/tests), or (B) preserve the “name”
contract by converting those URL entries (like
'https://www.pingxingshijie.cn/favicon.ico' and 'https://kie.ai/logo.png') to
icon identifiers and return only names (and map names to URLs in a separate
resolver). Make the chosen change consistently in the function signature,
exported type, JSDoc, and any callers in the repo.In
@web/default/src/features/usage-logs/components/dialogs/task-media-results-dialog.tsx:
- Around line 23-27: The copyUrl function currently writes to
navigator.clipboard without handling failures; update copyUrl(url: string,
successMessage: string) to catch promise rejections from
navigator.clipboard.writeText and surface an error (e.g., toast.error with a
helpful message) so clipboard permission/secure-context failures are not silent;
keep the success toast on then() and add a .catch() (or try/catch if you convert
to async/await) to log or toast the error and optionally console.error the
caught error for debugging.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `ddaf82af-c5c3-43e2-b851-f34e1607c888` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between dac55f0fdeb16bbbc2bdc472bda14e60431f3845 and e5d030799a7ed82444eb9032d8ca63bdccafeab2. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `web/classic/bun.lock` is excluded by `!**/*.lock` * `web/classic/public/kie-ai-logo.png` is excluded by `!**/*.png` </details> <details> <summary>📒 Files selected for processing (83)</summary> * `.gitignore` * `common/api_type.go` * `constant/channel.go` * `controller/channel-test.go` * `controller/model.go` * `controller/model_list_test.go` * `controller/relay.go` * `docs/pingxingshijie-api-reference.md` * `docs/pingxingshijie-downstream-curl.md` * `docs/pingxingshijie-openai-compatibility.md` * `docs/token-management-api-reference.md` * `dto/openai_request.go` * `dto/task.go` * `middleware/distributor.go` * `middleware/distributor_test.go` * `model/task.go` * `model/task_result_url.go` * `relay/channel/adapter.go` * `relay/channel/claude/relay-claude.go` * `relay/channel/task/ali/adaptor.go` * `relay/channel/task/ali/constants.go` * `relay/channel/task/doubao/adaptor.go` * `relay/channel/task/kie/adaptor.go` * `relay/channel/task/kie/adaptor_test.go` * `relay/channel/task/kie/constants.go` * `relay/channel/task/pingxingshijie/adaptor.go` * `relay/channel/task/pingxingshijie/asset_model_validation_test.go` * `relay/channel/task/pingxingshijie/constants.go` * `relay/channel/task/pingxingshijie/constants_test.go` * `relay/channel/task/pingxingshijie/draft_upscale_test.go` * `relay/channel/task/pingxingshijie/envelope.go` * `relay/channel/task/pingxingshijie/envelope_test.go` * `relay/channel/task/pingxingshijie/extract_video_id_test.go` * `relay/channel/task/pingxingshijie/kind.go` * `relay/channel/task/pingxingshijie/unwrap_task_data_test.go` * `relay/channel/task/pingxingshijie/upstream_metadata.go` * `relay/channel/task/pingxingshijie/upstream_metadata_test.go` * `relay/channel/volcengine/adaptor.go` * `relay/channel/volcengine/adaptor_test.go` * `relay/common/relay_info.go` * `relay/common/task_submit_req_test.go` * `relay/helper/stream_scanner.go` * `relay/relay_adaptor.go` * `relay/relay_adaptor_test.go` * `relay/relay_task.go` * `relay/relay_task_test.go` * `router/video-router.go` * `service/file_service.go` * `service/task_polling.go` * `web/classic/package.json` * `web/classic/src/components/table/task-logs/TaskLogsColumnDefs.jsx` * `web/classic/src/components/table/task-logs/TaskLogsTable.jsx` * `web/classic/src/components/table/task-logs/index.jsx` * `web/classic/src/components/table/task-logs/modals/ContentModal.jsx` * `web/classic/src/constants/channel.constants.js` * `web/classic/src/helpers/render.jsx` * `web/classic/src/hooks/task-logs/useTaskLogsData.js` * `web/classic/src/i18n/locales/en.json` * `web/classic/src/i18n/locales/fr.json` * `web/classic/src/i18n/locales/ja.json` * `web/classic/src/i18n/locales/ru.json` * `web/classic/src/i18n/locales/vi.json` * `web/classic/src/i18n/locales/zh-CN.json` * `web/classic/src/i18n/locales/zh-TW.json` * `web/default/src/features/channels/constants.ts` * `web/default/src/features/channels/lib/channel-utils.ts` * `web/default/src/features/usage-logs/components/columns/task-logs-columns.tsx` * `web/default/src/features/usage-logs/components/dialogs/task-media-results-dialog.tsx` * `web/default/src/features/usage-logs/lib/index.ts` * `web/default/src/features/usage-logs/lib/task-action-label.test.ts` * `web/default/src/features/usage-logs/lib/task-action-label.ts` * `web/default/src/features/usage-logs/lib/task-media-results.ts` * `web/default/src/features/usage-logs/types.ts` * `web/default/src/i18n/locales/en.json` * `web/default/src/i18n/locales/fr.json` * `web/default/src/i18n/locales/ja.json` * `web/default/src/i18n/locales/ru.json` * `web/default/src/i18n/locales/vi.json` * `web/default/src/i18n/locales/zh.json` * `web/default/src/lib/lobe-icon.tsx` * `web/default/tests/channel-utils.test.ts` * `web/default/tests/lobe-icon.test.tsx` * `web/default/tests/task-media-results.test.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+125
to
+128
| // PingXingShiJie: sync image models use OpenAI image relay; text uses chat (Volc-style upstream via adaptor mapping). | ||
| if channel.Type == constant.ChannelTypePingXingShiJie && strings.Contains(strings.ToLower(testModel), "seedream") { | ||
| requestPath = "/v1/images/generations" | ||
| } |
Contributor
There was a problem hiding this comment.
Keep endpoint type in sync when forcing PingXing image test path.
Line 125-128 switches requestPath to /v1/images/generations, but endpointType stays empty, so buildTestRequest(...) can still build a non-image request and later fail image conversion (invalid image request type).
🔧 Proposed fix
- // VolcEngine 图像生成模型
- if channel.Type == constant.ChannelTypeVolcEngine && strings.Contains(testModel, "seedream") {
- requestPath = "/v1/images/generations"
- }
-
- // PingXingShiJie: sync image models use OpenAI image relay; text uses chat (Volc-style upstream via adaptor mapping).
- if channel.Type == constant.ChannelTypePingXingShiJie && strings.Contains(strings.ToLower(testModel), "seedream") {
- requestPath = "/v1/images/generations"
- }
+ // Seedream image models should use OpenAI image endpoint and image request payload.
+ lowerModel := strings.ToLower(testModel)
+ if strings.Contains(lowerModel, "seedream") &&
+ (channel.Type == constant.ChannelTypeVolcEngine || channel.Type == constant.ChannelTypePingXingShiJie) {
+ requestPath = "/v1/images/generations"
+ endpointType = string(constant.EndpointTypeImageGeneration)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/channel-test.go` around lines 125 - 128, When forcing the
PingXingShiJie image test path you set requestPath but forgot to set
endpointType, causing buildTestRequest(...) to produce a non-image request;
update the branch that checks channel.Type == constant.ChannelTypePingXingShiJie
&& strings.Contains(strings.ToLower(testModel), "seedream") to also set
endpointType to the image type used by your request (e.g., the image/images
enum/value your code expects) so buildTestRequest(...) builds an image request
and image conversion won't fail.
Comment on lines
+395
to
+397
| if mimeType == "" && file.FileName != "" { | ||
| mimeType = "application/octet-stream" | ||
| } |
Contributor
There was a problem hiding this comment.
Make the fallback MIME unconditional for file payloads.
When file.FileName is empty, MIME remains blank even though this branch already confirmed file data exists. Defaulting to application/octet-stream in all empty-MIME cases keeps behavior consistent.
Suggested patch
- if mimeType == "" && file.FileName != "" {
+ if mimeType == "" {
mimeType = "application/octet-stream"
}📝 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 mimeType == "" && file.FileName != "" { | |
| mimeType = "application/octet-stream" | |
| } | |
| if mimeType == "" { | |
| mimeType = "application/octet-stream" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dto/openai_request.go` around lines 395 - 397, Change the conditional that
sets a fallback MIME so it does not depend on file.FileName: whenever mimeType
== "" and there is a file payload, set mimeType = "application/octet-stream"
regardless of file.FileName. Locate the logic that currently checks mimeType ==
"" && file.FileName != "" (using the mimeType and file.FileName variables) and
remove the file.FileName check so the fallback is unconditional for empty
mimeType on file payloads.
Comment on lines
+32
to
+39
| func TestGetModelRequestAssetUploadRejectsBlankModel(t *testing.T) { | ||
| c := newJSONContext(http.MethodPost, "/v1/assets/upload", `{"image_url":"https://example.com/a.jpg","asset_type":"Image"}`) | ||
|
|
||
| _, _, err := getModelRequest(c) | ||
|
|
||
| if err == nil { | ||
| t.Fatalf("expected blank model to be rejected for /v1/assets/upload; required model is %q", taskpxsj.AssetPlaceholderModel) | ||
| } |
Contributor
There was a problem hiding this comment.
Cover the explicit empty-string model case too.
This test currently exercises a missing model, not {"model":""}. That leaves a regression gap where the handler could still accept an explicit blank string and this suite would stay green. It also doesn't verify that the error mentions taskpxsj.AssetPlaceholderModel, which is part of the contract you described in the PR.
Suggested test tightening
func TestGetModelRequestAssetUploadRejectsBlankModel(t *testing.T) {
- c := newJSONContext(http.MethodPost, "/v1/assets/upload", `{"image_url":"https://example.com/a.jpg","asset_type":"Image"}`)
+ c := newJSONContext(http.MethodPost, "/v1/assets/upload", `{"model":"","image_url":"https://example.com/a.jpg","asset_type":"Image"}`)
_, _, err := getModelRequest(c)
if err == nil {
t.Fatalf("expected blank model to be rejected for /v1/assets/upload; required model is %q", taskpxsj.AssetPlaceholderModel)
}
+ if !strings.Contains(err.Error(), taskpxsj.AssetPlaceholderModel) {
+ t.Fatalf("expected error to mention required model %q, got %v", taskpxsj.AssetPlaceholderModel, err)
+ }
}📝 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
| func TestGetModelRequestAssetUploadRejectsBlankModel(t *testing.T) { | |
| c := newJSONContext(http.MethodPost, "/v1/assets/upload", `{"image_url":"https://example.com/a.jpg","asset_type":"Image"}`) | |
| _, _, err := getModelRequest(c) | |
| if err == nil { | |
| t.Fatalf("expected blank model to be rejected for /v1/assets/upload; required model is %q", taskpxsj.AssetPlaceholderModel) | |
| } | |
| func TestGetModelRequestAssetUploadRejectsBlankModel(t *testing.T) { | |
| c := newJSONContext(http.MethodPost, "/v1/assets/upload", `{"model":"","image_url":"https://example.com/a.jpg","asset_type":"Image"}`) | |
| _, _, err := getModelRequest(c) | |
| if err == nil { | |
| t.Fatalf("expected blank model to be rejected for /v1/assets/upload; required model is %q", taskpxsj.AssetPlaceholderModel) | |
| } | |
| if !strings.Contains(err.Error(), taskpxsj.AssetPlaceholderModel) { | |
| t.Fatalf("expected error to mention required model %q, got %v", taskpxsj.AssetPlaceholderModel, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/distributor_test.go` around lines 32 - 39, Update
TestGetModelRequestAssetUploadRejectsBlankModel to also exercise the explicit
empty-string model case by calling newJSONContext with a body that includes
`"model":""` (e.g.,
`{"model":"","image_url":"https://example.com/a.jpg","asset_type":"Image"}`),
call getModelRequest for that context, assert err is non-nil, and assert the
returned error message contains the expected placeholder constant
taskpxsj.AssetPlaceholderModel; keep the existing missing-model case as well so
both scenarios are covered.
Comment on lines
+37
to
+55
| func walkFirstImageLikeURL(v any) string { | ||
| switch x := v.(type) { | ||
| case map[string]any: | ||
| if u, ok := x["url"].(string); ok && strings.HasPrefix(u, "http") && looksLikeImageAssetURL(u) { | ||
| return u | ||
| } | ||
| for _, vv := range x { | ||
| if s := walkFirstImageLikeURL(vv); s != "" { | ||
| return s | ||
| } | ||
| } | ||
| case []any: | ||
| for _, item := range x { | ||
| if s := walkFirstImageLikeURL(item); s != "" { | ||
| return s | ||
| } | ||
| } | ||
| } | ||
| return "" |
Contributor
There was a problem hiding this comment.
walkFirstImageLikeURL does not have a stable “first” URL.
Line 43 ranges over a map[string]any, so payloads with multiple image-like url fields can return different results across calls. That makes result previews nondeterministic. Please make the descent order explicit here instead of relying on map iteration.
One deterministic fix
+import "sort"
+
func walkFirstImageLikeURL(v any) string {
switch x := v.(type) {
case map[string]any:
if u, ok := x["url"].(string); ok && strings.HasPrefix(u, "http") && looksLikeImageAssetURL(u) {
return u
}
- for _, vv := range x {
- if s := walkFirstImageLikeURL(vv); s != "" {
+ keys := make([]string, 0, len(x))
+ for k := range x {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+ for _, k := range keys {
+ if s := walkFirstImageLikeURL(x[k]); s != "" {
return s
}
}
case []any:📝 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
| func walkFirstImageLikeURL(v any) string { | |
| switch x := v.(type) { | |
| case map[string]any: | |
| if u, ok := x["url"].(string); ok && strings.HasPrefix(u, "http") && looksLikeImageAssetURL(u) { | |
| return u | |
| } | |
| for _, vv := range x { | |
| if s := walkFirstImageLikeURL(vv); s != "" { | |
| return s | |
| } | |
| } | |
| case []any: | |
| for _, item := range x { | |
| if s := walkFirstImageLikeURL(item); s != "" { | |
| return s | |
| } | |
| } | |
| } | |
| return "" | |
| import "sort" | |
| func walkFirstImageLikeURL(v any) string { | |
| switch x := v.(type) { | |
| case map[string]any: | |
| if u, ok := x["url"].(string); ok && strings.HasPrefix(u, "http") && looksLikeImageAssetURL(u) { | |
| return u | |
| } | |
| keys := make([]string, 0, len(x)) | |
| for k := range x { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| for _, k := range keys { | |
| if s := walkFirstImageLikeURL(x[k]); s != "" { | |
| return s | |
| } | |
| } | |
| case []any: | |
| for _, item := range x { | |
| if s := walkFirstImageLikeURL(item); s != "" { | |
| return s | |
| } | |
| } | |
| } | |
| return "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/task_result_url.go` around lines 37 - 55, walkFirstImageLikeURL
currently iterates a map (map[string]any) which yields nondeterministic order;
change the map descent in walkFirstImageLikeURL so keys are collected, sorted
(e.g., lexicographically), and then iterated in that sorted order before
recursing into values so the "first" image-like url is stable across runs; keep
the existing url check (x["url"]) and recursion logic but replace the direct
range over the map with a deterministic sorted-key loop.
Comment on lines
+137
to
+146
| u := t.PrivateData.ResultURL | ||
| if u == "" { | ||
| u = t.FailReason | ||
| } | ||
| return t.FailReason | ||
| if u == "" { | ||
| return "" | ||
| } | ||
| if !isVideoProxyContentURL(u, t.TaskID) { | ||
| return u | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t expose data: fallbacks as result URLs.
GetResultURL() now returns the FailReason fallback verbatim whenever PrivateData.ResultURL is empty. If an older row still has a data: URL there, this pushes the full base64 payload back into API responses/logs instead of suppressing it.
Guard the fallback before returning it
func (t *Task) GetResultURL() string {
u := t.PrivateData.ResultURL
if u == "" {
u = t.FailReason
}
+ u = strings.TrimSpace(u)
if u == "" {
return ""
}
+ if strings.HasPrefix(strings.ToLower(u), "data:") {
+ return ""
+ }
if !isVideoProxyContentURL(u, t.TaskID) {
return u
}Based on learnings: In controller/task_video.go, data: URLs are intentionally prevented from being stored in task.FailReason, and the same suppression pattern should be applied consistently across the codebase.
📝 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
| u := t.PrivateData.ResultURL | |
| if u == "" { | |
| u = t.FailReason | |
| } | |
| return t.FailReason | |
| if u == "" { | |
| return "" | |
| } | |
| if !isVideoProxyContentURL(u, t.TaskID) { | |
| return u | |
| } | |
| u := t.PrivateData.ResultURL | |
| if u == "" { | |
| u = t.FailReason | |
| } | |
| u = strings.TrimSpace(u) | |
| if u == "" { | |
| return "" | |
| } | |
| if strings.HasPrefix(strings.ToLower(u), "data:") { | |
| return "" | |
| } | |
| if !isVideoProxyContentURL(u, t.TaskID) { | |
| return u | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/task.go` around lines 137 - 146, GetResultURL currently returns
t.FailReason verbatim when PrivateData.ResultURL is empty, which can leak data:
base64 payloads; change the logic in GetResultURL so that after selecting u :=
t.PrivateData.ResultURL or u = t.FailReason, you validate u with
isVideoProxyContentURL(u, t.TaskID) and if that function indicates a disallowed
data: URL, return an empty string instead of u; keep references to
t.PrivateData.ResultURL, t.FailReason, isVideoProxyContentURL, and TaskID so
reviewers can find and update the GetResultURL implementation accordingly.
Comment on lines
+10
to
+12
| "@visactor/react-vchart": "^2.0.21", | ||
| "@visactor/vchart": "^2.0.21", | ||
| "@visactor/vchart-semi-theme": "^1.12.3", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
npm view `@visactor/vchart`@2.0.21 version peerDependencies
npm view `@visactor/react-vchart`@2.0.21 version peerDependencies
npm view `@visactor/vchart-semi-theme`@1.12.3 version peerDependenciesRepository: QuantumNous/new-api
Length of output: 228
@visactor/vchart-semi-theme is incompatible with vchart 2.x and must be resolved before merge.
The @visactor/vchart-semi-theme@1.12.3 package declares a peerDependency on @visactor/vchart: >=1.10.4, which covers only 1.x versions. Using it with @visactor/vchart@2.0.21 violates this constraint and will cause theme registration to fail at runtime.
Options:
- Upgrade
@visactor/vchart-semi-themeto a version compatible with vchart 2.x, or - Downgrade
@visactor/vchartand@visactor/react-vchartto 1.x if a compatible theme package exists
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/classic/package.json` around lines 10 - 12, The package.json lists
incompatible chart packages: `@visactor/vchart-semi-theme`@1.12.3 declares a
peerDependency for `@visactor/vchart` 1.x while the project depends on
`@visactor/vchart`@2.0.21 and `@visactor/react-vchart`@2.0.21; resolve by either
upgrading `@visactor/vchart-semi-theme` to a release that supports vchart 2.x
(replace the dependency entry for "@visactor/vchart-semi-theme" with the
compatible version) or by downgrading "@visactor/vchart" and
"@visactor/react-vchart" to matching 1.x versions (update both entries) and
ensure package-lock/yarn.lock is regenerated and CI installs succeed.
Comment on lines
39
to
+48
| useEffect(() => { | ||
| if (isModalOpen && isVideo) { | ||
| setVideoError(false); | ||
| setIsLoading(true); | ||
| } | ||
| }, [isModalOpen, isVideo]); | ||
| if (isModalOpen && isImage) { | ||
| setImageError(false); | ||
| setIsLoading(true); | ||
| } | ||
| }, [isModalOpen, isVideo, isImage]); |
Contributor
There was a problem hiding this comment.
Reset preview state when modalContent changes.
The effect never reruns for a new URL while the modal stays open. If one preview hits imageError/videoError, then the parent swaps modalContent, the fallback branch sticks and the next asset never gets a chance to load.
Small fix
- }, [isModalOpen, isVideo, isImage]);
+ }, [isModalOpen, isVideo, isImage, modalContent]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/classic/src/components/table/task-logs/modals/ContentModal.jsx` around
lines 39 - 48, The effect that resets preview state only depends on isModalOpen,
isVideo, and isImage, so it won't rerun when the parent swaps the URL
(modalContent) while the modal remains open; update the dependency array of the
useEffect that sets setVideoError(false), setImageError(false), and
setIsLoading(true) to also include modalContent (or the prop that carries the
new URL) so the effect runs whenever modalContent changes and the preview state
is properly reset when a new asset is loaded.
Comment on lines
+9
to
+14
| if (log.upstream_kind === 'asset') { | ||
| return 'Asset Upload' | ||
| } | ||
| if (log.upstream_kind === 'image') { | ||
| return 'Image Generation' | ||
| } |
Contributor
There was a problem hiding this comment.
Replace hardcoded task labels with i18n keys.
These returned strings are user-facing and bypass localization. Return translation keys here (and translate at render) to keep usage logs multilingual.
🌐 Suggested adjustment
export function getTaskActionLabel(
log: TaskActionLabelInput,
fallbackLabel: string
): string {
if (log.upstream_kind === 'asset') {
- return 'Asset Upload'
+ return 'usageLogs.taskAction.assetUpload'
}
if (log.upstream_kind === 'image') {
- return 'Image Generation'
+ return 'usageLogs.taskAction.imageGeneration'
}
return fallbackLabel
}As per coding guidelines: web/default/src/**: “Frontend i18n … use useTranslation() hook and call t('English key') in components”, and web/default/**/*.{ts,tsx}: “All user-facing text must support i18n using … t()”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/default/src/features/usage-logs/lib/task-action-label.ts` around lines 9
- 14, The function in task-action-label.ts currently returns hardcoded
user-facing strings for log.upstream_kind ('asset' -> 'Asset Upload', 'image' ->
'Image Generation'); replace those literal returns with stable i18n keys (e.g.
'usage.task.assetUpload', 'usage.task.imageGeneration') so the function returns
keys only, and update consuming components to call useTranslation() and
t(returnedKey) at render time; update any tests/fixtures to expect keys instead
of English text and add the new keys to the locale files.
Comment on lines
+68
to
+74
| function isStaleImageProxyUrl(url: string, source: TaskMediaSource): boolean { | ||
| return ( | ||
| source.upstream_kind === 'image' && | ||
| url.includes('/v1/videos/') && | ||
| url.includes('/content') | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Treat upstream_kind: 'asset' as image-like here.
The backend now distinguishes video | image | asset, but this extractor only special-cases 'image' and 'video'. For successful asset uploads, extension-less or proxy-shaped URLs can still fall through to the video heuristics, which means the new usage-log preview can show the wrong icon/dialog or skip rendering entirely.
Localized fix
function isStaleImageProxyUrl(url: string, source: TaskMediaSource): boolean {
return (
- source.upstream_kind === 'image' &&
+ (source.upstream_kind === 'image' || source.upstream_kind === 'asset') &&
url.includes('/v1/videos/') &&
url.includes('/content')
)
}
@@
if (allowTaskFallback) {
- if (source.upstream_kind === 'image') return 'image'
+ if (source.upstream_kind === 'image' || source.upstream_kind === 'asset') {
+ return 'image'
+ }
if (source.upstream_kind === 'video') return 'video'
if (VIDEO_ACTIONS.has(source.action)) return 'video'
}Also applies to: 97-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/default/src/features/usage-logs/lib/task-media-results.ts` around lines
68 - 74, Update the image-proxy heuristic to treat upstream_kind === 'asset' the
same as 'image': in isStaleImageProxyUrl (and the other predicate that similarly
checks source.upstream_kind for image/video heuristics later in the file),
include source.upstream_kind === 'asset' alongside 'image' so asset uploads use
the image-path logic and won't be misclassified as video or skipped.
Comment on lines
+117
to
+121
| if ( | ||
| isTaskVideoProxyUrl(url, source) && | ||
| results.some((result) => result.type === 'image') | ||
| ) { | ||
| return |
Contributor
There was a problem hiding this comment.
A stale task-proxy URL can still win over the real image.
extractTaskMediaResults() seeds result_url/fail_reason before it walks data, but the /v1/videos/{task_id}/content filter only suppresses that proxy once an image is already present. For legacy rows where the seeded URL is the stale proxy and the actual image only exists inside data, you keep the wrong first entry and the dialog opens the broken target.
A safer approach is to defer task-proxy fallbacks until after walkTaskData(), or to drop any seeded task-proxy entry once a concrete image result is discovered.
Also applies to: 183-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/default/src/features/usage-logs/lib/task-media-results.ts` around lines
117 - 121, extractTaskMediaResults currently seeds a task-proxy result (using
isTaskVideoProxyUrl) before calling walkTaskData, which lets a stale proxy win
even when a real image exists inside data; change the logic to treat proxy
entries as fallbacks: either (A) do not push the seeded proxy into results
up-front — instead keep it in a local fallbackCandidate and only append it after
walkTaskData if no concrete image result was produced, or (B) after walkTaskData
completes, scan results and remove any entries whose URL is a task-proxy
(isTaskVideoProxyUrl(url, source)) whenever a concrete image result exists;
apply the same change to the analogous block referenced at lines 183-185 so
proxy fallbacks cannot override real images.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
relay/channel/task/pingxingshijie/asset_request_body_test.go (1)
24-31: ⚡ Quick winPrefer structural JSON assertions over raw substring matching in this test.
strings.Contains(string(raw), "model")can become brittle with unrelated content. Decode once and assert key presence/absence directly.💡 Suggested refactor
import ( "io" - "strings" "testing" + "github.com/QuantumNous/new-api/common" relaycommon "github.com/QuantumNous/new-api/relay/common" ) @@ - if strings.Contains(string(raw), "model") { - t.Fatalf("asset upstream body must not include gateway-only model: %s", string(raw)) - } - if !strings.Contains(string(raw), `"image_url":"https://example.com/a.jpg"`) { - t.Fatalf("asset upstream body lost image_url: %s", string(raw)) - } - if !strings.Contains(string(raw), `"asset_type":"Image"`) { - t.Fatalf("asset upstream body lost asset_type: %s", string(raw)) - } + var got map[string]any + if err := common.Unmarshal(raw, &got); err != nil { + t.Fatalf("unmarshal upstream body failed: %v, raw=%s", err, string(raw)) + } + if _, ok := got["model"]; ok { + t.Fatalf("asset upstream body must not include gateway-only model: %s", string(raw)) + } + if got["image_url"] != "https://example.com/a.jpg" { + t.Fatalf("asset upstream body lost/changed image_url: %s", string(raw)) + } + if got["asset_type"] != "Image" { + t.Fatalf("asset upstream body lost/changed asset_type: %s", string(raw)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/pingxingshijie/asset_request_body_test.go` around lines 24 - 31, Replace brittle substring checks that use strings.Contains on raw with a single JSON decode of raw into a map[string]interface{} (e.g., var body map[string]interface{}; json.Unmarshal(raw, &body)), then assert the top-level "model" key is absent (check _, ok := body["model"]; if ok { t.Fatalf(...) }) and assert body["image_url"] equals "https://example.com/a.jpg" and body["asset_type"] equals "Image" using type assertions for string values; update the error messages to include the marshalled body when a check fails to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pingxingshijie-api-reference.md`:
- Around line 22-23: The current sentence implies every /v1 route uses Token 认证
+ Distribute; narrow it to only task submission/query routes by updating the doc
text so it states that routes under router/video-router.go related to task
submit/query (e.g., routes handling video task creation and status/query) use
TokenAuth + Distribute, and explicitly call out the exception
/v1/videos/:task_id/content which uses TokenOrUserAuth; reference
router/video-router.go and the /v1/videos/:task_id/content route and mention
TokenOrUserAuth vs TokenAuth + Distribute to avoid confusion.
In `@relay/channel/task/pingxingshijie/adaptor.go`:
- Around line 802-807: The success-case sets tr.Status/tr.Progress but only
reads URL from the top-level map m, which loses URLs when upstream embeds them
under the Result object; update the success branch (the switch/case block that
sets tr.Status = model.TaskStatusSuccess and tr.Progress = "100%") to attempt
extraction from the Result/result map first (the same object used to derive
status) and fall back to extractStringFromMap(m, "url", "asset_url", "AssetUrl")
if not found, then assign that value to tr.Url so successful tasks pick up URLs
returned inside Result.
---
Nitpick comments:
In `@relay/channel/task/pingxingshijie/asset_request_body_test.go`:
- Around line 24-31: Replace brittle substring checks that use strings.Contains
on raw with a single JSON decode of raw into a map[string]interface{} (e.g., var
body map[string]interface{}; json.Unmarshal(raw, &body)), then assert the
top-level "model" key is absent (check _, ok := body["model"]; if ok {
t.Fatalf(...) }) and assert body["image_url"] equals "https://example.com/a.jpg"
and body["asset_type"] equals "Image" using type assertions for string values;
update the error messages to include the marshalled body when a check fails to
aid debugging.
🪄 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: 18ccb10a-03cc-49a1-8a23-2e53a811f83a
📒 Files selected for processing (3)
docs/pingxingshijie-api-reference.mdrelay/channel/task/pingxingshijie/adaptor.gorelay/channel/task/pingxingshijie/asset_request_body_test.go
Comment on lines
+22
to
+23
| **说明**：下列路由挂在 `router/video-router.go` 的 `/v1` 分组上，使用 **Token 认证** + **Distribute（按模型选渠道）**。调用方必须在渠道中配置 **渠道类型 58（PingXingShiJie）**，且令牌对该渠道上的 **model** 有权限。 | ||
|
|
Contributor
There was a problem hiding this comment.
Clarify the scope of auth/distribution statement.
Line 22 currently reads like all /v1 routes in this doc use TokenAuth + Distribute, but /v1/videos/:task_id/content is an exception (TokenOrUserAuth). Narrow this sentence to task submit/query routes to avoid confusion.
✏️ Suggested wording
-**说明**：下列路由挂在 `router/video-router.go` 的 `/v1` 分组上，使用 **Token 认证** + **Distribute（按模型选渠道）**。调用方必须在渠道中配置 **渠道类型 58（PingXingShiJie）**，且令牌对该渠道上的 **model** 有权限。
+**说明**：本文档中的**任务提交/查询路由**挂在 `router/video-router.go` 的 `/v1` 分组上，使用 **Token 认证** + **Distribute（按模型选渠道）**。调用方必须在渠道中配置 **渠道类型 58（PingXingShiJie）**，且令牌对该渠道上的 **model** 有权限。
+`/v1/videos/:task_id/content` 为例外，使用 `TokenOrUserAuth`（见第 7 节）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pingxingshijie-api-reference.md` around lines 22 - 23, The current
sentence implies every /v1 route uses Token 认证 + Distribute; narrow it to only
task submission/query routes by updating the doc text so it states that routes
under router/video-router.go related to task submit/query (e.g., routes handling
video task creation and status/query) use TokenAuth + Distribute, and explicitly
call out the exception /v1/videos/:task_id/content which uses TokenOrUserAuth;
reference router/video-router.go and the /v1/videos/:task_id/content route and
mention TokenOrUserAuth vs TokenAuth + Distribute to avoid confusion.
Comment on lines
+802
to
+807
| case "active", "succeeded", "success", "completed": | ||
| tr.Status = model.TaskStatusSuccess | ||
| tr.Progress = "100%" | ||
| if u := extractStringFromMap(m, "url", "asset_url", "AssetUrl"); u != "" { | ||
| tr.Url = u | ||
| } |
Contributor
There was a problem hiding this comment.
Successful asset status may lose URL because extraction reads the wrong object.
On Line 805, status is derived from res (Result/result), but URL is read only from top-level m. If upstream returns URL inside Result, TaskInfo.Url stays empty even on success.
💡 Suggested fix
case "active", "succeeded", "success", "completed":
tr.Status = model.TaskStatusSuccess
tr.Progress = "100%"
- if u := extractStringFromMap(m, "url", "asset_url", "AssetUrl"); u != "" {
+ if u := extractStringFromMap(res, "url", "Url", "asset_url", "AssetUrl", "assetUrl"); u != "" {
+ tr.Url = u
+ } else if u := extractStringFromMap(m, "url", "Url", "asset_url", "AssetUrl", "assetUrl"); u != "" {
tr.Url = u
}📝 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
| case "active", "succeeded", "success", "completed": | |
| tr.Status = model.TaskStatusSuccess | |
| tr.Progress = "100%" | |
| if u := extractStringFromMap(m, "url", "asset_url", "AssetUrl"); u != "" { | |
| tr.Url = u | |
| } | |
| case "active", "succeeded", "success", "completed": | |
| tr.Status = model.TaskStatusSuccess | |
| tr.Progress = "100%" | |
| if u := extractStringFromMap(res, "url", "Url", "asset_url", "AssetUrl", "assetUrl"); u != "" { | |
| tr.Url = u | |
| } else if u := extractStringFromMap(m, "url", "Url", "asset_url", "AssetUrl", "assetUrl"); u != "" { | |
| tr.Url = u | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/task/pingxingshijie/adaptor.go` around lines 802 - 807, The
success-case sets tr.Status/tr.Progress but only reads URL from the top-level
map m, which loses URLs when upstream embeds them under the Result object;
update the success branch (the switch/case block that sets tr.Status =
model.TaskStatusSuccess and tr.Progress = "100%") to attempt extraction from the
Result/result map first (the same object used to derive status) and fall back to
extractStringFromMap(m, "url", "asset_url", "AssetUrl") if not found, then
assign that value to tr.Url so successful tasks pick up URLs returned inside
Result.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…et_id - Add GetByUpstreamTaskId to query tasks by private_data.upstream_task_id - Add getTaskForVideoFetch to prefer upstream_task_id for /v1/assets/ routes - Rename route param :task_id to :asset_id for asset fetch - Fallback to public task_id for backward compatibility - Add regression tests for upstream-task-id lookup Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- Change GET /v1/assets/:task_id to GET /v1/assets/:asset_id - Document fallback to public task_id for legacy clients - Update route summary table and compatibility notes Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/relay_task.go (1)
502-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRealtime fetch should reuse the task-scoped API key.
This follow-up call uses
channelModel.Key, even though Gemini/Vertex tasks persist the effective key intask.PrivateData.Keyat submit time. If the channel rotates keys or the task was created with a different key, realtime fetch can start polling with the wrong credentials.Suggested fix
- resp, err := adaptor.FetchTask(baseURL, channelModel.Key, ft, proxy) + apiKey := channelModel.Key + if task.PrivateData.Key != "" { + apiKey = task.PrivateData.Key + } + resp, err := adaptor.FetchTask(baseURL, apiKey, ft, proxy)Based on learnings: follow-up requests should reuse the effective authentication context captured at task submit time, preferring
task.PrivateData.Keyoverchannel.Key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 502 - 519, The realtime fetch uses channelModel.Key but should prefer the task-scoped effective API key stored on the task; change the call that builds credentials for adaptor.FetchTask to use task.PrivateData.Key if non-empty, falling back to channelModel.GetKey()/channelModel.Key otherwise (keep baseURL resolution and proxy retrieval as-is); update the local variable currently passed (channelModel.Key) to the effectiveKey (check task.PrivateData.Key first) before calling GetTaskAdaptor(...).FetchTask so follow-up polling uses the same auth context captured at submit time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/relay_task_test.go`:
- Around line 47-54: The test sets model.DB and common.UsingSQLite but doesn't
clear other DB engine flags, making GetByUpstreamTaskId() behavior
order-dependent; update the setup to save and restore common.UsingPostgreSQL and
common.UsingMySQL (alongside common.UsingSQLite and model.DB) and in the test
set only common.UsingSQLite = true and the others = false, then restore all
saved flags in the t.Cleanup closure so GetByUpstreamTaskId() will reliably take
the SQLite branch.
In `@relay/relay_task.go`:
- Around line 292-299: otherRatiosProduct is nondeterministic because it
multiplies values by iterating a map; to avoid floating-point rounding
differences that change int-truncated quotas, collect the values from ratios (or
keys), sort the numeric values in a deterministic order (e.g., ascending), then
multiply them in that sorted order and return the product; update the function
otherRatiosProduct to build a slice of ra values, sort.Float64s on that slice,
and multiply only those >0 and !=1.0 so the multiplication order is stable
across runs.
---
Outside diff comments:
In `@relay/relay_task.go`:
- Around line 502-519: The realtime fetch uses channelModel.Key but should
prefer the task-scoped effective API key stored on the task; change the call
that builds credentials for adaptor.FetchTask to use task.PrivateData.Key if
non-empty, falling back to channelModel.GetKey()/channelModel.Key otherwise
(keep baseURL resolution and proxy retrieval as-is); update the local variable
currently passed (channelModel.Key) to the effectiveKey (check
task.PrivateData.Key first) before calling GetTaskAdaptor(...).FetchTask so
follow-up polling uses the same auth context captured at submit time.
🪄 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: 7f4761e9-850d-4431-a7db-eb2f9156e90d
📒 Files selected for processing (8)
docs/pingxingshijie-api-reference.mddocs/pingxingshijie-openai-compatibility.mdmodel/task.gomodel/task_cas_test.gorelay/channel/adapter.gorelay/relay_task.gorelay/relay_task_test.gorouter/video-router.go
✅ Files skipped from review due to trivial changes (1)
- relay/channel/adapter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/video-router.go
Comment on lines
+47
to
+54
| oldDB := model.DB | ||
| oldUsingSQLite := common.UsingSQLite | ||
| model.DB = db | ||
| common.UsingSQLite = true | ||
| t.Cleanup(func() { | ||
| model.DB = oldDB | ||
| common.UsingSQLite = oldUsingSQLite | ||
| }) |
Contributor
There was a problem hiding this comment.
Reset all DB-engine flags in this SQLite test.
GetByUpstreamTaskId() checks common.UsingPostgreSQL and common.UsingMySQL before the SQLite branch. Only setting common.UsingSQLite = true makes this test order-dependent if another test left one of the other flags enabled.
Suggested fix
oldDB := model.DB
oldUsingSQLite := common.UsingSQLite
+ oldUsingMySQL := common.UsingMySQL
+ oldUsingPostgreSQL := common.UsingPostgreSQL
model.DB = db
common.UsingSQLite = true
+ common.UsingMySQL = false
+ common.UsingPostgreSQL = false
t.Cleanup(func() {
model.DB = oldDB
common.UsingSQLite = oldUsingSQLite
+ common.UsingMySQL = oldUsingMySQL
+ common.UsingPostgreSQL = oldUsingPostgreSQL
})📝 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
| oldDB := model.DB | |
| oldUsingSQLite := common.UsingSQLite | |
| model.DB = db | |
| common.UsingSQLite = true | |
| t.Cleanup(func() { | |
| model.DB = oldDB | |
| common.UsingSQLite = oldUsingSQLite | |
| }) | |
| oldDB := model.DB | |
| oldUsingSQLite := common.UsingSQLite | |
| oldUsingMySQL := common.UsingMySQL | |
| oldUsingPostgreSQL := common.UsingPostgreSQL | |
| model.DB = db | |
| common.UsingSQLite = true | |
| common.UsingMySQL = false | |
| common.UsingPostgreSQL = false | |
| t.Cleanup(func() { | |
| model.DB = oldDB | |
| common.UsingSQLite = oldUsingSQLite | |
| common.UsingMySQL = oldUsingMySQL | |
| common.UsingPostgreSQL = oldUsingPostgreSQL | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/relay_task_test.go` around lines 47 - 54, The test sets model.DB and
common.UsingSQLite but doesn't clear other DB engine flags, making
GetByUpstreamTaskId() behavior order-dependent; update the setup to save and
restore common.UsingPostgreSQL and common.UsingMySQL (alongside
common.UsingSQLite and model.DB) and in the test set only common.UsingSQLite =
true and the others = false, then restore all saved flags in the t.Cleanup
closure so GetByUpstreamTaskId() will reliably take the SQLite branch.
Comment on lines
+292
to
+299
| func otherRatiosProduct(ratios map[string]float64) float64 { | ||
| multiplier := 1.0 | ||
| for _, ra := range ratios { | ||
| if ra != 1.0 { | ||
| result *= ra | ||
| if ra != 1.0 && ra > 0 { | ||
| multiplier *= ra | ||
| } | ||
| } | ||
| return int(result) | ||
| return multiplier |
Contributor
There was a problem hiding this comment.
Make ratio multiplication deterministic before truncating.
ratios is a map, so otherRatiosProduct() can multiply the same factors in different orders. With floating-point math and the later int() cast, that can shift the computed quota by 1 on boundary cases.
Suggested fix
+import "sort"
...
func otherRatiosProduct(ratios map[string]float64) float64 {
+ keys := make([]string, 0, len(ratios))
+ for k := range ratios {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
multiplier := 1.0
- for _, ra := range ratios {
+ for _, k := range keys {
+ ra := ratios[k]
if ra != 1.0 && ra > 0 {
multiplier *= ra
}
}
return multiplier
}📝 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
| func otherRatiosProduct(ratios map[string]float64) float64 { | |
| multiplier := 1.0 | |
| for _, ra := range ratios { | |
| if ra != 1.0 { | |
| result *= ra | |
| if ra != 1.0 && ra > 0 { | |
| multiplier *= ra | |
| } | |
| } | |
| return int(result) | |
| return multiplier | |
| func otherRatiosProduct(ratios map[string]float64) float64 { | |
| keys := make([]string, 0, len(ratios)) | |
| for k := range ratios { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| multiplier := 1.0 | |
| for _, k := range keys { | |
| ra := ratios[k] | |
| if ra != 1.0 && ra > 0 { | |
| multiplier *= ra | |
| } | |
| } | |
| return multiplier | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/relay_task.go` around lines 292 - 299, otherRatiosProduct is
nondeterministic because it multiplies values by iterating a map; to avoid
floating-point rounding differences that change int-truncated quotas, collect
the values from ratios (or keys), sort the numeric values in a deterministic
order (e.g., ascending), then multiply them in that sorted order and return the
product; update the function otherRatiosProduct to build a slice of ra values,
sort.Float64s on that slice, and multiply only those >0 and !=1.0 so the
multiplication order is stable across runs.
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
pingxingshijie-assetfor routing and billing.Verification
go test -race ./middleware ./relay/channel/task/pingxingshijienode --experimental-strip-types src/features/usage-logs/lib/task-action-label.test.tsnpm run typecheckinweb/defaultnpm run buildinweb/defaultNotes
npm run lintstill fails on unrelated pre-existing files outside this patch.pingxingshijie-assetfor successful uploads.Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
Chores