Reviewing Open Saas code #369

sodic · 2025-02-17T13:22:40Z

No description provided.

sodic · 2025-02-17T19:42:18Z

template/app/README.md

+ - Make sure you have the `.env.client` and `.env.server` files with correct dev values in the root of the project.
+ - Run the database with `wasp start db` and leave it running.
+ - Run `wasp start` and leave it running.
+ - [OPTIONAL]: If this is the first time starting the app, or you've just made changes to your entities/prisma schema, also run `wasp db migrate-dev`.


Optional is probably not the right word here, since it's mandatory when it's mandatory, and has no effect otherwise.

sodic · 2025-02-17T19:44:04Z

template/app/src/admin/dashboards/users/SwitcherOne.tsx

+import { useState } from 'react';
+import { cn } from '../../../client/cn';
+
+const SwitcherOne = ({ user, updateIsUserAdminById }: { user?: Partial<User>; updateIsUserAdminById?: any }) => {


Why are we passing updateIsUserAdminById through a prop instead of importing it directly?

sodic · 2025-02-17T23:35:38Z

template/app/src/user/operations.ts

+  if (!context.user) {
+    throw new HttpError(401);
+  }
+
+  if (!context.user.isAdmin) {
+    throw new HttpError(403);
+  }


Until we get a proper middleware, we should probably cook up our own decorators for this (or at least call a function that checks everything and throws.

sodic · 2025-02-17T23:36:01Z

template/app/src/user/operations.ts

+  const updatedUser = await context.entities.User.update({
+    where: {
+      id,
+    },
+    data: {
+      isAdmin: data.isAdmin,
+    },
+  });
+
+  return updatedUser;


updatedUser can be returned directly, meaning that the await is redundant as well.

sodic · 2025-02-17T23:36:40Z

template/app/src/user/operations.ts

+  return updatedUser;
+};
+
+export const updateCurrentUserLastActiveTimestamp: UpdateCurrentUserLastActiveTimestamp<Pick<User, 'lastActiveTimestamp'>, User> = async ({ lastActiveTimestamp }, context) => {


We have plenty of long lines in this file (and other files). How come prettier doesn't take care of it? We should look into that

sodic · 2025-02-18T00:30:49Z

template/app/src/user/DropdownUser.tsx

+  const trigger = useRef<any>(null);
+  const dropdown = useRef<any>(null);
+
+  const toggleDropdown = () => setDropdownOpen((prev) => !prev);


I might be wrong, but I have a feeling React has a more native support for this (more precisely, I don't think we need this wrapper).

sodic · 2025-02-18T00:31:41Z

template/app/src/user/DropdownUser.tsx

+  const trigger = useRef<any>(null);
+  const dropdown = useRef<any>(null);


This is strange, both because of useRef (which should be used with cautionandany`.

sodic · 2025-02-18T00:32:10Z

template/app/src/user/DropdownUser.tsx

+          <span className='block text-sm font-medium dark:text-white'>{user.username}</span>
+        </span>
+        <CgProfile size='1.1rem' className='ml-1 mt-[0.1rem] dark:text-white' />
+        <svg


Again , we should probably extract SVGs into their own files.

sodic · 2025-02-18T00:37:03Z

template/app/src/user/DropdownUser.tsx

+  );
+};
+
+export default DropdownUser;


I advise against using defualt exports.

sodic · 2025-02-18T00:45:13Z

template/app/src/user/DropdownUser.tsx

TODO: Continue tomorrow (stopped at DropdownUser)

sodic

Second pass, superficial review of most modules.

sodic · 2025-02-18T15:39:44Z

template/app/src/payment/webhook.ts

+export const paymentsWebhook = paymentProcessor.webhook;
+export const paymentsMiddlewareConfigFn = paymentProcessor.webhookMiddlewareConfigFn;


This pattern looks weird, I should investigate what's going on here.

sodic · 2025-02-18T15:40:34Z

template/app/src/server/utils.ts

@@ -0,0 +1,8 @@
+export function requireNodeEnvVar(name: string): string {


I think something like this is now supported through Wasp via Zod.

sodic · 2025-02-18T15:47:43Z

template/app/src/server/scripts/dbSeeds.ts

+  const firstName = faker.person.firstName();
+  const lastName = faker.person.lastName();
+  const subscriptionStatus = faker.helpers.arrayElement<SubscriptionStatus | null>(['active', 'cancel_at_period_end', 'past_due', 'deleted', null]);
+  const now = new Date();
+  const createdAt = faker.date.past({ refDate: now });
+  const lastActiveTimestamp = faker.date.between({ from: createdAt, to: now });
+  const credits = subscriptionStatus ? 0 : faker.number.int({ min: 0, max: 10 });
+  const hasUserPaidOnStripe = !!subscriptionStatus || credits > 3 


Seems like some of these are redundant.

sodic · 2025-02-18T15:48:33Z

template/app/src/shared/common.ts

+export const DocsUrl = 'https://docs.opensaas.sh';
+export const BlogUrl = 'https://docs.opensaas.sh/blog';


These should be in all caps or in camelCase.

sodic · 2025-02-18T15:49:20Z

template/app/src/server/scripts/dbSeeds.ts

+ * For more info see: https://wasp.sh/docs/data-model/backends#seeding-the-database
+ */
+export async function seedMockUsers(prismaClient: PrismaClient) {
+  await Promise.all(generateMockUsersData(50).map((data) => 


The await is redundant.

sodic · 2025-02-18T16:16:25Z

template/app/src/demo-ai-app/DemoAppPage.tsx

+              const priorityOrder = ['low', 'medium', 'high'];
+              if (a.props.mainTask.priority && b.props.mainTask.priority) {
+                return (
+                  priorityOrder.indexOf(b.props.mainTask.priority) - priorityOrder.indexOf(a.props.mainTask.priority)
+                );
+              } else {
+                return 0;
+              }
+            })


This should probably be its own function.

sodic · 2025-02-18T16:16:29Z

template/app/src/demo-ai-app/DemoAppPage.tsx

+        )}
+      </table>
+
+      {/* ))} */}


What's this for?

sodic · 2025-02-18T16:19:32Z

template/app/src/client/App.tsx

+  const shouldDisplayAppNavBar = useMemo(() => {
+    return location.pathname !== routes.LoginRoute.build() && location.pathname !== routes.SignupRoute.build();
+  }, [location]);
+
+  const isAdminDashboard = useMemo(() => {
+    return location.pathname.startsWith('/admin');
+  }, [location]);


Why useMemo, was there a performance hit?

sodic · 2025-02-18T16:19:44Z

template/app/src/client/App.tsx

+    return location.pathname.startsWith('/admin');
+  }, [location]);
+
+  useEffect(() => {


We should name the useEffect.

sodic · 2025-02-18T16:20:04Z

template/app/src/client/App.tsx

+      const lastSeenAt = new Date(user.lastActiveTimestamp);
+      const today = new Date();
+      if (today.getTime() - lastSeenAt.getTime() > 5 * 60 * 1000) {
+        updateCurrentUserLastActiveTimestamp({ lastActiveTimestamp: today });


As mentioned, the timestamp sould be calculated on the backend.

sodic · 2025-02-19T09:03:20Z

template/app/public/.gitkeep

I don't think we need this

sodic · 2025-02-19T09:19:36Z

template/app/src/admin/layout/Header.tsx

+
+const Header = (props: {
+  sidebarOpen: string | boolean | undefined;
+  setSidebarOpen: (arg0: boolean) => void;


We should name the argument.

sodic · 2025-02-19T09:20:10Z

template/app/src/admin/layout/Sidebar.tsx

+  // close on click outside
+  useEffect(() => {


We should name the useEffect.

Applies to other places too.

sodic · 2025-02-19T09:20:35Z

template/app/src/admin/layout/Sidebar.tsx

+  const trigger = useRef<any>(null);
+  const sidebar = useRef<any>(null);


We should remove the anys.

sodic · 2025-02-19T09:20:58Z

template/app/src/admin/layout/Sidebar.tsx

+  const trigger = useRef<any>(null);
+  const sidebar = useRef<any>(null);
+
+  const storedSidebarExpanded = localStorage.getItem('sidebar-expanded');


This probably isn't the best way to do this in React.

Applies to other places too.

sodic · 2025-02-19T09:27:26Z

template/app/src/admin/layout/Sidebar.tsx

+    }
+  }, [sidebarExpanded]);
+
+  return (


The component body is too long, we should break it up.

sodic · 2025-02-19T09:30:01Z

template/app/src/admin/elements/forms/CheckboxOne.tsx

+import { useState } from 'react';
+import { cn } from '../../../client/cn';
+
+const CheckboxOne = () => {


I don't know how checkbox are done these days, but this seems like a lot.

sodic · 2025-02-19T09:30:22Z

template/app/src/admin/elements/forms/FormElementsPage.tsx

+const FormElements = ({ user }: { user: AuthUser }) => {
+  useRedirectHomeUnlessUserIsAdmin({ user });
+
+  return (


Long component, we should break it up.

sodic · 2025-02-19T09:30:33Z

template/app/src/admin/elements/forms/FormElementsPage.tsx

+import { useRedirectHomeUnlessUserIsAdmin } from '../../useRedirectHomeUnlessUserIsAdmin';
+
+const FormElements = ({ user }: { user: AuthUser }) => {
+  useRedirectHomeUnlessUserIsAdmin({ user });


Perhaps it makes sense extracting this into a decorator.

Maybe it's even something Wasp should add first -lass support for.

sodic · 2025-02-19T09:32:21Z

template/app/src/admin/elements/forms/SwitcherTwo.tsx

+import { useState } from 'react';
+import { cn } from '../../../client/cn';
+
+const SwitcherTwo = () => {


Can we make these components names more informative (e.g., SwitcherOne, CheckboxOne). I don't know what they do. Perhaps they're different designs of the same functionality. If that's the case, we should maybe somehow extract this functionality.

sodic · 2025-02-19T09:33:16Z