Feat/add dolphin file system extension - #412
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
| const destinationExists = await doesFileExist(destination); | ||
| if (destinationExists) { | ||
| if (asset.executable) { | ||
| await fs.chmod(destination, 0o755); |
There was a problem hiding this comment.
The permissions granted to this script are strictly those necessary for the extension's features to function properly without compromising the security of the user's device.
There was a problem hiding this comment.
but why does it need to be readable and executable by the group and all other users? These are the kind of details that I would like you to explain the why behind the decission
There was a problem hiding this comment.
The 0o755 permission level poses no security risk and is precisely what is required here.
In Linux octal notation, 0o755 breaks down as:
-
Owner (7 -> rwx): Full read, write, and execute permissions. Only the file owner can modify it.
-
Group (5 -> r-x): Read and execute permissions.
-
Others (5 -> r-x): Read and execute permissions.
We grant read/execute access to group and other users because the file manager—as well as other GUI background processes—needs to invoke and read this extension across different contexts. Restricting these permissions will prevent the extension from loading or rendering in context menus.
Security impact is minimal since write permissions are strictly withheld from non-owner users; no third party can edit, overwrite, or swap the file. Additionally, the file handles no sensitive data and solely serves as an entry point for the file manager to run the integration upon invocation.
|
|
||
| await fs.cp(source, destination); | ||
| if (asset.executable) { | ||
| await fs.chmod(destination, 0o755); |
| X-KDE-ServiceTypes=KonqPopupMenu/Plugin | ||
| Actions=InternxtCopyLink; | ||
| X-KDE-Submenu=Internxt Drive | ||
| X-KDE-Priority=TopLevel |
There was a problem hiding this comment.
For Dolphin, new options must be configured using a new .desktop file that directly modifies the system settings.
| print("") | ||
| sys.exit(0) | ||
|
|
||
| relative_path = file_path[len(root_folder):] |
There was a problem hiding this comment.
The functionality to generate the share link is still present in the app's code and can be accessed through the hydration API. This made it easy for us to create this script, which calls the corresponding endpoint and saves the link to the clipboard.
Since the extension's settings are managed directly through the system settings, it was necessary for the actions to be executed through this script.
| }, | ||
| "deb": { | ||
| "depends": [ | ||
| "recommends": [ |
There was a problem hiding this comment.
The installation of this add-on has been changed to “recommended” because in distributions such as Kubuntu that use KDE, this library is not available by default and prevents the application from being installed.
…hin-file-system-extension
…or file system extension (#421)
| fileManager: [nautilus, nemo, dolphin] | ||
|
|
||
| container: | ||
| image: ubuntu:24.04 |
There was a problem hiding this comment.
we could use either the newest lts or the oldest active, what do you think? (22.04)
| const execAsync = promisify(exec); | ||
|
|
||
| export type FileManagerType = 'nautilus' | 'nemo' | null; | ||
| export type FileManagerType = 'nautilus' | 'nemo' | 'dolphin' | null; |
There was a problem hiding this comment.
isnt SupportedFileManager basically this?
| { type: 'nautilus', desktopEntry: 'nautilus.desktop', hasBinary: hasNautilusBinary }, | ||
| ]; | ||
|
|
||
| export async function detectAvailableFileManager(): Promise<FileManagerType> { |
There was a problem hiding this comment.
You are making FileManagerType to have null just because this function is returning null and then
type FileManagerCandidate = { type: Exclude<FileManagerType, null>;
Why not rather make this function return FileManagerType | null?
| export async function isDolphinAvailable(): Promise<boolean> { | ||
| return (await detectAvailableFileManager()) === 'dolphin'; | ||
| } | ||
|
|
There was a problem hiding this comment.
What is this used for? I see it has no usage
There was a problem hiding this comment.
I had added them to generate alerts like the one generated by Nautilus, but now that I think about it, the alert should say that there is no extension available.
| return (await detectAvailableFileManager()) === 'dolphin'; | ||
| } | ||
|
|
||
| async function getDefaultDirectoryDesktopEntry(): Promise<string> { |
There was a problem hiding this comment.
why not make this method directly pass the type of desktop rather than a string??
| const nautilusExtensionFileName = 'internxt-virtual-drive.py'; | ||
| const nemoExtensionFileName = 'internxt-virtual-drive.py'; | ||
| const dolphinMenuFileName = 'internxt-virtual-drive.desktop'; | ||
| const dolphinHelperFileName = 'internxt-dolphin-actions.sh'; |
There was a problem hiding this comment.
This should be ina consttants files
| if (destinationExists) { | ||
| if (asset.executable) { | ||
| await fs.chmod(destination, 0o755); | ||
| } |
There was a problem hiding this comment.
Check the sonar issues because both sonar and gh are screaming
There was a problem hiding this comment.
The permissions granted to this script are strictly those necessary for the extension's features to function properly without compromising the security of the user's device.
There was a problem hiding this comment.
I have already read your reply to the gh issue, there is no need to copy paste it 🙂
| await fs.cp(source, destination); | ||
| if (asset.executable) { | ||
| await fs.chmod(destination, 0o755); | ||
| } |
…d clarity and functionality
| if (!fileManager) { | ||
| logger.debug({ | ||
| msg: '[FILE_MANAGER_EXTENSION] No compatible file manager found (Nautilus or Nemo)', | ||
| msg: '[FILE_MANAGER_EXTENSION] No compatible file manager found (Nautilus, Nemo or Dolphin)', |
There was a problem hiding this comment.
Question: Shouldnt we use a banner to tell the user this info? and perhaps an issue? or even a native notification, this way the user exactly knows what is happening when the error occurs
| type: 'dolphin', | ||
| reloadCommand: 'kquitapp6 dolphin || kquitapp5 dolphin || true', | ||
| assets: [ | ||
| { | ||
| source: 'dolphin/internxt-virtual-drive.desktop', | ||
| destination: `${homedir}/.local/share/kio/servicemenus/${DOLPHIN_MENU_FILENAME}`, | ||
| template: true, | ||
| executable: true, | ||
| }, | ||
| { | ||
| source: 'dolphin/internxt-virtual-drive.desktop', | ||
| destination: `${homedir}/.local/share/kservices5/ServiceMenus/${DOLPHIN_MENU_FILENAME}`, | ||
| template: true, | ||
| executable: true, | ||
| }, | ||
| { | ||
| source: `dolphin/${DOLPHIN_HELPER_FILENAME}`, | ||
| destination: `${homedir}/.local/share/internxt-dolphin-extension/${DOLPHIN_HELPER_FILENAME}`, | ||
| executable: true, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
perhaps this kind of information could go into the paths file no? what do you think?
| function getExtensionFile(source: string): string { | ||
| if (process.env.NODE_ENV === 'development') { | ||
| return path.join(__dirname, `../../../../assets/${assetDir}`, extensionFileName); | ||
| return path.join(__dirname, `../../../../assets/${source}`); | ||
| } else { | ||
| return path.join(process.resourcesPath, 'assets', assetDir, extensionFileName); | ||
| return path.join(process.resourcesPath, 'assets', source); | ||
| } |
| } | ||
|
|
||
| if (name === 'temp') { | ||
| return process.env.TMPDIR || '/tmp'; |
|
be7f20e
into
feat/add-nemo-file-system-extension




Summary
This PR adds Dolphin file manager integration to Internxt Drive on Linux.
What Changed
Notes
Validation
npm run package.