feat(stores): add sqlc typed SQL to whisper and codedb stores#346
Merged
rsnodgrass merged 4 commits intomainfrom Mar 29, 2026
Merged
feat(stores): add sqlc typed SQL to whisper and codedb stores#346rsnodgrass merged 4 commits intomainfrom
rsnodgrass merged 4 commits intomainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughReplaces ad-hoc SQL with sqlc-generated, context-aware query methods across CodeDB and Whisper; threads context through indexing and store flows; converts nullable handling to database/sql null types; adds sqlc schemas/models/queries; and exposes typed Queries via Store wrappers. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Indexer
end
rect rgba(200,255,200,0.5)
participant Queries
end
rect rgba(255,200,200,0.5)
participant DB
end
rect rgba(255,255,200,0.5)
participant Bleve
end
Indexer->>Queries: ListFileMtimes(ctx) / GetPRIDByNumber(ctx)
Queries->>DB: QueryContext / QueryRowContext
DB-->>Queries: rows / row
Queries-->>Indexer: typed results (IDs, rows)
Indexer->>Queries: InsertPullRequest/InsertPRComment/UpsertFileMtime(ctx, params)
Queries->>DB: ExecContext inserts/deletes/upserts
Indexer->>Bleve: Index file content using resolved DB IDs
Bleve-->>Indexer: index result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/codedb/sqlc/queries/repos.sql (1)
8-15: Make list queries deterministic and harden ref join integrity.Line 9, Line 12, and Line 15 return unordered result sets; this can cause nondeterministic behavior upstream. Also on Line 15, adding
c.repo_id = r.repo_idis a good defensive guard for data integrity.♻️ Proposed SQL refinement
-- name: ListRepoPaths :many -SELECT path FROM repos; +SELECT path FROM repos +ORDER BY path; -- name: ListCommitHashesByRepo :many -SELECT hash FROM commits WHERE repo_id = ?; +SELECT hash FROM commits +WHERE repo_id = ? +ORDER BY hash; -- name: ListRefsByRepo :many -SELECT r.name, c.hash FROM refs r JOIN commits c ON r.commit_id = c.id WHERE r.repo_id = ?; +SELECT r.name, c.hash +FROM refs r +JOIN commits c + ON r.commit_id = c.id + AND c.repo_id = r.repo_id +WHERE r.repo_id = ? +ORDER BY r.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/sqlc/queries/repos.sql` around lines 8 - 15, The three SQL queries (ListRepoPaths, ListCommitHashesByRepo, ListRefsByRepo) return unordered rows — make them deterministic by adding explicit ORDER BY clauses (e.g., ORDER BY path for ListRepoPaths, ORDER BY hash for ListCommitHashesByRepo, and ORDER BY r.name[, c.hash] for ListRefsByRepo) and harden the ref join in ListRefsByRepo by adding the defensive join predicate c.repo_id = r.repo_id to the WHERE/JOIN condition so refs cannot join to a commit from a different repo.internal/whisper/store/store.go (1)
304-315: Minor: Inconsistent UTC normalization forRelayedAttimestamp.Line 309 uses
time.Now().Format(...)without.UTC(), while other timestamp formatting in this file (lines 218, 277, 433-434) consistently applies.UTC()before formatting. This won't cause bugs since RFC3339 includes timezone info, but consistency aids readability.🔧 Optional consistency fix
- RelayedAt: time.Now().Format(time.RFC3339Nano), + RelayedAt: time.Now().UTC().Format(time.RFC3339Nano),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/whisper/store/store.go` around lines 304 - 315, The RelayedAt timestamp in MarkRelayed uses time.Now().Format(...) while other timestamps use UTC; update the call in Store.MarkRelayed so RelayedAt is generated with time.Now().UTC().Format(time.RFC3339Nano) when building the whisperdb.MarkRelayedParams passed to s.queries.MarkRelayed to match the file's UTC normalization convention.
🤖 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/codedb/index/indexer.go`:
- Around line 884-890: The code in indexer.go treats a failed diff-ID lookup
from q.GetDiffIDByCommitPath (used to populate diffDBID) as a success by
returning nil, which causes the commit to proceed without creating the Bleve
document; change the error handling so the lookup failure is propagated instead
of swallowed: when q.GetDiffIDByCommitPath returns an error, return that error
(or wrap it with context) from the surrounding function (the function containing
diffDBID), so callers know the indexing step failed and can retry or abort
rather than silently skipping Bleve indexing.
In `@internal/codedb/sqlc/schema.sql`:
- Around line 11-18: The commits table currently mixes repo-scoped repo_id with
a globally UNIQUE hash which causes collisions for forks; update the schema and
runtime DDL to use a consistent model—preferably make commits repo-scoped by
replacing the UNIQUE(hash) constraint with UNIQUE(repo_id, hash) (or
alternatively remove repo_id if you want global deduplication) and then update
code that queries commits (e.g., loadKnownCommits in
internal/codedb/index/indexer.go) to perform repo-aware lookups using repo_id
plus hash; ensure the chosen change is reflected in both the CREATE TABLE DDL
and any runtime migrations/DDL applied by the app.
---
Nitpick comments:
In `@internal/codedb/sqlc/queries/repos.sql`:
- Around line 8-15: The three SQL queries (ListRepoPaths,
ListCommitHashesByRepo, ListRefsByRepo) return unordered rows — make them
deterministic by adding explicit ORDER BY clauses (e.g., ORDER BY path for
ListRepoPaths, ORDER BY hash for ListCommitHashesByRepo, and ORDER BY r.name[,
c.hash] for ListRefsByRepo) and harden the ref join in ListRefsByRepo by adding
the defensive join predicate c.repo_id = r.repo_id to the WHERE/JOIN condition
so refs cannot join to a commit from a different repo.
In `@internal/whisper/store/store.go`:
- Around line 304-315: The RelayedAt timestamp in MarkRelayed uses
time.Now().Format(...) while other timestamps use UTC; update the call in
Store.MarkRelayed so RelayedAt is generated with
time.Now().UTC().Format(time.RFC3339Nano) when building the
whisperdb.MarkRelayedParams passed to s.queries.MarkRelayed to match the file's
UTC normalization convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97a55144-8567-4d20-9464-975ab4f44b4e
📒 Files selected for processing (31)
internal/codedb/index/github.gointernal/codedb/index/helpers_test.gointernal/codedb/index/indexer.gointernal/codedb/index/indexer_coverage_test.gointernal/codedb/sqlc/blobs.sql.gointernal/codedb/sqlc/commits.sql.gointernal/codedb/sqlc/db.gointernal/codedb/sqlc/diffs.sql.gointernal/codedb/sqlc/file_revs.sql.gointernal/codedb/sqlc/github.sql.gointernal/codedb/sqlc/models.gointernal/codedb/sqlc/queries/blobs.sqlinternal/codedb/sqlc/queries/commits.sqlinternal/codedb/sqlc/queries/diffs.sqlinternal/codedb/sqlc/queries/file_revs.sqlinternal/codedb/sqlc/queries/github.sqlinternal/codedb/sqlc/queries/refs.sqlinternal/codedb/sqlc/queries/repos.sqlinternal/codedb/sqlc/refs.sql.gointernal/codedb/sqlc/repos.sql.gointernal/codedb/sqlc/schema.sqlinternal/codedb/store/store.gointernal/whisper/store/schema.gointernal/whisper/store/sqlc/db.gointernal/whisper/store/sqlc/models.gointernal/whisper/store/sqlc/queries.sqlinternal/whisper/store/sqlc/queries.sql.gointernal/whisper/store/sqlc/schema.sqlinternal/whisper/store/store.gointernal/whisper/store/store_coverage_test.gosqlc.yaml
rsnodgrass
force-pushed
the
ryan/sqlc-typed-sql
branch
from
aa8640d to
7d722e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/codedb/index/indexer.go (1)
1060-1066:⚠️ Potential issue | 🟠 MajorDiff-ID lookup failures should not be treated as success.
At Line 1065, returning
nilsuppresses a real indexing failure and can leave SQL and Bleve out of sync for that diff path.Suggested fix
diffDBID, err := q.GetDiffIDByCommitPath(ctx, codedbsqlc.GetDiffIDByCommitPathParams{ CommitID: commitDBID, Path: path, }) if err != nil { - return nil // non-fatal: skip Bleve indexing for this diff + return fmt.Errorf("get diff id: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/indexer.go` around lines 1060 - 1066, The current code treats failures from q.GetDiffIDByCommitPath (called with codedbsqlc.GetDiffIDByCommitPathParams using commitDBID and path) as a success by returning nil, which hides real indexing errors; change the handling to propagate the error instead of returning nil (e.g., return fmt.Errorf or the original err) so the caller knows Bleve indexing for that diff failed, or explicitly log the error and return it from the enclosing function; ensure the returned error is used by the caller that invokes this block so SQL and Bleve remain in sync.
🧹 Nitpick comments (2)
internal/codedb/index/helpers_test.go (2)
71-83: Consider table-driven pattern for consistency.The test is functionally correct, but unlike
TestTimeToNullInt64above, it doesn't use the table-driven approach. For consistency and easier extensibility (e.g., adding whitespace-only string tests later), consider refactoring.♻️ Optional: table-driven refactor
func TestToNullString(t *testing.T) { t.Parallel() - got := toNullString("") - if got.Valid { - t.Error("empty string should be invalid") - } - - got = toNullString("hello") - if !got.Valid || got.String != "hello" { - t.Errorf("expected valid 'hello', got valid=%v string=%q", got.Valid, got.String) + tests := []struct { + name string + input string + wantValid bool + wantString string + }{ + {"empty string is invalid", "", false, ""}, + {"non-empty string is valid", "hello", true, "hello"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := toNullString(tt.input) + if got.Valid != tt.wantValid { + t.Errorf("valid = %v, want %v", got.Valid, tt.wantValid) + } + if got.Valid && got.String != tt.wantString { + t.Errorf("string = %q, want %q", got.String, tt.wantString) + } + }) } }As per coding guidelines: "Use table-driven tests for test cases; cover both success and error paths"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/helpers_test.go` around lines 71 - 83, Refactor TestToNullString to use a table-driven pattern like TestTimeToNullInt64: create a slice of test cases (e.g., name, input string, wantValid bool, wantString string), iterate with t.Run for each case, assert got := toNullString(input) against wantValid and wantString, and include cases for empty string and a normal string (and optionally whitespace-only) to keep consistency and make future additions easier.
85-98: Same suggestion: consider table-driven pattern.Similar to
TestToNullString, this test could be refactored to use table-driven tests for consistency with the rest of the file.♻️ Optional: table-driven refactor
func TestPtrIntToNullInt64(t *testing.T) { t.Parallel() - got := ptrIntToNullInt64(nil) - if got.Valid { - t.Error("nil should be invalid") - } - - n := 42 - got = ptrIntToNullInt64(&n) - if !got.Valid || got.Int64 != 42 { - t.Errorf("expected valid 42, got valid=%v int64=%d", got.Valid, got.Int64) + n := 42 + tests := []struct { + name string + input *int + wantValid bool + wantInt64 int64 + }{ + {"nil is invalid", nil, false, 0}, + {"non-nil is valid", &n, true, 42}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ptrIntToNullInt64(tt.input) + if got.Valid != tt.wantValid { + t.Errorf("valid = %v, want %v", got.Valid, tt.wantValid) + } + if got.Valid && got.Int64 != tt.wantInt64 { + t.Errorf("int64 = %d, want %d", got.Int64, tt.wantInt64) + } + }) } }As per coding guidelines: "Use table-driven tests for test cases"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/helpers_test.go` around lines 85 - 98, Refactor TestPtrIntToNullInt64 into a table-driven test to match the file's style: create a slice of test cases with name, input (*int) and expected validity/Int64, iterate with t.Run for each case, and replace the two existing assertions by table assertions; update references to ptrIntToNullInt64 and keep t.Parallel() inside the top-level test or per-subtest as appropriate.
🤖 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/codedb/index/indexer.go`:
- Around line 589-595: The code creates a fresh context via context.Background()
which drops caller cancellation and deadlines; replace that local ctx with the
incoming caller context (i.e., stop shadowing/creating ctx and use the
function's ctx parameter) so the sqlc calls use the propagated context for
UpsertRepo, GetRepoIDByName and the other queries referenced (lines around the
UpsertRepo/GetRepoIDByName calls and the similar sites at 603, 616, 1427).
Ensure you remove the context.Background() assignment, pass the existing ctx to
s.Queries().UpsertRepo/GetRepoIDByName (and the other query calls) so
cancellations/timeouts propagate correctly.
- Around line 923-927: The code in the parent linking loop treats any error from
q.GetCommitIDByHash(ctx, parentHex) as “parent missing” and continues, which
hides real DB errors; update the error handling in the block that sets
parentDBID to use errors.Is(err, sql.ErrNoRows) (or the package-specific
ErrNoRows) to detect the “not found” case and continue only then, but for all
other errors wrap the error with context (including parentHex and the calling
function or commit ID) and return or propagate it instead of continuing;
reference the call to q.GetCommitIDByHash, the parentHex variable, and the
parentDBID assignment when making the change.
---
Duplicate comments:
In `@internal/codedb/index/indexer.go`:
- Around line 1060-1066: The current code treats failures from
q.GetDiffIDByCommitPath (called with codedbsqlc.GetDiffIDByCommitPathParams
using commitDBID and path) as a success by returning nil, which hides real
indexing errors; change the handling to propagate the error instead of returning
nil (e.g., return fmt.Errorf or the original err) so the caller knows Bleve
indexing for that diff failed, or explicitly log the error and return it from
the enclosing function; ensure the returned error is used by the caller that
invokes this block so SQL and Bleve remain in sync.
---
Nitpick comments:
In `@internal/codedb/index/helpers_test.go`:
- Around line 71-83: Refactor TestToNullString to use a table-driven pattern
like TestTimeToNullInt64: create a slice of test cases (e.g., name, input
string, wantValid bool, wantString string), iterate with t.Run for each case,
assert got := toNullString(input) against wantValid and wantString, and include
cases for empty string and a normal string (and optionally whitespace-only) to
keep consistency and make future additions easier.
- Around line 85-98: Refactor TestPtrIntToNullInt64 into a table-driven test to
match the file's style: create a slice of test cases with name, input (*int) and
expected validity/Int64, iterate with t.Run for each case, and replace the two
existing assertions by table assertions; update references to ptrIntToNullInt64
and keep t.Parallel() inside the top-level test or per-subtest as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 307a9ea7-f855-4a00-b8d5-77058ccdd8aa
📒 Files selected for processing (17)
internal/codedb/index/github.gointernal/codedb/index/helpers_test.gointernal/codedb/index/indexer.gointernal/codedb/index/indexer_coverage_test.gointernal/codedb/sqlc/blobs.sql.gointernal/codedb/sqlc/commits.sql.gointernal/codedb/sqlc/db.gointernal/codedb/sqlc/diffs.sql.gointernal/codedb/sqlc/file_revs.sql.gointernal/codedb/sqlc/github.sql.gointernal/codedb/sqlc/models.gointernal/codedb/sqlc/queries/blobs.sqlinternal/codedb/sqlc/queries/commits.sqlinternal/codedb/sqlc/queries/diffs.sqlinternal/codedb/sqlc/queries/file_revs.sqlinternal/codedb/sqlc/queries/github.sqlinternal/codedb/sqlc/queries/refs.sql
✅ Files skipped from review due to trivial changes (10)
- internal/codedb/sqlc/queries/diffs.sql
- internal/codedb/sqlc/queries/blobs.sql
- internal/codedb/sqlc/db.go
- internal/codedb/sqlc/diffs.sql.go
- internal/codedb/sqlc/blobs.sql.go
- internal/codedb/sqlc/commits.sql.go
- internal/codedb/sqlc/file_revs.sql.go
- internal/codedb/sqlc/queries/commits.sql
- internal/codedb/sqlc/queries/github.sql
- internal/codedb/sqlc/github.sql.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/codedb/sqlc/queries/refs.sql
- internal/codedb/index/indexer_coverage_test.go
- internal/codedb/sqlc/queries/file_revs.sql
- internal/codedb/index/github.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/codedb/index/indexer.go (3)
589-595:⚠️ Potential issue | 🟠 MajorPropagate the caller
ctxthrough these sqlc/store helpers.These paths are all reached from cancellation-aware indexing/parsing flows, but each helper swaps that for
context.Background(). That drops deadlines/cancellation and can keep DB work running after the caller has already aborted. Please thread the incomingctxintoupsertRepo,loadKnownCommits,loadExistingRefs,insertParentLinks,insertDiff,buildTipFileRevs,ensureBlob, andopenReposFromDB.Also applies to: 603-603, 616-616, 915-916, 1043-1044, 1081-1082, 1198-1200, 1427-1427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/indexer.go` around lines 589 - 595, Current code creates a new context with context.Background() before calling SQL helpers, dropping deadlines/cancellation; instead thread the incoming caller ctx into these DB helper calls. Replace usages like ctx := context.Background() and subsequent q.UpsertRepo/GetRepoIDByName with the passed-in ctx, and update helper signatures and callsites for upsertRepo, loadKnownCommits, loadExistingRefs, insertParentLinks, insertDiff, buildTipFileRevs, ensureBlob, and openReposFromDB so they accept a context parameter and use it when calling s.Queries() and sqlc methods; ensure all call sites propagate the original ctx through the call chain to preserve cancellation/deadlines.
923-926:⚠️ Potential issue | 🟠 MajorDon't treat every parent lookup failure as “missing”.
Only
sql.ErrNoRowsshould skip the link. Any other error here silently drops a commit-parent edge and leaves the commit graph incomplete.
As per coding guidelines: "Useerrors.Is()anderrors.As()for error checking and type assertion; wrap errors with context".💡 Suggested fix
+import "errors" ... parentDBID, err = q.GetCommitIDByHash(ctx, parentHex) if err != nil { - continue // parent not yet in DB; skip link + if errors.Is(err, sql.ErrNoRows) { + continue // parent not yet in DB; skip link + } + return fmt.Errorf("get parent commit id %s: %w", parentHex, err) } + st.commitIDCache[parentHex] = parentDBID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/indexer.go` around lines 923 - 926, The code currently treats any error from q.GetCommitIDByHash(ctx, parentHex) as "missing" and silently continues; change this in the parent link block so only sql.ErrNoRows results in skipping the link. Specifically, call q.GetCommitIDByHash and if err != nil use errors.Is(err, sql.ErrNoRows) to continue, otherwise wrap/return or log the error with context (use fmt.Errorf or errors.Wrap-like pattern) so unexpected DB errors are not dropped; update the handling around parentDBID, err and ensure the error path uses the same context and propagation style as other DB errors in this file.
1060-1065:⚠️ Potential issue | 🟠 MajorPropagate diff-ID lookup failures.
Returning
nilhere commits the SQL diff row but skips the Bleve document, so later runs treat the commit as indexed while diff search stays stale for this path.💡 Suggested fix
diffDBID, err := q.GetDiffIDByCommitPath(ctx, codedbsqlc.GetDiffIDByCommitPathParams{ CommitID: commitDBID, Path: path, }) if err != nil { - return nil // non-fatal: skip Bleve indexing for this diff + return fmt.Errorf("get diff id for commit %d path %q: %w", commitDBID, path, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codedb/index/indexer.go` around lines 1060 - 1065, The call to q.GetDiffIDByCommitPath (using commitDBID and path) currently swallows errors by returning nil, causing the SQL diff row to be committed while Bleve indexing is skipped; change this to propagate the error instead of returning nil (e.g., return the error or a wrapped error) so callers know Bleve indexing failed; update the surrounding function (in indexer.go) to return the error from GetDiffIDByCommitPath (or wrap it with context) so upstream can handle/retry instead of marking the commit as fully indexed.
🤖 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/codedb/index/indexer.go`:
- Around line 1085-1087: The code treats any error from GetCommitIDByHash and
GetBlobIDByHash as "not found" which hides real DB failures; update callers
(e.g., buildTipFileRevs) and the blob insertion path to check errors.Is(err,
sql.ErrNoRows) for lookup-misses and only treat that case as a not-found/skip,
but for any other error return a wrapped error (use fmt.Errorf or errors.Wrap)
so real DB failures surface; specifically modify the buildTipFileRevs call
handling of GetCommitIDByHash to return the error when it's not sql.ErrNoRows,
and change the GetBlobIDByHash/InsertBlob flow to abort and return the wrapped
DB error instead of silently falling through when the error is not
sql.ErrNoRows.
---
Duplicate comments:
In `@internal/codedb/index/indexer.go`:
- Around line 589-595: Current code creates a new context with
context.Background() before calling SQL helpers, dropping
deadlines/cancellation; instead thread the incoming caller ctx into these DB
helper calls. Replace usages like ctx := context.Background() and subsequent
q.UpsertRepo/GetRepoIDByName with the passed-in ctx, and update helper
signatures and callsites for upsertRepo, loadKnownCommits, loadExistingRefs,
insertParentLinks, insertDiff, buildTipFileRevs, ensureBlob, and openReposFromDB
so they accept a context parameter and use it when calling s.Queries() and sqlc
methods; ensure all call sites propagate the original ctx through the call chain
to preserve cancellation/deadlines.
- Around line 923-926: The code currently treats any error from
q.GetCommitIDByHash(ctx, parentHex) as "missing" and silently continues; change
this in the parent link block so only sql.ErrNoRows results in skipping the
link. Specifically, call q.GetCommitIDByHash and if err != nil use
errors.Is(err, sql.ErrNoRows) to continue, otherwise wrap/return or log the
error with context (use fmt.Errorf or errors.Wrap-like pattern) so unexpected DB
errors are not dropped; update the handling around parentDBID, err and ensure
the error path uses the same context and propagation style as other DB errors in
this file.
- Around line 1060-1065: The call to q.GetDiffIDByCommitPath (using commitDBID
and path) currently swallows errors by returning nil, causing the SQL diff row
to be committed while Bleve indexing is skipped; change this to propagate the
error instead of returning nil (e.g., return the error or a wrapped error) so
callers know Bleve indexing failed; update the surrounding function (in
indexer.go) to return the error from GetDiffIDByCommitPath (or wrap it with
context) so upstream can handle/retry instead of marking the commit as fully
indexed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20d01fea-6a84-4fca-affe-0ed5b80fe0d2
📒 Files selected for processing (3)
internal/codedb/index/indexer.gointernal/codedb/sqlc/queries/symbols.sqlinternal/codedb/sqlc/symbols.sql.go
✅ Files skipped from review due to trivial changes (1)
- internal/codedb/sqlc/queries/symbols.sql
Co-Authored-By: SageOx <ox@sageox.ai>
Co-Authored-By: SageOx <ox@sageox.ai>
…king Co-Authored-By: SageOx <ox@sageox.ai>
Co-Authored-By: SageOx <ox@sageox.ai>
rsnodgrass
force-pushed
the
ryan/sqlc-typed-sql
branch
from
8924458 to
c0caa2e
Compare
4 tasks
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
Adds sqlc strongly-typed SQL code generation to both SQLite stores, replacing hand-written
database/sqlquery/scan code with compile-time-checked generated Go code.//go:embedso one.sqlfile serves both sqlc generation and runtime DDL. Dynamic queries (GetWhispers,GetWhispersPage) kept as hand-written SQL.codedbsqlc.New(tx)for typed inserts within existingindexState.tx. Dynamic search SQL andParseSymbolsIN-clause queries kept hand-written.*string/*int64pointer patterns withsql.NullString/sql.NullInt64throughout, matching sqlc conventions.graph LR subgraph "sqlc pipeline" S[schema.sql] --> G[sqlc generate] Q[queries.sql] --> G G --> C[db.go + models.go + queries.sql.go] end subgraph "runtime" S -->|go:embed| R[CreateSchema] C --> W[Store.queries] endTest plan
go build ./internal/codedb/... ./internal/whisper/...— cleango test ./internal/whisper/store/...— all passgo test ./internal/codedb/...— all pass (9 sub-packages)make lint— 0 issuesCo-authored-by: SageOx ox@sageox.ai
Summary by CodeRabbit
Chores
Tests