Skip to content

Commit eaf4595

Browse files
committed
Add lettered sub-item indentation and DEFAULT highlighting
Implement proper visual hierarchy for AskUserQuestion multi-select options: - Indent lettered sub-items (a), b), c)) under numbered questions - Highlight DEFAULT options with green background and black text - Document OpenTUI TextNodeRenderable constraints and workarounds Note: Wrapped lines return to column 0 due to OpenTUI limitations with inline text rendering. This is documented as a known constraint.
1 parent 1ca5e42 commit eaf4595

File tree

3 files changed

+208
-114
lines changed

3 files changed

+208
-114
lines changed

cli/knowledge.md

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ tmux new-session -d -s test-session 'cd /path/to/codebuff && bun --cwd=cli run d
5858

5959
## Migration from Custom OpenTUI Fork
6060

61-
**October 2024**: Migrated from custom `CodebuffAI/opentui#codebuff/custom` fork to official `@opentui/react@^0.1.27` and `@opentui/core@^0.1.27` packages. Updated to `^0.1.28` in February 2025.
61+
**October 2024**: Migrated from custom `CodebuffAI/opentui#codebuff/custom` fork to official `@opentui/react@^0.1.27` and `@opentui/core@^0.1.27` packages. Updated to `^0.1.28` in February 2025. **November 2025**: Upgraded to `0.1.41` to test if TextNodeRenderable constraints were relaxed (they weren't - still have same limitations).
6262

6363
**Lost Features from Custom Fork:**
6464

@@ -604,6 +604,98 @@ This prevents invalid children from reaching `TextNodeRenderable` while preservi
604604

605605
**Related**: `cli/src/hooks/use-message-renderer.tsx` ensures toggle headers render within a single `<text>` block for StyledText compatibility.
606606

607+
### Using `<text>` vs `<box>` for Container Elements
608+
609+
**CRITICAL**: `<text>` elements have strict child requirements and cannot accept arbitrary React elements as direct children.
610+
611+
**The Error:**
612+
```
613+
Error: TextNodeRenderable only accepts strings, TextNodeRenderable instances, or StyledText instances
614+
at add (/path/to/TextNode.ts:108:15)
615+
```
616+
617+
**When this occurs:**
618+
```tsx
619+
// ❌ WRONG: Passing React fragments/elements directly as children of <text>
620+
<text style={{ paddingLeft: 3 }}>
621+
{wrapSegmentsInFragments(children, 'key-prefix')} // Returns Fragments containing spans
622+
</text>
623+
```
624+
625+
**Why it fails:**
626+
- `wrapSegmentsInFragments()` returns `KeyedFragment` elements wrapping content
627+
- `<text>` expects direct text-renderable children (strings, `<span>`, `<strong>`, etc.)
628+
- React Fragments are not TextNodeRenderable instances
629+
- OpenTUI's TextNode cannot process the Fragment wrapper
630+
631+
**✅ CORRECT Solution: Use `<box>` for layout, `<text>` for content**
632+
633+
```tsx
634+
// Use <box> as container with paddingLeft style
635+
<box style={{ paddingLeft: 3 }}>
636+
<text>
637+
{wrapSegmentsInFragments(children, 'key-prefix')}
638+
</text>
639+
</box>
640+
```
641+
642+
**Why this works:**
643+
- `<box>` accepts any React elements as children (fragments, boxes, text, etc.)
644+
- `<box>` supports `paddingLeft` style property for indentation
645+
- The inner `<text>` properly wraps the text-renderable content
646+
- Wrapping behavior respects the box's paddingLeft on ALL lines
647+
648+
**Key Learnings:**
649+
650+
1. **For indentation with paddingLeft**: Use `<box style={{ paddingLeft: N }}>`, NOT `<text style={{ paddingLeft: N }}>`
651+
2. **`<text>` is for text rendering**: It should contain text-renderable elements (strings, spans, etc.)
652+
3. **`<box>` is for layout**: It can contain any React elements and supports layout styles
653+
4. **Fragments as children**: Only `<box>` can handle Fragment children; `<text>` cannot
654+
5. **paddingLeft on wrapped text**: Use `<box>` container to ensure wrapped lines maintain indentation
655+
656+
**Pattern for indented text blocks:**
657+
```tsx
658+
// ✅ Correct pattern for indented, wrappable text
659+
<box style={{ paddingLeft: 3 }}>
660+
<text>
661+
<span>Line one</span>
662+
{'\n'}
663+
<span>Line two</span>
664+
</text>
665+
</box>
666+
```
667+
668+
### Markdown Renderer Indentation Limitation
669+
670+
**Problem:** When markdown content is rendered as inline elements (strings, `<span>`, etc.) and wrapped in `<text>` by parent components, there's no way to maintain indentation on wrapped lines.
671+
672+
**Why we can't use `<box>` with `paddingLeft`:**
673+
- Markdown renderer returns text-renderable inline elements
674+
- Parent components wrap output in `<text>{markdownOutput}</text>`
675+
- `<text>` can only contain text-renderable children (strings, `<span>`, etc.)
676+
- `<box>` is NOT text-renderable, so returning it from markdown renderer causes:
677+
```
678+
Error: TextNodeRenderable only accepts strings, TextNodeRenderable instances, or StyledText instances
679+
```
680+
681+
**Current Solution:**
682+
- Use manual space prepending for lettered sub-items (e.g., ` a) Option`)
683+
- First line is indented correctly
684+
- **Known limitation:** Wrapped lines return to column 0 instead of maintaining indentation
685+
- This is an acceptable trade-off given OpenTUI's architecture
686+
687+
**Example of limitation:**
688+
```
689+
a) This is a very long option that wraps to
690+
the next line but doesn't stay indented
691+
b) Short option
692+
```
693+
694+
**Why this is acceptable:**
695+
- The first line indentation still provides visual hierarchy
696+
- Alternative approaches (returning `<box>` from renderer) violate OpenTUI's rendering constraints
697+
- Users can still clearly see the structure even with imperfect wrapping
698+
607699

608700

609701
## Command Menus

cli/src/utils/__tests__/markdown-renderer.test.tsx

Lines changed: 62 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,7 @@ b) Focus on email/in-app notifications first (simpler to implement)`
414414
// Convert to text content (recursively extracts all text including from nested spans)
415415
const textContent = getAllTextContent(output)
416416

417-
// Lettered items should be indented (3 spaces when under numbered lists)
417+
// Lettered items should be indented with manual spaces (3 spaces under numbered lists)
418418
expect(textContent).toContain(' a) (DEFAULT) PostgreSQL')
419419
expect(textContent).toContain(' b) Dedicated time-series')
420420
expect(textContent).toContain(' c) Redis for real-time')
@@ -435,19 +435,10 @@ a) Another option
435435
b) One more option`
436436

