Repository navigation
Conversation
Support short tmux format aliases like #S, #I, #W, #P, #D, and #F in the tmux-compat renderer so display-message returns the same values as the long-form placeholders.
|
@onthebed is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds preprocessing to tmux format string rendering in both CLI (Swift) and daemon (Go) components that expands short-form format aliases ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
Greptile SummaryThis PR fixes
Confidence Score: 3/5Core fix is correct but the integration test has a definite assertion bug that will fail on macOS. The Go and Swift implementations are solid and the Go unit tests verify the expected behavior. However, the Python integration test asserts #W against the workspace UUID (ws) instead of the workspace title (title), which is a P1 defect that guarantees a test failure when run on macOS. tests_v2/test_tmux_compat_matrix.py — the #W assertion and hardcoded index values need attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["tmuxRenderFormat(format, context, fallback)"] --> B["tmuxExpandShortFormatAliases"]
B --> C{"scan each byte"}
C -->|"ch != '#'"| D["write ch as-is"]
C -->|"ch == '#' and i+1 >= len"| D
D --> C
C -->|"ch == '#', next == '#'"| E["write '##', skip next"]
E --> C
C -->|"ch == '#', next in aliases"| F{"context has key?"}
F -->|yes| G["write context value, skip next"]
F -->|no| H["write '#', advance to next char"]
G --> C
H --> C
C -->|done| I["long-form pass: replace #{key} tokens"]
I --> J["regex strip remaining #{...}"]
J --> K["TrimSpace → return or fallback"]
Reviews (1): Last reviewed commit: "fix(tmux): expand short format aliases i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _must(short_aliases.stdout.strip() == f"cmux:0.0 {ws}", | ||
| f"display-message short aliases should expand: {short_aliases.stdout!r}") |
There was a problem hiding this comment.
#W asserted against workspace UUID instead of workspace title
ws is the UUID returned by c.new_workspace(), but #W expands to window_name, which is set from the workspace's title field (see tmuxFormatContext in tmux_compat.go, line ~259). By this point in the test the workspace has been renamed to title = f"tmux-title-{stamp}", so #W would expand to that renamed title — not the UUID — causing this assertion to always fail.
| _must(short_aliases.stdout.strip() == f"cmux:0.0 {ws}", | |
| f"display-message short aliases should expand: {short_aliases.stdout!r}") | |
| short_aliases = _run_cli(cli, ["display-message", "-p", "#S:#I.#P #W"]) | |
| _must(short_aliases.stdout.strip() == f"cmux:0.0 {title}", | |
| f"display-message short aliases should expand: {short_aliases.stdout!r}") |
| if key, ok := tmuxShortFormatAliases[next]; ok { | ||
| if value, exists := context[key]; exists { | ||
| builder.WriteString(value) | ||
| i++ | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
When a recognized alias (e.g. #I) appears in the format string but its context key (window_index) is absent from the map, the # is emitted and the loop advances to the alias letter, which is then emitted on the next iteration — reproducing the original #I literally. This passthrough is reasonable, but it diverges slightly from real tmux which would render an empty string. A brief comment clarifying the intended behavior would help future readers distinguish a deliberate design choice from a bug.
| short_aliases = _run_cli(cli, ["display-message", "-p", "#S:#I.#P #W"]) | ||
| _must(short_aliases.stdout.strip() == f"cmux:0.0 {ws}", | ||
| f"display-message short aliases should expand: {short_aliases.stdout!r}") |
There was a problem hiding this comment.
window_index=0 and pane_index=0 are fragile
The assertion "cmux:0.0 {title}" assumes #I (window_index) is 0 and #P (pane_index) is also 0. Window index depends on the order workspaces are listed by the daemon, which can be affected by pre-existing workspaces. Pane index after break-pane / join-pane / swap-pane operations may also not be 0. Consider reading back the actual index values from display-message #{window_index} and #{pane_index} and interpolating them into the expected string, or at least asserting that #S and #W expand correctly in isolation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_tmux_compat_matrix.py`:
- Around line 272-274: The assertion hard-codes dynamic tmux context values;
update the check around short_aliases (result of _run_cli) so it doesn't require
fixed indices. Instead parse or regex-match short_aliases.stdout.strip(): verify
the part after the space equals the expected window name variable ws, and verify
the prefix matches the pattern "cmux:<number>.<number>" (e.g., with a regex like
r"^cmux:\d+\.\d+$" applied to the first token). Use the existing _must helper to
assert both conditions against short_aliases.stdout.strip() so the test no
longer depends on exact runtime `#I/`#P values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c276b19e-f7f2-42f7-bcc8-c661f6e33391
📒 Files selected for processing (4)
CLI/cmux.swiftdaemon/remote/cmd/cmuxd-remote/tmux_compat.godaemon/remote/cmd/cmuxd-remote/tmux_compat_test.gotests_v2/test_tmux_compat_matrix.py
| short_aliases = _run_cli(cli, ["display-message", "-p", "#S:#I.#P #W"]) | ||
| _must(short_aliases.stdout.strip() == f"cmux:0.0 {ws}", | ||
| f"display-message short aliases should expand: {short_aliases.stdout!r}") |
There was a problem hiding this comment.
Avoid hard-coding dynamic tmux context values in this regression assertion.
At Line 184 the test renames the window, and #I/#P/#W depend on current runtime context. Asserting cmux:0.0 {ws} makes this check brittle and can fail despite correct short-alias expansion.
Suggested fix
- short_aliases = _run_cli(cli, ["display-message", "-p", "#S:`#I`.#P `#W`"])
- _must(short_aliases.stdout.strip() == f"cmux:0.0 {ws}",
- f"display-message short aliases should expand: {short_aliases.stdout!r}")
+ long_form = _run_cli(
+ cli,
+ ["display-message", "-p", "#{session_name}:#{window_index}.#{pane_index} #{window_name}"],
+ )
+ short_aliases = _run_cli(cli, ["display-message", "-p", "#S:`#I`.#P `#W`"])
+ _must(
+ short_aliases.stdout.strip() == long_form.stdout.strip(),
+ f"display-message short aliases should match long-form render: short={short_aliases.stdout!r} long={long_form.stdout!r}",
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_tmux_compat_matrix.py` around lines 272 - 274, The assertion
hard-codes dynamic tmux context values; update the check around short_aliases
(result of _run_cli) so it doesn't require fixed indices. Instead parse or
regex-match short_aliases.stdout.strip(): verify the part after the space equals
the expected window name variable ws, and verify the prefix matches the pattern
"cmux:<number>.<number>" (e.g., with a regex like r"^cmux:\d+\.\d+$" applied to
the first token). Use the existing _must helper to assert both conditions
against short_aliases.stdout.strip() so the test no longer depends on exact
runtime `#I/`#P values.
|
Superseded by #13608, which ports the short-format fix onto current |
2 participants
tmuxRenderFormatonly substituted#{...}placeholders, so short tmux aliases like#S,#I, and#Wcame back literally fromcmux __tmux-compat display-message -p.This teaches both tmux-compat renderers to expand the existing short aliases before the long-form pass, and adds regression coverage in the Go unit tests plus the tmux-compat matrix test.
Tested with
go test ./...indaemon/remote/cmd/cmuxd-remote; the macOS app-side matrix test was updated but not runnable in this Linux workspace.Fixes #3190
Summary by cubic
Expand short tmux format aliases (
#S,#I,#W,#P,#D,#F) in tmux-compat sodisplay-message -preturns real values instead of literal aliases. Fixes #3190.#{...}substitution inCLI/cmux.swiftanddaemon/remote/cmd/cmuxd-remote/tmux_compat.go.##correctly; map aliases to context keys; setwindow_flagsdefault to*in the CLI context.Written for commit 06a560d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests