Skip to content

Allow enable-chunking flag to override chunk size limit#78

Merged
kriskowal merged 6 commits into
mainfrom
enable-chunking-parameter
May 1, 2026
Merged

Allow enable-chunking flag to override chunk size limit#78
kriskowal merged 6 commits into
mainfrom
enable-chunking-parameter

Conversation

@kriskowal

Copy link
Copy Markdown
Collaborator

Summary

  • Extend the ?enable-chunking URL flag to accept an optional positive-integer byte count (e.g. ?enable-chunking=50000) that overrides the chain's configured chunk_size_limit_bytes for manual testing.
  • Presence-only behavior (?enable-chunking) is unchanged — still uses the chain config.
  • Invalid values (non-integer, zero, negative) fall back to the chain config and pop a toast.warn so the tester notices the typo.

Test plan

  • No flag → bundles install in one transaction (chain limit ignored).
  • ?enable-chunking → small bundles single-tx, bundles above chain limit chunked.
  • ?enable-chunking=10000 → preflight toast reports N chunks ≈ ceil(compressedSize / 10000).
  • ?enable-chunking=999999999 → normally-chunked bundle goes single-tx.
  • ?enable-chunking=abc / =0 / =-5 / =1.5 → warning toast on load, behaves as presence-only.

🤖 Generated with Claude Code

Accept an optional positive-integer byte value on the enable-chunking URL
param (e.g. ?enable-chunking=50000) so testers can drive the chunking
boundary by hand without governance-changing the chain. Invalid values
fall back to the chain config and surface a warning toast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@netlify

netlify Bot commented Apr 29, 2026

Copy link
Copy Markdown

Deploy Preview for cosmos-proposal-builder ready!

Name Link
🔨 Latest commit c591440
🔍 Latest deploy log https://app.netlify.com/projects/cosmos-proposal-builder/deploys/69f3f18187e30a00080801e1
😎 Deploy Preview https://deploy-preview-78--cosmos-proposal-builder.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

kriskowal and others added 2 commits April 29, 2026 05:52
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for an optional numeric ?enable-chunking=<bytes> query param to override the chain-configured chunk size limit (while keeping the presence-only ?enable-chunking behavior unchanged), and warns testers when an invalid override value is provided.

Changes:

  • Parse enable-chunking as either presence-only or a positive-integer byte override.
  • Emit a toast.warn when an invalid override value is present and fall back to chain config.
  • Apply the override when computing chunkSizeLimit for bundle installation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/config/agoric/agoric.tsx Outdated