437437
const output = renderMarkdown(markdown)
438-
const nodes = flattenNodes(output)
439438

440-
const textContent = nodes
441-
.map((node) => {
442-
if (typeof node === 'string') return node
443-
if (React.isValidElement(node)) {
444-
return flattenChildren(node.props.children).join('')
445-
}
446-
return ''
447-
})
448-
.join('')
439+
const textContent = getAllTextContent(output)
449440

450-
// All lettered items should have 3 spaces of indentation (under numbered lists)
441+
// All lettered items should be indented with manual spaces (3 spaces)
451442
expect(textContent).toContain(' a) First option')
452443
expect(textContent).toContain(' b) Second option')
453444
expect(textContent).toContain(' c) Third option')
@@ -461,19 +452,10 @@ b) Second standalone option
461452
c) Third standalone option`
462453

463454
const output = renderMarkdown(markdown)
464-
const nodes = flattenNodes(output)
465455

466-
const textContent = nodes
467-
.map((node) => {
468-
if (typeof node === 'string') return node
469-
if (React.isValidElement(node)) {
470-
return flattenChildren(node.props.children).join('')
471-
}
472-
return ''
473-
})
474-
.join('')
456+
const textContent = getAllTextContent(output)
475457

476-
// Should still be indented even without numbered parents
458+
// Should be indented with manual spaces (6 spaces at root level)
477459
expect(textContent).toContain(' a) First standalone')
478460
expect(textContent).toContain(' b) Second standalone')
479461
expect(textContent).toContain(' c) Third standalone')
@@ -490,7 +472,7 @@ c) Advanced configuration`
490472
// Convert to text content (recursively extracts all text including from nested spans)
491473
const textContent = getAllTextContent(output)
492474

