From ccf8b30b810f50cc00ec5311ecf573b0dc441d5a Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 17:26:35 -0400 Subject: [PATCH 01/89] feat(apps): seamless container/service runnables Two backward-compatible improvements so container-based services (e.g. code-server) need no launcher script: - Auto-bind the cached repo clone into the container when a runnable resolves working_dir to 'repo'. Previously the repo symlink dangled inside containers (only the work dir and param paths were bound), so the documented `working_dir: repo` escape hatch was unusable. Container bind-path computation is extracted into _container_bind_paths(). - For service-type jobs, emit a preamble that picks a free TCP port on the compute node and exports it as FG_SERVICE_PORT (with FG_HOSTNAME). New service-only `auto_url` field makes Fileglancer write http://$FG_HOSTNAME:$FG_SERVICE_PORT to SERVICE_URL_PATH, so a service that binds $FG_SERVICE_PORT needs no URL-writing code of its own. Net effect: a container service becomes a one-liner, e.g. command: code-server --bind-addr 0.0.0.0:$FG_SERVICE_PORT with auto_url: true and container: ghcr.io/coder/code-server. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/__init__.py | 2 + fileglancer/apps/jobs.py | 93 ++++++++++++++++++++++----- fileglancer/model.py | 12 ++++ frontend/src/shared.types.ts | 1 + tests/test_apps.py | 121 +++++++++++++++++++++++++++++++++++ 5 files changed, 212 insertions(+), 17 deletions(-) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index ad92584b..e8e8e48c 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -24,7 +24,9 @@ ) from fileglancer.apps.jobs import ( # noqa: F401 _build_container_script, + _container_bind_paths, _container_sif_name, + _SERVICE_PORT_HELPER, cancel_job, start_job_monitor, stop_job_monitor, diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 066d91ca..5e1d3905 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -390,6 +390,33 @@ def _container_sif_name(container_url: str) -> str: _DEFAULT_CONTAINER_CACHE_DIR = "$HOME/.fileglancer/apptainer_cache" +# Runtime helper emitted for service jobs. It allocates a free TCP port on the +# compute node (a port chosen on the submit host would be meaningless there) and +# exports it as FG_SERVICE_PORT along with FG_HOSTNAME, so a service command can +# bind to a known address without reimplementing port discovery. Prefers python +# (authoritative bind-to-0), then falls back to a bash probe of ephemeral ports. +_SERVICE_PORT_HELPER = r"""# Fileglancer service setup: pick a free port and expose the hostname +__fg_free_port() { + local p py i + for py in python3 python; do + if command -v "$py" >/dev/null 2>&1; then + p="$("$py" -c 'import socket; s=socket.socket(); s.bind(("",0)); print(s.getsockname()[1]); s.close()' 2>/dev/null)" || true + [ -n "$p" ] && { printf '%s' "$p"; return 0; } + fi + done + for i in $(seq 1 50); do + p=$(( (RANDOM % 16384) + 49152 )) + if ! (exec 3<>"/dev/tcp/127.0.0.1/$p") 2>/dev/null; then + printf '%s' "$p"; return 0 + fi + done + printf '%s' 8080 +} +export FG_HOSTNAME="$(hostname)" +export FG_SERVICE_PORT="$(__fg_free_port)" +""" + + def _build_container_script( container_url: str, command: str, @@ -428,6 +455,43 @@ def _build_container_script( return "\n".join(lines) +def _container_bind_paths(entry_point, parameters: dict, username: Optional[str], + cached_repo_dir) -> list[str]: + """Compute the host paths to bind-mount into a container runnable. + + Binds are drawn from three sources, in order: + + 1. Each file/directory parameter's value (a file binds its parent dir). + Cloud-storage URIs and non-absolute values are skipped — they are not + bind-mountable and would otherwise produce garbage binds. + 2. The runnable's explicit `bind_paths`. + 3. The cached repo clone, but only when the command runs from `repo`. The + `repo` symlink lives inside the (already-bound) work dir yet points at + the clone outside it, so without this bind the symlink dangles in the + container and the `cd` into it fails. Container runnables default to + `work`, so this is only added when the author opts into `repo`. + + The work dir itself is always bound by `_build_container_script`, so it is + not included here. + """ + bind_paths: list[str] = [] + for param in entry_point.flat_parameters(): + if param.type in ("file", "directory") and param.key in parameters: + path_val = str(parameters[param.key]) + expanded = expand_user_path(path_val, username) + if expanded.startswith(_URI_PREFIXES) or not expanded.startswith("/"): + continue + if param.type == "directory": + bind_paths.append(expanded) + else: + bind_paths.append(str(Path(expanded).parent)) + if entry_point.bind_paths: + bind_paths.extend(entry_point.bind_paths) + if entry_point.effective_working_dir == "repo": + bind_paths.append(str(cached_repo_dir)) + return bind_paths + + async def submit_job( username: str, app_url: str, @@ -633,6 +697,15 @@ async def submit_job( preamble_lines.append(f"export PATH=$PATH:{path_suffix}") if entry_point.type == "service": preamble_lines.append('export SERVICE_URL_PATH="$FG_WORK_DIR/service_url"') + preamble_lines.append(_SERVICE_PORT_HELPER) + # When the author opts in with auto_url, write the service URL for them + # using the port we just allocated, so a service that binds to + # $FG_SERVICE_PORT needs no URL-writing code of its own. A service that + # manages its own URL simply leaves auto_url unset and overwrites this. + if entry_point.auto_url: + preamble_lines.append( + 'printf \'http://%s:%s\' "$FG_HOSTNAME" "$FG_SERVICE_PORT" > "$SERVICE_URL_PATH"' + ) # Choose the working directory. 'work' runs from the job's work dir (the # repo is still reachable via the `repo` symlink); 'repo' runs from the # cloned project (optionally the manifest's subdirectory). cd_suffix may @@ -654,23 +727,9 @@ async def submit_job( # If container is defined, wrap command in apptainer exec if effective_container: - bind_paths = [] - for param in entry_point.flat_parameters(): - if param.type in ("file", "directory") and param.key in parameters: - path_val = str(parameters[param.key]) - # Expand ~ against the user's home (same normalization as the - # command). Skip cloud-storage URIs and anything that isn't an - # absolute local path — those are not bind-mountable and would - # otherwise produce garbage binds (e.g. s3://bucket/k -> s3:/bucket). - expanded = expand_user_path(path_val, username) - if expanded.startswith(_URI_PREFIXES) or not expanded.startswith("/"): - continue - if param.type == "directory": - bind_paths.append(expanded) - else: - bind_paths.append(str(Path(expanded).parent)) - if entry_point.bind_paths: - bind_paths.extend(entry_point.bind_paths) + bind_paths = _container_bind_paths( + entry_point, parameters, username, cached_repo_dir + ) command = _build_container_script( container_url=effective_container, diff --git a/fileglancer/model.py b/fileglancer/model.py index 5743ddc8..77e3ccbb 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -436,6 +436,16 @@ class AppEntryPoint(BaseModel): ), default=None, ) + auto_url: bool = Field( + description=( + "For service entry points only: have Fileglancer write " + "http://$FG_HOSTNAME:$FG_SERVICE_PORT to SERVICE_URL_PATH before the " + "command runs. Set this when your service binds to the " + "Fileglancer-provided $FG_SERVICE_PORT so you don't have to write the " + "URL file yourself." + ), + default=False, + ) requirements: List[str] = Field( description="Required tools for this entry point, e.g. ['apptainer']. Merged with manifest-level requirements.", default=[], @@ -550,6 +560,8 @@ def check_conda_container_exclusive(self): raise ValueError("conda_env and container are mutually exclusive — use one or the other") if self.bind_paths and not self.container: raise ValueError("bind_paths requires container to be set") + if self.auto_url and self.type != "service": + raise ValueError("auto_url is only valid for service entry points (type: service)") return self diff --git a/frontend/src/shared.types.ts b/frontend/src/shared.types.ts index 05beb24b..baff9068 100644 --- a/frontend/src/shared.types.ts +++ b/frontend/src/shared.types.ts @@ -126,6 +126,7 @@ type AppEntryPoint = { bind_paths?: string[]; container_args?: string; working_dir?: 'work' | 'repo'; + auto_url?: boolean; requirements?: string[]; }; diff --git a/tests/test_apps.py b/tests/test_apps.py index 4ccc35a2..7452dcda 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -22,6 +22,8 @@ build_requirements_check, _container_sif_name, _build_container_script, + _container_bind_paths, + _SERVICE_PORT_HELPER, build_command, collect_path_parameters, expand_user_path, @@ -679,6 +681,125 @@ def test_docker_prefix_not_doubled(self): assert "docker://ghcr.io/org/image:1.0" in script +class TestContainerBindPaths: + """_container_bind_paths decides what host paths get mounted into a container.""" + + def _ep(self, **kwargs): + defaults = dict(id="t", name="T", command="echo", + container="ghcr.io/org/image:tag") + defaults.update(kwargs) + return AppEntryPoint(**defaults) + + def test_directory_param_bound_directly(self): + ep = self._ep(parameters=[ + AppParameter(flag="--data", name="Data", type="directory") + ]) + binds = _container_bind_paths(ep, {"data": "/groups/lab/data"}, None, "/cache/repo") + assert "/groups/lab/data" in binds + + def test_file_param_binds_parent_dir(self): + ep = self._ep(parameters=[ + AppParameter(flag="--in", name="In", type="file") + ]) + binds = _container_bind_paths(ep, {"in": "/groups/lab/x.tif"}, None, "/cache/repo") + assert "/groups/lab" in binds + assert "/groups/lab/x.tif" not in binds + + def test_cloud_uri_and_relative_skipped(self): + ep = self._ep(parameters=[ + AppParameter(flag="--a", name="A", type="directory"), + AppParameter(flag="--b", name="B", type="directory"), + ]) + binds = _container_bind_paths( + ep, {"a": "s3://bucket/key", "b": "./rel"}, None, "/cache/repo" + ) + assert binds == [] # neither is a bind-mountable absolute local path + + def test_explicit_bind_paths_included(self): + ep = self._ep(bind_paths=["/shared/ref", "/scratch"]) + binds = _container_bind_paths(ep, {}, None, "/cache/repo") + assert "/shared/ref" in binds and "/scratch" in binds + + def test_repo_bound_only_when_working_dir_repo(self): + # Container default is working_dir=work → repo NOT bound. + work_ep = self._ep() + assert "work" == work_ep.effective_working_dir + assert "/cache/repo" not in _container_bind_paths(work_ep, {}, None, "/cache/repo") + + # Opt into repo → the cached clone is bound so the repo symlink resolves. + repo_ep = self._ep(working_dir="repo") + assert "/cache/repo" in _container_bind_paths(repo_ep, {}, None, "/cache/repo") + + +# --- Service port / URL tests (FG_SERVICE_PORT + auto_url) --- + +class TestServicePortHelper: + """The preamble helper Fileglancer injects for service-type jobs.""" + + def test_helper_exports_port_and_hostname(self): + assert "export FG_SERVICE_PORT=" in _SERVICE_PORT_HELPER + assert "export FG_HOSTNAME=" in _SERVICE_PORT_HELPER + + def test_helper_is_valid_bash(self): + # The generated snippet must at least parse as bash. + result = subprocess.run( + ["bash", "-n", "-c", _SERVICE_PORT_HELPER], + capture_output=True, text=True, + ) + assert result.returncode == 0, result.stderr + + def test_helper_picks_a_free_port_at_runtime(self): + # Run the helper and confirm FG_SERVICE_PORT is a plausible TCP port. + script = _SERVICE_PORT_HELPER + '\necho "$FG_SERVICE_PORT"' + result = subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, + ) + assert result.returncode == 0, result.stderr + port = int(result.stdout.strip().splitlines()[-1]) + assert 1 <= port <= 65535 + + def test_helper_robust_under_set_euo_pipefail(self): + # A deployment may prepend `set -euo pipefail` via script_prologue; the + # helper must still allocate a port and not abort the job. + script = "set -euo pipefail\n" + _SERVICE_PORT_HELPER + '\necho "$FG_SERVICE_PORT"' + result = subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, + ) + assert result.returncode == 0, result.stderr + assert int(result.stdout.strip().splitlines()[-1]) >= 1 + + +class TestServiceAutoUrl: + """auto_url is service-only and drives Fileglancer to write the URL file.""" + + def test_auto_url_defaults_false(self): + ep = AppEntryPoint(id="t", name="T", command="echo", type="service") + assert ep.auto_url is False + + def test_auto_url_allowed_on_service(self): + ep = AppEntryPoint(id="t", name="T", command="echo", + type="service", auto_url=True) + assert ep.auto_url is True + + def test_auto_url_rejected_on_job(self): + with pytest.raises(ValidationError, match="auto_url is only valid for service"): + AppEntryPoint(id="t", name="T", command="echo", + type="job", auto_url=True) + + def test_auto_url_write_command_uses_provided_port(self): + # Mirror the preamble line submit_job emits when auto_url is set, and + # confirm it writes a valid http URL built from the helper's values. + write_line = ( + 'printf \'http://%s:%s\' "$FG_HOSTNAME" "$FG_SERVICE_PORT"' + ) + script = _SERVICE_PORT_HELPER + "\n" + write_line + result = subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, + ) + assert result.returncode == 0, result.stderr + assert result.stdout.startswith("http://") + + # --- Path validation tests --- from fileglancer.apps import validate_path_for_shell, validate_path_in_filestore From 69dd372fdf5f817c3418a119d0dc4e96ffaf1e9c Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 19:43:04 -0400 Subject: [PATCH 02/89] feat(apps): choose which apps to add from a multi-app repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a repo that contains more than one app now lets the user pick which apps to add instead of always adding all of them. Backend: - New POST /api/apps/discover returns the repo's manifests (path, name, description, already_added) without adding anything. - AppAddRequest gains optional manifest_paths; POST /api/apps adds only that subset when provided, or all discovered manifests when omitted (unchanged default). Shared _discover_repo_manifests keeps error handling identical across both endpoints. Frontend: - AddAppDialog is now two-step: enter the URL, then — only when the repo has more than one app — a checklist of apps with a Select all / Deselect all toggle and an "Add Selected (N)" action. Single-app repos add directly as before. Already-added apps are shown checked and disabled. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/model.py | 19 ++ fileglancer/server.py | 61 +++- frontend/src/components/AppLaunch.tsx | 2 +- frontend/src/components/Apps.tsx | 13 +- .../components/ui/AppsPage/AddAppDialog.tsx | 323 +++++++++++++----- frontend/src/queries/appsQueries.ts | 42 ++- frontend/src/shared.types.ts | 8 + tests/test_apps_endpoints.py | 106 ++++++ 8 files changed, 480 insertions(+), 94 deletions(-) diff --git a/fileglancer/model.py b/fileglancer/model.py index 77e3ccbb..5ec22df8 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -713,6 +713,25 @@ class ManifestFetchRequest(BaseModel): class AppAddRequest(BaseModel): """Request to add an app""" url: str = Field(description="URL to the app manifest or GitHub repo") + manifest_paths: Optional[List[str]] = Field( + description=( + "Manifest paths (relative dirs within the repo) to add. When omitted " + "or null, every discovered manifest in the repo is added. Use this to " + "add a subset of a multi-app repository." + ), + default=None, + ) + + +class DiscoveredApp(BaseModel): + """A manifest discovered in a repo, for previewing before adding""" + manifest_path: str = Field(description="Relative directory path to the manifest within the repo", default="") + name: str = Field(description="App name from the manifest") + description: Optional[str] = Field(description="App description from the manifest", default=None) + already_added: bool = Field( + description="True if the user has already added this manifest from this repo", + default=False, + ) class AppRemoveRequest(BaseModel): diff --git a/fileglancer/server.py b/fileglancer/server.py index 6655a058..e3096e44 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1732,15 +1732,15 @@ async def get_user_apps(username: str = Depends(get_current_user)): user_app.description = manifest.description return result - @app.post("/api/apps", response_model=list[UserApp], - description="Add an app by URL (discovers all manifests in the repo)") - async def add_user_app(body: AppAddRequest, - username: str = Depends(get_current_user)): - # Clone the repo and discover all manifests. The worker resolves the - # branch as the user, so a private repo's real default is used. + async def _discover_repo_manifests(url: str, username: str): + """Clone/scan a repo and return (resolved_branch, canonical_url, discovered). + + Shared by the discover and add endpoints so both surface identical, + user-facing errors for a bad URL/revision/private-repo clone. + """ try: resolved_branch, discovered = await apps_module.discover_app_manifests( - body.url, username=username) + url, username=username) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) except HTTPException as e: @@ -1764,9 +1764,50 @@ async def add_user_app(body: AppAddRequest, ) # Bake the worker-resolved revision into the stored URL (so a repo whose - # default is e.g. "master" dedups against an explicit ".../tree/master"), - # and record the user's requested revision separately ("" = unpinned). - canonical_url, requested = apps_module.canonical_app_url(body.url, resolved_branch) + # default is e.g. "master" dedups against an explicit ".../tree/master"). + canonical_url, _ = apps_module.canonical_app_url(url, resolved_branch) + return resolved_branch, canonical_url, discovered + + @app.post("/api/apps/discover", response_model=list[DiscoveredApp], + description="Discover the apps (manifests) in a repo without adding them") + async def discover_user_apps(body: AppAddRequest, + username: str = Depends(get_current_user)): + _, canonical_url, discovered = await _discover_repo_manifests(body.url, username) + with db.get_db_session(settings.db_url) as session: + return [ + DiscoveredApp( + manifest_path=manifest_path, + name=manifest.name, + description=manifest.description, + already_added=db.get_user_app( + session, username, canonical_url, manifest_path) is not None, + ) + for manifest_path, manifest in discovered + ] + + @app.post("/api/apps", response_model=list[UserApp], + description="Add apps by URL (all discovered manifests, or the subset in manifest_paths)") + async def add_user_app(body: AppAddRequest, + username: str = Depends(get_current_user)): + # Clone the repo and discover all manifests. The worker resolves the + # branch as the user, so a private repo's real default is used. + resolved_branch, canonical_url, discovered = await _discover_repo_manifests( + body.url, username) + + # Restrict to the requested subset when manifest_paths is provided; an + # omitted/null list means "add everything" (backward compatible). Paths + # not present in the repo are ignored. + if body.manifest_paths is not None: + wanted = set(body.manifest_paths) + discovered = [(p, m) for p, m in discovered if p in wanted] + if not discovered: + raise HTTPException( + status_code=400, + detail="None of the requested apps were found in the repository.", + ) + + # Record the user's requested revision separately ("" = unpinned). + _, requested = apps_module.canonical_app_url(body.url, resolved_branch) new_apps: list[UserApp] = [] with db.get_db_session(settings.db_url) as session: diff --git a/frontend/src/components/AppLaunch.tsx b/frontend/src/components/AppLaunch.tsx index 2da22e74..c66856a5 100644 --- a/frontend/src/components/AppLaunch.tsx +++ b/frontend/src/components/AppLaunch.tsx @@ -166,7 +166,7 @@ export default function AppLaunch() { const handleInstall = async () => { try { - const apps = await addAppMutation.mutateAsync(appUrl); + const apps = await addAppMutation.mutateAsync({ url: appUrl }); const count = apps.length; toast.success(`${count} app${count !== 1 ? 's' : ''} added`); } catch (error) { diff --git a/frontend/src/components/Apps.tsx b/frontend/src/components/Apps.tsx index 3e1531a6..039558d2 100644 --- a/frontend/src/components/Apps.tsx +++ b/frontend/src/components/Apps.tsx @@ -10,6 +10,7 @@ import DeleteAppDialog from '@/components/ui/AppsPage/DeleteAppDialog'; import { useAppsQuery, useAddAppMutation, + useDiscoverAppsMutation, useUpdateAppMutation, useRemoveAppMutation, useShareAppMutation, @@ -26,6 +27,7 @@ export default function Apps() { const appsQuery = useAppsQuery(); const addAppMutation = useAddAppMutation(); + const discoverAppsMutation = useDiscoverAppsMutation(); const updateAppMutation = useUpdateAppMutation(); const removeAppMutation = useRemoveAppMutation(); const shareAppMutation = useShareAppMutation(); @@ -85,8 +87,13 @@ export default function Apps() { toast.success('Shared to catalog'); }; - const handleAddFromUrl = async (url: string) => { - const apps = await addAppMutation.mutateAsync(url); + const handleDiscover = (url: string) => discoverAppsMutation.mutateAsync(url); + + const handleAddFromUrl = async (url: string, manifestPaths?: string[]) => { + const apps = await addAppMutation.mutateAsync({ + url, + manifest_paths: manifestPaths + }); const count = apps.length; toast.success(`${count} app${count !== 1 ? 's' : ''} added`); setShowAddDialog(false); @@ -165,8 +172,10 @@ export default function Apps() { setShowAddDialog(false)} + onDiscover={handleDiscover} open={showAddDialog} /> void; - readonly onAdd: (url: string) => Promise; + readonly onDiscover: (url: string) => Promise; + readonly onAdd: (url: string, manifestPaths?: string[]) => Promise; + readonly discovering: boolean; readonly adding: boolean; } export default function AddAppDialog({ open, onClose, + onDiscover, onAdd, + discovering, adding }: AddAppDialogProps) { const [repoUrl, setRepoUrl] = useState(''); const [branch, setBranch] = useState(''); const [urlError, setUrlError] = useState(''); + // Once a multi-app repo is discovered we move to a selection step. + const [phase, setPhase] = useState<'input' | 'select'>('input'); + const [appUrl, setAppUrl] = useState(''); + const [discovered, setDiscovered] = useState([]); + const [selected, setSelected] = useState>(new Set()); + + // Apps that can still be added (not already in the user's list). + const addable = discovered.filter(app => !app.already_added); + const allSelected = addable.length > 0 && selected.size === addable.length; + + const resetState = () => { + setRepoUrl(''); + setBranch(''); + setUrlError(''); + setPhase('input'); + setAppUrl(''); + setDiscovered([]); + setSelected(new Set()); + }; const validateUrl = (url: string): boolean => { if (!url.trim()) { @@ -37,25 +62,66 @@ export default function AddAppDialog({ return true; }; - const handleAdd = async () => { + // Step 1: discover the apps in the repo. A single-app repo is added straight + // away; a multi-app repo advances to the checkbox selection step. + const handleContinue = async () => { if (!validateUrl(repoUrl)) { return; } - const appUrl = buildAppUrl(repoUrl, branch); + const url = buildAppUrl(repoUrl, branch); try { - await onAdd(appUrl); - setRepoUrl(''); - setBranch(''); + const apps = await onDiscover(url); + if (apps.length <= 1) { + await onAdd(url); + resetState(); + return; + } + setAppUrl(url); + setDiscovered(apps); + setSelected( + new Set(apps.filter(a => !a.already_added).map(a => a.manifest_path)) + ); setUrlError(''); + setPhase('select'); } catch (error) { - setUrlError(error instanceof Error ? error.message : 'Failed to add app'); + setUrlError( + error instanceof Error ? error.message : 'Failed to read repository' + ); } }; + // Step 2: add only the checked apps. + const handleAddSelected = async () => { + try { + await onAdd(appUrl, Array.from(selected)); + resetState(); + } catch (error) { + setUrlError( + error instanceof Error ? error.message : 'Failed to add apps' + ); + } + }; + + const toggleOne = (manifestPath: string) => { + setSelected(prev => { + const next = new Set(prev); + if (next.has(manifestPath)) { + next.delete(manifestPath); + } else { + next.add(manifestPath); + } + return next; + }); + }; + + const toggleAll = () => { + setSelected( + allSelected ? new Set() : new Set(addable.map(a => a.manifest_path)) + ); + }; + const handleClose = () => { - setRepoUrl(''); - setBranch(''); - setUrlError(''); + resetState(); onClose(); }; @@ -63,73 +129,176 @@ export default function AddAppDialog({ return ( - - Add App - - - - Enter a GitHub repository URL (HTTPS or SSH) containing a{' '} - runnables.yaml manifest. Private repositories are accessed - over SSH using your configured SSH key. - - - - { - const value = e.target.value; - setRepoUrl(value); - validateUrl(value); - }} - onKeyDown={e => { - if (e.key === 'Enter') { - handleAdd(); - } - }} - placeholder="https://github.com/org/repo or git@github.com:org/repo.git" - type="text" - value={repoUrl} - /> - - - - { - setBranch(e.target.value); - }} - onKeyDown={e => { - if (e.key === 'Enter') { - handleAdd(); - } - }} - placeholder="main" - type="text" - value={branch} - /> - - -
- - Add App - - - Cancel - -
+ {phase === 'input' ? ( + <> + + Add App + + + + Enter a GitHub repository URL (HTTPS or SSH) containing one or more{' '} + runnables.yaml manifests. Private repositories are + accessed over SSH using your configured SSH key. + + + + { + const value = e.target.value; + setRepoUrl(value); + validateUrl(value); + }} + onKeyDown={e => { + if (e.key === 'Enter') { + handleContinue(); + } + }} + placeholder="https://github.com/org/repo or git@github.com:org/repo.git" + type="text" + value={repoUrl} + /> + + + + { + setBranch(e.target.value); + }} + onKeyDown={e => { + if (e.key === 'Enter') { + handleContinue(); + } + }} + placeholder="main" + type="text" + value={branch} + /> + + +
+ + Continue + + + Cancel + +
+ + ) : ( + <> + + Select apps to add + + + This repository contains {discovered.length} apps. Choose which ones + to add. + + +
+ + {selected.size} selected + + +
+ +
+ {discovered.map(app => { + const cbId = `discover-${app.manifest_path || 'root'}`; + return ( +
+ toggleOne(app.manifest_path)} + /> +
+ + {app.already_added ? ( + + (already added) + + ) : null} + {app.description ? ( +

+ {app.description} +

+ ) : null} +

+ {app.manifest_path || 'repository root'} +

+
+
+ ); + })} +
+ + {urlError ? ( + + {urlError} + + ) : null} + +
+ + {`Add Selected (${selected.size})`} + + { + setPhase('input'); + setUrlError(''); + }} + variant="ghost" + > + Back + + + Cancel + +
+ + )}
); } diff --git a/frontend/src/queries/appsQueries.ts b/frontend/src/queries/appsQueries.ts index 96d10343..eb04b118 100644 --- a/frontend/src/queries/appsQueries.ts +++ b/frontend/src/queries/appsQueries.ts @@ -6,7 +6,12 @@ import { getResponseJsonOrError, throwResponseNotOkError } from '@/queries/queryUtils'; -import type { AppListing, AppManifest, UserApp } from '@/shared.types'; +import type { + AppListing, + AppManifest, + DiscoveredApp, + UserApp +} from '@/shared.types'; // --- Query Keys --- @@ -71,15 +76,44 @@ export function useManifestPreviewMutation(): UseMutationResult< }); } +export function useDiscoverAppsMutation(): UseMutationResult< + DiscoveredApp[], + Error, + string +> { + return useMutation({ + mutationFn: async (url: string) => { + const response = await sendFetchRequest('/api/apps/discover', 'POST', { + url + }); + const data = await getResponseJsonOrError(response); + if (!response.ok) { + throwResponseNotOkError(response, data); + } + return data as DiscoveredApp[]; + } + }); +} + export function useAddAppMutation(): UseMutationResult< UserApp[], Error, - string + { url: string; manifest_paths?: string[] } > { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (url: string) => { - const response = await sendFetchRequest('/api/apps', 'POST', { url }); + mutationFn: async ({ + url, + manifest_paths + }: { + url: string; + manifest_paths?: string[]; + }) => { + const response = await sendFetchRequest('/api/apps', 'POST', { + url, + // Only include manifest_paths when provided; omitting it adds all apps. + ...(manifest_paths ? { manifest_paths } : {}) + }); const data = await getResponseJsonOrError(response); if (!response.ok) { throwResponseNotOkError(response, data); diff --git a/frontend/src/shared.types.ts b/frontend/src/shared.types.ts index baff9068..5fa36d1f 100644 --- a/frontend/src/shared.types.ts +++ b/frontend/src/shared.types.ts @@ -150,6 +150,13 @@ type UserApp = { listing_id?: number; }; +type DiscoveredApp = { + manifest_path: string; + name: string; + description?: string; + already_added: boolean; +}; + type AppListing = { id: number; owner_username: string; @@ -258,6 +265,7 @@ export type { AppParameterItem, AppParameterSection, AppResourceDefaults, + DiscoveredApp, FetchRequestOptions, FileOrFolder, FileSharePath, diff --git a/tests/test_apps_endpoints.py b/tests/test_apps_endpoints.py index 9c62b1a9..567fbe1c 100644 --- a/tests/test_apps_endpoints.py +++ b/tests/test_apps_endpoints.py @@ -335,6 +335,112 @@ def test_add_app_pinned_revision_kept(test_client, db_session): assert rows[0].branch == "dev" +def test_discover_lists_all_apps(test_client): + """POST /api/apps/discover returns every manifest in the repo without adding.""" + m1 = _make_manifest(name="VS Code", description="IDE") + m2 = _make_manifest(name="JupyterLab", description="Notebook") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", [("vscode", m1), ("jupyterlab", m2)]))): + response = test_client.post( + "/api/apps/discover", + json={"url": "https://github.com/owner/monorepo"}, + ) + + assert response.status_code == 200 + body = response.json() + assert [a["manifest_path"] for a in body] == ["vscode", "jupyterlab"] + assert [a["name"] for a in body] == ["VS Code", "JupyterLab"] + assert all(a["already_added"] is False for a in body) + # Discovery must not create any rows. + assert test_client.get("/api/apps").json() == [] + + +def test_discover_marks_already_added(test_client, db_session): + """Apps the user already has are flagged already_added=True.""" + m1 = _make_manifest(name="VS Code") + m2 = _make_manifest(name="JupyterLab") + _seed_app(db_session, url="https://github.com/owner/monorepo", + manifest_path="vscode", branch="", + manifest=m1.model_dump(mode="json")) + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", [("vscode", m1), ("jupyterlab", m2)]))): + response = test_client.post( + "/api/apps/discover", + json={"url": "https://github.com/owner/monorepo"}, + ) + + assert response.status_code == 200 + by_path = {a["manifest_path"]: a for a in response.json()} + assert by_path["vscode"]["already_added"] is True + assert by_path["jupyterlab"]["already_added"] is False + + +def test_discover_no_manifests_404(test_client): + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", []))): + response = test_client.post( + "/api/apps/discover", + json={"url": "https://github.com/owner/empty"}, + ) + assert response.status_code == 404 + + +def test_add_subset_via_manifest_paths(test_client, db_session): + """manifest_paths adds only the selected apps, not the whole repo.""" + m1 = _make_manifest(name="VS Code") + m2 = _make_manifest(name="JupyterLab") + m3 = _make_manifest(name="marimo") + discovered = [("vscode", m1), ("jupyterlab", m2), ("marimo", m3)] + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", discovered))): + response = test_client.post( + "/api/apps", + json={ + "url": "https://github.com/owner/monorepo", + "manifest_paths": ["vscode", "marimo"], + }, + ) + + assert response.status_code == 200 + body = response.json() + assert {a["manifest_path"] for a in body} == {"vscode", "marimo"} + rows = list_user_apps(db_session, TEST_USERNAME) + assert {r.manifest_path for r in rows} == {"vscode", "marimo"} + + +def test_add_null_manifest_paths_adds_all(test_client, db_session): + """Omitting manifest_paths preserves the add-everything behavior.""" + m1 = _make_manifest(name="VS Code") + m2 = _make_manifest(name="JupyterLab") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", [("vscode", m1), ("jupyterlab", m2)]))): + response = test_client.post( + "/api/apps", + json={"url": "https://github.com/owner/monorepo"}, + ) + + assert response.status_code == 200 + assert len(response.json()) == 2 + assert len(list_user_apps(db_session, TEST_USERNAME)) == 2 + + +def test_add_manifest_paths_no_match_400(test_client, db_session): + """A manifest_paths list matching nothing in the repo is a client error.""" + m1 = _make_manifest(name="VS Code") + with patch("fileglancer.apps.discover_app_manifests", + new=AsyncMock(return_value=("main", [("vscode", m1)]))): + response = test_client.post( + "/api/apps", + json={ + "url": "https://github.com/owner/monorepo", + "manifest_paths": ["does-not-exist"], + }, + ) + + assert response.status_code == 400 + assert len(list_user_apps(db_session, TEST_USERNAME)) == 0 + + def test_add_app_dedups_bare_against_resolved_default(test_client, db_session): """The dedup-hole fix: a bare URL for a master-default repo matches an already stored '/tree/master' row, so the add is a no-op (409).""" From 6675403f7082c9ecfb22a5da702f2394080d65f4 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 19:46:58 -0400 Subject: [PATCH 03/89] fix(apps): black text and drop manifest path in app picker In the multi-app selection list, use foreground (black) text instead of the purple secondary color for the description, the "already added" tag, and the intro line, and remove the manifest-path line from each row. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/ui/AppsPage/AddAppDialog.tsx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx index 7b7f59d2..bb88c1a7 100644 --- a/frontend/src/components/ui/AppsPage/AddAppDialog.tsx +++ b/frontend/src/components/ui/AppsPage/AddAppDialog.tsx @@ -204,7 +204,7 @@ export default function AddAppDialog({ Select apps to add - + This repository contains {discovered.length} apps. Choose which ones to add. @@ -250,18 +250,15 @@ export default function AddAppDialog({ {app.name} {app.already_added ? ( - + (already added) ) : null} {app.description ? ( -

+

{app.description}

) : null} -

- {app.manifest_path || 'repository root'} -

); From b581b75ff4669f81dec5fe3d3c238665b85ea39f Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 20:15:23 -0400 Subject: [PATCH 04/89] fix(jobs): truncate Command to one line in Execution box The Command row in the job page's Execution card wrapped across many lines. Add an opt-in `truncate` mode to InfoRow (single line, ellipsis, full text on hover) and use it for Command. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/JobDetail.tsx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 11a971ab..5b2c79e2 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -223,10 +223,14 @@ function InfoCard({ /** A label/value row; renders nothing when the value is empty. */ function InfoRow({ label, - value + value, + truncate = false }: { readonly label: string; readonly value: ReactNode; + // When true, render the value as a single line clipped with an ellipsis + // (full text shown on hover) instead of wrapping across multiple lines. + readonly truncate?: boolean; }) { if (value === null || value === undefined || value === '') { return null; @@ -236,7 +240,10 @@ function InfoRow({ {label}: - + {value} @@ -358,7 +365,7 @@ function JobOverview({ ))} ) : null} - + From bbd37aa71db1d50121228c1f7c02990f2d14a11b Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 20:24:17 -0400 Subject: [PATCH 05/89] fix(jobs): show Recent output whenever stdout has content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Overview "Recent output" panel was gated on job.started_at, so it stayed hidden while a job was still PENDING (started_at null) even though stdout.log already had content — while "Recent errors" showed because it gates on stderr content. Gate stdout on content too (hasStdout), mirroring stderr, so output appears as soon as there is any. Drops the now-unused stdoutPending prop. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/JobDetail.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 5b2c79e2..25d2c5e6 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -275,7 +275,6 @@ function JobOverview({ job, stdoutContent, stderrContent, - stdoutPending, isDarkMode, onViewStdout, onViewStderr @@ -283,7 +282,6 @@ function JobOverview({ readonly job: Job; readonly stdoutContent: string | null | undefined; readonly stderrContent: string | null | undefined; - readonly stdoutPending: boolean; readonly isDarkMode: boolean; readonly onViewStdout: () => void; readonly onViewStderr: () => void; @@ -304,6 +302,7 @@ function JobOverview({ stderrContent !== null && stderrContent !== undefined ? tailLines(stripLsfFooter(stderrContent), RECENT_OUTPUT_LINES) : null; + const hasStdout = Boolean(stdoutTail && stdoutTail.trim()); const hasStderr = Boolean(stderrTail && stderrTail.trim()); return ( @@ -373,7 +372,7 @@ function JobOverview({ - {job.started_at ? ( + {hasStdout ? (
@@ -388,7 +387,7 @@ function JobOverview({
@@ -739,7 +738,6 @@ export default function JobDetail() { onViewStdout={() => setActiveTab('stdout')} stderrContent={stderrQuery.data} stdoutContent={stdoutQuery.data} - stdoutPending={stdoutQuery.isPending} /> From 98309765baaaccfaeb9cfadf02dfaaad929b8dc0 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 20:36:41 -0400 Subject: [PATCH 06/89] fix(apps): don't orphan jobs submitted while the poll loop is stopping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The poll loop slept a full poll_interval before clearing _poll_task and returning when it found no active jobs. A job submitted during that window saw _poll_task still set (not done), so submit_job's ensure_poll_loop() no-op'd — then the loop exited, leaving no poller and the job stuck in PENDING forever (never advancing to RUNNING, so services never surfaced their service_url). Decide to stop *before* sleeping, clear _poll_task with no await in between, and re-check for active jobs (catching a job submitted during the same cycle) so the loop keeps running instead of exiting. Also wrap the locked section in try/finally so the poll lock can't leak if the task is cancelled while held. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/jobs.py | 56 +++++++++++++++++++++++++++++----------- tests/test_apps.py | 38 +++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 15 deletions(-) diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 5e1d3905..4698275d 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -185,24 +185,50 @@ async def _poll_loop(settings): lock_fd = None try: lock_fd = open(_POLL_LOCK_PATH, "w") - fcntl.flock(lock_fd, fcntl.LOCK_EX | fcntl.LOCK_NB) try: - has_jobs = await _poll_jobs(settings) - except Exception: - logger.exception("Error in job poll loop") - has_jobs = True # keep polling on error - # Hold lock through the sleep so no other worker polls - # until this interval is over - await asyncio.sleep(settings.cluster.poll_interval) - fcntl.flock(lock_fd, fcntl.LOCK_UN) - lock_fd.close() + fcntl.flock(lock_fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + # Another worker holds the lock this cycle — skip and retry. + lock_fd.close() + lock_fd = None + await asyncio.sleep(settings.cluster.poll_interval) + continue - if not has_jobs: - logger.info("No active jobs — poll loop stopping") - _poll_task = None - return + # try/finally so the lock is always released, even if the task is + # cancelled (e.g. on shutdown) while we hold it. + try: + try: + has_jobs = await _poll_jobs(settings) + except Exception: + logger.exception("Error in job poll loop") + has_jobs = True # keep polling on error + + if not has_jobs: + # No active jobs — stop the loop. Clear _poll_task and + # return *without* sleeping first: sleeping here (as the + # old code did) left a poll_interval-long window in which + # _poll_task was still set but the loop was on its way out, + # so a job submitted during that window saw a live task, + # ensure_poll_loop() no-op'd, and the job was then orphaned + # in PENDING when the loop exited. Re-check for active jobs + # first (no await in between) to catch a job that was + # submitted during this very cycle. + if _get_any_active_username(settings) is None: + logger.info("No active jobs — poll loop stopping") + _poll_task = None + return + # A job appeared mid-cycle; keep polling. + continue + + # Hold the lock through the sleep so a co-worker doesn't + # double-poll within the same interval. + await asyncio.sleep(settings.cluster.poll_interval) + finally: + fcntl.flock(lock_fd, fcntl.LOCK_UN) + lock_fd.close() + lock_fd = None except OSError: - # Another worker is polling this cycle — skip and retry + # Opening the lock file failed — retry next cycle. if lock_fd: lock_fd.close() await asyncio.sleep(settings.cluster.poll_interval) diff --git a/tests/test_apps.py b/tests/test_apps.py index 7452dcda..1f6581b1 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1905,3 +1905,41 @@ def test_falls_back_to_directory_name(self, tmp_path, monkeypatch): monkeypatch.setattr(pixi_mod, "_get_git_repo_name", lambda d: None) manifest = PixiAdapter().convert(tmp_path) assert manifest.name == tmp_path.name + + +class TestPollLoopStopRace: + """The poll loop must not orphan a job submitted while it is stopping. + + Regression: the loop used to sleep a full poll_interval before clearing + _poll_task, so a job submitted during that window saw a live task, + ensure_poll_loop() no-op'd, and the job was left in PENDING forever once the + loop exited. The loop now decides to stop *before* sleeping and re-checks + for active jobs (no await in between) so a mid-cycle submission keeps it + alive. + """ + + def test_keeps_polling_when_job_appears_during_stop(self, tmp_path, monkeypatch): + import asyncio + from types import SimpleNamespace + from unittest.mock import AsyncMock, patch + import fileglancer.apps.jobs as jobs_mod + + monkeypatch.setattr(jobs_mod, "_POLL_LOCK_PATH", str(tmp_path / "poll.lock")) + monkeypatch.setattr(jobs_mod, "_poll_task", None, raising=False) + settings = SimpleNamespace(cluster=SimpleNamespace(poll_interval=0.01)) + + # _poll_jobs reports "no active jobs" every cycle. The stop re-check + # returns a user first (a job appeared mid-cycle -> keep going), then + # None (really nothing -> stop). + with patch.object(jobs_mod, "_poll_jobs", new=AsyncMock(return_value=False)) as poll_jobs, \ + patch.object(jobs_mod, "_get_any_active_username", + side_effect=["someuser", None]) as active_user: + async def run(): + await asyncio.wait_for(jobs_mod._poll_loop(settings), timeout=5) + asyncio.run(run()) + + # Polled twice: it did NOT exit on the first no-jobs cycle because the + # re-check still saw an active job. + assert poll_jobs.await_count == 2 + assert active_user.call_count == 2 + assert jobs_mod._poll_task is None From c3eefacb38c523dab61ce57fc1515d6601fe566f Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 20:39:13 -0400 Subject: [PATCH 07/89] docs: reword comments to describe behavior, not history Rewrite comments in the poll-loop stop path, the discover/add endpoint, and the poll-loop regression test so they explain what the code does and why, without referring to prior implementations. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/jobs.py | 17 ++++++++--------- fileglancer/server.py | 4 ++-- tests/test_apps.py | 10 ++++------ 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 4698275d..df87f8d6 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -204,15 +204,14 @@ async def _poll_loop(settings): has_jobs = True # keep polling on error if not has_jobs: - # No active jobs — stop the loop. Clear _poll_task and - # return *without* sleeping first: sleeping here (as the - # old code did) left a poll_interval-long window in which - # _poll_task was still set but the loop was on its way out, - # so a job submitted during that window saw a live task, - # ensure_poll_loop() no-op'd, and the job was then orphaned - # in PENDING when the loop exited. Re-check for active jobs - # first (no await in between) to catch a job that was - # submitted during this very cycle. + # No active jobs: stop the loop. Clear _poll_task and return + # with no await in between, so a concurrent submit_job() + # either sees this task still alive or starts a fresh loop — + # there is no gap where _poll_task is set while the loop is + # exiting. Re-check for active jobs immediately before + # returning (again, no await in between) so a job submitted + # during this cycle keeps the loop running rather than being + # left unpolled. if _get_any_active_username(settings) is None: logger.info("No active jobs — poll loop stopping") _poll_task = None diff --git a/fileglancer/server.py b/fileglancer/server.py index e3096e44..e1534d60 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1795,8 +1795,8 @@ async def add_user_app(body: AppAddRequest, body.url, username) # Restrict to the requested subset when manifest_paths is provided; an - # omitted/null list means "add everything" (backward compatible). Paths - # not present in the repo are ignored. + # omitted/null list means "add every discovered manifest". Paths not + # present in the repo are ignored. if body.manifest_paths is not None: wanted = set(body.manifest_paths) discovered = [(p, m) for p, m in discovered if p in wanted] diff --git a/tests/test_apps.py b/tests/test_apps.py index 1f6581b1..1cfc2cb3 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -1910,12 +1910,10 @@ def test_falls_back_to_directory_name(self, tmp_path, monkeypatch): class TestPollLoopStopRace: """The poll loop must not orphan a job submitted while it is stopping. - Regression: the loop used to sleep a full poll_interval before clearing - _poll_task, so a job submitted during that window saw a live task, - ensure_poll_loop() no-op'd, and the job was left in PENDING forever once the - loop exited. The loop now decides to stop *before* sleeping and re-checks - for active jobs (no await in between) so a mid-cycle submission keeps it - alive. + When _poll_jobs reports no active jobs, the loop re-checks for active jobs + before exiting, with no await in between, and keeps polling if one appeared + during the cycle. So a job submitted just as the loop is about to stop is + still picked up rather than left unpolled in PENDING. """ def test_keeps_polling_when_job_appears_during_stop(self, tmp_path, monkeypatch): From 879e3be0203aade18aa8d4888f03b07131c1a302 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 21:23:36 -0400 Subject: [PATCH 08/89] feat(apps): readiness-gated auto_url + FG_SERVICE_TOKEN + service_url_suffix Make publishing a service URL a first-class part of auto_url instead of per-app pre_run boilerplate: - auto_url now waits until $FG_SERVICE_PORT accepts a connection before writing SERVICE_URL_PATH (background probe, bounded, logs to stderr on give-up), so the link never appears while the container image is still pulling or the server is still binding. - Mint FG_SERVICE_TOKEN (URL-safe) alongside FG_SERVICE_PORT/FG_HOSTNAME, so a service can use it for auth and splice it into the URL. - New service_url_suffix: a restricted template (literal URL text plus the ${FG_SERVICE_TOKEN}/${FG_SERVICE_PORT}/${FG_HOSTNAME} placeholders) appended to the published URL for one-click token auth. Validated for shell-safety and requires auto_url. Scheme stays http for now. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/__init__.py | 1 + fileglancer/apps/jobs.py | 43 +++++++++++-- fileglancer/model.py | 47 +++++++++++++-- frontend/src/shared.types.ts | 1 + tests/test_apps.py | 114 +++++++++++++++++++++++++++++++---- 5 files changed, 182 insertions(+), 24 deletions(-) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index e8e8e48c..f29c32d4 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -24,6 +24,7 @@ ) from fileglancer.apps.jobs import ( # noqa: F401 _build_container_script, + _build_service_url_publisher, _container_bind_paths, _container_sif_name, _SERVICE_PORT_HELPER, diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index df87f8d6..536593bd 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -420,7 +420,10 @@ def _container_sif_name(container_url: str) -> str: # exports it as FG_SERVICE_PORT along with FG_HOSTNAME, so a service command can # bind to a known address without reimplementing port discovery. Prefers python # (authoritative bind-to-0), then falls back to a bash probe of ephemeral ports. -_SERVICE_PORT_HELPER = r"""# Fileglancer service setup: pick a free port and expose the hostname +# It also mints FG_SERVICE_TOKEN, a URL-safe random secret the service can use +# for auth (e.g. --token-password="$FG_SERVICE_TOKEN") and that auto_url can +# splice into the published URL via the ${FG_SERVICE_TOKEN} placeholder. +_SERVICE_PORT_HELPER = r"""# Fileglancer service setup: pick a free port, expose the hostname, mint a token __fg_free_port() { local p py i for py in python3 python; do @@ -439,9 +442,36 @@ def _container_sif_name(container_url: str) -> str: } export FG_HOSTNAME="$(hostname)" export FG_SERVICE_PORT="$(__fg_free_port)" +export FG_SERVICE_TOKEN="$(openssl rand -hex 24 2>/dev/null || python3 -c 'import secrets; print(secrets.token_hex(24))' 2>/dev/null || date +%s%N | sha256sum | cut -c1-48)" """ +def _build_service_url_publisher(suffix: str = "") -> str: + """Bash that publishes the service URL once $FG_SERVICE_PORT is live. + + Runs in the background so it tolerates a slow start (the container image may + still be pulling); it writes SERVICE_URL_PATH only when the port accepts a + connection, and gives up after a bounded wait, logging to stderr. `suffix` is + appended to http://$FG_HOSTNAME:$FG_SERVICE_PORT and may reference the + ${FG_SERVICE_TOKEN}/${FG_SERVICE_PORT}/${FG_HOSTNAME} shell variables; it is + validated for shell-safety at manifest load (AppEntryPoint), so it is safe to + embed inside the double-quoted printf argument here. + """ + return "\n".join([ + "# Publish the service URL once the port is accepting connections.", + "(", + " for _ in $(seq 1 3600); do", + ' if (exec 3<>"/dev/tcp/127.0.0.1/$FG_SERVICE_PORT") 2>/dev/null; then', + f' printf \'http://%s:%s%s\' "$FG_HOSTNAME" "$FG_SERVICE_PORT" "{suffix}" > "$SERVICE_URL_PATH"', + " exit 0", + " fi", + " sleep 1", + " done", + ' echo "Fileglancer: port $FG_SERVICE_PORT never opened; service URL not published." >&2', + ") &", + ]) + + def _build_container_script( container_url: str, command: str, @@ -723,13 +753,14 @@ async def submit_job( if entry_point.type == "service": preamble_lines.append('export SERVICE_URL_PATH="$FG_WORK_DIR/service_url"') preamble_lines.append(_SERVICE_PORT_HELPER) - # When the author opts in with auto_url, write the service URL for them - # using the port we just allocated, so a service that binds to - # $FG_SERVICE_PORT needs no URL-writing code of its own. A service that - # manages its own URL simply leaves auto_url unset and overwrites this. + # With auto_url, Fileglancer publishes the URL for the author: a + # background probe waits for $FG_SERVICE_PORT to accept connections and + # then writes http://$FG_HOSTNAME:$FG_SERVICE_PORT plus the optional + # (validated) service_url_suffix. A service that manages its own URL + # leaves auto_url unset and writes SERVICE_URL_PATH itself. if entry_point.auto_url: preamble_lines.append( - 'printf \'http://%s:%s\' "$FG_HOSTNAME" "$FG_SERVICE_PORT" > "$SERVICE_URL_PATH"' + _build_service_url_publisher(entry_point.service_url_suffix or "") ) # Choose the working directory. 'work' runs from the job's work dir (the # repo is still reachable via the `repo` symlink); 'repo' runs from the diff --git a/fileglancer/model.py b/fileglancer/model.py index 5ec22df8..566874e1 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -438,14 +438,25 @@ class AppEntryPoint(BaseModel): ) auto_url: bool = Field( description=( - "For service entry points only: have Fileglancer write " - "http://$FG_HOSTNAME:$FG_SERVICE_PORT to SERVICE_URL_PATH before the " - "command runs. Set this when your service binds to the " - "Fileglancer-provided $FG_SERVICE_PORT so you don't have to write the " - "URL file yourself." + "For service entry points only: have Fileglancer publish the service " + "URL for you. Once the service's port ($FG_SERVICE_PORT) is accepting " + "connections, Fileglancer writes http://$FG_HOSTNAME:$FG_SERVICE_PORT " + "(plus service_url_suffix, if set) to SERVICE_URL_PATH. Set this when " + "your service binds to the Fileglancer-provided $FG_SERVICE_PORT." ), default=False, ) + service_url_suffix: Optional[str] = Field( + description=( + "For auto_url service entry points: text appended to " + "http://$FG_HOSTNAME:$FG_SERVICE_PORT when publishing the URL, e.g. a " + "path and/or query for one-click auth. May contain the placeholders " + "${FG_SERVICE_TOKEN}, ${FG_SERVICE_PORT}, ${FG_HOSTNAME} (braces " + "required) and literal URL text; nothing else. Example: " + "'/?access_token=${FG_SERVICE_TOKEN}'." + ), + default=None, + ) requirements: List[str] = Field( description="Required tools for this entry point, e.g. ['apptainer']. Merged with manifest-level requirements.", default=[], @@ -484,6 +495,23 @@ def validate_container(cls, v): raise ValueError(f"container URL contains forbidden characters: {v!r}") return v + @field_validator("service_url_suffix") + @classmethod + def validate_service_url_suffix(cls, v): + if v is None: + return v + # Strip the recognized ${FG_*} placeholders, then reject anything that + # would be unsafe or unexpanded inside the double-quoted shell string the + # suffix is emitted into (a stray $, quote, backtick, backslash, newline). + residual = _SERVICE_URL_PLACEHOLDER.sub("", v) + if _SERVICE_URL_UNSAFE.search(residual): + raise ValueError( + "service_url_suffix may contain only literal URL text and the " + "placeholders ${FG_SERVICE_TOKEN}, ${FG_SERVICE_PORT}, " + f"${{FG_HOSTNAME}} (braces required); got: {v!r}" + ) + return v + @field_validator("bind_paths") @classmethod def validate_bind_paths(cls, v): @@ -562,6 +590,8 @@ def check_conda_container_exclusive(self): raise ValueError("bind_paths requires container to be set") if self.auto_url and self.type != "service": raise ValueError("auto_url is only valid for service entry points (type: service)") + if self.service_url_suffix is not None and not self.auto_url: + raise ValueError("service_url_suffix requires auto_url to be set") return self @@ -575,6 +605,13 @@ def check_conda_container_exclusive(self): ) _SHELL_METACHAR_PATTERN = re.compile(r'[;&|`$(){}!<>\n\r]') +# Placeholders Fileglancer substitutes into service_url_suffix at runtime. +_SERVICE_URL_PLACEHOLDER = re.compile(r'\$\{(?:FG_SERVICE_TOKEN|FG_SERVICE_PORT|FG_HOSTNAME)\}') +# The suffix is emitted inside a double-quoted shell string, so only these are +# dangerous once the recognized placeholders are removed: a stray $, a double +# quote, a backtick, a backslash, or a newline. Everything else (?, &, =, /, %, +# ...) is literal inside double quotes and valid in a URL. +_SERVICE_URL_UNSAFE = re.compile(r'[$"`\\\n\r]') _CONDA_ENV_NAME_PATTERN = re.compile(r'^[a-zA-Z0-9_.-]+$') _CONDA_ENV_PATH_FORBIDDEN = re.compile(r'[;&|`$(){}!<>\n\r]') diff --git a/frontend/src/shared.types.ts b/frontend/src/shared.types.ts index 5fa36d1f..689c707d 100644 --- a/frontend/src/shared.types.ts +++ b/frontend/src/shared.types.ts @@ -127,6 +127,7 @@ type AppEntryPoint = { container_args?: string; working_dir?: 'work' | 'repo'; auto_url?: boolean; + service_url_suffix?: string; requirements?: string[]; }; diff --git a/tests/test_apps.py b/tests/test_apps.py index 1cfc2cb3..e71308b6 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -24,6 +24,7 @@ _build_container_script, _container_bind_paths, _SERVICE_PORT_HELPER, + _build_service_url_publisher, build_command, collect_path_parameters, expand_user_path, @@ -736,9 +737,19 @@ def test_repo_bound_only_when_working_dir_repo(self): class TestServicePortHelper: """The preamble helper Fileglancer injects for service-type jobs.""" - def test_helper_exports_port_and_hostname(self): + def test_helper_exports_port_hostname_and_token(self): assert "export FG_SERVICE_PORT=" in _SERVICE_PORT_HELPER assert "export FG_HOSTNAME=" in _SERVICE_PORT_HELPER + assert "export FG_SERVICE_TOKEN=" in _SERVICE_PORT_HELPER + + def test_helper_mints_a_urlsafe_token(self): + import re + script = _SERVICE_PORT_HELPER + '\necho "$FG_SERVICE_TOKEN"' + result = subprocess.run(["bash", "-c", script], capture_output=True, text=True) + assert result.returncode == 0, result.stderr + token = result.stdout.strip().splitlines()[-1] + # Non-empty and URL-safe (hex), so no encoding is needed in the URL. + assert re.fullmatch(r"[0-9a-f]{16,}", token), token def test_helper_is_valid_bash(self): # The generated snippet must at least parse as bash. @@ -770,7 +781,7 @@ def test_helper_robust_under_set_euo_pipefail(self): class TestServiceAutoUrl: - """auto_url is service-only and drives Fileglancer to write the URL file.""" + """auto_url is service-only and drives Fileglancer to publish the URL file.""" def test_auto_url_defaults_false(self): ep = AppEntryPoint(id="t", name="T", command="echo", type="service") @@ -786,18 +797,95 @@ def test_auto_url_rejected_on_job(self): AppEntryPoint(id="t", name="T", command="echo", type="job", auto_url=True) - def test_auto_url_write_command_uses_provided_port(self): - # Mirror the preamble line submit_job emits when auto_url is set, and - # confirm it writes a valid http URL built from the helper's values. - write_line = ( - 'printf \'http://%s:%s\' "$FG_HOSTNAME" "$FG_SERVICE_PORT"' - ) - script = _SERVICE_PORT_HELPER + "\n" + write_line - result = subprocess.run( - ["bash", "-c", script], capture_output=True, text=True, - ) + +class TestServiceUrlSuffix: + """service_url_suffix is a restricted template validated for shell-safety.""" + + def _ep(self, suffix, **kw): + kw.setdefault("type", "service") + kw.setdefault("auto_url", True) + return AppEntryPoint(id="t", name="T", command="echo", + service_url_suffix=suffix, **kw) + + def test_allows_literal_and_known_placeholders(self): + ep = self._ep("/?access_token=${FG_SERVICE_TOKEN}") + assert ep.service_url_suffix == "/?access_token=${FG_SERVICE_TOKEN}" + + def test_allows_multiple_query_params_and_paths(self): + # ? & = / are literal inside the double-quoted emission; base paths ok. + ep = self._ep("/lab?token=${FG_SERVICE_TOKEN}&reset=1") + assert "reset=1" in ep.service_url_suffix + + def test_rejects_unknown_placeholder(self): + with pytest.raises(ValidationError, match="service_url_suffix"): + self._ep("/?t=${SECRET}") + + def test_rejects_bare_dollar(self): + with pytest.raises(ValidationError, match="service_url_suffix"): + self._ep("/?t=$FG_SERVICE_TOKEN") # braces required + + def test_rejects_shell_injection_chars(self): + for bad in ['/`whoami`', '/"x"', "/\\x"]: + with pytest.raises(ValidationError, match="service_url_suffix"): + self._ep(bad) + + def test_requires_auto_url(self): + with pytest.raises(ValidationError, match="service_url_suffix requires auto_url"): + AppEntryPoint(id="t", name="T", command="echo", type="service", + auto_url=False, service_url_suffix="/?t=x") + + +class TestServiceUrlPublisher: + """The backgrounded readiness probe that publishes SERVICE_URL_PATH.""" + + def test_publisher_is_valid_bash(self): + snippet = _build_service_url_publisher("/?access_token=${FG_SERVICE_TOKEN}") + result = subprocess.run(["bash", "-n", "-c", snippet], + capture_output=True, text=True) assert result.returncode == 0, result.stderr - assert result.stdout.startswith("http://") + assert "SERVICE_URL_PATH" in snippet and "3600" in snippet + + def test_publishes_tokenized_url_only_once_port_is_up(self, tmp_path): + import socket + # Bind a real port so the probe's TCP connect succeeds. + srv = socket.socket() + srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + srv.bind(("127.0.0.1", 0)) + port = srv.getsockname()[1] + srv.listen() + try: + url_file = tmp_path / "service_url" + env = ( + f'export FG_HOSTNAME=h1 FG_SERVICE_PORT={port} ' + f'FG_SERVICE_TOKEN=deadbeef SERVICE_URL_PATH={url_file}\n' + ) + # Run the publisher in the foreground (drop the trailing &) so the + # test can wait for it deterministically. + snippet = _build_service_url_publisher( + "/?access_token=${FG_SERVICE_TOKEN}").rstrip().removesuffix("&") + result = subprocess.run(["bash", "-c", env + snippet], + capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stderr + assert url_file.read_text() == f"http://h1:{port}/?access_token=deadbeef" + finally: + srv.close() + + def test_does_not_publish_when_port_never_opens(self, tmp_path): + import socket + # Grab a free port number, then close it so nothing is listening. + s = socket.socket(); s.bind(("127.0.0.1", 0)); port = s.getsockname()[1]; s.close() + url_file = tmp_path / "service_url" + env = ( + f'export FG_HOSTNAME=h1 FG_SERVICE_PORT={port} ' + f'FG_SERVICE_TOKEN=x SERVICE_URL_PATH={url_file}\n' + ) + # Shrink the loop to 2 iterations so the timeout path is quick. + snippet = _build_service_url_publisher("").replace("$(seq 1 3600)", "$(seq 1 2)") + snippet = snippet.rstrip().removesuffix("&") + result = subprocess.run(["bash", "-c", env + snippet], + capture_output=True, text=True, timeout=30) + assert not url_file.exists() + assert "never opened" in result.stderr # --- Path validation tests --- From 335add476601c9d8e97ec5d3e14dec8273c17513 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 22:01:21 -0400 Subject: [PATCH 09/89] =?UTF-8?q?feat(apps):=20surface=20"Downloading=20co?= =?UTF-8?q?ntainer=20image=E2=80=A6"=20during=20service=20startup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A container service can sit for minutes pulling its image before the URL appears, with nothing telling the user why. The generated job script now reports its phase: it writes 'pulling_image' to $FG_PHASE_PATH (a 'phase' file in the work dir) around the apptainer pull, then 'starting'. The worker reads it alongside the service URL, the Job model carries a `phase` field, and the job page shows "Downloading container image… first launch can take a few minutes" instead of a bare "starting up". Since Fileglancer generates the script, the phase is emitted deterministically at the pull instruction (only when the SIF isn't cached) rather than scraped from Apptainer's logs. The phase file rides the same work-dir read path as service_url, so no new access-model or extra worker dispatch. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/__init__.py | 1 + fileglancer/apps/jobfiles.py | 31 +++++++++++++++++++++ fileglancer/apps/jobs.py | 7 +++++ fileglancer/model.py | 1 + fileglancer/server.py | 16 +++++++---- fileglancer/user_worker.py | 7 +++-- frontend/src/components/JobDetail.tsx | 4 ++- frontend/src/shared.types.ts | 1 + tests/test_apps.py | 39 ++++++++++++++++++++++++++- 9 files changed, 96 insertions(+), 11 deletions(-) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index f29c32d4..61a68f8b 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -37,4 +37,5 @@ get_job_file_content, get_job_file_paths, get_service_url, + get_service_phase, ) diff --git a/fileglancer/apps/jobfiles.py b/fileglancer/apps/jobfiles.py index 33ff0543..c783e6a5 100644 --- a/fileglancer/apps/jobfiles.py +++ b/fileglancer/apps/jobfiles.py @@ -115,6 +115,37 @@ def get_service_url(db_job: db.JobDB) -> Optional[str]: return url +# Startup phases a service job writes to its 'phase' file so the UI can explain +# a wait before the URL appears (chiefly a container image still downloading). +_SERVICE_PHASES = ("pulling_image", "starting") + + +def get_service_phase(db_job: db.JobDB) -> Optional[str]: + """Read the current startup phase from a service job's work directory. + + The generated job script writes a short marker (see _SERVICE_PHASES) to a + 'phase' file — e.g. 'pulling_image' while Apptainer downloads the container + image, then 'starting'. This lets the UI show why a service is taking a + while before its URL is published. Returns None unless the job is a RUNNING + service with a recognized phase written. + """ + if getattr(db_job, 'entry_point_type', 'job') != 'service': + return None + if db_job.status != 'RUNNING': + return None + + phase_file = _resolve_work_dir(db_job) / "phase" + if not phase_file.is_file(): + return None + + try: + phase = phase_file.read_text().strip() + except OSError: + return None + + return phase if phase in _SERVICE_PHASES else None + + def get_job_file_paths(db_job: db.JobDB) -> dict[str, dict]: """Return file path info for a job's files (script, stdout, stderr, service_url). diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index 536593bd..f5a316dd 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -502,8 +502,12 @@ def _build_container_script( 'mkdir -p "$APPTAINER_CACHE_DIR"', f'SIF_PATH="$APPTAINER_CACHE_DIR/{sif_name}"', 'if [ ! -f "$SIF_PATH" ]; then', + # Report the (often multi-minute) image download so the UI can say so. + # FG_PHASE_PATH is set in the preamble; guard in case it is not. + ' [ -n "$FG_PHASE_PATH" ] && printf pulling_image > "$FG_PHASE_PATH" 2>/dev/null || true', f' apptainer pull "$SIF_PATH" {shlex.quote(docker_url)}', 'fi', + '[ -n "$FG_PHASE_PATH" ] && printf starting > "$FG_PHASE_PATH" 2>/dev/null || true', f'apptainer exec {bind_flags}{extra} "$SIF_PATH" \\', f' {command}', ] @@ -740,6 +744,9 @@ async def submit_job( preamble_lines = [ "unset PIXI_PROJECT_MANIFEST", f"export FG_WORK_DIR={shlex.quote(str(work_dir))}", + # Where the script reports its startup phase (e.g. pulling a container + # image). The UI reads this to explain a wait before a service is ready. + 'export FG_PHASE_PATH="$FG_WORK_DIR/phase"', ] # For local executor, trap EXIT to write the exit code to a file so # PID-based polling can determine the final status after the process exits. diff --git a/fileglancer/model.py b/fileglancer/model.py index 566874e1..4373d23a 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -812,6 +812,7 @@ class Job(BaseModel): work_dir: Optional[str] = Field(description="Working directory the job ran in", default=None) cluster_job_id: Optional[str] = Field(description="Cluster-assigned job ID", default=None) service_url: Optional[str] = Field(description="URL of the running service (for service-type jobs)", default=None) + phase: Optional[str] = Field(description="Startup phase of a running service before its URL is ready, e.g. 'pulling_image' or 'starting'", default=None) created_at: datetime = Field(description="When the job was created") started_at: Optional[datetime] = Field(description="When the job started running", default=None) finished_at: Optional[datetime] = Field(description="When the job finished", default=None) diff --git a/fileglancer/server.py b/fileglancer/server.py index e1534d60..84306bf2 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -2091,13 +2091,15 @@ async def get_jobs(status: Optional[str] = Query(None, description="Filter by st jobs = [] for j in db_jobs: service_url = None + phase = None if getattr(j, 'entry_point_type', 'job') == 'service' and j.status == 'RUNNING': try: result = await _worker_exec(username, "get_service_url", job_id=j.id) service_url = result.get("service_url") + phase = result.get("phase") except Exception: pass - jobs.append(_convert_job(j, service_url=service_url)) + jobs.append(_convert_job(j, service_url=service_url, phase=phase)) return JobResponse(jobs=jobs) @app.get("/api/jobs/{job_id}", response_model=Job, @@ -2113,13 +2115,15 @@ async def get_job(job_id: int, # rather than paying a worker roundtrip + NFS glob/stat. files = apps_module.get_job_file_paths(db_job) service_url = None + phase = None if getattr(db_job, 'entry_point_type', 'job') == 'service' and db_job.status == 'RUNNING': try: svc_result = await _worker_exec(username, "get_service_url", job_id=job_id) service_url = svc_result.get("service_url") + phase = svc_result.get("phase") except Exception: pass - return _convert_job(db_job, service_url=service_url, files=files) + return _convert_job(db_job, service_url=service_url, files=files, phase=phase) @app.post("/api/jobs/{job_id}/cancel", description="Cancel a running job") @@ -2180,11 +2184,12 @@ def _ensure_utc(dt: Optional[datetime]) -> Optional[datetime]: return dt.replace(tzinfo=UTC) return dt - def _convert_job(db_job: db.JobDB, service_url: str = None, files: dict = None) -> Job: + def _convert_job(db_job: db.JobDB, service_url: str = None, files: dict = None, + phase: str = None) -> Job: """Convert a database JobDB to a Pydantic Job model. - File-reading fields (service_url, files) must be passed in pre-computed - by the caller, since they require user-context file I/O. + File-reading fields (service_url, phase, files) must be passed in + pre-computed by the caller, since they require user-context file I/O. """ return Job( id=db_job.id, @@ -2210,6 +2215,7 @@ def _convert_job(db_job: db.JobDB, service_url: str = None, files: dict = None) work_dir=db_job.work_dir, cluster_job_id=db_job.cluster_job_id, service_url=service_url, + phase=phase, created_at=_ensure_utc(db_job.created_at), started_at=_ensure_utc(db_job.started_at), finished_at=_ensure_utc(db_job.finished_at), diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index 34112abe..a588aa22 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -743,15 +743,14 @@ def _action_get_job_file(request: dict, ctx: WorkerContext) -> dict: @action("get_service_url") def _action_get_service_url(request: dict, ctx: WorkerContext) -> dict: - """Read service URL from job work directory.""" - from fileglancer.apps.jobfiles import get_service_url + """Read the service URL and startup phase from a job's work directory.""" + from fileglancer.apps.jobfiles import get_service_url, get_service_phase job_id = request["job_id"] db_job = ctx.db.get_job(job_id, ctx.username) if db_job is None: return {"error": f"Job {job_id} not found", "status_code": 404} - url = get_service_url(db_job) - return {"service_url": url} + return {"service_url": get_service_url(db_job), "phase": get_service_phase(db_job)} # --------------------------------------------------------------------------- diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 25d2c5e6..799320a6 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -675,7 +675,9 @@ export default function JobDetail() { - Service is starting up... + {job.phase === 'pulling_image' + ? 'Downloading container image… first launch can take a few minutes.' + : 'Service is starting up…'} "$FG_PHASE_PATH"' in script + assert 'printf starting > "$FG_PHASE_PATH"' in script + pull_i = script.index("apptainer pull") + assert script.index("pulling_image") < pull_i < script.index("apptainer exec") + + def _svc(self, tmp_path, phase=None, **kw): + if phase is not None: + (tmp_path / "phase").write_text(phase) + kw.setdefault("entry_point_type", "service") + kw.setdefault("status", "RUNNING") + return _fake_job(work_dir=str(tmp_path), **kw) + + def test_reads_recognized_phase_for_running_service(self, tmp_path): + assert get_service_phase(self._svc(tmp_path, "pulling_image")) == "pulling_image" + (tmp_path / "phase").write_text("starting") + assert get_service_phase(self._svc(tmp_path)) == "starting" + + def test_none_when_not_running(self, tmp_path): + assert get_service_phase(self._svc(tmp_path, "pulling_image", status="PENDING")) is None + + def test_none_for_non_service(self, tmp_path): + assert get_service_phase(self._svc(tmp_path, "starting", entry_point_type="job")) is None + + def test_none_when_no_phase_file(self, tmp_path): + assert get_service_phase(self._svc(tmp_path)) is None + + def test_rejects_unknown_phase(self, tmp_path): + assert get_service_phase(self._svc(tmp_path, "garbage")) is None + + # --- Path validation tests --- from fileglancer.apps import validate_path_for_shell, validate_path_in_filestore From 1e21e121cbdd0b4d656c6eef16cd127bb2dc7ef0 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 22:17:02 -0400 Subject: [PATCH 10/89] fix(apps): pull with --disable-cache to avoid duplicate SIF storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generated script only pulls when its own SIF is missing, so Apptainer's internal layer/SIF cache (~/.apptainer/cache) mostly just duplicated every image — gigabytes that sped up a re-pull that rarely happens. Add --disable-cache to `apptainer pull` so the .sif we keep is the only copy; a re-pull (if that .sif is deleted) re-downloads from the registry. Co-Authored-By: Claude Opus 4.8 (1M context) --- fileglancer/apps/jobs.py | 7 ++++++- tests/test_apps.py | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index f5a316dd..c988dfd1 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -505,7 +505,12 @@ def _build_container_script( # Report the (often multi-minute) image download so the UI can say so. # FG_PHASE_PATH is set in the preamble; guard in case it is not. ' [ -n "$FG_PHASE_PATH" ] && printf pulling_image > "$FG_PHASE_PATH" 2>/dev/null || true', - f' apptainer pull "$SIF_PATH" {shlex.quote(docker_url)}', + # --disable-cache: the built SIF here is the only copy we keep. We pull + # only when it's missing, so Apptainer's own layer/SIF cache would just + # duplicate gigabytes to speed up a re-pull that rarely happens. Skipping + # it means a re-pull (if this SIF is deleted) re-downloads from the + # registry, which is the right trade for not double-storing every image. + f' apptainer pull --disable-cache "$SIF_PATH" {shlex.quote(docker_url)}', 'fi', '[ -n "$FG_PHASE_PATH" ] && printf starting > "$FG_PHASE_PATH" 2>/dev/null || true', f'apptainer exec {bind_flags}{extra} "$SIF_PATH" \\', diff --git a/tests/test_apps.py b/tests/test_apps.py index f97baf12..aa042779 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -624,6 +624,9 @@ def test_basic_script(self): bind_paths=[], ) assert "apptainer pull" in script + # Pull without populating Apptainer's own cache, so the SIF we keep is + # the only copy (no multi-GB duplicate under ~/.apptainer/cache). + assert "apptainer pull --disable-cache" in script assert "apptainer exec" in script assert "docker://ghcr.io/org/image:1.0" in script assert "ghcr.io_org_image_1.0.sif" in script From 6773a066cabc020ec1d6384d53c1a8dfd1002042 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 1 Jul 2026 22:44:40 -0400 Subject: [PATCH 11/89] fix(apps): keep the Apps tabs visible on the job detail page The job detail route (apps/jobs/:jobId) was a standalone route outside AppsLayout, so drilling into a job dropped the My Apps / App Catalog / Jobs tab bar. Nest it as a child of the AppsLayout route so it renders in the layout's Outlet with the tabs; the Jobs tab (NavLink to /apps/jobs, non-exact) stays highlighted. The URL is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/App.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 9bd7c430..7b05f1eb 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -128,6 +128,9 @@ const AppComponent = () => { } index /> } path="catalog" /> } path="jobs" /> + {/* Render job detail inside the layout so the My Apps / App + Catalog / Jobs tabs stay visible when drilling into a job. */} + } path="jobs/:jobId" /> { } path="apps/relaunch/:owner/:repo" /> - - - - } - path="apps/jobs/:jobId" - /> {tasksEnabled ? ( Date: Wed, 1 Jul 2026 22:48:42 -0400 Subject: [PATCH 12/89] fix(apps): drop the redundant "Back to Jobs" button on job detail The job detail page now renders inside AppsLayout with the tab bar, so the Jobs tab already provides the way back. Remove the button and its now-unused icon import. Co-Authored-By: Claude Opus 4.8 (1M context) --- frontend/src/components/JobDetail.tsx | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/frontend/src/components/JobDetail.tsx b/frontend/src/components/JobDetail.tsx index 799320a6..185d7e10 100644 --- a/frontend/src/components/JobDetail.tsx +++ b/frontend/src/components/JobDetail.tsx @@ -5,7 +5,6 @@ import { Link, useNavigate, useParams } from 'react-router'; import { Card, Tabs, Typography } from '@material-tailwind/react'; import { HiExternalLink, - HiOutlineArrowLeft, HiOutlineDownload, HiOutlineRefresh, HiOutlineStop @@ -541,15 +540,6 @@ export default function JobDetail() { return (
- navigate('/apps/jobs')} - variant="outline" - > - Back to Jobs - - {jobQuery.isPending ? (
{/* Title skeleton */} From 2e218f4b4a299e1ecd2e431df518b90bdd54e51d Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 08:32:21 -0400 Subject: [PATCH 13/89] feat(apps): create_if_missing directory params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an opt-in per-parameter boolean create_if_missing (directory params only). When set, Fileglancer creates the resolved directory as the user, within an allowed file share, just before the submit-time existence check — so a home default like ~/.fileglancer/logs works on first launch and overrides to new directories are created too. Backend: model field + validator (directory-only), collect_creatable_dirs mirroring collect_path_parameters, a create_dirs setuid-worker action that enforces file-share containment before makedirs(exist_ok=True), and a submit_job step that dispatches create_dirs before validate_paths. Frontend type parity in shared.types.ts. Co-Authored-By: Claude Opus 4.8 --- fileglancer/apps/__init__.py | 1 + fileglancer/apps/command.py | 32 +++++++++++++++ fileglancer/apps/jobs.py | 15 +++++++ fileglancer/model.py | 15 +++++++ fileglancer/user_worker.py | 31 +++++++++++++++ frontend/src/shared.types.ts | 1 + tests/test_apps.py | 76 ++++++++++++++++++++++++++++++++++++ tests/test_worker.py | 41 +++++++++++++++++++ 8 files changed, 212 insertions(+) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index 61a68f8b..481306ba 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -16,6 +16,7 @@ _TOOL_REGISTRY, build_command, build_requirements_check, + collect_creatable_dirs, collect_path_parameters, expand_user_path, merge_requirements, diff --git a/fileglancer/apps/command.py b/fileglancer/apps/command.py index d7a161eb..c3993e4a 100644 --- a/fileglancer/apps/command.py +++ b/fileglancer/apps/command.py @@ -517,3 +517,35 @@ def collect_path_parameters(entry_point: AppEntryPoint, parameters: dict, continue result.append((param.key, param.name, str(value))) return result + + +def collect_creatable_dirs(entry_point: AppEntryPoint, parameters: dict, + env_parameters: dict = None) -> list[tuple[str, str]]: + """Collect effective directory values for params with create_if_missing set. + + Mirrors collect_path_parameters' effective-value logic (user value else + default, across both the env and pipeline namespaces) but returns only + directory params carrying create_if_missing, as (param_name, raw_value) + tuples. Raw (un-expanded) values are returned so the setuid worker resolves + '~' as the target user. See submit_job. + """ + env_parameters = env_parameters or {} + env_flat = _flatten_param_items(entry_point.env_parameters) + param_flat = _flatten_param_items(entry_point.parameters) + groups = ((env_flat, env_parameters), (param_flat, parameters)) + + result: list[tuple[str, str]] = [] + for flat, values in groups: + for param in flat: + if param.type != "directory" or not param.create_if_missing: + continue + if param.key in values: + value = values[param.key] + elif param.default is not None: + value = param.default + else: + continue + if value == "": + continue + result.append((param.name, str(value))) + return result diff --git a/fileglancer/apps/jobs.py b/fileglancer/apps/jobs.py index c988dfd1..04da2627 100644 --- a/fileglancer/apps/jobs.py +++ b/fileglancer/apps/jobs.py @@ -28,6 +28,7 @@ from fileglancer.apps.command import ( build_command, build_requirements_check, + collect_creatable_dirs, collect_path_parameters, expand_user_path, merge_requirements, @@ -624,6 +625,20 @@ async def submit_job( # as a service account that isn't in the user's groups, so a redundant # server-side check would wrongly reject (or, on local FS, wrongly accept) # paths the user can actually access. + # Create any directory params flagged create_if_missing first, as the user, + # so a home default like '~/.fileglancer/logs' exists by the time the + # existence check below runs. Containment (within a file share) is enforced + # in the worker before makedirs, so this never writes outside a share. + creatable_dirs = collect_creatable_dirs(entry_point, parameters, env_parameters) + if creatable_dirs: + paths_to_create = {str(i): value for i, (_, value) in enumerate(creatable_dirs)} + creation = await _dispatch(username, "create_dirs", paths=paths_to_create) + errors = (creation or {}).get("errors") or {} + if errors: + idx = min(int(i) for i in errors) + param_name, _ = creatable_dirs[idx] + raise ValueError(f"Parameter '{param_name}': {errors[str(idx)]}") + path_params = collect_path_parameters(entry_point, parameters, env_parameters) if path_params: paths_to_check = {str(i): value for i, (_, _, value) in enumerate(path_params)} diff --git a/fileglancer/model.py b/fileglancer/model.py index 4373d23a..700cabd2 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -340,6 +340,12 @@ class AppParameter(BaseModel): pattern: Optional[str] = Field(description="Regex validation pattern for string types", default=None) hidden: bool = Field(description="Whether the parameter is hidden by default in the UI", default=False) raw: bool = Field(description="If true, value is appended to the command without shell quoting", default=False) + create_if_missing: bool = Field( + description="directory params only: create the resolved directory (as the user, " + "within an allowed file share) before the existence check, so a home " + "default like '~/.fileglancer/logs' works on first launch", + default=False, + ) @field_validator("flag") @classmethod @@ -363,6 +369,15 @@ def stringify_options(cls, v): return v return [str(item) for item in v] + @model_validator(mode='after') + def validate_create_if_missing(self): + if self.create_if_missing and self.type != "directory": + raise ValueError( + f"create_if_missing is only valid on directory parameters, " + f"but '{self.name}' has type '{self.type}'" + ) + return self + class AppParameterSection(BaseModel): """A collapsible section that groups parameters in the UI""" diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index a588aa22..9213d21b 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -655,6 +655,37 @@ def _action_validate_paths(request: dict, ctx: WorkerContext) -> dict: return {"errors": errors} +@action("create_dirs") +def _action_create_dirs(request: dict, ctx: WorkerContext) -> dict: + """Create directories for app params with create_if_missing set. + + Runs as the target user in the setuid worker. Each path is expanded ('~' + resolves to this user's home), confirmed to be within an allowed file share + (containment only — the directory need not exist yet), and only then created + with exist_ok=True. Never creates anything outside a file share. Best-effort: + per-path failures are returned in {"errors": {index: message}} rather than + aborting the batch. + """ + from fileglancer.apps.command import validate_path_in_filestore + + paths = request["paths"] + fsps = ctx.db.get_file_share_paths() + errors = {} + for key, path_value in paths.items(): + # Containment check without the exists/readable check — the directory is + # about to be created, so it legitimately may not exist yet. + error = validate_path_in_filestore(path_value, fsps, check_access=False) + if error: + errors[key] = error + continue + expanded = os.path.expanduser(path_value.replace("\\", "/")) + try: + os.makedirs(expanded, exist_ok=True) + except OSError as e: + errors[key] = str(e) + return {"errors": errors} + + @action("get_profile") def _action_get_profile(request: dict, ctx: WorkerContext) -> dict: """Get user profile information.""" diff --git a/frontend/src/shared.types.ts b/frontend/src/shared.types.ts index 38b44d51..5fe47179 100644 --- a/frontend/src/shared.types.ts +++ b/frontend/src/shared.types.ts @@ -79,6 +79,7 @@ type AppParameter = { pattern?: string; hidden?: boolean; raw?: boolean; + create_if_missing?: boolean; }; type AppParameterSection = { diff --git a/tests/test_apps.py b/tests/test_apps.py index aa042779..133ab25d 100644 --- a/tests/test_apps.py +++ b/tests/test_apps.py @@ -26,6 +26,7 @@ _SERVICE_PORT_HELPER, _build_service_url_publisher, build_command, + collect_creatable_dirs, collect_path_parameters, expand_user_path, ) @@ -1178,6 +1179,81 @@ def test_omits_path_params_without_value_or_default(self): assert collect_path_parameters(ep, {}) == [] +class TestCreateIfMissingValidation: + """create_if_missing is only valid on directory params.""" + + def test_accepted_on_directory(self): + p = AppParameter(key="d", name="Dir", type="directory", create_if_missing=True) + assert p.create_if_missing is True + + def test_defaults_false(self): + p = AppParameter(key="d", name="Dir", type="directory") + assert p.create_if_missing is False + + @pytest.mark.parametrize("bad_type", ["file", "string", "integer", "enum"]) + def test_rejected_on_non_directory(self, bad_type): + kwargs = {"key": "p", "name": "P", "type": bad_type, "create_if_missing": True} + if bad_type == "enum": + kwargs["options"] = ["a", "b"] + with pytest.raises(ValidationError): + AppParameter(**kwargs) + + +class TestCollectCreatableDirs: + """collect_creatable_dirs gathers directory params flagged create_if_missing.""" + + def test_collects_via_default_and_user_value(self): + ep = AppEntryPoint( + id="test", + name="test", + command="test_cmd", + env_parameters=[{ + "key": "envdir", "name": "Env Dir", "type": "directory", + "default": "~/.fileglancer/env", "create_if_missing": True, + }], + parameters=[ + {"key": "logdir", "name": "Log Dir", "type": "directory", + "flag": "--logdir", "default": "~/.fileglancer/logs", + "create_if_missing": True}, + # directory without the flag -> excluded + {"key": "indir", "name": "In Dir", "type": "directory", + "flag": "--indir", "default": "/data/in"}, + ], + ) + result = collect_creatable_dirs( + ep, + {"logdir": "/data/mylogs"}, # user override + env_parameters={}, + ) + # Env default included; pipeline 'logdir' uses the user value; 'indir' + # excluded (no create_if_missing). + assert result == [ + ("Env Dir", "~/.fileglancer/env"), + ("Log Dir", "/data/mylogs"), + ] + + def test_omits_when_no_effective_value(self): + ep = AppEntryPoint( + id="test", + name="test", + command="test_cmd", + parameters=[{"key": "logdir", "name": "Log Dir", "type": "directory", + "flag": "--logdir", "create_if_missing": True}], + ) + assert collect_creatable_dirs(ep, {}) == [] + + def test_omits_empty_string_value(self): + ep = AppEntryPoint( + id="test", + name="test", + command="test_cmd", + parameters=[{"key": "logdir", "name": "Log Dir", "type": "directory", + "flag": "--logdir", "default": "~/logs", + "create_if_missing": True}], + ) + assert collect_creatable_dirs(ep, {"logdir": ""}) == [] + + class TestExpandUserPath: """expand_user_path normalizes file/dir param values consistently.""" diff --git a/tests/test_worker.py b/tests/test_worker.py index e1152df9..14aa5349 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -28,6 +28,7 @@ _recv, _ACTIONS, _action_validate_proxied_path, + _action_create_dirs, WorkerContext, _HEADER_FMT, _HEADER_SIZE, @@ -637,3 +638,43 @@ def test_accepts_symlink(self, tmp_path): result = _action_validate_proxied_path( {"fsp_name": "vpp_symlink", "path": "link.txt"}, ctx) assert result == {"ok": True} + + +class TestCreateDirsAction: + """create_dirs makes directories within a share, refusing anything outside.""" + + def _ctx(self, mount_path): + fsp = FileSharePath(zone="test", name="cd", mount_path=str(mount_path)) + return WorkerContext(username="test", db=_StubDb([fsp])) + + def test_creates_missing_directory(self, tmp_path): + ctx = self._ctx(tmp_path) + target = tmp_path / "logs" / "run1" + result = _action_create_dirs({"paths": {"0": str(target)}}, ctx) + assert result == {"errors": {}} + assert target.is_dir() + + def test_existing_directory_is_noop(self, tmp_path): + ctx = self._ctx(tmp_path) + target = tmp_path / "logs" + target.mkdir() + result = _action_create_dirs({"paths": {"0": str(target)}}, ctx) + assert result == {"errors": {}} + assert target.is_dir() + + def test_refuses_path_outside_any_share(self, tmp_path): + share = tmp_path / "share" + share.mkdir() + ctx = self._ctx(share) + outside = tmp_path / "outside" / "dir" + result = _action_create_dirs({"paths": {"0": str(outside)}}, ctx) + assert "0" in result["errors"] + assert not outside.exists() + + def test_expands_tilde_as_the_user(self, tmp_path, monkeypatch): + # Point HOME at a share so '~' resolves inside it. + monkeypatch.setenv("HOME", str(tmp_path)) + ctx = self._ctx(tmp_path) + result = _action_create_dirs({"paths": {"0": "~/.fileglancer/logs"}}, ctx) + assert result == {"errors": {}} + assert (tmp_path / ".fileglancer" / "logs").is_dir() From 43a57058bb574eb7022b300944c9e282d67ba5a3 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 08:41:21 -0400 Subject: [PATCH 14/89] feat(files): home default + toolbar for the folder-select dialog Two improvements to the file/folder selector dialog (useFileSelector + FileSelectorButton): 1. Start in the user's home directory when opened with no path yet (opt-in via defaultToHome, enabled on the app launch form's directory/file pickers), instead of the top-level zones list. 2. Add "go home", "new folder", and "show/hide dot files" buttons mirroring the browser toolbar. Show/hide dot files is a dialog-local override that follows the global preference until toggled and never writes it. New folder creates a directory in the current file share. Co-Authored-By: Claude Opus 4.8 --- .../components/ui/AppsPage/AppLaunchForm.tsx | 1 + .../ui/FileSelector/FileSelectorButton.tsx | 141 +++++++++++++++++- frontend/src/hooks/useFileSelector.ts | 107 ++++++++++--- 3 files changed, 224 insertions(+), 25 deletions(-) diff --git a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx index 5dce41d5..9b51e055 100644 --- a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx +++ b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx @@ -165,6 +165,7 @@ function ParameterField({ } /> { if (showDialog) { @@ -78,11 +105,41 @@ export default function FileSelectorButton({ } }, [showDialog, selectItem]); + const resetNewFolder = () => { + setShowNewFolder(false); + setNewFolderName(''); + }; + const onClose = () => { reset(); + resetNewFolder(); setShowDialog(false); }; + const handleCreateFolder = async (e: FormEvent) => { + e.preventDefault(); + if ( + !currentFilesystem || + !newFolderName.trim() || + !nameValidation.isValid + ) { + return; + } + const base = currentFilesystem.path === '.' ? '' : currentFilesystem.path; + try { + await createFolderMutation.mutateAsync({ + fspName: currentFilesystem.fspName, + folderPath: joinPaths(base, newFolderName.trim()) + }); + toast.success('New folder created!'); + resetNewFolder(); + } catch (err) { + toast.error( + `Error creating folder: ${err instanceof Error ? err.message : String(err)}` + ); + } + }; + const handleSelect = () => { if (state.selectedItem) { lastSelectedParentPath = getParentPath(state.selectedItem.displayPath); @@ -142,6 +199,80 @@ export default function FileSelectorButton({ zonesData={zonesQuery.data} /> + {/* Toolbar: go home, new folder, show/hide dot files */} +
+ { + resetNewFolder(); + navigateHome(); + }} + triggerClasses={toolbarBtnClasses} + /> + { + if (currentFilesystem) { + setShowNewFolder(prev => !prev); + } + }} + triggerClasses={toolbarBtnClasses} + /> + +
+ + {/* Inline new-folder form */} + {showNewFolder && currentFilesystem ? ( +
+
+ ) => + setNewFolderName(event.target.value) + } + placeholder="New folder name ..." + type="text" + value={newFolderName} + /> + {nameValidation.errorMessage ? ( + + {nameValidation.errorMessage} + + ) : null} +
+ + Create + + + Cancel + +
+ ) : null} + {/* Search input */}
(overrideKey ? ([overrideKey] as ['linux_path']) : pathPreference), [overrideKey, pathPreference] ); - // Initialize location based on initialLocation prop + // The user's home directory as a filesystem location, when known. + const homeLocation = useMemo(() => { + if (!profile?.homeFileSharePathName) { + return undefined; + } + return { + type: 'filesystem', + fspName: profile.homeFileSharePathName, + path: profile.homeDirectoryName || '.' + }; + }, [profile]); + + // Where the selector starts (and returns to on reset): an explicit + // initialLocation, else the home directory when defaultToHome is set, else + // the top-level zones list. + const defaultLocation = useMemo(() => { + if (initialLocation) { + return { + type: 'filesystem', + fspName: initialLocation.fspName, + path: initialLocation.path + }; + } + if (defaultToHome && homeLocation) { + return homeLocation; + } + return { type: 'zones' }; + }, [initialLocation, defaultToHome, homeLocation]); + + // Initialize location based on the resolved default location const [state, setState] = useState({ - currentLocation: initialLocation - ? { - type: 'filesystem', - fspName: initialLocation.fspName, - path: initialLocation.path - } - : { type: 'zones' }, + currentLocation: defaultLocation, selectedItem: null }); + // Local-only dot-file visibility for the dialog: null follows the global + // preference; once the user toggles inside the dialog it overrides locally + // without ever writing the global preference. + const [hideDotFilesOverride, setHideDotFilesOverride] = useState< + boolean | null + >(null); + const hideDotFilesEffective = hideDotFilesOverride ?? hideDotFiles; + const toggleHideDotFiles = useCallback(() => { + setHideDotFilesOverride(prev => !(prev ?? hideDotFiles)); + }, [hideDotFiles]); + + // If the profile (and thus home) resolves only after mount, apply the home + // default once, as long as the user hasn't already navigated away. + const appliedHomeRef = useRef(false); + useEffect(() => { + if (!defaultToHome || initialLocation || options?.initialPath) { + return; + } + if (appliedHomeRef.current || !homeLocation) { + return; + } + appliedHomeRef.current = true; + setState(prev => + prev.currentLocation.type === 'zones' + ? { currentLocation: homeLocation, selectedItem: null } + : prev + ); + }, [defaultToHome, initialLocation, options?.initialPath, homeLocation]); + const [searchQuery, setSearchQuery] = useState(''); const normalizedQuery = searchQuery.trim().toLowerCase(); @@ -234,7 +291,10 @@ export default function useFileSelector(options?: FileSelectorOptions) { return items; } else { // In filesystem mode, return files from query - const files = fileQuery.data?.files || []; + let files = fileQuery.data?.files || []; + if (hideDotFilesEffective) { + files = files.filter(item => !item.name.startsWith('.')); + } if (normalizedQuery) { return files.filter(item => item.name.toLowerCase().includes(normalizedQuery) @@ -249,7 +309,8 @@ export default function useFileSelector(options?: FileSelectorOptions) { fileQuery.data, isFilteredByGroups, profile, - normalizedQuery + normalizedQuery, + hideDotFilesEffective ]); // Navigation methods @@ -261,21 +322,23 @@ export default function useFileSelector(options?: FileSelectorOptions) { }); }, []); + // Jump to the user's home directory (no-op until the profile resolves). + const navigateHome = useCallback(() => { + if (homeLocation) { + navigateToLocation(homeLocation); + } + }, [homeLocation, navigateToLocation]); + // Reset to initial state (for when dialog is closed/cancelled) const reset = useCallback(() => { lastResolvedPath.current = undefined; setSearchQuery(''); + setHideDotFilesOverride(null); setState({ - currentLocation: initialLocation - ? { - type: 'filesystem', - fspName: initialLocation.fspName, - path: initialLocation.path - } - : { type: 'zones' }, + currentLocation: defaultLocation, selectedItem: null }); - }, [initialLocation]); + }, [defaultLocation]); // Select an item and generate its full filesystem path // If no item provided, selects the current folder/location @@ -405,6 +468,8 @@ export default function useFileSelector(options?: FileSelectorOptions) { fileQuery, zonesQuery: zonesAndFspQuery, navigateToLocation, + navigateHome, + canGoHome: homeLocation !== undefined, selectItem, handleItemDoubleClick, reset, @@ -412,6 +477,8 @@ export default function useFileSelector(options?: FileSelectorOptions) { handleSearchChange, clearSearch, isFilteredByGroups, - userHasGroups + userHasGroups, + hideDotFiles: hideDotFilesEffective, + toggleHideDotFiles }; } From 04b88b4c53b31829244b685aada941470c701837 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 09:12:09 -0400 Subject: [PATCH 15/89] fix(apps): don't fail pre-submit validation for create_if_missing dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The launch form validates directory paths against the worker before submitting, and that existence check rejected a create_if_missing default like ~/.fileglancer/tensorboard with "Path does not exist" — before submit_job's create_dirs step could create it. Pass the create_if_missing param keys through /api/apps/validate-paths so the worker validates those for file-share containment only (not existence). The directory is still created at submit time, and paths outside any share are still rejected early. Co-Authored-By: Claude Opus 4.8 --- fileglancer/model.py | 5 +++ fileglancer/server.py | 3 +- fileglancer/user_worker.py | 8 ++++- .../components/ui/AppsPage/AppLaunchForm.tsx | 9 ++++- frontend/src/queries/appsQueries.ts | 6 ++-- tests/test_worker.py | 35 +++++++++++++++++++ 6 files changed, 61 insertions(+), 5 deletions(-) diff --git a/fileglancer/model.py b/fileglancer/model.py index 700cabd2..59e0e966 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -883,6 +883,11 @@ def validate_container_args(cls, v): class PathValidationRequest(BaseModel): """Request to validate file/directory paths""" paths: Dict[str, str] = Field(description="Map of parameter key to path value") + create_if_missing: List[str] = Field( + default=[], + description="Keys whose directory may not exist yet (create_if_missing " + "params): validated for file-share containment only, not existence", + ) class PathValidationResponse(BaseModel): diff --git a/fileglancer/server.py b/fileglancer/server.py index 84306bf2..67d4b62c 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -1914,7 +1914,8 @@ async def update_user_app(body: ManifestFetchRequest, description="Validate file/directory paths for app parameters") async def validate_paths(body: PathValidationRequest, username: str = Depends(get_current_user)): - result = await _worker_exec(username, "validate_paths", paths=body.paths) + result = await _worker_exec(username, "validate_paths", paths=body.paths, + create_if_missing=body.create_if_missing) return PathValidationResponse(errors=result.get("errors", {})) # --- Catalog (shared apps) API --- diff --git a/fileglancer/user_worker.py b/fileglancer/user_worker.py index 9213d21b..2be0af10 100644 --- a/fileglancer/user_worker.py +++ b/fileglancer/user_worker.py @@ -646,10 +646,16 @@ def _action_validate_paths(request: dict, ctx: WorkerContext) -> dict: from fileglancer.apps.command import validate_path_in_filestore paths = request["paths"] + # Keys whose directory may not exist yet (create_if_missing params) are + # checked for file-share containment only — Fileglancer creates them at + # submit time, so a missing-but-in-share path is valid here. + create_if_missing = set(request.get("create_if_missing") or []) fsps = ctx.db.get_file_share_paths() errors = {} for param_key, path_value in paths.items(): - error = validate_path_in_filestore(path_value, fsps) + error = validate_path_in_filestore( + path_value, fsps, check_access=param_key not in create_if_missing + ) if error: errors[param_key] = error return {"errors": errors} diff --git a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx index 9b51e055..0b473f00 100644 --- a/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx +++ b/frontend/src/components/ui/AppsPage/AppLaunchForm.tsx @@ -972,6 +972,10 @@ export default function AppLaunchForm({ // Filter out undefined/empty values and normalize paths to Linux format const params: Record = {}; const pathParams: Record = {}; + // Directory params flagged create_if_missing are validated for file-share + // containment only — Fileglancer creates them at submit time, so their + // absence must not fail pre-submit validation. + const createIfMissingKeys: string[] = []; for (const [key, val] of Object.entries(values)) { if (val !== undefined && val !== null && val !== '') { const paramDef = paramDefs.get(key); @@ -991,6 +995,9 @@ export default function AppLaunchForm({ !normalized.startsWith('https://') ) { pathParams[key] = normalized; + if (paramDef.type === 'directory' && paramDef.create_if_missing) { + createIfMissingKeys.push(key); + } } } else { params[key] = val; @@ -1002,7 +1009,7 @@ export default function AppLaunchForm({ if (Object.keys(pathParams).length > 0) { setValidating(true); try { - const pathErrors = await validatePaths(pathParams); + const pathErrors = await validatePaths(pathParams, createIfMissingKeys); if (Object.keys(pathErrors).length > 0) { setErrors(prev => ({ ...prev, ...pathErrors })); setValidating(false); diff --git a/frontend/src/queries/appsQueries.ts b/frontend/src/queries/appsQueries.ts index eb04b118..c3a63cb0 100644 --- a/frontend/src/queries/appsQueries.ts +++ b/frontend/src/queries/appsQueries.ts @@ -127,10 +127,12 @@ export function useAddAppMutation(): UseMutationResult< } export async function validatePaths( - paths: Record + paths: Record, + createIfMissing: string[] = [] ): Promise> { const response = await sendFetchRequest('/api/apps/validate-paths', 'POST', { - paths + paths, + create_if_missing: createIfMissing }); const data = await getResponseJsonOrError(response); if (!response.ok) { diff --git a/tests/test_worker.py b/tests/test_worker.py index 14aa5349..24931164 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -29,6 +29,7 @@ _ACTIONS, _action_validate_proxied_path, _action_create_dirs, + _action_validate_paths, WorkerContext, _HEADER_FMT, _HEADER_SIZE, @@ -678,3 +679,37 @@ def test_expands_tilde_as_the_user(self, tmp_path, monkeypatch): result = _action_create_dirs({"paths": {"0": "~/.fileglancer/logs"}}, ctx) assert result == {"errors": {}} assert (tmp_path / ".fileglancer" / "logs").is_dir() + + +class TestValidatePathsAction: + """validate_paths checks existence, except for create_if_missing keys.""" + + def _ctx(self, mount_path): + fsp = FileSharePath(zone="test", name="vp", mount_path=str(mount_path)) + return WorkerContext(username="test", db=_StubDb([fsp])) + + def test_missing_dir_fails_by_default(self, tmp_path): + ctx = self._ctx(tmp_path) + missing = tmp_path / "logs" + result = _action_validate_paths({"paths": {"logdir": str(missing)}}, ctx) + assert "logdir" in result["errors"] + + def test_missing_dir_ok_when_create_if_missing(self, tmp_path): + ctx = self._ctx(tmp_path) + missing = tmp_path / "logs" + result = _action_validate_paths( + {"paths": {"logdir": str(missing)}, "create_if_missing": ["logdir"]}, + ctx, + ) + assert result == {"errors": {}} + + def test_create_if_missing_still_enforces_containment(self, tmp_path): + share = tmp_path / "share" + share.mkdir() + ctx = self._ctx(share) + outside = tmp_path / "outside" / "logs" + result = _action_validate_paths( + {"paths": {"logdir": str(outside)}, "create_if_missing": ["logdir"]}, + ctx, + ) + assert "logdir" in result["errors"] From 549cb2b58681cc9504b7ca6e3fc211db5d4e11c0 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 09:59:06 -0400 Subject: [PATCH 16/89] feat(files): tighten the folder-select dialog layout Default to hiding dot files in the dialog regardless of the global preference, move the filter onto the toolbar row, drop the bottom "Selected" display, widen the dialog, and scale the file-list height with the viewport so shorter screens shrink the scrollable list. Co-Authored-By: Claude Opus 4.8 --- .../ui/FileSelector/FileSelectorButton.tsx | 72 ++++++++----------- .../ui/FileSelector/FileSelectorTable.tsx | 17 ++--- frontend/src/hooks/useFileSelector.ts | 24 +++---- 3 files changed, 42 insertions(+), 71 deletions(-) diff --git a/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx b/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx index 27a4eb64..54c66f47 100644 --- a/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx +++ b/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx @@ -177,7 +177,7 @@ export default function FileSelectorButton({ {showDialog ? ( @@ -199,7 +199,7 @@ export default function FileSelectorButton({ zonesData={zonesQuery.data} /> - {/* Toolbar: go home, new folder, show/hide dot files */} + {/* Toolbar: go home, new folder, show/hide dot files, filter */}
+
+ + + + + + {searchQuery ? ( + + ) : null} +
{/* Inline new-folder form */} @@ -273,33 +296,9 @@ export default function FileSelectorButton({ ) : null} - {/* Search input */} -
- - - - - - {searchQuery ? ( - - ) : null} -
- - {/* Table with loading/error states */} -
+ {/* Table with loading/error states. Height scales with the + viewport so shorter screens shrink the (scrollable) file list. */} +
{zonesQuery.isPending ? (
) : null} - {/* Selected path display */} - -
- - Selected: - - {state.selectedItem ? ( - - {state.selectedItem.displayPath} - - ) : ( -
- )} -
- {/* Action buttons */}
diff --git a/frontend/src/components/ui/FileSelector/FileSelectorTable.tsx b/frontend/src/components/ui/FileSelector/FileSelectorTable.tsx index 26e4df3e..fa500256 100644 --- a/frontend/src/components/ui/FileSelector/FileSelectorTable.tsx +++ b/frontend/src/components/ui/FileSelector/FileSelectorTable.tsx @@ -50,21 +50,12 @@ export default function FileSelectorTable({ onItemClick, onItemDoubleClick }: FileSelectorTableProps) { - const { hideDotFiles, pathPreference } = usePreferencesContext(); + const { pathPreference } = usePreferencesContext(); const [sorting, setSorting] = useState([]); - const displayFiles = useMemo(() => { - let filtered = data; - - // Filter out dot files if preference is set - if (hideDotFiles) { - filtered = filtered.filter( - (file: FileOrFolder) => !file.name.startsWith('.') - ); - } - - return filtered; - }, [data, hideDotFiles]); + // Dot-file visibility is handled upstream by useFileSelector (which keeps a + // dialog-local override), so the incoming data is already filtered. + const displayFiles = data; const columns = useMemo[]>( () => [ diff --git a/frontend/src/hooks/useFileSelector.ts b/frontend/src/hooks/useFileSelector.ts index 15f3928d..d95393c1 100644 --- a/frontend/src/hooks/useFileSelector.ts +++ b/frontend/src/hooks/useFileSelector.ts @@ -47,8 +47,7 @@ type FileSelectorOptions = { export default function useFileSelector(options?: FileSelectorOptions) { const { zonesAndFspQuery } = useZoneAndFspMapContext(); - const { pathPreference, isFilteredByGroups, hideDotFiles } = - usePreferencesContext(); + const { pathPreference, isFilteredByGroups } = usePreferencesContext(); const { profile } = useProfileContext(); const initialLocation = options?.initialLocation; @@ -95,16 +94,13 @@ export default function useFileSelector(options?: FileSelectorOptions) { selectedItem: null }); - // Local-only dot-file visibility for the dialog: null follows the global - // preference; once the user toggles inside the dialog it overrides locally + // Local-only dot-file visibility for the dialog. Always starts hidden, + // ignoring the global preference; the user can toggle it within the dialog // without ever writing the global preference. - const [hideDotFilesOverride, setHideDotFilesOverride] = useState< - boolean | null - >(null); - const hideDotFilesEffective = hideDotFilesOverride ?? hideDotFiles; + const [hideDotFilesLocal, setHideDotFilesLocal] = useState(true); const toggleHideDotFiles = useCallback(() => { - setHideDotFilesOverride(prev => !(prev ?? hideDotFiles)); - }, [hideDotFiles]); + setHideDotFilesLocal(prev => !prev); + }, []); // If the profile (and thus home) resolves only after mount, apply the home // default once, as long as the user hasn't already navigated away. @@ -292,7 +288,7 @@ export default function useFileSelector(options?: FileSelectorOptions) { } else { // In filesystem mode, return files from query let files = fileQuery.data?.files || []; - if (hideDotFilesEffective) { + if (hideDotFilesLocal) { files = files.filter(item => !item.name.startsWith('.')); } if (normalizedQuery) { @@ -310,7 +306,7 @@ export default function useFileSelector(options?: FileSelectorOptions) { isFilteredByGroups, profile, normalizedQuery, - hideDotFilesEffective + hideDotFilesLocal ]); // Navigation methods @@ -333,7 +329,7 @@ export default function useFileSelector(options?: FileSelectorOptions) { const reset = useCallback(() => { lastResolvedPath.current = undefined; setSearchQuery(''); - setHideDotFilesOverride(null); + setHideDotFilesLocal(true); setState({ currentLocation: defaultLocation, selectedItem: null @@ -478,7 +474,7 @@ export default function useFileSelector(options?: FileSelectorOptions) { clearSearch, isFilteredByGroups, userHasGroups, - hideDotFiles: hideDotFilesEffective, + hideDotFiles: hideDotFilesLocal, toggleHideDotFiles }; } From 38f27f35dd8b8ade79b03cdca1ee90a1c3c81630 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 10:13:53 -0400 Subject: [PATCH 17/89] feat(files): added path display/input box to path selection dialog --- .../ui/FileSelector/FileSelectorButton.tsx | 46 ++++++++++++++++++- frontend/src/hooks/useFileSelector.ts | 37 +++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx b/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx index 54c66f47..e3cf07c8 100644 --- a/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx +++ b/frontend/src/components/ui/FileSelector/FileSelectorButton.tsx @@ -1,5 +1,5 @@ import { useState, useEffect } from 'react'; -import type { ChangeEvent, FormEvent, MouseEvent } from 'react'; +import type { ChangeEvent, FormEvent, KeyboardEvent, MouseEvent } from 'react'; import { IconButton, Input, Typography } from '@material-tailwind/react'; import { HiOutlineFolder, HiOutlineFunnel, HiXMark } from 'react-icons/hi2'; import { HiHome, HiFolderAdd, HiEye, HiEyeOff } from 'react-icons/hi'; @@ -60,6 +60,9 @@ export default function FileSelectorButton({ const [showDialog, setShowDialog] = useState(false); const [showNewFolder, setShowNewFolder] = useState(false); const [newFolderName, setNewFolderName] = useState(''); + // Editable path field: local while typing, kept in sync with the current + // selection/location so pasting a path can navigate the browser there. + const [pathInput, setPathInput] = useState(''); // Use initialPath if provided, otherwise fall back to last confirmed selection's parent const effectiveInitialPath = @@ -71,8 +74,10 @@ export default function FileSelectorButton({ fileQuery, zonesQuery, navigateToLocation, + navigateToRawPath, navigateHome, canGoHome, + currentPathDisplay, selectItem, handleItemDoubleClick, reset, @@ -105,6 +110,22 @@ export default function FileSelectorButton({ } }, [showDialog, selectItem]); + // Reflect the current selection/location in the editable path field. + useEffect(() => { + setPathInput(currentPathDisplay); + }, [currentPathDisplay]); + + const handlePathInputSubmit = () => { + const trimmed = pathInput.trim(); + if (!trimmed || trimmed === currentPathDisplay) { + return; + } + if (!navigateToRawPath(trimmed)) { + toast.error('No file share found for that path'); + setPathInput(currentPathDisplay); + } + }; + const resetNewFolder = () => { setShowNewFolder(false); setNewFolderName(''); @@ -350,6 +371,29 @@ export default function FileSelectorButton({
) : null} + {/* Editable path: paste a path here to navigate the browser there */} +
+ + Path + + ) => + setPathInput(event.target.value) + } + onKeyDown={(event: KeyboardEvent) => { + if (event.key === 'Enter') { + event.preventDefault(); + handlePathInputSubmit(); + } + }} + placeholder="Type or paste a path to navigate ..." + type="text" + value={pathInput} + /> +
+ {/* Action buttons */}
diff --git a/frontend/src/hooks/useFileSelector.ts b/frontend/src/hooks/useFileSelector.ts index d95393c1..ef29dce4 100644 --- a/frontend/src/hooks/useFileSelector.ts +++ b/frontend/src/hooks/useFileSelector.ts @@ -193,6 +193,20 @@ export default function useFileSelector(options?: FileSelectorOptions) { return (zonesAndFspQuery.data?.[fspKey] as FileSharePath) || null; }, [state.currentLocation, zonesAndFspQuery.data]); + // Path to show in the editable path field, in the user's preferred format: + // the selected item when there is one, otherwise the current folder. + const currentPathDisplay = useMemo(() => { + if (state.selectedItem) { + return state.selectedItem.displayPath; + } + if (state.currentLocation.type === 'filesystem' && currentFsp) { + const subPath = + state.currentLocation.path === '.' ? '' : state.currentLocation.path; + return getPreferredPathForDisplay(pathPreference, currentFsp, subPath); + } + return ''; + }, [state.selectedItem, state.currentLocation, currentFsp, pathPreference]); + // Build the items to display based on current location const displayItems = useMemo((): FileOrFolder[] => { if (zonesAndFspQuery.isPending || !zonesAndFspQuery.data) { @@ -318,6 +332,27 @@ export default function useFileSelector(options?: FileSelectorOptions) { }); }, []); + // Resolve a raw path (in any OS format, e.g. pasted by the user) to an FSP + // and navigate the browser into it. Returns false if no FSP matches. + const navigateToRawPath = useCallback( + (rawPath: string): boolean => { + if (!rawPath.trim() || !zonesAndFspQuery.data) { + return false; + } + const resolved = resolvePathToFsp(rawPath, zonesAndFspQuery.data); + if (!resolved) { + return false; + } + navigateToLocation({ + type: 'filesystem', + fspName: resolved.fsp.name, + path: resolved.subpath || '.' + }); + return true; + }, + [zonesAndFspQuery.data, navigateToLocation] + ); + // Jump to the user's home directory (no-op until the profile resolves). const navigateHome = useCallback(() => { if (homeLocation) { @@ -464,7 +499,9 @@ export default function useFileSelector(options?: FileSelectorOptions) { fileQuery, zonesQuery: zonesAndFspQuery, navigateToLocation, + navigateToRawPath, navigateHome, + currentPathDisplay, canGoHome: homeLocation !== undefined, selectItem, handleItemDoubleClick, From ca71ff1ca5fe7f565665584e1ed311ef85f24d28 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 2 Jul 2026 13:07:30 -0400 Subject: [PATCH 18/89] feat(apps): app detail page, card banner with actions menu, tabbed launch Unify the apps UX around a drill-down hierarchy (My Apps > app detail > entry point launch) with the apps tabs visible throughout: - App cards get a banner: entry-point type icon (server for services, terminal for jobs) and Shared tag on the left, Launch button and a vertical-ellipsis actions menu (Launch/View/Share|Unshare/Update/ Remove) on the right, replacing the corner icon buttons and bottom Launch button. The whole card is clickable and opens the detail page. - New app detail page replaces AppInfoDialog: info table, per-entry- point Launch buttons, visible Share/Unshare and Update buttons, and Remove tucked in the ellipsis menu. - Launch/relaunch routes move inside AppsLayout so the tabs stay visible; the "Back to Apps" button is replaced by a shared back-arrow + app-name header. Back arrows walk up one level: launch -> detail (when installed), detail -> My Apps. - My Apps tab stays highlighted on detail/launch/relaunch pages. - App action mutations + share/remove dialogs consolidated into a reusable useAppActions hook shared by the cards and detail page. Co-Authored-By: Claude Fable 5 --- frontend/src/App.tsx | 61 ++-- .../src/__tests__/unitTests/appUrls.test.ts | 13 + frontend/src/components/AppDetail.tsx | 209 +++++++++++++ frontend/src/components/AppLaunch.tsx | 35 ++- frontend/src/components/Apps.tsx | 102 +----- .../ui/AppsPage/AppActionDialogs.tsx | 32 ++ .../src/components/ui/AppsPage/AppCard.tsx | 230 +++++--------- .../components/ui/AppsPage/AppInfoDialog.tsx | 290 ------------------ .../components/ui/AppsPage/AppInfoTable.tsx | 49 +++ .../components/ui/AppsPage/AppPageHeader.tsx | 61 ++++ .../components/ui/AppsPage/ShareAppDialog.tsx | 126 ++++++++ .../components/ui/Menus/DataLinksActions.tsx | 10 +- frontend/src/hooks/useAppActions.ts | 139 +++++++++ frontend/src/layouts/AppsLayout.tsx | 25 +- frontend/src/utils/appIcons.ts | 18 ++ frontend/src/utils/appUrls.ts | 16 + frontend/src/utils/index.ts | 4 + 17 files changed, 821 insertions(+), 599 deletions(-) create mode 100644 frontend/src/components/AppDetail.tsx create mode 100644 frontend/src/components/ui/AppsPage/AppActionDialogs.tsx delete mode 100644 frontend/src/components/ui/AppsPage/AppInfoDialog.tsx create mode 100644 frontend/src/components/ui/AppsPage/AppInfoTable.tsx create mode 100644 frontend/src/components/ui/AppsPage/AppPageHeader.tsx create mode 100644 frontend/src/components/ui/AppsPage/ShareAppDialog.tsx create mode 100644 frontend/src/hooks/useAppActions.ts create mode 100644 frontend/src/utils/appIcons.ts diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 7b05f1eb..12b66b2e 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -10,6 +10,7 @@ import { OtherPagesLayout } from './layouts/OtherPagesLayout'; import AppsLayout from './layouts/AppsLayout'; import Login from '@/components/Login'; import Apps from '@/components/Apps'; +import AppDetail from '@/components/AppDetail'; import Catalog from '@/components/Catalog'; import AppJobs from '@/components/AppJobs'; import AppLaunch from '@/components/AppLaunch'; @@ -128,50 +129,26 @@ const AppComponent = () => { } index /> } path="catalog" /> } path="jobs" /> - {/* Render job detail inside the layout so the My Apps / App - Catalog / Jobs tabs stay visible when drilling into a job. */} + {/* Render job/app detail and launch inside the layout so the + My Apps / App Catalog / Jobs tabs stay visible when drilling + into a job or an app. */} } path="jobs/:jobId" /> + } path="detail/:owner/:repo" /> + } + path="launch/:owner/:repo/:branch/:entryPointId" + /> + } + path="launch/:owner/:repo/:branch" + /> + } path="launch/:owner/:repo" /> + } + path="relaunch/:owner/:repo/:branch/:entryPointId" + /> + } path="relaunch/:owner/:repo" /> - - - - } - path="apps/launch/:owner/:repo/:branch/:entryPointId" - /> - - - - } - path="apps/launch/:owner/:repo/:branch" - /> - - - - } - path="apps/launch/:owner/:repo" - /> - - - - } - path="apps/relaunch/:owner/:repo/:branch/:entryPointId" - /> - - - - } - path="apps/relaunch/:owner/:repo" - /> {tasksEnabled ? ( { ); }); + test('builds detail paths, omitting the default branch and empty manifest path', () => { + expect(buildAppDetailPath('https://github.com/org/tool', '')).toBe( + '/apps/detail/org/tool' + ); + expect( + buildAppDetailPath( + 'https://github.com/org/tool/tree/feature/my-tool', + 'apps/demo' + ) + ).toBe('/apps/detail/org/tool?branch=feature%2Fmy-tool&path=apps%2Fdemo'); + }); + test('builds relaunch paths with slash branches in the query string', () => { expect(buildRelaunchPath('org', 'tool', 'release/2026-06', 'run')).toBe( '/apps/relaunch/org/tool?branch=release%2F2026-06&entryPointId=run' diff --git a/frontend/src/components/AppDetail.tsx b/frontend/src/components/AppDetail.tsx new file mode 100644 index 00000000..2b1f03fe --- /dev/null +++ b/frontend/src/components/AppDetail.tsx @@ -0,0 +1,209 @@ +import { useNavigate, useParams, useSearchParams } from 'react-router'; +import { Card, Typography } from '@material-tailwind/react'; +import { HiOutlinePlay, HiOutlineRefresh } from 'react-icons/hi'; +import { HiOutlineEllipsisVertical } from 'react-icons/hi2'; +import { FaUsers, FaUsersSlash } from 'react-icons/fa6'; + +import AppActionDialogs from '@/components/ui/AppsPage/AppActionDialogs'; +import AppInfoTable from '@/components/ui/AppsPage/AppInfoTable'; +import AppPageHeader from '@/components/ui/AppsPage/AppPageHeader'; +import DataLinksActionsMenu from '@/components/ui/Menus/DataLinksActions'; +import type { MenuItem } from '@/components/ui/Menus/FgMenuItems'; +import FgButton from '@/components/designSystem/atoms/FgButton'; +import FgIcon from '@/components/designSystem/atoms/FgIcon'; +import FgTooltip from '@/components/ui/widgets/FgTooltip'; +import { useAppsQuery } from '@/queries/appsQueries'; +import { useAppActions } from '@/hooks/useAppActions'; +import { + buildGithubUrl, + canonicalGithubUrl, + getAppIconType, + getEntryPointIconType +} from '@/utils'; +import type { UserApp } from '@/shared.types'; + +export default function AppDetail() { + const { owner, repo } = useParams<{ owner: string; repo: string }>(); + const [searchParams] = useSearchParams(); + const navigate = useNavigate(); + const appsQuery = useAppsQuery(); + const actions = useAppActions({ onRemoved: () => navigate('/apps') }); + + const manifestPath = searchParams.get('path') || ''; + const branch = searchParams.get('branch') || 'main'; + const appUrl = buildGithubUrl(owner!, repo!, branch); + + // Match by canonical URL identity rather than exact string, since stored app + // URLs may carry cosmetic variations (".git" suffix, trailing slash, + // "/tree/main") that don't survive the round-trip through the route. + const app = appsQuery.data?.find( + a => + canonicalGithubUrl(a.url) === appUrl && a.manifest_path === manifestPath + ); + + if (appsQuery.isPending) { + return ( + + Loading app... + + ); + } + + if (appsQuery.isError) { + return ( +
+ Failed to load apps: {appsQuery.error?.message || 'Unknown error'} +
+ ); + } + + if (!app) { + return ( +
+ + + This app is not in your apps. It may have been removed. + + navigate('/apps')} variant="outline"> + Go to My Apps + +
+ ); + } + + const isShared = app.listing_id !== undefined && app.listing_id !== null; + const runnables = app.manifest?.runnables ?? []; + + const menuItems: MenuItem[] = [ + { + name: 'Remove', + action: a => actions.requestRemove(a), + color: 'text-error' + } + ]; + + return ( +
+ + {isShared ? ( + + void actions.unshare(app)} + size="sm" + variant="outline" + > + Unshare + + + ) : ( + + actions.requestShare(app)} + size="sm" + variant="outline" + > + Share to Catalog + + + )} + + void actions.update(app)} + size="sm" + variant="outline" + > + Update + + + {runnables.length === 0 ? ( + actions.launch(app)} + size="sm" + > + Launch + + ) : null} + + actionProps={app} + menuItems={menuItems} + triggerIcon={HiOutlineEllipsisVertical} + /> + + } + icon={getAppIconType(app.manifest)} + title={app.name} + > + {isShared ? ( + + Shared + + ) : null} + + +
+ + + {runnables.length > 0 ? ( + <> + + Entry Points + +
+ {runnables.map(ep => ( + +
+ + + {ep.name} + + {ep.type === 'service' ? ( + + Service + + ) : null} +
+ {ep.description ? ( + + {ep.description} + + ) : null} + actions.launch(app, ep.id)} + size="sm" + > + Launch + +
+ ))} +
+ + ) : null} +
+ + +
+ ); +} diff --git a/frontend/src/components/AppLaunch.tsx b/frontend/src/components/AppLaunch.tsx index c66856a5..8e09605b 100644 --- a/frontend/src/components/AppLaunch.tsx +++ b/frontend/src/components/AppLaunch.tsx @@ -7,15 +7,17 @@ import { } from 'react-router'; import { Card, Typography } from '@material-tailwind/react'; -import { - HiOutlineArrowLeft, - HiOutlineDownload, - HiOutlinePlay -} from 'react-icons/hi'; +import { HiOutlineDownload, HiOutlinePlay } from 'react-icons/hi'; import toast from 'react-hot-toast'; import AppLaunchForm from '@/components/ui/AppsPage/AppLaunchForm'; -import { buildGithubUrl, canonicalGithubUrl } from '@/utils'; +import AppPageHeader from '@/components/ui/AppsPage/AppPageHeader'; +import { + buildAppDetailPath, + buildGithubUrl, + canonicalGithubUrl, + getAppIconType +} from '@/utils'; import { useAppsQuery, useAddAppMutation, @@ -178,14 +180,16 @@ export default function AppLaunch() { return (
- navigate('/apps')} - variant="outline" - > - Back to Apps - + {/* Not-installed banner */} {!appsQuery.isPending && !isInstalled ? ( @@ -257,9 +261,6 @@ export default function AppLaunch() { /> ) : manifest ? (
- - {displayName} - {manifest.description ? ( {manifest.description} ) : null} diff --git a/frontend/src/components/Apps.tsx b/frontend/src/components/Apps.tsx index 039558d2..cbc66f63 100644 --- a/frontend/src/components/Apps.tsx +++ b/frontend/src/components/Apps.tsx @@ -6,86 +6,24 @@ import toast from 'react-hot-toast'; import AppCard from '@/components/ui/AppsPage/AppCard'; import AddAppDialog from '@/components/ui/AppsPage/AddAppDialog'; -import DeleteAppDialog from '@/components/ui/AppsPage/DeleteAppDialog'; +import AppActionDialogs from '@/components/ui/AppsPage/AppActionDialogs'; import { useAppsQuery, useAddAppMutation, - useDiscoverAppsMutation, - useUpdateAppMutation, - useRemoveAppMutation, - useShareAppMutation, - useUnshareListingMutation + useDiscoverAppsMutation } from '@/queries/appsQueries'; +import { useAppActions } from '@/hooks/useAppActions'; import FgButton from './designSystem/atoms/FgButton'; import FgExternalLink from '@/components/designSystem/atoms/FgExternalLink'; import { DOCS_BASE_URL } from '@/constants/docs'; -import type { UserApp } from '@/shared.types'; export default function Apps() { - const [deleteApp, setDeleteApp] = useState(null); const [showAddDialog, setShowAddDialog] = useState(false); const appsQuery = useAppsQuery(); const addAppMutation = useAddAppMutation(); const discoverAppsMutation = useDiscoverAppsMutation(); - const updateAppMutation = useUpdateAppMutation(); - const removeAppMutation = useRemoveAppMutation(); - const shareAppMutation = useShareAppMutation(); - const unshareListingMutation = useUnshareListingMutation(); - - const handleRequestRemove = (app: UserApp) => { - setDeleteApp(app); - }; - - const handleConfirmDelete = async () => { - if (!deleteApp) { - return; - } - try { - await removeAppMutation.mutateAsync({ - url: deleteApp.url, - manifest_path: deleteApp.manifest_path - }); - toast.success('App removed'); - setDeleteApp(null); - } catch (error) { - const message = - error instanceof Error ? error.message : 'Failed to remove app'; - toast.error(message); - } - }; - - const handleUpdateApp = async ({ - url, - manifest_path - }: { - url: string; - manifest_path: string; - }) => { - try { - await updateAppMutation.mutateAsync({ url, manifest_path }); - toast.success('App updated'); - } catch (error) { - const message = - error instanceof Error ? error.message : 'Failed to update app'; - toast.error(message); - } - }; - - const handleShare = async (params: { - url: string; - manifest_path: string; - name: string; - description: string; - }) => { - await shareAppMutation.mutateAsync({ - url: params.url, - manifest_path: params.manifest_path, - name: params.name, - description: params.description - }); - toast.success('Shared to catalog'); - }; + const actions = useAppActions(); const handleDiscover = (url: string) => discoverAppsMutation.mutateAsync(url); @@ -99,17 +37,6 @@ export default function Apps() { setShowAddDialog(false); }; - const handleUnshare = async (listingId: number) => { - try { - await unshareListingMutation.mutateAsync({ listing_id: listingId }); - toast.success('Removed from catalog'); - } catch (error) { - const message = - error instanceof Error ? error.message : 'Failed to unshare'; - toast.error(message); - } - }; - return (
@@ -144,20 +71,9 @@ export default function Apps() {
{appsQuery.data.map(app => ( handleRequestRemove(app)} - onShare={handleShare} - onUnshare={() => - app.listing_id !== undefined - ? handleUnshare(app.listing_id) - : undefined - } - onUpdate={handleUpdateApp} - removing={removeAppMutation.isPending} - sharing={shareAppMutation.isPending} - unsharing={unshareListingMutation.isPending} - updating={updateAppMutation.isPending} /> ))}
@@ -178,13 +94,7 @@ export default function Apps() { onDiscover={handleDiscover} open={showAddDialog} /> - setDeleteApp(null)} - onConfirm={handleConfirmDelete} - open={deleteApp !== null} - removing={removeAppMutation.isPending} - /> +
); } diff --git a/frontend/src/components/ui/AppsPage/AppActionDialogs.tsx b/frontend/src/components/ui/AppsPage/AppActionDialogs.tsx new file mode 100644 index 00000000..9f2908b5 --- /dev/null +++ b/frontend/src/components/ui/AppsPage/AppActionDialogs.tsx @@ -0,0 +1,32 @@ +import DeleteAppDialog from '@/components/ui/AppsPage/DeleteAppDialog'; +import ShareAppDialog from '@/components/ui/AppsPage/ShareAppDialog'; +import type { AppActions } from '@/hooks/useAppActions'; + +/** + * The confirmation/share dialogs backing the two-step app actions from + * `useAppActions`. Render once per page that uses the hook. + */ +export default function AppActionDialogs({ + actions +}: { + readonly actions: AppActions; +}) { + return ( + <> + + + + ); +} diff --git a/frontend/src/components/ui/AppsPage/AppCard.tsx b/frontend/src/components/ui/AppsPage/AppCard.tsx index b214a059..4e0701a4 100644 --- a/frontend/src/components/ui/AppsPage/AppCard.tsx +++ b/frontend/src/components/ui/AppsPage/AppCard.tsx @@ -1,174 +1,114 @@ -import { useState } from 'react'; -import { useNavigate } from 'react-router'; +import type { KeyboardEvent } from 'react'; -import { Card, IconButton, Typography } from '@material-tailwind/react'; -import { buildLaunchPathFromApp } from '@/utils'; -import { - HiOutlineInformationCircle, - HiOutlinePlay, - HiOutlineTrash -} from 'react-icons/hi'; -import { FaUsers, FaUsersSlash } from 'react-icons/fa6'; +import { Card, Typography } from '@material-tailwind/react'; +import { HiOutlinePlay } from 'react-icons/hi'; +import { HiOutlineEllipsisVertical } from 'react-icons/hi2'; -import AppInfoDialog from '@/components/ui/AppsPage/AppInfoDialog'; +import DataLinksActionsMenu from '@/components/ui/Menus/DataLinksActions'; +import type { MenuItem } from '@/components/ui/Menus/FgMenuItems'; +import FgButton from '@/components/designSystem/atoms/FgButton'; import FgIcon from '@/components/designSystem/atoms/FgIcon'; import FgTooltip from '@/components/ui/widgets/FgTooltip'; +import type { AppActions } from '@/hooks/useAppActions'; +import { getAppIconType } from '@/utils'; import type { UserApp } from '@/shared.types'; -import FgButton from '@/components/designSystem/atoms/FgButton'; interface AppCardProps { readonly app: UserApp; - readonly onRemove: () => void; - readonly onUpdate: (params: { url: string; manifest_path: string }) => void; - readonly onShare: (params: { - url: string; - manifest_path: string; - name: string; - description: string; - }) => Promise; - readonly onUnshare: () => void; - readonly removing: boolean; - readonly updating: boolean; - readonly sharing: boolean; - readonly unsharing: boolean; + readonly actions: AppActions; } -export default function AppCard({ - app, - onRemove, - onUpdate, - onShare, - onUnshare, - removing, - updating, - sharing, - unsharing -}: AppCardProps) { - const navigate = useNavigate(); - const [infoOpen, setInfoOpen] = useState(false); - const [startInShareView, setStartInShareView] = useState(false); - +export default function AppCard({ app, actions }: AppCardProps) { const isShared = app.listing_id !== undefined && app.listing_id !== null; - const handleLaunch = () => { - navigate(buildLaunchPathFromApp(app.url, app.manifest_path)); - }; + const menuItems: MenuItem[] = [ + { name: 'Launch', action: a => actions.launch(a) }, + { name: 'View', action: a => actions.view(a) }, + { + name: 'Share to Catalog', + action: a => actions.requestShare(a), + shouldShow: !isShared + }, + { + name: 'Unshare', + action: a => void actions.unshare(a), + shouldShow: isShared + }, + { name: 'Update', action: a => void actions.update(a) }, + { + name: 'Remove', + action: a => actions.requestRemove(a), + color: 'text-error' + } + ]; - const handleInfo = () => { - setStartInShareView(false); - setInfoOpen(true); - }; + const handleView = () => actions.view(app); - const handleShare = () => { - setStartInShareView(true); - setInfoOpen(true); + const handleKeyDown = (event: KeyboardEvent) => { + if ( + (event.key === 'Enter' || event.key === ' ') && + event.target === event.currentTarget + ) { + event.preventDefault(); + handleView(); + } }; return ( - -
- - {app.name} - -
- - - - - + +
+
+ {isShared ? ( - - - - - - ) : ( - - - - - - )} - - + Shared + + ) : null} +
+
e.stopPropagation()} + > + + actions.launch(app)} size="sm" - variant="ghost" > - - + Launch + + + actionProps={app} + menuItems={menuItems} + triggerIcon={HiOutlineEllipsisVertical} + />
- {isShared ? ( -
- - Shared - -
- ) : null} - - {app.description ? ( - - {app.description} +
+ + {app.name} - ) : null} - - - Launch - - setInfoOpen(false)} - onLaunch={() => { - setInfoOpen(false); - handleLaunch(); - }} - onRemove={() => { - setInfoOpen(false); - onRemove(); - }} - onShare={onShare} - onUnshare={onUnshare} - onUpdate={() => - onUpdate({ url: app.url, manifest_path: app.manifest_path }) - } - open={infoOpen} - removing={removing} - sharing={sharing} - startInShareView={startInShareView} - unsharing={unsharing} - updating={updating} - /> + {app.description ? ( + + {app.description} + + ) : null} +
); } diff --git a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx b/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx deleted file mode 100644 index 66cb0968..00000000 --- a/frontend/src/components/ui/AppsPage/AppInfoDialog.tsx +++ /dev/null @@ -1,290 +0,0 @@ -import { useEffect, useState } from 'react'; -import { Typography } from '@material-tailwind/react'; -import { - HiOutlinePlay, - HiOutlineRefresh, - HiOutlineTrash -} from 'react-icons/hi'; -import { FaUsers, FaUsersSlash } from 'react-icons/fa6'; - -import FgDialog from '@/components/ui/Dialogs/FgDialog'; -import type { UserApp } from '@/shared.types'; -import FgButton from '@/components/designSystem/atoms/FgButton'; -import FgExternalLink from '@/components/designSystem/atoms/FgExternalLink'; -import FgTooltip from '@/components/ui/widgets/FgTooltip'; -import { parseGithubUrl } from '@/utils/appUrls'; - -/** - * The revision actually cloned, parsed out of the canonical app URL (which - * always carries it). Falls back to the requested branch, then null. - */ -function appRevision(app: UserApp): string | null { - try { - return parseGithubUrl(app.url).branch; - } catch { - return app.branch || null; - } -} - -interface AppInfoDialogProps { - readonly app: UserApp; - readonly open: boolean; - readonly onClose: () => void; - readonly onLaunch: () => void; - readonly onUpdate: () => void; - readonly onRemove: () => void; - readonly onShare: (params: { - url: string; - manifest_path: string; - name: string; - description: string; - }) => Promise; - readonly onUnshare: () => void; - readonly updating: boolean; - readonly removing: boolean; - readonly sharing: boolean; - readonly unsharing: boolean; - readonly startInShareView?: boolean; -} - -function AppInfoTable({ app }: { readonly app: UserApp }) { - const labelClass = - 'text-foreground font-medium pr-4 py-1.5 align-top whitespace-nowrap'; - const valueClass = 'text-foreground py-1.5'; - const revision = appRevision(app); - - return ( - - - - - - - {revision ? ( - - - - - ) : null} - {app.description ? ( - - - - - ) : null} - {app.manifest?.runnables && app.manifest.runnables.length > 0 ? ( - - - - - ) : null} - -
URL - - {app.url} - -
Revision{revision}
Description{app.description}
Entry Points - {app.manifest.runnables.map(ep => ep.name).join(', ')} -
- ); -} - -export default function AppInfoDialog({ - app, - open, - onClose, - onLaunch, - onUpdate, - onRemove, - onShare, - onUnshare, - updating, - removing, - sharing, - unsharing, - startInShareView = false -}: AppInfoDialogProps) { - const isShared = app.listing_id !== undefined && app.listing_id !== null; - - // The share form is shown as an inline view within this dialog (rather than a - // separate stacked dialog) so the dialog stays open throughout sharing. - const [shareView, setShareView] = useState(false); - const [name, setName] = useState(''); - const [description, setDescription] = useState(''); - const [shareError, setShareError] = useState(''); - - const openShareForm = () => { - setName(app.name); - setDescription(app.description ?? ''); - setShareError(''); - setShareView(true); - }; - - // Open into the share form when requested (and shareable), otherwise always - // return to the info view when the dialog is (re)opened. - useEffect(() => { - if (open) { - if (startInShareView && !isShared) { - openShareForm(); - } else { - setShareView(false); - } - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [open]); - - const handleShareSubmit = async () => { - if (!name.trim()) { - setShareError('Name is required'); - return; - } - try { - await onShare({ - url: app.url, - manifest_path: app.manifest_path, - name: name.trim(), - description: description.trim() - }); - setShareView(false); - } catch (e) { - setShareError(e instanceof Error ? e.message : 'Failed to share'); - } - }; - - return ( - - {shareView ? ( - <> - - Share to Catalog - - - Publish this app so other users can add it to their own collection. - You can customize the name and description before sharing without - affecting your own copy. - - -
- - { - setName(e.target.value); - setShareError(''); - }} - type="text" - value={name} - /> -
- -
- -