fix: let users switch a merge to a replace after the fact; v1.7.1#24
Merged
Conversation
… loop Reopening a recording via remembered File System Access permission (?url=file://...) could self-trigger forever: replaying history set activeId, which changed appendFilesAsDatasets's identity, which re-ran the SPA bootstrap effect that depends on it, which replayed history again. Symptoms were a flickering sidebar/player and hundreds of WorkerSourceCancelledError as the player was torn down and rebuilt in a tight loop, occasionally crashing the tab. Direct drag-and-drop was unaffected since it's a one-shot event handler, not an effect. Root-caused and fixed at the design-pattern level rather than patched: - useSpaUrlBootstrap wraps its handlers in React's useEffectEvent, so its effect only ever depends on (active, url) - no callback identity churn can retrigger it. react-hooks/exhaustive-deps now errors if this guarantee is ever violated, turning the bug class into a lint error instead of a silent regression. - resolveAppendGroupId/resolveActiveId (datasetSources.ts) make the "which group is really active" logic pure and directly testable, fixing the underlying activeId "orphan id" bounce. - Regression tests pin both the pure logic and the hook's Effect timing (useSpaUrlBootstrap.test.tsx feeds in a worse-than-production case: a fresh handler identity on every render). Also splits the 1400+ line RosViewerImpl.tsx into focused, independently testable hooks/components (useDatasetSession, usePlayerLifecycle, useRecordingSourceActions, useDatasetHistory, useThemeLanguagePreferences, useOpenFeedback, RosViewerLandingScreen, sourceLoading, rosViewerUtils), leaving RosViewerImpl.tsx as a thin composition layer. Fixed a latent bug found during the split: the hidden #rosview-inline-file/-tar inputs must render unconditionally even once a player exists, or history replay can't click them.
Opening more files while a session is active still merges them in
immediately and non-blocking, but now shows a dismissible toast
("Merged X into the current session") with a "Switch to replace
instead" action across every add-file entry point (drag-and-drop,
Navbar Open File/Open Directory, the hidden file input). Clicking it
detaches the just-added files into their own fresh session and
switches to it; the original session is left intact and reachable
from the sidebar Data tab rather than being destroyed.
- appendFilesAsDatasets (useDatasetSession.ts) now returns
{ groupId, merged, addedItemIds } instead of void, without touching
how it reads activeId/datasets (still via refs, so its identity
stays stable) or its own dependency array.
- notifyMergeWithUndo (useRecordingSourceActions.ts) reuses the
already-available offlineIntl/setExtraDatasets/setActiveId to show
the toast and perform the undo; wired into the four non-forced-new-
session entry points. Tar extraction and history replay are
unambiguous "open a new session" actions already and are untouched.
- Extracted fileBatchDisplayName (rosViewerUtils.ts) for the "first
+N" label shared by history recording and the new toast.
sonner's default toast layout puts the action button to the right of the message in a single row, which read as cramped once the message wrapped to two lines. Scope a classNames override to just this toast (message stacked above a full-width action button) via Tailwind's important-modifier utilities, since sonner's own attribute-selector base styles set align-items/gap/margin directly and would otherwise win on specificity. Scoped per-toast rather than globally, so plain toast.error()/toast.success() calls elsewhere are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RosViewerImpl.tsxinto focused, independently testable hooks/components (useDatasetSession,usePlayerLifecycle,useRecordingSourceActions,useDatasetHistory,useThemeLanguagePreferences,useOpenFeedback,RosViewerLandingScreen,sourceLoading,rosViewerUtils), leavingRosViewerImpl.tsxas a thin composition layer.?url=file://...) could self-trigger an infinite init loop (flickering sidebar/player, repeatedWorkerSourceCancelledError, occasional tab crashes). Root-caused viauseEffectEventinuseSpaUrlBootstrapplus new pure helpersresolveAppendGroupId/resolveActiveIdindatasetSources.ts(with regression tests).package.jsonversion to1.7.1.Test plan
npm run lintnpm run typechecknpm test(595 unit tests)npm run build(SPA) andnpm run build:libnpm run gen:e2e:fixtures && npm run test:e2e(37 Playwright tests, incl. new merge/replace-toast coverage inmulti-sources.spec.ts)