Fixes #39253 - reusable empty state page#10986
Conversation
|
Could you please also add Also |
There was a problem hiding this comment.
Pull request overview
Introduces a reusable ResourceLoadFailedEmptyState component under components/common/EmptyState, and refactors the existing ModelFormEmptyState and HostDetails empty state to use it. The goal is to give plugins a consistent, extensible "resource failed to load" pattern.
Changes:
- Adds
ResourceLoadFailedEmptyState(component + propTypes + tests) exported fromEmptyState/index.js, plus avariantpassthrough inDefaultEmptyState. - Replaces the previous
Redirect-basedRedirectToEmptyHostPagewith an in-pageHostDetailsEmptyStateusing the new component anduseForemanHostsPageUrl. - Rewrites
ModelFormEmptyStateto delegate to the new component.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack/.../EmptyState/ResourceLoadFailedEmptyState.js | New shared empty-state component with header/description/permissions/actions support |
| webpack/.../EmptyState/ResourceLoadFailedEmptyState.test.js | Tests for default/custom messaging, permissions, actions, back button toggle |
| webpack/.../EmptyState/EmptyStatePropTypes.js | Adds footerActionPropTypes and resourceLoadFailedEmptyStatePropTypes |
| webpack/.../EmptyState/index.js | Exports the new component |
| webpack/.../EmptyState/DefaultEmptyState.js | Forwards new variant prop to EmptyStatePattern |
| webpack/.../HostDetails/EmptyState/index.js | Replaces redirect with in-page empty state; uses useForemanHostsPageUrl |
| webpack/.../HostDetails/index.js | Renames import to HostDetailsEmptyState accordingly |
| webpack/.../routes/Models/ModelFormEmptyState.js | Refactored to use shared component |
Comments suppressed due to low confidence (1)
webpack/assets/javascripts/react_app/components/common/EmptyState/ResourceLoadFailedEmptyState.js:143
backButtonLabel: __('Return to the previous page')is evaluated once at module load, which means the translation is captured before the user's locale is necessarily applied and will never re-translate on locale change. Consider resolving the default inside the component (e.g.backButtonLabel ?? __('Return to the previous page')) rather than viadefaultProps. The same applies to dynamically-constructed strings likesprintf(__('Unable to load %s'), resourceLabel)— those are fine since they're inside the function body, but module-level__()calls are a known pitfall for i18n in this codebase.
backButtonLabel: __('Return to the previous page'),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </> | ||
| ) : null} | ||
| </p> | ||
| {errorMessage ? <p>Server returned: {errorMessage}</p> : null} |
| const HostDetailsEmptyState = ({ hostname }) => { | ||
| const hostsIndexUrl = useForemanHostsPageUrl(); | ||
| const hostsNewUrl = `${hostsIndexUrl}/new`; | ||
|
|
||
| return ( | ||
| <ResourceLoadFailedEmptyState | ||
| resourceLabel={__('host')} | ||
| resourceId={hostname} | ||
| primaryAction={{ | ||
| label: __('Back to all hosts'), | ||
| url: hostsIndexUrl, | ||
| ouiaId: 'host-details-empty-state-all-hosts', | ||
| }} | ||
| requiredPermissions={['view_hosts', 'create_hosts']} | ||
| secondaryActions={[ | ||
| { | ||
| label: __('Create a new host'), | ||
| url: hostsNewUrl, | ||
| ouiaId: 'host-details-empty-state-create-host', | ||
| }, | ||
| secondayActions: [ | ||
| { | ||
| title: __('Create a host'), | ||
| url: foremanUrl('/hosts/new'), | ||
| }, | ||
| ], | ||
| }, | ||
| }} | ||
| /> | ||
| ); | ||
| ]} | ||
| ouiaIdPrefix="host-details-empty-state" | ||
| /> | ||
| ); |
MariaAga
left a comment
MariaAga
left a comment
There was a problem hiding this comment.
I think it might be better split into 2 empty states: not found and no permissions
| <> | ||
| {secondaryActions.map((action, index) => ( | ||
| <React.Fragment | ||
| key={action.ouiaId || `${ouiaIdPrefix}-secondary-action-${index}`} | ||
| > |
There was a problem hiding this comment.
nit: <> and <React.Fragment> mean the same, could the file use one consistently?
| {requiredPermissions?.length > 0 ? ( | ||
| <> | ||
| {' '} | ||
| {__('Opening this requires the following permissions:')}{' '} | ||
| {requiredPermissions.map((permission, index) => ( | ||
| <React.Fragment key={permission}> | ||
| {index > 0 ? ', ' : null} | ||
| <strong>{permission}</strong> | ||
| </React.Fragment> | ||
| ))} | ||
| </> | ||
| ) : null} | ||
| </p> |
There was a problem hiding this comment.
Could the component use the usePermissions hook to see whats missing, and to know if It may not exist or you may not have permission to view it.' instead of telling the user that it might be one of the options?
There was a problem hiding this comment.
recalling theforeman/foreman_remote_execution#1030 (comment) I think yes in case we get 403 but not if we get 404 which would still be either/or
There was a problem hiding this comment.
which would still be either/or
But then the UI can check if the user has the permissions, and decide with that. or not?
There was a problem hiding this comment.
yes, but the way I read it there is a possible case where the user has permissions to view invocations in general but not a specific invocation, not sure how to isolate that case so it'd be still either either/or in case of 404. Anyway, I can isolate the 403 case so the page would be more granular
There was a problem hiding this comment.
yes, but the way I read it there is a possible case where the user has permissions to view invocations in general but not a specific invocation
Can you give me an example of when this can happen?
There was a problem hiding this comment.
meanwhile I went with your suggestion, so the messages are now more specific based on api response and usePermissions as well, so now in the case we're discussing it would say 'The %s with id %s could not be found. It may have been deleted or may not be available in your current organization or location scope.'
| {requiredPermissions?.length > 0 ? ( | ||
| <> | ||
| {' '} | ||
| {__('Opening this requires the following permissions:')}{' '} |
There was a problem hiding this comment.
nit: Accessing instead of Opening might be more correct
| title: __('All hosts'), | ||
| url: foremanUrl('/hosts'), | ||
| const HostDetailsEmptyState = ({ hostname }) => { | ||
| const hostsIndexUrl = useForemanHostsPageUrl(); |
There was a problem hiding this comment.
from an AI review yool: useForemanHostsPageUrl() can return /hosts (a Rails route) when displayNewHostsPage is false. Using history.push on that URL would navigate inside the SPA to a non-existent React route, rendering nothing. The old code used foremanUrl() for full-page navigation.
There was a problem hiding this comment.
seems like we check for displayNewHostsPage setting elsewhere, so I'd do the same approach here
ofedoren
changed the title
|
More general question, does this PR consider such cases as:
Can we potentially re-use this pattern there after its available here? |
|
@ofedoren, We talked with @Lukshio and @andreilakatos, and in the tasks page the empty state are more simple, and can be just done with the PF empty state. I think this PR is for when there are permission/db issues, that come from the DB, compared to "This task doesnt have resources/errors" |
this came from the discussion within theforeman/foreman_remote_execution#1030
The idea is to have a more unified empty state page that could be reusable in plugins with a fair amount of extensibility. This PR also adjusts the preexisting hardware model and hosts empty state pages to use the common pattern.