diff --git a/agents-docs/LESSONS.md b/agents-docs/LESSONS.md index 033f359..db5b996 100644 --- a/agents-docs/LESSONS.md +++ b/agents-docs/LESSONS.md @@ -25,6 +25,13 @@ Durable rules for AI agents working on this project. Read this file at session s ## Lessons +### "Shared from your device" must gate on local bytes, not uploader user id [attachments] [multi-device] + +- **Trigger:** a second device of the same user showed "Shared from your device" and hid the download affordance for a file uploaded from another device — `isUploader(attachment)` returned `uploaderPeerId === currentUserId`, but `uploaderPeerId` is the **user** id (set to `currentUser.id` in `publishAttachments`), so it is true on every device of the uploader, including ones that only synced metadata. +- **Rule:** key the sharing/ownership UI off whether *this device* holds the bytes, not who uploaded it — use `isSharingFromThisDevice(attachment, currentUserId)` (= `isUploaderUser && deviceHasLocalCopy`) from `attachment-sharing.rules.ts`; `deviceHasLocalCopy` = `available` + blob `objectUrl`, or a non-empty `savedPath`/`filePath` (synced metadata strips local paths, so it correctly reads as "no copy"). +- **Why:** same-user devices do **not** P2P with each other and sync only via `account_sync` (which strips `filePath`/`savedPath`), so the second device legitimately has no bytes; claiming ownership blocked the only path to view/download. For the regression to even be reachable in e2e, `account_sync`'s `chat-sync-batch` had to start carrying the `attachments` map (it previously dropped attachment metadata entirely) via `pushSavedRoomMessagesViaAccountSync(..., loadAttachmentMetas)`. +- **Example:** unit `attachment-sharing.rules.spec.ts` (`isSharingFromThisDevice({uploaderPeerId:'u1', available:false}, 'u1') === false`); e2e `e2e/tests/chat/multi-device-attachment-sharing.spec.ts` uploads on device A then logs device B in afterward so the `account_sync_peer_online` full-state push delivers the attachment, then asserts device B shows a Request button and **no** "Shared from your device". + ### Generate Android brand icons from the source mark; guard against stock Capacitor placeholders [mobile] [android] [assets] - **Trigger:** the Android app shipped the default Ionic/Capacitor launcher icon (and a white adaptive background) because no brand icon was ever generated into `toju-app/android/app/src/main/res/`. diff --git a/agents-docs/features/authentication.md b/agents-docs/features/authentication.md index 69b3fa8..3bb3021 100644 --- a/agents-docs/features/authentication.md +++ b/agents-docs/features/authentication.md @@ -70,7 +70,7 @@ When the same account is logged in on multiple devices, account-owned data is ke | Data | Mechanism | |---|---| | Server chat messages (live) | `chat_message` signaling relay (connection-scoped broadcast) **plus** `account_sync` `chat-message` / `message-revision` to sibling devices | -| Server chat messages (catch-up) | `account_sync` `chat-sync-batch` pushed when a sibling device comes online (`account_sync_peer_online`) | +| Server chat messages (catch-up) | `account_sync` `chat-sync-batch` pushed when a sibling device comes online (`account_sync_peer_online`); each batch carries its messages' **attachment metadata** (`attachments` map, local paths stripped) so sibling devices learn about synced attachments — they are then requestable/downloadable but never marked "Shared from your device" unless the bytes are local | | Voice / typing | Existing `voice_state` / `user_typing` relays | | Saved servers (join/leave) | `account_sync` payload `saved-room-sync` / `saved-room-remove` | | Profile avatar + card text | `account_sync` `user-avatar-full` + `user-avatar-chunk` | diff --git a/e2e/tests/chat/multi-device-attachment-sharing.spec.ts b/e2e/tests/chat/multi-device-attachment-sharing.spec.ts new file mode 100644 index 0000000..fc299df --- /dev/null +++ b/e2e/tests/chat/multi-device-attachment-sharing.spec.ts @@ -0,0 +1,96 @@ +import { test, expect } from '../../fixtures/multi-client'; +import { RegisterPage } from '../../pages/register.page'; +import { ServerSearchPage } from '../../pages/server-search.page'; +import { ChatMessagesPage, type ChatDropFilePayload } from '../../pages/chat-messages.page'; +import { + MULTI_DEVICE_PASSWORD, + loginSecondDeviceIntoServer, + uniqueMultiDeviceName +} from '../../helpers/multi-device-session'; + +const SHARED_FROM_DEVICE_TEXT = 'Shared from your device'; + +test.describe('Multi-device attachment sharing', () => { + test.describe.configure({ timeout: 300_000, retries: 1 }); + + test('only the uploading device claims "Shared from your device"; the second same-user device can request it', async ({ + createClient + }) => { + const suffix = uniqueMultiDeviceName('attach-share'); + const credentials = { + username: `share_${suffix}`, + displayName: 'Multi Device User', + password: MULTI_DEVICE_PASSWORD + }; + const serverName = `Attachment Sharing ${suffix}`; + const fileName = `${suffix}-handoff.bin`; + const caption = `Uploaded from device A ${suffix}`; + const fileAttachment = createBinaryFilePayload(fileName, 'application/octet-stream', `binary-body-${suffix}`); + const clientA = await createClient(); + const messagesA = new ChatMessagesPage(clientA.page); + + await test.step('device A registers, creates a server, and uploads a generic file', async () => { + const registerPage = new RegisterPage(clientA.page); + + await registerPage.goto(); + await registerPage.register(credentials.username, credentials.displayName, credentials.password); + await expect(clientA.page).toHaveURL(/\/dashboard/, { timeout: 15_000 }); + + const search = new ServerSearchPage(clientA.page); + + await search.createServer(serverName, { description: 'Multi-device attachment sharing regression coverage' }); + await expect(clientA.page).toHaveURL(/\/room\//, { timeout: 15_000 }); + + await messagesA.waitForReady(); + await messagesA.attachFiles([fileAttachment]); + await messagesA.sendMessage(caption); + await expect(messagesA.getMessageItemByText(caption)).toBeVisible({ timeout: 30_000 }); + }); + + await test.step('device A (the uploader) shows "Shared from your device"', async () => { + const bubbleA = messagesA.getMessageItemByText(caption); + + await expect(bubbleA.getByText(fileName, { exact: false })).toBeVisible({ timeout: 20_000 }); + await expect(bubbleA.getByText(SHARED_FROM_DEVICE_TEXT, { exact: false })).toBeVisible({ timeout: 20_000 }); + }); + + const clientB = await createClient(); + const messagesB = new ChatMessagesPage(clientB.page); + + await test.step('device B (same user) logs into the same server after the upload', async () => { + await loginSecondDeviceIntoServer(clientB.page, credentials, serverName); + // Keep device A active so it answers device B's account_sync_peer_online push. + await clientA.page.bringToFront(); + await messagesA.waitForReady(); + await clientB.page.bringToFront(); + await messagesB.waitForReady(); + }); + + await test.step('device B receives the message and its attachment via same-user account sync', async () => { + await expect(messagesB.getMessageItemByText(caption)).toBeVisible({ timeout: 90_000 }); + await expect(messagesB.getMessageItemByText(caption).getByText(fileName, { exact: false })) + .toBeVisible({ timeout: 90_000 }); + }); + + await test.step('device B does NOT claim to share it and can request/download the file', async () => { + const bubbleB = messagesB.getMessageItemByText(caption); + + // The regression: device B used to render "Shared from your device" and hide the + // download affordance because the synced metadata carried the uploader's user id. + await expect(bubbleB.getByText(SHARED_FROM_DEVICE_TEXT, { exact: false })).toHaveCount(0); + + // Device B must instead be able to fetch the file as any recipient would. + const getButton = bubbleB.getByRole('button', { name: /request|download/i }); + + await expect(getButton.first()).toBeVisible({ timeout: 20_000 }); + }); + }); +}); + +function createBinaryFilePayload(name: string, mimeType: string, content: string): ChatDropFilePayload { + return { + name, + mimeType, + base64: Buffer.from(content, 'utf8').toString('base64') + }; +} diff --git a/toju-app/src/app/domains/attachment/README.md b/toju-app/src/app/domains/attachment/README.md index dd54db4..57b6d25 100644 --- a/toju-app/src/app/domains/attachment/README.md +++ b/toju-app/src/app/domains/attachment/README.md @@ -147,6 +147,18 @@ Browser chat views render audio/video larger than 50 MB with the same generic fi An optional experimental VLC.js adapter can be enabled from General settings. When enabled, unsupported downloaded audio/video files show a manual Play action that lazy-loads `/vlcjs/metoyou-vlc-player.js`. The runtime is intentionally isolated in the experimental media domain and is not part of the default attachment path. +## Ownership and the "Shared from your device" label + +`uploaderPeerId` is the **user** id of whoever uploaded the file, not a per-device id. It is intentionally stable across a user's devices so an uploader can recognise their own attachments after sync. Because of that, "did *this* device upload it?" and "does *this* device hold the bytes?" are two different questions, and the UI must key the *sharing* affordance off the latter. + +`attachment-sharing.rules.ts` makes this explicit: + +- `isUploaderUser(attachment, currentUserId)` — the current user is the uploader (same user, any device). +- `deviceHasLocalCopy(attachment)` — this device physically holds the bytes (`available` + a blob `objectUrl`, or a non-empty `savedPath`/`filePath`). Synced metadata alone does not count, because P2P/account sync strips local paths. +- `isSharingFromThisDevice(attachment, currentUserId)` — `isUploaderUser && deviceHasLocalCopy`. Only this returns the "Shared from your device" state. + +The chat message item renders "Shared from your device" (and hides the request/download affordance) **only** when `isSharingFromThisDevice` is true. A second device of the same user that merely synced the message metadata is the uploader-user but holds no local copy, so it falls back to the normal recipient flow (request/download) instead of falsely claiming ownership and blocking the file (regression: the old check used `uploaderPeerId === currentUserId` and so claimed ownership on every device of the uploader). The transfer service uses the same rule to decide whether a no-peers failure should read "your original upload is missing" (sharing device) or "no connected peers" (any other device). + ## Persistence Attachment file persistence is platform-agnostic. `AttachmentStorageService` owns the `server//` and `direct-messages/...` path layout and delegates the raw byte IO to a pluggable `AttachmentFileStore` chosen by `PlatformService` (mirroring how `DatabaseService` picks a DB backend): diff --git a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts index 2345766..670aa87 100644 --- a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts +++ b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts @@ -8,6 +8,7 @@ import { selectCurrentUserId } from '../../../../store/users/users.selectors'; import { AttachmentStorageService } from '../../infrastructure/services/attachment-storage.service'; import { MAX_AUTO_SAVE_SIZE_BYTES } from '../../domain/constants/attachment.constants'; import { isImageAttachment, resolvePublishAttachmentIsImage } from '../../domain/logic/attachment-image.rules'; +import { isSharingFromThisDevice } from '../../domain/logic/attachment-sharing.rules'; import { shouldCopyUploaderMediaToAppData, shouldPersistDownloadedAttachment } from '../../domain/logic/attachment.logic'; import type { Attachment, AttachmentMeta } from '../../domain/models/attachment.model'; import { @@ -170,13 +171,15 @@ export class AttachmentTransferService { const connectedPeers = this.webrtc.getConnectedPeers(); const currentUserId = await this.resolveCurrentUserId(); - const isUploader = !!attachment.uploaderPeerId && - !!currentUserId && - attachment.uploaderPeerId === currentUserId; + // Only the device that actually still holds the original bytes should report a + // missing local upload. A second device of the same user that merely synced the + // metadata is not the sharing device, so it falls back to the regular peer-request + // flow (and the "no connected peers" error when offline) like any other recipient. + const sharingFromThisDevice = isSharingFromThisDevice(attachment, currentUserId); if (connectedPeers.length === 0) { this.runtimeStore.deletePendingRequest(requestKey); - attachment.requestError = isUploader + attachment.requestError = sharingFromThisDevice ? this.appI18n.instant(UPLOADER_LOCAL_FILE_MISSING_ERROR_KEY) : this.appI18n.instant(NO_CONNECTED_PEERS_REQUEST_ERROR_KEY); diff --git a/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.spec.ts b/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.spec.ts new file mode 100644 index 0000000..814cf8d --- /dev/null +++ b/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.spec.ts @@ -0,0 +1,69 @@ +import { + deviceHasLocalCopy, + isSharingFromThisDevice, + isUploaderUser +} from './attachment-sharing.rules'; + +describe('attachment sharing rules', () => { + describe('isUploaderUser', () => { + it('is true when the attachment uploader matches the current user', () => { + expect(isUploaderUser({ uploaderPeerId: 'user-1' }, 'user-1')).toBe(true); + }); + + it('is false when the uploader is a different user', () => { + expect(isUploaderUser({ uploaderPeerId: 'user-2' }, 'user-1')).toBe(false); + }); + + it('is false when either id is missing', () => { + expect(isUploaderUser({ uploaderPeerId: undefined }, 'user-1')).toBe(false); + expect(isUploaderUser({ uploaderPeerId: 'user-1' }, null)).toBe(false); + expect(isUploaderUser({ uploaderPeerId: 'user-1' }, '')).toBe(false); + }); + }); + + describe('deviceHasLocalCopy', () => { + it('is true when an available blob object url is present', () => { + expect(deviceHasLocalCopy({ available: true, objectUrl: 'blob:abc' })).toBe(true); + }); + + it('is true when a savedPath or filePath is present', () => { + expect(deviceHasLocalCopy({ available: false, savedPath: '/appdata/file.bin' })).toBe(true); + expect(deviceHasLocalCopy({ available: false, filePath: '/home/me/file.bin' })).toBe(true); + }); + + it('is false when only metadata exists (no bytes, no paths)', () => { + expect(deviceHasLocalCopy({ available: false })).toBe(false); + expect(deviceHasLocalCopy({ available: false, savedPath: ' ', filePath: '' })).toBe(false); + }); + + it('is false when marked available but no object url is present yet', () => { + expect(deviceHasLocalCopy({ available: true })).toBe(false); + }); + }); + + describe('isSharingFromThisDevice', () => { + it('is true for the uploader device that still holds the file locally', () => { + expect( + isSharingFromThisDevice({ uploaderPeerId: 'user-1', available: true, objectUrl: 'blob:abc' }, 'user-1') + ).toBe(true); + + expect( + isSharingFromThisDevice({ uploaderPeerId: 'user-1', available: false, savedPath: '/appdata/file.bin' }, 'user-1') + ).toBe(true); + }); + + it('is false on a second device of the same user that only synced metadata', () => { + // This is the regression: the user uploaded from another device, so the metadata + // carries uploaderPeerId === currentUserId, but this device holds no local bytes. + expect( + isSharingFromThisDevice({ uploaderPeerId: 'user-1', available: false }, 'user-1') + ).toBe(false); + }); + + it('is false for a different user even if they downloaded the file locally', () => { + expect( + isSharingFromThisDevice({ uploaderPeerId: 'user-1', available: true, objectUrl: 'blob:abc' }, 'user-2') + ).toBe(false); + }); + }); +}); diff --git a/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.ts b/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.ts new file mode 100644 index 0000000..3b30c4c --- /dev/null +++ b/toju-app/src/app/domains/attachment/domain/logic/attachment-sharing.rules.ts @@ -0,0 +1,40 @@ +import type { Attachment } from '../models/attachment.model'; + +/** True when the current user is the one who originally uploaded this attachment. */ +export function isUploaderUser( + attachment: Pick, + currentUserId: string | null | undefined +): boolean { + return !!attachment.uploaderPeerId && !!currentUserId && attachment.uploaderPeerId === currentUserId; +} + +/** + * True when this specific device physically holds the file bytes - either hydrated + * in memory (available blob object url) or persisted to disk / present at its original + * upload path. Synced metadata alone (no bytes, no local paths) does not count. + */ +export function deviceHasLocalCopy( + attachment: Pick +): boolean { + return (attachment.available === true && hasNonEmptyString(attachment.objectUrl)) || + hasNonEmptyString(attachment.savedPath) || + hasNonEmptyString(attachment.filePath); +} + +/** + * True only when the current user uploaded the file AND this device still holds a local + * copy to serve. A second device of the same user that only synced the message metadata + * is the uploader-user but has no local bytes, so it must behave like any other peer + * (request/download from peers) instead of claiming "Shared from your device" and hiding + * the download affordance. + */ +export function isSharingFromThisDevice( + attachment: Pick, + currentUserId: string | null | undefined +): boolean { + return isUploaderUser(attachment, currentUserId) && deviceHasLocalCopy(attachment); +} + +function hasNonEmptyString(value: string | null | undefined): boolean { + return typeof value === 'string' && value.trim().length > 0; +} diff --git a/toju-app/src/app/domains/attachment/index.ts b/toju-app/src/app/domains/attachment/index.ts index 2466f1f..e85ee9f 100644 --- a/toju-app/src/app/domains/attachment/index.ts +++ b/toju-app/src/app/domains/attachment/index.ts @@ -1,4 +1,5 @@ export * from './application/facades/attachment.facade'; export * from './domain/constants/attachment.constants'; +export * from './domain/logic/attachment-sharing.rules'; export * from './domain/logic/local-file-path.rules'; export * from './domain/models/attachment.model'; diff --git a/toju-app/src/app/domains/chat/feature/chat-messages/components/message-item/chat-message-item.component.html b/toju-app/src/app/domains/chat/feature/chat-messages/components/message-item/chat-message-item.component.html index ec1c228..2a4809e 100644 --- a/toju-app/src/app/domains/chat/feature/chat-messages/components/message-item/chat-message-item.component.html +++ b/toju-app/src/app/domains/chat/feature/chat-messages/components/message-item/chat-message-item.component.html @@ -413,7 +413,7 @@
{{ formatBytes(att.size) }}
- @if (!att.isUploader) { + @if (!att.isSharingFromThisDevice) { @if (!att.available) {
{ it('relays saved room messages in chat-sync-batch chunks to sibling devices', async () => { const relayAccountSync = vi.fn(); @@ -34,7 +46,8 @@ describe('pushSavedRoomMessagesViaAccountSync', () => { await pushSavedRoomMessagesViaAccountSync( { relayAccountSync }, loadRoomMessages, - ['room-a', 'room-b'] + ['room-a', 'room-b'], + () => ({}) ); expect(loadRoomMessages).toHaveBeenCalledWith('room-a'); @@ -51,4 +64,48 @@ describe('pushSavedRoomMessagesViaAccountSync', () => { messages: roomB }); }); + + it('includes attachment metadata so sibling devices learn about synced attachments', async () => { + const relayAccountSync = vi.fn(); + const roomA = [createMessage('m1', 'room-a'), createMessage('m2', 'room-a')]; + const attachmentForM1 = createAttachmentMeta('att-1', 'm1'); + const loadRoomMessages = vi.fn(async () => roomA); + const loadAttachmentMetas = vi.fn((messageIds: string[]) => + messageIds.includes('m1') ? { m1: [attachmentForM1] } : {} + ); + + await pushSavedRoomMessagesViaAccountSync( + { relayAccountSync }, + loadRoomMessages, + ['room-a'], + loadAttachmentMetas + ); + + expect(loadAttachmentMetas).toHaveBeenCalledWith(['m1', 'm2']); + expect(relayAccountSync).toHaveBeenCalledWith({ + type: 'chat-sync-batch', + roomId: 'room-a', + messages: roomA, + attachments: { m1: [attachmentForM1] } + }); + }); + + it('omits the attachments field when a chunk has no attachments', async () => { + const relayAccountSync = vi.fn(); + const roomA = [createMessage('m1', 'room-a')]; + const loadRoomMessages = vi.fn(async () => roomA); + + await pushSavedRoomMessagesViaAccountSync( + { relayAccountSync }, + loadRoomMessages, + ['room-a'], + () => ({}) + ); + + expect(relayAccountSync).toHaveBeenCalledWith({ + type: 'chat-sync-batch', + roomId: 'room-a', + messages: roomA + }); + }); }); diff --git a/toju-app/src/app/infrastructure/realtime/account-sync/account-sync-chat.helper.ts b/toju-app/src/app/infrastructure/realtime/account-sync/account-sync-chat.helper.ts index 10b8085..69ddd92 100644 --- a/toju-app/src/app/infrastructure/realtime/account-sync/account-sync-chat.helper.ts +++ b/toju-app/src/app/infrastructure/realtime/account-sync/account-sync-chat.helper.ts @@ -1,4 +1,8 @@ -import type { Message } from '../../../shared-kernel'; +import type { + Message, + ChatAttachmentMeta, + ChatEvent +} from '../../../shared-kernel'; import type { RealtimeSessionFacade } from '../../../core/realtime'; import { CHUNK_SIZE, @@ -6,22 +10,50 @@ import { chunkArray } from '../../../store/messages/messages.helpers'; +type AttachmentMetaMap = Record; + export async function pushSavedRoomMessagesViaAccountSync( webrtc: Pick, loadRoomMessages: (roomId: string) => Promise, - roomIds: readonly string[] + roomIds: readonly string[], + loadAttachmentMetas: (messageIds: string[]) => AttachmentMetaMap ): Promise { for (const roomId of roomIds) { const messages = await loadRoomMessages(roomId); for (const chunk of chunkArray(messages, CHUNK_SIZE)) { - webrtc.relayAccountSync({ + const chunkAttachments = collectChunkAttachments(chunk, loadAttachmentMetas); + const batch: ChatEvent = { type: 'chat-sync-batch', roomId, messages: chunk - }); + }; + + if (Object.keys(chunkAttachments).length > 0) { + batch.attachments = chunkAttachments; + } + + webrtc.relayAccountSync(batch); } } } +function collectChunkAttachments( + chunk: Message[], + loadAttachmentMetas: (messageIds: string[]) => AttachmentMetaMap +): AttachmentMetaMap { + const attachmentMetas = loadAttachmentMetas(chunk.map((message) => message.id)); + const chunkAttachments: AttachmentMetaMap = {}; + + for (const message of chunk) { + const metas = attachmentMetas[message.id]; + + if (metas && metas.length > 0) { + chunkAttachments[message.id] = metas; + } + } + + return chunkAttachments; +} + export const ACCOUNT_SYNC_MESSAGE_LIMIT = FULL_SYNC_LIMIT; diff --git a/toju-app/src/app/infrastructure/realtime/account-sync/account-sync.effects.ts b/toju-app/src/app/infrastructure/realtime/account-sync/account-sync.effects.ts index 0da5d83..c6a2ff9 100644 --- a/toju-app/src/app/infrastructure/realtime/account-sync/account-sync.effects.ts +++ b/toju-app/src/app/infrastructure/realtime/account-sync/account-sync.effects.ts @@ -19,6 +19,7 @@ import { selectSavedRooms } from '../../../store/rooms/rooms.selectors'; import { selectCurrentUser } from '../../../store/users/users.selectors'; import { FriendService } from '../../../domains/direct-message/application/services/friend.service'; import { CustomEmojiService } from '../../../domains/custom-emoji/application/custom-emoji.service'; +import { AttachmentFacade } from '../../../domains/attachment'; import { shouldApplyAccountSyncPayload } from './account-sync.rules'; import { ACCOUNT_SYNC_MESSAGE_LIMIT, pushSavedRoomMessagesViaAccountSync } from './account-sync-chat.helper'; import { pushProfileViaAccountSync } from './account-sync-profile.helper'; @@ -33,6 +34,7 @@ export class AccountSyncEffects { private readonly db = inject(DatabaseService); private readonly friends = inject(FriendService); private readonly customEmoji = inject(CustomEmojiService); + private readonly attachments = inject(AttachmentFacade); broadcastSavedRoom$ = createEffect( () => @@ -149,7 +151,8 @@ export class AccountSyncEffects { await pushSavedRoomMessagesViaAccountSync( this.webrtc, (roomId) => this.db.getMessages(roomId, ACCOUNT_SYNC_MESSAGE_LIMIT, 0), - savedRooms.map((room) => room.id) + savedRooms.map((room) => room.id), + (messageIds) => this.attachments.getAttachmentMetasForMessages(messageIds) ); const friends = await this.friends.friends();