[shaohuzhang1]

feat: chat theme

[shaohuzhang1]

Overall, the code is functionally reasonable, but there are a few edge cases and architectural concerns worth addressing.

Potential issues

1. SSR / non-browser safety

   • document.documentElement.style is accessed directly inside the store action.
   • This will throw in SSR, some tests, or non-browser environments.
   • Add a guard:

     if (typeof document === 'undefined') return

2. DEFAULT_THEME_COLOR is assumed valid

   • User input is validated with /^#[\da-f]{6}$/, but the default constant is not validated.
   • If DEFAULT_THEME_COLOR is ever shorthand like #fff, uppercase, or invalid, the RGB parsing can produce NaN, and the default-gradient comparison can behave inconsistently.
   • Safer approach: normalize/validate the default too, or enforce that it is always a lowercase 6-digit hex string.

3. Case-sensitive default comparison

   • Input is lowercased, but DEFAULT_THEME_COLOR is compared directly:

     if (this.themeColor === DEFAULT_THEME_COLOR)

   • If DEFAULT_THEME_COLOR is uppercase, e.g. #0052D9, and the user sets #0052d9, the code will treat it as non-default and set gradient variables even though it is the same color.
   • Compare normalized values:

     const defaultColor = DEFAULT_THEME_COLOR.toLowerCase()

4. Gradient behavior for custom colors may be intentional but is worth confirming

   • For custom colors, both gradient variables are set to the same color:

     rootStyle.setProperty('--mk-primary-gradient', this.themeColor)
     rootStyle.setProperty('--mk-primary-gradient-end', this.themeColor)

   • This effectively removes the “blue-purple” gradient.
   • If the intent is to keep a gradient, you may want to derive a gradient or only override the primary color. If the intent is to flatten the gradient, this is fine.

5. Initial CSS variables are not applied on store creation

   • The store initializes themeColor, but setTheme is not called automatically.
   • If the application relies on these CSS variables and has no CSS fallback, the initial theme may not be applied until setTheme is called.
   • Call it during app initialization or ensure CSS defines fallback values.

6. Store has direct DOM side effects

   • This makes the store harder to unit test and couples state management to the DOM.
   • Consider extracting DOM application into a helper, composable, plugin, or store subscription. This is not a bug, but it improves separation of concerns.

7. Only supports 6-digit hex colors

   • #fff, #ffffffff, rgb(...), hsl(...), etc. are rejected.
   • That may be intentional, but it should be documented as a contract.

Optimization suggestions

• Extract a color-normalization helper.
• Extract the hex-to-RGB conversion.
• Normalize/validate DEFAULT_THEME_COLOR.
• Use a named regex constant.
• Keep the DOM guard.
• resetTheme() is fine, but for clarity you could write:

  this.setTheme(DEFAULT_THEME_COLOR)

Example refactor sketch

const HEX_COLOR_RE = /^#[0-9a-f]{6}$/
const DEFAULT_COLOR = DEFAULT_THEME_COLOR.toLowerCase()

function normalizeColor(color?: string): string {
  const normalized = color?.trim().toLowerCase()
  return normalized && HEX_COLOR_RE.test(normalized) ? normalized : DEFAULT_COLOR
}

function hexToRgbChannels(hex: string): string {
  const value = hex.slice(1)
  const r = Number.parseInt(value.slice(0, 2), 16)
  const g = Number.parseInt(value.slice(2, 4), 16)
  const b = Number.parseInt(value.slice(4, 6), 16)
  return `${r} ${g} ${b}`
}

export const useChatThemeStore = defineStore('chat-theme', {
  state: () => ({ themeColor: DEFAULT_COLOR }),

  actions: {
    setTheme(themeColor?: string) {
      const color = normalizeColor(themeColor)
      this.themeColor = color

      if (typeof document === 'undefined') return

      const rootStyle = document.documentElement.style
      rootStyle.setProperty('--mk-primary', color)
      rootStyle.setProperty('--mk-primary-rgb', hexToRgbChannels(color))

      if (color === DEFAULT_COLOR) {
        rootStyle.removeProperty('--mk-primary-gradient')
        rootStyle.removeProperty('--mk-primary-gradient-end')
      } else {
        rootStyle.setProperty('--mk-primary-gradient', color)
        rootStyle.setProperty('--mk-primary-gradient-end', color)
      }
    },

    resetTheme() {
      this.setTheme(DEFAULT_COLOR)
    },
  },
})

No major functional bug is present if DEFAULT_THEME_COLOR is guaranteed to be a lowercase #rrggbb value and the store is only used client-side. The main improvements are SSR safety, default-color normalization, and separating DOM side effects from store state management.

[shaohuzhang1]

Main concern: fetchAuthProfile now mixes data fetching with a theme side effect. That makes the action less predictable and can create race conditions.

Potential issues

1. Side effect in a fetch action

   • fetchAuthProfile is named/typed like a pure fetch, but now mutates global theme state.
   • Any caller that only needs the profile will also accidentally change the theme.
   • Prefer applying the theme in the guard after the scope is confirmed, or add an explicit option such as applyTheme.

