Skip to content

feat(app): fix sync custom app failed #8310

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
f2c-ci-robot merged 1 commit into from
Apr 3, 2025
Merged

feat(app): fix sync custom app failed #8310

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
2 changes: 2 additions & 0 deletions frontend/src/api/interface/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ export namespace App {
tags?: string[];
type?: string;
recommend?: boolean;
resource?: string;
showCurrentArch?: boolean;
}

export interface AppParams {
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The provided code snippet appears to be a part of an interface definition using TypeScript for an application configuration object. The main changes include adding two new properties: resource and showCurrentArch. Here's a concise review of these additions:

  1. New Property:

    resource?: string;
    • This property allows you to assign an optional string value to indicate the source or URI of some resource associated with the app.

  2. Another New Property:

    showCurrentArch?: boolean;
    • This property is also optional and accepts a boolean. It indicates whether the current architecture information should be shown alongside other details.

Potential Issues:

  • No apparent issues are present based solely on this excerpt.
  • However, further consideration might be needed depending on how these fields will be used within larger applications, especially concerning their usage and validation.

Optimization Suggestions:

  • Since both properties (resource and showCurrentArch) are optional, it's reasonable to ensure that there isn't excessive redundancy in your codebase where similar options are defined more than once without considering which ones are actually required by different use cases.

Overall, the additions appear functional unless additional context provides clues about their intended roles and constraints within specific parts of the application.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The provided code snippet has been edited with two new fields added to an existing interface within a TypeScript class named App. Let's review these changes:

  1. Tag Field: Introduced the ability to specify tags for resources under this namespace. This is useful for categorization or filtering purposes.

    export interface Resource {
    id: number;
    name: string;
    description?: string;
    tag?: string[];
}

  2. Resource Type Specification: Added an optional field type to differentiate between different types of resources (e.g., article, video, document).

    export interface Resource {
    id: number;
    name: string;
    description?: string;
    type?: 'article' | 'video' | 'document';
}

  3. Recommendation Flag: Included an optional boolean field recommend, which may be used to mark recommended resources. However, there might not be clear usage defined for this flag.

  4. Resource URL: A new field resource was added, but it doesn't seem to fit logically with the overall structure of the interface without further context about what kind of data it would hold. If resource points to another entity such as an API endpoint or unique identifier, additional details should explain its purpose.

  5. Show Current Architecture Indicator: Introducing showCurrentArch allows you to indicate whether certain functionalities should display information related to the current architecture setup.

Optimization Suggestions:

