-
Notifications
You must be signed in to change notification settings - Fork 2
Improvements #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements #12
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces infrastructure and architectural changes to optimize the Docker build and application routing. The Dockerfile switches from Alpine-based Bun to a Debian-based image with a headless-shell final stage, integrating a new node_modules cleanup script executed during the production build. The application server refactors request handling by introducing a Router utility class that centralizes route registration and matching. Schema changes adjust the default sleep timeout to 0. New utility modules handle routing logic and aggressive module cleanup. Related tests are updated accordingly. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention during review:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/router.ts (1)
21-27: Be explicit about how route paths are interpreted as regular expressions
addwrapspathinnew RegExp(\^${path}$`),so any regex metacharacters inpathbecome part of the pattern; tooling also flags this as a variable‑based regex. That’s fine while you only pass static literals like/v1/health`, but if you ever start composing paths dynamically or from external input it could yield surprising matches or even ReDoS‑prone patterns.If you want literal paths by default, consider escaping regex metacharacters when building the pattern or adding a separate overload that accepts a precompiled
RegExpwhen you explicitly need regex routing.src/utils/clean-modules.ts (1)
5-202: node_modules cleanup is thorough; consider configurability and long‑term safetyThis script does a nice job of parallelising cleanup, logging progress, and gracefully skipping missing paths. Two areas to keep in mind:
NODE_MODULESis hard‑coded to/app/node_modules, which ties the script to the current Docker layout. If you ever want to reuse it outside the image build (or move the app), deriving this fromprocess.cwd()or an env var would make it more robust.- The deletion patterns are intentionally aggressive (
*.txt,*.ts/*.tsx/*.jsx, test directories, and specific subtrees under Lighthouse, axe-core, zod, etc.). That’s great for shrinking the image, but it’s worth validating via tests that no runtime code (especially around thereports/Lighthouse functionality) depends on any of the removed assets.If you start seeing dependency‑level quirks, introducing a small allow‑list or per‑package tuning hook here would be a good next step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.dockerignore(1 hunks)Dockerfile(1 hunks)src/schemas/screenshot.schema.ts(1 hunks)src/server.ts(1 hunks)src/utils/clean-modules.ts(1 hunks)src/utils/router.ts(1 hunks)tests/unit/screenshot.schema.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/server.ts (1)
src/utils/router.ts (1)
Router(18-40)
🪛 ast-grep (0.39.9)
src/utils/router.ts
[warning] 23-23: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${path}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (5)
tests/unit/screenshot.schema.test.ts (1)
14-27: Default sleep expectation updated correctlyThe test now matches the new default of 0 and keeps the rest of the defaults consistent, so it tracks the intended removal of the implicit 3s wait.
src/schemas/screenshot.schema.ts (1)
3-8: Sleep default moved to 0 as intendedThis keeps the existing range constraints while removing the implicit 3s delay; just make sure any clients that relied on the previous default now pass a non‑zero
sleepexplicitly if they still need it..dockerignore (1)
15-20: Ignoring lighthouse/ keeps the build context leanAdding
lighthouse/to the ignore list is consistent with other test artifacts and avoids shipping local Lighthouse data into image builds.src/server.ts (1)
8-19: Router integration cleanly centralises routingThe router setup registers the existing endpoints with their methods and delegates
fetchtorouter.handle, giving a clear single routing surface while preserving the 404 behaviour for unmatched paths.Dockerfile (1)
1-49: Docker multi‑stage layout looks solid; verify impact of aggressive node_modules cleanupThe new base→final flow (Debian Bun stage for
bun install --production+ cleanup, thenchromedp/headless-shellwith fonts, non‑rootchromeuser, andtinientrypoint) is coherent and should yield a lean, well‑behaved runtime image.The main thing to watch is running
clean-modules.tsright after install, since it strips a lot of docs, TS sources, tests, and some subtrees under dependencies. That’s great for size, but please double‑check that thereportsendpoint and any e2e flows using Lighthouse/Playwright still have all the runtime pieces they need innode_modules.
Docker Image Stats
Screenshot benchmark: Average of 3 runs on https://appwrite.io |
What does this PR do?
Test Plan
Manual, Unit & E2E tests.
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
Release Notes
Changes
Infrastructure & Performance