-
Notifications
You must be signed in to change notification settings - Fork 4
Set up Docker development environment for Cyberstorm Remix #1663
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
base: master
Are you sure you want to change the base?
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR moves Cyberstorm Remix local development to a Docker-first workflow: adds Dockerfile.dev and entrypoint.dev.sh, introduces tools/nginx/new-localhost.conf, and updates docker-compose.remix.development.yml to add rsync sync/watch services, new volumes/secrets, and remove the remix-nginx-proxy. It updates hostname mappings in beta-switch and public assets from Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1663 +/- ##
=======================================
Coverage 11.54% 11.55%
=======================================
Files 319 319
Lines 22922 22922
Branches 507 508 +1
=======================================
+ Hits 2647 2649 +2
+ Misses 20275 20273 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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: 1
🧹 Nitpick comments (2)
tools/nginx/new-localhost.conf (1)
18-18: Consider using$hostfor consistency.The Host header is hardcoded to
localhost, buttools/ts-dev-proxy/nginx.confuses$hostthroughout. Using$hostwould preserve the original Host header (new.localhost) and maintain consistency across nginx configs.- proxy_set_header Host localhost; + proxy_set_header Host $host;docker-compose.remix.development.yml (1)
34-42: Document the volume and secret prerequisites.The configuration relies on pre-existing resources:
- External volume:
thunderstore_nginx_conf(must be created beforedocker-compose up)- File:
./build-secrets/.npmrc(must exist beforedocker-compose up)- File:
./tools/nginx/*.conf(nginx config files)Add a setup section to the project README or a
.docker-compose-initscript documenting these prerequisites to prevent startup failures for new developers.Would you like me to generate a setup verification script or update documentation to clarify these prerequisites?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.eslintrc.json(1 hunks)apps/cyberstorm-remix/README.md(3 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/src/index.ts(1 hunks)packages/ts-uploader/README.md(1 hunks)tools/nginx/new-localhost.conf(1 hunks)tools/scripts/cyberstorm-dev-entrypoint.sh(1 hunks)tools/ts-dev-proxy/nginx.conf(5 hunks)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (17)
.eslintrc.json (1)
1-3: LGTM!Standard ESLint root configuration marker for the monorepo.
packages/ts-uploader/README.md (1)
75-84: LGTM!Documentation correctly updated to reflect localhost-based development environment. The explanation about modern browser resolution of localhost is helpful.
packages/beta-switch/src/index.ts (1)
38-50: LGTM!Development environment URLs correctly updated to localhost/new.localhost, consistent with the PR's migration objectives.
apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js (1)
25-36: LGTM!Mirrors the TypeScript version's localhost migration, maintaining consistency across both implementations.
tools/ts-dev-proxy/nginx.conf (6)
10-10: LGTM!Upstream correctly updated to use Docker service name
cyberstorm-remix:3000.
16-16: LGTM!Server name correctly updated to
new.localhostfor the development environment.
43-43: LGTM!Host headers consistently updated to use
$hostthroughout the configuration, ensuring proper header propagation to upstream services.Also applies to: 49-49, 54-54, 59-59, 64-64, 69-69, 74-74, 79-79, 84-84, 89-89, 94-94, 99-99, 113-113, 118-118, 124-124, 129-129, 134-134, 139-139, 144-144, 149-149, 157-157, 165-165, 173-173, 181-181, 186-186, 191-191
197-197: LGTM!Server name correctly updated to
localhostfor the legacy Django backend.
219-221: LGTM!Backend proxy correctly updated to use Docker service name
django:8000with proper Host header propagation.
232-260: LGTM!New
auth.localhostserver block properly configured with gzip and proxy settings consistent with other blocks.apps/cyberstorm-remix/README.md (3)
7-13: LGTM!Setup instructions clearly updated to reflect the new Docker-based workflow with localhost routing. The note about SERVER_SIDE_API_URL vs VITE_API_URL is helpful for understanding the dual configuration.
29-35: LGTM!Environment variable examples correctly updated to use localhost-based URLs, consistent with the migration.
57-65: LGTM!ts-proxy section appropriately updated with simplified Docker-based approach and localhost URLs.
docker-compose.remix.development.yml (4)
21-22: Verify the npmrc secret file exists and is properly configured.The
npmrcsecret references./build-secrets/.npmrc, which must exist for Compose to start. Additionally, verify the entrypoint script properly configures npm to use this secret (typically by copying it to~/.npmrc).
19-20: Verify that thenginxservice is defined in the Docker Compose stack.The
SERVER_SIDE_API_URLenvironment variable referenceshttp://nginx, which depends on a service namednginxbeing declared in the same Docker Compose stack and accessible on thethunderstore_defaultnetwork. Confirm this service is properly configured.
12-15: Verify the entrypoint script exists and is executable.The entrypoint script mounted at
./tools/scripts/cyberstorm-dev-entrypoint.shmust exist in the repository for the container to start successfully. Confirm this file is present and has executable permissions before deploying this compose configuration.
24-32: Verify external volume documentation and nginx service dependency order.The
nginx-configuratorservice uses an external volumethunderstore_nginx_confthat must be pre-created (not managed by docker-compose), or the service will fail at startup. Ensure setup documentation includes instructions to create this volume or provision it beforehand. Additionally, if annginxservice consumes configs from this volume, add an explicitdepends_on: nginxto guarantee the configurator populates the volume before nginx starts consuming it.
d908f57 to
339c0c4
Compare
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 (1)
tools/nginx/new-localhost.conf (1)
11-21: Host header differs from server_name; consider aligning with$hostRight now the server_name is
new.localhostbut the proxiedHostis forced tolocalhost. Unless you have upstream logic that explicitly depends onHost: localhost, it’s safer and more consistent with the other nginx configs to forward the original host.You can keep behavior the same while aligning with ts-dev-proxy like this:
location / { proxy_pass http://remix_dev; proxy_http_version 1.1; - proxy_set_header Host localhost; + proxy_set_header Host $host; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $connection_upgrade; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.eslintrc.json(1 hunks)apps/cyberstorm-remix/README.md(3 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/src/index.ts(1 hunks)packages/ts-uploader/README.md(1 hunks)tools/nginx/new-localhost.conf(1 hunks)tools/scripts/cyberstorm-dev-entrypoint.sh(1 hunks)tools/ts-dev-proxy/nginx.conf(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .eslintrc.json
- apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js
- packages/ts-uploader/README.md
- tools/scripts/cyberstorm-dev-entrypoint.sh
- packages/beta-switch/src/index.ts
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (5)
apps/cyberstorm-remix/README.md (3)
7-13: Remix + backend/nginx bootstrap flow looks coherentThe new 1–5 setup steps line up with the compose file and nginx-configurator pattern: backend comes up first, then Remix + configurator share
thunderstore_nginx_conf, and you getlocalhost/new.localhostwithout hosts-file hacks. This reads cleanly and should be straightforward for devs to follow.
27-36: Env sample matches the new localhost domain modelThe
.envexample (site/beta/auth URLs and.localhostcookie domain plus__VITE_ADDITIONAL_SERVER_ALLOWED_HOSTS=.localhost) looks consistent with the nginx server blocks forlocalhost,new.localhost, andauth.localhost. No changes needed here.
57-65: ts-proxy section is aligned with the localhost switchUsing
PUBLIC_SITE_URL/PUBLIC_API_URLpointing athttp://localhostfits the updatedserver_name localhostblock ints-dev-proxy/nginx.conf. The steps are minimal and clear for people who want to point Nimbus at production via ts-proxy.docker-compose.remix.development.yml (1)
4-38: Compose wiring for Remix dev + nginx-configurator looks solidThe
cyberstorm-remixservice setup (workspace mount, split node_modules volumes, entrypoint script, andSERVER_SIDE_API_URL=http://nginx) cleanly matches the new dev flow. Thenginx-configuratorloop that syncstools/nginx/*.confinto the externalthunderstore_nginx_confvolume is a simple, robust way to feed the backend nginx.Only thing to keep in mind is that both the
thunderstore_defaultnetwork andthunderstore_nginx_confvolume must exist (which your README’s “run Thunderstore compose first” step already covers), so this looks good as-is.tools/ts-dev-proxy/nginx.conf (1)
9-260: ts-dev-proxy nginx updates align well with the new Docker-based dev setupPointing
remix_upstreamatcyberstorm-remix:3000, standardizingproxy_set_header Host $host, switching the Django upstream todjango:8000, and adding the dedicatedauth.localhostserver all fit the localhost-focused workflow and match the other config/README changes.As long as the ts-dev-proxy stack is on the same Docker network as
cyberstorm-remixanddjango, this configuration should behave as intended without additional tweaks.
339c0c4 to
468223f
Compare
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: 1
🧹 Nitpick comments (3)
apps/cyberstorm-remix/docker-compose.dev.yml (1)
30-32: Verify external dependencies and file paths.The docker-compose file assumes that:
- The
build-secrets/.npmrcfile exists (line 32)- The
thunderstore_defaultnetwork is pre-created (line 35)- The
thunderstore_nginx_confvolume is pre-created (line 16)If any of these don't exist, the service will fail to start. Consider adding validation or setup instructions.
Can you confirm these resources are reliably pre-created in the dev environment? If not, I can help add initialization logic or clearer documentation.
apps/cyberstorm-remix/README.md (2)
30-32: Document localhost routing prerequisites.The Quick Start directs developers to open
http://new.localhostandhttp://localhost, but doesn't mention the prerequisites:
- Adding entries to
/etc/hosts(or DNS configuration)- Nginx configuration for routing (
tools/nginx/new-localhost.conf)Consider adding a brief note before the browser step or link to detailed setup documentation.
34-42: Add .env configuration step to Manual Setup.Line 39 references
.env.examplebut the Manual Setup section doesn't explain where to find it or what environment variables are required. Consider adding a link or brief description.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.eslintrc.json(1 hunks)README.md(1 hunks)apps/cyberstorm-remix/Dockerfile.dev(1 hunks)apps/cyberstorm-remix/README.md(1 hunks)apps/cyberstorm-remix/docker-compose.build.yml(2 hunks)apps/cyberstorm-remix/docker-compose.dev.yml(1 hunks)apps/cyberstorm-remix/entrypoint.dev.sh(1 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)docker-compose.remix.development.yml(0 hunks)packages/beta-switch/README.md(1 hunks)packages/beta-switch/src/index.ts(1 hunks)packages/cyberstorm-forms/README.md(1 hunks)packages/cyberstorm-theme/README.md(1 hunks)packages/dapper-fake/README.md(1 hunks)packages/dapper-ts/README.md(1 hunks)packages/graph-system/README.md(1 hunks)packages/react-dnd/README.md(1 hunks)packages/thunderstore-api/README.md(1 hunks)packages/ts-api-react-actions/README.md(1 hunks)packages/ts-api-react-forms/README.md(1 hunks)packages/ts-api-react/README.md(1 hunks)packages/ts-uploader-react/README.md(1 hunks)packages/ts-uploader/README.md(1 hunks)packages/typed-event-emitter/README.md(1 hunks)tools/nginx/new-localhost.conf(1 hunks)tools/ts-dev-proxy/Dockerfile(0 hunks)tools/ts-dev-proxy/docker-compose.yml(0 hunks)tools/ts-dev-proxy/nginx.conf(0 hunks)
💤 Files with no reviewable changes (4)
- tools/ts-dev-proxy/nginx.conf
- docker-compose.remix.development.yml
- tools/ts-dev-proxy/docker-compose.yml
- tools/ts-dev-proxy/Dockerfile
✅ Files skipped from review due to trivial changes (8)
- packages/cyberstorm-theme/README.md
- packages/thunderstore-api/README.md
- packages/react-dnd/README.md
- packages/dapper-ts/README.md
- packages/beta-switch/README.md
- packages/ts-api-react/README.md
- packages/graph-system/README.md
- packages/dapper-fake/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/nginx/new-localhost.conf
- packages/beta-switch/src/index.ts
- .eslintrc.json
- packages/ts-uploader/README.md
- apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (11)
packages/cyberstorm-forms/README.md (1)
1-24: Documentation updates look good.The namespace and feature descriptions are consistent with other package documentation changes in the PR. Ensure the package.json for
cyberstorm-formshas been updated to reflect the@thunderstorenamespace.packages/typed-event-emitter/README.md (1)
1-28: Documentation updates look good.Consistent with namespace standardization across packages. Verify the corresponding package.json reflects the
@thunderstorenamespace.packages/ts-uploader-react/README.md (1)
1-9: Documentation updates align with package namespace standardization.Verify package.json for
ts-uploader-reacthas been updated to@thunderstore/ts-uploader-react.packages/ts-api-react-forms/README.md (1)
1-9: Documentation updates consistent with monorepo namespace standardization.Verify package.json reflects the
@thunderstorenamespace.packages/ts-api-react-actions/README.md (1)
1-9: Documentation updates align with monorepo namespace strategy.Verify corresponding package.json has been updated to reflect the
@thunderstorenamespace.README.md (1)
1-74: Root README restructured for better navigation.The shift from procedural setup to a high-level project overview is appropriate for a monorepo. Ensure
apps/cyberstorm-remix/README.mdcontains comprehensive setup instructions since it's now the primary reference point for getting started.apps/cyberstorm-remix/Dockerfile.dev (1)
1-13: No issues identified. Theentrypoint.dev.shscript exists and properly handles command argument passing viaexec su node -c "$*". Node 24.1.0 satisfies the package.json constraint of>=20.17.0, ensuring compatibility with project dependencies.Likely an incorrect or invalid review comment.
apps/cyberstorm-remix/docker-compose.build.yml (1)
1-16: No breaking change: docker-compose.build.yml is not currently used in active CI/CD.The commented build job in
.github/workflows/test.ymlshows the intended invocation is already from the repository root, withdocker compose -f docker-compose.build.yml build. The path changes (context:../../,../../build-secrets/.npmrc) are correct for this invocation context. The build job is currently disabled pending secret configuration, so there is no active CI/CD to break.apps/cyberstorm-remix/entrypoint.dev.sh (2)
20-25: Consider both node_modules locations in dependency check.The script only checks and installs dependencies in
/workspace/node_modules(line 22), but the docker-compose defines a separate mounted volume forapps/cyberstorm-remix/node_modules(nimbus_node_modules). Depending on your monorepo structure, you may need to handle both locations.Clarify: Does
yarn installat the workspace root also install app-level dependencies, or do you need a separate install inapps/cyberstorm-remix?
14-18: Verify workspace mount ownership for the node user.The script sets node_modules ownership (line 18) and later runs yarn as the node user (line 24). However, if
/workspaceis owned by root, the node user may not have write permissions despite owningnode_modules. Verify that the workspace mount is writable by the node user or adjust the Dockerfile.dev to handle this.Also applies to: 24-24
apps/cyberstorm-remix/README.md (1)
5-28: Docker Quick Start is clear and well-structured.The step-by-step flow is easy to follow and the command examples are correct. Good use of code blocks and clear progression from backend to frontend startup.
468223f to
611db41
Compare
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx (1)
22-29: Drop the extraconsole.log(keepconsole.erroronly).Right now dev will print the same payload twice (
log+error), which is especially noisy under StrictMode. Suggest keeping a singleconsole.error(or switching the extra line toconsole.debugif you really want a “breadcrumb” without duplicating stack output).apps/cyberstorm-remix/app/root.tsx (1)
551-563: Ads containers render, butAdsInitis disabled — likely breaks ads loading.If the intent is “disable ads in dev”, gate both the containers and
AdsInitbehind an env check; otherwise, re-enableAdsInitso production still injects the Nitro script.
♻️ Duplicate comments (1)
apps/cyberstorm-remix/entrypoint.dev.sh (1)
58-59: Fix command execution: don’t use$*here (argument corruption).
This is the same issue previously flagged:$*flattens args. Withsu -c, a robust pattern is:-# Execute the passed command as node -exec su node -c "$*" +# Execute the passed command as node (preserve argv) +exec su node -s /bin/sh -c 'exec "$@"' sh "$@"
🧹 Nitpick comments (7)
packages/ts-uploader/README.md (1)
75-84: Clarify the optional nature of the S3 endpoint change.The phrasing "you might need to change" is vague. Consider clarifying whether this is an expected requirement for the test environment or a workaround for specific scenarios. Also, the "Additionally" transition at line 79 doesn't clearly link the jsdom limitation to when/why developers should modify the S3 endpoint.
Consider rephrasing to make the relationship between the jsdom limitation and the S3 endpoint configuration more explicit, e.g.: "When running tests in jsdom, XMLHttpRequest to
localhost:9000may fail. In such cases, configureUSERMEDIA_S3_SIGNING_ENDPOINT_URLtohttp://localhost:9000/instead."README.md (3)
22-43: Package naming inconsistency: brackets don't use @Thunderstore scope.The packages are now scoped as
@thunderstore/package-name(per the updated package READMEs), but the root README lists them in brackets without the scope (e.g.,[cyberstorm],[ts-api-react]). This could confuse developers about the actual package names when installing or importing.Consider either:
- Update brackets to include the @Thunderstore scope:
[@thunderstore/cyberstorm]- Add a note explaining that all packages are scoped under
@thunderstore/
5-16: Docker setup guidance is deferred but not surfaced here.The restructured README effectively points to
cyberstorm-remix/README.mdfor setup, which aligns with the PR's Docker-first approach. However, the root README doesn't explicitly mention Docker as the primary development method. Since this PR is titled "docker dev env setup," consider adding a brief note in the intro or Key Projects section explicitly calling out Docker as the recommended development approach.For example, enhance the intro to mention Docker: "This monorepo contains the frontend applications and shared libraries for thunderstore.io, optimized for Docker-based development."
52-58: Common Commands table is helpful but missing Docker-specific commands.The table covers Yarn operations but doesn't mention Docker commands or a reference to the Docker setup guide. Since Docker is now central to development, consider adding a note or link directing developers to Docker setup instructions in the cyberstorm-remix README.
Dockerfile.dev (1)
1-6: Consider droppingiptablesif it’s unused; confirmnode:24-bookwormis intentional.
socatis used by the entrypoint;iptablesdoesn’t appear to be. Also please double-check the repo/tooling supports Node 24 as the dev baseline.tools/nginx/new-localhost.conf (1)
1-19: Proxy headers look correct (Host preserved); optional tweak: set non-upgrade Connection toclose.
Currentmapuses'' ''; many setups use'' close;for clarity/predictability.apps/cyberstorm-remix/app/root.tsx (1)
302-303: Consider removing the commented debug line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
Dockerfile.dev(1 hunks)README.md(1 hunks)apps/cyberstorm-remix/Dockerfile.development(0 hunks)apps/cyberstorm-remix/README.md(1 hunks)apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx(1 hunks)apps/cyberstorm-remix/app/root.tsx(6 hunks)apps/cyberstorm-remix/docker-compose.dev.yml(1 hunks)apps/cyberstorm-remix/entrypoint.dev.sh(1 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)apps/cyberstorm-remix/react-router.config.ts(0 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/README.md(1 hunks)packages/beta-switch/src/index.ts(1 hunks)packages/cyberstorm-forms/README.md(1 hunks)packages/cyberstorm-theme/README.md(1 hunks)packages/dapper-fake/README.md(1 hunks)packages/dapper-ts/README.md(1 hunks)packages/graph-system/README.md(1 hunks)packages/react-dnd/README.md(1 hunks)packages/thunderstore-api/README.md(1 hunks)packages/ts-api-react-actions/README.md(1 hunks)packages/ts-api-react-forms/README.md(1 hunks)packages/ts-api-react/README.md(1 hunks)packages/ts-uploader-react/README.md(1 hunks)packages/ts-uploader/README.md(1 hunks)packages/typed-event-emitter/README.md(1 hunks)tools/nginx/new-localhost.conf(1 hunks)tools/ts-dev-proxy/Dockerfile(0 hunks)tools/ts-dev-proxy/docker-compose.yml(0 hunks)tools/ts-dev-proxy/nginx.conf(0 hunks)
💤 Files with no reviewable changes (5)
- tools/ts-dev-proxy/Dockerfile
- tools/ts-dev-proxy/docker-compose.yml
- tools/ts-dev-proxy/nginx.conf
- apps/cyberstorm-remix/Dockerfile.development
- apps/cyberstorm-remix/react-router.config.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/thunderstore-api/README.md
- packages/cyberstorm-theme/README.md
- apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js
- packages/beta-switch/README.md
- packages/dapper-ts/README.md
- packages/ts-api-react-forms/README.md
- packages/typed-event-emitter/README.md
- packages/dapper-fake/README.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : Use `RouteErrorBoundary` in route components by exporting it as `ErrorBoundary` from the route component file
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : `RouteErrorBoundary` catches errors in loaders and Promise rejections - ensure Suspense and Await are used in the main component to handle loader Promise rejections
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Error boundaries should handle render-time errors, while errors caused by user interaction should be handled by the component logic itself
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
🧬 Code graph analysis (1)
packages/beta-switch/src/index.ts (1)
apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js (1)
betaDev(31-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (13)
packages/graph-system/README.md (1)
1-11: Looks good.Clear and concise documentation for the new package. Appropriately scoped naming and sufficient detail for users to understand the library's purpose.
packages/ts-api-react/README.md (1)
1-14: Looks good.Clear documentation with appropriate features and scripts. The standardized @Thunderstore naming and feature descriptions are consistent with other packages in the PR.
packages/ts-api-react-actions/README.md (1)
1-14: Looks good.Solid documentation emphasizing the action/mutation focus. The zod integration callout is helpful context for validation.
packages/cyberstorm-forms/README.md (1)
1-24: Looks good.Excellent example showing how to integrate the package with react-hook-form. Clear feature descriptions and consistent with the standardized documentation approach.
packages/react-dnd/README.md (1)
1-13: Looks good.Focused and clear. The emphasis on minimal dependencies is a good feature callout for a utility library.
packages/ts-uploader-react/README.md (1)
1-14: Looks good.Clear documentation with helpful dependency callout. The cross-reference to @thunderstore/ts-uploader provides good context for users understanding the package hierarchy.
README.md (1)
60-73: Looks good.The Contributing section is well-structured. Pre-commit hooks, Chromatic visual testing, and VS Code recommendations are all relevant. The note about Chromatic being automatic on PRs provides helpful context.
packages/beta-switch/src/index.ts (1)
38-50: Dev host updates look consistent withnew.localhostrouting.
Matches the expected dev endpoints (legacy →localhost, beta →new.localhost) and doesn’t impact the prod/QA branching.apps/cyberstorm-remix/docker-compose.dev.yml (1)
20-35: Secret/volume wiring seems reasonable; please verify the external deps exist.
In particular: external volumethunderstore_nginx_confand../../build-secrets/.npmrcon fresh clones.apps/cyberstorm-remix/README.md (1)
1-44: Docs match the new Docker dev flow and localhost endpoints.
Nice simplification vs the previous env-heavy setup.docker-compose.remix.development.yml (1)
2-69: Helper-service orchestration +depends_on: condition: service_completed_successfullyis a solid dev UX.
The sync/watch split and nginx-conf seeding is straightforward to reason about.apps/cyberstorm-remix/app/root.tsx (2)
3-18: Imports look consistent with the new root-levelErrorBoundary.Also applies to: 33-35
605-640: This suggestion is incorrect —useEffectcannot be used in ErrorBoundaries for error handling.ErrorBoundaries only catch errors thrown during render; useEffect runs after render completes (in the effect phase). Moving the logging into useEffect would break the pattern—errors caught by the boundary must be logged/reported synchronously during render.
The current implementation (logging directly in render) is correct for an ErrorBoundary. If duplicate logging from re-renders is a concern in practice, that would need a different approach (e.g., tracking the error object identity), but useEffect is not the solution.
The type check for
error.datais reasonable defensive programming and can be kept separately if needed.Likely an incorrect or invalid review 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.
Pull request overview
This PR modernizes the Docker development environment setup by migrating from custom .temp domains to standard .localhost domains, which are automatically resolved by modern browsers without requiring hosts file modifications. The changes streamline the developer onboarding experience and improve documentation across the monorepo.
Key Changes
- Docker Environment Refactor: Replaces the nginx-based proxy setup (tools/ts-dev-proxy) with a simpler configuration that uses
.localhostdomains and rsync-based file synchronization - Domain Migration: Updates all development URLs from
thunderstore.temp/new.thunderstore.temptolocalhost/new.localhostacross configuration files and code - Documentation Overhaul: Significantly improves README files across packages with feature descriptions, usage examples, and a clearer Quick Start guide for Docker-based development
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/ts-dev-proxy/* | Removed old nginx proxy setup (nginx.conf, docker-compose.yml, Dockerfile) |
| tools/nginx/new-localhost.conf | New lightweight nginx config for proxying to cyberstorm-remix service |
| docker-compose.remix.development.yml | Complete rewrite: adds rsync-based sync services, updated environment variables, and volume management |
| Dockerfile.dev | New unified development Dockerfile with entrypoint script |
| apps/cyberstorm-remix/entrypoint.dev.sh | New entrypoint handling dependency installation, nginx config, and localhost-to-django mapping for SSR |
| apps/cyberstorm-remix/docker-compose.dev.yml | New standalone docker-compose for frontend-only development |
| apps/cyberstorm-remix/Dockerfile.development | Removed in favor of root-level Dockerfile.dev |
| apps/cyberstorm-remix/react-router.config.ts | Removed (middleware configuration no longer needed) |
| apps/cyberstorm-remix/README.md | Replaced detailed manual setup instructions with concise Docker Quick Start guide |
| apps/cyberstorm-remix/app/root.tsx | Added new ErrorBoundary implementation, commented out ads initialization, added debug console.log |
| apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx | Added duplicate console.log before console.error |
| packages/beta-switch/src/index.ts | Updated dev environment hostnames from .temp to .localhost |
| apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js | Updated dev environment hostnames from .temp to .localhost |
| packages/*/README.md | Enhanced with package descriptions, features, and usage examples (10 packages updated) |
| packages/ts-uploader/README.md | Updated documentation to reference localhost instead of thunderstore.localhost |
| README.md | Restructured to focus on key projects and packages with improved navigation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx
Outdated
Show resolved
Hide resolved
611db41 to
0d2ce5d
Compare
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
♻️ Duplicate comments (10)
apps/cyberstorm-remix/README.md (1)
35-44: Manual Setup section still needs prerequisites/clarity (Node/Yarn versions + backend expectations).
This matches earlier feedback: the current text doesn’t say whether “Manual Setup” is frontend-only and what versions/tools are expected.apps/cyberstorm-remix/entrypoint.dev.sh (2)
39-46: Makedjango_ipsingle-valued + make/etc/hostsupdate idempotent.
getent hosts django | awk '{print $1}'can yield multiple IPs/lines; and the fallback append can duplicatelocalhostmappings across restarts.- django_ip=$(getent hosts django | awk '{print $1}') + django_ip="$(getent hosts django | awk 'NR==1{print $1; exit}')" if [ -n "$django_ip" ]; then @@ - } > /tmp/hosts && cat /tmp/hosts > /etc/hosts || printf "%s localhost\n" "$django_ip" >> /etc/hosts + } > /tmp/hosts && cat /tmp/hosts > /etc/hosts || { + grep -qxF "$django_ip localhost" /etc/hosts || printf "%s localhost\n" "$django_ip" >> /etc/hosts + }
58-59: Fix argument handling:$*should be"$@"(preserve arg boundaries).- exec su node -c "$*" + exec su node -c "$@"docker-compose.remix.development.yml (3)
12-20: Verify exclusion list is complete.The rsync
--deleteflag will remove files in/workspacethat don't exist in/src. Whilenode_modules,.turbo, and.cacheare excluded, verify that other generated artifacts (e.g.,dist,build,.react-router) don't need exclusion.#!/bin/bash # Find common build output directories in the repository fd -t d -d 3 'dist|build|.react-router|out|.next|.output' --exclude node_modules --exclude .git
41-41: Bind port to localhost only.The port binding
3000:3000exposes the service on all network interfaces, which poses a security risk. Bind to localhost:127.0.0.1:3000:3000.- - "3000:3000" + - "127.0.0.1:3000:3000"
53-54: Fix YAML indentation for networks.The
networkskey is mis-indented. It should align withenvironmentas a sibling under the service definition.environment: - NODE_ENV=development ... - networks: - - thunderstore_default + networks: + - thunderstore_defaultapps/cyberstorm-remix/app/root.tsx (3)
302-302: Remove commented debug code.The commented
console.logshould be removed before merging to production.- // console.log(process.env.VITE_DEVELOPMENT); -
562-562: Clarify AdsInit removal.The
AdsInitcomponent is commented out but the component code remains (lines 644-694). If ads are disabled for development, consider using an environment check or removing the unused component.Is this temporary for development testing, or should the entire
AdsInitcomponent be removed?
621-639: Fix grammar in error message.Line 635 contains a grammatical error: "Server something error happens" should be more professional.
<div className="error__flavor"> - Beep boop. Server something error happens. + Beep boop. A server error occurred. </div>apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx (1)
26-27: Remove duplicate console logging.Both
console.logandconsole.errorare logging the same error. Theconsole.logon line 26 is redundant.} else if (error) { - console.log("Error boundary caught error", error); console.error("Error boundary caught error", error); }
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/root.tsx (2)
605-606: Clean up commented exports.The commented-out export and TODO comments should be removed if the ErrorBoundary implementation is finalized.
-// export { RouteErrorBoundary as ErrorBoundary } from "app/commonComponents/ErrorBoundary"; -// REMIX TODO: We don't have any data available in the root ErrorBoundary, so we might want to change the designs export function ErrorBoundary() {
607-620: LGTM - error boundary logic.The error handling logic correctly distinguishes between production (Sentry reporting) and development (console logging). However, consider cleaning up the commented loader code (lines 608-614) if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
Dockerfile.dev(1 hunks)README.md(1 hunks)apps/cyberstorm-remix/Dockerfile.development(0 hunks)apps/cyberstorm-remix/README.md(1 hunks)apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx(1 hunks)apps/cyberstorm-remix/app/root.tsx(6 hunks)apps/cyberstorm-remix/docker-compose.dev.yml(1 hunks)apps/cyberstorm-remix/entrypoint.dev.sh(1 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)apps/cyberstorm-remix/react-router.config.ts(0 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/README.md(1 hunks)packages/beta-switch/src/index.ts(1 hunks)packages/cyberstorm-forms/README.md(1 hunks)packages/cyberstorm-theme/README.md(1 hunks)packages/dapper-fake/README.md(1 hunks)packages/dapper-ts/README.md(1 hunks)packages/graph-system/README.md(1 hunks)packages/react-dnd/README.md(1 hunks)packages/thunderstore-api/README.md(1 hunks)packages/ts-api-react-actions/README.md(1 hunks)packages/ts-api-react-forms/README.md(1 hunks)packages/ts-api-react/README.md(1 hunks)packages/ts-uploader-react/README.md(1 hunks)packages/ts-uploader/README.md(1 hunks)packages/typed-event-emitter/README.md(1 hunks)tools/nginx/new-localhost.conf(1 hunks)tools/ts-dev-proxy/Dockerfile(0 hunks)tools/ts-dev-proxy/docker-compose.yml(0 hunks)tools/ts-dev-proxy/nginx.conf(0 hunks)
💤 Files with no reviewable changes (5)
- tools/ts-dev-proxy/nginx.conf
- tools/ts-dev-proxy/Dockerfile
- tools/ts-dev-proxy/docker-compose.yml
- apps/cyberstorm-remix/Dockerfile.development
- apps/cyberstorm-remix/react-router.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cyberstorm-theme/README.md
- packages/thunderstore-api/README.md
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/ts-uploader/README.md
- packages/graph-system/README.md
- packages/dapper-ts/README.md
- tools/nginx/new-localhost.conf
- packages/beta-switch/src/index.ts
- packages/ts-uploader-react/README.md
- packages/dapper-fake/README.md
- apps/cyberstorm-remix/docker-compose.dev.yml
- packages/typed-event-emitter/README.md
- Dockerfile.dev
- packages/ts-api-react-forms/README.md
- packages/ts-api-react-actions/README.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : Use `RouteErrorBoundary` in route components by exporting it as `ErrorBoundary` from the route component file
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Applies to apps/cyberstorm-remix/docs/**/*.route.{ts,tsx} : `RouteErrorBoundary` catches errors in loaders and Promise rejections - ensure Suspense and Await are used in the main component to handle loader Promise rejections
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
📚 Learning: 2025-12-09T12:41:16.448Z
Learnt from: CR
Repo: thunderstore-io/thunderstore-ui PR: 0
File: apps/cyberstorm-remix/docs/error-boundary.md:0-0
Timestamp: 2025-12-09T12:41:16.448Z
Learning: Error boundaries should handle render-time errors, while errors caused by user interaction should be handled by the component logic itself
Applied to files:
apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsxapps/cyberstorm-remix/app/root.tsx
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (12)
apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js (1)
25-36: Dev hostname/tld switch tolocalhost/new.localhostlooks consistent with the redirect logic.
Minor gotcha: if someone accesses dev via127.0.0.1, the button will treat it as “neither legacy nor beta” and redirect tolocalhost/new.localhost—fine if intentional, but worth sanity-checking.packages/react-dnd/README.md (1)
1-9: Doc refresh is clear and aligns with scoped package naming.packages/ts-api-react/README.md (1)
1-9: Doc update reads well; feature bullets are concrete and helpful.packages/beta-switch/README.md (1)
1-15: README now explains the DOM hooks + behavior clearly (esp. injection targets).packages/cyberstorm-forms/README.md (1)
1-19: Nice README improvement: features + import snippet make the package easier to adopt.README.md (1)
5-44: Restructure is a net win (quick “what is here / where do I go”).apps/cyberstorm-remix/entrypoint.dev.sh (1)
48-55: Sanity-check port 80 forwarding doesn’t conflict with other processes in this dev container.
If anything else binds:80(nginx, etc.), socat will fail and you’ll silently lose the SSR routing behavior.docker-compose.remix.development.yml (4)
2-10: LGTM!The nginx config copy service is correctly configured as a one-time setup task.
22-32: LGTM - continuous sync service.The watch service will continuously sync changes every 5 seconds. Ensure the SYNC_INTERVAL value balances responsiveness with resource usage.
55-68: LGTM - environment configuration.The environment variables and secrets configuration looks correct for the Vite development setup with localhost domains.
70-85: LGTM - volumes and secrets configuration.The volume and secrets declarations are correctly configured. The external nginx conf volume and npmrc secret setup look appropriate.
apps/cyberstorm-remix/app/root.tsx (1)
5-5: LGTM - error handling imports.The new imports for error boundary functionality are correctly added.
Also applies to: 17-17, 33-33
0d2ce5d to
b680ab9
Compare
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: 2
🧹 Nitpick comments (3)
apps/cyberstorm-remix/README.md (2)
23-23: Clarify when-vflag is necessary.The
-vflag removes volumes, which will delete database data and other persisted state. Consider explaining when this nuclear option is needed versus justdocker compose downand--build.
36-38: Consider clarifying backend requirement.The section title could be more explicit that the backend still requires Docker. This would align with the past review feedback about documenting the full stack requirement.
-## Manual Setup +## Manual Frontend Setup -If you prefer running Node locally instead of in Docker: +If you prefer running the **frontend** locally instead of in Docker, follow these steps. The backend should still be run via Docker as described above.docker-compose.remix.development.yml (1)
72-74: Document npmrc secret file requirement.The
build-secrets/.npmrcfile is required but may not be obvious to new developers. Ensure this is documented in setup instructions or a README, and verify the path is in.gitignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile.dev(1 hunks)apps/cyberstorm-remix/README.md(1 hunks)apps/cyberstorm-remix/entrypoint.dev.sh(1 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/src/index.ts(1 hunks)tools/nginx/new-localhost.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/beta-switch/src/index.ts
- apps/cyberstorm-remix/entrypoint.dev.sh
- apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (4)
tools/nginx/new-localhost.conf (1)
1-24: LGTM!The nginx configuration correctly handles websocket upgrades, preserves the Host header, disables caching for development, and proxies to the Remix service. The past concern about the Host header has been addressed.
Dockerfile.dev (2)
4-6: System dependencies installed for SSR routing.The
iptablesandsocatpackages are likely used by the entrypoint script for development SSR routing. While appropriate for a dev container, consider adding a comment explaining their purpose.
1-1: Node.js 24 is available and appropriate for this project.Node.js 24 was released on May 6, 2025, and entered Active LTS on October 28, 2025, with maintenance through April 30, 2028. The project's
package.jsonspecifies"engines": { "node": ">=20.17.0" }, which fully supports Node 24. Usingnode:24-bookworminDockerfile.devis valid and current.Likely an incorrect or invalid review comment.
docker-compose.remix.development.yml (1)
13-28: Well-designed sync strategy.The one-time sync followed by continuous watch pattern with atomic nginx config updates (using tmp file + mv) is a solid approach for development file synchronization.
b680ab9 to
340658e
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
- Added Dockerfile.dev for development setup with Node.js and necessary dependencies. - Created entrypoint script to manage directory permissions and install dependencies. - Updated docker-compose configuration for local development, including new sync and watch services. - Adjusted beta-switch script to use localhost for development. - Added new Nginx configuration for local development.
340658e to
5554b2c
Compare
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: 1
🧹 Nitpick comments (1)
apps/cyberstorm-remix/README.md (1)
36-45: Add Node.js and Yarn version requirements to Manual Setup.The Manual Setup section lacks prerequisites similar to those flagged in earlier review feedback. Add Node version and Yarn specifications to clarify requirements.
Apply this diff to document prerequisites:
## Manual Setup If you prefer running Node locally instead of in Docker: +### Prerequisites + +- [Node.js](https://nodejs.org/) (version 18.x or later recommended) +- [Yarn](https://yarnpkg.com/) (classic, v1.x) + +### Steps + -1. **Install Dependencies**: `yarn install` (in repo root) +1. **Install Dependencies**: From the repo root, run `yarn install` 2. **Configure Env**: Copy `.env.example` to `.env` in `apps/cyberstorm-remix`. 3. **Run Dev Server**: ```bash yarn workspace @thunderstore/cyberstorm-remix dev --port 3000 --host 0.0.0.0 ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile.dev(1 hunks)apps/cyberstorm-remix/README.md(1 hunks)apps/cyberstorm-remix/entrypoint.dev.sh(1 hunks)apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js(1 hunks)docker-compose.remix.development.yml(1 hunks)packages/beta-switch/src/index.ts(1 hunks)tools/nginx/new-localhost.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/beta-switch/src/index.ts
- tools/nginx/new-localhost.conf
- apps/cyberstorm-remix/entrypoint.dev.sh
- apps/cyberstorm-remix/public/cyberstorm-static/scripts/beta-switch.js
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (5)
docker-compose.remix.development.yml (3)
13-28: LGTM on rsync sync/watch services structure.The rsync initialization and watch pattern using atomic file operations (temp file + mv for nginx config) is solid. The
set -eerror handling is appropriate.
69-70: Verify externalthunderstore_nginx_confvolume exists before compose startup.This external volume must be created separately before
docker compose up. Confirm that Docker volume creation instructions or bootstrap steps are documented (e.g., in a setup script, README, or additional documentation).Also verify that
build-secrets/.npmrcexists locally, as it's referenced by the Docker Compose configuration at line 51.
74-74: Verifybuild-secrets/.npmrcexists and is properly excluded from version control.The npmrc secret is expected at
./build-secrets/.npmrc(relative to docker-compose file). Ensure:
- This file exists for local development
- The
.npmrcandbuild-secrets/are properly.gitignore'd to prevent credential leaks- Setup documentation instructs developers how to create this file
Dockerfile.dev (2)
6-7: File exists and is properly tracked.The
apps/cyberstorm-remix/entrypoint.dev.shscript is present in the repository and tracked in git. TheRUN chmod +xcommand on line 7 will properly make it executable at build time.
1-1: Use Node 20 instead of Node 24 in Dockerfile.dev.Your project pins Node to the 20.x branch (see
.github/actions/get-node-version/package.jsonwith^20.17.0), whilenode:24-bookwormcreates a mismatch between development and CI environments. Change tonode:20-bookwormfor consistency.⛔ Skipped due to learnings
Learnt from: anttimaki Repo: thunderstore-io/thunderstore-ui PR: 1559 File: packages/ts-api-react/package.json:16-18 Timestamp: 2025-10-01T11:36:11.065Z Learning: In the thunderstore-ui repository, exact Node version pinning (e.g., "24.9.0") is intentionally used in package.json engines fields rather than semver ranges, to ensure consistency across environments.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

No description provided.