Skip to content

feat: Fix a code issue from finding or known function#1107

Open
kgilpin wants to merge 4 commits intomasterfrom
feat/fix-code-issue-from-finding-or-known-function
Open

feat: Fix a code issue from finding or known function#1107
kgilpin wants to merge 4 commits intomasterfrom
feat/fix-code-issue-from-finding-or-known-function

Conversation

@kgilpin
Copy link
Contributor

@kgilpin kgilpin commented Jan 15, 2026

Adds an inline markdown icon to finding rows that opens a comprehensive finding report as a
markdown preview with clickable file links.

Intended usage:

  • Paste into an AI Assistant to fix the problem.

Key features:

  • Opens finding synopsis as markdown preview with workspace-relative clickable links
  • Shows complete rule context (ID, description, documentation)
  • Includes stack trace, SQL queries, and participating events without truncation
  • Automatic line number validation (strips invalid values)

Technical:

  • Created RawEventData type definitions for proper typing of snake_case event data
  • Fixed type mismatches in Finding interface fields

Example

Screenshot 2026-01-15 at 2 41 55 PM

N plus 1 SQL query

This AppMap matches the following analysis rule: n-plus-one-query

Finds occurrences of a query being repeated within a loop.

Impact Domain: Performance | Occurrences: 8

Finding Details

<templates>/UsersKgilpinSourceLandOfAppsDjangoOscar_Issue2484SrcOscarTemplatesOscarCatalogueBrowseHtml#render[218]
contains 8 occurrences of SQL: SELECT "partner_stockrecord"."id",
"partner_stockrecord"."product_id", "partner_stockrecord"."partner_id",
"partner_stockrecord"."partner_sku", "partner_stockrecord"."price_currency",
"partner_stockrecord"."price", "partner_stockrecord"."num_in_stock",
"partner_stockrecord"."num_allocated", "partner_stockrecord"."low_stock_threshold",
"partner_stockrecord"."date_created", "partner_stockrecord"."date_updated" FROM
"partner_stockrecord" WHERE "partner_stockrecord"."product_id" = ? LIMIT ?

Group Summary

SELECT "partner_stockrecord"."id", "partner_stockrecord"."product_id",
"partner_stockrecord"."partner_id", "partner_stockrecord"."partner_sku",
"partner_stockrecord"."price_currency", "partner_stockrecord"."price",
"partner_stockrecord"."num_in_stock", "partner_stockrecord"."num_allocated",
"partner_stockrecord"."low_stock_threshold", "partner_stockrecord"."date_created",
"partner_stockrecord"."date_updated" FROM "partner_stockrecord" WHERE
"partner_stockrecord"."product_id" = ? LIMIT ?

Source Location

src/oscar/apps/partner/strategy.py:199

Stack Trace

Code Event Context

  • commonAncestor: .UsersKgilpinSourceLandOfAppsDjangoOscar_Issue2484SrcOscarTemplatesOscarCatalogueBrowseHtml#render

AppMap

tmp/appmap/requests/1767987869_01673_http_127_0_0_1_8000_en-gb_catalogue_.appmap.json

References

@kgilpin kgilpin self-assigned this Jan 15, 2026
@kgilpin kgilpin requested a review from dividedmind January 15, 2026 19:43
@kgilpin kgilpin force-pushed the feat/fix-code-issue-from-finding-or-known-function branch 2 times, most recently from 043c2cd to f753c99 Compare January 15, 2026 20:09
if (!result) return;

return result.resourceUri;
return uriMap.get(result.detail!);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note we could instead just rename the property on the quick pick item so that wouldn't clash with the resourceUri proposal, eg. appmapResourceUri — showQuickPick accepts anything that extends a QuickPickItem and would just ignore unknown properties. (I find it mildly surprising that including it caused it to fail anyway — it seems it's just used to provide default label, description and icon if not explicitly set?)

: path.join(workspaceFolder.uri.fsPath, filePath);

// Convert to workspace-relative path for both display and link
const relativePath = vscode.workspace.asRelativePath(absolutePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will include workspace folder name if there are multiple folders. It's great for display, but I wonder: won't this break the link? I'm not sure how VSCode handles markdown links in this case.

In addition, what happens if the file is absolute and somehow in a different workspace folder than provided in the argument? Should this function validate it?

parts.push('');
}

return parts.join('\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps surprisingly, in v8 appending to a string is faster than pushing parts and then concatenating — strings are optimized for this case and this avoids the overhead of creating many objects. It might also be more readable.

}

// AppMap file (workspace-relative path as clickable link)
const workspaceRelativePath = vscode.workspace.asRelativePath(resolvedFinding.appMapUri.fsPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

This potentially has the same issue as above when there are multiple folders.

await vscode.window.showTextDocument(document);

// Then open the markdown preview
await vscode.commands.executeCommand('markdown.showPreview', document.uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we don't need to await this one; IMO it's better if opening preview fails silently than showing a potentially confusing error message.

if (!result) return;

return uriMap.get(result.detail!);
return result.resourceUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

This just reverts the previous commit?

if (finding.relatedEvents) {
state.traceFilter = finding.relatedEvents.map((evt) => ['id', evt.id].join(':')).join(' ');
const rawRelatedEvents = finding.relatedEvents as unknown as RawEventData[];
state.traceFilter = rawRelatedEvents.map((evt) => ['id', evt.id].join(':')).join(' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

The code in this function only uses id in events which should be fine either way. I don't understand the reason for this change.

*
* Note: The Finding interface from @appland/scanner incorrectly types some fields as Event
* when they are actually raw JSON objects. This type provides the correct structure.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

If we ever fix it there to actually return what is advertised in the typing then this code will break (BTW, we should probably open an issue or PR there). It would be more robust to have a wrapper that fixes up the objects instead.

kgilpin and others added 2 commits January 16, 2026 10:10
The previous commit (f753c99) accidentally restored AppMapQuickPickItem.ts
which had been deleted to remove dependency on the unstable
quickPickItemResources proposed API.

This commit restores the fix from e3857ae:
- Deletes AppMapQuickPickItem.ts
- Updates promptForAppMap to use Map-based URI lookup instead
- Updates tests to use plain vscode.QuickPickItem with detail property

This resolves the "Open Code Object in AppMap" error that was occurring
due to missing API proposal enablement.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add a context menu item to AppMap tree items that generates a PlantUML
sequence diagram file (.uml) alongside the AppMap.

Features:
- Right-click context menu on AppMap items
- Uses default package specifications (no prompts)
- Generates .uml file with PlantUML source
- Opens generated file in editor
- Hidden from command palette (context-only)

Implementation:
- New command: appmap.generateSequenceDiagramFiles
- Command handler: src/commands/generateSequenceDiagramFiles.ts
- Registered in extension.ts activation
- Uses @appland/sequence-diagram library with default exclusions

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@kgilpin kgilpin force-pushed the feat/fix-code-issue-from-finding-or-known-function branch from f753c99 to 626830c Compare January 16, 2026 15:11
Add inline markdown icon to finding tree items that opens a comprehensive
finding report as a markdown preview with clickable file links.

Features:
- Inline markdown icon on finding rows in Runtime Analysis sidebar
- Opens finding synopsis as markdown preview (not text editor)
- Workspace-relative file paths as clickable links with line numbers
- Word-wrapped messages (~100 chars) in code blocks
- Complete rule information (ID, description, documentation URL)
- Stack trace with clickable links (filters out invalid line numbers)
- Participating events with full SQL queries and method signatures
- Impact domain, occurrence count, and references
- Automatic line number validation (strips -1, 0, null values)

Implementation:
- New command: appmap.openFindingAsMarkdown
- Command handler: src/commands/openFindingAsMarkdown.ts
- Type definitions: src/types/rawEvent.ts for snake_case event fields
- Updated resolvedFinding.ts to cast events to raw types
- Updated findingsTreeDataProvider.ts to export FindingTreeItem class

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@kgilpin kgilpin force-pushed the feat/fix-code-issue-from-finding-or-known-function branch from a0bc962 to be8bbe1 Compare January 16, 2026 15:16
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