feat: add extension detail page for user-settings and admin dashboard#1939
feat: add extension detail page for user-settings and admin dashboard#1939netomi wants to merge 3 commits into
Conversation
…ns, add active field to indicate if a version for a target platform is active, remove obsolete delete routes as the delete is no on the detail page
|
gnugomez
left a comment
gnugomez
left a comment
There was a problem hiding this comment.
overall it's looking really good, it provides a way more clear UI for users to manage their versions
| useEffect(() => { | ||
| if (icon) { | ||
| URL.revokeObjectURL(icon); | ||
| } | ||
| service.getExtensionIcon(abortController.current, extension).then(setIcon); | ||
| setPage(0); | ||
| }, [extension]); |
There was a problem hiding this comment.
suggestion: given the refactor I would take the opportunity and extract the extension icon thing to it's own component, this way we can re-use the fetching logic, it's so annoying.
| import { ExtensionDetailRoutes } from '../../pages/extension-detail/extension-detail-routes'; | ||
| import { createRoute } from '../../utils'; | ||
|
|
||
| export const ExtensionDetailView: FunctionComponent<ExtensionDetailViewProps> = props => { |
There was a problem hiding this comment.
issue: we need to add a way to go back to the list of all extensions
after your PR if a user goes into the extension detail view and wants to go back, there's no other way than changing the tab and then going back to extensions or to go out of the settings.
| const deleteRoute = | ||
| (extension && createRoute([UserSettingsRoutes.EXTENSIONS, extension.namespace, extension.name, 'delete'])) || | ||
| ''; | ||
| const isAdminContext = pathname.startsWith(AdminDashboardRoutes.NAMESPACE_ADMIN); |
There was a problem hiding this comment.
issue: using pathname might not be the most resilient way of checking if it's in admin context.
a better option could be a react context
There was a problem hiding this comment.
if you use a prop for the extension route, there's no need for the react context #1939 (comment)
| const route = extension | ||
| ? isAdminContext | ||
| ? createRoute([AdminDashboardRoutes.EXTENSION_ADMIN, extension.namespace, extension.name]) | ||
| : createRoute([UserSettingsRoutes.EXTENSIONS, extension.namespace, extension.name]) | ||
| : ''; |
There was a problem hiding this comment.
suggestion: I don't think this route decision belongs here, I would provide it as a prop
| const loadExtension = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| const result = await service.getExtension(abortController.current, props.namespace, props.extension); | ||
| if (isError(result)) { | ||
| throw result; | ||
| } | ||
| setExtension(result); | ||
| } catch (err) { | ||
| if (err && (err as { status?: number }).status === 404) { | ||
| navigate(UserSettingsRoutes.EXTENSIONS); | ||
| } else { | ||
| handleError(err); | ||
| } | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [props.namespace, props.extension]); |
There was a problem hiding this comment.
suggestion (non-blocking): given that we decided to go with Tanstack query, it will be nice if all new data-fetching implementations use it
2 participants
This fixes #1500.
Previously there was no specific page to show details for an extension, both for the admin dashboard and the user settings. It was also not indicated if the version is actually active.
This PR overhauls that by adding a dedicated settings page for an extension that displays all versions of the extension in a table where you can delete individual versions.
Inactive versions for specific targets are formatted with strike-through atm, but this might change in the future.
Also when clicking on an extension on the namespace admin or scan admin page you will be navigated now the the settings page for this extension.
The code is extracted as a common component that is used from the admin dashboard and user settings where the delete action is parameterized to account for different endpoints.
Assisted-By: anthropic:claude-sonnet-4-6