-
Notifications
You must be signed in to change notification settings - Fork 36
Extract sanitizeLabelContent to dedicated helper module #3142
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
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot format recompile |
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.
Pull Request Overview
This PR extracts the sanitizeLabelContent function into a dedicated module and adds comprehensive test coverage. The function sanitizes label content by removing control characters, ANSI escape codes, HTML special characters, and neutralizing @mentions.
- Extracted duplicated
sanitizeLabelContentfunction into a reusable module - Added 24 comprehensive test cases covering edge cases and sanitization rules
- Updated
create_issue.cjsandadd_labels.cjsto import from the new module
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/workflow/js/sanitize_label_content.cjs | New module containing the extracted sanitization function with order of operations changed |
| pkg/workflow/js/sanitize_label_content.test.cjs | Comprehensive test suite for the sanitization function |
| pkg/workflow/js/create_issue.cjs | Removed inline function, now imports from new module |
| pkg/workflow/js/add_labels.cjs | Removed inline function, now imports from new module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Remove ANSI escape sequences FIRST (before removing control chars) | ||
| sanitized = sanitized.replace(/\x1b\[[0-9;]*[mGKH]/g, ""); | ||
| // Then remove control characters (except newlines and tabs) | ||
| sanitized = sanitized.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, ""); |
Copilot
AI
Nov 4, 2025
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.
The order of ANSI code removal and control character removal has been reversed from the original implementation. In the previous code (create_issue.cjs and add_labels.cjs), control characters were removed first (line 8), then ANSI codes (line 9). This change could alter behavior since ANSI escape sequences begin with \x1b (which is a control character in range \x00-\x1F). Removing control characters first would strip the initial \x1b, making ANSI code removal ineffective. The new order is correct, but this represents a behavior change that fixes a bug in the original code. Consider documenting this fix in the PR description or commit message.
| @@ -0,0 +1,142 @@ | |||
| import { describe, it, expect } from "vitest"; | |||
Copilot
AI
Nov 4, 2025
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.
This file uses ES6 import syntax but has a .cjs extension and uses require() for the actual module import (line 4). The file should either use all ES6 imports with .mjs extension, or use all CommonJS require() statements with .cjs extension. Change line 1 to const { describe, it, expect } = require(\"vitest\"); to be consistent with the CommonJS format.
| import { describe, it, expect } from "vitest"; | |
| const { describe, it, expect } = require("vitest"); |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add sanitize_label_content.cjs to embedded JavaScript sources - Convert createIssueScript and addLabelsScript to bundled scripts - Update GetJavaScriptSources to include sanitize_label_content.cjs - Update all usages to call getter functions for proper bundling - Fix test file to keep ES6 import for vitest (not CommonJS) This ensures the helper function is properly inlined in compiled workflows instead of leaving require() statements that would fail at runtime. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
…ize-label-content # Conflicts: # .github/workflows/smoke-copilot.firewall.lock.yml # .github/workflows/smoke-opencode.lock.yml # pkg/workflow/js.go
Plan: Extract sanitizeLabelContent to Dedicated Helper File
This PR extracts the duplicate
sanitizeLabelContentfunction fromcreate_issue.cjsandadd_labels.cjsinto a dedicated helper file following the repository pattern of "1 javascript helper per file".Changes implemented:
pkg/workflow/js/sanitize_label_content.cjswith the functionpkg/workflow/js/create_issue.cjsto import from new helperpkg/workflow/js/add_labels.cjsto import from new helperBundler Configuration:
sanitize_label_content.cjstoGetJavaScriptSources()mapcreateIssueScriptandaddLabelsScriptfrom direct embeds to bundled scriptsgetCreateIssueScript(),getAddLabelsScript())require()statements)Summary:
sanitize_label_content.cjs- Dedicated helper with proper JSDoc documentationsanitize_label_content.test.cjs- 28 comprehensive tests covering all sanitization scenarioscreate_issue.cjs- Imports helper, removed duplicate (13 lines reduced)add_labels.cjs- Imports helper, removed duplicate (13 lines reduced)pkg/workflow/js.go- Configured bundler to inline the helperAll tests passing ✅
Original prompt
sanitizeLabelContentto dedicated helper file #3132✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.