Conversation
Covers the "More Themes" fzf picker marking already-installed themes with a leading checkmark (HyDE-Project/HyDE#2093): the marker must round-trip through fzf's --ansi, which strips SGR color codes from every value it hands back via {} and keeps only the bare glyph. A first version of the marker compared against the colored prefix, which fzf's {} never actually contains, so every installed theme's JSON lookup silently failed. This test asserts the JSON lookup actually succeeds with fzf's real, already-stripped output, not just that the script doesn't crash on it -- the earlier, weaker version of this test passed against the buggy code, since the not-found fallback doesn't crash either. Also covers a related crash found while testing this: get_theme_preview() iterated JSON_DATA directly, raising TypeError instead of the intended "not found" fallback when the gallery cache isn't populated (--skip-clone on a fresh machine, or a failed clone). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1SjbyvkBXwHm4ZprDQc93
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
main()'s --preview branch used to discard get_theme_preview()'s return value entirely, so the not-found path (unknown theme, or missing gallery data) never reached fzf's preview pane -- it was just blank. Fixed alongside in HyDE-Project/HyDE#2093; this asserts the actual user-visible output (stdout), which the debug-log-only checks here didn't cover. Flagged by CodeRabbit on PR #2093. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1SjbyvkBXwHm4ZprDQc93
CodeRabbit on HyDE-Project/HyDE#2093: strip_installed_marker() has two call sites (get_theme_preview()'s JSON lookup, fzf_menu()'s final SELECTED_THEMES list) and only the first was covered, via the --preview subprocess path. Simulating fzf_menu()'s actual interactive selection isn't practical from a black-box test, but the function itself is pure -- a direct unit test against all three prefix shapes (colored, ansi-stripped, not-installed) covers both call sites at once, since they share it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1SjbyvkBXwHm4ZprDQc93
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--ansi-stripped output (bare glyph, no color codes), not just that the script doesn't crash — the first version of this test only checked "doesn't crash" and would have passed against the actual bug (the not-found fallback doesn't crash either).get_theme_preview()iteratingJSON_DATAdirectly raisedTypeErrorinstead of the intended "not found" message when the gallery cache isn't populated yet.Companion PR with the actual fix: HyDE-Project/HyDE#2093
Test plan
REPO_ROOT=<HyDE checkout> bash test_theme_import_installed_marker.shpasses against the fixedtheme.import.py