Skip to content

ENG-1574: Add dual-read console logs to setting setters#914

Open
sid597 wants to merge 7 commits intoeng-1470-test-dual-read-with-flag-on-and-fix-gapsv2from
eng-1574-add-console-logs-to-existing-function-check
Open

ENG-1574: Add dual-read console logs to setting setters#914
sid597 wants to merge 7 commits intoeng-1470-test-dual-read-with-flag-on-and-fix-gapsv2from
eng-1574-add-console-logs-to-existing-function-check

Conversation

@sid597
Copy link
Copy Markdown
Collaborator

@sid597 sid597 commented Mar 26, 2026

We now print all settings on init and then can call the window.dgDualReadLog() whenever we want to.

image
Open with Devin

Summary by CodeRabbit

  • New Features

    • Added diagnostic logging capabilities to verify settings consistency and identify potential configuration issues for improved troubleshooting.
  • Refactor

    • Improved internal code organization and modularity to enhance maintainability.

Log legacy and block prop values with match/mismatch status
when a setting is changed. Fix broken import in storedRelations.
@linear
Copy link
Copy Markdown

linear bot commented Mar 26, 2026

@supabase
Copy link
Copy Markdown

supabase bot commented Mar 26, 2026

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

devin-ai-integration[bot]

This comment was marked as resolved.

Log all legacy vs block prop settings on init. Remove setter
logging. Expose dgDualReadLog() on window for on-demand use.
devin-ai-integration[bot]

This comment was marked as resolved.

sid597 added 2 commits March 27, 2026 17:16
JSON.stringify is key-order dependent, causing false mismatches
when legacy and block props return keys in different order.
devin-ai-integration[bot]

This comment was marked as resolved.

@sid597
Copy link
Copy Markdown
Collaborator Author

sid597 commented Mar 27, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

Exports deepEqual utility function from accessors module and enhances settings initialization with optional dual-read consistency comparison between legacy and new settings stores, logging mismatches via console and window-exposed logger when the new store is enabled.

Changes

Cohort / File(s) Summary
Settings utilities refactor
apps/roam/src/components/settings/utils/accessors.ts, apps/roam/src/components/settings/utils/init.ts
Exports deepEqual function and expands initSchema to perform optional dual-read consistency checking. Adds logDualReadComparison function that reads legacy (feature flags, global, personal, per-node) and new store settings, compares them using deepEqual, aggregates mismatches, logs summaries/payloads to console, and exposes logger on window.dgDualReadLog. Wrapped in try/catch to warn on failures.

Sequence Diagram

sequenceDiagram
    participant initSchema as initSchema Function
    participant legacyReader as Legacy Settings Reader
    participant newReader as New Store Reader
    participant deepEqual as deepEqual Comparator
    participant logger as logDualReadComparison
    participant console as Console/Window

    initSchema->>initSchema: Check if new settings store enabled
    alt New store enabled
        initSchema->>legacyReader: Read legacy feature flags, global, personal, per-node settings
        legacyReader-->>initSchema: Legacy settings payload
        initSchema->>newReader: Read corresponding block-props/new-store values
        newReader-->>initSchema: New store payload
        initSchema->>logger: Call logDualReadComparison
        logger->>deepEqual: Compare legacy vs new (exclude flag)
        deepEqual-->>logger: Mismatch results
        logger->>logger: Aggregate mismatches by type
        logger->>console: Log summary and full payloads
        console-->>logger: Logged
        initSchema->>console: Expose logDualReadComparison as window.dgDualReadLog
    else New store disabled
        initSchema->>initSchema: Skip dual-read comparison
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ 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 'ENG-1574: Add dual-read console logs to setting setters' directly and specifically describes the main change in the pull request—adding dual-read logging functionality via console logs to setting operations, which aligns with the modifications to both accessors.ts and init.ts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/roam/src/components/settings/utils/init.ts (1)

344-349: Avoid duplicating the feature-flag string literal.

Line 348 hardcodes "Use new settings store", while the same key is also hardcoded in apps/roam/src/components/settings/utils/accessors.ts. Centralizing this key as a shared constant will prevent drift if the flag name changes.

♻️ Suggested refactor
+// e.g. from zodSchema or a shared settings-keys module
+import { USE_NEW_SETTINGS_STORE_FLAG } from "./zodSchema";

   const omitStoreFlag = (
     flags: Record<string, unknown>,
   ): Record<string, unknown> =>
     Object.fromEntries(
-      Object.entries(flags).filter(([k]) => k !== "Use new settings store"),
+      Object.entries(flags).filter(([k]) => k !== USE_NEW_SETTINGS_STORE_FLAG),
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/roam/src/components/settings/utils/init.ts` around lines 344 - 349, The
hardcoded feature flag string "Use new settings store" is duplicated; introduce
a single exported constant (e.g., NEW_SETTINGS_STORE_FLAG) in a shared module
and replace the literal in the omitStoreFlag function and the matching reference
in accessors.ts with that constant (update imports accordingly); ensure the
constant name is used in Object.entries(...).filter(([k]) => k !==
NEW_SETTINGS_STORE_FLAG) inside omitStoreFlag and in any accessor functions that
previously referenced the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/roam/src/components/settings/utils/init.ts`:
- Around line 344-349: The hardcoded feature flag string "Use new settings
store" is duplicated; introduce a single exported constant (e.g.,
NEW_SETTINGS_STORE_FLAG) in a shared module and replace the literal in the
omitStoreFlag function and the matching reference in accessors.ts with that
constant (update imports accordingly); ensure the constant name is used in
Object.entries(...).filter(([k]) => k !== NEW_SETTINGS_STORE_FLAG) inside
omitStoreFlag and in any accessor functions that previously referenced the
literal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f9358337-3c1f-4b25-9aeb-92b543c8f329

📥 Commits

Reviewing files that changed from the base of the PR and between 8830ade and 2397446.

📒 Files selected for processing (2)
  • apps/roam/src/components/settings/utils/accessors.ts
  • apps/roam/src/components/settings/utils/init.ts

@sid597 sid597 requested a review from mdroidian March 27, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants