Skip to content

Comments

feat(x2a): Adding ModulePage with details, former phase retrigger and logs#2327

Merged
elai-shalev merged 7 commits intoredhat-developer:mainfrom
eloycoto:MarekPR2319
Feb 16, 2026
Merged

feat(x2a): Adding ModulePage with details, former phase retrigger and logs#2327
elai-shalev merged 7 commits intoredhat-developer:mainfrom
eloycoto:MarekPR2319

Conversation

@eloycoto
Copy link
Contributor

A rebase form #2319

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Feb 13, 2026

Unexpected Changesets

The following changeset(s) reference packages that have not been changed in this PR:

  • /home/runner/work/rhdh-plugins/rhdh-plugins/workspaces/x2a/.changeset/clean-books-wish.md: @red-hat-developer-hub/backstage-plugin-x2a-backend, @red-hat-developer-hub/backstage-plugin-x2a-common

Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-x2a workspaces/x2a/plugins/x2a patch v1.0.0

@eloycoto eloycoto force-pushed the MarekPR2319 branch 3 times, most recently from fc09a3b to 00d5523 Compare February 13, 2026 17:36
@eloycoto eloycoto marked this pull request as ready for review February 13, 2026 17:41
Copy link
Contributor

@elai-shalev elai-shalev left a comment

Choose a reason for hiding this comment

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

Some comments

Comment on lines 64 to 66
// Do not merge
useSeedTestData();

Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be removed before merging

Comment on lines 1 to 3
/*
* Copyright Red Hat, Inc.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should not be merged

Comment on lines 1 to 3
/*
* Copyright Red Hat, Inc.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should not be merged

Comment on lines 71 to 84
<Grid container direction="row" spacing={3}>
{phase && (
<RunAction
instructions={t('modulePage.phases.reanalyzeInstructions')}
actionText={t('modulePage.phases.rerunAnalyze')}
/>
)}
{!phase && (
<RunAction
instructions={t('modulePage.phases.analyzeInstructions')}
actionText={t('modulePage.phases.runAnalyze')}
/>
)}
{/* TODO: Button for canceling the current job execution */}
Copy link
Contributor

Choose a reason for hiding this comment

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

this has analyze hardcoded?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only analyze in taken care of, both here and in the Button grid on L77.
I think this PR would need all three possibilities introduced (analyze, migrate, publish)

Comment on lines 50 to 56
<Grid item xs={12}>
<Button variant="primary">{actionText}</Button>
</Grid>
<Grid item xs={12}>
<Typography>{instructions}</Typography>
</Grid>
</>
Copy link
Contributor

Choose a reason for hiding this comment

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

this button has no OnClick method, it does nothing

Comment on lines 34 to 43
return (
<Link
to={buildArtifactUrl(artifact.value, '')}
target="_blank"
rel="noopener noreferrer"
key={artifact.id}
>
{humanizeArtifactType(t, artifact.type)}
</Link>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it can create a broken link if the artifact value is a relative path

>
<Link to="/">{t('page.title')}</Link>
<Typography>{t('modulePage.title')}</Typography>
<Typography>Breadcrumbs</Typography>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is leftover? (leftover bread)

separator=">"
className={classes.breadcrumbs}
>
<Link to="/">{t('page.title')}</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

should be /x2a/api?

<Grid item xs={3}>
<ItemField
label={t('modulePage.phases.startedAt')}
value={humanizeDate(phase?.startedAt || empty)}
Copy link
Contributor

Choose a reason for hiding this comment

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

when phase?.startedAt is falsy, empty is the translation of 'module.phases.none' (e.g., "None").
new Date("None") returns Invalid Date, which toLocaleString renders as the string "Invalid Date". The callers in ModuleTable.tsx guard against this by checking if (!lastJob) before calling humanizeDate, but PhasesCard does not — it always calls through. The fallback should be handled before calling humanizeDate.

@eloycoto eloycoto force-pushed the MarekPR2319 branch 2 times, most recently from 99fef1f to 89cf58d Compare February 16, 2026 14:38
mareklibra and others added 3 commits February 16, 2026 15:49
… logs

Signed-off-by: Marek Libra <marek.libra@gmail.com>
Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
}
return (
<Link
to={buildArtifactUrl(artifact.value, targetRepoUrl)}
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to add target repo branch after 2332 was merged

Comment on lines 71 to 84
<Grid container direction="row" spacing={3}>
{phase && (
<RunAction
instructions={t('modulePage.phases.reanalyzeInstructions')}
actionText={t('modulePage.phases.rerunAnalyze')}
/>
)}
{!phase && (
<RunAction
instructions={t('modulePage.phases.analyzeInstructions')}
actionText={t('modulePage.phases.runAnalyze')}
/>
)}
{/* TODO: Button for canceling the current job execution */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Only analyze in taken care of, both here and in the Button grid on L77.
I think this PR would need all three possibilities introduced (analyze, migrate, publish)

Comment on lines +34 to +43
<Breadcrumbs
aria-label="breadcrumb"
separator=">"
className={classes.breadcrumbs}
>
<Link to="/x2a/api">{t('page.title')}</Link>
<Typography>{t('modulePage.title')}</Typography>
</Breadcrumbs>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might need to dynamically generate the links here rather than hardcode /x2a/api. I know I said otherwise in a prior PR, I'm not sure how the breadcrumbs are used here
So maybe at least let's add a TODO: revise here so we take another look at it

Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
@sonarqubecloud
Copy link

@elai-shalev elai-shalev merged commit 4465f5c into redhat-developer:main Feb 16, 2026
9 checks 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