fix(note-blocks): address multi-agent review findings

Closes the 14 findings from the branch review:

- shared calendar (read-only view) was missing two fixes that the editor
  calendar got: mirror the `fetchRange` getter so events on the month
  grid's adjacent-month padding cells load, and normalize a legacy/unknown
  stored `view` (e.g. "week") to "month" so it never renders blank
- make the "first text block" selection deterministic with a
  `position ASC, id ASC` tiebreak everywhere it is queried, closing a
  Postgres description-wipe path when two blocks share a position
- query params are edited as a stable {id,key,value} row model so
  renaming a key no longer destroys the value input mid-edit, and new
  param keys are guaranteed unique
- refresh the block returned by CreateBlock after a post-commit rebalance
  so API/CLI consumers don't get a stale position
- clear _pendingUpdates on debounced-save success and on delete so the
  unload flush can't re-PUT already-saved or deleted blocks
- strip Unicode format/zero-width chars (\p{Cf}) in the markdown link
  scheme check so they can't split a dangerous scheme past the allowlist

Tests: ICS scheme allowlist + redirect-to-file:// SSRF block, auto-rebalance
order preservation, tied-position description determinism, _generateBetween
JS/Go parity, and Unicode link-scheme XSS cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: egeozcan

application_context/basic_entity_context.go

@@ -67,7 +67,7 @@ func (w *EntityWriter[T]) UpdateDescription(id uint, description string) error {
 			if err := tx.Table("note_blocks").
 				Select("id").
 				Where("note_id = ? AND type = ?", id, "text").
-				Order("position ASC").
+				Order("position ASC, id ASC").
 				Limit(1).
 				Find(&blocks).Error; err != nil {
 				return err

application_context/block_bugfix_test.go

@@ -2,6 +2,8 @@ package application_context
 
 import (
 	"encoding/json"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -214,6 +216,76 @@ func TestCreateBlock_AutoRebalancesLongPositions(t *testing.T) {
 	}
 }
 
+// Review #13: auto-rebalance triggered by a create must PRESERVE block order, not
+// just shorten positions. Uses 3 ordered blocks so order is actually verifiable.
+func TestCreateBlock_AutoRebalancePreservesOrder(t *testing.T) {
+	ctx := createBlockTestContext(t)
+	note, err := createTestNote(ctx, "n")
+	require.NoError(t, err)
+
+	a, err := ctx.CreateBlock(&query_models.NoteBlockEditor{NoteID: note.ID, Type: "divider", Position: "d", Content: json.RawMessage(`{}`)})
+	require.NoError(t, err)
+	b, err := ctx.CreateBlock(&query_models.NoteBlockEditor{NoteID: note.ID, Type: "divider", Position: "h", Content: json.RawMessage(`{}`)})
+	require.NoError(t, err)
+	// "zzzzzzzzzzzz" (12 chars) sorts last and exceeds the rebalance threshold,
+	// so creating it triggers the auto-rebalance over all three blocks.
+	c, err := ctx.CreateBlock(&query_models.NoteBlockEditor{NoteID: note.ID, Type: "divider", Position: "zzzzzzzzzzzz", Content: json.RawMessage(`{}`)})
+	require.NoError(t, err)
+
+	blocks, err := ctx.GetBlocksForNote(note.ID)
+	require.NoError(t, err)
+	require.Len(t, blocks, 3)
+	assert.Equal(t, a.ID, blocks[0].ID, "order A,B,C must be preserved through auto-rebalance")
+	assert.Equal(t, b.ID, blocks[1].ID)
+	assert.Equal(t, c.ID, blocks[2].ID)
+	for _, bl := range blocks {
+		assert.LessOrEqual(t, len(bl.Position), 8, "positions must be rebalanced under the threshold")
+	}
+}
+
+// Review #3: two text blocks at the SAME position must resolve the first text
+// block deterministically (position ASC, id ASC), so the older non-empty block
+// keeps owning the description instead of an empty later block wiping it.
+func TestCreateBlock_TiedPositionKeepsDescriptionDeterministic(t *testing.T) {
+	ctx := createBlockTestContext(t)
+	note, err := createTestNote(ctx, "n")
+	require.NoError(t, err)
+
+	_, err = ctx.CreateBlock(&query_models.NoteBlockEditor{NoteID: note.ID, Type: "text", Position: "d", Content: json.RawMessage(`{"text":"Hello"}`)})
+	require.NoError(t, err)
+	_, err = ctx.CreateBlock(&query_models.NoteBlockEditor{NoteID: note.ID, Type: "text", Position: "d", Content: json.RawMessage(`{"text":""}`)})
+	require.NoError(t, err)
+
+	var n models.Note
+	require.NoError(t, ctx.db.First(&n, note.ID).Error)
+	assert.Equal(t, "Hello", n.Description,
+		"with the id-ASC tiebreak the older 'Hello' block is first, so the description survives")
+}
+
+// Review #6: the ICS scheme allowlist accepts http(s) and rejects everything else.
+func TestICS_AllowedScheme(t *testing.T) {
+	for _, ok := range []string{"http://x/c.ics", "https://x/c.ics", "HTTPS://X/C.ics"} {
+		assert.True(t, allowedICSScheme(ok), "scheme should be allowed: %s", ok)
+	}
+	for _, bad := range []string{"file:///etc/passwd", "gopher://x/", "ftp://x/c.ics", "", "javascript:alert(1)", "not a url"} {
+		assert.False(t, allowedICSScheme(bad), "scheme should be rejected: %s", bad)
+	}
+}
+
+// Review #6: the runtime fetch must re-validate redirect targets, so an http(s)
+// URL that 302-redirects to a non-http(s) scheme is blocked (SSRF defense).
+func TestFetchICS_RejectsRedirectToNonHttpScheme(t *testing.T) {
+	ctx := createBlockTestContext(t)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "file:///etc/passwd", http.StatusFound)
+	}))
+	defer srv.Close()
+
+	_, _, err := ctx.fetchAndCacheICS(srv.URL, nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "redirect to non-http(s) URL blocked")
+}
+
 // B4: CreateBlock must also reject explicit null content (non-empty "null").
 func TestCreateBlock_RejectsNullContent(t *testing.T) {
 	ctx := createBlockTestContext(t)

application_context/block_context.go

@@ -132,7 +132,11 @@ func (ctx *MahresourcesContext) CreateBlock(editor *query_models.NoteBlockEditor
 		}
 	}
 
-	ctx.maybeRebalanceBlockPositions(editor.NoteID)
+	if ctx.maybeRebalanceBlockPositions(editor.NoteID) {
+		// A rebalance rewrote every position; refresh the returned block so its
+		// Position reflects the persisted value (API/CLI consumers read it).
+		ctx.db.First(&block, block.ID)
+	}
 
 	return &block, nil
 }
@@ -157,7 +161,7 @@ func (ctx *MahresourcesContext) GetBlocksForNote(noteID uint) ([]models.NoteBloc
 		return []models.NoteBlock{}, nil
 	}
 	var blocks []models.NoteBlock
-	err := ctx.db.Where("note_id = ?", noteID).Order("position ASC").Find(&blocks).Error
+	err := ctx.db.Where("note_id = ?", noteID).Order("position ASC, id ASC").Find(&blocks).Error
 	return blocks, err
 }
 
@@ -450,7 +454,7 @@ func seedFirstTextBlockFromDescription(db *gorm.DB, editor *query_models.NoteBlo
 func syncFirstTextBlockToDescriptionTx(tx *gorm.DB, noteID uint) error {
 	var blocks []models.NoteBlock
 	if err := tx.Where("note_id = ? AND type = ?", noteID, "text").
-		Order("position ASC").Limit(1).Find(&blocks).Error; err != nil {
+		Order("position ASC, id ASC").Limit(1).Find(&blocks).Error; err != nil {
 		return err
 	}
 
@@ -476,21 +480,24 @@ const rebalanceThreshold = 8
 
 // maybeRebalanceBlockPositions auto-rebalances a note's block positions when any
 // position string has grown past rebalanceThreshold. Best-effort and called
-// after a create/reorder commits; failures are logged, not propagated.
-func (ctx *MahresourcesContext) maybeRebalanceBlockPositions(noteID uint) {
+// after a create/reorder commits; failures are logged, not propagated. Returns
+// true if a rebalance actually ran (so callers can refresh stale in-memory rows).
+func (ctx *MahresourcesContext) maybeRebalanceBlockPositions(noteID uint) bool {
 	var positions []string
 	if err := ctx.db.Model(&models.NoteBlock{}).
 		Where("note_id = ?", noteID).
 		Pluck("position", &positions).Error; err != nil {
 		log.Printf("Warning: failed to read positions for rebalance check on note %d: %v", noteID, err)
-		return
+		return false
 	}
 	if !lib.NeedsRebalancing(positions, rebalanceThreshold) {
-		return
+		return false
 	}
 	if err := ctx.RebalanceBlockPositions(noteID); err != nil {
 		log.Printf("Warning: auto-rebalance failed for note %d: %v", noteID, err)
+		return false
 	}
+	return true
 }
 
 // RebalanceBlockPositions normalizes block positions for a note to prevent position string growth.
@@ -501,7 +508,7 @@ func (ctx *MahresourcesContext) RebalanceBlockPositions(noteID uint) error {
 		return gorm.ErrRecordNotFound
 	}
 	var blocks []models.NoteBlock
-	if err := ctx.db.Where("note_id = ?", noteID).Order("position ASC").Find(&blocks).Error; err != nil {
+	if err := ctx.db.Where("note_id = ?", noteID).Order("position ASC, id ASC").Find(&blocks).Error; err != nil {
 		return err
 	}
 

application_context/export_context.go

@@ -1494,7 +1494,7 @@ func (ctx *MahresourcesContext) loadNotePayload(id uint, plan *exportPlan) (*arc
 		Preload("Groups").
 		Preload("NoteType").
 		Preload("Blocks", func(db *gorm.DB) *gorm.DB {
-			return db.Order("position ASC")
+			return db.Order("position ASC, id ASC")
 		}).
 		First(&n, id).Error; err != nil {
 		return nil, err

application_context/note_context.go

@@ -189,7 +189,7 @@ func (ctx *MahresourcesContext) CreateOrUpdateNote(noteQuery *query_models.NoteE
 	// Sync description to first text block if blocks exist (backward compatibility)
 	if noteQuery.ID != 0 {
 		var blocks []models.NoteBlock
-		if err := tx.Where("note_id = ? AND type = ?", note.ID, "text").Order("position ASC").Limit(1).Find(&blocks).Error; err == nil && len(blocks) > 0 {
+		if err := tx.Where("note_id = ? AND type = ?", note.ID, "text").Order("position ASC, id ASC").Limit(1).Find(&blocks).Error; err == nil && len(blocks) > 0 {
 			content, _ := json.Marshal(map[string]string{"text": noteQuery.Description})
 			tx.Model(&blocks[0]).Update("content", content)
 		}
@@ -227,7 +227,7 @@ func (ctx *MahresourcesContext) GetNote(id uint) (*models.Note, error) {
 
 	return &note, ctx.db.Preload(clause.Associations).
 		Preload("Blocks", func(db *gorm.DB) *gorm.DB {
-			return db.Order("position ASC")
+			return db.Order("position ASC, id ASC")
 		}).
 		First(&note, id).Error
 }
@@ -379,7 +379,7 @@ func (ctx *MahresourcesContext) GetNoteByShareToken(token string) (*models.Note,
 	var note models.Note
 	err := ctx.db.
 		Preload("Blocks", func(db *gorm.DB) *gorm.DB {
-			return db.Order("position ASC")
+			return db.Order("position ASC, id ASC")
 		}).
 		Preload("Resources").
 		Preload("NoteType").

public/dist/main.js

@@ -0,0 +1,73 @@
+/**
+ * Guards the JS positionBetween/_generateBetween port against drift from the Go
+ * reference (lib/position.go). The branch ported two Go branches into
+ * _generateBetween to fix an ordering bug (positionBetween("a","aa") used to
+ * return "aan" > "aa"). Same extract-and-eval approach as the renderMarkdown
+ * test, since these are methods on the Alpine data factory.
+ */
+import { readFileSync } from 'fs';
+import { resolve, dirname } from 'path';
+import { fileURLToPath } from 'url';
+import { describe, it, expect } from 'vitest';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+const src = readFileSync(resolve(__dirname, './blockEditor.js'), 'utf-8');
+
+function extractMethod(name: string): string {
+  const re = new RegExp(name + '\\(before, after\\)\\s*\\{[\\s\\S]*?\\n\\s{4}\\},');
+  const m = src.match(re);
+  if (!m) throw new Error('could not extract ' + name + ' from blockEditor.js');
+  return m[0]
+    .replace(name + '(before, after)', 'function ' + name + '(before, after)')
+    .replace(/,\s*$/, '')
+    .replace(/this\._generateBetween/g, '_generateBetween');
+}
+
+const positionBetween = new Function(
+  `const MIN_CHAR = 'a'.charCodeAt(0);
+   const MAX_CHAR = 'z'.charCodeAt(0);
+   ${extractMethod('_generateBetween')}
+   ${extractMethod('positionBetween')}
+   return positionBetween;`
+)() as (before: string, after: string) => string;
+
+describe('positionBetween / _generateBetween — JS/Go parity', () => {
+  // Values verified against Go lib.PositionBetween.
+  it('matches Go for the regressed adjacent pair ("a","aa") -> "aa" (not "aan")', () => {
+    expect(positionBetween('a', 'aa')).toBe('aa');
+  });
+  it('matches Go for ("b","ba") -> "ba"', () => {
+    expect(positionBetween('b', 'ba')).toBe('ba');
+  });
+  it('matches Go for ("a","b") -> "an"', () => {
+    expect(positionBetween('a', 'b')).toBe('an');
+  });
+  it('returns "n" for the empty/empty (first block) case', () => {
+    expect(positionBetween('', '')).toBe('n');
+  });
+
+  it('keeps before < result, and result <= after (< after when not adjacent)', () => {
+    const pairs: [string, string][] = [
+      ['a', 'b'], ['a', 'aa'], ['b', 'ba'], ['n', 'na'], ['a', 'c'],
+      ['aa', 'ab'], ['m', 'n'], ['', 'n'], ['n', ''], ['a', ''],
+    ];
+    for (const [before, after] of pairs) {
+      const r = positionBetween(before, after);
+      expect(r > before).toBe(true);
+      // after === '' is an open upper bound ("past z"), so skip that check.
+      if (after !== '') expect(r <= after).toBe(true);
+    }
+  });
+
+  it('stays strictly ordered across repeated end-appends (the default add path)', () => {
+    let last = 'n';
+    let prev = '';
+    for (let i = 0; i < 50; i++) {
+      const next = positionBetween(last, '');
+      expect(next > last).toBe(true);
+      prev = last;
+      last = next;
+    }
+    expect(prev).not.toBe(last);
+  });
+});

src/components/blockEditor-position.test.ts

@@ -124,6 +124,18 @@ describe('XSS: link scheme sanitization (control-char bypass)', () => {
     expect(renderMarkdown('[x](vbscript:msgbox)')).not.toMatch(/<a\s/);
   });
 
+  it('blocks javascript: split by a zero-width space (U+200B)', () => {
+    expect(renderMarkdown('[x](java​script:foo)')).not.toMatch(/<a\s/);
+  });
+
+  it('blocks javascript: split by a soft hyphen (U+00AD)', () => {
+    expect(renderMarkdown('[x](java­script:foo)')).not.toMatch(/<a\s/);
+  });
+
+  it('blocks javascript: split by a word joiner (U+2060)', () => {
+    expect(renderMarkdown('[x](java⁠script:foo)')).not.toMatch(/<a\s/);
+  });
+
   it('still renders safe http/https/mailto links', () => {
     expect(renderMarkdown('[x](https://example.com)')).toMatch(/<a href="https:\/\/example\.com"/);
     expect(renderMarkdown('[x](http://example.com)')).toMatch(/<a href="http:\/\/example\.com"/);

src/components/blockEditor-render-markdown.test.ts

@@ -77,9 +77,11 @@ export function blockEditor(noteId, initialBlocks = []) {
           // A denylist on the raw string (.trim().startsWith('javascript:')) is
           // bypassable: browsers strip whitespace/control chars embedded in a URL
           // scheme, so "java\tscript:" resolves to "javascript:". Strip all
-          // whitespace + ASCII control chars first, then ALLOWLIST safe schemes
-          // (or relative URLs) rather than denylisting dangerous ones.
-          const cleaned = href.replace(/[\s\x00-\x1f]+/g, '').toLowerCase();
+          // whitespace, ASCII control chars, AND Unicode format/zero-width chars
+          // (\p{Cf}: U+200B, U+00AD, ... which JS \s does NOT cover) first, then
+          // ALLOWLIST safe schemes (or relative URLs) rather than denylisting.
+          const STRIP = /[\s\x00-\x1f]|\p{Cf}/gu;
+          const cleaned = href.replace(STRIP, '').toLowerCase();
           const hasScheme = /^[a-z][a-z0-9+.\-]*:/.test(cleaned);
           const safe = !hasScheme
               || cleaned.startsWith('http:')
@@ -90,7 +92,9 @@ export function blockEditor(noteId, initialBlocks = []) {
               return text;
           }
           // Defang control chars that survived in the original href so they
-          // cannot reconstitute a dangerous scheme in the attribute. (Quotes are
+          // cannot reconstitute a dangerous scheme in the attribute. Only ASCII
+          // control chars are browser-stripped from schemes; we deliberately keep
+          // regular whitespace here so legitimate URLs aren't mangled. (Quotes are
           // already HTML-escaped above, blocking attribute breakout.)
           const safeHref = href.replace(/[\x00-\x1f]/g, '');
           return `<a href="${safeHref}" class="text-blue-600 hover:underline" target="_blank" rel="noopener">${text}</a>`;
@@ -346,6 +350,9 @@ export function blockEditor(noteId, initialBlocks = []) {
           this.blocks[idx] = { ...this.blocks[idx], content: updated.content, updatedAt: updated.updatedAt };
         }
         delete this._prevContent[blockId];
+        // Clear the pending entry unless a newer edit superseded this save, so
+        // the unload flush doesn't redundantly re-PUT already-persisted content.
+        if (this._pendingUpdates[blockId] === content) delete this._pendingUpdates[blockId];
       } catch (err) {
         // Roll back the optimistic content so the UI reflects server truth and
         // the user is not misled into thinking an unsaved edit persisted.
@@ -398,6 +405,10 @@ export function blockEditor(noteId, initialBlocks = []) {
           throw new Error(errorData.error || `Failed to delete block: ${res.status}`);
         }
         this.blocks = this.blocks.filter(b => b.id !== blockId);
+        // Drop any pending optimistic state so the unload flush can never re-PUT
+        // a now-deleted block (which the server would reject).
+        delete this._pendingUpdates[blockId];
+        delete this._prevContent[blockId];
         this.announce('Block deleted');
         // Move focus to a sensible neighbor (the block now occupying the deleted
         // slot, else the add-block trigger) so keyboard users are not stranded.