refactor(webui): Migrate S3Manager to new Fastify architecture.#1098
Merged
Conversation
added 2 commits
July 11, 2025 17:07
Contributor
## Walkthrough
The changes refactor the S3Manager Fastify plugin to read configuration directly from a static JSON file instead of receiving options, rename its decorator property to "S3Manager" (capitalized), and update all usages accordingly. A new constant for pre-signed URL expiry is introduced, and related import statements are adjusted.
## Changes
| Files / File Groups | Change Summary |
|------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------|
| components/webui/server/src/app.ts | Removed S3Manager and settings imports; deleted conditional S3Manager plugin registration. |
| components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts | Refactored plugin to use static settings, renamed decorator to "S3Manager", updated logging, imports.|
| components/webui/server/src/fastify-v2/plugins/app/S3Manager/typings.ts | Added and exported PRE_SIGNED_URL_EXPIRY_TIME_SECONDS constant. |
| components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts | Updated decorator/property checks and usage from "s3Manager" to "S3Manager". |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant FastifyApp
participant S3Manager
Client->>FastifyApp: Request stream file
alt S3Manager configured
FastifyApp->>S3Manager: Generate pre-signed URL
S3Manager-->>FastifyApp: Return URL
FastifyApp-->>Client: Respond with pre-signed URL
else S3Manager not configured
FastifyApp-->>Client: Respond with error or fallback
endPossibly related PRs
Suggested reviewers
Learnt from: gibber9809 Learnt from: haiqi96 Learnt from: davemarco Learnt from: junhaoliao Learnt from: haiqi96 Learnt from: davemarco Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: junhaoliao Learnt from: haiqi96 Learnt from: haiqi96 Learnt from: junhaoliao |
Contributor
Author
I tested this with my s3 credentials and it still works.. |
Contributor
Author
|
@hoophalab , now that i test myself please review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/app.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
components/webui/server/src/app.ts (2)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
⏰ 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). (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
components/webui/server/src/app.ts (1)
1-57: Confirm S3Manager removal in v1 and registration in v2
- No remaining S3Manager references in
components/webui/server/src/app.ts(v1 plugin).- v2 plugin implementation lives in
components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts.- Route handler in
components/webui/server/src/routes/query.ts
correctly checksfastify.hasDecorator("S3Manager")and falls back when unset.Please verify that your v2 Fastify bootstrap (entrypoint) registers the S3Manager plugin and that querying routes produce valid pre-signed URLs in environments where S3 is configured.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/server/.env(1 hunks)components/webui/server/src/app.ts(0 hunks)
💤 Files with no reviewable changes (1)
- components/webui/server/src/app.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#743
File: components/log-viewer-webui/server/src/plugins/S3Manager.ts:76-76
Timestamp: 2025-04-07T09:30:20.684Z
Learning: In the S3Manager plugin, an `isEnabled` option should be added and checked instead of checking if the region is null. This would make the plugin's behavior more explicit.
Learnt from: junhaoliao
PR: y-scope/clp#743
File: components/log-viewer-webui/server/src/plugins/S3Manager.ts:76-76
Timestamp: 2025-04-07T09:30:20.684Z
Learning: In the S3Manager plugin, an `isEnabled` option should be added and checked instead of checking if the region is null. This would make the plugin's behavior more explicit.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/routes/query.js:9-10
Timestamp: 2025-01-17T22:33:23.547Z
Learning: When initializing S3Manager in log-viewer-webui, both StreamFilesS3Region and StreamFilesS3PathPrefix settings must be non-null for S3 functionality to work correctly.
components/webui/server/.env (1)
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The webui server logging can be configured via LOG_LEVEL and WEBUI_LOGS_DIR environment variables, with file logging enabled automatically in non-TTY environments when a logs directory is specified.
🔇 Additional comments (1)
components/webui/server/.env (1)
7-7: Hard-coded client proxies still target port 3000We’ve confirmed two lingering references in the Vite client config that must be updated now that the server listens on 3001:
• components/webui/client/vite.config.ts
- Line with
target: "http://localhost:3000/"- Line with
target: "ws://localhost:3000/"Please update these to port 3001—or better yet, read the backend URL from an env var (for example
VITE_SERVER_URL) to avoid future drift:- target: "http://localhost:3000/", + target: process.env.VITE_SERVER_URL || "http://localhost:3001/", … - target: "ws://localhost:3000/", + target: process.env.VITE_SERVER_URL?.replace(/^http/, "ws") || "ws://localhost:3001/",[fix_required]
added 2 commits
July 14, 2025 18:15
Contributor
Author
|
@hoophalab i merged main into this |
hoophalab
previously approved these changes
Jul 14, 2025
junhaoliao
reviewed
Jul 14, 2025
|
|
||
| // Only decorate if the region is set (i.e. s3 support is configured in package) | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| if (region) { |
Member
There was a problem hiding this comment.
can we check if the region is not null or empty here?
| const profile = settings.StreamFilesS3Profile; | ||
|
|
||
| // Only decorate if the region is set (i.e. s3 support is configured in package) | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
Member
There was a problem hiding this comment.
can we document why this lint suppression is needed?
| declare module "fastify" { | ||
| interface FastifyInstance { | ||
| s3Manager?: S3Manager; | ||
| export interface FastifyInstance { |
Member
There was a problem hiding this comment.
why do we want to export this now?
junhaoliao
previously approved these changes
Jul 14, 2025
junhaoliao
left a comment
Member
There was a problem hiding this comment.
deferring to @hoophalab 's review
added 2 commits
July 14, 2025 20:14
Contributor
Author
|
addressed @junhaoliao comments |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts(2 hunks)components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (3)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/S3Manager.js:0-0
Timestamp: 2025-01-17T23:25:38.165Z
Learning: In S3Manager.js, URL validation is handled by the URL constructor which throws TypeError for invalid URLs, making additional scheme (s3://) validation redundant.
components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts (3)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/S3Manager.js:0-0
Timestamp: 2025-01-17T23:25:38.165Z
Learning: In S3Manager.js, URL validation is handled by the URL constructor which throws TypeError for invalid URLs, making additional scheme (s3://) validation redundant.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/routes/query.js:9-10
Timestamp: 2025-01-17T22:33:23.547Z
Learning: When initializing S3Manager in log-viewer-webui, both StreamFilesS3Region and StreamFilesS3PathPrefix settings must be non-null for S3 functionality to work correctly.
components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts (6)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#743
File: components/log-viewer-webui/server/src/plugins/S3Manager.ts:76-76
Timestamp: 2025-04-07T09:30:20.684Z
Learning: In the S3Manager plugin, an `isEnabled` option should be added and checked instead of checking if the region is null. This would make the plugin's behavior more explicit.
Learnt from: junhaoliao
PR: y-scope/clp#743
File: components/log-viewer-webui/server/src/plugins/S3Manager.ts:76-76
Timestamp: 2025-04-07T09:30:20.684Z
Learning: In the S3Manager plugin, an `isEnabled` option should be added and checked instead of checking if the region is null. This would make the plugin's behavior more explicit.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Learnt from: haiqi96
PR: y-scope/clp#673
File: components/log-viewer-webui/server/src/routes/query.js:9-10
Timestamp: 2025-01-17T22:33:23.547Z
Learning: When initializing S3Manager in log-viewer-webui, both StreamFilesS3Region and StreamFilesS3PathPrefix settings must be non-null for S3 functionality to work correctly.
🪛 GitHub Actions: clp-lint
components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts
[error] 76-76: ESLint: Expected literal to be on the left side of !== (yoda). Two occurrences at column 13 and 32.
🔇 Additional comments (4)
components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts (1)
72-73: LGTM! Decorator name updated for consistency.The changes correctly update the decorator name from
s3ManagertoS3Managerto match the refactored plugin implementation. The conditional logic and usage remain appropriate.components/webui/server/src/fastify-v2/plugins/app/S3Manager/index.ts (3)
6-6: LGTM! Import renamed for consistency.The import rename from
fastifyPlugintofpfollows common naming conventions for this plugin utility.
8-10: LGTM! Configuration and typings imports added.The addition of settings import and PRE_SIGNED_URL_EXPIRY_TIME_SECONDS constant import supports the refactoring to centralize configuration within the plugin.
63-63: LGTM! Interface updated to match decorator name change.The FastifyInstance interface correctly reflects the renamed decorator from
s3ManagertoS3Manager.
junhaoliao
approved these changes
Jul 15, 2025
junhaoliao
approved these changes
Jul 15, 2025
3 tasks
junhaoliao
pushed a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
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
3 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.
Description
PR moves extract S3Manager into new fastify architecture. This is a chain of PRs which started with #1027
There are some minor style changes as well
Checklist
breaking change.
Validation performed
I am still testing... on aws. Likely @hoophalab will not be able to test since no access, so will have to rely on my validation.
Summary by CodeRabbit