Skip to content

perf: Resource management filters optimization #3815

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

Merged
wangdan-fit2cloud merged 1 commit into from
Aug 5, 2025
Merged

perf: Resource management filters optimization #3815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 41 additions & 18 deletions ui/src/views/system-resource-management/ApplicationResourceIndex.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
<template #header>
<div>
<span>{{ $t('common.status.label') }}</span>
<el-popover :width="200" trigger="click" :visible="statusVisible">
<el-popover :width="100" trigger="click" :visible="statusVisible">
<template #reference>
<el-button
style="margin-top: -2px"
Expand All @@ -109,19 +109,17 @@
<div class="filter">
<div class="form-item mb-16">
<div @click.stop>
<el-scrollbar height="300" style="margin: 0 0 0 10px">
<el-checkbox-group
v-model="statusArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in statusOptions"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</el-scrollbar>
<el-checkbox-group
v-model="statusArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in statusOptions"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</div>
</div>
</div>
Expand Down Expand Up @@ -177,21 +175,28 @@
</el-button>
</template>
<div class="filter">
<div class="form-item mb-16">
<div class="form-item mb-16 ml-4">
<div @click.stop>
<el-scrollbar height="300" style="margin: 0 0 0 10px">
<el-input
v-model="filterText"
:placeholder="$t('common.search')"
prefix-icon="Search"
clearable
/>
<el-scrollbar height="300" v-if="filterData.length">
<el-checkbox-group
v-model="workspaceArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in workspaceOptions"
v-for="item in filterData"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</el-scrollbar>
<el-empty v-else :description="$t('common.noData')" />
</div>
</div>
</div>
Expand Down Expand Up @@ -289,7 +294,7 @@
</template>

<script lang="ts" setup>
import { onMounted, ref, reactive, computed } from 'vue'
import { onMounted, ref, reactive, computed, watch } from 'vue'
import { useRouter, useRoute } from 'vue-router'
import ApplicationResourceApi from '@/api/system-resource-management/application'
import { t } from '@/locales'
Expand Down Expand Up @@ -427,10 +432,28 @@ const statusOptions = ref<any[]>([
},
])

const filterText = ref('')
const filterData = ref<any[]>([])

watch(
[() => workspaceOptions.value, () => filterText.value],
() => {
if (!filterText.value.length) {
filterData.value = workspaceOptions.value
}
filterData.value = workspaceOptions.value.filter((v: any) =>
v.label.toLowerCase().includes(filterText.value.toLowerCase()),
)
},
{ immediate: true },
)


function filterWorkspaceChange(val: string) {
if (val === 'clear') {
workspaceArr.value = []
}
filterText.value = ''
getList()
workspaceVisible.value = false
}
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Aug 5, 2025

Choose a reason for hiding this comment

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

Code Review and Optimizations

General Improvements:

  1. Import Statements: The import statements seem to be correctly set up with @/api/system-resource-management/application and @/locales.

  2. Vue Functionality:

    • Ensure that all Vue hooks (onMounted, ref, etc.) are properly imported under their respective namespaces (e.g., vue, vue-router).

  3. Conditional Rendering and Logic:

    • Use v-if, v-else-if, and v-else for better clarity in conditional rendering.
    • Consider using logical operators instead of nested checks where possible.

  4. Component Communication:

    • Ensure that the component's lifecycle methods (createEffect, setup) are used correctly.
    • Avoid mixing functional and non-functional components when it can lead to confusion or bugs.

Potential Issues and Suggestions:

  1. Performance Check:

    • If there is a large number of items being rendered, consider optimizing the checkbox list display or adding virtual scrolling.

  2. Validation:

    • Always validate form inputs before performing API requests to prevent errors.

  3. Security:

    • Ensure sensitive data is handled securely, especially if interacting with APIs.

  4. Code Readability:

    • Add comments near complex logic blocks to improve readability.
    • Refactor repetitive code into helper functions whenever applicable.

  5. Translation Support:

    • Verify that translations work as expected in various languages and locales.

  6. State Management:

    • Use Vuex or Pinia for managing global state if needed, rather than relying heavily on reactive.
    • Ensure proper state management practices follow best practices.