if (invalidOverrideRaw === null) return;
toast.warn(
`Ignoring invalid enable-chunking value ${JSON.stringify(invalidOverrideRaw)}; ` +
`expected a positive integer byte count. Falling back to chain config.`,

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useEffect will run twice in development under React.StrictMode, so this warning toast can appear twice for the same invalid URL value. Consider deduping by providing a stable toastId (or guarding with a useRef flag) so testers only see one warning per page load.

Suggested change
`expected a positive integer byte count. Falling back to chain config.`,
`expected a positive integer byte count. Falling back to chain config.`,
{
toastId: `agoric-enable-chunking-invalid-${invalidOverrideRaw}`,
},

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — added a stable toastId (enable-chunking-invalid-${invalidOverrideRaw}) so StrictMode's double-invoke doesn't double-toast. Pushed in 7f487c2.

Comment thread src/config/agoric/agoric.tsx Outdated
Comment on lines +80 to +82
const parsed = Number(raw);
if (!Number.isInteger(parsed) || parsed <= 0) {
return {

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation uses Number(raw) + Number.isInteger, which will accept non-decimal forms like 1e3, 0x10, or values with surrounding whitespace. If the intent is a strict positive base-10 byte count (per PR description), validate with a digit-only check (e.g., /^[1-9]\d*$/) before parsing so typos don’t silently become valid overrides.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these forms are acceptable.

Comment thread src/config/agoric/agoric.tsx Outdated
Comment on lines +62 to +66
const { enableChunking, chunkSizeOverride, invalidOverrideRaw } =
useMemo(() => {
const params = new URLSearchParams(window.location.search);
if (!params.has("enable-chunking")) {
return {

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds new URL parsing + warning behavior for enable-chunking, but there are no tests covering the override parsing or the invalid-value toast.warn path. Since there is already a src/config/agoric/agoric.spec.tsx, consider adding a test that sets window.location.search (or history.pushState) to an invalid value and asserts the warning fires and chunking falls back to chain config.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the parser into an exported pure helper parseEnableChunking(search) and added unit tests in agoric.spec.tsx covering: absent param, valueless presence, valid positive-integer override, and invalid raw values ("0", "-5", "1.5", "abc"). Pushed in 7f487c2.

kriskowal and others added 2 commits April 30, 2026 23:51
- Extract URL parsing into exported pure helper parseEnableChunking for testability
- Dedupe the invalid-value warning toast with a stable toastId so React.StrictMode does not pop it twice in dev
- Add unit tests covering absent / valueless / valid override / invalid override

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract parseEnableChunking + EnableChunkingState into sibling enableChunking.ts so the pure helper does not have to be exported from a component module.
- Switch state shape to a discriminated union (disabled / enabled / override / invalid) so impossible combinations like enabled-and-invalid-with-bytes can no longer be represented; consumers branch on .kind.
- Replace toastId-based dedupe with a useRef one-shot guard so React.StrictMode's double-invoke cannot pop the warning twice regardless of toastify timing.
- Rephrase the warning toast to drop "fall back to chain config" jargon in favor of "Using the chain's chunk_size_limit_bytes instead."
- Add spec coverage for leading-`?`-less search strings and repeated occurrences of the param.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kriskowal

Copy link
Copy Markdown
Collaborator Author

Aggregated multi-agent review of d4530bd. Acted on the high-value findings:

  • Tagged-union refactor. EnableChunkingState is now a discriminated union (disabled | enabled | override | invalid); consumers branch on .kind. Eliminates impossible combinations like "enabled with invalidRaw=null and override=42 simultaneously" and lets the chunkSizeLimit derivation read as a small switch rather than three nullable fields.
  • Helper relocated to src/config/agoric/enableChunking.ts. Exporting a pure URL parser from a 400-line component file was the only path the spec had to it; a sibling .ts reads cleaner and matches how src/utils/updateSearchString.ts is organized.
  • One-shot warning via useRef. toastId-based dedupe is timing-dependent under React StrictMode (the first toast.warn may not have registered before the second mount fires), so swapped to a deterministic useRef(false) guard.
  • Warning text reworded to drop "fall back to chain config" jargon — now says "Using the chain's chunk_size_limit_bytes instead."
  • Spec additions to pin the contract on a leading-?-less search string and on repeated enable-chunking occurrences (first wins).

Not addressed here, deliberately:

  • Stricter parser (rejecting 1e6, 0x10, leading whitespace, +5, etc.). Per your earlier reply, "all of these forms are acceptable" — so leaving the lenient Number() coercion as-is.
  • One reviewer flagged a pre-existing hazard from PR Feature flag for bundle chunking #77, unrelated to this PR's diff: if swingset_params.chunk_size_limit_bytes is ever returned as "0" (or a negative numeric string), chunkSizeLimit becomes 0/negative and the loop in src/installBundle/bundle.ts:59 (for (let i = 0; i < bytes.byteLength; i += chunkSizeLimit)) iterates forever. Today the chain doesn't return such a value, but the client has no defense. Worth a follow-up issue — happy to file one separately.

pluralizeEn already prefixes the count ("7 transactions"), so prefixing
${txCount} again produced "7 7 transactions" / "6 6 chunks" in the
preflight toast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kriskowal
kriskowal merged commit e940d6e into main May 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants