# Compliance Review -- discordo-plus

Reviewed: 2026-03-20 | Reviewer: Claude (automated)
Scope: all source files | Standard: General (Code Quality, Performance, Go Best Practices)
Reference: `research/TECHFILE.md` for dependency and version context.

---

## CRITICAL

### 1. ✅ FIXED — Cache.Get Panics on Missing Key
`internal/cache/cache.go:27` -- The `Get` method performs an unchecked type assertion `i.(uint)` on the result of `sync.Map.Load`. If the key does not exist, `i` is `nil` and the assertion panics. While `Exists` is called before `Get` in the current codebase (`message_input.go:499-500`), there is no guarantee callers will always check first, and the race window between `Exists` and `Get` on a `sync.Map` means concurrent `Invalidate` could delete the key between the two calls, causing a runtime panic.

**Fix:** Change `Get` to return `(uint, bool)` or use a comma-ok assertion: `if i, ok := c.items.Load(query); ok { return i.(uint) }; return 0`. Update callers to handle the zero/missing case.

**Implementation Risk:** Low. Only `message_input.go` calls `Get`, and the change is a straightforward API update. The fix also closes a potential TOCTOU race.

---

### 2. ✅ FIXED — HTTP Response Body Leak in Brotli Transport
`internal/http/transport.go:28-32` -- When the response has `Content-Encoding: br`, the original `resp.Body` is wrapped with `brotli.NewReader` via `io.NopCloser`. This means calling `Close()` on the new body is a no-op and the original body's connection is never returned to the pool. For long-running applications like a Discord client, this can exhaust available connections over time.

**Fix:** Replace `io.NopCloser(brotli.NewReader(resp.Body))` with a custom `ReadCloser` that wraps the brotli reader but delegates `Close()` to the original `resp.Body`. Example: `type brotliReadCloser struct { io.Reader; closer io.Closer }` with `func (b *brotliReadCloser) Close() error { return b.closer.Close() }`.

**Implementation Risk:** Low. The fix is a small wrapper struct. Verify with a test that the underlying body is closed when the brotli-wrapped response body is closed.

---

### 3. ✅ FIXED — Notification Avatar Download Uses Bare http.Get Without Timeout
`internal/notifications/notifications.go:93` -- `http.Get(url)` uses the default HTTP client which has no timeout. A slow or unresponsive Discord CDN server will block the goroutine indefinitely. Since notifications are triggered from gateway events, this could accumulate blocked goroutines during network issues.

**Fix:** Use `http.Client{Timeout: 10 * time.Second}` or create a request with `context.WithTimeout` and pass it to `http.DefaultClient.Do(req)`. Apply the same fix to `downloadToCache` in `messages_list.go:1191` which also uses bare `http.Get`.

**Implementation Risk:** Low. Adding a timeout may cause some avatar downloads to fail under poor network conditions, but this is preferable to indefinite blocking. Existing error handling already covers the failure path.

---

### 4. ✅ FIXED — Log File Opened With os.ModePerm (0777)
`internal/logger/logger.go:24` -- The log file is opened with `os.ModePerm` (0777), meaning it is world-readable and world-writable. Log files may contain sensitive debugging information (user IDs, channel names, error messages). Similarly, `internal/consts/consts.go:25` creates the cache directory with `os.ModePerm`.

**Fix:** Use `0600` for the log file (owner read/write only) and `0700` for the cache directory (owner only). Apply the same change to `guildstate.go:45` which writes state with `0644` (already better but could be `0600` for consistency).

**Implementation Risk:** Low. Users running in shared environments benefit from restrictive permissions. Existing users may need to manually fix permissions on existing files if they switch.

---

## CODE QUALITY

### 5. messages_list.go Is Over 1600 Lines -- God File
`internal/ui/chat/messages_list.go` -- This file contains message rendering, embed processing, URL extraction, attachment downloading, file I/O, external process management (image viewer, xdotool), keybind handling, selection logic, and help text generation. At 1600+ lines, it violates single-responsibility and is the hardest file for a new developer to navigate.

**Fix:** Extract into focused files: (a) `embed_renderer.go` for `embedLines`, `embedLineStyles`, `wrapStyledLine`, `lineWithURL`, `linkDisplayText`, `unescapeMarkdownEscapes`; (b) `attachment_handler.go` for `downloadToCache`, `openAttachment`, `saveAttachmentImage`, `openURL`, `viewerArgs`, `terminalGeometry`, `supportedImageTypes`; (c) `url_extractor.go` for `extractURLs`, `extractEmbedURLs`, `messageURLs`. Each extraction reduces this file by ~200-300 lines.

