Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: changing permission verify strategy #1374
base: main
Are you sure you want to change the base?
feat: changing permission verify strategy #1374
Changes from 25 commits
31feb4a
1cfe3dc
1f42ebd
71decd9
99364df
0adcc73
74d4975
9bb89a1
47f7731
715c4a6
92e641e
097aff1
f682c56
fb66557
40552cb
540fd80
5ac0c04
1d8a3c4
1b4f962
abf9d9b
3f8a440
33e1de7
16a3b1e
8ef2377
e4fefde
8f1acb4
5889f62
04db9a7
64b10a0
ce223a4
6698d2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
style: function is marked async but contains no await operations - should either use async fs operations or remove async keyword
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.
style: consider using a more unique filename with timestamp/uuid to avoid potential conflicts
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.
style: using synchronous fs operations can block the main thread - consider using async writeFile/rm
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.
style: error is silently swallowed - should at least log it for debugging purposes
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.
style: consider also checking for empty string case here to avoid unnecessary split operation
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.
logic: simple string split() will break arguments containing spaces (e.g. "C:\Program Files\Game.exe" or "-name "Player Name""). Consider using a more robust argument parser that handles quotes and escapes
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.
logic: multiple consecutive spaces will create empty strings in the output array. Should filter these out with .filter(Boolean) or similar
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.
logic: No error handling for spawn failures. Should wrap in try/catch and handle potential errors.
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.
logic: Empty catch block silently swallows errors from profile update. Should at least log the error for debugging.
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.
style: Local language change happens before profile update. If profile update fails, local and remote state will be out of sync.
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.
style: toReversed() is an ES2023 feature - consider using [...achievements].reverse() for better compatibility
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.
style: toSorted() is an ES2023 feature - consider using [...newAchievements].sort() for better compatibility
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.
logic: JSON.parse may throw if config.data is undefined or invalid JSON, even with null coalescing. Consider wrapping in try/catch
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.
style: handleUnauthorizedError parameter 'err' should be typed as 'unknown' for better type safety
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.
style: Inconsistent operator usage - line 9 uses === but line 13 uses ==. Should standardize on ===