-
Notifications
You must be signed in to change notification settings - Fork 76
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(hub): add dedicated name column for actors & builds #1995
feat(hub): add dedicated name column for actors & builds #1995
Conversation
Deploying rivet-hub with
|
Latest commit: |
63eb01a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://18caa143.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://02-03-feat-hub-add-dedicated.rivet-hub-7jb.pages.dev |
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.
PR Summary
This PR adds dedicated name columns for actors and builds, separating the name tag from other tags for better visibility and organization in the UI.
- Added new grid column layout in
frontend/apps/hub/src/domains/project/components/actors/actors-list.tsx
expanding from 6 to 7 columns - Introduced BUILT_IN_TAGS structure in
actor-tags.tsx
to properly categorize and filter tags for actors vs builds - Enhanced
usePatchActorBuildTagsMutation
inmutations.ts
to handle 'current' tag transitions between builds - Added name tag extraction logic in
actors-list-row.tsx
with fallback display for missing names - Updated builds table in
builds.tsx
to properly exclude built-in tags using the new category system
5 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -71,13 +73,17 @@ export const ActorsListRow = memo( | |||
/> | |||
</SmallText> | |||
<SmallText>{id.split("-")[0]}</SmallText> | |||
<SmallText> | |||
{(tags as Record<string, string>).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.
logic: unsafe type assertion - should validate tags is an object before casting to Record<string, string>
@@ -12,6 +12,8 @@ import { ActorRegion } from "./actor-region"; | |||
import { ActorStatusIndicator } from "./actor-status-indicator"; | |||
import { ActorTags } from "./actor-tags"; | |||
|
|||
const BUILT_IN_TAGS = ["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.
style: BUILT_IN_TAGS is defined but never used in this file - consider removing or using it to filter tags
tags?: unknown; | ||
excludeBuiltIn?: boolean; | ||
excludeBuiltIn?: keyof typeof BUILT_IN_TAGS; |
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.
style: tags is typed as unknown but used as object in implementation. Consider using Record<string, string> or a more specific type
tags?: unknown; | |
excludeBuiltIn?: boolean; | |
excludeBuiltIn?: keyof typeof BUILT_IN_TAGS; | |
tags?: Record<string, string>; | |
excludeBuiltIn?: keyof typeof BUILT_IN_TAGS; |
} & Rivet.servers.PatchBuildTagsRequest) => | ||
rivetClient.actor.builds.patchTags(buildId, { | ||
} & Rivet.servers.PatchBuildTagsRequest) => { | ||
console.log("mutating", request.tags); |
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.
style: Remove debug console.log before production deployment
current: "true", | ||
}), | ||
}); | ||
console.log("updating builds", currentBuilds.builds); |
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.
style: Remove debug console.log before production deployment
for (const build of currentBuilds.builds) { | ||
await rivetClient.actor.builds.patchTags(build.id, { | ||
project: projectNameId, | ||
environment: environmentNameId, | ||
body: { | ||
tags: { | ||
current: 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.
style: This loop makes multiple sequential API calls. Consider using Promise.all for better performance
for (const build of currentBuilds.builds) { | |
await rivetClient.actor.builds.patchTags(build.id, { | |
project: projectNameId, | |
environment: environmentNameId, | |
body: { | |
tags: { | |
current: null, | |
}, | |
}, | |
}); | |
} | |
await Promise.all(currentBuilds.builds.map(build => | |
rivetClient.actor.builds.patchTags(build.id, { | |
project: projectNameId, | |
environment: environmentNameId, | |
body: { | |
tags: { | |
current: null, | |
}, | |
}, | |
}) | |
)); |
// If setting build to current, remove current tag from all other builds with the same name | ||
if ( | ||
ogBuild.build.tags.name && | ||
(request.tags as Record<string, string> | undefined) | ||
?.current === "true" | ||
) { |
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.
logic: Type assertion on request.tags could fail silently. Consider adding runtime type validation
await mutateBuildTagsAsync({ | ||
projectNameId, | ||
environmentNameId, | ||
buildId: id, | ||
tags: { current: "true" }, | ||
exclusiveTags: ["current"], | ||
}); |
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.
logic: removing exclusiveTags parameter could allow multiple builds to have current=true tag simultaneously, potentially causing inconsistent behavior
if (latestBuilds.length > 0) { | ||
mutateUpgradeActors({ | ||
projectNameId, | ||
environmentNameId, | ||
buildTags: { current: "true" }, | ||
tags: { name: latestBulds[0].name }, | ||
tags: { name: latestBuilds[0].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.
style: no error handling for failed mutations - could leave system in inconsistent state if first mutation succeeds but second fails
Deploying rivet with
|
Latest commit: |
63eb01a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1ce11a9c.rivet.pages.dev |
Branch Preview URL: | https://02-03-feat-hub-add-dedicated.rivet.pages.dev |
a0ed162
to
2c5f0f9
Compare
b9b9ec4
to
2c5f0f9
Compare
bced972
to
f86b7c7
Compare
322d2c8
to
537f3d9
Compare
f86b7c7
to
02479da
Compare
537f3d9
to
63eb01a
Compare
Merge activity
|
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
Changes