Skip to content

ENG-2185 Move export settings out of Settings into the Export panel - #1374

Open
trangdoan982 wants to merge 5 commits into
eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitivefrom
eng-2185-move-export-settings-out-of-settings-into-the-export-panel
Open

ENG-2185 Move export settings out of Settings into the Export panel#1374
trangdoan982 wants to merge 5 commits into
eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitivefrom
eng-2185-move-export-settings-out-of-settings-into-the-export-panel

Conversation

@trangdoan982

@trangdoan982 trangdoan982 commented Aug 31, 2026

Copy link
Copy Markdown
Member

Stacked on eng-2186 — base is that branch, not main. Merge it first.

Reviewer brief

  • Result: the 7 export options are set from a collapsed Export options disclosure at the bottom of the Export dialog's Export tab. The Export tab is gone from Settings. Storage keys, the getExportSettings read path, and export behaviour are untouched, so saved values carry over.
  • Review focus: this supersedes eng-2186's b441825e flush in BaseTextPanel. That commit held the pending write in a ref and ran it on unmount, covering navigation away. The registry here is a superset: it also covers BaseNumberPanel and BaseSelectPanel, adds the flush-before-read the Export dialog needs, and commits exactly once — the ref version could fire syncToBlock twice if unmount landed inside the inner 100ms window. eng-2186's separate DiscourseNodeColorSetting flush is untouched.
  • Review focus: apps/roam/src/utils/pendingSettingWrites.ts and the useDeferredWrite hook in BlockPropSettingPanels.tsx. BaseNumberPanel and BaseSelectPanel deferred their block-prop write by 100ms and cancelled it on unmount, so change link type → click Export could export the previous value and change → Cancel dropped the edit. Panels now register the pending commit next to the timer, unmount commits instead of cancelling, and the Export dialog flushes before it runs. Debouncing is deliberately kept — committing per keystroke would write to Roam on every event. This touches a component every settings panel uses, so the blast radius is the thing to check; it also fixes the same loss in Settings.
  • Review focus: ExportOptions was moved with git mv, so it reads as a rename. The only content changes are the import paths (it is no longer inside settings/) and the component name.
  • Risk or follow-up: the in-app Roam pass did not happen — see Verification. The code is type-checked, linted, and confirmed present in the dev bundle, but nobody has clicked the 7 controls in a real graph yet.
  • Risk or follow-up: sharing-discourse-graph.md had a screenshot of the options inside the Settings dialog, showing the vertical tab list and the pre-taxonomy tab names. It is removed rather than replaced — the prose already names where the options live, and no fresh capture of the Export dialog exists yet, so a replacement image is worth adding once someone can grab one. The frontmatter close-up further down is a crop of the control itself and stays accurate.

Verification

  • pnpm install --frozen-lockfile then pnpm ci:validate from the repo root: exit 0. check-types across 12 packages, 142 tests in 23 files passing.
  • 5 new unit tests for the pending-write registry, including self-deregistration mid-flush and a commit that registers follow-up work.
  • ESLint on every changed apps/roam file: 0 errors. Export.tsx (40) and BlockPropSettingPanels.tsx (3) report the same warning counts before and after this change (stashed and re-ran to compare), so no new ones. One exhaustive-deps warning this change introduced in Settings.tsx is fixed rather than left.
  • Dev bundle rebuilt clean; dist/extension.js contains the new disclosure and posthog event, and no longer contains the discourse-graph-export tab id.
  • Not done: the manual Roam pass — setting all 7 options, confirming they persist to the same blocks and survive a reopen, and confirming an export honours a changed option. The Chrome extension driving the dev graph timed out, so this is unverified in a live graph. Settings are read inside the export callbacks in getExportTypes.ts, not at dialog mount, so live edits should apply to the next export, but that reasoning is from the code and has not been exercised.

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When:

Three items:

  • BlockPropSettingPanels.tsx write timing, in response to a review finding. Out of the ticket's Done When, but the move is what exposes the race — inside Settings the 100ms window was hard to hit, inside the Export dialog change option → Export is the primary flow. Fixing it in the shared panel was unavoidable; discussed and agreed before implementing.

  • The Advanced > Export tab and its advancedExport id are removed from eng-2186's taxonomy, which parked them with the comment Leaves Settings entirely in ENG-2185. The saved "discourse-graph-export" alias in settingsTabs.ts now points at the default tab rather than a tab that no longer renders, which is what satisfies the ticket's "deep links still resolve" clause. (My earlier standalone KNOWN_TAB_IDS fallback is gone — eng-2186's resolveSettingsTabId does the job properly.)

  • apps/website/content/roam/guides/sharing-discourse-graph.md said the options live on the Export tab of the configuration, which this change makes wrong.

The posthog initialTabId capture now reports the resolved tab rather than the requested one, so telemetry cannot claim a tab that never opened.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context. Use $dg-delegated-full-review when no other full-review workflow is available.

@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

ENG-2185

@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 5, 2026 11:12pm UTC

Request Review

@supabase

supabase Bot commented Aug 31, 2026

Copy link
Copy Markdown

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 ↗︎.

Comment thread apps/roam/src/components/settings/Settings.tsx

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment thread apps/roam/src/components/Export.tsx
@trangdoan982
trangdoan982 changed the base branch from main to roam-setting-overhaul August 31, 2026 17:05
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from 28c63a1 to a2f5284 Compare August 31, 2026 17:19
@trangdoan982
trangdoan982 changed the base branch from roam-setting-overhaul to eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitive August 31, 2026 17:19
@graphite-app

graphite-app Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

Please split this into smaller PRs unless there is a clear reason the changes need to land together.

If keeping it as one PR, please add a brief justification covering:

  • What single problem this PR solves
  • Why the files/changes are coupled

@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from d528b99 to 9d3d1ae Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitive branch from bd1b4fe to 592a07c Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from 9d3d1ae to cfba308 Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitive branch from 592a07c to 5c92bfd Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitive branch from 5c92bfd to 92a4176 Compare September 4, 2026 22:46
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from cfba308 to 478402c Compare September 4, 2026 22:46
trangdoan982 and others added 4 commits September 5, 2026 18:53
The 7 export controls were used by exactly one surface but lived in
Settings, so changing an option meant leaving the Export flow, opening
Settings, and coming back.

Move them into a collapsed "Export options" disclosure at the bottom of
the Export dialog's Export tab, and remove the Advanced > Export tab that
ENG-2186 parked in Settings for this change to take out. The controls,
their storage keys, and the read path in getExportSettings are unchanged,
so saved values persist and export behaviour is untouched.

The advancedExport tab id goes with it, and the saved
"discourse-graph-export" deep link now resolves to the default tab
instead of a tab that no longer renders.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The image showed the export options inside the Settings dialog, with the
vertical tab list and the pre-taxonomy tab names. Both the Export tab and
that chrome are gone, so the screenshot contradicted the prose above it.

Removed rather than replaced: the surrounding text already names where the
options live, and no fresh capture of the Export dialog is available yet.
The frontmatter close-up further down is a crop of the control itself, so
it stays accurate and is kept.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eads

The number and select panels in Export options defer their block-prop write
behind a short timer, so an option edited a moment before clicking Export was
read at its previous value.

Awaited rather than fired: the commit only starts the Roam block update, so a
synchronous flush would not have been enough on its own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on the Export dialog: the options toggle sits on the left of
one row with "Exporting N results" and the discourse-context checkbox on the
right, instead of taking a row of its own.

The option panels are also seeded from a fresh settings read each time the
section opens. Collapse unmounts them while closed, so reading once at dialog
mount meant a long-lived dialog could show values that had since changed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from 478402c to 7eaf604 Compare September 5, 2026 22:55
Review feedback: the checkbox sits right-aligned on the line above; the line
below is the Export options toggle on the left and the result count on the
right.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

1 participant