Conversation
Collaborator
Author
|
@coderabbitai review |
Collaborator
Author
|
@CodeRabbit review |
Resolves #2554
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughAdds a new Rollup feature config field 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
deploy/tools/envs-validator/test/.env.optimism (1)
9-10: Clean up dotenv-linter warnings in this fixture file.Lines 9–10 are functionally fine, but current ordering/newline warnings will keep lint noisy. Please reorder keys per project convention and ensure a trailing newline at EOF.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/tools/envs-validator/test/.env.optimism` around lines 9 - 10, Reorder the environment keys so they follow project convention by placing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER before NEXT_PUBLIC_ROLLUP_STAGE_INDEX, and ensure the fixture file ends with a single trailing newline (EOF newline) to silence dotenv-linter ordering and newline warnings; update the .env fixture containing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER and NEXT_PUBLIC_ROLLUP_STAGE_INDEX accordingly.docs/ENVS.md (1)
532-532: Normalize emphasis style on Line 532 to satisfy markdownlint (MD049).Switch underscore emphasis to asterisk emphasis in this row to clear the docs lint warning.
✏️ Suggested docs tweak
-| NEXT_PUBLIC_ROLLUP_L1_BASE_URL | `string` | Blockscout base URL for parent network. **DEPRECATED** _Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead_ | Required | - | `'http://eth-goerli.blockscout.com'` | v1.24.0+ | +| NEXT_PUBLIC_ROLLUP_L1_BASE_URL | `string` | Blockscout base URL for parent network. **DEPRECATED** *Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead* | Required | - | `'http://eth-goerli.blockscout.com'` | v1.24.0+ |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ENVS.md` at line 532, The table row for NEXT_PUBLIC_ROLLUP_L1_BASE_URL uses underscore emphasis around "Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead" which triggers MD049; update that cell to use asterisk emphasis (i.e. replace _Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead_ with *Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead*) so the markdown emphasis style matches the rest of the document; locate the row by the symbol NEXT_PUBLIC_ROLLUP_L1_BASE_URL and make this single-token emphasis change.lib/rollups/utils.ts (1)
17-37: Add explicit return types to exported top-level formatters.Lines 17, 26, and 35 are exported top-level module functions and should declare return types explicitly for consistency and safer API surfaces.
✍️ Suggested refactor
-export const formatZkEvmTxStatus = (status: typeof ZKEVM_L2_TX_STATUSES[number]) => { +export const formatZkEvmTxStatus = (status: typeof ZKEVM_L2_TX_STATUSES[number]): string => { @@ -export const formatZkEvmL2TxnBatchStatus = (status: typeof ZKEVM_L2_TX_BATCH_STATUSES[number]) => { +export const formatZkEvmL2TxnBatchStatus = (status: typeof ZKEVM_L2_TX_BATCH_STATUSES[number]): string => { @@ -export const formatZkSyncL2TxnBatchStatus = (status: typeof ZKSYNC_L2_TX_BATCH_STATUSES[number]) => { +export const formatZkSyncL2TxnBatchStatus = (status: typeof ZKSYNC_L2_TX_BATCH_STATUSES[number]): string => {As per coding guidelines, "Declare return types on top-level module functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rollups/utils.ts` around lines 17 - 37, The three exported top-level formatters (formatZkEvmTxStatus, formatZkEvmL2TxnBatchStatus, formatZkSyncL2TxnBatchStatus) lack explicit return types; update each function signature to declare a return type of string (e.g. change to formatZkEvmTxStatus(...): string) so the module surface is explicit and type-safe, keeping function bodies unchanged.deploy/tools/llms-txt-generator/index.ts (1)
29-30: Redundant ternary —layerLabelsalways returns defined strings.
layerLabelsalready falls back to{ current: 'L2', parent: 'L1' }when rollup is disabled (as seen inlib/rollups/utils.tslines 8–14), so therollupFeature.isEnabled ? ... : undefinedguard is unnecessary. The result ofundefinedis also safe here since all consuming templates are independently guarded byrollupFeature.isEnabled, but the ternary adds noise.♻️ Proposed simplification
- const currentToParentLayerLabel = rollupFeature.isEnabled ? layerLabels.current + '→' + layerLabels.parent : undefined; - const parentToCurrentLayerLabel = rollupFeature.isEnabled ? layerLabels.parent + '→' + layerLabels.current : undefined; + const currentToParentLayerLabel = `${ layerLabels.current }→${ layerLabels.parent }`; + const parentToCurrentLayerLabel = `${ layerLabels.parent }→${ layerLabels.current }`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/tools/llms-txt-generator/index.ts` around lines 29 - 30, The two ternary expressions creating currentToParentLayerLabel and parentToCurrentLayerLabel are redundant because layerLabels already provides defaults; remove the rollupFeature.isEnabled ? ... : undefined guards and assign the concatenated strings directly (use layerLabels.current + '→' + layerLabels.parent and layerLabels.parent + '→' + layerLabels.current) so the variables are always defined and simpler; keep any external checks that guard template usage based on rollupFeature.isEnabled intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/tools/envs-validator/schemas/features/rollup.ts`:
- Around line 172-184: Refactor the Yup conditional on
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER to use the callback form of when instead of the
object-with-then property to satisfy the lint rule; change the
when('NEXT_PUBLIC_ROLLUP_TYPE', { is: ..., then: ..., otherwise: ... }) usage
for NEXT_PUBLIC_ROLLUP_LAYER_NUMBER so the predicate and branch logic are
expressed in the (value, schema) => { ... } callback, preserving behavior: when
NEXT_PUBLIC_ROLLUP_TYPE is truthy return the unchanged schema, otherwise return
a schema.test that enforces the value is undefined with the same error message;
apply the same callback-style fix to similar fields like
NEXT_PUBLIC_ROLLUP_STAGE_INDEX and other instances across ui.ts and services.ts.
In `@lib/rollups/utils.ts`:
- Around line 35-36: The current formatZkSyncL2TxnBatchStatus replaces 'L1' then
'L2' sequentially which can clobber earlier replacements (e.g.
current=L3,parent=L2). Change the function to do a single-pass replacement using
a regex with word boundaries (e.g. /\bL[12]\b/g) and a replacer callback that
maps 'L1' -> layerLabels.parent and 'L2' -> layerLabels.current (reference
symbols: formatZkSyncL2TxnBatchStatus, ZKSYNC_L2_TX_BATCH_STATUSES,
layerLabels).
- Around line 10-11: Add the missing test preset for the
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER environment variable to the test baseline so
validation and tests have a concrete value; update
deploy/tools/envs-validator/test/.env.base to include a line like
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER=2 (or 3) to match the schema's .min(2)
constraint and prevent UI label degradation when code paths referencing layer
numbers (e.g., the L${ feature.layerNumber } / parent L${ feature.layerNumber -
1 } logic in lib/rollups/utils.ts) run in tests.
In `@ui/pages/ZkEvmL2Withdrawals.tsx`:
- Line 5: Add the missing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER entry to the
envs-validator test base presets: update the test .env.base file used by the
envs-validator to include NEXT_PUBLIC_ROLLUP_LAYER_NUMBER with the same value
used in .env.optimism so the test presets and deployment envs stay in sync;
ensure the variable name is exact (NEXT_PUBLIC_ROLLUP_LAYER_NUMBER) and run the
envs-validator tests to confirm no missing-var errors.
In `@ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx`:
- Around line 142-145: Fix the typo in the tooltip hint for
DetailedInfo.ItemLabel: change the string passed to the hint prop from "Heigh of
${ layerLabels.parent } block which includes ${ layerLabels.parent }
transactions" to "Height of ${ layerLabels.parent } block which includes ${
layerLabels.parent } transactions" so the word "Height" is spelled correctly;
update the hint prop on the DetailedInfo.ItemLabel that renders "{
layerLabels.parent } block".
---
Nitpick comments:
In `@deploy/tools/envs-validator/test/.env.optimism`:
- Around line 9-10: Reorder the environment keys so they follow project
convention by placing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER before
NEXT_PUBLIC_ROLLUP_STAGE_INDEX, and ensure the fixture file ends with a single
trailing newline (EOF newline) to silence dotenv-linter ordering and newline
warnings; update the .env fixture containing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER and
NEXT_PUBLIC_ROLLUP_STAGE_INDEX accordingly.
In `@deploy/tools/llms-txt-generator/index.ts`:
- Around line 29-30: The two ternary expressions creating
currentToParentLayerLabel and parentToCurrentLayerLabel are redundant because
layerLabels already provides defaults; remove the rollupFeature.isEnabled ? ...
: undefined guards and assign the concatenated strings directly (use
layerLabels.current + '→' + layerLabels.parent and layerLabels.parent + '→' +
layerLabels.current) so the variables are always defined and simpler; keep any
external checks that guard template usage based on rollupFeature.isEnabled
intact.
In `@docs/ENVS.md`:
- Line 532: The table row for NEXT_PUBLIC_ROLLUP_L1_BASE_URL uses underscore
emphasis around "Use `NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead" which triggers
MD049; update that cell to use asterisk emphasis (i.e. replace _Use
`NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead_ with *Use
`NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead*) so the markdown emphasis style
matches the rest of the document; locate the row by the symbol
NEXT_PUBLIC_ROLLUP_L1_BASE_URL and make this single-token emphasis change.
In `@lib/rollups/utils.ts`:
- Around line 17-37: The three exported top-level formatters
(formatZkEvmTxStatus, formatZkEvmL2TxnBatchStatus, formatZkSyncL2TxnBatchStatus)
lack explicit return types; update each function signature to declare a return
type of string (e.g. change to formatZkEvmTxStatus(...): string) so the module
surface is explicit and type-safe, keeping function bodies unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
icons/navigation/ecosystems.svgis excluded by!**/*.svg
📒 Files selected for processing (66)
configs/app/features/rollup.tsdeploy/tools/envs-validator/schemas/features/rollup.tsdeploy/tools/envs-validator/test/.env.optimismdeploy/tools/llms-txt-generator/index.tsdocs/ENVS.mdlib/hooks/useNavItems.tsxlib/metadata/templates/title.tslib/rollups/utils.tsui/block/BlockDetails.tsxui/deposits/optimisticL2/OptimisticDepositsListItem.tsxui/deposits/optimisticL2/OptimisticDepositsTable.tsxui/deposits/scrollL2/ScrollL2DepositsListItem.tsxui/deposits/scrollL2/ScrollL2DepositsTable.tsxui/deposits/shibarium/DepositsListItem.tsxui/deposits/shibarium/DepositsTable.tsxui/deposits/zkEvmL2/ZkEvmL2DepositsListItem.tsxui/deposits/zkEvmL2/ZkEvmL2DepositsTable.tsxui/disputeGames/optimisticL2/OptimisticL2DisputeGamesListItem.tsxui/disputeGames/optimisticL2/OptimisticL2DisputeGamesTable.tsxui/home/Stats.tsxui/home/Transactions.tsxui/home/latestDeposits/LatestDeposits.tsxui/home/latestDeposits/LatestDepositsItem.tsxui/messages/ArbitrumL2Messages.tsxui/messages/ArbitrumL2MessagesListItem.tsxui/messages/ArbitrumL2MessagesTable.tsxui/outputRoots/optimisticL2/OptimisticL2OutputRootsListItem.tsxui/outputRoots/optimisticL2/OptimisticL2OutputRootsTable.tsxui/pages/ArbitrumL2TxnWithdrawals.tsxui/pages/OptimisticL2Deposits.tsxui/pages/OptimisticL2OutputRoots.tsxui/pages/OptimisticL2Withdrawals.tsxui/pages/ScrollL2Deposits.tsxui/pages/ScrollL2Withdrawals.tsxui/pages/ShibariumDeposits.tsxui/pages/ShibariumWithdrawals.tsxui/pages/ZkEvmL2Deposits.tsxui/pages/ZkEvmL2Withdrawals.tsxui/shared/statusTag/ZkEvmL2TxnBatchStatus.tsxui/shared/statusTag/ZkSyncL2TxnBatchStatus.tsxui/tx/details/TxDetailsInterop.tsxui/tx/details/TxDetailsWithdrawalStatusArbitrum.tsxui/tx/details/TxInfo.tsxui/tx/details/TxInfoScrollFees.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchesListItem.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchesTable.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobCallData.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobCelestia.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobEigenda.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobEip4844.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchDetails.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchesListItem.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchesTable.tsxui/txnBatches/scrollL2/ScrollL2TxnBatchDetails.tsxui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsxui/txnBatches/zkSyncL2/ZkSyncL2TxnBatchDetails.tsxui/txnBatches/zkSyncL2/ZkSyncL2TxnBatchHashesInfo.tsxui/withdrawals/optimisticL2/OptimisticL2WithdrawalsListItem.tsxui/withdrawals/optimisticL2/OptimisticL2WithdrawalsTable.tsxui/withdrawals/scrollL2/ScrollL2WithdrawalsListItem.tsxui/withdrawals/scrollL2/ScrollL2WithdrawalsTable.tsxui/withdrawals/shibarium/WithdrawalsListItem.tsxui/withdrawals/shibarium/WithdrawalsTable.tsxui/withdrawals/zkEvmL2/ZkEvmL2WithdrawalsListItem.tsxui/withdrawals/zkEvmL2/ZkEvmL2WithdrawalsTable.tsx
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx (1)
142-142:⚠️ Potential issue | 🟡 MinorFix typo: "Heigh" → "Height".
- hint={ `Heigh of ${ layerLabels.parent } block which includes ${ layerLabels.parent } transactions` } + hint={ `Height of ${ layerLabels.parent } block which includes ${ layerLabels.parent } transactions` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx` at line 142, Fix the typo in the hint string inside ArbitrumL2TxnBatchDetails.tsx: update the hint prop value from "Heigh of ${ layerLabels.parent } block which includes ${ layerLabels.parent } transactions" to use "Height" instead of "Heigh" where the hint is constructed (reference the hint prop and layerLabels.parent usage).deploy/tools/envs-validator/schemas/features/rollup.ts (1)
172-184:⚠️ Potential issue | 🔴 CriticalBiome
lint/suspicious/noThenPropertyerror blocks CI — use callback form.The
then:key on line 178 inside the.when()object triggers this Biome lint rule. Refactor to the callback form to fix it:🔧 Proposed fix
- NEXT_PUBLIC_ROLLUP_LAYER_NUMBER: yup.number() - .positive() - .integer() - .min(2) - .when('NEXT_PUBLIC_ROLLUP_TYPE', { - is: (value: string) => Boolean(value), - then: (schema) => schema, - otherwise: (schema) => schema.test( - 'not-exist', - 'NEXT_PUBLIC_ROLLUP_LAYER_NUMBER can only be used with NEXT_PUBLIC_ROLLUP_TYPE', - value => value === undefined, - ), - }), + NEXT_PUBLIC_ROLLUP_LAYER_NUMBER: yup.number() + .integer() + .min(2) + .when('NEXT_PUBLIC_ROLLUP_TYPE', (value: string | undefined, schema) => { + if (Boolean(value)) { + return schema; + } + return schema.test( + 'not-exist', + 'NEXT_PUBLIC_ROLLUP_LAYER_NUMBER can only be used with NEXT_PUBLIC_ROLLUP_TYPE', + (currentValue) => currentValue === undefined, + ); + }),Note:
.positive()is also dropped above since.min(2)already guarantees the value is ≥ 2 (and therefore positive), making it redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/tools/envs-validator/schemas/features/rollup.ts` around lines 172 - 184, Refactor the NEXT_PUBLIC_ROLLUP_LAYER_NUMBER yup schema to use the callback form of .when instead of the object form to satisfy the lint rule: replace .when('NEXT_PUBLIC_ROLLUP_TYPE', { is: ..., then: ..., otherwise: ... }) with .when('NEXT_PUBLIC_ROLLUP_TYPE', (rollupType, schema) => rollupType ? schema : schema.test('not-exist', 'NEXT_PUBLIC_ROLLUP_LAYER_NUMBER can only be used with NEXT_PUBLIC_ROLLUP_TYPE', value => value === undefined)); also remove the redundant .positive() since .min(2) already enforces positivity.lib/rollups/utils.ts (1)
35-36:⚠️ Potential issue | 🔴 CriticalFix replacement clobbering in zkSync status formatting.
The current two-step replace can rewrite the first replacement result (e.g.,
L1 -> L2 -> L3), producing incorrect parent-layer labels forL3/L2configs.💡 Proposed fix
export const formatZkSyncL2TxnBatchStatus = (status: typeof ZKSYNC_L2_TX_BATCH_STATUSES[number]) => { - return status.replace('L1', layerLabels.parent).replace('L2', layerLabels.current); + return status.replace(/\bL[12]\b/g, (layer) => (layer === 'L1' ? layerLabels.parent : layerLabels.current)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rollups/utils.ts` around lines 35 - 36, The function formatZkSyncL2TxnBatchStatus incorrectly performs two sequential string.replace calls which can clobber earlier replacements (e.g., replacing 'L1' then matching the added 'L2'); update formatZkSyncL2TxnBatchStatus to do a single-pass replacement by matching both tokens at once (e.g., use a regex like /(L1|L2)/g) and map 'L1' -> layerLabels.parent and 'L2' -> layerLabels.current in the replacement callback so replacements never overwrite each other; keep references to ZKSYNC_L2_TX_BATCH_STATUSES and layerLabels unchanged.
🧹 Nitpick comments (2)
ui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsx (1)
87-89: Memoizestepsbefore passing it toVerificationSteps.
ZKEVM_L2_TX_BATCH_STATUSES.map(...)creates a new array every render; this should be memoized per project rule.♻️ Suggested refactor
const ZkEvmL2TxnBatchDetails = ({ query }: Props) => { const router = useRouter(); const { data, isPlaceholderData, isError, error } = query; + const verificationSteps = React.useMemo( + () => ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus), + [], + ); @@ <VerificationSteps - steps={ ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus) } + steps={ verificationSteps } currentStep={ formatZkEvmL2TxnBatchStatus(data.status) } isLoading={ isPlaceholderData } />As per coding guidelines, "Wrap
.filter(),.map(),.reduce()results inuseMemowhen passed as props or used as hook deps."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsx` around lines 87 - 89, The prop `steps` is recreated every render because you call ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus) inline; wrap that mapping in a useMemo inside the ZkEvmL2TxnBatchDetails component to memoize the resulting array and pass the memoized variable to VerificationSteps. Use useMemo(() => ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus), [/* include dependencies such as formatZkEvmL2TxnBatchStatus if not stable */]) so the steps array only changes when its dependencies change, then pass that memoized variable as the steps prop while keeping currentStep as-is.ui/block/BlockDetails.tsx (1)
309-313: Memoize mapped zkSync steps before passing as prop.
ZKSYNC_L2_TX_BATCH_STATUSES.map(...)is recreated on every render and passed toVerificationSteps. Hoist it intoReact.useMemoto prevent unnecessary re-renders of the child component.♻️ Proposed refactor
const BlockDetails = ({ query }: Props) => { @@ const { data, isPlaceholderData } = query; + const zksyncVerificationSteps = React.useMemo( + () => ZKSYNC_L2_TX_BATCH_STATUSES.map(formatZkSyncL2TxnBatchStatus), + [], + ); @@ { rollupFeature.type === 'zkSync' && data.zksync && ( <VerificationSteps - steps={ ZKSYNC_L2_TX_BATCH_STATUSES.map(formatZkSyncL2TxnBatchStatus) } + steps={ zksyncVerificationSteps } currentStep={ formatZkSyncL2TxnBatchStatus(data.zksync.status) } isLoading={ isPlaceholderData } /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/block/BlockDetails.tsx` around lines 309 - 313, The mapped zkSync steps passed into VerificationSteps are recreated each render; hoist that mapping into a memoized value using React.useMemo and pass that memo (instead of ZKSYNC_L2_TX_BATCH_STATUSES.map(...)) to VerificationSteps. Create something like const zkSyncSteps = React.useMemo(() => ZKSYNC_L2_TX_BATCH_STATUSES.map(formatZkSyncL2TxnBatchStatus), [ZKSYNC_L2_TX_BATCH_STATUSES, formatZkSyncL2TxnBatchStatus]) and then use zkSyncSteps as the steps prop while leaving currentStep (formatZkSyncL2TxnBatchStatus(data.zksync.status)) and isLoading (isPlaceholderData) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/app/features/rollup.ts`:
- Line 54: The layerNumber assignment can coerce an empty env string to 0 via
Number(''), so change the logic around
getEnvValue('NEXT_PUBLIC_ROLLUP_LAYER_NUMBER')/layerNumber to treat empty
strings as "no value" and fall back to 2: read the raw value from getEnvValue,
trim and if it's empty/null/undefined treat it as undefined, otherwise parse
with parseInt(value, 10) and if isNaN or less than 2 use 2; update the
layerNumber expression accordingly so getEnvValue + Number coercion cannot
produce 0.
In `@deploy/tools/envs-validator/schemas/features/rollup.ts`:
- Around line 172-184: Add the new environment variable
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER to the test preset file (.env.base) used by the
envs-validator tests so the test suite recognizes the new variable (it already
exists in .env.optimism and the schema validation in rollup.ts references
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER); update the .env.base file by adding a suitable
test value for NEXT_PUBLIC_ROLLUP_LAYER_NUMBER to match the schema constraints
(positive integer >= 2) so tests pass.
In `@deploy/tools/envs-validator/test/.env.optimism`:
- Line 10: Add the missing environment variable NEXT_PUBLIC_ROLLUP_LAYER_NUMBER
with value 5 to the test preset file .env.base so the test preset aligns with
the test file (.env.optimism) and envs-validator expectations; ensure the entry
follows the same KEY=VALUE formatting used by other variables in .env.base and
commit the updated .env.base.
- Around line 9-10: Fix dotenv-linter violations by reordering the environment
keys alphabetically and adding a trailing newline: move
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER so it appears before
NEXT_PUBLIC_ROLLUP_OUTPUT_ROOTS_ENABLED (ensuring proper alphabetical order
relative to NEXT_PUBLIC_ROLLUP_STAGE_INDEX and other keys) and ensure the file
ends with a blank line (add a final newline character).
In `@docs/ENVS.md`:
- Line 532: The markdown deprecation note for NEXT_PUBLIC_ROLLUP_L1_BASE_URL
uses underscore emphasis which triggers MD049; change the emphasis from
underscores to asterisks so the description reads "*Use
`NEXT_PUBLIC_ROLLUP_PARENT_CHAIN` instead*" (update the table cell containing
NEXT_PUBLIC_ROLLUP_L1_BASE_URL) to satisfy markdownlint MD049.
In `@lib/rollups/utils.ts`:
- Around line 17-24: Add explicit string return types to the exported formatter
functions: update the signatures of formatZkEvmTxStatus,
formatZkEvmL2TxnBatchStatus, and formatZkSyncL2TxnBatchStatus to declare a
return type of string (e.g., add ": string" after the parameter list) so their
exported types are explicit and consistent.
In `@ui/home/Stats.tsx`:
- Line 7: Add the missing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER entry to the test
preset file used by the envs-validator
(deploy/tools/envs-validator/test/.env.base) so tests include this
project-required variable; open the test .env.base and add a line defining
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER (with an appropriate default numeric value or
placeholder) consistent with other entries, matching the usage in
configs/app/features/rollup.ts and the docs/ENVS.md.
In `@ui/shared/statusTag/ZkEvmL2TxnBatchStatus.tsx`:
- Around line 20-24: The rendered status text was changed to all-lowercase in
the 'L1 Sequence Confirmed' case branch; update the assignment of text in that
switch case (the branch for 'L1 Sequence Confirmed' that currently sets text =
`${ layerLabels.parent } sequence confirmed`) to use title case (e.g., `${
layerLabels.parent } Sequence Confirmed`) so the visual casing matches the
original status label; locate the switch handling this status in
ZkEvmL2TxnBatchStatus.tsx and change the text string accordingly.
In `@ui/txnBatches/zkSyncL2/ZkSyncL2TxnBatchDetails.tsx`:
- Around line 98-102: The chained string.replace calls in
formatZkSyncL2TxnBatchStatus cause cascading replacements (e.g., replacing "L2"
then "L3" can turn "Sent to L1" into "Sent to L3"); fix by computing parentLabel
= `L${layerNumber-1}` and currentLabel = `L${layerNumber}` and perform a single,
targeted replacement of whole-word occurrences (use a single replace with a
regex word-boundary or replace only exact tokens) so only the intended layer
token is substituted; update formatZkSyncL2TxnBatchStatus (and any place that
uses ZKSYNC_L2_TX_BATCH_STATUSES if necessary) to use this safe replacement
approach.
---
Duplicate comments:
In `@deploy/tools/envs-validator/schemas/features/rollup.ts`:
- Around line 172-184: Refactor the NEXT_PUBLIC_ROLLUP_LAYER_NUMBER yup schema
to use the callback form of .when instead of the object form to satisfy the lint
rule: replace .when('NEXT_PUBLIC_ROLLUP_TYPE', { is: ..., then: ..., otherwise:
... }) with .when('NEXT_PUBLIC_ROLLUP_TYPE', (rollupType, schema) => rollupType
? schema : schema.test('not-exist', 'NEXT_PUBLIC_ROLLUP_LAYER_NUMBER can only be
used with NEXT_PUBLIC_ROLLUP_TYPE', value => value === undefined)); also remove
the redundant .positive() since .min(2) already enforces positivity.
In `@lib/rollups/utils.ts`:
- Around line 35-36: The function formatZkSyncL2TxnBatchStatus incorrectly
performs two sequential string.replace calls which can clobber earlier
replacements (e.g., replacing 'L1' then matching the added 'L2'); update
formatZkSyncL2TxnBatchStatus to do a single-pass replacement by matching both
tokens at once (e.g., use a regex like /(L1|L2)/g) and map 'L1' ->
layerLabels.parent and 'L2' -> layerLabels.current in the replacement callback
so replacements never overwrite each other; keep references to
ZKSYNC_L2_TX_BATCH_STATUSES and layerLabels unchanged.
In `@ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx`:
- Line 142: Fix the typo in the hint string inside
ArbitrumL2TxnBatchDetails.tsx: update the hint prop value from "Heigh of ${
layerLabels.parent } block which includes ${ layerLabels.parent } transactions"
to use "Height" instead of "Heigh" where the hint is constructed (reference the
hint prop and layerLabels.parent usage).
---
Nitpick comments:
In `@ui/block/BlockDetails.tsx`:
- Around line 309-313: The mapped zkSync steps passed into VerificationSteps are
recreated each render; hoist that mapping into a memoized value using
React.useMemo and pass that memo (instead of
ZKSYNC_L2_TX_BATCH_STATUSES.map(...)) to VerificationSteps. Create something
like const zkSyncSteps = React.useMemo(() =>
ZKSYNC_L2_TX_BATCH_STATUSES.map(formatZkSyncL2TxnBatchStatus),
[ZKSYNC_L2_TX_BATCH_STATUSES, formatZkSyncL2TxnBatchStatus]) and then use
zkSyncSteps as the steps prop while leaving currentStep
(formatZkSyncL2TxnBatchStatus(data.zksync.status)) and isLoading
(isPlaceholderData) as-is.
In `@ui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsx`:
- Around line 87-89: The prop `steps` is recreated every render because you call
ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus) inline; wrap that
mapping in a useMemo inside the ZkEvmL2TxnBatchDetails component to memoize the
resulting array and pass the memoized variable to VerificationSteps. Use
useMemo(() => ZKEVM_L2_TX_BATCH_STATUSES.map(formatZkEvmL2TxnBatchStatus), [/*
include dependencies such as formatZkEvmL2TxnBatchStatus if not stable */]) so
the steps array only changes when its dependencies change, then pass that
memoized variable as the steps prop while keeping currentStep as-is.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
icons/navigation/ecosystems.svgis excluded by!**/*.svg
📒 Files selected for processing (66)
configs/app/features/rollup.tsdeploy/tools/envs-validator/schemas/features/rollup.tsdeploy/tools/envs-validator/test/.env.optimismdeploy/tools/llms-txt-generator/index.tsdocs/ENVS.mdlib/hooks/useNavItems.tsxlib/metadata/templates/title.tslib/rollups/utils.tsui/block/BlockDetails.tsxui/deposits/optimisticL2/OptimisticDepositsListItem.tsxui/deposits/optimisticL2/OptimisticDepositsTable.tsxui/deposits/scrollL2/ScrollL2DepositsListItem.tsxui/deposits/scrollL2/ScrollL2DepositsTable.tsxui/deposits/shibarium/DepositsListItem.tsxui/deposits/shibarium/DepositsTable.tsxui/deposits/zkEvmL2/ZkEvmL2DepositsListItem.tsxui/deposits/zkEvmL2/ZkEvmL2DepositsTable.tsxui/disputeGames/optimisticL2/OptimisticL2DisputeGamesListItem.tsxui/disputeGames/optimisticL2/OptimisticL2DisputeGamesTable.tsxui/home/Stats.tsxui/home/Transactions.tsxui/home/latestDeposits/LatestDeposits.tsxui/home/latestDeposits/LatestDepositsItem.tsxui/messages/ArbitrumL2Messages.tsxui/messages/ArbitrumL2MessagesListItem.tsxui/messages/ArbitrumL2MessagesTable.tsxui/outputRoots/optimisticL2/OptimisticL2OutputRootsListItem.tsxui/outputRoots/optimisticL2/OptimisticL2OutputRootsTable.tsxui/pages/ArbitrumL2TxnWithdrawals.tsxui/pages/OptimisticL2Deposits.tsxui/pages/OptimisticL2OutputRoots.tsxui/pages/OptimisticL2Withdrawals.tsxui/pages/ScrollL2Deposits.tsxui/pages/ScrollL2Withdrawals.tsxui/pages/ShibariumDeposits.tsxui/pages/ShibariumWithdrawals.tsxui/pages/ZkEvmL2Deposits.tsxui/pages/ZkEvmL2Withdrawals.tsxui/shared/statusTag/ZkEvmL2TxnBatchStatus.tsxui/shared/statusTag/ZkSyncL2TxnBatchStatus.tsxui/tx/details/TxDetailsInterop.tsxui/tx/details/TxDetailsWithdrawalStatusArbitrum.tsxui/tx/details/TxInfo.tsxui/tx/details/TxInfoScrollFees.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchesListItem.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchesTable.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobCallData.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobCelestia.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobEigenda.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchBlobEip4844.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchDetails.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchesListItem.tsxui/txnBatches/optimisticL2/OptimisticL2TxnBatchesTable.tsxui/txnBatches/scrollL2/ScrollL2TxnBatchDetails.tsxui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsxui/txnBatches/zkSyncL2/ZkSyncL2TxnBatchDetails.tsxui/txnBatches/zkSyncL2/ZkSyncL2TxnBatchHashesInfo.tsxui/withdrawals/optimisticL2/OptimisticL2WithdrawalsListItem.tsxui/withdrawals/optimisticL2/OptimisticL2WithdrawalsTable.tsxui/withdrawals/scrollL2/ScrollL2WithdrawalsListItem.tsxui/withdrawals/scrollL2/ScrollL2WithdrawalsTable.tsxui/withdrawals/shibarium/WithdrawalsListItem.tsxui/withdrawals/shibarium/WithdrawalsTable.tsxui/withdrawals/zkEvmL2/ZkEvmL2WithdrawalsListItem.tsxui/withdrawals/zkEvmL2/ZkEvmL2WithdrawalsTable.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/rollups/utils.ts (3)
35-36: Chained.replaceorder still risks incorrect output for non-default layer configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rollups/utils.ts` around lines 35 - 36, The current formatZkSyncL2TxnBatchStatus uses chained .replace calls which can mis-substitute when layerLabels are non-default; update the function to perform safe tokenized replacement (e.g., replace using a single pass mapping or temporary placeholders) so both 'L2' and 'L1' are replaced deterministically; locate formatZkSyncL2TxnBatchStatus and change the replace logic to use a single regex or map (matching 'L2'|'L1') and substitute with layerLabels.current and layerLabels.parent respectively to avoid overlapping/reorder bugs.
10-11:NEXT_PUBLIC_ROLLUP_LAYER_NUMBERstill missing fromdeploy/tools/envs-validator/test/.env.base.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rollups/utils.ts` around lines 10 - 11, Add the missing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER entry to the test environment base so the rollup layer code can resolve `feature.layerNumber`/`L${ feature.layerNumber }` and its parent value; update the deploy/tools/envs-validator/test/.env.base to include a sensible integer (e.g., the current rollup layer) and ensure the envs-validator test fixtures and any schema that lists required NEXT_PUBLIC_* variables recognize NEXT_PUBLIC_ROLLUP_LAYER_NUMBER.
17-33: Add explicit: stringreturn types to all three formatter functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rollups/utils.ts` around lines 17 - 33, Add explicit ": string" return annotations to the formatter functions in this file: update the function signatures for formatZkEvmTxStatus and formatZkEvmL2TxnBatchStatus (and any other formatter function in the same file) to declare a string return type (e.g., change "export const formatZkEvmTxStatus = (status: ...)" to "export const formatZkEvmTxStatus = (status: ...): string" and similarly for formatZkEvmL2TxnBatchStatus), ensuring the switch/default branches still return string values and no runtime behavior is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx`:
- Around line 127-128: Fix the grammar in the hint string passed to the hint
prop in ArbitrumL2TxnBatchDetails: change the text from "Hash of ${
layerLabels.parent } transaction in which transactions was committed" to "Hash
of ${ layerLabels.parent } transaction in which transaction was committed"
(update the hint prop where layerLabels.parent is used to ensure correct
singular subject–verb agreement).
---
Duplicate comments:
In `@lib/rollups/utils.ts`:
- Around line 35-36: The current formatZkSyncL2TxnBatchStatus uses chained
.replace calls which can mis-substitute when layerLabels are non-default; update
the function to perform safe tokenized replacement (e.g., replace using a single
pass mapping or temporary placeholders) so both 'L2' and 'L1' are replaced
deterministically; locate formatZkSyncL2TxnBatchStatus and change the replace
logic to use a single regex or map (matching 'L2'|'L1') and substitute with
layerLabels.current and layerLabels.parent respectively to avoid
overlapping/reorder bugs.
- Around line 10-11: Add the missing NEXT_PUBLIC_ROLLUP_LAYER_NUMBER entry to
the test environment base so the rollup layer code can resolve
`feature.layerNumber`/`L${ feature.layerNumber }` and its parent value; update
the deploy/tools/envs-validator/test/.env.base to include a sensible integer
(e.g., the current rollup layer) and ensure the envs-validator test fixtures and
any schema that lists required NEXT_PUBLIC_* variables recognize
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER.
- Around line 17-33: Add explicit ": string" return annotations to the formatter
functions in this file: update the function signatures for formatZkEvmTxStatus
and formatZkEvmL2TxnBatchStatus (and any other formatter function in the same
file) to declare a string return type (e.g., change "export const
formatZkEvmTxStatus = (status: ...)" to "export const formatZkEvmTxStatus =
(status: ...): string" and similarly for formatZkEvmL2TxnBatchStatus), ensuring
the switch/default branches still return string values and no runtime behavior
is changed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
configs/app/features/rollup.tsdeploy/tools/llms-txt-generator/index.tslib/rollups/utils.tsui/block/BlockDetails.tsxui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsxui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- configs/app/features/rollup.ts
- ui/txnBatches/zkEvmL2/ZkEvmL2TxnBatchDetails.tsx
- deploy/tools/llms-txt-generator/index.ts
Comment on lines
+127
to
128
| hint={ `Hash of ${ layerLabels.parent } transaction in which transactions was committed` } | ||
| > |
Contributor
There was a problem hiding this comment.
Grammar error in hint text: "transactions was" → "transaction was".
"Hash of ${ layerLabels.parent } transaction in which transactions was committed" has a subject–verb agreement error ("transactions was"). It should read "transaction was committed" to match the singular subject "transaction".
✏️ Proposed fix
- hint={ `Hash of ${ layerLabels.parent } transaction in which transactions was committed` }
+ hint={ `Hash of ${ layerLabels.parent } transaction in which the transaction was committed` }📝 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.
Suggested change
| hint={ `Hash of ${ layerLabels.parent } transaction in which transactions was committed` } | |
| > | |
| hint={ `Hash of ${ layerLabels.parent } transaction in which the transaction was committed` } | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/txnBatches/arbitrumL2/ArbitrumL2TxnBatchDetails.tsx` around lines 127 -
128, Fix the grammar in the hint string passed to the hint prop in
ArbitrumL2TxnBatchDetails: change the text from "Hash of ${ layerLabels.parent }
transaction in which transactions was committed" to "Hash of ${
layerLabels.parent } transaction in which transaction was committed" (update the
hint prop where layerLabels.parent is used to ensure correct singular
subject–verb agreement).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description and Related Issue(s)
Resolves #2554
Makes rollup layer numbering configurable so instances can use L2/L3 (or other pairs) instead of the default L2/L1, as requested in Canny.
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER(default2) and centralizes layer labels inlib/rollups/utils.ts(layerLabels.current/layerLabels.parent).Proposed Changes
NEXT_PUBLIC_ROLLUP_LAYER_NUMBERand wired it in rollup config and ENV validator.lib/rollups/utils.tswithlayerLabelsand status-formatting helpers; all rollup views now use these labels instead of literal "L1"/"L2".docs/ENVS.md(new variable and minor wording: "L1" → "parent", "L2" → "rollup" where generic).Environment variables
NEXT_PUBLIC_ROLLUP_LAYER_NUMBER(optional, number, default2) – Layer number of the rollup. Used to show "L2"/"L1" (or e.g. "L3"/"L2") in the UI. Set to3for L3 chains.Breaking or Incompatible Changes
None. Default remains
2(L2/L1).Additional Information
.env.optimismexample updated for the new variable.Checklist for PR author
@coderabbitai review