fix: use normal endpoint for incremental updates, preserve domains#62
Conversation
The daemon's incrementalUpdate was calling AnalyzeIncremental which sent a changedFiles multipart field the API doesn't understand. This caused the API to return a full graph with new UUIDs for every node, resulting in 99% graph churn on every single-file update. Additionally, mergeGraph unconditionally replaced domains with the incremental response. Since the API reclassifies domains from scratch based on whatever's in the zip, sending 1 file produced garbage domains that overwrote the correct domain model. Fix: - Use AnalyzeSidecars (normal endpoint) instead of AnalyzeIncremental - Never replace domains on incremental merge — preserve from last full generate - Delete dead AnalyzeIncremental and postIncrementalZip code Verified: incremental update for 1 file now shows 0% node churn (was 99%) and domains are preserved identically. Closes supermodeltools#50
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoved the client's incremental-upload polling path ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/files/daemon_test.go (1)
286-289: Minor: Variable shadowing -dused for both daemon and loop variable.On line 287, the loop variable
dshadows the outerd(your test daemon). Doesn't break anything right now since you don't use the daemon after this point, but could cause confusion if someone adds code later.Quick fix would be to rename the loop variable to something like
dom:✨ Suggested tweak
names := make(map[string]bool) - for _, d := range result.Domains { - names[d.Name] = true + for _, dom := range result.Domains { + names[dom.Name] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/files/daemon_test.go` around lines 286 - 289, The loop uses the variable name `d` which shadows the outer test daemon variable `d`; rename the loop variable (for example to `dom`) in the loop that iterates over `result.Domains` where you populate the `names` map so the outer `d` is not shadowed and future code remains clear (update occurrences of the loop variable within that loop accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/files/daemon_test.go`:
- Around line 286-289: The loop uses the variable name `d` which shadows the
outer test daemon variable `d`; rename the loop variable (for example to `dom`)
in the loop that iterates over `result.Domains` where you populate the `names`
map so the outer `d` is not shadowed and future code remains clear (update
occurrences of the loop variable within that loop accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad592569-cb00-4ddd-9ac9-3e3ed596f1ce
📒 Files selected for processing (4)
internal/api/client.gointernal/files/daemon.gointernal/files/daemon_export_test.gointernal/files/daemon_test.go
💤 Files with no reviewable changes (1)
- internal/api/client.go
Rename `d` → `dom` in domain iteration to avoid shadowing the test daemon variable.
Integration test dataBefore fix (on
|
1 participant
Problem
Incremental updates are broken in two ways:
99% graph churn.
AnalyzeIncrementalsends achangedFilesmultipart field the API doesn't understand. The API returns a full graph with new UUIDs for every node. Editing 1 file churns 1797 out of 1801 nodes.Domain corruption.
mergeGraphunconditionally replaces domains with the incremental response. The API reclassifies domains from 1 file and returns garbage (5 real domains → 3 wrong ones likeLocalCache,GraphAPI,CLIConfig).Full investigation with repro data: #50
Fix
AnalyzeIncremental→AnalyzeSidecarsin the daemon'sincrementalUpdate. The small zip goes through the normal endpoint. Graph-fusion proved this returns a correct subgraph with 0% churn.AnalyzeIncrementalandpostIncrementalZip— dead code after the fix.Results
Tests
9 new tests covering merge logic:
TestMergeGraph_BasicMerge— new nodes added alongside existingTestMergeGraph_FileReplacement— re-processed file replaces old nodesTestMergeGraph_UUIDRemapping— relationships from unchanged files remap to new UUIDsTestMergeGraph_ExternalDependencyResolution— suffix matching resolves LocalDependency placeholdersTestMergeGraph_EmptyIncremental— doesn't panicTestMergeGraph_IncrementalNoMatchingExisting— new file added cleanlyTestMergeGraph_NilExisting— incremental into empty daemonTestMergeGraph_DomainsPreservedOnIncremental— domains survive incremental mergeTestMergeGraph_DomainsPreservedEvenWhenIncrementalHasMore— domains preserved even when incremental has moreTest plan
go test ./... -race— all tests passgo vet ./...— cleansupermodel watch→ trigger incremental viasupermodel hook→ diff graph before/after → 0% churn, domains identicalimported-byrelationships correct after incrementalKnown issue (pre-existing, not introduced by this PR)
The suffix matcher in
mergeGraphcan resolveLocalDependencynodes to the wrong file when multiple files share a common suffix (e.g.config/config.gomatchesarchdocs/pssg/config/config.go). This affects both the old and new code paths. Filed separately.Summary by CodeRabbit
Changes
Tests