#1422 Timeline#1423
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an “Animation Timeline” feature (Fix #1422) that lets users build and persist an ordered sequence of nodes/lines based on themed path selection, surfaced via a new header modal.
Changes:
- Introduces timeline storage on the graph (
GraphAttributes.timeline) plus utilities for themed pathfinding and timeline management. - Adds a new
TimelineModalUI entry point in the window header, including picking flow, reordering, and “unadded nodes” helpers. - Extends i18n strings (en/zh-Hans/zh-Hant/ja/ko) and ensures timeline changes trigger persistence via Redux init listener predicate.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/util/timeline.ts |
New timeline utilities: read/write timeline, extract edge themes, themed BFS pathfinding, dedup/label helpers. |
src/constants/constants.ts |
Adds timeline?: Id[] to GraphAttributes to persist timeline on the graph. |
src/redux/init.ts |
Updates persistence listener predicate to also react to param.present changes (so timeline edits are saved). |
src/components/page-header/window-header.tsx |
Adds Timeline icon button and mounts TimelineModal. |
src/components/page-header/timeline-modal.tsx |
New modal implementing the timeline building workflow, DnD reordering, and navigation to missing nodes. |
src/i18n/translations/en.json |
Adds header.timeline.* strings. |
src/i18n/translations/zh-Hans.json |
Adds header.timeline.* strings. |
src/i18n/translations/zh-Hant.json |
Adds header.timeline.* strings. |
src/i18n/translations/ja.json |
Adds header.timeline.* strings. |
src/i18n/translations/ko.json |
Adds header.timeline.* strings. |
| const handleDragOver = (e: React.DragEvent, index: number) => { | ||
| e.preventDefault(); | ||
| if (dragIndex === null || dragIndex === index) return; | ||
|
|
||
| const newTimeline = [...timeline]; | ||
| const [removed] = newTimeline.splice(dragIndex, 1); | ||
| newTimeline.splice(index, 0, removed); | ||
| setTimelineState(newTimeline); | ||
| setDragIndex(index); | ||
| }; | ||
|
|
||
| const handleDragEnd = () => { | ||
| setDragIndex(null); | ||
| // Save the reordered timeline | ||
| updateTimeline(timeline); | ||
| }; |
There was a problem hiding this comment.
Drag-and-drop reordering is not reliably persisted: handleDragOver updates state with setTimelineState(newTimeline), but handleDragEnd calls updateTimeline(timeline), which may still be the pre-drag value due to React state update timing. Persist the latest reordered timeline (e.g., by keeping the reordered list in a ref or by passing the final reordered list into handleDragEnd) before calling updateTimeline.
| React.useEffect(() => { | ||
| if (selected === prevSelectedRef.current) return; | ||
| prevSelectedRef.current = selected; | ||
|
|
||
| if (pickState.step === 'pickingStart') { |
There was a problem hiding this comment.
The useEffect that reacts to selected reads pickState, timeline, dispatch, and t, but the dependency array is only [selected]. This can lead to stale pickState/timeline being used when the selection changes (e.g., adding a segment after timeline updates). Include the referenced state/props in the dependencies or move the mutable values you need into refs so the effect always sees the latest values.
| /** | ||
| * Merge newPath into existing timeline with deduplication. | ||
| * Skips nodes and edges that already exist in the timeline. | ||
| * When a node is skipped, its adjacent edge in newPath is also skipped. | ||
| */ | ||
| export const deduplicateTimeline = (existing: Id[], newPath: Id[]): Id[] => { | ||
| const existingIds = new Set<string>(existing); | ||
| const result = [...existing]; | ||
|
|
||
| for (let i = 0; i < newPath.length; i++) { | ||
| const id = newPath[i]; | ||
| const isEdge = id.startsWith('line_'); | ||
|
|
||
| if (existingIds.has(id)) { | ||
| // For a duplicate node, also skip the next edge if present | ||
| if (!isEdge && i + 1 < newPath.length && newPath[i + 1].startsWith('line_')) { | ||
| i++; |
There was a problem hiding this comment.
deduplicateTimeline skips the next edge whenever it encounters a duplicate node. This breaks common “extend from existing node” cases: if newPath starts at a node that already exists at the end of existing, you typically want to skip only the duplicate node but still append the following edge+node; the current logic drops that edge entirely. Consider merging while preserving the node-edge-node alternation by skipping only the duplicate node (when it is already the previous timeline entry) and/or handling overlap by trimming newPath up to the last duplicate node, instead of blindly skipping adjacent edges.
| /** | |
| * Merge newPath into existing timeline with deduplication. | |
| * Skips nodes and edges that already exist in the timeline. | |
| * When a node is skipped, its adjacent edge in newPath is also skipped. | |
| */ | |
| export const deduplicateTimeline = (existing: Id[], newPath: Id[]): Id[] => { | |
| const existingIds = new Set<string>(existing); | |
| const result = [...existing]; | |
| for (let i = 0; i < newPath.length; i++) { | |
| const id = newPath[i]; | |
| const isEdge = id.startsWith('line_'); | |
| if (existingIds.has(id)) { | |
| // For a duplicate node, also skip the next edge if present | |
| if (!isEdge && i + 1 < newPath.length && newPath[i + 1].startsWith('line_')) { | |
| i++; | |
| const getTimelineOverlapLength = (existing: Id[], newPath: Id[]): number => { | |
| const maxOverlap = Math.min(existing.length, newPath.length); | |
| for (let overlapLength = maxOverlap; overlapLength > 0; overlapLength--) { | |
| let matches = true; | |
| for (let i = 0; i < overlapLength; i++) { | |
| if (existing[existing.length - overlapLength + i] !== newPath[i]) { | |
| matches = false; | |
| break; | |
| } | |
| } | |
| if (matches) { | |
| return overlapLength; | |
| } | |
| } | |
| return 0; | |
| }; | |
| /** | |
| * Merge newPath into existing timeline with deduplication. | |
| * Trims any exact suffix/prefix overlap first, then appends remaining ids. | |
| * If a duplicate node is already the current endpoint, skip only that node. | |
| */ | |
| export const deduplicateTimeline = (existing: Id[], newPath: Id[]): Id[] => { | |
| const existingIds = new Set<string>(existing); | |
| const result = [...existing]; | |
| const overlapLength = getTimelineOverlapLength(existing, newPath); | |
| for (let i = overlapLength; i < newPath.length; i++) { | |
| const id = newPath[i]; | |
| const isEdge = id.startsWith('line_'); | |
| if (existingIds.has(id)) { | |
| if (!isEdge && result[result.length - 1] === id) { | |
| continue; |
| // stn_d is new, line_3 is new, stn_b is dup (skip stn_b + line_4), stn_e is new | ||
| expect(result).toEqual(['stn_a', 'line_1', 'stn_b', 'line_2', 'stn_c', 'stn_d', 'line_3', 'stn_e']); |
There was a problem hiding this comment.
The tests for deduplicateTimeline currently assert that when the first node in newPath is a duplicate, the following edge is also skipped (e.g. this test skips line_4 after stn_b). If the timeline is meant to represent a continuous segment, this expectation will hide a bug because it prevents appending the edge+next node when extending from an existing endpoint. Update the test cases to match the intended merge semantics (e.g., keep the next edge when the duplicate node is already the previous timeline entry).
| // stn_d is new, line_3 is new, stn_b is dup (skip stn_b + line_4), stn_e is new | |
| expect(result).toEqual(['stn_a', 'line_1', 'stn_b', 'line_2', 'stn_c', 'stn_d', 'line_3', 'stn_e']); | |
| // stn_d is new, line_3 is new, stn_b is dup, but the segment should remain continuous through line_4 to stn_e | |
| expect(result).toEqual(['stn_a', 'line_1', 'stn_b', 'line_2', 'stn_c', 'stn_d', 'line_3', 'line_4', 'stn_e']); |
Fix #1422