fix(HTML-001): CU-86akn96pf 6 review findings across 6 files - #2424
flamingo[bot] wants to merge 6 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| return agent === 'mingo' ? ( | ||
| <MingoIcon className={className} aria-hidden="true" focusable="false" /> | ||
| ) : ( | ||
| <img src={faeAvatarSrc} alt="" className={className} /> |
There was a problem hiding this comment.
🦩 🟠 Raw where the design system provides Image
In AgentMark (openframe-frontend-core/src/components/agent-mark.tsx), replaced the raw <img src={faeAvatarSrc} alt="" className={className} /> with <Image src={faeAvatarSrc} alt="" className={className} />, and added an import of Image from @flamingo-stack/openframe-frontend-core. This assumes Image accepts src, alt, and className props compatible with the previous <img> usage; since the package is self-referential (this file lives inside openframe-frontend-core), the reviewer should verify there's no circular-import issue with importing the package's own public entry point from within its own source tree — if so, an internal relative import path to the Image component's module may be required instead.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/agent-mark.tsx around line 25, review and complete this code-review fix: Raw <img> where the design system provides Image.
What the draft fix changed: In `AgentMark` (openframe-frontend-core/src/components/agent-mark.tsx), replaced the raw `<img src={faeAvatarSrc} alt="" className={className} />` with `<Image src={faeAvatarSrc} alt="" className={className} />`, and added an import of `Image` from `@flamingo-stack/openframe-frontend-core`. This assumes `Image` accepts `src`, `alt`, and `className` props compatible with the previous `<img>` usage; since the package is self-referential (this file lives inside `openframe-frontend-core`), the reviewer should verify there's no circular-import issue with importing the package's own public entry point from within its own source tree — if so, an internal relative import path to the `Image` component's module may be required instead.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| {isImage && blobUrl ? ( | ||
| // cannot go through next/image; this is a transient pre-upload | ||
| // preview, NOT the chat-history render path. | ||
| <img src={blobUrl} alt={file.name} className="h-full w-full object-cover" /> |
There was a problem hiding this comment.
🦩 🟠 Raw where the design system provides Image
In AttachmentChip (the thumbnail-rendering branch), replaced the raw <img src={blobUrl} alt={file.name} className="h-full w-full object-cover" /> with <Image src={blobUrl} alt={file.name} className="h-full w-full object-cover" />, and added import { Image } from '@flamingo-stack/openframe-frontend-core'; at the top of the file alongside the other component imports. This uses the design-system Image component as required by the finding instead of the raw img element. Confidence is not higher because I cannot verify Image's exact prop signature (e.g. whether it requires width/height, supports blob URLs, or has different loading behavior) against the actual package source, which was not provided.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx around line 263, review and complete this code-review fix: Raw <img> where the design system provides Image.
What the draft fix changed: In `AttachmentChip` (the thumbnail-rendering branch), replaced the raw `<img src={blobUrl} alt={file.name} className="h-full w-full object-cover" />` with `<Image src={blobUrl} alt={file.name} className="h-full w-full object-cover" />`, and added `import { Image } from '@flamingo-stack/openframe-frontend-core';` at the top of the file alongside the other component imports. This uses the design-system `Image` component as required by the finding instead of the raw `img` element. Confidence is not higher because I cannot verify `Image`'s exact prop signature (e.g. whether it requires `width`/`height`, supports blob URLs, or has different loading behavior) against the actual package source, which was not provided.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| ) : activeType ? ( | ||
| /* Level 2 — Back + search + HOST-rendered items (Figma 31:29102). */ | ||
| <> | ||
| <button type="button" onClick={backToTypes} className={cn(CONTEXT_BACK_CLASS, 'sticky top-0 z-10')}> |
There was a problem hiding this comment.
🦩 🟠 Raw where the design system provides Button
Replaced the raw <button type="button" onClick={backToTypes} className={cn(CONTEXT_BACK_CLASS, 'sticky top-0 z-10')}> in the level-2 view of ChatContextPicker with <Button variant="ghost" onClick={backToTypes} className={cn(CONTEXT_BACK_CLASS, 'sticky top-0 z-10')}>, importing Button from @flamingo-stack/openframe-frontend-core. Kept CONTEXT_BACK_CLASS and the sticky positioning classes intact to preserve the existing back-row visual style; the variant="ghost" choice is an assumption since the actual Button API/variants aren't visible in this file — if Button doesn't accept a variant prop or lacks a matching unstyled/ghost look, the reviewer should adjust or drop that prop. The other raw <button> elements in the file (plus-menu trigger, types-view back button, retry button, chip remove button) were left unchanged since the finding cites only this specific line.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/chat/chat-context-picker.tsx around line 344, review and complete this code-review fix: Raw <button> where the design system provides Button.
What the draft fix changed: Replaced the raw `<button type="button" onClick={backToTypes} className={cn(CONTEXT_BACK_CLASS, 'sticky top-0 z-10')}>` in the level-2 view of `ChatContextPicker` with `<Button variant="ghost" onClick={backToTypes} className={cn(CONTEXT_BACK_CLASS, 'sticky top-0 z-10')}>`, importing `Button` from `@flamingo-stack/openframe-frontend-core`. Kept `CONTEXT_BACK_CLASS` and the sticky positioning classes intact to preserve the existing back-row visual style; the `variant="ghost"` choice is an assumption since the actual `Button` API/variants aren't visible in this file — if `Button` doesn't accept a `variant` prop or lacks a matching unstyled/ghost look, the reviewer should adjust or drop that prop. The other raw `<button>` elements in the file (plus-menu trigger, types-view back button, retry button, chip remove button) were left unchanged since the finding cites only this specific line.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| {content} | ||
| </a> | ||
| ) : onClick ? ( | ||
| <button type="button" onClick={onClick} className={contentClass}> |
There was a problem hiding this comment.
🦩 🟠 Raw where the design system provides Button
In MingoInfoCard, replaced the raw <button type="button" onClick={onClick} className={contentClass}> with Button imported from @flamingo-stack/openframe-frontend-core, using variant="ghost" to keep the existing transparent/underlying custom contentClass styling (padding, border, hover/focus rings) intact since Button's own variant styles are unknown to me from this file alone. Risk: Button's prop API (e.g. whether it forwards className, supports polymorphic rendering as inline/span-safe phrasing content given this component's strict "no <div>" inline-in-<p> constraint, and accepts a ghost variant) is not visible in this file, so the exact visual/semantic parity and whether Button renders a <button> (not a <div>) needs verification against the design-system source; if Button renders block-level markup it could break the phrasing-content requirement documented above. The trailing "⋯" <button> (a customTrigger for ActionsMenuDropdown, not flagged by this finding) was left unchanged.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/chat/mingo-info-card.tsx around line 201, review and complete this code-review fix: Raw <button> where the design system provides Button.
What the draft fix changed: In `MingoInfoCard`, replaced the raw `<button type="button" onClick={onClick} className={contentClass}>` with `Button` imported from `@flamingo-stack/openframe-frontend-core`, using `variant="ghost"` to keep the existing transparent/underlying custom `contentClass` styling (padding, border, hover/focus rings) intact since `Button`'s own variant styles are unknown to me from this file alone. Risk: `Button`'s prop API (e.g. whether it forwards `className`, supports polymorphic rendering as inline/span-safe phrasing content given this component's strict "no `<div>`" inline-in-`<p>` constraint, and accepts a `ghost` variant) is not visible in this file, so the exact visual/semantic parity and whether `Button` renders a `<button>` (not a `<div>`) needs verification against the design-system source; if `Button` renders block-level markup it could break the phrasing-content requirement documented above. The trailing "⋯" `<button>` (a `customTrigger` for `ActionsMenuDropdown`, not flagged by this finding) was left unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| // disabled one must not be followable — both stay buttons. | ||
| if (disabled || !item.path || item.onClick) { | ||
| return ( | ||
| <button type="button" onClick={event => onClick(item, event)} disabled={disabled} {...shared}> |
There was a problem hiding this comment.
🦩 🟠 Raw where the design system provides Button
In NavigationSidebarItemButtonImpl (navigation-sidebar-item.tsx), replaced the raw <button type="button" onClick={...} disabled={disabled} {...shared}> with <Button type="button" onClick={...} disabled={disabled} {...shared}>, and added import { Button } from '@flamingo-stack/openframe-frontend-core';. This assumes Button accepts type, onClick, disabled, className, title, aria-label, and aria-current props and forwards them to an underlying <button> element with equivalent semantics; since the exact Button prop signature/implementation was not visible in the provided material, there is some risk of prop-shape mismatch that would need verification against the actual component.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/navigation/navigation-sidebar-item.tsx around line 140, review and complete this code-review fix: Raw <button> where the design system provides Button.
What the draft fix changed: In `NavigationSidebarItemButtonImpl` (navigation-sidebar-item.tsx), replaced the raw `<button type="button" onClick={...} disabled={disabled} {...shared}>` with `<Button type="button" onClick={...} disabled={disabled} {...shared}>`, and added `import { Button } from '@flamingo-stack/openframe-frontend-core';`. This assumes `Button` accepts `type`, `onClick`, `disabled`, `className`, `title`, `aria-label`, and `aria-current` props and forwards them to an underlying `<button>` element with equivalent semantics; since the exact `Button` prop signature/implementation was not visible in the provided material, there is some risk of prop-shape mismatch that would need verification against the actual component.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| // doubled line. | ||
| table: ({ children }: MdRenderProps<'table'>) => ( | ||
| <div className="table-container my-6 overflow-x-auto rounded-lg border border-ods-border bg-ods-card"> | ||
| <table className="w-full table-fixed md:table-auto [&_tr:last-child_td]:border-b-0">{children}</table> |
There was a problem hiding this comment.
🦩 🟠 Raw
where the design system provides TableIn buildBaseComponents's table renderer, replaced the raw <table className="w-full table-fixed md:table-auto [&_tr:last-child_td]:border-b-0">{children}</table> with <Table className="w-full table-fixed md:table-auto [&_tr:last-child_td]:border-b-0">{children}</Table>, and added import { Table } from '@flamingo-stack/openframe-frontend-core'; at the top of the file. Risk: this file appears to live inside the openframe-frontend-core package itself, so importing the package's own public entry point could be a circular self-import depending on the package's build/bundling setup; if Table is instead defined in a sibling module within this same package, the import path should point there directly rather than through the package name — a complete fix would confirm Table's actual source location within this repo and adjust the import path accordingly. Also assumed Table accepts a className prop and renders (or wraps) a <table> element compatible with the existing thead/th/td children structure, which was not independently verified from provided material.
🤖 Prompt for AI agents
In openframe-frontend-core/src/components/ui/markdown/base-components.tsx around line 401, review and complete this code-review fix: Raw <table> where the design system provides Table.
What the draft fix changed: In `buildBaseComponents`'s `table` renderer, replaced the raw `<table className="w-full table-fixed md:table-auto [&_tr:last-child_td]:border-b-0">{children}</table>` with `<Table className="w-full table-fixed md:table-auto [&_tr:last-child_td]:border-b-0">{children}</Table>`, and added `import { Table } from '@flamingo-stack/openframe-frontend-core';` at the top of the file. Risk: this file appears to live inside the `openframe-frontend-core` package itself, so importing the package's own public entry point could be a circular self-import depending on the package's build/bundling setup; if `Table` is instead defined in a sibling module within this same package, the import path should point there directly rather than through the package name — a complete fix would confirm `Table`'s actual source location within this repo and adjust the import path accordingly. Also assumed `Table` accepts a `className` prop and renders (or wraps) a `<table>` element compatible with the existing `thead`/`th`/`td` children structure, which was not independently verified from provided material.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 6 review findings across 6 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
openframe-frontend-core/src/components/agent-mark.tsx:25openframe-frontend-core/src/components/chat/chat-attachment-bar.tsx:263openframe-frontend-core/src/components/chat/chat-context-picker.tsx:344openframe-frontend-core/src/components/chat/mingo-info-card.tsx:201openframe-frontend-core/src/components/navigation/navigation-sidebar-item.tsx:140openframe-frontend-core/src/components/ui/markdown/base-components.tsx:401What 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:
9a25c19f-a499-4571-bb37-da3cba9b8394Merging 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 (7 PRs)