ENG-993 Move sync settings to tabs #514
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe SuggestiveModeSettings component was refactored from a single-column layout to a tabbed UI with "Page Groups" and "Sync Config" tabs. New internal state manages tab selection and page relation settings. The embedding generation action was moved to the Sync Config tab with error handling, and the Include Parent And Child Blocks behavior was made conditional on page relations settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SuggestiveModeSettings
participant PageGroupsPanel
participant SyncConfigPanel
User->>SuggestiveModeSettings: Select Tab (Page Groups/Sync Config)
SuggestiveModeSettings->>SuggestiveModeSettings: Update selectedTabId state
alt Page Groups Tab
SuggestiveModeSettings->>PageGroupsPanel: Render with pageGroupsUid key
Note over PageGroupsPanel: Keyed refresh on UID change
else Sync Config Tab
SuggestiveModeSettings->>SyncConfigPanel: Render sync controls
Note over SyncConfigPanel: FlagPanel for page relations<br/>FlagPanel for parent/child blocks
User->>SyncConfigPanel: Toggle Include Page Relations
SyncConfigPanel->>SuggestiveModeSettings: Update includePageRelations
SuggestiveModeSettings->>SyncConfigPanel: Disable Parent/Child Blocks
User->>SyncConfigPanel: Trigger Generate & Upload
rect rgb(200, 240, 200)
SyncConfigPanel->>SyncConfigPanel: try - Execute embedding generation
end
rect rgb(240, 200, 200)
SyncConfigPanel->>SuggestiveModeSettings: catch - Show failure toast
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The change involves reorganizing a single component's UI structure and state management across multiple concerns (tab navigation, state coordination, conditional rendering, error handling). While contained to one file, the interaction between new state properties, conditional logic, and component lifecycle requires careful review to verify correct state synchronization and no unintended side effects. Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (4)
81-97: Effective vs. persisted setting for Parent/Child BlocksYou force value to true and disable the control when includePageRelations is true. If downstream logic reads only the persisted includeParentAndChildren value, behavior may diverge from UI. Either:
- compute an effectiveIncludeParentAndChildren = includePageRelations || persisted, and ensure consumers use it; or
- cascade-write includeParentAndChildren=true when includePageRelations turns on, and restore prior value on turn-off.
100-128: Guard against double‑clicks and show loading statePrevent concurrent submissions, disable the button while running, and surface errors consistently.
@@ - const [selectedTabId, setSelectedTabId] = useState<TabId>("page-groups"); + const [selectedTabId, setSelectedTabId] = useState<TabId>("page-groups"); + const [isEmbedding, setIsEmbedding] = useState(false); @@ - <Button + <Button icon="cloud-upload" text={"Generate & Upload All Node Embeddings"} - onClick={() => - void (async () => { + disabled={isEmbedding} + onClick={() => + void (async () => { + if (isEmbedding) return; + setIsEmbedding(true); renderToast({ @@ - try { + try { await createOrUpdateDiscourseEmbedding(); } catch (e) { console.error("Failed to generate embeddings", e); renderToast({ @@ - }); + }); + } finally { + setIsEmbedding(false); } })() } intent={Intent.PRIMARY} className={"mt-2"} />
54-57: Do you need key={pageGroupsUid}?Keying PageGroupsPanel to a constant UID forces a remount even when not necessary and can drop local state. If remounting isn’t required for correctness, remove the key to preserve state.
25-34: Add error handling to create the “Suggestive Mode” containerIf createBlock fails, the UI silently continues without a parent UID. Add try/catch and a toast to aid diagnostics.
useEffect(() => { if (suggestiveModeUid) return; void (async () => { - const smUid = await createBlock({ - parentUid: getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE), - node: { text: "Suggestive Mode" }, - }); - setSuggestiveModeUid(smUid); + try { + const smUid = await createBlock({ + parentUid: getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE), + node: { text: "Suggestive Mode" }, + }); + setSuggestiveModeUid(smUid); + } catch (e) { + console.error("Failed to create Suggestive Mode container", e); + renderToast({ + id: "discourse-suggestive-mode-error", + content: "Could not initialize Suggestive Mode settings block.", + intent: Intent.DANGER, + timeout: 5000, + }); + } })(); }, [suggestiveModeUid]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/settings/SuggestiveModeSettings.tsx(3 hunks)
🔇 Additional comments (1)
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (1)
67-79: Verify FlagPanel onChange prop placement with roamjs-components maintainer or package docsThe concern in the review comment is valid but unresolved: both FlagPanel usages at lines 67 and 81 pass
options={{ onChange: ... }}, but the actual FlagPanel API from roamjs-components cannot be verified through available search results. If the package expectsonChangeat the top level rather than nested inoptions, the state settersetIncludePageRelationswill never be called, breaking the dependent UI logic at line 81 (conditional description and value).Check the roamjs-components package (GitHub: dvargas92495/roamjs-components) or npm documentation to confirm whether FlagPanel expects:
onChangeas a direct prop, oroptions={{ onChange: ... }}as a nested prop
sid597
force-pushed
the
eng-993-move-sync-settings-to-tabs
branch
from
a4059bc to
c6463c8
Compare
Summary by CodeRabbit
New Features
Improvements