feat: add Octopus managed site integration#442
Conversation
- Add Octopus as a new managed site type - Support Octopus channel CRUD operations - Support model sync for Octopus channels - Add Octopus settings page with username/password auth - Hide group/priority/weight fields for Octopus (different concepts) - Enable auto_sync by default when importing to Octopus - Add /v1 suffix to base URLs for Octopus imports - Use official Octopus logo for icon - Display correct Octopus channel types in channel list - Add Octopus channel management documentation
dosubot
bot
added
the
size:XXL
WalkthroughAdds first-class Octopus managed-site support across types, API client and auth manager, octopus service implementation, model sync/scheduler, preferences + migration, UI (selectors, ChannelDialog, settings, channels table), icons, docs/locales, and wiring into managed-site and API override flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (UI)
participant Prefs as UserPreferencesContext
participant Settings as OctopusSettings
participant Auth as OctopusAuthManager
participant OctoAPI as Octopus API
participant Channels as ManagedSiteChannels
User->>Settings: enter baseUrl/username/password
Settings->>Prefs: updateOctopusBaseUrl/Username/Password
Prefs-->>Settings: confirm saved
User->>Settings: click "Validate"
Settings->>Auth: validateConfig(config)
Auth->>OctoAPI: POST /api/v1/user/login
OctoAPI-->>Auth: JWT + expireAt
Auth->>Auth: cache token (baseUrl:username)
Auth->>OctoAPI: GET /api/v1/user/status (Bearer JWT)
OctoAPI-->>Auth: 200 OK
Auth-->>Settings: validation result
Settings-->>User: show toast
User->>Channels: open channels list
Channels->>Prefs: getManagedSiteConfig()
Prefs->>Auth: getValidToken(config)
Auth-->>Channels: token
Channels->>OctoAPI: listChannels (Bearer token)
OctoAPI-->>Channels: channels
Channels-->>User: render Octopus-aware table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related Documentation Checked 20 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/ChannelDialog/hooks/useChannelForm.ts (1)
233-240:⚠️ Potential issue | 🔴 Critical
formData.typedoes not acceptOctopusOutboundType—type mismatch detected.
handleTypeChangeacceptsChannelType | OctopusOutboundTypebutChannelFormData.typeis typed strictly asChannelType(types/newapi.ts:63). AssigningnewTypedirectly toformData.typeon line 236 violates type safety. Either updateChannelFormDatato accept the union type, or narrow the parameter toChannelTypeonly.
🤖 Fix all issues with AI agents
In `@entrypoints/options/pages/BasicSettings/components/OctopusSettings.tsx`:
- Around line 53-69: The handlers handleBaseUrlChange, handleUsernameChange and
handlePasswordChange save raw input while handleValidateConfig trims values
first; update each handler to trim the incoming value before calling
updateOctopusBaseUrl, updateOctopusUsername, and updateOctopusPassword (e.g.,
use url.trim(), username.trim(), password.trim()) so persisted values match
validated values and avoid storing leading/trailing whitespace.
In `@entrypoints/options/pages/ManagedSiteChannels/index.tsx`:
- Around line 180-186: The column visibility logic currently only hides the
"group" column when isOctopus is true; update both the initial columnVisibility
state and the useEffect so that when isOctopus is true it also hides "priority"
and "weight". Locate the initial state for columnVisibility and add priority:
false and weight: false (in addition to group), and in the useEffect that calls
setColumnVisibility((prev) => ({ ...prev, group: !isOctopus })), expand the
update to also set priority: !isOctopus and weight: !isOctopus so all three
columns are toggled based on isOctopus.
In `@services/apiService/octopus/auth.ts`:
- Around line 51-64: Before calling response.json() check response.ok and handle
non-JSON error bodies: after the fetch call (the response variable returned from
fetch) if !response.ok read response.text() (or attempt response.json() in a
try/catch) and throw a descriptive Error containing response.status and the
response body so you don't surface a SyntaxError; then only parse and validate
the JSON (the data variable) and return data.data as OctopusLoginResponse as
before. Ensure you reference the existing response, data, and
OctopusLoginResponse symbols and keep the existing throw when the JSON payload
lacks code === 200 or data.token.
In `@services/apiService/octopus/index.ts`:
- Around line 48-52: The current response wrapper returns data.data ?? data
which can leak envelope fields into the typed payload; update the return in the
function that constructs the wrapper (the code using the local variable data and
returning success/message/data) to return data.data ?? null (or explicitly
handle the two shapes) so callers get a clean T instead of the full envelope;
ensure the message still uses data.message || "success" and adjust callers or
types to handle nullable data where appropriate.
- Around line 32-53: The current fetch flow calls response.json() unguarded and
only checks Octopus payload fields, which will throw a SyntaxError for non-JSON
error pages; modify the logic that performs the request (the block using fetch,
response, and buildOctopusAuthHeaders) to first check response.ok (or
response.status) and inspect the Content-Type before calling response.json(); if
response.ok is false, read response.text() (or attempt json parse in a
try/catch) and throw a new Error that includes response.status,
response.statusText and the response body (or parsed message) so callers get a
meaningful error; ensure downstream code that checks data.success / data.code
runs only after safely obtaining parsed JSON.
In `@services/modelSync/octopusModelSync.ts`:
- Around line 90-155: The runForChannel function currently mutates the input
channel (channel.models = ...) and uses any for errors; change it to avoid
mutating the caller by computing and assigning normalizedModels.join(",") to a
local variable (e.g., newModelsCsv) and only pass newModelsCsv into
updateChannelModels or return values without writing back to channel.models, and
change the catch to use error: unknown and extract message/status via
getErrorMessage(error) from ~/utils/error (so replace lastError: any with
lastError: unknown and use getErrorMessage(lastError) for message and httpStatus
handling); keep the existing calls to fetchChannelModels, haveModelsChanged, and
updateChannelModels but ensure you return oldModels and newModels arrays (not
relying on mutated channel) and import getErrorMessage.
In `@services/modelSync/scheduler.ts`:
- Around line 201-219: The Octopus branch in listChannels does not validate
config like createService does and can leak raw auth/fetch errors; update
listChannels to call getManagedSiteConfig(userPrefs), verify the returned config
and cast to OctopusConfig (similar to createService's validation), and if config
is missing/invalid throw the same user-friendly error used by createService;
only then call octopusApi.listChannels(octopusConfig) and map with
octopusChannelToManagedSite so the UI receives consistent, friendly errors.
In `@services/octopusService/octopusService.ts`:
- Around line 406-434: findMatchingChannel currently compares channels' stored
base_urls to the raw accountBaseUrl, but created channels use
buildOctopusBaseUrl(accountBaseUrl) (adds "/v1"), so comparisons never match;
update findMatchingChannel to normalize the incoming accountBaseUrl with
buildOctopusBaseUrl before comparing (e.g., compute const normalizedBase =
buildOctopusBaseUrl(accountBaseUrl) and compare chBaseUrl === normalizedBase),
ensuring consistency with prepareChannelFormData/autoConfigToOctopus and
preserving the rest of the matching logic (channels, parseDelimitedList,
octopusChannelToManagedSite).
- Around line 137-167: Define an explicit composite type (e.g.,
OctopusChannelWithData = ManagedSiteChannel & { _octopusData: OctopusChannel })
and remove the inline type assertion in octopusChannelToManagedSite; change the
function signature to return OctopusChannelWithData and construct the returned
object without "as" casting so _octopusData is a real required field. Replace
any downstream ad-hoc casts (for example in
services/modelSync/octopusModelSync.ts) to use OctopusChannelWithData as the
return/parameter type so the contract is documented and the duplicate casts are
eliminated.
- Around line 210-214: The code is incorrectly double-casting channel.type from
CreateChannelPayload/ChannelType into OctopusOutboundType (silencing the
compiler without converting values); add a mapping function (e.g.,
mapChannelTypeToOctopusOutboundType or convertChannelTypeToOctopus) that takes
ChannelType (numeric 0–55) and returns the correct OctopusOutboundType enum
value (0–5) and use it when building the OctopusCreateChannelRequest (replace
the (channel.type as unknown as OctopusOutboundType) cast); ensure the mapper
handles unknown/unsupported ChannelType values by falling back to
DEFAULT_OCTOPUS_CHANNEL_FIELDS.type.
In `@services/userPreferences.ts`:
- Around line 227-228: Bump the preferences version and add a migration to
initialize the new octopus field: update CURRENT_PREFERENCES_VERSION from 11 to
12 in services/configMigration/preferences/preferencesMigration.ts and add a
migration function that detects missing userPreferences.octopus on older
versions and merges or sets DEFAULT_OCTOPUS_CONFIG into the UserPreferences
object (the same shape used by getPreferences() and deepOverride). Ensure the
migration function returns the updated preferences object and is registered in
the migrations map so existing users receive the octopus initialization during
migration.
🧹 Nitpick comments (12)
constants/octopus.ts (1)
7-50: Hardcoded Chinese strings indescriptionfields bypass i18n.The
descriptionvalues (e.g.,"OpenAI 聊天补全 API","火山引擎 API") are hardcoded in Chinese. If these descriptions are rendered in UI, they won't be localized for non-Chinese users. Consider using translation keys or removing descriptions if they're unused.Also, the label strings are duplicated between
OctopusOutboundTypeOptionsandOctopusOutboundTypeNames. This is consistent with the existingChannelTypeOptions/ChannelTypeNamespattern inconstants/newApi.ts, so flagging as optional.services/apiService/octopus/auth.ts (2)
86-136: ConcurrentgetValidTokencalls can trigger redundant logins.If multiple callers invoke
getValidTokensimultaneously when the cache is empty, each will independently attemptlogin()since there's no deduplication. This wastes requests and could cause rate-limiting issues. Consider adding a pending-login promise to coalesce concurrent attempts.Also, Line 126: if
response.expire_atis in an unexpected format,new Date(...).getTime()returnsNaN, causing the token to never be considered valid from cache — it will re-login every time. Consider adding a validity check.Sketch for login deduplication and expireAt guard
class OctopusAuthManager { private tokenCache: Map<string, TokenCacheEntry> = new Map() + private pendingLogins: Map<string, Promise<string>> = new Map() // In getValidToken, wrap the login section: + const cacheKey = this.getCacheKey(config.baseUrl, config.username) + const pending = this.pendingLogins.get(cacheKey) + if (pending) return pending + + const loginPromise = this.performLogin(config, cacheKey) + this.pendingLogins.set(cacheKey, loginPromise) + try { + return await loginPromise + } finally { + this.pendingLogins.delete(cacheKey) + } // For expireAt: const expireAt = new Date(response.expire_at).getTime() + if (Number.isNaN(expireAt)) { + logger.warn("Invalid expire_at from Octopus login response", { expire_at: response.expire_at }) + }
141-149: UsegetErrorMessage()utility in catch block.Per coding guidelines, catch blocks should use the
getErrorMessage()utility from~/utils/errorfor consistent error handling.Proposed fix
+import { getErrorMessage } from "~/utils/error" + async validateConfig(config: OctopusConfig): Promise<boolean> { try { await this.getValidToken(config) return true } catch (error) { - logger.error("Config validation failed", error) + logger.error("Config validation failed", getErrorMessage(error)) return false } }As per coding guidelines: "Use
getErrorMessage()utility from~/utils/errorfor error handling in catch blocks"tests/services/managedSiteService/managedSiteService.test.ts (1)
51-80: Missing test coverage for the new OCTOPUS site type routing.
getManagedSiteService()now has an OCTOPUS branch, but no test verifies that selectingOCTOPUSasmanagedSiteTyperoutes to the octopus service. Consider adding a test case similar to the existing NEW_API and VELOERA cases.entrypoints/options/pages/BasicSettings/components/ManagedSiteSelector.tsx (1)
23-24: Stale JSDoc comment.The comment still reads "New API or Veloera" — consider updating it to include Octopus.
services/managedSiteService.ts (1)
117-174: Consider using the pre-built service objects instead of re-wiring each method.The
octopusService.tsalready exports aManagedSiteService-compatible object (seeservices/octopusService/octopusService.tslines 511–527). The same likely applies to the other services. Instead of manually mapping each method in every branch, you could return the pre-built objects directly:if (siteType === OCTOPUS) { return { ...octopusService.octopusService, messagesKey } }This would reduce duplication and the risk of wiring mismatches. Low priority since the current pattern is pre-existing.
services/modelSync/octopusModelSync.ts (3)
173-209: Catch block in worker also usesany.Same guideline issue — use
catch (error: unknown)andgetErrorMessage(error)here.Proposed fix
- } catch (error: any) { + } catch (error: unknown) { logger.error("Unexpected error for channel", { channelId: channel.id, error, }) result = { channelId: channel.id, channelName: channel.name, ok: false, - message: error?.message || "Unexpected error", + message: getErrorMessage(error), attempts: maxRetries + 1, finishedAt: Date.now(), }As per coding guidelines: "Use
getErrorMessage()utility from~/utils/errorfor error handling in catch blocks".
70-85: DuplicatedhaveModelsChangedlogic.This is an exact copy of the private method
ModelSyncService.haveModelsChangedfromservices/modelSync/modelSyncService.ts(lines 412–427). Consider extracting it into a shared utility to keep a single source of truth.
22-29: Duck-typing convention with_octopusDatais consistently maintained, but type safety could be improved.The
_octopusDataproperty relies on an inline type assertion rather than being a declared part ofManagedSiteChannel. While the pattern is intentionally set byoctopusChannelToManagedSite()and consistently used only for Octopus channels, it bypasses TypeScript's type system and could become fragile if future code creates OctopusManagedSiteChannelobjects through other paths.Consider extending the
ManagedSiteChanneltype with an optional_octopusDatafield, or using a branded type / discriminated union to formalize this convention and let the compiler catch inconsistencies.services/apiService/octopus/index.ts (1)
76-89: Client-side search fetches the full channel list on every call.
searchChannelsloads all channels vialistChannels, then filters locally. If the Octopus backend supports a search/filter endpoint, prefer server-side filtering to reduce payload size and latency.services/modelSync/scheduler.ts (2)
387-472: Octopus sync path intentionally skips model-redirect mapping — add a comment.The non-Octopus
executeSyncflow (lines 270–377) appliesModelRedirectServicemappings per channel after sync. The Octopus path omits this entirely. If this is intentional, a brief comment in the method body would help future maintainers understand it's by design rather than an oversight.
206-208: Unchecked config extraction —getManagedSiteConfigmay return a config that's not anOctopusConfig.The
as OctopusConfigcast on line 208 (and again at line 399) trusts thatgetManagedSiteConfigreturns the right shape for the current site type. IfgetManagedSiteConfigreturnsundefinedor a config for a different site type due to a race or misconfiguration, the cast will silently produce an object missing required fields (username,password), leading to confusing runtime errors downstream.Consider adding a guard (e.g., check that
configis truthy and has expected Octopus fields) before proceeding.Proposed guard example
if (siteType === OCTOPUS) { const { config } = getManagedSiteConfig(userPrefs) - const octopusConfig = config as OctopusConfig + if (!config || !("username" in config)) { + throw new Error(t(`messages:${messagesKey}.configMissing`)) + } + const octopusConfig = config as OctopusConfig const channels = await octopusApi.listChannels(octopusConfig)
|
Preview build ready for commit Artifacts:
Download from workflow run: https://github.com/qixing-jk/all-api-hub/actions/runs/21904661834 This build is for review only and is not released. |
ChannelFormData.type was strictly typed as ChannelType, but handleTypeChange in useChannelForm accepts both ChannelType and OctopusOutboundType. This caused a type mismatch when assigning the new type value to formData.type. Extend the type field to accept the union type to support both New API/Veloera and Octopus channel type scenarios.
…lers Trim baseUrl, username, and password values in their respective change handlers to ensure persisted values match validated values and avoid storing leading/trailing whitespace. Fixes qixing-jk#442 (comment)
Octopus does not use group, priority, or weight concepts. Update both the initial columnVisibility state and the useEffect to hide all three columns when isOctopus is true. Fixes: qixing-jk#442 (comment)
Check response.ok before calling response.json() to avoid SyntaxError when server returns non-JSON error bodies (e.g., HTML error pages). Now properly extracts error message from JSON or falls back to text. Fixes: qixing-jk#442 (comment)
…chOctopusApi - Check response.ok before parsing JSON to handle HTTP errors properly - Inspect Content-Type header before calling response.json() - Read response.text() for non-JSON error pages (e.g., HTML 502 pages) - Include HTTP status, statusText, and response body in error messages - Wrap JSON parsing in try/catch to prevent SyntaxError on malformed responses Fixes: qixing-jk#442 (comment)
…is undefined The previous implementation of `fetchOctopusApi` used `responseData.data ?? data` which would fall back to the entire response object (containing success, message, etc.) when `responseData.data` was undefined. This leaked envelope fields into the typed payload. Now returns `null` when data is missing, ensuring callers get a clean `T | null` instead of a potentially polluted envelope object. Ref: qixing-jk#442 (comment)
- Remove mutation of channel.models in runForChannel - Change lastError and catch clause from any to unknown - Use getErrorMessage() for safe error message extraction - Use ApiError instanceof check for httpStatus extraction Fixes: qixing-jk#442 (comment)
Add config validation to Octopus branches in listChannels and executeSyncForOctopus methods, matching the validation pattern used in createService. This ensures user-friendly error messages when config is missing/invalid, instead of leaking raw auth/fetch errors. Fixes: qixing-jk#442 (comment)
- Add OctopusChannelWithData type in types/managedSite.ts extending NewApiChannel with required _octopusData field - Change octopusChannelToManagedSite return type to OctopusChannelWithData and remove inline type assertion - Add isOctopusChannelWithData type guard in octopusModelSync.ts to replace ad-hoc casts Fixes: qixing-jk#442 (comment)
Replace incorrect double-casting (channel.type as unknown as OctopusOutboundType) with a proper mapping function that: - Validates and preserves OctopusOutboundType values (0-5) from Octopus forms - Maps ChannelType values to corresponding OctopusOutboundType for future use - Falls back to DEFAULT_OCTOPUS_CHANNEL_FIELDS.type for unsupported types Fixes: qixing-jk#442 (comment)
Fixes channel matching by applying buildOctopusBaseUrl to normalize the incoming accountBaseUrl before comparison. This ensures consistency with prepareChannelFormData which adds /v1 suffix when creating channels. Closes: qixing-jk#442 (comment)
Bump CURRENT_PREFERENCES_VERSION from 11 to 12 and add migration function that initializes missing userPreferences.octopus field with DEFAULT_OCTOPUS_CONFIG for existing users during migration. Fixes qixing-jk#442 (review comment)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
types/newapi.ts (1)
63-73:⚠️ Potential issue | 🟠 MajorUpdateChannelPayload.type should be broadened to
ChannelType | OctopusOutboundTypefor type safety.The form spreads
ChannelFormData(which hastype: ChannelType | OctopusOutboundType) directly intoUpdateChannelPayloadinuseChannelForm.ts(line 285). However,UpdateChannelPayload.typeis typed only asChannelType, creating a type mismatch when Octopus channels are updated. The Octopus service silently handles this viamapChannelTypeToOctopusOutboundType, but the type definition should reflect the actual usage to maintain type safety across all services.
🤖 Fix all issues with AI agents
In `@entrypoints/options/pages/BasicSettings/components/OctopusSettings.tsx`:
- Around line 74-101: handleValidateConfig trims
localBaseUrl/localUsername/localPassword for validation but doesn't persist
those trimmed values, so validation can succeed while the stored inputs still
contain whitespace; update handleValidateConfig to persist the trimmed values
before calling octopusAuthManager.validateConfig (e.g. call the corresponding
state setters like setLocalBaseUrl(trimmedUrl),
setLocalUsername(trimmedUsername), setLocalPassword(trimmedPassword) or trigger
the same save/update routine you use for preferences) then proceed with the
validateConfig call and show toast results as before.
In `@services/apiService/octopus/auth.ts`:
- Around line 57-68: In the if (!response.ok) block, avoid double-consuming the
response stream by first reading the body as text (const bodyText = await
response.text()), then attempt to parse JSON from that text (try { const
errorJson = JSON.parse(bodyText); errorBody = errorJson.message || bodyText }
catch { errorBody = bodyText }), replacing the current response.json() /
response.text() sequence so the stream is only read once before throwing the
Error with the composed message.
- Around line 138-139: The code sets const expireAt = new
Date(response.expire_at).getTime() which can produce NaN for invalid formats and
causes getValidToken's cached.expireAt > Date.now() + bufferTime check to always
fail; fix by validating response.expire_at before using it (e.g., parse and
check Number.isFinite or !Number.isNaN on the timestamp), and if it's invalid
fall back to a safe expiry (such as Date.now() + defaultTTL or
response.expires_in if available) and emit a warning via the logger so the
function (getValidToken) uses a valid numeric expireAt instead of NaN.
In `@services/apiService/octopus/index.ts`:
- Around line 83-88: The current error check uses a falsy check that skips code:
0; update the conditional around responseData.code in the octopus API handling
so it detects explicit numeric codes including 0 — e.g., replace the
truthy-check pattern with an existence check like "responseData.code !==
undefined && responseData.code !== 200" (the block around responseData.success
and responseData.code in services/apiService/octopus/index.ts) so that a numeric
0 is treated as a non-200 error and triggers the throw.
- Around line 44-57: The current error-handling consumes the Fetch Response body
twice by calling response.json() then response.text(), which can fail if json
parsing throws; instead read the body once via await response.text() into a
temporary string (e.g., rawBody) and then attempt JSON.parse(rawBody) to extract
errorData.message/errorData.error or fall back to rawBody; update the octopus
index.ts block that references response.json(), response.text(), errorMessage
and errorData to use this single-read approach so you never call response.body
methods twice.
In `@services/modelSync/scheduler.ts`:
- Around line 397-491: Add a short clarifying comment at the top of
executeSyncForOctopus explaining that this Octopus-specific sync path
intentionally omits ModelRedirectService mappings (unlike executeSync) because
runOctopusBatch / octopusApi.updateChannel only update the model list directly
and Octopus channels initialize model_mapping as an empty string, so redirect
logic is not applicable; mention this is intentional and note where to look
(ModelRedirectService and executeSync) if redirect behavior is ever required.
🧹 Nitpick comments (4)
types/newapi.ts (1)
3-3: Use absolute import path per coding guidelines.The import uses a relative path
./octopuswhile the coding guidelines require the~/prefix for imports. Line 1 already uses~/constants.Proposed fix
-import type { OctopusOutboundType } from "./octopus" +import type { OctopusOutboundType } from "~/types/octopus"As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "Use absolute imports with~/prefix for imports (configured in tsconfig.json)".entrypoints/options/pages/BasicSettings/components/OctopusSettings.tsx (1)
97-98: UsegetErrorMessage()in catch block per coding guidelines.The catch block discards the error. Per project conventions, use the
getErrorMessage()utility to include error details in the toast.Proposed fix
- } catch { - toast.error(t("octopus.validation.error")) + } catch (error) { + toast.error(t("octopus.validation.error") + ": " + getErrorMessage(error)) } finally {Add the import:
+import { getErrorMessage } from "~/utils/error"As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "UsegetErrorMessage()utility from~/utils/errorfor error handling in catch blocks".services/modelSync/scheduler.ts (1)
206-217: Consider extracting the duplicated Octopus config retrieval and validation.The pattern of
getManagedSiteConfig → cast → validate baseUrl/username/password → throwis repeated identically inlistChannels(lines 206–217) andexecuteSyncForOctopus(lines 408–418). A small helper likegetValidatedOctopusConfig(prefs, messagesKey)would DRY this up and ensure validation stays consistent.Example helper
private getValidatedOctopusConfig( prefs: Awaited<ReturnType<typeof userPreferences.getPreferences>>, messagesKey: string, ): OctopusConfig { const { config } = getManagedSiteConfig(prefs) const octopusConfig = config as OctopusConfig if (!octopusConfig?.baseUrl || !octopusConfig?.username || !octopusConfig?.password) { throw new Error(t(`messages:${messagesKey}.configMissing`)) } return octopusConfig }Also applies to: 408-418
services/octopusService/octopusService.ts (1)
291-297:getErrorMessage()fallbacks are dead code.
getErrorMessage()(seeutils/error.tslines 5–23) always returns a non-empty string — it falls through toJSON.stringifyorString(error). The|| "Failed to ..."fallbacks on lines 295, 339, and 369 will never execute.Proposed cleanup
- message: getErrorMessage(error) || "Failed to create channel", + message: getErrorMessage(error),Apply the same pattern for the
updateChannelanddeleteChannelcatch blocks.Also applies to: 335-341, 365-371
…ndling - Broaden UpdateChannelPayload.type to ChannelType | OctopusOutboundType - Fix double response stream consumption in auth.ts and index.ts - Add expireAt validation with fallback TTL in auth.ts - Fix code:0 being skipped by truthy check in index.ts - Persist trimmed values in OctopusSettings validation - Add clarifying comment for Octopus sync omitting ModelRedirectService
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@services/apiService/octopus/auth.ts`:
- Around line 101-161: getValidToken currently only returns the token string and
only updates the in-memory tokenCache, so persistent storage isn't updated after
login; change getValidToken to return an object { token: string, expireAt:
number } (update its return type from Promise<string> to
Promise<{token:string,expireAt:number}>) and ensure it sets the parsed expireAt
before returning (the same expireAt stored in tokenCache); then update callers
to call updateOctopusConfig (or the existing persistence function) with the
returned token and expireAt so the persistent cache is kept in sync across
service worker restarts; keep tokenCache.set(cacheKey, { token: response.token,
expireAt }) as-is.
🧹 Nitpick comments (5)
types/newapi.ts (1)
3-3: Use absolute import with~/prefix.Per coding guidelines, imports should use the
~/prefix. This relative import should be~/types/octopus.-import type { OctopusOutboundType } from "./octopus" +import type { OctopusOutboundType } from "~/types/octopus"As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "Use absolute imports with~/prefix for imports (configured in tsconfig.json)".services/apiService/octopus/index.ts (2)
15-16: Use absolute imports with~/prefix for sibling module imports.-import { octopusAuthManager } from "./auth" -import { buildOctopusAuthHeaders, normalizeBaseUrl } from "./utils" +import { octopusAuthManager } from "~/services/apiService/octopus/auth" +import { buildOctopusAuthHeaders, normalizeBaseUrl } from "~/services/apiService/octopus/utils"As per coding guidelines,
**/*.{ts,tsx,js,jsx}: "Use absolute imports with~/prefix for imports".
112-114: UsegetErrorMessage()utility in catch blocks per coding guidelines.All catch blocks pass raw
errortologger.error. The coding guidelines require usinggetErrorMessage()from~/utils/errorfor error handling in catch blocks.Example fix for one site (apply similarly to all):
+import { getErrorMessage } from "~/utils/error" ... } catch (error) { - logger.error("Failed to list channels", error) + logger.error("Failed to list channels", getErrorMessage(error)) throw error }As per coding guidelines: "Use
getErrorMessage()utility from~/utils/errorfor error handling in catch blocks".Also applies to: 154-156, 179-180, 201-203, 226-228, 249-251
services/modelSync/scheduler.ts (2)
38-38: Use absolute import path.-import { octopusChannelToManagedSite } from "../octopusService/octopusService" +import { octopusChannelToManagedSite } from "~/services/octopusService/octopusService"As per coding guidelines: "Use absolute imports with
~/prefix for imports".
410-427: Config validation is duplicated betweenlistChannelsandexecuteSyncForOctopus.Lines 207-217 and 421-427 have identical Octopus config validation logic. Consider extracting a small helper like
getValidatedOctopusConfig(prefs, messagesKey)to reduce duplication.Suggested helper
+ private getValidatedOctopusConfig( + prefs: Awaited<ReturnType<typeof userPreferences.getPreferences>>, + messagesKey: string, + ): OctopusConfig { + const { config } = getManagedSiteConfig(prefs) + const octopusConfig = config as OctopusConfig + if ( + !octopusConfig?.baseUrl || + !octopusConfig?.username || + !octopusConfig?.password + ) { + throw new Error(t(`messages:${messagesKey}.configMissing`)) + } + return octopusConfig + }
Remove token persistence fields from OctopusConfig since Octopus tokens have short default expiry (15 minutes) and can be customized via login expire parameter. Memory-only caching in service worker is sufficient. - Remove cachedToken and tokenExpireAt from OctopusConfig - Remove storage cache check logic from getValidToken - Reduce buffer time from 5 min to 1 min (matches short token TTL) - Update default TTL fallback from 24h to 15min (Octopus default)
|
感谢PR，看着完成度挺高，等我部署一个 Octopus 测试一下功能。 |
谢谢提出来问题，首次PR不太清楚流程。 |
- Create utils/string.ts with parseDelimitedList and normalizeList - Add normalizeBaseUrl to utils/url.ts - Remove duplicate local functions from octopusService, newApiService, veloeraService - Remove unused functions from octopus API service: - checkStatus (auth.ts) - toggleChannelEnabled, triggerModelSync, getLastSyncTime (index.ts) - parseCommaSeparated, toCommaSeparated, normalizeBaseUrl (utils.ts)
|
/api/user/models 这两个端点的对应的功能函数可以考虑显式实现一下，如果没有也返回值避免会退到common发出不必要的请求 |
|
此外账号密码的验证部分，可以考虑检测到403的情况下，提示用户去检查octopus CORS 白名单配置，因为初次安装是会触发403 |
- Add fetchAvailableModels and fetchGroups with JWT authentication - Add fetchSiteUserGroups and fetchAccountAvailableModels with common API signatures - Register OCTOPUS in siteOverrideMap for proper API routing - Add 403 CORS configuration hint in auth error handling - Add i18n translations for corsError message
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@services/apiService/octopus/index.ts`:
- Around line 131-136: The filter can crash if ch.base_urls is undefined; update
the predicate used in channels.filter (where lowerKeyword is defined) to guard
the .some call by using optional chaining or a fallback so that
ch.base_urls?.some(...) (or an equivalent check) is used and returns false when
base_urls is missing; modify the predicate in the channels.filter callback that
references ch.base_urls to avoid calling .some on undefined.
🧹 Nitpick comments (4)
services/apiService/octopus/auth.ts (1)
146-153: UsegetErrorMessage()for the error logged invalidateConfig.Per coding guidelines, catch blocks should use the
getErrorMessage()utility from~/utils/error. Currently the rawerrorobject is passed tologger.error.Proposed fix
+import { getErrorMessage } from "~/utils/error" + ... async validateConfig(config: OctopusConfig): Promise<boolean> { try { await this.getValidToken(config) return true } catch (error) { - logger.error("Config validation failed", error) + logger.error("Config validation failed", { error: getErrorMessage(error) }) return false } }As per coding guidelines: "Use
getErrorMessage()utility from~/utils/errorfor error handling in catch blocks".services/apiService/octopus/index.ts (3)
106-119: UsegetErrorMessage()in catch blocks throughout the file.All catch blocks pass the raw
errorobject directly tologger.error(). Per coding guidelines, usegetErrorMessage()from~/utils/errorfor error handling in catch blocks. This ensures consistent, safe error serialization (e.g., iferroris not anErrorinstance).Example fix for one block (apply similarly to all):
Proposed fix (example for listChannels)
+import { getErrorMessage } from "~/utils/error" + ... } catch (error) { - logger.error("Failed to list channels", error) + logger.error("Failed to list channels", { error: getErrorMessage(error) }) throw error }As per coding guidelines: "Use
getErrorMessage()utility from~/utils/errorfor error handling in catch blocks".Also applies to: 142-161, 166-185, 190-208, 213-231, 267-280, 286-297
304-357: Duplicated config-extraction logic infetchSiteUserGroupsandfetchAccountAvailableModels.Both functions repeat the same pattern: read preferences → extract octopusConfig → validate three fields → call the inner function. Extract a shared helper to reduce duplication.
Proposed refactor
+async function getOctopusConfigFromPreferences(): Promise<OctopusConfig | null> { + const prefs = await userPreferences.getPreferences() + const octopusConfig = prefs?.octopus + if ( + !octopusConfig?.baseUrl || + !octopusConfig?.username || + !octopusConfig?.password + ) { + return null + } + return { + baseUrl: octopusConfig.baseUrl, + username: octopusConfig.username, + password: octopusConfig.password, + } +} + export async function fetchSiteUserGroups( _request: ApiServiceRequest, ): Promise<string[]> { try { - const prefs = await userPreferences.getPreferences() - const octopusConfig = prefs?.octopus - if ( - !octopusConfig?.baseUrl || - !octopusConfig?.username || - !octopusConfig?.password - ) { + const config = await getOctopusConfigFromPreferences() + if (!config) { logger.warn("Octopus config not available, returning empty groups") return [] } - return await fetchGroups({ - baseUrl: octopusConfig.baseUrl, - username: octopusConfig.username, - password: octopusConfig.password, - }) + return await fetchGroups(config) } catch (error) { ... } }
17-19: Use absolute~/imports to align with codebase patterns.Lines 17-19 use relative imports (
../common/type,./auth,./utils), but every other file inservices/apiService/consistently uses absolute imports with the~/prefix. This inconsistency should be resolved for consistency.Proposed fix
-import type { ApiServiceRequest } from "../common/type" -import { octopusAuthManager } from "./auth" -import { buildOctopusAuthHeaders } from "./utils" +import type { ApiServiceRequest } from "~/services/apiService/common/type" +import { octopusAuthManager } from "~/services/apiService/octopus/auth" +import { buildOctopusAuthHeaders } from "~/services/apiService/octopus/utils"
已完成 |
这个部分的错误信息并没有传递上UI的toast，还是一样的用户不可见，同时常见情况可以考虑自定义错误消息，其它的应该是服务器返回的结果优先，如无具体错误message则fallback到扩展提供的错误消息 |
- Change validateConfig to return { success, error? } instead of boolean
so the caller can display the actual error message
- Prioritize server-returned message in login errors; fall back to
HTTP status + body when no structured message is available
- On 403, append CORS whitelist hint after the server message
- OctopusSettings now shows the specific error in toast, falling back
to the generic i18n message only when no detail is available
Fix |
2 participants
Closes #449
Summary
Changes
新增文件
types/octopus.ts/types/octopusConfig.ts- 类型定义constants/octopus.ts- Octopus 常量services/apiService/octopus/- Octopus API 服务services/octopusService/- Octopus 服务实现services/modelSync/octopusModelSync.ts- 模型同步components/icons/OctopusIcon.tsx- 图标组件entrypoints/options/.../OctopusSettings.tsx- 设置页面docs/docs/octopus-channel-management.md- 用户文档修改文件
constants/siteType.ts- 添加 OCTOPUS 类型services/managedSiteService.ts- 添加 Octopus 路由services/modelSync/scheduler.ts- 支持 Octopus 模型同步components/ChannelDialog/- 适配 Octopus 字段Test Plan
Summary by CodeRabbit
New Features
Background Sync
API / Services
Documentation
Localization