Files
pad/internal/server/handlers_changes.go
xarmian bf5ab5b366 chore: clear staticcheck SA + U1000 findings on main (TASK-764) (#249)
* chore: clear cosmetic staticcheck findings (TASK-764)

Apply zero-behavior-change fixes for 8 staticcheck findings on main:

- SA4023 cmd/pad/main.go:431 — drop always-true `if eventBus != nil`
  guard. eventBus is wrapped in metrics.NewInstrumentedBus a few lines
  above, which returns a concrete *InstrumentedBus that is never nil.
- SA1019 cmd/pad/main.go:3926 — replace deprecated strings.Title with
  golang.org/x/text/cases.Title(language.English).String. golang.org/x/text
  was already an indirect dep; now promoted to direct.
- SA4031 internal/server/handlers_changes.go:130 — delete dead
  `if updatedItems == nil { ... }` block. make([]T, n) always returns
  non-nil; the JSON marshalling already produced [] not null.
- SA9003 cmd/pad/init.go:351 — delete empty if branch and fold its
  intent into the surrounding comment.
- SA9003 internal/server/handlers_dashboard.go:125 — replace empty
  `if err == nil { ... }` branch with `_ = json.Unmarshal(...)` to
  match the sibling settings parse and document the best-effort intent.
- SA4006 internal/cli/format.go:153 — drop the dead initial
  `titlePart := item.Title` (overwritten in both branches below);
  declare titlePart with `var` instead.
- SA4006 internal/store/workspaces.go:70 — drop the dead first call
  to s.uniqueSlug; only the workspace-specific uniqueWorkspaceSlug
  is meaningful (workspace slugs are globally unique, not workspace-
  scoped like collection/item slugs).
- SA4000 internal/store/store_test.go:99 — remove always-true outer
  `if idx := len(connStr) - len(connStr); idx >= 0` and unindent the
  inner '?' query-string split.

go.mod side effects from `go mod tidy` under Go 1.26: golang.org/x/text
moves to direct (used directly now); pquerna/otp, prometheus/client_*
and trustelem/zxcvbn move from indirect to direct (they were already
used directly — Go 1.26's tidy correctly classifies them).

Verified:
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass (including the replaceDBName test path)
- `staticcheck -checks "SA1019,SA4000,SA4006,SA4023,SA4031,SA9003"` clean
  except for handlers_dashboard.go:221 (SA4006, dashboard visibility-
  filter dead block — handled in TASK-765)

Parent: PLAN-644.

* fix: clear SA5011 nil-deref in buildReconcileFindings (TASK-764)

extractItemStatus(item.Fields) on the first line of the function would
have panicked on a nil item before the `if item != nil && item.CodeContext
== nil` guard could fire. Staticcheck SA5011 flagged the inconsistency.

Drop the (item != nil) half of the guard — the function now documents
its non-nil contract in the doc comment. All callers (reconcile.go:204
plus three sites in cmd/pad/reconcile_test.go) already pass non-nil,
so this is documentation, not behaviour change.

Verified:
- `go build ./...` clean
- `go test ./cmd/pad/...` passes (the existing reconcile tests cover the
  contract)
- `staticcheck -checks SA5011 ./...` clean

Parent: PLAN-644.

* chore: silence SA4017 false positive in watchCmd SSE loop (TASK-764)

cmd/pad/main.go SSE keepalive branch:

    if strings.HasPrefix(line, ":") {
        continue
    }

Staticcheck SA4017 reports "HasPrefix doesn't have side effects and
its return value is ignored" — but the return value IS used as the
if condition. Two sibling strings.HasPrefix calls earlier in the same
for-loop body (matching "event: " and "data: " prefixes) are not
flagged, which strongly suggests an SSA-analysis quirk specific to
this branch rather than a real defect.

Suppress the finding with a //lint:ignore directive that explains
the false positive in-place. Rewriting to a different form (extract
to a bool var, comma-OK on a synthetic value, etc.) would be uglier
than the suppression comment.

Verified:
- `staticcheck -checks SA4017 ./...` clean
- `go build ./...` clean

Parent: PLAN-644.

* chore: delete dead code flagged by U1000 (TASK-764)

Pre-launch (no external contributors yet) — no consumer fork can be
relying on these unreferenced symbols, so we delete them rather than
carry the maintenance burden into v1.

## Helpers (14 functions, 1 type)

cmd/pad/main.go
- progressBar — never called

internal/cli/format.go
- stripHTMLTags — never called

internal/server/handlers_dashboard_test.go
- updateItem (test helper) — never called from any test

internal/server/handlers_items.go
- publishItemEvent — wrapper over publishItemEventWithName; all 5 call
  sites use the *WithName variant directly.
- resolveRelationFields — never called.
- resolveRelationFieldFiltersForWorkspace, resolveRelationFieldFilters,
  relationFilterKeys, resolveRelationFilterValue — closed loop of dead
  helpers (each one only called by another dead one in the family).
- extractStatus — never called (cmd/pad/reconcile.go has its own copy).

internal/server/handlers_versions.go
- handleGetDiff (HTTP handler) — never wired into setupRouter.
- diffsToChanges, diffChange (type) — only used by handleGetDiff above.
- Removes now-unused imports `strconv` and `dmp` (sergi/go-diff).

internal/server/middleware_ratelimit.go
- writeTooManyRequests — never called; the live ratelimit middleware
  uses a dedicated 429 path with Retry-After-Bucket headers.

internal/server/server.go
- guestVisibleItemIDs — never called. handlers_events.go had a
  comment cross-reference; updated to drop the reference.

## Constants

internal/events/redis_bus.go
- reconnectDelay — never read.

internal/store/api_tokens.go
- defaultTokenExpiryDays — never read.

## Out of scope
The 5 unwired handlers in internal/server/handlers_documents.go are
left alone: they are the subject of TASK-769 (a product decision —
wire up vs. delete — that may want different treatment per handler).

The two SA4006/SA4010 findings on internal/server/handlers_dashboard.go
visibility-filter block are similarly left for TASK-765.

## Verified
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass
- `staticcheck -checks "SA*,U1000" ./...` clean except the two TASK-
  765 / TASK-769 follow-ups noted above.

Parent: PLAN-644.

* docs: correct caller name in buildReconcileFindings doc (TASK-764)

Codex round 1 caught: the doc comment named the caller `reconcileSingle`
but the actual function is `reconcileItem` (cmd/pad/reconcile.go:204).
Fix the contract comment so it doesn't go stale on the first git blame.
2026-04-25 11:53:31 -04:00

146 lines
4.5 KiB
Go

package server
import (
"net/http"
"strconv"
"time"
)
// ChangesResponse is the response for GET /workspaces/{ws}/changes?since=<unix_ms>.
type ChangesResponse struct {
// Updated items (with full item data).
Updated []interface{} `json:"updated"`
// IDs of items that were deleted since the requested timestamp.
Deleted []string `json:"deleted"`
// Server timestamp at the time of this response (unix ms).
// Clients should use this as the `since` value for the next sync.
ServerTime int64 `json:"server_time"`
// Whether the collection metadata (counts, schemas) may have changed.
// True if any items were updated/deleted, signaling the client should
// also refresh collection metadata.
CollectionsChanged bool `json:"collections_changed"`
}
// handleGetChanges returns items modified since a given timestamp.
// GET /api/v1/workspaces/{ws}/changes?since=<unix_milliseconds>
//
// This is the incremental sync endpoint used by the frontend when the
// tab regains focus. Instead of re-fetching everything, the client sends
// the timestamp of its last successful sync and gets back only the delta.
func (s *Server) handleGetChanges(w http.ResponseWriter, r *http.Request) {
workspaceID, ok := s.getWorkspaceID(w, r)
if !ok {
return
}
sinceStr := r.URL.Query().Get("since")
if sinceStr == "" {
writeError(w, http.StatusBadRequest, "bad_request", "since query parameter is required (unix milliseconds)")
return
}
sinceMs, err := strconv.ParseInt(sinceStr, 10, 64)
if err != nil || sinceMs < 0 {
writeError(w, http.StatusBadRequest, "bad_request", "since must be a valid unix timestamp in milliseconds")
return
}
since := time.UnixMilli(sinceMs)
serverTime := time.Now().UnixMilli()
updated, deletedIDs, err := s.store.ItemsModifiedSince(workspaceID, since)
if err != nil {
writeInternalError(w, err)
return
}
// Filter by collection visibility so restricted members only see
// changes from collections they have access to.
visibleIDs, visErr := s.visibleCollectionIDs(r, workspaceID)
if visErr != nil {
writeInternalError(w, visErr)
return
}
// For guests with item-level grants, apply item-level filtering
// so they only see changes to items they actually have grants on.
fullCollIDs, grantedItemIDs, grantErr := s.guestResourceFilter(r, workspaceID)
if grantErr != nil {
writeInternalError(w, grantErr)
return
}
grantedItemSet := make(map[string]bool, len(grantedItemIDs))
for _, id := range grantedItemIDs {
grantedItemSet[id] = true
}
if visibleIDs != nil {
filtered := updated[:0]
allowedDeleted := deletedIDs[:0]
visibleSet := make(map[string]bool, len(visibleIDs))
for _, id := range visibleIDs {
visibleSet[id] = true
}
// For guests, build a set of full-collection-grant IDs to distinguish
// between full-collection access and item-only access.
fullCollSet := make(map[string]bool, len(fullCollIDs))
for _, id := range fullCollIDs {
fullCollSet[id] = true
}
for _, item := range updated {
if !visibleSet[item.CollectionID] {
continue
}
// For guests: if the collection is only visible via item grants,
// check the specific item is granted.
if len(grantedItemIDs) > 0 && !fullCollSet[item.CollectionID] && !grantedItemSet[item.ID] {
continue
}
filtered = append(filtered, item)
}
updated = filtered
// For deleted items, re-query with collection filter since we
// only have IDs. Fetch their collection_ids from the soft-deleted rows.
if len(deletedIDs) > 0 {
delItems, delErr := s.store.GetDeletedItemsWithCollection(workspaceID, deletedIDs)
if delErr != nil {
writeInternalError(w, delErr)
return
}
for _, item := range delItems {
if !visibleSet[item.CollectionID] {
continue
}
if len(grantedItemIDs) > 0 && !fullCollSet[item.CollectionID] && !grantedItemSet[item.ID] {
continue
}
allowedDeleted = append(allowedDeleted, item.ID)
}
deletedIDs = allowedDeleted
}
}
// Convert to interface slice for JSON marshaling. make() always
// returns a non-nil slice, which serialises to "[]" rather than
// "null", so we only need to guard deletedIDs (which can be nil
// when not initialised above).
updatedItems := make([]interface{}, len(updated))
for i, item := range updated {
updatedItems[i] = item
}
if deletedIDs == nil {
deletedIDs = []string{}
}
resp := ChangesResponse{
Updated: updatedItems,
Deleted: deletedIDs,
ServerTime: serverTime,
CollectionsChanged: len(updated) > 0 || len(deletedIDs) > 0,
}
writeJSON(w, http.StatusOK, resp)
}