fix(app): reach the annotator by clicking, on the asset that was clicked (#160) - #166
Merged
Merged
Conversation
…ked (#160) There was no way into the annotator from inside the app. Every gallery tile rendered `disabled`, the annotator's own "Open the gallery" button rendered `disabled`, and nothing anywhere navigated to `/jobs/:jobId` — so the one thing the product exists for was reachable only by pasting a URL whose job id had to be read out of the REST API first. The cause is one line that looks like an omission because it is one: `routes.tsx`'s `Gallery` passed no `onOpenAsset`, and `GalleryScreen` renders a tile `disabled` when that prop is absent. Every sibling route wires its callbacks. The two screens are keyed on different things — the gallery lists *assets in a batch*, the annotator opens a *job* — and `BatchAsset.job_id` is the only bridge. It is null exactly while the batch is a draft (#29's shape), so: - a tile in an approved batch opens `/jobs/{job_id}?asset={asset_id}`, and the annotator opens **on that asset**. Landing on the first one after clicking the fifth reads as the click being ignored. - a tile in a draft batch stays inert and carries the reason on the control itself. `Tile` decides that from the asset rather than trusting the caller, so the two inertias — "this host does not navigate" and "this asset has nowhere to go yet" — stay separate. - "Open the gallery" is handed the project and batch by `AnnotationPage`, which has already walked job → batch to find the pinned schema. The app never asks again for what the screen is holding. The asset travels as a query parameter rather than a path segment: the job is the annotator's identity and the asset is only where to start, which the next/previous buttons change without the URL becoming a lie. **The guard is the point.** `cycle.spec.ts` now reaches the annotator entirely by clicking — project → batch → tile → annotate → back to the gallery — and `jobIdOf`, the helper that fetched a job id so the spec could type a URL, is deleted along with the navigation. That helper *was* the workaround: a suite that fetches an id the product never shows cannot notice that the product never shows it. Deep-linking is still checked, as a reload of the URL the app itself produced, which drives #58's SPA fallback for real without reopening the door. Verified by mutation: disabling every tile again fails the walk at `toBeEnabled`, and ignoring the requested asset fails it at `3/3`. The positioning itself is a pure `assetPositionOf` with its own vitest cases, because jsdom reports every element as 0x0 and the gallery's virtualizer renders no tiles there to click. Frontend only: no Python, no migration, `openapi.json` and the generated client byte-identical, `FORMAT_VERSION` still 12. Closes #160
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
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.
Closes #160. Second of the
0.0.1-beta.2defect fixes, and the one that makes the product usable.The defect
There was no way into the annotator from inside the app. Three facts, each verified in the running wheel:
disabled.disabled./jobs/:jobId.The annotator was reachable only by pasting a URL whose job id had to be read out of the REST API first — which is exactly what
cycle.spec.tsdid, and why a suite with 76 browser scenarios plus a full real-server cycle never noticed.frontend/app/src/routes.tsx'sGallerypassed noonOpenAsset, andGalleryScreenrenders a tiledisabledwhen that prop is absent. Every sibling route wires its callbacks.The design question the issue raised
The two screens are keyed on different things — the gallery lists assets in a batch, the annotator opens a job — and
BatchAsset.job_idis the only bridge. It is present exactly once the batch leavesdraft(#29's deliberate shape). So:/jobs/{job_id}?asset={asset_id}, and the annotator opens on that asset. Clicking the fifth picture and getting the first reads as the click being ignored.Tiledecides that fromasset.job_id, not from the caller, so "this host does not navigate" and "this asset has nowhere to go yet" stay separate — the issue's "do not fix this by relaxingTile's disabled rule".(projectId, batchId)byAnnotationPage, which has already walked job → batch to find the pinned schema. The app never re-asks for what the screen is holding.The asset travels as a query parameter, not a path segment: the job is the annotator's identity, the asset is only where to start, and next/previous change it without the URL becoming a lie.
ui-corestill imports no router — navigation stays a callback prop, per the contract inroutes.tsx.Positioning, and why it is not an effect
The obvious spelling is
useState(0)plus an effect that jumps once the assets arrive. That is the exact shape of #159, the next issue in this milestone: an effect whose one chance to run happens while the thing it needs is still absent. Herechosenis null until the user navigates and the index falls through toassetPositionOf(assets, requested)— derived on every render, so there is no moment to miss, and a background refetch cannot pull somebody back to where they started.An id the job does not carry answers position
0. A stale link is not an error state.The guard, which is the deliverable
frontend/app/cycle/cycle.spec.ts— #48's harness, #59's real-server config, no new runner — now reaches the annotator entirely by clicking: project → batch → tile → annotate → back to the gallery. Two new steps:disabled,data-pending, and atitlenaming the draft.jobIdOf, the helper that fetched a job id through the browser's session so the spec couldpage.goto('./jobs/' + id), is deleted along with the navigation. That helper was the workaround. Deep-linking (criterion 5) is still checked — as apage.reload()of the URL the app itself produced, which is a freshGET /ui/jobs/<id>?asset=at the server and drives #58's SPA fallback for real, without reopening the door this task closed.Verified by mutation, both directions:
disabledagaintoBeEnabled()assetPositionOfignores the requested asset3/3, receiving1/3assetPositionOfis pure and exported with its own four vitest cases, because jsdom reports every element as 0×0 — the gallery's virtualizer renders no tiles there to click, whichgallery.test.tsxalready says out loud.Ledger
openapi.jsonandfrontend/ui-core/src/generated/api.tsbyte-identical;FORMAT_VERSIONstill 12.browser cycle (chromium)job already runs this config.Checks run locally
pnpm --filter @visionset/app cycle— 1 passed (the whole cycle, real server, built bundle)pnpm --filter @visionset/app e2e— 76 passedpnpm test— 711 annotator + 111 ui-core vitest (was 107), 22 node:testpnpm -r build,pnpm -r lint,pnpm -r typecheck— clean