Profiles: rename, naming, ordering, deletion, save/cancel UX - #31
Open
mwthomson wants to merge 13 commits into
Open
Profiles: rename, naming, ordering, deletion, save/cancel UX#31mwthomson wants to merge 13 commits into
mwthomson wants to merge 13 commits into
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Carries changes-saved / no-changes / removed / deleted / delete-failed messages across a redirect to the Profiles page via a `flash` query parameter, since the app has no session or cookie layer. Not wired into any handler yet.
Profiles now have a required display name and a Last Activated time. The Profiles list always shows the active Profile first, then the rest ordered by Last Activated (most recent first). Adding a Profile can optionally make it active immediately via a new checkbox on the Add form. Refs #29
A pre-Profile-rename state.json now has no `profiles` key, so deserialization fails and DireUI falls back to AppState::default() rather than attempting to interpret the old known_configs shape — this is the intended, documented behavior (ADR-0005), not a bug; this test pins it down explicitly.
Save now compares the submitted text against the Active Config's current on-disk content before writing: identical content redirects to Profiles with "no changes to save" instead of rewriting the file; a real change redirects with "changes saved"; a write failure re-renders the editor in place with "save failed: [reason]" and the user's unsaved edits intact. This also fixes raw-config write failures being silently swallowed (only eprintln'd) before this change. Cancel always discards and returns to Profiles.
… edit Task-review fix for two Important findings against commit 1c996c3, not new functionality: - save_raw_config compared the CRLF-normalized submitted textarea content against current_content read straight from disk, which is not normalized. A Config File with CRLF line endings therefore always looked "changed" on an unedited Save, silently rewriting the file's line endings every time. Normalize current_content the same way before the comparison. Adds a regression test (save_raw_config_detects_no_change_against_crlf_file_on_disk) that writes a CRLF file, submits it unchanged, and asserts both the no-change flash and that the on-disk bytes are untouched. - save_directives's write_config(...) call was changed to `let _ = write_config(...)` plus a comment, purely to silence the new #[must_use] warning after write_config started returning Result<(), String>. That function is out of scope for this task and is rewritten wholesale by a later task, so revert the call site back to a bare statement. This leaves an expected unused_must_use warning at that line until the later task replaces the function body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Compares the built Document against the on-disk parsed Document (structural equality, so this stays compatible with ADR-0001's round-trip preservation) before writing. Validation failures keep their existing per-field messages and now also show a top-level "save failed" message. Cancel always discards and returns to Profiles.
Adds CONTEXT.md entries for Last Activated, Remove, and Delete, and sharpens the Active Config/Profile entries to describe what's actually being built rather than the prior aspirational phrasing. Also records ADR-0005 (no state.json migration) and the implementation plan this branch executes.
…nt equality set_curated unconditionally rewrites a matched directive line's raw text to canonical single-space form on every save, for every field in CURATED_FIELDS, regardless of whether the value actually changed. This made the doc == original_doc check in save_directives unreliable: a config file with any curated directive line in non-canonical spacing (plausible in a hand-typed direwolf.conf) would get its spacing silently reformatted and incorrectly flash "changes saved" on a save where the user changed nothing. Fix by tracking, per field, whether the submitted value differs from original_doc's parsed value (via get_curated, before any mutation), and using that to decide NoChange vs. Saved instead of Document equality. Validation still runs for every field as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A pencil icon next to each Profile's name toggles it into an editable field (pure client-side toggle, no server round-trip for the toggle itself); saving posts to the new /profiles/rename route and returns to the Profiles page.
…e file) A trashcan icon opens a shared dialog with three choices: cancel, Remove (deletes the Profile record only), or a second-confirmed Delete (also removes the Config File from disk, irreversible). Deleting the active Profile is allowed — DireUI auto-promotes the next Profile by Last Activated so there's never a Profile list with nothing active.
.profile-rename-form{display:flex} and the browser's [hidden]{display:none}
have equal specificity, so the class rule (later in the cascade) was
winning and the rename form rendered open on every page load instead of
staying hidden until the pencil icon is clicked. Found during Task 9's
manual browser walkthrough — no automated test could catch this, since it's
purely a CSS cascade/rendering behavior. A more specific
.profile-rename-form[hidden] rule restores the intended hidden state.
…mpty list, name fallback, curated-line reformatting, stale terminology
Addresses the final whole-branch review of profiles-rework before merge:
- delete_profile now checks the submitted path is a known Profile before
touching the filesystem, closing an unauthenticated arbitrary-file-deletion
gap (this server has no auth/CSRF protection). Unknown paths redirect with
flash=delete_failed and leave disk and state untouched.
- views::first_run now accepts and renders a flash banner (via a shared
flash_banner_html helper also used by profiles_page), so ?flash=removed /
?flash=deleted is no longer silently dropped when the Profile list becomes
empty.
- state::normalize_profile_name falls back to the Config File's basename when
a Profile name is empty/whitespace after trimming, applied in both
add_profile and rename_profile, enforcing the "names are required" rule
server-side rather than relying solely on the HTML `required` attribute.
- save_directives now only calls set_curated (which unconditionally
reformats a line to canonical spacing) for fields whose submitted value
actually differs from the on-disk value, so a real edit to one curated
field no longer silently reformats unrelated curated lines. Untouched
fields are no longer re-validated on that save (accepted trade-off).
- Cleaned up stale "config manager"/"saved config"/".config-badge" wording
left over from the Profile rename, and adjusted the delete-icon
aria-label ("Remove or delete profile") since Remove, not Delete, is the
dialog's non-destructive default action.
Adds handler-level tests for delete_profile (unknown path rejected, known
path deleted, active-Profile-deletion promotion through the route handler),
a first_run flash-rendering test, state-level name-fallback/trim tests, and
a save_directives test proving an unrelated curated line's non-canonical
spacing survives a genuine single-field change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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
Renames DireUI's "Configurations" concept to "Profiles" throughout the codebase and adds the features scoped in #29 (closed):
changes saved/no changes to save/save failed: [reason]), with no-change detection that skips the write entirely, and a Cancel button that always discards silently.state.json's schema change (name + Last Activated timestamp) is an intentional breaking change with no migration, per new ADR-0005.Domain vocabulary updated in
CONTEXT.md(Profile, Active Config, Last Activated, Remove, Delete) to match.Built via
subagent-driven-development: 9 plan tasks (docs/superpowers/plans/2026-08-30-profiles-rework.md), each with its own implementer + task-scoped reviewer pass (two tasks needed one fix-and-re-review round each for real bugs the reviewers caught), followed by a final whole-branch review that found one Critical issue (an unvalidated delete path — arbitrary file deletion given this server has no auth/CSRF) and four Important cross-task issues, all fixed and independently re-verified. Full audit trail in.superpowers/sdd/progress.md.One non-blocking follow-up was filed separately as #30 (state.json's silent reset on an unreadable file could use a warning + backup-before-overwrite) rather than folded into this branch.
Test plan
cargo build— zero warningscargo test— 135/135 passing🤖 Generated with Claude Code