feat(image): Show image preview on hover over image link#2142
Conversation
WalkthroughAdded a new type guard 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 |
![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/core/component/DocumentPreview.tsx`:
- Around line 270-304: ImageCoverStrip is calling
storageServiceClient.getBinaryDocument directly (and swallowing errors and not
cleaning up blob URLs), which violates the rule to centralize network calls in
Tanstack Query hooks; refactor by creating/using a query hook in the queries
package (e.g., useBinaryDocumentQuery that accepts documentId and returns {
data, isLoading, error }) instead of calling
storageServiceClient.getBinaryDocument inside ImageCoverStrip, remove the manual
createSignal/createEffect blobUrl flow and rely on query.data?.blobUrl for the
Show/img src, and ensure the query hook handles cancellation/cleanup and
surfaces errors (remove the empty catch and any manual blob URL lifecycle
management in ImageCoverStrip).
- Around line 698-712: Replace the inline type assertions with an explicit type
guard: add a predicate function isPreviewItemNoAccess(obj): obj is
PreviewItemNoAccess that checks for the discriminant (e.g., obj?.access) and
then use it in the Match when clauses (e.g., when={isPreviewItemNoAccess(item())
&& item().access === 'no_access'} and when={isPreviewItemNoAccess(item()) &&
item().access === 'does_not_exist'}) so the JSX for Unauthorized and NotFound is
type-narrowed without casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e4695d5-9e96-449f-9acb-511b2782cae2
📒 Files selected for processing (1)
js/app/packages/core/component/DocumentPreview.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
js/app/packages/core/component/DocumentPreview.tsx (2)
701-709: 🧹 Nitpick | 🔵 TrivialDrop the
PreviewItemNoAccessassertions in theseMatchconditions.These casts sidestep the union’s narrowing and will silently drift if
PreviewItemchanges. Use an inline predicate (!item().loading && item().access === ...) or add anisPreviewItemNoAccesshelper alongside the other preview predicates.♻️ Small cleanup
- <Match when={(item() as PreviewItemNoAccess).access === 'no_access'}> + <Match when={!item().loading && item().access === 'no_access'}> ... - when={(item() as PreviewItemNoAccess).access === 'does_not_exist'} + when={!item().loading && item().access === 'does_not_exist'}As per coding guidelines: "Use type-driven design with types guiding function composition."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/DocumentPreview.tsx` around lines 701 - 709, The Match conditions use unsafe casts to PreviewItemNoAccess (e.g., (item() as PreviewItemNoAccess).access) which bypasses type narrowing; replace those casts with a runtime-safe predicate such as checking !item().loading && item().access === 'no_access' (and similar for 'does_not_exist'), or add an isPreviewItemNoAccess type-guard helper and use it in the Match when expression; update the Match when clauses that reference item(), PreviewItemNoAccess, and access to use the predicate/helper so the union is properly narrowed at runtime.
272-280:⚠️ Potential issue | 🟠 MajorRoute this image load through
@queriesinstead of fetching in the component.This effect calls
storageServiceClientdirectly from a UI component, so repeated hovers bypass query caching/deduplication and leave stale/error handling to ad hoc signal state. IfdocumentIdchanges or the request fails, the previous image can remain becauseblobUrlis never reset and the emptycatchhides the failure. Based on learnings: "All network calls to service clients must be done through Tanstack query in thequeriespackage. DO NOT introduce code that calls any client fromservice-clientsoutside of the queries package."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/DocumentPreview.tsx` around lines 272 - 280, The component directly calls storageServiceClient.getBinaryDocument inside createEffect (using props.documentId and setBlobUrl), which bypasses Tanstack query caching and hides errors; move this network call into the queries package by adding (or using) a query such as getBinaryDocument/getBinaryDocumentQuery that accepts documentId, returns blobUrl/error state, and handles retries; then replace the createEffect call with the query hook (or useQuery) from the queries package in DocumentPreview, derive blobUrl from the query result, and ensure blobUrl is cleared/reset when props.documentId changes and errors are surfaced instead of swallowed.
🤖 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/DocumentPreview.tsx`:
- Line 608: In DocumentPreview.tsx locate the accessory metadata row div (the
element with class "text-[0.8em] text-current/60 mt-1 select-none") and replace
the raw Tailwind color utility "text-current/60" with the project semantic text
token (e.g. a text-ink-* token used across previews such as text-ink-600 or the
designated secondary ink token) so the accessory note uses the semantic color
system and stays aligned with the rest of the preview palette.
---
Duplicate comments:
In `@js/app/packages/core/component/DocumentPreview.tsx`:
- Around line 701-709: The Match conditions use unsafe casts to
PreviewItemNoAccess (e.g., (item() as PreviewItemNoAccess).access) which
bypasses type narrowing; replace those casts with a runtime-safe predicate such
as checking !item().loading && item().access === 'no_access' (and similar for
'does_not_exist'), or add an isPreviewItemNoAccess type-guard helper and use it
in the Match when expression; update the Match when clauses that reference
item(), PreviewItemNoAccess, and access to use the predicate/helper so the union
is properly narrowed at runtime.
- Around line 272-280: The component directly calls
storageServiceClient.getBinaryDocument inside createEffect (using
props.documentId and setBlobUrl), which bypasses Tanstack query caching and
hides errors; move this network call into the queries package by adding (or
using) a query such as getBinaryDocument/getBinaryDocumentQuery that accepts
documentId, returns blobUrl/error state, and handles retries; then replace the
createEffect call with the query hook (or useQuery) from the queries package in
DocumentPreview, derive blobUrl from the query result, and ensure blobUrl is
cleared/reset when props.documentId changes and errors are surfaced instead of
swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcb0302e-801f-4a8a-87d0-7fb9ec78ba8b
📒 Files selected for processing (1)
js/app/packages/core/component/DocumentPreview.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/core/component/DocumentPreview.tsx (1)
595-602:⚠️ Potential issue | 🟡 MinorUse a semantic text token for the accessory note.
text-current/60is a raw Tailwind color utility. Replace with a semantic token liketext-ink-mutedto maintain consistency with the project's design system.Suggested fix
<Show when={accessories()}> {(acc) => ( - <div class="text-[0.8em] text-current/60 mt-1 select-none"> + <div class="text-[0.8em] text-ink-muted mt-1 select-none"> {`${acc().note} `} {getMentionsIcon(acc().icon)} </div> )} </Show>As per coding guidelines: "Use semantic color tokens instead of default tailwind styles for colors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/DocumentPreview.tsx` around lines 595 - 602, Replace the raw Tailwind color utility on the accessory note with the project's semantic token: in DocumentPreview.tsx locate the Show block that renders accessories() (the arrow function using acc() and acc().note) and change the class string that currently includes "text-current/60" to use the semantic token "text-ink-muted" (e.g., replace the token in the div class where acc().note is rendered) so the accessory note uses the design system color token instead of a raw utility.
🤖 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/DocumentPreview.tsx`:
- Around line 595-602: Replace the raw Tailwind color utility on the accessory
note with the project's semantic token: in DocumentPreview.tsx locate the Show
block that renders accessories() (the arrow function using acc() and acc().note)
and change the class string that currently includes "text-current/60" to use the
semantic token "text-ink-muted" (e.g., replace the token in the div class where
acc().note is rendered) so the accessory note uses the design system color token
instead of a raw utility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 939bd902-d501-4b07-b148-9734a20d148d
📒 Files selected for processing (5)
js/app/packages/core/component/DocumentPreview.tsxjs/app/packages/queries/preview/index.tsjs/app/packages/queries/preview/types.tsjs/app/packages/queries/storage/binary-document.tsjs/app/packages/queries/storage/keys.ts
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/DocumentPreview.tsx`:
- Around line 633-656: The fallback branches currently call item().owner and
item().updatedAt directly, exposing raw identifiers and bypassing the narrowed
accessibleItem() typing; update both fallbacks to reuse the existing UserInfo
component (pass accessibleItem().owner/id as needed) for the display name and
use accessibleItem().updatedAt with formatDate for the timestamp so both footer
branches remain typed and user-facing—replace the owner().replace('macro|', '')
usage with UserInfo (e.g., <UserInfo userId={accessibleItem().owner or id} />)
and replace item().updatedAt with accessibleItem().updatedAt when rendering
MetadataInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 988addd4-beed-4bf8-994f-12b80164a501
📒 Files selected for processing (1)
js/app/packages/core/component/DocumentPreview.tsx
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/DocumentPreview.tsx`:
- Around line 544-548: The container currently forces a fixed height via the
isImageType() conditional resulting in " h-[220px]" on the root div in
DocumentPreview, which lets the middle flex-1 strip collapse; instead remove the
fixed height from the root and apply bounded sizing to the image preview element
(the element rendered when isImageType() is true) using min/max-height or a
fixed max-height (e.g., max-h-220) so the image area stays visible, make the
overall card a flex column with the metadata strip set to overflow-auto and
flex-shrink-0 or a bounded height so metadata scrolls rather than collapsing the
image, and ensure the image uses object-contain/object-cover and does not
flex-shrink (reference isImageType(), the root div class build, and the image
preview DOM node) so the header/footer remain variable while the image preview
is preserved.
- Around line 268-290: ImageCoverStrip currently only renders the <img> when
query.data is present, leaving the reserved area blank while the binary fetch is
pending; update ImageCoverStrip (and its use of useBinaryDocumentQuery) to
render a placeholder/skeleton inside the same container while query is loading
(use query.isLoading or query.isFetching) and then replace it with the <img>
when query.data resolves, or alternatively render a low-fidelity blurred
background (e.g., CSS gradient or base64 tiny placeholder) until query.data is
available; ensure the placeholder uses the same sizing/positioning classes so
the strip never appears empty and keep the existing onLoad opacity transition
for the returned image.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b447864b-c6e2-4505-befa-f458d6f4eb01
📒 Files selected for processing (1)
js/app/packages/core/component/DocumentPreview.tsx
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/DocumentPreview.tsx`:
- Around line 268-292: ImageCoverStrip currently calls useBinaryDocumentQuery on
mount/hover which invokes getBinaryDocument and may download the full
attachment; confirm whether getBinaryDocument returns a lightweight preview
(thumbnail) and if it does not, change behavior by either switching
useBinaryDocumentQuery to request a preview/thumbnail variant (add a
preview/size flag) or defer the query until explicit open (e.g., trigger on
click/expand) to avoid downloading large binaries on hover; check and update
usages of ImageCoverStrip, useBinaryDocumentQuery, and getBinaryDocument to
ensure the request asks for a thumbnail-sized resource or is lazy-triggered.
In `@js/app/packages/core/component/HoverCard.tsx`:
- Line 104: HoverCard.tsx currently wraps the entire props.content in a bare
Suspense which causes the whole hover popup to suspend when DocumentPreview.tsx
fetches image data; instead, remove the top-level Suspense around props.content
and either (a) move the Suspense boundary down into DocumentPreview (or the
specific async image component) so only the image suspends, or (b) supply a
Suspense with a fixed-size fallback that preserves the card chrome while the
image loads; update HoverCard.tsx to render props.content directly and adjust
DocumentPreview.tsx (or the image subcomponent) to include Suspense with an
explicit fixed-size fallback if you cannot localize the async boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9eea2ab-a320-48cd-ad88-783771fc6bb8
📒 Files selected for processing (3)
js/app/packages/app/index.cssjs/app/packages/core/component/DocumentPreview.tsxjs/app/packages/core/component/HoverCard.tsx
| function ImageCoverStrip(props: { documentId: string; class?: string }) { | ||
| const query = useBinaryDocumentQuery(() => props.documentId); | ||
|
|
||
| return ( | ||
| <div | ||
| class={`w-full overflow-hidden relative bg-edge-muted ${props.class ?? 'h-32'}`} | ||
| > | ||
| <Show when={query.data}> | ||
| {(url) => ( | ||
| <img | ||
| src={url()} | ||
| class="absolute inset-0 w-full h-full object-cover opacity-0 transition-opacity duration-300" | ||
| onLoad={(e) => { | ||
| const img = e.target as HTMLImageElement; | ||
| requestAnimationFrame(() => { | ||
| img.style.opacity = '1'; | ||
| }); | ||
| }} | ||
| alt="" | ||
| /> | ||
| )} | ||
| </Show> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== binary-document query implementation =="
fd 'binary-document.ts$' | while read -r file; do
echo "--- $file ---"
sed -n '1,220p' "$file"
done
echo
echo "== storage API shape =="
rg -n -C3 'getBinaryDocument|binaryDocumentQueryOptions|thumbnail|resize' .Repository: macro-inc/macro
Length of output: 50372
Verify whether getBinaryDocument returns a preview or the full binary.
ImageCoverStrip mounts on hover and immediately triggers useBinaryDocumentQuery, which calls getBinaryDocument. Confirm that this endpoint returns a lightweight preview (e.g., thumbnail or low-resolution preview) rather than the full attachment. If it fetches the original, hovers will trigger expensive network transfers. The current 5-minute cache and 10-minute GC mitigate repeated hovers, but the first hover should not pull a large binary unnecessarily. If the endpoint returns the full attachment, defer it until explicit open or request a separate thumbnail-sized endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/DocumentPreview.tsx` around lines 268 - 292,
ImageCoverStrip currently calls useBinaryDocumentQuery on mount/hover which
invokes getBinaryDocument and may download the full attachment; confirm whether
getBinaryDocument returns a lightweight preview (thumbnail) and if it does not,
change behavior by either switching useBinaryDocumentQuery to request a
preview/thumbnail variant (add a preview/size flag) or defer the query until
explicit open (e.g., trigger on click/expand) to avoid downloading large
binaries on hover; check and update usages of ImageCoverStrip,
useBinaryDocumentQuery, and getBinaryDocument to ensure the request asks for a
thumbnail-sized resource or is lazy-triggered.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/core/component/HoverCard.tsx (1)
106-108:⚠️ Potential issue | 🟠 MajorOnly opt into Suspense when a fallback is actually supplied.
Solid keeps a suspended subtree out of the DOM until the tracked resources resolve, and TanStack Query documents
query.dataas a Suspense-triggering Solid resource. BecausesuspenseFallbackis optional here, any hover-card content that suspends but does not pass one now renders as an empty popup instead of using its existing loading path. Either make the fallback mandatory for async content or skip wrapping when it is absent. (docs.solidjs.com)♻️ Proposed fix
import { createContext, createEffect, createSignal, onCleanup, + Show, Suspense, useContext, } from 'solid-js'; @@ - <Suspense fallback={props.suspenseFallback}> - {props.content} - </Suspense> + <Show when={props.suspenseFallback} fallback={props.content}> + {(fallback) => ( + <Suspense fallback={fallback()}>{props.content}</Suspense> + )} + </Show>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/HoverCard.tsx` around lines 106 - 108, HoverCard currently always wraps props.content in Solid's Suspense which can hide suspended content when no fallback is supplied; change the render logic in HoverCard (around the Suspense usage) to only wrap props.content with <Suspense fallback={props.suspenseFallback}> when props.suspenseFallback is non-null/defined, otherwise render props.content directly (or alternatively make props.suspenseFallback a required prop and update types/signature accordingly); update the JSX in HoverCard.tsx to conditionally choose between Suspense wrapping and direct rendering so suspended resources render their existing loading path when no fallback is provided.
🤖 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/HoverCard.tsx`:
- Around line 106-108: HoverCard currently always wraps props.content in Solid's
Suspense which can hide suspended content when no fallback is supplied; change
the render logic in HoverCard (around the Suspense usage) to only wrap
props.content with <Suspense fallback={props.suspenseFallback}> when
props.suspenseFallback is non-null/defined, otherwise render props.content
directly (or alternatively make props.suspenseFallback a required prop and
update types/signature accordingly); update the JSX in HoverCard.tsx to
conditionally choose between Suspense wrapping and direct rendering so suspended
resources render their existing loading path when no fallback is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd5bc29a-5bf2-462d-a12c-bf19112c4176
📒 Files selected for processing (2)
js/app/packages/core/component/DocumentPreview.tsxjs/app/packages/core/component/HoverCard.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/core/component/HoverCard.tsx (1)
11-11:⚠️ Potential issue | 🟠 MajorKeep the async boundary out of the shared
HoverCard.Solid Query’s
datais a Solid resource, so reading unresolved query data inside a<Suspense>boundary suspends that boundary; Solid’s nested suspense guidance is to keep the boundary around the local async region when you don’t want the larger shell to wait. Moving this boundary up into the generic hover container means any suspendable child can blank the whole popup instead of just its async leaf. (tanstack.com)♻️ Proposed fix
import { createContext, createEffect, createSignal, onCleanup, - Suspense, useContext, } from 'solid-js'; @@ - <Suspense>{props.content}</Suspense> + {props.content}As per coding guidelines: "Make primitive UI components pure and prefer composition over props vs complex global state or context"
Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/component/HoverCard.tsx` at line 11, The shared HoverCard component currently includes a Suspense boundary (remove the Suspense import/usage from the HoverCard component) — this makes the entire popup suspend when any child reads unresolved Solid Query resources; instead, make HoverCard a pure presentation component (keep functions/props like HoverCard, HoverCardTrigger, HoverCardContent intact) and move any Suspense wrapping into the specific async child components that consume Solid Query resources (e.g., the component using useQuery inside the popup), so only those leaves suspend rather than the whole hover container.
🤖 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/HoverCard.tsx`:
- Line 11: The shared HoverCard component currently includes a Suspense boundary
(remove the Suspense import/usage from the HoverCard component) — this makes the
entire popup suspend when any child reads unresolved Solid Query resources;
instead, make HoverCard a pure presentation component (keep functions/props like
HoverCard, HoverCardTrigger, HoverCardContent intact) and move any Suspense
wrapping into the specific async child components that consume Solid Query
resources (e.g., the component using useQuery inside the popup), so only those
leaves suspend rather than the whole hover container.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6558356e-53d3-4d0b-8940-75e2e4a593a2
📒 Files selected for processing (2)
js/app/packages/core/component/DocumentPreview.tsxjs/app/packages/core/component/HoverCard.tsx
1 participant
Adds partial image preview to link hover preview popup