Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions create-a-container/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ deps:
npm --prefix client ci

build: deps
# Bake the release version into the client bundle (vite define, see
# client/vite.config.ts) — the deployed app has no git checkout to ask.
npm --prefix client pkg set version="$$(../package-version node)"
npm --prefix client run build

# Run the Manager locally against SQLite, with a dummy (mock) hypervisor so
Expand Down
5 changes: 0 additions & 5 deletions create-a-container/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,6 @@ function buildApp({
const { csrfGuard, jsonErrorHandler } = require('./middlewares/api');
app.use(csrfGuard);

// Set version info once at startup in app.locals
// Note: Version info is cached at startup. Server restart required to update version.
const { getVersionInfo } = require('./utils');
app.locals.versionInfo = getVersionInfo();

// --- Mount Routers ---
const apiV1Router = require('./routers/api/v1');

Expand Down
52 changes: 52 additions & 0 deletions create-a-container/client/src/app/AppFooter.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { useLocation } from 'react-router';
import { Bug } from 'lucide-react';
import { useSession } from '@/lib/auth';

const REPO_URL = 'https://github.com/mieweb/opensource-server';

// Baked in at build time (vite define): the packaging build writes the release
// version into package.json before `vite build`; dev builds keep 0.0.0.
const VERSION = __APP_VERSION__ === '0.0.0' ? null : __APP_VERSION__;

/**
* App-wide footer showing the running version (linked to the GitHub releases)
* and a "Report a bug" link that pre-fills the GitHub bug-report template with
* the current URL, username, and version.
*/
export function AppFooter() {
const { data: session } = useSession();
const location = useLocation();

const params = new URLSearchParams({ template: 'bug_report.yml', url: location.pathname });
if (session?.user) params.set('username', session.user);
params.set('version', VERSION ?? 'dev');
const bugReportUrl = `${REPO_URL}/issues/new?${params.toString()}`;

return (
<footer className="flex flex-wrap items-center justify-center gap-x-4 gap-y-1 border-t border-(--color-border,#e5e7eb) px-4 py-2 text-xs text-(--color-muted,#6b7280)">
{VERSION ? (
<a
href={`${REPO_URL}/releases`}
target="_blank"
rel="noopener noreferrer"
className="hover:underline"
aria-label={`Version ${VERSION} — view releases on GitHub`}
>
Version {VERSION}
</a>
) : (
<span>Development build</span>
)}
<a
href={bugReportUrl}
target="_blank"
rel="noopener noreferrer"
className="inline-flex items-center gap-1 hover:underline"
aria-label="Report a bug on GitHub"
>
<Bug className="size-3.5" aria-hidden="true" />
<span>Report a bug</span>
</a>
</footer>
);
}
2 changes: 2 additions & 0 deletions create-a-container/client/src/app/AppLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Sidebar, CommandPalette } from '@mieweb/ui';
import { AppSidebar } from './Sidebar';
import { AppTopHeader } from './Header';
import { AppBanner } from './Banner';
import { AppFooter } from './AppFooter';

export function AppLayout() {
return (
Expand All @@ -16,6 +17,7 @@ export function AppLayout() {
<main className="flex-1 overflow-y-auto overflow-x-hidden px-4 py-6 sm:px-6 lg:px-8">
<Outlet />
</main>
<AppFooter />
</div>
<CommandPalette placeholder="Search…" />
</div>
Expand Down
113 changes: 104 additions & 9 deletions create-a-container/client/src/pages/containers/ContainerFormPage.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import { useEffect, useMemo, useRef, useState } from 'react';
import { useNavigate, useParams } from 'react-router';
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';
import { useFieldArray, useForm } from 'react-hook-form';
Expand All @@ -14,6 +14,12 @@ import {
CardHeader,
CardTitle,
Input,
Modal,
ModalBody,
ModalClose,
ModalFooter,
ModalHeader,
ModalTitle,
Select,
Spinner,
Switch,
Expand Down Expand Up @@ -183,6 +189,28 @@ export function ContainerFormPage() {
const debouncedHostname = useDebouncedValue(hostname || '', 500);
const customTemplate = watch('customTemplate');
const collaborators = watch('collaborators') || [];
const watchedEntrypoint = watch('entrypoint');
const watchedEnvVars = watch('environmentVars');

// True when the form's env vars or entrypoint differ from the saved
// container — the changes that only take effect after a restart (#449).
const requiresRestart = useMemo(() => {
if (!isEdit || !container) return false;
if ((watchedEntrypoint || '') !== (container.entrypoint || '')) return true;
const saved = new Map(Object.entries(container.environmentVars || {}));
const current = (watchedEnvVars || []).filter((e) => e.key.trim());
if (current.length !== saved.size) return true;
return current.some((e) => saved.get(e.key) !== e.value);
}, [isEdit, container, watchedEntrypoint, watchedEnvVars]);

// The restart toggle follows restart-requiring edits (auto-on, so the user
// isn't left wondering why changes didn't apply) until the user overrides
// it manually — then their choice wins.
const restartTouchedRef = useRef(false);
useEffect(() => {
if (!isEdit || restartTouchedRef.current) return;
setValue('restart', requiresRestart);
}, [requiresRestart, isEdit, setValue]);

useEffect(() => {
if (container && isEdit && !initializedRef.current) {
Expand Down Expand Up @@ -226,6 +254,10 @@ export function ContainerFormPage() {

const [metadataMsg, setMetadataMsg] = useState<string | null>(null);
const [nvidiaTooltipOpen, setNvidiaTooltipOpen] = useState(false);
// Restart confirmation (issue #449): saving never restarts the container
// until the user explicitly confirms in this modal.
const [confirmRestartOpen, setConfirmRestartOpen] = useState(false);
const pendingValuesRef = useRef<FormData | null>(null);
const metadataMutation = useMutation({
mutationFn: (image: string) => queries.containerMetadata(siteId!, image),
onSuccess: (meta: ContainerMetadata) => {
Expand Down Expand Up @@ -326,6 +358,7 @@ export function ContainerFormPage() {
containerId: number;
jobId: number | null;
message: string;
pendingRestart?: boolean;
dnsWarnings: string[];
};
type SaveResult = UpdateResult | ContainerCreateResult;
Expand All @@ -337,7 +370,10 @@ export function ContainerFormPage() {
},
onSuccess: (result) => {
const dnsWarnings = (result as { dnsWarnings?: string[] }).dnsWarnings;
toast.success(isEdit ? 'Container updated' : 'Container queued for creation');
// Prefer the server's message so update-status wording (restarting /
// pending restart / updated) lives in one place.
const message = (result as { message?: string }).message;
toast.success(message || (isEdit ? 'Container updated' : 'Container queued for creation'));
// exact:true so we only invalidate the list query and not its prefix
// descendants (e.g. the still-mounted containerBootstrap query keyed
// ['sites', siteId, 'containers', 'new']), which would otherwise refetch
Expand All @@ -362,6 +398,17 @@ export function ContainerFormPage() {
},
});

// Saving with restart enabled must be confirmed first — a restart is
// disruptive and should never happen from a plain save (issue #449).
const onSubmit = (values: FormData) => {
if (isEdit && values.restart) {
pendingValuesRef.current = values;
setConfirmRestartOpen(true);
return;
}
mutation.mutate(values);
};

if ((isEdit && containerLoading) || bootstrapLoading) {
return (
<div className="flex justify-center p-12">
Expand All @@ -381,7 +428,7 @@ export function ContainerFormPage() {
];

return (
<form onSubmit={handleSubmit((v) => mutation.mutate(v))} noValidate>
<form onSubmit={handleSubmit(onSubmit)} noValidate>
<div className="mx-auto flex w-full max-w-4xl flex-col gap-6">
<FormPageHeader
icon={<Container className="size-6" />}
Expand Down Expand Up @@ -550,12 +597,25 @@ export function ContainerFormPage() {
</Tooltip>
</div>
{isEdit && (
<Switch
label="Restart container after saving"
description="Required if you change environment variables or entrypoint"
checked={!!restart}
onCheckedChange={(c) => setValue('restart', c)}
/>
<div className="flex flex-col gap-2">
<Switch
label="Restart container after saving"
description="Turns on automatically when you change environment variables or the entrypoint — those changes only take effect after a restart."
checked={!!restart}
onCheckedChange={(c) => {
restartTouchedRef.current = true;
setValue('restart', c);
}}
/>
{requiresRestart && !restart && (
<Alert variant="warning">
<AlertDescription>
This change requires a restart, but none will be performed — it takes
effect the next time the container restarts.
</AlertDescription>
</Alert>
)}
</div>
)}
</CardContent>
</Card>
Expand Down Expand Up @@ -823,6 +883,41 @@ export function ContainerFormPage() {
</Alert>
)}
</div>

<Modal open={confirmRestartOpen} onOpenChange={setConfirmRestartOpen}>
<ModalHeader>
<ModalTitle>Restart container?</ModalTitle>
<ModalClose />
</ModalHeader>
<ModalBody>
<p className="text-sm">
Saving will stop and start <strong>{container?.hostname}</strong>, interrupting
anything currently running in it.
</p>
</ModalBody>
<ModalFooter>
<Button
type="button"
variant="ghost"
className="cursor-pointer"
onClick={() => setConfirmRestartOpen(false)}
>
Cancel
</Button>
<Button
type="button"
variant="primary"
className="cursor-pointer"
isLoading={mutation.isPending}
onClick={() => {
if (pendingValuesRef.current) mutation.mutate(pendingValuesRef.current);
setConfirmRestartOpen(false);
}}
>
Save &amp; restart
</Button>
</ModalFooter>
</Modal>
</form>
);
}
9 changes: 8 additions & 1 deletion create-a-container/client/src/pages/jobs/JobDetailPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
import { ArrowLeft, Terminal } from 'lucide-react';
import { ButtonLink } from '@/components/ButtonLink';
import { ApiError } from '@/lib/api';
import { useCurrentSiteId } from '@/lib/currentSite';
import { keys, queries } from '@/lib/queries';
import type { JobStatusRow } from '@/lib/types';

Expand All @@ -37,6 +38,10 @@ function statusVariant(s: string): 'default' | 'success' | 'warning' | 'danger'

export function JobDetailPage() {
const { id } = useParams<{ id: string }>();
// /jobs/:id has no parent list route — Back returns to the current site's
// containers (where jobs are launched from), or the sites list as a fallback.
const currentSiteId = useCurrentSiteId();
const backTo = currentSiteId ? `/sites/${currentSiteId}/containers` : '/sites';
const { data: job, isLoading, error, refetch } = useQuery({
queryKey: keys.job(id!),
queryFn: () => queries.getJob(id!),
Expand Down Expand Up @@ -113,7 +118,9 @@ export function JobDetailPage() {
subtitle={job.command}
icon={<Terminal className="size-6" />}
actions={
<ButtonLink as={Link} to=".." relative="path" variant="ghost" leftIcon={<ArrowLeft className="size-4" />}>Back</ButtonLink>
<ButtonLink as={Link} to={backTo} variant="ghost" leftIcon={<ArrowLeft className="size-4" />}>
{currentSiteId ? 'Back to containers' : 'Back to sites'}
</ButtonLink>
}
/>

Expand Down
3 changes: 3 additions & 0 deletions create-a-container/client/src/vite-env.d.ts
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
/// <reference types="vite/client" />

/** App version baked in at build time (vite define); 0.0.0 in dev builds. */
declare const __APP_VERSION__: string;
9 changes: 9 additions & 0 deletions create-a-container/client/vite.config.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
import { defineConfig } from 'vite';
import react from '@vitejs/plugin-react';
import tailwindcss from '@tailwindcss/vite';
import { readFileSync } from 'node:fs';
import path from 'node:path';

const EXPRESS_TARGET = process.env.VITE_API_TARGET || 'http://localhost:3000';

// Baked-in app version for the footer: the packaging build sets package.json's
// version from the release tag (create-a-container/Makefile `build`); dev
// builds keep 0.0.0, which the UI renders as a development build.
const pkg = JSON.parse(readFileSync(path.resolve(__dirname, 'package.json'), 'utf8'));

export default defineConfig({
plugins: [react(), tailwindcss()],
define: {
__APP_VERSION__: JSON.stringify(pkg.version || '0.0.0'),
},
resolve: {
alias: {
'@': path.resolve(__dirname, './src'),
Expand Down
5 changes: 3 additions & 2 deletions create-a-container/openapi.v1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ paths:
put:
operationId: update_container
tags: [Containers]
summary: Update services/env/entrypoint; enqueues a restart job when needed (owner/admin)
summary: Update services/env/entrypoint; enqueues a restart job only when explicitly requested (owner/admin)
requestBody:
content:
application/json:
Expand All @@ -860,7 +860,7 @@ paths:
items: { $ref: '#/components/schemas/EnvVar' }
description: Full replacement set. Omitting it clears all user env vars (unless the request is restart-only).
entrypoint: { type: string, nullable: true, description: Omitting/blank clears the entrypoint (unless the request is restart-only) }
restart: { type: boolean, description: 'true forces a restart even with no config changes; alone, it performs a restart-only request' }
restart: { type: boolean, description: 'A restart job is enqueued only when true — config changes alone never restart the container (they apply on the next restart); alone, it performs a restart-only request' }
responses:
'200':
description: Updated, optional restart job
Expand All @@ -875,6 +875,7 @@ paths:
containerId: { type: integer }
jobId: { type: integer, nullable: true, description: 'Restart job id, when a restart was enqueued' }
dnsWarnings: { type: array, items: { type: string } }
pendingRestart: { type: boolean, description: 'true when env/entrypoint changes were saved but no restart was requested — they apply on the next restart' }
message: { type: string }
Comment on lines 875 to 879
'400': { $ref: '#/components/responses/BadRequest' }
'403': { description: 'forbidden — only the owner/admin may edit (collaborators have a read-only view); non-admins may not reassign ownership', content: { application/json: { schema: { $ref: '#/components/schemas/Error' } } } }
Expand Down
13 changes: 11 additions & 2 deletions create-a-container/routers/api/v1/containers.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,10 @@ router.put(
const ownerChanged = newOwnerUsername !== null && newOwnerUsername !== container.username;
const envChanged = !isRestartOnly && container.environmentVars !== envVarsJson;
const entrypointChanged = !isRestartOnly && container.entrypoint !== newEntrypoint;
const needsRestart = forceRestart || envChanged || entrypointChanged;
// Never restart implicitly (issue #449): a restart job is enqueued only
// when the caller explicitly asks for one. Saved env/entrypoint changes
// are applied by reconfigure-container.js on the next restart.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know this is doing exactly what the ticket asked for but I don't fully agree with the logic. The real problem with the implicit restart is that if, for example, the admin changed the default container vars, that would queue a restart regardless of the change the user made (possibly even if they made no changes). In reality, the implicit restart is good if the user intentionally changes entrypoint/environment because otherwise they may be left to wonder why their changes didn't take effect. Really there's 2 problems here:

  1. Invisible (to the user) changes by the admin can make a restart nessecary
  2. The user is not properly informed of changes requiring a restart

We can hook into this new solution to fix both of these, but we need one more layer on top. We keep this new "no restarts by default" logic to keep admin changes from bleeding into the container lifecycle behavior BUT if the user makes environment or entrypoint changes (or anything else requiring a restart) then some sort of noticable but unobtrusive UI affordance informs the user that this change requires a restart and the "restart" toggle is switched on. The user can still switch this off manually afterwards but another warning (under the toggle maybe) "this change requires a restart but a restart will not be performed".

const needsRestart = forceRestart;

let restartJob = null;
const dnsWarnings = [];
Expand Down Expand Up @@ -770,11 +773,17 @@ router.put(
}
}

const pendingRestart = !restartJob && (envChanged || entrypointChanged);
return ok(res, {
containerId: container.id,
jobId: restartJob ? restartJob.id : null,
dnsWarnings,
message: restartJob ? 'Container is restarting' : 'Container updated',
pendingRestart,
message: restartJob
? 'Container is restarting'
: pendingRestart
? 'Container updated — changes take effect on the next restart'
: 'Container updated',
});
}),
);
Expand Down
Loading
Loading