feat(apps/ensadmin): better handle projections and outdated snapshots #1251
Conversation
🦋 Changeset detectedLatest commit: ea42b6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
notrab
changed the title
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Appreciate your updates here 👍 Reviewed and shared feedback
| * as of real-time projection generated{" "} | ||
| <RelativeTime timestamp={projectedAt} includeSeconds conciseFormatting /> from indexing | ||
| status snapshot captured{" "} | ||
| <RelativeTime timestamp={snapshotTime} includeSeconds conciseFormatting />. |
There was a problem hiding this comment.
This is cool! The "Worst-Case Distance" value is refreshing every second, but the values highlighted in red don't seem to be updating. The final timestamp highlighted in red should definitely be updating each second.
There was a problem hiding this comment.
snapshotTime is now updated every second, but need to look at projectedAt .
| Worst-Case Distance* | ||
| </div> | ||
| <div className="text-sm"> | ||
| {worstCaseDistance !== null ? `${worstCaseDistance} seconds` : "N/A"} |
There was a problem hiding this comment.
Is there a special reason for this null check? Shouldn't this value always be guaranteed to be defined?
apps/ensadmin/src/components/indexing-status/projection-info.tsx
Outdated
Show resolved
Hide resolved
| // Subscribe to synchronized timestamp updates (ticks every second). | ||
| // All components using this hook will receive the same timestamp value, | ||
| // ensuring consistent projections across the entire UI. | ||
| const projectedAt = useSyncExternalStore( |
There was a problem hiding this comment.
Interesting. Sorry if this is a silly question but curious if there's a special reason to sync the projectedAt timestamp rather than just having a provider with a singleton holding the latest projection that updates itself automatically approximately once per second?
In my mental model, we're trying to build a very simple "stateful" ENSNode client, that maintains state in memory (therefore it is stateful) for the config and the latest projection that is automatically updated once per second.
Then the whole app should be able to make use of the state held by this "stateful" client everywhere.
Appreciate your advice.
| * | ||
| * UI component for presenting indexing stats UI for specific overall status. | ||
| */ | ||
| export function IndexingStatsShell({ |
There was a problem hiding this comment.
This is a general comment with overall feedback. Not about this file or this line necessarily.
I continue to experience the core issue described in #1200
Here's a screenshot from my testing of the latest Vercel Preview of this PR.
The backstory of this screenshot is that I am testing by keeping my browser window open to this page and then just waiting for the issue to reproduce itself. https://adminensnode-cbuey61q7-namehash.vercel.app/status?connection=https%3A%2F%2Fapi.alpha.green.ensnode.io%2F
What's happening is that the indexing status is loaded into memory and therefore the "Status" page is correctly rendering itself.
Then in the background, the requests for indexing status are happening automatically every few seconds..
- Indexing status request successful: UI auto-updates.
- Indexing status request successful: UI auto-updates.
- Indexing status request successful: UI auto-updates.
- Etc..
- Etc..
- then BAM an error when requesting the next indexing status:
which then causes this to appear in the UI:
This needs to be completely impossible. We should continue using the existing snapshot that's already cached into memory. It's still perfectly good!
There was a problem hiding this comment.
This shouldn't happen anymore.
There was a problem hiding this comment.
I was going to ditch this and use useIntervalWhen from rooks inside ensadmin, but rooks is a dependency of ensadmin and not ensnode-react. Thinking...
|
|
||
| updateTime(); | ||
|
|
||
| if (includeSeconds) { |
There was a problem hiding this comment.
We could use rooks here I guess with useIntervalWhen for cleaner code. Since this component is used elsewhere, I'm not sure if the original implementation was designed without this improvement.
Co-authored-by: lightwalker.eth <126201998+lightwalker-eth@users.noreply.github.com>
tk-o
force-pushed
the
cached-snapshot
branch
from
10dc7fd to
ea42b6c
Compare
When the API fails but we have a cached snapshot, users see the last known good data with a slightly degrading projection, rather than an error message. Closes #1200
Todos