Conversation
Replace hardcoded j/k/up/down strings in handleAnnotListKey with keymap.Resolve() ActionDown/ActionUp so remapped keys work in the annotation list popup. Enter stays hardcoded (modal convention), esc uses hybrid pattern matching handleHelpKey.
Accept io.Writer params for testability, remove os.Exit calls, unify color field mapping via colorFieldPtrs(), use early continue in applyTheme loop.
There was a problem hiding this comment.
Pull request overview
Implements the set of fixes called out by an external code review: stricter theme parsing (hex validation + optional keys), routing annotation-list navigation through the keymap, propagating write failures from Keymap.Dump(), refactoring handleThemes() to be non-exiting/testable, and deduplicating color-key ↔ options-field mapping.
Changes:
- Validate
color-*values as strict#RRGGBBduringtheme.Parse(), and allow certain background-related color keys to be omitted. - Route annotation list up/down navigation via
keymap.ActionUp/ActionDownand add tests for remapping behavior. - Make
Keymap.Dump(w)return an error and refactor theme handling + color mapping incmd/revdiff/main.go, with updated tests/docs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/annotlist.go | Uses keymap actions for annotation list navigation; keeps Enter/Esc handling in overlay logic. |
| ui/annotlist_test.go | Adds coverage for remapped navigation keys and Enter/Esc behavior. |
| theme/theme.go | Adds strict hex color validation and introduces a set of optional color keys. |
| theme/theme_test.go | Adds tests for hex validation, parse failures, and optional-key omission/roundtrip. |
| keymap/keymap.go | Changes Dump to return errors instead of discarding writer failures. |
| keymap/keymap_test.go | Updates existing tests for new Dump signature and adds failing-writer tests. |
| cmd/revdiff/main.go | Refactors handleThemes to return (bool, error) and deduplicates color field mapping via colorFieldPtrs. |
| cmd/revdiff/main_test.go | Adds/refactors tests for the new theme handling, optional key behavior, and color mapping. |
| docs/plans/completed/20260406-review-findings.md | Adds completed plan documenting the addressed findings and verification steps. |
| CLAUDE.md | Updates project docs to reflect new theme/keymap behavior and new mapping helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch action { //nolint:exhaustive // only navigation actions are relevant in annotation list overlay | ||
| case keymap.ActionUp: | ||
| if m.annotListCursor > 0 { | ||
| m.annotListCursor-- | ||
| // scroll up if cursor is above visible area |
There was a problem hiding this comment.
ActionUp/ActionDown handling runs before the hardcoded Enter/Esc handling. If a user remaps enter or esc (or any key producing tea.KeyEnter/tea.KeyEsc) to ActionUp/ActionDown, the first switch will return early and the overlay will no longer jump/close, contradicting the intended “enter/esc always work” behavior. Consider giving tea.KeyEnter/tea.KeyEsc precedence (handle them before the navigation switch, or explicitly guard the navigation cases).
cmd/revdiff/main_test.go
Outdated
| for _, key := range theme.ColorKeys() { | ||
| th.Colors[key] = "#" + key[:6] // unique value per key | ||
| } |
There was a problem hiding this comment.
This test populates theme colors with values like "#" + key[:6] (e.g. #color-), which are not valid hex colors. With hex validation now enforced at theme parse time, it’s safer for tests to use valid #RRGGBB values to avoid future failures if validation is later reused in apply/dump paths.
cmd/revdiff/main_test.go
Outdated
|
|
||
| done, err := handleThemes(&opts, "/some/dir", &stdout, &stderr) |
There was a problem hiding this comment.
This test passes a hard-coded absolute path (/some/dir) as themesDir. Because handleThemes may attempt an auto-init when the dir doesn’t exist (even if errors are ignored), this can cause unintended filesystem side effects (and could succeed if tests run with elevated permissions). Prefer using an existing t.TempDir() (or creating the directory) to keep the test hermetic.
| done, err := handleThemes(&opts, "/some/dir", &stdout, &stderr) | |
| themesDir := t.TempDir() | |
| done, err := handleThemes(&opts, themesDir, &stdout, &stderr) |
Handle modal enter/esc before keymap navigation in annotation list to prevent remapping conflicts. Use valid hex colors and t.TempDir() in tests.
Deploying revdiff with
|
| Latest commit: |
246ac1a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://552506fb.revdiff.pages.dev |
| Branch Preview URL: | https://review-findings.revdiff.pages.dev |
Address 5 findings from external code review.
Keymap.Dump()instead of silently discarding write failureshandleThemes()to return(bool, error)withio.Writerparams instead of callingos.ExitdirectlycolorFieldPtrs(), eliminating triple duplication of the 21-key list