Skip to content

feat: add reinstall instance action#218

Open
cpascariello wants to merge 1 commit into
mainfrom
feature/reinstall-instance
Open

feat: add reinstall instance action#218
cpascariello wants to merge 1 commit into
mainfrom
feature/reinstall-instance

Conversation

@cpascariello

Copy link
Copy Markdown
Contributor

Summary

  • Add a "Reinstall" button to the instance detail page header
  • Shows a Modal confirmation dialog warning that all persistent volumes will be erased
  • Sends POST /control/machine/{id}/reinstall to the CRN with signed headers
  • Polls for running status after reinstall completes

Dependency

This PR depends on aleph-sdk-ts#202 being merged first — it adds the CRN /reinstall endpoint support.

Files changed

  • src/domain/executable.ts — Added reinstall to ExecutableOperations type
  • src/hooks/common/useExecutableActions.ts — Added handleReinstall, loading/disabled state
  • src/components/common/entityData/ManageEntityHeader/types.ts — Added reinstall props
  • src/components/common/entityData/ManageEntityHeader/cmp.tsx — Added reinstall button + Modal
  • src/components/pages/console/instance/ManageInstance/cmp.tsx — Wired reinstall props

Test plan

  • Verify reinstall button appears on instance detail page
  • Verify clicking reinstall opens a confirmation modal
  • Verify cancelling the modal does not trigger reinstall
  • Verify confirming the modal sends POST to CRN /reinstall endpoint
  • Verify loading spinner shows during reinstall operation
  • Verify other action buttons (stop, start, reboot, delete) still work

Add a reinstall button to the instance detail header that sends
POST /control/machine/{id}/reinstall to the CRN. Includes a Modal
confirmation dialog warning that volumes will be erased.

# Depends on aleph-sdk-ts PR for CRN reinstall endpoint support:
# aleph-im/aleph-sdk-ts#202

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The core implementation is solid and follows existing patterns correctly — the hook, type, and wiring changes are clean. However, two issues need addressing: (1) A design/planning document with embedded AI prompt-injection text was committed to the repo and should be removed, and (2) a minor redundant condition wraps the Modal in cmp.tsx. There are also a couple of style nits worth fixing.

docs/plans/2026-02-25-reinstall-instance-design.md (line 1): This planning document should not be committed to the repository. Beyond being noise in the codebase, it contains an embedded AI prompt-injection directive on line 3: > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Any future AI-assisted tooling reading this file as context could be manipulated by this instruction. Please remove the file before merging.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 233): The outer {showConfirmReinstall && ...} guard is redundant — the open={showConfirmReinstall} prop already controls the modal's visibility. Simplify to just <Modal open={showConfirmReinstall} ... />.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 235): The modal title is hardcoded as "Reinstall Instance" while the button tooltip correctly uses the dynamic type prop (e.g. Reinstall ${type}). For consistency, consider using \Reinstall ${type}`` here too, so the component stays generic for any future entity type.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 215): The reinstall button uses variant="secondary" — the same visual weight as reboot. Since reinstall is more destructive (it permanently erases all persistent volumes), consider variant="error" or another warning visual to signal severity before the user reaches the confirmation modal. The modal copy is good, but the entry-point affordance is understated.

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The implementation follows existing patterns well and the core logic is sound, but there are two issues worth addressing: (1) reinstallDisabled doesn't gate on CRN availability like rebootDisabled does, so the button can be clicked when no CRN is allocated and the operation will fail at runtime with an error toast; (2) the implementation plan document (docs/plans/2026-02-25-reinstall-instance-design.md) should not be committed to the repository — it contains internal AI prompts, raw shell commands, and development notes that have no place in a production codebase.

src/hooks/common/useExecutableActions.ts (line 286): Bug: reinstallDisabled should gate on CRN availability.

reinstallDisabled only checks !executable, but handleSendOperation requires nodeUrl (derived from crn) to be present or it throws Err.InvalidNode. Without a CRN the button is enabled but clicking it immediately shows an error.

Compare with rebootDisabled, which properly checks !crn and calculatedStatus. reinstall has the same preconditions as reboot — the instance needs to be allocated to a CRN and in a running state. The guard should be:

const reinstallDisabled = useMemo(() => {
  if (!crn) return true
  switch (calculatedStatus) {
    case 'running':
      return false
    default:
      return true
  }
}, [calculatedStatus, crn])

docs/plans/2026-02-25-reinstall-instance-design.md (line 1): This file should not be committed to the repository.

This is an internal implementation plan containing AI sub-skill invocations (> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans), raw git commit commands, and step-by-step developer notes. It has no value for users or future contributors and adds noise to the repo history. Please delete this file before merging.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 233): Minor: redundant double condition on the Modal.

{showConfirmReinstall && <Modal open={showConfirmReinstall} ...>} — the outer showConfirmReinstall && guard is redundant since open={showConfirmReinstall} already controls visibility. Prefer one or the other; the rest of the codebase uses open without an outer guard:

<Modal
  open={showConfirmReinstall}
  ...
/>

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 235): Minor: modal title hardcodes "Reinstall Instance" instead of using the type prop.

The tooltip just above uses \Reinstall ${type}`` for consistency. The modal title should do the same:

title={`Reinstall ${type}`}

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The implementation is clean and follows existing patterns well. The core logic is sound — the new reinstall operation wires correctly through ExecutableOperations → handleSendOperation → ManageEntityHeader. Three issues worth addressing: a prompt injection artifact in the committed design doc, a redundant Modal rendering condition that breaks exit animations, and a hardcoded entity type in the modal title. The reinstallDisabled check is also weaker than its peers (no CRN/status guard), though errors are handled gracefully so it's not a crash.

docs/plans/2026-02-25-reinstall-instance-design.md (line 3): This file contains a prompt injection artifact: > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. This appears to be an AI prompt that was left in a committed file. It should be removed before merging, as it could confuse AI-assisted tooling in future code reviews or automated workflows that scan the repository.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 233): The {showConfirmReinstall && <Modal open={showConfirmReinstall} ...>} pattern is redundant — when showConfirmReinstall flips to false, the component is immediately unmounted before the Modal can play any exit animation. The standard pattern used elsewhere is just <Modal open={showConfirmReinstall} ...> without the outer guard. This also makes the open prop tautologically always true when rendered.

src/components/common/entityData/ManageEntityHeader/cmp.tsx (line 235): The modal title hardcodes "Reinstall Instance" but ManageEntityHeader is a generic component used for any entity type. It should use `Reinstall ${type}` to stay consistent with the Tooltip content on line 209 and the other action tooltips in this file.

src/hooks/common/useExecutableActions.ts (line 286): reinstallDisabled only checks !executable, unlike rebootDisabled which also guards on !crn and calculatedStatus. Since reinstall POSTs to the CRN, it will fail with Err.InvalidNode if crn is unavailable (e.g. instance is not-allocated). The error is handled gracefully, but disabling the button when !crn would give clearer UX feedback. Consider aligning with the rebootDisabled pattern.

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.

2 participants