feat: add support for other asset stores than S3 #59
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant changes to the configuration and functionality of the application, primarily focusing on the transition from AWS S3 storage to local disk storage. New environment variables are added to support this transition, while existing variables are updated or removed. Additionally, new Docker Compose configurations are introduced, and various files are modified to reflect the new storage approach. Documentation has been updated to clarify these changes, ensuring that the application can effectively utilize either local file systems or S3-compatible solutions. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (14)
.env.sample (1)
14-16: Consider documenting storage configuration requirements.The file now supports both S3 and local storage, but it's unclear which variables are required for each configuration:
- For S3: AWS_* and S3_* variables
- For local storage: DRIVE_DISK, ASSET_URL_BASE, and FS_ASSET_PATH
Consider adding a comment block explaining the requirements:
+# Storage Configuration +# For S3 storage (DRIVE_DISK=s3): +# Required: AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, S3_REGION, S3_BUCKET +# Optional: S3_ENDPOINT_URL, S3_FORCE_PATH_STYLE +# For local storage (DRIVE_DISK=local): +# Required: FS_ASSET_PATH +# Optional: ASSET_URL_BASE (defaults to HOME_URL if not set) DRIVE_DISK=local # Storage driver to use: 'local' for filesystem or 's3' for S3Also applies to: 17-22
compose-fs.yaml (1)
30-32: Consider enhancing volume configuration for production deployments.While the current volume configuration works, consider the following improvements for production:
- Use specific volume drivers based on your deployment environment (e.g., local, cloud)
- Configure backup strategies
- Set performance options
volumes: postgres_data: + driver: local # Or appropriate cloud driver + driver_opts: + type: none + device: /path/to/postgres/data + o: bind assets_data: + driver: local # Or appropriate cloud driver + driver_opts: + type: none + device: /path/to/assets + o: bindcompose.yaml (1)
Line range hint
47-56: Consider conditional service definition for S3-specific components.Given that the PR aims to support multiple storage backends, the
create-bucketservice is only relevant when using S3/MinIO storage. Consider moving S3-specific services to a separate compose file (e.g.,compose.s3.yaml) that can be included conditionally.This would make the configuration more maintainable and clearer in its intent. Example structure:
compose.yaml: Base configurationcompose.s3.yaml: S3-specific services (minio, create-bucket)compose.fs.yaml: Filesystem-specific configurationYou can then use
docker compose -f compose.yaml -f compose.s3.yaml upordocker compose -f compose.yaml -f compose.fs.yaml updepending on the storage backend.src/index.tsx (2)
5-5: Consider reorganizing storage-related importsThe imports from
"./s3"seem misplaced as they now handle multiple storage backends beyond S3. Consider moving these to a more appropriately named module like"./storage"or"./drive"to better reflect their generalized purpose.-import { DRIVE_DISK, assetPath } from "./s3"; +import { DRIVE_DISK, assetPath } from "./storage";Also applies to: 12-12
16-24: Consider a unified storage abstraction layerInstead of conditionally adding middleware based on storage type, consider implementing a unified storage middleware that internally handles the different storage backends. This would:
- Reduce runtime branching
- Provide a consistent interface for both local and S3 storage
- Make it easier to add new storage backends in the future
- Allow for runtime switching between storage backends
Example approach:
// storage/middleware.ts export const createStorageMiddleware = (config: StorageConfig) => { const storage = createStorageDriver(config); return async (c: Context, next: Next) => { if (c.req.path.startsWith('/assets/')) { return storage.serve(c); } return next(); }; }; // index.tsx app.use(createStorageMiddleware({ type: DRIVE_DISK, path: assetPath }));package.json (1)
29-29: Good choice of storage abstraction library!The addition of
flydriveis an excellent choice for implementing multi-driver storage support. It provides a clean abstraction layer that will allow seamless switching between S3 and local filesystem storage, directly addressing the objectives outlined in issue #10.src/media.ts (1)
6-7: Consider renamings3.tsto better reflect its new purpose.Since this file now handles multiple storage backends beyond just S3, consider renaming it to something more generic like
storage.tsordrive.tsto better reflect its expanded functionality.src/s3.ts (3)
17-19: Remove redundant export ofS3_URL_BASEYou are exporting both
assetUrlBaseandS3_URL_BASE, which have the same value. This redundancy might cause confusion and maintenance overhead. Consider using onlyassetUrlBasethroughout the codebase.Apply this diff to remove the redundant export:
export const assetUrlBase = process.env["ASSET_URL_BASE"]; if (assetUrlBase == null) throw new Error("ASSET_URL_BASE is required"); -export const S3_URL_BASE = assetUrlBase;Ensure that any references to
S3_URL_BASEin the codebase are updated toassetUrlBase.
31-33: ConsolidatedriveDiskandDRIVE_DISKvariablesCurrently, both
driveDiskandDRIVE_DISKhold the same value. This can lead to confusion and inconsistency. It's better to use a single constant for clarity.Apply this diff to simplify the variables:
-export const driveDisk = process.env["DRIVE_DISK"]; +export const DRIVE_DISK = process.env["DRIVE_DISK"]; if (DRIVE_DISK == null) throw new Error("DRIVE_DISK is required"); -export const DRIVE_DISK = driveDisk;
57-57: Handle undefinedS3_FORCE_PATH_STYLEenvironment variableYou're accessing
process.env["S3_FORCE_PATH_STYLE"]without providing a default value. If this environment variable isn't set, it will result inforcePathStylebeingfalse, but it's clearer to make this explicit.Apply this diff to provide a default boolean value:
// biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) - forcePathStyle: process.env["S3_FORCE_PATH_STYLE"] === "true", + forcePathStyle: process.env["S3_FORCE_PATH_STYLE"] === "true" ? true : false,Alternatively, you can use double negation to coerce the value to a boolean:
forcePathStyle: process.env["S3_FORCE_PATH_STYLE"] === "true", + forcePathStyle: !!(process.env["S3_FORCE_PATH_STYLE"] === "true"),src/api/v1/media.ts (2)
10-10: Consider renaming the'../../s3'module to reflect the new storage abstractionNow that the storage mechanism has been generalized beyond S3 using FlyDrive, importing from a module named
'../../s3'may be misleading. Renaming the module to something like'../../storage'or'../../assets'could enhance code clarity and maintainability.
41-41: EnsureassetUrlBaseproperly concatenates withpathWhen constructing the URL, make sure that
assetUrlBaseends with a trailing slash or thatpathdoes not start with a slash to prevent incorrect URL formation.Consider adjusting the URL construction:
-const url = new URL(path, assetUrlBase).href; +const url = new URL(path, assetUrlBase.endsWith('/') ? assetUrlBase : assetUrlBase + '/').href;src/pages/emojis.tsx (2)
7-7: Consider renaming the../s3module for clarityImporting
diskandassetUrlBasefrom../s3might be misleading since the storage abstraction now supports multiple backends beyond S3. Consider renaming the../s3module to something more generic like../storageto improve code clarity and maintainability.
176-176: EnsureassetUrlBasecorrectly constructs the emoji URLWhen constructing the emoji URL, make sure that
assetUrlBaseends with a forward slash (/), andpathdoes not start with one, to prevent any issues with URL concatenation. This will ensure that the resulting URL is correctly formed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (11)
.env.sample(1 hunks)compose-fs.yaml(1 hunks)compose.yaml(2 hunks)docs/src/content/docs/install/env.mdx(1 hunks)package.json(2 hunks)src/api/v1/accounts.ts(3 hunks)src/api/v1/media.ts(2 hunks)src/index.tsx(1 hunks)src/media.ts(2 hunks)src/pages/emojis.tsx(2 hunks)src/s3.ts(1 hunks)
🔇 Additional comments (16)
compose-fs.yaml (1)
11-13: Verify filesystem storage configuration across environments.
The filesystem storage configuration looks correct, but please ensure:
- The
/var/lib/hollopath is appropriate for your deployment environment - The asset URL base is correctly configured for production
- The filesystem permissions are properly set for the container user
✅ Verification successful
Based on the analysis of the code and configuration files, I can now provide a final response:
Filesystem storage configuration is properly implemented and secure
The configuration is correctly set up with:
- Docker volume mapping for persistent storage at
assets_data:/var/lib/hollo - Proper filesystem driver integration via
FSDriverfrom flydrive - Correct asset URL configuration for local serving
The implementation is secure because:
- The storage path is properly containerized using Docker volumes
- Assets are served through the application's controlled endpoint
- File permissions are managed through Docker's volume ownership
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the filesystem storage configuration is used in other files
echo "Checking for filesystem storage configuration references..."
rg "DRIVE_DISK|FS_ASSET_PATH|ASSET_URL_BASE" --type yaml --type typescript
# Check for potential file permission issues in the codebase
echo "Checking for file permission handling..."
rg "chmod|chown|fs\.chmod|fs\.chown"
Length of output: 370
Script:
#!/bin/bash
# Check for filesystem storage configuration references with correct file extensions
echo "Checking for filesystem storage configuration references..."
rg "DRIVE_DISK|FS_ASSET_PATH|ASSET_URL_BASE" --type yml --type ts --type js
# Check for file system related code
echo "Checking for filesystem related code..."
rg "fs\.(write|read|mkdir|access|stat|chmod|chown)" --type ts --type js
# Look for environment configuration files
echo "Looking for environment configuration files..."
fd -e yml -e yaml -e env
Length of output: 556
Script:
#!/bin/bash
# Check both compose files for configuration differences
echo "Comparing compose files..."
cat compose.yaml
echo "---"
cat compose-fs.yaml
# Search for filesystem operations in the codebase without file type restrictions
echo "Checking for filesystem operations..."
rg "import.*fs|require.*fs"
rg "writeFile|readFile|mkdir|access|stat"
Length of output: 67019
compose.yaml (3)
32-32: LGTM!
The indentation changes in the postgres service are consistent with the file's style.
38-38: LGTM!
The indentation changes in the minio service are consistent with the file's style.
Also applies to: 43-43
3-3: Verify the image reference strategy.
The image reference has been changed from a fully qualified registry path to a local reference. This might cause issues in production environments where the image needs to be pulled from a registry.
✅ Verification successful
Image reference strategy is intentional for different deployment scenarios
The image reference strategy is correct. The repository contains two compose files for different deployment scenarios:
compose.yaml: Uses local image tag for S3/MinIO storage backendcompose-fs.yaml: Uses fully qualified registry path for filesystem storage backend
Each file serves a distinct purpose and the image references are appropriately configured for their respective use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other compose files that might override this image
fd -e yaml -e yml compose
Length of output: 56
Script:
#!/bin/bash
# Check the content of both compose files to understand the image configuration strategy
echo "=== compose.yaml ==="
cat compose.yaml
echo -e "\n=== compose-fs.yaml ==="
cat compose-fs.yaml
Length of output: 2436
src/index.tsx (1)
16-24: 🛠️ Refactor suggestion
Add security measures and performance optimizations to static file serving
The static file serving implementation could benefit from several improvements:
- Validate
assetPathto prevent directory traversal attacks - Add cache control headers for better performance
- Add explicit error handling for missing files
- Consider making the disk type a typed constant
Consider this enhanced implementation:
+const STORAGE_TYPE = {
+ FILESYSTEM: 'fs',
+ S3: 's3'
+} as const;
+
-if (DRIVE_DISK === "fs") {
+if (DRIVE_DISK === STORAGE_TYPE.FILESYSTEM) {
+ // Validate asset path
+ if (!assetPath || !assetPath.startsWith('/')) {
+ throw new Error('Invalid asset path configuration');
+ }
+
app.use(
"/assets/*",
serveStatic({
root: assetPath,
rewriteRequestPath: (path) => path.substring("/assets".length),
+ headers: {
+ 'Cache-Control': 'public, max-age=86400',
+ },
}),
);
}Let's verify the asset path configuration:
package.json (1)
16-16: Verify AWS SDK version consistency.
The new AWS SDK package version matches other AWS dependencies, which is good. However, let's verify there are no other AWS SDK dependencies that might need updating.
✅ Verification successful
All AWS SDK dependencies are consistent at version ^3.577.0
All AWS SDK dependencies in package.json are using the same version (^3.577.0):
- @aws-sdk/client-s3
- @aws-sdk/credential-providers
- @aws-sdk/s3-request-presigner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent AWS SDK versions across all dependencies
# Test: Search for all AWS SDK dependencies and their versions
rg '"@aws-sdk/[^"]+": "\^[0-9]+\.[0-9]+\.[0-9]+"' package.json
# Additionally, check for any AWS SDK dependencies in yarn.lock to catch transitive dependencies
rg '@aws-sdk/' yarn.lock
Length of output: 275
src/media.ts (1)
37-37: Validate assetUrlBase before URL construction.
The URL construction assumes assetUrlBase is a valid URL. Consider validating it to prevent runtime errors.
Let's verify the validation of assetUrlBase in the codebase:
docs/src/content/docs/install/env.mdx (1)
81-128: Verify documentation aligns with implementation
The documentation changes align well with the PR objectives of supporting alternative asset stores. However, let's verify the implementation details:
✅ Verification successful
Documentation accurately reflects the implementation
The implementation in src/s3.ts perfectly aligns with the documented configuration options:
- FlyDrive is properly implemented using both FSDriver and S3Driver
- All documented environment variables are correctly handled:
DRIVE_DISKsupports both 'fs' and 's3' valuesASSET_URL_BASEis required and validatedFS_ASSET_PATHis required for filesystem storage- All S3-related variables (
S3_REGION,S3_BUCKET,S3_ENDPOINT_URL,S3_FORCE_PATH_STYLE,AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY) are implemented as documented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation matches the documented options
# Check if FlyDrive is properly implemented
echo "Checking for FlyDrive implementation..."
rg -l "flydrive"
# Verify supported drivers
echo "Verifying supported drivers..."
rg -l "disk\s*:\s*['\"](fs|s3)['\"]"
# Check for proper environment variable usage
echo "Checking environment variable usage..."
rg -l "(DRIVE_DISK|ASSET_URL_BASE|FS_ASSET_PATH)"
Length of output: 510
Script:
#!/bin/bash
# Let's examine the actual implementation details
# Check FlyDrive configuration and usage
echo "=== FlyDrive Configuration ==="
rg -A 5 "flydrive" src/
# Check environment variable handling
echo -e "\n=== Environment Variable Handling ==="
rg "process\.env\.(DRIVE_DISK|ASSET_URL_BASE|FS_ASSET_PATH)" src/
# Examine S3 implementation
echo -e "\n=== S3 Implementation ==="
cat src/s3.ts
# Check for disk configuration
echo -e "\n=== Disk Configuration ==="
rg "disk.*config" src/
Length of output: 3456
src/pages/emojis.tsx (2)
3-3: Approved: Importing 'mime' module
The import of the mime module is appropriate for handling MIME types and file extensions.
180-180: Approved: Inserting the emoji URL into the database
Storing the constructed URL into the customEmojis table is correctly implemented.
src/api/v1/accounts.ts (6)
21-21: Importing the 'mime' module for file type handling
The introduction of the mime module is appropriate for determining file extensions based on MIME types.
41-41: Updating imports to use 'disk' and 'assetUrlBase'
The import statement has been updated to use disk and assetUrlBase from "../../s3", reflecting the shift from S3 to local disk storage.
120-127: Ensure appropriate visibility settings for uploaded avatar
The uploaded avatar file is set with visibility: "public". Ensure that making the avatar publicly accessible aligns with the application's security and privacy requirements.
Please verify if setting visibility: "public" for avatars is appropriate. If not, consider adjusting the visibility settings or implementing access controls to protect user data.
131-138: Ensure appropriate visibility settings for uploaded header
Similarly, the header image is set with visibility: "public". Confirm that this setting complies with the application's privacy policies.
Please verify if setting visibility: "public" for headers is appropriate. Adjust the settings if necessary to protect user privacy.
127-127: Cache invalidation using timestamp in avatar URL
Appending Date.now() as a query parameter in the avatar URL effectively invalidates caches, ensuring clients load the most recent image after updates.
138-138: Cache invalidation using timestamp in header URL
Similarly, adding Date.now() to the header URL query parameter ensures that the most recent header image is displayed, bypassing any cached versions.
| DRIVE_DISK= | ||
| ASSET_URL_BASE= | ||
| FS_ASSET_PATH= |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and example values for new storage configuration.
The new variables need documentation to guide users on proper configuration:
DRIVE_DISK: Specify valid options (e.g., 's3', 'local')ASSET_URL_BASE: Indicate format and HTTPS recommendationFS_ASSET_PATH: Provide path format and permission requirements
Apply this diff to add documentation:
-DRIVE_DISK=
-ASSET_URL_BASE=
-FS_ASSET_PATH=
+# Storage configuration
+DRIVE_DISK=local # Storage driver to use: 'local' for filesystem or 's3' for S3
+ASSET_URL_BASE=https://assets.example.com # Base URL for accessing assets (HTTPS recommended)
+FS_ASSET_PATH=/path/to/assets # Absolute path to store assets when using local filesystem📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DRIVE_DISK= | |
| ASSET_URL_BASE= | |
| FS_ASSET_PATH= | |
| # Storage configuration | |
| DRIVE_DISK=local # Storage driver to use: 'local' for filesystem or 's3' for S3 | |
| ASSET_URL_BASE=https://assets.example.com # Base URL for accessing assets (HTTPS recommended) | |
| FS_ASSET_PATH=/path/to/assets # Absolute path to store assets when using local filesystem |
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Storage configuration requires additional security measures
The code review reveals several security considerations that need to be addressed:
FS_ASSET_PATHis used withFSDriverfrom theflydrivepackage withvisibility: "public"setting, but there are no explicit permission checks or directory security validations- The
ASSET_URL_BASEis used directly without URL sanitization, though it's only used as a base URL prefix - When
DRIVE_DISK=fs, assets are served directly throughserveStaticmiddleware without additional security controls
Recommendations:
- Add validation for
ASSET_URL_BASEto ensure it's a valid URL and ends with a trailing slash - Implement directory permission checks when initializing
FSDriverto ensureFS_ASSET_PATHhas correct permissions - Consider adding URL path sanitization in the
serveStaticmiddleware - Document the required filesystem permissions for
FS_ASSET_PATHin the deployment guide
🔗 Analysis chain
Verify security implications of new storage configuration.
The new storage configuration introduces potential security considerations:
- Ensure
FS_ASSET_PATHis properly secured with appropriate permissions - Verify that
ASSET_URL_BASEis properly sanitized to prevent XSS
Let's check for any security-related code:
Let me gather more information about how these environment variables are used in the codebase:
Let me check one more aspect regarding file system security by looking at the FSDriver implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL sanitization and path security measures
echo "Checking URL sanitization:"
rg -l "ASSET_URL_BASE" | xargs rg -A 5 "sanitize|validate|clean"
echo "Checking filesystem security:"
rg -l "FS_ASSET_PATH" | xargs rg -A 5 "chmod|permission|access"
Length of output: 1217
Script:
#!/bin/bash
# Check how these env variables are used in the code
echo "Checking ASSET_URL_BASE usage:"
rg -A 5 "ASSET_URL_BASE"
echo -e "\nChecking FS_ASSET_PATH usage:"
rg -A 5 "FS_ASSET_PATH"
echo -e "\nChecking DRIVE_DISK usage:"
rg -A 5 "DRIVE_DISK"
# Look for any URL or path validation/sanitization utilities
echo -e "\nChecking for URL/path validation utilities:"
rg -l "url|path" | xargs rg "validate|sanitize|normalize"
Length of output: 5126
Script:
#!/bin/bash
# Look for FSDriver implementation and security measures
echo "Checking FSDriver implementation:"
rg -A 10 "class FSDriver"
# Check for any security-related imports or utilities
echo -e "\nChecking for security-related imports:"
rg -l "FSDriver" | xargs rg "import.*(@node|path|fs|security)"
Length of output: 308
| ports: | ||
| - "3000:3000" | ||
| environment: | ||
| DATABASE_URL: "postgres://user:password@postgres:5432/database" | ||
| SECRET_KEY: "${SECRET_KEY}" | ||
| LOG_LEVEL: "${LOG_LEVEL}" | ||
| BEHIND_PROXY: "${BEHIND_PROXY}" | ||
| DRIVE_DISK: fs | ||
| ASSET_URL_BASE: http://localhost:3000/assets/ | ||
| FS_ASSET_PATH: /var/lib/hollo | ||
| depends_on: | ||
| - postgres | ||
| volumes: | ||
| - assets_data:/var/lib/hollo | ||
| restart: unless-stopped |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add health check and improve service configuration.
Consider the following improvements:
- Add health check to ensure container readiness
- Restrict port binding to localhost
- Add resource limits
ports:
- - "3000:3000"
+ - "127.0.0.1:3000:3000"
+ healthcheck:
+ test: ["CMD", "curl", "-f", "http://localhost:3000/health"]
+ interval: 30s
+ timeout: 10s
+ retries: 3
+ deploy:
+ resources:
+ limits:
+ memory: 1G📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ports: | |
| - "3000:3000" | |
| environment: | |
| DATABASE_URL: "postgres://user:password@postgres:5432/database" | |
| SECRET_KEY: "${SECRET_KEY}" | |
| LOG_LEVEL: "${LOG_LEVEL}" | |
| BEHIND_PROXY: "${BEHIND_PROXY}" | |
| DRIVE_DISK: fs | |
| ASSET_URL_BASE: http://localhost:3000/assets/ | |
| FS_ASSET_PATH: /var/lib/hollo | |
| depends_on: | |
| - postgres | |
| volumes: | |
| - assets_data:/var/lib/hollo | |
| restart: unless-stopped | |
| ports: | |
| - "127.0.0.1:3000:3000" | |
| environment: | |
| DATABASE_URL: "postgres://user:password@postgres:5432/database" | |
| SECRET_KEY: "${SECRET_KEY}" | |
| LOG_LEVEL: "${LOG_LEVEL}" | |
| BEHIND_PROXY: "${BEHIND_PROXY}" | |
| DRIVE_DISK: fs | |
| ASSET_URL_BASE: http://localhost:3000/assets/ | |
| FS_ASSET_PATH: /var/lib/hollo | |
| healthcheck: | |
| test: ["CMD", "curl", "-f", "http://localhost:3000/health"] | |
| interval: 30s | |
| timeout: 10s | |
| retries: 3 | |
| deploy: | |
| resources: | |
| limits: | |
| memory: 1G | |
| depends_on: | |
| - postgres | |
| volumes: | |
| - assets_data:/var/lib/hollo | |
| restart: unless-stopped |
| postgres: | ||
| image: postgres:17 | ||
| environment: | ||
| POSTGRES_USER: user | ||
| POSTGRES_PASSWORD: password | ||
| POSTGRES_DB: database | ||
| volumes: | ||
| - postgres_data:/var/lib/postgresql/data | ||
| restart: unless-stopped |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Postgres service security and reliability.
Several improvements are recommended for the Postgres service:
- Externalize credentials
- Add health check
- Add resource limits
- Consider setting additional Postgres configurations
postgres:
image: postgres:17
environment:
- POSTGRES_USER: user
- POSTGRES_PASSWORD: password
- POSTGRES_DB: database
+ POSTGRES_USER: ${POSTGRES_USER}
+ POSTGRES_PASSWORD: ${POSTGRES_PASSWORD}
+ POSTGRES_DB: ${POSTGRES_DB}
+ POSTGRES_INITDB_ARGS: "--auth-host=scram-sha-256"
+ healthcheck:
+ test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER} -d ${POSTGRES_DB}"]
+ interval: 10s
+ timeout: 5s
+ retries: 5
+ deploy:
+ resources:
+ limits:
+ memory: 512M📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| postgres: | |
| image: postgres:17 | |
| environment: | |
| POSTGRES_USER: user | |
| POSTGRES_PASSWORD: password | |
| POSTGRES_DB: database | |
| volumes: | |
| - postgres_data:/var/lib/postgresql/data | |
| restart: unless-stopped | |
| postgres: | |
| image: postgres:17 | |
| environment: | |
| POSTGRES_USER: ${POSTGRES_USER} | |
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD} | |
| POSTGRES_DB: ${POSTGRES_DB} | |
| POSTGRES_INITDB_ARGS: "--auth-host=scram-sha-256" | |
| healthcheck: | |
| test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER} -d ${POSTGRES_DB}"] | |
| interval: 10s | |
| timeout: 5s | |
| retries: 5 | |
| deploy: | |
| resources: | |
| limits: | |
| memory: 512M | |
| volumes: | |
| - postgres_data:/var/lib/postgresql/data | |
| restart: unless-stopped |
|
Early read through looks okay, though maybe rename that s3 file to storage or something? |
|
Thanks for your contribution! I will review this today. |
Co-authored-by: Hong Minhee (洪 民憙) <[email protected]>
Co-authored-by: Hong Minhee (洪 民憙) <[email protected]>
Co-authored-by: Hong Minhee (洪 民憙) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/s3.ts (2)
1-5: Consider renaming this file to better reflect its purposeAs this file now handles multiple storage backends beyond just S3, consider renaming it to
assets.tsorstorage.tsto better reflect its current responsibility.
38-40: Remove redundant DRIVE_DISK exportThe
DRIVE_DISKconstant is exported twice with the same value. Consider removing one of the exports to avoid confusion:export const driveDisk = process.env["DRIVE_DISK"] ?? "s3"; if (driveDisk == null) throw new Error("DRIVE_DISK is required"); -export const DRIVE_DISK = driveDisk;Use either
driveDiskorDRIVE_DISKconsistently throughout the codebase.docs/src/content/docs/install/env.mdx (4)
81-88: Enhance DRIVE_DISK documentation with missing detailsThe documentation should be enhanced with:
- Default value specification
- Link formatting for better accessibility
-### `DRIVE_DISK` +### `DRIVE_DISK` <Badge text="Required" /> The disk driver used by Hollo to store blobs such as avatars, custom emojis, and other media. Valid values are `fs` (local filesystem) and `s3` (S3-compatible object storage). +Default: `s3` -See https://flydrive.dev/docs/drive_manager for details about the drivers. +See [FlyDrive documentation](https://flydrive.dev/docs/drive_manager) for details about the drivers.
98-98: Fix grammatical issueAdd a comma after "e.g." for proper punctuation.
-The path in the local filesystem where blob assets are stored e.g. `/var/lib/hollo`. +The path in the local filesystem where blob assets are stored, e.g., `/var/lib/hollo`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~98-~98: Possible missing comma found.
Context: ... local filesystem where blob assets are stored e.g./var/lib/hollo. The directory mu...(AI_HYDRA_LEO_MISSING_COMMA)
127-128: Fix inconsistent line ending punctuationAdd a period at the end of the sentence for consistency with other sections.
-The secret key for S3-compatible object storage +The secret key for S3-compatible object storage.
81-81: Add overview section for storage configurationConsider adding a high-level overview section before the storage-related configurations to help users choose between storage options. This would improve documentation clarity and align with the PR's objective of supporting alternative asset stores.
+### Storage Configuration + +Hollo supports two storage options for assets: + +1. **Local Filesystem Storage**: Suitable for: + - Development environments + - Simple deployments + - Kubernetes setups with persistent volumes + +2. **S3-compatible Object Storage**: Suitable for: + - Production environments + - Distributed deployments + - Cloud-native architectures + +Choose the appropriate storage option based on your deployment needs and scale. + ### `DRIVE_DISK`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
compose.yaml(1 hunks)docs/src/content/docs/install/env.mdx(1 hunks)src/pages/emojis.tsx(2 hunks)src/s3.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- compose.yaml
🧰 Additional context used
📓 Learnings (1)
src/s3.ts (1)
Learnt from: dahlia
PR: dahlia/hollo#59
File: src/s3.ts:41-41
Timestamp: 2024-11-05T07:24:25.045Z
Learning: In `src/s3.ts`, when initializing the `FSDriver`, use `import.meta.dirname` to correctly handle filesystem paths in Bun.
🪛 LanguageTool
docs/src/content/docs/install/env.mdx
[uncategorized] ~98-~98: Possible missing comma found.
Context: ... local filesystem where blob assets are stored e.g. /var/lib/hollo. The directory mu...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (4)
src/s3.ts (1)
58-66: Review S3 bucket configuration security
While the code correctly validates credentials, consider adding additional security measures:
- Enable server-side encryption for the S3 bucket
- Configure CORS policies appropriately for the
assetUrlBase - Consider implementing bucket policy validation
Let's check if there are any security-related configurations:
src/pages/emojis.tsx (3)
3-3: LGTM: Import changes align with storage transition
The changes to imports correctly reflect the transition from S3 to local disk storage, with appropriate addition of mime type support.
Also applies to: 7-7
171-175:
Add error handling for storage operation
The disk.put operation could fail due to various reasons (e.g., disk full, permissions). Error handling should be added to provide appropriate feedback.
Apply this diff to add error handling:
+try {
await disk.put(path, content, {
contentType: image.type,
contentLength: content.byteLength,
visibility: "public",
});
+} catch (error) {
+ console.error('Failed to store emoji:', error);
+ return c.text("Failed to store emoji image", 500);
+}Likely invalid or redundant comment.
169-170:
Add validation for file extension
The mime.getExtension() might return undefined for unsupported file types, which could lead to invalid file paths.
Apply this diff to add validation:
const content = new Uint8Array(await image.arrayBuffer());
+const extension = mime.getExtension(image.type);
+if (!extension) {
+ return c.text("Unsupported image type", 400);
+}
-const path = `emojis/${shortcode}.${mime.getExtension(image.type)}`;
+const path = `emojis/${shortcode}.${extension}`;Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/media.ts (2)
6-7: Consider more explicit import naming.The import
diskcould be renamed to something more descriptive likestorageorfileStorageto better reflect its role as an abstract storage interface rather than a specific implementation.-import { disk } from "./s3"; +import { storage } from "./s3";
30-39: LGTM! Error handling has been properly implemented.The implementation correctly handles storage errors and uses appropriate content type and visibility settings.
Consider these minor improvements:
- Add more context to the error message
- Define content type as a constant
+const THUMBNAIL_CONTENT_TYPE = "image/webp"; try { await disk.put(`media/${id}/thumbnail.webp`, content, { - contentType: "image/webp", + contentType: THUMBNAIL_CONTENT_TYPE, contentLength: content.byteLength, visibility: "public", }); } catch (error) { - throw new Error(`Failed to store thumbnail: ${error.message}`); + throw new Error(`Failed to store thumbnail for media ${id}: ${error.message}`); }src/s3.ts (2)
1-19: Consider renaming the file to better reflect its purpose.The file name
s3.tsno longer accurately represents its functionality since it now handles multiple storage backends. As previously discussed, renaming it toassets.tsorstorage.tswould better reflect its current purpose.
47-50: Consider usingimport.meta.dirnamefor filesystem paths.While using
new URL(assetPath, import.meta.url)works and is documented in FlyDrive, usingimport.meta.dirnamemight be more appropriate for handling filesystem paths in Bun, as previously suggested.src/api/v1/media.ts (1)
34-49: LGTM! Well-structured storage abstraction implementation.The implementation:
- Properly handles file content conversion
- Validates MIME types before use
- Includes comprehensive error handling
- Maintains security through proper path construction
- Uses appropriate visibility settings
One minor suggestion to improve the error message:
- return c.json({ error: 'Failed to save media file' }, 500); + return c.json({ error: 'Failed to save media file. Please try again later or contact support if the issue persists.' }, 500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docs/src/content/docs/install/env.mdx(1 hunks)src/api/v1/accounts.ts(3 hunks)src/api/v1/media.ts(2 hunks)src/media.ts(2 hunks)src/s3.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/s3.ts (1)
Learnt from: dahlia
PR: dahlia/hollo#59
File: src/s3.ts:41-41
Timestamp: 2024-11-05T07:24:25.045Z
Learning: In `src/s3.ts`, when initializing the `FSDriver`, use `import.meta.dirname` to correctly handle filesystem paths in Bun.
🔇 Additional comments (7)
src/media.ts (2)
41-41: LGTM! URL construction properly uses the new base URL.
The URL construction correctly uses the new assetUrlBase, maintaining compatibility while supporting the new storage abstraction.
6-7: Verify storage abstraction implementation.
Let's ensure the storage abstraction is consistently implemented across the codebase.
Also applies to: 30-41
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information to verify the storage abstraction implementation:
Storage abstraction implementation is consistently applied
The verification shows:
- No direct AWS S3 SDK usage found in the codebase
- Consistent usage of
disk.put()operations across all file upload handlers with proper content type and visibility settings - Uniform usage of
assetUrlBasefor URL generation across media, accounts, and emoji endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent storage implementation across the codebase
# Test 1: Verify no direct S3 SDK usage remains
echo "Checking for direct AWS S3 SDK usage..."
rg -l "AWS.S3|@aws-sdk\/client-s3"
# Test 2: Verify consistent use of the storage abstraction
echo "Checking for consistent disk/storage usage pattern..."
rg -A 3 "disk\.(put|get|delete)"
# Test 3: Check for consistent URL base usage
echo "Checking for consistent URL base usage..."
rg "assetUrlBase|S3_URL_BASE"
Length of output: 2735
src/s3.ts (1)
31-40: LGTM! Well-implemented backward compatibility.
The implementation properly handles backward compatibility by defaulting to 's3' while warning users about future requirements. The warning message is clear and informative.
src/api/v1/media.ts (2)
10-10: LGTM! Clean transition from S3-specific to generic storage imports.
The change from S3-specific imports to generic storage abstractions (disk, assetUrlBase) aligns well with the goal of supporting multiple storage backends.
10-10: Verify storage configuration documentation.
Since this introduces new storage configuration options, let's ensure the documentation is complete.
✅ Verification successful
Let me check the actual content of the documentation to verify if it properly covers the storage configuration.
Storage configuration is well documented
The documentation thoroughly covers all storage-related configurations:
- Environment variables (
DRIVE_DISK,ASSET_URL_BASE,FS_ASSET_PATH) are clearly documented with requirements and examples - Both filesystem and S3 storage options are explained in detail
- S3-compatible storage settings are comprehensively documented with all required parameters
- Installation guides (Docker, Railway) include proper storage setup instructions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if storage configuration is documented
# Test: Look for documentation of new environment variables
echo "Checking for environment variable documentation..."
rg -l "DRIVE_DISK|ASSET_URL_BASE|FS_ASSET_PATH" "README.md" "docs/" || echo "No documentation found for new environment variables"
# Test: Look for storage configuration examples
echo "Checking for configuration examples..."
rg -l "storage|disk.*configuration" "README.md" "docs/" || echo "No configuration examples found"
Length of output: 476
Script:
#!/bin/bash
# Check the content of environment variables documentation
echo "=== Environment Variables Documentation ==="
rg -A 5 -B 5 "DRIVE_DISK|ASSET_URL_BASE|FS_ASSET_PATH" "docs/src/content/docs/install/env.mdx"
# Check storage configuration examples
echo -e "\n=== Storage Configuration Examples ==="
rg -A 5 -B 5 "storage|disk.*configuration" "docs/src/content/docs/install/env.mdx" "docs/src/content/docs/install/railway.mdx" "docs/src/content/docs/install/docker.mdx"
Length of output: 7636
src/api/v1/accounts.ts (2)
21-21: LGTM: Import changes align with storage transition
The changes to imports appropriately reflect the transition from S3 to local disk storage, with the addition of the mime package for proper file type handling.
Also applies to: 41-41
120-131: 🛠️ Refactor suggestion
Add error handling for avatar upload operation
While the MIME type validation is properly implemented, the disk.put operation lacks error handling. If the disk operation fails, it could result in an unhandled exception.
Add error handling:
const content = await form.avatar.arrayBuffer();
const path = `avatars/${account.id}.${mime.getExtension(form.avatar.type)}`;
+ try {
await disk.put(path, new Uint8Array(content), {
contentType: form.avatar.type,
contentLength: content.byteLength,
visibility: "public",
});
+ } catch (error) {
+ return c.json({ error: 'Failed to upload avatar image.' }, 500);
+ }
avatarUrl = new URL(`${path}?${Date.now()}`, assetUrlBase).href;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/media.ts (1)
30-42: Enhance error message with operation context.While the error handling is good, we can make the error message more specific to help with debugging.
- throw new Error(`Failed to store thumbnail: ${error.message}`, error); + throw new Error(`Failed to store thumbnail for media ${id}: ${error.message}`, error);src/storage.ts (2)
8-9: Refactor to eliminate the need for linter suppression comments.Multiple instances of
// biome-ignore lint/complexity/useLiteralKeysare used to suppress linter warnings when accessing environment variables with string literals. To improve code quality and maintainability, consider addressing the root cause by:
- Using dot notation for environment variables with valid identifier names (e.g.,
process.env.FS_ASSET_PATH).- Creating a configuration module or utility function to handle environment variables consistently and safely.
- Adjusting the TypeScript configuration to accommodate string literal keys if necessary.
This approach reduces reliance on linter suppression comments and enhances code clarity.
Also applies to: 11-12, 14-15, 17-18, 22-23, 25-26, 28-29, 31-32, 38-39, 65-65
39-40: Redundant null check ondriveDisk.Since
driveDiskis assigned a default value of"s3"(process.env["DRIVE_DISK"] ?? "s3"), it will never benullorundefined. Therefore, the checkif (driveDisk == null)is redundant and can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/api/v1/accounts.ts(3 hunks)src/api/v1/media.ts(2 hunks)src/index.tsx(1 hunks)src/media.ts(2 hunks)src/pages/emojis.tsx(2 hunks)src/storage.ts(1 hunks)
🔇 Additional comments (13)
src/index.tsx (2)
26-26: Good placement of static asset middleware.
The static asset serving middleware is correctly placed before the federation middleware. This ensures that asset requests are handled appropriately and don't get processed as federation requests.
5-5: Consider platform-agnostic imports for better portability.
The import serveStatic from hono/bun makes the code Bun-specific. Consider using a more platform-agnostic approach if there's a need to support other runtime environments in the future.
Let's check if there are other runtime-specific imports in the codebase:
Also applies to: 12-12
src/media.ts (3)
6-7: LGTM! Clean transition to generic storage imports.
The removal of S3-specific imports in favor of generic storage imports aligns well with the goal of supporting multiple storage backends.
44-44: LGTM! Safe URL construction.
The URL construction using the URL constructor is secure and correctly uses the new storage-agnostic base URL.
6-7: Verify consistent storage usage across media operations.
Let's ensure all media-related operations have been updated to use the new storage approach consistently.
Also applies to: 30-42
✅ Verification successful
Let me gather more information about the storage implementation and its usage.
Storage implementation is properly configured and consistently used
The verification shows that:
- The storage implementation properly handles both S3 and filesystem backends through a unified
diskinterface - All media-related operations (
src/media.ts,src/api/v1/media.ts,src/api/v1/accounts.ts,src/pages/emojis.tsx) consistently use:disk.put()for storing assetsassetUrlBasefor URL generation
- Proper error handling and configuration validation is in place
- The S3-specific code is only present in
storage.tsas part of the backend configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining S3-specific code in media operations
# and verify consistent usage of the new storage approach
# Check for any remaining S3 references in media-related files
echo "Checking for remaining S3 references in media operations..."
rg -l 'S3|s3' src/ | grep -i 'media\|upload\|storage'
# Check for consistent usage of the new disk storage
echo "Verifying consistent usage of new storage approach..."
rg -l 'disk\.put|disk\.get' src/ | grep -i 'media\|upload\|storage'
# Check for proper error handling pattern in storage operations
echo "Checking error handling in storage operations..."
ast-grep --pattern 'try {\n $$$\n await disk.$_($$$)\n $$$\n} catch'
Length of output: 600
Script:
#!/bin/bash
# Check the storage.ts implementation and references
echo "Checking storage.ts implementation..."
cat src/storage.ts
# Look for any storage-related imports across the codebase
echo -e "\nChecking storage imports..."
rg -l "from ['\"].*storage" src/
# Check the actual usage patterns
echo -e "\nChecking storage usage patterns..."
rg "disk\.|assetUrlBase" -A 2 src/
Length of output: 6194
src/api/v1/media.ts (2)
3-3: LGTM! Import changes align with storage abstraction.
The replacement of S3-specific imports with generic storage imports (assetUrlBase, disk) successfully implements the storage abstraction layer.
Also applies to: 11-11
41-45: Consider making storage options configurable.
The storage options (particularly visibility: "public") are hardcoded. Consider making these configurable through environment variables to support different deployment scenarios.
src/pages/emojis.tsx (2)
3-3: LGTM: Import changes align with storage transition
The changes correctly replace S3-specific imports with the new storage abstraction imports and add necessary MIME type handling support.
Also applies to: 8-8
176-176: Ensure URL construction handles missing trailing slash
The URL construction with new URL(path, assetUrlBase) assumes assetUrlBase ends with a trailing slash. If it doesn't, the last segment of the base URL will be replaced with the path.
Let's verify the URL construction behavior:
Consider normalizing the base URL:
- const url = new URL(path, assetUrlBase).href;
+ const base = assetUrlBase.endsWith('/') ? assetUrlBase : `${assetUrlBase}/`;
+ const url = new URL(path, base).href;src/api/v1/accounts.ts (3)
21-21: LGTM: Import changes align with storage abstraction.
The changes correctly replace S3-specific imports with storage abstraction imports, supporting the PR's objective of enabling alternative storage solutions.
Also applies to: 57-57
124-131: LGTM: Proper implementation of storage abstraction for avatar uploads.
The implementation correctly uses the new storage abstraction layer with appropriate metadata (content type, length, visibility) and proper URL construction.
137-145: LGTM: Proper error handling for header image uploads.
The implementation includes appropriate error handling with a clear error message and status code.
src/storage.ts (1)
49-50: Verify that assetPath resolves correctly in FSDriver initialization.
When initializing the FSDriver, assetPath is combined with import.meta.url using new URL(assetPath, import.meta.url). If assetPath is an absolute file system path, using new URL() may not produce the intended result. Consider verifying that assetPath correctly resolves to the desired directory. You might use path.resolve() for absolute paths or ensure that assetPath is specified relative to import.meta.url.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/api/v1/media.ts (1)
42-46: Consider private storage with signed URLs for better security.The current implementation assumes public visibility for all uploaded media. Consider implementing private storage with signed URLs for better security control, especially if sensitive media needs to be stored.
This could be implemented by:
- Setting
visibility: "private"in storage options- Generating signed URLs when serving media
- Implementing URL signing middleware
Would you like assistance in implementing this security enhancement?
src/storage.ts (1)
11-33: Consider consolidating environment variable declarations.The repeated
biome-ignorecomments could be simplified by grouping environment variable declarations.Consider refactoring like this:
-// biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) -export const assetPath = process.env["FS_ASSET_PATH"]; -// biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) -const region = process.env["S3_REGION"]; -// ... more individual declarations +// biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) +const { + FS_ASSET_PATH: assetPath, + S3_REGION: region, + S3_BUCKET: bucket, + ASSET_URL_BASE: assetUrlBase, + S3_ENDPOINT_URL: endpointUrl, + AWS_ACCESS_KEY_ID: accessKeyId, + AWS_SECRET_ACCESS_KEY: secretAccessKey, +} = process.env;docs/src/content/docs/install/env.mdx (1)
164-164: Add missing period for consistency.Add a period at the end of the line to maintain consistency with other descriptions in the documentation.
-The secret key for S3-compatible object storage +The secret key for S3-compatible object storage.src/pages/emojis.tsx (1)
177-183: Consider implementing a content addressing scheme for emoji storageWhile the current implementation works well, consider using content-based addressing (e.g., using a hash of the file content) for the stored files instead of using the shortcode directly in the path. This would:
- Prevent potential naming conflicts
- Enable deduplication of identical emoji images
- Make it easier to implement CDN caching strategies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docs/src/content/docs/install/env.mdx(1 hunks)src/api/v1/accounts.ts(3 hunks)src/api/v1/media.ts(2 hunks)src/pages/emojis.tsx(2 hunks)src/storage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/v1/accounts.ts
🔇 Additional comments (11)
src/api/v1/media.ts (3)
3-3: LGTM: Import changes align with storage abstraction goals.
The removal of S3-specific imports and addition of generic storage imports (assetUrlBase, disk) successfully implements the transition to a more flexible storage solution.
Also applies to: 11-11
42-46: LGTM: Storage parameters are properly configured.
The storage operation includes all necessary metadata:
- Content type preservation
- Content length specification
- Public visibility setting
50-50: Verify URL construction with assetUrlBase.
The URL construction needs validation to ensure it handles various assetUrlBase formats correctly.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
URL construction with assetUrlBase is properly implemented and validated
The implementation is safe because:
assetUrlBaseis required and validated at startup:if (assetUrlBase == null) throw new Error("ASSET_URL_BASE is required")- URL construction is consistent across the codebase using the same pattern
- The base URL is loaded from environment variables, ensuring configuration control
- Cache busting is properly implemented where needed (e.g., with
?${Date.now()})
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify assetUrlBase configuration and usage
# Test 1: Check if assetUrlBase is properly configured
rg -l "ASSET_URL_BASE|assetUrlBase" --type yaml --type typescript
# Test 2: Check URL construction patterns
ast-grep --pattern 'new URL($_, assetUrlBase)'
Length of output: 582
Script:
#!/bin/bash
# Check assetUrlBase configuration and initialization
rg -A 5 "assetUrlBase.*="
# Check environment variable configuration
rg -l "ASSET_URL_BASE" --type env --type ts --type js --type json
# Check if there are any URL validation or sanitization
rg -B 2 -A 2 "new URL\("
Length of output: 44452
Script:
#!/bin/bash
# Check assetUrlBase validation and error handling
rg -B 3 -A 3 "assetUrlBase"
# Check if there are any URL validation utilities
rg -l "validateUrl|isValidUrl|urlValidation"
# Check storage configuration and initialization
rg -A 5 "storage.*=|storage.*initialize"
Length of output: 3969
src/storage.ts (2)
1-9: LGTM! Clean imports and type definition.
The imports are well-organized and the DriveDisk type is clearly defined.
91-92: LGTM! Clean disk export.
The disk instance is properly exported using the configured driver.
docs/src/content/docs/install/env.mdx (4)
81-100: LGTM! Well-documented configuration with migration guidance.
The documentation for DRIVE_DISK is clear, comprehensive, and includes valuable migration steps. The caution note about migration is particularly helpful for production deployments.
101-112: LGTM! Clear documentation with important requirements.
The documentation for ASSET_URL_BASE is well-structured with good examples and critical requirements clearly highlighted in the caution note.
113-125: LGTM! Comprehensive documentation with detailed requirements.
The documentation for FS_ASSET_PATH is thorough, with clear requirements and helpful Docker-specific guidance.
126-164: LGTM! Well-organized S3 settings with clear IAM requirements.
The S3 settings section is well-structured with appropriate badges and comprehensive IAM permission requirements.
src/pages/emojis.tsx (2)
3-3: LGTM: Import changes align with storage transition
The changes correctly replace S3-specific imports with the new storage abstraction layer imports. The addition of the mime package is appropriate for handling file extensions.
Also applies to: 8-8
164-187: LGTM: Robust implementation of emoji upload with proper validation and error handling
The implementation includes:
- Strong shortcode validation
- Proper MIME type handling
- Comprehensive error handling for storage operations
- Appropriate file visibility settings
All previously identified issues from past reviews have been addressed.
| if (!lstatSync(assetPath).isDirectory()) { | ||
| throw new Error(`Asset path must point to a directory: ${assetPath}`); | ||
| } | ||
| access( | ||
| assetPath, | ||
| constants.F_OK | constants.R_OK | constants.W_OK, | ||
| (err) => { | ||
| if (err) { | ||
| throw new Error(`${assetPath} must be readable and writable`); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Fix asynchronous access check in synchronous context.
The access() callback won't block execution, making the error handling ineffective. Also, permissions should be checked before attempting lstatSync.
Apply this fix:
- if (!lstatSync(assetPath).isDirectory()) {
- throw new Error(`Asset path must point to a directory: ${assetPath}`);
- }
- access(
- assetPath,
- constants.F_OK | constants.R_OK | constants.W_OK,
- (err) => {
- if (err) {
- throw new Error(`${assetPath} must be readable and writable`);
- }
- },
- );
+ try {
+ // Check permissions first
+ accessSync(assetPath, constants.F_OK | constants.R_OK | constants.W_OK);
+ // Then check if it's a directory
+ if (!lstatSync(assetPath).isDirectory()) {
+ throw new Error(`Asset path must point to a directory: ${assetPath}`);
+ }
+ } catch (err) {
+ throw new Error(`${assetPath} must be a readable and writable directory`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!lstatSync(assetPath).isDirectory()) { | |
| throw new Error(`Asset path must point to a directory: ${assetPath}`); | |
| } | |
| access( | |
| assetPath, | |
| constants.F_OK | constants.R_OK | constants.W_OK, | |
| (err) => { | |
| if (err) { | |
| throw new Error(`${assetPath} must be readable and writable`); | |
| } | |
| }, | |
| ); | |
| try { | |
| // Check permissions first | |
| accessSync(assetPath, constants.F_OK | constants.R_OK | constants.W_OK); | |
| // Then check if it's a directory | |
| if (!lstatSync(assetPath).isDirectory()) { | |
| throw new Error(`Asset path must point to a directory: ${assetPath}`); | |
| } | |
| } catch (err) { | |
| throw new Error(`${assetPath} must be a readable and writable directory`); | |
| } |
| if (bucket == null) throw new Error("S3_BUCKET is required"); | ||
| if (accessKeyId == null) throw new Error("AWS_ACCESS_KEY_ID is required"); | ||
| if (secretAccessKey == null) | ||
| throw new Error("AWS_SECRET_ACCESS_KEY is required"); | ||
|
|
||
| driver = new S3Driver({ | ||
| credentials: fromEnv(), | ||
| region: region == null || region === "" ? "auto" : region, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify S3 credential handling.
The code validates individual credentials but then uses fromEnv() which already handles AWS credential chain resolution.
Consider removing individual credential checks since fromEnv() will throw appropriate errors if credentials are missing:
- if (accessKeyId == null) throw new Error("AWS_ACCESS_KEY_ID is required");
- if (secretAccessKey == null)
- throw new Error("AWS_SECRET_ACCESS_KEY is required");
-
driver = new S3Driver({
credentials: fromEnv(),Committable suggestion skipped: line range outside the PR's diff.
src/storage.ts
Outdated
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| let driveDisk = process.env["DRIVE_DISK"]; | ||
| if (!driveDisk) { | ||
| getLogger(["hollo", "assets"]).warn( | ||
| "DRIVE_DISK is not configured; defaults to 's3'. " + | ||
| "The DRIVE_DISK environment variable will be mandatory in the future versions.", | ||
| ); | ||
| driveDisk = "s3"; | ||
| } else if (["fs", "s3"].includes(driveDisk)) { | ||
| throw new Error(`Unknown DRIVE_DISK value: '${driveDisk}'`); | ||
| } | ||
| export const DRIVE_DISK: DriveDisk = | ||
| driveDisk === "fs" || driveDisk === "s3" ? driveDisk : "s3"; |
There was a problem hiding this comment.
Fix incorrect validation logic for DRIVE_DISK.
There's a critical logic error in the validation. The current code throws an error when driveDisk is valid ("fs" or "s3") instead of when it's invalid.
Apply this fix:
-} else if (["fs", "s3"].includes(driveDisk)) {
+} else if (!["fs", "s3"].includes(driveDisk)) {
throw new Error(`Unknown DRIVE_DISK value: '${driveDisk}'`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |
| let driveDisk = process.env["DRIVE_DISK"]; | |
| if (!driveDisk) { | |
| getLogger(["hollo", "assets"]).warn( | |
| "DRIVE_DISK is not configured; defaults to 's3'. " + | |
| "The DRIVE_DISK environment variable will be mandatory in the future versions.", | |
| ); | |
| driveDisk = "s3"; | |
| } else if (["fs", "s3"].includes(driveDisk)) { | |
| throw new Error(`Unknown DRIVE_DISK value: '${driveDisk}'`); | |
| } | |
| export const DRIVE_DISK: DriveDisk = | |
| driveDisk === "fs" || driveDisk === "s3" ? driveDisk : "s3"; | |
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | |
| let driveDisk = process.env["DRIVE_DISK"]; | |
| if (!driveDisk) { | |
| getLogger(["hollo", "assets"]).warn( | |
| "DRIVE_DISK is not configured; defaults to 's3'. " + | |
| "The DRIVE_DISK environment variable will be mandatory in the future versions.", | |
| ); | |
| driveDisk = "s3"; | |
| } else if (!["fs", "s3"].includes(driveDisk)) { | |
| throw new Error(`Unknown DRIVE_DISK value: '${driveDisk}'`); | |
| } | |
| export const DRIVE_DISK: DriveDisk = | |
| driveDisk === "fs" || driveDisk === "s3" ? driveDisk : "s3"; |
src/storage.ts
Outdated
| import { constants, access, lstatSync } from "node:fs"; | ||
| import { fromEnv } from "@aws-sdk/credential-providers"; | ||
| import { getLogger } from "@logtape/logtape"; | ||
| import { Disk } from "flydrive"; | ||
| import { FSDriver } from "flydrive/drivers/fs"; | ||
| import { S3Driver } from "flydrive/drivers/s3"; | ||
| import type { DriverContract } from "flydrive/types"; | ||
|
|
||
| export type DriveDisk = "fs" | "s3"; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| export const assetPath = process.env["FS_ASSET_PATH"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| const region = process.env["S3_REGION"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| const bucket = process.env["S3_BUCKET"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| export const assetUrlBase = process.env["ASSET_URL_BASE"]; | ||
| if (assetUrlBase == null) throw new Error("ASSET_URL_BASE is required"); | ||
| export const S3_URL_BASE = assetUrlBase; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| const endpointUrl = process.env["S3_ENDPOINT_URL"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| const accessKeyId = process.env["AWS_ACCESS_KEY_ID"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| const secretAccessKey = process.env["AWS_SECRET_ACCESS_KEY"]; | ||
|
|
||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| let driveDisk = process.env["DRIVE_DISK"]; | ||
| if (!driveDisk) { | ||
| getLogger(["hollo", "assets"]).warn( | ||
| "DRIVE_DISK is not configured; defaults to 's3'. " + | ||
| "The DRIVE_DISK environment variable will be mandatory in the future versions.", | ||
| ); | ||
| driveDisk = "s3"; | ||
| } else if (["fs", "s3"].includes(driveDisk)) { | ||
| throw new Error(`Unknown DRIVE_DISK value: '${driveDisk}'`); | ||
| } | ||
| export const DRIVE_DISK: DriveDisk = | ||
| driveDisk === "fs" || driveDisk === "s3" ? driveDisk : "s3"; | ||
|
|
||
| let driver: DriverContract; | ||
| switch (DRIVE_DISK) { | ||
| case "fs": | ||
| if (!assetPath) { | ||
| throw new Error("FS_ASSET_PATH is required"); | ||
| } | ||
| if (!lstatSync(assetPath).isDirectory()) { | ||
| throw new Error(`Asset path must point to a directory: ${assetPath}`); | ||
| } | ||
| access( | ||
| assetPath, | ||
| constants.F_OK | constants.R_OK | constants.W_OK, | ||
| (err) => { | ||
| if (err) { | ||
| throw new Error(`${assetPath} must be readable and writable`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| driver = new FSDriver({ | ||
| location: new URL(assetPath, import.meta.url), | ||
| visibility: "public", | ||
| }); | ||
| break; | ||
| case "s3": | ||
| if (bucket == null) throw new Error("S3_BUCKET is required"); | ||
| if (accessKeyId == null) throw new Error("AWS_ACCESS_KEY_ID is required"); | ||
| if (secretAccessKey == null) | ||
| throw new Error("AWS_SECRET_ACCESS_KEY is required"); | ||
|
|
||
| driver = new S3Driver({ | ||
| credentials: fromEnv(), | ||
| region: region == null || region === "" ? "auto" : region, | ||
| endpoint: endpointUrl, | ||
| bucket: bucket, | ||
| // biome-ignore lint/complexity/useLiteralKeys: tsc complains about this (TS4111) | ||
| forcePathStyle: process.env["S3_FORCE_PATH_STYLE"] === "true", | ||
| visibility: "public", | ||
| }); | ||
| break; | ||
| default: | ||
| throw new Error(`Unknown DRIVE_DISK value: '${DRIVE_DISK}'`); | ||
| } | ||
|
|
||
| export const disk = new Disk(driver); |
There was a problem hiding this comment.
💡 Codebase verification
Error handling improvements needed for storage operations
The verification revealed several concerns with the current error handling implementation:
src/media.ts: While it has error handling, it just re-throws the error without cleanupsrc/api/v1/media.ts: Basic error handling returns 500 status but lacks cleanup of partially uploaded filessrc/api/v1/accounts.ts: Similar to media endpoint, returns 500 status without proper cleanup- No
finallyblocks or cleanup routines found in any of the files using disk operations - Missing error handling for partial uploads or corrupted files
Recommendations:
- Add cleanup routines in catch blocks to remove partially uploaded files
- Implement finally blocks to ensure resources are properly released
- Add validation of uploaded content before storage
- Consider implementing retry mechanisms for transient failures
🔗 Analysis chain
Verify error handling in dependent code.
Since this is a critical storage component, please ensure that all components using the disk instance properly handle potential storage errors and clean up temporary files.
Let's verify the error handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper error handling in files using the storage module
# Look for imports of the storage module
echo "Files importing storage module:"
rg -l "from ['\"].*storage['\"]"
# Check for try-catch blocks around disk operations
echo "\nError handling patterns:"
rg "try.*disk\." -A 5 -B 1
# Look for temporary file handling
echo "\nTemporary file handling:"
rg "(?i)temp.*file|cleanup|unlink" -A 3
Length of output: 399
Script:
#!/bin/bash
# Let's examine the actual usage of disk operations in the identified files
# Check disk operations in media.ts
echo "=== Disk operations in media.ts ==="
rg "disk\." src/media.ts -A 3 -B 1
# Check disk operations in API endpoints
echo -e "\n=== Disk operations in API endpoints ==="
rg "disk\." "src/api/v1/media.ts" "src/api/v1/accounts.ts" -A 3 -B 1
# Look for error handling patterns around storage operations
echo -e "\n=== Error handling patterns ==="
ast-grep --pattern 'try {
$$$
disk.$_($$$)
$$$
} catch'
Length of output: 1480
Script:
#!/bin/bash
# Let's examine the complete try-catch blocks to verify error handling
echo "=== Error handling in media.ts ==="
rg "try \{" src/media.ts -A 10 -B 1
echo -e "\n=== Error handling in API endpoints ==="
rg "try \{" "src/api/v1/media.ts" "src/api/v1/accounts.ts" -A 10 -B 1
# Check for any cleanup or error recovery code
echo -e "\n=== Cleanup patterns ==="
rg "finally|delete.*disk\.|remove.*disk\." src/api/v1/media.ts src/api/v1/accounts.ts src/media.ts -A 3 -B 1
Length of output: 2762
|
You might've missed it, but it's also possible to configure multiple stores and then select between them using the manager API: https://flydrive.dev/docs/drive_manager |
Use FlyDrive as abstraction for the storage layer used to store assets such as avatars, custom emojis, or media from posts.
Closes #10
Summary by CodeRabbit
Release Notes
New Features
DRIVE_DISK,ASSET_URL_BASE, andFS_ASSET_PATH.holloandpostgresservices.DRIVE_DISKconfiguration.Bug Fixes
Documentation
Chores
package.jsonfor enhanced functionality..gitignoreentry for theassets/directory.