-
-
Notifications
You must be signed in to change notification settings - Fork 4
Diff filtering #58
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
base: master
Are you sure you want to change the base?
Diff filtering #58
Conversation
Still need to decide on a strategy for default/global filters in app settings (current is only per-session), as well as strategy for persisting filters within a session (i.e. url params vs history state?)
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
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 introduces a comprehensive diff filtering system that allows users to filter visible files by status (added, removed, modified, etc.) and by file path patterns using regular expressions. The implementation refactors the file tree and statistics computation to work with filtered file sets, and adds a new dialog UI for configuring filters.
- Adds file filtering by status and path patterns with include/exclude modes
- Refactors statistics computation to be derived from filtered files rather than pre-computed
- Restructures file tree state management into a dedicated FileTreeState class
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
web/src/lib/components/diff-filtering/index.svelte.ts |
Implements core filtering logic with DiffFilterDialogState class for managing file status and path filters |
web/src/lib/components/diff-filtering/DiffFilterDialog.svelte |
Provides UI for configuring filters with toggle groups for file status and form for regex patterns |
web/src/lib/components/sidebar/index.svelte.ts |
Extracts file tree logic into FileTreeState class, now operates on filtered file details |
web/src/lib/components/sidebar/Sidebar.svelte |
Updates to use new FileTreeState class and reference filtered file arrays |
web/src/lib/diff-viewer.svelte.ts |
Adds filteredFileDetails with vlist index mapping, converts stats to derived computation, integrates DiffFilterDialogState |
web/src/routes/+page.svelte |
Imports and renders DiffFilterDialog, updates references to use filtered file details and stats summary |
web/src/routes/FileHeader.svelte |
Updates to access addedLines/removedLines directly from file details and use viewer.fileTree for tree operations |
web/src/lib/components/tree/Tree.svelte |
Prevents re-instantiation of TreeState when instance is already provided via bindable prop |
web/src/lib/components/menu-bar/MenuBar.svelte |
Adds "Edit Filters" menu item and refactors dialog opening to use unified openDialog method |
web/src/lib/util.ts |
Removes FileTreeNodeData type and makeFileTree function (moved to sidebar module) |
web/src/lib/github.svelte.ts |
Exports FILE_STATUSES constant array for use in filter dialog |
Comments suppressed due to low confidence (1)
web/src/lib/diff-viewer.svelte.ts:851
- The search functionality (findSearchResults) searches through all files in fileDetails rather than only the filtered files. This means search results could include matches from files that are currently hidden by filters, leading to confusing user experience where users see matches they can't navigate to.
private async findSearchResults(): Promise<SearchResults> {
let query = this.searchQueryDebounced.current;
if (!query) {
return SearchResults.EMPTY;
}
query = query.toLowerCase();
const diffPromises = this.fileDetails.map((details) => {
if (details.type !== "text") {
return undefined;
}
return details.structuredPatch;
});
const diffs = await Promise.all(diffPromises);
let total = 0;
const lines: Map<FileDetails, number[][]> = new Map();
const counts: Map<FileDetails, number> = new Map();
const mappings: Map<number, FileDetails> = new Map();
for (let i = 0; i < diffs.length; i++) {
const diff = diffs[i];
if (diff === undefined) {
continue;
}
const details = this.fileDetails[i];
const lineNumbers: number[][] = [];
let found = false;
for (let j = 0; j < diff.hunks.length; j++) {
const hunk = diff.hunks[j];
const hunkLineNumbers: number[] = [];
lineNumbers[j] = hunkLineNumbers;
let lineIdx = 0;
for (let k = 0; k < hunk.lines.length; k++) {
if (isNoNewlineAtEofLine(hunk.lines[k])) {
// These lines are 'merged' into the previous line
continue;
}
const count = countOccurrences(hunk.lines[k].slice(1).toLowerCase(), query);
if (count !== 0) {
counts.set(details, (counts.get(details) ?? 0) + count);
total += count;
if (!hunkLineNumbers.includes(lineIdx)) {
hunkLineNumbers.push(lineIdx);
}
found = true;
}
lineIdx++;
}
}
if (found) {
mappings.set(total, details);
lines.set(details, lineNumbers);
}
}
return new SearchResults(counts, total, mappings, lines);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const vlistFileIdx = this.filteredFileDetails.vlistIndices[fileIdx]; | ||
| this.vlist.scrollToIndex(vlistFileIdx, { align: "start", smooth }); |
Copilot
AI
Jan 11, 2026
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.
When scrolling to a filtered-out file, vlistIndices will return -1, but this value is passed directly to vlist.scrollToIndex. This could cause unexpected behavior or errors. Consider checking if vlistFileIdx is -1 and handling the case where the file is currently filtered out (e.g., by returning early or showing a message).
| const vlistFileIdx = this.filteredFileDetails.vlistIndices[file.index]; | ||
| if (vlistFileIdx < startIdx || vlistFileIdx > endIdx) { | ||
| this.vlist.scrollToIndex(vlistFileIdx, { align: "start" }); |
Copilot
AI
Jan 11, 2026
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.
Similar to scrollToFile, when scrolling to a match in a filtered-out file, vlistFileIdx could be -1. This value is then used in comparisons with startIdx and endIdx, and potentially passed to scrollToIndex. Consider checking if the file is filtered out and handling this case appropriately.
Missing handling for edge cases like back navigating to hidden files.
Maybe want to always show the indicator with different text to have a top level button? (not in menubar)