-
Notifications
You must be signed in to change notification settings - Fork 1
Claude/roadmap feature branches 01 m22wg q ff vy gz ya p sh w1 m9k #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ab74136
2c33449
d5bc2e0
8cc99a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,21 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState, useCallback } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState, useCallback, useEffect } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PanelLeftClose, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PanelLeftOpen, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PanelRightClose, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PanelRightOpen, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from 'lucide-react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ChronoscopeProvider } from './context/ChronoscopeContext'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ChronoscopeProvider, useChronoscope } from './context/ChronoscopeContext'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Header, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ControlPlane, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Viewport, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DataStream, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Waypoints, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TemporalJournal, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from './components'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getCoordinatesFromUrl, updateUrlWithCoordinates } from './utils/urlManager'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { addJournalEntry } from './utils/temporalJournal'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface ChronoscopeAppProps { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onApiKeyChange: () => void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -22,6 +25,36 @@ function ChronoscopeApp({ onApiKeyChange }: ChronoscopeAppProps) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [leftPanelOpen, setLeftPanelOpen] = useState(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [rightPanelOpen, setRightPanelOpen] = useState(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [mobileTab, setMobileTab] = useState<'controls' | 'viewport' | 'data'>('viewport'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { state, setCoordinates, renderScene } = useChronoscope(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Read URL coordinates on mount and auto-render | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const urlCoords = getCoordinatesFromUrl(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (urlCoords) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setCoordinates(urlCoords); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Small delay to ensure state is set before rendering | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| renderScene(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, 100); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, []); // eslint-disable-line react-hooks/exhaustive-deps | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Update URL and save to journal when scene is rendered | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (state.currentScene) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| updateUrlWithCoordinates(state.currentScene.coordinates); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Save to journal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addJournalEntry( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| state.currentScene.coordinates, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| state.currentScene.locationName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !!state.generatedImage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Notify journal component to refresh | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| window.dispatchEvent(new Event('journalUpdated')); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [state.currentScene]); // eslint-disable-line react-hooks/exhaustive-deps | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This effect has a bug related to journaling. It only runs when To fix this,
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="min-h-screen bg-chrono-black flex flex-col"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -54,6 +87,7 @@ function ChronoscopeApp({ onApiKeyChange }: ChronoscopeAppProps) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="flex-1 overflow-y-auto p-4 space-y-4"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <ControlPlane /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Waypoints /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <TemporalJournal /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -134,6 +168,7 @@ function ChronoscopeApp({ onApiKeyChange }: ChronoscopeAppProps) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="space-y-4"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <ControlPlane /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Waypoints /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <TemporalJournal /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {mobileTab === 'viewport' && <Viewport />} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
setTimeoutto delayrenderSceneis an anti-pattern in React and can lead to bugs. TherenderScenefunction is closed over from the initial render and will not have the updated coordinates fromsetCoordinates, which can cause it to work with stale state. This is a race condition that might appear to work due to timing luck but is not reliable.A more idiomatic and robust approach is to use separate
useEffecthooks to first set the state, and then react to that state change to perform the render. This ensures you are always working with the latest state.Consider replacing this
useEffectwith the following two hooks:This change also allows you to remove the
eslint-disablecomment, as dependencies are correctly managed.