feat: redis app install lock#151
Conversation
WalkthroughAdds Redis-backed distributed install coordination and locking; introduces refresh-token auth flows and frontend support; adds CI/CD workflows and container/CI configs; updates environment schema and preview-mode guarding. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppModule.onModuleInit
participant Redis as Redis (ioredis)
participant Lock as InstallLock
participant Init as Initialization Steps
App->>Redis: GET INSTALL_FLAG
alt INSTALL_FLAG exists
Redis-->>App: flag present
App->>App: log and exit initialization
else not installed
App->>Lock: lock(300s)
Lock->>Redis: SET lock_key NX EX 300 (owner token)
alt lock acquired
Redis-->>Lock: OK
Lock-->>App: true
App->>Init: perform setup (i18n, languages, perms...)
Init-->>App: step results
App->>Lock: extend(300s)
Lock->>Redis: EVAL extend-lua (compare & expire)
Redis-->>Lock: extended
App->>Init: finalize (menus, admin user)
Init-->>App: success
App->>Redis: SET INSTALL_FLAG
Redis-->>App: ack
App->>Lock: release()
Lock->>Redis: EVAL release-lua (compare & del)
Redis-->>Lock: deleted
else lock held by other
Redis-->>Lock: nil (SET NX failed)
Lock-->>App: false
App->>Lock: wait(MAX_RETRY)
Lock->>Redis: poll exist()/GET lock_key repeatedly
alt lock cleared within retries
Lock-->>App: proceed or retry acquire
else timed out
Lock-->>App: timeout -> exit
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
template/nestJs/src/install-lock.ts (1)
12-26: EnsurelockValueis only set when the lock is actually acquired
lock()assignsthis.lockValuebefore knowing whetherSET NXsucceeded, so callers can observe a non‑nulllockValueeven when this instance never owned the lock. TodayAppModuleguards onlockStatus, but this makesrelease()/extend()easy to misuse from other call sites and risks targeting another owner’s lock.Refactor to compute a candidate value locally and only persist it on successful acquisition:
async lock(seconds: number = 300): Promise<boolean> { const redis = this.redisService.getRedis(); - // 锁值就是持有者标识 - this.lockValue = `${hostname()}:${process.pid}:${Date.now()}`; - - const result = await redis.set( - this.LOCK_KEY, - this.lockValue, + // 锁值就是持有者标识 + const value = `${hostname()}:${process.pid}:${Date.now()}`; + + const result = await redis.set( + this.LOCK_KEY, + value, 'EX', seconds, 'NX' ); - - return result === 'OK'; + if (result === 'OK') { + this.lockValue = value; + return true; + } + + this.lockValue = null; + return false; }template/nestJs/src/app.module.ts (2)
105-110: NormalizeisInstalledto a boolean for clarity
redis.exists(INSTALL_FLAG)returns a number, andisInstalledsimply forwards that, relying on JavaScript truthiness in call sites. For readability and type safety, it would be clearer to expose aPromise<boolean>:-async isInstalled(redis: Redis) { - return redis.exists(INSTALL_FLAG); -} +async isInstalled(redis: Redis): Promise<boolean> { + const count = await redis.exists(INSTALL_FLAG); + return count > 0; +}This keeps callers explicit and avoids accidentally depending on numeric semantics.
112-155: Consider making lock TTL and wait behavior configurableRight now the lock TTL is 5 minutes (extended once by another 5 minutes), while
MAX_RETRY = 20with a2000 * idelay gives roughly a 7‑minute maximum wait. That means a waiting node will give up and exit before the extended TTL is reached, even if the installer is still legitimately running.This may be acceptable for your environment, but if install time or Redis latency can vary, it would be safer to drive both TTL and MAX_RETRY/delay from configuration so operators can tune them without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
template/nestJs/src/app.module.ts(5 hunks)template/nestJs/src/install-lock.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
template/nestJs/src/app.module.ts (3)
template/nestJs/src/install-lock.ts (1)
lock(12-26)template/nestJs/src/menu/init/menuData.ts (1)
menuData(1-292)template/nestJs/src/application/init/data.ts (1)
applicationData(1-88)
⏰ 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: e2e-test
🔇 Additional comments (2)
template/nestJs/src/install-lock.ts (1)
28-104: Redis-based lock implementation looks solid overallThe
release(),exist(),wait(), andextend()methods correctly use Lua scripts andEX/NXsemantics to ensure that only the current owner can release or extend the lock, andwait()behaves as a bounded, increasing backoff waiter aroundexist(). Given the intended usage inAppModule, these operations form a coherent distributed lock primitive.template/nestJs/src/app.module.ts (1)
263-270: Guarantee Redis lock release beforeprocess.exitin the error pathIn the
catchblock you callprocess.exit(-1)without first releasing the Redis lock, relying on thefinallyblock to run. However,process.exit()terminates the process synchronously and does not reliably execute asynchronous code infinallyblocks—only synchronous 'exit' listeners are guaranteed. The lock key can remain set until TTL expiry, causing other nodes to see a stale lock and experience cascading failures.Explicitly release the lock before exiting:
} catch (e) { const err = e as HttpException; Logger.error(err.message); Logger.error(`Please clear the database and try again`); + await this.lock.release(); process.exit(-1); } finally { await this.lock.release(); }
* feat(nest.js): add preview mode flag * style: refine the preview mode warning tip * fix: use relative path
* ci: docker check * fix: restore ci
|
@GaoNeng-wWw 想了解下这个PR的作用是什么呢？主要是解决什么问题呢？ |
就是将安装标识放到了redis里，主要是想解决云原生不一定能挂载volume的问题。 分布式锁主要是为了避免同一时间多个节点同时开始初始化操作。 |
了解了，这个在分布式集群场景下应该有用，麻烦处理下冲突哈 |
…wWw/tiny-pro into feat/redis-storage-install-flag
|
/cmd docker --check |
🐳 Docker Check Status
Status: ⏳ Building... Preparing environment and running Docker health checks. |
✅ Docker Check Status
Status: SUCCESS! Service is built, running, and responded to health check on |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
template/tinyvue/src/utils/auth.ts (1)
21-23: Refresh token not cleared on logout.
clearToken()only removes the access token, leaving the refresh token insessionStorage. This could be a security concern if a stale refresh token remains after logout.Consider adding a
clearRefreshTokenfunction and updatingclearTokento clear both, or create aclearAllTokensfunction:const clearToken = () => { sessionStorage.removeItem(TOKEN_KEY); + sessionStorage.removeItem(REFRESH_TOKEN_KEY); };Or alternatively:
+const clearRefreshToken = () => { + sessionStorage.removeItem(REFRESH_TOKEN_KEY); +}; + const clearToken = () => { sessionStorage.removeItem(TOKEN_KEY); }; -export { isLogin, getToken, setToken, clearToken, getRefreshToken, setRefreshToken}; +export { isLogin, getToken, setToken, clearToken, getRefreshToken, setRefreshToken, clearRefreshToken };template/nestJs/src/auth/token.service.ts (1)
36-61: Recompute user token count after pruning expired entries to avoid over-revoking sessionsIn
issueToken,userTokenCountis captured before you callrevokeExpiredToken(uid). If there are many expired JTIs in the lists,revokeExpiredTokenwill clean them out, butuserTokenCountstays at the old (larger) value. Thewhile (userTokenCount >= limit)loop can then revoke more valid tokens than necessary, potentially logging out extra devices.You can fix this by recomputing the count after pruning:
async issueToken( uid: number, token: TokenData ){ const redis = this.redisService.getRedis(); - let userTokenCount = await this.getUserTokenCount(uid); + let userTokenCount = await this.getUserTokenCount(uid); const limit = Number.parseInt(this.cfg.get<string>('DEVICE_LIMIT')); if (limit > 0 && userTokenCount >= limit) { // 先尝试吊销所有过期的Token (无法通过jti找到token都被视作过期的token) - await this.revokeExpiredToken(uid); - while ( - userTokenCount >= limit - ) { + await this.revokeExpiredToken(uid); + userTokenCount = await this.getUserTokenCount(uid); + while (userTokenCount >= limit) { const lastToken = await this.getLastToken(uid); if (!lastToken) { break; } await this.revokeToken(lastToken.accessToken) userTokenCount -= 1; } }This keeps the “max devices” logic aligned with the actual number of live tokens after cleanup.
Also applies to: 148-168
🧹 Nitpick comments (7)
template/tinyvue/src/api/user.ts (1)
47-53: Consider renamingflushTokentorefreshTokenfor clarity.The term "flush" typically implies clearing/emptying, whereas this function refreshes/renews tokens. A name like
refreshTokenorrenewTokenwould better convey intent.Also, adding an explicit return type annotation improves readability:
-export function flushToken(data: RefreshToken){ - return axios.post<LoginResponse>(`${import.meta.env.VITE_BASE_API}/auth/token/refresh`, data) +export function refreshToken(data: RefreshToken): Promise<AxiosResponse<LoginResponse>> { + return axios.post<LoginResponse>(`${import.meta.env.VITE_BASE_API}/auth/token/refresh`, data); }template/nestJs/.env.example (1)
20-21: PREVIEW_MODE example is correct; consider key ordering only if you enforce dotenv-linter rulesAdding
PREVIEW_MODE=truehere matches the config/schema and guard behavior. dotenv-linter’s UnorderedKey warning is purely stylistic—if you want a clean lint run, you can movePREVIEW_MODEinto whatever ordering convention you follow; otherwise behavior is already correct.template/nestJs/src/auth/dto/refresh-token.dto.ts (1)
1-6: DTO shape looks good; optionally tighten validationThe DTO matches the frontend
RefreshTokeninterface and is sufficient. If you want stricter runtime validation, you could also enforce string type:-import { IsNotEmpty } from "class-validator"; +import { IsNotEmpty, IsString } from "class-validator"; export class RefreshToken { - @IsNotEmpty() + @IsString() + @IsNotEmpty() token: string; }template/nestJs/src/auth/token.service.ts (1)
185-192: Clarify return type forgetTokenByJti
redis.getwill typically resolve tostring | null; making that explicit improves readability and typing:- async getTokenByJti( - id: number, - jti: string, - type: 'at' | 'rt' - ) { - const redis = this.redisService.getRedis(); - return redis.get(`${type}:${id}:${jti}`); - } + async getTokenByJti( + id: number, + jti: string, + type: 'at' | 'rt' + ): Promise<string | null> { + const redis = this.redisService.getRedis(); + return redis.get(`${type}:${id}:${jti}`); + }template/nestJs/src/auth/auth.service.ts (1)
46-76: HardenrefreshTokenagainst invalid decode results and fix minor namingThe refresh flow (revoking old access/refresh tokens and issuing a fresh pair) is good, but there are two small robustness issues:
this.jwtService.decode(...)can theoretically yield a non-object (or null/undefined), in which case'refreshTokenJti' in tokenwill throw a TypeError instead of returning a clean 4xx.- Variable name
refresTokenObjectis a typo and hurts readability.You can address both with a small guard and rename:
- async refreshToken( - maybeToken: string - ){ - const token = this.jwtService.decode<AccessTokenPayload | RefreshTokenPayload>(maybeToken); - if ('refreshTokenJti' in token) { + async refreshToken(maybeToken: string) { + const token = this.jwtService.decode< + AccessTokenPayload | RefreshTokenPayload + >(maybeToken); + + if (!token || typeof token !== 'object') { + throw new HttpException( + this.i18n.translate('exception.common.tokenError'), + HttpStatus.BAD_REQUEST, + ); + } + + if ('refreshTokenJti' in token) { throw new HttpException( this.i18n.translate('exception.common.tokenError'), HttpStatus.BAD_REQUEST ) } - const refresTokenObject = token as RefreshTokenPayload; - const {id, jti, accessTokenJti, email} = refresTokenObject; + const refreshTokenObject = token as RefreshTokenPayload; + const { id, jti, accessTokenJti, email } = refreshTokenObject;This keeps the discriminator logic but avoids runtime errors on malformed tokens and slightly improves readability.
template/nestJs/src/auth/auth.controller.ts (1)
15-21: Refresh endpoint wiring looks good; consider minor path consistencyThe new public
POST /auth/token/refreshendpoint cleanly delegates toauthService.refreshToken(body.token)and matches the DTO shape; behavior-wise this is fine.For consistency with the other routes (
'login','logout','api-token'), you might prefer dropping the leading slash and use@Post('token/refresh'), but this is purely stylistic since Nest normalizes both.template/nestJs/src/app.module.ts (1)
106-111: NormalizeisInstalledto return a boolean instead of Redis’ numeric exists resultRight now
isInstalledreturns the raw result ofredis.exists, which is a number, and relies on truthiness at call sites. Converting that to an explicit boolean will make intent clearer and reduce surprises if Redis behavior changes.- async isInstalled(redis: Redis) { - return redis.exists(INSTALL_FLAG); - } - async setIsInstalled(redis: Redis){ + async isInstalled(redis: Redis): Promise<boolean> { + const exists = await redis.exists(INSTALL_FLAG); + return Boolean(exists); + } + + async setIsInstalled(redis: Redis) { return redis.set(INSTALL_FLAG, '1'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/deploy-obs.yml(1 hunks).github/workflows/docker-check.yaml(1 hunks).github/workflows/e2e-test.yml(1 hunks)template/nestJs/.env-ci(1 hunks)template/nestJs/.env.example(1 hunks)template/nestJs/docker-compose-ci.yml(1 hunks)template/nestJs/dockerfile(1 hunks)template/nestJs/endpoint.sh(0 hunks)template/nestJs/src/app.module.ts(5 hunks)template/nestJs/src/auth/auth.controller.ts(1 hunks)template/nestJs/src/auth/auth.service.ts(2 hunks)template/nestJs/src/auth/dto/refresh-token.dto.ts(1 hunks)template/nestJs/src/auth/token.service.ts(2 hunks)template/nestJs/src/config-schema.ts(2 hunks)template/nestJs/src/public/reject.guard.ts(1 hunks)template/tinyvue/src/api/user.ts(1 hunks)template/tinyvue/src/store/modules/user/index.ts(4 hunks)template/tinyvue/src/store/modules/user/types.ts(1 hunks)template/tinyvue/src/utils/auth.ts(3 hunks)
💤 Files with no reviewable changes (1)
- template/nestJs/endpoint.sh
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/e2e-test.yml
- template/nestJs/docker-compose-ci.yml
🧰 Additional context used
🧬 Code graph analysis (5)
template/nestJs/src/auth/dto/refresh-token.dto.ts (1)
template/tinyvue/src/api/user.ts (1)
RefreshToken(47-49)
template/nestJs/src/auth/auth.controller.ts (2)
template/nestJs/src/auth/dto/refresh-token.dto.ts (1)
RefreshToken(3-6)template/tinyvue/src/api/user.ts (1)
RefreshToken(47-49)
template/nestJs/src/public/reject.guard.ts (1)
template/nestJs/src/config-schema.ts (1)
Configure(3-23)
template/tinyvue/src/store/modules/user/index.ts (4)
template/tinyvue/src/utils/auth.ts (4)
getRefreshToken(25-25)getToken(25-25)setRefreshToken(25-25)setToken(25-25)template/nestJs/src/auth/auth.service.ts (2)
getToken(28-30)refreshToken(46-76)template/nestJs/src/auth/auth.controller.ts (1)
refreshToken(17-21)template/tinyvue/src/api/user.ts (1)
flushToken(51-53)
template/nestJs/src/app.module.ts (4)
template/nestJs/src/config-schema.ts (1)
Configure(3-23)template/nestJs/src/install-lock.ts (1)
lock(12-26)template/nestJs/src/menu/init/menuData.ts (1)
menuData(1-292)template/nestJs/src/application/init/data.ts (1)
applicationData(1-88)
🪛 actionlint (1.7.9)
.github/workflows/docker-check.yaml
26-26: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
56-56: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 dotenv-linter (4.0.0)
template/nestJs/.env.example
[warning] 21-21: [UnorderedKey] The PREVIEW_MODE key should go before the REDIS_HOST key
(UnorderedKey)
⏰ 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: e2e-test
🔇 Additional comments (8)
template/nestJs/dockerfile (1)
27-27: The "./" prefix improves path clarity without affecting functionality.Adding the explicit "./" prefix to the script reference in the CMD instruction makes the intent clearer—indicating the script runs from the current working directory (
/builder/). Since WORKDIR is set to/builder/, bothendpoint.shand./endpoint.shresolve identically, so this change is a stylistic improvement for readability.Ensure that
endpoint.shis present in the Docker build context and has executable permissions set.template/tinyvue/src/store/modules/user/index.ts (1)
96-138: LGTM on the login flow token handling.The login flow correctly extracts both
accessTokenandrefreshTokenfrom the response, updates the store state, and persists both tokens to storage. The error handling withclearToken()in the catch block is appropriate.template/nestJs/src/config-schema.ts (1)
22-45: PREVIEW_MODE config wiring looks consistent and type-safeAdding
PREVIEW_MODE: booleantoConfigureand the validation schema (with a sensible default) matches its usage in guards and env files; no issues from my side here.template/nestJs/.env-ci (1)
1-24: CI env template aligns with config schema and guard behaviorVariable names/shape match the Nest config schema, and setting
PREVIEW_MODE=falsefor CI is appropriate so the preview guard doesn’t interfere with health checks..github/workflows/deploy-obs.yml (1)
1-69: OBS deploy workflow looks coherent end-to-endThe build, version derivation, Vite env wiring, and obsutil upload steps form a consistent pipeline to
tiny-pro-vue/${VERSION}/vue-pro/pages/; I don’t see functional issues here, just ensure you validate once via a manualworkflow_dispatchrun after merge.template/nestJs/src/public/reject.guard.ts (1)
11-24: Preview gating inRejectRequestGuardis straightforward and correctInjecting
ConfigService<Configure>and short‑circuiting on!PREVIEW_MODEcleanly scopes this guard to preview mode only, leaving existing reflection/i18n behavior untouched.template/nestJs/src/auth/auth.service.ts (1)
36-44: Logout now correctly targets the current access token by JTIUsing
verifyto read{ id, jti }and then revoking the single matching access token viagetTokenByJti(id, jti, 'at')gives you precise session invalidation without touching other sessions. This looks good and aligns with the new token model.template/nestJs/src/app.module.ts (1)
112-278: Redis-backed install flag and distributed lock flow look coherentThe new startup sequence—Redis-backed
INSTALL_FLAG, pre-checks,InstallLock-based mutual exclusion, and clear error/exit semantics on timeouts or partial failures—forms a reasonable pessimistic locking scheme for clustered installs. The use ofwait+existto distinguish “other node succeeded” vs “other node crashed” is clear, and the finalfinally { await this.lock.release(); }covers the success path.
| - name: Checkout Repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.issue.pull_request.head.sha }} | ||
| repository: ${{ github.event.repository.full_name }} | ||
| - name: Post Initial Status | ||
| if: github.event_name == 'issue_comment' | ||
| uses: peter-evans/create-or-update-comment@v3 | ||
| with: | ||
| issue-number: ${{ github.event.issue.number }} | ||
| body: | | ||
| ## 🐳 Docker Check Status | ||
| > CI triggered by command `/cmd docker --check`. | ||
|
|
||
| **Status:** ⏳ **Building**... Preparing environment and running Docker health checks. | ||
| - name: Setup Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
| - name: Set up Docker Compose | ||
| uses: docker/setup-compose-action@v1 | ||
| - name: Run docker compose | ||
| working-directory: ./template/nestJs | ||
| run: docker compose -f docker-compose-ci.yml up -d | ||
|
|
||
| - name: Wait Service | ||
| working-directory: ./template/nestJs | ||
| run: | | ||
| timeout 60 bash -c ' | ||
| until [[ "$(docker compose -f docker-compose-ci.yml ps --services --filter "status=running" | wc -l)" -ge 1 ]]; do | ||
| sleep 2 | ||
| done | ||
| ' | ||
|
|
||
| - name: Send HealthCheck Request | ||
| run: curl -k --retry 20 --retry-delay 5 --retry-all-errors -f http://localhost:3000/healthCheck | ||
|
|
||
| - name: Update Status on Failure | ||
| if: failure() && github.event_name == 'issue_comment' | ||
| uses: peter-evans/create-or-update-comment@v3 | ||
| with: | ||
| issue-number: ${{ github.event.issue.number }} | ||
| body: | | ||
| ## ❌ Docker Check Status FAILED | ||
| > CI triggered by command `/cmd docker --check`. | ||
|
|
||
| **Status:** **FAILURE!** The service failed to start or did not pass the health check. | ||
|
|
||
| [查看完整的 CI 运行日志](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) | ||
| - name: Check service logs | ||
| if: failure() | ||
| working-directory: ./template/nestJs | ||
| run: docker compose -f docker-compose-ci.yml logs back | ||
| - name: Update Status on Success | ||
| if: success() && github.event_name == 'issue_comment' | ||
| uses: peter-evans/create-or-update-comment@v3 | ||
| with: | ||
| issue-number: ${{ github.event.issue.number }} | ||
| body: | | ||
| ## ✅ Docker Check Status | ||
| > CI triggered by command `/cmd docker --check`. | ||
|
|
||
| **Status:** **SUCCESS!** Service is built, running, and responded to health check on `http://localhost:3000/healthCheck`. | ||
| - name: Shutdown Service | ||
| working-directory: ./template/nestJs | ||
| run: docker compose -f docker-compose-ci.yml down |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions issue_comment event payload pull_request head sha availability
💡 Result:
Short answer: The issue_comment webhook/event payload does NOT include the pull request's head SHA. The event's issue object only has a pull_request link (if the comment is on a PR), so you must fetch the PR to get head.sha.
Why and how:
- Docs show issue_comment runs in the context of the default-branch commit (GITHUB_SHA is last commit on default branch) and that you can detect a PR comment with github.event.issue.pull_request. [1]
- Community issues confirm workflows cannot directly check out the PR head from issue_comment because the head SHA/branch is not in the payload. You need to call the API to retrieve the PR details (which include pull_request.head.sha). [2]
What to do (concise):
- In your workflow, detect a PR comment: if: ${{ github.event.issue.pull_request }}.
- Use the PR URL from github.event.issue.pull_request.url (or github.event.issue.number) and call the REST API to get the PR object and read .head.sha (via curl + GITHUB_TOKEN, actions/github-script, or octokit). Then checkout that SHA/branch if needed.
References:
[1] GitHub Actions — Events that trigger workflows (issue_comment details).
[2] actions/checkout issue discussing inability to checkout PR from issue_comment payload.
🌐 Web query:
peter-evans create-or-update-comment action latest version v4
💡 Result:
The latest released version is v5.0.0 (v4.0.0 is an earlier release). [1][2]
Sources:
- Releases (v5.0.0 and v4.0.0). [1]
- README showing usage examples with @v5. [2]
Fix PR ref checkout, always tear down containers, and upgrade comment action to v5
There are a few issues worth addressing:
- Incorrect ref on
actions/checkoutforissue_commentevents
The issue_comment event payload does not include pull_request.head.sha. The github.event.issue.pull_request field is a link object only, not the full PR details. To reliably check out the PR head, use the explicit PR ref pattern:
- - name: Checkout Repository
- uses: actions/checkout@v4
- with:
- ref: ${{ github.event.issue.pull_request.head.sha }}
- repository: ${{ github.event.repository.full_name }}
+ - name: Checkout Repository
+ uses: actions/checkout@v4
+ with:
+ repository: ${{ github.event.repository.full_name }}
+ ref: refs/pull/${{ github.event.issue.number }}/head- Containers are not torn down on failure
The Shutdown Service step runs only on success by default. If docker compose up or the health check fails, the stack is left running:
- name: Shutdown Service
+ if: always()
working-directory: ./template/nestJs
run: docker compose -f docker-compose-ci.yml down- Outdated
peter-evans/create-or-update-commentversion
The action is available at v5.0.0. Upgrade all three instances:
- uses: peter-evans/create-or-update-comment@v3
+ uses: peter-evans/create-or-update-comment@v5🧰 Tools
🪛 actionlint (1.7.9)
26-26: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
56-56: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: the runner of "peter-evans/create-or-update-comment@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/docker-check.yaml lines 19-82: fix three issues — update the
actions/checkout ref for issue_comment events to use the PR ref pattern
refs/pull/${{ github.event.issue.number }}/head (keep repository param), upgrade
all three peter-evans/create-or-update-comment usages to v5, and ensure
containers are always torn down by adding if: always() to the Shutdown Service
step (also consider making the "Check service logs" step run on failure() only
and keep that unchanged); apply these changes so the workflow reliably checks
out the PR head, uses the newer comment action, and always brings down the
compose stack.
| const IS_PREVIEW_MOD = this.cfg.get('PREVIEW_MODE'); | ||
| if (IS_PREVIEW_MOD) { | ||
| Logger.warn('You are currently in demonstration mode. All additions, deletions, and modifications request will be rejected'); | ||
| Logger.warn('If you want to disable the demo mode, please set the `PREVIEW_MODE` environment variable to `false`') | ||
| Logger.warn('Alternatively, you can create an `.env` file and set `PREVIEV_MODE` to `false`') | ||
| } | ||
| const LOCK_FILE = join(data, 'lock'); | ||
| if (existsSync(LOCK_FILE)) { | ||
| const redis = this.redis.getRedis(); | ||
| if (await this.isInstalled(redis)){ | ||
| Logger.warn( | ||
| 'Lock file exists, if you want init again, please remove dist or dist/lock' | ||
| `Lock file exists, if you want init again, please remove ${INSTALL_FLAG} in your redis` | ||
| ); |
There was a problem hiding this comment.
Fix PREVIEW_MODE env var typo and update wording to reflect Redis flag
Two small text issues that can confuse users:
- Log message suggests creating
.envwithPREVIEV_MODE, which is misspelled. - The “Lock file exists” warning now refers to a Redis key, not a file.
You can fix both without changing behavior:
- if (IS_PREVIEW_MOD) {
- Logger.warn('You are currently in demonstration mode. All additions, deletions, and modifications request will be rejected');
- Logger.warn('If you want to disable the demo mode, please set the `PREVIEW_MODE` environment variable to `false`')
- Logger.warn('Alternatively, you can create an `.env` file and set `PREVIEV_MODE` to `false`')
- }
+ if (IS_PREVIEW_MOD) {
+ Logger.warn(
+ 'You are currently in demonstration mode. All additions, deletions, and modifications request will be rejected',
+ );
+ Logger.warn(
+ 'If you want to disable the demo mode, please set the `PREVIEW_MODE` environment variable to `false`',
+ );
+ Logger.warn(
+ 'Alternatively, you can create an `.env` file and set `PREVIEW_MODE` to `false`',
+ );
+ }
@@
- if (await this.isInstalled(redis)){
- Logger.warn(
- `Lock file exists, if you want init again, please remove ${INSTALL_FLAG} in your redis`
- );
+ if (await this.isInstalled(redis)) {
+ Logger.warn(
+ `Install flag exists, if you want init again, please remove ${INSTALL_FLAG} in your Redis`,
+ );
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const IS_PREVIEW_MOD = this.cfg.get('PREVIEW_MODE'); | |
| if (IS_PREVIEW_MOD) { | |
| Logger.warn('You are currently in demonstration mode. All additions, deletions, and modifications request will be rejected'); | |
| Logger.warn('If you want to disable the demo mode, please set the `PREVIEW_MODE` environment variable to `false`') | |
| Logger.warn('Alternatively, you can create an `.env` file and set `PREVIEV_MODE` to `false`') | |
| } | |
| const LOCK_FILE = join(data, 'lock'); | |
| if (existsSync(LOCK_FILE)) { | |
| const redis = this.redis.getRedis(); | |
| if (await this.isInstalled(redis)){ | |
| Logger.warn( | |
| 'Lock file exists, if you want init again, please remove dist or dist/lock' | |
| `Lock file exists, if you want init again, please remove ${INSTALL_FLAG} in your redis` | |
| ); | |
| const IS_PREVIEW_MOD = this.cfg.get('PREVIEW_MODE'); | |
| if (IS_PREVIEW_MOD) { | |
| Logger.warn( | |
| 'You are currently in demonstration mode. All additions, deletions, and modifications request will be rejected', | |
| ); | |
| Logger.warn( | |
| 'If you want to disable the demo mode, please set the `PREVIEW_MODE` environment variable to `false`', | |
| ); | |
| Logger.warn( | |
| 'Alternatively, you can create an `.env` file and set `PREVIEW_MODE` to `false`', | |
| ); | |
| } | |
| const redis = this.redis.getRedis(); | |
| if (await this.isInstalled(redis)) { | |
| Logger.warn( | |
| `Install flag exists, if you want init again, please remove ${INSTALL_FLAG} in your Redis`, | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In template/nestJs/src/app.module.ts around lines 113 to 123, there are two text
issues: fix the typo `PREVIEV_MODE` to `PREVIEW_MODE` in the `.env` instruction,
and update the second warning to refer to a Redis key rather than a lock file
(e.g., change "Lock file exists" wording to "Lock flag exists in Redis" or
similar and mention removing the INSTALL_FLAG key from Redis); make these
wording changes only, preserving existing behavior and log levels.
| refreshToken: getRefreshToken(), | ||
| accessToken: getToken() |
There was a problem hiding this comment.
Type assertion may hide null values.
getRefreshToken() and getToken() return string | null, but the state type expects string. This could lead to runtime issues when accessing these values if the user isn't logged in.
Either update the type in types.ts to allow null, or provide default empty strings:
- refreshToken: getRefreshToken(),
- accessToken: getToken()
+ refreshToken: getRefreshToken() ?? '',
+ accessToken: getToken() ?? '',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refreshToken: getRefreshToken(), | |
| accessToken: getToken() | |
| refreshToken: getRefreshToken() ?? '', | |
| accessToken: getToken() ?? '' |
| flushToken(){ | ||
| const refreshToken = this.refreshToken as string; | ||
| flushToken({token: refreshToken}) | ||
| .then((tokenPair) => { | ||
| return tokenPair.data | ||
| }) | ||
| .then((data) => { | ||
| this.refreshToken = data.refreshToken; | ||
| this.accessToken = data.accessToken; | ||
| setRefreshToken(data.refreshToken) | ||
| setToken(data.accessToken); | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Missing error handling and naming collision in flushToken action.
Several issues with this action:
-
No error handling: If the refresh request fails (e.g., expired refresh token, network error), the error is silently swallowed and tokens aren't cleared, potentially leaving the app in an inconsistent state.
-
Naming collision: The action
flushTokenshadows the imported API functionflushToken, making the code confusing. -
Inconsistent pattern: Other actions use
async/await, but this uses.then()chaining.
- flushToken(){
- const refreshToken = this.refreshToken as string;
- flushToken({token: refreshToken})
- .then((tokenPair) => {
- return tokenPair.data
- })
- .then((data) => {
- this.refreshToken = data.refreshToken;
- this.accessToken = data.accessToken;
- setRefreshToken(data.refreshToken)
- setToken(data.accessToken);
- })
+ async refreshTokens() {
+ try {
+ const refreshToken = this.refreshToken;
+ if (!refreshToken) {
+ throw new Error('No refresh token available');
+ }
+ const res = await flushToken({ token: refreshToken });
+ const { accessToken, refreshToken: newRefreshToken } = res.data;
+ this.refreshToken = newRefreshToken;
+ this.accessToken = accessToken;
+ setRefreshToken(newRefreshToken);
+ setToken(accessToken);
+ } catch (err) {
+ clearToken();
+ this.resetInfo();
+ throw err;
+ }
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In template/tinyvue/src/store/modules/user/index.ts around lines 69 to 81, the
flushToken action shadows the imported API function, lacks error handling, and
uses .then() while other actions use async/await; rename the action (e.g.,
refreshTokenAction or refreshTokens) or rename the imported API (e.g.,
apiFlushToken) to avoid collision, convert the action to async/await, wrap the
API call in try/catch, on success update this.refreshToken, this.accessToken and
call setRefreshToken/setToken, and on error clear in-memory tokens and
persistent storage (setRefreshToken('')/setToken('') or use dedicated remove
functions) and rethrow or return a failure indicator so callers can handle it.
| reset?: boolean; | ||
| } | ||
| export type UserState = UserInfo & UserFilterData; | ||
| export type UserState = UserInfo & UserFilterData & {refreshToken: string; accessToken: string;}; |
There was a problem hiding this comment.
Type mismatch: tokens can be null when not logged in.
getRefreshToken() and getToken() return string | null from sessionStorage.getItem(). The current type definition requires string, which will cause a type error when assigning null values during initialization.
-export type UserState = UserInfo & UserFilterData & {refreshToken: string; accessToken: string;};
+export type UserState = UserInfo & UserFilterData & {refreshToken: string | null; accessToken: string | null;};🤖 Prompt for AI Agents
In template/tinyvue/src/store/modules/user/types.ts around line 41, the
UserState type currently requires refreshToken and accessToken to be string but
sessionStorage.getItem() can return null; change those properties to allow
string | null (e.g., refreshToken: string | null; accessToken: string | null)
and update any initial state or assignments to accept null values so TypeScript
errors are resolved.
2 participants
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
不在本地存储flag而是使用redis, 避免了云容器无法持久化volume导致的宕机后难以启动. 该PR实现了悲观锁. 同一时刻只能有一个节点持有锁, 其余节点必须自旋等待.
Does this PR introduce a breaking change?
对于存量已经初始化过的项目, 需要在redis中手动执行
SET FLAG:INSTALL 1Other information
Summary by CodeRabbit
New Features
Chores
DevOps
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.