Skip to content

fix: style #7774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shaohuzhang1 wants to merge 2 commits into from
Closed

fix: style #7774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30b17bd
fix: style
wangdan-fit2cloud Nov 9, 2026
3f0d88e
Merge branch 'v3' into pr@v3@fix_style
wangdan-fit2cloud Nov 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ui/src/components/global/mk-dropdown/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ defineExpose({ handleOpen, handleClose })
v-if="!hideWhenEmpty || hasDropdownContent(slots.dropdown?.())"
class="mk-dropdown"
ref="dropdownRef"
:popper-options="popperOptions"
:persistent="persistent && hasOpened"
v-bind="$attrs"
@visible-change="handleVisibleChange"

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a concise review:

  1. Function Call in Render (slots.dropdown?.()): Calling slots.dropdown during template rendering causes unnecessary re-evaluation on every parent update. Use <slot name="dropdown" /> in the template instead, or compute hasDropdownContent via a watcher/computed property that checks slot existence/length without invoking functions directly in v-if conditionals if avoidable.
  2. Attribute Order: Placing ref before props (popper-options) is fine, but ensure popperOptions is defined and reactive in the script setup to benefit from Vue 3’s automatic unwrapping.
  3. Optimization: If hasDropdownContent is expensive, memoize it or use computed. Avoid passing $attrs unless necessary to prevent prop leakage.

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the code snippet:

1. Potential Issues & Irregularities

  • Prop Binding vs. Component Definition: The code adds a new prop :popper-options="popperOptions". You must ensure that popperOptions is defined in the component's script (via ref, computed, or props) and that the underlying dropdown component (e.g., el-dropdown or a custom one) actually accepts a popper-options prop. If the library uses options or a different naming convention, this will be silently ignored.
  • Vue 3 Syntax Consistency: The syntax $attrs, v-if, and defineExpose indicates Vue 3. Ensure popperOptions is reactive if it needs to update dynamically.

2. Optimization Suggestions

  • Type Safety: If using TypeScript, define an interface for popperOptions to prevent runtime errors from incorrect configuration keys.
  • Default Values: If popperOptions is passed from a parent, consider defining a default value inside the component to prevent undefined errors if not provided.
  • Performance: If popperOptions is complex and static, use shallowRef or non-reactive constants to avoid unnecessary deep reactivity overhead.

Example Fix (Vue 3 Composition API Script):

<script setup lang="ts">
import { ref } from 'vue';

// Define popper options correctly
const popperOptions = ref({
  modifiers: [
    { name: 'offset', options: { offset: [0, 8] } }
  ],
});
</script>

Expand Down
3 changes: 2 additions & 1 deletion ...rc/views/application-detail/overview/display-setting/components/DisplaySettingPreview.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { computed } from 'vue'
import type { CSSProperties } from 'vue'
import type { ApplicationDisplaySetting } from '@/api/types'
import defaultFloatIconUrl from '/MaxKB.gif?url'

defineOptions({ name: 'DisplaySettingPreview' })

