chore: upgrade flo-ai and ui improvements#279
Conversation
📝 WalkthroughWalkthroughThis PR updates the Gemini LLM message content handler to properly extract content from dict responses, enhances ChatBot attachment display with combined rendering and popover overflow handling, enables YAML editor container scrolling, and bumps flo-ai dependencies across server modules from 1.1.3 to 1.1.4. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
wavefront/client/src/components/ChatBot.tsx (3)
110-116: Minor: consolidate redundant slice math and consider memoization.
remainingCombinedAttachmentsCountandremainingCombinedAttachmentsare two computations of the same thing — prefer driving the count fromremainingCombinedAttachments.length. Also, becausecombinedAttachmentsis rebuilt on every render, you may want to wrap the derivation inuseMemoto avoid spreading both arrays and re-mapping on unrelated state changes (e.g. typing in the input). Not a correctness issue, purely cleanliness.♻️ Proposed refactor
- const combinedAttachments = [ - ...uploadedImages.map((image, index) => ({ kind: 'image' as const, image, originalIndex: index })), - ...uploadedDocuments.map((document, index) => ({ kind: 'document' as const, document, originalIndex: index })), - ]; - const visibleCombinedAttachments = combinedAttachments.slice(0, 2); - const remainingCombinedAttachmentsCount = Math.max(combinedAttachments.length - visibleCombinedAttachments.length, 0); - const remainingCombinedAttachments = combinedAttachments.slice(2); + const combinedAttachments = useMemo( + () => [ + ...uploadedImages.map((image, index) => ({ kind: 'image' as const, image, originalIndex: index })), + ...uploadedDocuments.map((document, index) => ({ kind: 'document' as const, document, originalIndex: index })), + ], + [uploadedImages, uploadedDocuments] + ); + const visibleCombinedAttachments = combinedAttachments.slice(0, 2); + const remainingCombinedAttachments = combinedAttachments.slice(2); + const remainingCombinedAttachmentsCount = remainingCombinedAttachments.length;(Add
useMemoto thereactimport.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/client/src/components/ChatBot.tsx` around lines 110 - 116, Derive remainingCombinedAttachmentsCount from remainingCombinedAttachments.length and memoize the combined attachments derivation: wrap building combinedAttachments, visibleCombinedAttachments, remainingCombinedAttachments (currently using combinedAttachments.slice(...)) in a useMemo hook so they’re only recalculated when uploadedImages or uploadedDocuments change; compute visibleCombinedAttachments = combinedAttachments.slice(0,2), remainingCombinedAttachments = combinedAttachments.slice(2) inside the memo and set remainingCombinedAttachmentsCount = remainingCombinedAttachments.length, referencing the existing symbols combinedAttachments, visibleCombinedAttachments, remainingCombinedAttachments, remainingCombinedAttachmentsCount and importing useMemo from react.
235-281: Use a stable compound key for visible attachments.Keying the visible chip list by the loop
indexis functionally fine today but becomes fragile once removals and mixed kinds are involved — React may reuse a DOM node for an image chip as a document chip (or vice-versa) during reorders, causing brief visual glitches. The popover list already uses a compound key; mirror that here for consistency.♻️ Proposed change
- {visibleCombinedAttachments.map((attachment, index) => + {visibleCombinedAttachments.map((attachment) => attachment.kind === 'image' ? ( <div - key={index} + key={`image-${attachment.originalIndex}`} ... > ... </div> ) : ( <div - key={index} + key={`document-${attachment.originalIndex}`} ... > ... </div> ) )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/client/src/components/ChatBot.tsx` around lines 235 - 281, The visible attachments list uses the loop index as key which can cause DOM reuse glitches; update the key on the mapped elements in ChatBot.tsx (the mapped visibleCombinedAttachments in the JSX that renders image/document chips) to a stable compound key such as combining attachment.kind and attachment.originalIndex (e.g., `${attachment.kind}-${attachment.originalIndex}`) to match the popover list's approach and ensure consistent identity across removals and mixed kinds; keep the existing onClick handlers (handleRemoveImage / handleRemoveDocument) unchanged.
231-233: Container mixesflex-wrapwithoverflow-x-auto.Switching the outer wrapper to
flex-wrapmeans chips will wrap to new lines, sooverflow-x-autoon the same element effectively becomes dead styling (no horizontal overflow can occur). Either dropoverflow-x-autoor keep the previous non-wrapping layout with horizontal scroll — mixing them is confusing and can also cause unexpected vertical growth pushing the composer down when many chips are present.🧹 Suggested cleanup
- <div className="flex min-w-0 flex-1 flex-wrap items-center gap-2 overflow-x-auto"> + <div className="flex min-w-0 flex-1 flex-wrap items-center gap-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/client/src/components/ChatBot.tsx` around lines 231 - 233, The attachments container currently mixes "flex-wrap" with "overflow-x-auto" in the div that renders when (uploadedImages.length > 0 || uploadedDocuments.length > 0), which prevents horizontal scrolling and causes vertical growth; decide which behavior you want and fix the className accordingly — either remove "overflow-x-auto" to keep wrapping chips on multiple lines, or remove "flex-wrap" (and ensure "flex-nowrap" if needed) so the container stays single-line and horizontal scrolling via "overflow-x-auto" works; update the className on that div in ChatBot.tsx (the Scrollable Attachments Container rendering the uploadedImages/uploadedDocuments chips) to reflect the chosen option.wavefront/client/src/pages/apps/[appId]/workflows/[id].tsx (1)
729-729: Verify that overflow behavior works as intended.Adding
overflow-autoto this div may not achieve the intended scrolling behavior because the div lacks a height constraint (e.g.,flex-1ormax-h-*). Without such a constraint, the div will naturally size to fit its content (CodeMirror's 500px + text), andoverflow-autowon't trigger scrolling.Additionally, this creates nested scroll containers since the parent
DialogContentalready hasoverflow-y-autoandmax-h-[90vh](line 725), which can lead to confusing UX with multiple scrollbars.If the goal is to make the editor area scroll independently while keeping the header and footer fixed, consider adding
flex-1or an explicit height constraint to this div. Otherwise, theDialogContent's existing overflow already handles scrolling for the entire dialog.Please verify the rendered behavior to confirm whether this achieves the desired UX. You can test with YAML content that exceeds 500px in the editor to see which container scrolls.
🎨 Suggested layout pattern for independent content scrolling
If the intent is to keep header/footer fixed while only the editor area scrolls:
- <div className="flex flex-col gap-3 overflow-auto py-4"> + <div className="flex flex-1 flex-col gap-3 overflow-auto py-4">This requires the
DialogContentto use flex layout (which it likely does), allowing this div to take available space and scroll independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/client/src/pages/apps/`[appId]/workflows/[id].tsx at line 729, The div with class "flex flex-col gap-3 overflow-auto py-4" inside the DialogContent creates a nested scroll container without a height constraint so overflow-auto may never trigger; update that div (or DialogContent layout) to give it a height/flex constraint (e.g., add flex-1 or a max-h-* class) if you want the editor area (CodeMirror) to scroll independently, or remove overflow-auto to rely on DialogContent's existing overflow-y-auto/max-h-[90vh] for single-container scrolling; after changing, verify rendered behavior in the [appId]/workflows/[id].tsx dialog by loading YAML content that makes CodeMirror exceed 500px to confirm which container scrolls and that header/footer remain fixed as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flo_ai/flo_ai/llm/gemini_llm.py`:
- Around line 208-213: The get_message_content function currently returns None
for inputs that are neither str nor dict; change its signature from response:
Dict[str, Any] to response: Any and ensure it always returns a str by adding a
final fallback that returns str(response) (or '' if response is None) so callers
like agent.py (which call self.llm.get_message_content(...).strip().upper()) and
tests expecting __str__ behavior won't break; keep the existing handling for str
and dict (use response.get('content') -> str if present, else '') but ensure
every code path returns a string.
In `@wavefront/client/src/components/ChatBot.tsx`:
- Around line 293-294: In ChatBot.tsx, remove the stray "b" token from the
PopoverContent className (the JSX element using PopoverContent with className="b
flex flex-col gap-1.5 opacity-80"); update that className to only valid Tailwind
utilities (e.g., "flex flex-col gap-1.5 opacity-80") so there are no
invalid/typo classes left in the PopoverContent element.
---
Nitpick comments:
In `@wavefront/client/src/components/ChatBot.tsx`:
- Around line 110-116: Derive remainingCombinedAttachmentsCount from
remainingCombinedAttachments.length and memoize the combined attachments
derivation: wrap building combinedAttachments, visibleCombinedAttachments,
remainingCombinedAttachments (currently using combinedAttachments.slice(...)) in
a useMemo hook so they’re only recalculated when uploadedImages or
uploadedDocuments change; compute visibleCombinedAttachments =
combinedAttachments.slice(0,2), remainingCombinedAttachments =
combinedAttachments.slice(2) inside the memo and set
remainingCombinedAttachmentsCount = remainingCombinedAttachments.length,
referencing the existing symbols combinedAttachments,
visibleCombinedAttachments, remainingCombinedAttachments,
remainingCombinedAttachmentsCount and importing useMemo from react.
- Around line 235-281: The visible attachments list uses the loop index as key
which can cause DOM reuse glitches; update the key on the mapped elements in
ChatBot.tsx (the mapped visibleCombinedAttachments in the JSX that renders
image/document chips) to a stable compound key such as combining attachment.kind
and attachment.originalIndex (e.g.,
`${attachment.kind}-${attachment.originalIndex}`) to match the popover list's
approach and ensure consistent identity across removals and mixed kinds; keep
the existing onClick handlers (handleRemoveImage / handleRemoveDocument)
unchanged.
- Around line 231-233: The attachments container currently mixes "flex-wrap"
with "overflow-x-auto" in the div that renders when (uploadedImages.length > 0
|| uploadedDocuments.length > 0), which prevents horizontal scrolling and causes
vertical growth; decide which behavior you want and fix the className
accordingly — either remove "overflow-x-auto" to keep wrapping chips on multiple
lines, or remove "flex-wrap" (and ensure "flex-nowrap" if needed) so the
container stays single-line and horizontal scrolling via "overflow-x-auto"
works; update the className on that div in ChatBot.tsx (the Scrollable
Attachments Container rendering the uploadedImages/uploadedDocuments chips) to
reflect the chosen option.
In `@wavefront/client/src/pages/apps/`[appId]/workflows/[id].tsx:
- Line 729: The div with class "flex flex-col gap-3 overflow-auto py-4" inside
the DialogContent creates a nested scroll container without a height constraint
so overflow-auto may never trigger; update that div (or DialogContent layout) to
give it a height/flex constraint (e.g., add flex-1 or a max-h-* class) if you
want the editor area (CodeMirror) to scroll independently, or remove
overflow-auto to rely on DialogContent's existing overflow-y-auto/max-h-[90vh]
for single-container scrolling; after changing, verify rendered behavior in the
[appId]/workflows/[id].tsx dialog by loading YAML content that makes CodeMirror
exceed 500px to confirm which container scrolls and that header/footer remain
fixed as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7a18a2a-43fe-4ed5-bd51-cdf438248ba5
⛔ Files ignored due to path filters (1)
wavefront/server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
flo_ai/flo_ai/llm/gemini_llm.pywavefront/client/src/components/ChatBot.tsxwavefront/client/src/pages/apps/[appId]/workflows/[id].tsxwavefront/server/modules/agents_module/pyproject.tomlwavefront/server/modules/knowledge_base_module/pyproject.tomlwavefront/server/modules/tools_module/pyproject.toml
| def get_message_content(self, response: Dict[str, Any]) -> str: | ||
| if isinstance(response, str): | ||
| return response | ||
| if hasattr(response, 'content') and response.content is not None: | ||
| return str(response.content) | ||
| return '' | ||
| if isinstance(response, dict): | ||
| content = response.get('content') | ||
| return str(content) if content is not None else '' |
There was a problem hiding this comment.
Critical: missing fallback return — non-str/non-dict inputs now implicitly return None, breaking callers and existing tests.
Previously, get_message_content ended with return '' (or str(response.content) via hasattr), so non-str/non-dict inputs always produced a string. After this change, any input that is neither str nor dict falls through and returns None implicitly, which:
- Breaks
flo_ai/flo_ai/agent/agent.py:616, which chains.strip().upper()on the result (self.llm.get_message_content(analysis_response).strip().upper()). ANonereturn raisesAttributeError: 'NoneType' object has no attribute 'strip'. - Breaks the existing integration test
test_get_message_content_objectinflo_ai/tests/integration-tests/test_gemini_llm_real.py:252-261, which passes aMockObjectand expects'Mock object string'via__str__(). - Diverges from the consistent pattern in sibling LLMs (e.g.,
flo_ai/flo_ai/llm/anthropic_llm.py:217-221) which fall back tostr(response)for non-dict inputs.
Also note the declared type response: Dict[str, Any] is narrower than the actual accepted types — consider widening to Any to match the other LLM implementations and the stringly-typed MockObject case.
🐛 Proposed fix — restore `str(response)` fallback and align signature
- def get_message_content(self, response: Dict[str, Any]) -> str:
- if isinstance(response, str):
- return response
- if isinstance(response, dict):
- content = response.get('content')
- return str(content) if content is not None else ''
+ def get_message_content(self, response: Any) -> str:
+ if isinstance(response, str):
+ return response
+ if isinstance(response, dict):
+ return str(response.get('content', ''))
+ return str(response)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flo_ai/flo_ai/llm/gemini_llm.py` around lines 208 - 213, The
get_message_content function currently returns None for inputs that are neither
str nor dict; change its signature from response: Dict[str, Any] to response:
Any and ensure it always returns a str by adding a final fallback that returns
str(response) (or '' if response is None) so callers like agent.py (which call
self.llm.get_message_content(...).strip().upper()) and tests expecting __str__
behavior won't break; keep the existing handling for str and dict (use
response.get('content') -> str if present, else '') but ensure every code path
returns a string.
| <PopoverContent align="start" className="w-64 bg-gray-800 p-2"> | ||
| <div className="b flex flex-col gap-1.5 opacity-80"> |
There was a problem hiding this comment.
Stray b class in popover content.
className="b flex flex-col gap-1.5 opacity-80" contains a leftover b class that isn't a valid Tailwind utility and appears to be a typo.
🧹 Proposed fix
- <PopoverContent align="start" className="w-64 bg-gray-800 p-2">
- <div className="b flex flex-col gap-1.5 opacity-80">
+ <PopoverContent align="start" className="w-64 bg-gray-800 p-2">
+ <div className="flex flex-col gap-1.5 opacity-80">📝 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.
| <PopoverContent align="start" className="w-64 bg-gray-800 p-2"> | |
| <div className="b flex flex-col gap-1.5 opacity-80"> | |
| <PopoverContent align="start" className="w-64 bg-gray-800 p-2"> | |
| <div className="flex flex-col gap-1.5 opacity-80"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wavefront/client/src/components/ChatBot.tsx` around lines 293 - 294, In
ChatBot.tsx, remove the stray "b" token from the PopoverContent className (the
JSX element using PopoverContent with className="b flex flex-col gap-1.5
opacity-80"); update that className to only valid Tailwind utilities (e.g.,
"flex flex-col gap-1.5 opacity-80") so there are no invalid/typo classes left in
the PopoverContent element.
* chore: upgrade flo-ai and ui improvements * fix(flo-ai): fix gemini response
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores