Skip to content

Conversation

@ciiay
Copy link
Member

@ciiay ciiay commented Sep 5, 2025

Hey, I just made a Pull Request!

For RHIDP-8301
This works with rhdh pr #3366

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Test config:

dynamicPlugins:
  rootDirectory: dynamic-plugins-root
  frontend:
    default.main-menu-items:
      menuItems:
        ...
        default.admin:
          title: Administration
          icon: admin
...
    red-hat-developer-hub.backstage-plugin-marketplace:
      appIcons:
        - name: pluginsIcon
          importName: PluginsIcon
      dynamicRoutes:
        - path: /extensions
          importName: DynamicMarketplacePluginRouter
          menuItem:
            icon: pluginsIcon
            text: Extensions
      menuItems:
        extensions:
          parent: default.admin

Screen recording of testing on RHDH
Updated on 9/19 (with rhdh pr #3366)

rhidp_8301_new.mp4

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Sep 5, 2025

Important

This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior.

Changed Packages

Package Name Package Path Changeset Bump Current Version
app workspaces/marketplace/packages/app none v0.0.0
@red-hat-developer-hub/backstage-plugin-marketplace-backend workspaces/marketplace/plugins/marketplace-backend minor v0.8.0
@red-hat-developer-hub/backstage-plugin-marketplace workspaces/marketplace/plugins/marketplace minor v0.9.3

@ShiranHi
Copy link

ShiranHi commented Sep 7, 2025

Thank you @ciiay for this PR, looks great. I have some questions:

  1. For the empty state, can we use this text and reduce the table height?
  2. Can we move the Version column closer to the plugin name and the Actions column to the right-hand side. like here?
  3. I don't know if this is part of the PR but can we add tooltips that appear when hovering over the icons in the Actions column, like here?

@logonoff logonoff removed their request for review September 8, 2025 01:01
@ciiay ciiay requested a review from a team as a code owner September 9, 2025 01:15
@ciiay ciiay force-pushed the rhidp-8301-create-installed-plugins-tab branch from c6035e7 to a414de9 Compare September 9, 2025 01:16
@ciiay
Copy link
Member Author

ciiay commented Sep 9, 2025

Thank you @ciiay for this PR, looks great. I have some questions:

  1. For the empty state, can we use this text and reduce the table height?
  2. Can we move the Version column closer to the plugin name and the Actions column to the right-hand side. like here?
  3. I don't know if this is part of the PR but can we add tooltips that appear when hovering over the icons in the Actions column, like here?

Thanks for the review. Uploaded the new screen recording for the fix.

@ciiay ciiay requested a review from debsmita1 September 9, 2025 12:39
@debsmita1
Copy link
Member

debsmita1 commented Sep 9, 2025

We want to show a single row for plugins(Ex: Marketplace) that have both backend and frontend components

Screenshot 2025-09-09 at 9 13 57 PM

@ShiranHi the backend and frontend components will have different versions, how do we display that ?

Comment on lines 124 to 125
orderBy = { field: 'name' },
orderDirection = 'asc',
Copy link
Member

Choose a reason for hiding this comment

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

These properties are deprecated

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@ciiay
Copy link
Member Author

ciiay commented Sep 9, 2025

We want to show a single row for plugins(Ex: Marketplace) that have both backend and frontend components

Screenshot 2025-09-09 at 9 13 57 PM @ShiranHi the backend and frontend components will have different versions, how do we display that ?

Good point. Sharing a quick heads-up on the “single row per plugin” work and a few pain points.

What my PR does now: I group rows by a normalized base name (strip org/prefix and -frontend/-backend/-dynamic), prefer the FE package when both exist, and show that version. This meets the single-row AC but doesn’t look at the plugin extension YAML.

Pain points:

  1. Some plugins have more than FE/BE (e.g., Marketplace also has a catalog backend module), so string-based grouping is not reliable.
  2. Package versions often differ across FE/BE/modules—unclear which version we should show in the single Version column.
  3. “Installed count” is ambiguous: if only some packages are present, do we count it as installed or partial?
  4. If we use the extension YAML as source of truth, doing runtime checks across YAMLs may be expensive.

Questions and concerns:

  1. What should be the source of truth for grouping? The extension YAML packages or just the pluginProvider.plugins() returned values?
  2. For the Version column on a single row, do we show frontend package version or what version we should show?
  3. What should we do with partially installed plugins?
  4. If extension YAML file is the source, are we okay with a lightweight/precomputed mapping to avoid heavy runtime checks?
  5. Name alignment: Are pluginProvider.plugins() package names guaranteed to match the YAML entries (scope/suffix included)? If not, should we add a small story to align names or introduce a stable mapping/ID?

cc. @christoph-jerolimov

@ShiranHi
Copy link

@ShiranHi the backend and frontend components will have different versions, how do we display that ?

Yes, discussed here.

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

I think we're almost fine here. Some small details, nothing major and I'm fine if @debsmita1 will merge it.

Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
@ciiay ciiay requested a review from ShiranHi September 19, 2025 06:28
@debsmita1 debsmita1 self-requested a review September 22, 2025 15:25
Copy link
Member

@debsmita1 debsmita1 left a comment

Choose a reason for hiding this comment

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

Why is the "Install" disabled, can we provide a tooltip ?

Screenshot 2025-09-23 at 1 31 51 PM

The action on the drawer shows "Update" but the installation page shows "Install" instead of "Edit"

Screen.Recording.2025-09-23.at.3.59.59.PM.mov

Should be packages

Screenshot 2025-09-23 at 1 33 03 PM

@debsmita1
Copy link
Member

debsmita1 commented Sep 23, 2025

Tested it locally along with redhat-developer/rhdh#3366, and the functionality works fine.

@ciiay
Copy link
Member Author

ciiay commented Sep 24, 2025

Why is the "Install" disabled, can we provide a tooltip ?

Changed it to Save to align up with the plugin install page.

Should be packages

Updated to packages, also updated the plugin install page to use plugins too so both are consistent.

@ciiay ciiay requested a review from debsmita1 September 24, 2025 12:05
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
3.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link
Member

@debsmita1 debsmita1 left a comment

Choose a reason for hiding this comment

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

Verified that changes made to a package config, updates the yaml file configured in the extensions configuration, and the same is reflected in the edit plugin configuration as well

Screen.Recording.2025-09-25.at.1.29.45.PM.mov
  • When updating the package config, users should be directed to the "Installed Packages" tab instead of the package info drawer.
  • Since there can be multiple packages with config updates, @ShiranHi could you guide us on how we should handle an alert message in this case?
  • Also, we should hide the "Edit Instructions" section if no instructions are available for the package.
  • @ShiranHi do you agree that we show the same "Actions" dropdown here like we have on the plugins info drawer (We can address this as part of https://issues.redhat.com/browse/RHIDP-6810)
Image Image

Let's address the issues in a separate PR

/lgtm

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

I have a bunch of comment but I think we can merge it now and then take a look into this together:

"prettier": "^3.4.2",
"typescript": "~5.3.0"
"typescript": "~5.3.0",
"yaml": "^2.8.1"

Choose a reason for hiding this comment

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

I don't think this is needed on the workspace level? :)

"@red-hat-developer-hub/backstage-plugin-marketplace-common": "workspace:^",
"express": "^4.17.1",
"express-promise-router": "^4.1.0",
"yaml": "^2.7.1",

Choose a reason for hiding this comment

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

plugins/marketplace-backend/src/installation/FileInstallationStorage.ts is using the yaml package. Please move this back from devDependencies to dependencies.

queryKey: ['marketplaceApi', 'getPackageByName', namespace, name],
queryFn: () => marketplaceApi.getPackageByName(namespace, name),
queryFn: () => marketplaceApi.getPackageByName(namespace!, name!),
enabled: Boolean(namespace && name),

Choose a reason for hiding this comment

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

I think we should have another parameter or another hook for this. I think another hook would be right.

But we can keep it for now.

Comment on lines +23 to +41
const [count, setCount] = useState<number>(0);
const [loading, setLoading] = useState<boolean>(true);
const [error, setError] = useState<Error | undefined>(undefined);
const dynamicPluginInfo = useApi(dynamicPluginsInfoApiRef);

useEffect(() => {
const fetchCount = async () => {
try {
setLoading(true);
setError(undefined);
const plugins = await dynamicPluginInfo.listLoadedPlugins();
setCount(plugins.length);
} catch (err) {
setError(err as Error);
setCount(0);
} finally {
setLoading(false);
}
};

Choose a reason for hiding this comment

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

In general we used react-query in this plugin instead of manual state handling

Comment on lines +233 to +262
const fetchData = async (
query: Query<InstalledPackageRow>,
): Promise<QueryResult<InstalledPackageRow>> => {
const { page = 0, pageSize = 5 } = query || {};

try {
// Ensure data available; avoid re-fetching on search or pagination
const installed = installedQuery.data ?? [];
const packagesResponse = packagesQuery.data ?? { items: [] as any[] };

const entitiesByName = new Map(
(packagesResponse.items ?? []).map(entity => [
(entity.metadata?.name ?? '').toLowerCase(),
entity,
]),
);

// Build rows for ALL installed items; if no matching entity, mark missing
const rows: InstalledPackageRow[] = installed.map(p => {
const normalized = p.name
.replace(/[@/]/g, '-')
.replace(/-dynamic$/, '')
.replace(/^-+/, '')
.toLowerCase();
const entity = entitiesByName.get(normalized) as any | undefined;
const rawName = entity
? (entity.metadata?.title as string) ||
(entity.metadata?.name as string)
: getReadableName(p.name);
const cleanedName = rawName.replace(/\s+(frontend|backend)$/i, '');

Choose a reason for hiding this comment

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

I know this is copied but this looks like a lot of code to me that the backstage table is already doing? But maybe I'm wrong in the detail.

Fine for now.

<Route path="/*" Component={Tabs} />
</Routes>
</ReactQueryProvider>
<InstallationContextProvider>

Choose a reason for hiding this comment

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

We can get rid of the InstallationContextProvider or? Doesn't it return null?

<Route
path="/packages/:namespace/:name/install"
Component={MarketplacePackageInstallPage}
Component={MarketplacePackageEditPage}

Choose a reason for hiding this comment

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

Actually, why is it renamed? 😄

import { MarketplacePackageEditContentLoader } from '../components/MarketplacePackageEditContent';

const PackageInstallHeader = () => {
const PackageEditHeader = () => {

Choose a reason for hiding this comment

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

Why renaming things?

Comment on lines +58 to +61
{/* Force remount on navigation within same route to reseed editor */}
<MarketplacePackageEditContentLoader
key={`${location.key}-${location.search}`}
/>

Choose a reason for hiding this comment

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

🤯

Comment on lines +42 to +53
const baseName = packageName
.replace(/^@Red Hat Developer Hub\/Backstage Plugin\s*/, '') // Red Hat plugins (display names)
.replace(/^@red-hat-developer-hub\/backstage-plugin-/, '') // Red Hat plugins (package names)
.replace(/^red-hat-developer-hub-backstage-plugin-/, '') // Red Hat kebab-case
.replace(/^@backstage-community\/plugin-/, '') // Community plugins
.replace(/^backstage-community-plugin-/, '') // Community plugins alt
.replace(/^@backstage\/plugin-/, '') // Official Backstage plugins
.replace(/^backstage-plugin-/, '') // Generic backstage plugins
.replace(/-dynamic$/, '') // Remove -dynamic suffix
.replace(/-frontend$/, '') // Remove -frontend suffix
.replace(/-backend$/, '') // Remove -backend suffix
.replace(/^[\s-]+/, '') // Remove leading spaces or dashes

Choose a reason for hiding this comment

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

🙀

@christoph-jerolimov christoph-jerolimov merged commit dad9806 into redhat-developer:main Sep 25, 2025
9 of 10 checks passed
@debsmita1
Copy link
Member

I will address these review comments as part of https://issues.redhat.com/browse/RHIDP-6810

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants