feat: sync: add instances and allow pushing entries#18
Open
MoustaphaCamara wants to merge 30 commits into
Open
feat: sync: add instances and allow pushing entries#18MoustaphaCamara wants to merge 30 commits into
MoustaphaCamara wants to merge 30 commits into
Conversation
Add elabftw instance page link to documentation on API
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCaptain’s log: entry timestamps migrated to modified_at; UpdateEntry persists encrypted edits; per-profile eLabFTW instances and local→remote mappings added; an HTTP client and push sync (POST/PATCH) with conflict checks were implemented, plus frontend instance management, push modal, and bindings. ChangeseLabFTW Entry Sync Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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 unit tests (beta)
Comment |
MoustaphaCamara
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
sqlite.go (1)
81-93: ⚡ Quick winCaptain, sensors detect a potential data integrity gap in the
local2remotetable.The foreign key constraint references
elabftw_instances(id)withON DELETE CASCADE, which is commendable. However, there is no foreign key fromlocal_idtoentries(id). When an entry is deleted viaDeleteEntry, orphan rows will persist inlocal2remote.Recommend adding a foreign key constraint to maintain referential integrity, Number One's assessment:
🛡️ Proposed schema amendment
CREATE TABLE IF NOT EXISTS local2remote ( id INTEGER PRIMARY KEY AUTOINCREMENT, instance INTEGER NOT NULL, remote_id INTEGER NOT NULL, local_id INTEGER NOT NULL, type TEXT NOT NULL CHECK (type IN ('experiment', 'resource', 'template')), created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), modified_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), FOREIGN KEY (instance) REFERENCES elabftw_instances(id) ON DELETE CASCADE, + FOREIGN KEY (local_id) REFERENCES entries(id) ON DELETE CASCADE, UNIQUE(instance, local_id, type), UNIQUE(instance, remote_id, type) );🤖 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 `@sqlite.go` around lines 81 - 93, The local2remote table lacks a foreign key from local_id to entries(id), so deleted entries can leave orphaned local2remote rows; update the CREATE TABLE for local2remote to add a FOREIGN KEY (local_id) REFERENCES entries(id) ON DELETE CASCADE (or the appropriate ON DELETE behavior) so that DeleteEntry will cascade and maintain referential integrity; locate the local2remote table definition in sqlite.go and add the foreign key constraint alongside the existing FOREIGN KEY (instance) definition.sync_push.go (2)
64-71: 💤 Low valueRecommend distinguishing "entry not found" from other database errors, Commander.
When the entry does not exist,
sql.ErrNoRowsis wrapped generically. Consider providing a clearer error message for the crew:🛡️ Proposed enhancement
err = db.QueryRow(` SELECT title, body FROM entries WHERE id = ? `, entryID).Scan(&encryptedTitle, &encryptedBody) + if err == sql.ErrNoRows { + return nil, fmt.Errorf("entry not found") + } if err != nil { return nil, fmt.Errorf("query entry: %w", err) }🤖 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 `@sync_push.go` around lines 64 - 71, The code handling db.QueryRow(...).Scan(&encryptedTitle, &encryptedBody) currently wraps all errors the same; modify the error handling to detect sql.ErrNoRows and return a distinct, clearer error (e.g., "entry not found" or a sentinel error) instead of the generic "query entry" wrap. Update the block around QueryRow/Scan in sync_push.go (the QueryRow + Scan call) to if errors.Is(err, sql.ErrNoRows) { return nil, fmt.Errorf("entry not found: %w", err) } else { return nil, fmt.Errorf("query entry: %w", err) } so callers can distinguish not-found from other DB failures.
24-33: 💤 Low valueSensors detect a discrepancy in entity type handling, Number One.
The
local2remoteschema permitstype IN ('experiment', 'resource', 'template'), yetelabftwEntityPathonly handlesexperimentandresource. If template push support is planned for a future mission, this is acceptable. Otherwise, consider aligning the schema constraint with the supported entity types.🤖 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 `@sync_push.go` around lines 24 - 33, The elabftwEntityPath function only maps "experiment" and "resource" but the local2remote schema allows "template" as well; either add a "template" case in elabftwEntityPath returning the correct eLabFTW path (e.g., the template endpoint) so pushes for templates are supported, or update the schema to remove "template" from the allowed types to match current implementation (modify the validation/schema that defines type IN ('experiment','resource','template') accordingly). Ensure you edit the elabftwEntityPath function (and any callers relying on it) if you add the template case, or the schema/validation logic if you remove template from allowed values.frontend/src/components/Modal.svelte (2)
14-37: ⚡ Quick winRecommend raising shields for keyboard navigators. A bare
<dialog open>with a hand-rolled backdrop is not a true modal: there is no focus trap and, more importantly, no Escape-to-close. A mouse pilot can dismiss via the backdrop, but a keyboard-only crew member is stranded. Add an Escape handler (andaria-modal='true') at minimum.🛡️ Minimal keyboard escape hatch
function stopClick(event: MouseEvent): void { event.stopPropagation(); } + + function onKeydown(event: KeyboardEvent): void { + if (event.key === 'Escape') onClose(); + } </script> + +<svelte:window onkeydown={onKeydown} /> <div class='modal-backdrop' role='presentation' onclick={onClose}> <dialog class='modal panel' aria-labelledby='modal-title' + aria-modal='true' open onclick={stopClick} >🤖 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 `@frontend/src/components/Modal.svelte` around lines 14 - 37, The dialog lacks keyboard escape and modal semantics; add aria-modal="true" to the <dialog> and wire an Escape key handler that calls the existing onClose handler (use onMount to add a keydown listener that checks for event.key === 'Escape' and invokes onClose, and onDestroy to remove the listener) and keep stopClick to prevent backdrop clicks from propagating into the dialog; this ensures the dialog element (and functions onClose and stopClick) will close on Escape and the event listener is cleaned up.
30-34: ⚖️ Poor tradeoffNumber One to Helm: replace Modal’s legacy
<slot>projection with Svelte 5 snippets (in runes/$props mode)
Infrontend/src/components/Modal.svelte, the legacy<slot />/<slot name="actions" />content-projection is deprecated in Svelte 5 in favor of snippets ({#snippet...}+{@render...}). Only make this coordinated refactor ifModal.svelteis actually using runes/$props, and then migrate the namedactionsconsumer (e.g.,InstancesPushModal.svelte) to match—otherwise legacy slots remain supported in legacy mode.🤖 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 `@frontend/src/components/Modal.svelte` around lines 30 - 34, Check whether Modal.svelte is using runes/$props; if it is, replace the legacy <slot/> and <slot name='actions'/> usage with Svelte 5 snippets: change the consumer pattern in Modal.svelte to define and render a snippet (use {`#snippet` 'default' $props} / {`#snippet` 'actions' $props}) and update callers (e.g., InstancesPushModal.svelte) to provide content via {`@render` 'default' $props} and {`@render` 'actions' $props}; if Modal.svelte is not using runes/$props, leave legacy slots unchanged. Ensure the snippet names match ('default' and 'actions') and preserve any props passed through runes/$props.
🤖 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 `@frontend/src/components/Instances/InstancesView.svelte`:
- Around line 129-146: The site URL input isn't marked required so saveInstance
runs (preventDefault) and dispatches an empty siteUrl; add client-side
validation to stop bad submits by marking the URL input (id=instanceSiteUrl)
with required and also add a guard in the saveInstance handler to check the
instanceSiteUrl value (and, if desired, require instanceApiKey when
editingInstanceId is falsy) before dispatching—if validation fails, return early
and surface a user-facing error instead of sending an empty payload to the
backend.
In `@frontend/src/components/Modal.svelte`:
- Around line 17-22: The modal uses aria-labelledby='modal-title' but no element
has that id; add the id "modal-title" to the modal heading (the <h1> that
renders {title}) or change aria-labelledby to reference the existing element id
if you prefer; update the <h1> in the Modal component so it includes
id="modal-title" to ensure the dialog is properly labeled for assistive tech.
In `@frontend/src/css/inputs.css`:
- Around line 44-72: The custom checkbox lacks a keyboard focus indicator
because the hidden input (".checkbox-row input") only controls ".checkbox-box"
for :checked and :hover; add a rule that targets the hidden input's
:focus-visible state and styles the adjacent ".checkbox-box" (e.g.,
input:focus-visible + .checkbox-box) to show an accessible focus ring/outline or
box-shadow using the existing color variables (like --orange or --orange-hover)
and appropriate outline-offset so keyboard users can see focus without altering
mouse hover behavior.
---
Nitpick comments:
In `@frontend/src/components/Modal.svelte`:
- Around line 14-37: The dialog lacks keyboard escape and modal semantics; add
aria-modal="true" to the <dialog> and wire an Escape key handler that calls the
existing onClose handler (use onMount to add a keydown listener that checks for
event.key === 'Escape' and invokes onClose, and onDestroy to remove the
listener) and keep stopClick to prevent backdrop clicks from propagating into
the dialog; this ensures the dialog element (and functions onClose and
stopClick) will close on Escape and the event listener is cleaned up.
- Around line 30-34: Check whether Modal.svelte is using runes/$props; if it is,
replace the legacy <slot/> and <slot name='actions'/> usage with Svelte 5
snippets: change the consumer pattern in Modal.svelte to define and render a
snippet (use {`#snippet` 'default' $props} / {`#snippet` 'actions' $props}) and
update callers (e.g., InstancesPushModal.svelte) to provide content via {`@render`
'default' $props} and {`@render` 'actions' $props}; if Modal.svelte is not using
runes/$props, leave legacy slots unchanged. Ensure the snippet names match
('default' and 'actions') and preserve any props passed through runes/$props.
In `@sqlite.go`:
- Around line 81-93: The local2remote table lacks a foreign key from local_id to
entries(id), so deleted entries can leave orphaned local2remote rows; update the
CREATE TABLE for local2remote to add a FOREIGN KEY (local_id) REFERENCES
entries(id) ON DELETE CASCADE (or the appropriate ON DELETE behavior) so that
DeleteEntry will cascade and maintain referential integrity; locate the
local2remote table definition in sqlite.go and add the foreign key constraint
alongside the existing FOREIGN KEY (instance) definition.
In `@sync_push.go`:
- Around line 64-71: The code handling db.QueryRow(...).Scan(&encryptedTitle,
&encryptedBody) currently wraps all errors the same; modify the error handling
to detect sql.ErrNoRows and return a distinct, clearer error (e.g., "entry not
found" or a sentinel error) instead of the generic "query entry" wrap. Update
the block around QueryRow/Scan in sync_push.go (the QueryRow + Scan call) to if
errors.Is(err, sql.ErrNoRows) { return nil, fmt.Errorf("entry not found: %w",
err) } else { return nil, fmt.Errorf("query entry: %w", err) } so callers can
distinguish not-found from other DB failures.
- Around line 24-33: The elabftwEntityPath function only maps "experiment" and
"resource" but the local2remote schema allows "template" as well; either add a
"template" case in elabftwEntityPath returning the correct eLabFTW path (e.g.,
the template endpoint) so pushes for templates are supported, or update the
schema to remove "template" from the allowed types to match current
implementation (modify the validation/schema that defines type IN
('experiment','resource','template') accordingly). Ensure you edit the
elabftwEntityPath function (and any callers relying on it) if you add the
template case, or the schema/validation logic if you remove template from
allowed values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d385cf1-fb27-46c9-81be-5e7780f00e32
📒 Files selected for processing (15)
app.goelabftw_client.gofrontend/src/components/Instances/InstancesPushModal.sveltefrontend/src/components/Instances/InstancesView.sveltefrontend/src/components/MainApp.sveltefrontend/src/components/Modal.sveltefrontend/src/components/ProfileSelector/ProfileSelectorUnlockForm.sveltefrontend/src/css/inputs.cssfrontend/src/css/main.cssfrontend/wailsjs/go/main/App.d.tsfrontend/wailsjs/go/main/App.jsfrontend/wailsjs/go/models.tssqlite.gosync_instances.gosync_push.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/utils/helpers.ts (1)
33-36: ⚡ Quick winCaptain’s security note: harden external URL opening with protocol validation.
Line 35 opens whatever string is passed. Even though today’s caller uses
https, this exported helper should defensively allow onlyhttp:/https:before callingBrowserOpenURL.Proposed patch
export function openExternalURL(url: string): void { - if (!url) return; - BrowserOpenURL(url); + const trimmed = url?.trim(); + if (!trimmed) return; + + try { + const parsed = new URL(trimmed); + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return; + BrowserOpenURL(parsed.toString()); + } catch { + return; + } }🤖 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 `@frontend/src/utils/helpers.ts` around lines 33 - 36, openExternalURL currently calls BrowserOpenURL on any non-empty string; update it to defensively validate the URL protocol first: parse the input string with the URL constructor inside a try/catch, ensure urlObj.protocol is exactly "http:" or "https:" (case-insensitive by lowercasing), and only then call BrowserOpenURL(url); otherwise return without calling BrowserOpenURL (or optionally log/reject). Reference the openExternalURL function and BrowserOpenURL symbol when implementing the protocol check.
🤖 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.
Nitpick comments:
In `@frontend/src/utils/helpers.ts`:
- Around line 33-36: openExternalURL currently calls BrowserOpenURL on any
non-empty string; update it to defensively validate the URL protocol first:
parse the input string with the URL constructor inside a try/catch, ensure
urlObj.protocol is exactly "http:" or "https:" (case-insensitive by
lowercasing), and only then call BrowserOpenURL(url); otherwise return without
calling BrowserOpenURL (or optionally log/reject). Reference the openExternalURL
function and BrowserOpenURL symbol when implementing the protocol check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7e84c1f-1995-4bd0-b001-9f33870c4c04
📒 Files selected for processing (7)
frontend/src/components/Alert.sveltefrontend/src/components/Instances/InstancesPushModal.sveltefrontend/src/components/Instances/InstancesView.sveltefrontend/src/components/MainApp.sveltefrontend/src/components/Modal.sveltefrontend/src/css/main.cssfrontend/src/utils/helpers.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/Alert.svelte
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/components/Modal.svelte
- frontend/src/css/main.css
- frontend/src/components/Instances/InstancesPushModal.svelte
- frontend/src/components/Instances/InstancesView.svelte
- frontend/src/components/MainApp.svelte
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
1 participant
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.
fix #16
Summary by CodeRabbit
New Features
UI Improvements