GitHub User Experience Enahncement#59
Open
jmsunseri wants to merge 31 commits into
Open
Conversation
Move source_files/resources deletion inside do_import_archive's transaction.atomic() block via new wipe_existing parameter. Move github_last_commit/github_last_sync save to after import succeeds, preventing a failed import from recording a false commit SHA or leaving the project in an empty state.
Remove setup_test_environment() call that conflicted with Django's test runner (was the reason these tests were .skip-ed). Rename test_import_archive.skip to .py. Add TestWipeExisting test class covering the new wipe_existing parameter: replaces source files and resources, preserves files on default (False), and rolls back on failure.
…elpers Start zip download in parallel with tree/manifest validation using ThreadPoolExecutor. Move the SHA check before any network work so unchanged repos return instantly without downloading anything. Extract validate_resources_against_tree() and parse_manifest_from_tree() as pure functions from github_pull. Extract PEBBLEJS_BUILTIN_RESOURCES as a frozenset constant. Add 24 unit tests covering get_root_path, resource validation, Pebble.js builtins, and manifest parsing including edge cases for variant suffixes, missing resources, subdirectories, package.json, and invalid JSON.
When github_pull detects a change between the stored commit and the remote branch, it now uses GitHub's Compare API to fetch only the files that changed, then applies minimal DB updates (add/modify/remove SourceFile and ResourceFile/ResourceVariant records) within an atomic transaction. A force=True parameter (exposed via checkbox in the pull modal) falls back to the existing full-zip wipe-and-replace path. This also serves as the path for initial pulls where github_last_commit is None. Extracted testable helpers: validate_resources_against_tree, parse_manifest_from_tree, _apply_delta_changes, _upsert_source_file, _upsert_resource_variant, _remove_file_by_path, _sync_resource_files_from_manifest, _fetch_file_content, _infer_resource_kind_from_path, _strip_resource_dir_prefix. Added 47 unit tests covering the new helpers, including FetchFileContentTest and RemoveFileByPathTest with mocked GitHub API. Fixed Django template syntax in github-pull-prompt modal.
Refactor SSE event handlers into a separate module for testability. Export handlers from the SSE module and add unit tests for both the JavaScript event handlers and the Python SSE event publishing logic.
- Emit proper SSE event types instead of embedding in JSON data - Replace spinning icon with animated "Pulling"/"Building" pending pill - Close pull prompt immediately on confirm; remove page reload hack - Move force-pull warning behind checkbox toggle - Auto-trigger build after webhook-initiated pull - Add GITHUB_HOOK_URL setting for ngrok in local development - Fix GitTree attribute access (x.path instead of x)
Moves the full page reload logic from OnPullComplete into a new CloudPebble.Sidebar.Refresh function. This updates project info and repopulates source/resource lists without losing editor state. Also removes `if (pane)` guards in github.js and adds docstrings to git test methods.
Contributor
Author
|
The tests are still failing because of a permissions issue regarding the pipeline adding comments to the github PR. Maybe this would work if it were coming from a branch on this repo and not an outside fork? |
Contributor
Author
There was a problem hiding this comment.
Pull request overview
This PR introduces a real-time event pipeline (Redis pub/sub → Django SSE → browser EventSource) to surface GitHub pull/build status in the UI, and refactors GitHub pulls to support incremental “delta” sync (with a full re-import fallback/option) plus auto-build behavior parity with webhook-triggered pulls.
Changes:
- Added SSE infrastructure: Redis event publisher, authenticated SSE endpoint, nginx streaming support, and browser-side
sse.jshandlers wired into the project page. - Implemented GitHub delta pull flow and a “wipe on pull” option (full re-import via
wipe_existing=True), plus additional tests around SSE, git sync, and archive import semantics. - Updated UI/UX: sidebar pending indicators, improved pull confirmation dialog, GitHub pane settings and status display, and “no full page reload” sidebar refresh.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents GITHUB_HOOK_URL and ngrok workflow for local webhook callbacks. |
| nginx/nginx.conf | Adds a streaming-friendly nginx location for the SSE endpoint. |
| docker-compose.yml | Adds optional .env.local loading for web and celery. |
| cloudpebble/utils/events.py | Introduces Redis pub/sub event publishing helper. |
| cloudpebble/ide/utils/cloudpebble_test.py | Removes explicit test environment setup call. |
| cloudpebble/ide/urls.py | Registers the new per-project SSE endpoint route. |
| cloudpebble/ide/tests/test_sse.py | Adds unit tests for SSE formatting, auth, and event publishing from tasks. |
| cloudpebble/ide/tests/test_import_archive.py | Adds tests for wipe_existing behavior and rollback safety. |
| cloudpebble/ide/tests/test_git.py | Adds extensive tests around git pull routing and delta sync helpers. |
| cloudpebble/ide/templates/ide/project/github.html | Adds “wipe on auto-pull” UI and GitHub last-sync display area. |
| cloudpebble/ide/templates/ide/project.html | Updates pull modal copy/controls and includes sse.js. |
| cloudpebble/ide/tasks/git.py | Implements delta sync, force/full pull routing, and publishes pull/build SSE events. |
| cloudpebble/ide/tasks/build.py | Publishes build completion SSE events. |
| cloudpebble/ide/tasks/archive.py | Adds wipe_existing option to delete existing files/resources during import. |
| cloudpebble/ide/static/ide/js/sse.js | Adds client-side SSE connection and event dispatch to existing handlers. |
| cloudpebble/ide/static/ide/js/sidebar.js | Adds pending-pill indicator helpers and a sidebar “refresh” method. |
| cloudpebble/ide/static/ide/js/github.js | Updates GitHub pane UI, adds pull force option, and SSE-driven pull callbacks. |
| cloudpebble/ide/static/ide/js/compile.js | Adds compile pane hooks for build start/complete events. |
| cloudpebble/ide/static/ide/js/cloudpebble.js | Initializes the SSE client during project init. |
| cloudpebble/ide/static/ide/js/tests/sse-events.test.js | Adds JS tests verifying SSE handler behavior. |
| cloudpebble/ide/static/ide/css/ide.css | Styles the sidebar pending pill. |
| cloudpebble/ide/models/project.py | Adds github_hook_force project setting. |
| cloudpebble/ide/migrations/0013_github_hook_force.py | Migrates the new github_hook_force field. |
| cloudpebble/ide/api/sse.py | Adds authenticated SSE streaming endpoint implementation. |
| cloudpebble/ide/api/project.py | Exposes hook_force in project info payload. |
| cloudpebble/ide/api/git.py | Adds force/full-pull parameter plumbing and persists hook-force setting. |
| cloudpebble/cloudpebble/settings.py | Adds GITHUB_HOOK_URL override for webhook callback base URL. |
| cloudpebble/.gitignore | Ignores node_modules/ in the cloudpebble subdir. |
| .gitignore | Ignores .env.local at repo root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+338
to
+347
| resource_root = project.resources_path + '/' | ||
| manifest_resources = manifest.get('resources', {}).get('media', []) | ||
| project_type = manifest.get('projectType', 'native') | ||
|
|
||
| for resource in manifest_resources: | ||
| path = resource_root + resource['file'] | ||
| if project_type == 'pebblejs' and resource['name'] in PEBBLEJS_BUILTIN_RESOURCES: | ||
| continue | ||
| if path not in paths_notags: | ||
| raise Exception("Resource %s not found in repo." % path) |
Contributor
Author
There was a problem hiding this comment.
this should be resolved now
Comment on lines
+473
to
+505
| manifest_kind = 'package.json' if 'pebble' in manifest else 'appinfo.json' | ||
| resource_root = project.resources_path + '/' | ||
|
|
||
| with transaction.atomic(): | ||
| project_options, media_map, dependencies = load_manifest_dict(manifest, manifest_kind) | ||
|
|
||
| for k, v in project_options.items(): | ||
| setattr(project, k, v) | ||
| project.full_clean() | ||
| project.set_dependencies(dependencies) | ||
|
|
||
| tag_map = {v: k for k, v in ResourceVariant.VARIANT_STRINGS.items() if v} | ||
|
|
||
| existing_sources = {s.project_path: s for s in project.source_files.all()} | ||
| existing_resources = {r.file_name: r for r in project.resources.all()} | ||
|
|
||
| for change in changed_files: | ||
| filename = change.filename | ||
| status = change.status | ||
|
|
||
| if status in ('added', 'modified', 'renamed'): | ||
| if status == 'renamed' and change.previous_filename: | ||
| _remove_file_by_path(project, change.previous_filename, existing_sources, existing_resources) | ||
|
|
||
| if filename.startswith(resource_root): | ||
| _upsert_resource_variant(project, repo, change, existing_resources, tag_map) | ||
| else: | ||
| try: | ||
| base_filename, target = SourceFile.get_details_for_path(project.project_type, filename) | ||
| _upsert_source_file(project, repo, change, base_filename, target, existing_sources) | ||
| except ValueError: | ||
| logger.debug("Skipping unrecognized file in delta: %s", filename) | ||
| continue |
Contributor
Author
There was a problem hiding this comment.
this should be resolved now
| tag_map = {v: k for k, v in ResourceVariant.VARIANT_STRINGS.items() if v} | ||
|
|
||
| existing_sources = {s.project_path: s for s in project.source_files.all()} | ||
| existing_resources = {r.file_name: r for r in project.resources.all()} |
Contributor
Author
There was a problem hiding this comment.
this should be resolved now
Comment on lines
+511
to
+514
|
|
||
| project.save() | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
this should be resolved now
Comment on lines
+317
to
+336
| CloudPebble.Editor.GetUnsavedFiles = function() { return 0; }; | ||
| $('#sidebar-sources').empty(); | ||
| $('#sidebar-resources').empty(); | ||
| create_initial_sections(CloudPebble.ProjectInfo.type); | ||
| Ajax.Get('/ide/project/' + PROJECT_ID + '/info').then(function(data) { | ||
| CloudPebble.ProjectInfo = data; | ||
| var is_alloy = data.type === 'alloy'; | ||
| $.each(data.source_files, function(index, value) { | ||
| if (is_alloy && value.target === 'embeddedjs' && value.is_binary) { | ||
| if (CloudPebble.Resources && _.isFunction(CloudPebble.Resources.AddAlloyAsset)) { | ||
| CloudPebble.Resources.AddAlloyAsset(value); | ||
| } | ||
| return; | ||
| } | ||
| CloudPebble.Editor.Add(value); | ||
| }); | ||
| $.each(data.resources, function(index, value) { | ||
| CloudPebble.Resources.Add(value); | ||
| }); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
this should be resolved now
Comment on lines
31
to
48
| @@ -43,6 +44,7 @@ def set_project_repo(request, project_id): | |||
| branch = request.POST['branch'] | |||
| auto_pull = bool(int(request.POST['auto_pull'])) | |||
| auto_build = bool(int(request.POST['auto_build'])) | |||
| hook_force = bool(int(request.POST.get('hook_force', '0'))) | |||
|
|
|||
Contributor
Author
There was a problem hiding this comment.
this should be fixed now
Comment on lines
+9
to
+14
| def publish_event(project_id, event_type, **kwargs): | ||
| data = {'type': event_type} | ||
| data.update(kwargs) | ||
| channel = 'project_events:{}'.format(project_id) | ||
| redis_client.publish(channel, json.dumps(data)) | ||
| logger.debug("Published %s to %s", event_type, channel) No newline at end of file |
Contributor
Author
There was a problem hiding this comment.
this should be fixed now
The validation function now supports `package.json` manifests where resources are nested under the `pebble` key. It also correctly handles projects cloned into a subdirectory by prepending the `root` path to the resource lookup. Fix a trailing parenthesis in README.
Use the full manifest path (including the resource directory prefix) when keying the existing resources dictionary, so that delta changes are correctly matched against the modified file paths.
When applying delta changes, pass media_map to _upsert_resource_variant so the correct resource kind is used from the manifest instead of inferring from the path. When syncing from manifest, update kind and menuIcon on existing resources if they differ.
Add pyrightconfig.json
Catch RedisError, ConnectionError and generic connection failures to prevent crashes when Redis is unavailable.
Use try/finally in the done callback to guarantee GetUnsavedFiles is restored even if an error is thrown. Also restore it on ajax failure.
Contributor
Author
|
@jplexer i think i have remedied all of the comments made by autipilot. Can you have it re-review to see if it thinks all of its concerns have been satisfied? |
Contributor
|
@jplexer can you try deploying to a dev instance and double checking it works pls |
Move was_clean to code_mirror instance so the state survives editor detach. Only reload the active buffer when it has no unsaved edits, preventing user work from being clobbered after a GitHub pull.
Wrap github_last_commit and github_last_sync saves in transaction.atomic() across full, delta, and no-new-commit pull paths so a partial failure never leaves the project with new files but the old SHA.
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
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
This MR adds real-time GitHub pull and build notifications via Server-Sent Events (SSE), introduces incremental delta sync for GitHub pulls, and fixes several bugs discovered along the way.
SSE Event System
/ide/project/<id>/events): Streams real-time events to the browser using Redis pub/sub. Events are published by Celery tasks (pull_start,pull_complete,pull_failed,build_start,build_complete) and delivered as properly formatted SSE named events (event: pull_start\ndata: {...}\n\n).sse.js: Connects to the SSE endpoint and dispatches events to handler functions exposed onCloudPebble.Events. Handlers update the sidebar with pending pill indicators (e.g., "Pulling...", "Building..." with animated dots) and trigger appropriate UI updates.utils/events.py:publish_event(project_id, event_type, **kwargs)utility for publishing events to Redis.Sidebar Pending Indicators
ShowPending/HidePending). These cycle through "Pulling" → "Pulling." → "Pulling.." → "Pulling..." and disappear cleanly on completion. The pills use the site's orange accent color (#ff4700).GitHub Pull Improvements
do_github_pullnow publishes SSE events and triggers an automatic build whengithub_hook_buildis enabled, matching the behavior of webhook-triggered pulls. Previously, manual pulls never triggered a build.get_root_path(x)was passingGitTreeElementobjects instead ofx.pathstrings in two locations in_github_pull_full, causing delta comparisons to fail.OnPullComplete/OnPullFailed/OnPullStarthandlers referenced a localpanevariable that was undefined outsideshow_github_pane(). Fixed to use direct jQuery selectors.window.location.reload(true),OnPullCompletecallsCloudPebble.Sidebar.Refresh()which silently re-fetches the file list from the server and rebuilds the sidebar without disrupting the user's current view. No navigation to the GitHub pane occurs.Build Notifications
build_startevents show a "Building..." pill in the sidebar.build_completeclears it.build_start— the pill is sufficient feedback.Configuration & Infrastructure
GITHUB_HOOK_URLenv var: Separate setting for GitHub webhook callback URL (via ngrok in dev), independent ofPUBLIC_URLwhich must remainlocalhostfor Firebase Auth.proxy_buffering off,proxy_cache off,proxy_read_timeout 86400, andchunked_transfer_encoding offto ensure proper streaming.docker-compose.yml: AddedGITHUB_HOOK_URLto environment config.GITHUB_HOOK_URLinstructions.Tests
test_sse.py, 20 tests): CoversSSEEventStreamformatting,publish_event, the SSE view auth/project checks,hooked_commitevent publishing,do_github_pullevent publishing and auto-build behavior, andrun_compileevent publishing.test_git.py, 67 tests): CoversGetRootPath,ValidateResourcesAgainstTree,ParseManifestFromTree,PebblejsBuiltins,InferResourceKind,StripResourceDirPrefix,FetchFileContent,RemoveFileByPath,GithubPullRoutingTest(force→full, delta, fallback, no-change, etc.),GithubPullDeltaTest,ApplyDeltaChangesTest,UpsertSourceFileTest,UpsertResourceVariantTest.sse-events.test.js, 8 tests): Loads realsse.jsviareadFileSync+new Function(), verifies handlers callShowPending/HidePendingandOnPullStart/OnPullComplete/etc. with correct arguments.Migration
0013_github_hook_force.py: Addsgithub_hook_forceboolean field to the Project model.New GitHub pull settings
The text here changes based on the checked or unchecked status
Warning if you check the box when manual pull
Default pull now only fetches the delta
Off camera i'm pushing a commit to github and you can see the label on the left that github is pulling then building. Notice testing.c stays because we did not have the force check box checked.
ScreencastAutomatic.webm.mp4
Now when you do a manual pull and the setting is set to automatically build it will actually automatically build. Notice the testing.c file goes away because we had the checkbox checked
ScreencastManual.webm.mp4