fix(progress) + feat(insights): eight defects, and a conclusion you can remove - #602
Merged
Conversation
…ame rules
Four of the ten progress-path defects found while tracing the assistant handoff.
- "Mark as finished" wrote two different locators: web the {"type":"end"}
sentinel, mobile scroll:<lastSlug>:0 — so a book finished on the phone reopened
at the top of its last chapter. One definition now, in shared, imported by both.
- PUT /me/progress had two code paths writing one row. The insert assigned
request.Percent by hand with no ProgressUnit check and never recorded
completion, so the FIRST write for a book kept a number every later write would
have refused, and a book finished in one write stayed unfinished. It now hands
the row to the same ApplyProgressUpdate. Covered by ProgressUnitEndpointTests,
which fails against the previous build.
- ReadingProgressDto existed twice in TypeScript with nothing linking the copies.
- "Talk this book over" swallowed openURL's rejection, leaving a tap that does
nothing — the one outcome a reader cannot tell from a broken button.
Two defects are deliberately NOT fixed and the reasons are written down:
LocatorKind on the catalog path would refuse the mark-as-read sentinel it was
meant to protect, and MaxChapterNumber's only reader was the deleted RAG gate.
Also deletes five web modules and one stylesheet with no importers, and corrects
the hook inventory that listed three of them as live.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
The web-local insights client unwrapped `.items`, an envelope the endpoint does not send: GET /me/insights returns Results.Ok(dtos) over a bare List. The line was copied from the mcpKeys client, where the envelope is real. The component stored undefined and read .length off it on the next render, so every signed-in BookDetailPage and UserBookDetailPage threw — with or without insights — in the change that was meant to make the section render at all. The component's own test mocks this module wholesale, so it could not see the shape. The new test talks to the client and pins what the server actually sends, including the empty case that is by far the most common (0 insights exist). Found by an adversarial read of the insight lifecycle, not by a failing test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
A misfiled conclusion was permanent. Replacing covers the common miss — a poor
conclusion about the RIGHT chapter, which the assistant re-runs and overwrites —
but nothing ever revisits a slot the model did not mean to write, and no client
could remove one.
DELETE /me/insights/{id}, reader-only. 404 rather than 403 for someone else's id,
so the endpoint cannot be used to learn that an id exists. Hard delete, matching
the entity's stance (a конспект, not a log) and avoiding a deleted_at that would
have to enter both partial unique index filters — where the NULLS NOT DISTINCT
rule protecting the book-level row lives.
Deliberately NOT an MCP tool. save_insight's worst case is one bad paragraph the
reader removes in a tap; a delete tool's worst case is a year of конспект gone,
driven by a stateless bridge that cannot confirm intent against a table with no
soft-delete and no trash. The asymmetry is categorical, not a matter of degree.
Affordance on both clients, with the failure path tested: a delete the server
refuses leaves the row on screen, because it is still there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
… it did not Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
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.
Two things that arrived together: the progress-path debt recorded while tracing the assistant handoff, and what a three-way consilium on insight categories turned up while arguing.
Progress path — four defects fixed, two deliberately not
"Mark as finished" wrote two different things. Web sent the
{"type":"end"}sentinel; mobile, from the same menu on the same kind of book, sentscroll:<lastSlug>:0— so a book marked finished on the phone reopened at the top of its last chapter. One definition now (packages/shared/src/reader/progressLocators.ts), both clients import it.The first write for a book skipped the rules.
PUT /me/progress/{editionId}had two code paths writing one row: an update that checksProgressUnit.IsTrusted, and an insert that assignedrequest.Percentby hand. So the first write for an edition kept an untrusted number every later write would have refused — and a book finished in a single write never got aCompletedAt. The insert now hands the row to the sameApplyProgressUpdate.ProgressUnitEndpointTestsfails against the previous build and passes against this one.ReadingProgressDtoexisted twice in TypeScript with nothing linking the copies.A tap that did nothing now says so — "Talk this book over" swallowed
openURL's rejection, which is right for "you have no browser" and indistinguishable from a broken button.Not fixed, on purpose (written up in
assistant-handoff.mdso nobody "fixes" them):LocatorKindon the catalog path would refuse the very sentinel it was meant to protect — catalog editions have no second coordinate space; andmarkAsUnreadnot loweringMaxChapterNumberis moot, because that column's only reader was the RAG gate deleted withask_book.The web insights section threw on every signed-in book page
Found by the consilium, not by a test. The web-local insights client unwrapped
.items— an envelopeGET /me/insightsdoes not send (it returns a bare list; the envelope belongs to/me/mcp/keys, where the line was copied from). The component storedundefinedand read.lengthoff it on the next render. Every signed-inBookDetailPageandUserBookDetailPage, with or without insights, in the change that was meant to make that section render at all. Its own test mocks the module wholesale, so it could not see the shape; the new test pins the wire shape instead.A conclusion can be removed
Saving again replaces the insight for the same chapter, so a poor conclusion about the right chapter self-corrects. One filed against the wrong chapter never does — nothing revisits that slot. It was permanent.
DELETE /me/insights/{id}, reader-only, 404 (not 403) for someone else's id, hard delete. Deliberately not an MCP tool:save_insight's worst case is one bad paragraph removed in a tap; a delete tool's worst case is a year of конспект gone, from a stateless bridge that cannot confirm intent.Also
Five web modules and one stylesheet with no importers deleted — three were listed as live in
CLAUDE.md's own hook inventory.Verification
dotnet format textstack.sln --verify-no-changes— cleandotnet test tests/TextStack.UnitTests; integration--filter Insights(18) and--filter ProgressUnit(2) — pass against a locally rebuilt APItsc --noEmiton web and mobile — clean🤖 Generated with Claude Code
https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E