refactor: launcher actions#4222
Conversation
|
You can access the deployment of this PR at https://renku-ci-ui-4222.dev.renku.ch |
andre-code
force-pushed
the
andrea/add-job-launcher
branch
from
6a8e225 to
0ea3ef9
Compare
andre-code
force-pushed
the
andrea/refactor-launcher-buttons
branch
from
641ea09 to
c3c9b80
Compare
test: add launcher action cypress coverage refactor: add launcher environment readiness hook refactor: split launcher actions by category and placement
andre-code
force-pushed
the
andrea/refactor-launcher-buttons
branch
from
c3c9b80 to
b2a0dd3
Compare
leafty
left a comment
leafty
left a comment
There was a problem hiding this comment.
General note, AI usage: please disclose AI use. See https://github.com/ghostty-org/ghostty/blob/main/AI_POLICY.md as a general starting point for guidance.
| export type ProjectPermissions = Permissions & { | ||
| isLoadingPermissions: boolean; | ||
| }; |
There was a problem hiding this comment.
This is weird. Why is it called ProjectPermissions when it is generic?
There was a problem hiding this comment.
This type is really Permissions plus a loading flag from the hook, not the API's ProjectPermissions. I'll rename it to something like PermissionsWithLoadingState and keep it close to the hook so the generic permissions.types.ts stays API-aligned.
| } | ||
| }, [fetchPermissions, isUninitialized, projectId]); | ||
|
|
||
| const isLoadingPermissions = isLoading || isUninitialized; |
There was a problem hiding this comment.
Logic error:
| const isLoadingPermissions = isLoading || isUninitialized; | |
| const isLoadingPermissions = isLoading || (projectId && isUninitialized); |
You can see above that we trigger fetching data when projectId && isUninitialized => this is when the projectId is set and the API fetch has not been triggered yet.
If you do not check for projectId (not null, not empty), then isUninitialized will be true forever.
There was a problem hiding this comment.
Misnomer, the parent component is an Offcanvas. Use JobOffcanvasSubmit.tsx.
| const displayBuildActions = | ||
| isCodeEnvironment && | ||
| write && | ||
| (resolvedUseOldImage || lastBuild?.status !== "succeeded"); | ||
|
|
||
| const submitTooltip = | ||
| resolvedUseOldImage && containerImage?.accessible !== false | ||
| ? `Launch ${categoryDefinition.text.inline} using an older image` | ||
| : undefined; |
There was a problem hiding this comment.
Move this below L76 since they are not used before.
| return <JobPanelSubmit launcher={launcher} useOldImage={useOldImage} />; | ||
| } | ||
|
|
||
| const defaultAction = (() => { |
There was a problem hiding this comment.
You should probably useMemo() here.
| Submit | ||
| </Button> | ||
| </> | ||
| <Button color="outline-primary" className={cx("disabled")} size="sm"> |
There was a problem hiding this comment.
| <Button color="outline-primary" className={cx("disabled")} size="sm"> | |
| <Button color="outline-primary" className="disabled" size="sm"> |
But also: why not set the disabled prop then?
| </Button> | ||
| </> | ||
| <Button color="outline-primary" className={cx("disabled")} size="sm"> | ||
| <Loader size={12} inline /> Checking launcher |
There was a problem hiding this comment.
| <Loader size={12} inline /> Checking launcher | |
| <Loader className="me-1" size={12} inline />Checking launcher |
| }, [launcherHash, setHash]); | ||
|
|
||
| return ( | ||
| <span id={`open-panel-btn-${launcherId}`}> |
There was a problem hiding this comment.
Bad practice: unnecessary span.
| <span id={`open-panel-btn-${launcherId}`}> |
For tooltips, use ref, innerRef, or set the ID on the component itself (avoid if possible)
| containerImage?.accessible === true || | ||
| useOldImage; | ||
|
|
||
| const showSubmitJob = !(isBuildInProgress && !hasValidImage && !useOldImage); |
There was a problem hiding this comment.
This is not readable:
| const showSubmitJob = !(isBuildInProgress && !hasValidImage && !useOldImage); | |
| const showSubmitJob = !isBuildInProgress || hasValidImage || useOldImage; |
| enabled?: boolean; | ||
| lastBuild?: Build; | ||
| lastSuccessfulBuild?: Build; | ||
| useOldImage?: boolean; |
There was a problem hiding this comment.
Q: Are there cases when we use useOldImage={false}?
There was a problem hiding this comment.
No, it is no longer needed, so I've removed it.
andre-code
force-pushed
the
andrea/refactor-launcher-buttons
branch
3 times, most recently
from
3edeb28 to
895fe21
Compare
…in SessionLauncherCard, remove unnecessary span
andre-code
force-pushed
the
andrea/refactor-launcher-buttons
branch
from
895fe21 to
6f02989
Compare
|
Hi @leafty , thanks for reviewing the PR. I’ve added the AI disclosure and simplified the logic by reusing more of the hook, with a single launcher action per type (job or session). |
3 participants
Refactors launcher buttons into shared session and job components with one readiness hook so the card and side panel use the same actions and adds cypress tests for launcher actions.
AI disclosure: Used Cursor as a coding assistant for parts of this PR (autocomplete/refactor suggestions). All code was written/reviewed and is fully understood by me; not all of it is AI-generated — Cursor was used selectively as a dev tool.
/deploy renku=feat/add-jobs amalthea-sessions=fix/job-status-when-pod-missing renku-data-services=eikek/multiple-jobs-per-launcher extra-values=dataService.imageBuilders.enabled=true,dataService.imageBuilders.strategyName=renku-buildpacks-v3,dataService.imageBuilders.outputImagePrefix=harbor.dev.renku.ch/renku-build/,dataService.imageBuilders.nodeSelector.renku.io/node-purpose=user,dataService.imageBuilders.tolerations[0].effect=NoSchedule,dataService.imageBuilders.tolerations[0].key=renku.io/dedicated,dataService.imageBuilders.tolerations[0].operator=Equal,dataService.imageBuilders.tolerations[0].value=user