Open
Conversation
Wire OutboxProcessor with journalDb + documentsDirectory in OutboxService so bundling actually activates in production. Extend _maybeProcessBundle per-item failure path with retry-cap tracking so poison bundled items surface retryCapReached like the head-only flow does. Reuse JSON bytes the enumerator already read for media-path discovery (AttachmentDescriptor now carries a bytes field) so the bundler does not re-read the same file, and parallelize media reads via Future.wait. Move zip encoding and decoding onto worker isolates so multi-MiB bundles do not block frame work on the sync isolate. Extract _shouldSkipExistingFile so the single-file and bundle-entry write paths share the non-agent dedup rule, drop the unused bundleMaxBytesOverride field, fold AttachmentDescriptor into a record, and introduce the attachmentBundleDirPrefix constant.
|
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:
📝 WalkthroughWalkthroughAdds an optional outbox attachment bundling feature ( Changes
Sequence Diagram(s)sequenceDiagram
participant Outbox as OutboxProcessor
participant Enum as AttachmentEnumerator
participant Encoder as ZipEncoder/Isolate
participant Sender as MatrixMessageSender
participant Room as MatrixRoom
Outbox->>Outbox: Check useBundledAttachmentsFlag
Outbox->>Enum: enumerateAttachments(item1)
Enum-->>Outbox: descriptors
Outbox->>Enum: enumerateAttachments(item2)
Enum-->>Outbox: descriptors
Outbox->>Outbox: Coalesce entries (≤8MiB)
Outbox->>Encoder: _encodeBundleBytes(entries)
Encoder-->>Outbox: zipped bytes
Outbox->>Sender: sendAttachmentBundle(entries)
Sender->>Room: sendFileEvent(.bundles/{bundleId}.zip, com.lotti.bundle=true)
Room-->>Sender: eventId
Sender-->>Outbox: eventId
Outbox->>Sender: send(item1, skipAttachmentPaths={paths})
Sender-->>Outbox: success
Outbox->>Sender: send(item2, skipAttachmentPaths={paths})
Sender-->>Outbox: success
Outbox->>Outbox: Mark items sent
sequenceDiagram
participant Room as Matrix/Network
participant Ingestor as AttachmentIngestor
participant Decoder as ZipDecoder/Isolate
participant FS as FileSystem
Room->>Ingestor: Attachment event (com.lotti.bundle: true)
Ingestor->>Decoder: _decodeBundleEntries(zipBytes) via isolate
Decoder-->>Ingestor: Map<relativePath, bytes>
loop For each entry
Ingestor->>Ingestor: Path traversal check
Ingestor->>Ingestor: Dedupe check
alt Safe & writable
Ingestor->>FS: atomicWriteBytes(targetPath)
FS-->>Ingestor: written
else Blocked or exists
Ingestor-->>Ingestor: log skip
end
end
Ingestor-->>Room: Process complete (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces outbox attachment bundling, allowing multiple pending sync items' files to be packed into a single zip archive (up to 8 MiB) and uploaded as one Matrix file event. This optimization reduces round-trips over slow connections. The implementation includes a new attachment enumerator, updates to the outbox processor to handle bundling logic, and support in the attachment ingestor for unpacking bundles on the receiving side. Review feedback highlighted the need for better error handling during zip encoding and decoding within worker isolates, as well as ensuring type safety when handling archive entry content.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/sync/matrix/pipeline/attachment_ingestor.dart (1)
104-109:⚠️ Potential issue | 🟠 MajorHandled bundle events will be re-downloaded on every later scan.
For bundle events,
rpAnypoints at the outer.bundles/...zip, but_saveAttachment()never writes that file to disk. After the first successful unpack,_isLocalFileMissingOrEmpty(rpAny)therefore staystrue, so every subsequent catch-up/live-scan pass treats the same event as needing repair and downloads the whole bundle again. Please make the repair probe bundle-aware instead of keying it off the unsaved outer zip path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart` around lines 104 - 109, The current repair probe uses rpAny (the outer .bundles/...zip path) with _isLocalFileMissingOrEmpty, but _saveAttachment never writes that outer zip so the probe always returns true and triggers re-downloads; change the shouldRepairLocal logic to detect bundle events (e.g., rpAny containing ".bundles/") and, for those cases, probe the actual saved target produced by unpacking (the normalized inner key or the unpack directory/marker that _saveAttachment writes) instead of rpAny — call _isLocalFileMissingOrEmpty on that inner/unpacked key (and ensure _inFlightSavePaths uses the same normalized inner key) so alreadyHandled bundles are not repeatedly re-downloaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/sync/matrix/attachment_enumerator.dart`:
- Around line 71-77: Normalize and validate the paths before reading files:
after constructing jsonFullPath from documentsDirectory.path and
message.jsonPath, call p.normalize on the joined path and then verify
p.isWithin(documentsDirectory.path, normalizedPath) (or equivalent) to reject
any paths that escape the documents directory; do the same validation for the
other media-path construction used later (the logic around the second file read
at the other location referenced in this file), and only perform
File(...).readAsBytes() when the normalized path is within documentsDirectory,
otherwise return or throw a safe error.
In `@lib/features/sync/matrix/matrix_service.dart`:
- Around line 480-495: The sendAttachmentBundle method bypasses the
unverified-device gate that sendMatrixMsg enforces; before calling
_messageSender.sendAttachmentBundle in sendAttachmentBundle, query the same
unverified-device checker (e.g., call getUnverifiedDevices() or the same
verifier used by sendMatrixMsg) and if any unverified devices are present, log
an event via _loggingService (similar message/context as in sendMatrixMsg) and
return null instead of uploading, so bundle uploads are blocked when devices are
unverified.
In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart`:
- Around line 433-441: The _shouldSkipExistingFile helper currently uses async
file operations with an inline ignore for avoid_slow_async_io; replace the async
calls with synchronous ones to conform to background sync file I/O conventions:
use target.existsSync() instead of await target.exists() and target.lengthSync()
instead of await target.length(), remove the ignore and adjust the method to
return the appropriate bool synchronously while keeping the existing
isAgentPayloadPath(relativePath) check and exception handling in the
_shouldSkipExistingFile function.
In `@lib/features/sync/outbox/outbox_processor.dart`:
- Around line 307-316: The bundling loop (using bundled, enumerated, bundleSize)
incorrectly skips non-bundleable entries and continues scanning, allowing later
outbox items to be sent before earlier pending ones; change the loop so that
upon encountering the first non-bundleable entry — e.atts.isEmpty, total >
SyncTuning.outboxBundleMaxBytes, or bundleSize + total >
SyncTuning.outboxBundleMaxBytes — you break out of the loop and fall back to the
head-only send path instead of continuing to consider later enumerated entries;
ensure the rest of the code uses the resulting bundled list and preserves
head-first ordering for OutboxItem/AttachmentDescriptor processing.
- Around line 368-405: The bundled-item loop lacks a try/catch around the send
operation so an exception can escape and abort processQueue; wrap the
send+timeout call for each refreshed item (the call to
_messageSender.send(...).timeout(...) in the for (final e in bundled) loop) in a
try/catch, and in the catch block mirror the head-only handling: capture the
exception via _loggingService.captureException (include domain/subDomain such as
'OUTBOX'/'bundle.send' and stackTrace), await _repository.markRetry(refreshed),
call _recordBundleItemFailure(refreshed) (include timedOut when applicable), set
anyFailure = true, and continue to the next bundled item so retry bookkeeping
always runs.
In `@lib/features/sync/outbox/outbox_service.dart`:
- Around line 1638-1643: The file is missing the dart:typed_data import required
for the Uint8List type used in the sendAttachmentBundle method; add "import
'dart:typed_data';" to the file imports (so the declaration of Future<String?>
sendAttachmentBundle({ required Map<String, Uint8List> entries, }) and any use
of Uint8List in this file compiles) and ensure the existing method forwarder to
_matrixService.sendAttachmentBundle still references the same signature.
In `@pubspec.yaml`:
- Line 4: Update project metadata to match pubspec.yaml's version: add a new
entry for "0.9.956+3931" to CHANGELOG.md under the current version header (copy
the relevant release notes or summary for 0.9.956 from your release notes) and
update flatpak/com.matthiasn.lotti.metainfo.xml to include the same version
string (0.9.956+3931) in its version/release metadata so both files are
synchronized with the pubspec.yaml version.
In `@test/features/sync/matrix/pipeline/attachment_ingestor_test.dart`:
- Line 1543: The test currently reads the host file File('/etc/passwd') which
couples it to the runner OS; replace that with a temporary, controlled file
created within the test (e.g. via Directory.systemTemp.createTempSync or
similar) containing known content and then assert using
expect(File(tempPath).readAsStringSync(), isNot(contains('pwned'))); locate the
offending call to File('/etc/passwd')/expect and change it to create and use a
temp file (or a fixture in the test's temp dir) so the assertion validates
traversal behavior against controlled data rather than the host filesystem.
---
Outside diff comments:
In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart`:
- Around line 104-109: The current repair probe uses rpAny (the outer
.bundles/...zip path) with _isLocalFileMissingOrEmpty, but _saveAttachment never
writes that outer zip so the probe always returns true and triggers
re-downloads; change the shouldRepairLocal logic to detect bundle events (e.g.,
rpAny containing ".bundles/") and, for those cases, probe the actual saved
target produced by unpacking (the normalized inner key or the unpack
directory/marker that _saveAttachment writes) instead of rpAny — call
_isLocalFileMissingOrEmpty on that inner/unpacked key (and ensure
_inFlightSavePaths uses the same normalized inner key) so alreadyHandled bundles
are not repeatedly re-downloaded.
🪄 Autofix (Beta)
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
Run ID: db5f5681-f6f0-4c64-9c74-5cb3d3a382ac
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
CHANGELOG.mdflatpak/com.matthiasn.lotti.metainfo.xmllib/database/journal_db/config_flags.dartlib/features/sync/README.mdlib/features/sync/matrix/attachment_enumerator.dartlib/features/sync/matrix/consts.dartlib/features/sync/matrix/matrix_message_sender.dartlib/features/sync/matrix/matrix_service.dartlib/features/sync/matrix/pipeline/attachment_ingestor.dartlib/features/sync/outbox/outbox_processor.dartlib/features/sync/outbox/outbox_service.dartlib/features/sync/tuning.dartlib/utils/consts.dartpubspec.yamltest/database/database_test.darttest/features/sync/matrix/attachment_enumerator_test.darttest/features/sync/matrix/matrix_message_sender_test.darttest/features/sync/matrix/pipeline/attachment_ingestor_test.darttest/features/sync/outbox/outbox_processor_test.darttest/features/sync/outbox/outbox_retry_cap_db_test.dart
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2957 +/- ##
========================================
Coverage 92.30% 92.30%
========================================
Files 1104 1105 +1
Lines 81336 81542 +206
========================================
+ Hits 75074 75271 +197
- Misses 6262 6271 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/features/sync/outbox/outbox_processor.dart (1)
313-324: Consider requiring ≥2 items to take the bundle path.When
bundled.length == 1the zip encoding cost (plus isolate hop and receiver unpack) buys nothing over the head-only path — the event count is the same and the single attachment gets wrapped in zip overhead. Gating on a minimum size avoids this:♻️ Proposed gate
- if (bundled.isEmpty) return null; + if (bundled.length < 2) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/sync/outbox/outbox_processor.dart` around lines 313 - 324, The bundling path should only be taken when there are at least two items to justify zip/isolates overhead: after building the list `bundled` (which collects `({OutboxItem item, List<AttachmentDescriptor> atts})` entries from `enumerated` and uses `SyncTuning.outboxBundleMaxBytes`), change the early-return check from `if (bundled.isEmpty) return null;` to require a minimum of two entries (e.g., `if (bundled.length < 2) return null;`) so single-item bundles fall back to the head-only path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart`:
- Around line 378-384: process() currently treats bundle events as "local file
missing" because .bundles/<id>.zip is never persisted, causing shouldRepairLocal
to stay true and re-download bundles every pass; fix by short-circuiting the
repair path for bundle events: inside process(), detect bundle events via
attachmentBundleKey and if the eventId is already in _handledAttachmentEventIds
(or otherwise alreadyHandled), set shouldRepairLocal = false (or return early)
so the code does not enqueue repair/download or call _isLocalFileMissingOrEmpty
for bundles; ensure you still call _unpackBundle only when first handling the
bundle (use _handledAttachmentEventIds, _unpackBundle, and
_shouldSkipExistingFile as needed) so subsequent passes skip re-downloading the
.zip while per-entry dedup remains intact.
---
Nitpick comments:
In `@lib/features/sync/outbox/outbox_processor.dart`:
- Around line 313-324: The bundling path should only be taken when there are at
least two items to justify zip/isolates overhead: after building the list
`bundled` (which collects `({OutboxItem item, List<AttachmentDescriptor> atts})`
entries from `enumerated` and uses `SyncTuning.outboxBundleMaxBytes`), change
the early-return check from `if (bundled.isEmpty) return null;` to require a
minimum of two entries (e.g., `if (bundled.length < 2) return null;`) so
single-item bundles fall back to the head-only path.
🪄 Autofix (Beta)
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
Run ID: 0b5b31a3-a1bd-491e-a3b6-e6fff8da98eb
📒 Files selected for processing (8)
CHANGELOG.mdflatpak/com.matthiasn.lotti.metainfo.xmllib/features/sync/matrix/attachment_enumerator.dartlib/features/sync/matrix/matrix_message_sender.dartlib/features/sync/matrix/matrix_service.dartlib/features/sync/matrix/pipeline/attachment_ingestor.dartlib/features/sync/outbox/outbox_processor.darttest/features/sync/matrix/pipeline/attachment_ingestor_test.dart
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- flatpak/com.matthiasn.lotti.metainfo.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/sync/matrix/matrix_service.dart
- lib/features/sync/matrix/attachment_enumerator.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/features/sync/outbox/outbox_processor_test.dart (1)
1071-1072: Optional: guarddeleteSyncagainst flaky cleanup.
addTearDown(() => tmp.deleteSync(recursive: true))will throw if the directory is already gone or a file is locked (notably on Windows CI), turning a passing test into a red teardown. Wrapping withtry { ... } catch (_) {}or usingtmp.existsSync()guard is a cheap safety net and matches common patterns forsystemTempscratch dirs. Same pattern recurs at lines 1175–1176, 1238–1239, 1312–1313, 1391–1392, 1470–1471, 1562–1563.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/sync/outbox/outbox_processor_test.dart` around lines 1071 - 1072, The teardown currently calls addTearDown(() => tmp.deleteSync(recursive: true)) which can throw if the temp directory is already removed or locked (Windows CI); change the teardown to safely attempt deletion by either wrapping tmp.deleteSync(recursive: true) in a try/catch (catch and ignore any exception) or check tmp.existsSync() before deleting, and apply the same fix for the other tmp tearDowns that create systemTemp directories (the blocks using the tmp variable at the other noted locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/features/sync/outbox/outbox_processor_test.dart`:
- Around line 1559-1629: The test never asserts that the repeated-subject
counters were cleared; add a third tick that fails again for the same subject
and assert the emitted sendFailed diagnostic reports repeats == 1. Concretely:
after Tick 2 (where sendResult = true and proc.processQueue() verified
markSent), set sendResult = false, call await proc.processQueue() again, and
verify the loggingService.captureEvent (MockLoggingService.captureEvent) was
invoked with a payload whose repeats field equals 1 (same pattern used in the
"resets on success for the same subject" test), ensuring the
OutboxProcessor.processQueue() reset of the
_lastFailedSubject/_lastFailedRepeats tracker is exercised.
---
Nitpick comments:
In `@test/features/sync/outbox/outbox_processor_test.dart`:
- Around line 1071-1072: The teardown currently calls addTearDown(() =>
tmp.deleteSync(recursive: true)) which can throw if the temp directory is
already removed or locked (Windows CI); change the teardown to safely attempt
deletion by either wrapping tmp.deleteSync(recursive: true) in a try/catch
(catch and ignore any exception) or check tmp.existsSync() before deleting, and
apply the same fix for the other tmp tearDowns that create systemTemp
directories (the blocks using the tmp variable at the other noted locations).
🪄 Autofix (Beta)
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
Run ID: aeb9b830-37b3-4cea-860a-e007b86048d1
📒 Files selected for processing (4)
test/features/sync/matrix/attachment_enumerator_test.darttest/features/sync/matrix/matrix_service_test.darttest/features/sync/matrix/pipeline/attachment_ingestor_test.darttest/features/sync/outbox/outbox_processor_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/features/sync/matrix/attachment_enumerator_test.dart
Add tests for the uncovered lines codecov flagged in the bundle feature: - MatrixService.sendAttachmentBundle: no-room, unverified-devices, and happy-path delegation. - OutboxProcessor bundle path: head-first break when the first pending item is non-bundleable, per-item send exception triggers markRetry plus bundle.send captureException, retryCapReached diagnostic surfaces when a bundled item's retries hit the cap, and success clears the repeated-subject tracker. - attachment_enumerator path hardening: relative jsonPath with .. escapes and absolute jsonPath pointing outside docs both return empty without reading from disk. - attachment_ingestor: bundle unpack tolerates corrupt zip input and writes nothing (the try/catch around the isolate decode covers the stricter throwing case in archive). Replace the /etc/passwd traversal assertion with an escape target in a sibling dir of the test's temp documents directory so the test no longer couples to host state.
matthiasn
force-pushed
the
feat/bundled_attachments
branch
from
907e497 to
efc9910
Compare
Three more targeted tests for the paths codecov still flagged on commit 907e497: - Bundled-item send timeout: a send() that never completes exercises the onTimeout callback and the timedOut=true branch of the bundle diagnostic so the timeout path isn't dead on coverage. - Enumerator empty JSON: a zero-byte jsonPath returns an empty descriptor list so the bundler never ships a zero-byte payload. - Enumerator unparseable JSON: a readable but non-JournalEntity JSON returns just the base descriptor (no media), covering the catch around JournalEntity.fromJson. Strengthen the existing "bundled item success resets repeated-subject diagnostic counters" test with a third tick that fails again for the same subject, asserting the diagnostic emits repeats=1 (not 2). Without the Tick 2 success clearing the tracker, Tick 3 would report repeats=2 — so this actually exercises the reset line on the success branch instead of just reaching markSent.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/sync/matrix/pipeline/attachment_ingestor.dart (1)
358-394:⚠️ Potential issue | 🟠 MajorDon’t dedupe on the outer
.bundles/*.zippath before unpacking.
_shouldSkipExistingFile(file, relativePath)runs before the bundle check, so if an older app version has already written.bundles/<id>.ziplocally, this method returns early and_unpackBundle()never runs. That breaks the upgrade path: the zip exists, but the real inner attachment files are still missing.Handle
attachmentBundleKeybefore the existing-file dedupe, or treat an existing outer zip as input to unpack rather than proof that the bundle has already been applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart` around lines 358 - 394, The dedupe check using _shouldSkipExistingFile is currently run before detecting bundle payloads, causing existing outer .bundles/*.zip files to prevent _unpackBundle from running; change the flow to check event.content[attachmentBundleKey] first (or, if a local file exists and relativePath is a bundle, treat that existing zip as input) so that bundle handling calls _unpackBundle regardless of _shouldSkipExistingFile; update the logic around event.downloadAndDecryptAttachment()/downloadedBytes and use _unpackBundle(bundleRelativePath: relativePath, downloadedBytes: ..., logging: logging) for bundle cases before returning based on _shouldSkipExistingFile.
♻️ Duplicate comments (1)
test/features/sync/outbox/outbox_processor_test.dart (1)
1872-1942:⚠️ Potential issue | 🟡 MinorAdd a third tick to prove the bundled success really resets the repeat counter.
Right now this only shows
fail -> success. It never fails the same subject again and inspects the nextsendFaileddiagnostic, so the_lastFailedRepeatsreset is still unproven. Please add one more tick withsendResult = falseand assert the next failure for this subject logsrepeats=1.As per coding guidelines: "Every test must assert something meaningful beyond
findsOneWidget; verify content, state changes, callbacks, or error handling."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/sync/outbox/outbox_processor_test.dart` around lines 1872 - 1942, Add a third "tick" after the success to prove the bundled success resets the repeat counter: after the existing success branch (where sendResult = true and proc.processQueue() returned and repo.markSent was verified) set sendResult = false again, call await proc.processQueue(), verify repo.markRetry(item) is called a second time, and assert that loggingService.captureEvent (MockLoggingService.captureEvent) was invoked for this failure with a diagnostic object containing repeats = 1 (or otherwise inspect the captured event argument to ensure repeats==1); reference OutboxProcessor, the sendResult toggle, repo.markRetry(item), and log.captureEvent to locate and implement these assertions.
🧹 Nitpick comments (1)
test/features/sync/matrix/matrix_service_test.dart (1)
87-88: Use shared test doubles for the new Matrix room/device types.Please move
_RoomFake,_MockRoom,_MockDeviceKeys, and the corresponding fallback registration into the shared test helpers instead of adding more inline doubles here. That keeps Matrix test setup consistent and avoids per-file duplication.As per coding guidelines: "In tests, use centralized mocks from
test/mocks/mocks.dart; never define mock classes inline" and "In tests, use centralized fallback values fromtest/helpers/fallbacks.dart."Also applies to: 440-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/sync/matrix/matrix_service_test.dart` around lines 87 - 88, The test defines local doubles `_RoomFake`, `_MockRoom`, `_MockDeviceKeys` and calls `registerFallbackValue(_RoomFake())` inline; move those mock/fake classes and the fallback registration into the shared test helpers so tests reuse centralized doubles. Specifically, remove the inline definitions and fallback registration here and instead import and use the shared mocks from the centralized test helpers (the equivalents provided in `test/mocks/mocks.dart` and `test/helpers/fallbacks.dart`), replacing local references with the shared `_RoomFake`, `_MockRoom`, `_MockDeviceKeys` and ensuring `registerFallbackValue` is called from the centralized setup so no per-file mock/fallback duplication remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart`:
- Around line 104-118: The short-circuit that skips repair for bundle events
(isBundleEvent derived from attachmentBundleKey) causes permanently missing
bundle contents when the initial unpack/download fails because bundle event ids
get added to _handledAttachmentEventIds too early; change the flow so bundle
events are only recorded as handled after a successful unpack/write (i.e., delay
adding to _handledAttachmentEventIds until unpack completes), or alternatively
introduce a separate "bundleApplied" state flag for bundle event ids and keep
the current shouldRepairLocal logic but consult that flag instead of
_handledAttachmentEventIds; update the code paths that set/consult
_handledAttachmentEventIds, the isBundleEvent branch, and any logic that clears
or retries entries in
_inFlightSavePaths/_normalizeKey/_isLocalFileMissingOrEmpty to reflect this new
ordering.
---
Outside diff comments:
In `@lib/features/sync/matrix/pipeline/attachment_ingestor.dart`:
- Around line 358-394: The dedupe check using _shouldSkipExistingFile is
currently run before detecting bundle payloads, causing existing outer
.bundles/*.zip files to prevent _unpackBundle from running; change the flow to
check event.content[attachmentBundleKey] first (or, if a local file exists and
relativePath is a bundle, treat that existing zip as input) so that bundle
handling calls _unpackBundle regardless of _shouldSkipExistingFile; update the
logic around event.downloadAndDecryptAttachment()/downloadedBytes and use
_unpackBundle(bundleRelativePath: relativePath, downloadedBytes: ..., logging:
logging) for bundle cases before returning based on _shouldSkipExistingFile.
---
Duplicate comments:
In `@test/features/sync/outbox/outbox_processor_test.dart`:
- Around line 1872-1942: Add a third "tick" after the success to prove the
bundled success resets the repeat counter: after the existing success branch
(where sendResult = true and proc.processQueue() returned and repo.markSent was
verified) set sendResult = false again, call await proc.processQueue(), verify
repo.markRetry(item) is called a second time, and assert that
loggingService.captureEvent (MockLoggingService.captureEvent) was invoked for
this failure with a diagnostic object containing repeats = 1 (or otherwise
inspect the captured event argument to ensure repeats==1); reference
OutboxProcessor, the sendResult toggle, repo.markRetry(item), and
log.captureEvent to locate and implement these assertions.
---
Nitpick comments:
In `@test/features/sync/matrix/matrix_service_test.dart`:
- Around line 87-88: The test defines local doubles `_RoomFake`, `_MockRoom`,
`_MockDeviceKeys` and calls `registerFallbackValue(_RoomFake())` inline; move
those mock/fake classes and the fallback registration into the shared test
helpers so tests reuse centralized doubles. Specifically, remove the inline
definitions and fallback registration here and instead import and use the shared
mocks from the centralized test helpers (the equivalents provided in
`test/mocks/mocks.dart` and `test/helpers/fallbacks.dart`), replacing local
references with the shared `_RoomFake`, `_MockRoom`, `_MockDeviceKeys` and
ensuring `registerFallbackValue` is called from the centralized setup so no
per-file mock/fallback duplication remains.
🪄 Autofix (Beta)
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
Run ID: 11a3b4a8-aa56-4e04-83a7-50a94a557483
📒 Files selected for processing (6)
lib/features/sync/matrix/pipeline/attachment_ingestor.darttest/features/sync/matrix/attachment_enumerator_test.darttest/features/sync/matrix/matrix_message_sender_test.darttest/features/sync/matrix/matrix_service_test.darttest/features/sync/matrix/pipeline/attachment_ingestor_test.darttest/features/sync/outbox/outbox_processor_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/features/sync/matrix/attachment_enumerator_test.dart
- test/features/sync/matrix/matrix_message_sender_test.dart
The earlier re-download-every-pass fix gated the repair heuristic on !isBundleEvent, which short-circuited bundles once their event id landed in _handledAttachmentEventIds at the top of process(). Because that set is populated before the download+unpack, a first-pass failure (download throws, zip decode throws, etc.) left the id in place and no subsequent catch-up pass would ever retry. The bundled items' text events could not recover: their inner relativePaths were never registered in AttachmentIndex so DescriptorDownloader had nothing to pull, and SyncSequenceLogService saw the counters as received so BackfillRequestService never fired. Introduce _appliedBundleEventIds, populated only after _unpackBundle successfully decodes the zip. For bundle events the repair gate now consults that set instead of _isLocalFileMissingOrEmpty, so the retry path fires until the bundle is applied, then stops. Mirrors the existing _handledAttachmentEventIds LRU bookkeeping to keep memory bounded. Add a regression test that drives three passes through process() for the same bundle event: pass 1 fails mid-download, pass 2 succeeds and persists the inner entry, pass 3 must not trigger another download.
matthiasn
force-pushed
the
feat/bundled_attachments
branch
from
5353118 to
c87d6a4
Compare
Add the config flag to FlagsPage.displayedItems alongside its icon (Icons.inventory_2_outlined), localized title, and description so the toggle is actually reachable from the UI. The gzip flag landed in the page during 0.9.954; the bundle flag was added in 0.9.956 but missed this surface. Add configFlagUseBundledAttachments / ...Description strings to all six ARB files (en, de, fr, es, cs, ro) with informal phrasing per the project convention, and regenerate the l10n Dart bindings via `make l10n`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests