Feat/235 typesafety changes #238

April-Bates-Dev · 2023-09-01T13:58:12Z

No description provided.

April-Bates-Dev · 2023-09-01T13:58:47Z

package.json

-    "lint-fix": "eslint . --ext=js,ts --fix",
-    "lint-test": "eslint . --ext=js,ts",
+    "lint-fix": "eslint . --ext=js,ts,tsx --fix",
+    "lint-test": "eslint . --ext=js,ts,tsx",


[Note] We now have tsx files

April-Bates-Dev · 2023-09-01T13:59:14Z

services/core/functions/update-user/config.ts

@@ -8,7 +8,7 @@ import { buildResourceName } from "../../helpers";
 import { commonLambdaEnvironment } from "../helpers/commonLambdaEnvironment";
 import { commonLambdaProps } from "../helpers/commonLambdaProps";

-interface UpdateUserLambdaProps {
+type UpdateUserLambdaProps = {


[Note] I changed a lot of interfaces to types for consistency across the repo

April-Bates-Dev · 2023-09-01T14:00:18Z

services/core/helpers/env-helpers.ts

-  const getCdkContext = (): Record<string, unknown> | undefined => {
-    const cdkContextEnv = process.env.CDK_CONTEXT_JSON;
+const getCliCdkContext = (cliArg: string): string | undefined => {
+  const cdkContextEnv = process.env.CDK_CONTEXT_JSON ?? "";


[Note] We need a long term fix for getting the environment variables in a nice way, as there a number of fixes like this across the code base and we could do something consistent

April-Bates-Dev · 2023-09-01T14:00:54Z

services/core/helpers/env-helpers.ts

+  const parsedCdKContext: unknown = JSON.parse(cdkContextEnv);
+
+  if (isRecord(parsedCdKContext) && cliArg in parsedCdKContext) {
+    const cdkCli = parsedCdKContext[cliArg];


[Note] I had to do this type check in two stages as otherwise typescript was getting confused 😮‍💨

github-actions · 2023-09-01T14:01:03Z

Risk Level 3 - /home/runner/work/code-review-gpt/code-review-gpt/src/common/git/getFilesWithChanges.ts

The code changes in this file have a moderate risk to the code base. There are a few potential issues and areas for improvement:

The getFilesWithChanges function uses the exit function from the process module. This can cause the entire process to exit abruptly, which may not be desired. Consider throwing an error instead and handling it at a higher level.
The getFilesWithChanges function throws an error if there is a failure in getting the files with changes. It would be better to provide a more informative error message to the user.

Here are some example code snippets for the suggested changes:

Throwing an error instead of using exit:

export const getFilesWithChanges = async (
  isCi: string | undefined
): Promise<ReviewFile[]> => {
  const fileNames = await getChangedFilesNames(isCi);

  if (fileNames.length === 0) {
    throw new Error(\"No files with changes found. Please stage your changes and try again.\");
  }

  const files = await Promise.all(
    fileNames.map(async (fileName) => {
      const fileContent = await readFile(fileName, \"utf8\");
      const changedLines = await getChangedFileLines(isCi, fileName);

      return { fileName, fileContent, changedLines };
    })
  );

  return files;
};

Providing a more informative error message:

export const getFilesWithChanges = async (
  isCi: string | undefined
): Promise<ReviewFile[]> => {
  try {
    const fileNames = await getChangedFilesNames(isCi);

    if (fileNames.length === 0) {
      throw new Error(\"No files with changes found. Please stage your changes and try again.\");
    }

    const files = await Promise.all(
      fileNames.map(async (fileName) => {
        const fileContent = await readFile(fileName, \"utf8\");
        const changedLines = await getChangedFileLines(isCi, fileName);

        return { fileName, fileContent, changedLines };
      })
    );

    return files;
  } catch (error) {
    throw new Error(`Failed to get files with changes: ${error.message}`);
  }
};



---

**Risk Level 3 - /home/runner/work/code-review-gpt/code-review-gpt/services/web-app/src/components/home/Features.tsx**

The code changes in this file have a moderate risk to the code base. The changes involve adding a new import statement and updating the component to use a new Card component. The code is clean and readable. However, there is a typo in the import statement \"use client\" which should be removed.


---

**Risk Level 4 - /home/runner/work/code-review-gpt/code-review-gpt/services/web-app/src/components/home/Hero.tsx**

The code changes in this file have a high risk to the code base. The changes involve adding a new function component and using multiple framer-motion hooks and transformations. The code is complex and may introduce bugs or performance issues. It is recommended to thoroughly test the changes before merging them into the code base.



---
🔍🔧🚨



---

#### Powered by [Code Review GPT](https://github.com/mattzcarey/code-review-gpt)

April-Bates-Dev · 2023-09-01T14:01:56Z

services/web-app/src/app/layout.tsx

-import { Theme } from "@radix-ui/themes";
-import { NavBar } from "@/components/navbar";
+import Footer from "../components/footer/footer";
+import { NavBar } from "../components/navbar";


[Note] I changed all imports to relative imports so we had consistency across the code base. In the future it would be nice to use absolute paths everywhere

github-actions · 2023-09-01T14:02:35Z

Test results summary:

✅ [PASS] - Test case: Bad variable name
❌ [FAIL] - Test case: Exposed secret
✅ [PASS] - Test case: Too many nested loops
⚠️ [WARN] - Test case: Unawaited Promise

SUMMARY: ✅ PASS: 2 - ⚠️ WARN: 1 - ❌ FAIL: 1

Tests Powered by Code Review GPT

April-Bates-Dev · 2023-09-01T14:03:15Z

services/web-app/src/app/profile/page.tsx

      } finally {
        setLoading(false);
      }
    };
-    fetchData();
+    void fetchData();


[Note] I had to use void here as useEffects don't like async functions.

April-Bates-Dev · 2023-09-01T14:04:16Z

services/web-app/src/components/buttons/basicButton.tsx

@@ -1,5 +1,4 @@
 import React from "react";
-import { Button } from "@radix-ui/themes";


[Note] I'm slightly worried that we are using both tailwind and radix- I think we should choose just one as otherwise I think we are going to run into problems in future (with them fighting each other etc)

April-Bates-Dev · 2023-09-01T14:04:46Z

services/web-app/src/components/home/Hero.tsx

@@ -2,9 +2,10 @@ import { Button, Link } from "@radix-ui/themes";
 import { motion, useScroll, useSpring, useTransform } from "framer-motion";
 import React, { useRef } from "react";
 import { MdNorthEast } from "react-icons/md";
+


[Note] These changes had been made as linting hadn't been run on the code Aiden added

April-Bates-Dev · 2023-09-01T14:05:28Z

services/web-app/src/lib/hooks/useAxios.tsx

+  axiosInstance.interceptors.request.use((config) => {
+    //TODO: investigate this as the Session type returned from does not seem to contain 'token'
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+    config.headers.Authorization = session.token;


[Note] I didn't have time to investigate this :(

I'll add this to my current bug ticket :)

April-Bates-Dev · 2023-09-01T14:07:22Z

services/web-app/src/lib/types.ts

+    userId: string,
+    apiKey: string,
+    name: string,
+    pictureUrl?: string,


[Note] This is a bug- by setting "" values here you actually setting constants rather than default values- so if you tried to do

const user: User; user.name = 'Adam'

You would see an error as user.name can only be set to "".

April-Bates-Dev · 2023-09-01T14:09:52Z

src/config.ts

+    gitlabSha: process.env.CI_COMMIT_SHA ?? '',
+    gitlabToken: process.env.GITLAB_TOKEN ?? '',
+    projectId: process.env.CI_PROJECT_ID ?? '',
+    mergeRequestIIdString: process.env.CI_MERGE_REQUEST_IID ?? '',


[Note] This is again gross but I want to fix the process.env types properly and in the mean time at least our fix is consistent (this is the fix in another file)

SEBRATHEZEBRA

Looks good to me! The consistency of everything looks so much better :)

April-Bates-Dev added 13 commits August 31, 2023 17:33

fix: updating package lock

83ce724

fix: changing interfaces to types for consistency

999913a

fix: changing linting rules to cover new tsx files

5d7f38e

fix: quick lint fixes on new tsx files

fdd1b18

fix: changing eslint setup to account for new files

4352204

fix: adding return types and consistent import paths

3b09a06

fix: fixing useAxios issues

30d32f5

fix: awaiting onclick promises

afeba66

fix: error handling and async behaviour in setLoading hook

5c3fc55

fix: error handling and typesafety on Profile page

aa0974f

fix: small changes to allow front end to run

97ac46b

fix: fixing getCliCdkContext type issues

eac21ad

fix: fixing more typesafety issues

3037a29

April-Bates-Dev commented Sep 1, 2023

View reviewed changes

fix: readding removed code

14d0162

SEBRATHEZEBRA approved these changes Sep 1, 2023

View reviewed changes

April-Bates-Dev merged commit b4e257e into main Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/235 typesafety changes #238

Feat/235 typesafety changes #238

April-Bates-Dev commented Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

github-actions bot commented Sep 1, 2023 •

edited

Loading

April-Bates-Dev Sep 1, 2023

github-actions bot commented Sep 1, 2023 •

edited

Loading

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

SEBRATHEZEBRA Sep 1, 2023

April-Bates-Dev Sep 1, 2023

April-Bates-Dev Sep 1, 2023

SEBRATHEZEBRA left a comment

		@@ -1,5 +1,4 @@
		import React from "react";
		import { Button } from "@radix-ui/themes";

Feat/235 typesafety changes #238

Feat/235 typesafety changes #238

Conversation

April-Bates-Dev commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023 • edited Loading

Test results summary:

Tests Powered by Code Review GPT

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEBRATHEZEBRA left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading