fix(oclite): fix regression - spinner, user display, help, MCP (#146) - #147
Conversation
- Add bootstrap spinner with elapsed time before Ink renders - Change version from 1.0 to 0.1 - Add user message display after submit (blue ❯ prefix) - Implement /help command with available commands list - Make MCP initialization non-blocking (lazy load) - Disable input during streaming to prevent race conditions
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
randomm
commented
Feb 5, 2026
🔴 CRITICAL: User Message Not Displayed in UIFile: The PR adds Issue
Current App.tsx (lines 95-100){/* Streaming content */}{state.streaming.text&&(<Box><Text>{state.streaming.text}</Text></Box>)}Required Fix{/* User message with blue prompt */}{state.streaming.userMessage&&(<BoxmarginBottom={1}><Textcolor="blue">❯ {state.streaming.userMessage}</Text></Box>)}{/* Streaming content */}{state.streaming.text&&(<Box><Text>{state.streaming.text}</Text></Box>)}ImpactWithout this fix, users will NOT see their submitted message, defeating the purpose of fixing issue #146 symptom #3 ("No echo of user message"). This must be fixed before merge. |
randomm
commented
Feb 5, 2026
🟡 Race Condition: Spinner Signal HandlersFile: IssueSignal handlers reference constcleanup=()=>{if(!stopped&&interval){// interval could be nullclearInterval(interval)process.stdout.write("\x1B[?25h")stopped=true}}process.on("SIGINT",cleanup)// Registered AFTER interval assignmentprocess.on("SIGTERM",cleanup)ProblemIf SIGINT/SIGTERM arrives between lines 13-20 (after Recommended FixMove signal handler registration before interval assignment: letinterval: Timer|null=nullletstopped=falseconststart=Date.now()constcleanup=()=>{if(stopped)returnstopped=trueif(interval)clearInterval(interval)process.stdout.write("\x1B[?25h")}// Register handlers FIRSTprocess.on("SIGINT",cleanup)process.on("SIGTERM",cleanup)process.stdout.write("\x1B[?25l")// THEN create intervalinterval=setInterval(()=>{// ... spinner logic},50)Priority: Medium - Edge case but can leave terminal in bad state. |
randomm
commented
Feb 5, 2026
🟡 MCP Lazy Loading: Non-Atomic State CheckFile: Issue
functioninitializeInBackground(){constinit=initState()if(init.initialized||init.failed||init.promise)return// ① Check// ② Async gap - other calls can enter hereinit.promise=Promise.resolve().then(()=>state())// ③ Async operation.then(()=>{init.initialized=true// ④ Mutation happens later})}ProblemMultiple concurrent calls to Potential FixUse immediate synchronous flag: functioninitializeInBackground(){constinit=initState()if(init.initialized||init.failed||init.promise)return// Set flag IMMEDIATELY (synchronous)constplaceholder=Promise.resolve()init.promise=placeholder// Then do async workinit.promise=placeholder.then(()=>state()).then(()=>{init.initialized=truelog.info("MCP initialization complete")}).catch((error)=>{init.failed=truelog.error("MCP initialization failed",{ error })})}Priority: Low-Medium - Theoretical race, unlikely in practice but worth fixing. |
randomm
commented
Feb 5, 2026
Code Review Summary: PR #147CI/CD Status✅ check-standards: PASS Issues Found🔴 CRITICAL (Must Fix Before Merge)
🟡 MEDIUM (Should Fix)
🟢 LOW (Consider Fixing)
What Works Well ✅
RecommendationsBefore merge:
Post-merge improvements:
VerdictChanges Requested - Critical issue prevents core functionality from working. State is managed correctly but UI doesn't show the user's message. The PR is 80% complete - just needs the UI rendering piece to match the state management work already done. |
randomm
commented
Feb 5, 2026
✅ Re-Review Complete: All Issues ResolvedFixed Issues1. ✅ User Message Display (CRITICAL) - FIXEDImplementation:
Verification: User message now visible immediately after submission ✅ 2. ✅ Spinner Race Condition (MEDIUM) - FIXEDImplementation:
Verification: No race conditions, proper cleanup on all exit paths ✅ Code Quality Analysis✅ Type Safety: No CI/CD Status
VerdictAll requested changes from previous review have been properly addressed. The implementation is production-ready with:
Ready for merge once check-duplicates completes (or is manually skipped if runner remains unavailable). |
Summary
Fixes critical regression in oclite TUI after PR #144 merge.
Changes
Testing
Fixes#146