Skip to content

Monorepo phase 2 - #9105

Draft
helin24 wants to merge 4 commits into
flutter:mainfrom
helin24:monorepo-phase-2
Draft

Monorepo phase 2#9105
helin24 wants to merge 4 commits into
flutter:mainfrom
helin24:monorepo-phase-2

Conversation

@helin24

@helin24 helin24 commented Aug 31, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution! Please replace this text with:

  • a description of what this PR is changing and why
  • any relevant issues
  • a description of how to verify the change is working
  • screenshots/gif if relevant

Review the contribution guidelines below:

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
  • I've included the required information in the description above.
  • My up-to-date information is in the AUTHORS file.
  • I've updated CHANGELOG.md if appropriate.
Contribution guidelines:
  • See
    our contributor guide and
    the Flutter organization contributor guide
    for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Dart contributions to our repos should follow the Dart style guide and use
    dart format.
  • Java and Kotlin contributions should strive to follow Java and Kotlin best
    practices (discussion).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[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.

Comment on lines +254 to +259
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[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;">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

[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.

Suggested change
<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>"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[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.

Suggested change
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]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant