Skip to content

Fix macOS goal editing updates#5758

Merged
kodjima33 merged 1 commit intomainfrom
codex/macos-goal-edit-fix
Mar 17, 2026
Merged

Fix macOS goal editing updates#5758
kodjima33 merged 1 commit intomainfrom
codex/macos-goal-edit-fix

Conversation

@kodjima33
Copy link
Copy Markdown
Collaborator

Summary\n- add a desktop API client method for full goal updates\n- wire the goal edit sheet to save title, current value, and target value\n- refresh cached/local goals after a successful update\n\n## Testing\n- xcrun swift build -c debug --package-path Desktop\n- launched Omi Dev.app and verified the desktop app started successfully\n

@kodjima33 kodjima33 merged commit f97384b into main Mar 17, 2026
1 check passed
@kodjima33 kodjima33 deleted the codex/macos-goal-edit-fix branch March 17, 2026 21:25
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 17, 2026

Greptile Summary

This PR fixes macOS goal editing by plumbing a full goal-update flow: a new updateGoal PATCH method in APIClient, a new onUpdateGoal closure in GoalsWidget, and a new updateGoal function in DashboardViewModel that calls the API and refreshes the local goal list.

Key observations:

  • The API client change is correct — the patch helper sets Content-Type: application/json via buildHeaders, and the CodingKeys enum properly maps Swift camelCase to the backend's snake_case (current_value, target_value). The goalsCache is cleared after the call, consistent with surrounding methods.
  • GoalsWidget correctly replaces the old onUpdateProgress-only call with the richer onUpdateGoal callback, and the GoalEditSheet already populates title, currentValue, and targetValue from the existing goal on onAppear.
  • Unlike updateGoalProgress (which applies an optimistic in-memory update before the API call), updateGoal waits for the full round-trip before refreshing the UI. This introduces a visible delay between the user tapping "Save" and seeing the updated goal.
  • If GoalStorage.syncServerGoal fails silently (via try?), the subsequent getLocalGoals will return the old local data even though the backend update succeeded, leaving the UI appearing unchanged. Adding a direct array update as a fallback would be more robust.

Confidence Score: 4/5

  • Safe to merge — the core fix is correct and follows established patterns; minor UX and resilience improvements are possible.
  • The API client and widget wiring are correct. The main concerns are a missing optimistic update (causing UI lag compared to updateGoalProgress) and the possibility of a silent syncServerGoal failure leaving stale data in the UI despite a successful API update. Neither is a blocking regression, but the optimistic-update gap is a noticeable UX inconsistency in an otherwise deliberately responsive design.
  • desktop/Desktop/Sources/MainWindow/Pages/DashboardPage.swift — updateGoal lacks the optimistic update pattern used by the sibling updateGoalProgress.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/APIClient.swift Adds updateGoal PATCH method. Uses the existing patch helper correctly (Content-Type header set via buildHeaders, explicit CodingKeys for snake_case mapping). Clears goalsCache after the call, consistent with surrounding methods. No issues found.
desktop/Desktop/Sources/MainWindow/Components/GoalsWidget.swift Adds onUpdateGoal callback to GoalsWidget and wires it to GoalEditSheet.onSave. The old behaviour (calling onUpdateProgress with just the current value) is correctly replaced with a full-update call. Preview stub updated. No issues.
desktop/Desktop/Sources/MainWindow/Pages/DashboardPage.swift Adds updateGoal to DashboardViewModel and wires the new onUpdateGoal closure in the view. Missing optimistic update (unlike the sibling updateGoalProgress) and the silent syncServerGoal failure can leave the UI with stale data after a successful API call.

Sequence Diagram

