feat: add Close method to UI interface and implement resource cleanup in REPL#452
feat: add Close method to UI interface and implement resource cleanup in REPL#452prasad89 wants to merge 1 commit intoGoogleCloudPlatform:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a Close method to the UI interface to enable proper resource cleanup when the REPL terminates. The implementation ensures that UI resources are properly released when the application exits.
- Added
Close() errormethod to the UI interface for resource cleanup - Implemented the
Closemethod in the TUI struct to quit the underlying program - Added cleanup call in the REPL function to ensure resources are freed on exit
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ui/interfaces.go | Added Close method to UI interface with documentation |
| pkg/ui/tui.go | Implemented Close method to quit the program if it exists |
| cmd/main.go | Added UI cleanup call in REPL function with error logging |
|
|
||
| func (u *TUI) Close() error { | ||
| if u.program != nil { | ||
| u.program.Quit() |
There was a problem hiding this comment.
The Quit() method may not immediately stop the program and could leave resources in an inconsistent state. Consider using Kill() instead for more forceful termination, or check if the program is still running after Quit() and handle accordingly.
There was a problem hiding this comment.
Hey, I cloned this pr to manually test and I wasn't able to get it to work in a way that would close #439. To close 439, I would expect exit or quit to take me out of the terminal. I also tried with --ui-type tui since I see you have changes in tui, and still had the same non-closing behavior. I'll attach demos to this comment.
I made #453 before I saw this pr, though it closes the terminal on exit or quit, it does not clean up any resources like your PR does. Mike brought up a good point on that PR about adding a regression test since we've run into this problem more than once.
I think the changes in this PR are still good and help us follow best practices, I just didn't see the behavior I expected. If you could possibly demo the behavior that #439 expects, then feel free to resolve/ignore this.
terminal: https://github.com/user-attachments/assets/2a90f00a-ae50-4292-a453-7ad207950a86
tui: https://github.com/user-attachments/assets/820143c7-791a-4cc7-a69b-f651a190d224
| func (u *TUI) ClearScreen() { | ||
| } | ||
|
|
||
| func (u *TUI) Close() error { |
There was a problem hiding this comment.
(this may be out of scope of this pr, feel free to resolve or ignore if it is): Do we need a function like this to clean up web ui resources too? If we do, but it's out of scope for this pr, we can make an issue requesting it in a later pr.
2 participants
In this PR I have addressed #439 (comment) by adding Close method to UI interface and implement resource cleanup in REPL
Closes #439