chore: refactor#2
Merged
Merged
Conversation
Move shared enums (RepositoryMode, GitFileStatus, DiffSectionKind, RepositoryRole) into a new common/ namespace and add a ReviewContext discriminated union there. Domain modules re-export the names they need so existing import paths keep working. Add per-domain request DTOs to auth/ (AuthSetupRequest, AuthLoginRequest, AuthResumeRequest, AuthWipeRequest, AuthResumeResponse) and adopt them in the bridge auth client, bridge client facade, and the electron auth IPC handler. Drop the back-compat UIPreferencesResponse alias; consumers use UIPreferences directly. Document the type/interface and request/response conventions in packages/contracts/AGENTS.md. Phase 1 of the monorepo SOLID refactor.
Move every domain method off the *Store god-object onto a dedicated repository type: UserRepo, SessionRepo, ReviewRepo, CommentRepo, PendingCommentRepo, BranchRepo, RepoRepo, PreferenceRepo, WalkthroughRepo. Store is now a thin composition root that owns the *sql.DB, *db.Queries, a shared clock, and one field per repo. Extract the embedded schema apply and the one-shot legacy migrations (drop workflow_* tables, ALTER review_sessions context columns, ALTER *_comments range columns) into a separate Migrator type so the Store constructor's only initialization responsibility is wiring repos. CommentRepo writes review-timeline events as side effects via a small ReviewEventWriter interface satisfied by ReviewRepo, so the two repos stay independently constructible. ReviewRepo.ReviewDetail takes a CommentRepo explicitly when composing the denormalized response. Update all eight services to take the specific repository (or pair of repositories) they need rather than the whole Store. The httpx bootstrap wires services with store.Users, store.Sessions, etc. Tests are pointed at the new repos; the clock setter on Store (SetNow) replaces the prior direct mutation of store.now. Phase 2 of the monorepo SOLID refactor.
review/runner.go: define GitExecutor and GhExecutor interfaces with default implementations (execGit, execGh) that shell out to the git and gh binaries. Package-level vars gitExec/ghExec are swappable via SetGit and SetGh for scoped test overrides; the existing top-level helpers (gitOutput, gitBytes, ghOutput, hasGh) now route through the interfaces so review code is no longer hard-wired to exec. review/status_parser.go and review/difftree_parser.go: lift parseStatus and parseDiffTreeNameStatus into StatusParser and DiffTreeParser types with pure Parse methods that can be tested against captured fixtures without spawning git. Add status_parser_test.go covering plain modifications, renames, and the diff-tree A/D/M/R record shapes. walkthrough/anthropic.go: define AnthropicClient interface with an httpAnthropicClient default impl that wraps the existing HTTP call. walkthrough.Generate now calls client.Messages instead of building the HTTP request inline; SetClient swaps the active client for tests. Phase 3 of the monorepo SOLID refactor.
Server held eight concrete *service.X fields plus a dead StoreFactory. Replace them with a single Services ServiceRegistry interface field. Handlers consume services through the interface (s.Services.Auth(), s.Services.Reviews(), ...) so the transport layer no longer knows the service constructors or the storage repositories that back them. Add registry.go with a private services struct implementing ServiceRegistry; bootstrap.go now calls newServiceRegistry(store) once and hands the interface to Server. The dead StoreFactory field is removed. Phase 4 of the monorepo SOLID refactor.
…port
Replace the 313-line HttpWorkflowBridgeClient god-class with a small
ApiClient facade that composes per-domain client classes:
AuthClient, BranchClient, PendingCommentClient, PreferenceClient,
PullRequestClient, RepositoriesClient, RepositoryClient, ReviewClient,
SystemClient, WalkthroughClient. Each takes an HttpTransport at
construction so consumers depend on a one-method interface rather than
the unix-socket transport directly.
http.ts: extract HttpTransport interface with SocketHttpTransport as
the default implementation. Replace the weak BridgeError type with a
discriminated union keyed by `kind` ("transport" | "auth" | "validation"
| "notFound" | "conflict" | "server"); the kind is derived from the
HTTP status. Add isBridgeError type guard.
createApiClient(socketPath) is the new factory. createApiClientFromTransport
lets tests provide an in-memory transport. The old WorkflowBridgeClient
interface and client-types.ts are removed.
Update every electron IPC handler (auth, branches, pending-comments,
preferences, pull-requests, repositories, repository, reviews, system,
walkthrough) to consume the new shape: `(await client()).auth.setup(req)`
instead of `(await client()).authSetup(req)`. The electron/bridge.ts
module now uses createApiClient directly instead of the removed
createWorkflowBridgeClient + unixTarget helpers.
Wire-format normalization for the ReviewContext discriminated union
is deferred to a later cleanup; bridge consumers still read the flat
`contextKind`/`contextSha` fields for now.
Phase 6 of the monorepo SOLID refactor.
…Router, lifecycle settings-store.ts: introduce SettingsStore interface with an ElectronSettingsStore default implementation owning app.getPath() + fs access. setSettingsStore() swaps the active store for tests; the existing module-level function exports (readSavedSettings, writeSavedSettings, settingsPath, defaultWorkflowDbPath, cleanSettings) become façade wrappers around the active store. bridge.ts: split into bridge-process.ts and bridge.ts (client factory). BridgeProcessHandle wraps the spawned Go child or attached external socket. spawnBridge/killBridge own the subprocess + socket lifecycle; bridge.ts owns the ApiClient + waitForReady plumbing only. ipc/router.ts: introduce IpcRouter with on(channel, handler) and register(ipcMain). Each ipc/*.ts handler module now takes the router and registers via router.on(...) instead of calling ipcMain.handle directly. The top-level ipc.ts builds and registers the router; tests can inspect handler bindings via buildRouter(deps) without an Electron runtime. lifecycle.ts: extract the whenReady/single-instance/window-management/ quit flow out of main.ts into runApp(initialIntent). main.ts shrinks to ~10 lines: parse intent, dispatch to showHelpAndExit or runApp. Phase 7 of the monorepo SOLID refactor.
Introduce packages/ui/src/lib/services/ wrapping the window.diffApp IPC shim in domain-shaped services: AuthService, PreferenceService, RepositoryService, ReviewService, WalkthroughService. A ServiceRegistry composes them and is exposed via a memoized services() accessor; setServices() swaps the registry for tests. Extract useAuthForm composable that owns the submitting/error pair used by both AuthLogin and AuthSetup. AuthLogin and AuthSetup now consume services().auth and useAuthForm so the validation/submit branches are trivial and the bridge dependency is one indirection away. App.vue still calls window.diffApp directly. Migrating its ~20 call sites and extracting useCommentWorkflow / useRepositoryNavigation composables is left for a follow-up to keep this phase shippable. Phase 8 of the monorepo SOLID refactor.
patch.ts becomes a thin barrel re-exporting from patch-parser.ts (pure parsing: parseHunkHeader, parsePatch, splitPatch, splitPatchLines, getHunkInfos, and the PatchLine/SplitRow/HunkInfo types) and patch-expander.ts (the applyExpansions splice that consumes parsed lines plus ExpandedContext records). Same public API, but the two concerns (read raw diff text vs splice in fetched context) no longer share a 345-line file. Worker protocol formalised: workers/highlight-protocol.ts defines HighlightRequest, PrewarmRequest, WorkerRequest, HighlightResponse, PrewarmResponse, ErrorResponse, WorkerResponse, and HighlightThemeId. highlight.worker.ts imports the types and re-exports them so the existing import from "../workers/highlight.worker" keeps working. highlightPool.ts now stamps `satisfies HighlightRequest`/PrewarmRequest on its postMessage payloads so drift between request constructors and the worker's switch on `kind` fails at compile. The bigger DiffBody/TopBar/Sidebar template decompositions and the useContextExpansion / useLineSelection per-instance scoping are noted in the plan as a follow-up branch — each is its own session-sized piece of work and lumping them in here risked breaking the app. Phase 9 (final) of the monorepo SOLID refactor.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a significant architectural refactoring, decomposing the monolithic storage layer into domain-specific repositories and introducing a service registry for improved dependency management. The bridge client and UI service layers were also refactored to use domain-specific sub-clients and a pluggable transport. Feedback identifies critical issues within the new database migrator, notably the destructive schema reset logic and the misuse of connection-scoped SQLite pragmas which may lead to inconsistent states in a connection pool. Additionally, the review points out potential resource leaks from unclosed database rows, SQL injection vulnerabilities from unquoted identifiers, and the need for more robust HTTP response handling to ensure connection reuse.
Replace the Material Theme Darker dark variant and the existing light palette with the monochrome-neutral Pierre identity used across diffs.com, diffshub.com, and trees.software. Token names are preserved so component styles cascade without per-file edits. - style.css: rewrite light and dark :root blocks with Pierre oklch neutrals; sidebar darker than canvas in dark (#101010 vs ~#242424), lighter in light (#f7f7f7 vs #ffffff); --radius dropped to 10px; diff fills repointed at Pierre pairs (#ffdbd6/#ff2e3f delete, #d6f5e0/#00c758 add in light; #3e1715/#ff6762 and #0e2818/#39d97e in dark); drop Primer Primitives imports; font stack switches to Geist (UI) and Berkeley Mono / JetBrains Mono / Fira Code (code). - accent.ts: collapse the five-swatch accent table to a single Pierre-neutral preset so existing UIAccent preferences keep working without a contracts migration; diffBgs() now resolves directly to the Pierre tokens. - licht.json / dunkel.json: align editor canvas, cursor, selection, and line numbers with Pierre neutrals; tokenColors retained.
Run scripts/blank-lines.ts before oxfmt in the Makefile format target so the documented `make format` workflow applies the same blank-line rules that the per-package format scripts already use. Scoped to packages/ui and packages/bridge to match the existing oxfmt scope.
useContextExpansion / useLineSelection no longer hold module-level reactive state — each call to the composable returns a fresh per- instance store, so mounting two consumers in the same window doesn't share (and corrupt) section expansions or selection anchors. useDiffNavigation is split: keyboard-shortcut wiring moves to a new useKeyboardShortcuts composable. useDiffNavigation keeps just the navigation primitives (selectAdjacent, jumpToHunk) so callers can bind them outside of a keydown context. App.vue updates its imports. highlight.ts wraps the per-line tokenize cache, inflight set, and the lazy main-thread fallback in a Highlighter class. Module-level globals (cache map, inflight set, fallbackHighlighter, fallbackInit promise, fallbackLoaded set, currentTheme computed, rev shallowRef) collapse into private fields with a shared singleton driving the existing highlightLine / ensureLanguage / highlighterRev exports. browser-fallback.ts extracts createMockDiffApp() from installBrowser- Fallback so tests and storybook scenarios can spin up a fresh in-memory bridge without touching window.diffApp. Follow-up to the Phase 9 deferred-work list.
Sidebar.vue (299 → 161 LOC): - Extract useFileListFilter composable owning totalCount, viewedCount, filteredFiles, filteredAllPaths, and progressPct so the bookkeeping lives outside the template. - Extract FileListView.vue (changed-files ScrollArea + empty state) and SidebarActions.vue (Submit / Comment / Request button group). - Sidebar now wires the search input + scope toggle + tree view + two subcomponents and forwards their events; the heavy template blocks collapse into single tags. TopBar.vue (503 → 175 LOC): - Extract BranchPicker.vue (DropdownMenu + create-branch Dialog, owns useBranches, createOpen, createName state). - Extract FileSearchPopover.vue (Command/Popover/Input + result list, owns useFileSearch and the basename/dirname helpers). - Extract UserMenu.vue (avatar dropdown + log-out). - TopBar keeps the head-sha pill, diff stats, refresh button, and the tweaks popover inline; the rest delegates to the three new components. Follow-up to the Phase 9 deferred-work list.
DiffBody.vue (1081 → 954 LOC) sheds three concerns into per-instance composables that mirror its existing structure: - useDiffStyles(diffStyle): owns the active DiffStyleColors palette and the bgFor / numBgFor / barFor / sign helpers that branch on a DiffCellKind. The same kind label is exported so the renderer's CellRender union references one source of truth. - useDiffHighlighting(lang): owns highlightHtml, withRanges, escapeHtml, and re-exports computeWordHi. Reads the shared highlighterRev so callers re-render when async tokens land. - useHunkInfo(hideWhitespace): replaces the hand-rolled Map cache with a reactive computed that invalidates automatically on whitespace toggle. Exposes hunksFor / findHunk / nextHunkOldStart. DiffBody.vue's setup now imports the three composables, wires them with toRef(props, ...), and drops ~145 lines of inline helpers. Template unchanged — the rename + extraction is purely script-side, so the visible render path is preserved. Closes the Phase 9 deferred-work list for now; the residual items (DiffBody.vue template split into DiffViewRenderer + lifting useContextExpansion to DiffList, App.vue composable extractions, and the ReviewContext wire-format normalization) remain follow-ups.
Extend the launch parser to accept the same commit-ish and pull-request
shapes codiff supports:
- Full SHAs up to 64 hex chars (SHA-256-ready)
- HEAD / @ revision syntax: HEAD, HEAD~3, HEAD^, HEAD@{1}, @{1}
- `#42` as a shorthand for `pr 42`
- `pr <github-url>` and a bare GitHub PR URL as a positional
- `git-diff <path> <ref>` two-positional form
Add canonical fields `commitRef` and `pullRequestNumber` to LaunchIntent;
keep `sha` / `prNumber` as aliases for one release so renderer code
doesn't break. Existing-path always wins over revision syntax (e.g. a
folder literally named "HEAD~3" still opens as a directory).
Back the new surface on the Go side with two single-responsibility
helpers:
- review.ResolveCommitRef / ResolveParentRef wrap `git rev-parse --verify`
so callers don't reimplement the dance. Used by phase 5's diff loader.
- review.ReadGitHubRemotes parses `git remote -v` into structured
GitHubRemote entries (origin promoted to first). Enables `#42` to
resolve owner/repo without `gh` for future flows.
Tests: 28 cases in launch-intent.test.ts (every pattern, positive +
negative, including the HEAD~3-as-path disambiguation rule); Go tests
cover ref resolution against a temp repo and remote parsing across
git@, https://, ssh:// URL shapes plus origin promotion.
…tartup
Eliminates the N+1 git invocations the audit found across the
repository-state, commit-state, and PR-state paths. Mirrors codiff's
6-8x startup speedup, but in our Go stack.
Three orthogonal changes, each in its own single-responsibility unit:
1. lineparse.SplitUnifiedPatch (new internal/lineparse/split.go)
Splits a multi-file `git diff` output into per-file sections keyed
by b-side path. Handles renames (via `rename to`), deletions,
binary patches, quoted paths with spaces, and bodies that don't
end in a newline. Covered by 10 golden-style tests.
2. review.RootFor + WithRootCache (new internal/review/root_cache.go)
Request-scoped cache for `git rev-parse --show-toplevel`. The 11
call sites that used to invoke rev-parse directly now go through
RootFor; with the cache installed (HTTP middleware), one rev-parse
covers the whole request. Errors are cached too so misconfigured
paths don't trigger retry storms.
3. Batched diffs across three readers:
- ReadRepositoryState: replaces the per-file `git diff [--cached]`
loop with two batched calls, then splits. Also fans out the four
cheap startup queries (branch, head, status, both diffs) in an
errgroup so wall time collapses to the slowest single call.
- ReadCommitState: replaces per-file `git show` with one batched
`git show` + split. Delegates ref resolution to phase 1's
ResolveCommitRef.
- ReadPullRequestState: replaces per-file `git diff base..head`
with one batched call + split.
Wiring: a tiny withRequestCaches middleware installs the cache once
per HTTP request, so every git lookup within the request reuses it.
Bench: ReadRepositoryState on a 50-file working copy lands at ~150 ms
on a VM (replacing what was previously >250 ms of subprocess startup).
Real wins scale with file count.
Adds ~/.git-diff/config.yaml as the user-editable home for app-wide
settings: theme, walkthrough provider/model, budgets, and a fully
remappable keymap. Snake_case keys throughout, hot-reload on save,
served to the renderer through the existing Electron IPC bridge.
Go side — new internal/userconfig package, one file per responsibility
(no god struct):
- schema.go Config / Walkthrough / Keymap structs with mapstructure tags
- defaults.go pure Defaults() Config so the app boots without a file
- loader.go viper-backed Load(path) — defaults merge per-key
- watcher.go fsnotify-driven Watch with a 200 ms debounce; survives
transient parse errors via the onError callback
- writer.go first-run WriteDefaults — idempotent, never overwrites
- reader.go narrow Reader interface + AtomicReader implementation
handlers depend on (Dependency Inversion)
- broker.go pub/sub fan-out for hot-reload subscribers
- service.go thin orchestrator wiring loader → reader → watcher → broker
HTTP layer — GET /v1/userconfig returns the resolved config as JSON,
GET /v1/userconfig/stream is an SSE endpoint that emits a "config" event
on every reload. Snake_case keys preserved on the wire.
TS contracts — new userconfig domain exports UserConfig, Keymap,
KeymapAction, WalkthroughProvider. Mirrors the Go struct 1:1.
Bridge — UserConfigClient with a single get() method; SSE is a follow-up
once the unix-socket transport exposes streaming.
Vue composables — three small units (single responsibility each):
- keymapMatcher.ts pure parseBinding + matchesBinding utility, 15 tests
- useUserConfig.ts module-singleton reactive ref, lazy first fetch
- useKeymap.ts defaults ← server merge as a computed ref
Refactor useKeyboardShortcuts to consume useKeymap — every shortcut
(next/prev file, hunk nav, toggle viewed, file filter, submit comment,
diff search) is now sourced from the merged keymap so editing
config.yaml takes effect on the next keystroke. Arrow keys stay
hardcoded as next/prev-file aliases for ergonomics.
Electron preload + IPC handler + browser-fallback updated to expose
getUserConfig() consistently across the bridge surface.
Tests: 16 Go userconfig tests + 15 TS matcher tests + the existing
suite. All green except the pre-existing App.test.ts Pinia setup
failure that predates this work.
Adds a code-editor-style command palette that any component can register into. The binding to open it comes from the YAML config's keymap.command_bar (default cmd+shift+p) — the same one phase 3 exposed — so users can rebind without touching code. Three single-responsibility units: - useCommandRegistry.ts — module-singleton registry with register / deregister / list. Re-registering an id replaces the existing entry (HMR-safe). When called from a component setup(), the disposer auto-fires on onUnmounted. - formatShortcut.ts — pure binding → display string. "cmd+shift+p" → "⌘⇧P" on macOS, "Cmd+Shift+P" elsewhere. Named keys (enter, escape, arrowdown, …) render as symbols on mac and capitalised labels off mac. - CommandPalette.vue — mounted once in App.vue. Uses the existing shadcn-style Dialog + reka-ui Command primitives (which give us fuzzy filtering for free, no extra dep). Groups commands by section, shows the live shortcut to the right of each row. App.vue registers an initial four commands: toggle whitespace, copy review as Markdown, generate AI walkthrough, copy current file path. More can be added inline by any component. Tests: 11 cases across useCommandRegistry (register / replace / deregister / dispose / async run) and formatShortcut (mac + non-mac symbol tables, named keys, empty input, backslash).
Two coupled reworks in one phase:
(1) New internal/ai package with a narrow Provider interface and a
Registry. Adding a third provider (OpenAI, Bedrock, Ollama) only
requires a new provider_*.go and a Register() call in bootstrap —
no caller edits.
- provider.go Provider interface (ID, DefaultModel,
SupportsModel, Generate). Narrow on purpose;
Streaming/Embedding go in sibling interfaces
when needed (ISP).
- registry.go thread-safe Registry with Register/Get/List
- request_helpers.go cross-provider helpers (model fallback,
combined prompt, capped max-tokens)
- provider_anthropic.go ports the existing /v1/messages logic
out of the walkthrough package. Reads
ANTHROPIC_API_KEY at request time so key
rotation doesn't need a restart.
- provider_codex.go shells out to `codex exec --model <m> --stdin`.
Looks up the binary on PATH then
~/.codex/bin/codex. Fails loudly with
ErrCodexNotInstalled per the user's decision —
no silent fallback.
(2) internal/walkthrough decomposed from a single ~250-line file into
seven single-responsibility units, and the schema upgraded to codiff's
grouped shape (groups → per-file action + impact + note).
- types.go Walkthrough/Group/FileEntry + Action/Impact
enums + Budget. Legacy Order/Notes mirrors
kept for one release.
- fingerprint.go mixes schemaVersion + providerID into the
hash so switching providers (or shipping a new
schema) invalidates cached rows automatically.
- prompt_builder.go pure BuildPrompt(PromptInput); instructs the
model to emit the grouped JSON schema with
kebab-case ids, rationale, action, impact.
- budget_enforcer.go pure enforceBudget; codiff's 160 KB total /
4 KB per-file caps, sourced from user config.
Marks truncated content with [...truncated]
and emits an "omitted after" flag for the
last included file when the total cap fires.
- response_parser.go pure Parse(raw); strips ```json fences,
decodes, and hoists legacy flat order/notes
into a single group for compatibility.
- schema_validator.go pure Validate; drops hallucinated paths,
normalises invalid action/impact enums to
sensible defaults, rejects when every entry
filters away.
- service.go ~80-line orchestrator: budget → prompt →
ai.Provider → parse → validate → assemble.
Service layer (internal/service/walkthrough.go) now:
- Depends on ai.Registry and userconfig.Reader (Dependency Inversion)
- Reads provider/model/budgets from the YAML config
- Stamps the user's preferred model on the request via a thin
modelOverrideProvider adapter so providers stay simple
- Persists the new Groups shape in addition to legacy Order/Notes
Storage adds groups_json + provider_id columns with a migration so
existing user databases upgrade in place. Walkthroughs cached before
this change keep loading and re-generate on next access (schema
version mixed into the fingerprint).
HTTP handler surfaces ErrProviderUnavailable + ErrCodexNotInstalled as
412 Precondition Failed so the renderer can show targeted remediation.
Contracts (TS) extended with WalkthroughGroup / WalkthroughFileEntry /
WalkthroughAction / WalkthroughImpact. WalkthroughPanel.vue rebuilt
as a collapsible accordion: one group per disclosure, action + impact
chips per file, collapse state keyed by walkthrough fingerprint so it
survives within a walkthrough but resets when the diff changes.
Backwards-compat path: legacy order/notes render as one synthetic
"Files" group.
Tests: 10 AI tests (registry + both providers, with a fake-binary
codex injection) + 9 walkthrough tests (fingerprint stability + provider
mixing, prompt builder + budget truncation, parser for both shapes,
validator's path filtering + enum normalisation, orchestrator end-to-end
with a mock provider). All green.
@pierre/diffs is the primary diff parser used by patch-parser.ts (parseViaPierre), with a hand-rolled line scanner as a safety net. @pierre/trees is the file-tree backbone in RepoFileTree.vue. Both were already pulled in as deps in earlier phases — this commit adds the explicit test coverage Phase 6 of the codiff sync plan called for. Three focused tests against a real unified diff sample: - parsePatchFiles surface check: returns at least one parsed file with one hunk. Loudly fails if the beta library's export shape changes (1.2.0-beta.6 is a fast-moving target). - parsePatch round-trip: typed PatchLine[] preserves del/add ordering AND keeps oldLine/newLine anchors on context rows. - getHunkInfos: derives one hunk with the correct boundaries and isLast flag. If pierre regresses, these fail before we silently fall back to the line scanner — protecting the syntax-highlighting + hunk-nav code paths that depend on the typed PatchLine shape. No production-code changes; pierre is already the primary path.
Comments described several types, helpers, and CSS aliases as mirroring or matching codiff. Rewrite them to describe the code on its own terms; no behavior changes.
Removes the ServiceRegistry interface and the *services wrapper — both had a single concrete implementation and were never mocked. Service fields now live directly on Server, construction is inlined into bootstrap.go's Server literal, and Server.Auth is renamed to Session to avoid colliding with the new auth *service.AuthService field.
Removed comments that only paraphrased the symbol they sat on (docstrings restating the function name, section dividers, "owns/bundles/composes" preambles). Tightened the keepers to one or two lines that name a non-obvious WHY: invariants, hidden couplings, gotchas (CPU sampling, SQLite ALTER, scanner trailing newline). Godoc on real interface contracts and package docs stay.
…tracts Same rule as the api pass: drop multi-line JSDoc preambles that paraphrase the symbol they describe (composable summaries, "wraps window.diffApp" narration, "the UI renders" interface restatements). Keep the JSDoc that documents a non-obvious contract (modifier strictness, nil/empty behaviour, deprecated mirrors).
Promote `mattn/go-sqlite3` and `golang-migrate/v4` to direct requires (they're imported with `_` blanks in storage/migrator code), and drop `modernc.org/sqlite` plus its transitive chain — nothing in packages/api imports it; the migrator uses the mattn-backed sqlite3 driver.
Drop detectLegacyDB and the ALTER-based bridge that fixed up pre-golang-migrate SQLite DBs. Fresh DBs only from here on — any pre-baseline DB must be re-created. Removes the add*Columns / dropLegacy* helpers, columnSet, sqliteQuoteIdentifier, BaselineSchemaSQL, the baselineVersion const, and the matching tests. Run() now just calls mg.Up().
Opens the connection via gorm.Open(sqlite.Open(...)) and hands repos the same *sql.DB via gdb.DB(), so every existing query keeps working while the GORM-based port lands incrementally. Adds models.go with row structs for every table (no associations, by design — joins stay explicit to prevent N+1).
…hrough) UserRepo, SessionRepo, PreferenceRepo, and WalkthroughRepo now use *gorm.DB. SaveUIPreferences batches its writes into one upsert + one bulk delete inside a single transaction; the original per-key INSERT/DELETE loop is gone. Other ports preserve method signatures, return types, and on-disk shape. Logger is now configured to ignore ErrRecordNotFound so Take()-on-missing calls no longer emit spurious warnings.
…ators) SyncBranches now issues a single batched upsert plus a single DELETE for stale rows — the previous per-branch INSERT loop is gone. Repository listings keep their owner-or-collaborator LEFT JOIN as one raw query so list endpoints stay O(1) statements. No GORM associations were added.
ReviewRepo, ReviewEventRepo, CommentRepo, and PendingCommentRepo now use *gorm.DB. PromotePendingComments runs as one txn with exactly three statements (SELECT scope, batched CreateInBatches, bulk DELETE IN) — replacing the per-row INSERT/DELETE loop the old implementation ran. ReviewCommentRow keeps deleted_at as a nullable RFC3339Nano string rather than gorm.DeletedAt, so the on-disk shape is unchanged and existing user databases keep working unchanged. Removes the *db.Queries (sqlc) wiring from Store; everything routes through *gorm.DB now. The sqlc package itself stays until Phase 5 cleanup.
Every repository runs through *gorm.DB now, so the stub sqlc package (internal/storage/db, internal/storage/sqlc) and its query.sql / sqlc.yaml become dead weight. Also removes nullString/fromNull/nullableString/ nullableInt/scanner from store.go — GORM handles nil pointers and sql.NullString translation natively, so these helpers had no remaining callers. go mod tidy cleans up the resulting indirect deps. N+1 audit (logger at info): SaveUIPreferences emits one upsert + one bulk delete + one re-fetch SELECT regardless of patch size; PromotePending, SyncBranches, and list endpoints stay O(1) statements.
…/updated_at Every table now follows one shape: id INTEGER PRIMARY KEY AUTOINCREMENT first, created_at/updated_at last. UUID/path/token/composite PKs are replaced with numeric ids; natural keys (path, token, (user_id,key), etc.) become UNIQUE. Foreign keys flip to INTEGER: review_events.review_id, review_comments.review_id, repository_users.repository_id, repository_branches.repository_id. Storage, service, and HTTP layers all carry int64 ids end-to-end; URL params parse via new pathInt64 helper. Two tables renamed for consistency with FK direction: ui_preferences -> user_preferences and branches -> repository_branches. Go types follow: UIPreferenceRow -> UserPreferenceRow, BranchRow -> RepositoryBranchRow, BranchRepo -> RepositoryBranchRepo (with companion file rename). Frontend types still carry string ids and need updating in a follow-up.
Mirrors the storage refactor: all review/comment/pending-comment ids and the new repository id are number, not string. UIPreferences becomes UserPreferences (renamed to match the user_preferences table). - contracts: ReviewSession/ReviewEvent/ReviewComment/PendingComment.id, reviewId, commentId are number; Repository gains id/createdAt/updatedAt; UIPreferences renamed to UserPreferences. - bridge clients: drop URL-encoding now that ids are numeric. - ui: electron preload, ipc handlers, services, browser-fallback, tests switch every id from string to number. - regenerate contracts/bridge dist after src changes.
Introduces internal/db with Conn/Now/SetNow/SetForTest. Store.Open now installs the global on boot; Store.SetNow forwards to db.SetNow so a later cut-over of repo internals stays transparent. Repos still carry their own *gorm.DB and *clock fields — this is the transitional phase before the leaf/mid-tier/composite ports.
…fs, walkthrough) UserRepo, SessionRepo, PreferenceRepo and WalkthroughRepo drop their db/clk fields and now read the gorm connection and clock from internal/db. Constructors take no arguments. Composite and mid-tier repos still hold their fields — converted in follow-up phases.
… collaborators) RepositoryBranchRepo, RepoRepo and CollaboratorRepo drop their db/clk fields and read both from internal/db. Constructors now take no arguments.
ReviewEventRepo, ReviewRepo, CommentRepo and PendingCommentRepo drop their db/clk fields and read both from internal/db. ReviewRepo and CommentRepo keep their *ReviewEventRepo dependency. PendingComment's gorm.io/gorm import is retained for the tx callback type.
With every repo now reading the gorm connection and clock from internal/db, the per-Store *clock, *gorm.DB and the unused Store.DB() accessor are dead weight. SetNow becomes a one-line forward to db.SetNow.
…oins Adds gorm foreignKey/references tags to row structs so the Go layer mirrors the FK constraints already enforced in the baseline migration. Tags are documentation + scaffolding for explicit .Joins() chains; Preload remains banned so list endpoints stay O(1) queries. Rewrites the two repos that used db.Conn().Raw(...) for joins (repositories.go ListRepositoriesForUser/GetRepository, repository_users.go List + post-Grant lookup) to use GORM's typed Table/Select/Joins/Where/Order chain. The dynamic CASE WHEN role expression stays as a Select() fragment.
Project is moving under the oullin GitHub organization. Updates the module declarations in both go.mod files, every Go import, and the GitHub repo URLs referenced by setup.sh, packages/ui/package.json, README.md, and storage/docs/README.md. The Electron appId (io.gocanto.git-diff) is intentionally left in place to avoid breaking already-installed app data.
Lay the foundation for the planned packages/api test sweep: - storage/testhelpers_test.go: share newTestStore plus seed helpers (users, repos, branches, reviews, comments, pending comments, events, collaborators, walkthroughs) so every storage and service test can bootstrap state without duplicating boilerplate. The local helper in users_test.go is removed in favour of the shared one. - app/httpx/testhelpers_test.go + stubprovider_test.go: wire a Server with a real *storage.Store, all eight services, a stub usercfg reader/broker, and an ai.Registry seeded with a deterministic provider. Mirrors the production wiring in bootstrap.go so handler tests exercise the same construction path. Exposes testServerOptions with an Authenticated flag for routes behind requireAuth. - app/httpx/gitfixture_test.go: ephemeral on-disk git repos for the repository_* handlers and review-package tests; supports WriteFile/Commit/Branch/Checkout/HeadSHA. - service/testhelpers_test.go: matching newTestStore plus user/repo seeds for the upcoming service-layer suite. - app/httpx/server_test.go: rewrite the healthz test on the new fixture and add three more covering the requireAuth allow/deny matrix. All packages still green via `go test ./...`.
Add tests for the previously untested storage repos plus the db connection wrapper: - db/db_test.go: Conn panics before Init, SetForTest/Now/SetNow semantics. - storage/sessions_test.go: raw-vs-hashed token semantics, blank/ unknown token rejection, last_used_at advancement, idempotent delete, hashToken determinism. - storage/repositories_test.go: name default from base dir, upsert conflict, ownership-required errors, list union of owner + collaborator with correct Role column, GetByPath skipping Role, RemoveRepository ownership enforcement. - storage/repository_branches_test.go: SyncBranches insert + reconcile + skip-empty, locked branch survives reconcile, lock not-found, unlock clears metadata, IsBranchLocked, DeleteBranchRow. - storage/repository_users_test.go: collaborator grant/list ordering, upsert on conflict, invalid role + owner-as-collaborator + non- owner + missing-repo error paths, revoke, empty list returns non- nil slice. - storage/reviews_test.go: defaulted title + context kind, implicit review_started event, ListReviews ordering + limit defaulting, GetReviewByID missing, ReviewDetail aggregation across events and comments. - storage/comments_test.go: AuthorLabel default, comment_added / comment_edited events, soft-delete semantics, list ordering. - storage/review_events_test.go: metadata default, supplied metadata preserved, ascending order. - storage/pending_comments_test.go: ContextKind default, ownership on update/delete, scope-filtered list, PromotePendingComments three-statement transaction, missing-review error. - storage/preferences_test.go: mixed upsert+clear in one call, empty-patch no-op, key trimming and blank-key drop, empty initial state. - storage/walkthrough_test.go: groups JSON round-trip, missing row returns found=false, upsert overwrites. All `go test ./...` packages green.
Add tests for every file in internal/service: - auth_test.go: AuthConfig defaulting; Setup short-password/already- set/happy-path; Login no-password/wrong-password/remember=false vs true; Resume propagating ErrSessionNotFound; Logout blank no-op; Wipe refusing non-active users and falling back when target blank. - review_test.go: Create auth gate + ID assignment, List default limit + auth gate, Detail aggregating events + comments, UpdateComment + DeleteComment soft-delete visibility. - branches_test.go: List for registered vs unregistered repo, Lock/Unlock auth gate, registered-only resolution path, locked branch refuses Delete via ErrBranchAlreadyLocked. Includes a real on-disk git repo fixture (gitfixture_test.go) because the service shells out to git via internal/review. - collaborators_test.go: auth gates on List/Grant/Revoke and a full grant -> list -> revoke round-trip. - pending_comments_test.go: auth gates on every method, required- field validation, defaultRepoRoot fallback, Promote review-id validation, full lifecycle CRUD. - preferences_test.go: auth gate + round-trip Save/Get. - repository_test.go: auth gates, blank-path rejection on Remove, upsert/list/remove lifecycle, ErrRepositoryNotOwned propagated when a non-owner tries to remove. - walkthrough_test.go: ErrProviderUnavailable on empty registry, cache hit when stored fingerprint matches the one derived from (state, providerID), Refresh=true bypasses cache. All tests use the real *storage.Store via the helper added in Phase 0. `go test ./...` green.
Per-handler tests under packages/api/internal/app/httpx that exercise the public /v1/* surface end-to-end through httptest.Server + BuildMux, with a real *storage.Store and the full service wiring introduced in Phase 0. - auth_handlers_test.go: state shape for fresh user, short-password 400, conflict on second setup, wrong-password 401, login pre-setup 409, resume unknown token 401, wipe-other-user 403, full setup -> login -> logout round-trip, wipe self -> 204. - preferences_handlers_test.go: 401 without auth, empty-map shape for new user, bad-JSON 400, round-trip Save -> Get. - repositories_handlers_test.go: 401 unauth, upsert + list round- trip, bad JSON / missing path 400 on POST + DELETE, list collaborators path-required 400, add+list collaborator, remove collaborator integer parsing + missing params 400, 204 on revoke. - reviews_handlers_test.go: auth gate, happy create -> 201, bad JSON 400, bad limit 400, list shape, detail 400/404 paths, full lifecycle (event + comment + patch + delete), patch bad id 400. - pending_comments_handlers_test.go: auth gate, missing required fields 400, bad JSON 400, full lifecycle, promote review-id required 400, patch bad id 400. - system_handlers_test.go: GET /v1/system/stats decodes to SystemStats (numbers are platform-dependent so only the shape is asserted). - userconfig_handlers_test.go: GET returns Defaults shape, returns 503 when reader nil; SSE stream emits an initial `event: config` frame and returns 503 when broker nil. - walkthrough_handlers_test.go: bad JSON 400, commit kind without sha 400, non-git path 400 (provider-unavailable path is covered by the service-layer test). - repository_handlers_test.go (git-backed): state for a real repo 200, non-git 400, open bad JSON 400, open for real repo 200, commit-without-sha 400, commit for real sha 200, log returns commits + bad limit 400, file path-required 400 + happy path, file-range missing-params + bad-integer matrix, branches list includes main, create + delete branch round-trip, checkout bad JSON + happy path, lock on unregistered repo maps through branch error handler. All packages green via `go test ./...`.
Fill the biggest remaining gaps in the side packages. internal/walks: - response_parser_test.go: plain JSON, ```json fences, bare ``` fences, decode-error path, full groups/files decode. - schema_validator_test.go: unknown-path filter, empty-group removal, error when nothing remains, action/impact normalisation. - budget_enforcer_test.go: per-file truncation with marker, total budget cutoff + OmittedAfter flag, zero-budget defaulting, binary-section marker in concatenatePatches. - prompt_builder_test.go: instructions + repo/mode/commit/file lines present, commit line omitted when SHA blank, omitted-tail marker emitted when budget caps remaining files. internal/review: - patch_test.go: countPatchLines additions vs deletions skipping +++/--- headers, isBinaryPatch markers, fingerprint determinism and content-sensitivity, pathSectionID format. - difftree_parser_test.go: A/D/M simple cases, R/C consume two extra fields, unknown status falls back to modified, skips empty/truncated input. - runner_test.go: SetGit swaps and restores, gitOutput propagates fake-executor errors, SetGh + ghOutput round-trip via fake. - gitfixture_test.go: shared on-disk repo fixture with macOS symlink resolution (renamed helper to resolveRepoTopLevel to avoid colliding with the unexported resolveTopLevel in root_cache.go). - commit_log_test.go: returns commits with SHAs + subject, honours limit, clamps negative + oversize limits to default. - branch_test.go: ListBranches sees main, CreateBranch switches the working copy, validation rejects empty/leading-dash/space/ '..' names, DeleteBranch refuses currently-checked-out, CheckoutBranch succeeds + detects WorkingTreeDirtyError. - state_reader_test.go: clean repo yields zero changed files, untracked + staged are surfaced via parseStatus, non-git path errors. `go test ./...` green across the whole packages/api module.
Wire the new test suites into a CI-friendly entry point. - `make test-api` runs `go test -race ./...` inside packages/api with the project's pinned GOCACHE/GOPATH so the cache lives under storage/.cache (already gitignored). - `make test-api-cover` does the same plus -covermode=atomic and a coverage profile, then fails the build when the total line coverage drops below API_COVERAGE_FLOOR (60.0%). Today the suite reports ~65.5% total, so the floor leaves a small headroom for noise and acts as a regression guard. The plan's aspirational 80% target is reached by raising this floor as new tests land. - Add coverage.out to the repo .gitignore so cover runs don't dirty the tree. Both targets pass locally: make test-api — 14 packages, all green make test-api-cover — total 65.5%, above the 60% floor
# Conflicts: # Makefile
Backend prerequisites for porting two codiff v0.7.0 features to this fork: inline image comparison (needs raw bytes at HEAD / index / worktree) and "Open Config in Editor" (needs the absolute config-file path exposed to the renderer). - GET /v1/repository/file/raw streams blob bytes, picking Content-Type from the extension. Reuses the existing 2 MB cap and path-traversal guards via a new review.ReadRepositoryBlob helper with sentinel errors that map to 404 / 413. - /v1/userconfig (GET + SSE) now includes the config-file path so the renderer can call shell.openPath without re-deriving it. - Adds a stash-regression test asserting StatusParser yields zero entries for empty porcelain input — locks in that future enrichment of status output cannot reintroduce the codiff stash bug.
Closes the user-visible half of the v0.7.0 port; backend prerequisites landed in e7518be. Image diff: - Renders changed image files (png/jpg/gif/webp/svg/avif/ico/bmp) as HEAD vs worktree (or parent vs commit in commit mode), with each side showing the preview, dimensions, and byte size. Added/deleted files show only the present side. - Routed from DiffList.vue based on extension via a new isImagePath helper in contracts; FileContentViewer's standalone preview now also renders images instead of "preview unavailable". - Fetches blobs through a new diffApp.readRepositoryFileBytes IPC that delegates to bridge.repository.readFileBytes; bridge gains a requestBytes transport path for binary responses. Open Config in Editor: - New "Open Config in Editor…" item under the macOS app menu and a diffApp.openUserConfigFile renderer affordance. - The config path is resolved server-side and passed to shell.openPath inside the main process — the renderer cannot supply an arbitrary path, so a compromised renderer can't coerce the main process into opening unrelated files.
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.
No description provided.