sequenceDiagram
    participant User
    participant GoalEditSheet
    participant GoalsWidget
    participant DashboardPage
    participant DashboardViewModel
    participant APIClient
    participant GoalStorage

    User->>GoalEditSheet: Tap "Save"
    GoalEditSheet->>GoalsWidget: onSave(title, current, target)
    GoalEditSheet->>GoalEditSheet: onDismiss() — sheet dismissed immediately
    GoalsWidget->>DashboardPage: onUpdateGoal(goal, title, current, target)
    DashboardPage->>DashboardViewModel: Task { await updateGoal(...) }
    DashboardViewModel->>APIClient: updateGoal(goalId, title, currentValue, targetValue)
    APIClient->>APIClient: PATCH v1/goals/{goalId} (JSON body)
    APIClient-->>DashboardViewModel: updated: Goal
    APIClient->>APIClient: goalsCache = nil
    DashboardViewModel->>GoalStorage: syncServerGoal(updated) [try?]
    GoalStorage-->>DashboardViewModel: (may silently fail)
    DashboardViewModel->>GoalStorage: getLocalGoals()
    GoalStorage-->>DashboardViewModel: [Goal]
    DashboardViewModel->>DashboardPage: goals updated → UI refreshed
Loading

Last reviewed commit: 19ca2a6

Comment on lines +161 to +179
func updateGoal(_ goal: Goal, title: String, currentValue: Double, targetValue: Double) async {
log("Goals: Updating goal '\(goal.title)' -> title='\(title)', current=\(currentValue), target=\(targetValue)")

do {
let updated = try await APIClient.shared.updateGoal(
goalId: goal.id,
title: title,
currentValue: currentValue,
targetValue: targetValue
)

_ = try? await GoalStorage.shared.syncServerGoal(updated)
goals = try await GoalStorage.shared.getLocalGoals()
log("Goals: Updated goal '\(updated.title)' confirmed by API")
} catch {
logError("Failed to update goal", error: error)
goals = (try? await GoalStorage.shared.getLocalGoals()) ?? goals
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No optimistic update before API call

updateGoalProgress immediately updates both goals[index].currentValue and local SQLite before making the API call, giving instant UI feedback. The new updateGoal waits for the full API round-trip before refreshing the UI, which can cause a noticeable lag when editing a goal's title or target.

Consider applying a similar optimistic strategy — update the in-memory goals array immediately, then reconcile with the API response:

func updateGoal(_ goal: Goal, title: String, currentValue: Double, targetValue: Double) async {
    log("Goals: Updating goal '\(goal.title)' -> title='\(title)', current=\(currentValue), target=\(targetValue)")

    // Optimistic update
    if let index = goals.firstIndex(where: { $0.id == goal.id }) {
        goals[index].title = title
        goals[index].currentValue = currentValue
        goals[index].targetValue = targetValue
    }

    do {
        let updated = try await APIClient.shared.updateGoal(
            goalId: goal.id,
            title: title,
            currentValue: currentValue,
            targetValue: targetValue
        )

        _ = try? await GoalStorage.shared.syncServerGoal(updated)
        goals = try await GoalStorage.shared.getLocalGoals()
        log("Goals: Updated goal '\(updated.title)' confirmed by API")
    } catch {
        logError("Failed to update goal", error: error)
        goals = (try? await GoalStorage.shared.getLocalGoals()) ?? goals
    }
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +172 to +173
_ = try? await GoalStorage.shared.syncServerGoal(updated)
goals = try await GoalStorage.shared.getLocalGoals()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stale local data when syncServerGoal fails silently

syncServerGoal is called with try?, so if the local SQLite write fails, the subsequent getLocalGoals() will return the old values — making the UI appear unchanged even though the API update succeeded. The user sees no error and has to re-open the goals list to observe the change is gone.

The same pattern exists in createGoal. If a silent local-sync failure is acceptable here, consider at least applying the server response directly to the in-memory array as a fallback:

_ = try? await GoalStorage.shared.syncServerGoal(updated)
// Refresh from local; fall back to injecting the API response directly
if let refreshed = try? await GoalStorage.shared.getLocalGoals(), !refreshed.isEmpty {
    goals = refreshed
} else if let index = goals.firstIndex(where: { $0.id == updated.id }) {
    goals[index] = updated
}

Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
## Summary\n- add a desktop API client method for full goal updates\n-
wire the goal edit sheet to save title, current value, and target
value\n- refresh cached/local goals after a successful update\n\n##
Testing\n- xcrun swift build -c debug --package-path Desktop\n- launched
Omi Dev.app and verified the desktop app started successfully\n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant