Skip to content

Improvments#568

Open
v-aidaba wants to merge 9 commits intomicrosoft:mainfrom
v-aidaba:improvments
Open

Improvments#568
v-aidaba wants to merge 9 commits intomicrosoft:mainfrom
v-aidaba:improvments

Conversation

@v-aidaba
Copy link
Copy Markdown

@v-aidaba v-aidaba commented Apr 8, 2026

No description provided.

Comment thread src/mcp/tools/certification.ts Outdated

return formatResults(allChecks);
} catch (error) {
return `❌ Error checking certification readiness: ${error.message}`;
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.

error might be unknown.
You can use something like this: (error instanceof Error) ? error.message : String(error). Please fix this in other codebase of this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I replaced the error handling with a safer check.

Comment thread src/mcp/McpServer.ts Outdated
servers: {
pbiviz: {
command: "npx",
args: ["pbiviz", "mcp"]
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.

It is edge case, but still, if user does not have tools locally or globally, npx will search for pbiviz in npm (it is not exist there). So I think, better to use following args:
"args": ["-y", "powerbi-visuals-tools", "mcp"]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated to npx -y powerbi-visuals-tools mcp

"servers": {
"pbiviz": {
"command": "npx",
"args": ["pbiviz", "mcp"]
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.

It is edge case, but still, if user does not have tools locally or globally, npx will search for pbiviz in npm (it is not exist there). So I think, better to use following args:
"args": ["-y", "powerbi-visuals-tools", "mcp"]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated to npx -y powerbi-visuals-tools mcp.

Comment thread src/mcp/tools/visualInfo.ts Outdated

return formatVisualInfo(info);
} catch (error) {
return `❌ Error reading visual info: ${error.message}`;
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.

error might be unknown.
You can use something like this: (error instanceof Error) ? error.message : String(error). Please fix this in other codebase of this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed - I replaced the error handling

Comment thread src/mcp/tools/vulnerabilities.ts Outdated

return formatResults([...codeResults, ...eslintResults]);
} catch (error) {
return `❌ Error scanning project: ${error.message}`;
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.

error might be unknown.
You can use something like this: (error instanceof Error) ? error.message : String(error). Please fix this in other codebase of this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

Comment thread src/mcp/tools/vulnerabilities.ts Outdated

for (const entry of entries) {
const fullPath = path.join(dir, entry.name);
if (entry.isDirectory() && entry.name !== 'node_modules') {
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.

don't we want to exclude '.tmp' as well, samee as you do in another places?
if (entry.isDirectory() && entry.name !== 'node_modules' && entry.name !== '.tmp') {

And actually, I don't like the fact that you have similar getSourceFiles functions in different files. Define it in one single place (some utils file) and reuse it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I updated it to exclude .tmp as well, and moved the duplicated getSourceFiles logic into a shared util so all checks reuse the same implementation.

Comment thread src/mcp/tools/certification.ts Outdated
const checks: CertificationCheck[] = [];

for (const { file, description } of REQUIRED_FILES) {
const exists = fs.existsSync(path.join(rootPath, file));
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.

RECOMMENDED_FILES use upper-case strings, but what if another developer have lower-case. That will work in windows, but for OS cross-platform support better to add case-insensitive method to utils file and use it for such case.

Method example (or do better):


/**
 * Case-insensitive version of fs.existsSync.
 * Reads the parent directory listing and compares filenames in lowercase.
 */
export function existsIgnoreCase(filePath: string): boolean {
    const dir = path.dirname(filePath);
    const name = path.basename(filePath).toLowerCase();
    try {
        const entries = fs.readdirSync(dir);
        return entries.some(entry => entry.toLowerCase() === name);
    } catch {
        return false;
    }
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added a shared case-insensitive file existence check in utils and updated the recommended file validation to use it.

Comment thread src/CommandManager.ts Outdated
public static async mcpInit(rootPath: string) {
await initMcpConfig(rootPath);
}
} No newline at end of file
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.

Please end all files with a new line.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added

Comment thread src/mcp/tools/availableApis.ts Outdated
name: 'fetchMoreData',
category: 'data',
description: 'Enables loading additional data in chunks. Essential for large datasets that exceed the initial row limit.',
minApiVersion: '2.6.0', codePatterns: [/fetchMoreData/], example: `// In update method
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.

keep structure consistently, split fields by a new line

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I updated the structure to keep it consistent and split the fields into separate lines

Comment thread src/mcp/McpServer.ts
@@ -0,0 +1,166 @@
/*
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.

Can we add some tests for new MCP features?

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.

2 participants