Remix encoding fix #665
Remix encoding fix #665
Conversation
Co-authored-by: Eric Lau <ericglau@outlook.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a UI test job to CI, introduces root and UI test scripts, configures AVA with ts-node, and updates Remix URL generation to cross-environment base64 encode source without invalid characters. Adds unit tests validating code encoding/decoding and deployProxy flag handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI (Wizard)
participant FN as remixURL()
participant ENC as Base64 Encoder
participant URL as URL
User->>UI: Generate Remix link
UI->>FN: remixURL(source, deployProxy?)
note over FN: New cross-environment encoding path
FN->>ENC: Encode UTF-8 source to base64 (no padding)
ENC-->>FN: encodedCode
FN->>URL: Create URL and set search params (code, deployProxy?)
URL-->>UI: Remix URL
UI-->>User: Open in Remix link
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
package.json (1)
32-32: tsx only at root couples UI tests to hoistingUI package executes
node --import tsxbut doesn’t declaretsxitself. Addtsxtopackages/uidevDependencies to avoid breakage under non-hoisted installs or PnP.Apply in packages/ui/package.json:
"devDependencies": { + "tsx": "^4.19.2", "@rollup/plugin-alias": "^5.0.0",packages/ui/package.json (1)
12-13: Expose a top-level test:ui in this workspace (optional) and declare tsx locallyScripts look good. Consider also adding
tsxto this package’s devDependencies (see root comment) to decouple from hoisting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks)package.json(3 hunks)packages/ui/package.json(2 hunks)packages/ui/src/solidity/remix.node.test.ts(1 hunks)packages/ui/src/solidity/remix.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/solidity/remix.node.test.ts (1)
packages/ui/src/solidity/remix.ts (1)
remixURL(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: check
- GitHub Check: build (stylus, default)
- GitHub Check: build (cairo, default)
- GitHub Check: build (stellar, compile)
- GitHub Check: build (stellar, default)
- GitHub Check: build (solidity, default)
- GitHub Check: mcp
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
package.json (1)
10-10: LGTM: added test:ui scriptThis nicely scopes UI tests and aligns with the new CI job.
.github/workflows/test.yml (1)
43-52: Make Node version explicit for the UI job
node --import tsx --testdepends on recent Node. Either ensure your setup action pins Node ≥18 (preferably 20+) or add an explicitactions/setup-nodestep here for determinism.If setup already pins Node ≥20, ignore.
packages/ui/src/solidity/remix.node.test.ts (3)
12-21: LGTM: round‑trip sanity checkCovers the basic path and asserts presence of the code param.
23-31: LGTM: deployProxy flag behaviorCorrect: only set when upgradeable is true.
33-96: LGTM: complex/Unicode contract caseGood coverage for SPDX, imports, inheritance, and Unicode literals (e.g., emoji).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. It is recommended to resolve "Warn" alerts too. Learn more about Socket for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
CoveMB
force-pushed
the
remix-encoding-fix
branch
from
28afbb6 to
d4112dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/ts-node-register.cjs (1)
6-9: Optional: speed up tests with swc transpilerYou can shave test time by enabling ts-node’s swc transpiler (keep transpileOnly). Requires adding @swc/core as a devDependency.
Apply this diff (and add @swc/core devDependency):
require('ts-node').register({ transpileOnly: true, project: path.join(__dirname, 'tsconfig.test.json'), + transpiler: 'ts-node/transpilers/swc-experimental', });packages/ui/src/solidity/remix.test.ts (1)
5-9: Make decoder tolerant of trimmed paddingSince URLs trim “=”, some atob implementations require re-padding. Re-pad defensively to avoid environment-specific flakiness while still mirroring Remix’s flow.
Apply this diff:
-const decodeBase64 = (b64Payload: string) => { - const raw = atob(decodeURIComponent(b64Payload)); +const decodeBase64 = (b64Payload: string) => { + const decoded = decodeURIComponent(b64Payload); + const pad = (4 - (decoded.length % 4)) % 4; + const padded = pad ? decoded + '='.repeat(pad) : decoded; + const raw = atob(padded); const bytes = Uint8Array.from(raw, c => c.charCodeAt(0)); return new TextDecoder().decode(bytes); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.json(2 hunks)packages/ui/ava.config.js(1 hunks)packages/ui/package.json(3 hunks)packages/ui/src/solidity/remix.test.ts(1 hunks)packages/ui/src/solidity/remix.ts(1 hunks)packages/ui/ts-node-register.cjs(1 hunks)packages/ui/tsconfig.test.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/ui/tsconfig.test.json
- packages/ui/ava.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/package.json
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T19:26:06.112Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#652
File: packages/core/confidential/print-versioned.js:1-1
Timestamp: 2025-09-18T19:26:06.112Z
Learning: CJS shims like `module.exports = require('./dist/print-versioned')` in packages/core/confidential are expected to be CJS-only and are used specifically within packages/ui, not as general-purpose exports requiring conditional exports mapping.
Applied to files:
packages/ui/ts-node-register.cjs
🪛 GitHub Check: format-lint
packages/ui/src/solidity/remix.ts
[failure] 10-10:
Unexpected any. Specify a different type
[failure] 7-7:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (solidity, default)
🔇 Additional comments (2)
packages/ui/src/solidity/remix.ts (2)
16-16: LGTM: using URLSearchParams for code paramThis ensures proper percent-encoding of reserved characters.
4-14: Fix unbounded spread and removeanycasts; resolve lint failures and avoid stack/heap issues
- String.fromCharCode(...bytes) on the full Uint8Array can overflow the call stack for large sources.
- ESLint “Unexpected any” on
(globalThis as any)is failing format-lint.- Use chunked conversion and typed access to global features. This keeps your cross-runtime intent, preserves “trim padding,” and makes lint green.
Apply this diff:
- // Encode to base64 in a way that works in both browser and Node. - const encodedCode = ((): string => { - // Prefer browser btoa when available - if (typeof (globalThis as any).btoa === 'function') { - const bytes = new TextEncoder().encode(code); - const binary = String.fromCharCode(...bytes); - return (globalThis as any).btoa(binary).replace(/=*$/, ''); - } - // Fallback to Node Buffer - return Buffer.from(code, 'utf8').toString('base64').replace(/=*$/, ''); - })(); + // Cross‑runtime Base64 over UTF‑8 bytes. Trims "=" padding to keep URLs compact. + const encodedCode = ((): string => { + const g = globalThis as unknown as { + btoa?: (data: string) => string; + Buffer?: { from(input: string, encoding: string): { toString(encoding: 'base64'): string } }; + }; + // Prefer Node/Buffer when available (works in Node and many bundler environments) + if (g.Buffer && typeof g.Buffer.from === 'function') { + return g.Buffer.from(code, 'utf8').toString('base64').replace(/=*$/, ''); + } + // Browser path: build the binary string in chunks to avoid call‑stack overflow + if (g.btoa && typeof TextEncoder !== 'undefined') { + const bytes = new TextEncoder().encode(code); + let binary = ''; + const CHUNK = 0x8000; // 32k chunks + for (let i = 0; i < bytes.length; i += CHUNK) { + binary += String.fromCharCode(...bytes.subarray(i, i + CHUNK)); + } + return g.btoa(binary).replace(/=*$/, ''); + } + throw new Error('No Base64 encoder available in this environment'); + })();
packages/ui/src/solidity/remix.ts
Outdated
| @@ -1,11 +1,9 @@ | |||
| export function remixURL(code: string, upgradeable = false, overrideRemixURL?: string): URL { | |||
| const remix = new URL(overrideRemixURL ?? 'https://remix.ethereum.org'); | |||
| export function remixURL(code: string, upgradeable = false): URL { | |||
There was a problem hiding this comment.
This needs to keep the overrideRemixURL?: string parameter and use it on line 2, since it is required for Polkadot Wizard to use the Polkadot fork of Remix.
Fixes #662
Summary
remixURLbehavior and wires UI tests into CI.Motivation
atob (decodeURIComponent(...)). This ensures consistent, binary-safe Base64 and trims padding to keep the URL compact.Key Changes
encodedCode; keep padding trimmed.codeparam decodes back to original source.deployProxyflag behavior.testscript using Node’s test runner withtsx.test:uiscript andtsxdev dependency.uijob to run UI tests.Behavioral Impact
codeparameter; behavior is otherwise unchanged.How To Test
Before Remix side MR is merged
After Remix side MR is merged
** Note**