ENG-1454: Base getter() task#828
Merged
Merged
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
* ENG-1454: Enable dual read feature flag * add caller * resolve merge conflicts * review * example fix * review
* ENG-1455: Dual-read from old-system settings and from blockprops * remove console.logs * address review * prettier * add comment describing why we do it like this * reviewed * rebase, review * address review * explicit throw, now zodschema default reads * replace throw with warn, throw was too strong, with warn we test if legacy and block props are same
…ult reads (#813) * ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback) * review, fix
* ENG-1473: Rebase onto updated eng-1472, resolve conflicts and fix missing import * ENG-1473: Review fixes — remove dead extensionAPI params, fix type casts * ENG-1473: Fix restrict-template-expressions warnings in query utils * rename persistQueryPages → setQueryPages, setQueryPages → setInitialQueryPages * restore legacy type coercion for query-pages
* ENG-1467: Port global setting consumer reads (→ getGlobalSetting) * prettier * rebase onto updated eng-1472, resolve conflicts and fix tsc
* ENG-1479: Port suggestive mode settings to dual-read Add dual-read routing to getFeatureFlag for flags with legacy counterparts. FEATURE_FLAG_LEGACY_MAP maps "Suggestive mode enabled" and "Enable left sidebar" to their config tree reads, gated by isNewSettingsStoreEnabled(). Flags without legacy entries go straight to block props (no behavior change). Migrate value reads from getFormattedConfigTree() / extensionAPI.settings to accessors: - index.ts: getUidAndBooleanSetting → getFeatureFlag (accessor handles legacy fallback now) - AdminPanel: suggestiveModeEnabled.value → getFeatureFlag in both FeatureFlagsTab and main component. Removed unused useMemo. - SuggestiveModeSettings: includePageRelations state init → getGlobalSetting. Dropped initialValue from both GlobalFlagPanels. - hyde.ts: orphan extensionAPI.settings.get() reads → getGlobalSetting. Fixes bug where sync config toggles had no runtime effect (keys were never written by any code). Structural UIDs remain with getFormattedConfigTree(). pageGroups.groups NOT migrated — type mismatch (legacy PageGroup has UIDs, Zod PageGroup does not). Deferred to ENG-1470. * Align hyde.ts fallback defaults with Zod schema * Fix eslint naming-convention warnings in FEATURE_FLAG_LEGACY_MAP * Add TODO(ENG-1484) for suggestive mode reactivity
* Rebase ENG-1468 onto eng-1472 (fresh redo) * Remove unnecessary lazy initializer from complement useState * Break circular dep: inline DISCOURSE_CONFIG_PAGE_TITLE * Address review: case-insensitive attribute lookup, empty-string fallback, move DISCOURSE_CONFIG_PAGE_TITLE to data/constants
…er-based reactivity - Extract mergeGlobalSectionWithAccessor/mergePersonalSectionsWithAccessor to getLeftSidebarSettings.ts (used by LeftSidebarView, GlobalSettings, PersonalSettings) - Settings panels now actually use accessor values (not fire-and-forget) - Replace setLeftSidebarFlagHandler with settingsEmitter pub/sub - useConfig subscribes to emitter instead of old subscribe() - Wire up global/personal/flag handlers in pullWatchers to emit - Remove dead newValue===oldValue guard (hasPropChanged already checks) - Remove dead ?./?? in personal merge (Zod defaults guarantee values)
toggleFoldedState mutates folded.uid in-place then dual-writes to block props, triggering pull watch → emitter → buildConfig(). Without refreshConfigTree(), buildConfig() reads stale UIDs from the cached tree, overwriting the in-place mutation and orphaning Roam blocks.
sid597
force-pushed
the
migration-block-init-staging-branch
branch
from
21b6000 to
3b4c80a
Compare
…on personal panels
…d from block props
initSingleDiscourseNode was writing backedBy: "default" which caused nodes to be filtered out of the settings panel when the flag is ON. Also fixes up existing graphs that already stored the wrong value.
Previous commit accidentally included an older change that set "Query pages" default to []. Pre-migration, setQueryPages auto-seeds this path on every plugin load, so the effective user-facing default has always been ["discourse-graph/queries/*"]. Restore that value.
Add reifiedRelationTriples to PERSONAL_KEYS and map "Reified relation triples" -> "use-reified-relations" in PERSONAL_SCHEMA_PATH_TO_LEGACY_KEY. Without these entries, readAllLegacyPersonalSettings iterates over PERSONAL_KEYS values and omits this field, so the migration to block props fills in the Zod default (false) instead of the user's stored preference.
BaseTextPanel: track the inner setTimeout via debounceRef so unmount cleanup cancels it instead of letting it fire on an unmounted component. BaseNumberPanel and BaseSelectPanel: add the same 100ms ordering gap between the non-awaited syncToBlock write and refreshConfigTree that BaseFlagPanel and BaseTextPanel already have, tracked via refreshTimeoutRef for cleanup. Without the gap, refreshConfigTree can read the tree before the async Roam write settles.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Iterate top-level keys per group (Personal, Global, Feature Flags, each node) and log which specific keys mismatch, with legacy vs block values side-by-side. Previously only group-level match/mismatch was reported. - Skip template, specification, and index in node comparisons — their legacy and block-prop shapes diverge structurally (uid + empty children on templates; condition uids stripped on save) not semantically. Inline comment cites the responsible readers/writers. - Fix getLegacyQuerySettingByParentUid returning a hardcoded "node" for returnNode instead of reading scratch > return > [value] from the tree.
…writer (#975) The Datalog .q scan that resolved the top-level settings block on every write took ~318 ms per call on real graphs. Replaced with a hash-indexed roamAlphaAPI.pull by page title that walks :block/children — same pattern bulkReadSettings already uses. Behavior equivalence: the old query matched any descendant via :block/page; the new pull traverses direct :block/children only. Safe here because setBlockPropBasedSettings is only reached with keys[0] set to one of the three top-level settings keys (feature flags, global, personal user-id), all guaranteed to be direct children of the settings page. Measured on a real graph: BaseFlagPanel click handler 420 ms → 97 ms (−77%). The remaining 95 ms is refreshConfigTree > register translators, the inherent dual-read cost ENG-1616 flagged for post-ENG-1470 cleanup.
…dren (#982) Legacy tree init was calling getSubTree with parentUid, which fires createBlock without awaiting. Subsequent createBlock calls under those uids raced Roam's async write and failed with 'Parent entity doesn't exist'. Replace the three fire-and-forget lookups with tree-mode getSubTree + awaited createBlock fallback.
Comment on lines
+882
to
+884
| globalSettings: readAllLegacyGlobalSettings() as GlobalSettings, | ||
| personalSettings: readAllLegacyPersonalSettings() as PersonalSettings, | ||
| }; |
Contributor
There was a problem hiding this comment.
🟡 Legacy settings path bypasses Zod validation via unsafe as cast
When the "Use new settings store" feature flag is disabled (the default), bulkReadSettings() returns readAllLegacyGlobalSettings() as GlobalSettings and readAllLegacyPersonalSettings() as PersonalSettings without running the data through Zod validation. The new-store path at lines 889-890 correctly validates via GlobalSettingsSchema.parse() / PersonalSettingsSchema.parse(), which would catch type mismatches (e.g., NaN from parseInt for Max filename length, or unexpected shapes from Relations). The legacy path silently passes through whatever the readers produce, potentially causing runtime errors in components that expect validated types.
Example: NaN propagation from legacy export reader
getUidAndIntSetting in apps/roam/src/utils/getExportSettings.ts:49 uses parseInt(...) which can return NaN. The legacy global reader at accessors.ts:423 uses ?? DEFAULT but NaN ?? 64 evaluates to NaN (not null/undefined). The as GlobalSettings cast masks this, while GlobalSettingsSchema.parse() would coerce or reject it.
Prompt for agents
The legacy fallback path in bulkReadSettings() casts readAllLegacyGlobalSettings() and readAllLegacyPersonalSettings() to GlobalSettings / PersonalSettings without Zod validation. The new-store path at lines 889-890 correctly uses .parse(). To fix, run the legacy results through the same Zod schemas: GlobalSettingsSchema.parse(readAllLegacyGlobalSettings()) and PersonalSettingsSchema.parse(readAllLegacyPersonalSettings()). This ensures both code paths produce validated, schema-conformant data and prevents malformed legacy data from propagating as unexpected types.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
142
to
151
| window.clearTimeout(debounceRef.current); | ||
| debounceRef.current = window.setTimeout(() => { | ||
| if (errorRef.current) return; | ||
| setter(settingKeys, newValue); | ||
| syncToBlock?.(newValue); | ||
| debounceRef.current = window.setTimeout(() => { | ||
| if (errorRef.current) return; | ||
| refreshConfigTree(); | ||
| setter(settingKeys, newValue); | ||
| }, 100); | ||
| }, DEBOUNCE_MS); |
Contributor
There was a problem hiding this comment.
🟡 Nested debounce in BaseTextPanel can skip block-props setter for intermediate values
The handleChange function stores the inner setTimeout ID in the same debounceRef.current as the outer one. If the user types a value, the outer timeout fires (calling syncToBlock), and then the user types again within the 100ms inner window, clearTimeout(debounceRef.current) cancels only the inner timeout — meaning syncToBlock already wrote the intermediate value to the Roam block, but the block-props setter and refreshConfigTree never fire for that value. This creates a transient inconsistency between the Roam block tree and the block-props store until the next change settles. While not data-losing (the final value eventually syncs both), it can cause stale reads from the block-props store during the window.
Sequence that triggers the inconsistency
- User types "A" → outer timeout starts (250ms)
- Outer fires → syncToBlock("A") writes to Roam, inner timeout (100ms) starts, stored in debounceRef
- User types "AB" within 100ms → clearTimeout clears inner, new outer starts
- Result: Roam block has "A" written but setter/refreshConfigTree never ran for "A"
- Eventually "AB" settles and both stores sync
Prompt for agents
In BaseTextPanel's handleChange, the nested setTimeout pattern shares debounceRef.current for both the outer and inner timeouts, which means a new keystroke during the inner 100ms window cancels the block-props setter but not the already-executed syncToBlock. Consider using a separate ref for the inner timeout so both can be independently cancelled on cleanup and on new keystrokes. Alternatively, collapse the two timeouts into a single debounced callback that does syncToBlock, refreshConfigTree, and setter in sequence after the debounce settles.
Was this helpful? React with 👍 or 👎 to provide feedback.
* ENG-1668: Remove dead initDiscourseNodePages + skip redundant refreshConfigTree Remove initDiscourseNodePages (and helpers hasNonDefaultNodes, initSingleDiscourseNode) from initSchema — migrateDiscourseNodes already handles block prop writes, making this redundant on every load. Make the second refreshConfigTree(settings) conditional — only runs when initializeDiscourseNodes actually creates pages (first install). On existing graphs the tree is identical to what the first call already read. * Add per-step load timing logs to index.ts * Add per-step load timing logs to initObservers * Split listeners + queryBuilder timing logs * Add detailed timing logs to nodeTagPopupButtonObserver callback * Add detailed timing logs to all observer callbacks * Thread config tree through legacy getters to avoid redundant fetches * Remove timing instrumentation from index.ts and initObservers * Add total plugin load time log
* ENG-1672: Flip left sidebar merge helpers to iterate snapshot * ENG-1672: Route getExportSettings runtime reads through bulkReadSettings * ENG-1672: Restore defaultValue hydration for Template and Attributes panels PR #784 dropped the defaultValue prop when porting BlocksPanel to DualWriteBlocksPanel, breaking block-props reads when legacy is empty. Restore the hydration path and add flag-gated dev nuke command. * ENG-1672: Fix review findings — children:[] not undefined, merge attributes with tree UIDs * ENG-1672: Route createDiscourseNode template reads through getDiscourseNodeSetting * fix: render left sidebar settings from selected store * ENG-1672: Preserve legacy sidebar config when accessor snapshot is undefined * Revert "ENG-1672: Preserve legacy sidebar config when accessor snapshot is undefined" This reverts commit 3b3f197.
* ENG-1713: Read Relations panel from globalSettings, fix migrate clobbering reconcile DiscourseRelationConfigPanel.refreshRelations now derives the list from globalSettings.Relations (flag-aware via bulkReadSettings) instead of querying the legacy tree directly. This removes the panel's dependency on the cached relations parent uid, which is "" on first open of a fresh graph until the dialog is closed and reopened. initSchema now refreshes discourseConfigRef.tree between initSettingsPageBlocks and migrateGraphLevel when the cache hasn't caught up to the seeded grammar block. Without it, getLegacyGlobalSetting reads the stale cache, returns schema-default placeholder-keyed Relations (_INFO-rel, etc.), and migrateSection overwrites the real-uid keys reconcileRelationKeys just wrote — leaving block props keyed by placeholders that the legacy edit panel cannot resolve. Cold path adds ~6ms once per graph; hot path skips via .some() check. * ENG-1713: Defer back() 50ms after relation save so list reflects write setGlobalSetting fires window.roamAlphaAPI.data.block.update without awaiting the returned promise. The previous code called back() in the same tick, so handleBack's refreshRelations ran a sync pull before Roam's index reflected the write — the list rendered pre-edit data on add / edit / delete from inside RelationEditPanel. setTimeout(back, 50) gives Roam a tick to commit before the read. This is a band-aid: the proper fix is either to await the write chain (plumb async through setBlockProps -> setGlobalSetting) or refresh via a pull watcher / saved-data callback. Tracked separately for cleanup. Also drops the explanatory comment above the conditional refreshConfigTree in initSchema; the why is captured on ENG-1471 instead.
Single conflict: apps/roam/src/components/LeftSidebarView.tsx — HEAD's dual-read plumbing (initialSnapshot, sectionIndex, handleToggle) unioned with main's drag-and-drop (SortableList, dragHandle, reorderGlobalChildren). SectionChildren removed (dead after main's ChildRow/SortableList path). Known followup: DnD reorder + fold toggle can race on stale section index when toggle fires before reorder write settles. To be tracked separately.
* ENG-1716: Render template settings from block props When the new settings store flag is on, materialize the block-props template into an ephemeral Template-Block-props block as the last child of node.type, render that, and delete it on unmount. Flag-off behavior is unchanged. * ENG-1716: Dual-write template edits back to legacy Template block When the new settings store flag is on, edits in the buffer Template-Block-props block are mirrored to the legacy Template block via position-walked block.update calls. On length mismatch (only expected during testing/manual divergence) the mirror logs a warning and skips that subtree. * ENG-1716: Extend buffer->legacy mirror to handle add/delete Mirror now creates legacy children when buffer has extras and deletes legacy children when buffer has fewer, in addition to in-place block.update for matching positions. Replaces the prior length-match guard which caused dual-write to silently skip whenever the user added or removed template lines. * ENG-1716: Guard render effect against stale async continuation The ensureChildren promise resolves asynchronously, so its .then callback could run after the effect's cleanup fired - re-registering a pull watch on a stale renderUid and stomping pullWatchArgsRef. Add a cancelled flag (same pattern as the buffer-lifecycle effect) so the continuation aborts after teardown. * ENG-1716: Mirror heading and open alongside string The mirror only pushed block string to legacy, so a heading-level or collapse-state change in the buffer never propagated. Update the same block.update call to include heading and open whenever any of the three differ between buffer and legacy. Also drops uid from the buffer-creation effect's dependency array since the body no longer references it. * ENG-1716: Rename useNewStore -> isNewStore useNewStore reads like a React hook (use* convention) but is a boolean. Rename to isNewStore so the name matches the type.
…staging-branch # Conflicts: # apps/roam/src/components/canvas/uiOverrides.tsx # apps/roam/src/components/settings/utils/zodSchema.example.ts # apps/roam/src/utils/createDiscourseNode.ts # apps/roam/src/utils/initializeObserversAndListeners.ts # apps/roam/src/utils/registerCommandPaletteCommands.ts
…ns (#1034) setInitialQueryPages reads the personal query-pages setting from the snapshot and immediately calls .includes / spreads it. With the new settings store flag OFF, bulkReadSettings returns the raw legacy value cast as PersonalSettings, but the legacy extensionAPI shape is string | string[] | Record<string, string>. That throws on objects and spreads strings per-character. Route through the existing getQueryPages helper (already used by isQueryPage, listActiveQueries, resolveQueryBuilderRef), which coerces the legacy shapes to string[] before the append logic runs.
…ata loss (#1035) * ENG-1757: Read fresh relation settings in editor to prevent duplicate-edit data loss RelationEditPanel was reading the relation from the globalSettings prop, which is a snapshot taken when the Relations tab was opened. After handleDuplicate writes a new relation via setGlobalSetting, the parent's snapshot is not refreshed. Opening the editor on the duplicated row found no entry, initialized source/destination/complement to blanks, and saving overwrote the relation with empty endpoints. Switch the lookup to getGlobalSettings().Relations[uid] so the editor always reads current state. The prop is now unused in both panels and is removed along with the Settings.tsx pass-through. * ENG-1757 Refresh relation state after relation mutations
…on (#1036) * ENG-1756: Preserve existing feature flag block-prop values during legacy migration readAllLegacyFeatureFlags only sources Enable left sidebar from legacy config. The migration wrote the full FeatureFlags object (with Zod defaults filling the unsourced keys as false) via setBlockProps, which clobbered any pre-existing true values for Duplicate node alert enabled and Suggestive mode overlay enabled. Expose LEGACY_SOURCED_FEATURE_FLAG_KEYS to identify which keys the legacy reader actually sources. The migration now overlays only those keys onto the existing block-prop values, leaving everything else untouched. * ENG-1756: Remove type assertion on LEGACY_SOURCED_FEATURE_FLAG_KEYS Declare the keys as a const tuple and derive the map's key type from it, so the array's type accurately describes its contents without an assertion.
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
2 participants
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.
We want to migrate all Discourse Graphs settings in the Roam extension from block-tree storage and extensionAPI.get/set into a consistent, section-sharded, :block/props storage model. This simplifies reads/writes, enables typed validation, and makes settings more portable across apps