Repository navigation
Move rebuild into the Leaflet menu and sidebar icon row - #2
Conversation
|
starting review... |
![eve-code-review-imann24[bot]](https://avatars.githubusercontent.com/in/5224018?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moves the Rebuild control into a hammer icon in the sidebar footer and adds a native macOS "Rebuild app…" menu item that opens the same dialog.
Findings: 0 blockers, 1 should-fix, 1 nit.
Docs checked: no wiki or ticket was available for this repo, so I found no linked spec or conventions. I checked src-tauri/capabilities/default.json myself. core:default already covers listen, so no new permission is needed. The existing e2e specs click Rebuild app by accessible name, and the new icon button keeps that name.
| useEffect(() => { | ||
| void load(readPreference<string>('root', '')); | ||
| }, [load]); | ||
| useEffect(() => { |
There was a problem hiding this comment.
should-fix: The new open-rebuild listener, which is the only path the native menu uses, has no automated test. The PR says it was checked by hand with the IPC harness. tests/fixtures/desktop.ts already imports emit from @tauri-apps/api/event, so a small case in tests/rebuild.spec.ts could emit open-rebuild and assert the dialog opens. That would stop this from regressing silently. The Rust side (menu insertion at index 2 and the event id match) is also untested, but that's harder to cover.
| .setup(|app| { | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| let menu = Menu::default(app.handle())?; |
There was a problem hiding this comment.
nit: The Leaflet submenu is found by comparing its text to app.package_info().name, and the item is inserted at a hard-coded index 2. If the name doesn't match, or the Tauri default menu layout changes, the Rebuild item silently disappears with no log. Consider logging when the submenu isn't found, or adding a comment explaining that index 2 sits just after About and the separator.
|
I submitted a single review on PR #2 as a comment. It has one should-fix finding and one nit, and no blockers.
I found no linked ticket, spec or team conventions, because no wiki is configured for this repo. I checked the capabilities file myself. |
|
starting review... |
There was a problem hiding this comment.
This update adds the e2e test I asked for and a log line when the Leaflet submenu isn't found. The log line covers my earlier nit. The new test has a problem, though.
Findings: 1 blocker, 0 should-fix, 0 nit.
Docs checked: no wiki or ticket is available for this repo, so I found no linked spec or conventions.
| await expect( | ||
| page.getByRole('button', { name: 'Rebuild app', exact: true }), | ||
| ).toBeVisible(); | ||
| await emit('open-rebuild'); |
There was a problem hiding this comment.
blocker: This emit runs in the Playwright Node process, not in the page. @tauri-apps/api/event's emit invokes window.__TAURI_INTERNALS__, and Node has no window, so the test should throw before the dialog is touched. The mocked IPC and the app's listen only exist in the browser page. tests/fixtures/desktop.ts already has its own emit and exposes window.journalTest. Either add a harness method there, for example openRebuildFromMenu: () => emit('open-rebuild'), and call it with page.evaluate(() => window.journalTest.openRebuildFromMenu()), or call emit inside page.evaluate via the page's module. Then drop the Node-side import on line 2. Please also re-run pnpm test:e2e. The PR description's 21 passing tests predate this test.
|
I requested changes on PR #2 at head e29f37e. There is one blocker and nothing else. I didn't run the tests. The blocker comes from reading the test code and the Tauri
No wiki or ticket exists for this repo, so I checked no docs. |
|
starting review... |
There was a problem hiding this comment.
This PR replaces the sidebar Rebuild button with a hammer icon and adds a native macOS "Rebuild app…" menu item, and both open the existing rebuild dialog.
The earlier blocker is fixed. The menu-event test now emits from inside the page through a new openRebuildFromMenu harness method. The dialog heading it asserts is <h2>Rebuild app</h2> in RebuildDialog.tsx. The PR now reports 22 passing e2e tests.
Findings: 0 blockers, 0 should-fix, 0 nit.
Docs checked: no wiki or ticket is available for this repo, so I found no linked spec or conventions. I checked src-tauri/capabilities/default.json myself. core:default already covers listen.
|
I approved PR #2 at head bc89b12 with no open findings. The earlier blocker is fixed. The menu-event test now emits from inside the page through a new No wiki or ticket exists for this repo, so I checked no docs. I did read |
The full-width Rebuild app button consumed an extra row in the sidebar. Replace it with a hammer icon beside Refresh and Choose Folder, with an accessible label and tooltip, and add Rebuild app… under the native macOS Leaflet menu. Both controls open the existing rebuild dialog; the command palette action remains available. Update the README to describe the controls.
Validation:
pnpm buildandpnpm format:checkpassed.pnpm test: 27 tests passed.pnpm test:e2e: 22 tests passed.cargo test --locked --manifest-path src-tauri/Cargo.toml: 13 passed, 1 intentionally ignored.cargo fmt --manifest-path src-tauri/Cargo.toml -- --checkpassed.The native menu update takes effect after rebuilding the desktop app.