EDM-3207: Add ImageBuilder logs, userConfig and additional export format #458
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds SSH public-key remote access configuration to image builds, introduces a QCOW2 Disk Container export format, makes export formats data-driven, implements image build/export logs with SSE streaming and a client hook, updates related utilities/types/UI, and increases server write timeout for streaming. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant LogsTab as ImageBuildLogsTab
participant Hook as useImageBuildLogs
participant API as API Server
participant Stream as Event Stream
User->>LogsTab: Select log source (BUILD or EXPORT)
LogsTab->>Hook: useImageBuildLogs(resourceId, resourceType, isActive)
Hook->>API: GET /api/.../logs?resourceId=...&type=...&active=true
API-->>Hook: 200 OK (Content-Type: text/event-stream)
rect rgba(100,200,150,0.5)
Note over Hook,Stream: SSE streaming (isActive = true)
Hook->>Stream: create reader
Stream-->>Hook: chunks (data: line\n)
Hook->>Hook: buffer & parse SSE lines
Hook-->>LogsTab: incremental log updates (isStreaming = true)
end
LogsTab->>User: render logs (auto-scroll while streaming)
alt switch to completed/non-streaming
LogsTab->>Hook: useImageBuildLogs(..., isActive=false)
Hook->>API: GET /api/.../logs?active=false
API-->>Hook: full log text
Hook-->>LogsTab: final logs (isStreaming = false)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
celdrake
force-pushed
the
EDM-3207-add-logs-and-user-config
branch
from
a7fb448 to
0d56396
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/utils.ts`:
- Around line 154-159: The code currently sets spec.userConfiguration whenever
values.userConfiguration.username and .publickey are present, ignoring the
feature toggle; update the condition to also check the enable flag (e.g.,
values.userConfiguration?.enabled or the actual boolean property used) before
assigning to spec.userConfiguration so that SSH credentials are only included
when the userConfiguration feature is enabled; modify the block referencing
values.userConfiguration and spec.userConfiguration accordingly to require both
enabled === true and non-empty username/publickey.
In
`@libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildLogsTab.tsx`:
- Around line 147-153: The effect in ImageBuildLogsTab currently only scrolls to
bottom when isStreaming is true, so when logs finish streaming the view can
remain at the bottom; update the React.useEffect that references logsRef, logs
and isStreaming so that when isStreaming is false (i.e., logs completed) you set
logsRef.current.scrollTop = 0 to reset to the top, and ensure the effect also
runs when the log source changes by adding the relevant source identifier (if
present) to the dependency list; locate the effect using the symbols
React.useEffect, logsRef, logs and isStreaming and change the conditional to
handle both branches (scrollHeight when streaming, scrollTop = 0 when not) and
include the source dependency if there is a prop like source/logSource.
In `@libs/ui-components/src/components/ImageBuilds/useImageBuildLogs.ts`:
- Around line 60-63: Old logs persist when resourceId changes because fetchLogs
appends to previous state via setLogs((prev) => ...) without clearing; update
fetchLogs to clear existing logs at start (e.g., call setLogs([]) or the empty
initial value used by your component) and reset related state
(setIsLoading(true), setError(null)) before starting the new fetch, and
additionally guard streaming appends by checking a currentResourceId/ref or
abort signal so only logs for the active resourceId are appended (refer to
fetchLogs, resourceId, setLogs, and any streaming append logic).
- Around line 15-22: getLogContent currently only recognizes lines starting with
"data: " (with a space) and thus ignores "data:" (no space) per the SSE spec;
update getLogContent to check for lines starting with "data:" (use
startsWith('data:')) and then extract the payload by slicing after the "data:"
prefix and, if present, remove a single leading space (e.g., trim only one
leading space or conditionally drop it) before pushing to logLines so both
"data:value" and "data: value" are handled.
In `@proxy/app.go`:
- Around line 97-101: The server's http.Server is setting WriteTimeout to 15 *
time.Minute which can abort long-running proxied streaming responses; update the
srv construction (the http.Server instance created in app.go, where Handler:
corsHandler(router) and Addr: config.BridgePort are set) to either set
WriteTimeout to 0 (disable the absolute cap) or read a configurable timeout
value (e.g., config.BridgeWriteTimeout or similar) and apply that so long-lived
streamed operations (ImageBuilder, FlightCtl, etc.) are not prematurely closed;
ensure the config key is added/parsed and default behavior remains safe if
unset.
🧹 Nitpick comments (4)
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/RegistrationStep.tsx (2)
54-68: Consider batching setFieldValue calls to reduce re-renders.Multiple
setFieldValuecalls in the disable branch (lines 64-66) will trigger multiple form re-renders. Consider usingsetValuesor Formik's batch update if this causes performance issues.♻️ Optional refactor using setValues
const handleRemoteAccessToggle = (enabled: boolean) => { if (enabled) { // Ensure userConfiguration object exists if (!values.userConfiguration) { setFieldValue('userConfiguration', { username: '', publickey: '', enabled: true }); } else { setFieldValue('userConfiguration.enabled', true); } } else { // Clear userConfiguration when disabled - setFieldValue('userConfiguration.username', ''); - setFieldValue('userConfiguration.publickey', ''); - setFieldValue('userConfiguration.enabled', false); + setFieldValue('userConfiguration', { username: '', publickey: '', enabled: false }); } };
165-186: Public key helper text appears outside the conditional checkbox content.The helper text at lines 182-184 is placed outside the
CheckboxFieldchildren, meaning it will always be visible regardless of whether remote access is enabled. If this is intentional (to provide context before the user enables the feature), consider moving it above the checkbox or adding visual distinction. If it should only appear when enabled, move it inside theCheckboxFieldchildren.<CheckboxField name="userConfiguration.enabled" label={t('Provide an SSH public key to enable passwordless login once your image is deployed.')} onChangeCustom={handleRemoteAccessToggle} > <FormGroup label={t('Username')} fieldId="user-config-username"> <TextField name="userConfiguration.username" aria-label={t('Username')} helperText={t('The username for the user account')} /> </FormGroup> - <UploadField name="userConfiguration.publickey" label={t('Public key')} /> + <UploadField + name="userConfiguration.publickey" + label={t('Public key')} + helperText={t('Paste the content of an SSH public key you want to use to connect to the device.')} + /> </CheckboxField> - <Content component="small"> - {t('Paste the content of an SSH public key you want to use to connect to the device.')} - </Content>libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildLogsTab.tsx (1)
50-63: Add a default label fallback for unknown formats.
Line 50-63 returnsundefinedif a new format is added without updating this switch, which would render a blank label. A small fallback avoids that.♻️ Suggested fallback
const getExportFormatText = (t: TFunction, format?: ExportFormatType) => { switch (format) { case undefined: return t('Select format'); case ExportFormatType.ExportFormatTypeVMDK: return t('Virtualization (VMDK)'); case ExportFormatType.ExportFormatTypeQCOW2: return t('OpenStack/KVM (QCOW2)'); case ExportFormatType.ExportFormatTypeQCOW2DiskContainer: return t('OpenShift Virtualization (QCOW2)'); case ExportFormatType.ExportFormatTypeISO: return t('Metal installer (ISO)'); + default: + return format ? format.toUpperCase() : t('Select format'); } };libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/utils.ts (1)
33-35: Update the export format list comment.
Line 33-35 still lists only three formats, but QCOW2 Disk Container is now included.📝 Comment update
-// Returns an array with one item per format (VMDK, QCOW2, ISO), where each item is either +// Returns an array with one item per format (ISO, QCOW2 Disk Container, QCOW2, VMDK), where each item is either
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/utils.ts
Outdated
Show resolved
Hide resolved
| React.useEffect(() => { | ||
| // For live logs (when they are streaming), we auto-scroll to the bottom. | ||
| // For completed logs, we show the logs with scroll at the top | ||
| if (logsRef.current && logs && isStreaming) { | ||
| logsRef.current.scrollTop = logsRef.current.scrollHeight; | ||
| } | ||
| }, [logs, isStreaming]); |
There was a problem hiding this comment.
Reset scroll position for non‑streaming logs.
Line 147-153 only scrolls to bottom when streaming; switching to completed logs can leave the view at the bottom despite the comment. Consider resetting to the top when isStreaming is false (and on source change).
🐛 Suggested adjustment
React.useEffect(() => {
- // For live logs (when they are streaming), we auto-scroll to the bottom.
- // For completed logs, we show the logs with scroll at the top
- if (logsRef.current && logs && isStreaming) {
- logsRef.current.scrollTop = logsRef.current.scrollHeight;
- }
-}, [logs, isStreaming]);
+ // For live logs (when they are streaming), we auto-scroll to the bottom.
+ // For completed logs, we show the logs with scroll at the top
+ if (!logsRef.current || !logs) {
+ return;
+ }
+ logsRef.current.scrollTop = isStreaming ? logsRef.current.scrollHeight : 0;
+}, [logs, isStreaming, selectedEntityId]);🤖 Prompt for AI Agents
In
`@libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildLogsTab.tsx`
around lines 147 - 153, The effect in ImageBuildLogsTab currently only scrolls
to bottom when isStreaming is true, so when logs finish streaming the view can
remain at the bottom; update the React.useEffect that references logsRef, logs
and isStreaming so that when isStreaming is false (i.e., logs completed) you set
logsRef.current.scrollTop = 0 to reset to the top, and ensure the effect also
runs when the log source changes by adding the relevant source identifier (if
present) to the dependency list; locate the effect using the symbols
React.useEffect, logsRef, logs and isStreaming and change the conditional to
handle both branches (scrollHeight when streaming, scrollTop = 0 when not) and
include the source dependency if there is a prop like source/logSource.
There was a problem hiding this comment.
This would keep resetting the scroll to the top as the logs change for completed logs, which is not what we want.
There was a problem hiding this comment.
@celdrake You're absolutely right—resetting on every logs change would be disruptive for users trying to read completed logs.
I notice the comment says "For completed logs, we show the logs with scroll at the top," but the current implementation doesn't actually scroll to the top for completed logs. If you'd like completed logs to initially appear scrolled to the top (just when first loading or switching sources), you could track selectedEntityId in the dependencies instead of logs. But if the current behavior (letting users scroll freely through completed logs) is what you want, then the implementation is fine as-is—perhaps just update the comment to reflect that completed logs maintain user scroll position?
Either way, the current approach makes sense for user experience. 👍
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR `#371` (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
0d56396 to
c6bb6a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
173-186: "View logs" button is missing anonClickhandler.The PR objective states "Adds UI capability to view logs for Image Builds and Image Exports", but this button at line 175 doesn't have an
onClickhandler and won't perform any action when clicked.Proposed fix
<FlexItem> {exists ? ( - <Button variant="secondary">{t('View logs')}</Button> + <Button variant="secondary" onClick={() => onViewLogs?.(format)}>{t('View logs')}</Button> ) : (You'll also need to add
onViewLogsto the component props:export type ImageExportFormatCardProps = { // ... existing props onViewLogs?: (format: ExportFormatType) => void; };
🧹 Nitpick comments (2)
libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildLogsTab.tsx (1)
97-99: Inconsistent return object structure.The early return includes
availableExportFormatswhich is not destructured at line 85. Consider returninghasExports: falsefor consistency with the normal return path.🔧 Suggested fix
// ImageExports can only exist once the ImageBuild is complete if (!isImageBuildComplete(imageBuild)) { - return { selectableEntities: entities, availableExportFormats: [] as ExportFormatType[], failedExportsCount: 0 }; + return { selectableEntities: entities, hasExports: false, failedExportsCount: 0 }; }libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/RegistrationStep.tsx (1)
166-186: Consider placing the SSH key helper text inside the checkbox scope.The helper text at lines 182-184 describing SSH public key paste instructions is always visible, even when remote access is disabled. This could be confusing since the instruction is only relevant when the feature is enabled.
🔧 Suggested adjustment
Move the helper text inside the
CheckboxFieldchildren or usehelperTextprop on theUploadFieldinstead:<UploadField name="userConfiguration.publickey" label={t('Public key')} /> + <Content component="small"> + {t('Paste the content of an SSH public key you want to use to connect to the device.')} + </Content> </CheckboxField> - <Content component="small"> - {t('Paste the content of an SSH public key you want to use to connect to the device.')} - </Content> </FormSection>
| } else { | ||
| // Clear userConfiguration when disabled | ||
| setFieldValue('userConfiguration.username', ''); | ||
| setFieldValue('userConfiguration.publickey', ''); | ||
| setFieldValue('userConfiguration.enabled', false); | ||
| } |
There was a problem hiding this comment.
why do you clear it out ? we can leave the values, and just ignore them when enabled === false
There was a problem hiding this comment.
Fixed, now users can uncheck the checkbox. The data is kept but ignored.
| <Badge | ||
| isRead={false} | ||
| style={ | ||
| { | ||
| '--pf-v6-c-badge--BackgroundColor': | ||
| 'var(--pf-t--global--icon--color--status--danger--default, #c9190b)', | ||
| } as React.CSSProperties | ||
| } | ||
| > | ||
| {failedExportsCount} | ||
| </Badge> |
There was a problem hiding this comment.
Dont we want to use Label instead ? which supports the error/failed state OOTB
There was a problem hiding this comment.
Actually this badge doesn't make sense anymore.
Initially the selector between ImageBuild/Exports were two buttons, and for export format there was an additional dropdown. The badge would show in the "Exports" button.
Now the design changed to a MenuList with two MenuGroup, and I see no badge. I'll remove it.
celdrake
force-pushed
the
EDM-3207-add-logs-and-user-config
branch
from
2d14a08 to
f193f30
Compare
celdrake
force-pushed
the
EDM-3207-add-logs-and-user-config
branch
2 times, most recently
from
b663dc4 to
7cde0fa
Compare
celdrake
force-pushed
the
EDM-3207-add-logs-and-user-config
branch
from
7cde0fa to
29053d1
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.