feat: add noCache option to clear cached values#600
Open
RodrigoHamuy wants to merge 15 commits intopmndrs:mainfrom
Open
feat: add noCache option to clear cached values#600RodrigoHamuy wants to merge 15 commits intopmndrs:mainfrom
noCache option to clear cached values#600RodrigoHamuy wants to merge 15 commits intopmndrs:mainfrom
Conversation
- Add `store.clearPath(path)` to hard-delete a cached path, guarded by `__refCount` so it's a no-op while the path is still mounted - Add `clearOnUnmount?: boolean` per-input schema option that automatically clears the path when the component unmounts - Use a `Set` for clearOnUnmountPaths so add/remove correctly handles dynamic changes to the option between renders - Add integration tests covering mount/unmount lifecycle, nested folder paths, value preservation, and both clearOnUnmount mechanisms Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add `clearOnUnmount` prop to `LevaRootProps` that overrides individual input settings, clearing all paths from the store on component unmount - Add `clearOnUnmount` / `setClearOnUnmount` to `StoreType` and `Store` so the flag is readable at cleanup time without needing reactive state - Read `store.clearOnUnmount` in the useControls cleanup, falling back to the per-input Set when the store-level flag is false - Add test covering the store-level flag via `levaStore.setClearOnUnmount` Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 41b0ed9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a noCache feature enabling inputs to discard cached values on unmount across three mechanisms: panel-level via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 41b0ed9:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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/leva/src/useControls.test.tsx (1)
133-148: Consider adding a regression test forLevaRootprop lifecycle wiring.You already test
store.setClearOnUnmount(true)directly; adding one test that mounts/unmounts<LevaRoot clearOnUnmount />would better protect the prop-to-store lifecycle behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/useControls.test.tsx` around lines 133 - 148, Add a regression test that verifies the LevaRoot prop wiring to the store by mounting and unmounting <LevaRoot clearOnUnmount /> instead of calling levaStore.setClearOnUnmount(true) directly: render a component tree wrapped with LevaRoot (with clearOnUnmount prop), change a control value via levaStore.setValueAtPath('myNumber', 42, true), unmount the wrapper, then remount the same control and assert it reset to its default (like the existing NumberComponent assertions). Reference LevaRoot, levaStore, setValueAtPath and NumberComponent to locate where to add this test so the prop-to-store lifecycle behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/leva/src/components/Leva/LevaRoot.tsx`:
- Around line 110-112: The effect sets store.setClearOnUnmount(clearOnUnmount)
but never restores the previous flag, so capture the store's prior value before
setting and restore it in the effect cleanup to avoid leaving stale state when
this root unmounts or the store instance changes; specifically, in the useEffect
that calls store.setClearOnUnmount(clearOnUnmount) save the previous value
(e.g., const prev = store.clearOnUnmount or call a getter if available) then
return a cleanup that calls store.setClearOnUnmount(prev) (or sets false if no
getter exists), and ensure this logic runs whenever store or clearOnUnmount
changes.
---
Nitpick comments:
In `@packages/leva/src/useControls.test.tsx`:
- Around line 133-148: Add a regression test that verifies the LevaRoot prop
wiring to the store by mounting and unmounting <LevaRoot clearOnUnmount />
instead of calling levaStore.setClearOnUnmount(true) directly: render a
component tree wrapped with LevaRoot (with clearOnUnmount prop), change a
control value via levaStore.setValueAtPath('myNumber', 42, true), unmount the
wrapper, then remount the same control and assert it reset to its default (like
the existing NumberComponent assertions). Reference LevaRoot, levaStore,
setValueAtPath and NumberComponent to locate where to add this test so the
prop-to-store lifecycle behavior is covered.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/clear-path-on-unmount.mdpackages/leva/src/components/Leva/LevaRoot.tsxpackages/leva/src/store.tspackages/leva/src/types/internal.tspackages/leva/src/types/public.tspackages/leva/src/useControls.test.tsxpackages/leva/src/useControls.tspackages/leva/src/utils/data.tspackages/leva/src/utils/input.ts
- Add clear-on-unmount.stories.tsx with 4 stories covering per-input, panel-level, and imperative (store.clearPath) usage - Document clearOnUnmount input option in inputs.md - Document clearOnUnmount panel prop and store.clearPath in configuration.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The useEffect cleanup now resets the store flag to false when the panel unmounts (or the store prop changes), preventing stale clearOnUnmount state on custom stores. Also adds a test that exercises the prop→store wiring through LevaPanel rather than calling setClearOnUnmount directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
RodrigoHamuy
changed the title
clearOnUnmount option to remove inputs from store on unmount
RodrigoHamuy
changed the title
clearOnUnmount option to remove inputs from store on unmountclearOnUnmount option to clear cached values
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/leva/stories/clear-on-unmount.stories.tsx (1)
137-168: MakeClearPathremount behavior directly testable in-story.The comment says “next time the component remounts,” but this story has no mount/unmount control. Add a toggle so the stated behavior is demonstrable without leaving the story.
Suggested story tweak
export const ClearPath: StoryFn = () => { const store = useCreateStore() + const [mounted, setMounted] = React.useState(true) - const values = useControls({ position: { x: 0, y: 0 }, speed: 1 }, { store }) + const values = useControls({ position: { x: 0, y: 0 }, speed: 1 }, { store }) return ( <div> <LevaPanel store={store} /> + <button onClick={() => setMounted((m) => !m)}>{mounted ? 'Unmount' : 'Mount'}</button> - <div style={{ padding: 20, border: '1px solid `#444`', margin: '10px 0' }}> + {mounted && <div style={{ padding: 20, border: '1px solid `#444`', margin: '10px 0' }}> <p> <strong>store.clearPath</strong> imperatively discards a single cached input. </p> <pre>{JSON.stringify(values, null, ' ')}</pre> - </div> + </div>} <button onClick={() => { // Discard the cached position; next mount will use the initial value. store.clearPath('position') }}> Clear position cache </button> </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/stories/clear-on-unmount.stories.tsx` around lines 137 - 168, The ClearPath story describes remount behavior but offers no way to mount/unmount; modify the ClearPath component to add a local boolean state (e.g. isMounted via useState) and a "Toggle mount" button, then render the LevaPanel and the controls block conditionally based on isMounted so you can unmount and remount the UI in-story; keep using useCreateStore() to create the store and call store.clearPath('position') from the existing "Clear position cache" button so testers can clear the cached value and then toggle remount to observe the initial value re-applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/leva/stories/clear-on-unmount.stories.tsx`:
- Around line 137-168: The ClearPath story describes remount behavior but offers
no way to mount/unmount; modify the ClearPath component to add a local boolean
state (e.g. isMounted via useState) and a "Toggle mount" button, then render the
LevaPanel and the controls block conditionally based on isMounted so you can
unmount and remount the UI in-story; keep using useCreateStore() to create the
store and call store.clearPath('position') from the existing "Clear position
cache" button so testers can clear the cached value and then toggle remount to
observe the initial value re-applied.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/getting-started/configuration.mddocs/getting-started/inputs.mdpackages/leva/src/components/Leva/LevaRoot.tsxpackages/leva/src/useControls.test.tsxpackages/leva/stories/clear-on-unmount.stories.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/getting-started/inputs.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/leva/src/useControls.test.tsx
RodrigoHamuy
changed the title
clearOnUnmount option to clear cached valuesnoCache option to clear cached values
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/leva/src/useControls.ts (2)
208-213: Cleanup logic is correct; consider silencing the lint warning.The static analysis flags that the
forEachcallback should not return a value. Whilestore.clearPathreturnsvoid, thesetStateinside it technically returns the new state which could be interpreted as a return value.You can silence the lint by wrapping in braces or using
for...of:♻️ Optional: use for...of to avoid lint warning
return () => { store.disposePaths(paths) const pathsToClear = store.noCache ? paths : [...noCachePaths] - pathsToClear.forEach((path) => store.clearPath(path)) + for (const path of pathsToClear) { + store.clearPath(path) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/useControls.ts` around lines 208 - 213, The lint warning about returning a value from the forEach callback can be silenced by avoiding an implicit return; update the cleanup to iterate without returning by replacing pathsToClear.forEach((path) => store.clearPath(path)) with an explicit block or a for...of loop so the callback body does not implicitly return anything—apply this change around the existing store.disposePaths(paths) and ensure you keep references to store.clearPath and pathsToClear (or noCachePaths) intact.
161-165: Redundantdeletecall in fresh Set.The
noCachePathsSet is created fresh on each memo computation (line 142), so theelsebranch callingdeleteis a no-op—the path can't exist in a newly created Set.♻️ Suggested simplification
if (noCache) { noCachePaths.add(path) - } else { - noCachePaths.delete(path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/useControls.ts` around lines 161 - 165, The else branch calling noCachePaths.delete(path) is redundant because noCachePaths is a freshly created Set during the memo; remove the else/delete call and simplify the block so you only add the path when noCache is true (identify the variables noCachePaths and the conditional around noCache in useControls.ts to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/leva/stories/clear-on-unmount.stories.tsx`:
- Around line 145-168: The ClearPath story currently calls
store.clearPath('position') while useControls is still mounted so the call is a
no-op; modify the story (ClearPath) to demonstrate clearing by adding an
unmount/remount toggle: render the useControls + LevaPanel behind a conditional
controlled by a boolean state (e.g., isMounted) created with useState, move the
"Clear position cache" button so it first sets isMounted to false (unmounting
the controls) and then calls store.clearPath('position') (or provide two
buttons: one to unmount/remount and one to clear while unmounted), keeping
references to useCreateStore and store.clearPath unchanged so the demo shows the
cache being cleared on unmount.
---
Nitpick comments:
In `@packages/leva/src/useControls.ts`:
- Around line 208-213: The lint warning about returning a value from the forEach
callback can be silenced by avoiding an implicit return; update the cleanup to
iterate without returning by replacing pathsToClear.forEach((path) =>
store.clearPath(path)) with an explicit block or a for...of loop so the callback
body does not implicitly return anything—apply this change around the existing
store.disposePaths(paths) and ensure you keep references to store.clearPath and
pathsToClear (or noCachePaths) intact.
- Around line 161-165: The else branch calling noCachePaths.delete(path) is
redundant because noCachePaths is a freshly created Set during the memo; remove
the else/delete call and simplify the block so you only add the path when
noCache is true (identify the variables noCachePaths and the conditional around
noCache in useControls.ts to locate the change).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/clear-path-on-unmount.mddocs/getting-started/configuration.mddocs/getting-started/inputs.mdpackages/leva/src/components/Leva/LevaRoot.tsxpackages/leva/src/store.tspackages/leva/src/types/internal.tspackages/leva/src/types/public.tspackages/leva/src/useControls.test.tsxpackages/leva/src/useControls.tspackages/leva/src/utils/data.tspackages/leva/src/utils/input.tspackages/leva/stories/clear-on-unmount.stories.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/getting-started/inputs.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/leva/src/types/public.ts
- .changeset/clear-path-on-unmount.md
- packages/leva/src/useControls.test.tsx
- packages/leva/src/utils/input.ts
- packages/leva/src/utils/data.ts
… feedback - Export StoreType and Data from leva/plugin entrypoint so plugin-plot resolves the same type identity as the sanitize function signature - Fix plugin-plot import to use leva/plugin instead of bare path alias - Remove redundant noCachePaths.delete in useMemo (fresh Set each run) - Replace forEach with for...of in cleanup to avoid implicit-return lint - Add mount/unmount toggle to ClearPath story so clearPath behavior is demonstrable without leaving the story Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CI build cache (key: Linux-pnpm-build-) is never busted, so CI runs
with a compiled leva-plugin.cjs.d.ts that pre-dates the noCache/setNoCache
additions to StoreType. This causes a structural mismatch when plot-plugin
imports StoreType and passes it to createPlugin's sanitize.
Fix: replace the explicit StoreType annotation with a structural inline
type { get: (path: string) => any } — only the .get method is used in
sanitize, so any version of StoreType satisfies it. Also revert the
unnecessary StoreType/Data re-export from leva/plugin.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The build cache key used hashFiles('/packages/**/*') with an absolute
path — the leading slash makes it resolve to the runner root, not the
repo, so it always produced the same (empty) hash. The cache therefore
never busted, leaving CI with a compiled leva-plugin.cjs.d.ts that
pre-dates the noCache/setNoCache additions to StoreType.
Fix the glob to hashFiles('packages/**/*.ts', 'packages/**/*.tsx') so
the key changes whenever TypeScript sources change.
Also restore the explicit store: StoreType annotation in plot-plugin.ts
that was incorrectly weakened in the previous commit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
store.clearPath(path)— new method to imperatively discard the cached value of an input that is no longer mountednoCachetoInputOptions— per-input flag; whentruethe input's cached value is discarded from the store on unmountnoCachetoLevaRootProps— panel-level flag; whentrueall inputs managed by that panel have their cached values discarded on unmount, overriding the per-input settingSummary by CodeRabbit
New Features
noCacheoption to automatically clear individual input values from cache on unmount.noCacheprop to panels to clear all cached values on unmount.store.clearPath()method for imperatively removing specific cached values.Documentation