Post-merge release-safety review: fix silent note-save failure + hygiene#29
Merged
Conversation
…ygiene Multi-agent review (7 dimensions, adversarially verified) of the merged security/stability work found no P0s and two P1s, both fixed here: - DailyNoteStore.save no longer swallows persistence failures behind a false success signal: it returns Bool, mutates todayContent only after the write is decided, logs + captures failures to Sentry with standard classification tags, and NoteEditorSheet keeps the sheet open with a retry alert (swipe-dismiss blocked mid-save so typed text can't be destroyed by a failing in-flight save). - StoreResetTests now pins reset() for all six user-owned stores (DailyNoteStore and GroceryListStore were unpinned — the exact cross-user data-leak regression class PR #28 fixed). Tiny verified P2 hardening included: - SearchView post-log Undo surfaces delete failures (alert + Sentry) instead of silently leaving the entry logged. - FoodDetailView favorite toggle captures errors to Sentry (was the only unobserved user-initiated Supabase write). - .gitignore adds *.xcarchive / *.xcresult. - claude.yml pins actions/checkout to the v4.3.1 release SHA (secret in scope in that job). - ci.yml also runs on push to main so direct pushes can't bypass tests. - config.toml: akfit://auth-callback added to the redirect allow-list (exact entry for OAuth + ** wildcard for query-string recovery links, per GoTrue glob semantics) and documents the dashboard-managed production auth posture (email confirmations, leaked-password protection). - Stale pre-PR#28 reset() comments in four stores corrected; README and release checklist note that CLAUDE.md / docs/ui-reference are local-only. Verified: 119 tests in 18 suites pass on iPhone 17 Pro simulator; edited TOML/YAML parse clean; production Supabase advisors checked live (only pre-existing WARNs: pg_trgm in public, leaked-password protection off). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3aqVLXYVueZBeKd4U7imw
|
Updates to Preview Branch (audit/post-merge-fable5-review) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
There was a problem hiding this comment.
Pull request overview
This PR tightens release-safety and observability around user-owned data writes (notably daily notes), completes coverage for identity-transition store resets, and applies small CI/config/docs hygiene updates to reduce silent failures and improve production alignment.
Changes:
- Make daily note saves non-silent by returning save success/failure, only updating in-memory content after persistence is decided, and surfacing failures in the UI (with Sentry logging).
- Add missing store-reset tests for
DailyNoteStoreandGroceryListStore, and clarify reset-callsite documentation across stores. - Add error reporting/alerts for post-log Undo failures and favorite toggle failures; update CI triggers and repo ignore/docs/config notes.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| supabase/config.toml | Adds iOS deep-link redirect allow-list entries + clarifying production notes for auth URL configuration. |
| README.md | Documents that docs/ui-reference/ and CLAUDE.md are intentionally gitignored/local-only. |
| docs/release-checklist.md | Clarifies CLAUDE.md is gitignored/local-only and not present in a fresh clone. |
| AkFit/AkFitTests/StoreResetTests.swift | Adds reset contract tests for DailyNoteStore and GroceryListStore. |
| AkFit/AkFit/Views/Search/SearchView.swift | Surfaces Undo-delete failures via alert + Sentry capture instead of silent failure. |
| AkFit/AkFit/Views/FoodDetail/FoodDetailView.swift | Captures favorite-toggle errors to Sentry (store still handles optimistic revert). |
| AkFit/AkFit/Views/Dashboard/DashboardView.swift | Keeps note editor open on save failure; blocks interactive dismiss while saving. |
| AkFit/AkFit/Logging/GroceryListStore.swift | Updates reset documentation to reflect centralized identity-transition resets. |
| AkFit/AkFit/Logging/FoodLogStore.swift | Updates reset documentation to reflect centralized identity-transition resets. |
| AkFit/AkFit/Logging/FavoriteFoodStore.swift | Updates toggle behavior documentation to match optimistic revert + rethrow. |
| AkFit/AkFit/Logging/DailyNoteStore.swift | Changes save to return Bool, adds logging/Sentry reporting, and avoids optimistic in-memory updates on failed save. |
| AkFit/AkFit/Logging/BodyweightStore.swift | Updates reset documentation to reflect centralized identity-transition resets. |
| .gitignore | Ignores Xcode archives and result bundles (*.xcarchive, *.xcresult). |
| .github/workflows/claude.yml | Pins actions/checkout to a SHA due to secret scope in the job. |
| .github/workflows/ci.yml | Runs CI on push to main in addition to PRs/workflow_dispatch to prevent bypass. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+136
| guard canApplyUserOwnedState(for: userId) else { return true } | ||
| todayContent = content |
Owner
Author
There was a problem hiding this comment.
Good catch on the doc imprecision — the code behavior is intentional (verified in review round 2: a post-upsert identity change means the write DID persist for the original account, so returning true is correct and RootView resets the store's local state), but the comment overstated the false case. Clarified in 7449a9b: identity change BEFORE the write → false; after a successful upsert → true with the local mutation skipped.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- DailyNoteStore.save doc: identity change BEFORE the write returns false; identity change AFTER a successful upsert returns true (the write persisted for the original account; only the local mutation is skipped, RootView resets the store). Code behavior unchanged — the comment was imprecise (Copilot review). - Add the auth_code Sentry tag to the quick_log_undo and favorite_toggle captures, matching the richer grocery/daily-note tag idiom — session expiry is exactly the failure class these paths can hit (Greptile P2). 119/119 tests pass locally after the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3aqVLXYVueZBeKd4U7imw
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
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
Multi-agent release-safety review of the merged security/stability work (#28): 7 review dimensions, every finding adversarially verified, then a two-round final verification gate on the resulting diff (all lenses APPROVE, zero bugs). No P0s existed. This PR fixes the two verified P1s plus small, verified hygiene items — no refactors, no UI redesign, no new features.
P1 fixes
DailyNoteStore.savereturns success, only updates in-memory content after the write is decided, and reports failures to Sentry with the standard classification tags. The note editor keeps the sheet (and the typed text) open with a retry alert on failure, and blocks swipe-to-dismiss while a save is in flight. Previously a failed save dismissed the sheet, showed the note as saved for the rest of the session, and the text vanished on next launch.StoreResetTestspinsreset()for all six user-owned stores —DailyNoteStoreandGroceryListStorewere unpinned, which is the exact cross-user data-leak regression class Audit and fix AkFit security stability issues #28 fixed.Verified hygiene (tiny)
.gitignoreadds*.xcarchive/*.xcresult.claude.ymlpinsactions/checkoutto the v4.3.1 release SHA (a secret is in scope in that job).main, so direct pushes can't bypass the test suites.config.toml: addsakfit://auth-callback(exact, for Google OAuth) andakfit://auth-callback**(recovery links carry?flow=…&state=…, which exact entries don't match under GoTrue glob rules) to the redirect allow-list, and documents the dashboard-managed production auth posture.CLAUDE.mdanddocs/ui-reference/are local-only.Files / areas changed
15 files, +206/−28. App:
DailyNoteStore,DashboardView(NoteEditorSheet),SearchView,FoodDetailView, comment-only edits in four stores. Tests:StoreResetTests(+2 tests). Repo:.gitignore,ci.yml,claude.yml,supabase/config.toml,README.md,docs/release-checklist.md.Risk areas
pull_request+push) — intended, costs one extra run.Verification performed
actions/checkoutv4.3.1 tag via GitHub API;git ls-filesswept — no tracked artifacts/secrets.pg_trgmin public schema).Follow-ups outside this repo (dashboard): enable leaked-password protection; confirm the production redirect allow-list contains both
akfit://auth-callbackandakfit://auth-callback**.🤖 Generated with Claude Code
https://claude.ai/code/session_01X3aqVLXYVueZBeKd4U7imw
Greptile Summary
This PR fixes two verified bugs from a prior release-safety audit and adds matching hygiene:
DailyNoteStore.savenow returnsBoolso the note editor can keep the sheet open with a retry alert on failure (previously a failed save silently dismissed the sheet and the typed text was lost on next launch), andStoreResetTestsgains pinned tests forDailyNoteStoreandGroceryListStoreto close a cross-user data-leak coverage gap.save()returnsfalseon network/session failure,NoteEditorSheetblocks swipe-to-dismiss while in-flight (interactiveDismissDisabled), and shows a retry alert on failure;todayContentis only updated after a confirmed write.@Testcases exerciseDailyNoteStore.reset()(async/guest path, verifiestodayContentandisSavingcleared) andGroceryListStore.reset()(verifies all user-visible state), closing the regression class that Audit and fix AkFit security stability issues #28 introduced the centralized hook to prevent.SearchViewand favorite-toggle inFoodDetailViewnow catch errors, report to Sentry with the standard three-tag classification set, and surface alerts;claude.ymlpinsactions/checkoutto a SHA because a secret is in scope.Confidence Score: 5/5
Safe to merge — all changes are additive error-handling or corrections to previously silent failure paths, with no regressions identified.
Every behaviour change is a tightening of an existing contract: save returns false instead of silently succeeding, the editor keeps typed text instead of discarding it, and previously swallowed errors now reach Sentry and the user. The note-save flow is straightforward (local isSaving state mirrors the store, defer clears it, interactiveDismissDisabled guards the in-flight window). The two new test cases exercise the exact reset paths the prior audit flagged as uncovered. No new async hazards, no schema changes, no removed guards.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram actor User participant Sheet as NoteEditorSheet participant Store as DailyNoteStore participant Supabase User->>Sheet: tap Done Sheet->>Sheet: "isSaving = true" Sheet->>Store: save(content:userId:) async Bool Store->>Store: guard canApplyUserOwnedState alt Guest mode Store->>Store: saveDailyNote (UserDefaults) Store->>Store: "todayContent = content" Store-->>Sheet: return true else Authenticated Store->>Store: "isSaving = true" Store->>Supabase: requireAuthenticatedUserIDForWrite() Store->>Supabase: upsert daily_note alt Success Supabase-->>Store: 200 OK Store->>Store: "todayContent = content" Store-->>Sheet: return true Sheet->>Sheet: dismiss() else Failure Supabase-->>Store: error Store->>Store: log + Sentry.captureNonFatal Store-->>Sheet: return false Sheet->>Sheet: "showSaveError = true" Sheet-->>User: alert Couldnt save note end Store->>Store: "isSaving = false (defer)" end Sheet->>Sheet: "isSaving = false (defer)"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram actor User participant Sheet as NoteEditorSheet participant Store as DailyNoteStore participant Supabase User->>Sheet: tap Done Sheet->>Sheet: "isSaving = true" Sheet->>Store: save(content:userId:) async Bool Store->>Store: guard canApplyUserOwnedState alt Guest mode Store->>Store: saveDailyNote (UserDefaults) Store->>Store: "todayContent = content" Store-->>Sheet: return true else Authenticated Store->>Store: "isSaving = true" Store->>Supabase: requireAuthenticatedUserIDForWrite() Store->>Supabase: upsert daily_note alt Success Supabase-->>Store: 200 OK Store->>Store: "todayContent = content" Store-->>Sheet: return true Sheet->>Sheet: dismiss() else Failure Supabase-->>Store: error Store->>Store: log + Sentry.captureNonFatal Store-->>Sheet: return false Sheet->>Sheet: "showSaveError = true" Sheet-->>User: alert Couldnt save note end Store->>Store: "isSaving = false (defer)" end Sheet->>Sheet: "isSaving = false (defer)"Reviews (2): Last reviewed commit: "Address PR review: clarify save() identi..." | Re-trigger Greptile