feat: implement tui - #10
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an interactive TUI launched via Changes
Sequence DiagramsequenceDiagram
participant User
participant TUI as TUI (Bubble Tea)
participant Store as Store (encrypted)
participant DB as Database
participant Renderer as Renderer
User->>TUI: run `dbtree open`
TUI->>User: prompt for master password
User->>TUI: submit password
TUI->>Store: NewStore(password) / Load()
Store->>Store: Argon2id derive key, AES‑GCM decrypt file
Store-->>TUI: return connections[] or error
TUI->>User: show connections menu
User->>TUI: select connection / add new
TUI->>DB: open connection URL (with DSN adjustments)
TUI->>DB: ping / introspect schema
DB-->>TUI: schema metadata
TUI->>Renderer: build graph → render (text/json/chart)
Renderer-->>TUI: rendered output
TUI->>User: display rendered schema (viewport)
User->>TUI: toggle format/shape / refresh / quit
TUI->>DB: (on refresh) re-introspect
TUI->>User: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
store/store.go (2)
208-210: Consider stronger Argon2 parameters for production use.The current parameters (
time=1, memory=64KB) are on the lower end. OWASP recommends at minimumtime=2, memory=19MBfor Argon2id. While this is acceptable for a local CLI tool where UX matters, consider documenting this trade-off or providing a configuration option for higher-security environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store/store.go` around lines 208 - 210, The deriveKey function uses Argon2 with very low parameters (argon2.IDKey in deriveKey); update it to use stronger defaults (e.g., time=2 and memory=19456) and keep parallelism and key length as appropriate, and make these parameters configurable (via constants, config struct or environment variables) so production deployments can raise CPU/memory cost without changing code; also add a short comment documenting the trade-off between UX and security and the recommended OWASP baseline.
86-93: Password verification triggers double file read.
NewStorereads the file (lines 75-78), then callss.Load()(line 89) which reads it again. This is functionally correct but slightly inefficient. For a local file this is acceptable, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store/store.go` around lines 86 - 93, NewStore currently reads the file once then calls Store.Load(), causing a second read; avoid the double read by reusing the already-read bytes: modify NewStore and Store so the bytes read earlier are passed into decryption instead of calling Load() again — either add a new method LoadFromBytes(data []byte) (or decryptFromBytes) and call that from NewStore using the already-read contents, or change Load to accept an optional []byte parameter and use the provided data when non-nil; update references to Load/NewStore accordingly (look for NewStore, Store, and Load in the diff) so password verification uses the cached bytes rather than re-reading the file.tui/password.go (1)
45-62: Consider the blocking nature of key derivation.
loadConnectionsrunsstore.NewStore(password)which includes Argon2 key derivation. While this is a blocking operation that may cause a brief UI pause, this is acceptable for a CLI tool. For a smoother UX, you could show a "Unlocking..." indicator before triggering the command, though this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/password.go` around lines 45 - 62, store.NewStore(password) performs Argon2 key derivation which blocks the UI; to avoid a pause, set an "unlocking" state before invoking loadConnections or have loadConnections emit an immediate unlocking message and then perform the blocking work in the background. Concretely: in the Update handler set model.unlocking = true (or dispatch an unlockingMsg) and re-render a spinner/"Unlocking..." indicator, then return the tea.Cmd(loadConnections(password)); alternatively modify loadConnections to first return an unlockingMsg synchronously and then perform store.NewStore(password) and send connectionsLoadedMsg; reference loadConnections, store.NewStore, and connectionsLoadedMsg (and add an unlockingMsg/flag) so the UI shows progress while Argon2 runs.tui/schema.go (1)
88-128: Consider adding a timeout to database operations.The
loadSchemafunction usescontext.Background()with no timeout, which could cause the TUI to hang indefinitely if the database is unresponsive. A timeout would improve user experience and prevent resource leaks.♻️ Proposed fix to add timeout
func loadSchema(conn *store.Connection, format render.Format, shape render.Shape) tea.Cmd { return func() tea.Msg { connURL := conn.URL // Strip protocol prefixes for drivers that need it switch conn.Driver { case "mysql": connURL = strings.TrimPrefix(connURL, "mysql://") case "sqlite3": connURL = strings.TrimPrefix(connURL, "sqlite://") } db, err := sql.Open(conn.Driver, connURL) if err != nil { return schemaLoadedMsg{err: fmt.Errorf("failed to open database: %w", err)} } defer db.Close() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := db.PingContext(ctx); err != nil { return schemaLoadedMsg{err: fmt.Errorf("failed to connect: %w", err)} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/schema.go` around lines 88 - 128, The loadSchema function uses context.Background() with no timeout which can hang; wrap the background context in a cancellable timeout (e.g., ctx, cancel := context.WithTimeout(context.Background(), <reasonable-duration>); defer cancel()) and pass that ctx into db.PingContext and database.InspectSchema (and any other DB/long-running calls like graph.Build or render.Render if they accept contexts) so the operation is bounded and resources are released on timeout; adjust the timeout value or make it configurable as needed and ensure defer cancel() is present to avoid leaks.tui/newconn.go (1)
18-27: Shift+Tab behaves the same as Tab.The expression
(m.newConnFocus + 1) % 2always increments the focus index regardless of whether Tab or Shift+Tab was pressed. With only two fields this works but is inconsistent with standard keyboard navigation expectations.♻️ Proposed fix for correct bidirectional navigation
case tea.KeyTab, tea.KeyShiftTab: - m.newConnFocus = (m.newConnFocus + 1) % 2 + if msg.Type == tea.KeyShiftTab { + m.newConnFocus = (m.newConnFocus + 1) % 2 // With 2 fields, both directions wrap the same + } else { + m.newConnFocus = (m.newConnFocus + 1) % 2 + }Since there are only 2 fields, both directions produce the same result. This is fine as-is, but if more fields are added later, consider:
case tea.KeyTab: m.newConnFocus = (m.newConnFocus + 1) % len(m.newConnInputs) case tea.KeyShiftTab: m.newConnFocus = (m.newConnFocus - 1 + len(m.newConnInputs)) % len(m.newConnInputs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/newconn.go` around lines 18 - 27, The Tab and Shift+Tab handling currently always increments m.newConnFocus via (m.newConnFocus + 1) % 2, so Shift+Tab behaves the same as Tab; split the combined case into separate handlers for tea.KeyTab and tea.KeyShiftTab and update m.newConnFocus accordingly using the length of m.newConnInputs (increment with +1 and decrement with -1 then add len(m.newConnInputs) before modulo) so navigation wraps correctly for both directions; after updating m.newConnFocus keep the same loop that calls Focus() or Blur() on m.newConnInputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 35-39: Update the modules list and subcommands mention in the
CLAUDE.md documentation: add entries describing the new tui/ package ("tui/ —
Interactive terminal UI for managing connections and browsing schemas") and the
store/ package ("store/ — Encrypted persistence for saved database
connections"), and include the new open subcommand in the subcommands list
alongside version and update (i.e., mention "Subcommands: version, update, open"
or similar). Ensure the wording matches the existing style used for other
entries (e.g., reference cmd/dbtree/main.go parsing flags and subcommands) and
place the new lines where the modules and subcommands are described.
In `@store/store.go`:
- Around line 137-147: The Add method appends a Connection without checking for
duplicate names; update Store.Add to load existing connections (using s.Load()),
iterate them and if any existing.Connection.Name equals the incoming conn.Name
return a descriptive error (e.g., "connection name already exists") instead of
appending, otherwise set conn.LastUsed and call s.Save(connections). Ensure you
reference the Connection.Name field and preserve current behavior of LastUsed
and Save when no duplicate is found.
In `@tui/schema.go`:
- Around line 63-68: The viewSchema method can dereference a nil currentConn
when m.loading is true; update model.viewSchema to check m.currentConn for nil
before accessing m.currentConn.Name (or use a safe fallback string) so that when
m.loading is true and m.currentConn == nil it does not panic; locate the
viewSchema function and guard the m.currentConn.Name access (or render a
placeholder) to ensure safe execution.
---
Nitpick comments:
In `@store/store.go`:
- Around line 208-210: The deriveKey function uses Argon2 with very low
parameters (argon2.IDKey in deriveKey); update it to use stronger defaults
(e.g., time=2 and memory=19456) and keep parallelism and key length as
appropriate, and make these parameters configurable (via constants, config
struct or environment variables) so production deployments can raise CPU/memory
cost without changing code; also add a short comment documenting the trade-off
between UX and security and the recommended OWASP baseline.
- Around line 86-93: NewStore currently reads the file once then calls
Store.Load(), causing a second read; avoid the double read by reusing the
already-read bytes: modify NewStore and Store so the bytes read earlier are
passed into decryption instead of calling Load() again — either add a new method
LoadFromBytes(data []byte) (or decryptFromBytes) and call that from NewStore
using the already-read contents, or change Load to accept an optional []byte
parameter and use the provided data when non-nil; update references to
Load/NewStore accordingly (look for NewStore, Store, and Load in the diff) so
password verification uses the cached bytes rather than re-reading the file.
In `@tui/newconn.go`:
- Around line 18-27: The Tab and Shift+Tab handling currently always increments
m.newConnFocus via (m.newConnFocus + 1) % 2, so Shift+Tab behaves the same as
Tab; split the combined case into separate handlers for tea.KeyTab and
tea.KeyShiftTab and update m.newConnFocus accordingly using the length of
m.newConnInputs (increment with +1 and decrement with -1 then add
len(m.newConnInputs) before modulo) so navigation wraps correctly for both
directions; after updating m.newConnFocus keep the same loop that calls Focus()
or Blur() on m.newConnInputs.
In `@tui/password.go`:
- Around line 45-62: store.NewStore(password) performs Argon2 key derivation
which blocks the UI; to avoid a pause, set an "unlocking" state before invoking
loadConnections or have loadConnections emit an immediate unlocking message and
then perform the blocking work in the background. Concretely: in the Update
handler set model.unlocking = true (or dispatch an unlockingMsg) and re-render a
spinner/"Unlocking..." indicator, then return the
tea.Cmd(loadConnections(password)); alternatively modify loadConnections to
first return an unlockingMsg synchronously and then perform
store.NewStore(password) and send connectionsLoadedMsg; reference
loadConnections, store.NewStore, and connectionsLoadedMsg (and add an
unlockingMsg/flag) so the UI shows progress while Argon2 runs.
In `@tui/schema.go`:
- Around line 88-128: The loadSchema function uses context.Background() with no
timeout which can hang; wrap the background context in a cancellable timeout
(e.g., ctx, cancel := context.WithTimeout(context.Background(),
<reasonable-duration>); defer cancel()) and pass that ctx into db.PingContext
and database.InspectSchema (and any other DB/long-running calls like graph.Build
or render.Render if they accept contexts) so the operation is bounded and
resources are released on timeout; adjust the timeout value or make it
configurable as needed and ensure defer cancel() is present to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7ec93b2-30c2-49ee-9d38-503a0cfe0d95
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
CLAUDE.mdcmd/dbtree/main.gogo.modstore/store.gotui/menu.gotui/model.gotui/newconn.gotui/password.gotui/schema.gotui/styles.gotui/tui.go
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@store/store.go`:
- Around line 144-217: The Add, Remove and TouchLastUsed methods currently
perform load-modify-save without serialization and saveWithSalt writes directly
to s.filePath, risking concurrent overwrites and partial writes; update these
mutators (Add, Remove, TouchLastUsed) to acquire a process-level file lock on
s.filePath (or a dedicated lock file) before calling Load/modify and Save so the
entire read-modify-write is serialized, and modify saveWithSalt to write to a
temp file in the same directory (e.g., s.filePath + ".tmp" with secure
permissions), fsync the temp file and directory, then atomically rename the temp
into s.filePath; ensure the lock is held across the temp-write+rename and
released afterwards and reuse existing Load/Save helper functions where
possible.
In `@tui/password.go`:
- Around line 15-23: When handling tea.KeyEnter in the password input handler,
short-circuit and ignore the Enter key if m.unlocking is already true to prevent
duplicate concurrent calls to loadConnections; check m.unlocking at the top of
the case (the same change should be applied to the other Enter block around
lines 52-68) and only proceed to read password via m.passwordInput.Value(), set
m.passwordErr, flip m.unlocking and call loadConnections when m.unlocking is
false, ensuring no second loadConnections is queued while a previous unlock is
in flight so connectionsLoadedMsg handlers don't overwrite passwordErr or
connStore out of order.
In `@tui/schema.go`:
- Around line 29-57: Concurrent loadSchema requests can overwrite newer views
because responses are applied unconditionally; add a request identifier or echo
of the requested view to each loadSchema call and ignore stale responses in the
schemaLoadedMsg handler. Concretely: when invoking loadSchema from the key
handlers (cases handling "f", "s", "r" and "b" behavior that cancels), attach a
unique request ID or include the requested m.currentConn, m.format and m.shape
with the request; propagate that ID/view through schemaLoadedMsg; then update
the message handling logic that processes schemaLoadedMsg to compare the
incoming ID/view with the model's current expected ID/view (and only apply
output/err if they match), and ensure m.loading is cleared only for matching
responses. Reference functions/values: loadSchema, schemaLoadedMsg, m.loading,
m.currentConn, m.format, m.shape.
- Around line 25-28: The switch on msg.String() only checks for bare "q" and
doesn't support Vim-style ":q" or ":quit"; update the model (tui/schema.go) to
add a command buffer field (e.g., m.commandBuf string) and modify the Update
logic around msg.String(), m.quitting and tea.Quit to: start a command buffer
when ':' is pressed (set m.commandBuf=":"), append subsequent key inputs to
m.commandBuf, handle Enter to parse commands (treat ":q" and ":quit" as quit and
call tea.Quit while setting m.quitting), and handle Escape/backspace to clear or
edit the buffer; keep existing single-key "q" behavior but ensure the new
command parsing lives alongside it in the same Update path that currently checks
msg.String().
- Around line 100-108: The MySQL DSN must be built via mysql.Config.FormatDSN()
(or equivalent) instead of simply trimming "mysql://"; update the branch that
handles conn.Driver == "mysql" so you parse connURL (as a URL), populate a
mysql.Config (set User, Pass, Net="tcp", Addr=host[:port], DBName=path without
leading '/', and any query params), call cfg.FormatDSN(), and assign that to
connURL before calling sql.Open(conn.Driver, connURL). Ensure the logic still
handles sqlite3 trimming for sqlite:// and that sql.Open is called with the
formatted MySQL DSN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20a1e1c9-ffc2-4a48-8ad4-70a92b6a51c8
📒 Files selected for processing (6)
CLAUDE.mdstore/store.gotui/model.gotui/newconn.gotui/password.gotui/schema.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tui/model.go
- tui/newconn.go
- CLAUDE.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #8