Incorrect custom command configuration is handled poorly

[brandondong]

Describe the bug
I was trying to set up a custom command for the first time.

customCommands:
  - key: '<c-m>'
    context: 'localBranches'
    command: 'git merge master'

Putting that in my config and launching lazygit on Windows causes it to just immediately terminate with no error message and a strange artifact at the edge of the window.

Only later did I learn that ctrl-m was not supported: docs/keybindings/Custom_Keybindings.md

lazygit/pkg/gui/keybindings/keybindings.go

Line 110 in 0fd7b9b

log.Fatalf("Unrecognized key %s for keybinding. For permitted values see %s", strings.ToLower(key), constants.Links.Docs.CustomKeybindings)

log.Fatal while the TUI has already started appears to cause this issue. I tried different shells and terminals on Windows to the same result. On WSL, it's better but multiline log.Fatal's still do not render quite correctly

(This log.Fatal happens when providing an invalid custom command context)

I think the ideal behavior would be for the custom command key/context validation to happen in

lazygit/pkg/config/app_config.go

Line 207 in 853a04d

if err := base.Validate(); err != nil {

Custom command misconfigurations would then behave consistently with how other user config errors are (gracefully) handled:

1. If a config misconfiguration happens on startup, then lazygit cleanly prints the error and exits before starting the TUI.
2. If a config misconfiguration happens during user config reload, an error popup will be shown and the old valid config will still be used instead of immediately crashing.
3. If a config misconfiguration happens during repository switch, an error popup will be shown and the switch will be prevented instead of immediately crashing.

Actually making that fix is a bit of a refactor though...

Also, I noticed there are 3 disagreeing sources for what is a valid context value.

1. The docs: https://github.com/jesseduffield/lazygit/blob/master/docs/Custom_Command_Keybindings.md#contexts
2. What is actually allowed:

   lazygit/pkg/gui/services/custom_commands/keybinding_creator.go

   Line 65 in 853a04d

   ctx, ok := self.contextForContextKey(types.ContextKey(context))

   (basically any context key, for example context: 'statusSpacer1' is accepted...)
3. The context keys listed in the log fatal error message:

   lazygit/pkg/gui/services/custom_commands/keybinding_creator.go

   Line 87 in 853a04d

   allContextKeyStrings := lo.Map(context.AllContextKeys, func(key types.ContextKey, _ int) string {

   Almost the same as 2 but missing some of the more recently added context keys.

To Reproduce
Steps to reproduce the behavior:

Add

customCommands:
  - key: '<c-m>'
    context: 'localBranches'
    command: 'git merge master'

or

customCommands:
  - key: '<c-n>'
    context: 'localBranches2'
    command: 'git merge master'

to your config and then launch lazygit

Expected behavior
See above

Screenshots
If applicable, add screenshots to help explain your problem.

Version info:
master branch

Additional context
Add any other context about the problem here.

Note: please try updating to the latest version or manually building the latest master to see if the issue still occurs.

[stefanhaller]

Thanks for the detailed report. I wonder if we should break it down a bit to make it easier to address:

1. validating keybindings. I agree it would be good to do this in Validate(). This could be a first PR. The "bit of a refactor" that you mentioned is just to make the valid key names known from inside pkg/config, is that right? I don't have a great idea yet what the cleanest way to do that is.
2. sanitize validation of context names. Currently we allow any valid context key (that statusSpacer1 example is really silly), it would be nice to be able to tell which contexts are focusable (basically the first section of the first const statement in pkg/gui/context/context.go.
3. I'm not sure it's worth writing a generator that auto-generates the table in the docs from the actual contexts in the code. BTW, another source of valid context names is in the json schema, which comes from here. So those two would have to be manually kept up to date with 2. (I haven't checked if they are right now).
4. Validating context keys in Validate(). This is tricky because the config package doesn't have access to the gui stuff (contexts), and again I'm not sure what's the best way to solve this.

[brandondong]

Yeah, I was originally planning to write a fix myself after running into this issue but realized I'm probably not the right person to be mucking about with lazygit's code organization :D. So I filed this issue instead with the details. The breakdown of steps sounds like a good idea.

[stefanhaller]

Ah too bad. I was hoping you would come up with some bright ideas for those. 😄 Never mind, I'll see what I can do, but it doesn't have very high priority for me right now.

[ChrisMcD1]

The log.Fatal resulting in poor output seems to be a universal problem anytime after the TUI launches. Is there a known alternative that outputs the expected error text? I figured that could be a small bandaid to the problem.

[stefanhaller]

The log.Fatal resulting in poor output seems to be a universal problem anytime after the TUI launches. Is there a known alternative that outputs the expected error text? I figured that could be a small bandaid to the problem.

We have the utils.Safe function that is supposed to solve this, but maybe it's not used often enough or something.

[stefanhaller]

Here's a PR that validates the key names (but not the context names yet): #4275.

Testing and code reviews very welcome.