Debug tab manual block number modal #1729
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive debug data workflow for deployment scenarios in a Rainlang-based application. It adds new Rust data structures and methods for evaluating deployments at specific block numbers, enables setting and managing block numbers for different networks, and implements a shared state mechanism for concurrent fuzzing operations. The UI is refactored to provide interactive controls for block height selection, real-time scenario debugging, and detailed result visualization. New Svelte modal components are introduced for block number and scenario debugging, and the service layer is updated to support the new commands and data models. Test coverage and error handling are expanded in the core logic. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant ModalDebugBlockNumber
participant ChartService
participant TauriBackend
participant SharedState
participant FuzzRunner
UI->>ModalDebugBlockNumber: Open modal to set block numbers
ModalDebugBlockNumber-->>UI: User sets block numbers for networks
UI->>ChartService: makeDeploymentsDebugDataMap(dotrain, settings, blockNumbers)
ChartService->>TauriBackend: invoke('make_deployment_debug', params)
TauriBackend->>SharedState: lock debug_runner
SharedState->>FuzzRunner: run make_debug_data(context, blockNumbers)
FuzzRunner-->>SharedState: Return DeploymentsDebugDataMap
SharedState-->>TauriBackend: DeploymentsDebugDataMap
TauriBackend-->>ChartService: DeploymentsDebugDataMap
ChartService-->>UI: Display debug data in ScenarioDebugTable
UI->>ModalScenarioDebug: Open modal for scenario debug details
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
crates/common/src/fuzz/mod.rs (1)
44-80: 🧹 Nitpick (assertive)Great addition – but consider deriving
Clone,PartialEqfor ergonomicsThe new debug data structs look solid and are well annotated for WASM. Adding
Clone,PartialEq(and possiblyEq) derives is essentially free and unlocks:
- easy test assertions (
assert_eq!)- cheap value cloning when passing across threads/channels
-#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]for each of the three structs.
🛑 Comments failed to post (13)
tauri-app/src/lib/components/modal/ModalScenarioDebug.svelte (1)
10-16: 🧹 Nitpick (assertive)
Consider removing empty div elements
There are empty divs at lines 12 and 15 that don't contain any content. If they're intended as placeholders for future content, consider adding a comment explaining their purpose, otherwise they can be removed.
<Modal title={`Debugging scenario for pair ${pair}`} bind:open outsideclose size="lg"> - <div class="flex items-center"> - <div class="flex w-full items-center justify-end"></div> - </div> <EvalResultsTable table={data} /> - <div class="flex flex-col gap-y-2 text-sm"></div> </Modal>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<Modal title={`Debugging scenario for pair ${pair}`} bind:open outsideclose size="lg"> <EvalResultsTable table={data} /> </Modal>packages/ui-components/src/lib/components/EditableSpan.svelte (1)
24-41: 🧹 Nitpick (assertive)
Consider addressing the accessibility warning
The component has an
a11y-no-static-element-interactionswarning that's being ignored. Consider addressing this accessibility issue by using a semantic element likebuttonor adding appropriate ARIA attributes.<!-- svelte-ignore a11y-no-static-element-interactions --> -<div +<button data-testid="editableSpanWrapper" class="flex items-center gap-x-1 border-b-2 border-dotted text-sm text-gray-400 dark:text-gray-400" + aria-label="Edit block number" > <PenSolid class="h-3 w-3 cursor-pointer" /> <span>Block:</span> <div class="relative"> <span data-testid="editableSpan" bind:this={editableSpan} bind:textContent contenteditable="true" on:keydown={handleKeyDown} on:blur={inputBlurred}>{displayValue}</span > </div> -</div> +</button>Alternatively, if you want to keep it as a
div, add proper role and keyboard event handlers:<!-- svelte-ignore a11y-no-static-element-interactions --> <div data-testid="editableSpanWrapper" class="flex items-center gap-x-1 border-b-2 border-dotted text-sm text-gray-400 dark:text-gray-400" + role="button" + tabindex="0" + aria-label="Edit block number" + on:keydown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + editableSpan.focus(); + } + }} >tauri-app/src/lib/components/ScenarioDebugTable.svelte (4)
127-168: 🧹 Nitpick (assertive)
Add
keyprops to inner{#each}blocksWithout
key, Svelte’s diffing may recycle DOM nodes causing flickering whenresults.pairsDataorder changes on refresh.{#each results.pairsData as item (item.pair)} … {/each}
29-37: 🛠️ Refactor suggestion
⚠️ Potential issueStabilise
queryKeystore to avoid stale subscriptions & memory churn
queryKeyis recreated on every reactive run ($: queryKey = writable(...)).
Down-stream consumers (fileUpdate,createQuery, etc.) keep references to the old store, so subsequent.setcalls affect an orphaned instance and the query never re-runs. In addition this leaks one store per update cycle.-// ❌ recreated on every reactive pass -$: queryKey = writable([$globalDotrainFile.text, $settingsText]); +// ✅ create once … +const queryKey = writable([$globalDotrainFile.text, $settingsText]); + +// …and just update its value reactively +$: queryKey.set([$globalDotrainFile.text, $settingsText]);
fileUpdatecan then be simplified toqueryKey.set([dotrain, settings]);.Also applies to: 51-56
134-145:
⚠️ Potential issueGuard against empty data to prevent runtime errors
transformData(fuzzResult)[0]assumes a non-empty array; iftransformDatareturns[]the subsequent.entries,.filter, etc. will throw.-{@const data = transformData(fuzzResult)[0]} +{@const _td = transformData(fuzzResult)} +{#if _td.length === 0} + <TableBodyCell colspan="4" class="text-red-500"> + transformData returned no rows + </TableBodyCell> +{:else} + {@const data = _td[0]} + <!-- existing logic --> +{/if}Committable suggestion skipped: line range outside the PR's diff.
44-47: 💡 Verification agent
🛠️ Refactor suggestion
❓ Verification inconclusive
Ensure Svelte reactivity when mutating
blockNumbersDirectly mutating the object does not trigger a rerender; components bound to
blockNumbers(e.g.ModalDebugBlockNumber) will therefore receive stale data.- blockNumbers[res.dataMap[deploymentKey].chainId] = - res.dataMap[deploymentKey].blockNumber || 0; + blockNumbers = { + ...blockNumbers, + [res.dataMap[deploymentKey].chainId]: + res.dataMap[deploymentKey].blockNumber ?? 0, + };Make sure the modal now reflects live updates.
Ensure Svelte reactivity when updating
blockNumbersFile:
tauri-app/src/lib/components/ScenarioDebugTable.svelte
Lines: 44–47Directly mutating an object’s property does not trigger a Svelte rerender, so any components bound to
blockNumbers(e.g.ModalDebugBlockNumber) will see stale data. Update the loop to reassign the entire object instead of mutating it in place:for (const deploymentKey in res.dataMap) { - blockNumbers[res.dataMap[deploymentKey].chainId] = - res.dataMap[deploymentKey].blockNumber || 0; + blockNumbers = { + ...blockNumbers, + [res.dataMap[deploymentKey].chainId]: + res.dataMap[deploymentKey].blockNumber ?? 0, + }; }Confirm that the modal now reflects live updates.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (const deploymentKey in res.dataMap) { blockNumbers = { ...blockNumbers, [res.dataMap[deploymentKey].chainId]: res.dataMap[deploymentKey].blockNumber ?? 0, }; }tauri-app/src/routes/orders/add/+page.svelte (2)
212-230: 🧹 Nitpick (assertive)
Safeguard
chainsiteration to skip non-chain exports
Object.values(chains)also contains helper functions (defineChain, etc.) whoseidproperty isundefined. Accessing.nameon those can raise a type error under"noUncheckedIndexedAccess".-const networkKey = - Object.values(chains).find((v) => v.id === chainId)?.name ?? +const networkKey = + Object.values(chains) + .filter((v: any): v is { id: number; name: string } => typeof v === 'object' && 'id' in v) + .find((v) => v.id === chainId)?.name ??Alternatively import only the required chain objects:
import { mainnet, sepolia, … } from 'viem/chains';
308-309: 🧹 Nitpick (assertive)
Use one-way prop instead of two-way binding for
networks
ScenarioDebugTablenever mutatesnetworks, yet the parent usesbind:networks, implying two-way binding and creating misleading API surface.-<TabItem title="Debug"><ScenarioDebugTable bind:networks={deploymentNetworks} /></TabItem> +<TabItem title="Debug"><ScenarioDebugTable {deploymentNetworks} /></TabItem>(or
networks={deploymentNetworks}if the prop is still namednetworks)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<TabItem title="Debug"><ScenarioDebugTable {deploymentNetworks} /></TabItem> <TabItem title="Charts">tauri-app/src/lib/components/modal/ModalDebugBlockNumber.svelte (2)
18-25: 🛠️ Refactor suggestion
Incorrect Tailwind/Flowbite props:
z-50000&outsidecloseare likely ineffective
z-50000is not part of default Tailwind’sz-indexscale, and Flowbite-Svelte’sModalprop isoutsideClose, notoutsideclose.
Unexpected class / prop names silently fall through, so the modal may render behind other overlays and clicking the backdrop might never close the dialog.-<Modal - class="z-50000" - … - outsideclose +<Modal + class="z-[50000]" /* arbitrary value, works with Tailwind JIT */ + … + outsideClose /* camel-case prop defined by Flowbite-Svelte */
38-50: 🛠️ Refactor suggestion
Three issues in the input handler
placeHolder→placeholder(HTML is lowercase).parseInt(... ) || undefinedtreats “0” as invalid because0is falsy.- Mutating
blockNumbers[...] = …does not trigger Svelte reactivity for the parent, as the reference doesn’t change.-<Input - type="number" - size="sm" - class="self-center" - placeHolder="Enter Block Height" - value={blockNumbers[Number(chainId)]} - on:input={(e) => { - if (e.currentTarget instanceof HTMLInputElement) { - blockNumbers[Number(chainId)] = - parseInt(e.currentTarget.value, 10) || undefined; - } - }} -></Input> +<Input + type="number" + size="sm" + class="self-center" + placeholder="Enter Block Height" + value={blockNumbers[Number(chainId)]} + on:input={(e) => { + const v = parseInt(e.currentTarget.value, 10); + blockNumbers[Number(chainId)] = + Number.isNaN(v) ? undefined : v; + + /* ensure reactivity for parent consumers */ + blockNumbers = { ...blockNumbers }; + }} +/>crates/common/src/fuzz/impls.rs (3)
346-349: 🧹 Nitpick (assertive)
Potential RNG contention in multi-thread tests
context.rng.fill_bytesis executed in the parent task and therefore safe, but if future code moves the RNG use inside the spawned task it will introduce unsynchronised access toTestRng, which is notSync. Consider encapsulating RNG inside the context and exposing anext_random()method that takes&mut selfexplicitly, avoiding accidental sharing across threads.
420-429: 🧹 Nitpick (assertive)
Shadowing
contexthides the function parameter and hurts readabilityInside
run_debuga local variable namedcontext(the 5×5 grid) shadows the&mut FuzzRunnerContextparameter. This makes the code harder to follow and is a potential foot-gun for future refactors.- let mut context = vec![vec![U256::from(0); 5]; 5]; + let mut eval_ctx = vec![vec![U256::from(0); 5]; 5]; … - context[1][0] = rand::random(); + eval_ctx[1][0] = rand::random(); … - context[3][0] = … + eval_ctx[3][0] = …Then pass
eval_ctxtofork_parse/eval3Call.Committable suggestion skipped: line range outside the PR's diff.
537-544: 🧹 Nitpick (assertive)
unwrap_or(&HashMap::new())allocates every loop iteration
unwrap_ortakes its argument eagerly, so you build a brand-newHashMapfor every deployment that already hasblock_numbers. Useunwrap_or_default()on theOption<&HashMap>or match once outside the loop to avoid needless allocations.-let block_number = if let Some(cached_block_number) = block_numbers - .as_ref() - .unwrap_or(&HashMap::new()) - .get(&result.chain_id) -{ +let block_number = if let Some(bn_map) = block_numbers.as_ref() { + if let Some(cached) = bn_map.get(&result.chain_id) { + *cached + } else { + fetch_latest_block(&scenario).await? // helper fn + } +} else { + fetch_latest_block(&scenario).await? +};This small tweak removes the per-iteration allocation and makes the intent clearer.
Committable suggestion skipped: line range outside the PR's diff.
rouzwelt
changed the title
Motivation
resolves #1728
This is a follow up for #1599 and #1727
This PR provides ability for user to set desired debug block numbers for each chain of deployments on a modal that pops up by clicking on the
editicon next torefershandplay/pauseicons on debug tab.Solution
ModalDebugContext.sveltewhich is modal that pops up with fields to set block numbers for each chain to run the debug atScenarioDebugTable.svelteto host the modal with a flowbiteediticon to open it upadd orderpage where most of changes to the detrain text is handled, now added a reactive element that gathers the networks from available deployments and passes them downstreamScreen Recording
Screen.Recording.2025-05-05.at.1.53.05.AM.mov
Screen.Recording.2025-05-05.at.3.04.15.AM.mov
Checks
By submitting this for review, I'm confirming I've done the following: