fix : added ASCII fallback for Markdown blockquote pipe character - #3436
fix : added ASCII fallback for Markdown blockquote pipe character#3436tmdeveloper007 wants to merge 2 commits into
Conversation
github-actions
Bot
added
area:widgets
📝 WalkthroughWalkthroughThe Code and Markdown widgets now use ASCII vertical separators when ChangesUnicode fallback rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/widgets/src/display/Code.test.ts`:
- Line 78: Replace the accessor spy for the plain data property caps.unicode
with direct property overriding using the supported restoration/cleanup pattern
in packages/widgets/src/display/Code.test.ts at lines 78-78 and
packages/widgets/src/display/Markdown.test.ts at lines 54-54. Apply the same
change at both sites and ensure each test restores the original property value
after execution.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c20c9c22-1d56-4a5a-bd8d-56505403a320
📒 Files selected for processing (4)
packages/widgets/src/display/Code.test.tspackages/widgets/src/display/Code.tspackages/widgets/src/display/Markdown.test.tspackages/widgets/src/display/Markdown.ts
| }); | ||
|
|
||
| it('uses ASCII pipe for gutter separator when caps.unicode is false', () => { | ||
| vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)packages/widgets/src/display/(Code|Markdown)\.(ts|test\.ts)$|caps\.unicode|caps:' || true
echo "== inspect caps definitions/usages =="
rg -n "export .*caps|caps.*unicode|unicode.*:" . -g '*.ts' -g '*.tsx' | head -100
echo "== inspect test slices =="
for f in packages/widgets/src/display/Code.test.ts packages/widgets/src/display/Markdown.test.ts; do
if [ -f "$f" ]; then
echo "--- $f line count ---"
wc -l "$f"
echo "--- $f relevant lines ---"
sed -n '1,120p' "$f" | cat -n
fi
done
echo "== inspect Widget/caps imports in display files =="
sed -n '1,40p' packages/widgets/src/display/Code.ts packages/widgets/src/display/Markdown.ts 2>/dev/null | cat -nRepository: Karanjot786/TermUI
Length of output: 22979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { vi } = typeof require === 'function' ? require('vitest') : undefined;
if (!vi) {
console.log('vitest module not available in test runner; checking vitest package metadata only');
} else {
const target = { unicode: true };
try {
vi.spyOn(target, 'unicode', 'get').mockReturnValue(false);
console.log('getter-spy succeeded');
} catch (e) {
console.log(`getter-spy error: ${e.name}: ${e.message}`);
}
try {
Object.defineProperty(target, 'unicode', { get() { return true; }, enumerable: true, configurable: true });
vi.spyOn(target, 'unicode', 'get').mockReturnValue(false);
console.log('getter-spy after accessor succeeded');
} catch (e) {
console.log(`getter-spy on accessor error: ${e.name}: ${e.message}`);
}
}
JSRepository: Karanjot786/TermUI
Length of output: 1011
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { vi } = typeof require === 'function' ? require('vitest') : undefined;
if (!vi) {
console.log('vitest module not available in test runner; checking vitest package metadata only');
} else {
const target = { unicode: true };
try {
vi.spyOn(target, 'unicode', 'get').mockReturnValue(false);
console.log('getter-spy succeeded');
} catch (e) {
console.log(`getter-spy error: ${e.name}: ${e.message}`);
}
try {
Object.defineProperty(target, 'uncode', { get() { return true; }, enumerable: true, configurable: true });
} catch (e) {
return;
}
}
JSRepository: Karanjot786/TermUI
Length of output: 756
🌐 Web query:
Vitest vi.spyOn getter overload does property need getter documentation
💡 Result:
Yes, when using vi.spyOn to target a getter (or setter), you must provide the 'get' (or 'set') argument as the third parameter to the function [1][2][3]. The function signature is defined as [1][3]: vi.spyOn<T, K extends keyof T>(object: T, key: K, accessor?: 'get' | 'set'): Mock<T[K]> By passing 'get' as the third argument, you explicitly tell Vitest to spy on the property's getter descriptor rather than attempting to spy on the property's value [2][4]. This is required for mocking or observing behavior on property getters [2][5]. Example usage [2]: const spy = vi.spyOn(object, 'propertyName', 'get').mockReturnValue('mocked value');
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://vitest.dev/guide/mocking/classes
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 4: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/packages/spy/src/index.ts
- 5: https://vitest.dev/guide/mocking
🌐 Web query:
Vitest vi.spyOn plain data property no getter mockReturnValue works
💡 Result:
In Vitest, vi.spyOn is designed specifically to spy on functions (methods) or getters/setters [1][2]. If you attempt to use vi.spyOn on a plain data property (a property that is not a function or a getter), Vitest will throw an error because it expects the target to be a function [3]. If you need to mock a plain data property, vi.spyOn will not work directly because it tries to wrap the existing property in a mock function [4]. Instead, you should modify the property directly on the object. Here is the recommended approach for handling plain data properties in tests: 1. Manual Override: Since it is a plain data property, you can simply overwrite the value before the test and restore it afterwards. const originalValue = obj.prop; obj.prop = 'mocked value'; // run test obj.prop = originalValue; // cleanup 2. Using Object.defineProperty: If you need to ensure the property behaves like a getter or if you want to use more advanced mocking tools, you can redefine the property using Object.defineProperty to create a getter that returns your mocked value [1][3]. const originalDescriptor = Object.getOwnPropertyDescriptor(obj, 'prop'); Object.defineProperty(obj, 'prop', { get: => 'mocked value', configurable: true, }); // run test Object.defineProperty(obj, 'prop', originalDescriptor); // cleanup If you were trying to use mockReturnValue, that method is only available on objects that are already mock functions (created via vi.fn or an active vi.spyOn on a function) [5][6]. Because a plain data property is not a function, it does not have the mockReturnValue method [2][4].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.9/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: https://github.com/vitest-dev/vitest/blob/fda52af9c3974d5fd16161e1ed502dd47fd38ceb/packages/spy/src/index.ts
- 4: https://main.vitest.dev/guide/learn/mock-functions
- 5: https://vitest.dev/api/mock.html
- 6: https://github.com/vitest-dev/vitest/blob/v4.1.9/docs/api/mock.md
🌐 Web query:
Nuxthub vitest 3 vi.spyOn property no getter mockReturnValue source
💡 Result:
To mock a property that does not have an explicit getter using vi.spyOn, you must explicitly specify 'get' as the accessor argument [1][2]. When you want to spy on or mock a simple object property that lacks a getter, you can force Vitest to treat it as a getter by passing the accessor parameter [1][3]. This allows you to then use mock functions like mockReturnValue or mockImplementation on that property [3]. The syntax is: vi.spyOn(object, 'propertyName', 'get').mockReturnValue(value) Without specifying 'get', vi.spyOn expects to target a function or an existing getter/setter, and may throw an error if the property is a primitive or a non-function value [4][5]. By providing 'get', you instruct Vitest to redefine the property on the object as a getter that returns your mocked value [1][3]. Note that this works by redefining the property descriptor on the target object [1][4]. If you need to revert these changes after your test, ensure you use vi.restoreAllMocks or mockRestore on the returned spy to restore the original property descriptor [1].
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://github.com/vitest-dev/vitest/blob/main/docs/guide/mocking.md
- 3: https://vitest.dev/guide/mocking/classes
- 4: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/packages/spy/src/index.ts
- 5: https://github.com/vitest-dev/vitest/blob/fda52af9c3974d5fd16161e1ed502dd47fd38ceb/packages/spy/src/index.ts
Use the supported property-mock pattern for caps.unicode.
caps.unicode is a plain data property, not a method or accessor, so `vi.spyOn(..., 'unicode', 'get').mockReturnValue(...) is not appropriate. Override the property directly with restoration/cleanup instead.
📍 Affects 2 files
packages/widgets/src/display/Code.test.ts#L78-L78(this comment)packages/widgets/src/display/Markdown.test.ts#L54-L54
🤖 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 `@packages/widgets/src/display/Code.test.ts` at line 78, Replace the accessor
spy for the plain data property caps.unicode with direct property overriding
using the supported restoration/cleanup pattern in
packages/widgets/src/display/Code.test.ts at lines 78-78 and
packages/widgets/src/display/Markdown.test.ts at lines 54-54. Apply the same
change at both sites and ensure each test restores the original property value
after execution.
Source: Learnings
1 participant
Description
Fixes a bug in the Markdown widget where blockquote lines always render with a Unicode box-drawing pipe character
│(U+2502), even when the terminal does not support Unicode. This causes display issues on ASCII-only terminals.The fix adds a
caps.unicodecheck and falls back to the ASCII pipe character|when Unicode is disabled.Changes
packages/widgets/src/display/Markdown.ts— replaced hardcoded│with${caps.unicode ? '│' : '|'}packages/widgets/src/display/Markdown.test.ts— added test verifying ASCII pipe fallbackRelated Issues
Type of Change
Checklist
Note: Please assign this PR to the
tmdeveloper007account.Summary by CodeRabbit
|character when needed, while retaining the Unicode│character where supported.