[ENG-204] Move from localStorage to extensionAPI.settings

[trangdoan982]

Summary by CodeRabbit

• New Features
  • Added seamless migration of user settings from browser storage to a centralized extension settings system for improved reliability and consistency.
• Refactor
  • Updated settings and token management throughout the app to use new utility functions, ensuring smoother storage handling and better compatibility.
  • Enhanced GitHub OAuth flow with improved state persistence, error handling, and detailed logging.
• Chores
  • Introduced utility modules to centralize and standardize settings management.

[linear]

ENG-204 Move from localStorage to extensionAPI.settings

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
discourse-graph	⬜️ Skipped (Inspect)			Apr 21, 2025 9:01pm

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

The changes introduce a new settings management system that centralizes and abstracts access to persistent settings, migrating from direct localStorage usage to a unified extension API. Utility functions for getting and setting settings are added, including special handling for OAuth tokens and namespaced keys. Existing components are refactored to use these utilities instead of direct localStorage access. A migration utility is introduced and invoked at extension startup to transfer legacy settings to the new system. Additional improvements include better OAuth state persistence, enhanced logging, and error handling in the GitHub export component.

Changes

File(s)	Change Summary
apps/roam/src/utils/settings.ts	New module providing utility functions (getSetting, setSetting, getRoamJSSetting, setRoamJSSetting) for unified settings management and migration from localStorage to extension API.
apps/roam/src/utils/migrateSettings.ts	New utility function migrateSettings to migrate settings from localStorage to the extension API, handling both standard and namespaced keys, and cleaning up migrated entries.
apps/roam/src/index.ts	Imports and invokes migrateSettings at the start of extension initialization to ensure settings are migrated before other logic runs.
apps/roam/src/components/Export.tsx	Refactored to use getSetting and setSetting utilities for persistent settings instead of direct localStorage access; added debug logging for the GitHub access token.
apps/roam/src/components/ExportGithub.tsx	Refactored OAuth token and state management to use new settings utilities and persistent OAuth state; improved logging, error handling, and replaced inline click handler with a dedicated function.
apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx	Refactored to use getRoamJSSetting and setRoamJSSetting utilities for managing "roamjs:"-prefixed settings instead of direct localStorage access.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ExportGithubComponent
    participant SettingsUtils
    participant ExtensionAPI
    participant LocalStorage

    User->>ExportGithubComponent: Clicks "Authorize GitHub"
    ExportGithubComponent->>SettingsUtils: getSetting("oauth-github")
    alt Not found in ExtensionAPI
        SettingsUtils->>LocalStorage: Get "oauth-github"
        alt Found in LocalStorage
            SettingsUtils->>ExtensionAPI: setSetting("oauth-github", token)
            SettingsUtils->>LocalStorage: Remove "oauth-github"
        end
    end
    ExportGithubComponent->>User: Opens GitHub OAuth window
    ExportGithubComponent->>BackendAPI: Poll for OAuth token (retry loop)
    BackendAPI-->>ExportGithubComponent: Returns token or error
    ExportGithubComponent->>SettingsUtils: setSetting("oauth-github", token)

Loading

sequenceDiagram
    participant ExtensionStartup
    participant MigrateSettings
    participant SettingsUtils
    participant LocalStorage
    participant ExtensionAPI

    ExtensionStartup->>MigrateSettings: Call migrateSettings()
    MigrateSettings->>SettingsUtils: getSetting(key)
    alt Not found in ExtensionAPI
        SettingsUtils->>LocalStorage: Get key
        alt Found in LocalStorage
            SettingsUtils->>ExtensionAPI: setSetting(key, value)
            SettingsUtils->>LocalStorage: Remove key
        end
    end
    MigrateSettings->>ExtensionStartup: Migration complete

Loading

Poem

🐰
Settings hop from place to place,
From localStorage to a safer space.
Tokens and keys, all tucked in tight,
With helpers guiding every byte.
Migration done, the logs all cheer—
A tidy warren, crystal clear!
—A rabbit’s ode to settings, here!

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[trangdoan982]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🔭 Outside diff range comments (2)

apps/roam/src/components/Export.tsx (1)

44-54: ⚠️ Potential issue

Unused localStorageGet/Set imports will break strict TypeScript builds

localStorageGet and localStorageSet are no longer referenced after the settings‑API migration, yet they remain imported.
With noUnusedLocals (default in many TS setups) this is a compile‑time error.

-import localStorageGet from "roamjs-components/util/localStorageGet";
-import { ExportGithub } from "./ExportGithub";
-import localStorageSet from "roamjs-components/util/localStorageSet";
+import { ExportGithub } from "./ExportGithub";

(If those helpers are still needed elsewhere, keep only the relevant one.)

apps/roam/src/components/ExportGithub.tsx (1)

1-15: ⚠️ Potential issue

useMemo import is now unused after the refactor

useMemo is imported (line 8) but no longer referenced anywhere, producing a compile error.

If you revert to using useMemo for the isDev flag (recommended, see next comment) the import becomes valid again; otherwise remove it.

🧹 Nitpick comments (7)

apps/roam/src/utils/settings.ts (1)

8-52: Well-structured settings retrieval with migration logic.

The getSetting function follows a good pattern of prioritizing the extension API while maintaining backward compatibility with localStorage. The special handling for OAuth tokens is particularly important for a smooth transition.

Consider removing or conditionalizing the console.log statements for production builds to avoid leaking token type information.

-      console.log(`Found GitHub token in localStorage: ${typeof localValue}`);
+      if (process.env.NODE_ENV === "development") {
+        console.log(`Found GitHub token in localStorage: ${typeof localValue}`);
+      }

-    console.log(`Extension API GitHub token: ${typeof value}`);
+    if (process.env.NODE_ENV === "development") {
+      console.log(`Extension API GitHub token: ${typeof value}`);
+    }

apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (1)

872-882: Redundant default value handling.

There's redundancy in how default values are handled:

-                  elementsRef.current[tab] = JSON.parse(
-                    getRoamJSSetting("discourse-relation-copy", "{}") || "{}",
-                  ).map((n: { data: { id: string } }) =>
+                  elementsRef.current[tab] = JSON.parse(
+                    getRoamJSSetting("discourse-relation-copy", "{}")
+                  ).map((n: { data: { id: string } }) =>

The utility already provides a default value, so the additional || "{}" is unnecessary.

apps/roam/src/utils/migrateSettings.ts (1)

10-38: Well-structured migration utility.

The migration function effectively handles both standard and RoamJS-prefixed settings with appropriate error handling for JSON parsing. The completion log helps with debugging.

Consider adding error handling for localStorage operations which could fail in some browser environments:

export default function migrateSettings(): void {
+  try {
    // Migrate standard settings
    SETTINGS_TO_MIGRATE.forEach((key) => {
      // getSetting will automatically migrate from localStorage if needed
      getSetting(key);
    });

    // Migrate roamjs: prefixed settings
    ROAMJS_PREFIXED_SETTINGS.forEach((key) => {
      const localKey = `roamjs:${key}`;
-     const localValue = localStorage.getItem(localKey);
+     let localValue = null;
+     try {
+       localValue = localStorage.getItem(localKey);
+     } catch (e) {
+       console.warn(`Failed to access localStorage for key ${localKey}:`, e);
+     }

      if (localValue !== null) {
        try {
          // Try to parse as JSON if possible
          const parsedValue = JSON.parse(localValue);
          setRoamJSSetting(key, parsedValue);
        } catch {
          // If not JSON, store as string
          setRoamJSSetting(key, localValue);
        }

        // Clean up localStorage
+       try {
          localStorage.removeItem(localKey);
+       } catch (e) {
+         console.warn(`Failed to remove localStorage key ${localKey}:`, e);
+       }
      }
    });

    console.log("Settings migration completed");
+  } catch (e) {
+    console.error("Error during settings migration:", e);
+  }
}

apps/roam/src/components/Export.tsx (1)

172-177: Remove noisy dev‐only log or wrap it behind a debug flag

console.log("gitHubAccessToken", gitHubAccessToken); leaks potentially sensitive information and clutters the console for every render.

-  console.log("gitHubAccessToken", gitHubAccessToken);
+#   if (process.env.NODE_ENV === "development") {
+#     console.debug("gitHubAccessToken", Boolean(gitHubAccessToken));
+#   }

apps/roam/src/components/ExportGithub.tsx (3)

67-72: Avoid storing the token twice & scrub verbose logs

1. Storing in both localStorage (raw) and setSetting duplicates state and complicates invalidation.
   If backward compatibility is still required, prefer the setSetting call because it already wipes localStorage when needed.

2. console.log of token presence is excessive for production.

-    console.log("Received GitHub token:", !!token);
-    // Store in both localStorage and extension settings for maximum compatibility
-    localStorage.setItem("oauth-github", token);
-    setSetting("oauth-github", token);
+    if (process.env.NODE_ENV === "development") {
+      console.debug("Received GitHub token");
+    }
+    setSetting("oauth-github", token); // will clean localStorage internally

---

80-98: fetchAndSetInstallation reads settings on every render – memoise once

The function is recreated on every render and immediately references getSetting, doing I/O work that never changes during one user session.

Consider moving the token acquisition outside the callback or wrapping the callback in useCallback with [] and supplying getSetting("oauth-github") as the memoised value.

---

208-262: Throttle the OAuth polling loop and surface failure

The current implementation polls 30 × 1 s. If the backend is down, the user waits 30 s with no feedback except the final error.
Consider:

• Using exponential back‑off or a shorter total timeout (e.g., 10 s).
• Showing a spinner / toast while polling.
• Clearing authWindow.current when polling finishes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dfed0bd and f4ffbfd.

📒 Files selected for processing (6)

• apps/roam/src/components/Export.tsx (4 hunks)
• apps/roam/src/components/ExportGithub.tsx (5 hunks)
• apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (4 hunks)
• apps/roam/src/index.ts (2 hunks)
• apps/roam/src/utils/migrateSettings.ts (1 hunks)
• apps/roam/src/utils/settings.ts (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (5)

apps/roam/src/index.ts (1)

apps/roam/src/utils/migrateSettings.ts (1)

• migrateSettings (10-38)

apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (1)

apps/roam/src/utils/settings.ts (2)

• getRoamJSSetting (70-85)
• setRoamJSSetting (90-96)

apps/roam/src/components/Export.tsx (1)

apps/roam/src/utils/settings.ts (2)

• getSetting (8-52)
• setSetting (57-65)

apps/roam/src/components/ExportGithub.tsx (1)

apps/roam/src/utils/settings.ts (2)

• setSetting (57-65)
• getSetting (8-52)

apps/roam/src/utils/migrateSettings.ts (1)

apps/roam/src/utils/settings.ts (2)

• getSetting (8-52)
• setRoamJSSetting (90-96)

🔇 Additional comments (9)

apps/roam/src/utils/settings.ts (3)

1-4: Good use of existing roamjs-components utilities.

The imports are appropriate, using existing utilities for extension API interaction and localStorage operations which maintains consistency.

---

57-65: Clean implementation of settings storage.

The setSetting function appropriately stores values in the extension API and cleans up localStorage. This approach ensures proper migration and prevents duplicate storage.

---

70-85:

✅ Verification successful

Good special handling for RoamJS prefixed settings.

The function correctly handles special RoamJS prefixed keys, including JSON parsing with appropriate error handling.

Note that there's a potential inconsistency in the key handling flow:

• getRoamJSSetting checks localStorage with the prefixed key first
• Then falls back to getSetting with the unprefixed key
• This could lead to unexpected behavior if keys are accessed inconsistently

Can you verify that all code accessing these settings consistently uses these utility functions rather than direct localStorage access?

---

🏁 Script executed:

#!/bin/bash
# Search for direct localStorage access to RoamJS prefixed keys
rg 'localStorage\.(get|set)Item\("roamjs:'

Length of output: 44

---

🏁 Script executed:

#!/bin/bash
set -e

# 1. Search for any direct localStorage getItem/setItem with "roamjs:" prefix (single or double quotes)
rg -n "localStorage\.(getItem|setItem)\(['\"]roamjs:" -A2 -B2

# 2. List all occurrences of "roamjs:" for broader context
rg -n "roamjs:" -A2 -B2

Length of output: 74

---

All RoamJS settings use the helper functions
A search across the codebase shows no direct localStorage.getItem/setItem calls for keys prefixed with roamjs:. This confirms that getRoamJSSetting (and its counterpart) are used consistently.

apps/roam/src/index.ts (2)

25-25: Good addition of migration utility import.

Importing the migration utility is a clean approach to handle the localStorage to extension API transition.

---

54-56: Well-placed migration call at startup.

Placing the migration call at the very beginning of the extension initialization ensures all settings are available in the new format before any components try to access them. This is the ideal position for this operation.

apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (2)

52-52: Good switch to centralized settings utilities.

Importing the settings utilities is appropriate and aligns with the broader refactoring effort.

---

913-916: Good implementation of settings storage.

Using setRoamJSSetting to store the relation copy data is appropriate and matches the overall pattern.

apps/roam/src/utils/migrateSettings.ts (1)

1-5: Well-organized settings migration constants.

The clear separation of standard settings and RoamJS-prefixed settings makes the migration process more maintainable. If new settings need to be added, developers can simply extend these arrays.

apps/roam/src/components/Export.tsx (1)

203-206: Clear token consistently in both stores

setSetting("oauth-github", "") removes the token from extension settings but leaves any stray value in localStorage, potentially causing getSetting to re‑migrate the empty string back.

-        setSetting("oauth-github", "");
+        setSetting("oauth-github", "");
+        localStorage.removeItem("oauth-github"); // ensure full cleanup

[mdroidian]

Nice, just a couple of thoughts

[mdroidian]

Quick tip: remember to delete the branch on GitHub when you are done. If we don't they the branch menus just get noisy.