feat: polish app#2166
Conversation
deadlyjack
commented
Jun 8, 2026
deadlyjack
commented
- Bump version to 1.12.3 (versionCode 973), sync package.json/lockfile
- Add "terminal not installed prompt" and "terminal first launch prompt" i18n strings (32 locales)
- Show terminal installation prompt on first launch; prompt on navigating to public/ directory
- Auto-enable update checks without prompt for Play Store installs; guard against NaN versionCode
- Move ad initialization to run only on non-Play Store builds after promotions fetch
- Prevent eruda download failures from crashing developer tools init
- Remove stray admob extraneous entry from lockfile
- Bump version to 1.12.3 (versionCode 973), sync package.json/lockfile - Add "terminal not installed prompt" and "terminal first launch prompt" i18n strings (32 locales) - Show terminal installation prompt on first launch; prompt on navigating to public/ directory - Auto-enable update checks without prompt for Play Store installs; guard against NaN versionCode - Move ad initialization to run only on non-Play Store builds after promotions fetch - Prevent eruda download failures from crashing developer tools init - Remove stray admob extraneous entry from lockfile
github-actions
Bot
added
the
translations
Greptile SummaryThis PR bumps the version to 1.12.3 and delivers a set of polish fixes: terminal install prompts on first launch and when navigating to
Confidence Score: 4/5Safe to merge with minor follow-up; no data-loss or security regressions, but three UX rough edges warrant attention before or shortly after shipping. The version bump, i18n additions, NaN guard, and ad/update-check logic changes are all correct. The three issues are: the empty catch in devTools.js losing the original download-error signal, the fileBrowser terminal flow navigating to public/ even when installation failed, and the first-launch prompt offering no loader or error feedback to the user. None break core editing functionality. src/lib/devTools.js (silent catch loses diagnostic info) and src/pages/fileBrowser/fileBrowser.js (navigation after failed install) deserve a second look before release. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[onDeviceReady] --> B[Get install source]
B --> C{Play Store?}
C -- Yes --> D[Auto-enable update checks]
C -- No --> E[Show update check consent dialog]
D --> F[loadApp + plugins]
E --> F
F --> G[fetchPromotions + startAd]
G --> H[promptTerminalInstall]
H --> I{Already prompted?}
I -- Yes --> Z[Done]
I -- No --> J{Terminal installed?}
J -- Yes --> Z
J -- No --> K{Supported?}
K -- No --> Z
K -- Yes --> L[Show first-launch confirm]
L -- No --> M[Set flag, done]
L -- Yes --> N[fire-and-forget checkAndInstallTerminal]
FB[fileBrowser: navigate to public/] --> FB1{Terminal installed?}
FB1 -- Yes --> FB2[navigate]
FB1 -- No --> FB3[Show install prompt]
FB3 -- No --> FB4[return, no navigation]
FB3 -- Yes --> FB5[Show loader + install]
FB5 --> FB6[navigate regardless of install result]
Reviews (1): Last reviewed commit: "feat: polish app" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch { | ||
| } finally { | ||
| if (showLoader) loader.destroy(); | ||
| } |
There was a problem hiding this comment.
The empty catch {} swallows the CDN download error, so control falls through to helpers.toInternalUri(erudaPath) and the <script> tag load — on a file that was never written. The onerror callback then rejects the inner Promise, which is caught by the outer catch and re-thrown. Dev tools init still fails, but the caller now sees a cryptic "script load error" rather than the original CDN/network error, making the failure harder to diagnose. At minimum the caught error should be logged; better still, return early to avoid the misleading follow-on failure.
| try { | ||
| loaderInstance.show(); | ||
| const res = await terminalManager.checkAndInstallTerminal(); | ||
| if (res.error) { | ||
| throw new Error(res.error); | ||
| } | ||
| } catch (error) { | ||
| helpers.error(error); | ||
| } finally { | ||
| loaderInstance.destroy(); | ||
| } | ||
| } else { | ||
| return; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.error("Terminal check failed:", e); | ||
| helpers.error(e, dirUrl); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
When the user confirms installation but terminalManager.checkAndInstallTerminal() rejects (or res.error is set), helpers.error(error) is called but then execution falls out of the inner try/catch/finally and reaches navigate(dirUrl, dirName) regardless. The user sees an error dialog and is then navigated into public/ with a terminal that is still not installed. Consider only calling navigate() when either (a) the terminal was already installed, or (b) the user chose to install and installation succeeded.
| if (shouldInstall) { | ||
| const { default: terminalManager } = await import( | ||
| "components/terminal/terminalManager" | ||
| ); | ||
| terminalManager.checkAndInstallTerminal().catch(console.error); | ||
| } |
There was a problem hiding this comment.
When the user accepts the first-launch terminal prompt, terminalManager.checkAndInstallTerminal() is kicked off with .catch(console.error) and no loader UI. If installation fails, the user gets no visible feedback (compare with checkAndNavigate in fileBrowser.js, which wraps the same call in a loader and surfaces the error via helpers.error). Consider adding a loader and an error alert here as well for consistency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
We should not remove the opt-in prompt for checking updates on play store builds, it makes sense to let the user decide and have their consent before checking updates for them. |
2 participants
