Fix Add Data wizard Step 3 "Verify now" error recovery loop#2359
Fix Add Data wizard Step 3 "Verify now" error recovery loop#2359
Conversation
When "Verify now" is clicked on Step 3, verification polling now starts inline on the same page via IngestionVerificationPanel instead of dropping the user back to Step 2. Local state tracks whether the user explicitly triggered verification from Step 3, so verification that auto-started in Step 2 does not interfere with the initial Step 3 view. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
✅ UI Smoke Test Review —
|
| Total | Passed | Failed | Flaky | Skipped |
|---|---|---|---|---|
| 11 | 11 | 0 | 0 | 0 |
Individual test results
| Status | Test | Duration |
|---|---|---|
| ✅ | onboarding user reaches the connect entrypoint from the welcome screen | 675ms |
| ✅ | add data entrypoint exposes the technology picker and step 2 | 1653ms |
| ✅ | metrics user connects, picks a metric, and gets a line chart-ready result | 2089ms |
| ✅ | traces user opens a trace and pivots from service map context into Query Lab | 2178ms |
| ✅ | security-focused user validates auth tab switching before submitting credentials | 665ms |
| ✅ | ops user confirms connection guardrails and can reset back to the landing state | 2927ms |
| ✅ | query lab preserves query text and results after navigating to Console and back | 2478ms |
| ✅ | logs explorer route is available and runs a logs query | 2211ms |
| ✅ | logs landing page dimension drill-down opens query lab with selected filter | 2095ms |
| ✅ | task manager, ILM, and templates pages render with headings | 2093ms |
| ✅ | pages have no axe accessibility violations | 15617ms |
📸 Screenshot Preflight
Result: ✅ Clean — no errors detected
No console errors, page errors, or UI alerts detected.
💡 Recommendations
- ✨ All smoke tests pass and no UI errors detected. Ready for review.
📎 Full diagnostics and screenshots: workflow run
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds three props— Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
QA Exploration FindingsI have completed the interactive testing and screenshot evaluation across multiple pages (Dashboards, Add Data, Cluster Overview) and viewports.
No visual layout regressions or toolbar height mismatches (inputs/comboboxes are consistently 40px) were found during the exploration. No unexpected console errors occurred during the wizard steps. What is this? | From workflow: Explore: Pull Request Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the unresolved review feedback with a surgical fix.
Validation run
Also observed PR check runs; relevant repo checks were green (no straightforward CI failure to fix). What is this? | From workflow: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Handle verification error state by resetting and restarting polling, and disable Verify now when the Elasticsearch connection is unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the unresolved review feedback with a minimal code change in
Validation
Also checked PR checks and comments; no straightforward external CI failure required additional code changes in this run. What is this? | From workflow: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the remaining unresolved review thread with a surgical code change.
Tests run
What is this? | From workflow: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@peek/src/components/addData/AddDataStepSuccess.tsx`:
- Around line 81-85: The showInlineVerification boolean currently hides the
inline panel when verification.status === "error"; change its logic so the panel
remains visible for the "error" state (i.e., include "error" as an allowed
status) by updating the condition that builds showInlineVerification (which
references verifyClicked and verification.status) so it does not exclude
"error", and apply the same change to the duplicate logic around the other
occurrence (lines with the same condition near the IngestionVerificationPanel
usage) so the IngestionVerificationPanel stays mounted for error states while
leaving any separate retry CTA handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22e96797-116e-4717-ac2c-cadc29e67694
📒 Files selected for processing (1)
peek/src/components/addData/AddDataStepSuccess.tsx
Keep Step 3 inline ingestion verification mounted for the error state so users can see troubleshooting context after a failed Verify now action. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
peek/src/components/addData/AddDataStepSuccess.tsx (1)
100-100: 🧹 Nitpick | 🔵 Trivial
text.secondarycontrast on Dark Mode flagged in linked issue.Line 100 uses
color="text.secondary". Per issue#2358's secondary objective, this palette token has insufficient contrast in Dark Mode. Not strictly blocking this PR, but noting it's still unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@peek/src/components/addData/AddDataStepSuccess.tsx` at line 100, In AddDataStepSuccess.tsx the Typography element uses color="text.secondary" which fails contrast in dark mode; update the component (the Typography instance in AddDataStepSuccess) to avoid text.secondary on dark by either switching to color="text.primary" or applying a theme-aware override (e.g., useTheme or sx) so that in dark mode it uses theme.palette.text.primary while keeping text.secondary in light mode; modify the Typography props accordingly to ensure sufficient contrast across themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@peek/src/components/addData/AddDataStepSuccess.tsx`:
- Around line 81-82: Add a retry CTA inside the IngestionVerificationPanel when
verification.status === "error" so users can re-attempt verification without
closing the panel: update the panel to render a "Retry" button in its error
state that calls the same verification trigger used by the outer "Verify now"
button (the existing handler tied to verifyClicked/verification flow), and
expose an onRetry prop on IngestionVerificationPanel if needed so
AddDataStepSuccess can pass the handler; ensure the button is shown only when
verification.status === "error" and reuses the existing verification logic
rather than duplicating behavior.
---
Outside diff comments:
In `@peek/src/components/addData/AddDataStepSuccess.tsx`:
- Line 100: In AddDataStepSuccess.tsx the Typography element uses
color="text.secondary" which fails contrast in dark mode; update the component
(the Typography instance in AddDataStepSuccess) to avoid text.secondary on dark
by either switching to color="text.primary" or applying a theme-aware override
(e.g., useTheme or sx) so that in dark mode it uses theme.palette.text.primary
while keeping text.secondary in light mode; modify the Typography props
accordingly to ensure sufficient contrast across themes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dc37b8a-01f1-442f-b7a6-e0266ca7066d
📒 Files selected for processing (1)
peek/src/components/addData/AddDataStepSuccess.tsx
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Implemented a small follow-up fix for Step 3 verification error recovery and pushed it to this PR. Addressed review threads with code changes
Change made:
Replied instead of fixing
Validation run
What is this? | From workflow: Address PR Review Feedback Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
I have explored the UI interactively and reviewed the screenshots to check for layout issues, visual regressions, and broken interactions. Testing performed:
Findings: What is this? | From workflow: Explore: Pull Request Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Clicking "Verify now" on Step 3 called
onBack(), dropping the user to Step 2 with no feedback. The user had to guess what happened and click "Continue" to return.Changes
AddDataStepSuccess.tsx: "Verify now" now triggersverification.startPolling()/checkNow()and rendersIngestionVerificationPanelinline on Step 3AddDataPage.tsx: Passesverification,connectionAvailable,signalExpectationto Step 3Local
verifyClickedstate gates the inline panel so that verification auto-started from Step 2 doesn't replace the initial Alert on arrival at Step 3.Fixes #2358
The body of this PR is automatically managed by the workflow runtime.
The body of this PR is automatically managed by the Update PR Body workflow.