Fix media download error handling and improve code organization#1248
Open
vl-ivanov wants to merge 4 commits into
Open
Fix media download error handling and improve code organization#1248vl-ivanov wants to merge 4 commits into
vl-ivanov wants to merge 4 commits into
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarMediaDownload
participant NotificationSystem
User->>SidebarMediaDownload: Initiate media download
SidebarMediaDownload->>SidebarMediaDownload: Check content length and reader
alt Content length or reader is nil or invalid
SidebarMediaDownload->>NotificationSystem: Add negative notification ("Downloading media failed")
SidebarMediaDownload-->>User: Return early, abort download
else Valid content length and reader
SidebarMediaDownload->>SidebarMediaDownload: Proceed with download, show progress notification
SidebarMediaDownload->>NotificationSystem: Update notification on progress
SidebarMediaDownload-->>User: Download completes successfully
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)ui/src/components/sidebar/MediaSidebar/MediaSidebar.test.tsx(0 hunks)ui/src/components/sidebar/SidebarDownloadMedia.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- ui/src/components/sidebar/MediaSidebar/MediaSidebar.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: DanielOaks
PR: photoview/photoview#1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:44-57
Timestamp: 2025-04-13T05:39:39.494Z
Learning: For Photoview PRs, feature enhancements like improved error logging should be suggested as separate PRs rather than being included in dependency update PRs, which should focus on minimal necessary code changes.
Learnt from: googollee
PR: photoview/photoview#1228
File: dependencies/build_imagemagick.sh:0-0
Timestamp: 2025-06-27T13:17:26.027Z
Learning: In the photoview project, when doing rollbacks/reverts, googollee prefers to keep the revert clean and address any code quality issues in separate PRs rather than mixing fixes with the revert itself.
Learnt from: DanielOaks
PR: photoview/photoview#1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:101-115
Timestamp: 2025-04-13T05:38:45.180Z
Learning: Error handling improvements and code style changes should be addressed in dedicated PRs rather than bundled with dependency update PRs which should remain focused on minimal changes required for the updates.
Learnt from: kkovaletp
PR: photoview/photoview#1165
File: ui/package.json:62-62
Timestamp: 2025-01-27T19:55:21.158Z
Learning: Updating vite-plugin-pwa to match workbox v7 dependencies in the Photoview project is planned for a future PR and should not be included in the current PWA implementation changes.
Learnt from: kkovaletp
PR: photoview/photoview#1200
File: api/scanner/media_encoding/encode_photo.go:39-39
Timestamp: 2025-05-09T17:59:05.683Z
Learning: In PhotoView, when removing a feature like thumbnail preferences, it's important to clean up all related code including GraphQL schema definitions, resolvers, UI components, tests, and auto-generated TypeScript files. After removing GraphQL schema definitions, always regenerate TypeScript types with `npm run genSchemaTypes` to remove enum types and operation files that are no longer needed. Don't forget to check for references in global type definitions like `globalTypes.ts`.
Learnt from: kkovaletp
PR: photoview/photoview#1200
File: api/scanner/media_encoding/encode_photo.go:39-39
Timestamp: 2025-05-09T17:59:05.683Z
Learning: In PhotoView, when removing a feature like thumbnail preferences, all related code should be completely removed including GraphQL schema definitions, resolvers, UI components, tests, and generated TypeScript files. TypeScript definitions should be regenerated with `npm run genSchemaTypes` after removing GraphQL schema definitions to ensure all references are removed.
.gitignore (3)
Learnt from: kkovaletp
PR: photoview/photoview#1239
File: Dockerfile:23-29
Timestamp: 2025-07-05T19:01:23.736Z
Learning: In the Photoview project's UI package.json, build tools like Vite are placed in the `dependencies` section rather than `devDependencies`, allowing production builds to work correctly with `npm ci --omit=dev` while still having access to build tools during the Docker build stage.
Learnt from: kkovaletp
PR: photoview/photoview#1239
File: Dockerfile:23-29
Timestamp: 2025-07-05T19:01:23.736Z
Learning: In the Photoview project's UI package.json, build tools like Vite are placed in the `dependencies` section rather than `devDependencies`, allowing production builds to work correctly with `npm ci --omit=dev` while still having access to build tools during the Docker build stage.
Learnt from: kkovaletp
PR: photoview/photoview#1221
File: .github/workflows/build.yml:132-137
Timestamp: 2025-06-21T11:30:57.197Z
Learning: In the Photoview repository, cache scopes in GitHub Actions workflows are intentionally named consistently across different workflow files (build.yml and tests.yml) to enable cache reuse between workflows when dependency files match. The "test-ui" and "test-api" prefixes are used strategically for this cross-workflow cache optimization.
ui/src/components/sidebar/SidebarDownloadMedia.tsx (1)
Learnt from: kkovaletp
PR: photoview/photoview#965
File: ui/src/components/sidebar/SidebarDownloadMedia.tsx:58-58
Timestamp: 2025-02-07T18:49:05.058Z
Learning: In the Photoview frontend, message notifications can include an optional `onDismiss` callback property, which is primarily used for progress notifications to handle cancellation of ongoing operations.
🧬 Code Graph Analysis (1)
ui/src/components/sidebar/SidebarDownloadMedia.tsx (3)
ui/src/helpers/utils.ts (1)
isNil(38-40)ui/src/components/messages/Messages.tsx (1)
MessageState(27-46)ui/src/components/sidebar/MediaSidebar/MediaSidebar.tsx (1)
MediaSidebarMedia(242-270)
⏰ 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). (8)
- GitHub Check: CodeQL (go)
- GitHub Check: CodeQL (javascript)
- GitHub Check: Build Docker Image (refs/pull/1248/merge)
- GitHub Check: Build Dependencies Image
- GitHub Check: Test API (sqlite)
- GitHub Check: Test API (mysql)
- GitHub Check: Test API (postgres)
- GitHub Check: Test UI
🔇 Additional comments (5)
.gitignore (1)
45-46: LGTM! Appropriate exclusions for build artifacts.Adding test reports and build artifacts to
.gitignorefollows best practices for keeping generated files out of version control.ui/src/components/sidebar/SidebarDownloadMedia.tsx (4)
14-16: LGTM! Clean import organization.The import reordering and addition of the
isNilutility are appropriate improvements to code organization.
77-77: LGTM! Simplified variable declaration.Removing the unnecessary
nullinitialization is a good cleanup since the variable gets assigned conditionally later.
183-185: LGTM! Cleaner return statement.Directly returning the
Blobinstead of assigning to an intermediate variable improves code readability.
259-259: LGTM! Fixed the typo in type name.Correcting
SidebarMediaDownladPropstoSidebarMediaDownloadPropsfixes the spelling error and improves code consistency.Also applies to: 263-263
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1248 +/- ##
==========================================
+ Coverage 58.23% 58.70% +0.46%
==========================================
Files 205 205
Lines 17957 17968 +11
Branches 564 580 +16
==========================================
+ Hits 10458 10548 +90
+ Misses 7114 7035 -79
Partials 385 385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/components/sidebar/SidebarDownloadMedia.test.tsx(1 hunks)ui/src/components/sidebar/SidebarDownloadMedia.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/sidebar/SidebarDownloadMedia.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DanielOaks
PR: photoview/photoview#1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:44-57
Timestamp: 2025-04-13T05:39:39.494Z
Learning: For Photoview PRs, feature enhancements like improved error logging should be suggested as separate PRs rather than being included in dependency update PRs, which should focus on minimal necessary code changes.
Learnt from: googollee
PR: photoview/photoview#1228
File: dependencies/build_imagemagick.sh:0-0
Timestamp: 2025-06-27T13:17:26.027Z
Learning: In the photoview project, when doing rollbacks/reverts, googollee prefers to keep the revert clean and address any code quality issues in separate PRs rather than mixing fixes with the revert itself.
Learnt from: DanielOaks
PR: photoview/photoview#1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:101-115
Timestamp: 2025-04-13T05:38:45.180Z
Learning: Error handling improvements and code style changes should be addressed in dedicated PRs rather than bundled with dependency update PRs which should remain focused on minimal changes required for the updates.
Learnt from: kkovaletp
PR: photoview/photoview#1165
File: ui/package.json:62-62
Timestamp: 2025-01-27T19:55:21.158Z
Learning: Updating vite-plugin-pwa to match workbox v7 dependencies in the Photoview project is planned for a future PR and should not be included in the current PWA implementation changes.
Learnt from: kkovaletp
PR: photoview/photoview#1200
File: api/scanner/media_encoding/encode_photo.go:39-39
Timestamp: 2025-05-09T17:59:05.683Z
Learning: In PhotoView, when removing a feature like thumbnail preferences, it's important to clean up all related code including GraphQL schema definitions, resolvers, UI components, tests, and auto-generated TypeScript files. After removing GraphQL schema definitions, always regenerate TypeScript types with `npm run genSchemaTypes` to remove enum types and operation files that are no longer needed. Don't forget to check for references in global type definitions like `globalTypes.ts`.
ui/src/components/sidebar/SidebarDownloadMedia.test.tsx (2)
Learnt from: kkovaletp
PR: photoview/photoview#965
File: ui/src/components/messages/MessageProgress.tsx:14-22
Timestamp: 2024-12-14T20:19:15.719Z
Learning: In the `MessageProgress` component (`ui/src/components/messages/MessageProgress.tsx`), avoid extracting constants (e.g., thresholds, colors) unless they are reused elsewhere to keep the code concise and maintainable.
Learnt from: kkovaletp
PR: photoview/photoview#965
File: ui/src/components/messages/Message.tsx:10-11
Timestamp: 2024-12-14T14:58:22.663Z
Learning: In `ui/src/components/messages/Message.tsx`, the `negative` and `positive` props in the `MessageProps` type are part of the contract and should be retained as-is.
🧬 Code Graph Analysis (1)
ui/src/components/sidebar/SidebarDownloadMedia.test.tsx (4)
ui/src/components/messages/Messages.tsx (1)
MessageState(27-46)ui/src/components/sidebar/SidebarDownloadMedia.tsx (1)
downloadMediaShowProgress(306-306)api/graphql/models/generated.go (1)
NotificationType(196-196)ui/src/components/messages/SubscriptionsHook.ts (1)
Message(24-36)
⏰ 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). (8)
- GitHub Check: CodeQL (javascript)
- GitHub Check: CodeQL (go)
- GitHub Check: Build Docker Image (refs/pull/1248/merge)
- GitHub Check: Test API (mysql)
- GitHub Check: Test API (postgres)
- GitHub Check: Test API (sqlite)
- GitHub Check: Test UI
- GitHub Check: Build Dependencies Image
- Added positive scenario
Contributor
Author
|
@kkovaletp please, review :) |
kkovaletp
reviewed
Jul 19, 2025
| yarn-error.log* | ||
| .eslintcache | ||
| ui/junit-report.xml | ||
| api/photoview |
Contributor
There was a problem hiding this comment.
What is the api/photoview and why should it be globally ignored?
If it is something for the local API server execution, it is better to move it under the already ignored folders:
/storage
/database
/tmp
kkovaletp
reviewed
Jul 19, 2025
kkovaletp
reviewed
Jul 19, 2025
Comment on lines
+52
to
+62
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| expect(MessageState.add).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: NotificationType.Close, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| props: expect.objectContaining({ | ||
| negative: true, | ||
| header: 'Downloading media failed', | ||
| }), | ||
| }) | ||
| ) |
Contributor
There was a problem hiding this comment.
Are these ESLint inline mutters the only way to implement this verification logic? As far as I know, in most cases when you need to mute a linter, there should be an exceptional need. If that is it - ok. I'm asking just to make sure)
kkovaletp
reviewed
Jul 19, 2025
Comment on lines
+114
to
+115
| content: `The content length of the downloaded media is 0 bytes, which has no sense and usually | ||
| means that there is an unknown lower-level error.`, |
Contributor
There was a problem hiding this comment.
From this message, I see that you've checked my repo's implementation) I see that you've enhanced it by covering more negative cases and adding tests.
I have more messages defined there, but I guess that implementing them would require more complex changes...
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
2 participants
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
Bug Fixes
Style
Chores
.gitignoreto exclude specific test reports and directories from version control.Tests