Repository navigation
Milestone/stu implement updated UI ux - #100
Conversation
…ust the first (cherry picked from commit d5a3c92)
…ust the first (cherry picked from commit d5a3c92)
…can reach a class transitively (cherry picked from commit 9d338ad)
…can reach a class transitively (cherry picked from commit 9d338ad)
…hape (cherry picked from commit ff9bfac)
…hape (cherry picked from commit ff9bfac)
(cherry picked from commit 1ed78b6)
(cherry picked from commit 1ed78b6)
…t HTML (cherry picked from commit ece1237)
…t HTML (cherry picked from commit ece1237)
…ust the first (cherry picked from commit d5a3c92)
…can reach a class transitively (cherry picked from commit 9d338ad)
…hape (cherry picked from commit ff9bfac)
(cherry picked from commit 1ed78b6)
…t HTML (cherry picked from commit ece1237)
…ust the first (cherry picked from commit d5a3c92)
…can reach a class transitively (cherry picked from commit 9d338ad)
…hape (cherry picked from commit ff9bfac)
(cherry picked from commit 1ed78b6)
…t HTML (cherry picked from commit ece1237)
…ust the first (cherry picked from commit d5a3c92)
…can reach a class transitively (cherry picked from commit 9d338ad)
…hape (cherry picked from commit ff9bfac)
(cherry picked from commit 1ed78b6)
…t HTML (cherry picked from commit ece1237)
…pe in a Set The public deploy route accepted an unbounded sdkPackages array and deduped it with repeated result.contains scans, so an unauthenticated caller could submit a very large list for quadratic parsing work and an equally large scratch pubspec. Cap the list at maxDependenciesPerRequest (200) - the same per-request budget the dependency maps use, since every accepted name is emitted as a dependencies: entry in the generated pubspec - and reject with the same 'Too many entries in verification...' error style. Deduplicate via a Set instead, keeping insertion order and the List<String> contract. Also rebuild dist/ so the committed output carries this branch's token changes (new CSS bundle beside the unchanged JS).
The default fixture router continued every unrecognized request, so an endpoint missing from the fixture map (runpipeline-image, the staging FlutterFlow sync, an env-overridden deploy URL) could silently reach a paid generation or a real project. The catch-all now aborts unmatched requests so un-fixture'd calls fail tests loudly. Only three request classes are allowed through, all explicit: - fixture-map URLs are fulfilled as before; - static assets index.html references are fulfilled with inert stubs (Tailwind, Google Fonts CSS, YouTube embeds), removing the page load's dependency on live CDNs and killing the YouTube player/ad/ tracking request cascade; - same-origin requests the local Vite dev server serves (the document, modules, Vite internals, public assets) continue, except /api paths, which vite.config.js proxies to real AI providers; - the SRI-pinned static scripts (JSZip, highlight.js, FingerprintJS) continue because a local stub can never match the pinned digest and app.js needs FingerprintJS to resolve identity; SRI verifies them byte-for-byte. Verified by rerunning the smoke suite (6 passed, 10x faster with the YouTube cascade gone) and by probing that fetches to runpipeline-image, the staging sync and /api proxy paths all abort. STU-372
buildArtifactBundle synthesized deployOrder values like "file-1" while the artifacts themselves get real IDs such as "custom-action-greet-user". Normalization preserves unknown order IDs, and the deployment planner matches order entries against artifact.id, filtering out every synthetic value — so a deploy journey driven by these fixtures produced an empty deployment plan instead of exercising artifact deployment. Map the artifacts first and derive deployOrder from the mapped artifacts' IDs, the same relationship src/artifactBundle.js guarantees. Verified against the real contract: oneArtifact now plans lib/custom_code/actions/GreetUser.dart and manyArtifacts plans all three files in class -> action -> widget order with no planner errors. STU-372
The pull-request workflow ran only unit, build, deploy-guard, proxy and leak-scan checks, so regressions in fixture routes, selectors and smoke assertions could merge while CI stayed green. Add the Chromium browser install and npm run test:ui after the production build, so the deterministic browser suite gates every pull request. Step style and the pinned-action/least-permission hardening from the earlier CI commits are unchanged. STU-372
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Submitted attachments can disappear before generation
When runThinkingPipeline waits on preflight, removing an uploading image aborts it despite its slot already being snapshotted. Generation then silently omits that submitted image.
…ui-ux' into pr105 Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> # Conflicts: # e2e/stu385-composer-settings.spec.js # e2e/stu391-tutorial-onboarding.spec.js # src/composerAdapter.js
- cancelTyping() lands each running demo's end state instead of abandoning a submittable prefix (view switches, tab hides). - A pointerdown takeover releases demo ownership after the restore, so the first Backspace trims the complete prompt; focus/keydown restores stay demo-owned so typing mid-demo still writes a fresh prompt. - e2e: click+Backspace trims one char; switchView during typing and during the hold both settle the shipped prompt and Generate submits it whole. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
* fix: hero demo takeover can never submit a partial prompt A mid-demo click/keypress canceled the typing timer but left the frozen prefix in the field, so a Generate click or Enter could submit a half-typed example. Takeover now lands the demo's own end state — the complete shipped prompt with the caret retired — via finishHeroDemo. Two takeover shapes skip the restore instead: an interaction arriving with text already selected (select-all, assistive insert) is an edit in flight — overwriting the field would destroy the selection the incoming replacement needs — and a takeover during the hold period has nothing typed to complete. Both freeze the demo on the prefix and mark it demo-owned: send stays disabled and Enter stays inert until a real input event hands the text to the user. Also fixes the reduced-motion listener lifecycle: a pagehide dispose made bfcache restores lose the listener, so a demo replayed after back-forward navigation ignored a later flip. The listener now lives for the document's lifetime (dispose reserved for genuine teardown), and a repeat initComposer on the same DOM replaces the dead registration via a WeakMap instead of stacking a second listener. Adds an e2e covering a mid-demo click followed by an immediate submit, and demo-completion waits in smoke/stu376 where prompts were being polled mid-type. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> * fix: edits while the demo owns the field never join the example Takeover restore landed the full shippedPrompt in the field, but a keystroke, Backspace, paste, drop or IME commit that followed then joined the example — the browser inserts into whatever the field holds, and the input handler treated the combination as user text. Text the demo supplied while interrupted is now tracked as demo-owned (heroDemoOwned) through the frozen and restored states. A beforeinput listener — the single funnel every real edit passes through — clears it before the native mutation lands, so typing mid-demo produces a prompt of the user's own and Backspace removes demo text rather than deleting inside the example. A demo that finishes untouched hands the shipped text back as the field's own content, so ordinary editing resumes. New e2e coverage: mid-demo typing yields only the typed text, and Backspace during the demo leaves an empty field. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> * fix: system cancellations and pointer takeovers settle the hero demo - cancelTyping() lands each running demo's end state instead of abandoning a submittable prefix (view switches, tab hides). - A pointerdown takeover releases demo ownership after the restore, so the first Backspace trims the complete prompt; focus/keydown restores stay demo-owned so typing mid-demo still writes a fresh prompt. - e2e: click+Backspace trims one char; switchView during typing and during the hold both settle the shipped prompt and Generate submits it whole. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…init The demo-window catches raced the demo's real-time clock: an 8s deadline against a ~5.2s hold left <3s for page init on a loaded worker, so the 'a live reduced-motion flip mid-demo' spec timed out before the typing window opened. Raise the shared catch budget to 20s — it covers init plus the hold while still ending before the demo could finish, and leaves every assertion unchanged. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Fix flaky stu376 demo-window catch: budget must cover page init + the 5.2s hold
There was a problem hiding this comment.
🔍 Confirm generated bundle deployment path
The PR commits generated assets alongside application source. Confirm whether deployment serves dist and whether the committed bundle matches the final build.
Was this helpful? React with 👍 or 👎 to provide feedback.
…e-origin proxy The runner's CORS allowlist admits only the production origin, so under npm run dev the browser's preflight was rejected and provisioning failed instantly as UnconfirmedDeployError. Vite now proxies /api/ffai-runner to the runner and the dev build calls that same-origin path; production still uses the runner URL directly. The e2e endpoint fixture is a glob so the existing routes match whichever URL form the app uses. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
When a second modal opens while the commit-progress overlay still holds focus, setBackgroundInert marks the overlay aria-hidden around the focused element and Chrome blocks it. Drop focus to the body so the opening modal's own focus step can take over, and rebuild dist. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
The fill jumped between stage boundaries (doneCount/3) so nothing moved while a stage ran. Each stage now owns a third of the track and the fill eases asymptotically toward 95% of its slice on the existing 250ms tick, landing on the real boundary only when the stage's response arrives. Failed stages freeze where they eased; skipped slices (refine re-entry) start consumed. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Server errors trigger duplicate writes
When syncCustomCodeChanges returns HTTP 500, pushCodeWithRetry sends the same write again. The first response can follow a successful write, so retries can apply one deploy multiple times.
|
This is the deliberate trade-off this code documents and locks in tests, so flagging the semantics rather than treating it as a bug: an HTTP 500 from the synchronous push is ambiguous — the write may or may not have been applied — and The alternative — treat a 500 like every other 5xx and stop retrying — removes the duplicate-write risk at the cost of failing a deploy that may have been refused before any write. Happy to make that change if you'd rather have zero retry ambiguity, but it's an intentional design choice on this branch rather than an oversight, so I've left it as designed. |
…their boundary - Failed runs pin the fill's rendered width with the transition disabled, so the bar stops where it visibly was instead of drifting 700ms more. - A completed active stage sits on its own boundary (activeStage/3), so stage-2 re-entry runs finish on 100% instead of retreating to 66.7%. - e2e: held stages 2/3 assert upper ceilings; a new spec pins the frozen width across the transition window; the refine spec asserts a 100% finish. - Timer comment now covers both elements it drives. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…rack % - Reading getComputedStyle after transition:none resolves the pending target, not the mid-flight position — capture first, then pin. - The pin is now renderedPx/trackPx as a percentage, so a later viewport resize keeps the same proportion instead of a stale pixel count. - e2e: frozen pin asserts a %-valued inline width and a fixed computed width across a 900ms window. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
* fix: pipeline progress bar eases inside each stage's slice The fill jumped between stage boundaries (doneCount/3) so nothing moved while a stage ran. Each stage now owns a third of the track and the fill eases asymptotically toward 95% of its slice on the existing 250ms tick, landing on the real boundary only when the stage's response arrives. Failed stages freeze where they eased; skipped slices (refine re-entry) start consumed. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> * fix: review follow-ups — freeze fill on failure, done stages land on their boundary - Failed runs pin the fill's rendered width with the transition disabled, so the bar stops where it visibly was instead of drifting 700ms more. - A completed active stage sits on its own boundary (activeStage/3), so stage-2 re-entry runs finish on 100% instead of retreating to 66.7%. - e2e: held stages 2/3 assert upper ceilings; a new spec pins the frozen width across the transition window; the refine spec asserts a 100% finish. - Timer comment now covers both elements it drives. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> * fix: read rendered width before disabling the transition; freeze as track % - Reading getComputedStyle after transition:none resolves the pending target, not the mid-flight position — capture first, then pin. - The pin is now renderedPx/trackPx as a percentage, so a later viewport resize keeps the same proportion instead of a stale pixel count. - e2e: frozen pin asserts a %-valued inline width and a fixed computed width across a 900ms window. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> * test: assert the frozen fill keeps its track share across a resize Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Stuart Gardoll <sgardoll@gmail.com>
The error-input overlay is absolutely positioned over the results stage, which clips overflow; its centered card overflowed top and bottom whenever the stage was shorter than the card, leaving the textarea and actions unreachable. Scroll the panel and center the card with auto margins so it centers when there is room and scrolls from the top when there is not. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 3 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Stored project bypasses conflicting selection
When a different project is bound, getConfirmedStoredProjectId still returns the stored project after listing it. The deploy can target the stored project despite the active selection pointing elsewhere.
| - name: Install Playwright Chromium | ||
| if: steps.browser-tests.outputs.ui == 'yes' || steps.browser-tests.outputs.accessibility == 'yes' | ||
| run: npx playwright install chromium --with-deps | ||
|
|
||
| - name: Run browser tests | ||
| if: steps.browser-tests.outputs.ui == 'yes' | ||
| run: npm run test:ui | ||
|
|
||
| - name: Run accessibility modal tests | ||
| if: steps.browser-tests.outputs.accessibility == 'yes' | ||
| run: npm run test:accessibility |
There was a problem hiding this comment.
🔍 Check browser-suite CI budget
CI now runs the full Playwright suite and accessibility checks. Confirm the required job has sufficient time for the deployment scenarios and their fixtures.
Was this helpful? React with 👍 or 👎 to provide feedback.
At <=920px the phase title measured ~304px against the 298px content
column, so the longest title ('Understanding your prompt') wrapped to a
second line and pushed every later section down 30px — the 390px exact
fidelity spec caught the 527px panel that should be 497px. Tightening the
mobile tracking keeps the same size and line-height on one line.
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
- provisionStream: only an explicit "result" event (or a bare JSON payload with a boolean success) counts as the runner's answer — a heartbeat or progress event no longer fabricates a final result. - composerAdapter: typing a chip or tapping elsewhere mid-typing now completes the chip's end state (finishChipTyping) instead of cancelling, so partial text can never be submitted. - app.js: readAsDataUrl aborts on signal so an aborted upload frees the send gate; removePromptImage refuses while a generation is running so run-claimed uploads can't be deleted mid-run. - app.js: verified-write credits in the dirty ledger are spent once a fresh export postdates them (matching, reformatted, or missing) — uncertain records still survive. - app.js: getConfirmedStoredProjectId follows a confirmed key+generation binding that names a different project instead of returning the stored project it replaced. - heroMarkField/logoMotion: pagehide disposes only when the page is not persisted; a bfcache-persisted page pauses and resumes on pageshow. - pubspecSync: rewriteVersionMember skips blank and comment-only lines inside a block mapping, so a version member written after them is still found and rewritten (regression tests added). Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
|
Resolved the remaining live Devin Review findings in cd4c657:
Two findings were left as-is by design (retry on HTTP-500 → |
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 3 new potential issues.
🐛 2 issues in files not directly in the diff
🐛 Abandoned generation remains on screen
When navigation interrupts a running generation, switchView cancels its requests but leaves the progress panel visible. Returning home restores that panel, although no run remains to finish or clear it.
⚠️ Failed replacement resets selected file
When refinement or error regeneration fails, restoreAndShowReplacementFailure restores the selected artifact but showResultsView switches to Summary. The user's previous file view is lost despite the retained result.
| * only the UI's waiting/rendering: on expiry the UI stops claiming progress, | ||
| * but it never aborts or retries the in-flight remote write. | ||
| */ | ||
| export const DEPLOY_UI_TIMEOUT_MS = 120_000; |
There was a problem hiding this comment.
🔍 Deploy UI bound versus runner lifetime
The browser reports an unconfirmed deploy after 120 seconds, while the runner can operate for 900 seconds. Check production provisioning durations to determine how often normal deployments enter the reconciliation flow.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This bound only governs UI waiting/rendering — it never aborts or retries the in-flight write, and the runner keeps working until its own server-side deadline. The derivation comment above the constant already covers the gap: observed provision latency is on the order of one to a few minutes, so 120s is comfortably longer than a healthy provision yet well below the 900s Cloud Run timeout, meaning the UI can never out-wait a real server decision. A deploy that legitimately takes >120s would land in the UNCONFIRMED → reconciliation path by design; whether that's frequent enough to warrant raising the bound is a product-observability question for real production metrics rather than something the code can decide here.
…ailure - switchView: a run abandoned by navigation now dismisses its progress panel and returns the stage to settled content — the previous result when one exists, otherwise the ready state — so returning home can no longer resurrect a panel for a run that no longer exists. - restoreAndShowReplacementFailure: resultsViewMode joins the result snapshot, so a failed refinement or regeneration restores the Summary or file surface the user was inspecting instead of resetting to Summary. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Model switch drops submitted images
During upload preflight, switching to a non-vision model makes updatePromptImageAvailability abort the submitted image uploads. runThinkingPipeline retains those slots but later filters out slots without URLs, so generation silently loses the images.
updatePromptImageAvailability now leaves attachments alone while a run is in flight — the run froze submittedImages before its first await, so dropping promptImages then would abort uploads the slice still expects to commit and silently strip the images it was started with. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.