OCPBUGS-76283: Show modal when downloading pod logs #16014
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@logonoff: This pull request references Jira Issue OCPBUGS-76283, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-76283, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/core
logonoff
changed the title
logonoff
force-pushed
the
OCPBUGS-76283-log-ux
branch
from
e090472 to
4d4fd67
Compare
|
@logonoff: This pull request references Jira Issue OCPBUGS-76283, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
OCPBUGS-76283-log-ux
branch
from
4d4fd67 to
8962a0c
Compare
📝 WalkthroughWalkthroughThis change introduces a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@frontend/packages/console-shared/src/components/modals/FetchProgressModal.tsx`:
- Around line 73-125: The fetchData function must validate the Response before
streaming: after awaiting coFetch(urlRef.current, ...) check response.ok and
that response.body exists; if response.ok is false or response.body is
null/undefined, set an appropriate error via setErrorMessage (use
response.status/text or t('Could not fetch data')), setEndState('failed'),
setIsDownloading(false) and return early to avoid calling
response.body.getReader(); this guard should live just after the coFetch call in
fetchData to prevent treating error responses as successful streams.
In `@frontend/public/components/utils/resource-log.tsx`:
- Around line 716-723: handleFetchedLogData currently passes detect() result
straight to TextDecoder which can be null or an unsupported label; update the
function to validate and normalize the encoding: call detect(new
Uint8Array(arrayBuffer)), if it returns null or an unsupported label map it to
'utf-8' (or another TextDecoder-supported label), then wrap the new
TextDecoder(...).decode(...) and the Blob/URL creation in a try-catch to handle
decode errors (logging or showing a user error) and ensure URL.revokeObjectURL
is used after open; references: handleFetchedLogData, detect, TextDecoder,
URL.createObjectURL.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/modals/__tests__/FetchProgressModal.spec.tsx (1)
72-103: Add an assertion that the data callback fires on success.The success test validates UI and state, but not that the fetched payload is delivered to the callback—this is core behavior and worth locking down.
Suggested test enhancement
await waitFor(() => { expect(setIsDownloading).toHaveBeenCalledWith(false); }); + await waitFor(() => { + expect(mockCallback).toHaveBeenCalledWith(expect.any(ArrayBuffer)); + }); rerender( <FetchProgressModal
| const fetchData = useCallback(async () => { | ||
| setBytesDownloaded(0); | ||
| abortControllerRef.current = new AbortController(); | ||
|
|
||
| try { | ||
| const response = await coFetch(urlRef.current, { | ||
| signal: abortControllerRef.current.signal, | ||
| }); | ||
|
|
||
| const reader = response.body.getReader(); | ||
| const chunks: Uint8Array[] = []; | ||
| let received = 0; | ||
|
|
||
| try { | ||
| // Stream reading requires an indefinite loop that exits when done is true | ||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| // Sequential reads are required; each chunk depends on the previous read completing | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
|
|
||
| chunks.push(value); | ||
| received += value.length; | ||
| setBytesDownloaded(received); | ||
| } | ||
|
|
||
| const arrayBuffer = new Uint8Array(received); | ||
| let position = 0; | ||
| for (const chunk of chunks) { | ||
| arrayBuffer.set(chunk, position); | ||
| position += chunk.length; | ||
| } | ||
|
|
||
| callbackRef.current(arrayBuffer.buffer); | ||
| setEndState('complete'); | ||
| } finally { | ||
| reader.releaseLock(); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof DOMException && error.name === 'AbortError') { | ||
| setErrorMessage(t('Download canceled')); | ||
| // Download was cancelled, do nothing | ||
| return; | ||
| } | ||
| // eslint-disable-next-line no-console | ||
| console.error('Failed to fetch:', error); | ||
| setErrorMessage(error?.message || t('Could not fetch data')); | ||
| } finally { | ||
| abortControllerRef.current = null; | ||
| setIsDownloading(false); | ||
| setEndState((prev) => (prev === 'complete' ? 'complete' : 'failed')); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for coFetch implementation
fd -type f -name '*.ts' -o -name '*.tsx' | xargs rg -l 'export.*coFetch|function coFetch' | head -20Repository: openshift/console
Length of output: 404
🏁 Script executed:
# Also search in common directories
fd -type f \( -name '*fetch*.ts' -o -name '*fetch*.tsx' \) | head -20Repository: openshift/console
Length of output: 231
🏁 Script executed:
# Search for coFetch definition more broadly
rg 'const coFetch|export.*coFetch' -A 5 | head -100Repository: openshift/console
Length of output: 3008
🏁 Script executed:
# Find the co-fetch implementation
fd -name 'co-fetch*' -type fRepository: openshift/console
Length of output: 291
🏁 Script executed:
# Search in console/internal
find . -path '*console/internal*' -name '*fetch*' -type f | grep -v node_modules | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find where coFetch is defined
rg 'export.*const coFetch|export function coFetch' -A 20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find co-fetch file more carefully
find . -name 'co-fetch.ts' -o -name 'co-fetch.tsx' 2>/dev/null | head -5Repository: openshift/console
Length of output: 90
🏁 Script executed:
# Check the FetchProgressModal import
head -50 frontend/packages/console-shared/src/components/modals/FetchProgressModal.tsx | grep -A 2 -B 2 'coFetch\|import'Repository: openshift/console
Length of output: 528
🏁 Script executed:
# Search for coFetch in internal directory
rg 'coFetch' frontend/ --type ts --type tsx -l | head -20Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Read the coFetch implementation
cat ./frontend/public/co-fetch.tsRepository: openshift/console
Length of output: 4841
🏁 Script executed:
# Check test cases for FetchProgressModal
cat frontend/packages/console-shared/src/components/modals/__tests__/FetchProgressModal.spec.tsxRepository: openshift/console
Length of output: 8063
🏁 Script executed:
# Search for coFetch implementations in SDK or elsewhere
rg 'export.*consoleFetch|export.*coFetch' -A 10 | head -80Repository: openshift/console
Length of output: 7271
🏁 Script executed:
# Get the full console-fetch.ts implementation
cat frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.tsRepository: openshift/console
Length of output: 6752
🏁 Script executed:
# Check getUtilsConfig to see what appFetch is
rg 'appFetch' -A 5 -B 5 | head -100Repository: openshift/console
Length of output: 10212
🏁 Script executed:
# Look for configSetup and getUtilsConfig
find . -name '*configSetup*' -o -name '*config*' | grep -E '(utils|setup)' | head -10Repository: openshift/console
Length of output: 400
🏁 Script executed:
# Search for appFetch implementation in the main console app
rg 'appFetch.*=' -A 10 | head -150Repository: openshift/console
Length of output: 2157
🏁 Script executed:
# Look for where coFetch is initialized or configured
rg 'coFetch|appFetch' frontend/public --type ts -A 3 -B 3 | head -100Repository: openshift/console
Length of output: 6490
Add validation for response status and body before streaming.
coFetch returns the raw Response without validating status codes. If the server returns non-2xx or a response without body, the component will either render "success" for an error payload or throw a low-signal TypeError when calling getReader().
Add this guard after the coFetch call:
Required fix
const response = await coFetch(urlRef.current, {
signal: abortControllerRef.current.signal,
});
+ if (!response.ok || !response.body) {
+ throw new Error(t('Could not fetch data'));
+ }
const reader = response.body.getReader();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchData = useCallback(async () => { | |
| setBytesDownloaded(0); | |
| abortControllerRef.current = new AbortController(); | |
| try { | |
| const response = await coFetch(urlRef.current, { | |
| signal: abortControllerRef.current.signal, | |
| }); | |
| const reader = response.body.getReader(); | |
| const chunks: Uint8Array[] = []; | |
| let received = 0; | |
| try { | |
| // Stream reading requires an indefinite loop that exits when done is true | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| // Sequential reads are required; each chunk depends on the previous read completing | |
| // eslint-disable-next-line no-await-in-loop | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| chunks.push(value); | |
| received += value.length; | |
| setBytesDownloaded(received); | |
| } | |
| const arrayBuffer = new Uint8Array(received); | |
| let position = 0; | |
| for (const chunk of chunks) { | |
| arrayBuffer.set(chunk, position); | |
| position += chunk.length; | |
| } | |
| callbackRef.current(arrayBuffer.buffer); | |
| setEndState('complete'); | |
| } finally { | |
| reader.releaseLock(); | |
| } | |
| } catch (error) { | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| setErrorMessage(t('Download canceled')); | |
| // Download was cancelled, do nothing | |
| return; | |
| } | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to fetch:', error); | |
| setErrorMessage(error?.message || t('Could not fetch data')); | |
| } finally { | |
| abortControllerRef.current = null; | |
| setIsDownloading(false); | |
| setEndState((prev) => (prev === 'complete' ? 'complete' : 'failed')); | |
| } | |
| const fetchData = useCallback(async () => { | |
| setBytesDownloaded(0); | |
| abortControllerRef.current = new AbortController(); | |
| try { | |
| const response = await coFetch(urlRef.current, { | |
| signal: abortControllerRef.current.signal, | |
| }); | |
| if (!response.ok || !response.body) { | |
| throw new Error(t('Could not fetch data')); | |
| } | |
| const reader = response.body.getReader(); | |
| const chunks: Uint8Array[] = []; | |
| let received = 0; | |
| try { | |
| // Stream reading requires an indefinite loop that exits when done is true | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| // Sequential reads are required; each chunk depends on the previous read completing | |
| // eslint-disable-next-line no-await-in-loop | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| chunks.push(value); | |
| received += value.length; | |
| setBytesDownloaded(received); | |
| } | |
| const arrayBuffer = new Uint8Array(received); | |
| let position = 0; | |
| for (const chunk of chunks) { | |
| arrayBuffer.set(chunk, position); | |
| position += chunk.length; | |
| } | |
| callbackRef.current(arrayBuffer.buffer); | |
| setEndState('complete'); | |
| } finally { | |
| reader.releaseLock(); | |
| } | |
| } catch (error) { | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| setErrorMessage(t('Download canceled')); | |
| // Download was cancelled, do nothing | |
| return; | |
| } | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to fetch:', error); | |
| setErrorMessage(error?.message || t('Could not fetch data')); | |
| } finally { | |
| abortControllerRef.current = null; | |
| setIsDownloading(false); | |
| setEndState((prev) => (prev === 'complete' ? 'complete' : 'failed')); | |
| } |
🤖 Prompt for AI Agents
In
`@frontend/packages/console-shared/src/components/modals/FetchProgressModal.tsx`
around lines 73 - 125, The fetchData function must validate the Response before
streaming: after awaiting coFetch(urlRef.current, ...) check response.ok and
that response.body exists; if response.ok is false or response.body is
null/undefined, set an appropriate error via setErrorMessage (use
response.status/text or t('Could not fetch data')), setEndState('failed'),
setIsDownloading(false) and return early to avoid calling
response.body.getReader(); this guard should live just after the coFetch call in
fetchData to prevent treating error responses as successful streams.
| /** callback for FetchProgressModal to process fetched log data */ | ||
| const handleFetchedLogData = useCallback((arrayBuffer: ArrayBuffer) => { | ||
| const encoding = detect(new Uint8Array(arrayBuffer)); | ||
| const text = new TextDecoder(encoding).decode(arrayBuffer); | ||
| const blob = new Blob([text], { type: 'text/plain;charset=utf-8' }); | ||
| const url = URL.createObjectURL(blob); | ||
| window.open(url, '_blank'); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
chardet detect() return value null unsupported encoding javascript
💡 Result:
- In the
chardet(runk/node-chardet) npm package,detect()is typed to returnstring | null—sonullis an expected result when the library can’t make a confident guess from the bytes you provided. [1] - Make sure you’re passing the original bytes (
Buffer/Uint8Array), not a JS string (strings are already UTF‑16 in memory, which makes “detecting the original encoding” meaningless and can lead to bad/ambiguous results). [2] chardetsupports only a fixed set of encodings (UTF-8/16/32, ISO-2022-, Shift_JIS, Big5, EUC-, GB18030, ISO-8859-, windows-125, KOI8-R, etc.). If the real data is outside these, detection may be unreliable and can yieldnullor a “best effort” guess. [2]
Why you then see “unsupported encoding” in JavaScript/Node
Even if chardet.detect() returns a string, that encoding label may not be supported by Buffer.toString() / TextDecoder (Node only supports a small set like utf8, utf16le, latin1, etc.). So doing this often fails:
const enc = chardet.detect(buf); // e.g. "windows-1252" or "UTF-16 LE"
const text = buf.toString(enc); // throws: unknown/unsupported encodingWhat to do
- Handle
nulland/or useanalyse()to inspect confidence and apply a threshold. [2] - Normalize encoding names for Node built-ins (example:
"UTF-16 LE"→"utf16le"). - For encodings like
windows-1252, decode with a library such asiconv-lite(since Node’sBufferwon’t decode it directly).
Sources:
[1] chardet API docs showing detect: (buffer: Uint8Array) => string | null https://www.jsdocs.io/package/chardet (v2.1.1)
[2] chardet README (bytes-only input note + supported encodings list) https://npm.io/package/chardet
🏁 Script executed:
cat frontend/public/components/utils/resource-log.tsx | head -750 | tail -50Repository: openshift/console
Length of output: 1838
🏁 Script executed:
rg "detect\|chardet" frontend/public/components/utils/resource-log.tsx -B 3 -A 3Repository: openshift/console
Length of output: 43
🏁 Script executed:
rg "from.*detect\|import.*detect" frontend/public/components/utils/resource-log.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
head -100 frontend/public/components/utils/resource-log.tsx | rg "import|detect"Repository: openshift/console
Length of output: 1818
🏁 Script executed:
sed -n '1,50p' frontend/public/components/utils/resource-log.tsxRepository: openshift/console
Length of output: 1034
🏁 Script executed:
rg "handleFetchedLogData" frontend/public/components/utils/resource-log.tsx -B 5 -A 15Repository: openshift/console
Length of output: 1427
Guard against null returns and unsupported encoding labels from chardet.detect().
The detect() function returns string | null, and even when it returns a string, that encoding label may not be supported by TextDecoder (chardet supports ~30 encodings; TextDecoder supports a subset). Currently there's no null check or error handling, so TextDecoder will throw and the user's download is incorrectly marked as failed. Add a null guard, normalize to a supported label (e.g., UTF-8), and wrap in try-catch:
Safer decode with fallback
- const encoding = detect(new Uint8Array(arrayBuffer));
- const text = new TextDecoder(encoding).decode(arrayBuffer);
+ const detected = detect(new Uint8Array(arrayBuffer));
+ const encoding = typeof detected === 'string' ? detected : 'utf-8';
+ let text: string;
+ try {
+ text = new TextDecoder(encoding).decode(arrayBuffer);
+ } catch {
+ text = new TextDecoder('utf-8').decode(arrayBuffer);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** callback for FetchProgressModal to process fetched log data */ | |
| const handleFetchedLogData = useCallback((arrayBuffer: ArrayBuffer) => { | |
| const encoding = detect(new Uint8Array(arrayBuffer)); | |
| const text = new TextDecoder(encoding).decode(arrayBuffer); | |
| const blob = new Blob([text], { type: 'text/plain;charset=utf-8' }); | |
| const url = URL.createObjectURL(blob); | |
| window.open(url, '_blank'); | |
| }, []); | |
| /** callback for FetchProgressModal to process fetched log data */ | |
| const handleFetchedLogData = useCallback((arrayBuffer: ArrayBuffer) => { | |
| const detected = detect(new Uint8Array(arrayBuffer)); | |
| const encoding = typeof detected === 'string' ? detected : 'utf-8'; | |
| let text: string; | |
| try { | |
| text = new TextDecoder(encoding).decode(arrayBuffer); | |
| } catch { | |
| text = new TextDecoder('utf-8').decode(arrayBuffer); | |
| } | |
| const blob = new Blob([text], { type: 'text/plain;charset=utf-8' }); | |
| const url = URL.createObjectURL(blob); | |
| window.open(url, '_blank'); | |
| }, []); |
🤖 Prompt for AI Agents
In `@frontend/public/components/utils/resource-log.tsx` around lines 716 - 723,
handleFetchedLogData currently passes detect() result straight to TextDecoder
which can be null or an unsupported label; update the function to validate and
normalize the encoding: call detect(new Uint8Array(arrayBuffer)), if it returns
null or an unsupported label map it to 'utf-8' (or another TextDecoder-supported
label), then wrap the new TextDecoder(...).decode(...) and the Blob/URL creation
in a try-catch to handle decode errors (logging or showing a user error) and
ensure URL.revokeObjectURL is used after open; references: handleFetchedLogData,
detect, TextDecoder, URL.createObjectURL.
8962a0c to
588c24b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @yapei |
|
New changes are detected. LGTM label has been removed. |
|
/assign @XiyunZhao |
openshift-merge-robot
added
the
needs-rebase
logonoff
force-pushed
the
OCPBUGS-76283-log-ux
branch
from
cf46905 to
62374bb
Compare
openshift-merge-robot
removed
the
needs-rebase
logonoff
force-pushed
the
OCPBUGS-76283-log-ux
branch
from
62374bb to
790d653
Compare
|
@logonoff: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hi @logonoff, I encountered a potential edge case while verifying this PR Here is the script I used |
6 participants
Before this update, clicking View raw logs when a resource log is very long would cause the console to hang while it logs the logs. With this release, a modal now appears which shows the current log download progress, which also allows cancellation of the download.
Screencast.From.2026-02-11.16-47-20.mp4