You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "hqhq1025 (via GitHub)" <gi...@apache.org> on 2026/09/13 09:17:41 UTC
[PR] fix(desktop): keep sidebar actions anchored to the left [maka]
hqhq1025 opened a new pull request, #5253:
URL: https://github.com/apache/maka/pull/5253
## Summary
Keep the sidebar toggle, Search, and New Task in one fixed left titlebar cluster, in both sidebar states and while resizing.
The current main build places the collapsed cluster at the far right of the window. At a 1240px viewport its leading edge moves from 184px to 1160px. This patch anchors the cluster to the existing platform-safe left gutter and constrains the first grid track to its content size when the sidebar is absent. The three-action cluster stays at x=98px on macOS.
- Preserve Astryx's collapse/resize controller, zero-width hidden navigation, persisted width, and existing workbar layout.
- Reuse the existing new-task command and make it leave WorkHub, including focus transfer from its separate WebContentsView.
- Move the presentation-only topbar component into its planned `shell` ownership zone with injected copy/callbacks. Remove the redundant new-task callback wrapper and obsolete comments. Architecture rules are unchanged; legacy debt does not increase.
- Put fixed-position assertions in Storybook and keep two Electron regressions for native keyboard input and the remounted resize handle.
No public API, storage schema, dependency, or navigation-mode change.
## Before / After
Both captures use the same isolated synthetic session catalog, new-task screen, light theme, 1240 x 820 renderer viewport, 260px expanded sidebar, frozen fixture clock, and 100% zoom. Before is main `09f0a5d364a3da869c06069f166d758ef6025855`; after is this revision. No personal session data is included.
Screenshot upload is currently blocked: the native `gh pr create --attach` route returned HTTP 500 twice before creating a PR. All four matched images were captured and visually checked locally. This PR stays draft until the before/after images are attached and their rendering is verified.
## Verification
- Desktop workspace dependency build, main/preload/overlay/renderer build, and Desktop typecheck passed.
- Storybook build and complete render smoke passed: 356 stories, 383 theme renders, including updated sidebar geometry and existing wide/narrow workbar titlebar checks.
- The final six-test Electron run was **2 passed, 4 failed**. Three failures occurred before test bodies while awaiting the first window. The fourth spent 59.24s in setup and exhausted its 60s budget before reload; teardown then interrupted the modal assertion. An isolated rerun of the three existing regressions also timed out during setup.
- The new WorkHub/native-input journey separately passed in 20.4s without changing source or timeouts. Native drag-collapse/keyboard resize and archived-only startup passed in the final batch. The earlier six-test run, before adding the WorkHub exit fix, passed.
- Additional Electron geometry inspection passed in light/dark at 1240, 1000, 990, 720, and 480px, with both sidebar states and the right workbar open: all three actions remain hit-testable and inside the viewport.
- Strict renderer architecture check against `09f0a5d36`, targeted Biome, locale hygiene, E2E budget, Astryx inventory, ASF headers, Knip, and diff whitespace checks passed.
- An earlier batch overlapping Storybook also encountered startup/readiness timeouts. These results do not establish a sidebar regression, but an environment-only cause is not proven and the final Electron suite is not green.
- A subsequent unchanged-main control run passed draft focus (38.0s) and project-group reload (58.8s). These slow passes do not explain or clear the patched-run failures.
- Native OS window dragging and Windows/Linux caption-button hit testing were not exercised. Existing platform gutters and drag-region ownership remain unchanged. Full repository unit/Electron suites were not run.
## Pending Verification
- [ ] Obtain a clean focused Electron run and investigate any reproducible startup failure before marking ready.
- [ ] Verify native window dragging and Windows/Linux caption-button hit targets.
- [ ] Attach and verify the four matched before/after screenshots once GitHub upload succeeds.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Codex implemented the user-selected direction, performed local and independent agent review, added regressions, captured comparisons, and prepared this PR.
## Checklist
- [ ] Tests cover the change and fail without it
- [ ] Lint, format, typecheck and the affected suites pass locally
Regression coverage was added and browser checks passed, but the complete checklist remains open because the final Electron batch timed out and the new tests were not run against the unpatched build.
Does this PR entail a change in behavior?
- [x] Yes - described under Summary above
- [ ] No
External human approval and CI are still required before merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep sidebar actions anchored to the left [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5253:
URL: https://github.com/apache/maka/pull/5253#discussion_r4011813148
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -1239,7 +1239,8 @@ function AppShellContent({
showModelSetupToast,
toastApi,
});
- const openNewTaskSurface = useCallback(() => {
+ const createSession = useCallback(async () => {
+ setWorkHubActive(false);
Review Comment:
P3. `exitWorkHub` (line 1353) already wraps this call; reuse it rather than inlining a second `setWorkHubActive(false)`.
##########
apps/desktop/src/renderer/styles/shell-layout.css:
##########
@@ -342,11 +343,8 @@ dialog:modal {
.maka-shell-topbar-rail {
grid-column: 1;
- /* These controls operate on the sidebar, so their anchor is its trailing
- edge — not the platform-safe area's leading edge beside the traffic
- lights. The first titlebar grid track already ends one gap before the
- sidebar seam and follows the user-resized sidebar width. */
- justify-self: end;
+ /* Collapse and resize move the column edge, never the global actions. */
+ justify-self: start;
Review Comment:
P1. This is what makes the rail *look* fixed: it pins the cluster to the start of a track that is still the invalid fallback (see the top-level comment). With the inline `0` replaced by the CSS rule's `0px`, `main`'s template already places the rail at x=28 without this change. If the expanded-state move to the window edge is intended, keep it, but it should be described as a layout decision rather than the fix.
##########
apps/desktop/e2e/sidebar-layout.spec.ts:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { COMPOSER_INPUT, ensureSidebarExpanded, expect, getWorkHubPage, test } from './fixtures';
+
+// Native keyboard delivery must follow focus from the titlebar search action
+// through dialog dismissal into the new-task composer. DOM focus alone would
+// miss input lost when the separate WorkHub WebContentsView relinquishes focus.
+test('titlebar search and new task receive native input, including when leaving WorkHub', async ({
+ sessionLocalWindow: { page, app },
+}) => {
+ const nativeWindow = await app.browserWindow(page);
+ await nativeWindow.evaluate((window) => {
+ window.show();
+ window.focus();
+ window.webContents.focus();
+ });
+ const chrome = page.locator('.maka-shell-topbar-rail');
+ const sidebar = page.locator('.maka-sidenav-motion');
+ const composer = page.locator(COMPOSER_INPUT);
+
+ const search = chrome.getByRole('button', { name: '搜索任务' });
+ await search.focus();
+ await search.press('Enter');
+ const searchInput = page.getByRole('combobox', { name: '搜索任务' });
+ await expect(searchInput).toBeFocused();
+ await nativeWindow.evaluate((window) => {
+ for (const character of 'sidebar') {
+ window.webContents.sendInputEvent({ type: 'char', keyCode: character });
+ }
+ });
+ await expect(searchInput).toHaveValue('sidebar');
+ await page.keyboard.press('Escape');
+ await expect(search).toBeFocused();
+ await page.keyboard.press('Tab');
+ const newTask = chrome.getByRole('button', { name: '新任务', exact: true });
+ await expect(newTask).toBeFocused();
+
+ await newTask.click();
+ await expect(composer).toBeFocused();
+ await expect(composer).toHaveText('');
+ await nativeWindow.evaluate((window) => {
+ for (const character of 'new task draft') {
+ window.webContents.sendInputEvent({ type: 'char', keyCode: character });
+ }
+ });
+ await expect(composer).toHaveText('new task draft');
+
+ await ensureSidebarExpanded(page);
+ await page.getByRole('button', { name: '设置', exact: true }).click();
+ await page.getByRole('button', { name: '通用', exact: true }).click();
+ const enableWorkHub = page.getByRole('switch', { name: '启用 WorkHub', exact: true });
+ await enableWorkHub.click();
+ await expect(enableWorkHub).toBeChecked();
+ await page.keyboard.press('Escape');
+ await sidebar.getByRole('button', { name: 'WorkHub', exact: true }).click();
+ const workhub = await getWorkHubPage(app);
+ await expect(page.locator('.workHubDock')).toBeVisible();
+ await chrome.getByRole('button', { name: '收起侧边栏' }).click();
+ await workhub.locator(COMPOSER_INPUT).click();
+ await expect(workhub.locator(COMPOSER_INPUT)).toBeFocused();
+
+ await newTask.click();
+ await expect(page.locator('.workHubDock')).toBeHidden();
+ await expect(composer).toBeVisible();
+ await expect(composer).toBeFocused();
+ await expect(composer).toHaveText('new task draft');
+ // Do not focus webContents here: the new-task action must hand input back.
+ await nativeWindow.evaluate((window) => {
+ for (const character of ' returned from WorkHub ') {
+ window.webContents.sendInputEvent({ type: 'char', keyCode: character });
+ }
+ });
+ await expect(composer).toContainText('returned from WorkHub');
+});
+
+// After drag-collapse remounts Astryx's handle, Electron key events must still
+// resize it. Geometry confirms native input reached the remounted control,
+// rather than merely checking DOM focus or dispatching a renderer key event.
+test('native keyboard resizing reaches the restored handle after drag-collapse', async ({
Review Comment:
P2. This test exercises the Astryx resize handle (drag resize, drag collapse, restore, keyboard resize), none of which this PR touches. Per `e2e/AGENTS.md` E2E is for behavior this change adds; the resize handle already has its coverage. In the first test, the WorkHub `WebContentsView` focus hand-back is the one real Electron boundary; search focus, Escape, tab order and composer text are Storybook-layer checks.
##########
apps/desktop/src/renderer/shell/topbar-actions.tsx:
##########
@@ -84,14 +65,29 @@ function ChromeColumnToggle(props: {
}
export function AppShellTopbarActions(props: {
+ copy: {
Review Comment:
P2. This replaces `useUiLocale()` with a five-field `copy` prop threaded from AppShell. Nine other renderer components still resolve copy through the hook, and the stories render without a locale provider either way. I removed the prop and restored the hook locally and both call sites still work, so this is a second copy-resolution path with no demonstrated need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep sidebar actions anchored to the left [maka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5253:
URL: https://github.com/apache/maka/pull/5253#issuecomment-6307577727
<!-- maka-lifecycle:stale-close -->
This pull request has been closed because it received no new commits during the 7-day grace period. This is not a judgement on the merit of the change; it only keeps the review queue aligned with work that is actively in flight.
The branch and review history are preserved. Push a new commit and reopen the pull request whenever the work is ready to continue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep sidebar actions anchored to the left [maka]
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #5253:
URL: https://github.com/apache/maka/pull/5253#issuecomment-5655958899
The implementation direction is sound, but there are three P2 issues worth addressing. No P0/P1 issues were found.
## Spec
1. **P2: The minimum sidebar width can break breadcrumb-to-sidebar-seam alignment.**
`shell-layout.css:301` uses `minmax(..., max-content)`. At the supported 180px minimum sidebar width on macOS, the three-button cluster can expand the first track beyond the sidebar width, shifting the breadcrumb past the seam.
Minimal fix: increase the minimum sidebar width or define a narrow-width compression/hiding strategy, then add a geometry assertion at 180px.
2. **P2: The core “while resizing” requirement lacks direct regression coverage.**
`app-shell.stories.tsx:535` only compares bounds before and after collapsing, while the story keeps the sidebar fixed at 260px with `onWidthChange={noop}`.
`sidebar-layout.spec.ts:112` performs a drag resize but only asserts the resulting sidebar width, not that the three titlebar actions remain stationary.
Please make the story harness width-controllable and verify stable action bounds at representative widths such as 180, 260, and 320px. The regression should also be demonstrated to fail with the previous CSS.
3. **P2: The expanded layout now contains two identical “New Task” actions.**
The new titlebar action is at `topbar-actions.tsx:102`, while the existing sidebar action remains at `session-sidebar-nav.tsx:55`.
This introduces duplicate functionality and an additional keyboard focus stop. The previous implementation explicitly documented avoiding this duplication. If it is intentional, the rationale should be documented; otherwise, move the assistant target to the titlebar action and remove the sidebar entry.
## Standards
- **P2: The Electron test includes behavior that belongs at a lower test tier.**
`sidebar-layout.spec.ts:38` includes search-dialog focus, Escape restoration, Tab ordering, and ordinary composer assertions. These are browser/Storybook contracts.
The repository requires choosing the lowest appropriate tier for each assertion. The Electron test should retain only the native WorkHub `WebContentsView` → main-window input handoff and remounted-handle keyboard-resize behavior.
- **P2: The new regression coverage has not been shown to detect the original defect.**
`e2e/AGENTS.md:59` requires demonstrating that replacement coverage fails against the old implementation or a targeted mutation. The PR checklist currently acknowledges that this was not done.
## Overall Assessment
- The CSS approach using `justify-self: start` and `max-content` is generally appropriate.
- Moving the presentation-only component into `shell/` follows the renderer ownership rules.
- No deep refactor appears necessary.
- The sidebar’s duplicate “New Task” entry is removable production code.
- The renderer-only assertions in the Electron journey can be moved or removed.
- Current GitHub CI is green: all 40 Electron tests pass, along with 356 Storybook stories and 383 renders. The PR description’s “2 passed, 4 failed” result is outdated.
- The repository-required `smoke:real-window` report is still missing. Native window dragging and Windows/Linux caption hit targets remain residual risks.
- The PR is mergeable, but currently remains `REVIEW_REQUIRED/BLOCKED` pending independent human approval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep sidebar actions anchored to the left [maka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5253: fix(desktop): keep sidebar actions anchored to the left
URL: https://github.com/apache/maka/pull/5253
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep sidebar actions anchored to the left [maka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5253:
URL: https://github.com/apache/maka/pull/5253#issuecomment-6160453228
<!-- maka-lifecycle:stale-warning -->
This pull request has had no new commits for 30 days and has been marked stale. It will be closed in 7 days unless a new commit is pushed.
Comments do not reset this timer: only a new commit does. If the pull request is intentionally long-lived, a maintainer can apply the `pinned` label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org