Add missing documentation, break up large UI components, simplify/cleanup

[dabrahams]

It's a large PR but mostly trivial transformations. The individual steps can easily be broken into separate PRs if that will help.

[camio]

I left minor tweaks and suggestions, but it otherwise looks good to me. I especially like how the CLAUDE.md is developing.

[camio]

Note the recommended message style documentation for .expect. The argument should describe the reason you think this should work okay.

[dabrahams]

But I can't say that it should work okay; it's simply that the app is not in a viable environment for running if it's unable to build its menus. I have no idea why it might fail or whether it can plausibly happen, but any error is unrecoverable.

[camio]

You just stated the reason you think it should work okay. Put that in the expect clause and it reads better IMO:

Suggested change

	.expect("Failed to append separator to File menu");
	.expect("GUI subsystem operational");

You are saying you expect that the GUI subsystem is operational. That's all that needs to be said. There's no need to give additional context (e.g. that you happen to be adding a separator to the file menu).

[camio]

You removed MAIN_CSS in Desktop's main.rs. Should the same not be done here?

[camio]

Suggested change

	let window = window()
	.ok_or_else(|| anyhow::anyhow!("Failed to get window object - browser API unavailable"))?;
	let document = window
	.document()
	.ok_or_else(|| anyhow::anyhow!("Failed to get document object - browser API unavailable"))?;
	.ok_or_else(|| anyhow!("Failed to get window object - browser API unavailable"))?;
	let document = window.document().ok_or_else(|| {
	anyhow!("Failed to get document object - browser API unavailable")
	})?;
	let window = window().context("Failed to get window object - browser API unavailable")?;
	let document = window.document().context("Failed to get document object - browser API unavailable")?;

[dabrahams]

.context doesn't work in these cases because they are Options, not Results, but these should use expect, so I'll fix that.

[camio]

The documentation for Context shows a trait implementation for Option. But, yeah, expect is the better choice here.

[camio]

Suggested change

	.map_err(|_| anyhow!("Failed to create Blob from content"))?;
	.context("Failed to create Blob from content")?;

I'll stop making the suggestions. You get the idea.

[dabrahams]

Would be great if it worked. That Result type doesn't have a .context method. I'm adding this module in order to be able to do it. Whether it's a good idea, I dunno.

use anyhow::Result;

/// Anyhow's `context` and `with_context` methods for other result types.
trait AnyhowContext<T, E> {
    /// Returns `self`, adding `context` if `self` is an `Err`.
    fn context<C>(self, context: C) -> Result<T, Error>
    where
        C: Display + Send + Sync + 'static;

    /// Returns `self`, adding the result of `makeContext()` if `self`
    /// is an `Err`.
    fn with_context<C, F>(self, makeContext: F) -> Result<T, Error>
    where
        C: Display + Send + Sync + 'static,
        F: FnOnce() -> C;
}

/// Anyhow's `context` and `with_context` methods for `std::Result`.
impl<T, E> AnyhowContext<T, E> for std::Result<T, E> {
    fn context<C>(self, context: C) -> Result<T, Error>
    where
        C: Display + Send + Sync + 'static,
    {
        anyhow!(self).context(context)
    }

    fn with_context<C, F>(self, f: F) -> Result<T, Error>
    where
        C: Display + Send + Sync + 'static,
        F: FnOnce() -> C,
    {
        anyhow!(self).with_context(f)
    }
}

[dabrahams]

Actually, it's QUITE a bit more complicated than that due to JSValue not being Error-conforming, nor Send/Sync. Try it yourself. I'm not doing this.

[camio]

JSValue isn't Send/Sync for good reasons, but we can convert it into serde_json::Value which is. Something like this works:

#[derive(Debug)]
struct JsonEncodedError(serde_json::Value);

impl fmt::Display for JsonEncodedError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{}", self.0)
    }
}

impl std::error::Error for JsonEncodedError {}

impl From<JsValue> for JsonEncodedError {
    fn from(v: JsValue) -> Self {
        JsonEncodedError(
            serde_wasm_bindgen::from_value(v)
                .expect("JsValue to serde_json::Value conversion always succeeds"),
        )
    }
}

impl From<serde_json::Value> for JsonEncodedError {
    fn from(v: serde_json::Value) -> Self {
        JsonEncodedError(v)
    }
}

And then the user code would be something like this:

    let blob = Blob::new_with_str_sequence(&array)
        .map_err(Into::<JsonEncodedError>::into)
        .context("Failed to create Blob from content")?;

I think it would be a good idea to do this, otherwise we're swallowing potentially useful error information.

[camio]

Suggested change

	let path = file_path(filename);
	fs::remove_file(&path).with_context(|| format!("Failed to delete file '{filename}'"))
	fs::remove_file(&file_path(filename)).with_context(|| format!("Failed to delete file '{filename}'"))

[camio]

Suggested change

	let storage_dir = storage_directory();
	let mut files = collect_json_files_from_dir(&storage_dir)?;
	let mut files = collect_json_files_from_dir(&storage_directory())?;

These one off variables are made in several places. Inlining them makes the code more concise without loss of clarity.

[dabrahams]

I loathe that pattern; blame the AI. I'll try to get rid of them.

[dabrahams]

Suggested change

const MAIN_CSS: Asset = asset!("/assets/main.css");