fix: unified profile - wire repository reads, self-publish, and self-profile freshness#1130
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
cameronvoell
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| func bind(session: any ProfilePublishSession) async { | ||
| await publisher.attach(session: session) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 High Repository/ProfilesRepository.swift:244
publishMyProfile routes every save through the durable publisher, but a name-only call (displayName set, avatarBytes nil) can silently cancel a pending avatar upload. enqueueJob calls supersedeOlderThan for the conversation, which removes the earlier avatar job before it drains. processNameOnlyJob then re-sends the avatar from profileStore — the last published avatar — not the newer source image in publishStore. So if a user saves a new photo and then quickly saves just their name before the upload finishes, the avatar job is superseded and the photo change is lost.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 244:
`publishMyProfile` routes every save through the durable publisher, but a name-only call (`displayName` set, `avatarBytes` nil) can silently cancel a pending avatar upload. `enqueueJob` calls `supersedeOlderThan` for the conversation, which removes the earlier avatar job before it drains. `processNameOnlyJob` then re-sends the avatar from `profileStore` — the last *published* avatar — not the newer source image in `publishStore`. So if a user saves a new photo and then quickly saves just their name before the upload finishes, the avatar job is superseded and the photo change is lost.
| .publisher(in: databaseReader, scheduling: .immediate) | ||
| .catch { _ in Just(nil) } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
🟡 Medium Repository/ProfilesRepository.swift:95
fetchSelfProfile returns nil when no DBSelfProfile row exists, even if DBProfileAvatar rows for the current user's inbox are present. publishMyProfile(displayName: nil, avatarBytes: ...) only writes avatar slots and never creates a DBSelfProfile row, so selfProfilePublisher() keeps emitting nil after the user publishes a profile photo. Any UI using this reactive read path will not see the self avatar until a name or metadata is also saved. Consider falling back to a UnifiedProfile built from the avatar rows when no DBSelfProfile row exists, or creating a default DBSelfProfile row on first publish.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 95:
`fetchSelfProfile` returns `nil` when no `DBSelfProfile` row exists, even if `DBProfileAvatar` rows for the current user's inbox are present. `publishMyProfile(displayName: nil, avatarBytes: ...)` only writes avatar slots and never creates a `DBSelfProfile` row, so `selfProfilePublisher()` keeps emitting `nil` after the user publishes a profile photo. Any UI using this reactive read path will not see the self avatar until a name or metadata is also saved. Consider falling back to a `UnifiedProfile` built from the avatar rows when no `DBSelfProfile` row exists, or creating a default `DBSelfProfile` row on first publish.
There was a problem hiding this comment.
🟠 High
startProfileMirroring() calls sharedProfilesRepository.bind(session:), which resumes pending DBProfilePublishJob rows from the durable publish store. Because DBProfilePublishJob is not keyed by inbox id, pending jobs left behind by a previous account survive an account reset and are replayed against the new account's session after bind. A stale name-only job retries on its old conversationId, producing perpetual backoff failures, and if a stale DBSelfProfile row also remains, it can send the previous account's self name on the new account. Consider clearing profilePublishJob and profileAvatarSource in the account-reset cleanup path before bind resumes them.
Also found in 1 other location(s)
ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift:108
fetchSelfProfilereads an unfilteredDBSelfProfilerow (DBSelfProfile.fetchOne(db)) and then hydrates avatars for that row's inbox.SessionManager.wipeResidualInboxRows()does not clear the new canonical profile tables, so after delete-all-data / pairing-adoption flows the database can still contain the previous account'sselfProfilerow. Once this publisher is used, the next account can see the old account's self name/avatar because the read path never scopes the row to the current inbox.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Messaging/MessagingService.swift around line 209:
`startProfileMirroring()` calls `sharedProfilesRepository.bind(session:)`, which resumes pending `DBProfilePublishJob` rows from the durable publish store. Because `DBProfilePublishJob` is not keyed by inbox id, pending jobs left behind by a previous account survive an account reset and are replayed against the new account's session after `bind`. A stale name-only job retries on its old `conversationId`, producing perpetual backoff failures, and if a stale `DBSelfProfile` row also remains, it can send the previous account's self name on the new account. Consider clearing `profilePublishJob` and `profileAvatarSource` in the account-reset cleanup path before `bind` resumes them.
Also found in 1 other location(s):
- ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift:108 -- `fetchSelfProfile` reads an unfiltered `DBSelfProfile` row (`DBSelfProfile.fetchOne(db)`) and then hydrates avatars for that row's inbox. `SessionManager.wipeResidualInboxRows()` does not clear the new canonical profile tables, so after delete-all-data / pairing-adoption flows the database can still contain the previous account's `selfProfile` row. Once this publisher is used, the next account can see the old account's self name/avatar because the read path never scopes the row to the current inbox.
| private let profileStore: any ProfileStoreProtocol | ||
| private let selfProfileStore: any SelfProfileStoreProtocol | ||
| private let selfInboxId: String | ||
| private let selfInboxIdProvider: @Sendable () async -> String? |
There was a problem hiding this comment.
🟡 Medium Repository/ProfilePublisher.swift:33
selfInboxIdProvider returns nil before the inbox is ready, so publish(...) and publishConversation(...) return success without enqueuing any jobs — the profile update is silently dropped with no error. Consider detecting the nil case and deferring or surfacing an error so the caller is not told the publish succeeded.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilePublisher.swift around line 33:
`selfInboxIdProvider` returns `nil` before the inbox is ready, so `publish(...)` and `publishConversation(...)` return success without enqueuing any jobs — the profile update is silently dropped with no error. Consider detecting the `nil` case and deferring or surfacing an error so the caller is not told the publish succeeded.
170950b to
69b95b5
Compare
cameronvoell
force-pushed
the
cv/unified-profile-repository-read-write-live
branch
from
564cbd8 to
4fa3f85
Compare
| if sawSelf, try await selfProfileStore.load() == nil { | ||
| /// Upserts the self row from the legacy-derived name/metadata. A fresh seed | ||
| /// uses the floor timestamp (like everything else here); an update reflects a | ||
| /// legacy rename with a real timestamp. Legacy values win when present but a |
There was a problem hiding this comment.
🟠 High Repository/ProfileBackfill.swift:90
mirrorSelf overwrites an existing DBSelfProfile with values from the legacy DBMemberProfile row whenever the legacy row is non-blank. When the user edits their own profile via publishMyProfile/updateSelfProfile, the new name/metadata is saved only to selfProfileStore, but the legacy memberProfile row still holds the stale old value. On the next startup backfill (or any ProfileMemberMirror run that includes the self row), mergedName/mergedMetadata are recomputed from that stale legacy row and saved back over the user's newer self profile, silently reverting the edit.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileBackfill.swift around line 90:
`mirrorSelf` overwrites an existing `DBSelfProfile` with values from the legacy `DBMemberProfile` row whenever the legacy row is non-blank. When the user edits their own profile via `publishMyProfile`/`updateSelfProfile`, the new name/metadata is saved only to `selfProfileStore`, but the legacy `memberProfile` row still holds the stale old value. On the next startup backfill (or any `ProfileMemberMirror` run that includes the self row), `mergedName`/`mergedMetadata` are recomputed from that stale legacy row and saved back over the user's newer self profile, silently reverting the edit.
graphite-app
Bot
force-pushed
the
cv/profile-table-activate
branch
from
69b95b5 to
a9adc0d
Compare
graphite-app
Bot
force-pushed
the
cv/unified-profile-repository-read-write-live
branch
from
4fa3f85 to
24d5edb
Compare
| .publisher(in: databaseReader, scheduling: .immediate) | ||
| .catch { _ in Just(nil) } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
🟠 High Repository/ProfilesRepository.swift:95
fetchSelfProfile calls DBSelfProfile.fetchOne(db) with no inboxId filter, so when multiple DBSelfProfile rows exist (e.g. after an account reset or pairing wipe that does not clear selfProfile), selfProfilePublisher() emits whichever row GRDB returns first rather than the current user's row. This surfaces a stale name and avatar for the current user. Consider filtering by the resolved selfInboxId, or document why only one row is expected to exist.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 95:
`fetchSelfProfile` calls `DBSelfProfile.fetchOne(db)` with no `inboxId` filter, so when multiple `DBSelfProfile` rows exist (e.g. after an account reset or pairing wipe that does not clear `selfProfile`), `selfProfilePublisher()` emits whichever row GRDB returns first rather than the current user's row. This surfaces a stale name and avatar for the current user. Consider filtering by the resolved `selfInboxId`, or document why only one row is expected to exist.
| @@ -78,12 +82,29 @@ struct ProfileBackfill { | |||
| } | |||
There was a problem hiding this comment.
🟡 Medium Repository/ProfileBackfill.swift:82
mirrorSelf computes mergedMetadata as metadata ?? existing?.metadata, so a legacy memberProfile change that sets metadata to nil can never clear the canonical DBSelfProfile. Every re-run of ProfileBackfill preserves the old metadata instead of reflecting the clear, and downstream self-profile publishers keep serving stale data. Consider distinguishing "no metadata in this row" from "metadata was explicitly cleared" so the nil value is honored when the legacy row is the current author.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileBackfill.swift around line 82:
`mirrorSelf` computes `mergedMetadata` as `metadata ?? existing?.metadata`, so a legacy `memberProfile` change that sets metadata to `nil` can never clear the canonical `DBSelfProfile`. Every re-run of `ProfileBackfill` preserves the old metadata instead of reflecting the clear, and downstream self-profile publishers keep serving stale data. Consider distinguishing \"no metadata in this row\" from \"metadata was explicitly cleared\" so the `nil` value is honored when the legacy row is the current author.
cameronvoell
force-pushed
the
cv/unified-profile-repository-read-write-live
branch
from
24d5edb to
aa29d71
Compare
cameronvoell
force-pushed
the
cv/profile-table-activate
branch
from
a9adc0d to
1cc4b0f
Compare
| .publisher(in: databaseReader, scheduling: .immediate) | ||
| .catch { _ in Just(nil) } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
🟡 Medium Repository/ProfilesRepository.swift:95
fetchSelfProfile builds the self profile's avatars only from DBProfileAvatar conversation slots. A freshly published avatar is first persisted in DBProfileAvatarSource, and DBProfileAvatar rows are only created after per-conversation publishes succeed. When the user has no conversations or all publishes are still pending/failed, selfProfilePublisher keeps emitting a profile with no avatar even though the avatar was accepted locally — the locally-stored avatar from DBProfileAvatarSource is never consulted. Consider also reading DBProfileAvatarSource in fetchSelfProfile so the pending avatar is reflected before per-conversation slots exist.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 95:
`fetchSelfProfile` builds the self profile's avatars only from `DBProfileAvatar` conversation slots. A freshly published avatar is first persisted in `DBProfileAvatarSource`, and `DBProfileAvatar` rows are only created after per-conversation publishes succeed. When the user has no conversations or all publishes are still pending/failed, `selfProfilePublisher` keeps emitting a profile with no avatar even though the avatar was accepted locally — the locally-stored avatar from `DBProfileAvatarSource` is never consulted. Consider also reading `DBProfileAvatarSource` in `fetchSelfProfile` so the pending avatar is reflected before per-conversation slots exist.
|
|
||
| // MARK: - Reactive reads | ||
|
|
||
| /// Reactive identity for one inbox, hydrated fresh from the stores via |
There was a problem hiding this comment.
🟡 Medium Repository/ProfilesRepository.swift:61
profilePublisher, profilesPublisher, and selfProfilePublisher use .catch { _ in Just(...) }, which replaces the failed ValueObservation with a one-shot Just publisher that completes immediately. After any database or decoding error, subscribers stop receiving updates permanently and profile rendering stays stale until they resubscribe or restart the app. Consider using .tryCatch with retry/recovery that preserves the observation, or a different error-handling strategy that keeps the subscription alive.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 61:
`profilePublisher`, `profilesPublisher`, and `selfProfilePublisher` use `.catch { _ in Just(...) }`, which replaces the failed `ValueObservation` with a one-shot `Just` publisher that completes immediately. After any database or decoding error, subscribers stop receiving updates permanently and profile rendering stays stale until they resubscribe or restart the app. Consider using `.tryCatch` with retry/recovery that preserves the observation, or a different error-handling strategy that keeps the subscription alive.
|
folded into #1126 |
1 participant
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Wire repository reads, self-publish, and self-profile freshness into unified profile layer
ProfilesRepositorynow accepts apublishStoreanddatabaseReader, owns aProfilePublisher, and exposes reactive GRDBValueObservation-backed publishers (profilePublisher,profilesPublisher,selfProfilePublisher) for profile reads.ProfileBackfillupserts the self profile on each mirror run, reflecting renames without erasing existing non-blank names; skips writes when nothing changed.MessagingServicebinds aMessagingProfilePublishSessionand starts aProfileShadowComparatoron mirror start, and unbinds/stops them on mirror stop.ProfileShadowComparatoractor periodically compares identity and avatar data between the new profile tables and the legacy contacts table, logging discrepancies without mutating data.ProfilePublishernow resolves self inbox id lazily via an async provider closure and caches it; publish and drain operations no-op if the id is unavailable.📊 Macroscope summarized aa29d71. 6 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
(Automatic summaries will resume when PR exits draft mode or review begins).🗂️ Filtered Issues
No issues evaluated.