feat(style): Google Drivemaxx share menu#2182
Merged
Conversation
Contributor
aidanhb
commented
Mar 25, 2026
aidanhb
commented
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:
WalkthroughUpdated EntityIcon to use WideGlobe for the Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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/core/component/ForwardToChannel.tsx`:
- Around line 281-312: The recipient column can grow past the dialog because the
RecipientSelector container lacks a min-width constraint; update the wrapper div
that contains RecipientSelector (the element using classes "flex-1 px-1 py-2
min-h-11") to include min-w-0 so the selectedOptions/chips (from
RecipientSelector) will truncate instead of pushing the dialog wider; leave
ShareOptions and the surrounding Show block unchanged.
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 758-764: The MiniToggleSwitch is missing an accessible name, so
update this call site to pass a descriptive label (e.g., label="Enable public
link" or similar) to MiniToggleSwitch where checked is computed from
publicAccessLevel() and onChange calls setPublicPermissions, or alternatively
modify MiniToggleSwitch to accept and forward an aria-label prop to the
underlying KSwitch.Label and then set aria-label here; ensure the
label/aria-label is specific and matches the intent of the control (toggles
public link/view permissions).
- Around line 563-567: The Dialog.Content in ShareButton.tsx removed the
DialogWrapper's max-height and overflow-hidden guard, allowing the stacked share
form/recipients list/public-link card to overflow the viewport; restore a
constrained scrolling shell by either wrapping the stacked card area inside a
container with the previous behaviors (e.g., a fixed max-height like
max-h-[calc(100vh-...)] plus overflow-auto and overflow-hidden on the dialog
root) or reusing the existing DialogWrapper component around Dialog.Content so
the lower controls remain reachable; update the container around the card stack
(the block rendering recipients list and public-link card) to apply the
max-height/overflow constraints referenced in the original DialogWrapper to fix
the overflow on short screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f71275c9-e6ad-415d-aea7-beab5d02a79c
⛔ Files ignored due to path filters (5)
js/app/packages/macro-icons/wide/comment.svgis excluded by!**/*.svgjs/app/packages/macro-icons/wide/edit.svgis excluded by!**/*.svgjs/app/packages/macro-icons/wide/eye.svgis excluded by!**/*.svgjs/app/packages/macro-icons/wide/globe.svgis excluded by!**/*.svgjs/app/packages/macro-icons/wide/user-circle.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
js/app/packages/core/component/EntityIcon.tsxjs/app/packages/core/component/FormControls/MiniToggleSwitch.tsxjs/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/TopBar/ShareButton.tsxjs/app/packages/ui/components/Button.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/component/ForwardToChannel.tsx`:
- Around line 386-392: The Cancel Button in ForwardToChannel.tsx currently calls
props.onSubmit, causing Cancel to behave like a successful submit; change it to
call a dedicated cancel/close handler instead: add an onCancel (or onClose) prop
to ForwardToChannel's props interface and use onClick={() => props.onCancel?.()}
on the Cancel Button (or, if a suitable close handler already exists in the
parent, call that instead), then update the parent (ShareButton/ShareModal) to
pass the dialog-close callback (e.g., setIsSharePermOpen(false)) as onCancel so
Cancel only closes the dialog and does not invoke the submit success flow.
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 717-767: The badge and dropdown currently duplicate the same state
control: publicAccessLevel() / setPublicPermissions, causing redundant controls;
make the badge non-interactive and keep MiniToggleSwitch as the single control.
Remove the DropdownMenu.Trigger wrapper and any onSelect handler on
DropdownMenu.Item (or remove the entire DropdownMenu/DropdownMenu.Item block) so
the badge simply renders informational text based on publicAccessLevel(), and
leave MiniToggleSwitch (checked={publicAccessLevel() != null} onChange={(on) =>
setPublicPermissions(on ? 'view' : null)}) as the only interactive control;
ensure you keep the visual styles but drop click handlers and classList changes
that imply interactivity on the badge.
- Around line 656-667: The Switch block in ShareButton.tsx uses a Match with
when={true} as a catch-all which is an anti-pattern; update the Switch to use
its fallback prop instead: remove the final <Match when={true}> that renders
<IconUsers .../> and move that JSX into the Switch's fallback so the Switch
handles the default case; keep the existing Match that checks
channelNameMap().get(recipient.channel_id) and renders <UserCircle .../>
unchanged and ensure props/classes are preserved when moving the IconUsers
element into fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3b4e98f-1077-403f-9b17-0fad9dede234
📒 Files selected for processing (2)
js/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/TopBar/ShareButton.tsx
Contributor
|
awkward state with a lot of recipients 
|
Contributor
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 (2)
js/app/packages/core/component/ForwardToChannel.tsx (1)
165-267:⚠️ Potential issue | 🔴 CriticalAwait the share mutations before closing the dialog.
The
Promise.all(...)started on Line 201 is dropped on the floor, and Line 266 still callsprops.onSubmit?.()immediately. That lets the multi-recipient path show success before any request finishes, and a later failure/rejection still leaves the modal closed as if sharing succeeded. Please makehandleSubmitasync, await the queued send/permission promises (preferPromise.allSettled(...)), and keep the new Share button disabled while submission is in flight so duplicate clicks cannot enqueue duplicate shares.Also applies to: 394-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/ForwardToChannel.tsx` around lines 165 - 267, Convert handleSubmit to an async function and await all send/permission promises instead of dropping them: collect promises returned from submitChannelPermissions, sendToChannel, and sendToUsers into an array (use Promise.allSettled(...) to wait for all outcomes), then derive overall success/failure to show the correct toast and only call props.onSubmit?.() after awaiting those results; introduce a local in-flight state (e.g., isSubmitting) toggled at start/end of handleSubmit to disable the Share button while awaiting to prevent duplicate clicks; apply the same change to the other similar multi-recipient send block later in the file (the other submit handler with identical sendToChannel/sendToUsers usage).js/app/packages/core/component/TopBar/ShareButton.tsx (1)
958-964:⚠️ Potential issue | 🟠 MajorForward the label to make the permission trigger accessible and use the polymorphic API to avoid nested buttons.
The
labelprop is accepted but unused. These permission dropdowns are announced only as "View", "Edit", etc., making them indistinguishable in assistive-tech navigation. Forward the label (e.g., "Permission" from ForwardToChannel) as an accessible name to the trigger. Additionally,DropdownMenu.Triggersupports theaspolymorphic prop to render as a Button directly instead of wrapping one—this avoids nesting button elements in the DOM. Update the composition to use<DropdownMenu.Trigger as={Button} aria-label={props.label} ...>pattern rather than nesting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 958 - 964, The ShareOptions component accepts a label prop but doesn't forward it to the dropdown trigger, causing the permission trigger to be announced only as "View"/"Edit"; also it nests a Button inside DropdownMenu.Trigger instead of using the polymorphic API. Update ShareOptions to pass props.label as an accessible name (aria-label) to the trigger and use DropdownMenu.Trigger's polymorphic prop to render as the Button component (e.g., <DropdownMenu.Trigger as={Button} aria-label={props.label} ...>) so you avoid nested buttons and ensure the trigger is properly announced by assistive tech; change any existing nested <Button> inside DropdownMenu.Trigger to rely on the polymorphic render instead while preserving props.disabled and other attributes.
🤖 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/core/component/TopBar/ShareButton.tsx`:
- Around line 650-655: Replace the clickable div used for recipient navigation
with a semantic interactive element (preferably a <button type="button"> or an
<a> link) so keyboard and assistive tech can focus/activate it; locate the
element that currently has class "flex items-center gap-2 overflow-hidden
cursor-pointer" and the onClick={() => navigateToChannel(recipient.channel_id)}
handler and move that handler to the new button/link, preserve the existing
classes (convert class to className if using JSX), add a descriptive aria-label
(e.g., aria-label={`Open channel ${recipient.name || recipient.channel_id}`}),
and remove any manual tabIndex/onKeyDown hacks since a native button/link will
provide keyboard activation and correct semantics.
---
Outside diff comments:
In `@js/app/packages/core/component/ForwardToChannel.tsx`:
- Around line 165-267: Convert handleSubmit to an async function and await all
send/permission promises instead of dropping them: collect promises returned
from submitChannelPermissions, sendToChannel, and sendToUsers into an array (use
Promise.allSettled(...) to wait for all outcomes), then derive overall
success/failure to show the correct toast and only call props.onSubmit?.() after
awaiting those results; introduce a local in-flight state (e.g., isSubmitting)
toggled at start/end of handleSubmit to disable the Share button while awaiting
to prevent duplicate clicks; apply the same change to the other similar
multi-recipient send block later in the file (the other submit handler with
identical sendToChannel/sendToUsers usage).
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 958-964: The ShareOptions component accepts a label prop but
doesn't forward it to the dropdown trigger, causing the permission trigger to be
announced only as "View"/"Edit"; also it nests a Button inside
DropdownMenu.Trigger instead of using the polymorphic API. Update ShareOptions
to pass props.label as an accessible name (aria-label) to the trigger and use
DropdownMenu.Trigger's polymorphic prop to render as the Button component (e.g.,
<DropdownMenu.Trigger as={Button} aria-label={props.label} ...>) so you avoid
nested buttons and ensure the trigger is properly announced by assistive tech;
change any existing nested <Button> inside DropdownMenu.Trigger to rely on the
polymorphic render instead while preserving props.disabled and other attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c338e987-7af7-46d0-aa0e-79cdda1a810b
📒 Files selected for processing (2)
js/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/TopBar/ShareButton.tsx
Comment on lines
+650
to
+655
| <div class="flex justify-between"> | ||
| <div | ||
| class="flex items-center gap-2 overflow-hidden cursor-pointer" | ||
| onClick={() => | ||
| navigateToChannel(recipient.channel_id) | ||
| } |
Contributor
There was a problem hiding this comment.
Use a real button/link for recipient navigation.
Line 650 makes the recipient row mouse-only. Keyboard users cannot focus or activate it, and assistive tech does not get navigation semantics. Render this as a button or link instead of a clickable div.
♿ Suggested change
- <div
- class="flex items-center gap-2 overflow-hidden cursor-pointer"
- onClick={() => navigateToChannel(recipient.channel_id)}
- >
+ <button
+ type="button"
+ class="min-w-0 flex items-center gap-2 overflow-hidden text-left"
+ onClick={() => navigateToChannel(recipient.channel_id)}
+ >
...
- </div>
+ </button>📝 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.
Suggested change
| <div class="flex justify-between"> | |
| <div | |
| class="flex items-center gap-2 overflow-hidden cursor-pointer" | |
| onClick={() => | |
| navigateToChannel(recipient.channel_id) | |
| } | |
| <div class="flex justify-between"> | |
| <button | |
| type="button" | |
| class="min-w-0 flex items-center gap-2 overflow-hidden text-left" | |
| onClick={() => | |
| navigateToChannel(recipient.channel_id) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 650 -
655, Replace the clickable div used for recipient navigation with a semantic
interactive element (preferably a <button type="button"> or an <a> link) so
keyboard and assistive tech can focus/activate it; locate the element that
currently has class "flex items-center gap-2 overflow-hidden cursor-pointer" and
the onClick={() => navigateToChannel(recipient.channel_id)} handler and move
that handler to the new button/link, preserve the existing classes (convert
class to className if using JSX), add a descriptive aria-label (e.g.,
aria-label={`Open channel ${recipient.name || recipient.channel_id}`}), and
remove any manual tabIndex/onKeyDown hacks since a native button/link will
provide keyboard activation and correct semantics.
Contributor
|
awkward state with really long group chat name 
|
Contributor
|
awkward tool tip |
Contributor
|
awkward state with short window 
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/app/packages/core/component/CustomScrollbar.tsx (1)
105-111:⚠️ Potential issue | 🟡 MinorRecompute metrics when the scroll content changes.
ResizeObserveron the container only fires when its own box changes. In the new chip scroller fromjs/app/packages/core/component/RecipientSelector.tsx, adding/removing chips changesscrollWidthwithout resizing the element, so the thumb stays hidden/stale until the user manually scrolls.♻️ Suggested fix
container.addEventListener('scroll', handleScroll, { passive: true }); const resizeObserver = new ResizeObserver(updateScrollMetrics); + const mutationObserver = new MutationObserver(() => { + updateScrollMetrics(); + }); resizeObserver.observe(container); + mutationObserver.observe(container, { + childList: true, + subtree: true, + characterData: true, + }); onCleanup(() => { container.removeEventListener('scroll', handleScroll); resizeObserver.disconnect(); + mutationObserver.disconnect(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/CustomScrollbar.tsx` around lines 105 - 111, The scroll thumb becomes stale when children change because ResizeObserver on the container doesn't fire for internal content width changes; add a MutationObserver (e.g., mutationObserver) that watches the container for childList and subtree changes and calls updateScrollMetrics (and optionally attributes) so metrics are recomputed when chips are added/removed; create the observer alongside the existing resizeObserver and ensure you start it on container and call mutationObserver.disconnect() in the same onCleanup block (also keep the existing container.removeEventListener('scroll', handleScroll) and resizeObserver.disconnect()).js/app/packages/core/component/TopBar/ShareButton.tsx (1)
1098-1107:⚠️ Potential issue | 🟠 MajorRefactor to use polymorphic
asprop to avoid nested buttons.
DropdownMenu.Triggerrenders a<button>by default, andButtonalso renders a<button>. This creates invalid nested button markup (<button><button>...</button></button>) which breaks focus and keyboard handling.Use the trigger's polymorphic API instead:
<DropdownMenu.Trigger as={Button} ...>(see established patterns inMediaButtons.tsx,ComposeEmailInput.tsx, andsort-dropdown.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 1098 - 1107, DropdownMenu.Trigger and Button are both rendering <button>, causing nested buttons; change to use the Trigger's polymorphic API by rendering the Trigger as the Button component (use DropdownMenu.Trigger as={Button}) and move props currently on the inner Button (disabled, class, variant, children like currentValueText() and ChevronDownIcon) onto DropdownMenu.Trigger; keep the open handling (open={isOpen()} onOpenChange={setIsOpen}) on the Trigger and remove the nested Button element so you no longer have a Button inside DropdownMenu.Trigger.
♻️ Duplicate comments (1)
js/app/packages/core/component/TopBar/ShareButton.tsx (1)
724-728:⚠️ Potential issue | 🟡 MinorUse a semantic button for channel navigation.
This is still a mouse-only
div, so keyboard users can't focus or open the channel row from the access list.♿ Suggested fix
- <div - class="flex items-center gap-2 overflow-hidden cursor-pointer" - onClick={() => - navigateToChannel(recipient.channel_id) - } - > + <button + type="button" + class="min-w-0 flex items-center gap-2 overflow-hidden text-left" + aria-label={`Open channel ${channelNameMap().get(recipient.channel_id)?.name || recipient.channel_id}`} + onClick={() => + navigateToChannel(recipient.channel_id) + } + > ... - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 724 - 728, Replace the clickable non-semantic div used for channel navigation with a real button element so keyboard users can focus and activate it; locate the element in ShareButton.tsx where the div has class "flex items-center gap-2 overflow-hidden cursor-pointer" and onClick={() => navigateToChannel(recipient.channel_id)} and change it to <button type="button"> keeping the same classes, preserving the onClick handler (navigateToChannel(recipient.channel_id)), remove any tabIndex hacks, and ensure the button has an accessible label (either visible text or an aria-label) so screen reader users can understand the action.
🤖 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/core/component/ForwardToChannel.tsx`:
- Around line 333-340: The onTab handler in ForwardToChannel.tsx is currently
returning true (onTab={() => true}), which signals MarkdownArea that the Tab was
handled and traps focus; change this to not consume Tab (either remove the onTab
prop or return false/undefined, e.g., onTab={() => false}) so the default focus
behavior (and keyboard access to Cancel/Share) is preserved; update the prop on
the MarkdownArea usage in ForwardToChannel.tsx accordingly.
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 776-790: The ShareOptions dropdown is shown to non-owners and
allows Remove Access while setChannelPermissions silently returns; change the
rendering in the component that renders <ShareOptions> so that when
props.userPermissions !== Permissions.OWNER you do not render the interactive
<ShareOptions> control but instead render a read-only access label (e.g.,
"Viewer" or the current recipient.access_level) and disable any actions; keep
<ShareOptions>, setChannelPermissions(recipient.channel_id, ...), and
removeChannelAccess(recipient.channel_id) behavior unchanged for owner users
(props.userPermissions === Permissions.OWNER) so only owners see the interactive
dropdown.
---
Outside diff comments:
In `@js/app/packages/core/component/CustomScrollbar.tsx`:
- Around line 105-111: The scroll thumb becomes stale when children change
because ResizeObserver on the container doesn't fire for internal content width
changes; add a MutationObserver (e.g., mutationObserver) that watches the
container for childList and subtree changes and calls updateScrollMetrics (and
optionally attributes) so metrics are recomputed when chips are added/removed;
create the observer alongside the existing resizeObserver and ensure you start
it on container and call mutationObserver.disconnect() in the same onCleanup
block (also keep the existing container.removeEventListener('scroll',
handleScroll) and resizeObserver.disconnect()).
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 1098-1107: DropdownMenu.Trigger and Button are both rendering
<button>, causing nested buttons; change to use the Trigger's polymorphic API by
rendering the Trigger as the Button component (use DropdownMenu.Trigger
as={Button}) and move props currently on the inner Button (disabled, class,
variant, children like currentValueText() and ChevronDownIcon) onto
DropdownMenu.Trigger; keep the open handling (open={isOpen()}
onOpenChange={setIsOpen}) on the Trigger and remove the nested Button element so
you no longer have a Button inside DropdownMenu.Trigger.
---
Duplicate comments:
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 724-728: Replace the clickable non-semantic div used for channel
navigation with a real button element so keyboard users can focus and activate
it; locate the element in ShareButton.tsx where the div has class "flex
items-center gap-2 overflow-hidden cursor-pointer" and onClick={() =>
navigateToChannel(recipient.channel_id)} and change it to <button type="button">
keeping the same classes, preserving the onClick handler
(navigateToChannel(recipient.channel_id)), remove any tabIndex hacks, and ensure
the button has an accessible label (either visible text or an aria-label) so
screen reader users can understand the action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c031a709-4b52-41c8-bda1-71be32dc26bf
📒 Files selected for processing (4)
js/app/packages/core/component/CustomScrollbar.tsxjs/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/RecipientSelector.tsxjs/app/packages/core/component/TopBar/ShareButton.tsx
Comment on lines
+776
to
+790
| <ShareOptions | ||
| permissions={recipient.access_level} | ||
| setPermissions={(accessLevel) => { | ||
| if (accessLevel === null) { | ||
| removeChannelAccess(recipient.channel_id); | ||
| } else if ( | ||
| accessLevel !== recipient.access_level | ||
| ) { | ||
| setChannelPermissions( | ||
| recipient.channel_id, | ||
| accessLevel | ||
| ); | ||
| } | ||
| }} | ||
| /> |
Contributor
There was a problem hiding this comment.
Make the recipient access control owner-only.
This dropdown renders for viewers too. In that state setChannelPermissions silently returns, while choosing “Remove Access” still calls removeChannelAccess, so non-owners get a broken—and potentially destructive—control. Render read-only access text unless props.userPermissions === Permissions.OWNER.
🐛 Suggested fix
- <div class="flex items-center">
- <ShareOptions
- permissions={recipient.access_level}
- setPermissions={(accessLevel) => {
- if (accessLevel === null) {
- removeChannelAccess(recipient.channel_id);
- } else if (
- accessLevel !== recipient.access_level
- ) {
- setChannelPermissions(
- recipient.channel_id,
- accessLevel
- );
- }
- }}
- />
- </div>
+ <div class="flex items-center">
+ <Show
+ when={props.userPermissions === Permissions.OWNER}
+ fallback={
+ <span class="text-xs font-medium text-ink-muted">
+ {accessLevelText(recipient.access_level)}
+ </span>
+ }
+ >
+ <ShareOptions
+ permissions={recipient.access_level}
+ setPermissions={(accessLevel) => {
+ if (accessLevel === null) {
+ removeChannelAccess(recipient.channel_id);
+ } else if (
+ accessLevel !== recipient.access_level
+ ) {
+ setChannelPermissions(
+ recipient.channel_id,
+ accessLevel
+ );
+ }
+ }}
+ />
+ </Show>
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/TopBar/ShareButton.tsx` around lines 776 -
790, The ShareOptions dropdown is shown to non-owners and allows Remove Access
while setChannelPermissions silently returns; change the rendering in the
component that renders <ShareOptions> so that when props.userPermissions !==
Permissions.OWNER you do not render the interactive <ShareOptions> control but
instead render a read-only access label (e.g., "Viewer" or the current
recipient.access_level) and disable any actions; keep <ShareOptions>,
setChannelPermissions(recipient.channel_id, ...), and
removeChannelAccess(recipient.channel_id) behavior unchanged for owner users
(props.userPermissions === Permissions.OWNER) so only owners see the interactive
dropdown.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/component/ForwardToChannel.tsx`:
- Around line 299-317: The initializer effect that resets submitAccessLevel
whenever selectedOptions() changes is overwriting user choices; remove or stop
relying on that createEffect and instead preserve manual changes by adding a
manualOverride flag and a wrapped setter: when ShareOptions calls
setSubmitAccessLevel, set manualOverride=true; change the logic that previously
reset submitAccessLevel on selectedOptions updates to only apply when
manualOverride is false (or when the destination/channel truly changes), and
ensure ShareOptions is disabled (or reflects channel defaults) until a final
destination is chosen if you prefer that UX; reference the ShareOptions
component, the setSubmitAccessLevel setter, the submitAccessLevel signal, and
the selectedOptions signal when making these changes.
- Around line 399-402: The Share button in ForwardToChannel.tsx currently forces
height via the class "h-[22px]" (and also uses "py-2"), which conflicts with the
Button component's size handling and causes clipping; remove "h-[22px]" from the
Button's class and let the Button's size prop control height (add or ensure
size="sm" on this Button), and also remove or reduce the vertical padding
("py-2") so the size prop and internal padding determine the final box (i.e.,
update the Button element that uses selectedOptions() to drop "h-[22px]" and
adjust "py-2" accordingly).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81f64840-e0f9-452d-ad49-9cd6cf4767ff
📒 Files selected for processing (1)
js/app/packages/core/component/ForwardToChannel.tsx
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.