CONSOLE-5039: Remove useSafetyFirst #15937
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5039 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
/label px-approved |
openshift-ci
bot
added
px-approved
openshift-ci
bot
added
component/core
|
@logonoff: This pull request references CONSOLE-5039 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
📝 WalkthroughWalkthroughThis pull request removes the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@frontend/packages/console-shared/src/hooks/usePodsWatcher.ts`:
- Line 1: The state podData in usePodsWatcher is initialized as undefined but
the hook's return type declares podData: PodRCData, causing a mismatch; update
the podData state/type to allow undefined (e.g., PodRCData | undefined) and
adjust the hook's return signature so podData can be undefined until loaded is
true, referencing the podData state declaration and the PodRCData type and the
usePodsWatcher return type to locate the changes.
🧹 Nitpick comments (3)
frontend/packages/webterminal-plugin/src/components/cloud-shell/useCloudShellWorkspace.ts (1)
149-157: Optional: State setters are stable and can be omitted from dependencies.React guarantees that
useStatesetters have stable identity across renders. IncludingsetNamespace,setNoNamespaceFound, andsetSearchingin the dependency array is harmless but unnecessary. Same applies to the effect on line 41. This is a minor cleanup opportunity.♻️ Suggested cleanup
}, [ isKubeAdmin, searchNamespaces, - setNamespace, uid, - setNoNamespaceFound, - setSearching, workspaceModel, ]);frontend/packages/console-dynamic-plugin-sdk/src/app/components/utils/rbac.tsx (1)
145-156: Optional: Stable setters can be removed from dependency array.Same note as elsewhere in this PR -
setLoadingandsetAllowedare stable references and don't need to be in the dependency array.♻️ Suggested cleanup
}, [ - setLoading, - setAllowed, group, resource, subresource, verb, name, namespace, impersonateKey, skipCheck, ]);frontend/packages/knative-plugin/src/utils/fetch-dynamic-eventsources-utils.ts (1)
248-275: LGTM with minor note on inconsistent dependency array.The migration is correct. Note that line 274 includes
setModelRefsin the dependency array (unnecessary since setters are stable), while the other two hooks in this file use empty arrays. Consider removing it for consistency, or adding the eslint-disable comment like the others if you want to keep lint happy.♻️ Consistency fix
} + // eslint-disable-next-line react-hooks/exhaustive-deps - }, [setModelRefs]); + }, []); return modelRefs;
| @@ -1,5 +1,4 @@ | |||
| import { useMemo, useCallback, useEffect } from 'react'; | |||
| import { useSafetyFirst } from '@console/dynamic-plugin-sdk/src/app/components/safety-first'; | |||
| import { useMemo, useCallback, useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PodRCData type definition
echo "=== Searching for PodRCData definition ==="
rg -n "type PodRCData|interface PodRCData" --type ts --type tsx
echo ""
echo "=== Checking tsconfig.json for strictNullChecks ==="
find . -name "tsconfig.json" -type f | head -10 | while read f; do
echo "File: $f"
rg "strictNullChecks" "$f"
done
echo ""
echo "=== Examining usePodsWatcher.ts file ==="
cat -n frontend/packages/console-shared/src/hooks/usePodsWatcher.ts | head -30Repository: openshift/console
Length of output: 2452
🏁 Script executed:
#!/bin/bash
# Find types file in console-shared
echo "=== Looking for types file in console-shared ==="
find frontend/packages/console-shared/src -name "types*" -type f
echo ""
echo "=== Checking console-shared tsconfig ==="
find frontend/packages/console-shared -name "tsconfig.json" -type f | while read f; do
echo "File: $f"
cat "$f" | grep -A2 -B2 "strictNullChecks"
done
echo ""
echo "=== Searching for PodRCData with better pattern ==="
rg "PodRCData" frontend/packages/console-shared/src --type ts -A 2 -B 2
echo ""
echo "=== Check return type mismatch on line 18 ==="
sed -n '14,25p' frontend/packages/console-shared/src/hooks/usePodsWatcher.tsRepository: openshift/console
Length of output: 10441
🏁 Script executed:
#!/bin/bash
echo "=== Reading PodRCData definition ==="
cat -n frontend/packages/console-shared/src/types/pod.ts
echo ""
echo "=== Looking for tsconfig.json in console-shared parent dirs ==="
find frontend/packages/console-shared -name "tsconfig.json"
echo ""
echo "=== Check frontend root tsconfig ==="
ls -la frontend/tsconfig.json 2>/dev/null && cat frontend/tsconfig.json | grep -A 2 -B 2 "strictNullChecks"Repository: openshift/console
Length of output: 1669
🏁 Script executed:
#!/bin/bash
echo "=== Check tsconfig strictNullChecks ==="
cat frontend/tsconfig.json | grep -A 2 -B 2 "strictNullChecks"
echo ""
echo "=== Find PodRCData actual definition ==="
find . -path ./node_modules -prune -o -name "console-types*" -type f -print
echo ""
echo "=== Search for PodRCData type definition ==="
rg "type PodRCData|interface PodRCData" --type ts -A 5 -B 2Repository: openshift/console
Length of output: 1650
🏁 Script executed:
#!/bin/bash
echo "=== Full usePodsWatcher implementation ==="
cat -n frontend/packages/console-shared/src/hooks/usePodsWatcher.ts
echo ""
echo "=== Check frontend tsconfig for compilerOptions ==="
cat frontend/tsconfig.jsonRepository: openshift/console
Length of output: 5043
Widen podData state type to match runtime behavior.
Line 21 initializes podData with undefined, but the return type (line 18) declares podData: PodRCData without allowing undefined. This creates a type mismatch: consumers expect a fully-formed PodRCData object, but receive undefined on initial render before loaded becomes true. Align the type annotation with the actual state lifecycle.
🔧 Suggested fix
- const [podData, setPodData] = useState<PodRCData>(undefined);
+ const [podData, setPodData] = useState<PodRCData | undefined>(undefined);Also update the return type:
-): { loaded: boolean; loadError: string; podData: PodRCData } => {
+): { loaded: boolean; loadError: string; podData: PodRCData | undefined } => {🤖 Prompt for AI Agents
In `@frontend/packages/console-shared/src/hooks/usePodsWatcher.ts` at line 1, The
state podData in usePodsWatcher is initialized as undefined but the hook's
return type declares podData: PodRCData, causing a mismatch; update the podData
state/type to allow undefined (e.g., PodRCData | undefined) and adjust the
hook's return signature so podData can be undefined until loaded is true,
referencing the podData state declaration and the PodRCData type and the
usePodsWatcher return type to locate the changes.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vojtechszocs 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 |
|
/retest-required |
|
@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. |
4 participants
Per reactwg/react-18#82, the warning that
useSafetyFirstwas meant to suppress did not actually protect against memory leaks.I quickly went through all places where
useSafetyFirstis used, and they all follow the pattern of calling setState after a promise is handled. This was explicitly mentioned in the React WG as not a memory leak because "there's nothing holding onto the component indefinitely", so GC will eventually do its thing.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.