Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The modal focus-trap logic can allow Tab navigation to escape when the overlay itself has focus, and the current tests/stubs don’t accurately model DOM contains() behavior to prevent regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new MarkEdit extension (“version browser”) that overlays the editor with a UI to browse saved file versions, render diffs (unified/split), and restore or delete local versions, with user-configurable diff settings.
Changes:
- Adds the version browser runtime (overlay view, keyboard interactions, request gating, status/spinner timing, diff rendering, and settings parsing).
- Sets up the Vite build pipeline (single-file output) and Vitest test harness for automated validation.
- Adds assets (HTML/CSS/SVG) and a comprehensive test suite covering browser workflow, diff configuration, status timing, and settings parsing.
File summaries
| File | Description |
|---|---|
yarn.lock |
Adds the resolved dependency graph for the extension and its tooling/runtime deps. |
vitest.config.mts |
Introduces Vitest configuration for the repository. |
vite.config.mts |
Adds Vite configuration (including single-file bundling and module alias shims). |
tsconfig.json |
Adds TypeScript compiler configuration for the project. |
tests/themes.test.ts |
Tests the Pierre/Shiki theme shim behavior. |
tests/status.test.ts |
Tests status/spinner delay, cancellation, and minimum spinner duration. |
tests/settings.test.ts |
Tests settings parsing/defaulting/clamping behavior. |
tests/interaction.test.ts |
Tests request gating, action enable/disable logic, and focus trapping behavior. |
tests/diff.test.ts |
Tests diff rendering options and persistence of diff style selection. |
tests/browser.test.ts |
End-to-end workflow tests for loading versions, rendering diffs, restore/delete flows, and keyboard/pointer behaviors. |
src/vite-env.d.ts |
Adds Vite client type references for TS. |
src/view.ts |
Implements DOM construction, version select population, and style injection for the overlay UI. |
src/status.ts |
Implements status message handling with delayed spinner and minimum-duration behavior. |
src/shiki.ts |
Provides a Shiki shim/exports suitable for bundling constraints. |
src/settings.ts |
Defines settings schema, defaults, and robust parsing/clamping from MarkEdit user settings. |
src/pierre-themes.ts |
Adds theme descriptors and a theme factory shim required by Pierre. |
src/interaction.ts |
Implements request gating, action-state updates, and focus trapping for the modal overlay. |
src/diff.ts |
Wraps Pierre diff rendering and applies user settings, including persisted unified/split mode. |
src/browser.ts |
Main orchestration for showing/closing the overlay, loading versions, caching, rendering diffs, and wiring interactions. |
README.md |
Documents installation, usage, settings, and build/test commands. |
package.json |
Adds scripts and dependencies for building/testing the extension. |
main.ts |
Registers the “Browse Versions” menu item in MarkEdit. |
LICENSE |
Adds MIT license for the project. |
assets/downloaded.svg |
Icon asset for “downloaded from iCloud” state. |
assets/cloud.svg |
Icon asset for iCloud/non-local state. |
assets/browser.html |
Overlay HTML template for the version browser UI. |
assets/browser.css |
Overlay styles for layout, theming, focus states, and animations. |
.gitignore |
Ignores node_modules/ and macOS metadata. |
Review details
Suppressed comments (1)
tests/interaction.test.ts:158
createFocusContextstubsoverlay.contains()in a way that differs from real DOM semantics (in the DOM,overlay.contains(overlay)istrue). This can mask focus-trap bugs related toactiveElement === overlay; update the stub to include the overlay itself incontains().
const overlay = {
contains(element: HTMLElement) {
return focusableElements.includes(element);
},
- Files reviewed: 22/29 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The overlay markup uses fixed IDs referenced by aria-describedby, but the close lifecycle removes the prior overlay asynchronously, creating a verified duplicate-ID accessibility/validity issue when reopening quickly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/interaction.ts:72
- Same duplicate-ID risk as the Restore description:
aria-describedbyis set to the fixed IDversion-browser-delete-description, which is defined in browser.html. Because the previous overlay is removed asynchronously during close, reopening quickly can result in multiple elements sharing this ID and an ambiguousaria-describedbytarget. Prefer per-instance IDs (or wiring the description elements throughBrowserElements) and use the generated ID here.
deleteButton.disabled = disabled;
if (isNonlocal) {
deleteButton.setAttribute('aria-disabled', 'true');
deleteButton.setAttribute('aria-describedby', 'version-browser-delete-description');
} else {
- Files reviewed: 22/29 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Local uncached version selection can leave the previous diff visible while new content loads, causing stale/mismatched UI until fetch completes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/29 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is cohesive and is accompanied by thorough automated tests covering the key UI workflows and edge cases introduced by the new extension.
Review details
- Files reviewed: 22/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.