Skip to content

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

Merged
trangdoan982 merged 8 commits into
roam-setting-overhaulfrom
eng-2185-move-export-settings-out-of-settings-into-the-export-panel
Sep 28, 2026
Merged

trangdoan982 merged 8 commits into
roam-setting-overhaulfrom
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

https://entire.io/gh/DiscourseGraphs/discourse-graph/trails/39

https://www.loom.com/share/0c24f4b276f049d29f5c21bbeb67083a

https://www.loom.com/share/d794a7965df54225bf5b5bb78ac1dadb

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: 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.
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.
  • 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.

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When:
    • 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.

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.

Known gap: export can read a stale setting

The pending-write flush this PR used before (await flushPendingSettingWrites() in the export path) has been removed. It and its registry are tracked in ENG-2286, which blocks this ticket.

Until ENG-2286 lands, editing an export option and pressing Export within about 350 ms reads the option at its previous value. The export settings and the Export button share this dialog, so no unmount happens on that path and the commit-on-unmount landed in ENG-2186 (#1375) does not cover it.

The underlying debounce bug is ENG-2287.

@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.

Project Deployment Actions Updated
discourse-graph Ready Ready Preview Sep 28, 2026 3:20pm UTC

Request Review

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-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-2185-move-export-settings-out-of-settings-into-the-export-panel branch from cfba308 to 478402c 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 478402c to 7eaf604 Compare September 5, 2026 22:55
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from a0293d1 to d29883b Compare September 7, 2026 22:23
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from d29883b to 156347d Compare September 8, 2026 01:02
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from f29d9c6 to 24c90a9 Compare September 17, 2026 19:59
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from a82e191 to 37df89d Compare September 17, 2026 20:18
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from 37df89d to f7c8e77 Compare September 17, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from f7c8e77 to 037e56c Compare September 18, 2026 20:40
An error occurred while trying to automatically change base from eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put to roam-setting-overhaul September 28, 2026 14:42
trangdoan982 and others added 8 commits September 28, 2026 10:46
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>
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>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ltip

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The flush and its registry move to ENG-2286, which blocks this ticket.

Known gap until then: the export settings and the Export button share this
dialog, so an option edited within ~350ms of pressing Export is read at its
previous value. No unmount happens in that path, so the commit-on-unmount in
ENG-2186 does not cover it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@trangdoan982 trangdoan982 reopened this Sep 28, 2026
@supabase

supabase Bot commented Sep 28, 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 ↗︎.

Base automatically changed from eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put to roam-setting-overhaul September 28, 2026 14:52
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from 037e56c to b370327 Compare September 28, 2026 15:06
@trangdoan982
trangdoan982 force-pushed the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch from b370327 to 82e01e3 Compare September 28, 2026 15:18
@trangdoan982
trangdoan982 merged commit f28cd07 into roam-setting-overhaul Sep 28, 2026
10 checks passed
@trangdoan982
trangdoan982 deleted the eng-2185-move-export-settings-out-of-settings-into-the-export-panel branch September 28, 2026 17:45

This branch was successfully deployed

1 active deployment
Preview — 82e01e3b Deployed Sep 28, 2026 by vercel[bot]
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