-
Notifications
You must be signed in to change notification settings - Fork 3k
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?
Changes from all 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,47 +1,8 @@ | ||
import type { AppUpdaterEvent } from "@types"; | ||
import { registerEvent } from "../register-event"; | ||
import updater, { UpdateInfo } from "electron-updater"; | ||
import { WindowManager } from "@main/services"; | ||
import { app } from "electron"; | ||
import { publishNotificationUpdateReadyToInstall } from "@main/services/notifications"; | ||
|
||
const { autoUpdater } = updater; | ||
|
||
const sendEvent = (event: AppUpdaterEvent) => { | ||
WindowManager.mainWindow?.webContents.send("autoUpdaterEvent", event); | ||
}; | ||
|
||
const sendEventsForDebug = false; | ||
|
||
const isAutoInstallAvailable = | ||
process.platform !== "darwin" && process.env.PORTABLE_EXECUTABLE_FILE == null; | ||
|
||
const mockValuesForDebug = () => { | ||
sendEvent({ type: "update-available", info: { version: "1.3.0" } }); | ||
sendEvent({ type: "update-downloaded" }); | ||
}; | ||
|
||
const newVersionInfo = { version: "" }; | ||
import { UpdateManager } from "@main/services/update-manager"; | ||
|
||
const checkForUpdates = async (_event: Electron.IpcMainInvokeEvent) => { | ||
autoUpdater | ||
.once("update-available", (info: UpdateInfo) => { | ||
sendEvent({ type: "update-available", info }); | ||
newVersionInfo.version = info.version; | ||
}) | ||
.once("update-downloaded", () => { | ||
sendEvent({ type: "update-downloaded" }); | ||
publishNotificationUpdateReadyToInstall(newVersionInfo.version); | ||
}); | ||
|
||
if (app.isPackaged) { | ||
autoUpdater.autoDownload = isAutoInstallAvailable; | ||
autoUpdater.checkForUpdates(); | ||
} else if (sendEventsForDebug) { | ||
mockValuesForDebug(); | ||
} | ||
|
||
return isAutoInstallAvailable; | ||
return UpdateManager.checkForUpdates(); | ||
}; | ||
|
||
registerEvent("checkForUpdates", checkForUpdates); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,21 @@ | ||
import fs from "node:fs"; | ||
import path from "node:path"; | ||
|
||
import { registerEvent } from "../register-event"; | ||
|
||
const checkFolderWritePermission = async ( | ||
_event: Electron.IpcMainInvokeEvent, | ||
path: string | ||
) => | ||
new Promise((resolve) => { | ||
fs.access(path, fs.constants.W_OK, (err) => { | ||
resolve(!err); | ||
}); | ||
}); | ||
testPath: string | ||
) => { | ||
const testFilePath = path.join(testPath, ".hydra-write-test"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
try { | ||
fs.writeFileSync(testFilePath, ""); | ||
fs.rmSync(testFilePath); | ||
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return true; | ||
} catch (err) { | ||
return false; | ||
} | ||
Comment on lines
+16
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}; | ||
|
||
registerEvent("checkFolderWritePermission", checkFolderWritePermission); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export const parseLaunchOptions = (params: string | null): string[] => { | ||
if (!params) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return []; | ||
} | ||
|
||
return params.split(" "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,23 +2,30 @@ import { gameRepository } from "@main/repository"; | |
|
||
import { registerEvent } from "../register-event"; | ||
import { shell } from "electron"; | ||
import { spawn } from "child_process"; | ||
import { parseExecutablePath } from "../helpers/parse-executable-path"; | ||
import { parseLaunchOptions } from "../helpers/parse-launch-options"; | ||
|
||
const openGame = async ( | ||
_event: Electron.IpcMainInvokeEvent, | ||
gameId: number, | ||
executablePath: string, | ||
launchOptions: string | null | ||
) => { | ||
// TODO: revisit this for launchOptions | ||
const parsedPath = parseExecutablePath(executablePath); | ||
const parsedParams = parseLaunchOptions(launchOptions); | ||
|
||
await gameRepository.update( | ||
{ id: gameId }, | ||
{ executablePath: parsedPath, launchOptions } | ||
); | ||
|
||
shell.openPath(parsedPath); | ||
if (parsedParams.length === 0) { | ||
shell.openPath(parsedPath); | ||
return; | ||
} | ||
|
||
spawn(parsedPath, parsedParams, { shell: false, detached: true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}; | ||
|
||
registerEvent("openGame", openGame); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,15 @@ import { registerEvent } from "../register-event"; | |
|
||
import type { UserPreferences } from "@types"; | ||
import i18next from "i18next"; | ||
import { patchUserProfile } from "../profile/update-profile"; | ||
|
||
const updateUserPreferences = async ( | ||
_event: Electron.IpcMainInvokeEvent, | ||
preferences: Partial<UserPreferences> | ||
) => { | ||
if (preferences.language) { | ||
i18next.changeLanguage(preferences.language); | ||
patchUserProfile({ language: preferences.language }).catch(() => {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
Comment on lines
12
to
15
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
return userPreferencesRepository.upsert( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ export const mergeAchievements = async ( | |
).filter((achievement) => achievement.name) as UnlockedAchievement[]; | ||
|
||
const newAchievementsMap = new Map( | ||
achievements.reverse().map((achievement) => { | ||
achievements.toReversed().map((achievement) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return [achievement.name.toUpperCase(), achievement]; | ||
}) | ||
); | ||
|
@@ -92,7 +92,7 @@ export const mergeAchievements = async ( | |
userPreferences?.achievementNotificationsEnabled | ||
) { | ||
const achievementsInfo = newAchievements | ||
.sort((a, b) => { | ||
.toSorted((a, b) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return a.unlockTime - b.unlockTime; | ||
}) | ||
.map((achievement) => { | ||
|
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