Repository navigation
UI/upgraded UI - #1605
Merged
Merged
UI/upgraded UI#1605
Conversation
Contributor
📝 WalkthroughWalkthroughAdds brand constants and theme fields; implements a builtin status line with segment builders, worst-rate aggregation, and width-fitting; introduces highlightFuzzyMatch and integrates it into pickers; refactors Dialog and FuzzyPicker APIs; updates logos, startup palette, spinner behavior, and adds CompletionFlash; wires footer/status-line settings. ChangesBrand, Status Line, and UI Polish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Spinner/CompletionFlash.tsx`:
- Around line 48-49: The elapsedMs computation in CompletionFlash uses
loadingStartTimeRef.current and totalPausedMsRef.current without null guards,
which can produce NaN if refs are uninitialized; update the logic in
CompletionFlash to defensively handle null/undefined for
loadingStartTimeRef.current and totalPausedMsRef.current (e.g., coalesce to 0 or
early-return if wasActive is false and refs are missing) so the MIN_TURN_MS
comparison behaves predictably; locate the elapsedMs line and change the
calculation to use safe fallbacks or an explicit null check before comparing to
MIN_TURN_MS.
In `@src/components/Spinner/SpinnerAnimationRow.tsx`:
- Around line 182-184: The parensWidth currently always adds the mode glyph
space when !hasRunningTeammates, but the glyph is actually rendered only when
!hasRunningTeammates && !thinkingOnly && parts.length > 0; update the
parensWidth calculation in SpinnerAnimationRow (symbol: parensWidth) to add the
extra 2 + SEP_WIDTH only under that full condition (use hasRunningTeammates,
thinkingOnly, and parts.length) so width reservation matches actual rendering
and avoids over-reserving on narrow terminals.
In `@src/utils/config.ts`:
- Around line 598-600: The createDefaultGlobalConfig factory should explicitly
set defaultStatusLineEnabled: true to match the documented default and other
boolean defaults; update the createDefaultGlobalConfig function to include
defaultStatusLineEnabled: true so the produced config object self-documents this
default instead of relying on UI fallbacks (refer to the
defaultStatusLineEnabled field and createDefaultGlobalConfig).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc3a0147-e8b1-4b29-b46a-8fbae6f2dfc6
📒 Files selected for processing (31)
src/components/BuiltinStatusLine.test.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/CustomSelect/select.tsxsrc/components/DevChannelsDialog.tsxsrc/components/HistorySearchDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/LogoV2/Clawd.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/LogoV2/LogoV2.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/components/QuickOpenDialog.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/Settings/Config.tsxsrc/components/Spinner.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/components/StartupScreen.tssrc/components/StructuredDiff/Fallback.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/design-system/Dialog.tsxsrc/components/design-system/FuzzyPicker.tsxsrc/constants/brand.tssrc/constants/figures.tssrc/native-ts/color-diff/index.tssrc/screens/REPL.tsxsrc/utils/config.tssrc/utils/highlightMatch.test.tsxsrc/utils/highlightMatch.tsxsrc/utils/theme.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/components/IdleReturnDialog.tsxsrc/constants/figures.tssrc/components/QuickOpenDialog.tsxsrc/components/Spinner/utils.tssrc/components/WorktreeExitDialog.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/LogoV2/LogoV2.tsxsrc/components/DevChannelsDialog.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/native-ts/color-diff/index.tssrc/components/HistorySearchDialog.tsxsrc/components/StartupScreen.tssrc/components/StructuredDiff/Fallback.tsxsrc/components/CustomSelect/select.tsxsrc/screens/REPL.tsxsrc/constants/brand.tssrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/StartupScreen.palettes.tssrc/components/design-system/FuzzyPicker.tsxsrc/components/Settings/Config.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.tsxsrc/components/LogoV2/Clawd.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/utils/highlightMatch.test.tsxsrc/utils/config.tssrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsxsrc/utils/theme.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/IdleReturnDialog.tsxsrc/constants/figures.tssrc/components/QuickOpenDialog.tsxsrc/components/Spinner/utils.tssrc/components/WorktreeExitDialog.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/LogoV2/LogoV2.tsxsrc/components/DevChannelsDialog.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/native-ts/color-diff/index.tssrc/components/HistorySearchDialog.tsxsrc/components/StartupScreen.tssrc/components/StructuredDiff/Fallback.tsxsrc/components/CustomSelect/select.tsxsrc/screens/REPL.tsxsrc/constants/brand.tssrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/StartupScreen.palettes.tssrc/components/design-system/FuzzyPicker.tsxsrc/components/Settings/Config.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.tsxsrc/components/LogoV2/Clawd.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/utils/highlightMatch.test.tsxsrc/utils/config.tssrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsxsrc/utils/theme.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/components/IdleReturnDialog.tsxsrc/constants/figures.tssrc/components/QuickOpenDialog.tsxsrc/components/Spinner/utils.tssrc/components/WorktreeExitDialog.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/LogoV2/LogoV2.tsxsrc/components/DevChannelsDialog.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/native-ts/color-diff/index.tssrc/components/HistorySearchDialog.tsxsrc/components/StartupScreen.tssrc/components/StructuredDiff/Fallback.tsxsrc/components/CustomSelect/select.tsxsrc/screens/REPL.tsxsrc/constants/brand.tssrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/StartupScreen.palettes.tssrc/components/design-system/FuzzyPicker.tsxsrc/components/Settings/Config.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.tsxsrc/components/LogoV2/Clawd.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/utils/highlightMatch.test.tsxsrc/utils/config.tssrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsxsrc/utils/theme.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/components/BuiltinStatusLine.test.tsxsrc/utils/highlightMatch.test.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/BuiltinStatusLine.test.tsxsrc/utils/highlightMatch.test.tsx
🔇 Additional comments (43)
src/components/CustomSelect/select.tsx (1)
592-592: LGTM!src/components/ScrollKeybindingHandler.tsx (1)
568-572: LGTM!src/components/StructuredDiff/Fallback.tsx (1)
410-415: LGTM!src/native-ts/color-diff/index.ts (1)
736-746: LGTM!src/utils/highlightMatch.tsx (1)
29-69: LGTM!src/utils/highlightMatch.test.tsx (1)
1-70: LGTM!src/components/QuickOpenDialog.tsx (1)
13-13: LGTM!Also applies to: 192-192
src/components/design-system/Dialog.tsx (1)
34-72: LGTM!src/components/design-system/FuzzyPicker.tsx (2)
25-28: LGTM!
175-175: LGTM!src/components/DevChannelsDialog.tsx (1)
89-89: LGTM!src/components/IdleReturnDialog.tsx (1)
94-94: LGTM!src/components/WorktreeExitDialog.tsx (1)
227-229: LGTM!src/components/HistorySearchDialog.tsx (2)
12-12: LGTM!
91-96: LGTM!src/components/LogoV2/CondensedLogo.tsx (1)
4-4: LGTM!Also applies to: 92-92, 105-105, 145-145
src/components/LogoV2/LogoV2.tsx (1)
10-10: LGTM!Also applies to: 377-377
src/components/LogoV2/Clawd.tsx (1)
101-101: LGTM!Also applies to: 109-109, 117-117, 135-135, 143-143, 150-150, 190-190, 198-198, 206-206
src/components/StartupScreen.ts (1)
18-18: LGTM!Also applies to: 213-213
src/components/StartupScreen.palettes.ts (1)
22-35: LGTM!Also applies to: 98-98, 101-102
src/components/Spinner/CompletionFlash.tsx (1)
1-64: Component structure and logic look solid otherwise.The transition detection (tracking
prevActiveRef), flash duration gating (MIN_TURN_MS), timeout cleanup, and suppression logic all follow correct patterns. The reduced-motion check is a nice accessibility touch.src/constants/brand.ts (1)
35-37: Check WORDMARK_WIDTH’s terminal width calculation (stringWidth vs .length) and row-length consistency
WORDMARK_WIDTHis computed fromWORDMARK_OPEN[0].lengthandWORDMARK_CLAUDE[0].length, but.lengthcounts UTF-16 code units—not rendered terminal column width (box/block chars can be double-width). It also assumes row[1]matches row[0]even though only[0]is used.If
WORDMARK_WIDTHis used for precise terminal column alignment, switch the calculation to the repo’s existing terminal “string width” helper (e.g.,stringWidth) and/or add a guard/assert that the wordmark rows have consistent visual width; otherwise (rough/non-alignment use), the current approach may be fine.src/utils/theme.ts (1)
9-13: LGTM!Also applies to: 123-126, 456-459
src/components/Spinner.tsx (1)
211-212: LGTM!src/components/Spinner/SpinnerAnimationRow.tsx (1)
221-229: LGTM!src/components/Spinner/utils.ts (1)
4-9: LGTM!src/constants/figures.ts (1)
6-8: LGTM!src/components/BuiltinStatusLine.tsx (6)
1-35: LGTM!
36-89: LGTM!
91-107: LGTM!
109-117: LGTM!
118-176: LGTM!
177-181: LGTM!src/components/BuiltinStatusLine.test.tsx (3)
1-70: LGTM!
72-93: LGTM!
95-107: LGTM!src/components/PromptInput/PromptInputFooter.tsx (3)
19-19: LGTM!
121-122: LGTM!
141-141: LGTM!src/screens/REPL.tsx (1)
69-69: LGTM!Also applies to: 4586-4589
src/utils/config.ts (1)
770-770: LGTM!src/components/Settings/Config.tsx (2)
504-522: LGTM!
1232-1234: LGTM!
…lse spinner Give OpenClaude a real visual identity instead of reskinned upstream visuals: - theme.ts: add brand/brandShimmer keys and repoint the claude family (claude, claudeShimmer, clawd_body, briefLabelClaude) to the gitlawb orange #ff7a1a across all 6 themes — darkened variants for light backgrounds, luminance-separated variants for daltonized themes, redBright floor for 16-color ANSI themes. Values stay rgb() strings (parseRGB in Spinner/utils.ts silently fails on hex). - brand.ts (new): BRAND_NAME, BRAND_TAGLINE, accent constant, and a 2-row Unicode half-block OPENCLAUDE wordmark split for two-tone render. - LogoV2: full logo shows the wordmark + centralized tagline; CondensedLogo drops the duplicated OPEN CLAUDE header and renders a brand-colored name+version line; Clawd mascot body now clawd_body. - Startup splash: new 'ember' gradient palette (brand orange) as the default (/logo keeps sunset et al.); tagline centralized from brand.ts. - Spinner: dot-pulse glyph sweep (· ∘ ○ ◎ ◉ ●) replacing the asterisk set (drops Ghostty/darwin special-cases); defaults switched to brand/brandShimmer. Reduced-motion and stall interpolation unchanged. - figures.ts: TEARDROP_ASTERISK marker → ◎ to match the new glyph family. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
Ship a status bar that renders when no custom /statusline command is configured: `model · ctx % · session cost · rate limit`. Previously this slot was empty unless the user wired up their own shell command. - BuiltinStatusLine.tsx (new): pure segment builders (buildBuiltinStatusSegments / fitSegments — drops lowest-priority segments right-to-left as the terminal narrows) plus a memoized component reusing the exact data pipeline the custom StatusLine feeds to user commands (getRuntimeMainLoopModel, getCurrentUsage, calculateContextPercentages, getTotalCost, getRawUtilization). Pure in-process computation — no subprocess, no debounce. - Context % colors warning ≥70 / error ≥90 (aligned with auto-compact warnings); rate limit shows the worst of the 5h/7d windows and is absent (not 0%) for API-key users; cost hidden at $0. - PromptInputFooter: custom statusline always wins; the built-in bar also suppresses the "? for shortcuts" hint like the custom one does. - defaultStatusLineEnabled global-config toggle (default on) surfaced in /config settings. - Unit tests for segment building, narrow-width fitting, threshold colors, and custom-statusline precedence. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
…iff gutter, dialog hints - Fuzzy pickers: new highlightFuzzyMatch (contiguous-run or greedy- subsequence, bold so the focused row's color survives) applied to Quick Open and prompt-history results. FuzzyPicker now passes the live query as a third renderItem argument so memoized render callbacks can't capture a stale query. - Select: inline option descriptions now truncate with an ellipsis on narrow terminals instead of clipping at the edge (row made shrinkable, description wrapped in truncate-end). - CompletionFlash (new): static `✓ Done · 12s` row for ~1.5s after a response completes. Keys off the isLoading transition (the spinner also hides mid-turn for streaming text); same row footprint as the spinner; suppressed for sub-second turns, brief mode, open permission/prompt queues, running teammates, and reduced motion. - Diff gutter: line numbers on +/- lines are now dimmed (previously full decoration intensity competed with content, worst on light themes); +/- sigils keep full intensity. Mirrored in the non-highlighted Fallback renderer. - Dialog: optional showNavigationHint prepends "↑/↓ navigate" to the default input guide; opted in from Select-hosting dialogs (IdleReturn, DevChannels, WorktreeExit). Dialog rewritten as plain React (was react-compiler output) to take the new prop safely. - Spinner: the ↑/↓ request-direction glyph now leads the status parens whenever any status shows — previously buried in the tokens part, which only appears after 30s; width gating reserves its space. - Replaced the stale `/`-search TODO in ScrollKeybindingHandler with a pointer to the shipped REPL transcript search. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
…l thinking gate - createDefaultGlobalConfig now sets defaultStatusLineEnabled: true explicitly, matching the factory's other default-true booleans (read sites already coalesced to true; no behavior change). - SpinnerAnimationRow: the width gate reserves mode-glyph space that a thinking-only spin never uses; on narrow terminals where nothing else renders, re-try the thinking gate with that space returned so '(thinking)' shows instead of nothing. CompletionFlash NaN guard was reviewed and not applied: both refs are useRef(0) — initialized numbers by type and construction — and the active→inactive transition guard guarantees the start time was set before the elapsed math runs. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
kevincodex1
force-pushed
the
ui/upgrade-combined
branch
from
June 12, 2026 03:00
dbfc407 to
130b320
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/components/Spinner/SpinnerAnimationRow.tsx (1)
182-184: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueparensWidth still over-reserves in edge cases (same as past review).
The
parensWidthcalculation reserves space for the mode glyph (2) + separator whenever!hasRunningTeammates, but the glyph is only rendered when!hasRunningTeammates && !thinkingOnly && parts.length > 0(line 240). This over-reserves by2 + SEP_WIDTHin thethinkingOnlyandparts.length === 0cases.The second-chance gating (lines 205-214) provides a workaround by recalculating with
bareAvailablewhen needed, so the impact is mitigated in practice. Worth noting for future refactoring if the width logic is revisited.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Spinner/SpinnerAnimationRow.tsx` around lines 182 - 184, parensWidth currently adds the glyph+separator width whenever !hasRunningTeammates, but the glyph is actually rendered only when !hasRunningTeammates && !thinkingOnly && parts.length > 0; update the computation of parensWidth in SpinnerAnimationRow so it only adds the 2 + SEP_WIDTH delta under that exact condition (use the existing variables hasRunningTeammates, thinkingOnly, parts.length) and ensure any downstream gating math that uses parensWidth (including the second-chance recalculation that uses bareAvailable) still behaves correctly after this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/BuiltinStatusLine.test.tsx`:
- Around line 95-107: Add focused tests that cover the config-off path and
PromptInputFooter footer-selection branch: extend the
builtinStatusLineShouldDisplay suite to include a case where
defaultStatusLineEnabled: false returns false and a case where statusLine:
undefined with defaultStatusLineEnabled: false returns false; also add unit
tests for the PromptInputFooter component exercising the three branches (custom
statusLine via statusLine: {type:'command',...}, builtin when
builtinStatusLineShouldDisplay(...) is true, and none when
defaultStatusLineEnabled is false and no custom statusLine) to assert the
user-visible footer output for each branch. Use the existing helper function
builtinStatusLineShouldDisplay and the PromptInputFooter component names to
locate code to test.
In `@src/components/design-system/Dialog.tsx`:
- Around line 49-56: The UI still renders the cancel shortcut even when
cancellation keybinding is disabled; update the defaultInputGuide so the
ConfigurableShortcutHint for cancel is only rendered when isCancelActive is true
(i.e. wrap the ConfigurableShortcutHint usage with a conditional check for
isCancelActive), keeping the other hints (KeyboardShortcutHint for
navigate/confirm and the Byline/exitState logic) unchanged; this touches the
useKeybinding/isCancelActive logic and the ConfigurableShortcutHint inside
defaultInputGuide.
In `@src/components/PromptInput/PromptInputFooter.tsx`:
- Around line 121-122: suppressHint currently disables the "? for shortcuts"
hint whenever statusLineShouldDisplay or builtinStatusLineShouldDisplay are
true, but those functions alone don't account for the extra render guards used
later (isShort, exitMessage.show, isPasting, mode !== 'prompt'), causing the
hint to be hidden even when no status line is actually rendered; modify the
suppressHint computation (or create a single shared boolean like
actualStatusLineVisible) to mirror the exact render condition used at the later
check by AND-ing the statusLineShouldDisplay/builtinStatusLineShouldDisplay
results with !isShort, !exitMessage.show, !isPasting and mode === 'prompt', and
then use that unified boolean in place of the current suppressHint expression
(referencing suppressHint, statusLineShouldDisplay,
builtinStatusLineShouldDisplay, isShort, exitMessage.show, isPasting, and mode).
In `@src/components/Spinner/CompletionFlash.tsx`:
- Around line 47-59: The effect that creates the completion flash needs to clear
any existing flash when suppression turns on: inside the same useEffect handling
turnActive/wasActive/suppressed/reducedMotion (the block referencing wasActive,
suppressed, reducedMotion, setFlash, and the timer variable), add logic that if
suppressed is true then call setFlash(null) and clear any pending timeout before
returning so an active flash (flash/✓ Done) is removed immediately when
suppression becomes active; ensure the existing timer cleanup
(clearTimeout(timer)) still runs when suppression causes an early return.
In `@src/screens/REPL.tsx`:
- Around line 4586-4589: The CompletionFlash suppression currently checks
isBriefOnly, hasRunningTeammates, toolUseConfirmQueue.length,
promptQueue.length, and viewedAgentTask; extend that suppressed expression to
also include the active sandbox/worker/elicitation prompt queues (e.g.
sandboxPromptQueue.length > 0 || workerPromptQueue.length > 0 ||
elicitationPromptQueue.length > 0) so the flash is suppressed whenever any of
those queues are non-empty; update the suppressed prop on CompletionFlash
accordingly (refer to the CompletionFlash JSX and the existing
toolUseConfirmQueue/promptQueue/viewedAgentTask symbols to locate where to add
these checks).
---
Duplicate comments:
In `@src/components/Spinner/SpinnerAnimationRow.tsx`:
- Around line 182-184: parensWidth currently adds the glyph+separator width
whenever !hasRunningTeammates, but the glyph is actually rendered only when
!hasRunningTeammates && !thinkingOnly && parts.length > 0; update the
computation of parensWidth in SpinnerAnimationRow so it only adds the 2 +
SEP_WIDTH delta under that exact condition (use the existing variables
hasRunningTeammates, thinkingOnly, parts.length) and ensure any downstream
gating math that uses parensWidth (including the second-chance recalculation
that uses bareAvailable) still behaves correctly after this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5da3f744-8bda-4002-955b-b2107c794e13
📒 Files selected for processing (31)
src/components/BuiltinStatusLine.test.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/CustomSelect/select.tsxsrc/components/DevChannelsDialog.tsxsrc/components/HistorySearchDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/LogoV2/Clawd.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/LogoV2/LogoV2.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/components/QuickOpenDialog.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/Settings/Config.tsxsrc/components/Spinner.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/components/StartupScreen.tssrc/components/StructuredDiff/Fallback.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/design-system/Dialog.tsxsrc/components/design-system/FuzzyPicker.tsxsrc/constants/brand.tssrc/constants/figures.tssrc/native-ts/color-diff/index.tssrc/screens/REPL.tsxsrc/utils/config.tssrc/utils/highlightMatch.test.tsxsrc/utils/highlightMatch.tsxsrc/utils/theme.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/components/DevChannelsDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/StartupScreen.tssrc/constants/figures.tssrc/components/HistorySearchDialog.tsxsrc/constants/brand.tssrc/utils/highlightMatch.tsxsrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/QuickOpenDialog.tsxsrc/components/StructuredDiff/Fallback.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.test.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/screens/REPL.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/components/Settings/Config.tsxsrc/components/design-system/Dialog.tsxsrc/native-ts/color-diff/index.tssrc/components/design-system/FuzzyPicker.tsxsrc/utils/config.tssrc/components/LogoV2/Clawd.tsxsrc/utils/theme.tssrc/components/LogoV2/LogoV2.tsxsrc/components/CustomSelect/select.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/components/DevChannelsDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/StartupScreen.tssrc/constants/figures.tssrc/components/HistorySearchDialog.tsxsrc/constants/brand.tssrc/utils/highlightMatch.tsxsrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/QuickOpenDialog.tsxsrc/components/StructuredDiff/Fallback.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.test.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/screens/REPL.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/components/Settings/Config.tsxsrc/components/design-system/Dialog.tsxsrc/native-ts/color-diff/index.tssrc/components/design-system/FuzzyPicker.tsxsrc/utils/config.tssrc/components/LogoV2/Clawd.tsxsrc/utils/theme.tssrc/components/LogoV2/LogoV2.tsxsrc/components/CustomSelect/select.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/DevChannelsDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/StartupScreen.tssrc/constants/figures.tssrc/components/HistorySearchDialog.tsxsrc/constants/brand.tssrc/utils/highlightMatch.tsxsrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/QuickOpenDialog.tsxsrc/components/StructuredDiff/Fallback.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.test.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/screens/REPL.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/components/Settings/Config.tsxsrc/components/design-system/Dialog.tsxsrc/native-ts/color-diff/index.tssrc/components/design-system/FuzzyPicker.tsxsrc/utils/config.tssrc/components/LogoV2/Clawd.tsxsrc/utils/theme.tssrc/components/LogoV2/LogoV2.tsxsrc/components/CustomSelect/select.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsx
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/components/DevChannelsDialog.tsxsrc/components/IdleReturnDialog.tsxsrc/components/WorktreeExitDialog.tsxsrc/components/StartupScreen.tssrc/constants/figures.tssrc/components/HistorySearchDialog.tsxsrc/constants/brand.tssrc/utils/highlightMatch.tsxsrc/components/Spinner.tsxsrc/components/ScrollKeybindingHandler.tsxsrc/components/QuickOpenDialog.tsxsrc/components/StructuredDiff/Fallback.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/utils/highlightMatch.test.tsxsrc/components/Spinner/utils.tssrc/components/StartupScreen.palettes.tssrc/screens/REPL.tsxsrc/components/LogoV2/CondensedLogo.tsxsrc/components/Spinner/SpinnerAnimationRow.tsxsrc/components/BuiltinStatusLine.test.tsxsrc/components/Settings/Config.tsxsrc/components/design-system/Dialog.tsxsrc/native-ts/color-diff/index.tssrc/components/design-system/FuzzyPicker.tsxsrc/utils/config.tssrc/components/LogoV2/Clawd.tsxsrc/utils/theme.tssrc/components/LogoV2/LogoV2.tsxsrc/components/CustomSelect/select.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/highlightMatch.test.tsxsrc/components/BuiltinStatusLine.test.tsx
🔇 Additional comments (19)
src/components/LogoV2/CondensedLogo.tsx (1)
4-4: LGTM!Also applies to: 92-92, 105-105, 145-145
src/components/LogoV2/LogoV2.tsx (1)
10-10: LGTM!Also applies to: 377-377
src/components/LogoV2/Clawd.tsx (1)
101-101: LGTM!Also applies to: 109-109, 117-117, 135-135, 143-143, 150-150, 190-190, 198-198, 206-206
src/components/StartupScreen.ts (1)
18-18: LGTM!Also applies to: 213-213
src/components/StartupScreen.palettes.ts (1)
22-35: LGTM!Also applies to: 98-98, 101-102
src/components/CustomSelect/select.tsx (1)
592-592: LGTM!src/components/StructuredDiff/Fallback.tsx (1)
410-415: LGTM!src/native-ts/color-diff/index.ts (1)
736-747: LGTM!src/components/ScrollKeybindingHandler.tsx (1)
568-572: LGTM!src/components/Spinner.tsx (1)
211-212: LGTM!src/components/Spinner/SpinnerAnimationRow.tsx (2)
169-169: LGTM!
229-244: LGTM!src/components/Spinner/utils.ts (1)
4-8: LGTM!src/constants/figures.ts (1)
6-8: LGTM!src/utils/highlightMatch.tsx (1)
29-69: LGTM!src/utils/highlightMatch.test.tsx (1)
1-70: LGTM!src/components/design-system/FuzzyPicker.tsx (1)
25-28: LGTM!Also applies to: 175-175
src/components/HistorySearchDialog.tsx (1)
12-12: LGTM!Also applies to: 91-96
src/components/QuickOpenDialog.tsx (1)
13-13: LGTM!Also applies to: 192-192
…overage - Dialog: the cancel shortcut hint now renders only when isCancelActive, matching the keybinding it advertises. - PromptInputFooter: new exported resolveFooterStatusLine(settings, guards, config?) is the single source of truth for which status line renders — custom wins over builtin, and all render guards (prompt mode, short fullscreen, exit message, pasting) force null. suppressHint and the render now share it, so '? for shortcuts' is suppressed only when a status line actually renders. - CompletionFlash: suppression now clears an already-active flash (the effect resets state and the render guard skips the commit-gap frame) instead of only preventing new ones. - REPL: flash suppression reuses the existing hasActivePrompt aggregate, which covers the sandbox, worker-sandbox, and elicitation queues the hand-rolled expression missed. - builtinStatusLineShouldDisplay takes an injectable config (defaults to getGlobalConfig()) so the config-off path is testable without module mocks; tests added for config-off, custom-wins-regardless, and the resolver's full variant x guard matrix. Validation: typecheck exit 0; full suite 3733 green; smoke + bundle guard green. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/BuiltinStatusLine.tsx (1)
155-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the artificial 10-column floor from the fit budget.
fitSegments()already returns an empty list when even the model does not fit, butMath.max(columns - 5, 10)prevents that path on panes narrower than 10 columns. In that case the footer still reserves a row and suppresses? for shortcutseven though no full segment fits in the actual terminal width.Suggested fix
const segments = fitSegments(buildBuiltinStatusSegments({ ...computed, rateLimit: getWorstRateLimit() }), // paddingX from the footer (2 each side) is already outside this Box; // keep a 1-col safety margin against the truncate ellipsis. - Math.max(columns - 5, 10)); + Math.max(columns - 5, 0));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/BuiltinStatusLine.tsx` around lines 155 - 161, The code artificially floors the fit budget to 10 which prevents fitSegments from returning an empty list on narrow panes; update the call that computes the budget (currently Math.max(columns - 5, 10)) to allow budgets below 10 (e.g. Math.max(columns - 5, 0) or clamp to 0) so fitSegments(buildBuiltinStatusSegments({...}, getWorstRateLimit())) can return an empty array when nothing fits; change the expression near the fitSegments call in BuiltinStatusLine.tsx to use 0 as the minimum instead of 10.
♻️ Duplicate comments (1)
src/components/PromptInput/PromptInputFooter.test.tsx (1)
16-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd one component-level footer regression test.
These tests only prove the resolver output. They still miss the user-visible contract this PR changes in
PromptInputFooter: rendering the custom line vs builtin line vs nothing, and keeping? for shortcutsvisible when no status line is actually on screen. A wiring regression there would pass this suite unchanged.As per coding guidelines, "Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PromptInput/PromptInputFooter.test.tsx` around lines 16 - 57, Tests only exercise the resolveFooterStatusLine helper but miss the user-visible behavior in PromptInputFooter; add a component-level test that mounts PromptInputFooter (using the existing guardsPass and customSettings variants and DEFAULT_GLOBAL_CONFIG) and asserts that when customSettings is provided the DOM shows the custom status line text, when empty config it shows the builtin status line text, and when defaultStatusLineEnabled is false it renders nothing for status line while still displaying the "? for shortcuts" hint; use render from the testing library and queryByText/getByText to verify presence/absence for both the custom and builtin cases and that the shortcuts hint remains visible whenever the resolver returns null.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Spinner/CompletionFlash.tsx`:
- Around line 65-67: The render guard in CompletionFlash (the if checking flash
and suppressed) must also bail out when reduced motion is enabled to avoid one
committed frame of the flash; update the conditional that currently reads "if
(!flash || suppressed) return null;" to also check the reduced-motion state
(e.g., prefersReducedMotion or whatever boolean from the hook) so it becomes "if
(!flash || suppressed || prefersReducedMotion) return null;", ensuring the
element never renders when motion is reduced.
---
Outside diff comments:
In `@src/components/BuiltinStatusLine.tsx`:
- Around line 155-161: The code artificially floors the fit budget to 10 which
prevents fitSegments from returning an empty list on narrow panes; update the
call that computes the budget (currently Math.max(columns - 5, 10)) to allow
budgets below 10 (e.g. Math.max(columns - 5, 0) or clamp to 0) so
fitSegments(buildBuiltinStatusSegments({...}, getWorstRateLimit())) can return
an empty array when nothing fits; change the expression near the fitSegments
call in BuiltinStatusLine.tsx to use 0 as the minimum instead of 10.
---
Duplicate comments:
In `@src/components/PromptInput/PromptInputFooter.test.tsx`:
- Around line 16-57: Tests only exercise the resolveFooterStatusLine helper but
miss the user-visible behavior in PromptInputFooter; add a component-level test
that mounts PromptInputFooter (using the existing guardsPass and customSettings
variants and DEFAULT_GLOBAL_CONFIG) and asserts that when customSettings is
provided the DOM shows the custom status line text, when empty config it shows
the builtin status line text, and when defaultStatusLineEnabled is false it
renders nothing for status line while still displaying the "? for shortcuts"
hint; use render from the testing library and queryByText/getByText to verify
presence/absence for both the custom and builtin cases and that the shortcuts
hint remains visible whenever the resolver returns null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e6c96a2-833b-4123-924a-d1e042a74cc5
📒 Files selected for processing (7)
src/components/BuiltinStatusLine.test.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/PromptInput/PromptInputFooter.test.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/design-system/Dialog.tsxsrc/screens/REPL.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/components/BuiltinStatusLine.test.tsxsrc/components/PromptInput/PromptInputFooter.test.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/screens/REPL.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/components/BuiltinStatusLine.test.tsxsrc/components/PromptInput/PromptInputFooter.test.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/screens/REPL.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/BuiltinStatusLine.test.tsxsrc/components/PromptInput/PromptInputFooter.test.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/screens/REPL.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/BuiltinStatusLine.test.tsxsrc/components/PromptInput/PromptInputFooter.test.tsx
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/components/BuiltinStatusLine.test.tsxsrc/components/PromptInput/PromptInputFooter.test.tsxsrc/components/PromptInput/PromptInputFooter.tsxsrc/screens/REPL.tsxsrc/components/Spinner/CompletionFlash.tsxsrc/components/BuiltinStatusLine.tsxsrc/components/design-system/Dialog.tsx
🔇 Additional comments (1)
src/components/design-system/Dialog.tsx (1)
56-56: LGTM!
- BuiltinStatusLine: remove the artificial 10-column floor on the fit budget — fitSegments returning [] is the signal that nothing fits, and the empty branch already handles it (row-reserve in fullscreen, null otherwise). The floor forced a truncated segment onto panes narrower than the model name. - CompletionFlash: the render guard now also skips the one frame between reducedMotion flipping on and the effect clearing the flash, same as the suppressed case. Component-level mount tests for PromptInputFooter were considered and deliberately not added: the footer needs 10+ mocked contexts to mount, mock.module harnesses leak across bun test files in this repo, and the branching under review is exactly the pure resolveFooterStatusLine contract already pinned by tests. Validation: typecheck exit 0; full suite 3733 green; smoke + bundle guard green. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
Member
Author
|
please have a look bro when you have time @jatmn |
jatmn
approved these changes
Jun 12, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
adityachaudhary99
added a commit
to adityachaudhary99/openclaude
that referenced
this pull request
Jun 12, 2026
…on work
CodeRabbit round 8 P2: the previous implementation called
mock.restore() at file-scope on the assumption it would clear module
mocks registered by other test files (lspRecommendation.test.ts,
officialMarketplaceStartupCheck.test.ts). Bun's docs confirm
mock.restore() does NOT clear mock.module() registrations, so the
partial mock from the prior file persisted and the import of _test
broke ("Export named '_test' not found").
The reproducer is real: running lspRecommendation.test.ts first then
this file fails. But the root cause is a pre-existing bug in
lspRecommendation.test.ts (its mock for '../config.js' is missing
normalizeMaxMessagesCompactionThreshold, added to config.ts in
PR Twigpine#1605 / commit 2002e4c). That lspRecommendation test fails on
its own on origin/main, and was already failing the lspRecommendation
test before the marketplaceManager test even ran.
Drop the mock.restore() call so the test's import relies on Bun's
natural module resolution. The marketplaceManager test passes in
isolation. The full-suite failure is caused by the pre-existing
lspRecommendation bug, not by this test. Fixing that is out of scope
for Twigpine#1531.
adityachaudhary99
added a commit
to adityachaudhary99/openclaude
that referenced
this pull request
Jun 12, 2026
…on work
CodeRabbit round 8 P2: the previous implementation called
mock.restore() at file-scope on the assumption it would clear module
mocks registered by other test files (lspRecommendation.test.ts,
officialMarketplaceStartupCheck.test.ts). Bun's docs confirm
mock.restore() does NOT clear mock.module() registrations, so the
partial mock from the prior file persisted and the import of _test
broke ("Export named '_test' not found").
The reproducer is real: running lspRecommendation.test.ts first then
this file fails. But the root cause is a pre-existing bug in
lspRecommendation.test.ts (its mock for '../config.js' is missing
normalizeMaxMessagesCompactionThreshold, added to config.ts in
PR Twigpine#1605 / commit 2002e4c). That lspRecommendation test fails on
its own on origin/main, and was already failing the lspRecommendation
test before the marketplaceManager test even ran.
Drop the mock.restore() call so the test's import relies on Bun's
natural module resolution. The marketplaceManager test passes in
isolation. The full-suite failure is caused by the pre-existing
lspRecommendation bug, not by this test. Fixing that is out of scope
for Twigpine#1531.
adityachaudhary99
added a commit
to adityachaudhary99/openclaude
that referenced
this pull request
Jun 12, 2026
…ache-busted URL
CodeRabbit round 8 follow-up + jatmn round 8 P2: the test file's static
`import { _test } from './marketplaceManager.js'` resolves to whichever
mock was registered first for the bare path. Two other test files
register partial mocks at module top-level:
- lspRecommendation.test.ts:32 mocks the path with 9 exports (no _test)
- officialMarketplaceStartupCheck.test.ts:96 mocks it with 9 exports
(no _test)
Whichever runs first wins, and downstream test files re-importing the
same bare path inherit that partial mock — yielding
"Export named '_test' not found in module './marketplaceManager.js'"
and 4 failing tests in the full suite.
The fix uses three techniques:
1. The static import at the top of marketplaceManager.test.ts is changed
to `'./marketplaceManager.js?bust=this-test-needs-the-real-module'`.
Bun treats the query-string suffix as a distinct module id, so the
import bypasses the bare-path mock.module() registration in
lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts.
The constant suffix is sufficient because we only need the import to
happen once at module top-level — no per-test re-evaluation is
required for the cache-sharing test block.
2. The EXDEV describe block's dynamic re-import (line 263) was already
using a non-busted URL `'./marketplaceManager.ts'` to pick up the
mocked axios. It gets the partial mock from the other test files
instead, with the same `_test` is undefined failure. Apply the
same `?bust=exdev-test-reimport` suffix so the re-import is treated
as a fresh module id and still picks up the axios mock (because
`'axios'` is a different module id than the busted marketplace URL).
3. The two upstream test files (lspRecommendation.test.ts and
officialMarketplaceStartupCheck.test.ts) had stale manual mock
lists that were missing recent exports (e.g.
`normalizeMaxMessagesCompactionThreshold` added in PR Twigpine#1605,
`logAntError`, `logMCPError`, `isSourceInBlocklist`, etc.). These
missing entries broke transitive imports of marketplaceManager →
config / debug / log / marketplaceHelpers. Switch their mocks to the
`...await import('...real=...')` spread pattern already used for
growthbook (line 41). This preserves all real exports while
overriding the test-specific ones — robust to future additions.
The same comment in the static-import block documents the technique
so future readers understand why a query string is part of the URL.
Verified locally: `bun test --max-concurrency=1 ./src/utils/plugins/`
runs all 37 tests across 5 files (lspRecommendation,
marketplaceManager, officialMarketplaceStartupCheck, pluginLoader,
gitAvailability) with 0 failures, regardless of file order. Each
test file in isolation also passes.
kevincodex1
pushed a commit
that referenced
this pull request
Jun 14, 2026
#1500) (#1531) * fix(plugins): prevent ENOENT on Windows marketplace cache finalization (#1500) * test(plugins): add regression test for Windows marketplace cache finalization (#1500) Add regression tests covering the case-only path comparison bug where fs.rm would destroy source data when temp and final cache paths differ only in case on case-insensitive filesystems (Windows NTFS). Three test cases: 1. Generic mixed-case name (MyMarketplace/mymarketplace) — verifies the samePathCaseInsensitive guard skips rm+rename 2. GitHub-style naming (AgriciDaniel-claude-obsidian) — explicitly models the exact #1500 bug report scenario 3. Already-lowercase name (claude-obsidian) — verifies the string-equality fast path that GitHub sources hit post-fix Also exports loadAndCacheMarketplace from _test for testability. * test: add rename-failure fallback regression test for EXDEV Add a regression test that forces the rename-failure fallback path in loadAndCacheMarketplace. Uses a 'url' source with a mocked axios response so the temp cache path (timestamp-based) truly differs from the final cache path (marketplace.name.toLowerCase()), bypassing the samePathCaseInsensitive guard. The test stubs rename to throw EXDEV and verifies the cp+rm fallback correctly copies data to the final location, cleans up the temporary path, and returns the final cache path. * test: fix brittle temp-cleanup assertion in marketplace fallback test Replace fragile rmSpy.call filter with direct readdirSync(cacheDir) to verify the real temp directory state. Fixes CI failure at line 316. * test: use pre-call snapshot for platform-agnostic fallback assertion Replace brittle readdirSync length check with delta comparison against pre-call directory state. Works on both case-sensitive (Linux) and case-insensitive (Windows) filesystems. * test: isolate marketplace mocks and fix cross-platform temp file assertion - Move axios mock inside describe with beforeAll/afterAll lifecycle - Filter temp_*.json files from directory delta comparison - Add mock isolation to prevent leakage to other test files * test(marketplace): assert renameSpy called and no temp files linger after EXDEV fallback * test(marketplace): add cp spy and isolate axios mock to test lifecycle Address outstanding review gaps for #1531: 1. The rename-failure fallback (cp + rm) is now explicitly verified via a cpSpy on the real fs/promises.cp. The mock.module('fs/promises', ...) registration in beforeAll wraps cp with a call-through spy, so the test exercises the actual filesystem fallback while still asserting the call args (temp source, finalCachePath dest, { recursive: true } option). Previously, the test only proved the final file existed, which could be reached by a different code path or by leftover pre-test state. 2. The axios mock is now isolated to the test lifecycle: the axiosGetSpy is created in beforeEach (with a fresh implementation) instead of at module scope, and the mock.module('axios', ...) factory uses a closure indirection so the spy can be swapped per test. This prevents call counts and implementations from leaking across tests in this suite or into other suites. 3. cpSpy.mockClear() runs in beforeEach to reset .mock.calls / .mock.results between tests while preserving the call-through behavior of the persistent mock.module() registration. Skipped: the P1 about finalCachePath lowercasing the cache directory (marketplaceManager.ts:1712). The current code already preserves cacheDir and lowercases only marketplace.name, so the original review concern was already addressed by an earlier commit. Skipped: temp path detection via rmSpy filter and explicit temp_* cleanup verification. Both are already covered by the existing afterEntries/beforeEntries snapshot comparison (L334-377) and the lingeringTempFiles assertion (L380-385). * Revert "test(marketplace): add cp spy and isolate axios mock to test lifecycle" This reverts commit c57ad55. * test(marketplace): force real module + drop unused source.name Two pre-existing issues that blocked typecheck and caused 4 marketplace tests to fail in the full suite: 1. mock.module('./marketplaceManager.js', async (original) => ...) called original() to 'force the real module' but original() returns the previously-registered factory's output, which is a stale mock from a prior test file (e.g. officialMarketplaceStartupCheck.test.ts or lspRecommendation.test.ts). The stale mock has its own env-var behavior, so getMarketplacesCacheDir() returned the leaked /tmp/openclaude-marketplaces instead of the test's tempDir. The real module reads process.env.CLAUDE_CODE_PLUGIN_CACHE_DIR live on every call, so the beforeEach override takes effect immediately. Forcing the real module makes the env-var-driven cache path work correctly. The factory now returns undefined so Bun uses the real module regardless of any previously-registered mock. 2. The EXDEV test's url source had a 'name' field that does not exist in the MarketplaceSource.url variant of the schema (only url and optional headers are allowed). The field was unused — the test hardcodes finalCachePath from the marketplace manifest name (lowercased), not from source.name. Drop the field; typecheck goes green. * test(marketplace): use mock.restore() to clear stale marketplaceManager mock Previous attempt used mock.module(path, () => undefined) but Bun rejects that — TypeError: 'mock(module, fn) requires a function that returns an object'. mock.module() with no factory isn't a documented API, so switch to mock.restore() which clears all module mocks. After mock.restore(), the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore() also cleared), so the suite-specific mock still works. Skipped: spying on cp via setFsImplementation. The FsOperations interface (src/utils/fsOperations.ts L23-72) has no cp method, so adding cp: cpSpy to setFsImplementation would fail typecheck. The rename-failure fallback is still verified via filesystem state assertions: before/after readdirSync snapshot plus the explicit lingeringTempFiles === [] check. * Revert "test(marketplace): use mock.restore() to clear stale marketplaceManager mock" This reverts commit bbbebfb. * test(marketplace): fix mock.module TypeError (factory must return object) 0c917a1 used () => undefined to force the real module, but Bun rejects factories that don't return an object ('mock(module, fn) requires a function that returns an object'). Switch to mock.restore() at the top of the file — clears all module mocks registered by prior test files so the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore also cleared), so suite-specific mocks still work. The env-var-leak bug (getMarketplacesCacheDir returning the leaked /tmp/openclaude-marketplaces instead of the test's tempDir) is a deeper issue with how process.env is read in Bun's test runner. Pre-existing on origin/main and out of scope for this PR; documented in the PR body. * test(marketplace): drop mock.restore() to let natural module resolution work CodeRabbit round 8 P2: the previous implementation called mock.restore() at file-scope on the assumption it would clear module mocks registered by other test files (lspRecommendation.test.ts, officialMarketplaceStartupCheck.test.ts). Bun's docs confirm mock.restore() does NOT clear mock.module() registrations, so the partial mock from the prior file persisted and the import of _test broke ("Export named '_test' not found"). The reproducer is real: running lspRecommendation.test.ts first then this file fails. But the root cause is a pre-existing bug in lspRecommendation.test.ts (its mock for '../config.js' is missing normalizeMaxMessagesCompactionThreshold, added to config.ts in PR #1605 / commit 2002e4c). That lspRecommendation test fails on its own on origin/main, and was already failing the lspRecommendation test before the marketplaceManager test even ran. Drop the mock.restore() call so the test's import relies on Bun's natural module resolution. The marketplaceManager test passes in isolation. The full-suite failure is caused by the pre-existing lspRecommendation bug, not by this test. Fixing that is out of scope for #1531. * test(marketplace): import real marketplaceManager in full suite via cache-busted URL CodeRabbit round 8 follow-up + jatmn round 8 P2: the test file's static `import { _test } from './marketplaceManager.js'` resolves to whichever mock was registered first for the bare path. Two other test files register partial mocks at module top-level: - lspRecommendation.test.ts:32 mocks the path with 9 exports (no _test) - officialMarketplaceStartupCheck.test.ts:96 mocks it with 9 exports (no _test) Whichever runs first wins, and downstream test files re-importing the same bare path inherit that partial mock — yielding "Export named '_test' not found in module './marketplaceManager.js'" and 4 failing tests in the full suite. The fix uses three techniques: 1. The static import at the top of marketplaceManager.test.ts is changed to `'./marketplaceManager.js?bust=this-test-needs-the-real-module'`. Bun treats the query-string suffix as a distinct module id, so the import bypasses the bare-path mock.module() registration in lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts. The constant suffix is sufficient because we only need the import to happen once at module top-level — no per-test re-evaluation is required for the cache-sharing test block. 2. The EXDEV describe block's dynamic re-import (line 263) was already using a non-busted URL `'./marketplaceManager.ts'` to pick up the mocked axios. It gets the partial mock from the other test files instead, with the same `_test` is undefined failure. Apply the same `?bust=exdev-test-reimport` suffix so the re-import is treated as a fresh module id and still picks up the axios mock (because `'axios'` is a different module id than the busted marketplace URL). 3. The two upstream test files (lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts) had stale manual mock lists that were missing recent exports (e.g. `normalizeMaxMessagesCompactionThreshold` added in PR #1605, `logAntError`, `logMCPError`, `isSourceInBlocklist`, etc.). These missing entries broke transitive imports of marketplaceManager → config / debug / log / marketplaceHelpers. Switch their mocks to the `...await import('...real=...')` spread pattern already used for growthbook (line 41). This preserves all real exports while overriding the test-specific ones — robust to future additions. The same comment in the static-import block documents the technique so future readers understand why a query string is part of the URL. Verified locally: `bun test --max-concurrency=1 ./src/utils/plugins/` runs all 37 tests across 5 files (lspRecommendation, marketplaceManager, officialMarketplaceStartupCheck, pluginLoader, gitAvailability) with 0 failures, regardless of file order. Each test file in isolation also passes. * test(marketplace): make typecheckable via @ts-expect-error + template literal jatmn round 13 P2: the cache-busted imports from cc6c184 (commit) were valid runtime under Bun but TypeScript couldn't resolve them, breaking `tsc --noEmit`. Fix by combining two patterns already in the codebase: 1. **Static import with `// @ts-expect-error`**: the top-of-file import uses the `?bust=this-test-needs-the-real-module` query string. The `// @ts-expect-error` directive above the import statement suppresses the TS2307 ("Cannot find module"). Pattern from `src/hooks/useApiKeyVerification.test.tsx:104`. 2. **Dynamic import with template literal + `as typeof` cast**: the EXDEV describe block's re-import (which must happen after `mock.module('axios', ...)` is set up) uses a template literal `` `./marketplaceManager.ts?bust=exdev-test-reimport-${Date.now()}` `` so TypeScript treats the specifier as a dynamic `string` (not a literal that it tries to resolve at compile time). The `as typeof import('./marketplaceManager.js')` cast then re-types the result. Pattern from `src/utils/hookChains.integration.test.ts:43-50`. Also added `!` non-null assertions on the cached module-level `loadAndCacheMarketplace` reference to silence the "possibly undefined" warnings that arose when the variable was declared without an initializer. Verified: - `bun x tsc --noEmit --ignoreDeprecations 6.0` against src/utils/plugins/marketplaceManager.test.ts: no errors except the pre-existing `bun:test` ambient module declaration (which is the same error all *.test.ts files in the repo show). - `bun test --max-concurrency=1 src/utils/plugins/marketplaceManager.test.ts`: 4/4 pass. - `bun test --max-concurrency=1` of the three test files that share marketplaceManager.js (lspRecommendation, officialMarketplaceStartupCheck, marketplaceManager): 11/11 pass. - The 16 failures in the broader smoke run are pre-existing on origin/main (GlobalConfig — showCacheStats, provider-specific cap tests for deepseek/gpt-4o/MiniMax-M2.7/etc., all added by the recent Fireworks AI provider upstream commit). Not introduced by this PR. * debug(marketplace): log EXDEV rm/renames * test(marketplace): use rmSync instead of mocked fs/promises rm for EXDEV fallback openclaudeInstallSurfaces.test.ts mocks fs/promises with a no-op rm. The EXDEV test's rm wrapper delegated to NodeFsOperations.rm which used the mocked rmPromise, leaving temp_*.json artifacts. Use the synchronous rmSync from 'fs' (not mocked) instead. * fix(plugins): gate marketplace cache path comparisons on FS case-sensitivity The cache finalization and cleanup paths compared full paths with an unconditional .toLowerCase(), so on case-sensitive filesystems (Linux) two distinct cache entries differing only in case were treated as the same directory. This skipped the rename in loadAndCacheMarketplace, skipped stale-cache cleanup in addMarketplaceSource, and made removeMarketplaceSource delete a recomputed lowercase path that misses a mixed-case cache dir. Add isCaseInsensitiveFs()/pathsEqualForFs() helpers and gate both comparisons on them, and delete the recorded installLocation in removeMarketplaceSource instead of a recomputed path. Pins the existing #1500 tests to win32 (the case-insensitive scenario they model) so they stay deterministic on the case-sensitive Linux CI runner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(plugins): verify EXDEV cp+rm fallback via an injectable fs.cp spy The marketplace cache finalization fallback called cp() via the direct fs/promises import, so it could not be observed through the swappable fs implementation that rm/rename already use. The EXDEV regression test could only assert end state, not that the cp+rm fallback branch ran. Add cp to FsOperations/NodeFsOperations and route the finalization fallback through fs.cp, then spy on it in the EXDEV test to assert the copy ran exactly once with temp -> final and { recursive: true }. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(plugins): probe cache volume case-sensitivity instead of assuming by platform isCaseInsensitiveFs() assumed every darwin process runs on a case-insensitive volume, but macOS can mount case-sensitive APFS/HFS+. On those volumes two cache paths differing only in case are distinct, so pathsEqualForFs() wrongly reported them equal and addMarketplaceSource() skipped cleanup of the old mixed-case cache dir, orphaning it. Replace the platform heuristic with isCaseInsensitiveFsAt(dir): Windows is always case-insensitive (and has unreliable inode numbers) so trust the platform there; elsewhere stat the directory under a case-flipped name and treat the volume as case-insensitive only when both names resolve to the same inode/dev. Memoized per directory, fails safe to case-sensitive. pathsEqualForFs() now takes the cache dir to probe. Tests inject statSync to simulate both volume kinds, so they're portable (no real case-sensitive mount required). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
deagwon97
pushed a commit
to deagwon97/openclaude
that referenced
this pull request
Jun 15, 2026
* feat(ui): OpenClaude brand identity — orange accent, wordmark, dot-pulse spinner Give OpenClaude a real visual identity instead of reskinned upstream visuals: - theme.ts: add brand/brandShimmer keys and repoint the claude family (claude, claudeShimmer, clawd_body, briefLabelClaude) to the gitlawb orange #ff7a1a across all 6 themes — darkened variants for light backgrounds, luminance-separated variants for daltonized themes, redBright floor for 16-color ANSI themes. Values stay rgb() strings (parseRGB in Spinner/utils.ts silently fails on hex). - brand.ts (new): BRAND_NAME, BRAND_TAGLINE, accent constant, and a 2-row Unicode half-block OPENCLAUDE wordmark split for two-tone render. - LogoV2: full logo shows the wordmark + centralized tagline; CondensedLogo drops the duplicated OPEN CLAUDE header and renders a brand-colored name+version line; Clawd mascot body now clawd_body. - Startup splash: new 'ember' gradient palette (brand orange) as the default (/logo keeps sunset et al.); tagline centralized from brand.ts. - Spinner: dot-pulse glyph sweep (· ∘ ○ ◎ ◉ ●) replacing the asterisk set (drops Ghostty/darwin special-cases); defaults switched to brand/brandShimmer. Reduced-motion and stall interpolation unchanged. - figures.ts: TEARDROP_ASTERISK marker → ◎ to match the new glyph family. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * feat(ui): built-in default statusline Ship a status bar that renders when no custom /statusline command is configured: `model · ctx % · session cost · rate limit`. Previously this slot was empty unless the user wired up their own shell command. - BuiltinStatusLine.tsx (new): pure segment builders (buildBuiltinStatusSegments / fitSegments — drops lowest-priority segments right-to-left as the terminal narrows) plus a memoized component reusing the exact data pipeline the custom StatusLine feeds to user commands (getRuntimeMainLoopModel, getCurrentUsage, calculateContextPercentages, getTotalCost, getRawUtilization). Pure in-process computation — no subprocess, no debounce. - Context % colors warning ≥70 / error ≥90 (aligned with auto-compact warnings); rate limit shows the worst of the 5h/7d windows and is absent (not 0%) for API-key users; cost hidden at $0. - PromptInputFooter: custom statusline always wins; the built-in bar also suppresses the "? for shortcuts" hint like the custom one does. - defaultStatusLineEnabled global-config toggle (default on) surfaced in /config settings. - Unit tests for segment building, narrow-width fitting, threshold colors, and custom-statusline precedence. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * feat(ui): polish pass — fuzzy match highlighting, completion flash, diff gutter, dialog hints - Fuzzy pickers: new highlightFuzzyMatch (contiguous-run or greedy- subsequence, bold so the focused row's color survives) applied to Quick Open and prompt-history results. FuzzyPicker now passes the live query as a third renderItem argument so memoized render callbacks can't capture a stale query. - Select: inline option descriptions now truncate with an ellipsis on narrow terminals instead of clipping at the edge (row made shrinkable, description wrapped in truncate-end). - CompletionFlash (new): static `✓ Done · 12s` row for ~1.5s after a response completes. Keys off the isLoading transition (the spinner also hides mid-turn for streaming text); same row footprint as the spinner; suppressed for sub-second turns, brief mode, open permission/prompt queues, running teammates, and reduced motion. - Diff gutter: line numbers on +/- lines are now dimmed (previously full decoration intensity competed with content, worst on light themes); +/- sigils keep full intensity. Mirrored in the non-highlighted Fallback renderer. - Dialog: optional showNavigationHint prepends "↑/↓ navigate" to the default input guide; opted in from Select-hosting dialogs (IdleReturn, DevChannels, WorktreeExit). Dialog rewritten as plain React (was react-compiler output) to take the new prop safely. - Spinner: the ↑/↓ request-direction glyph now leads the status parens whenever any status shows — previously buried in the tokens part, which only appears after 30s; width gating reserves its space. - Replaced the stale `/`-search TODO in ScrollKeybindingHandler with a pointer to the shipped REPL transcript search. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * fix(ui): address review — explicit statusline default, narrow-terminal thinking gate - createDefaultGlobalConfig now sets defaultStatusLineEnabled: true explicitly, matching the factory's other default-true booleans (read sites already coalesced to true; no behavior change). - SpinnerAnimationRow: the width gate reserves mode-glyph space that a thinking-only spin never uses; on narrow terminals where nothing else renders, re-try the thinking gate with that space returned so '(thinking)' shows instead of nothing. CompletionFlash NaN guard was reviewed and not applied: both refs are useRef(0) — initialized numbers by type and construction — and the active→inactive transition guard guarantees the start time was set before the elapsed math runs. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * fix(ui): address review round 2 — hint accuracy, flash suppression, coverage - Dialog: the cancel shortcut hint now renders only when isCancelActive, matching the keybinding it advertises. - PromptInputFooter: new exported resolveFooterStatusLine(settings, guards, config?) is the single source of truth for which status line renders — custom wins over builtin, and all render guards (prompt mode, short fullscreen, exit message, pasting) force null. suppressHint and the render now share it, so '? for shortcuts' is suppressed only when a status line actually renders. - CompletionFlash: suppression now clears an already-active flash (the effect resets state and the render guard skips the commit-gap frame) instead of only preventing new ones. - REPL: flash suppression reuses the existing hasActivePrompt aggregate, which covers the sandbox, worker-sandbox, and elicitation queues the hand-rolled expression missed. - builtinStatusLineShouldDisplay takes an injectable config (defaults to getGlobalConfig()) so the config-off path is testable without module mocks; tests added for config-off, custom-wins-regardless, and the resolver's full variant x guard matrix. Validation: typecheck exit 0; full suite 3733 green; smoke + bundle guard green. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> * fix(ui): drop fit-budget floor, cover reduced-motion render gap - BuiltinStatusLine: remove the artificial 10-column floor on the fit budget — fitSegments returning [] is the signal that nothing fits, and the empty branch already handles it (row-reserve in fullscreen, null otherwise). The floor forced a truncated segment onto panes narrower than the model name. - CompletionFlash: the render guard now also skips the one frame between reducedMotion flipping on and the effect clearing the flash, same as the suppressed case. Component-level mount tests for PromptInputFooter were considered and deliberately not added: the footer needs 10+ mocked contexts to mount, mock.module harnesses leak across bun test files in this repo, and the branching under review is exactly the pure resolveFooterStatusLine contract already pinned by tests. Validation: typecheck exit 0; full suite 3733 green; smoke + bundle guard green. Co-Authored-By: OpenClaude <openclaude@gitlawb.com> --------- Co-authored-by: OpenClaude <openclaude@gitlawb.com>
deagwon97
pushed a commit
to deagwon97/openclaude
that referenced
this pull request
Jun 15, 2026
Twigpine#1500) (Twigpine#1531) * fix(plugins): prevent ENOENT on Windows marketplace cache finalization (Twigpine#1500) * test(plugins): add regression test for Windows marketplace cache finalization (Twigpine#1500) Add regression tests covering the case-only path comparison bug where fs.rm would destroy source data when temp and final cache paths differ only in case on case-insensitive filesystems (Windows NTFS). Three test cases: 1. Generic mixed-case name (MyMarketplace/mymarketplace) — verifies the samePathCaseInsensitive guard skips rm+rename 2. GitHub-style naming (AgriciDaniel-claude-obsidian) — explicitly models the exact Twigpine#1500 bug report scenario 3. Already-lowercase name (claude-obsidian) — verifies the string-equality fast path that GitHub sources hit post-fix Also exports loadAndCacheMarketplace from _test for testability. * test: add rename-failure fallback regression test for EXDEV Add a regression test that forces the rename-failure fallback path in loadAndCacheMarketplace. Uses a 'url' source with a mocked axios response so the temp cache path (timestamp-based) truly differs from the final cache path (marketplace.name.toLowerCase()), bypassing the samePathCaseInsensitive guard. The test stubs rename to throw EXDEV and verifies the cp+rm fallback correctly copies data to the final location, cleans up the temporary path, and returns the final cache path. * test: fix brittle temp-cleanup assertion in marketplace fallback test Replace fragile rmSpy.call filter with direct readdirSync(cacheDir) to verify the real temp directory state. Fixes CI failure at line 316. * test: use pre-call snapshot for platform-agnostic fallback assertion Replace brittle readdirSync length check with delta comparison against pre-call directory state. Works on both case-sensitive (Linux) and case-insensitive (Windows) filesystems. * test: isolate marketplace mocks and fix cross-platform temp file assertion - Move axios mock inside describe with beforeAll/afterAll lifecycle - Filter temp_*.json files from directory delta comparison - Add mock isolation to prevent leakage to other test files * test(marketplace): assert renameSpy called and no temp files linger after EXDEV fallback * test(marketplace): add cp spy and isolate axios mock to test lifecycle Address outstanding review gaps for Twigpine#1531: 1. The rename-failure fallback (cp + rm) is now explicitly verified via a cpSpy on the real fs/promises.cp. The mock.module('fs/promises', ...) registration in beforeAll wraps cp with a call-through spy, so the test exercises the actual filesystem fallback while still asserting the call args (temp source, finalCachePath dest, { recursive: true } option). Previously, the test only proved the final file existed, which could be reached by a different code path or by leftover pre-test state. 2. The axios mock is now isolated to the test lifecycle: the axiosGetSpy is created in beforeEach (with a fresh implementation) instead of at module scope, and the mock.module('axios', ...) factory uses a closure indirection so the spy can be swapped per test. This prevents call counts and implementations from leaking across tests in this suite or into other suites. 3. cpSpy.mockClear() runs in beforeEach to reset .mock.calls / .mock.results between tests while preserving the call-through behavior of the persistent mock.module() registration. Skipped: the P1 about finalCachePath lowercasing the cache directory (marketplaceManager.ts:1712). The current code already preserves cacheDir and lowercases only marketplace.name, so the original review concern was already addressed by an earlier commit. Skipped: temp path detection via rmSpy filter and explicit temp_* cleanup verification. Both are already covered by the existing afterEntries/beforeEntries snapshot comparison (L334-377) and the lingeringTempFiles assertion (L380-385). * Revert "test(marketplace): add cp spy and isolate axios mock to test lifecycle" This reverts commit c57ad55. * test(marketplace): force real module + drop unused source.name Two pre-existing issues that blocked typecheck and caused 4 marketplace tests to fail in the full suite: 1. mock.module('./marketplaceManager.js', async (original) => ...) called original() to 'force the real module' but original() returns the previously-registered factory's output, which is a stale mock from a prior test file (e.g. officialMarketplaceStartupCheck.test.ts or lspRecommendation.test.ts). The stale mock has its own env-var behavior, so getMarketplacesCacheDir() returned the leaked /tmp/openclaude-marketplaces instead of the test's tempDir. The real module reads process.env.CLAUDE_CODE_PLUGIN_CACHE_DIR live on every call, so the beforeEach override takes effect immediately. Forcing the real module makes the env-var-driven cache path work correctly. The factory now returns undefined so Bun uses the real module regardless of any previously-registered mock. 2. The EXDEV test's url source had a 'name' field that does not exist in the MarketplaceSource.url variant of the schema (only url and optional headers are allowed). The field was unused — the test hardcodes finalCachePath from the marketplace manifest name (lowercased), not from source.name. Drop the field; typecheck goes green. * test(marketplace): use mock.restore() to clear stale marketplaceManager mock Previous attempt used mock.module(path, () => undefined) but Bun rejects that — TypeError: 'mock(module, fn) requires a function that returns an object'. mock.module() with no factory isn't a documented API, so switch to mock.restore() which clears all module mocks. After mock.restore(), the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore() also cleared), so the suite-specific mock still works. Skipped: spying on cp via setFsImplementation. The FsOperations interface (src/utils/fsOperations.ts L23-72) has no cp method, so adding cp: cpSpy to setFsImplementation would fail typecheck. The rename-failure fallback is still verified via filesystem state assertions: before/after readdirSync snapshot plus the explicit lingeringTempFiles === [] check. * Revert "test(marketplace): use mock.restore() to clear stale marketplaceManager mock" This reverts commit bbbebfb. * test(marketplace): fix mock.module TypeError (factory must return object) 0c917a1 used () => undefined to force the real module, but Bun rejects factories that don't return an object ('mock(module, fn) requires a function that returns an object'). Switch to mock.restore() at the top of the file — clears all module mocks registered by prior test files so the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore also cleared), so suite-specific mocks still work. The env-var-leak bug (getMarketplacesCacheDir returning the leaked /tmp/openclaude-marketplaces instead of the test's tempDir) is a deeper issue with how process.env is read in Bun's test runner. Pre-existing on origin/main and out of scope for this PR; documented in the PR body. * test(marketplace): drop mock.restore() to let natural module resolution work CodeRabbit round 8 P2: the previous implementation called mock.restore() at file-scope on the assumption it would clear module mocks registered by other test files (lspRecommendation.test.ts, officialMarketplaceStartupCheck.test.ts). Bun's docs confirm mock.restore() does NOT clear mock.module() registrations, so the partial mock from the prior file persisted and the import of _test broke ("Export named '_test' not found"). The reproducer is real: running lspRecommendation.test.ts first then this file fails. But the root cause is a pre-existing bug in lspRecommendation.test.ts (its mock for '../config.js' is missing normalizeMaxMessagesCompactionThreshold, added to config.ts in PR Twigpine#1605 / commit 2002e4c). That lspRecommendation test fails on its own on origin/main, and was already failing the lspRecommendation test before the marketplaceManager test even ran. Drop the mock.restore() call so the test's import relies on Bun's natural module resolution. The marketplaceManager test passes in isolation. The full-suite failure is caused by the pre-existing lspRecommendation bug, not by this test. Fixing that is out of scope for Twigpine#1531. * test(marketplace): import real marketplaceManager in full suite via cache-busted URL CodeRabbit round 8 follow-up + jatmn round 8 P2: the test file's static `import { _test } from './marketplaceManager.js'` resolves to whichever mock was registered first for the bare path. Two other test files register partial mocks at module top-level: - lspRecommendation.test.ts:32 mocks the path with 9 exports (no _test) - officialMarketplaceStartupCheck.test.ts:96 mocks it with 9 exports (no _test) Whichever runs first wins, and downstream test files re-importing the same bare path inherit that partial mock — yielding "Export named '_test' not found in module './marketplaceManager.js'" and 4 failing tests in the full suite. The fix uses three techniques: 1. The static import at the top of marketplaceManager.test.ts is changed to `'./marketplaceManager.js?bust=this-test-needs-the-real-module'`. Bun treats the query-string suffix as a distinct module id, so the import bypasses the bare-path mock.module() registration in lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts. The constant suffix is sufficient because we only need the import to happen once at module top-level — no per-test re-evaluation is required for the cache-sharing test block. 2. The EXDEV describe block's dynamic re-import (line 263) was already using a non-busted URL `'./marketplaceManager.ts'` to pick up the mocked axios. It gets the partial mock from the other test files instead, with the same `_test` is undefined failure. Apply the same `?bust=exdev-test-reimport` suffix so the re-import is treated as a fresh module id and still picks up the axios mock (because `'axios'` is a different module id than the busted marketplace URL). 3. The two upstream test files (lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts) had stale manual mock lists that were missing recent exports (e.g. `normalizeMaxMessagesCompactionThreshold` added in PR Twigpine#1605, `logAntError`, `logMCPError`, `isSourceInBlocklist`, etc.). These missing entries broke transitive imports of marketplaceManager → config / debug / log / marketplaceHelpers. Switch their mocks to the `...await import('...real=...')` spread pattern already used for growthbook (line 41). This preserves all real exports while overriding the test-specific ones — robust to future additions. The same comment in the static-import block documents the technique so future readers understand why a query string is part of the URL. Verified locally: `bun test --max-concurrency=1 ./src/utils/plugins/` runs all 37 tests across 5 files (lspRecommendation, marketplaceManager, officialMarketplaceStartupCheck, pluginLoader, gitAvailability) with 0 failures, regardless of file order. Each test file in isolation also passes. * test(marketplace): make typecheckable via @ts-expect-error + template literal jatmn round 13 P2: the cache-busted imports from cc6c184 (commit) were valid runtime under Bun but TypeScript couldn't resolve them, breaking `tsc --noEmit`. Fix by combining two patterns already in the codebase: 1. **Static import with `// @ts-expect-error`**: the top-of-file import uses the `?bust=this-test-needs-the-real-module` query string. The `// @ts-expect-error` directive above the import statement suppresses the TS2307 ("Cannot find module"). Pattern from `src/hooks/useApiKeyVerification.test.tsx:104`. 2. **Dynamic import with template literal + `as typeof` cast**: the EXDEV describe block's re-import (which must happen after `mock.module('axios', ...)` is set up) uses a template literal `` `./marketplaceManager.ts?bust=exdev-test-reimport-${Date.now()}` `` so TypeScript treats the specifier as a dynamic `string` (not a literal that it tries to resolve at compile time). The `as typeof import('./marketplaceManager.js')` cast then re-types the result. Pattern from `src/utils/hookChains.integration.test.ts:43-50`. Also added `!` non-null assertions on the cached module-level `loadAndCacheMarketplace` reference to silence the "possibly undefined" warnings that arose when the variable was declared without an initializer. Verified: - `bun x tsc --noEmit --ignoreDeprecations 6.0` against src/utils/plugins/marketplaceManager.test.ts: no errors except the pre-existing `bun:test` ambient module declaration (which is the same error all *.test.ts files in the repo show). - `bun test --max-concurrency=1 src/utils/plugins/marketplaceManager.test.ts`: 4/4 pass. - `bun test --max-concurrency=1` of the three test files that share marketplaceManager.js (lspRecommendation, officialMarketplaceStartupCheck, marketplaceManager): 11/11 pass. - The 16 failures in the broader smoke run are pre-existing on origin/main (GlobalConfig — showCacheStats, provider-specific cap tests for deepseek/gpt-4o/MiniMax-M2.7/etc., all added by the recent Fireworks AI provider upstream commit). Not introduced by this PR. * debug(marketplace): log EXDEV rm/renames * test(marketplace): use rmSync instead of mocked fs/promises rm for EXDEV fallback openclaudeInstallSurfaces.test.ts mocks fs/promises with a no-op rm. The EXDEV test's rm wrapper delegated to NodeFsOperations.rm which used the mocked rmPromise, leaving temp_*.json artifacts. Use the synchronous rmSync from 'fs' (not mocked) instead. * fix(plugins): gate marketplace cache path comparisons on FS case-sensitivity The cache finalization and cleanup paths compared full paths with an unconditional .toLowerCase(), so on case-sensitive filesystems (Linux) two distinct cache entries differing only in case were treated as the same directory. This skipped the rename in loadAndCacheMarketplace, skipped stale-cache cleanup in addMarketplaceSource, and made removeMarketplaceSource delete a recomputed lowercase path that misses a mixed-case cache dir. Add isCaseInsensitiveFs()/pathsEqualForFs() helpers and gate both comparisons on them, and delete the recorded installLocation in removeMarketplaceSource instead of a recomputed path. Pins the existing Twigpine#1500 tests to win32 (the case-insensitive scenario they model) so they stay deterministic on the case-sensitive Linux CI runner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(plugins): verify EXDEV cp+rm fallback via an injectable fs.cp spy The marketplace cache finalization fallback called cp() via the direct fs/promises import, so it could not be observed through the swappable fs implementation that rm/rename already use. The EXDEV regression test could only assert end state, not that the cp+rm fallback branch ran. Add cp to FsOperations/NodeFsOperations and route the finalization fallback through fs.cp, then spy on it in the EXDEV test to assert the copy ran exactly once with temp -> final and { recursive: true }. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(plugins): probe cache volume case-sensitivity instead of assuming by platform isCaseInsensitiveFs() assumed every darwin process runs on a case-insensitive volume, but macOS can mount case-sensitive APFS/HFS+. On those volumes two cache paths differing only in case are distinct, so pathsEqualForFs() wrongly reported them equal and addMarketplaceSource() skipped cleanup of the old mixed-case cache dir, orphaning it. Replace the platform heuristic with isCaseInsensitiveFsAt(dir): Windows is always case-insensitive (and has unreliable inode numbers) so trust the platform there; elsewhere stat the directory under a case-flipped name and treat the volume as case-insensitive only when both names resolve to the same inode/dev. Memoized per directory, fails safe to case-sensitive. pathsEqualForFs() now takes the cache dir to probe. Tests inject statSync to simulate both volume kinds, so they're portable (no real case-sensitive mount required). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
hotmanxp
pushed a commit
to hotmanxp/openclaude
that referenced
this pull request
Jun 15, 2026
Twigpine#1500) (Twigpine#1531) * fix(plugins): prevent ENOENT on Windows marketplace cache finalization (Twigpine#1500) * test(plugins): add regression test for Windows marketplace cache finalization (Twigpine#1500) Add regression tests covering the case-only path comparison bug where fs.rm would destroy source data when temp and final cache paths differ only in case on case-insensitive filesystems (Windows NTFS). Three test cases: 1. Generic mixed-case name (MyMarketplace/mymarketplace) — verifies the samePathCaseInsensitive guard skips rm+rename 2. GitHub-style naming (AgriciDaniel-claude-obsidian) — explicitly models the exact Twigpine#1500 bug report scenario 3. Already-lowercase name (claude-obsidian) — verifies the string-equality fast path that GitHub sources hit post-fix Also exports loadAndCacheMarketplace from _test for testability. * test: add rename-failure fallback regression test for EXDEV Add a regression test that forces the rename-failure fallback path in loadAndCacheMarketplace. Uses a 'url' source with a mocked axios response so the temp cache path (timestamp-based) truly differs from the final cache path (marketplace.name.toLowerCase()), bypassing the samePathCaseInsensitive guard. The test stubs rename to throw EXDEV and verifies the cp+rm fallback correctly copies data to the final location, cleans up the temporary path, and returns the final cache path. * test: fix brittle temp-cleanup assertion in marketplace fallback test Replace fragile rmSpy.call filter with direct readdirSync(cacheDir) to verify the real temp directory state. Fixes CI failure at line 316. * test: use pre-call snapshot for platform-agnostic fallback assertion Replace brittle readdirSync length check with delta comparison against pre-call directory state. Works on both case-sensitive (Linux) and case-insensitive (Windows) filesystems. * test: isolate marketplace mocks and fix cross-platform temp file assertion - Move axios mock inside describe with beforeAll/afterAll lifecycle - Filter temp_*.json files from directory delta comparison - Add mock isolation to prevent leakage to other test files * test(marketplace): assert renameSpy called and no temp files linger after EXDEV fallback * test(marketplace): add cp spy and isolate axios mock to test lifecycle Address outstanding review gaps for Twigpine#1531: 1. The rename-failure fallback (cp + rm) is now explicitly verified via a cpSpy on the real fs/promises.cp. The mock.module('fs/promises', ...) registration in beforeAll wraps cp with a call-through spy, so the test exercises the actual filesystem fallback while still asserting the call args (temp source, finalCachePath dest, { recursive: true } option). Previously, the test only proved the final file existed, which could be reached by a different code path or by leftover pre-test state. 2. The axios mock is now isolated to the test lifecycle: the axiosGetSpy is created in beforeEach (with a fresh implementation) instead of at module scope, and the mock.module('axios', ...) factory uses a closure indirection so the spy can be swapped per test. This prevents call counts and implementations from leaking across tests in this suite or into other suites. 3. cpSpy.mockClear() runs in beforeEach to reset .mock.calls / .mock.results between tests while preserving the call-through behavior of the persistent mock.module() registration. Skipped: the P1 about finalCachePath lowercasing the cache directory (marketplaceManager.ts:1712). The current code already preserves cacheDir and lowercases only marketplace.name, so the original review concern was already addressed by an earlier commit. Skipped: temp path detection via rmSpy filter and explicit temp_* cleanup verification. Both are already covered by the existing afterEntries/beforeEntries snapshot comparison (L334-377) and the lingeringTempFiles assertion (L380-385). * Revert "test(marketplace): add cp spy and isolate axios mock to test lifecycle" This reverts commit c57ad55. * test(marketplace): force real module + drop unused source.name Two pre-existing issues that blocked typecheck and caused 4 marketplace tests to fail in the full suite: 1. mock.module('./marketplaceManager.js', async (original) => ...) called original() to 'force the real module' but original() returns the previously-registered factory's output, which is a stale mock from a prior test file (e.g. officialMarketplaceStartupCheck.test.ts or lspRecommendation.test.ts). The stale mock has its own env-var behavior, so getMarketplacesCacheDir() returned the leaked /tmp/openclaude-marketplaces instead of the test's tempDir. The real module reads process.env.CLAUDE_CODE_PLUGIN_CACHE_DIR live on every call, so the beforeEach override takes effect immediately. Forcing the real module makes the env-var-driven cache path work correctly. The factory now returns undefined so Bun uses the real module regardless of any previously-registered mock. 2. The EXDEV test's url source had a 'name' field that does not exist in the MarketplaceSource.url variant of the schema (only url and optional headers are allowed). The field was unused — the test hardcodes finalCachePath from the marketplace manifest name (lowercased), not from source.name. Drop the field; typecheck goes green. * test(marketplace): use mock.restore() to clear stale marketplaceManager mock Previous attempt used mock.module(path, () => undefined) but Bun rejects that — TypeError: 'mock(module, fn) requires a function that returns an object'. mock.module() with no factory isn't a documented API, so switch to mock.restore() which clears all module mocks. After mock.restore(), the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore() also cleared), so the suite-specific mock still works. Skipped: spying on cp via setFsImplementation. The FsOperations interface (src/utils/fsOperations.ts L23-72) has no cp method, so adding cp: cpSpy to setFsImplementation would fail typecheck. The rename-failure fallback is still verified via filesystem state assertions: before/after readdirSync snapshot plus the explicit lingeringTempFiles === [] check. * Revert "test(marketplace): use mock.restore() to clear stale marketplaceManager mock" This reverts commit bbbebfb. * test(marketplace): fix mock.module TypeError (factory must return object) 0c917a1 used () => undefined to force the real module, but Bun rejects factories that don't return an object ('mock(module, fn) requires a function that returns an object'). Switch to mock.restore() at the top of the file — clears all module mocks registered by prior test files so the real marketplaceManager.js is used on import. The EXDEV describe's beforeAll re-registers the axios mock (which mock.restore also cleared), so suite-specific mocks still work. The env-var-leak bug (getMarketplacesCacheDir returning the leaked /tmp/openclaude-marketplaces instead of the test's tempDir) is a deeper issue with how process.env is read in Bun's test runner. Pre-existing on origin/main and out of scope for this PR; documented in the PR body. * test(marketplace): drop mock.restore() to let natural module resolution work CodeRabbit round 8 P2: the previous implementation called mock.restore() at file-scope on the assumption it would clear module mocks registered by other test files (lspRecommendation.test.ts, officialMarketplaceStartupCheck.test.ts). Bun's docs confirm mock.restore() does NOT clear mock.module() registrations, so the partial mock from the prior file persisted and the import of _test broke ("Export named '_test' not found"). The reproducer is real: running lspRecommendation.test.ts first then this file fails. But the root cause is a pre-existing bug in lspRecommendation.test.ts (its mock for '../config.js' is missing normalizeMaxMessagesCompactionThreshold, added to config.ts in PR Twigpine#1605 / commit 2002e4c). That lspRecommendation test fails on its own on origin/main, and was already failing the lspRecommendation test before the marketplaceManager test even ran. Drop the mock.restore() call so the test's import relies on Bun's natural module resolution. The marketplaceManager test passes in isolation. The full-suite failure is caused by the pre-existing lspRecommendation bug, not by this test. Fixing that is out of scope for Twigpine#1531. * test(marketplace): import real marketplaceManager in full suite via cache-busted URL CodeRabbit round 8 follow-up + jatmn round 8 P2: the test file's static `import { _test } from './marketplaceManager.js'` resolves to whichever mock was registered first for the bare path. Two other test files register partial mocks at module top-level: - lspRecommendation.test.ts:32 mocks the path with 9 exports (no _test) - officialMarketplaceStartupCheck.test.ts:96 mocks it with 9 exports (no _test) Whichever runs first wins, and downstream test files re-importing the same bare path inherit that partial mock — yielding "Export named '_test' not found in module './marketplaceManager.js'" and 4 failing tests in the full suite. The fix uses three techniques: 1. The static import at the top of marketplaceManager.test.ts is changed to `'./marketplaceManager.js?bust=this-test-needs-the-real-module'`. Bun treats the query-string suffix as a distinct module id, so the import bypasses the bare-path mock.module() registration in lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts. The constant suffix is sufficient because we only need the import to happen once at module top-level — no per-test re-evaluation is required for the cache-sharing test block. 2. The EXDEV describe block's dynamic re-import (line 263) was already using a non-busted URL `'./marketplaceManager.ts'` to pick up the mocked axios. It gets the partial mock from the other test files instead, with the same `_test` is undefined failure. Apply the same `?bust=exdev-test-reimport` suffix so the re-import is treated as a fresh module id and still picks up the axios mock (because `'axios'` is a different module id than the busted marketplace URL). 3. The two upstream test files (lspRecommendation.test.ts and officialMarketplaceStartupCheck.test.ts) had stale manual mock lists that were missing recent exports (e.g. `normalizeMaxMessagesCompactionThreshold` added in PR Twigpine#1605, `logAntError`, `logMCPError`, `isSourceInBlocklist`, etc.). These missing entries broke transitive imports of marketplaceManager → config / debug / log / marketplaceHelpers. Switch their mocks to the `...await import('...real=...')` spread pattern already used for growthbook (line 41). This preserves all real exports while overriding the test-specific ones — robust to future additions. The same comment in the static-import block documents the technique so future readers understand why a query string is part of the URL. Verified locally: `bun test --max-concurrency=1 ./src/utils/plugins/` runs all 37 tests across 5 files (lspRecommendation, marketplaceManager, officialMarketplaceStartupCheck, pluginLoader, gitAvailability) with 0 failures, regardless of file order. Each test file in isolation also passes. * test(marketplace): make typecheckable via @ts-expect-error + template literal jatmn round 13 P2: the cache-busted imports from cc6c184 (commit) were valid runtime under Bun but TypeScript couldn't resolve them, breaking `tsc --noEmit`. Fix by combining two patterns already in the codebase: 1. **Static import with `// @ts-expect-error`**: the top-of-file import uses the `?bust=this-test-needs-the-real-module` query string. The `// @ts-expect-error` directive above the import statement suppresses the TS2307 ("Cannot find module"). Pattern from `src/hooks/useApiKeyVerification.test.tsx:104`. 2. **Dynamic import with template literal + `as typeof` cast**: the EXDEV describe block's re-import (which must happen after `mock.module('axios', ...)` is set up) uses a template literal `` `./marketplaceManager.ts?bust=exdev-test-reimport-${Date.now()}` `` so TypeScript treats the specifier as a dynamic `string` (not a literal that it tries to resolve at compile time). The `as typeof import('./marketplaceManager.js')` cast then re-types the result. Pattern from `src/utils/hookChains.integration.test.ts:43-50`. Also added `!` non-null assertions on the cached module-level `loadAndCacheMarketplace` reference to silence the "possibly undefined" warnings that arose when the variable was declared without an initializer. Verified: - `bun x tsc --noEmit --ignoreDeprecations 6.0` against src/utils/plugins/marketplaceManager.test.ts: no errors except the pre-existing `bun:test` ambient module declaration (which is the same error all *.test.ts files in the repo show). - `bun test --max-concurrency=1 src/utils/plugins/marketplaceManager.test.ts`: 4/4 pass. - `bun test --max-concurrency=1` of the three test files that share marketplaceManager.js (lspRecommendation, officialMarketplaceStartupCheck, marketplaceManager): 11/11 pass. - The 16 failures in the broader smoke run are pre-existing on origin/main (GlobalConfig — showCacheStats, provider-specific cap tests for deepseek/gpt-4o/MiniMax-M2.7/etc., all added by the recent Fireworks AI provider upstream commit). Not introduced by this PR. * debug(marketplace): log EXDEV rm/renames * test(marketplace): use rmSync instead of mocked fs/promises rm for EXDEV fallback openclaudeInstallSurfaces.test.ts mocks fs/promises with a no-op rm. The EXDEV test's rm wrapper delegated to NodeFsOperations.rm which used the mocked rmPromise, leaving temp_*.json artifacts. Use the synchronous rmSync from 'fs' (not mocked) instead. * fix(plugins): gate marketplace cache path comparisons on FS case-sensitivity The cache finalization and cleanup paths compared full paths with an unconditional .toLowerCase(), so on case-sensitive filesystems (Linux) two distinct cache entries differing only in case were treated as the same directory. This skipped the rename in loadAndCacheMarketplace, skipped stale-cache cleanup in addMarketplaceSource, and made removeMarketplaceSource delete a recomputed lowercase path that misses a mixed-case cache dir. Add isCaseInsensitiveFs()/pathsEqualForFs() helpers and gate both comparisons on them, and delete the recorded installLocation in removeMarketplaceSource instead of a recomputed path. Pins the existing stay deterministic on the case-sensitive Linux CI runner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(plugins): verify EXDEV cp+rm fallback via an injectable fs.cp spy The marketplace cache finalization fallback called cp() via the direct fs/promises import, so it could not be observed through the swappable fs implementation that rm/rename already use. The EXDEV regression test could only assert end state, not that the cp+rm fallback branch ran. Add cp to FsOperations/NodeFsOperations and route the finalization fallback through fs.cp, then spy on it in the EXDEV test to assert the copy ran exactly once with temp -> final and { recursive: true }. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(plugins): probe cache volume case-sensitivity instead of assuming by platform isCaseInsensitiveFs() assumed every darwin process runs on a case-insensitive volume, but macOS can mount case-sensitive APFS/HFS+. On those volumes two cache paths differing only in case are distinct, so pathsEqualForFs() wrongly reported them equal and addMarketplaceSource() skipped cleanup of the old mixed-case cache dir, orphaning it. Replace the platform heuristic with isCaseInsensitiveFsAt(dir): Windows is always case-insensitive (and has unreliable inode numbers) so trust the platform there; elsewhere stat the directory under a case-flipped name and treat the volume as case-insensitive only when both names resolve to the same inode/dev. Memoized per directory, fails safe to case-sensitive. pathsEqualForFs() now takes the cache dir to probe. Tests inject statSync to simulate both volume kinds, so they're portable (no real case-sensitive mount required). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit a7d6580)
hotmanxp
added a commit
to hotmanxp/openclaude
that referenced
this pull request
Jun 16, 2026
Twigpine#1386) Cherry-picked from upstream 716c1d4. Adds: - Auto-compact prompt on /resume (ResumeCompactPrompt component) for large sessions - Determinate progress bar (CompactProgressBar) driven by runForkedAgent onStreamEvent - COMPACT_TIMEOUT_MS=120s Promise.race guard around fork agent - character-level progress ratio estimation - createChildAbortController wrapping for proper parent abort propagation OC-specific changes: - commands/compact/compact.ts: keep OC's ultraplan-choice / ultraplan-launch dialogs alongside new resume-compact - services/compact/compact.ts: add 'createChildAbortController' import (OC missing) - screens/REPL.tsx: keep OC's inline critical-dialog resolution (no resolveCriticalInputDialog helper) - screens/REPL.tsx: skip <CompletionFlash /> — depends on PR Twigpine#1605 (UI upgraded) skipped per AGENTS.md - compact.test.ts: skip GitHub Native Anthropic mode test (legacy provider per AGENTS.md) Includes 5 new tests in resumeCompactPrompt.test.ts.
hotmanxp
added a commit
to hotmanxp/openclaude
that referenced
this pull request
Jun 16, 2026
Goal: bring `bun run typecheck` to 0 errors on main-opencc. Five files touched, no behavior changes: - config.ts: cherry-pick da7bc4a (Twigpine#1629) referenced MaxMessagesCompactionThreshold without importing the type. Add the type, threshold constant tuple, and normalize helper from PR Twigpine#1605. - Config.tsx: drop dead `&& pendingScope` clause at line 1666; showSubmenu === 'ExternalIncludes' guard already gates the dialog. - initReplBridge.ts: cast msg.message.content at line 293 — UserMessage stores response shape (ContentBlock[]) but getContentText expects request shape (ContentBlockParam[]); cast via Parameters<>[0]. - compact.test.ts: helpers must include top-level `content` per Message interface; rewrite mock to handle optional `msg.message?.content`. - sessionStorage.test.ts: 5 tests reference upstream-only goalStates (loadTranscriptFile) and `goal` arg (restoreSessionMetadata) — OC uses appState.goalSentinel instead. Convert to test.skip + no-op placeholder per AGENTS.md skip-legacy-test pattern. Build + impacted test suites verified green.
4 of 5 tasks
8 tasks done
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
brand/brandShimmertheme keys; theclaudecolor family(
claude,claudeShimmer,clawd_body,briefLabelClaude) repointed to the gitlawborange #ff7a1a across all 6 themes (darkened on light backgrounds, luminance-separated
for daltonized,
redBrightfloor on 16-color ANSI). Newsrc/constants/brand.tswithname, tagline, accent, and a 2-row half-block OPENCLAUDE wordmark shown in the full
logo. Startup splash defaults to a new brand-orange
embergradient (existing /logopalette choices kept); CondensedLogo drops its duplicated "OPEN CLAUDE" header; Clawd
mascot body renders in
clawd_body; spinner switches to a dot-pulse glyph sweep(· ∘ ○ ◎ ◉ ●), removing the Ghostty/darwin glyph special-cases.
model · ctx % · session cost · rate limitrenders when nocustom /statusline command is configured. Pure in-process computation reusing the custom
StatusLine's data pipeline (no subprocess/debounce); segments drop right-to-left on
narrow terminals; ctx colors warning ≥70% / error ≥90%; rate limit shows the worst of
the 5h/7d windows and is absent (not 0%) for API-key users; a configured custom
statusline always wins;
defaultStatusLineEnabledtoggle in /config.passes the live query to
renderItemso memoized callbacks can't go stale); Selectinline descriptions truncate with
…instead of clipping; transient✓ Done · 12scompletion flash after responses >1s (keys off the isLoading transition; suppressed for
brief mode, open prompts, running teammates, reduced motion); diff gutter line numbers
dimmed on +/- lines with sigils at full intensity (mirrored in the Fallback renderer);
spinner status parens lead with the ↑/↓ request-direction glyph immediately instead of
after 30s; Dialog gains an opt-in "↑/↓ navigate" hint (IdleReturn/DevChannels/
WorktreeExit); stale transcript-search TODO replaced with a pointer to the shipped
REPL search.
no status bar without manual /statusline setup, and several core surfaces had known
rough edges (clipped descriptions, unhighlighted search results, gutter-heavy diffs,
no completion feedback).
Impact
color="claude"is used (~52 callsites); new wordmark, splash palette, spinner, and mascot styling; a default status bar
appears after the first assistant turn (disable in /config; custom statusline
unaffected); match highlighting in pickers; completion flash; more readable diffs.
Theme names/settings unchanged.
theme.ts+constants/brand.ts; theme values must stayrgb()strings (never hex) because thespinner stall/shimmer interpolation parses them with
parseRGB, which silently failson hex.
Dialogrewritten from react-compiler output to plain React.FuzzyPicker.renderItemgained a thirdqueryargument (backward-compatible).Testing
bun run buildbun run smokebun run checkbun test src/components src/utils/highlightMatch.test.tsx— 182pass / 0 fail on this branch (incl. new
BuiltinStatusLine.test.tsxandhighlightMatch.test.tsx, plus the StartupScreen palette/logo, ThemePicker, andThemeProvider suites);
tsc --noEmitcleanNotes
reuses the existing model/usage/cost/rate-limit pipeline unchanged
passes in iTerm2 (truecolor), Terminal.app (256-color + Apple branches), and tmux
(level-2 clamp) before merge
redBright(no orange inthat palette); orange→red stall fade is subtler than the old blue→red; no search on the
legacy non-virtual-scroll transcript path (intentionally skipped); the three workstreams
also exist as separate single-commit branches (
ui/brand-identity,ui/builtin-statusline,ui/polish-pass) on this fork if separate review is preferredScreen.Recording.2026-06-12.at.10.35.31.AM.mov
Summary by CodeRabbit
New Features
Settings
Enhancements
UI/Visual
Tests