Repository navigation
OU-1389: restructure folders - #1038
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1389 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 task 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
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (245)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (117)
WalkthroughThe PR reorganizes backend monitoring packages and frontend modules, adds manifest patching and HTTP/TLS tests, introduces shared console and table primitives, adds incident and alert utilities, and implements Perses dashboard APIs and time-series UI components. ChangesBackend server and monitoring package updates
Web application module restructuring
Feature functionality and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
All coderabbit nitpick's are explicitly out of this PR's scope of work as stated in the description |
|
/test? |
|
/test ? |
|
/test e2e-monitoring e2e-coo |
|
/retest |
|
@PeterYurkovich: The following test failed, say
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. |
openshift-ci
Bot
added
the
needs-rebase
d168459 to
1d089a9
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/features/alerts/pages/alerts-page/agentic-runs/alert-identifier.spec.ts (1)
71-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale describe block name.
The suite is still titled
'matchesProposal'after the function was renamed tomatchesAgenticRun.🔧 Proposed fix
-describe('matchesProposal', () => { +describe('matchesAgenticRun', () => {🤖 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/features/alerts/pages/alerts-page/agentic-runs/alert-identifier.spec.ts` around lines 71 - 99, Rename the describe block from matchesProposal to matchesAgenticRun so the suite title matches the function under test and the existing test cases remain unchanged.
🟡 Minor comments (19)
web/src/shared/console/console-shared/src/components/empty-state/AccessDenied.tsx-15-15 (1)
15-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
titleTextinstead oftitleonEmptyState.titleis ignored here, so the “Restricted access” heading won’t render.🤖 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/shared/console/console-shared/src/components/empty-state/AccessDenied.tsx` at line 15, Update the EmptyState usage in AccessDenied to pass the translated “Restricted access” value through the titleText prop instead of title, ensuring the heading renders correctly.web/src/shared/console/utils/getLastLanguage.ts-1-2 (1)
1-2: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn type should be
string | null
localStorage.getItemcan returnnull, so this helper shouldn’t promise a plainstring.🤖 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/shared/console/utils/getLastLanguage.ts` around lines 1 - 2, Update the getLastLanguage return type to string | null so it accurately reflects localStorage.getItem’s nullable result, while preserving the existing LAST_LANGUAGE_LOCAL_STORAGE_KEY lookup behavior.web/src/shared/store/fetch-alerts.tsx-26-41 (1)
26-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSurface partial external-source failures in
web/src/shared/store/fetch-alerts.tsx:26-43.
Promise.allSettled()filters out rejectedsource.fetch(namespace)calls and still returnsstatus: 'success', sofetchAlertingData()treats a partial external-source failure as a normal load and shows incomplete alert data with no error state. Return the failedsourceIds or a partial-error result so callers can report it.🤖 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/shared/store/fetch-alerts.tsx` around lines 26 - 41, Update fetchAlertingData’s Promise.allSettled processing to retain rejected external-source results and identify their sourceIds, rather than silently filtering them out. Return or propagate a partial-error result alongside any successfully fetched groups so callers can report failures and avoid treating incomplete alert data as a normal success.web/src/shared/console/public/components/factory/text-filter.tsx-12-15 (1)
12-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the missing label case in the fallback placeholder.
When
labelis omitted, this rendersFilter ...instead of a sensible generic placeholder. Fall back to a separate string when no label is provided.🤖 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/shared/console/public/components/factory/text-filter.tsx` around lines 12 - 15, Update the placeholderText fallback in the TextFilter component to use a sensible generic placeholder when label is omitted, while retaining the translated “Filter {{label}}...” text when label is present. Keep the existing explicit placeholder precedence unchanged.web/src/features/perses-dashboards/ols-tool-ui/helpers/useTimeRange.ts-23-39 (1)
23-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParsed
endoffsets are ignored whenstartis absent.
parseTimeExpracceptsNOW-…values, but this branch dropsendDateunlessstartDateis also present. Soend="NOW-1h"with nostartfalls back topastDurationfrom now instead of honoring the requested end anchor.🤖 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/features/perses-dashboards/ols-tool-ui/helpers/useTimeRange.ts` around lines 23 - 39, The useTimeRange hook must preserve a parsed end anchor when start is absent. Update the logic in useTimeRange so valid endDate values such as “NOW-1h” are returned in the resulting time range instead of falling back to pastDuration; retain the existing relative-range behavior for an absent endDate and the absolute range behavior when both dates exist.web/src/features/perses-dashboards/components/project/ProjectMenuToggle.tsx-21-34 (1)
21-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeyboard shortcut ignores
disabled.
onToggle(true)fires onshortCutregardless of thedisabledprop, so a disabled toggle can still be opened via keyboard even though it's visually/click disabled.🐛 Suggested fix
if ( shortCut && + !disabled && event.key === shortCut &&🤖 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/features/perses-dashboards/components/project/ProjectMenuToggle.tsx` around lines 21 - 34, Update handleMenuKeys in ProjectMenuToggle so the keyboard shortcut also checks the disabled prop before calling onToggle(true). Preserve the existing target filtering and event-prevention behavior for enabled toggles.web/src/features/perses-dashboards/components/ToastProvider.tsx-34-40 (1)
34-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTimestamp-based keys can collide.
new Date().getTime().toString()has millisecond resolution; twoaddAlertcalls in the same millisecond produce identical keys. SinceremoveAlertfilters by key equality, colliding keys will dismiss unrelated toasts together and break React's reconciliation (duplicate keys).🐛 Suggested fix: use a collision-resistant key
const addAlert = (title: string, variant: AlertProps['variant']) => { - const key = new Date().getTime().toString(); + const key = crypto.randomUUID(); setAlerts((prevAlerts) => [{ title, variant, key }, ...prevAlerts]); };🤖 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/features/perses-dashboards/components/ToastProvider.tsx` around lines 34 - 40, Replace the millisecond timestamp key generation in ToastProvider’s addAlert with a collision-resistant unique key mechanism. Ensure every alert receives a distinct key so removeAlert only dismisses the intended toast and React list keys remain unique.web/src/shared/components/table/table-pagination.tsx-18-66 (1)
18-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback pagination handlers assume
setPage/setPerPageare defined, but they're optional.All of
setPage,setPerPage,onSetPage,onPerPageSelectare optional inTablePaginationProps. If a consumer supplies onlyitemCount/page/perPage(the only required props) and omits both handler pairs, the defaultonPerPageSelect/onSetPageclosures will callsetPage(...)/setPerPage(...)onundefined, throwing at runtime on the first page/per-page change.🐛 Proposed fix: guard against missing setters
- onPerPageSelect={ - onPerPageSelect - ? onPerPageSelect - : (e, v) => { - // When changing the number of results per page, - // keep the start row approximately the same - const firstRow = (page - 1) * perPage; - setPage(Math.floor(firstRow / v) + 1); - setPerPage(v); - } - } - onSetPage={onSetPage ? onSetPage : (e, v) => setPage(v)} + onPerPageSelect={ + onPerPageSelect + ? onPerPageSelect + : (e, v) => { + // When changing the number of results per page, + // keep the start row approximately the same + const firstRow = (page - 1) * perPage; + setPage?.(Math.floor(firstRow / v) + 1); + setPerPage?.(v); + } + } + onSetPage={onSetPage ? onSetPage : (e, v) => setPage?.(v)}Alternatively, model this with a discriminated/union type so TypeScript enforces that at least one full pair is supplied.
Also applies to: 68-78
🤖 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/shared/components/table/table-pagination.tsx` around lines 18 - 66, Guard the fallback handlers in TablePagination against absent setPage and setPerPage callbacks. In the default onPerPageSelect closure, only invoke setPage and setPerPage when each exists, and in the default onSetPage handler, invoke setPage only when defined; preserve supplied onSetPage and onPerPageSelect callbacks unchanged.web/src/features/alerts/pages/alerts-page/LabelFilter.tsx-25-26 (1)
25-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
enableShortcutis still a no-op.LabelFilter.tsx:25-39documents/focusing the filter, but the prop is never read or wired up here, so callers get no shortcut behavior. Either implement the key handler or remove the prop/doc.🤖 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/features/alerts/pages/alerts-page/LabelFilter.tsx` around lines 25 - 26, The enableShortcut prop in LabelFilter is currently unused despite documenting slash-key focus behavior. Wire enableShortcut into the component’s keyboard shortcut handling so pressing “/” focuses the filter when enabled, or remove the prop and its documentation if shortcut behavior is not supported.web/src/shared/components/query-browser/query-browser.scss-20-54 (1)
20-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing positioning context for absolutely-positioned fade overlays.
:before/:afteruseposition: absolutewithtop/bottom/left/right: 0, but.monitoring-plugin-horizontal-scrollnever setsposition: relative. Without it, the pseudo-elements will anchor to the nearest positioned ancestor instead of this container, breaking the intended edge-fade effect.🔧 Proposed fix
.monitoring-plugin-horizontal-scroll { + position: relative; overflow-x: auto; overflow-y: hidden;🤖 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/shared/components/query-browser/query-browser.scss` around lines 20 - 54, Add position: relative to .monitoring-plugin-horizontal-scroll so its absolutely positioned :before and :after fade overlays anchor to the scroll container.web/src/shared/components/query-browser/query-browser.scss-1-9 (1)
1-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd empty line before declaration (stylelint).
Static analysis flags a missing blank line before the declaration on Line 3.
🔧 Proposed fix
.monitoring-plugin-dashboards__legend-wrap { - $legend-content-height: 75px; + + $legend-content-height: 75px; height: 100%;🤖 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/shared/components/query-browser/query-browser.scss` around lines 1 - 9, Update the .monitoring-plugin-dashboards__legend-wrap rule by adding the required blank line between the $legend-content-height variable declaration and the height declaration, preserving all existing styles.Source: Linters/SAST tools
web/src/shared/console/utils/single-typeahead-dropdown.tsx-338-345 (1)
338-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
selectedItemWidthcan beundefined, producing an invalidcalc()CSS value.When
resizeToFitis true but nothing is selected yet,selectedItemWidthevaluates toundefined(short-circuit ofresizeToFit && selectedValue && ...), which gets interpolated into the width string as literalundefinedpx. Browsers will silently drop the invalid declaration, but it's fragile and unintentional.🐛 Proposed fix
const selectedItemWidth = useMemo(() => { return ( resizeToFit && selectedValue && getTextWidth(String(selectedValue.children), '14px RedHatText') - ); + ) || 0; }, [resizeToFit, selectedValue]);Also applies to: 374-382
🤖 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/shared/console/utils/single-typeahead-dropdown.tsx` around lines 338 - 345, Update the selectedItemWidth useMemo so it always returns a valid numeric fallback when resizeToFit is enabled without a selectedValue, preventing undefined from being interpolated into the width calc. Preserve the existing getTextWidth calculation when selectedValue exists and keep the current behavior when resizeToFit is disabled.web/src/features/incidents/utils/processIncidents.spec.ts-636-645 (1)
636-645: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest doesn't actually exercise the "not provided" default path.
This test passes
nowexplicitly as the second argument, so it never tests the case wheremaxEndTimeis omitted/undefined — it's effectively a duplicate of the earlier explicit-value tests.🐛 Proposed fix
- const result = getIncidentsTimeRanges(timespan, now); + const result = getIncidentsTimeRanges(timespan);🤖 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/features/incidents/utils/processIncidents.spec.ts` around lines 636 - 645, Update the test case around getIncidentsTimeRanges to omit the second, maxEndTime argument when invoking it, so it exercises the default-to-current-time path. Keep the existing before/after time assertions and test name unchanged.web/src/shared/console/utils/single-typeahead-dropdown.tsx-79-89 (1)
79-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
getContext('2d')can returnnull; no guard before use.
canvas.getContext('2d')can returnnullin some environments (e.g. jsdom/test runners without canvas support), which would throw oncontext.font = font. This only triggers whenresizeToFitis used, but it's a straightforward crash surface.🛡️ Proposed guard
const context = canvas.getContext('2d'); + if (!context) { + return 0; + } context.font = font;🤖 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/shared/console/utils/single-typeahead-dropdown.tsx` around lines 79 - 89, Update getTextWidth to handle a null result from canvas.getContext('2d') before accessing context.font or measuring text; return an appropriate fallback width when no 2D context is available, while preserving the existing measurement behavior when the context exists.pkg/server/server_test.go-272-272 (1)
272-272: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing context on HTTP request construction (noctx).
http.NewRequest(Line 272),httpClient.Get(Line 297), andhttptest.NewRequest(Lines 654, 666) should use the*WithContextvariants. As per path instructions, Go code should use "context.Context for cancellation and timeouts."Also applies to: 297-297, 654-654, 666-666
🤖 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 `@pkg/server/server_test.go` at line 272, Update the HTTP request construction in the affected tests to use context-aware APIs: replace http.NewRequest and httptest.NewRequest with their WithContext variants, and replace the httpClient.Get call with an explicitly constructed context-bound request. Use the appropriate existing test context, preserving each request’s method, URL, and body.Source: Path instructions
web/src/shared/components/table/TableCheckboxFilter.tsx-39-41 (1)
39-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
containerRefis declared but never attached to a DOM node, soappendTonever resolves.
containerRef(Line 41) is only ever read at Line 143 (appendTo={containerRef.current || undefined}); it's never passed as arefto any rendered element, socontainerRef.currentis alwaysnullandappendToalways falls back toundefined. Compare withTableFilters.tsx, where the equivalentattributeContainerRefis attached via a wrapping<div ref={attributeContainerRef}>.🐛 Proposed fix: wrap the Popper in a container div
return ( <ToolbarFilter key={ouiaId} data-ouia-component-id={ouiaId} ... > - <Popper - trigger={...} - triggerRef={toggleRef} - popper={...} - popperRef={menuRef} - /*eslint-disable-next-line react-hooks/refs */ - appendTo={containerRef.current || undefined} - aria-label={`${title ?? filterId} filter`} - isVisible={isOpen} - /> + <div ref={containerRef}> + <Popper + trigger={...} + triggerRef={toggleRef} + popper={...} + popperRef={menuRef} + /*eslint-disable-next-line react-hooks/refs */ + appendTo={containerRef.current || undefined} + aria-label={`${title ?? filterId} filter`} + isVisible={isOpen} + /> + </div> </ToolbarFilter> );Also applies to: 96-146
🤖 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/shared/components/table/TableCheckboxFilter.tsx` around lines 39 - 41, Attach containerRef to a rendered wrapper around the Popper in TableCheckboxFilter, ensuring the element encompasses the relevant menu content so appendTo={containerRef.current || undefined} resolves to that container. Follow the wrapping pattern used by attributeContainerRef in TableFilters.tsx and preserve the existing Popper behavior.pkg/server/server_test.go-92-92 (1)
92-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnchecked error returns flagged by errcheck.
server.Shutdown(Line 92),os.RemoveAll(Lines 115, 167, 195),l.Close(Line 253),resp.Body.Close(Line 282), andr.Body.Close(Line 298) all have unchecked error returns. As per path instructions, Go code must "Never ignore error returns."🛠️ Example fix pattern (apply similarly at each site)
- defer os.RemoveAll(tmpDir) + defer func() { + if err := os.RemoveAll(tmpDir); err != nil { + t.Logf("failed to remove temp dir: %v", err) + } + }()As per path instructions, "Never ignore error returns."
Also applies to: 115-115, 167-167, 195-195, 253-253, 282-282, 298-298
🤖 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 `@pkg/server/server_test.go` at line 92, Handle every unchecked error in server_test.go: server.Shutdown, each os.RemoveAll call, l.Close, resp.Body.Close, and r.Body.Close. Update the relevant test cleanup paths to explicitly check or assert each returned error using the repository’s existing test error-handling conventions, without changing the cleanup behavior.Source: Path instructions
pkg/server/server_test.go-74-99 (1)
74-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPotential "fail in goroutine after test completed" flakiness.
The background goroutine calls
t.ErrorfifStartHTTPServerreturns an unexpected error, butcleanup()only callsShutdown+cancel()+ a fixed 100ms sleep, without explicitly waiting for the goroutine to finish. If the goroutine's post-Shutdowncheck runs after the test has already completed, this can panic the test binary.Consider using a
sync.WaitGroup(or a done channel) incleanup()to deterministically wait for the goroutine before returning.🤖 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 `@pkg/server/server_test.go` around lines 74 - 99, Update startTestServer to track the StartHTTPServer goroutine with a sync.WaitGroup or done channel, and have cleanup wait for that goroutine after initiating Shutdown and cancellation. Preserve the existing unexpected-error reporting while ensuring cleanup does not return until the server goroutine has finished.pkg/server/server_test.go-306-341 (1)
306-341: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
buildHTTPClientmutates the caller'sTLSConfig/HTTPTransportinstead of copying them.The comments say "Make our own copy," but
tlsConfig := conf.TLSConfigandtransport := conf.HTTPTransportare pointer assignments, not copies, when non-nil. Subsequent mutations (tlsConfig.Certificates = append(...),transport.TLSClientConfig = tlsConfig) modify the caller's original struct. Harmless in current tests only because fresh literals are passed each time, but it's a latent bug if a config is ever reused across calls.🛠️ Proposed fix: shallow-copy before mutating
- tlsConfig := &tls.Config{} - if conf.TLSConfig != nil { - tlsConfig = conf.TLSConfig - } + tlsConfig := &tls.Config{} + if conf.TLSConfig != nil { + *tlsConfig = *conf.TLSConfig + } ... - transport := &http.Transport{} - if conf.HTTPTransport != nil { - transport = conf.HTTPTransport - } + transport := &http.Transport{} + if conf.HTTPTransport != nil { + *transport = *conf.HTTPTransport + }🤖 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 `@pkg/server/server_test.go` around lines 306 - 341, Update buildHTTPClient to shallow-copy non-nil conf.TLSConfig and conf.HTTPTransport values before modifying them, while retaining fresh defaults for nil inputs. Ensure certificate appends and transport TLSClientConfig/timeout assignments affect only the copied structs, not caller-owned configuration.
🧹 Nitpick comments (20)
web/src/shared/hooks/useMonitoring.ts (1)
4-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify: pass through context directly.
Destructuring and immediately rebuilding an identical object is redundant since
MonitoringContext's value already matches this shape.♻️ Simplification
-export const useMonitoring = () => { - const { - prometheus, - plugin, - useAlertsTenancy, - useMetricsTenancy, - accessCheckLoading, - displayNamespaceSelector, - } = useContext(MonitoringContext); - return { - prometheus, - plugin, - useAlertsTenancy, - useMetricsTenancy, - accessCheckLoading, - displayNamespaceSelector, - }; -}; +export const useMonitoring = () => useContext(MonitoringContext);🤖 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/shared/hooks/useMonitoring.ts` around lines 4 - 21, Update useMonitoring to return the MonitoringContext value directly instead of destructuring and reconstructing the same object. Preserve the existing context shape and all exposed properties.web/src/shared/console/console-shared/error/fallbacks/withFallback.tsx (2)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
anyused forFallbackComponentprop type.As per coding guidelines, "Avoid
anyin TypeScript; prefer specific response or domain types instead." This is already suppressed viaeslint-disable, but a more specific type (e.g.,ComponentType<ErrorBoundaryFallbackProps>, matchingErrorBoundaryProps.FallbackComponent) would avoid the exception entirely.🤖 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/shared/console/console-shared/error/fallbacks/withFallback.tsx` around lines 4 - 8, Replace the any-based FallbackComponent type in WithFallback with the specific fallback props type used by ErrorBoundaryProps.FallbackComponent, such as ErrorBoundaryFallbackProps. Remove the eslint-disable suppression while preserving the existing generic component and return types.Source: Coding guidelines
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefault export instead of named export.
As per coding guidelines for
web/src/**/*.{ts,tsx}, "Define React components as functional components with explicit type annotations... and use named exports." This module usesexport default withFallback.🤖 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/shared/console/console-shared/error/fallbacks/withFallback.tsx` at line 20, Change the withFallback export from a default export to a named export, preserving the existing component implementation and its explicit functional component type annotation. Update any imports that consume this symbol to use the named export form.Source: Coding guidelines
web/src/shared/console/utils/getLastLanguage.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFilename should be kebab-case per path instructions.
getLastLanguage.tsis a plain utility file (not a hook or component); path instructions require kebab-case utility filenames.As per path instructions, "Use kebab-case for utility filenames, such as
safe-fetch-hook.tsandpoll-hook.ts."🤖 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/shared/console/utils/getLastLanguage.ts` around lines 1 - 2, Rename the utility file containing getLastLanguage from getLastLanguage.ts to kebab-case get-last-language.ts, preserving its exported function and implementation unchanged.Source: Path instructions
web/src/shared/console/utils/async.tsx (2)
85-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProp/state types don't match actual usage.
Component: ComponentType(Line 91) excludesnull, but is assignednullat Lines 17 and 26.LoadingComponent?: ReactNode(Line 87) is rendered as<LoadingComponent />(Line 81), which requires a component type, not aReactNode— the defaultLoadingBoxis a component reference, not a renderable node.♻️ Proposed fix
type AsyncComponentProps = { loader: () => Promise<ComponentType>; - LoadingComponent?: ReactNode; + LoadingComponent?: ComponentType; // eslint-disable-next-line `@typescript-eslint/no-explicit-any` } & any; type AsyncComponentState = { - Component: ComponentType; + Component: ComponentType | null; loader: () => Promise<ComponentType>; };🤖 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/shared/console/utils/async.tsx` around lines 85 - 93, Align the async component types with their runtime usage: update AsyncComponentState.Component to allow null for the initial and loading states, and type AsyncComponentProps.LoadingComponent as a component type that can be invoked with JSX. Preserve the existing default LoadingBox component reference and rendering behavior.
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
& anycollapsesAsyncComponentPropstoany.Intersecting with
any(Line 89) defeats typechecking on the whole props shape, not just the rest-props passthrough. Consider bounding toRecord<string, unknown>or a generic parameter instead.🤖 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/shared/console/utils/async.tsx` around lines 85 - 89, Update AsyncComponentProps to remove the intersection with any, preserving type checking for the required loader and optional LoadingComponent props. Use a bounded rest-props shape such as Record<string, unknown>, or introduce a generic parameter if needed to support additional component props without collapsing the entire type to any.web/src/features/incidents/assets/incidents-styles.css (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.pf-m-hiddenmisuses PatternFly's modifier namespace for a generic utility.PatternFly reserves the
pf-m-prefix for component-scoped BEM modifiers (e.g..pf-v6-c-alert.pf-m-danger); generic utilities use thepf-v6-u-prefix (PatternFly already ships.pf-v6-u-display-none). Naming a block-agnostic hide-utilitypf-m-hiddenrisks confusion with, or accidental collision against, real component modifiers.🤖 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/features/incidents/assets/incidents-styles.css` around lines 33 - 35, The generic hidden utility uses PatternFly’s modifier namespace; rename `.pf-m-hidden` to the existing `pf-v6-u-display-none` utility and update any references to the old selector while preserving its `display: none` behavior.web/src/shared/console/utils/ref-width-hook.ts (1)
7-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
setRefparameter type doesn't reflect nullable ref callback semantics.React invokes ref callbacks with
T | nullon unmount, butsetRefis typed as(e: HTMLDivElement) => void. The body already defends against this withe?.clientWidthandref.current = e, but the type mismatch is only hidden by theas [Ref<HTMLDivElement>, number]cast at line 32, which suppresses the type checker rather than fixing the signature.🛠️ Proposed fix
- const setRef = useCallback((e: HTMLDivElement) => { + const setRef = useCallback((e: HTMLDivElement | null) => { const newWidth = e?.clientWidth; if (newWidth && ref.current?.clientWidth !== newWidth) { setWidth(e.clientWidth); } ref.current = e; }, []);Also applies to: 32-32
🤖 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/shared/console/utils/ref-width-hook.ts` around lines 7 - 13, Update the setRef callback parameter type to accept HTMLDivElement or null, matching React ref callback semantics and the existing nullable-safe body. Remove the Ref<HTMLDivElement> cast in the hook’s returned tuple so TypeScript validates the corrected callback signature directly.web/src/features/perses-dashboards/components/ToastProvider.tsx (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
import typefor type-only symbols.Several new files import symbols that are used only in type positions via regular (value) imports, violating the guideline to use
typeimports for type-only symbols.
web/src/features/perses-dashboards/components/ToastProvider.tsx#L1-L9:ReactNode,FC(fromreact) andAlertProps(from@patternfly/react-core) are only used as types (FC<{...}>,AlertProps['variant']) — import them viaimport type.web/src/shared/components/query-browser/query-browser-theme.ts#L1-L2:ChartThemeDefinitionis only used to annotateconst theme: ChartThemeDefinition— import it viaimport type.web/src/shared/components/table/useTablePagination.ts#L1-L6:UseDataViewPaginationProps,KeyboardEvent, andMouseEvent as ReactMouseEventare only used as type annotations — import them viaimport type.As per path instructions, "Use
typeimports for symbols that are only used for type checking."🤖 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/features/perses-dashboards/components/ToastProvider.tsx` around lines 1 - 9, Convert the type-only imports to `import type`: in web/src/features/perses-dashboards/components/ToastProvider.tsx lines 1-9, move ReactNode, FC, and AlertProps out of the value imports; in web/src/shared/components/query-browser/query-browser-theme.ts lines 1-2, import ChartThemeDefinition as a type; and in web/src/shared/components/table/useTablePagination.ts lines 1-6, import UseDataViewPaginationProps, KeyboardEvent, and ReactMouseEvent as types. Keep runtime imports separate and unchanged.Source: Path instructions
web/src/features/alerts/pages/alerts-page/agentic-runs/alert-identifier.ts (1)
18-42: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse a single
TextEncoderinstance.A new
TextEncoder()is instantiated twice per label (name and value) inside the loop. Hoisting a single encoder outside the loop avoids repeated allocation.♻️ Proposed fix
export const computeAlertFingerprint = (labels: Record<string, string>): string => { const names = Object.keys(labels).sort(); + const encoder = new TextEncoder(); let hash = FNV_OFFSET_BASIS; for (const name of names) { const value = labels[name]; - const bytes = new TextEncoder().encode(name); + const bytes = encoder.encode(name); for (const b of bytes) { hash ^= BigInt(b); hash = (hash * FNV_PRIME) & UINT64_MASK; } hash ^= BigInt(SEPARATOR_BYTE); hash = (hash * FNV_PRIME) & UINT64_MASK; - const valueBytes = new TextEncoder().encode(value); + const valueBytes = encoder.encode(value);🤖 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/features/alerts/pages/alerts-page/agentic-runs/alert-identifier.ts` around lines 18 - 42, Update computeAlertFingerprint to instantiate one TextEncoder before iterating over labels, then reuse it for encoding both each label name and value instead of constructing encoders inside the loop.web/src/shared/console/console-shared/error/error-boundary.tsx (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the constructor
propsparameter.As per coding guidelines, "Avoid
anyin TypeScript; prefer specific response or domain types instead."constructor(props)is currently untyped.- constructor(props) { + constructor(props: ErrorBoundaryInnerProps) { super(props); this.state = this.defaultState; }🤖 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/shared/console/console-shared/error/error-boundary.tsx` around lines 37 - 40, Type the props parameter in the error boundary class constructor using the component’s specific props type, while preserving the existing super call and defaultState initialization.Source: Coding guidelines
web/src/shared/hooks/useIsVisible.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
refparameter instead of leaving it implicitany.As per coding guidelines, "Avoid
anyin TypeScript; prefer specific response or domain types instead."refcurrently has no annotation.♻️ Proposed fix
-import { useState, useEffect } from 'react'; +import { useState, useEffect, RefObject } from 'react'; -export const useIsVisible = (ref) => { +export const useIsVisible = (ref: RefObject<Element | null>) => {🤖 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/shared/hooks/useIsVisible.ts` at line 3, Annotate the ref parameter in useIsVisible with the appropriate specific ref type instead of leaving it implicit any, matching the value the hook observes and the existing project typing conventions.Source: Coding guidelines
web/src/features/perses-dashboards/utils/perses/datasource-client.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
DatasourceResource.Only used as a return-type annotation (line 43).
As per coding guidelines, "Use
typeimports for symbols that are only used for type checking."🤖 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/features/perses-dashboards/utils/perses/datasource-client.ts` at line 14, Update the DatasourceResource import in datasource-client.ts to a type-only import, since it is only used in the return-type annotation and has no runtime usage.Source: Coding guidelines
web/src/features/perses-dashboards/utils/datasource-api.ts (2)
16-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJSDoc documents a dashboard-scoped URL shape that the implementation always rejects.
The comment lists
/proxy/projects/{project}/dashboards/{dashboard}/{name}as a possible output, but the code throws unconditionally wheneverdashboardis truthy, so that shape can never actually be produced. Update the doc to reflect that dashboard-level datasources are unsupported and always throw.🤖 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/features/perses-dashboards/utils/datasource-api.ts` around lines 16 - 45, Update the JSDoc for buildProxyUrl to remove the dashboard-scoped URL from the documented outputs and explicitly state that providing dashboard throws because dashboard-level datasources are unsupported. Keep the existing global and project URL documentation unchanged.
29-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
'datasources'/'globaldatasources'duplicate existing exported constants.
resourceis already exported from./perses/datasource-client.ts(imported one line above) with the same value'datasources'; the global equivalent could similarly be exported from./perses/global-datasource-client.ts. Restating the literals here risks silent drift if either segment name changes.♻️ Proposed fix
-import { fetchDatasourceList } from './perses/datasource-client'; -import { fetchGlobalDatasourceList } from './perses/global-datasource-client'; +import { fetchDatasourceList, resource as datasourceResource } from './perses/datasource-client'; +import { + fetchGlobalDatasourceList, + globalDatasourceResource, +} from './perses/global-datasource-client'; ... - let url = `${!project && !dashboard ? 'globaldatasources' : 'datasources'}/${encodeURIComponent( + let url = `${!project && !dashboard ? globalDatasourceResource : datasourceResource}/${encodeURIComponent( name, )}`;(requires exporting
globalDatasourceResourcefromglobal-datasource-client.ts)🤖 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/features/perses-dashboards/utils/datasource-api.ts` around lines 29 - 50, Update buildProxyUrl to reuse the imported datasource resource constant instead of hardcoding 'datasources', and export/import a corresponding global datasource resource constant from global-datasource-client.ts for the global path. Preserve the existing project and dashboard handling while eliminating duplicated resource literals.web/src/features/perses-dashboards/utils/perses/global-datasource-client.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
GlobalDatasourceResource.It's used only as a type annotation (line 25), not as a runtime value.
As per coding guidelines, "Use `type` imports for symbols that are only used for type checking."♻️ Proposed fix
-import { GlobalDatasourceResource } from '`@perses-dev/core`'; +import type { GlobalDatasourceResource } from '`@perses-dev/core`';🤖 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/features/perses-dashboards/utils/perses/global-datasource-client.ts` at line 14, Update the GlobalDatasourceResource import to be type-only, since it is used only as a type annotation in this module and not at runtime.Source: Coding guidelines
web/src/features/perses-dashboards/utils/perses/datasource-cache-api.ts (1)
191-217: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
addCsrfTokenmutates the cached datasource object in place.
setDatasource/setGlobalDatasourcestore the exact object reference later passed intoaddCsrfToken, which reassignsdatasource.spec.plugin.specdirectly on that shared reference rather than returning a copy. Any other holder of the previously-returned object will observe this in-place change, and repeated cache hits keep re-mutating the same object.♻️ Proposed fix
const addCsrfToken = <T extends DatasourceResource | GlobalDatasourceResource | undefined>( datasource: T, ): T => { if (!datasource?.spec?.plugin?.spec) { return datasource; } ... - datasource.spec.plugin.spec = { - ...pluginSpec, - proxy: { spec: { ...proxySpec, headers: { ...existingHeaders, 'X-CSRFToken': getCSRFToken(), 'Sec-Fetch-Site': 'same-origin' } } }, - }; - return datasource; + return { + ...datasource, + spec: { + ...datasource.spec, + plugin: { + ...datasource.spec.plugin, + spec: { + ...pluginSpec, + proxy: { spec: { ...proxySpec, headers: { ...existingHeaders, 'X-CSRFToken': getCSRFToken(), 'Sec-Fetch-Site': 'same-origin' } } }, + }, + }, + }, + }; };🤖 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/features/perses-dashboards/utils/perses/datasource-cache-api.ts` around lines 191 - 217, Update addCsrfToken to create and return a copied datasource structure instead of assigning to datasource.spec.plugin.spec on the input object. Preserve the existing plugin, proxy, and headers values while applying the CSRF headers, and ensure setDatasource/setGlobalDatasource continue storing the returned copy without mutating previously cached references.web/src/features/perses-dashboards/utils/dashboard-api.ts (1)
126-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
createPersesProjectduplicates URL-building logic instead of reusingbuildURL.Every other function in this file (
updateDashboard,createDashboard,deleteDashboard,getDashboards) builds its URL via the sharedbuildURLhelper; this one manually concatenatesPERSES_PROXY_BASE_PATHwith a hardcoded'/api/v1/projects', duplicatingbuildURL'sbasePath+apiPrefixlogic. If either constant changes, this call site can silently drift out of sync.♻️ Proposed fix
export const createPersesProject = async (projectName: string): Promise<ProjectResource> => { - const createProjectURL = '/api/v1/projects'; - const persesURL = `${PERSES_PROXY_BASE_PATH}${createProjectURL}`; + const persesURL = buildURL({ resource: 'projects' });🤖 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/features/perses-dashboards/utils/dashboard-api.ts` around lines 126 - 144, Update createPersesProject to build its endpoint with the shared buildURL helper, using the projects API path or corresponding API identifiers already used by the other dashboard functions. Remove the manual createProjectURL and PERSES_PROXY_BASE_PATH concatenation while preserving the existing POST payload and return behavior.web/src/shared/hooks/useFeatures.ts (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType name should be PascalCase.
features(andFeaturesResponse's sibling) should follow PascalCase for type aliases, e.g.Features. Currently it also shadows thefeaturesstate variable name declared later (line 27), which is confusing.As per path instructions, "Use PascalCase for type aliases and interface names... such as MonitoringResource, TimeRange...".
♻️ Proposed rename
-type features = { +type Features = { 'acm-alerting': boolean; 'perses-dashboards': boolean; incidents: boolean; };-const noFeatures: features = { +const noFeatures: Features = {- const [features, setFeatures] = useState<features>(noFeatures); + const [features, setFeatures] = useState<Features>(noFeatures);🤖 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/shared/hooks/useFeatures.ts` around lines 4 - 8, Rename the `features` type alias to `Features` and update all references to use the new PascalCase name, including any related `FeaturesResponse` type usage. Keep the later `features` state variable unchanged so the type and state identifiers no longer conflict.Source: Path instructions
pkg/server/server_test.go (1)
37-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestCreateHTTPServernever exercises the success path.Both table entries set
err: true; there's no case with a valid TLS min/max combo assertingrequire.NoError, so the "succeeds otherwise" behavior is untested.♻️ Suggested addition
{ cfg: &Config{ TLSMinVersion: tls.VersionTLS13, TLSMaxVersion: tls.VersionTLS12, CertFile: "/etc/tls/server.crt", PrivateKeyFile: "/etc/tls/server.key", }, err: true, }, + { + cfg: &Config{ + TLSMinVersion: tls.VersionTLS12, + TLSMaxVersion: tls.VersionTLS13, + CertFile: "/etc/tls/server.crt", + PrivateKeyFile: "/etc/tls/server.key", + }, + err: false, + },🤖 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 `@pkg/server/server_test.go` around lines 37 - 72, Add a valid TLS configuration case to TestCreateHTTPServer with compatible minimum and maximum TLS versions and err set to false, so the existing require.NoError assertion exercises the successful createHTTPServer path while preserving the current invalid-configuration cases.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (11)
web/src/features/incidents/utils/utils.spec.ts (1)
91-126: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: inspect insertPaddingPointsForChart implementation for sorting behavior. rg -n -A 40 'export const insertPaddingPointsForChart' web/src/features/incidents/utils/utils.tsRepository: openshift/monitoring-plugin
Length of output: 165
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the implementation and the related spec. git ls-files 'web/src/features/incidents/**' | sed -n '1,200p' printf '\n---\n' rg -n 'insertPaddingPointsForChart|padding point|gap > 5 minutes|threshold \+ 1 second' web/src/features/incidents -SRepository: openshift/monitoring-plugin
Length of output: 6185
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'utils.ts lines: ' wc -l web/src/features/incidents/utils/utils.ts printf 'utils.spec.ts lines: ' wc -l web/src/features/incidents/utils/utils.spec.ts printf '\n--- utils.ts (90-190) ---\n' sed -n '90,190p' web/src/features/incidents/utils/utils.ts printf '\n--- utils.spec.ts (80-140) ---\n' sed -n '80,140p' web/src/features/incidents/utils/utils.spec.tsRepository: openshift/monitoring-plugin
Length of output: 7623
Padding points can be emitted out of order for gaps under 10 minutes. For gaps between 302s and 599s,
insertPaddingPointsForChartinserts the current point’s+300spadding before the next point’s-300spadding, producing non-monotonic timestamps (for example,1300then1200). Any chart consumer assuming ascending x-values will render this incorrectly; the padding points need to be ordered or sorted before returning.🤖 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/features/incidents/utils/utils.spec.ts` around lines 91 - 126, Update insertPaddingPointsForChart so padding points for gaps between 302 and 599 seconds are returned in ascending timestamp order, preventing the current point’s +300-second padding from preceding the next point’s -300-second padding. Preserve the existing padding values and behavior, and sort or emit the generated points chronologically before returning.web/src/features/perses-dashboards/utils/perses/datasource-cache-api.ts (1)
56-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cache key generation risks collisions across different
(kind, name, project)tuples, and the negative-lookup cache is shared across the local/global domains.
generateKeyconcatenateskind,name, andprojectwith a bare-and no escaping, so two structurally different selectors (e.g.kind:"a", name:"b-c"vskind:"a-b", name:"c") can collide on the same key. On top of that,emptyDatasourcesis a single cache shared by both the project-scopedgetDatasource/setUndefinedDatasourcepath and the globalgetGlobalDatasource/setUndefinedGlobalDatasourcepath, with no prefix distinguishing the two domains — a colliding key from either domain could cause a false "not found" (or false cache hit) for the other domain's lookup, serving/denying the wrong datasource.🐛 Proposed fix
private generateKey(selector: DatasourceSelector, project?: string): string { - let key = selector.kind; - if (selector.name !== undefined) { - key += `-${selector.name}`; - } - if (project !== undefined) { - key += `-${project}`; - } - return key; + return JSON.stringify([project ?? null, selector.kind, selector.name ?? null]); }Optionally also namespace
emptyDatasourceskeys per domain (e.g. prefixlocal:vsglobal:) so a local and global negative lookup can never collide even after this change.Also applies to: 86-88, 100-109
🤖 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/features/perses-dashboards/utils/perses/datasource-cache-api.ts` around lines 56 - 58, Update generateKey to produce an unambiguous key for the complete kind, name, and project tuple, avoiding bare delimiter concatenation collisions. Namespace emptyDatasources entries by lookup domain, using distinct local and global prefixes in getDatasource/setUndefinedDatasource and getGlobalDatasource/setUndefinedGlobalDatasource, so negative-cache entries cannot cross domains.web/src/shared/components/table/TableFilters.tsx (1)
118-122: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'ts-loader|babel-loader|swc-loader|`@babel/preset-typescript`' web/webpack.config.ts web/package.json 2>/dev/nullRepository: openshift/monitoring-plugin
Length of output: 268
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- webpack config excerpt ---' sed -n '120,200p' web/webpack.config.ts echo echo '--- search for SWC/TypeScript enum options ---' rg -n 'constEnum|constEnums|treatConstEnumAsEnum|swc-loader|jsc\.parser|typescript' web -g '!**/node_modules/**'Repository: openshift/monitoring-plugin
Length of output: 28962
🌐 Web query:
SWC const enum cross-file imported exported const enum transpile behavior swc-loader💡 Result:
SWC operates on a per-file basis and does not perform cross-file analysis [1][2]. Consequently, it cannot perform cross-file inlining for const enums [3][4]. When you use const enums in SWC, the following behavior applies: 1. Single-file Inlining: SWC successfully inlines const enum members when they are used within the same file where they are defined [3]. 2. Cross-file Limitation: If a const enum is defined in one file and imported into another, SWC cannot inline the value because it lacks the necessary cross-file context [3][4]. Unlike the TypeScript compiler (tsc), which can track these references across project files, SWC processes files in isolation [1][2]. 3. Runtime Error Risk: Because const enums are intended to be replaced by their values at compile time and disappear from the runtime output, importing them across files in an SWC-transpiled environment can lead to runtime errors (e.g., "reference not found" or "undefined") if the runtime expects an object that was never emitted [5][4]. Recommendations for swc-loader usage: - Enable isolatedModules: Always set "isolatedModules": true in your tsconfig.json when using SWC [6][5]. This forces TypeScript to flag cross-file const enum references as errors, preventing you from encountering unexpected runtime issues [5]. - Avoid Const Enums for Cross-file Export: If you need to share enums across files, use standard TypeScript enums instead of const enums [2]. Standard enums will be transpiled into objects that exist at runtime, allowing them to be safely imported and used across multiple files [4]. - Bundler-level Inlining: If you absolutely require const enum inlining across files, you may need to use a bundler that performs its own semantic analysis or cross-module optimization (such as esbuild) rather than relying solely on SWC's file-by-file transpilation [7].
Citations:
- 1: swc-project/swc#1584
- 2: swc-project/swc#486
- 3: swc-project/swc#940
- 4: aspect-build/rules_ts#398
- 5: swc-project/swc#7124
- 6: https://stackoverflow.com/questions/75274551/cant-import-enum-unless-i-made-it-const-enum-and-set-isolatedmodules-to-false
- 7: swc-project/swc#8114
🌐 Web query:
SWC constEnums option exported const enum imported from another file docs💡 Result:
SWC does not currently support the cross-file inlining of const enum members [1][2][3]. Because SWC operates on a file-by-file basis, it lacks the necessary type-checking and global context to resolve and inline values from
const enumdefinitions imported from other files [4][3][5]. When you use aconst enumexported from another file: 1. SWC cannot inline the constant value at the usage site because it does not have visibility into the imported module's constant values during the individual file's transpilation process [1][3][5]. 2. If left as is, this can result in runtime errors (e.g., "ReferenceError: [EnumName] is not defined") because theconst enumobject does not exist at runtime [6][5]. 3. Unlike the TypeScript compiler (tsc), which can perform cross-file analysis to resolve these values, SWC will generally preserve the import or attempt to treat it as a runtime object, which fails forconst enum[1][5]. It is highly recommended to follow the TypeScriptisolatedModulesbest practices when using SWC [6]. UnderisolatedModules: true, TypeScript will actually throw an error if you try to reference an ambientconst enummember because it recognizes that such transpilers cannot safely resolve the value [6]. If you encounter this issue, common workarounds include: - Changing theconst enumto a standardenumif runtime object existence is acceptable [3]. - Replacing theconst enumwith a standardexport constobject withas constassertions, which are safely preserved and transpiled by SWC. Note: There is no native SWC configuration option (such as a hypotheticalconstEnumsflag for the core compiler) that enables cross-file inlining forconst enum[2][3]. Some third-party tools, likegraphql-codegen, have aconstEnumsoption, but this is specific to code generation for those specific libraries and does not affect how SWC handles existing TypeScript files [7][8][9].Citations:
- 1: swc-project/swc#940
- 2: swc-project/swc#1584
- 3: swc-project/swc#486
- 4: https://swc.rs/docs/migrating-from-tsc
- 5: aspect-build/rules_ts#398
- 6: swc-project/swc#7124
- 7: https://the-guild.dev/graphql/codegen/plugins/typescript/typescript
- 8: dotansimha/graphql-code-generator#3296
- 9: https://app.unpkg.com/@graphql-codegen/typescript@1.15.2/files/config.d.ts
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,240p' web/webpack.config.ts | nl -ba | sed -n '130,190p'Repository: openshift/monitoring-plugin
Length of output: 205
🌐 Web query:
site:swc.rs const enum swc transpile cross file💡 Result:
SWC does not support cross-file transpilation for TypeScript const enums [1]. Because SWC operates on a file-by-file basis, it lacks the necessary visibility across multiple files to resolve const enum member values that are defined in a different module [1]. In TypeScript, const enums are intended to be inlined at compile time; without full-project context, SWC cannot perform this inlining for imports, which can lead to runtime errors when the code expects the value to be present [1]. To manage this limitation: 1. SWC recommends using the isolatedModules flag in your TypeScript configuration, which will warn you about code patterns—such as cross-file const enum usage—that may not be correctly interpreted during a file-by-file transformation [1]. 2. If you require full support for const enums across modules, it is recommended to continue using the official TypeScript compiler (tsc) for those specific parts of your build process or for type checking, as SWC is designed strictly as a transpiler and does not perform type checking [1].
Citations:
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- tsconfig files mentioning const enum / isolatedModules / preserveConstEnums ---' rg -n 'isolatedModules|preserveConstEnums|constEnum|noEmit' web/tsconfig*.json web/**/*.json echo echo '--- TableFilterOption definition and import sites ---' rg -n 'TableFilterOption' web/src echo echo '--- surrounding code in TableFilters.tsx ---' sed -n '1,180p' web/src/shared/components/table/TableFilters.tsxRepository: openshift/monitoring-plugin
Length of output: 249
🌐 Web query:
esbuild const enum cross-file imported exported TypeScript behavior💡 Result:
In esbuild, the behavior regarding TypeScript
const enumis fundamentally constrained by its design principle of performing isolated module transpilation [1][2]. Because esbuild does not implement a full TypeScript type checker, it cannot reliably perform cross-file analysis to track, resolve, and inlineconst enummembers defined in other modules [3][4][2]. Key aspects of this behavior include: 1. Inlining Limitations: esbuild supports inliningconst enumvalues only when they are defined and used within the same file [3][4][5]. It does not support inlining these values across file boundaries, even when bundling [6][3][4]. When you import aconst enumfrom another file, esbuild treats it as a standard runtime object reference [3]. 2. Runtime Risks with 'declare': Usingdeclare const enumis generally problematic with esbuild [7]. Becausedeclaresignals that the enum exists only as a type annotation, esbuild does not emit any JavaScript code for it [7]. If you reference it at runtime, your code will fail with aReferenceErrorbecause the identifier will not exist [7]. This is considered an unsupported pattern in esbuild, and it is recommended to avoid usingdeclarewithconst enum[7]. 3. Consistency with Isolated Modules: This behavior is consistent with running the official TypeScript compiler (tsc) with theisolatedModules: trueflag, as well as with other transpilers like Babel [1][2]. esbuild enforces this limitation to maintain its performance and avoid the complexity of replicating TypeScript's type-aware compilation process [8][7][2]. To ensure your code remains compatible with esbuild, it is recommended to avoiddeclare const enumentirely and, if cross-module usage is required, use standardenumor plain objects [7]. If you must useconst enum, be aware that its values will not be inlined when imported from other files [3][4].Citations:
- 1: evanw/esbuild#4216
- 2: evanw/esbuild#3359
- 3: evanw/esbuild#534
- 4: evanw/esbuild#128
- 5: evanw/esbuild#3416
- 6: evanw/esbuild#3425
- 7: evanw/esbuild#4394
- 8: evanw/esbuild#2298
Replace the exported
const enumhere — this file is transpiled by SWC in development and esbuild in production, so cross-fileconst enumimports can break. Use a regularenumor anas constobject instead.🤖 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/shared/components/table/TableFilters.tsx` around lines 118 - 122, Replace the exported const enum TableFilterOption with a regular enum or an as-const object so cross-file imports remain compatible with both SWC development and esbuild production transpilation; preserve the existing CHECKBOX, TEXT, and LABEL values and update references only as needed for the chosen representation.web/src/shared/components/table/useTableFilters.ts (1)
9-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the hook and its call sites. printf '\n## useTableFilters outline\n' ast-grep outline web/src/shared/components/table/useTableFilters.ts --view expanded || true printf '\n## search for useTableFilters usages\n' rg -n "useTableFilters\(" web/src -g '*.ts' -g '*.tsx' || true printf '\n## search for initialFilters prop usage patterns nearby\n' rg -n "initialFilters" web/src -g '*.ts' -g '*.tsx' || trueRepository: openshift/monitoring-plugin
Length of output: 1894
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant file with line numbers for exact behavior. cat -n web/src/shared/components/table/useTableFilters.ts | sed -n '1,220p'Repository: openshift/monitoring-plugin
Length of output: 3994
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the hook's direct consumers and surrounding code in one pass. for f in $(rg -l "useTableFilters\(" web/src -g '*.ts' -g '*.tsx'); do echo "### $f" cat -n "$f" | sed -n '1,220p' echo doneRepository: openshift/monitoring-plugin
Length of output: 165
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## file" cat -n web/src/shared/components/table/useTableFilters.ts | sed -n '1,220p' echo echo "## usages" rg -n "useTableFilters\(" web/src -g '*.ts' -g '*.tsx' || true echo echo "## initialFilters references" rg -n "initialFilters" web/src -g '*.ts' -g '*.tsx' || trueRepository: openshift/monitoring-plugin
Length of output: 5543
🏁 Script executed:
#!/bin/bash set -euo pipefail files=( web/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsx web/src/features/alerts/pages/alerts-page/AlertsPage.tsx web/src/features/alerts/pages/silences-page/SilencesPage.tsx web/src/features/perses-dashboards/pages/dashboard-list-page/dashboard-list.tsx web/src/features/targets/pages/targets-page.tsx ) for f in "${files[@]}"; do echo "### $f" ast-grep outline "$f" --view expanded || true echo cat -n "$f" | sed -n '1,260p' echo doneRepository: openshift/monitoring-plugin
Length of output: 50383
🏁 Script executed:
#!/bin/bash set -euo pipefail for f in \ web/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsx \ web/src/features/alerts/pages/alerts-page/AlertsPage.tsx \ web/src/features/alerts/pages/silences-page/SilencesPage.tsx \ web/src/features/perses-dashboards/pages/dashboard-list-page/dashboard-list.tsx \ web/src/features/targets/pages/targets-page.tsx do echo "### $f" cat -n "$f" | sed -n '50,170p' echo doneRepository: openshift/monitoring-plugin
Length of output: 25674
Sync
filterswithinitialFiltersupdatesweb/src/shared/components/table/useTableFilters.ts:14-34— the hook only readsinitialFiltersin the lazy initializer, so callers that rebuild it later won’t get added/removed filter keys reflected in state. Add an effect or another sync path if dynamic filter sets are expected.🤖 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/shared/components/table/useTableFilters.ts` around lines 9 - 34, Update useTableFilters so filters synchronizes when initialFilters changes, including reflecting added and removed filter keys while preserving URL-derived values for current keys. Add the sync near the existing filters state initialization and ensure it does not reset unrelated state unnecessarily.web/src/shared/components/table/useTablePagination.ts (1)
13-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
statedrifts from the URL after mount.
state.page/state.perPageare computed once insideuseState's initializer, so they only reflectsearchParamsat mount time. Browser back/forward navigation, or any other code that mutates the same query params, will changesearchParamswithout ever updatingstate, leaving the pagination UI out of sync with the URL it's supposed to drive.Prefer deriving
page/perPagedirectly fromsearchParamson every render (no localstateneeded) and callingupdateSearchParamsfrom the handlers, so the URL remains the single source of truth.♻️ Suggested fix: derive from searchParams instead of duplicating state
- const [state, setState] = useState({ - page: parsePositiveInt(searchParams?.get(pageParam), page), - perPage: parsePositiveInt(searchParams?.get(perPageParam), perPage), - }); + const currentPage = parsePositiveInt(searchParams?.get(pageParam), page); + const currentPerPage = parsePositiveInt(searchParams?.get(perPageParam), perPage); @@ const onPerPageSelect = ( _event: ReactMouseEvent | KeyboardEvent | MouseEvent | undefined, newPerPage: number, ) => { - if (newPerPage !== state.perPage) { + if (newPerPage !== currentPerPage) { updateSearchParams(1, newPerPage); - setState({ perPage: newPerPage, page: 1 }); } }; @@ const onSetPage = ( _event: ReactMouseEvent | KeyboardEvent | MouseEvent | undefined, newPage: number, ) => { - if (newPage !== state.page) { - updateSearchParams(newPage, state.perPage); - setState((prev) => ({ ...prev, page: newPage })); + if (newPage !== currentPage) { + updateSearchParams(newPage, currentPerPage); } }; @@ return { - ...state, + page: currentPage, + perPage: currentPerPage, onPerPageSelect, onSetPage, };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const useTablePagination = ({ page = 1, perPage = 20, pageParam = PaginationParams.PAGE, perPageParam = PaginationParams.PER_PAGE, }: UseDataViewPaginationProps) => { const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePositiveInt(searchParams?.get(pageParam), page); const currentPerPage = parsePositiveInt(searchParams?.get(perPageParam), perPage); const updateSearchParams = useCallback( (page: number, perPage: number) => { setSearchParams?.((prev) => { const prevParams = new URLSearchParams(prev); prevParams.set(pageParam, `${page}`); prevParams.set(perPageParam, `${perPage}`); // Only update if there is a change in parameters to avoid unnecessary re-renders if (prev.toString() !== prevParams.toString()) { return prevParams; } return prev; }); }, [setSearchParams, pageParam, perPageParam], ); const onPerPageSelect = ( _event: ReactMouseEvent | KeyboardEvent | MouseEvent | undefined, newPerPage: number, ) => { if (newPerPage !== currentPerPage) { updateSearchParams(1, newPerPage); } }; const onSetPage = ( _event: ReactMouseEvent | KeyboardEvent | MouseEvent | undefined, newPage: number, ) => { if (newPage !== currentPage) { updateSearchParams(newPage, currentPerPage); } }; return { page: currentPage, perPage: currentPerPage, onPerPageSelect, onSetPage, };🤖 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/shared/components/table/useTablePagination.ts` around lines 13 - 60, Remove the local state initialized in useTablePagination and derive page and perPage from searchParams on every render using the existing defaults. Update onPerPageSelect and onSetPage to compare against those derived values and call updateSearchParams without setState, making the URL the single source of truth and keeping browser navigation synchronized.web/src/shared/console/console-shared/hooks/useDocumentListener.ts (1)
27-62: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Untyped event parameters (implicit
any).
handleEvent's andhandleKeyEvents'eparameters have no type annotation. Since these callbacks are passed todocument.addEventListenerrather than used inline where JSX provides contextual typing,eis implicitlyany.As per coding guidelines, "Avoid `any` in TypeScript; prefer specific response or domain types instead."♻️ Proposed fix
- const handleEvent = useCallback((e) => { + const handleEvent = useCallback((e: MouseEvent) => { if (!ref?.current?.contains(e.target)) { setVisible(false); } }, []); const handleKeyEvents = useCallback( - (e) => { + (e: KeyboardEvent) => { // Don't steal focus from a modal open on top of the page. if (isModalOpen()) { return; } - const { nodeName } = e.target; + const { nodeName } = e.target as HTMLElement;🤖 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/shared/console/console-shared/hooks/useDocumentListener.ts` around lines 27 - 62, Add explicit DOM event types to the e parameters of handleEvent and handleKeyEvents in the document listener hook, using the event types required by their respective addEventListener registrations. Preserve the existing target, key, and preventDefault behavior without introducing any.Source: Coding guidelines
web/src/shared/console/graphs/bar.tsx (1)
54-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
LabelComponentprop is threaded through but never rendered — customization is dead code.
Baraccepts and forwardsLabelComponenttoBarChart(line 144), andBarChartPropsdeclares it (line 163), butBarChart's destructured props (lines 54-64) omitLabelComponent, and the render always uses the hardcoded localLabelcomponent (line 89). Any caller supplying a customLabelComponenttoBargets no effect whatsoever.🛠️ Proposed fix
const BarChart: FC<BarChartProps> = ({ barSpacing = 15, barWidth = DEFAULT_BAR_WIDTH, data = [], + LabelComponent = Label, loading = false, noLink = false, query, theme = getCustomTheme(ChartThemeColor.blue, barTheme), title, titleClassName, }) => { ... <div className="graph-bar__label"> - <Label title={datum.x} metric={datum.metric} /> + <LabelComponent title={datum.x} metric={datum.metric} /> </div>Also applies to: 86-90, 116-129, 144-144, 159-171, 173-187
🤖 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/shared/console/graphs/bar.tsx` around lines 54 - 64, Update BarChart to destructure the declared LabelComponent prop and render it instead of the hardcoded local Label component, preserving the local Label as the default when no custom component is supplied. Ensure Bar, BarChartProps, and the chart render path consistently pass and use the caller-provided label component.web/src/shared/console/module/k8s/label-selector.js (1)
118-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrap
casebodies withlet/constin blocks to avoid switch-scope leakage.Biome flags
let found(line 130),let keep(line 143),const inConjuncts(line 200), andconst notInConjuncts(line 211) as declared directly under acasewithout a block — these are visible to every othercasein the sameswitch, a known TDZ/scope-leak hazard even though no active collision exists today.🐛 Proposed fix (repeat for all 4 flagged cases)
- case 'in': - let found = false; - if (labels[conjunct.key] || labels[conjunct.key] === '') { + case 'in': { + let found = false; + if (labels[conjunct.key] || labels[conjunct.key] === '') { for (let i = 0; !found && i < conjunct.values.length; i++) { if (labels[conjunct.key] === conjunct.values[i]) { found = true; } } } if (!found) { return false; } break; + } - case 'not in': - let keep = true; + case 'not in': { + let keep = true; if (labels[conjunct.key]) { for (let i = 0; keep && i < conjunct.values.length; i++) { keep = labels[conjunct.key] !== conjunct.values[i]; } } if (!keep) { return false; } + }Apply the analogous
{ }wrapping to the'in'/'not in'cases insidecovers()(lines 198-220).Also applies to: 182-223
🧰 Tools
🪛 Biome (2.5.3)
[error] 130-130: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.(lint/correctness/noSwitchDeclarations)
[error] 143-143: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.(lint/correctness/noSwitchDeclarations)
🤖 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/shared/console/module/k8s/label-selector.js` around lines 118 - 155, Wrap the `'in'` and `'not in'` case bodies in the switch within `covers()` with braces, keeping `let found` and `let keep` scoped to their respective cases. Apply the same block wrapping to the analogous cases declaring `const inConjuncts` and `const notInConjuncts`, without changing their existing matching behavior.Source: Linters/SAST tools
web/src/shared/console/utils/async.tsx (2)
16-19: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm target/useDefineForClassFields to determine if this manifests today fd tsconfig.json web -x cat {}Repository: openshift/monitoring-plugin
Length of output: 811
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== tsconfig files under web ==\n' fd -a 'tsconfig*.json' web printf '\n== compilerOptions snippets ==\n' for f in $(fd -a 'tsconfig*.json' web); do echo "--- $f" python3 - <<'PY' "$f" import json,sys path=sys.argv[1] with open(path) as fh: data=json.load(fh) print(data.get("compilerOptions", {})) PY done printf '\n== any explicit useDefineForClassFields ==\n' rg -n '"useDefineForClassFields"|useDefineForClassFields' web -g 'tsconfig*.json'Repository: openshift/monitoring-plugin
Length of output: 1236
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '== web/tsconfig.json ==' cat -n web/tsconfig.json echo echo '== web/src/shared/console/utils/async.tsx ==' cat -n web/src/shared/console/utils/async.tsx echo echo '== any explicit useDefineForClassFields in web ==' rg -n 'useDefineForClassFields|target' web/tsconfig*.json web/*/tsconfig*.jsonRepository: openshift/monitoring-plugin
Length of output: 4407
🌐 Web query:
TypeScript useDefineForClassFields default target es2021 uninitialized class field emits this.props = void 0💡 Result:
When useDefineForClassFields is enabled (which defaults to true for target targets of ES2022 or later [1][2][3]), TypeScript enforces ECMAScript class field semantics by ensuring uninitialized class fields are explicitly defined as undefined [4][2][5]. To achieve this, the compiler emits code that defines these fields on the instance [4][5]. Depending on your specific target configuration, this may manifest as: 1. Object.defineProperty(this, "fieldName", { enumerable: true, configurable: true, writable: true, value: void 0 }); [4][5] 2. this.fieldName = void 0; [6][7] This behavior occurs because ECMAScript class fields (as opposed to legacy assignment-based fields) mandate that fields are present on the instance [1][5]. If you do not want this behavior—for example, if it interferes with your initialization logic or decorators—you can explicitly set "useDefineForClassFields": false in your tsconfig.json [2][5]. Alternatively, you can prevent the emission of this initialization code by using the declare keyword [5]. When you mark a field as declare, TypeScript treats it as a type-only declaration, which is erased during compilation and does not generate any runtime initialization code [8][5]. Example of avoiding the emission: class MyClass { declare myField: string; // No runtime initialization emitted [5] constructor { this.myField = "value"; } }
Citations:
- 1: microsoft/TypeScript#59632
- 2: https://stackoverflow.com/questions/78880546/class-attributes-are-initialized-to-undefined-when-updating-tsconfig-target-from
- 3: vitejs/vite#10296
- 4: https://blog.wodkiewi.cz/posts/2023-10-31-class-fields-and-ts-2/
- 5: microsoft/TypeScript#37640
- 6: https://stackoverflow.com/questions/77003094/why-is-typescript-generating-assignments-to-void-0-in-class-constructors
- 7: react/create-react-app#6506
- 8: microsoft/TypeScript#33509
Remove the
propsclass fieldweb/src/shared/console/utils/async.tsx:18
props: AsyncComponentProps;overwrites React’s inheritedthis.propsat runtime, sorender()can seeundefined. Delete the declaration;Componentalready providesprops.🤖 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/shared/console/utils/async.tsx` around lines 16 - 19, Remove the explicit props class field from AsyncComponent; rely on React Component’s inherited this.props while preserving the existing state declaration and component behavior.
48-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stale-loader race: an in-flight load can commit a component from an outdated
loader.
loadComponent()doesn't tie the resolvedComponentback to theloaderthat produced it. Ifprops.loaderchanges while a previous load is still pending (triggering a freshloadComponent()call viagetDerivedStateFromProps/componentDidUpdate), the stale promise can still resolve later and overwritestate.Componentwith the wrong module — regardless of which loader is now current.🔒 Proposed fix
private loadComponent() { + const { loader } = this.state; this.state .loader() .then((Component) => { if (!Component) { return Promise.reject(AsyncComponentError.ComponentNotFound); } - if (this.isAsyncMounted) { + if (this.isAsyncMounted && this.state.loader === loader) { this.setState({ Component }); } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private loadComponent() { const { loader } = this.state; this.state .loader() .then((Component) => { if (!Component) { return Promise.reject(AsyncComponentError.ComponentNotFound); } if (this.isAsyncMounted && this.state.loader === loader) { this.setState({ Component }); } }) .catch((error) => { if (error === AsyncComponentError.ComponentNotFound) { // eslint-disable-next-line no-console console.error('Component does not exist in module'); } else { setTimeout(() => this.loadComponent(), this.retryAfter); } }); }🤖 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/shared/console/utils/async.tsx` around lines 48 - 67, Update loadComponent to capture the current loader before starting the async request, then only commit the resolved Component when that loader is still the active state.loader and isAsyncMounted is true. Ensure stale resolutions cannot overwrite state.Component, while preserving the existing missing-component error and retry behavior.web/src/shared/console/utils/single-typeahead-dropdown.tsx (1)
416-432: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rendered option
iddoesn't match thearia-activedescendantid, breaking keyboard/screen-reader focus tracking.
setActiveAndFocusedItem(line 186) computes the active id fromcreateItemId(focusedItem.value), but here the rendered option usescreateItemId(k)— the array index, notv.value. These will almost never match, soaria-activedescendantwill point to a nonexistent element id whenever an option is focused via keyboard.🐛 Proposed fix
<SelectOptionComponent key={k} isSelected={selectedKey === v.value} isFocused={focusedItemIndex === k} - id={createItemId(k)} + id={createItemId(v.value)} value={v.value}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{filteredSelectOptions.map((v, k) => { const SelectOptionComponent = v.value === CREATE_NEW ? SelectOption : (OptionComponent ?? SelectOption); return ( <SelectOptionComponent key={k} isSelected={selectedKey === v.value} isFocused={focusedItemIndex === k} id={createItemId(v.value)} value={v.value} {...v} > {v.children || v.value} </SelectOptionComponent> ); })}🤖 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/shared/console/utils/single-typeahead-dropdown.tsx` around lines 416 - 432, Update the option id in the filteredSelectOptions.map rendering to use createItemId(v.value) instead of the array index k, matching the identifier computed by setActiveAndFocusedItem and keeping aria-activedescendant aligned with the focused option.
| import { ActionType, ObserveAction } from './actions'; | ||
|
|
||
| import { MONITORING_DASHBOARDS_VARIABLE_ALL_OPTION_KEY } from '../components/dashboards/legacy/utils'; | ||
| import { MONITORING_DASHBOARDS_VARIABLE_ALL_OPTION_KEY } from '../../features/legacy-dashboards/utils/utils'; |
There was a problem hiding this comment.
for a follow up PR. but we are importing here from features. Maybe it would be better to put this into constants.
| } from './store'; | ||
| import { produce } from 'immer'; | ||
| import { applySilences } from '../components/alerting/AlertUtils'; | ||
| import { applySilences } from '../../features/alerts/components/AlertUtils'; |
There was a problem hiding this comment.
same here it seems we are importing from features. Maybe we could move this into store in a follow up PR
| } from '../components/Incidents/model'; | ||
| import { Variable } from '../components/dashboards/legacy/legacy-variable-dropdowns'; | ||
| } from '../../features/incidents/types/model'; | ||
| import { Variable } from '../../features/legacy-dashboards/components/legacy-variable-dropdowns'; |
| import { AggregatedAlert } from './alerting/AlertsAggregates'; | ||
| import { QueryParams } from '../constants/query-params'; | ||
| import { AlertSource, MonitoringResource, Target, TimeRange } from '../types/types'; | ||
| import { AggregatedAlert } from '../../features/alerts/pages/alerts-page/AlertsAggregates'; |
There was a problem hiding this comment.
maybe move this to types?
| import { GraphUnits } from './metrics/units'; | ||
| import { DataTestIDs } from './data-test'; | ||
| import { useMonitoring } from '../hooks/useMonitoring'; | ||
| import { GraphUnits } from '../../../features/metrics/utils/units'; |
| import { SingleTypeaheadDropdown } from '../../console/utils/single-typeahead-dropdown'; | ||
| import { CombinedDashboardMetadata } from '../perses/hooks/useDashboardsData'; | ||
| import { SingleTypeaheadDropdown } from '../console/utils/single-typeahead-dropdown'; | ||
| import { CombinedDashboardMetadata } from '../../features/perses-dashboards/hooks/useDashboardsData'; |
There was a problem hiding this comment.
probably we are not using this shared dashboard dropdown anymore...
|
for a follow up. We probably need to update the |
There was a problem hiding this comment.
I thought the pages folder only contained pages, are we including also the components of those pages? should they be under feature//components?
There was a problem hiding this comment.
I was thinking that the feature/components is the components that are shared across the feature, while feature/page/page-x/*.tsx would have component files that are directly related to that page. In this case the AggregateAlertTableRow is only used on the alerts-page, so while it could live under feature/components, putting it next to the only file that uses it makes more sense to me IMO.
There was a problem hiding this comment.
I'm fine if we want to keep the feature/pages as only pages, it doesn't matter that much to me, I just thought that this structure helps with the mental overhead. Since all the pages files are named the same thing the only thing the user needs to check is the file name and location to understand that files relationship to all other files. If it is located in /feature/components its a shared component across the single feature, if it is /shared/components its a shared component across multiple features. If it is located in /feature/pages/page-x/ then it is a component related to a single page. And if the filename ends with *Page.tsx then you know it is the page component
|
Agree with all the locations that could have files split up and individual items moved to the shared folder. I just wanted to get the files moved in one PR and the address the rest in a follow up. Also agree that the documentation needs to be updated, I will also do that in a follow up PR |
|
/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
4 participants
This PR looks to update the folder structure of the proposed monitoring-plugin monorepo setup: https://github.com/observability-ui/harness/pull/4/changes
This PR performs as minimal changes as possible to the actual files, and only looks to move the files and create the new folder structure. Further work to do things like split files, refactor to fit the new structure, ect. are left to future PRs.
This PR is independant of #1034 and they can be merged in either order
Summary by CodeRabbit