Feat/folder delete confirmation #950
Conversation
…SSIE-Org#946 - Add PKGBUILD for binary package installation from releases - Add PKGBUILD-git for building from source - Add pictopy.install for post-install hooks - Add .SRCINFO metadata file - Add publish-aur.yml workflow for automatic AUR publishing - Update build-and-release.yml to generate tar.gz for AUR - Update tauri.conf.json to build all Linux targets (deb, rpm, appimage) - Update README.md with AUR installation instructions This enables Arch Linux users to install PictoPy via: yay -S pictopy paru -S pictopy Closes AOSSIE-Org#946
- Add confirmation dialog with warning message before deleting a folder - Display folder path in the confirmation dialog - Show clear warning that the action is irreversible - Add Cancel and Delete buttons with proper styling - Disable buttons during deletion to prevent double-clicks Fixes AOSSIE-Org#939
github-actions
bot
added
enhancement
📝 WalkthroughWalkthroughThis PR introduces Arch Linux AUR packaging support via new GitHub Actions workflows, adds AUR package metadata and build scripts, implements a delete confirmation dialog for folder removal in the frontend, and updates Tauri bundler configuration for Linux distributions. Changes
Sequence Diagram(s)sequenceDiagram
actor GH as GitHub<br/>Release Event
participant WF as publish-aur.yml<br/>Workflow
participant Assets as Release<br/>Assets
participant Build as Checksum<br/>Computation
participant AUR as AUR<br/>Repository
GH->>WF: Trigger on release published
WF->>WF: Extract version from tag
WF->>Assets: Wait for release assets
Assets-->>WF: Assets available
WF->>Assets: Download amd64 tarball
Assets-->>WF: Tarball data
WF->>Build: Compute SHA256
Build-->>WF: x86_64 checksum (or SKIP)
WF->>Assets: Download arm64 tarball
Assets-->>WF: Tarball data
WF->>Build: Compute SHA256
Build-->>WF: aarch64 checksum (or SKIP)
WF->>WF: Update aur/PKGBUILD<br/>(version, sources, checksums)
WF->>WF: Generate aur/.SRCINFO<br/>(metadata + checksums)
WF->>AUR: Deploy via github-actions-deploy-aur<br/>(credentials, commit details)
AUR-->>WF: Publish complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 5
Fix all issues with AI Agents 🤖
In @.github/workflows/build-and-release.yml:
- Around line 384-389: Replace the brittle "sleep 30" + two chained curl
commands with a retry loop that polls for the release asset and fails fast if
unavailable: implement a loop that attempts to curl the release URL(s) (the two
curl -fLo ... pictopy.deb lines) up to N times with a short sleep between
attempts, verify the downloaded file exists and has non‑zero size after each
attempt, and exit with a non‑zero status (failing the job) if all retries fail;
ensure the workflow logs the URL and curl error on each failure to aid
debugging.
In @.github/workflows/publish-aur.yml:
- Around line 33-36: The "Wait for release assets" step currently uses a brittle
fixed sleep ("sleep 60"); replace it with retry logic that polls for asset
availability with exponential backoff and a max attempt limit. Update the "Wait
for release assets" step to repeatedly check the release assets (e.g., via
GitHub API or gh release view for the target tag) and on success exit the loop,
otherwise sleep for an increasing interval (e.g., 2^n seconds) between attempts
and fail the job after the max attempts; ensure the step logs each attempt and
the final success/failure.
- Around line 46-60: The workflow currently substitutes "SKIP" when curl fails,
allowing publishing without checksum verification; update the download blocks so
that if the curl for the required x86_64 tarball fails (the branch that would
set SHA256_X86_64) the job fails immediately (e.g., log the error and exit
non‑zero) instead of writing "sha256_x86_64=SKIP" to $GITHUB_OUTPUT; for the
aarch64 block decide whether it is optional — if optional keep the current
behavior but emit a warning and skip publishing artifacts for that arch,
otherwise fail similarly on download error (affecting SHA256_AARCH64 and its
curl branch) so no publish proceeds without valid sha256 outputs.
In @aur/.SRCINFO:
- Around line 39-42: Replace the placeholder "SKIP" values for sha256sums_x86_64
and sha256sums_aarch64 in .SRCINFO with the real SHA256 checksums for the
corresponding source_x86_64 and source_aarch64 release artifacts; update the AUR
publishing workflow to download each release asset, compute its SHA256 digest,
write those digests into the PKGBUILD (or .SRCINFO) and then regenerate .SRCINFO
(e.g., via makepkg --printsrcinfo) before publishing so the published .SRCINFO
contains the actual checksums instead of SKIP.
In @aur/PKGBUILD-git:
- Around line 57-95: The build() function runs multiple external commands (pip
install, pyinstaller, npm install, cargo build, etc.) without error handling, so
failures are silently ignored; update the top of build() to enable strict shell
error handling (e.g., set -euo pipefail and a trap to log the failing
command/exit code) so any failing command inside build()
(backend/sync-microservice/frontend steps, pyinstaller, npm install, cargo
build) aborts the script immediately and prints a useful error message; ensure
activation/deactivation of venvs and cd operations also propagate failures so
the build cannot continue after an error.
🧹 Nitpick comments (5)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
75-77: Consider using a stable key for the folders list.While this is existing code, the new deletion feature makes this more relevant: using
indexas a key can cause React reconciliation issues when folders are deleted. Usefolder.folder_idfor a stable, unique identifier.🔎 Suggested improvement for React key prop
- {folders.map((folder: FolderDetails, index: number) => ( + {folders.map((folder: FolderDetails) => ( <div - key={index} + key={folder.folder_id} className="group border-border bg-background/50 relative rounded-lg border p-4 transition-all hover:border-gray-300 hover:shadow-sm dark:hover:border-gray-600" >aur/PKGBUILD-git (1)
104-113: Useinstallcommand for better control over file permissions.Using
cp -rfollowed bychmod -R 755gives execute permissions to all files, including data files that shouldn't be executable. This could be a security concern.🔎 Recommended approach using install command
While the current approach works for a bundled application, consider using more granular permissions:
# Install backend resources install -dm755 "${pkgdir}/usr/lib/pictopy/resources/backend" - cp -r backend/dist/PictoPy_Server/* "${pkgdir}/usr/lib/pictopy/resources/backend/" + find backend/dist/PictoPy_Server -type f -executable -exec install -Dm755 {} "${pkgdir}/usr/lib/pictopy/resources/backend/{}" \; + find backend/dist/PictoPy_Server -type f ! -executable -exec install -Dm644 {} "${pkgdir}/usr/lib/pictopy/resources/backend/{}" \; # Install sync microservice resources install -dm755 "${pkgdir}/usr/lib/pictopy/resources/sync-microservice" - cp -r sync-microservice/dist/PictoPy_Sync/* "${pkgdir}/usr/lib/pictopy/resources/sync-microservice/" - - # Set permissions for resources - chmod -R 755 "${pkgdir}/usr/lib/pictopy/resources" + find sync-microservice/dist/PictoPy_Sync -type f -executable -exec install -Dm755 {} "${pkgdir}/usr/lib/pictopy/resources/sync-microservice/{}" \; + find sync-microservice/dist/PictoPy_Sync -type f ! -executable -exec install -Dm644 {} "${pkgdir}/usr/lib/pictopy/resources/sync-microservice/{}" \;However, if the current approach works and PyInstaller bundles are meant to have those permissions, this can remain as-is.
.github/workflows/build-and-release.yml (1)
407-415: Updatesoftprops/action-gh-releaseto a newer version.The static analysis tool flagged that
softprops/action-gh-release@v1is outdated. Update tov2for improved compatibility and features.🔎 Proposed fix
- name: Upload tar.gz to release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with: tag_name: ${{ steps.get_version.outputs.tag }}aur/PKGBUILD (1)
57-60: Usecp -ato preserve file permissions for libraries.When copying library files, using
cp -a(archive mode) preserves permissions, ownership, and timestamps, which is important for proper library functionality.🔎 Proposed fix
# Install libraries and resources if [ -d "usr/lib" ]; then - cp -r usr/lib "${pkgdir}/usr/" + cp -a usr/lib "${pkgdir}/usr/" fi.github/workflows/publish-aur.yml (1)
97-146: Consider using<<-EOFfor cleaner heredoc handling.The current approach uses a heredoc with spaces, then removes them with
sed. This is fragile if the indentation changes. Using<<-EOFwith tabs would automatically strip leading tabs.Alternatively, the current approach works but ensure the sed pattern stays synchronized with the heredoc indentation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build-and-release.yml.github/workflows/publish-aur.ymlREADME.mdaur/.SRCINFOaur/PKGBUILDaur/PKGBUILD-gitaur/README.mdaur/pictopy.installfrontend/src-tauri/tauri.conf.jsonfrontend/src/pages/SettingsPage/components/FolderManagementCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (1)
frontend/src/components/ui/dialog.tsx (6)
Dialog(133-133)DialogContent(135-135)DialogHeader(138-138)DialogTitle(141-141)DialogDescription(136-136)DialogFooter(137-137)
🪛 actionlint (1.7.9)
.github/workflows/build-and-release.yml
408-408: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (19)
frontend/src/pages/SettingsPage/components/FolderManagementCard.tsx (5)
1-14: LGTM! Imports are correctly added for the confirmation dialog feature.All new imports (
useState,AlertTriangle, and Dialog components) are properly utilized in the implementation.
40-44: LGTM! State management for the confirmation dialog is properly initialized.The state variables are correctly typed and initialized with appropriate default values.
46-65: LGTM with a suggestion to handle dialog closure edge case.The event handlers correctly implement the confirmation flow with proper state management and cleanup. However, consider preventing the dialog from being closed via overlay click or ESC key while deletion is in progress.
The
Dialogcomponent acceptsonOpenChangewhich can be triggered by overlay clicks or ESC key. While buttons are disabled duringdeleteFolderPending, users might still close the dialog through these alternative methods. Consider adding a check:🔎 Suggested enhancement to prevent premature dialog closure
- <Dialog open={isDeleteDialogOpen} onOpenChange={setIsDeleteDialogOpen}> + <Dialog + open={isDeleteDialogOpen} + onOpenChange={(open) => { + // Prevent closing dialog during deletion + if (!deleteFolderPending) { + setIsDeleteDialogOpen(open); + if (!open) { + setFolderToDelete(null); + } + } + }} + >
105-113: LGTM! Delete button correctly triggers the confirmation dialog.The button now opens the confirmation dialog instead of immediately deleting, which addresses the PR objective of preventing accidental deletions. The disabled state during deletion is properly maintained.
170-209: Excellent implementation of the delete confirmation dialog!The confirmation dialog thoroughly addresses all PR objectives:
- ✓ Displays folder path for user verification (line 183)
- ✓ Clear warning about irreversibility (lines 185-188)
- ✓ Warning icon with appropriate red styling (lines 175-177)
- ✓ Well-styled Cancel and Delete buttons (lines 192-207)
- ✓ Buttons disabled during deletion with loading state (lines 195, 202, 205)
The implementation follows accessibility best practices with proper Dialog structure, ARIA labels, and screen reader support.
frontend/src-tauri/tauri.conf.json (2)
10-10: Verify build time and artifact size implications.Changing from an explicit list to
"all"will generate bundle formats for all platforms supported by Tauri. This aligns with the PR's goal of adding broader Linux distribution support but may increase CI build time and storage requirements for release artifacts.Please verify that your CI/CD pipeline can handle the additional build time and artifact storage, especially if this runs on every PR or commit.
16-21: Bothappimage.bundleMediaFrameworkandrpm.releaseare supported in Tauri 2.9.1. The configuration is correct—bundleMediaFramework: trueincludes media libraries in the AppImage, andrpm.release: "1"properly sets the RPM release number. No changes needed.README.md (1)
5-34: LGTM! Clear and comprehensive installation documentation.The installation instructions are well-organized and cover multiple platforms and installation methods. The inclusion of multiple AUR helpers and both stable/git variants provides good user flexibility.
aur/PKGBUILD-git (2)
52-55: LGTM! Standard pkgver() implementation for git packages.The version detection using
git describewith appropriate transformation and fallback is correct for AUR git packages.
139-163: LGTM! Good fallback for missing LICENSE file.The license installation with inline fallback ensures the package always includes a license, even if the LICENSE file is missing from the repository. This is good defensive packaging practice.
aur/README.md (3)
13-49: LGTM! Clear installation instructions.The installation section covers multiple AUR helpers and both package variants, providing users with flexible installation options.
79-99: LGTM! Secure SSH key setup instructions.The SSH key generation instructions follow current best practices by using ed25519 and clearly separate public (for AUR) and private (for GitHub) key handling.
101-122: LGTM! Helpful troubleshooting section.The troubleshooting guide covers common issues with practical diagnostic and resolution commands.
.github/workflows/build-and-release.yml (1)
391-406: LGTM!The extraction, repackaging, and checksum generation logic is correct and follows standard practices for creating distribution archives.
aur/PKGBUILD (2)
51-85: LGTM!The
package()function is well-structured with appropriate defensive checks for optional files (icons, libraries, license). The renaming frompicto-pytopictopyis consistently handled across binaries, desktop entries, and icons.
48-49: Consider replacingSKIPwith actual checksums for production releases.Using
SKIPbypasses integrity verification, which is acceptable during development but poses a security risk in production. Thepublish-aur.ymlworkflow computes checksums from the built artifacts (pictopy_amd64.tar.gzandpictopy_arm64.tar.gz) and replaces these placeholders with actual checksums before publishing to AUR, so this approach is appropriate for automated releases.aur/pictopy.install (1)
1-47: LGTM!The install hooks follow Arch Linux packaging best practices:
- Proper guards for optional executables (
gtk-update-icon-cache,update-desktop-database)- Helpful user messaging about first-run AI model downloads and data preservation
- Correct use of
$1inpost_upgradefor version parameter.github/workflows/publish-aur.yml (2)
149-162:force_push: truemay overwrite manual AUR fixes.Using
force_push: truewill overwrite any changes made directly to the AUR repository. This is acceptable if the AUR package is exclusively managed through this workflow, but be aware it could discard legitimate manual fixes (e.g., urgent patches applied directly to AUR).Is the AUR repository intended to be managed exclusively through this CI workflow?
62-87: LGTM!The PKGBUILD update logic correctly handles version substitution and checksums. The sed patterns for updating source URLs and checksums are appropriate.
1 participant
Description
This PR adds a confirmation dialog before folder deletion to prevent accidental data loss.
Changes Made
Before
Clicking the delete button immediately deleted the folder without any warning.
After
Related Issue
Fixes #939
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.