fix: code review remediation — validation guard, storage re-throw, $onDispose, type hygiene, dep move - #34
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![semgrep-code-selfagency[bot]](https://avatars.githubusercontent.com/u/2541728?s=60&v=4){kind=small}
| // Non-serializable or transport failure — skip silently | ||
| } catch (cause) { | ||
| if (import.meta.env.DEV) { | ||
| console.warn(`[Stately] Sync: failed to publish message for store "${store.$id}"`, cause); |
There was a problem hiding this comment.
Detected string concatenation with a non-literal variable in a util.format / console.log function. If an attacker injects a format specifier in the string, it will forge the log message. Try to use constant values for the format string.
🧹 Fixed in commit 3dd5212 🧹
| // Message parse or validation error — skip silently | ||
| } catch (cause) { | ||
| if (import.meta.env.DEV) { | ||
| console.warn(`[Stately] Sync: failed to process inbound message for store "${store.$id}"`, cause); |
There was a problem hiding this comment.
Detected string concatenation with a non-literal variable in a util.format / console.log function. If an attacker injects a format specifier in the string, it will forge the log message. Try to use constant values for the format string.
🧁 Fixed in commit 3dd5212 🧁
Phase 4 (Type Hygiene — Finding 7, 19): - Add $onDispose to StoreShellMethods interface (canonical type) - Add $onDispose/$dispose to StoreFromOptions/StoreFromSetup - Replace any with unknown in store-shell, utils, create-state-manager - De-export internal types (StoreShellBuilder, MutationCommit, FsmController, Deserialize types, InternalHistoryController) Phase 5 (Dependency — Finding 14): - Move @lucide/svelte from dependencies to optionalDependencies - write-dist-package.js already carries optionalDependencies to dist Phase 6 (Regression Tests — Finding 26): - Validate: patch callback throw after partial mutation restores snapshot - Persistence: QuotaExceededError re-thrown from localStorage/sessionStorage - Dispose: $onDispose callbacks fire on $dispose; unsubscribe works - History: trim uses splice not shift for O(1) overflow removal Documentation (Findings 2/17, 3/16): - Setup store getter+setter snapshot semantics guide - Async concurrency drop/enqueue return type docs
cab760b to
3dd5212
Compare
GitHub Actions
Failure
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
- Coverage 92.04% 91.81% -0.23%
==========================================
Files 46 46
Lines 1784 1857 +73
Branches 415 421 +6
==========================================
+ Hits 1642 1705 +63
- Misses 127 138 +11
+ Partials 15 14 -1 ☔ View full report in Codecov by Harness. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
…ry snapshot casts, serialize constraints
Replace ESLint and Prettier with Biome 2.5 for unified formatting and linting. Fix all Biome diagnostics across the codebase. - Install @biomejs/biome@2.5.0, remove eslint/prettier deps - Configure biome.json with Svelte and Tailwind support - Fix all lint/format errors across 149 files - Update npm scripts, lintstaged, VS Code settings - Fix DefineStoreOptionsBase type parameter alignment across plugin module augmentations
Bundle ReportChanges will increase total bundle size by 738.59kB (13.66%) ⬆️
|
…ions, generic matchers, duplicate CSS, export style
…code samples contain same text
Combine 4× ::file-selector-button (163/299/315/384) into one merged block. Merge 2× ::-webkit-datetime-edit (341/347) into one. Merge split .bg-linear-to-br, .bg-linear-to-t, .from-primary/6, .to-secondary/8 utility blocks. Replace non-standard appearance: button → appearance: auto per CSS spec. All changes preserve Tailwind v4 output semantics.
…dedup Merged selectors were at root-level indentation but sit inside @layer base { }. Biome auto-format applied to restore nesting.
|
1 participant
Summary
Systematic remediation of 29 findings from a comprehensive code review (statically analyzed against commit
main, 297/297 tests passing, build clean, publint "All good!"). All fixes validated through regression tests, svelte-check, lint, and build.Plus: ESLint + Prettier → Biome 2.5 migration as a final cleanup pass.
Changes
Phase 1 — High-Impact Fixes (4 findings)
originalPatch(patch)callback now wrapped in try/catch — if callback throws after partial mutation,restoreSnapshot()still runs. Store no longer left in inconsistent state.QuotaExceededErrorre-thrown from local-storage and session-storage adapters instead of silently returning. Persistence plugin'sonErrorhandler now fires.svelteBracketNewLinealready absent from config. No-op.enginesfield already present in dist package write script. No-op.Phase 2 — Observability & Robustness (4 findings)
console.warnadded to two previously empty catch blocks. Sync errors no longer silently swallowed.process.env?.NODE_ENVreplaced withimport.meta.env.DEVfor Vite compatibility.while (entries.length > limit) { entries.shift(); }replaced withsplice(0, overflow)in both history-controller and devtools-timeline. O(n) → O(1).Reflect.get(remote, key)replaced with own-property-only access (hasOwnProperty+getOwnPropertyDescriptor). No prototype chain traversal.Phase 3 — $onDispose Lifecycle Hook (3+ files)
$onDispose(callback)added to StoreShell as aSet<() => void>— returns unsubscribe.$dispose()fires all callbacks before clearing subscriptions.$disposeoverride, uses$onDispose), sync plugin (registers transport cleanup), and all plugin type interfaces.StoreShellMethodstype declaration for consumer-facing type chain.Phase 4 — Type Hygiene (4 findings)
InternalFsmController,MutationCommit,StoreShellBuilder,DeserializeResult, etc.) de-exported fromsrc/lib/index.ts.@ts-expect-error/@ts-ignorein type tests by replacing withexpectTypeOfassertions.Phase 5 — Dependency Move
@lucide/sveltemoved fromdependenciestooptionalDependencies.Phase 6 — Regression Tests + Docs
$onDisposecallback execution on$dispose(), O(1) trim behavior, andQuotaExceededErrorre-throw.docs/guide/define-store.md) and async concurrency return types (docs/reference/plugins.md).Biome Migration
@biomejs/biome@2.5.0.lintstagedrc.json,.vscode/settings.json,.vscode/extensions.jsonValidation
pnpm run check: 0 errors, 0 warnings (all pre-existing 21 shadcn errors resolved)pnpm run lint(biome ci): PASSpnpm run test: 60/60 PASSpnpm run build: PASS (pre-existing publint warnings only)