493-
// Should preserve DEFAULT markers and apply indentation (3 spaces under list)
475+
// Should preserve DEFAULT markers with indentation (3 spaces)
494476
expect(textContent).toContain(' a) (DEFAULT) Standard')
495477
expect(textContent).toContain(' b) Custom')
496478
expect(textContent).toContain(' c) Advanced')
@@ -503,19 +485,11 @@ b) Short option
503485
c) Another very detailed option explaining all the trade-offs and considerations you should think about`
504486

505487
const output = renderMarkdown(markdown)
506-
const nodes = flattenNodes(output)
507488

508-
const textContent = nodes
509-
.map((node) => {
510-
if (typeof node === 'string') return node
511-
if (React.isValidElement(node)) {
512-
return flattenChildren(node.props.children).join('')
513-
}
514-
return ''
515-
})
516-
.join('')
489+
const textContent = getAllTextContent(output)
517490

518-
// Long text should still be indented (3 spaces under list)
491+
// Long text should be indented (3 spaces)
492+
// Note: Wrapped lines will go to column 0 (OpenTUI limitation)
519493
expect(textContent).toContain(' a) This is a very long option')
520494
expect(textContent).toContain(' b) Short option')
521495
expect(textContent).toContain(' c) Another very detailed')
@@ -531,19 +505,10 @@ e) Option E
531505
f) Option F`
532506

533507
const output = renderMarkdown(markdown)
534-
const nodes = flattenNodes(output)
535508

536-
const textContent = nodes
537-
.map((node) => {
538-
if (typeof node === 'string') return node
539-
if (React.isValidElement(node)) {
540-
return flattenChildren(node.props.children).join('')
541-
}
542-
return ''
543-
})
544-
.join('')
509+
const textContent = getAllTextContent(output)
545510

546-
// All lettered items a-f should be indented (3 spaces under list)
511+
// All lettered items a-f should be indented (3 spaces)
547512
expect(textContent).toContain(' a) Option A')
548513
expect(textContent).toContain(' b) Option B')
549514
expect(textContent).toContain(' c) Option C')
@@ -592,24 +557,15 @@ b) Final option
592557
Conclusion text at the end.`
593558

594559
const output = renderMarkdown(markdown)
595-
const nodes = flattenNodes(output)
596560

597-
const textContent = nodes
598-
.map((node) => {
599-
if (typeof node === 'string') return node
600-
if (React.isValidElement(node)) {
601-
return flattenChildren(node.props.children).join('')
602-
}
603-
return ''
604-
})
605-
.join('')
561+
const textContent = getAllTextContent(output)
606562

607-
// Context and conclusion should not be indented
563+
// Context and conclusion should be present
608564
expect(textContent).toContain('Here\'s some context')
609565
expect(textContent).toContain('And some text in between')
610566
expect(textContent).toContain('Conclusion text at the end')
611567

612-
// Lettered items should be indented (3 spaces under list)
568+
// Lettered items should be indented (3 spaces)
613569
expect(textContent).toContain(' a) Option one')
614570
expect(textContent).toContain(' b) Option two')
615571
expect(textContent).toContain(' a) Another option')
@@ -620,49 +576,45 @@ Conclusion text at the end.`
620576
const markdown = `This is a sentence that mentions a) something in the middle.`
621577

622578
const output = renderMarkdown(markdown)
623-
const nodes = flattenNodes(output)
624579

625-
const textContent = nodes
626-
.map((node) => {
627-
if (typeof node === 'string') return node
628-
if (React.isValidElement(node)) {
629-
return flattenChildren(node.props.children).join('')
630-
}
631-
return ''
632-
})
633-
.join('')
580+
const textContent = getAllTextContent(output)
634581

635-
// Should not add indentation for a) in the middle of a sentence
636-
expect(textContent).not.toContain(' a) something')
582+
// Should contain the text normally (no special handling for a) mid-sentence)
637583
expect(textContent).toContain('a) something in the middle')
584+
585+
// Should NOT have indentation spaces added
586+
expect(textContent).not.toContain(' a) something')
638587
})
639588

640-
test('makes DEFAULT options bold', () => {
589+
test('highlights DEFAULT options with green background and black text', () => {
641590
const markdown = `1. Choose your option:
642591
a) (DEFAULT) Standard configuration
643592
b) Custom configuration
644593
c) Advanced configuration`
645594

646595
const output = renderMarkdown(markdown)
647596

648-
// Find all span elements with BOLD attribute (recursively)
649-
const boldSpans = findAllElements(
597+
// Find all span elements with green background
598+
const defaultSpans = findAllElements(
650599
output,
651-
(el) => el.type === 'span' && el.props.attributes === TextAttributes.BOLD
600+
(el) => el.type === 'span' && el.props.bg === '#86efac'
652601
)
653602

654-
// Should have at least one bold span
655-
expect(boldSpans.length).toBeGreaterThan(0)
603+
// Should have at least one DEFAULT span
604+
expect(defaultSpans.length).toBeGreaterThan(0)
656605

657-
// Check that bold span contains DEFAULT text
658-
const boldTexts = boldSpans.map((span) =>
606+
// Check that it has black text
607+
expect(defaultSpans[0].props.fg).toBe('#000000')
608+
609+
// Check that highlighted span contains DEFAULT text
610+
const highlightedTexts = defaultSpans.map((span) =>
659611
flattenChildren(span.props.children).join('')
660612
)
661-
const hasDEFAULT = boldTexts.some((text) => text.includes('(DEFAULT)'))
613+
const hasDEFAULT = highlightedTexts.some((text) => text.includes('(DEFAULT)'))
662614
expect(hasDEFAULT).toBe(true)
663615
})
664616

665-
test('bolds multiple DEFAULT options', () => {
617+
test('highlights multiple DEFAULT options', () => {
666618
const markdown = `1. First question:
667619
a) (DEFAULT) Option A
668620
b) Option B
@@ -672,24 +624,46 @@ b) Non-default option`
672624

673625
const output = renderMarkdown(markdown)
674626

675-
// Find all span elements with BOLD attribute (recursively)
676-
const boldSpans = findAllElements(
627+
// Find all span elements with green background
628+
const defaultSpans = findAllElements(
677629
output,
678-
(el) => el.type === 'span' && el.props.attributes === TextAttributes.BOLD
630+
(el) => el.type === 'span' && el.props.bg === '#86efac'
679631
)
680632

681-
// Should have at least 2 bold spans
682-
expect(boldSpans.length).toBeGreaterThanOrEqual(2)
633+
// Should have at least 2 DEFAULT spans
634+
expect(defaultSpans.length).toBeGreaterThanOrEqual(2)
635+
636+
// All should have black text
637+
defaultSpans.forEach((span) => {
638+
expect(span.props.fg).toBe('#000000')
639+
})
683640

684-
// Both bold spans should contain DEFAULT text
685-
const boldTexts = boldSpans.map((span) =>
641+
// Both should contain DEFAULT text
642+
const highlightedTexts = defaultSpans.map((span) =>
686643
flattenChildren(span.props.children).join('')
687644
)
688-
const defaultCount = boldTexts.filter((text) =>
645+
const defaultCount = highlightedTexts.filter((text) =>
689646
text.includes('(DEFAULT)')
690647
).length
691648

692649
expect(defaultCount).toBeGreaterThanOrEqual(2)
693650
})
651+
652+
test('indents lettered items with manual spaces', () => {
653+
const markdown = `1. Question?
654+
a) (DEFAULT) Very long option text
655+
b) Short option`
656+
657+
const output = renderMarkdown(markdown)
658+
659+
const textContent = getAllTextContent(output)
660+
661+
// Should have manual indentation spaces (3 spaces under list)
662+
expect(textContent).toContain(' a) (DEFAULT) Very long')
663+
expect(textContent).toContain(' b) Short option')
664+
665+
// Note: Wrapped lines will return to column 0 due to OpenTUI constraints
666+
// This is a known limitation documented in knowledge.md
667+
})
694668
})
695669
})

0 commit comments

Comments
 (0)