Act on the desktop customer-feedback pass (#411-#418) - #423
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2f9b8fb to
dd63931
Compare
vishaltandale00
left a comment
vishaltandale00
left a comment
There was a problem hiding this comment.
Review of the exact PR head. I found one P1 and four P2 issues; details are attached inline.
|
All five findings are addressed in
Two things to look at rather than take on trustThe P1 fix changes what a state mismatch does. It no longer ends the The PRD had drifted. It still promised Evidence
The two account-session tests were confirmed failing against the previous Footer numbers are measured in the running app over CDP at a 960px viewport, not Not addressed hereTwo earlier observations are still open by choice: the footer pairs a glyph gear |
vishaltandale00
left a comment
vishaltandale00
left a comment
There was a problem hiding this comment.
Review of the latest iteration at 2391711. The prior updater and provider-card findings are resolved. I found one P1 and four P2 issues; details are attached inline.
|
All five addressed in The pattern in this roundFour of the five were siblings of first-round fixes — one callback branch, one The sharpest finding is that the test I added last round asserted the P1 bug. Measured, not assertedFooter geometry re-measured in the running app over CDP:
PRDOne checkpoint added, naming which outcomes own presentation: every terminal Still open by choiceThe footer pairs a glyph gear with a drawn person mark, and the optional-account |
Eight reported problems, filed as #411-#418 and fixed together because five of them land in the same minified stylesheet lines and cannot be split into reviewable hunks. Behavior: - #411 The updater checked once per launch, so a session left open past a release never learned it was behind. createDesktopUpdater now owns a recurring check beside the startup one. A poll is skipped in every in-flight phase, so it never resets progress the user is watching; autoDownload stays false; development never polls; the timer is disposed on shutdown. - #413 Sign-in opened the browser without asking for it to come forward, and the user was left staring at an unchanged Relayer window. All five openExternal seams now pass { activate: true } explicitly. - #415 A settled callback left the user in a spent browser tab. The callback handler now presents the Relayer window for every outcome it decides — verified, cancelled, or malformed — reusing the existing presentation path. A superseded callback and a plain timeout present nothing, and a window that cannot be presented never fails a sign-in. Presentation: - #412 Onboarding actions become plain labels (Connect, Back, Continue, Refresh models) in both the first-run screen and the settings dialog, and the nested boxes go: the harness summary and member rows lose their enclosures, the custom-family group becomes one left rule, and the connection field is outlined instead of filled. - #414 The light-theme lavender gradient on the first-run and new-thread surfaces becomes var(--bg), the same #fafafa BrowserWindow paints before first render, so there is no colour shift on launch. - #416 .workspace-layout gained top padding at all three breakpoints. The header row and Environment panel no longer start flush against the window chrome, and the top inset matches the right one. - #417 The model picker takes its surface from the theme tokens instead of #1b1e21 and #fff, the family select stops outweighing the models it filters, supporting text rises from 9px to 11px, and spacing lands on one 4px rhythm. Drops a stylesheet line that repeated its predecessor verbatim minus one rule, which would otherwise have needed every picker edit applied twice. - #418 The account control moves from a fixed bottom-right pill into the sidebar footer beside Settings, and the :has() rule that made .node-context-dock-actions reserve 112px for that pill goes with it. The collapsed 58px rail stacks its footer, because two controls do not fit side by side. #418 reverses the placement recorded in ADR 0008; the ADR, architecture doc, PRD ACC-008, and the evidence-capture script that asserted the corner geometry are updated to match, with the reason kept in the ADR. Not covered: #413 also asks for an in-flight fallback that re-opens the sign-in page from inside the app. That needs a new IPC channel, preload surface, and control, and the original failure still wants reproduction against a packaged macOS build with Chrome both cold and already running. #413 stays open for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two pieces of follow-up feedback on the desktop pass.
The recovery button read "Repair execution configurations", and its sibling
"Refresh models and set up defaults". Both are now "Repair" and "Refresh
models". The earlier pass shortened the refresh button in index.html, but
auth.js overwrites that text on every render:
recoveryButton.textContent = recovery?.label ?? "Refresh models and set up defaults";
so the markup change never reached the screen. The labels now change at the
sources that actually produce them, in provider-onboarding-model.js and
provider-ui.js, and in that fallback. The two blocking-reason strings stay full
sentences; they explain why setup is blocked rather than labelling an action, and
"Repair." alone would explain nothing.
The account control's mark was the glyph U+25CE, which reads as a bare circle
next to real iconography. It becomes a drawn person in the sidebar's existing
stroked-SVG idiom, matching the collapse and new-thread marks.
Carrying an inline SVG meant the label could no longer be the button's whole
text, because the controller assigns textContent and would erase it. The label
moves into its own span, which the controller writes when present and falls back
to the button when absent, so callers passing a bare element still work. That
also lets the collapsed rail reuse `body.sidebar-collapsed .footer-button span`,
the rule every other footer control already uses, so the font-size:0 workaround
added with the corner-pill move is gone. `#desktopAccountButton.textContent`
still resolves to the label alone, so the provider-ux evidence script's
"Signing in…" assertion is unaffected.
Verified in the running app against a copy of a real profile: the mark renders,
the labels are short, and the model picker shows real members at the intended
13px/11px scale.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five findings, one P1 and four P2. P1. A stale callback cancelled the login that replaced it. When a login is replaced on the same loopback port, the predecessor's callback is served by the replacement's listener, and the state check treated that as a malformed callback of the active attempt: it settled the replacement as error and presented the window. State is what identifies which attempt a callback belongs to, so a foreign state is now answered 400 and otherwise ignored, leaving the owning attempt in flight and presenting nothing. A callback that does carry this attempt's state and is still malformed keeps failing closed as before. This changes what a state mismatch does: it no longer ends the attempt. The security property is unchanged and slightly stronger — no token exchange, and a stray loopback request can no longer cancel a legitimate sign-in. P2. Provider completions never restored focus, though #415 required it. Connect and reconnect both settle in relayer:provider-connect-complete, which had no presentation authority. It now presents on a terminal connected result only. P2. The launch check bypassed the phase guard the poll uses, and re-fired on every window creation, so recreating a window while an update was downloading or staged could reset it to checking. Both scheduled discoveries now run through one guarded backgroundCheck, and the launch check is scheduled at most once per process. window.mjs no longer schedules its own. P2. The footer overflowed the 210px rail the 980px breakpoint switches to. The measured content box was 191px against 196px of content, and the circular update indicator was being flex-shrunk from 31px to 24.53px. The indicator no longer shrinks, and the footer drops its labels below 980px exactly as the collapsed rail does. Re-measured at 960px: 191px against 191px, indicator back to 31px. P2. The shortened recovery labels left several provider cards exposing the same bare accessible name, and the PRD still promised the long wording. The visible label stays short; the accessible name now carries the full operation and the exact provider, and the PRD records that split. Tests. The two account-session tests were confirmed to fail against the prior callback handler before the fix. New coverage coming out of the review: a same-port replacement whose predecessor's callback must not cancel or present it; a foreign state that leaves the attempt live and lets its own callback still complete; provider completion presenting only when connected; the launch check guarded and not repeated on window recreation; and the footer contract at the narrow breakpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review round. Four of the five findings were siblings of fixes from the first round that were applied to one instance and not swept, so each fix here was checked against its neighbours. P1. Ownership is now established before anything may touch the attempt. The previous round fixed the state-mismatch branch but left the method/host/path branch settling and presenting, so the browser's own /favicon.ico probe of the callback page cancelled a live sign-in and pulled the window forward. Both branches now answer 400 and return without settling. Only a request carrying this attempt's state may settle it. The test added last round asserted the favicon behaviour, so it encoded the bug as intent. It now asserts the opposite, alongside the wrong path, method and host cases the pre-existing test covered, and proves the listener survives all of them by completing the real callback afterwards. The malformed-shape case keeps its own coverage with a callback that does carry the attempt's state. P2. completeConnection() can throw for terminal post-browser failures such as catalog discovery or runtime registration. The await preceded presentWindow(), so those left the browser in front of an error the renderer reported behind it. Every terminal outcome now returns to Relayer; only a still-pending attempt presents nothing. The PRD records which outcomes own presentation. P2. Hiding the footer labels below 980px left the Settings button named by its gear glyph. The glyph is now decorative and the button carries an explicit name, as does the update indicator, which was named by its arrow for the same reason. Both label-hiding rules were then corrected to spare the glyph: moving it into a span would otherwise have made the collapsed rail hide it too, trading a naming defect for a blank button. P2. Above the breakpoint the 244px sidebar left the account label 68.1px for text that needs 69.8px, so "Signing in…" wrapped to two lines inside a 34px control. Footer labels no longer wrap, and slightly tighter padding keeps the common case from ellipsizing. Measured at 960, 981, 1000 and 1420px: no wrap, no overflow, the indicator at its full diameter, and Settings named "Settings". P2. The PRD requirement added last round for a contextual accessible name was applied to the provider cards and missed first-run. auth.js now sets the same name from the onboarding provider. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ready-phase guards landed in #443 explaining themselves in terms of "the window opens a periodic check five seconds after launch". This branch moved that check into the updater and put it behind backgroundCheck, so the sentence no longer describes the code around it. The guards still matter, for a different caller: scheduled discovery is stopped earlier by backgroundCheck, so what these guards now protect is the explicit check from Settings, which stays unguarded because the user asked for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
68d7498 to
94bebfa
Compare
vishaltandale00
left a comment
vishaltandale00
left a comment
There was a problem hiding this comment.
Review of exact head 94bebfa. I found two P2 issues; details are attached inline.
Required proof gaps:
- The new browser-outcome promise in docs/prd/index.html has no named checkpoint or real-service IPC test distinguishing terminal, pending, stale, and unowned outcomes.
- test/desktop-account-ui.test.mjs verifies raw strings rather than computed footer geometry and accessibility.
- test/provider-ui.test.mjs searches source text instead of executing both recovery-name branches.
…phase Third review round: two findings and three proof gaps. P2. The catch added last round presented for every completeConnection() rejection. Those rejections are not equivalent. completeConnection cancels the pending connection for a reconnect account failure, an unavailable reconnect login, and any catalog, readiness or runtime-registration failure. It does not cancel for an unknown connection, or for a connect whose account check failed transiently, which leaves the attempt live and cancellable. Presenting there pulls focus out of a browser flow the user is still completing, which is the boundary this branch put in the PRD one round earlier. Terminality is now recorded where the attempt is actually settled, on the error, under a registry symbol shared with the IPC layer. Presentation asks that question instead of treating any throw as the end. P2. The update indicator carried a fixed name of "Application update available" while the button is visible for available, downloading, ready and failed. It announced an update after a failed check and never announced a staged one. The name now follows the phase. Proof gaps. The browser-outcome promise had no named checkpoints. It is now BRW-001 through BRW-003, splitting terminal from transient, unknown and pending, and the IPC test covers those four outcomes rather than success and failure alone. The recovery-name assertions read auth.js source text and searched rendered markup for a prefix. Both recovery branches are now executed and their rendered accessible names compared exactly, including that two cards on one screen do not collide. The footer geometry assertions checked CSS strings. jsdom has no layout engine, so a unit test cannot answer whether the footer fits or a label wraps. Those questions moved to capture-provider-ux-video.mjs, which measures the rendered footer in Chromium: overflow, the indicator's diameter, whether a busy label wraps, and the Settings and indicator accessible names. The remaining CSS assertions say only that the rules they depend on exist, and say so. updateIndicatorName moved into its own module because updates.js cannot be imported without a DOM, so the naming can be exercised directly across every phase. The sealed telemetry inventory records the new module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both findings and all three proof gaps addressed in Proof gapsNamed checkpoints. The browser-outcome promise is now BRW-001 to BRW-003, Recovery names are executed, not searched. Both branches now render and their Footer geometry moved to where layout exists. One caveat worth stating One structural change
Judgement call to checkFor the terminal-failure boundary you offered two options. I took the typed Still open by choiceThe footer pairs a glyph gear with a drawn person mark, and the optional-account |
The two presentation items left open through the review rounds. The footer paired a glyph gear with a drawn person mark, so the two controls read at different weights beside each other. Settings is now drawn in the same stroked idiom, and the sizing rule covers both marks rather than the account one alone. The gear was the only remaining decorative glyph in the footer, so the label-hiding rules keep their aria-hidden guard for whatever is added next. The optional-account step was the last surface still carrying a coloured wash: a warm radial in light, a cool one in dark, while the first-run and new-thread surfaces had already been flattened. It now uses the themed ground, which makes the light override unnecessary rather than merely different. Verified in the running app: backgroundImage "none", rgb(250,250,250) in light and rgb(11,12,13) in dark. The rendered footer audit added in the previous commit reported a wrap here, and it was the audit that was wrong. It compared the label's natural text width against its box, which flags an ellipsis as a wrap. With nowrap the label truncates instead, and a truncated label is not a wrapped one. The audit now counts line boxes, which is the question it was meant to ask. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishaltandale00
left a comment
vishaltandale00
left a comment
There was a problem hiding this comment.
Reviewed exact head c0466c5. In addition to the inline findings, BRW-001 through BRW-003 still appear only in the browser-behavior narrative; they are absent from the product-requirements list and the verification table. No deterministic checkpoint currently crosses the real service → IPC classification boundary or verifies renderer ownership after a transient completion rejection.
Verification on this head: the focused provider/UI portfolio passed 141 tests; npm run build and full PR git diff --check passed. Hosted deterministic Vitest, quick checks, TypeScript builds, and PRD readability are green. Local npm run check stopped at the known harness failure because Cargo did not provide CARGO_BIN_EXE_relayer-app-server; all preceding Rust tests passed.
…kpoints Fourth review round: one P1 and six P2. P1. A connect whose account check failed rejected while the backend kept the pending attempt. completeConnection is the caller's poll, and its loop in provider-settings-connection.js continues only on "pending"; a rejection exits it and the finally releases ownership. The attempt then kept owning the provider name with nobody polling it, so the retry was refused and the setup was invisible. A connect that has not succeeded yet now stays pending, carrying why, so the loop keeps polling and the attempt stays cancellable. Reconnect still settles and reports terminally, because there the service cancels the attempt. Terminality is now a type this module owns, constructed before any cleanup runs. Stamping a property onto the provider's error threw an unmarked TypeError on a frozen error, and marking after cleanup let a cleanup rejection replace the failure being reported. The original error is preserved as the cause. connect() and reconnect() create the pending attempt before handing the URL to the browser, and a rejected handoff left it owning the provider name with nothing downstream aware of it. The handoff now cancels what it created before propagating. The update indicator named actions its button does not perform. Clicking it opens the popover; restart, retry and download belong to the nested control. The names now state the phase and then the operation this button actually performs. The drawn settings mark is wider than the glyph it replaced, and at the standard 244px footer with the update indicator visible it truncated both labels to "Settin…" and "Signing i…". Measured before and after: 50/49 and 70/69 truncated, now 50/50 and 70/70. The rendered footer audit was unsound twice over. getClientRects().length cannot prove a single line for a blockified flex item, it ran only at 1280px, and it accepted truncation, which is how it missed the regression above. It now counts line boxes from range rects, rejects truncation, and runs at 960, 981, 1280 and 1420. Confirmed against the reverted footer: it fails with "The account label is truncated at 981px", the breakpoint boundary the old probe never visited. The first-run recovery checkpoint rendered Settings provider-card markup, not auth.js. The naming decision moved into provider-onboarding-model beside the action it pairs with, and both first-run branches are exercised there against the function auth.js calls. BRW-001 to BRW-003 existed only in prose. They are now numbered requirements with verification-table rows, joined by BRW-004 for the browser handoff, and each row names the deterministic proof that carries it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All seven addressed in The P1 settles a question I left openLast round I argued against "cancel before rejecting" because it would kill a Named checkpointsBRW-001 to BRW-004 are now numbered requirements with verification-table rows, The service to IPC classification boundary is now crossed by tests against the I verified the audit fails before trusting itThe footer audit was unsound twice. This time I reverted the footer fix and Three of these were mine to begin withThe wider drawn gear truncated both footer labels, the indicator names promised |
vishaltandale00
left a comment
vishaltandale00
left a comment
There was a problem hiding this comment.
Reviewed exact head 3b88325b. The prior update-name, footer-width, wrapping-audit, terminal-wrapper, transient-ownership, and browser-handoff fixes are present; the rendered footer and accessibility states now pass at 960/981/1280/1420. I found one P1 and seven P2 issues, attached inline.
Required verification: the affected provider/UI portfolio passed 157 tests, including the Chromium evidence capture; npm run build and full-PR git diff --check passed. Hosted deterministic Vitest, quick checks, TypeScript builds, and PRD readability are green. Local npm run check again stopped only at the known harness problem where Cargo did not provide CARGO_BIN_EXE_relayer-app-server, after the preceding Rust tests passed. Adversarial reviews are non-certifying.
Remaining mapping gaps include no checkpoint for renderer destruction/window recreation, notification failure, or teardown rejection. The new terminal wrapper also lacks committed regression cases for a frozen cause and rejecting cleanup.
A pending managed connection reserves the provider name, but the poll that settles it and the ownership that cancels it lived only in renderer memory. Round 4 moved the leak's trigger from "a transient failure rejects" to "the renderer stops polling"; it did not close the leak. Give the attempt an owner in main. The renderer that begins it owns it, and its contents being destroyed cancels it, so closing a window frees the name instead of holding it until the app restarts. Both shipping managed adapters collapse every runtime, process, protocol and parse failure into one `unavailable` result, so a check that cannot reach a verdict is transient only for a bounded run; after that the attempt settles and readmits the name. An aborted check settles at once. Only `disconnected` means the login is still open. Pending cleanup now runs its stages independently: sequentially, a rejecting close() skipped runtime-state and credential removal while the runtime was already gone from both collections that could revisit it. Provider change notifications become best-effort at all six send sites; sending into destroyed contents was turning a settled connection into a rejection, before the browser return it precedes. The four mapped checkpoints move to the seams that ship. BRW-002 and BRW-004 drive the production poll loop and a real definition service, and assert the name is readmitted rather than that cancelConnection was called. The first-run recovery name and the update indicator are proven in the Chromium capture against rendered DOM, since this repository has no DOM runtime for Vitest: the capture drives real update phases through the renderer's own subscription instead of posing the control, and two new first-run variants render both recovery branches. Every new checkpoint was verified to fail when the behavior it claims is removed. Issue #448 tracks the two ways an attempt can still outlive its owner: an abandoned sign-in whose account stays disconnected, and a reload that replaces the JavaScript context but keeps the same contents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 5 addressed at The P1 and one P2 were the same defect
The attempt now has an owner in main: the renderer that begins it, cancelled when its contents are destroyed. And a check that cannot reach a verdict is transient for a bounded run rather than forever, because Decisions taken, with the alternatives namedBounded, not typed. Three consecutive verdict-less checks, counter reset by any Bind now, file the rest. Two ways to outlive an owner remain, and both need a timeout duration that is a product decision with no evidence yet: an abandoned sign-in whose account stays Every new checkpoint was verified to fail
Where the rendered proof wentThere is no DOM runtime for Vitest here — jsdom is not a dependency — so both rendered checkpoints went into the Chromium capture. The evidence harness now drives real update statuses through the renderer's own subscription instead of the audit posing the control, and two new first-run variants render both recovery branches through Not doneThe "retain and reconcile failed cleanup ownership" half of the teardown finding. Scope, worth your judgementThis started as eight UI presentation items and now carries a provider connection-lifecycle change. That is why depth keeps appearing in one file across rounds — a one-line symptom fix in round 3 brought a different problem in with it. The presentation work and the lifecycle work could reasonably be separated before merge. |
|
Following up on the scope note at the end of my last comment, with evidence rather than a feeling. Three review rounds have found bugs in one file, and each was a different way a pending attempt fails to release the provider name it reserved. Round 3: a transient failure rejects and the name stays held. Round 4: a failed browser handoff orphans the attempt. Round 5: a dead renderer holds it, and So I checked whether the reservation should exist. Nothing in this PR changed — this is an investigation, and the branch is untouched since The reservation has no backingThe rule is one clause in Deleting that clause, the full deterministic suite passes — 2061 cases. That includes the readmit tests I added this round, which is itself worth noting: they only distinguish anything because the reservation exists. The database already enforces itCREATE UNIQUE INDEX model_providers_active_label_nocase
ON model_providers(label COLLATE NOCASE)
WHERE lifecycle_state IN ('active','removal_pending');
Two same-label attempts are close to unreachable
In every case I could construct it either never fires, or fires on the leak, where it blocks a legitimate retry citing an attempt the user cannot see or cancel. The honest cost of removing it
That failure mode is already reachable today through a narrower window, and the reservation does not close it. A uniqueness re-check immediately before persisting would close it and cover cases the reservation misses. What I am proposing, and not proposingNot proposing to touch it here. This is a lifecycle design change and #423 is five review rounds deep on presentation work; it belongs in its own PR. Proposing that the follow-up is replace the reservation with a pre-persist check, rather than #448's current direction of adding a deadline to defend it. That would make BRW-002, BRW-004 and BRW-005 defensive rather than load-bearing, and close #448 as moot. #448's premise is now in question and I can annotate it if you agree with the reading. Two limits on thisThe reachability analysis is from reading code, not from driving the app, so there may be an entry point I have not found. And "removing it breaks no tests" is an argument from absence — the same shape of reasoning that produced two bad measurements earlier in this PR. I would want a test proving a real collision fails cleanly under the re-check before anyone acts on this. |
|
Good investigation. I agree this belongs outside #423, and I lean toward provider ID—not a pending label—being the identity of an attempt, with label exclusivity decided at the connected commit. I would not close #448 as moot. On the current head, reservation is now explicit architecture/product behavior ( For the follow-up, I would define first durable commit wins and keep the SQLite transaction/unique index authoritative. A persisted-label check under the serialized service queue is useful for early UX, but it is not sufficient authority; the current unique-index collision becomes a raw internal database error, so the transaction path needs to map it to a stable, actionable catalog conflict. The losing attempt must terminate and clean all runtime, state, catalog, and credential side effects. Preserve the preparing-order, connection-ID, and reconnect same-ID guards, and align label normalization across renderer, main, and SQLite. The core proof should exercise two same-label managed attempts in both completion orders: exactly one durable definition, a clean terminal conflict for the loser, and complete loser cleanup. It should also cover API-key creation and the active/removal-pending/rename/direct-app-server boundaries. So I agree with a separate design PR and likely with moving exclusivity to durable commit; I disagree that this makes BRW-002/004/005 merely defensive or #448 moot. |
|
You are right, and the part I got wrong is the part that matters. I analyzed what a pending attempt reserves and never asked what else it owns. The leak is a process, not a nameA pending attempt holds four things, all reclaimed only by
That makes BRW-005 load-bearing in a way I missed. Its cancel-on-destroy is the only thing that kills that process, and it stays load-bearing under every exclusivity model — moving the label rule does not touch it. Withdrawing "merely defensive" and "moot": both were wrong, and for the same reason. Split, as you asked#448 retitled and rewritten to own orphan-attempt and process cleanup. It now leads with the process rather than the name, tabulates what an attempt owns, keeps the two remaining paths (⌘R reload keeping the same #449 files the exclusivity work with your constraints recorded: provider ID as attempt identity, first durable commit wins, the SQLite transaction and I also recorded your point that the reservation is now explicit architecture and PRD behavior on this head, so removing it is a product decision with remapped evidence rather than a refactor. On this PRNothing changed; the branch is still One thing I would flag on your direction. Making two same-label pending attempts intentionally reachable means the losing attempt's cleanup becomes a routine path rather than an error path, and today that cleanup is only exercised through cancellation. #448 and #449 landing together, in that order, seems right to me for exactly that reason. |
|
Agreed—the split now captures the right boundaries, and keeping #423 unchanged is the right scope decision. I agree #448 must ship no later than #449. It does not have to be a separately merged predecessor if one atomic change proves both contracts, but #449 should not ship alone: removing the reservation would let repeated reload/retry cycles accumulate multiple UUID-backed, same-label orphan runtimes even though SQLite still protects durable integrity. One refinement is still needed in #448. Its current direction to run teardown stages independently describes the Please make #448 explicitly retain cleanup ownership and the runtime handle until every applicable idempotent stage succeeds or is proven absent, with retry/reconciliation, diagnostics, and shutdown handling. Its proof should make each stage fail independently—including close failing once and succeeding on retry—and cover every exit path, including expiry/reload, explicit cancellation, terminal provider failure, shutdown, and the #449 conflict loser. With that addition: #448 before or atomically with #449, and no further lifecycle expansion in #423. |
|
Refinement taken, and you are right that my round-5 fix only did half the job — I said as much on that thread and then wrote the weaker half into #448 anyway. Verified before updating. Both cleanup sites drop ownership before teardown and discard what teardown returns: const pending = this.pendingConnections.get(connectionId);
if (!pending) return false;
this.pendingConnections.delete(connectionId);
...
this.runtimes.delete(connectionId);
await Promise.allSettled([ pending.runtime.close?.(), ... ]);
return true;
#448 updatedA new section, Cleanup can forget a live process even when it runs, states that independent stages are already on this head and are not sufficient, with the code above and the shutdown case. The required behavior is now explicit: idempotent independently retryable stages, ownership released only when the last is discharged, failed cleanup retained and reconciled including at startup for ownership surviving a previous run, failures recorded in diagnostics rather than swallowed, and shutdown discharging or preserving outstanding ownership instead of clearing unconditionally. The proof section takes your list as the bar — each stage failing independently, OrderingRecorded in both issues: #448 ships before #449 or atomically with it, not necessarily as a separately merged predecessor if one change proves both contracts, and #449 never alone. I wrote your reason in rather than the bare rule, since it is the part that makes the constraint self-evident — repeated reload and retry accumulating multiple UUID-backed same-label orphan runtimes, with SQLite still protecting durable integrity but nothing protecting the processes. #423Unchanged at |
1 participant
Acts on eight pieces of desktop customer feedback, filed as #411-#418.
They ship together because five of them land in the same minified
styles.csslines, which cannot be split into separately reviewable hunks.
Behavior
window.mjs, 5s after load). A session left open past a release never learned it was behind.createDesktopUpdaterowns a recurring check beside the startup one. Both are guarded bybackgroundCheck, so neither disturbs a phase the user is already watching.autoDownloadstaysfalse, development never polls, the timer is disposed on shutdown.openExternalseams pass{ activate: true }explicitly instead of relying on an unstated platform default.127.0.0.1:…/auth/callback, while the app had already moved on behind them.Presentation
var(--bg)— the same#fafafaBrowserWindowpaints before first render, so there is no colour shift on launch..workspace-layoutgains top padding at all three breakpoints. The header row and Environment panel no longer start flush against the window chrome, and the top inset matches the right one.#1b1e21/#fff, the family<select>stops outweighing the models it filters, supporting text rises 9px → 11px, and spacing lands on one 4px rhythm.:has()rule that made.node-context-dock-actionsreserve 112px for that pill goes with it.Connection lifecycle
Review found that #413 and #415 rest on a distinction the service did not make:
which failures of a browser leg are over, and which leave the attempt live. That
brought the pending-connection lifecycle into scope, recorded as BRW-001 to
BRW-005 in the PRD.
and is cancelled when that renderer's contents are destroyed. Before this, a
closed window held the provider name — and its runtime — until the app
restarted.
parse failure into one
unavailableresult, so a check that cannot reach averdict keeps the attempt pending only for a bounded run, then settles and
frees the name. An aborted check settles at once. Only
disconnectedmeansthe login is still open.
leaves the user in the browser flow they are still completing.
into destroyed contents was turning a settled connection into a rejection.
close()nolonger skips runtime-state and credential removal.
Two things reviewers should look at
#418 reverses a recorded decision. ADR 0008 specified the account control
"anchored to the bottom-right of the application viewport rather than
participating in sidebar layout". The customer asked for the opposite. This
implements the customer's request and updates ADR 0008,
docs/architecture.md,PRD ACC-008, and the evidence-capture script that asserted the corner geometry,
keeping the reason in the ADR.
#415's in-app fallback is not here. The activation and return fixes are, but
the fallback that re-opens the sign-in page while
signing-inneeds a new IPCchannel, preload surface, and control. The original failure also still wants
reproduction against a packaged macOS build with Chrome cold and already
running. #413 stays open for that remainder.
Verification
npm run checkandnpm run buildpass on the final tree — 2061 deterministicVitest cases plus the process-bound Chromium evidence capture.
Layout and accessible names are computed against real Chromium, not asserted
from source, because jsdom has no layout engine and this repository has no DOM
runtime for Vitest. The footer audit measures line boxes from
Rangerects at960, 981, 1280 and 1420px and rejects truncation. The update indicator is driven
through the renderer's own subscription across all four visible phases, reading
the rendered accessible name and proving activation opens the details rather
than performing the nested action. Two new first-run variants render both
recovery branches through
auth.js.Every checkpoint added in the last two rounds was verified to fail when the
behavior it claims is removed.
Follow-ups
covers a closed window; a ⌘R reload and an abandoned browser sign-in remain.
at a pending attempt. Ships after An orphaned provider connection attempt leaks a live runtime process until restart #448 or atomically with it, never alone.