feat: Modal updates #7306
Merged
feat: Modal updates #7306
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 31 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/refresh-components/Modal.tsx">
<violation number="1" location="web/src/refresh-components/Modal.tsx:489">
P2: The "Space-between layout" example in the JSDoc is now impossible to achieve. The new implementation hardcodes `justifyContent="end"`, and since `className` is removed via `WithoutStyles` and `justifyContent` isn't exposed as a prop, users cannot create a space-between layout. Consider either updating the example to reflect what's actually achievable, or adding a `justifyContent` prop to `ModalFooterProps`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Refactors Modal component to remove className overrides from all subcomponents (Header, Body, Footer, Overlay), consolidating styling into enum-based props like twoTone and size variants. Introduces WithoutStyles utility type to enforce type-safety by omitting className and style props. Updates 31 files across the codebase to use the new prop-based styling system instead of className overrides.
Confidence Score: 2/5
- Modal.Footer layout change will break UserFilesModal's space-between footer layout
- Modal.Footer now hardcodes
justifyContent="end"using Section component, which breaks UserFilesModal's footer that requires space-between layout (left-side file count controls and right-side Done button). The left-aligned content will not display correctly since Section enforces right-alignment. This affects a user-facing file selection modal. - web/src/components/modals/UserFilesModal.tsx requires immediate attention for broken footer layout; web/src/refresh-components/Modal.tsx may need justifyContent prop added to Modal.Footer
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| web/src/refresh-components/Modal.tsx | 4/5 | Removes className prop from Modal subcomponents, adds WithoutStyles type, introduces twoTone prop for Modal.Body, and provides sizeVariant context |
| web/src/types.ts | 5/5 | Adds new WithoutStyles utility type to enforce consistent styling by removing className and style props |
| web/src/components/modals/UserFilesModal.tsx | 5/5 | Removes className overrides and replaces useFilter hook with inline useMemo for search filtering |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Modal as Modal Component
participant Body as Modal.Body
participant Footer as Modal.Footer
participant Section as Section Component
Dev->>Modal: "Use Modal with className prop"
Modal-->>Dev: "❌ TypeScript error: className not allowed"
Dev->>Modal: "Use Modal.Body with twoTone prop"
Modal->>Body: "Apply twoTone=true"
Body->>Body: "Add bg-background-tint-01 class"
Body->>Section: "Wrap children in Section padding=1"
Body-->>Modal: "Styled body rendered"
Dev->>Modal: "Use Modal.Footer with children"
Modal->>Footer: "Render footer content"
Footer->>Section: "Apply justifyContent=end by default"
alt Footer needs space-between layout
Footer->>Section: "justifyContent hardcoded to end"
Section-->>Footer: "All items align right"
Footer-->>Dev: "⚠️ Layout broken for space-between"
else Footer needs default end layout
Footer->>Section: "justifyContent=end works"
Section-->>Footer: "Items align right correctly"
Footer-->>Dev: "✅ Layout correct"
end
Contributor
Additional Comments (4)
Consider one of these approaches:
The same issue may affect other modals that need non-standard footer layouts. Prompt To Fix With AIThis is a comment left during a code review.
Path: web/src/components/modals/UserFilesModal.tsx
Line: 272:298
Comment:
Modal.Footer now uses Section with `justifyContent="end"` by default, which doesn't support `justify-between` layout. The left-aligned content and right-aligned button need space-between justification, but Modal.Footer currently hardcodes `justifyContent="end"`.
Consider one of these approaches:
1. Pass custom `justifyContent` prop to Modal.Footer if it supports it
2. Wrap the footer content in a custom container with flex justify-between
3. Update Modal.Footer to accept a justifyContent prop for flexible layouts
The same issue may affect other modals that need non-standard footer layouts.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: web/src/components/modals/UserFilesModal.tsx
Line: 296:296
Comment:
The `className="ml-auto"` won't work as expected since Modal.Footer is now a Section with `justifyContent="end"`. With end justification, all items align to the right naturally, so ml-auto is unnecessary. However, the left-side content (lines 274-293) won't display on the left as intended.
How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: web/src/app/admin/connector/[ccPairId]/IndexAttemptErrorsModal.tsx
Line: 231:236
Comment:
The Button still has `className="ml-4 whitespace-nowrap"`. While the Modal.Body migration is complete, consider whether this button should be in Modal.Footer instead (which would eliminate the need for custom margin classes) or if the margin is intentional for spacing within the Body content.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: web/src/components/modals/UserFilesModal.tsx
Line: 284:284
Comment:
IconButton has a `className` override for conditional styling. Consider whether this should use a prop-based style variant instead to align with the PR's goal of removing className overrides.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/components/modals/UserFilesModal.tsx">
<violation number="1" location="web/src/components/modals/UserFilesModal.tsx:271">
P2: Inconsistent gap conversion: the original `gap-2` (0.5rem) was changed to `gap={2}` (2rem), which is 4x larger than intended. The Header section correctly uses `gap={0.5}` for the same original class.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Subash-Mohan
reviewed
Jan 9, 2026
Subash-Mohan
reviewed
Jan 9, 2026
Subash-Mohan
reviewed
Jan 9, 2026
Subash-Mohan
reviewed
Jan 9, 2026
Subash-Mohan
reviewed
Jan 9, 2026
Contributor
There was a problem hiding this comment.
6 issues found across 63 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/tools/tool_implementations/web_search/clients/exa_client.py">
<violation number="1">
P1: The comment says to skip partial/empty content entries, but the code unconditionally appends all results without filtering. Add a condition to skip entries with empty `full_content`.</violation>
</file>
<file name="backend/onyx/server/query_and_chat/session_loading.py">
<violation number="1">
P2: Hardcoded magic string 'Generation was stopped' creates fragile coupling with `process_message.py`. Consider extracting this to a shared constant or using a dedicated field on `ChatMessage` to track cancellation state, ensuring this detection doesn't silently break if the message text changes.</violation>
</file>
<file name="web/src/sections/actions/modals/AddOpenAPIActionModal.tsx">
<violation number="1" location="web/src/sections/actions/modals/AddOpenAPIActionModal.tsx:320">
P0: Using `useMemo` and `useEffect` inside Formik's render props callback violates the Rules of Hooks. Hooks must be called at the top level of a React component, not inside callbacks. This can cause unpredictable behavior or crashes.
Consider extracting the Formik children into a separate component (e.g., `AddOpenAPIActionFormContent`) where hooks can be called at the top level.</violation>
</file>
<file name="web/src/app/admin/connector/[ccPairId]/InlineFileManagement.tsx">
<violation number="1" location="web/src/app/admin/connector/[ccPairId]/InlineFileManagement.tsx:413">
P2: The `primary` prop was removed from the 'Confirm & Save' button. In a confirmation modal, the primary action button should typically have visual emphasis to distinguish it from the cancel button. Consider adding `primary` back to maintain consistent button hierarchy.</violation>
</file>
<file name="web/src/refresh-components/EmptyMessage.tsx">
<violation number="1" location="web/src/refresh-components/EmptyMessage.tsx:62">
P2: The description `Text` is rendered unconditionally, so when `description` is undefined, an empty span with styling/padding is added to the DOM. Consider conditionally rendering it:
```tsx
{description && (
<Text text03 secondaryBody>
{description}
</Text>
)}
```</violation>
</file>
<file name="web/src/app/chat/components/input/ChatInputBar.tsx">
<violation number="1">
P2: The truthy check `if (initialMessage)` prevents syncing empty strings. If `initialMessage` changes from a non-empty value to `""`, the message state won't be cleared. Consider using `if (initialMessage !== undefined)` or removing the condition entirely since `initialMessage` has a default value of `""`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/sections/actions/modals/AddOpenAPIActionModal.tsx">
<violation number="1" location="web/src/sections/actions/modals/AddOpenAPIActionModal.tsx:434">
P1: JSON parsing at the start of `handleSubmit` is not wrapped in try-catch. If the definition contains invalid JSON, `parseJsonWithTrailingCommas` will throw an unhandled exception. Consider wrapping the entire function body in a try-catch or moving the parsing inside the existing try blocks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
|
|
||
| const handleSubmit = async (values: OpenAPIActionFormValues) => { | ||
| const parsedDefinition = parseJsonWithTrailingCommas(values.definition); |
Contributor
There was a problem hiding this comment.
P1: JSON parsing at the start of handleSubmit is not wrapped in try-catch. If the definition contains invalid JSON, parseJsonWithTrailingCommas will throw an unhandled exception. Consider wrapping the entire function body in a try-catch or moving the parsing inside the existing try blocks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/sections/actions/modals/AddOpenAPIActionModal.tsx, line 434:
<comment>JSON parsing at the start of `handleSubmit` is not wrapped in try-catch. If the definition contains invalid JSON, `parseJsonWithTrailingCommas` will throw an unhandled exception. Consider wrapping the entire function body in a try-catch or moving the parsing inside the existing try blocks.</comment>
<file context>
@@ -212,15 +204,239 @@ export default function AddOpenAPIActionModal({
+ );
+
+ const handleSubmit = async (values: OpenAPIActionFormValues) => {
+ const parsedDefinition = parseJsonWithTrailingCommas(values.definition);
+ const derivedName = parsedDefinition?.info?.title;
+ const derivedDescription = parsedDefinition?.info?.description;
</file context>
Suggested change
| const parsedDefinition = parseJsonWithTrailingCommas(values.definition); | |
| let parsedDefinition; | |
| try { | |
| parsedDefinition = parseJsonWithTrailingCommas(values.definition); | |
| } catch (error) { | |
| console.error("Error parsing OpenAPI definition:", error); | |
| setPopup({ | |
| message: "Invalid JSON format in OpenAPI definition", | |
| type: "error", | |
| }); | |
| return; | |
| } |
✅ Addressed in 8a82875
jessicasingh7
pushed a commit
that referenced
this pull request
Jan 12, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR removes
classNameoverrides from every exported component inside ofModal.tsx. All styles are consolidated into specific enum selections.Summary by cubic
Unified the modal API by removing className overrides and introducing explicit props for layout and styling. This makes modals consistent and simpler to use, with built-in spacing, overflow, and backgrounds.
Refactors
Migration
Written for commit 8a82875. Summary will update on new commits.