Skip to content

Conversation

@sigmachirality
Copy link
Member

@sigmachirality sigmachirality commented Dec 9, 2025

Screenshot 2025-12-08 at 7 36 13 PM

@semanticdiff-com
Copy link

semanticdiff-com bot commented Dec 9, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  src/lib/nodes/utils.ts  77% smaller
  src/lib/nodes/list.tsx  25% smaller
  deno.lock Unsupported file format

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 9, 2025

Greptile Overview

Greptile Summary

Reworks the sf nodes ls --verbose output to display VM metadata more clearly by separating the active VM from previous VMs, and improves duration formatting to show human-readable text instead of just hours.

Key changes:

  • Added date-fns functions for better duration formatting (e.g., "1 day, 5 hours" instead of "73 hours")
  • Separated VM display into "Active VM" section and "Previous VMs" table
  • Removed emojis from section headers, using consistent cyan bold text
  • Changed "Virtual Machines" header to "Previous VMs" for clarity

Issues found:

  • Redundant display of VM image ID in "Current VM Image" section (already shown in "Active VM" section)
  • Logic bug where slice(1) is used to get previous VMs, but getLastVM() sorts to find the most recent VM - if the array isn't pre-sorted, wrong VMs will be displayed
  • PR title mentions adding sf node vms command, but this command doesn't appear in the changes

Confidence Score: 2/5

  • This PR has logic bugs that will cause incorrect VM information to be displayed
  • Score reflects critical logic issues: redundant image display and potential VM filtering bug where slice(1) assumes array position 0 is the active VM, but getLastVM() sorts to find it. These bugs will cause user confusion and incorrect data display.
  • Pay close attention to src/lib/nodes/list.tsx - specifically the VM filtering logic at lines 364-370 and redundant display at lines 444-456

Important Files Changed

File Analysis

Filename Score Overview
src/lib/nodes/list.tsx 2/5 reworked VM display in verbose mode with improved formatting and duration display, but has logic issues with VM filtering and redundant image display

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as sf nodes ls --verbose
    participant NodesClient
    participant API
    participant NodeVerboseDisplay
    participant VMTable

    User->>CLI: Execute command
    CLI->>NodesClient: Fetch nodes
    NodesClient->>API: GET /nodes
    API-->>NodesClient: Return nodes array
    NodesClient-->>CLI: nodes data
    
    loop For each node
        CLI->>NodeVerboseDisplay: Render node
        NodeVerboseDisplay->>NodeVerboseDisplay: Get lastVM via getLastVM()
        NodeVerboseDisplay->>NodeVerboseDisplay: Calculate duration with date-fns
        
        alt lastVM exists
            NodeVerboseDisplay->>NodeVerboseDisplay: Display Active VM section
            NodeVerboseDisplay->>NodeVerboseDisplay: Show VM ID, status, dates, image
        end
        
        alt node.vms.data.length > 1
            NodeVerboseDisplay->>VMTable: Render Previous VMs (slice(1))
            VMTable->>VMTable: Sort by updated_at, show first 5
        end
        
        NodeVerboseDisplay->>NodeVerboseDisplay: Display Schedule section
        NodeVerboseDisplay->>NodeVerboseDisplay: Display Pricing section
        
        alt node.vms.data[0].image_id exists
            NodeVerboseDisplay->>NodeVerboseDisplay: Display Current VM Image section
            Note over NodeVerboseDisplay: ⚠️ Redundant - already shown in Active VM
        end
        
        NodeVerboseDisplay-->>CLI: Rendered node output
    end
    
    CLI-->>User: Display formatted output
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. src/lib/nodes/list.tsx, line 444-456 (link)

    logic: redundant display of image ID - already shown in "Active VM" section at line 359

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@sigmachirality sigmachirality changed the title feat: expand VM metadata shown in sf nodes ls --verbose, add sf node vms feat: [ENG-2396] expand VM metadata shown in sf nodes ls --verbose Dec 9, 2025
@sigmachirality sigmachirality self-assigned this Dec 9, 2025
@sigmachirality sigmachirality merged commit 73f483d into main Dec 9, 2025
1 check passed
@sigmachirality sigmachirality deleted the dt/nodes-verbose branch December 9, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants