feat(browser): add mobile viewport toggle with configurable size#1615
Open
ryuuuuukun wants to merge 1 commit intomanaflow-ai:mainfrom
Open
feat(browser): add mobile viewport toggle with configurable size#1615ryuuuuukun wants to merge 1 commit intomanaflow-ai:mainfrom
ryuuuuukun wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable mobile viewport feature: new localization keys, persistent viewport width/height settings, a browser toolbar toggle that switches viewport size and user‑agent and reloads the WebView, UI to edit viewport dimensions in Settings, and minor Xcode project release product metadata updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant BrowserPanelView as BrowserPanelView
participant AppStorage
participant BrowserPanel as BrowserPanel
participant WebView
User->>BrowserPanelView: Tap mobile viewport button
activate BrowserPanelView
BrowserPanelView->>AppStorage: Read/persist mobileViewportWidth/Height
BrowserPanelView->>BrowserPanel: setMobileViewport(enabled)
activate BrowserPanel
BrowserPanel->>BrowserPanel: Update isMobileViewport & effectiveUserAgent
alt enabled
BrowserPanel->>WebView: Set customUserAgent = mobileUserAgent
else disabled
BrowserPanel->>WebView: Set customUserAgent = safariUserAgent
end
BrowserPanel->>WebView: Trigger reload / navigate
deactivate BrowserPanel
BrowserPanelView->>WebView: Resize frame to mobileViewportWidth/Height (if enabled)
WebView-->>User: Render page with new UA & viewport
deactivate BrowserPanelView
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ryuuuuukun
force-pushed
the
feat/browser-mobile-viewport-toggle
branch
from
5f49176 to
656f4e7
Compare
![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
🧹 Nitpick comments (3)
Sources/Panels/BrowserPanelView.swift (2)
756-778: Add explicit accessibility semantics to the icon-only mobile toggle.On Line 761, VoiceOver currently has to infer meaning from the symbol. Add a localized
accessibilityLabel/accessibilityValueso users can hear current mode and target state clearly.♿ Suggested accessibility improvement
private var mobileViewportButton: some View { Button(action: { panel.setMobileViewport(!panel.isMobileViewport) panel.reload() }) { Image(systemName: "iphone") .symbolRenderingMode(.monochrome) .cmuxFlatSymbolColorRendering() .font(.system(size: devToolsButtonIconSize, weight: .medium)) .foregroundStyle(panel.isMobileViewport ? cmuxAccentColor() : Color(nsColor: .secondaryLabelColor)) .frame(width: addressBarButtonSize, height: addressBarButtonSize, alignment: .center) .background( RoundedRectangle(cornerRadius: 4) .fill(panel.isMobileViewport ? cmuxAccentColor().opacity(0.2) : Color.clear) ) } .buttonStyle(OmnibarAddressButtonStyle()) .frame(width: addressBarButtonSize, height: addressBarButtonSize, alignment: .center) .safeHelp(panel.isMobileViewport ? String(localized: "browser.mobileViewport.switchToDesktop", defaultValue: "Switch to Desktop View") : String(localized: "browser.mobileViewport.switchToMobile", defaultValue: "Switch to Mobile View (\(mobileViewportWidth)×\(mobileViewportHeight))")) + .accessibilityLabel(String(localized: "browser.mobileViewport.toggle", defaultValue: "Mobile viewport")) + .accessibilityValue( + panel.isMobileViewport + ? String(localized: "browser.mobileViewport.state.mobile", defaultValue: "Mobile \(mobileViewportWidth)×\(mobileViewportHeight)") + : String(localized: "browser.mobileViewport.state.desktop", defaultValue: "Desktop") + ) .accessibilityIdentifier("BrowserMobileViewportButton") }As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI (labels, buttons, menus, dialogs, tooltips, error messages)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 756 - 778, The mobileViewportButton is icon-only so VoiceOver cannot infer state; add explicit localized accessibility semantics by attaching an accessibilityLabel and accessibilityValue (or a combined accessibilityLabel describing current mode and target action) to the Button or its Image. Use String(localized: "browser.mobileViewport.currentMode", defaultValue: "Mobile View") and String(localized: "browser.mobileViewport.targetAction", defaultValue: "Switch to Desktop View") (and the inverse for desktop state) and reference panel.isMobileViewport to choose which localized strings to supply; keep the existing safeHelp but add .accessibilityLabel(...) and .accessibilityValue(...) (or one combined .accessibilityLabel(...)) to the mobileViewportButton so VoiceOver reads the current mode and the action.
1057-1063: Clamp persisted viewport dimensions before applying frame constraints.If settings ever persist
<= 0, Lines 1058-1059 can collapse or invalidate layout. Defensive clamping here makes the view robust regardless of storage state.🛡️ Suggested defensive clamp
- .frame( - maxWidth: panel.isMobileViewport ? CGFloat(mobileViewportWidth) : .infinity, - maxHeight: panel.isMobileViewport ? CGFloat(mobileViewportHeight) : .infinity, - alignment: .topLeading - ) + .frame( + maxWidth: panel.isMobileViewport ? CGFloat(max(1, mobileViewportWidth)) : .infinity, + maxHeight: panel.isMobileViewport ? CGFloat(max(1, mobileViewportHeight)) : .infinity, + alignment: .topLeading + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 1057 - 1063, Clamp the persisted viewport dimensions before applying the frame modifiers: when using panel.isMobileViewport with mobileViewportWidth and mobileViewportHeight in the .frame(...) calls, ensure the values are defensively clamped to a positive minimum (e.g., >= 1) so <=0 values from storage don't collapse the layout; update the code that reads/uses mobileViewportWidth and mobileViewportHeight (the values passed into the .frame(maxWidth:..., maxHeight:...)) to use a clampedWidth/clampedHeight derived from max(1, persistedValue) or similar before applying the frame constraints.Sources/Panels/BrowserPanel.swift (1)
3865-3872: The sole caller already reloads immediately after toggling, so current behavior is functionally correct, but the API contract remains fragile.
setMobileViewport(_:)is called only in BrowserPanelView.swift (line 758), and that call site explicitly invokesreload()on line 759. The current implementation works without bugs. However, the function design—requiring callers to remember to reload—is fragile and could mislead future maintainers. Consider documenting the reload expectation more prominently, or restructure the API to own the reload whenshouldRenderWebViewis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 3865 - 3872, setMobileViewport(_:) currently flips isMobileViewport and updates webView.customUserAgent but leaves reload responsibility to callers, which is fragile; modify setMobileViewport(_ enabled: Bool) (and its use sites like the caller in BrowserPanelView.swift) so the method itself triggers reload() when shouldRenderWebView is true (i.e., after setting isMobileViewport and updating webView.customUserAgent call reload()), or alternatively add a clear doc comment on setMobileViewport(_: ) stating it does NOT reload and callers must call reload()—pick one approach and apply consistently so future callers of setMobileViewport(_: ) don’t forget to call reload().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 4243-4249: Replace the three bare string literals used in the
viewport controls with localized strings: change TextField placeholders "W" and
"H" (the TextField initializers bound to mobileViewportWidth and
mobileViewportHeight) to use String(localized:
"settings.viewport.width.placeholder", defaultValue: "W") and String(localized:
"settings.viewport.height.placeholder", defaultValue: "H"), and replace the
separator Text("×") with Text(String(localized: "settings.viewport.separator",
defaultValue: "×")); ensure the localization keys are descriptive and consistent
with other settings keys and that the TextField and Text initializers continue
to use the same values/format arguments.
---
Nitpick comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3865-3872: setMobileViewport(_:) currently flips isMobileViewport
and updates webView.customUserAgent but leaves reload responsibility to callers,
which is fragile; modify setMobileViewport(_ enabled: Bool) (and its use sites
like the caller in BrowserPanelView.swift) so the method itself triggers
reload() when shouldRenderWebView is true (i.e., after setting isMobileViewport
and updating webView.customUserAgent call reload()), or alternatively add a
clear doc comment on setMobileViewport(_: ) stating it does NOT reload and
callers must call reload()—pick one approach and apply consistently so future
callers of setMobileViewport(_: ) don’t forget to call reload().
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 756-778: The mobileViewportButton is icon-only so VoiceOver cannot
infer state; add explicit localized accessibility semantics by attaching an
accessibilityLabel and accessibilityValue (or a combined accessibilityLabel
describing current mode and target action) to the Button or its Image. Use
String(localized: "browser.mobileViewport.currentMode", defaultValue: "Mobile
View") and String(localized: "browser.mobileViewport.targetAction",
defaultValue: "Switch to Desktop View") (and the inverse for desktop state) and
reference panel.isMobileViewport to choose which localized strings to supply;
keep the existing safeHelp but add .accessibilityLabel(...) and
.accessibilityValue(...) (or one combined .accessibilityLabel(...)) to the
mobileViewportButton so VoiceOver reads the current mode and the action.
- Around line 1057-1063: Clamp the persisted viewport dimensions before applying
the frame modifiers: when using panel.isMobileViewport with mobileViewportWidth
and mobileViewportHeight in the .frame(...) calls, ensure the values are
defensively clamped to a positive minimum (e.g., >= 1) so <=0 values from
storage don't collapse the layout; update the code that reads/uses
mobileViewportWidth and mobileViewportHeight (the values passed into the
.frame(maxWidth:..., maxHeight:...)) to use a clampedWidth/clampedHeight derived
from max(1, persistedValue) or similar before applying the frame constraints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afcac171-e5ab-4098-80b6-5951ed6d8278
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Resources/Localizable.xcstrings">
<violation number="1" location="Resources/Localizable.xcstrings:4760">
P2: Mobile viewport tooltip text is hardcoded to `390px` in localization, causing stale/incorrect help text when viewport dimensions are user-configurable.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:3103">
P2: New mobile viewport size settings are persisted but not included in `resetAllSettings()`, so 'Reset All Settings' does not fully reset browser configuration.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanel.swift (1)
3422-3425: Centralize UA application to avoid drift across call paths.UA assignment is repeated in navigation, reload, and toggle paths. A single helper reduces mismatch risk over time.
♻️ Proposed refactor
+ private var effectiveUserAgent: String { + isMobileViewport + ? BrowserUserAgentSettings.mobileUserAgent + : BrowserUserAgentSettings.safariUserAgent + } + + private func applyEffectiveUserAgent() { + webView.customUserAgent = effectiveUserAgent + } private func performNavigation( request: URLRequest, originalURL: URL, recordTypedNavigation: Bool, preserveRestoredSessionHistory: Bool ) { @@ - webView.customUserAgent = isMobileViewport - ? BrowserUserAgentSettings.mobileUserAgent - : BrowserUserAgentSettings.safariUserAgent + applyEffectiveUserAgent() shouldRenderWebView = true @@ func reload() { - webView.customUserAgent = isMobileViewport - ? BrowserUserAgentSettings.mobileUserAgent - : BrowserUserAgentSettings.safariUserAgent + applyEffectiveUserAgent() webView.reload() } @@ func setMobileViewport(_ enabled: Bool) { isMobileViewport = enabled - webView.customUserAgent = enabled - ? BrowserUserAgentSettings.mobileUserAgent - : BrowserUserAgentSettings.safariUserAgent + applyEffectiveUserAgent() }Also applies to: 3859-3861, 3869-3871
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 3422 - 3425, Centralize user-agent assignment by adding a single helper (e.g., applyUserAgent()) that sets webView.customUserAgent = isMobileViewport ? BrowserUserAgentSettings.mobileUserAgent : BrowserUserAgentSettings.safariUserAgent and call this helper from all places that currently set the UA (navigation handling, reload logic, and the mobile viewport toggle code paths where isMobileViewport is changed) so the same logic is used everywhere and drift is prevented; update callers to invoke applyUserAgent() after any change to isMobileViewport or when reloading/navigating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 4757-4766: The localized labels currently hardcode "390px" in the
value strings for both English ("Switch to Mobile View (390px)") and Japanese
("モバイル表示に切替 (390px)"); change those values to a format placeholder (e.g. "Switch
to Mobile View (%@)" and "モバイル表示に切替 (%@)") or to a generic label (e.g. "Switch
to Mobile View") and then append/format the live dimensions in Swift when
displaying (use NSLocalizedString for the key and supply the runtime
width×height string), updating the entries under the same stringUnit keys so
localization tooling stays intact.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3865-3867: Update the documentation for setMobileViewport(_:) to
accurately state that it updates mobile emulation state and user-agent only, not
the frame size; remove or change the claim “Only resizes the frame — does NOT
reload the page.” and instead mention that frame/resizing behavior is handled in
BrowserPanelView (e.g., BrowserPanelView.applyFrameConstraints or similar) so
callers know setMobileViewport only toggles UA/viewport state and does not
perform layout changes.
---
Nitpick comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3422-3425: Centralize user-agent assignment by adding a single
helper (e.g., applyUserAgent()) that sets webView.customUserAgent =
isMobileViewport ? BrowserUserAgentSettings.mobileUserAgent :
BrowserUserAgentSettings.safariUserAgent and call this helper from all places
that currently set the UA (navigation handling, reload logic, and the mobile
viewport toggle code paths where isMobileViewport is changed) so the same logic
is used everywhere and drift is prevented; update callers to invoke
applyUserAgent() after any change to isMobileViewport or when
reloading/navigating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29cf1ad8-626c-44d7-a31a-2e3082452d72
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Panels/BrowserPanelView.swift
- Sources/cmuxApp.swift
ryuuuuukun
force-pushed
the
feat/browser-mobile-viewport-toggle
branch
2 times, most recently
from
7600deb to
f88e611
Compare
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 `@Sources/cmuxApp.swift`:
- Around line 4238-4265: The two TextField bindings for mobile viewport
dimensions (the value bindings mobileViewportWidth and mobileViewportHeight used
in the SettingsCardRow) currently allow zero or negative numbers; clamp these
values to a valid positive range (e.g. minimum of 1) by validating on change or
in the property setters so the stored values are replaced with max(1, newValue)
(and optionally clip to a sensible upper bound). Update the handlers around the
TextField bindings (the mobileViewportWidth/mobileViewportHeight change path) to
enforce this constraint so the mobile preview never receives 0/negative
dimensions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e2f374a-59bd-48d4-a191-5f5f8b29039c
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/BrowserPanelView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanelView.swift (1)
619-623: Consider keeping the mobile toggle visible on new-tab pages.With this condition, users can’t turn mobile mode off while on the blank/new-tab state. Keeping
mobileViewportButtonvisible would improve recovery/discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 619 - 623, The conditional hides the mobileViewportButton when panel.isShowingNewTabPage, preventing users from toggling mobile mode on new-tab pages; change the UI condition so mobileViewportButton is always included (remove it from the guarded block) while keeping browserThemeModeButton and developerToolsButton inside the if !panel.isShowingNewTabPage block, ensuring mobileViewportButton remains visible even when panel.isShowingNewTabPage is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 756-777: The icon-only mobileViewportButton lacks an explicit
accessibilityLabel and uses a concatenated help string; update the safeHelp and
add an accessibilityLabel (or accessibilityValue/state) for mobileViewportButton
using fully localized format strings via String(localized:..., defaultValue:...)
for both states, e.g. keys for "browser.mobileViewport.switchToDesktop" and a
formatted key like "browser.mobileViewport.switchToMobile.withDimensions" that
accepts width/height (use mobileViewportWidth and mobileViewportHeight when
formatting), and base the label/state on panel.isMobileViewport so VoiceOver
gets a descriptive localized string rather than a raw concatenation.
- Around line 1056-1059: The frame call uses persisted
mobileViewportWidth/mobileViewportHeight directly, which can be <= 0 and
collapse the web surface; before applying them in the .frame for
panel.isMobileViewport (the view using panel.isMobileViewport and the
mobileViewportWidth/mobileViewportHeight symbols), clamp those persisted
dimensions to a safe minimum (e.g., max(1, width) / max(1, height) or a defined
MobileViewport.minimumWidth/Height) and use the clamped CGFloat values in the
maxWidth/maxHeight parameters so stale/corrupt values cannot produce a zero or
negative frame.
---
Nitpick comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 619-623: The conditional hides the mobileViewportButton when
panel.isShowingNewTabPage, preventing users from toggling mobile mode on new-tab
pages; change the UI condition so mobileViewportButton is always included
(remove it from the guarded block) while keeping browserThemeModeButton and
developerToolsButton inside the if !panel.isShowingNewTabPage block, ensuring
mobileViewportButton remains visible even when panel.isShowingNewTabPage is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bd3b035-c3fe-46fe-90f1-ba712da239cf
📒 Files selected for processing (1)
Sources/Panels/BrowserPanelView.swift
ryuuuuukun
force-pushed
the
feat/browser-mobile-viewport-toggle
branch
from
fec9e30 to
d14d42c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Sources/Panels/BrowserPanel.swift (1)
3982-3984:⚠️ Potential issue | 🟡 Minor
setMobileViewportdocs are still inaccurate.Line 3983 says this method “resizes the frame,” but this function only updates viewport mode state + UA. That mismatch can mislead call sites.
📝 Suggested doc-only fix
- /// Toggle mobile viewport emulation (iPhone 14 equivalent UA + 390px viewport width). - /// Only resizes the frame — does NOT reload the page. + /// Toggle mobile viewport emulation by updating panel state and User-Agent. + /// Frame constraints are handled by BrowserPanelView; callers decide whether to reload. func setMobileViewport(_ enabled: Bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 3982 - 3984, The docstring for setMobileViewport is misleading: it claims the method "resizes the frame — does NOT reload the page" but the implementation only updates viewport mode state and the user agent string. Update the documentation on the setMobileViewport(_:) function to accurately state that it toggles the internal mobile viewport mode and user-agent (e.g., sets viewportMode and updates UA) without changing the window/frame size or triggering a reload, and remove the wording about resizing the frame.Sources/cmuxApp.swift (1)
4784-4800:⚠️ Potential issue | 🟠 MajorLocalize the viewport control literals and enforce positive dimension bounds.
This block reintroduces previously flagged issues: bare user-facing literals (
"W","×","H") and unrestricted numeric inputs that can store invalid values (e.g.,0/negative).💡 Proposed fix
SettingsCardRow( String(localized: "settings.browser.mobileViewport", defaultValue: "Mobile Viewport Size"), subtitle: String(localized: "settings.browser.mobileViewport.subtitle", defaultValue: "Width × Height used for the mobile viewport toggle.") ) { HStack(spacing: 4) { - TextField("W", value: $mobileViewportWidth, format: .number) + TextField( + String(localized: "settings.browser.mobileViewport.widthPlaceholder", defaultValue: "W"), + value: $mobileViewportWidth, + format: .number + ) .textFieldStyle(.roundedBorder) .frame(width: 64) .multilineTextAlignment(.center) - Text("×") + Text(String(localized: "settings.browser.mobileViewport.separator", defaultValue: "×")) .foregroundStyle(.secondary) - TextField("H", value: $mobileViewportHeight, format: .number) + TextField( + String(localized: "settings.browser.mobileViewport.heightPlaceholder", defaultValue: "H"), + value: $mobileViewportHeight, + format: .number + ) .textFieldStyle(.roundedBorder) .frame(width: 64) .multilineTextAlignment(.center) } + .onChange(of: mobileViewportWidth) { newValue in + mobileViewportWidth = min(max(newValue, 1), 4096) + } + .onChange(of: mobileViewportHeight) { newValue in + mobileViewportHeight = min(max(newValue, 1), 4096) + } }As per coding guidelines: “All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")… Never use bare string literals in SwiftUIText(),Button(), alert titles, etc.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 4784 - 4800, Replace bare literals in the viewport control with localized strings and prevent zero/negative sizes: change the "W", "H", and "×" Text/TextField placeholders to use String(localized: ..., defaultValue: ...) keys (e.g., "settings.browser.mobileViewport.width.short", "settings.browser.mobileViewport.height.short", "settings.browser.mobileViewport.times") and ensure the bound state variables mobileViewportWidth and mobileViewportHeight cannot become <= 0 by clamping them to a minimum (e.g., 1) when they change (use .onChange or a didSet/validation helper) and/or enforce a minimum in the input handling where SettingsCardRow and the two TextField bindings are defined so only positive integers are stored.Resources/Localizable.xcstrings (1)
4947-4953:⚠️ Potential issue | 🟡 MinorRemove hardcoded
390pxfrom the mobile-toggle label.Line 4947 and Line 4953 hardcode the default width, so the text can drift from user-configured viewport dimensions.
Proposed localization fix
- "value": "Switch to Mobile View (390px)" + "value": "Switch to Mobile View (%@)" ... - "value": "モバイル表示に切替 (390px)" + "value": "モバイル表示に切替 (%@)"Then pass the live dimension string from Swift (for example,
390×844) when rendering this label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 4947 - 4953, Update the localized mobile-toggle label in Resources/Localizable.xcstrings by removing the hardcoded "390px" text (e.g., replace "Switch to Mobile View (390px)" / "モバイル表示に切替 (390px)" with a placeholder-based string such as "Switch to Mobile View (%@)" and corresponding Japanese placeholder) and then pass the live dimension string (e.g., "390×844") from the Swift UI code when setting the label (where the label is created/updated) so the displayed value reflects the current configured viewport dimension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 900-901: Revert the unintended product name/identifier changes:
restore PRODUCT_BUNDLE_IDENTIFIER to com.cmuxterm.app and PRODUCT_NAME to cmux
in the project.pbxproj, and also restore TEST_HOST to reference
"$(BUILT_PRODUCTS_DIR)/cmux.app/Contents/MacOS/cmux"; if the new name is
intentional, instead update all dependent scripts and config to match (update
scripts/build-sign-upload.sh's APP_PATH, appcast/Sparkle identifiers, and
scripts/reload.sh, reloadp.sh, rebuild.sh) and ensure Sparkle's bundle
identifier matches the new PRODUCT_BUNDLE_IDENTIFIER so auto-updates continue to
work.
In `@Sources/cmuxApp.swift`:
- Around line 3571-3572: resetAllSettings() does not restore the new mobile
viewport AppStorage values; update resetAllSettings() to explicitly reset
mobileViewportWidth and mobileViewportHeight back to
BrowserMobileViewportSettings.defaultWidth and
BrowserMobileViewportSettings.defaultHeight (or clear their AppStorage keys) so
the persisted viewport dimensions are returned to defaults when "Reset All
Settings" runs.
---
Duplicate comments:
In `@Resources/Localizable.xcstrings`:
- Around line 4947-4953: Update the localized mobile-toggle label in
Resources/Localizable.xcstrings by removing the hardcoded "390px" text (e.g.,
replace "Switch to Mobile View (390px)" / "モバイル表示に切替 (390px)" with a
placeholder-based string such as "Switch to Mobile View (%@)" and corresponding
Japanese placeholder) and then pass the live dimension string (e.g., "390×844")
from the Swift UI code when setting the label (where the label is
created/updated) so the displayed value reflects the current configured viewport
dimension.
In `@Sources/cmuxApp.swift`:
- Around line 4784-4800: Replace bare literals in the viewport control with
localized strings and prevent zero/negative sizes: change the "W", "H", and "×"
Text/TextField placeholders to use String(localized: ..., defaultValue: ...)
keys (e.g., "settings.browser.mobileViewport.width.short",
"settings.browser.mobileViewport.height.short",
"settings.browser.mobileViewport.times") and ensure the bound state variables
mobileViewportWidth and mobileViewportHeight cannot become <= 0 by clamping them
to a minimum (e.g., 1) when they change (use .onChange or a didSet/validation
helper) and/or enforce a minimum in the input handling where SettingsCardRow and
the two TextField bindings are defined so only positive integers are stored.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3982-3984: The docstring for setMobileViewport is misleading: it
claims the method "resizes the frame — does NOT reload the page" but the
implementation only updates viewport mode state and the user agent string.
Update the documentation on the setMobileViewport(_:) function to accurately
state that it toggles the internal mobile viewport mode and user-agent (e.g.,
sets viewportMode and updates UA) without changing the window/frame size or
triggering a reload, and remove the wording about resizing the frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6d41bbf-263e-4aeb-9408-0a08dcd26d10
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/BrowserPanelView.swift
|
This! Please merge! Need this every day! @austinywang promise you, best feature ever :D |
Author
|
Conflicts have been resolved. Could you please review and merge when you have a chance? Thank you! |
Add a phone icon button to the browser toolbar that toggles mobile viewport emulation (default 390×844, iPhone 14). Switches User-Agent to mobile Safari for accurate responsive rendering. Viewport size is configurable via Settings > Browser. Localized tooltips (en/ja). Closes manaflow-ai#1614 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ryuuuuukun
force-pushed
the
feat/browser-mobile-viewport-toggle
branch
from
d5d29b8 to
7db7ba4
Compare
Author
|
Rebased onto latest main and squashed into a single clean commit. The previous branch had stale merge commits and unrelated noise mixed in — all cleaned up now. @austinywang Would appreciate a review when you get a chance! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1614
Screenshots
Changes
BrowserPanel.swiftBrowserMobileViewportSettings,setMobileViewport()methodBrowserPanelView.swiftcmuxApp.swiftLocalizable.xcstringsTest plan
navigator.userAgentin DevTools console)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Localization
Branding