Expand Down Expand Up @@ -79,6 +80,6 @@ const previewHeaderStyle = computed<CSSProperties>(() => ({
</div>
</div>
<!-- 浮窗图标 -->
<img :src="previewImages.float_icon || './MaxKB.gif'" alt="" class="size-8 shrink-0 object-contain" />
<img :src="previewImages.float_icon || defaultFloatIconUrl" alt="" class="size-8 shrink-0 object-contain" />
</div>
</template>

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analysis & Suggestions

1. Naming Convention Inconsistency (Potential Issue)

  • Issue: The imported variable is named defaultFloatIconUrl (camelCase), but the fallback previously used was a root-relative path string '/MaxKB.gif'.
  • Risk: Ensure that /MaxKB.gif actually exists in your project's public folder. If you are relying on Vite/Webpack asset resolution, using a relative path like ../assets/MaxKB.gif is safer than an absolute root path unless you are certain of the build configuration.

2. Missing Null/Empty String Check (Optimization/Safety)

  • Issue: The expression previewImages.float_icon || defaultFloatIconUrl relies on falsy checking.
  • Risk: If previewImages.float_icon is an empty string "" or 0, the fallback will work as intended. However, if it's a valid non-empty string that is broken, the fallback won't trigger. For robustness, consider explicit checks if necessary: 
    :src="previewImages.float_icon?.trim() || defaultFloatIconUrl"

3. Alt Text Accessibility (Best Practice)

  • Suggestion: The alt="" makes the image effectively hidden from screen readers. If this image is purely decorative, this is correct. If the float icon conveys meaning (e.g., "Open Chat"), provide descriptive alt text: 
    alt="Chat assistant icon"

4. Import Path Portability (Minor Suggestion)

  • Suggestion: Using '/MaxKB.gif?url' suggests Vite-specific syntax (?url). Ensure this aligns with your bundler. If you want stricter type safety, ensure your IDE recognizes the .gif?url module declaration.

Summary of Recommended Change

The change from a hardcoded string to an imported module is good practice. It ensures the asset is resolved by the build tool and benefits from hashing/cache-busting. No critical bugs detected. Just verify the file path resolves correctly in your build environment.

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the code changes:

1. Import Path Issue (Potential Bug)

Irregularity: ./ vs /
The original fallback was './MaxKB.gif' (relative path). The new import uses import defaultFloatIconUrl from '/MaxKB.gif?url' with Vite's ?urlquery. In Vite, a leading/typically refers to the project root (whereindex.htmlandpublic/live), whereas./` in a module context is often interpreted differently depending on the bundler configuration or context.

If MaxKB.gif is not located at the root of your Vite project but rather inside src/assets or similar, this import will fail during build/dev server runtime. You likely need to verify the path. If the file is in src, it should probably be:

import defaultFloatIconUrl from '@/assets/MaxKB.gif?url' // or appropriate relative/path alias

2. Hardcoded Path in Import

Suggestion: While functional, hardcoding the filename directly in the import statement ('/MaxKB.gif?url') makes it less flexible than using an alias if the file location changes. If this asset is shared across multiple components, consider moving it to a constant or using a clear public URL if intended for the public folder.

3. Vue Reactivity & Optimization (No Change Needed, but Good Practice)

  • The change correctly moves from a string literal fallback in the template (|| './...') to a pre-imported variable. This is good. It avoids re-evaluating the fallback string in every render cycle and ensures the asset is handled by the bundler (hashed filenames for cache busting in production).
  • Using previewImages.float_icon || defaultFloatIconUrl remains efficient as previewImages.float_icon is presumably reactive.

4. Alt Attribute

Minor Observation: The <img> tag has alt="". For accessibility (a11y), if this icon is decorative, alt="" is correct. However, if it represents a "float window launcher," consider adding descriptive alt text like alt="Chat Launcher Icon" unless explicitly hidden from screen readers via ARIA attributes. This doesn't relate directly to the diff but is worth noting for overall quality.

✅ Recommended Fix (if path is incorrect)

Assuming MaxKB.gif is NOT at the Vite root (/public/MaxKB.gif), adjust the import path accordingly:

- import defaultFloatIconUrl from '/MaxKB.gif?url'
+ import defaultFloatIconUrl from '@/assets/images/MaxKB.gif?url' // Adjust path based on actual location

If it is in the public/ directory, ensure that /MaxKB.gif resolves correctly at runtime. Note that files in public/ are served as-is, so ?url might not be necessary (and could even cause issues if not expected by the dev server/prod server handling). In that case, just:

const defaultFloatIconUrl = '/MaxKB.gif'; // No import needed, use string directly or define const

But since you're using ?url, it's highly likely the file is inside src/, so use an alias or relative import accordingly.

1 change: 1 addition & 0 deletions ui/src/views/application/components/ButtonCreateApplication.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function handleRefresh() {
placement="bottom-end"
persistent
:class="{ 'w-full': $slots.trigger }"
:popper-options="fitTriggerWidth ? { modifiers: [{ name: 'computeStyles', options: { roundOffsets: false } }] } : undefined"
:popper-style="dropdownStyle"
:popper-class="popperClass"
@visible-change="handleVisibleChange"

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the code snippet, focusing on irregularities, potential issues, and optimization suggestions.

Potential Issues & Irregularities

  1. Overriding popper-options completely:

    • The expression :popper-options="fitTriggerWidth ? { ... } : undefined" replaces any existing or default popper-options. If other parts of your application rely on specific Popper.js modifiers (e.g., for positioning fixes, event listeners, or accessibility), they will be lost when fitTriggerWidth is truthy.
    • Recommendation: Instead of defining the entire object inline, merge this modifier into existing options using a computed property or a helper function that accepts existing options and appends this modifier.

  2. Hardcoded Modifier Name computeStyles:

    • While computeStyles is the correct modifier name in Popper.js v2+, if you are using an older version of Popper.js (v1.x) or a library that doesn't expose this structure directly, this might not work as expected. Ensure your dependency versions match this API.

  3. Repeated Evaluation:

    • If fitTriggerWidth is a prop or computed property, the ternary operator creates a new object every time the condition changes or re-renders. This can cause unnecessary re-renders or performance overhead in Popper/Popper-class components, especially if these components are inside large lists or frequently updated views.
    • Optimization: Cache the result in a computed property.

  4. Semantic Ambiguity fitTriggerWidth:

    • Disabling roundOffsets is often done to fix sub-pixel rendering gaps between the trigger and the popper. However, the variable name fitTriggerWidth suggests a width-related logic. Ensure that toggling roundOffsets is indeed the intended behavior for "fitting" the width, or if it's a side-effect fix for visual alignment.

Optimization Suggestions

✅ Recommended Refactor (Using Computed Property)

Move the logic out of the template to avoid creating new objects on every render and to improve readability.

In <script> (Vue Composition API or Options API):

// Example using Composition API (setup)
const popperOptions = computed(() => {
  // Base options from props or defaults
  const baseOptions = props.basePopperOptions || {};
  
  if (props.fitTriggerWidth) {
    return {
      ...baseOptions,
      modifiers: [
        ...(baseOptions.modifiers || []), 
        {
          name: 'computeStyles',
          options: {
            roundOffsets: false
          }
        }
      ]
    };
  }
  
  return baseOptions;
});

In Template:

<popper
  :popper-options="popperOptions"
  :popper-style="dropdownStyle"
  :popper-class="popperClass"
  @visible-change="handleVisibleChange"
/>

Summary of Actions

Issue Severity Action
Object recreated on render Medium Use computed() to cache popperOptions.
Overwriting all options High Merge new modifier with existing/default options.
Inline complex logic Low Move to script for readability and testing.
Dependency compatibility Low Verify Popper.js version supports computeStyles.roundOffsets.

Final Note

If fitTriggerWidth rarely changes (e.g., set once at mount), the performance impact is negligible. However, for maintainability and safety against future prop changes, wrapping it in a computed property is the best practice.

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the key issues and suggestions for the provided code snippet:

1. Invalid JavaScript Object Syntax

The value assigned to :popper-options is malformed. You have nested modifiers, but the syntax is incorrect. Specifically, options inside a modifier object should be passed differently depending on the library (likely Nuxt UI or Headless UI/Vue-Popper), but generally, standard Popper.js configuration requires a flat structure or specific nesting.

Current:

{ modifiers: [{ name: 'computeStyles', options: { roundOffsets: false } }] }

Note: While this looks syntactically correct as an object literal, roundOffsets is not a valid option for computeStyles. The correct property is gpuAcceleration or using applyStyles. To disable rounding, you typically need to use modifiers like offset or customize gpuAcceleration.

Actually, in Popper.js v2, there is no roundOffsets in computeStyles. If you want to prevent rounding, you often set gpuAcceleration: false or use custom modifiers. However, Vue’s template parser might also choke if this isn’t extracted into a computed property/method due to complexity.

2. Performance & Maintainability

Inlining complex configuration objects directly in the template can lead to:

  • Harder-to-read templates.
  • Potential reactivity issues if not handled carefully.
  • Difficulty testing logic separately from UI.

3. Suggested Improvement

✅ Extract to a Computed Property

Move the logic out of the template for clarity and potential caching (though Vue may optimize it, explicit control is better):

<script setup>
// ...
const fitTriggerWidth = ref(false) // example state

const popperOptions = computed(() => 
  fitTriggerWidth.value 
    ? { 
        modifiers: [
          {
            name: 'computeStyles',
            options: {
              gpuAcceleration: false, // Common way to reduce layout shift/jumpiness
            },
          },
        ],
      }
    : undefined
)
</script>

<template>
  <component
    :popper-options="popperOptions"
    :popper-style="dropdownStyle"
    :popper-class="popperClass"
    @visible-change="handleVisibleChange"
  />
</template>

✅ Fix Popper Options (If Using Vuetify/Nuxt UI)

If you're trying to ensure the dropdown matches trigger width, many libraries (like Nuxt UI) provide built-in props (e.g., matchTriggerWidth or fit) that are more reliable than manually tweaking Popper modifiers. Check your component library docs first — you might not need :popper-options at all.

Summary of Actions:

  1. Verify library-specific props: Use built-in features like matchTriggerWidth if available instead of raw Popper configs.
  2. Extract logic: Move inline objects to computed properties.
  3. Validate Popper options: Ensure roundOffsets is actually supported; likely you meant gpuAcceleration: false or similar.

Expand Down
1 change: 1 addition & 0 deletions ui/src/views/knowledge/components/ButtonCreateKnowledge.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ function handleImportCreate(file: UploadFile) {
placement="bottom-end"
persistent
:class="{ 'w-full': $slots.trigger }"
:popper-options="fitTriggerWidth ? { modifiers: [{ name: 'computeStyles', options: { roundOffsets: false } }] } : undefined"
:popper-style="dropdownStyle"
:popper-class="popperClass"
@visible-change="handleVisibleChange"
Expand Down
1 change: 1 addition & 0 deletions ui/src/views/tool/components/ButtonCreateTool.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ function handleRefresh() {
placement="bottom-end"
persistent
:class="{ 'w-full': $slots.trigger }"
:popper-options="fitTriggerWidth ? { modifiers: [{ name: 'computeStyles', options: { roundOffsets: false } }] } : undefined"
:popper-style="dropdownStyle"
:popper-class="popperClass"
@visible-change="handleVisibleChange"
Expand Down
Loading