feat: sync log #93
feat: sync log #93
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/Sync.tsx (2)
53-55: Standardize the order in equality comparisonsIn the condition
if (LogStatus.WAIT !== logState), consider placing the variablelogStateon the left side for consistency and readability, i.e.,if (logState !== LogStatus.WAIT). This is a common convention that enhances code clarity.Apply this change:
-if (LogStatus.WAIT !== logState) { +if (logState !== LogStatus.WAIT) { setLogState(LogStatus.WAIT); }
19-24: Consider implementing internationalization (i18n) for status messagesThe
LogStatusTextMapuses Chinese text for status messages. If the application is intended for a broader audience or may support multiple languages in the future, consider integrating internationalization to handle translations dynamically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Sync.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/components/Sync.tsx (1)
88-94: EnsurelogIdis defined before using non-null assertionIn the
hrefattribute of theButton, you're usinglogId!with a non-null assertion. WhilelogState === LogStatus.SUCCESSimplies thatlogIdshould be defined, it's safer to ensure thatlogIdis indeed defined to prevent potential runtime errors. Consider adding a conditional check or adjusting the logic to guaranteelogIdis defined when needed.Run the following script to verify that
logIdis always defined whenlogStateisLogStatus.SUCCESS:
| href={logState === LogStatus.SUCCESS ? genLogFileUrl(logId!) : undefined} | ||
| target='_blank' | ||
| onClick={() => { | ||
| if (!logId) { | ||
| doSync(); | ||
| return; | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the use of both href and onClick on the Button component
Using both href and onClick on the Button can lead to unexpected behaviors. In Ant Design, when href is provided, the button renders as an anchor tag. Since you're also using onClick, it's important to ensure that the combination doesn't cause any issues. Consider separating the concerns:
- When
logState === LogStatus.SUCCESS, render the button as a link. - Otherwise, use the
onClickhandler for initiating the sync.
Here's a possible refactor:
<Button
size="small"
type="primary"
loading={logState === LogStatus.WAIT}
{...(logState === LogStatus.SUCCESS
? {
href: genLogFileUrl(logId!),
target: '_blank',
}
: {
onClick: () => {
if (!logId) {
doSync();
}
},
})}
>
{LogStatusTextMap[logState]}
</Button>| if (retryCountRef.current > MAX_RETRY) { | ||
| setLogState(LogStatus.ERROR); | ||
| return; | ||
| } else { | ||
| if (LogStatus.WAIT !== logState) { | ||
| setLogState(LogStatus.WAIT); | ||
| } | ||
| setTimeout(() => { | ||
| logPolling(id); | ||
| }, 1000); |
There was a problem hiding this comment.
Reset retryCountRef.current when initiating a new sync task
Currently, retryCountRef.current is not reset when starting a new sync task. This could lead to incorrect retry counts if the user initiates multiple syncs without refreshing the page, potentially causing the polling to stop prematurely. Please reset retryCountRef.current to 0 at the start of the doSync function to ensure accurate retry counts for each sync operation.
Apply this diff to fix the issue:
async function doSync() {
+ retryCountRef.current = 0;
try {
const response = await fetch(`${SYNC_REGISTRY}/-/package/${pkgName}/syncs`, {
method: 'PUT',
})Committable suggestion was skipped due to low confidence.
“查看日志”时文字看看不见了，需要hover @elrrrrrrr |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor