candy-hermit: Fix string items, scrolling viewport, HelpBar/StatusBar, and 8 other remediations#1167
Merged
Conversation
Coerce raw strings to FilteredItem in constructor and withItems() so that $item->value() calls never fatal on string inputs. Adds private coerceItems() helper that wraps non-Item entries as FilteredItem($i + 1, (string) $entry) and passes Items through unchanged. selected() now always returns an Item (not null unless the filtered list is empty). Mirrors charmbracelet/bubbletea item coercion pattern.
Tests the full string-items path: coercion in ::new(), filter narrowing via type(), View() rendering, selected() returning valid Item, withItems() re-coercion, custom itemFormatter receiving string values, and allCount() / itemCount() counts. Mirrors charmbracelet/bubbletea string-item integration test pattern.
Wrap the file-read loop in try/finally to guarantee the handle is closed even when json_decode throws JsonException. Wrap the json_decode call in its own try/catch so a single malformed line skips rather than aborting the full read. Adds testAllSkipsCorruptLine (verifies 2 valid items returned when a bad JSON line is sandwiched between good lines) and testAllClosesHandleEvenOnCorruptLines (verifies repeated all() calls don't exhaust file descriptors).
The anonymous Handler class tracked $byteOffset and $charPositions across every print/execute/dispatch call, but only $charString (the accumulated rune string) was ever returned. Remove the dead accounting fields and simplify all dispatch methods to empty no-ops. No functional change: printableText() still returns the same rune string. Existing highlight tests (CJK, emoji, SGR placement) confirm byte-identical output is preserved.
Tests that View() output for 'banana' filtered by 'an' with setMatchStyle yellow (\x1b[33m) contains the exact highlighted fragment \x1b[33man\x1b[0m, confirming correct SGR wrap placement around the matched run. Uses assertNotFalse(strpos) rather than assertStringContainsString because PHPUnit failure output represents non-printable bytes ([1b) by their escaped form, which can mislead the eye when comparing byte-exact needles against raw string haystacks.
When backspace reduces the filtered list to empty, the min() clamp could yield cursor = -1 (min(cursor, 0-1) = min(cursor, -1) = -1 when cursor >= 0). Wrap with max(0, ...) so an emptied list always yields cursor 0. Adds testBackspaceCursorNeverNegative: sets a filter that excludes everything, types to empty result, backspaces, and asserts cursor() === 0.
Replace the stale COLUMNS/LINES env-var size provider with a live query via SugarCraft\Core\Util\Tty::size(), wrapped in a private ttySize() helper that falls back to 80x24 on any exception (signal handlers must not propagate exceptions). Also fixes a pre-existing bug where STDIN (a resource) was passed to SignalForwarder::attachSigwinchToFd() which expects an int file descriptor — now casts to (int) STDIN. Updates the [pattern:sigwinch-...] CALIBER_LEARNINGS.md note to reflect the Tty::size() provider. Adds testAttachSigwinchInstallsHandler: when SIGWINCH+pcntl are available it asserts attachSigwinch() returns true with a callback set; skips otherwise.
- Removed $clone->isShown = true from setOffset() — it is now a pure position setter - Updated testFluentSetters to call ->show() explicitly and added assertion that setOffset alone does NOT show - Added ->show() to 4 HermitRankerTest methods that relied on setOffset auto-show side effect - All 63 tests pass
…sBar wiring Step 9: itemFormatter now accepts optional third int param (number) - Default closure shows ordinal: '3. cherry' not just 'cherry' - Updated docblock and call sites to pass number() Step 10: Scrolling viewport in View() - Compute top offset to keep cursor visible in window - Center cursor in visible window when possible - Added testScrollingViewport tests Step 11: Wire HelpBar and StatusBar into View() - HelpBar/StatusBar render() output appended after items - Bars extend the overlay past windowHeight - Added testHelpBarAndStatusBarRenderInView
- Strip raw newlines/carriage returns from formatted item strings - Prevents embedded newlines from injecting extra rows in output - Added testItemWithEmbeddedNewlineDoesNotInjectRows
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 3 medium |
| CodeStyle | 11 minor |
🟢 Metrics 12 complexity · 0 duplication
Metric Results Complexity 12 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
1 participant
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
Implements 12 of 15 remediation steps from
findings/plan_candy-hermit.mdforcandy-hermit:Itemat every entry point (prevents fatal at runtime)FileHistory::all()against corrupt JSON lines + handle leakprintableText()backspace()cursor floor to 0 (prevents -1 cursor)attachSigwinch()to query real TTY size viaTty::size()setOffset()from auto-show (pure positioning setter)itemFormatternow shows item ordinal (e.g. "3. cherry")HelpBarandStatusBarintoView()outputTest plan
68 tests, 147 assertions — all pass.
Notes
matchAll()perf) not implementedcandy-hermitis a leaf library — no downstream dependency ordering required