Skip to content

Conversation

yaroslav8765
Copy link
Contributor

No description provided.

@yaroslav8765 yaroslav8765 requested a review from Copilot August 28, 2025 14:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes TypeScript errors across the codebase by adding proper type annotations, correcting imports, and resolving type inconsistencies.

  • Added explicit type annotations throughout Vue components and TypeScript files
  • Fixed import statements to use correct paths and type exports
  • Refactored duplicated type definitions into reusable interfaces

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
adminforth/types/Common.ts Added FieldGroup type, imported AdminForthActionInput, refactored field group definitions
adminforth/spa/vite.config.ts Added type annotations for function parameters and error handling
adminforth/spa/src/views/ShowView.vue Fixed type imports and added type annotations for computed properties and functions
adminforth/spa/src/views/ListView.vue Added comprehensive type annotations and fixed array handling logic
adminforth/spa/src/stores/modal.ts Made ModalContentType properties required instead of optional
adminforth/spa/src/spa_types/core.ts Updated import path for AdminForth types
adminforth/spa/src/components/* Fixed type annotations, event handlers, and prop definitions across multiple components
adminforth/spa/src/afcl/* Added proper TypeScript annotations for form components
adminforth/spa/src/adminforth.ts Fixed duplicate imports and corrected function return types
Files not reviewed (1)
  • dev-demo/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

…tTableVirtual, enhance v-for key in ThreeDotsMenu, and tidy up Common.ts
…nhance type definitions in modal and views for better type safety
adminforth.list.refresh = async () => {
return await getList();
}
const result = await getList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you return result of getList?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adminforth.list.refresh method expect us to return only an error. await getList(); will be executed anyway
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, then why do you return empty objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because refresh(): Promise<{...}> its not a Promise and it still has to return something

Copy link
Collaborator

@SerVitasik SerVitasik Sep 12, 2025

Choose a reason for hiding this comment

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

Why we can't return something if !result and ok: true or false?
For example like this

adminforth.list.refresh = async () => {
  const result = await getList();
  if (!result) {
    return { error: "Failed to refresh", ok: false};
  }
  if ('error' in result && result.error != null) {
    return { error: String(result.error), ok: false };
  }
  return {ok: true};
};

refresh(): Promise<{ error?: string, ok: boolean}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch is not about re-writing FrontendAPI. Change refresh(): Promise<{ error?: string, ok: boolean}> will cause a lot of new TS error, which will have to be resolved as well. Does adding ok: boolean will give us any advantages?

Copy link
Contributor

Choose a reason for hiding this comment

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

@SerVitasik good finding, I think that indeed we see that logic changed - refresh previously returned all data but now it will return only error if it happened.
However type is controversial to what happened before - type actually said that only error could be returned but not all data. So I can understand Yaroslav - actually he tried in this code to follow type implementation.

So in such tasks we need to decide, whether we will stick to type or actual behavior we had before. And we need to be indeed very careful when we removing actual backward compatibility - if some code was reading results of refresh, it will brake now fully.

But I ask question to myself - is it really needed that some code should read from refresh? I mean imagine case you are using refresh - generally it is just to update table, not to fetch actual data. If you have idea where you need exactly read all data from refresh - please write it. If we will need it we can create another task where we need both to change type, maybe even add prop like { error: ..., data... }
For now I think implementation corresponds to type

Copy link
Collaborator

Choose a reason for hiding this comment

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

This branch is not about re-writing FrontendAPI. Change refresh(): Promise<{ error?: string, ok: boolean}> will cause a lot of new TS error, which will have to be resolved as well. Does adding ok: boolean will give us any advantages?

Yes, it is. At the very least, if we have an empty result, the user will see an error rather than an empty object. And it is always better to see ok: true than an empty object when the operation is successful.
As for returning the data, i don't think we need it. Personally, i only use list.refresh() for updates.

adminforth.list.refresh = async () => {
return await getList();
}
const result = await getList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, then why do you return empty objects?

…esourceListTable and ResourceListTableVirtual, and add 'FieldGroup' type in ShowView
@ivictbor ivictbor merged commit f2aec6a into next Sep 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants