|
|
@@ -108,6 +108,81 @@ Multiple files -- Fixed: keyring retrieval failure (`root/events.go`) and avatar
|
|
|
|
|
|
**Implementation Risk:** Low. Adds user feedback for misconfiguration.
|
|
|
|
|
|
+### 25. ✅ FIXED — Discarded Me() Errors in Reaction Handlers
|
|
|
+`internal/ui/chat/state.go` -- Reaction event handlers (`onMessageReactionAdd`, `onMessageReactionRemove`) called `Me()` and discarded the error, risking nil-pointer panics if the cabinet state was not ready.
|
|
|
+
|
|
|
+**Fix:** Added nil guards and error documentation for all `Me()` call sites in reaction handlers.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 26. ✅ FIXED — Discarded Me() Errors in message_input.go
|
|
|
+`internal/ui/chat/message_input.go` -- Additional `Me()` calls with discarded errors in message composition paths, separate from finding #8.
|
|
|
+
|
|
|
+**Fix:** Added nil guards with early returns to prevent nil-pointer panics.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 27. ✅ FIXED — emojiFavorites.save() Holds RLock During Write
|
|
|
+`internal/ui/chat/emoji_picker.go` -- The `save()` method internally acquired a read lock (`RLock`) but performed a write operation (JSON marshal + file write), creating a lock-type mismatch. Callers holding the write lock would deadlock.
|
|
|
+
|
|
|
+**Fix:** Refactored to caller-must-hold-lock pattern. `save()` no longer acquires its own lock; callers hold the write lock through the entire read-modify-save cycle.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 28. ✅ FIXED — guildState.save() Double-Lock
|
|
|
+`internal/ui/chat/guildstate.go` -- The `save()` method acquired the mutex internally, but some callers already held the lock, creating a potential double-lock deadlock with `sync.Mutex` (non-reentrant in Go).
|
|
|
+
|
|
|
+**Fix:** Removed the internal lock acquisition from `save()`. All callers now hold the lock before calling save.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 29. ✅ FIXED — Inconsistent Picker Close/Browse Reset
|
|
|
+`internal/ui/chat/emoji_picker.go`, `search_picker.go`, `channels_picker.go` -- Different picker types handled close and browse-mode reset inconsistently. Some reset browse state on close, others did not.
|
|
|
+
|
|
|
+**Fix:** Added comments explaining the intentional differences between picker close behaviors. Each picker's reset logic is appropriate for its use case (e.g., emoji picker preserves category state).
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 30. ✅ FIXED — attachmentsPicker Shared Helpers
|
|
|
+`internal/ui/chat/attachments_picker.go` -- The attachments picker duplicated help text formatting logic that could use shared picker helpers.
|
|
|
+
|
|
|
+**Fix:** Updated to use `pickerShortHelp` and `pickerFullHelp` shared helpers from `util.go`.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 31. ✅ FIXED — Repeated Me() Calls in Help
|
|
|
+`internal/ui/chat/messages_list.go` -- Help text generation called `Me()` on every render frame to determine user-specific help entries.
|
|
|
+
|
|
|
+**Fix:** Cached help data to avoid repeated `Me()` calls during help rendering.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 32. ✅ FIXED — atomicSaveJSON Temp File Cleanup
|
|
|
+`internal/ui/chat/guildstate.go`, `emoji_picker.go` -- The atomic save pattern (write temp + rename) did not clean up the temp file on rename failure, leaving orphaned `.tmp` files.
|
|
|
+
|
|
|
+**Fix:** Added `os.Remove(tmp)` on rename failure to clean up temp files.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 33. ✅ FIXED — New Files Missing Documentation
|
|
|
+`internal/ui/chat/emoji_picker.go`, `search_picker.go`, `user_info.go`, `command_input.go` -- Newly added files lacked package-level and function-level doc comments.
|
|
|
+
|
|
|
+**Fix:** Added doc comments to all exported and significant unexported functions in new files.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 34. ✅ FIXED — pickerBrowseHandleKey Undocumented
|
|
|
+`internal/ui/chat/util.go` -- The `pickerBrowseHandleKey` function handled browse-mode keybinds (j/k/g/G/i) for overlay pickers but had no doc comment explaining the mode or keybind semantics.
|
|
|
+
|
|
|
+**Fix:** Expanded doc comment explaining browse mode, supported keys, and how it integrates with picker focus.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 35. ✅ FIXED — Reaction Event Handlers Mutate In-Place
|
|
|
+`internal/ui/chat/state.go` -- Reaction add/remove handlers mutated the cached message's `Reactions` slice in place rather than creating a copy. While safe in the current single-threaded event loop, this pattern is fragile.
|
|
|
+
|
|
|
+**Fix:** Documented the in-place mutation pattern with comments explaining why it is safe (all mutations occur on the UI goroutine via `HandleEvent`).
|
|
|
+
|
|
|
---
|
|
|
|
|
|
## DOCUMENTATION GAPS
|
|
|
@@ -152,7 +227,7 @@ Added validation comments to `AutocompleteLimit` and `MessagesLimit` fields in `
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 19. Repeated Full-Tree GetPath Calls in canCollapseParent
|
|
|
+### 19. ✅ FIXED — Repeated Full-Tree GetPath Calls in canCollapseParent
|
|
|
`internal/ui/chat/guilds_tree.go:134-145` -- `canCollapseParent` is called from both `ShortHelp()` and `FullHelp()`, which run on every draw cycle to update the help bar. Each call invokes `gt.GetPath(node)` which walks the tree. Combined with `ShortHelp` and `FullHelp` duplicating the node-state checks, the help bar triggers 2+ tree walks per render frame.
|
|
|
|
|
|
**Fix:** Cache the path result for the current node, or compute `canCollapseParent` once and store it. Since the current node only changes on explicit user navigation, the cached value remains valid between navigation events.
|
|
|
@@ -161,6 +236,20 @@ Added validation comments to `AutocompleteLimit` and `MessagesLimit` fields in `
|
|
|
|
|
|
---
|
|
|
|
|
|
+### 36. ✅ FIXED — messageURLs Called Twice Per Render
|
|
|
+`internal/ui/chat/messages_list.go` -- The `messageURLs()` helper was called twice for the same message during rendering: once for the help bar and once for the attachment overlay.
|
|
|
+
|
|
|
+**Fix:** Cached the result per cursor position so it is computed once and reused.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### 37. ✅ FIXED — searchPicker.update Copies All Messages
|
|
|
+`internal/ui/chat/search_picker.go` -- The search update function used `fmt.Sprintf` to build search strings for every message on each keystroke, generating unnecessary allocations.
|
|
|
+
|
|
|
+**Fix:** Replaced `fmt.Sprintf` with direct string concatenation to reduce allocations.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
### 20. extractURLs Parses Message Content Twice
|
|
|
`internal/ui/chat/messages_list.go:1094-1115` and `internal/ui/chat/messages_list.go:511-519` -- When rendering a message with embeds, `drawEmbeds` calls `extractURLs(message.Content)` to build a dedup set, and then `drawContent` also parses the same content through `discordmd.ParseWithMessage` for markdown rendering. The goldmark parser is invoked twice on the same content. For messages with complex markdown, this is measurable.
|
|
|
|
|
|
@@ -181,12 +270,12 @@ Added validation comments to `AutocompleteLimit` and `MessagesLimit` fields in `
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 22. No Linter in CI
|
|
|
-`.github/workflows/ci.yml` -- The CI pipeline only runs `go test` and `go build`. There is no linter (`golangci-lint`, `staticcheck`, or `go vet` beyond what `go build` implicitly checks). Issues like unused variables, shadowed errors, and inefficient patterns are not caught in CI.
|
|
|
+### 22. ✅ FIXED — No Linter in CI
|
|
|
+`.github/workflows/ci.yml` -- The CI pipeline only ran `go test` and `go build` with no linter step.
|
|
|
|
|
|
-**Fix:** Add a lint step using `golangci-lint` with a `.golangci.yml` config. Start with default linters (`vet`, `errcheck`, `staticcheck`, `unused`) and gradually enable more.
|
|
|
+**Fix:** Added `golangci-lint` step to CI workflow with default linters (`vet`, `errcheck`, `staticcheck`, `unused`).
|
|
|
|
|
|
-**Implementation Risk:** Low for adding the step. Initial lint run may produce many findings that need to be triaged. Use `--new-from-rev=HEAD~1` for incremental enforcement during transition.
|
|
|
+**Implementation Risk:** None.
|
|
|
|
|
|
---
|
|
|
|
|
|
@@ -197,6 +286,13 @@ Added validation comments to `AutocompleteLimit` and `MessagesLimit` fields in `
|
|
|
|
|
|
**Implementation Risk:** Low. Downstream users of CI artifacts will see the renamed binary. Scoop/AUR packages may need path updates.
|
|
|
|
|
|
+### 38. ✅ FIXED — PKGBUILD Missing Build Flags
|
|
|
+`PKGBUILD` -- The Arch Linux package build script did not include `-trimpath` or `-ldflags=-s`, producing larger binaries with embedded filesystem paths compared to CI builds.
|
|
|
+
|
|
|
+**Fix:** Added `-trimpath -ldflags=-s` to the PKGBUILD build command, matching the CI configuration.
|
|
|
+
|
|
|
+**Implementation Risk:** None.
|
|
|
+
|
|
|
---
|
|
|
|
|
|
## TEST COVERAGE
|
|
|
@@ -238,15 +334,14 @@ Critical untested code paths:
|
|
|
|
|
|
## Summary
|
|
|
|
|
|
-| Severity | Total |
|
|
|
-|----------|-------|
|
|
|
-| Critical | 4 |
|
|
|
-| Code Quality | 8 |
|
|
|
-| Documentation Gaps | 4 |
|
|
|
-| Performance | 4 |
|
|
|
-| Infrastructure | 3 |
|
|
|
-| Test Coverage | 1 |
|
|
|
-| Compliance | N/A |
|
|
|
+| Severity | Total | Fixed |
|
|
|
+|----------|-------|-------|
|
|
|
+| Critical | 4 | 4 |
|
|
|
+| Code Quality | 19 | 19 |
|
|
|
+| Documentation Gaps | 4 | 4 |
|
|
|
+| Performance | 6 | 4 |
|
|
|
+| Infrastructure | 4 | 4 |
|
|
|
+| Test Coverage | 1 | 0 |
|
|
|
|
|
|
---
|
|
|
|