fix(lspcmd): surface rename failures to the user - #10
Conversation
Rename previously only logged gopls errors and closed the floating input, so colliding names looked like a no-op. Notify LevelError on rename/apply failures and on empty edits so collisions are visible. Fixes unstablebuild#2 Signed-off-by: Daniel <danielbae@ucla.edu>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
| } | ||
|
|
||
| func (r *renameFloatingHandler) notifyRenameError(err error) { | ||
| if r.notify == nil || err == nil { |
There was a problem hiding this comment.
We should strive to keep control flow explicit in the orchestrating function. Furthermore, r.notify should be guaranteed to not be nil at construction. What's the meaning of r.notify being nil? What's the production scenario that would pass a nil notification facility? It doesn't exist. If it's nil, we should let it panic: it's a programmer error. I would even move it at renameFloatingHandler so we enforce this invariant and let it crash the first time one of us constructs renameFloatingHandler with a nil notification facility.
If we agree that r.notify can't be nil, then the err != nil is redundant, at which point you can remove this helper altogether, and just inline, in notifyRenameError's call sites:
if edit == nil || renameEditEmpty(edit) {
_, _ = r.notify.Notify(browserapi.LevelError, "rename: no edits returned")
return true, true
}
...
if err != nil {
r.log.Warn("rename apply", "err", err)
_, _ = r.notify.Notify(browserapi.LevelError, "rename: %s", err)
return true, true
}| // gopls can return a non-nil empty edit for some failure modes; treat that | ||
| // the same as a failed rename so the user sees feedback. | ||
| func renameEditEmpty(edit *semanticapi.WorkspaceEdit) bool { | ||
| if edit == nil { |
There was a problem hiding this comment.
This is currently redundant as it stands, because both call sites already checked that edit is indeed `nil.
| } | ||
|
|
||
| h := newTestRenameFloating(t, "oldName", lsp, &mockEditor{}) | ||
| h := newTestRenameFloating(t, "oldName", lsp, &mockEditor{}, nil) |
There was a problem hiding this comment.
As discussed, don't pass nil here. Just pass a nop implementation of browserapi.Notifications. There's plenty in the codebase, if we were organized (which we ought to be!) we should have a browsertest.NopNotifications() implementation, and use in tests that we don't care about notifications.
Care to take a stab at this while we're at it?
~/src/rune main*
❯ rg 'type nopNotifications struct\{\}'
internal/llm/llamaserver/e2e_test.go
258:type nopNotifications struct{}
internal/ide/idepkg/provisioning.go
99:type nopNotifications struct{}
internal/text/textrpc/client_server_test.go
1431:type nopNotifications struct{}
internal/ide/notifications.go
175:type nopNotifications struct{}
|
Thanks for taking this one! |
2 participants
Summary
Fixes #2: LSP rename collisions (and other gopls rename failures) were only logged, so the floating rename input closed with no user-visible feedback.
This wires
browserapi.Notificationsinto the rename floating handler and notifiesLevelErrorwhen:Matches the existing error-notification pattern used by definition/hover/navigate handlers.
Test plan
go test ./internal/ide/idelsp/lspcmd/ -run 'TestRenameFloating|TestRenameHandler|TestApplyWorkspace|TestApplyEdits'