2. Race condition / out-of-order responses

   • If fetchAuthProfile('A') is started, then fetchAuthProfile('B') starts and resolves first, A can resolve later and overwrite the theme with A’s theme.
   • Guard the side effect, e.g. only apply when the requested scope is still the current scope:

     if (scope === this.scope) {
       useChatThemeStore().setTheme(authProfile.custom_theme?.theme_color)
     }

   • If this.scope is updated after this call, move the theme application to the guard after setScope.

3. Repeated cached calls still reapply the theme

   • Even when scope === this.scope && this.authProfile reuses the cached profile, the theme is applied again.
   • If the user can manually change the theme, later calls to fetchAuthProfile may unexpectedly reset it.
   • Consider applying the theme only when the profile is first loaded or when the scope changes.

4. Undefined theme value

   • authProfile.custom_theme?.theme_color may be undefined.
   • Check setTheme behavior:
     • If it ignores undefined, stale themes from the previous scope may remain.
     • If it resets to default, make that behavior explicit.
   • Safer:

     const themeColor = authProfile.custom_theme?.theme_color
     if (themeColor) {
       useChatThemeStore().setTheme(themeColor)
     }

     or explicitly apply a default/reset value if that is intended.

5. Possible circular dependency

   • auth now imports theme. Verify that theme does not import auth, otherwise circular imports can cause initialization issues.

6. No in-flight request deduplication

   • Concurrent calls for the same scope before authProfile is cached will trigger multiple API requests.
   • Optional optimization: cache the pending promise per scope.

7. Readability

   • The nested ternary is harder to read. async/await would be clearer:

     async fetchAuthProfile(scope: string): Promise<AuthProfile<unknown>> {
       const authProfile =
         scope === this.scope && this.authProfile
           ? this.authProfile
           : isPortalScope(scope)
             ? await ChatAuthApi.getPortalAuthProfile()
             : await ChatAuthApi.getApplicationAuthProfile(scope)
     
       if (scope === this.scope) {
         useChatThemeStore().setTheme(authProfile.custom_theme?.theme_color)
       }
     
       return authProfile
     }

Recommendation

Keep fetchAuthProfile focused on fetching/caching, and apply the theme in the caller/guard where the current scope is known. If the side effect must stay here, guard it by current scope, handle undefined theme explicitly, and avoid reapplying it on cached calls unless that is intentional.

[shaohuzhang1]

Overall, the diff is mostly formatting/import-style cleanup and does not change the main logic. However, there are a few points worth checking:

Potential issues

1. border-N300 was removed

   - <div class="mk-gray-card-lg space-y-4 border border-N300">
   + <div class="mk-gray-card-lg space-y-4 border">

   • If border-N300 was a valid design token, this is a visual regression: the border color will change to the default/inherited color.
   • If border-N300 was invalid or no longer used, this is fine, but prefer using a valid token such as border-neutral-300 or the project’s actual token.
   • Recommendation: Restore border-N300 unless the color change is intentional.

2. Long template lines may conflict with the formatter/linter

   <MkDynamicsForm ref="dynamicsFormRef" :view="..." :render-data="..." v-model="form_data" />
   <el-button :type="..." :disabled="..." @click="submit">

   • The original code was wrapped across multiple lines; this diff makes the lines longer.
   • If the project uses Prettier or vue/max-attributes-per-line, this may fail formatting/lint checks.
   • Recommendation: Run the project formatter and keep the formatted output.

3. Duplicate readonly condition

   :view="is_submit || READONLY_VIEWS.includes(viewType)"
   :disabled="is_submit || READONLY_VIEWS.includes(viewType) || !sendMessage"

   • The same logic is repeated. Extracting it into a computed property reduces duplication and keeps the form and button in sync.
   • Assuming is_submit and viewType are refs:

   const isReadonly = computed(
     () => is_submit.value || READONLY_VIEWS.includes(viewType.value)
   )

   Then:

   <MkDynamicsForm :view="isReadonly" ... />
   <el-button :disabled="isReadonly || !sendMessage" ...>

   • This is a readability/maintainability improvement, not a functional bug.

4. Duplicate editorId on two MdPreview components

   <MdPreview editorId="conversation-text" ... />
   ...
   <MdPreview editorId="conversation-text" ... />

   • If editorId is used as a DOM id or editor instance key, using it twice may cause conflicts.
   • Recommendation: Use unique IDs, for example:

   editorId="conversation-leading-text"
   editorId="conversation-trailing-text"

   • This issue already existed, but it is worth fixing.

5. submit should also guard against readonly/duplicate submission

   • The button is disabled, but defensive code inside submit is still useful:

   if (isReadonly.value || !sendMessage) return

   • If submit performs an async request, ensure it cannot be triggered twice while pending. Set a submitting flag before await and reset it in finally.

Minor suggestions

• The import quote change from " to ' is fine and likely consistent with project style.
• Removing the blank line before </script> is harmless.
• Snake_case names such as is_submit, form_data, and form_field_list are inconsistent with typical TS/Vue camelCase style, but this is only a style issue if the project enforces it.
• READONLY_VIEWS.includes(...) is fine for two items. If the list grows significantly, consider a Set<ViewType>:

  const READONLY_VIEWS = new Set<ViewType>([VIEW_TYPE.LOG, VIEW_TYPE.SHARE])
  // READONLY_VIEWS.has(viewType.value)

Most important action

Check whether removing border-N300 is intentional. If not, restore it. Then run the project formatter and consider extracting the repeated readonly condition into a computed property.