Repository navigation
Stabilize settings rendering and enforce release integrity - #98
Conversation
This comment has been minimized.
This comment has been minimized.
Co-authored-by: frostmute <989225+frostmute@users.noreply.github.com>
namedTemplates data
This comment has been minimized.
This comment has been minimized.
frostmute
left a comment
frostmute
left a comment
There was a problem hiding this comment.
PR #98 stabilization pass: permanently close the settings/release regression
Please expand this PR into the complete v2.1.5 settings stabilization fix.
The existing normalizeNamedTemplates() change is valid and should be retained, but it addresses only one possible path to the blank settings pane reported in #97. We need to fix the settings lifecycle ambiguity, harden persisted settings hydration, add regression coverage for the actual user-visible failure, and prevent the release pipeline from producing mismatched source/version/artifact states again.
Goal
After this PR and the associated release-workflow changes:
- Make It Rain settings must reliably open and render through the normal imperative PluginSettingTab.display() lifecycle.
- Malformed or legacy persisted settings must not prevent the settings pane from opening.
- The plugin must have exactly one clear settings-rendering path.
- CI must detect inconsistent release metadata.
- The release workflow must version first, test/build second, and release artifacts produced from the exact commit being tagged.
- v2.1.5 must establish a clean release boundary without rewriting old release tags.
Do not migrate the settings UI to Obsidian’s declarative settings API in this PR. That is explicitly out of scope.
⸻
- Keep and strengthen persisted-data normalization
Retain the new normalizeNamedTemplates() work.
The settings loader must treat persisted JSON as untrusted runtime data rather than assuming TypeScript types are valid at runtime.
At minimum, verify namedTemplates behavior for:
- null
- arrays
- strings/numbers/other non-object values
- valid object maps
- objects containing a mixture of valid string values and invalid non-string values
Malformed top-level namedTemplates should safely fall back to the defaults.
Invalid entries inside an otherwise valid object should be discarded without throwing.
Do not silently remove valid user-defined string templates.
Also inspect the existing hydration of these structured settings for the same class of unsafe assumption:
- importPresets
- contentTypeTemplates
- contentTypeTemplateToggles
- namedTemplates
Make only targeted defensive changes required to guarantee settings rendering. Do not turn this into a broad settings-schema refactor.
⸻
- Remove the hybrid declarative/imperative lifecycle
In src/settings.ts, completely remove the getSettingDefinitions() override that returns an empty array.
Do not replace it with another empty/stub implementation.
Make It Rain currently has a large imperative settings UI. For v2.1.5 it should unambiguously use the imperative PluginSettingTab.display() lifecycle.
The public lifecycle should conceptually be:
Obsidian
↓
display()
↓
renderSettings()
↓
DOM
There should not be competing display(), update(), and getSettingDefinitions() entry paths whose relationship depends on framework behavior.
⸻
- Make display() the authoritative framework entry point
Keep/implement:
display(): void {
this.renderSettings();
}
Rename/refactor the current update() renderer into a clearly named method such as:
private renderSettings(): void
If existing internal operations need to trigger a refresh after actions such as template import, preset rename/delete, etc., route those calls through the same renderer or through a small refresh() wrapper that delegates to it.
There must ultimately be one implementation responsible for constructing the settings DOM.
Do not duplicate rendering logic.
⸻
- Replace the misleading lifecycle regression test
The current test contains an assumption equivalent to:
Obsidian calls display() whenever getSettingDefinitions() returns an empty array.
Remove that assumption from the tests and source comments.
Add/retain a direct contract test proving that calling display() renders the real settings interface.
At minimum assert visible markers such as:
- make-it-rain-settings-container
- Connection & Core Setup
- Template Engine
The test should validate our own supported contract:
display() -> rendered settings UI
not an inferred fallback relationship between declarative and imperative APIs.
Also add a regression guard ensuring this class does not reintroduce its own getSettingDefinitions() override as part of this imperative implementation. Use a robust assertion appropriate to the installed Obsidian API/mock environment rather than relying on an assertion that would fail merely because a future base class defines the method.
⸻
- Add the missing user-visible regression test
The current Copilot test proves malformed data can pass through loadSettings(). That is necessary but insufficient.
Add a test covering the complete failure chain from #97:
malformed persisted settings
↓
loadSettings()
↓
construct settings tab
↓
display()
↓
settings render successfully
For example, load persisted data containing:
{
namedTemplates: null
}
Then instantiate the settings tab, call display(), assert that it does not throw, and assert that actual settings sections appear in the DOM.
This test is important because #97 is about being unable to access settings, not merely about the shape of plugin.settings.
Where practical, add another case representing valid legacy/minimal saved settings so upgrades from older plugin versions remain covered.
⸻
- Add a single release-metadata verification script
Create a small script such as:
scripts/verify-release.mjs
or an equivalently named script.
It must fail with a non-zero exit code if release metadata is inconsistent.
Verify at minimum:
package.json.version === manifest.json.version
and:
versions.json[manifest.version] === manifest.json.minAppVersion
When an expected version is supplied by CI/release automation, also verify:
package.json.version === expectedVersion
manifest.json.version === expectedVersion
When executing in a tagged release context, verify that the release/tag version matches manifest.json.version.
Make failures explicit and easy to diagnose in Actions logs.
Expose this through an npm script, for example:
"verify-release": "node scripts/verify-release.mjs"
The exact name is flexible, but there should be one authoritative verification implementation shared by CI and release automation.
⸻
- Eliminate duplicate version-bump implementations
The repository already contains:
scripts/version-bump.mjs
while .github/workflows/release.yml independently edits package.json, manifest.json, and versions.json using inline Node scripts.
Remove this duplication.
There should be one authoritative versioning path.
Prefer using npm’s normal version mechanism plus the existing/refactored version-bump script, or otherwise consolidate version mutation into a repository script that both humans and automation can invoke.
After setting a release version, these files must agree:
package.json
manifest.json
versions.json
For v2.1.5 the expected state is:
package.json.version = 2.1.5
manifest.json.version = 2.1.5
versions.json["2.1.5"] = manifest.minAppVersion
Do not rewrite historical version entries unnecessarily.
⸻
- Fix the release workflow ordering
The current .github/workflows/release.yml builds the plugin before updating release version metadata.
That ordering must be removed.
The release sequence should instead be:
checkout
↓
validate requested version
↓
set/update version metadata
↓
verify metadata consistency
↓
install dependencies
↓
tests
↓
lint / relevant static checks
↓
build
↓
verify artifacts
↓
commit release metadata
↓
identify exact release commit
↓
tag that exact commit
↓
verify tag/commit/version relationship
↓
create GitHub release from those artifacts
The key invariant is:
The uploaded main.js, manifest.json, and styles.css must all be products of the exact source/version state represented by the release tag.
Never build main.js, mutate version metadata afterward, and then package those files together.
⸻
- Use deterministic dependency installation
In GitHub Actions use:
npm ci
instead of:
npm install
assuming the repository’s committed lockfile supports it.
Both CI and release builds should use deterministic dependency installation.
⸻
- Run the real quality gates before publishing
The release workflow must not rely solely on a successful build.
Before creating a tag/release, run the relevant existing checks, including at minimum:
npm test
npm run build
and the project’s lint/static checks where currently supported.
Run the release metadata verifier before and after the build as appropriate.
A failed test, version invariant, or build must prevent tagging/publishing.
⸻
- Verify release artifacts before publishing
Before creating the GitHub release, assert that the required artifacts exist:
main.js
manifest.json
styles.css
Parse the release manifest.json and verify:
id === "make-it-rain"
version === requested release version
Also verify that the tag points to the exact intended release commit.
Do not merely assume these conditions based on earlier steps.
⸻
- Make CI enforce metadata integrity
Update .github/workflows/ci.yml so normal PRs/main builds run the release/version metadata consistency check.
This should catch states such as:
package.json 2.1.5
manifest.json 2.1.4
or a missing/mismatched versions.json entry before they reach a release.
Keep the existing secret scan, tests, coverage, and build checks.
⸻
- Simplify dangerous release switches
Review the current:
skip_version_bump
force
workflow inputs.
Prefer removing them if possible.
A normal release should have one deterministic path rather than multiple combinations of repository state and workflow flags.
Do not silently overwrite an existing published release/tag.
If the requested release version/tag already exists in a way that would require history mutation, fail clearly and require a new patch version instead.
Historical releases/tags 2.1.2, 2.1.3, and 2.1.4 should not be rewritten as part of this work.
⸻
- v2.1.5 acceptance criteria
Do not consider this stabilization complete until all of the following are true:
- getSettingDefinitions() is no longer overridden by RaindropToObsidianSettingTab.
- display() is the clear framework entry point for the imperative settings UI.
- There is one underlying settings DOM renderer.
- Existing internal settings refreshes still work.
- namedTemplates: null cannot crash settings loading/rendering.
- Other malformed namedTemplates shapes are handled defensively.
- A regression test loads malformed persisted settings and then successfully calls display().
- The rendered DOM test proves actual settings controls/sections exist.
- Existing tests pass.
- CI passes.
- Package, manifest, and versions metadata are programmatically checked for consistency.
- Release metadata is established before main.js is built.
- Release tests/checks run before tagging/publishing.
- The release tag points at the exact release commit.
- Uploaded artifacts correspond to that exact version/source state.
- Existing release tags are not rewritten.
⸻
Scope constraints
Please keep this focused.
Do NOT:
- migrate the settings UI to the declarative settings API;
- redesign the settings UI;
- change unrelated plugin behavior;
- perform broad refactors unrelated to #97/release integrity;
- rewrite historical release tags;
- paper over failing tests;
- weaken CI checks to get the PR green.
If implementation reveals that any requested invariant conflicts with the current Obsidian API/types or repository architecture, document the conflict in the PR rather than silently inventing another compatibility shim.
⸻
Final PR report
When finished, update the PR description or leave a summary containing:
- the distinct root/failure paths addressed;
- files changed;
- lifecycle behavior before vs. after;
- persisted-data normalization added;
- regression tests added;
- release workflow ordering before vs. after;
- version/release invariants now enforced;
- commands/checks run and their results;
- any remaining risks or follow-up work.
The intended outcome is not merely “Issue #97 appears fixed.”
The intended outcome is:
This class of blank-settings regression and mismatched-release regression is now mechanically difficult to reintroduce.
Co-authored-by: frostmute <989225+frostmute@users.noreply.github.com>
Addressed in commit Implemented the full stabilization pass requested: removed the |
frostmute
left a comment
frostmute
left a comment
There was a problem hiding this comment.
The stabilization implementation is largely correct. Please make the following final release-integrity corrections before this PR is considered mergeable.
- Resolve the v2.1.5 pre-versioning conflict
The PR currently commits package.json, manifest.json, and versions.json as v2.1.5, while release.yml later runs:
npm version "${{ inputs.version }}" --no-git-tag-version
Running the release workflow for 2.1.5 against source already at 2.1.5 can fail because npm rejects an unchanged version.
Choose one authoritative model and make it explicit.
Preferred model: the release workflow owns the transition to the release version. Do not pre-version ordinary source merely to prepare the release.
If there is a compelling repository-specific reason to keep source pre-versioned, make the workflow safely support the same-version case without weakening any other release invariants.
Add coverage/validation sufficient to prevent this workflow/source-state contradiction from recurring.
- Include package-lock.json in release metadata integrity
The current PR has:
package.json 2.1.5
manifest.json 2.1.5
versions.json 2.1.5
package-lock.json 2.1.2
verify-release.mjs currently does not detect this.
Extend release verification so the root package version represented by package-lock.json, including the root packages[""] entry where applicable, must agree with package.json.
The intended invariant is:
package.json.version
package-lock.json.version
package-lock.json.packages[""].version
manifest.json.version
expected release version
with:
versions.json[version] = manifest.json.minAppVersion
Use the repository’s normal npm version mechanism to update the lockfile rather than editing it manually.
- Avoid partial remote publication of the release commit/tag
The workflow currently performs:
git push origin HEAD:main
git push origin "$VERSION"
as separate operations.
A failure between those commands can leave the release commit on main without its corresponding tag, creating a partially published release state.
Prefer an atomic push of the release commit and tag if supported by the GitHub remote/workflow environment.
The desired invariant is:
The release commit and its release tag become visible remotely together, or neither does.
Retain the existing remote tag SHA verification afterward.
- Run the actual CI workflow successfully
The current GitHub CI run for this head reports action_required, not a successful execution.
After these corrections, ensure the real CI workflow executes and passes, including:
- deterministic install;
- release metadata verification;
- secret scan;
- unit tests;
- coverage;
- build;
- artifact verification.
Do not treat external review-bot statuses as a substitute for the repository CI workflow.
Do not change the settings lifecycle/data-hydration implementation unless required by a failing regression test. That portion of the stabilization work is currently aligned with the requested architecture.
Restore source metadata to 2.1.2 so the release workflow owns the npm version transition to 2.1.5. Verify both lockfile root versions, reject unchanged or older requests, and publish the release commit and tag atomically. Add npm lifecycle and atomic-push regression coverage.
frostmute
changed the title
namedTemplates data
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. |
|
@codex review |
![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: 026e33ba25
ℹ️ 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".
Users could not open Make It Rain settings when persisted structured settings were malformed. This PR makes the settings UI use one imperative rendering path, protects hydration from malformed data, and fixes release version/artifact integrity.
Fixes #97.
Settings behavior and regression coverage
display()delegates to the singlerenderSettings()DOM builder; the emptygetSettingDefinitions()override and competingupdate()entry point are removed. Internal refreshes use the same renderer.namedTemplates,contentTypeTemplates,contentTypeTemplateToggles, andimportPresets, retaining valid user templates.loadSettings()followed bydisplay(), assert real settings sections, and guard against reintroducing an own declarative override.Release integrity corrections
scripts/prepare-release.mjsrejects unchanged, older, or malformed release versions, verifies input metadata, invokes the existing npm version lifecycle with--no-git-tag-version, and verifies the result. npm updates the lockfile; the existing version hook updates the manifest and version map.verify-release.mjschecks package.json, both package-lock.json root version representations, manifest.json, the versions.json minimum-app mapping, and any expected version/tag context. A v2/v3 lockfile without its root package entry fails.git push --atomicfor main and the release tag. Remote tag SHA verification remains. Releases are serialized and checkout explicitly targets main.npm run test:release. Release automation also runs the release regression suite and secret scan before publication.Release regression tests
tests/release/release-integrity.test.mjscovers the real npm transition from 2.1.2 to 2.1.5, preservation of historical version entries, no premature tag, unchanged/older/invalid requests, mismatched or missing lockfile root versions, legacy v1 lockfiles, expected/tag/manifest/minAppVersion mismatches, and rejection of both remote refs when an atomic publication's tag is rejected.Validation status
The release-integrity follow-up is committed as
026e33b. JSON and JavaScript syntax checks passed while preparing the changes. This session has GitHub connector tools but no local shell, so npm install/tests/lint/build have not been run locally.The PR was briefly closed/reopened to trigger CI on the revised head. The new CI Build run 37604434504 concluded
action_requiredwith no jobs executed. GitHub rejected a retry with HTTP 403: “This workflow run cannot be retried.” The connector provides no workflow approval action. A maintainer must approve the run in GitHub; a successful repository CI run is still required before merging. Review-bot statuses do not satisfy that requirement. The settings implementation was not changed during this follow-up, and no release was published or historical tag rewritten.