OU-1315: reset queries when namespace changes in dev perspective#932
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1315 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 bug to target the "5.0.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. |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich, zhuje 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 |
WalkthroughThe PR updates ChangesNamespace Change Detection with Conditional Query Clearing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/MetricsPage.tsx (1)
1407-1415: ⚡ Quick winFix
useEffectdependencies in namespace-reset logic.The effect at line 1407 reads
displayNamespaceSelectoron line 1410 but it's missing from the dependency array[namespace, dispatch, setNamespace]. Meanwhile,setNamespaceis in the array but never used in the effect body. This creates a stale closure risk ifdisplayNamespaceSelectorchanges during component lifetime. Also resetprevNamespace.currentwhen the selector becomes visible to keep the baseline in sync.Proposed patch
const prevNamespace = useRef(namespace); useEffect(() => { // In the developer perspective (display namespace selector is false since all dev perspective // pages add them automatically) then clear queries when the namespace changes - if (prevNamespace.current === namespace || displayNamespaceSelector) { + if (displayNamespaceSelector) { + prevNamespace.current = namespace; + return; + } + if (prevNamespace.current === namespace) { return; } prevNamespace.current = namespace; dispatch(queryBrowserDeleteAllQueries()); - }, [namespace, dispatch, setNamespace]); + }, [namespace, displayNamespaceSelector, dispatch]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/MetricsPage.tsx` around lines 1407 - 1415, The effect uses displayNamespaceSelector but doesn't include it in the dependency array and includes setNamespace which is unused; update the useEffect that references prevNamespace, namespace, displayNamespaceSelector, dispatch and queryBrowserDeleteAllQueries to: add displayNamespaceSelector to the dependency array, remove setNamespace from the dependencies, and inside the effect when displayNamespaceSelector becomes true reset prevNamespace.current (e.g. to undefined/null) so the baseline is in sync; keep the existing logic that compares prevNamespace.current to namespace and dispatches queryBrowserDeleteAllQueries() when it changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/src/components/MetricsPage.tsx`:
- Around line 1407-1415: The effect uses displayNamespaceSelector but doesn't
include it in the dependency array and includes setNamespace which is unused;
update the useEffect that references prevNamespace, namespace,
displayNamespaceSelector, dispatch and queryBrowserDeleteAllQueries to: add
displayNamespaceSelector to the dependency array, remove setNamespace from the
dependencies, and inside the effect when displayNamespaceSelector becomes true
reset prevNamespace.current (e.g. to undefined/null) so the baseline is in sync;
keep the existing logic that compares prevNamespace.current to namespace and
dispatches queryBrowserDeleteAllQueries() when it changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fc403dce-50b0-4f64-988c-9c4c1cffcd0f
📒 Files selected for processing (1)
web/src/components/MetricsPage.tsx
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
@PeterYurkovich: This pull request references OU-1315 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 bug to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
|
/cherry-pick release-4.22 |
|
@PeterYurkovich: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
@PeterYurkovich: 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. |
|
@PeterYurkovich: new pull request created: #934 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 kubernetes-sigs/prow repository. |
5 participants
Summary by CodeRabbit