Skip to content

feat(overview): flag shared files with a sharing indicator - #125

Open
karlitschek wants to merge 1 commit into
mainfrom
feat/sharing-indicator
Open

feat(overview): flag shared files with a sharing indicator#125
karlitschek wants to merge 1 commit into
mainfrom
feat/sharing-indicator

Conversation

@karlitschek

Copy link
Copy Markdown
Member

What

Files in the overview couldn't be told apart by sharing state. This adds a "shared" indicator to both the grid cards and the list rows.

List Grid
share icon in the row's indicator area, beside the favourite star chip badge over the top-right of the thumbnail

How it decides "shared"

Mirrors the Files app's sharing-status definition, so the indicator means the same thing here as elsewhere in Nextcloud:

  • Outgoing — the current user has shared the file (oc:share-types is non-empty), or
  • Incoming — the file is owned by someone else and shared with them.

The overview's DAV search didn't fetch sharing state, so it now registers the oc:share-types property (the same call files_sharing makes). The nested oc:share-types payload is flattened the same way the Files app does.

Accessibility

The indicator has an accessible label — "Shared", or "Shared by {owner}" for an incoming share — used as both the ARIA name and the tooltip, so the state is never carried by icon or colour alone.

Notes

  • New isShared/isIncomingShare/outgoingShareTypes util (pure, takes currentUid explicitly like filterFiles) and a ShareIndicator component, each with a .spec.ts.
  • FileCard gains an optional overlay slot (positioned over the preview) for the grid badge.
  • Theme tokens throughout (--color-box-shadow-rgb for the badge chip shadow, etc.).
  • Source onlyjs//css/ still need a maintainer /compile.
  • Verified end-to-end on a dev instance: the search now returns <oc:share-types> and the indicator shows on a shared file (grid and list), owner-only files stay unmarked.

🤖 Generated with Claude Code

Files in the overview could not be told apart by sharing state. Show a
"shared" indicator on both the grid cards and the list rows.

A file counts as shared when the current user has shared it out (a
non-empty oc:share-types) or it is owned by someone else and shared with
them — the same definition the Files app uses. The overview did not fetch
sharing state, so register the oc:share-types DAV property, as
files_sharing does, to include it in the search results.

The indicator carries an accessible label ("Shared", or "Shared by
{owner}" for an incoming share) so the state is never conveyed by icon or
colour alone. In the grid it uses a new FileCard overlay slot over the
thumbnail; in the list it joins the favourite star in the indicator slot.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a sharing-state indicator to the Office overview so users can distinguish shared vs non-shared files consistently with Nextcloud Files (via oc:share-types and ownership), rendering the indicator in both grid cards (overlay badge) and list rows (row indicator area), with accessible labeling.

Changes:

  • Register oc:share-types for the overview DAV search and add a sharing-state utility (isShared / isIncomingShare / outgoingShareTypes).
  • Introduce a ShareIndicator component (with ARIA label + tooltip) and integrate it into grid and list views.
  • Extend FileCard with an optional overlay slot to support badge-style overlays over thumbnails.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/views/OfficeOverview.vue Renders ShareIndicator in grid overlay and list indicator area; adds indicator layout styling.
src/utils/fileSharing.ts New pure helpers to normalize oc:share-types and determine shared/incoming share state.
src/utils/fileSharing.spec.ts Adds unit tests for the new sharing helpers.
src/test-utils/fixtures.ts Extends makeNode fixtures to include nested DAV-shaped share types.
src/services/officeFiles.ts Registers oc:share-types DAV property so the overview search returns sharing state.
src/services/officeFiles.spec.ts Verifies DAV property registration occurs at module load.
src/components/ShareIndicator.vue New share icon indicator with accessible label/tooltip for outgoing vs incoming shares.
src/components/ShareIndicator.spec.ts Unit tests for rendering and labeling behavior of ShareIndicator.
src/components/FileCard.vue Adds overlay slot and overlay positioning/chip styling over the preview thumbnail.
src/components/FileCard.spec.ts Tests overlay slot rendering behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +15 to +21
it('flattens the nested DAV shape for a single share type', () => {
expect(outgoingShareTypes(makeNode({ shareTypes: [3] }))).toEqual([3])
})

it('flattens the nested DAV shape for several share types', () => {
expect(outgoingShareTypes(makeNode({ shareTypes: [0, 3] }))).toEqual([0, 3])
})
<button type="button" class="file-card" @click="$emit('click', $event)">
<div class="file-card__preview">
<slot name="preview" />
<span v-if="$slots.overlay" class="file-card__overlay">
@moodyjmz

Copy link
Copy Markdown
Contributor

TL;DR: Solid feature, good test coverage on the new utils/component. Two real accessibility defects in the label, plus a reuse-before-you-write miss and the recurring untested-wiring gap. Proposed fixes below.

Findings and proposed fixes

1. Double-escaped owner names in the accessible label (medium-high)

t() defaults to escape: true, so a placeholder value goes through escape-html before substitution. Reproduced directly:

translate('office', 'Shared by {owner}', { owner: "O'Brien & <b>x</b>" })
→ "Shared by O&#39;Brien &amp; &lt;b&gt;x&lt;/b&gt;"

That string then gets bound via :aria-label/:title, which Vue compiles to setAttribute() — a sink that never HTML-decodes. So any owner display name containing &, <, >, ", or ' produces a mangled, literal-entity accessible name and tooltip — a screen reader announces the entity text, not the real name. Not exploitable (no injection risk, setAttribute can't execute markup), but it does directly undercut the PR's own stated goal that this label is the accessible name.

Fix — the escaping is redundant on a DOM-attribute sink, so opt the placeholder out:

return owner
  ? t('office', 'Shared by {owner}', { owner: { value: owner, escape: false } })
  : t('office', 'Shared')

Worth a regression test with a name containing a special character, since the current fixture ('Bob') can't catch this — and worth knowing this same blind spot exists for every other translated placeholder in the repo, since vitest.setup.ts's translate() stub doesn't escape either.

2. :title duplicates :aria-label (medium)

Per the accessible-name/description computation, aria-label wins the name and title falls through to the description — so at default verbosity a screen reader announces "Shared. Shared." (or "Shared by Bob. Shared by Bob."). title also never fires on keyboard focus, so it isn't adding a keyboard-accessible tooltip either. Suggest dropping :title entirely, or moving to a v-tooltip-style pattern if a visible-on-hover tooltip is wanted independently of the accessible name.

3. Reuse missed: NcIconSvgWrapper already does this (low-medium)

NcIconSvgWrapper accepts a name prop that sets the accessible name itself and drops aria-hidden when provided (confirmed in its compiled output). The hand-rolled role="img" + aria-label wrapper span duplicates behavior the component already has:

<NcIconSvgWrapper :path="mdiShareVariant" :size="size" :name="label" />

removes the need for the outer <span> and its ARIA attributes entirely.

4. OfficeOverview.vue's wiring is untested (medium)

Both new call sites — the grid overlay slot and the list indicator slot — have no coverage in OfficeOverview.spec.ts, which this commit doesn't touch. Worth a test asserting ShareIndicator actually renders in both views for a shared file, and doesn't for an unshared one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants