-
Notifications
You must be signed in to change notification settings - Fork 21
Redesign error reporting to not show false texts #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Redesign error reporting to not show false texts #790
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamkankovsky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a screenshot or recording to the bug report to help explain the issue.
- It might be helpful to include the application version in the bug report.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
cf004cb
to
60a1e06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamkankovsky @garrett my preference it to completely remove the inline editing of the journal from the error report dialog. What's your preference?
Thanks for working on this! I agree. The logs shouldn't be editable. I think the logs shouldn't even be visible (at least by default). I've said it before and still think it's the case:
The bug reporting flow is still atrocious overall (which isn't really our fault, as we need more people and also outside help), and if we really want to make sure people are able to report bugs, we really need to: talk about it, have someone dedicated to solve this, and (critically) get support outside of our team to make sure there's a place to report bugs. As it stands, Anaconda actually does nothing to help "report" an issue. All Anaconda does is to basically provide a link to bugzilla, where an account is needed. It's still impossible to log into this account (Bugzilla) if you need email access and/or 2FA to sign into the bug reporting tool — especially if you need to first create an account. The minimal fixes right now would be to:
I think we need to have a two-pronged approach:
These are possible next-steps. It is on-topic overall, but of scope of this particular PR: This still does not address the giant issues with the entire concept or flow with online reporting. To address those, we'd need more support not just within the team, but outside as well. (We'd need a place and a method to actually report issues without the friction of needing an account, for example. And we'd need someone who can make that happen not just on our side, but on the issue tracker side too.) One alternative to having online reporting is to have offline reporting. That would require having a read-write media where any relevant logs are stored and a document explaining how to report the issue, such as an HTML file (README.html or REPORT.html or something obviously named) that has formatting with instructions and a link to create a new issue. This would allow someone to report what happened in Anaconda when they have access to a computer (either after a successful installation, on a different computer, or using an existing OS (for example Windows) that they haven't wiped out with installation). These files would need to be written to the root of the media or in a top-level sub-folder if there are more than 2 files. This could be the boot install media — or even a separate USB stick, if the install media is write-only. A third method would be to use a QR code to help report an issue from their phone, but that would be tricky, as QR codes can support a lot of text, but wouldn't be able to support something as huge as logs. So it probably wouldn't really be much more useful than what we already have, unless we somehow get clever about it. The above three methods are not mutually exclusive. We could, for example, improve the online reporting workflow and also create the offline reporting workflow. Offline reporting would be especially useful when there is not internet access on the computer (no access to wifi, missing driver support, no network access), on non-live media installs, on installations where screen resolution is quite limited, and during screenreader usage (so someone can access issue reporting in a better environment). But this all requires:
I think there is value in this, especially the offline approach. We, as a team, could handle the offline method all by ourselves without other teams being involved. A proper online reporting approach has a lot of gotchas and depends on so many other variables to get right (other teams, APIs, submitting the file, optionally editing the file somehow, anonymous reporting, etc.). |
@garrett @adamkankovsky My preference would be to remove the journal from the error reporting dialog and follow the two designs from Garrett listed below. |
60a1e06
to
4ebe4d1
Compare
For making the 'Review and edit the file' an actual link, we would need to introduce sometihng like cockpit-terminal into anaconda. We currently do not have it, so let's convert it to plain text, and disucss with @garrett the possibility for follow up for introducing the terminal functionality. |
4ebe4d1
to
4148fca
Compare
4148fca
to
afd058d
Compare
afd058d
to
991371b
Compare
For editing, we'd probably want to use a variant of the code editor component from PatternFly, rather than the terminal: (Editing in the terminal is a bit problematic, as the browser does eat certain keyboard shortcuts and keypresses before they get passed to terminal emulation. This affects all the common in-terminal editors like vim, emacs, nano, etc. And using a browser-based editor would be a simpler, more intuitive editor for most people too.) |
Now that we do not show the file editor within the UI there is not reason for seperate /tmp/webui.log, users can attach the existing anaconda files in /tmp/ We should also remove all relevant code that changes the /tmp/journal to /tmp/webui.log as there is nothing that differs now between these two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamkankovsky - I've reviewed your changes - here's some feedback:
- Consider extracting the connected/disconnected helper logic (List vs HelperTextItem) into a small subcomponent to keep BZReportModal slimmer and improve readability.
- Review whether replacing the inline log textarea with instructions to manually attach the log file still satisfies the intended user workflow and accessibility requirements.
- Double-check that the smaller modal variant provides adequate space for all content (especially the 3-item instruction list and any dynamic details) without causing scroll or layout issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the connected/disconnected helper logic (List vs HelperTextItem) into a small subcomponent to keep BZReportModal slimmer and improve readability.
- Review whether replacing the inline log textarea with instructions to manually attach the log file still satisfies the intended user workflow and accessibility requirements.
- Double-check that the smaller modal variant provides adequate space for all content (especially the 3-item instruction list and any dynamic details) without causing scroll or layout issues.
## Individual Comments
### Comment 1
<location> `src/components/Error.jsx:182` </location>
<code_context>
- fieldId={idPrefix + "-bz-report-modal-review-log"}
- label={_("Log")}
- >
- <TextArea
- value={logContent}
- onChange={(_, value) => setLogContent(value)}
</code_context>
<issue_to_address>
Orphaned log-editing code remains after removing the TextArea
Please remove unused state variables (`logContent`, `setLogContent`, `preparingReport`) if the log editor is no longer needed, or restore the editor if manual attachment is not a full replacement.
</issue_to_address>
### Comment 2
<location> `src/components/Error.jsx:188` </location>
<code_context>
+ </List>
+ )
+ : (
+ <HelperTextItem icon={<DisconnectedIcon />}>
+ {isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive}
+ </HelperTextItem>
</code_context>
<issue_to_address>
Wrap `HelperTextItem` in a `HelperText` container
Using `HelperTextItem` outside `<HelperText>` may cause styling or accessibility issues. Please wrap it in a `<HelperText>` for consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
: (
<HelperTextItem icon={<DisconnectedIcon />}>
{isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive}
</HelperTextItem>
)}
=======
: (
<HelperText>
<HelperTextItem icon={<DisconnectedIcon />}>
{isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive}
</HelperTextItem>
</HelperText>
)}
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fieldId={idPrefix + "-bz-report-modal-review-log"} | ||
label={_("Log")} | ||
> | ||
<TextArea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Orphaned log-editing code remains after removing the TextArea
Please remove unused state variables (logContent
, setLogContent
, preparingReport
) if the log editor is no longer needed, or restore the editor if manual attachment is not a full replacement.
src/components/Error.jsx
Outdated
: ( | ||
<HelperTextItem icon={<DisconnectedIcon />}> | ||
{isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive} | ||
</HelperTextItem> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Wrap HelperTextItem
in a HelperText
container
Using HelperTextItem
outside <HelperText>
may cause styling or accessibility issues. Please wrap it in a <HelperText>
for consistency.
: ( | |
<HelperTextItem icon={<DisconnectedIcon />}> | |
{isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive} | |
</HelperTextItem> | |
)} | |
: ( | |
<HelperText> | |
<HelperTextItem icon={<DisconnectedIcon />}> | |
{isBootIso ? networkHelperMessageBootIso : networkHelperMessageLive} | |
</HelperTextItem> | |
</HelperText> | |
)} |
I am proposing a new design here; https://issues.redhat.com/browse/INSTALLER-4197?focusedId=27399623&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-27399623 The reason is that the current implementation is missing critical information. Waiting for @garrett s review. |
c4feedf
to
fc9056a
Compare
Do not show the inline editor, rather give the user a hint that the files might contain sensitive information, and they can choose to not upload or edit by their own means. Since inline editing is no longer happening let's remove all relevant code to inline editing. Also do not create another /tmp/webui.log file, use the /tmp/journal.log that anaconda creates when CopyLogsWithTasks is run in post install.
Instead of this multi-layer wrapper, which is not the Patternfly recommendation.
fc9056a
to
6699b32
Compare
No description provided.