Repository navigation
Refine modal footers, filepath display and explorer filtering - #344
flyingrobots wants to merge 18 commits into
Conversation
flyingrobots
added
the
work-in-progress
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flyingrobots
removed
the
work-in-progress
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b74c3739fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import type { FooterPresentation, FooterSurfaceObservation } from '../../ui/footer-layout-view.js'; | ||
| import { resolveWorkspaceLayout } from '../../ui/drawer-layout.js'; | ||
| import { resolveSettingsDrawerWidth } from '../../ui/settings-drawer.js'; |
There was a problem hiding this comment.
Move the observation DTO and layout-independent derivation into the application layer, and let the UI consume them. This new application module imports its contract and geometry helpers from src/ui, reversing the canonical dependency direction (UI -> app, not app -> UI) and coupling application state derivation to terminal presentation details such as settings-drawer width.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| @@ -0,0 +1,84 @@ | |||
| import { FOOTER_LAYOUT, type FooterLayout } from '../ui/footer-layout.js'; | |||
There was a problem hiding this comment.
Import FOOTER_LAYOUT and FooterLayout directly from src/ports/footer-preferences.ts here and in footer-preferences.ts. These persistence adapters currently depend on a UI barrel for their runtime contract, violating the adapter-to-port boundary and making headless preference loading transitively load presentation code.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| if (state.footerPresentation != null) return renderFooterLayout({ | ||
| presentation: state.footerPresentation, width, background, | ||
| command: state.commandLine?.active ? { | ||
| i18n: state.i18n, commandLine: state.commandLine, contextLine: commandLineHints(state.i18n), commandLineError: state.commandLineError, | ||
| } : undefined, |
There was a problem hiding this comment.
When an Arabic, Hebrew, or other RTL locale uses the production footer, this branch routes every new layout through renderFooterLayout without passing state.i18n.direction. Consequently its inactive-command path always paints focused text from the left and global text from the right, bypassing the existing RTL alignment behavior; only the nested command renderer still receives direction. Thread direction into the new renderer and place logical sides accordingly.
Useful? React with 👍 / 👎.
| const layout = nextFooterLayout(model.footerLayout, delta); | ||
| if (context.footerPreferences?.save(layout).ok !== false) return [{ ...model, footerLayout: layout }, []]; |
There was a problem hiding this comment.
When switching from Compact to Focused or Per-pane, the body viewport shrinks by one row but this update leaves editor.scrollRow unchanged. If the cursor occupied Compact's extra bottom row, it falls outside the new source viewport and disappears until another editor action calls ensureEditorVisible; growing the viewport can likewise expose a row without refreshing viewport-bounded projections. Reconcile the editor against editorViewport when the selected layout changes its row count.
Useful? React with 👍 / 👎.
| } finally { | ||
| if (existsSync(temporary)) unlinkSync(temporary); |
There was a problem hiding this comment.
When another process edits ~/.jimrc concurrently, both writers can read the same source and pass this comparison before either performs its rename; the last rename then replaces the entire document and loses the other writer's unrelated settings. The read-before-rename check is therefore a TOCTOU guard rather than a compare-and-swap. Serialize writers with an atomic lock or equivalent protocol before replacing the document.
Useful? React with 👍 / 👎.
| function saveDocument(path: string, layout: FooterLayout): void { | ||
| const target = existsSync(path) ? realpathSync(path) : path; |
There was a problem hiding this comment.
When ~/.jimrc is a dangling symlink, existsSync(path) returns false, so this branch treats the link as a missing file and later renames the temporary file over the symlink itself. That destroys the configured indirection instead of retaining the symlink identity or reporting that its target is unavailable. Detect the directory entry with lstat/readlink and fail safely or resolve the intended target without replacing the link.
Useful? React with 👍 / 👎.
| const characters = Array.from(identity); | ||
| while (characters.length > 0 && visibleLineLength(characters.join('')) > width - 1) characters.shift(); | ||
| return `…${characters.join('')}`; |
There was a problem hiding this comment.
When a long file path contains a decomposed character or a multi-code-point emoji, Array.from splits it into individual code points and the loop can remove only part of the grapheme. A narrow footer can therefore render an orphan combining mark, variation selector, or ZWJ sequence instead of a valid filename tail. Segment by grapheme cluster, then apply display-cell clipping to those clusters.
Useful? React with 👍 / 👎.
flyingrobots
added
the
work-in-progress
flyingrobots
changed the title
Goal
Let users choose footer density and filepath display, with a clear mode badge and a modal explorer filter prompt. Apply the user's Settings border and selection feedback.
Plan
Current status
Full local check passed 892 tests: 880 passed, 12 existing skips, no failures or quality regressions. Forty-nine focused UI/preference tests pass. Native production Echo checks at 100×32 and 60×16 verified filtering, header/caret/list clicks, saving/restart, light/dark Settings and temporarily showing a hidden footer for input. Implementation CI run 38764169878 passed all 12 jobs. The final witness and exact-head check are in progress.
Stacks on #342, which depends on #339. No merge is performed here.
Closes #343.