OU-1144: fix: request namespaced resources for filter options#337
OU-1144: fix: request namespaced resources for filter options#337jgbernalp wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgbernalp: This pull request references OU-1144 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 "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. |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
/retest |
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
@jgbernalp: This pull request references OU-1144 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 "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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@jgbernalp I can’t see pods whose logs are not being sent out. Is that correct? Do we want to display all pods no matter logs are sent to loki or not ? |
|
@anpingli yes. This will be an indicator that something is wrong with the collector or the application producing logs not reaching loki. We merge the k8s api results with loki labels results, to show resources without logs or deleted resources logs. |
|
@jgbernalp the bold lines below are not as expected. when log in as kubeadmin,the namespaces can be displayed base on tenant. that is correct when log in as non-cluster-admin users with role belowoc adm policy add-cluster-role-to-user view ${user_name} the namespaces can be displayed base on tenant. that is correct. Step to test:
Expected Result: Actual Result: |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
add-namespaced-filters
branch
from
2468253 to
36ad8f8
Compare
openshift-ci
Bot
removed
the
needs-rebase
WalkthroughThis pull request refactors the attribute filtering system to support namespace-scoped Kubernetes resource queries and empty state handling. It updates type signatures to pass filters instead of search queries, adds namespace-aware abort handling for concurrent requests, and introduces configurable empty state messages for namespaces, pods, and containers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@jgbernalp: This pull request references OU-1144 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. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/filters/search-select.tsx (1)
38-48:⚠️ Potential issue | 🟠 MajorRules of Hooks violation:
useTranslationinside a plain helper.
getOptionComponentsis a regular function (not a component or custom hook), but it calls theuseTranslationhook. This violates React's Rules of Hooks —react-hooks/rules-of-hookswill flag it, and the behavior only appears stable because the parent always calls this helper the same number of times per render.Either promote the helper to a component (
<OptionComponents ... />), or lift theuseTranslationcall into the parentSearchSelect(which already callsuseTranslationon line 121) and pass the translated fallback in:🛡️ Proposed fix
const getOptionComponents = ( attributeOptions: SelectOptionProps[], attributeError: Error | undefined, attributeLoading: boolean, selections: Array<string>, focusedItemIndex: number | null, onInputKeyDown: (event: React.KeyboardEvent) => void, - emptyStateMessage?: string | null, + emptyStateMessage?: string | null, + noResultsFallback: string = 'No results found', ) => { - const { t } = useTranslation('plugin__logging-view-plugin'); - if (attributeLoading) { ... } ... - title={emptyStateMessage || t('No results found')} + title={emptyStateMessage || noResultsFallback}And in the caller (line 400-408), pass
t('No results found').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/filters/search-select.tsx` around lines 38 - 48, getOptionComponents is a plain helper that incorrectly calls the React hook useTranslation (violating Rules of Hooks); move the translation out of this helper by removing useTranslation from getOptionComponents and instead accept a translated empty state string (e.g., emptyStateMessage or noResultsText) as a parameter, then update the caller (SearchSelect) which already calls useTranslation to pass t('No results found') (or the appropriate translation) into getOptionComponents; alternatively, convert getOptionComponents into a proper React component (e.g., OptionComponents) that can call useTranslation itself.web/src/pages/logs-dev-page.tsx (1)
199-205:⚠️ Potential issue | 🟡 MinorAvoid mutating the
filtersstate object in place.
filtersWithNamespace = filters ?? {}keeps the same reference as the React statefilterswhen it's defined, sofiltersWithNamespace['namespace'] = new Set(...)mutates the state object directly. This bypasses React's change-detection and can cause downstream consumers that compare by reference (e.g., memoized selectors, effect dependencies, or theSearchSelecteffect readingfilters) to miss the update. Clone before assigning:🛡️ Proposed fix
- const filtersWithNamespace = filters ?? {}; - - filtersWithNamespace['namespace'] = new Set(namespace ? [namespace] : []); + const filtersWithNamespace: Filters = { + ...(filters ?? {}), + namespace: new Set(namespace ? [namespace] : []), + }; const queryToUse = updateQuery(filtersWithNamespace, tenant);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/logs-dev-page.tsx` around lines 199 - 205, The code mutates the React state object by assigning namespace onto filtersWithNamespace which reuses the filters reference; to fix, create a shallow clone first (e.g., copy the object returned by filters ?? {}) before setting filtersWithNamespace['namespace'] = new Set(...), then pass that cloned object into updateQuery/runQuery; update the block around filtersWithNamespace, updateQuery, and runQuery (where namespace and configLoaded are used) so downstream consumers (e.g., SearchSelect) see a new reference instead of an in-place mutation.
🧹 Nitpick comments (2)
web/cypress/e2e/integration/logs-dev-page.cy.ts (1)
321-321: Remove emptywithinblock.
cy.byTestID(TestIds.AttributeFilters).within(() => {});is a no-op and looks like leftover scaffolding — either add the intended assertions (e.g., asserting the new empty-state alert renders) or drop the line.🧹 Proposed cleanup
- cy.byTestID(TestIds.AttributeFilters).within(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/integration/logs-dev-page.cy.ts` at line 321, The test contains a no-op within call: cy.byTestID(TestIds.AttributeFilters).within(() => {}); — either remove this empty within block or replace it with the intended assertions (for example, check for the empty-state alert or other elements under TestIds.AttributeFilters). Locate the usage of TestIds.AttributeFilters and the within(...) call in logs-dev-page.cy.ts and delete the empty within closure or populate it with the specific assertions you expect to run against the AttributeFilters container.web/src/hooks/useURLState.ts (1)
107-110: Memo deps don't includet.
tis now part of thegetAttributesinputs but isn't in theuseMemodependency array. If the i18n language changes at runtime,t's reference changes but memoized attributes (including any translatedemptyStateMessage/labels produced viat) won't be recomputed, leaving stale strings. Consider addingt(ori18n.language) to the dependency list.- [tenant, config, schema, ...(attributesDependencies || [])], + [tenant, config, schema, t, ...(attributesDependencies || [])],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/useURLState.ts` around lines 107 - 110, In useURLState update the React.useMemo for attributes so it re-runs when the translation function changes: include t (or i18n.language) in the dependency array for the memoized computation that calls getAttributes({ tenant, config, schema, t }); specifically update the dependencies for attributes (used in useURLState) to [tenant, config, schema, t, ...(attributesDependencies || [])] or use i18n.language instead of t to ensure translated labels like emptyStateMessage are recomputed when language changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/attribute-filters.tsx`:
- Around line 874-904: The current podResource fallback calls
resourceDataSource({ resource: 'pods', ... }) with no namespace, causing a
cluster-wide /api/v1/pods request (403 for namespace-scoped roles); instead,
derive the set of allowed tenant namespaces first and build podResource as an
array of per-namespace requests by calling resourceDataSource({ resource:
'pods', namespace: <eachNamespace>, filter: ... }) for each allowed namespace
(respecting tenant via the existing filter switch), then use that array in the
Promise.allSettled call alongside lokiLabelValuesDataSource (keep symbols
podResource, resourceDataSource, namespacesQuery, tenant, podLabel,
mergeSettledResults) so no cluster-wide endpoint is requested when no namespace
filter is selected.
In `@web/src/components/filters/search-select.tsx`:
- Around line 261-263: The useEffect in SearchSelect currently calls
getAttributeOptions(filters) but only lists [tenant] as a dependency; update the
effect to include the relevant filter slice (or at minimum include filters) and
getAttributeOptions in its dependency array so options are refetched whenever
filters change (e.g., add filters and getAttributeOptions to the deps), or if
you intentionally rely on remounting via attribute-filter.tsx's namespacesKey,
add a clear comment above the effect documenting that contract to avoid
regressions; reference the useEffect in SearchSelect and the getAttributeOptions
call when making the change.
---
Outside diff comments:
In `@web/src/components/filters/search-select.tsx`:
- Around line 38-48: getOptionComponents is a plain helper that incorrectly
calls the React hook useTranslation (violating Rules of Hooks); move the
translation out of this helper by removing useTranslation from
getOptionComponents and instead accept a translated empty state string (e.g.,
emptyStateMessage or noResultsText) as a parameter, then update the caller
(SearchSelect) which already calls useTranslation to pass t('No results found')
(or the appropriate translation) into getOptionComponents; alternatively,
convert getOptionComponents into a proper React component (e.g.,
OptionComponents) that can call useTranslation itself.
In `@web/src/pages/logs-dev-page.tsx`:
- Around line 199-205: The code mutates the React state object by assigning
namespace onto filtersWithNamespace which reuses the filters reference; to fix,
create a shallow clone first (e.g., copy the object returned by filters ?? {})
before setting filtersWithNamespace['namespace'] = new Set(...), then pass that
cloned object into updateQuery/runQuery; update the block around
filtersWithNamespace, updateQuery, and runQuery (where namespace and
configLoaded are used) so downstream consumers (e.g., SearchSelect) see a new
reference instead of an in-place mutation.
---
Nitpick comments:
In `@web/cypress/e2e/integration/logs-dev-page.cy.ts`:
- Line 321: The test contains a no-op within call:
cy.byTestID(TestIds.AttributeFilters).within(() => {}); — either remove this
empty within block or replace it with the intended assertions (for example,
check for the empty-state alert or other elements under
TestIds.AttributeFilters). Locate the usage of TestIds.AttributeFilters and the
within(...) call in logs-dev-page.cy.ts and delete the empty within closure or
populate it with the specific assertions you expect to run against the
AttributeFilters container.
In `@web/src/hooks/useURLState.ts`:
- Around line 107-110: In useURLState update the React.useMemo for attributes so
it re-runs when the translation function changes: include t (or i18n.language)
in the dependency array for the memoized computation that calls getAttributes({
tenant, config, schema, t }); specifically update the dependencies for
attributes (used in useURLState) to [tenant, config, schema, t,
...(attributesDependencies || [])] or use i18n.language instead of t to ensure
translated labels like emptyStateMessage are recomputed when language changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbd06a59-1a88-4272-b923-2d4810855d6c
📒 Files selected for processing (10)
hack/docker-compose/loki/promtail-config.ymlweb/cypress/e2e/integration/logs-dev-page.cy.tsweb/locales/en/plugin__logging-view-plugin.jsonweb/src/attribute-filters.tsxweb/src/components/filters/attribute-filter.tsxweb/src/components/filters/attribute-value-data.tsxweb/src/components/filters/filter.types.tsweb/src/components/filters/search-select.tsxweb/src/hooks/useURLState.tsweb/src/pages/logs-dev-page.tsx
|
/cherry-pick release-6.2 |
|
@jgbernalp: 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. |
|
/cherry-pick release-6.1 |
|
@jgbernalp: 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. |
|
@jgbernalp: 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. |
|
/cherry-pick release-coo-ocp-4.22 |
|
@jgbernalp: 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. |
|
/cherry-pick release-coo-ocp-4.15 |
|
@jgbernalp: 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. |
|
/cherry-pick release-coo-ocp-4.12 |
|
@jgbernalp: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, 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 |
8 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation