Rule Authoring UI - #490
Open
ThisIsMissEm wants to merge 6 commits into
Open
ThisIsMissEm wants to merge 6 commits into
ThisIsMissEm wants to merge 6 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+43
to
+55
| import { | ||
| CONDITION_OPERATOR_OPTIONS, | ||
| Condition, | ||
| ConditionOperator, | ||
| EMPTY_BUILDER_MODEL, | ||
| Outcome, | ||
| OutcomeArg, | ||
| RuleBuilderModel, | ||
| SML_IDENTIFIER_RE, | ||
| applyMissingImports, | ||
| generateSmlFromBuilder, | ||
| outcomeArgsForEffect, | ||
| } from './ruleBuilderSml'; |
`.appWrapper` set `min-height: 100vh`, which never gives the layout a definite height -- a percentage height resolves to `auto` unless its parent's height is definite, and `min-height` does not make it so. Every `height: 100%` beneath it was therefore inert: `#root`, `.viewContainer` in a dozen views, and every `max-height: 100%` meant to bound an inner scroll region. The page scrolled as one document and the inner scroll areas simply grew. The rest of the layout already assumes the bound: `.mainColumn` sets `min-width: 0`, `.contentWrapper` sets `min-height: 0`, and the view roots set `height: 100%` with their own overflow. Those only matter when something is being constrained. `100vh` rather than `100%` because `body` carries only `min-height`, so a percentage would hit the same dead end one level up. Two consequences needed handling. `.contentWrapper` gets `overflow-y: auto` so a view that does not manage its own scrolling is not clipped by the new bound -- `auto` rather than `hidden`, so the bound is not a trap. And `.topBar` and `.sidebarHeader` get `flex: 0 0 auto`: `height` alone is only a basis for a flex item, and both sat next to a `flex: 1 1 auto` sibling that could squeeze them once the column stopped growing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
`osprey_ui` is a browser-only program, but tsconfig set no `types` allowlist, so TypeScript swept `node_modules/@types` and pulled in `@types/node`. That put `Buffer`, `__dirname` and Node's timer overloads in scope for application code, where none of them exist at runtime. The visible symptom was `setTimeout` resolving to Node's overload and returning `NodeJS.Timeout` rather than a number, which the codebase has worked around in two different ways -- some call sites use `window.setTimeout` to force the DOM overload, others do not. The underlying hazard is larger: nothing stopped a `Buffer` or `process.cwd()` from typechecking in a bundle that has neither. `"types": []` only disables *automatic* inclusion of global type packages; anything reached through `import` is unaffected, so React and friends are untouched. `process.env` still resolves, because `src/env.d.ts` references `@rsbuild/core/types`, which declares it for the browser build. `rsbuild.config.ts` is genuinely Node code but was never in this program -- `include` is `["src"]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
The wire shapes and the calls that fetch them, ahead of anything rendering them.
Responses are snake_case throughout, with no exceptions -- including the Rule
Builder models, which once carried camelCase aliases and were the single
departure from that. Converting is the client's job now, so `Raw*` types describe
what arrives and `RulesActions` maps them to the camelCase view models, mirroring
how `RawUIConfig` is already handled.
Two shapes rather than one for a rules table row. `RuleDraftSummary` is what the
list serves; `RuleRecord` extends it with the SML and is served when one draft is
the subject. Extending rather than duplicating means anything needing only summary
fields can be typed to the narrower shape and still accept a full record.
`id` is a string, not a number: Osprey mints ids as snowflakes in places, and a
64-bit id exceeds JavaScript's exact integer range, so parsing one would silently
drop its low bits. `cid` is a content address of the SML, which is what lets the
server later answer whether a deployed file still matches the draft that produced
it without transferring the source twice.
Refusals come back in two shapes and the client distinguishes them: 409 and 422
carry the same `DraftValidation` envelope validation uses, so a rejected save can
be rendered through the validation panel rather than as an opaque message, while
404/503 carry `{error: ...}`.
`getDeployPlan` passes `validateStatus: () => true` so an absent optional endpoint
resolves rather than rejecting -- the shared interceptor files every failure in a
global error store that an unrelated page renders, and a 404 for a build that
predates the endpoint is not an error anyone should see.
`GET /config` now reports three flags: whether the deployment can deploy at all,
and whether this user may edit or deploy. They are separate because they want
different UI -- one hides a control, the others disable it with a reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
The registry listed the engine's loaded rules and nothing about work in flight, so a draft someone had staged was invisible until they told you about it. A banner above the list carries the rows that are not yet live, ordered by how much they want attention: a draft whose author has marked it ready sorts and counts ahead of work still in progress. Deployed rows are excluded rather than listed last -- they are already in the registry below, and showing them here puts the same rule on the page twice, the second time under a heading that contradicts its status. The Edit link now opens the draft when one exists for a rule. Drafts are upserted by path, so an editor opened on the deployed file would have replaced whatever was staged there the moment it saved, with nothing on screen to say so. Rows with an edit underway are marked, so the list says which rules someone is already changing. Authoring is gated on CAN_EDIT_RULES. "Add rule" is disabled with a reason rather than hidden -- it is the page's one prominent authoring control, so it is the right place to explain the ability is missing. The per-row Edit buttons are hidden instead: repeating that explanation on every row is noise once the button has given it. Status colour tracks whether a row wants someone's attention rather than whether it is good, so the one awaiting deployment is the one that stands out and a live rule is neutral. The draft path is set in the app's own link colour, which measured 7.08:1 where antd's default measured 3.35:1, and is distinguished by its monospace family rather than an underline -- rule paths are full of underscores, and an underline lands exactly where they do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
Deploying is the only action in the UI that changes what the engine runs, and it was not offered at all -- a draft could be staged and then only shipped by hand. `DeployButton` decides which action a user gets. Without a rules directory there is no deploy story, so nothing renders. Without CAN_DEPLOY_RULES the user gets "Request deployment" rather than a disabled Deploy: that ability is a standing property of the person, so a greyed-out button would occupy the position of the action forever without ever becoming usable. Requesting has no confirmation and no typed name -- it is reversible, changes nothing live, and is idempotent, so the friction that guards a deploy would be ceremony. `DeployModal` confirms the deploy, and renders a plan rather than describing one. `GET /rules/drafts/<id>/deploy-plan` reports what the deploy would actually do, so each row states a fact -- "overwrites what is on disk", "already required" -- instead of the conditional prose a client can only guess at. The wiring checkbox rewrites the main.sml row as it is toggled, so its consequence is shown rather than explained, and the plan answers for both wiring choices at once so toggling re-renders instead of re-fetching. The modal is mounted only while open, so opening is its own reset and a dismissed attempt leaves nothing behind. Confirming requires typing the rule name, and the plan is held on screen for a beat before the button unlocks: this is the irreversible step, and it should read as considered rather than quick. One headline carries the severity, computed from the plan so it cannot contradict the rows beneath it -- reassuring when nothing would change, a warning only when the rule will actually start running, and merely informational when the file is written but never loaded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
Authoring a rule meant editing SML on disk. This adds an editor that stages one as a draft, on two routes: `?draftId=` opens a stored draft, `?path=` opens a rule the engine has loaded. Query params rather than path segments because rule paths contain slashes and react-router v5 has no clean repeating-segment param. Two axes, deliberately separate. Edit and Preview are what the page is *for* right now -- changing the rule, or reading back what would be saved. Builder and Code are which surface does the changing, and sit on the editing card because both are ways of editing. Preview shows the whole record, metadata included, which is what keeps it distinct from the Code editor rather than a read-only copy of it. The Rule Builder generates SML from a model, so it never reproduces stored text byte for byte. Dirtiness is therefore measured against what the editor first rendered rather than against the stored source -- otherwise an untouched draft opened in the Builder reads as modified the moment it loads. All three saved fields are baselined, not just the source, so dirtiness never depends on whether a stored row happens to exist: keying off that made a `?path=` edit permanently dirty and lit up Save about a second after load on a page nobody had touched. A `?path=` editor is identified by file path, not by row, so once it saved it could not tell its own new draft from a stranger's. Reload reopened the file, warned about "an edit in progress", and the next save replaced the row it had just made -- a loop one person reaches by doing nothing unusual. It now adopts an existing draft at that path, the same rule the registry's Edit link follows. The parse that decides whether the Builder can represent a file is deferred unless the editor is about to open in it, removing a request and a serialised round trip from every code-first open. Validation is debounced to a second and aborts a superseded request rather than discarding its reply, since validating means assembling the engine's whole source set server-side. The header sits outside the scrolling region rather than being sticky: sticky would need an opaque background, a z-index, and negative margins to cover the gutters, and the side panel below would have to offset against a header height that changes with the title. Beneath it the editor and side panel share one scroll area, so the scrollbar lands on the window edge rather than in the seam between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rr2ChMJq3ozimojhVq7faG
ThisIsMissEm
force-pushed
the
emelia/rule-drafts-ui
branch
from
September 2, 2026 23:46
8b3b12f to
0df941f
Compare
julietshen
marked this pull request as ready for review
September 3, 2026 15:00
julietshen
requested review from
a team,
EXBreder,
ayubun and
haileyok
as code owners
September 3, 2026 15:00
2 tasks
4 tasks
Contributor
|
Tested 0df941f, some comments; When using argument `delay_action_by` to `LabelAdd` has incompatible type
has type `int`, expected `TimeDeltaT | None`
rules/more_rule.sml:18:58while argument `apply_if` to `LabelAdd` has incompatible type
has type `int`, expected `RuleT | None`
rules/more_rule.sml:18:51The UI doesn't offer a way to create these either, it would be great if these were handled, could be as more options to fill for each argument, or a helper description that shows how to fill in these values which is then used to create the right argument in the API. When adding a label as an effect, the
After saving a draft - also happens with deployed rules-, the edit page for the rule becomes strictly SML, and that defeats the goal of the Rule Authoring UI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is the UI side of the work in #489, it was originally the #403 branch, but I've ended up heavily reworking the UI to hopefully feel better.
@julietshen if you're wanting to check out a branch to take for a spin, then it's this branch. You can add a
osprey_worker/src/osprey/worker/lib/acls/dev_acl_assignments.jsonto give you two users to test with:Checklist
uv run ruff check .passes (no unused imports or other lint errors)uv tool run fawltydeps --check-unused --pyenv .venvpasses (no unused dependencies)CHANGELOG.mdwith my changes, if notable (refer to Keep a Changelog conventions)