Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA Terminal User Interface (TUI) system is introduced, replacing direct terminal-kit calls and console logging with a categorized logging infrastructure and a full-screen interactive UI. The Logger class is refactored to delegate to a new CategorizedLogger with event-driven updates. TUIManager orchestrates the TUI rendering, state management, and command processing. The main index.ts is updated to initialize and lifecycle-manage the TUI. Shell scripts and GitHub Actions workflows are added for legacy mode support and .beads file management. ASCII art assets and documentation complete the feature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Main Process
participant TUI as TUIManager
participant Log as CategorizedLogger
participant Term as Terminal (terminal-kit)
participant Node as Node App Logic
Main->>Log: CategorizedLogger.getInstance(config)
Main->>TUI: TUIManager.getInstance(config)
Main->>TUI: start()
TUI->>Term: initialize screen & render header/tab layout
TUI->>Log: enableTuiMode()
Node->>Log: log(level, category?, message)
Log->>Log: filter & write to files, buffer
Log->>Log: emit "log" event
Log->>TUI: log event received
TUI->>TUI: buffer & filter by active tab
TUI->>Term: render log area
Main->>TUI: updateNodeInfo({port, peers, block, status})
TUI->>Term: update header / status display
Term->>TUI: user key input
alt CMD mode
TUI->>TUI: parse command (help|quit|clear|peers|status)
TUI->>Log: query entries
TUI->>Term: display command result
else Nav / Tab switch
TUI->>TUI: change active tab / scroll
TUI->>Log: fetch entries
TUI->>Term: re-render log view
end
Term->>TUI: quit (Ctrl+C / quit command)
TUI->>Log: disableTuiMode()
TUI->>Term: restore terminal state
TUI->>Main: emit quit -> Main triggers graceful shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
.beads/issues.jsonl (1)
1-7: Consider sorting phases sequentially for readability.While the JSONL format doesn't enforce ordering, the entries are currently in the order: 1, 2, 5, 4, 3, 6, epic. Reordering them sequentially (1→2→3→4→5→6→epic) would improve human readability and make the progression clearer at a glance.
This is a minor organizational improvement and non-urgent—consider applying if you're already editing this file.
src/utilities/tui/CategorizedLogger.ts (3)
224-229: Config parameter is silently ignored after first instantiation.When
getInstance(config)is called after the singleton already exists, theconfigparameter is ignored without warning. This could lead to subtle bugs if callers expect their config to be applied.Consider logging a warning or throwing when config is passed to an already-initialized instance:
static getInstance(config?: LoggerConfig): CategorizedLogger { if (!CategorizedLogger.instance) { CategorizedLogger.instance = new CategorizedLogger(config) + } else if (config) { + // Optionally warn that config is ignored + console.warn("CategorizedLogger already initialized, config ignored") } return CategorizedLogger.instance }
200-200: UnusedfileHandlesmap and synchronous file writes.
- The
fileHandlesMap (line 200) is declared but never populated -appendToFileuses synchronousfs.appendFileSyncinstead of streams.closeFileHandles()iterates over an always-empty map, making it a no-op.- Synchronous file I/O can block the event loop under heavy logging load.
Either remove the unused
fileHandlesdeclaration or implement async file streams:- private fileHandles: Map<string, fs.WriteStream> = new Map()Or use async writes with buffering for production workloads:
private appendToFile(filename: string, content: string): void { const filepath = path.join(this.config.logsDir, filename) fs.appendFile(filepath, content, (err) => { if (err) console.error(`Failed to write to log file: ${filepath}`, err) }) }Also applies to: 429-438
570-583: Duplicate category list violates DRY principle.
getCategories()returns a hardcoded array identical toALL_CATEGORIES(lines 177-188). These could get out of sync.Reuse the existing constant:
static getCategories(): LogCategory[] { - return [ - "CORE", - "NETWORK", - "PEER", - "CHAIN", - "SYNC", - "CONSENSUS", - "IDENTITY", - "MCP", - "MULTICHAIN", - "DAHR", - ] + return [...ALL_CATEGORIES] }src/index.ts (2)
307-310: Duplicate--no-tuiflag parsing.The
--no-tuiflag is checked here (lines 307-310) and also indigestArguments()(lines 111-114). The early check here usesincludes("no-tui")which matches bothno-tuiand--no-tui, butdigestArgumentsusessplit("=")logic expecting a different format.Consider consolidating the flag handling:
- // Check for --no-tui flag early (before warmup processes args fully) - if (process.argv.includes("no-tui") || process.argv.includes("--no-tui")) { - indexState.TUI_ENABLED = false - } + // Check for --no-tui flag early (before warmup processes args fully) + if (process.argv.includes("--no-tui")) { + indexState.TUI_ENABLED = false + }And update
digestArgumentsto match:- case "no-tui": + // no-tui is handled early in main(), but keep for completeness + case "--no-tui": log.info("[MAIN] TUI disabled, using scrolling log output") indexState.TUI_ENABLED = false break
321-329: Hardcoded version string.The version
"1.0.0"is hardcoded. This will become stale and inconsistent with the actual package version.Consider reading from
package.json:import { version } from "../package.json" // or const packageJson = JSON.parse(fs.readFileSync("package.json", "utf8")) indexState.tuiManager.updateNodeInfo({ version: packageJson.version, // ... })src/utilities/tui/TUIManager.ts (3)
124-133: Consider graceful shutdown instead of immediate process.exit.The
quitcommand handler callsprocess.exit(0)after only 500ms. This may not allow sufficient time for cleanup operations (closing connections, flushing logs, etc.) to complete, potentially causing data loss or resource leaks.Consider emitting a shutdown event and letting the main application orchestrate the graceful shutdown:
{ name: "quit", description: "Exit the node", handler: (_args, tui) => { tui.addCmdOutput("Shutting down...") - setTimeout(() => { - tui.emit("quit") - tui.stop() - process.exit(0) - }, 500) + tui.emit("quit") + // Let the main application handle graceful shutdown + // It should call tui.stop() and then process.exit() after cleanup }, },
724-728: Consider delegating exit to main application.Similar to the
quitcommand handler,handleQuit()immediately callsprocess.exit(0)without allowing time for graceful shutdown. Consider emitting the event and letting the main application orchestrate the shutdown sequence.private handleQuit(): void { this.emit("quit") this.stop() - process.exit(0) + // Let main application handle process.exit after cleanup }
947-963: Make localhost detection more robust.The
checkIfStandalone()method uses case-sensitive string matching to detect localhost peers (lines 954-955). This may not catch all localhost variations:
- IPv6 localhost (
::1)- Case variations (
LOCALHOST,LocalHost)- IP variations (
127.*)const nonLocalPeers = peers.filter(peer => { const connStr = peer.connection?.string?.toLowerCase() || "" - return !connStr.includes("localhost") && !connStr.includes("127.0.0.1") + return !connStr.includes("localhost") && + !connStr.includes("127.0.0.1") && + !connStr.includes("::1") && + !/^127\./.test(connStr) })src/utilities/tui/LegacyLoggerAdapter.ts (3)
197-213: Remove incomplete terminal suppression logic.Lines 204-210 contain incomplete logic to suppress terminal output. The
prevTerminalvariable is declared but never used, and the comment indicates the implementation is incomplete. This dead code should be removed or completed.static info(message: string, logToTerminal = true): void { if (this.LOG_ONLY_ENABLED) return const { tag, cleanMessage } = extractTag(message) const category = inferCategory(tag) - // Temporarily adjust terminal output based on parameter - const config = this.logger.getConfig() - const prevTerminal = config.terminalOutput - - if (!logToTerminal && !this.logger.isTuiMode()) { - // In non-TUI mode, we need to suppress terminal for this call - // We'll emit the event but not print - } - this.logger.info(category, cleanMessage) }Note: The
logToTerminalparameter is effectively ignored. If this behavior is intentional, document it. Otherwise, implement proper suppression or remove the parameter.
258-291: Mixing of sync and async file operations.The
custom()method is async but uses synchronous file operations (fs.rmSync,fs.appendFileSync), which can block the event loop. Additionally, thefs.promises.writeFilecall at line 274 is not awaited.Either make the method fully synchronous or fully asynchronous:
static async custom( logfile: string, message: string, logToTerminal = true, cleanFile = false, ): Promise<void> { if (this.LOG_ONLY_ENABLED) return const customPath = `${this.LOG_CUSTOM_PREFIX}${logfile}.log` const timestamp = new Date().toISOString() const logEntry = `[INFO] [${timestamp}] ${message}\n` // Clean file if requested if (cleanFile) { try { - fs.rmSync(customPath, { force: true }) - await fs.promises.writeFile(customPath, "") + await fs.promises.rm(customPath, { force: true }) + await fs.promises.writeFile(customPath, "") } catch { // Ignore errors } } // Write to custom file try { - fs.appendFileSync(customPath, logEntry) + await fs.promises.appendFile(customPath, logEntry) } catch { // Ignore errors }
325-385: Consider async alternatives for file operations.Methods
cleanLogs(),getPublicLogs(), andgetDiagnostics()use synchronous file operations that can block the event loop. While these are likely called infrequently for diagnostic purposes, providing async alternatives would improve performance in high-throughput scenarios.Consider adding async versions:
static async cleanLogsAsync(withCustom = false): Promise<void> static async getPublicLogsAsync(): Promise<string> static async getDiagnosticsAsync(): Promise<string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.beads/issues.jsonl(1 hunks)res/demos_banner_ascii(1 hunks)res/demos_logo_ascii(1 hunks)res/demos_logo_ascii_bn(1 hunks)res/demos_logo_ascii_bn_small(1 hunks)res/demos_logo_ascii_bn_xsmall(1 hunks)src/index.ts(11 hunks)src/utilities/logger.ts(1 hunks)src/utilities/mainLoop.ts(1 hunks)src/utilities/tui/CategorizedLogger.ts(1 hunks)src/utilities/tui/LegacyLoggerAdapter.ts(1 hunks)src/utilities/tui/TUIManager.ts(1 hunks)src/utilities/tui/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/utilities/mainLoop.ts (1)
src/utilities/tui/CategorizedLogger.ts (1)
log(313-344)
src/index.ts (5)
src/utilities/logger.ts (2)
TUIManager(28-28)CategorizedLogger(26-26)src/utilities/tui/index.ts (2)
TUIManager(25-25)CategorizedLogger(11-11)src/utilities/tui/CategorizedLogger.ts (3)
log(313-344)CategorizedLogger(193-591)error(393-396)src/utilities/sharedState.ts (2)
getSharedState(266-268)publicKeyHex(94-107)src/libs/blockchain/routines/findGenesisBlock.ts (1)
findGenesisBlock(39-86)
src/utilities/tui/CategorizedLogger.ts (2)
src/utilities/logger.ts (4)
LogLevel(27-27)LogCategory(27-27)LogEntry(27-27)CategorizedLogger(26-26)src/utilities/tui/index.ts (5)
LogLevel(15-15)LogCategory(16-16)LogEntry(17-17)LoggerConfig(18-18)CategorizedLogger(11-11)
🔇 Additional comments (24)
.beads/issues.jsonl (2)
1-7: Approve: Comprehensive TUI tracking structure.The issue tracking data is well-structured with detailed design documentation, clear acceptance criteria, and properly defined dependencies. Phase statuses appropriately reflect work progression (completed phases closed, Phase 5 actively in progress, future phases open). The embedded design specs provide good context for implementation phases.
1-7: JSONL format and data integrity validated successfully.All 7 issue entries are valid JSON with complete required fields (id, title, description, design, acceptance_criteria, status, priority, issue_type, timestamps, labels, and dependencies). Dependency graph is coherent—all 24 dependency references resolve to existing issue IDs. The structure properly tracks the TUI implementation workflow across 6 phases plus epic, with appropriate status progression (5 closed, 1 in_progress, 2 open).
res/demos_logo_ascii_bn (1)
1-59: LGTM!Static ASCII art asset for the TUI branding. No code concerns.
res/demos_logo_ascii_bn_xsmall (1)
1-11: LGTM!Compact ASCII art variant for smaller terminal displays.
res/demos_logo_ascii_bn_small (1)
1-28: LGTM!Medium-sized ASCII art variant complements the other logo sizes.
res/demos_banner_ascii (1)
1-14: LGTM!ASCII text banner asset for TUI headers or startup display.
res/demos_logo_ascii (1)
1-59: LGTM!Full-size ASCII art logo using
@characters—good fallback for terminals with limited Unicode support.src/utilities/tui/CategorizedLogger.ts (6)
14-59: LGTM! Well-structured type definitions.The types and interfaces are well-designed with clear documentation. The
LogLevel,LogCategory,LogEntry, andLoggerConfigprovide a solid foundation for the categorized logging system.
66-139: LGTM! Correct ring buffer implementation.The circular buffer correctly handles wrap-around with head/tail pointers. The
push,getAll,getLast, andfiltermethods are implemented correctly.
460-473: LGTM! Terminal output with color coding.The timestamp extraction and color-coded output implementation is correct and provides good readability for terminal logs.
476-487: LGTM! Chronological ordering via entry IDs.Using
idfor sorting ensures correct chronological order when merging entries from multiple category buffers. The implementation is correct.
349-364: Filtered logs are not stored in buffers.The
shouldLogcheck happens before creating and storing theLogEntry. This means logs filtered by level or category are permanently lost, not just hidden from display. If the user changes filters, they won't see previously-filtered logs.Is this the intended behavior? If users should be able to see previously-filtered logs after changing filters, consider always storing entries and filtering only on retrieval:
debug(category: LogCategory, message: string): LogEntry | null { - if (!this.shouldLog("debug", category)) return null - return this.log("debug", category, message) + const entry = this.log("debug", category, message) + // Filtering happens in display/retrieval, not storage + return entry }Also applies to: 369-404
593-600: LGTM! Convenient singleton default export.The default export provides easy access to the logger instance while the class export allows for testing and direct instantiation control.
src/utilities/mainLoop.ts (1)
234-235: LGTM! Diagnostic output migrated to categorized logger.The change correctly routes diagnostic output through the new logging system with appropriate
debuglevel and[MAIN LOOP]prefix for category detection.src/utilities/tui/index.ts (1)
1-31: LGTM! Clean barrel file with proper type exports.The module provides a well-organized public API surface. Using
export typefor interfaces ensures proper tree-shaking and avoids runtime overhead.src/index.ts (3)
312-334: LGTM! TUI initialization with proper fallback.Good defensive coding with try/catch that falls back to standard output if TUI fails to start. The initial node info with placeholder values provides immediate visual feedback.
76-78: LGTM! Logging migration maintains consistency.The log calls correctly use the category tags (e.g.,
[SYNC],[MAIN]) that the LegacyLoggerAdapter will auto-detect for category mapping.
352-363: LGTM! Progressive TUI updates with node lifecycle.The node info is updated incrementally as information becomes available. Truncating the public key to 16 characters is a sensible UI choice.
src/utilities/logger.ts (1)
1-29: LGTM! Clean backward-compatibility wrapper with clear documentation.The module serves as an excellent migration bridge:
- Existing
import log from "src/utilities/logger"continues to work viaLegacyLoggerAdapter- New code can use
CategorizedLoggerdirectly- Type exports enable proper TypeScript usage
- Documentation provides clear guidance for gradual migration
src/utilities/tui/TUIManager.ts (3)
681-701: LGTM - Command execution is safe.The command execution logic properly validates commands against a predefined whitelist (
COMMANDSarray) and doesn't execute arbitrary code. Unknown commands are safely rejected.
289-293: LGTM - Proper interval cleanup.The refresh interval is properly cleaned up in the
stop()method with appropriate null checks.Also applies to: 305-309
706-712: LGTM - Bounded buffer prevents memory leak.The CMD output buffer is properly bounded to 500 lines, preventing unbounded memory growth.
src/utilities/tui/LegacyLoggerAdapter.ts (2)
165-192: LGTM - Proper error handling and fallback.The
setLogsDir()method has appropriate error handling with fallback to a default directory.
387-409: LGTM - Clean API delegation.The new API access methods provide clean delegation to the underlying
CategorizedLoggerinstance, supporting the migration path mentioned in the file header.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/utilities/tui/TUIManager.ts (2)
262-295: Event listeners are not explicitly removed instop().The
setupInputHandlers()andsetupResizeHandler()methods register event listeners viaterm.on("key", ...)andterm.on("resize", ...)without storing references. Thestop()method callsterm.grabInput(false)but does not explicitly remove these listeners. If the TUI is stopped and restarted, listeners will accumulate.This was flagged in a previous review. Store listener references and call
term.off()instop()to prevent listener accumulation:+ private keyHandler: ((key: string) => void) | null = null + private resizeHandler: ((width: number, height: number) => void) | null = null private setupInputHandlers(): void { - term.on("key", (key: string) => { - this.handleKeyPress(key) - }) + this.keyHandler = (key: string) => this.handleKeyPress(key) + term.on("key", this.keyHandler) } private setupResizeHandler(): void { - term.on("resize", (width: number, height: number) => { + this.resizeHandler = (width: number, height: number) => { this.width = width this.height = height this.logAreaHeight = this.height - HEADER_HEIGHT - TAB_HEIGHT - FOOTER_HEIGHT this.render() - }) + } + term.on("resize", this.resizeHandler) } stop(): void { // ... existing code ... + if (this.keyHandler) term.off("key", this.keyHandler) + if (this.resizeHandler) term.off("resize", this.resizeHandler) + this.keyHandler = null + this.resizeHandler = null // ... rest of stop ... }
330-404:TAG_TO_CATEGORYmapping is duplicated fromLegacyLoggerAdapter.ts.This mapping (lines 332-392) duplicates the one in
LegacyLoggerAdapter.ts(lines 20-117). This violates DRY and creates maintenance burden when adding new tags.This was flagged in a previous review. Extract the mapping and helper functions to a shared module:
// src/utilities/tui/tagMapping.ts export const TAG_TO_CATEGORY: Record<string, LogCategory> = { /* ... */ } export function extractTag(message: string): { tag: string | null; cleanMessage: string } { /* ... */ } export function inferCategory(tag: string | null): LogCategory { /* ... */ }
🧹 Nitpick comments (6)
src/utilities/tui/TUIManager.ts (2)
58-79: UnusedCOLORSconstant.The
COLORSobject is defined but never referenced in the code. The rendering methods use inline terminal-kit color methods (e.g.,term.cyan(),term.green()) instead of this constant.Either remove the unused constant or refactor the rendering methods to use it for consistency:
-const COLORS = { - // Status colors - statusRunning: "green", - statusSyncing: "yellow", - statusStopped: "red", - statusError: "brightRed", - - // Log level colors - logDebug: "magenta", - logInfo: "white", - logWarning: "yellow", - logError: "red", - logCritical: "brightRed", - - // UI colors - border: "cyan", - header: "brightCyan", - tabActive: "brightWhite", - tabInactive: "gray", - footer: "gray", - footerKey: "brightYellow", -}
1423-1438:getStatusColor()method is defined but never used.The helper method
getStatusColor()is implemented but not called anywhere in the codebase. Either remove it or use it in the status rendering logic.- /** - * Get status color based on node status - */ - private getStatusColor(): string { - switch (this.nodeInfo.status) { - case "running": - return "green" - case "syncing": - return "yellow" - case "starting": - return "cyan" - case "stopped": - return "gray" - case "error": - return "red" - default: - return "white" - } - }src/utilities/tui/CategorizedLogger.ts (4)
200-201:fileHandlesmap is populated but never used.The
fileHandlesproperty is declared and cleared incloseFileHandles(), butappendToFile()usesfs.promises.appendFile()directly without creating or storing write streams. This is dead code.Remove the unused property and method, or implement proper stream-based file writing for better performance:
- private fileHandles: Map<string, fs.WriteStream> = new Map() - /** - * Close all file handles - */ - private closeFileHandles(): void { - for (const stream of this.fileHandles.values()) { - stream.close() - } - this.fileHandles.clear() - }Also update
resetInstance():static resetInstance(): void { if (CategorizedLogger.instance) { - CategorizedLogger.instance.closeFileHandles() CategorizedLogger.instance = null } }
479-486:getAllEntries()performance concern with large buffers.Each call to
getAllEntries()merges all category buffers and sorts by ID. With 10 categories × 500 entries = 5000 entries, this is O(n log n) per call. The render loop calls this frequently viaupdateFilteredLogs().Consider caching the merged result and invalidating on new log entry:
private mergedEntriesCache: LogEntry[] | null = null private log(...): LogEntry { // ... existing code ... this.mergedEntriesCache = null // Invalidate cache return entry } getAllEntries(): LogEntry[] { if (this.mergedEntriesCache) return this.mergedEntriesCache const allEntries: LogEntry[] = [] for (const buffer of this.categoryBuffers.values()) { allEntries.push(...buffer.getAll()) } this.mergedEntriesCache = allEntries.sort((a, b) => a.id - b.id) return this.mergedEntriesCache }
569-582:getCategories()duplicatesALL_CATEGORIESconstant.The static method returns a hardcoded array that duplicates the
ALL_CATEGORIESconstant defined at line 177-188.Reuse the existing constant:
static getCategories(): LogCategory[] { - return [ - "CORE", - "NETWORK", - "PEER", - "CHAIN", - "SYNC", - "CONSENSUS", - "IDENTITY", - "MCP", - "MULTICHAIN", - "DAHR", - ] + return [...ALL_CATEGORIES] }
594-599: Default singleton export may cause issues with testing.Exporting a pre-instantiated singleton as the default export makes it harder to test code that imports this module, as the singleton is created at import time.
Consider documenting that
CategorizedLogger.getInstance()should be preferred over the default export for testability, or remove the default instance export:-// SECTION Default Export - Singleton Instance - -/** - * Default logger instance - */ -const logger = CategorizedLogger.getInstance() - -export default logger +// Use CategorizedLogger.getInstance() to obtain the singleton instance +export default CategorizedLogger
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/utilities/tui/CategorizedLogger.ts(1 hunks)src/utilities/tui/LegacyLoggerAdapter.ts(1 hunks)src/utilities/tui/TUIManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utilities/tui/CategorizedLogger.ts (2)
src/utilities/logger.ts (4)
LogLevel(27-27)LogCategory(27-27)LogEntry(27-27)CategorizedLogger(26-26)src/utilities/tui/index.ts (5)
LogLevel(15-15)LogCategory(16-16)LogEntry(17-17)LoggerConfig(18-18)CategorizedLogger(11-11)
🔇 Additional comments (3)
src/utilities/tui/CategorizedLogger.ts (2)
66-139: Well-implemented ring buffer with clear semantics.The
RingBuffer<T>class is correctly implemented with proper circular buffer logic, O(1) push operations, and efficient retrieval methods.
193-219: Per-category buffers prevent log starvation - good design.Using separate
RingBufferinstances per category ensures that a noisy category (e.g., PEER) won't evict logs from quieter categories. This is a thoughtful design choice.src/utilities/tui/TUIManager.ts (1)
1080-1081: No issue found—the code is safe.
getSharedStateis a getter that always returns a validSharedStatesingleton instance viagetInstance(). ThelastBlockNumberproperty is always defined (initialized to0) and never null or undefined. The nullish coalescing operator on line 1080 is unnecessary but harmless, as it will never fall back tothis.nodeInfo.blockNumber.
Capture console.error at module initialization to prevent TUI interception when logging file write failures. This avoids infinite recursion if the TUI patches console methods and routes them back through the logger. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create src/utilities/tui/tagCategories.ts as single source of truth - Update LegacyLoggerAdapter.ts to import from shared module - Update TUIManager.ts to import from shared module (also fixes missing tags) The TUIManager had a partial copy missing ~25 tag mappings. Now both files use the complete authoritative mapping from one location. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…exit - Remove process.exit(0) from TUIManager.handleQuit() - Add "quit" event listener in index.ts for application-level shutdown - Implement 5-second timeout fallback for forced termination - Add placeholder cleanup for peerManager and mcpServer The TUI now signals intent via event, letting the application perform cleanup (flush writes, close connections) before exiting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…rence The only() method was saving console.log to originalLog then replacing it with a no-op, but then calling console.log (the no-op) to print the styled "ONLY" message. Now uses this.originalLog to ensure messages actually appear. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utilities/tui/TUIManager.ts (1)
853-861: Verify control flow prevents CMD category in getEntriesByCategory.The control flow in
setActiveTab()(lines 716-732) setsisCmdMode = truewhen the CMD tab is active and only callsupdateFilteredLogs()for non-CMD tabs (line 732). However,updateFilteredLogs()is also called fromhandleLogEntry()(line 841) without checkingisCmdMode.If
handleLogEntry()is triggered while the CMD tab is active,updateFilteredLogs()will attempt to callgetEntriesByCategory(activeTab.category)with"CMD", which is not a validLogCategoryand will cause a TypeScript error or runtime issue.Apply this guard:
private updateFilteredLogs(): void { const activeTab = TABS[this.activeTabIndex] if (activeTab.category === "ALL") { this.filteredLogs = this.logger.getAllEntries() + } else if (activeTab.category === "CMD") { + // CMD tab doesn't use filtered logs + this.filteredLogs = [] } else { this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) } }
🧹 Nitpick comments (2)
src/utilities/tui/TUIManager.ts (2)
894-910: Consider more robust standalone detection.The
checkIfStandalone()method checks for localhost/127.0.0.1 peers by examining the connection string withincludes(). This could produce false positives if a peer has "localhost" or "127.0.0.1" elsewhere in the connection string (e.g., in a query parameter or path).Consider using URL parsing for more accurate detection:
private checkIfStandalone(): boolean { try { const peers = PeerManager.getInstance().getPeers() if (peers.length === 0) return true // Check if all peers are localhost const nonLocalPeers = peers.filter(peer => { const connStr = peer.connection?.string?.toLowerCase() || "" - return !connStr.includes("localhost") && !connStr.includes("127.0.0.1") + try { + const url = new URL(connStr) + const hostname = url.hostname + return hostname !== "localhost" && hostname !== "127.0.0.1" + } catch { + // If URL parsing fails, use includes as fallback + return !connStr.includes("localhost") && !connStr.includes("127.0.0.1") + } }) return nonLocalPeers.length === 0 } catch { // If we can't get peers, assume standalone return true } }
1362-1377: Potentially unused helper method.The
getStatusColor()method is defined but doesn't appear to be used anywhere in the file. The rendering methods use direct color calls (e.g.,term.bgGreen) rather than this helper.Consider removing this unused method or documenting its intended future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/index.ts(11 hunks)src/utilities/tui/CategorizedLogger.ts(1 hunks)src/utilities/tui/LegacyLoggerAdapter.ts(1 hunks)src/utilities/tui/TUIManager.ts(1 hunks)src/utilities/tui/tagCategories.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utilities/tui/LegacyLoggerAdapter.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/utilities/tui/TUIManager.ts (3)
src/utilities/tui/tagCategories.ts (2)
LogCategory(114-114)TAG_TO_CATEGORY(14-111)src/utilities/tui/CategorizedLogger.ts (4)
LogCategory(25-35)info(380-383)CategorizedLogger(196-593)LogEntry(40-46)src/utilities/sharedState.ts (1)
getSharedState(266-268)
🔇 Additional comments (21)
src/utilities/tui/tagCategories.ts (1)
1-114: LGTM! Clean single source of truth for tag categorization.This module successfully extracts the tag-to-category mapping into a shared constant, addressing the DRY concern raised in previous reviews. The mapping is comprehensive, well-organized by category, and easy to maintain.
src/index.ts (8)
19-19: LGTM! Clean TUI state integration.The addition of
TUI_ENABLEDandtuiManagerstate fields is well-structured, with appropriate defaults (TUI enabled by default, manager initially null) and proper typing (TUIManager | null).Also applies to: 39-40, 51-51, 56-56, 68-68
76-78: LGTM! Comprehensive logging migration in warmup flow.The migration from console/terminal-kit to the new logger API is thorough and consistent. The addition of the
--no-tuiflag handling at line 111-114 provides a clean fallback mechanism.Also applies to: 83-120, 158-226
307-369: TUI initialization looks solid with proper error handling.The TUI initialization sequence is well-structured:
- Early
--no-tuidetection prevents unnecessary setup- Graceful fallback to standard output on TUI startup failure
- The quit event handler implements proper cleanup with a 5-second timeout fallback
The cleanup sequence correctly calls
process.exit(0)in thefinallyblock rather than directly, which aligns with the graceful shutdown pattern recommended in previous reviews.
375-380: LGTM! Clean TUI state update after warmup.The pattern of conditionally updating TUI node info after warmup is correct, with proper null checks and partial updates.
229-303: LGTM! Consistent logging throughout preMainLoop.The logging migration in
preMainLoopis thorough and uses appropriate log levels (info for normal flow, warning for potential issues, debug for detailed information).
387-398: LGTM! Comprehensive TUI state update after preMainLoop.The node info update captures key runtime state (identity, peers, block number) from appropriate sources and sets a reasonable initial status of "syncing". The public key truncation (first 16 chars) is suitable for header display.
402-460: LGTM! Clear logging for server startup sequence.The signaling and MCP server initialization flows use appropriate log levels and maintain the failsafe pattern for MCP startup failures.
461-469: LGTM! Final TUI state update before main loop.The final node info update correctly transitions to "running" status and reflects the current sync state from shared state. Well-placed just before entering the main loop.
src/utilities/tui/TUIManager.ts (6)
1-103: LGTM! Well-structured types and configuration.The type definitions (
NodeInfo,TUIConfig,Tab) and constants (layout, colors, tabs) provide a clean foundation for the TUI. The 12-tab configuration covers all log categories plus special ALL and CMD tabs.
105-175: LGTM! Comprehensive command system.The command definitions provide a useful set of built-in commands (help, quit, clear, status, peers, sync) with clear handlers. The command system architecture allows easy extension.
177-248: LGTM! Solid singleton initialization with comprehensive state management.The TUIManager constructor properly initializes all state including per-tab scroll offsets, CMD terminal state, and logger event subscriptions. The singleton pattern with
resetInstance()for testing is a good practice.
328-405: LGTM! Console interception with proper safeguards.The console interception implementation correctly:
- Uses the shared
TAG_TO_CATEGORYmapping imported fromtagCategories.ts(addressing the DRY concern from previous reviews)- Guards against double-interception at lines 349-350 (addressing the previous concern)
- Routes console output through the categorized logger with tag detection
- Properly restores original console methods in
stop()
425-622: LGTM! Comprehensive input handling with proper history management.The input handling system correctly:
- Manages both normal and CMD modes
- Implements comprehensive keyboard shortcuts (navigation, tabs, scrolling)
- Includes command history with a 100-entry limit (lines 568-570, addressing the previous memory leak concern)
- Handles edge cases like empty input and special keys
624-675: LGTM! Command execution and graceful quit handling.The command execution and quit handling are well-implemented:
- Commands are parsed and dispatched correctly
- CMD output buffer is managed with a 500-line limit
handleQuit()correctly emits the "quit" event and callsstop()without directly callingprocess.exit(), following the graceful shutdown pattern confirmed in previous reviewssrc/utilities/tui/CategorizedLogger.ts (6)
1-62: LGTM! Clean type definitions with recursion prevention.The module correctly captures
originalConsoleErrorat initialization (line 13) to prevent TUI interception recursion, addressing the concern from previous reviews. The type definitions forLogLevel,LogCategory,LogEntry, andLoggerConfigare comprehensive and well-documented.
64-142: LGTM! Solid RingBuffer implementation.The circular buffer implementation correctly manages fixed-size storage with proper head/tail pointer updates, overflow handling, and utility methods (getAll, getLast, filter, clear).
144-310: LGTM! Well-designed logger configuration with per-category buffers.The logger correctly initializes per-category ring buffers (line 218-221), preventing log loss when one category is highly active. The TUI mode toggle and configuration methods are clean and thread-safe.
312-407: LGTM! Comprehensive logging methods with proper filtering.The core logging flow correctly:
- Filters by level and category via
shouldLog()- Stores entries in per-category ring buffers
- Emits events for TUI integration
- Writes to files asynchronously
- Conditionally outputs to terminal based on TUI mode
409-475: LGTM! File logging with proper recursion prevention.The file logging implementation correctly:
- Writes to multiple log files (all.log, level-specific, category-specific)
- Uses asynchronous file writes with
fs.promises.appendFile()for non-blocking I/O- Uses
originalConsoleErrorin the catch handler (line 438) to prevent TUI interception recursion, addressing the concern from previous reviews- Provides proper cleanup via
closeFileHandles()
477-602: LGTM! Comprehensive buffer access and utility methods.The logger provides extensive buffer access methods:
- Merging entries from all categories with chronological sorting by ID
- Filtering by category, level, or both
- Buffer management (clear, size)
- File cleanup utilities
- Static helpers for introspection
The default export of a singleton instance provides a convenient ready-to-use logger.
Store listener callbacks as class properties and remove them in stop() to prevent memory leaks and multiple handler executions when TUI is restarted. Null out references after removal to avoid retained closures. - Add keyListener and resizeListener properties - Store callbacks before attaching with term.on() - Remove listeners with term.off() in stop() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Change unbounded quantifier [A-Za-z0-9_ ]+ to bounded {1,50} to prevent
super-linear backtracking on malicious input. Add .trim() to handle any
trailing spaces in captured tags.
Affects extractTag() in LegacyLoggerAdapter.ts and
extractCategoryFromMessage() in TUIManager.ts.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utilities/tui/TUIManager.ts (1)
869-877: GuardupdateFilteredLogs()against"CMD"tab to avoid invalid category usage.
Tab.categoryisLogCategory | "ALL" | "CMD", butlogger.getEntriesByCategoryexpects aLogCategory. When the active tab is"CMD",updateFilteredLogs()can end up callinggetEntriesByCategory("CMD")(e.g., viahandleLogEntry), which is outside the declared log categories and may breakCategorizedLoggerdepending on its implementation.Add an explicit guard for
"CMD"so the logger never sees that pseudo-category:private updateFilteredLogs(): void { const activeTab = TABS[this.activeTabIndex] - if (activeTab.category === "ALL") { - this.filteredLogs = this.logger.getAllEntries() - } else { - this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) - } + if (activeTab.category === "ALL") { + this.filteredLogs = this.logger.getAllEntries() + } else if (activeTab.category === "CMD") { + // CMD tab doesn't show log entries + this.filteredLogs = [] + } else { + this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) + } }
🧹 Nitpick comments (4)
src/utilities/tui/TUIManager.ts (4)
217-231: Unsubscribe the logger"log"listener onstop()/resetInstance()to avoid leaks across instances.The constructor registers a
"log"listener with:this.logger.on("log", this.handleLogEntry.bind(this))but
stop()/resetInstance()never remove it. SinceresetInstance()can create a newTUIManagerwhile the old instance’s listener remains attached, logger events will keep references to the old instance and callhandleLogEntryon it indefinitely.Store the bound listener and remove it during
stop():export class TUIManager extends EventEmitter { @@ - private logger: CategorizedLogger + private readonly logger: CategorizedLogger + private logListener: ((entry: LogEntry) => void) | null = null @@ - this.logger = CategorizedLogger.getInstance() - - this.nodeInfo = { + this.logger = CategorizedLogger.getInstance() + + this.logListener = this.handleLogEntry.bind(this) + this.logger.on("log", this.logListener) + + this.nodeInfo = { @@ stop(): void { if (!this.isRunning) return @@ - // Disable TUI mode in logger - this.logger.disableTuiMode() + // Disable TUI mode in logger and detach listener + this.logger.disableTuiMode() + if (this.logListener) { + this.logger.off("log", this.logListener) + this.logListener = null + }
resetInstance()already callsstop(), so this will clean up listeners across start/stop cycles and tests.Also applies to: 246-251, 305-340
569-637: Consider splittinghandleCmdInputto reduce complexity and match Sonar’s threshold.
handleCmdInputhas a fairly largeswitchonkeyand Sonar flags its cognitive complexity (17 vs allowed 15). Functionally it looks correct, but you can reduce complexity and improve readability by extracting small helpers, e.g.:
navigateCmdHistory(direction: "up" | "down")executeCurrentCmd()clearOrQuitOnCtrlC()Example sketch:
private executeCurrentCmdInput(): void { this.executeCommand(this.cmdInput) if (this.cmdInput.trim()) { this.cmdHistory.push(this.cmdInput) if (this.cmdHistory.length > 100) this.cmdHistory.shift() } this.cmdHistoryIndex = this.cmdHistory.length this.cmdInput = "" this.render() }and call
this.executeCurrentCmdInput()from the"ENTER"case. Similar extraction for history navigation would satisfy Sonar without changing behavior.
57-80: Tighten up unused config/color helpers (optional cleanup).
COLORS,TUIConfig.debugMode, andgetStatusColor()are currently unused in this file; colors and status rendering are all hard-coded viaterm.*calls.To avoid dead code and keep the API surface minimal:
- Either wire these through (e.g., drive header/status colors off
COLORS/getStatusColor()and usedebugModeto toggle footer hints), or- Remove them from this first version to reduce noise and add them back when they’re actually needed.
Also applies to: 183-215, 1378-1393
362-405: Consider usingutil.inspectfor object rendering and mappingconsole.logto info level.The current interception uses
String(a)which yields"[object Object]"for non-string arguments, making structured logs harder to debug. Similarly, routingconsole.logtologger.debug()hides third-party library output when running at info level.Both refinements align with Node.js logging best practices:
Object rendering: Use
util.inspect()with controlled depth to preserve object structure:import util from "util" const message = args .map(arg => (typeof arg === "string" ? arg : util.inspect(arg, { depth: 3, colors: false }))) .join(" ")Severity mapping: Map
console.logtoinfolevel since it represents user-visible output:- this.logger.debug(category, `[console.log] ${cleanMessage}`) + this.logger.info(category, `[console.log] ${cleanMessage}`)These changes improve interoperability with third-party libraries and make console output more useful in production logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/utilities/tui/TUIManager.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
src/utilities/tui/TUIManager.ts
[warning] 383-383: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 401-401: arrow function is equivalent to String. Use String directly.
[warning] 347-347: Use the "RegExp.exec()" method instead.
[warning] 186-186: Member 'scrollOffsets' is never reassigned; mark it as readonly.
[failure] 569-569: Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.
[warning] 377-377: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 401-401: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 183-183: Member 'config' is never reassigned; mark it as readonly.
[warning] 395-395: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 389-389: arrow function is equivalent to String. Use String directly.
[warning] 395-395: arrow function is equivalent to String. Use String directly.
[warning] 383-383: arrow function is equivalent to String. Use String directly.
[warning] 480-480: Prefer Number.parseInt over parseInt.
[warning] 182-182: Member 'logger' is never reassigned; mark it as readonly.
[warning] 389-389: 'a' will use Object's default stringification format ('[object Object]') when stringified.
Extract switch case handlers into dedicated methods: - handleCmdEscape, handleCmdEnter, handleCmdBackspace - handleCmdHistoryUp, handleCmdHistoryDown - handleCmdCtrlC, handleCmdCharInput - addToHistory (extracted nested logic) Use early returns and null-safe operators (??) to simplify control flow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace parseInt(key) with Number.parseInt(key, 10) for consistency and explicit radix parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/utilities/tui/TUIManager.ts (3)
24-39: Unify tag parsing/category inference betweenTUIManagerandLegacyLoggerAdapter.Both
TUIManager.extractCategoryFromMessage()andLegacyLoggerAdapter’sextractTag/inferCategoryuse effectively the same regex and mapping logic, now backed by a sharedTAG_TO_CATEGORYtable. This duplication makes it easy for the two paths to drift.Consider moving the tag‑parsing helpers into the shared
tagCategories.ts(or a small sibling liketagParsing.ts) and reusing them in both places, e.g.:
- In
tagCategories.tsexport:
extractTag(message): { tag: string | null; cleanMessage: string }inferCategory(tag: string | null): LogCategory- In
TUIManager, replaceextractCategoryFromMessagewith a call to these helpers.- In
LegacyLoggerAdapter, reuse the same helpers instead of its local implementations.That way, changes to tag syntax or categorization are centralized in one place.
Also applies to: 346-358
125-135: CMDquitcommand reintroduces directprocess.exit(0), bypassing graceful shutdown.The
quitcommand directly callsprocess.exit(0)after emitting"quit"andtui.stop(), whilehandleQuit()is explicitly designed to let the main app handle shutdown via the"quit"event. This duplicates logic, bypasses any async cleanup the app may perform, and partially undoes the earlier refactor that removed direct exits fromhandleQuit().Consider delegating the command to
handleQuit()and making that method callable from the command layer:@@ { name: "quit", description: "Exit the node", handler: (_args, tui) => { tui.addCmdOutput("Shutting down...") setTimeout(() => { - tui.emit("quit") - tui.stop() - process.exit(0) + // Reuse the same graceful shutdown path as keyboard-based quit + tui.handleQuit() }, 500) }, }, @@ - private handleQuit(): void { + handleQuit(): void { this.stop() this.emit("quit") }This keeps all quit flows going through a single, graceful path and leaves responsibility for actually exiting the process to the main application.
Also applies to: 689-692
871-878: GuardupdateFilteredLogs()against"CMD"tab category to avoid invalid logger calls.
activeTab.categoryis typed asLogCategory | "ALL" | "CMD", butupdateFilteredLogs()treats everything except"ALL"as a validLogCategoryand passes it togetEntriesByCategory. When the active tab is"CMD", this will pass"CMD"into the logger, which is neither a validLogCategorynor something the logger expects.Add an explicit guard for the
"CMD"tab so the logger only receives real categories:private updateFilteredLogs(): void { const activeTab = TABS[this.activeTabIndex] if (activeTab.category === "ALL") { this.filteredLogs = this.logger.getAllEntries() + } else if (activeTab.category === "CMD") { + // CMD tab does not use the log area + this.filteredLogs = [] } else { this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) } }This matches the intent that CMD mode uses its own output buffer and avoids passing unsupported categories into the logger.
src/utilities/tui/LegacyLoggerAdapter.ts (2)
100-116:info()ignoreslogToTerminaland contains dead config code.The
info()method currently:
- Computes
configandprevTerminalfromthis.logger.getConfig()- Checks
!logToTerminal && !this.logger.isTuiMode()- But never actually changes logger behavior based on
logToTerminalSo the parameter is effectively ignored while leaving confusing, unused code around.
Either fully implement the “no terminal output for this call” behavior, or simplify by explicitly ignoring the parameter. If you don’t need per-call suppression, a minimal cleanup would be:
- static info(message: string, logToTerminal = true): void { + static info(message: string, _logToTerminal = true): void { if (this.LOG_ONLY_ENABLED) return const { tag, cleanMessage } = extractTag(message) const category = inferCategory(tag) - - // Temporarily adjust terminal output based on parameter - const config = this.logger.getConfig() - const prevTerminal = config.terminalOutput - - if (!logToTerminal && !this.logger.isTuiMode()) { - // In non-TUI mode, we need to suppress terminal for this call - // We'll emit the event but not print - } this.logger.info(category, cleanMessage) }If legacy callers rely on
logToTerminal, consider wiring it through the underlying logger’s config in a try/finally instead.
240-257:cleanLogs()duplicates cleanup already handled byCategorizedLogger.
cleanLogs()delegates tothis.logger.cleanLogs(withCustom)and then immediately performs its own directory scan andrmSyncloop underthis.LOGS_DIR. This duplicates responsibility, increases I/O surface, and can race with the logger’s own file operations.Given that
CategorizedLogger.cleanLogs()is the new source of truth, consider dropping the legacy manual removal logic:static cleanLogs(withCustom = false): void { this.logger.cleanLogs(withCustom) - - // Also clean using legacy paths for compatibility - if (fs.existsSync(this.LOGS_DIR)) { - const files = fs.readdirSync(this.LOGS_DIR) - for (const file of files) { - if (file.startsWith("custom_") && !withCustom) { - continue - } - try { - fs.rmSync(`${this.LOGS_DIR}/${file}`, { force: true }) - } catch { - // Ignore errors - } - } - } }If some deployments still depend on the legacy behavior, gate the extra cleanup behind an explicit flag rather than running it unconditionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/utilities/tui/LegacyLoggerAdapter.ts(1 hunks)src/utilities/tui/TUIManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/utilities/tui/TUIManager.ts (4)
src/utilities/tui/tagCategories.ts (2)
LogCategory(114-114)TAG_TO_CATEGORY(14-111)src/utilities/tui/CategorizedLogger.ts (3)
LogCategory(25-35)CategorizedLogger(196-593)LogEntry(40-46)src/libs/peer/PeerManager.ts (1)
PeerManager(20-423)src/utilities/sharedState.ts (1)
getSharedState(266-268)
src/utilities/tui/LegacyLoggerAdapter.ts (3)
src/utilities/tui/tagCategories.ts (2)
LogCategory(114-114)TAG_TO_CATEGORY(14-111)src/utilities/tui/CategorizedLogger.ts (2)
LogCategory(25-35)CategorizedLogger(196-593)src/utilities/sharedState.ts (1)
getSharedState(266-268)
🪛 GitHub Check: SonarCloud Code Analysis
src/utilities/tui/TUIManager.ts
[warning] 349-349: Remove duplicates in this character class.
[warning] 182-182: Member 'logger' is never reassigned; mark it as readonly.
[warning] 385-385: arrow function is equivalent to String. Use String directly.
[warning] 403-403: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 349-349: Use the "RegExp.exec()" method instead.
[warning] 633-633: Prefer String#codePointAt() over String#charCodeAt().
[warning] 9-9: Prefer node:events over events.
[warning] 183-183: Member 'config' is never reassigned; mark it as readonly.
[warning] 379-379: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 385-385: 'a' will use Object's default stringification format ('[object Object]') when stringified.
[warning] 391-391: arrow function is equivalent to String. Use String directly.
src/utilities/tui/LegacyLoggerAdapter.ts
[warning] 26-26: Use the "RegExp.exec()" method instead.
[warning] 54-54: Make this public static property readonly.
[warning] 57-57: Make this public static property readonly.
[warning] 52-52: Make this public static property readonly.
[warning] 60-60: Make this public static property readonly.
[warning] 26-26: Remove duplicates in this character class.
[warning] 55-55: Make this public static property readonly.
[warning] 50-50: Make this public static property readonly.
[warning] 53-53: Make this public static property readonly.
[warning] 108-108: Remove this useless assignment to variable "prevTerminal".
[warning] 47-47: Member 'logger' is never reassigned; mark it as readonly.
[warning] 51-51: Make this public static property readonly.
Add LogCategory to import from tagCategories.ts for explicit type resolution. The code worked before due to TypeScript inference, but explicit imports are clearer and safer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/utilities/tui/TUIManager.ts (2)
125-135:quitcommand still callsprocess.exit(0)directly, bypassing graceful shutdown.This was flagged in a previous review. The
quitcommand handler (line 132) callsprocess.exit(0)directly aftertui.stop(), which bypasses any asynchronous cleanup the main application might perform in response to the"quit"event. This is inconsistent withhandleQuit()(lines 714-717) which was fixed to delegate shutdown to the application.Consider reusing the graceful shutdown path:
{ name: "quit", description: "Exit the node", handler: (_args, tui) => { tui.addCmdOutput("Shutting down...") setTimeout(() => { - tui.emit("quit") - tui.stop() - process.exit(0) + // Reuse the same graceful shutdown path + ;(tui as any).handleQuit() }, 500) }, },Alternatively, make
handleQuit()public and call it directly.
896-904:getEntriesByCategorymay receive invalid"CMD"category.When the CMD tab is active,
activeTab.categoryis"CMD", which is not a validLogCategorytype. While this code path may not execute in practice (CMD tab usesrenderCmdAreainstead of log filtering), the type system doesn't enforce this, and the call at line 902 could fail or produce unexpected results.Add an explicit guard for the CMD category:
private updateFilteredLogs(): void { const activeTab = TABS[this.activeTabIndex] if (activeTab.category === "ALL") { this.filteredLogs = this.logger.getAllEntries() + } else if (activeTab.category === "CMD") { + // CMD tab doesn't use filtered logs + this.filteredLogs = [] } else { this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) } }src/utilities/tui/LegacyLoggerAdapter.ts (2)
100-116: Unused variablesconfigandprevTerminalwith incomplete implementation.The
info()method retrieves config and storesprevTerminal(lines 107-108) but never uses them. The comment at lines 110-113 indicates an intent to conditionally suppress terminal output, but the implementation is incomplete—the if-block is empty.Either complete the implementation or remove the dead code:
static info(message: string, logToTerminal = true): void { if (this.LOG_ONLY_ENABLED) return const { tag, cleanMessage } = extractTag(message) const category = inferCategory(tag) - // Temporarily adjust terminal output based on parameter - const config = this.logger.getConfig() - const prevTerminal = config.terminalOutput - - if (!logToTerminal && !this.logger.isTuiMode()) { - // In non-TUI mode, we need to suppress terminal for this call - // We'll emit the event but not print - } - this.logger.info(category, cleanMessage) }
240-257: Double file cleanup may cause race conditions or redundant I/O.This was flagged in a previous review. The method calls
this.logger.cleanLogs(withCustom)then immediately performs manual file deletion in the same directory. This could cause issues if the logger's cleanup is async or if files are being written concurrently.Consider removing the legacy cleanup if
CategorizedLogger.cleanLogs()already handles this:static cleanLogs(withCustom = false): void { this.logger.cleanLogs(withCustom) - - // Also clean using legacy paths for compatibility - if (fs.existsSync(this.LOGS_DIR)) { - const files = fs.readdirSync(this.LOGS_DIR) - for (const file of files) { - if (file.startsWith("custom_") && !withCustom) { - continue - } - try { - fs.rmSync(`${this.LOGS_DIR}/${file}`, { force: true }) - } catch { - // Ignore errors - } - } - } }
🧹 Nitpick comments (3)
.beads/issues.jsonl (1)
1-13: Optional: Consider reorganizing issues by topic or sequence.The TUI phases appear non-sequentially (1, 2, 5, 4, 3, 6) with ZK Identity phases interleaved throughout. While JSONL files are often append-only, reorganizing by topic (TUI phases together, then ZK phases) or by phase sequence would improve readability and maintainability.
</comment_end>
src/utilities/tui/TUIManager.ts (1)
1405-1420: Unused methodgetStatusColor()andCOLORSconstant.The
getStatusColor()method is defined but never called. Similarly, theCOLORSconstant (lines 59-80) is never referenced. These appear to be dead code, possibly from an earlier design iteration.Consider removing unused code to improve maintainability:
-// SECTION Color Schemes - -const COLORS = { - // Status colors - statusRunning: "green", - ... -}And remove the
getStatusColor()method, or document its intended future use with a TODO comment.src/utilities/tui/LegacyLoggerAdapter.ts (1)
161-194: Inconsistent sync/async file operations incustom()method.The method is declared
asyncbut mixes sync and async file operations:
- Line 176:
fs.rmSync(sync)- Line 177:
await fs.promises.writeFile(async)- Line 185:
fs.appendFileSync(sync)This inconsistency could cause confusion and doesn't leverage the async nature of the method.
Consider using consistent async operations:
- // Write to custom file - try { - fs.appendFileSync(customPath, logEntry) - } catch { - // Ignore errors - } + // Write to custom file + try { + await fs.promises.appendFile(customPath, logEntry) + } catch { + // Ignore errors + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.beads/.local_version(1 hunks).beads/issues.jsonl(1 hunks)src/utilities/tui/LegacyLoggerAdapter.ts(1 hunks)src/utilities/tui/TUIManager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .beads/.local_version
🔇 Additional comments (12)
.beads/issues.jsonl (1)
1-13: Data structure is well-formed and comprehensive.The issue entries are properly structured with all required fields, dependencies, timestamps, and rich metadata. The design sections contain detailed technical specifications, acceptance criteria are clearly defined with checklists, and status tracking includes proper timestamps. The JSONL format is correctly applied with valid JSON on each line.
</comment_end>
src/utilities/tui/TUIManager.ts (7)
937-953: LGTM!The
checkIfStandalone()method is well-implemented with proper optional chaining, defensive error handling, and a safe fallback to standalone mode when peers cannot be determined.
1168-1200: LGTM!The log entry rendering with timestamp extraction, level icons, and category display is well-structured. The timestamp parsing is safe given
LogEntry.timestampis typed asDate.
236-251: LGTM!The singleton pattern implementation is correct with proper instance management and cleanup in
resetInstance().
614-620: LGTM!Command history properly limits size to 100 entries, preventing memory accumulation. This addresses the previous concern about unbounded command history.
364-407: LGTM!Console interception is well-implemented with:
- Guard against double-interception (line 366)
- Category extraction from messages
- Appropriate routing to logger methods
The choice to map
console.logtodebuglevel (rather thaninfo) is reasonable for captured external library output.
267-340: LGTM!The TUI lifecycle management is well-implemented:
- Idempotent
start()with early return- Proper listener cleanup in
stop()using stored references- Correct ordering: remove listeners → restore console → restore terminal
- TUI mode toggling on the logger
1058-1078: No issues found. ThegetSharedStateusage is correct. It's a getter property (not a function call) that returns theSharedStatesingleton instance viaSharedState.getInstance(). The property access patterngetSharedState.lastBlockNumberandgetSharedState.syncStatusis appropriate and matches the actual export mechanism defined insrc/utilities/sharedState.ts(lines 269-273). The naming convention with "get" is accurate—it reflects that this is a getter, not a regular constant.src/utilities/tui/LegacyLoggerAdapter.ts (4)
24-31: LGTM!The
extractTagfunction has a well-designed regex with proper ReDoS mitigation using bounded quantifier{1,50}instead of unbounded+. Good defensive coding.
199-235: LGTM!The
only()anddisableOnlyMode()methods are now properly implemented:
- Original
console.logis stored before overwritingdisableOnlyMode()correctly restores the original- ONLY messages use
originalLogto bypass the no-op overrideThis addresses previous concerns about permanent console mutation and silent ONLY messages.
13-39: LGTM!The imports and helper functions are well-structured:
TAG_TO_CATEGORYandLogCategoryproperly imported from sharedtagCategoriesmoduleinferCategoryhas correct return type- Both helpers have clear, focused responsibilities
This addresses the previous concerns about duplicated mapping and missing type imports.
68-95:replace(".", "")only removes the first dot.Line 74 uses
identityFile?.replace(".", "")which only removes the first occurrence of.. If the identity file name contains multiple dots (e.g.,"node.identity.json"), only the first dot is removed, resulting in"nodeidentity.json".If the intent is to remove all dots or just the leading dot, consider:
-const identityFile = getSharedState.identityFile?.replace(".", "") ?? "" +// Remove only leading dot (if present) +const identityFile = getSharedState.identityFile?.replace(/^\./, "") ?? "" +// OR remove all dots +const identityFile = getSharedState.identityFile?.replaceAll(".", "") ?? ""Verify the expected identity file naming convention to determine the correct approach.
| {"id":"node-1q8","title":"Phase 1: Categorized Logger Utility","description":"Create a new categorized Logger utility that serves as a drop-in replacement for the current logger. Must support categories and be TUI-ready.","design":"## Logger Categories\n\n- **CORE** - Main bootstrap, warmup, general operations\n- **NETWORK** - RPC server, connections, HTTP endpoints\n- **PEER** - Peer management, peer gossip, peer bootstrap\n- **CHAIN** - Blockchain, blocks, mempool\n- **SYNC** - Synchronization operations\n- **CONSENSUS** - PoR BFT consensus operations\n- **IDENTITY** - GCR, identity management\n- **MCP** - MCP server operations\n- **MULTICHAIN** - Cross-chain/XM operations\n- **DAHR** - DAHR-specific operations\n\n## API Design\n\n```typescript\n// New logger interface\ninterface LogEntry {\n level: LogLevel;\n category: LogCategory;\n message: string;\n timestamp: Date;\n}\n\ntype LogLevel = 'debug' | 'info' | 'warning' | 'error' | 'critical';\ntype LogCategory = 'CORE' | 'NETWORK' | 'PEER' | 'CHAIN' | 'SYNC' | 'CONSENSUS' | 'IDENTITY' | 'MCP' | 'MULTICHAIN' | 'DAHR';\n\n// Usage:\nlogger.info('CORE', 'Starting the node');\nlogger.error('NETWORK', 'Connection failed');\nlogger.debug('CHAIN', 'Block validated #45679');\n```\n\n## Features\n\n1. Emit events for TUI to subscribe to\n2. Maintain backward compatibility with file logging\n3. Ring buffer for in-memory log storage (TUI display)\n4. Category-based filtering\n5. Log level filtering","acceptance_criteria":"- [ ] LogCategory type with all 10 categories defined\n- [ ] New Logger class with category-aware methods\n- [ ] Event emitter for TUI integration\n- [ ] Ring buffer for last N log entries (configurable, default 1000)\n- [ ] File logging preserved (backward compatible)\n- [ ] Unit tests for logger functionality","status":"closed","priority":1,"issue_type":"feature","assignee":"claude","created_at":"2025-12-04T15:45:22.238751684+01:00","updated_at":"2025-12-04T15:57:01.3507118+01:00","closed_at":"2025-12-04T15:57:01.3507118+01:00","labels":["logger","phase-1","tui"],"dependencies":[{"issue_id":"node-1q8","depends_on_id":"node-wrd","type":"parent-child","created_at":"2025-12-04T15:46:41.663898616+01:00","created_by":"daemon"}]} | ||
| {"id":"node-66u","title":"Phase 2: TUI Framework Setup","description":"Set up the TUI framework using terminal-kit (already installed). Create the basic layout structure with panels.","design":"## Layout Structure\n\n```\n┌─────────────────────────────────────────────────────────────────┐\n│ HEADER: Node info, status, version │\n├─────────────────────────────────────────────────────────────────┤\n│ TABS: Category selection │\n├─────────────────────────────────────────────────────────────────┤\n│ │\n│ LOG AREA: Scrollable log display │\n│ │\n├─────────────────────────────────────────────────────────────────┤\n│ FOOTER: Controls and status │\n└─────────────────────────────────────────────────────────────────┘\n```\n\n## Components\n\n1. **TUIManager** - Main orchestrator\n2. **HeaderPanel** - Node info display\n3. **TabBar** - Category tabs\n4. **LogPanel** - Scrollable log view\n5. **FooterPanel** - Controls and input\n\n## terminal-kit Features to Use\n\n- ScreenBuffer for double-buffering\n- Input handling (keyboard shortcuts)\n- Color support\n- Box drawing characters","acceptance_criteria":"- [ ] TUIManager class created\n- [ ] Basic layout with 4 panels renders correctly\n- [ ] Terminal resize handling\n- [ ] Keyboard input capture working\n- [ ] Clean exit handling (restore terminal state)","status":"closed","priority":1,"issue_type":"feature","assignee":"claude","created_at":"2025-12-04T15:45:22.405530697+01:00","updated_at":"2025-12-04T16:03:17.66943608+01:00","closed_at":"2025-12-04T16:03:17.66943608+01:00","labels":["phase-2","tui","ui"],"dependencies":[{"issue_id":"node-66u","depends_on_id":"node-1q8","type":"blocks","created_at":"2025-12-04T15:46:29.51715706+01:00","created_by":"daemon"},{"issue_id":"node-66u","depends_on_id":"node-wrd","type":"parent-child","created_at":"2025-12-04T15:46:41.730819864+01:00","created_by":"daemon"}]} |
There was a problem hiding this comment.
Consider updating the TUI Epic status to reflect progress.
The TUI Epic (node-wrd, line 13) is marked as "open", but 4 of its 6 child phases (Phases 1, 2, 3, 4) are already "closed", Phase 5 is "in_progress", and only Phase 6 remains "open". Updating the epic's status to "in_progress" would better reflect the actual project state and completion progress.
</comment_end>
Also applies to: 9-9, 11-13
🤖 Prompt for AI Agents
In .beads/issues.jsonl around lines 1-2, the TUI Epic (node-wrd, currently
"open") should be updated to reflect that most child phases are closed or in
progress; change its "status" field to "in_progress", update the "updated_at"
timestamp to now (ISO8601), and ensure its dependency/children metadata still
references the correct phase issues without changing their closed_at fields; do
not mark the epic closed.
1 similar comment
|
This PR modifies Files that will be reverted:
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.beads/issues.jsonl (1)
7-7: Epic status still needs updating (duplicate concern).As previously noted, the TUI Epic (node-wrd) remains marked as "open" despite 4 of 6 phases being closed and Phase 5 in progress. Update the status to "in_progress" and refresh the "updated_at" timestamp to accurately reflect project completion state.
src/utilities/tui/TUIManager.ts (2)
892-900: GuardupdateFilteredLogsagainst"CMD"category.
Tab.categoryincludes"CMD", but the logger’sgetEntriesByCategoryAPI is intended for real log categories. When the CMD tab is active (and when new log entries arrive),updateFilteredLogs()can be invoked withactiveTab.category === "CMD"and will callgetEntriesByCategory("CMD").Even though the CMD tab doesn't render log entries, making the intent explicit keeps both runtime behavior and typing cleaner:
private updateFilteredLogs(): void { const activeTab = TABS[this.activeTabIndex] - if (activeTab.category === "ALL") { - this.filteredLogs = this.logger.getAllEntries() - } else { - this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) - } + if (activeTab.category === "ALL") { + this.filteredLogs = this.logger.getAllEntries() + } else if (activeTab.category === "CMD") { + // CMD tab does not display categorized logs + this.filteredLogs = [] + } else { + this.filteredLogs = this.logger.getEntriesByCategory(activeTab.category) + } }This avoids ever passing
"CMD"into the logger.
125-135: Routequitcommand throughhandleQuit()instead of callingprocess.exit(0)directly.The
quitcommand currently emits"quit", stops the TUI, and then callsprocess.exit(0), which bypasses any asynchronous cleanup the main app might do in response to the"quit"event. You already have a graceful path inhandleQuit()that stops the TUI and emits"quit"without forcing an immediate exit.Reusing
handleQuit()here avoids duplicating shutdown logic and keeps the shutdown strategy consistent:{ name: "quit", description: "Exit the node", handler: (_args, tui) => { tui.addCmdOutput("Shutting down...") setTimeout(() => { - tui.emit("quit") - tui.stop() - process.exit(0) + // Reuse the same graceful shutdown path as keyboard-based quit + tui.handleQuit() }, 500) }, },And make
handleQuitcallable from the command handler:- private handleQuit(): void { + handleQuit(): void { this.stop() this.emit("quit") }This aligns the CMD
quitbehavior with the keyboardQ/CTRL_Cpath and with previous feedback about avoiding directprocess.exit.Also applies to: 698-701
🧹 Nitpick comments (1)
src/utilities/tui/TUIManager.ts (1)
229-231: Optionally detach the loggerloglistener on stop/reset to avoid accumulation.The ctor registers a bound handler on the shared
CategorizedLogger:this.logger.on("log", this.handleLogEntry.bind(this))but
stop()/resetInstance()never remove that listener. Across multipleresetInstance()calls (e.g., in tests), this can accumulate listeners and retain oldTUIManagerinstances.Consider storing the bound listener and removing it on stop/reset:
export class TUIManager extends EventEmitter { + private logListener: ((entry: LogEntry) => void) | null = null; @@ - this.logger = CategorizedLogger.getInstance() - - this.nodeInfo = { + this.logger = CategorizedLogger.getInstance() + + this.nodeInfo = { @@ - // Subscribe to log events - this.logger.on("log", this.handleLogEntry.bind(this)) + // Subscribe to log events + this.logListener = this.handleLogEntry.bind(this) + this.logger.on("log", this.logListener) @@ stop(): void { if (!this.isRunning) return @@ - // Disable TUI mode in logger + // Disable TUI mode in logger this.logger.disableTuiMode() @@ this.emit("stopped") } @@ static resetInstance(): void { if (TUIManager.instance) { - TUIManager.instance.stop() + TUIManager.instance.stop() + if (TUIManager.instance.logListener) { + TUIManager.instance.logger.off("log", TUIManager.instance.logListener) + TUIManager.instance.logListener = null + } TUIManager.instance = null } }This keeps the singleton clean across start/stop/reset cycles, especially in test environments.
Also applies to: 246-251, 305-340
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.beads/issues.jsonl(1 hunks)README.md(1 hunks)run(6 hunks)src/utilities/tui/TUIManager.ts(1 hunks)
🔇 Additional comments (2)
README.md (1)
48-79: TUI documentation matches implementation and CLI flags.The new TUI section, controls table, and
-t/--no-tuilegacy mode docs are consistent with therunscript andTUIManagerkey handling. No changes needed.run (1)
55-59: TUI on/off UX and startup command wiring look good.The
-t/--no-tuihelp text, examples, display summary, and theFINAL_COMMANDaugmentation with-- --no-tuicleanly expose legacy (non‑TUI) mode without affecting default behavior. This matches the README and keeps the startup flow straightforward.Also applies to: 66-68, 403-407, 615-619, 623-624
| # Handle long options (--no-tui) before getopts | ||
| for arg in "$@"; do | ||
| case $arg in | ||
| --no-tui) | ||
| NO_TUI=true | ||
| # Remove --no-tui from arguments so getopts doesn't choke on it | ||
| set -- "${@/--no-tui/}" | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file
fd -t f "^run$" | head -5Repository: kynesyslabs/node
Length of output: 63
🏁 Script executed:
# Read the run file around lines 309-318
head -320 run | tail -20Repository: kynesyslabs/node
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Test the current implementation behavior vs. proposed fix
# Test 1: Current implementation with ${@/--no-tui/}
echo "=== TEST 1: Current implementation ==="
test_current() {
set -- "-p" "53551" "--no-tui" "-x" "value"
echo "Before processing: \$@ = $*"
echo "Number of args before: $#"
for arg in "$@"; do
case $arg in
--no-tui)
# Current approach
set -- "${@/--no-tui/}"
;;
esac
done
echo "After processing: \$@ = $*"
echo "Number of args after: $#"
echo "Arg list:"
for i in "${!@}"; do
echo " [$((i+1))] = '${!i}'"
done
}
test_current
# Test 2: Proposed fix with FILTERED_ARGS
echo ""
echo "=== TEST 2: Proposed fix ==="
test_proposed() {
set -- "-p" "53551" "--no-tui" "-x" "value"
echo "Before processing: \$@ = $*"
echo "Number of args before: $#"
FILTERED_ARGS=()
for arg in "$@"; do
case "$arg" in
--no-tui)
NO_TUI=true
;;
*)
FILTERED_ARGS+=("$arg")
;;
esac
done
set -- "${FILTERED_ARGS[@]}"
echo "After processing: \$@ = $*"
echo "Number of args after: $#"
echo "Arg list:"
for i in "${!@}"; do
echo " [$((i+1))] = '${!i}'"
done
}
test_proposedRepository: kynesyslabs/node
Length of output: 576
🏁 Script executed:
# Read the run file to see the actual implementation and how getopts is used
sed -n '309,330p' runRepository: kynesyslabs/node
Length of output: 779
🏁 Script executed:
# Also check for getopts usage to understand the impact
rg -n "getopts" run -A 3 -B 1Repository: kynesyslabs/node
Length of output: 524
Fix --no-tui preprocessing: ${@/--no-tui/} leaves empty argument slots that break getopts.
Using ${@/--no-tui/} removes the text but leaves the positional argument empty. This creates a gap in $@ that getopts will see as a blank argument, breaking option parsing for commands like ./run -p 53551 --no-tui. The argument count remains unchanged but with a missing value, confusing the parser.
Build a filtered array instead:
-# Handle long options (--no-tui) before getopts
+# Handle long option (--no-tui) before getopts without breaking other args
+FILTERED_ARGS=()
for arg in "$@"; do
- case $arg in
+ case "$arg" in
--no-tui)
NO_TUI=true
- # Remove --no-tui from arguments so getopts doesn't choke on it
- set -- "${@/--no-tui/}"
;;
+ *)
+ FILTERED_ARGS+=("$arg")
+ ;;
esac
done
+set -- "${FILTERED_ARGS[@]}"This removes --no-tui entirely while preserving argument boundaries for getopts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Handle long options (--no-tui) before getopts | |
| for arg in "$@"; do | |
| case $arg in | |
| --no-tui) | |
| NO_TUI=true | |
| # Remove --no-tui from arguments so getopts doesn't choke on it | |
| set -- "${@/--no-tui/}" | |
| ;; | |
| esac | |
| done | |
| # Handle long option (--no-tui) before getopts without breaking other args | |
| FILTERED_ARGS=() | |
| for arg in "$@"; do | |
| case "$arg" in | |
| --no-tui) | |
| NO_TUI=true | |
| ;; | |
| *) | |
| FILTERED_ARGS+=("$arg") | |
| ;; | |
| esac | |
| done | |
| set -- "${FILTERED_ARGS[@]}" |
🤖 Prompt for AI Agents
In run around lines 309 to 318, the preprocessing for --no-tui uses
"${@/--no-tui/}" which leaves empty positional slots and breaks getopts; instead
iterate over "$@" and build a new array of args that excludes any exact
"--no-tui" entries, set NO_TUI=true when seen, then replace the positional
parameters with the filtered array (set -- "${filtered[@]}") so argument
boundaries are preserved and getopts works correctly.
| switch (key) { | ||
| // Quit | ||
| case "q": | ||
| case "Q": | ||
| case "CTRL_C": | ||
| this.handleQuit() | ||
| break | ||
|
|
||
| // Tab switching with number keys | ||
| case "0": | ||
| case "1": | ||
| case "2": | ||
| case "3": | ||
| case "4": | ||
| case "5": | ||
| case "6": | ||
| case "7": | ||
| case "8": | ||
| case "9": | ||
| this.setActiveTab(Number.parseInt(key, 10)) | ||
| break | ||
|
|
||
| case "-": | ||
| this.setActiveTab(10) // DAHR tab | ||
| break | ||
|
|
||
| case "=": | ||
| this.setActiveTab(11) // CMD tab | ||
| break | ||
|
|
||
| // Tab navigation | ||
| case "TAB": | ||
| case "RIGHT": | ||
| this.nextTab() | ||
| break | ||
|
|
||
| case "SHIFT_TAB": | ||
| case "LEFT": | ||
| this.previousTab() | ||
| break | ||
|
|
||
| // Scrolling | ||
| case "UP": | ||
| case "k": | ||
| this.scrollUp() | ||
| break | ||
|
|
||
| case "DOWN": | ||
| case "j": | ||
| this.scrollDown() | ||
| break | ||
|
|
||
| case "PAGE_UP": | ||
| this.scrollPageUp() | ||
| break | ||
|
|
||
| case "PAGE_DOWN": | ||
| this.scrollPageDown() | ||
| break | ||
|
|
||
| case "HOME": | ||
| this.scrollToTop() | ||
| break | ||
|
|
||
| case "END": | ||
| this.scrollToBottom() | ||
| break | ||
|
|
||
| // Toggle auto-scroll | ||
| case "a": | ||
| case "A": | ||
| this.toggleAutoScroll() | ||
| break | ||
|
|
||
| // Clear logs | ||
| case "c": | ||
| case "C": | ||
| this.clearLogs() | ||
| break | ||
|
|
||
| // Help | ||
| case "h": | ||
| case "H": | ||
| case "?": | ||
| this.showHelp() | ||
| break | ||
| } |
There was a problem hiding this comment.
Align help text with actual key bindings (S/P/R are not implemented).
showHelp() advertises S=start, P=pause, R=restart:
this.logger.info("CORE", "Controls: S=start, P=pause, R=restart, Q=quit")but handleKeyPress() does not handle S, P, or R at all—only Q, navigation keys, scroll keys, A, C, H, and ?. This can confuse users when they try keys that do nothing.
Either wire up those keys, or (simpler) adjust the help text to only list the implemented controls, e.g.:
- this.logger.info("CORE", "Controls: S=start, P=pause, R=restart, Q=quit")
- this.logger.info("CORE", "Other: A=auto-scroll, C=clear, H=help")
+ this.logger.info("CORE", "Controls: Q=quit")
+ this.logger.info("CORE", "Other: A=auto-scroll, C=clear, H=help")This keeps in-TUI help accurate for current behavior.
Also applies to: 1365-1372
1 participant
User description
Added a tui as default visualization
PR Type
Enhancement
Description
Implemented comprehensive TUI (Terminal User Interface) system as default visualization for the node
Created
TUIManagerorchestrator managing layout, keyboard input, and panel coordination with 12 tabbed categories (All, Core, Net, Peer, Chain, Sync, Cons, ID, MCP, XM, DAHR, CMD)Developed
CategorizedLoggerwith 10 log categories using ring buffers for fixed-size circular storage and event emission for real-time TUI displayImplemented
LegacyLoggerAdaptermaintaining backward compatibility with existing Logger API while routing through new categorized logging systemAdded command terminal (CMD tab) with history and built-in commands (help, quit, clear, status, peers, sync)
Integrated node info header displaying version, status, identity, port, peers, block number, and sync status
Implemented scrollable log display with auto-scroll toggle, per-tab scroll offsets, and real-time filtering
Added console interception to route all output through TUI logger, preventing external library corruption
Integrated TUI initialization in main node startup with
--no-tuiflag for fallback to standard outputRefactored logger system to use new TUI-integrated architecture while preserving all existing import paths
Added ASCII logo assets for TUI header display in multiple sizes
Diagram Walkthrough
File Walkthrough
6 files
TUIManager.ts
Main TUI orchestrator with panels and controlssrc/utilities/tui/TUIManager.ts
input, and panel coordination
Chain, Sync, Cons, ID, MCP, XM, DAHR, CMD)
scroll offsets, and real-time filtering
(help, quit, clear, status, peers, sync)
peers, block number, sync status
logger, preventing external library corruption
CategorizedLogger.ts
TUI-ready categorized logging system with ring bufferssrc/utilities/tui/CategorizedLogger.ts
NETWORK, PEER, CHAIN, SYNC, CONSENSUS, IDENTITY, MCP, MULTICHAIN,
DAHR)
category to prevent memory bloat
backward compatibility
active
LegacyLoggerAdapter.ts
Backward compatibility layer for legacy Logger APIsrc/utilities/tui/LegacyLoggerAdapter.ts
while routing through new CategorizedLogger
from existing log messages like
[PEER],[MAIN]custom, only, cleanLogs, getPublicLogs, getDiagnostics)
code
index.ts
TUI module exports and type definitionssrc/utilities/tui/index.ts
LegacyLoggerAdapter, and TUIManager
NodeInfo, TUIConfig) for type safety
new logging API
index.ts
Integrated TUI into main node startup and loggingsrc/index.ts
failure
console.logandterm.*calls with categorized logger callsthroughout warmup and preMainLoop
--no-tuicommand-line flag to disable TUI and use scrolling logoutput
after preMainLoop, when running)
sync status
mainLoop.ts
Updated mainLoop diagnostics loggingsrc/utilities/mainLoop.ts
console.logcall inlogCurrentDiagnostics()with categorizedlogger debug call
1 files
logger.ts
Logger refactored to use new TUI-integrated systemsrc/utilities/logger.ts
LegacyLoggerAdapter
Logger exports
migration
1 files
issues.jsonl
TUI implementation issue tracking and project management.beads/issues.jsonl
6 phases
Log Display, Info Panel)
migration complete
5 files
demos_logo_ascii_bn
Full-size DEMOS ASCII logo assetres/demos_logo_ascii_bn
display
demos_logo_ascii_bn_small
Small DEMOS ASCII logo assetres/demos_logo_ascii_bn_small
demos_banner_ascii
DEMOS banner ASCII art assetres/demos_banner_ascii
demos_logo_ascii
Add large ASCII art logo resource fileres/demos_logo_ascii
@symbols and special characters to create the logo artworkdemos_logo_ascii_bn_xsmall
Add small ASCII art logo resource fileres/demos_logo_ascii_bn_xsmall
█(full block) characters to create a minimalist logo designSummary by CodeRabbit
New Features
Improvements
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.