Add error notification banner system#649
Merged
Merged
Conversation
Route runtime error surfaces through one ui/notifications.js module instead of inconsistent printText overlays and duplicated inline-style banners. Unrecoverable failures (startup, WebGL context loss, physics out-of-memory) show a banner with a Reload action; project crashes show a friendly dismissible banner. Recoverable problems (CSG2, autosave, storage) log a console warning only. All banner text is translatable and theme-aware; raw error detail stays in the console. A crashing project no longer reloads the starter project over the child's work. Autosave refuses to overwrite a good save with an empty workspace, so the banner's Reload action restores reliably. https://claude.ai/code/session_014dfd5yijb6Td1SP93T97Cn
Contributor
📝 WalkthroughWalkthroughThis PR implements a centralized error and notification system for Flock, replacing scattered console logging and direct DOM manipulation with a unified error handler that displays accessible, themed banners to users while logging enhanced details to the console for developers. ChangesCentralized Error Notification System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 docstrings
🧪 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 |
Deploying flockxr with
|
| Latest commit: |
5d9e453
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8c5d0b64.flockxr.pages.dev |
| Branch Preview URL: | https://claude-determined-goldberg-y.flockxr.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/main.js`:
- Around line 1094-1103: The startup flow currently launches flock.initialize()
in a detached async IIFE so window.onload continues even if initialization
fails; change the code so the window.onload handler awaits flock.initialize
directly (make the onload handler async), keep the try/catch around await
flock.initialize, call handleError(error, { source: "startup", fatal: true }) in
the catch, and then return/exit the onload handler (do not continue startup or
call hideLoadingScreen) to prevent further app execution when flock.initialize
fails; this touches the flock.initialize call, the existing handleError usage,
and hideLoadingScreen so you only call hideLoadingScreen on successful
initialization.
In `@style.css`:
- Line 1768: The font-family declaration uses quoted single-word family name
"Asap"; update the CSS rule containing the font-family property (font-family:
"Asap", sans-serif;) to remove the quotes so it reads font-family: Asap,
sans-serif; ensuring the change is made where the font-family property is
defined in the stylesheet.
🪄 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: 6f00a9a6-a478-4070-9b3e-3c991b1e0394
📒 Files selected for processing (10)
flock.jslocale/en.jslocale/es.jsmain/execution.jsmain/files.jsmain/main.jsstyle.csstests/notifications.test.jstests/tests.htmlui/notifications.js
| align-items: center; | ||
| gap: 12px; | ||
| padding: 12px 16px; | ||
| font-family: "Asap", sans-serif; |
Contributor
There was a problem hiding this comment.
Remove quotes around the font family name.
The font family name "Asap" should not be quoted according to CSS conventions for single-word font names.
📝 Proposed fix
- font-family: "Asap", sans-serif;
+ font-family: Asap, sans-serif;As per coding guidelines, stylelint flagged: Expected no quotes around "Asap".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| font-family: "Asap", sans-serif; | |
| font-family: Asap, sans-serif; |
🧰 Tools
🪛 GitHub Actions: Prettier / prettier
[warning] Prettier reported formatting issues.
🪛 Stylelint (17.11.1)
[error] 1768-1768: Expected no quotes around "Asap" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@style.css` at line 1768, The font-family declaration uses quoted single-word
family name "Asap"; update the CSS rule containing the font-family property
(font-family: "Asap", sans-serif;) to remove the quotes so it reads font-family:
Asap, sans-serif; ensuring the change is made where the font-family property is
defined in the stylesheet.
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
Introduces a centralized error notification banner system to display user-friendly error messages for startup failures, physics crashes, WebGL context loss, and runtime errors. Replaces ad-hoc error handling with a consistent, accessible notification UI.
Key Changes
New notification module (
ui/notifications.js): Provides a DOM-based banner system with:showBanner()anddismissBanner()for displaying/removing notificationshandleError()as a single error funnel that logs details to console and shows friendly translated messagesisBenignAbort()to filter out benign abort errors (cancelled fetches, teardown) while preserving genuine physics OOM crashesinstallGlobalErrorHandlers()to catch unhandled errors and promise rejections globallyStyling (
style.css): Added.flock-bannerand related classes with theme-aware colors across all five themes (light, dark, dark high-contrast, high-contrast, low-vision)Error handling integration:
Autosave robustness (
main/files.js): Added validation to prevent overwriting good autosaves with empty workspace states, enabling recovery via the Reload actionTranslations: Added new message keys for all error types and banner actions in English and Spanish locales
Tests (
tests/notifications.test.js): Comprehensive test suite covering banner creation, deduplication, dismissal, actions, error handling, and benign abort detectionNotable Implementation Details
role="alert",aria-live="assertive") for accessibilityhttps://claude.ai/code/session_014dfd5yijb6Td1SP93T97Cn
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests