fix: don't show suspended apps in list for terminate and logs commands

[solimander]

No description provided.

[claude]

Claude finished @solimander's task —— View job

---

Pull Request Review

Reviewing PR: "fix: don't show suspended apps in list for terminate and logs commands"

Todo List:

• Read the modified file to understand the exact change
• Analyze the change context and impact
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage needs
• Provide comprehensive feedback

Code Change Analysis

The PR makes a single-line change in pkg/commands/utils/interactive.go:301:

- return status != common.ContractAppStatusTerminated
+ return status != common.ContractAppStatusTerminated && status != common.ContractAppStatusSuspended

This affects the isEligible function's default case, which determines which apps are shown in the interactive selection list for commands other than "view", "start", and "stop".

Review Feedback

✅ Code Quality & Best Practices

• Good: The change follows existing patterns in the codebase (same logical structure as the condition for terminated apps)
• Good: Consistent with other status checks throughout the file
• Good: The condition is clear and readable

✅ Correctness & Logic

• Excellent: The change correctly implements the intended behavior described in the PR title
• Good: Based on the existing code logic, suspended apps should indeed be excluded from terminate and logs commands:
  • Logs: Line 206 in info.go already treats suspended apps as having no available logs
  • Terminate: Suspended apps are likely in a state where termination is not meaningful/applicable
• Good: Other commands affected by this change (like "upgrade") would also benefit from excluding suspended apps

✅ Impact Analysis

Commands affected by this change (using the default case):

• terminate (lifecycle.go:125)
• logs (info.go:161)
• upgrade (upgrade.go:42)
• Any other future commands that don't have explicit eligibility logic

This aligns well with the business logic since suspended apps typically cannot be operated on.

⚠️ Potential Issues

1. Missing Test Coverage: There are no tests covering the GetAppIDInteractive function or the isEligible logic. This makes it harder to verify the change works correctly and prevents regression.

2. Inconsistent PR Description: The PR title mentions only "terminate and logs commands" but the change affects all commands using the default case (including upgrade). Consider updating the title or description for accuracy.

✅ Performance Considerations

• Good: No performance impact - this is a simple boolean condition check
• Good: The change doesn't add complexity to the existing logic

✅ Security Concerns

• Good: No security implications identified
• Good: The change restricts functionality appropriately (doesn't expose new attack vectors)

Recommendations

1. Consider adding tests: While not blocking, adding unit tests for the isEligible function would improve maintainability:

   func TestIsEligible(t *testing.T) {
       // Test cases for different actions and statuses
   }

2. Update PR description: Consider clarifying that this affects all non-view/start/stop commands, not just terminate and logs.

3. Documentation: Consider adding a comment to the isEligible function explaining the business logic for each action type.

Verdict

✅ LGTM - This is a solid, focused fix that correctly implements the intended behavior. The change is minimal, safe, and follows existing patterns in the codebase. The logic makes business sense given how suspended apps are handled elsewhere in the application.

---