feat(frontend): bump @heroui/react from 2.8.10 to 3.0.1 and fix failing tests#4434
Draft
Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Draft
feat(frontend): bump @heroui/react from 2.8.10 to 3.0.1 and fix failing tests#4434Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Conversation
…ng tests - Upgrade @heroui/react to v3.0.1 - Remove HeroUIProvider (dropped in v3, React 19 handles it natively) - Replace Navbar/NavbarItem/NavbarContent with plain HTML nav (removed in v3) - Rename BreadcrumbItem to BreadcrumbsItem, remove itemClasses prop - Replace Input label/labelPlacement with plain HTML label elements - Replace Autocomplete/AutocompleteItem with plain HTML combobox in ProjectSelector - Replace Pagination compound component (initialPage/onChange removed in v3) - Replace variant='solid' with variant='primary' in ProjectsDashboardDropDown - Remove DropdownSection title prop (removed in v3) - Update jest.config.ts to handle ESM modules from @heroui v3 - Add __mocks__/@heroui/react.js CJS mock for Jest compatibility - Fix pre-existing URLSearchParams.entries().filter() bug in metrics page - Update affected tests to match new component implementations Closes OWASP#4394
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a local Jest mock for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/components/ProjectsDashboardDropDown.tsx (1)
65-69:⚠️ Potential issue | 🟠 MajorUse
<Header>component for section titles in v3.The v3 API changed from a
titleprop to a compound component pattern. Section titles must be rendered using a<Header>component as the first child insideDropdownSection. Replacedata-titlewith:<DropdownSection key={section.title}> <Header>{section.title}</Header> {section.items.map((item) => ( <DropdownItem key={item.key}>{item.label}</DropdownItem> ))} </DropdownSection>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ProjectsDashboardDropDown.tsx` around lines 65 - 69, Replace the current use of DropdownSection with the v3 compound API by removing the data-title attribute and rendering a Header component as the first child: inside the DropdownSection for each section (identified by section.title) add <Header> with section.title, then render the existing section.items.map to produce DropdownItem children (use item.key and item.label as before) so the structure becomes Header followed by DropdownItem elements within DropdownSection.frontend/package.json (1)
25-34:⚠️ Potential issue | 🟠 MajorComplete the migration to
@heroui/reactv3 by updating all component imports.The individual
@herouiv2 sub-packages (@heroui/button,@heroui/tooltip,@heroui/modal,@heroui/select,@heroui/skeleton,@heroui/system,@heroui/toast,@heroui/autocomplete) are actively imported throughout the codebase (60+ instances across 40+ files). Since@heroui/reactv3 consolidates all these components into the main package, these v2 packages should be removed from dependencies and all imports should be migrated to use@heroui/reactinstead (e.g.,import { Button } from '@heroui/react'instead ofimport { Button } from '@heroui/button'). This completes the v3 migration and eliminates version fragmentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` around lines 25 - 34, The package.json still lists legacy v2 sub-packages (e.g., "@heroui/button", "@heroui/tooltip", "@heroui/modal", "@heroui/select", "@heroui/skeleton", "@heroui/system", "@heroui/toast", "@heroui/autocomplete") while the project has migrated to "@heroui/react" v3; remove these v2 dependencies and update all import sites (e.g., occurrences of import { Button } from '@heroui/button', import { Tooltip } from '@heroui/tooltip', etc.) to import from '@heroui/react' (e.g., import { Button, Tooltip, Modal, Select, Skeleton, System, Toast, Autocomplete } from '@heroui/react'), then run a global search/replace across the codebase, update package.json to only include "@heroui/react": "^3.x", and run install and the test/build pipeline to ensure no missing exports remain.frontend/src/components/ModuleForm.tsx (2)
296-312:⚠️ Potential issue | 🟠 MajorDuplicate "Project Name" label - label exists both here and inside ProjectSelector.
There's a label "Project Name *" at line 297, and
ProjectSelectoritself also renders a label at lines 427-432. This results in duplicate labels for the same input, which is confusing and creates accessibility issues (multiple labels for one input).Proposed fix - remove either the external label or the one inside ProjectSelector
Option 1: Remove the external label (keep ProjectSelector self-contained):
<div className="w-full min-w-0" style={{ maxWidth: '100%', overflow: 'hidden' }}> - <label htmlFor="projectSelector" className="mb-1 block text-sm font-semibold text-gray-600 dark:text-gray-300">Project Name<span aria-hidden="true"> *</span></label> <ProjectSelectorOption 2: Remove the label inside ProjectSelector and pass it as a prop for external control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 296 - 312, The form renders a duplicate "Project Name" label: an external label next to the ProjectSelector and another label inside the ProjectSelector component; remove one to avoid duplicate labels and accessibility issues. Choose one approach: either delete the external <label> block and let ProjectSelector remain self-contained (update ModuleForm.tsx to remove the label wrapper and keep passing value/defaultName/onProjectChange/isInvalid/errorMessage), or refactor ProjectSelector to accept a label prop (e.g., labelText) and remove its internal <label>, then pass the "Project Name *" label from ModuleForm via that prop; update references to ProjectSelector props accordingly (ProjectSelector, formData.projectId, formData.projectName, onProjectChange, isInvalid, errorMessage) so only a single label is rendered.
242-268:⚠️ Potential issue | 🟠 MajorIncorrect label placement - "Project Name" label wraps the "Experience Level" select.
The label at line 243 says "Project Name *" but the component inside this wrapper (lines 244-267) is the
Selectfor "Experience Level". This appears to be a copy-paste error during the migration.Proposed fix - remove the incorrect label
<div className="w-full min-w-0" style={{ maxWidth: '100%', overflow: 'hidden' }}> - <label htmlFor="projectSelector" className="mb-1 block text-sm font-semibold text-gray-600 dark:text-gray-300">Project Name<span aria-hidden="true"> *</span></label> <Select id="experienceLevel" label="Experience Level" - selectedKeys={🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 242 - 268, The label "Project Name" is incorrectly wrapping the Experience Level Select; update the label to match the Select by changing its htmlFor and text to "experienceLevel" / "Experience Level *" (or remove the label if a separate Project Name field exists elsewhere) so it corresponds to the Select with id="experienceLevel" and the EXPERIENCE_LEVELS mapping; ensure the accessibility htmlFor/id pair and required marker remain correct for the Select component and adjust any duplicated Project Name label elsewhere if needed.
🧹 Nitpick comments (4)
frontend/src/components/BreadCrumbsWrapper.tsx (1)
30-34: Minor inconsistency:isDisabledprop used here but not inBreadCrumbs.tsx.This component uses
isDisabled={isLast}whileBreadCrumbs.tsxdoesn't use this prop for the last breadcrumb item. Consider aligning the behavior across both components for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/BreadCrumbsWrapper.tsx` around lines 30 - 34, The Breadcrumbs behavior is inconsistent: BreadCrumbsWrapper sets isDisabled={isLast} on HeroUIBreadcrumbItem but BreadCrumbs does not; pick one behavior and apply it to both components for consistency. Update the BreadCrumbs component to pass isDisabled={isLast} into the same breadcrumb render path (or remove isDisabled from HeroUIBreadcrumbItem in BreadCrumbsWrapper if you prefer enabled items)—ensure the prop usage and the isLast calculation are mirrored between BreadCrumbs and BreadCrumbsWrapper so both use the same logic and prop name.frontend/src/app/projects/dashboard/metrics/page.tsx (2)
303-377: Consider extracting duplicated pagination handler logic.The
fetchMorelogic withupdateQueryis repeated three times (forPaginationPrevious,PaginationLink, andPaginationNext). While this works correctly, extracting it into a shared helper would improve maintainability.Example helper extraction
const handlePageChange = async (page: number) => { const newOffset = (page - 1) * PAGINATION_LIMIT const newPagination = { offset: newOffset, limit: PAGINATION_LIMIT } setPagination(newPagination) await fetchMore({ variables: { filters, pagination: newPagination, ordering: buildOrderingWithTieBreaker(ordering), }, updateQuery: (prev, { fetchMoreResult }) => { if (!fetchMoreResult) return prev return { ...prev, projectHealthMetrics: fetchMoreResult.projectHealthMetrics } }, }) }Then use:
onPress={() => handlePageChange(getCurrentPage() - 1)}for Previous, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/dashboard/metrics/page.tsx` around lines 303 - 377, Duplicate pagination/fetchMore logic should be extracted into a single async helper (e.g. handlePageChange) that takes page:number, computes newOffset using PAGINATION_LIMIT, builds newPagination, calls setPagination(newPagination) and calls fetchMore with variables { filters, pagination: newPagination, ordering: buildOrderingWithTieBreaker(ordering) } and the common updateQuery that returns fetchMoreResult.projectHealthMetrics; then replace the inline onPress handlers on PaginationPrevious, each PaginationLink, and PaginationNext to simply call handlePageChange(<targetPage>).
328-352: Verify pagination performance with large page counts.When
metricsLengthis large, rendering all page links viaArray.from({ length: ... })could create many DOM elements. Consider adding ellipsis logic for datasets with many pages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/projects/dashboard/metrics/page.tsx` around lines 328 - 352, The current pagination renders every page link using Array.from based on metricsLength and PAGINATION_LIMIT which can create excessive DOM nodes; modify the pagination in the component (references: metricsLength, PAGINATION_LIMIT, PaginationItem, PaginationLink, getCurrentPage, setPagination, fetchMore) to implement a compact/ellipsis pagination strategy (show first, last, current ±N pages and ellipses) and render only those page numbers, keeping the existing onPress logic (compute newOffset, setPagination, await fetchMore with filters/pagination/ordering) for each rendered page link so behavior is unchanged but DOM count is greatly reduced for large page counts.frontend/src/components/ModuleForm.tsx (1)
445-451: Hidden clear button may cause accessibility issues.The clear button with
className="hidden"is completely hidden from all users including screen reader users. If this button is intended to be triggered programmatically or via tests only, consider usingaria-hidden="true"instead, or remove it if the functionality isn't needed in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 445 - 451, The clear button currently uses className="hidden" which hides it from screen readers and users; either make it test-only/hidden-from-visual-users but still ignored by assistive tech by replacing the visual hide with aria-hidden="true" (keep data-testid="autocomplete-clear" and the onClick handler using setInputValue, setItems, setIsOpen, onProjectChange so tests can trigger it programmatically), or remove the button entirely if production functionality isn't needed; update ModuleForm.tsx accordingly and ensure automated tests are adjusted to use the new approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/forms/shared/FormDateInput.tsx`:
- Around line 30-36: In FormDateInput, the required asterisk span is rendered as
a sibling of the <label> which breaks semantics and layout; move the conditional
span into the label so it is a child (use the existing id, label and required
props/variables), i.e. render the label content and then append the {required &&
<span ...>*</span>} inside the label element, preserving the aria-hidden and
classes so styling and accessibility remain the same.
In `@frontend/src/components/forms/shared/FormTextInput.tsx`:
- Around line 36-42: In FormTextInput (component FormTextInput, props label, id,
required) the required asterisk <span> is rendered as a sibling after the
closing </label>, which can break spacing; move the conditional {required &&
<span ...>*</span>} inside the label element so the asterisk is part of the
label content (keep aria-hidden and classes) to ensure correct visual
association and inline spacing with the label text.
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 456-468: The dropdown list items rendered in items.map use only
onClick (see data-testid="autocomplete-item" and handleSelect) and lack keyboard
accessibility; update the component to add tabIndex={0} to each <li>, implement
an onKeyDown handler that calls handleSelect for Enter/Space, and add arrow-key
focus management (track a focusedIndex state, move focus with ArrowUp/ArrowDown
and set aria-selected appropriately) and handle Escape to close the dropdown
(call the existing close/toggle method or set isOpen=false). Ensure the
onKeyDown is attached to each item (or the list container) and use the unique
identifiers (items.map, handleSelect, data-testid="autocomplete-item") to locate
and update the code.
- Line 3: Remove the unused imports Autocomplete and ListBoxItem from the top of
ModuleForm.tsx: delete them from the import statement that references
'@heroui/react' so the module only imports symbols actually used by the file
(e.g., keep any other used imports but remove Autocomplete and ListBoxItem).
---
Outside diff comments:
In `@frontend/package.json`:
- Around line 25-34: The package.json still lists legacy v2 sub-packages (e.g.,
"@heroui/button", "@heroui/tooltip", "@heroui/modal", "@heroui/select",
"@heroui/skeleton", "@heroui/system", "@heroui/toast", "@heroui/autocomplete")
while the project has migrated to "@heroui/react" v3; remove these v2
dependencies and update all import sites (e.g., occurrences of import { Button }
from '@heroui/button', import { Tooltip } from '@heroui/tooltip', etc.) to
import from '@heroui/react' (e.g., import { Button, Tooltip, Modal, Select,
Skeleton, System, Toast, Autocomplete } from '@heroui/react'), then run a global
search/replace across the codebase, update package.json to only include
"@heroui/react": "^3.x", and run install and the test/build pipeline to ensure
no missing exports remain.
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 296-312: The form renders a duplicate "Project Name" label: an
external label next to the ProjectSelector and another label inside the
ProjectSelector component; remove one to avoid duplicate labels and
accessibility issues. Choose one approach: either delete the external <label>
block and let ProjectSelector remain self-contained (update ModuleForm.tsx to
remove the label wrapper and keep passing
value/defaultName/onProjectChange/isInvalid/errorMessage), or refactor
ProjectSelector to accept a label prop (e.g., labelText) and remove its internal
<label>, then pass the "Project Name *" label from ModuleForm via that prop;
update references to ProjectSelector props accordingly (ProjectSelector,
formData.projectId, formData.projectName, onProjectChange, isInvalid,
errorMessage) so only a single label is rendered.
- Around line 242-268: The label "Project Name" is incorrectly wrapping the
Experience Level Select; update the label to match the Select by changing its
htmlFor and text to "experienceLevel" / "Experience Level *" (or remove the
label if a separate Project Name field exists elsewhere) so it corresponds to
the Select with id="experienceLevel" and the EXPERIENCE_LEVELS mapping; ensure
the accessibility htmlFor/id pair and required marker remain correct for the
Select component and adjust any duplicated Project Name label elsewhere if
needed.
In `@frontend/src/components/ProjectsDashboardDropDown.tsx`:
- Around line 65-69: Replace the current use of DropdownSection with the v3
compound API by removing the data-title attribute and rendering a Header
component as the first child: inside the DropdownSection for each section
(identified by section.title) add <Header> with section.title, then render the
existing section.items.map to produce DropdownItem children (use item.key and
item.label as before) so the structure becomes Header followed by DropdownItem
elements within DropdownSection.
---
Nitpick comments:
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 303-377: Duplicate pagination/fetchMore logic should be extracted
into a single async helper (e.g. handlePageChange) that takes page:number,
computes newOffset using PAGINATION_LIMIT, builds newPagination, calls
setPagination(newPagination) and calls fetchMore with variables { filters,
pagination: newPagination, ordering: buildOrderingWithTieBreaker(ordering) } and
the common updateQuery that returns fetchMoreResult.projectHealthMetrics; then
replace the inline onPress handlers on PaginationPrevious, each PaginationLink,
and PaginationNext to simply call handlePageChange(<targetPage>).
- Around line 328-352: The current pagination renders every page link using
Array.from based on metricsLength and PAGINATION_LIMIT which can create
excessive DOM nodes; modify the pagination in the component (references:
metricsLength, PAGINATION_LIMIT, PaginationItem, PaginationLink, getCurrentPage,
setPagination, fetchMore) to implement a compact/ellipsis pagination strategy
(show first, last, current ±N pages and ellipses) and render only those page
numbers, keeping the existing onPress logic (compute newOffset, setPagination,
await fetchMore with filters/pagination/ordering) for each rendered page link so
behavior is unchanged but DOM count is greatly reduced for large page counts.
In `@frontend/src/components/BreadCrumbsWrapper.tsx`:
- Around line 30-34: The Breadcrumbs behavior is inconsistent:
BreadCrumbsWrapper sets isDisabled={isLast} on HeroUIBreadcrumbItem but
BreadCrumbs does not; pick one behavior and apply it to both components for
consistency. Update the BreadCrumbs component to pass isDisabled={isLast} into
the same breadcrumb render path (or remove isDisabled from HeroUIBreadcrumbItem
in BreadCrumbsWrapper if you prefer enabled items)—ensure the prop usage and the
isLast calculation are mirrored between BreadCrumbs and BreadCrumbsWrapper so
both use the same logic and prop name.
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 445-451: The clear button currently uses className="hidden" which
hides it from screen readers and users; either make it
test-only/hidden-from-visual-users but still ignored by assistive tech by
replacing the visual hide with aria-hidden="true" (keep
data-testid="autocomplete-clear" and the onClick handler using setInputValue,
setItems, setIsOpen, onProjectChange so tests can trigger it programmatically),
or remove the button entirely if production functionality isn't needed; update
ModuleForm.tsx accordingly and ensure automated tests are adjusted to use the
new approach.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8ca16c3-7557-4a1f-b28c-9068ffde254a
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
frontend/__mocks__/@heroui/react.jsfrontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsxfrontend/__tests__/unit/components/forms/shared/FormDateInput.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/jest.config.tsfrontend/package.jsonfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/components/BreadCrumbs.tsxfrontend/src/components/BreadCrumbsWrapper.tsxfrontend/src/components/ModuleForm.tsxfrontend/src/components/ProjectsDashboardDropDown.tsxfrontend/src/components/ProjectsDashboardNavBar.tsxfrontend/src/components/forms/shared/FormDateInput.tsxfrontend/src/components/forms/shared/FormTextInput.tsxfrontend/src/wrappers/provider.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 17 files
Confidence score: 4/5
- This PR is likely safe to merge, with only low-to-moderate issues (severity 5/10 and 4/10) and no clear merge-blocking risk.
- In
frontend/__tests__/unit/components/ModuleForm.test.tsx, the test may be flaky because it reads autocomplete items before async suggestions are guaranteed to render, which can cause intermittent CI failures. - In
frontend/src/components/ModuleForm.tsx, the new "Project Name" label appears above the Experience Level select and targetsprojectSelector, which can confuse users and create incorrect label/input association for accessibility. - Pay close attention to
frontend/__tests__/unit/components/ModuleForm.test.tsxandfrontend/src/components/ModuleForm.tsx- stabilize async test timing and fix the mislabeled field association.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/ModuleForm.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:416">
P2: Test may be flaky because it synchronously reads autocomplete items before async suggestion rendering is guaranteed complete.</violation>
</file>
<file name="frontend/src/components/ModuleForm.tsx">
<violation number="1" location="frontend/src/components/ModuleForm.tsx:243">
P2: The new "Project Name" label is rendered above the Experience Level select and points to projectSelector, so it mislabels the Experience Level field and associates the label with the wrong input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Move required asterisk inside label element (FormDateInput, FormTextInput) - Remove unused Autocomplete and ListBoxItem imports from ModuleForm - Remove misplaced Project Name labels above Experience Level field - Add keyboard navigation to ProjectSelector dropdown (arrow keys, Enter, Escape) - Add ARIA attributes to combobox (role, aria-expanded, aria-autocomplete, aria-activedescendant) - Fix async test timing in ModuleForm autocomplete test - Update label text matchers in tests to handle asterisk inside label
Contributor
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ModuleForm.tsx">
<violation number="1" location="frontend/src/components/ModuleForm.tsx:427">
P2: `focusedIndex` can become out of bounds after `items` updates, so pressing Enter can call `handleSelect` with `undefined` and crash when it dereferences `project.name`/`project.id`. Add a bounds check or reset the index when `items` changes.</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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/components/ModuleForm.tsx (1)
365-397: Skip lookups for confirmed selections.After Line 411 commits a project, the current lookup path still searches again using the committed label. That adds an unnecessary round-trip on edit forms and can reopen the popup right after Line 410 closed it when sibling matches exist. Guard the lookup when
value/defaultNamealready represent a selected project.Also applies to: 400-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 365 - 397, The lookup still runs after a project is committed; update the debounced fetchSuggestions (and the similar lookup function used around lines 400–411) to early-return when the current query matches an already-selected project so we skip the network round-trip. Concretely, at the top of fetchSuggestions (and the other lookup path) add a guard: if value (the selected project id / marker) or defaultName (the committed project label) is present and trimmedQuery === defaultName (or otherwise clearly identifies the committed selection), then call setItems([]), setIsOpen(false) (or no-op) and return immediately; otherwise proceed with the existing length check, client.query call, filtering, setItems and setIsOpen logic. Ensure you update both places that perform the lookup to use the same guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 455-459: The UI is hiding the project error whenever inputValue is
non-empty, letting Save silently no-op; update the logic that computes
shouldShowInvalid (and the aria-describedby condition used in the JSX around
handleInputChange/handleKeyDown) to depend on a validation/submission state
instead of inputValue alone — e.g., use a flag like hasSubmitted ||
isTouchedCombinedWithValidation (or create showProjectError = validationFailed
&& !projectId) and set aria-invalid and aria-describedby based on that flag;
make the same change for the corresponding block at the second occurrence (lines
~496-499) so the inline error is shown after validation has failed rather than
whenever inputValue is non-empty.
- Around line 443-463: The combobox input (id="projectSelector") lacks an
aria-controls linkage to its popup listbox; add a stable id on the popup (e.g.,
"project-listbox") and set the input's aria-controls to that id when open (use
the existing isOpen state), also add aria-haspopup="listbox" on the input and
ensure the popup element uses role="listbox" and the same id so screen readers
can associate projectSelector with the list; update related handlers
(handleKeyDown/handleInputChange) only if needed to preserve the same id usage
when toggling isOpen and focusedIndex.
- Around line 352-353: The focusedIndex state can become out-of-range when the
suggestions array (items) changes; add logic to reset or clamp focusedIndex
whenever items is replaced or its length changes: in the ModuleForm component
watch items (or the suggestions source) and call setFocusedIndex(-1) or
setFocusedIndex(Math.min(focusedIndex, items.length - 1)) as appropriate so
handleSelect never receives undefined; update any keyboard navigation handlers
that set focusedIndex (and the useState declarations focusedIndex /
setFocusedIndex) to rely on this clamp/reset behavior to keep focusedIndex
valid.
---
Nitpick comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 365-397: The lookup still runs after a project is committed;
update the debounced fetchSuggestions (and the similar lookup function used
around lines 400–411) to early-return when the current query matches an
already-selected project so we skip the network round-trip. Concretely, at the
top of fetchSuggestions (and the other lookup path) add a guard: if value (the
selected project id / marker) or defaultName (the committed project label) is
present and trimmedQuery === defaultName (or otherwise clearly identifies the
committed selection), then call setItems([]), setIsOpen(false) (or no-op) and
return immediately; otherwise proceed with the existing length check,
client.query call, filtering, setItems and setIsOpen logic. Ensure you update
both places that perform the lookup to use the same guard.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee779e65-d7e9-4554-ab1f-6b83e403bc3c
📒 Files selected for processing (10)
frontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/__tests__/unit/components/ProgramForm.test.tsxfrontend/__tests__/unit/components/forms/shared/FormDateInput.test.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/EditModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/components/ModuleForm.tsxfrontend/src/components/forms/shared/FormDateInput.tsxfrontend/src/components/forms/shared/FormTextInput.tsx
✅ Files skipped from review due to trivial changes (5)
- frontend/tests/unit/pages/CreateModule.test.tsx
- frontend/tests/unit/pages/EditProgram.test.tsx
- frontend/tests/unit/pages/CreateProgram.test.tsx
- frontend/tests/unit/components/ProgramForm.test.tsx
- frontend/tests/unit/components/forms/shared/FormDateInput.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/forms/shared/FormTextInput.tsx
- frontend/tests/unit/components/ModuleForm.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 476-494: Add a click-outside handler using a ref and useEffect to
close the dropdown when the user clicks outside the combobox: create a
containerRef for the wrapper <div> that contains the list, attach
ref={containerRef} to that wrapper, then in a useEffect add a document
'mousedown' or 'click' listener that checks if event.target is outside
containerRef.current and if so calls the dropdown-close action (e.g.,
setIsOpen(false) or your close handler) and resets focusedIndex
(setFocusedIndex(-1)); ensure you remove the listener in the cleanup function.
Target the existing symbols isOpen, items, handleSelect, setFocusedIndex, and
the wrapper around the list so the dropdown closes on outside clicks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 611e1994-98ea-4f4d-89f3-5831d55459a3
📒 Files selected for processing (1)
frontend/src/components/ModuleForm.tsx
Comment on lines
+476
to
+494
| {isOpen && items.length > 0 && ( | ||
| <ul role="listbox" className="absolute z-50 mt-1 w-full rounded-md border border-gray-200 bg-white shadow-lg dark:border-gray-700 dark:bg-gray-800"> | ||
| {items.map((project, index) => ( | ||
| <li | ||
| key={project.id} | ||
| id={`project-option-${index}`} | ||
| role="option" | ||
| aria-selected={focusedIndex === index} | ||
| tabIndex={0} | ||
| data-testid="autocomplete-item" | ||
| data-text-value={project.name} | ||
| onClick={() => { handleSelect(project); setFocusedIndex(-1) }} | ||
| onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); handleSelect(project); setFocusedIndex(-1) } }} | ||
| className={`cursor-pointer px-3 py-2 text-sm text-gray-800 hover:bg-gray-100 dark:text-gray-200 dark:hover:bg-gray-700 ${focusedIndex === index ? 'bg-gray-100 dark:bg-gray-700' : ''}`} | ||
| > | ||
| {project.name} | ||
| </li> | ||
| ))} | ||
| </ul> |
Contributor
There was a problem hiding this comment.
Dropdown does not close when clicking outside.
The custom combobox lacks a click-outside handler. Users must press Escape or select an item to close the dropdown, which differs from expected combobox behavior and can leave the dropdown floating when focus moves elsewhere.
Proposed fix using useEffect with document listener
+ const containerRef = useRef<HTMLDivElement>(null)
+
+ useEffect(() => {
+ const handleClickOutside = (e: MouseEvent) => {
+ if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
+ setIsOpen(false)
+ setFocusedIndex(-1)
+ }
+ }
+ if (isOpen) {
+ document.addEventListener('mousedown', handleClickOutside)
+ }
+ return () => document.removeEventListener('mousedown', handleClickOutside)
+ }, [isOpen])Then add ref={containerRef} to the wrapper <div> at line 442.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 477-477: Use , , or instead of the "listbox" role to ensure accessibility across all devices.
[warning] 482-482: Non-interactive elements should not be assigned interactive roles.
[warning] 477-477: Non-interactive elements should not be assigned interactive roles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ModuleForm.tsx` around lines 476 - 494, Add a
click-outside handler using a ref and useEffect to close the dropdown when the
user clicks outside the combobox: create a containerRef for the wrapper <div>
that contains the list, attach ref={containerRef} to that wrapper, then in a
useEffect add a document 'mousedown' or 'click' listener that checks if
event.target is outside containerRef.current and if so calls the dropdown-close
action (e.g., setIsOpen(false) or your close handler) and resets focusedIndex
(setFocusedIndex(-1)); ensure you remove the listener in the cleanup function.
Target the existing symbols isOpen, items, handleSelect, setFocusedIndex, and
the wrapper around the list so the dropdown closes on outside clicks.
- Reset focusedIndex when items list changes to prevent out-of-bounds access - Add aria-controls and aria-haspopup to link combobox to listbox - Add id to listbox ul for aria-controls reference - Always surface validation error regardless of typing state - Update test to reflect corrected validation behavior
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ModuleForm.tsx">
<violation number="1" location="frontend/src/components/ModuleForm.tsx:439">
P2: Project selector now shows validation errors while users are typing because the `isTyping` guard was removed, even though typing clears `projectId` and marks it touched. This causes a regression where "Project is required" appears during normal search input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ring active input - Suppress error message and aria-invalid while user is actively typing - Error surfaces only after field is blurred or value is cleared - Balances CodeRabbit a11y feedback with cubic UX regression concern
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ModuleForm.tsx">
<violation number="1" location="frontend/src/components/ModuleForm.tsx:440">
P2: Validation errors for the required project selection are suppressed whenever the user has typed but not selected a project, so a submit failure can occur with no visible feedback.</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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ModuleForm.tsx (1)
365-397:⚠️ Potential issue | 🟠 MajorGuard async suggestions against stale responses and fully close the popup when clearing results.
An older
client.querycan resolve after a newer query—or afterhandleSelecthas already closed the picker—and then repopulateitems/ reopen the popup with stale suggestions. Also, the<2 charsand error branches clearitemswithout clearingisOpen, so Line 463 can still expose an expanded combobox with no listbox.💡 One way to keep suggestion state coherent
-import { useState, useEffect, useCallback } from 'react' +import { useState, useEffect, useCallback, useRef } from 'react' ... + const requestIdRef = useRef(0) + const fetchSuggestions = useCallback( debounce(async (query: string) => { const trimmedQuery = query.trim() + const requestId = ++requestIdRef.current + if (trimmedQuery.length < 2) { setItems([]) + setIsOpen(false) setIsLoading(false) setFocusedIndex(-1) return } setIsLoading(true) try { const { data } = await client.query({ query: SearchProjectNamesDocument, variables: { query: trimmedQuery }, }) + if (requestId !== requestIdRef.current) return const projects = data?.searchProjects || [] const filtered = projects.filter((proj) => proj.id !== value) setItems(filtered.slice(0, 5)) setIsOpen(filtered.length > 0) setFocusedIndex(-1) } catch (err) { + if (requestId !== requestIdRef.current) return // eslint-disable-next-line no-console console.error( 'Error fetching project suggestions:', err instanceof Error ? err.message : String(err), err ) setItems([]) + setIsOpen(false) + setFocusedIndex(-1) } finally { - setIsLoading(false) + if (requestId === requestIdRef.current) { + setIsLoading(false) + } } }, 300), [client, value] )Also applies to: 463-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 365 - 397, The async fetchSuggestions can apply stale responses and fails to close the popup when clearing items; add a monotonic request token (e.g., requestIdRef) that you increment before starting the client.query in fetchSuggestions (or before the debounce call), capture the current token locally, and after any await check that the token still matches before calling setItems/setIsOpen/setFocusedIndex/setIsLoading; also ensure both the trimmed <2 branch and the catch branch explicitly call setIsOpen(false) and setFocusedIndex(-1) so the combobox fully closes when results are cleared. Use the symbols fetchSuggestions, client.query (SearchProjectNamesDocument), setItems, setIsOpen, setFocusedIndex, setIsLoading, and a new requestIdRef to locate and implement these changes.
♻️ Duplicate comments (1)
frontend/src/components/ModuleForm.tsx (1)
439-441:⚠️ Potential issue | 🟠 MajorDon’t suppress the invalid state after a failed submit.
isTypingonly checks for non-empty text plus emptyvalue. If the user types free text and hits Save without picking an option,projectIdis still empty butaria-invalidstays false and the inline error never renders. Please drive this from focus/blur or an explicit submit-attempt flag instead ofinputValuealone.Also applies to: 460-467, 500-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 439 - 441, The current logic using isTyping (const isTyping = inputValue.trim() !== '' && !value) suppresses invalid state after a failed submit; change the UX to rely on a focus/blur or explicit submit-attempt flag instead of inputValue alone: introduce and use a boolean like submitAttempted (set true on Save/submit handler) or a focused state (set on focus/blur) and update displayError and shouldShowInvalid to use that flag plus isInvalid (e.g., displayError = (submitAttempted || !focused) ? errorMessage : undefined; shouldShowInvalid = (submitAttempted || !focused) ? isInvalid : false), replacing uses of isTyping in the places that define isTyping, displayError, and shouldShowInvalid (also update the analogous blocks around lines 460-467 and 500-504).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 365-397: The async fetchSuggestions can apply stale responses and
fails to close the popup when clearing items; add a monotonic request token
(e.g., requestIdRef) that you increment before starting the client.query in
fetchSuggestions (or before the debounce call), capture the current token
locally, and after any await check that the token still matches before calling
setItems/setIsOpen/setFocusedIndex/setIsLoading; also ensure both the trimmed <2
branch and the catch branch explicitly call setIsOpen(false) and
setFocusedIndex(-1) so the combobox fully closes when results are cleared. Use
the symbols fetchSuggestions, client.query (SearchProjectNamesDocument),
setItems, setIsOpen, setFocusedIndex, setIsLoading, and a new requestIdRef to
locate and implement these changes.
---
Duplicate comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 439-441: The current logic using isTyping (const isTyping =
inputValue.trim() !== '' && !value) suppresses invalid state after a failed
submit; change the UX to rely on a focus/blur or explicit submit-attempt flag
instead of inputValue alone: introduce and use a boolean like submitAttempted
(set true on Save/submit handler) or a focused state (set on focus/blur) and
update displayError and shouldShowInvalid to use that flag plus isInvalid (e.g.,
displayError = (submitAttempted || !focused) ? errorMessage : undefined;
shouldShowInvalid = (submitAttempted || !focused) ? isInvalid : false),
replacing uses of isTyping in the places that define isTyping, displayError, and
shouldShowInvalid (also update the analogous blocks around lines 460-467 and
500-504).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10f84687-bfbf-43b7-b39d-a63aa3d45445
📒 Files selected for processing (1)
frontend/src/components/ModuleForm.tsx
Parent form already controls isInvalid/errorMessage via touched.projectId state. Child component should trust props directly rather than second-guessing them. Update test to reflect correct component responsibility boundary.
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/ModuleForm.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:934">
P2: Test name and assertion conflict: case says error should be hidden while assertion requires it to be present.</violation>
</file>
<file name="frontend/src/components/ModuleForm.tsx">
<violation number="1" location="frontend/src/components/ModuleForm.tsx:439">
P2: Autocomplete now shows required-project validation while user is still typing, due to removed `isTyping` guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
|
|
||
| const displayError = errorMessage |
Contributor
There was a problem hiding this comment.
P2: Autocomplete now shows required-project validation while user is still typing, due to removed isTyping guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/ModuleForm.tsx, line 439:
<comment>Autocomplete now shows required-project validation while user is still typing, due to removed `isTyping` guard.</comment>
<file context>
@@ -436,9 +436,8 @@ export const ProjectSelector = ({
- const isTyping = inputValue.trim() !== '' && !value
- const displayError = isTyping ? undefined : errorMessage
- const shouldShowInvalid = isTyping ? false : isInvalid
+ const displayError = errorMessage
+ const shouldShowInvalid = isInvalid
</file context>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ModuleForm.tsx (1)
242-265:⚠️ Potential issue | 🟡 MinorRestore
labelPlacement="outside-left"to match other Select components.The
labelPlacementprop is missing from this Select component. All other Select components in the codebase (SortBy.tsx, CountryFilter.tsx, and issues/page.tsx) uselabelPlacement="outside-left". Without this prop, the label will default to"inside"instead, creating a visual inconsistency.Add
labelPlacement="outside-left"to align with the existing pattern unless this deviation is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 242 - 265, The Select for experienceLevel is missing the labelPlacement prop causing inconsistent label positioning; update the Select component instance (the JSX Select with id="experienceLevel" and onSelectionChange={handleSelectChange}) to include labelPlacement="outside-left" so it matches other Select usages (e.g., SortBy, CountryFilter) and restores consistent label placement.
♻️ Duplicate comments (1)
frontend/src/components/ModuleForm.tsx (1)
479-497:⚠️ Potential issue | 🟡 MinorDropdown still does not close when clicking outside.
The custom combobox implementation handles Escape and item selection but lacks a click-outside handler. Users clicking elsewhere on the page will leave the dropdown open, which differs from standard combobox behavior.
This was flagged in a previous review and remains unaddressed.
Proposed fix using useEffect with document listener
Add a ref and click-outside effect:
+ import { useRef } from 'react' export const ProjectSelector = ({ // ... }: ProjectSelectorProps) => { const client = useApolloClient() + const containerRef = useRef<HTMLDivElement>(null) const [inputValue, setInputValue] = useState(defaultName || '') // ... + useEffect(() => { + const handleClickOutside = (e: MouseEvent) => { + if (containerRef.current && !containerRef.current.contains(e.target as Node)) { + setIsOpen(false) + setFocusedIndex(-1) + } + } + if (isOpen) { + document.addEventListener('mousedown', handleClickOutside) + } + return () => document.removeEventListener('mousedown', handleClickOutside) + }, [isOpen]) return ( - <div data-testid="autocomplete" className="relative w-full min-w-0" ...> + <div ref={containerRef} data-testid="autocomplete" className="relative w-full min-w-0" ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 479 - 497, Add a click-outside handler so the combobox closes when clicking elsewhere: create a ref (e.g., dropdownRef) and attach it to the rendered container (the <ul> that is shown when isOpen && items.length > 0), then add a useEffect that registers a document mousedown/touchstart listener which checks if the event target is outside dropdownRef.current and, if so, calls the component state updater to close the menu (the same setter that controls isOpen) and resets focus (setFocusedIndex(-1)); clean up the listener in the effect return. Ensure the handler does not run when the menu is already closed and works alongside existing handleSelect calls.
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
28-109: Consider removing the unused Autocomplete mock.The
AutocompleteandAutocompleteItemmock is no longer exercised sinceProjectSelectornow renders native HTML elements instead of using@heroui/reactcomponents. The tests interact with the actualdata-testidattributes from the native elements (autocomplete-input,autocomplete-item,autocomplete-clear).Keeping this mock adds confusion about which elements tests are actually interacting with.
♻️ Proposed cleanup
// Mock heroui components jest.mock('@heroui/react', () => ({ - Autocomplete: ({ - children, - inputValue, - _selectedKey, - onInputChange, - onSelectionChange, - isInvalid, - errorMessage, - isLoading, - label, - id, - }: { - children: React.ReactNode - inputValue?: string - _selectedKey?: string | null - onInputChange?: (value: string) => void - onSelectionChange?: (key: React.Key | Set<React.Key> | 'all') => void - isInvalid?: boolean - errorMessage?: string - isLoading?: boolean - label?: string - id?: string - }) => ( - <div data-testid="autocomplete"> - <label htmlFor={id}>{label}</label> - <input - id={id} - data-testid="autocomplete-input" - value={inputValue || ''} - data-selected-key={_selectedKey ?? ''} - onChange={(e) => onInputChange?.(e.target.value)} - data-loading={isLoading} - data-invalid={isInvalid} - /> - {errorMessage && <span data-testid="autocomplete-error">{errorMessage}</span>} - <div data-testid="autocomplete-items">{children}</div> - <button - type="button" - data-testid="autocomplete-select-item" - onClick={() => onSelectionChange?.(new Set(['project-1']))} - > - Select Project 1 - </button> - <button - type="button" - data-testid="autocomplete-select-all" - onClick={() => onSelectionChange?.('all')} - > - Select All - </button> - <button - type="button" - data-testid="autocomplete-clear" - onClick={() => { - onInputChange?.('') - onSelectionChange?.(null) - }} - > - Clear Selection - </button> - <button - type="button" - data-testid="autocomplete-select-single" - onClick={() => onSelectionChange?.('project-1')} - > - Select Single Key - </button> - </div> - ), - AutocompleteItem: ({ - children, - textValue, - }: { - children: React.ReactNode - textValue?: string - }) => ( - <div data-testid="autocomplete-item" data-text-value={textValue}> - {children} - </div> - ), + // ProjectSelector now uses native HTML elements, no HeroUI Autocomplete mocks needed }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ModuleForm.test.tsx` around lines 28 - 109, Remove the unused jest.mock for '@heroui/react' in ModuleForm.test.tsx: delete the mocked Autocomplete and AutocompleteItem implementation (the entire jest.mock(...) block) since ProjectSelector now renders native elements and tests target native data-testid attributes (e.g., "autocomplete-input", "autocomplete-item", "autocomplete-clear"); ensure no other tests in this file rely on the mock after removal and run the tests to confirm nothing else breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 242-265: The Select for experienceLevel is missing the
labelPlacement prop causing inconsistent label positioning; update the Select
component instance (the JSX Select with id="experienceLevel" and
onSelectionChange={handleSelectChange}) to include labelPlacement="outside-left"
so it matches other Select usages (e.g., SortBy, CountryFilter) and restores
consistent label placement.
---
Duplicate comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 479-497: Add a click-outside handler so the combobox closes when
clicking elsewhere: create a ref (e.g., dropdownRef) and attach it to the
rendered container (the <ul> that is shown when isOpen && items.length > 0),
then add a useEffect that registers a document mousedown/touchstart listener
which checks if the event target is outside dropdownRef.current and, if so,
calls the component state updater to close the menu (the same setter that
controls isOpen) and resets focus (setFocusedIndex(-1)); clean up the listener
in the effect return. Ensure the handler does not run when the menu is already
closed and works alongside existing handleSelect calls.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 28-109: Remove the unused jest.mock for '@heroui/react' in
ModuleForm.test.tsx: delete the mocked Autocomplete and AutocompleteItem
implementation (the entire jest.mock(...) block) since ProjectSelector now
renders native elements and tests target native data-testid attributes (e.g.,
"autocomplete-input", "autocomplete-item", "autocomplete-clear"); ensure no
other tests in this file rely on the mock after removal and run the tests to
confirm nothing else breaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4374ff2e-7822-4dab-8848-ab8682d59a87
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/src/components/ModuleForm.tsx
ProjectSelector trusts isInvalid/errorMessage props from parent directly. Test now accurately describes the scenario being covered.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/ModuleForm.tsx (1)
469-475: Clarify or remove the hidden clear button.This button is permanently hidden via
className="hidden"and cannot be activated by users. If it exists solely for testing, consider whether the tests could instead interact with the component's actual API (e.g., simulating user clearing the input). If it's a placeholder for future functionality, add a comment explaining the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ModuleForm.tsx` around lines 469 - 475, The clear button is permanently hidden (className="hidden") and either should be removed or clarified; either delete this button element if it's only used for tests and update tests to simulate clearing via the input API, or keep it but add an explicit comment explaining its purpose and make it accessible (remove "hidden" and wire it into the UI) so users can trigger the handlers (setInputValue, setItems, setIsOpen, onProjectChange); update tests accordingly to use the real interaction or the preserved button and ensure the handlers are exercised as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/ModuleForm.tsx`:
- Around line 469-475: The clear button is permanently hidden
(className="hidden") and either should be removed or clarified; either delete
this button element if it's only used for tests and update tests to simulate
clearing via the input API, or keep it but add an explicit comment explaining
its purpose and make it accessible (remove "hidden" and wire it into the UI) so
users can trigger the handlers (setInputValue, setItems, setIsOpen,
onProjectChange); update tests accordingly to use the real interaction or the
preserved button and ensure the handlers are exercised as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66942935-c67e-444f-8f6d-bdc886c322af
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ModuleForm.test.tsxfrontend/src/components/ModuleForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/ModuleForm.test.tsx
…elector Button is intentionally hidden from users and exists solely for test automation.
arkid15r
requested changes
Mar 31, 2026
Collaborator
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please take care of the automated tools' comments first.
Also I don't think all these changes are relevant and isn't something that was added by AI and not verified by a human.
Collaborator
There was a problem hiding this comment.
Why do we need these changes?
Contributor
Author
There was a problem hiding this comment.
On jest.config.ts:
HeroUI v3 ships as ESM, which Jest can't handle natively. Without these changes, 3 test suites fail with a module resolution error. The transform change forces CommonJS output, the moduleNameMapper points Jest to the CJS mock, and the transformIgnorePatterns tells Jest to transpile the HeroUI packages instead of skipping them. I verified this by reverting just the jest.config.ts changes and running the full test suite locally.
On provider.tsx:
The comment block was accidentally removed during the HeroUIProvider cleanup. Restored it in the latest commit.
|
|
||
| // <AppInitializer> is a component that initializes the Django session. | ||
| // It ensures the session is synced with Django when the app starts. | ||
| // AppInitializer is mounted once. Its job is to call useDjangoSession(), |
…pInitializer comments
|
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.
Resolves #4394
Proposed change
Upgrades
@heroui/reactfrom v2.8.10 to v3.0.1 and migrates all affected components to the new API.Package updates
@heroui/reactto3.0.1@heroui/button,@heroui/tooltip, etc.) — consolidated into@heroui/reactin v3Component migrations
HeroUIProviderwrapper (dropped in v3, React 19 handles it natively)Navbar/NavbarItem/NavbarContentwith plain HTML<nav>(removed in v3)BreadcrumbItem→BreadcrumbsItem, removeitemClassespropInputlabel/labelPlacement with plain<label>elementsAutocomplete/AutocompleteItemwith plain HTML comboboxinitialPage/onChangeremoved in v3)variant='solid'withvariant='primary', removeDropdownSectiontitle propTest infrastructure
jest.config.tsto handle ESM modules from@heroui/reactv3__mocks__/@heroui/react.jsCJS mock for Jest compatibilityChecklist
make check-testlocally: all warnings addressed, tests passed