Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/connect-react/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
<!-- markdownlint-disable MD024 -->
# Changelog

# [1.0.0-preview.8] - 2024-12-09

- Disabled submit button when form is incomplete

# [1.0.0-preview.7] - 2024-12-05

- Use proper casing for `stringOptions` now that configure prop is properly async
Expand Down
4 changes: 2 additions & 2 deletions packages/connect-react/examples/nextjs/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions packages/connect-react/examples/nextjs/src/app/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@
componentKey="slack-send-message"
configuredProps={configuredProps}
onUpdateConfiguredProps={setConfiguredProps}
onSubmit={async () => {
try {
await client.actionRun({
userId,
actionId: "slack-send-message",
configuredProps,
});
} catch (error) {
console.error('Action run failed:', error);

Check failure on line 41 in packages/connect-react/examples/nextjs/src/app/page.tsx

View workflow job for this annotation

GitHub Actions / Lint Code Base

Strings must use doublequote
// Consider showing user-friendly error message
}
}}
/>
</FrontendClientProvider>
</>
Expand Down
18 changes: 12 additions & 6 deletions packages/connect-react/src/components/ControlSubmit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,22 @@ export type ControlSubmitProps = {

export function ControlSubmit(props: ControlSubmitProps) {
const { form } = props;
const { submitting } = form;
const {
propsNeedConfiguring, submitting,
} = form;

const {
getProps, theme,
} = useCustomize();
const baseStyles: CSSProperties = {
const baseStyles = (disabled: boolean): CSSProperties => ({
width: "fit-content",
textTransform: "capitalize",
backgroundColor: theme.colors.primary,
color: theme.colors.neutral0,
backgroundColor: disabled
? theme.colors.neutral10
: theme.colors.primary,
color: disabled
? theme.colors.neutral40
: theme.colors.neutral0,
padding: `${theme.spacing.baseUnit * 1.75}px ${
theme.spacing.baseUnit * 16
}px`,
Expand All @@ -29,9 +35,9 @@ export function ControlSubmit(props: ControlSubmitProps) {
? 0.5
: undefined,
margin: "0.5rem 0 0 0",
};
});

return <input type="submit" value={submitting
? "Submitting..."
: "Submit"} {...getProps("controlSubmit", baseStyles, props)} disabled={submitting} />;
: "Submit"} {...getProps("controlSubmit", baseStyles(propsNeedConfiguring.length || submitting), props)} disabled={propsNeedConfiguring.length || submitting} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the logic for 'disabled' and 'opacity' properties

The current logic may not correctly determine the disabled state due to type differences. Also, the opacity should reflect the disabled state.

Apply this diff to fix the logic:

 const disabled = propsNeedConfiguring.length > 0 || submitting;

 return <input type="submit" value={submitting
   ? "Submitting..."
-  : "Submit"} {...getProps("controlSubmit", baseStyles(propsNeedConfiguring.length || submitting), props)} disabled={propsNeedConfiguring.length || submitting} />;
+  : "Submit"} {...getProps("controlSubmit", baseStyles(disabled), props)} disabled={disabled} />;

Update the baseStyles function to use disabled for opacity:

 opacity: disabled
   ? 0.5
   : undefined,

Committable suggestion skipped: line range outside the PR's diff.

}
9 changes: 7 additions & 2 deletions packages/connect-react/src/components/InternalField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { FormFieldContext } from "../hooks/form-field-context";
import { useFormContext } from "../hooks/form-context";
import { Field } from "./Field";
import { useApp } from "../hooks/use-app";
import { useEffect } from "react";

