Skip to content

refactor(configurator): type codec.ts and its call sites (SL-016/017/024/026) - #478

Merged
jackgranatowski merged 2 commits into
claude/pr-469-audit-rebase-ggp0e4from
claude/audit-pr4-codec-types
Jul 2, 2026
Merged

jackgranatowski merged 2 commits into
claude/pr-469-audit-rebase-ggp0e4from
claude/audit-pr4-codec-types

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Summary

Fourth themed PR from the SLASHED technical-debt audit (PR #469), covering the codec.ts readability/type-safety group. Targets the long-lived integration branch claude/pr-469-audit-rebase-ggp0e4 per the agreed one-branch/multiple-PRs workflow (PR1–PR3 are still open on the same base and unmerged).

  • SL-016 (already landed in an earlier batch, carried forward on this branch): de-obfuscated internal names in configurator/src/lib/codec.ts.
  • SL-017 / SL-024: replaced any at codec.ts's 7 registry/options call sites (buildNameToIdMap, buildIdToNameMap, encode, decode ×2 params, readShareFromHash ×2 sites, readShareFromHashIfPresent) and its two consumers (App.svelte, CheatsheetPanel.svelte) with new TokenRegistry, ApiIndex, ApiIndexToken, SlashedClass, ClassIndex, TokenRegistryEntry and DecodeOptions interfaces added to configurator/src/types.ts. Malformed JSON/options shapes are now caught at compile time instead of silently no-op'ing at runtime.
    • Two call sites (CheatsheetPanel.svelte's .filter() predicates over the raw JSON imports) are left with inferred types rather than an explicit ApiIndexToken/SlashedClass annotation — svelte-check rejected the explicit annotation there because the generated JSON's tier/kind fields are widened to plain string by TS's JSON-module inference, which isn't assignable to the interfaces' literal-union fields. Inference gives the same safety without the mismatch.
    • The catch (err: any) in decode() is left as-is — SL-017/024 is about parameter/data typing, not catch-clause error typing.
  • SL-026: added a comment explaining why MAX_VALUE_BYTES and MAX_ID happen to share the same value (65535) despite being independent limits, so a future edit doesn't conflate them.

Test plan

  • npx tsc --noEmit — clean
  • npx svelte-check --tsconfig ./tsconfig.json — 0 errors, 0 warnings
  • npm run test:unit (vitest) — 72/72 passed, including codec.test.js, css.test.js, share.test.js unchanged

Generated by Claude Code

…ared limit constants

SL-017/024: replace `any` at codec.ts's 7 registry/options parameters and
its two consumers (App.svelte, CheatsheetPanel.svelte) with new TokenRegistry,
ApiIndex, SlashedClass and DecodeOptions interfaces in types.ts, so malformed
JSON or option shapes are caught at compile time instead of silently
no-op'ing at runtime.

SL-026: document why MAX_VALUE_BYTES and MAX_ID share the same numeric
value (65535) despite being independent limits, so a future edit doesn't
assume they're the same constant in disguise.
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 73f67110-fe67-4b35-90ca-2459ee106952

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/audit-pr4-codec-types

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Refactor configurator codec typing and document shared 65535 limits

✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Replaces any in codec registry/options parameters with shared interfaces in types.ts.
• Types generated JSON consumers in App.svelte and CheatsheetPanel.svelte to prevent silent
 no-ops.
• Documents why value-length and token-id limits both equal 65535 despite being independent.
Diagram

graph TD
  app["App.svelte"] --> types["types.ts"]
  cheat["CheatsheetPanel.svelte"] --> types --> codec["lib/codec.ts"]
  app --> apiJson[("api-index.generated.json")]
  cheat --> classJson[("classes.generated.json")]
  codec --> regJson[("token-registry.generated.json")]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add runtime validation (zod/io-ts) at JSON/code boundaries
  • ➕ Catches malformed/generated JSON at runtime, not just at compile time
  • ➕ Can produce user-friendly error messages instead of silent fallbacks
  • ➖ Adds dependency and runtime overhead
  • ➖ Likely redundant if JSON is always generated by trusted scripts in CI
2. Preserve literal unions from generated JSON via `as const` / generated `.d.ts`
  • ➕ Allows explicit annotations in Svelte filters without widening-to-string issues
  • ➕ Keeps strongest possible typing for enum-like fields (e.g., tier/kind)
  • ➖ Requires changes to JSON generation/build tooling
  • ➖ Can increase TS compile cost and type verbosity

Recommendation: The PR’s approach (shared interfaces + typed parameters) is the best fit for a generated-data pipeline: it removes any from key boundaries with minimal churn and no runtime cost. Consider runtime validation only if these JSON inputs can be user-supplied or fetched dynamically; otherwise compile-time typing is sufficient.

Files changed (4) +73 / -12

Enhancement (1) +54 / -0
types.tsAdd interfaces for generated JSON shapes and codec decode options +54/-0

Add interfaces for generated JSON shapes and codec decode options

• Introduces 'ApiIndex', 'ApiIndexToken', 'ClassIndex', 'SlashedClass', 'TokenRegistry', 'TokenRegistryEntry', and 'DecodeOptions' to centralize typing for generated JSON and codec call sites. This enables compile-time detection of malformed shapes and more explicit contracts across the configurator.

configurator/src/types.ts

Refactor (3) +19 / -12
App.svelteType api-index JSON cast via ApiIndex interface +2/-2

Type api-index JSON cast via ApiIndex interface

• Replaces the 'tokensRaw as any' cast with 'tokensRaw as ApiIndex' when normalizing the generated API index input. This tightens the boundary so missing/incorrect 'tokens' shape is surfaced by TypeScript instead of being masked by 'any'.

configurator/src/App.svelte

CheatsheetPanel.svelteRemove 'any' in token filtering and type class filtering +4/-3

Remove 'any' in token filtering and type class filtering

• Drops the 'any' annotation in the tokens '.filter()' predicate (letting inference apply) and annotates class filtering with 'SlashedClass'. This keeps Svelte/TS happy with JSON module type widening while still improving safety in downstream use.

configurator/src/components/panels/CheatsheetPanel.svelte

codec.tsIntroduce TokenRegistry/DecodeOptions typing and clarify 65535 limits +13/-7

Introduce TokenRegistry/DecodeOptions typing and clarify 65535 limits

• Types the registry and options parameters for encode/decode/share-reading APIs using 'TokenRegistry' and 'DecodeOptions', removing 'any' at the core codec boundary. Adds an explanatory comment documenting why MAX_VALUE_BYTES and MAX_ID share 65535 without being the same constraint.

configurator/src/lib/codec.ts

@qodo-code-review

qodo-code-review Bot commented Jul 2, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 6 rules

Grey Divider


Remediation recommended

1. sanitize option ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
readShareFromHash() is typed to accept DecodeOptions (including sanitize), but it always passes
sanitizeValue to decode() and never uses options.sanitize. This creates a misleading exported API
and prevents callers from customizing sanitization as the type signature implies.
Code

configurator/src/lib/codec.ts[R267-271]

+export function readShareFromHash(hashOrParam: string, options: DecodeOptions = {}): Record<string, string> {
+  const knownTokensSet = new Set(tokensData.tokens.map((tok) => tok.name));
  const isKnown = options.isKnown ?? ((name: string) => knownTokensSet.has(name));
  let trimmed = String(hashOrParam ?? "").trim();
  const match = trimmed.match(SHARE_PARAM_RE);
Relevance

⭐⭐⭐ High

Team often accepts correctness fixes in configurator codec APIs (e.g., codec.ts hardening accepted
in PR #427).

PR-#427
PR-#462
PR-#429

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
DecodeOptions explicitly includes a sanitize function, and readShareFromHash is declared to accept
DecodeOptions, but its body always passes sanitizeValue into decode() and never references
options.sanitize.

configurator/src/types.ts[70-74]
configurator/src/lib/codec.ts[267-276]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`readShareFromHash()` now accepts `DecodeOptions`, whose type includes an optional `sanitize` function, but the implementation hard-codes `sanitizeValue` and discards `options.sanitize`. This makes the public helper’s API contract incorrect.

### Issue Context
- If the intent is that `readShareFromHash()` always enforces `sanitizeValue` for safety, its parameter type should not advertise `sanitize`.
- If customization is intended, forward the caller-provided sanitizer (ideally composing it with `sanitizeValue` so baseline sanitization still occurs).

### Fix Focus Areas
- configurator/src/lib/codec.ts[267-276]
- configurator/src/types.ts[70-74]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread configurator/src/lib/codec.ts Outdated
…re-link readers

readShareFromHash()/readShareFromHashIfPresent() were typed to accept
DecodeOptions.sanitize but always forced sanitizeValue internally (a
CSS-injection safeguard that must not be caller-overridable) and silently
discarded any options.sanitize passed in. Narrow their accepted options to
a new ShareOptions type (isKnown only) so the signature matches actual
behavior instead of advertising a knob that does nothing.

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in 2af608f. readShareFromHash()/readShareFromHashIfPresent() always forced sanitizeValue (intentionally, since it's a CSS-injection safeguard that shouldn't be caller-overridable on the public share-link entry points), but the new DecodeOptions type advertised a sanitize knob that did nothing. Narrowed their accepted options to a new ShareOptions type (isKnown only) so the signature now matches actual behavior instead of silently discarding an option no real caller ever passed.


Generated by Claude Code

@jackgranatowski
jackgranatowski merged commit a5975d3 into claude/pr-469-audit-rebase-ggp0e4 Jul 2, 2026
9 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