feat: allow restore-from-gist URL without token; fall back to public …#119
feat: allow restore-from-gist URL without token; fall back to public …#119
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds unauthenticated URL-based Gist restore/pull flows, centralizes gist filename→category mapping ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "UI (internal/ui/gist.go)"
participant Storage as "Storage (internal/storage/gist_sync.go)"
participant GistAPI as "Gist API"
User->>UI: Enter Gist URL/ID
UI->>UI: parse gistID, set gistFetchPending, bump gistFetchSeq
UI->>Storage: doFetchAvailableGistCategoriesCmd(requestID, gistID)
Storage->>GistAPI: Fetch gist (auth if available, else unauth)
GistAPI-->>Storage: Return gist files
Storage-->>UI: gistRestoreAvailableMsg(gist, categories, requestID)
UI->>User: Show categories
User->>UI: Select categories, confirm
UI->>Storage: doCheckGistConflictsCmd(gist, prefs)
Storage->>Storage: ConflictingFilesForGist(compare gist vs local via teraConfigDir)
Storage-->>UI: gistConflictCheckMsg(gist, prefs, conflicts)
alt No conflicts or force
UI->>Storage: restoreGistCmd(gist, prefs, force)
Storage->>Storage: RestoreFromGistDirect / PullFromGist (fetch raw content)
Storage->>Storage: stageAndWriteGistFiles -> atomicWriteFile(write staged files)
Storage-->>UI: Restore complete
UI->>User: Display success
else Conflicts
UI->>User: Prompt resolution
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (2)
internal/storage/gist_sync.go (1)
369-423: Consider extracting shared category-detection logic.The switch-case logic for mapping gist filenames to
SyncPrefscategories is duplicated across three locations:
AvailableCategories(lines 172-192)AvailableCategoriesFromGist(lines 376-394)AvailableCategoriesFromGistFiles(lines 405-420)All three iterate over file names and apply the same mapping. A helper function could reduce duplication:
♻️ Suggested refactor to extract common logic
// categorizePath updates prefs based on a single gist filename. func categorizePath(name string, prefs *SyncPrefs) { relPath := gistFilenameToRelPath(name) switch { case relPath == "config.yaml": prefs.Settings = true case relPath == filepath.Join("data", "favorites", SystemFileSearchHistory): prefs.SearchHistory = true case strings.HasPrefix(filepath.ToSlash(relPath), "data/favorites/"): prefs.Favorites = true case relPath == filepath.Join("data", "station_ratings.json") || relPath == filepath.Join("data", "voted_stations.json"): prefs.RatingsVotes = true case relPath == filepath.Join("data", "blocklist.json"): prefs.Blocklist = true case relPath == filepath.Join("data", "station_metadata.json") || relPath == filepath.Join("data", "station_tags.json"): prefs.MetadataTags = true } } // AvailableCategoriesFromGistFiles can then become: func AvailableCategoriesFromGistFiles(files map[string]gist.GistFile) SyncPrefs { var prefs SyncPrefs for name := range files { categorizePath(name, &prefs) } return prefs }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 369 - 423, The duplicate switch/categorization logic in AvailableCategories, AvailableCategoriesFromGist, and AvailableCategoriesFromGistFiles should be extracted into a single helper (e.g., categorizePath(name string, prefs *SyncPrefs)) that calls gistFilenameToRelPath and updates the SyncPrefs flags; then replace each loop body to call categorizePath for each filename and return the aggregated prefs, keeping existing function signatures (AvailableCategories, AvailableCategoriesFromGist, AvailableCategoriesFromGistFiles) unchanged.internal/ui/gist.go (1)
1042-1057: Note: Signature mismatch ifRestoreFromGistDirectgains aforceparameter.If
RestoreFromGistDirectis updated to accept aforceparameter as suggested in the storage review, this call site will need to be updated accordingly:- if err := storage.RestoreFromGistDirect(g, prefs); err != nil { + if err := storage.RestoreFromGistDirect(g, prefs, force); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/gist.go` around lines 1042 - 1057, The call site in restoreGistCmd needs updating to match the new storage.RestoreFromGistDirect signature: if storage.RestoreFromGistDirect is extended to accept a force bool, pass the existing force variable through when calling it in the standalone-restore branch (the same way PullFromGist is invoked), so update the call in GistModel.restoreGistCmd to call storage.RestoreFromGistDirect(g, prefs, force) and preserve the current error handling and successMsg behavior.
🤖 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/storage/gist_sync.go`:
- Around line 425-463: RestoreFromGistDirect currently writes staged files
directly and can silently overwrite existing files; add a pre-write conflict
detection step that inspects each staged entry (staged map) against the existing
destination (baseDir + relPath) using os.Stat/os.ReadFile and byte-equality
comparison, collect any differing paths into a conflicts slice and return a
descriptive error (or abort) if non-empty instead of writing; implement this as
a small helper like detectGistConflicts(staged map[string][]byte, baseDir
string) ([]string, error) and call it in RestoreFromGistDirect before creating
directories/writing files so restores without a manager/token still report/avoid
overwrites.
In `@internal/ui/gist.go`:
- Around line 1027-1040: The doCheckGistConflictsCmd currently skips conflict
detection when mgr == nil which returns gistConflictCheckMsg with conflicts=nil
and allows restores to proceed silently; update doCheckGistConflictsCmd to call
the standalone conflict checker you added for RestoreFromGistDirect (instead of
returning early) so it always computes conflicts: locate doCheckGistConflictsCmd
and replace the mgr==nil branch to invoke the new checker (the same logic used
by RestoreFromGistDirect), return any error as errMsg (as you do for
ConflictingGistFiles), and return gistConflictCheckMsg with the checker’s
conflicts so overwrite warnings are shown even when no mgr/token exists.
---
Nitpick comments:
In `@internal/storage/gist_sync.go`:
- Around line 369-423: The duplicate switch/categorization logic in
AvailableCategories, AvailableCategoriesFromGist, and
AvailableCategoriesFromGistFiles should be extracted into a single helper (e.g.,
categorizePath(name string, prefs *SyncPrefs)) that calls gistFilenameToRelPath
and updates the SyncPrefs flags; then replace each loop body to call
categorizePath for each filename and return the aggregated prefs, keeping
existing function signatures (AvailableCategories, AvailableCategoriesFromGist,
AvailableCategoriesFromGistFiles) unchanged.
In `@internal/ui/gist.go`:
- Around line 1042-1057: The call site in restoreGistCmd needs updating to match
the new storage.RestoreFromGistDirect signature: if
storage.RestoreFromGistDirect is extended to accept a force bool, pass the
existing force variable through when calling it in the standalone-restore branch
(the same way PullFromGist is invoked), so update the call in
GistModel.restoreGistCmd to call storage.RestoreFromGistDirect(g, prefs, force)
and preserve the current error handling and successMsg behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8eb8a1b4-7c0b-4c94-ad16-ccfac1406ab8
📒 Files selected for processing (2)
internal/storage/gist_sync.gointernal/ui/gist.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/storage/gist_sync.go (2)
340-347:⚠️ Potential issue | 🟠 MajorThe restore write path is still crash-unsafe.
Staging downloads only protects the fetch phase. Once these loops start, a later failure can leave some files updated and others old, and
os.WriteFilecan truncate an existing file if the process dies mid-write. At minimum, switch these writes toatomicWriteFile; otherwise the “mid-restore failure” comment overstates the safety of this flow.Use the existing atomic write helper
- if err := os.WriteFile(destPath, content, 0600); err != nil { + if err := atomicWriteFile(destPath, content, 0600); err != nil { return fmt.Errorf("failed to write %s: %w", relPath, err) }Apply the same change in both restore loops.
Also applies to: 466-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 340 - 347, The restore loop that writes staged files uses os.WriteFile and is crash-unsafe; replace the direct writes in the loop iterating over staged (using destPath := filepath.Join(m.configDir, relPath)) with the existing atomicWriteFile helper so each file is written atomically (preserve the 0600 mode and keep the os.MkdirAll(dir, 0755) before writing); make the same change in the second restore loop (the other occurrence around the 466-473 region) so both restore paths use atomicWriteFile instead of os.WriteFile.
297-309:⚠️ Potential issue | 🟠 MajorReject non-backup gists before restoring.
FindBackupGistonly accepts gists that containtera-manifest.json, but these new direct restore paths skip that guard entirely. A random gist that happens to containconfig.yamlormetadata.jsonwill now be treated as restorable and can overwrite local state.Suggested guard
func (m *GistSyncManager) PullFromGist(g *gist.Gist, prefs SyncPrefs, force bool) error { if g == nil { return fmt.Errorf("gist is required") } + if _, ok := g.Files[backupGistMarkerFile]; !ok { + return fmt.Errorf("gist is not a tera backup") + } if !force { conflicts, err := m.ConflictingGistFiles(g, prefs) if err != nil { return err @@ func RestoreFromGistDirect(g *gist.Gist, prefs SyncPrefs, force bool) error { if g == nil { return fmt.Errorf("gist is required") } + if _, ok := g.Files[backupGistMarkerFile]; !ok { + return fmt.Errorf("gist is not a tera backup") + } if !force { conflicts, err := ConflictingFilesForGist(g, prefs) if err != nil { return err }Also applies to: 432-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 297 - 309, The PullFromGist path currently trusts any gist containing config.yaml/metadata.json and can overwrite local state; update GistSyncManager.PullFromGist to reject non-backup gists by verifying the gist contains the backup manifest (tera-manifest.json) before proceeding (e.g., reuse the same check logic as FindBackupGist or add a helper like isBackupGist), and return a clear error when the manifest is missing; apply the same manifest-check guard to the other restore path that currently skips it (the similar restore function around the later diff) and keep existing conflict checks (ConflictingGistFiles/RestoreConflictError) intact.
🤖 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/ui/gist.go`:
- Around line 513-523: The code currently sets m.state = gistStateSyncProgress
before invoking doFetchAvailableGistCategoriesCmd(gistID), which causes errors
to be handled from the progress state and drop the user out of the URL form;
instead, keep the current URL-entry state and invoke
doFetchAvailableGistCategoriesCmd without changing m.state, show a transient
m.message/m.messageIsError to inform the user while the fetch runs, and only
switch m.state to gistStateSyncProgress inside the success path of the command
(or have the command send a message that the Update handler uses to set m.state
on success); update the error handling in the command's result to set
m.message/m.messageIsError and leave m.state unchanged so the user stays on the
input form for correction/retry.
---
Outside diff comments:
In `@internal/storage/gist_sync.go`:
- Around line 340-347: The restore loop that writes staged files uses
os.WriteFile and is crash-unsafe; replace the direct writes in the loop
iterating over staged (using destPath := filepath.Join(m.configDir, relPath))
with the existing atomicWriteFile helper so each file is written atomically
(preserve the 0600 mode and keep the os.MkdirAll(dir, 0755) before writing);
make the same change in the second restore loop (the other occurrence around the
466-473 region) so both restore paths use atomicWriteFile instead of
os.WriteFile.
- Around line 297-309: The PullFromGist path currently trusts any gist
containing config.yaml/metadata.json and can overwrite local state; update
GistSyncManager.PullFromGist to reject non-backup gists by verifying the gist
contains the backup manifest (tera-manifest.json) before proceeding (e.g., reuse
the same check logic as FindBackupGist or add a helper like isBackupGist), and
return a clear error when the manifest is missing; apply the same manifest-check
guard to the other restore path that currently skips it (the similar restore
function around the later diff) and keep existing conflict checks
(ConflictingGistFiles/RestoreConflictError) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c137342b-d04d-41a0-9241-a8631762afd3
📒 Files selected for processing (2)
internal/storage/gist_sync.gointernal/ui/gist.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/gist.go (1)
267-283:⚠️ Potential issue | 🟠 MajorDon't send every
gistStateSyncProgressfailure back to the URL form.Line 280 now rewrites all progress-state errors to
gistStateRestoreGistURL, but that state is also used by zip export, zip restore, sync-to-gist, and the actual restore command. A failed export or zip restore will now dump the user onto the gist URL screen. The URL fetch path no longer entersgistStateSyncProgress, so this branch never fixes the retry case it was added for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/gist.go` around lines 267 - 283, The error branch for errMsg currently maps any gistStateSyncProgress failure to gistStateRestoreGistURL which incorrectly redirects failures from zip export/restore and other flows; instead, only redirect to gistStateRestoreGistURL if the sync was actually triggered from the URL form (or the prior state was gistStateRestoreGistURL). Update the errMsg handling in the switch so the gistStateSyncProgress case checks a reliable indicator (e.g., a new boolean field like m.syncTriggeredFromURL or an existing prior-state field such as m.prevState) and only set m.state = gistStateRestoreGistURL when that indicator shows the URL flow initiated the sync; otherwise restore the appropriate state (e.g., gistStateMenu or m.prevState) so export/restore flows are not dropped onto the URL screen.
🤖 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/storage/atomic_write.go`:
- Around line 48-53: After renaming the temp file with os.Rename(tmpPath, path)
you must fsync the parent directory so the directory entry is durable; open the
parent directory (using the target path's directory), call Sync() and close it,
handle and return any error from that Sync, and only set success = true after
the directory Sync succeeds—this complements the existing tmp.Sync() and ensures
the rename is not lost on crash.
In `@internal/storage/gist_sync.go`:
- Around line 382-401: AvailableCategoriesFromGist and
AvailableCategoriesFromGistFiles must first verify the backup marker file
("tera-manifest.json") exists before inspecting filenames; if the marker is
missing, return an empty SyncPrefs immediately to avoid treating arbitrary gists
as restorables. Update AvailableCategoriesFromGist to check g.Files for
"tera-manifest.json" and return SyncPrefs{} (no error) when absent, and update
AvailableCategoriesFromGistFiles to do the same on the files map, then proceed
to call categorizePath for each name only when the marker is present.
---
Outside diff comments:
In `@internal/ui/gist.go`:
- Around line 267-283: The error branch for errMsg currently maps any
gistStateSyncProgress failure to gistStateRestoreGistURL which incorrectly
redirects failures from zip export/restore and other flows; instead, only
redirect to gistStateRestoreGistURL if the sync was actually triggered from the
URL form (or the prior state was gistStateRestoreGistURL). Update the errMsg
handling in the switch so the gistStateSyncProgress case checks a reliable
indicator (e.g., a new boolean field like m.syncTriggeredFromURL or an existing
prior-state field such as m.prevState) and only set m.state =
gistStateRestoreGistURL when that indicator shows the URL flow initiated the
sync; otherwise restore the appropriate state (e.g., gistStateMenu or
m.prevState) so export/restore flows are not dropped onto the URL screen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8827688f-434f-410c-97bc-faa27f924045
📒 Files selected for processing (5)
internal/storage/atomic_write.gointernal/storage/gist_sync.gointernal/storage/history.gointernal/storage/lock.gointernal/ui/gist.go
💤 Files with no reviewable changes (1)
- internal/storage/history.go
✅ Files skipped from review due to trivial changes (1)
- internal/storage/lock.go
| if err := os.Rename(tmpPath, path); err != nil { | ||
| return fmt.Errorf("failed to rename temp file to destination: %w", err) | ||
| } | ||
|
|
||
| success = true | ||
| return nil |
There was a problem hiding this comment.
Sync the parent directory after Line 48.
tmp.Sync() only makes the temp file durable. The rename itself is still vulnerable until the containing directory entry is flushed, so a crash right after Line 48 can lose the new file even though the helper returned success.
💡 Minimal durability fix
if err := os.Rename(tmpPath, path); err != nil {
return fmt.Errorf("failed to rename temp file to destination: %w", err)
}
+ dirHandle, err := os.Open(dir)
+ if err != nil {
+ return fmt.Errorf("failed to open parent directory: %w", err)
+ }
+ defer dirHandle.Close()
+ if err := dirHandle.Sync(); err != nil {
+ return fmt.Errorf("failed to sync parent directory: %w", err)
+ }
success = true
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/atomic_write.go` around lines 48 - 53, After renaming the
temp file with os.Rename(tmpPath, path) you must fsync the parent directory so
the directory entry is durable; open the parent directory (using the target
path's directory), call Sync() and close it, handle and return any error from
that Sync, and only set success = true after the directory Sync succeeds—this
complements the existing tmp.Sync() and ensures the rename is not lost on crash.
…rd in category inspectio
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/storage/gist_sync.go`:
- Around line 297-303: PullFromGist currently assumes g.Files is populated and
returns “missing tera-manifest.json” for listing responses; before checking for
backupGistMarkerFile (and similarly in AvailableCategoriesFromGist), detect when
len(g.Files) == 0 and call the GitHub client’s GetGist method (same approach
used in ConflictingGistFiles) to fetch the full gist, replace or update g with
the full result, then proceed with the existing marker/file checks (referenced
symbols: PullFromGist, AvailableCategoriesFromGist, ConflictingGistFiles,
gist.Gist.Files, GetGist, backupGistMarkerFile).
In `@internal/ui/gist.go`:
- Around line 364-367: The handler for gistRestoreAvailableMsg should ignore
results if the user has already left the URL form: before setting m.pendingGist,
m.checklist, and m.state = gistStateRestoreGistChecklist, verify the current
m.state is gistStateRestoreGistURL (or another expected pre-fetch state) so
stale async results from doFetchAvailableGistCategoriesCmd are dropped; if the
state matches, set m.pendingGist and m.checklist via availableChecklist and
transition m.state to gistStateRestoreGistChecklist and also clear the "Fetching
Gist…" progress text (e.g., reset whatever status/progress field shows that
message) so a cancelled fetch doesn't leave a stale progress message visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3d117474-f71c-43a3-bf38-70745c7a9dc3
📒 Files selected for processing (3)
internal/storage/atomic_write.gointernal/storage/gist_sync.gointernal/ui/gist.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/storage/atomic_write.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/ui/gist.go (1)
364-374:⚠️ Potential issue | 🟠 MajorCorrelate URL-fetch responses with the active request.
The URL form stays interactive while
doFetchAvailableGistCategoriesCmdruns, but the command returns unversioned success/error messages. If the user presses Enter twice or cancels with Esc, an older request can still open the checklist or replace the current screen message after cancel. Please return a dedicated fetch result message carrying a request ID (and error), ignore anything that's no longer current, and clear the transient"Fetching Gist…"status when the URL flow is cancelled.Also applies to: 519-540, 986-1021
internal/storage/gist_sync.go (1)
426-449:⚠️ Potential issue | 🟡 MinorHydrate sparse gists in the standalone helpers too.
ConflictingFilesForGistandRestoreFromGistDirectstill assumeg.Filesis populated. Passing a summary gist here yields either no conflicts or a false missing-marker error, unlikePullFromGistandAvailableCategoriesFromGist, which already hydrate first.💡 Suggested fix
func ConflictingFilesForGist(g *gist.Gist, prefs SyncPrefs) ([]string, error) { if g == nil { return nil, fmt.Errorf("gist is required") } + if len(g.Files) == 0 { + full, err := gist.GetGistPublic(g.ID) + if err != nil { + return nil, fmt.Errorf("failed to fetch gist: %w", err) + } + g = full + } configDir, err := os.UserConfigDir() if err != nil { return nil, fmt.Errorf("failed to get config directory: %w", err) } @@ func RestoreFromGistDirect(g *gist.Gist, prefs SyncPrefs, force bool) error { if g == nil { return fmt.Errorf("gist is required") } + if len(g.Files) == 0 { + full, err := gist.GetGistPublic(g.ID) + if err != nil { + return fmt.Errorf("failed to fetch gist: %w", err) + } + g = full + } if _, ok := g.Files[backupGistMarkerFile]; !ok { return fmt.Errorf("gist is not a tera backup (missing %s)", backupGistMarkerFile) }Also applies to: 455-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 426 - 449, ConflictingFilesForGist (and similarly RestoreFromGistDirect) assumes g.Files is populated but may be given a summary gist; before using g.Files, perform the same gist-hydration step used by PullFromGist and AvailableCategoriesFromGist (i.e. fetch/expand the full gist so g.Files is populated) and return any fetch error; then proceed to compute conflicts as before so summary gists no longer produce missing/empty results.
🧹 Nitpick comments (1)
internal/storage/gist_sync.go (1)
430-435: Use one shared config-dir resolver for the no-token path.These helpers rebuild
filepath.Join(os.UserConfigDir(), "tera")inline. Please route that through a shared resolver so restore/conflict checks stay aligned with the app's normal config-dir lookup and remain easy to test.Also applies to: 471-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 430 - 435, The code constructs the app config path inline using os.UserConfigDir() + "tera" (configDir/baseDir) for the no-token path; replace these inline constructions with the app's shared config-dir resolver so both the restore/conflict logic and the normal lookup use the same implementation. Concretely, remove the os.UserConfigDir()/filepath.Join(...) usage in the no-token branches and call the shared resolver function used elsewhere (e.g. ResolveConfigDir or GetAppConfigDir) to obtain the base config directory, then use that return value where configDir/baseDir were used; apply the same change for the second occurrence mentioned around the later block so both sites use the single resolver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/storage/gist_sync.go`:
- Around line 426-449: ConflictingFilesForGist (and similarly
RestoreFromGistDirect) assumes g.Files is populated but may be given a summary
gist; before using g.Files, perform the same gist-hydration step used by
PullFromGist and AvailableCategoriesFromGist (i.e. fetch/expand the full gist so
g.Files is populated) and return any fetch error; then proceed to compute
conflicts as before so summary gists no longer produce missing/empty results.
---
Nitpick comments:
In `@internal/storage/gist_sync.go`:
- Around line 430-435: The code constructs the app config path inline using
os.UserConfigDir() + "tera" (configDir/baseDir) for the no-token path; replace
these inline constructions with the app's shared config-dir resolver so both the
restore/conflict logic and the normal lookup use the same implementation.
Concretely, remove the os.UserConfigDir()/filepath.Join(...) usage in the
no-token branches and call the shared resolver function used elsewhere (e.g.
ResolveConfigDir or GetAppConfigDir) to obtain the base config directory, then
use that return value where configDir/baseDir were used; apply the same change
for the second occurrence mentioned around the later block so both sites use the
single resolver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0811c9a4-2841-47f3-a81e-cb261c074751
📒 Files selected for processing (2)
internal/storage/gist_sync.gointernal/ui/gist.go
…pers, guard double-submit in URL form
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/storage/gist_sync.go (1)
427-432: Reuse the canonical config-dir resolver here.
internal/config/loader.go:16-27already owns this logic. Duplicating it here makesteraConfigDir()a second source of truth, so a future change to the app’s config-dir rules can silently split gist restore/sync from the rest of the app. If importing that package would create a cycle, I’d still prefer moving the shared helper lower rather than hardcoding"tera"twice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 427 - 432, teraConfigDir duplicates the app's config-dir resolution; replace its logic by reusing the canonical resolver in internal/config/loader.go (or move that shared helper into a lower-level package if importing causes a cycle). Update references to teraConfigDir so they call the shared function from internal/config/loader.go (or the newly extracted helper) instead of duplicating os.UserConfigDir + "tera", ensuring only one source of truth for the config directory across gist_sync.go and the rest of the app.
🤖 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/ui/gist.go`:
- Around line 72-75: The guard using m.state != gistStateRestoreGistURL is
insufficient; instead generate and track a unique request ID when starting a
gist fetch, store it on the component (e.g., m.currentRestoreRequestID), include
that ID in the response messages (e.g., add a requestID string field to
gistRestoreAvailableMsg), and then ignore incoming results whose requestID does
not match m.currentRestoreRequestID before updating state or opening the
checklist; apply the same pattern to the other fetch/response pairs referenced
(the other response message types and their handlers) so stale responses are
fenced off even if the user re-enters the form.
---
Nitpick comments:
In `@internal/storage/gist_sync.go`:
- Around line 427-432: teraConfigDir duplicates the app's config-dir resolution;
replace its logic by reusing the canonical resolver in internal/config/loader.go
(or move that shared helper into a lower-level package if importing causes a
cycle). Update references to teraConfigDir so they call the shared function from
internal/config/loader.go (or the newly extracted helper) instead of duplicating
os.UserConfigDir + "tera", ensuring only one source of truth for the config
directory across gist_sync.go and the rest of the app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b053df7f-74e9-4626-80e0-b3beb15a632f
📒 Files selected for processing (2)
internal/storage/gist_sync.gointernal/ui/gist.go
…fetch responses by request ID
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/storage/gist_sync.go (1)
432-463:ConflictingFilesForGistre-fetches usingGetGistPublicwhich may fail for authenticated gists.When
g.Filesis empty, the function callsgist.GetGistPublic(g.ID)(line 439). If the original gist was fetched via an authenticated client (and is a private gist), the public re-fetch will fail with a 404. This is inconsistent with the UI flow wheredoFetchAvailableGistCategoriesCmdalready fetches the full gist and passes it with populatedFiles.In practice, the UI always passes a fully-fetched gist here, so this is unlikely to trigger. However, for API consistency and defensive coding:
💡 Consider documenting or removing the empty-files branch
Since callers always provide a fully-fetched gist, you could either:
- Document this precondition and remove the re-fetch logic
- Accept an optional authenticated client parameter for private gist support
// ConflictingFilesForGist checks for existing local files that would be // overwritten by a restore from the given Gist. Works without a GistSyncManager. +// The caller must ensure g.Files is populated (e.g., by fetching the full gist first). func ConflictingFilesForGist(g *gist.Gist, prefs SyncPrefs) ([]string, error) { if g == nil { return nil, fmt.Errorf("gist is required") } - if len(g.Files) == 0 { - full, err := gist.GetGistPublic(g.ID) - if err != nil { - return nil, fmt.Errorf("failed to fetch gist: %w", err) - } - g = full - } + if len(g.Files) == 0 { + return nil, fmt.Errorf("gist files not populated; fetch full gist first") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 432 - 463, ConflictingFilesForGist currently re-fetches the gist via gist.GetGistPublic when g.Files is empty which breaks for private/authenticated gists; remove that public re-fetch and instead require a fully-populated gist: if g.Files is empty return a clear error (or document the precondition) so callers like doFetchAvailableGistCategoriesCmd must pass the full gist; update ConflictingFilesForGist and its callers/tests accordingly to avoid unauthenticated HTTP calls for private gists.
🤖 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/storage/gist_sync.go`:
- Around line 432-463: ConflictingFilesForGist currently re-fetches the gist via
gist.GetGistPublic when g.Files is empty which breaks for private/authenticated
gists; remove that public re-fetch and instead require a fully-populated gist:
if g.Files is empty return a clear error (or document the precondition) so
callers like doFetchAvailableGistCategoriesCmd must pass the full gist; update
ConflictingFilesForGist and its callers/tests accordingly to avoid
unauthenticated HTTP calls for private gists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 01028364-a19a-429c-ac10-09b78440272e
📒 Files selected for processing (2)
internal/storage/gist_sync.gointernal/ui/gist.go
…ion error in ConflictingFilesForGist and RestoreFromGistDirect
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/gist_sync.go (1)
336-359:⚠️ Potential issue | 🟠 MajorStaging the bytes here is not atomic across the full restore set.
If a later
MkdirAlloratomicWriteFilefails, earlier files are already committed and the config tree is left in a mixed snapshot. That means the “everything is ready before writing” comment on Line 336 overstates the guarantee. If partial restores are not acceptable, this needs a two-phase swap or rollback strategy instead of per-file commits.Also applies to: 497-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 336 - 359, The current restore loop stages file bytes in staged but then writes each file directly into m.configDir using os.MkdirAll and atomicWriteFile, which can leave a partially-updated config if any MkdirAll/atomicWriteFile fails; change to a two-phase replace: create a temporary directory (e.g., m.configDir+".tmp" or use os.MkdirTemp), write all staged contents into that temp tree using the same atomicWriteFile and MkdirAll logic, verify success, then atomically swap the tree into place with os.Rename (or remove the old config and rename the temp), and ensure cleanup/rollback of the temp dir on any error; apply the same change for the second block referenced (lines ~497-517) so both restore paths are atomic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/storage/gist_sync.go`:
- Around line 336-359: The current restore loop stages file bytes in staged but
then writes each file directly into m.configDir using os.MkdirAll and
atomicWriteFile, which can leave a partially-updated config if any
MkdirAll/atomicWriteFile fails; change to a two-phase replace: create a
temporary directory (e.g., m.configDir+".tmp" or use os.MkdirTemp), write all
staged contents into that temp tree using the same atomicWriteFile and MkdirAll
logic, verify success, then atomically swap the tree into place with os.Rename
(or remove the old config and rename the temp), and ensure cleanup/rollback of
the temp dir on any error; apply the same change for the second block referenced
(lines ~497-517) so both restore paths are atomic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8ed32add-e8ec-44ea-bb58-b86eaac3e340
📒 Files selected for processing (1)
internal/storage/gist_sync.go
… comment in Gist restore paths
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/storage/gist_sync.go (1)
498-526: Consider extracting shared fetch-and-write logic.The staged fetch and atomic write loop (lines 500-526) is nearly identical to lines 336-362 in
PullFromGist. A shared helper could reduce duplication:♻️ Optional refactor idea
// stageAndWriteGistFiles fetches content and writes atomically to baseDir. func stageAndWriteGistFiles( httpClient *http.Client, files map[string]gist.GistFile, prefs SyncPrefs, baseDir string, ) error { staged := make(map[string][]byte) for name, gistFile := range files { relPath := gistFilenameToRelPath(name) if relPath == "" || !syncPrefForRelPath(relPath, prefs) { continue } content, err := fetchRawContent(httpClient, gistFile.RawURL, gistFile.Content) if err != nil { return fmt.Errorf("failed to fetch %s: %w", name, err) } staged[relPath] = []byte(content) } for relPath, content := range staged { destPath := filepath.Join(baseDir, relPath) if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { return fmt.Errorf("failed to create directory for %s: %w", relPath, err) } if err := atomicWriteFile(destPath, content, 0600); err != nil { return fmt.Errorf("failed to write %s: %w", relPath, err) } } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 498 - 526, The fetch-and-write loop in PullFromGist and the block in gist_sync.go duplicate logic; extract a helper (e.g., stageAndWriteGistFiles) that accepts an *http.Client, files map[string]gist.GistFile, prefs SyncPrefs and baseDir and does the relPath filtering with gistFilenameToRelPath, uses fetchRawContent to fetch content, stages bytes, and writes them with os.MkdirAll + atomicWriteFile; replace both original blocks (the loop in PullFromGist and the staged loop in gist_sync.go) with calls to this helper to remove duplication and preserve the same error messages and behavior.
🤖 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/storage/gist_sync.go`:
- Around line 498-526: The fetch-and-write loop in PullFromGist and the block in
gist_sync.go duplicate logic; extract a helper (e.g., stageAndWriteGistFiles)
that accepts an *http.Client, files map[string]gist.GistFile, prefs SyncPrefs
and baseDir and does the relPath filtering with gistFilenameToRelPath, uses
fetchRawContent to fetch content, stages bytes, and writes them with os.MkdirAll
+ atomicWriteFile; replace both original blocks (the loop in PullFromGist and
the staged loop in gist_sync.go) with calls to this helper to remove duplication
and preserve the same error messages and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 680ae994-3607-48f7-89a3-3344119e696a
📒 Files selected for processing (2)
internal/storage/backup.gointernal/storage/gist_sync.go
…and-write logic in PullFromGist and RestoreFromGistDirect
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/storage/gist_sync.go (2)
263-275:⚠️ Potential issue | 🟡 MinorOnly flag differing files as conflicts.
Both helpers currently treat mere existence as an overwrite conflict. If the local file already has the same bytes as the gist, this still returns
RestoreConflictError, so a no-op re-run of pull/restore is blocked and retries after a partial success unnecessarily requireforce=true. Please compare contents and only report paths whose existing bytes differ from the staged gist content.Also applies to: 414-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 263 - 275, The current conflict detection marks any existing local file as a conflict; instead, read the staged gist content for each gist entry (use the same map entry referenced by g.Files[name] or the staged content source used elsewhere in gist_sync.go), read the local file bytes (os.ReadFile / ioutil.ReadFile) at absPath, and compare byte-for-byte; only append relPath to conflicts if the local bytes differ from the gist bytes. Update the loop that calls gistFilenameToRelPath and syncPrefForRelPath (and the analogous block around lines 414-425) to perform this content comparison before treating the file as a conflict.
464-497:⚠️ Potential issue | 🟠 MajorAvoid leaving a half-restored config on disk.
stageAndWriteGistFilesis still only atomic per file. Once the write loop starts, a laterMkdirAll/atomicWriteFilefailure returns after earlier files have already been committed, so pull/restore can leave a mixed old/new config set. Please add a rollback path or switch to a temp-tree + final swap strategy so the multi-file operation commits all-or-nothing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/gist_sync.go` around lines 464 - 497, The current stageAndWriteGistFiles only guarantees atomicity per file and can leave a partially-updated config if a later write fails; change it to build the full tree in a temporary directory and then atomically swap it into place. Specifically: after fetching contents into staged (using gistFilenameToRelPath and fetchRawContent as currently done), create a temp dir next to baseDir (os.MkdirTemp/os.CreateTemp), write every staged file into that temp tree using the same atomicWriteFile helper (preserve relative paths), fsync as needed, then perform an atomic swap by renaming the existing baseDir out to a backup and renaming the temp dir to baseDir (os.Rename). On any failure, remove the temp dir and if you already moved the original, rename the backup back. Keep all error returns but ensure cleanup paths remove temp and restore backup so the operation is all-or-nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/storage/gist_sync.go`:
- Around line 263-275: The current conflict detection marks any existing local
file as a conflict; instead, read the staged gist content for each gist entry
(use the same map entry referenced by g.Files[name] or the staged content source
used elsewhere in gist_sync.go), read the local file bytes (os.ReadFile /
ioutil.ReadFile) at absPath, and compare byte-for-byte; only append relPath to
conflicts if the local bytes differ from the gist bytes. Update the loop that
calls gistFilenameToRelPath and syncPrefForRelPath (and the analogous block
around lines 414-425) to perform this content comparison before treating the
file as a conflict.
- Around line 464-497: The current stageAndWriteGistFiles only guarantees
atomicity per file and can leave a partially-updated config if a later write
fails; change it to build the full tree in a temporary directory and then
atomically swap it into place. Specifically: after fetching contents into staged
(using gistFilenameToRelPath and fetchRawContent as currently done), create a
temp dir next to baseDir (os.MkdirTemp/os.CreateTemp), write every staged file
into that temp tree using the same atomicWriteFile helper (preserve relative
paths), fsync as needed, then perform an atomic swap by renaming the existing
baseDir out to a backup and renaming the temp dir to baseDir (os.Rename). On any
failure, remove the temp dir and if you already moved the original, rename the
backup back. Keep all error returns but ensure cleanup paths remove temp and
restore backup so the operation is all-or-nothing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3ceb1925-b0ec-49ea-91fb-7a2d809028d3
📒 Files selected for processing (1)
internal/storage/gist_sync.go
|
@coderabbitai Regarding to your Outside diff range comments, the following is the reply: |
|
Tip For best results, initiate chat on the files or code changes.
I've recorded both decisions as learnings for future reviews. (˵ •̀ ᴗ - ˵) ✧ ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/storage/gist_sync.go`:
- Around line 333-349: The gist filename mapping allows different public
filenames to collapse to the same local relPath (e.g., "search-history.json" and
"fav--search-history.json" both map to filepath.Join("data","favorites",
SystemFileSearchHistory)), so modify the mapping logic (in categorizePath and
the code that builds the staged map which keys by relPath) to detect and reject
duplicate mapped destinations up front: when gistFilenameToRelPath(name) returns
a relPath that is already claimed, return an error (or skip and log) instead of
overwriting staged[relPath]; additionally explicitly reserve the fav-- +
SystemFileSearchHistory form by treating any mapping that produces
filepath.Join("data","favorites", SystemFileSearchHistory) as valid only when
the original gist filename has the fav-- prefix (or reject the alias), and apply
the same collision check to the other mapping block mentioned (lines ~481-492)
so no two different gist names can map to the same relPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: df755052-092d-4bf2-80b5-388c0a9fd9be
📒 Files selected for processing (1)
internal/storage/gist_sync.go
| func categorizePath(name string, prefs *SyncPrefs) { | ||
| relPath := gistFilenameToRelPath(name) | ||
| switch { | ||
| case relPath == "config.yaml": | ||
| prefs.Settings = true | ||
| case relPath == filepath.Join("data", "favorites", SystemFileSearchHistory): | ||
| prefs.SearchHistory = true | ||
| case strings.HasPrefix(filepath.ToSlash(relPath), "data/favorites/"): | ||
| prefs.Favorites = true | ||
| case relPath == filepath.Join("data", "station_ratings.json") || | ||
| relPath == filepath.Join("data", "voted_stations.json"): | ||
| prefs.RatingsVotes = true | ||
| case relPath == filepath.Join("data", "blocklist.json"): | ||
| prefs.Blocklist = true | ||
| case relPath == filepath.Join("data", "station_metadata.json") || | ||
| relPath == filepath.Join("data", "station_tags.json"): | ||
| prefs.MetadataTags = true |
There was a problem hiding this comment.
Reject filename aliases that collapse onto the same restore path.
The new shared mapping accepts both search-history.json and fav--search-history.json, but both resolve to the same local file. That means a crafted public gist can be categorized incorrectly and, during restore, whichever entry is seen last silently wins because staged is keyed by relPath. Please reject duplicate mapped destinations up front, or explicitly reserve fav-- + SystemFileSearchHistory.
Also applies to: 481-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/gist_sync.go` around lines 333 - 349, The gist filename
mapping allows different public filenames to collapse to the same local relPath
(e.g., "search-history.json" and "fav--search-history.json" both map to
filepath.Join("data","favorites", SystemFileSearchHistory)), so modify the
mapping logic (in categorizePath and the code that builds the staged map which
keys by relPath) to detect and reject duplicate mapped destinations up front:
when gistFilenameToRelPath(name) returns a relPath that is already claimed,
return an error (or skip and log) instead of overwriting staged[relPath];
additionally explicitly reserve the fav-- + SystemFileSearchHistory form by
treating any mapping that produces filepath.Join("data","favorites",
SystemFileSearchHistory) as valid only when the original gist filename has the
fav-- prefix (or reject the alias), and apply the same collision check to the
other mapping block mentioned (lines ~481-492) so no two different gist names
can map to the same relPath.
…o prevent destination collision
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
…fetch
Closes #
📑 Description
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
Improvements
Bug Fixes