Repository navigation
feat: add correctness/each-index-key — flag {#each} blocks keyed by their index - #266
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds the ChangesEach-index-key correctness rule
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SvelteParser
participant collectEachBlocks
participant correctnessEachIndexKey
participant Diagnostic
SvelteParser->>collectEachBlocks: traverse each blocks
collectEachBlocks->>correctnessEachIndexKey: provide indexKey facts
correctnessEachIndexKey->>Diagnostic: report matching blocks
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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: 1
🧹 Nitpick comments (1)
packages/core/test/component-parse.test.ts (1)
27-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the two unwrapping branches.
isIndexKeynow explicitly unwrapsTSSatisfiesExpressionandTSAsExpression, but these tests exercise neither. Add one case for each wrapper so parser-AST changes cannot silently disable detection.🤖 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 `@packages/core/test/component-parse.test.ts` around lines 27 - 46, Extend the each-block key tests around isIndexKey to cover both AST unwrapping branches: add one index-key case wrapped in TSSatisfiesExpression and another wrapped in TSAsExpression, asserting each produces hasKey: true with indexKey: true. Keep the existing direct, renamed, composite, and non-index key coverage unchanged.
🤖 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 `@docs/superpowers/specs/2026-07-22-each-index-key-design.md`:
- Around line 39-43: Update the “Not detected (non-goals)” section to stop
labeling wrapped index expressions and composite keys such as String(i),
template interpolation, and item.id + '-' + i as legitimate non-goals. Either
specify that index-dependent key expressions are detected broadly, or explicitly
describe these forms as an intentional v1 limitation; retain only accurate
non-goals such as missing index bindings, unkeyed blocks, and constant-list
blocks.
---
Nitpick comments:
In `@packages/core/test/component-parse.test.ts`:
- Around line 27-46: Extend the each-block key tests around isIndexKey to cover
both AST unwrapping branches: add one index-key case wrapped in
TSSatisfiesExpression and another wrapped in TSAsExpression, asserting each
produces hasKey: true with indexKey: true. Keep the existing direct, renamed,
composite, and non-index key coverage 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0f1d861-98ea-42e6-a6c8-323e4e1cfbf4
⛔ Files ignored due to path filters (1)
packages/action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (12)
.changeset/each-index-key.mddocs/src/content/docs/ja/rules/correctness/each-index-key.mddocs/src/content/docs/rules/correctness/each-index-key.mddocs/superpowers/plans/2026-07-22-each-index-key.mddocs/superpowers/specs/2026-07-22-each-index-key-design.mdpackages/core/src/component-parse.tspackages/core/src/component.tspackages/core/src/index.tspackages/core/src/rules/correctness/each-index-key.tspackages/core/src/rules/index.tspackages/core/test/component-parse.test.tspackages/core/test/each-index-key.test.ts
String(i), `${i}`, and i.toString() are still position-based identity,
just wrapped — extend correctness/each-index-key's detection beyond the
bare identifier case, unwrapping TS satisfies/as at every recursion step.
Document the trivial-stringification detection and stop calling index-plus-item-data composite keys unconditionally "legitimate" — they dodge Svelte's duplicate-key error but still lose move-tracking, so state the trade-off instead of only the upside. en/ja rule pages and the design spec updated together.
Rebuild the bundled action dist to pick up the each-index-key detection change in @svelte-vitals/core.
…, detect index coercions
Exempt length-only lists (Array(n), [...Array(n)], Array.from({length: n})) at
the each-block collector - both each-key and each-index-key were false-
positiving on skeleton/placeholder lists that have no item identity to key by.
Also widen correctness/each-index-key's index-coercion detection to Number(i)
and i + '' (either operand order), and unwrap non-null assertions (i!) in the
key expression alongside the existing satisfies/as TS wrappers.
kit-module-parse.ts and vite-config-parse.ts each carried their own unwrapTs (satisfies/as only). Delete both local copies and import the one now exported from component-parse.ts, which also unwraps non-null assertions (x!). Broadening TS-wrapper unwrapping in the Kit-module and Vite-config parsers is strictly more correct; no existing test pinned non-null-assertion behavior there.
…rrections Document the widened index-coercion detection (Number(i), i + '', TS non-null i!) and the shared length-only-list exemption in both the en and ja rule pages for correctness/each-index-key and correctness/each-key. Correct the design spec's stale Testing-section wording and record the actual Detection/non-goals behavior shipped in this review wave.
Rebuild the bundled GitHub Action dist to pick up the core fixes/refactor from this review wave (identity-free-list exemption, index-coercion detection, shared unwrapTs).
1 participant
Summary
First of three rules from the Svelte best-practices survey (
documentation/docs/07-misc/01-best-practices.md): the official guidance is explicit — "The key must uniquely identify the object. Do not use the index as a key" — but nothing in the toolchain surfaces it.correctness/each-index-key(warning) flags{#each items as item, i (i)}: an index key gives items position-based identity, the same failure mode as an unkeyed block (element state, focus, and transitions stick to positions on reorder/insert/remove), masked by a visible key that makes the block look safe.Sister rule to
correctness/each-key(unkeyed detection), kept separate for granular off/severity control. A block never triggers both.How it works
collectEachBlockssets an optionalEachBlockFact.indexKeywhen the key expression — aftersatisfies/asunwrapping — is exactly the block's index binding. Deliberately conservative: composite keys containing the index ((item.id + '-' + i)adds uniqueness), wrapped forms (String(i)), no-index blocks, and itemless/constant-list blocks are never flagged. Rides the existing component channel; inlinesvelte-vitals-disable-next-linesuppression works (verified empirically).Review process
3 tasks via subagent-driven development with per-task reviews, then a final adversarial whole-branch review running 21 empirical probes against the built dist (nested/shadowed indexes, snippets, await blocks, TS wrappers, parenthesized keys, suppression, mutual exclusivity with each-key) — all behaved per spec; READY TO MERGE with zero findings.
Verification
pnpm build/pnpm typecheck/pnpm lintpass (2 pre-existing warnings inmeta-object.test.tsonly)pnpm test: core 603, cli 694, vite 162, action 15, mcp 20 — all greendocs-linksgate green; changeset (minor × core/cli/vite/mcp);packages/action/distrebuilt via full workspace build🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
{#each}blocks keyed by their own index.Documentation