Fix Chapters page responsiveness and country filter reset behavior#4358
Open
anurag2787 wants to merge 7 commits intoOWASP:mainfrom
Open
Fix Chapters page responsiveness and country filter reset behavior#4358anurag2787 wants to merge 7 commits intoOWASP:mainfrom
anurag2787 wants to merge 7 commits intoOWASP:mainfrom
Conversation
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:
WalkthroughRefactors SearchPageLayout into separate mobile/desktop render paths, adjusts CountryFilter selection and responsive sizing, updates Search/SearchPageLayout padding, and modifies multiple unit tests to use plural queries, visibility-based selection, and DOM/class-based assertions instead of single-element or data-testid checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 8 files
Confidence score: 3/5
- There is a concrete user-facing regression risk in
frontend/src/components/SearchPageLayout.tsx: the mobile controls wrapper renders unconditionally, which can introduce unintended emptymt-2 mb-4spacing on pages without mobile controls. - The most severe reliability concern is in
frontend/src/components/CountryFilter.tsx, whereonSelectionChangemay passnullbut the value is force-cast tostring, creating a plausible runtime contract violation foronCountryChange(country: string)consumers. - Several test updates in
frontend/__tests__/unit/pages/Users.test.tsx,frontend/__tests__/unit/pages/Projects.test.tsx, andfrontend/__tests__/unit/components/SearchPageLayout.test.tsxweaken visibility/styling assertions, reducing regression detection for responsive behavior and class-level UI expectations. - Pay close attention to
frontend/src/components/SearchPageLayout.tsxandfrontend/src/components/CountryFilter.tsx- layout spacing and null-handling behavior are the main merge risks.
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/pages/Projects.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Projects.test.tsx:83">
P2: The new fallback (`|| searchInputs[0]`) makes the visibility check ineffective: the test will pass as long as any matching input exists, even if none are visible. This weakens regression protection for the responsive SearchBar behavior.</violation>
</file>
<file name="frontend/__tests__/unit/pages/Users.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Users.test.tsx:74">
P2: The new visibility-selection logic still only asserts DOM presence, so hidden search inputs can pass the test and mask responsive regressions.</violation>
</file>
<file name="frontend/src/components/SearchPageLayout.tsx">
<violation number="1" location="frontend/src/components/SearchPageLayout.tsx:100">
P2: Mobile controls wrapper renders unconditionally, leaving empty `mt-2 mb-4` spacing on pages without mobile filter/sort controls.</violation>
</file>
<file name="frontend/__tests__/unit/components/SearchPageLayout.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/SearchPageLayout.test.tsx:460">
P3: The `rounded-l-none` styling regression check was removed: the test now only asserts that "Sort" text renders, so it will still pass even if the `rounded-l-none` class is missing or moved, weakening the intended styling regression coverage.</violation>
</file>
<file name="frontend/src/components/CountryFilter.tsx">
<violation number="1" location="frontend/src/components/CountryFilter.tsx:36">
P2: `onSelectionChange` can receive `null` on clear, but the handler force-casts `key` to string and forwards it to `onCountryChange`, violating the `(country: string)` contract and risking runtime issues in parents.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 3
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/SearchPageLayout.test.tsx (1)
539-542: Consider usingString.rawfor cleaner CSS selector escaping.The escaped backslashes work but are harder to read.
String.rawprovides a cleaner syntax.♻️ Optional refactor
const flexRow = container.querySelector( - 'div.hidden.w-full.items-center.justify-center.md\\:flex.gap-0' + String.raw`div.hidden.w-full.items-center.justify-center.md\:flex.gap-0` )Apply similar changes to lines 563-565 and 589.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/SearchPageLayout.test.tsx` around lines 539 - 542, Replace the hard-to-read escaped CSS selector strings passed to container.querySelector (e.g., the selector used when assigning flexRow from container.querySelector) with String.raw template literals so you can write the selector without double backslashes; update the three occurrences referenced (the selector setting flexRow and the selectors at the later lines called out in the comment) to use String.raw`div.hidden.w-full.items-center.justify-center.md\:flex.gap-0` (and analogous selectors) so the escaping is clearer while keeping the exact selector text.frontend/__tests__/unit/pages/Chapters.test.tsx (1)
120-126: Potential runtime error whenclosest()returns null for both selectors.If an input element has neither
.md:hiddennor.md:flexancestor,closestreturnsnullandwindow.getComputedStyle(null)throws a TypeError. The ternary at line 124 only handles the case whenclosestreturns a truthy value, but the||at line 123 means you're passing whichever is truthy (or null if both are null) togetComputedStyle.🔧 Proposed fix
await waitFor(() => { const searchInputs = screen.getAllByPlaceholderText('Search for chapters...') const visibleInput = searchInputs.find((input) => { const closest = input.closest('.md\\:hidden') || input.closest('.md\\:flex') - return closest ? window.getComputedStyle(closest).display !== 'none' : true + return closest ? globalThis.getComputedStyle(closest).display !== 'none' : true }) || searchInputs[0] expect(visibleInput).toBeInTheDocument()The logic is actually safe because when
closestis null, it returnstrue(treating the input as visible). However, per static analysis, preferglobalThisoverwindowfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/Chapters.test.tsx` around lines 120 - 126, The test's visible input selection uses input.closest(...) then calls window.getComputedStyle which can throw if closest returns null; update the logic in the searchInputs / visibleInput block (the code that uses getAllByPlaceholderText and closest) to first check for a non-null ancestor before calling getComputedStyle and fall back to treating the input as visible when no ancestor is found, and replace window.getComputedStyle with globalThis.getComputedStyle for consistency.
🤖 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/__tests__/unit/pages/Contribute.test.tsx`:
- Around line 146-154: The test computes styles on input.closest(...) without
guarding for null, which can throw in getComputedStyle; update the
searchInputs/visibleInput logic so when calling window.getComputedStyle you
first check closest('.md\\:hidden') and closest('.md\\:flex') for truthiness
(e.g., store the result in a variable like ancestor and only call
getComputedStyle(ancestor) if ancestor !== null), and fall back to treating that
input as hidden/visible appropriately before selecting visibleInput used by
fireEvent.change; adjust the visibleInput selection to handle the no-ancestor
case safely.
In `@frontend/src/components/CountryFilter.tsx`:
- Line 46: The Tailwind classes `w-42` and `min-w-42` in the CountryFilter
component are invalid and ignored; remove those tokens from the class
definitions (look for the `base: 'w-42 md:w-52'` entry and the similar
`min-w-42` occurrence) and replace them with a valid spacing utility such as
`w-40` or `w-44` (or alternatively add a custom 42 value to tailwind.config.mjs
if you truly need 42). Ensure both occurrences mentioned in the comment are
updated so only valid Tailwind classes remain.
- Around line 34-38: The onSelectionChange handler currently force-casts key to
string which hides null clear events; update the handler in CountryFilter (the
selectedKey/onSelectionChange block) to explicitly check for key === null and
call onCountryChange('') for clears, otherwise treat key as a string and map the
'__all__' sentinel to ''. Remove the unsafe "key as string" cast so null is
handled explicitly and keep the onCountryChange contract (string) intact.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/SearchPageLayout.test.tsx`:
- Around line 539-542: Replace the hard-to-read escaped CSS selector strings
passed to container.querySelector (e.g., the selector used when assigning
flexRow from container.querySelector) with String.raw template literals so you
can write the selector without double backslashes; update the three occurrences
referenced (the selector setting flexRow and the selectors at the later lines
called out in the comment) to use
String.raw`div.hidden.w-full.items-center.justify-center.md\:flex.gap-0` (and
analogous selectors) so the escaping is clearer while keeping the exact selector
text.
In `@frontend/__tests__/unit/pages/Chapters.test.tsx`:
- Around line 120-126: The test's visible input selection uses
input.closest(...) then calls window.getComputedStyle which can throw if closest
returns null; update the logic in the searchInputs / visibleInput block (the
code that uses getAllByPlaceholderText and closest) to first check for a
non-null ancestor before calling getComputedStyle and fall back to treating the
input as visible when no ancestor is found, and replace window.getComputedStyle
with globalThis.getComputedStyle for consistency.
🪄 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: 78f2b96f-3bf3-46c6-b1fc-306ef6148a43
📒 Files selected for processing (8)
frontend/__tests__/unit/components/SearchPageLayout.test.tsxfrontend/__tests__/unit/pages/Chapters.test.tsxfrontend/__tests__/unit/pages/Committees.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/Projects.test.tsxfrontend/__tests__/unit/pages/Users.test.tsxfrontend/src/components/CountryFilter.tsxfrontend/src/components/SearchPageLayout.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 8 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/pages/Chapters.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Chapters.test.tsx:124">
P2: Responsive visibility assertion relies on Tailwind/media-query CSS, but Jest mocks CSS with identity-obj-proxy so computed styles won’t reflect breakpoints. This makes the visibility check unreliable and can pass even when real responsive behavior is wrong.</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.
♻️ Duplicate comments (1)
frontend/src/components/CountryFilter.tsx (1)
35-38:⚠️ Potential issue | 🔴 CriticalType safety violation:
key as stringdoes not handle null from clear button.When the clear button is clicked, HeroUI Autocomplete emits
nullinonSelectionChange. The castkey as stringis a type violation—nullpasses through and reachesonCountryChange(null), which expects astring. While this may work at runtime (the input clears), it violates the component's type contract and is fragile.Explicitly handle the null case:
♻️ Type-safe fix
onSelectionChange={(key) => { + if (key == null) { + onCountryChange('') + return + } - const countryKey = key as string + const countryKey = String(key) onCountryChange(countryKey === '__all__' ? '' : countryKey) }}This preserves the current behavior (clear doesn't reset to "All Countries") while eliminating the type violation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CountryFilter.tsx` around lines 35 - 38, The onSelectionChange handler in CountryFilter casts key with "key as string" which lets null slip through to onCountryChange (violating its string type); update the handler to explicitly handle key === null by calling onCountryChange('') and otherwise treat key as a string and apply the existing "__all__" check (i.e., if key === '__all__' call onCountryChange('') else call onCountryChange(key)). This keeps the current clear behavior (input clears but doesn’t select "All Countries") while preserving type safety for the CountryFilter component and its onCountryChange callback.
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/SearchPageLayout.test.tsx (1)
594-595: UseString.rawfor consistency with other selectors.Static analysis correctly flags that
String.rawshould be used here to avoid manual escaping, matching the pattern used elsewhere in this file (lines 545, 569).♻️ Proposed fix
- const filterWrapper = container.querySelector('div[class*="\\[&>div\\]:rounded-r-none"]') + const filterWrapper = container.querySelector(String.raw`div[class*="\[&>div\]:rounded-r-none"]`) expect(filterWrapper).toBeInTheDocument()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/SearchPageLayout.test.tsx` around lines 594 - 595, Replace the manually escaped selector string used in container.querySelector with a String.raw template literal to match the pattern used elsewhere: update the call that assigns filterWrapper (variable filterWrapper and the expression container.querySelector(...)) to use String.raw`div[class*="[&>div]:rounded-r-none"]` (remove the current backslash-escaped brackets) and run the unit tests to verify the selector still finds the element.frontend/__tests__/unit/pages/Contribute.test.tsx (1)
146-154: Consider using a fallback instead of non-null assertion.The non-null assertion
visibleInput!at line 154 is flagged by static analysis. While the preceding assertions verify the input exists, adding a fallback would be more defensive and consistent with how other similar patterns handle this.♻️ Suggested improvement
const visibleInput = searchInputs.find((input) => { const closest = input.closest(String.raw`.md\:hidden`) || input.closest(String.raw`.md\:flex`) return closest ? globalThis.getComputedStyle(closest).display !== 'none' : true - }) + }) || searchInputs[0] expect(visibleInput).toBeDefined() expect(visibleInput).toBeVisible() - fireEvent.change(visibleInput!, { target: { value: '' } }) + fireEvent.change(visibleInput, { target: { value: '' } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/Contribute.test.tsx` around lines 146 - 154, The test currently uses a non-null assertion on visibleInput when calling fireEvent.change; replace this with a defensive fallback/guard: after computing visibleInput (from getAllByPlaceholderText and the find callback), assert or throw if it's null (e.g., if (!visibleInput) throw new Error('Expected visible input');) or assign const input = visibleInput ?? /* fallback element or fail */ and then call fireEvent.change(input, { target: { value: '' } }); update references to use input instead of visibleInput! to avoid the non-null assertion while preserving the existing assertions (expect(visibleInput).toBeDefined(), expect(visibleInput).toBeVisible()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/CountryFilter.tsx`:
- Around line 35-38: The onSelectionChange handler in CountryFilter casts key
with "key as string" which lets null slip through to onCountryChange (violating
its string type); update the handler to explicitly handle key === null by
calling onCountryChange('') and otherwise treat key as a string and apply the
existing "__all__" check (i.e., if key === '__all__' call onCountryChange('')
else call onCountryChange(key)). This keeps the current clear behavior (input
clears but doesn’t select "All Countries") while preserving type safety for the
CountryFilter component and its onCountryChange callback.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/SearchPageLayout.test.tsx`:
- Around line 594-595: Replace the manually escaped selector string used in
container.querySelector with a String.raw template literal to match the pattern
used elsewhere: update the call that assigns filterWrapper (variable
filterWrapper and the expression container.querySelector(...)) to use
String.raw`div[class*="[&>div]:rounded-r-none"]` (remove the current
backslash-escaped brackets) and run the unit tests to verify the selector still
finds the element.
In `@frontend/__tests__/unit/pages/Contribute.test.tsx`:
- Around line 146-154: The test currently uses a non-null assertion on
visibleInput when calling fireEvent.change; replace this with a defensive
fallback/guard: after computing visibleInput (from getAllByPlaceholderText and
the find callback), assert or throw if it's null (e.g., if (!visibleInput) throw
new Error('Expected visible input');) or assign const input = visibleInput ?? /*
fallback element or fail */ and then call fireEvent.change(input, { target: {
value: '' } }); update references to use input instead of visibleInput! to avoid
the non-null assertion while preserving the existing assertions
(expect(visibleInput).toBeDefined(), expect(visibleInput).toBeVisible()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00f6cb17-f8f3-4dcb-98dd-dc909a7418d9
📒 Files selected for processing (8)
frontend/__tests__/unit/components/SearchPageLayout.test.tsxfrontend/__tests__/unit/pages/Chapters.test.tsxfrontend/__tests__/unit/pages/Committees.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/__tests__/unit/pages/Projects.test.tsxfrontend/__tests__/unit/pages/Users.test.tsxfrontend/src/components/CountryFilter.tsxfrontend/src/components/SearchPageLayout.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/unit/pages/Committees.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/tests/unit/pages/Users.test.tsx
- frontend/tests/unit/pages/Projects.test.tsx
- frontend/src/components/SearchPageLayout.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 4 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/CountryFilter.tsx">
<violation number="1" location="frontend/src/components/CountryFilter.tsx:37">
P2: Clear action is ignored in a controlled country filter, so parent state can remain stale and filtering may not reset.</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.
♻️ Duplicate comments (1)
frontend/src/components/CountryFilter.tsx (1)
34-42:⚠️ Potential issue | 🟠 MajorClear now leaves the parent filter stuck on the previous country.
selectedKeyis still controlled byselectedCountry, andfrontend/src/app/chapters/page.tsx:117-122only updates that value throughonCountryChange. Returning onkey == nullmeans the clear action never reaches the parent, so the old country can remain applied and any rerender can snap the selection back. The clear path needs a distinct propagated state instead of being ignored.💡 Suggested direction
selectedKey={selectedCountry === '' ? '__all__' : selectedCountry} onSelectionChange={(key) => { if (key == null) { - return // Don't update on clear - keeps input empty + onCountryChange(null) + return } - const countryKey = key as string + const countryKey = String(key) onCountryChange(countryKey === '__all__' ? '' : countryKey) }}This also needs the
CountryFiltercontract and the parentselectedCountrystate to represent a cleared value distinctly (for examplenull), or a separateinputValuestate if you want to keep the selected filter asstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CountryFilter.tsx` around lines 34 - 42, The clear action is being swallowed because onSelectionChange returns early when key == null, leaving the parent selectedCountry unchanged; update the CountryFilter contract and handlers so clearing propagates: modify onSelectionChange to call onCountryChange with a distinct cleared value (e.g., null or '') instead of returning when key == null, and ensure the parent state (selectedCountry in the parent component and any usage in frontend/src/app/chapters/page.tsx) accepts and treats that distinct cleared value as “no filter” (or alternatively introduce a separate inputValue in CountryFilter to track UI clearing and continue to call onCountryChange('__all__' or null) so selectedKey (controlled by selectedCountry) stays in sync). Ensure selectedKey, onSelectionChange, and onCountryChange consistently use the chosen cleared sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/CountryFilter.tsx`:
- Around line 34-42: The clear action is being swallowed because
onSelectionChange returns early when key == null, leaving the parent
selectedCountry unchanged; update the CountryFilter contract and handlers so
clearing propagates: modify onSelectionChange to call onCountryChange with a
distinct cleared value (e.g., null or '') instead of returning when key == null,
and ensure the parent state (selectedCountry in the parent component and any
usage in frontend/src/app/chapters/page.tsx) accepts and treats that distinct
cleared value as “no filter” (or alternatively introduce a separate inputValue
in CountryFilter to track UI clearing and continue to call
onCountryChange('__all__' or null) so selectedKey (controlled by
selectedCountry) stays in sync). Ensure selectedKey, onSelectionChange, and
onCountryChange consistently use the chosen cleared sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a63e8c2-1d90-4557-a061-79a3ca572b1c
📒 Files selected for processing (4)
frontend/__tests__/unit/components/SearchPageLayout.test.tsxfrontend/__tests__/unit/pages/Chapters.test.tsxfrontend/__tests__/unit/pages/Contribute.test.tsxfrontend/src/components/CountryFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/tests/unit/pages/Chapters.test.tsx
- frontend/tests/unit/pages/Contribute.test.tsx
- frontend/tests/unit/components/SearchPageLayout.test.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/CountryFilter.tsx (1)
33-37:⚠️ Potential issue | 🟠 MajorClearing the field is not propagating state reset to the parent filter.
At Line 33-34, returning on
nullskipsonCountryChange, soselectedCountryinfrontend/src/app/chapters/page.tsxis not reset and downstream filter/refetch logic stays stale.Proposed fix
- selectedKey={selectedCountry} + selectedKey={selectedCountry || null} onSelectionChange={(key) => { if (key == null) { - return + onCountryChange('') + return } - const countryKey = key as string - onCountryChange(countryKey === '__all__' ? '' : countryKey) + onCountryChange(String(key)) }}For `@heroui/autocomplete` v2.3.34, confirm: 1) `onSelectionChange` emits `null` when the clear action is used. 2) `selectedKey={null}` is the intended controlled "empty selection" state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CountryFilter.tsx` around lines 33 - 37, The handler currently returns early when the autocomplete selection handler receives null, preventing propogation of the reset to the parent; update the onSelectionChange logic in the CountryFilter component so that when key === null it calls onCountryChange('') (treating null as cleared selection) instead of returning, preserving the existing branch that converts '__all__' to '' for non-null keys; ensure this uses the same onCountryChange symbol so the parent selectedCountry is reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/CountryFilter.tsx`:
- Around line 33-37: The handler currently returns early when the autocomplete
selection handler receives null, preventing propogation of the reset to the
parent; update the onSelectionChange logic in the CountryFilter component so
that when key === null it calls onCountryChange('') (treating null as cleared
selection) instead of returning, preserving the existing branch that converts
'__all__' to '' for non-null keys; ensure this uses the same onCountryChange
symbol so the parent selectedCountry is reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4e5a44d-8610-44ff-9f09-f05a8605d599
📒 Files selected for processing (1)
frontend/src/components/CountryFilter.tsx
Contributor
Author
arkid15r
reviewed
Mar 25, 2026
Collaborator
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Thanks for improving that @anurag2787
Would it be possible to align the search bar width with the components below (see the screenshot)
anurag2787
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
15e141a
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/Search.tsx">
<violation number="1" location="frontend/src/components/Search.tsx:84">
P2: Unconditional `md:py-4` overrides the `p-0` branch at medium+ breakpoints, reintroducing vertical padding for `rounded-none`/`rounded-r-none` inline layouts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
Hi @arkid15r i have updated it could you please re-review it Thanks! |
kasya
requested changes
Mar 27, 2026
Collaborator
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 thanks for working on this! I like the update for keyboard navigation with backspace 👍🏼
Noticed a couple of issues with the UI on some screen sizes though ⬇️
| ))} | ||
| </div> | ||
|
|
||
| {/* Mobile layout */} |
Collaborator
There was a problem hiding this comment.
Can we update this so there's no gap between the two on the mobile screens? This way we can keep border-r-0 on the filter too.
Right now, it looks weird on larger screens and I feel like that would make more sense for these 2 components to fill the whole width.
anurag2787
force-pushed
the
fix/responsive-chapter-page
branch
from
de8869d to
8ca539f
Compare
|
Contributor
Author
|
Hi @kasya i have updated it but keeped the layout same so that UI doesnot breaks in medium screen |
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
3 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.
Proposed change
This PR improves the Chapters page UI on smaller screens and fixes an issue with the country filter input.
Resolves #4282
Screencast.from.2026-03-25.01-09-03.webm
Checklist
make check-testlocally: all warnings addressed, tests passed