fix: a wizard that congratulated failed deploys, and a modal missing its own recommendation (beads batch 11) - #193
Conversation
CashPilot-4og — the wizard's final screen was unconditional markup: "You're all set! Your services are being deployed." It said that whether five services deployed, none did, or every deploy returned 403. wizardState.deployed was written twice and read NOWHERE, so a user whose deploys all failed was congratulated and sent to an empty dashboard with no idea anything had gone wrong. Step 4 now renders from what actually deployed, with four distinct outcomes: everything deployed, partly deployed (naming how many did not), nothing deployed though services were selected, and nothing selected in the first place. Exercised in a real DOM against all four. CashPilot-zr9 — the credential modal filtered to required fields, which hid exactly the credentials the app recommends. Bytelixir's session cookie expires in about two hours; its remember_web and xsrf_token cookies last a year and are what stop collection dying the same afternoon. Both are optional, so the credential-health panel said "a longer-lived credential exists for this service" while the modal it links to offered nowhere to put it. Storj was worse: its only field is optional, so the modal rendered no inputs at all — an empty dialog. Every field is rendered now, with optional ones labelled so they are not mistaken for required. storj goes from 0 fields to 1, bytelixir from 1 to 3. Negative controls: reverting the filter fails one test, reverting the wizard copy and its render call fails two more.
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe setup wizard now reports the actual deployment result. Credential modals now show optional collector fields and label them clearly. Tests cover deployment outcomes, field counts, optional labels, and health-panel recommendations. ChangesWizard UI updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/static/js/app.js`:
- Around line 1592-1596: In app/static/js/app.js lines 1592-1596, update the
setup completion flow around _deployToWorkers to track whether deployment was
attempted, showing “Setup saved” when selected services were skipped and
retaining the failure message only when an attempted deployment has zero
successes. In tests/test_beads_batch_11.py lines 44-63, add a regression test
covering selected services reaching step 4 without a deployment attempt and
verify the saved-setup messaging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89a8f5c9-1554-4fe7-9c48-ad57878a6046
📒 Files selected for processing (3)
app/static/js/app.jsapp/templates/setup.htmltests/test_beads_batch_11.py
tests/test_beads_batch_11.py is PR #193's work. A broad `git add -A tests/` swept it onto this branch, where the code it tests does not exist — so it failed here, and I pushed that. Scope check after removing it: this branch touches the two compose files, the cryptography constraint in pyproject/requirements, uv.lock, and the compose-pin test. Nothing else.
From CodeRabbit on this PR, and a real gap in the fix it reviewed. Step 3 offers 'Skip to Summary', so a user can select services and reach the final screen having deployed nothing. My first version keyed only on deployed.length === 0, so it told them 'None of the selected services could be deployed' — sending them to hunt a failure that never happened, which is a different way of being wrong than the bug I was fixing but the same kind. The distinction is whether a deploy was ATTEMPTED, so deployService now records that. Five outcomes, exercised in a real DOM: all deployed You're all set! partial Partly deployed (names how many did not) attempted, 0 ok Nothing was deployed skipped step 3 Setup saved — nothing deployed YET none selected Setup saved Also asserted that the flag is initialised in both the declaration and the reset: a flag nothing resets would carry the first run's state into a second pass through the wizard. Negative control: removing the skip branch fails the test that names it.
|
Fixed — you were right, and it is the same kind of wrongness as the bug this PR set out to fix. Step 3 offers Skip to Summary, so a user can select services and reach step 4 having deployed nothing. Keying only on The distinction is whether a deploy was attempted, so I also asserted the flag is initialised in both the declaration and the wizard reset — a flag nothing resets would carry the first run's state into a second pass. Negative control: removing the skip branch fails the test that names it. 2524 tests, ruff clean. |
… past a CVE (#194) * fix: the quickstart installed 1.4, and bump cryptography past a CVE Issue #188 is someone unable to create an account on v1.4. The cause is the first-run setup-token bug — v1.4.4 has the token gate in deps.py and no token field in onboarding.html, so the owner account cannot be created at all — fixed months ago in v1.11.0. They got that version from us. docker-compose.yml pinned 1.4 while its own header said the images "track the `latest` tag", and told the reader to replace `:latest`, a string the file did not contain anywhere. Anyone checking whether they were current concluded they were. Both compose files now pin 1.11, and the header describes what the file actually does. A test asserts the pin equals the newest released major.minor rather than a fixed number, so this fails the next time it falls behind instead of quietly aging for seven minor versions. Also bumps cryptography to >=50.0.0 for the open Dependabot alert (high: PKCS#7 EnvelopedData decryption exposes a Bleichenbacher oracle). CashPilot uses Fernet, not PKCS#7 EnvelopedData, so the vulnerable path is not one we call — but the constraint allowed the vulnerable range and there is no reason to keep it. Verifying this took three attempts, which is worth recording. I first concluded v1.4 had no token gate at all, from `git show v1.4:... | grep -c` returning 0 — the tag is v1.4.4, `git show` produced NOTHING, and a count of zero on empty input reads exactly like a confirmed absence. The byte counts in the corrected check are there so a later reader can tell the two apart. * fix: complete the CVE fix in the lockfile, and stop the pin test skipping in CI All three from CodeRabbit on this PR, all correct. The cryptography bump was INCOMPLETE. Dependabot's alert names uv.lock as the manifest, and the lock still declared >=48.0.1 and pinned cryptography 48.0.1 — inside the vulnerable range — so changing pyproject.toml and requirements.txt alone would have left the alert open while looking fixed. Regenerated with `uv lock --upgrade-package cryptography`: now 50.0.0, and the diff touches nothing else. The drift test would have SKIPPED in CI. actions/checkout does not fetch tags, so `git tag` returns nothing there and the test hit its own pytest.skip — decorative in the one place it is meant to run, which is the same shape of defect as the stale pin it guards. CI now fetches tags, and the test FAILS rather than skips when CI has none, so a regression in the workflow cannot quietly disarm it. The pin scan now includes commented lines. docker-compose.fleet.yml's commented remote-worker block is a template users uncomment and run, so a stale pin there ships an old image just as surely as a live one — and my first version stripped comments precisely to ignore it. Verified locally: cryptography 50.0.0 exists and installs cleanly; the suite ran against 48.0.0 here because this venv has no pip, while CI installs from requirements.txt and exercises 50.0.0. Negative control: planting a stale pin in the commented template now fails the test. * chore: remove a test file that belongs to another branch tests/test_beads_batch_11.py is PR #193's work. A broad `git add -A tests/` swept it onto this branch, where the code it tests does not exist — so it failed here, and I pushed that. Scope check after removing it: this branch touches the two compose files, the cryptography constraint in pyproject/requirements, uv.lock, and the compose-pin test. Nothing else.
1 participant
Two screens that told the user something untrue.
4og— the wizard congratulated you for deployments that never happened.The final screen was unconditional markup: "You're all set! Your services are being deployed." Shown whether five deployed, none did, or every deploy returned 403.
wizardState.deployedwas written twice and read nowhere, so a user whose deploys all failed was congratulated and sent to an empty dashboard with no idea anything had gone wrong.Now rendered from what actually deployed, exercised in a real DOM across all four cases:
zr9— the modal hid exactly the credentials the app recommends.Bytelixir's session cookie expires in ~2 hours; its
remember_webandxsrf_tokencookies last a year and are what stop collection dying the same afternoon. Both are optional, so the credential-health panel said "a longer-lived credential exists for this service" while the modal it links to offered nowhere to put it.Storj was worse — its only field is optional, so the modal rendered no inputs at all.
Optional fields are labelled so they aren't mistaken for required.
Verification: 2521 tests, 95.14% coverage, ruff clean, JS parses. Negative controls: reverting the field filter fails 1 test; reverting the wizard copy and its render call fails 2 more.
Summary by CodeRabbit
New Features
Bug Fixes