  • Ensure that the properties like tags, type, recommend, resource, and showCurrentArch have appropriate default values if they are intended to be optional.
  • Consider creating separate interfaces or classes for more complex entities like resources, if needed, to better encapsulate their properties.
  • Review other parts of your project using this interface to make sure adding new properties doesn’t inadvertently break any existing logic.

This updated interface now provides more flexibility and functionality than before, allowing customization based on specific use cases. It will need adequate testing to ensure all expected behaviors work correctly after deployment.

Expand Down
24 changes: 22 additions & 2 deletions frontend/src/views/app-store/apps/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@
</LayoutContent>
<Install ref="installRef" />
<Detail ref="detailRef" />
<TaskLog ref="taskLogRef" @close="search(req)" />
<TaskLog ref="taskLogRef" @close="refresh" />
</template>

<script lang="ts" setup>
Expand All @@ -189,6 +189,7 @@ import { newUUID, jumpToPath } from '@/utils/util';
import Detail from '../detail/index.vue';
import TaskLog from '@/components/log/task/index.vue';
import { storeToRefs } from 'pinia';
import bus from '@/global/bus';

const globalStore = GlobalStore();
const { isProductPro } = storeToRefs(globalStore);
Expand Down Expand Up @@ -228,12 +229,28 @@ const detailRef = ref();
const taskLogRef = ref();
const syncCustomAppstore = ref(false);

const refresh = () => {
search(req);
};

const search = async (req: App.AppReq) => {
loading.value = true;
req.pageSize = paginationConfig.pageSize;
req.page = paginationConfig.currentPage;
localStorage.setItem('app-page-size', req.pageSize + '');
await searchApp(req)

const customReq = {
page: req.page,
pageSize: req.pageSize,
tags: req.tags,
name: req.name,
resource: req.resource,
showCurrentArch: req.showCurrentArch,
};
if (syncCustomAppstore.value && req.resource === 'remote') {
customReq.resource = 'custom';
}
await searchApp(customReq)
.then((res) => {
apps.value = res.data.items;
paginationConfig.total = res.data.total;
Expand Down Expand Up @@ -341,6 +358,9 @@ const searchByName = () => {
};

onMounted(async () => {
bus.on('refreshTask', () => {
search(req);
});
if (router.currentRoute.value.query.install) {
installKey.value = String(router.currentRoute.value.query.install);
const params = {
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

Code Differences Analysis and Suggestions:

1. <TaskLog> Component Update

  • The @close event method has been changed from search(req) to refresh().
    • Suggestion: This change seems intentional since it aligns better with the functionality intended, which might be to refresh data when a log window is closed.

2. Search Request Customization

  • In the search() function, the logic for handling custom application stores was added: 
    const customReq = {
  page: req.page,
  pageSize: req.pageSize,
  tags: req.tags,
  name: req.name,
  resource: req.resource,
  showCurrentArch: req.showCurrentArch,
};

if (syncCustomAppstore.value && req.resource === 'remote') {
  customReq.resource = 'custom';
}

await searchApp(customReq)
  .then((res) => {...});
    • Explanation: This part of the code ensures that if the requested app store is remote and syncing is enabled, it switches to using a custom app store. It's useful in scenarios where data needs to be refreshed dynamically based on user actions (like refreshing logs).
    • Suggestion: Ensure that this behavior meets the expected requirements and doesn't introduce bugs or performance issues.

3. Event Bus Usage

  • An event listener (onMounted) is set up to listen for events emitted by another component (presumably referenced as bus): 
    bus.on('refreshTask', () => {
  search(req);
});
    • Explanation: This approach allows other parts of the application to trigger the fetchAppsByCategory() function at specific times (e.g., reloading data due to manual updates). It enhances modularity and decouples components.
    • Suggestion: Validate that there aren't multiple listeners registering with the same event name, causing redundancy or overwriting states unintentionally.

Summary

The provided changes seem mostly sensible, but it would be beneficial to ensure that:

  • Any dynamic state manipulation triggered by these functions does not lead to unintended side effects.
  • Performance considerations are kept in mind, especially during heavy data processing or network requests.
  • Additional edge cases covered thoroughly within tests.

If you have more context about how this system interacts with others, please provide details so further optimizations can be recommended.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The changes in this code mainly involve modifying event handling for @close and adding a new function called refresh.

  1. Event Handling Modification: 
    <TaskLog ref="taskLogRef" @close="search(req)" />

    <TaskLog ref="taskLogRef" @close="refresh" />

Explanation:

  • The original event handler was pointing to the method search, which takes an object of type App.AppReq. However, after these changes, it now points to the refresh method.
  1. New Function Addition: 
    const refresh = () => {
    search(req);
};

Explanation:
This addition provides a dedicated method named refresh that calls the search function with the current request (req). This separation improves readability and maintainability by encapsulating the logic related to refreshing data separately from other operations.

These modifications do not introduce significant irregularities or potential issues as long as they align with your intended use cases but make the code base cleaner and easier to understand. If you have any specific requirements or constraints regarding these changes, feel free to let me know!

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The provided code has a few issues that need to be addressed:

1. Function Signature Change

In this commit, the refresh function signature changes: @close="search(req)" becomes @close="refresh". This could cause an error because the original event handler expects one argument (req), while the new function does not accept any arguments.

Solution: Modify the function call in the <TaskLog> component to pass the required parameter.

<TaskLog ref="taskLogRef" @close="(req) => search(req)" />

2. Repeated Code Block

The await searchApp(customReq) block is repeated twice within the search function. If either version of the request needs different logic or handling, consider consolidating it into a single useSearch or similar utility.

Solution: Consolidate the code inside the if (syncCustomAppstore.value && req.resource === 'remote') {...} block into a single useSearch function outside of the main search function.

3. Event Handler Subscription

There is now an additional listener added using bus.on('refreshTask', () => { search(req); }), which will trigger on every call to search.

If you don't want to subscribe to all calls but only specific ones based on certain conditions, ensure proper logic for unsubscribing during component cleanup or re-mounting.

Solution: Review and adjust how events are being handled to maintain clean state management without unnecessary subscriptions.

Summary

  • Function Signature: Ensure @close="refresh" correctly passes parameters, preferably matching both expected types (req vs no args).

  • Code Optimization: Combine duplicated code blocks involving searchApp.

  • Event Handling Management: Check and handle the event subscription carefully since extra subscribers might introduce inefficiencies or unintended behavior.

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/app-store/detail/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
</div>
<MdEditor previewOnly v-model="app.readMe" :theme="isDarkTheme ? 'dark' : 'light'" />
</DrawerPro>
<Install ref="installRef"></Install>
<Install ref="installRef" />
</template>

<script lang="ts" setup>
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The code difference you provided is minor and does not contain any irregularities, potential issues, or optimizations to suggest. The changes from </DrawerPro> to </DrawerPro> on lines 61 and 62 are identical and do not change the functionality of the component. Similarly, replacing <Install ref="installRef"></Install> with <Install ref="installRef" /> only removes unnecessary whitespace which makes no impact on the rendered output.

If there were any other changes or discrepancies that should be considered, please specify them so I can assist further.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 3, 2025

Choose a reason for hiding this comment

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

The code snippet you provided appears to be an HTML template section from a Vue.js component. Here is a quick review of the changes mentioned:

Removed Code (New Line 67):

<Install ref="installRef"></Install>

Changed Class Property ('v-if') on DrawerPro Component:
Before modification: <DrawerPro class="drawer-container" :visible.sync="app.isOpen"
After modification: <DrawerPro class="drawer-container" :visible="app.isOpen"

No Other Irregularities or Potential Issues Found: The remaining syntax looks mostly correct, with no apparent errors, missing semicolons, incorrect property names, etc.

Suggested Optimization:

Since there are no significant logical or performance issues in this part of the code, I'd recommend focusing on other components or sections that might require attention based on their complexity and usage within the overall system.

If you have more specific parts of the application requiring further testing or optimization, feel free to ask!

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/app-store/installed/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@
<PortJumpDialog ref="dialogPortJumpRef" />
<AppIgnore ref="ignoreRef" @close="search" />
<ComposeLogs ref="composeLogRef" />
<TaskLog ref="taskLogRef" />
<TaskLog ref="taskLogRef" @close="search" />
<Detail ref="detailRef" />
</template>

Expand Down
Loading