feat(x2a): Adding Projects Details Page, fixes in the Modules Details Page#2448
feat(x2a): Adding Projects Details Page, fixes in the Modules Details Page#2448mareklibra wants to merge 2 commits intoredhat-developer:mainfrom
Conversation
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Unexpected ChangesetsThe following changeset(s) reference packages that have not been changed in this PR:
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
|
Review Summary by QodoAdd Projects Details Page and init phase logs endpoint
WalkthroughsDescription• Add new /projects/:projectId/log endpoint for init phase logs • Create ProjectPage component with project details and modules overview • Refactor PhaseDetails component for reusability across init and module phases • Expand Project model to include initJob field with sensitive data removed • Add comprehensive test coverage for init phase log retrieval • Update translations across multiple languages for new UI components • Reorganize component structure and improve code reusability Diagramflowchart LR
A["Backend Router"] -->|GET /projects/:projectId/log| B["Init Phase Logs"]
C["Project Model"] -->|includes initJob| D["ProjectPage Component"]
D -->|displays| E["ProjectDetailsCard"]
D -->|displays| F["ProjectModulesCard"]
D -->|displays| G["InitPhaseCard"]
G -->|uses| H["PhaseDetails Component"]
I["ModulePage"] -->|refactored to use| H
File Changes1. workspaces/x2a/plugins/x2a-backend/src/router/jobs.ts
|
Code Review by Qodo
1. Seeds test data
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
workspaces/x2a/plugins/x2a/src/components/ProjectList/EmptyProjectList.tsx
Outdated
Show resolved
Hide resolved
mareklibra
force-pushed
the
FLPATH-3143.projectDetailPage
branch
3 times, most recently
from
38b5e76 to
8b30238
Compare
mareklibra
force-pushed
the
FLPATH-3143.projectDetailPage
branch
2 times, most recently
from
612dabe to
7d8818a
Compare
Review Summary by QodoAdd Project Details Page and init phase logging support
WalkthroughsDescription• Add new Project Details Page with project information display • Implement init phase logging endpoint for project discovery phase • Refactor shared components for reusability across pages • Update translations and improve module details page Diagramflowchart LR
A["Project List"] -->|Navigate| B["Project Details Page"]
B -->|Display| C["Project Info Card"]
B -->|Display| D["Modules Card"]
B -->|Display| E["Init Phase Card"]
E -->|Fetch Logs| F["GET /projects/:projectId/log"]
F -->|Return| G["Init Phase Logs"]
C -->|Show| H["Migration Plan"]
File Changes1. workspaces/x2a/plugins/x2a/src/components/ProjectPage/ProjectPage.tsx
|
Code Review by Qodo
1. K8s log errors swallowed
|
elai-shalev
left a comment
elai-shalev
left a comment
There was a problem hiding this comment.
Some comments on the code, I still need to run the UI and inspect more deeply
| throw error; | ||
| // Might happen, i.e. the pod is in ContainerCreating state | ||
| this.#logger.warn(`Failed to get job logs: ${error.message}`); | ||
| return ''; |
There was a problem hiding this comment.
This handles ContainerCreating gracefully but also hides auth failures, network errors, namespace misconfigurations, etc. Callers (the route handler) cannot distinguish "pod not ready" from "something is broken." Maybe we could catch only the specific error (e.g., check error message/code for pod-not-ready conditions) and re-throw unexpected errors. Or return a richer result type like { log: string; error?: string }.
returning '' may be insufficient, we should throw or return something more substantial
| // If job is finished, return logs from database | ||
| if (latestJob.status === 'success' || latestJob.status === 'error') { | ||
| logger.info( | ||
| `Job ${latestJob.id} is finished (status: ${latestJob.status}), returning logs from database`, | ||
| ); | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| const log = await x2aDatabase.getJobLogs({ jobId: latestJob.id }); | ||
| if (!log) { | ||
| logger.error(`Log not found for a finished job ${latestJob.id}`); | ||
| } | ||
| res.send(log || ''); | ||
| return; | ||
| } | ||
|
|
||
| // Check if job has k8sJobName | ||
| if (!latestJob.k8sJobName) { | ||
| logger.warn( | ||
| `Job ${latestJob.id} has no k8sJobName, returning empty logs`, | ||
| ); | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.send(''); | ||
| return; | ||
| } | ||
|
|
||
| // Get logs from Kubernetes | ||
| const logs = await kubeService.getJobLogs(latestJob.k8sJobName, streaming); | ||
|
|
||
| // Set content type | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
|
|
||
| // Handle streaming vs non-streaming | ||
| if (streaming && typeof logs !== 'string') { | ||
| logs.pipe(res); | ||
| } else { | ||
| res.send(logs as string); | ||
| } |
There was a problem hiding this comment.
This logic in L74-110 is similar to L170-205.
We should extract a shared handleLogRetrieval(job, kubeService, res, streaming) helper function, it's a lot of duplication
|
|
||
| // Handle streaming vs non-streaming | ||
| if (streaming && typeof logs !== 'string') { | ||
| logs.pipe(res); |
There was a problem hiding this comment.
If the k8s stream errors mid-transfer, the client receives a truncated response with no error indication and no cleanup. We should add an error listener on the stream (e.g., logs.on('error', ...)) and handle it by ending the response with an error indicator or at least log it server-side.
| @@ -0,0 +1,113 @@ | |||
| /** | |||
There was a problem hiding this comment.
Unlike ModulePage which has a refresh counter to re-fetch after actions, ProjectPage has no way to auto-update. If a user lands here while an init job is running, the status/logs won't update without a manual browser refresh.
Suggestion: Add a refresh state (like ModulePage has) and consider a polling interval while the init job is in pending/running state.
There was a problem hiding this comment.
So far we do not ave polling in place, this will be handled by FLPATH-3337 or similar.
The ModulePage refresh is for update after an active action (Run phase).
There is no such action on the project's page so far.
| const { projectId } = useRouteRefParams(projectRouteRef); | ||
| const clientService = useClientService(); | ||
| // TODO: call actions - delete, sync | ||
| const [error, _] = useState<string | undefined>(); |
There was a problem hiding this comment.
the setter is never called, so the error banner at line 88 can never appear
There was a problem hiding this comment.
right, it's a placeholder for a next PR. The first one in a queue is for Delete project action which will use that.
|
|
||
| import { Module } from '@red-hat-developer-hub/backstage-plugin-x2a-common'; | ||
|
|
||
| export const getLastJob = (rowData: Module) => { |
There was a problem hiding this comment.
we should make it clearer that getLastJob isn't about chronology, but "most advanced". is analyze is rerun, we would still expect publish to return?
There was a problem hiding this comment.
Yes, I think we should return the last reached phase, no matter the user stepped back and tries to execute an earlier phase again.
Otherwise we should probably need to hard delete the "more ones on an early-phase retrigger. I would not do it until we receive such requirement based on real production testing, such UX is risky.
| // Might happen, i.e. the pod is in ContainerCreating state | ||
| this.#logger.warn(`Failed to get job logs: ${error.message}`); | ||
| return ''; |
There was a problem hiding this comment.
1. K8s log errors swallowed 🐞 Bug ⛯ Reliability
KubeService.getJobLogs now catches all errors and returns '', hiding real failures (auth/network/404/etc.) and making clients see “no logs” rather than an actionable error; it can also lead to empty logs being persisted during reconciliation flows.
Agent Prompt
### Issue description
`KubeService.getJobLogs` currently swallows *all* errors and returns an empty string. This hides real failures from API clients and also breaks the intent of `reconcileJobStatus` which expects `getJobLogs` to throw on failure.
### Issue Context
Returning `''` for every exception makes "no logs" indistinguishable from "couldn't fetch logs" and can cause empty logs to be written into the DB.
### Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/src/services/KubeService.ts[401-433]
- workspaces/x2a/plugins/x2a-backend/src/router/common.ts[312-326]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| </Grid> | ||
|
|
||
| <Grid {...gridItemProps}> | ||
| <ItemField |
There was a problem hiding this comment.
can this be a link? It's a bit weird to have an external url that you cannot click!
|
Could we make these signs more interacive? |
… Page Signed-off-by: Marek Libra <marek.libra@gmail.com>
mareklibra
force-pushed
the
FLPATH-3143.projectDetailPage
branch
2 times, most recently
from
bf040c5 to
751177b
Compare
mareklibra
force-pushed
the
FLPATH-3143.projectDetailPage
branch
from
751177b to
8ebe6e2
Compare
Review Summary by QodoAdd Project Details Page with init phase logs and improve module details page
WalkthroughsDescription• **Added Project Details Page** with init phase information, migration plan display, and phase details card • **Implemented init phase log endpoint** (GET /projects/:projectId/log) with streaming support for retrieving project initialization logs • **Extracted reusable PhaseDetails component** for displaying phase information and logs, supporting both module and init phases • **Enhanced project enrichment** with initJob field containing the last initialization job details • **Added utility functions** for SCM provider detection (getScmProvider), repository branch URL building (buildRepoBranchUrl), and error stringification (stringifyError) • **Made project names clickable** in the project list table, linking to the new project details page • **Updated translations** across multiple languages (Spanish, Italian, French, German) with new keys for project details page and init phase card • **Improved Kubernetes pod log retrieval** with pending state validation to avoid fetching logs from uninitialized pods • **Fixed various issues** in Module Details Page including translation key updates and import path corrections • **Comprehensive test coverage** added for new endpoints, utilities, and components Diagramflowchart LR
A["Project List"] -->|"click project name"| B["Project Details Page"]
B --> C["InitPhaseCard"]
B --> D["ProjectModulesCard"]
C --> E["PhaseDetails Component"]
D --> E
E -->|"fetch logs"| F["GET /projects/:projectId/log"]
F --> G["Kubernetes Logs"]
F --> H["Database Logs"]
I["SCM Provider Utils"] --> J["Build Repo URLs"]
K["Error Stringify Utils"] --> L["Improved Error Handling"]
File Changes1. workspaces/x2a/plugins/x2a-backend/src/router/jobs.test.ts
|
Code Review by Qodo
1. GitHub Enterprise misdetected
|
| export const getScmProvider = (repoUrl: string): 'github' | 'gitlab' => { | ||
| // Based on https://docs.github.com/en/enterprise-cloud@latest/admin/managing-your-enterprise-account/changing-the-url-for-your-enterprise | ||
| // GitHub URLs should always contain github.com. | ||
| return repoUrl.includes('github.com') ? 'github' : 'gitlab'; |
There was a problem hiding this comment.
1. Github enterprise misdetected 🐞 Bug ✓ Correctness
getScmProvider classifies anything not containing github.com as gitlab, which breaks GitHub Enterprise/custom GitHub host support already present in the codebase. This will generate incorrect branch URLs (GitLab-style /-/tree/...) and can select the wrong auth token descriptor/scopes for GitHub Enterprise repos.
Agent Prompt
## Issue description
`getScmProvider` currently treats any repo URL that doesn't contain `github.com` as `gitlab`. This misclassifies GitHub Enterprise/custom GitHub hosts (e.g. `github.example.com`) and causes:
- wrong repository branch links in the UI (GitLab-style `/-/tree/...` instead of GitHub `/tree/...`)
- wrong auth token descriptor/scopes selection (`read_repository`/`write_repository` instead of `repo`)
## Issue Context
The codebase already supports custom GitHub hosts via `normalizeRepoUrl`, so this misclassification is a real compatibility bug.
## Fix Focus Areas
- workspaces/x2a/plugins/x2a-common/src/utils/getScmProvider.ts[21-25]
- workspaces/x2a/plugins/x2a-common/src/utils/getScmProvider.test.ts[1-35]
- workspaces/x2a/plugins/x2a/src/components/tools/buildRepoBranchUrl.ts[23-37]
## Suggested approach
1. Update `getScmProvider` to parse the URL (or tolerate RepoUrlPicker-style inputs by prefixing `https://` similarly to `normalizeRepoUrl`).
2. Determine provider by hostname:
- if hostname includes/endsWith `gitlab` or equals known GitLab domains -> `gitlab`
- else if hostname includes `github` OR is not recognized as GitLab -> default to `github` (since treating unknown as GitLab breaks GitHub Enterprise links/scopes).
- (Optional) consider an explicit allowlist/config hook later.
3. Add a unit test that asserts `getScmProvider('https://github.example.com/org/repo.git') === 'github'`.
4. Ensure `buildRepoBranchUrl` produces `/tree/<branch>` for that URL.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Let's address that in a separate PR (FLPATH-3398). Additional data (on top of the URL) will need to be stored.
- Handle stream errors in job log endpoint - Add sendJobLogs helper, KubeService edge-case handling - Add buildRepoBranchUrl for clickable repo links - Rename getLastJob to getLastPhaseReached
mareklibra
force-pushed
the
FLPATH-3143.projectDetailPage
branch
from
8ebe6e2 to
98e7f1d
Compare
|
3 participants
Fixes: FLPATH-3143
Adding Projects Details Page.
Fixed various issues in the Modules Details Page.
TODO:
Left for follow-ups:
✔️ Checklist