Skip to content

Improve terminal backup#2007

Draft
RohitKushvaha01 wants to merge 5 commits intoAcode-Foundation:mainfrom
RohitKushvaha01:main
Draft

Improve terminal backup#2007
RohitKushvaha01 wants to merge 5 commits intoAcode-Foundation:mainfrom
RohitKushvaha01:main

Conversation

@RohitKushvaha01
Copy link
Copy Markdown
Member

No description provided.

@RohitKushvaha01 RohitKushvaha01 self-assigned this Apr 2, 2026
@RohitKushvaha01 RohitKushvaha01 marked this pull request as draft April 2, 2026 12:20
@RohitKushvaha01 RohitKushvaha01 marked this pull request as draft April 2, 2026 12:20
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR improves the terminal backup flow by introducing a new dedicated TerminalBackup settings page (with checkboxes for Alpine base and home directory selection), making the Terminal.backup() API accept selective-backup options, fixing the resolved backup file path to usr/aterm_backup.tar, and binding /home and /root inside the Alpine container to $PREFIX/public.

Key changes and findings:

  • P1 bug in Terminal.js: "alpine/data" is absent from the excludePaths array. When packages is true, the code calls excludePaths.splice(excludePaths.indexOf("alpine/data"), 1) which evaluates to splice(-1, 1) and removes the last element ("alpine/tmp") instead. This causes temp files to be included in the backup when packages are selected, and alpine/data is never actually excluded when packages are disabled.
  • Dead code in terminalBackup.js: The packages UI row is commented out, making setPackagesTooltip(), enforcePackageRule(), and the "packages" callback branch unreachable. The backupSettings.public field is also never used.
  • Unused import: helpers is imported in terminalBackup.js but never referenced.
  • Shared home/root bind mount in init-sandbox.sh: Both /home and /root now map to $PREFIX/public, which could cause dotfile and directory collisions for tools or packages that treat them as separate directories.

Confidence Score: 2/5

  • Not safe to merge as-is due to a logic bug in the backup archive selection that mishandles the alpine/data exclude path.
  • A concrete P1 logic bug causes the wrong tar exclude path to be removed when the packages option is enabled (splice on index -1 removes alpine/tmp instead of alpine/data), leading to incorrect backup content in both the packages-on and packages-off cases. The remaining issues are dead code and an unused import, which are cosmetic but indicate the feature is partially implemented.
  • src/plugins/terminal/www/Terminal.js requires attention for the excludePaths splice bug; src/pages/terminalBackup/terminalBackup.js has dead code that should be cleaned up before merge.

Important Files Changed

Filename Overview
src/plugins/terminal/www/Terminal.js Adds options support to backup() for selective archiving, but excludePaths is missing "alpine/data", causing a splice-on-wrong-index bug that incorrectly removes "alpine/tmp" and always includes data files regardless of the packages flag. Also fixes the resolved backup file path to include usr/.
src/pages/terminalBackup/terminalBackup.js New settings page for configuring and triggering backups. Contains dead code (helper functions and a callback case tied to the commented-out packages item), an unused public state field, and an unused helpers import.
src/pages/terminalBackup/index.js Thin lazy-loading entry point that dynamically imports and delegates to terminalBackup.js. No issues found.
src/plugins/terminal/scripts/init-sandbox.sh Adds bind mounts for /home and /root both pointing to $PREFIX/public, unifying home directories. This creates a shared underlying directory for both paths, which may cause unexpected collisions for tools that distinguish between /home and /root.
src/settings/terminalSettings.js Replaces the inline terminalBackup() function with the new TerminalBackup page import; refactors terminalRestore to module scope. The change is straightforward with no new issues introduced.

Sequence Diagram

sequenceDiagram
    participant User
    participant Settings as terminalSettings.js
    participant BackupPage as TerminalBackup page
    participant TerminalJS as Terminal.backup
    participant Shell as Executor shell
    participant FS as system.copyToUri

    User->>Settings: tap Backup
    Settings->>BackupPage: TerminalBackup(onclose)
    BackupPage->>User: show settings page (alpineBase, home)
    User->>BackupPage: configure options and tap backup button
    BackupPage->>BackupPage: FileBrowser folder picker returns url
    BackupPage->>TerminalJS: backup with alpineBase, packages, home options
    TerminalJS->>Shell: check isInstalled
    TerminalJS->>Shell: tar -cf $PREFIX/aterm_backup.tar with excludes and includes
    Shell-->>TerminalJS: ok
    TerminalJS-->>BackupPage: resolve dataDirectory + usr/aterm_backup.tar
    BackupPage->>FS: copy backup to user selected folder
    FS-->>BackupPage: done
    BackupPage->>User: alert Backup successful
Loading

Reviews (1): Last reviewed commit: "format" | Re-trigger Greptile

Comment on lines +358 to +361
if (opts.packages) {
includeFiles.push("alpine/data");
excludePaths.splice(excludePaths.indexOf("alpine/data"), 1);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 alpine/data missing from excludePaths, causing wrong element to be spliced

"alpine/data" is not present in the excludePaths array, so excludePaths.indexOf("alpine/data") returns -1. Calling excludePaths.splice(-1, 1) then removes the last element — "alpine/tmp" — instead of "alpine/data". This results in two unintended behaviors:

  1. When packages is false, alpine/data is never excluded from the tar archive (user-installed packages are always included).
  2. When packages is true, alpine/tmp is not excluded, so temporary files end up in the backup.

The fix is to add "alpine/data" to excludePaths so the splice call removes the correct entry:

Suggested change
if (opts.packages) {
includeFiles.push("alpine/data");
excludePaths.splice(excludePaths.indexOf("alpine/data"), 1);
}
if (opts.packages) {
includeFiles.push("alpine/data");
excludePaths.splice(excludePaths.indexOf("alpine/data"), 1);

And add "alpine/data" to the excludePaths array:

const excludePaths = [
    "alpine/data",   // <-- add this
    "alpine/system",
    ...
    "alpine/tmp",
];

import alert from "dialogs/alert";
import loader from "dialogs/loader";
import FileBrowser from "pages/fileBrowser";
import helpers from "utils/helpers";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unused import

helpers is imported but never referenced anywhere in this file.

Suggested change
import helpers from "utils/helpers";

(Remove the import entirely.)

Comment on lines +67 to +68
ARGS="$ARGS -b $PREFIX/public:/home"
ARGS="$ARGS -b $PREFIX/public:/root"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 /home and /root bound to the same directory

Both /home and /root inside the Alpine container now point to $PREFIX/public. While this is consistent with the backup's handling of the home option (which only archives public), it means any process writing to /home/<user> or /root shares the same underlying directory. This can cause unexpected collisions — for example, if a package creates /root/.bashrc and a user's home directory is also /home/root, they'll overwrite each other's dotfiles.

Consider whether the intent is truly to unify these paths, and add a comment explaining the reasoning if so.

Comment on lines +60 to +67
function setPackagesTooltip() {
const packagesRow = document.querySelector('[data-key="packages"]');
if (!packagesRow) return;
const input = packagesRow.querySelector(".input-checkbox input");
if (input) {
input.disabled = !!backupSettings.alpineBase;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Functions reference a commented-out UI element

The packages settings item is disabled in the items array (wrapped in a block comment), so these helper functions will always have a null reference for the row. setPackagesTooltip() returns early immediately, enforcePackageRule() never updates the checkbox UI, and the matching case in callback is never triggered.

Also, backupSettings.public is declared in the initial state object but never consumed by any item or callback — it can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant