feat: add --addr flag to daft-dashboard cli #5444

VOID001 · 2025-10-24T10:01:02Z

Changes Made

Add --addr flag to daft dashboard, so that it can support arbitrary address
Add warning when running daft-dashboard with addr 0.0.0.0 or :: to
avoid accidentally exposing the service to the external network

NOTE: Will first make daft-cli work, the python.rs will get changed in another patch

Related Issues

Checklist

Documented in API Docs (if applicable)
Documented in User Guide (if applicable)
If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
Documentation builds and is formatted properly

greptile-apps

Greptile Overview

Greptile Summary

Adds --addr flag to daft-dashboard CLI to support binding to arbitrary addresses, with CLI default changed to 127.0.0.1 for security.

Critical Issues Found:

Invalid IPv6 URL generation: DEFAULT_SERVER_ADDR changed from Ipv4Addr::UNSPECIFIED to "::" (IPv6), but generate_interactive_html creates http://:::port which is invalid syntax - IPv6 addresses must be wrapped in brackets [::]:port
Panic on invalid input: Address parsing uses panic! instead of proper error handling via Result
Inconsistent security model: CLI defaults to 127.0.0.1 (localhost only) but python.rs hardcodes "::" (all interfaces), contradicting the PR's stated security goal

Recommendations:

Fix IPv6 URL formatting in generate_interactive_html by detecting IPv6 and wrapping in brackets
Change launch_server to return Result instead of panicking on parse errors
Align python.rs default with CLI's 127.0.0.1 or update both to use a consistent DEFAULT_SERVER_ADDR

Confidence Score: 1/5

This PR has critical bugs that will cause runtime failures and security issues
Score of 1 reflects multiple critical issues: (1) generates syntactically invalid URLs when using IPv6 addresses that will break the dashboard's JavaScript functionality, (2) server can panic/crash on invalid address input, (3) stated security goal contradicted by hardcoded "::" binding to all interfaces in python.rs. These issues require fixes before merge.
src/daft-dashboard/src/lib.rs requires immediate attention for URL formatting and error handling. src/daft-dashboard/src/python.rs needs alignment with security goals.

Important Files Changed

File Analysis

Filename	Score	Overview
src/daft-dashboard/src/lib.rs	1/5	Changed `DEFAULT_SERVER_ADDR` to `"::"` (IPv6) and added `addr` parameter to `launch_server()` - critical issues: panic on invalid input, generates invalid IPv6 URLs, conflicts with stated security goal
src/daft-cli/src/python.rs	3/5	Added `--addr` flag with default `127.0.0.1` for CLI - implementation looks correct but inconsistent with python.rs hardcoded `"::"`
src/daft-dashboard/src/python.rs	2/5	Updated to use new `launch_server()` signature with hardcoded `"::"` address - inconsistent with CLI default of `127.0.0.1`, note added about making addr/port configurable

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as daft-cli
    participant Dashboard as daft-dashboard::lib
    participant Browser
    
    User->>CLI: daft-dashboard --addr 127.0.0.1 --port 80
    CLI->>CLI: Parse args (addr, port, verbose)
    CLI->>Dashboard: launch_server(addr, port, shutdown_fn)
    Dashboard->>Dashboard: Parse addr string to IpAddr
    Dashboard->>Dashboard: Bind TcpListener to (addr, port)
    Dashboard-->>CLI: Server running
    CLI-->>User: Display "View dashboard at http://addr:port"
    
    User->>Browser: Navigate to http://addr:port
    Browser->>Dashboard: HTTP GET /
    Dashboard->>Dashboard: generate_interactive_html(df_id, host, port)
    Note over Dashboard: Bug: IPv6 addr "::" becomes<br/>http://:::port (invalid URL)
    Dashboard-->>Browser: HTML with JavaScript
    Browser->>Dashboard: Fetch /api/dataframes/{id}/cell
    Dashboard-->>Browser: Cell data
    
    User->>CLI: Ctrl+C
    CLI->>Dashboard: Send shutdown signal
    Dashboard->>Dashboard: Graceful shutdown
    Dashboard-->>CLI: Shutdown complete

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

src/daft-dashboard/src/lib.rs

src/daft-cli/src/python.rs

codecov · 2025-10-24T10:44:26Z

Codecov Report

❌ Patch coverage is 30.76923% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.44%. Comparing base (219d4c6) to head (33e4c2d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/daft-cli/src/python.rs	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5444      +/-   ##
==========================================
- Coverage   71.45%   71.44%   -0.02%     
==========================================
  Files        1001     1001              
  Lines      126760   126783      +23     
==========================================
+ Hits        90579    90581       +2     
- Misses      36181    36202      +21

Files with missing lines	Coverage Δ
src/daft-dashboard/src/lib.rs	`89.15% <100.00%> (+0.04%)`	⬆️
src/daft-dashboard/src/python.rs	`69.11% <100.00%> (+1.93%)`	⬆️
src/daft-cli/src/python.rs	`6.34% <0.00%> (-1.06%)`	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

VOID001 · 2025-10-24T15:50:01Z

The PR test suite looks like failed without any error message

srilman

Looks good so far, just a couple of things. Thanks for working on this @VOID001!

src/daft-cli/src/python.rs

src/daft-dashboard/src/lib.rs

srilman · 2025-10-27T17:10:59Z

src/daft-dashboard/src/python.rs

 }

 #[pyfunction]
+// TODO(void001): Make addr & port configurable


Actually I don't think we want this to be configurable for the jupyter notebook side, it should be auto-detected

Hmm I saw a comment at L86, shall we remove the comment as well?
BTW what does it mean by "auto-detect"

let port = super::DEFAULT_SERVER_PORT; // TODO: Make configurable

srilman · 2025-10-31T02:21:33Z

@VOID001 can you re-request a review when you're ready? Thanks!

VOID001 · 2025-11-02T14:54:05Z

@greptile

greptile-apps

Greptile Overview

Greptile Summary

Adds --addr flag to daft-dashboard CLI enabling custom bind addresses, but introduces inconsistencies between stated security goals and implementation

Key Changes:

CLI now accepts --addr flag (defaults to 0.0.0.0 with port 80)
Modified launch_server() to accept IpAddr parameter
Added warning when binding to unspecified addresses (0.0.0.0 or ::)
Python API still hardcodes DEFAULT_SERVER_ADDR (0.0.0.0)

Issues Found:

PR description states default should be 127.0.0.1 for security, but implementation uses 0.0.0.0 in CLI and maintains 0.0.0.0 constant
Previous review comments identified critical issues with panic on invalid address and IPv6 URL formatting that remain unresolved

Confidence Score: 2/5

This PR has unresolved critical issues and contradicts its stated security goals
Score reflects multiple critical issues: panic on invalid address input (needs Result return), IPv6 URL formatting bug, and fundamental contradiction between PR description (claiming 127.0.0.1 default for security) and actual implementation (0.0.0.0 default). Previous review comments remain unaddressed.
Pay close attention to src/daft-dashboard/src/lib.rs (panic handling and IPv6 URL generation) and src/daft-cli/src/python.rs (default address security)

Important Files Changed

File Analysis

Filename	Score	Overview
src/daft-cli/src/python.rs	3/5	Added `--addr` flag with `0.0.0.0` default and warning for unspecified addresses, but contradicts PR description stating default should be `127.0.0.1` for security
src/daft-dashboard/src/lib.rs	2/5	Modified `launch_server` to accept `IpAddr` parameter with panic on invalid address, but needs Result return type for proper error handling
src/daft-dashboard/src/python.rs	3/5	Updated Python launcher to use `DEFAULT_SERVER_ADDR` (still `0.0.0.0`), maintaining consistency but not addressing security concerns from PR description

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as daft-cli
    participant Dashboard as daft-dashboard
    participant Server as Axum Server
    
    User->>CLI: Run daft dashboard --addr 0.0.0.0 --port 80
    CLI->>CLI: Parse address argument
    CLI->>CLI: Check if addr.is_unspecified()
    alt Address is unspecified (0.0.0.0 or ::)
        CLI->>User: Display warning about security risk
    end
    CLI->>Dashboard: launch_server(addr, port, shutdown_fn)
    Dashboard->>Server: TcpListener::bind((addr, port))
    Server->>Server: Start HTTP server on specified address
    Server->>CLI: Server ready
    CLI->>User: Display connection instructions with http://addr:port
    
    Note over User,Server: Python API Launch Path (from Python code)
    User->>Dashboard: dashboard.launch()
    Dashboard->>Dashboard: Use hardcoded DEFAULT_SERVER_ADDR (0.0.0.0)
    Dashboard->>Server: TcpListener::bind((DEFAULT_SERVER_ADDR, 3238))
    Server->>User: Return ConnectionHandle

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

VOID001 · 2025-11-02T15:02:28Z

@greptile

greptile-apps

Greptile Overview

Greptile Summary

This PR adds an --addr flag to the daft-dashboard CLI to allow binding to arbitrary addresses, moving away from the hardcoded default. It includes a security warning when binding to all interfaces (0.0.0.0 or ::).

Major changes:

Added --addr CLI parameter with default 0.0.0.0 in daft-cli
Updated launch_server signature to accept addr: IpAddr parameter
Added warning message for unspecified addresses that bind to all network interfaces
Updated URL formatting to use SocketAddr instead of separate host/port strings

Critical issues found:

generate_interactive_html function still hardcodes DEFAULT_SERVER_ADDR instead of using the actual server address, causing generated HTML to contain incorrect URLs
The Python binding's generate_interactive_html passes DEFAULT_SERVER_ADDR.to_string() as the host, perpetuating the problem
Default of 0.0.0.0 conflicts with PR description's security goal of using 127.0.0.1
IPv6 addresses in URLs need bracket notation (e.g., http://[::]:3238) which isn't handled

Confidence Score: 2/5

This PR has multiple critical functional bugs that will break interactive HTML generation
Score reflects critical logic errors where generate_interactive_html hardcodes the server address instead of using the actual bind address, resulting in broken URLs in generated HTML. Additionally, IPv6 addresses aren't properly formatted in URLs, and the default address choice contradicts the stated security goals.
All three files need attention: src/daft-dashboard/src/lib.rs and src/daft-dashboard/src/python.rs have the critical URL generation bug, while src/daft-cli/src/python.rs has the default address inconsistency

Important Files Changed

File Analysis

Filename	Score	Overview
src/daft-cli/src/python.rs	3/5	Adds `--addr` flag with default `0.0.0.0` and warning for unspecified addresses; passes address to `launch_server` but default conflicts with security goal
src/daft-dashboard/src/lib.rs	2/5	Updates `launch_server` to accept address parameter but `generate_interactive_html` still hardcodes `DEFAULT_SERVER_ADDR`, breaking URL generation
src/daft-dashboard/src/python.rs	2/5	Updates `launch` to pass address to `launch_server` but `generate_interactive_html` still uses hardcoded `DEFAULT_SERVER_ADDR`, generating incorrect URLs

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as daft-cli
    participant Dashboard as daft-dashboard
    participant Server as Axum Server
    
    User->>CLI: Run with --addr flag
    CLI->>CLI: Parse addr (IpAddr)
    CLI->>CLI: Check if unspecified
    alt Address is 0.0.0.0 or ::
        CLI->>User: Display warning
    end
    CLI->>CLI: Create SocketAddr
    CLI->>Dashboard: launch_server(addr, port, shutdown_fn)
    Dashboard->>Server: Bind TcpListener to (addr, port)
    Server->>User: Server running
    
    User->>Server: Request interactive HTML
    Server->>Dashboard: generate_interactive_html()
    Note over Dashboard: ⚠️ Uses DEFAULT_SERVER_ADDR<br/>instead of actual addr
    Dashboard->>Server: Return HTML with hardcoded addr
    Server->>User: Return HTML
    Note over User: URLs in HTML may be incorrect

Additional Comments (2)

src/daft-dashboard/src/lib.rs, line 38-43 (link)

logic: generate_interactive_html doesn't receive the actual server address, so generated HTML will always use DEFAULT_SERVER_ADDR (0.0.0.0) in URLs even when server binds to a different address like 127.0.0.1
```
fn generate_interactive_html(
    record_batch: &RecordBatch,
    df_id: &str,
    addr: &IpAddr,
    port: u16,
) -> String {
```
src/daft-dashboard/src/python.rs, line 57-62 (link)

logic: passing DEFAULT_SERVER_ADDR.to_string() as host parameter means generated HTML always uses 0.0.0.0 in URLs, breaking functionality when server uses different address - needs to track and use actual server address

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

VOID001 · 2025-11-02T15:11:05Z

Hi @srilman , could you please take another look for this patch? Thanks!

srilman

Looks great VOID001, just 2 nits and then I think we can merge it!

srilman · 2025-11-03T18:49:11Z

src/daft-cli/src/python.rs

-            .bold()
-            .magenta()
-            .underlined(),
+            console::style(format!("http://{}", socket_addr))


Can we include the port here?

it's already included，this is a better way to join ip and port together, even if it is a ipv6 address，it can display correctly

it's better than just format ip:port which will cause trouble to v6 address

Now it display like this

srilman · 2025-11-03T18:49:40Z

src/daft-dashboard/src/lib.rs

    port: u16,
    shutdown_fn: impl Future<Output = ()> + Send + 'static,
 ) -> std::io::Result<()> {
+    // parse the addr to either v4 or v6 address


Remove comment, not longer relevant?

Ah, my bad. removed. PTAL

- Add `--addr` flag to daft dashboard, so that it can support arbitrary address - Add warning when running `daft-dashboard` with addr 0.0.0.0 or :: to avoid accidentally exposing the service to the external network **NOTE**: Will first make daft-cli work, the python.rs will get changed in another patch  - [ ] Documented in API Docs (if applicable) - [ ] Documented in User Guide (if applicable) - [ ] If adding a new documentation page, doc is added to `docs/mkdocs.yml` navigation - [ ] Documentation builds and is formatted properly Signed-off-by: JQ <[email protected]>

github-actions bot added the feat label Oct 24, 2025

greptile-apps bot reviewed Oct 24, 2025

View reviewed changes

src/daft-dashboard/src/lib.rs Outdated Show resolved Hide resolved

src/daft-dashboard/src/lib.rs Outdated Show resolved Hide resolved

src/daft-cli/src/python.rs Outdated Show resolved Hide resolved

srilman reviewed Oct 27, 2025

View reviewed changes

VOID001 force-pushed the feat-add-addr-to-daft-dashboard branch from 8b759a2 to 31605a1 Compare November 2, 2025 14:53

greptile-apps bot reviewed Nov 2, 2025

View reviewed changes

VOID001 force-pushed the feat-add-addr-to-daft-dashboard branch from 31605a1 to ba60eb5 Compare November 2, 2025 15:01

greptile-apps bot reviewed Nov 2, 2025

View reviewed changes

srilman reviewed Nov 3, 2025

View reviewed changes

VOID001 force-pushed the feat-add-addr-to-daft-dashboard branch from ba60eb5 to 33e4c2d Compare November 4, 2025 01:47

feat: add --addr flag to daft-dashboard cli #5444

Are you sure you want to change the base?

feat: add --addr flag to daft-dashboard cli #5444

Conversation

VOID001 commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Made

Related Issues

Checklist

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Greptile Summary

Confidence Score: 1/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VOID001 commented Oct 24, 2025

Uh oh!

srilman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srilman Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

VOID001 Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

srilman commented Oct 31, 2025

Uh oh!

VOID001 commented Nov 2, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Uh oh!

VOID001 commented Nov 2, 2025

Uh oh!

greptile-apps bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Greptile Overview

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Additional Comments (2)

Uh oh!

VOID001 commented Nov 2, 2025

Uh oh!

srilman left a comment

Choose a reason for hiding this comment

Uh oh!

srilman Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

VOID001 Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VOID001 Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VOID001 Nov 4, 2025

VOID001 commented Oct 24, 2025 •

edited

Loading

codecov bot commented Oct 24, 2025 •

edited

Loading

greptile-apps bot left a comment •

edited

Loading

VOID001 Nov 4, 2025 •

edited

Loading

VOID001 Nov 4, 2025 •

edited

Loading