Change smart picker behavior by chrip · Pull Request #130 · Euro-Office/web-apps

chrip · 2026-06-19T13:57:37Z

This introduced calling smart picker by keystroke "/" anywhere in a doc, ppt, or xls.
Details are in #122

For testing you need connector app and web-apps on the following branches:
https://github.com/Euro-Office/eurooffice-nextcloud/tree/fix/issue-122-smart-picker-behavior
https://github.com/Euro-Office/web-apps/tree/fix/issue-122-smart-picker-behavior

MonaAghili · 2026-06-24T07:30:13Z

+            // on a selected cell and mid-edit (e.g. "asdf /"); _smartPickerReplace
+            // then drives the "/" removal on insert.
+            document.addEventListener('keydown', function(e) {
+                var key = e.key || (e.originalEvent && e.originalEvent.key);


I believe e.originalEvent only exists on jquery wrapped events
This is a native listener and e is already the KeyboardEvent
This fallback never runs Just var key = e.key

MonaAghili · 2026-06-24T08:09:08Z

+                    // Not editing (selected cell): append to the committed cell
+                    // text via the value path (no formula operators, so no "+");
+                    // drop a trailing "/" trigger if present.
+                    var cell = this.api.asc_getCellInfo && this.api.asc_getCellInfo();
+                    var cur = (cell && cell.asc_getText && cell.asc_getText()) || '';
+                    if (cur.slice(-1) === '/') {
+                        cur = cur.slice(0, -1);
+                    }
+                    this.api.asc_insertInCell(cur + data, Asc.c_oAscPopUpSelectorType.None);


The comment says "no formula operators so no +", but that's not really the issue here.
As I investigate
asc_getText() internally calls getValueForEdit() (WorksheetView.js:13669) which it returns the same value shown in the formula bar. For formula cells that's the formula itself (for example, =SUM(A1:A10)) not the evaluated result

As a result cur + data becomes something like:

=SUM(A1:A10)https://example.com (the formula source with the =) when you pass that to asc_insertInCell the SDK sees a string starting with = and tries to parse it as a formula
The url appended after it makes it syntactically invalid → the cell becomes #NAME?. The original formula is gone.

Even for non formula cells the behavior isn't ideal! If the cell contains 42 concatenation produces 42https://... converting the numeric value into plain text instead of preserving it as a number

Suggestion: if the cell is empty (cur === '') just insert data directly.

If the cell isn't empty, it's not obvious what the expected behavior should be. Should we replace the existing value append to it or reject the operation? It would be better to decide that explicitly instead of silently producing incorrect results for non-empty cells.

…mething Addresses the second review comment on PR #130. asc_getText() returns the formula-bar value, so cur + data wrote "=SUM(A1:A10)https://..." into a formula cell, which the SDK then failed to parse -- #NAME?, formula gone. The reviewer also noted values fare no better: "42" became "42https://..." and stopped being a number. Both are silent data loss. The previous guard only caught formulas. Appending, replacing and rejecting are all defensible for a non-empty cell, and the review asked for that decision to be explicit rather than implied, so: reject any non-empty cell and name the two unambiguous paths -- an empty cell, or editing the cell, where the branch above inserts at the cursor. An empty cell now inserts the link alone rather than concatenating onto ''. The string is renamed to txtCellNotEmpty since it is no longer only about formulas, and moved to keep the locale file sorted. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…nsert The "/" trigger cancelled its own keystroke. sdkjs inserts printable characters from CDocument.OnKeyPress (EnterText), which preventDefault on keydown suppresses, so "/" could not be typed after a space at all: the menu reopened on every attempt and the character never reached the document. insertLink then "removed" that "/" with pluginMethod_InputText('', '/'), which loops emulateKeyDownApi(8) once per character of textReplace -- deleting the real character before the caret, usually the space the user had just typed. The trigger no longer cancels anything, and the flow now follows Nextcloud's Text app, which drives the same interaction with @tiptap/suggestion configured as {char: '/', allowedPrefixes: [' ']}: the "/" is written to the document, each further character is written and narrows the list, space or a second "/" ends the match because tiptap's query class is [^\s/], backspacing over the trigger closes the menu, and accepting an entry replaces "/" plus the query the way tiptap's command() calls deleteRange(range). Up/Down/Enter/Tab/Escape are the only keys taken from the editor, which is why the listener moved to the capture phase -- sdkjs binds its own handler to #area_id, so a bubble listener runs too late to stop the caret moving. Focus stays in the document, so the highlight uses the class bootstrap's :focus rule already styles rather than moving focus. Addresses the rest of the review on PR #130: - Spreadsheet: four statements sat after an unconditional return, so _smartPickerReplace was never set and the documented "/" removal did not exist. Removed; the replacement text now comes from the shared session. - _smartPickerSlashArtificial was never assigned true, leaving the cancel-path cleanup dead in all three editors. Dropped. - SmartPickerMenu leaked a Common.UI.Menu per keystroke: Menu registers with Menu.Manager on construction and only unregisters from remove(), which hide() never calls. Every later hideAll() walked the accumulated list. - Provider icon_url reached MenuItem's unescaped <img src="<%= iconImg %>">. It is now checked against a scheme allowlist; these urls come from whichever Nextcloud apps registered a provider. - The pending-request flag was trusted for two minutes, so an unrelated insertLink in that window took the backspace path. Cut to 60s and cleared on any keystroke in the editor, which proves the host's picker is gone. - txtAnyLink could not be translated: _applyLocalization builds Common.Views.SmartPickerMenu as a plain object, which the module then replaced. It uses the _.extend pattern the controllers use, and the strings are in each editor's locale file. - Missing AGPL headers on the two files this branch added. - e.key is no longer assumed to be a string; it is absent on some synthetic and IME events, and throwing from a document keydown listener breaks typing. The trigger, the session state machine and the pending-request tracking were copy-pasted into three controllers and had already drifted -- the dead block existed only in the spreadsheet. They now live in Common.Utils.SmartPicker; only the insertion itself, which genuinely differs per editor, stays behind. Unit tests cover slashCanTrigger, sanitizeIconUrl and the pending-request tracking. They run under `node --test` and, once the harness repair lands, in test/unit-tests/common/index.html. Refs #122 Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

The setup module publishes assert/expect and stubs the ambient Common.* state, and it sat in the same flat require array as the suites that read them. RequireJS makes no promise about the order of independent siblings in one array, so nest the call and let the setup resolve first -- the same pattern the vendor globals above already use. The SmartPicker suite registration moves to #130, which is where the module and its test file come from. This branch now stands on its own: 12 tests, no failures. Assisted-by: ClaudeCode:claude-opus-5

j-base64 · 2026-08-14T15:29:11Z

Review in progress (started earlier but found some things that need additional checks before posting)

The setup module publishes assert/expect and stubs the ambient Common.* state, and it sat in the same flat require array as the suites that read them. RequireJS makes no promise about the order of independent siblings in one array, so nest the call and let the setup resolve first -- the same pattern the vendor globals above already use. The SmartPicker suite registration moves to #130, which is where the module and its test file come from. This branch now stands on its own: 12 tests, no failures. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

j-base64 · 2026-08-17T11:07:48Z

Reviewed asap as requested first

Method: AI-assisted (single + multi-angle) + manual testing.
Tested live (chrome) in documenteditor, spreadsheeteditor, and presentationeditor.

Verdict: 👀 some lookups are recommended to the Author

Note: looks like new commits landed on the PR while doing the review. I checked the diff and dont look to adress any of the points mentionned below, flaggin it in case anything else moved underneath that I missed.

I share my report below, beeing new in the codebase, i've left criticality/priority entirely to your judgment below.

📄 Full report here (click to expand)

What's working well 🙌

The core happy path is solid: typing / in an empty spot, picking "Files," and having it cleanly replace the / with a link works exactly as issue #122 describes. Confirmed this in documenteditor, spreadsheeteditor, and presentationeditor. The native caret menu, the provider list, the hand-off to Nextcloud's own picker — all of that came through cleanly.

Things I think are worth a look before merge 👀

1. 🟠HIGH - The / trigger seems to only work once per session. 🔍 Reproduced live in spreadsheeteditor and presentationeditor. Once typing / has opened the contextual menu — whether you go on to pick something and get a link, or just abandon it without picking anything (confirmed both ways, the abandon case in spreadsheeteditor) — the next / in a different cell/textbox gets typed as a literal character instead of opening the menu. It comes back to life only after reloading the document. The AI traced the possible root cause to installTrigger() in SmartPicker.js: the lastKey variable it uses to check "did whitespace precede this /" is shared across the whole page and never resets on cell/caret change or after a pick. Same shared file is used by documenteditor, so it almost certainly affects that too, though I didn't explicitly re-test the repeat-use case there. The toolbar button stayed unaffected wherever I tried it.

2. 🟡MEDIUM - Clicking away doesn't actually truly close the caret menu feature, even though it visually looks like it does. 🔍 Reproduced live in documenteditor. Typed /, clicked elsewhere in the document — even far from the popup — the menu appeared to disappear, but pressing Enter afterward, even much later, silently reopened Nextcloud's picker using whatever option was left selected, with no visual sign anything was still pending. The AI then ran a separate scripted check to explain why: the menu's container element is never actually removed from the DOM after that click, meaning its own click-outside handler never runs at all. Likely cause: the editor's own core mouse-handling probably intercepts the click before SmartPicker.js's own click-outside listener ever sees it. Not re-tested in the other two editors, but the code is shared.

3. ⚪LOW - A keystroke in the narrow gap between picking an option and Nextcloud's dialog opening leaves the trigger stranded. 🔍 Reproduced live in documenteditor. Picking an option closes the caret session immediately, before the reply arrives. Any keystroke landing in that gap — even an unrelated one, like a stray y — clears the pending state early, so when the real reply lands the cleanup step gets skipped: I saw both the / trigger and the extra keystroke survive intact, sitting right in front of the inserted link (eg "/yhttp://localhost:8082/f/11"
instead of "http://localhost:8082/f/11"). Traced to onActivity/createPending() in SmartPicker.js. Not re-tested in the other two editors, but the code is shared.

4. ⚪LOW - The menu doesn't reposition on window resize, despite its own code comment saying it should. 🔍 Reproduced live in documenteditor. Opened the caret menu, resized the browser window without typing anything further — the menu stayed anchored at its original position instead of following the resize, matching a doc-comment in SmartPickerMenu.js that describes repositioning as expected behavior that the code doesn't actually implement (no resize listener registered anywhere). Minor compared to the others above — recoverable by just retyping / — but a real, confirmed gap between the comment and the code.

Things the AI found reading the code, that I couldn't reproduce through the UI

Want to be upfront that these didn't hold up under live testing, even though the code pattern itself looks worth a defensive fix:

5. Two different entry points (the toolbar button and the caret menu) could in theory misattribute each other's replies. The AI's static review flagged that createPending() has no way to tell which reply belongs to which request, and the toolbar button never registers with it at all. I tried to test this directly — open the caret picker, then click the toolbar button before finishing — but couldn't: Nextcloud's own dialog is a focus-trapping modal, and clicking the ribbon while it's open doesn't reach the toolbar in any of the three editors.

6. A cell being edited via / doesn't get the same "don't overwrite existing content" guard as its sibling code path. Spreadsheet-specific, tested in spreadsheeteditor. The AI noticed insertLink's isCellEdited branch in the spreadsheet controller doesn't have the non-empty-cell check that the branch right next to it does (added earlier for a similar formula-corruption concern). I tried feeding it a formula-like value (=1+1) through the "Any link" picker myself, to see if it would become a live formula, but Nextcloud's own dialog wouldn't even accept non-URL input, so I couldn't get a malicious value that far. Might be worth a defensive check anyway, given how close it sits to code that was already fixed once for this exact class of bug.

7. Switching the active cell while a request is pending, in theory, could misdirect the result. Spreadsheet-specific, tested in spreadsheeteditor. This one also came from the AI's static read, not something I'd spotted myself. I tried to test whether clicking a different cell while Nextcloud's picker dialog is open could cause the link to land in the wrong place, but that dialog turns out to be a proper focus-trapping modal — neither clicks nor keyboard reach the sheet underneath while it's open, so I couldn't actually get into this state through normal use.

Smaller things

These came out of the AI's static read of the code rather than something I verified by hand:

Deleting the old btn-nc-assistant icon and repointing everything to the new btn-nc-add icon also seems to have changed the icon on the unrelated "Ask Nextcloud Assistant" context-menu item (a different, untouched feature), in all three editors. Couldn't verify this live since the Assistant app isn't part of my test setup, but it looked clear from the diff.
The new unit tests (slashCanTrigger, sanitizeIconUrl, pending-request bookkeeping) don't cover insertLink or the trigger/activity logic — which is exactly the code most of the findings above live in. There's already a very recent commit registering the SmartPicker suite in the test runner though, so it looks like this is being actively worked on 🙂
The shared trigger/session wiring that the header comment in SmartPicker.js describes as consolidated still looks copy-pasted across all three editor controllers, with some drift already visible between the three copies (different comment wording, one has an extra helper the others don't).
The four mobile/index.html files (doc/presentation/spreadsheet/visio) changed only in their hashed CSS filenames — looks like a full local build's output got committed alongside the source changes, might be worth double-checking that's intentional.
Worth asking: the Assistant-insert wiring feels like a separate feature bundled into a PR titled "Change smart picker behavior" — was that intentional, or would it make sense to split out?
AssistantInsert's retry loop (the separate "Insert into document" Assistant flow, not something I tested live) gives up after 2 seconds and calls pluginMethod_PasteHtml anyway — even though its own code comment says that exact call gets silently dropped by a re-entrancy guard — then reports success unconditionally regardless. On a slow machine, the user's generated content could vanish with no error shown.

Just sharing, not asking for changes

Also from the AI's static read, minor enough that I didn't chase these further:

sanitizeIconUrl's regex (shared by all three editors) allows a /\host/path form through as if it were same-origin, though it doesn't seem to open anything beyond what's already allowed one line above it.
txtCellNotEmpty, in spreadsheeteditor, has a hardcoded English fallback string that duplicates the locale file entry — the only string in the file doing this.
A handful of sdkjs-internal DOM ids are hardcoded in several places instead of a shared constant.
getHolder() returns a jQuery-wrapped element under an option named holderEl, which reads as if it should be a raw DOM node — small trap for whoever wires up a fourth consumer later.
Every registered provider's icon gets fetched cross-origin the instant / is typed, not just the one eventually picked — a passive "someone is composing" signal to any app with a registered provider.

Reviewed Assisted by Claude Sonnet 5

Thanks for all the work on this, happy to pair on any of the above if useful 🙌

test/unit-tests/common/index.html could not run at all. Every path in it, and in the two test files it loads, pointed into a "web-apps — копия" directory that is not in this repository, and requirejs aborts the whole run on the first 404. The suite has been dead long enough for the rest of it to rot behind that. Working outwards from there: - Paths now resolve inside this checkout. baseUrl was '../../apps/', which from test/unit-tests/common/ is test/apps/ -- a directory that has never existed. - mocha.setup() no longer passes ignoreLeaks. Removed in mocha 4, and setup() calls every key as a method, so an unknown one throws "self[opt] is not a function" before a single test registers. - chai 5 is ESM only ("type": "module", no UMD build), so requirejs' classic script tag dies on `export`. The page imports it as a module and registers it under the id the test files already use, leaving define(['chai']) and require('chai') untouched. - jquery, underscore and backbone load before the tests, and are published to window. The components read them as globals without declaring them as dependencies, and underscore's UMD build registers as AMD without leaving a global behind, so requirejs was free to evaluate a component first. - Module ids ending in .js resolve against the page rather than baseUrl, so the ids inside the test files lost their extension and the ones in index.html kept theirs. That leaves the Button suite, which was failing on its own terms: - .andSelf() was removed in jQuery 3; it is .addBack() now. - Common.UI.Scaling.currentRatio(), Common.Locale.isCurrentLanguageRtl() and Common.NotificationCenter are read at render time. The real modules pull in 'core' and the whole application bootstrap, which is the opposite of a unit test, so common.js stubs the three. 12 tests, no failures. Serve the repository over http and open test/unit-tests/common/index.html -- requirejs cannot load modules from file://, where Chrome gives every URL its own opaque origin. No product code is touched. The runner also registers the SmartPicker unit test, whose module and test file arrive with Euro-Office#130. Until that merges, requirejs 404s on it and aborts the run, so this has to land second. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…vior Refs #122 - Rename button caption and tooltip from "Smart Picker" / "Ask Nextcloud Assistant" to "Add from Nextcloud" in all three editors - On toolbar button click, insert "/" at cursor position before opening the picker, so the flow matches the Notes app slash-command UX - When the user selects a result, the inserted "/" is replaced by the hyperlink via pluginMethod_InputText backspace + add_Hyperlink - Replace sparkle icon with a plain "+" in the same stroke style as other toolbar icons (btn-inserthyperlink template) Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…t at cursor Refs #122 Works together with the Euro-Office/eurooffice-nextcloud branch "fix/issue-122-smart-picker-behavior" (host-side integration). Both branches are required for the feature to work. - Show the "Add from Nextcloud" toolbar button only while connected to a Nextcloud server: the button now defaults to hidden (visible:false) and is toggled by a new setSmartPickerAvailable host command (api.js + Gateway). The "Ask Nextcloud Assistant" context-menu item keeps its existing setAssistantAvailable gating. - Trigger the picker by typing "/" in the editor body, layout-independent (e.key). In spreadsheets the keydown listener runs in the capture phase so it also fires while a cell is being edited. - Replace the inserted/typed "/" with the selected result on success and leave it in place on cancel (new setSmartPickerCancel host command). - Restore editor focus after insert and cancel via edit:complete. - Spreadsheets: insert the link as text (cell hyperlinks are whole-cell), using isCellEdited to pick pluginMethod_InputText at the cursor while editing (re-focusing the cell input) vs asc_insertInCell on a selected cell; strip the "/" trigger and never fall into the formula path. - Use a dedicated "+" icon (btn-nc-add / btn-big-nc-add) drawn in the standard 1px toolbar stroke and drop the old btn-nc-assistant sparkle. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…s pickers Typing "/" after a space or newline opens an editor-native menu at the cursor listing what Nextcloud offers to insert. Choosing an entry hands off to Nextcloud's own picker for that provider, so only the provider choice is drawn by us. The pickers themselves carry behaviour that is invisible from the outside -- minimum search lengths, per-provider result shapes, icon resolution -- and reimplementing them means rediscovering all of it one defect at a time. The provider list is pushed in by the host rather than fetched here, because a provider is only openable where its picker component is registered, and that is a fact about the host page. The list arrives as an object: Gateway relays commands through jQuery's trigger(), which spreads an array into separate handler arguments, so a bare array would arrive as its first element. Positioning is per editor. Writer and Presentation anchor on #id_target_cursor, the caret element the drawing document moves; #area_id_parent is not the caret, since sdkjs places that IME wrapper at caretBottom plus a chain of IME offsets. The spreadsheet has no text caret unless a cell is being edited inline, so it anchors on the active cell via asc_getActiveCellCoord(), as its own popups do. "/" is accepted using the same rule as Nextcloud's editors, which configure Tribute.js with requireLeadingSpace: it fires at the start of the text or after a single whitespace character. Non-character keys are ignored when tracking the previous keystroke, or a German keyboard's Shift+7 would hide the space before it. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

"Add from Nextcloud" opened a dialog we built ourselves. Point it at Nextcloud's own Smart Picker instead, with no provider preselected so it shows the provider list. The caret menu stays as it is: that exists to keep the "/" flow inside the editor, whereas this button is the "give me the full Nextcloud picker" entry point. Deletes AssistantDialog.js, the action list we maintained in it, and its 16 locale strings per editor. Its insertion code was the part worth keeping, so that moves to Common.Utils.AssistantInsert -- including the retry around pluginMethod_PasteHtml, which is re-entrancy guarded and silently drops a second insertion. Adds an insertAssistantResult command so the host can hand back a result to insert. Nextcloud's Assistant form has no way of its own to write into our document; it accepts actionButtons, so the connector adds an "Insert into document" button whose output comes back through here and is pasted as HTML, keeping headings, lists and emphasis. Common.Assistant is now only used to record whether the Assistant is available; the request channel it wraps has no callers left. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

Removes what this branch added and no longer uses: Common.Assistant in full (its request/getTaskTypes/run/cancel had no callers, and setAvailable only stored a value nothing read), Gateway's requestAssistant and setAssistantResult, and api.js's _setAssistantResult with its export and doc line. Left alone deliberately: setAssistantAvailable exists in origin/main, and the handler that consumes it lives in DocumentHolder.js, which this branch never touched. That is what shows "Ask Nextcloud Assistant" in the context menu, so it keeps working without any of our code -- verified in the built bundle, where the command survives and ours are gone. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…mething Addresses the second review comment on PR #130. asc_getText() returns the formula-bar value, so cur + data wrote "=SUM(A1:A10)https://..." into a formula cell, which the SDK then failed to parse -- #NAME?, formula gone. The reviewer also noted values fare no better: "42" became "42https://..." and stopped being a number. Both are silent data loss. The previous guard only caught formulas. Appending, replacing and rejecting are all defensible for a non-empty cell, and the review asked for that decision to be explicit rather than implied, so: reject any non-empty cell and name the two unambiguous paths -- an empty cell, or editing the cell, where the branch above inserts at the cursor. An empty cell now inserts the link alone rather than concatenating onto ''. The string is renamed to txtCellNotEmpty since it is no longer only about formulas, and moved to keep the locale file sorted. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…nsert The "/" trigger cancelled its own keystroke. sdkjs inserts printable characters from CDocument.OnKeyPress (EnterText), which preventDefault on keydown suppresses, so "/" could not be typed after a space at all: the menu reopened on every attempt and the character never reached the document. insertLink then "removed" that "/" with pluginMethod_InputText('', '/'), which loops emulateKeyDownApi(8) once per character of textReplace -- deleting the real character before the caret, usually the space the user had just typed. The trigger no longer cancels anything, and the flow now follows Nextcloud's Text app, which drives the same interaction with @tiptap/suggestion configured as {char: '/', allowedPrefixes: [' ']}: the "/" is written to the document, each further character is written and narrows the list, space or a second "/" ends the match because tiptap's query class is [^\s/], backspacing over the trigger closes the menu, and accepting an entry replaces "/" plus the query the way tiptap's command() calls deleteRange(range). Up/Down/Enter/Tab/Escape are the only keys taken from the editor, which is why the listener moved to the capture phase -- sdkjs binds its own handler to #area_id, so a bubble listener runs too late to stop the caret moving. Focus stays in the document, so the highlight uses the class bootstrap's :focus rule already styles rather than moving focus. Addresses the rest of the review on PR #130: - Spreadsheet: four statements sat after an unconditional return, so _smartPickerReplace was never set and the documented "/" removal did not exist. Removed; the replacement text now comes from the shared session. - _smartPickerSlashArtificial was never assigned true, leaving the cancel-path cleanup dead in all three editors. Dropped. - SmartPickerMenu leaked a Common.UI.Menu per keystroke: Menu registers with Menu.Manager on construction and only unregisters from remove(), which hide() never calls. Every later hideAll() walked the accumulated list. - Provider icon_url reached MenuItem's unescaped <img src="<%= iconImg %>">. It is now checked against a scheme allowlist; these urls come from whichever Nextcloud apps registered a provider. - The pending-request flag was trusted for two minutes, so an unrelated insertLink in that window took the backspace path. Cut to 60s and cleared on any keystroke in the editor, which proves the host's picker is gone. - txtAnyLink could not be translated: _applyLocalization builds Common.Views.SmartPickerMenu as a plain object, which the module then replaced. It uses the _.extend pattern the controllers use, and the strings are in each editor's locale file. - Missing AGPL headers on the two files this branch added. - e.key is no longer assumed to be a string; it is absent on some synthetic and IME events, and throwing from a document keydown listener breaks typing. The trigger, the session state machine and the pending-request tracking were copy-pasted into three controllers and had already drifted -- the dead block existed only in the spreadsheet. They now live in Common.Utils.SmartPicker; only the insertion itself, which genuinely differs per editor, stays behind. Unit tests cover slashCanTrigger, sanitizeIconUrl and the pending-request tracking. They run under `node --test` and, once the harness repair lands, in test/unit-tests/common/index.html. Refs #122 Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

… adds The four files this branch adds carried the Ascensio System SIA header, copied from neighbouring upstream files to satisfy the "every file needs a licence header" rule. That got the licence right and the copyright holder wrong: it credited Ascensio for code they did not write, and asserted their Section 7 additional terms -- the non-infringement warranty exclusion and the CC-BY-SA clause on GUI elements -- over Nextcloud-authored work. Those are Ascensio's terms to place on Ascensio's code. Replaced with the SPDX header this repository already uses for exactly this case, in apps/spreadsheeteditor/main/app/view/CheckBoxSettingsDialog.js: /*! * SPDX-FileCopyrightText: 2026 Nextcloud GmbH or an Nextcloud affiliate company and Euro-Office contributors * SPDX-License-Identifier: AGPL-3.0-or-later */ AGPL-3.0-or-later matches LICENSE.txt, so the licence itself is unchanged. The /*! form is deliberate -- terser keeps bang comments and strips plain ones, so the notice survives into the built bundle. Affects only files this branch adds: apps/common/main/lib/util/AssistantInsert.js apps/common/main/lib/util/SmartPicker.js apps/common/main/lib/view/SmartPickerMenu.js test/unit-tests/common/main/lib/util/SmartPicker.js Upstream files this branch modifies keep their original Ascensio headers. The two SVGs it adds stay bare, as every other asset in the repository is. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

The suite file arrives with this branch, so its registration belongs here too rather than in the harness fix (#194), which now stands on its own. Note that the runner itself only works once #194 lands -- every path in this file still points at a directory that is not in the repository. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

…icker PENDING_TIMEOUT was one minute, which is inside the time an ordinary interaction takes. Picking a provider starts the record; the reply consumes it and deletes the "/query" the user typed. In between, the host's picker is a modal in front of the editor, so nothing clears the record -- and expiring there does not fail safe: insertLink still inserts the link, but consume() has already returned null, so the trigger text is left in the document beside it. Measured against a running editor, same clicks in the same order: 11 s from picking a provider to confirming -> "Hello /prohttp://..." became "Hello http://..." correct 77 s -> "Hello /prohttp://..." wrong Raised to ten minutes rather than removing the check, because the check is still the documented backstop for a host that neither answers nor cancels. Expiring is only protective if the document moved without a keystroke, which onActivity already covers for the keyboard, and the caret cannot move on its own -- so a generous value gives up nothing that was actually being guarded. The new test pins the failure: it passes at ten minutes and fails at one, and the existing stale-request test is written relative to the constant, so it keeps its meaning either way. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

pluginMethod_InputText(text, textReplace) does not match textReplace against the document. It fires textReplace.length backspaces at the caret and inserts. Verified against a running editor: with "Hello world" and the caret at the end, asking it to replace "ZZZZ" -- a string that appears nowhere -- left "Hello w". So the deletion is only correct while the caret still sits right after the text the user typed, and co-editing reaches that state without the user doing anything: a remote change shifts this user's caret, and onActivity does not fire because a remote change is not a keystroke here. The trigger is also plain text in the shared document while the picker is open, so a co-author may tidy away what looks like a typo. Raising PENDING_TIMEOUT widened that window, which is what prompted looking at it. Failure is not symmetric: a stray "/query" left behind is cosmetic and the user can delete it, while eating four characters of someone else's sentence is data loss that syncs to everyone. So triggerStillThere permits the deletion only when the word before the caret still matches what was typed, and the link is inserted either way. It compares the query rather than the whole trigger because "/" is punctuation and so a word boundary: after "Hello /pro" the word part before the caret is "pro". A bare "/" expects "", which is what a caret after punctuation gives. asc_GetCurrentWord is exported by word/api.js only, so Presentation and Spreadsheet cannot be asked and keep the previous behaviour rather than lose the feature -- exporting it there would extend the guard to them unchanged. The guard also permits the deletion if the probe throws; a diagnostic must not stop an insertion. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

Four defects in one file, all found in review of #130 and each reproduced in a running editor before it was touched. "/" only worked once per document. lastKey stands in for "the character before the caret" and nothing ever reset it, so typing "/" left it as "/" for good and slashCanTrigger refused every later trigger -- another cell, another text box, another paragraph -- until a space happened to be typed first. Anything that moves the caret elsewhere now forgets it. Undefined is the permissive value, which is the deliberate trade: this cannot read the document the way tiptap can, and a menu one Escape away beats a trigger that silently stops working. Clicking away did not end the session. sdkjs handles pointerdown on its canvas overlay and cancels it, so no compatibility mousedown is ever synthesised: measured in a running Writer, a click in the document area fires pointerdown and click on #id_viewer_overlay and no mousedown at all. The mousedown-only listener therefore never ran. The list was hidden by the editor's own hideAll(), so it looked dismissed, while Enter much later still opened the host's picker for whatever was left highlighted. Now bound to both. A keystroke between picking a provider and the host's modal taking focus cancelled the request. It is not proof the picker never opened -- the reply is still ours, and the character landed in the document behind the trigger. Within a hand-off grace such a key extends the text the reply has to delete instead, so "/f" plus a stray "y" no longer survives in front of the inserted link. triggerStillThere refused the commonest flow there is. It expected asc_GetCurrentWord(-1) to answer "" for a bare "/", on the grounds that punctuation is a word boundary. Measured against a running Writer it answers "/" -- the boundary rule holds only once a query follows -- so type "/", pick the first entry, and the "/" stayed in the document in front of the link. Alongside those: sanitizeIconUrl now rejects "/\host/path", which parses as the protocol-relative "//host/path"; the sdkjs element ids this feature reaches for are named once in one map instead of spelled out at each use; the option carrying a jQuery object is called `holder` rather than `holderEl`; and install() gathers the Gateway wiring the three editor controllers each carried a drifting copy of, so the next fix lands in all three at once. Tests cover all of it, including the two DOM-level regressions, which run in the browser harness only. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

The comment on _position() said the container has to be re-aligned "again after any resize", and nothing ever listened for one: the menu stayed where it was opened while the document reflowed underneath it. Re-reading the anchor rather than re-clamping the old point is what a resize actually calls for -- the caret moves, and the cell the spreadsheet anchors to moves with it. Verified in a running Writer: shrinking the viewport moved the caret from x=431 to x=272 and the menu followed, where before it stayed at its original x. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

The retry loop waited two seconds for sdkjs's PasteHtml re-entrancy guard to clear and then called PasteHtml anyway -- the one call its own comment says that guard drops without a word. On a slow machine the Assistant's answer disappeared and nothing said so. Falls back to PasteText, which is not behind that guard and keeps the content at the cost of the formatting, and only warns when there is no text to fall back on. The guard element id and the retry budget are named rather than inline. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

Three things the three controllers each needed, and now share. The toolbar button registered nothing with the pending record and did not clear it either, so a request the host never answered nor cancelled -- its modal closed some way that told us nothing -- was still sitting there when the button's own reply arrived, and that insertion would delete a "/query" typed minutes ago somewhere else. It clears the record first. In the spreadsheet the deletion is aimed blind: triggerStillThere needs asc_GetCurrentWord, which is exported only in word/api.js, so it always answers "cannot tell" there. The cell the trigger was typed into is recorded with the request and compared when the reply lands; a selection reached by scrolling or by a co-author's change is not where the "/" was, so nothing of ours is deleted there. The Gateway wiring itself moves into Common.Utils.SmartPicker.install: it was three copies that had already drifted in comment wording and in which guards each carried. Only the anchor, the recorded cell and insertLink stay per editor. Also: txtCellNotEmpty comes from the locale file with no hardcoded English beside it, as every other string these controllers show does; and the edit-mode branch of insertLink now says why it deliberately has no empty-cell guard -- it writes at the cursor the user put there, where the branch below appends to a value it never read. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

This branch needed a plus for the new "Add from Nextcloud" button and took it by repointing btn-nc-assistant at it and deleting the sparkle, which also changed the icon of the context-menu item for the Assistant -- a different, untouched feature -- in all three editors. btn-nc-assistant is restored and the menu item points at it again; btn-big-nc-add is the button's own icon. The 24px btn-nc-add had no user left once the menu item stopped borrowing it, so it goes rather than sit in every sprite unused. Sprites regenerated with build/scripts/deploy-sprites.js, which makes the diff against main purely additive. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

The four mobile/index.html files changed in one hashed css filename each -- a local build's output committed alongside the source changes, with nothing of this feature in it. Back to main's content. They are generated and tracked, which is what makes that happen at all; #195 stops tracking them. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>

chrip · 2026-08-19T06:40:27Z

@j-base64 @MonaAghili @moodyjmz — thanks for the thorough review, that was
really useful. I went through every point and pushed fixes for all of them.
The trigger now survives more than one use per document, clicking away really
does end the session, the resize gap is closed, and the toolbar button no
longer competes with the caret menu. A couple of the spreadsheet items I
answered in code comments rather than by changing behaviour — happy to discuss
if you disagree. Branch is rebased on main, so the old inline comments show as
outdated.

Needs the host side to work: Euro-Office/eurooffice-nextcloud#70.

Ready for another look.

chrip requested a review from a team as a code owner June 19, 2026 13:57

chrip requested review from MonaAghili, j-base64 and moodyjmz and removed request for a team June 19, 2026 13:57

MonaAghili reviewed Jun 24, 2026

View reviewed changes

chrip force-pushed the fix/issue-122-smart-picker-behavior branch from f8cbbc5 to ba48759 Compare August 12, 2026 20:22

chrip mentioned this pull request Aug 13, 2026

test(unit-tests): make the harness runnable again #194

Merged

chrip requested a review from MonaAghili August 14, 2026 06:14

chrip force-pushed the fix/issue-122-smart-picker-behavior branch from 89d25ac to 7488dce Compare August 14, 2026 15:29

chrip added 11 commits August 18, 2026 11:01

chrip mentioned this pull request Aug 18, 2026

Dropdown menu items still render icons the old way — every menu icon is blank (all editors) Euro-Office/DocumentServer#313

Open

chrip added 5 commits August 18, 2026 17:04

chrip force-pushed the fix/issue-122-smart-picker-behavior branch from 7488dce to b5ed4bd Compare August 18, 2026 15:12

chrip mentioned this pull request Aug 19, 2026

change smart picker behavior Euro-Office/eurooffice-nextcloud#70

Open

Navigation Menu

Uh oh!

Change smart picker behavior - #130

Change smart picker behavior#130
chrip wants to merge 17 commits into
mainfrom
fix/issue-122-smart-picker-behavior

chrip commented Jun 19, 2026

Uh oh!

MonaAghili Jun 24, 2026

Uh oh!

MonaAghili Jun 24, 2026 •

edited

Loading

Uh oh!

j-base64 commented Aug 14, 2026

Uh oh!

j-base64 commented Aug 17, 2026 •

edited

Loading

What's working well 🙌

Things I think are worth a look before merge 👀

Things the AI found reading the code, that I couldn't reproduce through the UI

Smaller things

Just sharing, not asking for changes

Uh oh!

chrip commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

chrip commented Jun 19, 2026

Uh oh!

MonaAghili Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

MonaAghili Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-base64 commented Aug 14, 2026

Uh oh!

j-base64 commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's working well 🙌

Things I think are worth a look before merge 👀

Things the AI found reading the code, that I couldn't reproduce through the UI

Smaller things

Just sharing, not asking for changes

Uh oh!

chrip commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MonaAghili Jun 24, 2026 •

edited

Loading

j-base64 commented Aug 17, 2026 •

edited

Loading