ENG-2185 Move export settings out of Settings into the Export panel - #1374
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
28c63a1 to
a2f5284
Compare
PR size/scope checkThis PR is over our review-size guideline.
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:
|
d528b99 to
9d3d1ae
Compare
9d3d1ae to
cfba308
Compare
cfba308 to
478402c
Compare
478402c to
7eaf604
Compare
a0293d1 to
d29883b
Compare
d29883b to
156347d
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b39f50174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| // Awaited, not just fired: a commit only starts the Roam update, and an option edited | ||
| // a moment ago would otherwise still read as its previous value here. | ||
| await flushPendingSettingWrites(); |
There was a problem hiding this comment.
When a user toggles one of the four GlobalFlagPanel options (or adds/removes frontmatter) and immediately clicks Export, this flush does not cover that write. BaseFlagPanel.handleChange awaits direct roamAlphaAPI calls and only afterward calls setGlobalSetting, while BaseMultiTextPanel also performs untracked legacy creates/deletes; neither path registers those operations with pendingSettingWrites. On a slow write, the Markdown/PDF callback's getExportSettings() can therefore read the previous value and produce an export with references, embeds, or frontmatter contrary to the selection. Register and track those writes so this flush waits for every displayed option.
Useful? React with 👍 / 👎.
| // Collapse unmounts the panels while closed, so each open seeds them from current values. | ||
| const exportGlobalSettings = useMemo( | ||
| () => bulkReadSettings().globalSettings, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Could we read the current settings explicitly when opening the options and store that snapshot in state? That would make the intent clearer and avoid the extra useMemo dependency and lint suppression.
| <Button | ||
| minimal={true} | ||
| small={true} | ||
| icon={exportOptionsOpen ? "chevron-down" : "chevron-right"} |
There was a problem hiding this comment.
We could use rightIcon to fix the alignment issue
| We have a range of options for customizing the markdown export. These can be found on the `Export` tab of the discourse graph configuration. | ||
|
|
||
|  | ||
| We have a range of options for customizing the markdown export. These can be found under `Export options` on the `Export` tab of the export dialog, alongside the export you are about to run. |
There was a problem hiding this comment.
Maybe change one or two of these "Export"s, otherwise it reads like "export ... export ... export ... export".
"Change export dialog to "share dialog" and remove ", alongside the export you are about to run"
5b39f50 to
f29d9c6
Compare
f29d9c6 to
24c90a9
Compare
a82e191 to
37df89d
Compare
37df89d to
f7c8e77
Compare
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>
f7c8e77 to
037e56c
Compare
2 participants
https://www.loom.com/share/0c24f4b276f049d29f5c21bbeb67083a
https://www.loom.com/share/d794a7965df54225bf5b5bb78ac1dadb
Reviewer brief
Export optionsdisclosure at the bottom of the Export dialog'sExporttab. The Export tab is gone from Settings. Storage keys, thegetExportSettingsread path, and export behaviour are untouched, so saved values carry over.ExportOptionswas moved withgit mv, so it reads as a rename. The only content changes are the import paths (it is no longer insidesettings/) and the component name.Verification
pnpm install --frozen-lockfilethenpnpm ci:validatefrom the repo root: exit 0.check-typesacross 12 packages, 142 tests in 23 files passing.apps/roamfile: 0 errors.Export.tsx(40) andBlockPropSettingPanels.tsx(3) report the same warning counts before and after this change (stashed and re-ran to compare), so no new ones. Oneexhaustive-depswarning this change introduced inSettings.tsxis fixed rather than left.dist/extension.jscontains the new disclosure and posthog event, and no longer contains thediscourse-graph-exporttab id.Scope check
$scope-checkagainst the ENG ticket and final diff.Done When:apps/website/content/roam/guides/sharing-discourse-graph.mdsaid the options live on theExporttab of the configuration, which this change makes wrong.Local delegated full review
$dg-delegated-full-reviewwhen 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.