Repository navigation
fix(ui-web): drop a stale import row, and stop shortening a path chip's target - #615
Conversation
b18cb1c to
c286d5e
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The two requested fixes are sound as written. The prose chip now carries the exact absolute workspace path through to the /file route, whose resolver handles absolute paths without rebasing them onto the session working directory. The onboarding wiring also keeps the cross-domain dependency at the app seam while ensuring wizard-started imports are followed by the import-sync store.
Nonblocking follow-up: watched is page-lifetime state rather than run-specific. If a tab follows one run, then a distinct CLI run settles while that tab remains open, a later reconnect or wizard-close refresh can treat the CLI result as watched and draw its receipt. This requires the prior web run, the external run, and a later refresh in the same tab, so it does not hold this cleanup.
Coverage: reviewed the full diff, surrounding callers, both branch commits and relevant history, backward-compatibility behavior, test changes, AGENTS.md and Web UI context terms, and the source/seam/store architecture constraints. I found no weakened tests or out-of-scope architecture violation.
Verification:
npm test -- --run src/app/install.test.ts src/features/importSync/ImportSyncPage.test.tsx src/features/importSync/store.test.ts src/features/workspace/source.test.ts: 4 files, 72 tests passed.npm test: 188 files, 2,474 tests passed.npm run type-check: passed.npm run lint: zero errors; four existing warnings in untouched files.git diff --check github/main...HEAD: passed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
This is a patch-equivalent rebase of the two reviewed PR commits onto current main; git range-diff reports both commits unchanged, and the current PR diff still contains only the same eight Web UI files. The updated base introduces no conflict or behavioral interaction with the import-store and path-resolution changes. No new inline findings were added; the nonblocking follow-up already recorded on the prior revision remains the only caveat.
Coverage for this revision: checked the replay delta and full PR diff, commit history, affected callers, backward compatibility, test changes, AGENTS.md and Web UI context terms, and the source/seam/store architecture constraints. The tests remain intact and the replay adds no project-rule violation.
Verification:
git range-diff 18de3d745^..b18cb1ce54dd 894618b51^..c286d5e1d: both commits patch-equivalent.npm test -- --run src/app/install.test.ts src/features/importSync/ImportSyncPage.test.tsx src/features/importSync/store.test.ts src/features/workspace/source.test.ts: 4 files, 72 tests passed.npm test: 188 files, 2,474 tests passed.npm run type-check: passed.npm run lint: zero errors; four warnings in untouched files.git diff --check github/main...HEAD: passed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The new per-run watching/followed transition resolves the previously recorded page-lifetime issue. The added regression exercises the long-lived-tab sequence directly, and successful starts, discovered running imports, polling to settlement, stop/resume, dismissal, and replacement by a later settled run remain coherent. The path-chip half is unchanged.
Nonblocking follow-up: start() sets watching before import.run answers. If the gateway refuses without an active run (for example, nothing to import) and an older settled status remains on disk, the immediate refresh() converts that older status into followed. A stale receipt can then appear after the wizard is exited. This needs an old state file, a non-running refusal, and the same tab's later wizard exit, so it is a narrow edge case rather than a reason to hold this cleanup.
Coverage: reviewed the new delta and full PR diff, affected callers and backend refusal semantics, history, backward compatibility, test changes, AGENTS.md and Web UI context terms, and the source/seam/store architecture constraints. No tests were weakened.
Verification:
npm test -- --run src/app/install.test.ts src/features/importSync/ImportSyncPage.test.tsx src/features/importSync/store.test.ts src/features/workspace/source.test.ts: 4 files, 73 tests passed.npm test: 188 files, 2,475 tests passed.npm run type-check: passed.npm run lint: zero errors; four warnings in untouched files.git diff --check github/main...HEAD: passed.
…eipt The row above Settings was drawn from the importer's state file alone. That file outlives every run that wrote it, so any install whose file is not empty greeted its next reader with "memory import finished", whether or not that reader had ever started an import or seen the onboarding wizard. A run the CLI started records no request, so its failure count carries no retry either: the row said "1 did not make it" and the click was dead. A settled run, finished or stopped short, now earns a row only when one of two things holds: its click can still act on it, or this page followed the run, by asking for one or by finding one already in flight. Everything live (the scan, the run, the wrapping phases) is unchanged. The wizard's data-sync step now asks for its run through the same store verb the rail's own click uses instead of calling import.run a second way, so a run started from the wizard is one the page is following. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
A backticked path naming the workspace became a chip whose target was that
path made relative to the workspace. The viewer's route used to root a
relative path at agent home, which made it resolve; it roots one at the
session's working directory now (_workspace_root), so a workspace path outside
that directory was re-rooted onto it. Measured on a live gateway, with the
file present under the configured workspace:
GET /file?path=skills/quick-summarize/SKILL.md&session=s
-> 404 <launch-dir>/skills/quick-summarize/SKILL.md
GET /file?path=<home>/workspace/skills/quick-summarize/SKILL.md&session=s
-> 200
The chip carried the first form, so it opened onto "that file is no longer
there" for a file that was there. It carries the path as the answer wrote it
now. The other half of livePathOf, a file this session changed, has always
answered with the path as written; shortening for display is the panel's own
job (shortPath), not this resolver's.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
…ed in Review follow-up on the previous commit. The marker that said "this page followed the run" was page-lifetime state, so a tab that followed one import and stayed open would draw the receipt of the next run somebody settled from the CLI: the flag was set once and never cleared, and any later settled status inherited it. It is per run now. A run in flight is `watching`; the read that finds it settled turns that into `followed`, the run's own signature, which is the same identity the dismissal already uses. A settled row is drawn when its click can still act on it or when it IS that run. A page outlives a run, so the marker cannot belong to the page. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
514f759 to
4f19708
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
This revision is a patch-equivalent replay of all three reviewed PR commits onto d50a13c1c, the current target tip. After fetching github/main, the PR diff contains only the same eight Web UI files; the apparent Python test delta was the newly merged base commit. No new finding or interaction was introduced, and the nonblocking follow-up already recorded on the prior revision remains the only caveat.
Coverage: checked the replay delta and full PR diff, current target history, affected callers, backward compatibility, test integrity, AGENTS.md and Web UI context terms, and the source/seam/store architecture constraints.
Verification:
git range-diff 894618b51^..514f7594e 19051db5d^..4f1970842: all three commits patch-equivalent.npm test -- --run src/app/install.test.ts src/features/importSync/ImportSyncPage.test.tsx src/features/importSync/store.test.ts src/features/workspace/source.test.ts: 4 files, 73 tests passed.npm run type-check: passed.npm run lint: zero errors; four warnings in untouched files.git diff --check github/main...HEAD: passed.
Review follow-up. `start` marked the page as watching before `import.run`
answered, so a refusal ("nothing to import") left the mark set and the read
that follows adopted whatever settled run was already on file as this page's
own. The wizard's sync step could then be exited into a receipt for an import
somebody else ran.
The mark is set only once the gateway says a run started. A refusal changes
nothing, which is what a refusal means.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The refused-start transition now marks a run as watching only when import.run reports that it started. The subsequent status read still adopts an independently running import, so the fix removes the stale-receipt path without regressing the existing-running case. The new regression test covers the exact settled-state refusal sequence. The previously recorded follow-up is resolved, and no new finding remains.
Coverage: reviewed the new delta and full PR diff, start/refusal/status race paths, affected callers and backend result semantics, history, backward compatibility, test integrity, AGENTS.md and Web UI context terms, and the source/seam/store architecture constraints. No tests were weakened.
Verification:
npm test -- --run src/app/install.test.ts src/features/importSync/ImportSyncPage.test.tsx src/features/importSync/store.test.ts src/features/workspace/source.test.ts: 4 files, 74 tests passed.npm run type-check: passed.npm run lint: zero errors; four warnings in untouched files.git diff --check github/main...HEAD: passed.
|
Not a blocker -- accepting. One nit, and an account of the two things I went after hardest and could not break. NIT. WHAT I CHASED AND DROPPED, because the reasoning is worth more than the verdict: (1) I expected An absolute path is used as-is; only a relative one is rooted at the session workspace. So the change is right and its stated reason is the real one: under the old form, The near-miss worth recording: (2) I expected the WHAT I RAN, in the PR's own tree merged onto the tip, with ui-web's own lockfile ( MUTATIONS, one per commit, because four fixes deserve four biting tests and passing suites prove nothing on their own: The last two are the ones I care about: each reverts to a version that is still plausible, and each kills exactly the test whose name is that commit's subject. That is the shape a fix's test should have. ALSO CHECKED AND SOUND: the |
…'s target (#615) ## Summary Two independent defects a tester hit on a fresh install, both in the page the architecture branch just brought onto main. They are unrelated root causes and sit in one commit each; the PR is one because they are both one-domain fixes to the same page and share a verification run. **1. The rail's import row greeted readers who never imported anything.** The row above Settings was drawn from the importer's state file alone. That file outlives every run that wrote it (`ImportState`: "Entries outlive the run that wrote them"), so any install whose file is not empty showed "memory import finished" to its next reader, whether or not that reader had ever started an import or seen the onboarding wizard. A run the CLI started records no request, so `resumable` is false and the failure count has no retry behind it: the row said "1 did not make it", the click was dead, and the only affordance was the dismiss cross, remembered per browser in localStorage. The rule now: a settled run, finished or stopped short, earns a row only when its click can still act on it, or this page followed the run (asked for one, or found one already in flight). Everything live is unchanged. The wizard's data-sync step now asks for its run through the same store verb the rail's own click uses, instead of calling `import.run` a second way, so a run started from the wizard is one the page is following. Deliberate behaviour change, called out for review: a stopped run that cannot be resumed used to draw a permanently un-clickable "paused" row. It still draws on a page that followed the run; it no longer draws on a page that only found it in the file. Same disease as the finished row (visible, out of reach). Say so if you want that one kept and I will narrow the guard to the finished row. **2. A backticked workspace path opened onto "that file is no longer there".** `livePathOf` turned a path naming the workspace into that path made relative to the workspace, and the chip carried the relative form. The viewer's route used to root a relative path at agent home, which made it resolve; it roots one at the session's working directory now (`_workspace_root`), so a workspace path outside that directory was re-rooted onto the wrong parent. The chip now carries the path as the answer wrote it. The other half of the same function, a file this session changed, has always answered with the path as written, and shortening for display is the panel's own job (`shortPath`). This one predates the rebuilt frontend: the same line shipped in the old `ui-web/src/live/170-workspace.js`. **3. Review follow-up: the follow marker belongs to a run, not to a page.** The first commit's marker was page-lifetime state, so a tab that followed one import and stayed open would draw the receipt of the next run somebody settled from the CLI: set once, never cleared, inherited by any later settled status. It is per run now. A run in flight is `watching`; the read that finds it settled turns that into `followed`, the run's own signature, which is the same identity the dismissal already uses. A settled row draws when its click can act on it or when it IS that run. **4. Review follow-up: a refused import is not a run to follow.** `start` marked the page as watching before `import.run` answered, so a refusal ("nothing to import") left the mark set and the read that follows adopted whatever settled run was already on file. The mark is set only once the gateway says a run started. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Commands and results, all from this branch at `d1a7ef1c2`: ``` ui-web $ npx vitest run Test Files 188 passed (188) Tests 2476 passed (2476) ui-web $ npx tsc --noEmit -p tsconfig.json -> exit 0 ui-web $ npx eslint . -> exit 0 (0 errors, 4 pre-existing warnings) $ npx commitlint --from github/main --to HEAD --config commitlint.config.cjs -> exit 0 $ python scripts/check_commit_messages.py github/main..HEAD -> exit 0 $ python scripts/check_source_language.py github/main..HEAD -> exit 0 $ python scripts/check_large_files.py github/main..HEAD -> exit 0 ``` Reverse verification: neutralising the `stale` guard turns exactly the six new gate cases red, including the one for a later run this page did not follow; marking a refused run as watched again turns the refusal case red; reverting the wizard wiring turns its own case red; reverting the one-line path change turns the three updated resolver cases red. Restored and hash-compared each time. Real-host run, both fixes, on a gateway started against an isolated `RAVEN_HOME` seeded with a leftover import (2 sources, 1 failed, no request recorded) and a real file under the configured workspace. Same gateway, only the built page swapped, `index.html` hashes compared each swap. Import row: | page | rail | | --- | --- | | main (`52eb09b25`) | `importSync-done importSync-warn :: memory import finished / 1 did not make it`, retry button `disabled: true` | | this branch | no row; the rest of the rail draws (re-run after the review follow-up, same result) | | this branch, after adding `tier` + `platforms` to the same leftover | `importSync-paused :: paused / 100%`, button enabled, labelled "resume" | The third row is the control: it proves the page was connected and reading `import.status`, so the empty rail in row two is a decision and not a dead page. Path chip, measured over the live route from the authenticated page, file present at `<home>/workspace/skills/quick-summarize/SKILL.md`: ``` GET /file?path=skills/quick-summarize/SKILL.md&session=s -> 404 <launch-dir>/skills/quick-summarize/SKILL.md GET /file?path=<home>/workspace/skills/quick-summarize/SKILL.md&session=s -> 200 ``` The first form is what the chip carried; the second is what it carries now. Note the same relative request answers 200 with no session, which is why this never showed up in a check that had no conversation open. Not verified end to end: rendering a chip on the real page needs a model turn, so the frontend half is covered by the resolver's own tests and the route half by the measurement above, each read at the layer that owns it. Base history: the branch now sits on `d50a13c1c`, which carries #613 (the lint and contract drift that made `main` red at `52eb09b25`) and #618 (the `test_subagent_probe.py` case that was the one job still red here). Both were `main`'s own failures, reproduced on `main`'s own runs at the same bases, and nothing in this PR touches Python. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk User-visible changes, both in the page only. No contract, no backend, no persisted format. - The rail's import row disappears for a settled run this page did not follow. A reader who relied on the leftover row to notice a months-old failed import loses that notice; `raven import status` still reports it, with the per-item reasons the row never carried. - A path chip's target changes from workspace-relative to as-written. A chip that used to resolve only because the session happened to run in the workspace resolves the same way now, since the route resolves an absolute path without re-rooting. Rollback: revert either commit on its own. They touch disjoint files. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Summary
Two independent defects a tester hit on a fresh install, both in the page the
architecture branch just brought onto main. They are unrelated root causes and
sit in one commit each; the PR is one because they are both one-domain fixes to
the same page and share a verification run.
1. The rail's import row greeted readers who never imported anything.
The row above Settings was drawn from the importer's state file alone. That
file outlives every run that wrote it (
ImportState: "Entries outlive the runthat wrote them"), so any install whose file is not empty showed "memory import
finished" to its next reader, whether or not that reader had ever started an
import or seen the onboarding wizard. A run the CLI started records no request,
so
resumableis false and the failure count has no retry behind it: the rowsaid "1 did not make it", the click was dead, and the only affordance was the
dismiss cross, remembered per browser in localStorage.
The rule now: a settled run, finished or stopped short, earns a row only when
its click can still act on it, or this page followed the run (asked for one, or
found one already in flight). Everything live is unchanged. The wizard's
data-sync step now asks for its run through the same store verb the rail's own
click uses, instead of calling
import.runa second way, so a run started fromthe wizard is one the page is following.
Deliberate behaviour change, called out for review: a stopped run that cannot
be resumed used to draw a permanently un-clickable "paused" row. It still draws
on a page that followed the run; it no longer draws on a page that only found
it in the file. Same disease as the finished row (visible, out of reach). Say
so if you want that one kept and I will narrow the guard to the finished row.
2. A backticked workspace path opened onto "that file is no longer there".
livePathOfturned a path naming the workspace into that path made relative tothe workspace, and the chip carried the relative form. The viewer's route used
to root a relative path at agent home, which made it resolve; it roots one at
the session's working directory now (
_workspace_root), so a workspace pathoutside that directory was re-rooted onto the wrong parent. The chip now
carries the path as the answer wrote it. The other half of the same function, a
file this session changed, has always answered with the path as written, and
shortening for display is the panel's own job (
shortPath).This one predates the rebuilt frontend: the same line shipped in the old
ui-web/src/live/170-workspace.js.3. Review follow-up: the follow marker belongs to a run, not to a page.
The first commit's marker was page-lifetime state, so a tab that followed one
import and stayed open would draw the receipt of the next run somebody settled
from the CLI: set once, never cleared, inherited by any later settled status.
It is per run now. A run in flight is
watching; the read that finds itsettled turns that into
followed, the run's own signature, which is the sameidentity the dismissal already uses. A settled row draws when its click can act
on it or when it IS that run.
4. Review follow-up: a refused import is not a run to follow.
startmarked the page as watching beforeimport.runanswered, so a refusal("nothing to import") left the mark set and the read that follows adopted
whatever settled run was already on file. The mark is set only once the gateway
says a run started.
Type
Verification
Commands and results, all from this branch at
d1a7ef1c2:Reverse verification: neutralising the
staleguard turns exactly the six newgate cases red, including the one for a later run this page did not follow;
marking a refused run as watched again turns the refusal case red; reverting
the wizard wiring turns its own case red; reverting the one-line path change
turns the three updated resolver cases red. Restored and hash-compared each
time.
Real-host run, both fixes, on a gateway started against an isolated
RAVEN_HOMEseeded with a leftover import (2 sources, 1 failed, no requestrecorded) and a real file under the configured workspace. Same gateway, only
the built page swapped,
index.htmlhashes compared each swap.Import row:
52eb09b25)importSync-done importSync-warn :: memory import finished / 1 did not make it, retry buttondisabled: truetier+platformsto the same leftoverimportSync-paused :: paused / 100%, button enabled, labelled "resume"The third row is the control: it proves the page was connected and reading
import.status, so the empty rail in row two is a decision and not a deadpage.
Path chip, measured over the live route from the authenticated page, file
present at
<home>/workspace/skills/quick-summarize/SKILL.md:The first form is what the chip carried; the second is what it carries now.
Note the same relative request answers 200 with no session, which is why this
never showed up in a check that had no conversation open.
Not verified end to end: rendering a chip on the real page needs a model turn,
so the frontend half is covered by the resolver's own tests and the route half
by the measurement above, each read at the layer that owns it.
Base history: the branch now sits on
d50a13c1c, which carries #613 (the lintand contract drift that made
mainred at52eb09b25) and #618 (thetest_subagent_probe.pycase that was the one job still red here). Both weremain's own failures, reproduced onmain's own runs at the same bases, andnothing in this PR touches Python.
Risk
User-visible changes, both in the page only. No contract, no backend, no
persisted format.
A reader who relied on the leftover row to notice a months-old failed import
loses that notice;
raven import statusstill reports it, with the per-itemreasons the row never carried.
that used to resolve only because the session happened to run in the
workspace resolves the same way now, since the route resolves an absolute
path without re-rooting.
Rollback: revert either commit on its own. They touch disjoint files.
Related Issues
N/A