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

feat: Workflows function calling support #603

Merged
merged 31 commits into from
Oct 29, 2024
Merged

Conversation

ramedina86
Copy link
Collaborator

No description provided.

</template>

<script setup lang="ts">
import * as monaco from "monaco-editor";
Copy link
Collaborator

@madeindjs madeindjs Oct 28, 2024

Choose a reason for hiding this comment

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

You might need to use the ESM import, it might improve bundle size. From their sample, they use

Suggested change
import * as monaco from "monaco-editor";
import * as monaco from "monaco-editor/esm/vs/editor/editor.api";

Not tested though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Monaco import was a mess back in the day. I'll create a task and you can look into it in detail, perform tests, etc.

graphId: "999-999",
};

const toolForm: Ref<ToolForm> = ref(toolFormInitValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const toolForm: Ref<ToolForm> = ref(toolFormInitValue);
const toolForm = ref<ToolForm>(toolFormInitValue);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This syntax is really nice, I didn't know it existed, thanks for bringing it up.

src/ui/src/builder/BuilderFieldsTools.vue Outdated Show resolved Hide resolved
Comment on lines 142 to 169
function getToolFromForm(): Tool {
const { type, code, graphId } = toolForm.value;
if (type == "function") {
return {
...JSON.parse(code),
type,
};
}
if (type == "graph") {
return {
type,
graph_ids: [graphId],
};
}
throw "Unexpected tool type.";
}

function validateToolForm(form: ToolForm): string[] {
let errors = [];
const { name } = form;
if (!name) {
errors.push("The name cannot be empty.");
}
if (Object.keys(tools.value).includes(name)) {
errors.push("An existing tool with the specified name already exists.");
}
return errors;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional, but I think it's a better pattern to use computed here. For example.

Suggested change
function getToolFromForm(): Tool {
const { type, code, graphId } = toolForm.value;
if (type == "function") {
return {
...JSON.parse(code),
type,
};
}
if (type == "graph") {
return {
type,
graph_ids: [graphId],
};
}
throw "Unexpected tool type.";
}
function validateToolForm(form: ToolForm): string[] {
let errors = [];
const { name } = form;
if (!name) {
errors.push("The name cannot be empty.");
}
if (Object.keys(tools.value).includes(name)) {
errors.push("An existing tool with the specified name already exists.");
}
return errors;
}
const tooolFromForm = computed<Tool | null>(() => {
const { type, code, graphId } = toolForm.value;
if (type == "function") {
return {
...JSON.parse(code),
type,
};
}
if (type == "graph") {
return {
type,
graph_ids: [graphId],
};
}
return null;
});
const toolFormErrors = computed<string[]>(() => {
let errors = [];
const { name } = form;
if (!name) {
errors.push("The name cannot be empty.");
}
if (Object.keys(tools.value).includes(name)) {
errors.push("An existing tool with the specified name already exists.");
}
return errors;
});

};
</script>

<style>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't it be scoped ?

Comment on lines +7 to +17
<div
v-for="(tool, toolName) in tools"
:key="toolName"
class="tool"
@click="editTool(toolName)"
>
<div class="toolName">{{ toolName }}</div>
<button class="delete" @click.stop="deleteTool(toolName)">
<i class="material-symbols-outlined">delete</i>
</button>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's clickable, it should be a button (with type="button", otherwise it sends parent form)

Suggested change
<div
v-for="(tool, toolName) in tools"
:key="toolName"
class="tool"
@click="editTool(toolName)"
>
<div class="toolName">{{ toolName }}</div>
<button class="delete" @click.stop="deleteTool(toolName)">
<i class="material-symbols-outlined">delete</i>
</button>
</div>
<button
v-for="(tool, toolName) in tools"
:key="toolName"
class="tool"
type="button"
@click="editTool(toolName)"
>
<div class="toolName">{{ toolName }}</div>
<button class="delete" @click.stop="deleteTool(toolName)">
<i class="material-symbols-outlined">delete</i>
</button>
</button>

Otherwise, you need to declare more attributes

Suggested change
<div
v-for="(tool, toolName) in tools"
:key="toolName"
class="tool"
@click="editTool(toolName)"
>
<div class="toolName">{{ toolName }}</div>
<button class="delete" @click.stop="deleteTool(toolName)">
<i class="material-symbols-outlined">delete</i>
</button>
</div>
<div
v-for="(tool, toolName) in tools"
:key="toolName"
class="tool"
tabindex="0"
@click="editTool(toolName)"
@keydown.enter="editTool(toolName)"
>
<div class="toolName">{{ toolName }}</div>
<button class="delete" @click.stop="deleteTool(toolName)">
<i class="material-symbols-outlined">delete</i>
</button>
</div>

Copy link
Collaborator Author

@ramedina86 ramedina86 Oct 29, 2024

Choose a reason for hiding this comment

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

otherwise it sends parent form

I don't understand this one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed that it needs tabindex and keydown.enter though

Copy link
Collaborator

Choose a reason for hiding this comment

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

otherwise it sends parent form

I was referring to the type attribute of the <button>. If you don't specify the type="button", it's by default <button type="submit"> (doc). This can make the button submit the parent form if there is any

<form onsubmit="alert('submited')">
  <input type="submit" value="This button triggers submit form" />
  <button type="submit">This button triggers submit form</button>
  <button>This button triggers submit form</button>

  <button type="button">This button doesn't triggers submit form</button>
</form>

Check this codepen

So it's always good to specify type="button" to avoid weird behavior

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I didn't know! Thought it was button by default

</div>

<WdsButton
variant="builder"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variant doesn't exist, no ?

type WdsButtonVariant = "primary" | "secondary" | "tertiary";

Comment on lines 20 to 27
<WdsButton
variant="builder"
size="small"
@click="resetAndShowToolFormModal"
>
<i class="material-symbols-outlined">add</i>
Add tool</WdsButton
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we create a task to have icon attribute in WdsButton ? For example, here you should have a gap between the icon and the test Figma

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does have a gap, the WdsButton is actually a flex with a gap in it, so whatever you put in the slot comes with a gap.

@ramedina86 ramedina86 merged commit dd0df2f into dev Oct 29, 2024
18 checks passed
@ramedina86 ramedina86 deleted the feat-workflows-tools-1 branch December 3, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants