feat(acl): add acl scan to asset sync#954
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ACL drift detection, sync entry points, and token address label updates across state, pages, tables, modals, and devtools. A new utility computes drift, state marks affected ANTs, and the UI exposes sync actions and indicators. ChangesACL Drift Detection and Ownership Sync
Sequence Diagram(s)sequenceDiagram
participant ManagePage
participant dispatchArNSUpdate
participant computeAclDrift
participant SyncOwnershipModal
ManagePage->>dispatchArNSUpdate: refresh() with wallet + arioContract
dispatchArNSUpdate->>computeAclDrift: owner, ario
computeAclDrift-->>dispatchArNSUpdate: driftRecords, ownedMints
dispatchArNSUpdate->>ManagePage: ants with needsOwnerSync
ManagePage->>SyncOwnershipModal: open with outOfSyncItems
SyncOwnershipModal->>SyncOwnershipModal: syncAcl per mint
SyncOwnershipModal-->>ManagePage: onSynced()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Permaweb Preview DeployedYour preview is available at: https://arns-react-pr-954_arns.ar.io
|
![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 (1)
package.json (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the alpha SDK exactly rather than with a caret.
^4.1.0-alpha.1allows resolution to drift to other prerelease/minor builds of the4.xline. Since this PR depends on freshly-added APIs in an alpha release, an exact pin (4.1.0-alpha.1) reduces the risk of a future install picking up a different, possibly breaking, alpha.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 32, The "`@ar.io/sdk`" dependency is using a caret version pin (^4.1.0-alpha.1) which allows npm to resolve to other prerelease or minor versions of the 4.x line. Since this PR depends on APIs from this specific alpha release, remove the caret character from the version string to pin it exactly to "4.1.0-alpha.1", ensuring consistent installs and preventing unexpected breaking changes from other alpha versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/data-display/tables/DomainsTable.tsx`:
- Around line 250-256: The Link component at line 255 uses target="_blank" to
open an external URL in a new tab without including the rel="noopener
noreferrer" attribute, creating a tabnabbing security vulnerability. Add the
rel="noopener noreferrer" attribute to the Link component to prevent the newly
opened page from accessing the window.opener property and potentially
redirecting the original page. This hardening protects against tabnabbing
attacks when opening external links in new tabs.
In
`@src/components/modals/ant-management/SyncOwnershipModal/SyncOwnershipModal.tsx`:
- Around line 60-63: The catch block in the error handler is emitting the raw
error `e` to the eventEmitter without normalizing it to the project's expected
error contract. Before emitting the error via eventEmitter.emit('error', ...),
check whether the caught error is a NotificationOnlyError (expected/user-facing)
or a standard Error (unexpected), and normalize it accordingly so that expected
failures are properly classified and handled per the project's coding guidelines
for error types.
- Line 91: The style prop in SyncOwnershipModal contains an invalid CSS value
where `padding: 'none'` is being applied for mobile devices. The string 'none'
is not a valid CSS padding value and will be ignored by the browser. Replace
`'none'` with a valid CSS padding value such as `0` or `'0'` to properly remove
padding on mobile devices while maintaining the correct styling behavior.
- Around line 67-74: The `syncAll` function lacks a re-entry guard, allowing
multiple concurrent executions if the user clicks quickly before the UI disables
based on the `running` state. Add a guard check at the beginning of the
`syncAll` function that immediately returns if `running` is already true,
preventing duplicate non-idempotent `syncAcl` calls. This ensures only one sync
loop can execute at a time by blocking re-entry until the current operation
completes.
In `@src/state/actions/dispatchArNSUpdate.ts`:
- Around line 205-208: The issue is that `ownedMints` stays `null` only when
`computeAclDrift` throws, but when `scanOwnedAntAssetMints` silently swallows
parse errors via catch blocks, `computeAclDrift` returns a non-null empty Set
instead of null. This makes the drop condition unable to distinguish between a
successful scan finding zero assets versus a degraded scan that silently skipped
all assets, causing unintended mass-dropping of ANTs. Modify the logic to
distinguish scan success from scan degradation by checking whether
`getProgramAccounts` actually returned zero raw accounts versus whether all
assets were skipped/unparseable. When scan degradation is detected (all assets
skipped but getProgramAccounts returned accounts), fall back to the legacy owner
check condition instead of applying the `ownedMints !== null` branch that relies
on the unreliable empty Set result.
---
Nitpick comments:
In `@package.json`:
- Line 32: The "`@ar.io/sdk`" dependency is using a caret version pin
(^4.1.0-alpha.1) which allows npm to resolve to other prerelease or minor
versions of the 4.x line. Since this PR depends on APIs from this specific alpha
release, remove the caret character from the version string to pin it exactly to
"4.1.0-alpha.1", ensuring consistent installs and preventing unexpected breaking
changes from other alpha versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c80d5e28-d0eb-4ce7-83b2-baf59dc25aa9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.jsonsrc/components/data-display/tables/DomainsTable.tsxsrc/components/modals/ant-management/SyncOwnershipModal/SyncOwnershipModal.tsxsrc/components/pages/Manage/Manage.tsxsrc/components/pages/Settings/devtools/AclSync.tsxsrc/components/pages/Settings/devtools/DevTools.tsxsrc/state/actions/dispatchArNSUpdate.tssrc/state/contexts/ArNSState.tsxsrc/utils/aclSync.ts
Comment on lines
+250
to
+256
| <Link | ||
| className="link gap-2 w-fit whitespace-nowrap items-center" | ||
| to={`https://${encodeDomainToASCII( | ||
| row.getValue('name'), | ||
| )}.${NETWORK_DEFAULTS.ARNS.HOST}`} | ||
| target="_blank" | ||
| > |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden external link opened in a new tab.
At Line 255, target="_blank" is used without rel="noopener noreferrer", which leaves a tabnabbing window.
🛡️ Proposed fix
<Link
className="link gap-2 w-fit whitespace-nowrap items-center"
to={`https://${encodeDomainToASCII(
row.getValue('name'),
)}.${NETWORK_DEFAULTS.ARNS.HOST}`}
target="_blank"
+ rel="noopener noreferrer"
>📝 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
| <Link | |
| className="link gap-2 w-fit whitespace-nowrap items-center" | |
| to={`https://${encodeDomainToASCII( | |
| row.getValue('name'), | |
| )}.${NETWORK_DEFAULTS.ARNS.HOST}`} | |
| target="_blank" | |
| > | |
| <Link | |
| className="link gap-2 w-fit whitespace-nowrap items-center" | |
| to={`https://${encodeDomainToASCII( | |
| row.getValue('name'), | |
| )}.${NETWORK_DEFAULTS.ARNS.HOST}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/data-display/tables/DomainsTable.tsx` around lines 250 - 256,
The Link component at line 255 uses target="_blank" to open an external URL in a
new tab without including the rel="noopener noreferrer" attribute, creating a
tabnabbing security vulnerability. Add the rel="noopener noreferrer" attribute
to the Link component to prevent the newly opened page from accessing the
window.opener property and potentially redirecting the original page. This
hardening protects against tabnabbing attacks when opening external links in new
tabs.
Comment on lines
+60
to
+63
| } catch (e) { | ||
| setStatus((s) => ({ ...s, [mint]: 'error' })); | ||
| eventEmitter.emit('error', e); | ||
| return false; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize emitted errors to the project’s expected/unknown error contract.
At Line 62, emitting raw e bypasses the expected NotificationOnlyError vs Error split, so expected wallet/user-facing failures can be misclassified and over-reported.
As per coding guidelines, **/*.{ts,tsx}: “Use NotificationOnlyError for expected/user-facing errors ... Use standard Error for unexpected errors ... Use eventEmitter ... for error handling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/modals/ant-management/SyncOwnershipModal/SyncOwnershipModal.tsx`
around lines 60 - 63, The catch block in the error handler is emitting the raw
error `e` to the eventEmitter without normalizing it to the project's expected
error contract. Before emitting the error via eventEmitter.emit('error', ...),
check whether the caught error is a NotificationOnlyError (expected/user-facing)
or a standard Error (unexpected), and normalize it accordingly so that expected
failures are properly classified and handled per the project's coding guidelines
for error types.
Source: Coding guidelines
Comment on lines
+67
to
+74
| async function syncAll() { | ||
| setRunning(true); | ||
| try { | ||
| // Sequential — one signed tx per ANT; parallel would prompt the wallet | ||
| // N times at once. | ||
| for (const item of remaining) { | ||
| await syncOne(item.mint); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard syncAll against re-entry before running state is committed.
At Line 67, a fast double-click can start multiple loops before the disabled state is rendered, causing duplicate non-idempotent syncAcl calls.
🔒 Proposed fix
-import { useState } from 'react';
+import { useRef, useState } from 'react';
@@
const [running, setRunning] = useState(false);
+ const runningRef = useRef(false);
@@
async function syncAll() {
+ if (runningRef.current) return;
+ runningRef.current = true;
setRunning(true);
try {
// Sequential — one signed tx per ANT; parallel would prompt the wallet
// N times at once.
for (const item of remaining) {
await syncOne(item.mint);
}
} finally {
+ runningRef.current = false;
setRunning(false);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/modals/ant-management/SyncOwnershipModal/SyncOwnershipModal.tsx`
around lines 67 - 74, The `syncAll` function lacks a re-entry guard, allowing
multiple concurrent executions if the user clicks quickly before the UI disables
based on the `running` state. Add a guard check at the beginning of the
`syncAll` function that immediately returns if `running` is already true,
preventing duplicate non-idempotent `syncAcl` calls. This ensures only one sync
loop can execute at a time by blocking re-entry until the current operation
completes.
| return ( | ||
| <div | ||
| className="modal-container" | ||
| style={isMobile ? { padding: 'none' } : {}} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a valid CSS value for mobile padding.
At Line 91, padding: 'none' is invalid and ignored. Use 0 (or '0') to reliably remove padding.
🎯 Proposed fix
- style={isMobile ? { padding: 'none' } : {}}
+ style={isMobile ? { padding: 0 } : {}}📝 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
| style={isMobile ? { padding: 'none' } : {}} | |
| style={isMobile ? { padding: 0 } : {}} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/modals/ant-management/SyncOwnershipModal/SyncOwnershipModal.tsx`
at line 91, The style prop in SyncOwnershipModal contains an invalid CSS value
where `padding: 'none'` is being applied for mobile devices. The string 'none'
is not a valid CSS padding value and will be ignored by the browser. Replace
`'none'` with a valid CSS padding value such as `0` or `'0'` to properly remove
padding on mobile devices while maintaining the correct styling behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/Navbar/NotificationMenu/NotificationMenu.tsx`:
- Around line 99-104: Fix the singular/plural agreement in the notification
message built in NotificationMenu so the verb matches the count. Update the
message JSX in the NotificationMenu component to make the verb conditional
alongside the noun, using the same antsNeedingSync check that already controls
the “name(s)” text, so the singular case reads naturally and the plural case
remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c84901e-07bb-4525-bb53-4ca50a17a482
📒 Files selected for processing (15)
src/components/Tooltips/ANTDetailsTip.tsxsrc/components/Tooltips/DomainDetailsTip.tsxsrc/components/cards/ANTCard/ANTCard.tsxsrc/components/data-display/tables/DomainsTable.tsxsrc/components/forms/DomainSettings/DomainSettings.tsxsrc/components/inputs/text/NameTokenSelector/NameTokenSelector.tsxsrc/components/layout/Navbar/NotificationMenu/NotificationMenu.tsxsrc/components/modals/ant-management/AddControllerModal/AddControllerModal.tsxsrc/components/modals/ant-management/ReassignNameModal/ReassignNameModal.tsxsrc/components/modals/ant-management/RemoveControllerModal/RemoveControllerModal.tsxsrc/components/modals/ant-management/TransferANTModal/TransferANTModal.tsxsrc/components/pages/Manage/Manage.tsxsrc/components/pages/ManageDomain/ManageDomain.tsxsrc/utils/aclSync.tssrc/utils/network.ts
✅ Files skipped from review due to trivial changes (7)
- src/components/Tooltips/ANTDetailsTip.tsx
- src/components/inputs/text/NameTokenSelector/NameTokenSelector.tsx
- src/components/Tooltips/DomainDetailsTip.tsx
- src/components/modals/ant-management/RemoveControllerModal/RemoveControllerModal.tsx
- src/components/modals/ant-management/AddControllerModal/AddControllerModal.tsx
- src/utils/network.ts
- src/components/modals/ant-management/TransferANTModal/TransferANTModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/pages/Manage/Manage.tsx
- src/components/data-display/tables/DomainsTable.tsx
Comment on lines
+99
to
+104
| message: ( | ||
| <span className="w-full"> | ||
| <span className="text-bold">{antsNeedingSync}</span> name | ||
| {antsNeedingSync === 1 ? '' : 's'} you own need ownership synced. | ||
| </span> | ||
| ), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix subject-verb agreement in the singular message.
When antsNeedingSync === 1, the message reads "1 name you own need ownership synced", which is grammatically incorrect. The verb should be conditional like the noun.
✏️ Proposed fix
- <span className="text-bold">{antsNeedingSync}</span> name
- {antsNeedingSync === 1 ? '' : 's'} you own need ownership synced.
+ <span className="text-bold">{antsNeedingSync}</span> name
+ {antsNeedingSync === 1 ? ' you own needs' : 's you own need'} ownership
+ synced.📝 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
| message: ( | |
| <span className="w-full"> | |
| <span className="text-bold">{antsNeedingSync}</span> name | |
| {antsNeedingSync === 1 ? '' : 's'} you own need ownership synced. | |
| </span> | |
| ), | |
| message: ( | |
| <span className="w-full"> | |
| <span className="text-bold">{antsNeedingSync}</span> name | |
| {antsNeedingSync === 1 ? ' you own needs' : 's you own need'} ownership | |
| synced. | |
| </span> | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/Navbar/NotificationMenu/NotificationMenu.tsx` around
lines 99 - 104, Fix the singular/plural agreement in the notification message
built in NotificationMenu so the verb matches the count. Update the message JSX
in the NotificationMenu component to make the verb conditional alongside the
noun, using the same antsNeedingSync check that already controls the “name(s)”
text, so the singular case reads naturally and the plural case remains
unchanged.
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.
Summary by CodeRabbit
@ar.io/sdkdependency.