components/ui-components/link.tsx (1)

12-18: Consider adding accessibility indicator for external links.

External links that open in new tabs should include an accessibility indicator for screen reader users. The current implementation correctly applies rel="noopener noreferrer" for security, but lacks an indication that the link opens in a new window.

♿ Proposed accessibility enhancement

  if (isExternal) {
    return (
      <Headless.DataInteractive>
-        <a {...props} ref={ref} target="_blank" rel="noopener noreferrer" />
+        <a 
+          {...props} 
+          ref={ref} 
+          target="_blank" 
+          rel="noopener noreferrer"
+          aria-label={props['aria-label'] || `${props.children} (opens in new tab)`}
+        />
      </Headless.DataInteractive>
    );
  }

Alternatively, you can add a visual indicator (icon) or let consumers provide their own aria-label.

server/lib/posts.ts (2)

84-110: Consider null handling for user data.

The leftJoin with the user table (line 104) allows for null user fields if the user record is missing. While the schema enforces a foreign key with cascade delete (making this rare), defensive programming suggests either:

1. Using an innerJoin instead, which would return null for the entire post if user is missing
2. Adding explicit null checks and handling for user fields before constructing the user object (lines 137-141)

This prevents potential issues if database inconsistencies occur or consumers expect non-null user data.

♻️ Option 1: Use innerJoin for stricter guarantee

     .from(posts)
-    .leftJoin(user, eq(posts.authorId, user.id))
+    .innerJoin(user, eq(posts.authorId, user.id))
     .where(eq(posts.id, id))
     .limit(1);

---

114-122: Consider performance implications of two separate queries.

Splitting tag retrieval into a separate query (lines 114-122) results in two database round trips instead of one. While this approach offers clarity and flexibility, it may have performance implications if this function is called frequently.

Consider:

• Keeping the two-query approach if it's simpler to maintain
• Using a single query with proper joins if performance becomes a concern
• Adding caching if this endpoint is heavily trafficked

components/Comments/CommentsArea.tsx (1)

187-188: Replace as any type assertion with proper typing.

The as any cast bypasses type safety. Since Comments and Children types are already defined in this file (lines 119-120), consider defining a proper recursive type or using a type guard to safely access the children property.

♻️ Proposed type-safe approach

Define a proper recursive comment type at the top of the file with the other types:

+type CommentWithChildren = NonNullable<Comments>[number] & {
+  children?: CommentWithChildren[];
+};
+
 type Comments = typeof comments;
 type Children = typeof firstChild;
 type FieldName = "comment" | "reply" | "edit";

Then update the usage:

-
-      const children = (comment as any).children;
+      const children = (comment as CommentWithChildren).children;

components/editor/editor/plugins/upload-images.tsx (1)

21-22: Restore ESLint suppression or fix the underlying type issue.

The ESLint disable comment was removed, but the underlying type assertion as any on line 22 remains. This bypasses type safety without addressing the root cause. The TODO comment on line 7 indicates there are known type issues in this file.

Consider one of the following approaches:

1. Restore the ESLint suppression with a comment explaining why the type assertion is necessary
2. Fix the type issue properly by defining the correct type for the transaction metadata

Option 1: Restore suppression with explanation

 set = set.map(tr.mapping, tr.doc);
 // See if the transaction adds or removes any placeholders
+// eslint-disable-next-line @typescript-eslint/no-explicit-any -- ProseMirror meta requires any for plugin-specific data
 const action = tr.getMeta(uploadKey as any);

Option 2: Define proper type for metadata

+type UploadAction = 
+  | { add: { id: object; pos: number; src: string | ArrayBuffer } }
+  | { remove: { id: object } }
+  | undefined;
+
 const UploadImagesPlugin = () =>
   new Plugin({
     key: uploadKey as unknown as PluginKey,
     state: {
       init() {
         return DecorationSet.empty;
       },
       apply(tr, set) {
         set = set.map(tr.mapping, tr.doc);
         // See if the transaction adds or removes any placeholders
-        const action = tr.getMeta(uploadKey as any);
+        const action = tr.getMeta(uploadKey as any) as UploadAction;

components/NewsletterCTA/NewsletterCTA.tsx (1)

37-54: Consider adding aria-label to the disabled button for better accessibility.

The disabled "Subscribed" button would benefit from an explicit aria-label to provide context to screen reader users.

♿ Suggested accessibility enhancement

         {isSubscribed ? (
           <button
             disabled
+            aria-label="Already subscribed to newsletter"
             className="flex w-full cursor-not-allowed items-center justify-center gap-2 rounded-lg bg-white/80 px-6 py-2.5 text-sm font-semibold text-pink-600"
           >

context/SidebarContext.tsx (1)

26-28: Optimize toggleSidebar to avoid unnecessary re-creation.

The toggleSidebar callback includes isCollapsed in its dependency array, which causes the function to be recreated on every state change. Since you're using the updater form of setIsCollapsed, you can reference the current state without including it in dependencies.

♻️ Proposed optimization

-  const toggleSidebar = useCallback(() => {
-    setIsCollapsed(!isCollapsed);
-  }, [isCollapsed, setIsCollapsed]);
+  const toggleSidebar = useCallback(() => {
+    setIsCollapsed((prev) => !prev);
+  }, [setIsCollapsed]);

components/PostEditor/components/UrlMetadataPreview.tsx (1)

84-84: Remove empty comment.

The empty comment {} on line 84 serves no purpose and should be removed.

♻️ Cleanup suggestion

-          {}
           <img

app/api/fetch-metadata/route.ts (1)

147-157: Duplicate isValidUrl function exists in utils/utils.ts.

This function duplicates isValidUrl from utils/utils.ts (shown in relevant code snippets). The only difference is that this version explicitly checks for http: or https: protocols, which is a stricter and more secure validation for this use case.

Consider either:

1. Importing and extending the existing utility, or
2. Extracting this stricter version to a shared location

app/(app)/[username]/[slug]/page.tsx (1)

105-179: Consider consolidating getUserPost and getUserLinkPost helpers.

These two functions share ~90% identical code. The only differences are:

• Line 70 vs 146: eq(posts.type, "article") vs eq(posts.type, "link")
• A few additional fields in getUserLinkPost (externalUrl, coverImage)

Consider extracting a shared helper that accepts type and optional field selections as parameters.

♻️ Suggested consolidation

async function getUserPostByType(
  username: string, 
  postSlug: string, 
  postType: "article" | "link"
) {
  const userRecord = await db.query.user.findFirst({
    columns: { id: true },
    where: eq(user.username, username),
  });

  if (!userRecord) return null;

  const baseFields = {
    id: posts.id,
    title: posts.title,
    body: posts.body,
    excerpt: posts.excerpt,
    slug: posts.slug,
    status: posts.status,
    publishedAt: posts.publishedAt,
    updatedAt: posts.updatedAt,
    readingTime: posts.readingTime,
    showComments: posts.showComments,
    upvotesCount: posts.upvotesCount,
    downvotesCount: posts.downvotesCount,
    type: posts.type,
    // Author fields...
    authorId: user.id,
    authorName: user.name,
    authorImage: user.image,
    authorUsername: user.username,
    authorBio: user.bio,
    // Link-specific (null for articles)
    externalUrl: posts.externalUrl,
    coverImage: posts.coverImage,
    canonicalUrl: posts.canonicalUrl,
  };

  const postResults = await db
    .select(baseFields)
    .from(posts)
    .leftJoin(user, eq(posts.authorId, user.id))
    .where(
      and(
        eq(posts.slug, postSlug),
        eq(posts.authorId, userRecord.id),
        eq(posts.status, "published"),
        eq(posts.type, postType),
        lte(posts.publishedAt, new Date().toISOString()),
      ),
    )
    .limit(1);

  // ... rest of mapping logic
}

app/(app)/[username]/[slug]/_userLinkDetail.tsx (1)

101-123: Optimistic updates look good, but consider query invalidation.

The optimistic update pattern is correctly implemented. However, the mutation doesn't invalidate or update the query cache on success, which could lead to stale data if the user navigates away and returns.

Consider adding onSuccess to invalidate the query:

const utils = api.useUtils();

const { mutate: vote, status: voteStatus } = api.content.vote.useMutation({
  onMutate: async ({ voteType }) => {
    // ... existing optimistic update
  },
  onError: () => {
    // ... existing error handling
  },
  onSuccess: () => {
    utils.content.getUserLinkBySlug.invalidate({ username, slug: contentSlug });
  },
});

components/PostEditor/tabs/LinkTab.tsx (2)

37-52: Redundant title auto-population logic.

The title is auto-populated in two places: the onSuccess callback (lines 40-41) and the useEffect (lines 48-50). While both check !title before updating, this duplication could lead to confusion.

Consider consolidating this logic in one place—either keep it solely in onSuccess or solely in useEffect.

Suggested simplification (remove useEffect)

-  // Update title when metadata changes (if title is still empty)
-  useEffect(() => {
-    if (metadata?.title && !title) {
-      onTitleChange(metadata.title);
-    }
-  }, [metadata?.title, title, onTitleChange]);

The onSuccess callback already handles this case when metadata is first fetched.

---

83-97: Magic number in URL length check.

The condition url.length > 10 is arbitrary. Short URLs like http://t.co/abc (15 chars) would pass, but the threshold seems disconnected from actual URL validity.

Consider using the isValidUrl helper (already used by useLinkMetadata) or simply relying on the existing isLoading || error || metadata conditions.

Suggested change

-      {(url.length > 10 || isLoading || error || metadata) && (
+      {(isLoading || error || metadata) && (

The hook already handles URL validation internally, so the preview section will appear once a fetch starts.

server/api/router/content.ts (1)

1179-1180: Consider adding length constraint to username input.

The username field uses z.string() without length constraints. The user table enforces a 40-character limit. Consider adding .max(40) for consistency:

Suggested change

-    .input(GetContentBySlugSchema.extend({ username: z.string() }))
+    .input(GetContentBySlugSchema.extend({ username: z.string().min(1).max(40) }))

drizzle/seed-sources.ts (2)

18-25: Slug generation may preserve leading/trailing hyphens.

If a source name has leading/trailing spaces, they become hyphens before trim() can act (since trim() only removes whitespace). For the current hardcoded data this isn't an issue, but consider:

More robust slug generation

 const generateSlug = (name: string): string => {
   return name
     .toLowerCase()
+    .trim()
     .replace(/[^a-z0-9\s-]/g, "")
     .replace(/\s+/g, "-")
     .replace(/-+/g, "-")
-    .trim();
+    .replace(/^-|-$/g, "");
 };

---

424-500: Seed script processes sources sequentially without transaction.

The script creates users and sources one at a time. If it fails mid-execution, partial data remains in the database. For a seed script this is generally acceptable (re-run is idempotent due to existence checks), but consider wrapping in a transaction if atomicity is needed.

components/PostEditor/hooks/useLinkMetadata.ts (2)

38-47: Consider reusing the existing isValidUrl utility.

There's already an isValidUrl function in utils/utils.ts. While this version adds protocol validation (http/https only), consider extending the shared utility instead of duplicating it. If the protocol check is specific to this use case, at minimum rename this to isValidHttpUrl for clarity.

-function isValidUrl(url: string): boolean {
+function isValidHttpUrl(url: string): boolean {

---

66-128: Potential stale closure with metadata in dependency array.

Including metadata in the useCallback dependency array means fetchMetadata is recreated whenever metadata changes. This could cause the effect at line 158 to re-run unnecessarily since fetchMetadata is in its deps. The de-duplication check at line 69 relies on lastFetchedUrlRef anyway.

Suggested fix

  const fetchMetadata = useCallback(
    async (targetUrl: string) => {
      // Skip if already fetched this URL
-     if (targetUrl === lastFetchedUrlRef.current && metadata) {
+     if (targetUrl === lastFetchedUrlRef.current) {
        return;
      }
      // ... rest of function
    },
-   [metadata, onSuccess, onError],
+   [onSuccess, onError],
  );

components/Theme/ThemeProvider.tsx (1)

6-12: Consider respecting system preference.

Setting defaultTheme="dark" forces dark mode for new users. Consider using enableSystem with defaultTheme="system" to respect OS-level dark mode preferences for a better default UX.

- <ThemeProvider attribute="class" defaultTheme="dark">
+ <ThemeProvider attribute="class" defaultTheme="system" enableSystem>

If dark mode is an intentional design choice, this is fine as-is.

e2e/my-posts.spec.ts (1)

14-19: Consider replacing the fixed timeout with a proper wait condition.

The loading indicator wait is good for test stability, but the additional 500ms waitForTimeout is a code smell that may mask a race condition. Fixed timeouts make tests either flaky (if too short) or unnecessarily slow (if too long).

♻️ Consider waiting for actual content instead

  // Wait for content to load - wait for loading message to disappear
  await expect(page.getByText("Fetching your posts...")).toBeHidden({
    timeout: 15000,
  });
-  // Additional wait for content to render
-  await page.waitForTimeout(500);
+  // Wait for actual content to be present
+  await page.waitForSelector("article, [data-testid='empty-state']", {
+    timeout: 5000,
+  });

This waits for either an article or empty state to appear, which is a more reliable condition than an arbitrary timeout.

components/Header/MinimalHeader.tsx (2)

39-41: Consider simplifying the boolean expression.

The enabled option can be simplified.

✨ Suggested simplification

   const { data: count } = api.notification.getCount.useQuery(undefined, {
-    enabled: session ? true : false,
+    enabled: !!session,
   });

---

144-148: Consider using next/image for the user avatar.

Using the native <img> tag bypasses Next.js image optimization. Since Image from next/image is already imported (line 18), consider using it for consistency and performance benefits.

✨ Suggested change

               {session.user?.image ? (
-                  <img
+                  <Image
                     className="h-7 w-7 rounded-full bg-neutral-300 object-cover lg:h-8 lg:w-8"
                     src={session.user.image}
                     alt={`${session.user?.name}'s avatar`}
+                    width={32}
+                    height={32}
+                    unoptimized
                   />

Note: unoptimized may be needed if user images come from external domains not configured in next.config.js.

e2e/utils/utils.ts (1)

150-190: **Handle potential undefined return when using onConflictDoNothing().**When using onConflictDoNothing() with .returning(), the result is an empty array [] if there's a conflict. This means result[0] will be undefined when a conflict occurs, and the function will silently return undefined without indicating whether the insert succeeded or was skipped due to conflict.

For E2E test utilities, this may be acceptable since tests typically use unique slugs, but consider adding a check for clarity.

✨ Optional: Add explicit undefined check

     .onConflictDoNothing()
     .returning();
-    return result[0];
+    if (!result[0]) {
+      throw Error(`E2E test link post not created (likely slug conflict): ${slug}`);
+    }
+    return result[0];

components/SideBar/AppSidebar.tsx (1)

180-180: Remove unnecessary .toLowerCase() on customStyle.

The customStyle values in the socialLinks array are already lowercase (e.g., "hover:bg-twitter focus:bg-twitter"). The .toLowerCase() call is redundant.

✨ Suggested fix

-              className={`focus-style rounded-md p-1 text-neutral-400 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle.toLowerCase()}`}
+              className={`focus-style rounded-md p-1 text-neutral-400 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle}`}

e2e/editor.spec.ts (1)

446-460: Duplicate tag test assertion may be fragile.

Using span:has-text("duplicate") to count tag instances could match unintended elements. Consider using a more specific selector tied to the tag badge component.

✨ Suggested improvement

If tags have a specific class or data attribute, use that instead:

// Example: if tag badges have a specific class
const tagCount = await page.locator('.tag-badge:has-text("duplicate")').count();
// Or use a data attribute
const tagCount = await page.locator('[data-testid="tag"]:has-text("duplicate")').count();

components/ui-components/sidebar-layout.tsx (2)

24-30: Missing fill attribute for color inheritance.

Unlike MenuIcon, this SVG is missing fill="currentColor", which may cause the icon to not inherit the text color properly in dark mode or themed contexts.

Suggested fix

 function CloseMenuIcon() {
   return (
-    <svg data-slot="icon" viewBox="0 0 20 20" aria-hidden="true">
+    <svg data-slot="icon" viewBox="0 0 20 20" aria-hidden="true" fill="currentColor">
       <path d="M6.28 5.22a.75.75 0 0 0-1.06 1.06L8.94 10l-3.72 3.72a.75.75 0 1 0 1.06 1.06L10 11.06l3.72 3.72a.75.75 0 1 0 1.06-1.06L11.06 10l3.72-3.72a.75.75 0 0 0-1.06-1.06L10 8.94 6.28 5.22Z" />
     </svg>
   );
 }

---

122-126: Consider using a more specific transition property.

Using transition-all on the main content area could trigger transitions on unintended properties. Since only the left padding needs to animate, a more specific transition would be slightly more performant.

Suggested refinement

       <main
-        className={`flex flex-1 flex-col transition-all duration-300 ease-in-out lg:min-w-0 lg:pt-16 ${contentPadding}`}
+        className={`flex flex-1 flex-col transition-[padding] duration-300 ease-in-out lg:min-w-0 lg:pt-16 ${contentPadding}`}
       >

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Learnt from: RangerCreaky
Repo: codu-code/codu PR: 1146
File: server/api/router/series.ts:109-135
Timestamp: 2024-10-18T04:18:28.906Z
Learning: In the TypeScript file `server/api/router/series.ts`, when comments are included to aid reviewers, avoid suggesting their removal.

Learnt from: dineshsutihar
Repo: codu-code/codu PR: 1153
File: app/(app)/articles/[slug]/page.tsx:91-104
Timestamp: 2024-10-22T08:43:13.236Z
Learning: In `app/(app)/articles/[slug]/page.tsx`, when rendering content, the `renderedContent` needs to be treated as a string using a type assertion because it's used with `dangerouslySetInnerHTML`, and removing the type assertion could lead to runtime issues.

Learnt from: JohnAllenTech
Repo: codu-code/codu PR: 1187
File: e2e/setup.ts:23-25
Timestamp: 2024-10-28T23:51:53.631Z
Learning: In the e2e tests setup scripts, it's acceptable to hardcode IDs for test data.

Learnt from: JohnAllenTech
Repo: codu-code/codu PR: 1158
File: e2e/articles.spec.ts:208-224
Timestamp: 2024-10-20T01:10:48.663Z
Learning: In `e2e/articles.spec.ts`, tests within the `Authenticated Articles Page` describe block assume the user is already authenticated, so explicit authentication steps are not necessary within individual tests.

Learnt from: JohnAllenTech
Repo: codu-code/codu PR: 1139
File: e2e/articles.spec.ts:133-147
Timestamp: 2024-10-17T01:03:59.199Z
Learning: In `e2e/articles.spec.ts` (TypeScript Playwright tests), avoid refactoring duplicated code into helper functions unless the code is reused elsewhere.

Learnt from: JohnAllenTech
Repo: codu-code/codu PR: 1139
File: e2e/articles.spec.ts:0-0
Timestamp: 2024-10-17T01:07:41.406Z
Learning: In our end-to-end tests, using `waitUntil: "networkidle"` in `page.waitForNavigation` is unreliable because of ongoing background requests, so it should be avoided.

Learnt from: petercr
Repo: codu-code/codu PR: 1203
File: e2e/settings.spec.ts:39-41
Timestamp: 2024-11-09T15:07:15.871Z
Learning: The settings page (`app/(app)/settings/_client.tsx`) does not use a loading spinner with `data-testid="save-loading"` during the save operation.

app/(editor)/create/[[...paramsArr]]/_client.tsx (4)

106-128: Consider consolidating related state with useReducer.

With 15+ useState declarations, the component's state management is becoming complex. Related state (form fields, UI flags, publishing options) could be grouped into a reducer for better maintainability and clearer state transitions.

Example refactor pattern

type EditorState = {
  form: {
    title: string;
    body: string;
    linkUrl: string;
    linkTitle: string;
    // ... other form fields
  };
  ui: {
    viewPreview: boolean;
    showPublishConfirm: boolean;
    // ... other UI state
  };
  // ... other state groups
};

const [state, dispatch] = useReducer(editorReducer, initialState);

This would make state updates more predictable and easier to test.

---

183-183: Consider memoizing PREVIEW_URL.

The preview URL is reconstructed on every render. While this is a simple string operation, memoizing it would follow React best practices and prevent unnecessary recalculations.

Suggested refactor

-  const PREVIEW_URL = `${process.env.NODE_ENV === "development" ? "http://localhost:3000" : "https://www.codu.co"}/draft/${createData?.id || postId}`;
+  const PREVIEW_URL = useMemo(
+    () => `${process.env.NODE_ENV === "development" ? "http://localhost:3000" : "https://www.codu.co"}/draft/${createData?.id || postId}`,
+    [createData?.id, postId]
+  );

---

478-491: Consider refactoring unsaved changes tracking.

The eslint-disable comments for set-state-in-effect indicate a code smell. Setting state directly in effects based on other state values can cause unnecessary re-renders and is generally discouraged. Consider tracking unsaved changes by comparing current form values to saved values in a useMemo, or using refs to track the last saved state.

Alternative approach

// Track last saved state with refs
const lastSavedStateRef = useRef({ title: '', body: '', linkUrl: '', linkTitle: '' });

// Compute unsaved changes as derived state
const hasUnsavedChanges = useMemo(() => {
  if (activeTab === "write") {
    return title !== lastSavedStateRef.current.title || 
           body !== lastSavedStateRef.current.body;
  } else {
    return linkUrl !== lastSavedStateRef.current.linkUrl || 
           linkTitle !== lastSavedStateRef.current.linkTitle;
  }
}, [title, body, linkUrl, linkTitle, activeTab]);

// Update ref after successful save
const savePost = async () => {
  // ... save logic
  lastSavedStateRef.current = { title, body, linkUrl, linkTitle };
};

This eliminates the effects and the need for eslint-disable.

---

743-750: Consider memoizing Markdoc rendering.

The Markdoc parse/transform/render pipeline runs on every render. Memoizing this operation would prevent unnecessary reprocessing when other state changes.

Performance optimization

+  const renderedContent = useMemo(() => {
+    if (!body) return null;
+    return Markdoc.renderers.react(
+      Markdoc.transform(Markdoc.parse(body), config),
+      React,
+      { components: markdocComponents }
+    );
+  }, [body]);
+
   <article ...>
     <h1>{title || "Untitled"}</h1>
-    {Markdoc.renderers.react(
-      Markdoc.transform(Markdoc.parse(body), config),
-      React,
-      { components: markdocComponents }
-    )}
+    {renderedContent}
   </article>

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Learnt from: dineshsutihar
Repo: codu-code/codu PR: 1153
File: app/(app)/articles/[slug]/page.tsx:91-104
Timestamp: 2024-10-22T08:43:13.236Z
Learning: In `app/(app)/articles/[slug]/page.tsx`, when rendering content, the `renderedContent` needs to be treated as a string using a type assertion because it's used with `dangerouslySetInnerHTML`, and removing the type assertion could lead to runtime issues.