Conversation
Current Aviator status
This PR was merged using Aviator.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
✅ FlexReview StatusCommon Owner:
Review SLO: |
Summary of ChangesHello @draftcode, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by allowing pre-av-sync hooks to be interactive. The use of bubbletea.ExecProcess to give the hook TTY control is a solid approach. The implementation is well-structured, with a new preAvSyncHookModel to manage the hook's lifecycle, and the code includes excellent comments explaining the design choices. The refactoring in internal/git/git.go to extract a Cmd function is a good enhancement for code reuse. I have one high-severity suggestion regarding context propagation to prevent orphaned processes, which is particularly important for interactive commands like this.
| return uiutils.ErrCmd(err) | ||
| } | ||
| m.hasHook = true | ||
| cmd := m.repo.Cmd(context.Background(), []string{"hook", "run", "--ignore-missing", "pre-av-sync"}, nil) |
There was a problem hiding this comment.
Using context.Background() here can lead to the interactive hook process being orphaned if the user cancels the av sync command (e.g., with Ctrl+C). This could leave the user's terminal in an inconsistent state.
To fix this, you should propagate the context from the cobra command all the way down to here. This will ensure that if the main process is cancelled, the hook process is also terminated.
Here are the suggested steps:
- Add a
ctx context.Contextfield to thesyncViewModelstruct. - In the
syncCmd.RunEfunction, passcmd.Context()when creating thesyncViewModel. - Add a
ctx context.Contextfield topreAvSyncHookModeland accept it as an argument innewPreAvSyncHookModel. - In
initSync, passvm.ctxwhen callingnewPreAvSyncHookModel. - Finally, use
m.ctxhere instead ofcontext.Background().
This change would also allow you to replace other instances of context.Background() in syncViewModel methods, improving the robustness of the command.
|
This PR is stacked on top of #600 which was merged. This PR (and its dependents) need to be synchronized on the commit that was merged into # Switch to this branch
git checkout git_exec_interactive
# Sync this branch (and its children) on top of the merge commit
av sync |
Previously the hook was executed while av having a control on tty. This means that the hook cannot be interactive. This commit changes the hook execution to be done with bubbletea's ExecProcess, which allows the hook to take over the underlying tty. This allows users to create interactive hook, which was not possible before. There was a try-and-error on the visual of the hook execution. Details are written in the comment of preAvSyncHookModel.View(). This will not change anything if there is no pre-av-sync hook. If there is a hook, it will show a pre-av-sync hook status shown in the sync view.
1baff6e to
d4d8d9d
Compare
Previously the hook was executed while av having a control on tty. This
means that the hook cannot be interactive. This commit changes the hook
execution to be done with bubbletea's ExecProcess, which allows the
hook to take over the underlying tty. This allows users to create
interactive hook, which was not possible before.
There was a try-and-error on the visual of the hook execution. Details
are written in the comment of preAvSyncHookModel.View(). This will not
change anything if there is no pre-av-sync hook. If there is a hook, it
will show a pre-av-sync hook status shown in the sync view.