feat: convex optimizations, mobile studio ui#22
Conversation
…and supporting UI components, alongside new schema, migrations, and utility files.
…, and history drawers
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a mobile-first Studio surface (bottom nav, drawers/sheets), modularized gallery components (virtualized/standard grids, ThumbnailData), moves heavy generation fields into a new generatedImageDetails table with migration tooling, and expands tests, hooks, and CSS for mobile behavior and animation tuning. Changes
Sequence Diagram(s)sequenceDiagram
participant User as rgba(25,130,196,0.5)
participant Nav as rgba(46,204,113,0.5)
participant Drawer as rgba(155,89,182,0.5)
participant Shell as rgba(241,196,15,0.5)
participant Convex as rgba(231,76,60,0.5)
User->>Nav: Tap Generate FAB
Nav->>Drawer: request close editor drawer (if open)
Nav->>Shell: call onGenerate()
Shell->>Shell: run pre-checks (balance, batch, debounce)
Shell->>Convex: start generation mutation
Convex-->>Shell: return result
Shell-->>User: update UI (generating state, toast, gallery refresh)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…lection and virtualization.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/generatedImages.ts (1)
840-866: Missing cascade delete forgeneratedImageDetailsrecords.When deleting an image via
remove(and similarlyremoveMany), the associatedgeneratedImageDetailsrecord is not deleted, leaving orphaned data.🔧 Suggested fix for `remove`
handler: async (ctx, args) => { // ... existing validation ... const r2Key = image.r2Key const thumbnailR2Key = image.thumbnailR2Key + // Delete associated details record if it exists + const details = await ctx.db + .query("generatedImageDetails") + .withIndex("by_image", (q) => q.eq("imageId", args.imageId)) + .unique(); + if (details) { + await ctx.db.delete(details._id); + } await ctx.db.delete(args.imageId) return { r2Key, thumbnailR2Key } },Apply similar logic to
removeManyinside the map callback.
🤖 Fix all issues with AI agents
In @.agent/investigations/mobile-sidebar-control.md:
- Around line 121-131: The fenced code block showing the component tree
(containing SidebarProvider, Sidebar, SidebarContent, sidebarContent,
ScrollArea, PromptFeature, ControlsFeature, and Generate Button) is missing a
language tag; update the opening triple-backtick to include a language
identifier (e.g., ```text) so the block becomes a fenced code block with a
language and resolves the MD040 lint warning.
In `@components/gallery/history-filters.tsx`:
- Around line 234-260: The "Select all"/"Clear" buttons inside the "Image
Models" CommandGroup currently call handleSelectAllModels/handleClearModels
which select/clear both image and video models; change the UI to use
section-specific handlers: add handleSelectAllImageModels and
handleClearImageModels (and ensure Video Models uses
handleSelectAllVideoModels/handleClearVideoModels) and wire those into the
Button onClick callbacks in the "Image Models" CommandGroup so the buttons only
affect image models; keep existing generic handlers if needed elsewhere.
In `@components/studio/canvas/image-canvas.test.tsx`:
- Around line 34-39: The test "shows empty state when no image is provided"
currently only asserts the container exists via
screen.getByTestId("image-canvas"); update it to actually verify the empty-state
indicator or rename the test to reflect the current assertion. Either rename the
it(...) title to something like "renders image canvas container when no image
provided" or add an assertion that checks the empty-state icon (e.g., query for
the ImagePlus element by its test id/alt/aria label) after rendering
<ImageCanvas image={null} />; ensure you reference the ImageCanvas component,
the it(...) test name, and the existing screen.getByTestId("image-canvas") call
when making the change.
In `@components/studio/gallery/image-gallery.tsx`:
- Around line 270-272: The select-all checkbox uses selectedIds.size ===
images.length which is wrong when selectedIds contains ids outside the current
filtered/visible images; update the checked logic to compare only against
visible images (e.g., compute const allVisibleSelected = images.length > 0 &&
images.every(img => selectedIds.has(img.id)) or const visibleSelectedCount =
images.filter(i => selectedIds.has(i.id)).length and use visibleSelectedCount
=== images.length) and use that in the checked prop where the checkbox is
rendered (referencing images and selectedIds in the image-gallery component).
- Around line 39-51: Move the global console.error patch out of module scope and
into a scoped dev-only effect inside the ImageGallery component (or the
component that renders this file) so it does not permanently replace
console.error; specifically, detect development mode, capture the original
console.error into a local variable (originalError), set console.error to the
wrapper that filters the "flushSync was called from inside a lifecycle method"
message, and restore originalError in the effect cleanup to avoid repeated
wrapping across mounts/tests; also ensure the wrapper is installed only once per
mount (not on every render) by using a mounting-only hook (e.g., useEffect with
empty deps) and guard against double-wrapping.
In `@components/studio/gallery/virtualized-gallery-grid.test.tsx`:
- Around line 51-112: The afterEach currently only restores
global.IntersectionObserver; restore all other globals and prototype overrides
to avoid leaking test state: reassign global.ResizeObserver back to its original
(capture originalResizeObserver in the beforeEach), restore
HTMLElement.prototype.getBoundingClientRect (capture
originalGetBoundingClientRect), and redefine/remove the custom offsetHeight and
clientHeight descriptors (capture original descriptors via
Object.getOwnPropertyDescriptor and restore them in afterEach). Ensure these
captures happen at the top of beforeEach (e.g., originalResizeObserver,
originalGetBoundingClientRect, originalOffsetHeightDescriptor,
originalClientHeightDescriptor) and then reapply them in afterEach alongside
restoring originalIntersectionObserver and clearing mocks.
In `@components/studio/mobile/mobile-studio-layout.tsx`:
- Around line 134-159: The bottom padding on the canvas (the main element with
className containing "pb-16") doesn't match the actual bottom nav height used by
MobileStudioNavigation ("h-20"), causing 16px of content to sit under the nav;
update the canvas padding to match the nav height (replace "pb-16" with "pb-20"
or use a shared constant/utility) in the main container that has
data-testid="mobile-canvas-container" so the canvas reserved space equals the
MobileStudioNavigation height.
In `@convex/detailsMigration.ts`:
- Around line 156-215: The migration currently only scans the latest BATCH_SIZE
* 2 rows in migrateAllDetails and can incorrectly return complete when older
generatedImages lack generatedImageDetails; change the handler to paginate
through all generatedImages (or iterate via a cursor) rather than a single fixed
window: use repeated queries with .take(BATCH_SIZE) and an offset/cursor until
no more images remain, for each page check
generatedImageDetails.withIndex("by_image", q => q.eq("imageId",
image._id)).unique() and create missing details, accumulating totalCreated and
stopping only when the full set has been scanned (or when no pages left), or
alternatively prefetch all existing generatedImageDetails imageIds and compare
against all generatedImages to find true missing records before returning
complete. Ensure you remove the BATCH_SIZE * 2 shortcut and rely on full
pagination so batchCreated === 0 truly means there are no remaining missing
records.
- Around line 373-435: The bug is that remaining is computed from withLegacy
which only counts items seen before the early break; change the loop so you
still scan allImages to count legacy items even when the batch strip limit is
reached: remove the early "if (stripped >= BATCH_SIZE) break;" and instead,
after computing hasLegacy and incrementing withLegacy, if stripped >= BATCH_SIZE
then continue (skip DB patch/strip work) so that withLegacy reflects the total
across allImages; then compute remaining = withLegacy - stripped as before.
Update references in stripLegacyFields, allImages, withLegacy, stripped, and
BATCH_SIZE accordingly.
In `@scripts/audit-tests.ts`:
- Around line 87-116: The code uses hardcoded backslashes for path checks and
splitting (in the sourceFiles filter and when computing dir), causing
cross-platform failures; import dirname and sep from 'path', normalize file
paths with path.normalize (or replace both slashes with path.sep) before
performing contains checks (e.g., replace file.includes("convex\\_generated")
and file.includes("components\\ui") with checks against a normalized path), and
replace the manual relPath.split("\\") logic with dirname(relPath) to compute
dir, ensuring comparisons against SHADCN_COMPONENTS and TEST_EXTENSIONS still
use normalized paths when constructing baseWithoutExt + tExt or looking up
testFiles.
In `@todo/bandwidth-audit-analysis.md`:
- Around line 105-109: The "Next Steps" section contradicts the document's Part
2 which already marks the P0 items as complete; update the Next Steps so it
reflects the current state by either removing or marking completed the items
that are already done: remove or change the entries for adding by_owner_status
indexes in schema.ts, updating getUserActiveBatches and getActiveGenerations to
use the indexes, and the generatedImages table split to indicate they are
complete (or move any genuinely remaining work into a clear "Outstanding" list).
Ensure the section references the exact symbols: schema.ts,
getUserActiveBatches, getActiveGenerations, and generatedImages table so readers
can quickly verify status.
In `@todo/mobile-experience-spec.md`:
- Line 71: Fix the typo in the documentation line describing scroll handling:
change "nativey" to "natively" in the sentence "* **Scroll Handling**: Remove
nested `ScrollArea` if the Drawer handles scrolling nativey, to prevent "scroll
chaining" issues." so it reads "natively"; no other changes required—update the
single occurrence of "nativey" in the mobile-experience-spec content.
🧹 Nitpick comments (22)
components/studio/batch/batch-config-button.tsx (1)
44-50: Consider handling empty input gracefully.When the user clears the input field,
parseInt("", 10)returnsNaN, which is correctly filtered by!isNaN(value). However, this means the input can become empty with no update, potentially leaving the UI in a confusing state where the displayed value differs from the internal state.Consider resetting to a default value (e.g., 1) when the input is cleared or invalid:
💡 Optional: Reset to minimum on invalid input
const handleCountChange = (e: React.ChangeEvent<HTMLInputElement>) => { const value = parseInt(e.target.value, 10) - if (!isNaN(value)) { - const clampedValue = Math.min(Math.max(value, 1), 1000) - onSettingsChange({ ...settings, count: clampedValue }) - } + const clampedValue = isNaN(value) ? 1 : Math.min(Math.max(value, 1), 1000) + onSettingsChange({ ...settings, count: clampedValue }) }components/studio/batch/batch-config-button.test.tsx (1)
328-342: Consider adding a test for empty/invalid input handling.The tests cover clamping for values above max and below min, but there's no test for what happens when the input is cleared (empty string) or contains non-numeric text. This would document the expected behavior.
💡 Optional: Add test for invalid input
it("does not call onSettingsChange when input is empty", () => { const onSettingsChange = vi.fn() render( <BatchConfigButton {...defaultProps} settings={{ enabled: true, count: 10 }} onSettingsChange={onSettingsChange} /> ) const input = screen.getByTestId("batch-count-input") fireEvent.change(input, { target: { value: "" } }) // Current behavior: no callback on invalid input expect(onSettingsChange).not.toHaveBeenCalled() })hooks/use-local-storage.ts (2)
28-63: Avoidas TJSON.parse assertions; add a validator/codec.Casting parsed storage data to
Tcan mask corruption and bypass type safety. Consider an optional type guard/decoder so invalid data falls back safely. As per coding guidelines, avoidascasts.♻️ Suggested refactor (optional type guard)
-export function useLocalStorage<T>( - key: string, - initialValue: T, -): [T, (value: T | ((val: T) => T)) => void] { +export function useLocalStorage<T>( + key: string, + initialValue: T, + options?: { isValid?: (value: unknown) => value is T }, +): [T, (value: T | ((val: T) => T)) => void] { // Always initialize with initialValue for SSR hydration safety const [storedValue, setStoredValue] = useState<T>(initialValue); @@ - return JSON.parse(item) as T; + const parsed: unknown = JSON.parse(item); + const isValid = + options?.isValid ?? ((_: unknown): _ is T => true); + if (isValid(parsed)) return parsed; } } catch (error) { console.warn(`Error reading localStorage key "${storageKey}":`, error); } return initialValue; }, - [initialValue], + [initialValue, options?.isValid], );
86-112: ReusereadFromStoragein the storage event handler.This avoids duplicating parse logic and removes the
as Tcast while keeping guard behavior consistent. As per coding guidelines, avoidascasts.♻️ Suggested refactor
- const handleStorageChange = (e: StorageEvent) => { - if (e.key === key) { - if (e.newValue !== null && e.newValue !== "undefined") { - try { - const parsed = JSON.parse(e.newValue) as T; - setStoredValue(parsed); - } catch (error) { - console.warn( - `Error parsing storage event for key "${key}":`, - error, - ); - } - } else { - // Key was removed or set to null - setStoredValue(initialValue); - } - } - }; + const handleStorageChange = (e: StorageEvent) => { + if (e.key !== key) return; + if (e.newValue === null || e.newValue === "undefined") { + setStoredValue(initialValue); + return; + } + setStoredValue(readFromStorage(key)); + }; @@ - }, [key, initialValue]); + }, [key, initialValue, readFromStorage]);components/studio/gallery/gallery-thumbnail.tsx (1)
112-119: Consider condensing the inline comments.The multi-line comment explaining Tailwind merge behavior is verbose and somewhat speculative. Since
cn(usingtailwind-merge) is a standard pattern in this codebase, a brief note suffices:Suggested simplification
- // Use template literal to allow overriding size classes via className - // If className contains width/height classes, they will win due to tailwind precedence (if configured) - // or we rely on cn to merge. But standard tailwind merge might not override if not conflicting. - // Actually, sizeClasses[size] adds w-X h-X. - // To allow responsive sizing (w-full h-auto), we need to ensure those classes take precedence or are strictly applied. - // In this case, passing "w-full h-auto aspect-square" in className should override fixed sizes if using proper tw-merge (cn uses it). + // Size classes can be overridden via className (tailwind-merge handles conflicts) sizeClasses[size], classNamecomponents/studio/mobile/mobile-editor-drawer.test.tsx (1)
16-30: UnusedonOpenChangeparameter in mock.The
onOpenChangeprop is declared in the type but never used in the mock implementation. While this doesn't break tests, it's worth either removing from the type or adding a comment that it's intentionally ignored.🧹 Minor cleanup
Drawer: ({ children, open, - }: { children: React.ReactNode open?: boolean - onOpenChange?: (open: boolean) => void + // onOpenChange intentionally not used in mock - component behavior tested via props }) => (components/settings/profile-card.test.tsx (1)
44-51: Fragile skeleton detection approach.Using
classList.contains("animate-pulse")to detect skeletons couples the test to implementation details. If the skeleton implementation changes (e.g., different animation class), this test will break silently.Consider adding a
data-testidto the skeleton component for more robust testing, or document this coupling as intentional.components/studio/mobile/mobile-studio-navigation.tsx (1)
126-180: Respect reduced‑motion for pulse/ping animations.
Consider gating the generating animations withmotion-safeand disabling them undermotion-reducefor accessibility.🧩 Suggested adjustment
- isGenerating && "animate-pulse ring-primary/40", + isGenerating && + "motion-safe:animate-pulse motion-reduce:animate-none ring-primary/40", @@ - className="absolute inset-0 rounded-full bg-primary animate-ping opacity-30" + className="absolute inset-0 rounded-full bg-primary motion-safe:animate-ping motion-reduce:animate-none opacity-30" @@ - isGenerating && "animate-pulse", + isGenerating && + "motion-safe:animate-pulse motion-reduce:animate-none",convex/detailsMigration.ts (1)
102-119: Centralize legacy-field access instead of repeatingascasts. The same cast pattern appears again in Phase 2 helpers; consider a small helper that returns legacy fields via runtime checks to keep strict typing and reduce duplication. As per coding guidelines, avoidascasts in TypeScript.convex/detailsMigration.test.ts (1)
10-52: Reduce repeatedas unknown ascasts by switching to runtime checks or a small helper. This keeps the tests strict without depending on unchecked casts, and makes the assertions clearer.As per coding guidelines, avoid
ascasts in TypeScript.Also applies to: 55-81
convex/batchGeneration.test.ts (1)
34-121: Consider replacing repeatedas unknown ascasts with runtime checks or a helper. This keeps the tests strict and avoids unchecked casts throughout the file.As per coding guidelines, avoid
ascasts in TypeScript.components/studio/gallery/image-gallery.test.tsx (1)
98-113: Avoidas unknown asfor the IntersectionObserver mock.
You can type the mock without casts to keep TS strict.♻️ Suggested refactor
class MockIntersectionObserver implements IntersectionObserver { readonly root: Element | Document | null = null; readonly rootMargin: string = ""; readonly thresholds: readonly number[] = []; constructor(private callback: IntersectionObserverCallback) {} @@ takeRecords = vi.fn(() => [] as IntersectionObserverEntry[]); } -global.IntersectionObserver = - MockIntersectionObserver as unknown as typeof IntersectionObserver; +const IntersectionObserverMock: typeof IntersectionObserver = + MockIntersectionObserver; +global.IntersectionObserver = IntersectionObserverMock;components/studio/mobile/mobile-history-drawer.test.tsx (1)
15-29: Align the Drawer mock with real Vaul behavior.
Current mock hides children when closed, but real Vaul renders them; tests that assert “not rendered” can give false confidence. Consider rendering children regardless ofopenand assert on visibility/context instead.Also applies to: 196-205
convex/singleGeneration.ts (1)
224-240: Avoidv.any()for generationParams; reuse the validator.
This bypasses schema guarantees and weakens type safety. PrefergenerationParamsValidator(or a narrowed schema) for consistency. As per coding guidelines, avoidanyin TypeScript.♻️ Suggested fix
export const storeGeneratedImage = internalMutation({ args: { @@ contentType: v.string(), sizeBytes: v.number(), - generationParams: v.any(), + generationParams: generationParamsValidator, visibility: v.union(v.literal("public"), v.literal("unlisted")), },components/studio/gallery/standard-gallery-grid.test.tsx (1)
55-73: Avoidas unknown asin the IntersectionObserver mock.
You can assign the mock using a typed const and keep strict TS.♻️ Suggested refactor
- global.IntersectionObserver = class MockIntersectionObserver + class MockIntersectionObserver implements IntersectionObserver { @@ takeRecords = vi.fn(() => []); - } as unknown as typeof IntersectionObserver; + } + const IntersectionObserverMock: typeof IntersectionObserver = + MockIntersectionObserver; + global.IntersectionObserver = IntersectionObserverMock;components/studio/mobile/mobile-editor-drawer.tsx (1)
66-81: Consider usingsatisfiesor spreading to avoid the double cast.The
as unknown as (number | string)[]cast onSNAP_POINTS(line 71) works around the readonly tuple type but violates the guideline againstascasts. A cleaner alternative:🔧 Suggested improvement
- snapPoints={SNAP_POINTS as unknown as (number | string)[]} + snapPoints={[...SNAP_POINTS]}Spreading creates a mutable copy, avoiding the need for type assertions.
components/studio/gallery/virtualized-gallery-grid.tsx (2)
192-207: Extract duplicated scroll container lookup into a helper.The DOM traversal logic to find the actual scroll container (lines 195-207) is duplicated in the failsafe effect (lines 275-288). Consider extracting this into a reusable helper function.
♻️ Suggested refactor
+ // Helper to find the actual scroll container (handles mobile drawer case) + const findActualScrollContainer = React.useCallback( + (scrollContainer: HTMLElement): HTMLElement => { + if (!isMobile || !drawerState) return scrollContainer; + let parent = scrollContainer.parentElement; + while (parent) { + const styles = window.getComputedStyle(parent); + if (styles.overflowY === "auto" || styles.overflowY === "scroll") { + return parent; + } + parent = parent.parentElement; + } + return scrollContainer; + }, + [isMobile, drawerState] + );Then use
findActualScrollContainer(scrollContainer)in both effects.
264-320: Failsafe timer may trigger redundantonLoadMorecalls.The failsafe effect runs whenever
isLoadingMoretransitions tofalse, but if the IntersectionObserver is still observing and the sentinel is visible, both could fireonLoadMorearound the same time. The 100ms delay helps but doesn't fully prevent this.Consider tracking whether a load was already triggered via a ref to prevent duplicate calls:
🔧 Suggested improvement
+ const loadTriggeredRef = React.useRef(false); + + // Reset on successful load + React.useEffect(() => { + if (!isLoadingMore) { + loadTriggeredRef.current = false; + } + }, [isLoadingMore]); // In both the observer callback and failsafe: - onLoadMore(); + if (!loadTriggeredRef.current) { + loadTriggeredRef.current = true; + onLoadMore(); + }components/studio/gallery/standard-gallery-grid.tsx (1)
73-82: Inconsistent animation delay with VirtualizedGalleryGrid.This component uses a 350ms delay (line 79) while
VirtualizedGalleryGriduses 250ms. This inconsistency could cause different infinite-scroll behavior depending on which grid variant is rendered.🔧 Suggested fix
Consider extracting the delay value to a shared constant in
types.ts:// In types.ts export const DRAWER_ANIMATION_DELAY_MS = 300; // Vaul ~300ms animation + bufferThen import and use in both grid components for consistency.
components/studio/mobile/mobile-history-drawer.tsx (1)
107-119: Same snap points type cast as MobileEditorDrawer.Line 112 has the same
as unknown aspattern. Consider the spread approach here as well for consistency.🔧 Suggested fix
- snapPoints={SNAP_POINTS as unknown as (number | string)[]} + snapPoints={[...SNAP_POINTS]}components/studio/gallery/persistent-image-gallery.tsx (1)
319-337: Avoidascasts in the image-mapping branch.
A tiny union type guard keeps this path type-safe and removes theas ThumbnailDatacasts.♻️ Suggested refactor
- const mappedImages = React.useMemo(() => { + type ConvexHistoryImage = (typeof convexResults)[number]; + type GalleryImage = ConvexHistoryImage | ThumbnailData; + const isConvexHistoryImage = ( + img: GalleryImage, + ): img is ConvexHistoryImage => "_id" in img; + + const mappedImages = React.useMemo(() => { const newCache = new Map<string, ThumbnailData>(); const stableImages = results.map((img) => { // Handle both Convex results and cached ThumbnailData // Convex results have _id, cached ThumbnailData has id - const id = "_id" in img ? String(img._id) : (img as ThumbnailData).id; + const id = isConvexHistoryImage(img) ? String(img._id) : img.id; const cached = imageCache.current.get(id); // Get values from either format const url = img.url; const visibility = img.visibility; const model = img.model; const contentType = img.contentType; - const creationTime = - "_creationTime" in img - ? img._creationTime - : (img as ThumbnailData)._creationTime; + const creationTime = img._creationTime;As per coding guidelines, avoid
ascasts in TypeScript.components/studio/gallery/image-gallery.tsx (1)
127-135: Avoidascast onwindowand clean up debug ref.Use a global
Windowdeclaration instead of a type assertion, and remove the property on unmount to prevent stale refs. As per coding guidelines, avoidascasts.♻️ Suggested refactor
+declare global { + interface Window { + __galleryScrollRef?: React.RefObject<HTMLDivElement | null>; + } +}- React.useEffect(() => { - if (typeof window !== "undefined") { - interface GalleryWindow extends Window { - __galleryScrollRef?: React.RefObject<HTMLDivElement | null>; - } - (window as GalleryWindow).__galleryScrollRef = scrollContainerRef; - } - }, []); + React.useEffect(() => { + window.__galleryScrollRef = scrollContainerRef; + return () => { + delete window.__galleryScrollRef; + }; + }, []);
…utdated documentation and assets.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/studio/canvas/image-canvas.tsx (1)
156-162: Add accessible empty-state text.
With the icon-only empty state, screen readers won’t get any context. Add ansr-onlylabel (or aria-label) so the idle state is announced.✅ Suggested fix
- <div className={cn( + <div className={cn( "w-20 h-20 flex items-center justify-center rounded-3xl", "bg-foreground/[0.02] backdrop-blur-sm border border-white/5", "shadow-[0_4px_20px_-4px_rgba(0,0,0,0.1)]" )}> - <ImagePlus data-testid="empty-state-icon" className="h-8 w-8 text-foreground/20" strokeWidth={1.5} /> + <span className="sr-only">No image yet</span> + <ImagePlus aria-hidden="true" data-testid="empty-state-icon" className="h-8 w-8 text-foreground/20" strokeWidth={1.5} /> </div>
♻️ Duplicate comments (1)
components/studio/gallery/image-gallery.tsx (1)
39-51: Scope and clean up the globalconsole.errorpatch.This module-scope override leaks across the app and tests. Please scope it (dev-only) and restore on unmount.
🧹 Nitpick comments (4)
components/studio/mobile/mobile-studio-layout.tsx (1)
47-53: Unused propsbatchSettingsandisBatchActivein interface.These props are declared in the interface but are not destructured or used anywhere in the component implementation. Either remove them from the interface or implement their functionality.
♻️ Option 1: Remove unused props if not needed yet
- /** Batch mode settings (mobile only) */ - batchSettings?: { - enabled: boolean - count: number - } - /** Whether batch is active (mobile only) */ - isBatchActive?: boolean - /** Selection mode state for gallery bulk actions */Alternatively, if these are planned for future use, add a TODO comment explaining the intended usage, or pass them to
MobileStudioNavigationif it supports them.untested-files.md (1)
1-7: Consider excluding this generated file from version control.This file is auto-generated by
scripts/audit-tests.tswith a timestamp that changes on each run. Tracking it in git will cause unnecessary churn. Consider addinguntested-files.mdto.gitignoreand regenerating it on demand, or remove the timestamp from line 3 if you prefer to track the file for CI/documentation purposes.scripts/audit-tests.ts (1)
133-135: Consider documenting the Bun runtime requirement or adding a Node.js fallback.
Bun.writeis Bun-specific. If the project consistently uses Bun, this is fine. Otherwise, consider adding a fallback usingfs/promises.writeFilefor Node.js compatibility:♻️ Optional: Node.js compatible alternative
+import { writeFile } from "fs/promises"; + // At line 133: - await Bun.write("untested-files.md", output); + // Use Bun.write if available, otherwise fall back to Node.js fs + if (typeof Bun !== "undefined") { + await Bun.write("untested-files.md", output); + } else { + await writeFile("untested-files.md", output, "utf-8"); + }components/studio/gallery/virtualized-gallery-grid.test.tsx (1)
83-100: Avoid double-casting the mock IntersectionObserver.You can type the mock constructor directly instead of
as unknown as, which aligns with the “avoidascasts” guideline.♻️ Proposed refactor
- global.IntersectionObserver = class MockIntersectionObserver - implements IntersectionObserver { + class MockIntersectionObserver implements IntersectionObserver { readonly root: Element | Document | null = null; readonly rootMargin: string = ""; readonly thresholds: readonly number[] = []; @@ observe = observeMock; unobserve = vi.fn(); disconnect = disconnectMock; takeRecords = vi.fn(() => []); - } as unknown as typeof IntersectionObserver; + } + global.IntersectionObserver = MockIntersectionObserver;As per coding guidelines, avoid
ascasts where a typed constructor assignment works.
… parameter types, and add TypeScript ignore for custom fetch
…ersectionObserver mocking - Add explicit generic type parameters to vi.fn() calls in gallery grid tests for observe, unobserve, disconnect, and takeRecords methods - Replace direct property assignments with method implementations in IntersectionObserver mock to ensure proper type safety - Update batch-config-button test to properly handle data-vaul-no-drag attribute as a destructured parameter - Add missing batch mode props (batchSettings, isBatchActive) to mobile studio layout component - Enhance mobile studio navigation with batch mode awareness for generate button state and aria labels - Improve type safety across test mocks by explicitly defining callback signatures for IntersectionObserver constructor - Update Convex library files with improved type definitions and error handling - These changes ensure better type checking during testing and reduce reliance on type assertions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/use-batch-mode.test.ts (1)
2-4: MissingafterEachimport causes runtime error.
afterEachis used on line 83 but is not imported fromvitest. This will cause aReferenceErrorat runtime.🐛 Proposed fix
import { act, renderHook } from "@testing-library/react" -import { beforeEach, describe, expect, it, vi } from "vitest" +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" import { useBatchMode } from "./use-batch-mode"
🧹 Nitpick comments (5)
components/studio/gallery/virtualized-gallery-grid.test.tsx (1)
198-216: Consider moving timer management toafterEachfor robustness.If an assertion fails before
vi.useRealTimers()is called, fake timers remain active for subsequent tests. This pattern is fragile and can cause cascading test failures that are difficult to debug.♻️ Suggested improvement
Move timer setup/teardown to the test lifecycle:
beforeEach(() => { + vi.useFakeTimers(); // ... existing setup }); afterEach(() => { + vi.useRealTimers(); // ... existing teardown });Then remove
vi.useFakeTimers()andvi.useRealTimers()from individual tests. Alternatively, wrap each test body in atry/finallyblock to ensurevi.useRealTimers()is always called.components/studio/batch/batch-config-button.test.tsx (1)
19-53: Avoidascasts by typing captured PopoverContent props.Define a concrete type for
capturedPopoverContentPropsand use a small helper to assert handlers instead ofascasting. This keeps the tests type-safe and aligns with the TS guidelines.♻️ Proposed refactor
-// Mock Radix Popover to capture the event handler props -let capturedPopoverContentProps: Record<string, unknown> = {} +type CapturedPopoverContentProps = { + onPointerDownOutside?: (e: { preventDefault: () => void }) => void + onFocusOutside?: (e: { preventDefault: () => void }) => void + onInteractOutside?: (e: { preventDefault: () => void }) => void + "data-vaul-no-drag"?: unknown +} + +// Mock Radix Popover to capture the event handler props +let capturedPopoverContentProps: CapturedPopoverContentProps = {} + +const requireHandler = <T,>(handler: T | undefined, name: string): T => { + if (!handler) { + throw new Error(`${name} handler not provided`) + } + return handler +}-const handler = capturedPopoverContentProps.onPointerDownOutside as ( - e: { preventDefault: () => void } -) => void -expect(handler).toBeDefined() +const handler = requireHandler( + capturedPopoverContentProps.onPointerDownOutside, + "onPointerDownOutside" +)-const handler = capturedPopoverContentProps.onFocusOutside as ( - e: { preventDefault: () => void } -) => void -expect(handler).toBeDefined() +const handler = requireHandler( + capturedPopoverContentProps.onFocusOutside, + "onFocusOutside" +)-const handler = capturedPopoverContentProps.onInteractOutside as ( - e: { preventDefault: () => void } -) => void -expect(handler).toBeDefined() +const handler = requireHandler( + capturedPopoverContentProps.onInteractOutside, + "onInteractOutside" +)As per coding guidelines, avoid
ascasts in TypeScript.Also applies to: 214-261
components/studio/mobile/mobile-studio-navigation.tsx (1)
219-221: Optional: announce selection count updates for screen readers.This makes the live count change more discoverable for assistive tech users.
♿️ Suggested tweak
- <span className="text-sm font-medium text-foreground"> + <span + className="text-sm font-medium text-foreground" + aria-live="polite" + aria-atomic="true" + > {count} selected </span>convex/lib/openrouter.test.ts (1)
7-14: Type the mockedfetchFnto avoid implicitany.This keeps mocks aligned with the actual fetch signature and adheres to the TS no-
anyguideline.♻️ Proposed refactor
function createMockDeps(overrides: Partial<OpenRouterDeps> = {}): OpenRouterDeps { + const fetchFn = vi.fn< + Parameters<OpenRouterDeps["fetchFn"]>, + ReturnType<OpenRouterDeps["fetchFn"]> + >(); return { apiKey: "test-api-key", - fetchFn: vi.fn(), + fetchFn, timeoutMs: 30_000, ...overrides, }; }convex/lib/openrouter.ts (1)
28-33: AlignfetchFntyping with the globalfetchsignature.This prevents type narrowing (string-only URL) and matches how
fetchis typed elsewhere (e.g., Groq).♻️ Proposed refactor
+export type FetchFn = (...args: Parameters<typeof fetch>) => ReturnType<typeof fetch>; export interface OpenRouterDeps { apiKey: string | undefined; - fetchFn: (url: string, init?: RequestInit) => Promise<Response>; + fetchFn: FetchFn; timeoutMs: number; }
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.