feat(record-tour): clearer keyframe layout, recapture, and two-step save#886
Conversation
… save Address the Record Map Tour UX report (#874): - Lay each keyframe out over two rows so the transition control gets its own line with a full "Transition N seconds" label that no longer clips in a narrow panel. - Add a Recapture (camera) action to every keyframe row so a stop can be re-framed in place from the current map view, instead of deleting and re-adding it (which lost its order and segment duration). - Decouple recording from saving: a finished take is held in memory and the user names it and clicks Save (or Discard to re-record), rather than an automatic map-tour.webm download firing the instant capture ends. The custom name flows through to both the native Save As dialog and the browser download fallback. Fixes #874
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRecordTourDialog now holds finished recordings until the user saves or discards them. The dialog adds a ready state, freezes editing while a take is held, and updates the save helper and localized labels for the new flow. ChangesRecord tour save workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewThree findings follow, each with an inline comment on the relevant lines. No security, performance, or CLAUDE.md issues were identified. Bugs / LogicSilent discard of pending recording when keyframes are edited — medium confidence QualityWrong error message for save failures — high confidence Minor nits on the filename input (grouped) — low confidence Checked and found nothing to raise
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx`:
- Around line 33-37: The default output name in RecordTourDialog is hard-coded
and bypasses localization; replace the DEFAULT_FILE_NAME usage in
RecordTourDialog so the initial filename comes from react-i18next via t(), and
add the corresponding translation key to the dialog’s locale entries with
en.json as the source of truth. Update any other references in the dialog flow
around the filename field and save handling so they use the localized value
instead of the literal map-tour string.
- Around line 256-258: Separate the recording-cancel and save-failure states in
RecordTourDialog so the UI message matches the phase that failed. Update the
zero-byte branch in the recording flow and the disk-write failure branch in the
save flow to use distinct status/error state instead of reusing saveCancelled
and recordTour.recordError across both phases. Use the existing RecordTourDialog
handlers around the blob-size check and the save-to-disk logic to set a
recording-cancel message when the recording never produced data, and a
save-failure message only when the write step fails after a successful
recording.
- Around line 95-97: The RecordTourDialog state keeps pendingBlob in memory
after the dialog is closed, since the component stays mounted and no open-driven
cleanup runs. Update RecordTourDialog to watch the open prop and clear
pendingBlob whenever the dialog transitions to closed, reusing the existing
pendingBlob/setPendingBlob state and any existing reset helper if present so the
ready-state blob is released on close.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2abd9dff-a876-4f1a-9753-410ff10a5ba4
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.json
| // "ready" holds a finished recording in memory so saving is a deliberate second | ||
| // step (name + Save) rather than an automatic download the moment recording ends. | ||
| type Status = "idle" | "recording" | "ready" | "saving"; | ||
|
|
||
| const DEFAULT_FILE_NAME = "map-tour"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Localize the default output name.
Line 37 hard-codes "map-tour", and that value is shown in the filename field and native/browser save flow. It should come from i18n like the rest of this dialog copy, with a corresponding key in apps/geolibre-desktop/src/i18n/locales/en.json.
As per coding guidelines, "UI strings must be translatable via react-i18next; add new user-facing strings using t() function and update locales in apps/geolibre-desktop/src/i18n/locales/*.json (en.json is source of truth)".
Also applies to: 95-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx` around
lines 33 - 37, The default output name in RecordTourDialog is hard-coded and
bypasses localization; replace the DEFAULT_FILE_NAME usage in RecordTourDialog
so the initial filename comes from react-i18next via t(), and add the
corresponding translation key to the dialog’s locale entries with en.json as the
source of truth. Update any other references in the dialog flow around the
filename field and save handling so they use the localized value instead of the
literal map-tour string.
Source: Coding guidelines
| // 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); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all call sites for RecordTourDialog and inspect whether callers
# conditionally render it or always keep it mounted with open={...}.
rg -n -C3 --type=tsx '\bRecordTourDialog\b' apps/geolibre-desktop/srcRepository: opengeos/GeoLibre
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 --type ts '\bRecordTourDialog\b' apps/geolibre-desktop/srcRepository: opengeos/GeoLibre
Length of output: 3719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the RecordTourDialog implementation, specifically looking for logic handling the 'open' prop change
# and the 'pendingBlob' state.
sed -n '79,150p' apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxRepository: opengeos/GeoLibre
Length of output: 3567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the usage of the 'open' prop and 'useEffect' hooks in RecordTourDialog
sed -n '120,200p' apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxRepository: opengeos/GeoLibre
Length of output: 3063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for useEffect hooks in the component, specifically looking for 'open' in the dependency array
rg -A 10 "useEffect.*open" apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxRepository: opengeos/GeoLibre
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Effect hooks to see how the component reacts to prop changes
rg -B 2 -A 10 "useEffect\(" apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxRepository: opengeos/GeoLibre
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the word "effect" or "Effect" to find any effects in the file
rg -in 'effect' apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxRepository: opengeos/GeoLibre
Length of output: 155
Close action does not release pendingBlob
The component is always mounted (parent never unmounts it), and there is no useEffect hook listening to the open prop. Closing the dialog while in the ready state leaves the large blob in component memory. Please explicitly clear pendingBlob when the dialog closes.
Relevant snippet from `RecordTourDialog.tsx`
const [pendingBlob, setPendingBlob] = useState<Blob | null>(null);
// ...
// No useEffect watches `open` to trigger clearPendingBlob()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx` around
lines 95 - 97, The RecordTourDialog state keeps pendingBlob in memory after the
dialog is closed, since the component stays mounted and no open-driven cleanup
runs. Update RecordTourDialog to watch the open prop and clear pendingBlob
whenever the dialog transitions to closed, reusing the existing
pendingBlob/setPendingBlob state and any existing reset helper if present so the
ready-state blob is released on close.
- Surface the discard consequence in the "ready" UI: the message now warns
that editing the tour discards the captured recording, so the silent drop
when a keyframe is edited is no longer a hidden footgun.
- Use a dedicated recordTour.saveError ("Could not save the video.") in the
handleSave catch instead of recordError, which wrongly blamed the recording
step for a file-write failure.
- autoFocus the file name input when the panel switches to the save step, and
reset the name to the default after a successful save or a discard so a
second recording doesn't show the previous name.
Code reviewBugs
Performance
SecurityNothing of concern in this diff. The Quality
CLAUDE.md
|
- Freeze the keyframe-editing controls (Add current view, recapture, reorder, remove, segment duration) while a finished take is held in the "ready" state, so the recording can only be dropped via the explicit Discard button rather than a one-click edit whose footer warning may have scrolled off-screen. The ready-state message reverts to "discard to re-record" accordingly. - Guard handleSave with a savingRef so a fast double-click or Enter keyboard repeat can't fire two concurrent saves (two pickers / two downloads) before the "saving" state re-renders.
Code reviewReviewed Bugs
Performance
Quality
SecurityNothing to flag. CLAUDE.mdNew i18n strings are added only to Overall: the PR is well-structured and the core logic is correct. The one actionable item is the FPS |
- Widen saveBinaryFileWithFallback (and saveBinaryFileBrowser) to accept Blob | Uint8Array. The browser path now writes a Blob straight through, and the Tauri path materializes bytes only after the save dialog is confirmed. - RecordTourDialog passes the recorded Blob directly instead of copying it into a Uint8Array on every handleSave call, so a cancel-and-retry no longer re-copies a potentially several-hundred-MB recording. Existing Uint8Array callers are unaffected (backward-compatible union type).
- Freeze the FPS input while a recording is held (status "ready"), matching the other editing controls: a captured take's frame rate is already fixed, so editing FPS then would only mislead. - Keep the fly-to preview available in the "ready" state via a separate previewDisabled prop gated on busy alone. Previewing only moves the map camera and can't produce a stale recording, so users can verify the tour before choosing Save or Discard without having to discard first.
Code reviewBugs
Quality
Security / Performance / CLAUDE.mdNothing to flag. The two-step save flow, the |
- Disable the panel close button while a finished take is held ("ready"), using
editingFrozen instead of busy, so closing can't silently strand a recording
in memory or drop the user back into the save step on reopen. Save or discard
first.
- Document that saveBinaryFileWithFallback uses a Blob's own .type as-is and
only applies options.mimeType when wrapping a Uint8Array, so the contract is
explicit for future callers.
Code reviewReviewed the three changed files: Bugs
Quality
SecurityNothing found. File name sanitisation via PerformanceNothing found. The Blob-to- CLAUDE.mdAll new user-facing strings are in |
- Stop clearing the "Saved …" / outcome banner on plain keyframe edits. The per-edit invalidateRecording() was dead for its real purpose (editing is frozen while a take is held, so it never had a pending blob to drop) and only caused a regression: clicking into a duration field wiped the save confirmation. Replaced it with a narrower clearResultMessages() called only from addCurrentView (its original pre-PR behavior); recapture/remove/move/ duration no longer touch the banner. - Add a finally safety net in handleRecord so an exception escaping the catch (e.g. t() throwing) can't leave the dialog stuck on "recording" with a null abort controller; it falls back to "idle". - Log the raw error in handleSave's catch (console.warn) so a save failure is debuggable behind the user-facing message. - Note that arrayBuffer() rejects on a detached Blob and the rejection propagates to the caller's catch.
Code reviewOverall the PR is well-structured: the new `"ready"` state machine is clean, the double-click guard on `handleSave` is correct, and the `savingRef`/`editingFrozen` interplay is sound. Three findings below. Bugs
Quality
Security, Performance, CLAUDE.mdNothing to flag. The |
- Clear the "Saved …" outcome banner when recapturing a keyframe, matching addCurrentView: both change a keyframe's camera, so the saved video no longer matches the tour and the success banner would otherwise mislead. Plain reorder/duration tweaks still leave the banner alone.
- Pluralize the transition unit against the value being typed (falling back to the committed value), so the label tracks the input and reads "1 second" rather than "1 seconds" mid-edit before blur commits. - Correct an inaccurate comment: Blobs have no "detached" state (that's ArrayBuffer); arrayBuffer() can still reject for other reasons (OOM, unavailable backing store), which propagates to the caller's catch.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx`:
- Around line 119-122: The saved banner handling in RecordTourDialog should also
clear when a keyframe is removed, since that changes the tour content just like
adding or recapturing. Update the logic around the saved banner state used by
removeKeyframe() so savedName is reset there too, and make sure the same
banner-clearing behavior stays consistent with the existing edit handling for
camera-changing actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 722bd410-73c8-4a72-b265-067d014cee42
📒 Files selected for processing (1)
apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx
| // control) and while a finished take is held in the "ready" state, so | ||
| // closing can't silently strand a recording in memory or drop the | ||
| // user back into the save step on reopen. Save or discard first. | ||
| disabled={editingFrozen} |
There was a problem hiding this comment.
The close button is disabled in both busy and ready states, but users in the "ready" state have no visible or accessible hint explaining why the ✕ is greyed out. Someone who finishes recording and instinctively clicks ✕ will find it unresponsive with no feedback — the code comment explains the intent, but that isn't surfaced in the UI.
A title attribute conditioned on the "ready" state would help (also useful for aria-description/tooltip on the desktop side):
| disabled={editingFrozen} | |
| disabled={editingFrozen} | |
| === "ready" ? t("recordTour.saveOrDiscardFirst") : undefined} |
Requires adding "saveOrDiscardFirst" to en.json (e.g. "Save or discard the recording first."). The title won't surface on touch, but it covers mouse and keyboard users and sets a baseline for a future tooltip.
| {t("recordTour.secondsUnit")} | ||
| {/* Pluralize against the value being typed (falling back to the | ||
| committed one) so the unit tracks the input: "1 second", not | ||
| "1 seconds", even mid-edit before blur commits. */} |
There was a problem hiding this comment.
The plural form ("second" vs. "seconds") is driven by keyframe.durationMs / 1000 — the committed store value — not by the text local state shown in the <Input> above. When the user types an out-of-range value (e.g. 0, which is below MIN_SEGMENT_SECONDS = 0.5), onDurationSeconds is skipped, so durationMs stays at the old value. The visible number in the input and the pluralization word then diverge until blur reconciles them.
For the common valid-typing path this is invisible, so it's low priority. If you want to close the gap, the simplest fix is to pluralize against the in-progress numeric value and fall back to the committed one when out of range:
| "1 seconds", even mid-edit before blur commits. */} | |
| {t("recordTour.secondsLong", { | |
| count: (() => { | |
| const v = Number(text); | |
| return Number.isFinite(v) && v >= MIN_SEGMENT_SECONDS && v <= MAX_SEGMENT_SECONDS | |
| ? v | |
| : keyframe.durationMs / 1000; | |
| })(), | |
| })} |
This is cosmetic; the committed value is always what actually gets recorded.
| // caller's catch. | ||
| const bytes = | ||
| content instanceof Blob | ||
| ? new Uint8Array(await content.arrayBuffer()) |
There was a problem hiding this comment.
On the Tauri path, content.arrayBuffer() materialises a full Uint8Array copy of the blob. For a long recording this means both the Blob (still referenced by pendingBlob state in the dialog) and a fresh Uint8Array of the same size live in memory simultaneously until writeFile returns and the Uint8Array goes out of scope.
The code comment already acknowledges that the conversion is deferred until after the dialog is confirmed — that's the right trade-off. Just flagging that on a constrained device and a multi-minute recording the peak is roughly 2× the video size. If Tauri ever exposes a streaming write or direct-Blob write, this would be worth revisiting. No action needed now.
Code reviewThree files changed: BugsNothing found. The status-machine transitions ( SecurityNo issues found. The Performance
Quality
CLAUDE.md
|
| inputMode="decimal" | ||
| aria-label={t("recordTour.segmentSeconds")} | ||
| className="h-7 w-14" | ||
| className="h-7 w-16" |
There was a problem hiding this comment.
Accessibility: conflicting <label> and aria-label give sighted and screen-reader users different label text.
The wrapping <label> element implicitly associates with this <Input>, so clicking the word "Transition" will focus the field (good). But the aria-label="Seconds to move here" attribute takes precedence over the implicit <label> association for screen readers, so assistive technology announces "Seconds to move here" rather than "Transition". The two labels convey different information.
Options:
- Drop the
aria-labeland let the screen reader compute the label from the<label>content ("Transition [value] seconds"). The<label>already wraps the input so the association is automatic. - Keep
aria-labelbut use a<div>instead of<label>, and add a standalone<label htmlFor="…">that mirrors thearia-labeltext so visual and accessible names match.
| className="h-7 w-16" | |
(Leave the aria-label if you want the richer description for AT users, but then switch the outer <label className=…> to a <div className=…> so the implicit association no longer conflicts.)
| aria-label={t("recordTour.recapture")} | ||
| title={t("recordTour.recapture")} | ||
| disabled={disabled} | ||
| onClick={onRecapture} |
There was a problem hiding this comment.
Nit: "Update to current map view" can be read in the wrong direction.
The intended meaning is "overwrite this keyframe's camera with the current map view", but the phrasing "Update to current map view" could be read as "fly the map to this keyframe's view" — which is what the fly-to button already does.
| onClick={onRecapture} | |
Suggest changing the en.json value to something unambiguous, e.g. "Update this keyframe to current map view" or "Replace with current map view". Low-stakes, but tooltips are often the only affordance users have for icon-only buttons.
| const blob = | ||
| content instanceof Blob | ||
| ? content | ||
| : new Blob([toArrayBuffer(content)], { type: options.mimeType }); |
There was a problem hiding this comment.
Minor footgun: options.mimeType is silently ignored when a Blob with an empty (or wrong) type is passed.
The comment correctly documents this, but the contract is easy to violate. The current caller (handleSave) always passes a Blob whose type was set by MediaRecorder (e.g. "video/webm;codecs=vp9"), so it works today.
A lightweight guard would make the guarantee explicit:
| : new Blob([toArrayBuffer(content)], { type: options.mimeType }); | |
| const blob = | |
| content instanceof Blob | |
| ? content | |
| : new Blob([toArrayBuffer(content)], { type: options.mimeType }); |
No code change needed, but worth a follow-up if saveBinaryFileWithFallback is ever called with a typeless Blob — the result would be a download or FSA write with no content-type, which some browsers handle poorly. Consider adding a console.warn assertion in dev mode or updating the SaveBinaryFileOptions type to document which field applies in which case.
Code reviewReviewed BugsNothing blocking found. The state machine transitions are correct:
SecurityNo issues. Filename sanitisation ( PerformanceNo issues. The lazy-materialise-bytes strategy in the Tauri path (convert Quality
CLAUDE.mdAll new user-facing strings use |
Make the "Saved …" banner behavior fully consistent: every edit that actually changes the tour now clears it, while no-op interactions leave it alone. - removeKeyframe clears the banner (dropping a stop changes the recording), per CodeRabbit's note. - move clears only on an in-range reorder (guards the inert end-of-list click). - setSegmentSeconds clears only when the duration actually changes, so merely focusing and blurring a field (which re-commits the same value) no longer wipes the banner — preserving the earlier no-op-blur fix.
giswqs
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
All inline threads resolved and the one actionable item (clear the saved-recording banner on keyframe removal) is addressed in 41c9a52.
| const move = (index: number, delta: number) => { | ||
| const target = index + delta; | ||
| // Guard the no-op (already at an end) so an inert reorder click can't clear | ||
| // the banner; the buttons are disabled there anyway, this is belt-and-braces. | ||
| if (target < 0 || target >= keyframes.length) return; | ||
| clearResultMessages(); | ||
| setKeyframes((current) => { | ||
| const next = [...current]; | ||
| const target = index + delta; | ||
| if (target < 0 || target >= next.length) return current; | ||
| [next[index], next[target]] = [next[target], next[index]]; | ||
| return next; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Medium confidence — inner bounds guard removed from functional updater
The original move had the out-of-range check inside the setKeyframes updater, operating on next (the latest committed state):
if (target < 0 || target >= next.length) return current;That's now gone. The outer check on keyframes.length works correctly for every synchronous click handler today — the closure snapshot is always fresh in React's single-threaded model — but if current.length ever diverges from the closure's keyframes.length (concurrent state batching, future refactoring that queues multiple setKeyframes calls in the same tick, React Strict Mode double-invoking updaters), the unchecked swap silently writes undefined into next[index] or next[target], corrupting the array without a type error.
Both goals (skip clearResultMessages() on no-ops, and a safe updater) can be achieved together by keeping the guard in both places:
| const move = (index: number, delta: number) => { | |
| const target = index + delta; | |
| // Guard the no-op (already at an end) so an inert reorder click can't clear | |
| // the banner; the buttons are disabled there anyway, this is belt-and-braces. | |
| if (target < 0 || target >= keyframes.length) return; | |
| clearResultMessages(); | |
| setKeyframes((current) => { | |
| const next = [...current]; | |
| const target = index + delta; | |
| if (target < 0 || target >= next.length) return current; | |
| [next[index], next[target]] = [next[target], next[index]]; | |
| return next; | |
| }); | |
| }; | |
| const move = (index: number, delta: number) => { | |
| const target = index + delta; | |
| // Guard the no-op (already at an end) so an inert reorder click can't clear | |
| // the banner; the buttons are disabled there anyway, this is belt-and-braces. | |
| if (target < 0 || target >= keyframes.length) return; | |
| clearResultMessages(); | |
| setKeyframes((current) => { | |
| const next = [...current]; | |
| if (target < 0 || target >= next.length) return current; | |
| [next[index], next[target]] = [next[target], next[index]]; | |
| return next; | |
| }); | |
| }; |
| const setSegmentSeconds = (id: string, seconds: number) => { | ||
| const durationMs = Math.round(seconds * 1000); | ||
| // Clear the banner only on a real change, so merely focusing and blurring a | ||
| // duration field (which re-commits the same value) doesn't wipe it. | ||
| const current = keyframes.find((kf) => kf.id === id); | ||
| if (current && current.durationMs !== durationMs) clearResultMessages(); | ||
| setKeyframes((prev) => | ||
| prev.map((kf) => (kf.id === id ? { ...kf, durationMs } : kf)), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Low confidence — stale-closure comparison for banner suppression
keyframes.find() reads the render-closure snapshot of keyframes, while the actual mutation goes through the functional updater (which receives prev, the latest committed state). For two very rapid onChange events that fire before React re-renders (e.g. held-down arrow key in the number spinner), the snapshot could still show the pre-first-edit durationMs, causing the second identical != comparison to miss the difference and silently skip clearResultMessages().
In practice each onChange fires as a separate browser event, React flushes between them, and the closure is always up-to-date, so this is a theoretical edge. If robustness matters here the comparison could use a useRef that's kept in sync on each committed render, or the banner-clear could be done unconditionally on every duration-input change (the current no-op-blur concern is already handled since onBlur re-commits the same value, not onChange). Not blocking.
Code review placeholder - will replace |
|
test |
Code review |
Code reviewAfter 8+ rounds of CodeRabbit/Claude iteration the code is in good shape. Three areas worth noting: Bugs
Quality
SecurityNo issues. Filename normalization ( PerformanceThe CLAUDE.md
|
1 participant
Summary
Addresses the Record Map Tour UX report in #874. Three changes to
RecordTourDialog:Transition [N] secondswith the full word spelled out. This removes the cramped4/sclipping reported on narrow panels, addressing the layout and label-visibility asks without needing a manual resize.File namefield (defaulting tomap-tour) with explicit Save video… and Discard buttons, rather than an automaticmap-tour.webmdownload firing the instant recording ends. The chosen name flows through to both the native Save As dialog (File System Access) and the browser anchor-download fallback used by browsers like Safari. Editing the tour after recording discards the now-stale take.The keyframe coordinates are kept (they identify each stop and power the click-to-preview fly-to) but no longer crowd the transition control, which now has its own dedicated line as requested.
Verification
Drove the real app at
localhost:5173with Playwright in both light and dark themes using basemap views:Transition N secondslabel with no clipping.my-custom-tour.webmvia the download fallback; confirmed the native picker path is taken whenshowSaveFilePickeris available.npm run buildandpre-commit run(eslint, build, formatting) pass; frontend tests green.Fixes #874
Summary by CodeRabbit