-
Notifications
You must be signed in to change notification settings - Fork 1
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
Handle unsupported chain deployments #282
Handle unsupported chain deployments #282
Conversation
…upported chain Signed-off-by: Aaron Sutula <asutula@users.noreply.github.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
{!deployment && ( | ||
<NeedsDeploy def={def} env={env} isAuthorized={isAuthorized} /> | ||
)} |
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.
Just noticed this noop conditional check that is always true
.
return ( | ||
<div className="p-4"> | ||
<p>Error getting table by id</p> | ||
<p>Error getting table by id: {ensureError(err).message}</p> |
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.
Pretty simple, just display the error message.
if (err instanceof ApiError && err.status === 404) { | ||
return ( | ||
<div className="p-4"> | ||
<p> | ||
Table id {tableId} not found on chain {chainId}. | ||
</p> | ||
</div> | ||
); | ||
} | ||
console.error("Error getting table by id:", { err }); |
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.
This wasn't really adding anything, just extra code.
let data: Result<Record<string, unknown>> | undefined; | ||
let error: Error | undefined; | ||
try { | ||
const baseUrl = helpers.getBaseUrl(chainId); | ||
const tbl = new Database({ baseUrl }); | ||
data = await tbl.prepare(`SELECT * FROM ${tableName};`).all(); | ||
} catch (err) { | ||
error = ensureError(err); | ||
} |
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.
Main change here is to make data
optional and save any error that happens. Then, we can render UI conditionally based on if we have data or not. UI can still be rendered for elements that don't depend on the table data. If an error exists, we can display information about it.
const formattedData = data ? objectToTableData(data.results) : undefined; | ||
const columns: Array<ColumnDef<unknown>> | undefined = data | ||
? data.results.length | ||
? Object.keys(data.results[0] as object).map((col) => ({ | ||
accessorKey: col, | ||
header: col, | ||
})) | ||
: [] | ||
: undefined; |
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.
Making these optional as well since they're derived from data
and to support easy conditional rendering.
Signed-off-by: Aaron Sutula <asutula@users.noreply.github.com>
Signed-off-by: Aaron Sutula <asutula@users.noreply.github.com>
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.
Nice!
There are two cases where this is relevant:
/table/<table id>
urlIn both cases, the table being loaded might be for a chain we no longer support.
In the first case, it's as simple as just displaying the error message that says "unsupported chain id".
In the second case, we still need to display enough UI to convey the error message and also give the user a way to delete the now-invalid deployment by un-deploying the table. The solution looks something like this:
(Note, I forced the UI into an error state, so some of the surrounding data will not be what you expect in a real scenario)