| Commit message (Collapse) | Author | Age |
|---|
| |
|
|
| |
Missed in 9f7f82dbd9adee8ae18ae7ff2600b3e1dc8fadbc.
|
| | |
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
HTTP routes are now defined in a single, unified module, pulling them out of the topical modules they were formerly part of.
This is intended to improve the navigability of the codebase. Previously, finding the handler corresponding to a specific endpoint required prior familiarity, though in practice you could usually guess from topic area. Now, all routes are defined in one place; if you know the path, you can read down the list to find the handler.
Handlers themselves live with the domain they are most appropriately "part of," generally (in this version, universally) in a `handlers` submodule. The handlers themselves have been flattened down; rather than representing a path and a method, they now represent a named operation (which is suspiciously similar to the path in most cases). This means that we no longer have constructs like `crate::ui::routes::ch::channel` - it's now `crate::ui::handlers::channel` instead.
## Disclaimer
I Solemnly Swear I Didn't Change Any Handlers.
## Prior art
I've inadvertently reinvented Django's `urls.py` convention, and I've opted to lean into that.
Merges flatter-routes-reorg into main.
|
| | |
| |
| |
| |
| |
| | |
Each domain module that exposes handlers does so through a `handlers` child module, ideally as a top-level symbol that can be plugged directly into Axum's `MethodRouter`. Modules could make exceptions to this - kill the doctrinaire inside yourself, after all - but none of the API modules that actually exist need such exceptions, and consistency is useful.
The related details of request types, URL types, response types, errors, &c &c are then organized into modules under `handlers`, along with their respective tests.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
HTTP routes are now defined in a single, unified module, pulling them out of the topical modules they were formerly part of.
This is intended to improve the navigability of the codebase. Previously, finding the handler corresponding to a specific endpoint required prior familiarity, though in practice you could usually guess from topic area. Now, all routes are defined in `crate::routes`.
Other than changing visibility, I've avoided making changes to the handlers at the ends of those routes.
|
| |/
|
|
| |
I've replaced it with something more general, which will be applicable no matter how we restructure the routing.
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The two middlewares were identical but for the specific `IntoResponse` impl used to generate the response when setup has not been completed. However, unifying them while still using `from_fn_with_state` lead to this horrorshow:
.route_layer(middleware::from_fn_with_state(
app.clone(),
|state, req, next| {
setup::middeware::setup_required(UNAVAILABLE, state, req, next)
}
))
It's a lot to read, and it surfaces the entire signature of a state-driven middleware `fn` into the call site solely to close over one argument (`UNAVAILABLE`).
Rather than doing that, I've converted this middleware into a full blown Tower middleware, following <https://docs.rs/axum/latest/axum/middleware/index.html#towerservice-and-pinboxdyn-future>. I considered taking this further and implementing a custom future to remove the allocation for `Box::pin`, but honestly, that allocation isn't hurting anyone and this code already got long enough in the translation.
The new API looks like:
.route_layer(setup::Required(app.clone()))
Or like:
.route_layer(setup::Required(app.clone()).with_fallback(RESPONSE))
Part of a broader project to reorganize our routing. This is intended to reduce the number of "why do we have two of these?" questions when the answer isn't very compelling. We had two of these because I needed them to behave slightly differently, and at the time I didn't fully understand how to abstract out that difference. Now I do, thanks to Axum's excellent documentation.
Merges unify-setup-required into main.
|
| | |
| |
| |
| |
| |
| | |
For endpoints that are unavailable, that default behaviour no longer needs to be specified: `Required(app)` will do that for you. For endpoints that are redirects until setup is completed, `Require(app).with_fallback(…response…)` will do that.
To make this a bit harder to break by accident, the default unavailable response is now its own type.
|
| |/
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The two middlewares were identical but for the specific `IntoResponse` impl used to generate the response when setup has not been completed. However, unifying them while still using `from_fn_with_state` lead to this horrorshow:
.route_layer(middleware::from_fn_with_state(
app.clone(),
|state, req, next| {
setup::middeware::setup_required(UNAVAILABLE, state, req, next)
}
))
It's a lot to read, and it surfaces the entire signature of a state-driven middleware `fn` into the call site solely to close over one argument (`UNAVAILABLE`).
Rather than doing that, I've converted this middleware into a full blown Tower middleware, following <https://docs.rs/axum/latest/axum/middleware/index.html#towerservice-and-pinboxdyn-future>. I considered taking this further and implementing a custom future to remove the allocation for `Box::pin`, but honestly, that allocation isn't hurting anyone and this code already got long enough in the translation.
The new API looks like:
.route_layer(setup::Required::or_unavailable(app.clone()))
Or like:
.route_layer(setup::Required::with_fallback(app.clone(), RESPONSE))
One thing I would have liked to have avoided is the additional `app.clone()` argument, but there isn't a way to extract the _state_ from a request inside of an Axum middleware. It has to be passed in externally - that's what `from_fn_with_state` is doing under the hood, as well. Using `State` as an extractor doesn't work; the `State` extractor is special in a _bunch_ of ways, and this is one of them. Other extractors would work. Realistically, I'd probably want to explore interfaces like
.route_layer(setup::Required(app).or_unavailable())
or
.route_layer(app.setup().required().or_unavailable())
|
| |
|
|
|
|
| |
build` fails.
This was missed in 4396c912771f136f7d397a67f247c81532520b85.
|
| |
|
|
|
|
|
|
|
|
|
|
| |
From its documentation:
> This command is similar to `npm install`, except it's meant to be used in automated environments such as test platforms, continuous integration, and deployment -- or any situation where you want to make sure you're doing a clean install of your dependencies.
We don't need a clean install, necessarily - just a complete one that matches the package configuration. However, this command is clearly documented as being used for automated environments, and I think integration with another build tool is close enough to that intention to fit.
It also promises not to rewrite `package.json` or `package-lock.json`. (`npm install`, on the other hand, rewrites `package-lock.json` regularly.) As we do not intend to change the source tree when building it, this is the preferred behaviour.
Finally, this fixes a behaviour I encountered where certain `cargo` commands - sometimes including `cargo build` - could completely reformat `package-lock.json` without any warning and without any user-visible rationale for it.
|
| | |
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| | |
There isn't a corresponding fix script for this as the lockfiles are rebuilt every time you run a command that resolves dependencies, in either `cargo` or `npm`. Furthermore, `cargo build` (and anything else that runs `build.rs`) will implicitly run `npm install` and update `package-lock.json` in the process.
This was originally part of [another proposal][pr-6]. I've broken it out to make the intent clearer, and to make the proposal easier to get a handle on in isolation from other, related changes. Thanks to @wlonk for their input on this!
[pr-6]: https://codeberg.org/ojacobson/pilcrow/pulls/6
Merges prop/lockfile-checks into main.
|
| |/
|
|
| |
There isn't a corresponding fix script for this as the lockfiles are rebuilt every time you run a command that resolves dependencies, in either `cargo` or `npm`. Furthermore, `cargo build` (and anything else that runs `build.rs`) will implicitly run `npm install` and update `package-lock.json` in the process.
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The new `tools/check-lint` script checks lints across _all_ lintable files - JS (through `eslint`), and Rust (through `clippy` and `cargo check`). It also checks `eslint` against the whole project, not just against what's in the `ui` subdir, which means it now catches lintable issues in various JS config files.
This was originally part of [another proposal][pr-6]. I've broken it out to make the intent clearer, and to make the proposal easier to get a handle on in isolation from other, related changes. Thanks to @wlonk for their input on this!
[pr-6]: https://codeberg.org/ojacobson/pilcrow/pulls/6
Merges prop/lint-checks into main.
|
| | |
| |
| |
| | |
This is entirely mechanical; we don't consider coverage reports to be "part of the project," and there's no point in wasting `eslint`'s time (or ours) in reviewing lints there.
|
| |/
|
|
| |
The new `tools/check-lint` script checks lints across _all_ lintable files - JS (through `eslint`), and Rust (through `clippy` and `cargo check`). It also checks `eslint` against the whole project, not just against what's in the `ui` subdir, which means it now catches lintable issues in various JS config files.
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
* Changes JS formatting policy to expect trailing commas in most of the places they're allowed.
* Moves format checking out of `npm` and into tool scripts.
* _Documents_ formatting tools, as well as a shot at a policy for format-only changes designed to alleviate low-value review.
This was originally part of [another proposal][pr-6]. I've broken it out to make the intent clearer, and to make the proposal easier to get a handle on in isolation from other, related changes. Thanks to @wlonk for their input on this!
[pr-6]: https://codeberg.org/ojacobson/pilcrow/pulls/6
Merges prop/project-formatting into main.
|
| | |
| |
| |
| | |
These are ephemeral, and should not even be _considered_ by `prettier`.
|
| | |
| |
| |
| | |
This is purely a stylistic preference. However, it has helped keep diffs smaller (since diffs at the end of an object or array don't require changing the last line of the array), and it's something a lot of tools and IDEs now default to expecting to.
|
| | | |
|
| | |
| |
| |
| |
| |
| | |
The new `tools/check-format` script checks _all_ project formatting - JS (through `prettier`), and Rust (through `rustfmt`). It also checks `prettier` against the whole project, not just against what's in the `ui` subdir, which means it now catches formatting issues in various JS config files (like `.prettierrc` itself).
This commit does not include style _fixes_, which means that it does not pass its own `tools/check-format` script. This is intentional, and is intended to make the Git history a bit easier to reason about; a future commit will include format fixes.
|
| |/
|
|
|
| |
* It no longer mentions files that do not exist in this project.
* It now _does_ mention files that do exist, that `prettier` should never touch, like sqlx's query- metadata JSON files, `package-lock.json`, or our bundled copies of Mermaid.
|
| |\
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The added suppression for `manual_ok_err` is a mixed choice; I'd prefer `r.ok()` in most senses, but `BroadcastStream` is still new enough that I wouldn't be entirely surprised if the Tokio team added new error variants, that we'd want to expressly handle.
I do feel a bit better suppressing individual [`clippy::pedantic`][pedantic] lints; they're allow-by-default for this reason anyways, and I opted into them (see 452c8d0d9edb9894c108b6d577806c7c9d0071dd) knowing that not all of them would be perfectly appropriate.
[pedantic]: https://doc.rust-lang.org/clippy/
Merges prop/missed-lints into main.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
The added suppression for `manual_ok_err` is a mixed choice; I'd prefer `r.ok()` in most senses, but `BroadcastStream` is still new enough that I wouldn't be entirely surprised if the Tokio team added new error variants, that we'd want to expressly handle.
I do feel a bit better suppressing individual [`clippy::pedantic`][pedantic] lints; they're allow-by-default for this reason anyways, and I opted into them (see 452c8d0d9edb9894c108b6d577806c7c9d0071dd) knowing that not all of them would be perfectly appropriate.
[pedantic]: https://doc.rust-lang.org/clippy/
|
| |\ \
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
I've opted to run with `--coverage` to ensure that we continue exercising coverage support. Hat tip to @wlonk for holding me accountable on this - I had thought coverage was broken, but I was holding it wrong.
Also adjusts the code coverage failure thresholds to match here-and-now reality. I'm not offering a policy thought here, just making sure we
1. use the coverage checking we have, and
2. set standards we are actually achieving.
This was originally part of [another proposal][pr-6]. I've broken it out to make the intent clearer, and to make the proposal easier to get a handle on in isolation from other, related changes. Thanks to @wlonk for their input on this!
[pr-6]: https://codeberg.org/ojacobson/pilcrow/pulls/6
Merges prop/test-tool into main.
|
| | | |
| | |
| | |
| | | |
These values were drawn from `npx vitest --run --coverage` as of this commit. I offer no opinion on whether they are _desireable_ coverage thresholds, only that they are what we are actually achieving, and what we are accepting in practice.
|
| | |/
| |
| |
| | |
I've opted to run with `--coverage` to ensure that we continue exercising coverage support. Hat tip to @wlonk for holding me accountable on this - I had thought coverage was broken, but I was holding it wrong.
|
| |\ \
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
Calling through `npm` wasn't adding anything other than complexity, and it made it somewhat harder to follow what tools did what.
I'm also pretty sure `tools/build-ui` was totally unused.
This was originally part of [another proposal][pr-6]. I've broken it out to make the intent clearer, and to make the proposal easier to get a handle on in isolation from other, related changes. Thanks to @wlonk for their input on this!
[pr-6]: https://codeberg.org/ojacobson/pilcrow/pulls/6
Merges prop/build-without-npm into main.
|
| | |/
| |
| |
| |
| |
| | |
Calling through `npm` wasn't adding anything other than complexity, and it made it somewhat harder to follow what tools did what.
I'm also pretty sure `tools/build-ui` was totally unused.
|
| |\ \
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
This has a couple of material consequences:
* It will be (much) easier to reorganize the source tree, as the path to the output is no longer relative to where the config files are when building the final binary. If we do decide to move `ui` into its own child crate, we won't have to make a bunch of (very similar) changes to the Svelte build process at that time.
* There is less chance of a stale build contaminating a new one, since changes to the crate change the project hash in `OUT_DIR`. For example, while working on this change, `OUT_DIR` was at various points:
* `target/debug/build/pilcrow-7cfeef3536ddd3e7/out`
* `target/debug/build/pilcrow-09d4ddbc12bef36b/out`
* `target/release/build/pilcrow-070d373bd5f850a1`
This may use more space on disk, but it's all reclaimable with `cargo clean` and Rust is _far_ more profligate with disk space than Svelte will ever be.
* It's more consistent with Cargo's expectations around generated source files, and thus potentially easier to onboard Rust developers into.
Merges prop/ui-build-to-cargo-output into main.
|
| | |/
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This has a couple of material consequences:
* It will be (much) easier to reorganize the source tree, as the path to the output is no longer relative to where the config files are when building the final binary. If we do decide to move `ui` into its own child crate, we won't have to make a bunch of (very similar) changes to the Svelte build process at that time.
* There is less chance of a stale build contaminating a new one, since changes to the crate change the project hash in `OUT_DIR`. For example, while working on this change, `OUT_DIR` was at various points:
* `target/debug/build/pilcrow-7cfeef3536ddd3e7/out`
* `target/debug/build/pilcrow-09d4ddbc12bef36b/out`
* `target/release/build/pilcrow-070d373bd5f850a1`
This may use more space on disk, but it's all reclaimable with `cargo clean` and Rust is _far_ more profligate with disk space than Svelte will ever be.
* It's more consistent with Cargo's expectations around generated source files, and thus potentially easier to onboard Rust developers into.
|
| |\ \
| | |
| | |
| | |
| | |
| | | |
from prop/merge-template into main
Reviewed-on: https://codeberg.org/ojacobson/pilcrow/pulls/14
|
| | |/
| |
| |
| |
| |
| |
| |
| | |
For a merge with a single incoming commit, this will generally be redundant - the default pull request subject and body are drawn from the commit itself, so they'll get repeated in the merge commit. However, for a pull request with multiple commits, the pull request title and body are likely a better merge commit message than the default offered by Codeberg.
I've been doing basically this for manual merges regardless.
Further reading: <https://forgejo.org/docs/latest/user/merge-message-templates/>
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
This allows developers (hi) to run tools installed via NPM without having to run `npm exec` or `npx` to do so. It's mostly for ergonomics, but it's nice to be able to run `vite build` and have it "just" work.
The "canonical" invocations, used in tool scripts or when `build.rs` has to run Node, uses `npx`. More generally, we don't assume direnv, even though the source tree provides it.
There is no corresponding `layout rust` in `direnv`, but the same niche is fulfilled by the `PATH_add` targeting Cargo's output directory. I also considered using `PATH_add` instead of `layout` for this, as they do the same thing (at least as of this version of `direnv`), but I think it's probably more appropriate to use the parts of direnv that express the broader intention to use Node's tools than to manually implement them myself.
If you're using `direnv` across this change, you'll need to re-run `direnv allow`.
|
| |/
|
|
|
|
| |
This was a simple omission on my part. In f26dd0d662d8fc33108d072031329e707f54300b, I added a `.envrc` file that was intended to be tracked, and had it reference `.envrc.local` so that others could customize the environment, through direnv, without requiring that those changes be submitted upstream.
I forgor to mark that file as ignored. It never bit us, but I tried to use a .envrc.local file today to experiment on some Node-specific PATH ideas (I want to be able to run `vite` without `npx`!) and noticed that Git wanted to track my config.
|
| |\
| |
| |
| | |
Notably, one of them was hiding a real (if unreachable) bug, by converting a "the token you have presented is not valid because the user was deleted" scenario into an internal server error, when it should have been an authorization error.
|
| | |
| |
| |
| | |
Notably, one of them was hiding a real (if unreachable) bug, by converting a "the token you have presented is not valid" scenario into an internal server error.
|
| |\ \
| |/
|/|
| |
| |
| |
| |
| | |
Tests were inadvertently broken in 96d363fd9290d43d2e6a11e2e5269fb8ccf6d65d (probably in 9f0b5b00f7ada4c5735230d0f93e04a3c58d4d7a).
The tests found a potentially-real accessibility problem! Great success.
Unfortunately, the switch to an editable div also broke the tests completely. I've marked them as skipped rather than removing them, out of optimism at the underlying bug being fixed one day.
|
| | |
| |
| |
| | |
We … can't test this, I think, because of a bug in `user-event`. Maybe there's an alternative that directly manipulates the DOM, but I'd prefer not to do that.
|
| |/
|
|
| |
The hidden `textarea` used to attach the form value to the DOM was being included in the ARIA accessibility tree, at least in testing (I didn't check in a browser). While we could suppress this iwth `aria-role="hidden"`, the WHATWG recommendation is to Not Do That, and to find another way to hide the element, instead. Marking the element as hidden accomplishes that goal, _and_ gets rid of a style rule.
|
| |\ |
|
| | | |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
In ae93188f0f4f36086622636ba9ae4810cbd1f8c9, `remote.channels.all` became a flat array of channels, instead of a map, in order to simplify some of the reasoning around how state changes propagate. However, I neglected to remove all Map-shaped calls referring to it.
This lead to some pretty interesting behaviour:
* The client could not track unread state, because reconciling local state against the remote state would find no remote state, then throw away local state entirely as a result.
* The client would not actually update when a new channel appeared.
* The client would not actually update when a channel disappeared.
|
| |\| |
|
| | |
| |
| |
| |
| |
| | |
vanished.
This may happen if the user has a link to a channel open when the channel is deleted/expires, or if they return to the app after the last channel they looked at has expired.
|
| | |
| |
| |
| | |
`session`.
|
| | |
| |
| |
| | |
channel's creation time.
|
| | |
| |
| |
| | |
been read.
|
| | | |
|
