diff --git a/.claude/skills/rust-code-style/SKILL.md b/.claude/skills/rust-code-style/SKILL.md new file mode 100644 index 0000000..76cc18d --- /dev/null +++ b/.claude/skills/rust-code-style/SKILL.md @@ -0,0 +1,73 @@ +--- +name: rust-code-style +description: Project conventions for this Yew/WASM frontend — Rust coding style grounded in programmer-cognition research (Felienne Hermans' "The Programmer's Brain") plus Yew 0.23 framework best practices. Use when writing, reviewing, or refactoring code in this project to keep names descriptive, functions short and single-purpose, error handling explicit, Rust idioms (iterators, `?`, exhaustive matching, newtypes) applied consistently, AND Yew components correct (pure `view`, side effects driven by messages not `changed()`, keyed lists, no panics in WASM, deliberate cloning). +--- + +# Rust + Yew Code Style + +Conventions for writing and reviewing this project's code. It is a **Yew 0.23** WASM single-page app +(struct components, `yew-router`, `ybc`/Bulma, `gloo-*`, `reqwasm`), so the rules cover both general +Rust style **and** Yew framework usage. The guiding rule: **write code for the human brain, not the computer.** Code is read far more often than written, so every decision should reduce the reader's cognitive load. + +## Core Principles + +1. **Optimize for reading, not writing** — code is read ~10x more than written. +2. **Reduce cognitive load** — working memory holds ~7 items; don't waste slots on cryptic names. +3. **Make intent explicit** — names reveal purpose without forcing the reader to hold context in their head. +4. **Favor clear beacons** — readers scan for recognizable patterns; descriptive names beat single letters. + +These cash out into four defaults: full names over abbreviations, explicit over implicit, simple over clever, readable over writeable. + +## The non-negotiable rule + +**Never use single-letter variable names**, except in three narrow cases: tiny (1–2 line) closures with obvious context, math formulas matching standard domain notation (`dx`, `dy`), and generic type parameters (`T`, `U`). When in doubt, use a full name. + +## Reference guides + +Load the relevant guide when working in that area — each contains DON'T/DO examples: + +- **[references/naming.md](references/naming.md)** — variable, loop, iterator, boolean, function, and domain naming; abbreviations; pronounceability. +- **[references/functions.md](references/functions.md)** — function length (≤25 lines), single responsibility, parameter limits (≤3), nesting depth (≤2), guard clauses. +- **[references/rust-idioms.md](references/rust-idioms.md)** — explicit signatures, type aliases, newtypes, enums for state, avoiding `unwrap()`/`expect()`, error context, exhaustive matching, `#[must_use]`, iterators, `?`, `let-else`, `LazyLock`/`OnceLock`, `NonZero`, `#[non_exhaustive]`. +- **[references/organization-and-quality.md](references/organization-and-quality.md)** — module structure, file size limits, comments (why not what), public-API docs, error design, testing conventions, anti-patterns (god objects, magic numbers, clever code). +- **[references/yew-best-practices.md](references/yew-best-practices.md)** — Yew 0.23 component conventions: pure `view`, side effects from messages (not `changed()`), keyed lists, no panics in WASM (`SessionStorage`/header/JSON `unwrap`), deliberate cloning + `Rc`, `Properties` design, initial-load placement, callbacks, shared context, `yew-router`, and when to use function components + hooks. **Load this whenever touching anything under `src/components/`, `src/pages/`, or `lib.rs`.** + +## Code review checklist + +Tooling (run first — catches most mechanical issues automatically): +- [ ] `cargo fmt` applied +- [ ] `cargo clippy -- -D warnings` passes + +Naming and structure: +- [ ] No single-letter variable names (except the limited exceptions above) +- [ ] All names are clear and descriptive +- [ ] Functions are short and focused (<25 lines) +- [ ] Nesting depth is minimal (≤2 levels) — use `let-else` for destructuring guards + +Error handling: +- [ ] No `unwrap()` or `expect()` in production code +- [ ] Error handling is explicit with context +- [ ] String formatting uses capture syntax: `"{val}"` not `"{}"` + val + +API design: +- [ ] Public APIs have documentation +- [ ] Public enums in library code are `#[non_exhaustive]` if they may grow +- [ ] Types make invalid states unrepresentable where possible + +Yew components (see [references/yew-best-practices.md](references/yew-best-practices.md) for details): +- [ ] `view` is pure — no I/O, storage writes, `unwrap()`/panics, or heavy computation +- [ ] No side effects in `changed()` — only prop→state reconciliation (a user action → `Msg` → effect) +- [ ] Every `.map()`-rendered list element has a stable `key` (uuid/id, not the array index) +- [ ] No `unwrap()`/`expect()` on `SessionStorage`, response headers, or JSON — route to `Msg::Error`/`Logoff` (WASM panics blank the page) +- [ ] `.clone()` limited to values moved into closures/`async move`; large shared data uses `Rc` +- [ ] `Properties` derive `PartialEq`, stay small, pass `Callback`s/ids (or `Rc<…>`) not big owned collections +- [ ] Initial data loads in `create`/`rendered(first_render)`, never in `view` +- [ ] In-app navigation uses `yew-router` (`Navigator`/`Link`), not `window.location` + +General: +- [ ] File is ≤500 lines — split by responsibility if over; target is 200–300 +- [ ] Tests are comprehensive with descriptive names +- [ ] No commented-out code +- [ ] No magic numbers — all constants are named +- [ ] Code follows existing patterns in the codebase (struct components stay struct components) +- [ ] Rust idioms are used (iterators, `?`, `let-else`, pattern matching) diff --git a/.claude/skills/rust-code-style/references/functions.md b/.claude/skills/rust-code-style/references/functions.md new file mode 100644 index 0000000..5f9f21b --- /dev/null +++ b/.claude/skills/rust-code-style/references/functions.md @@ -0,0 +1,125 @@ +# Function Design + +## Contents +- [Keep functions short](#keep-functions-short) +- [Single Responsibility Principle](#single-responsibility-principle) +- [Limit function parameters](#limit-function-parameters) +- [No deep nesting](#no-deep-nesting) + +## Keep functions short + +**Target:** 5–15 lines. **Maximum:** 25 lines. If longer, extract smaller functions with descriptive names. + +## Single Responsibility Principle + +Each function does one thing well. + +**DON'T:** +```rust +fn process_user(id: i32) -> Result<()> { + let user = db.fetch(id)?; + if user.email.is_empty() { return Err(Error::Invalid); } + let perms = calc_perms(&user)?; + db.update_perms(user.id, perms)?; + email::send(&user.email, "Welcome")?; + log::info("User processed: {}", user.id); + cache.invalidate(user.id); + Ok(()) +} +``` + +**DO:** +```rust +fn process_user(id: i32) -> Result<()> { + let user = fetch_and_validate_user(id)?; + grant_user_permissions(&user)?; + notify_user_by_email(&user)?; + log_user_activity(&user)?; + invalidate_user_cache(&user)?; + Ok(()) +} +``` + +## Limit function parameters + +**Maximum:** 3 parameters. If more are needed, use a struct/config object. + +**DON'T:** +```rust +fn create_user( + name: String, + email: String, + age: i32, + role: String, + dept: String, + manager: String, + location: String +) -> User +``` + +**DO:** +```rust +struct CreateUserRequest { + name: String, + email: String, + age: i32, + role: String, + department: String, + manager: String, + location: String, +} + +fn create_user(request: CreateUserRequest) -> User +``` + +## No deep nesting + +**Maximum nesting level:** 2. Use early returns, guard clauses, and `let-else` for destructuring guards (stable since 1.65). + +**DON'T:** +```rust +fn process(data: Option) -> Result<()> { + if let Some(d) = data { + if d.is_valid() { + if d.has_permission() { + if d.is_ready() { + // deeply nested work + } + } + } + } + Ok(()) +} +``` + +**DO:** +```rust +fn process(data: Option) -> Result<()> { + let data = data.ok_or(Error::NoData)?; + + if !data.is_valid() { + return Err(Error::Invalid); + } + if !data.has_permission() { + return Err(Error::Forbidden); + } + if !data.is_ready() { + return Err(Error::NotReady); + } + + // work at top level + Ok(()) +} +``` + +When the guard involves destructuring, prefer `let-else` over `if let` + nesting: + +```rust +fn process(event: Option) -> Result<()> { + let Some(event) = event else { + return Ok(()); + }; + // event is in scope here, no extra nesting + handle(event) +} +``` diff --git a/.claude/skills/rust-code-style/references/naming.md b/.claude/skills/rust-code-style/references/naming.md new file mode 100644 index 0000000..e38db88 --- /dev/null +++ b/.claude/skills/rust-code-style/references/naming.md @@ -0,0 +1,247 @@ +# Naming Conventions + +## Contents +- [Never use single-letter variable names](#never-use-single-letter-variable-names) +- [Limited exceptions to the single-letter rule](#limited-exceptions-to-the-single-letter-rule) +- [Loop variables must be descriptive](#loop-variables-must-be-descriptive) +- [Iterators and temporaries need names too](#iterators-and-temporaries-need-names-too) +- [Use full, descriptive names](#use-full-descriptive-names) +- [Avoid abbreviations](#avoid-abbreviations) +- [Use pronounceable names](#use-pronounceable-names) +- [Boolean names should form questions](#boolean-names-should-form-questions) +- [Function names should be verbs](#function-names-should-be-verbs) +- [Use domain-specific language](#use-domain-specific-language) + +## Never use single-letter variable names + +This is a hard rule with very limited exceptions. Single-letter variables force readers to keep mental mappings in working memory, scroll back to find context, and guess meaning from usage. + +**DON'T:** +```rust +let b = get_balance(); +let u = fetch_user(); +let c = calculate_cost(u, b); +let t = SystemTime::now(); +let r = make_request(); +let s = "hello"; +let i = 0; +let n = items.len(); +``` + +**DO:** +```rust +let balance = get_balance(); +let user = fetch_user(); +let total_cost = calculate_cost(user, balance); +let timestamp = SystemTime::now(); +let response = make_request(); +let greeting = "hello"; +let index = 0; +let item_count = items.len(); +``` + +## Limited exceptions to the single-letter rule + +Only acceptable in these narrow contexts: + +1. **Very short closures (1–2 lines) with obvious context:** +```rust +// Acceptable - x is clearly each number in the iterator +let doubled: Vec<_> = numbers.iter().map(|x| x * 2).collect(); + +// Better - still prefer descriptive names when not obvious +let doubled: Vec<_> = numbers.iter().map(|num| num * 2).collect(); +``` + +2. **Mathematical formulas where letters match domain notation:** +```rust +fn distance(x1: f64, y1: f64, x2: f64, y2: f64) -> f64 { + let dx = x2 - x1; + let dy = y2 - y1; + (dx * dx + dy * dy).sqrt() +} +``` + +3. **Generic type parameters (by convention):** +```rust +struct Container { value: T } +fn map(item: T, f: impl Fn(T) -> U) -> U +``` + +**When in doubt, use a full name. Always.** + +## Loop variables must be descriptive + +**DON'T:** +```rust +for i in 0..users.len() { + process(users[i]); +} + +for (i, u) in users.iter().enumerate() { + println!("{}: {}", i, u.name); +} +``` + +**DO:** +```rust +for user_index in 0..users.len() { + process(users[user_index]); +} + +for (index, user) in users.iter().enumerate() { + println!("{}: {}", index, user.name); +} + +// Or better, avoid indices when possible +for user in &users { + process(user); +} +``` + +## Iterators and temporaries need names too + +**DON'T:** +```rust +let x = users.iter().filter(|u| u.is_active()); +let y = x.map(|u| u.email.clone()); +let z: Vec<_> = y.collect(); +``` + +**DO:** +```rust +let active_users = users.iter().filter(|user| user.is_active()); +let user_emails = active_users.map(|user| user.email.clone()); +let email_list: Vec<_> = user_emails.collect(); + +// Or chain with clear intermediate meaning +let email_list: Vec<_> = users + .iter() + .filter(|user| user.is_active()) + .map(|user| user.email.clone()) + .collect(); +``` + +## Use full, descriptive names + +**DON'T:** +```rust +let usr = get_u(); +let amt = calc_a(usr); +let cfg = load_cfg(); +let db = connect(); +let req = parse_req(); +let resp = mk_resp(); +``` + +**DO:** +```rust +let user = get_user(); +let total_amount = calculate_total_amount(user); +let config = load_config(); +let database = connect_to_database(); +let request = parse_request(); +let response = create_response(); +``` + +## Avoid abbreviations + +Only use widely-known abbreviations (HTML, URL, API, ID, HTTP, JSON, XML, SQL). + +**DON'T:** +```rust +proc_req() // process_request +init_cfg() // initialize_config +chk_val() // check_validation +get_usr_prfl() // get_user_profile +calc_tot() // calculate_total +fn_usr() // find_user +``` + +**DO:** +```rust +process_request() +initialize_config() +check_validation() +get_user_profile() +calculate_total() +find_user() +``` + +## Use pronounceable names + +If you can't say it in conversation with a teammate, don't use it. + +**DON'T:** +```rust +let usrxfdt = ...; // "user x f d t"? +let genymdhms = ...; // "gen y m d h m s"? +let prcssr = ...; // "p r c s s r"? +``` + +**DO:** +```rust +let user_transfer_data = ...; +let generated_timestamp = ...; +let processor = ...; +``` + +## Boolean names should form questions + +**DON'T:** +```rust +let authenticated; +let valid; +let active; +let enabled; +let admin; +``` + +**DO:** +```rust +let is_authenticated; +let is_valid; +let is_active; +let is_enabled; +let is_admin; +// or +let has_permission; +let can_edit; +let should_retry; +``` + +## Function names should be verbs + +**DON'T:** +```rust +fn user() -> User +fn data() -> Data +fn response() -> Response +fn validation() -> bool +``` + +**DO:** +```rust +fn get_user() -> User +fn fetch_data() -> Data +fn create_response() -> Response +fn validate_input() -> bool +``` + +## Use domain-specific language + +Names should reflect the business domain, not implementation details. + +**DON'T:** +```rust +fn insert_db_record() +fn update_cache_entry() +fn serialize_to_json() +``` + +**DO:** +```rust +fn save_user() +fn update_user_profile() +fn export_user_data() +``` diff --git a/.claude/skills/rust-code-style/references/organization-and-quality.md b/.claude/skills/rust-code-style/references/organization-and-quality.md new file mode 100644 index 0000000..3778462 --- /dev/null +++ b/.claude/skills/rust-code-style/references/organization-and-quality.md @@ -0,0 +1,289 @@ +# Code Organization, Comments, Errors, Testing & Anti-Patterns + +## Contents +- [Module structure](#module-structure) +- [File size limits](#file-size-limits) +- [Group related code together](#group-related-code-together) +- [Comments: why, not what](#comments-why-not-what) +- [Document all public APIs](#document-all-public-apis) +- [Remove dead code](#remove-dead-code) +- [Make errors explicit and specific](#make-errors-explicit-and-specific) +- [Provide context in error messages](#provide-context-in-error-messages) +- [Testing](#testing) +- [Cognitive load reduction](#cognitive-load-reduction) +- [Anti-patterns to avoid](#anti-patterns-to-avoid) + +## Module structure + +```rust +// Consistent ordering: +// 1. Module-level documentation +// 2. Imports (grouped: std, external crates, internal crates, current crate) +// 3. Constants +// 4. Type definitions (structs, enums) +// 5. Trait implementations +// 6. Public functions +// 7. Private functions +// 8. Tests module + +use std::collections::HashMap; + +use actix_web::{web, HttpResponse}; +use serde::{Deserialize, Serialize}; + +use crate::database::Database; +use crate::error::Error; + +const MAX_RETRIES: u32 = 3; + +#[derive(Debug, Serialize, Deserialize)] +pub struct User { + pub id: i32, + pub name: String, +} + +// ... rest of module +``` + +## File size limits + +**Target:** 200–300 lines. **Maximum:** 500 lines. Split larger files by responsibility into separate modules. + +## Group related code together + +Keep related functions, types, and constants close to each other. + +## Comments: why, not what + +**DON'T:** +```rust +// Increment counter +counter += 1; + +// Loop through users +for user in users { ... } +``` + +**DO:** +```rust +// Account for zero-indexed array in user-facing display +counter += 1; + +// Process only active users to avoid triggering suspended account emails +for user in users.iter().filter(|u| u.is_active()) { ... } +``` + +## Document all public APIs + +```rust +/// Fetches a user by their unique identifier. +/// +/// # Arguments +/// * `user_id` - The unique user identifier +/// +/// # Returns +/// * `Ok(User)` - The user if found +/// * `Err(Error::NotFound)` - If no user exists with that ID +/// * `Err(Error::Database)` - If database connection fails +/// +/// # Example +/// ``` +/// let user = get_user(42)?; +/// println!("Found user: {}", user.name); // Display trait — human-readable +/// ``` +pub async fn get_user(user_id: i32) -> Result { + // implementation +} +``` + +## Remove dead code + +Don't comment out code. Delete it. Version control remembers. + +## Make errors explicit and specific + +**DON'T:** +```rust +fn get_user(id: i32) -> Option // Why None? Not found? DB error? + +enum Error { + Failed, // What failed? +} +``` + +**DO:** +```rust +fn get_user(user_id: i32) -> Result + +#[derive(Debug, thiserror::Error)] +enum Error { + #[error("User {0} not found")] + UserNotFound(i32), + + #[error("Database connection failed: {0}")] + DatabaseConnection(String), + + #[error("Invalid user data: {0}")] + InvalidData(String), +} +``` + +## Provide context in error messages + +```rust +database + .fetch_user(user_id) + .await + .map_err(|error| Error::Database( + format!("Failed to fetch user {user_id} from database: {error}") + ))? +``` + +## Testing + +### Test names should be descriptive sentences + +**DON'T:** +```rust +#[test] +fn test1() { ... } + +#[test] +fn test_user() { ... } + +#[test] +fn auth() { ... } +``` + +**DO:** +```rust +#[test] +fn authenticated_user_can_access_protected_endpoint() { ... } + +#[test] +fn unauthenticated_request_returns_401() { ... } + +#[test] +fn deleted_user_cannot_login() { ... } +``` + +### One logical assert per test + +Focus each test on a single behavior. + +**DON'T:** +```rust +#[test] +fn test_user() { + let user = create_user(); + assert!(user.is_valid()); + assert_eq!(user.email, "test@example.com"); + assert!(user.can_login()); + assert_eq!(user.role, Role::User); +} +``` + +**DO:** +```rust +#[test] +fn newly_created_user_is_valid() { + let user = create_user(); + assert!(user.is_valid()); +} + +#[test] +fn newly_created_user_has_correct_email() { + let user = create_user(); + assert_eq!(user.email, "test@example.com"); +} +``` + +### Use descriptive test data + +**DON'T:** +```rust +let user = User { name: "a", age: 1, email: "b" }; +``` + +**DO:** +```rust +let admin_user = User { + name: "Admin Smith", + age: 35, + email: "admin@example.com", +}; +``` + +## Cognitive load reduction + +### Chunk related information + +Group related variables, functions, and logic together. The brain processes chunks, not individual items. + +### Use whitespace strategically + +Separate logical blocks with blank lines to create visual chunks. + +```rust +// Group 1: Fetch and validate +let user = get_user(user_id)?; +validate_user(&user)?; + +// Group 2: Calculate and update +let new_balance = calculate_balance(&user)?; +update_balance(user_id, new_balance)?; + +// Group 3: Notify and log +send_notification(&user)?; +log_balance_update(user_id, new_balance); +``` + +### Maintain consistent patterns + +If you solve a problem one way, solve similar problems the same way throughout the codebase. + +### Avoid surprising behavior + +Functions should do exactly what their names suggest, nothing more, nothing less. + +## Anti-patterns to avoid + +### God objects/functions +No single function/struct should do everything. + +### Magic numbers +```rust +// DON'T +if retries > 3 { ... } + +// DO +const MAX_RETRIES: u32 = 3; +if retries > MAX_RETRIES { ... } +``` + +### Premature optimization +Make it work, make it right, then make it fast. + +### Copy-paste programming +Extract common logic into reusable functions. + +### Clever code +If it makes you feel smart, it's probably too clever. Simplify. + +**DON'T:** +```rust +let result = items.iter().fold(HashMap::new(), |mut acc, x| { + *acc.entry(x.category).or_insert(vec![]).push(x); acc +}); +``` + +**DO:** +```rust +let mut items_by_category = HashMap::new(); +for item in items { + items_by_category + .entry(item.category) + .or_default() + .push(item); +} +``` diff --git a/.claude/skills/rust-code-style/references/rust-idioms.md b/.claude/skills/rust-code-style/references/rust-idioms.md new file mode 100644 index 0000000..f19ae50 --- /dev/null +++ b/.claude/skills/rust-code-style/references/rust-idioms.md @@ -0,0 +1,273 @@ +# Rust-Specific Guidelines + +## Contents +- [Prefer explicit types in signatures](#prefer-explicit-types-in-signatures) +- [Use type aliases for complex types](#use-type-aliases-for-complex-types) +- [Leverage the newtype pattern](#leverage-the-newtype-pattern) +- [Use enums for state](#use-enums-for-state) +- [Avoid unwrap() and expect() in production code](#avoid-unwrap-and-expect-in-production-code) +- [Use descriptive error context](#use-descriptive-error-context) +- [Pattern matching should be exhaustive](#pattern-matching-should-be-exhaustive) +- [Use #[must_use] for important results](#use-must_use-for-important-results) +- [Prefer iterators over index loops](#prefer-iterators-over-index-loops) +- [Use ? instead of manual match](#use--instead-of-manual-match) +- [Use let-else for early returns with destructuring](#use-let-else-for-early-returns-with-destructuring) +- [Use LazyLock and OnceLock for lazy statics](#use-lazylock-and-oncelock-for-lazy-statics) +- [Use NonZero to make invalid numeric states unrepresentable](#use-nonzerot-to-make-invalid-numeric-states-unrepresentable) +- [Use #[non_exhaustive] for public enums in libraries](#use-non_exhaustive-for-public-enums-in-libraries) + +## Prefer explicit types in signatures + +Prefer `async fn` over returning `impl Future` or `BoxFuture` — it's clearer and avoids unnecessary boxing. Reserve `BoxFuture` for trait objects or when you must erase the future type across an API boundary. + +When using `impl Trait` in return position with captured lifetimes (Rust 1.82+), use the `use<>` precise-capturing syntax to be explicit about what lifetimes the opaque type captures. + +**DON'T:** +```rust +pub fn process(data: impl Serialize) -> BoxFuture<'static, Result> +``` + +**DO:** +```rust +pub async fn process(data: RequestData) -> Result + +// When returning impl Trait with lifetimes (1.82+) +fn active_users<'a>(&'a self) -> impl Iterator + use<'a> { ... } +``` + +## Use type aliases for complex types + +**DON'T:** +```rust +fn handler() -> Pin> + Send>> +``` + +**DO:** +```rust +type HandlerFuture = Pin> + Send>>; + +fn handler() -> HandlerFuture +``` + +## Leverage the newtype pattern + +Make invalid states unrepresentable. + +**DON'T:** +```rust +fn send_email(email: String) -> Result<()> +// Any string can be passed, even invalid emails +``` + +**DO:** +```rust +struct ValidatedEmail(String); + +impl ValidatedEmail { + fn new(email: String) -> Result { + // validate email format + Ok(Self(email)) + } +} + +fn send_email(email: ValidatedEmail) -> Result<()> +// Only validated emails can be passed +``` + +## Use enums for state + +**DON'T:** +```rust +struct User { + status: String, // "active", "suspended", "deleted"? + deleted_at: Option, + suspension_reason: Option, +} +``` + +**DO:** +```rust +enum UserStatus { + Active, + Suspended { reason: String, until: DateTime }, + Deleted { deleted_at: DateTime }, +} + +struct User { + status: UserStatus, +} +``` + +## Avoid unwrap() and expect() in production code + +**DON'T:** +```rust +let user = db.get_user(id).unwrap(); +let config = load_config().expect("config must exist"); +``` + +**DO:** +```rust +let user = db.get_user(id)?; +let config = load_config() + .map_err(|e| Error::Config(format!("Failed to load config: {e}")))?; +``` + +## Use descriptive error context + +**DON'T:** +```rust +db.fetch_user(id).map_err(|e| Error::Database(e))? +``` + +**DO:** +```rust +db.fetch_user(id) + .map_err(|e| Error::Database(format!("Failed to fetch user {id}: {e}")))? +``` + +## Pattern matching should be exhaustive + +**DON'T:** +```rust +match status { + Status::Active => process(), + _ => {} // Silent failure for all other cases +} +``` + +**DO:** +```rust +match status { + Status::Active => process(), + Status::Suspended => handle_suspended(), + Status::Deleted => Err(Error::UserDeleted), +} +``` + +## Use #[must_use] for important results + +```rust +#[must_use = "transaction must be committed or rolled back"] +pub struct Transaction { ... } + +#[must_use = "iterator is lazy and does nothing unless consumed"] +pub fn process_items(&self) -> impl Iterator +``` + +## Prefer iterators over index loops + +**DON'T:** +```rust +for i in 0..items.len() { + process(&items[i]); +} +``` + +**DO:** +```rust +for item in &items { + process(item); +} +``` + +## Use ? instead of manual match + +**DON'T:** +```rust +let user = match db.get_user(id) { + Ok(u) => u, + Err(e) => return Err(e.into()), +}; +``` + +**DO:** +```rust +let user = db.get_user(id)?; +``` + +Note: `?` relies on `From` for error conversion. Implement `From for YourError` to enable propagation across error types without manual `.map_err()`. + +## Use let-else for early returns with destructuring + +`let-else` (stable since 1.65) is the idiomatic guard clause when you need to destructure and return early if the pattern doesn't match. It keeps the happy path at the top level without nesting. + +**DON'T:** +```rust +fn process(event: Option) -> Result<()> { + if let Some(event) = event { + // happy path deeply nested + handle(event)?; + } + Ok(()) +} +``` + +**DO:** +```rust +fn process(event: Option) -> Result<()> { + let Some(event) = event else { + return Ok(()); + }; + handle(event) +} +``` + +Also useful for enum variants: +```rust +let Status::Active { since } = user.status else { + return Err(Error::UserNotActive); +}; +``` + +## Use LazyLock and OnceLock for lazy statics + +Since Rust 1.80, `std::sync::LazyLock` and `std::sync::OnceLock` are stable in std — prefer them over the `once_cell` or `lazy_static` crates. + +```rust +// LazyLock: value computed on first access +static CONFIG: std::sync::LazyLock = + std::sync::LazyLock::new(|| Config::load().expect("config must be valid at startup")); + +// OnceLock: value set exactly once at runtime +static FLAG: std::sync::OnceLock = std::sync::OnceLock::new(); + +fn is_enabled() -> bool { + *FLAG.get_or_init(|| std::env::var("FEATURE").is_ok()) +} +``` + +## Use NonZero\ to make invalid numeric states unrepresentable + +`std::num::NonZero` (unified type since 1.79, previously `NonZeroU32` etc.) encodes the constraint in the type system, eliminating runtime checks. + +```rust +use std::num::NonZero; + +// DON'T +fn set_page_size(size: u32) { + assert!(size > 0, "page size must be non-zero"); + // ... +} + +// DO +fn set_page_size(size: NonZero) { + // guaranteed non-zero by the type +} +``` + +## Use #[non_exhaustive] for public enums in libraries + +Mark public enums `#[non_exhaustive]` when you may add variants in future versions. This forces downstream callers to include a wildcard arm, preventing breakage when you extend the enum. + +```rust +#[non_exhaustive] +#[derive(Debug)] +pub enum Error { + NotFound, + PermissionDenied, + // future variants won't break downstream match expressions +} +``` + +Do **not** use `#[non_exhaustive]` on internal enums — exhaustive matching is a feature there, catching unhandled cases at compile time. diff --git a/.claude/skills/rust-code-style/references/yew-best-practices.md b/.claude/skills/rust-code-style/references/yew-best-practices.md new file mode 100644 index 0000000..42f967e --- /dev/null +++ b/.claude/skills/rust-code-style/references/yew-best-practices.md @@ -0,0 +1,298 @@ +# Yew Framework Best Practices + +This project is a **Yew 0.23** WASM single-page app (struct components, `yew-router` 0.20, `ybc` +Bulma widgets, `gloo-*`, `reqwasm`). These guidelines extend the Rust style rules with framework +conventions specific to Yew. They are grounded in this codebase's existing patterns: struct +components with a `Msg` enum, `Properties` carrying `Callback`s, and async work driven through +`ctx.link().send_future(...)`. + +> **Convention first:** this codebase uses **struct components** uniformly. Match that for changes to +> existing components. Reach for **function components + hooks** only for genuinely new, self-contained +> widgets (see the last section) — do not rewrite working struct components into hooks. + +## Contents +- [Keep `view` pure — no side effects, no panics](#keep-view-pure) +- [Drive side effects from messages, never from `changed()`](#side-effects-from-messages) +- [Always give keyed list items a stable `key`](#keyed-lists) +- [Don't panic in WASM — route failures through `Msg::Error`](#dont-panic) +- [Clone deliberately, not reflexively](#clone-deliberately) +- [Properties: minimal, `PartialEq`, `Rc` for big payloads](#properties) +- [Load initial data in `create`/`rendered`, not `view`](#initial-load) +- [Callbacks: build in `view`/`create`, keep handlers thin](#callbacks) +- [Prefer shared context over scattered storage reads](#shared-context) +- [Routing with yew-router](#routing) +- [When to use function components + hooks](#function-components) + +## Keep `view` pure {#keep-view-pure} + +`view` runs on every re-render. It must be a pure function of `self` + `ctx.props()`: build `Html`, +nothing else. No network calls, no storage writes, no panics, no expensive computation. + +**DON'T** — panics inside `view` (a single bad row blanks the whole page): +```rust +impl Admins { + fn is_row_selected(&self, id: u64) -> Classes { + let admin = self.list.iter().filter(|a| a.id == id).next().unwrap(); // panics if absent + if admin.selected { classes!("is-selected") } else { classes!("") } + } +} +``` + +**DO** — total functions, no unwrap, use iterator adapters with names: +```rust +fn row_class(&self, admin_id: u64) -> Classes { + let is_selected = self.list.iter().any(|admin| admin.id == admin_id && admin.selected); + if is_selected { classes!("is-selected") } else { Classes::new() } +} +``` + +Extract complex markup into helper methods returning `Html` (e.g. `fn render_row(&self, ...) -> Html`) +to keep `view` scannable. + +## Drive side effects from messages, never from `changed()` {#side-effects-from-messages} + +`changed()` exists to reconcile new props into local state and report whether a re-render is needed. +Performing I/O, showing dialogs, or firing network requests there is an anti-pattern: `changed()` is +called on **every** parent re-render and prop change, so the effect fires unpredictably and repeatedly. + +**DON'T** — a DELETE request + a blocking prompt run inside `changed()`: +```rust +fn changed(&mut self, ctx: &Context, _old: &Self::Properties) -> bool { + if ctx.props().on_delete.is_some() { + if let Some(reason) = gloo_dialogs::prompt("Reason...", None) { + ctx.link().send_future(async move { /* DELETE ... */ }); // fires on any prop change + } + } + true +} +``` + +**DO** — reconcile props only; trigger the effect from an explicit message: +```rust +fn changed(&mut self, ctx: &Context, _old: &Self::Properties) -> bool { + let endpoint_changed = ctx.props().end_point != self.endpoint; + if endpoint_changed { + self.endpoint = ctx.props().end_point.clone(); + ctx.link().send_message(Msg::Loading); + } + endpoint_changed +} + +// the delete is its own message, raised by the button's onclick: +Msg::DeleteRequested => { + let Some(reason) = gloo_dialogs::prompt("Reason...", None).filter(|r| !r.trim().is_empty()) + else { return false; }; + ctx.link().send_future(async move { /* DELETE ... */ }); + false +} +``` + +Rule of thumb: **a user action → a `Msg` → an effect.** Props changing should at most schedule a +`Msg`, never perform the work directly. + +## Always give keyed list items a stable `key` {#keyed-lists} + +When rendering a `Vec` into rows/cards with `.map()`, attach a `key` that is stable across renders +(a uuid or db id — never the array index). Without keys, Yew falls back to positional diffing: +edits/deletes can reattach state to the wrong element and re-render more than necessary. + +**DON'T:** +```rust +let rows: Html = self.list.iter().map(|admin| html! { + /* ... */ +}).collect(); +``` + +**DO:** +```rust +let rows: Html = self.list.iter().map(|admin| html! { + /* ... */ +}).collect(); +``` + +## Don't panic in WASM — route failures through `Msg::Error` {#dont-panic} + +A panic in WASM aborts the module and leaves a blank page with only a console trace — there is no +unwinding and no recovery. The "avoid `unwrap()`/`expect()`" rule from `rust-idioms.md` is therefore +**hard** in components. Two recurring offenders here: + +- `gloo_storage::SessionStorage::get("jwt").unwrap()` at the top of nearly every async block — panics + if the user is logged out / the key is missing. +- Unwrapping the response `Authorization` header (including inside the `bastion_resp!` macro) — panics + on any 2xx response that doesn't carry the header. + +**DO** — fall back to a recoverable `Msg::Error` (and a logoff where auth is missing): +```rust +let jwt = match SessionStorage::get::("jwt") { + Ok(token) => token, + Err(_) => return Msg::Logoff, +}; +// for the macro: treat a missing Authorization header as Err, not a panic. +``` + +Selecting `.unwrap()`-free patterns here also matters because WASM panics are invisible to most +end users — they just see a broken screen. + +## Clone deliberately, not reflexively {#clone-deliberately} + +`.clone()` is sometimes unavoidable in Yew (moving owned data into `async move` blocks and `Callback` +closures). But high clone density (some files here clone 30–50×) usually signals reflexive cloning in +`view` and handlers that re-clones large structs every render. + +- Clone **only** the fields you actually move into an `async move`/closure, not the whole struct. +- For data shared across components or stored in props, wrap it in `Rc` (or `Rc<[T]>`) so a clone is + a refcount bump, not a deep copy. `Rc` also makes `Properties` `PartialEq` cheap. +- Borrow in `view` where the value is only read; clone at the point of capture. + +**DON'T:** +```rust +let whole_admin = self.admin_for_config.as_ref().unwrap().clone(); // clones everything +let uuid = whole_admin.uuid; +``` +**DO:** +```rust +let admin_uuid = self.admin_for_config.as_ref().map(|admin| admin.uuid.clone()); +let Some(admin_uuid) = admin_uuid else { return false; }; +``` + +## Properties: minimal, `PartialEq`, `Rc` for big payloads {#properties} + +- Every `Properties` struct must derive `PartialEq` (it does here) — Yew uses it to skip re-rendering + children whose props are unchanged. Don't break it by storing non-comparable or always-changing + fields. +- Keep props **small**: pass identifiers + `Callback`s, not large owned collections. If a child needs + a big list, pass `Rc>` so equality is a pointer compare and clones are cheap. +- Name callback props for the event they signal (`on_delete`, `on_selection`, `on_update`) and emit + domain data, not UI noise. + +## Load initial data in `create`/`rendered`, not `view` {#initial-load} + +Kick off the first fetch from `create` (via `ctx.link().send_message(Msg::Loading)`) or from +`rendered(first_render)` guarded by `if first_render`. Never start loads from `view` (it re-runs on +every render → request storms). This codebase already does this correctly — keep it. + +```rust +fn rendered(&mut self, ctx: &Context, first_render: bool) { + if first_render { + ctx.link().send_message(Msg::Loading); + } +} +``` + +## Callbacks: build in `view`/`create`, keep handlers thin {#callbacks} + +- Create callbacks with `ctx.link().callback(...)` / `callback_future(...)`. Capturing per-row values + (e.g. an id) into the closure is fine. +- Keep `update` arms short and single-purpose (same ≤25-line / single-responsibility rule). Extract a + fetch into a private `async fn` or helper that returns the next `Msg`, rather than inlining a long + request builder in every arm. +- Prefer `send_message`/`send_future` over `wasm_bindgen_futures::spawn_local` inside components, so + results flow back as messages through the normal update loop. + +## Prefer shared context over scattered storage reads {#shared-context} + +Reading `jwt`/`puuid` from `SessionStorage` at the top of every async block couples each component to +global storage and multiplies the panic surface. For shared session state, prefer a Yew +`ContextProvider` consumed via `ctx.link().context(...)` (struct components) or +`use_context` (function components), or a state crate (`yewdux`). Centralizes the read, makes the +dependency explicit in props/context, and gives one place to handle "not logged in". + +## Logging: use the `log` facade, not `gloo_console` directly {#logging} + +The app initializes `wasm_logger` in `src/bin/app.rs`, which bridges the `log` crate to the browser +console with level filtering. **Log through the `log` facade** (`log::error!`, `log::warn!`, +`log::info!`, `log::debug!`) — not `gloo_console::log!`/`error!` directly. The facade gives one place +to set verbosity (raise to `warn` for production via `wasm_logger::Config::new(log::Level::Warn)`), +consistent formatting, and the option to swap backends later. + +- **Levels:** `error!` for failures the user is told about; `warn!` for handled-but-notable + (HTTP non-2xx, missing selection); `info!` for coarse lifecycle (logged in/out); `debug!` for + developer tracing. Don't log at `info`/`error` for routine flow. +- **No commented-out logs.** Delete `// console::log!(...)` lines — they are dead code. If a trace is + worth keeping, make it a real `log::debug!` (filtered out in production by level). +- **Capture syntax:** `log::warn!("request failed with HTTP {status}")`, not `"{}", status`. +- **Don't log secrets** — never log the JWT, passwords, or secret `value`/`userAttributes` payloads. + +```rust +// DON'T: direct console + dead commented trace + leaks the token +// console::log!("jwt:{}", &jwt); +gloo_console::error!("Parse error in json:{}", err.to_string()); + +// DO: facade, level-appropriate, no secret +log::error!("could not parse response JSON: {error}"); +``` + +## Testing: extract pure logic, run it on the host target {#testing} + +Component lifecycle and `view` can only run under `wasm-bindgen-test` (a browser/node +runtime). So the highest-value, cheapest tests come from **extracting pure logic into free +functions / associated functions** and testing them with plain `#[test]` (host `cargo test`): +selection counts, row-class predicates, grouping/sorting, formatting/truncation, filter builders. +This is a direct payoff of the "keep `view` pure" rule — pure helpers are trivially testable. + +Two gotchas when building fixtures for host tests: +- **Don't call `Model::default()` if its `Default` impl touches `js_sys`/`web_sys`.** Several models + here (e.g. `SymmetricKey`) build a date via `js_sys::Date` in `Default`, which panics on the host + target ("cannot call wasm-bindgen imported functions on non-wasm targets"). Construct the fixture + field-by-field instead, or deserialize it from a JSON literal. +- **Deserialize from a representative server payload** when a struct has many fields — it doubles as a + contract check that the model still parses what the API returns (see the `GenericPage` tests). + +```rust +#[cfg(test)] +mod tests { + use super::*; + fn admins() -> Vec { + let page: AdminPage = serde_json::from_str(SAMPLE_PAGE_JSON).expect("sample parses"); + page.content + } + #[test] + fn count_selected_counts_only_marked() { /* ... */ } +} +``` + +## Routing with yew-router {#routing} + +- Define routes as a `#[derive(Routable)]` enum; render with ` render={...}/>`. +- Navigate with the `Navigator` (`ctx.link().navigator()` / `use_navigator`), and link with + ` to={...}>` — don't hand-build `href`s or poke `window.location` for in-app navigation. +- Keep the route→component mapping in one `switch` function. + +## When to use function components + hooks {#function-components} + +For a **new, self-contained** widget with simple local state, a function component is less ceremony: + +```rust +#[function_component(KeyPicker)] +fn key_picker(props: &KeyPickerProps) -> Html { + let selected = use_state(|| props.default_uuid.clone()); + let on_change = { + let selected = selected.clone(); + Callback::from(move |uuid: String| selected.set(uuid)) + }; + // use_effect_with(deps, ...) for side effects; use_memo for derived data. + html! { /* ... */ } +} +``` + +Guidance: +- Use `use_effect_with(deps, ...)` (not bare `use_effect`) so effects re-run only when `deps` change — + the hook equivalent of "don't fire side effects on every render". +- `use_state`/`use_reducer` for local state, `use_memo` for derived values, `use_callback` for stable + callbacks, `use_context` for shared session state. +- **Do not** convert the existing 35 struct components to hooks as part of unrelated work — that's a + separate, deliberate migration. Match the struct-component convention when editing them. + +## Yew review checklist + +- [ ] `view` is pure: no I/O, no storage writes, no `unwrap()`/panics, no heavy computation +- [ ] No side effects (fetch/dialog/storage) inside `changed()` — only prop→state reconciliation (+ optional `send_message`) +- [ ] Every `.map()`-rendered list element has a stable `key` (uuid/id, not index) +- [ ] No `unwrap()`/`expect()` on `SessionStorage`, response headers, or JSON in components — route to `Msg::Error`/`Logoff` +- [ ] `.clone()` is limited to values genuinely moved into closures/`async move`; large shared data uses `Rc` +- [ ] `Properties` derive `PartialEq`, stay small, and pass `Callback`s/ids (or `Rc<…>`) not big owned collections +- [ ] Initial loads happen in `create`/`rendered(first_render)`, never in `view` +- [ ] `update` arms are short and single-purpose; long fetches extracted to helpers +- [ ] In-app navigation uses `yew-router` (`Navigator`/`Link`), not `window.location` +- [ ] Logging goes through the `log` facade at an appropriate level (not raw `gloo_console`); no commented-out logs; no secrets logged +- [ ] New self-contained widgets may use function components + hooks (`use_effect_with` for effects); existing struct components are left as struct components diff --git a/.claude/skills/ui-cognitive-simplicity/SKILL.md b/.claude/skills/ui-cognitive-simplicity/SKILL.md new file mode 100644 index 0000000..a80a8fc --- /dev/null +++ b/.claude/skills/ui-cognitive-simplicity/SKILL.md @@ -0,0 +1,130 @@ +--- +name: ui-cognitive-simplicity +description: Reduce operator cognitive load in the Bastion Config UI (Rust/Yew/WASM). Load this BEFORE writing or editing ANY operator-facing Rust/Yew code — component, page, form, flow, error message, wizard, and specifically before ADDING a panel, notice, badge, chip, banner or button to a page that already has some. Applies even when the request is framed as a feature ("add a teardown ladder"), a backend follow-on ("surface the new field"), a bug fix, or "just add X" — and even if it never says UX, UI, simplicity or cognitive load. Also use when deciding whether logic belongs in the UI or the Bastion backend, and when reviewing an existing flow — always propose the simpler flow, not merely implement the requested one. If you are about to write html! for anything an operator will read, this skill applies. +--- + +# UI Cognitive Simplicity (Bastion Config, Yew/WASM) + +North star: **an operator should never hold more than a few things in working memory at once.** Human working memory fits roughly 4–6 items ("The Programmer's Brain", Hermans). Bastion's domain — key ceremonies, cert lifecycles, policies, migrations — is intrinsically complex; the UI's job is to eliminate all *extraneous* load so the operator's budget is spent on the decision that actually matters ("rotate now or later?"), not on remembering steps, states, or vocabulary. + +This skill applies the cognitive-complexity reduction method from *Rust Full-Stack Development with AI Pair Programming* (vocabulary preload, schema-first, controlled expansion, explicit pitfall callouts) to UI work. + +## The prime directive: UI renders state, it never derives it + +The single biggest complexity leak is a UI that *computes* what the backend already knows. + +- If a component decides "which buttons are valid for a key in state `PREPARED`", that state machine now lives in two codebases and drifts. The backend must send the current state **and the allowed next actions**; the UI maps them to buttons. Nothing else. +- If the UI must chain more than one API call to fulfil one operator intent ("rotate cert" = issue + install + verify), that is a backend defect. Do not build a client-side orchestration. Propose a single ceremony endpoint to the Bastion repo (see its `rest-api-cognitive-simplicity` skill) and keep the UI to: one click → one request → one streamed/polled status. +- Client-side validation may *mirror* server rules for fast feedback, but the server response is the source of truth. Never encode policy (allowed algorithms, rotation windows, RBAC) in the UI. + +Why: every rule moved server-side is a rule the operator (and the UI maintainer) no longer has to remember, and a rule that can't silently drift. + +## One journey per screen — the panel budget + +A page answers **one** question: "what, if anything, must I do here?" Every +element competing to answer it costs the reader a decision before they reach the +answer. + +- **Count the panels a page renders in its HEALTHY steady state. The target is + zero.** A finished state ("Attach complete") is a fact, not an instruction — + put it in the header line as a chip. Status that needs nobody (a current + revocation list, a version that matches) is a chip too. +- **The advice slot always means somebody must act.** The moment it also carries + congratulations or routine status, the reader learns to skim it, and the one + time it matters they skim past that too. +- **Never render advice for a journey the operator has not started.** A teardown + ladder shown to everyone advertises "Step 1 of 3" of something nobody is + doing, beside a panel saying the opposite. Destructive flows are ENTERED — + the destructive button opens the checklist, and a second press performs it. +- **Two panels disagreeing in mood is a defect**, not a layout issue: a green + "you are done" beside a blue "step 1 of 3" beside a red alarm makes the + reader's first task working out which one is about them. + +Observed 2026-08-04: a healthy PG deployment rendered exactly those three at +once, one of them added by a change that never loaded this skill. + +## Flow audit — run this before and after any flow change + +Count three numbers for the operator's path: + +1. **Steps** — clicks/screens from intent to done. +2. **Memories** — things they must carry between steps (an ID, a filename, "which state was it in?", a value copied from another screen). +3. **Decisions** — questions the UI asks them. + +A change is a simplification only if it lowers at least one number without raising the others. When asked to modify an existing flow, report these numbers for current vs. proposed — even when not asked. Proposing the better flow is in scope by default in this project. + +For every input field and decision, ask in order (stop at the first yes): +1. Can the server **discover** it? (it already knows the domain, the key, the last-used value) → remove the field. +2. Can it be **defaulted**? → default it, tuck the override under "Advanced". +3. Can it be **derived** from something already entered? → derive it, show it read-only. +4. Can it be **postponed** until actually needed? → move it out of the main path. +Only then may it stay as a question. + +## Yew/Rust specifics — make the compiler carry the load + +Rust's type system is a cognitive aid: complexity encoded in types is complexity the reader no longer simulates in their head. + +- **Model async screens as one enum, not booleans.** `is_loading` + `error: Option` + `data: Option` creates 8 theoretical states of which 4 are impossible — the reader must prove that to themselves every time. Instead: + + ```rust + enum Remote { Idle, Loading, Loaded(T), Failed(ApiError) } + ``` + + One `match` renders it; the compiler guarantees every state has a face. Same pattern for ceremonies: `enum RotateFlow { PickTarget, Confirming(Target), Streaming(Progress), Done(Summary), Failed(ApiError) }`. +- **Routes stay a typed enum** (`#[derive(Routable)]`) — never build URLs from strings. Missing pages become compile errors, and the enum is the site map (a schema the reader can load in one glance). +- **One component = one concept.** If you cannot name a component without "And" (`KeyListAndRotationPanel`), split it. Props read-only, state owned, messages up — the book's "rooms and mailboxes" model. A component whose `html!` needs scrolling is two components. +- **Name states with domain vocabulary, verbatim from the backend enums** (`PREPARED`, `ACTIVE`, `REVOKED`). The same noun everywhere — UI label, Rust variant, API field — is vocabulary preload: the operator learns the term once and it works on every screen, in every error, in the docs. +- Keep `spawn_local` blocks tiny: fetch → set state. Business logic in async blocks inside components is unfindable later. + +## UX rules for this product (recent best practices, applied) + +- **Schema-first screens.** Every screen answers within one second: where am I (domain, entity), what state is it in, what can I do next. For lifecycle entities (keys, certs) show the state machine visually — current state highlighted, possible transitions as the actual buttons. The diagram *is* the controls. +- **Progressive disclosure (controlled expansion).** First render shows the happy path only. Advanced options (algo overrides, custom validity, raw config) live behind one "Advanced" expander, collapsed by default. Never present 12 fields when 2 are required. +- **Recognition over recall.** Pickers over free-text IDs, recent values, last-used defaults. If the operator has to paste a UUID from another screen, the flow failed the audit above. +- **Visibility of long operations.** Migrations and rotations stream progress (SSE). Render structured phases ("Connecting → Applying changelog → Done"), not a raw log; keep the raw log behind an expander for debugging. Announce completion via `aria-live` — the operator will tab away. +- **Errors: what happened, why, what to do next** — all three, in operator vocabulary, with the failing field highlighted in place. Show all invalid fields at once, not first-failure-only. A dead-end error ("400 Bad Request") is a bug; if the server response lacks remediation, file it against the API. +- **Undo beats confirmation** for anything reversible. Reserve confirmation for genuinely irreversible ceremonies (key `destroy`, cert `revoke`) and make it informative: state exactly what stops working ("3 services currently decrypt with this key"). Type-to-confirm only for the truly destructive. +- **Empty states teach.** A domain with no keys shows the one next action and one sentence of why — not a blank table. +- **No optimistic UI for crypto state.** Ceremonies render server-confirmed state only; a spinner is honest, a premature "ACTIVE" badge is a lie the operator acts on. + +## Poka-yoke: the operator never fills a form the server will refuse + +Every flow and ceremony where a human can make a mistake is mistake-proofed. Use the highest rung that fits: + +Strongest first (Shingo); use the highest rung that fits, and keep the last one always: + +1. **Eliminate** -- make the wrong state unrepresentable. Precedent: the attach ticket, one redeemable value instead of two the operator had to match. +2. **Prevent** -- the wrong action is not offered. The read that precedes the action carries `allowedActions` plus `disabledReasons`/`whyNot`, so the button is disabled WITH its reason before the operator invests effort. +3. **Warn at the source** -- an action that creates a condition a LATER flow will fail on says so in its own confirmation ("after this, nobody in domain X can approve JIT sessions"). +4. **Detect early** -- shape rules checked as the operator types. +5. **Detect late** -- the refusal at submit. Always present (state can change between read and write), but never the FIRST time the operator learns something the server already knew. + +In the UI that means: + +- **Render `whyNot` / `disabledReasons` before the first input.** If the read behind a screen says an action is unavailable, its button is disabled and the reason (with its next step and who to ask) is visible where the button is -- before a dialog opens, not after Submit. If the server does not send it, the UI does NOT compute it: file the gap against the API (`rest-api-cognitive-simplicity`, "Poka-yoke"). +- **Confirmations of actions that break later flows say so**, in the server's words ("after this, nobody in domain X can approve JIT sessions"). +- **A refusal is shown where the operator is looking**: `FormError` brings itself into view, marked fields scroll into view (`first_problem`), and a long dialog body shows that more is hidden (ybc scroll shadow, BASTION-108). +- Flow audit: count "steps until the operator learns it cannot work". Anything above 1 for a condition the server knew is a defect. +- Found 2026-10-05 (BASTION-109 -> 111): "no eligible approver" arrived after the DBA had filled duration, reason and ticket. + +## Cognitive pitfalls — call these out explicitly in reviews + +The book's practice: naming a confusion point removes its cost. When you review UI code, flag these patterns by name: + +- **State machine duplicated in the UI** ("the backend already knows this — render `allowed_actions` instead"). +- **Boolean explosion** ("model this as one enum, impossible states unrepresentable"). +- **Vocabulary drift** (UI says "remove", API says "destroy", docs say "delete" — pick the API term everywhere). +- **Operator-as-orchestrator** (a runbook step that says "then click X, then go to Y" is a missing backend endpoint). +- **Hidden prerequisite / refusal after effort** (a flow that fails late because of something checkable up front — surface it before step 1, disabled-with-reason, not as an error after step 3; see "Poka-yoke"). +- **Silent cause** (an action whose confirmation does not say which later flow it makes impossible). + +## Flow proposal template + +When proposing a better flow (do this even for working flows you touch): + +``` +Current: N steps / M memories / K decisions — list them +Proposed: n steps / m memories / k decisions — list them +Moved to backend: +Removed questions: +``` diff --git a/.claude/skills/ui-page-modernization/SKILL.md b/.claude/skills/ui-page-modernization/SKILL.md new file mode 100644 index 0000000..a8983c5 --- /dev/null +++ b/.claude/skills/ui-page-modernization/SKILL.md @@ -0,0 +1,47 @@ +--- +name: ui-page-modernization +description: Repeatable per-page loop to harden and modernize the Bastion operator UI (Rust/Yew/WASM) one page at a time — enumerate happy/unhappy cases, mirror the spec's input constraints with shared Rust validation, cover with repeatable Playwright e2e (deterministic seed, idempotent login, X-Trace-Id correlation), then simplify. Use whenever fixing UI defects, adding or changing form validation (min/max/pattern/required), writing or updating Playwright specs, syncing the UI to spec/bastion-api.yaml, or redesigning a page/flow, or ADDING any panel/notice/badge to an existing page — even if the request only says "fix the login page", "clean up this form", or "add X to the deployment page". Pair it with `ui-cognitive-simplicity`, which owns the panel budget and the flow audit; run that one first when the change alters what a page renders. The backend half of this contract is bastion's `validation-error-contract` skill. +--- + +# UI Page Modernization Loop (Bastion Config, Yew/WASM) + +North star: **modernize the UI one page at a time, and never let a page regress once it's green.** Each page is a vertical slice — its input rules, its errors, its tests, and its layout are brought into agreement in a fixed order, then locked behind repeatable Playwright coverage. The loop only moves forward: you do not simplify a page's UX until its validation and tests are green, because the tests are what make the simplification safe. + +Companions in this repo: `ui-cognitive-simplicity` (the *what* of good UX — steps/memories/decisions), `rust-code-style` (Yew/Rust idiom). The server side of the same contract lives in the **bastion** repo's `validation-error-contract` and `rest-api-cognitive-simplicity` skills. This skill is the *process* that drives all of them page by page. + +## The loop — run these seven steps per page, in order + +Do not skip ahead. Each step has an exit gate; if the gate fails, fix it before the next step. + +1. **Enumerate cases.** Write the happy path plus every unhappy path *first*, as a list, before touching code. For each input: empty, below-min, above-max, boundary (min and max exactly), wrong pattern, and server-reject (a value that passes the client but the backend refuses). For login this is: valid creds → dashboard/reset; empty field → dialog; 404 → "combination not found"; 412 → YubiKey path. *Gate: the list exists and names the expected user-visible outcome for each case.* + +2. **Spec is truth.** Encode the field constraints and the error codes in [`spec/bastion-api.yaml`](../../../spec/bastion-api.yaml). If a rule (a min length, a regex, a required field) is not in the spec, it does not exist — add it there before enforcing it anywhere. The spec is the shared contract with the backend; a rule that lives only in `#[validate(...)]` will silently drift. *Gate: every constraint you're about to enforce has a home in the spec.* + +3. **Mirror on the UI — via the shared validation module, never copy-paste.** Enforce the spec's rules with the `validator` crate, but source shared patterns from **one** place. Today the mm/dd/yyyy `DATE_REGEXP` is redefined in `model/symmetric_key.rs`, `model/cert.rs`, and `lib.rs`, and `pg_migration.rs` hand-rolls its own `validate()` — that is the drift this step eliminates. Create/extend a single `src/validation` module that owns the shared regexes and validators; every component imports from it. *Gate: no new bespoke regex or one-off `fn validate`; the component uses the shared module and its `#[validate]` messages match the spec.* + +4. **Hand off to the backend.** Client validation is for *fast feedback only* — the server is the source of truth (see `ui-cognitive-simplicity`: "UI renders state, it never derives it"). Open a matching change in the bastion repo (`validation-error-contract`) so the same constraint is enforced server-side and returns a stable error code. Never encode policy (allowed algorithms, RBAC, rotation windows) in the UI; mirror only shape-level rules (length/pattern/required). *Gate: the backend enforces the rule and the UI maps its error code to a message rather than inventing one.* + +5. **Unify the error surface.** The UI must present one consistent error experience. Map the backend's stable error code → a user-facing message in one place; do not scatter `format!("Error:{}", status)` across components (login.rs still does this). When the backend consolidates its two error shapes (`ApiErrorResponse` vs `GenericErrorMessage`), consume the unified one. *Gate: each unhappy case shows a specific, human message — never a bare status code.* + +6. **Cover with repeatable Playwright.** Tests must be deterministic and re-runnable against the same deployment without hand-resetting state: + - **Selectors:** add `data-testid` to the page's inputs, buttons, and error containers *as part of this step* — this is also what makes step 7's redesign safe. Never select on Bulma classes or label text. + - **Login fixture is state-aware** (the deployment tiers down nightly): try the post-reset password first; on a fresh boot, log in with the initial password, detect the `/reset-pwd` screen, and reset. Assert on *being on the reset screen*, not on which password worked. + - **Correlation:** set `X-Trace-Id: ` via `page.setExtraHTTPHeaders` — the backend's `AuditWebFilter` threads it into the audit/trace record, so a failing case can be joined to the exact backend line. + - **One spec per page**, one `test()` per enumerated case from step 1, each asserting the specific outcome. *Gate: the whole spec passes twice in a row against the same running deployment with no manual reset.* + +7. **Simplify — only now.** With the page green, apply `ui-cognitive-simplicity`: run its flow audit (count steps / memories / decisions), remove or default or derive every field the server already knows, model async screens as one enum, and align to modern form UX (inline field-level errors, disabled-until-valid submit, single primary action). The Playwright snapshot + assertions from step 6 are your regression net — a redesign that changes an asserted outcome or the visual baseline must be intentional. *Gate: at least one of steps/memories/decisions went down, none went up, and the spec from step 6 is still green.* + +## Repeatability rules (non-negotiable) + +- **Deterministic starting state.** Seed via the Bruno `api-walkthrough` golden path (local/ephemeral only). Against the real AWS deployment, keep the suite **read-only** except the one first-boot password reset — never seed or run destructive actions there. +- **No sleeps.** Wait on conditions (element visible, request settled), never `waitForTimeout`. +- **Idempotent.** A spec must pass on a fresh boot *and* on a re-run the same day. If it only passes once, it is not done. +- **Isolated by trace id.** Every run tags its requests so failures are diagnosable after the fact via CloudWatch + the audit trail. + +## Anti-patterns this loop exists to kill + +- A regex or validation rule copy-pasted into a second component instead of imported from `src/validation`. +- A client rule with no spec entry and no backend counterpart (it *will* drift). +- A test that selects on `.button.is-primary` or visible text, or that needs the DB reset by hand between runs. +- Redesigning a page's layout before it has passing e2e coverage — you've removed your own safety net. +- The UI deciding *policy* (what's allowed) rather than *presentation* (how to show what the server allowed). diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0d2d1c9..ee01a37 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -6,37 +6,35 @@ jobs: name: Build runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions-rs/toolchain@v1 + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable with: - toolchain: stable - target: wasm32-unknown-unknown - default: true + targets: wasm32-unknown-unknown components: clippy, rustfmt - - uses: actions-rs/cargo@v1 - with: - command: clippy - - uses: actions-rs/cargo@v1 - with: - command: fmt - args: -- --check - - uses: actions-rs/cargo@v1 - with: - command: build + - name: Run clippy + run: cargo clippy + - name: Check formatting + run: cargo fmt -- --check + - name: Build + run: cargo build + - name: Install nightly for docs + uses: dtolnay/rust-toolchain@nightly + - name: Build docs (all features) + run: RUSTDOCFLAGS="--cfg docsrs" cargo doc --all-features --no-deps example_basic: name: Example | Basic needs: build runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions-rs/toolchain@v1 + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable with: - toolchain: stable - target: wasm32-unknown-unknown - default: true + targets: wasm32-unknown-unknown components: clippy, rustfmt - - name: fetch trunk - run: wget -qO- https://github.com/thedodd/trunk/releases/download/v0.16.0/trunk-x86_64-unknown-linux-gnu.tar.gz | tar -xzf- - - name: build example - run: cd examples/basic && ../../trunk build + - name: Install trunk + uses: taiki-e/install-action@v2 + with: + tool: trunk + - name: Build example + run: cd examples/basic && trunk build diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 5086929..8f3d60a 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -9,14 +9,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Setup | Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup | Rust - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - profile: minimal - override: true + uses: dtolnay/rust-toolchain@stable - name: Build | Publish run: cargo publish --token ${{ secrets.CRATES_IO_TOKEN }} @@ -26,7 +22,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Setup | Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup | Create Release Log run: cat CHANGELOG.md | tail -n +7 | head -n 25 > RELEASE_LOG.md diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml new file mode 100644 index 0000000..000bb2c --- /dev/null +++ b/.github/workflows/rust.yml @@ -0,0 +1,22 @@ +name: Rust + +on: + push: + branches: [ "master" ] + pull_request: + branches: [ "master" ] + +env: + CARGO_TERM_COLOR: always + +jobs: + build: + + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + - name: Build + run: cargo build --verbose + - name: Run tests + run: cargo test --verbose diff --git a/.gitignore b/.gitignore index 3909f2f..ac8a3e4 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,6 @@ target # file ignores Cargo.lock +# Local Claude state stays local; the skills are shared (Kostya, 2026-10-05). +/.claude/* +!/.claude/skills/ diff --git a/CHANGELOG.md b/CHANGELOG.md index cabdaa8..515d701 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,65 @@ This changelog follows the patterns described here: https://keepachangelog.com/e Subheadings to categorize changes are `added, changed, deprecated, removed, fixed, security`. ## Unreleased +### added +- `id: Option` prop on every form control (`Select`, `MultiSelect`, + `Input`, `TextArea`, `Checkbox`, `Radio`, `File`). It is rendered on the + native control (not the Bulma wrapper) and omitted when `None`, so existing + call sites are unaffected. +- `label_for: Option` prop on `Field`, rendered as `