fix: make minor changes to fix dark mode + fixed bottom mobile action bar for dao create flow#945
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds a new focusRing design token and CSS variable, applies it across many focus-visible styles, converts numerous interactive Flex wrappers to semantic buttons with ARIA attributes, improves PopUp keyboard handling and ariaHasPopup, extends DropdownSelect sizing API, and mounts a portal-based mobile FormNavButtons bar. Several small layout/style and accessibility tweaks across uploads, review, and auction UI. No breaking public API changes beyond added optional props and widened children typing. ChangesFocus Ring & Accessibility & Layout Migration
Sequence DiagramsequenceDiagram
participant User
participant UI as Components
participant Theme as ZordTheme
participant DOM as Browser
User->>UI: Tab / focus element
UI->>Theme: read focusRing token
Theme-->>UI: returns focusRing (`#0085FF`)
UI->>DOM: render focus-visible outline using focusRing
DOM-->>User: show focus ring
User->>UI: press Enter/Space on PopUp trigger
UI->>UI: onKeyDown toggles openState
UI->>DOM: render menu with `chainPopUpItem` buttons (disabled/aria-current)
User->>UI: click chain option
UI->>UI: update selection state / aria attributes
User->>UI: click Upload button
UI->>DOM: fileInputRef.click()
DOM-->>User: browser file picker opens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/SingleImageUpload/SingleImageUpload.tsx (1)
78-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
<input type="file">nested inside<button>is invalid HTML per spec.The WHATWG HTML spec requires that
<button>content must be "phrasing content, but there must be no interactive content descendant."<input>,<select>,<textarea>, and other interactive elements are explicitly forbidden as descendants of<button>. Some browsers (Edge/Chrome/Safari) parse this permissively, while others (Firefox) enforce the spec restriction.Since
fileInputRefis already set up to programmatically trigger the input via.click(), the input doesn't need to be inside the button — move it to be a sibling within<Stack>:🐛 Proposed fix — move `` outside the button wrapper
{!isUploading && isMounted && !value && ( <> <Flex color="text4" mb={'x2'} style={{ textAlign: 'center' }}> {inputLabel} </Flex> <Flex fontWeight={'display'} style={{ textAlign: 'center' }}> {helperText} </Flex> </> )} - - <input - className={defaultUploadStyle} - id={`${id}-file-upload`} - data-testid="file-upload" - name="file" - type="file" - ref={fileInputRef} - multiple={false} - onChange={(event) => { - handleFileUpload(event.currentTarget.files) - }} - /> </Flex> + <input + className={defaultUploadStyle} + id={`${id}-file-upload`} + data-testid="file-upload" + name="file" + type="file" + ref={fileInputRef} + multiple={false} + onChange={(event) => { + handleFileUpload(event.currentTarget.files) + }} + /> + {uploadArtworkError && (🤖 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 `@packages/ui/src/SingleImageUpload/SingleImageUpload.tsx` around lines 78 - 126, The file input is currently nested inside the Flex that is rendered as a button (the clickable upload wrapper), which is invalid HTML; move the <input> rendered with id `${id}-file-upload` and className `defaultUploadStyle` out of the Flex/button and render it as a sibling (e.g., directly inside the parent <Stack>) so the Flex remains a non-nested interactive button; keep the same ref `fileInputRef`, the same onChange handler `handleFileUpload(event.currentTarget.files)`, type="file", multiple={false}, data-testid, name, and aria attributes, and keep the Flex onClick calling `fileInputRef.current?.click()` so behavior and styling are preserved while avoiding interactive content inside a button.
🧹 Nitpick comments (8)
packages/zord/src/components/PopUp.tsx (1)
98-128: ⚡ Quick win
role="button"on a wrapper that contains an interactive child creates nested interactive elements — an ARIA violation.The ARIA content model for
role="button"disallows interactive descendants. In the default case (notriggerprop), the rendered HTML is effectively:<div role="button" tabindex="0"> <button>...</button> <!-- ← also focusable, also clickable --> </div>This produces two tab stops for one control and is rejected by ARIA validators. The inner
<Button>already handles keyboard activation (Enter/Space naturally fires its click → bubbles to the outeronClick), so the outerrole="button"/tabIndex={0}/onKeyDownare redundant for the default case and harmful for the custom-triggercase when callers pass a button.The minimal fix that avoids the duplication while retaining the keyboard improvement for non-interactive triggers is to render the outer wrapper as a native
<button>via theasprop:♻️ Suggested approach
- <Box - role="button" - tabIndex={0} - aria-expanded={openState} - aria-haspopup="menu" - onClick={(e: React.MouseEvent) => { - e.stopPropagation() - setOpenState(!openState) - }} - onKeyDown={(e: React.KeyboardEvent) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - e.stopPropagation() - setOpenState((prev) => !prev) - } - }} - ref={setTriggerElement} - className={[triggerClassName]} - > + <Box + as="button" + type="button" + aria-expanded={openState} + aria-haspopup="menu" + onClick={(e: React.MouseEvent) => { + e.stopPropagation() + setOpenState(!openState) + }} + ref={setTriggerElement} + className={[triggerClassName]} + style={{ background: 'none', border: 'none', padding: 0, cursor: 'pointer' }} + >When rendered as a native
<button>, keyboard activation (Enter/Space) is handled by the browser natively, andtabIndexis automatic. The default innerButtonchild should then be removed in favour of the button-element wrapper itself holding the icon — or the default trigger slot can be reorganised so there is no longer a nested button.🤖 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 `@packages/zord/src/components/PopUp.tsx` around lines 98 - 128, The wrapper Box that renders the trigger currently uses role="button", tabIndex and onKeyDown while sometimes rendering an interactive child (the default Button or a user-supplied button), causing nested interactive elements; update the PopUp trigger render so the wrapper is a native button element when it should act as the trigger (use Box's as="button" or render a native <button>), remove role="button", tabIndex and custom onKeyDown in that branch, and ensure the default case no longer nests a Button inside the wrapper (move the Icon into the wrapper/button or render the default trigger as the wrapper itself); keep setTriggerElement, onClick that toggles setOpenState, aria-expanded and aria-haspopup attributes on the wrapper/button so keyboard activation and focus behave correctly without duplicate tab stops.apps/web/src/styles/globals.css (1)
47-47: 💤 Low valueNote:
--focus-ring-colorandvars.color.focusRingare separate systems with the same hardcoded value.Both are
#0085ff, but they're maintained independently — one inglobals.css(used byreact-mde-theme.css, etc.) and the other in the vanilla-extract token system (used by all TS CSS modules). A future color change will need to update both locations.This is an acceptable trade-off since vanilla-extract tokens compile at build time and cannot be referenced from plain CSS, but a co-location comment would help.
Also applies to: 74-74
🤖 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 `@apps/web/src/styles/globals.css` at line 47, Two separate focus ring color sources (--focus-ring-color in globals.css and vars.color.focusRing in the vanilla-extract tokens) are duplicated; add a co-location comment next to the CSS variable (--focus-ring-color) indicating that vars.color.focusRing holds the canonical/tokenized value and must be updated in sync (and vice versa in the token definition if possible), and include a brief note about why they are separate (vanilla-extract compiles at build time) so future maintainers know to change both places; reference the identifiers --focus-ring-color and vars.color.focusRing when adding the comments.apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx (2)
188-191: 💤 Low valueHard-coded
borderRadius: '8px'overrides theborderRadius="normal"prop.
borderRadius="normal"already maps to a theme token; layering an inlinestyle={{ borderRadius: '8px' }}on top defeats that and bypasses the theme. If a specific value is required, prefer a theme token (e.g.vars.radii.normal/curved) or move it intowrongNetworkButton.css.🤖 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 `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx` around lines 188 - 191, In ChainMenu.tsx remove the inline style that sets style={{ borderRadius: '8px' }} (which overrides the component prop) and instead rely on the existing borderRadius="normal" prop or apply the theme token (e.g. vars.radii.normal or vars.radii.curved) via the component's props or the wrongNetworkButton.css stylesheet; update the JSX where align/justify are set (inside the ChainMenu component) to use the prop or CSS class so the theme token is used consistently rather than a hard-coded pixel value.
215-217: 💤 Low valueMove button-reset styles into the CSS class.
Inline
style={{ border: 0, width: '100%', textAlign: 'left' }}resets the user-agent button defaults at every render and is easy to drift from the desktop/mobile counterparts. SincechainPopUpButtonis already applied here, fold these resets (plusbackground: 'transparent',font: inherit, etc.) into that class so all chain entries share the same baseline regardless ofas.🤖 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 `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx` around lines 215 - 217, The inline reset styles on the chain entry button (style={{ border: 0, width: '100%', textAlign: 'left' }}) should be moved into the existing CSS class (chainPopUpButton) so all chain entries share the same baseline; update the stylesheet where chainPopUpButton is defined to include resets like border: 0, width: 100%, text-align: left, background: transparent, font: inherit, padding/appearance resets as appropriate for mobile/desktop parity, then remove the style prop from the element in ChainMenu.tsx (the element that has disabled, aria-current and isSelectedChain checks) so styling is driven by the class only.packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx (2)
40-46: 💤 Low value
isMountedgating for portal is correct, but consideruseSyncExternalStoreor the layout-effect pattern.The current pattern (
useState(false)+useEffect(() => setIsMounted(true), [])) intentionally produces a no-op first client render to match SSR — that's fine. Just be aware this means the mobile bar flashes in one frame after hydration. If you ever notice a visible delay on slow devices, switching touseSyncExternalStore(subscribe, () => true, () => false)avoids the extra render. Not blocking.🤖 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 `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx` around lines 40 - 46, The current mount gating uses isMounted/setIsMounted with React.useEffect which intentionally delays the client render and can cause a one-frame mobile bar flash; replace this pattern in FormNavButtons by either switching the effect to useLayoutEffect (useLayoutEffect(() => setIsMounted(true), [])) to avoid the visual flash, or implement useSyncExternalStore with a subscribe/no-op subscriber and a server snapshot of false and client snapshot of true (useSyncExternalStore(subscribe, () => true, () => false)) so hydration matches and eliminates the extra render; update references to isMounted and setIsMounted accordingly and keep resetForm usage unchanged.
77-191: ⚡ Quick winDesktop and mobile button rows are near-duplicates — extract a shared sub-component.
The reset / back / continue blocks are repeated almost verbatim between
formNavDesktopRowand the portal-renderedformNavMobileBar. The two paths already disagree subtly (mobile continue is alwaystype="button"and useshandleMobileContinue, desktop honorsisSubmitand addsonMouseDownpreventDefault), and that drift will only get worse. A smallNavRowhelper that takes avariant: 'desktop' | 'mobile'(or just anonContinue/continueTypepair) would make the difference explicit and prevent one side getting an a11y/disabled fix the other doesn't.🤖 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 `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx` around lines 77 - 191, Extract the duplicated button row into a new presentational component (e.g., NavRow) and replace both the desktop JSX inside formNavDesktopRow and the portal JSX inside formNavMobileBar with it; NavRow should accept props: variant?: 'desktop'|'mobile' or explicit onContinue: () => void, continueType: 'submit'|'button', continueOnMouseDownPreventDefault?: boolean, and the existing flags/handlers (showReset, setIsModalOpen, hasPrev, onPrev, children, nextDisabled, nextLabel) and use classNames formNavResetButton and formNavContinueButton; implement desktop by passing onContinue={isSubmit ? undefined : onNext}, continueType={isSubmit ? 'submit' : 'button'}, continueOnMouseDownPreventDefault={isSubmit}, and mobile by passing onContinue={handleMobileContinue}, continueType='button' so the subtle behavior differences (type and onMouseDown) remain explicit and centralized.packages/zord/src/elements/Button.css.ts (1)
111-204: 💤 Low valueDisabled-state text colors are inconsistent across variants.
The new
[disabled]blocks mix two conventions for the foreground color:
primary,positive,destructiveuse the full-strengthonAccent/onPositive/onNegative.secondary,secondaryAccent,outline,ghostuse the dimmedonNeutralDisabled/onGhostDisabled.If the intent is full-contrast labels on color-tinted disabled backgrounds, this is fine — but in that case
onAccentDisabled/onPositiveDisabled/onNegativeDisabled(which are explicitly defined incolor-theme.ts) are unused. Worth confirming this is deliberate; otherwise the tokens that were added to the theme will silently rot.🤖 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 `@packages/zord/src/elements/Button.css.ts` around lines 111 - 204, Disabled-state text colors are inconsistent across Button variants: the primary/positive/destructive styles use full-strength tokens (e.g., onAccent, onPositive, onNegative) while secondary/secondaryAccent/outline/ghost use dimmed tokens (onNeutralDisabled/onGhostDisabled), leaving the explicit onAccentDisabled/onPositiveDisabled/onNegativeDisabled tokens unused; update the `[disabled]` selectors in the Button.css.ts variant styles (primary, positive, destructive, secondary, secondaryAccent, outline, ghost) to consistently reference the correct disabled foreground tokens from color-theme.ts (either switch the dimmed variants to their specific disabled tokens like onAccentDisabled/onPositiveDisabled/onNegativeDisabled or swap the full-strength ones to the neutral disabled tokens) so all variants use the intended disabled color convention.packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsx (1)
475-487: 💤 Low valueMove the button reset (
borderWidth: 0, padding: 0) intodeployCheckboxStyleVariants.The same inline
style={{ borderWidth: 0, padding: 0 }}is repeated on all four toggle Flex-buttons (terms, chain, rewards, fast-DAO). Folding it into the existingdeployCheckboxStyleVariantsclasses (alongsidebackground: 'transparent',font: 'inherit', etc.) keeps the JSX clean and ensures every confirmation box stays visually identical when one is updated.🤖 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 `@packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsx` around lines 475 - 487, The inline style { borderWidth: 0, padding: 0 } used on the toggle button should be removed from the JSX and folded into deployCheckboxStyleVariants so both the 'default' and 'confirmed' variant objects include borderWidth: 0 (or border: 'none') and padding: 0 alongside the existing background/font rules; update the JSX for the four toggle buttons (the one using hasConfirmedTerms/setHasConfirmedTerms and the similar chain/rewards/fast-DAO toggle components) to drop the style prop so they inherit the reset from deployCheckboxStyleVariants and remain visually consistent.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@packages/create-dao-ui/src/components/AuctionSettingsForm/AuctionSettingsForm.tsx`:
- Around line 207-221: The inline style on the Flex button is removing the class
border (style={{ borderWidth: 0 }}), making the unchecked toggle invisible;
remove the inline borderWidth override from the Flex used for the Fast DAO
toggle (the element using deployCheckboxStyleVariants and props enableFastDAO,
onClick handleFastDAOToggle) and instead add padding: 0 to the shared
deployCheckboxStyle (or create a base deployCheckboxStyleVariants['button']) in
ReviewAndDeploy.css.ts so the UA padding is removed but the class border
(border: 1px solid ${vars.color.text1}) remains.
In `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx`:
- Around line 53-60: handleMobileContinue is using
document.querySelector('form') which can target the wrong form when the mobile
bar is portaled; change the component API to accept a formId (or formRef) prop
and update handleMobileContinue to call
document.getElementById(formId)?.requestSubmit() (or
formRef.current?.requestSubmit()) when isSubmit is true, keeping the existing
onNext() behavior when isSubmit is false; ensure the prop is passed through
where FormNavButtons is rendered and update any callers to provide the
create-DAO form's id or ref so the continue button triggers the correct form
submission.
In `@packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsx`:
- Line 493: Update the awkward helper string in the ReviewAndDeploy component:
replace the current text "I have reviewed and acknowledge and agree to the" with
a clearer, consistent-tense phrase such as "I have reviewed, acknowledge, and
agree to the" (locate the JSX string in the ReviewAndDeploy component within
ReviewAndDeploy.tsx and update the text node).
In
`@packages/create-proposal-ui/src/components/TransactionForm/Migration/PauseAuctionsForm.tsx`:
- Around line 107-120: The inline style on the button in PauseAuctionsForm (the
Flex used as button with className={checkboxStyleVariants[reduceDelay ?
'confirmed' : 'default']}) is zeroing out the CSS border via style={{
borderWidth: 0, padding: 0 }}; remove the borderWidth: 0 from that inline style
and instead add the required padding rules into the corresponding CSS class in
ReplaceArtworkForm.css (the variants referenced by checkboxStyleVariants) so the
unchecked border from the stylesheet remains visible; keep the onClick handler
(setReduceDelay) and aria attributes unchanged.
In `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx`:
- Around line 107-116: The file input is currently nested inside the interactive
button in SingleMediaUpload which is invalid HTML; move the <input type="file">
element out of the button wrapper (so it is a sibling, not a child) while
keeping the button's onClick handler that calls fileInputRef.current?.click(),
retain the fileInputRef, onChange handler, accept/multiple props and
aria-related props (e.g., inputLabel) on the input, and keep the button using
className singleMediaUploadWrapper and aria-label derived from inputLabel to
preserve accessibility and behavior.
In `@packages/zord/src/components/PopUp.tsx`:
- Line 102: The PopUp component currently hardcodes aria-haspopup="menu", which
is semantically incorrect for generic popups; update PopUp to accept an optional
prop (e.g., ariaHasPopup or popupType) via the BasePopUpProps interface and use
that prop to set aria-haspopup (with a sensible default such as "dialog" or
undefined), and update any internal usage in the PopUp implementation to read
that prop instead of the hardcoded "menu" so authors can specify "menu",
"listbox", "dialog", or omit it as appropriate.
---
Outside diff comments:
In `@packages/ui/src/SingleImageUpload/SingleImageUpload.tsx`:
- Around line 78-126: The file input is currently nested inside the Flex that is
rendered as a button (the clickable upload wrapper), which is invalid HTML; move
the <input> rendered with id `${id}-file-upload` and className
`defaultUploadStyle` out of the Flex/button and render it as a sibling (e.g.,
directly inside the parent <Stack>) so the Flex remains a non-nested interactive
button; keep the same ref `fileInputRef`, the same onChange handler
`handleFileUpload(event.currentTarget.files)`, type="file", multiple={false},
data-testid, name, and aria attributes, and keep the Flex onClick calling
`fileInputRef.current?.click()` so behavior and styling are preserved while
avoiding interactive content inside a button.
---
Nitpick comments:
In `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx`:
- Around line 188-191: In ChainMenu.tsx remove the inline style that sets
style={{ borderRadius: '8px' }} (which overrides the component prop) and instead
rely on the existing borderRadius="normal" prop or apply the theme token (e.g.
vars.radii.normal or vars.radii.curved) via the component's props or the
wrongNetworkButton.css stylesheet; update the JSX where align/justify are set
(inside the ChainMenu component) to use the prop or CSS class so the theme token
is used consistently rather than a hard-coded pixel value.
- Around line 215-217: The inline reset styles on the chain entry button
(style={{ border: 0, width: '100%', textAlign: 'left' }}) should be moved into
the existing CSS class (chainPopUpButton) so all chain entries share the same
baseline; update the stylesheet where chainPopUpButton is defined to include
resets like border: 0, width: 100%, text-align: left, background: transparent,
font: inherit, padding/appearance resets as appropriate for mobile/desktop
parity, then remove the style prop from the element in ChainMenu.tsx (the
element that has disabled, aria-current and isSelectedChain checks) so styling
is driven by the class only.
In `@apps/web/src/styles/globals.css`:
- Line 47: Two separate focus ring color sources (--focus-ring-color in
globals.css and vars.color.focusRing in the vanilla-extract tokens) are
duplicated; add a co-location comment next to the CSS variable
(--focus-ring-color) indicating that vars.color.focusRing holds the
canonical/tokenized value and must be updated in sync (and vice versa in the
token definition if possible), and include a brief note about why they are
separate (vanilla-extract compiles at build time) so future maintainers know to
change both places; reference the identifiers --focus-ring-color and
vars.color.focusRing when adding the comments.
In `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx`:
- Around line 40-46: The current mount gating uses isMounted/setIsMounted with
React.useEffect which intentionally delays the client render and can cause a
one-frame mobile bar flash; replace this pattern in FormNavButtons by either
switching the effect to useLayoutEffect (useLayoutEffect(() =>
setIsMounted(true), [])) to avoid the visual flash, or implement
useSyncExternalStore with a subscribe/no-op subscriber and a server snapshot of
false and client snapshot of true (useSyncExternalStore(subscribe, () => true,
() => false)) so hydration matches and eliminates the extra render; update
references to isMounted and setIsMounted accordingly and keep resetForm usage
unchanged.
- Around line 77-191: Extract the duplicated button row into a new
presentational component (e.g., NavRow) and replace both the desktop JSX inside
formNavDesktopRow and the portal JSX inside formNavMobileBar with it; NavRow
should accept props: variant?: 'desktop'|'mobile' or explicit onContinue: () =>
void, continueType: 'submit'|'button', continueOnMouseDownPreventDefault?:
boolean, and the existing flags/handlers (showReset, setIsModalOpen, hasPrev,
onPrev, children, nextDisabled, nextLabel) and use classNames formNavResetButton
and formNavContinueButton; implement desktop by passing onContinue={isSubmit ?
undefined : onNext}, continueType={isSubmit ? 'submit' : 'button'},
continueOnMouseDownPreventDefault={isSubmit}, and mobile by passing
onContinue={handleMobileContinue}, continueType='button' so the subtle behavior
differences (type and onMouseDown) remain explicit and centralized.
In `@packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsx`:
- Around line 475-487: The inline style { borderWidth: 0, padding: 0 } used on
the toggle button should be removed from the JSX and folded into
deployCheckboxStyleVariants so both the 'default' and 'confirmed' variant
objects include borderWidth: 0 (or border: 'none') and padding: 0 alongside the
existing background/font rules; update the JSX for the four toggle buttons (the
one using hasConfirmedTerms/setHasConfirmedTerms and the similar
chain/rewards/fast-DAO toggle components) to drop the style prop so they inherit
the reset from deployCheckboxStyleVariants and remain visually consistent.
In `@packages/zord/src/components/PopUp.tsx`:
- Around line 98-128: The wrapper Box that renders the trigger currently uses
role="button", tabIndex and onKeyDown while sometimes rendering an interactive
child (the default Button or a user-supplied button), causing nested interactive
elements; update the PopUp trigger render so the wrapper is a native button
element when it should act as the trigger (use Box's as="button" or render a
native <button>), remove role="button", tabIndex and custom onKeyDown in that
branch, and ensure the default case no longer nests a Button inside the wrapper
(move the Icon into the wrapper/button or render the default trigger as the
wrapper itself); keep setTriggerElement, onClick that toggles setOpenState,
aria-expanded and aria-haspopup attributes on the wrapper/button so keyboard
activation and focus behave correctly without duplicate tab stops.
In `@packages/zord/src/elements/Button.css.ts`:
- Around line 111-204: Disabled-state text colors are inconsistent across Button
variants: the primary/positive/destructive styles use full-strength tokens
(e.g., onAccent, onPositive, onNegative) while
secondary/secondaryAccent/outline/ghost use dimmed tokens
(onNeutralDisabled/onGhostDisabled), leaving the explicit
onAccentDisabled/onPositiveDisabled/onNegativeDisabled tokens unused; update the
`[disabled]` selectors in the Button.css.ts variant styles (primary, positive,
destructive, secondary, secondaryAccent, outline, ghost) to consistently
reference the correct disabled foreground tokens from color-theme.ts (either
switch the dimmed variants to their specific disabled tokens like
onAccentDisabled/onPositiveDisabled/onNegativeDisabled or swap the full-strength
ones to the neutral disabled tokens) so all variants use the intended disabled
color convention.
🪄 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: 456ef081-d79e-4e82-b820-c36537f6e35f
📒 Files selected for processing (31)
apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsxapps/web/src/modules/explore/SearchInput.css.tsapps/web/src/modules/playground/components/PlaygroundLanding.css.tsapps/web/src/pages/create.tsxapps/web/src/styles/flatpickr-theme.cssapps/web/src/styles/globals.cssapps/web/src/styles/react-mde-theme.csspackages/create-dao-ui/src/components/AuctionSettingsForm/AuctionSettingsForm.tsxpackages/create-dao-ui/src/components/CreateNavigation/NavSection.css.tspackages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.css.tspackages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsxpackages/create-proposal-ui/src/components/TransactionForm/AddArtwork/AddArtworkForm.tsxpackages/create-proposal-ui/src/components/TransactionForm/Migration/PauseAuctionsForm.tsxpackages/create-proposal-ui/src/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.tsxpackages/dao-ui/src/components/DaoCard/DaoCard.css.tspackages/feed-ui/src/SearchInput.css.tspackages/ui/src/Artwork/Artwork.css.tspackages/ui/src/Fields/Radio.tsxpackages/ui/src/Fields/StickySave.tsxpackages/ui/src/Fields/styles.css.tspackages/ui/src/SingleImageUpload/SingleImageUpload.css.tspackages/ui/src/SingleImageUpload/SingleImageUpload.tsxpackages/ui/src/SingleMediaUpload/SingleMediaUpload.css.tspackages/ui/src/SingleMediaUpload/SingleMediaUpload.tsxpackages/ui/src/WalletProfilePreview/WalletProfilePreview.css.tspackages/zord/src/components/PopUp.tsxpackages/zord/src/elements/Button.css.tspackages/zord/src/theme.css.tspackages/zord/src/tokens/color.tspackages/zord/src/utils/color-theme.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/zord/src/components/PopUp.tsx (1)
100-130:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNested interactive elements:
role="button"wrapper containing a<Button>child violates ARIA-in-HTML.The
Boxreceivesrole="button"andtabIndex={0}, making it an interactive element. But when notriggerprop is supplied, it renders the default<Button>as a direct child — producing<div role="button"><button>…</button></div>. This is prohibited by the ARIA-in-HTML spec: an element withrole="button"must not contain focusable/interactive descendants. Screen readers and assistive tools expose two overlapping interactive targets for a single control.Additionally, when consumers pass their own
trigger(e.g., a<Button>or<a>) the same nesting problem arises.The cleanest fix is to not put
role="button"on the outer wrapper (which serves only as a positioning anchor for Popper) and instead move the keyboard and ARIA attributes onto the actual interactive element:♻️ Proposed fix
<Box - role="button" - tabIndex={0} - aria-expanded={openState} - aria-haspopup={ariaHasPopup} onClick={(e: React.MouseEvent) => { e.stopPropagation() - setOpenState(!openState) + setOpenState((prev) => !prev) }} - onKeyDown={(e: React.KeyboardEvent) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - e.stopPropagation() - setOpenState((prev) => !prev) - } - }} ref={setTriggerElement} className={[triggerClassName]} > {trigger || ( <Button variant="ghost" size="sm" borderRadius="round" p="x3" style={{ minWidth: 0, height: 'auto' }} + aria-expanded={openState} + aria-haspopup={ariaHasPopup} + onClick={(e) => { + e.stopPropagation() + setOpenState((prev) => !prev) + }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + e.stopPropagation() + setOpenState((prev) => !prev) + } + }} > <Icon id="dots" size="md" /> </Button> )} </Box>For the case where a custom
triggeris passed, consumers should own the ARIA/keyboard attributes on their trigger element, or the wrapper could clone the trigger with injected props — but that is a separate design decision.🤖 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 `@packages/zord/src/components/PopUp.tsx` around lines 100 - 130, The wrapper Box in PopUp.tsx currently has role="button" and tabIndex={0} while also rendering an interactive child (the default <Button> or a consumer-supplied trigger), violating ARIA-in-HTML; remove role="button", tabIndex and the keyboard/ARIA handlers from the Box and instead attach aria-expanded, aria-haspopup, onClick and onKeyDown to the actual trigger element: for the default trigger, move these props onto the <Button> used there, and for a custom trigger, cloneElement(trigger, { aria-expanded, aria-haspopup, onClick, onKeyDown, ref: setTriggerElement }) so consumers' interactive elements receive the attributes (keep the Box as a non-interactive positioning anchor and keep setTriggerElement ref on the interactive trigger when cloning).
🧹 Nitpick comments (3)
packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx (1)
106-106: ⚡ Quick winAssociate the visible label with the file input via
htmlFor.The
<label>at line 106 has nohtmlFor, so it is not programmatically linked to any control. The new<input>at line 156 now carries a stableid={file-upload-${id}}. AddinghtmlForwould make the label text a proper click target that opens the file picker — conforming to standard HTML semantics and improving assistive-technology support at zero cost.♻️ Proposed refactor
- <label className={defaultInputLabelStyle}>{inputLabel}</label> + <label htmlFor={`file-upload-${id}`} className={defaultInputLabelStyle}>{inputLabel}</label>🤖 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 `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx` at line 106, The label element using defaultInputLabelStyle with text inputLabel isn't programmatically linked to the file input; update the label to include htmlFor matching the file input's id (id={`file-upload-${id}`}) in the SingleMediaUpload component so clicking the label opens the file picker and improves accessibility.packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx (2)
84-198: 💤 Low valueDesktop and mobile rows duplicate the same three buttons.
The reset, back, and continue (children-fallback) buttons are repeated almost verbatim in both layouts. A drift between branches (e.g. updating an
aria-label, button height, or click handler) is easy to miss. Consider extracting<NavResetButton />,<NavBackButton />, and<NavContinueButton onContinue={...} />helpers (or a single<NavRow />that renders all three) and reusing them in both the desktop wrapper and the portal so the two paths cannot diverge.🤖 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 `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx` around lines 84 - 198, The desktop and mobile render paths in FormNavButtons duplicate the same three controls (reset, back, continue) causing maintenance drift; extract reusable components (e.g., NavResetButton, NavBackButton, NavContinueButton or a single NavRow) that encapsulate the existing behaviors and props (use FormNavButtons' setIsModalOpen for reset, onPrev for back, onNext/handleMobileContinue and nextDisabled/nextLabel/isSubmit/children for continue) and replace both the desktop block and the createPortal mobile block with the shared component(s) so attributes like aria-label, className (formNavResetButton/formNavContinueButton), sizes, and handlers remain in one place.
45-47: 💤 Low valuePrefer
useEffectoveruseLayoutEffectfor the mount guard.
useLayoutEffectruns synchronously and blocks paint on the client, which is unnecessary for this pattern. Since the only purpose is to set a flag after hydration (no layout measurement or DOM reads),useEffectaccomplishes the same thing without blocking paint and is the standard idiom for mount guards.♻️ Proposed change
-import React, { useState } from 'react' +import React, { useEffect, useState } from 'react' @@ - React.useLayoutEffect(() => { - setIsMounted(true) - }, []) + useEffect(() => { + setIsMounted(true) + }, [])🤖 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 `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx` around lines 45 - 47, The component currently uses React.useLayoutEffect to set the mount guard (React.useLayoutEffect(() => { setIsMounted(true) }, [])), which blocks paint unnecessarily; replace that hook with React.useEffect so setIsMounted(true) runs after paint/hydration instead of synchronously. Locate the useLayoutEffect call in the FormNavButtons component and swap it to useEffect (keeping the same effect body and dependency array) to preserve behavior without blocking rendering.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/ui/src/Fields/Radio.tsx`:
- Around line 31-33: The Radio component currently uses toggle-button semantics
(aria-pressed) for each option inside the options.map rendering; change the
container Flex to have role="radiogroup" and each option element to use
role="radio" and aria-checked={isSelected} instead of aria-pressed, ensuring the
selection state (e.g., selected value variable used in Radio) drives
aria-checked; also update any keyboard handling or onClick handlers tied to
aria-pressed to treat options as radios (space/arrow behavior) if present so
assistive tech receives correct radio semantics.
In `@packages/ui/src/SingleImageUpload/SingleImageUpload.tsx`:
- Around line 115-126: The file input in SingleImageUpload currently lacks an
accept attribute which would prevent users from selecting invalid file types;
update the <input> element (the one using defaultUploadStyle, id
`${id}-file-upload`, ref fileInputRef and onChange that calls handleFileUpload)
to include an appropriate accept value (e.g., the allowed image MIME types or
extensions your validation expects like "image/*" or a specific list such as
"image/png,image/jpeg,image/webp") so the picker filters invalid files earlier
while keeping your existing MIME-type validation intact.
In `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx`:
- Around line 154-165: The file input is missing the accept attribute even
though acceptableMIME is computed; update the <input> rendered in
SingleMediaUpload (the element with id `file-upload-${id}`, ref `fileInputRef`,
name "file" and onChange that calls handleFileUpload) to include
accept={acceptableMIME} (or an empty string if acceptableMIME is falsy) so the
OS file picker pre-filters allowed types while keeping handleFileUpload as the
validation backstop.
In `@packages/zord/src/components/PopUp.tsx`:
- Around line 105-108: The onClick handler uses setOpenState(!openState) which
can close over a stale openState value; change it to the functional updater form
setOpenState(prev => !prev) to match the onKeyDown pattern and avoid race
conditions—update the onClick arrow handler that calls setOpenState and ensure
it calls the functional updater instead of using the closed-over openState.
---
Outside diff comments:
In `@packages/zord/src/components/PopUp.tsx`:
- Around line 100-130: The wrapper Box in PopUp.tsx currently has role="button"
and tabIndex={0} while also rendering an interactive child (the default <Button>
or a consumer-supplied trigger), violating ARIA-in-HTML; remove role="button",
tabIndex and the keyboard/ARIA handlers from the Box and instead attach
aria-expanded, aria-haspopup, onClick and onKeyDown to the actual trigger
element: for the default trigger, move these props onto the <Button> used there,
and for a custom trigger, cloneElement(trigger, { aria-expanded, aria-haspopup,
onClick, onKeyDown, ref: setTriggerElement }) so consumers' interactive elements
receive the attributes (keep the Box as a non-interactive positioning anchor and
keep setTriggerElement ref on the interactive trigger when cloning).
---
Nitpick comments:
In `@packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx`:
- Around line 84-198: The desktop and mobile render paths in FormNavButtons
duplicate the same three controls (reset, back, continue) causing maintenance
drift; extract reusable components (e.g., NavResetButton, NavBackButton,
NavContinueButton or a single NavRow) that encapsulate the existing behaviors
and props (use FormNavButtons' setIsModalOpen for reset, onPrev for back,
onNext/handleMobileContinue and nextDisabled/nextLabel/isSubmit/children for
continue) and replace both the desktop block and the createPortal mobile block
with the shared component(s) so attributes like aria-label, className
(formNavResetButton/formNavContinueButton), sizes, and handlers remain in one
place.
- Around line 45-47: The component currently uses React.useLayoutEffect to set
the mount guard (React.useLayoutEffect(() => { setIsMounted(true) }, [])), which
blocks paint unnecessarily; replace that hook with React.useEffect so
setIsMounted(true) runs after paint/hydration instead of synchronously. Locate
the useLayoutEffect call in the FormNavButtons component and swap it to
useEffect (keeping the same effect body and dependency array) to preserve
behavior without blocking rendering.
In `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx`:
- Line 106: The label element using defaultInputLabelStyle with text inputLabel
isn't programmatically linked to the file input; update the label to include
htmlFor matching the file input's id (id={`file-upload-${id}`}) in the
SingleMediaUpload component so clicking the label opens the file picker and
improves accessibility.
🪄 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: 1af01b85-6481-4221-a8a6-33e8ce93b3b9
📒 Files selected for processing (14)
apps/web/src/layouts/DefaultLayout/Nav.styles.css.tsapps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsxapps/web/src/styles/globals.csspackages/create-dao-ui/src/components/AuctionSettingsForm/AuctionSettingsForm.tsxpackages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.css.tspackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsxpackages/create-proposal-ui/src/components/TransactionForm/Migration/PauseAuctionsForm.tsxpackages/create-proposal-ui/src/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.css.tspackages/ui/src/Fields/Radio.tsxpackages/ui/src/Fields/styles.css.tspackages/ui/src/SingleImageUpload/SingleImageUpload.tsxpackages/ui/src/SingleMediaUpload/SingleMediaUpload.tsxpackages/zord/src/components/PopUp.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx (1)
59-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude all captured values in
useCallbackdependencies.The callback uses
formik,onUploadStart, andonUploadSettledbut only lists[id, uploadType]as dependencies. If these props change between renders (which is likely if not explicitly memoized by the parent), the callback references stale values, causing bugs.Suggested fix
const handleFileUpload = React.useCallback( async (_input: FileList | null) => { // ... callback body }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [id, uploadType] + [acceptableMIME, formik, id, onUploadSettled, onUploadStart] )🤖 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 `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx` around lines 59 - 101, The useCallback handleFileUpload closes over props and helpers that aren’t listed in its dependency array—add the missing dependencies so the callback doesn’t capture stale values: include formik, onUploadStart, onUploadSettled, uploadFile, normalizeIPFSUrl, and acceptableMIME (in addition to the existing id and uploadType) in the dependency array for handleFileUpload so it updates whenever any of those change.packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsx (1)
24-34:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffInaccessible accordion toggle — keyboard users and screen-reader users cannot interact with this section.
The outer
Flexrenders as a<div>and carries anonClickhandler, but:
- It has no
tabIndex, so it is not reachable via the Tab key.- It has no
role="button"orrole="region", so assistive technology doesn't know it is interactive.- There is no
onKeyDownhandler, so Enter/Space do nothing.- There is no
aria-expandedattribute to communicate open/closed state.The PR description notes that multiple interactive
Flexwrappers were converted to semantic buttons — this component appears to have been missed.♿ Proposed minimal fix
- <Flex - direction={'column'} - align={'start'} - width={'100%'} - borderRadius={'curved'} - mb={'x4'} - borderWidth={'thin'} - borderColor={'primary'} - className={reviewSectionStyleVariants[isOpen ? 'open' : 'default']} - onClick={() => setIsOpen((bool) => !bool)} - > + <Flex + direction={'column'} + align={'start'} + width={'100%'} + borderRadius={'curved'} + mb={'x4'} + borderWidth={'thin'} + borderColor={'primary'} + className={reviewSectionStyleVariants[isOpen ? 'open' : 'default']} + >Then convert the heading row into the interactive element (it is the natural click target):
- <Flex - align={'center'} - justify={'center'} - className={reviewSectionSubHeading} - px={'x6'} - py={'x4'} - > + <Flex + as="button" + align={'center'} + justify={'center'} + className={reviewSectionSubHeading} + px={'x6'} + py={'x4'} + aria-expanded={isOpen} + onClick={() => setIsOpen((bool) => !bool)} + >🤖 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 `@packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsx` around lines 24 - 34, The outer interactive Flex in ReviewSection.tsx (the element using className={reviewSectionStyleVariants[isOpen ? 'open' : 'default']} and onClick={() => setIsOpen((bool) => !bool)}) is currently a non-focusable div; change it to a semantic interactive element (convert the clickable heading row/Flex into a <button> or set role="button") and add keyboard and a11y affordances: ensure it is focusable (tabIndex if not a button), add onKeyDown handling for Enter/Space that calls setIsOpen, include aria-expanded={isOpen} to communicate state, and preserve the existing className and styling so reviewSectionStyleVariants and setIsOpen behavior remain intact.apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx (1)
203-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
cursor: not-allowedmay not render on nativedisabledbuttons.Browsers commonly apply
pointer-events: noneto:disabledbuttons, which suppresses anycursorCSS set on the element. The intended visual signal (not-allowed hand) may be silently dropped. If the design requires this cursor, apply it via a CSS rule targeting:disabledusingpointer-events: auto; cursor: not-allowedin thechainPopUpItemstyle definition instead.🛠 Proposed fix in the style class (Nav.styles.css.ts)
export const chainPopUpItem = style({ // existing styles... + ':disabled': { + pointerEvents: 'auto', + cursor: 'not-allowed', + }, })Then remove the runtime
cursorprop from the Zord Flex for this case (or leave it as a no-op fallback).Also applies to: 215-215
🤖 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 `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx` around lines 203 - 210, The runtime cursor prop on the Zord Flex in ChainMenu.tsx is ineffective for native disabled buttons; update the chainPopUpItem style in Nav.styles.css.ts to include a rule for :disabled that sets pointer-events: auto and cursor: not-allowed (and any necessary specificity), then remove the dynamic cursor prop usage in the Flex rendering (the branches that set 'not-allowed' vs undefined) so the visual cursor is driven by the CSS class; apply the same change for the other occurrence around the lines mentioned.
🧹 Nitpick comments (2)
packages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsx (1)
9-23: 💤 Low value
useMemoon three trivially cheap scalar derivations adds boilerplate with no practical benefit.Each memoized value is a single
.lengthaccess or a shortreduceover a small array. These recompute in microseconds; wrapping them inuseMemoonly adds overhead and noise.♻️ Simplified inline derivations
- const filesCount = React.useMemo(() => { - if (!ipfsUpload) return 0 - return Object.keys(ipfsUpload).length - }, [ipfsUpload]) - - const traitCategoriesCount = React.useMemo(() => { - return orderedLayers?.length || 0 - }, [orderedLayers]) - - const totalTraitOptions = React.useMemo(() => { - if (!orderedLayers) return 0 - return orderedLayers.reduce((acc, layer) => { - return acc + (layer.properties?.length || 0) - }, 0) - }, [orderedLayers]) + const filesCount = ipfsUpload ? Object.keys(ipfsUpload).length : 0 + const traitCategoriesCount = orderedLayers?.length ?? 0 + const totalTraitOptions = orderedLayers?.reduce( + (acc, layer) => acc + (layer.properties?.length ?? 0), + 0 + ) ?? 0🤖 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 `@packages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsx` around lines 9 - 23, The three React.useMemo usages (filesCount, traitCategoriesCount, totalTraitOptions) in PreviewArtwork.tsx are unnecessary for trivial scalar computations; replace them with direct synchronous derivations: compute filesCount as ipfsUpload ? Object.keys(ipfsUpload).length : 0, traitCategoriesCount as orderedLayers?.length || 0, and totalTraitOptions by reducing orderedLayers inline (or using a simple loop) when rendering or just before return — remove the corresponding React.useMemo imports/usages to reduce boilerplate and keep orderedLayers and ipfsUpload references intact.apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx (1)
216-216: 💤 Low valueConsider
aria-current="location"over"true"for chain selector context.String
"true"is a valid WAI-ARIA token foraria-current, but"location"is the semantically correct value for a geographic/network location indicator and gives screen readers more useful context.- aria-current={selectedChain.id === chain.id ? 'true' : undefined} + aria-current={selectedChain.id === chain.id ? 'location' : undefined}🤖 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 `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx` at line 216, Replace the aria-current value for the chain selector so it uses the semantic token "location" instead of "true": in the ChainMenu component where aria-current is set based on selectedChain.id === chain.id, change the emitted string from "true" to "location" (i.e., set aria-current={selectedChain.id === chain.id ? 'location' : undefined}) to provide screen readers with the correct context.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/auction-ui/src/components/Auction.css.ts`:
- Around line 203-206: In the &:focus rule in Auction.css.ts (and the similar
bidCommentInput focus rule), remove the explicit outlineStyle: 'auto' which
overrides the shorthand outline and causes cross-browser inconsistency; keep the
outline: `2px solid ${vars.color.focusRing}` (or set outlineStyle: 'solid' if
you prefer explicit properties) so the themed 2px solid focus ring
(vars.color.focusRing) is consistently applied across browsers.
In `@packages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsx`:
- Around line 9-12: The filesCount computation in the React.useMemo uses
Object.keys(ipfsUpload).length even though ipfsUpload is an array; update the
memo (function computing filesCount) to return ipfsUpload.length when ipfsUpload
is present (e.g., if (!ipfsUpload) return 0; return ipfsUpload.length) so it
uses the array's native length property; adjust the dependency still to
[ipfsUpload] and keep the symbol names filesCount, ipfsUpload, and the
React.useMemo wrapper unchanged.
---
Outside diff comments:
In `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx`:
- Around line 203-210: The runtime cursor prop on the Zord Flex in ChainMenu.tsx
is ineffective for native disabled buttons; update the chainPopUpItem style in
Nav.styles.css.ts to include a rule for :disabled that sets pointer-events: auto
and cursor: not-allowed (and any necessary specificity), then remove the dynamic
cursor prop usage in the Flex rendering (the branches that set 'not-allowed' vs
undefined) so the visual cursor is driven by the CSS class; apply the same
change for the other occurrence around the lines mentioned.
In `@packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsx`:
- Around line 24-34: The outer interactive Flex in ReviewSection.tsx (the
element using className={reviewSectionStyleVariants[isOpen ? 'open' :
'default']} and onClick={() => setIsOpen((bool) => !bool)}) is currently a
non-focusable div; change it to a semantic interactive element (convert the
clickable heading row/Flex into a <button> or set role="button") and add
keyboard and a11y affordances: ensure it is focusable (tabIndex if not a
button), add onKeyDown handling for Enter/Space that calls setIsOpen, include
aria-expanded={isOpen} to communicate state, and preserve the existing className
and styling so reviewSectionStyleVariants and setIsOpen behavior remain intact.
In `@packages/ui/src/SingleMediaUpload/SingleMediaUpload.tsx`:
- Around line 59-101: The useCallback handleFileUpload closes over props and
helpers that aren’t listed in its dependency array—add the missing dependencies
so the callback doesn’t capture stale values: include formik, onUploadStart,
onUploadSettled, uploadFile, normalizeIPFSUrl, and acceptableMIME (in addition
to the existing id and uploadType) in the dependency array for handleFileUpload
so it updates whenever any of those change.
---
Nitpick comments:
In `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx`:
- Line 216: Replace the aria-current value for the chain selector so it uses the
semantic token "location" instead of "true": in the ChainMenu component where
aria-current is set based on selectedChain.id === chain.id, change the emitted
string from "true" to "location" (i.e., set aria-current={selectedChain.id ===
chain.id ? 'location' : undefined}) to provide screen readers with the correct
context.
In `@packages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsx`:
- Around line 9-23: The three React.useMemo usages (filesCount,
traitCategoriesCount, totalTraitOptions) in PreviewArtwork.tsx are unnecessary
for trivial scalar computations; replace them with direct synchronous
derivations: compute filesCount as ipfsUpload ? Object.keys(ipfsUpload).length :
0, traitCategoriesCount as orderedLayers?.length || 0, and totalTraitOptions by
reducing orderedLayers inline (or using a simple loop) when rendering or just
before return — remove the corresponding React.useMemo imports/usages to reduce
boilerplate and keep orderedLayers and ipfsUpload references intact.
🪄 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: 140ff720-06ad-4bd0-934c-3b4af0803367
📒 Files selected for processing (32)
apps/web/src/layouts/DefaultLayout/Nav.styles.css.tsapps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsxapps/web/src/modules/dashboard/Dashboard.tsxapps/web/src/modules/dashboard/SingleDaoSelector.css.tsapps/web/src/modules/explore/ExploreSortMenu.tsxapps/web/src/modules/explore/ExploreToolbar.tsxapps/web/src/modules/playground/PlaygroundPage.tsxapps/web/src/styles/globals.csspackages/auction-ui/src/components/AllBids/AllBids.tsxpackages/auction-ui/src/components/AllBids/BidCard.tsxpackages/auction-ui/src/components/Auction.css.tspackages/create-dao-ui/src/components/AuctionSettingsForm/AuctionSettingsForm.tsxpackages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.css.tspackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewItem.css.tspackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.css.tspackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsxpackages/create-proposal-ui/src/components/TransactionForm/Migration/PauseAuctionsForm.tsxpackages/create-proposal-ui/src/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.css.tspackages/dao-ui/src/components/DaoCard/DaoCard.css.tspackages/dao-ui/src/components/DaoCard/DaoCard.tsxpackages/ui/src/DropdownSelect/DropdownSelect.css.tspackages/ui/src/DropdownSelect/DropdownSelect.tsxpackages/ui/src/Fields/Radio.tsxpackages/ui/src/Fields/styles.css.tspackages/ui/src/SingleImageUpload/SingleImageUpload.test.tsxpackages/ui/src/SingleImageUpload/SingleImageUpload.tsxpackages/ui/src/SingleMediaUpload/SingleMediaUpload.tsxpackages/ui/src/WalletIdentity/WalletIdentity.tsxpackages/zord/src/components/PopUp.tsx
💤 Files with no reviewable changes (2)
- packages/ui/src/DropdownSelect/DropdownSelect.css.ts
- packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.css.ts
✅ Files skipped from review due to trivial changes (8)
- packages/create-proposal-ui/src/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.css.ts
- packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.css.ts
- apps/web/src/modules/explore/ExploreSortMenu.tsx
- apps/web/src/modules/dashboard/Dashboard.tsx
- apps/web/src/modules/dashboard/SingleDaoSelector.css.ts
- packages/dao-ui/src/components/DaoCard/DaoCard.css.ts
- packages/dao-ui/src/components/DaoCard/DaoCard.tsx
- packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewItem.css.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/src/layouts/DefaultLayout/Nav.styles.css.ts
- packages/create-proposal-ui/src/components/TransactionForm/Migration/PauseAuctionsForm.tsx
- packages/ui/src/SingleImageUpload/SingleImageUpload.tsx
- packages/create-dao-ui/src/components/FormNavButtons/FormNavButtons.tsx
- packages/ui/src/Fields/Radio.tsx
- packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewAndDeploy.tsx
- packages/zord/src/components/PopUp.tsx
- packages/create-dao-ui/src/components/AuctionSettingsForm/AuctionSettingsForm.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx (1)
209-209: ⚡ Quick winPrefer
aria-pressed(oraria-checkedwith role) overaria-currentfor selectable buttons.For button-based option selection,
aria-current="location"is less semantically precise than a pressed/checked state model.Suggested accessibility tweak
- aria-current={selectedChain.id === chain.id ? 'location' : undefined} + aria-pressed={selectedChain.id === chain.id}🤖 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 `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx` at line 209, The current button uses aria-current to indicate selection (aria-current={selectedChain.id === chain.id ? 'location' : undefined}); change this to a pressed/checked pattern: for a plain selectable button set aria-pressed={selectedChain.id === chain.id} (boolean), or if you intend a radio/option role add role="menuitemradio" (or "radio") and use aria-checked={selectedChain.id === chain.id}; update the attribute on the same element rendering the chain option (the code referencing selectedChain and chain in ChainMenu.tsx).
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@apps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsx`:
- Line 209: The current button uses aria-current to indicate selection
(aria-current={selectedChain.id === chain.id ? 'location' : undefined}); change
this to a pressed/checked pattern: for a plain selectable button set
aria-pressed={selectedChain.id === chain.id} (boolean), or if you intend a
radio/option role add role="menuitemradio" (or "radio") and use
aria-checked={selectedChain.id === chain.id}; update the attribute on the same
element rendering the chain option (the code referencing selectedChain and chain
in ChainMenu.tsx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d5f9dc-6c59-4794-afc0-5013db44be43
📒 Files selected for processing (5)
apps/web/src/layouts/DefaultLayout/Nav.styles.css.tsapps/web/src/layouts/DefaultLayout/NavMenu/ChainMenu.tsxpackages/auction-ui/src/components/Auction.css.tspackages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsxpackages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/create-dao-ui/src/components/ReviewAndDeploy/ReviewSection.tsx
- packages/create-dao-ui/src/components/ReviewAndDeploy/PreviewArtwork.tsx
- packages/auction-ui/src/components/Auction.css.ts
- apps/web/src/layouts/DefaultLayout/Nav.styles.css.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
New Features
Improvements
Tests