Skip to content

Conversation

@jpenilla
Copy link
Member

@jpenilla jpenilla commented Dec 20, 2025

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?). And following that we should have handling for edge cases like back navigating to hidden files.

Also would like to have a top-level button for opening the menu somewhere, not just in the menu bar.
And a way to indicate filters are active (maybe on the button?) would also be good, especially if users configure the eventual default/global filters. We all know about how people forget what's in their global gitignores!

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?)
@jpenilla jpenilla changed the title Initial diff filtering implementation Diff filtering Dec 20, 2025
@jpenilla jpenilla added type: feature New feature or request area: diff viewer labels Dec 20, 2025
@github-actions
Copy link

github-actions bot commented Dec 20, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
diffs ✅ Ready (View Log) Visit Preview aeb52e1

Copy link

Copilot AI left a 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.

Comment on lines +31 to +43
<ToggleGroup.Root class="flex flex-wrap gap-0.5" type="multiple" bind:value={instance.selectedFileStatuses}>
{#each FILE_STATUSES as status (status)}
{@const statusProps = getFileStatusProps(status)}
<ToggleGroup.Item
aria-label="Toggle {statusProps.title} Files"
value={status}
class="flex cursor-pointer items-center gap-1 rounded-sm btn-ghost px-2 py-1 data-[state=off]:text-em-med data-[state=off]:hover:text-em-high data-[state=on]:btn-ghost-visible"
>
<span class="aria-hidden size-4 {statusProps.iconClasses}"></span>
{statusProps.title}
</ToggleGroup.Item>
{/each}
</ToggleGroup.Root>
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The ToggleGroup allows users to deselect all file statuses, which would result in all files being filtered out and an empty diff view. Consider either preventing the last status from being deselected, or displaying a helpful message when no files match the current filters.

Copilot uses AI. Check for mistakes.
Comment on lines +543 to +544
const vlistFileIdx = this.filteredFileDetails.vlistIndices[fileIdx];
this.vlist.scrollToIndex(vlistFileIdx, { align: "start", smooth });
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

When a file is filtered out, vlistFileIdx will be -1. This will cause scrollToIndex to be called with -1, which may result in unexpected scrolling behavior. The function should check if the file is currently visible before attempting to scroll, or handle the -1 case explicitly.

Copilot uses AI. Check for mistakes.
Comment on lines +561 to +563
const vlistFileIdx = this.filteredFileDetails.vlistIndices[file.index];
if (vlistFileIdx < startIdx || vlistFileIdx > endIdx) {
this.vlist.scrollToIndex(vlistFileIdx, { align: "start" });
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

When a file is filtered out, vlistFileIdx will be -1. This could lead to incorrect behavior when checking viewport bounds or scrolling. The function should verify that the file is currently visible (vlistFileIdx >= 0) before attempting to scroll to it.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants