feat: add mod preset persistence (ADR-008)#154
Merged
Conversation
closes #109 - Add support for saving and clearing default mod presets. - Persist default mods via `preferences.svelte.ts` and inject into bare URLs during bootstrap. - Update tests to cover mod persistence behavior. - Document the feature with ADR-008: Mod Preset Persistence.
…resolution - Introduce `resolveVersionedPath` for handling versioned routes. - Simplify bootstrap navigation logic to include preferences. - Remove outdated `canonicalizeMalformedVersionURL` method. - Update tests to reflect the new routing approach.
…ith `mods` and `tileset` - Rename APIs and properties for clarity: `defaultMods` -> `mods`, `preferredTileset` -> `tileset`. - Update localStorage keys to `cbn-guide:mods` and `cbn-guide:tileset`. - Refactor persistence methods and related tests. - Update ADR-008 to reflect key renaming.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Caution Review failedPull request was closed or merged during review Summary by CodeRabbit
WalkthroughAdds localStorage-backed mod-preset persistence, injects saved mods into bare URLs during bootstrap via replaceState, renames tileset APIs, replaces pathname-based version resolver, and updates tests/docs to match new preference and routing behavior. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Prefs as Preferences
participant Storage as localStorage
participant Nav as Navigation
participant History as HistoryAPI
App->>Prefs: initializePreferences()
Prefs->>Storage: getItem('cbn-guide:mods')
Storage-->>Prefs: JSON or null
Prefs->>Prefs: parse & validate → mods[]
Prefs-->>App: preferences{tileset, mods}
App->>Nav: bootstrapApplication()
Nav->>Nav: resolveVersionedPath(location.pathname, builds)
Nav->>Nav: compute canonical URL (include prefs.mods if ?mods absent)
alt bare URL and prefs.mods != []
Nav->>History: replaceState(canonical URL)
History-->>Nav: URL replaced
end
Nav->>Nav: updatePageState()
Nav-->>App: route resolved
App->>Nav: changeMods(mods)
Nav->>Prefs: setMods(mods)
Prefs->>Storage: setItem('cbn-guide:mods', JSON)
Prefs->>Prefs: update preferences.mods
Nav->>History: navigate(new URL)
Possibly related issues
Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/preferences.svelte.test.ts`:
- Around line 80-81: Test titles refer to removed/renamed API symbols and
incorrect expected values, causing misleading CI output; update the test
descriptions to match the current API and assertions. Rename titles that mention
setPreferredTileset to reference setTileset (e.g., "setTileset rejects invalid
values without mutating the preference"), change any titles mentioning
setDefaultMods or clearSavedDefaultMods to the current functions (e.g.,
setDefaultMods, clearDefaultMods or whatever the code now uses), and fix titles
that say "returns null" to state "returns []" where the assertion expects an
empty array; ensure each test title matches the assertion and the actual
function name (referencing setTileset, setDefaultMods,
clearSavedDefaultMods/getSavedDefaultMods as applicable).
In `@src/preferences.svelte.ts`:
- Around line 130-134: Update the stale docstring for the _resetPreferences test
helper to reference the renamed fields `tileset` and `mods` instead of
`preferredTileset` and `defaultPreferences.preferredTileset`; the comment should
state that the function resets `tileset` and `mods` to the defaults (e.g.,
`defaultPreferences.tileset`/`defaultPreferences.mods`) and still does not touch
localStorage so the docstring accurately matches the _resetPreferences
implementation.
- Around line 115-128: Add JSDoc comments above the exported functions setMods
and clearSavedMods describing their public API, parameters, return type, side
effects (that they persist to storage via persistMods/clearStoredDefaultMods and
update the preferences.mods state), behavior when mods is empty, and how
failures are handled or surfaced (e.g., whether persistence errors are thrown or
swallowed). Ensure each JSDoc includes a short summary, `@param` for the mods
array in setMods, `@returns` void, and an `@remarks` or `@throws` clause clarifying
persistence failure semantics so callers know expected behavior.
- Line 24: preferences.mods is being assigned by reference from
defaultPreferences.mods and from caller input (e.g., in setMods), which allows
downstream in-place mutations to mutate shared state; fix by always copying the
mods array/object on assignment and when returning from setters—replace direct
assignments of defaultPreferences.mods or incoming mods with a shallow clone
(e.g., spread or Array.from) or a deep clone if nested, and ensure any functions
that set or reset mods (such as the initializer that sets mods, the setMods
implementation, and any reset-to-default path) create a new array/object instead
of reusing the original reference so defaultPreferences cannot be mutated.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 07a362cd-6789-468f-9ba0-9030e404f8a9
📒 Files selected for processing (11)
docs/adr/ADR-008_mod_preset_persistence.mddocs/adr/README.mddocs/routing.mdsrc/navigation.svelte.tssrc/navigation.test.tssrc/preferences.svelte.test.tssrc/preferences.svelte.tssrc/routing.svelte.tssrc/routing.test.tssrc/routing.url.test.tssrc/schema.test.ts
💤 Files with no reviewable changes (1)
- src/schema.test.ts
- Remove duplicate `hasVersionlessHomePath` implementation from `navigation.svelte.ts`. - Centralize logic in `routing.svelte.ts` for improved maintainability.
… API naming - Update test descriptions from `defaultMods` and `preferredTileset` to `mods` and `tileset`. - Ensure test cases align with recent property and method renaming for clarity.
…rator usage - Update assignments of `mods` to use spread operator for immutability. - Refactor functions to consistently apply array cloning for preferences.
Closed
5 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routing.svelte.ts (1)
228-234:⚠️ Potential issue | 🔴 CriticalBehavioral delta confirmed:
popstatehandler bypasses URL canonicalization.Current implementation routes
popstateevents throughupdatePageState()→parseRoute(), which directly parses the URL state without invokingresolveVersionedPath(). Canonicalization occurs only at bootstrap viabuildCanonicalBootstrapURL(). Malformed URLs in browser history will surface unchanged after back/forward navigation.Test coverage gap: Existing popstate tests validate state synchronization only. No test scenarios cover navigation through browser history containing malformed URLs—recommend adding coverage for this edge case before production deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routing.svelte.ts` around lines 228 - 234, The popstate handler currently calls updatePageState() which leads to parseRoute() without applying URL canonicalization, so modify the flow so popstateHandler (or updatePageState) first passes the current location through resolveVersionedPath() (or ensure parseRoute calls resolveVersionedPath at its entry) before parsing; update tests to include a popstate navigation scenario with a malformed/historic URL and assert that the resolved/canonical URL (as produced by buildCanonicalBootstrapURL()/resolveVersionedPath) is used for routing and state sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/preferences.svelte.test.ts`:
- Around line 110-121: The "mods" test block only clears MODS_STORAGE_KEY but
initializePreferences() uses both keys, so update the beforeEach and afterEach
in the describe("mods", ...) block to also remove TILESET_STORAGE_KEY (alongside
MODS_STORAGE_KEY); keep calls to installMockStorage() and _resetPreferences()
as-is and ensure both localStorage.removeItem?.(MODS_STORAGE_KEY) and
localStorage.removeItem?.(TILESET_STORAGE_KEY) are invoked to prevent tileset
state leaking between tests.
- Around line 44-55: The tests only clear TILESET_STORAGE_KEY but
initializePreferences() also reads MODS_STORAGE_KEY, risking cross-test
contamination; update the beforeEach and afterEach in the "preferences" describe
block (where installMockStorage() and _resetPreferences() are used) to also
remove MODS_STORAGE_KEY from localStorage (i.e., call
localStorage.removeItem?.(MODS_STORAGE_KEY)) so both TILESET_STORAGE_KEY and
MODS_STORAGE_KEY are purged before and after each test to ensure full isolation.
In `@src/preferences.svelte.ts`:
- Around line 131-139: Update the docstring for _resetPreferences to accurately
describe its behavior: state that it resets both preferences.tileset and
preferences.mods to the corresponding values from defaultPreferences (and that
it does not touch localStorage), or alternatively change the implementation to
only reset tileset if that was intended; locate the function _resetPreferences
and adjust either the comment text or the assignment statements
(preferences.tileset and preferences.mods) so docstring and implementation
match.
---
Outside diff comments:
In `@src/routing.svelte.ts`:
- Around line 228-234: The popstate handler currently calls updatePageState()
which leads to parseRoute() without applying URL canonicalization, so modify the
flow so popstateHandler (or updatePageState) first passes the current location
through resolveVersionedPath() (or ensure parseRoute calls resolveVersionedPath
at its entry) before parsing; update tests to include a popstate navigation
scenario with a malformed/historic URL and assert that the resolved/canonical
URL (as produced by buildCanonicalBootstrapURL()/resolveVersionedPath) is used
for routing and state sync.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 102046aa-0241-4e0a-b033-b9ad88a9c711
📒 Files selected for processing (4)
src/navigation.svelte.tssrc/preferences.svelte.test.tssrc/preferences.svelte.tssrc/routing.svelte.ts
…nsistency - Rename test suite from "preferences" to "tileset" for alignment with updated terminology. - Revise helper documentation to generalize preference reset behavior.
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
Implements mod preset persistence via
localStorage. When users select mods in theModSelector, they are saved as a default. Visiting the guide without amodsquery parameter automatically loads the saved preset viareplaceState, maintaining the URL as the source of truth.Why
Users often play with a consistent set of mods (e.g.
aftershock,magiclysm). Manually re-selecting them on every visit or bookmarking complex URLs is tiresome. This feature provides a "set once, forget" experience for the default view.What Changed
defaultModstoUserPreferenceswithlocalStoragesync.bootstrapApplicationwhen no mods are present in the URL. Added persistence trigger tochangeMods.preferences.svelte.tsand bootstrap injection innavigation.test.ts.What's Improved
Compromises / Trade-offs
localStorage.Behavior Changes
/stable/) now redirect to/stable/?mods=...if a preset is saved.Reviewer Notes / Potential Triggers
bootstrapApplication. It ensures that URL parameters always take precedence over the local preset.Critical Path For Manual Testing
/nightly/aftershock) and click Apply./nightly/. It should automatically append?mods=aftershock./nightly/directly. It should redirect to?mods=aftershock./nightly/?mods=(empty). It should STAY empty and NOT load the preset.Verification
src/preferences.svelte.test.tsandsrc/navigation.test.ts.pnpm checkandpnpm lintwere recommended by project rules but not verified in this PR session due to local tool issues.