feat(ios): edge to edge#4146
Open
peterchinman wants to merge 14 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a full-frame mobile layout system. A new float-region mechanism ( 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts (1)
8-8: 💤 Low valueConsider consolidating
NEAR_BOTTOM_THRESHOLDconstantThis defines
NEAR_BOTTOM_THRESHOLD = 100, whilejs/app/packages/channel/Channel/constants.tsexportsNEAR_BOTTOM_THRESHOLD = 50. If the difference is intentional (AI chat needs a larger threshold than channel), consider documenting why. Otherwise, consider importing from a shared location for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts` at line 8, The NEAR_BOTTOM_THRESHOLD constant is defined with a value of 100 in the create-mobile-keyboard-scroll-pin.ts file, but a different value of 50 is exported from js/app/packages/channel/Channel/constants.ts. Determine if the 100 value is intentional for this AI chat component or if it should match the shared constant. If the different value is needed, add a comment above the NEAR_BOTTOM_THRESHOLD definition explaining why the AI component requires a larger threshold than the channel component. If the values should be the same, import NEAR_BOTTOM_THRESHOLD from the shared constants file instead of defining it locally.js/app/packages/app/index.css (1)
226-237: ⚡ Quick winRemove commented-out code or uncomment with proper whitespace.
The
[data-press-pulse]block contains commented-outbackground-colortransitions (lines 229, 233, 236) that have stylelint whitespace violations. Either uncomment these lines if the background-color transition is needed, or remove them entirely.🧹 Proposed fix to clean up commented code
If the background-color transitions are not needed:
[data-press-pulse] { transition: - transform 100ms cubic-bezier(0.34, 1.56, 0.64, 1); - /*background-color 150ms ease-out;*/ + transform 100ms cubic-bezier(0.34, 1.56, 0.64, 1); } [data-press-pulse][data-pressed] { transform: scale(1.08); - /*background-color: var(--color-active);*/ transition: - transform 100ms ease-out; - /*background-color 100ms ease-out;*/ + transform 100ms ease-out; }If the background-color transitions should be enabled:
[data-press-pulse] { transition: transform 100ms cubic-bezier(0.34, 1.56, 0.64, 1), - /*background-color 150ms ease-out;*/ + background-color 150ms ease-out; } [data-press-pulse][data-pressed] { transform: scale(1.08); - /*background-color: var(--color-active);*/ + background-color: var(--color-active); transition: transform 100ms ease-out, - /*background-color 100ms ease-out;*/ + background-color 100ms ease-out; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/index.css` around lines 226 - 237, The [data-press-pulse] and [data-press-pulse][data-pressed] CSS rules contain commented-out background-color transitions that have stylelint whitespace violations. Either remove all the commented-out lines entirely (the background-color property within the transition shorthand and the standalone background-color properties), or if the background-color transitions are needed functionality, uncomment them and ensure proper CSS syntax with correct whitespace in the transition property values.Source: Linters/SAST tools
js/app/packages/channel/Channel/ThreadList.tsx (1)
62-67: ⚡ Quick winRename
FullFrameTheadListScrollInsetsbefore this API spreads further.Line 62 introduces an exported type with a typo (
Thead), and Line 84 reuses it in props. Since this is part of the exported surface, it’s worth correcting now to avoid leaking the misspelling across callers.Suggested diff
-export type FullFrameTheadListScrollInsets = { +export type FullFrameThreadListScrollInsets = { /** Space reserved before the first message (e.g. status bar + floating header). */ start: number; /** Space reserved after the last message (e.g. floating input + dock). */ end: number; }; @@ - fullFrameScrollInsets?: Accessor<FullFrameTheadListScrollInsets>; + fullFrameScrollInsets?: Accessor<FullFrameThreadListScrollInsets>; @@ -const NO_SCROLL_INSETS: FullFrameTheadListScrollInsets = { start: 0, end: 0 }; +const NO_SCROLL_INSETS: FullFrameThreadListScrollInsets = { start: 0, end: 0 };Also applies to: 84-84, 113-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/channel/Channel/ThreadList.tsx` around lines 62 - 67, The exported type FullFrameTheadListScrollInsets contains a typo in its name where "Thead" should be "Thread". Rename this type to FullFrameThreadListScrollInsets in its definition on line 62, and update all references to this type throughout the file including the props usage mentioned on lines 84 and 113 to use the corrected name.js/app/packages/channel/Call/ChannelCallButton.tsx (1)
37-37: ⚡ Quick winReplace the nested ternary in
variantselection with explicit branching.Line 37 uses a nested ternary, which conflicts with the project TS/TSX style rule and is harder to scan.
Suggested diff
+ const variant = () => { + if (isMobile()) return 'ghost'; + return isCallInProgress() ? 'success' : 'base'; + }; + return ( <Show when={!call.isInThisChannel()}> <Button onClick={handleClick} tooltip={tooltip()} - variant={isMobile() ? 'ghost' : isCallInProgress() ? 'success' : 'base'} + variant={variant()} size="sm" depth={2}As per coding guidelines,
Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/channel/Call/ChannelCallButton.tsx` at line 37, The variant property assignment in ChannelCallButton.tsx uses a nested ternary operator that violates the project's coding guidelines. Replace the nested ternary expression (checking isMobile() first, then isCallInProgress(), with fallback to 'base') with either a switch statement or if/else chain for better readability. Maintain the same conditional logic where mobile devices get 'ghost', active calls get 'success', and the default variant is 'base'.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app/packages/app/component/mobile/MobileDock.css`:
- Around line 1-14: The keyframes names morePopoverShow and morePopoverHide
violate the stylelint keyframes-name-pattern rule which requires kebab-case
naming. Rename the morePopoverShow keyframe to more-popover-show and the
morePopoverHide keyframe to more-popover-hide throughout the CSS file, and
update any corresponding animation property references that use these keyframe
names to match the new kebab-case naming convention.
In `@js/app/packages/app/component/mobile/MobileDock.tsx`:
- Around line 42-57: The icon-only dock buttons (Home, Inbox, More, Create) in
MobileDockButtonProps and their corresponding implementations lack accessible
names, making them inaccessible to screen readers. Ensure each button element
rendered for these icon-only dock buttons includes an aria-label attribute that
describes the button's purpose. You can use the existing label prop or create
appropriate descriptive labels for each button based on their function, and
apply the aria-label to the actual button HTML element in the
render/implementation sections around lines 63-99, 181-189, and 322-356.
In `@js/app/packages/block-email/component/compose/ComposeToolbar.tsx`:
- Around line 180-193: The attachment button in ComposeToolbar is rendered
unconditionally in the mobile toolbar section (the div with
ref={props.attachButtonRef}), but it should respect the ctx.hideAttachments flag
like the desktop path does. Wrap the attachment button div with a conditional
check that only renders it when !ctx.hideAttachments() is true, ensuring that
compose modes that intentionally disable attachments are properly respected on
mobile. Apply the same fix to the additional attachment control mentioned in
lines 194-201.
In
`@js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts`:
- Around line 56-82: The ResizeObserver created by makeResizeObserver in the
onMount function is missing explicit cleanup on component unmount. Add an
onCleanup callback after the onMount function that properly disconnects the
observer by calling the appropriate cleanup method on the observer object
returned by makeResizeObserver, ensuring the wrapper element is no longer
observed and resources are freed when the component is destroyed.
---
Nitpick comments:
In `@js/app/packages/app/index.css`:
- Around line 226-237: The [data-press-pulse] and
[data-press-pulse][data-pressed] CSS rules contain commented-out
background-color transitions that have stylelint whitespace violations. Either
remove all the commented-out lines entirely (the background-color property
within the transition shorthand and the standalone background-color properties),
or if the background-color transitions are needed functionality, uncomment them
and ensure proper CSS syntax with correct whitespace in the transition property
values.
In `@js/app/packages/channel/Call/ChannelCallButton.tsx`:
- Line 37: The variant property assignment in ChannelCallButton.tsx uses a
nested ternary operator that violates the project's coding guidelines. Replace
the nested ternary expression (checking isMobile() first, then
isCallInProgress(), with fallback to 'base') with either a switch statement or
if/else chain for better readability. Maintain the same conditional logic where
mobile devices get 'ghost', active calls get 'success', and the default variant
is 'base'.
In `@js/app/packages/channel/Channel/ThreadList.tsx`:
- Around line 62-67: The exported type FullFrameTheadListScrollInsets contains a
typo in its name where "Thead" should be "Thread". Rename this type to
FullFrameThreadListScrollInsets in its definition on line 62, and update all
references to this type throughout the file including the props usage mentioned
on lines 84 and 113 to use the corrected name.
In
`@js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts`:
- Line 8: The NEAR_BOTTOM_THRESHOLD constant is defined with a value of 100 in
the create-mobile-keyboard-scroll-pin.ts file, but a different value of 50 is
exported from js/app/packages/channel/Channel/constants.ts. Determine if the 100
value is intentional for this AI chat component or if it should match the shared
constant. If the different value is needed, add a comment above the
NEAR_BOTTOM_THRESHOLD definition explaining why the AI component requires a
larger threshold than the channel component. If the values should be the same,
import NEAR_BOTTOM_THRESHOLD from the shared constants file instead of defining
it locally.
🪄 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: ef447cd5-77da-4773-9b5c-a2ca49106435
📒 Files selected for processing (55)
js/app/packages/app/component/Layout.tsxjs/app/packages/app/component/ResponsiveBlockToolbar.tsxjs/app/packages/app/component/home/home.tsxjs/app/packages/app/component/mobile/MobileDock.cssjs/app/packages/app/component/mobile/MobileDock.tsxjs/app/packages/app/component/mobile/MobileEdgeFade.tsxjs/app/packages/app/component/mobile/MobileSearch.tsxjs/app/packages/app/component/mobile/PillTabs.tsxjs/app/packages/app/component/mobile/float-regions/FloatRegion.tsxjs/app/packages/app/component/mobile/float-regions/FloatRegionHost.tsxjs/app/packages/app/component/mobile/float-regions/float-region-state.tsjs/app/packages/app/component/mobile/pressPulse.tsjs/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-mobile-create-button.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-mobile-search.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-mobile-settings-button.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-tabs.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Settings.tsxjs/app/packages/app/component/side-panel/SidePanel.tsxjs/app/packages/app/component/split-layout/components/HeaderIsland.tsxjs/app/packages/app/component/split-layout/components/HeaderTitleMenu.tsxjs/app/packages/app/component/split-layout/components/SplitHeader.tsxjs/app/packages/app/component/split-layout/components/SplitLabel.tsxjs/app/packages/app/component/split-layout/components/SplitPanel.tsxjs/app/packages/app/index.cssjs/app/packages/block-automation/component/Automation.tsxjs/app/packages/block-canvas/component/FloatingMenu.tsxjs/app/packages/block-canvas/component/ToolBar.tsxjs/app/packages/block-canvas/component/TopBar.tsxjs/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/block-channel/component/Top.tsxjs/app/packages/block-chat/component/Chat.tsxjs/app/packages/block-code/component/CodeMirror.tsxjs/app/packages/block-code/component/HtmlPreview.tsxjs/app/packages/block-email/component/BottomReplyButtons.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/MessageList.tsxjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-md/component/Block.tsxjs/app/packages/block-md/component/Notebook.tsxjs/app/packages/block-md/component/TopBar.tsxjs/app/packages/block-pdf/component/TopBar.tsxjs/app/packages/block-pdf/component/block.cssjs/app/packages/block-project/component/TopBar.tsxjs/app/packages/channel/Call/ChannelCallButton.tsxjs/app/packages/channel/Channel/ActiveCallMessage.tsxjs/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Channel/ChannelTopBarLiveIndicators.tsxjs/app/packages/channel/Channel/ThreadList.tsxjs/app/packages/channel/Input/ChannelInputContainer.tsxjs/app/packages/core/component/AI/component/message/ChatMessages.tsxjs/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.tsjs/app/packages/core/constant/stackingContext.ts
💤 Files with no reviewable changes (2)
- js/app/packages/app/component/next-soup/soup-view/soup-view-mobile-settings-button.tsx
- js/app/packages/app/component/next-soup/soup-view/soup-view-mobile-search.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/app/component/next-soup/soup-view/soup-view.tsx (1)
374-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire the mobile search hotkey to an actual opener.
The mobile
cmd+fbranch setsmobileSearchOpen, but this component never reads that signal or renders mobile search. That consumes the shortcut once with no UI, then blocks later attempts because the signal staystrue; connect it to the new mobile search flow or let the hotkey fall through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines 374 - 388, The mobileSearchOpen signal is set to true in the keyDownHandler within the registerHotkey call when on mobile, but this component does not actually read or render based on this signal, so the hotkey is consumed with no visible effect and subsequent attempts are blocked. Either wire the mobileSearchOpen signal to the actual mobile search UI rendering in the component's return statement to make it functional, or change the keyDownHandler to return false in the mobile branch to allow the hotkey to fall through to other handlers instead of consuming it without effect.
🧹 Nitpick comments (1)
js/app/packages/app/component/next-soup/soup-view/soup-view.tsx (1)
369-372: ⚡ Quick winUse a plain accessor for this cheap derivation.
docsUrlonly indexes a constant map fromactiveListView(), socreateMemois unnecessary here.♻️ Proposed refactor
- const docsUrl = createMemo(() => { + const docsUrl = () => { const view = activeListView(); return view ? LIST_VIEW_DOCS_URL[view] : undefined; - }); + };As per coding guidelines, “Use createMemo in SolidJs only when you need referential stability or the derivation is expensive. Cheap derivations do not need it regardless of subscriber count.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines 369 - 372, The docsUrl variable uses createMemo for a simple constant map lookup based on activeListView(), which is a cheap derivation that doesn't require memoization. Replace the createMemo wrapper with a plain accessor function that performs the same logic. Remove the createMemo call and convert the callback function to a regular arrow function that indexes LIST_VIEW_DOCS_URL based on the activeListView value, returning undefined if no view exists.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsx`:
- Around line 403-414: The onClick handler for the button in the mobile filter
drawer is not matching the button's label behavior. When isSole() is true, the
button displays "All" with aria-label "Show all inboxes", but the onClick
handler still calls picker.selectOnly(option.id), which maintains the
single-inbox filter instead of clearing it. Modify the onClick handler to
conditionally check isSole() and call an appropriate method to reset or clear
the inbox filter when the user wants to see all inboxes, while keeping the
current picker.selectOnly(option.id) call for when isSole() is false.
In `@js/app/packages/block-md/component/Block.tsx`:
- Around line 92-99: The ingestS3Snapshot function violates the query-layer
contract by directly calling storageServiceClient.fetchCachedSnapshot instead of
routing through TanStack Query. Replace the direct
storageServiceClient.fetchCachedSnapshot call with a corresponding query hook
from the queries package. Create or use an existing query hook (likely something
like useFetchCachedSnapshot or similar) that handles the network request,
caching, and invalidation, then use that hook's query result instead of directly
awaiting the service client call. This ensures the call goes through the proper
query layer and respects shared caching/invalidation behavior.
- Around line 148-151: The three unawaited ingest function calls
(ingestLocalSnapshot, ingestS3Snapshot, and ingestRemoteSnapshot) lack error
handling, which means any promise rejections become unhandled and can break
initialization. Add .catch() handlers to each of these three function calls to
gracefully handle any potential errors that may occur during the ingest
operations, ensuring that rejections are caught and logged or handled
appropriately rather than becoming unhandled promise rejections.
---
Outside diff comments:
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 374-388: The mobileSearchOpen signal is set to true in the
keyDownHandler within the registerHotkey call when on mobile, but this component
does not actually read or render based on this signal, so the hotkey is consumed
with no visible effect and subsequent attempts are blocked. Either wire the
mobileSearchOpen signal to the actual mobile search UI rendering in the
component's return statement to make it functional, or change the keyDownHandler
to return false in the mobile branch to allow the hotkey to fall through to
other handlers instead of consuming it without effect.
---
Nitpick comments:
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 369-372: The docsUrl variable uses createMemo for a simple
constant map lookup based on activeListView(), which is a cheap derivation that
doesn't require memoization. Replace the createMemo wrapper with a plain
accessor function that performs the same logic. Remove the createMemo call and
convert the callback function to a regular arrow function that indexes
LIST_VIEW_DOCS_URL based on the activeListView value, returning undefined if no
view exists.
🪄 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: 38ccd0c6-4347-4f5a-88c1-d8a8eadc35d5
📒 Files selected for processing (24)
js/app/packages/app/component/Layout.tsxjs/app/packages/app/component/home/home.tsxjs/app/packages/app/component/mobile/MobileDock.cssjs/app/packages/app/component/mobile/MobileDock.tsxjs/app/packages/app/component/mobile/MobileSearch.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-mobile-create-button.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-tabs.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Settings.tsxjs/app/packages/app/component/split-layout/components/SplitHeader.tsxjs/app/packages/app/component/split-layout/components/SplitPanel.tsxjs/app/packages/app/index.cssjs/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/block-channel/component/Top.tsxjs/app/packages/block-chat/component/Chat.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-md/component/Block.tsxjs/app/packages/block-md/component/Notebook.tsxjs/app/packages/channel/Call/ChannelCallButton.tsxjs/app/packages/channel/Channel/ThreadList.tsxjs/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts
💤 Files with no reviewable changes (1)
- js/app/packages/app/component/home/home.tsx
✅ Files skipped from review due to trivial changes (1)
- js/app/packages/block-md/component/Notebook.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- js/app/packages/app/component/next-soup/soup-view/soup-view-mobile-create-button.tsx
- js/app/packages/block-email/component/compose/ComposeLayout.tsx
- js/app/packages/app/component/mobile/MobileDock.css
- js/app/packages/app/component/split-layout/components/SplitPanel.tsx
- js/app/packages/app/index.css
- js/app/packages/block-channel/component/Top.tsx
- js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts
- js/app/packages/block-email/component/Email.tsx
- js/app/packages/app/component/mobile/MobileSearch.tsx
- js/app/packages/channel/Channel/ThreadList.tsx
- js/app/packages/app/component/split-layout/components/SplitHeader.tsx
- js/app/packages/app/component/settings/Settings.tsx
- js/app/packages/block-channel/component/NewChannelBlockAdapter.tsx
- js/app/packages/app/component/Layout.tsx
- js/app/packages/app/component/mobile/MobileDock.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
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/app/component/next-soup/soup-view/soup-view.tsx (1)
374-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire the mobile search hotkey to an actual opener.
The mobile
cmd+fbranch setsmobileSearchOpen, but this component never reads that signal or renders mobile search. That consumes the shortcut once with no UI, then blocks later attempts because the signal staystrue; connect it to the new mobile search flow or let the hotkey fall through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines 374 - 388, The mobileSearchOpen signal is set to true in the keyDownHandler within the registerHotkey call when on mobile, but this component does not actually read or render based on this signal, so the hotkey is consumed with no visible effect and subsequent attempts are blocked. Either wire the mobileSearchOpen signal to the actual mobile search UI rendering in the component's return statement to make it functional, or change the keyDownHandler to return false in the mobile branch to allow the hotkey to fall through to other handlers instead of consuming it without effect.
🧹 Nitpick comments (1)
js/app/packages/app/component/next-soup/soup-view/soup-view.tsx (1)
369-372: ⚡ Quick winUse a plain accessor for this cheap derivation.
docsUrlonly indexes a constant map fromactiveListView(), socreateMemois unnecessary here.♻️ Proposed refactor
- const docsUrl = createMemo(() => { + const docsUrl = () => { const view = activeListView(); return view ? LIST_VIEW_DOCS_URL[view] : undefined; - }); + };As per coding guidelines, “Use createMemo in SolidJs only when you need referential stability or the derivation is expensive. Cheap derivations do not need it regardless of subscriber count.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines 369 - 372, The docsUrl variable uses createMemo for a simple constant map lookup based on activeListView(), which is a cheap derivation that doesn't require memoization. Replace the createMemo wrapper with a plain accessor function that performs the same logic. Remove the createMemo call and convert the callback function to a regular arrow function that indexes LIST_VIEW_DOCS_URL based on the activeListView value, returning undefined if no view exists.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsx`:
- Around line 403-414: The onClick handler for the button in the mobile filter
drawer is not matching the button's label behavior. When isSole() is true, the
button displays "All" with aria-label "Show all inboxes", but the onClick
handler still calls picker.selectOnly(option.id), which maintains the
single-inbox filter instead of clearing it. Modify the onClick handler to
conditionally check isSole() and call an appropriate method to reset or clear
the inbox filter when the user wants to see all inboxes, while keeping the
current picker.selectOnly(option.id) call for when isSole() is false.
In `@js/app/packages/block-md/component/Block.tsx`:
- Around line 92-99: The ingestS3Snapshot function violates the query-layer
contract by directly calling storageServiceClient.fetchCachedSnapshot instead of
routing through TanStack Query. Replace the direct
storageServiceClient.fetchCachedSnapshot call with a corresponding query hook
from the queries package. Create or use an existing query hook (likely something
like useFetchCachedSnapshot or similar) that handles the network request,
caching, and invalidation, then use that hook's query result instead of directly
awaiting the service client call. This ensures the call goes through the proper
query layer and respects shared caching/invalidation behavior.
- Around line 148-151: The three unawaited ingest function calls
(ingestLocalSnapshot, ingestS3Snapshot, and ingestRemoteSnapshot) lack error
handling, which means any promise rejections become unhandled and can break
initialization. Add .catch() handlers to each of these three function calls to
gracefully handle any potential errors that may occur during the ingest
operations, ensuring that rejections are caught and logged or handled
appropriately rather than becoming unhandled promise rejections.
---
Outside diff comments:
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 374-388: The mobileSearchOpen signal is set to true in the
keyDownHandler within the registerHotkey call when on mobile, but this component
does not actually read or render based on this signal, so the hotkey is consumed
with no visible effect and subsequent attempts are blocked. Either wire the
mobileSearchOpen signal to the actual mobile search UI rendering in the
component's return statement to make it functional, or change the keyDownHandler
to return false in the mobile branch to allow the hotkey to fall through to
other handlers instead of consuming it without effect.
---
Nitpick comments:
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 369-372: The docsUrl variable uses createMemo for a simple
constant map lookup based on activeListView(), which is a cheap derivation that
doesn't require memoization. Replace the createMemo wrapper with a plain
accessor function that performs the same logic. Remove the createMemo call and
convert the callback function to a regular arrow function that indexes
LIST_VIEW_DOCS_URL based on the activeListView value, returning undefined if no
view exists.
🪄 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: 38ccd0c6-4347-4f5a-88c1-d8a8eadc35d5
📒 Files selected for processing (24)
js/app/packages/app/component/Layout.tsxjs/app/packages/app/component/home/home.tsxjs/app/packages/app/component/mobile/MobileDock.cssjs/app/packages/app/component/mobile/MobileDock.tsxjs/app/packages/app/component/mobile/MobileSearch.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-mobile-create-button.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view-tabs.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Settings.tsxjs/app/packages/app/component/split-layout/components/SplitHeader.tsxjs/app/packages/app/component/split-layout/components/SplitPanel.tsxjs/app/packages/app/index.cssjs/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/block-channel/component/Top.tsxjs/app/packages/block-chat/component/Chat.tsxjs/app/packages/block-email/component/Email.tsxjs/app/packages/block-email/component/compose/ComposeLayout.tsxjs/app/packages/block-email/component/compose/ComposeToolbar.tsxjs/app/packages/block-md/component/Block.tsxjs/app/packages/block-md/component/Notebook.tsxjs/app/packages/channel/Call/ChannelCallButton.tsxjs/app/packages/channel/Channel/ThreadList.tsxjs/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts
💤 Files with no reviewable changes (1)
- js/app/packages/app/component/home/home.tsx
✅ Files skipped from review due to trivial changes (1)
- js/app/packages/block-md/component/Notebook.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- js/app/packages/app/component/next-soup/soup-view/soup-view-mobile-create-button.tsx
- js/app/packages/block-email/component/compose/ComposeLayout.tsx
- js/app/packages/app/component/mobile/MobileDock.css
- js/app/packages/app/component/split-layout/components/SplitPanel.tsx
- js/app/packages/app/index.css
- js/app/packages/block-channel/component/Top.tsx
- js/app/packages/core/component/AI/component/message/create-mobile-keyboard-scroll-pin.ts
- js/app/packages/block-email/component/Email.tsx
- js/app/packages/app/component/mobile/MobileSearch.tsx
- js/app/packages/channel/Channel/ThreadList.tsx
- js/app/packages/app/component/split-layout/components/SplitHeader.tsx
- js/app/packages/app/component/settings/Settings.tsx
- js/app/packages/block-channel/component/NewChannelBlockAdapter.tsx
- js/app/packages/app/component/Layout.tsx
- js/app/packages/app/component/mobile/MobileDock.tsx
🛑 Comments failed to post (3)
js/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsx (1)
403-414:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the
Allaction reset the inbox scope.When
isSole()is true the button saysAll/ “Show all inboxes”, but the handler still callspicker.selectOnly(option.id), so it leaves the one-inbox filter unchanged.🐛 Proposed fix
- onClick={() => picker.selectOnly(option.id)} + onClick={() => { + if (isSole()) picker.reset(); + else picker.selectOnly(option.id); + }}📝 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={picker.hasMultiple()}> <button type="button" class="shrink-0 px-3 text-xs text-ink-muted hover:text-ink hover:bg-hover transition-colors" aria-label={ isSole() ? 'Show all inboxes' : `Show only ${option.label}` } onClick={() => { if (isSole()) picker.reset(); else picker.selectOnly(option.id); }} > {isSole() ? 'All' : 'Only'}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsx` around lines 403 - 414, The onClick handler for the button in the mobile filter drawer is not matching the button's label behavior. When isSole() is true, the button displays "All" with aria-label "Show all inboxes", but the onClick handler still calls picker.selectOnly(option.id), which maintains the single-inbox filter instead of clearing it. Modify the onClick handler to conditionally check isSole() and call an appropriate method to reset or clear the inbox filter when the user wants to see all inboxes, while keeping the current picker.selectOnly(option.id) call for when isSole() is false.js/app/packages/block-md/component/Block.tsx (2)
92-99:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute service-client access through the queries package
ingestS3Snapshotdirectly callsstorageServiceClient.fetchCachedSnapshotfrom component code. This breaks the query-layer contract and bypasses shared caching/invalidation behavior.As per coding guidelines, “All network calls to service clients MUST go through TanStack Query in the
queriespackage. Do NOT call service clients directly from components or other packages.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/block-md/component/Block.tsx` around lines 92 - 99, The ingestS3Snapshot function violates the query-layer contract by directly calling storageServiceClient.fetchCachedSnapshot instead of routing through TanStack Query. Replace the direct storageServiceClient.fetchCachedSnapshot call with a corresponding query hook from the queries package. Create or use an existing query hook (likely something like useFetchCachedSnapshot or similar) that handles the network request, caching, and invalidation, then use that hook's query result instead of directly awaiting the service client call. This ensures the call goes through the proper query layer and respects shared caching/invalidation behavior.Source: Coding guidelines
148-151:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch errors from unawaited ingest calls
The three ingest calls are intentionally unawaited, but uncaught. A rejection here becomes an unhandled promise rejection and can break initialization flow without controlled recovery.
Suggested fix
+const fireAndReport = (label: string, promise: Promise<unknown>) => { + void promise.catch((error) => { + console.error(`Failed ${label}`, error); + }); +}; + // unawaited — state machine handles precedence -ingestLocalSnapshot(loroManager, snapshotStore, walStore); -ingestS3Snapshot(loroManager, blockId); -ingestRemoteSnapshot(loroManager, data.doInitialSync); +fireAndReport( + 'local snapshot ingest', + ingestLocalSnapshot(loroManager, snapshotStore, walStore) +); +fireAndReport('s3 snapshot ingest', ingestS3Snapshot(loroManager, blockId)); +fireAndReport( + 'remote snapshot ingest', + ingestRemoteSnapshot(loroManager, data.doInitialSync) +);📝 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.const fireAndReport = (label: string, promise: Promise<unknown>) => { void promise.catch((error) => { console.error(`Failed ${label}`, error); }); }; // unawaited — state machine handles precedence fireAndReport( 'local snapshot ingest', ingestLocalSnapshot(loroManager, snapshotStore, walStore) ); fireAndReport('s3 snapshot ingest', ingestS3Snapshot(loroManager, blockId)); fireAndReport( 'remote snapshot ingest', ingestRemoteSnapshot(loroManager, data.doInitialSync) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/block-md/component/Block.tsx` around lines 148 - 151, The three unawaited ingest function calls (ingestLocalSnapshot, ingestS3Snapshot, and ingestRemoteSnapshot) lack error handling, which means any promise rejections become unhandled and can break initialization. Add .catch() handlers to each of these three function calls to gracefully handle any potential errors that may occur during the ingest operations, ensuring that rejections are caught and logged or handled appropriately rather than becoming unhandled promise rejections.
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
1 participant
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.
Mobile chrome system
This branch reworks mobile into a full-frame model: panels span the whole screen edge-to-edge, and floating "chrome" (header islands at top, an accessory bar + dock at the bottom) overlays the content, which scrolls beneath it. The following primitives support that — please use them rather than re-inventing.
Layout CSS variables
--mobile-content-inset-topSplitPanel(per panel)--mobile-content-inset-bottomFloatRegionHost(published on<html>)--mobile-chrome-gutter(0.75rem):rootinindex.csspx-(--mobile-chrome-gutter)so dock / accessory / header edges line up--safe-top/-bottom/-left/-right:rootConvention: a block's scrollable content carries these as in-scroll padding (e.g.
mobile:pt-(--mobile-content-inset-top) mobile:pb-(--mobile-content-inset-bottom)), so content rests clear of the chrome but under-scrolls it. There is no static panel spacer and nomobileContentAnchoranymore — each block pads its own scroller.islandutility (@utility island)The shared floating-pill look (⚠️ ui
bg-surface text-ink ring ring-edge shadow-md). Use the classisland(composes with variants, e.g.mobile:island) plus your own shape classes (size-10 rounded-full,h-10 rounded-full px-3). Don't hand-write the bg/ring/shadow combo.ButtonresetsLayerdepth to 0 — Buttons placed in mobile chrome render darker; passdepth={4}.HeaderIsland(split-layout/components/HeaderIsland)Wrap any SplitHeader contribution in this. Desktop: renders children inline (no-op). Mobile: renders a floating island pill at the chrome's Layer depth. Every header-slot contribution must be island-wrapped (directly, or via
SplitLabel/ResponsiveBlockToolbar) — there's no fallback styling for bare slot content.Float regions (
mobile/float-regions/)The bottom-chrome system. Regions are
['accessory', 'dock'](accessory above dock). Contribute with<FloatRegion region="accessory|dock" priority? active?>— children portal into the globalFloatRegionHost. Inside aSplitPanel, contributions from inactive (background-swipe) panels are auto-ignored; highestprioritywins a region, ties to most-recently-mounted. Contributions own their horizontal padding (px-(--mobile-chrome-gutter)) and must setpointer-events-auto(host is pointer-transparent). UseFloatRegionOrInlineto render in-flow on desktop and float on mobile.PillTabs(mobile/PillTabs)Presentational horizontal pill-tab strip used by the soup-view tabs, search category filters, and settings tabs.
<PillTabs items value onChange preserveFocus? class?>. Owns the scroll strip, island-pill styling,pressPulse, haptics, and auto-scroll-active-into-view.preserveFocuskeeps an open keyboard up (preventDefault on pointerdown).pressPulsedirective (mobile/pressPulse)Press feedback for floating chrome buttons:
<button use:pressPulse>(orref={pressPulse}).MobileEdgeFade(mobile/MobileEdgeFade)MobileTopEdgeFade/MobileBottomEdgeFade— surface-color scrims so content dissolves as it under-scrolls the chrome.