feat(dpi): refactor DPI UI - #815
m-dilorenzi wants to merge 4 commits into
Conversation
Tbaile
left a comment
Tbaile
left a comment
There was a problem hiding this comment.
The notes I've left in the useApplicationGroups are valid for all the new composables, also there's no point to test looping functions, tests only things that eventually will break if data underneath changes or edge cases.
Validation should stick to the backend unless needed, the only validation I could get behind is the step-by-step one, and I'm starting to think that is better to hit a backend API for validation only.
| const active = res.data.active ?? false | ||
|
|
||
| if (active !== isActive.value) { | ||
| queryClient.invalidateQueries({ queryKey: DPI_LOADED_APPLICATIONS_KEY }) | ||
| queryClient.invalidateQueries({ queryKey: DPI_LOADED_PROTOCOLS_KEY }) | ||
| } | ||
|
|
||
| isActive.value = active |
There was a problem hiding this comment.
We can just invalidate the queries, it's data saved in the machine, not a big issue
| totalSteps: number | ||
| currentStep: number | ||
| stepLabel: string | ||
| barSize?: 'sm' | 'md' | 'lg' | 'xl' |
There was a problem hiding this comment.
Let's keep just what we use
| barSize?: 'sm' | 'md' | 'lg' | 'xl' | |
| barSize?: 'sm' | 'md' |
| totalSteps: number | ||
| currentStep: number | ||
| stepLabel: string | ||
| barSize?: 'sm' | 'md' | 'lg' | 'xl' |
There was a problem hiding this comment.
Why generating more than we need?
There was a problem hiding this comment.
Unrelated fix, let's not change what we don't need
| export function useApplicationGroups() { | ||
| return useQuery({ | ||
| queryKey: [...APPLICATION_GROUPS_KEY, 'all'], | ||
| queryFn: ({ signal }) => | ||
| ubusCall<ListAppGroupsResponse>('ns.dpi', 'list-appgroups', {}, { signal }), | ||
| select: (res) => res.data.values.data | ||
| }) | ||
| } | ||
|
|
||
| export function useApplicationGroupsPage(params: { | ||
| search: Ref<string> | ||
| page: Ref<number> | ||
| pageSize: Ref<number> | ||
| }) { | ||
| return useQuery({ | ||
| queryKey: [...APPLICATION_GROUPS_KEY, 'page', params.search, params.page, params.pageSize], | ||
| queryFn: ({ signal }) => | ||
| ubusCall<ListAppGroupsResponse>( | ||
| 'ns.dpi', | ||
| 'list-appgroups', | ||
| { | ||
| search: params.search.value || undefined, | ||
| limit: params.pageSize.value, | ||
| page: params.page.value | ||
| }, | ||
| { signal } | ||
| ), | ||
| select: (res) => res.data.values, | ||
| placeholderData: keepPreviousData | ||
| }) | ||
| } |
There was a problem hiding this comment.
Not liking this separation, should be one single call with optional parameters
| const loading = ref({ | ||
| deleteRule: false | ||
| }) | ||
| const deleteError = ref<Error>() |
There was a problem hiding this comment.
this can be extracted by error inside the mutation
| const nameSchema = v.object({ | ||
| name: v.pipe( | ||
| v.string(), | ||
| v.trim(), | ||
| v.minLength(1, 'required'), | ||
| v.maxLength(NAME_MAX_LENGTH, 'name_too_long') | ||
| ) | ||
| }) | ||
|
|
There was a problem hiding this comment.
This approach might not be the best. While all code seem fine we are having a single component behave completely differently by it's state, probably extracting a presentation component and then two components that apply logic (create/edit) is the best approach here
|
|
||
| const { t } = useI18n() | ||
|
|
||
| const deleteError = ref<Error>() |
There was a problem hiding this comment.
Error can be extracted by the mutation, if you need different behaviours you can just apply a watch function
There was a problem hiding this comment.
This might have the same "issue" of the CreateApplicationModal, single component for multiple usages
a2ad89a to
f7485d7
Compare
2 participants
This PR contains a rework of the DPI Filter UI, replacing the old card-based rule editor with a table-driven interface and introducing application groups as a first-class concept.
Main changes
useDpiRules,useApplicationGroups,useDpiCatalog) built on TanStack Query, with typed payloads, shared query keys and centralized cache invalidation. The catalog composable merges the netifyd catalog with the loaded applications/protocols and exposes categories with icons and availability info.NeMultiTextInputgains ahelperTextprop and improved spacing;NeSteppergains configurable bar size and color classes.