Overall, the code looks well-structured and follows recommended Vue.js patterns. Minor adjustments could improve performance and maintainability based on additional context and usage details provided.

Expand Down
33 changes: 29 additions & 4 deletions ui/src/views/system-resource-management/KnowledgeResourceIndex.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,21 +98,28 @@
</el-button>
</template>
<div class="filter">
<div class="form-item mb-16">
<div class="form-item mb-16 ml-4">
<div @click.stop>
<el-scrollbar height="300" style="margin: 0 0 0 10px">
<el-input
v-model="filterText"
:placeholder="$t('common.search')"
prefix-icon="Search"
clearable
/>
<el-scrollbar height="300" v-if="filterData.length">
<el-checkbox-group
v-model="workspaceArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in workspaceOptions"
v-for="item in filterData"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</el-scrollbar>
<el-empty v-else :description="$t('common.noData')" />
</div>
</div>
</div>
Expand Down Expand Up @@ -245,7 +252,7 @@
</template>

<script lang="ts" setup>
import { onMounted, ref, reactive, computed } from 'vue'
import { onMounted, ref, reactive, computed, watch } from 'vue'
import { useRouter, useRoute } from 'vue-router'
import KnowledgeResourceApi from '@/api/system-resource-management/knowledge'
import UserApi from '@/api/user/user'
Expand Down Expand Up @@ -349,10 +356,28 @@ function reEmbeddingKnowledge(row: any) {
const workspaceOptions = ref<any[]>([])
const workspaceVisible = ref(false)
const workspaceArr = ref<any[]>([])

const filterText = ref('')
const filterData = ref<any[]>([])

watch(
[() => workspaceOptions.value, () => filterText.value],
() => {
if (!filterText.value.length) {
filterData.value = workspaceOptions.value
}
filterData.value = workspaceOptions.value.filter((v: any) =>
v.label.toLowerCase().includes(filterText.value.toLowerCase()),
)
},
{ immediate: true },
)

function filterWorkspaceChange(val: string) {
if (val === 'clear') {
workspaceArr.value = []
}
filterText.value = ''
getList()
workspaceVisible.value = false
}
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Aug 5, 2025

Choose a reason for hiding this comment

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

Potential Improvements

  1. Dynamic Filter Logic: The dynamic filtering logic in the watch hook is redundant because it already happens when filterText.value changes. This can be improved by simplifying the logic.

    watch(() => workspaceOptions.value, ([newOptions]) => {
  filterData.value = newOptions;
}, { immediate: true });

  2. Conditional Rendering: The presence of an "Empty Data" message should be based solely on whether filterData.length is zero rather than checking its value against another property (!filterText.length). Additionally, there's redundancy in always showing ElScrollbar. Only show it if you have data to display.

    const displayScrollbar = computed(() => !!filterData.value.length);

3. **Type Definitions**: Ensure that all properties and values used are properly typed, especially since TypeScript is being utilized in your project. However, these corrections can typically be resolved with proper type definitions which seem to not be part of this provided snippet.

4. **Code Readability**: While minimal in size, some lines might benefit from clearer naming or breaking down into smaller helper functions for better maintainability.

Here's what your updated code could look like:

```typescript
// ...
const displayScrollbar = computed(() => !!filterData.value.length);

return {
  // other variables & methods...

  watch: {
    [() => workspaceOptions.value]: ([newOptions]) => {
      filterData.value = newArray;
    },
  },

  created() {
    getList();
  },
};
</script>

Note that this example assumes that getList() returns a promise-like object or that getList method itself handles asynchronous calls internally so updating the UI once data arrives appropriately. Adjustments would need to match how exactly you're integrating API requests here.

Expand Down
31 changes: 27 additions & 4 deletions ui/src/views/system-resource-management/ModelResourceIndex.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,28 @@
</el-button>
</template>
<div class="filter">
<div class="form-item mb-16">
<div class="form-item mb-16 ml-4">
<div @click.stop>
<el-scrollbar height="300" style="margin: 0 0 0 10px">
<el-input
v-model="filterText"
:placeholder="$t('common.search')"
prefix-icon="Search"
clearable
/>
<el-scrollbar height="300" v-if="filterData.length">
<el-checkbox-group
v-model="workspaceArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in workspaceOptions"
v-for="item in filterData"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</el-scrollbar>
<el-empty v-else :description="$t('common.noData')" />
</div>
</div>
</div>
Expand Down Expand Up @@ -230,7 +237,7 @@
</template>

<script lang="ts" setup>
import { onBeforeMount, onMounted, ref, reactive, nextTick, computed } from 'vue'
import { onBeforeMount, onMounted, ref, reactive, watch, computed } from 'vue'
import type { Provider, Model } from '@/api/type/model'
import EditModel from '@/views/model/component/EditModel.vue'
import ParamSettingDialog from '@/views/model/component/ParamSettingDialog.vue'
Expand Down Expand Up @@ -314,6 +321,22 @@ const getRowProvider = computed(() => {
}
})

const filterText = ref('')
const filterData = ref<any[]>([])

watch(
[() => workspaceOptions.value, () => filterText.value],
() => {
if (!filterText.value.length) {
filterData.value = workspaceOptions.value
}
filterData.value = workspaceOptions.value.filter((v: any) =>
v.label.toLowerCase().includes(filterText.value.toLowerCase()),
)
},
{ immediate: true },
)

function filterWorkspaceChange(val: string) {
if (val === 'clear') {
workspaceArr.value = []
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Aug 5, 2025

Choose a reason for hiding this comment

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

Potential Issues and Optimization Suggestions:

  1. Repetition of Code: The el-checkbox group is rendered within both visible data (workspaceOptions) and hidden placeholder text when filterData is empty. This could lead to redundant rendering.

  2. Unnecessary Use of ref: Although not incorrect, using a ref for a simple state like filterText can be avoided if it's used directly in template expressions without being modified elsewhere.

  3. Duplicate Logic: The logic inside watch that updates filterData mirrors part of the initial filtering logic in getFilteredWorkspaces, which introduces redundancy but does not seem necessary unless there's additional functionality in mind.

  4. Optimization Suggestion:

    Given these points, removing unnecessary elements and states such as the placeholder divs or direct use of refs might increase performance slightly, especially in complex components with heavy usage. Consider simplifying the JSX structure in the component templates where this issue occurs.

By addressing these suggestions, you'll improve the overall efficiency and cleanliness of your Vue.js component while maintaining functional correctness.

Expand Down
31 changes: 27 additions & 4 deletions ui/src/views/system-resource-management/ToolResourceIndex.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,21 +119,28 @@
</el-button>
</template>
<div class="filter">
<div class="form-item mb-16">
<div class="form-item mb-16 ml-4">
<div @click.stop>
<el-scrollbar height="300" style="margin: 0 0 0 10px">
<el-input
v-model="filterText"
:placeholder="$t('common.search')"
prefix-icon="Search"
clearable
/>
<el-scrollbar height="300" v-if="filterData.length">
<el-checkbox-group
v-model="workspaceArr"
style="display: flex; flex-direction: column"
>
<el-checkbox
v-for="item in workspaceOptions"
v-for="item in filterData"
:key="item.value"
:label="item.label"
:value="item.value"
/>
</el-checkbox-group>
</el-scrollbar>
<el-empty v-else :description="$t('common.noData')" />
</div>
</div>
</div>
Expand Down Expand Up @@ -269,7 +276,7 @@
</template>

<script lang="ts" setup>
import { onMounted, ref, reactive, computed } from 'vue'
import { onMounted, ref, reactive, computed, watch } from 'vue'
import { cloneDeep } from 'lodash'
import InitParamDrawer from '@/views/tool/component/InitParamDrawer.vue'
import ToolResourceApi from '@/api/system-resource-management/tool'
Expand Down Expand Up @@ -444,6 +451,22 @@ async function changeState(row: any) {
}
}

const filterText = ref('')
const filterData = ref<any[]>([])

watch(
[() => workspaceOptions.value, () => filterText.value],
() => {
if (!filterText.value.length) {
filterData.value = workspaceOptions.value
}
filterData.value = workspaceOptions.value.filter((v: any) =>
v.label.toLowerCase().includes(filterText.value.toLowerCase()),
)
},
{ immediate: true },
)

function filterWorkspaceChange(val: string) {
if (val === 'clear') {
workspaceArr.value = []
Expand Down
Loading