refactor: gets rid of semver dependency in localStorage migrations #2419
Conversation
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| "apps/api/node_modules/@opentelemetry/core": { | ||
| "version": "2.0.0", | ||
| "resolved": "https://registry.npmjs.org/@opentelemetry/core/-/core-2.0.0.tgz", |
There was a problem hiding this comment.
🔄 Carefully review the package-lock.json diff
Resolve the comment if everything is ok
+ node_modules/git-semver-tags/node_modules/conventional-commits-filter 5.0.0
+ node_modules/git-semver-tags/node_modules/conventional-commits-parser 6.2.1 
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2419 +/- ##
==========================================
- Coverage 51.29% 51.27% -0.02%
==========================================
Files 1069 1069
Lines 29442 29451 +9
Branches 6508 6504 -4
==========================================
Hits 15101 15101
+ Misses 14105 13944 -161
- Partials 236 406 +170
🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
apps/deploy-web/src/utils/localStorage.ts (2)
16-16: Improve suffix stripping regex to handle all semantic versioning formats.The current regex
/-[\w]+$/only strips simple suffixes like-betabut doesn't fully handle semantic versioning formats. For example:
1.2.0-alpha.1→ leaves.1(doesn't match dots)1.2.0+build.123→ doesn't strip build metadata (no+handling)🔎 Proposed fix for comprehensive suffix stripping
- const currentVersion = APP_VERSION.replace(/-[\w]+$/, ""); + const currentVersion = APP_VERSION.replace(/[-+].*$/, "");This strips everything from the first hyphen or plus sign to the end, handling both pre-release identifiers (after
-) and build metadata (after+) according to semantic versioning.
52-65: Consider documenting the deviation from semantic versioning rules.The custom
compareVersionsimplementation only compares numeric version parts (major.minor.patch) and doesn't handle semantic versioning pre-release identifiers or build metadata. This differs from the removedsemverlibrary behavior where, for example,1.0.0-alpha < 1.0.0according to semver rules.Since the PR explicitly removes the semver dependency to reduce bundle size, this trade-off appears intentional. However, if the project might use pre-release versions in the future, consider adding a comment documenting this limitation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/deploy-web/package.jsonapps/deploy-web/src/utils/localStorage.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/utils/localStorage.ts
🧬 Code graph analysis (1)
apps/deploy-web/src/utils/localStorage.ts (1)
packages/releaser/.release-it.js (1)
version(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
stalniy
force-pushed
the
refactor/localStorage-migrations
branch
from
0ddade4 to
d2f3c29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/deploy-web/src/utils/localStorage.ts (2)
20-31: Consider sorting migration versions for future-proofing.The current code relies on object key insertion order to determine migration execution order. If a future developer adds a migration out of order (e.g., adds
"0.15.0"after"3.11.1"), migrations would execute in the wrong sequence.🔎 Proposed fix to sort migrations by version
if (hasPreviousVersion) { - Object.keys(migrations).forEach(migrationVersion => { + Object.keys(migrations) + .sort(compareVersions) + .forEach(migrationVersion => { if (compareVersions(migrationVersion, version) > 0) {
52-65: Consider adding defensive NaN validation.While the current callers pass stripped version strings, this function has no input validation. If
Number()producesNaN(e.g., from malformed input), comparisons silently return incorrect results rather than failing explicitly.Since the inputs are now properly sanitized at lines 16-17, the immediate risk is low. However, defensive validation would make the function more robust for potential future use.
🔎 Optional defensive validation
function compareVersions(a: string, b: string): number { const partsA = a.split(".").map(Number); const partsB = b.split(".").map(Number); + + if (partsA.some(isNaN) || partsB.some(isNaN)) { + throw new Error(`Invalid version format: "${a}" or "${b}"`); + } + const maxLength = Math.max(partsA.length, partsB.length);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/deploy-web/package.jsonapps/deploy-web/src/utils/localStorage.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/utils/localStorage.ts
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/deploy-web/src/utils/localStorage.ts
🧬 Code graph analysis (1)
apps/deploy-web/src/utils/localStorage.ts (1)
packages/releaser/.release-it.js (1)
version(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate (packages) / validate-unsafe
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/deploy-web/src/utils/localStorage.ts (2)
16-18: Good fix for the version consistency issue.Both
currentVersionandversionnow have suffixes stripped at the point of initialization, which addresses the previous concern aboutgetVersion()returning rawAPP_VERSION. This ensurescompareVersionsreceives clean numeric version strings.
37-50: Approach is valid - suffix stripping at caller site.While line 49 still returns raw
APP_VERSION, the caller (line 17) now strips suffixes. This approach works correctly, though moving the stripping intogetVersion()would encapsulate the normalization logic. Current implementation is acceptable.
stalniy
force-pushed
the
refactor/localStorage-migrations
branch
from
d2f3c29 to
9ea6884
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/deploy-web/src/utils/localStorage.ts (3)
16-17: Use a more robust regex for stripping version suffixes.The current regex
/-[\w]+$/only strips a dash followed by word characters at the end, which doesn't fully handle semantic versioning formats:
1.0.0+build→ not stripped (no dash)1.0.0-alpha.1→ becomes1.0.0.1(leaves.1)1.0.0-beta+build→ becomes1.0.0+build(leaves+build)Semver defines prerelease and build metadata as everything after the first
-or+. A more complete regex would be/[-+].*$/.Additionally, the stripping is duplicated on both lines. Consider moving this logic into
getVersion()(line 49) to strip once at the source rather than at every call site.🔎 Proposed fix
Update the regex pattern:
- const currentVersion = APP_VERSION.replace(/-[\w]+$/, ""); - const version = getVersion().replace(/-[\w]+$/, ""); + const currentVersion = APP_VERSION.replace(/[-+].*$/, ""); + const version = getVersion().replace(/[-+].*$/, "");Or better, eliminate duplication by moving the strip into
getVersion()— see comment on line 49.
49-49: Consider stripping suffixes withingetVersion()for better encapsulation.While line 17 now strips the result after calling
getVersion(), it would be cleaner to havegetVersion()itself return a normalized version without suffixes. This:
- Eliminates duplication (currently stripped on lines 16 and 17)
- Encapsulates the normalization logic in one place
- Reduces coupling between
migrateLocalStorageand version format details🔎 Proposed refactor
function getVersion(): string { const latestUpdatedVersion = localStorage.getItem("latestUpdatedVersion"); if (latestUpdatedVersion) { return latestUpdatedVersion; } const isOldVersionUpgrade = Object.keys(localStorage).some(key => key.endsWith(".data") || key.endsWith(".wallet")); if (isOldVersionUpgrade) { return "1.0.0"; } - return APP_VERSION; + return APP_VERSION.replace(/[-+].*$/, ""); }Then update lines 16-17 to remove the duplicate stripping:
- const currentVersion = APP_VERSION.replace(/[-+].*$/, ""); - const version = getVersion().replace(/[-+].*$/, ""); + const currentVersion = APP_VERSION.replace(/[-+].*$/, ""); + const version = getVersion();
52-65: Add input validation to prevent NaN in version comparisons.While the stripping on lines 16-17 should prevent non-numeric parts from reaching
compareVersions, defensive validation would make the function more robust:
- If the stripping regex is incomplete (see comment on lines 16-17), parts like
"0-rc"could produceNaNNaNcomparisons silently return incorrect results rather than failing explicitly- Explicit validation provides clearer error messages when invalid versions are passed
🔎 Proposed validation
function compareVersions(a: string, b: string): number { const partsA = a.split(".").map(Number); const partsB = b.split(".").map(Number); + + if (partsA.some(isNaN) || partsB.some(isNaN)) { + throw new Error(`Invalid version format: "${a}" or "${b}" contains non-numeric parts`); + } + const maxLength = Math.max(partsA.length, partsB.length); for (let i = 0; i < maxLength; i++) { const partA = partsA[i] ?? 0; const partB = partsB[i] ?? 0; if (partA > partB) return 1; if (partA < partB) return -1; } return 0; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/deploy-web/package.jsonapps/deploy-web/src/utils/localStorage.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/utils/localStorage.ts
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/deploy-web/src/utils/localStorage.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: validate (apps/indexer) / validate-unsafe
- GitHub Check: validate (packages) / validate-unsafe
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- GitHub Check: validate (apps/log-collector) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
stalniy
force-pushed
the
refactor/localStorage-migrations
branch
from
9ea6884 to
0a5dc61
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/deploy-web/src/utils/localStorage.ts (2)
21-31: Consider adding an upper bound check to prevent running future migrations.The condition only verifies
migrationVersion > version(greater than the stored version). If a developer adds a migration for a future release while working on the current version, that migration would run prematurely.🔎 Proposed fix
Object.keys(migrations).forEach(migrationVersion => { - if (compareVersions(migrationVersion, version) > 0) { + if (compareVersions(migrationVersion, version) > 0 && compareVersions(migrationVersion, currentVersion) <= 0) { try { console.log(`Applying version ${migrationVersion}`); // Execute local storage migration migrations[migrationVersion](); } catch (error) { console.log(error); } } });
52-65: Implementation is correct; consider optional defensive validation.The previous NaN concern is mitigated since line 17 strips suffixes before calling this function, and migration keys are hardcoded clean versions. For additional robustness against future misuse, you could optionally add input validation, but it's not critical given the current controlled usage.
🔎 Optional defensive validation
function compareVersions(a: string, b: string): number { const partsA = a.split(".").map(Number); const partsB = b.split(".").map(Number); + + if (partsA.some(isNaN) || partsB.some(isNaN)) { + throw new Error(`Invalid version format: "${a}" or "${b}"`); + } + const maxLength = Math.max(partsA.length, partsB.length);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/deploy-web/package.jsonapps/deploy-web/src/utils/localStorage.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/utils/localStorage.ts
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Applied to files:
apps/deploy-web/src/utils/localStorage.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: validate (apps/notifications) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: validate (apps/provider-proxy) / should-validate-unsafe / should-validate
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/deploy-web/src/utils/localStorage.ts (2)
5-10: LGTM!The migrations record structure is clean and provides a clear pattern for defining version-specific localStorage cleanup tasks.
37-50: LGTM!The previous concern about returning an unstripped
APP_VERSIONon line 49 is now mitigated by the suffix stripping on line 17 at the call site.
Why
To reduce bundle size
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.