NO-JIRA: refactor: Separate custom hooks for re-use#191
NO-JIRA: refactor: Separate custom hooks for re-use#191openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
|
@alanconway: This pull request explicitly references no jira issue. 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. |
dcb983e to
da6647a
Compare
| netobserveAvailable: boolean; | ||
| constraint: korrel8r.Constraint; | ||
| }> = ({ domains, graph, loggingAvailable, netobserveAvailable, constraint }) => { | ||
| }> = ({ graph, loggingAvailable, netobserveAvailable, constraint }) => { |
There was a problem hiding this comment.
If we aren't going to use the domains prop anymore lets remove it from the prop type and not pass it in where the component is used
83d8813 to
91aa6e8
Compare
|
/retest |
cfc6f3b to
a092cd4
Compare
|
/retest Another OOMKill. Added --runInBand to the unit test runner - runs about 3x faster with 10x less memory so hopefully this will fix the proble. |
|
/lgtm |
a092cd4 to
96a9345
Compare
WalkthroughRefactors domain handling and navigation by introducing hooks ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
/label qe-approved |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Korrel8rPanel.tsx (1)
225-233:⚠️ Potential issue | 🟡 Minor
domainsprop is passed toTopologybut never used — dead code from incomplete refactor.
TopologyPropsstill declaresdomains(line 254) andKorrel8rPanelstill passes it (line 227), but theTopologycomponent destructures only{ result, t, constraint }(line 261) —domainsis silently ignored. SinceKorrel8rTopologynow sources domains internally via hooks, this prop should be removed entirely.Suggested fix
interface TopologyProps { - domains: korrel8r.Domains; result?: Result; constraint: korrel8r.Constraint; t: TFunction; setSearch: (search: Search) => void; }const topologySection = ( <Topology - domains={domains} result={result} constraint={search.constraint} t={t} setSearch={setSearch} /> );And in
Korrel8rPanel, theuseDomains()call on line 53 is now unused and can also be removed:- const domains = useDomains(); const locationQuery = useLocationQuery();Note:
useLocationQueryalready callsuseDomains()internally, so there's no need to call it separately inKorrel8rPanel.Also applies to: 253-261
🧹 Nitpick comments (1)
web/src/hooks/useDomains.tsx (1)
15-18: DuplicateAlertDomainin the domains list.
allDomains(fromweb/src/korrel8r/all-domains.ts) already containsnew AlertDomain(). Spreading it here and then appending anothernew AlertDomain(alertIDs)creates a duplicate that silently overwrites the first via theMap-basedDomains.set. It works, but is misleading and fragile — a future reader (or a change toDomains) might not expect the overwrite.Consider filtering out the existing
AlertDomainto make the intent explicit:Suggested fix
- () => new Domains(...allDomains, new AlertDomain(alertIDs)), + () => new Domains(...allDomains.filter(d => d.name !== 'alert'), new AlertDomain(alertIDs)),
- useNavigateToQuery - useDomain
96a9345 to
78c4f19
Compare
|
My first coderabbit spanking! Fixed 2 minor issues. |
|
@alanconway: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alanconway, PeterYurkovich 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 |
/cc @PeterYurkovich