Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughEmailAuth: failure toast changed from `toast.failure` to `toast.alert` and message updated to instruct selecting email permissions on sign‑in; control flow still navigates to success and `AlreadyInitialized` remains treated as success. Soup-view: added `isMailView` memo, `useEmailLinksQuery` plus `hasLinkError` logic, conditional rendering of <EmailPermissionsBanner /> and UI dimming (`pointer-events-none`, `opacity-10`) when errors exist. Account: removed `connect`, moved disabled-email tooltip to a QuestionCircle button with Popover placed top and updated copy about Google email permissions. Compose: replaced inline link-error UI with <EmailPermissionsBanner /> and removed connect/warning SVG. Added exported `EmailPermissionsBanner` component that displays a message and a Logout button. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 288-307: The permissions banner UI duplicated in soup-view.tsx
(Show block using hasLinkError(), Caution, UIButton, logoutHandler) should be
extracted into a shared component (e.g., EmailPermissionsBanner) and reused from
both js/app/packages/app/component/next-soup/soup-view/soup-view.tsx and
js/app/packages/block-email/component/Compose.tsx; create EmailPermissionsBanner
that accepts props for the logout handler and any class/variant overrides, move
the markup (Caution icon, text, Logout UIButton) into that component, replace
the inline Show blocks in both files with <EmailPermissionsBanner
onLogout={logoutHandler} /> (or equivalent) while preserving the conditional
rendering via hasLinkError() in the caller or by accepting a visible prop.
In `@js/app/packages/app/component/settings/Account.tsx`:
- Around line 229-233: QuestionCircle is currently only mouse-interactive and
not keyboard-accessible; make it focusable and announce the tooltip by adding a
tabIndex={0} (or ensuring it's a focusable element), wire keyboard handlers
(onFocus -> setShowTooltip(true), onBlur -> setShowTooltip(false)), and also
handle activation keys (onKeyDown to open/close on Enter/Space) so keyboard
users can discover the tooltip; additionally add appropriate ARIA attributes
(e.g., aria-describedby pointing to the tooltip id and role="button" if it's
interactive) to the QuestionCircle component and ensure the tooltip content is
linked to that id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7070447a-6d3d-4594-885d-a9f686d79a4e
📒 Files selected for processing (4)
js/app/packages/app/component/EmailAuth.tsxjs/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Account.tsxjs/app/packages/block-email/component/Compose.tsx
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/block-email/component/Compose.tsx (1)
852-871: 🧹 Nitpick | 🔵 TrivialExtract the email permissions banner into a shared component.
This banner markup and copy is duplicated with
soup-view.tsx(lines 288-307). Consider extracting anEmailPermissionsBannercomponent that acceptsonLogoutas a prop to keep messaging and styling consistent across the application.♻️ Suggested shared component
Create a new file
js/app/packages/core/component/EmailPermissionsBanner.tsx:import Caution from '@icon/regular/warning.svg'; import { Button } from '@ui/components/Button'; interface EmailPermissionsBannerProps { onLogout: () => void; } export function EmailPermissionsBanner(props: EmailPermissionsBannerProps) { return ( <div class="w-full bg-alert-bg border-b border-t border-alert/20 text-alert-ink p-2"> <div class="flex items-center justify-between gap-2"> <Caution class="size-4" /> <span class="text-sm"> Email requires additional Google permissions. Select the permissions on sign-in to enable. </span> <span class="grow" /> <Button variant="secondary" size="sm" class="px-2" onClick={props.onLogout} > Logout </Button> </div> </div> ); }Then use it in both files:
-<Show when={hasLinkError()}> - <div class="w-full bg-alert-bg border-b border-t border-alert/20 text-alert-ink p-2"> - <div class="flex items-center justify-between gap-2"> - <Caution class="size-4" /> - <span class="text-sm"> - Email requires additional Google permissions. Select the - permissions on sign-in to enable. - </span> - <span class="grow" /> - <Button - variant="secondary" - size="sm" - class="px-2" - onClick={logoutHandler} - > - Logout - </Button> - </div> - </div> -</Show> +<Show when={hasLinkError()}> + <EmailPermissionsBanner onLogout={logoutHandler} /> +</Show>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/block-email/component/Compose.tsx` around lines 852 - 871, Extract the duplicated banner markup into a shared React component named EmailPermissionsBanner that accepts a prop onLogout: () => void; implement the component to render the same markup, icon (Caution), Button and copy used currently, then replace the duplicated blocks in Compose.tsx (the Show when={hasLinkError()} block) and in soup-view.tsx with <EmailPermissionsBanner onLogout={logoutHandler} /> (or appropriate handler), ensuring you import EmailPermissionsBanner and the same Caution and Button symbols in the new component so styling and behavior remain identical.
🤖 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/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 47-50: The file mixes a renamed import `UIButton` (from
`@ui/components/Button`) and a local `Button` (from `./filters-bar/button`),
causing inconsistent component naming; rename the local `Button` export/import
to a clearer name (e.g., `FilterButton`) at its source and update all usages in
this file (references to `Button` in `soup-view.tsx`) to `FilterButton` so you
can import the shared UI Button as `Button` (or vice versa) consistently across
the codebase; update the export in `./filters-bar/button` (component name and/or
default export) and adjust the import in `soup-view.tsx` accordingly to
eliminate the `UIButton` alias and standardize names.
---
Outside diff comments:
In `@js/app/packages/block-email/component/Compose.tsx`:
- Around line 852-871: Extract the duplicated banner markup into a shared React
component named EmailPermissionsBanner that accepts a prop onLogout: () => void;
implement the component to render the same markup, icon (Caution), Button and
copy used currently, then replace the duplicated blocks in Compose.tsx (the Show
when={hasLinkError()} block) and in soup-view.tsx with <EmailPermissionsBanner
onLogout={logoutHandler} /> (or appropriate handler), ensuring you import
EmailPermissionsBanner and the same Caution and Button symbols in the new
component so styling and behavior remain identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0baae16-cb0b-429b-b97e-d5f688d44d0e
📒 Files selected for processing (2)
js/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/block-email/component/Compose.tsx
| import { useEmailLinksQuery } from '@queries/email/link'; | ||
| import { useLogout } from '@core/auth/logout'; | ||
| import Caution from '@icon/regular/warning.svg'; | ||
| import { Button as UIButton } from '@ui/components/Button'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a consistent import alias for the Button component.
The file imports Button from @ui/components/Button as UIButton (line 50), while there's already a local Button import from ./filters-bar/button (line 103). This naming distinction is necessary to avoid conflicts, but consider whether the local Button component should be renamed (e.g., FilterButton) at its source to allow consistent naming across the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines
47 - 50, The file mixes a renamed import `UIButton` (from
`@ui/components/Button`) and a local `Button` (from `./filters-bar/button`),
causing inconsistent component naming; rename the local `Button` export/import
to a clearer name (e.g., `FilterButton`) at its source and update all usages in
this file (references to `Button` in `soup-view.tsx`) to `FilterButton` so you
can import the shared UI Button as `Button` (or vice versa) consistently across
the codebase; update the export in `./filters-bar/button` (component name and/or
default export) and adjust the import in `soup-view.tsx` accordingly to
eliminate the `UIButton` alias and standardize names.
…oltip - Extract duplicated banner markup into shared EmailPermissionsBanner component - Wrap QuestionCircle icon in a button with focus/blur handlers for keyboard accessibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/core/component/EmailPermissionsBanner.tsx`:
- Line 17: The onClick handler on the Button in EmailPermissionsBanner.tsx
invokes the async logout() without awaiting or handling errors; change the
onClick for the Button (the handler that currently calls logout()) to an async
function that awaits logout() and catches/reports errors (e.g., call
toast.failure or otherwise log/display the failure) so rejections are not
unhandled and the user gets feedback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 598ab2b5-ee36-4679-be96-1e83bed99a25
📒 Files selected for processing (4)
js/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Account.tsxjs/app/packages/block-email/component/Compose.tsxjs/app/packages/core/component/EmailPermissionsBanner.tsx
| sign-in to enable. | ||
| </span> | ||
| <span class="grow" /> | ||
| <Button variant="secondary" size="sm" class="px-2" onClick={() => logout()}> |
There was a problem hiding this comment.
Handle or explicitly discard the async logout() promise.
logout() is async (returns a Promise). Invoking it without await or .catch() means any rejection is unhandled and the user receives no feedback if logout fails.
Consider adding minimal error handling:
Suggested fix
- <Button variant="secondary" size="sm" class="px-2" onClick={() => logout()}>
+ <Button variant="secondary" size="sm" class="px-2" onClick={() => void logout()}>Or, for user feedback on failure:
onClick={async () => {
try {
await logout();
} catch {
toast.failure('Logout failed');
}
}}📝 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.
| <Button variant="secondary" size="sm" class="px-2" onClick={() => logout()}> | |
| <Button variant="secondary" size="sm" class="px-2" onClick={() => void logout()}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/EmailPermissionsBanner.tsx` at line 17, The
onClick handler on the Button in EmailPermissionsBanner.tsx invokes the async
logout() without awaiting or handling errors; change the onClick for the Button
(the handler that currently calls logout()) to an async function that awaits
logout() and catches/reports errors (e.g., call toast.failure or otherwise
log/display the failure) so rejections are not unhandled and the user gets
feedback.
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/app/component/next-soup/soup-view/soup-view.tsx`:
- Around line 198-206: The email links query runs unconditionally; change
useEmailLinksQuery to accept an options param with enabled: Accessor<boolean>
and wire that into the query's enabled flag (e.g., enabled: options?.enabled?.()
?? true) so it only runs when desired, then update the call in soup-view.tsx to
call useEmailLinksQuery({ enabled: isMailView }) instead of the unconditional
call; reference useEmailLinksQuery and isMailView when making the changes.
In `@js/app/packages/app/component/settings/Account.tsx`:
- Around line 209-210: The current element uses boolean interpolation in the
class string which can emit a literal "false" token; replace the dynamic class
construction with Solid's classList by keeping the static classes ("flex
items-center justify-between") on class and moving the conditional "mb-[18px]"
into classList keyed by the boolean expression using showEmailModal() (i.e.,
classList should apply "mb-[18px]" when !showEmailModal()). Locate the JSX
element that references showEmailModal() and update its class handling
accordingly so no boolean values are interpolated into the class string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff996c81-8806-4948-9bb3-9e126cd11173
📒 Files selected for processing (2)
js/app/packages/app/component/next-soup/soup-view/soup-view.tsxjs/app/packages/app/component/settings/Account.tsx
| const emailLinksQuery = useEmailLinksQuery(); | ||
| const hasLinkError = createMemo(() => { | ||
| if (!isMailView()) return false; | ||
| if (emailLinksQuery.isPending) return false; | ||
| return ( | ||
| emailLinksQuery.isError || | ||
| (emailLinksQuery.data && emailLinksQuery.data.links.length === 0) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if useEmailLinksQuery supports an enabled option
ast-grep --pattern $'export function useEmailLinksQuery($$$) {
$$$
}'
# Also check the query definition for createQuery options
rg -n -A 20 'useEmailLinksQuery' --type ts -g '*/queries/**'Repository: macro-inc/macro
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Check TanStack Query documentation and verify if useQuery supports enabled option
# Also search for examples of enabled option usage in the codebase
rg -n "enabled:" --type ts | head -20Repository: macro-inc/macro
Length of output: 2001
🏁 Script executed:
#!/bin/bash
# Look at the full soup-view.tsx file around the query usage
cat -n js/app/packages/app/component/next-soup/soup-view/soup-view.tsx | sed -n '190,210p'Repository: macro-inc/macro
Length of output: 809
🏁 Script executed:
#!/bin/bash
# Check if there are other conditional query examples in the codebase
rg -n "enabled:\s*(true|false|\$)" --type ts | head -30Repository: macro-inc/macro
Length of output: 2200
🏁 Script executed:
#!/bin/bash
# Get full definition of useEmailLinksQuery
cat -n js/app/packages/queries/email/link.tsRepository: macro-inc/macro
Length of output: 954
🏁 Script executed:
#!/bin/bash
# Check how other query hooks accept options (e.g., useEntitiesForProperty)
cat -n js/app/packages/app/component/property-edit-modal/hooks/useEntitiesForProperty.ts | sed -n '75,95p'Repository: macro-inc/macro
Length of output: 815
🏁 Script executed:
#!/bin/bash
# Check a query hook that accepts enabled parameter
cat -n js/app/packages/queries/preview/preview.ts | sed -n '1,50p'Repository: macro-inc/macro
Length of output: 2278
Modify useEmailLinksQuery() to conditionally enable based on view context.
Currently, useEmailLinksQuery() runs unconditionally on every soup view render, but the data is only used when isMailView() is true. Following the pattern used by other query hooks in the codebase (e.g., useEntitiesForProperty, useItemPreview), update useEmailLinksQuery() to accept an enabled options parameter:
export function useEmailLinksQuery(options?: { enabled: Accessor<boolean> }) {
return useQuery(() => ({
queryKey: emailKeys.links.queryKey,
queryFn: async () => throwOnErr(async () => await emailClient.getLinks()),
staleTime: LINK_STALE_TIME,
refetchOnWindowFocus: 'always',
enabled: options?.enabled?.() ?? true,
}));
}Then pass enabled: isMailView when calling it in soup-view.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/next-soup/soup-view/soup-view.tsx` around lines
198 - 206, The email links query runs unconditionally; change useEmailLinksQuery
to accept an options param with enabled: Accessor<boolean> and wire that into
the query's enabled flag (e.g., enabled: options?.enabled?.() ?? true) so it
only runs when desired, then update the call in soup-view.tsx to call
useEmailLinksQuery({ enabled: isMailView }) instead of the unconditional call;
reference useEmailLinksQuery and isMailView when making the changes.
| class={`flex items-center justify-between ${!showEmailModal() && 'mb-[18px]'}`} | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid boolean interpolation in class strings.
Line 209 can produce a literal false class token. Prefer classList for conditional classes.
Proposed cleanup
- <div
- class={`flex items-center justify-between ${!showEmailModal() && 'mb-[18px]'}`}
- >
+ <div
+ class="flex items-center justify-between"
+ classList={{ 'mb-[18px]': !showEmailModal() }}
+ >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/settings/Account.tsx` around lines 209 - 210,
The current element uses boolean interpolation in the class string which can
emit a literal "false" token; replace the dynamic class construction with
Solid's classList by keeping the static classes ("flex items-center
justify-between") on class and moving the conditional "mb-[18px]" into classList
keyed by the boolean expression using showEmailModal() (i.e., classList should
apply "mb-[18px]" when !showEmailModal()). Locate the JSX element that
references showEmailModal() and update its class handling accordingly so no
boolean values are interpolated into the class string.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/core/component/EmailPermissionsBanner.tsx (1)
21-21:⚠️ Potential issue | 🟡 MinorHandle
logout()failures in the click handler.At Line 21,
logout()is async and currently called without rejection handling; a failure can become an unhandled promise rejection and gives no user feedback.Suggested fix
- onClick={() => logout()} + onClick={() => { + void logout().catch((error) => { + console.error('Logout failed', error); + }); + }}#!/bin/bash set -euo pipefail echo "== useLogout implementation (verify async callback) ==" nl -ba js/app/packages/core/auth/logout.ts | sed -n '1,80p' echo echo "== EmailPermissionsBanner onClick handler ==" nl -ba js/app/packages/core/component/EmailPermissionsBanner.tsx | sed -n '15,30p'Expected result:
useLogoutwraps anasyncfunction, and this handler currently invokeslogout()without local.catch(...)handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/EmailPermissionsBanner.tsx` at line 21, The onClick handler in EmailPermissionsBanner currently calls the async logout() (from useLogout) without handling rejections; wrap the call so failures are caught and surfaced (e.g., call logout().catch(...) or make the onClick an async function and use try/catch) and ensure you surface an error to the user or UI (toast/error state) when logout() fails; update the onClick in EmailPermissionsBanner to call the safe wrapper and reference logout/useLogout so the promise rejection is handled locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/app/packages/core/component/EmailPermissionsBanner.tsx`:
- Line 21: The onClick handler in EmailPermissionsBanner currently calls the
async logout() (from useLogout) without handling rejections; wrap the call so
failures are caught and surfaced (e.g., call logout().catch(...) or make the
onClick an async function and use try/catch) and ensure you surface an error to
the user or UI (toast/error state) when logout() fails; update the onClick in
EmailPermissionsBanner to call the safe wrapper and reference logout/useLogout
so the promise rejection is handled locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce92f991-ed63-4f0e-8240-405554d965f9
📒 Files selected for processing (1)
js/app/packages/core/component/EmailPermissionsBanner.tsx
1 participant
Summary