Repository navigation
fix: the 2026-10-17 review's medium findings (reinstall approval, setup tests, pagination, AppService scope) - #6
Merged
Conversation
--reinstall uninstalls the app first and deletes everything on the instance that is not in source, including the records in the app's tables. The deploy-and-verify skill let an agent retry with it after only telling the user. The skill (both copies, via npm run sync:skills), AGENTS.md rule 2 and TROUBLESHOOTING.md now require the user's explicit approval first.
setup.mjs is the first command every adopter runs and had no tests. Add npm test (node --test) and tests/setup.test.mjs, which runs setup.mjs on temporary copies of the repository, as a git checkout with an origin and as a plain directory, and checks: - the scope and name rewrite (no x_snc_example left outside the files setup skips), the new scopeId, package.json, the lockfile root, CHANGELOG.md and the removal of src/fluent/generated/; - the .gitignore-aware fallback walk outside git; - the refusals: unsafe scopes and names, a dirty tree, and a second run without --force, which must write nothing; --dry-run writes nothing. The setup tests skip themselves once the app is set up, and setup.mjs leaves tests/setup.test.mjs as written, so derived apps keep a passing npm test. CI runs npm test in the build job; docs list the command.
Offset pages over a newest-first list shift when rows are created between requests, so "Load more" could repeat the last shown row and give React duplicate keys; every create, update or delete reloaded only the first page and dropped the rest. - src/client/services/pagination.js: mergeById (one row per sys_id, the newer copy in place) and loadPages (reload N pages from the top). - App.jsx merges each "Load more" page by sys_id, counts loaded pages and reloads all of them after a change; "Load more" waits for a running reload and drops an answer that a reload overtook. - AppService.listItems orders sys_created_on ties by sys_id, so rows created in the same second do not move between pages. - tests/pagination.test.mjs covers the helpers and both scenarios. Same API (limit and offset). Built and unit-tested; it needs an instance to observe in the UI page.
FP, ITEM_TABLE, CATEGORIES, MAX_NAME and the other limits were top-level vars in the Script Include, which may be shared with other Script Includes in the same transaction: a second file declaring CATEGORIES or MAX_NAME could change AppService's validation depending on load order. Wrap AppService.server.js in a function that returns the class, so AppService is the only name it declares. The compiled sys_script_include record differs only in its script text. A vm comparison of the old and new file over create/update/delete/list/readBody inputs gave identical results. tests/app-service.test.mjs checks that only AppService is declared, that colliding globals declared before or after it change nothing, and the list query. AGENTS.md rule 5, the Script Include recipe and TROUBLESHOOTING.md teach the pattern. Not yet run on an instance.
… the approval rule The quoted test glob needs Node 22's own glob expansion; unquoted, the shell expands it on Node 20. TROUBLESHOOTING's cross-scope section now carries the same approval caveat, and the setup tests' temp repos ignore global and system git config and hooks.
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.
Fixes four open medium findings from the 2026-10-17 repository review. They are build-verified only: no instance was used. The instance steps are below, and the owner decision is listed at the end.
--reinstallafter only telling the user--reinstalldeletes the app's table recordssetup.mjshad no tests or CI coveragenpm test(tests/setup.test.mjs) runs setup on temp copies, with and without git. It checks the rename, the new scopeId, the lockfile, CHANGELOG and keys reset, and the refusals (unsafe names, a dirty tree, a second run without--forcethat writes nothing). The tests skip in apps made from the template. CI runs them. The temp repos ignore global and system git config and hookssys_id, and every loaded page reloads after a create, update or delete. A "Load more" answer that a reload overtook is dropped. The server breakssys_created_onties bysys_idvarconstants could collide across Script IncludesAppServiceis the only global the file declares. The built record's XML outside<script>is byte-identical to main's (api_name, access, the callable flags)Verified:
npm run lintandnpm run build:ci(frozen keys;keys.tsunchanged) pass.npm testpasses 39/39 on Node 20.18.0, 20.20.2, 22.23.3 and 26.sync:skills --checkpasses.--reinstallmention had no approval rule.Instance steps (not run):
npm run build, thennpm run deploy -- --auth <alias>, thennpm run test:instance -- --auth <alias>. The ATF suite shows AppService still resolves and validates through the wrapper.x_snc_example_app.dowith more than 50 items and click "Load more".Known limits (follow-ups):
release.ymldoesn't runnpm test.Owner decision (not applied)
example-item.now.ts:9, 27;app-acls.now.ts:15-20). Turn offallowWebServiceAccess, or enforce the rules on the platform (a before insert/update business rule or a data policy) with an ATF step for the Table API path?