chore: demo features script deloyer#356
Conversation
ashuralyk
commented
Jan 27, 2026
ashuralyk
commented
- I have read the Contributing Guidelines
|
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @ashuralyk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new "Deploy Script" tool into the demo application, empowering users to interact with the CKB blockchain by deploying files as Type ID cells. This enhancement provides a robust mechanism for both initial deployment and subsequent updates of on-chain data, complete with comprehensive UI feedback and validation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for deploying scripts, which is a great addition to the demo application. The main component, DeployScript, is well-structured but has grown quite large. For future maintainability, consider breaking it down into smaller components and custom hooks. For instance, the file upload UI and the Type ID checking logic could be extracted. I've provided a couple of specific suggestions to improve error handling and reduce code duplication in the new component. The other changes in the pull request are solid, including a necessary fix in the Message component to ensure valid HTML.
| signer.getRecommendedAddress().then((addr) => { | ||
| setUserAddress(addr); | ||
| }); |
There was a problem hiding this comment.
The promise returned by signer.getRecommendedAddress() doesn't handle potential rejections. If this promise fails, it will result in an unhandled promise rejection in the application. It's a good practice to add a .catch() block to handle such errors gracefully, for instance by logging the error and notifying the user.
signer.getRecommendedAddress().then((addr) => {
setUserAddress(addr);
}).catch((e) => {
console.error("Failed to get recommended address:", e);
error("Failed to get user address.");
});
| try { | ||
| const typeIdBytes = ccc.bytesFrom(normalizedTypeIdArgs); | ||
| if (typeIdBytes.length !== 32) { | ||
| setFoundCell(null); | ||
| setFoundCellAddress(""); | ||
| setIsAddressMatch(null); | ||
| setCellCheckError( | ||
| "Type ID args must be 32 bytes (64 hex characters)", | ||
| ); | ||
| isCheckingRef.current = false; | ||
| return; | ||
| } | ||
| } catch { | ||
| setFoundCell(null); | ||
| setFoundCellAddress(""); | ||
| setIsAddressMatch(null); | ||
| setCellCheckError("Invalid Type ID args format"); | ||
| isCheckingRef.current = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The state reset logic for foundCell, foundCellAddress, etc., is duplicated in this try...catch block and in other places within the useEffect hook. This duplication makes the code harder to read and maintain. You can refactor this by consolidating the error handling and state reset logic into a single catch block, which will make the code cleaner and less error-prone.
try {
const typeIdBytes = ccc.bytesFrom(normalizedTypeIdArgs);
if (typeIdBytes.length !== 32) {
throw new Error(
"Type ID args must be 32 bytes (64 hex characters)",
);
}
} catch (e: unknown) {
const message =
e instanceof Error && e.message.startsWith("Type ID")
? e.message
: "Invalid Type ID args format";
setFoundCell(null);
setFoundCellAddress("");
setIsAddressMatch(null);
setCellCheckError(message);
isCheckingRef.current = false;
return;
}
| } | ||
| > | ||
| <p className={`text-sm break-all whitespace-pre-wrap ${colorClass}`}> | ||
| <div className={`text-sm break-all whitespace-pre-wrap ${colorClass}`}> |
There was a problem hiding this comment.
I'm wondering: why do we need to change this element?
There was a problem hiding this comment.
Because embedding div under a tag p would trigger warning prompt in Application, so I made this change for better compatibility, and indeed it doesn't bring other problems to other components.
Though I can understand this, it looks weird on my side. I think it needs some visual adjustment.
|
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
This new tool overall looks good to me. The problem is that the code seems overcomplicated (640-line function). While it brings a fancy visual effect, I suggest refactoring this code to make it less coupled.
| size="sm" | ||
| iconName="FileCode" | ||
| onClick={onSelect} | ||
| className={isSelected ? "border-purple-500 bg-purple-50" : ""} |
There was a problem hiding this comment.
The bg-purple-500 seems not working.
| }, [signer]); | ||
|
|
||
| // Compare addresses when both are available | ||
| useEffect(() => { |
There was a problem hiding this comment.
It's better to use useMemo here.
| }`} | ||
| onDragOver={handleDragOver} | ||
| onDragLeave={handleDragLeave} | ||
| onDrop={handleDrop} |
There was a problem hiding this comment.
I suggest moving this logic into a separate component to reduce the complexity.
| }); | ||
| } | ||
|
|
||
| function ConfirmationModal({ |
There was a problem hiding this comment.
Honestly, I love this popup, which makes transaction confirmation look cool. Meanwhile, it seems better to extract this part into a standalone component and move the confirmation-related logic into it to improve cohesion.
|
/gemini-review |
|
Hello @ashuralyk, I see you've requested a code review with |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Deploy Script' feature, which is a significant addition. The implementation is well-structured, separating UI components, business logic, and state management into different files and a custom hook. The code is generally clean and follows good React practices. I've identified a couple of areas for improvement related to performance and code maintainability, with specific suggestions to address them.
| setIsWaitingConfirmation(false); | ||
| setConfirmationMessage(""); | ||
| setConfirmationTxHash(""); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| error("Deployment failed:", msg); | ||
| setIsWaitingConfirmation(false); | ||
| setConfirmationMessage(""); | ||
| setConfirmationTxHash(""); | ||
| } finally { | ||
| setIsDeploying(false); | ||
| } |
There was a problem hiding this comment.
There's duplicated code for resetting state in the try and catch blocks. To improve maintainability and ensure state is always reset correctly, you can move all state updates to the finally block. This centralizes the cleanup logic.
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
error("Deployment failed:", msg);
} finally {
setIsDeploying(false);
setIsWaitingConfirmation(false);
setConfirmationMessage("");
setConfirmationTxHash("");
}
| (async () => { | ||
| try { | ||
| const { script: lock } = await signer.getRecommendedAddressObj(); | ||
| const typeIdScript = await ccc.Script.fromKnownScript( | ||
| signer.client, | ||
| ccc.KnownScript.TypeId, | ||
| "", | ||
| ); | ||
| const cells: ccc.Cell[] = []; | ||
| for await (const cell of signer.client.findCells({ | ||
| script: typeIdScript, | ||
| scriptType: "type", | ||
| scriptSearchMode: "prefix", | ||
| withData: true, | ||
| filter: { script: lock }, | ||
| })) { | ||
| cells.push(cell); | ||
| setTypeIdCells([...cells]); | ||
| } | ||
| } catch { | ||
| // ignore | ||
| } finally { | ||
| setIsScanningCells(false); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Calling setTypeIdCells inside the for await...of loop causes a React re-render for every cell found. This can be inefficient if there are many cells. It's better to collect all cells into an array first and then call setTypeIdCells once after the loop to update the state.
(async () => {
try {
const { script: lock } = await signer.getRecommendedAddressObj();
const typeIdScript = await ccc.Script.fromKnownScript(
signer.client,
ccc.KnownScript.TypeId,
"",
);
const cells: ccc.Cell[] = [];
for await (const cell of signer.client.findCells({
script: typeIdScript,
scriptType: "type",
scriptSearchMode: "prefix",
withData: true,
filter: { script: lock },
})) {
cells.push(cell);
}
setTypeIdCells(cells);
} catch {
// ignore
} finally {
setIsScanningCells(false);
}
})();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Deploy Script" feature, allowing users to deploy files as CKB cells with a Type ID. The implementation is well-structured, separating UI components, business logic, and hooks into distinct files. It also adds a reusable file upload component and enhances the existing Message component. The overall approach is solid, and I've identified a minor logic issue in the useDeployScript hook that could be refactored for improved clarity and robustness.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Deploy Script" tool that allows users to upload and deploy files as CKB cells with the Type ID trait, as well as update or burn existing Type ID cells. It includes supporting UI components, helper utilities, custom hooks, a reusable file upload area, and transaction confirmation modals. The review feedback highlights three key issues: a critical bug in the deployment logic where failing to call completeInputs leaves the cell capacity at an invalid ccc.Zero, a bug in the debounced Type ID cell check hook that ignores user keystrokes during active checks, and a potential race condition in the cell scanning effect that could overwrite state with stale results.
| tx = ccc.Transaction.from({ | ||
| inputs: [{ previousOutput: foundCell.outPoint }], | ||
| outputs: [ | ||
| { | ||
| ...foundCell.cellOutput, | ||
| capacity: ccc.Zero, | ||
| }, | ||
| ], | ||
| outputsData: [fileBytes], | ||
| }); | ||
| typeIdArgsValue = normalized; |
There was a problem hiding this comment.
In the update path of runDeploy, the transaction is built with capacity: ccc.Zero but completeInputs is never called. This will result in the output cell keeping a capacity of ccc.Zero, which is invalid on CKB and will cause the transaction to fail. Additionally, if the new file size is larger than the original cell's capacity, the transaction will fail due to insufficient capacity. Calling completeInputs ensures that the output capacity is set to its minimum required occupied capacity, and adds more inputs from the signer if needed.
| tx = ccc.Transaction.from({ | |
| inputs: [{ previousOutput: foundCell.outPoint }], | |
| outputs: [ | |
| { | |
| ...foundCell.cellOutput, | |
| capacity: ccc.Zero, | |
| }, | |
| ], | |
| outputsData: [fileBytes], | |
| }); | |
| typeIdArgsValue = normalized; | |
| tx = ccc.Transaction.from({ | |
| inputs: [{ previousOutput: foundCell.outPoint }], | |
| outputs: [ | |
| { | |
| ...foundCell.cellOutput, | |
| capacity: ccc.Zero, | |
| }, | |
| ], | |
| outputsData: [fileBytes], | |
| }); | |
| await tx.completeInputs(signer); | |
| typeIdArgsValue = normalized; |
| useEffect(() => { | ||
| const normalized = normalizeTypeIdArgs(typeIdArgs); | ||
|
|
||
| if (lastCheckedTypeIdRef.current === normalized || isCheckingRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| if (!typeIdArgs.trim()) { | ||
| lastCheckedTypeIdRef.current = ""; | ||
| resetCellCheckState(); | ||
| return; | ||
| } | ||
|
|
||
| const timeoutId = setTimeout(async () => { | ||
| if (isCheckingRef.current) return; | ||
| isCheckingRef.current = true; | ||
| lastCheckedTypeIdRef.current = normalized; | ||
|
|
||
| try { | ||
| const typeIdBytes = ccc.bytesFrom(normalized); | ||
| if (typeIdBytes.length !== 32) { | ||
| resetCellCheckState( | ||
| "Type ID args must be 32 bytes (64 hex characters)", | ||
| ); | ||
| isCheckingRef.current = false; | ||
| return; | ||
| } | ||
| } catch { | ||
| resetCellCheckState("Invalid Type ID args format"); | ||
| isCheckingRef.current = false; | ||
| return; | ||
| } | ||
|
|
||
| setIsCheckingCell(true); | ||
| setCellCheckError(""); | ||
|
|
||
| try { | ||
| const typeIdScript = await ccc.Script.fromKnownScript( | ||
| client, | ||
| ccc.KnownScript.TypeId, | ||
| normalized, | ||
| ); | ||
| const cell = await client.findSingletonCellByType(typeIdScript, true); | ||
|
|
||
| if (cell) { | ||
| setFoundCell(cell); | ||
| const address = ccc.Address.fromScript( | ||
| cell.cellOutput.lock, | ||
| client, | ||
| ).toString(); | ||
| setFoundCellAddress(address); | ||
| setCellCheckError(""); | ||
| } else { | ||
| resetCellCheckState("Type ID cell not found on-chain"); | ||
| } | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| resetCellCheckState(`Error checking Type ID: ${msg}`); | ||
| } finally { | ||
| setIsCheckingCell(false); | ||
| isCheckingRef.current = false; | ||
| } | ||
| }, 500); | ||
|
|
||
| return () => clearTimeout(timeoutId); | ||
| }, [typeIdArgs, client, resetCellCheckState]); |
There was a problem hiding this comment.
The debounced Type ID cell check hook has a bug where keystrokes typed while a check is in progress (isCheckingRef.current === true) are completely ignored. This causes the UI to get stuck and fail to update when the user types quickly. Replacing the ref-based lock with a standard React active flag cleanup pattern ensures that stale requests are discarded while new inputs are always correctly scheduled and resolved.
useEffect(() => {
const normalized = normalizeTypeIdArgs(typeIdArgs);
if (lastCheckedTypeIdRef.current === normalized) {
return;
}
if (!typeIdArgs.trim()) {
lastCheckedTypeIdRef.current = "";
resetCellCheckState();
return;
}
let active = true;
const timeoutId = setTimeout(async () => {
lastCheckedTypeIdRef.current = normalized;
try {
const typeIdBytes = ccc.bytesFrom(normalized);
if (typeIdBytes.length !== 32) {
if (active) {
resetCellCheckState(
"Type ID args must be 32 bytes (64 hex characters)",
);
}
return;
}
} catch {
if (active) {
resetCellCheckState("Invalid Type ID args format");
}
return;
}
if (active) {
setIsCheckingCell(true);
setCellCheckError("");
}
try {
const typeIdScript = await ccc.Script.fromKnownScript(
client,
ccc.KnownScript.TypeId,
normalized,
);
const cell = await client.findSingletonCellByType(typeIdScript, true);
if (!active) return;
if (cell) {
setFoundCell(cell);
const address = ccc.Address.fromScript(
cell.cellOutput.lock,
client,
).toString();
setFoundCellAddress(address);
setCellCheckError("");
} else {
resetCellCheckState("Type ID cell not found on-chain");
}
} catch (err) {
if (!active) return;
const msg = err instanceof Error ? err.message : String(err);
resetCellCheckState(`Error checking Type ID: ${msg}`);
} finally {
if (active) {
setIsCheckingCell(false);
}
}
}, 500);
return () => {
active = false;
clearTimeout(timeoutId);
};
}, [typeIdArgs, client, resetCellCheckState]);| useEffect(() => { | ||
| if (!signer) { | ||
| setTypeIdCells([]); | ||
| return; | ||
| } | ||
| setIsScanningCells(true); | ||
| (async () => { | ||
| try { | ||
| const { script: lock } = await signer.getRecommendedAddressObj(); | ||
| const typeIdScript = await ccc.Script.fromKnownScript( | ||
| signer.client, | ||
| ccc.KnownScript.TypeId, | ||
| "", | ||
| ); | ||
| const cells: ccc.Cell[] = []; | ||
| for await (const cell of signer.client.findCells({ | ||
| script: typeIdScript, | ||
| scriptType: "type", | ||
| scriptSearchMode: "prefix", | ||
| withData: true, | ||
| filter: { script: lock }, | ||
| })) { | ||
| cells.push(cell); | ||
| } | ||
| setTypeIdCells(cells); | ||
| } catch { | ||
| // ignore | ||
| } finally { | ||
| setIsScanningCells(false); | ||
| } | ||
| })(); | ||
| }, [signer, refreshTrigger]); |
There was a problem hiding this comment.
The useEffect hook that scans for Type ID cells does not handle potential race conditions. If the signer or refreshTrigger changes while a scan is in progress, the stale asynchronous scan could finish later and overwrite the state with outdated cells. Adding an active flag to ignore stale results prevents this race condition.
useEffect(() => {
if (!signer) {
setTypeIdCells([]);
return;
}
setIsScanningCells(true);
let active = true;
(async () => {
try {
const { script: lock } = await signer.getRecommendedAddressObj();
if (!active) return;
const typeIdScript = await ccc.Script.fromKnownScript(
signer.client,
ccc.KnownScript.TypeId,
"",
);
if (!active) return;
const cells: ccc.Cell[] = [];
for await (const cell of signer.client.findCells({
script: typeIdScript,
scriptType: "type",
scriptSearchMode: "prefix",
withData: true,
filter: { script: lock },
})) {
if (!active) return;
cells.push(cell);
}
if (!active) return;
setTypeIdCells(cells);
} catch {
// ignore
} finally {
if (active) {
setIsScanningCells(false);
}
}
})();
return () => {
active = false;
};
}, [signer, refreshTrigger]);2 participants
