Skip to content

Conversation

@hamo-o
Copy link
Contributor

@hamo-o hamo-o commented Mar 29, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

SSR 서버를 띄우기 위한 파일 경로가 잘못 설정되어 있어 수정해서 다시 올립니다.

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

Summary by CodeRabbit

  • New Features
    • Enhanced deployment by introducing dynamic directory management and automated, timestamped backup creation for frontend releases.
    • Updated build and preview processes to support a dedicated server build for improved server-side rendering.
    • Improved server configuration with environment-sensitive file serving and flexible port assignment for a more robust production experience.

@hamo-o hamo-o added this to the 7️⃣ 7차 스프린트 milestone Mar 29, 2025
@hamo-o hamo-o requested review from dioo1461, efdao and kwon204 March 29, 2025 04:34
@hamo-o hamo-o self-assigned this Mar 29, 2025
@coderabbitai
Copy link

coderabbitai bot commented Mar 29, 2025

Walkthrough

This pull request updates the frontend deployment and build processes. The deployment script now dynamically determines directory paths using the APP_DIR variable, adds backup functionality with timestamped archives, updates file copy commands, and revises rollback logic. In addition, the build configuration is enhanced by introducing a new server build script and modifying existing build and preview scripts in package.json, while the server configuration now supports dynamic port assignment, environment-based HTML path determination, and conditional static file serving.

Changes

File(s) Change Summary
script/deploy/frontend_deploy.sh Updated SSR_DIR to use $APP_DIR, added BACKUP_DIR and TIMESTAMP variables; implemented a backup process with tar.gz archiving; updated copy and rollback commands.
frontend/package.json Added "build:server" script; updated "build" and "preview" scripts to run client and server builds sequentially; added "cross-env" dependency.
frontend/server/index.js Introduced isProduction, PORT, and htmlPath constants; modified static file serving to use different directories based on environment; updated port usage in server startup.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant DeployScript as script/deploy/frontend_deploy.sh
  participant FileSystem

  User ->> DeployScript: Start Deployment
  DeployScript ->> FileSystem: Check/Create backup directory ($BACKUP_DIR)
  DeployScript ->> FileSystem: Archive FRONTEND_DIR using TIMESTAMP
  DeployScript ->> FileSystem: Copy new client files from $APP_DIR/frontend/dist/client to $FRONTEND_DIR
  alt Rollback Triggered
    DeployScript ->> FileSystem: Extract backup file from $BACKUP_DIR using TIMESTAMP
  end
Loading
sequenceDiagram
  participant Developer
  participant BuildProcess

  Developer ->> BuildProcess: Trigger "build" script
  BuildProcess ->> BuildProcess: Execute "build:client"
  BuildProcess ->> BuildProcess: Execute "build:server" (with SSR)
  BuildProcess ->> BuildProcess: Run "tsc -b" for TypeScript compilation
  BuildProcess -->> Developer: Build complete
Loading

Possibly related PRs

  • [ALL-Feat] 이사하기 #340: Addresses similar modifications in the frontend_deploy.sh script regarding backup processes and dynamic directory management.

Suggested reviewers

  • kwon204
  • dioo1461
  • efdao

Poem

I'm a hopping rabbit in the code field today,
Watching backups and builds in a lively display.
With dynamic paths and servers that glow,
I bounce through the scripts, letting flexibility flow.
Carrots and code, a joyous array!
🥕🐇
Cheers to our changes, hip-hip-hooray!

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@hamo-o hamo-o changed the title [All-Feat] Nginx 리버스 프록시 설정 [All-Feat] Nginx 리버스 프록시 설정, 경로 수정 Mar 29, 2025
Copy link

@coderabbitai coderabbitai bot left a 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)
script/deploy/nginx/frontend.conf (1)

50-55: Separate static asset handling with caching is consistent with SSR best practices.
Providing a 7-day cache for static resources is good. If you version your assets by hash, consider extending the cache duration for better performance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d33db15 and 22725cfa74db8b5012994c361bf63804dc24b7ba.