type FieldInternalProps<T extends ConfigurableProp> = {
prop: T;
Expand All @@ -14,7 +15,7 @@ export function InternalField<T extends ConfigurableProp>({
}: FieldInternalProps<T>) {
const formCtx = useFormContext();
const {
id: formId, configuredProps, setConfiguredProp,
id: formId, configuredProps, registerField, setConfiguredProp,
} = formCtx;

const appSlug = prop.type === "app" && "app" in prop
Expand Down Expand Up @@ -44,7 +45,11 @@ export function InternalField<T extends ConfigurableProp>({
app, // XXX fix ts
},
};

useEffect(() => registerField(fieldCtx), [
app,
configuredProps,
prop,
])
return (
<FormFieldContext.Provider value={fieldCtx}>
<Field field={fieldCtx} form={formCtx} />
Expand Down
53 changes: 50 additions & 3 deletions packages/connect-react/src/hooks/form-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
import isEqual from "lodash.isequal";
import { useQuery } from "@tanstack/react-query";
import type {
ComponentReloadPropsOpts, ConfigurableProp, ConfigurableProps, ConfiguredProps, V1Component,
ComponentReloadPropsOpts, ConfigurableProp, ConfigurableProps, ConfiguredProps, V1Component, PropValue,

Check failure on line 7 in packages/connect-react/src/hooks/form-context.tsx

View workflow job for this annotation

GitHub Actions / Lint Code Base

'PropValue' is defined but never used
} from "@pipedream/sdk";
import { useFrontendClient } from "./frontend-client-context";
import type { ComponentFormProps } from "../components/ComponentForm";
import type { FormFieldContext } from "./form-field-context";
import { appPropError } from "./use-app";

export type DynamicProps<T extends ConfigurableProps> = { id: string; configurableProps: T; }; // TODO

Expand All @@ -17,12 +19,15 @@
configuredProps: ConfiguredProps<T>;
dynamicProps?: DynamicProps<T>; // lots of calls require dynamicProps?.id, so need to expose
dynamicPropsQueryIsFetching?: boolean;
fields: Record<string, FormFieldContext<ConfigurableProp>>;
id: string;
isValid: boolean;
optionalPropIsEnabled: (prop: ConfigurableProp) => boolean;
optionalPropSetEnabled: (prop: ConfigurableProp, enabled: boolean) => void;
props: ComponentFormProps<T>;
propsNeedConfiguring: string[];
queryDisabledIdx?: number;
registerField: <T extends ConfigurableProp>(field: FormFieldContext<T>) => void;
setConfiguredProp: (idx: number, value: unknown) => void; // XXX type safety for value (T will rarely be static right?)
setSubmitting: (submitting: boolean) => void;
submitting: boolean;
Expand Down Expand Up @@ -64,6 +69,10 @@
queryDisabledIdx,
setQueryDisabledIdx,
] = useState<number | undefined>(0);
const [
fields,
setFields,
] = useState<Record<string, FormFieldContext<ConfigurableProp>>>({});
const [
submitting,
setSubmitting,
Expand Down Expand Up @@ -129,6 +138,16 @@
enabled: reloadPropIdx != null, // TODO or props.dynamicPropsId && !dynamicProps
});

const [
propsNeedConfiguring,
setPropsNeedConfiguring,
] = useState<string[]>([]);
useEffect(() => {
checkPropsNeedConfiguring()
}, [
configuredProps,
]);

// XXX fix types of dynamicProps, props.component so this type decl not needed
let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || [];
if (propNames?.length) {
Expand All @@ -147,7 +166,7 @@

// these validations are necessary because they might override PropInput for number case for instance
// so can't rely on that base control form validation
const propErrors = (prop: ConfigurableProp, value: unknown): string[] => {
const propErrors = <T extends ConfigurableProps>(prop: ConfigurableProp, value: unknown): string[] => {

Check failure on line 169 in packages/connect-react/src/hooks/form-context.tsx

View workflow job for this annotation

GitHub Actions / Lint Code Base

'T' is defined but never used
const errs: string[] = [];
if (value === undefined) {
if (!prop.optional) {
Expand All @@ -173,7 +192,10 @@
errs.push("not a string");
}
} else if (prop.type === "app") {
// TODO need to know about auth type
const field = fields[prop.name]
const app = field.extra.app
const err = appPropError(prop, value, app)
if (err) errs.push(err)
}
return errs;
};
Expand Down Expand Up @@ -302,6 +324,27 @@
setEnabledOptionalProps(newEnabledOptionalProps);
};

const checkPropsNeedConfiguring = () => {
const _propsNeedConfiguring = []
for (const prop of configurableProps) {
if (!prop || prop.optional || prop.hidden) continue
const value = configuredProps[prop.name as keyof ConfiguredProps<T>]
const errors = propErrors(prop, value)
if (errors.length) {
_propsNeedConfiguring.push(prop.name)
}
}
// propsNeedConfiguring.splice(0, propsNeedConfiguring.length, ..._propsNeedConfiguring)
setPropsNeedConfiguring(_propsNeedConfiguring)
}

const registerField = <T extends ConfigurableProp>(field: FormFieldContext<T>) => {
setFields((fields) => {
fields[field.prop.name] = field
return fields
});
};

// console.log("***", configurableProps, configuredProps)
const value: FormContext<T> = {
id,
Expand All @@ -313,9 +356,13 @@
configuredProps,
dynamicProps,
dynamicPropsQueryIsFetching,
errors,
fields,
optionalPropIsEnabled,
optionalPropSetEnabled,
propsNeedConfiguring,
queryDisabledIdx,
registerField,
setConfiguredProp,
setSubmitting,
submitting,
Expand Down
66 changes: 65 additions & 1 deletion packages/connect-react/src/hooks/use-app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import {
useQuery, type UseQueryOptions,
} from "@tanstack/react-query";
import { useFrontendClient } from "./frontend-client-context";
import type { AppRequestResponse } from "@pipedream/sdk";
import type {
AppRequestResponse, AppResponse, ConfigurablePropApp,
PropValue,
} from "@pipedream/sdk";

/**
* Get details about an app
Expand All @@ -23,3 +26,64 @@ export const useApp = (slug: string, opts?:{ useQueryOpts?: Omit<UseQueryOptions
app: query.data?.data,
};
};

type AppResponseWithExtractedCustomFields = AppResponse & {
extracted_custom_fields_names: string[]
}

type AppCustomField = {
name: string
optional?: boolean
}

type OauthAppPropValue = PropValue<"app"> & {
oauth_access_token?: string
}

function getCustomFields(app: AppResponse): AppCustomField[] {
const isOauth = app.auth_type === "oauth"
const userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]")
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential JSON parsing errors

Using JSON.parse without error handling can throw exceptions if app.custom_fields_json contains invalid JSON. Consider adding error handling to prevent application crashes.

Apply this diff to add error handling:

 const isOauth = app.auth_type === "oauth"
-let userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]")
+let userDefinedCustomFields = [];
+try {
+  userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]");
+} catch (error) {
+  console.error("Failed to parse custom fields JSON:", error);
+}

Committable suggestion skipped: line range outside the PR's diff.

if ("extracted_custom_fields_names" in app && app.extracted_custom_fields_names) {
const extractedCustomFields = ((app as AppResponseWithExtractedCustomFields).extracted_custom_fields_names || []).map(
(name) => ({
name,
}),
)
userDefinedCustomFields.push(...extractedCustomFields)
}
return userDefinedCustomFields.map((cf: AppCustomField) => {
return {
...cf,
// if oauth, treat all as optional (they are usually needed for getting access token)
optional: cf.optional || isOauth,
}
})
}

export function appPropError(prop: ConfigurablePropApp, value: unknown, app: AppResponse | undefined): string | undefined {
if (!app) {
return "app field not registered"
}
if (!value) {
return "no app configured"
}
if (typeof value !== "object") {
return "not an app"
}
const _value = value as PropValue<"app">
if ("authProvisionId" in _value && !_value.authProvisionId) {
if (app.auth_type) {
if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) {
return "missing oauth token"
}
if (app.auth_type === "oauth" || app.auth_type === "keys") {
for (const cf of getCustomFields(app)) {
if (!cf.optional && !_value[cf.name]) {
return "missing custom field"
}
}
}
return "no auth provision configured"
}
}
}
2 changes: 0 additions & 2 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading