refactor(settings): overhaul settings UX around the user's mental model - #26
Open
ClaudiaFang wants to merge 9 commits into
Open
ClaudiaFang wants to merge 9 commits into
ClaudiaFang wants to merge 9 commits into
Conversation
Replace the monolithic src/settings.ts with src/settings/{types,defaults,
migrate,SettingsTab,sections/*,components/*}, organized as Storage → Links
→ Upload behavior → Image optimization → Watermark → Advanced instead of
the old Connection/Upload/Image processing/Watermark/Debug grouping.
- Storage: destination choice (S3-compatible vs local vault folder) with
provider presets (Cloudflare R2, AWS S3, MinIO, Backblaze B2, other) that
prefill sensible defaults without discarding manually entered values.
Test connection moved after the required fields.
- Links: separated from Storage — public URL mode (auto vs custom domain/
CDN) and query-string options, which used to live under "Connection".
- Upload behavior: positive trigger toggles (paste/drag/auto-upload on
create) instead of a negative "disable" toggle, file types and
exclusions split out, and a pointer to frontmatter overrides.
- Backward compatibility: new storageDestination/storageProvider/
publicUrlMode fields are optional and derived from existing legacy
fields (localUpload, useCustomImageUrl, useCustomEndpoint/customEndpoint)
via migrateSettings() only when absent, so existing installs keep
working without re-entering credentials. The legacy
disableAutoUploadOnCreate field stays persisted as-is; only the UI
mapping (isAutoUploadOnCreateEnabled/setAutoUploadOnCreateEnabled) is
positive.
- Shared logic: extracted resolvePublicBaseUrl/appendQueryString/
resolvePublicUrl/buildObjectKey into uploader.ts as the single source of
truth for path/URL resolution, reused by main.ts (createS3Client),
pasteHandler.ts (real upload), and the new settings outcome preview —
no duplicate logic between settings UI and the upload implementation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
- styles.css: rewrite around the new section/advanced-disclosure/status- row/outcome-preview DOM structure (still under the r2-* namespace), remove now-unused .r2-btn-success/.r2-btn-error (state is shown via text now, not button color), reuse Obsidian theme variables throughout, add narrow-pane/mobile handling for long values (endpoint/bucket/URL). - README.md: restructure the Configuration section around the six Storage/Links/Upload behavior/Image optimization/Watermark/Advanced questions, document the auto-upload-on-create side effect (replaces the local attachment with the remote link) and the WebP→compression→ watermark processing order, and update the Cloudflare R2 quick setup to the new Storage/Links fields. Left the "Watermark Bucket Uploader" product name as-is — renaming it is tracked separately and out of scope here. No README.zh-TW.md exists in this branch to update. - tests: settingsMigrate.test.ts (backward-compat derivation, idempotency, provider-defaults non-destructive behavior, positive/negative toggle mapping), urlResolution.test.ts (resolvePublicBaseUrl for AWS-style/ custom-endpoint/path-style/custom-CDN, query string handling, buildObjectKey), outcomePreview.test.ts (the settings preview uses the exact same buildObjectKey/resolvePublicUrl functions as the real upload path, for both S3 and local destinations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| export function deriveStorageProvider(data: Partial<R2UploaderSettings>): StorageProvider { | ||
| const endpoint = (data.customEndpoint ?? "").toLowerCase(); | ||
| if (data.useCustomEndpoint && endpoint.includes("r2.cloudflarestorage.com")) return "cloudflare-r2"; |
| export function deriveStorageProvider(data: Partial<R2UploaderSettings>): StorageProvider { | ||
| const endpoint = (data.customEndpoint ?? "").toLowerCase(); | ||
| if (data.useCustomEndpoint && endpoint.includes("r2.cloudflarestorage.com")) return "cloudflare-r2"; | ||
| if (data.useCustomEndpoint && endpoint.includes("backblazeb2.com")) return "backblaze-b2"; |
…point field - applyProviderDefaults: only ever turn useCustomEndpoint ON when the provider requires it and the user hasn't touched it yet. Previously it keyed off whether customEndpoint had text, so a user who manually toggled "Custom endpoint" on but hadn't typed a URL yet would have that toggle silently flipped back off when switching to a provider that doesn't require a custom endpoint (e.g. AWS S3) — discarding a manual choice, contrary to the "never discard manually configured values when switching providers" requirement. Added tests for both the toggle-preserved and still-defaults-on-for-a-fresh-field cases. - DEFAULT_SETTINGS.storageProvider: aligned to "aws-s3" to match what migrateSettings()/deriveStorageProvider() actually produce for a fresh install (previously "other" — no runtime effect since migrateSettings always runs on load, but a latent inconsistency for any future code path reading DEFAULT_SETTINGS directly). - StorageSection.ts Endpoint URL field: guard the https:// prefix and trailing-slash normalization so clearing the field leaves it empty instead of resetting to "https://" — matching the guard already used by LinksSection.ts's customImageUrl field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
…URL contradictions Addresses PR #26 review regressions: - SettingsTab is now a thin shell (SettingsNavigation + active section only) instead of six <details> accordions — restores the planned Storage/Links/ Upload behavior/Image optimization/Watermark/Advanced tab structure, with only the active page mounted at a time. - Desktop uses a left-nav + full-width content grid; the 720px max-width cap is removed. Narrow panes switch to horizontally-scrollable top tabs (no hamburger menu), via CSS only — same nav component both ways. - applyProviderDefaults() now sets region/useCustomEndpoint/forcePathStyle deterministically per provider (and clears a stale endpoint that belonged to a different provider) instead of only ever adding defaults — this is what let the UI show "AWS S3" while runtime still talked to an R2 host. - resolvePublicBaseUrl() no longer derives the public link from the S3 API endpoint; only AWS S3's virtual-hosted addressing can be auto-derived, every other provider requires an explicit custom public URL. uploadFile() now calls resolvePublicUrl() directly instead of a stale cached `imageUrlPath`, and createS3Client() no longer writes that field. - Status row distinguishes not-configured / configured-untested / connected / connection-failed, with provider-aware completeness checks, instead of showing "✓ Ready" from four non-empty fields alone. - Links' outcome preview gets a refresh() handle so public-URL-mode/base-URL /query-string changes update it immediately without redrawing the tab. Tests: rewrote the assertions that encoded the old (broken) additive provider-default and endpoint-as-public-URL behavior, and added provider transition tests (fresh→AWS, fresh→R2, AWS→R2, R2→AWS, R2→MinIO, MinIO→AWS) plus provider-aware configuration-validation tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLev3uMKqTg7PsAgGfaagM
| function endpointMatchesProvider(endpoint: string, provider: StorageProvider): boolean { | ||
| const e = (endpoint ?? "").toLowerCase(); | ||
| if (!e) return false; | ||
| if (provider === "cloudflare-r2") return e.includes("r2.cloudflarestorage.com"); |
| const e = (endpoint ?? "").toLowerCase(); | ||
| if (!e) return false; | ||
| if (provider === "cloudflare-r2") return e.includes("r2.cloudflarestorage.com"); | ||
| if (provider === "backblaze-b2") return e.includes("backblazeb2.com"); |
…rmed persisted data Root cause of the "settings page renders as just the heading" regression: migrateSettings() trusted a persisted storageProvider/storageDestination/ publicUrlMode whenever it was merely non-undefined, even if it wasn't a currently-valid value (e.g. written by an older/different build of this schema). getProviderPreset()/providerCanAutoPublicUrl() then indexed PROVIDER_PRESETS with that value and threw on an unrecognized key, and because SettingsTab.display() rendered the status row before the nav and active config page, that throw aborted the whole settings pane right after the "Paste to S3" heading. - migrateSettings() now validates storageProvider/storageDestination/ publicUrlMode against their current allowed values and re-derives from legacy fields when invalid, same as when absent. Valid persisted values are kept as-is; credentials/bucket/folder/endpoints/URLs are untouched. - getProviderPreset()/providerCanAutoPublicUrl() fall back to the "other" preset instead of throwing on an unrecognized provider key. - SettingsTab.display() now renders nav + the active config page before the status row, and refreshStatus() is wrapped so a status-row failure shows a minimal inline warning instead of blocking the rest of the page. - Added regression tests (raw/malformed persisted data, not just valid R2UploaderSettings) and a settings-render smoke test that drives R2UploaderSettingTab.display() through a fake DOM/Setting mock to assert the config UI still renders when status-row inputs are broken. Verified these tests fail with the pre-fix code and pass with it (git stash of the two source files against the new tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
Previously the status row only ever showed "connected" after the user manually clicked "Test connection" — reopening settings in a new Obsidian session always started back at "configured, connection not tested" even if the last session's connection was fine. onload() now fires a one-time, silent connection check (S3 destination + fully configured only) right after createS3Client(), storing the result in the existing (non-persisted) lastConnectionResult field. It's fire-and-forget — never blocks onload, no Notice — so the settings status row reflects real connectivity the first time it's opened in a session, without requiring a manual click. Manual "Test connection" and the existing "needs retest after credential changes" behavior (createS3Client resets lastConnectionResult) are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
…tive note
ignorePattern previously only checked the currently active Markdown note
via shouldIgnoreCurrentFile(), which was wrong for auto-upload-on-create:
it checked whichever note happened to be open instead of the newly
created attachment's path, so a pattern like "Private/**" would not
reliably skip a matching created file.
Add a single pure helper, matchesIgnorePattern(pattern, {notePath,
filePath}), used by both pasteHandler and main's auto-upload-on-create
path, replacing the duplicated minimatch call and the removed
shouldIgnore()/shouldIgnoreCurrentFile() zero-argument APIs. An upload is
now ignored if the pattern matches either the relevant note path or the
source vault file path (never the generated S3 object key). Update the
"Ignore pattern" setting description to document this.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uj2kZV1XMWSqnNqvVYyjn7
…der Advanced
Cloudflare R2, MinIO, Backblaze B2, and other S3-compatible providers all
require a custom endpoint to connect at all, but the endpoint field and the
"Custom endpoint" toggle lived under Advanced — invisible for a normal R2
setup and impossible to debug without opening it. AWS S3 doesn't need an
endpoint at all, so it never belonged in that shared Advanced block either.
- Storage form now derives what's required from the provider preset
(requiresCustomEndpoint/region): Endpoint URL moves to the main form,
full-width, only for providers that need it; the "Custom endpoint" toggle
now only appears for providers where it's genuinely optional (AWS).
Region shows as a disabled, provider-derived "auto" for R2 instead of an
editable field. Force path-style URLs stays in Advanced as the one
genuinely optional compatibility override.
- Add validateStorageConfiguration() as the single source of truth for
provider-aware required fields, returning structured missing-field info.
StatusRow's isConfigurationComplete() and the Storage section's "Test
connection" preflight both delegate to it, so a missing field is reported
locally ("Endpoint URL is required for Cloudflare R2.") before any S3
HeadBucket call, instead of a vague SDK/network error.
- Move the provider label map from StatusRow into migrate.ts (single
source, avoids a circular import with the new validation module).
- Extend the obsidian/dom test mocks to actually record Setting name/desc
text and attach input elements into the tree, so render tests can assert
on field placement and disabled state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018d9JKhATCCJtgQKSAQFhHH
manifest.json id, package.json name, install path, and internal class/CSS/settings-key identifiers are unchanged. Settings page heading already read "Paste to S3" from a prior commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVMfQPvre1tFmKKEm8dzHc
|
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.
Summary
Replaces the implementation-oriented settings layout (Connection settings / Upload settings / Image processing / Watermark / Debug) with one organized around the questions a user actually asks, in order:
Key UX changes:
auto+ custom endpoint) without ever discarding manually entered values. Test connection moved to after the required fields (configure → test → see result), with a persistent result line.customImageUrl/ public URL derivation / query string now live here, since they answer "what link gets inserted," not "where is the object uploaded." A read-only outcome preview shows the exact object key and markdown link the current settings would produce.disableAutoUploadOnCreate) for backward compatibility — only the UI mapping is positive. The description now explicitly discloses that this feature uploads the new attachment and replaces the local file with the remote link, since that side effect wasn't previously called out.Files
src/settings.ts(701 lines) removed, replaced bysrc/settings/:types.ts,defaults.ts,migrate.ts— schema, defaults, and backward-compat derivation/provider-preset logic (all pure, unit tested)SettingsTab.ts— composes sections only, no business logicsections/{Storage,Links,UploadBehavior,ImageOptimization,Watermark,Advanced}Section.tscomponents/{fields,SettingSection,StatusRow,OutcomePreview}.ts— shared field builders, section/disclosure primitives, the top status summary, and the outcome previewsrc/uploader.ts— extractedresolvePublicBaseUrl,appendQueryString,resolvePublicUrl,buildObjectKey(see "Shared logic" below)src/main.ts—createS3Client()now callsresolvePublicBaseUrl;loadSettings()now callsmigrateSettings()src/pasteHandler.ts—handleFileUploadnow callsbuildObjectKeyinstead of re-deriving the key inlineCSS
styles.csswas rewritten around the new DOM structure (still under ther2-*namespace): top-level collapsible sections, nested "Advanced ▸" disclosures, a status-row primitive, an outcome-preview primitive, and the existing watermark-preview canvas styles. Removed now-unused.r2-btn-success/.r2-btn-error(connection-test state is now shown as text, not button color). Reuses Obsidian theme variables throughout (--background-secondary,--text-muted,--color-green/red, etc.), and adds narrow-pane/mobile handling so long endpoint/bucket/URL values wrap instead of overflowing.Shared logic
Per the task's critical rule, the settings outcome preview does not reimplement any path/URL logic.
buildObjectKeyandresolvePublicUrllive insrc/uploader.ts(the real upload implementation) and are imported by bothpasteHandler.ts(actual upload) andsrc/settings/components/OutcomePreview.ts(preview).tests/outcomePreview.test.tsasserts the preview's output is byte-for-byte what calling those functions directly would produce.Tests
61 tests passing (30 pre-existing + 31 new):
tests/settingsMigrate.test.ts— legacy→new field derivation (storageDestination/storageProvider/publicUrlMode), idempotency, that migration never overwrites an explicitly-persisted new field, provider-default application never clobbering manually configured region/endpoint/forcePathStyle/bucket/credentials, and the positive/negative auto-upload-on-create mapping round-trip.tests/urlResolution.test.ts—resolvePublicBaseUrlfor AWS-style default, Cloudflare/custom-endpoint, path-style, and custom-CDN cases;appendQueryString;buildObjectKey.tests/outcomePreview.test.ts— preview output equals directly callingbuildObjectKey/resolvePublicUrl, for both S3 and local destinations, and the.webp/.pngextension switch.Compatibility
storageDestination,storageProvider,publicUrlMode) are optional on the type and derived from existing legacy fields (localUpload,useCustomEndpoint/customEndpoint,useCustomImageUrl) only when absent from persisted data — existing credentials, endpoints, folders, and custom URLs are never touched.disableAutoUploadOnCreatestays the persisted field name; no data migration needed for it, only a UI-level positive/negative mapping.localUploadboolean is kept in sync with the newstorageDestinationfield sopasteHandler.ts's existing runtime logic (which still readssettings.localUpload) is unaffected.Follow-ups (not fixed here, out of scope for this refactor)
README.zh-TW.mdexists onmasterin this repo, so there was nothing to keep in sync there (it exists on other branches doing an unrelated rebrand).Verification
Ran the repo's quality gates (
npm run lint,tsc -noEmit -skipLibCheck,npm test,npm run build) — all passing. Verification was via code reading and these automated gates only; I did not load the plugin into a live Obsidian vault.🤖 Generated with Claude Code