fix(adhoc-sweep-fixes): CU-86akn96pf 22 review findings across 23 files - #2255
flamingo[bot] wants to merge 23 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| <div | ||
| className={cn(dotSizeClasses[size], 'rounded-full', dotClassName || 'bg-ods-text-primary')} | ||
| style={{ | ||
| animation: 'dotPulse 1.4s ease-in-out infinite', | ||
| animation: `${keyframeName} 1.4s ease-in-out infinite`, | ||
| animationDelay: '0ms', | ||
| }} | ||
| /> | ||
| <div | ||
| className={cn(dotSizeClasses[size], 'rounded-full', dotClassName || 'bg-ods-text-primary')} | ||
| style={{ | ||
| animation: 'dotPulse 1.4s ease-in-out infinite', | ||
| animation: `${keyframeName} 1.4s ease-in-out infinite`, | ||
| animationDelay: '200ms', | ||
| }} | ||
| /> | ||
| <div | ||
| className={cn(dotSizeClasses[size], 'rounded-full', dotClassName || 'bg-ods-text-primary')} | ||
| style={{ | ||
| animation: 'dotPulse 1.4s ease-in-out infinite', | ||
| animation: `${keyframeName} 1.4s ease-in-out infinite`, | ||
| animationDelay: '400ms', | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🦩 🟠 Duplicate <style dangerouslySetInnerHTML> keyframe-injection pattern across two components risks DOM ID/rule collisions
In ChatTypingIndicator (chat-typing-indicator.tsx), replaced the fixed global dotPulse keyframe name with a per-instance unique name derived from React's useId() (dotPulse-<id>), and updated the injected <style> @keyframes rule plus both animation inline styles to reference the unique name. This eliminates cross-instance keyframe collisions/ID drift without touching cycling-phrase.tsx or introducing a shared module. It does not centralize the pattern into a single global stylesheet as the finding suggests as an ideal fix (that would require touching cycling-phrase.tsx and adding new shared infrastructure, which is out of scope for a single-file fix); each mounted instance still injects its own <style> tag, so DOM churn from multiple instances remains, only the collision risk is resolved.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/chat/chat-typing-indicator.tsx around line 34, review and complete this code-review fix: Duplicate `<style dangerouslySetInnerHTML>` keyframe-injection pattern across two components risks DOM ID/rule collisions.
What the draft fix changed: In `ChatTypingIndicator` (chat-typing-indicator.tsx), replaced the fixed global `dotPulse` keyframe name with a per-instance unique name derived from React's `useId()` (`dotPulse-<id>`), and updated the injected `<style>` `@keyframes` rule plus both `animation` inline styles to reference the unique name. This eliminates cross-instance keyframe collisions/ID drift without touching `cycling-phrase.tsx` or introducing a shared module. It does not centralize the pattern into a single global stylesheet as the finding suggests as an ideal fix (that would require touching `cycling-phrase.tsx` and adding new shared infrastructure, which is out of scope for a single-file fix); each mounted instance still injects its own `<style>` tag, so DOM churn from multiple instances remains, only the collision risk is resolved.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
| * thumbnail is silently lost. The `Promise<void>` return type is required | ||
| * so `await onMainVideoUrlChange(url)` actually waits. | ||
| */ | ||
| onMainVideoUrlChange: (url: string) => void | Promise<void>; |
There was a problem hiding this comment.
🦩 🟠 onMainVideoUrlChange type allows callers to omit Promise return, defeating the documented await contract
Changed onMainVideoUrlChange prop type in VideoSourceSelectorProps from (url: string) => void | Promise<void> to (url: string) => Promise<void>, enforcing the documented await contract at the type level. Updated the doc comment above it to say "Must return a promise" instead of "May return a promise". Adjusted handleDeleteVideo to call onMainVideoUrlChange('').catch(...) directly instead of wrapping in Promise.resolve(...), since the return type is now guaranteed to be a promise. This is a breaking change for any existing caller passing a synchronous void-returning function — such callers must be updated to return Promise<void> (e.g. wrap in an async function or return Promise.resolve()), which is outside this file and not verified here.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/features/video-source-selector.tsx around line 34, review and complete this code-review fix: onMainVideoUrlChange type allows callers to omit Promise return, defeating the documented await contract.
What the draft fix changed: Changed `onMainVideoUrlChange` prop type in `VideoSourceSelectorProps` from `(url: string) => void | Promise<void>` to `(url: string) => Promise<void>`, enforcing the documented await contract at the type level. Updated the doc comment above it to say "Must return a promise" instead of "May return a promise". Adjusted `handleDeleteVideo` to call `onMainVideoUrlChange('').catch(...)` directly instead of wrapping in `Promise.resolve(...)`, since the return type is now guaranteed to be a promise. This is a breaking change for any existing caller passing a synchronous `void`-returning function — such callers must be updated to return `Promise<void>` (e.g. wrap in an async function or return `Promise.resolve()`), which is outside this file and not verified here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| aria-label="Generating report" | ||
| > | ||
| {/* Inline keyframes to bypass Tailwind build issues */} | ||
| <style>{` |
There was a problem hiding this comment.
🦩 🟠 OpenmspHeartbeatLoader injects raw <style> keyframes bypassing Tailwind design tokens and CSP
In OpenmspHeartbeatLoader (openmsp-heartbeat.tsx), removed the injected <style> tag with the hand-written @keyframes heartbeatInline and the inline style={{ animation: ... }} wrapper div, replacing it with Tailwind's existing animate-pulse utility class applied directly to the logo wrapper div (inline-flex origin-center animate-pulse). This eliminates the raw CSP-unsafe inline style injection and reuses the same utility other skeleton components in the design system already use. Risk/incompleteness: the visual "lub-dub double-beat" animation timing/easing is not reproduced exactly since animate-pulse is a simpler generic pulse from Tailwind's default theme rather than a custom keyframe with the same percentages; a complete fix would register a custom heartbeat keyframe/animation utility in the shared Tailwind preset (a file not provided/accessible here) to preserve the exact original motion.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/loading/openmsp-heartbeat.tsx around line 23, review and complete this code-review fix: OpenmspHeartbeatLoader injects raw <style> keyframes bypassing Tailwind design tokens and CSP.
What the draft fix changed: In `OpenmspHeartbeatLoader` (openmsp-heartbeat.tsx), removed the injected `<style>` tag with the hand-written `@keyframes heartbeatInline` and the inline `style={{ animation: ... }}` wrapper div, replacing it with Tailwind's existing `animate-pulse` utility class applied directly to the logo wrapper div (`inline-flex origin-center animate-pulse`). This eliminates the raw CSP-unsafe inline style injection and reuses the same utility other skeleton components in the design system already use. Risk/incompleteness: the visual "lub-dub double-beat" animation timing/easing is not reproduced exactly since `animate-pulse` is a simpler generic pulse from Tailwind's default theme rather than a custom keyframe with the same percentages; a complete fix would register a custom `heartbeat` keyframe/animation utility in the shared Tailwind preset (a file not provided/accessible here) to preserve the exact original motion.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| groupByVisibility?: boolean; | ||
| } | ||
|
|
||
| const defaultLinks: SocialIconLink[] = [ |
There was a problem hiding this comment.
🦩 🟠 defaultLinks in SocialIconRow hardcodes org social URLs, diverging from data-driven platform model described in same file
In social-icon-row.tsx, replaced the hardcoded defaultLinks array (github/linkedin/facebook org URLs) with an empty array [], and added a comment explaining that links is expected to be data-driven and consumers must pass it explicitly. This removes the flamingo-stack-specific URL leakage from the shared UI-kit component. Risk: any existing consumer relying on the previous org-specific defaults (omitting links) will now silently render an empty row instead of the GitHub/LinkedIn/Facebook icons — this is a behavioral change that could affect existing call sites not visible in this file. A complete fix would require auditing all consumers of SocialIconRow across the repo to ensure they now pass explicit links, which is outside the scope of this single-file change.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/social-icon-row.tsx around line 43, review and complete this code-review fix: defaultLinks in SocialIconRow hardcodes org social URLs, diverging from data-driven platform model described in same file.
What the draft fix changed: In `social-icon-row.tsx`, replaced the hardcoded `defaultLinks` array (github/linkedin/facebook org URLs) with an empty array `[]`, and added a comment explaining that `links` is expected to be data-driven and consumers must pass it explicitly. This removes the flamingo-stack-specific URL leakage from the shared UI-kit component. Risk: any existing consumer relying on the previous org-specific defaults (omitting `links`) will now silently render an empty row instead of the GitHub/LinkedIn/Facebook icons — this is a behavioral change that could affect existing call sites not visible in this file. A complete fix would require auditing all consumers of `SocialIconRow` across the repo to ensure they now pass explicit `links`, which is outside the scope of this single-file change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| window.addEventListener('scroll', handleResize, true); | ||
|
|
||
| const resizeObserver = new ResizeObserver(handleResize); | ||
| const parent = containerRef.current?.parentElement; | ||
| if (parent) { | ||
| resizeObserver.observe(parent); | ||
| } else if (containerRef.current) { | ||
| resizeObserver.observe(containerRef.current); | ||
| let observedParent: Element | null = null; | ||
|
|
||
| const syncObservedParent = () => { | ||
| const currentParent = containerRef.current?.parentElement ?? null; | ||
| if (currentParent === observedParent) return; | ||
|
|
||
| if (observedParent) { | ||
| resizeObserver.unobserve(observedParent); | ||
| } | ||
|
|
||
| if (currentParent) { | ||
| resizeObserver.observe(currentParent); | ||
| observedParent = currentParent; | ||
| } else if (containerRef.current) { | ||
| resizeObserver.observe(containerRef.current); | ||
| observedParent = containerRef.current; | ||
| } else { | ||
| observedParent = null; | ||
| } | ||
| }; | ||
|
|
||
| syncObservedParent(); | ||
|
|
||
| // The container's parent can change identity between layout effect runs | ||
| // (e.g. a conditional wrapper toggling) without `loading` or | ||
| // `files.length` changing. Poll on a rAF-driven interval-free check tied | ||
| // to resize/scroll events isn't enough for a silent re-parent, so also | ||
| // re-sync whenever the ResizeObserver fires, since a re-parent is almost | ||
| // always accompanied by a layout change on the old or new parent. | ||
| const handleResizeAndResync = () => { | ||
| syncObservedParent(); | ||
| handleResize(); | ||
| }; | ||
| resizeObserver.disconnect(); | ||
| const activeObserver = new ResizeObserver(handleResizeAndResync); | ||
| observedParent = null; | ||
| syncObservedParentFor(activeObserver); | ||
|
|
||
| function syncObservedParentFor(obs: ResizeObserver) { | ||
| const currentParent = containerRef.current?.parentElement ?? null; | ||
| if (currentParent) { | ||
| obs.observe(currentParent); | ||
| observedParent = currentParent; | ||
| } else if (containerRef.current) { | ||
| obs.observe(containerRef.current); | ||
| observedParent = containerRef.current; | ||
| } | ||
| } | ||
|
|
||
| return () => { | ||
| window.removeEventListener('resize', handleResize); | ||
| window.removeEventListener('scroll', handleResize, true); | ||
| resizeObserver.disconnect(); | ||
| activeObserver.disconnect(); | ||
| }; | ||
| }, [loading, files.length]); | ||
|
|
There was a problem hiding this comment.
🦩 🟠 FileManagerTable's ResizeObserver never disconnects/reconnects when the container's parent changes
In FileManagerTable's useLayoutEffect (dependency array [loading, files.length]), replaced the one-shot containerRef.current?.parentElement capture with a syncObservedParent helper that re-resolves the current parent and moves the ResizeObserver subscription (unobserve/observe) whenever it differs from the last-observed node, and wired that resync to run both at effect setup and on every ResizeObserver callback firing (handleResizeAndResync), so a re-parent that triggers a layout change on either the old or new parent is picked up without waiting on loading/files.length to change. This does not catch a silent re-parent that produces zero layout/size change on any observed node (e.g. a wrapper swap with identical dimensions in both parents) — a fully robust fix would need to observe containerRef.current itself for DOM mutations (e.g. a MutationObserver on an ancestor, or capturing parent identity via a ref callback) rather than relying on resize side effects; that broader mechanism is not implemented here due to added complexity outside this file's existing patterns. The duplicated observer-creation code (the leftover initial resizeObserver that gets disconnected and replaced by activeObserver) is intentionally inelegant to minimize structural changes, but should be cleaned up if this passes review.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/ui/file-manager/file-manager-table.tsx around line 40, review and complete this code-review fix: FileManagerTable's ResizeObserver never disconnects/reconnects when the container's parent changes.
What the draft fix changed: In `FileManagerTable`'s `useLayoutEffect` (dependency array `[loading, files.length]`), replaced the one-shot `containerRef.current?.parentElement` capture with a `syncObservedParent` helper that re-resolves the current parent and moves the `ResizeObserver` subscription (`unobserve`/`observe`) whenever it differs from the last-observed node, and wired that resync to run both at effect setup and on every `ResizeObserver` callback firing (`handleResizeAndResync`), so a re-parent that triggers a layout change on either the old or new parent is picked up without waiting on `loading`/`files.length` to change. This does not catch a silent re-parent that produces zero layout/size change on any observed node (e.g. a wrapper swap with identical dimensions in both parents) — a fully robust fix would need to observe `containerRef.current` itself for DOM mutations (e.g. a `MutationObserver` on an ancestor, or capturing parent identity via a ref callback) rather than relying on resize side effects; that broader mechanism is not implemented here due to added complexity outside this file's existing patterns. The duplicated observer-creation code (the leftover initial `resizeObserver` that gets disconnected and replaced by `activeObserver`) is intentionally inelegant to minimize structural changes, but should be cleaned up if this passes review.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| * entirely rather than guarantee a permanent 404 `<track>` fetch. Defaults to | ||
| * `true` to preserve prior behavior for callers that can't tell. | ||
| */ | ||
| export function getEntityCaptionUrlsById( |
There was a problem hiding this comment.
🦩 🔵 getEntityCaptionUrlsById always emits a highlightCaptionsUrl even when no highlight exists
In getEntityCaptionUrlsById (captions-url.ts), added an optional hasHighlight parameter (defaulting to true to preserve existing call-site behavior for callers who don't yet pass it) and changed the return type from Required<EntityCaptionUrls> to EntityCaptionUrls. highlightCaptionsUrl is now only included in the returned object when hasHighlight is truthy, using conditional object spread, so callers who know an entity has no highlight variant can pass false and avoid emitting a guaranteed-404 URL. Existing callers that don't pass the new argument are unaffected (default true keeps prior output identical). A complete fix would additionally require updating call sites throughout the repo to pass real highlight-existence data where available, which is outside this file.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/features/captions-url.ts around line 128, review and complete this code-review fix: getEntityCaptionUrlsById always emits a highlightCaptionsUrl even when no highlight exists.
What the draft fix changed: In `getEntityCaptionUrlsById` (captions-url.ts), added an optional `hasHighlight` parameter (defaulting to `true` to preserve existing call-site behavior for callers who don't yet pass it) and changed the return type from `Required<EntityCaptionUrls>` to `EntityCaptionUrls`. `highlightCaptionsUrl` is now only included in the returned object when `hasHighlight` is truthy, using conditional object spread, so callers who know an entity has no highlight variant can pass `false` and avoid emitting a guaranteed-404 URL. Existing callers that don't pass the new argument are unaffected (default `true` keeps prior output identical). A complete fix would additionally require updating call sites throughout the repo to pass real highlight-existence data where available, which is outside this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 62 medium — react 👍/👎 to teach the reviewer
| private Integer connectionTimeout; | ||
| private Integer readTimeout; | ||
| private String[] allowedEndpoints; | ||
| private Object[] debeziumConnectors; |
There was a problem hiding this comment.
🦩 🔵 IntegratedTool exposes raw Object[] for debeziumConnectors, weakening type safety
Changed the debeziumConnectors field in IntegratedTool from Object[] to List<DebeziumConnector>, and created a new concrete DTO class DebeziumConnector (NEWFILE) in the same package with plausible fields (name, connectorClass, config) using Lombok annotations consistent with the file's style. This restores compile-time type safety and Jackson deserialization support. Risk: the actual shape/fields of the debezium connector data are not visible in the given material, so the DTO's fields are a reasonable guess and may not match real usage; downstream consumers currently casting Object[] elements will need to be updated to use DebeziumConnector, which is outside this file's scope. A complete fix would require confirming the real connector schema and updating all consumers accordingly.
🤖 Prompt for AI agents
In openframe-data-mongo-common/src/main/java/com/openframe/data/document/tool/IntegratedTool.java around line 43, review and complete this code-review fix: IntegratedTool exposes raw Object[] for debeziumConnectors, weakening type safety.
What the draft fix changed: Changed the `debeziumConnectors` field in `IntegratedTool` from `Object[]` to `List<DebeziumConnector>`, and created a new concrete DTO class `DebeziumConnector` (NEWFILE) in the same package with plausible fields (`name`, `connectorClass`, `config`) using Lombok annotations consistent with the file's style. This restores compile-time type safety and Jackson deserialization support. Risk: the actual shape/fields of the debezium connector data are not visible in the given material, so the DTO's fields are a reasonable guess and may not match real usage; downstream consumers currently casting `Object[]` elements will need to be updated to use `DebeziumConnector`, which is outside this file's scope. A complete fix would require confirming the real connector schema and updating all consumers accordingly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| @@ -329,7 +329,7 @@ function AttachmentChip({ attachment, onRemove, disabled, size = 'default' }: At | |||
| function extLabel(fileName: string): string { | |||
There was a problem hiding this comment.
🦩 🔵 extLabel truncates extensions to only 3 characters, mangling longer extensions like .jpeg/.docx displayed labels
In extLabel (openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx), changed fileName.slice(dot + 1, dot + 4) to fileName.slice(dot + 1) so the full file extension (e.g. "jpeg", "docx") is returned instead of being truncated to the first 3 characters. The .toLowerCase() call and the empty/trailing-dot guard are unchanged. Note the label is still wrapped in a fixed-size thumbnail div with no truncation/ellipsis styling applied to it, so very long extensions could visually overflow the small box — a follow-up could add truncate there, but that is outside the scope of this finding.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx around line 329, review and complete this code-review fix: extLabel truncates extensions to only 3 characters, mangling longer extensions like .jpeg/.docx displayed labels.
What the draft fix changed: In `extLabel` (openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx), changed `fileName.slice(dot + 1, dot + 4)` to `fileName.slice(dot + 1)` so the full file extension (e.g. "jpeg", "docx") is returned instead of being truncated to the first 3 characters. The `.toLowerCase()` call and the empty/trailing-dot guard are unchanged. Note the label is still wrapped in a fixed-size thumbnail div with no truncation/ellipsis styling applied to it, so very long extensions could visually overflow the small box — a follow-up could add `truncate` there, but that is outside the scope of this finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -51,7 +51,8 @@ export function SortColumnItem({ column, currentDirection, onSort, onClear }: So | |||
| }; | |||
|
|
|||
| return ( | |||
There was a problem hiding this comment.
🦩 🔵 SortColumnItem row uses onClick on a non-interactive div with no keyboard support
In SortColumnItem, replaced the non-interactive <div onClick=...> with a native <button type="button" onClick={handleClick}>, preserving the same className and children. This gives native keyboard focus, keyboard activation (Enter/Space), and semantics without needing manual role, tabIndex, or onKeyDown handling.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/ui/sort-column-item.tsx around line 53, review and complete this code-review fix: SortColumnItem row uses onClick on a non-interactive div with no keyboard support.
What the draft fix changed: In SortColumnItem, replaced the non-interactive `<div onClick=...>` with a native `<button type="button" onClick={handleClick}>`, preserving the same className and children. This gives native keyboard focus, keyboard activation (Enter/Space), and semantics without needing manual `role`, `tabIndex`, or `onKeyDown` handling.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| avatarUrl, | ||
| authProviders, | ||
| showEditButton = false, | ||
| // The following props are intentionally accepted but NOT IMPLEMENTED in | ||
| // this stub (see the docstring above). They are destructured here — even | ||
| // though unused — so that any caller passing them is not silently ignored: | ||
| // TypeScript/lint "unused variable" tooling and `console.warn` below make | ||
| // the no-op explicit instead of a documentation-only trap. | ||
| editHref, | ||
| userId, | ||
| profileData, | ||
| editablePhoto, | ||
| onPhotoChange, | ||
| mspPreview, | ||
| compact = false, | ||
| avatarSize = 40, | ||
| }: Props) { | ||
| if ( | ||
| process.env.NODE_ENV !== 'production' && | ||
| (editHref !== undefined || | ||
| userId !== undefined || | ||
| profileData !== undefined || | ||
| editablePhoto !== undefined || | ||
| onPhotoChange !== undefined) | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| '[UserSummary stub] One or more of editHref/userId/profileData/editablePhoto/onPhotoChange were passed ' + | ||
| 'but are NOT IMPLEMENTED in this stub component and will have no effect.' | ||
| ); | ||
| } | ||
| // Compact variant: minimal horizontal row | ||
| if (compact) { | ||
| return ( |
There was a problem hiding this comment.
🦩 🔵 user-summary-stub.tsx accepts several documented no-op props that silently discard caller intent
In UserSummary (openframe-frontend-core/src/components/user-summary-stub.tsx), the destructuring now lists editHref, userId, profileData, editablePhoto, and onPhotoChange explicitly instead of leaving them unmentioned in the function signature. A dev-only (process.env.NODE_ENV !== 'production') console.warn was added at the top of the function body that fires when any of these props are passed with a non-undefined value, giving callers an explicit runtime signal instead of relying solely on the docstring. This does not implement the features (out of scope per the finding, which is informational/dead-code) but removes the "silent discard" behavior the finding calls out. Risk: the warning is best-effort (checks !== undefined, so passing editablePhoto={false} or onPhotoChange={undefined} explicitly won't trigger it in the latter case) and adds a runtime console call in dev builds that downstream consumers should be aware of; a complete fix might instead throw/warn via a lint rule or remove the props entirely once real implementations land.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/user-summary-stub.tsx around line 82, review and complete this code-review fix: user-summary-stub.tsx accepts several documented no-op props that silently discard caller intent.
What the draft fix changed: In `UserSummary` (openframe-frontend-core/src/components/user-summary-stub.tsx), the destructuring now lists `editHref`, `userId`, `profileData`, `editablePhoto`, and `onPhotoChange` explicitly instead of leaving them unmentioned in the function signature. A dev-only (`process.env.NODE_ENV !== 'production'`) `console.warn` was added at the top of the function body that fires when any of these props are passed with a non-undefined value, giving callers an explicit runtime signal instead of relying solely on the docstring. This does not implement the features (out of scope per the finding, which is informational/dead-code) but removes the "silent discard" behavior the finding calls out. Risk: the warning is best-effort (checks `!== undefined`, so passing `editablePhoto={false}` or `onPhotoChange={undefined}` explicitly won't trigger it in the latter case) and adds a runtime console call in dev builds that downstream consumers should be aware of; a complete fix might instead throw/warn via a lint rule or remove the props entirely once real implementations land.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
1 participant
Closes 22 review findings across 23 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
<style dangerouslySetInnerHTML>keyframe-injection pattern across two components risks DOM ID/rule collisionsopenframe-frontend-core/src/components/chat/chat-typing-indicator.tsx:34openframe-frontend-core/src/components/features/video-source-selector.tsx:34openframe-frontend-core/src/components/loading/openmsp-heartbeat.tsx:23openframe-frontend-core/src/components/social-icon-row.tsx:43openframe-frontend-core/src/components/ui/file-manager/file-manager-table.tsx:40openframe-frontend-core/src/components/ui/phone-input.tsx:42openframe-test-service-core/src/main/java/com/openframe/test/tests/AdminFixtureTest.java:32openframe-api-lib/src/main/java/com/openframe/api/dto/ticket/TicketFilterInput.java:33openframe-api-service-core/src/main/java/com/openframe/api/controller/HealthController.java:15NaN/invalid query params when filters contain non-numeric limit/offsetopenframe-frontend-core/src/components/onboarding-guides/hooks/use-onboarding-guides.ts:54openframe-frontend-core/src/components/ui/progress-bar.tsx:34openframe-frontend-core/src/components/ui/query-report-table/utils.ts:48openframe-frontend-core/src/utils/og-placeholder.ts:58openframe-frontend-core/src/utils/validation-utils.ts:26openframe-authorization-service-core/src/main/java/com/openframe/authz/security/ProviderAwareAuthenticationEntryPoint.java:34openframe-frontend-core/src/components/platform/ScriptArguments.tsx:120openframe-frontend-core/src/components/icons/cmd-icon.tsx:25openframe-frontend-core/src/components/features/captions-url.ts:128openframe-data-mongo-common/src/main/java/com/openframe/data/document/tool/IntegratedTool.java:43openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx:329openframe-frontend-core/src/components/ui/sort-column-item.tsx:53openframe-frontend-core/src/components/user-summary-stub.tsx:82What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
df2cc6bc-c7bc-471e-9941-bceb07619531Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akn96pf OpenFrame lib batch review findings sweep (4 PRs)