fix(thinkingVisibility): support v2.1.x native binary format #353
Conversation
Two patches are now required for full thinking visibility:
1. Case statement - prevents thinking blocks from being skipped
Pattern: case"thinking":{if(!X&&!Y)return null
2. FbH collapsed view - shows expanded instead of "(ctrl+o to expand)"
Pattern: if(!(X||Y))return ZZZ
Key changes:
- Add support for new v2.1.x format with opening brace after case
- Add second patch for FbH collapsed view function
- Use flexible regex to handle variable name changes between versions
- Maintain backwards compatibility with older format
- Patch FIRST occurrence only (binary has duplicate code sections)
Tested on Claude Code v2.1.2 native binary (Linux x64).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSplit the previous single-step thinking-visibility patch into two coordinated patches: one modifies the thinking-case condition to use inline truthiness, the other converts an FbH collapsed-view guard into an expanded-view form. Both patches handle multiple code formats and are applied sequentially with incremental diffs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/patches/thinkingVisibility.ts (2)
117-147: Non-null assertions assumeidentifiersis always present.The
identifiers!assertions on lines 126 and 128 rely on the implementation always populating this optional field. While the current code does always set it when returning non-null, this creates a fragile coupling.Consider adding a guard or using optional chaining with a fallback:
- if (caseLocation.identifiers!.length === 4) { + if (caseLocation.identifiers?.length === 4) {
149-172: Partial patching may produce unexpected behavior.Per the PR objectives, both patches are required for full visibility. If Patch 1 succeeds but Patch 2 fails (or vice versa), the function returns a partially-patched file with
anyPatched = true. This could leave the binary in an inconsistent state where thinking blocks are partially functional.Consider whether partial patching should be treated as a failure, or at minimum add a warning when only one patch succeeds:
+ if (anyPatched && (!caseLocation || !fbhLocation)) { + console.warn('patch: thinkingVisibility: only partial patching applied'); + } + return anyPatched ? newFile : null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/patches/thinkingVisibility.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/patches/thinkingVisibility.ts (1)
src/patches/index.ts (2)
LocationResult(71-75)showDiff(89-127)
🔇 Additional comments (3)
src/patches/thinkingVisibility.ts (3)
5-14: Clear documentation of the two-patch strategy.The JSDoc clearly explains the rationale and approach for both patches. This is helpful for future maintainers understanding why two separate patches are needed.
24-73: LGTM - robust pattern matching with fallback for older versions.The function correctly handles two different binary formats with appropriate fallback logic. The capture groups and identifier extraction are well-structured.
87-115: Correct first-match behavior for duplicate code sections.The use of
.match()without thegflag correctly returns only the first occurrence, which is essential per the comment about the binary containing source + runtime copies. The restrictive pattern[A-Z]for single uppercase letters helps avoid false positives.
- Use optional chaining instead of non-null assertions - Add warning when only partial patching applied Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/patches/thinkingVisibility.ts:
- Around line 172-175: The current warning logic uses anyPatched plus existence
of caseLocation/fbhLocation and misses cases where a location exists but its
patch was skipped (e.g., fbhLocation exists but fbhLocation.identifiers[2] is
undefined). Introduce two explicit booleans (e.g., casePatched and fbhPatched)
and set them true only when the corresponding patch actually succeeds (not just
when a location is found); mark them false when skipped due to missing
identifiers like fbhLocation.identifiers[2]. Replace the final check with a
partial-patch detection such as: if ((casePatched || fbhPatched) &&
!(casePatched && fbhPatched)) console.warn('patch: thinkingVisibility: only
partial patching applied'); to ensure any partial application is logged.
🧹 Nitpick comments (1)
src/patches/thinkingVisibility.ts (1)
32-34: Consider documenting the alternate pattern's complexity.The fallback pattern is quite complex with multiple capture groups and non-greedy matching across significant context (
.+?isTranscriptMode:). While this maintains backward compatibility, the pattern's reliance on surrounding code structure makes it fragile.Consider adding a comment explaining what specific version(s) this pattern targets and what structural assumptions it makes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/current-sessionsrc/patches/thinkingVisibility.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/current-session
🔇 Additional comments (4)
src/patches/thinkingVisibility.ts (4)
5-86: Excellent documentation of the two-patch strategy.The inline comments clearly explain:
- Why both patches are needed for full visibility
- How each patch works (replacing variables with truthy literals)
- Version compatibility considerations
- Critical constraint about patching only the first FbH occurrence
This makes the code highly maintainable.
123-123: Good use of optional chaining for safety.The optional chaining on lines 123 (
caseLocation?.identifiers) and 151 (fbhLocation?.identifiers?.[2]) properly handles cases where location matching succeeds but identifiers are unexpectedly missing. This aligns with the PR's goal of replacing non-null assertions with safer access patterns.Also applies to: 151-151
149-170: Sequential patching approach is correct.Using
newFilefor Patch 2 (line 150) instead ofoldFileproperly accounts for any length changes from Patch 1. SincegetFbhCollapsedLocationre-searches the entire modified file with regex, the returned indices are valid for the current state ofnewFile. This is the right approach for applying multiple patches that might affect each other's positions.
87-115: The code already correctly handles the concerns raised. The uppercase constraint is documented on line 90 ("Using uppercase letter constraint to avoid false positives"), the first-occurrence logic is properly implemented viamatch(), and the FbH function's purpose is fully explained in the comment block (lines 75-86). Pattern verification against actual minified binaries would require external binary analysis outside this repo's scope; the current implementation uses a deliberate first-match approach to ensure correctness even if the pattern appears multiple times in the binary.
| // Warn if only partial patching applied (both patches needed for full visibility) | ||
| if (anyPatched && (!caseLocation || !fbhLocation)) { | ||
| console.warn('patch: thinkingVisibility: only partial patching applied'); | ||
| } |
There was a problem hiding this comment.
Warning condition doesn't catch all partial-patching cases.
The condition anyPatched && (!caseLocation || !fbhLocation) only warns when a location isn't found, but it won't warn if a location is found yet patching fails due to missing identifiers.
Scenario that bypasses the warning:
caseLocationfound with valid identifiers → Patch 1 applied,anyPatched = truefbhLocationfound butfbhLocation.identifiers[2]is undefined → Patch 2 skipped- Condition evaluates:
true && (false || false)=false→ no warning
Track whether each patch was applied individually:
🔍 Proposed fix to catch all partial-patching scenarios
export const writeThinkingVisibility = (oldFile: string): string | null => {
let newFile = oldFile;
- let anyPatched = false;
+ let patch1Applied = false;
+ let patch2Applied = false;
// Patch 1: Case statement visibility
const caseLocation = getCaseStatementLocation(oldFile);
if (caseLocation?.identifiers) {
// ... replacement logic ...
showDiff(
oldFile,
newFile,
replacement,
caseLocation.startIndex,
caseLocation.endIndex
);
- anyPatched = true;
+ patch1Applied = true;
}
// Patch 2: FbH collapsed view (use newFile to account for offset changes)
const fbhLocation = getFbhCollapsedLocation(newFile);
if (fbhLocation?.identifiers?.[2]) {
// ... replacement logic ...
showDiff(
newFile,
patchedFile,
replacement,
fbhLocation.startIndex,
fbhLocation.endIndex
);
newFile = patchedFile;
- anyPatched = true;
+ patch2Applied = true;
}
// Warn if only partial patching applied (both patches needed for full visibility)
- if (anyPatched && (!caseLocation || !fbhLocation)) {
+ if ((patch1Applied || patch2Applied) && !(patch1Applied && patch2Applied)) {
console.warn('patch: thinkingVisibility: only partial patching applied');
}
- return anyPatched ? newFile : null;
+ return (patch1Applied || patch2Applied) ? newFile : null;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/patches/thinkingVisibility.ts around lines 172 - 175, The current
warning logic uses anyPatched plus existence of caseLocation/fbhLocation and
misses cases where a location exists but its patch was skipped (e.g.,
fbhLocation exists but fbhLocation.identifiers[2] is undefined). Introduce two
explicit booleans (e.g., casePatched and fbhPatched) and set them true only when
the corresponding patch actually succeeds (not just when a location is found);
mark them false when skipped due to missing identifiers like
fbhLocation.identifiers[2]. Replace the final check with a partial-patch
detection such as: if ((casePatched || fbhPatched) && !(casePatched &&
fbhPatched)) console.warn('patch: thinkingVisibility: only partial patching
applied'); to ensure any partial application is logged.
|
Closing in favor of #352 which provides a more comprehensive fix for all v2.1.x patches. Tested and confirmed working. Thanks @basekevin! |
|
Did you guys figure out how to display thinking blocks as dim and italic like once before the update? Or am I the only one bothered by it? 
|
2 participants
Summary
Fixes
thinkingVisibilitypatch for Claude Code v2.1.x native binaries.The current patch fails with:
Root cause
Two issues:
case"thinking":that the current regex doesn't expectSolution
Patch 1: Case statement - prevents thinking blocks from being skipped
Patch 2: FbH collapsed view - shows expanded view instead of "(ctrl+o to expand)"
Key changes
Test plan
Note
Other patches also need updating for v2.1.x (thinker verbs, context limit, etc.) - those are separate issues.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.