-
Notifications
You must be signed in to change notification settings - Fork 48
Observatory UI refactorings #4520
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
base: main
Are you sure you want to change the base?
Conversation
| ) : simReplays.length === 0 ? ( | ||
| '—' | ||
| ) : ( | ||
| <div className="flex flex-wrap gap-"> |
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.
Invalid Tailwind CSS class gap- with no value specified. This will cause the styling to fail.
// Fix: Specify a valid gap value
<div className="flex flex-wrap gap-2">| <div className="flex flex-wrap gap-"> | |
| <div className="flex flex-wrap gap-2"> |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| useEffect(() => { | ||
| const load = async () => { | ||
| setPublicLeaderboard((prev) => ({ ...prev, loading: prev.entries.length === 0, error: null })) | ||
| try { | ||
| // Use the new endpoint that returns entries with VOR already computed | ||
| const response = await repo.getPublicLeaderboard() | ||
| setPublicLeaderboard({ entries: response.entries, loading: false, error: null }) | ||
| } catch (error: any) { | ||
| setPublicLeaderboard({ entries: [], loading: false, error: error.message ?? 'Failed to load leaderboard' }) | ||
| } | ||
| } | ||
| load() | ||
| const intervalId = window.setInterval(() => load(), REFRESH_INTERVAL_MS) | ||
| return () => { | ||
| window.clearInterval(intervalId) | ||
| } | ||
| }, [repo]) |
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.
Missing cleanup flag for async operation in useEffect. The old code had an ignore flag to prevent state updates on unmounted components, but this was removed in the refactor. This will cause "Can't perform a React state update on an unmounted component" warnings and potential memory leaks if the component unmounts before the async getPublicLeaderboard() completes.
// Fix: Add back the ignore flag pattern
useEffect(() => {
let ignore = false
const load = async () => {
setPublicLeaderboard((prev) => ({ ...prev, loading: prev.entries.length === 0, error: null }))
try {
const response = await repo.getPublicLeaderboard()
if (!ignore) {
setPublicLeaderboard({ entries: response.entries, loading: false, error: null })
}
} catch (error: any) {
if (!ignore) {
setPublicLeaderboard({ entries: [], loading: false, error: error.message ?? 'Failed to load leaderboard' })
}
}
}
load()
const intervalId = window.setInterval(() => load(), REFRESH_INTERVAL_MS)
return () => {
ignore = true
window.clearInterval(intervalId)
}
}, [repo])| useEffect(() => { | |
| const load = async () => { | |
| setPublicLeaderboard((prev) => ({ ...prev, loading: prev.entries.length === 0, error: null })) | |
| try { | |
| // Use the new endpoint that returns entries with VOR already computed | |
| const response = await repo.getPublicLeaderboard() | |
| setPublicLeaderboard({ entries: response.entries, loading: false, error: null }) | |
| } catch (error: any) { | |
| setPublicLeaderboard({ entries: [], loading: false, error: error.message ?? 'Failed to load leaderboard' }) | |
| } | |
| } | |
| load() | |
| const intervalId = window.setInterval(() => load(), REFRESH_INTERVAL_MS) | |
| return () => { | |
| window.clearInterval(intervalId) | |
| } | |
| }, [repo]) | |
| useEffect(() => { | |
| let ignore = false | |
| const load = async () => { | |
| setPublicLeaderboard((prev) => ({ ...prev, loading: prev.entries.length === 0, error: null })) | |
| try { | |
| // Use the new endpoint that returns entries with VOR already computed | |
| const response = await repo.getPublicLeaderboard() | |
| if (!ignore) { | |
| setPublicLeaderboard({ entries: response.entries, loading: false, error: null }) | |
| } | |
| } catch (error: any) { | |
| if (!ignore) { | |
| setPublicLeaderboard({ entries: [], loading: false, error: error.message ?? 'Failed to load leaderboard' }) | |
| } | |
| } | |
| } | |
| load() | |
| const intervalId = window.setInterval(() => load(), REFRESH_INTERVAL_MS) | |
| return () => { | |
| ignore = true | |
| window.clearInterval(intervalId) | |
| } | |
| }, [repo]) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nishu-builder
left a comment
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.
ty for this!

Most changes under the hood, or pretty minor in how it looks - extracted common UI components, got rid of more inline styles, etc.
For context, my longer-term goal is to eventually extract the common React components library to reuse here, in gridworks and everywhere else.
The next iteration I'm going to do on this, though (if I will have time and no one else does it first) is Next.js rewrite, because the big part of other code that I want to trim down is loading states.