Repository navigation
Fill the Canvas pane with its Markdown editor - #1304
jameskraus wants to merge 1 commit into
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency) and Groot (product/accessibility) reviews.
No actionable defects found in this incremental editor-fill change. Source-review assessment: 9/10 for minimalness, elegance and correctness.
Reviewed head 4e21a49d4d4410f81f646a3ea33ecf3f36f10439 against base 726d62246813d532345dd9be54d64371d90ebf8e. Checked the complete three-file diff, shared Textarea/Tabs and panel integration, short-pane overflow, hidden Edit/History handling, draft retention, and success/error/retry/cancel focus paths. The existing browser journey gains a layout assertion without adding cases. The unchanged underlying Canvas feature was not exhaustively re-audited.
Optional documentation alignment: src/bundled/canvas/Canvas.module.css:29 intentionally removes the resize grip, as the PR describes. Consider recording this Canvas-specific fill-mode exception in src/shared/design-system/DESIGN.md, whose Form composition section still specifies manual vertical resizing and a visible grip. This is not a request to restore the grip or block the change.
Public description, commit messages and all three attached screenshots inspected; no disclosure finding on those surfaces.
Validation limits: source inspection only; no tests, builds, app launches or PR code executed. CI was not assessed in this review. Rendered keyboard/focus behavior, both browser engines, narrow/short panes, enlarged text, native desktop/live-relay behavior, performance and human acceptance remain unverified by this review. The PR’s reported QA is author-provided evidence, not a run performed here. This COMMENT is not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Reviewed head 4e21a49d4 against base 726d62246. No findings. I checked:
- The resize override.
Canvas.module.cssis unlayered, soresize: nonebeats.buzz-textarea'sresize: verticalin@layer components. - Hidden panels.
.edit[hidden]is needed and correctly specific, and the History panel keeps the UAhiddenbehavior. - The grid-to-flex swap. Replacing grid
.stackwith a flex column keeps the same--space-4gap, and cross-axis stretch still bounds width. - Short panes.
flex: 1 0 autokeeps the 16-row minimum, so they scroll as described.
The new 16px footer-inset assertion is a real layout check that belongs in a browser test. The earlier optional DESIGN.md note about the fill-mode exception still stands.
Non-blocking: the description says "Readiness: Draft", but the PR is marked ready. Align one or the other per the repo's PR checklist. Merge depends on #570 (see that review). This COMMENT is not an approval.
726d622 to
fa90597
Compare
4e21a49 to
5e20777
Compare
The editor kept a fixed 16-row height, leaving the rest of a tall side pane empty above the footer. The Edit tab is now a column whose editor grows to the footer's inset. Below 16 rows it keeps that height and the pane scrolls, as before. A filling editor cannot be dragged smaller, so it has no resize grip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Kraus <jamespkraus@gmail.com>
5e20777 to
87dac25
Compare
fa90597 to
5a7b41b
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No actionable findings in the restacked editor-fill change. Source-review assessment: 9/10 for minimalness, elegance and correctness.
Reviewed head 87dac25 against base 5a7b41b. Checked the complete three-file diff and current Canvas integration: bounded pane scrolling, hidden Edit/History handling, shared Textarea and Tabs, draft retention, async guards, and success/error/retry/cancel focus paths. The existing browser journey adds a layout assertion without adding cases. This is an integration review of this PR, not an exhaustive audit of the 255-file upstream endpoint difference since the previously reviewed revision.
The prior optional documentation note about the intentional Canvas fill-mode exception to the shared resize-grip recipe remains unchanged; it is not a request to restore the grip. Public description, this PR’s commit metadata and all three attached screenshots were inspected with no disclosure finding.
Validation limits: source inspection only; no tests, builds, app launches or PR code executed, and CI was not assessed. Browser/native layout, short-pane and error-state visibility, rendered keyboard/focus behavior, enlarged text, performance, live-relay behavior and human acceptance remain unverified by this review. Author-reported QA was not rerun. This COMMENT is not approval or merge authorization.
|
🤖 Codex: Addressed the description/readiness mismatch. The description no longer calls this PR draft; its existing ready state is preserved. Before/after screenshots were recaptured from the current base |
The Canvas editor has a fixed 16-row height, so a tall side pane leaves empty space between the editor and the footer. This PR makes the Edit tab a column whose Markdown editor grows down to the footer.
Behavior
Changes
channels/Canvas.module.css. The scroll body, the outer stack and the Edit tab panel are flex columns. The editor grows (flex: 1 0 auto) and hasresize: none. The Edit panel setsdisplay: nonewhen hidden, because itsdisplay: flexwould otherwise override thehiddenattribute.channels/CanvasEditor.tsx. These two containers use the Canvas classes instead of the sharedChannelTemplatesstack, and the Edit panel loses its inner wrapper div.Design
Textarea(code variant), panelTabs, and the existing Canvas panel and footer. No shared component or token changes.Textareahas no fill mode, and the design guide's Textarea recipe includes manual vertical resizing. Canvas removes the grip in its own CSS, because a filling editor can't be resized. If other surfaces need an editor that fills its space, aTextareafill option belongs in the design system.Tests
channel-tabs.spec.mjs› Canvas opens once beside Thread and retains its channel draft gains one assertion: the editor ends at the footer's 16px inset. Layout needs a real browser. Without the CSS change, the assertion fails in Chromium and WebKit (a 369px gap); with it, it passes. No browser cases are added or removed.Stack
Stacked on #1303, above #570.
Readiness
The existing review state is preserved. The restack keeps this PR's layout change; the complete channel-tabs and Canvas-history browser files pass in Chromium and WebKit on the integrated stack (14 checks).
Native desktop, live-relay checks, and human acceptance of the latest stack remain deferred.