📒 Files selected for processing (2)
  • script/deploy/frontend_deploy.sh (1 hunks)
  • script/deploy/nginx/frontend.conf (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/nginx/frontend.conf (1)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#350
File: script/deploy/nginx/frontend.conf:49-51
Timestamp: 2025-03-25T07:32:54.031Z
Learning: For Nginx configurations with SSR (Server-Side Rendering), caching directives like 'expires' and 'Cache-Control' headers should be removed from proxy location blocks to prevent API requests from being cached by browsers. Static assets should have their own location blocks with appropriate caching settings.
🔇 Additional comments (2)
script/deploy/frontend_deploy.sh (1)

7-7: Switch to dynamic SSR directory path is beneficial.
Good improvement for flexibility and maintainability. Just ensure $APP_DIR is correctly set before usage in different environments.

script/deploy/nginx/frontend.conf (1)

37-42:

Details

❓ Verification inconclusive

Proxy configuration for SSR looks appropriate.
By specifying proxy_http_version 1.1, Upgrade, and Connection headers, you correctly enable WebSocket and SSE. Confirm that port 5173 is indeed where your SSR process is listening in production.

Also applies to: 48-48


🏁 Script executed:

#!/bin/bash
# Verify SSR is actually running on port 5173
ss -tlnp | grep ':5173'

Length of output: 116


Below is an updated verification step to help confirm that your SSR process is indeed running on port 5173. Since the original command using ss failed due to the command not being available in the environment, please run the following fallback script to check using lsof:

Rewritten Review Comment:
Ensure SSR Port Verification for Nginx Proxy

The proxy configuration for SSR looks appropriate. By specifying proxy_http_version 1.1 along with the Upgrade and Connection headers, you correctly enable WebSocket and SSE support. However, please verify that the SSR process is indeed listening on port 5173 in production. Note that the previous check using ss failed because the tool was not available. Use the above fallback script (or a similar method) to manually confirm that the SSR process is active on port 5173.

Also applies to: Lines 48-48


🏁 Script executed:

#!/bin/bash
# Verify SSR is running on port 5173 using lsof
lsof -i tcp:5173

Length of output: 113


Ensure SSR Port Verification for Nginx Proxy

The proxy configuration for SSR is correctly set up with proxy_http_version 1.1, Upgrade, and Connection headers enabling WebSocket and SSE. However, it's essential to confirm that the SSR process is listening on port 5173 in production. Attempts to automatically verify this using ss and lsof failed due to those tools not being available in the environment. Please perform a manual verification (or use alternative tools such as netstat if available) to ensure that your SSR process is indeed active on port 5173.

Also applies to: Lines 48-48

Copy link
Contributor

@kwon204 kwon204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just ask; pm2 start $SSR_DIR/index.js 여기에서 index.js를 어디서 가져오는 지 궁금합니다.

@hamo-o
Copy link
Contributor Author

hamo-o commented Mar 29, 2025

just ask; pm2 start $SSR_DIR/index.js 여기에서 index.js를 어디서 가져오는 지 궁금합니다.

아래 파일을 실행시키는 작업입니다.

image

여기서 전체 파일을 app으로 복사하는게 아닌가요?!!

image

@kwon204
Copy link
Contributor

kwon204 commented Mar 29, 2025

just ask; pm2 start $SSR_DIR/index.js 여기에서 index.js를 어디서 가져오는 지 궁금합니다.

아래 파일을 실행시키는 작업입니다.

image 여기서 전체 파일을 `app`으로 복사하는게 아닌가요?!! image
- name: Copy files to EC2
        uses: appleboy/scp-action@master
        with:
          host: ${{ secrets.EC2_HOST }}
          username: ${{ secrets.EC2_USERNAME }}
          key: ${{ secrets.EC2_SSH_KEY }}
          source: "frontend/dist/"
          target: "~/app"

여기서 복사하는 게 빌드 시에 생성되는 dist/** 파일들이라 frontend/server/index.js는 복사가 되고 있지 않습니다!
그래서 복사하려면 관련 코드를 .github/workflows/fe-cd.yml에 추가해야 할 거 같습니다.

@hamo-o
Copy link
Contributor Author

hamo-o commented Mar 29, 2025

just ask; pm2 start $SSR_DIR/index.js 여기에서 index.js를 어디서 가져오는 지 궁금합니다.

아래 파일을 실행시키는 작업입니다.
image
여기서 전체 파일을 app으로 복사하는게 아닌가요?!!
image

- name: Copy files to EC2
        uses: appleboy/scp-action@master
        with:
          host: ${{ secrets.EC2_HOST }}
          username: ${{ secrets.EC2_USERNAME }}
          key: ${{ secrets.EC2_SSH_KEY }}
          source: "frontend/dist/"
          target: "~/app"

여기서 복사하는 게 빌드 시에 생성되는 dist/** 파일들이라 frontend/server/index.js는 복사가 되고 있지 않습니다! 그래서 복사하려면 관련 코드를 .github/workflows/fe-cd.yml에 추가해야 할 거 같습니다.

아 그러네요 그부분을 수정하기보다는 제가 서버파일을 빌드해서 dist에 넣어주는 작업이 필요할 것 같습니다

@hamo-o hamo-o force-pushed the feature/all/ssr-script branch from 22725cf to 35f3c9b Compare April 6, 2025 22:59
Copy link

@coderabbitai coderabbitai bot left a 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

🔭 Outside diff range comments (1)
script/deploy/frontend_deploy.sh (1)

20-30: 💡 Verification agent

🧩 Analysis chain

Review server restart sequence.

The script restarts the SSR server before copying the new frontend files (line 35), which might cause the server to run with outdated files. Additionally, it's not clear how the SSR server files are being updated.


🏁 Script executed:

#!/bin/bash
# Check if the index.js file exists in the SSR_DIR directory
if [ -f "$SSR_DIR/index.js" ]; then
  echo "Found index.js in $SSR_DIR"
  echo "File contents:"
  head -n 20 "$SSR_DIR/index.js"
else
  echo "index.js not found in $SSR_DIR"
  echo "Directory contents:"
  ls -la "$SSR_DIR" || echo "Directory does not exist or is empty"
fi

# Check if the server build is available in the frontend/dist directory
if [ -d "$APP_DIR/frontend/dist/server" ]; then
  echo "Server build found in dist/server directory"
  echo "Directory contents:"
  ls -la "$APP_DIR/frontend/dist/server"
else
  echo "Server build not found in dist/server directory"
fi

Length of output: 409


Fix Deployment Sequence and Environment Variables for SSR Server

  • The verification script indicates that the expected SSR files (e.g., index.js in the SSR_DIR and the server build in frontend/dist/server) are missing. This reinforces the concern that the SSR server is being restarted before updated files are deployed.
  • Reorder the deployment steps so that the updated SSR/frontend files are copied or built first, ensuring that when the SSR server is restarted (using PM2), it runs the latest version.
  • Verify that environment variables (SSR_DIR and APP_DIR) are correctly defined so that the file paths resolve properly during deployment.
🧹 Nitpick comments (3)
frontend/server/index.js (1)

44-44: Consider adding error handling for server startup.

While the dynamic port configuration is good, the server startup lacks error handling. Adding a try-catch block around the server initialization would make the application more robust.

- app.listen(PORT);
- // eslint-disable-next-line no-console
- console.log(`\nServer running at \x1b[33mhttp://localhost:${PORT}\x1b[0m \n`);
+ app.listen(PORT, (err) => {
+   if (err) {
+     // eslint-disable-next-line no-console
+     console.error('Failed to start server:', err);
+     process.exit(1);
+   }
+   // eslint-disable-next-line no-console
+   console.log(`\nServer running at \x1b[33mhttp://localhost:${PORT}\x1b[0m \n`);
+ });

Also applies to: 46-46

frontend/package.json (1)

11-11: Consider separating preview from full build.

While the updated preview script works correctly, it always rebuilds the entire application before starting the server. Consider adding a separate script for just running the production server without rebuilding for faster iterations during testing.

"preview": "pnpm build && cross-env NODE_ENV=production node dist/server",
+ "serve:prod": "cross-env NODE_ENV=production node dist/server",
script/deploy/frontend_deploy.sh (1)

53-55: Consider parameterizing the backup retention policy.

The script currently keeps the 5 most recent backups. Consider making this configurable by adding a variable for the number of backups to retain.

-# 오래된 백업 정리 (최근 5개만 유지)
+# Define backup retention count
+BACKUP_RETENTION=5
+
+# 오래된 백업 정리 (최근 $BACKUP_RETENTION개만 유지)
 echo "오래된 프론트엔드 백업 정리 중..."
-cd $APP_DIR/frontend/backups && ls -t | tail -n +6 | xargs rm -f || true
+cd $APP_DIR/frontend/backups && ls -t | tail -n +$((BACKUP_RETENTION+1)) | xargs rm -f || true
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22725cfa74db8b5012994c361bf63804dc24b7ba and 35f3c9b.

⛔ Files ignored due to path filters (1)
  • frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • frontend/package.json (2 hunks)
  • frontend/server/index.js (3 hunks)
  • script/deploy/frontend_deploy.sh (3 hunks)
🔇 Additional comments (13)
frontend/server/index.js (5)

3-3: Good addition of the process import.

Using the node:process module allows proper access to environment variables needed for the new environment-aware server configuration.


9-10: Well-structured environment and port configuration.

Great job implementing environment detection and configurable port settings. Using environment variables with sensible defaults (PORT=5173) follows best practices for configurable server deployments.


13-13: Appropriate dynamic HTML path configuration.

The conditional path setting based on environment is a good approach for managing different file structures between development and production environments.


25-27: Good implementation of conditional static file serving.

Conditionally serving static files only in production mode is appropriate since this aligns with the updated build structure where client assets are in the '../client' directory in production.


32-32: Correct usage of dynamic HTML path.

This properly utilizes the environment-specific path determined earlier.

frontend/package.json (3)

9-9: Good addition of server build script.

The new server build script correctly configures Vite to build the SSR component with appropriate output directory.


10-10: Well-structured build script update.

The updated build script now properly chains client and server builds, ensuring both parts of the application are built during the process.


56-56: Good addition of cross-env dependency.

Adding cross-env ensures that environment variables are set consistently across different operating systems, which is important for cross-platform development.

script/deploy/frontend_deploy.sh (5)

6-6: Good improvement to SSR directory path.

Making the SSR directory path relative to APP_DIR rather than using a hardcoded path improves maintainability and flexibility.


9-10: Well-structured backup configuration.

The addition of backup directory and timestamp variables provides a solid foundation for the backup system.


12-16: Good implementation of the backup process.

Creating backups before deployment is an excellent practice that enables recovery in case of deployment failures.


35-35: Correct update to copy command for new build structure.

The updated path correctly references the client files from the new build structure.


48-48: Proper update to rollback command.

Using the BACKUP_DIR variable ensures consistency with the backup creation process.

@hamo-o hamo-o requested a review from kwon204 April 6, 2025 23:03
Copy link

@coderabbitai coderabbitai bot left a 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)
script/deploy/frontend_deploy.sh (1)

12-16: Frontend Backup Process:
The backup process is clearly implemented by creating the backup directory (if missing) and archiving the current frontend state using tar with elevated permissions:

mkdir -p $BACKUP_DIR
sudo tar -czf $BACKUP_DIR/frontend_$TIMESTAMP.tar.gz -C $FRONTEND_DIR .

This is a robust approach. One minor note: verify that using sudo here is necessary given the file/directory permissions in your environment.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35f3c9b and 00dd225.

📒 Files selected for processing (1)
  • script/deploy/frontend_deploy.sh (3 hunks)
🔇 Additional comments (4)
script/deploy/frontend_deploy.sh (4)

6-6: Dynamic SSR Directory Update:
Changing the SSR directory assignment from a hardcoded path to

SSR_DIR=$APP_DIR/frontend/dist/server

improves flexibility by tying the path to the environment variable APP_DIR. This ensures that the deployment will correctly reference the built server artifacts regardless of the absolute location.


9-10: Backup Directory and Timestamp Initialization:
Introducing the BACKUP_DIR variable and computing a dynamic TIMESTAMP using

BACKUP_DIR=$APP_DIR/frontend/backups
TIMESTAMP=$(date +"%Y%m%d%H%M%S")

allows for organized, timestamped backups of the frontend. This maximizes traceability and simplifies rollback management. Make sure that the $APP_DIR and backup paths are correctly set up in your deployment environment.


35-35: Updated File Copy Command:
Modifying the copy command to:

sudo cp -r $APP_DIR/frontend/dist/client/* $FRONTEND_DIR/

ensures that only the client artifacts are deployed to the web root. This addresses the prior issue of incorrect file path usage. Confirm that all needed static assets are present in the client folder after the build step.


47-48: Rollback Process Correction:
Updating the rollback extraction command to use the newly defined $BACKUP_DIR:

sudo tar -xzf $BACKUP_DIR/frontend_$TIMESTAMP.tar.gz -C $FRONTEND_DIR

creates a consistent backup-restore strategy. This change ensures that rollbacks are performed using the same backing-up logic.

Copy link
Contributor

@kwon204 kwon204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다

Copy link
Contributor

@dioo1461 dioo1461 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !~

@hamo-o hamo-o merged commit d87bccc into dev Apr 7, 2025
1 check passed
@hamo-o hamo-o deleted the feature/all/ssr-script branch April 7, 2025 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants