fix(attachments): restore missing cached files#358
Open
ThunderTr77 wants to merge 3 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ThunderTr77, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
github-actions
Bot
added
the
area:agent-service
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR ensures attachment cache files are present when reusing database records by adding a conditional cache-restoration helper and refactoring record persistence; tests verify both restoration when missing and reuse when present. ChangesAttachment cache restoration when reusing existing database records
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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
`@apps/desktop/tests/services/AgentService/infrastructure/attachment-storage.test.ts`:
- Around line 157-173: The test should assert that the existing attachment
record was actually reused: call ensurePersistedAttachmentIndex and capture its
return value, then assert the returned id equals the mocked record id (43) and
also assert createAttachmentRecord (or its mock, e.g.,
createAttachmentRecordMock) was not called, in addition to the existing
expect(copyFileMock).not.toHaveBeenCalled(); this makes reuse explicit—update
the test using the existing mocks findAttachmentByHashMock, copyFileMock, and
createAttachmentRecordMock and verify returned id and lack of
createAttachmentRecord invocation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1cc5dfb-668e-4d51-b9a5-938b8a030957
📒 Files selected for processing (2)
apps/desktop/src/services/AgentService/infrastructure/attachments/storage.tsapps/desktop/tests/services/AgentService/infrastructure/attachment-storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (rust)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Frontend Quality
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
🔇 Additional comments (4)
apps/desktop/src/services/AgentService/infrastructure/attachments/storage.ts (2)
121-127: LGTM!
142-149: LGTM!apps/desktop/tests/services/AgentService/infrastructure/attachment-storage.test.ts (2)
6-126: LGTM!
128-155: LGTM!
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
Related issue or RFC
AI assistance disclosure
Testing evidence
corepack pnpm --dir apps/desktop exec vitest run --configLoader runner tests/services/AgentService/infrastructure/attachment-storage.test.tspassed.corepack pnpm --dir apps/desktop exec eslint src/services/AgentService/infrastructure/attachments/storage.ts tests/services/AgentService/infrastructure/attachment-storage.test.tspassed.corepack pnpm --dir apps/desktop exec prettier --check src/services/AgentService/infrastructure/attachments/storage.ts tests/services/AgentService/infrastructure/attachment-storage.test.tspassed.corepack pnpm --dir apps/desktop run test:typecheckpassed.git diff --checkpassed.pnpm test:prwas not run locally; targeted checks cover the changed attachment storage path and CI should run the full suite.Did you follow TDD (test-first) for feature and fix work? A regression test was added with the fix.
Risk notes
AgentService, runtime, MCP, or schema impact: Attachment storage behavior only; no transport, runtime, MCP, or schema change.Screenshots or recordings
Not UI-facing.
Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC. (N/A: no boundary change.)pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence. (N/A.)pnpm test:e2elocally or documented why CI is the first valid proof. (N/A.)