Skip to content
Merged
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
125 changes: 118 additions & 7 deletions apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
ArrowUp,
Camera,
Circle,
Download,
FolderOpen,
GripHorizontal,
MapPin,
Plus,
Expand All @@ -15,11 +17,23 @@ import {
} from "lucide-react";
import { useRef, useState } from "react";
import { useTranslation } from "react-i18next";
import { saveBinaryFileWithFallback } from "../../lib/tauri-io";
import {
openLocalDataFileWithFallback,
saveBinaryFileWithFallback,
saveTextFileWithFallback,
} from "../../lib/tauri-io";
import {
DEFAULT_FPS,
DEFAULT_SEGMENT_SECONDS,
estimateTourDurationMs,
isTourRecordingSupported,
MAX_FPS,
MAX_SEGMENT_SECONDS,
MIN_FPS,
MIN_SEGMENT_SECONDS,
parseTourConfig,
recordTour,
serializeTourConfig,
type TourKeyframe,
TourRecordingUnsupportedError,
} from "../../lib/tour-recorder";
Expand All @@ -35,12 +49,10 @@ interface RecordTourDialogProps {
type Status = "idle" | "recording" | "ready" | "saving";

const DEFAULT_FILE_NAME = "map-tour";
const DEFAULT_FPS = 30;
const MIN_FPS = 10;
const MAX_FPS = 60;
const DEFAULT_SEGMENT_SECONDS = 4;
const MIN_SEGMENT_SECONDS = 0.5;
const MAX_SEGMENT_SECONDS = 30;
// Default leaf name for the saved tour *configuration* (the editable JSON),
// distinct from the recorded video's name so the two exports are easy to tell
// apart in a downloads folder.
const DEFAULT_CONFIG_FILE_NAME = "map-tour-setup";

function createId(): string {
if (typeof crypto !== "undefined" && "randomUUID" in crypto) {
Expand Down Expand Up @@ -92,6 +104,9 @@ export function RecordTourDialog({
const [error, setError] = useState<string | null>(null);
const [savedName, setSavedName] = useState<string | null>(null);
const [saveCancelled, setSaveCancelled] = useState(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: comment overstates the separation guarantee

The comment says the config banner is "kept separate from the video save banner so the two messages never clobber each other". In fact they can coexist in the DOM simultaneously (good), but both are erased by clearResultMessages() whenever any new operation starts — so a "Saved setup as…" banner does disappear if the user immediately starts a recording.

The comment would be more accurate as: "kept separate so both can display simultaneously; cleared together when any new operation begins."

This is cosmetic only.

// Outcome banner for the configuration save/load actions (kept separate from
// the video save banner so the two messages never clobber each other).
const [configMessage, setConfigMessage] = useState<string | null>(null);
// The finished recording, held until the user names it and clicks Save.
const [pendingBlob, setPendingBlob] = useState<Blob | null>(null);
const [fileName, setFileName] = useState(DEFAULT_FILE_NAME);
Expand Down Expand Up @@ -126,6 +141,7 @@ export function RecordTourDialog({
const clearResultMessages = () => {
setSavedName(null);
setSaveCancelled(false);
setConfigMessage(null);
setError(null);
};

Expand Down Expand Up @@ -357,6 +373,69 @@ export function RecordTourDialog({
setStatus("idle");
};

// Export the editable tour setup (keyframes, durations, FPS) as a JSON file so
// it can be reloaded and refined later, independent of the recorded video.
const handleSaveConfig = async () => {
if (keyframes.length === 0) return;
try {
const content = serializeTourConfig(keyframes, fps);
const fileType = t("recordTour.configFileType");
const name = await saveTextFileWithFallback(content, {
defaultName: `${DEFAULT_CONFIG_FILE_NAME}.json`,
filters: [{ name: fileType, extensions: ["json"] }],
browserTypes: [
{ description: fileType, accept: { "application/json": [".json"] } },
],
mimeType: "application/json",
});
// Cancelling the dialog returns null and is a no-op, so only clear a prior
// result banner once the file is actually written.
if (name) {
clearResultMessages();
setConfigMessage(t("recordTour.configSaved", { name }));
Comment on lines +394 to +395

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug — clearResultMessages() wipes the "Video saved as…" banner on config save

clearResultMessages() zeros out savedName (the video-saved notification) along with the config message and any error. Saving the setup doesn't change the tour, so the docstring on clearResultMessages says it should only be called by edits that actually change the tour — this call is outside that contract.

A user who records a tour, saves the video ("Saved as map-tour.webm"), and then immediately clicks "Save setup" will see the video-saved banner silently disappear even though nothing in the tour changed.

The fix is to clear only the states that are directly relevant here (the previous config banner and any lingering load/save error) while leaving savedName intact:

Suggested change
clearResultMessages();
setConfigMessage(t("recordTour.configSaved", { name }));
setError(null);
setConfigMessage(t("recordTour.configSaved", { name }));

(The new setConfigMessage(...) call already replaces the old config message, so there's no need to explicitly null it first.)

}
} catch (err) {
console.warn("Tour configuration save failed", err);
clearResultMessages();
setError(t("recordTour.configSaveError"));
}
};

// Load a previously saved tour setup, replacing the current keyframe list and
// frame rate. Fresh ids are minted so reloaded rows never collide. A bad file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quality (low confidence): window.confirm shows a native OS dialog that may be suppressed in some contexts

window.confirm is synchronous and displays the browser/OS native dialog, which can be blocked in cross-origin iframes or certain embedded contexts (e.g. some Jupyter environments). It also does not respect the app's design system or dark theme.

The existing codebase's tauri-io.ts already provides ask() from @tauri-apps/plugin-dialog (for Tauri) and presumably a fallback, which would give a better-integrated confirmation. Alternatively, an inline "confirm" banner (e.g. a yellow warning that appears above the button row when keyframes exist, with a "Load anyway" secondary button) would avoid the modal entirely.

This is a deliberate tradeoff and the current implementation is safe — just flagging it for the design review pass.

// throws a parse error, surfaced as a translated message rather than a crash.
const handleLoadConfig = async () => {
// Loading replaces the whole tour, so confirm first when there is existing
// work a misclick would otherwise wipe.
if (keyframes.length > 0 && !window.confirm(t("recordTour.confirmLoad"))) {
Comment thread
giswqs marked this conversation as resolved.
Comment thread
giswqs marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

window.confirm() is suppressed in cross-origin iframes in Chrome 92+ and modern Firefox — it returns false without ever showing a dialog. The app is embedded in Jupyter notebooks (per CLAUDE.md), where the widget iframe is cross-origin, so any user who has added even one keyframe inside a Jupyter cell would find the Load button silently broken: the confirm call returns false, the function returns early, and no file picker opens with no explanation shown.

The existing window.confirm calls in DesktopShell.tsx and StoryMapPanel.tsx follow the same pattern, so this is consistent with the codebase — but RecordTourDialog is exposed in the Jupyter embed while those files arguably aren't. Consider replacing with a small non-blocking in-app confirm using an existing shadcn AlertDialog, or at minimum falling through to the file picker when window.confirm is not available:

Suggested change
if (keyframes.length > 0 && !window.confirm(t("recordTour.confirmLoad"))) {
if (keyframes.length > 0) {
// window.confirm is suppressed in cross-origin iframes (e.g. Jupyter).
// When it returns false and was actually suppressed (not explicitly
// cancelled), we'd silently block the load — fall through instead.
const dialogAvailable =
typeof window.confirm === "function" &&
!window.top?.location.origin !== window.location.origin; // cross-origin
if (dialogAvailable && !window.confirm(t("recordTour.confirmLoad"))) {
return;
}
}

(The iframe-detection heuristic is imperfect; a proper shadcn AlertDialog is the cleaner fix, but the fallback above would at least stop silently blocking the action.)

return;
}
try {
const fileType = t("recordTour.configFileType");
const result = await openLocalDataFileWithFallback({
filters: [{ name: fileType, extensions: ["json"] }],
accept: ".json,application/json",
readText: true,
});
// Only a null result means the picker was cancelled (a no-op). An empty
// file still flows through so parseTourConfig surfaces a real error rather
// than silently doing nothing after the user explicitly chose a file.
if (result == null) return;
Comment thread
giswqs marked this conversation as resolved.
clearResultMessages();
const config = parseTourConfig(result.text ?? "");
setKeyframes(config.keyframes.map((kf) => ({ ...kf, id: createId() })));
setFps(config.fps);
setFpsText(String(config.fps));
setConfigMessage(
t("recordTour.configLoaded", { count: config.keyframes.length }),
);
} catch (err) {
console.warn("Tour configuration load failed", err);
clearResultMessages();
setError(t("recordTour.configLoadError"));
}
};
Comment thread
giswqs marked this conversation as resolved.

const totalSeconds = estimateTourDurationMs(keyframes) / 1000;
const canRecord =
keyframes.length >= 2 && RECORDING_SUPPORTED && status === "idle";
Expand Down Expand Up @@ -411,6 +490,33 @@ export function RecordTourDialog({
</p>
)}

{/* Save / load the editable tour setup so work can be paused, resumed,
and reused across sessions (independent of the recorded video). */}
<div className="flex items-center gap-2">
<Button
type="button"
variant="outline"
size="sm"
className="flex-1"
disabled={editingFrozen}
onClick={handleLoadConfig}
>
<FolderOpen className="mr-1.5 h-3.5 w-3.5" />
{t("recordTour.loadConfig")}
</Button>
<Button
type="button"
variant="outline"
size="sm"
className="flex-1"
disabled={editingFrozen || keyframes.length === 0}
onClick={handleSaveConfig}
>
<Download className="mr-1.5 h-3.5 w-3.5" />
{t("recordTour.saveConfig")}
</Button>
</div>

<div className="flex items-center justify-between gap-2">
<Button
type="button"
Expand Down Expand Up @@ -510,6 +616,11 @@ export function RecordTourDialog({
{t("recordTour.saveCancelled")}
</p>
)}
{configMessage && (
<p className="text-sm text-emerald-600 dark:text-emerald-400">
{configMessage}
</p>
)}
{error && <p className="text-sm text-destructive">{error}</p>}

{busy ? (
Expand Down
9 changes: 9 additions & 0 deletions apps/geolibre-desktop/src/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,15 @@
"hint": "Pan and zoom the map, then capture each view. The map stays interactive while this panel is open.",
"unsupported": "This browser cannot record the map canvas. Try a recent version of Chrome, Edge, or Firefox.",
"addView": "Add current view",
"loadConfig": "Load setup",
"saveConfig": "Save setup",
"confirmLoad": "Replace the current keyframes with the loaded setup?",
"configFileType": "Tour Configuration",
"configSaved": "Saved setup as {{name}}",
"configLoaded_one": "Loaded setup with {{count}} keyframe.",
"configLoaded_other": "Loaded setup with {{count}} keyframes.",
"configSaveError": "Could not save the tour setup. Please try again.",
"configLoadError": "Could not load the tour setup. The file may be invalid.",
"keyframeCount_one": "{{count}} keyframe",
"keyframeCount_other": "{{count}} keyframes",
"empty": "Pan and zoom the map to a starting view, then add it as the first keyframe. Add at least two keyframes to record a tour.",
Expand Down
3 changes: 3 additions & 0 deletions apps/geolibre-desktop/src/lib/tauri-io.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,9 @@ export async function openLocalDataFileWithFallback(
reject(error);
}
};
// Resolve (rather than hang) when the dialog is dismissed without a pick;
// `change` never fires on cancel, so without this the Promise never settles.
input.addEventListener("cancel", () => resolve(null));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good fix for the hanging-promise bug. Worth noting: the cancel event on <input type="file"> shipped in Chrome 113, Firefox 113, and Safari 16.4 (all April–September 2023). On older browsers the cancel event never fires and the promise still won't settle on dismiss. If pre-2023 browser support is in scope, a complementary focus / visibilitychange heuristic would be needed. Not a blocker — this is strictly an improvement over the status quo.

input.click();
});
}
Expand Down
Loading
Loading