**Implementation Risk:** Medium. All extracted functions are file-private (`lowercase`), so no external API changes. However, the functions access `messagesList` fields through receiver methods, so some will need to become methods on `messagesList` or take explicit parameters. Test with `go build ./...` after each extraction.

---

### 6. ✅ FIXED — Duplicated ShortHelp/FullHelp Logic in guilds_tree.go
`internal/ui/chat/guilds_tree.go:67-132` -- The `ShortHelp()` and `FullHelp()` methods contain nearly identical logic for determining the `selectDesc` string based on node state (lines 69-86 duplicated at 100-117). This 18-line block is copy-pasted.

**Fix:** Extract a helper method `func (gt *guildsTree) selectActionDesc() string` that returns "collapse", "expand", or the default description based on node state. Call it from both `ShortHelp` and `FullHelp`.

**Implementation Risk:** Low. Pure refactoring of display logic with no behavioral change.

---

### 7. ✅ FIXED — Duplicated attachmentsPicker Layer Setup
`internal/ui/chat/messages_list.go:1178-1187` and `internal/ui/chat/messages_list.go:1318-1327` -- The code to show the attachments picker overlay is identical in `showAttachmentsList` and `saveImage`: create centered layer, add with overlay options, send to front, set focus. This 10-line pattern is duplicated verbatim.

**Fix:** Extract a method `func (ml *messagesList) showAttachmentsOverlay()` that handles the layer setup and focus. Call it from both `showAttachmentsList` and `saveImage` after setting items.

**Implementation Risk:** Low. Simple extraction of identical code.

---

### 8. ✅ FIXED — Inconsistent Error Handling: Some Paths Silently Ignore Errors
`internal/ui/chat/messages_list.go:1409` -- `me, _ := ml.chatView.state.Cabinet.Me()` discards the error. If the state is not ready, `me` is nil and `me.ID` will panic on the next line. This pattern appears in multiple places: `state.go:195`, `messages_list.go:1446`, `messages_list.go:1539`, `messages_list.go:1563`.

**Fix:** Handle the error case: `me, err := ml.chatView.state.Cabinet.Me(); if err != nil { slog.Error(...); return }`. Apply to all instances where `Me()` is called with a discarded error.

**Implementation Risk:** Low. Adding error checks prevents nil-pointer panics. The `Me()` call should always succeed after login, but defensive coding prevents crashes during edge-case race conditions (e.g., state reconnection).

---

### 9. ✅ FIXED — Magic Number: embedLineStyles Uses Hardcoded Array Size [8]
`internal/ui/chat/messages_list.go:670` -- `[8]tcell.Style{}` is a magic number that must manually stay in sync with the number of `embedLineKind` constants (lines 660-668). Adding a new embed line kind without updating the array size causes a silent bug (zero-value style).

**Fix:** Define a sentinel `const embedLineKindCount = 8` after the last `iota` constant, or use a slice. Example: add `embedLineKindCount` as the last iota entry and use `[embedLineKindCount]tcell.Style{}`.

**Implementation Risk:** Low. Compile-time enforcement prevents future mismatches.

---

### 10. ✅ FIXED — guildState Uses Mutex Where RWMutex Would Be Appropriate
`internal/ui/chat/guildstate.go:14-17` -- `guildState` uses `sync.Mutex` for all access, but `isExpanded` (line 61) is a read-only operation that could use `RLock`. Guild state is read frequently (every node creation during `onReady`) and written infrequently (user expand/collapse action).

**Fix:** Change `mu sync.Mutex` to `mu sync.RWMutex` and use `gs.mu.RLock()`/`gs.mu.RUnlock()` in `isExpanded`.

**Implementation Risk:** Low. Standard Go concurrency pattern upgrade.

---

### 11. Inconsistent Log Level Usage
Multiple files -- Errors that represent user-facing failures are logged at different levels without consistency: `slog.Info` for failed keyring retrieval (`root/events.go:29`), `slog.Info` for failed avatar cache (`notifications.go:65`), `slog.Error` for failed presence lookup (`message_input.go:571-572`) which is actually a normal condition (presence not always available). The inconsistency makes log filtering unreliable.

**Fix:** Establish a convention: `slog.Error` for failures that affect functionality (failed API calls, state corruption), `slog.Warn` for degraded-but-functional conditions (missing avatar, unknown presence), `slog.Info` for expected alternative paths (keyring not found, config file missing). Apply consistently across all files.

**Implementation Risk:** Low. Log level changes have no functional impact but significantly improve operational debugging.

---

### 12. ✅ FIXED — Invalid Log Level Silently Defaults to Zero Value
`cmd/root.go:29-39` -- The `switch` on `logLevel` has no `default` case. An invalid value like `--log-level=trace` silently results in `slog.Level(0)` which is `slog.LevelInfo`. The user gets no feedback that their log level was invalid.

**Fix:** Add a `default` case that either returns an error (`return fmt.Errorf("unknown log level: %q", logLevel)`) or logs a warning and falls back to info.

**Implementation Risk:** Low. Adds user feedback for misconfiguration.

---

## DOCUMENTATION GAPS

### 13. No File-Level Comments on Most Source Files
All `.go` files except `internal/cache/cache.go` lack a package-level doc comment explaining the file's purpose. For a new developer, understanding what `state.go`, `events.go`, `keybinds.go`, and `util.go` contain requires reading them entirely. The `internal/ui/chat/` package is particularly challenging with 10+ files and no file-level guidance.

---

### 14. No Documentation on the Event/Command Architecture
`internal/ui/chat/model.go`, `internal/ui/chat/events.go`, `internal/ui/root/events.go` -- The application uses a custom event-driven architecture (tview.Event/tview.Command pattern) that is central to understanding the codebase. There is no documentation explaining: (a) how events flow from gateway to UI, (b) the Command pattern (returning closures from handlers), (c) the `listen()` loop pattern, or (d) how `tview.Batch` composes commands. A new developer cannot understand the control flow without reverse-engineering it.

---

### 15. No Documentation on the Rendering Pipeline
`internal/markdown/renderer.go`, `internal/ui/chat/messages_list.go` -- The message rendering pipeline (Discord message -> goldmark AST -> tview.LineBuilder -> tview.Line with styled segments) is complex and undocumented. Key concepts like `styleStack`, `linkDepth`, `MergeStyle`, embed rendering with wrapping, and OSC 8 URL metadata have no explanatory comments.

---

### 16. Config Validation Rules Undocumented
`internal/config/config.go:102-103` -- `AutocompleteLimit` and `MessagesLimit` are `uint8` types. The config.toml comment says "minimum and maximum value is 1 and 100" for `messages_limit`, but this constraint is not enforced in code. A user setting `messages_limit = 255` would silently exceed the Discord API limit. The `uint8` type provides implicit 0-255 range but the TOML comment claims 1-100.

---

## PERFORMANCE

### 17. Unbounded itemByID Cache in messagesList
`internal/ui/chat/messages_list.go:58` -- `itemByID map[discord.MessageID]*tview.TextView` caches rendered message TextViews and is only cleared on channel switch (`reset()`) or when messages are modified. For channels with heavy traffic, this map grows without bound as new messages arrive. Each `tview.TextView` holds rendered line data, styles, and a text buffer. Over a long session in a busy channel, this can accumulate thousands of entries.

**Fix:** Add a size cap. When `len(itemByID) > maxCacheSize` (e.g., 500), evict entries for message IDs no longer in the current `messages` slice. Alternatively, clear the cache when it exceeds a threshold during `addMessage`.

**Implementation Risk:** Low-medium. Cache eviction during message additions may cause brief re-rendering of older messages if the user scrolls up. The visual impact is negligible since re-rendering is the fallback path.

---

### 18. ✅ FIXED — collapseParentNode Uses O(n) Tree Walk
`internal/ui/chat/guilds_tree.go:380-395` -- `collapseParentNode` walks the entire tree from root to find the parent of the given node, even though `GetPath(node)` (used in `canCollapseParent`) returns the full path including the parent. The O(n) walk runs on every `-` keypress.

**Fix:** Use `gt.GetPath(node)` to get the parent directly: `path := gt.GetPath(node); if len(path) >= 3 { parent := path[len(path)-2]; ... }`. This is O(depth) instead of O(total nodes).

**Implementation Risk:** Low. `GetPath` is already used in `canCollapseParent` and `expandPathToNode`, so it is a known-reliable method. Verify the path includes the root node as the first element.

---

### 19. 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.

**Implementation Risk:** Low. Cache invalidation happens naturally when `SetCurrentNode` is called.

---

### 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.

**Fix:** Parse content once and pass both the rendered lines and the extracted URLs to the embed renderer. This requires restructuring `drawDefaultMessage` to call a combined parse+render function that returns both outputs.

**Implementation Risk:** Medium. The rendering pipeline currently separates content rendering from embed rendering. Merging them requires careful handling of the `forceMarkdown` flag used in embed descriptions.

---

## INFRASTRUCTURE

### 21. ✅ FIXED — CI Uses go-version: stable -- Not Pinned
`.github/workflows/ci.yml:25` -- As documented in `research/TECHFILE.md` (finding #5), the CI uses `go-version: stable` which auto-advances when Go releases a new version. This can introduce unexpected breakage from new vet rules or behavior changes.

**Fix:** Pin to `go-version: "1.26"` or `"1.26.x"`. Update intentionally when upgrading.

**Implementation Risk:** None from pinning. The risk is the current unpinned behavior.

---

### 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.

**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.

**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.

---

### 23. ✅ FIXED — CI Artifact Name Does Not Match Binary Name
`.github/workflows/ci.yml:41-43` -- CI uploads artifact named `discordo_$OS_$ARCH` but the binary is `discordo` (or `discordo.exe`). The project README and CLAUDE.md refer to the binary as `discordo-plus`. There is a naming inconsistency between the upstream module path (`github.com/ayn2op/discordo`), the CI artifact name, and the intended installation name.

**Fix:** Update the build step to `go build -trimpath -ldflags=-s -o discordo-plus .` (or `discordo-plus.exe` on Windows) to match the documented binary name. Update the artifact upload to reference the correct file.

**Implementation Risk:** Low. Downstream users of CI artifacts will see the renamed binary. Scoop/AUR packages may need path updates.

---

## TEST COVERAGE

### 24. Only 2 Packages Have Tests -- Minimal Coverage
`internal/config/config_test.go`, `internal/keyring/keyring_test.go` -- Out of 12 internal packages, only 2 have any tests. Zero test coverage for: markdown rendering, clipboard operations, HTTP transport, notifications, cache, guild state persistence, UI components, embed processing, URL extraction, and the entire event handling pipeline.

Critical untested code paths:
- `cache.go:Get` -- can panic (finding #1)
- `transport.go` -- body leak (finding #2)
- `guildstate.go` -- JSON marshal/unmarshal roundtrip
- `messages_list.go` -- `extractURLs`, `messageURLs`, `wrapStyledLine`, `unescapeMarkdownEscapes`, `linkDisplayText`, `sameLocalDate` are all pure functions ideal for unit testing
- `config/theme.go` -- TOML unmarshaling for all wrapper types
- `util.go` -- `MergeStyle`, `SortGuildChannels`, `ChannelToString`

**Fix:** Prioritize tests for: (a) pure functions in messages_list.go (URL extraction, wrapping, escaping), (b) cache.go roundtrip and race conditions, (c) guildstate.go persistence roundtrip, (d) theme.go unmarshaling edge cases. These require no mocking and have clear input/output contracts.

**Implementation Risk:** None. Tests are additive.

---

## POSITIVES

- Go 1.26 module minimum is the latest stable release; dependency versions are current (see TECHFILE.md).
- Error wrapping with `%w` is used consistently in config loading, HTTP transport, and attachment handling.
- The event/command architecture cleanly separates I/O from UI updates; all gateway operations run in commands (closures), keeping the UI thread responsive.
- Config defaults are embedded via `//go:embed`, eliminating runtime file-not-found failures.
- Token storage uses the OS keyring (zalando/go-keyring) rather than plaintext files.
- Guild state persistence uses proper mutex synchronization around both reads and writes.
- The typing indicator implementation correctly uses `time.AfterFunc` with cleanup, preventing timer leaks.
- `selectedChannelMu` RWMutex correctly protects the selected channel across goroutines.
- The markdown renderer's style stack pattern is clean and handles nested formatting correctly.
- The embed deduplication logic (`embedLineDedupKey`) prevents duplicate content across embed fields.
- Platform-specific code uses Go's filename-based build tag convention cleanly (clipboard, editor, suspend).
- The `ConfigurePicker` helper in `util.go` avoids duplicating picker setup across three picker types.
- `wrapStyledLine` correctly uses grapheme cluster width via `uniseg` for accurate wrapping with wide glyphs and emoji.

---

## Summary

| Severity | Total |
|----------|-------|
| Critical | 4 |
| Code Quality | 8 |
| Documentation Gaps | 4 |
| Performance | 4 |
| Infrastructure | 3 |
| Test Coverage | 1 |
| Compliance | N/A |

---

## Files Reviewed

| # | File | Type |
|---|---|---|
| 1 | `main.go` | Entry point |
| 2 | `cmd/root.go` | CLI / app init |
| 3 | `internal/config/config.go` | Config struct + loader |
| 4 | `internal/config/config.toml` | Embedded default config |
| 5 | `internal/config/config_test.go` | Config tests |
| 6 | `internal/config/keybinds.go` | Keybind definitions |
| 7 | `internal/config/theme.go` | Theme TOML unmarshaling |
| 8 | `internal/config/editor_unix.go` | Unix editor command |
| 9 | `internal/config/editor_default.go` | Non-unix editor command |
| 10 | `internal/consts/consts.go` | App name + cache dir |
| 11 | `internal/logger/logger.go` | slog file logger |
| 12 | `internal/keyring/keyring.go` | Token keyring wrapper |
| 13 | `internal/keyring/keyring_test.go` | Keyring tests |
| 14 | `internal/cache/cache.go` | Autocomplete cache |
| 15 | `internal/http/transport.go` | HTTP transport (gzip + brotli) |
| 16 | `internal/http/client.go` | API client constructor |
| 17 | `internal/http/headers.go` | Request headers |
| 18 | `internal/http/props.go` | Discord identify properties |
| 19 | `internal/notifications/notifications.go` | Notification dispatch |
| 20 | `internal/notifications/desktop_toast.go` | Non-darwin notification |
| 21 | `internal/notifications/desktop_toast_darwin.go` | macOS notification |
| 22 | `internal/clipboard/clipboard.go` | Clipboard format types |
| 23 | `internal/clipboard/clipboard_default.go` | Default clipboard impl |
| 24 | `internal/clipboard/clipboard_wayland.go` | Wayland clipboard impl |
| 25 | `internal/markdown/renderer.go` | Discord markdown renderer |
| 26 | `internal/ui/util.go` | Shared UI helpers |
| 27 | `internal/ui/root/model.go` | Root model + help overlay |
| 28 | `internal/ui/root/events.go` | Root events (token, clipboard) |
| 29 | `internal/ui/root/keybinds.go` | Root keymap |
| 30 | `internal/ui/root/suspend_unix.go` | Unix SIGTSTP suspend |
| 31 | `internal/ui/root/suspend_default.go` | No-op suspend |
| 32 | `internal/ui/chat/model.go` | Chat model + layout |
| 33 | `internal/ui/chat/state.go` | Gateway state commands |
| 34 | `internal/ui/chat/events.go` | Chat event handlers |
| 35 | `internal/ui/chat/keybinds.go` | Chat keymap |
| 36 | `internal/ui/chat/guilds_tree.go` | Guild/channel tree |
| 37 | `internal/ui/chat/guildstate.go` | Guild expand persistence |
| 38 | `internal/ui/chat/messages_list.go` | Message rendering + actions |
| 39 | `internal/ui/chat/message_input.go` | Message input + mentions |
| 40 | `internal/ui/chat/attachments_picker.go` | Attachment picker |
| 41 | `internal/ui/chat/channels_picker.go` | Channel picker |
| 42 | `internal/ui/chat/mentions_list.go` | Mentions autocomplete list |
| 43 | `internal/ui/chat/util.go` | Picker config + humanJoin |
| 44 | `internal/ui/login/model.go` | Login model |
| 45 | `internal/ui/login/events.go` | Login events |
| 46 | `internal/ui/login/keybinds.go` | Login keymap |
| 47 | `internal/ui/login/token/model.go` | Token login form |
| 48 | `internal/ui/login/token/events.go` | Token events |
| 49 | `internal/ui/login/qr/model.go` | QR login model |
| 50 | `internal/ui/login/qr/events.go` | QR login events + crypto |
| 51 | `go.mod` | Module manifest |
| 52 | `.github/workflows/ci.yml` | CI configuration |
| 53 | `.gitignore` | Git ignore rules |
| 54 | `README.md` | Project readme |
| 55 | `CLAUDE.md` | Project context |
| 56 | `internal/config/config.toml` | Default config (embedded) |