sync: ambient directories, receiving rules, and the gates that were red - #624
sync: ambient directories, receiving rules, and the gates that were red#624alichherawalla wants to merge 278 commits into
Conversation
The knip gate had been failing on this branch, which is why it has 250+ commits and has never been pushed. Each symbol was checked against src, pro/ and the tests before touching it - the orphan gate earlier flagged a util that pro imports and the cruiser cannot see, so 'the tool says unused' is not sufficient evidence on its own. Deleted, because only their own declaration referenced them: - proLicenseService: PRO_DEVICE_LIMIT, ActivateResult, ActivateFailureCode, checkProStatus, and readProFromKeychain - which existed only to serve checkProStatus. The similarly-named function in pro/licensing/proLicenseProvider is that file's OWN local function, not an import of this one. - messageContext: parseMessageContext, a one-line pass-through to parseSyncedMessageContext. - rag/chunking: the ChunkOptions re-export. Un-exported rather than deleted, because each is still used inside its own file: MEDIA_STORE_DOWNLOADS_GRANT, ModelMatch, MeshResidencyCapabilities, RemoteChatStreamPreview, and ToolMessages' getToolIcon, getToolLabel and ToolResultBubbleInner. The three ToolMessages helpers are covered through rendering by __tests__/rntl/components/ChatMessageTools.test.tsx, which references them in comments only - it never imported them, so narrowing the surface changes nothing about their coverage. 611 suites, 8781 tests green. tsc and knip both clean.
parseSyncedMessageContext and the SyncedMessageContext type were imported solely for parseMessageContext, which went with the dead exports.
Android lint was failing the push, and it was right to. minSdk is 24, but java.time.Instant only exists from API 26, so three Kotlin sync modules would throw NoSuchMethodError on Android 7 rather than merely warn: - screenshot/ScreenshotWatcher.kt:213 Instant.ofEpochSecond - directory/SyncDirectorySourceModule.kt:95 Instant.ofEpochMilli - downloads/SyncDownloadsModule.kt:204 and :245 Instant.ofEpochMilli All three are on the ambient-sharing path, and each one formats a createdAt that goes over the wire to another device. Core library desugaring backports them, which is why that rather than hand-rolling the formatting in three places: these strings are compared across devices, and SimpleDateFormat would have to reproduce Instant's exact ISO-8601 shape (including when it does and does not print fractional seconds) or sync would see two spellings of the same moment.
The Kotlin E2E test would not compile: both BlobUploader.Request and BlobServer.Pending gained an offset when resume landed - 'payload bytes the receiver already holds' on one side, 'bytes already on disk' on the other - and this test predates it. Four call sites, all set to 0L, because each of these four journeys sends or receives a WHOLE payload and compares every byte, so there is nothing held on either side. Resume from a non-zero offset has no Kotlin-side test yet; that is noted at the call sites rather than left to be rediscovered.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds mobile Sync integration across React Native, Android, iOS, licensing, discovery, encrypted transfers, model and knowledge-document synchronization, UI surfaces, test boundaries, integration tests, and supporting configuration. ChangesMobile Sync Integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (37)
src/services/rag/database.ts-169-174 (1)
169-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRespect
byteOffsetandbyteLengthwhen decoding the embedding blob.
new Float32Array(blob.buffer)reads the whole backing buffer. It ignoresblob.byteOffsetandblob.byteLength. If the driver returns a view into a shared or pooled buffer, this decodes the wrong bytes, and it throws when the buffer length is not a multiple of 4.The write side already preserves offset and length. The new test harness in
__tests__/hardening/batch9-kb-roundtrip.test.tsbindsnew Uint8Array(view.buffer, view.byteOffset, view.byteLength), so the read side should be symmetric.🐛 Proposed fix
private blobToEmbedding(blob: any): number[] { if (blob instanceof ArrayBuffer) return Array.from(new Float32Array(blob)); - if (blob?.buffer instanceof ArrayBuffer) - return Array.from(new Float32Array(blob.buffer)); + if (ArrayBuffer.isView(blob)) + return Array.from( + new Float32Array( + blob.buffer, + blob.byteOffset, + blob.byteLength / Float32Array.BYTES_PER_ELEMENT, + ), + ); return []; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/rag/database.ts` around lines 169 - 174, Update blobToEmbedding to decode only the view’s byte range by constructing Float32Array with blob.buffer, blob.byteOffset, and a Float32-aligned byte length derived from blob.byteLength; preserve direct ArrayBuffer handling and the empty-array fallback.src/services/sync/nativeMeshResidency.ts-70-78 (1)
70-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not map an invalid grace value to unlimited residency.
nullmeans unbounded residency. When native constants omit, negate, or return a non-finitebackgroundGraceSeconds, this code also returnsnull. The UI can then report unlimited background reachability for a bridge that did not provide that capability.Use
0orNO_RESIDENCYwhensurvivesBackgroundis false and the grace value is invalid.Proposed fix
const grace = constants.backgroundGraceSeconds; + const survivesBackground = constants.survivesBackground === true; return { - survivesBackground: constants.survivesBackground === true, + survivesBackground, backgroundGraceSeconds: - typeof grace === 'number' && Number.isFinite(grace) && grace >= 0 + survivesBackground + ? null + : typeof grace === 'number' && Number.isFinite(grace) && grace >= 0 ? grace - : null, + : 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/sync/nativeMeshResidency.ts` around lines 70 - 78, Update the residency mapping around backgroundGraceSeconds so invalid, missing, negative, or non-finite grace values resolve to 0 or NO_RESIDENCY when survivesBackground is false, rather than null. Preserve null only for valid unbounded residency cases, and keep valid non-negative grace values unchanged.android/app/src/main/AndroidManifest.xml-109-118 (1)
109-118: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle Android’s dataSync foreground-service daily cap.
The declaration and permissions are present, but
MeshResidencyServicestarts onbegin()and only stops on explicitend()/cleanup. On Android 14+, if the service is stopped during the daily cap,startForegroundcan throw aSecurityExceptionon later startup paths; add retry behavior before returning failure asmesh_residency_denied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/AndroidManifest.xml` around lines 109 - 118, Update MeshResidencyService startup, specifically begin() and its startForeground failure handling, to detect SecurityException caused by the Android 14+ dataSync foreground-service daily cap and retry before returning mesh_residency_denied. Preserve the existing explicit end()/cleanup behavior and return denial only after the permitted retry attempts are exhausted.metro.config.js-56-61 (1)
56-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMap
@offgrid/syncto its resolved bundled entrypoint.The external package directory is already wrapped differently from
@offgrid/rag, and the package is documented as prebuilt CJS underdist/. Point the aliases to the actual file returned by Node package resolution instead of leaving the main import unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metro.config.js` around lines 56 - 61, Update the '`@offgrid/sync`' alias in the Metro configuration to resolve to the package’s bundled main entrypoint under dist, matching Node package resolution for the prebuilt CJS package. Keep the existing explicit rn, rn-discovery, and portable subpath mappings unchanged.src/utils/localTime.ts-45-61 (1)
45-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn invalid input renders as
" NaN".
asDatepasses any unparseable string through tonew Date(value), which yields an Invalid Date.
formatWeekdayhandles that case:getDay()returnsNaNand the?? ''guard applies.formatShortDatehandles only half of it.MONTHS[NaN]falls back to'', butgetDate()returnsNaNand is interpolated directly, so the result is the string" NaN".
formatWhenreaches that path for every invalid input.calendarDaysAgoreturnsNaN, all three comparisons at Lines 57 to 59 are false, and the function falls through toformatShortDate. A conversation row with a corruptupdatedAttherefore displays" NaN".This module is now the single formatter for four screens, so one guard covers all of them.
🐛 Proposed guard
export function formatShortDate(value: Date | number | string): string { const date = asDate(value); + if (Number.isNaN(date.getTime())) return ''; return `${MONTHS[date.getMonth()] ?? ''} ${date.getDate()}`; }Guard
formatClockTimeandformatWhenthe same way, or reject the invalid date once inasDate's callers.Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/localTime.ts` around lines 45 - 61, Update formatWhen and formatClockTime to guard invalid Date values consistently, ensuring invalid inputs return the module’s established empty/fallback representation instead of reaching formatShortDate and producing “ NaN”. Preserve the existing formatting branches for valid dates.__tests__/unit/sync/keygenPersonalMeshRegistry.test.ts-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the missing apostrophes in the test titles.
Six titles lost their apostrophe and now read as corrupted text in the test output: "the licence s record", "the provider s own id", "this phone s own details", "this phone s own activity", "another device whose record is thin" (unaffected), "keeping a device s details current".
Use the typographic apostrophe
’or escape the straight quote so the titles read correctly.✏️ Proposed fixes
-describe('the licence s record of the mesh, from the phone', () => { +describe("the licence's record of the mesh, from the phone", () => {- it('identifies each device by its fingerprint, not the provider s own id', async () => { + it("identifies each device by its fingerprint, not the provider's own id", async () => {- it('fills in this phone s own details when the provider s record is thin', async () => { + it("fills in this phone's own details when the provider's record is thin", async () => {- it('falls back to this phone s own activity when the provider s times are unusable', async () => { + it("falls back to this phone's own activity when the provider's times are unusable", async () => {- describe('keeping a device s details current', () => { + describe("keeping a device's details current", () => {Also applies to: 117-117, 141-141, 159-159, 184-184, 310-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/sync/keygenPersonalMeshRegistry.test.ts` at line 24, Restore the missing apostrophes in the affected test titles within the mesh registry test descriptions, including the titles around “licence s record,” “provider s own id,” “this phone s own details,” “this phone s own activity,” and “device s details current.” Use a typographic apostrophe or escaped straight apostrophe so each description reads correctly, leaving the unaffected title unchanged.__tests__/unit/licensing/proLicenseProvider.test.ts-290-311 (1)
290-311: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a distinct second key so the rollback assertion can observe which licence was restored.
Both activations use
LICENCE_KEY. The assertion checks onlycredentialSaved: true, so it cannot distinguish "the previous licence was put back" from "the failed licence was left in place". The test catches total credential loss, but not the property its title claims.Add a second licence and assert the stored credential is the original one.
💚 Proposed strengthening
+ const SECOND_KEY = 'OFFGRID-A-SECOND-LICENCE'; + keygen.addLicence({ key: SECOND_KEY, seats: 3 }); const failing = activationOwner({ commit: async () => { throw new Error('the transaction is gone'); }, }); provider.setDirectEntitlementActivationOwner(failing.owner); - await provider.proLicenseProvider.activate!(LICENCE_KEY); + await provider.proLicenseProvider.activate!(SECOND_KEY); await expect( provider.proLicenseProvider.getInfo(), ).resolves.toMatchObject({ credentialSaved: true, });Assert the surviving credential names the first licence, if
getInfo()exposes the key or licence id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/licensing/proLicenseProvider.test.ts` around lines 290 - 311, Strengthen the test “restores the previous licence when activation fails over an existing one” by using a distinct second licence key for the failing activation. Assert that getInfo() identifies the originally activated licence, using its exposed key or licence ID, rather than checking only credentialSaved: true; preserve the existing rollback scenario and failure setup.__tests__/integration/onboarding/proBootFlow.test.ts-195-195 (1)
195-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the test: the correction is no longer in the background.
The title says "in the background". The body comment at Line 217 and the assertions state the opposite: the launch awaits one revalidation, so the correction lands before the first screen is drawn. The title now describes the behaviour this test was changed to reject.
✏️ Proposed title fix
- it('corrects a stale cached answer in the background', async () => { + it('corrects a stale cached answer before the launch completes', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/integration/onboarding/proBootFlow.test.ts` at line 195, Rename the test case currently titled “corrects a stale cached answer in the background” to describe that launch awaits revalidation and the stale answer is corrected before the first screen is drawn. Update only the test title, keeping its body comment and assertions unchanged.__tests__/pro/sync/modelTransfer.integration.test.tsx-243-247 (1)
243-247: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale "STILL RED" note.
The comment states that this assertion fails and that giving the phone a licence "trips a reconciliation issue (
replacement_incomplete)". The test is not skipped, andinstallLicensedPhonenow runs inbeforeEachat Line 139. A future reader cannot tell whetherreplacement_incompleteis still open. Delete the note, or replace it with one sentence that states the resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pro/sync/modelTransfer.integration.test.tsx` around lines 243 - 247, Remove the stale “STILL RED” explanatory comment near the affected assertion, or replace it with a concise sentence describing the current resolution now that installLicensedPhone runs in beforeEach. Do not retain outdated claims about the missing row or unresolved replacement_incomplete issue.__tests__/pro/sync/ambientShare.integration.test.tsx-210-224 (1)
210-224: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear
screenshotListenerbetween tests.
beforeEachreassignsscreenshotListeneronly when the spy sees a newSyncScreenshotCapturedsubscription.afterEachdoes not clear it. If a later test in this file never subscribes, thewaitForat Line 362 resolves against the listener captured by the previous test and delivers the event to a service that was already stopped. Reset the variable inafterEach.🧪 Proposed cleanup
afterEach(async () => { mesh.restore(); + screenshotListener = undefined; await ambientShareService.setRule({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pro/sync/ambientShare.integration.test.tsx` around lines 210 - 224, Update the afterEach cleanup in the test suite to reset the screenshotListener variable after each test, ensuring later tests cannot reuse a listener captured from a stopped service. Keep the existing teardown sequence and clear the variable unconditionally alongside the other test-state cleanup.__tests__/unit/sync/pairingSecretStore.test.ts-719-731 (1)
719-731: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the pending copy as well, or the test only covers the tombstone.
The test mutates
inFlight.revocationIdat Line 723, but no assertion reads the pending revocation back. Only the tombstone copy is verified at Line 728. IfgetPendingstarts returning the stored object by reference, this test still passes.💚 Proposed added assertion
await store.beginLocal(paired(), pending()); const inFlight = store.getPending('the-mac')!; inFlight.revocationId = 'edited'; + expect(store.getPending('the-mac')?.revocationId).toBe('revocation-1'); await store.completeLocal(pending(), tombstone());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/sync/pairingSecretStore.test.ts` around lines 719 - 731, Extend the test “gives copies of a pending revocation and a tombstone” to assert that retrieving the pending record after mutating the object returned by getPending does not expose the mutation. Keep the existing tombstone assertion and add a corresponding getPending assertion for the original revocation ID.src/services/sync/nativeClipboard.ts-37-56 (1)
37-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReference-count the native enable state.
nativeClipboardBoundaryis a module-level singleton, andobservereturns an unsubscribe function. That shape allows two concurrent subscribers. Each one callssetEnabled(true)at line 51. The first disposer to run callssetEnabled(false)at line 53, which stops native observation for the second subscriber while itsNativeEventEmittersubscription is still attached. That subscriber then receives no events and reports no error.This ordering occurs whenever a restart path subscribes before the previous disposer runs. The symptom is a clipboard that silently stops syncing.
Track the active subscriber count and disable the native module only when it reaches zero.
🐛 Proposed fix
+let activeObservers = 0; + export const nativeClipboardBoundary: NativeClipboardBoundary = { observe(listener): () => void { const nativeModule = module(); const emitter = new NativeEventEmitter(nativeModule); const subscription: EmitterSubscription = emitter.addListener( CLIPBOARD_CHANGED_EVENT, (value: unknown) => { if (!value || typeof value !== 'object') return; const change = value as Partial<NativeClipboardChange>; if (typeof change.text !== 'string' || typeof change.ts !== 'number') { return; } listener({ text: change.text, ts: change.ts }); }, ); - nativeModule.setEnabled(true); + activeObservers += 1; + if (activeObservers === 1) nativeModule.setEnabled(true); + let disposed = false; return () => { - nativeModule.setEnabled(false); + if (disposed) return; + disposed = true; subscription.remove(); + activeObservers -= 1; + if (activeObservers === 0) nativeModule.setEnabled(false); }; },The
disposedguard also keeps the count correct if a caller invokes the disposer twice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/sync/nativeClipboard.ts` around lines 37 - 56, Update the module-level nativeClipboardBoundary state and observe method to reference-count active subscribers: increment the count when observe subscribes, and have the returned disposer use a disposed guard so it decrements at most once and calls nativeModule.setEnabled(false) only when the count reaches zero. Keep each subscription’s removal behavior unchanged.__tests__/pro/sync/downloadsSharing.integration.test.tsx-31-36 (1)
31-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
setresets the wrongrequestscounter.Line 33 assigns
{ requests: 0 }ontostate, but the counter that tests read and thatrequestAllFilesAccessincrements isrecord.requests. The reset therefore has no effect, and it adds an unusedrequestskey tostate. The suite only passes becausebeforeEachresetsboundary.requestsat Line 92. Reset the counter onrecord.🐛 Proposed fix
const state = { ...initial }; const record = { - set: (next: { media: boolean; allFiles: boolean }) => Object.assign(state, next, { requests: 0 }), + set: (next: { media: boolean; allFiles: boolean }) => { + Object.assign(state, next); + record.requests = 0; + }, grantAllFiles: () => (state.allFiles = true), requests: 0, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pro/sync/downloadsSharing.integration.test.tsx` around lines 31 - 36, Update the record.set method to reset record.requests rather than assigning requests onto state, while preserving the existing media and allFiles updates. Ensure requestAllFilesAccess and the tests observe the same reset counter on record.ios/SyncProximityModule.swift-131-132 (1)
131-132: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the display name by bytes, not by characters.
MCPeerID(displayName:)accepts at most 63 UTF-8 bytes and raises an exception above that.parsed.id.prefix(60)counts Characters. A 60-character identifier that contains multi-byte characters exceeds 63 bytes, and the initializer then terminates the app.parsed.idcomes from JavaScript andProximityDevice.parsechecks only that it is a non-empty string.Truncate on the UTF-8 byte count instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/SyncProximityModule.swift` around lines 131 - 132, Update the display-name construction immediately before MCPeerID in the proximity sync flow to truncate parsed.id by UTF-8 byte count rather than Swift Character count, ensuring the resulting name is at most 63 UTF-8 bytes while remaining valid text. Preserve the existing parsed.id source and MCPeerID creation flow.scripts/blob-e2e/run.mjs-27-33 (1)
27-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a non-numeric
--size.
Number(flag('size', ...))producesNaNfor--size abcand for a trailing--sizewith no value.randomBytes(NaN)at Line 56 then throwsERR_OUT_OF_RANGEbefore any test runs. Validate the parsed value with the--phonecheck.🔢 Proposed fix
if (!phone) { console.error('--phone <harness> is required'); process.exit(2); } +if (!Number.isInteger(size) || size <= 0) { + console.error('--size must be a positive whole number of bytes'); + process.exit(2); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/blob-e2e/run.mjs` around lines 27 - 33, Validate the parsed size alongside the required --phone check, rejecting NaN and other non-numeric or invalid size values before the test flow reaches randomBytes. Use the existing size value and preserve the current required-argument error handling pattern.src/stores/chatMessageMutationActions.ts-52-64 (1)
52-64: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConversation
updatedAtadvances locally without a matching sync mutation. Several actions bumpconversation.updatedAtbut emit noconversationPutMutation, so the peer keeps the previousupdated_atand conversation ordering diverges across devices.src/stores/chatStore.tsalready emits aconversationPutMutationafteraddMessage,setConversationProject, andunfileConversationsForProject; these sites break that pattern.
src/stores/chatMessageMutationActions.ts#L52-L64:updateMessageInConversationsetsupdatedAt: nextUpdatedAt(conversation.updatedAt)for every caller. Either emit aconversationPutMutationfrom the callers, or stop bumpingupdatedAthere for local-only metadata.src/stores/chatMessageMutationActions.ts#L97-L117:updateMessageThinkingandupdateMessageAudiochange only local metadata, yet they still advanceupdatedAt. LeaveupdatedAtunchanged in these two actions.src/stores/chatStore.ts#L381-L393:updateCompactionStateadvancesupdatedAt. EmitemitSyncMutation(conversationPutMutation(conversation))after theset, using the same pattern assetConversationProject.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/chatMessageMutationActions.ts` around lines 52 - 64, In src/stores/chatMessageMutationActions.ts#L52-L64, updateMessageInConversation currently advances updatedAt for all callers; adjust its callers or metadata handling so local-only updates do not create unsynchronized timestamps. Specifically, in src/stores/chatMessageMutationActions.ts#L97-L117, keep updatedAt unchanged for updateMessageThinking and updateMessageAudio; in src/stores/chatStore.ts#L381-L393, after updateCompactionState updates the conversation, emit conversationPutMutation via emitSyncMutation using the existing setConversationProject pattern.src/stores/chatStore.ts-232-259 (1)
232-259: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
addMessageemits a sync mutation for a conversation that may not exist.The
setat Line 235 maps over conversations and matches nothing whenconversationIdis unknown. The message is never stored locally. Line 251 still emitsmessagePutMutation, so the peer receives a message row whoseconversation_idpoints at a conversation the peer never receives.Guard the emission on the conversation being found.
🐛 Proposed fix
- emitSyncMutation(messagePutMutation(conversationId, message)); const conversation = get().conversations.find( conv => conv.id === conversationId, ); - if (conversation) + if (conversation) { + emitSyncMutation(messagePutMutation(conversationId, message)); emitSyncMutation(conversationPutMutation(conversation)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/chatStore.ts` around lines 232 - 259, Update addMessage so it only emits messagePutMutation after confirming conversationId matches an existing conversation; when no conversation is found, do not emit either message or conversation sync mutations, while preserving the existing behavior for valid conversations.__tests__/unit/sync/ambientShareService.test.ts-96-135 (1)
96-135: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the AsyncStorage mock store tied to the same mock instance after module reload.
launch()callsjest.resetModules()before requiringambientShareService, so the service receives a fresh@react-native-async-storage/async-storagemock copy. Write reads back from storage withbeforeEachclearing, and the restart test may pass without proving persisted state survives service reload. Hoist the mock store intojest.setup.ts, or otherwise ensure all service launches use a shared AsyncStorage mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/sync/ambientShareService.test.ts` around lines 96 - 135, The test’s AsyncStorage state is recreated across jest.resetModules(), so restart coverage may not verify persistence. Update the AsyncStorage mock setup used by ambientShareService launches to store data in a shared hoisted mock instance, such as the one defined in jest.setup.ts, and ensure beforeEach clearing and all launch() reloads use that same store.src/screens/SettingsAppearanceRow.tsx-36-51 (1)
36-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd accessible names and selected state to each appearance control.
The icon-only
TouchableOpacitybuttons only expose the row label as context. Label each control with its appearance mode and mark the active option as selected:
accessibilityRole="button",accessibilityLabel={System/Light/Dark mode}, andaccessibilityState={{ selected: themeMode === mode }}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/SettingsAppearanceRow.tsx` around lines 36 - 51, Update each appearance-mode TouchableOpacity in the theme selector to include accessibilityRole="button", an accessibilityLabel derived from the current mode (System, Light, or Dark), and accessibilityState with selected set to themeMode === mode; preserve the existing onPress and styling behavior.src/screens/SettingsCommunitySections.tsx-43-46 (1)
43-46: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle outbound-link failures.
Linking.openURL()rejects when the OS cannot open the target URL. These handlers do not catch the rejection, so a failed browser/X-intent open leaves the tap without user feedback. Catch and show an alert or visible failure state here and forshareOnX()at lines 46, 64, 87, and 117.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/SettingsCommunitySections.tsx` around lines 43 - 46, Handle rejected outbound-link promises in the onPress handler using Linking.openURL and in shareOnX(), including all referenced call sites, and present an alert or visible failure state when opening the browser/X intent fails. Preserve the existing successful navigation and sharing behavior.src/screens/SettingsScreen.tsx-196-207 (1)
196-207: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the privacy statement to account for Sync.
Sync copies chats, projects, files, and clipboard content between paired devices. The Privacy First card in Settings still says data stays on this device, so it should be conditional on Sync or reworded to distinguish no server processing from encrypted device-to-device transfer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/SettingsScreen.tsx` around lines 196 - 207, Update the Privacy First card in Settings to account for the hasSync state: when Sync is enabled, describe encrypted device-to-device transfer of chats, projects, files, and copied text instead of claiming data stays only on the device; preserve the existing privacy wording when Sync is unavailable.src/services/rag/pastedNote.ts-48-53 (1)
48-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTruncate before you trim.
Line 53 runs
.slice(0, MAX_TITLE_LENGTH)after.trim(). If character 60 of the cleaned title is a space, the truncated title keeps a trailing space, and the filename becomes"<title> .txt". A trailing space in a path component is rejected on Windows and is stripped by some SMB and cloud targets. The file doc states that knowledge-document sync ships these notes to the user's other devices, so the name has to survive on a desktop target.Trim again after the cut.
🐛 Proposed fix
const cleaned = title .replace(/[/\\]/g, ' ') .replace(/\s+/g, ' ') .replace(/^\.+/, '') .trim() - .slice(0, MAX_TITLE_LENGTH); + .slice(0, MAX_TITLE_LENGTH) + // Trimmed again after the cut: the boundary can land on a space, and a trailing space is not a + // valid path component on every device a note syncs to. + .trim();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/rag/pastedNote.ts` around lines 48 - 53, Update the title-cleaning chain in the pasted-note title logic so the truncated result is trimmed again after applying MAX_TITLE_LENGTH. Preserve the existing sanitization and pre-truncation trim while ensuring the final filename component cannot end with whitespace.src/services/sync/fileChecksum.ts-18-47 (1)
18-47: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd missing coverage for the native and chunked checksum contract.
fileTransferChecksumdocuments thatchecksumFromSha512HexandIncrementalChecksum().digest()produce the same value for the same file, but tests only exerciseIncrementalChecksumdirectly. If the two implementations diverge on bytes or the empty-file path, sender/receiver pairs can compute different transfer checksums after one side falls back or the native hash is unavailable. Add a direct parity test for both paths, including zero-byte input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/sync/fileChecksum.ts` around lines 18 - 47, The checksum tests need direct parity coverage between the native and chunked paths, including empty files. Add tests targeting fileTransferChecksum that mock RNFS.hash and RNFS.read as needed, verify both paths return the same checksum for identical non-empty bytes, and separately verify they agree for zero-byte input while preserving the existing fallback behavior.__tests__/utils/modelTransferFsBoundary.ts-106-108 (1)
106-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMatch
unlinktoreact-native-fsfor missing files.This test-level fake uses
rmSync(..., { force: true }), so a missing path resolves instead of rejecting. For__tests__/unit/sync/explicitSharedFileSource.test.ts:326-332, that still covers the expected tolerance, but make the fake reject on a missing path and rely ondiscardExplicitSharedFilehandling the cleanup failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/utils/modelTransferFsBoundary.ts` around lines 106 - 108, Update the test fake’s unlink implementation to reject when the normalized path is missing, matching react-native-fs behavior; remove the force-based suppression from the volume removal in the unlink mock. Keep discardExplicitSharedFile responsible for handling the cleanup failure and preserve the existing successful removal behavior.ios/BlobChannelServer.swift-59-64 (1)
59-64: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSweep expired pending transfers so the listener can stop.
claimrejects an entry whoseexpiresAthas passed, but it leaves that entry inpending. Nothing else removes it.release(requestId:)is then the only path that empties the map, so an offer that is never released keepspendingnon-empty forever. The listener stays bound, which contradicts the rule at lines 10-11 that the server listens only while a transfer is pending.Drop expired entries whenever the map is inspected.
♻️ Proposed change
fileprivate func claim(_ head: BlobChannelSupport.Head) -> Pending? { queue.sync { + pending = pending.filter { $0.value.expiresAt > Date() } guard let transfer = pending[head.requestId], transfer.expiresAt > Date() else { return nil } guard BlobChannelSupport.matches(head.token, transfer.token) else { return nil } pending.removeValue(forKey: head.requestId) + if pending.isEmpty { /* the listener stops once this session settles */ } return transfer } }Apply the same filter inside
finishedAll()so an abandoned offer cannot hold the port open.Also applies to: 108-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/BlobChannelServer.swift` around lines 59 - 64, Update finishedAll() and the pending-map inspection in claim(requestId:) to remove entries whose expiresAt has passed before evaluating pending state. Reuse the existing expiration criteria, then preserve the current stopLocked() behavior when the sweep leaves pending empty; release(requestId:) should continue removing its requested entry normally.android/app/src/main/java/ai/offgridmobile/sync/MeshResidencyModule.kt-63-71 (1)
63-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the stop call in
invalidate().
end()treatsMeshResidencyService.stopas a call that can throwIllegalStateException.invalidate()makes the same call without a guard. If it throws during a reload or teardown, the exception propagates into React Native's teardown path andsuper.invalidate()never runs.🛡️ Proposed fix
override fun invalidate() { // A reload or teardown must not leave an orphan notification promising reachability the // JS engine can no longer provide. if (held) { - MeshResidencyService.stop(reactContext) - held = false + try { + MeshResidencyService.stop(reactContext) + } catch (e: IllegalStateException) { + // Teardown must complete regardless; the service is stopped on process death. + } + held = false } super.invalidate() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/ai/offgridmobile/sync/MeshResidencyModule.kt` around lines 63 - 71, Update invalidate() to guard MeshResidencyService.stop(reactContext) against IllegalStateException, matching the handling in end(). Ensure held is cleared and super.invalidate() always executes even when stopping the service fails.docs/GAPS_BACKLOG.md-415-426 (1)
415-426: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed "Open" table.
The header declares three columns. Rows PM4, PM5, and PM6 supply only two cells, so their text lands in the Gap column and the Verdict column is empty. The blank lines at Line 422 and Line 424 also terminate the table, so PM9, PM10, and PM11 render as separate headerless tables.
Move the closed-decision text into the Verdict column for PM4, PM5, and PM6, and remove the blank lines inside the table.
📝 Proposed structure for the affected rows
-| PM4 | ~~Android reinstall orphans a licensed seat.~~ **By design, not a gap.** Android wipes the Keystore on uninstall so a reinstall mints a new fingerprint and consumes a seat. This is the exact case auto-eviction of the least-active installation plus user-driven device management already answers: the dead entry is by definition least-active, so it is what gets replaced. Closed. | -| PM5 | ~~No license-key revocation or rotation.~~ **Out of scope by product decision (2026-07-30).** We do not support revoking or rotating a key: if a key is compromised, that is the user's loss. Device eviction remains the only removal mechanism. Do not re-open this as a gap. | -| PM6 | ~~Two shared projections have zero callers.~~ **Wrong - both are rendered** (`KnownDevicesSection.tsx:78`, `DevicesScreen.tsx:2348`); the original grep excluded `shared/`. The real defect was the COPY: the confirmation said eviction "removes the pairing from both devices", omitting that the seat is freed and what happens to the target's saved licence. Fixed in shared: the copy now splits on reachability, so an offline device is told cleanup stays queued rather than claimed already clean. Closed. | +| PM4 | ~~Android reinstall orphans a licensed seat.~~ Android wipes the Keystore on uninstall so a reinstall mints a new fingerprint and consumes a seat. Auto-eviction of the least-active installation plus user-driven device management already answers this: the dead entry is by definition least-active, so it is what gets replaced. | **By design, not a gap.** Closed. | +| PM5 | ~~No license-key revocation or rotation.~~ We do not support revoking or rotating a key: if a key is compromised, that is the user's loss. Device eviction remains the only removal mechanism. | **Out of scope by product decision (2026-07-30).** Do not re-open this as a gap. | +| PM6 | ~~Two shared projections have zero callers.~~ Both are rendered (`KnownDevicesSection.tsx:78`, `DevicesScreen.tsx:2348`); the original grep excluded `shared/`. The real defect was the COPY: the confirmation said eviction "removes the pairing from both devices", omitting that the seat is freed and what happens to the target's saved licence. | **Wrong.** Fixed in shared: the copy now splits on reachability, so an offline device is told cleanup stays queued rather than claimed already clean. Closed. |Also delete the blank lines at Line 422 and Line 424 so PM9, PM10, and PM11 stay in the same table.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/GAPS_BACKLOG.md` around lines 415 - 426, Fix the Markdown table in the affected PM4–PM11 section: ensure PM4, PM5, and PM6 each have separate ID, Gap, and Verdict cells, placing their closed-decision text in Verdict. Remove the blank lines between rows so PM9, PM10, and PM11 remain part of the same table.Source: Linters/SAST tools
android/app/src/main/java/ai/offgridmobile/clipboard/SyncClipboardModule.kt-21-27 (1)
21-27: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
getItemAt(0)withitemCount.
ClipData.getItemAt(0)throwsIndexOutOfBoundsExceptionwhenprimaryClip.itemCountis zero, even thoughprimaryClipis non-null. Return from theOnPrimaryClipChangedListenerbefore reading this index.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/ai/offgridmobile/clipboard/SyncClipboardModule.kt` around lines 21 - 27, Update the primary-clip handling in the OnPrimaryClipChangedListener to return when clip.itemCount is zero before calling getItemAt(0). Preserve the existing label guard and text-processing flow for clips containing at least one item.docs/SYNC_INTEGRATION_PLAN.md-25-36 (1)
25-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the Phase 0 checkboxes to match what this PR landed.
Lines 28 to 36 leave Metro resolution, native modules, injection glue, the dev surface, and on-device verification unchecked. This PR adds the Metro configuration, the native modules, and the injection glue, and
docs/HANDOFF_SYNC_SESSION.mdLine 71 records Android to macOS LAN verified on device on 2026-07-31. A reader who starts from this plan repeats work that is already done.Mark the completed items, or add a short note stating which items the handoff document supersedes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/SYNC_INTEGRATION_PLAN.md` around lines 25 - 36, Update the Phase 0 checklist in the documented plan to reflect the PR’s completed Metro resolution, native module setup, injection glue, and Android-to-macOS on-device verification, using the verification recorded in HANDOFF_SYNC_SESSION.md; leave the minimal dev surface unchecked unless this PR also completed it, or add a concise superseding note pointing readers to the handoff document.docs/HANDOFF_SYNC_SESSION.md-24-26 (1)
24-26: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRemove the hardware UDID from the committed document.
Replace the committed iPhone UDID with a placeholder and direct users to obtain it with
xcrun devicectl list devices, or omit it and letios-device.shselect the device.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/HANDOFF_SYNC_SESSION.md` around lines 24 - 26, Replace the hardcoded iPhone UDID in the iOS device command within HANDOFF_SYNC_SESSION.md with a safe placeholder, and instruct users to obtain their device identifier via xcrun devicectl list devices or omit IOS_DEVICE_ID to let ios-device.sh select the device.scripts/ios-device.sh-112-129 (1)
112-129: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDistinguish absent
IOS_DEVICE_IDdevices from tunnel startup failures.If
IOS_DEVICE_IDis set to a UDID that is not inlist_candidates,wake_devicetreats the absence like a tunnel that cannot start and prints that the device “is present”. Add a candidate check for the override path, or changewake_deviceto report when the UDID is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ios-device.sh` around lines 112 - 129, Update the IOS_DEVICE_ID override path to verify that the requested UDID appears in list_candidates before calling wake_device. If it is absent, report that the specified device is not connected and exit without claiming it is present; preserve the existing tunnel startup failure message for candidates that are found.ios/BlobChannelUploader.swift-152-165 (1)
152-165: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOne
receivecall may return fewer than twelve bytes and fail a completed upload.
connection.receive(minimumIncompleteLength: 1, ...)returns as soon as any byte is available. If the peer's status line arrives split across TCP segments,answercan be shorter than"HTTP/1.1 200", and the guard rejects a successful upload after the whole payload was sent. SetminimumIncompleteLengthto the length of the prefix being tested, or read until the first\r\n.🐛 Proposed fix
- connection.receive(minimumIncompleteLength: 1, maximumLength: 1 << 12) { data, _, _, _ in + connection.receive(minimumIncompleteLength: 12, maximumLength: 1 << 12) { data, _, _, _ in🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/BlobChannelUploader.swift` around lines 152 - 165, Update expectSuccess to wait for enough response data to validate the complete "HTTP/1.1 200" prefix, either by setting receive’s minimumIncompleteLength to that prefix length or by reading through the first "\r\n"; preserve the existing timeout and error handling.scripts/physical-sync/iosKnowledgeSyncAdapter.mjs-1-11 (1)
1-11: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestrict direct invocation or pin a Node version that supports TypeScript stripping.
The npm script sets
--experimental-strip-types --no-warnings, but Node’s minimum support for that flag via shebang starts only at select versions above>=20; older supported Node installations will fail importing../../../provit/src/ios/wdaActor.ts. Ensure the adapter is not invoked asnode scripts/physical-sync/iosKnowledgeSyncAdapter.mjson unsupported releases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/physical-sync/iosKnowledgeSyncAdapter.mjs` around lines 1 - 11, Update the direct invocation path for iosKnowledgeSyncAdapter so it cannot run on Node versions that do not support --experimental-strip-types in the shebang. Pin or validate the Node version before importing wdaActor.ts, and ensure the npm script and executable usage consistently enforce that supported runtime requirement.src/services/sync/nativeDirectorySource.ts-163-169 (1)
163-169: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a rejected
requestAllFilesAccess.
SyncDownloadsModule.requestAllFilesAccessrejects withdownloads_all_files_unavailablewhen both the all-files settings intent and the app-details fallback fail.upgrade()does not catch that, so the rejection propagates to the tap handler andrefreshDownloadsAccess()never runs. The card then keeps stale access state.🛡️ Proposed fix
async upgrade(): Promise<void> { const boundary = downloadsModule(); if (!boundary?.requestAllFilesAccess) return; - await boundary.requestAllFilesAccess(); - await refreshDownloadsAccess(); + try { + await boundary.requestAllFilesAccess(); + } catch { + // No settings screen exists on this build. The card keeps its current wording. + } + await refreshDownloadsAccess(); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/sync/nativeDirectorySource.ts` around lines 163 - 169, Update NativeDirectorySource.upgrade so a rejection from boundary.requestAllFilesAccess is caught, including the downloads_all_files_unavailable case, and refreshDownloadsAccess() still executes afterward. Preserve the existing early return when requestAllFilesAccess is unavailable and avoid propagating this expected access-upgrade failure to the tap handler.ios/e2e/main.swift-27-27 (1)
27-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire all arguments before indexing them.
Both modes read
arguments[8], but this guard accepts seven or eight arguments. A malformed command then traps beforefail()can report usage. Require at least nine arguments.Proposed fix
-guard arguments.count >= 7 else { fail("usage: serve | stream") } +guard arguments.count >= 9 else { fail("usage: serve | stream") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/e2e/main.swift` at line 27, Update the argument-count guard in the main argument-handling flow to require at least nine arguments before either serve or stream mode indexes arguments[8]. Preserve the existing fail("usage: serve | stream") behavior for shorter invocations.android/app/src/main/java/ai/offgridmobile/sync/BlobChannelModule.kt-29-30 (1)
29-30: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease native resources when the React bridge is invalidated.
workand theBlobServerlistener outlive the JavaScript bridge. If invalidation occurs beforerelease(), the old module can keep a transfer port open and retain work after its owner is gone. Addinvalidate()that stops the server, shuts down the executor, and then callssuper.invalidate().Proposed fix
- private val server by lazy { BlobServer(::emitProgress, ::emitOutcome) } + private val server = BlobServer(::emitProgress, ::emitOutcome) + + override fun invalidate() { + server.stop() + work.shutdownNow() + super.invalidate() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/ai/offgridmobile/sync/BlobChannelModule.kt` around lines 29 - 30, Add an override of invalidate() to BlobChannelModule that stops the lazy BlobServer, shuts down work, and then invokes super.invalidate(). Ensure cleanup is safe when invalidation occurs before server initialization.ios/BlobChannelSupport.swift-57-65 (1)
57-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
ifa_addrbefore dereferencing it.
getifaddrsreturnsifaddrsentries whoseifa_addrcan beNULL. Line 60 readsifa_addr.pointee.sa_familybefore any null check, so an entry without an address can trap instead of returning no LAN endpoint. Bind the pointer withguard letand pass that pointer togetnameinfo.Proposed fix
for pointer in sequence(first: start, next: { $0.pointee.ifa_next }) { let flags = Int32(pointer.pointee.ifa_flags) guard flags & IFF_UP != 0, flags & IFF_LOOPBACK == 0 else { continue } - guard pointer.pointee.ifa_addr.pointee.sa_family == UInt8(AF_INET) else { continue } + guard let socketAddress = pointer.pointee.ifa_addr, + socketAddress.pointee.sa_family == UInt8(AF_INET) + else { continue } var host = [CChar](repeating: 0, count: Int(NI_MAXHOST)) guard getnameinfo( - pointer.pointee.ifa_addr, socklen_t(pointer.pointee.ifa_addr.pointee.sa_len), &host, + socketAddress, socklen_t(socketAddress.pointee.sa_len), &host,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/BlobChannelSupport.swift` around lines 57 - 65, In the interface iteration around the existing guards, safely unwrap pointer.pointee.ifa_addr before accessing sa_family. Use the unwrapped address pointer for both the IPv4 family check and the subsequent getnameinfo call, continuing to the next entry when it is nil.
Same cause as desktop's: 'file:../shared/packages/sync' points at a sibling repo CI never checked out, so typecheck failed with TS2307, the architecture gate reported 26 phantom dependencies, and 260 of 611 jest suites could not locate <workspace>/../shared/packages/sync/src/index.ts. Nothing to do with the diff. Added to all four jobs, including the test job whose install step is worded differently and would otherwise have been missed. Checked out inside the workspace and moved up one level, since actions/checkout refuses a path outside it - and that is where jest's moduleNameMapper points.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 63-68: Stop masking dependency-install failures for the shared
packages/sync package: replace the ci-or-install fallback with a locked install
that fails immediately, and ensure the corresponding lockfile is committed or
the root lockfile installs the full shared monorepo. Apply this consistently at
.github/workflows/ci.yml lines 63-68, 138-143, 211-216, and 303-308, while
preserving failure propagation for both install and build commands.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a26d2128-24f3-409f-a214-f66732b97aba
📒 Files selected for processing (1)
.github/workflows/ci.yml
Every Activity row advertises what it can do (capabilities) while the projection separately registers what actually does it (dispatchAction). Those are two halves of one thing and nothing in the type system pairs them - a row can render Retry, Cancel and Dismiss and have every press reach nothing. syncControlCenterData.ts says exactly that in a comment. So the central test is a sweep: build a real projection over five rows (a live send, a completed receive, a failed ambient share, a failed knowledge document, a failed model job), render the real section, and for every button that is visible AND enabled - Open included - press it and require a handler to have been called. Any future row that grows a button without wiring it fails this by construction. The sweep asserts it pressed at least four controls, so it cannot quietly shrink to proving nothing. Alongside it, the words the user actually reads: each of the seven states is asserted on ITS OWN row rather than anywhere on screen, because direction changes the word and 'Sent' on a file that arrived would tell the user their phone sent something it did not. Plus progress in bytes as well as percent, and the invariant that nothing is ever enabled while hidden. TransferActivitySection.tsx 74% -> 83% statements, 79% -> 90% branches. This closes the 'retry, cancel and dismiss actually act' and 'make a dead button impossible' items.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/pro/ui/transferActivitySection.test.tsx`:
- Around line 321-336: Update the test around the sync activity button loop to
select the action-specific mock from acts for each fixture action before
pressing the button, using the existing action-to-handler mapping. Compare that
mock’s call count before and after fireEvent.press(button), and assert only that
expected handler increased rather than summing calls across all handlers.
- Around line 32-42: Update the beforeAll module-loading logic for
projectMobileSyncActivity and TransferActivitySection so only MODULE_NOT_FOUND
errors whose missing module is exactly one of the two requested private
specifiers set available to false; rethrow evaluation errors and missing
dependencies from those modules instead of suppressing them.
- Around line 213-232: Add the queued transfer state to the `expected` label map
in the test, keyed by `Queued.png` and matching the UI’s queued label text, so
the existing loop validates that row and the `checked` count covers every
transfer state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16153ade-8292-46c8-b33d-9a27b9b52c5c
📒 Files selected for processing (1)
__tests__/pro/ui/transferActivitySection.test.tsx
…ssing I hit this while writing the shared-file delivery tests: five cases reported green against a module that could not be imported at all. The guard the pro-dependent suites use is a plain try/catch that sets available=false, every case early-returns, and the suite passes having asserted nothing. Coverage was what gave it away - 0% on a file with five passing tests. requirePro() keeps the behaviour that matters and removes the trap. An open-core checkout genuinely has no pro/, so those suites still skip. A pro/ that IS present and whose module will not load now throws, with the underlying cause attached and a message that says the suite would otherwise have passed without asserting anything. Verified against the real case: requirePro on the module that broke (a transitive import constructs a NativeEventEmitter for react-native-tcp-socket, whose native module the jest environment does not provide) throws rather than skipping. All three guarded suites now go through it; 90 tests still pass.
The preview is what makes a file list useful - it tells the user whether they want a file without opening it. So the cases that matter are all the ways a preview cannot be produced, because each has to say something DIFFERENT and none may leave a blank space or a spinner that never resolves: the bytes are not on this phone, a transfer is still in flight (the caller's own wording wins), the type has nothing to show, the read failed but the file is still there so the message points at opening it, and an empty file is not a preview. Two worth keeping beyond coverage: - a PDF goes through the extractor and never through a utf8 read, because the first bytes of a PDF are binary and showing those as a preview is worse than showing nothing - fileUri() prefixes exactly once, since 'file://file:///...' fails to load and renders an empty box The image case asserts a property rather than a number - a tall photo gets a tall box - so it holds whatever thumbnail size the projection picks. The projection and the app's real dark palette are both production code here; only the filesystem and the icon font are stood in for. SharedFilePreview.tsx: 50% statements, 42% branches, 57% functions -> 100%, 94%, 100%.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/pro/helpers/requirePro.ts`:
- Around line 15-22: __tests__/pro/helpers/requirePro.ts:15-22: Export a
synchronous predicate that checks whether pro/package.json exists.
__tests__/pro/ui/receivingSection.test.tsx:33-36: Use that predicate to choose
describe.skip before registering the suite, while retaining requirePro in active
setup so an unloadable present Pro module still fails.
__tests__/pro/ui/sharedFilePreview.test.tsx:35-43: Choose describe.skip from the
predicate before registration, move module loading into the active suite, and
retain requirePro for the unloadable-module failure path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8afaf08a-49fb-45dd-9bf7-657bd221f37b
📒 Files selected for processing (5)
__tests__/pro/helpers/requirePro.ts__tests__/pro/ui/receivingSection.test.tsx__tests__/pro/ui/sharedFilePreview.test.tsx__tests__/pro/ui/transferActivitySection.test.tsx__tests__/pro/ui/useExplicitFileShare.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/pro/ui/useExplicitFileShare.test.ts
- tests/pro/ui/transferActivitySection.test.tsx
| export function requirePro<T>(specifier: string): T | undefined { | ||
| try { | ||
| return require(specifier) as T; | ||
| } catch (cause) { | ||
| const submodule = path.resolve(__dirname, '../../../pro'); | ||
| if (!fs.existsSync(path.join(submodule, 'package.json'))) { | ||
| console.warn(`pro/ is absent - skipping the suite that needs ${specifier}`); | ||
| return undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Register Pro suites as skipped when pro/ is absent.
requirePro returns undefined, but it does not invoke Jest skip behavior. The UI suites decide availability in beforeAll, after Jest registers their cases. The shared-file suite then reports no-op cases as passed instead of skipped.
__tests__/pro/helpers/requirePro.ts#L15-L22: Export a synchronous predicate that checks forpro/package.json.__tests__/pro/ui/receivingSection.test.tsx#L33-L36: Selectdescribe.skipfrom that predicate before registering the suite. RetainrequireProin active setup to fail when Pro is present but unloadable.__tests__/pro/ui/sharedFilePreview.test.tsx#L35-L43: Selectdescribe.skipfrom that predicate before registering the suite. Move module loading into the active suite and retainrequireProfor the unloadable-module failure path.
📍 Affects 3 files
__tests__/pro/helpers/requirePro.ts#L15-L22(this comment)__tests__/pro/ui/receivingSection.test.tsx#L33-L36__tests__/pro/ui/sharedFilePreview.test.tsx#L35-L43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/pro/helpers/requirePro.ts` around lines 15 - 22,
__tests__/pro/helpers/requirePro.ts:15-22: Export a synchronous predicate that
checks whether pro/package.json exists.
__tests__/pro/ui/receivingSection.test.tsx:33-36: Use that predicate to choose
describe.skip before registering the suite, while retaining requirePro in active
setup so an unloadable present Pro module still fails.
__tests__/pro/ui/sharedFilePreview.test.tsx:35-43: Choose describe.skip from the
predicate before registration, move module loading into the active suite, and
retain requirePro for the unloadable-module failure path.
…ke all of them Four jobs (lint, typecheck, architecture, test) each repeated the SAME seven setup steps and differed only in the gate at the end. That duplication was not just slow, it was the bug: the shared-provisioning step was wrong in all four copies at once. The provisioning bug. `shared` is an npm-WORKSPACES monorepo - the lockfile and the build tool (tsup) live at the root, and packages/sync declares neither. The step ran `npm ci` INSIDE packages/sync, which failed (no lockfile there), fell through to `npm install`, pulled the member's five runtime deps, and left the build to die on `sh: 1: tsup: not found`, exit 127. Every gate then failed on the same unresolvable module wearing a different hat. Fixed by installing at the workspace root; verified locally that a member build then resolves tsup from the root and emits dist/. Two more things the duplication was hiding: - the `lint` job had no `env: PRO_SUBMODULE_PAT` at all, so it never provisioned shared or the pro submodule - it was passing without them - only @offgrid/sync was ever built. This app consumes @offgrid/rag too, and its dist/ was never produced Setup now happens once and every gate runs as a step against it. Each gate carries `if: !cancelled() && steps.install.outcome == 'success'`, so a red gate does NOT hide the ones after it - the whole list still reports, which is the one thing four separate checks were genuinely good at, and the job still fails if any gate does. Node is pinned to 26 for all of them now (the test gate already required it: node:sqlite's native teardown segfaults under jest --forceExit on 24, and the module is flag-gated on 22). The runner is macos-latest because SwiftLint and the iOS tests need it. Trade-off, stated plainly: the gates no longer run in parallel, so wall-clock is longer. Checked first that no required status checks are configured by name on main, so dropping the four check names cannot block a PR.
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)
.github/workflows/ci.yml (1)
100-119: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHandle fork installs when the shared monorepo is absent.
@offgrid/ragand@offgrid/syncare regularfile:../shared/packages/*dependencies. The checkout/build step for../sharedruns only whenPRO_SUBMODULE_PATis set, so fork PRs reachnpm ci, fail to resolve those local packages, and stop the job before any gate runs. Use a public stand-in for forks or skip the JS gates explicitly when the secret is absent.Also,
steps.install.outcome == 'success'is dead for install failures:Install dependencieshas nocontinue-on-error, so a failednpm ciexits the job before later gate steps are reached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 100 - 119, Update the CI workflow’s “Install dependencies” path to handle forks where PRO_SUBMODULE_PAT is absent: provide a publicly available stand-in for the ../shared file dependencies, or explicitly skip the JavaScript gates in that case. Ensure the workflow does not rely on steps.install.outcome after a failing npm ci can already terminate the job; use conditional setup or continue-on-error with an explicit failure/skip flow so later gates behave as intended.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
timeout-minutesto the unified job.The four parallel jobs are now one sequential job that runs lint, typecheck, depcruise, knip, Jest, Gradle, and
xcodebuild. The default 360-minute limit now applies to the whole chain instead of each gate. Onmacos-latest, a hung native step bills at the macOS multiplier for the full window.⚙️ Proposed job timeout
jobs: ci: runs-on: macos-latest + timeout-minutes: 90🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 23 - 25, Set an explicit timeout-minutes value on the unified ci job definition alongside runs-on, covering the entire sequential lint, typecheck, depcruise, knip, Jest, Gradle, and xcodebuild workflow within an appropriate bounded limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 100-119: Update the CI workflow’s “Install dependencies” path to
handle forks where PRO_SUBMODULE_PAT is absent: provide a publicly available
stand-in for the ../shared file dependencies, or explicitly skip the JavaScript
gates in that case. Ensure the workflow does not rely on steps.install.outcome
after a failing npm ci can already terminate the job; use conditional setup or
continue-on-error with an explicit failure/skip flow so later gates behave as
intended.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 23-25: Set an explicit timeout-minutes value on the unified ci job
definition alongside runs-on, covering the entire sequential lint, typecheck,
depcruise, knip, Jest, Gradle, and xcodebuild workflow within an appropriate
bounded limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6f00c09-7840-442c-82ea-07d82145d159
📒 Files selected for processing (1)
.github/workflows/ci.yml
…ity in this repo
Removes every reference to the Provit repo. Two kinds of reference, handled differently.
The one real dependency: scripts/physical-sync/iosKnowledgeSyncAdapter.mjs imported WdaActor
from '../../../provit/src/ios/wdaActor.ts' - a relative path OUTSIDE this repo, so it could
only ever resolve on a machine that happened to have the sibling checkout. It was broken
anywhere else, CI included.
Rather than delete the capability, it moves in:
- scripts/ios/wda-client.mjs - a small dependency-free WebDriverAgent client with the eight
methods the harness actually uses (isReady, session, windowSize, screenshot, source, tap,
swipe, back) plus findByLabel/tapLabel/type. Locate by accessibility label, tap the true
centre, so a layout change moves the target instead of breaking the test.
- scripts/ios/launch-wda.mjs - the WDA-server recipe, ported with the seven hard-won facts
intact (generic iOS destination or it hangs on "Device is busy"; sign with the app's team;
neutralise the cosmetic icon post-action or build-for-testing aborts on Xcode 26; install
via devicectl; launch via test-without-building; no tunnel needed; keep the phone unlocked).
It now discovers the attached device instead of hardcoding one UDID, and errors with the
install command when WebDriverAgent is absent.
Everything else was prose: ~24 test files and 8 docs used "Provit" as the NAME of the
on-device test plan and journey engine ("Provit case 17", "Provit plan lines 1409-1549",
"(Provit) - jest cannot prove the SIGKILL"). Those now say what they mean - "device case 17",
"on-device test-plan lines", "(device-only)" - so the distinction the comments were drawing
survives without pointing at a repo that is going away.
Verified: 0 mentions left, the harness's own node:test suite passes, all 72 suites containing
edited comments pass (254 tests), eslint and tsc clean.
A drifted lock file should degrade, not break. The fallback is back but at the WORKSPACE ROOT rather than inside packages/sync, so it still provides tsup - which is the whole reason the member-level version was useless. Prompted by a real drift: five packages were committed to shared without regenerating its lock file, so a fresh checkout resolved fifteen workspaces against a lock that knew nine and npm ci refused. That lock is fixed in shared now; this is the belt to its braces.
Proof-of-approach, not a finished suite: can a small WDA/adb harness replace a framework, and can it produce coverage. Both answered yes, on real hardware. What it is. Two clients with a deliberately identical surface - scripts/ios/wda-client.mjs over WebDriverAgent and scripts/android/adb-client.mjs over adb - so ONE test file drives an iPhone and a OnePlus unchanged. The runner and assertions are node:test and node:assert, already used in this repo. No Appium server, no drivers, no version matrix. The part that makes it usable rather than a toy is waitFor(): a test waits for the screen it expects instead of sleeping a guessed interval. Detox gets this from the RN bridge; a black-box driver has to poll, and Appium would not have given it either. Six sharp edges found by running it, each now handled and commented with what it looked like: - the notification shade covers the app, and uiautomator then describes SystemUI - so source() collapses it and re-reads, because on a real phone notifications arrive mid-run - `uiautomator dump` fails with "could not get idle state" on this app (a live indicator means it never idles) - only --compressed works, and it exits 0 either way, so the file's absence is the only signal - that failure left the PREVIOUS dump in place and the old code read it: silently stale UI, the worst failure mode a driver can have. It now throws instead - scroll momentum: a row's centre read mid-fling is stale by the time the tap lands, so the tap hits nothing and the screen just does not change. waitForStable() waits for two identical reads - Android's compressed dump contains only rendered nodes, so anything below the fold does not exist until scrolled to. iOS returns the whole tree. scrollToLabel() covers the difference - a run inherited whatever screen the last one left. restart() gives a known start state Coverage, proved but not wired end to end. Hermes has no V8 coverage API, so babel.config.js gains an E2E_COVERAGE=1 gate that adds babel-plugin-istanbul - verified both ways: the default build contains no __coverage__, the gated build counts branches. scripts/e2e/collect-coverage.mjs reads the counters out of the running app over Metro's Hermes CDP, needing no app code at all, and exits 1 with instructions rather than writing an empty report that would read as 0%. Status: 4 of 5 assertions green on Android with screenshots from both devices. One is skipped, not hidden - the Files row's tap does not navigate on Android while Activity, the same component one row above, does. Root cause is likely that React Native does not expose testID on a TouchableOpacity to Android's accessibility tree (it does on a Text), so that row can only be reached through its synthesised label. The fix is a src change and is proposed, not made.
Correcting a wrong conclusion. I reported that React Native does not expose testID to Android's
accessibility tree for touchables. It does - as a node's resource-id, on the Button. What hid it
was this driver's own search:
const text = `${node.label || node.name || node.value || ''}`
|| short-circuits, so a node carrying BOTH a synthesised content-desc (built from its children)
and a resource-id was only ever searched by the description. Every testID on an accessible
container was invisible, on both platforms. findByLabel and labels() now read all three fields,
and the matched field is what comes back, so a search by testID returns the testID.
With that fixed the suite targets testIDs throughout - open-sync-from-home, sync-open-activity,
sync-open-files - and no longer depends on copy, which gets rewritten and translated. No src
change was needed after all.
Two more device realities handled while proving it:
- the touch target is the touchable, not the Card around it: sync-home-card is a container and
tapping it does nothing, open-sync-from-home is the button
- an element in the bottom band of the screen sits in the system's gesture-navigation strip,
which swallows the tap. scrollAndTap nudges the list up first
Android: 4 pass, 1 skipped. The skip is a suspected PRODUCT bug written up for manual
reproduction - tapping the Files row does not navigate, while Sharing and Activity do, with the
driver doing the identical thing to all three.
Single-device tests can only prove a screen renders, which the jest suites already do faster. Everything that makes this product what it is happens BETWEEN devices, so the harness now drives two at once. scripts/e2e/mesh.mjs hands a test both drivers plus the primitives that a two-device test actually needs: - both(): run the same thing on both in parallel, failing with WHICH device could not manage it - converge(): poll both until each satisfies its OWN condition. Asymmetry is the point - after pairing one device shows "1 connected" and the other shows the first device's name; after a transfer one says Sent and the other Received. Asserting the same string on both is how a two-device test ends up either wrong or vacuous. On timeout it names the device that did not get there, because "the phone never showed the Mac as connected" is a diagnosis and "timeout" is not - captureBoth(): both screens at the same moment, which is what you want when a two-device run fails - the interesting thing is usually what the OTHER device was showing Devices are addressed as roles (a, b) rather than platforms, so a pairing test reads the same whichever way round the hardware is. They are restarted sequentially, not in parallel: two cold starts contend for one USB bus and one Metro, and a slow launch reads as a broken app. __tests__/device/meshPairing.e2e.mjs is the first journey: both open, both reach Devices, each shows its OWN pairing code (asserted different - one shared code would make the code useless as confirmation of which device is pairing), each discovers the other over real mDNS, and - the invariant the desktop projection bug broke - neither device claims a relationship the other denies. Passes on the iPhone and the OnePlus together, 39s.
The dead-code gate caught two things and was right about both: - babel-plugin-istanbul reads as unused because it is referenced by NAME inside babel.config.js, and only when E2E_COVERAGE=1, so there is no import for knip to follow. It is genuinely used: the instrumented e2e build depends on it. - xcrun is a macOS system binary that scripts/ios/launch-wda.mjs shells out to. It is not, and cannot be, an npm dependency. Only those two. adb and xcodebuild were also added at first and knip pointed out they were not needed - it only inspects binaries invoked from package.json scripts, and the clients call those through execFile. Removed rather than left as noise in the ignore list.
…eenshots The submodule pointer still referenced 6cb796e while mobile-pro's branch has moved on to c44a5b5 (all pushed). Without the bump, a fresh clone of this branch checks out pro at a commit that predates the work this branch depends on. Also lands the iOS screenshots from the device suite; the Android ones went in with the harness and these were regenerated after.
Four findings from the review, three of them mine and all fair. The sweep summed calls across EVERY handler, so it would pass a Retry button wired to a dismiss handler - which is exactly the class of defect it was written to rule out. It now groups handlers by verb and requires a press to reach a handler for THAT verb and none belonging to another. Grouped by verb rather than mapped per row so the test does not re-encode the projection's routing table. All eight cases still pass, so the production wiring was right; the test now proves it instead of assuming it. Queued.png was in the fixture but had no entry in the expectations map, so the test claiming to cover "every state a transfer can be in" silently skipped it. Asserted now. The pro suites decided availability in beforeAll, after jest had registered their cases - so an open-core checkout reported no-op cases as PASSED rather than skipped, looking like it verified the pro surface having run nothing. That is the same silent-green trap requirePro was written to close, one level up. proIsPresent() is synchronous and file-system based so the choice between describe and describe.skip happens at module level, where it has to. And the CI point: `npm ci || npm install` masked a lock mismatch by resolving a different graph and letting the later gates run against dependencies nobody committed. The drift it papered over was real - five packages committed to shared without regenerating its lock - and is fixed at the source, so the fallback is gone from both repos. A drifted lock should stop the build. 100 pro tests pass, tsc and eslint clean.
InputStream.skip returns how many bytes it actually skipped and is free to skip fewer than asked. That return value was discarded, so on a RESUMED upload the read cursor could sit earlier in the file than intended while the frame loop below carried on filling every frame - meaning the short-read guard never fired. The uploader then sealed bytes from one position under a frame index claiming another, and the receiver either failed GCM authentication or wrote a file of the right SIZE with the wrong contents. Only on resume, which is the hardest place to notice it. RandomAccessFile.seek is absolute, and RandomAccessFile.read(ByteArray, Int, Int) has the same contract as the stream read, so the frame loop is unchanged. iOS has always done it this way (BlobChannelUploader.swift and BlobChannelServer.swift both seek(toFileOffset:)); Android was the one platform out of step. Found by review, not by a test, and the test gap is the real story: all four Kotlin blob journeys pass offset = 0, so the resume path has never been exercised. The gap is now written where someone will look for it, along with the honest caveat that such a test would probably NOT have caught THIS bug - skip() on a local file normally does advance the full amount - and that writing it needs host-side plumbing, because the harness's desktop side always starts from an empty destination. Compiles clean (:app:compileDebugKotlin).
|
…pass it Addresses the one unresolved review comment from #624. requirePro returns undefined and this suite decided availability in beforeAll - after jest had already registered its cases - so an open-core run without the private submodule reported ten no-op cases as PASSED. That is the worst of the three outcomes: it claims the Receiving section is covered when nothing ran. It now selects describe.skip from the synchronous proIsPresent() predicate, which is what its siblings (sharedFilePreview, transferActivitySection, explicitFileShare) already do. All four are consistent now.
1 participant
What this is
The mobile side of ambient directory sharing and per-device receiving, plus the work needed to make this branch
pushable at all — it had 264 commits and had never been pushed, because five separate gates were failing.
New-code coverage
Coverage of the lines this branch ADDS (not of whole files it touched), measured with
shared/scripts/new-code-coverage.mjs:611 suites, 8781 tests green.
COVERAGE.mdat the workspace root carries the ecosystem view.The gates, and what each one was actually hiding
Worth reading, because two were real defects rather than lint noise:
java.time.Instantneeds API 26;minSdkis 24. Three Kotlin sync modules(
ScreenshotWatcher,SyncDirectorySourceModule,SyncDownloadsModule) would have thrownNoSuchMethodError, each on the ambient-sharing path formatting acreatedAtthat goes to another device.Fixed with core library desugaring rather than hand-rolled formatting, because those strings are compared
across devices and
SimpleDateFormatwould have to reproduceInstant's exact ISO-8601 shape.BlobUploader.RequestandBlobServer.Pendingboth gained a resumeoffset; the E2E test predated it. Four call sites, all0L, since each journey sends or receives a wholepayload.
src,pro/and the tests first — thearchitecture gate had just flagged
coalesce.tsas an orphan whenpro/imports it and the cruiser cannotsee
pro/, so "the tool says unused" is not sufficient evidence.Two tests left skipped, with the reason in the file
PersonalMeshDeviceEvictionCoordinator.evict()announces the registry change BEFORE finalising itstransaction, and on mobile that announcement runs reconciliation, which finalises the transaction the caller is
still holding. Diagnosed in
docs/GAPS_BACKLOG.md; the fix is a src change awaiting a decision.registration, not only replacements, so activation refuses even with a free seat when no runtime is up. Honest
behaviour; the harness needs to be able to start one.
Notable new coverage
The receiving scope is worth review: the same switch routes to a global handler or a per-device one depending
on the selected scope, and wiring it wrong is silent — turn off screenshots from one laptop and stop accepting
them from everything. The tests press real buttons and assert which callback fires with which arguments.
Summary by CodeRabbit
New Features
Bug Fixes