fix(channel): media viewer showing navigation for single item#2825
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/core/component/Lightbox.tsx (1)
269-342: 🛠️ Refactor suggestion | 🟠 MajorDead
navVisiblehelper — the opacity class is alwaysopacity-100.
navVisibleis hard-coded to() => true, so the ternaries on lines 317 and 333 (and 350 for the index label) always resolve to'opacity-100', and the'opacity-0 pointer-events-none'branch is unreachable. Now that rendering is gated by<Show when={props.navigationVisible !== false}>, this leftover fade logic is dead code and adds noise. Also, each arrow is wrapped in its own<Show>with the same condition — consolidating to a single<Show>makes the intent clearer.♻️ Proposed cleanup
- const navButtonClass = - 'absolute top-1/2 -translate-y-1/2 bg-dialog backdrop-blur-sm rounded-lg border border-edge p-2 shadow-md hover:bg-button transition-opacity duration-300 disabled:cursor-not-allowed disabled:opacity-50'; - - const navVisible = () => true; + const navButtonClass = + 'absolute top-1/2 -translate-y-1/2 bg-dialog backdrop-blur-sm rounded-lg border border-edge p-2 shadow-md hover:bg-button disabled:cursor-not-allowed disabled:opacity-50'; @@ {/* Nav arrows — desktop only */} - <Show when={!isMobile()}> - <Show when={props.navigationVisible !== false}> - <button - class={cn( - navButtonClass, - 'left-4', - navVisible() ? 'opacity-100' : 'opacity-0 pointer-events-none' - )} - style={{ 'z-index': stackingContext.zModal + 1 }} - onClick={props.onPrevious} - disabled={!props.onPrevious} - aria-label="Previous image" - > - <ChevronLeftIcon class="w-5 h-5 text-ink" /> - </button> - </Show> - - <Show when={props.navigationVisible !== false}> - <button - class={cn( - navButtonClass, - 'right-4', - navVisible() ? 'opacity-100' : 'opacity-0 pointer-events-none' - )} - style={{ 'z-index': stackingContext.zModal + 1 }} - onClick={props.onNext} - disabled={!props.onNext} - aria-label="Next image" - > - <ChevronRightIcon class="w-5 h-5 text-ink" /> - </button> - </Show> - </Show> + <Show when={!isMobile() && props.navigationVisible !== false}> + <button + class={cn(navButtonClass, 'left-4')} + style={{ 'z-index': stackingContext.zModal + 1 }} + onClick={props.onPrevious} + disabled={!props.onPrevious} + aria-label="Previous image" + > + <ChevronLeftIcon class="w-5 h-5 text-ink" /> + </button> + + <button + class={cn(navButtonClass, 'right-4')} + style={{ 'z-index': stackingContext.zModal + 1 }} + onClick={props.onNext} + disabled={!props.onNext} + aria-label="Next image" + > + <ChevronRightIcon class="w-5 h-5 text-ink" /> + </button> + </Show>Apply the analogous cleanup at lines 346–358 for the index indicator (drop
navVisible()from the class).As per coding guidelines: "Eliminate redundant code and abstractions" and "Reduce unnecessary complexity and nesting in code logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/Lightbox.tsx` around lines 269 - 342, navVisible() is dead code and the arrow opacity ternaries are unreachable; remove the navVisible helper and simplify the markup by consolidating the two separate <Show when={props.navigationVisible !== false}> wrappers into a single <Show> that wraps both navigation buttons (and the index indicator), and update the button/index class usage to drop the navVisible() ternaries (use navButtonClass plus the static positioning and desired opacity classes, e.g. 'left-4 opacity-100' and 'right-4 opacity-100'); update references in the Lightbox component (navVisible, navButtonClass, the buttons using onPrevious/onNext, and the index label) accordingly so there’s no dead helper or redundant <Show> nesting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/channel/Media/MediaViewerDialog.tsx`:
- Around line 62-84: The two identical conditional wrappers using Show (Show
when={props.navigationVisible !== false}) should be merged into a single Show
that wraps both navigation buttons; locate the Show blocks around the left and
right nav buttons (the buttons using navButtonClass, ChevronLeftIcon and
ChevronRightIcon, stackingContext.zModal, onPrevious/onNext and disabled checks)
and move both buttons inside one Show with the same condition so behavior and
props (aria-label, onClick, disabled, style) remain unchanged.
---
Outside diff comments:
In `@js/app/packages/core/component/Lightbox.tsx`:
- Around line 269-342: navVisible() is dead code and the arrow opacity ternaries
are unreachable; remove the navVisible helper and simplify the markup by
consolidating the two separate <Show when={props.navigationVisible !== false}>
wrappers into a single <Show> that wraps both navigation buttons (and the index
indicator), and update the button/index class usage to drop the navVisible()
ternaries (use navButtonClass plus the static positioning and desired opacity
classes, e.g. 'left-4 opacity-100' and 'right-4 opacity-100'); update references
in the Lightbox component (navVisible, navButtonClass, the buttons using
onPrevious/onNext, and the index label) accordingly so there’s no dead helper or
redundant <Show> nesting.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ac218ec4-9256-46f6-abb9-64305c18eea5
📒 Files selected for processing (2)
js/app/packages/channel/Media/MediaViewerDialog.tsxjs/app/packages/core/component/Lightbox.tsx
| <Show when={props.navigationVisible !== false}> | ||
| <button | ||
| class={cn(navButtonClass, 'left-4')} | ||
| style={{ 'z-index': stackingContext.zModal + 1 }} | ||
| onClick={props.onPrevious} | ||
| disabled={!props.onPrevious} | ||
| aria-label="Previous media" | ||
| > | ||
| <ChevronLeftIcon class="h-5 w-5 text-ink" /> | ||
| </button> | ||
| </Show> | ||
|
|
||
| <button | ||
| class={cn(navButtonClass, 'right-4')} | ||
| style={{ 'z-index': stackingContext.zModal + 1 }} | ||
| onClick={props.onNext} | ||
| disabled={!props.onNext} | ||
| aria-label="Next media" | ||
| > | ||
| <ChevronRightIcon class="h-5 w-5 text-ink" /> | ||
| </button> | ||
| <Show when={props.navigationVisible !== false}> | ||
| <button | ||
| class={cn(navButtonClass, 'right-4')} | ||
| style={{ 'z-index': stackingContext.zModal + 1 }} | ||
| onClick={props.onNext} | ||
| disabled={!props.onNext} | ||
| aria-label="Next media" | ||
| > | ||
| <ChevronRightIcon class="h-5 w-5 text-ink" /> | ||
| </button> | ||
| </Show> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate the two <Show> blocks with identical conditions.
Both nav buttons are wrapped in separate <Show when={props.navigationVisible !== false}> blocks with the same condition. Merging into a single <Show> reduces duplication and makes the intent clearer.
♻️ Proposed fix
- <Show when={props.navigationVisible !== false}>
- <button
- class={cn(navButtonClass, 'left-4')}
- style={{ 'z-index': stackingContext.zModal + 1 }}
- onClick={props.onPrevious}
- disabled={!props.onPrevious}
- aria-label="Previous media"
- >
- <ChevronLeftIcon class="h-5 w-5 text-ink" />
- </button>
- </Show>
-
- <Show when={props.navigationVisible !== false}>
- <button
- class={cn(navButtonClass, 'right-4')}
- style={{ 'z-index': stackingContext.zModal + 1 }}
- onClick={props.onNext}
- disabled={!props.onNext}
- aria-label="Next media"
- >
- <ChevronRightIcon class="h-5 w-5 text-ink" />
- </button>
- </Show>
+ <Show when={props.navigationVisible !== false}>
+ <button
+ class={cn(navButtonClass, 'left-4')}
+ style={{ 'z-index': stackingContext.zModal + 1 }}
+ onClick={props.onPrevious}
+ disabled={!props.onPrevious}
+ aria-label="Previous media"
+ >
+ <ChevronLeftIcon class="h-5 w-5 text-ink" />
+ </button>
+
+ <button
+ class={cn(navButtonClass, 'right-4')}
+ style={{ 'z-index': stackingContext.zModal + 1 }}
+ onClick={props.onNext}
+ disabled={!props.onNext}
+ aria-label="Next media"
+ >
+ <ChevronRightIcon class="h-5 w-5 text-ink" />
+ </button>
+ </Show>As per coding guidelines: "Reduce unnecessary complexity and nesting in code logic".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Show when={props.navigationVisible !== false}> | |
| <button | |
| class={cn(navButtonClass, 'left-4')} | |
| style={{ 'z-index': stackingContext.zModal + 1 }} | |
| onClick={props.onPrevious} | |
| disabled={!props.onPrevious} | |
| aria-label="Previous media" | |
| > | |
| <ChevronLeftIcon class="h-5 w-5 text-ink" /> | |
| </button> | |
| </Show> | |
| <button | |
| class={cn(navButtonClass, 'right-4')} | |
| style={{ 'z-index': stackingContext.zModal + 1 }} | |
| onClick={props.onNext} | |
| disabled={!props.onNext} | |
| aria-label="Next media" | |
| > | |
| <ChevronRightIcon class="h-5 w-5 text-ink" /> | |
| </button> | |
| <Show when={props.navigationVisible !== false}> | |
| <button | |
| class={cn(navButtonClass, 'right-4')} | |
| style={{ 'z-index': stackingContext.zModal + 1 }} | |
| onClick={props.onNext} | |
| disabled={!props.onNext} | |
| aria-label="Next media" | |
| > | |
| <ChevronRightIcon class="h-5 w-5 text-ink" /> | |
| </button> | |
| </Show> | |
| <Show when={props.navigationVisible !== false}> | |
| <button | |
| class={cn(navButtonClass, 'left-4')} | |
| style={{ 'z-index': stackingContext.zModal + 1 }} | |
| onClick={props.onPrevious} | |
| disabled={!props.onPrevious} | |
| aria-label="Previous media" | |
| > | |
| <ChevronLeftIcon class="h-5 w-5 text-ink" /> | |
| </button> | |
| <button | |
| class={cn(navButtonClass, 'right-4')} | |
| style={{ 'z-index': stackingContext.zModal + 1 }} | |
| onClick={props.onNext} | |
| disabled={!props.onNext} | |
| aria-label="Next media" | |
| > | |
| <ChevronRightIcon class="h-5 w-5 text-ink" /> | |
| </button> | |
| </Show> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Media/MediaViewerDialog.tsx` around lines 62 - 84,
The two identical conditional wrappers using Show (Show
when={props.navigationVisible !== false}) should be merged into a single Show
that wraps both navigation buttons; locate the Show blocks around the left and
right nav buttons (the buttons using navButtonClass, ChevronLeftIcon and
ChevronRightIcon, stackingContext.zModal, onPrevious/onNext and disabled checks)
and move both buttons inside one Show with the same condition so behavior and
props (aria-label, onClick, disabled, style) remain unchanged.
dev-rb
deleted the
rahul/fix-channel-media-viewer-navigation-arrows-showing-for-single-image
branch
1 participant
No description provided.