fix: Job agent config in create deployment form #259
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx (3)
54-55: Simplify query parameters by removing default empty stringSince the query is only enabled when
workspaceis notnull, you can safely passworkspace?.idwithout the default empty string"". Passingundefinedis acceptable and avoids unintended queries with an empty string ID.Apply this diff:
- const systems = api.system.list.useQuery( - { workspaceId: workspace?.id ?? "" }, - { enabled: workspace != null }, - ); + const systems = api.system.list.useQuery( + { workspaceId: workspace?.id }, + { enabled: workspace != null }, + );
59-61: Simplify query parameters by removing default empty stringSimilarly, in the
jobAgentsQquery, you can remove the default empty string forworkspace?.idsince the query is only enabled whenworkspaceis notnull.Apply this diff:
- const jobAgentsQ = api.job.agent.byWorkspaceId.useQuery(workspace?.id ?? "", { + const jobAgentsQ = api.job.agent.byWorkspaceId.useQuery(workspace?.id, { enabled: workspace != null, });
235-239: Avoid hardcoded widths in styles for better responsivenessThe
githubFormStyleConfigpassed to theJobAgentConfigcomponent contains hardcoded widths, which might not be responsive on different screen sizes. Consider using responsive design principles or relative units to improve the UI adaptability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx(5 hunks)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx (1)
65-65: Verify that SCHEMA.createDeployment includes validations for new fields
Ensure that SCHEMA.createDeployment has validation rules for the newly added fields jobAgentId and jobAgentConfig to prevent any runtime errors due to missing validations.
| .mutateAsync(deployment) | ||
| .then(() => | ||
| router.push( | ||
| `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`, | ||
| ); | ||
| setOpen(false); | ||
| onSuccess?.(); | ||
| }) | ||
| .catch(() => { | ||
| ), | ||
| ) | ||
| .then(() => setOpen(false)) | ||
| .then(() => onSuccess?.()) | ||
| .catch(() => | ||
| setError("root", { | ||
| message: "Deployment with this slug already exists", | ||
| }); | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor onSubmit function to use async/await for better readability and error handling
The current implementation uses promise chaining with .then() and .catch(), which can lead to nested callbacks and reduced readability. Refactoring the onSubmit function to use async/await will make the code more readable and easier to maintain.
Apply this diff:
const onSubmit = handleSubmit(async (deployment) => {
+ try {
const systemSlug = systems.data?.items.find(
(system) => system.id === form.getValues("systemId"),
)?.slug;
- await createDeployment
- .mutateAsync(deployment)
- .then(() =>
- router.push(
- `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`,
- ),
- )
- .then(() => setOpen(false))
- .then(() => onSuccess?.())
- .catch(() =>
- setError("root", {
- message: "Deployment with this slug already exists",
- }),
- );
+ await createDeployment.mutateAsync(deployment);
+ router.push(
+ `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`,
+ );
+ setOpen(false);
+ onSuccess?.();
+ } catch (error) {
+ setError("root", {
+ message: "Deployment with this slug already exists",
+ });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .mutateAsync(deployment) | |
| .then(() => | |
| router.push( | |
| `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`, | |
| ); | |
| setOpen(false); | |
| onSuccess?.(); | |
| }) | |
| .catch(() => { | |
| ), | |
| ) | |
| .then(() => setOpen(false)) | |
| .then(() => onSuccess?.()) | |
| .catch(() => | |
| setError("root", { | |
| message: "Deployment with this slug already exists", | |
| }); | |
| }); | |
| }), | |
| ); | |
| try { | |
| await createDeployment.mutateAsync(deployment); | |
| router.push( | |
| `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`, | |
| ); | |
| setOpen(false); | |
| onSuccess?.(); | |
| } catch (error) { | |
| setError("root", { | |
| message: "Deployment with this slug already exists", | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx (2)
59-63: Consider adding type safety for jobAgents initializationThe initialization looks good, but could benefit from explicit typing for better type safety.
- const jobAgents = jobAgentsResult ?? []; + const jobAgents: typeof jobAgentsResult = jobAgentsResult ?? [];
238-242: Consider extracting hardcoded style valuesThe hardcoded style values for the GitHub form configuration could be moved to a constants file or theme configuration for better maintainability.
+ // In a separate constants file + export const GITHUB_FORM_STYLE_CONFIG = { + className: "flex flex-col gap-2 items-center w-[450px]", + buttonWidth: "w-[450px]", + } as const; // In the component - githubFormStyleConfig={{ - className: "flex flex-col gap-2 items-center w-[450px]", - buttonWidth: "w-[450px]", - }} + githubFormStyleConfig={GITHUB_FORM_STYLE_CONFIG}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx(4 hunks)
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx (3)
79-82: LGTM! Clean implementation of job agent selection
The job agent selection logic is well-implemented with proper null safety and efficient array lookup.
128-132: LGTM! Clean loading state implementation
The loading state and spinner implementation is well done with proper centering and animation.
89-108: 🛠️ Refactor suggestion
Refactor submission logic to use async/await
The promise chain could be simplified using async/await for better readability and error handling.
Previous suggestion still applies:
const onSubmit = handleSubmit(async (deployment) => {
+ try {
const systemSlug = systems?.items.find(
(system) => system.id === deployment.systemId,
)?.slug;
- await createDeployment
- .mutateAsync(deployment)
- .then(
- () =>
- systemSlug != null &&
- router.push(
- `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`,
- ),
- )
- .then(() => setOpen(false))
- .then(() => onSuccess?.())
- .catch(() =>
- setError("root", {
- message: "Deployment with this slug already exists",
- }),
- );
+ await createDeployment.mutateAsync(deployment);
+ if (systemSlug != null) {
+ await router.push(
+ `/${workspaceSlug}/systems/${systemSlug}/deployments/${deployment.slug}`,
+ );
+ }
+ setOpen(false);
+ onSuccess?.();
+ } catch (error) {
+ setError("root", {
+ message: "Deployment with this slug already exists",
+ });
+ }
});
adityachoudhari26
deleted the
job-agent-config-create-deployment-dialog
branch
Summary by CodeRabbit
New Features
Improvements