Draft
Conversation
github-actions
bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements server-side JSON file-backed preferences storage and integrates URL persistence across the application. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
Contributor
![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 (1)
frontend/viewer/src/AppRoutes.svelte (1)
25-28: Avoid magic number for debounce delay.
200should be a named constant so this behavior is easier to tune and reason about.💡 Suggested tweak
+ const URL_SAVE_DEBOUNCE_MS = 200; onMount(() => { let timeout: ReturnType<typeof setTimeout>; const saveUrl = () => { clearTimeout(timeout); timeout = setTimeout(() => { const { pathname, search, hash } = document.location; void appStorage.lastUrl.set(pathname + search + hash); - }, 200); + }, URL_SAVE_DEBOUNCE_MS); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/viewer/src/AppRoutes.svelte` around lines 25 - 28, Replace the magic number 200 with a named constant (e.g., URL_SAVE_DEBOUNCE_MS) declared in module scope of AppRoutes.svelte and use that constant when creating the timeout; update the timeout logic that references the variable timeout and the call to appStorage.lastUrl.set(pathname + search + hash) to use the new constant so the debounce delay is easy to find and tune.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/FwLite/FwLiteShared/Services/JsonFilePreferencesService.cs`:
- Around line 76-90: Save() currently catches and logs all exceptions, causing
Set()/Remove() to appear successful even when disk writes fail; change Save() to
surface failures instead of swallowing them by either rethrowing the caught
exception after logging (throw;) or returning a failure result that callers must
check, and update Set() and Remove() to propagate that failure (i.e., throw on
Save failure or return/propagate a false result) so callers get an exception or
error when File.WriteAllText/File.Move fails; reference Save(), Set(), Remove(),
_logger and _filePath when making the change.
In `@frontend/viewer/src/lib/utils/storage-prop.svelte.ts`:
- Line 27: The constructor currently calls void this.load() and other spots
(lines ~48-54) fire-and-forget the async load which can leave this.loading true
and produce unhandled rejections if backend.get() rejects; update the code so
the load() promise is observed and errors are handled: either add a .catch(...)
to the void this.load() call(s) in the constructor and at the other
fire-and-forget sites to log/handle the error, and/or modify the load() method
(the function named load that calls backend.get()) to wrap its await in
try/catch and a finally that always sets this.loading = false; ensure references
to backend.get() remain and use the class method names (constructor, load) so
the loading flag cannot remain true and no unhandled rejection escapes.
---
Nitpick comments:
In `@frontend/viewer/src/AppRoutes.svelte`:
- Around line 25-28: Replace the magic number 200 with a named constant (e.g.,
URL_SAVE_DEBOUNCE_MS) declared in module scope of AppRoutes.svelte and use that
constant when creating the timeout; update the timeout logic that references the
variable timeout and the call to appStorage.lastUrl.set(pathname + search +
hash) to use the new constant so the debounce delay is easy to find and tune.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/FwLite/FwLiteMaui/App.xaml.csbackend/FwLite/FwLiteShared/Services/JsonFilePreferencesService.csbackend/FwLite/FwLiteShared/Services/PreferenceKey.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/FwLiteWeb/FwLiteWebKernel.csbackend/FwLite/FwLiteWeb/Program.csfrontend/viewer/src/AppRoutes.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/PreferenceKey.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/index.tsfrontend/viewer/src/lib/services/service-provider.tsfrontend/viewer/src/lib/utils/app-storage.svelte.tsfrontend/viewer/src/lib/utils/project-storage.svelte.tsfrontend/viewer/src/lib/utils/storage-prop.svelte.ts
Comment on lines
+76
to
+90
| private void Save() | ||
| { | ||
| try | ||
| { | ||
| var directory = Path.GetDirectoryName(_filePath); | ||
| if (directory is not null) Directory.CreateDirectory(directory); | ||
| var json = JsonSerializer.Serialize(_cache, new JsonSerializerOptions { WriteIndented = true }); | ||
| var tempPath = _filePath + ".tmp"; | ||
| File.WriteAllText(tempPath, json); | ||
| File.Move(tempPath, _filePath, overwrite: true); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| _logger.LogWarning(e, "Failed to save preferences to {FilePath}", _filePath); | ||
| } |
There was a problem hiding this comment.
Do not swallow persistence failures in Save().
Set()/Remove() can appear successful even when disk writes fail, causing silent data loss for preference updates.
Proposed fix
private void Save()
{
try
{
var directory = Path.GetDirectoryName(_filePath);
if (directory is not null) Directory.CreateDirectory(directory);
var json = JsonSerializer.Serialize(_cache, new JsonSerializerOptions { WriteIndented = true });
var tempPath = _filePath + ".tmp";
File.WriteAllText(tempPath, json);
File.Move(tempPath, _filePath, overwrite: true);
}
catch (Exception e)
{
_logger.LogWarning(e, "Failed to save preferences to {FilePath}", _filePath);
+ throw;
}
}📝 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.
Suggested change
| private void Save() | |
| { | |
| try | |
| { | |
| var directory = Path.GetDirectoryName(_filePath); | |
| if (directory is not null) Directory.CreateDirectory(directory); | |
| var json = JsonSerializer.Serialize(_cache, new JsonSerializerOptions { WriteIndented = true }); | |
| var tempPath = _filePath + ".tmp"; | |
| File.WriteAllText(tempPath, json); | |
| File.Move(tempPath, _filePath, overwrite: true); | |
| } | |
| catch (Exception e) | |
| { | |
| _logger.LogWarning(e, "Failed to save preferences to {FilePath}", _filePath); | |
| } | |
| private void Save() | |
| { | |
| try | |
| { | |
| var directory = Path.GetDirectoryName(_filePath); | |
| if (directory is not null) Directory.CreateDirectory(directory); | |
| var json = JsonSerializer.Serialize(_cache, new JsonSerializerOptions { WriteIndented = true }); | |
| var tempPath = _filePath + ".tmp"; | |
| File.WriteAllText(tempPath, json); | |
| File.Move(tempPath, _filePath, overwrite: true); | |
| } | |
| catch (Exception e) | |
| { | |
| _logger.LogWarning(e, "Failed to save preferences to {FilePath}", _filePath); | |
| throw; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/FwLite/FwLiteShared/Services/JsonFilePreferencesService.cs` around
lines 76 - 90, Save() currently catches and logs all exceptions, causing
Set()/Remove() to appear successful even when disk writes fail; change Save() to
surface failures instead of swallowing them by either rethrowing the caught
exception after logging (throw;) or returning a failure result that callers must
check, and update Set() and Remove() to propagate that failure (i.e., throw on
Save failure or return/propagate a false result) so callers get an exception or
error when File.WriteAllText/File.Move fails; reference Save(), Set(), Remove(),
_logger and _filePath when making the change.
| constructor(key: string, backend: IPreferencesService) { | ||
| this.#key = key; | ||
| this.#backend = backend; | ||
| void this.load(); |
There was a problem hiding this comment.
Handle load() failures to prevent unhandled rejections and stuck loading state.
If backend.get() rejects, the constructor’s fire-and-forget call can leave loading true forever.
Proposed fix
private async load(): Promise<void> {
- const value = await this.#backend.get(this.#key);
- if (!this.#hasBeenSet) {
- this.#value = value ?? '';
- this.#hasBeenSet = true;
- }
+ try {
+ const value = await this.#backend.get(this.#key);
+ if (!this.#hasBeenSet) {
+ this.#value = value ?? '';
+ }
+ } catch (error) {
+ console.warn(`Failed to load storage key '${this.#key}'`, error);
+ } finally {
+ if (!this.#hasBeenSet) {
+ this.#hasBeenSet = true;
+ }
+ }
}Also applies to: 48-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/viewer/src/lib/utils/storage-prop.svelte.ts` at line 27, The
constructor currently calls void this.load() and other spots (lines ~48-54)
fire-and-forget the async load which can leave this.loading true and produce
unhandled rejections if backend.get() rejects; update the code so the load()
promise is observed and errors are handled: either add a .catch(...) to the void
this.load() call(s) in the constructor and at the other fire-and-forget sites to
log/handle the error, and/or modify the load() method (the function named load
that calls backend.get()) to wrap its await in try/catch and a finally that
always sets this.loading = false; ensure references to backend.get() remain and
use the class method names (constructor, load) so the loading flag cannot remain
true and no unhandled rejection escapes.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
myieye
changed the title
myieye
changed the base branch from
develop
to
use-json-file-for-preferences-on-linux
myieye
force-pushed
the
claude/track-url-session-restore-9Wd2q
branch
from
ab65466 to
1545e20
Compare
myieye
force-pushed
the
use-json-file-for-preferences-on-linux
branch
from
8ba07d0 to
fb7c92c
Compare
myieye
force-pushed
the
claude/track-url-session-restore-9Wd2q
branch
from
1545e20 to
5f944e1
Compare
Base automatically changed from
use-json-file-for-preferences-on-linux
to
develop
March 4, 2026 13:45
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.
Resolves #2189