Open
Conversation
WalkthroughAdds a new SQLite-backed local ticketing backend with seed-file import, an embedded HTTP UI and handler, config-driven initialization (ticketing.backend = "local"), validation rejecting legacy task_file, and wiring into main to start/shutdown the local UI server. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Dev Browser
participant UI as Local UI Server
participant Handler as Local UI Handler
participant Backend as Local Backend
participant DB as SQLite DB
Browser->>UI: GET / (index.html)
UI-->>Browser: serve SPA (embedded index.html)
Browser->>UI: GET /api/tickets
UI->>Handler: route -> handler logic
Handler->>Backend: ListTickets / GetTicket
Backend->>DB: SQL SELECT queries
DB-->>Backend: rows
Backend-->>Handler: ticket JSON
Handler-->>UI: JSON response
UI-->>Browser: render tickets
Browser->>UI: POST /api/tickets (create)
UI->>Handler: CreateTicket
Handler->>Backend: CreateTicket (transaction)
Backend->>DB: INSERT ticket + event
DB-->>Backend: ok
Backend-->>Handler: success
Handler-->>UI: 201 Created
UI-->>Browser: show created ticket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
pkg/plugin/ticketing/noop/noop_test.go (1)
21-29: Consider usingt.Runsubtests for better test isolation.The test works correctly, but grouping four different method calls means a failure in an earlier method will mask whether subsequent methods work. Using subtests would provide clearer output and independent execution.
♻️ Optional refactor using t.Run subtests
func TestLifecycleMethods_AreNoOps(t *testing.T) { backend := New() ctx := context.Background() - require.NoError(t, backend.MarkInProgress(ctx, "TICKET-1")) - require.NoError(t, backend.MarkComplete(ctx, "TICKET-1", engine.TaskResult{Summary: "done"})) - require.NoError(t, backend.MarkFailed(ctx, "TICKET-1", "boom")) - require.NoError(t, backend.AddComment(ctx, "TICKET-1", "comment")) + t.Run("MarkInProgress", func(t *testing.T) { + require.NoError(t, backend.MarkInProgress(ctx, "TICKET-1")) + }) + t.Run("MarkComplete", func(t *testing.T) { + require.NoError(t, backend.MarkComplete(ctx, "TICKET-1", engine.TaskResult{Summary: "done"})) + }) + t.Run("MarkFailed", func(t *testing.T) { + require.NoError(t, backend.MarkFailed(ctx, "TICKET-1", "boom")) + }) + t.Run("AddComment", func(t *testing.T) { + require.NoError(t, backend.AddComment(ctx, "TICKET-1", "comment")) + }) }As per coding guidelines: "Prefer table-driven tests using
t.Runwith subtests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/plugin/ticketing/noop/noop_test.go` around lines 21 - 29, Split the single TestLifecycleMethods_AreNoOps into independent subtests using t.Run so each backend method is run in its own subtest (e.g., "MarkInProgress", "MarkComplete", "MarkFailed", "AddComment"); create the backend with New() and a context for the parent test, then for each subtest call the corresponding method (MarkInProgress, MarkComplete, MarkFailed, AddComment) and assert require.NoError within that subtest so failures are isolated and reported per-method.pkg/plugin/ticketing/local/seed.go (1)
45-81: Consider validating that seed tasks have non-empty titles.The function validates that each task has a non-empty ID but does not validate titles. Given that
ticketInsertValuesin store.go requires non-empty titles, a seed file with an empty title would fail during import with a less descriptive error. Based on learnings: "Validate all external input (ticket descriptions, plugin responses, webhook payloads) in Go code".♻️ Optional: Add title validation
for _, task := range tasks { if task.ID == "" { return nil, fmt.Errorf("seed file %q contains a task with an empty id", path) } + if task.Title == "" { + return nil, fmt.Errorf("seed file %q contains task %q with an empty title", path, task.ID) + } if _, exists := seen[task.ID]; exists { return nil, fmt.Errorf("seed file %q contains duplicate ticket id %q", path, task.ID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/plugin/ticketing/local/seed.go` around lines 45 - 81, The loadSeedTickets function currently only validates task.ID; add a non-empty title check inside the loop that returns a clear error if task.Title == "" (include the seed path and task.ID in the message). Update the loop in loadSeedTickets (where tasks are iterated and seen map is maintained) to validate title before appending to tickets and ensure the error text mirrors existing style (fmt.Errorf("seed file %q contains a task with an empty title for id %q", path, task.ID)). This prevents later DB insert failures and surfaces a descriptive validation error early.internal/localui/handler.go (2)
220-259: String-matching for error classification is fragile.
statusForrelies on substring matching (e.g.,"not found","already exists") which can break if error messages change or match unintended errors. Consider using sentinel errors or error types for more robust classification.♻️ Consider using sentinel errors or typed errors
Define sentinel errors in the local ticketing package:
// In pkg/plugin/ticketing/local/errors.go var ( ErrTicketNotFound = errors.New("ticket not found") ErrTicketExists = errors.New("ticket already exists") )Then use
errors.IsinstatusFor:func statusFor(err error) int { switch { case err == nil: return http.StatusOK - case strings.Contains(err.Error(), "not found"): - return http.StatusNotFound - case strings.Contains(err.Error(), "no rows in result set"): + case errors.Is(err, localticket.ErrTicketNotFound): return http.StatusNotFound + case errors.Is(err, localticket.ErrTicketExists): + return http.StatusConflict // ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/handler.go` around lines 220 - 259, The statusFor function uses fragile string matching on error.Error(); replace those checks with error type/sentinel comparisons: define exported sentinel errors (e.g., ErrTicketNotFound, ErrTicketExists, ErrConstraintFailed, ErrInProgress) in the ticketing/local package and ensure the code that returns those errors wraps them (using fmt.Errorf("%w", ...) or errors.Wrap) so callers can be matched; then update statusFor to use errors.Is(err, pkg.ErrTicketNotFound)/errors.Is(err, pkg.ErrTicketExists)/errors.Is(...) instead of strings.Contains for those cases while keeping the existing JSON/EOF/type checks intact.
99-136: Validate required fields before calling the service.The handler trims inputs but does not validate that required fields like
IDandTitleare non-empty before callingCreateTicket. While the service may reject empty values, returning a 400 from the handler with a clear message improves the API contract.♻️ Optional: Add validation for required fields
ticket := ticketing.Ticket{ ID: strings.TrimSpace(req.ID), Title: strings.TrimSpace(req.Title), Description: strings.TrimSpace(req.Description), TicketType: strings.TrimSpace(req.TicketType), Labels: trimSlice(req.Labels), RepoURL: strings.TrimSpace(req.RepoURL), ExternalURL: strings.TrimSpace(req.ExternalURL), } + if ticket.ID == "" { + h.writeError(w, fmt.Errorf("ticket id must not be empty"), http.StatusBadRequest) + return + } + if ticket.Title == "" { + h.writeError(w, fmt.Errorf("ticket title must not be empty"), http.StatusBadRequest) + return + } err := h.service.CreateTicket(r.Context(), ticket)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/handler.go` around lines 99 - 136, In handleCreateTicket, validate required fields (at least ID and Title) after trimming and before calling h.service.CreateTicket: check strings.TrimSpace(req.ID) and strings.TrimSpace(req.Title) (and any other required fields such as Description if desired), and if any are empty call h.writeError with an explanatory error (e.g., "id is required" / "title is required") and http.StatusBadRequest, rather than proceeding to h.service.CreateTicket; keep using existing helpers (decodeJSONBody, trimSlice) and preserve subsequent calls to h.service.CreateTicket and h.service.GetTicket when validation passes.pkg/plugin/ticketing/local/store.go (1)
43-55: GetTicket does not wrap sql.ErrNoRows with a descriptive error.When the ticket is not found,
scanStoredTicketwill returnsql.ErrNoRows, which is passed through without context. This makes error classification in the handler'sstatusForfunction rely on string matching for "no rows in result set".♻️ Wrap ErrNoRows for consistent error handling
func (b *Backend) GetTicket(ctx context.Context, id string) (*StoredTicket, error) { row := b.db.QueryRowContext(ctx, `SELECT `+ticketSelectColumns+` FROM tickets WHERE id = ?`, id, ) ticket, err := scanStoredTicket(row) if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, fmt.Errorf("ticket %q not found", id) + } return nil, err } return &ticket, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/plugin/ticketing/local/store.go` around lines 43 - 55, GetTicket passes sql.ErrNoRows from scanStoredTicket through unchanged, so callers must string-match the message; update GetTicket to detect sql.ErrNoRows (from scanStoredTicket) and return a wrapped or sentinel error instead (e.g., fmt.Errorf("ticket %s: not found: %w", id, sql.ErrNoRows) or return a package-level ErrTicketNotFound) to give callers a clear, typed signal; change the return path in GetTicket (where err != nil) to check errors.Is(err, sql.ErrNoRows) and return the wrapped/sentinel error while leaving other errors unchanged so handlers like statusFor can reliably check the condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/robodev/main.go`:
- Around line 706-717: The local ticketing UI is being mounted on the probe
listener (see localBackend use with localui.NewHandler and
mux.Handle("/local/"...) / mux.HandleFunc("/local"...)), exposing mutable POST
endpoints to unauthenticated consumers; fix by moving the local UI off the probe
server: create a dedicated server (separate http.Serve/ListenAndServe) with its
own mux and attach localUIHandler there (or alternatively wrap localUIHandler
with an authentication middleware) and ensure the new listener is bound
appropriately (e.g., localhost-only or protected port) so the probe listener no
longer registers "/local/" routes.
In `@docs/getting-started/configuration.md`:
- Around line 146-148: Update the migration note for the deprecated
ticketing.config.task_file to mention that, in addition to setting
ticketing.backend: local and ticketing.config.seed_file, you must also configure
ticketing.config.store_path (the local datastore path) for the controller to
start; edit the text that references ticketing.backend,
ticketing.config.seed_file, and ticketing.config.task_file to add a brief
sentence stating that ticketing.config.store_path is required and what it should
point to.
In `@internal/config/validate.go`:
- Around line 102-106: The config currently only validates seed_file when it's a
non-empty string, allowing non-string values like 123 to slip through; update
the Load()/Validate() logic that inspects c.Ticketing.Config (the "seed_file"
key) to explicitly reject non-string types: if "seed_file" is present and not a
string, return a clear error (e.g., via Validate() return) identifying
"ticketing.config.seed_file" as the wrong type; keep using
validateStorePath("ticketing.config.seed_file", seedFile) when the value is a
non-empty string to validate the path.
In `@internal/localui/index.html`:
- Around line 576-577: The ticketId is interpolated directly into API paths
(e.g., api(`/api/tickets/${ticketId}`) and
api(`/api/tickets/${ticketId}/comments`)), which breaks when IDs contain
reserved characters; wrap ticketId with encodeURIComponent(...) everywhere it’s
used in path templates (including the other occurrences noted around lines
642-644 and 656) so all calls to api(...) use
api(`/api/tickets/${encodeURIComponent(ticketId)}`) and
api(`/api/tickets/${encodeURIComponent(ticketId)}/comments`) to properly escape
slashes, question marks, hashes, etc.
- Around line 523-524: The requeue button enablement should only allow terminal
ticket states (not "ready"); change the check on
document.getElementById("requeue-button").disabled to use a terminalStates set
(e.g. ['completed','failed','canceled'] or whatever the app uses) and enable the
button only when ticket.state is one of those terminal states; additionally,
after the requeue action completes successfully, invoke the UI refresh routine
that repopulates the sidebar/pills/stats (call the existing function that
refreshes the ticket list—e.g.
refreshTicketList()/updateTicketList()/refreshSidebarPills()—from the requeue
success handler so the sidebar and stats are updated as well).
In `@pkg/plugin/ticketing/local/schema.go`:
- Around line 57-74: openDatabase and migrate perform blocking I/O without a
context; change their signatures to accept ctx context.Context, propagate ctx
into ensureStoreDirectory (or update ensureStoreDirectory to accept ctx and use
context-aware file ops), replace sql.Open + db.Exec calls with context-aware
variants (use sql.OpenContext or keep sql.Open but ensure subsequent operations
like ExecContext for "PRAGMA journal_mode=WAL" and any migrations use
ExecContext/QueryContext), pass ctx into migrate so migrations use
db.ExecContext/Tx.ExecContext, and preserve behavior such as
db.SetMaxOpenConns(1) for ":memory:" while ensuring resources are closed on
context cancellation/errors.
In `@pkg/plugin/ticketing/local/types.go`:
- Around line 84-92: The unused struct fileTicket is flagged by linters and
blocks CI; either remove the fileTicket type declaration from
pkg/plugin/ticketing/local/types.go or wire it up where seed parsing uses it
(e.g., the seed parser function that decodes ticket YAML/JSON) so the type is
referenced; if keeping it, move it next to the parser or update the parser to
unmarshal into fileTicket (referencing fileTicket) so golangci-lint no longer
reports it as unused.
---
Nitpick comments:
In `@internal/localui/handler.go`:
- Around line 220-259: The statusFor function uses fragile string matching on
error.Error(); replace those checks with error type/sentinel comparisons: define
exported sentinel errors (e.g., ErrTicketNotFound, ErrTicketExists,
ErrConstraintFailed, ErrInProgress) in the ticketing/local package and ensure
the code that returns those errors wraps them (using fmt.Errorf("%w", ...) or
errors.Wrap) so callers can be matched; then update statusFor to use
errors.Is(err, pkg.ErrTicketNotFound)/errors.Is(err,
pkg.ErrTicketExists)/errors.Is(...) instead of strings.Contains for those cases
while keeping the existing JSON/EOF/type checks intact.
- Around line 99-136: In handleCreateTicket, validate required fields (at least
ID and Title) after trimming and before calling h.service.CreateTicket: check
strings.TrimSpace(req.ID) and strings.TrimSpace(req.Title) (and any other
required fields such as Description if desired), and if any are empty call
h.writeError with an explanatory error (e.g., "id is required" / "title is
required") and http.StatusBadRequest, rather than proceeding to
h.service.CreateTicket; keep using existing helpers (decodeJSONBody, trimSlice)
and preserve subsequent calls to h.service.CreateTicket and h.service.GetTicket
when validation passes.
In `@pkg/plugin/ticketing/local/seed.go`:
- Around line 45-81: The loadSeedTickets function currently only validates
task.ID; add a non-empty title check inside the loop that returns a clear error
if task.Title == "" (include the seed path and task.ID in the message). Update
the loop in loadSeedTickets (where tasks are iterated and seen map is
maintained) to validate title before appending to tickets and ensure the error
text mirrors existing style (fmt.Errorf("seed file %q contains a task with an
empty title for id %q", path, task.ID)). This prevents later DB insert failures
and surfaces a descriptive validation error early.
In `@pkg/plugin/ticketing/local/store.go`:
- Around line 43-55: GetTicket passes sql.ErrNoRows from scanStoredTicket
through unchanged, so callers must string-match the message; update GetTicket to
detect sql.ErrNoRows (from scanStoredTicket) and return a wrapped or sentinel
error instead (e.g., fmt.Errorf("ticket %s: not found: %w", id, sql.ErrNoRows)
or return a package-level ErrTicketNotFound) to give callers a clear, typed
signal; change the return path in GetTicket (where err != nil) to check
errors.Is(err, sql.ErrNoRows) and return the wrapped/sentinel error while
leaving other errors unchanged so handlers like statusFor can reliably check the
condition.
In `@pkg/plugin/ticketing/noop/noop_test.go`:
- Around line 21-29: Split the single TestLifecycleMethods_AreNoOps into
independent subtests using t.Run so each backend method is run in its own
subtest (e.g., "MarkInProgress", "MarkComplete", "MarkFailed", "AddComment");
create the backend with New() and a context for the parent test, then for each
subtest call the corresponding method (MarkInProgress, MarkComplete, MarkFailed,
AddComment) and assert require.NoError within that subtest so failures are
isolated and reported per-method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3a38535-bae1-46b8-a4ba-6d583d019ddc
📒 Files selected for processing (17)
cmd/robodev/main.godocs/getting-started/configuration.mddocs/plugins/ticketing.mdinternal/config/config_test.gointernal/config/validate.gointernal/localui/handler.gointernal/localui/handler_test.gointernal/localui/index.htmlpkg/plugin/ticketing/local/backend.gopkg/plugin/ticketing/local/backend_test.gopkg/plugin/ticketing/local/schema.gopkg/plugin/ticketing/local/seed.gopkg/plugin/ticketing/local/store.gopkg/plugin/ticketing/local/types.gopkg/plugin/ticketing/noop/noop.gopkg/plugin/ticketing/noop/noop_test.gotests/integration/localdev_test.go
Comment on lines
+523
to
+524
| document.getElementById("requeue-button").disabled = ticket.state === "in_progress"; | ||
| } |
There was a problem hiding this comment.
Only enable requeue for terminal tickets, and refresh the list after it runs.
The current check leaves the button enabled for ready tickets even though the UI/docs describe requeue as a terminal-state action, and a successful requeue only updates the detail pane. The sidebar pills and stats stay stale until a full page refresh.
Also applies to: 651-658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/localui/index.html` around lines 523 - 524, The requeue button
enablement should only allow terminal ticket states (not "ready"); change the
check on document.getElementById("requeue-button").disabled to use a
terminalStates set (e.g. ['completed','failed','canceled'] or whatever the app
uses) and enable the button only when ticket.state is one of those terminal
states; additionally, after the requeue action completes successfully, invoke
the UI refresh routine that repopulates the sidebar/pills/stats (call the
existing function that refreshes the ticket list—e.g.
refreshTicketList()/updateTicketList()/refreshSidebarPills()—from the requeue
success handler so the sidebar and stats are updated as well).
Comment on lines
+57
to
+74
| func openDatabase(path string) (*sql.DB, error) { | ||
| if err := ensureStoreDirectory(path); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| db, err := sql.Open("sqlite", path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("opening sqlite database: %w", err) | ||
| } | ||
| if path == ":memory:" { | ||
| db.SetMaxOpenConns(1) | ||
| } | ||
| if _, err := db.Exec("PRAGMA journal_mode=WAL"); err != nil { | ||
| db.Close() | ||
| return nil, fmt.Errorf("setting wal mode: %w", err) | ||
| } | ||
|
|
||
| return db, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "schema.go" | grep -E "pkg/plugin/ticketing"Repository: unitaryai/RoboDev
Length of output: 99
🏁 Script executed:
wc -l ./pkg/plugin/ticketing/local/schema.goRepository: unitaryai/RoboDev
Length of output: 103
🏁 Script executed:
cat -n ./pkg/plugin/ticketing/local/schema.goRepository: unitaryai/RoboDev
Length of output: 3737
🏁 Script executed:
rg "openDatabase|migrate\(" ./pkg/plugin/ticketing/local/ -A 2 -B 2Repository: unitaryai/RoboDev
Length of output: 1417
🏁 Script executed:
grep -n "func.*Backend" ./pkg/plugin/ticketing/local/backend.go | head -20Repository: unitaryai/RoboDev
Length of output: 808
🏁 Script executed:
sed -n '33,62p' ./pkg/plugin/ticketing/local/backend.goRepository: unitaryai/RoboDev
Length of output: 693
🏁 Script executed:
rg "func New" ./pkg/plugin/ticketing/ -B 2 -A 10 | head -50Repository: unitaryai/RoboDev
Length of output: 3634
Thread a startup context through DB open and migration.
The openDatabase() and migrate() functions perform blocking I/O (sql.Open, db.Exec, os.MkdirAll) without context.Context, preventing callers from cancelling or bounding startup if the database or filesystem is slow or locked. Add a context parameter to both functions and switch to ExecContext and context-aware I/O operations.
This applies to both lines 57–74 and lines 90–106 per the coding guideline: "Use context.Context for cancellation and timeouts on all I/O operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/plugin/ticketing/local/schema.go` around lines 57 - 74, openDatabase and
migrate perform blocking I/O without a context; change their signatures to
accept ctx context.Context, propagate ctx into ensureStoreDirectory (or update
ensureStoreDirectory to accept ctx and use context-aware file ops), replace
sql.Open + db.Exec calls with context-aware variants (use sql.OpenContext or
keep sql.Open but ensure subsequent operations like ExecContext for "PRAGMA
journal_mode=WAL" and any migrations use ExecContext/QueryContext), pass ctx
into migrate so migrations use db.ExecContext/Tx.ExecContext, and preserve
behavior such as db.SetMaxOpenConns(1) for ":memory:" while ensuring resources
are closed on context cancellation/errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/localui/handler_test.go (1)
125-148:⚠️ Potential issue | 🟡 MinorDon't lock ready-state requeue into the handler contract.
This test creates
LOCAL-2and immediately expects/requeueto succeed while the ticket is stillready. That bakes the same non-terminal requeue behaviour into the suite and will fight the UI fix; move the ticket into a terminal state first, then assert requeue succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/handler_test.go` around lines 125 - 148, The test TestHandler_CreatesCommentsAndRequeuesTickets currently calls the /requeue endpoint while LOCAL-2 is still in the ready state; change the test to transition LOCAL-2 into a terminal state before invoking handler.ServeHTTP for the /api/tickets/LOCAL-2/requeue call — e.g. send a request that patches or transitions the ticket to a terminal state (closed/done) via the handler (using the existing HTTP helpers in the test) after creating/commenting and before the requeue request so the requeue assertion no longer depends on non-terminal behaviour.internal/localui/index.html (2)
523-523:⚠️ Potential issue | 🟡 MinorKeep requeue terminal-only and refresh the list after it runs.
The current disable check still leaves
readytickets requeueable, and the success path only refreshes the detail pane. The sidebar pills and masthead stats stay stale until a full reload.💡 Suggested fix
- document.getElementById("requeue-button").disabled = ticket.state === "in_progress"; + const terminalStates = new Set(["completed", "failed"]); + document.getElementById("requeue-button").disabled = !terminalStates.has(ticket.state);const encodedTicketId = encodeURIComponent(selectedTicketId); await api(`/api/tickets/${encodedTicketId}/requeue`, {method: "POST"}); showToast("Ticket requeued"); + await refreshTickets(); await loadTicket(selectedTicketId);Also applies to: 653-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/index.html` at line 523, The requeue button is incorrectly enabled for non-terminal tickets and only refreshes the detail pane after success; change the disable logic for the element with id "requeue-button" so it is only enabled for terminal tickets (i.e., set disabled = ticket.state !== "terminal" or equivalent) and, in the requeue success handler (the code path that currently refreshes the detail pane), also call the same routines that refresh the sidebar list/pills and masthead stats (or dispatch the existing update/refresh functions used elsewhere) so the sidebar and masthead reflect the requeue immediately.
572-581:⚠️ Potential issue | 🟠 MajorGuard
loadTicketagainst stale responses.Rapidly clicking two tickets can race these requests. A slower response from an older click will overwrite
selectedTicketIdand the detail pane after a newer ticket is selected, so follow-up comment/requeue actions can hit the wrong ticket while the sidebar still highlights the newer row.💡 Suggested fix
async function loadTicket(ticketId) { selectedTicketId = ticketId; + const requestToken = Symbol(ticketId); + loadTicket.latestRequest = requestToken; renderTicketList(ticketsCache); const encodedTicketId = encodeURIComponent(ticketId); const [ticketPayload, commentsPayload] = await Promise.all([ api(`/api/tickets/${encodedTicketId}`), api(`/api/tickets/${encodedTicketId}/comments`) ]); + if (loadTicket.latestRequest !== requestToken) { + return; + } renderTicketDetail(ticketPayload.ticket); renderComments(commentsPayload.comments); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/index.html` around lines 572 - 581, The loadTicket function can race: save the requested ticketId into a local const (e.g., const requestedId = ticketId) before the awaits and after the Promise.all check that selectedTicketId === requestedId (or otherwise compare encodeURIComponent(requestedId) if you prefer) and only call renderTicketDetail and renderComments when they match; if they don’t match, drop/ignore the stale response so the newer selection and sidebar state are not overwritten. Use the existing symbols loadTicket, selectedTicketId, renderTicketDetail, renderComments, and ticketsCache when implementing this guard.
🧹 Nitpick comments (1)
internal/localui/handler_test.go (1)
164-186: Collapse the one-off error-path tests into subtests.The not-found, conflict, validation, and unexpected-error cases all follow the same arrange/request/assert shape. A small table with
t.Runwould trim duplication and make it easier to add new API cases consistently. As per coding guidelines, "Prefer table-driven tests usingt.Runwith subtests".Also applies to: 216-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/handler_test.go` around lines 164 - 186, Combine the separate one-off tests (TestHandler_ReturnsNotFoundForMissingTicket, TestHandler_ReturnsConflictForDuplicateTicket and the similar cases around lines 216-257) into a single table-driven test (e.g., TestHandler_ErrorCases) that iterates a slice of cases and calls t.Run for each subtest; each case struct should include fields like name, method, url, body, and wantStatus, initialize the handler once with newTestHandler, build the httptest.NewRequest and Recorder inside the loop, call handler.ServeHTTP(rec, req), and assert the response code equals wantStatus to preserve the same arrange/request/assert flow while removing duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/robodev/main.go`:
- Around line 163-171: The local SQLite backend created by initLocalBackend is
not closed on shutdown; update the shutdown/cleanup path that currently stops
HTTP servers to also call localBackend.Close() (guarded by non-nil), logging any
error and ensuring Close() is invoked before os.Exit so file handles and DB
state are flushed; modify the branch where cfg.Ticketing.Backend == "local"
(which sets localBackend and controller.WithTicketing(localBackend)) to register
this Close in the same cleanup sequence (or defer it where shutdown is
orchestrated).
---
Duplicate comments:
In `@internal/localui/handler_test.go`:
- Around line 125-148: The test TestHandler_CreatesCommentsAndRequeuesTickets
currently calls the /requeue endpoint while LOCAL-2 is still in the ready state;
change the test to transition LOCAL-2 into a terminal state before invoking
handler.ServeHTTP for the /api/tickets/LOCAL-2/requeue call — e.g. send a
request that patches or transitions the ticket to a terminal state (closed/done)
via the handler (using the existing HTTP helpers in the test) after
creating/commenting and before the requeue request so the requeue assertion no
longer depends on non-terminal behaviour.
In `@internal/localui/index.html`:
- Line 523: The requeue button is incorrectly enabled for non-terminal tickets
and only refreshes the detail pane after success; change the disable logic for
the element with id "requeue-button" so it is only enabled for terminal tickets
(i.e., set disabled = ticket.state !== "terminal" or equivalent) and, in the
requeue success handler (the code path that currently refreshes the detail
pane), also call the same routines that refresh the sidebar list/pills and
masthead stats (or dispatch the existing update/refresh functions used
elsewhere) so the sidebar and masthead reflect the requeue immediately.
- Around line 572-581: The loadTicket function can race: save the requested
ticketId into a local const (e.g., const requestedId = ticketId) before the
awaits and after the Promise.all check that selectedTicketId === requestedId (or
otherwise compare encodeURIComponent(requestedId) if you prefer) and only call
renderTicketDetail and renderComments when they match; if they don’t match,
drop/ignore the stale response so the newer selection and sidebar state are not
overwritten. Use the existing symbols loadTicket, selectedTicketId,
renderTicketDetail, renderComments, and ticketsCache when implementing this
guard.
---
Nitpick comments:
In `@internal/localui/handler_test.go`:
- Around line 164-186: Combine the separate one-off tests
(TestHandler_ReturnsNotFoundForMissingTicket,
TestHandler_ReturnsConflictForDuplicateTicket and the similar cases around lines
216-257) into a single table-driven test (e.g., TestHandler_ErrorCases) that
iterates a slice of cases and calls t.Run for each subtest; each case struct
should include fields like name, method, url, body, and wantStatus, initialize
the handler once with newTestHandler, build the httptest.NewRequest and Recorder
inside the loop, call handler.ServeHTTP(rec, req), and assert the response code
equals wantStatus to preserve the same arrange/request/assert flow while
removing duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bafec447-cc45-4f4c-a494-c86bebc116ba
📒 Files selected for processing (8)
cmd/robodev/main.godocs/getting-started/configuration.mddocs/plugins/ticketing.mdinternal/config/config_test.gointernal/config/validate.gointernal/localui/handler_test.gointernal/localui/index.htmlpkg/plugin/ticketing/local/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/getting-started/configuration.md
- internal/config/validate.go
- internal/config/config_test.go
- pkg/plugin/ticketing/local/types.go
Comment on lines
+163
to
+171
| } else if cfg.Ticketing.Backend == "local" { | ||
| var localErr error | ||
| localBackend, localErr = initLocalBackend(cfg, logger) | ||
| if localErr != nil { | ||
| logger.Error("failed to initialise local ticketing backend", "error", localErr) | ||
| os.Exit(1) | ||
| } | ||
| opts = append(opts, controller.WithTicketing(localBackend)) | ||
| logger.Info("local ticketing backend initialised") |
There was a problem hiding this comment.
Close the SQLite backend during shutdown.
initLocalBackend opens a long-lived SQLite store, but the shutdown path only stops the HTTP servers. Add localBackend.Close() alongside the existing cleanup so file handles and pending DB state are flushed before exit.
💡 Suggested fix
if localUISrv != nil {
if err := localUISrv.Shutdown(shutdownCtx); err != nil {
logger.Error("local ticketing UI server shutdown error", "error", err)
}
}
+ if localBackend != nil {
+ if err := localBackend.Close(); err != nil {
+ logger.Error("local ticketing backend close error", "error", err)
+ }
+ }
if webhookSrv != nil {
if err := webhookSrv.Shutdown(shutdownCtx); err != nil {
logger.Error("webhook server shutdown error", "error", err)
}
}Also applies to: 816-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/robodev/main.go` around lines 163 - 171, The local SQLite backend created
by initLocalBackend is not closed on shutdown; update the shutdown/cleanup path
that currently stops HTTP servers to also call localBackend.Close() (guarded by
non-nil), logging any error and ensuring Close() is invoked before os.Exit so
file handles and DB state are flushed; modify the branch where
cfg.Ticketing.Backend == "local" (which sets localBackend and
controller.WithTicketing(localBackend)) to register this Close in the same
cleanup sequence (or defer it where shutdown is orchestrated).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/localui/index.html (1)
520-530:⚠️ Potential issue | 🟡 MinorKeep requeue terminal-only and refresh the sidebar after it succeeds.
Line 529 still enables requeue for
readytickets, and Line 671 only refreshes the detail pane. That makes the success toast misleading for no-op requeues and leaves the list badges/stats stale until a full reload.💡 Suggested change
Introduce a shared
terminalStatesset near the other state helpers, then update these two spots:- document.getElementById("requeue-button").disabled = ticket.state === "in_progress"; + document.getElementById("requeue-button").disabled = !terminalStates.has(ticket.state); - await loadTicket(selectedTicketId); + await refreshTickets(); + await loadTicket(selectedTicketId);Also applies to: 663-671
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/localui/index.html` around lines 520 - 530, Introduce a shared terminalStates set near the state helpers and make requeue availability depend on it: in renderTicketDetail set document.getElementById("requeue-button").disabled = !terminalStates.has(ticket.state) so only terminal tickets can be requeued; then update the requeue success handler (the code path that currently only refreshes the detail pane) to also trigger the sidebar/list refresh function used elsewhere (the same function that updates ticket list/badges/stats) so the list and badges are refreshed after a successful requeue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/localui/handler.go`:
- Around line 114-124: The handler currently trims request fields then
constructs ticketing.Ticket and calls h.service.CreateTicket, but does not
validate that req.ID and req.Title are non-empty; add validation in
handleCreateTicket after trimming (check strings.TrimSpace(req.ID) and
strings.TrimSpace(req.Title)) and return an HTTP 400 with an explanatory message
if either is empty, preventing calling h.service.CreateTicket with invalid data;
reference the handleCreateTicket function, the req.ID and req.Title fields, the
ticketing.Ticket construction, and the h.service.CreateTicket call when making
the change.
In `@internal/localui/index.html`:
- Around line 641-661: In addComment, prevent double-submits by disabling the
comment button or setting a local "isSubmitting" state at the start of the
function, wrap the async POST call in a try/finally, and re-enable the button /
clear "isSubmitting" in the finally block so the UI is only interactive again
after the request settles (also keep existing showToast, clearing of
`#comment-body` and calling loadTicket(selectedTicketId) inside the try after a
successful await api call and handle errors in the catch to show an error
toast). Reference the addComment function and the API POST to
`/api/tickets/${encodedTicketId}/comments` when applying this change.
---
Duplicate comments:
In `@internal/localui/index.html`:
- Around line 520-530: Introduce a shared terminalStates set near the state
helpers and make requeue availability depend on it: in renderTicketDetail set
document.getElementById("requeue-button").disabled =
!terminalStates.has(ticket.state) so only terminal tickets can be requeued; then
update the requeue success handler (the code path that currently only refreshes
the detail pane) to also trigger the sidebar/list refresh function used
elsewhere (the same function that updates ticket list/badges/stats) so the list
and badges are refreshed after a successful requeue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02a409fc-8204-4f61-b003-716789061897
📒 Files selected for processing (3)
internal/localui/handler.gointernal/localui/handler_test.gointernal/localui/index.html
Comment on lines
+114
to
+124
| ticket := ticketing.Ticket{ | ||
| ID: strings.TrimSpace(req.ID), | ||
| Title: strings.TrimSpace(req.Title), | ||
| Description: strings.TrimSpace(req.Description), | ||
| TicketType: strings.TrimSpace(req.TicketType), | ||
| Labels: trimSlice(req.Labels), | ||
| RepoURL: strings.TrimSpace(req.RepoURL), | ||
| ExternalURL: strings.TrimSpace(req.ExternalURL), | ||
| } | ||
|
|
||
| err := h.service.CreateTicket(r.Context(), ticket) |
There was a problem hiding this comment.
Reject blank ticket IDs and titles in the API handler.
handleCreateTicket only trims the request. A direct POST can bypass the browser checks and still send an empty id or title, which either creates unusable records or turns into an opaque storage error. Fail fast with a 400 before calling CreateTicket.
💡 Suggested change
ticket := ticketing.Ticket{
ID: strings.TrimSpace(req.ID),
Title: strings.TrimSpace(req.Title),
Description: strings.TrimSpace(req.Description),
TicketType: strings.TrimSpace(req.TicketType),
Labels: trimSlice(req.Labels),
RepoURL: strings.TrimSpace(req.RepoURL),
ExternalURL: strings.TrimSpace(req.ExternalURL),
}
+
+ if ticket.ID == "" {
+ h.writeError(w, fmt.Errorf("ticket id must not be empty"), http.StatusBadRequest)
+ return
+ }
+ if ticket.Title == "" {
+ h.writeError(w, fmt.Errorf("ticket title must not be empty"), http.StatusBadRequest)
+ return
+ }
err := h.service.CreateTicket(r.Context(), ticket)As per coding guidelines, "Validate all external input (ticket descriptions, plugin responses, webhook payloads) in Go code".
📝 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.
Suggested change
| ticket := ticketing.Ticket{ | |
| ID: strings.TrimSpace(req.ID), | |
| Title: strings.TrimSpace(req.Title), | |
| Description: strings.TrimSpace(req.Description), | |
| TicketType: strings.TrimSpace(req.TicketType), | |
| Labels: trimSlice(req.Labels), | |
| RepoURL: strings.TrimSpace(req.RepoURL), | |
| ExternalURL: strings.TrimSpace(req.ExternalURL), | |
| } | |
| err := h.service.CreateTicket(r.Context(), ticket) | |
| ticket := ticketing.Ticket{ | |
| ID: strings.TrimSpace(req.ID), | |
| Title: strings.TrimSpace(req.Title), | |
| Description: strings.TrimSpace(req.Description), | |
| TicketType: strings.TrimSpace(req.TicketType), | |
| Labels: trimSlice(req.Labels), | |
| RepoURL: strings.TrimSpace(req.RepoURL), | |
| ExternalURL: strings.TrimSpace(req.ExternalURL), | |
| } | |
| if ticket.ID == "" { | |
| h.writeError(w, fmt.Errorf("ticket id must not be empty"), http.StatusBadRequest) | |
| return | |
| } | |
| if ticket.Title == "" { | |
| h.writeError(w, fmt.Errorf("ticket title must not be empty"), http.StatusBadRequest) | |
| return | |
| } | |
| err := h.service.CreateTicket(r.Context(), ticket) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/localui/handler.go` around lines 114 - 124, The handler currently
trims request fields then constructs ticketing.Ticket and calls
h.service.CreateTicket, but does not validate that req.ID and req.Title are
non-empty; add validation in handleCreateTicket after trimming (check
strings.TrimSpace(req.ID) and strings.TrimSpace(req.Title)) and return an HTTP
400 with an explanatory message if either is empty, preventing calling
h.service.CreateTicket with invalid data; reference the handleCreateTicket
function, the req.ID and req.Title fields, the ticketing.Ticket construction,
and the h.service.CreateTicket call when making the change.
Comment on lines
+641
to
+661
| async function addComment() { | ||
| if (!selectedTicketId) { | ||
| showToast("Select a ticket first"); | ||
| return; | ||
| } | ||
|
|
||
| const body = document.getElementById("comment-body").value.trim(); | ||
| if (!body) { | ||
| showToast("Comment body is empty"); | ||
| return; | ||
| } | ||
|
|
||
| const encodedTicketId = encodeURIComponent(selectedTicketId); | ||
| await api(`/api/tickets/${encodedTicketId}/comments`, { | ||
| method: "POST", | ||
| body: JSON.stringify({body}), | ||
| }); | ||
| document.getElementById("comment-body").value = ""; | ||
| showToast("Comment added"); | ||
| await loadTicket(selectedTicketId); | ||
| } |
There was a problem hiding this comment.
Disable the comment action while the POST is in flight.
This path performs a non-idempotent write, but the button stays clickable until the request settles. A quick double-click will persist the same note twice.
💡 Suggested change
async function addComment() {
if (!selectedTicketId) {
showToast("Select a ticket first");
return;
}
const body = document.getElementById("comment-body").value.trim();
if (!body) {
showToast("Comment body is empty");
return;
}
- const encodedTicketId = encodeURIComponent(selectedTicketId);
- await api(`/api/tickets/${encodedTicketId}/comments`, {
- method: "POST",
- body: JSON.stringify({body}),
- });
- document.getElementById("comment-body").value = "";
- showToast("Comment added");
- await loadTicket(selectedTicketId);
+ const button = document.getElementById("comment-button");
+ button.disabled = true;
+ try {
+ const encodedTicketId = encodeURIComponent(selectedTicketId);
+ await api(`/api/tickets/${encodedTicketId}/comments`, {
+ method: "POST",
+ body: JSON.stringify({body}),
+ });
+ document.getElementById("comment-body").value = "";
+ showToast("Comment added");
+ await loadTicket(selectedTicketId);
+ } finally {
+ button.disabled = false;
+ }
}📝 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.
Suggested change
| async function addComment() { | |
| if (!selectedTicketId) { | |
| showToast("Select a ticket first"); | |
| return; | |
| } | |
| const body = document.getElementById("comment-body").value.trim(); | |
| if (!body) { | |
| showToast("Comment body is empty"); | |
| return; | |
| } | |
| const encodedTicketId = encodeURIComponent(selectedTicketId); | |
| await api(`/api/tickets/${encodedTicketId}/comments`, { | |
| method: "POST", | |
| body: JSON.stringify({body}), | |
| }); | |
| document.getElementById("comment-body").value = ""; | |
| showToast("Comment added"); | |
| await loadTicket(selectedTicketId); | |
| } | |
| async function addComment() { | |
| if (!selectedTicketId) { | |
| showToast("Select a ticket first"); | |
| return; | |
| } | |
| const body = document.getElementById("comment-body").value.trim(); | |
| if (!body) { | |
| showToast("Comment body is empty"); | |
| return; | |
| } | |
| const button = document.getElementById("comment-button"); | |
| button.disabled = true; | |
| try { | |
| const encodedTicketId = encodeURIComponent(selectedTicketId); | |
| await api(`/api/tickets/${encodedTicketId}/comments`, { | |
| method: "POST", | |
| body: JSON.stringify({body}), | |
| }); | |
| document.getElementById("comment-body").value = ""; | |
| showToast("Comment added"); | |
| await loadTicket(selectedTicketId); | |
| } finally { | |
| button.disabled = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/localui/index.html` around lines 641 - 661, In addComment, prevent
double-submits by disabling the comment button or setting a local "isSubmitting"
state at the start of the function, wrap the async POST call in a try/finally,
and re-enable the button / clear "isSubmitting" in the finally block so the UI
is only interactive again after the request settles (also keep existing
showToast, clearing of `#comment-body` and calling loadTicket(selectedTicketId)
inside the try after a successful await api call and handle errors in the catch
to show an error toast). Reference the addComment function and the API POST to
`/api/tickets/${encodedTicketId}/comments` when applying this change.
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.
Summary
ticketing.backend: localSQLite backend with optional one-timeseed_fileimport/local/for listing, creating, commenting on, and requeueing ticketsnoop + task_filelocal-dev path and document the explicit local backend flowTesting
Screenshot
Summary by CodeRabbit