- Notifications
You must be signed in to change notification settings - Fork1.1k
feat(site): use new task data model and endpoints#20431
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(site): use new task data model and endpoints#20431
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mafredri commentedOct 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
5f883dd to1e0548cCompareeea8ed5 to6d13866Compare5fc4328 to17fddfaComparee1f7b87 to0554fd1Comparebd91d75 tod530c4dCompared530c4d toc9f1efdCompare9220413 toe605686Comparec9f1efd toaecddafCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nothing major stuck out to me for the frontend changes, but I did have a few questions.
The main thing that stuck out to me was whether we can stop polling every five seconds for some of the tasks, or if that's something we absolutely need for UX
site/src/api/api.ts Outdated
| getTasks=async( | ||
| filter:TypesGen.TasksFilter, | ||
| ):Promise<readonlyTypesGen.Task[]>=>{ | ||
| constquery=[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: can we type this asstring[]?
site/src/modules/apps/apps.ts Outdated
| exporttypeWorkspaceAppWithAgent=WorkspaceApp&{ | ||
| agent:WorkspaceAgent; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: I know this is used in a few other files, but just because this is only used in one spot for this file, I'd move the type definition to be closer to where it's used, especially since it's more of a "kludge" type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Moved tomodules/tasks/apps.ts
site/src/modules/apps/apps.ts Outdated
| returnworkspace.latest_build.resources | ||
| .flatMap((r)=>r.agents??[]) | ||
| .flatMap((agent)=> | ||
| agent.apps.map((app)=>({ | ||
| ...app, | ||
| agent, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wish I knew a little bit more of Coder's backend implementations so I didn't have to ask this: are the same apps able to be shared across multiple agents? I'm wondering if we need any de-duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, a workspace_app is linked to a single workspace_agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes. We can change this function to accept an agent ID, but the agent ID can be null depending on the state. Unfortunately, sice Go does not support uninon types, we need to update the code in a few places to handle that. Since we are quite out of time for this, I'm going to skip it for now.
| constfilter:TypesGen.TasksFilter={ | ||
| owner:user.username, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should be able to extract it outside of the component so it doesn't get recreated each render
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if I go it... the filter needs to be mounted inside of the component, so we can access theuser.username 🤔
| path:`/tasks/${MockTasks[0].owner_name}/${MockTasks[0].id}`, | ||
| pathParams:{ | ||
| workspace:MockTasks[0].workspace.name, | ||
| owner_name:MockTasks[0].owner_name, | ||
| task:MockTasks[0].id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not a huge deal, but I'm wondering if we can:
- Split off the first mock task into a separate variable
- Put the variable in the array at the first position still
- Export the variable along with the array
It's not a big deal for a story, but in production, I would want to make sure that when we're dealing with empty arrays properly (and ourtsconfig file isn't tuned tightly enough to catch potential mismatches at the type level)
| refetchInterval:({ state})=>{ | ||
| returnstate.error ?false :5_000; | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want this to be constantly refetching? Because even if all tasks are settled, we'll still refetch every five seconds, as long as there is no error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, because task has statuses and states that are updated during its lifecycle.
| refetchInterval:({ state})=>{ | ||
| returnstate.error ?false :5_000; | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same question here
| queryKey:["workspace",workspace.id,"parameters"], | ||
| queryFn:()=>API.getWorkspaceParameters(workspace), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like the API setup is a little bit off here. The API function only needs the workspace's latest build, but we're not putting it in the query key, and we're also putting in the workspace's ID, which isn't used by the function at all
Wondering if it might be worth changing what the API function accepts at input to minimize its dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I do agree with you, but we are doing this in three other places in the codebase, so I'm not going to touch this right now, since it would be outside of the scope.
| if( | ||
| workspace.latest_build.job.completed_at&& | ||
| !workspace.latest_build.has_ai_task | ||
| ){ | ||
| thrownewWorkspaceDoesNotHaveAITaskError(workspace); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need equivalent functionality still, or do we have guarantees that we'll never trigger it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We've inverted the relationship, there is now a tasks table with a workspace_id column.
| current_state:{ | ||
| ...(firstTask.current_stateasTaskStateEntry), | ||
| state:"idle", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of doing the type assertion, could we pass a function tomockResolvedValue, and have it throw an error iffirstTask.current_state is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Backend changes LGTM
| TemplateDisplayNamestring`json:"template_display_name" table:"template display name"` | ||
| TemplateIconstring`json:"template_icon" table:"template icon"` | ||
| WorkspaceID uuid.NullUUID`json:"workspace_id" format:"uuid" table:"workspace id"` | ||
| WorkspaceNamestring`json:"workspace_name" table:"workspace name"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Might be good to add some asserts for this incoderd/aitasks_test.go e.g. inTestTasks/{Get,List}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BE changes look good 👍🏻
| FilterQuerystring`json:"filter_query,omitempty"` | ||
| } | ||
| // Experimental response shape for tasks list (server returns []Task). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| //Experimentalresponse shape for tasks list (server returns []Task). | |
| //TaskListResponse is theresponse shape for tasks list. | |
| // | |
| // Experimental: This type is experimental and may change in the future. |
…ite-use-new-tasks-data-model-and-endpoints
51d3abb intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Updates the UI to use the new API endpoints for tasks and use its new data model.
Disclaimer: Since the base data model for tasks changed, we had to do a quite large refactor and I'm sorry for that 🙏, but you'll notice most of the changes are to adjust the types.
Closescoder/internal#976