OU-1272: fix config, refresh interval and expanded items memoization to avoid unnecessary re renders#361
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgbernalp: This pull request references OU-1272 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughUpdated test log-generator image/command and promtail pipeline to use JSON Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/refresh-interval-dropdown.tsx (1)
49-77:⚠️ Potential issue | 🟡 MinorConfirm the new mount-time refresh is intentional.
With
delaynow initialized from the persisted selection (line 49) and the effect triggeringonRefreshRef.current?.()once wheneverdelay !== 0(line 72), any page that mounts this dropdown with a stored non-OFF interval will fire an extraonRefreshon mount. Since logs-page also has its ownuseEffect([...configLoaded])that callsrunQuery()when the config loads, pages can end up refreshing twice on initial mount. This appears unintentional given the commit message focuses on "memoization to avoid unnecessary re renders." If the goal is only "refresh immediately when the user picks a new interval," consider gating the immediate call to skip the initial render:🔧 Optional: skip immediate call on first mount
+ const isFirstRun = React.useRef(true); React.useEffect(() => { clearTimer(); if (delay !== 0) { - onRefreshRef.current?.(); + if (!isFirstRun.current) { + onRefreshRef.current?.(); + } timer.current = setInterval(() => onRefreshRef.current?.(), delay); } + isFirstRun.current = false; return () => clearTimer(); }, [delay]);Additionally, the pattern on line 52 (
onRefreshRef.current = onRefresh;during render) is a side effect during render. Consider wrapping it inReact.useLayoutEffector, on React 19.2+, preferuseEffectEventto align with React guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/refresh-interval-dropdown.tsx` around lines 49 - 77, The effect that watches delay causes an immediate onRefresh on mount when delay is initialized from persisted state; to fix it, add a mounted/ref guard so the useEffect that calls onRefreshRef.current only triggers the immediate onRefresh when delay changes after mount (i.e., skip the first run), and move the render-time assignment onRefreshRef.current = onRefresh into a React.useLayoutEffect (or useEffectEvent if available) so you don't perform side effects during render; update the existing symbols delay, timer, onRefreshRef, the useEffect block, and the handleSelectedValue/setStoredRefreshInterval flow to ensure selecting a new interval still triggers an immediate refresh but initial mount does not.
🧹 Nitpick comments (3)
web/src/components/refresh-interval-dropdown.tsx (1)
51-52: This pattern is safe in React 17.0.2; concurrent rendering concern does not apply.Line 52 mutates
onRefreshRef.currentduring render, which would be problematic in React 18+ with concurrent rendering enabled. However, this project targets React 17.0.2, which does not enable concurrent rendering by default. The pattern is safe and intentionally avoids includingonRefreshin theuseEffectdependency array (line 77) to prevent unnecessary effect re-runs.If the project plans to upgrade to React 18+, consider using
React.useLayoutEffect()to move the mutation after commit:const onRefreshRef = React.useRef(onRefresh); - onRefreshRef.current = onRefresh; + React.useLayoutEffect(() => { + onRefreshRef.current = onRefresh; + });(Note:
useEffectEventmentioned in the original suggestion is not available in React 17 or 18; it was introduced in React 19.2.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/refresh-interval-dropdown.tsx` around lines 51 - 52, The current code assigns onRefreshRef.current = onRefresh during render which is unsafe for concurrent rendering; update the component to set onRefreshRef.current inside a lifecycle hook instead (e.g., move the assignment into a React.useLayoutEffect or React.useEffect) so the ref mutation happens after commit; locate the onRefreshRef and onRefresh symbols and the effect that currently omits onRefresh (the effect around useEffect at/near the former line 77) and ensure the hook updates onRefreshRef.current without adding onRefresh to unrelated effect dependency arrays.web/src/components/logs-table.tsx (1)
171-218: Expansion correctness now relies on the parent row memoization always re-rendering.Reading
expandedItemsRef.currentinsideTableRowComponentmeans the row itself never subscribes to the expansion state — it only reflects the latest ref value if its parent happens to re-render. Today this works becauseRowMemoinvirtualized-logs-table.tsxhas a custom comparator that returnsfalse(re-render) unlessisScrolling. If that comparator is ever tightened, or if a row is expanded/collapsed while scrolling is true, the expand UI will silently desynchronize from state.Since
handleRowToggleis already stable, consider keepingexpandedItemsas a real prop (or a derivedisExpandedper row) so memoization remains correctness-safe rather than relying on the memo comparator's current behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/logs-table.tsx` around lines 171 - 218, The TableRowComponent currently reads expansion state from expandedItemsRef.current which makes expansion UI depend on parent re-renders; change TableRow so expansion is passed as a real prop (e.g., add expandedItems:Set<number> or better, an isExpanded:boolean computed per row) instead of reading expandedItemsRef inside TableRowComponent, update TableRow signature and the call sites (where RowMemo in virtualized-logs-table.tsx renders TableRowComponent) to pass the per-row isExpanded and keep handleRowToggle as the stable callback; this ensures TableRow/RowMemo correctness even if the RowMemo comparator is tightened or scrolling is true.web/src/components/virtualized-logs-table.tsx (1)
132-210: Consider invalidating the measurement cache when the dataset identity changes.Memoizing
cellMeasurementCachewith[]provides good stability, but becausekeyMapperkeys heights byrowIndex, entries persist across full dataset swaps (e.g., a new query result). Rows that are remeasured on next render will overwrite entries, but rows not currently visible retain their cached height, which may not match the new log at that position. Whendatafundamentally changes, callcellMeasurementCache.clearAll()to prevent stale heights from leaking into new rows.♻️ One possible approach
const cellMeasurementCache = React.useMemo( () => new CellMeasurerCache({ fixedWidth: true, minHeight: 1, keyMapper: (rowIndex) => rowIndex, }), [], ); + + // Reset cached row heights when the underlying dataset identity changes + // so stale heights from previous results don't leak into new rows at the same index. + React.useEffect(() => { + cellMeasurementCache.clearAll(); + }, [data, cellMeasurementCache]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/virtualized-logs-table.tsx` around lines 132 - 210, The CellMeasurerCache (cellMeasurementCache) is memoized with [] but keys heights by rowIndex (keyMapper), so when the underlying dataset (data) is replaced stale heights can be reused; fix by invalidating the cache when dataset identity changes: either include data (or a stable dataset id) in the memo dependencies for the CellMeasurerCache creation or add a useEffect that watches data and calls cellMeasurementCache.clearAll() whenever the data reference changes so cached heights are cleared before VirtualTableBody/rowRenderer reuse them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/docker-compose/docker-compose.test.yml`:
- Around line 35-42: Remove the --overwrite flag from the command array in the
test docker-compose service (leave --loop, --format=json, --type=log,
--number=10, --delay=100ms, --output=/var/log/fake.log intact) so flog appends
instead of truncating /var/log/fake.log; this avoids disturbing promtail's
position tracking (positions.yaml) — if disk growth is a concern, implement
external rotation/cleanup instead of using --overwrite.
In `@web/src/components/logs-table.tsx`:
- Around line 301-312: The length-only check for dataChanged can miss updates
when streaming saturates (e.g., STREAMING_MAX_LOGS_LIMIT); change the logic in
the block using prevLogsDataRef, logsData, dataChanged, expandedItems and
setExpandedItems so it also resets expandedItems when the logsData reference
changes or when streaming append occurs: instead of only comparing
result.length, use reference inequality (prevLogsDataRef.current !== logsData)
or detect streaming appends (appendData/streamingResponse cases) to
setExpandedItems(new Set()). Update the comparison around prevLogsDataRef and
dataChanged to trigger a reset whenever logsData is a new object (or when
streaming append is detected) so expanded detail rows don’t point to rotated
entries.
In `@web/src/hooks/useLogs.ts`:
- Around line 272-273: The change made configRef.current read synchronous which
allows getLogs/getMoreLogs/getVolume/getHistogram to run against defaultConfig
before LogsConfigProvider finishes loading; revert to waiting for the real
config by reading via the provider's async loader or by guarding calls: update
the callers (or the hook wrappers around
getLogs/getMoreLogs/getVolume/getHistogram) to await logsContext.fetchConfig()
or check logsContext.configLoaded before using configRef.current, and
specifically ensure logs-alerts-metrics.tsx does not call getLogs without a
configLoaded guard; use the existing LogsConfigProvider symbols (configRef,
fetchConfig, configLoaded, getLogs/getMoreLogs/getVolume/getHistogram) to
implement the wait so queries never execute against defaultConfig.
---
Outside diff comments:
In `@web/src/components/refresh-interval-dropdown.tsx`:
- Around line 49-77: The effect that watches delay causes an immediate onRefresh
on mount when delay is initialized from persisted state; to fix it, add a
mounted/ref guard so the useEffect that calls onRefreshRef.current only triggers
the immediate onRefresh when delay changes after mount (i.e., skip the first
run), and move the render-time assignment onRefreshRef.current = onRefresh into
a React.useLayoutEffect (or useEffectEvent if available) so you don't perform
side effects during render; update the existing symbols delay, timer,
onRefreshRef, the useEffect block, and the
handleSelectedValue/setStoredRefreshInterval flow to ensure selecting a new
interval still triggers an immediate refresh but initial mount does not.
---
Nitpick comments:
In `@web/src/components/logs-table.tsx`:
- Around line 171-218: The TableRowComponent currently reads expansion state
from expandedItemsRef.current which makes expansion UI depend on parent
re-renders; change TableRow so expansion is passed as a real prop (e.g., add
expandedItems:Set<number> or better, an isExpanded:boolean computed per row)
instead of reading expandedItemsRef inside TableRowComponent, update TableRow
signature and the call sites (where RowMemo in virtualized-logs-table.tsx
renders TableRowComponent) to pass the per-row isExpanded and keep
handleRowToggle as the stable callback; this ensures TableRow/RowMemo
correctness even if the RowMemo comparator is tightened or scrolling is true.
In `@web/src/components/refresh-interval-dropdown.tsx`:
- Around line 51-52: The current code assigns onRefreshRef.current = onRefresh
during render which is unsafe for concurrent rendering; update the component to
set onRefreshRef.current inside a lifecycle hook instead (e.g., move the
assignment into a React.useLayoutEffect or React.useEffect) so the ref mutation
happens after commit; locate the onRefreshRef and onRefresh symbols and the
effect that currently omits onRefresh (the effect around useEffect at/near the
former line 77) and ensure the hook updates onRefreshRef.current without adding
onRefresh to unrelated effect dependency arrays.
In `@web/src/components/virtualized-logs-table.tsx`:
- Around line 132-210: The CellMeasurerCache (cellMeasurementCache) is memoized
with [] but keys heights by rowIndex (keyMapper), so when the underlying dataset
(data) is replaced stale heights can be reused; fix by invalidating the cache
when dataset identity changes: either include data (or a stable dataset id) in
the memo dependencies for the CellMeasurerCache creation or add a useEffect that
watches data and calls cellMeasurementCache.clearAll() whenever the data
reference changes so cached heights are cleared before
VirtualTableBody/rowRenderer reuse them.
🪄 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: 88260fd0-54e0-4802-a045-7469735fdb82
📒 Files selected for processing (10)
hack/docker-compose/docker-compose.test.ymlhack/docker-compose/loki/promtail-config.ymlweb/src/components/logs-table.tsxweb/src/components/refresh-interval-dropdown.tsxweb/src/components/virtualized-logs-table.tsxweb/src/hooks/LogsConfigProvider.tsxweb/src/hooks/useLogs.tsweb/src/pages/logs-detail-page.tsxweb/src/pages/logs-dev-page.tsxweb/src/pages/logs-page.tsx
jgbernalp
force-pushed
the
fix-view-rerender
branch
from
2b1836d to
f70a775
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
jgbernalp
force-pushed
the
fix-view-rerender
branch
from
f70a775 to
5b9db3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/pages/logs-page.tsx (1)
104-116:⚠️ Potential issue | 🟠 MajorApply the config-loaded guard to volume queries too.
runQuerynow avoids calling config-backed APIs before config is loaded, butrunVolumecan still be triggered fromLogsToolbarand callgetVolumewith the default/incomplete config.Proposed fix
const runVolume = () => { + if (!configLoaded) return; + getVolume({ query, tenant, timeRange, schema }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/logs-page.tsx` around lines 104 - 116, runVolume currently calls getVolume regardless of configLoaded; add the same configLoaded guard used in runQuery so volume queries don't run before config is ready. Modify the runVolume function to return early when configLoaded is false (like runQuery does), then call getVolume({ query, tenant, timeRange, schema }) only when configLoaded is true; this mirrors the protection already applied for runQuery/getLogs and runQuery/getHistogram.web/src/pages/logs-detail-page.tsx (1)
134-146:⚠️ Potential issue | 🟠 MajorGuard volume queries until config is loaded.
This page now protects logs and histogram requests, but
runVolumecan still execute beforeconfigLoadedand use the default config viagetVolume.Proposed fix
const runVolume = () => { + if (!configLoaded) return; + getVolume({ query, tenant: tenant.current, namespace, timeRange, schema }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/logs-detail-page.tsx` around lines 134 - 146, The runVolume function should guard against running before configuration is ready: add the same configLoaded check used in runQuery so runVolume returns early when configLoaded is false. Update the runVolume implementation (the function that calls getVolume) to only call getVolume({ query, tenant: tenant.current, namespace, timeRange, schema }) when configLoaded is true, mirroring the protection around runQuery and getHistogram to prevent using default config.
🤖 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/components/alerts/logs-alerts-metrics.tsx`:
- Around line 24-32: The effect in the component uses tenant and config.schema
inside its body (when calling getLogs) but they are not listed in the dependency
array, so update the useEffect dependencies to include tenant and config.schema
(or getSchema(config.schema) if you prefer tracking the derived schema)
alongside rule?.query, timeRange, and configLoaded; ensure the symbols mentioned
are used: useEffect, getLogs, rule?.query, timeRange, configLoaded, tenant,
config.schema, and getSchema so the effect re-runs whenever tenant or schema
change.
---
Outside diff comments:
In `@web/src/pages/logs-detail-page.tsx`:
- Around line 134-146: The runVolume function should guard against running
before configuration is ready: add the same configLoaded check used in runQuery
so runVolume returns early when configLoaded is false. Update the runVolume
implementation (the function that calls getVolume) to only call getVolume({
query, tenant: tenant.current, namespace, timeRange, schema }) when configLoaded
is true, mirroring the protection around runQuery and getHistogram to prevent
using default config.
In `@web/src/pages/logs-page.tsx`:
- Around line 104-116: runVolume currently calls getVolume regardless of
configLoaded; add the same configLoaded guard used in runQuery so volume queries
don't run before config is ready. Modify the runVolume function to return early
when configLoaded is false (like runQuery does), then call getVolume({ query,
tenant, timeRange, schema }) only when configLoaded is true; this mirrors the
protection already applied for runQuery/getLogs and runQuery/getHistogram.
🪄 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: 349efc66-7b71-41ca-92a0-bc39a93f6e5c
📒 Files selected for processing (11)
hack/docker-compose/docker-compose.test.ymlhack/docker-compose/loki/promtail-config.ymlweb/src/components/alerts/logs-alerts-metrics.tsxweb/src/components/logs-table.tsxweb/src/components/refresh-interval-dropdown.tsxweb/src/components/virtualized-logs-table.tsxweb/src/hooks/LogsConfigProvider.tsxweb/src/hooks/useLogs.tsweb/src/pages/logs-detail-page.tsxweb/src/pages/logs-dev-page.tsxweb/src/pages/logs-page.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/components/refresh-interval-dropdown.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/docker-compose/docker-compose.test.yml
- hack/docker-compose/loki/promtail-config.yml
- web/src/hooks/useLogs.ts
- web/src/pages/logs-dev-page.tsx
- web/src/components/virtualized-logs-table.tsx
- web/src/hooks/LogsConfigProvider.tsx
- web/src/components/logs-table.tsx
jgbernalp
force-pushed
the
fix-view-rerender
branch
from
5b9db3b to
4247e3d
Compare
…unnecessary re renders Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
fix-view-rerender
branch
from
4247e3d to
859040b
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@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-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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, 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 |
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
@jgbernalp: new pull request created: #364 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: new pull request created: #365 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
This PR:
Summary by CodeRabbit
Bug Fixes
Performance
Chores