Monorepo phase 2 - #9105
Conversation
There was a problem hiding this comment.
Code Review
This pull request restructures the repository into a monorepo by introducing a dedicated dart/ directory containing the Dart IntelliJ plugin codebase, build configurations, and several custom AI agent skills. Key feedback focuses on critical CI/CD issues, including the incorrect placement of GitHub Actions workflows in a subdirectory where they will be ignored, and an invalid regular expression in the presubmit check. Additionally, there are concerns regarding a potential XSS vulnerability in the triage dashboard, fragile string matching in the LSP patch script, and performance inefficiencies in the issue triage scripts.
| if [ -f "$REPORT" ]; then | ||
| echo "Comparing baseline against report in $REPORT" | ||
| grep "^*" "$REPORT" | grep -v "com\.intellij\.platform\.dartlsp" | sort > current_issues.tmp || true | ||
|
|
There was a problem hiding this comment.
[MUST-FIX] The regular expression ^* used in grep is incorrect. In regular expressions, * is a quantifier meaning "zero or more of the preceding character" (which is ^, the start of the line). This causes grep to match every single line in the report, rendering the baseline comparison ineffective.\n\nTo match a literal asterisk at the start of the line, you must escape it as ^\* or use ^[*].
grep "^\*" "$REPORT" | grep -v "com\.intellij\.platform\.dartlsp" | sort > current_issues.tmp || true| @@ -0,0 +1,194 @@ | |||
| name: presubmit | |||
There was a problem hiding this comment.
[MUST-FIX] GitHub Actions only detects and executes workflow files located in the .github/workflows/ directory at the root of the repository. Workflows placed in subdirectories like dart/.github/workflows/ will be completely ignored by GitHub.\n\nPlease move all workflow files to the root .github/workflows/ directory and adjust the paths (such as working-directory and script paths) to account for the new root-relative locations.
| target_line = " if (!ProjectFileIndex.getInstance(project).isInContent(file)) return false\n" | ||
| import_line = "import com.intellij.openapi.roots.ProjectFileIndex\n" | ||
| modified = False | ||
| if target_line in lsp_server_impl_content: | ||
| lsp_server_impl_content = lsp_server_impl_content.replace(target_line, "") | ||
| modified = True |
There was a problem hiding this comment.
[CONCERN] The exact string matching for target_line is fragile and will fail silently if the upstream formatting of LspServerImpl.kt changes slightly (e.g., different indentation, spacing, or line endings).\n\nUsing a regular expression replacement with re.sub is much more robust and less prone to breaking during future upstream syncs.
target_pattern = r'^\s*if\s*\(!ProjectFileIndex\.getInstance\(project\)\.isInContent\(file\)\)\s*return\s+false\r?\n'\n import_line = "import com.intellij.openapi.roots.ProjectFileIndex\n"\n modified = False\n if re.search(target_pattern, lsp_server_impl_content, re.MULTILINE):\n lsp_server_impl_content = re.sub(target_pattern, "", lsp_server_impl_content, flags=re.MULTILINE)\n modified = True| "> | ||
| <div style="display: flex; justify-content: space-between; align-items: flex-start; gap: 1rem;"> | ||
| <h4 style="font-size: 0.925rem; font-weight: 500; font-family: 'Outfit', sans-serif;"> | ||
| <a href="https://github.com/${d.repo}/issues/${d.id}" target="_blank" style="color: #f3f4f6; text-decoration: none; display: inline-flex; align-items: center; gap: 0.25rem;"> |
There was a problem hiding this comment.
[CONCERN] details.title is inserted directly into the HTML template without sanitization. If an issue title contains malicious HTML or JavaScript, this could lead to a Cross-Site Scripting (XSS) vulnerability when rendering the triage summary page.\n\nSince DOMPurify is already loaded in this dashboard, please wrap details.title in DOMPurify.sanitize() to ensure it is safely rendered.
| <a href="https://github.com/${d.repo}/issues/${d.id}" target="_blank" style="color: #f3f4f6; text-decoration: none; display: inline-flex; align-items: center; gap: 0.25rem;"> | |
| <span style="color: #818cf8; font-family: monospace;">#${d.id}</span> ${DOMPurify.sanitize(details.title)} |
| assignees = [] | ||
| if raw_assignees: | ||
| name_map = {} | ||
| git_log = run_cmd(["git", "log", "--format=%an <%ae>"]) |
There was a problem hiding this comment.
[CONCERN] Running git log without any limit can be extremely slow and memory-intensive on repositories with large commit histories.\n\nConsider limiting the log depth (e.g., using -n 1000 or limiting by date) to ensure the script remains fast and efficient.
| git_log = run_cmd(["git", "log", "--format=%an <%ae>"]) | |
| git_log = run_cmd(["git", "log", "-n", "1000", "--format=%an <%ae>"]) |
| print(f"Found {len(untriaged_issues)} untriaged issues. Fetching comments for up to {limit}...") | ||
|
|
||
| enriched_issues = [] | ||
| for i, issue in enumerate(untriaged_issues[: limit]): |
There was a problem hiding this comment.
[CONCERN] Fetching comments sequentially for each issue using gh issue view in a loop is highly inefficient. For 50 issues, this results in 50 separate process spawns and network requests, which will be extremely slow and could trigger rate limits.\n\nConsider optimizing this by fetching all issues and their comments in a single GraphQL query using gh api graphql.
d7ec295 to
cdf443b
Compare
cdf443b to
cfdeb82
Compare
Thanks for your contribution! Please replace this text with:
Review the contribution guidelines below:
AUTHORSfile.CHANGELOG.mdif appropriate.Contribution guidelines:
our contributor guide and
the Flutter organization contributor guide
for general expectations for PRs.
dart format.practices (discussion).