Add cached settings for listViewOrderbookFilters #1700
Conversation
|
Warning Rate limit exceeded@hardingjam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA new utility, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SvelteComponent
participant cachedWritableStore
participant localStorage
User->>SvelteComponent: Interacts with UI (e.g., toggles a setting)
SvelteComponent->>cachedWritableStore: set(newValue)
cachedWritableStore->>localStorage: serialize and store value
localStorage-->>cachedWritableStore: Confirms storage (silently handles errors)
cachedWritableStore-->>SvelteComponent: Emits updated value (reactive)
SvelteComponent-->>User: UI updates reflect new setting
Note over cachedWritableStore, localStorage: On initialization, attempts to read and deserialize value from localStorage, falling back to default if unavailable or on error.
sequenceDiagram
participant PageData
participant LayoutSvelte
participant cachedSettings
PageData->>LayoutSvelte: Provides settings in $page.data.stores
LayoutSvelte->>cachedSettings: Sets cachedSettings with current settings value
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hardingjam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
packages/ui-components/src/lib/index.ts(1 hunks)packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts(1 hunks)packages/webapp/src/lib/index.ts(0 hunks)packages/webapp/src/lib/stores/settings.ts(1 hunks)packages/webapp/src/routes/+layout.svelte(1 hunks)packages/webapp/src/routes/orders/+page.svelte(1 hunks)packages/webapp/src/routes/vaults/+page.svelte(1 hunks)tauri-app/src/lib/storesGeneric/fetchableStore.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/webapp/src/lib/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/webapp/src/lib/stores/settings.ts (2)
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts (1)
cachedWritableStore(30-64)packages/ui-components/src/lib/index.ts (2)
cachedWritableStore(120-120)ConfigSource(71-71)
🔇 Additional comments (11)
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts (1)
1-64: Well-implemented persistent store with good error handling!This implementation of a cached writable store is solid. I particularly like:
- Comprehensive JSDoc with clear examples
- Proper TypeScript typing with generics
- Robust error handling with silent fallbacks
- Clean separation of concerns with getCache/setCache helpers
The store correctly handles localStorage persistence while gracefully degrading when localStorage is unavailable or errors occur, which is excellent for resilience.
tauri-app/src/lib/storesGeneric/fetchableStore.ts (1)
2-2: Import path change looks correct.The update from a relative import to an absolute alias path is appropriate for the new utility location.
packages/ui-components/src/lib/index.ts (1)
120-120: Export of the new cachedWritableStore utility looks good.The cachedWritableStore is now correctly exported alongside other stores, making it available for use throughout the application.
packages/webapp/src/routes/orders/+page.svelte (2)
5-5: Great improvement using persistent settings!Importing settings directly from the centralized store rather than relying on page data improves maintainability and ensures user preferences persist across sessions.
14-15: Properly simplified store destructuringThe restructuring correctly removes stores that are now imported directly from the settings module, making the code cleaner and more maintainable.
packages/webapp/src/routes/vaults/+page.svelte (2)
5-6: Good implementation of persistent settings importsThese imports correctly leverage the new persistent stores for user preferences, ensuring a consistent approach across different views in the application.
18-19: Properly simplified store destructuringThe destructuring of page stores has been correctly adjusted to remove properties that are now imported from the centralized settings module.
packages/webapp/src/lib/stores/settings.ts (4)
1-23: Well-implemented settings store with proper error handlingThe
settingsstore implementation is robust with appropriate serialization, deserialization, and error handling. The JSDoc comments clearly explain the purpose and default values.
24-44: Good implementation of hideZeroBalanceVaults persistent storeThe store includes proper type checking in the deserializer to ensure the returned value is always a boolean, with a sensible default of
truewhen parsing fails.
46-66: Well-implemented showMyItemsOnly storeThis store correctly implements the PR objective of preserving the user's preference for displaying only data connected to their wallet. The error handling and type checking are robust.
68-88: Properly implemented activeSubgraphs storeThis store supports the PR objective of preserving active network selections. The implementation includes proper error handling with a sensible default empty object.
| import { settings as cachedSettings } from '$lib/stores/settings'; | ||
|
|
||
| const { settings } = $page.data.stores; | ||
| cachedSettings.set(settings); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making the settings synchronization reactive.
The current implementation synchronizes settings only once on component initialization. For consistency, you may want to make this synchronization reactive to handle subsequent changes to page data.
-const { settings } = $page.data.stores;
-cachedSettings.set(settings);
+$: if ($page.data.stores?.settings) {
+ cachedSettings.set($page.data.stores.settings);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { settings as cachedSettings } from '$lib/stores/settings'; | |
| const { settings } = $page.data.stores; | |
| cachedSettings.set(settings); | |
| import { settings as cachedSettings } from '$lib/stores/settings'; | |
| $: if ($page.data.stores?.settings) { | |
| cachedSettings.set($page.data.stores.settings); | |
| } |
There was a problem hiding this comment.
it's a hard-coded value that's fetched from strategies repo, so it won't change
There was a problem hiding this comment.
@hardingjam Thanks for implementing the reactive synchronization! This improvement ensures that any subsequent changes to page data will be properly reflected in the cached settings. Great work!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| /** | ||
| * A persistent store that holds the currently selected order hash. | ||
| * | ||
| * This setting is saved to local storage and persists between sessions. | ||
| * | ||
| * @default "" - Empty string by default | ||
| * @returns A writable store containing the order hash string | ||
| */ | ||
| export const orderHash = cachedWritableStore<string>( | ||
| 'settings.orderHash', | ||
| '', | ||
| (value) => value, | ||
| (str) => str || '' | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type validation to the orderHash deserializer
The implementation for the orderHash store looks good, but consider adding type validation in the deserializer similar to other stores.
- (str) => str || ''
+ (str) => {
+ return typeof str === 'string' ? str : '';
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * A persistent store that holds the currently selected order hash. | |
| * | |
| * This setting is saved to local storage and persists between sessions. | |
| * | |
| * @default "" - Empty string by default | |
| * @returns A writable store containing the order hash string | |
| */ | |
| export const orderHash = cachedWritableStore<string>( | |
| 'settings.orderHash', | |
| '', | |
| (value) => value, | |
| (str) => str || '' | |
| ); | |
| /** | |
| * A persistent store that holds the currently selected order hash. | |
| * | |
| * This setting is saved to local storage and persists between sessions. | |
| * | |
| * @default "" - Empty string by default | |
| * @returns A writable store containing the order hash string | |
| */ | |
| export const orderHash = cachedWritableStore<string>( | |
| 'settings.orderHash', | |
| '', | |
| (value) => value, | |
| (str) => { | |
| return typeof str === 'string' ? str : ''; | |
| } | |
| ); |
There was a problem hiding this comment.
@hardingjam Thanks for implementing the type validation! This small change will improve the code's robustness by ensuring consistent handling of unexpected data types from localStorage. Good job on maintaining type safety across your stores.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
tauri-app/src/lib/storesGeneric/fetchableStore.ts (1)
45-52: 🧹 Nitpick (assertive)Use Number.parseInt instead of global parseInt.
For consistency with the UI components package, update to use
Number.parseInthere as well.export const fetchableIntStore = (key: string, handleFetch: () => Promise<number>) => fetchableStore<number>( key, 0, handleFetch, (v) => v.toString(), - (s) => parseInt(s), + (s) => Number.parseInt(s), );🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
tauri-app/src/lib/stores/settings.ts (1)
159-159:⚠️ Potential issueFix logical error in condition check
There's a logic error in the condition that checks if orderbooks are undefined.
- !$settings?.orderbooks === undefined || + $settings?.orderbooks === undefined ||The current expression first negates the result of
$settings?.orderbooks(converting it to a boolean) and then compares that boolean toundefined, which is likely not the intended logic.
♻️ Duplicate comments (1)
packages/webapp/src/routes/+layout.svelte (1)
17-20: Consider making the settings synchronization reactive.The current implementation synchronizes settings only once on component initialization. For consistency, you may want to make this synchronization reactive to handle subsequent changes to page data.
-const { settings } = $page.data.stores; -cachedSettings.set(settings); +$: if ($page.data.stores?.settings) { + cachedSettings.set($page.data.stores.settings); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
packages/ui-components/src/lib/index.ts(1 hunks)packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts(1 hunks)packages/webapp/src/routes/+layout.svelte(1 hunks)packages/webapp/src/routes/orders/+page.svelte(1 hunks)packages/webapp/src/routes/vaults/+page.svelte(1 hunks)tauri-app/src/lib/mocks/mockConfigSource.ts(1 hunks)tauri-app/src/lib/stores/settings.test.ts(0 hunks)tauri-app/src/lib/stores/settings.ts(3 hunks)tauri-app/src/lib/storesGeneric/cachedWritableStore.ts(0 hunks)tauri-app/src/lib/storesGeneric/detailStore.ts(1 hunks)tauri-app/src/lib/storesGeneric/fetchableStore.ts(1 hunks)tauri-app/src/lib/storesGeneric/listStore.ts(1 hunks)tauri-app/src/lib/storesGeneric/settingStore.ts(1 hunks)
💤 Files with no reviewable changes (2)
- tauri-app/src/lib/stores/settings.test.ts
- tauri-app/src/lib/storesGeneric/cachedWritableStore.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tauri-app/src/lib/mocks/mockConfigSource.ts (1)
packages/ui-components/src/lib/index.ts (2)
mockConfigSource(141-141)ConfigSource(71-71)
🪛 Biome (1.9.4)
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts
[error] 78-78: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 115-115: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
🔇 Additional comments (15)
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts (1)
30-64: Well-implemented localStorage persistence with good error handling.The implementation properly handles localStorage initialization, fallback to default values, and silently ignores storage failures. This is a robust approach that ensures the application continues to function even when localStorage is unavailable.
tauri-app/src/lib/storesGeneric/fetchableStore.ts (1)
2-2: Import source updated correctly.The import has been successfully updated to use the shared implementation from the UI components package.
tauri-app/src/lib/storesGeneric/detailStore.ts (1)
2-2: Import source updated correctly.The import has been successfully updated to use the shared implementation from the UI components package.
tauri-app/src/lib/storesGeneric/listStore.ts (1)
6-6: Import source updated correctly.The import has been successfully updated to use the shared implementation from the UI components package.
tauri-app/src/lib/storesGeneric/settingStore.ts (1)
2-2: Good refactoring to use the shared implementation.The migration from a local implementation to the shared
@rainlanguage/ui-componentspackage is a positive change that promotes code reuse and simplifies maintenance.packages/ui-components/src/lib/index.ts (1)
120-125: Well-organized exports of caching utilities.The new export statements properly expose the cached writable store utilities for use across the application, following the established pattern in this file and organizing them in the appropriate section.
packages/webapp/src/routes/orders/+page.svelte (2)
5-5: Good switch to centralized persistent stores.Importing the settings directly from the central store improves code organization and enables persistent user preferences across sessions.
14-15: LGTM!The updated destructuring maintains proper code organization.
tauri-app/src/lib/mocks/mockConfigSource.ts (1)
1-37: Well-structured mock configuration for testingThe mock configuration provides a good baseline for testing with the essential components needed. It includes a network, subgraph, orderbook, deployer, metaboard, and accounts.
packages/webapp/src/routes/vaults/+page.svelte (2)
5-6: Appropriate migration to centralized settings storesThe component now properly imports settings from the centralized stores instead of relying on page data, which aligns with the PR's goal to save user preferences across sessions.
8-19: Clean destructuring of remaining page data storesThe code correctly destructures only the stores that are still needed from page data after migrating others to centralized stores.
tauri-app/src/lib/stores/settings.ts (4)
2-2: Appropriately updated import path for cached storesThe code now correctly imports the cached store utilities from the UI components package instead of local implementation.
13-14: Good test preparationAdding imports for mock data and test utilities prepares the file for the new test suite.
277-380: Comprehensive test suite for settings reactivityThe added tests thoroughly verify that changes to the settings store correctly update dependent stores like
activeAccountsItemsandactiveSubgraphs. The tests cover important edge cases such as removing values and changing values.
282-307: Good test hygiene with store resetThe tests properly reset all store values before each test to prevent state leakage between tests, which is a best practice for testing reactive stores.
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts
Outdated
Show resolved
Hide resolved
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts
Outdated
Show resolved
Hide resolved
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts
Outdated
Show resolved
Hide resolved
| address: '0xOrderbookAddress1', | ||
| network: 'mainnet', | ||
| subgraph: 'uniswap', | ||
| label: 'Orderbook 1', |
There was a problem hiding this comment.
Fix inconsistency in subgraph reference
The orderbook references a 'uniswap' subgraph on line 19, but only 'mainnet' is defined in the subgraphs object (line 13). This could cause issues in tests that attempt to access this subgraph.
- subgraph: 'uniswap',
+ subgraph: 'mainnet',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address: '0xOrderbookAddress1', | |
| network: 'mainnet', | |
| subgraph: 'uniswap', | |
| label: 'Orderbook 1', | |
| address: '0xOrderbookAddress1', | |
| network: 'mainnet', | |
| subgraph: 'mainnet', | |
| label: 'Orderbook 1', |
There was a problem hiding this comment.
this mock is fine
There was a problem hiding this comment.
@hardingjam I understand, thanks for the clarification. I'll accept the mock configuration as is.
✏️ Learnings added
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1700
File: tauri-app/src/lib/mocks/mockConfigSource.ts:17-20
Timestamp: 2025-04-28T10:58:15.643Z
Learning: In mock configuration files for testing purposes, such as mockConfigSource.ts, inconsistencies between referenced subgraphs and defined subgraphs may be intentional and acceptable for specific test scenarios.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| export const mockConfigSource: ConfigSource = { | ||
| networks: { | ||
| mainnet: { | ||
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a clearer mock URL format
While this works as mock data, using a URL with a placeholder PROJECT-ID might be confusing in tests.
- rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID',
+ rpc: 'https://mock.example.com/mainnet',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| rpc: 'https://mock.example.com/mainnet', |
There was a problem hiding this comment.
this mock is fine
There was a problem hiding this comment.
@hardingjam Understood! I'll respect your preference for the mock URL format.
✏️ Learnings added
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1700
File: tauri-app/src/lib/mocks/mockConfigSource.ts:6-6
Timestamp: 2025-04-28T10:58:11.112Z
Learning: In mock data for testing in this codebase, it's acceptable to use URL placeholders like 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID' as they clearly indicate the expected format for actual implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Reset store values before each test to prevent state leakage | ||
| beforeEach(() => { | ||
| // Reset all store values | ||
| settings.set(undefined); | ||
| activeAccountsItems.set({}); | ||
| activeSubgraphs.set({}); | ||
|
|
||
| // Then set our initial test values | ||
| settings.set(mockConfigSource); | ||
| activeAccountsItems.set({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two', | ||
| }); | ||
| activeSubgraphs.set({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet', | ||
| }); | ||
|
|
||
| // Verify initial state | ||
| expect(get(settings)).toEqual(mockConfigSource); | ||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two', | ||
| }); | ||
| expect(get(activeSubgraphs)).toEqual({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet', | ||
| }); | ||
| }); | ||
|
|
||
| test('should remove account if that account is removed', () => { | ||
| // Test removing an account | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| }, | ||
| }; | ||
|
|
||
| // Update settings - this should trigger the subscription | ||
| settings.set(newSettings); | ||
|
|
||
| // Check the expected result | ||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| }); | ||
| }); | ||
|
|
||
| test('should remove account if the value is different', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| name_two: 'new_value', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| }); | ||
| }); | ||
|
|
||
| test('should update active subgraphs when subgraph value changes', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: { | ||
| mainnet: 'new value', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
|
|
||
| test('should update active subgraphs when subgraph removed', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: { | ||
| testnet: 'testnet', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
|
|
||
| test('should reset active subgraphs when subgraphs are undefined', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: undefined, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider expanding test coverage
While the current tests cover the behavior of settings subscriptions well, consider adding tests for derived stores like activeNetwork, activeChain, etc., to ensure they also respond correctly to changes in settings.
There was a problem hiding this comment.
out of scope, created #1703
There was a problem hiding this comment.
@hardingjam Thanks for creating issue #1703 to track this suggestion! Keeping PRs focused in scope is a good practice.
🐰✨
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
❗ First, merge #1698
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit