Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(frontend): add typechecks and fix existing type errors in frontend #9336

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Jan 26, 2025

We want to be able to use typechecking and see errors before they occur. This is a PR to help enable us to do so by fixing the existing errors and hopefully not causing new ones.

Changes 🏗️

  • adds check to ci
  • disables some code points
  • fixes lots of type errors
  • fixes a bunch of the stories

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • added types
    • Ran some of the stories
    • Asked all the relevant parties for manual checks

Copy link

supabase bot commented Jan 26, 2025

This pull request has been ignored for the connected project bgwpwdsxblryihinutbx because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Copy link
Contributor

This PR targets the master branch but does not come from dev or a hotfix/* branch.

Automatically setting the base branch to dev.

@github-actions github-actions bot changed the base branch from master to dev January 26, 2025 11:36
@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end size/xl labels Jan 26, 2025
Copy link

netlify bot commented Jan 26, 2025

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit d725e10
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6796466aa0315c0008e85aca

Comment on lines +4 to +15
// import { getReviewableAgents } from "@/components/admin/marketplace/actions";
// import AdminMarketplaceAgentList from "@/components/admin/marketplace/AdminMarketplaceAgentList";
// import AdminFeaturedAgentsControl from "@/components/admin/marketplace/AdminFeaturedAgentsControl";
import { Separator } from "@/components/ui/separator";
async function AdminMarketplace() {
const reviewableAgents = await getReviewableAgents();
// const reviewableAgents = await getReviewableAgents();

return (
<>
<AdminMarketplaceAgentList agents={reviewableAgents.items} />
<Separator className="my-4" />
<AdminFeaturedAgentsControl className="mt-4" />
{/* <AdminMarketplaceAgentList agents={reviewableAgents.items} />
<Separator className="my-4" />
<AdminFeaturedAgentsControl className="mt-4" /> */}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swiftyos is this chill?

Copy link

netlify bot commented Jan 26, 2025

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit d725e10
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/6796466abc885a0008c46e51

name: "AI Video Generator",
storeListingVersionId: "123",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing attribute + extra attribute

menuItemGroups: defaultMenuItemGroups,
},
};

export const WithActiveLink: Story = {
args: {
...Default.args,
activeLink: "/library",
// activeLink: "/library",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old inputs -- I think?

}

export const NavbarLink = ({ name, href }: NavbarLinkProps) => {
export const NavbarLink = ({ name, href, className }: NavbarLinkProps) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we reallly accept classNames here? its to fix us passing them in externally but like why would you do that to a navbar link? it should be styled internally imo

description: "URL of the user's profile image",
},
links: {
profile: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data shape changes fixed

],
profile: {
name: "Olivia Grace",
username: "@ograce1421",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably swap this example lol but this just fixes the data shape

description: string;
additionalImages?: string[];
};
initialData?: PublishAgentInfoInitialData;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used in the story so exported here

console.log("Rating card closed");
},
// onSubmit: (rating) => {
// console.log("Rating submitted:", rating);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swiftyos is there a better way to handle these

@@ -44,6 +44,9 @@ export const Rejected: Story = {
};

export const AllStatuses: Story = {
args: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a funny requirement because how the story is defined. We could do this better but its fine for now

@@ -25,12 +26,14 @@ interface AgentsSectionProps {
sectionTitle: string;
agents: Agent[];
hideAvatars?: boolean;
className?: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another input that seems weird and should be not here. Why would an agent section need external styling?

@@ -3,14 +3,14 @@ import { ReactNode } from "react";

export function LaunchDarklyProvider({ children }: { children: ReactNode }) {
if (
process.env.NEXT_PUBLIC_LAUNCHDARKLY_ENABLED === true &&
process.env.NEXT_PUBLIC_LAUNCHDARKLY_ENABLED === "true" &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aarushik93 is this okay or will this break the world?

@@ -661,20 +660,6 @@ export const NodeGenericInputField: FC<{
handleInputClick={handleInputClick}
/>
);
case "object":
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was an impossible case

@@ -72,10 +80,300 @@ const FormExample = () => {
};

export const Default: Story = {
args: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there has to be a better way to do this

@@ -7,7 +7,13 @@ import { cn } from "@/lib/utils";

const TooltipProvider = TooltipPrimitive.Provider;

const Tooltip = ({ children, delayDuration = 10 }) => (
const Tooltip = ({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing type info -- added

@@ -65,18 +65,21 @@ export type BlockIOObjectSubSchema = BlockIOSubSchemaMeta & {
properties: { [key: string]: BlockIOSubSchema };
default?: { [key: keyof BlockIOObjectSubSchema["properties"]]: any };
required?: (keyof BlockIOObjectSubSchema["properties"])[];
secret?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these changes need mirrored to the backend anywhere?

@ntindle ntindle marked this pull request as ready for review January 26, 2025 12:22
@ntindle ntindle requested review from a team as code owners January 26, 2025 12:22
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Data Loss

When deleting a node, only node.data is saved in history payload instead of full node object, which could lead to incomplete undo functionality

payload: { node: deletedNodeData.data },
Type Error

Environment variable comparison uses strict equality with 'true' string instead of checking for string value 'true', which could cause feature flag to be incorrectly disabled

process.env.NEXT_PUBLIC_LAUNCHDARKLY_ENABLED === "true" &&
!process.env.NEXT_PUBLIC_LAUNCHDARKLY_CLIENT_ID

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end Review effort [1-5]: 2 size/xl
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants