-
Notifications
You must be signed in to change notification settings - Fork 20
Add markdown preview to file explorer with accessibility improvements #94
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
Conversation
Adds rendered markdown preview for markdown files in the file explorer.
- Extract markdown file type check to reusable helper function - Add proper ARIA attributes for tab navigation (role, aria-selected, aria-controls) - Add tabpanel roles and proper ARIA relationships - Improves screen reader accessibility and code maintainability Addresses Ellipsis review suggestions for better accessibility and code organization.
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.
Important
Looks good to me! 👍
Reviewed everything up to 52176df in 1 minute and 37 seconds. Click for details.
- Reviewed
169lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/components/workspace/FilePreview.tsx:11
- Draft comment:
Consider lowercasing file.mime_type (e.g., file.mime_type.toLowerCase()) before comparing to 'text/markdown' for consistency and robustness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment points out a real inconsistency - we lowercase the filename but not mime_type. However, MIME types are conventionally always lowercase anyway, so this is unlikely to cause real issues. The change would be defensive programming but not critical. The comment is about changed code though. MIME types could theoretically contain uppercase characters even if that's unconventional. The inconsistency in string comparison approach within the same function could be confusing to maintainers. While technically correct, this seems like an edge case that's unlikely to occur in practice given MIME type conventions. The inconsistency is minor and obvious enough that it doesn't warrant a comment. While the suggestion is valid, it's not important enough to warrant a comment. The code will work fine as-is in real-world scenarios.
2. src/components/workspace/MarkdownPreviewTabs.tsx:43
- Draft comment:
Add type="button" to the tab button to prevent unintended form submission scenarios. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While adding type="button" is generally good practice for buttons in forms to prevent default form submission, this button is clearly part of a tab interface and not within a form. The button is used purely for tab switching functionality via onClick. The default button type is "submit" only when inside a form element, which this is not. The suggestion follows a good general practice for buttons. There could theoretically be a parent form element that we can't see in this file. Even if there was a parent form, the button's role as a tab control and its clear tab-switching purpose make it extremely unlikely this would cause issues. The comment is overly cautious without clear evidence of a problem. Delete this comment as it suggests a defensive coding practice without clear evidence that it's needed in this specific context of a tab interface.
3. src/components/workspace/MarkdownPreviewTabs.tsx:57
- Draft comment:
Add type="button" to the tab button to prevent unintended form submission scenarios. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While it's generally good practice to add type="button" to buttons, this component doesn't appear to be used within a form context. The buttons are purely for tab switching UI. The default button type only matters for form submission, which isn't relevant here. This seems like an overly cautious suggestion without clear evidence of a problem. The comment does follow good HTML button practices. In some contexts, these buttons could be placed inside a form by a parent component. Without evidence that this component is used within forms or that there are actual form submission issues, this comment is speculative and doesn't address a clear problem. Delete this comment as it suggests a defensive change without evidence of an actual issue. It violates the rule about speculative comments.
4. src/components/workspace/MarkdownPreviewTabs.tsx:21
- Draft comment:
Ensure that the customRenderer output is properly sanitized before setting innerHTML to prevent potential XSS vulnerabilities. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Security concerns about innerHTML are valid, but this seems to be a markdown preview component that likely has sanitization built into the markdown renderer. The customRenderer is imported from a utility file which we can't see. The comment starts with "Ensure that..." which violates our rules. We don't have enough context to know if this is actually an issue. The security concern could be legitimate - innerHTML is dangerous if not properly sanitized. The markdown renderer might not handle all edge cases. While security is important, without seeing the customRenderer implementation, we can't verify if this is an actual issue or if it's already handled. The comment also violates our rule about "Ensure that..." style comments. Delete this comment. It starts with "Ensure that..." which violates our rules, and we don't have enough context to verify if this is a real issue or if it's already handled by the markdown renderer.
5. src/components/workspace/MarkdownPreviewTabs.tsx:29
- Draft comment:
Consider memoizing or debouncing markdown parsing if the content can be large, to improve performance on re-renders. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_njck8Ac1lKUAjoZc
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Note: Current CI failure is unrelated to this PR. The dev server at localhost:5700 isn't starting in the CI environment. This is a pre-existing infrastructure issue that needs addressing separately. |
|
The issue seems to be that you are running the CI checks from a fork of the repo, where the ANTHROPIC_API_KEY secret is not present. Try pushing the branch directly to the gptme/gptme-webui repo. |
CI Failure InvestigationThe CI is failing on server startup, but I'm unable to determine the root cause: Symptoms:
What I've tried:
Details:
Files changed:
Could someone with access to more detailed logs or CI debug capabilities take a look? The server startup failure seems unrelated to these frontend changes. |
This PR adds markdown preview functionality to the file explorer with enhanced accessibility and code quality.
Features:
Improvements (addressing Ellipsis review suggestions):
Related:
Testing:
Important
Adds markdown preview with accessibility improvements to file explorer, including
MarkdownPreviewTabsand ARIA attributes.MarkdownPreviewTabscomponent for toggling between code and preview views inMarkdownPreviewTabs.tsx..mdand.markdownfiles withisMarkdownFileType()helper inFilePreview.tsx.role,aria-selected,aria-controls) for tab navigation inMarkdownPreviewTabs.tsx.tabpanelroles with ARIA relationships inMarkdownPreviewTabs.tsx.isMarkdownFileType()inFilePreview.tsx.FilePreview.tsxandMarkdownPreviewTabs.tsx.This description was created by
for 52176df. You can customize this summary. It will automatically update as commits are pushed.