Conversation
|
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideAdds Docker/Kamal-based deployment support with preview environments, updates CI workflows for production and preview deployments, adjusts Next.js image settings for reverse-proxy/docker compatibility, and bumps core frontend dependencies to the latest patch versions. Sequence diagram for preview environment deployment and PR commentsequenceDiagram
actor Dev
participant GH as GitHub
participant WF as deploy-preview-app.yml
participant Kamal
participant Reg as ContainerRegistry
participant Srv as Server
participant PR as PullRequest
Dev->>GH: Push to feature branch
GH-->>WF: Trigger workflow (push, branch != main/next)
WF->>WF: Compute BRANCH_SLUG
WF->>WF: Set KAMAL_APP_NAME, KAMAL_APP_DOMAIN
WF->>Reg: Build & push Docker image
WF->>Kamal: kamal deploy -d preview
Kamal->>Srv: Provision/Update preview container
Kamal->>Srv: Run Next.js app (NODE_ENV=preview)
WF->>GH: github-script list open PRs for branch
GH-->>WF: Matching PR (if any)
WF->>PR: Create or update comment
Note over WF,PR: "## Preview deployment ready" with preview URL
Dev->>PR: Open PR, click preview URL
Sequence diagram for preview environment removal on PR close or branch deletesequenceDiagram
actor Maint as Maintainer
participant GH as GitHub
participant WF as deploy-preview-app.yml
participant Kamal
participant Srv as Server
participant PR as PullRequest
Maint->>GH: Close PR or delete branch
GH-->>WF: Trigger remove-preview job
WF->>WF: Determine branch name
WF->>WF: Compute BRANCH_SLUG
WF->>WF: Set KAMAL_APP_NAME, KAMAL_APP_DOMAIN
WF->>Kamal: kamal remove -y -d preview
Kamal->>Srv: Stop and remove preview container
alt Event is pull_request closed
WF->>PR: Comment "Preview deployment removed"
else Branch delete event
WF->>GH: No PR comment
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for making updates to your pull request. Our team will take a look and provide feedback as soon as possible. Please wait for any GitHub Actions to complete before editing your pull request. If you have any additional questions or concerns, feel free to let us know. Thank you for your contributions! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
lint-stagedpattern inpackage.jsonwas changed from./src/**/*.{ts,js,jsx,tsx}to./src/**/*.{mx}, which will effectively disable linting/formatting on real source files; this looks accidental and should be adjusted to the intended extensions. - The heading text change to
The FAIR Data wave (next-21)insrc/pages/index.tsxappears environment/version-specific and may not be suitable for production UI; consider removing the suffix or deriving it dynamically if needed for debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `lint-staged` pattern in `package.json` was changed from `./src/**/*.{ts,js,jsx,tsx}` to `./src/**/*.{mx}`, which will effectively disable linting/formatting on real source files; this looks accidental and should be adjusted to the intended extensions.
- The heading text change to `The FAIR Data wave (next-21)` in `src/pages/index.tsx` appears environment/version-specific and may not be suitable for production UI; consider removing the suffix or deriving it dynamically if needed for debugging.
## Individual Comments
### Comment 1
<location path="package.json" line_range="35" />
<code_context>
},
"lint-staged": {
- "./src/**/*.{ts,js,jsx,tsx}": [
+ "./src/**/*.{mx}": [
"yarn lint --fix",
"yarn format"
</code_context>
<issue_to_address>
**issue (bug_risk):** The lint-staged glob `./src/**/*.{mx}` looks off and will likely stop linting from running on your TS/JS files.
The previous glob `./src/**/*.{ts,js,jsx,tsx}` matched your TS/JS files, but `./src/**/*.{mx}` matches none of them. Unless this was intentional, please restore a multi-extension glob so lint/format still run on your source files.
</issue_to_address>
### Comment 2
<location path=".github/workflows/deploy-app.yml" line_range="66" />
<code_context>
+ # Step 8: Initial Kamal setup (only needed once per server)
+ # Note: This step might need to be run manually on the server first time
+ # to add the user to the docker group: `sudo usermod -aG docker $USER | newgrp docker | docker ps`
+ - run: kamal setup
</code_context>
<issue_to_address>
**issue (bug_risk):** The production deployment workflow stops at `kamal setup` and never performs an actual deploy.
Given the workflow name and comments, this job appears intended to deploy on pushes to `next`, but it currently only runs `kamal setup`, which is typically a one-time or infrequent operation. Consider adding a `kamal deploy` (e.g., `kamal deploy -d <env>`) step so new pushes actually roll out to production instead of just re-running setup.
</issue_to_address>
### Comment 3
<location path="config/deploy.yml" line_range="2" />
<code_context>
+# Kamal Deployment Configuration
+# This file configures how your Nuxt.js application is deployed using Kamal
+# Kamal is a deployment tool that automates containerized app deployment
+
</code_context>
<issue_to_address>
**nitpick (typo):** The deploy config comments reference Nuxt.js while this repo is using Next.js.
Please update the comments to reference "Next.js" instead of "Nuxt.js" to align with the actual framework used in this repo and the Dockerfile.
Suggested implementation:
```
# This file configures how your Next.js application is deployed using Kamal
```
```
# Name of your Next.js application. Used to uniquely configure containers.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }, | ||
| "lint-staged": { | ||
| "./src/**/*.{ts,js,jsx,tsx}": [ | ||
| "./src/**/*.{mx}": [ |
There was a problem hiding this comment.
issue (bug_risk): The lint-staged glob ./src/**/*.{mx} looks off and will likely stop linting from running on your TS/JS files.
The previous glob ./src/**/*.{ts,js,jsx,tsx} matched your TS/JS files, but ./src/**/*.{mx} matches none of them. Unless this was intentional, please restore a multi-extension glob so lint/format still run on your source files.
| # Step 8: Initial Kamal setup (only needed once per server) | ||
| # Note: This step might need to be run manually on the server first time | ||
| # to add the user to the docker group: `sudo usermod -aG docker $USER | newgrp docker | docker ps` | ||
| - run: kamal setup |
There was a problem hiding this comment.
issue (bug_risk): The production deployment workflow stops at kamal setup and never performs an actual deploy.
Given the workflow name and comments, this job appears intended to deploy on pushes to next, but it currently only runs kamal setup, which is typically a one-time or infrequent operation. Consider adding a kamal deploy (e.g., kamal deploy -d <env>) step so new pushes actually roll out to production instead of just re-running setup.
| @@ -0,0 +1,75 @@ | |||
| # Kamal Deployment Configuration | |||
| # This file configures how your Nuxt.js application is deployed using Kamal | |||
There was a problem hiding this comment.
nitpick (typo): The deploy config comments reference Nuxt.js while this repo is using Next.js.
Please update the comments to reference "Next.js" instead of "Nuxt.js" to align with the actual framework used in this repo and the Dockerfile.
Suggested implementation:
# This file configures how your Next.js application is deployed using Kamal
# Name of your Next.js application. Used to uniquely configure containers.
📝 Changed routes:
Commit 9d57666 (https://fairdataihub-website-dlhi9z93h-fairdataihub.vercel.app). |
| }); | ||
| } | ||
|
|
||
| comment-preview-on-pr: |
There was a problem hiding this comment.
This doesn't trigger or wait for an actual deploy it seems. You could remove this and rely on the post-deploy comment from the deploy-ui job or update the message in this to say something along the lines of "Preview in progress"
There was a problem hiding this comment.
comment-preview-on-pr has been broken. need to fix it. THis is for when you open the pr AFTER the app is deployed. still haven't figured out what the issue is
| APP_DOMAIN="website-${BRANCH_SLUG}.fairdataihub.org" | ||
|
|
||
| echo "KAMAL_APP_NAME=${APP_NAME}" >> "$GITHUB_ENV" | ||
| echo "KAMAL_APP_DOMAIN=${APP_DOMAIN}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
This overrides the hardcoded variable values. Should we just place them in the github secrets?
There was a problem hiding this comment.
no because this is expected. the app name and domain will change for each branch
| delete: | ||
|
|
||
| jobs: | ||
| deploy-ui: |
There was a problem hiding this comment.
deploy-app.yml uses docker/setup-buildx-action@v3 so we could probably just add it onto here as well
There was a problem hiding this comment.
i was using the vercel deployment example. need to see if it helps
| # ENV NEXT_TELEMETRY_DISABLED=1 | ||
|
|
||
| # Copy production dependencies and build output | ||
| COPY --from=dependencies --chown=node:node /app/node_modules ./node_modules |
There was a problem hiding this comment.
Do we need all the dependencies in node_modules? We could build next.js in standalone mode from what I see in their documentation
There was a problem hiding this comment.
We could test and see if it works
There was a problem hiding this comment.
Tried this out and it went from 1gb image size to now 466MB. Build time was much faster with this enabled
| @@ -0,0 +1 @@ | |||
| # Secrets for preview deployments No newline at end of file | |||
There was a problem hiding this comment.
To auto load these secrets for preview with renaming the file to: secrets.preview
Then in the config file adding -d preview to kamal deploy/setup will work out
There was a problem hiding this comment.
intersting i though -env was the correct way. will have to fix
2 participants
This PR adds docker support for our nextjs application. It also support the generation of preview environments and preview urls for review. currently this is not how i would do staging environment based deployment but we are getting there. First steps
Summary by Sourcery
Introduce Docker/Kamal-based deployment for the Next.js app, including preview and production workflows, and adjust app configuration and dependencies accordingly.
New Features:
Enhancements:
CI:
Deployment: